From fa640ebe612167400075f4e32dbb14068f849b3d Mon Sep 17 00:00:00 2001 From: Oliver Kieselbach Date: Wed, 22 Jul 2026 01:38:47 +0200 Subject: [PATCH] fix(security): resolve CodeQL log-forging findings and suppress by-design queries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bucket B (real fixes) + Bucket A (config) of the CodeQL alert triage. Log-forging (cs/log-forging, 8 sinks): add Helpers/LogSanitizer that strips CR/LF and other control characters, and apply it to the logged copy of every user-controlled value in AuthenticationMiddleware, PolicyEnforcementMiddleware (Invoke + DecideAsync), McpQuotaEnforcementMiddleware and TableUserUsageRepository. Only the logged value is sanitized — routing, authorization and storage keys are untouched, so tracing fidelity for legitimate values (paths, UPNs, GUIDs) is preserved. Structured logging already isolated the fields; this closes the CRLF vector as defense-in-depth and clears the query. Three genuine JS/TS fixes: - guardValidation.ts: replace the no-op `.replace(/%/g, "%")` with a real escapeRegExp() — the author intended to escape; this also closes the latent metacharacter gap (js/identity-replacement). - access-guard.ts: use a `%s` format argument instead of interpolating the UPN into the console.error format string (js/tainted-format-string). - docs-corpus.ts: strip HTML tags with a loop-until-stable stripHtmlTags() so the sanitizer is robust and recognized as complete (js/incomplete-multi-char-sanitization). CodeQL config: filter the two queries that fire structurally on this codebase, where the flagged behavior is the feature — cs/exposure-of-sensitive-information (audit/notification logs legitimately record the actor UPN / recipient email) and cs/cleartext-storage-of-sensitive-information (rate limiters key on the trusted client IP). Default suites stay active; genuine one-off false positives from other queries are dismissed individually so those queries stay sharp. Tests: Backend xUnit 3110 (+9 LogSanitizer), MCP vitest 19 (+1 tag-strip), Web vitest 49, all tsc clean. Co-Authored-By: Claude Opus 4.8 (1M context) --- .github/codeql/codeql-config.yml | 31 ++++++++++ .github/workflows/codeql.yml | 1 + .../LogSanitizerTests.cs | 57 +++++++++++++++++++ .../TableStorage/TableUserUsageRepository.cs | 3 +- .../Helpers/LogSanitizer.cs | 33 +++++++++++ .../Middleware/AuthenticationMiddleware.cs | 12 ++-- .../McpQuotaEnforcementMiddleware.cs | 2 +- .../Middleware/PolicyEnforcementMiddleware.cs | 18 ++++-- .../src/__tests__/docs-corpus.test.ts | 15 +++++ .../autopilot-monitor-mcp/src/access-guard.ts | 2 +- .../autopilot-monitor-mcp/src/docs-corpus.ts | 20 ++++++- .../utils/guardValidation.ts | 10 +++- 12 files changed, 189 insertions(+), 15 deletions(-) create mode 100644 .github/codeql/codeql-config.yml create mode 100644 src/Backend/AutopilotMonitor.Functions.Tests/LogSanitizerTests.cs create mode 100644 src/Backend/AutopilotMonitor.Functions/Helpers/LogSanitizer.cs diff --git a/.github/codeql/codeql-config.yml b/.github/codeql/codeql-config.yml new file mode 100644 index 00000000..900b33b2 --- /dev/null +++ b/.github/codeql/codeql-config.yml @@ -0,0 +1,31 @@ +# CodeQL configuration. +# +# The default query suites stay active — this file only filters out two C# queries +# that fire structurally on this codebase, where the flagged behavior is the feature, +# not a defect: +# +# cs/exposure-of-sensitive-information +# Audit and operational logs record WHO performed an action (actor UPN / email) +# and notification services deliver mail to a recipient address. Writing that +# identity to the log / notification IS the purpose of the field. Every such site +# is a legitimate, reviewed use — the query cannot distinguish it from a leak. +# +# cs/cleartext-storage-of-sensitive-information +# The rate limiters and distress guard use the trusted client IP as their bucket +# key. The IP is an identifier for throttling, not a secret at rest; storing it in +# the in-memory / table rate-limit key is by design. +# +# Genuine one-off false positives from OTHER queries (e.g. an open-redirect the taint +# model can't see is allow-listed, a rate-limited route it doesn't recognize) are +# dismissed individually in the Security tab so those queries stay sharp — they are +# NOT suppressed here. +# +# Log-injection (cs/log-forging) is deliberately NOT filtered: user-controlled values +# are sanitized at the sink via Helpers/LogSanitizer, so the query stays useful for +# catching a future unsanitized sink. + +query-filters: + - exclude: + id: cs/exposure-of-sensitive-information + - exclude: + id: cs/cleartext-storage-of-sensitive-information diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml index c9ed27b7..ffa14773 100644 --- a/.github/workflows/codeql.yml +++ b/.github/workflows/codeql.yml @@ -47,6 +47,7 @@ jobs: with: languages: ${{ matrix.language }} build-mode: ${{ matrix.build-mode }} + config-file: .github/codeql/codeql-config.yml - name: Build if: matrix.build-mode == 'manual' diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/LogSanitizerTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/LogSanitizerTests.cs new file mode 100644 index 00000000..5df27fd4 --- /dev/null +++ b/src/Backend/AutopilotMonitor.Functions.Tests/LogSanitizerTests.cs @@ -0,0 +1,57 @@ +using AutopilotMonitor.Functions.Helpers; + +namespace AutopilotMonitor.Functions.Tests; + +public class LogSanitizerTests +{ + [Fact] + public void Clean_WithCrlf_StripsNewlines() + { + // The core log-forging vector: a value that injects a fake log line. + var forged = "/api/sessions\r\n[Auth Middleware] FAKE: admin granted"; + var cleaned = LogSanitizer.Clean(forged); + Assert.Equal("/api/sessions[Auth Middleware] FAKE: admin granted", cleaned); + Assert.DoesNotContain('\r', cleaned!); + Assert.DoesNotContain('\n', cleaned!); + } + + [Fact] + public void Clean_WithBareLineFeedOrCarriageReturn_StripsBoth() + { + Assert.Equal("ab", LogSanitizer.Clean("a\nb")); + Assert.Equal("ab", LogSanitizer.Clean("a\rb")); + } + + [Fact] + public void Clean_WithOtherControlChars_StripsThem() + { + // Tab (9), NUL (0) and ESC (27, terminal escape) are all removed. Built + // from char codes so the source file carries no literal control bytes. + var input = $"a{(char)9}b{(char)0}c{(char)27}de"; + Assert.Equal("abcde", LogSanitizer.Clean(input)); + } + + [Theory] + [InlineData("/api/sessions/abc-123/events")] + [InlineData("user@contoso.com")] + [InlineData("11111111-2222-3333-4444-555555555555")] + [InlineData("GET")] + public void Clean_WithLegitimateValue_ReturnsUnchanged(string value) + { + // Real paths, UPNs, GUIDs and methods never contain control chars, so + // tracing fidelity is fully preserved. + Assert.Equal(value, LogSanitizer.Clean(value)); + } + + [Fact] + public void Clean_WithNull_ReturnsNull() + { + Assert.Null(LogSanitizer.Clean(null)); + } + + [Fact] + public void Clean_WithEmpty_ReturnsEmpty() + { + Assert.Equal("", LogSanitizer.Clean("")); + } +} diff --git a/src/Backend/AutopilotMonitor.Functions/DataAccess/TableStorage/TableUserUsageRepository.cs b/src/Backend/AutopilotMonitor.Functions/DataAccess/TableStorage/TableUserUsageRepository.cs index 74308754..2ac99397 100644 --- a/src/Backend/AutopilotMonitor.Functions/DataAccess/TableStorage/TableUserUsageRepository.cs +++ b/src/Backend/AutopilotMonitor.Functions/DataAccess/TableStorage/TableUserUsageRepository.cs @@ -1,3 +1,4 @@ +using AutopilotMonitor.Functions.Helpers; using AutopilotMonitor.Functions.Security; using AutopilotMonitor.Functions.Services; using AutopilotMonitor.Shared; @@ -72,7 +73,7 @@ namespace AutopilotMonitor.Functions.DataAccess.TableStorage } _logger.LogWarning("Failed to increment user usage after {MaxRetries} retries: user={UserId}, endpoint={Endpoint}", - maxRetries, userId, endpoint); + maxRetries, LogSanitizer.Clean(userId), LogSanitizer.Clean(endpoint)); } public async Task> GetUsageByUserAsync(string userId, string? dateFrom = null, string? dateTo = null) diff --git a/src/Backend/AutopilotMonitor.Functions/Helpers/LogSanitizer.cs b/src/Backend/AutopilotMonitor.Functions/Helpers/LogSanitizer.cs new file mode 100644 index 00000000..aa192d23 --- /dev/null +++ b/src/Backend/AutopilotMonitor.Functions/Helpers/LogSanitizer.cs @@ -0,0 +1,33 @@ +using System.Text.RegularExpressions; + +namespace AutopilotMonitor.Functions.Helpers; + +/// +/// Strips control characters from user-controlled values before they reach a log +/// sink, so a caller cannot forge additional log lines (CRLF injection) or smuggle +/// terminal escape sequences through the message. Structured logging already stores +/// each value as a discrete property, so this is defense-in-depth — it also clears +/// the CodeQL cs/log-forging alerts by removing the newline characters they key on. +/// Only ever apply to the LOGGED copy of a value; never to a value used for routing, +/// authorization, or storage keys. +/// +public static class LogSanitizer +{ + // \p{Cc} = Unicode "Other, control" (C0 + C1 ranges), which includes CR and LF. + private static readonly Regex ControlChars = new(@"\p{Cc}", RegexOptions.Compiled); + + /// + /// Removes CR, LF, and every other control character from . + /// Returns null/empty inputs unchanged. + /// + public static string? Clean(string? value) + { + if (string.IsNullOrEmpty(value)) + return value; + + // Explicit CR/LF removal first — this is the log-injection barrier the CodeQL + // taint model recognizes; the regex pass then drops any remaining control chars. + var sanitized = value.Replace("\r", string.Empty).Replace("\n", string.Empty); + return ControlChars.Replace(sanitized, string.Empty); + } +} diff --git a/src/Backend/AutopilotMonitor.Functions/Middleware/AuthenticationMiddleware.cs b/src/Backend/AutopilotMonitor.Functions/Middleware/AuthenticationMiddleware.cs index d7790fcb..17435b7f 100644 --- a/src/Backend/AutopilotMonitor.Functions/Middleware/AuthenticationMiddleware.cs +++ b/src/Backend/AutopilotMonitor.Functions/Middleware/AuthenticationMiddleware.cs @@ -11,6 +11,7 @@ using Microsoft.AspNetCore.Http; using System.Net; using System.Security.Claims; using AutopilotMonitor.Functions.Security; +using AutopilotMonitor.Functions.Helpers; namespace AutopilotMonitor.Functions.Middleware; @@ -57,9 +58,12 @@ public class AuthenticationMiddleware : IFunctionsWorkerMiddleware // JWT validation. The exempt set is DERIVED from EndpointAccessPolicyCatalog — never a // hand-kept parallel list — so a new anonymous/device route can't drift out of sync here. var requestPath = httpContext.Request.Path.Value ?? string.Empty; + // Sanitized copies for logging only — never used for routing/authorization decisions. + var logPath = LogSanitizer.Clean(requestPath); + var logMethod = LogSanitizer.Clean(httpContext.Request.Method); if (SkipsJwtValidation(httpContext.Request.Method, requestPath)) { - _logger.LogDebug("[Auth Middleware] JWT-exempt route: {Method} {Path}", httpContext.Request.Method, requestPath); + _logger.LogDebug("[Auth Middleware] JWT-exempt route: {Method} {Path}", logMethod, logPath); await next(context); return; } @@ -68,7 +72,7 @@ public class AuthenticationMiddleware : IFunctionsWorkerMiddleware var authHeader = httpContext.Request.Headers.Authorization.ToString(); if (string.IsNullOrEmpty(authHeader) || !authHeader.StartsWith("Bearer ", StringComparison.OrdinalIgnoreCase)) { - _logger.LogWarning("[Auth Middleware] Blocked unauthenticated request to {Path}", requestPath); + _logger.LogWarning("[Auth Middleware] Blocked unauthenticated request to {Path}", logPath); httpContext.Response.StatusCode = (int)HttpStatusCode.Unauthorized; httpContext.Response.ContentType = "application/json"; await httpContext.Response.WriteAsJsonAsync(new @@ -96,7 +100,7 @@ public class AuthenticationMiddleware : IFunctionsWorkerMiddleware // MSA, tid 9188040d-…) carries a GUID tid, so this reject is loss-free. if (!IsValidTenantId(tenantId)) { - _logger.LogWarning("[Auth Middleware] Rejected token with missing/non-GUID tid for {Path}", requestPath); + _logger.LogWarning("[Auth Middleware] Rejected token with missing/non-GUID tid for {Path}", logPath); throw new SecurityTokenValidationException("Token tid claim is missing or not a valid GUID"); } @@ -207,7 +211,7 @@ public class AuthenticationMiddleware : IFunctionsWorkerMiddleware if (!authenticated) { - _logger.LogWarning("[Auth Middleware] Blocked request with invalid token to {Path}", requestPath); + _logger.LogWarning("[Auth Middleware] Blocked request with invalid token to {Path}", logPath); httpContext.Response.StatusCode = (int)HttpStatusCode.Unauthorized; httpContext.Response.ContentType = "application/json"; await httpContext.Response.WriteAsJsonAsync(new diff --git a/src/Backend/AutopilotMonitor.Functions/Middleware/McpQuotaEnforcementMiddleware.cs b/src/Backend/AutopilotMonitor.Functions/Middleware/McpQuotaEnforcementMiddleware.cs index e2f9289f..74d9f653 100644 --- a/src/Backend/AutopilotMonitor.Functions/Middleware/McpQuotaEnforcementMiddleware.cs +++ b/src/Backend/AutopilotMonitor.Functions/Middleware/McpQuotaEnforcementMiddleware.cs @@ -159,7 +159,7 @@ public class McpQuotaEnforcementMiddleware : IFunctionsWorkerMiddleware } catch (Exception ex) { - logger.LogWarning(ex, "[McpQuota] Failed to record usage: user={UserId}, endpoint={Endpoint}", oid, normalizedEndpoint); + logger.LogWarning(ex, "[McpQuota] Failed to record usage: user={UserId}, endpoint={Endpoint}", LogSanitizer.Clean(oid), LogSanitizer.Clean(normalizedEndpoint)); } }); } diff --git a/src/Backend/AutopilotMonitor.Functions/Middleware/PolicyEnforcementMiddleware.cs b/src/Backend/AutopilotMonitor.Functions/Middleware/PolicyEnforcementMiddleware.cs index 1e9fd71e..cdad5a83 100644 --- a/src/Backend/AutopilotMonitor.Functions/Middleware/PolicyEnforcementMiddleware.cs +++ b/src/Backend/AutopilotMonitor.Functions/Middleware/PolicyEnforcementMiddleware.cs @@ -57,6 +57,10 @@ public class PolicyEnforcementMiddleware : IFunctionsWorkerMiddleware var httpMethod = httpContext.Request.Method; var queryTenantId = httpContext.Request.Query["tenantId"].FirstOrDefault(); + // Sanitized copies for logging only — never used for routing/authorization decisions. + var logPath = LogSanitizer.Clean(requestPath); + var logMethod = LogSanitizer.Clean(httpMethod); + // Authenticated principal set by AuthenticationMiddleware (null on anonymous/device routes). // Read from FunctionContext.Items (reliable in the isolated worker) rather than httpContext.User. ClaimsPrincipal? principal = null; @@ -76,7 +80,7 @@ public class PolicyEnforcementMiddleware : IFunctionsWorkerMiddleware } catch (Exception ex) { - _logger.LogError(ex, "[PolicyEnforcement] Service error evaluating policy for {Method} {Path}", httpMethod, requestPath); + _logger.LogError(ex, "[PolicyEnforcement] Service error evaluating policy for {Method} {Path}", logMethod, logPath); httpContext.Response.StatusCode = (int)HttpStatusCode.ServiceUnavailable; httpContext.Response.ContentType = "application/json"; httpContext.Response.Headers["Retry-After"] = "5"; @@ -92,13 +96,13 @@ public class PolicyEnforcementMiddleware : IFunctionsWorkerMiddleware { context.Items[RequestContext.ItemsKey] = result.Context!; _logger.LogDebug("[PolicyEnforcement] ALLOW {Method} {Path} user={User} role={Role}", - httpMethod, requestPath, result.UserIdentifier, result.UserRole); + logMethod, logPath, LogSanitizer.Clean(result.UserIdentifier), result.UserRole); await next(context); return; } _logger.LogWarning("[PolicyEnforcement] DENIED {Method} {Path} status={Status} user={User} role={Role} reason={Reason}", - httpMethod, requestPath, result.StatusCode, result.UserIdentifier, result.UserRole, result.LogReason); + logMethod, logPath, result.StatusCode, LogSanitizer.Clean(result.UserIdentifier), result.UserRole, result.LogReason); httpContext.Response.StatusCode = result.StatusCode; httpContext.Response.ContentType = "application/json"; await httpContext.Response.WriteAsJsonAsync(new { error = result.ErrorCode, message = result.ErrorMessage }); @@ -115,12 +119,16 @@ public class PolicyEnforcementMiddleware : IFunctionsWorkerMiddleware internal async Task DecideAsync( string httpMethod, string requestPath, string? queryTenantId, ClaimsPrincipal? principal) { + // Sanitized copies for logging only — never used for routing/authorization decisions. + var logPath = LogSanitizer.Clean(requestPath); + var logMethod = LogSanitizer.Clean(httpMethod); + var catalogEntry = EndpointAccessPolicyCatalog.FindPolicy(httpMethod, requestPath); if (catalogEntry == null) { // Fail-closed: unregistered route → 403 _logger.LogError("[PolicyEnforcement] BLOCKED unregistered route: {Method} {Path} — not in catalog (fail-closed)", - httpMethod, requestPath); + logMethod, logPath); return PolicyResult.Deny((int)HttpStatusCode.Forbidden, "Forbidden", "Access denied.", "anonymous", "N/A", "Unregistered"); } @@ -188,7 +196,7 @@ public class PolicyEnforcementMiddleware : IFunctionsWorkerMiddleware if (crossTenant && !hasGlobalScope && !delegatedGrantsRead) { _logger.LogWarning("[PolicyEnforcement] BLOCKED cross-tenant access: user={User} jwtTenant={JwtTenant} target={Target} scoping={Scoping} path={Path}", - decision.UserIdentifier, jwtTenantId, namedTarget, catalogEntry.TenantScoping, requestPath); + LogSanitizer.Clean(decision.UserIdentifier), LogSanitizer.Clean(jwtTenantId), LogSanitizer.Clean(namedTarget), catalogEntry.TenantScoping, logPath); return PolicyResult.Deny((int)HttpStatusCode.Forbidden, "CrossTenantAccessDenied", "Access denied. You can only access your own tenant's resources.", decision.UserIdentifier, decision.UserRole, "CrossTenant"); diff --git a/src/McpServer/autopilot-monitor-mcp/src/__tests__/docs-corpus.test.ts b/src/McpServer/autopilot-monitor-mcp/src/__tests__/docs-corpus.test.ts index 9dc33b9a..c58e7746 100644 --- a/src/McpServer/autopilot-monitor-mcp/src/__tests__/docs-corpus.test.ts +++ b/src/McpServer/autopilot-monitor-mcp/src/__tests__/docs-corpus.test.ts @@ -129,6 +129,21 @@ describe('loadDocsCorpus', () => { expect(chunks.map((c) => c.metadata.heading)).toContain('General › Is Autopilot Monitor free?'); }); + it('strips inline HTML tags from a title, leaving no angle brackets', async () => { + const chunks = await withBundle( + { + 'troubleshooting/faq.md': page( + 'type: FAQ\ntags: [faq]', + ['# FAQ', '', '## General', '', '
', '', 'Does keep-awake work during ESP?', '', 'Keep-awake is an opt-in behavior during the user ESP phase for supported devices.', '', '
'].join('\n'), + ), + }, + (root) => loadDocsCorpus(root), + ); + const headings = chunks.map((c) => c.metadata.heading); + expect(headings).toContain('General › Does keep-awake work during ESP?'); + expect(headings.every((h) => h == null || (!h.includes('<') && !h.includes('>')))).toBe(true); + }); + it('never emits a heading from inside a fenced code block', async () => { const chunks = await withBundle( { diff --git a/src/McpServer/autopilot-monitor-mcp/src/access-guard.ts b/src/McpServer/autopilot-monitor-mcp/src/access-guard.ts index 51820cd3..5c17338c 100644 --- a/src/McpServer/autopilot-monitor-mcp/src/access-guard.ts +++ b/src/McpServer/autopilot-monitor-mcp/src/access-guard.ts @@ -232,7 +232,7 @@ async function checkAccess(upn: string, token: string, clientIp: string): Promis }, MAX_ACCESS_CACHE_ENTRIES); return result; } catch (err) { - console.error(`[access-guard] Backend check failed for ${upn}:`, err); + console.error('[access-guard] Backend check failed for %s:', upn, err); // Fail-closed: deny on backend error return { allowed: false, reason: 'Backend access check unavailable', isGlobalAdmin: false, isGlobalReader: false, infraError: true }; } diff --git a/src/McpServer/autopilot-monitor-mcp/src/docs-corpus.ts b/src/McpServer/autopilot-monitor-mcp/src/docs-corpus.ts index 9c5d40b7..5dc74fe7 100644 --- a/src/McpServer/autopilot-monitor-mcp/src/docs-corpus.ts +++ b/src/McpServer/autopilot-monitor-mcp/src/docs-corpus.ts @@ -217,6 +217,24 @@ interface Section { content: string; } +/** + * Remove HTML tags from a string. The pattern is applied repeatedly until the + * string stops changing, so tag removal can never re-form a tag from the joined + * remainder. A single `.replace(/<[^>]+>/g, '')` happens to be complete for this + * pattern, but the looped form is the shape CodeQL accepts as a robust sanitizer + * (js/incomplete-multi-character-sanitization) and is defensive against any + * future change to the tag regex. + */ +function stripHtmlTags(input: string): string { + let prev: string; + let out = input; + do { + prev = out; + out = out.replace(/<[^>]+>/g, ''); + } while (out !== prev); + return out; +} + /** * Split a page body into heading-delimited sections. * @@ -262,7 +280,7 @@ function splitIntoSections(body: string): Section[] { const summary = /^\s*(.*?)<\/summary>\s*$/i.exec(line); if (summary) { flush(detailsTitle ?? h3 ?? h2); - detailsTitle = summary[1].replace(/<[^>]+>/g, '').trim(); + detailsTitle = stripHtmlTags(summary[1]).trim(); continue; } if (/^\s*<\/details>\s*$/i.test(line)) { diff --git a/src/Web/autopilot-monitor-web/utils/guardValidation.ts b/src/Web/autopilot-monitor-web/utils/guardValidation.ts index f9883841..04f6eb13 100644 --- a/src/Web/autopilot-monitor-web/utils/guardValidation.ts +++ b/src/Web/autopilot-monitor-web/utils/guardValidation.ts @@ -65,13 +65,19 @@ const ALLOWED_USER_PROFILE_SUBDIRS = ["AppData\\Local", "AppData\\Roaming"]; // Helpers // --------------------------------------------------------------------------- +/** Escape a literal string so it can be embedded in a RegExp without its + * characters being interpreted as regex metacharacters. */ +function escapeRegExp(literal: string): string { + return literal.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); +} + /** Expand known Windows environment variables and custom tokens (case-insensitive). */ function expandEnvVars(path: string): string { let result = path; // Expand custom agent token first - result = result.replace(new RegExp(USER_PROFILE_TOKEN.replace(/%/g, "%"), "gi"), USER_PROFILE_PLACEHOLDER); + result = result.replace(new RegExp(escapeRegExp(USER_PROFILE_TOKEN), "gi"), USER_PROFILE_PLACEHOLDER); for (const [envVar, replacement] of Object.entries(COMMON_ENV_VARS)) { - const regex = new RegExp(envVar.replace(/%/g, "%"), "gi"); + const regex = new RegExp(escapeRegExp(envVar), "gi"); result = result.replace(regex, replacement); } return result;