mirror of
https://github.com/okieselbach/AutopilotMonitor.git
synced 2026-10-24 04:32:32 +00:00
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 <noreply@anthropic.com>
This commit is contained in:
@@ -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"));
|
||||
|
||||
|
||||
@@ -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];
|
||||
|
||||
/// <summary>
|
||||
/// ASSUME-BREACH layer for the Global-Admin-only surface. A 403 on a <c>GlobalAdminOnly</c> 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;
|
||||
|
||||
/// <summary>Pure projection of a denial into the record the reporter writes; separated so tests can pin it without an HttpContext.</summary>
|
||||
/// <summary>Longest request path kept in a denial record; the route catalog has nothing longer.</summary>
|
||||
private const int MaxDenialPathLength = 256;
|
||||
|
||||
/// <summary>
|
||||
/// 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.
|
||||
/// </summary>
|
||||
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,
|
||||
|
||||
Reference in New Issue
Block a user