fix(security): neutralize CR/LF in rendered log records (CWE-117)

[groupthinking]

Canonical issue

Closes #1271.

The central formatter fix that #810's confirmed-but-unmitigated review thread
(#810 discussion)
identified as living outside #810's router.py scope.

Outcome

Every rendered log record now emits as a single physical line. Attacker-controlled
line separators can no longer forge, corrupt, or split downstream log entries —
including via logger.error(..., exc_info=True), logger.exception(...), and
structured extra fields — not just the inline-sanitized message string. JSON
logs stay parseable.

Scope

• src/youtube_extension/backend/config/logging_config.py — StructuredFormatter.format()
  escapes every str.splitlines() boundary (LF, CR, VT, FF, FS, GS, RS, NEL, LS, PS)
  plus ESC in the final rendered record, via a shared
  sanitize_log_record() / _UNSAFE_LOG_CHARS table. Escapes are emitted as
  JSON-valid \uXXXX sequences; backslash is escaped first so the transform is
  unambiguous and reversible.
• tests/unit/test_logging_config_crlf.py — regression tests asserting against
  rendered handler output (message, exc_info traceback, logger.exception,
  extra fields), JSON-log parseability, reversibility, full splitlines-boundary
  coverage, and a benign-record no-op.

Single-purpose: this PR contains only the CWE-117 formatter hardening. (An earlier
revision carried an unrelated perf commit; that work has its own canonical home in
#1269 and has been dropped from this branch.)

Risk

• Risk level: low
• Failure mode: multi-line tracebacks now render on one line with separators shown
  as \uXXXX escapes. Content is fully preserved and losslessly reversible; JSON
  logging is improved (a raw newline or a non-JSON escape in %(message)s would
  previously have broken JSON parsing).
• Rollback: revert this commit; the formatter returns to prepend-only behavior.

Verification

Tied to head b393c8f.

• Focused tests — pytest tests/unit/test_logging_config_crlf.py → 9 passed
• Addresses all three Copilot findings — see the summary comment below.
• Required engineering CI green on the prior head (CodeQL, Security Scan JS,
  lint-python, lint-frontend, guards, bandit, npm-audit, python-safety, gitleaks,
  dependency-review, trivy, "Canonical issue and evidence"); re-running on this head.
• Review threads — Copilot findings addressed.

Production evidence

Not applicable to runtime behavior: this is a Python-only logging change with no
web surface. The apps/web Vercel preview does not exercise it. Correctness is
proven by the rendered-output regression suite above rather than a deployment.

Agent provenance

Human-authored (repo owner's account) with Claude Code assistance, per the
Co-Authored-By trailer on the commit. Kept as an owner PR without an agent-lock
manifest, consistent with the repo's convention for owner-authored changes.
Awaiting final human review and merge approval to protected main.

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
v0-uvai	Ready	Preview, v0	Aug 7, 2026 5:57pm

[coderabbitai]

Important

Review skipped

Auto reviews are limited based on label configuration.

🏷️ Required labels (at least one) (1)

• [‘architecture-gap’, ‘bug’, ‘ci-cd’, ‘ci/cd’, ‘copilot-rabbit’, ‘documentation’, ‘duplicate’, ‘enhancement’, ‘frontend’, ‘github_actions’, ‘good first issue’, ‘help wanted’, ‘high-priority’, ‘invalid’, ‘javascript’, ‘ml-model’, ‘needs-triage’, ‘pipeline-critical’, ‘placeholder-code’, ‘priority:high’, ‘python’, ‘python:uv’, ‘question’, ‘styling’, ‘tests’, ‘v0’]

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited)

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 683d5fdc-1f07-442f-bcba-5947c88bc49c

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Snapshot Warnings

⚠️: No snapshots were found for the head SHA f76669a.

Ensure 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 Files

None

[github-actions]

Agent Completion Truth Gate: BLOCKED

Reasons: invalid_payload

Machine-readable verdict

{
  "details": {
    "collection_errors": [
      "incomplete_linked_issue_contract",
      "linked_issue_not_agent_task",
      "missing_intent_snapshot",
      "missing_agent_run_id",
      "missing_agent_login"
    ],
    "invalid_fields": [
      "policy.agent_login",
      "policy.run_id"
    ]
  },
  "reasons": [
    "invalid_payload"
  ],
  "verdict": "blocked"
}

Workflow evidence

[Copilot]

Pull request overview

Hardens rendered logs against CWE-117 injection and carries an unrelated GitHub upload performance optimization.

Changes:

• Escapes line separators in finalized log records.
• Adds rendered-output security regression tests.
• Offloads upload file reading and encoding from the event loop.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
logging_config.py	Adds centralized log sanitization.
test_logging_config_crlf.py	Tests message, traceback, and extra-field sanitization.
deployment_manager.py	Moves file reading and encoding to worker threads.
test_deployment_manager.py	Tests upload concurrency, integrity, errors, and cancellation.

Suppressed comments (2)

src/youtube_extension/backend/config/logging_config.py:34

• The separator table still omits U+001C (file separator), U+001D (group separator), and U+001E (record separator). Python treats all three as line boundaries, and U+001E is also used as a record delimiter, so attacker-controlled values containing them can still split downstream records despite the stated acceptance criterion. Escape these characters and include them in the regression input.

}

src/youtube_extension/backend/config/logging_config.py:31

• With enable_json_logging=True, these replacements are interpolated directly into a JSON string, but \v, \x1b, and \x85 are not valid JSON escapes. A record containing any of these characters remains single-line but becomes unparsable, allowing attacker-controlled input to corrupt or drop downstream JSON logs. Encode them as JSON-compatible \u000b, \u001b, and \u0085 sequences and update the expected output test.

    ord("\v"): "\\v",
    ord("\f"): "\\f",
    ord("\x1b"): "\\x1b",  # ESC — terminal control / escape sequences
    0x85: "\\x85",  # NEL (Unicode next line)

[groupthinking]

Remediation-routine disposition: HALTED(awaiting_human_governance_gate) — ready for your review, no autonomous merge.

Ran the per-PR terminal-state check on head 3f733f5:

• Engineering surface is green. Vercel READY; CodeQL, Security Scan, lint-python/frontend, bandit, npm-audit, python-safety, gitleaks, dependency-review, trivy all pass. CodeRabbit is skipped by this repo's label config. Focused suite tests/unit/test_logging_config_crlf.py → 6 passed.
• The only failing check is agent-completion/truth-gate/pr-1270 (invalid_payload) — red by design. As the PR body states, this routine deliberately did not fabricate a frozen pre-dispatch intent snapshot or a trusted terminal-agent-result, honoring fix(security): sanitize user-controlled values in API logs (CWE-117 log injection) #810's directive not to weaken or impersonate the gate. I am not overriding or "fixing" that gate; doing so would be the exact impersonation the gate exists to catch.

Next step is yours: final human review + merge approval to protected main. Nothing further will be pushed autonomously.

---

Generated by Claude Code

[groupthinking]

Thanks @copilot — all three findings were valid and are fixed on head b393c8f:

1. Missing separators (U+001C/1D/1E — FS/GS/RS). Added. The table is now the
   full set of str.splitlines() boundaries (LF, CR, VT, FF, FS, GS, RS, NEL, LS,
   PS) plus ESC. test_all_splitlines_boundaries_are_escaped builds its input from
   the table and asserts the result collapses to a single splitlines() element.

2. Invalid JSON escapes (\v, \x1b, \x85). Every replacement is now a
   JSON-valid \uXXXX sequence ( , �, , …) instead of a Python
   shorthand, so enable_json_logging=True records stay parseable.
   test_json_logging_output_stays_parseable renders through the module's JSON format
   and round-trips the message through json.loads.

3. Non-reversible transform. Backslash is now escaped first
   (ord("\\"): "\\\\"), so a real newline ( ) and a literal backslash-n
   (\\n) never collide. test_encoding_is_reversible pins this.

The unrelated perf commit that was previously carried here has been dropped — that
work lives in its own canonical PR #1269.

---

Generated by Claude Code

[groupthinking]

Daily-control containment: returned this PR to draft at its exact current head. Focused issue #1271 is linked, but ready state preceded reconciliation of the required active-agent receipt, exact-head independent review, and deployment evidence. No code or branch was discarded.

[groupthinking]

CWE-117 cluster consolidation — 6 competing PRs, empirically compared

Six open PRs implement CR/LF log-forging neutralization, five of them touching the same two files (backend/config/logging_config.py, tests/unit/test_logging_config_crlf.py): #810, #1255, #1263, #1265, #1266, #1270.

I loaded logging_config.py from each PR head and pushed every control/separator codepoint through StructuredFormatter.format(). All five parse and all five stop a basic \r\n injection. They differ only in which codepoints they cover:

PR	NUL 0x00	ESC 0x1b	FS/GS/RS 0x1c-1e	LF/CR/VT/FF	NEL/LS/PS	Also fixes
#1263	—	—	—	yes	yes	—
#1265	—	yes	—	yes	yes	intelligent_cache.py (conflicting)
#1266	—	—	yes	yes	yes	intelligent_cache.py
#1255	yes	—	yes	yes	yes	—
#1270	—	yes	yes	yes	yes	—
#810	n/a — call-site approach					logsafe.py, api/v1/router.py, video_processing_service.py

Findings

1. fix(security): neutralize CWE-117 log forging in StructuredFormatter #1263 has zero unique coverage. It is strictly dominated by fix(security): neutralize CR/LF in rendered log records (CWE-117 formatter gap) #1255, fix(security): neutralize CR/LF in rendered log records (CWE-117) #1265, fix(security): neutralize CR/LF log forging in StructuredFormatter (CWE-117) #1266 and fix(security): neutralize CR/LF in rendered log records (CWE-117) #1270. Nothing is lost by closing it.
2. fix(security): neutralize CR/LF in rendered log records (CWE-117) #1265 is strictly dominated by fix(security): neutralize CR/LF in rendered log records (CWE-117) #1270 on the formatter, and it is currently CONFLICTING. Its only unique contribution is the intelligent_cache.py fix, which fix(security): neutralize CR/LF log forging in StructuredFormatter (CWE-117) #1266 also carries in a mergeable state.
3. No single PR is complete. fix(security): neutralize CR/LF in rendered log records (CWE-117) #1270 has the widest separator coverage and the best escaping design — \uXXXX output stays valid JSON when enable_json_logging is on, and it escapes backslash first so the encoding is reversible. But fix(security): neutralize CR/LF in rendered log records (CWE-117) #1270 does not neutralize NUL (0x00), and fix(security): neutralize CR/LF in rendered log records (CWE-117 formatter gap) #1255 is the only PR that does. A NUL reaching a C-based log shipper can truncate the record.

Proposed resolution

• Close fix(security): neutralize CWE-117 log forging in StructuredFormatter #1263 — strictly dominated, no unique coverage.
• Close fix(security): neutralize CR/LF in rendered log records (CWE-117) #1265 — strictly dominated by fix(security): neutralize CR/LF in rendered log records (CWE-117) #1270 and conflicting; intelligent_cache.py is preserved by fix(security): neutralize CR/LF log forging in StructuredFormatter (CWE-117) #1266.
• Take fix(security): neutralize CR/LF in rendered log records (CWE-117) #1270 as the formatter base, and add ord("\x00"): "\\u0000" to _UNSAFE_LOG_CHARS before merging, otherwise closing fix(security): neutralize CR/LF in rendered log records (CWE-117 formatter gap) #1255 regresses NUL coverage.
• Then close fix(security): neutralize CR/LF in rendered log records (CWE-117 formatter gap) #1255 once that codepoint is carried over.
• Keep fix(security): neutralize CR/LF log forging in StructuredFormatter (CWE-117) #1266 only for intelligent_cache.py; rebase it onto fix(security): neutralize CR/LF in rendered log records (CWE-117) #1270 so the two do not both rewrite logging_config.py.
• Keep fix(security): sanitize user-controlled values in API logs (CWE-117 log injection) #810 — it is the only call-site fix (logsafe.py plus router/service), which is defence-in-depth on top of the central sink rather than a duplicate of it.

Net: 6 PRs to 3, with strictly greater coverage than any of the 6 has today.

Reproduction

probe = [0x00,0x0A,0x0B,0x0C,0x0D,0x1B,0x1C,0x1D,0x1E,0x85,0x2028,0x2029]
fmt = StructuredFormatter("%(message)s")
for cp in probe:
    rec = logging.LogRecord("t", logging.INFO, "p", 1, "x=%s", (chr(cp),), None)
    print(hex(cp), chr(cp) not in fmt.format(rec))  # True == neutralized

[linear-code]

GRV-342