From b6178db8e603cb31529bab32da14e368d29f5d76 Mon Sep 17 00:00:00 2001 From: Oliver Kieselbach Date: Fri, 8 May 2026 14:12:39 +0200 Subject: [PATCH] fix(backend/security): enforce body-tenant for non-GA + lift query-tenant guards to middleware MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes a tenant-boundary write-side hole and removes two manual cross-tenant checks in favor of catalog-driven middleware enforcement. SubmitSessionReport + SubmitDiagFilesReport (the violation): The functions previously fell back to the JWT tenantId only when the body's TenantId was empty, otherwise trusting the body verbatim. A Tenant Admin of Tenant-X could POST a body with tenantId=Y and have the report blob, the SessionReports row, and the audit-log entry all attributed to Tenant-Y (integrity / non-repudiation issue, no read-side leak). New behavior: GA-aware. Non-GAs who send a body tenantId that disagrees with their JWT get a 403 + warning log. GAs may legitimately submit cross-tenant reports (the UI sends the session's tenantId), but MUST set the body field explicitly (no silent fallback that would mis-file under the GA's home tenant). Diagnostics + Progress middleware elevation (defense-in-depth): diagnostics/download-url and progress/sessions/{sessionId}/events lifted from TenantScoping.None (in-function guard) to TenantScoping.QueryParam (catalog + middleware). Same GA bypass semantics — the in-function check was correct but copy-paste fragile. Functions now read requestCtx.TargetTenantId, which the middleware has already validated against the JWT. Test coverage: CrossTenantAccessTests gains 10 new InlineData rows across the 5 QueryParam test methods (no-query / same-tenant / cross-non-GA blocked / cross-GA allowed / scoping correctness) for both lifted routes. diagnostics/download-url moved out of JwtImplicitRoutes_NoCrossTenantCheck since it's no longer JWT-implicit. Verified: dotnet build clean, 1067/1067 backend tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../CrossTenantAccessTests.cs | 11 ++++++- .../DiagnosticsDownloadFunction.cs | 16 ++++------ .../Progress/ProgressPortalFunction.cs | 27 +++++----------- .../Reports/SubmitDiagFilesReportFunction.cs | 29 ++++++++++++++++- .../Reports/SubmitSessionReportFunction.cs | 31 ++++++++++++++++++- .../Security/EndpointAccessPolicyCatalog.cs | 4 +-- 6 files changed, 84 insertions(+), 34 deletions(-) diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/CrossTenantAccessTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/CrossTenantAccessTests.cs index 4599547c..cc544589 100644 --- a/src/Backend/AutopilotMonitor.Functions.Tests/CrossTenantAccessTests.cs +++ b/src/Backend/AutopilotMonitor.Functions.Tests/CrossTenantAccessTests.cs @@ -137,7 +137,6 @@ public class CrossTenantAccessTests [InlineData("GET", "/api/metrics/app")] [InlineData("GET", "/api/audit/logs")] [InlineData("GET", "/api/rules/gather")] - [InlineData("GET", "/api/diagnostics/download-url")] public void JwtImplicitRoutes_NoCrossTenantCheck(string httpMethod, string path) { var (isBlocked, routeTenantId) = SimulateCrossTenantCheck(httpMethod, path, TenantA, isGlobalAdmin: false); @@ -205,6 +204,8 @@ public class CrossTenantAccessTests [InlineData("GET", "/api/sessions/abc-123/analysis")] [InlineData("GET", "/api/sessions/abc-123/vulnerability-report")] [InlineData("GET", "/api/bootstrap/sessions")] + [InlineData("GET", "/api/diagnostics/download-url")] + [InlineData("GET", "/api/progress/sessions/abc-123/events")] public void QueryParam_NoQueryTenant_DefaultsToJwt(string httpMethod, string path) { var (isBlocked, targetTenantId) = SimulateQueryParamCheck(httpMethod, path, TenantA, isGlobalAdmin: false, queryTenantId: null); @@ -219,6 +220,8 @@ public class CrossTenantAccessTests [InlineData("GET", "/api/sessions/abc-123")] [InlineData("GET", "/api/sessions/abc-123/events")] [InlineData("GET", "/api/bootstrap/sessions")] + [InlineData("GET", "/api/diagnostics/download-url")] + [InlineData("GET", "/api/progress/sessions/abc-123/events")] public void QueryParam_SameTenant_IsAllowed(string httpMethod, string path) { var (isBlocked, targetTenantId) = SimulateQueryParamCheck(httpMethod, path, TenantA, isGlobalAdmin: false, queryTenantId: TenantA); @@ -236,6 +239,8 @@ public class CrossTenantAccessTests [InlineData("GET", "/api/sessions/abc-123/vulnerability-report")] [InlineData("GET", "/api/bootstrap/sessions")] [InlineData("DELETE", "/api/bootstrap/sessions/CODE123")] + [InlineData("GET", "/api/diagnostics/download-url")] + [InlineData("GET", "/api/progress/sessions/abc-123/events")] public void QueryParam_CrossTenant_NonGA_IsBlocked(string httpMethod, string path) { var (isBlocked, targetTenantId) = SimulateQueryParamCheck(httpMethod, path, TenantA, isGlobalAdmin: false, queryTenantId: TenantB); @@ -250,6 +255,8 @@ public class CrossTenantAccessTests [InlineData("GET", "/api/sessions/abc-123")] [InlineData("GET", "/api/sessions/abc-123/events")] [InlineData("GET", "/api/bootstrap/sessions")] + [InlineData("GET", "/api/diagnostics/download-url")] + [InlineData("GET", "/api/progress/sessions/abc-123/events")] public void QueryParam_CrossTenant_GlobalAdmin_IsAllowed(string httpMethod, string path) { var (isBlocked, targetTenantId) = SimulateQueryParamCheck(httpMethod, path, TenantA, isGlobalAdmin: true, queryTenantId: TenantB); @@ -268,6 +275,8 @@ public class CrossTenantAccessTests [InlineData("GET", "bootstrap/sessions")] [InlineData("POST", "bootstrap/sessions")] [InlineData("DELETE", "bootstrap/sessions/{code}")] + [InlineData("GET", "diagnostics/download-url")] + [InlineData("GET", "progress/sessions/{sessionId}/events")] public void QueryParamRoutes_HaveCorrectScoping(string httpMethod, string routeTemplate) { var entry = EndpointAccessPolicyCatalog.Entries diff --git a/src/Backend/AutopilotMonitor.Functions/Functions/Diagnostics/DiagnosticsDownloadFunction.cs b/src/Backend/AutopilotMonitor.Functions/Functions/Diagnostics/DiagnosticsDownloadFunction.cs index a8bdcfe9..c1f0c43b 100644 --- a/src/Backend/AutopilotMonitor.Functions/Functions/Diagnostics/DiagnosticsDownloadFunction.cs +++ b/src/Backend/AutopilotMonitor.Functions/Functions/Diagnostics/DiagnosticsDownloadFunction.cs @@ -44,27 +44,23 @@ namespace AutopilotMonitor.Functions.Functions.Diagnostics { try { - // Authentication + MemberRead authorization enforced by PolicyEnforcementMiddleware + // Authentication, MemberRead authz, AND cross-tenant access enforced by + // PolicyEnforcementMiddleware (catalog: TenantScoping.QueryParam). + // requestCtx.TargetTenantId is the middleware-validated tenantId from the + // ?tenantId= query param (GA bypass already applied). var requestCtx = req.GetRequestContext(); var query = HttpUtility.ParseQueryString(req.Url.Query); - var tenantId = query["tenantId"]; var blobName = query["blobName"]; - if (string.IsNullOrEmpty(tenantId) || string.IsNullOrEmpty(blobName)) + if (string.IsNullOrEmpty(query["tenantId"]) || string.IsNullOrEmpty(blobName)) { var badRequest = req.CreateResponse(HttpStatusCode.BadRequest); await badRequest.WriteAsJsonAsync(new { success = false, message = "tenantId and blobName query parameters are required." }); return badRequest; } - // Validate tenant access - if (!requestCtx.IsGlobalAdmin && tenantId != requestCtx.TenantId) - { - var forbiddenResponse = req.CreateResponse(HttpStatusCode.Forbidden); - await forbiddenResponse.WriteAsJsonAsync(new { success = false, message = "Access denied." }); - return forbiddenResponse; - } + var tenantId = requestCtx.TargetTenantId; // Validate blob name (prevent path traversal, double-encoding, and null byte attacks) var decodedBlobName = Uri.UnescapeDataString(blobName); diff --git a/src/Backend/AutopilotMonitor.Functions/Functions/Progress/ProgressPortalFunction.cs b/src/Backend/AutopilotMonitor.Functions/Functions/Progress/ProgressPortalFunction.cs index 931e0b4e..1fd5bf26 100644 --- a/src/Backend/AutopilotMonitor.Functions/Functions/Progress/ProgressPortalFunction.cs +++ b/src/Backend/AutopilotMonitor.Functions/Functions/Progress/ProgressPortalFunction.cs @@ -92,14 +92,15 @@ public class ProgressPortalFunction try { - // Authentication + AuthenticatedUser authorization enforced by PolicyEnforcementMiddleware + // Authentication, AuthenticatedUser authz, AND cross-tenant access enforced by + // PolicyEnforcementMiddleware (catalog: TenantScoping.QueryParam). + // requestCtx.TargetTenantId is the middleware-validated tenantId from the + // ?tenantId= query param (GA bypass already applied). var requestCtx = req.GetRequestContext(); var userIdentifier = requestCtx.UserPrincipalName; var query = HttpUtility.ParseQueryString(req.Url.Query); - var requestedTenantId = query["tenantId"]; - - if (string.IsNullOrEmpty(requestedTenantId)) + if (string.IsNullOrEmpty(query["tenantId"])) { var badRequest = req.CreateResponse(HttpStatusCode.BadRequest); await badRequest.WriteAsJsonAsync(new @@ -113,22 +114,10 @@ public class ProgressPortalFunction return badRequest; } - // Cross-tenant access only for Global Admins - if (!requestCtx.IsGlobalAdmin && requestedTenantId != requestCtx.TenantId) - { - var forbiddenResponse = req.CreateResponse(HttpStatusCode.Forbidden); - await forbiddenResponse.WriteAsJsonAsync(new - { - success = false, - message = "Access denied. You can only view events for sessions in your own tenant.", - sessionId = sessionId, - count = 0, - events = Array.Empty() - }); - return forbiddenResponse; - } + var requestedTenantId = requestCtx.TargetTenantId; - if (requestCtx.IsGlobalAdmin && requestedTenantId != requestCtx.TenantId) + // Audit log for GA cross-tenant access (middleware allowed it; logged here for visibility). + if (requestCtx.IsGlobalAdmin && !string.Equals(requestedTenantId, requestCtx.TenantId, StringComparison.OrdinalIgnoreCase)) { _logger.LogInformation("{SessionPrefix} Global Admin {User} accessing cross-tenant progress events (tenant: {TenantId})", sessionPrefix, userIdentifier, requestedTenantId); diff --git a/src/Backend/AutopilotMonitor.Functions/Functions/Reports/SubmitDiagFilesReportFunction.cs b/src/Backend/AutopilotMonitor.Functions/Functions/Reports/SubmitDiagFilesReportFunction.cs index 27588ea3..67c17507 100644 --- a/src/Backend/AutopilotMonitor.Functions/Functions/Reports/SubmitDiagFilesReportFunction.cs +++ b/src/Backend/AutopilotMonitor.Functions/Functions/Reports/SubmitDiagFilesReportFunction.cs @@ -74,10 +74,37 @@ namespace AutopilotMonitor.Functions.Functions.Reports return badRequestResponse; } - if (string.IsNullOrEmpty(request.TenantId)) + // Tenant identity: enforce JWT tenantId for non-GAs (prevents body + // tampering / horizontal escalation). Global Admins MAY submit reports + // on behalf of foreign tenants and MUST specify the target tenantId. + if (!requestCtx.IsGlobalAdmin) { + if (!string.IsNullOrEmpty(request.TenantId) + && !string.Equals(request.TenantId, tenantId, StringComparison.OrdinalIgnoreCase)) + { + _logger.LogWarning( + "SubmitDiagFilesReport: BLOCKED cross-tenant body for non-GA user={User} jwtTenant={JwtTenant} bodyTenant={BodyTenant}", + userIdentifier, tenantId, request.TenantId); + var forbidden = req.CreateResponse(HttpStatusCode.Forbidden); + await forbidden.WriteAsJsonAsync(new + { + success = false, + message = "Body tenantId must match your authenticated tenant." + }); + return forbidden; + } request.TenantId = tenantId; } + else if (string.IsNullOrEmpty(request.TenantId)) + { + var bad = req.CreateResponse(HttpStatusCode.BadRequest); + await bad.WriteAsJsonAsync(new + { + success = false, + message = "tenantId is required in body for Global Admin submissions." + }); + return bad; + } var metadata = await _sessionReportService.SubmitDiagFilesReportAsync(request, userIdentifier); diff --git a/src/Backend/AutopilotMonitor.Functions/Functions/Reports/SubmitSessionReportFunction.cs b/src/Backend/AutopilotMonitor.Functions/Functions/Reports/SubmitSessionReportFunction.cs index f47f5e5e..5664b130 100644 --- a/src/Backend/AutopilotMonitor.Functions/Functions/Reports/SubmitSessionReportFunction.cs +++ b/src/Backend/AutopilotMonitor.Functions/Functions/Reports/SubmitSessionReportFunction.cs @@ -71,10 +71,39 @@ namespace AutopilotMonitor.Functions.Functions.Reports // Ensure sessionId consistency request.SessionId = sessionId; - if (string.IsNullOrEmpty(request.TenantId)) + + // Tenant identity: enforce JWT tenantId for non-GAs (prevents body + // tampering / horizontal escalation). Global Admins MAY submit reports + // against foreign tenants — the UI sends the session's tenantId, which + // for cross-tenant GA views is the foreign tenant. + if (!requestCtx.IsGlobalAdmin) { + if (!string.IsNullOrEmpty(request.TenantId) + && !string.Equals(request.TenantId, tenantId, StringComparison.OrdinalIgnoreCase)) + { + _logger.LogWarning( + "SubmitSessionReport: BLOCKED cross-tenant body for non-GA user={User} jwtTenant={JwtTenant} bodyTenant={BodyTenant}", + userIdentifier, tenantId, request.TenantId); + var forbidden = req.CreateResponse(HttpStatusCode.Forbidden); + await forbidden.WriteAsJsonAsync(new + { + success = false, + message = "Body tenantId must match your authenticated tenant." + }); + return forbidden; + } request.TenantId = tenantId; } + else if (string.IsNullOrEmpty(request.TenantId)) + { + var bad = req.CreateResponse(HttpStatusCode.BadRequest); + await bad.WriteAsJsonAsync(new + { + success = false, + message = "tenantId is required in body for Global Admin submissions." + }); + return bad; + } // Submit report var metadata = await _sessionReportService.SubmitReportAsync(request, userIdentifier); diff --git a/src/Backend/AutopilotMonitor.Functions/Security/EndpointAccessPolicyCatalog.cs b/src/Backend/AutopilotMonitor.Functions/Security/EndpointAccessPolicyCatalog.cs index cef661c2..bdb6414c 100644 --- a/src/Backend/AutopilotMonitor.Functions/Security/EndpointAccessPolicyCatalog.cs +++ b/src/Backend/AutopilotMonitor.Functions/Security/EndpointAccessPolicyCatalog.cs @@ -121,7 +121,7 @@ public static class EndpointAccessPolicyCatalog new("POST", "realtime/groups/join", EndpointPolicy.MemberRead), new("POST", "realtime/groups/leave", EndpointPolicy.MemberRead), new("GET", "progress/sessions", EndpointPolicy.AuthenticatedUser), - new("GET", "progress/sessions/{sessionId}/events", EndpointPolicy.AuthenticatedUser), + new("GET", "progress/sessions/{sessionId}/events", EndpointPolicy.AuthenticatedUser, TenantScoping.QueryParam), new("PUT", "preview/notification-email", EndpointPolicy.AuthenticatedUser), new("GET", "feedback/status", EndpointPolicy.AuthenticatedUser), new("POST", "feedback", EndpointPolicy.AuthenticatedUser), @@ -156,7 +156,7 @@ public static class EndpointAccessPolicyCatalog new("GET", "metrics/ime-versions", EndpointPolicy.MemberRead), new("GET", "audit/logs", EndpointPolicy.MemberRead), new("GET", "audit/hardware-rejected", EndpointPolicy.MemberRead), - new("GET", "diagnostics/download-url", EndpointPolicy.MemberRead), + new("GET", "diagnostics/download-url", EndpointPolicy.MemberRead, TenantScoping.QueryParam), new("GET", "rules/gather", EndpointPolicy.MemberRead), new("GET", "rules/analyze", EndpointPolicy.MemberRead), new("GET", "rules/ime-log-patterns", EndpointPolicy.MemberRead),