Same-repo pull_request runs get REGISTRY_PASSWORD from head-supplied YAML — the route #853 found under the dispatch class #885

New Issue

2026-08-30T01:40:56+02:00

timothy commented

2026-08-30 01:40:56 +02:00

Split out of #853, whose probe found this while establishing that workflow_dispatch is not the narrowest route to the registry credential. Recorded in ci.workflow-dispatch-ref-unrestricted.

The route

.gitea/workflows/docker-build.yml triggers on pull_request:. Gitea resolves that from the PR head, so the run executes attacker-authored YAML. That is the general form of the problem, and it is bigger than the jobs listed below: head-supplied YAML can name any secret in the repo store, not only the ones the committed workflows happen to reference. ci.gate-trigger-base-resolved already states this — "any PR-added workflow can reference RENOVATE_TOKEN, a write:repository bot PAT in the same store".

The invariant is "every job on the PR route that names secrets.REGISTRY_PASSWORD", not a list — and NOT "every container: job", which names only five of six (see the correction comment below). Today that is six jobs in docker-build.yml holding REGISTRY_PASSWORD and running on pull_request:

Job	Line	Guard
`toolchain-preflight`	141	none
`test`	163	none — required context
`migrations`	387	none — required context
`functional-e2e`	600	`pull_request \|\| ref == main`
`api-docs`	1027	`event_name == 'pull_request'`
`format`	1143	`event_name == 'pull_request'`

(build, line 820, is the only REGISTRY_PASSWORD job excluded — if: github.event_name != 'pull_request'.) test and migrations are both branch-protection required contexts per .gitea/required-status-contexts.json.

A fix scoped to today's six names will miss the seventh. Derive the population; do not restate it — testing.guard-derives-population-from-source, and the an-issues-file-list-is-not-the-population shape.

So "push a branch, open a PR" reaches the credential with no act outside the ordinary contribution flow, where a dispatch costs one. The per-step if: ref gates on the publish steps (lines 906/934) are not a mitigation — they live in the same head-supplied file and an attacker edits them out.

This is why #853 was accepted rather than fixed: restricting dispatch would have closed the more deliberate of two routes and left the cheaper one open. That is the finding, not a deferral of it.

Actor set (measured 2026-08-30)

