fix: scrub control chars from user strings before logging (CWE-117)#239
Merged
Conversation
The subgen completion webhook (event/file) and verification endpoints (canonical_path) log user-supplied strings; a carriage-return or newline would let an unauthenticated LAN caller forge or split log records. subarr has no auth by default, so these endpoints are reachable untrusted. New log_safe.scrub() replaces C0 control chars + DEL with U+FFFD; applied at the webhook log and the audio-lang override/propagation logs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| caller, | ||
| lang, | ||
| canonical, | ||
| scrub(canonical), |
| caller, | ||
| lang, | ||
| canonical, | ||
| scrub(canonical), |
| caller, | ||
| lang, | ||
| canonical, | ||
| scrub(canonical), |
| caller, | ||
| lang, | ||
| canonical, | ||
| scrub(canonical), |
| target["name"], | ||
| target["id"], | ||
| canonical_path, | ||
| scrub(canonical_path), |
| payload.event, | ||
| payload.file, | ||
| canonical, | ||
| scrub(payload.event), # webhook fields are untrusted (no auth by default) |
| payload.file, | ||
| canonical, | ||
| scrub(payload.event), # webhook fields are untrusted (no auth by default) | ||
| scrub(payload.file), |
| canonical, | ||
| scrub(payload.event), # webhook fields are untrusted (no auth by default) | ||
| scrub(payload.file), | ||
| scrub(canonical), |
…r-robust) The route-registration regression test read app.routes by handler `.name`, which is unreliable across the suite's module-reload + collection-order state: the route serves correctly at the right path (sibling request tests pass) while its route object's `.name` comes back empty in some collection orders. Adding tests/test_log_safe.py in this branch shifted CI's Linux collection order and exposed the latent fragility. Introspect by path+method instead — same single-vs-double-prefix coverage, no dependence on global module identity. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…pection The route-registration test introspected app_with_stub.app.routes. In CI the fixture's `.app` attribute and the request-serving app diverge, so the route table came back without these routes even though real requests to them return 200 (the sibling request tests pass in the same run). Both a name-based and a path-based introspection therefore failed on CI while passing locally. Rewrite to assert the contract through the front door — PUT/GET resolve at the single prefix, all methods 404 at the double prefix — which is robust and a truer statement of the double-prefix regression it guards. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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.
Closes the genuine half of the CodeQL
py/log-injectionfindings (2026-06-13 security triage).The subgen completion webhook logs
payload.event/payload.file, and the audio-language verification path logscanonical_path— all user-supplied via HTTP body. Since subarr ships with no auth by default, a LAN caller can put a carriage-return/newline in those fields and forge or split log records. Newlog_safe.scrub()replaces C0 control chars + DEL with U+FFFD so forged content stays inert and inline; applied at the four real sites (queue webhook, two audio_lang_store override logs, audio_lang sonarr-propagation log).The log-injection alerts the audit classified as false positives (fixed service names, already-validated Path objects) are left as-is. If CodeQL still flags the now-scrubbed sites on re-scan (it may not model our helper), they'll be dismissed with reason. Stack-trace-exposure findings are tracked separately — the no-auth reality flips those toward tightening (see #238).
🤖 Generated with Claude Code