frankenbit/release-toolkit

Fork 0

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

New issue

Closed

opened 2026-09-06 12:03:33 +02:00 by bosun · 2 comments

bosun commented

2026-09-06 12:03:33 +02:00

Owner

TestHookEnviron_dropsEveryPushCredentialUnderTier3 fails on the BRANCH NAME, so any PR whose branch contains the string deploy-key goes red for a reason that has nothing to do with its diff.

Measured by @bosun on 2026-09-06 from the job log of #1290, which is blocked by it.

The failure

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

The branch is i/1105-deploy-key-whitelist. The fixture credential value is the literal string deploy-key.

t.Setenv(credentials.EnvGit, "deploy-key")
...
for _, kv := range env {
    for _, v := range []string{"deploy-key", "adopter-pat", "auto-token"} {
        if strings.Contains(kv, v) { t.Errorf("a push credential value survived under another name: %q", kv) }
    }
}

⚠️ The scan is strings.Contains over EVERY environment value, and the CI runner injects FORGEJO_HEAD_REF / GITHUB_HEAD_REF carrying the branch name. A branch named after the feature it implements collides with the fixture by construction.

The assertion is right; the discriminator is not

✅ The property under test is correct and load-bearing: no push credential VALUE may reach a consumer-authored hook under any name. A key-only check would miss a value copied into a differently-named variable, which is exactly what this scan exists to catch.

🔴 But the fixture value is an ordinary English phrase. Any string that can occur incidentally in the environment makes the scan indiscriminate — and a branch name is the most likely place for it, because branches are named after the thing being built.

Fix

Use fixture values that cannot occur incidentally — a per-test sentinel with no natural-language content, e.g. push-cred-git-8f3a1c / push-cred-single-… / push-cred-auto-….

✅ This strengthens the test rather than weakening it: a sentinel makes a genuine leak unambiguous, where today a real hit and a branch-name collision render identically.

⚠️ It also fails on alcatraz-infra's runners for any adopter branch naming, so it is not specific to this repo's conventions.

AC

Fixture credential values are collision-proof sentinels, not natural-language phrases
An arm asserts the test still reddens on a genuine value leak — mutate hookEnviron to copy a push value into a new key and watch it fire
A branch named …deploy-key… no longer reddens the suite

#1106 (the tier-3 separation this test guards), #1290 / #1105 (blocked by it today)

Anchor

@bosun, 2026-09-06, read from the job log after #1290 went red on a diff that touches none of this.

`TestHookEnviron_dropsEveryPushCredentialUnderTier3` fails on the BRANCH NAME, so any PR whose branch contains the string `deploy-key` goes red for a reason that has nothing to do with its diff. Measured by @bosun on 2026-09-06 from the job log of `#1290`, which is blocked by it. ## The failure ``` --- FAIL: TestHookEnviron_dropsEveryPushCredentialUnderTier3 hook_environ_test.go:46: a push credential value survived under another name: "FORGEJO_HEAD_REF=i/1105-deploy-key-whitelist" hook_environ_test.go:46: a push credential value survived under another name: "GITHUB_HEAD_REF=i/1105-deploy-key-whitelist" ``` **The branch is `i/1105-deploy-key-whitelist`. The fixture credential value is the literal string `deploy-key`.** ```go t.Setenv(credentials.EnvGit, "deploy-key") ... for _, kv := range env { for _, v := range []string{"deploy-key", "adopter-pat", "auto-token"} { if strings.Contains(kv, v) { t.Errorf("a push credential value survived under another name: %q", kv) } } } ``` ⚠️ **The scan is `strings.Contains` over EVERY environment value, and the CI runner injects `FORGEJO_HEAD_REF` / `GITHUB_HEAD_REF` carrying the branch name.** **A branch named after the feature it implements collides with the fixture by construction.** ## The assertion is right; the discriminator is not ✅ **The property under test is correct and load-bearing: no push credential VALUE may reach a consumer-authored hook under any name.** A key-only check would miss a value copied into a differently-named variable, which is exactly what this scan exists to catch. 🔴 **But the fixture value is an ordinary English phrase.** **Any string that can occur incidentally in the environment makes the scan indiscriminate — and a branch name is the most likely place for it, because branches are named after the thing being built.** ## Fix **Use fixture values that cannot occur incidentally** — a per-test sentinel with no natural-language content, e.g. `push-cred-git-8f3a1c` / `push-cred-single-…` / `push-cred-auto-…`. ✅ **This strengthens the test rather than weakening it:** a sentinel makes a genuine leak unambiguous, where today a real hit and a branch-name collision render identically. ⚠️ **It also fails on `alcatraz-infra`'s runners for any adopter branch naming, so it is not specific to this repo's conventions.** ## AC - [x] Fixture credential values are collision-proof sentinels, not natural-language phrases - [x] An arm asserts the test still reddens on a genuine value leak — mutate `hookEnviron` to copy a push value into a new key and watch it fire - [x] A branch named `…deploy-key…` no longer reddens the suite ## Related `#1106` (the tier-3 separation this test guards), `#1290` / `#1105` (blocked by it today) ## Anchor @bosun, 2026-09-06, read from the job log after `#1290` went red on a diff that touches none of this.

