Skip to content

fix(security): sanitize the remaining log-injection sinks, guard the nightly allowlist - #1098

Merged
philmerrell merged 3 commits into
developfrom
fix/log-injection-and-nightly-allowlist-guard
Sep 14, 2026
Merged

philmerrell merged 3 commits into
developfrom
fix/log-injection-and-nightly-allowlist-guard

Conversation

@philmerrell

Copy link
Copy Markdown
Contributor

Follow-up to the CodeQL findings surfaced by the 1.21.0 release PR (#1097). Neither is a regression from that release — CodeQL runs only on main, so a release PR is the first time a whole release's code meets it, and 12 of these 17 sites have been on main for some time.

Deliberately not folded into #1097: see "Why this is separate" below.

1. py/log-injection — 17 sites

scrub_log() already exists, already names this rule in its docstring, and already has 162 adopted call sites. None of the files that use it appear in the alert list — which is the empirical proof that CodeQL models it as a sanitizer. So this was never a missing mechanism; these are missed call sites.

A handler-level logging filter was the obvious alternative and is the wrong fix. It would neutralize the values at emit time but is invisible to taint analysis, so all 17 alerts would stand while the PR looked like it had fixed them.

Two shapes, both wrapped:

  • Message-level — a user-controlled path param, id or exception interpolated into the message (model_id, hf_id, instance_type, skill_id, session_id, prompt_name, rag_assistant_id).
  • extra={...} — several sites already scrubbed the f-string but carried the raw value in the structured dict. The current formatter doesn't render extra, so it isn't a sink today; it becomes one under a structured formatter, and a half-scrubbed call reads as though it were already handled. Wrapped for the whole dict in each file touched — not only the two arms CodeQL flagged — so the unflagged siblings can't drift back.

Deliberately unchanged:

Site Why
shares/service.py:683 Sanitized by ShareService._sanitize_id, an allowlist regex stricter than scrub_log. CodeQL doesn't model it. Stacking a no-op scrub_log on top to satisfy the analyzer would make the code worse — this alert wants dismissing, not code.
shared/rbac/admin_service.py Same extra shape, not flagged. Left alone rather than widening this diff into files it doesn't otherwise touch.
config_cache.py:178 The py/clear-text-logging-sensitive-data high. key is one of five module constants; it fired on the parameter name. False positive — wants dismissing.

2. The nightly branch allowlist had nothing pinning it

CodeQL reports 8 high-severity actions/cache-poisoning/* on nightly.yml. They're already mitigated: the workflow runs privileged (schedule/workflow_dispatch, so its jobs can write the default-branch Actions cache scope), and its parser resolves track tokens through a case that assigns literal "main"/"develop" and refuses anything else with exit 1. The header comment cites the CWE by name.

But CodeQL can't see through a shell case — so the alert stays, and the allowlist that makes it a false positive had no test. Widening that case statement, dropping the exit 1, or deriving a ref from the token would silently make a standing high-severity finding real, and nothing would fail.

Four guards, each mutation-verified:

Guard Mutation Result
Refs are allowlisted literals test_backend_ref="feature/evil" ✅ fails
Refs are never shell expansions test_backend_ref="${token#test-backend-}" ✅ fails
Unknown branches exit 1, not warn exit 1echo soft-fail ✅ fails
No checkout ref from event/inputs ref: ${{ inputs.tracks }} ✅ fails

Why this is separate from the release

These findings predate 1.21.0, so shipping it without them leaves prod exactly where it is today on this axis. Folding 9 source files — including inference_api/chat/routes.py, the hot path — into a 58-PR release that has already cleared its gates would trade a real if small regression risk against zero urgency, and would invalidate the review and the written release notes. CodeQL isn't a required check on main, so nothing is blocked either way.

Verification

  • 4821 passed, 3 skipped (full backend suite)
  • All four new guards mutation-verified above; nightly.yml restored byte-clean after each

🤖 Generated with Claude Code

philmerrell and others added 3 commits September 13, 2026 21:38
…nightly allowlist

Two CodeQL findings surfaced by the 1.21.0 release PR, neither of which is a
regression from that release — CodeQL runs only on `main`, so a release PR is
the first time a whole release's code meets it, and 12 of these 17 sites have
been on main for some time.

## py/log-injection — 17 sites

`scrub_log()` already exists and already names this rule, with 162 adopted call
sites. None of the files that use it appear in the alert list, which is the
empirical proof that CodeQL models it as a sanitizer. So these were never a
missing mechanism — they are missed call sites.

A handler-level logging filter was the obvious alternative and is the wrong
fix: it would neutralize the values at emit time but is invisible to taint
analysis, so it would leave all 17 alerts standing while looking like a fix.

Two shapes, both wrapped:

- Message-level — a user-controlled path param, id or exception interpolated
  into the log message (`model_id`, `hf_id`, `instance_type`, `skill_id`,
  `session_id`, `prompt_name`, `rag_assistant_id`).
- `extra={...}` — several sites already scrubbed the f-string message but
  carried the raw value in the structured dict. The current formatter does not
  render `extra`, so it is not a sink *today*; it becomes one under a
  structured formatter, and the half-scrubbed call reads as if it were already
  handled. Wrapped for the whole dict in each file touched, not only the two
  arms CodeQL flagged, so the unflagged siblings cannot drift back.

Deliberately NOT changed: `shares/service.py` sanitizes with
`ShareService._sanitize_id`, an allowlist regex that is *stricter* than
`scrub_log`. CodeQL does not model it; adding a no-op `scrub_log` on top to
satisfy the analyzer would make the code worse. That alert wants dismissing,
not code. `shared/rbac/admin_service.py` carries the same `extra` shape and was
not flagged — left alone rather than widening this diff into files it does not
otherwise touch.

## The nightly branch allowlist had nothing pinning it

CodeQL reports 8 high-severity `actions/cache-poisoning/*` on nightly.yml.
They are already mitigated: the workflow runs privileged (schedule/dispatch, so
it can WRITE the default-branch Actions cache scope), and its parser resolves
track tokens through a `case` that assigns literal "main"/"develop" and refuses
anything else with `exit 1`. The header comment cites the CWE by name.

But CodeQL cannot see through a shell `case`, so the alert stays — and the
allowlist that makes it a false positive had no test. Widening that case
statement, dropping the `exit 1`, or deriving a ref from the token would
silently make a standing high-severity finding real, and nothing would fail.

Adds four guards, each mutation-verified:

- refs are allowlisted literals      (mutation: ref="feature/evil" -> fails)
- refs are never shell expansions    (mutation: ref="${token#...}" -> fails)
- unknown branches exit 1, not warn  (mutation: exit 1 -> echo    -> fails)
- no checkout ref from event/inputs  (mutation: ref: inputs.tracks -> fails)

Verified: 4821 backend tests pass.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…as the check

Two documentation defects, both hit for real during the v1.21.0 prod backfill.

## `python` is not the interpreter that can run these

Every one of the six `backfill_*.py` docstrings showed `AWS_PROFILE=… python
backend/scripts/…`, which assumes an already-activated venv and otherwise dies
at `import boto3` before doing anything:

    ModuleNotFoundError: No module named 'boto3'

That form was copied into the v1.21.0 release notes, so the one instruction
whose entire purpose is reaching an operator without context was the one that
could not be pasted. All six now name `backend/.venv/bin/python`, verified by
running each with `--help`. Nothing here imports `apis.*` except
`backfill_skill_bundles.py`, which puts `../src` on `sys.path` itself — so the
venv's interpreter is the only requirement for all six, from the repo root.

## `describe-table` ItemCount cannot verify a fresh backfill

Both the release notes and the skill template said to verify that "the index
item count matches the tool count". DynamoDB refreshes table and index
ItemCount roughly every SIX HOURS, so a correct prod backfill
(`stamped=31 skipped=0 failed=0`) still reported:

    Table.GlobalSecondaryIndexes[?IndexName=='EntityTypeIndex'].ItemCount
    [ 0 ]

— indistinguishable, to the operator reading it, from the backfill silently
doing nothing. The check now given is a live `query … --select COUNT` on the
index, paired with a scan for rows still lacking `GSI5PK`. That second half is
not belt-and-braces: a PARTIAL backfill is precisely the case the read path's
zero-result fallback does not cover, by design, because detecting it would mean
scanning every time — which is the cost the index removes.

Both fixes land in the skill template too, so the next release inherits the
corrected guidance rather than reproducing this.

Earlier RELEASE_NOTES.md entries keep the old form: they are the historical
record of what shipped, and the skill is explicit that previous entries are
never edited. The scripts' own docstrings are where anyone actually lands.

Verified: all six scripts run as documented; 112 supply-chain + backfill tests pass.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@philmerrell

Copy link
Copy Markdown
Contributor Author

Added a third commit (d5e3c5e2) fixing the two documentation defects this PR's own subject matter turned up during the v1.21.0 prod backfill.

1. python is not an interpreter that can run these. All six backfill_*.py docstrings showed AWS_PROFILE=… python backend/scripts/…, which assumes an activated venv and otherwise dies at import boto3 before doing anything. That form was copied into the v1.21.0 release notes — so the one instruction whose entire purpose is reaching an operator without context was the one that couldn't be pasted. All six now name backend/.venv/bin/python; each verified with --help.

2. describe-table ItemCount cannot verify a fresh backfill. The notes and the skill template both said to check "the index item count matches the tool count". DynamoDB refreshes item counts roughly every six hours, so a correct prod run (stamped=31 skipped=0 failed=0) still printed [ 0 ] — indistinguishable from the backfill having done nothing. Replaced with a live query … --select COUNT, paired with a scan for rows still missing GSI5PK, because a partial backfill is exactly the case the read path's zero-result fallback does not cover by design.

Both fixes also land in .claude/skills/cutting-a-release/SKILL.md, so the next release inherits corrected guidance instead of reproducing this.

Earlier RELEASE_NOTES.md entries deliberately keep the old form — they're the historical record of what shipped, and the skill is explicit that previous entries are never edited. The scripts' own docstrings are where anyone actually lands.

Verified: all six scripts run as documented; 112 supply-chain + backfill tests pass. Branch also has origin/develop merged in, so it carries the 1.21.0 notes it edits.

@philmerrell
philmerrell merged commit 86edd8a into develop Sep 14, 2026
6 checks passed
@philmerrell
philmerrell deleted the fix/log-injection-and-nightly-allowlist-guard branch September 14, 2026 18:55
philmerrell added a commit that referenced this pull request Sep 15, 2026
`_resolve_kb_usage` interpolated `assistant_id` — a user-controlled path
parameter — straight into a `logger.warning` f-string, so `\r`/`\n` in it could
forge additional log lines (CodeQL `py/log-injection`, alert #864, medium).

New in this release: the sink arrived with the KB storage usage bar (8fb4f8f,
#1108), on a branch parallel to the #1098 sweep that sanitized every other
instance — so the release was about to ship a regression against a rule it
enforces elsewhere. Fixed to that sweep's own convention: `%s` lazy formatting
with `scrub_log()` on each user-influenced value.

Scanned every logger call this release ADDED for the same shape; the rest carry
exceptions, ints, or already-scrubbed values. The pre-existing f-string logger
calls elsewhere in the tree are deliberately left alone — that is a sweep, not
release-branch work.

Backend suite: 8626 passed, 3 skipped.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Sep 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant