fix(security): neutralize CR/LF log forging in StructuredFormatter (CWE-117) - #1266
fix(security): neutralize CR/LF log forging in StructuredFormatter (CWE-117)#1266groupthinking wants to merge 4 commits into
Conversation
invalidate_by_tags() issued smembers+delete one tag at a time, so the caller's wall-clock latency grew linearly with tag count and stale entries stayed readable for the whole window. Fan the per-tag work out with asyncio.gather, reusing the per-layer tag-write semaphore already introduced for set() so the combined fan-out cannot exhaust the shared connection pool. One permit covers both commands for a tag since the delete depends on the smembers result. Closes #1261. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
CodeRabbit's adversarial review of #1262 found the permit-scope comment factually wrong: it claimed holding one permit across the smembers/delete pair keeps concurrently-held pool connections at the permit count "instead of twice it". redis.asyncio checks a connection out only for each command and returns it to the pool between the two awaits, so acquiring the permit per command would cap in-flight commands at the same limit. The real reason to hold across the pair is scheduling determinism and avoiding permit churn, not preventing a doubling of held connections. - Rewrite the inline comment in invalidate_by_tags() to state the true policy. - Fix the matching docstring of test_invalidate_holds_one_permit_across_both_commands. - Add test_invalidate_starts_no_command_after_context_exit_on_cancel, the cancellation regression guard CodeRabbit asked for: cancel mid-smembers and assert no Redis command starts after the connection context manager exits. No behaviour change; scheduling and return values are untouched. 163 passed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RJP3fieq6JJdEAZXQsJ3J6
The cancellation guard added in e0ebdb3 passes against the pre-change serial implementation on main, so it cannot detect a regression back to sequential invalidation, and its smembers mock has no try/finally, so it never observes whether a cancelled child actually unwound. Measured: running it against `git show origin/main:intelligent_cache.py` yields 1 passed. Replace it with test_invalidate_cancellation_drains_before_conn_closes, which asserts a strict superset: - all three tags are in flight concurrently, via a bounded wait that fails with a diagnostic instead of hanging when they are not; - every cancelled child runs its finally before __aexit__ closes the connection (recorded by a try/finally in the smembers mock); - no Redis command starts after __aexit__ (the original assertion). Against pristine main this test fails, taking the class prove-fail count from 3 to 4. Also notes in the permit-scope comment that holding one permit across the pair bounds how many tags can sit half-invalidated when a delete fails. Full file 163 passed; wider sweep 474 passed; ruff parity unchanged. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…WE-117) StructuredFormatter did not sanitize the rendered log record, so any user-controlled value carrying CR/LF — interpolated into a message, an `exc_info=True` traceback (`str(exc)`), or a structured `extra` field — could splice a forged log line (CWE-117). This was confirmed against rendered handler output, not just a helper's return value. Fix at the single formatter chokepoint: escape CR/LF and the other line separators in the fully rendered record so every sink is covered at once and each record occupies exactly one physical line. Escaping (rather than dropping) keeps the original text greppable. Adds focused regression tests asserting on rendered stream output for the message vector, the traceback vector, all targeted separators, and that a clean payload is left unescaped. Sibling to the per-sink `_safe_log()` hardening tracked on #810/#913: this addresses the traceback/extras vector centrally on `main`, independent of that PR's router-scoped change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Mb4JRePtEQKAgaDEZHicqA
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
Next review available in: 42 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. Scanned FilesNone |
Agent Completion Truth Gate: BLOCKEDReasons: Machine-readable verdict{
"details": {
"invalid_fields": [
"issue.number",
"policy.agent_login",
"policy.run_id"
]
},
"reasons": [
"invalid_payload"
],
"verdict": "blocked"
} |
|
Status — automated remediation run (head This PR lands a self-contained CWE-117 fix: CR/LF (and other line separators) are now neutralized centrally in Verified locally on
Blocker — Next step (human/owner) — pick one:
Not auto-merging to protected I'll keep watching this PR: if CodeRabbit (now enabled via the added labels) or CI surfaces anything actionable on the diff itself, I'll push a fix. Generated by Claude Code |
|
@coderabbitai review Requesting a single review of this CWE-117 log-forging fix ( Generated by Claude Code |
|
✅ Action performedReview finished.
|
2 participants
Canonical issue
Progresses #913 / #898 (CWE-117 log-injection hardening). Sibling to #810.
Outcome
StructuredFormatternow neutralizes CR/LF (and other line separators) in thefully rendered log record, so no user-controlled value — interpolated into a
message, an
exc_info=Truetraceback, or a structuredextrafield — can forgea fake log line. Each record is guaranteed to occupy exactly one physical line.
Scope
src/youtube_extension/backend/config/logging_config.py— escape lineseparators at the single
StructuredFormatter.format()chokepoint.tests/unit/test_logging_config_crlf.py— regression tests asserting onrendered stream output.
_safe_log()router hardening tracked on fix(security): sanitize user-controlled values in API logs (CWE-117 log injection) #810 (raq7mgbranch). This PR addresses the traceback/
extravector centrally onmain, independent of that change, and does not touchrouter.py.Risk
line instead of multiple lines. Text is preserved and greppable; this is also
required for correctness under JSON logging, where embedded newlines break the
per-line JSON object.
Verification
Tied to head
9ee059c:tests/unit/test_logging_config_crlf.py: 4 passed(message vector,
exc_infotraceback vector, all targeted separators, cleanpayload unchanged). Asserted against rendered handler output, not a
helper's return value.
own physical line via both message and traceback) and its elimination after
(9 physical lines → 2; no forged line-starts).
ruff checkclean on both changed files.Production evidence
Not applicable to a Vercel preview: this is a Python-only backend logging change
and is not exercised by the Next.js
apps/webbuild. Behavior is verified by thefocused unit tests and the before/after reproduction above.
Agent handoff
(
_safe_log()per sink); this is the central formatter fix it explicitlyrecommended as a sibling.
main🤖 Generated with Claude Code
https://claude.ai/code/session_01Mb4JRePtEQKAgaDEZHicqA
Generated by Claude Code