Harden logging: neutralize log injection + stop logging sensitive tokens (#464, #465)#480
Open
jorisjonkers-dev-agents[bot] wants to merge 2 commits into
Open
Harden logging: neutralize log injection + stop logging sensitive tokens (#464, #465)#480jorisjonkers-dev-agents[bot] wants to merge 2 commits into
jorisjonkers-dev-agents[bot] wants to merge 2 commits into
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| val fullPath = rootLocation.resolve(path).normalize() | ||
|
|
||
| log.info("Storing {} at {}", multipart.originalFilename, fullPath) | ||
| log.info("Storing {} at {}", multipart.originalFilename.forLog(), fullPath) |
|
|
||
| override fun createList(name: String, folderName: String?): Long { | ||
| log.info("Creating Brevo list '{}'", name) | ||
| log.info("Creating Brevo list '{}'", name.forLog()) |
| req.folderId = contributionPeriodsFolder | ||
| val response = contactsApi.createList(req) | ||
| log.info("Created Brevo list '{}' id={}", name, response.id) | ||
| log.info("Created Brevo list '{}' id={}", name.forLog(), response.id) |
| response.id | ||
| } catch (e: RestClientResponseException) { | ||
| log.error("Failed to create Brevo list '{}'", name, e) | ||
| log.error("Failed to create Brevo list '{}'", name.forLog(), e) |
| val listId = listIdSequence.getAndIncrement() | ||
| lists[listId] = MockList(listId = listId, listName = name, folderName = folderName) | ||
| log.info("Mock: Created list id={} name='{}'", listId, name) | ||
| log.info("Mock: Created list id={} name='{}'", listId, name.forLog()) |
| // someone pointing forward-auth at us via Host injection) gets | ||
| // ADMIN-required. Warn so the operator notices. | ||
| log.warn("forward-auth: unknown host '{}' — defaulting to ADMIN", forwardedHost) | ||
| log.warn("forward-auth: unknown host '{}' — defaulting to ADMIN", forwardedHost.forLog()) |
ExtraToast
requested changes
Jul 18, 2026
ExtraToast
left a comment
ExtraToast
left a comment
Contributor
There was a problem hiding this comment.
Review comments made by CodeQL, dismiss and address appropriately, then re-requeested review
…ens (#464, #465) Fixes CodeQL java/log-injection (×10) and java/sensitive-log (×3). - Add LogSanitizer.forLog() extension in shared/util — strips CR/LF from user-controlled values before they reach log call sites. - EmailTrackingController: drop raw tracking token from all three log statements; log outbox.id only when found, remove token from the not-found and error branches entirely. Resolves java/sensitive-log ×3 and the corresponding java/log-injection hits for this file. - BrevoListAdapter: sanitize user-supplied list name at all three createList log calls (lines 51, 57, 60). - MockContactAdapter: sanitize list name at createList log call (line 84). - ForwardAuthController: sanitize X-Forwarded-Host header value at the unknown-host warn (line 66). - ExceptionLoggingResolver: sanitize request.requestURI and ex.message at the catch-all error log (line 41). - FileService: sanitize multipart.originalFilename at the storeMultipart info log (line 92). Closes #464 Closes #465 Co-Authored-By: Joris Jonkers <info@jorisjonkers.dev>
CodeQL (java/log-injection) flagged two contact-email interpolations the initial hardening pass missed: the created-contact and deleted-contact log lines embed the user-provided email verbatim. Wrap both with .forLog() to strip CR/LF, matching the sanitizer already applied to the list-name and filename call sites. Co-Authored-By: Joris Jonkers <info@jorisjonkers.dev>
jorisjonkers-dev-agents
Bot
force-pushed
the
fix/log-security-hardening
branch
from
July 18, 2026 06:30
ae2bf75 to
e9470cf
Compare
Contributor
Author
|
Addressed the additional CodeQL Sanitized (wrapped user-provided value with
These two contact-email interpolations were the remaining unsanitized user-influenced String values within the files this PR touches; the previously-flagged sites ( Commit: e9470cf |
ExtraToast
requested changes
Jul 18, 2026
ExtraToast
left a comment
ExtraToast
left a comment
Contributor
There was a problem hiding this comment.
There's still a lot of codeql notifications on this, fix them.
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
2 participants
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
LogSanitizer.forLog()extension (strips CR/LF) applied at every CodeQL-flagged log site to neutralize log injection (CodeQLjava/log-injection×10, Neutralize log injection in user-influenced log statements (CodeQL java/log-injection) #464).EmailTrackingControllerto stop logging a sensitive capability value (CodeQLjava/sensitive-log×3, Stop logging sensitive email tracking tokens (CodeQL java/sensitive-log) #465); theoutbox.idis retained where a correlator is needed.Changes
New helper —
shared/util/LogSanitizer.ktString?.forLog()replaces[\r\n]with underscores and handles nulls. Applied consistently at every flagged site so the pattern is greppable.EmailTrackingController(lines 53, 55, 57)Drops
tokenfrom all three log calls. The success path logsoutbox.idonly; the not-found and error paths omit any token reference. This closes both thejava/sensitive-logalerts (#465) and the correspondingjava/log-injectionhits for this file (#464).BrevoListAdapter(lines 51, 57, 60)User-supplied list
namewrapped in.forLog()at all threecreateListlog calls.MockContactAdapter(line 84)Same
name.forLog()fix in the mock adapter'screateListlog.ForwardAuthController(line 66)X-Forwarded-Hostheader value sanitized before the unknown-host warn log.ExceptionLoggingResolver(line 41)request.requestURIandex.messageboth sanitized in the catch-all error log.FileService(line 92)multipart.originalFilenamesanitized before the store info log.Test plan
java/log-injectionalerts and all 3java/sensitive-logalerts.Closes #464
Closes #465