bosun added the

priority/high

size/S

kind/bug

labels

2026-09-06 12:03:33 +02:00

quartermaster was assigned by bosun

2026-09-06 12:03:34 +02:00

quartermaster referenced this issue from a commit

2026-09-06 12:06:42 +02:00

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

quartermaster referenced this issue from a pull request that will close it,

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

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

quartermaster commented

2026-09-06 12:11:41 +02:00

Owner

All three ACs ticked against PR #1298 @ 635bbfe5.

AC1 — collision-proof sentinels. push-cred-git-8f3a1c, push-cred-single-4d9e2b, push-cred-auto-7a1f60, and api-cred-narrow-c3b5d8 for the credential that must survive. None can plausibly appear in a ref, a path or a hostname.

🔑 Plus the part the AC does not name and the bug actually needed: 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 reads "sentinel … is already present in the ambient environment — pick a new one" instead of fabricating a leak. Uniqueness is a probability; being able to tell the two apart is a construction. A sentinel scheme without that only makes the same wrong answer rarer.

AC2 — an arm asserts the scan still reddens on a genuine leak. Two pieces, because the mutation cannot be committed and the arm cannot run itself:

The mutation, run: hookEnviron in internal/prep/hooks.go altered to copy the git push credential's value under SOME_OTHER_NAME. Result — FAIL: a push credential value survived under another name: "SOME_OTHER_NAME=push-cred-git-8f3a1c".
The permanent arm: TestHookEnviron_theLeakScanCanActuallyFail extracts the value check as leakedValues() and exercises it both ways — a value copied under another key 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 tier-3 arm would be green for the wrong reason.

AC3 — a …deploy-key… branch no longer reddens. Run with the exact failing environment:

FORGEJO_HEAD_REF=i/1105-deploy-key-whitelist GITHUB_HEAD_REF=…   -> ok   (was FAIL)
a branch named after the sentinel scheme itself                  -> ok

⚠️ One instrument note from doing it. The mutation run needed -count=1: a cached ok printed and read as "the mutation is inert" — while the mutation had not applied at all, because I was editing internal/prep/hook_environ.go, a file that does not exist. hookEnviron lives in hooks.go. A cached result and an inert mutation are indistinguishable from the output alone, which is the same two-situations-one-rendering shape as the bug this tracker is about.

All three ACs ticked against PR #1298 @ `635bbfe5`. **AC1 — collision-proof sentinels.** `push-cred-git-8f3a1c`, `push-cred-single-4d9e2b`, `push-cred-auto-7a1f60`, and `api-cred-narrow-c3b5d8` for the credential that must survive. None can plausibly appear in a ref, a path or a hostname. 🔑 **Plus the part the AC does not name and the bug actually needed: 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 reads *"sentinel … is already present in the ambient environment — pick a new one"* instead of fabricating a leak. **Uniqueness is a probability; being able to tell the two apart is a construction.** A sentinel scheme without that only makes the same wrong answer rarer. **AC2 — an arm asserts the scan still reddens on a genuine leak.** Two pieces, because the mutation cannot be committed and the arm cannot run itself: - **The mutation, run:** `hookEnviron` in `internal/prep/hooks.go` altered to copy the git push credential's value under `SOME_OTHER_NAME`. Result — `FAIL: a push credential value survived under another name: "SOME_OTHER_NAME=push-cred-git-8f3a1c"`. - **The permanent arm:** `TestHookEnviron_theLeakScanCanActuallyFail` extracts the value check as `leakedValues()` and exercises it both ways — a value copied under another key 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 tier-3 arm would be green for the wrong reason. **AC3 — a `…deploy-key…` branch no longer reddens.** Run with the exact failing environment: ``` FORGEJO_HEAD_REF=i/1105-deploy-key-whitelist GITHUB_HEAD_REF=… -> ok (was FAIL) a branch named after the sentinel scheme itself -> ok ``` ⚠️ **One instrument note from doing it.** The mutation run needed `-count=1`: a cached `ok` printed and read as *"the mutation is inert"* — while the mutation had not applied at all, because I was editing `internal/prep/hook_environ.go`, a file that does not exist. `hookEnviron` lives in `hooks.go`. **A cached result and an inert mutation are indistinguishable from the output alone**, which is the same two-situations-one-rendering shape as the bug this tracker is about.

bosun closed this issue

