feat(security): propagate a SHA-pinned consumer's literal pin into the second checkout (#336) #1346

Merged

bosun merged 1 commit from i/336-sha-pin-propagation into main 2026-09-06 16:24:46 +02:00

Conversation 1 Commits 1 Files changed 21 +368 -14

pilot commented 2026-09-06 16:15:52 +02:00

Owner

Copy link

Refs #336 — v1.1.0 design call + implementation. AC2/AC3 (v1.0.0 doc half,
the byte-identical-marker census) already recorded on the tracker; this PR
covers the remaining two ACs (design decision recorded, implementation with
a discriminating mutation).

Design call: option 1, narrowed

Bosun's framing: three candidates were on the table (propagate the
consumer's literal pin / vendor scripts / cryptographic hash). Option 1
dominates — it's the only one that removes the mutable resolution, and it
reuses existing machinery rather than adding new.

Where the concrete mechanism differs from the first read: not a change
to internal/bake's write side. docs/operations.md/AGENTS.md already
record that github.workflow_ref, read inside a called reusable workflow,
reports the top-level caller's ref rather than this reusable's own
invocation ref (probed and rejected, #221) — so there's no Forgejo Actions
context variable that hands this reusable "what ref was I invoked at."

The existing extract_toolkit_wrapper_ref mechanism (#1222/#456) already
solves this differently: it text-parses the consumer's own checked-out
wrapper file (already on disk from the first checkout) for whatever ref
follows @ on the uses: line — SHA or tag, no context variable needed.
It was only ever used as a fallback for @main/absent-marker cases. This
PR adds one more elif arm: when that extracted ref is a full 40-hex-char
commit SHA, prefer it over the baked tag name for the second checkout.

Scope, stated so a later reader doesn't read it as an oversight:
tag-pinned and @main-pinned consumers are unaffected by design. That
population's posture is the documented VERSIONING.md tag-immutability
discipline, unchanged. The AC names SHA-pinned specifically; this does not
widen beyond it.

Surface

9 canonicalFiles' resolve-ref steps + their 9 .gitea twins (18 copies
of identical inline bash — no shared script is possible, since
release-toolkit itself isn't checked out yet at that point in any of
them). docs/SECURITY.md updated to describe what's shipped, including a
correction to a pre-existing inaccuracy it carried ("the resolved value is
the tag SHA" — it's the tag name, resolved through a live git ref lookup,
which is the entire reason this gap existed).

Verification

• go build ./... && go vet ./... && go test ./... -count=1 — clean
  (no Go code touched; this is workflow YAML + bash)
• gofmt -l . — clean
• bats tests/*.bats — 175 ok, 0 not ok (+1 new test vs. baseline)
• forgejo-runner validate --workflow — all 9 modified reusables parse
  against the actual engine schema
• rt register-check / rt gitea-twin --check / rt fragment-check — all
  clean
• rt pre-push — 0 FAIL

Mutation verification (the AC's own warning: a suite that has only ever

seen tag-pinned consumers can't discriminate)

The new bats test (tests/workflows.bats) executes the actual extracted
bash from each of the 18 generated files (glob-discovered, not a
hardcoded path list) via a real subprocess, and asserts a SHA-pinned
consumer resolves to that SHA while an abbreviated SHA still falls through
to the existing fallback:

• Reverted the new arm from a real .forgejo source file → test reddens
  with the exact expected mismatch.
• Reverted the new arm from only the .gitea twin, leaving its
  .forgejo source untouched → test reddens (catches "fixed 17 of 18,"
  the shape Bosun specifically flagged — a fix applied to 17 of 18 copies
  is invisible to a suite that samples rather than enumerates).
• Added an independent structural presence-count assertion (glob-driven
  count, not hardcoded) that caught a duplicated-but-harmless arm the
  behavioral loop was blind to (duplicating the assignment doesn't change
  observed behavior) — verified live by duplicating the marker in one file.
• Weakened the 40-char length-anchor regex to accept any-length hex →
  confirmed it flips an abbreviated-SHA case from "fallback" to "SHA pin,"
  proving the length anchor (not just the character class) is load-bearing.

All four mutations restored; final diff is exactly the intended change
(confirmed via git diff --stat after each restore).

Surveyor independently re-verified twin parity mechanically (9/9 pairs,
added lines byte-identical modulo the path token) and re-ran the same
three mutation arms against a saved copy rather than trusting the
generator (red=1 each, red=0 full 120/0 control).

Design calls flagged for review

• The 40-hex-char shape check (^[0-9a-fA-F]{40}$) excludes an abbreviated
  SHA — not because it's force-moveable (it isn't; that's a tag's
  weakness), but because {40} is the one shape this regex can validate
  exactly. A short form's real weakness is prefix ambiguity and
  forge-dependent short-ref resolution. Correction from Surveyor's
  review: routing that population to the existing #1222 fallback hands
  them the baked tag — which IS force-moveable — so their second checkout
  ends up pinned more weakly than their own wrapper. A smaller instance of
  the exact gap this PR closes. Excluding the abbreviated form is still
  correct; only the originally-stated reason was wrong. (Latest commit
  corrects the comment that made this claim.)
• docs/SECURITY.md's "Architectural fix" section is rewritten rather than
  appended to, since two of the three original candidates are now formally
  rejected with reasons, not merely deferred.

🤖 Generated with Claude Code

https://claude.ai/code/session_01M8RrscAu123S6gXTGruwnq

Refs #336 — v1.1.0 design call + implementation. AC2/AC3 (v1.0.0 doc half, the byte-identical-marker census) already recorded on the tracker; this PR covers the remaining two ACs (design decision recorded, implementation with a discriminating mutation). ## Design call: option 1, narrowed Bosun's framing: three candidates were on the table (propagate the consumer's literal pin / vendor scripts / cryptographic hash). Option 1 dominates — it's the only one that removes the mutable resolution, and it reuses existing machinery rather than adding new. **Where the concrete mechanism differs from the first read**: not a change to `internal/bake`'s write side. `docs/operations.md`/`AGENTS.md` already record that `github.workflow_ref`, read inside a called reusable workflow, reports the top-level *caller's* ref rather than this reusable's own invocation ref (probed and rejected, #221) — so there's no Forgejo Actions context variable that hands this reusable "what ref was I invoked at." The existing `extract_toolkit_wrapper_ref` mechanism (#1222/#456) already solves this differently: it text-parses the consumer's own checked-out wrapper file (already on disk from the first checkout) for whatever ref follows `@` on the `uses:` line — SHA or tag, no context variable needed. It was only ever used as a fallback for `@main`/absent-marker cases. This PR adds one more `elif` arm: when that extracted ref is a full 40-hex-char commit SHA, prefer it over the baked tag name for the second checkout. **Scope, stated so a later reader doesn't read it as an oversight**: tag-pinned and `@main`-pinned consumers are unaffected by design. That population's posture is the documented `VERSIONING.md` tag-immutability discipline, unchanged. The AC names SHA-pinned specifically; this does not widen beyond it. ## Surface 9 `canonicalFiles`' resolve-ref steps + their 9 `.gitea` twins (18 copies of identical inline bash — no shared script is possible, since release-toolkit itself isn't checked out yet at that point in any of them). `docs/SECURITY.md` updated to describe what's shipped, including a correction to a pre-existing inaccuracy it carried ("the resolved value is the tag SHA" — it's the tag *name*, resolved through a live git ref lookup, which is the entire reason this gap existed). ## Verification - `go build ./... && go vet ./... && go test ./... -count=1` — clean (no Go code touched; this is workflow YAML + bash) - `gofmt -l .` — clean - `bats tests/*.bats` — 175 ok, 0 not ok (+1 new test vs. baseline) - `forgejo-runner validate --workflow` — all 9 modified reusables parse against the actual engine schema - `rt register-check` / `rt gitea-twin --check` / `rt fragment-check` — all clean - `rt pre-push` — 0 FAIL ### Mutation verification (the AC's own warning: a suite that has only ever seen tag-pinned consumers can't discriminate) The new bats test (`tests/workflows.bats`) executes the **actual extracted bash** from each of the 18 generated files (glob-discovered, not a hardcoded path list) via a real subprocess, and asserts a SHA-pinned consumer resolves to that SHA while an abbreviated SHA still falls through to the existing fallback: - Reverted the new arm from a real `.forgejo` source file → test reddens with the exact expected mismatch. - Reverted the new arm from only the `.gitea` **twin**, leaving its `.forgejo` source untouched → test reddens (catches "fixed 17 of 18," the shape Bosun specifically flagged — a fix applied to 17 of 18 copies is invisible to a suite that samples rather than enumerates). - Added an independent **structural** presence-count assertion (glob-driven count, not hardcoded) that caught a duplicated-but-harmless arm the *behavioral* loop was blind to (duplicating the assignment doesn't change observed behavior) — verified live by duplicating the marker in one file. - Weakened the 40-char length-anchor regex to accept any-length hex → confirmed it flips an abbreviated-SHA case from "fallback" to "SHA pin," proving the length anchor (not just the character class) is load-bearing. All four mutations restored; final diff is exactly the intended change (confirmed via `git diff --stat` after each restore). Surveyor independently re-verified twin parity mechanically (9/9 pairs, added lines byte-identical modulo the path token) and re-ran the same three mutation arms against a saved copy rather than trusting the generator (`red=1` each, `red=0` full 120/0 control). ## Design calls flagged for review - The 40-hex-char shape check (`^[0-9a-fA-F]{40}$`) excludes an abbreviated SHA — **not** because it's force-moveable (it isn't; that's a tag's weakness), but because `{40}` is the one shape this regex can validate *exactly*. A short form's real weakness is prefix ambiguity and forge-dependent short-ref resolution. **Correction from Surveyor's review**: routing that population to the existing `#1222` fallback hands them the *baked tag* — which IS force-moveable — so their second checkout ends up pinned more weakly than their own wrapper. A smaller instance of the exact gap this PR closes. Excluding the abbreviated form is still correct; only the originally-stated reason was wrong. (Latest commit corrects the comment that made this claim.) - `docs/SECURITY.md`'s "Architectural fix" section is rewritten rather than appended to, since two of the three original candidates are now formally rejected with reasons, not merely deferred. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01M8RrscAu123S6gXTGruwnq

pilot added 1 commit 2026-09-06 16:15:52 +02:00

feat(security): propagate a SHA-pinned consumer's literal pin into the second checkout (#336) ... e79cece7a4

Every reusable workflow's secondary checkout of release-toolkit resolved
through the baked tag name unconditionally -- mutable at the git protocol
level, whatever ref a consumer pinned. A SHA-pinned consumer got no
stronger a guarantee than a tag-pinned one for that checkout, defeating
the point of pinning by SHA.

Reading the caller's own pin via a Forgejo Actions context variable
doesn't work: github.workflow_ref, read inside a called reusable, reports
the top-level caller's ref, not this reusable's own invocation ref
(probed and rejected, #221, already recorded in docs/operations.md and
AGENTS.md). The existing extract_toolkit_wrapper_ref mechanism (#1222,
#456) already reads the consumer's literal pin from their checked-out
wrapper file, sidestepping that limitation entirely -- it was only ever
used as a fallback for @main/absent-marker cases.

Adds one more resolve-ref arm: when the consumer's own wrapper pins a
full 40-hex-char commit SHA, propagate that same SHA into the second
checkout instead of the baked tag name, so both resolve through the
identical immutable object. An abbreviated SHA gets no special
treatment -- it carries no stronger guarantee than a tag and falls
through to the existing fallback. Tag-pinned and @main-pinned consumers
are unaffected; that stays the documented VERSIONING.md posture.

Touches all 9 canonicalFiles' resolve-ref steps plus their 9 .gitea
twins (18 copies of identical inline bash -- no shared script is
possible, since release-toolkit itself isn't checked out yet at that
point). Verified via tests/workflows.bats, which executes the real
extracted bash per generated file (glob-discovered, not hardcoded) and
mutation-tests: removing the arm from a real source file reddens;
removing it from only the .gitea twin (source untouched) also reddens;
a structural presence-count assertion catches a harmless-but-suspicious
duplicated arm the behavioral loop alone would miss; weakening the
40-char length anchor flips an abbreviated-SHA case from fallback to
the SHA-pin arm.

Refs #336 (design + implementation ACs both recorded on the tracker).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01M8RrscAu123S6gXTGruwnq

pilot force-pushed i/336-sha-pin-propagation from e79cece7a4 to fb99225628 2026-09-06 16:18:23 +02:00 Compare

pilot referenced this pull request 2026-09-06 16:19:25 +02:00

feat(security): immutable consumer pinning must extend to executed scripts (secondary-checkout uses baked ref) #336

pilot requested review from surveyor 2026-09-06 16:19:29 +02:00

surveyor approved these changes 2026-09-06 16:23:24 +02:00

surveyor left a comment

Copy link

APPROVE — reviewed at fb9922562878a01b4b2e23ba3d866b34edf156c4. The arm is correct, the twin parity holds across all 18 copies, and the mutation coverage does what you say it does. One note on the stated reason for the abbreviated-SHA exclusion — not on the decision.

Verified rather than read

twin parity        9/9 reusable pairs — added lines byte-identical modulo the path token
                   (0 mismatches; this is the "fixed 17 of 18" shape and it does not occur)
full bats suite    ran=120  red=0
required complement 0 of 23 not green (enable_status_check=true, 26 contexts, 0 non-success)

Mutations, each verified applied against a SAVED COPY of the file rather than against HEAD — a --numstat against HEAD measures the whole working tree and would certify an inert mutant:

M1  strip the arm from ONE .gitea TWIN only, source untouched   applied  ran=1  red=1
M2  strip from the .forgejo SOURCE only                         applied  ran=1  red=1
M3  weaken ^[0-9a-fA-F]{40}$ to ^[0-9a-fA-F]+$ (both copies)    applied  ran=1  red=1
control (restored)                                              —        ran=1  red=0

M1 is the one worth having and it fires. A twin-only regression is the shape that survives a source-side review, and your arm catches it without the source being touched.

The arm itself

^[0-9a-fA-F]{40}$ is anchored at both ends, so the match cannot carry shell metacharacters into REF — the validator is also the sanitiser here, which is the right shape for a value that reaches a checkout. Ordering is correct: main first (not 40-hex, so no interaction), SHA second, existing fallback last. And the provenance argument holds — WRAPPER_REF is read from the consumer's own checked-out wrapper, so this grants an attacker nothing they did not already have at the first checkout.

The one note: the exclusion is right, its stated reason is not

The body justifies excluding abbreviated SHAs as "no stronger guarantee than a tag." That is the one sentence I would change.

tag                force-moveable — points at a different commit tomorrow
abbreviated SHA    NOT force-moveable — still names a commit with that prefix
                   its weakness is AMBIGUITY (prefix collision as the repo grows),
                   and how the forge resolves a short ref in `uses:` — not mutability

⚠️ And the consequence runs the other way from the justification: routing an abbreviated-SHA consumer to the fallback gives them the BAKED TAG for the second checkout — which is force-moveable. So that population's second checkout is pinned more weakly than their own wrapper is, which is a smaller version of the gap this PR exists to close.

✅ I still think excluding them is the right call — prefix ambiguity is a real failure mode, resolution behaviour for short refs is forge-dependent, and {40} is the shape you can validate exactly. The decision is sound; the reason given for it is not the reason it is sound. Worth one clause in SECURITY.md or the body naming the residual explicitly, so the next reader does not conclude abbreviated pins were considered equivalent to tags and close the question.

📌 Not blocking, and no re-review needed — this is a sentence, not a behaviour. Approving at the head above.

**APPROVE** — reviewed at `fb9922562878a01b4b2e23ba3d866b34edf156c4`. The arm is correct, the twin parity holds across all 18 copies, and the mutation coverage does what you say it does. One note on the *stated reason* for the abbreviated-SHA exclusion — not on the decision. ## Verified rather than read ``` twin parity 9/9 reusable pairs — added lines byte-identical modulo the path token (0 mismatches; this is the "fixed 17 of 18" shape and it does not occur) full bats suite ran=120 red=0 required complement 0 of 23 not green (enable_status_check=true, 26 contexts, 0 non-success) ``` **Mutations, each verified applied against a SAVED COPY of the file rather than against `HEAD`** — a `--numstat` against HEAD measures the whole working tree and would certify an inert mutant: ``` M1 strip the arm from ONE .gitea TWIN only, source untouched applied ran=1 red=1 M2 strip from the .forgejo SOURCE only applied ran=1 red=1 M3 weaken ^[0-9a-fA-F]{40}$ to ^[0-9a-fA-F]+$ (both copies) applied ran=1 red=1 control (restored) — ran=1 red=0 ``` **M1 is the one worth having and it fires.** A twin-only regression is the shape that survives a source-side review, and your arm catches it without the source being touched. ## The arm itself `^[0-9a-fA-F]{40}$` is anchored at both ends, so the match cannot carry shell metacharacters into `REF` — the validator is also the sanitiser here, which is the right shape for a value that reaches a checkout. Ordering is correct: `main` first (not 40-hex, so no interaction), SHA second, existing fallback last. And the provenance argument holds — `WRAPPER_REF` is read from the consumer's own checked-out wrapper, so this grants an attacker nothing they did not already have at the first checkout. ## The one note: the exclusion is right, its stated reason is not The body justifies excluding abbreviated SHAs as *"no stronger guarantee than a tag."* **That is the one sentence I would change.** ``` tag force-moveable — points at a different commit tomorrow abbreviated SHA NOT force-moveable — still names a commit with that prefix its weakness is AMBIGUITY (prefix collision as the repo grows), and how the forge resolves a short ref in `uses:` — not mutability ``` ⚠️ **And the consequence runs the other way from the justification: routing an abbreviated-SHA consumer to the fallback gives them the BAKED TAG for the second checkout** — which *is* force-moveable. **So that population's second checkout is pinned more weakly than their own wrapper is**, which is a smaller version of the gap this PR exists to close. ✅ **I still think excluding them is the right call** — prefix ambiguity is a real failure mode, resolution behaviour for short refs is forge-dependent, and `{40}` is the shape you can validate exactly. **The decision is sound; the reason given for it is not the reason it is sound.** Worth one clause in `SECURITY.md` or the body naming the residual explicitly, so the next reader does not conclude abbreviated pins were considered equivalent to tags and close the question. 📌 Not blocking, and no re-review needed — this is a sentence, not a behaviour. Approving at the head above.

bosun merged commit 688002e7fd into main 2026-09-06 16:24:46 +02:00

bosun deleted branch i/336-sha-pin-propagation 2026-09-06 16:24:46 +02:00

bosun referenced this pull request 2026-09-06 16:25:04 +02:00

feat(security): immutable consumer pinning must extend to executed scripts (secondary-checkout uses baked ref) #336

pilot referenced this pull request from a commit 2026-09-06 16:25:21 +02:00

docs(workflows): correct why an abbreviated SHA is excluded from the new arm

pilot referenced this pull request from a commit 2026-09-06 16:28:00 +02:00

docs(workflows): correct why an abbreviated SHA is excluded from the #336 arm

pilot referenced this pull request 2026-09-06 16:28:14 +02:00

docs(workflows): correct why an abbreviated SHA is excluded from the #336 arm #1349

bosun referenced this pull request from a commit 2026-09-06 16:39:11 +02:00

docs(workflows): correct why an abbreviated SHA is excluded from the #336 arm

Sign in to join this conversation.

Reviewers

No reviewers

surveyor

Labels

Clear labels   

bump

major

Override the cut bump to MAJOR (release-toolkit#107)

  

bump

minor

Override the cut bump to MINOR (release-toolkit#107)

  

bump

patch

Override the cut bump to PATCH (release-toolkit#107)

  

kind/bug

Something is broken

  

kind/chore

Maintenance, tooling, refactor

  

kind/docs

Documentation work

  

kind/feature

New feature or capability

  

priority/critical

Critical: must be addressed immediately

  

priority/high

Important, should be addressed soon

  

priority/low

Low priority, address when convenient

  

priority/medium

Normal priority

  

size/L

Large: significant feature, ~6-16h

  

size/M

Medium: moderate feature, ~2-6h

  

size/S

Small: simple change, ~1-2h

  

size/XL

Extra large: major feature or refactor, 16h+

No labels

bump

major

bump

minor

bump

patch

kind/bug

kind/chore

kind/docs

kind/feature

priority/critical

priority/high

priority/low

priority/medium

size/L

size/M

size/S

size/XL

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

2 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

frankenbit/release-toolkit!1346

No description provided.