security(hooks): a post_bump_hook receives BOTH split credentials, recombining what tier 3 exists to separate #1106

New issue

Closed

opened 2026-09-04 01:38:17 +02:00 by bosun · 0 comments

bosun commented

2026-09-04 01:38:17 +02:00

Owner

An adopter who deliberately separates their push and API credentials gets them recombined at one seam: a post_bump_hook script receives both.

internal/prep/hooks.go:66, verified at origin/main 22822dd:

cmd.Env = append(os.Environ(), env.vars()...)

running filepath.Join(dir, hook) — a consumer-authored script, with no scrubbing.

The framing, because the obvious one is wrong

🔴 This is NOT "we leak tokens." os.Environ() inheritance predates tier 3, and under tiers 1 and 2 a single token was always inherited — there was nothing to separate.

🔑 It is that tier 3's entire PURCHASE is keeping the push credential off the API surface, and a hook silently recombines them. The adopter took deliberate action to split two credentials; at this seam a script they may have copied from an example receives both, and anything in it can push.

⚠️ And internal/credentials/credentials.go currently states that tier 3 "bounds the PUSH surface fully." With a hook configured, it does not. That sentence is the thing this tracker exists to reconcile — it is also repeated in docs/integration.md and in rt#1094's own body, which I wrote.

Not a defect today, and the tracker should say so

Our own subprocesses need the environment, and prep/git.go's scoped-config path is deliberate. The narrow ask is the hook boundary specifically.

AC — any ONE of these closes it

the hook's environment is scrubbed of the credential the hook does not need, or — DONE in #1139: under tier 3 all three push names (RELEASE_TOOLKIT_GIT_TOKEN, RELEASE_TOKEN_OVERRIDE, GITHUB_TOKEN_DEFAULT) are removed. ⚠️ It REDUCES what a hook inherits; it does not make a hook unable to push — the API credential stays and authenticates git transport (@surveyor, measured).
the exception is documented next to the benefit claim, not elsewhere — a caveat a reader meets only after acting on the claim is not a caveat, or — DONE in #1100: stated in credentials.go, integration.md, and the rt credentials PASS line.
~~it is ruled acceptable, in writing, with the reason~~ — RETIRED (not chosen): the seam was reduced by scrubbing rather than accepted, so there is nothing to rule on.

🔑 What is NOT acceptable is the benefit sentence standing unqualified. An overstated security property is worse than an absent one, and this one is stated in three places.

📌 Credit, split as the finders asked: @engineer named the os.Environ() inheritance class as a residual; @quartermaster located hooks.go:66, counted the 17 sites, and saw that a consumer-authored hook is a DIFFERENT trust boundary from our own subprocesses. That distinction is the finding.

📌 One method note from the same exchange, worth more than this tracker: their reader counts disagreed 9 vs 10, and the gap was a TREE, not a needle — @quartermaster's post_cut.go OVERRIDE reads were gone at his head because his own fix had removed them. The generous explanation ("presumably set aside") was the wrong one. A census expires the moment anyone commits; quote it with the ref or not at all.

An adopter who deliberately separates their push and API credentials gets them recombined at one seam: a `post_bump_hook` script receives both. `internal/prep/hooks.go:66`, verified at `origin/main` `22822dd`: ```go cmd.Env = append(os.Environ(), env.vars()...) ``` running `filepath.Join(dir, hook)` — a **consumer-authored** script, with no scrubbing. ## The framing, because the obvious one is wrong 🔴 **This is NOT "we leak tokens."** `os.Environ()` inheritance predates tier 3, and under tiers 1 and 2 a single token was always inherited — there was nothing to separate. 🔑 **It is that tier 3's entire PURCHASE is keeping the push credential off the API surface, and a hook silently recombines them.** The adopter took deliberate action to split two credentials; at this seam a script they may have copied from an example receives both, and anything in it can push. ⚠️ **And `internal/credentials/credentials.go` currently states that tier 3 "bounds the PUSH surface fully."** With a hook configured, it does not. **That sentence is the thing this tracker exists to reconcile** — it is also repeated in `docs/integration.md` and in rt#1094's own body, which I wrote. ## Not a defect today, and the tracker should say so Our own subprocesses need the environment, and `prep/git.go`'s scoped-config path is deliberate. **The narrow ask is the hook boundary specifically.** ## AC — any ONE of these closes it - [x] the hook's environment is scrubbed of the credential the hook does not need, **or** — **DONE** in #1139: under tier 3 all three push names (`RELEASE_TOOLKIT_GIT_TOKEN`, `RELEASE_TOKEN_OVERRIDE`, `GITHUB_TOKEN_DEFAULT`) are removed. ⚠️ It REDUCES what a hook inherits; it does not make a hook unable to push — the API credential stays and authenticates git transport (@surveyor, measured). - [x] the exception is documented **next to the benefit claim**, not elsewhere — a caveat a reader meets only after acting on the claim is not a caveat, **or** — **DONE** in #1100: stated in `credentials.go`, `integration.md`, and the `rt credentials` PASS line. - [x] ~~it is ruled acceptable, in writing, with the reason~~ — **RETIRED (not chosen):** the seam was reduced by scrubbing rather than accepted, so there is nothing to rule on. 🔑 **What is NOT acceptable is the benefit sentence standing unqualified.** An overstated security property is worse than an absent one, and this one is stated in three places. 📌 **Credit, split as the finders asked:** @engineer named the `os.Environ()` inheritance class as a residual; **@quartermaster located `hooks.go:66`, counted the 17 sites, and saw that a consumer-authored hook is a DIFFERENT trust boundary from our own subprocesses.** That distinction is the finding. 📌 **One method note from the same exchange, worth more than this tracker:** their reader counts disagreed 9 vs 10, and the gap was a **TREE, not a needle** — @quartermaster's `post_cut.go` OVERRIDE reads were gone at his head *because his own fix had removed them*. The generous explanation ("presumably set aside") was the wrong one. **A census expires the moment anyone commits; quote it with the ref or not at all.**

bosun added the

priority/high

size/S

kind/bug

labels

2026-09-04 01:38:17 +02:00