From 88290caadcfb02972013b4257a74fc75f8ac340f Mon Sep 17 00:00:00 2001 From: Oliver Kieselbach Date: Fri, 4 Sep 2026 16:25:52 +0200 Subject: [PATCH] security: sanitize method and decoded path in the privileged-denial record CodeQL flagged the new denial flow (cs/log-forging #83-#87): header values and the request path travel from the deny branch into a log line, the ops-events table and the alert channels. Headers were already stripped of control characters and capped; the path was not, and Request.Path is percent-DECODED, so %0A arrives as a newline. BuildPrivilegedDenial now cleans method and path (LogSanitizer) and caps the path at 256 chars, once, before any sink. Test pins CR/LF stripping and the cap. The remaining alerts are the known false-positive class (structured ILogger sinks with sanitized discrete properties). Co-Authored-By: Claude Fable 5.1 --- .../PolicyEnforcementMiddlewareTests.cs | 20 +++++++++++++++++++ .../Middleware/PolicyEnforcementMiddleware.cs | 20 +++++++++++++------ 2 files changed, 34 insertions(+), 6 deletions(-) diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/PolicyEnforcementMiddlewareTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/PolicyEnforcementMiddlewareTests.cs index dfcbe30e..367fbe5d 100644 --- a/src/Backend/AutopilotMonitor.Functions.Tests/PolicyEnforcementMiddlewareTests.cs +++ b/src/Backend/AutopilotMonitor.Functions.Tests/PolicyEnforcementMiddlewareTests.cs @@ -242,6 +242,26 @@ public class PolicyEnforcementMiddlewareTests Assert.Null(reader.CorrelationId); } + [Fact] + public async Task BuildPrivilegedDenial_strips_control_characters_from_method_and_decoded_path() + { + var h = BuildHarness(); + var principal = AuthedPrincipal(TenantA, "admin@contoso.com"); + var r = await h.Middleware.DecideAsync("POST", "/api/global/raw/logs", null, principal); + + // A percent-decoded path or a hand-crafted method can carry CR/LF; the record feeds log lines, + // the ops-events table and alert channels, so the barrier sits in the projection. + var d = PolicyEnforcementMiddleware.BuildPrivilegedDenial( + r, "PO\r\nST", "/api/global/raw/logs\r\nforged: line" + new string('x', 400), principal, globalRole: null, + clientSource: null, mcpToolName: null, correlationId: null); + + Assert.Equal("POST", d.Method); + Assert.DoesNotContain('\n', d.Path); + Assert.DoesNotContain('\r', d.Path); + Assert.StartsWith("/api/global/raw/logsforged: line", d.Path); + Assert.Equal(256, d.Path.Length); + } + private static ClaimsPrincipal AuthedPrincipal(string tenantId, string upn, string objectId = TestOid) => new(new ClaimsIdentity(new[] { new Claim("tid", tenantId), new Claim("upn", upn), new Claim("oid", objectId) }, "TestAuth")); diff --git a/src/Backend/AutopilotMonitor.Functions/Middleware/PolicyEnforcementMiddleware.cs b/src/Backend/AutopilotMonitor.Functions/Middleware/PolicyEnforcementMiddleware.cs index d5437367..9db217fb 100644 --- a/src/Backend/AutopilotMonitor.Functions/Middleware/PolicyEnforcementMiddleware.cs +++ b/src/Backend/AutopilotMonitor.Functions/Middleware/PolicyEnforcementMiddleware.cs @@ -132,11 +132,11 @@ public class PolicyEnforcementMiddleware : IFunctionsWorkerMiddleware private static string? DenialHeader(HttpContext httpContext, string name) { var raw = LogSanitizer.Clean(httpContext.Request.Headers[name].FirstOrDefault()); - if (string.IsNullOrEmpty(raw)) - return null; - return raw.Length <= MaxDenialHeaderLength ? raw : raw[..MaxDenialHeaderLength]; + return string.IsNullOrEmpty(raw) ? null : Truncate(raw, MaxDenialHeaderLength); } + private static string Truncate(string value, int max) => value.Length <= max ? value : value[..max]; + /// /// ASSUME-BREACH layer for the Global-Admin-only surface. A 403 on a GlobalAdminOnly route by /// an authenticated caller is the one denial that must never pass silently: it is either an operator @@ -171,13 +171,21 @@ public class PolicyEnforcementMiddleware : IFunctionsWorkerMiddleware && result.Policy == EndpointPolicy.GlobalAdminOnly && result.StatusCode == (int)HttpStatusCode.Forbidden; - /// Pure projection of a denial into the record the reporter writes; separated so tests can pin it without an HttpContext. + /// Longest request path kept in a denial record; the route catalog has nothing longer. + private const int MaxDenialPathLength = 256; + + /// + /// Pure projection of a denial into the record the reporter writes; separated so tests can pin it + /// without an HttpContext. Method and path are caller-controlled too (the path is percent-DECODED, + /// so %0A arrives as a newline) and the record's text reaches log lines, the ops-events table and + /// the alert channels — sanitized and capped here, once, before any of that. + /// internal static PrivilegedDenial BuildPrivilegedDenial( PolicyResult result, string httpMethod, string requestPath, ClaimsPrincipal principal, string? globalRole, string? clientSource, string? mcpToolName, string? correlationId) => new( - Method: httpMethod, - Path: requestPath, + Method: LogSanitizer.Clean(httpMethod) ?? string.Empty, + Path: Truncate(LogSanitizer.Clean(requestPath) ?? string.Empty, MaxDenialPathLength), StatusCode: result.StatusCode, Reason: result.LogReason, Policy: result.Policy?.ToString() ?? string.Empty,