Check mutable-ref pins against ref history under --verify (#156) · github/gh-actions-lock@f0e80cd · GitHub
Skip to content

Commit f0e80cd

Browse files
nodeselectornodeselector
authored
Check mutable-ref pins against ref history under --verify (#156)
* 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. * 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. * 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. * 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.
1 parent d8674de commit f0e80cd

10 files changed

Lines changed: 327 additions & 21 deletions

File tree

‎README.md‎

Lines changed: 5 additions & 0 deletions

‎cmd/gh-actions-lock/command_test.go‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -209,9 +209,10 @@ const nodeActionYAML = "name: Test Action\nruns:\n using: node20\n"
209209
func testRepoResponse(nameWithOwner, oid, actionYAML string) map[string]any {
210210
return map[string]any{
211211
"nameWithOwner": nameWithOwner,
212-
// commit and tag answer the parity query from the same fixture.
212+
// commit, tag and head answer the parity query from the same fixture.
213213
"commit": map[string]any{"oid": oid},
214214
"tag": map[string]any{"oid": oid},
215+
"head": map[string]any{"oid": oid},
215216
"object": map[string]any{
216217
"oid": oid,
217218
"file": map[string]any{
@@ -248,6 +249,7 @@ func parityOK(req *http.Request) (*http.Response, error) {
248249
"nameWithOwner": owner + "/" + body.Variables[fmt.Sprintf("name%d", i)].(string),
249250
"commit": oid,
250251
"tag": oid,
252+
"head": oid,
251253
}
252254
}
253255
return httpmock.JSONResponse(map[string]any{"data": data})(req)

‎cmd/gh-actions-lock/format/terminal.go‎

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -148,7 +148,7 @@ func renderTermFindingDetail(out *ui.UI, f checks.Finding, dep string) {
148148
}
149149
out.TermDetail("%s %s %s", icon, out.TermDim(label), dep)
150150
out.TermDetail(" %s", f.Detail)
151-
if f.Category == checks.UnreachablePin && f.Dependency != nil {
151+
if f.Category == checks.UnreachablePin && !f.RefMissing && f.Dependency != nil {
152152
owner, repo := f.Dependency.OwnerRepo()
153153
if owner != "" {
154154
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) {
174174
}
175175
}
176176

177-
// findingLabel is categoryLabel with a terminal-only title for refs that
178-
// don't exist upstream, which share the not-pinned category in JSON.
177+
// findingLabel is categoryLabel with terminal-only titles for refs that
178+
// don't exist upstream, which share the not-pinned and unreachable-pin
179+
// categories in JSON.
179180
func findingLabel(f checks.Finding) string {
180-
if f.RefMissing {
181+
switch {
182+
case f.RefMissing && f.Category == checks.UnreachablePin:
183+
return "Ref no longer exists"
184+
case f.RefMissing:
181185
return "Ref does not exist"
182186
}
183187
return categoryLabel(f.Category)
@@ -294,7 +298,7 @@ func renderFindingDetail(out *ui.UI, f checks.Finding, dep string) {
294298
}
295299
out.Detail("%s %s %s", icon, out.Dim(label), dep)
296300
out.Detail(" %s", f.Detail)
297-
if f.Category == checks.UnreachablePin && f.Dependency != nil {
301+
if f.Category == checks.UnreachablePin && !f.RefMissing && f.Dependency != nil {
298302
owner, repo := f.Dependency.OwnerRepo()
299303
if owner != "" {
300304
out.Detail(" ↳ %s", out.Dim(DepReleaseURL(f.Dependency.Hostname, owner+"/"+repo, nil)))

‎cmd/gh-actions-lock/parity_test.go‎

Lines changed: 234 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -716,3 +716,237 @@ func TestParity_DependabotProxyFirstRunKeepsRenamedName(t *testing.T) {
716716
assert.Contains(t, lock, "'krzema12/github-actions-typing@v2.2.2':")
717717
assert.NotContains(t, lock, "typesafegithub")
718718
}
719+
720+
// TestParity_VerifyMutableRefHistory: --verify checks that a commit locked
721+
// under a mutable ref is still in that ref's history, since a fork-network
722+
// commit exists under the upstream name. A legitimately moved ref stays
723+
// sticky and silent. Plain runs make no extra calls.
724+
func TestParity_VerifyMutableRefHistory(t *testing.T) {
725+
const live = "cccccccccccccccccccccccccccccccccccccccc"
726+
compareURL := `repos/actions/checkout/compare/` + parityTestSHA + `\.\.\.` + live
727+
tests := []struct {
728+
name string
729+
compare httpmock.Responder
730+
// want is the finding category, "" for a clean pass.
731+
want string
732+
}{
733+
{"impostor blocks", httpmock.JSONResponse(map[string]any{"status": "diverged", "merge_base_commit": map[string]any{"sha": parityTestMoved}}), "unreachable-pin"},
734+
{"moved forward passes", httpmock.JSONResponse(map[string]any{"status": "ahead", "merge_base_commit": map[string]any{"sha": parityTestSHA}}), ""},
735+
{"compare 503 is inconclusive", httpmock.StatusResponse(http.StatusServiceUnavailable), "reachability-unknown"},
736+
}
737+
for _, tt := range tests {
738+
for _, proxy := range []bool{false, true} {
739+
t.Run(tt.name+"/"+map[bool]string{false: "graphql", true: "dependabot proxy"}[proxy], func(t *testing.T) {
740+
reg := &httpmock.Registry{}
741+
if proxy {
742+
t.Setenv("GH_ACTIONS_LOCK_DEPENDABOT_PROXY", "1")
743+
useDefaultTransport(t, reg)
744+
reg.Register(httpmock.REST("GET", `^/repos/actions/checkout$`), httpmock.JSONResponse(map[string]any{
745+
"full_name": "actions/checkout", "id": 1, "owner": map[string]any{"id": 1},
746+
}))
747+
reg.Register(httpmock.REST("GET", `^/repos/actions/checkout/commits/`+parityTestSHA+`$`), httpmock.JSONResponse(map[string]any{"sha": parityTestSHA}))
748+
reg.Register(httpmock.REST("GET", `^/repos/actions/checkout/commits/v4$`), httpmock.JSONResponse(map[string]any{"sha": live}))
749+
} else {
750+
reg.Register(httpmock.GraphQL(`head: object\(expression`), httpmock.JSONResponse(map[string]any{
751+
"data": map[string]any{"a0": map[string]any{
752+
"nameWithOwner": "actions/checkout",
753+
"commit": map[string]any{"oid": parityTestSHA},
754+
"head": map[string]any{"oid": live},
755+
}},
756+
}))
757+
}
758+
for range 10 {
759+
reg.Register(httpmock.REST("GET", compareURL), tt.compare)
760+
}
761+
path := writeTempWorkflow(t, `
762+
name: ci
763+
on: push
764+
jobs:
765+
test:
766+
runs-on: ubuntu-latest
767+
steps:
768+
- uses: actions/checkout@v4
769+
`, "actions/checkout@v4=sha1-"+parityTestSHA)
770+
771+
stdout, stderr, err := runCommandWithHTTP(t, reg, "--verify", "--json=valid,findings", path)
772+
if proxy && tt.want == "reachability-unknown" {
773+
require.Error(t, err, stderr)
774+
assert.NotErrorIs(t, err, errSilent, "inconclusive under the proxy is a retry")
775+
assert.Contains(t, err.Error(), "HTTP 503")
776+
return
777+
}
778+
var p parityPayload
779+
require.NoError(t, json.Unmarshal([]byte(stdout), &p), stdout+stderr)
780+
switch tt.want {
781+
case "":
782+
require.NoError(t, err, stderr)
783+
assert.Empty(t, p.Findings, "a moved mutable ref stays sticky and silent")
784+
case "unreachable-pin":
785+
require.ErrorIs(t, err, errSilent)
786+
require.Len(t, p.Findings, 1)
787+
assert.Equal(t, "unreachable-pin", p.Findings[0].Category)
788+
assert.Equal(t, "error", p.Findings[0].Severity)
789+
assert.Contains(t, p.Findings[0].Detail, "is not in actions/checkout's history")
790+
assert.Contains(t, p.Findings[0].Detail, "v4 currently points at cccccccccccc")
791+
assert.NotEmpty(t, p.Findings[0].DocURL, "the impostor keeps its doc link")
792+
default:
793+
require.NoError(t, err, stderr)
794+
require.Len(t, p.Findings, 1)
795+
assert.Equal(t, tt.want, p.Findings[0].Category)
796+
assert.Equal(t, "warning", p.Findings[0].Severity)
797+
}
798+
})
799+
}
800+
}
801+
802+
for _, proxy := range []bool{false, true} {
803+
t.Run("plain run makes no extra calls/"+map[bool]string{false: "graphql", true: "dependabot proxy"}[proxy], func(t *testing.T) {
804+
reg := &httpmock.Registry{}
805+
if proxy {
806+
t.Setenv("GH_ACTIONS_LOCK_DEPENDABOT_PROXY", "1")
807+
useDefaultTransport(t, reg)
808+
reg.Register(httpmock.REST("GET", `^/repos/actions/checkout$`), httpmock.JSONResponse(map[string]any{
809+
"full_name": "actions/checkout", "id": 1, "owner": map[string]any{"id": 1},
810+
}))
811+
reg.Register(httpmock.REST("GET", `^/repos/actions/checkout/commits/`+parityTestSHA+`$`), httpmock.JSONResponse(map[string]any{"sha": parityTestSHA}))
812+
} else {
813+
reg.Register(httpmock.GraphQL(`head: object\(expression`), func(*http.Request) (*http.Response, error) {
814+
t.Error("a plain run must not resolve mutable refs")
815+
return nil, io.EOF
816+
})
817+
registerParity(reg)
818+
}
819+
path := writeTempWorkflow(t, `
820+
name: ci
821+
on: push
822+
jobs:
823+
test:
824+
runs-on: ubuntu-latest
825+
steps:
826+
- uses: actions/checkout@v4
827+
`, "actions/checkout@v4=sha1-"+parityTestSHA)
828+
829+
_, stderr, err := runCommandWithHTTP(t, reg, "--no-onboard", "--no-narrow", "--no-interactive", path)
830+
require.NoError(t, err, stderr)
831+
for _, req := range reg.Requests {
832+
assert.NotContains(t, req.URL.Path, "/compare/")
833+
assert.NotContains(t, req.URL.Path, "/commits/v4")
834+
}
835+
})
836+
}
837+
}
838+
839+
// TestParity_VerifyMissingMutableRefBlocks: under --verify a mutable ref
840+
// that no longer exists is a definitive answer the runner rejects, so it
841+
// blocks rather than passing on the commit alone.
842+
func TestParity_VerifyMissingMutableRefBlocks(t *testing.T) {
843+
for _, proxy := range []bool{false, true} {
844+
t.Run(map[bool]string{false: "graphql", true: "dependabot proxy"}[proxy], func(t *testing.T) {
845+
reg := &httpmock.Registry{}
846+
if proxy {
847+
t.Setenv("GH_ACTIONS_LOCK_DEPENDABOT_PROXY", "1")
848+
useDefaultTransport(t, reg)
849+
}
850+
// One pass for --json, one for the terminal.
851+
for range 2 {
852+
if proxy {
853+
reg.Register(httpmock.REST("GET", `^/repos/actions/checkout$`), httpmock.JSONResponse(map[string]any{
854+
"full_name": "actions/checkout", "id": 1, "owner": map[string]any{"id": 1},
855+
}))
856+
reg.Register(httpmock.REST("GET", `^/repos/actions/checkout/commits/`+parityTestSHA+`$`), httpmock.JSONResponse(map[string]any{"sha": parityTestSHA}))
857+
reg.Register(httpmock.REST("GET", `^/repos/actions/checkout/commits/v4$`), httpmock.StatusResponse(http.StatusNotFound))
858+
} else {
859+
reg.Register(httpmock.GraphQL(`head: object\(expression`), httpmock.JSONResponse(map[string]any{
860+
"data": map[string]any{"a0": map[string]any{
861+
"nameWithOwner": "actions/checkout",
862+
"commit": map[string]any{"oid": parityTestSHA},
863+
"head": nil,
864+
}},
865+
}))
866+
}
867+
}
868+
path := writeTempWorkflow(t, `
869+
name: ci
870+
on: push
871+
jobs:
872+
test:
873+
runs-on: ubuntu-latest
874+
steps:
875+
- uses: actions/checkout@v4
876+
`, "actions/checkout@v4=sha1-"+parityTestSHA)
877+
878+
stdout, stderr, err := runCommandWithHTTP(t, reg, "--verify", "--json=valid,findings", path)
879+
require.ErrorIs(t, err, errSilent, stderr)
880+
var p parityPayload
881+
require.NoError(t, json.Unmarshal([]byte(stdout), &p), stdout+stderr)
882+
require.Len(t, p.Findings, 1)
883+
assert.Equal(t, "unreachable-pin", p.Findings[0].Category)
884+
assert.Equal(t, "error", p.Findings[0].Severity)
885+
assert.Equal(t, "ref v4 no longer exists in actions/checkout; the runner will reject this pin", p.Findings[0].Detail)
886+
assert.Equal(t, "point `uses:` at a tag or branch that exists, then run `gh actions-lock`", p.Findings[0].Remediation)
887+
assert.Empty(t, p.Findings[0].DocURL, "the hardening guide is unrelated to a missing ref")
888+
889+
stdout, stderr, err = runCommandWithHTTP(t, reg, "--verify", path)
890+
require.ErrorIs(t, err, errSilent)
891+
term := stdout + stderr
892+
assert.Contains(t, term, "Ref no longer exists actions/checkout@v4")
893+
assert.Contains(t, term, "ref v4 no longer exists in actions/checkout; the runner will reject this pin")
894+
assert.Contains(t, term, "point `uses:` at a tag or branch that exists, then run `gh actions-lock`")
895+
assert.NotContains(t, term, "not in upstream history")
896+
assert.NotContains(t, term, "/releases")
897+
assert.NotContains(t, term, "see:")
898+
})
899+
}
900+
}
901+
902+
// TestParity_VerifyRefCaseSensitive: refs differing only in case are
903+
// distinct refs to the runner, so each gets its own history check.
904+
func TestParity_VerifyRefCaseSensitive(t *testing.T) {
905+
for _, proxy := range []bool{false, true} {
906+
t.Run(map[bool]string{false: "graphql", true: "dependabot proxy"}[proxy], func(t *testing.T) {
907+
reg := &httpmock.Registry{}
908+
var heads []string
909+
if proxy {
910+
t.Setenv("GH_ACTIONS_LOCK_DEPENDABOT_PROXY", "1")
911+
useDefaultTransport(t, reg)
912+
for range 2 {
913+
reg.Register(httpmock.REST("GET", `^/repos/actions/checkout$`), httpmock.JSONResponse(map[string]any{
914+
"full_name": "actions/checkout", "id": 1, "owner": map[string]any{"id": 1},
915+
}))
916+
reg.Register(httpmock.REST("GET", `^/repos/actions/checkout/commits/`+parityTestSHA+`$`), httpmock.JSONResponse(map[string]any{"sha": parityTestSHA}))
917+
}
918+
for _, ref := range []string{"Main", "main"} {
919+
reg.Register(httpmock.REST("GET", `^/repos/actions/checkout/commits/`+ref+`$`), func(req *http.Request) (*http.Response, error) {
920+
heads = append(heads, ref)
921+
return httpmock.JSONResponse(map[string]any{"sha": parityTestSHA})(req)
922+
})
923+
}
924+
} else {
925+
reg.Register(httpmock.GraphQL(`head: object\(expression`), func(req *http.Request) (*http.Response, error) {
926+
body, _ := io.ReadAll(req.Body)
927+
for _, ref := range []string{"Main", "main"} {
928+
if strings.Contains(string(body), `"`+ref+`^{commit}"`) {
929+
heads = append(heads, ref)
930+
}
931+
}
932+
repo := map[string]any{"nameWithOwner": "actions/checkout", "commit": map[string]any{"oid": parityTestSHA}, "head": map[string]any{"oid": parityTestSHA}}
933+
return httpmock.JSONResponse(map[string]any{"data": map[string]any{"a0": repo, "a1": repo}})(req)
934+
})
935+
}
936+
path := writeTempWorkflow(t, `
937+
name: ci
938+
on: push
939+
jobs:
940+
test:
941+
runs-on: ubuntu-latest
942+
steps:
943+
- uses: actions/checkout@Main
944+
- uses: actions/checkout@main
945+
`, "actions/checkout@Main=sha1-"+parityTestSHA, "actions/checkout@main=sha1-"+parityTestSHA)
946+
947+
_, stderr, err := runCommandWithHTTP(t, reg, "--verify", path)
948+
require.NoError(t, err, stderr)
949+
assert.ElementsMatch(t, []string{"Main", "main"}, heads)
950+
})
951+
}
952+
}

‎cmd/gh-actions-lock/run.go‎

Lines changed: 3 additions & 1 deletion

0 commit comments

Comments
 (0)