fix(security): address open CodeQL code-scanning alerts#1152
Merged
Aaronontheweb merged 2 commits intoMay 22, 2026
Conversation
- workflows: add `permissions: contents: read` to website-rebuild.yml so the rebuild job no longer inherits the repo's default GITHUB_TOKEN scope (cs-actions/missing-workflow-permissions, alert netclaw-dev#21). - webhooks: sanitize the raw route value via SanitizeWebhookId before logging the route_not_found case — ASP.NET URL-decodes path segments, so a caller could otherwise inject CR/LF into log lines (cs/log-forging, alert netclaw-dev#22). - lifecycle: add a SanitizeReason helper to DaemonLifecycleNotifier that strips control chars and caps length at 200, and apply it in NotifyShutdown (HTTP-tainted via ShutdownDaemonRequest.Reason) and NotifyCrashing (cs/log-forging, alert netclaw-dev#18). - reminders: harden ReminderDefinitionStore.GetPath with an explicit base-directory containment check on top of the existing Uri.EscapeDataString encoding, and add a regression test covering traversal, backslash, and absolute-path ids (cs/path-injection, alert netclaw-dev#17).
Follow-ups from the post-CodeQL code review: - webhooks: sanitize `route` once and pass the safe value to both WebhookTelemetry.RecordRouteNotFound and the log line, not just the log. The metric tag was the same log-forging surface and additionally an unbounded high-cardinality vector against the `route` dimension. - lifecycle: widen the SanitizeReason predicate from `char.IsControl` to also strip U+2028 (LINE SEPARATOR) and U+2029 (PARAGRAPH SEPARATOR); these are categories Zl/Zp (not Cc) so IsControl misses them, but JSON- line readers and many log shippers still split on them. - lifecycle: strip sanitized chars rather than replacing them with space, so an attacker-controlled `reason=ok\nlevel=critical` collapses to `reason=oklevel=critical` instead of `reason=ok level=critical` — a space would have been a plausible field separator to key=value log parsers. - lifecycle: when truncating reason at 200 chars, back off one position if the cut would orphan a high surrogate, so downstream UTF-8 encoders don't emit U+FFFD or throw. - tests: cover the four behaviors above with a Theory across CR/LF, NUL, U+2028, U+2029, and a surrogate-pair-boundary truncation case.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Triaged every open CodeQL alert on the repo and addressed each as a real fix or a documented dismissal. Two commits:
fix(security): address open CodeQL alerts— the primary fixes.fix(security): tighten log/telemetry sanitization from review— follow-ups from a post-fix code review.Real fixes (CodeQL should auto-resolve these as
fixedon the next scan)actions/missing-workflow-permissions— addedpermissions: contents: readto.github/workflows/website-rebuild.yml. The job only dispatches an external workflow via a fine-grained PAT, so the workflow's ownGITHUB_TOKENneeds nothing.cs/log-forginginWebhookEndpointRouteBuilderExtensions.cs— the URL-path-decodedroutevalue is now sanitized via the existingSanitizeWebhookIdallowlist before being passed to both thelogger.LogWarningcall andWebhookTelemetry.RecordRouteNotFound. (The second call site was caught in review — the metric tag was the same log-forging surface and additionally an unbounded high-cardinality vector.)cs/log-forginginDaemonLifecycleNotifier.cs— the HTTP-taintedreason(fromShutdownDaemonRequest.Reason) is now run through a newSanitizeReasonhelper before being logged or copied into alert context/summary. The helper:char.IsControlmatches (Cc category, including CR/LF/NUL/ESC), plus U+2028 LINE SEPARATOR and U+2029 PARAGRAPH SEPARATOR (Zl/Zp — missed byIsControlbut split-on by JSON-line readers and many log shippers).reason=ok\nlevel=criticalbecomesreason=oklevel=criticalinstead of an attacker-friendlyreason=ok level=critical.NotifyCrashingeven though current callers pass code-side constants — closes off future regressions.cs/path-injectioninReminderDefinitionStore.cs—GetPathalready usedUri.EscapeDataString(which neutralizes path separators), but CodeQL doesn't recognize that as a sanitizer. Added an explicit base-directory containment check on the canonicalized path; throwsArgumentExceptionif anything ever escapes. Belt-and-suspenders + makes the scanner happy. New[Theory]regression test covers../../etc/passwd, backslash traversal, and absolute paths.Dismissed as false positives (with per-alert explanatory comments)
cs/user-controlled-bypass—webhooksConfig.Enabledis DI-injected server-side config, not user input. CodeQL has no model for ASP.NET DI so it flags every minimal-API route-handler parameter.cs/log-forginginMcpOAuthService.cs—serverNameis validated against the operator-managed MCP server dictionary before reaching these log lines (so it is effectively operator-configured),stateis a server-generated PKCE value, andclientIdcomes from the operator-configured OAuth server's DCR response. Endpoint also requires authorization. None of these flow from an untrusted remote caller.Test plan
dotnet build src/Netclaw.Daemon/Netclaw.Daemon.csproj— succeeds, 0 warningsdotnet build src/Netclaw.Actors/Netclaw.Actors.csproj— succeeds, 0 warningsdotnet test src/Netclaw.Daemon.Tests --filter "...Webhook|...Lifecycle|...DaemonLifecycleNotifier"— 57/57 pass (includes 5 new InlineData rows + a surrogate-pair-boundary case)dotnet test src/Netclaw.Actors.Tests --filter "...Reminder"— 144/144 pass (includes the new path-traversal Theory)dotnet slopwatch analyze— 0 issuespwsh -File ./scripts/Add-FileHeaders.ps1 -Verify— all files have headersfixed