Exactly two write-capable accounts: timothy (owner, is_admin: true) and renovate (permission: write, is_admin: false). Against the admin no self-administered control is a boundary. Against the bot it is a real gap: review-verdict/h10 and the Renovate exemption rule (#698, #742, #845) bound what Renovate can get merged; nothing bounds what it can run.

Two separable pieces of work

1. The pull_request: credential exposure (the hard one). Gitea 1.27.1 has no fork/first-run approval gate and no per-environment secret scoping — probed in #853 across the REST API (308 paths), the loaded app.ini, and the gitea CLI. So the options are design-side, and each has a real cost:

Drop the registry login from PR-route jobs — all six, derived not listed. They need it to pull the pinned toolchain image, so this means a separate read-only pull credential; ci.actions-credential-scoping already establishes that read:repository/write:package are separable scopes, so a pull-only token looks feasible. Cost this first. Note it bounds the blast radius rather than closing the route: head YAML can still name other secrets.
Or accept and record, having actually enumerated the alternatives rather than asserting there are none.

2. tag_protections is empty (the cheap one). Live state is []. Gitea 1.27.1 does support tag protection (name_pattern + whitelist_usernames/whitelist_teams). A v* rule whitelisting the release operator closes docker-build.yml's tag-push row against the bot at no operational cost, since only the operator cuts releases. Deliberately not applied in #853's PR: its failure mode is a broken release cut, so it needs its own verification that a legitimate v* push still succeeds. Note it buys nothing against the admin actor.

Do these as two changes, not one — (2) is configuration with a release-cut blast radius, (1) is a credential redesign.

Done-when

A read-only registry pull credential for PR-route jobs is costed, and either implemented or rejected with the reason recorded
Whichever is chosen, the PR-route job population is derived (not a hand-listed six) by the predicate "names secrets.REGISTRY_PASSWORD and runs on the PR route", so a new job cannot silently join it unprotected
The v* tag protection is applied and a real release-cut tag push is verified to still succeed, or the rule is rejected with its reason
ci.workflow-dispatch-ref-unrestricted and ci.actions-credential-scoping are updated to match whatever is decided
Adversarial review passed

Split out of #853, whose probe found this while establishing that `workflow_dispatch` is *not* the narrowest route to the registry credential. Recorded in `ci.workflow-dispatch-ref-unrestricted`. ## The route `.gitea/workflows/docker-build.yml` triggers on `pull_request:`. Gitea resolves that from the **PR head**, so the run executes **attacker-authored YAML**. That is the general form of the problem, and it is bigger than the jobs listed below: head-supplied YAML can name **any** secret in the repo store, not only the ones the committed workflows happen to reference. `ci.gate-trigger-base-resolved` already states this — *"any PR-added workflow can reference `RENOVATE_TOKEN`, a `write:repository` bot PAT in the same store"*. **The invariant is "every job on the PR route that names `secrets.REGISTRY_PASSWORD`", not a list — and NOT "every `container:` job", which names only five of six (see the correction comment below).** Today that is **six** jobs in `docker-build.yml` holding `REGISTRY_PASSWORD` and running on `pull_request`: | Job | Line | Guard | |---|---|---| | `toolchain-preflight` | 141 | none | | `test` | 163 | none — **required context** | | `migrations` | 387 | none — **required context** | | `functional-e2e` | 600 | `pull_request \|\| ref == main` | | `api-docs` | 1027 | `event_name == 'pull_request'` | | `format` | 1143 | `event_name == 'pull_request'` | (`build`, line 820, is the only `REGISTRY_PASSWORD` job excluded — `if: github.event_name != 'pull_request'`.) `test` and `migrations` are both branch-protection required contexts per `.gitea/required-status-contexts.json`. **A fix scoped to today's six names will miss the seventh.** Derive the population; do not restate it — `testing.guard-derives-population-from-source`, and the `an-issues-file-list-is-not-the-population` shape. So **"push a branch, open a PR"** reaches the credential with *no act outside the ordinary contribution flow*, where a dispatch costs one. The per-step `if:` ref gates on the publish steps (lines 906/934) are not a mitigation — they live in the same head-supplied file and an attacker edits them out. This is why #853 was accepted rather than fixed: restricting dispatch would have closed the more deliberate of two routes and left the cheaper one open. That is the finding, not a deferral of it. ## Actor set (measured 2026-08-30) Exactly two write-capable accounts: `timothy` (owner, `is_admin: true`) and `renovate` (`permission: write`, `is_admin: false`). Against the admin no self-administered control is a boundary. Against the bot it is a real gap: `review-verdict/h10` and the Renovate exemption rule (#698, #742, #845) bound what Renovate can get **merged**; nothing bounds what it can **run**. ## Two separable pieces of work **1. The `pull_request:` credential exposure (the hard one).** Gitea 1.27.1 has no fork/first-run approval gate and no per-environment secret scoping — probed in #853 across the REST API (308 paths), the loaded `app.ini`, and the `gitea` CLI. So the options are design-side, and each has a real cost: - Drop the registry login from PR-route jobs — **all six, derived not listed**. They need it to pull the pinned toolchain image, so this means a separate read-only pull credential; `ci.actions-credential-scoping` already establishes that `read:repository`/`write:package` are separable scopes, so a pull-only token looks feasible. **Cost this first.** Note it bounds the blast radius rather than closing the route: head YAML can still name other secrets. - Or accept and record, having actually enumerated the alternatives rather than asserting there are none. **2. `tag_protections` is empty (the cheap one).** Live state is `[]`. Gitea 1.27.1 *does* support tag protection (`name_pattern` + `whitelist_usernames`/`whitelist_teams`). A `v*` rule whitelisting the release operator closes `docker-build.yml`'s tag-push row against the bot at no operational cost, since only the operator cuts releases. Deliberately not applied in #853's PR: its failure mode is a **broken release cut**, so it needs its own verification that a legitimate `v*` push still succeeds. Note it buys nothing against the admin actor. Do these as two changes, not one — (2) is configuration with a release-cut blast radius, (1) is a credential redesign. ## Done-when - [ ] A read-only registry pull credential for PR-route jobs is costed, and either implemented or rejected with the reason recorded - [ ] Whichever is chosen, the PR-route job population is **derived** (not a hand-listed six) by the predicate "names `secrets.REGISTRY_PASSWORD` and runs on the PR route", so a new job cannot silently join it unprotected - [ ] The `v*` tag protection is applied **and** a real release-cut tag push is verified to still succeed, or the rule is rejected with its reason - [ ] `ci.workflow-dispatch-ref-unrestricted` and `ci.actions-credential-scoping` are updated to match whatever is decided - [ ] Adversarial review passed

timothy added the ci-cd priority: medium security labels 2026-08-30 01:41:03 +02:00

timothy commented

2026-08-30 02:15:46 +02:00

Correction to this issue's body — the invariant I wrote in it is wrong, and wrong in exactly the way it warns against.

The body said the population is "every container: job on the PR route". It is not. toolchain-preflight (docker-build.yml:141) is deliberately container-free — runs-on: small, no container: key, and the credential enters at :154 via ETV_REGISTRY_AUTH rather than a registry login. docker-build.yml has exactly five container: blocks, so that predicate names five of the six PR-route jobs and would go stale on day one.

The correct predicate is every job on the PR route that names secrets.REGISTRY_PASSWORD. Re-derived against main at 94a3d1349 (after #884, which touched docker-build.yml):

toolchain-preflight  line  141  container=False  runs-on-PR=True
test                 line  163  container=True   runs-on-PR=True   <- required context
migrations           line  387  container=True   runs-on-PR=True   <- required context
functional-e2e       line  600  container=True   runs-on-PR=True
build                line  820  container=False  runs-on-PR=False  <- correctly excluded
api-docs             line 1027  container=True   runs-on-PR=True
format               line 1143  container=True   runs-on-PR=True

Six on the PR route; five of them container:. The body's table remains correct — only the stated invariant was wrong.

This matters for the Done-when box that asks the population to be derived rather than listed: derived by the wrong predicate is not better than a list, it is a list with a false claim of completeness attached. Found by cold review of #853's PR.

**Correction to this issue's body — the invariant I wrote in it is wrong, and wrong in exactly the way it warns against.** The body said the population is *"every `container:` job on the PR route"*. It is not. `toolchain-preflight` (`docker-build.yml:141`) is **deliberately container-free** — `runs-on: small`, no `container:` key, and the credential enters at `:154` via `ETV_REGISTRY_AUTH` rather than a registry login. `docker-build.yml` has exactly five `container:` blocks, so that predicate names **five of the six** PR-route jobs and would go stale on day one. The correct predicate is **every job on the PR route that names `secrets.REGISTRY_PASSWORD`**. Re-derived against `main` at `94a3d1349` (after #884, which touched `docker-build.yml`): ``` toolchain-preflight line 141 container=False runs-on-PR=True test line 163 container=True runs-on-PR=True <- required context migrations line 387 container=True runs-on-PR=True <- required context functional-e2e line 600 container=True runs-on-PR=True build line 820 container=False runs-on-PR=False <- correctly excluded api-docs line 1027 container=True runs-on-PR=True format line 1143 container=True runs-on-PR=True ``` Six on the PR route; five of them `container:`. The body's table remains correct — only the stated invariant was wrong. This matters for the Done-when box that asks the population to be **derived** rather than listed: derived *by the wrong predicate* is not better than a list, it is a list with a false claim of completeness attached. Found by cold review of #853's PR.

timothy referenced this issue from a commit

2026-08-30 02:45:48 +02:00

docs(853): probe says dispatch CAN'T be ref-restricted — and restricting it would close nothing

timothy referenced this issue from a commit

2026-08-30 02:45:49 +02:00

fix(853): the generalizing step was already in the corpus — un-narrow, and count six not three

timothy referenced this issue from a commit

2026-08-30 02:45:49 +02:00

fix(853): trim a residual tail that survived the reversal, and rewrap the workflow comment

timothy referenced this issue from a commit

2026-08-30 02:45:49 +02:00

fix(853): the invariant I wrote to replace a stale list was itself stale on day one