From 51a4e465cbeb60f288bf8830760b6337df06fd2b Mon Sep 17 00:00:00 2001 From: Jeff Martin Date: Sun, 11 Oct 2026 13:18:47 -0700 Subject: [PATCH 1/4] Check mutable-ref pins against ref history under --verify Commits from a fork network are reachable under the upstream repo name, so a hand-edited actions.lock can lock a branch or partial-version ref to a commit that was never in that ref's history. Since #145 nothing caught this on --verify. --verify now resolves the ref head inside the batched parity query (or REST under the Dependabot proxy). When the head differs from the locked commit, one Compare call decides: not an ancestor is unreachable-pin, inconclusive is a warning (exit 2 under the proxy). Plain runs make no extra calls. --- README.md | 5 ++ cmd/gh-actions-lock/parity_test.go | 118 +++++++++++++++++++++++++++++ cmd/gh-actions-lock/run.go | 4 +- internal/ghapi/graphql_parity.go | 27 ++++++- internal/ghapi/repos.go | 7 +- internal/pipeline/parity.go | 27 ++++++- internal/pipeline/run.go | 5 +- 7 files changed, 183 insertions(+), 10 deletions(-) diff --git a/README.md b/README.md index a1f645d3..ed4a27a9 100644 --- a/README.md +++ b/README.md @@ -53,6 +53,11 @@ To check without writing anything, for example in CI, run: gh actions-lock --verify ``` +`--verify` also checks that a commit locked under a branch or partial version +(e.g. `main`, `v4`) is still in that ref's history. `--verify-local` runs +offline and only checks that every ref has a lockfile entry, not that the +entries are trustworthy; use `--verify` in CI for that. + Pins to branches or partial versions (e.g. `main`, `v4`) are not advanced when upstream moves. To regenerate the lockfile from the current upstream commits, run: diff --git a/cmd/gh-actions-lock/parity_test.go b/cmd/gh-actions-lock/parity_test.go index c264fec8..a8e28e16 100644 --- a/cmd/gh-actions-lock/parity_test.go +++ b/cmd/gh-actions-lock/parity_test.go @@ -716,3 +716,121 @@ func TestParity_DependabotProxyFirstRunKeepsRenamedName(t *testing.T) { assert.Contains(t, lock, "'krzema12/github-actions-typing@v2.2.2':") assert.NotContains(t, lock, "typesafegithub") } + +// TestParity_VerifyMutableRefHistory: --verify checks that a commit locked +// under a mutable ref is still in that ref's history, since a fork-network +// commit exists under the upstream name. A legitimately moved ref stays +// sticky and silent. Plain runs make no extra calls. +func TestParity_VerifyMutableRefHistory(t *testing.T) { + const live = "cccccccccccccccccccccccccccccccccccccccc" + compareURL := `repos/actions/checkout/compare/` + parityTestSHA + `\.\.\.` + live + tests := []struct { + name string + compare httpmock.Responder + // want is the finding category, "" for a clean pass. + want string + }{ + {"impostor blocks", httpmock.JSONResponse(map[string]any{"status": "diverged", "merge_base_commit": map[string]any{"sha": parityTestMoved}}), "unreachable-pin"}, + {"moved forward passes", httpmock.JSONResponse(map[string]any{"status": "ahead", "merge_base_commit": map[string]any{"sha": parityTestSHA}}), ""}, + {"compare 503 is inconclusive", httpmock.StatusResponse(http.StatusServiceUnavailable), "reachability-unknown"}, + } + for _, tt := range tests { + for _, proxy := range []bool{false, true} { + t.Run(tt.name+"/"+map[bool]string{false: "graphql", true: "dependabot proxy"}[proxy], func(t *testing.T) { + reg := &httpmock.Registry{} + if proxy { + t.Setenv("GH_ACTIONS_LOCK_DEPENDABOT_PROXY", "1") + useDefaultTransport(t, reg) + reg.Register(httpmock.REST("GET", `^/repos/actions/checkout$`), httpmock.JSONResponse(map[string]any{ + "full_name": "actions/checkout", "id": 1, "owner": map[string]any{"id": 1}, + })) + reg.Register(httpmock.REST("GET", `^/repos/actions/checkout/commits/`+parityTestSHA+`$`), httpmock.JSONResponse(map[string]any{"sha": parityTestSHA})) + reg.Register(httpmock.REST("GET", `^/repos/actions/checkout/commits/v4$`), httpmock.JSONResponse(map[string]any{"sha": live})) + } else { + reg.Register(httpmock.GraphQL(`head: object\(expression`), httpmock.JSONResponse(map[string]any{ + "data": map[string]any{"a0": map[string]any{ + "nameWithOwner": "actions/checkout", + "commit": map[string]any{"oid": parityTestSHA}, + "head": map[string]any{"oid": live}, + }}, + })) + } + for range 10 { + reg.Register(httpmock.REST("GET", compareURL), tt.compare) + } + path := writeTempWorkflow(t, ` +name: ci +on: push +jobs: + test: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 +`, "actions/checkout@v4=sha1-"+parityTestSHA) + + stdout, stderr, err := runCommandWithHTTP(t, reg, "--verify", "--json=valid,findings", path) + if proxy && tt.want == "reachability-unknown" { + require.Error(t, err, stderr) + assert.NotErrorIs(t, err, errSilent, "inconclusive under the proxy is a retry") + assert.Contains(t, err.Error(), "HTTP 503") + return + } + var p parityPayload + require.NoError(t, json.Unmarshal([]byte(stdout), &p), stdout+stderr) + switch tt.want { + case "": + require.NoError(t, err, stderr) + assert.Empty(t, p.Findings, "a moved mutable ref stays sticky and silent") + case "unreachable-pin": + require.ErrorIs(t, err, errSilent) + require.Len(t, p.Findings, 1) + assert.Equal(t, "unreachable-pin", p.Findings[0].Category) + assert.Equal(t, "error", p.Findings[0].Severity) + assert.Contains(t, p.Findings[0].Detail, "is not in actions/checkout's history") + assert.Contains(t, p.Findings[0].Detail, "v4 currently points at cccccccccccc") + default: + require.NoError(t, err, stderr) + require.Len(t, p.Findings, 1) + assert.Equal(t, tt.want, p.Findings[0].Category) + assert.Equal(t, "warning", p.Findings[0].Severity) + } + }) + } + } + + for _, proxy := range []bool{false, true} { + t.Run("plain run makes no extra calls/"+map[bool]string{false: "graphql", true: "dependabot proxy"}[proxy], func(t *testing.T) { + reg := &httpmock.Registry{} + if proxy { + t.Setenv("GH_ACTIONS_LOCK_DEPENDABOT_PROXY", "1") + useDefaultTransport(t, reg) + reg.Register(httpmock.REST("GET", `^/repos/actions/checkout$`), httpmock.JSONResponse(map[string]any{ + "full_name": "actions/checkout", "id": 1, "owner": map[string]any{"id": 1}, + })) + reg.Register(httpmock.REST("GET", `^/repos/actions/checkout/commits/`+parityTestSHA+`$`), httpmock.JSONResponse(map[string]any{"sha": parityTestSHA})) + } else { + reg.Register(httpmock.GraphQL(`head: object\(expression`), func(*http.Request) (*http.Response, error) { + t.Error("a plain run must not resolve mutable refs") + return nil, io.EOF + }) + registerParity(reg) + } + path := writeTempWorkflow(t, ` +name: ci +on: push +jobs: + test: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 +`, "actions/checkout@v4=sha1-"+parityTestSHA) + + _, stderr, err := runCommandWithHTTP(t, reg, "--no-onboard", "--no-narrow", "--no-interactive", path) + require.NoError(t, err, stderr) + for _, req := range reg.Requests { + assert.NotContains(t, req.URL.Path, "/compare/") + assert.NotContains(t, req.URL.Path, "/commits/v4") + } + }) + } +} diff --git a/cmd/gh-actions-lock/run.go b/cmd/gh-actions-lock/run.go index d105a953..208c5f19 100644 --- a/cmd/gh-actions-lock/run.go +++ b/cmd/gh-actions-lock/run.go @@ -89,7 +89,8 @@ func bindCheckFlags(cmd *cobra.Command, opts *checkOptions) { cmd.Flags().BoolVar(&opts.verify, "verify", false, "Frozen check: run the same checks as the default without writing; fails if the lockfile is incomplete or the runner would reject a pin") cmd.Flags().BoolVar(&opts.verifyLocal, "verify-local", false, "Offline lockfile coverage check: verify every action ref has a lockfile entry.\n"+ - "No network calls, no authentication required — ideal for pre-commit hooks.") + "No network calls, no authentication required — ideal for pre-commit hooks.\n"+ + "It does not check that the entries are trustworthy; use --verify in CI for that.") cmd.Flags().StringVar(&opts.profileDir, "profile", "", "Enable profiling: write trace, CPU profile, and HTTP log to `dir`") cmd.Flags().BoolVar(&opts.noMigrateLocalActions, "no-migrate-local-actions", false, "Do not rewrite same-repo `uses: ./…` actions to the inherently-pinned `uses: $/…` form.\n"+ @@ -295,6 +296,7 @@ func runCheck(cmd *cobra.Command, opts *checkOptions, newResolver resolverFunc) Store: store, Pool: pool, Relock: relock, + Verify: opts.verify, Profile: prof, } // Defer spinner start until actual network work begins. The fast path diff --git a/internal/ghapi/graphql_parity.go b/internal/ghapi/graphql_parity.go index 34f366dc..1bb0f582 100644 --- a/internal/ghapi/graphql_parity.go +++ b/internal/ghapi/graphql_parity.go @@ -21,6 +21,9 @@ type PinCheck struct { SHA string // Tag, when set, must still peel to SHA. Tag string + // Head, when set, is a mutable ref whose current commit is reported as + // HeadOID so the caller can check SHA is still in its history. + Head string } // PinState is the host's view of a PinCheck. Err marks an inconclusive check. @@ -37,7 +40,9 @@ type PinState struct { CommitFound bool // TagOID is the commit Tag peels to; empty when the tag is gone. TagOID string - Err error + // HeadOID is the commit Head peels to; empty when the ref is gone. + HeadOID string + Err error } // pinCheckBatchSize is larger than the action-file batch because each alias @@ -145,6 +150,11 @@ func buildPinCheckQuery(pins []PinCheck) (string, map[string]any) { fmt.Fprintf(&decl, ", $tag%d: String!", i) fmt.Fprintf(&body, " tag: object(expression: $tag%d) { oid }", i) } + if p.Head != "" { + vars[fmt.Sprintf("head%d", i)] = p.Head + "^{commit}" + fmt.Fprintf(&decl, ", $head%d: String!", i) + fmt.Fprintf(&body, " head: object(expression: $head%d) { oid }", i) + } body.WriteString(" }") } body.WriteString(" }") @@ -199,6 +209,9 @@ func parsePinCheckResponse(data map[string]json.RawMessage, pins []PinCheck, gql Tag *struct { OID string `json:"oid"` } `json:"tag"` + Head *struct { + OID string `json:"oid"` + } `json:"head"` } if err := json.Unmarshal(raw, &repo); err != nil { states[i].Err = fmt.Errorf("failed to parse: %w", err) @@ -213,6 +226,9 @@ func parsePinCheckResponse(data map[string]json.RawMessage, pins []PinCheck, gql if repo.Tag != nil { states[i].TagOID = repo.Tag.OID } + if repo.Head != nil { + states[i].HeadOID = repo.Head.OID + } } return states } @@ -236,7 +252,7 @@ func aliasError(gqlErr *api.GraphQLError, alias string) error { if len(item.Path) == 0 || item.Path[0] != alias { continue } - if item.Type == "NOT_FOUND" && len(item.Path) >= 2 && (item.Path[1] == "commit" || item.Path[1] == "tag") { + if item.Type == "NOT_FOUND" && len(item.Path) >= 2 && (item.Path[1] == "commit" || item.Path[1] == "tag" || item.Path[1] == "head") { continue } return errors.New(item.Message) @@ -246,7 +262,7 @@ func aliasError(gqlErr *api.GraphQLError, alias string) error { // checkPinsREST costs one repos/{owner}/{repo} call per unique repo plus one // commits call per intact pin: the tag when one is set (a match proves the -// commit exists), otherwise the SHA. +// commit exists), otherwise the SHA. A Head costs one more commits call. func (c *Client) checkPinsREST(ctx context.Context, pins []PinCheck) []PinState { states := make([]PinState, len(pins)) for i, p := range pins { @@ -283,6 +299,11 @@ func (c *Client) checkPinREST(ctx context.Context, p PinCheck) PinState { if err != nil { return PinState{Err: err} } + if p.Head != "" && s.CommitFound { + if s.HeadOID, _, err = c.restCommit(ctx, p.Owner, p.Repo, p.Head); err != nil { + return PinState{Err: err} + } + } return s } diff --git a/internal/ghapi/repos.go b/internal/ghapi/repos.go index a6550160..32934c91 100644 --- a/internal/ghapi/repos.go +++ b/internal/ghapi/repos.go @@ -427,7 +427,12 @@ func (c *Client) CompareRefs(ctx context.Context, owner, repo, base, head string url.PathEscape(owner), url.PathEscape(repo), url.PathEscape(base), url.PathEscape(head)) var resp compareResponse - if err := c.rest.DoWithContext(ctx, http.MethodGet, path, nil, &resp); err != nil { + if c.restOnly { + err = c.anonGet(ctx, path, &resp) + } else { + err = c.rest.DoWithContext(ctx, http.MethodGet, path, nil, &resp) + } + if err != nil { return "", "", err } return resp.Status, resp.MergeBaseCommit.SHA, nil diff --git a/internal/pipeline/parity.go b/internal/pipeline/parity.go index 36a71258..bf7cbc8c 100644 --- a/internal/pipeline/parity.go +++ b/internal/pipeline/parity.go @@ -1,6 +1,7 @@ package pipeline import ( + "cmp" "context" "fmt" "strings" @@ -17,7 +18,9 @@ import ( // checkParity re-checks every locked pin in each workflow's recorded closure // the way the runner does at job start: same repository identity, commit // still present, exact-version tag still at the commit. Mutable refs that -// moved are not reported; the lock is sticky until --relock. +// moved are not reported; the lock is sticky until --relock. With verify, a +// commit on a mutable ref must still be in that ref's history: a fork-network +// commit resolves under the upstream name, so existence alone proves nothing. // // A pinned commit fixes the dependencies it declares, so the flattened // closure in the lockfile is checked in one batch with no recursion. @@ -32,7 +35,7 @@ import ( // // Under the Dependabot proxy any inconclusive check is an error: the run // aborts so Dependabot retries rather than shipping an unverified lock. -func checkParity(ctx context.Context, r *resolve.Resolver, parsed []checks.ParsedWorkflow, store *lockfile.State, report *checks.Report, recordedKeys map[string]bool, resolved []dep.Dependency, parents map[string][]string, keep func(checks.Category) bool) error { +func checkParity(ctx context.Context, r *resolve.Resolver, parsed []checks.ParsedWorkflow, store *lockfile.State, report *checks.Report, recordedKeys map[string]bool, resolved []dep.Dependency, parents map[string][]string, keep func(checks.Category) bool, verify bool) error { gh := r.GHClient() if gh == nil || store == nil { return nil @@ -59,8 +62,12 @@ func checkParity(ctx context.Context, r *resolve.Resolver, parsed []checks.Parse pinIdx[key] = i pins = append(pins, ghapi.PinCheck{Owner: o.lp.Pin.Owner, Repo: o.lp.Pin.Repo, SHA: lockedSHA(o.lp.Action.Commit)}) } - if !o.fresh && checks.IsImmutableRef(o.lp.Pin.Ref) { + switch { + case o.fresh: + case checks.IsImmutableRef(o.lp.Pin.Ref): pins[i].Tag = o.lp.Pin.Ref + case verify && !parserlock.IsFullSha(o.lp.Pin.Ref): + pins[i].Head = o.lp.Pin.Ref } if j, dup := owned[[2]int{o.report, i}]; dup { owners[i][j].fresh = owners[i][j].fresh || o.fresh @@ -107,6 +114,18 @@ func checkParity(ctx context.Context, r *resolve.Resolver, parsed []checks.Parse if ctx.Err() != nil { return ctx.Err() } + for i := range states { + st, pc := &states[i], pins[i] + if pc.Head == "" || st.Err != nil || !st.CommitFound || st.HeadOID == "" || strings.EqualFold(st.HeadOID, pc.SHA) { + continue + } + switch ancestry, detail := r.CheckAncestry(ctx, pc.Owner, pc.Repo, pc.SHA, st.HeadOID); ancestry { + case resolve.AncestryNotAncestor: + st.CommitFound = false + case resolve.AncestryUnknown: + st.Err = fmt.Errorf("checking %s history: %s", pc.Head, detail) + } + } if ghapi.DependabotProxy() { for i, st := range states { if st.Err != nil { @@ -248,7 +267,7 @@ func parityFinding(pw checks.ParsedWorkflow, ref parserlock.ActionRef, lp lockfi } case !st.CommitFound: f.Category = checks.UnreachablePin - f.Detail = checks.NotInHistoryDetail(pc.SHA, nwo, ref.Ref, st.TagOID) + via + f.Detail = checks.NotInHistoryDetail(pc.SHA, nwo, ref.Ref, cmp.Or(st.TagOID, st.HeadOID)) + via f.Remediation = checks.AcceptMovedRemediation case pc.Tag != "" && !strings.EqualFold(st.TagOID, pc.SHA): f.Category = checks.UnreachablePin diff --git a/internal/pipeline/run.go b/internal/pipeline/run.go index b3144237..8c2c8a7d 100644 --- a/internal/pipeline/run.go +++ b/internal/pipeline/run.go @@ -22,6 +22,9 @@ type RunOptions struct { Pool *pinpool.Pool // Relock re-resolves every ref live instead of trusting the lockfile. Relock bool + // Verify also checks that each pin on a mutable ref is still in that + // ref's history, at one Compare call per ref that moved. + Verify bool // Resolver UX hooks — set these for interactive spinner mode. OnResolveProgress func(done, total int) @@ -134,7 +137,7 @@ func Run(ctx context.Context, opts RunOptions) (*RunResult, error) { } } endParity := prof.Phase(" parity check") - err := checkParity(ctx, r, parsed, opts.Store, report, recordedKeys, resolved, parents, keep) + err := checkParity(ctx, r, parsed, opts.Store, report, recordedKeys, resolved, parents, keep, opts.Verify) endParity() if err != nil { return nil, err From 86ddf09f695232adb8bdc8c586a7fa4b092730af Mon Sep 17 00:00:00 2001 From: Jeff Martin Date: Sun, 11 Oct 2026 14:06:34 -0700 Subject: [PATCH 2/4] Keep refs case-sensitive in the parity dedup key The runner keys refs case-sensitively, so @Main and @main are different refs and each needs its own --verify history check. --- cmd/gh-actions-lock/parity_test.go | 52 ++++++++++++++++++++++++++++++ internal/pipeline/parity.go | 4 ++- 2 files changed, 55 insertions(+), 1 deletion(-) diff --git a/cmd/gh-actions-lock/parity_test.go b/cmd/gh-actions-lock/parity_test.go index a8e28e16..da129430 100644 --- a/cmd/gh-actions-lock/parity_test.go +++ b/cmd/gh-actions-lock/parity_test.go @@ -834,3 +834,55 @@ jobs: }) } } + +// TestParity_VerifyRefCaseSensitive: refs differing only in case are +// distinct refs to the runner, so each gets its own history check. +func TestParity_VerifyRefCaseSensitive(t *testing.T) { + for _, proxy := range []bool{false, true} { + t.Run(map[bool]string{false: "graphql", true: "dependabot proxy"}[proxy], func(t *testing.T) { + reg := &httpmock.Registry{} + var heads []string + if proxy { + t.Setenv("GH_ACTIONS_LOCK_DEPENDABOT_PROXY", "1") + useDefaultTransport(t, reg) + for range 2 { + reg.Register(httpmock.REST("GET", `^/repos/actions/checkout$`), httpmock.JSONResponse(map[string]any{ + "full_name": "actions/checkout", "id": 1, "owner": map[string]any{"id": 1}, + })) + reg.Register(httpmock.REST("GET", `^/repos/actions/checkout/commits/`+parityTestSHA+`$`), httpmock.JSONResponse(map[string]any{"sha": parityTestSHA})) + } + for _, ref := range []string{"Main", "main"} { + reg.Register(httpmock.REST("GET", `^/repos/actions/checkout/commits/`+ref+`$`), func(req *http.Request) (*http.Response, error) { + heads = append(heads, ref) + return httpmock.JSONResponse(map[string]any{"sha": parityTestSHA})(req) + }) + } + } else { + reg.Register(httpmock.GraphQL(`head: object\(expression`), func(req *http.Request) (*http.Response, error) { + body, _ := io.ReadAll(req.Body) + for _, ref := range []string{"Main", "main"} { + if strings.Contains(string(body), `"`+ref+`^{commit}"`) { + heads = append(heads, ref) + } + } + repo := map[string]any{"nameWithOwner": "actions/checkout", "commit": map[string]any{"oid": parityTestSHA}, "head": map[string]any{"oid": parityTestSHA}} + return httpmock.JSONResponse(map[string]any{"data": map[string]any{"a0": repo, "a1": repo}})(req) + }) + } + path := writeTempWorkflow(t, ` +name: ci +on: push +jobs: + test: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@Main + - uses: actions/checkout@main +`, "actions/checkout@Main=sha1-"+parityTestSHA, "actions/checkout@main=sha1-"+parityTestSHA) + + _, stderr, err := runCommandWithHTTP(t, reg, "--verify", path) + require.NoError(t, err, stderr) + assert.ElementsMatch(t, []string{"Main", "main"}, heads) + }) + } +} diff --git a/internal/pipeline/parity.go b/internal/pipeline/parity.go index bf7cbc8c..8de2cdd8 100644 --- a/internal/pipeline/parity.go +++ b/internal/pipeline/parity.go @@ -55,7 +55,9 @@ func checkParity(ctx context.Context, r *resolve.Resolver, parsed []checks.Parse o.lp.Action.RepoID = store.RecordedRepoID(o.ref.Owner, o.ref.Repo) } } - key := strings.ToLower(o.lp.Pin.String() + ":" + lockedSHA(o.lp.Action.Commit)) + // Pin.String folds owner/repo only: refs are case-sensitive, as + // the runner keys them. + key := o.lp.Pin.String() + ":" + strings.ToLower(lockedSHA(o.lp.Action.Commit)) i, seen := pinIdx[key] if !seen { i = len(pins) From 3cc4cf63961c6b6d7b8d687934364fe7c498044b Mon Sep 17 00:00:00 2001 From: Jeff Martin Date: Sun, 11 Oct 2026 14:07:13 -0700 Subject: [PATCH 3/4] Block --verify when a locked mutable ref no longer exists The runner rejects a pin whose ref doesn't resolve, so a missing branch or partial-version ref is unreachable-pin, not a pass. A failed lookup stays on the inconclusive path. --- cmd/gh-actions-lock/command_test.go | 4 ++- cmd/gh-actions-lock/parity_test.go | 47 +++++++++++++++++++++++++++++ internal/pipeline/parity.go | 7 ++++- 3 files changed, 56 insertions(+), 2 deletions(-) diff --git a/cmd/gh-actions-lock/command_test.go b/cmd/gh-actions-lock/command_test.go index d3461867..05d6759f 100644 --- a/cmd/gh-actions-lock/command_test.go +++ b/cmd/gh-actions-lock/command_test.go @@ -209,9 +209,10 @@ const nodeActionYAML = "name: Test Action\nruns:\n using: node20\n" func testRepoResponse(nameWithOwner, oid, actionYAML string) map[string]any { return map[string]any{ "nameWithOwner": nameWithOwner, - // commit and tag answer the parity query from the same fixture. + // commit, tag and head answer the parity query from the same fixture. "commit": map[string]any{"oid": oid}, "tag": map[string]any{"oid": oid}, + "head": map[string]any{"oid": oid}, "object": map[string]any{ "oid": oid, "file": map[string]any{ @@ -248,6 +249,7 @@ func parityOK(req *http.Request) (*http.Response, error) { "nameWithOwner": owner + "/" + body.Variables[fmt.Sprintf("name%d", i)].(string), "commit": oid, "tag": oid, + "head": oid, } } return httpmock.JSONResponse(map[string]any{"data": data})(req) diff --git a/cmd/gh-actions-lock/parity_test.go b/cmd/gh-actions-lock/parity_test.go index da129430..c37018ea 100644 --- a/cmd/gh-actions-lock/parity_test.go +++ b/cmd/gh-actions-lock/parity_test.go @@ -835,6 +835,53 @@ jobs: } } +// TestParity_VerifyMissingMutableRefBlocks: under --verify a mutable ref +// that no longer exists is a definitive answer the runner rejects, so it +// blocks rather than passing on the commit alone. +func TestParity_VerifyMissingMutableRefBlocks(t *testing.T) { + for _, proxy := range []bool{false, true} { + t.Run(map[bool]string{false: "graphql", true: "dependabot proxy"}[proxy], func(t *testing.T) { + reg := &httpmock.Registry{} + if proxy { + t.Setenv("GH_ACTIONS_LOCK_DEPENDABOT_PROXY", "1") + useDefaultTransport(t, reg) + reg.Register(httpmock.REST("GET", `^/repos/actions/checkout$`), httpmock.JSONResponse(map[string]any{ + "full_name": "actions/checkout", "id": 1, "owner": map[string]any{"id": 1}, + })) + reg.Register(httpmock.REST("GET", `^/repos/actions/checkout/commits/`+parityTestSHA+`$`), httpmock.JSONResponse(map[string]any{"sha": parityTestSHA})) + reg.Register(httpmock.REST("GET", `^/repos/actions/checkout/commits/v4$`), httpmock.StatusResponse(http.StatusNotFound)) + } else { + reg.Register(httpmock.GraphQL(`head: object\(expression`), httpmock.JSONResponse(map[string]any{ + "data": map[string]any{"a0": map[string]any{ + "nameWithOwner": "actions/checkout", + "commit": map[string]any{"oid": parityTestSHA}, + "head": nil, + }}, + })) + } + path := writeTempWorkflow(t, ` +name: ci +on: push +jobs: + test: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 +`, "actions/checkout@v4=sha1-"+parityTestSHA) + + stdout, stderr, err := runCommandWithHTTP(t, reg, "--verify", "--json=valid,findings", path) + require.ErrorIs(t, err, errSilent, stderr) + var p parityPayload + require.NoError(t, json.Unmarshal([]byte(stdout), &p), stdout+stderr) + require.Len(t, p.Findings, 1) + assert.Equal(t, "unreachable-pin", p.Findings[0].Category) + assert.Equal(t, "error", p.Findings[0].Severity) + assert.Equal(t, "ref v4 no longer exists in actions/checkout; the runner will reject this pin", p.Findings[0].Detail) + assert.Equal(t, "point `uses:` at a tag or branch that exists, then run `gh actions-lock`", p.Findings[0].Remediation) + }) + } +} + // TestParity_VerifyRefCaseSensitive: refs differing only in case are // distinct refs to the runner, so each gets its own history check. func TestParity_VerifyRefCaseSensitive(t *testing.T) { diff --git a/internal/pipeline/parity.go b/internal/pipeline/parity.go index 8de2cdd8..a68e9d47 100644 --- a/internal/pipeline/parity.go +++ b/internal/pipeline/parity.go @@ -20,7 +20,8 @@ import ( // still present, exact-version tag still at the commit. Mutable refs that // moved are not reported; the lock is sticky until --relock. With verify, a // commit on a mutable ref must still be in that ref's history: a fork-network -// commit resolves under the upstream name, so existence alone proves nothing. +// commit resolves under the upstream name, so existence alone proves nothing, +// and the ref must still exist: the runner rejects a pin whose ref is gone. // // A pinned commit fixes the dependencies it declares, so the flattened // closure in the lockfile is checked in one batch with no recursion. @@ -271,6 +272,10 @@ func parityFinding(pw checks.ParsedWorkflow, ref parserlock.ActionRef, lp lockfi f.Category = checks.UnreachablePin f.Detail = checks.NotInHistoryDetail(pc.SHA, nwo, ref.Ref, cmp.Or(st.TagOID, st.HeadOID)) + via f.Remediation = checks.AcceptMovedRemediation + case pc.Head != "" && st.HeadOID == "": + f.Category = checks.UnreachablePin + f.Detail = fmt.Sprintf("ref %s no longer exists in %s; the runner will reject this pin%s", pc.Head, nwo, via) + f.Remediation = "point `uses:` at a tag or branch that exists, then run `gh actions-lock`" case pc.Tag != "" && !strings.EqualFold(st.TagOID, pc.SHA): f.Category = checks.UnreachablePin f.ObservedSHA = st.TagOID From 3b7dcb1b13729cab8d2ac79f08560488f6671532 Mon Sep 17 00:00:00 2001 From: Jeff Martin Date: Sun, 11 Oct 2026 14:22:49 -0700 Subject: [PATCH 4/4] Title a vanished mutable ref plainly under --verify A locked branch or partial-version ref that no longer exists read as "Pinned commit not in upstream history" with release and hardening links, which describes an impostor commit. It now reads "Ref no longer exists" with only the remediation, and omits doc_url in JSON. The category stays unreachable-pin. --- cmd/gh-actions-lock/format/terminal.go | 14 ++++++--- cmd/gh-actions-lock/parity_test.go | 43 ++++++++++++++++++-------- internal/pipeline/checks/finding.go | 7 +++-- internal/pipeline/parity.go | 5 ++- 4 files changed, 47 insertions(+), 22 deletions(-) diff --git a/cmd/gh-actions-lock/format/terminal.go b/cmd/gh-actions-lock/format/terminal.go index 0e44ab86..a5360ca9 100644 --- a/cmd/gh-actions-lock/format/terminal.go +++ b/cmd/gh-actions-lock/format/terminal.go @@ -148,7 +148,7 @@ func renderTermFindingDetail(out *ui.UI, f checks.Finding, dep string) { } out.TermDetail("%s %s %s", icon, out.TermDim(label), dep) out.TermDetail(" %s", f.Detail) - if f.Category == checks.UnreachablePin && f.Dependency != nil { + if f.Category == checks.UnreachablePin && !f.RefMissing && f.Dependency != nil { owner, repo := f.Dependency.OwnerRepo() if owner != "" { out.TermDetail(" ↳ %s", out.TermDim(DepReleaseURL(f.Dependency.Hostname, owner+"/"+repo, nil))) @@ -174,10 +174,14 @@ func renderTermFindingDetail(out *ui.UI, f checks.Finding, dep string) { } } -// findingLabel is categoryLabel with a terminal-only title for refs that -// don't exist upstream, which share the not-pinned category in JSON. +// findingLabel is categoryLabel with terminal-only titles for refs that +// don't exist upstream, which share the not-pinned and unreachable-pin +// categories in JSON. func findingLabel(f checks.Finding) string { - if f.RefMissing { + switch { + case f.RefMissing && f.Category == checks.UnreachablePin: + return "Ref no longer exists" + case f.RefMissing: return "Ref does not exist" } return categoryLabel(f.Category) @@ -294,7 +298,7 @@ func renderFindingDetail(out *ui.UI, f checks.Finding, dep string) { } out.Detail("%s %s %s", icon, out.Dim(label), dep) out.Detail(" %s", f.Detail) - if f.Category == checks.UnreachablePin && f.Dependency != nil { + if f.Category == checks.UnreachablePin && !f.RefMissing && f.Dependency != nil { owner, repo := f.Dependency.OwnerRepo() if owner != "" { out.Detail(" ↳ %s", out.Dim(DepReleaseURL(f.Dependency.Hostname, owner+"/"+repo, nil))) diff --git a/cmd/gh-actions-lock/parity_test.go b/cmd/gh-actions-lock/parity_test.go index c37018ea..00b38ea2 100644 --- a/cmd/gh-actions-lock/parity_test.go +++ b/cmd/gh-actions-lock/parity_test.go @@ -788,6 +788,7 @@ jobs: assert.Equal(t, "error", p.Findings[0].Severity) assert.Contains(t, p.Findings[0].Detail, "is not in actions/checkout's history") assert.Contains(t, p.Findings[0].Detail, "v4 currently points at cccccccccccc") + assert.NotEmpty(t, p.Findings[0].DocURL, "the impostor keeps its doc link") default: require.NoError(t, err, stderr) require.Len(t, p.Findings, 1) @@ -845,19 +846,24 @@ func TestParity_VerifyMissingMutableRefBlocks(t *testing.T) { if proxy { t.Setenv("GH_ACTIONS_LOCK_DEPENDABOT_PROXY", "1") useDefaultTransport(t, reg) - reg.Register(httpmock.REST("GET", `^/repos/actions/checkout$`), httpmock.JSONResponse(map[string]any{ - "full_name": "actions/checkout", "id": 1, "owner": map[string]any{"id": 1}, - })) - reg.Register(httpmock.REST("GET", `^/repos/actions/checkout/commits/`+parityTestSHA+`$`), httpmock.JSONResponse(map[string]any{"sha": parityTestSHA})) - reg.Register(httpmock.REST("GET", `^/repos/actions/checkout/commits/v4$`), httpmock.StatusResponse(http.StatusNotFound)) - } else { - reg.Register(httpmock.GraphQL(`head: object\(expression`), httpmock.JSONResponse(map[string]any{ - "data": map[string]any{"a0": map[string]any{ - "nameWithOwner": "actions/checkout", - "commit": map[string]any{"oid": parityTestSHA}, - "head": nil, - }}, - })) + } + // One pass for --json, one for the terminal. + for range 2 { + if proxy { + reg.Register(httpmock.REST("GET", `^/repos/actions/checkout$`), httpmock.JSONResponse(map[string]any{ + "full_name": "actions/checkout", "id": 1, "owner": map[string]any{"id": 1}, + })) + reg.Register(httpmock.REST("GET", `^/repos/actions/checkout/commits/`+parityTestSHA+`$`), httpmock.JSONResponse(map[string]any{"sha": parityTestSHA})) + reg.Register(httpmock.REST("GET", `^/repos/actions/checkout/commits/v4$`), httpmock.StatusResponse(http.StatusNotFound)) + } else { + reg.Register(httpmock.GraphQL(`head: object\(expression`), httpmock.JSONResponse(map[string]any{ + "data": map[string]any{"a0": map[string]any{ + "nameWithOwner": "actions/checkout", + "commit": map[string]any{"oid": parityTestSHA}, + "head": nil, + }}, + })) + } } path := writeTempWorkflow(t, ` name: ci @@ -878,6 +884,17 @@ jobs: assert.Equal(t, "error", p.Findings[0].Severity) assert.Equal(t, "ref v4 no longer exists in actions/checkout; the runner will reject this pin", p.Findings[0].Detail) assert.Equal(t, "point `uses:` at a tag or branch that exists, then run `gh actions-lock`", p.Findings[0].Remediation) + assert.Empty(t, p.Findings[0].DocURL, "the hardening guide is unrelated to a missing ref") + + stdout, stderr, err = runCommandWithHTTP(t, reg, "--verify", path) + require.ErrorIs(t, err, errSilent) + term := stdout + stderr + assert.Contains(t, term, "Ref no longer exists actions/checkout@v4") + assert.Contains(t, term, "ref v4 no longer exists in actions/checkout; the runner will reject this pin") + assert.Contains(t, term, "point `uses:` at a tag or branch that exists, then run `gh actions-lock`") + assert.NotContains(t, term, "not in upstream history") + assert.NotContains(t, term, "/releases") + assert.NotContains(t, term, "see:") }) } } diff --git a/internal/pipeline/checks/finding.go b/internal/pipeline/checks/finding.go index d51f1965..5f10e3cf 100644 --- a/internal/pipeline/checks/finding.go +++ b/internal/pipeline/checks/finding.go @@ -47,9 +47,10 @@ type Finding struct { // finding (regardless of outcome). Lets renderers distinguish // "we didn't look" from "we looked and found nothing." RecommendedSearched bool - // RefMissing marks a not-pinned finding whose ref definitively does not - // exist upstream. Terminal output titles it separately; the JSON - // category stays not-pinned. + // RefMissing marks a not-pinned or unreachable-pin finding whose ref + // definitively does not exist upstream. Terminal output titles it + // separately and skips the release and doc links; the JSON category is + // unchanged. RefMissing bool // TrackedAction marks a not-pinned bare SHA whose owner/repo the // workflow already locks at another ref. It is a ref change on a diff --git a/internal/pipeline/parity.go b/internal/pipeline/parity.go index a68e9d47..0abbd432 100644 --- a/internal/pipeline/parity.go +++ b/internal/pipeline/parity.go @@ -276,6 +276,7 @@ func parityFinding(pw checks.ParsedWorkflow, ref parserlock.ActionRef, lp lockfi f.Category = checks.UnreachablePin f.Detail = fmt.Sprintf("ref %s no longer exists in %s; the runner will reject this pin%s", pc.Head, nwo, via) f.Remediation = "point `uses:` at a tag or branch that exists, then run `gh actions-lock`" + f.RefMissing = true case pc.Tag != "" && !strings.EqualFold(st.TagOID, pc.SHA): f.Category = checks.UnreachablePin f.ObservedSHA = st.TagOID @@ -301,7 +302,9 @@ func parityFinding(pw checks.ParsedWorkflow, ref parserlock.ActionRef, lp lockfi default: return checks.Finding{}, false } - f.DocURL = DocURLFor(f.Category) + if !f.RefMissing { + f.DocURL = DocURLFor(f.Category) + } return f, true }