fix(logging): scrub control characters from exc_text and stack_info too - #175
Closed
wshallwshall wants to merge 1 commit into
Closed
fix(logging): scrub control characters from exc_text and stack_info too#175wshallwshall wants to merge 1 commit into
wshallwshall wants to merge 1 commit into
Conversation
… (BACKLOG #335) `ControlCharScrubFilter` escaped only the rendered message, so a CR/LF inside a logged exception traceback reached the human-readable text sink verbatim -- enough to forge a whole log record. `Formatter.format` appends `exc_text` and `stack_info` AFTER the message the filter was cleaning, so the two fields most likely to carry attacker-shaped content were the two it never saw. Tracebacks are legitimately multi-line, so this does NOT collapse them to one line -- that would trade a forgery risk for an unreadable log. Each physical line is stamped with a continuation marker instead, which keeps the traceback readable while making every line unmistakably a continuation rather than a new record. Scope is the item's: the JSON sink was already unaffected (it encodes, so control characters cannot break framing there), and the reachable call sites are few. PROVEN TO FAIL: disabling the exc_text branch turns test_control_char_filter_scrubs_exception_traceback red; restoring turns it green. The test asserts on the scrubbed traceback rather than on the filter being called, so it cannot pass by the code merely executing. ADR 0034's accepted-risk register is updated in the same commit -- it recorded this as an accepted residual, and leaving that entry standing after the residual is closed is the banner-rot this project has a gate for. 42 tests pass; ruff + mypy clean.
wshallwshall
enabled auto-merge (squash)
August 4, 2026 12:46
This was referenced Aug 4, 2026
Collaborator
Author
|
Superseded by #177, which batches this with the other two small code changes into one CI cycle. Nothing is dropped — this commit is carried across verbatim in #177. Closing rather than leaving it open because 🤖 Batched by Claude Code |
auto-merge was automatically disabled
August 4, 2026 14:09
Pull request was closed
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.
Implements BACKLOG #335.
ControlCharScrubFilter.filternow applies the translation table torecord.exc_textandrecord.stack_info, not only the message — so a control character arriving via a traceback is scrubbed like any other.Tracebacks stay multi-line with a continuation marker rather than being collapsed, which keeps them readable while removing the injection surface. ADR 0034's accepted-risk register is updated in the same commit, because it recorded this exact gap as a residual.
Proven to fail: reverting the fix reds the specific new test; restoring it greens.
This flips the item's banner to
✅ DONEand updates its ranked-table row, but does not update the four census lines or the tier counts. So on merge,docs/BACKLOG.mdwill assert an open-item count one higher than the truth.No gate catches that.
backlog_status_check.pyis explicitly structural — its own docstring says it "cannot know whether a banner is truthful, only" the structure — so it will pass. This is the same shape as the two-dot hygiene diff and the other green-and-blind instruments: it merges clean and publishes a wrong count.Not fixed here on purpose.
docs/BACKLOG.mdhas one writer at a time, and the standing rule is re-derive the census, never apply a delta — so the correct fix is for the next commit that legitimately holds that file to re-derive all four lines and the tier counts from the merged content, accounting for this closure and everything else landed by then. The next such commit is already queued and will do it.