From 5ef6c8b1d25e15388ee8527e2d63e825b2faf8ee Mon Sep 17 00:00:00 2001 From: Oliver Kieselbach Date: Tue, 5 May 2026 12:12:54 +0200 Subject: [PATCH] fix(mcp): MCP server security & capability audit (Phases A-E) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Critical: GET /api/metrics/mcp-usage/user/{userId} (TenantAdminOrGA, no TenantScoping) handed the oid to the repo without checking that returned records belong to the caller's tenant. A tenant admin from tenant A could read MCP usage records of any oid — including oids in tenant B. McpUsageMetricsFunction.GetUserUsage now blocks 403 CrossTenantAccessDenied when records are tenant-foreign and caller is not a Global Admin. The pure decision logic lives in UsageCrossTenantGuard for unit-testability (9 tests covering own/foreign/mixed/GA/empty/null/case-insensitive/empty-tid paths). Also: - MCP get_api_usage tool was calling /api/metrics/mcp-usage/{userId} (without /user/), which is unregistered and returned 404 — corrected. - list_blocked_devices description honestly states the endpoint is GA-only on both variants (catalog policy, no change to the gate). - days clamp [1, 365] added to /metrics/{app, geographic, geographic/sessions} for parity with /metrics/summary, /metrics/usage, /metrics/platform. - get_geographic_metrics, get_geographic_sessions zod schemas now constrain days to int().min(1).max(365) — same pattern as the other tools. - search/sessions, search/sessions-by-event, search/sessions-by-cve switched limit handling from Math.Min(lim, 100) to Math.Clamp(lim, 1, 100); the old form let a negative ?limit=-5 reach the repo. Audit static analysis confirmed all other tenant-scoped endpoints route tenantId through TenantHelper.GetTenantId(req) (JWT-only, ignores query), so ?tenantId= is silently dropped. Live-verified with an Operator token: 14 cross-tenant query probes returned only the caller's tenant, 13 GlobalAdminOnly routes returned 403. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../UsageCrossTenantGuardTests.cs | 126 ++++++++++++++++++ .../Metrics/GetAppMetricsFunction.cs | 5 +- .../GetGeographicLocationSessionsFunction.cs | 2 + .../Metrics/GetGeographicMetricsFunction.cs | 2 + .../Metrics/McpUsageMetricsFunction.cs | 30 ++++- .../Metrics/UsageCrossTenantGuard.cs | 37 +++++ .../Sessions/SearchSessionsByCveFunction.cs | 2 +- .../Sessions/SearchSessionsByEventFunction.cs | 2 +- .../Sessions/SearchSessionsFunction.cs | 2 +- .../autopilot-monitor-mcp/src/tools/admin.ts | 11 +- .../src/tools/sessions.ts | 5 +- 11 files changed, 213 insertions(+), 11 deletions(-) create mode 100644 src/Backend/AutopilotMonitor.Functions.Tests/UsageCrossTenantGuardTests.cs create mode 100644 src/Backend/AutopilotMonitor.Functions/Functions/Metrics/UsageCrossTenantGuard.cs diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/UsageCrossTenantGuardTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/UsageCrossTenantGuardTests.cs new file mode 100644 index 00000000..7f4d4896 --- /dev/null +++ b/src/Backend/AutopilotMonitor.Functions.Tests/UsageCrossTenantGuardTests.cs @@ -0,0 +1,126 @@ +using AutopilotMonitor.Functions.Functions.Metrics; +using AutopilotMonitor.Shared.DataAccess; + +namespace AutopilotMonitor.Functions.Tests; + +/// +/// Unit tests for the cross-tenant guard on GET /api/metrics/mcp-usage/user/{userId}. +/// +/// Background: the route is catalog-policy TenantAdminOrGA with no TenantScoping +/// (the path parameter is an Azure AD object id, not a tenant id). Without this guard, a tenant +/// admin from tenant A could read MCP usage records of any oid — including oids in tenant B — +/// because the function blindly hands the oid to IUserUsageRepository.GetUsageByUserAsync. +/// +public class UsageCrossTenantGuardTests +{ + private const string TenantA = "00000000-0000-0000-0000-aaaaaaaaaaaa"; + private const string TenantB = "00000000-0000-0000-0000-bbbbbbbbbbbb"; + + private static UserUsageRecord Rec(string tid, string endpoint = "ep") => new() + { + UserId = "oid-1", + UserPrincipalName = "alice@contoso.com", + TenantId = tid, + Endpoint = endpoint, + Date = "20260505", + RequestCount = 1, + }; + + [Fact] + public void OwnTenantOnly_NonGA_Allowed() + { + var records = new[] { Rec(TenantA), Rec(TenantA, "ep2") }; + + var blocked = UsageCrossTenantGuard.IsForeignTenantAccess(records, TenantA, isGlobalAdmin: false); + + Assert.False(blocked); + } + + [Fact] + public void ForeignTenantOnly_NonGA_Blocked() + { + var records = new[] { Rec(TenantB), Rec(TenantB, "ep2") }; + + var blocked = UsageCrossTenantGuard.IsForeignTenantAccess(records, TenantA, isGlobalAdmin: false); + + Assert.True(blocked); + } + + [Fact] + public void MixedTenants_NonGA_Blocked() + { + // Even one foreign record must trip the guard — a tenant admin must not see another tenant's row. + var records = new[] { Rec(TenantA), Rec(TenantB) }; + + var blocked = UsageCrossTenantGuard.IsForeignTenantAccess(records, TenantA, isGlobalAdmin: false); + + Assert.True(blocked); + } + + [Fact] + public void ForeignTenant_GlobalAdmin_Allowed() + { + // GA can see usage across all tenants. + var records = new[] { Rec(TenantB) }; + + var blocked = UsageCrossTenantGuard.IsForeignTenantAccess(records, TenantA, isGlobalAdmin: true); + + Assert.False(blocked); + } + + [Fact] + public void EmptyRecords_NonGA_Allowed() + { + // No records → caller learns nothing; safe to return 200 with empty list. + var blocked = UsageCrossTenantGuard.IsForeignTenantAccess( + Array.Empty(), TenantA, isGlobalAdmin: false); + + Assert.False(blocked); + } + + [Fact] + public void NullRecords_DoesNotThrow() + { + var blocked = UsageCrossTenantGuard.IsForeignTenantAccess(null!, TenantA, isGlobalAdmin: false); + Assert.False(blocked); + } + + [Fact] + public void RecordWithEmptyTenantId_Ignored() + { + // Defensive: legacy records without TenantId must not be treated as a leak signal + // (they could belong to anyone). Other records still drive the decision. + var records = new[] + { + Rec(""), // unknown tenant — ignored + Rec(TenantA), // own tenant + }; + + var blocked = UsageCrossTenantGuard.IsForeignTenantAccess(records, TenantA, isGlobalAdmin: false); + + Assert.False(blocked); + } + + [Fact] + public void CallerTenantIdEmpty_NotBlocked() + { + // Anonymous / device path scenarios shouldn't reach this code — but if they do, + // we don't synthesize a 403 from missing claims; let the catalog policy handle auth. + var records = new[] { Rec(TenantA) }; + + var blocked = UsageCrossTenantGuard.IsForeignTenantAccess(records, "", isGlobalAdmin: false); + + Assert.False(blocked); + } + + [Fact] + public void TenantIdComparison_IsCaseInsensitive() + { + // Azure AD tids are GUIDs; some sources upper-case them. Comparison must not flag those as foreign. + var records = new[] { Rec(TenantA.ToUpperInvariant()) }; + + var blocked = UsageCrossTenantGuard.IsForeignTenantAccess(records, TenantA, isGlobalAdmin: false); + + Assert.False(blocked); + } +} diff --git a/src/Backend/AutopilotMonitor.Functions/Functions/Metrics/GetAppMetricsFunction.cs b/src/Backend/AutopilotMonitor.Functions/Functions/Metrics/GetAppMetricsFunction.cs index bd15970c..6dbb18da 100644 --- a/src/Backend/AutopilotMonitor.Functions/Functions/Metrics/GetAppMetricsFunction.cs +++ b/src/Backend/AutopilotMonitor.Functions/Functions/Metrics/GetAppMetricsFunction.cs @@ -28,12 +28,15 @@ namespace AutopilotMonitor.Functions.Functions.Metrics var tenantId = TenantHelper.GetTenantId(req); _logger.LogInformation($"Fetching app metrics for tenant {tenantId}"); - // Optional time filter: ?days=7 (default: 30) + // Optional time filter: ?days=7 (default: 30, clamped to [1, 365] for consistency + // with metrics/summary, metrics/usage, and metrics/platform). var query = System.Web.HttpUtility.ParseQueryString(req.Url.Query); var daysParam = query["days"]; int days = 30; if (!string.IsNullOrEmpty(daysParam) && int.TryParse(daysParam, out var parsedDays) && parsedDays > 0) days = parsedDays; + if (days < 1) days = 1; + if (days > 365) days = 365; var cutoff = DateTime.UtcNow.AddDays(-days); diff --git a/src/Backend/AutopilotMonitor.Functions/Functions/Metrics/GetGeographicLocationSessionsFunction.cs b/src/Backend/AutopilotMonitor.Functions/Functions/Metrics/GetGeographicLocationSessionsFunction.cs index 51054d2f..7e071625 100644 --- a/src/Backend/AutopilotMonitor.Functions/Functions/Metrics/GetGeographicLocationSessionsFunction.cs +++ b/src/Backend/AutopilotMonitor.Functions/Functions/Metrics/GetGeographicLocationSessionsFunction.cs @@ -46,6 +46,8 @@ namespace AutopilotMonitor.Functions.Functions.Metrics int days = 30; if (!string.IsNullOrEmpty(daysParam) && int.TryParse(daysParam, out var parsedDays) && parsedDays > 0) days = parsedDays; + if (days < 1) days = 1; + if (days > 365) days = 365; var groupBy = query["groupBy"] ?? "city"; diff --git a/src/Backend/AutopilotMonitor.Functions/Functions/Metrics/GetGeographicMetricsFunction.cs b/src/Backend/AutopilotMonitor.Functions/Functions/Metrics/GetGeographicMetricsFunction.cs index 432c80c8..db696ed6 100644 --- a/src/Backend/AutopilotMonitor.Functions/Functions/Metrics/GetGeographicMetricsFunction.cs +++ b/src/Backend/AutopilotMonitor.Functions/Functions/Metrics/GetGeographicMetricsFunction.cs @@ -39,6 +39,8 @@ namespace AutopilotMonitor.Functions.Functions.Metrics int days = 30; if (!string.IsNullOrEmpty(daysParam) && int.TryParse(daysParam, out var parsedDays) && parsedDays > 0) days = parsedDays; + if (days < 1) days = 1; + if (days > 365) days = 365; var groupBy = query["groupBy"] ?? "city"; diff --git a/src/Backend/AutopilotMonitor.Functions/Functions/Metrics/McpUsageMetricsFunction.cs b/src/Backend/AutopilotMonitor.Functions/Functions/Metrics/McpUsageMetricsFunction.cs index d62c7de6..ee6ef382 100644 --- a/src/Backend/AutopilotMonitor.Functions/Functions/Metrics/McpUsageMetricsFunction.cs +++ b/src/Backend/AutopilotMonitor.Functions/Functions/Metrics/McpUsageMetricsFunction.cs @@ -7,6 +7,8 @@ using Microsoft.Azure.Functions.Worker; using Microsoft.Azure.Functions.Worker.Http; using Microsoft.Extensions.Logging; +// Cross-tenant guard helpers — exposed for unit testing. + namespace AutopilotMonitor.Functions.Functions.Metrics { /// @@ -75,7 +77,12 @@ namespace AutopilotMonitor.Functions.Functions.Metrics } /// - /// GET /api/metrics/mcp-usage/user/{userId}?dateFrom=&dateTo= — Usage for a specific user + /// GET /api/metrics/mcp-usage/user/{userId}?dateFrom=&dateTo= — Usage for a specific user. + /// + /// Catalog policy is TenantAdminOrGA, but the route has no TenantScoping — middleware can't + /// enforce cross-tenant access since {userId} is an Azure AD object id, not a tenant id. + /// We therefore enforce here: if the records' TenantId differs from the caller's tenant + /// (and the caller is not a Global Admin), block with 403. /// [Function("GetMcpUserUsage")] public async Task GetUserUsage( @@ -91,6 +98,27 @@ namespace AutopilotMonitor.Functions.Functions.Metrics var records = await _userUsageRepo.GetUsageByUserAsync(userId, dateFrom, dateTo); + var ctx = req.GetRequestContext(); + if (UsageCrossTenantGuard.IsForeignTenantAccess(records, ctx.TenantId, ctx.IsGlobalAdmin)) + { + var foundTenants = records + .Select(r => r.TenantId) + .Where(t => !string.IsNullOrEmpty(t)) + .Distinct(StringComparer.OrdinalIgnoreCase) + .ToArray(); + _logger.LogWarning( + "[McpUsage] Blocked cross-tenant access: caller={Caller} callerTid={CallerTid} targetUser={UserId} foundTenants={Tenants}", + ctx.UserPrincipalName, ctx.TenantId, userId, string.Join(",", foundTenants)); + + var forbidden = req.CreateResponse(HttpStatusCode.Forbidden); + await forbidden.WriteAsJsonAsync(new + { + error = "CrossTenantAccessDenied", + message = "Access denied. You can only access usage for users in your own tenant." + }); + return forbidden; + } + var response = req.CreateResponse(HttpStatusCode.OK); await response.WriteAsJsonAsync(new { userId, records }); return response; diff --git a/src/Backend/AutopilotMonitor.Functions/Functions/Metrics/UsageCrossTenantGuard.cs b/src/Backend/AutopilotMonitor.Functions/Functions/Metrics/UsageCrossTenantGuard.cs new file mode 100644 index 00000000..9682da89 --- /dev/null +++ b/src/Backend/AutopilotMonitor.Functions/Functions/Metrics/UsageCrossTenantGuard.cs @@ -0,0 +1,37 @@ +using AutopilotMonitor.Shared.DataAccess; + +namespace AutopilotMonitor.Functions.Functions.Metrics; + +/// +/// Cross-tenant access guard for GET /api/metrics/mcp-usage/user/{userId}. +/// +/// The route accepts an Azure AD object id (oid), which has no inherent tenant scoping — +/// middleware can't validate it the way it does for {tenantId} routes. The function +/// therefore inspects the returned s and blocks the response +/// if any record belongs to a tenant other than the caller's, unless the caller is a Global Admin. +/// +public static class UsageCrossTenantGuard +{ + /// + /// Returns true if the records contain at least one entry whose tenant differs from + /// AND the caller is not a Global Admin. + /// Empty record sets and GA callers always pass (return false). + /// + public static bool IsForeignTenantAccess( + IEnumerable records, + string callerTenantId, + bool isGlobalAdmin) + { + if (isGlobalAdmin) return false; + if (records == null) return false; + if (string.IsNullOrEmpty(callerTenantId)) return false; + + foreach (var r in records) + { + if (string.IsNullOrEmpty(r.TenantId)) continue; + if (!string.Equals(r.TenantId, callerTenantId, StringComparison.OrdinalIgnoreCase)) + return true; + } + return false; + } +} diff --git a/src/Backend/AutopilotMonitor.Functions/Functions/Sessions/SearchSessionsByCveFunction.cs b/src/Backend/AutopilotMonitor.Functions/Functions/Sessions/SearchSessionsByCveFunction.cs index 18d719bf..da251cb7 100644 --- a/src/Backend/AutopilotMonitor.Functions/Functions/Sessions/SearchSessionsByCveFunction.cs +++ b/src/Backend/AutopilotMonitor.Functions/Functions/Sessions/SearchSessionsByCveFunction.cs @@ -54,7 +54,7 @@ public class SearchSessionsByCveFunction double? minCvssScore = double.TryParse(query["minCvssScore"], out var mcs) ? mcs : null; var overallRisk = query["overallRisk"]; - var limit = int.TryParse(query["limit"], out var lim) ? Math.Min(lim, 100) : 50; + var limit = int.TryParse(query["limit"], out var lim) ? Math.Clamp(lim, 1, 100) : 50; var sessions = await _sessionRepo.SearchSessionsByCveAsync(tenantId, cveId, minCvssScore, overallRisk, limit); diff --git a/src/Backend/AutopilotMonitor.Functions/Functions/Sessions/SearchSessionsByEventFunction.cs b/src/Backend/AutopilotMonitor.Functions/Functions/Sessions/SearchSessionsByEventFunction.cs index cabd1495..dd34bddd 100644 --- a/src/Backend/AutopilotMonitor.Functions/Functions/Sessions/SearchSessionsByEventFunction.cs +++ b/src/Backend/AutopilotMonitor.Functions/Functions/Sessions/SearchSessionsByEventFunction.cs @@ -52,7 +52,7 @@ public class SearchSessionsByEventFunction return badReq; } - var limit = int.TryParse(query["limit"], out var lim) ? Math.Min(lim, 100) : 50; + var limit = int.TryParse(query["limit"], out var lim) ? Math.Clamp(lim, 1, 100) : 50; var sessions = await _sessionRepo.SearchSessionsByEventAsync(tenantId, eventType, null, null, null, limit); diff --git a/src/Backend/AutopilotMonitor.Functions/Functions/Sessions/SearchSessionsFunction.cs b/src/Backend/AutopilotMonitor.Functions/Functions/Sessions/SearchSessionsFunction.cs index 8885f3ca..ad173ce2 100644 --- a/src/Backend/AutopilotMonitor.Functions/Functions/Sessions/SearchSessionsFunction.cs +++ b/src/Backend/AutopilotMonitor.Functions/Functions/Sessions/SearchSessionsFunction.cs @@ -58,7 +58,7 @@ public class SearchSessionsFunction GeoCountry = query["geoCountry"], AgentVersion = query["agentVersion"], ImeAgentVersion = query["imeAgentVersion"], - Limit = int.TryParse(query["limit"], out var lim) ? Math.Min(lim, 100) : 50, + Limit = int.TryParse(query["limit"], out var lim) ? Math.Clamp(lim, 1, 100) : 50, }; if (bool.TryParse(query["isPreProvisioned"], out var ipp)) filter.IsPreProvisioned = ipp; diff --git a/src/McpServer/autopilot-monitor-mcp/src/tools/admin.ts b/src/McpServer/autopilot-monitor-mcp/src/tools/admin.ts index 980a4411..4e963b6d 100644 --- a/src/McpServer/autopilot-monitor-mcp/src/tools/admin.ts +++ b/src/McpServer/autopilot-monitor-mcp/src/tools/admin.ts @@ -25,7 +25,7 @@ export function registerAdminTools(server: McpServer): void { let data: unknown; const params: Record = { tenantId: args.tenantId, dateFrom: args.dateFrom, dateTo: args.dateTo }; if (args.userId) { - data = await apiFetch(`/api/metrics/mcp-usage/${encodeURIComponent(args.userId)}${buildQuery(params)}`); + data = await apiFetch(`/api/metrics/mcp-usage/user/${encodeURIComponent(args.userId)}${buildQuery(params)}`); } else if (args.daily) { data = await apiFetch(`/api/global/metrics/mcp-usage/daily${buildQuery(params)}`); } else { @@ -43,10 +43,12 @@ export function registerAdminTools(server: McpServer): void { 'get_geographic_metrics', 'Get geographic distribution of enrollments — where devices are enrolling from, with performance comparisons. ' + 'Shows per-location: session counts, success rates, avg/median/p95 duration, throughput, and outlier detection. ' + - 'Omit tenantId for cross-tenant view (Global Admin). Use get_geographic_sessions to drill into a specific location.', + 'Omit tenantId for cross-tenant view (Global Admin). Use get_geographic_sessions to drill into a specific location. ' + + 'days accepts any value 1-365 (e.g. 5, 7, 12, 30, 90).', { tenantId: z.string().optional().describe('Tenant ID. Omit for cross-tenant view (Global Admin only).'), - days: z.coerce.number().optional().default(30).describe('Time range in days (default: 30)'), + days: z.coerce.number().int().min(1).max(365).optional().default(30) + .describe('Time range in days (1-365). Defaults to 30.'), groupBy: z.enum(['country', 'region', 'city']).optional().default('city') .describe('Geographic grouping level (default: "city")'), }, @@ -77,7 +79,8 @@ export function registerAdminTools(server: McpServer): void { region: z.string().optional().describe('Region/state filter (e.g. "Saxony", "North Carolina"). Used with country.'), city: z.string().optional().describe('City filter (e.g. "Falkenstein"). Used with country.'), tenantId: z.string().optional().describe('Tenant ID. Omit for cross-tenant view (Global Admin only).'), - days: z.coerce.number().optional().default(30).describe('Time range in days (default: 30)'), + days: z.coerce.number().int().min(1).max(365).optional().default(30) + .describe('Time range in days (1-365). Defaults to 30.'), }, READ_ONLY, async (args) => withToolTelemetry('get_geographic_sessions', async () => { diff --git a/src/McpServer/autopilot-monitor-mcp/src/tools/sessions.ts b/src/McpServer/autopilot-monitor-mcp/src/tools/sessions.ts index b2575951..e518a759 100644 --- a/src/McpServer/autopilot-monitor-mcp/src/tools/sessions.ts +++ b/src/McpServer/autopilot-monitor-mcp/src/tools/sessions.ts @@ -378,9 +378,10 @@ export function registerSessionTools(server: McpServer): void { server.tool( 'list_blocked_devices', 'List devices currently blocked from enrolling. Blocked devices have their enrollment sessions rejected by the backend. ' + - 'Omit tenantId to list blocked devices across ALL tenants (requires Global Admin).', + 'Global Admin only — both the tenant-scoped (?tenantId=) and cross-tenant variants of this endpoint require Global Admin. ' + + 'Tenant Admins and Operators receive 403 (the backend manages the device block list as a platform-wide concern).', { - tenantId: z.string().optional().describe('Tenant ID to scope results. Omit for cross-tenant listing (Global Admin only).'), + tenantId: z.string().optional().describe('Tenant ID to scope results. Optional — both forms require Global Admin.'), }, READ_ONLY, async (args) => withToolTelemetry('list_blocked_devices', async () => {