Repository navigation
Security: sanitize logged user input + recognize LogSanitizer + mask email (v3.9.1) - #256
Merged
Merged
Conversation
…; mask email (v3.9.1)
Clears the CodeQL log-forging / sensitive-info backlog (21 alerts), two ways:
1. Genuine gaps fixed — values that were logged raw now go through LogSanitizer:
- AuthController: GetRequestPath() and GetUserAgent() (client-controlled) are
sanitized, covering every auth audit-log call.
- UsersController: actor and target usernames in the admin audit logs.
2. Already-sanitized sinks (metadata lookups, SMTP) were flagged only because
CodeQL analyses LogSanitizer's body and didn't recognize it as a sanitizer.
Sanitize() now ends with an explicit CR/LF String.Replace — functionally a
no-op (control chars, incl. CR/LF, are already replaced) but the newline
removal is the barrier the cs/log-forging query recognizes, so the whole class
of alerts clears.
3. SMTP logs now use LogSanitizer.MaskEmail (e.g. "r***@example.com") instead of
the full address, reducing PII in operational logs (the cs/exposure alerts).
The actual email send is unchanged — only the log argument is masked. Version
bumped 3.9.0 -> 3.9.1 (3.9.0 is released).
Tests: LogSanitizer Sanitize tests unchanged (same output) plus new MaskEmail
coverage. Full suite 478 passing. The reduction of the existing main alert count
is confirmed by CodeQL's post-merge scan.
Note: one cs/cleartext-storage alert on UsersController (the boolean
ForcePasswordReset audit field) is a false positive flagged only for the
"password" substring; it needs a dismissal, which this change can't do in code.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NnmDYLFwxwNNrPn8G5cJEW
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if (string.IsNullOrWhiteSpace(_options.Host)) | ||
| { | ||
| _logger.LogWarning("Email send skipped because Smtp:Host is not configured. Intended recipient {ToAddress}.", LogSanitizer.Sanitize(toAddress)); | ||
| _logger.LogWarning("Email send skipped because Smtp:Host is not configured. Intended recipient {ToAddress}.", LogSanitizer.MaskEmail(toAddress)); |
| // auth failure, rejected recipient) must never propagate into a | ||
| // caller's HTTP response — see Global Constraints. Logged, not rethrown. | ||
| _logger.LogError(ex, "Failed to send email to {ToAddress}.", LogSanitizer.Sanitize(toAddress)); | ||
| _logger.LogError(ex, "Failed to send email to {ToAddress}.", LogSanitizer.MaskEmail(toAddress)); |
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
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.
Clears the CodeQL log-forging / sensitive-info backlog (21 open alerts on
main). The investigation found two-thirds of them were already sanitized — CodeQL just didn't recognize our first-partyLogSanitizer. So the fix is both real gap-closing and making the sanitizer recognizable.1. Genuine gaps (were logged raw)
GetRequestPath()andGetUserAgent()are client-controlled and were logged unsanitized; both now go throughLogSanitizer, covering every auth audit-log call.2. Make
LogSanitizerCodeQL-recognizableThe metadata-lookup and SMTP sinks already called
LogSanitizer.Sanitize(...), but CodeQL analyses the helper's body (astring.Createchar loop) and doesn't see it as a sanitizer, so it kept flagging them.Sanitize()now ends with an explicitReplace("\r","_").Replace("\n","_")— functionally a no-op (control chars incl. CR/LF are already replaced above), but newline-removal viaString.Replaceis the barrier thecs/log-forgingquery recognizes. So the whole class clears without churning every call site.3. Mask email PII in SMTP logs
The two
cs/exposure-of-sensitive-informationalerts were full email addresses in operational logs. SMTP logs now useLogSanitizer.MaskEmail→r***@example.com(first char + domain). The actual email send is unchanged — only the log argument is masked.Tests
Existing
LogSanitizerbehaviour is unchanged (same outputs; verified) plus newMaskEmailcoverage (normal, malformed, control-char cases). Full suite 478 passing locally.Honest caveats
Replacecan't be verified locally — I can't run CodeQL here. The post-merge CodeQL scan ofmainconfirms the count drop; if any log-forging alerts survive, they're the same recognized-sanitizer gap and we can add a CodeQL sanitizer model or dismiss.cs/cleartext-storagealert remains (the booleanForcePasswordResetaudit field in UsersController) — a false positive flagged only because the name contains "password". It needs a dismissal (a code change can't fix a name-based heuristic), which I'm blocked from doing; that's a one-click dismiss for you.Version: 3.9.0 → 3.9.1 (3.9.0 is released).
🤖 Generated with Claude Code
https://claude.ai/code/session_01NnmDYLFwxwNNrPn8G5cJEW