From 6b4f7009b531013c2fe790db30bb35ec99bcee4b Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Tue, 22 Sep 2026 10:24:07 +0200 Subject: [PATCH 1/2] ci: every checkout turns the job's token off, and a guard asks each one actions/checkout leaves the token it cloned with in .git/config, and the steps after it run go test over the pull request's own code, which can read that file. An outside review of #117 named it on the one job it was reading, and #117 turned the token off there. A fix at one job of twenty four was an inconsistency (O230). Checked per step before turning off all of them: no step in these workflows pushes, fetches or commits after a checkout. The only git commands are diff, cat-file and rev-parse, all local. The release, the attestation and the pages talk to GitHub through gh with a token in the environment, or through actions that carry their own, and none of that reads .git/config. The weight is small - a pull request's token from a fork is read only and the workflows ask for contents: read - and the class is real on every checkout alike. The guard reads each workflow through the YAML parser rather than the line under uses:, because in pages.yml the with block sits under a comment and the key can sit anywhere inside it. It self tests its predicate on six shapes the tree does not contain, since every checkout is off now and a rule that weakened would find nothing to let through. It counts what it saw and refuses a walk that found far fewer than the twenty four measured. Two mutations. Co-Authored-By: Claude Opus 5 --- .github/workflows/attest-release.yml | 2 + .github/workflows/ci.yml | 33 +++++- .github/workflows/dependency-review.yml | 2 + .github/workflows/dev-build.yml | 4 + .github/workflows/pages.yml | 1 + .github/workflows/release.yml | 8 ++ .github/workflows/tool-versions.yml | 2 + .github/workflows/verify-release.yml | 4 + internal/guard/checkoutcredentials_test.go | 122 +++++++++++++++++++++ 9 files changed, 173 insertions(+), 5 deletions(-) create mode 100644 internal/guard/checkoutcredentials_test.go diff --git a/.github/workflows/attest-release.yml b/.github/workflows/attest-release.yml index 2bf77100..f441e4ab 100644 --- a/.github/workflows/attest-release.yml +++ b/.github/workflows/attest-release.yml @@ -45,6 +45,8 @@ jobs: attestations: write steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false - name: fetch what the maintainer signed shell: bash diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2764959f..23f0aab5 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -67,7 +67,16 @@ jobs: # holds this job to setting it and the imports job to not setting it. TFG_IMPORT_TABLE_JOB: "1" steps: + # Every checkout in these workflows turns the token off. The checkout + # keeps the job's token in .git/config unless told not to, and the jobs + # go on to run code from the pull request under test. Nothing after a + # checkout here pushes or fetches - the release and the pages talk to + # GitHub through gh and through actions that carry their own token - so + # the credential has no use once the tree is on disk. A guard asks each + # checkout (checkoutcredentials_test.go), so a new one cannot forget. - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false - uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 with: @@ -353,11 +362,6 @@ jobs: timeout-minutes: 15 steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - # The checkout keeps the job's token in .git/config unless told not - # to, and this job goes on to run go test over the pull request's - # own code. Nothing here pushes, so the token has no use after the - # checkout. Asked for by an outside review of #117, and the other - # checkouts in these workflows are O230. with: persist-credentials: false @@ -383,6 +387,8 @@ jobs: timeout-minutes: 15 steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false - uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 with: @@ -425,6 +431,8 @@ jobs: timeout-minutes: 15 steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false - uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 with: @@ -468,6 +476,8 @@ jobs: timeout-minutes: 15 steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false - uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 with: @@ -512,6 +522,8 @@ jobs: timeout-minutes: 15 steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false - uses: actions/setup-python@5fda3b95a4ea91299a34e894583c3862153e4b97 # v7.0.0 with: @@ -576,6 +588,8 @@ jobs: timeout-minutes: 20 steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false - uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 with: @@ -646,6 +660,7 @@ jobs: steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: + persist-credentials: false # The comparison needs the earlier commit, and the default checkout # fetches one. fetch-depth: 0 @@ -742,6 +757,8 @@ jobs: CGO_ENABLED: "1" steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false - uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 with: @@ -817,6 +834,8 @@ jobs: CGO_ENABLED: "0" steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false - uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 with: @@ -893,6 +912,8 @@ jobs: CGO_ENABLED: "0" steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false - uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 with: @@ -948,6 +969,8 @@ jobs: CGO_ENABLED: "0" steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false - uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 with: diff --git a/.github/workflows/dependency-review.yml b/.github/workflows/dependency-review.yml index fed68d59..f5b8ab20 100644 --- a/.github/workflows/dependency-review.yml +++ b/.github/workflows/dependency-review.yml @@ -26,6 +26,8 @@ jobs: timeout-minutes: 10 steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false - uses: actions/dependency-review-action@a1d282b36b6f3519aa1f3fc636f609c47dddb294 # v5.0.0 with: diff --git a/.github/workflows/dev-build.yml b/.github/workflows/dev-build.yml index 0e376ccf..c45562fa 100644 --- a/.github/workflows/dev-build.yml +++ b/.github/workflows/dev-build.yml @@ -69,6 +69,8 @@ jobs: CGO_ENABLED: "0" steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false - uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 with: @@ -153,6 +155,8 @@ jobs: shell: bash steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false - uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 with: diff --git a/.github/workflows/pages.yml b/.github/workflows/pages.yml index 818310ed..c58709d7 100644 --- a/.github/workflows/pages.yml +++ b/.github/workflows/pages.yml @@ -65,6 +65,7 @@ jobs: # nosemgrep: yaml.github-actions.security.workflow-run-target-code-checkout.workflow-run-target-code-checkout - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: + persist-credentials: false # The commit CI passed on, not whatever main happens to be now. On a # dispatch there is no such commit and the default ref is right. ref: ${{ github.event.workflow_run.head_sha || github.ref }} diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 583d9ff0..ecc66b47 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -42,6 +42,8 @@ jobs: CGO_ENABLED: "0" steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false - uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 with: @@ -151,6 +153,8 @@ jobs: CGO_ENABLED: "0" steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false - uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 with: @@ -258,6 +262,8 @@ jobs: shell: bash steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false - uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 with: @@ -363,6 +369,8 @@ jobs: attestations: write steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false - uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 with: diff --git a/.github/workflows/tool-versions.yml b/.github/workflows/tool-versions.yml index 45f7b2cb..68dc02b7 100644 --- a/.github/workflows/tool-versions.yml +++ b/.github/workflows/tool-versions.yml @@ -39,6 +39,8 @@ jobs: timeout-minutes: 10 steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false - name: compare each pin with its latest release env: diff --git a/.github/workflows/verify-release.yml b/.github/workflows/verify-release.yml index cdc51416..e14511e6 100644 --- a/.github/workflows/verify-release.yml +++ b/.github/workflows/verify-release.yml @@ -41,6 +41,8 @@ jobs: contents: read steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false - name: which release this is id: which @@ -151,6 +153,8 @@ jobs: contents: read steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false - name: which release this is id: which diff --git a/internal/guard/checkoutcredentials_test.go b/internal/guard/checkoutcredentials_test.go new file mode 100644 index 00000000..403fe2e8 --- /dev/null +++ b/internal/guard/checkoutcredentials_test.go @@ -0,0 +1,122 @@ +package guard + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/goccy/go-yaml" +) + +// Every checkout in a workflow turns the job's token off once the tree is on +// disk. +// +// actions/checkout writes the token it cloned with into .git/config and leaves +// it there, so that a later step can push. No step in these workflows pushes: +// the release, the attestation and the pages talk to GitHub through gh with a +// token in the environment, or through actions that carry their own. What the +// jobs do after a checkout is run go test over the code of the pull request +// under test - and that code can read .git/config. The token of a pull request +// from a fork is read only, so the class is real and the weight is small, and +// it is the same class on every one of the twenty four checkouts, which is why +// the answer is every one of them and a guard, not a fix at the job somebody +// happened to review (O230). +// +// Asked through the YAML parser rather than by searching the text, because +// `with:` can sit under a comment block and the key can sit anywhere inside +// it - a regular expression tying the key to the line after `uses:` would read +// pages.yml wrong today. +func TestEveryCheckoutTurnsItsTokenOff(t *testing.T) { + // Asking the predicate about shapes the tree does not currently contain. + // Every checkout DOES turn its token off today, so a change that weakened + // the rule would find nothing to let through and stay green. These cases + // keep a hold on the rule itself. + for _, c := range []struct { + with map[string]any + off bool + why string + }{ + {map[string]any{"persist-credentials": false}, true, "the key is there and false"}, + {map[string]any{"persist-credentials": "false"}, true, "the action reads its inputs as strings, so a quoted false is the same answer"}, + {nil, false, "no with block at all means the default, which keeps the token"}, + {map[string]any{"fetch-depth": 0}, false, "a with block that says nothing about the token keeps it"}, + {map[string]any{"persist-credentials": true}, false, "the key is there and true"}, + {map[string]any{"persist-credentials": "no"}, false, "a word that is not false is not false"}, + } { + if tokenTurnedOff(c.with) != c.off { + t.Errorf("tokenTurnedOff(%v) should be %v, because %s", c.with, c.off, c.why) + } + } + + dir := filepath.Join(repoRoot(t), ".github", "workflows") + entries, err := os.ReadDir(dir) + if err != nil { + t.Skipf("the workflows are not here: %v", err) + } + + seen := 0 + for _, e := range entries { + if e.IsDir() || !strings.HasSuffix(e.Name(), ".yml") { + continue + } + body, err := os.ReadFile(filepath.Join(dir, e.Name())) + if err != nil { + t.Fatalf("reading %s: %v", e.Name(), err) + } + var workflow struct { + Jobs map[string]struct { + Steps []struct { + Uses string `yaml:"uses"` + With map[string]any `yaml:"with"` + } `yaml:"steps"` + } `yaml:"jobs"` + } + if err := yaml.Unmarshal(body, &workflow); err != nil { + t.Fatalf("reading %s: %v", e.Name(), err) + } + for jobName, job := range workflow.Jobs { + for _, step := range job.Steps { + if !strings.HasPrefix(step.Uses, "actions/checkout@") { + continue + } + seen++ + if tokenTurnedOff(step.With) { + continue + } + t.Errorf("%s, job %q checks out with the token left in .git/config, and the steps "+ + "after it run the pull request's own code. Nothing in these workflows pushes, "+ + "so turn it off:\n"+ + " with:\n"+ + " persist-credentials: false", + e.Name(), jobName) + } + } + } + + // Measured 2026-09-22: twenty four checkouts across eight workflows. A + // walk that found far fewer would report a clean tree while reading + // nothing - a renamed key, a changed suffix, a parser that stopped seeing + // steps - which is the way this guard is most likely to break. + if seen < 20 { + t.Errorf("only %d checkouts were found under %s, and there are twenty four. Either the "+ + "workflows moved or the way this reads them stopped working, and this guard "+ + "checked nothing", seen, dir) + } +} + +// tokenTurnedOff reports whether a checkout's with block says +// persist-credentials: false. +// +// A bare false parses as a boolean and a quoted one as a string, and the +// action reads either as false, so both are accepted. Anything else - the key +// missing, true, or a word - leaves the token where the action puts it. +func tokenTurnedOff(with map[string]any) bool { + switch v := with["persist-credentials"].(type) { + case bool: + return !v + case string: + return v == "false" + } + return false +} From 8727378cccfb00680f34bc17209dd616843a2e95 Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Tue, 22 Sep 2026 10:38:01 +0200 Subject: [PATCH 2/2] guard: the checkout guard reads .yaml too, and two readings of the count agree An outside review of #118 made two points about the guard. The first is right: actions/checkout in a file ending .yaml would run and go unasked, where the pinning guard beside it takes both suffixes. The second mixed two things - a checkout the walk sees without the key is reported whatever the count says, and the count guards against a parser that stopped seeing steps - but the floor of twenty was a number from nowhere. The proposed exact constant of twenty four is turned down: kept by hand, it goes stale with the next workflow and would turn red on a good change, a new checkout with the key and no bump. Instead the text is read once more, for a second count of "uses: actions/checkout@" outside comments, and the YAML walk has to find the same number. No constant, and a parser dropping part of the steps is caught by the disagreement. A third mutation proves it: the walk stops recognising a checkout, the text still holds twenty four. Co-Authored-By: Claude Opus 5 --- internal/guard/checkoutcredentials_test.go | 43 ++++++++++++++-------- 1 file changed, 28 insertions(+), 15 deletions(-) diff --git a/internal/guard/checkoutcredentials_test.go b/internal/guard/checkoutcredentials_test.go index 403fe2e8..8f0fb0c1 100644 --- a/internal/guard/checkoutcredentials_test.go +++ b/internal/guard/checkoutcredentials_test.go @@ -26,7 +26,13 @@ import ( // Asked through the YAML parser rather than by searching the text, because // `with:` can sit under a comment block and the key can sit anywhere inside // it - a regular expression tying the key to the line after `uses:` would read -// pages.yml wrong today. +// pages.yml wrong today. The text is still read, once, for a second count of +// the checkouts: a parser that stopped seeing some of the steps would +// otherwise report a clean tree over the ones it dropped. Two readers that +// have to agree replace a number somebody would keep by hand - an outside +// review of #118 asked for an exact count of twenty four, and that constant +// would go stale with the next workflow while proving nothing the agreement +// does not. func TestEveryCheckoutTurnsItsTokenOff(t *testing.T) { // Asking the predicate about shapes the tree does not currently contain. // Every checkout DOES turn its token off today, so a change that weakened @@ -55,15 +61,17 @@ func TestEveryCheckoutTurnsItsTokenOff(t *testing.T) { t.Skipf("the workflows are not here: %v", err) } - seen := 0 + seen, inText := 0, 0 for _, e := range entries { - if e.IsDir() || !strings.HasSuffix(e.Name(), ".yml") { + name := e.Name() + if e.IsDir() || (!strings.HasSuffix(name, ".yml") && !strings.HasSuffix(name, ".yaml")) { continue } - body, err := os.ReadFile(filepath.Join(dir, e.Name())) + body, err := os.ReadFile(filepath.Join(dir, name)) if err != nil { - t.Fatalf("reading %s: %v", e.Name(), err) + t.Fatalf("reading %s: %v", name, err) } + inText += strings.Count(withoutYamlComments(string(body)), "uses: actions/checkout@") var workflow struct { Jobs map[string]struct { Steps []struct { @@ -73,7 +81,7 @@ func TestEveryCheckoutTurnsItsTokenOff(t *testing.T) { } `yaml:"jobs"` } if err := yaml.Unmarshal(body, &workflow); err != nil { - t.Fatalf("reading %s: %v", e.Name(), err) + t.Fatalf("reading %s: %v", name, err) } for jobName, job := range workflow.Jobs { for _, step := range job.Steps { @@ -89,19 +97,24 @@ func TestEveryCheckoutTurnsItsTokenOff(t *testing.T) { "so turn it off:\n"+ " with:\n"+ " persist-credentials: false", - e.Name(), jobName) + name, jobName) } } } - // Measured 2026-09-22: twenty four checkouts across eight workflows. A - // walk that found far fewer would report a clean tree while reading - // nothing - a renamed key, a changed suffix, a parser that stopped seeing - // steps - which is the way this guard is most likely to break. - if seen < 20 { - t.Errorf("only %d checkouts were found under %s, and there are twenty four. Either the "+ - "workflows moved or the way this reads them stopped working, and this guard "+ - "checked nothing", seen, dir) + // The two readings have to agree, and the text has to have found + // something. A parser that stopped seeing steps - a renamed key, a + // changed suffix, a shape it does not decode - would otherwise report a + // clean tree over the checkouts it dropped, which is the way this guard + // is most likely to break. Measured 2026-09-22: twenty four, by both. + if inText == 0 { + t.Errorf("no checkout was found in the text under %s, so this guard checked nothing. "+ + "Either the workflows moved or the way this reads them stopped working", dir) + } + if seen != inText { + t.Errorf("the YAML walk found %d checkouts and the text holds %d. The walk is the one "+ + "that judges them, so every checkout it does not see is one it does not ask "+ + "- find out which shape it stopped decoding", seen, inText) } }