From d8afe2e3be56ab40ce3a0ac241280d7f25475061 Mon Sep 17 00:00:00 2001 From: Phil Merrell Date: Sun, 13 Sep 2026 21:38:31 -0600 Subject: [PATCH 1/2] fix(security): sanitize the remaining log-injection sinks, guard the nightly allowlist MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../apis/app_api/admin/roles/agent_pins.py | 2 +- .../app_api/admin/services/model_icons.py | 9 +- .../src/apis/app_api/fine_tuning/routes.py | 9 +- backend/src/apis/app_api/sessions/routes.py | 2 +- backend/src/apis/app_api/skills/routes.py | 5 +- backend/src/apis/app_api/skills/service.py | 16 +-- .../src/apis/app_api/skills/user_service.py | 6 +- backend/src/apis/app_api/tools/discovery.py | 6 +- backend/src/apis/inference_api/chat/routes.py | 5 +- .../test_nightly_ref_allowlist.py | 131 ++++++++++++++++++ 10 files changed, 169 insertions(+), 22 deletions(-) create mode 100644 backend/tests/supply_chain/test_nightly_ref_allowlist.py diff --git a/backend/src/apis/app_api/admin/roles/agent_pins.py b/backend/src/apis/app_api/admin/roles/agent_pins.py index f07e8ba59..02673a0a6 100644 --- a/backend/src/apis/app_api/admin/roles/agent_pins.py +++ b/backend/src/apis/app_api/admin/roles/agent_pins.py @@ -124,7 +124,7 @@ async def put_role_agent_pins( f"Admin {scrub_log(admin.email)} set {len(request.pins)} default pin(s) on role {scrub_log(role_id)}", extra={ "event": "role_agent_pins_updated", - "role_id": role_id, + "role_id": scrub_log(role_id), "pin_count": len(request.pins), "admin_user_id": admin.user_id, }, diff --git a/backend/src/apis/app_api/admin/services/model_icons.py b/backend/src/apis/app_api/admin/services/model_icons.py index f253ee4d6..8fcba9f77 100644 --- a/backend/src/apis/app_api/admin/services/model_icons.py +++ b/backend/src/apis/app_api/admin/services/model_icons.py @@ -24,6 +24,7 @@ model_icon_version, normalize_icon, ) +from apis.shared.security.log_sanitize import scrub_log logger = logging.getLogger(__name__) @@ -68,7 +69,9 @@ async def upload_model_icon(model_id: str, content: bytes) -> Tuple[Optional[str try: key = store.put(model_id=model_id, content=data, ext=ext, content_type=content_type) except IconStoreError as e: - logger.error(f"Icon storage unavailable for model {model_id}: {e}") + logger.error( + f"Icon storage unavailable for model {scrub_log(model_id)}: {scrub_log(e)}" + ) raise ModelIconError("Icon storage is unavailable.", status_code=503) from e previous = model.icon_key @@ -76,7 +79,7 @@ async def upload_model_icon(model_id: str, content: bytes) -> Tuple[Optional[str if previous and previous != key: store.delete(previous) - logger.info(f"🖼️ model-icons: uploaded icon for model {model_id}") + logger.info(f"🖼️ model-icons: uploaded icon for model {scrub_log(model_id)}") return key, model_icon_url(model_id, key) @@ -90,7 +93,7 @@ async def remove_model_icon(model_id: str) -> Tuple[Optional[str], Optional[str] if previous: get_model_icon_store().delete(previous) - logger.info(f"🖼️ model-icons: removed icon for model {model_id}") + logger.info(f"🖼️ model-icons: removed icon for model {scrub_log(model_id)}") return None, None diff --git a/backend/src/apis/app_api/fine_tuning/routes.py b/backend/src/apis/app_api/fine_tuning/routes.py index 4e94bc903..f357440db 100644 --- a/backend/src/apis/app_api/fine_tuning/routes.py +++ b/backend/src/apis/app_api/fine_tuning/routes.py @@ -42,6 +42,7 @@ from .inference_repository import InferenceRepository, get_inference_repository from .script_packaging_service import ScriptPackagingService, get_script_packaging_service from .dependencies import require_fine_tuning_access +from apis.shared.security.log_sanitize import scrub_log logger = logging.getLogger(__name__) @@ -285,7 +286,9 @@ async def preflight_huggingface_model(hf_id: str, spec) -> None: async with httpx.AsyncClient(timeout=10.0) as client: response = await client.get(f"https://huggingface.co/api/models/{hf_id}") except httpx.HTTPError as e: - logger.warning(f"HuggingFace pre-flight unavailable for {hf_id}: {e}") + logger.warning( + f"HuggingFace pre-flight unavailable for {scrub_log(hf_id)}: {scrub_log(e)}" + ) return if response.status_code == 404: @@ -298,7 +301,7 @@ async def preflight_huggingface_model(hf_id: str, spec) -> None: ) if response.status_code >= 400: logger.warning( - f"HuggingFace pre-flight returned {response.status_code} for {hf_id}" + f"HuggingFace pre-flight returned {response.status_code} for {scrub_log(hf_id)}" ) return @@ -425,7 +428,7 @@ def _budgeted_runtime( if effective < requested_seconds: logger.info( f"Clamped max runtime from {requested_seconds}s to {effective}s " - f"to fit ${remaining_usd:.2f} remaining on {instance_type}" + f"to fit ${remaining_usd:.2f} remaining on {scrub_log(instance_type)}" ) return effective diff --git a/backend/src/apis/app_api/sessions/routes.py b/backend/src/apis/app_api/sessions/routes.py index 69ad4e80b..47c01fee4 100644 --- a/backend/src/apis/app_api/sessions/routes.py +++ b/backend/src/apis/app_api/sessions/routes.py @@ -724,7 +724,7 @@ async def signal_turn_interrupted_endpoint( if not await is_session_lease_held(session_id, user_id): logger.info( "Ignoring navigated_away for session %s — no turn in flight", - session_id, + scrub_log(session_id), ) return Response(status_code=204) diff --git a/backend/src/apis/app_api/skills/routes.py b/backend/src/apis/app_api/skills/routes.py index 487c5dbb7..e528f1b92 100644 --- a/backend/src/apis/app_api/skills/routes.py +++ b/backend/src/apis/app_api/skills/routes.py @@ -62,6 +62,7 @@ UserSkillNotFoundError, get_user_skill_service, ) +from apis.shared.security.log_sanitize import scrub_log logger = logging.getLogger(__name__) @@ -548,7 +549,9 @@ async def get_accessible_skill( user: User = Depends(get_current_user_from_session), ) -> SkillDetailResponse: """Read one skill the current user can reach, catalog or self-authored.""" - logger.info(f"User {user.name} reading skill '{skill_id}'") + logger.info( + f"User {scrub_log(user.name)} reading skill '{scrub_log(skill_id)}'" + ) skill = await _require_accessible_skill(skill_id, user) diff --git a/backend/src/apis/app_api/skills/service.py b/backend/src/apis/app_api/skills/service.py index 5eae8f35f..8b4efeaa8 100644 --- a/backend/src/apis/app_api/skills/service.py +++ b/backend/src/apis/app_api/skills/service.py @@ -144,7 +144,7 @@ async def get_catalog_skill(self, skill_id: str) -> Optional[SkillDefinition]: "as not found", extra={ "event": "admin_skill_cross_tier_denied", - "skill_id": skill_id, + "skill_id": scrub_log(skill_id), }, ) return None @@ -237,7 +237,7 @@ async def update_skill( f"Admin {scrub_log(admin.email)} updated skill: {scrub_log(skill_id)}", extra={ "event": "skill_updated", - "skill_id": skill_id, + "skill_id": scrub_log(skill_id), "admin_user_id": admin.user_id, "admin_email": admin.email, "changes": list(updates.keys()), @@ -279,7 +279,7 @@ async def delete_skill( f"Admin {scrub_log(admin.email)} deleted skill: {scrub_log(skill_id)}", extra={ "event": "skill_deleted", - "skill_id": skill_id, + "skill_id": scrub_log(skill_id), "admin_user_id": admin.user_id, "admin_email": admin.email, "soft_delete": soft, @@ -401,10 +401,10 @@ async def add_resource( f"Admin {scrub_log(admin.email)} uploaded reference file to skill {scrub_log(skill_id)}", extra={ "event": "skill_resource_added", - "skill_id": skill_id, + "skill_id": scrub_log(skill_id), # NB: not "filename" — that key is reserved on LogRecord and # raises KeyError when the record is actually emitted. - "resource_filename": filename, + "resource_filename": scrub_log(filename), "size": len(content), "admin_user_id": admin.user_id, }, @@ -460,9 +460,9 @@ async def delete_resource( f"Admin {scrub_log(admin.email)} deleted reference file from skill {scrub_log(skill_id)}", extra={ "event": "skill_resource_deleted", - "skill_id": skill_id, + "skill_id": scrub_log(skill_id), # NB: not "filename" — reserved on LogRecord (see add_resource). - "resource_filename": filename, + "resource_filename": scrub_log(filename), "admin_user_id": admin.user_id, }, ) @@ -632,7 +632,7 @@ async def set_roles_for_skill( f"Admin {scrub_log(admin.email)} set roles for skill {scrub_log(skill_id)}", extra={ "event": "skill_roles_updated", - "skill_id": skill_id, + "skill_id": scrub_log(skill_id), "admin_user_id": admin.user_id, "roles_added": list(to_add), "roles_removed": list(to_remove), diff --git a/backend/src/apis/app_api/skills/user_service.py b/backend/src/apis/app_api/skills/user_service.py index 672748215..bcbfe3a7f 100644 --- a/backend/src/apis/app_api/skills/user_service.py +++ b/backend/src/apis/app_api/skills/user_service.py @@ -183,7 +183,7 @@ async def create_my_skill( f"User {scrub_log(user.email)} created skill: {scrub_log(skill_id)}", extra={ "event": "user_skill_created", - "skill_id": skill_id, + "skill_id": scrub_log(skill_id), "owner_user_id": user.user_id, }, ) @@ -213,7 +213,7 @@ async def update_my_skill( f"User {scrub_log(user.email)} updated skill: {scrub_log(skill_id)}", extra={ "event": "user_skill_updated", - "skill_id": skill_id, + "skill_id": scrub_log(skill_id), "owner_user_id": user.user_id, "changes": list(updates.keys()), }, @@ -242,7 +242,7 @@ async def delete_my_skill(self, skill_id: str, user: User) -> None: f"User {scrub_log(user.email)} deleted skill: {scrub_log(skill_id)}", extra={ "event": "user_skill_deleted", - "skill_id": skill_id, + "skill_id": scrub_log(skill_id), "owner_user_id": user.user_id, }, ) diff --git a/backend/src/apis/app_api/tools/discovery.py b/backend/src/apis/app_api/tools/discovery.py index 9fec082d9..a88db7b73 100644 --- a/backend/src/apis/app_api/tools/discovery.py +++ b/backend/src/apis/app_api/tools/discovery.py @@ -38,6 +38,7 @@ ToolDefinition, _clip, ) +from apis.shared.security.log_sanitize import scrub_log logger = logging.getLogger(__name__) @@ -367,7 +368,10 @@ def _get() -> ResolvedPrompt: return await asyncio.to_thread(_get) except Exception as exc: # noqa: BLE001 - surfaced as a 502 by the route logger.warning( - "prompts/get failed for %s/%s: %s", tool.tool_id, prompt_name, exc + "prompts/get failed for %s/%s: %s", + scrub_log(tool.tool_id), + scrub_log(prompt_name), + scrub_log(exc), ) raise RuntimeError(f"The server could not compose that prompt: {exc}") from exc diff --git a/backend/src/apis/inference_api/chat/routes.py b/backend/src/apis/inference_api/chat/routes.py index 3bc00454f..14d9fd29e 100644 --- a/backend/src/apis/inference_api/chat/routes.py +++ b/backend/src/apis/inference_api/chat/routes.py @@ -2001,7 +2001,10 @@ async def invocations(request: InvocationRequest, current_user: User = Depends(g if await bump_last_used_at(input_data.rag_assistant_id): await resume_inactive_policies(input_data.rag_assistant_id) except Exception as bump_err: - logger.warning(f"lastUsedAt bump failed for assistant {input_data.rag_assistant_id}: {bump_err}") + logger.warning( + f"lastUsedAt bump failed for assistant " + f"{scrub_log(input_data.rag_assistant_id)}: {scrub_log(bump_err)}" + ) # 2b. Agent Designer Phase 3 — resolve the Agent's governed capabilities # for the INVOKING user (D5), before the expensive KB search. v1 blocks diff --git a/backend/tests/supply_chain/test_nightly_ref_allowlist.py b/backend/tests/supply_chain/test_nightly_ref_allowlist.py new file mode 100644 index 000000000..2f4d47c82 --- /dev/null +++ b/backend/tests/supply_chain/test_nightly_ref_allowlist.py @@ -0,0 +1,131 @@ +"""Guard the nightly workflow's branch allowlist. + +``nightly.yml`` runs on ``schedule``/``workflow_dispatch``, so its jobs execute +in the context of the default branch and hold a token that can **write** the +default-branch GitHub Actions cache scope. Checking out an arbitrary ref there +would let unreviewed code run while holding that token and poison cache entries +that privileged workflows later restore (CWE-349; CodeQL +``actions/cache-poisoning/*``). + +The workflow defends against this by resolving track tokens through a ``case`` +statement that assigns **literal** ``"main"`` / ``"develop"`` strings, and by +refusing any other branch with a hard ``exit 1``. CodeQL cannot see through a +shell ``case``, so it still reports the checkout steps — which means the only +thing standing between this repo and that finding being real is the allowlist +itself, with nothing asserting it stays intact. + +These tests are that assertion. They fail if someone widens the allowlist, +drops the deny branch, or lets attacker-influenced text reach a ``ref:``. +""" + +import re +from pathlib import Path + +import pytest +import yaml + +# Repository root is 3 levels up from backend/tests/supply_chain/ +REPO_ROOT = Path(__file__).resolve().parents[3] +NIGHTLY = REPO_ROOT / ".github" / "workflows" / "nightly.yml" + +# The only branches a privileged nightly run may check out. Both are +# protected/PR-only, so anything running from them has been reviewed. +ALLOWED_REFS = {"main", "develop"} + +# `_ref=""` assignments in the resolve-tracks shell step. +_REF_ASSIGNMENT = re.compile(r'^\s*(\w*_ref)="([^"]*)"', re.MULTILINE) + + +@pytest.fixture(scope="module") +def nightly_text() -> str: + assert NIGHTLY.is_file(), f"missing workflow: {NIGHTLY}" + return NIGHTLY.read_text() + + +@pytest.fixture(scope="module") +def nightly_yaml(nightly_text: str) -> dict: + return yaml.safe_load(nightly_text) + + +def test_ref_assignments_are_allowlisted_literals(nightly_text: str) -> None: + """Every `*_ref=` assignment is a literal branch name from the allowlist. + + The empty string is permitted: it is the "track not selected" initializer, + and an unselected track's job never runs. + """ + assignments = _REF_ASSIGNMENT.findall(nightly_text) + assert assignments, "found no *_ref= assignments — has the parser moved?" + + offenders = { + f"{name}={value!r}" + for name, value in assignments + if value != "" and value not in ALLOWED_REFS + } + assert not offenders, ( + "nightly.yml assigns a ref outside the allowlist " + f"{sorted(ALLOWED_REFS)}: {sorted(offenders)}. " + "A privileged nightly run must only check out reviewed branches — see " + "the security note at the top of nightly.yml." + ) + + +def test_no_ref_is_interpolated_from_a_track_token(nightly_text: str) -> None: + """Refs are assigned as literals, never sliced out of the track token. + + A parser that did `ref="${token#test-backend-}"` would pass the allowlist + test above (no literal to see) while feeding attacker-influenced text + straight to `ref:`. + """ + interpolated = [ + line.strip() + for line in nightly_text.splitlines() + if re.search(r'^\s*\w*_ref="?\$', line) + ] + assert not interpolated, ( + "nightly.yml derives a ref from a shell expansion rather than a " + f"literal: {interpolated}. Keep refs literal — see nightly.yml's " + "security note." + ) + + +def test_unknown_branch_is_rejected_not_ignored(nightly_text: str) -> None: + """A track naming a non-allowlisted branch fails the run. + + Falling through to the `*)` warning branch instead would leave the track + silently unselected — safe today, but it removes the signal that tells an + operator why their branch did not run, and invites "just add a default". + """ + deny_arm = re.search( + r"test-backend-\*\|.*?;;", + nightly_text, + re.DOTALL, + ) + assert deny_arm, "the wildcard deny arm for unknown branches is gone" + assert "exit 1" in deny_arm.group(0), ( + "the wildcard branch arm no longer fails the run; an unreviewed " + "branch name must be rejected, not warned about" + ) + + +def test_checkout_refs_come_only_from_the_resolver(nightly_yaml: dict) -> None: + """No checkout step reads a ref straight from workflow inputs or event data. + + The resolver is the choke point; a `ref:` naming `github.event.*` or + `inputs.*` would route around it. + """ + forbidden = ("github.event", "inputs.", "github.head_ref") + offenders: list[str] = [] + + for job_name, job in (nightly_yaml.get("jobs") or {}).items(): + for step in job.get("steps") or []: + uses = str(step.get("uses", "")) + if "actions/checkout" not in uses: + continue + ref = str((step.get("with") or {}).get("ref", "")) + if any(token in ref for token in forbidden): + offenders.append(f"{job_name}: ref={ref!r}") + + assert not offenders, ( + "a nightly checkout takes its ref from untrusted input rather than " + f"the resolve-tracks allowlist: {offenders}" + ) From d5e3c5e2ae97959d4cbd2176d838309365004f05 Mon Sep 17 00:00:00 2001 From: Phil Merrell Date: Sun, 13 Sep 2026 22:43:13 -0600 Subject: [PATCH 2/2] docs(backfill): give the venv interpreter, and stop naming ItemCount as the check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .claude/skills/cutting-a-release/SKILL.md | 21 +++++++++++-- RELEASE_NOTES.md | 31 +++++++++++++++++-- .../scripts/backfill_artifact_tool_merge.py | 4 +-- .../backfill_artifact_user_index_keys.py | 4 +-- .../backfill_false_interrupted_markers.py | 4 +-- backend/scripts/backfill_session_static_sk.py | 4 +-- backend/scripts/backfill_skill_bundles.py | 4 +-- .../scripts/backfill_tool_catalog_index.py | 13 +++++--- 8 files changed, 65 insertions(+), 20 deletions(-) diff --git a/.claude/skills/cutting-a-release/SKILL.md b/.claude/skills/cutting-a-release/SKILL.md index fa41973d3..e196f96b4 100644 --- a/.claude/skills/cutting-a-release/SKILL.md +++ b/.claude/skills/cutting-a-release/SKILL.md @@ -225,12 +225,27 @@ required" — that is the note that sends someone digging through `git log`: > for tool rows written before it existed. Idempotent; dry-run by default. > > ```bash -> AWS_PROFILE= python backend/scripts/backfill_tool_catalog_index.py \ +> AWS_PROFILE= backend/.venv/bin/python backend/scripts/backfill_tool_catalog_index.py \ > --table -app-roles --region us-west-2 --apply > ``` > -> Verify `skipped=0 failed=0` and that the index item count matches the tool -> count before considering the deploy complete. +> Verify `skipped=0 failed=0`, then confirm with a live Query on the index. + +Two details that cost real time on the v1.21.0 prod run, both worth carrying +into whatever backfill note you write: + +- **Give the backend venv's interpreter, not a bare `python`.** These scripts + need `boto3`, which is not in the system Python — a bare `python` fails with + `ModuleNotFoundError: No module named 'boto3'` before it does anything. All + six `backfill_*.py` scripts are runnable as + `backend/.venv/bin/python backend/scripts/