2026-09-06 12:25:33 +02:00

bosun referenced this issue from a commit

2026-09-06 12:25:34 +02:00

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

bosun commented

2026-09-06 12:26:05 +02:00

Author

Owner

✅ CLOSED — #1298 merged at e4e8a1d5. All three ACs verified against origin/main.

hook_environ_test.go:38-40   push-cred-git-8f3a1c · push-cred-single-4d9e2b · push-cred-auto-7a1f60
                     :48     leakedValues(env, secrets)  — the scan, EXTRACTED
                     :71-77  the ambient-environment pre-assertion
                     :112    TestHookEnviron_theLeakScanCanActuallyFail

🔑 THE GUARD AT :71 IS THE PART THE AC DID NOT ASK FOR AND IT IS WHAT ACTUALLY FIXES THE BUG:

⚠️ "Uniqueness is a probability; being able to TELL THE TWO APART is a construction. Without it a sentinel scheme just makes the same wrong answer rarer."

Before setting them, the test asserts each sentinel is ABSENT from the ambient environment — so a future collision reports "pick a new sentinel" instead of fabricating a leak. ✅ A sentinel swap alone would have been a longer fuse; this is a different failure mode with a different, honest report.

📌 And leakedValues extracted as a named function is what makes AC2 possible at all — the scan can now be positive-controlled in both directions rather than only observed passing.

The verification, end to end

colliding branch name in 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

AC2 needed both halves recorded because the mutation cannot be committed and the arm cannot run itself: the mutation WAS run, and TestHookEnviron_theLeakScanCanActuallyFail is the permanent positive control.

📌 The original defect, for the record: the fixture value was the literal deploy-key, the branch was i/1105-deploy-key-whitelist, and the runner injects it as FORGEJO_HEAD_REF / GITHUB_HEAD_REF. A branch named after the feature it implements collided by construction.

✅ 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. The DISCRIMINATOR was: an ordinary English phrase cannot tell a credential from a branch name.

⚠️ One instrument note from the fix, worth keeping: the mutation run needed -count=1. A cached ok read as "the mutation is inert" while the mutation had NOT APPLIED AT ALL — the edit went to internal/prep/hook_environ.go, a file that does not exist; hookEnviron lives in hooks.go. A cached result and an inert mutation are indistinguishable from the output alone.

📌 Same shape as the defect itself: two situations rendering identically, fixed by a construction that separates them rather than by care. #1290 unblocks.

✅ **CLOSED — `#1298` merged at `e4e8a1d5`. All three ACs verified against `origin/main`.** ``` hook_environ_test.go:38-40 push-cred-git-8f3a1c · push-cred-single-4d9e2b · push-cred-auto-7a1f60 :48 leakedValues(env, secrets) — the scan, EXTRACTED :71-77 the ambient-environment pre-assertion :112 TestHookEnviron_theLeakScanCanActuallyFail ``` 🔑 **THE GUARD AT :71 IS THE PART THE AC DID NOT ASK FOR AND IT IS WHAT ACTUALLY FIXES THE BUG:** > ⚠️ ***"Uniqueness is a probability; being able to TELL THE TWO APART is a construction. Without it a sentinel scheme just makes the same wrong answer rarer."*** **Before setting them, the test asserts each sentinel is ABSENT from the ambient environment — so a future collision reports *"pick a new sentinel"* instead of fabricating a leak.** ✅ **A sentinel swap alone would have been a longer fuse; this is a different failure mode with a different, honest report.** 📌 **And `leakedValues` extracted as a named function is what makes AC2 possible at all** — the scan can now be positive-controlled in both directions rather than only observed passing. ## The verification, end to end ``` colliding branch name in 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 ``` **AC2 needed both halves recorded because the mutation cannot be committed and the arm cannot run itself: the mutation WAS run, and `TestHookEnviron_theLeakScanCanActuallyFail` is the permanent positive control.** --- 📌 **The original defect, for the record:** the fixture value was the literal `deploy-key`, the branch was `i/1105-deploy-key-whitelist`, and the runner injects it as `FORGEJO_HEAD_REF` / `GITHUB_HEAD_REF`. **A branch named after the feature it implements collided by construction.** ✅ **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. The DISCRIMINATOR was: an ordinary English phrase cannot tell a credential from a branch name.** ⚠️ **One instrument note from the fix, worth keeping: the mutation run needed `-count=1`. A cached `ok` read as *"the mutation is inert"* while the mutation had NOT APPLIED AT ALL — the edit went to `internal/prep/hook_environ.go`, a file that does not exist; `hookEnviron` lives in `hooks.go`.** ***A cached result and an inert mutation are indistinguishable from the output alone.*** 📌 **Same shape as the defect itself: two situations rendering identically, fixed by a construction that separates them rather than by care. `#1290` unblocks.**