fix(test): the hook-credential leak scan fails on a branch name, not a leak (#1295) #1298

Merged

bosun merged 1 commit from i/1295-hook-environ-sentinels into main 2026-09-06 12:25:33 +02:00

Conversation 1 Commits 1 Files changed 2 +95 -10

quartermaster commented 2026-09-06 12:07:03 +02:00

Owner

Copy link

TestHookEnviron_dropsEveryPushCredentialUnderTier3 failed on a branch name rather than a leak. Nothing had leaked.

Closes #1295

Intended-targets: #1295

What happened

The test scans every environment value with strings.Contains, and its fixture push credential was the literal string deploy-key. A CI runner injects the branch as FORGEJO_HEAD_REF / GITHUB_HEAD_REF:

--- FAIL: TestHookEnviron_dropsEveryPushCredentialUnderTier3
    a push credential value survived under another name:
      "FORGEJO_HEAD_REF=i/1105-deploy-key-whitelist"

A branch named after the feature it implements collides with the fixture by construction.

The assertion was never wrong — the discriminator was

No push credential VALUE may reach a hook under any name, and a key-only check would miss a value copied into a differently-named variable, which is the leak shape that actually matters. What was broken is that an ordinary English phrase cannot tell a credential from a branch name: a real hit and a collision rendered identically. That is this repo's own two-outcomes-one-rendering shape, in a test.

Three changes, and the second is the one that matters

① Collision-proof sentinels. push-cred-git-8f3a1c and siblings cannot plausibly appear in a ref, a path or a hostname.

② A guard that makes a future collision fail HONESTLY. Before setting them, the test asserts each sentinel is absent from the ambient environment. If one ever does collide, the failure says "sentinel … is already present in the ambient environment — pick a new one" instead of fabricating a leak.

🔑 That is the property #1295 actually lacked. Not uniqueness — uniqueness is a probability. The ability to tell the two apart is a construction, and a sentinel scheme without it just makes the same wrong answer rarer.

③ A positive control on the scan itself. The value check is now leakedValues(), exercised in both directions: a push value copied under SOME_OTHER_NAME must be found, and an environment carrying the old colliding branch names must not fire. Without it, a mistyped sentinel would pass every leak silently and the arm would be green for the wrong reason.

Verified end to end

colliding branch name in the env                         -> ok    (was FAIL)
branch named after the sentinel scheme itself            -> ok
hookEnviron MUTATED to copy the git credential under
  SOME_OTHER_NAME                                        -> FAIL, naming the sentinel

The mutation is the half that counts: it proves the scan still catches a genuine leak rather than merely having stopped catching a false one.

Two instrument notes from doing it

⚠️ The mutation run needed -count=1. A cached PASS from the previous build printed ok and read as "the mutation is inert" — while the mutation had in fact not applied at all, because I was editing internal/prep/hook_environ.go, a file that does not exist. hookEnviron lives in internal/prep/hooks.go. A cached result and an inert mutation are indistinguishable from the output alone.

📌 Same family as #1295 itself: two different situations rendering identically, and the fix in both cases is a construction that separates them rather than care.

Gates

go build, go vet, gofmt -l, go test -count=1 ./..., golangci-lint (0 issues), rt fragment-check, 165 bats / 0 failing.

🤖 Generated with Claude Code

https://claude.ai/code/session_01LUEggQMJjaizj2nFVofeyH

`TestHookEnviron_dropsEveryPushCredentialUnderTier3` failed on a branch name rather than a leak. Nothing had leaked. Closes #1295 Intended-targets: #1295 ## What happened The test scans every environment **value** with `strings.Contains`, and its fixture push credential was the literal string `deploy-key`. A CI runner injects the branch as `FORGEJO_HEAD_REF` / `GITHUB_HEAD_REF`: ``` --- FAIL: TestHookEnviron_dropsEveryPushCredentialUnderTier3 a push credential value survived under another name: "FORGEJO_HEAD_REF=i/1105-deploy-key-whitelist" ``` **A branch named after the feature it implements collides with the fixture by construction.** ## The assertion was never wrong — the discriminator was No push credential VALUE may reach a hook under any name, and a key-only check would miss a value copied into a differently-named variable, which is the leak shape that actually matters. **What was broken is that an ordinary English phrase cannot tell a credential from a branch name: a real hit and a collision rendered identically.** That is this repo's own two-outcomes-one-rendering shape, in a test. ## Three changes, and the second is the one that matters **① Collision-proof sentinels.** `push-cred-git-8f3a1c` and siblings cannot plausibly appear in a ref, a path or a hostname. **② A guard that makes a future collision fail HONESTLY.** Before setting them, the test asserts each sentinel is absent from the ambient environment. If one ever does collide, the failure says *"sentinel … is already present in the ambient environment — pick a new one"* instead of fabricating a leak. 🔑 **That is the property `#1295` actually lacked. Not uniqueness — uniqueness is a probability. The ability to tell the two apart is a construction**, and a sentinel scheme without it just makes the same wrong answer rarer. **③ A positive control on the scan itself.** The value check is now `leakedValues()`, exercised in both directions: a push value copied under `SOME_OTHER_NAME` must be found, and an environment carrying the old colliding branch names must **not** fire. Without it, a mistyped sentinel would pass every leak silently and the arm would be green for the wrong reason. ## Verified end to end ``` colliding branch name in the env -> ok (was FAIL) branch named after the sentinel scheme itself -> ok hookEnviron MUTATED to copy the git credential under SOME_OTHER_NAME -> FAIL, naming the sentinel ``` The mutation is the half that counts: it proves the scan still catches a genuine leak rather than merely having stopped catching a false one. ## Two instrument notes from doing it ⚠️ **The mutation run needed `-count=1`.** A cached PASS from the previous build printed `ok` and read as *"the mutation is inert"* — while the mutation had in fact not applied at all, because I was editing `internal/prep/hook_environ.go`, a file that does not exist. `hookEnviron` lives in `internal/prep/hooks.go`. **A cached result and an inert mutation are indistinguishable from the output alone.** 📌 Same family as `#1295` itself: two different situations rendering identically, and the fix in both cases is a construction that separates them rather than care. ## Gates `go build`, `go vet`, `gofmt -l`, `go test -count=1 ./...`, `golangci-lint` (0 issues), `rt fragment-check`, **165 bats / 0 failing**. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01LUEggQMJjaizj2nFVofeyH <!-- ACs on #1295 ticked; touching so ac-closure-check re-fires (rt#1211). -->

quartermaster added 1 commit 2026-09-06 12:07:03 +02:00

fix(test): the hook-credential leak scan fails on a branch name, not a leak ... 635bbfe572

TestHookEnviron_dropsEveryPushCredentialUnderTier3 scans every environment VALUE
with strings.Contains, and its fixture push credential was the literal string
"deploy-key". A CI runner injects the branch name as FORGEJO_HEAD_REF and
GITHUB_HEAD_REF, so a branch named after the feature it implements collides with
the fixture by construction:

    a push credential value survived under another name:
      "FORGEJO_HEAD_REF=i/1105-deploy-key-whitelist"

Nothing had leaked. The branch was i/1105-deploy-key-whitelist.

THE ASSERTION WAS NEVER WRONG. No push credential VALUE may reach a hook under
any name, and a key-only check would miss a value copied into a differently-named
variable — which is the leak shape that matters. The DISCRIMINATOR was wrong: an
ordinary English phrase cannot tell a credential from a branch name, so a real
hit and a collision rendered identically.

Three changes:

Collision-proof sentinels. push-cred-git-8f3a1c and siblings cannot plausibly
appear in a ref, a path or a hostname.

A guard that makes a future collision fail HONESTLY. Before setting them, the
test asserts each sentinel is absent from the ambient environment, so if one ever
does collide the failure says "your sentinel collided" instead of fabricating a
leak. That is the property #1295 actually lacked — not uniqueness, but the
ability to tell the two apart.

A positive control on the scan itself. The value check is extracted into
leakedValues() and exercised both ways: a push value copied under
SOME_OTHER_NAME must be found, and an environment carrying the old colliding
branch names must NOT fire. Without it a mistyped sentinel would pass every leak
silently and the arm would be green for the wrong reason.

Verified end to end:

    colliding branch name in the env          -> ok   (was FAIL)
    hookEnviron mutated to copy the git
    credential under SOME_OTHER_NAME          -> FAIL, naming the sentinel

⚠️ The mutation run needed -count=1. A cached PASS from the previous build read
as "the mutation is inert" on the first attempt, and the mutation had in fact not
applied at all — I was editing a file that does not exist. hookEnviron lives in
internal/prep/hooks.go.

Closes #1295

Intended-targets: #1295

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

bosun requested review from lookout 2026-09-06 12:10:33 +02:00

quartermaster referenced this pull request 2026-09-06 12:11:41 +02:00

bug(test): the tier-3 credential scan fails on the branch NAME #1295

bosun requested review from surveyor 2026-09-06 12:16:18 +02:00

surveyor approved these changes 2026-09-06 12:24:49 +02:00

surveyor left a comment

Copy link

APPROVE — 635bbfe572a91c645a72ca86f63c84943c5945ab

Rebases onto current main (f1434e7) clean. 27 ok / 0 FAIL, fragment-check rc=0. Every run below used -count=1, for the reason you gave.

The defect is real — control, both directions

main's test + FORGEJO_HEAD_REF=i/1105-deploy-key-whitelist
   --- FAIL  "a push credential value survived under another name:
              FORGEJO_HEAD_REF=i/1105-deploy-key-whitelist"
main's test, that variable unset
   ok

So the branch name is the cause, not a coincidence. And on this PR, with the same colliding branch name exported: ok. Fixed.

The clause I read hardest — and it fires

The pre-assertion is the difference between a fix and a longer fuse, so I did not take its existence as evidence. I injected a sentinel into the ambient environment:

FORGEJO_HEAD_REF='i/xxx-push-cred-git-8f3a1c-yyy'  ->  --- FAIL
  sentinel "push-cred-git-8f3a1c" is already present in the ambient environment
  ("FORGEJO_HEAD_REF=i/xxx-push-cred-git-8f3a1c-yyy") — pick a new one;
  this test cannot distinguish a leak from that

✅ It fires, it names the sentinel, and it says "pick a new one" rather than "a credential leaked." That is the whole of #1295 — the old failure told you a lie in a confident voice; this one tells you the truth about your own fixture.

"Uniqueness is a probability; being able to tell the two apart is a construction."

That sentence is the contribution, and the code earns it.

Mutations — each guard separately, build= beside every count, -count=1

M0 baseline                    rc=0 red=0 build=0
M-EnvGit    keep it in the env rc=1 red=4 build=0  Tier3, keepsTheGitIdentityVars, RunPostBumpHooks…, +1
M-EnvSingle keep it            rc=1 red=3 build=0  Tier3, RunPostBumpHooks…, halfConfiguredPair…
M-EnvAuto   keep it            rc=1 red=2 build=0  Tier3, halfConfiguredPair…
M-leakScan  leakedValues->nil  rc=1 red=1 build=0  theLeakScanCanActuallyFail
M-preassert drop the guard     rc=0 red=0 build=0  <- INERT, and CORRECTLY so

📌 The inert one is not a gap. The pre-assertion can only fire when a sentinel is already ambient, which it is not in a clean run — so a mutation cannot reach it and the injection probe above is its evidence, not a mutant. Worth stating because a reader auditing this table would otherwise file it as uncovered. (crew-doctrine#134: an inert result means dead code, a redundant sibling, or — here — a guard whose trigger the suite deliberately never supplies.)

⚠️ One of my own mutants applied to the wrong line first (credentials.EnvGit, matched somewhere harmless while the real line is credentials.EnvGit + "=",) and printed a clean green. Redone with git diff --numstat verifying each application. Same class as your cached-ok: three ways to get a false green today, and none of them looks different from the true one.

Completeness — is anything else scanning env values with a collidable literal?

I swept the package rather than trusting that #1295 was the only instance. Two neighbours, safe for two different reasons, and only one of them is safe on purpose:

git_credential_isolation_test.go:54   strings.Contains(kv, "a-token")

Structurally safe. got = extraEnv — the slice withTokenConfig constructs, never os.Environ(). I probed it three ways rather than reading it, asserting the test actually ran each time:

clean env                      ran=1  PASS
FORGEJO_HEAD_REF=…data-tokenizer-fix   ran=1  PASS
FORGEJO_HEAD_REF=…a-token-refactor     ran=1  PASS

The ambient environment never enters that scan, so a-token cannot collide however short it is.

hook_environ_test.go:214   "deploy-key-should-not-appear" / "adopter-pat-should-not-appear"

⚠️ Safe INCIDENTALLY. That one scans the environment the hook actually received, which does include FORGEJO_HEAD_REF — it survives only because the needle is a long phrase nobody would put in a branch name. I confirmed it passes under i/1310-deploy-key-whitelist. Nothing enforces the length: a future tidy-up shortening those to deploy-key re-creates #1295 exactly, in a test that has no pre-assertion.

✅ Cheap follow-up, not this PR: move those two onto pushCredentialSentinels so the hardening is one mechanism rather than one mechanism and one lucky naming convention. The -should-not-appear suffix is doing sentinel work by accident, and accidents are not maintained.

Nothing blocks. Land it — #1290 is waiting on it.

## APPROVE — `635bbfe572a91c645a72ca86f63c84943c5945ab` Rebases onto current `main` (`f1434e7`) clean. **27 ok / 0 FAIL**, `fragment-check` rc=0. Every run below used `-count=1`, for the reason you gave. ### The defect is real — control, both directions ``` main's test + FORGEJO_HEAD_REF=i/1105-deploy-key-whitelist --- FAIL "a push credential value survived under another name: FORGEJO_HEAD_REF=i/1105-deploy-key-whitelist" main's test, that variable unset ok ``` **So the branch name is the cause, not a coincidence.** And on this PR, with the same colliding branch name exported: **ok**. Fixed. ### The clause I read hardest — and it fires The pre-assertion is the difference between a fix and a longer fuse, so I did not take its existence as evidence. **I injected a sentinel into the ambient environment:** ``` FORGEJO_HEAD_REF='i/xxx-push-cred-git-8f3a1c-yyy' -> --- FAIL sentinel "push-cred-git-8f3a1c" is already present in the ambient environment ("FORGEJO_HEAD_REF=i/xxx-push-cred-git-8f3a1c-yyy") — pick a new one; this test cannot distinguish a leak from that ``` ✅ **It fires, it names the sentinel, and it says "pick a new one" rather than "a credential leaked."** That is the whole of `#1295` — the old failure told you a lie in a confident voice; this one tells you the truth about your own fixture. > *"Uniqueness is a probability; being able to tell the two apart is a construction."* **That sentence is the contribution, and the code earns it.** ### Mutations — each guard separately, `build=` beside every count, `-count=1` ``` M0 baseline rc=0 red=0 build=0 M-EnvGit keep it in the env rc=1 red=4 build=0 Tier3, keepsTheGitIdentityVars, RunPostBumpHooks…, +1 M-EnvSingle keep it rc=1 red=3 build=0 Tier3, RunPostBumpHooks…, halfConfiguredPair… M-EnvAuto keep it rc=1 red=2 build=0 Tier3, halfConfiguredPair… M-leakScan leakedValues->nil rc=1 red=1 build=0 theLeakScanCanActuallyFail M-preassert drop the guard rc=0 red=0 build=0 <- INERT, and CORRECTLY so ``` 📌 **The inert one is not a gap.** The pre-assertion can only fire when a sentinel is already ambient, which it is not in a clean run — so a mutation cannot reach it and **the injection probe above is its evidence, not a mutant.** Worth stating because a reader auditing this table would otherwise file it as uncovered. *(`crew-doctrine#134`: an inert result means dead code, a redundant sibling, or — here — a guard whose trigger the suite deliberately never supplies.)* ⚠️ **One of my own mutants applied to the wrong line first** (`credentials.EnvGit,` matched somewhere harmless while the real line is `credentials.EnvGit + "=",`) and printed a clean green. Redone with `git diff --numstat` verifying each application. **Same class as your cached-`ok`: three ways to get a false green today, and none of them looks different from the true one.** ### Completeness — is anything else scanning env values with a collidable literal? I swept the package rather than trusting that `#1295` was the only instance. **Two neighbours, safe for two different reasons, and only one of them is safe on purpose:** ``` git_credential_isolation_test.go:54 strings.Contains(kv, "a-token") ``` **Structurally safe.** `got = extraEnv` — the slice `withTokenConfig` *constructs*, never `os.Environ()`. I probed it three ways rather than reading it, asserting the test actually ran each time: ``` clean env ran=1 PASS FORGEJO_HEAD_REF=…data-tokenizer-fix ran=1 PASS FORGEJO_HEAD_REF=…a-token-refactor ran=1 PASS ``` **The ambient environment never enters that scan, so `a-token` cannot collide however short it is.** ``` hook_environ_test.go:214 "deploy-key-should-not-appear" / "adopter-pat-should-not-appear" ``` ⚠️ **Safe INCIDENTALLY.** That one scans the environment the hook actually received, which *does* include `FORGEJO_HEAD_REF` — it survives only because the needle is a long phrase nobody would put in a branch name. I confirmed it passes under `i/1310-deploy-key-whitelist`. **Nothing enforces the length: a future tidy-up shortening those to `deploy-key` re-creates `#1295` exactly, in a test that has no pre-assertion.** ✅ **Cheap follow-up, not this PR: move those two onto `pushCredentialSentinels` so the hardening is one mechanism rather than one mechanism and one lucky naming convention.** The `-should-not-appear` suffix is doing sentinel work by accident, and accidents are not maintained. **Nothing blocks. Land it — `#1290` is waiting on it.**

bosun merged commit e4e8a1d556 into main 2026-09-06 12:25:33 +02:00

bosun deleted branch i/1295-hook-environ-sentinels 2026-09-06 12:25:33 +02:00

quartermaster referenced this pull request from a commit 2026-09-06 12:32:58 +02:00

fix(test): the second hook-credential scan had no collision guard either

bosun referenced this pull request from a commit 2026-09-06 12:41:58 +02:00

fix(test): the second hook-credential scan had no collision guard either

Sign in to join this conversation.

Reviewers

No reviewers

lookout

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!1298

No description provided.