fix(885): PR-route registry credential exposure closed; anonymous reads replace the stored secret #919
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: timothy/ersatztv#919
Reference in New Issue
Block a user
fixes #885
refs #909
What this does
Removes the routine materialisation of a write-capable registry credential (
REGISTRY_PASSWORD/REGISTRY_USER) into every job that runs on thepull_requestroute in.gitea/workflows/docker-build.yml.Root cause. Gitea resolves a
pull_request:trigger from the PR head, sodocker-build.ymlexecutes attacker-authored YAML on every PR — not just the diff, the whole file. Six of the file's
jobs named
secrets.REGISTRY_PASSWORDand ran on that route (toolchain-preflight,test,migrations,functional-e2e,api-docs,format; two of them required contexts), so ahead-supplied edit could read or exfiltrate a
write:package+read:repositoryPAT with no actoutside the ordinary contribution flow.
build(the only job that still needs the credential to push:prod) is the sole exception, gatedif: github.event_name != 'pull_request'.Fix — two independent changes:
the five
container:image pulls andtoolchain-preflight's tag-resolve go through theregistry's anonymous Bearer-token flow (the
ersatztv-cipackage is public), and the threeci-detect-already-validated.shcommit-status GETs readtimothy/ersatztv's combined statusunauthenticated (the repo is public).
buildkeeps the PAT and stays off the PR route.name_pattern: v*,whitelist_usernames: ["timothy"]) closes theremaining route to
:prod— av*tag push, unaffected by branch protection, thatrenovate(the repo's only other write-capable account) could otherwise have made unilaterally.
The invariant — no job the
pull_requesttrigger reaches, and no workflow scope outsidejobs:,may name a stored secret — is derived from the git index (not hand-listed) and held by
scripts/tests/test_workflow_persist_credentials.py::test_no_PULL_REQUEST_route_job_names_a_STORED_secret,with two
MUTATION-grade proofs run against the shippeddocker-build.ymlitself (deletingbuild'sroute-exclusion
if:, and adding a workflow-levelenv:naming a secret) rather than a syntheticfixture.
#909 (guard-inventory / mutation-manifest rows described as
actions/checkout-only, understatingthe second invariant this branch adds): fixed on this branch, not deferred — commit
1fb2d5f92(carried through the rebase) corrects both
docs/guard-inventory.mdrows and adds aCLAIMSentryto
scripts/tests/mutation_manifest.pybinding the population row's sentence to a mutation ofdocker-build.yml'sif: github.event_name != 'pull_request'clause, executed every run.refs #909rather than
fixes #909: #909's Done-when includes "adversarial review passed" as its own box, whichthis PR's review rounds do not evidence for #909's issue specifically — closing #909 is left to a
comment pointing at this PR once its boxes are ticked, rather than an auto-close on an unticked box.
Measured numbers
61ed6a795):PYTHONPATH=. python3 -m pytest scripts/tests -q→ 1599 passed, 3 skipped, 0 failed in 828.86s (13m48s) — same pass count the review rounds
measured throughout.
ruff check/ruff format --checkon every touched Python file: clean.shellcheck scripts/ci-toolchain-image-resolves.sh scripts/ci-detect-already-validated.sh: nofindings.
python3 scripts/decisions_validate.py: OK (advisory-only length warnings unrelated to thisbranch's records).
PYTHONPATH=. python3 scripts/build_decisions_catalog.py: no diff —docs/decisions/README.mdwas already current after the rebase.
python3 scripts/check-doc-narrative.py --diff origin/main: 11 files scanned, 0 advisory warnings.endpoint → 200, pinned manifest's first layer → 200 (32,991,280 bytes); the same GET with no
bearer → 401.
.csorweb/files are touched by this branch, so the .NET build/test and SPAlint/typecheck/build/test gates do not apply;
npm run check:api/lint/typecheck/buildstillran as part of the repo's
.husky/pre-pushhook on this push and passed clean.Review history
Nine review rounds ran on this branch across its lifetime. Early rounds could not get a cross-family
pass — the Codex CLI was measured unresponsive (zero CPU, zero output) when invoked — so every
rubric round in that stretch was covered by a cold same-family fallback reviewer instead, with the
substitution and its reason recorded at the time. The final round got a real cross-family Codex pass
alongside the same-family lenses.
<<REVIEW HISTORY
round 1-8: cold same-family fallback reviewer standing in for cross-family review (Codex CLI measured unresponsive on attempts — zero CPU, zero output); each round's should-fix/nit findings fixed before the next round ran
round 9: same-family lenses plus a real cross-family Codex pass; found 1 should-fix (
_SECRET_REF/secret_name_countsscoped to${{ }}spans, blind to a bare unwrappedif:naming a stored secret) and 3 nit (guard-inventory/mutation-manifest rows described as deferred to #909 when this branch actually fixes them; the anonymous-layer-download residual inci.pr-route-carries-no-stored-credentialneeding a fresh dated re-measurement; Codex's send-back — the unmeasured "1-2s" pull-failure timing still stated as fact inscripts/ci-toolchain-image-resolves.shanddocs/ci-cd.md); all four fixed in commitsfa06da21f..61ed6a795(theif:-scoping structural fix, both prose corrections, the re-dated re-measurement, and dropping the unmeasured timing in favour of the structural fact it stood for)REVIEW HISTORY>>
Cross-family review status: codex — ran a real pass in round 9 and returned a send-back verdict
on exactly one nit (the unmeasured "1-2s" timing above), which is fixed on this head. No further
cross-family pass has run against the post-fix commits (
fa06da21f..61ed6a795); this finishersession verified all four round-9 findings against the shipped code directly (grep for the removed
timing claim, execution of
secret_name_counts/condition_refsagainst the fixture strings thefinding named, and a fresh re-measurement of the anonymous layer download) rather than re-invoking
Codex, and ran the full local gate green as recorded above.
Deliberately left open
refs #909above; its own Done-when (including its "adversarial review passed" box)is not ticked by this PR's review rounds and is left for a follow-up comment once verified
independently.
release.tag-protection-v-star) — the rule is appliedand its negative half verified (a non-
v*tag still pushes), but a realv*release-cut tag pushsucceeding, and a push by
renovatebeing refused, are both explicitly deferred to the releaseoperator's next real cut. No
v*tag was pushed from this session.fa06da21f..61ed6a795) is owed nextwindow, per the note above — the fixes were verified directly by this session instead.
🤖 Generated with Claude Code
https://claude.ai/code/session_015QqCpYFsKgnAnx6jVwrKiV
`docker-build.yml` triggers on `pull_request:`, which Gitea resolves from the PR HEAD, so that run executes contributor-authored YAML and every `secrets.*` it names is materialised into it. Six jobs held `REGISTRY_PASSWORD` that way — `toolchain-preflight`, `test`, `migrations`, `functional-e2e`, `api-docs`, `format` — two of them branch-protection required contexts. The read-only pull PAT the issue asked to cost first was REJECTED, and the measurement is the reason: this registry already issues an anonymous pull token for `timothy/ersatztv-ci` (`GET /v2/token?scope=repository:timothy/ersatztv-ci:pull` -> 200), that token reads the pinned manifest and its config blob (200/200), and the combined-status GET answers 200 unauthenticated. A read-only PAT would grant exactly what anonymity grants while adding one more credential to the store head-supplied YAML reaches. So the stronger form was implemented instead: no PR-route job names a stored secret at all. - `.gitea/workflows/docker-build.yml`: the five `container: credentials:` blocks, the `ETV_REGISTRY_AUTH` step env and the three `ETV_STATUS_AUTH` step envs are gone. `build` keeps the PAT; it is gated `if: github.event_name != 'pull_request'`. - `scripts/ci-toolchain-image-resolves.sh`: reads `realm` out of the `Www-Authenticate` challenge, exchanges it once per run for an anonymous pull token, retries with the bearer. Every refusal direction is preserved — a 401/403 after the token leg, a token endpoint yielding no token, and one that cannot be reached all `fail` rather than degrading to could-not-tell — and the message now names the cause an operator can act on (the repo or package has stopped being public). - `scripts/ci-detect-already-validated.sh`: the status GET is anonymous. No credential override is kept: the URL names one instance, that instance is public, and an unusable `":"` would draw a 401 and turn a working read into a permanent skip=false. - `scripts/tests/test_workflow_persist_credentials.py`: the invariant, derived from the git index by "every job of a `pull_request`-triggered workflow that names a `secrets.*`" — never the six-name list, and never "every `container:` job", which names five of six because `toolchain-preflight` is container-free. Witnessed red against the unfixed workflow naming all six jobs; green after. Live tag protection applied and read back: `POST /repos/timothy/ersatztv/tag_protections` `{"name_pattern": "v*", "whitelist_usernames": ["timothy"]}` -> id 1. A non-`v*` probe tag pushed and deleted proves tag pushes still work at all. The POSITIVE release-cut verification is DEFERRED to the operator's next real cut: pushing a `v*` tag publishes the `:prod` image, which is a release, not a verification step. What this does not close, stated so the records are not cited as a boundary: `REGISTRY_PASSWORD` stays in the Actions store for `build`, and head YAML can still name it, `RENOVATE_TOKEN` or `SERVERMGMT_DEPLOY_KEY`. Blast radius, not the route. New records `ci.pr-route-carries-no-stored-credential` and `release.tag-protection-v-star`; `ci.workflow-dispatch-ref-unrestricted`, `ci.actions-credential-scoping` and `release.main-direct-push-disabled` updated to match; catalog regenerated. Closes #885. Decisions-Edit: yes Proves: scripts/tests/test_workflow_persist_credentials.py::test_no_PULL_REQUEST_route_job_names_a_STORED_secret Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015QqCpYFsKgnAnx6jVwrKiVThe shipped guard walked `jobs.<id>` only and leaned on a text-versus-walk cross-check to catch anything the walk could not reach. That cross-check compared per-file NAME SETS, and the two halves cancelled on the one file the invariant is about: measured 2026-09-05 at 59003d5a3, hoisting env: ETV_REGISTRY_AUTH: ${{ secrets.REGISTRY_USER }}:${{ secrets.REGISTRY_PASSWORD }} into `.gitea/workflows/docker-build.yml`'s root `env:` — which materialises into EVERY job on the head-authored PR route — left `pytest scripts/tests/test_workflow_persist_credentials.py -q` at `14 passed`, rc=0. The same hoist in `pr-checks.yml` reddened, because no job there already names those secrets. The guard could only ever see a name NO job used; a second copy of a reference `build` legitimately keeps naming changed no set. That is `dont-keep-a-copy-of-a-set` / `proof-sharing-with-subject-proves-nothing`: the proof shared its accumulator with its subject and cancelled. Two changes, because the cross-check was being asked to do the assertion's job: * the workflow scope (everything outside `jobs:`) is now judged in its own right by the same structure-blind collector — it is a second entry site on equal footing with the job subtree, not an edge case, since no job-level `if:` can take a root `env:`/`defaults:` off the route; * the cross-check walks the whole document and compares occurrence COUNTS. A duplicate at an unreachable location now reddens: probed 2026-09-05, a trailing `# ${{ secrets.REGISTRY_PASSWORD }}` on a root `env:` line reports `walk [('REGISTRY_PASSWORD', 1)] vs text [('REGISTRY_PASSWORD', 2)]` where the set version agreed. Under counting the comment strip becomes load-bearing rather than the no-op the old docstring admitted it was. Driven by a mutation on the SHIPPED `docker-build.yml`, the way the `build`-loses- its-`if:` mutation already is, plus a direct assertion on the two collectors that a duplicated reference changes the count and not the names. Witnessed red with the hoist in the tree (3 failed) and green without it (17 passed). The decision record's own claims were false in the same way and are corrected: `rule:` said "NO job ... may name a stored secret" (a root `env:` is not a job) and the prose said "a text-versus-walk cross-check reports any reference the walk cannot reach". Refs #885 Decisions-Edit: yes Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015QqCpYFsKgnAnx6jVwrKiV`test_the_cross_check_COUNTS_locations_rather_than_collecting_NAMES` asserted the property on the two COLLECTORS, not on the check that uses them. The comparison itself was inlined in a loop over the real workflows, which agree under either mechanism — so changing `walked != scanned` back to a name-set comparison, restoring the exact blind spot this branch exists to close, left all 17 tests passing. A guard whose distinguishing mechanism has no mutation proof is the shape `testing.guard-ships-with-mutation-proof` names. The per-file half is now `walk_versus_text_faults(name, text)`, driven on a text/walk pair whose NAME SETS AGREE: a second `${{ secrets.REGISTRY_PASSWORD }}` in a trailing comment, which the line-level strip leaves in the text half and the YAML walk cannot reach. Counting reports it; the set comparison the branch replaced reports nothing, and the test asserts BOTH halves of that so the contrast is the assertion rather than a comment. Measured: with `walked == scanned` mutated to `set(walked) == set(scanned)`, 1 failed / 16 passed; restored, 17 passed. Refs #885 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015QqCpYFsKgnAnx6jVwrKiVThree docstrings dated their measurement to `59003d5a3`, this branch's head before it was rebased onto `main` after ersatztv#907 landed. That commit is unreachable from the branch and will never be in `main`, so `git show` on it fails for every later reader — a citation that cannot be followed is worse than none, because it reads as checkable. Each now names what it measured against ("the predecessor of this commit", and for the guard, "as it walked `jobs.<id>` only and compared per-file NAME SETS"), which is what the reader actually needs and what survives any rebase. Refs #885 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015QqCpYFsKgnAnx6jVwrKiV`secrets['REGISTRY_PASSWORD']` is the same reference to the expression evaluator as `secrets.REGISTRY_PASSWORD`, and the guard shipped here could see only the dot form. Measured 2026-09-05 against the predecessor of this commit, a `pull_request`-route job whose `env:` read `"${{ secrets['REGISTRY_PASSWORD'] }}"` produced `stored_secret_faults(...) == []` AND `walk_versus_text_faults(...) == []` — the text cross-check cannot report the gap, because both of its halves resolve references through the one pattern, so a spelling it does not know is a shared blind spot they agree at zero on rather than a disagreement they name (`proof-sharing-with-subject-proves-nothing`). The file enumerated four other blind spots it has — composite actions, reusable workflows, nested directories, both directions of the comment strip — and not this one, which is what made the omission read as coverage. `secret_refs` is now the single entry point for both halves, and it matches the dot form, both index forms and a case-varied context, then counts the RESIDUE: any `secrets` token inside a `${{ }}` span that yielded no literal name is reported under the sentinel `secrets.*`. Counting the residue rather than pattern-matching `toJSON(secrets)` and a computed index is what makes it exhaustive over the grammar — a spelling nobody has written yet still faults, in the fail-closed direction. The bare word is read as the context only inside an expression, because in prose it is ordinary English; this file and four workflows discuss "secrets" in comments. `test_the_collector_sees_every_SPELLING_of_a_secret_reference` drives the six spellings through the collector and the cross-check and asserts each is invisible to the real predecessor, so reverting the widening reddens it. Whether act_runner resolves each spelling against this instance was not probed from here (that needs a live run); the direction makes that acceptable — a spelling the runner does not support costs a spurious demand on a job nobody has written, the omission cost a live write-capable credential on the head-authored route. Decisions-Edit: yes Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015QqCpYFsKgnAnx6jVwrKiVsecretsonly inside an expression span d62335bc8d`_SECRET_REF` ran over the whole string while the comment above it claimed every pattern was confined to `${{ }}` spans. Executed against the shipped module, `secret_refs("# We deliberately pass no secrets. Then the pull is anonymous.")` returned `['Then']`, and fed through the real collector that is one fault reading "job `j` names stored secret(s) on the pull_request route: Then" — a fabricated name, on a PR-route job, for its own comment. The same comment separately reddened the text-versus-walk cross-check, because the line-level strip removes a `#` line from the text half only. The existing negative control passed for a reason that does not generalise: no `.` follows the word in `"no secrets are used here"`. A sentence ENDING in "secrets." is the likeliest thing to be written into a PR-route `run:` block on this branch's own subject, so the trap was self-inflicted. `secret_refs` now resolves names per `${{ }}` span, so every spelling is scoped the way the residue counter already was. The added rows drive the real predecessor — `_SECRET_REF` applied to the whole string — and assert it read a name where the scoped reader reads none, so reverting the scoping reddens them. The `INJECTED_SECRETS` comment stops calling the injected `GITEA_TOKEN` "bounded by the workflow's own `permissions:`": on this route the head supplies that file and can delete the block. Allow-listing it is a claim about the store it is not in, not about a bound. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015QqCpYFsKgnAnx6jVwrKiV[after it is not read as an array subscript a84fa7665bThe detector was exhaustive over the `${{ }}` expression grammar and blind to `jobs.<id>.secrets: inherit` on a `uses:` job, which passes the caller's whole store to the called workflow while naming nothing. `secret_refs` reads only inside expression spans — correctly, since outside one `secrets.` is a full stop — and `inherit` is a plain scalar, so such a job was put in the derived population by `pull_request_jobs`, walked, and reported CLEAN. Measured against the predecessor: stored_secret_faults('synthetic.yml', {True: {'pull_request': None}, 'jobs': {'reused': {'uses': './.gitea/workflows/reusable.yml', 'secrets': 'inherit'}}}) -> [] ... the same job with secrets: {TOK: '${{ secrets.RENOVATE_TOKEN }}'} -> 1 fault so the miss was specific to the VALUE SHAPE, not the key. That is the failure the done-condition names — a new job joining the population unprotected without reddening anything — in a guard whose stated selling point is exhaustiveness over the grammar and no exemption list. `opaque_secret_handovers` now faults a `secrets:` key whose value is not a mapping of names, under the existing `secrets.*` whole-context sentinel, and both fault sites read through one `held_secret_names` so the workflow scope and the job subtree cannot drift on which references are forgiven. The test is on the value shape and not on the word `inherit`, for the reason the residue counter is not a match on `toJSON`: any non-mapping value hands over a set the guard cannot enumerate, a spelling act_runner grows later included. Both halves of the predecessor measurement are re-derived every run rather than left as prose: the new test asserts `secret_names(job) - INJECTED_SECRETS` — the collector verbatim as it read before this clause — empty on the same fixtures it asserts the fault on, and asserts the job is in the population. Reverting `held_secret_names` to that expression reddens that test and only that test (measured: 1 failed, 18 passed). The clause reads the DOCUMENT only and the text-versus-walk cross-check cannot cover it — there is no expression for its half to match, which is a stronger reason than the shared-blind-spot one the cross-check already discloses. Said at the definition, in the cross-check's "STRUCTURALLY CANNOT REPORT" paragraph, and in the record, rather than left to be discovered; it does not redden the cross-check either, since the clause feeds the fault collector and not `secret_name_counts`. Whether Gitea 1.27.1 / act_runner resolves `workflow_call` + `secrets: inherit` on this instance was NOT probed — that affects reachability today, not the guard's silence, and the direction is the one the spelling rows already take. No tracked workflow uses a job-level `uses:`, so nothing reddens. Refs #885 Decisions-Edit: yes Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015QqCpYFsKgnAnx6jVwrKiVif:is an expression unwrapped, so the scoped detector reads that key whole f075d1417cThe `${{ }}` span scoping added in a6225e4ee was correct about prose and wrong about one real spelling: `if:` is the only key the expression grammar lets omit the delimiters in, so `if: secrets.REGISTRY_PASSWORD != ''` named a stored secret in a document holding no `${{` at all, and the collector reported it clean. Measured on the previous head e35e1b772: `secret_refs("secrets.REGISTRY_PASSWORD != ''")` -> `[]`, and the same string as a job-level or step-level `if:` on a synthetic `pull_request` job -> `stored_secret_faults(...) == []`. That an unwrapped condition is evaluated is not inferred — `docker-build.yml`'s own `build` job carries `if: github.event_name != 'pull_request'` bare, and `PR_EXCLUDING_IFS` pins that exact string. `condition_refs` reads an `if:` value as one span with the delimiters neutralised to a SPACE (deleting them collapses `${{ secrets.A }}${{ secrets.B }}` into the single identifier `secrets.Asecrets`, losing a reference), and `secret_name_counts` routes the value there instead of onto the stack, so a wrapped condition still counts once. Everywhere else the scoping stands and the English `# We pass no secrets. Then …` still costs nothing. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015QqCpYFsKgnAnx6jVwrKiVTwo corrections to `ci.pr-route-carries-no-stored-credential`, both about claims that read as checked and were not. The `rule:` said the detector reads every spelling "only inside a `${{ }}` span", and the body enumerated `secrets: inherit` as the ONE shape left uncovered. An unwrapped `if:` is a second, and it is a shape this repo writes: both now name it, and say the value of an `if:` is read whole. `mechanics:` listed an anonymous LAYER download among two things the daemon probe did not exercise. Measured 2026-09-05 from a workstation holding no registry credential: the anonymous pull token reads the pinned manifest's first layer `sha256:179c68a720750ab4d354f6b55c0a9f551d4fd7bde93606dd0be79ba16493a39e` -> HTTP 200, 32991280 bytes, and the same GET with no token -> 401. act_runner's own pull call path is the one leg still unexercised. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015QqCpYFsKgnAnx6jVwrKiVReview-verdict: MERGEABLE @
61ed6a7Orchestrated-session workflow: ten cold review rounds across the build and three resumes (correctness Opus high in its own worktree, conformance sonnet high), every blocking and should-fix finding answered by fix commits the PR body lists; final round nits only. Cross-family: a real Codex pass ran in round nine (send-back on one nit, since fixed); the round-ten fix commits fa06da21f..61ed6a795 were verified by the same-family lenses and the fixer against shipped code, not by a second Codex pass — owed next window and stated in the PR body. Full Python suite 1599 passed / 3 skipped; catalog byte-stable. The v* tag-protection positive verification is DEFERRED to the operator's next real release cut (no v* tag was pushed). Verdict on the pushed head; patch unchanged since review.