From 8dba5d7f8886e660e9009710bd6170415903c387 Mon Sep 17 00:00:00 2001 From: Jeff Martin Date: Thu, 8 Oct 2026 15:32:03 -0700 Subject: [PATCH 1/4] Handle transferred repository identities across hosts Copilot-Session: d108b66b-d0c6-4437-8b78-3375f5305a26 --- cmd/gh-actions-lock/command_test.go | 541 +++++++++++++++++++- cmd/gh-actions-lock/format/terminal.go | 14 +- cmd/gh-actions-lock/format/terminal_test.go | 12 + cmd/gh-actions-lock/pin_summary.go | 17 + cmd/gh-actions-lock/run.go | 4 - cmd/gh-actions-lock/selfrepository_test.go | 2 +- internal/dep/dependency.go | 33 +- internal/dep/dependency_test.go | 38 ++ internal/ghapi/graphql_action_files.go | 37 +- internal/ghapi/graphql_action_files_test.go | 24 + internal/ghapi/repos.go | 19 +- internal/ghapi/repos_dedup_test.go | 8 +- internal/ghapi/rest_fallback.go | 36 +- internal/ghapi/rest_fallback_test.go | 109 +++- internal/lockfile/direct_tracker.go | 3 + internal/pin/commit.go | 33 ++ internal/pin/plan.go | 95 +++- internal/pin/plan_test.go | 135 ++++- internal/pin/record.go | 5 +- internal/pipeline/checks/category.go | 3 + internal/pipeline/checks/category_test.go | 3 +- internal/pipeline/checks/finding.go | 4 + internal/pipeline/checks/resolver.go | 3 + internal/pipeline/diagnose.go | 30 ++ internal/pipeline/diagnose_test.go | 23 + internal/pipeline/doc_urls.go | 1 + internal/pipeline/run.go | 235 ++++++++- internal/pipeline/run_test.go | 25 + internal/resolve/discovery.go | 28 +- internal/resolve/resolver.go | 5 + internal/workflowfile/rewrite.go | 57 +++ test/integration/run.rb | 25 +- test/scenarios/catalog.yml | 38 +- 33 files changed, 1566 insertions(+), 79 deletions(-) diff --git a/cmd/gh-actions-lock/command_test.go b/cmd/gh-actions-lock/command_test.go index fdeba99d..12f369bf 100644 --- a/cmd/gh-actions-lock/command_test.go +++ b/cmd/gh-actions-lock/command_test.go @@ -12,6 +12,7 @@ import ( parserlock "github.com/github/actions-lockfile/go/pkg/lockfile" "github.com/github/gh-actions-lock/cmd/gh-actions-lock/format" "github.com/github/gh-actions-lock/internal/ghapi/httpmock" + lockstore "github.com/github/gh-actions-lock/internal/lockfile" "github.com/github/gh-actions-lock/internal/pinpool" "github.com/github/gh-actions-lock/internal/resolve" "github.com/stretchr/testify/assert" @@ -85,6 +86,524 @@ jobs: assert.Empty(t, payload.Findings) } +func TestCheckCommand_RewritesMovedRepository(t *testing.T) { + const ( + oldNWO = "krzema12/github-actions-typing" + newNWO = "typesafegithub/github-actions-typing" + ref = "v2.2.2" + sha = "9ddf35b71a482be7d8922b28e8d00df16b77e315" + ) + for _, tt := range []struct { + name string + ref string + pins []string + args []string + }{ + {name: "fresh onboarding", ref: ref}, + { + name: "existing immutable lockfile", + ref: ref, + pins: []string{oldNWO + "@" + ref + "=sha1-" + sha}, + }, + { + name: "existing mutable lockfile", + ref: "v2", + pins: []string{oldNWO + "@v2=sha1-" + sha}, + }, + { + name: "rescan", + ref: ref, + pins: []string{oldNWO + "@" + ref + "=sha1-" + sha}, + args: []string{"--rescan"}, + }, + } { + t.Run(tt.name, func(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + reg.Register( + httpmock.GraphQLForRepo("krzema12", "github-actions-typing"), + httpmock.JSONResponse(map[string]any{ + "data": map[string]any{ + "a0": testRepoResponse(newNWO, sha, nodeActionYAML), + }, + }), + ) + if tt.name == "existing mutable lockfile" { + reg.Register( + httpmock.REST("GET", `repos/krzema12/github-actions-typing$`), + httpmock.JSONResponse(map[string]any{ + "full_name": newNWO, + "id": 502427408, + "owner": map[string]any{"id": 129620060}, + }), + ) + } + reg.Register( + httpmock.REST("GET", `repos/typesafegithub/github-actions-typing$`), + httpmock.JSONResponse(map[string]any{ + "full_name": newNWO, + "id": 502427408, + "owner": map[string]any{"id": 129620060}, + }), + ) + workflowPath := writeTempWorkflow(t, ` +name: ci +on: push +jobs: + test: + runs-on: ubuntu-latest + steps: + - uses: `+oldNWO+`@`+tt.ref+` +`, tt.pins...) + args := append(tt.args, "--no-narrow", workflowPath) + stdout, stderr, err := runCommandWithHTTP(t, reg, args...) + + require.NoError(t, err, "stdout:\n%s", stdout) + assert.Contains(t, stdout+stderr, "Action repository transferred: "+oldNWO+" → "+newNWO) + workflow, readErr := os.ReadFile(workflowPath) + require.NoError(t, readErr) + assert.Contains(t, string(workflow), "uses: "+newNWO+"@"+tt.ref) + assert.NotContains(t, string(workflow), oldNWO) + pins := readTempLockfilePins(t) + assert.Contains(t, pins, "'"+newNWO+"@"+tt.ref+"'") + assert.NotContains(t, pins, oldNWO) + + localReg := &httpmock.Registry{} + _, _, verifyErr := runCommandWithHTTP(t, localReg, "--verify-local", workflowPath) + require.NoError(t, verifyErr) + localReg.Verify(t) + }) + } +} + +func TestCheckCommand_RejectsReplacedRepository(t *testing.T) { + const ( + nwo = "owner/action" + ref = "v1" + sha = "1111111111111111111111111111111111111111" + ) + for _, tt := range []struct { + name string + args []string + }{ + {name: "default"}, + {name: "verify", args: []string{"--verify"}}, + } { + t.Run(tt.name, func(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + reg.Register( + httpmock.REST("GET", `repos/owner/action$`), + httpmock.JSONResponse(map[string]any{ + "full_name": nwo, + "id": 2, + "owner": map[string]any{"id": 1}, + }), + ) + reg.Register( + httpmock.GraphQLForRepo("owner", "action"), + httpmock.JSONResponse(map[string]any{ + "data": map[string]any{ + "a0": testRepoResponse(nwo, sha, nodeActionYAML), + }, + }), + ) + workflowPath := writeTempWorkflow(t, ` +name: ci +on: push +jobs: + test: + runs-on: ubuntu-latest + steps: + - uses: `+nwo+`@`+ref+` +`, nwo+"@"+ref+"=sha1-"+sha) + workflowBefore, err := os.ReadFile(workflowPath) + require.NoError(t, err) + lockPath := filepath.Join(".github", "workflows", "actions.lock") + lockBefore, err := os.ReadFile(lockPath) + require.NoError(t, err) + + args := append(tt.args, "--no-narrow", workflowPath) + stdout, stderr, err := runCommandWithHTTP(t, reg, args...) + + require.Error(t, err) + output := stdout + stderr + err.Error() + assert.Contains(t, output, "repository identity changed for "+nwo+": the lockfile records repository ID 1, but the current repository ID is 2") + assert.Contains(t, output, "This may indicate a namespace takeover") + assert.Contains(t, output, "review "+nwo+" before trusting it") + workflowAfter, readErr := os.ReadFile(workflowPath) + require.NoError(t, readErr) + assert.Equal(t, workflowBefore, workflowAfter) + lockAfter, readErr := os.ReadFile(lockPath) + require.NoError(t, readErr) + assert.Equal(t, lockBefore, lockAfter) + }) + } +} + +func TestCheckCommand_PrefersLiveMovedRepositoryOverSeededAlias(t *testing.T) { + const ( + oldNWO = "old/action" + newNWO = "new/action" + oldSHA = "1111111111111111111111111111111111111111" + liveSHA = "2222222222222222222222222222222222222222" + childSHA = "3333333333333333333333333333333333333333" + ) + for _, tt := range []struct { + name string + refs []string + }{ + {name: "seeded alias first", refs: []string{newNWO + "@v1", oldNWO + "@v1"}}, + {name: "live redirect first", refs: []string{oldNWO + "@v1", newNWO + "@v1"}}, + } { + t.Run(tt.name, func(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + reg.Register( + httpmock.REST("GET", `repos/new/action$`), + httpmock.JSONResponse(map[string]any{ + "full_name": newNWO, + "id": 1, + "owner": map[string]any{"id": 1}, + }), + ) + reg.Register( + httpmock.GraphQLForRepo("old", "action"), + httpmock.JSONResponse(map[string]any{ + "data": map[string]any{ + "a0": testRepoResponse(newNWO, liveSHA, "runs:\n using: composite\n steps:\n - uses: child/action@v1\n"), + }, + }), + ) + reg.Register( + httpmock.GraphQLForRepo("child", "action"), + httpmock.JSONResponse(map[string]any{ + "data": map[string]any{ + "a0": testRepoResponse("child/action", childSHA, nodeActionYAML), + }, + }), + ) + reg.Register( + httpmock.REST("GET", `repos/child/action$`), + httpmock.JSONResponse(map[string]any{ + "full_name": "child/action", + "id": 4, + "owner": map[string]any{"id": 3}, + }), + ) + workflowPath := writeTempWorkflow(t, ` +name: ci +on: push +jobs: + test: + runs-on: ubuntu-latest + steps: + - uses: `+strings.Join(tt.refs, ` + - uses: `)+` +`, newNWO+"@v1=sha1-"+oldSHA) + + stdout, _, err := runCommandWithHTTP(t, reg, "--no-narrow", workflowPath) + + require.NoError(t, err, "stdout:\n%s", stdout) + store, loadErr := lockstore.LoadState(".", nil) + require.NoError(t, loadErr) + file := store.File() + action, ok := file.Dependencies[newNWO+"@v1"] + require.True(t, ok) + assert.Equal(t, "sha1-"+liveSHA, action.Commit) + assert.Equal(t, []string{"child/action@v1"}, action.Uses) + workflow, readErr := os.ReadFile(workflowPath) + require.NoError(t, readErr) + assert.NotContains(t, string(workflow), oldNWO) + }) + } +} + +func TestCheckCommand_VerifyRejectsMovedRepositoryInExistingLockfile(t *testing.T) { + const ( + oldNWO = "krzema12/github-actions-typing" + newNWO = "typesafegithub/github-actions-typing" + ref = "v2.2.2" + sha = "9ddf35b71a482be7d8922b28e8d00df16b77e315" + ) + reg := &httpmock.Registry{} + defer reg.Verify(t) + reg.Register( + httpmock.GraphQLForRepo("krzema12", "github-actions-typing"), + httpmock.JSONResponse(map[string]any{ + "data": map[string]any{ + "a0": testRepoResponse(newNWO, sha, nodeActionYAML), + }, + }), + ) + workflowPath := writeTempWorkflow(t, ` +name: ci +on: push +jobs: + test: + runs-on: ubuntu-latest + steps: + - uses: `+oldNWO+`@`+ref+` +`, oldNWO+"@"+ref+"=sha1-"+sha) + workflowBefore, readErr := os.ReadFile(workflowPath) + require.NoError(t, readErr) + lockPath := filepath.Join(".github", "workflows", "actions.lock") + lockBefore, readErr := os.ReadFile(lockPath) + require.NoError(t, readErr) + + stdout, stderr, err := runCommandWithHTTP(t, reg, "--verify", "--no-narrow", workflowPath) + + require.Error(t, err) + assert.Contains(t, stdout+stderr, "repository "+oldNWO+" has been renamed or transferred to "+newNWO) + workflowAfter, readErr := os.ReadFile(workflowPath) + require.NoError(t, readErr) + assert.Equal(t, workflowBefore, workflowAfter) + lockAfter, readErr := os.ReadFile(lockPath) + require.NoError(t, readErr) + assert.Equal(t, lockBefore, lockAfter) +} + +func TestCheckCommand_VerifyRejectsKnownMoveWhenResolutionFails(t *testing.T) { + const ( + oldNWO = "old/action" + newNWO = "new/action" + ref = "v1" + sha = "1111111111111111111111111111111111111111" + ) + reg := &httpmock.Registry{} + defer reg.Verify(t) + reg.Register( + httpmock.REST("GET", `repos/old/action$`), + httpmock.JSONResponse(map[string]any{"full_name": newNWO}), + ) + reg.Register( + httpmock.GraphQLForRepo("old", "action"), + httpmock.StatusResponse(http.StatusInternalServerError), + ) + workflowPath := writeTempWorkflow(t, ` +name: ci +on: push +jobs: + test: + runs-on: ubuntu-latest + steps: + - uses: `+oldNWO+`@`+ref+` +`, oldNWO+"@"+ref+"=sha1-"+sha) + + stdout, _, err := runCommandWithHTTP(t, reg, "--verify", "--json=valid,findings", workflowPath) + + require.ErrorIs(t, err, errSilent) + var payload struct { + Valid bool `json:"valid"` + Findings []format.Finding `json:"findings"` + } + require.NoError(t, json.Unmarshal([]byte(stdout), &payload)) + assert.False(t, payload.Valid) + require.Len(t, payload.Findings, 2) + assert.Equal(t, "reachability-unknown", payload.Findings[0].Category) + assert.Equal(t, "ref-changed", payload.Findings[1].Category) + assert.Contains(t, payload.Findings[1].Detail, oldNWO+" has been renamed or transferred to "+newNWO) +} + +func TestCheckCommand_FixRejectsKnownMoveWhenResolutionFails(t *testing.T) { + const ( + oldNWO = "old/action" + newNWO = "new/action" + ref = "v1" + sha = "1111111111111111111111111111111111111111" + ) + reg := &httpmock.Registry{} + defer reg.Verify(t) + reg.Register( + httpmock.REST("GET", `repos/old/action$`), + httpmock.JSONResponse(map[string]any{"full_name": newNWO}), + ) + for range 2 { + reg.Register( + httpmock.GraphQLForRepo("old", "action"), + httpmock.StatusResponse(http.StatusInternalServerError), + ) + } + workflowPath := writeTempWorkflow(t, ` +name: ci +on: push +jobs: + test: + runs-on: ubuntu-latest + steps: + - uses: `+oldNWO+`@`+ref+` +`, oldNWO+"@"+ref+"=sha1-"+sha) + workflowBefore, readErr := os.ReadFile(workflowPath) + require.NoError(t, readErr) + lockPath := filepath.Join(".github", "workflows", "actions.lock") + lockBefore, readErr := os.ReadFile(lockPath) + require.NoError(t, readErr) + + stdout, stderr, err := runCommandWithHTTP(t, reg, "--no-narrow", workflowPath) + + require.Error(t, err) + require.ErrorContains(t, err, "resolving transferred repository") + assert.NotContains(t, stdout+stderr, "All workflows valid") + workflowAfter, readErr := os.ReadFile(workflowPath) + require.NoError(t, readErr) + assert.Equal(t, workflowBefore, workflowAfter) + lockAfter, readErr := os.ReadFile(lockPath) + require.NoError(t, readErr) + assert.Equal(t, lockBefore, lockAfter) +} + +func TestCheckCommand_RejectsAnchoredMovedRepositoryRewrite(t *testing.T) { + const ( + oldNWO = "krzema12/github-actions-typing" + newNWO = "typesafegithub/github-actions-typing" + ref = "v2.2.2" + sha = "9ddf35b71a482be7d8922b28e8d00df16b77e315" + ) + reg := &httpmock.Registry{} + defer reg.Verify(t) + reg.Register( + httpmock.GraphQLForRepo("krzema12", "github-actions-typing"), + httpmock.JSONResponse(map[string]any{ + "data": map[string]any{ + "a0": testRepoResponse(newNWO, sha, nodeActionYAML), + }, + }), + ) + workflowPath := writeTempWorkflow(t, ` +name: ci +on: push +jobs: + test: + runs-on: ubuntu-latest + steps: + - &typing + uses: `+oldNWO+`@`+ref+` + - *typing +`) + before, readErr := os.ReadFile(workflowPath) + require.NoError(t, readErr) + + _, _, err := runCommandWithHTTP(t, reg, "--no-narrow", workflowPath) + + require.ErrorContains(t, err, "cannot update an anchored or aliased `uses:` value") + after, readErr := os.ReadFile(workflowPath) + require.NoError(t, readErr) + assert.Equal(t, string(before), string(after)) + _, statErr := os.Stat(filepath.Join(".github", "workflows", "actions.lock")) + assert.ErrorIs(t, statErr, os.ErrNotExist) +} + +func TestCheckCommand_RejectsMovedRepositoryInRemoteComposite(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + reg.Register( + httpmock.GraphQLForRepo("root", "composite"), + httpmock.JSONResponse(map[string]any{ + "data": map[string]any{ + "a0": testRepoResponse("root/composite", strings.Repeat("a", 40), "runs:\n using: composite\n steps:\n - uses: old/action@v1\n"), + }, + }), + ) + reg.Register( + httpmock.GraphQLForRepo("old", "action"), + httpmock.JSONResponse(map[string]any{ + "data": map[string]any{ + "a0": testRepoResponse("new/action", strings.Repeat("b", 40), nodeActionYAML), + }, + }), + ) + workflowPath := writeTempWorkflow(t, ` +name: ci +on: push +jobs: + test: + runs-on: ubuntu-latest + steps: + - uses: root/composite@v1 +`) + + _, _, err := runCommandWithHTTP(t, reg, "--no-narrow", workflowPath) + + var transferred *resolve.TransferredRepositoryError + require.ErrorAs(t, err, &transferred) + assert.Equal(t, "old/action", transferred.Original) + assert.Equal(t, "new/action", transferred.Canonical) + assert.Equal(t, "root/composite@v1", transferred.Parent) + _, statErr := os.Stat(filepath.Join(".github", "workflows", "actions.lock")) + assert.ErrorIs(t, statErr, os.ErrNotExist) +} + +func TestCheckCommand_RejectsTransferredRecordedRemoteCompositeRef(t *testing.T) { + const ( + parentNWO = "root/composite" + oldNWO = "old/action" + newNWO = "new/action" + parentSHA = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + childSHA = "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb" + ) + reg := &httpmock.Registry{} + defer reg.Verify(t) + reg.Register( + httpmock.REST("GET", `repos/root/composite$`), + httpmock.JSONResponse(map[string]any{"full_name": parentNWO}), + ) + reg.Register( + httpmock.REST("GET", `repos/old/action$`), + httpmock.JSONResponse(map[string]any{"full_name": newNWO}), + ) + reg.Register( + httpmock.GraphQLForRepo("root", "composite"), + httpmock.JSONResponse(map[string]any{ + "data": map[string]any{ + "a0": testRepoResponse(parentNWO, parentSHA, "runs:\n using: composite\n steps:\n - uses: "+oldNWO+"@v1\n"), + }, + }), + ) + reg.Register( + httpmock.GraphQLForRepo("old", "action"), + httpmock.JSONResponse(map[string]any{ + "data": map[string]any{ + "a0": testRepoResponse(newNWO, childSHA, nodeActionYAML), + }, + }), + ) + + dir := t.TempDir() + require.NoError(t, os.MkdirAll(filepath.Join(dir, ".github", "workflows"), 0o755)) + workflowPath := filepath.Join(dir, ".github", "workflows", "ci.yml") + workflow := "name: ci\non: push\njobs:\n test:\n runs-on: ubuntu-latest\n steps:\n - uses: " + parentNWO + "@v1\n" + require.NoError(t, os.WriteFile(workflowPath, []byte(workflow), 0o600)) + lockPath := filepath.Join(dir, ".github", "workflows", "actions.lock") + lockYAML := "version: '" + parserlock.Version + "'\ndependencies:\n" + + " '" + parentNWO + "@v1':\n" + + " ref: 'v1'\n commit: 'sha1-" + parentSHA + "'\n owner_id: 1\n repo_id: 1\n" + + " uses:\n - '" + oldNWO + "@v1'\n" + + " '" + oldNWO + "@v1':\n" + + " ref: 'v1'\n commit: 'sha1-" + childSHA + "'\n owner_id: 2\n repo_id: 2\n" + + "workflows:\n '.github/workflows/ci.yml':\n - '" + parentNWO + "@v1'\n" + require.NoError(t, os.WriteFile(lockPath, []byte(lockYAML), 0o600)) + t.Chdir(dir) + workflowArg := ".github/workflows/ci.yml" + + workflowBefore, err := os.ReadFile(workflowPath) + require.NoError(t, err) + lockBefore, err := os.ReadFile(lockPath) + require.NoError(t, err) + + stdout, stderr, err := runCommandWithHTTP(t, reg, "--no-narrow", workflowArg) + + require.Error(t, err) + require.ErrorContains(t, err, oldNWO+" has been renamed or transferred to "+newNWO) + require.ErrorContains(t, err, "upstream composite "+parentNWO+"@v1") + assert.NotContains(t, stdout+stderr, "All workflows valid") + workflowAfter, readErr := os.ReadFile(workflowPath) + require.NoError(t, readErr) + assert.Equal(t, workflowBefore, workflowAfter) + lockAfter, readErr := os.ReadFile(lockPath) + require.NoError(t, readErr) + assert.Equal(t, lockBefore, lockAfter) +} + func TestCheck_BareSHAUsesExactMajorTagAndVerifiesLocally(t *testing.T) { const ( sha = "b6e2e70617bc3265edd6dab6c906732b2f1ae151" @@ -249,10 +768,6 @@ func writeTempWorkflow(t *testing.T, body string, pins ...string) string { return filepath.ToSlash(wfRel) } -// writeTempLockfile writes a v0.0.1 actions.lock fixture covering the given -// workflow file. Owner/repo IDs are stubbed; the read path doesn't validate -// them. All user-supplied scalars are single-quoted to mirror the -// production emitter (see internal/lockfile/store.go::marshalDeterministic). // writeTempLockfile writes a minimal lockfile for the given pins. Each // pinString has the form "owner/repo@ref" (the v0.0.2 pin key format). // A synthetic commit hash is generated from the pin key for each entry. @@ -787,9 +1302,9 @@ jobs: // TestCheck_SeedFromLockfile_SkipsHTTPForCachedDeps verifies that // SeedFromLockfile pre-warms the resolution cache so known deps skip -// network calls, while new deps still resolve from the network. +// action-file resolution, while new deps still resolve from the network. // The workflow has two deps: checkout (in lockfile) and setup-go (not in -// lockfile). Only setup-go should hit the HTTP mock. +// lockfile). Checkout needs only one repository identity request. func TestCheck_SeedFromLockfile_SkipsHTTPForCachedDeps(t *testing.T) { reg := &httpmock.Registry{} defer reg.Verify(t) @@ -797,8 +1312,16 @@ func TestCheck_SeedFromLockfile_SkipsHTTPForCachedDeps(t *testing.T) { checkoutSHA := "de0fac2e4500dabe0009e67214ff5f5447ce83dd" setupGoSHA := "4a3601121dd01d1626a1e23e37211e3254c1c06c" - // Only register an HTTP stub for setup-go (the NEW dep). - // No stub for checkout — the seed must serve it from cache. + 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}, + }), + ) + // No GraphQL stub for checkout: after its NWO is validated, the seed + // must still serve its action resolution from cache. reg.Register( httpmock.GraphQLForRepo("actions", "setup-go"), httpmock.JSONResponse(map[string]any{ @@ -851,7 +1374,7 @@ jobs: require.NoError(t, json.Unmarshal([]byte(stdout), &payload)) // The finding should be about setup-go being unpinned, NOT about checkout. - // If checkout required an HTTP call, reg.Verify would fail (no stub registered). + // If checkout required GraphQL action resolution, no stub would match. require.Len(t, payload.Findings, 1) assert.Equal(t, "not-pinned", payload.Findings[0].Category) assert.Contains(t, payload.Findings[0].Dependency, "setup-go") diff --git a/cmd/gh-actions-lock/format/terminal.go b/cmd/gh-actions-lock/format/terminal.go index c87d7da2..039dbf4a 100644 --- a/cmd/gh-actions-lock/format/terminal.go +++ b/cmd/gh-actions-lock/format/terminal.go @@ -121,7 +121,7 @@ func PresentReadOnlyFailures(out *ui.UI, report *checks.Report) (hasFixable bool g := groups[key] out.TermBlank() for _, f := range g.findings { - if IsAutoFixable(f.Category) { + if IsAutoFixable(f) { hasFixable = true } renderTermFindingDetail(out, f, key) @@ -184,6 +184,8 @@ func categoryLabel(c checks.Category) string { return "Misleading SHA" case checks.UnreachablePin: return "Unreachable pin" + case checks.RepositoryChanged: + return "Repository identity changed" case checks.Stale: return "Unused lockfile entry" } @@ -250,6 +252,7 @@ func renderErrorFindings(out *ui.UI, report *checks.Report, failedCount, checked parts := []string{} for _, cat := range []checks.Category{ checks.UnreachablePin, + checks.RepositoryChanged, checks.RefChanged, checks.NotPinned, checks.OnboardingRequired, checks.LocalAction, checks.InvalidSelfRepositoryRef, checks.Stale, checks.MisleadingSHA, @@ -424,7 +427,7 @@ func renderWarnings(out *ui.UI, report *checks.Report, willRemediate bool) { // remediator should not re-print it in non-interactive mode). func IsAlertedCategory(c checks.Category) bool { switch c { - case checks.UnreachablePin, checks.MisleadingSHA, checks.OnboardingRequired: + case checks.UnreachablePin, checks.MisleadingSHA, checks.RepositoryChanged, checks.OnboardingRequired: return true } return false @@ -436,8 +439,11 @@ func IsAlertedCategory(c checks.Category) bool { // (unreachable-pin, misleading-sha) need investigation or --accept-moved, and // local-path actions aren't supported at all — so none of those should // trigger the "Re-run without --no-fix to apply fixes" hint. -func IsAutoFixable(c checks.Category) bool { - switch c { +func IsAutoFixable(f checks.Finding) bool { + if f.Category == checks.RefChanged && f.ParentNWO != "" { + return false + } + switch f.Category { case checks.NotPinned, checks.RefChanged, checks.Stale: return true } diff --git a/cmd/gh-actions-lock/format/terminal_test.go b/cmd/gh-actions-lock/format/terminal_test.go index c31082f1..a125d716 100644 --- a/cmd/gh-actions-lock/format/terminal_test.go +++ b/cmd/gh-actions-lock/format/terminal_test.go @@ -609,6 +609,18 @@ func TestPresentReadOnlyFailures_FixableReported(t *testing.T) { } } +func TestTransferredRepositoryAutoFixableOnlyWhenWritable(t *testing.T) { + direct := checks.Finding{Category: checks.RefChanged} + remote := checks.Finding{Category: checks.RefChanged, ParentNWO: "root/composite@v2"} + + if !IsAutoFixable(direct) { + t.Fatal("direct transfer should be auto-fixable") + } + if IsAutoFixable(remote) { + t.Fatal("remote transfer should not be auto-fixable") + } +} + // TestPresentReadOnlyFailures_ValidReportSilent verifies a clean report // produces no output and reports nothing fixable. func TestPresentReadOnlyFailures_ValidReportSilent(t *testing.T) { diff --git a/cmd/gh-actions-lock/pin_summary.go b/cmd/gh-actions-lock/pin_summary.go index de569230..b97133b1 100644 --- a/cmd/gh-actions-lock/pin_summary.go +++ b/cmd/gh-actions-lock/pin_summary.go @@ -70,6 +70,8 @@ func renderPinSummary(ctx context.Context, console *ui.UI, record *pin.Record, r investigated := record.Investigated() narrowed := record.Narrowed() + renderTransferredRepositories(console, report) + if len(pinned) > 0 { console.TermBlank() renderPinnedEntries(console, pinned) @@ -164,6 +166,21 @@ func renderPinSummary(ctx context.Context, console *ui.UI, record *pin.Record, r return nil } +func renderTransferredRepositories(console *ui.UI, report *checks.Report) { + seen := map[string]bool{} + for _, wr := range report.Workflows { + for _, d := range wr.ResolvedDeps { + for _, ref := range d.OriginalRefs { + change := ref.NWO() + " → " + d.NWO + if !seen[change] { + seen[change] = true + console.TermSuccess("Action repository transferred: %s", change) + } + } + } + } +} + // renderCooldownFindings surfaces the fresh-tag nudge on the terminal in fix // mode, so it shows even on a clean pin where PresentResults renders nothing. // Cooldown-ignored notices are surfaced earlier by PresentResults (both modes). diff --git a/cmd/gh-actions-lock/run.go b/cmd/gh-actions-lock/run.go index b7eb42c4..38ba6b50 100644 --- a/cmd/gh-actions-lock/run.go +++ b/cmd/gh-actions-lock/run.go @@ -227,10 +227,6 @@ func runCheck(cmd *cobra.Command, opts *checkOptions, newResolver resolverFunc) if opts.acceptMoved || opts.relock { opts.rescan = true } - trustLockfileCaches := !opts.rescan - if trustLockfileCaches { - r.SeedFromLockfile(store.AllDeps()) - } endSetup() opts.workflowPaths = paths diff --git a/cmd/gh-actions-lock/selfrepository_test.go b/cmd/gh-actions-lock/selfrepository_test.go index 0bb8c2cd..506b5b73 100644 --- a/cmd/gh-actions-lock/selfrepository_test.go +++ b/cmd/gh-actions-lock/selfrepository_test.go @@ -181,7 +181,7 @@ dependencies: _, _, err = runCommandWithHTTP(t, transport) require.NoError(t, err) - assert.Zero(t, transport.calls.Load()) + assert.Positive(t, transport.calls.Load(), "recorded repositories must still revalidate identity") action, err = os.ReadFile(actionPath) require.NoError(t, err) diff --git a/internal/dep/dependency.go b/internal/dep/dependency.go index 199fba9a..33a47c1e 100644 --- a/internal/dep/dependency.go +++ b/internal/dep/dependency.go @@ -15,8 +15,9 @@ import ( // resolver traversal, and lockfile serialization — never persisted on disk // and not part of any public API. type Dependency struct { - Hostname string // owning GitHub instance; empty means github.com - NWO string // owner/repo (no path) + Hostname string // owning GitHub instance; empty means github.com + NWO string // owner/repo (no path) + OriginalRefs []parserlock.ActionRef // Path is the optional sub-action subpath as written in `uses:` // (e.g. "save" for actions/cache/save). It is preserved on the // in-memory dep so resolver-time graph traversal can fetch the @@ -78,14 +79,32 @@ func detectHashAlgo(hash string) string { return "sha1" } -// Dedup returns a copy of deps with duplicates (by Key) removed, -// preserving first-seen order. +// Dedup returns a copy of deps with duplicates (by Key) removed, preserving +// first-seen order. A redirected result replaces a seeded result because it +// carries the live repository identity, SHA, and action metadata. func Dedup(deps []Dependency) []Dependency { - seen := make(map[string]bool, len(deps)) + seen := make(map[string]int, len(deps)) out := make([]Dependency, 0, len(deps)) for _, d := range deps { - if k := d.Key(); !seen[k] { - seen[k] = true + k := d.Key() + if idx, ok := seen[k]; ok { + if len(out[idx].OriginalRefs) == 0 && len(d.OriginalRefs) > 0 { + out[idx] = d + continue + } + have := make(map[string]bool, len(out[idx].OriginalRefs)) + for _, ref := range out[idx].OriginalRefs { + have[ref.FullName()+"@"+ref.Ref] = true + } + for _, ref := range d.OriginalRefs { + refKey := ref.FullName() + "@" + ref.Ref + if !have[refKey] { + out[idx].OriginalRefs = append(out[idx].OriginalRefs, ref) + have[refKey] = true + } + } + } else { + seen[k] = len(out) out = append(out, d) } } diff --git a/internal/dep/dependency_test.go b/internal/dep/dependency_test.go index 52421832..609f31cc 100644 --- a/internal/dep/dependency_test.go +++ b/internal/dep/dependency_test.go @@ -3,7 +3,9 @@ package dep import ( "testing" + parserlock "github.com/github/actions-lockfile/go/pkg/lockfile" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestDependencyStringRoundTrip(t *testing.T) { @@ -54,3 +56,39 @@ func TestDependencyKey(t *testing.T) { d := Dependency{NWO: "actions/checkout", Ref: "v4", SHA: "abc"} assert.Equal(t, "actions/checkout@v4", d.Key()) } + +func TestDedupMergesTransferredSources(t *testing.T) { + live := Dependency{ + NWO: "new/action", + Ref: "v1", + SHA: "live", + Path: "live-path", + OriginalRefs: []parserlock.ActionRef{ + {Owner: "old", Repo: "action", Ref: "v1"}, + }, + } + seeded := Dependency{NWO: "new/action", Ref: "v1", SHA: "seeded"} + for _, tt := range []struct { + name string + deps []Dependency + }{ + { + name: "seeded before live redirect", + deps: []Dependency{seeded, live}, + }, + { + name: "live redirect before seeded", + deps: []Dependency{live, seeded}, + }, + } { + t.Run(tt.name, func(t *testing.T) { + got := Dedup(tt.deps) + + require.Len(t, got, 1) + assert.Equal(t, "live", got[0].SHA) + assert.Equal(t, "live-path", got[0].Path) + require.Len(t, got[0].OriginalRefs, 1) + assert.Equal(t, "old/action", got[0].OriginalRefs[0].NWO()) + }) + } +} diff --git a/internal/ghapi/graphql_action_files.go b/internal/ghapi/graphql_action_files.go index 37ff1a2f..a28e5fde 100644 --- a/internal/ghapi/graphql_action_files.go +++ b/internal/ghapi/graphql_action_files.go @@ -27,14 +27,15 @@ func (r ActionFileRequest) NWO() string { return r.Owner + "/" + r.Repo } // for one ActionFileRequest. Err is non-nil when this specific ref could // not be resolved (e.g. not found, SSO required). type ActionFileResult struct { - Hostname string - Owner string - Repo string - Path string - Ref string - CommitOID string - ActionYML string - Err error + Hostname string + Owner string + Repo string + OriginalNWO string + Path string + Ref string + CommitOID string + ActionYML string + Err error } // repoResponse is the raw GraphQL response shape for a single repository alias. @@ -298,6 +299,12 @@ func parseActionFileResponse(data map[string]json.RawMessage, refs []ActionFileR results[idx].Err = fmt.Errorf("failed to parse: %w", err) continue } + if repo.NameWithOwner != "" { + if err := canonicalizeActionFileResult(&results[idx], ref, repo.NameWithOwner); err != nil { + results[idx].Err = err + continue + } + } if repo.Object == nil || repo.Object.OID == "" { n := len(ref.Ref) @@ -327,6 +334,20 @@ func parseActionFileResponse(data map[string]json.RawMessage, refs []ActionFileR return results } +func canonicalizeActionFileResult(result *ActionFileResult, ref ActionFileRequest, canonical string) error { + owner, name, ok := strings.Cut(canonical, "/") + if !ok || owner == "" || name == "" { + return fmt.Errorf("invalid canonical repository name %q", canonical) + } + if strings.EqualFold(canonical, ref.NWO()) { + return nil + } + result.OriginalNWO = ref.NWO() + result.Owner = owner + result.Repo = name + return nil +} + // samlBlockedOwners returns the set of repository owners whose resolution // failed an organization SAML SSO enforcement check. func samlBlockedOwners(gqlErr *api.GraphQLError, refs []ActionFileRequest, aliasMap map[string]int) map[string]bool { diff --git a/internal/ghapi/graphql_action_files_test.go b/internal/ghapi/graphql_action_files_test.go index 9d1fd47b..27f9e630 100644 --- a/internal/ghapi/graphql_action_files_test.go +++ b/internal/ghapi/graphql_action_files_test.go @@ -182,6 +182,30 @@ func TestParseActionFileResponse_AnnotatedTagPeeled(t *testing.T) { } } +func TestParseActionFileResponse_CanonicalizesMovedRepository(t *testing.T) { + refs := []ActionFileRequest{{ + Owner: "krzema12", Repo: "github-actions-typing", Ref: "v2.2.2", + }} + data := map[string]json.RawMessage{ + "a0": json.RawMessage(`{"nameWithOwner":"typesafegithub/github-actions-typing","object":{"oid":"9ddf35b71a482be7d8922b28e8d00df16b77e315"}}`), + } + + results := parseActionFileResponse(data, refs, map[string]int{"a0": 0}, nil, "") + + if results[0].Err != nil { + t.Fatalf("unexpected error: %v", results[0].Err) + } + if results[0].OriginalNWO != "krzema12/github-actions-typing" { + t.Fatalf("original repository = %q", results[0].OriginalNWO) + } + if got := results[0].Owner + "/" + results[0].Repo; got != "typesafegithub/github-actions-typing" { + t.Fatalf("canonical repository = %q", got) + } + if results[0].CommitOID != "9ddf35b71a482be7d8922b28e8d00df16b77e315" { + t.Fatalf("commit = %q", results[0].CommitOID) + } +} + func TestParseActionFileResponse_Errors(t *testing.T) { refs := []ActionFileRequest{ {Owner: "actions", Repo: "checkout", Ref: "v6"}, diff --git a/internal/ghapi/repos.go b/internal/ghapi/repos.go index f6703758..fa5b9e6b 100644 --- a/internal/ghapi/repos.go +++ b/internal/ghapi/repos.go @@ -134,6 +134,7 @@ func (c *Client) ListTags(ctx context.Context, owner, repo string) ([]TagEntry, // branch, the numeric owner and repo IDs (lockfile write), and the visibility // and last-push time (tag freshness/immutability checks). type repoMeta struct { + NameWithOwner string DefaultBranch string OwnerID int64 RepoID int64 @@ -142,9 +143,10 @@ type repoMeta struct { } // repoMetadata fetches repos/{owner}/{repo} at most once per run, coalescing -// concurrent callers via singleflight and caching the result. GetDefaultBranch, -// RepoIDs, and RepoMetadata all derive from it, so a repo costs one round-trip -// instead of one per consumer. The request runs under a cancel-free context: +// concurrent callers via singleflight and caching the result. CanonicalNWO, +// GetDefaultBranch, RepoIDs, and RepoMetadata all derive from it, so a repo +// costs one round-trip instead of one per consumer. The request runs under a +// cancel-free context: // callers fan out under scan/errgroup contexts that cancel on first // match/error, and a coalesced caller's cancellation must not abort the shared // fetch for the others waiting on it. @@ -162,6 +164,7 @@ func (c *Client) repoMetadata(ctx context.Context, owner, repo string) (repoMeta return m, nil } var resp struct { + FullName string `json:"full_name"` DefaultBranch string `json:"default_branch"` Visibility string `json:"visibility"` PushedAt string `json:"pushed_at"` @@ -181,6 +184,7 @@ func (c *Client) repoMetadata(ctx context.Context, owner, repo string) (repoMeta } } m := repoMeta{ + NameWithOwner: resp.FullName, DefaultBranch: resp.DefaultBranch, OwnerID: resp.Owner.ID, RepoID: resp.ID, @@ -196,6 +200,15 @@ func (c *Client) repoMetadata(ctx context.Context, owner, repo string) (repoMeta return v.(repoMeta), nil } +// CanonicalNWO returns the repository's current owner/name. +func (c *Client) CanonicalNWO(ctx context.Context, owner, repo string) (string, error) { + m, err := c.repoMetadata(ctx, owner, repo) + if err != nil { + return "", err + } + return m.NameWithOwner, nil +} + // GetDefaultBranch returns the repo's default branch name (e.g. "main"), or // "" if the lookup fails. Backed by the shared repoMetadata fetch. func (c *Client) GetDefaultBranch(ctx context.Context, owner, repo string) string { diff --git a/internal/ghapi/repos_dedup_test.go b/internal/ghapi/repos_dedup_test.go index 2d11f807..ab946340 100644 --- a/internal/ghapi/repos_dedup_test.go +++ b/internal/ghapi/repos_dedup_test.go @@ -60,6 +60,7 @@ func (t *countingTransport) RoundTrip(req *http.Request) (*http.Response, error) })(req) default: // repos/{owner}/{repo} return httpmock.JSONResponse(map[string]any{ + "full_name": "o/r", "default_branch": "main", "id": int64(20), "owner": map[string]any{"id": int64(10)}, @@ -141,8 +142,8 @@ func TestRepoIDs_CoalescesConcurrent(t *testing.T) { } } -// RepoIDs and GetDefaultBranch both derive from repos/{owner}/{repo}; they -// must share a single round-trip rather than fetching it twice. +// Repository metadata consumers must share a single round-trip rather than +// fetching it once per field. func TestRepoMetadata_SharedAcrossConsumers(t *testing.T) { tr := newCountingTransport(2 * time.Millisecond) c := newCountingClient(t, tr) @@ -154,6 +155,9 @@ func TestRepoMetadata_SharedAcrossConsumers(t *testing.T) { if owner, repo, err := c.RepoIDs(context.Background(), "o", "r"); err != nil || owner != 10 || repo != 20 { t.Errorf("RepoIDs = (%d, %d, %v)", owner, repo, err) } + if nwo, err := c.CanonicalNWO(context.Background(), "o", "r"); err != nil || nwo != "o/r" { + t.Errorf("CanonicalNWO = (%q, %v)", nwo, err) + } }) if n := tr.count("repos/o/r"); n != 1 { diff --git a/internal/ghapi/rest_fallback.go b/internal/ghapi/rest_fallback.go index f2c8ee02..a5ffc686 100644 --- a/internal/ghapi/rest_fallback.go +++ b/internal/ghapi/rest_fallback.go @@ -261,13 +261,41 @@ func (c *Client) resolveAnonymous(ctx context.Context, ref ActionFileRequest) Ac Ref: ref.Ref, } + canonical := "" + if metadata, ok := c.repoMetaCache.Get(ForRepo(ref.Owner, ref.Repo)); ok { + canonical = metadata.NameWithOwner + } else if c.restOnly { + var metadata struct { + FullName string `json:"full_name"` + } + path := fmt.Sprintf("repos/%s/%s", url.PathEscape(ref.Owner), url.PathEscape(ref.Repo)) + if err := c.anonGet(ctx, path, &metadata); err != nil { + result.Err = fmt.Errorf("anonymous fallback: %w", err) + return result + } + canonical = metadata.FullName + } else { + metadata, err := c.repoMetadata(ctx, ref.Owner, ref.Repo) + if err != nil { + result.Err = fmt.Errorf("anonymous fallback: %w", err) + return result + } + canonical = metadata.NameWithOwner + } + if canonical != "" { + if err := canonicalizeActionFileResult(&result, ref, canonical); err != nil { + result.Err = err + return result + } + } + base := c.anonBase() // Resolve ref → commit SHA via the commits endpoint. commitURL := fmt.Sprintf("%s/repos/%s/%s/commits/%s", base, - url.PathEscape(ref.Owner), - url.PathEscape(ref.Repo), + url.PathEscape(result.Owner), + url.PathEscape(result.Repo), url.PathEscape(ref.Ref), ) sha, err := c.anonGetCommitSHA(ctx, commitURL) @@ -285,14 +313,14 @@ func (c *Client) resolveAnonymous(ctx context.Context, ref ActionFileRequest) Ac yamlPath = ref.Path + "/action.yaml" } - content, err := c.anonGetFileContent(ctx, base, ref.Owner, ref.Repo, sha, ymlPath) + content, err := c.anonGetFileContent(ctx, base, result.Owner, result.Repo, sha, ymlPath) if err != nil { if code, _ := StatusCode(err); code != http.StatusNotFound { result.Err = err return result } // Try .yaml extension. - content, err = c.anonGetFileContent(ctx, base, ref.Owner, ref.Repo, sha, yamlPath) + content, err = c.anonGetFileContent(ctx, base, result.Owner, result.Repo, sha, yamlPath) if err != nil { // Reusable workflows have no action metadata; other failures // must not silently truncate a composite's dependency graph. diff --git a/internal/ghapi/rest_fallback_test.go b/internal/ghapi/rest_fallback_test.go index 36926338..e58b93bd 100644 --- a/internal/ghapi/rest_fallback_test.go +++ b/internal/ghapi/rest_fallback_test.go @@ -81,7 +81,10 @@ func TestResolveActionFiles_SSOFallbackForActionsOrg(t *testing.T) { // GraphQL transport returns SAML error for actions/checkout. tr := roundTripFunc(func(req *http.Request) (*http.Response, error) { if req.Method == http.MethodGet { - return jsonHTTP(map[string]any{"visibility": "public"}) + return jsonHTTP(map[string]any{ + "full_name": "actions/checkout", + "visibility": "public", + }) } return jsonHTTP(map[string]any{ "data": map[string]any{"a0": nil}, @@ -177,6 +180,10 @@ func TestResolveActionFiles_RESTOnlyUsesPrivateRepo(t *testing.T) { t.Errorf("fallback request method = %s, want GET", r.Method) } switch { + case r.URL.Path == "/repos/actions/checkout": + json.NewEncoder(w).Encode(map[string]string{"full_name": "actions/checkout"}) + case r.URL.Path == "/repos/actions/setup-go": + json.NewEncoder(w).Encode(map[string]string{"full_name": "actions/setup-go"}) case strings.Contains(r.URL.Path, "/commits/"): json.NewEncoder(w).Encode(map[string]string{"sha": "abc123def456abc123def456abc123def456abc1"}) case strings.Contains(r.URL.Path, "/contents/"): @@ -215,6 +222,101 @@ func TestResolveActionFiles_RESTOnlyUsesPrivateRepo(t *testing.T) { } } +func TestResolveActionFiles_RESTFallbackCanonicalizesMovedRepository(t *testing.T) { + const ( + oldNWO = "old/action" + newNWO = "new/action" + sha = "abc123def456abc123def456abc123def456abc1" + ) + assertCanonical := func(t *testing.T, result ActionFileResult) { + t.Helper() + if result.Err != nil { + t.Fatalf("resolution failed: %v", result.Err) + } + if result.OriginalNWO != oldNWO { + t.Fatalf("original repository = %q, want %q", result.OriginalNWO, oldNWO) + } + if got := result.Owner + "/" + result.Repo; got != newNWO { + t.Fatalf("canonical repository = %q, want %q", got, newNWO) + } + if result.CommitOID != sha { + t.Fatalf("commit = %q, want %q", result.CommitOID, sha) + } + } + + t.Run("REST only", func(t *testing.T) { + t.Setenv("GH_ACTIONS_LOCK_DEPENDABOT_PROXY", "1") + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case r.URL.Path == "/repos/old/action": + json.NewEncoder(w).Encode(map[string]string{"full_name": newNWO}) + case strings.HasPrefix(r.URL.Path, "/repos/new/action/commits/"): + json.NewEncoder(w).Encode(map[string]string{"sha": sha}) + case strings.HasPrefix(r.URL.Path, "/repos/new/action/contents/"): + fmt.Fprint(w, "name: moved action") + default: + http.NotFound(w, r) + } + })) + defer srv.Close() + + c, err := New("github.com", WithClientTransport(roundTripFunc(func(req *http.Request) (*http.Response, error) { + t.Fatalf("REST-only mode used authenticated transport: %s %s", req.Method, req.URL) + return nil, nil + }))) + if err != nil { + t.Fatal(err) + } + c.anonBaseURL = srv.URL + + results := c.ResolveActionFiles(context.Background(), []ActionFileRequest{{ + Owner: "old", Repo: "action", Ref: "v1", + }}) + assertCanonical(t, results[0]) + }) + + t.Run("SSO fallback", func(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case strings.HasPrefix(r.URL.Path, "/repos/new/action/commits/"): + json.NewEncoder(w).Encode(map[string]string{"sha": sha}) + case strings.HasPrefix(r.URL.Path, "/repos/new/action/contents/"): + fmt.Fprint(w, "name: moved action") + default: + http.NotFound(w, r) + } + })) + defer srv.Close() + + tr := roundTripFunc(func(req *http.Request) (*http.Response, error) { + if req.Method == http.MethodGet { + return jsonHTTP(map[string]any{ + "full_name": newNWO, + "visibility": "public", + }) + } + return jsonHTTP(map[string]any{ + "data": map[string]any{"a0": nil}, + "errors": []map[string]any{{ + "message": "Resource protected by organization SAML enforcement.", + "path": []any{"a0"}, + "extensions": map[string]any{"saml_failure": true}, + }}, + }) + }) + c, err := New("github.com", WithClientTransport(tr)) + if err != nil { + t.Fatal(err) + } + c.anonBaseURL = srv.URL + + results := c.ResolveActionFiles(context.Background(), []ActionFileRequest{{ + Owner: "old", Repo: "action", Ref: "v1", + }}) + assertCanonical(t, results[0]) + }) +} + func TestResolveActionFiles_BadCredentialsFallbackFailsClosed(t *testing.T) { tests := []struct { name string @@ -237,7 +339,10 @@ func TestResolveActionFiles_BadCredentialsFallbackFailsClosed(t *testing.T) { if tt.repoStatus != http.StatusOK { return statusResponse(req, tt.repoStatus) } - return jsonHTTP(map[string]any{"visibility": "private"}) + return jsonHTTP(map[string]any{ + "full_name": "example/action", + "visibility": "private", + }) } return badCredentialsResponse(req) }) diff --git a/internal/lockfile/direct_tracker.go b/internal/lockfile/direct_tracker.go index 29661cf7..6333b533 100644 --- a/internal/lockfile/direct_tracker.go +++ b/internal/lockfile/direct_tracker.go @@ -34,6 +34,9 @@ func NewDirectTracker(refs []parserlock.ActionRef, deps []dep.Dependency) Direct direct := make([]bool, len(deps)) for i, d := range deps { direct[i] = want[d.Key()] + for _, ref := range d.OriginalRefs { + direct[i] = direct[i] || want[ref.NWO()+"@"+ref.Ref] + } } return DirectTracker{direct: direct} } diff --git a/internal/pin/commit.go b/internal/pin/commit.go index 30908ee9..b1e3332e 100644 --- a/internal/pin/commit.go +++ b/internal/pin/commit.go @@ -30,6 +30,10 @@ func Commit(ctx context.Context, rec *Record, store *lockfile.State, copts *Comm progress = copts.OnProgress } + if err := validateRequiredRewrites(rec.Workflows); err != nil { + return err + } + // Phase 1: Rewrite workflow files (uses: line changes). if len(rec.Workflows) > 0 { progress("Rewriting workflows") @@ -86,6 +90,35 @@ func Commit(ctx context.Context, rec *Record, store *lockfile.State, copts *Comm return nil } +func validateRequiredRewrites(plans []WorkflowPlan) error { + for _, wp := range plans { + if len(wp.RequiredRewrites) == 0 { + continue + } + found := make(map[string]int) + paths := append([]string{wp.Path}, wp.SelfActionFiles...) + for _, path := range paths { + wf, err := workflowfile.Load(path) + if err != nil { + return fmt.Errorf("validating required rewrites in %s: %w", path, err) + } + matches, err := wf.ValidateRequiredActionRefRewrites(wp.RequiredRewrites) + if err != nil { + return fmt.Errorf("validating required rewrites in %s: %w", path, err) + } + for oldUse, count := range matches { + found[oldUse] += count + } + } + for oldUse := range wp.RequiredRewrites { + if found[oldUse] == 0 { + return fmt.Errorf("required action rewrite for %s was not found in writable workflow sources", oldUse) + } + } + } + return nil +} + func rewriteWorkflow(wp WorkflowPlan) error { wf, err := workflowfile.Load(wp.Path) if err != nil { diff --git a/internal/pin/plan.go b/internal/pin/plan.go index cf2f8882..c535d66b 100644 --- a/internal/pin/plan.go +++ b/internal/pin/plan.go @@ -8,6 +8,7 @@ import ( parserlock "github.com/github/actions-lockfile/go/pkg/lockfile" "github.com/github/gh-actions-lock/internal/dep" + "github.com/github/gh-actions-lock/internal/ghapi" "github.com/github/gh-actions-lock/internal/lockfile" "github.com/github/gh-actions-lock/internal/pinpool" "github.com/github/gh-actions-lock/internal/pipeline/checks" @@ -142,17 +143,36 @@ func planWorkflow(ctx context.Context, wr checks.WorkflowReport, opts PlanOption if finding.Category == checks.InvalidSelfRepositoryRef { return planResult{}, nil } + if finding.Category == checks.RepositoryChanged { + return planResult{}, fmt.Errorf("%s; %s", finding.Detail, finding.Remediation) + } } rewriteRefs := wr.RewriteRefs if rewriteRefs == nil { rewriteRefs = wr.ActionRefs } + resolvedTracker := lockfile.NewDirectTracker(rewriteRefs, wr.ResolvedDeps) + _, transferErr := validateTransferredRepositories(wr.ResolvedDeps, resolvedTracker, wr.ResolvedParents) + if transferErr != nil { + return planResult{}, transferErr + } + hasTransfer := false + knownTransfersNeedingResolution := make(map[ghapi.NWORef]bool) + for _, d := range wr.ResolvedDeps { + hasTransfer = hasTransfer || len(d.OriginalRefs) > 0 + if d.SHA == "" { + for _, ref := range d.OriginalRefs { + knownTransfersNeedingResolution[ghapi.ForNWORef(ref.Owner, ref.Repo, ref.Ref)] = true + } + } + } + // Drop stale inventory entries so a re-pin converges: the orphan leaves // workflows[path] and Save's GC removes its dependencies[] entry. inventory := pruneStaleInventory(wr.Inventory, wr.Findings, opts.AcceptMoved, opts.Relock) repinMoved := repinsMoved(opts) && wr.CountByCategory(checks.RefMoved) > 0 - if !wr.NeedsAttention() && !repinMoved { + if !wr.NeedsAttention() && !repinMoved && !hasTransfer { entries = verifiedEntries(inventory, wr.Path) rw := narrowVerifiedEntries(ctx, entries, opts, rewriteRefs) if err := rejectPartialSelfActionRewrites(opts, wr.SelfActionRefs, rw); err != nil { @@ -189,6 +209,10 @@ func planWorkflow(ctx context.Context, wr checks.WorkflowReport, opts PlanOption unrecordedRefs, inventorySHA = partitionByInventory(nil, wr.ActionRefs) entries = verifiedEntries(nil, wr.Path) } + if hasTransfer { + unrecordedRefs, inventorySHA = partitionByInventory(nil, wr.ActionRefs) + entries = verifiedEntries(nil, wr.Path) + } if len(unrecordedRefs) == 0 { rw := narrowVerifiedEntries(ctx, entries, opts, rewriteRefs) @@ -203,6 +227,14 @@ func planWorkflow(ctx context.Context, wr checks.WorkflowReport, opts PlanOption status("resolving " + wr.Path) deps, parentMap, resolveErr := opts.Resolver.ResolveAllRecursive(ctx, unrecordedRefs) if resolveErr != nil { + for _, d := range deps { + for _, ref := range d.OriginalRefs { + delete(knownTransfersNeedingResolution, ghapi.ForNWORef(ref.Owner, ref.Repo, ref.Ref)) + } + } + if len(knownTransfersNeedingResolution) > 0 { + return planResult{}, fmt.Errorf("resolving transferred repository: %w", resolveErr) + } // A resolved root is not pinnable when its transitive graph is incomplete. for _, ref := range unrecordedRefs { entries = append(entries, Entry{ @@ -220,6 +252,11 @@ func planWorkflow(ctx context.Context, wr checks.WorkflowReport, opts PlanOption // workflow YAML. rootTracker := lockfile.NewDirectTracker(unrecordedRefs, deps) rewriteTracker := lockfile.NewDirectTracker(rewriteRefs, deps) + canonicalRekeys, err := validateTransferredRepositories(deps, rewriteTracker, parentMap) + if err != nil { + return planResult{}, err + } + parentMap = dep.RekeyParentMap(parentMap, canonicalRekeys) // Narrow mutable version tags to exact patch tags. status("pinning " + wr.Path) @@ -258,12 +295,14 @@ func planWorkflow(ctx context.Context, wr checks.WorkflowReport, opts PlanOption } } deps = filtered - // Rebuild the root tracker against the filtered slice. + // Filtering changes indices, so both index-aligned trackers must follow. rootTracker = lockfile.NewDirectTracker(unrecordedRefs, deps) + rewriteTracker = lockfile.NewDirectTracker(rewriteRefs, deps) } for k, v := range rlRewrites { rewrites[k] = v } + requiredRewrites := addTransferredRepositoryRewrites(deps, rewriteTracker, rewrites) // Update parent map keys to reflect narrowed/normalized refs. parentRewrites := make(map[string]string) @@ -295,9 +334,10 @@ func planWorkflow(ctx context.Context, wr checks.WorkflowReport, opts PlanOption } if len(rewrites) > 0 { wplans = append(wplans, WorkflowPlan{ - Path: wr.Path, - Rewrites: rewrites, - SelfActionFiles: wr.SelfActionFiles, + Path: wr.Path, + Rewrites: rewrites, + RequiredRewrites: requiredRewrites, + SelfActionFiles: wr.SelfActionFiles, }) } else if len(wplans) == 0 { // Keep the workflow in the plan so its lockfile entry is updated. @@ -331,6 +371,51 @@ func rejectPartialSelfActionRewrites(opts PlanOptions, selfActionRefs []parserlo return nil } +func validateTransferredRepositories(deps []dep.Dependency, directTracker lockfile.DirectTracker, parentMap dep.ParentMap) (map[string]string, error) { + rekeys := make(map[string]string) + for i, d := range deps { + for _, ref := range d.OriginalRefs { + oldKey := ref.NWO() + "@" + ref.Ref + if parents := parentMap[oldKey]; len(parents) > 0 { + return nil, &resolve.TransferredRepositoryError{ + Original: ref.NWO(), + Canonical: d.NWO, + Parent: parents[0], + } + } + if !directTracker.IsDirect(i) { + return nil, &resolve.TransferredRepositoryError{ + Original: ref.NWO(), + Canonical: d.NWO, + Parent: "unknown", + } + } + rekeys[oldKey] = d.Key() + } + } + return rekeys, nil +} + +func addTransferredRepositoryRewrites(deps []dep.Dependency, directTracker lockfile.DirectTracker, rewrites map[string]string) map[string]string { + required := make(map[string]string) + for i, d := range deps { + if !directTracker.IsDirect(i) { + continue + } + for _, ref := range d.OriginalRefs { + newUse := d.NWO + if ref.Path != "" { + newUse += "/" + ref.Path + } + oldUse := ref.FullName() + "@" + ref.Ref + newUse += "@" + d.Ref + rewrites[oldUse] = newUse + required[oldUse] = newUse + } + } + return required +} + // narrowDirectDeps rewrites direct partial semver refs to exact patch tags, // leaving bare SHA and transitive refs for reverse lookup. func narrowDirectDeps(ctx context.Context, opts PlanOptions, deps []dep.Dependency, directTracker lockfile.DirectTracker, rewrites map[string]string, preservedDeps map[int]bool) { diff --git a/internal/pin/plan_test.go b/internal/pin/plan_test.go index 131fc52f..c3655b4b 100644 --- a/internal/pin/plan_test.go +++ b/internal/pin/plan_test.go @@ -2,14 +2,17 @@ package pin import ( "context" + "io" + "net/http" + "strings" "testing" "github.com/github/gh-actions-lock/internal/dep" + "github.com/github/gh-actions-lock/internal/lockfile" "github.com/github/gh-actions-lock/internal/pipeline/checks" parserlock "github.com/github/actions-lockfile/go/pkg/lockfile" "github.com/github/gh-actions-lock/internal/ghapi/httpmock" - "github.com/github/gh-actions-lock/internal/lockfile" "github.com/github/gh-actions-lock/internal/pinpool" "github.com/github/gh-actions-lock/internal/resolve" "github.com/github/gh-actions-lock/internal/tag" @@ -17,6 +20,126 @@ import ( "github.com/stretchr/testify/require" ) +func TestTransferredRepositoryRewritePreservesSubpath(t *testing.T) { + deps := []dep.Dependency{{ + NWO: "new/action", + OriginalRefs: []parserlock.ActionRef{{ + Owner: "old", Repo: "action", Path: "sub", Ref: "v1.2.3", + }}, + Path: "sub", + Ref: "v1.2.3", + }} + refs := []parserlock.ActionRef{{ + Owner: "old", Repo: "action", Path: "sub", Ref: "v1.2.3", + }} + tracker := lockfile.NewDirectTracker(refs, deps) + + rekeys, err := validateTransferredRepositories(deps, tracker, nil) + require.NoError(t, err) + assert.Equal(t, "new/action@v1.2.3", rekeys["old/action@v1.2.3"]) + + rewrites := map[string]string{} + addTransferredRepositoryRewrites(deps, tracker, rewrites) + assert.Equal(t, "new/action/sub@v1.2.3", rewrites["old/action/sub@v1.2.3"]) +} + +func TestTransferredRepositoryRejectsRemoteParentEvenWhenAlsoDirect(t *testing.T) { + original := parserlock.ActionRef{Owner: "old", Repo: "action", Ref: "v1"} + deps := []dep.Dependency{{ + NWO: "new/action", + OriginalRefs: []parserlock.ActionRef{original}, + Ref: "v1", + }} + tracker := lockfile.NewDirectTracker([]parserlock.ActionRef{original}, deps) + + _, err := validateTransferredRepositories(deps, tracker, dep.ParentMap{ + "old/action@v1": {"root/composite@v2"}, + }) + + var transferred *resolve.TransferredRepositoryError + require.ErrorAs(t, err, &transferred) + assert.Equal(t, "root/composite@v2", transferred.Parent) +} + +func TestTransferredRepositoryRewriteAfterEarlierLookupIssue(t *testing.T) { + const ( + orphanSHA = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + transferredSHA = "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb" + ) + reg := &httpmock.Registry{} + reg.Register( + httpmock.GraphQLForRepo("orphan", "action"), + httpmock.JSONResponse(map[string]any{ + "data": map[string]any{ + "a0": map[string]any{ + "nameWithOwner": "orphan/action", + "object": map[string]any{ + "oid": orphanSHA, + "file": map[string]any{"object": map[string]any{"text": "runs:\n using: node20\n"}}, + }, + }, + "a1": map[string]any{ + "nameWithOwner": "new/action", + "object": map[string]any{ + "oid": transferredSHA, + "file": map[string]any{"object": map[string]any{"text": "runs:\n using: node20\n"}}, + }, + }, + }, + }), + ) + transport := roundTripFunc(func(req *http.Request) (*http.Response, error) { + if req.Method == http.MethodPost { + return reg.RoundTrip(req) + } + status, body := http.StatusOK, "[]" + if strings.HasSuffix(req.URL.Path, "/repos/orphan/action") { + body = `{"default_branch":"main"}` + } else if strings.Contains(req.URL.Path, "/git/ref/") { + status, body = http.StatusNotFound, `{"message":"Not Found"}` + } + return &http.Response{ + StatusCode: status, + Body: io.NopCloser(strings.NewReader(body)), + Header: http.Header{"Content-Type": []string{"application/json"}}, + Request: req, + }, nil + }) + pool := pinpool.New(2, nil) + resolver, err := resolve.New("github.com", pool, resolve.WithTransport(transport)) + require.NoError(t, err) + original := parserlock.ActionRef{Owner: "old", Repo: "action", Ref: "v1"} + wr := checks.WorkflowReport{ + Path: ".github/workflows/test.yml", + Findings: []checks.Finding{{ + ActionRef: &original, + Category: "unpinned", + Severity: checks.SeverityWarning, + Confidence: checks.ConfidenceHigh, + }}, + ActionRefs: []parserlock.ActionRef{ + {Owner: "orphan", Repo: "action", Ref: orphanSHA}, + original, + }, + RewriteRefs: []parserlock.ActionRef{original}, + } + + result, err := planWorkflow(context.Background(), wr, PlanOptions{ + Resolver: resolver, + Pool: pool, + }, func(string) {}) + require.NoError(t, err) + reg.Verify(t) + require.Len(t, result.wplans, 1) + assert.Equal(t, "new/action@v1", result.wplans[0].RequiredRewrites["old/action@v1"]) +} + +type roundTripFunc func(*http.Request) (*http.Response, error) + +func (f roundTripFunc) RoundTrip(req *http.Request) (*http.Response, error) { + return f(req) +} + func TestNarrowDirectDeps_PreservesRefWhenExactTagIsFromAnotherFamily(t *testing.T) { const sha = "94de994a9f6fffee200243214e17002e2920bb59" @@ -119,14 +242,14 @@ func TestPlanWorkflow_PartialResolutionFailure(t *testing.T) { goodSHA := "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" - // Both refs fold into one batched query: a0=good/action resolves, + // Both refs fold into one batched query: a0=old/action redirects and resolves, // a1=bad/private is null (repo not found). reg.Register( - httpmock.GraphQLForRepo("good", "action"), + httpmock.GraphQLForRepo("old", "action"), httpmock.JSONResponse(map[string]any{ "data": map[string]any{ "a0": map[string]any{ - "nameWithOwner": "good/action", + "nameWithOwner": "new/action", "object": map[string]any{ "oid": goodSHA, "file": map[string]any{"object": map[string]any{"text": "name: Good\nruns:\n using: node20\n"}}, @@ -145,7 +268,7 @@ func TestPlanWorkflow_PartialResolutionFailure(t *testing.T) { Path: ".github/workflows/test.yml", Findings: []checks.Finding{ { - ActionRef: &parserlock.ActionRef{Owner: "good", Repo: "action", Ref: "v1"}, + ActionRef: &parserlock.ActionRef{Owner: "old", Repo: "action", Ref: "v1"}, Category: "unpinned", Severity: checks.SeverityWarning, Confidence: checks.ConfidenceHigh, @@ -158,7 +281,7 @@ func TestPlanWorkflow_PartialResolutionFailure(t *testing.T) { }, }, ActionRefs: []parserlock.ActionRef{ - {Owner: "good", Repo: "action", Ref: "v1"}, + {Owner: "old", Repo: "action", Ref: "v1"}, {Owner: "bad", Repo: "private", Ref: "main"}, }, } diff --git a/internal/pin/record.go b/internal/pin/record.go index 0811dcd9..5c5b02a1 100644 --- a/internal/pin/record.go +++ b/internal/pin/record.go @@ -49,8 +49,9 @@ type Entry struct { // WorkflowPlan records what Commit must write for one workflow file. // Internal to the pin lifecycle; not serialized. type WorkflowPlan struct { - Path string - Rewrites map[string]string + Path string + Rewrites map[string]string + RequiredRewrites map[string]string // SelfActionFiles are in-repo action definition files reached from this // workflow through `$/…`. The same rewrites apply to their `uses:` lines. SelfActionFiles []string diff --git a/internal/pipeline/checks/category.go b/internal/pipeline/checks/category.go index 802efcdc..45ced40e 100644 --- a/internal/pipeline/checks/category.go +++ b/internal/pipeline/checks/category.go @@ -33,6 +33,9 @@ const ( // entry was tampered with. The check can't distinguish those, so it // fails closed without asserting an attack. UnreachablePin Category = "unreachable-pin" + // RepositoryChanged means the repository at a locked name has a different + // numeric repository ID and is therefore not the repository that was pinned. + RepositoryChanged Category = "repository-changed" // Valid means the dependency is pinned and verified. Valid Category = "valid" // RunOnly means the workflow has no action refs (only run: diff --git a/internal/pipeline/checks/category_test.go b/internal/pipeline/checks/category_test.go index f1ddc200..f2d25420 100644 --- a/internal/pipeline/checks/category_test.go +++ b/internal/pipeline/checks/category_test.go @@ -18,6 +18,7 @@ func TestCategoryStringsAreFrozen(t *testing.T) { {Stale, "stale"}, {MisleadingSHA, "misleading-sha"}, {UnreachablePin, "unreachable-pin"}, + {RepositoryChanged, "repository-changed"}, {Valid, "valid"}, {RunOnly, "run-only"}, {AncestryUnknown, "ancestry-unknown"}, @@ -50,7 +51,7 @@ func TestCategoryIsInconclusive(t *testing.T) { } blocking := []Category{ NotPinned, ShaAsRef, RefChanged, RefMoved, Stale, - MisleadingSHA, UnreachablePin, + MisleadingSHA, UnreachablePin, RepositoryChanged, Valid, RunOnly, OnboardingRequired, VersionRef, LocalAction, StaleWorkflow, SelfRepositoryAction, InvalidSelfRepositoryRef, diff --git a/internal/pipeline/checks/finding.go b/internal/pipeline/checks/finding.go index 0d82da92..88bdceac 100644 --- a/internal/pipeline/checks/finding.go +++ b/internal/pipeline/checks/finding.go @@ -77,6 +77,10 @@ type WorkflowReport struct { SelfActionRefs []parserlock.ActionRef // Deps are the existing pinned dependencies (nil if not pinned). Deps []dep.Dependency + // ResolvedDeps and ResolvedParents retain live resolver identity details + // needed by the pin planner, including repository transfers. + ResolvedDeps []dep.Dependency + ResolvedParents dep.ParentMap // Inventory lists all dependencies with direct/transitive classification. Inventory []InventoryEntry // ParseWarnings from ExtractActionRefs (e.g. malformed uses: lines). diff --git a/internal/pipeline/checks/resolver.go b/internal/pipeline/checks/resolver.go index 304b4a48..e0ceb775 100644 --- a/internal/pipeline/checks/resolver.go +++ b/internal/pipeline/checks/resolver.go @@ -44,6 +44,9 @@ func NewPrewarmedResolver(r *resolve.Resolver, live []dep.Dependency) *prewarmed for _, d := range live { owner, repo := d.OwnerRepo() a.refs[ghapi.ForNWORef(owner, repo, d.Ref)] = d.SHA + for _, ref := range d.OriginalRefs { + a.refs[ghapi.ForNWORef(ref.Owner, ref.Repo, ref.Ref)] = d.SHA + } } return a } diff --git a/internal/pipeline/diagnose.go b/internal/pipeline/diagnose.go index da402006..426210e7 100644 --- a/internal/pipeline/diagnose.go +++ b/internal/pipeline/diagnose.go @@ -127,7 +127,10 @@ func diagnoseOneParsed(ctx context.Context, pw checks.ParsedWorkflow, r *resolve parentMap := map[string][]string{} if r != nil { parentMap = resolvedParents + wr.ResolvedDeps = liveDeps + wr.ResolvedParents = resolvedParents populateInventoryParents(wr.Inventory, parentMap) + appendTransferredRepositoryFindings(&wr, liveDeps, parentMap) } var checkR checks.CheckResolver @@ -170,6 +173,33 @@ func diagnoseOneParsed(ctx context.Context, pw checks.ParsedWorkflow, r *resolve return wr } +func appendTransferredRepositoryFindings(wr *checks.WorkflowReport, deps []dep.Dependency, parentMap dep.ParentMap) { + direct := lockfile.NewDirectTracker(wr.RewriteRefs, deps) + for i, d := range deps { + for _, ref := range d.OriginalRefs { + finding := checks.Finding{ + WorkflowPath: wr.Path, + Category: checks.RefChanged, + Severity: checks.SeverityError, + Confidence: checks.ConfidenceHigh, + ActionRef: &ref, + Detail: fmt.Sprintf("repository %s has been renamed or transferred to %s", ref.NWO(), d.NWO), + Remediation: fmt.Sprintf("update `uses:` from %s to %s", ref.NWO(), d.NWO), + } + oldKey := ref.NWO() + "@" + ref.Ref + if parents := parentMap[oldKey]; len(parents) > 0 { + finding.ParentNWO = parents[0] + finding.Detail += fmt.Sprintf(" in upstream composite %s", parents[0]) + finding.Remediation = "the upstream composite must update its `uses:` reference" + } else if !direct.IsDirect(i) { + finding.ParentNWO = "unknown" + finding.Remediation = "the upstream composite containing this reference must update its `uses:` reference" + } + wr.Findings = append(wr.Findings, finding) + } + } +} + // selfRepositoryFinding builds the informational finding for a workflow that // references same-repo actions via `$/…`. These are inherently pinned. func selfRepositoryFinding(pw checks.ParsedWorkflow) checks.Finding { diff --git a/internal/pipeline/diagnose_test.go b/internal/pipeline/diagnose_test.go index 4a7d8f60..a42f9639 100644 --- a/internal/pipeline/diagnose_test.go +++ b/internal/pipeline/diagnose_test.go @@ -4,6 +4,8 @@ import ( "context" "testing" + parserlock "github.com/github/actions-lockfile/go/pkg/lockfile" + "github.com/github/gh-actions-lock/internal/dep" "github.com/github/gh-actions-lock/internal/lockfile" "github.com/github/gh-actions-lock/internal/pipeline/checks" "github.com/stretchr/testify/assert" @@ -111,3 +113,24 @@ func TestDiagnoseOneParsed_SelfRepositoryResolutionError(t *testing.T) { assert.Equal(t, checks.SeverityError, wr.Findings[0].Severity) assert.False(t, wr.IsValid()) } + +func TestTransferredRepositoryFindingNamesRemoteComposite(t *testing.T) { + original := parserlock.ActionRef{Owner: "old", Repo: "action", Ref: "v1"} + wr := checks.WorkflowReport{Path: ".github/workflows/ci.yml"} + + appendTransferredRepositoryFindings(&wr, []dep.Dependency{{ + NWO: "new/action", + Ref: "v1", + OriginalRefs: []parserlock.ActionRef{original}, + }}, dep.ParentMap{ + "old/action@v1": {"root/composite@v2"}, + }) + + require.Len(t, wr.Findings, 1) + assert.Equal(t, checks.SeverityError, wr.Findings[0].Severity) + assert.Contains(t, wr.Findings[0].Detail, "old/action") + assert.Contains(t, wr.Findings[0].Detail, "new/action") + assert.Contains(t, wr.Findings[0].Detail, "root/composite@v2") + assert.Equal(t, "root/composite@v2", wr.Findings[0].ParentNWO) + assert.Contains(t, wr.Findings[0].Remediation, "upstream composite") +} diff --git a/internal/pipeline/doc_urls.go b/internal/pipeline/doc_urls.go index 1739a73d..f7e6ed6d 100644 --- a/internal/pipeline/doc_urls.go +++ b/internal/pipeline/doc_urls.go @@ -36,6 +36,7 @@ var docURLs = map[checks.Category]string{ checks.MisleadingSHA: securityHardeningBase + "#using-third-party-actions", checks.RefMoved: securityHardeningBase + "#using-third-party-actions", checks.UnreachablePin: securityHardeningBase + "#using-third-party-actions", + checks.RepositoryChanged: securityHardeningBase + "#using-third-party-actions", checks.OnboardingRequired: securityHardeningBase + "#using-third-party-actions", checks.AncestryUnknown: securityHardeningBase + "#using-third-party-actions", checks.ReachabilityUnknown: securityHardeningBase + "#using-third-party-actions", diff --git a/internal/pipeline/run.go b/internal/pipeline/run.go index 46aa908a..e0267467 100644 --- a/internal/pipeline/run.go +++ b/internal/pipeline/run.go @@ -2,15 +2,18 @@ package pipeline import ( "context" + "fmt" "strings" parserlock "github.com/github/actions-lockfile/go/pkg/lockfile" "github.com/github/gh-actions-lock/internal/dep" + "github.com/github/gh-actions-lock/internal/ghapi" "github.com/github/gh-actions-lock/internal/lockfile" "github.com/github/gh-actions-lock/internal/pinpool" "github.com/github/gh-actions-lock/internal/pipeline/checks" "github.com/github/gh-actions-lock/internal/profile" "github.com/github/gh-actions-lock/internal/resolve" + "github.com/github/gh-actions-lock/internal/workflowfile" ) // RunOptions configures the Run pipeline. @@ -56,9 +59,29 @@ func Run(ctx context.Context, opts RunOptions) (*RunResult, error) { // Immutable full-semver pins (e.g. v4.2.1) are NOT trusted blindly: // they're routed through live resolution + ancestry so a stale or // unreachable pin is caught on the default path, not just under - // --rescan. Mutable recorded refs (v4, v4.2, branches) legitimately - // move, so they stay trusted (seeded from the lockfile) until --rescan. + // --rescan. Mutable recorded refs (v4, v4.2, branches) legitimately move, + // so they stay trusted after a cheap repository identity check confirms + // the NWO. skippedRescan := 0 + fastPlans := make([]fastPathPlan, len(parsed)) + identityRefs := make([][]repositoryIdentityRef, len(parsed)) + var lockSnapshot parserlock.File + if opts.Store != nil { + lockSnapshot = opts.Store.File() + } + homeHostname := "" + if r != nil { + homeHostname = r.Hostname() + } + for i := range parsed { + if len(parsed[i].LocalPaths) == 0 && + len(parsed[i].SelfRepositoryRefErrs) == 0 && + len(parsed[i].SelfRepositoryResolutionErrs) == 0 { + fastPlans[i] = planFastPath(parsed[i]) + identityRefs[i] = repositoryIdentityRefs(parsed[i].Path, lockSnapshot, homeHostname) + } + } + repositoryIdentities := lookupRepositoryIdentities(ctx, r, opts.Pool, identityRefs) var seedDeps []dep.Dependency recordedKeys := make(map[string]bool) for i := range parsed { @@ -73,9 +96,29 @@ func Run(ctx context.Context, opts RunOptions) (*RunResult, error) { if opts.Rescan { continue } - plan := planFastPath(parsed[i]) - // Mutable recorded refs are trusted without a live re-check - // (surfaced in the summary so the operator can --rescan them). + plan := fastPlans[i] + trustedMutable := plan.mutableRefs[:0] + for _, ref := range plan.mutableRefs { + identityRef := lockedRepositoryIdentity(identityRefs[i], ref) + identity := repositoryIdentities[repositoryIdentityKey(identityRef.Hostname, ref)] + if identity.matches(ref.NWO(), identityRef.RepoID) { + trustedMutable = append(trustedMutable, ref) + } + } + for _, item := range identityRefs[i] { + if item.Parent == "" { + continue + } + identity := repositoryIdentities[repositoryIdentityKey(item.Hostname, item.Ref)] + if !identity.matches(item.Ref.NWO(), item.RepoID) { + trustedMutable = nil + break + } + } + plan.resolved = plan.resolved && len(trustedMutable) == len(plan.mutableRefs) + plan.mutableRefs = trustedMutable + // Mutable recorded refs are trusted without live action resolution + // after the repository identity check above. skippedRescan += len(plan.mutableRefs) if plan.resolved { parsed[i].Resolved = true @@ -145,6 +188,7 @@ func Run(ctx context.Context, opts RunOptions) (*RunResult, error) { // Phase 3: Diagnose. endDiag := prof.Phase(" diagnose (parallel)") report := DiagnoseParsed(ctx, parsed, r, opts.Store, opts.Pool) + appendKnownRepositoryIdentityFindings(report, identityRefs, repositoryIdentities) endDiag() valid := report.IsValid() @@ -155,6 +199,183 @@ func Run(ctx context.Context, opts RunOptions) (*RunResult, error) { }, nil } +type repositoryIdentityRef struct { + Ref parserlock.ActionRef + Hostname string + Parent string + RepoID int64 +} + +func repositoryIdentityRefs(path string, file parserlock.File, homeHostname string) []repositoryIdentityRef { + var refs []repositoryIdentityRef + index := make(map[ghapi.NWORef]int) + add := func(ref parserlock.ActionRef, hostname, parent string, repoID int64) { + key := ghapi.ForNWORef(ref.Owner, ref.Repo, ref.Ref) + if i, ok := index[key]; ok { + if hostname != "" { + refs[i].Hostname = hostname + } + if parent != "" { + refs[i].Parent = parent + } + if repoID != 0 { + refs[i].RepoID = repoID + } + return + } + index[key] = len(refs) + refs = append(refs, repositoryIdentityRef{Ref: ref, Hostname: hostname, Parent: parent, RepoID: repoID}) + } + seen := make(map[string]bool) + var walk func(string, string) + walk = func(pinKey, parent string) { + if seen[pinKey] { + return + } + seen[pinKey] = true + pin, ok := parserlock.ParsePin(pinKey) + if !ok { + return + } + action := file.Dependencies[pinKey] + hostname := action.Hostname + if hostname == "" { + hostname = homeHostname + } + add(parserlock.ActionRef{Owner: pin.Owner, Repo: pin.Repo, Ref: pin.Ref}, hostname, parent, action.RepoID) + for _, child := range action.Uses { + walk(child, pinKey) + } + } + for _, root := range file.Workflows[workflowfile.KeyFromPath(path)] { + walk(root, "") + } + return refs +} + +type repositoryIdentity struct { + canonical string + repoID int64 +} + +func (i repositoryIdentity) matches(nwo string, repoID int64) bool { + return i.canonical != "" && strings.EqualFold(i.canonical, nwo) && + (repoID == 0 || i.repoID == repoID) +} + +func lockedRepositoryIdentity(refs []repositoryIdentityRef, ref parserlock.ActionRef) repositoryIdentityRef { + key := ghapi.ForNWORef(ref.Owner, ref.Repo, ref.Ref) + for _, item := range refs { + if ghapi.ForNWORef(item.Ref.Owner, item.Ref.Repo, item.Ref.Ref) == key { + return item + } + } + return repositoryIdentityRef{Ref: ref} +} + +func repositoryIdentityKey(hostname string, ref parserlock.ActionRef) string { + return strings.ToLower(hostname + "/" + ref.NWO()) +} + +func lookupRepositoryIdentities(ctx context.Context, r *resolve.Resolver, pool *pinpool.Pool, workflows [][]repositoryIdentityRef) map[string]repositoryIdentity { + type indexedRef struct { + idx int + ref repositoryIdentityRef + } + var repos []indexedRef + seen := make(map[string]bool) + for _, identities := range workflows { + for _, item := range identities { + if item.Hostname == "" && r != nil { + item.Hostname = r.Hostname() + } + key := repositoryIdentityKey(item.Hostname, item.Ref) + if !seen[key] { + seen[key] = true + repos = append(repos, indexedRef{idx: len(repos), ref: item}) + } + } + } + results := make([]repositoryIdentity, len(repos)) + if r != nil { + _ = pinpool.RunTyped(pool, ctx, "", repos, + func(indexedRef) string { return "" }, + func(ctx context.Context, _ int, item indexedRef) error { + ref := item.ref.Ref + canonical, err := r.CanonicalNWO(ctx, ref.Owner, ref.Repo) + if err == nil { + results[item.idx].canonical = canonical + _, repoID, idErr := r.RepoIDs(ctx, item.ref.Hostname, ref.Owner, ref.Repo) + if idErr == nil { + results[item.idx].repoID = repoID + } + } + return nil + }, + ) + } + identities := make(map[string]repositoryIdentity, len(repos)) + for _, item := range repos { + identities[repositoryIdentityKey(item.ref.Hostname, item.ref.Ref)] = results[item.idx] + } + return identities +} + +func appendKnownRepositoryIdentityFindings(report *checks.Report, workflows [][]repositoryIdentityRef, identities map[string]repositoryIdentity) { + for i := range report.Workflows { + wr := &report.Workflows[i] + for _, item := range workflows[i] { + ref := item.Ref + identity := identities[repositoryIdentityKey(item.Hostname, ref)] + if identity.canonical == "" { + continue + } + if strings.EqualFold(identity.canonical, ref.NWO()) { + if item.RepoID != 0 && identity.repoID != 0 && identity.repoID != item.RepoID { + wr.Findings = append(wr.Findings, checks.Finding{ + WorkflowPath: wr.Path, + Category: checks.RepositoryChanged, + Severity: checks.SeverityError, + Confidence: checks.ConfidenceHigh, + ActionRef: &ref, + Detail: fmt.Sprintf("repository identity changed for %s: the lockfile records repository ID %d, but the current repository ID is %d. This may indicate a namespace takeover", ref.NWO(), item.RepoID, identity.repoID), + Remediation: fmt.Sprintf("review %s before trusting it. If the replacement is expected, remove its lockfile entry and run `gh actions-lock` again", ref.NWO()), + }) + } + continue + } + if resolvedTransfer(wr.ResolvedDeps, ref) { + continue + } + known := dep.Dependency{ + Hostname: item.Hostname, + NWO: identity.canonical, + Ref: ref.Ref, + OriginalRefs: []parserlock.ActionRef{ref}, + } + wr.ResolvedDeps = append(wr.ResolvedDeps, known) + if item.Parent != "" { + if wr.ResolvedParents == nil { + wr.ResolvedParents = make(dep.ParentMap) + } + wr.ResolvedParents[ref.NWO()+"@"+ref.Ref] = []string{item.Parent} + } + appendTransferredRepositoryFindings(wr, []dep.Dependency{known}, wr.ResolvedParents) + } + } +} + +func resolvedTransfer(deps []dep.Dependency, original parserlock.ActionRef) bool { + for _, d := range deps { + for _, ref := range d.OriginalRefs { + if strings.EqualFold(ref.NWO(), original.NWO()) && ref.Ref == original.Ref { + return true + } + } + } + return false +} + // fastPathPlan describes how the pre-resolution fast path treats one // recorded workflow. type fastPathPlan struct { @@ -162,8 +383,8 @@ type fastPathPlan struct { // no refs, is a local-path action, or every recorded ref is a trusted // mutable pin. resolved bool - // mutableRefs are recorded refs (v4, v4.2, branches) trusted from the - // lockfile without a live re-check. + // mutableRefs are recorded refs (v4, v4.2, branches) eligible for trust + // from the lockfile after their repository identities are validated. mutableRefs []parserlock.ActionRef } diff --git a/internal/pipeline/run_test.go b/internal/pipeline/run_test.go index 380feff9..840e142e 100644 --- a/internal/pipeline/run_test.go +++ b/internal/pipeline/run_test.go @@ -91,6 +91,31 @@ func TestPlanFastPath(t *testing.T) { } } +func TestRepositoryIdentityRefsIncludesLockedClosure(t *testing.T) { + file := parserlock.File{ + Workflows: map[string][]string{ + ".github/workflows/ci.yml": {"root/composite@v1", "other/action@v1"}, + }, + Dependencies: map[string]parserlock.Action{ + "root/composite@v1": {RepoID: 10, Uses: []string{"old/action@v1"}}, + "old/action@v1": {RepoID: 20}, + "other/action@v1": {RepoID: 30}, + }, + } + got := repositoryIdentityRefs(".github/workflows/ci.yml", file, "") + + assert.Len(t, got, 3) + assert.Equal(t, "root/composite", got[0].Ref.NWO()) + assert.EqualValues(t, 10, got[0].RepoID) + assert.Empty(t, got[0].Parent) + assert.Equal(t, "old/action", got[1].Ref.NWO()) + assert.EqualValues(t, 20, got[1].RepoID) + assert.Equal(t, "root/composite@v1", got[1].Parent) + assert.Equal(t, "other/action", got[2].Ref.NWO()) + assert.EqualValues(t, 30, got[2].RepoID) + assert.Empty(t, got[2].Parent) +} + func TestPartitionRefs(t *testing.T) { tests := []struct { name string diff --git a/internal/resolve/discovery.go b/internal/resolve/discovery.go index 30b50bb8..978607e0 100644 --- a/internal/resolve/discovery.go +++ b/internal/resolve/discovery.go @@ -52,6 +52,19 @@ func IsInvalidSelfRepositoryRef(err error) bool { return errors.As(err, &target) } +// TransferredRepositoryError reports a transferred action referenced by a +// remote composite that this repository cannot rewrite. +type TransferredRepositoryError struct { + Original string + Canonical string + Parent string +} + +func (e *TransferredRepositoryError) Error() string { + return fmt.Sprintf("repository %s has been renamed or transferred to %s; upstream composite %s must update its `uses:` reference", + e.Original, e.Canonical, e.Parent) +} + // selfRepositoryPrefix marks a `$/…` self repository action inside a composite's // nested uses. Kept local to avoid importing the workflowfile package into the // resolver; the sibling detection here is a plain prefix check. @@ -383,12 +396,17 @@ func (r *Resolver) resolveWithActionYMLParallel(ctx context.Context, refs []reso for j, idx := range b.idxs { ref := refs[idx].ref if j < len(res) && res[j].Err == nil { + var originalRefs []parserlock.ActionRef + if res[j].OriginalNWO != "" { + originalRefs = append(originalRefs, ref) + } d := dep.Dependency{ - Hostname: res[j].Hostname, - NWO: res[j].Owner + "/" + res[j].Repo, - Path: res[j].Path, - Ref: ref.Ref, - SHA: res[j].CommitOID, + Hostname: res[j].Hostname, + NWO: res[j].Owner + "/" + res[j].Repo, + OriginalRefs: originalRefs, + Path: res[j].Path, + Ref: ref.Ref, + SHA: res[j].CommitOID, } r.cache.Put(cacheKey(ref), resolvedEntry{dep: d, actionYML: res[j].ActionYML}) results[idx] = resolveResult{dep: d, yml: res[j].ActionYML, ok: true} diff --git a/internal/resolve/resolver.go b/internal/resolve/resolver.go index 929cd36e..c4e061aa 100644 --- a/internal/resolve/resolver.go +++ b/internal/resolve/resolver.go @@ -180,6 +180,11 @@ func (r *Resolver) RepoIDs(ctx context.Context, hostname, owner, repo string) (i return client.RepoIDs(ctx, owner, repo) } +// CanonicalNWO returns the repository's current owner/name. +func (r *Resolver) CanonicalNWO(ctx context.Context, owner, repo string) (string, error) { + return r.gh.CanonicalNWO(ctx, owner, repo) +} + // branchHint returns the branch previously recorded as containing sha in // owner/repo, or "" if no hint exists. func (r *Resolver) branchHint(owner, repo, sha string) string { diff --git a/internal/workflowfile/rewrite.go b/internal/workflowfile/rewrite.go index e1818d74..bee831ef 100644 --- a/internal/workflowfile/rewrite.go +++ b/internal/workflowfile/rewrite.go @@ -77,6 +77,63 @@ func (f *File) RewriteActionRefs(replacements map[string]string) ([]byte, int, e return []byte(strings.Join(lines, "\n")), changed, nil } +// ValidateRequiredActionRefRewrites rejects replacements that cannot be +// applied without changing every alias of an anchored scalar. +func (f *File) ValidateRequiredActionRefRewrites(replacements map[string]string) (map[string]int, error) { + matches := make(map[string]int) + blocked := "" + var walk func(*yaml.Node, bool, int) + walk = func(node *yaml.Node, anchored bool, depth int) { + if node == nil || depth > maxYAMLWalkDepth { + return + } + anchored = anchored || node.Anchor != "" || node.Kind == yaml.AliasNode + switch node.Kind { + case yaml.DocumentNode, yaml.SequenceNode: + for _, child := range node.Content { + walk(child, anchored, depth+1) + } + case yaml.MappingNode: + for i := 0; i < len(node.Content)-1; i += 2 { + keyNode := node.Content[i] + valueNode := node.Content[i+1] + if keyNode.Value == "uses" { + target := valueNode + if valueNode.Kind == yaml.AliasNode && valueNode.Alias != nil { + target = valueNode.Alias + } + if target.Kind == yaml.ScalarNode { + oldValue := strings.TrimSpace(target.Value) + if _, ok := replacements[oldValue]; ok { + matches[oldValue]++ + if anchored || valueNode.Kind == yaml.AliasNode || target.Anchor != "" { + blocked = oldValue + } + } + } + } + walk(valueNode, anchored, depth+1) + } + } + } + walk(&f.root, false, 0) + if blocked != "" { + return nil, fmt.Errorf("required action rewrite for %s cannot update an anchored or aliased `uses:` value", blocked) + } + _, changed, err := f.RewriteActionRefs(replacements) + if err != nil { + return nil, err + } + total := 0 + for _, count := range matches { + total += count + } + if changed != total { + return nil, fmt.Errorf("required action rewrite matched %d `uses:` values but could update only %d", total, changed) + } + return matches, nil +} + // MigrateLocalActionsToSelfRepository rewrites same-repo `./…` composite action // references to the inherently-pinned `$/…` form. Only local paths that // resolve to an in-repo action file are rewritten — that in-repo existence is diff --git a/test/integration/run.rb b/test/integration/run.rb index 84d682bd..a0a2a1ac 100644 --- a/test/integration/run.rb +++ b/test/integration/run.rb @@ -343,6 +343,7 @@ def golden_json_diff(expected, actual, path) # ── Fixture data ──────────────────────────────────────────────────────── CHECKOUT_SHA = "de0fac2e4500dabe0009e67214ff5f5447ce83dd" +CHECKOUT_V420_SHA = "d632683dd7b4114ad314bca15554477dd762a938" SETUP_GO_SHA = "4a3601121dd01d1626a1e23e37211e3254c1c06c" CACHE_SHA = "27d5ce7f107fe9357f9df03efb73ab90386fccae" MAIN_BRANCH_SHA = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" @@ -437,7 +438,7 @@ def golden_json_diff(expected, actual, path) dependencies: { "actions/checkout@v4.2.0" => { "ref" => "v4.2.0", - "commit" => "sha1-#{CHECKOUT_SHA}", + "commit" => "sha1-#{CHECKOUT_V420_SHA}", "owner_id" => 44036562, "repo_id" => 197814629 } @@ -647,6 +648,26 @@ def wire_checkout_fresh(s, token) self_repository_shared_dependency: ->(s) { wire_checkout_success(s, "gho_fake_self_repository_token") }, + transferred_repository_rewritten: ->(s) { + fixture_sha = "ea53476fdc172d8552df5af9658a45a367e4f41d" + s.stub_server do |srv| + srv.on(:POST, %r{/graphql$}) do |_req| + [200, { "Content-Type" => "application/json" }, + JSON.generate({ data: { a0: { + nameWithOwner: "nodeselector/actions-test-fixtures", + object: { + oid: fixture_sha, + file: { object: { text: "name: Fixture\nruns:\n using: node20\n main: index.js\n" } } + } + } } })] + end + srv.on(:GET, %r{/repos/nodeselector/actions-test-fixtures$}) do |_req| + [200, { "Content-Type" => "application/json" }, + JSON.generate({ full_name: "nodeselector/actions-test-fixtures", id: 1_203_329_948, owner: { id: 29_457_092 } })] + end + end + s.env("GH_TOKEN" => "gho_fake_transfer_fixture_token") + }, # SSO scenarios: catch-all 403 with X-GitHub-SSO header sso_auth_failure: ->(s) { @@ -671,7 +692,7 @@ def wire_checkout_fresh(s, token) JSON.generate([{ name: "v4", commit: { sha: fake_sha } }])] elsif req.path.match?(%r{/repos/actions/checkout$}) [200, { "Content-Type" => "application/json" }, - JSON.generate({ default_branch: "main", visibility: "private", pushed_at: "2024-01-01T00:00:00Z", id: 1, owner: { id: 44036562 } })] + JSON.generate({ full_name: "actions/checkout", default_branch: "main", visibility: "private", pushed_at: "2024-01-01T00:00:00Z", id: 1, owner: { id: 44036562 } })] elsif req.path.include?("/compare/") [200, { "Content-Type" => "application/json" }, JSON.generate({ status: "behind", merge_base_commit: { sha: fake_sha } })] diff --git a/test/scenarios/catalog.yml b/test/scenarios/catalog.yml index 79377cbd..2378f1b7 100644 --- a/test/scenarios/catalog.yml +++ b/test/scenarios/catalog.yml @@ -1958,7 +1958,7 @@ scenarios: valid: true - name: dbot_transient_403_drops_pin category: dependabot - description: "SSO 403 on a previously-pinned action — pin retained, clean exit" + description: "SSO 403 on a previously-pinned action — pin retained with an inconclusive warning" needs_stub: true tags: [stub] flags: ["--no-onboard", "--no-narrow", "--no-interactive", "--json=valid,findings"] @@ -1976,7 +1976,9 @@ scenarios: - expr: '.valid' equals: "true" - expr: '.findings | length' - equals: "0" + equals: "1" + - expr: '.findings[0].category' + equals: "reachability-unknown" lockfile_contains: - "version: 'v0.0.2'" @@ -1984,11 +1986,6 @@ scenarios: - "ref: 'v4'" - "commit: 'sha1-de0fac2e4500dabe0009e67214ff5f5447ce83dd'" - golden_json: - cli_version: (devel) - findings: [] - lockfile_version: v0.0.2 - valid: true - name: dbot_impostor_blocks category: dependabot description: "Orphaned commit (no tag or branch) produces reachability-unknown warning in JSON findings" @@ -2010,6 +2007,33 @@ scenarios: - expr: '.findings[0].category' equals: 'reachability-unknown' + - name: transferred_repository_rewritten + category: security + description: "Real compiled binary consumes a controlled GraphQL redirect fixture, rewrites writable workflow source, and emits canonical lock metadata" + needs_stub: true + tags: [stub] + flags: ["--no-narrow", "--no-interactive"] + fixtures: + workflows: + ci.yml: + name: CI + actions: ["nodeselector/actions-test-fixtures-old@v1"] + expect: + exit: 0 + lockfile_exists: true + lockfile_contains: + - "'nodeselector/actions-test-fixtures@v1':" + lockfile_excludes: + - "nodeselector/actions-test-fixtures-old" + output_contains: + - "Action repository transferred: nodeselector/actions-test-fixtures-old → nodeselector/actions-test-fixtures" + files_contain: + .github/workflows/ci.yml: + - "uses: nodeselector/actions-test-fixtures@v1" + files_exclude: + .github/workflows/ci.yml: + - "uses: nodeselector/actions-test-fixtures-old@v1" + - name: dbot_forgery_blocks category: dependabot description: "Stale pin (lockfile SHA not reachable from the ref head) produces unreachable-pin/error finding" From d6f2aa0fe10d207262a57142c78cd7c2ab85409b Mon Sep 17 00:00:00 2001 From: Jeff Martin Date: Fri, 9 Oct 2026 08:56:53 -0700 Subject: [PATCH 2/4] Fail closed when locked repository identity can't be verified Repository identity lookups for locked dependencies used to drop errors silently, which let rate limits or SSO blocks bypass the rename, transfer and namespace-takeover checks. Any lookup failure now produces a blocking repository-identity-unknown finding, and nothing is written. The host-checked RepoIDs lookup now runs before CanonicalNWO, so the canonical name always comes from the recorded host. A repo ID mismatch is checked before comparing names, which also catches a transfer to a different repository. Stale lockfile roots the workflow no longer uses are skipped, so a deleted repository can't block the run that prunes it. --- cmd/gh-actions-lock/check_json_golden_test.go | 8 + cmd/gh-actions-lock/command_test.go | 141 +++++++++++++++++- cmd/gh-actions-lock/format/terminal.go | 6 +- cmd/gh-actions-lock/proxima_test.go | 4 +- cmd/gh-actions-lock/selfrepository_test.go | 17 ++- internal/pin/plan.go | 2 +- internal/pipeline/checks/category.go | 5 + internal/pipeline/checks/category_test.go | 3 +- internal/pipeline/doc_urls.go | 23 +-- internal/pipeline/run.go | 72 ++++++--- internal/pipeline/run_test.go | 14 +- test/integration/run.rb | 20 +++ test/scenarios/catalog.yml | 12 +- 13 files changed, 273 insertions(+), 54 deletions(-) diff --git a/cmd/gh-actions-lock/check_json_golden_test.go b/cmd/gh-actions-lock/check_json_golden_test.go index 996827b3..0eb8ffe2 100644 --- a/cmd/gh-actions-lock/check_json_golden_test.go +++ b/cmd/gh-actions-lock/check_json_golden_test.go @@ -82,6 +82,14 @@ func TestCheckCommand_JSONGolden(t *testing.T) { }), ) + // Locked repository identities. checkout uses the helper default (1). + for nwo, id := range map[string]int{"actions/setup-go": 2, "actions/cache": 3, "helper/only-transitive": 5} { + reg.Register( + httpmock.REST("GET", "^/repos/"+nwo+"$"), + httpmock.JSONResponse(map[string]any{"id": id, "full_name": nwo, "owner": map[string]any{"id": id}}), + ) + } + // Resolve the fixture path against the package directory BEFORE // chdir'ing into the tempdir — UPDATE_GOLDEN rewrites the source // expected.json, not a copy under the tempdir. diff --git a/cmd/gh-actions-lock/command_test.go b/cmd/gh-actions-lock/command_test.go index 12f369bf..87ad6b85 100644 --- a/cmd/gh-actions-lock/command_test.go +++ b/cmd/gh-actions-lock/command_test.go @@ -2,6 +2,7 @@ package main import ( "encoding/json" + "fmt" "io" "net/http" "os" @@ -128,7 +129,7 @@ func TestCheckCommand_RewritesMovedRepository(t *testing.T) { }, }), ) - if tt.name == "existing mutable lockfile" { + if len(tt.pins) > 0 { reg.Register( httpmock.REST("GET", `repos/krzema12/github-actions-typing$`), httpmock.JSONResponse(map[string]any{ @@ -155,6 +156,13 @@ jobs: steps: - uses: `+oldNWO+`@`+tt.ref+` `, tt.pins...) + if len(tt.pins) > 0 { + lockPath := filepath.Join(".github", "workflows", "actions.lock") + lock, readErr := os.ReadFile(lockPath) + require.NoError(t, readErr) + lock = []byte(strings.ReplaceAll(string(lock), "repo_id: 1\n", "repo_id: 502427408\n")) + require.NoError(t, os.WriteFile(lockPath, lock, 0o600)) + } args := append(tt.args, "--no-narrow", workflowPath) stdout, stderr, err := runCommandWithHTTP(t, reg, args...) @@ -374,7 +382,7 @@ func TestCheckCommand_VerifyRejectsKnownMoveWhenResolutionFails(t *testing.T) { defer reg.Verify(t) reg.Register( httpmock.REST("GET", `repos/old/action$`), - httpmock.JSONResponse(map[string]any{"full_name": newNWO}), + httpmock.JSONResponse(map[string]any{"full_name": newNWO, "id": 1, "owner": map[string]any{"id": 1}}), ) reg.Register( httpmock.GraphQLForRepo("old", "action"), @@ -405,6 +413,101 @@ jobs: assert.Contains(t, payload.Findings[1].Detail, oldNWO+" has been renamed or transferred to "+newNWO) } +func TestCheckCommand_RejectsUnverifiableRepositoryIdentity(t *testing.T) { + const ( + nwo = "owner/action" + ref = "v1" + sha = "1111111111111111111111111111111111111111" + ) + tests := []struct { + name string + args []string + response map[string]any + status int + category string + detail string + }{ + { + name: "verify fails closed when lookup errors", + args: []string{"--verify"}, + status: http.StatusForbidden, + category: "repository-identity-unknown", + detail: "couldn't verify the repository identity for " + nwo, + }, + { + name: "default run fails closed when lookup errors", + status: http.StatusForbidden, + category: "repository-identity-unknown", + detail: "couldn't verify the repository identity for " + nwo, + }, + { + name: "rescan fails closed when lookup errors", + args: []string{"--rescan"}, + status: http.StatusForbidden, + category: "repository-identity-unknown", + detail: "couldn't verify the repository identity for " + nwo, + }, + { + name: "transfer to a different repository ID is blocked", + response: map[string]any{"full_name": "other/action", "id": 2, "owner": map[string]any{"id": 2}}, + category: "repository-changed", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + reg := &httpmock.Registry{} + responder := httpmock.StatusResponse(tt.status) + if tt.response != nil { + responder = httpmock.JSONResponse(tt.response) + } + for range 4 { + reg.Register(httpmock.REST("GET", `repos/owner/action$`), responder) + } + workflow := ` +name: ci +on: push +jobs: + test: + runs-on: ubuntu-latest + steps: + - uses: ` + nwo + `@` + ref + ` +` + workflowPath := writeTempWorkflow(t, workflow, nwo+"@"+ref+"=sha1-"+sha) + workflowBefore, err := os.ReadFile(workflowPath) + require.NoError(t, err) + lockPath := filepath.Join(filepath.Dir(workflowPath), "actions.lock") + lockBefore, err := os.ReadFile(lockPath) + require.NoError(t, err) + + args := append(append([]string{}, tt.args...), "--no-interactive", "--json=valid,findings", workflowPath) + stdout, _, err := runCommandWithHTTP(t, reg, args...) + + require.Error(t, err) + var payload struct { + Valid bool `json:"valid"` + Findings []format.Finding `json:"findings"` + } + require.NoError(t, json.Unmarshal([]byte(stdout), &payload), stdout) + assert.False(t, payload.Valid) + var found *format.Finding + for i := range payload.Findings { + if payload.Findings[i].Category == tt.category { + found = &payload.Findings[i] + } + } + require.NotNil(t, found, "findings: %+v", payload.Findings) + assert.Contains(t, found.Detail, tt.detail) + + gotWorkflow, err := os.ReadFile(workflowPath) + require.NoError(t, err) + assert.Equal(t, string(workflowBefore), string(gotWorkflow)) + lockAfter, err := os.ReadFile(lockPath) + require.NoError(t, err) + assert.Equal(t, string(lockBefore), string(lockAfter)) + }) + } +} + func TestCheckCommand_FixRejectsKnownMoveWhenResolutionFails(t *testing.T) { const ( oldNWO = "old/action" @@ -416,7 +519,7 @@ func TestCheckCommand_FixRejectsKnownMoveWhenResolutionFails(t *testing.T) { defer reg.Verify(t) reg.Register( httpmock.REST("GET", `repos/old/action$`), - httpmock.JSONResponse(map[string]any{"full_name": newNWO}), + httpmock.JSONResponse(map[string]any{"full_name": newNWO, "id": 1, "owner": map[string]any{"id": 1}}), ) for range 2 { reg.Register( @@ -545,11 +648,11 @@ func TestCheckCommand_RejectsTransferredRecordedRemoteCompositeRef(t *testing.T) defer reg.Verify(t) reg.Register( httpmock.REST("GET", `repos/root/composite$`), - httpmock.JSONResponse(map[string]any{"full_name": parentNWO}), + httpmock.JSONResponse(map[string]any{"full_name": parentNWO, "id": 1, "owner": map[string]any{"id": 1}}), ) reg.Register( httpmock.REST("GET", `repos/old/action$`), - httpmock.JSONResponse(map[string]any{"full_name": newNWO}), + httpmock.JSONResponse(map[string]any{"full_name": newNWO, "id": 2, "owner": map[string]any{"id": 2}}), ) reg.Register( httpmock.GraphQLForRepo("root", "composite"), @@ -815,6 +918,32 @@ func readTempLockfilePins(t *testing.T) string { return string(b) } +// lockfileIdentityTransport serves repository metadata matching the +// owner_id/repo_id 1 that writeTempLockfile records, so every pin/update can +// verify locked repository identity. Stubs registered by a test take +// precedence. +type lockfileIdentityTransport struct { + next http.RoundTripper +} + +func (t lockfileIdentityTransport) RoundTrip(req *http.Request) (*http.Response, error) { + resp, err := t.next.RoundTrip(req) + if err == nil || !strings.Contains(err.Error(), "no registered HTTP stubs matched") { + return resp, err + } + parts := strings.Split(strings.Trim(req.URL.Path, "/"), "/") + if req.Method != http.MethodGet || len(parts) != 3 || parts[0] != "repos" { + return resp, err + } + body := fmt.Sprintf(`{"id":1,"full_name":%q,"default_branch":"main","owner":{"id":1}}`, parts[1]+"/"+parts[2]) + return &http.Response{ + StatusCode: http.StatusOK, + Header: http.Header{"Content-Type": []string{"application/json"}}, + Body: io.NopCloser(strings.NewReader(body)), + Request: req, + }, nil +} + func runCommandWithHTTP(t *testing.T, rt http.RoundTripper, args ...string) (string, string, error) { t.Helper() @@ -824,7 +953,7 @@ func runCommandWithHTTP(t *testing.T, rt http.RoundTripper, args ...string) (str require.NoError(t, err) newResolver := func(hostname string, pool *pinpool.Pool) (*resolve.Resolver, error) { - return resolve.New(hostname, pool, resolve.WithTransport(rt)) + return resolve.New(hostname, pool, resolve.WithTransport(lockfileIdentityTransport{rt})) } cmd := newRootCmd(newResolver) diff --git a/cmd/gh-actions-lock/format/terminal.go b/cmd/gh-actions-lock/format/terminal.go index 039dbf4a..244e1bea 100644 --- a/cmd/gh-actions-lock/format/terminal.go +++ b/cmd/gh-actions-lock/format/terminal.go @@ -186,6 +186,8 @@ func categoryLabel(c checks.Category) string { return "Unreachable pin" case checks.RepositoryChanged: return "Repository identity changed" + case checks.RepositoryIdentityUnknown: + return "Repository identity unverified" case checks.Stale: return "Unused lockfile entry" } @@ -252,7 +254,7 @@ func renderErrorFindings(out *ui.UI, report *checks.Report, failedCount, checked parts := []string{} for _, cat := range []checks.Category{ checks.UnreachablePin, - checks.RepositoryChanged, + checks.RepositoryChanged, checks.RepositoryIdentityUnknown, checks.RefChanged, checks.NotPinned, checks.OnboardingRequired, checks.LocalAction, checks.InvalidSelfRepositoryRef, checks.Stale, checks.MisleadingSHA, @@ -427,7 +429,7 @@ func renderWarnings(out *ui.UI, report *checks.Report, willRemediate bool) { // remediator should not re-print it in non-interactive mode). func IsAlertedCategory(c checks.Category) bool { switch c { - case checks.UnreachablePin, checks.MisleadingSHA, checks.RepositoryChanged, checks.OnboardingRequired: + case checks.UnreachablePin, checks.MisleadingSHA, checks.RepositoryChanged, checks.RepositoryIdentityUnknown, checks.OnboardingRequired: return true } return false diff --git a/cmd/gh-actions-lock/proxima_test.go b/cmd/gh-actions-lock/proxima_test.go index 6b7d234e..272260a1 100644 --- a/cmd/gh-actions-lock/proxima_test.go +++ b/cmd/gh-actions-lock/proxima_test.go @@ -37,7 +37,7 @@ func proximaFixture(t *testing.T, publicStatus int) http.RoundTripper { switch { case tenant && path == "/repos/tenant/internal": return httpmock.JSONResponse(map[string]any{ - "visibility": "internal", "id": 111, "owner": map[string]any{"id": 11}, + "full_name": "tenant/internal", "visibility": "internal", "id": 111, "owner": map[string]any{"id": 11}, })(req) case path == "/repos/actions/public": if tenant { @@ -47,7 +47,7 @@ func proximaFixture(t *testing.T, publicStatus int) http.RoundTripper { return httpmock.StatusResponse(publicStatus)(req) } return httpmock.JSONResponse(map[string]any{ - "visibility": "public", "id": 222, "owner": map[string]any{"id": 22}, + "full_name": "actions/public", "visibility": "public", "id": 222, "owner": map[string]any{"id": 22}, })(req) case tenant && path == "/graphql": var body struct { diff --git a/cmd/gh-actions-lock/selfrepository_test.go b/cmd/gh-actions-lock/selfrepository_test.go index 506b5b73..e7010259 100644 --- a/cmd/gh-actions-lock/selfrepository_test.go +++ b/cmd/gh-actions-lock/selfrepository_test.go @@ -5,19 +5,28 @@ import ( "net/http" "os" "path/filepath" + "strings" "sync/atomic" "testing" + "github.com/github/gh-actions-lock/internal/ghapi/httpmock" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) type requestCountingTransport struct { calls atomic.Int64 + // repoIDs serves repository metadata for these owner/name values. + repoIDs map[string]int64 } -func (t *requestCountingTransport) RoundTrip(*http.Request) (*http.Response, error) { +func (t *requestCountingTransport) RoundTrip(req *http.Request) (*http.Response, error) { t.calls.Add(1) + nwo := strings.TrimPrefix(req.URL.Path, "/repos/") + if id, ok := t.repoIDs[nwo]; ok { + return httpmock.JSONResponse(map[string]any{"full_name": nwo, "id": id, "owner": map[string]any{"id": 44036562}})(req) + } return nil, errors.New("unexpected HTTP request") } @@ -97,7 +106,11 @@ jobs: func TestExistingSHARefRewritesSelfRepositoryAction(t *testing.T) { const sha = "bcd2ba49218906704ab6c1aa796996da409d3eb1" - transport := &requestCountingTransport{} + transport := &requestCountingTransport{repoIDs: map[string]int64{ + "actions/create-github-app-token": 642580244, + "actions/checkout": 197814629, + "actions/setup-go": 485264523, + }} dir := t.TempDir() require.NoError(t, os.Mkdir(filepath.Join(dir, ".git"), 0o755)) diff --git a/internal/pin/plan.go b/internal/pin/plan.go index c535d66b..d6b0625b 100644 --- a/internal/pin/plan.go +++ b/internal/pin/plan.go @@ -143,7 +143,7 @@ func planWorkflow(ctx context.Context, wr checks.WorkflowReport, opts PlanOption if finding.Category == checks.InvalidSelfRepositoryRef { return planResult{}, nil } - if finding.Category == checks.RepositoryChanged { + if finding.Category == checks.RepositoryChanged || finding.Category == checks.RepositoryIdentityUnknown { return planResult{}, fmt.Errorf("%s; %s", finding.Detail, finding.Remediation) } } diff --git a/internal/pipeline/checks/category.go b/internal/pipeline/checks/category.go index 45ced40e..f0b03021 100644 --- a/internal/pipeline/checks/category.go +++ b/internal/pipeline/checks/category.go @@ -36,6 +36,11 @@ const ( // RepositoryChanged means the repository at a locked name has a different // numeric repository ID and is therefore not the repository that was pinned. RepositoryChanged Category = "repository-changed" + // RepositoryIdentityUnknown means the current identity of a locked + // repository could not be fetched. Unlike the other unknown categories it + // blocks: the lockfile cannot be trusted or updated without proving the + // repository is still the one that was pinned. + RepositoryIdentityUnknown Category = "repository-identity-unknown" // Valid means the dependency is pinned and verified. Valid Category = "valid" // RunOnly means the workflow has no action refs (only run: diff --git a/internal/pipeline/checks/category_test.go b/internal/pipeline/checks/category_test.go index f2d25420..242195be 100644 --- a/internal/pipeline/checks/category_test.go +++ b/internal/pipeline/checks/category_test.go @@ -19,6 +19,7 @@ func TestCategoryStringsAreFrozen(t *testing.T) { {MisleadingSHA, "misleading-sha"}, {UnreachablePin, "unreachable-pin"}, {RepositoryChanged, "repository-changed"}, + {RepositoryIdentityUnknown, "repository-identity-unknown"}, {Valid, "valid"}, {RunOnly, "run-only"}, {AncestryUnknown, "ancestry-unknown"}, @@ -51,7 +52,7 @@ func TestCategoryIsInconclusive(t *testing.T) { } blocking := []Category{ NotPinned, ShaAsRef, RefChanged, RefMoved, Stale, - MisleadingSHA, UnreachablePin, RepositoryChanged, + MisleadingSHA, UnreachablePin, RepositoryChanged, RepositoryIdentityUnknown, Valid, RunOnly, OnboardingRequired, VersionRef, LocalAction, StaleWorkflow, SelfRepositoryAction, InvalidSelfRepositoryRef, diff --git a/internal/pipeline/doc_urls.go b/internal/pipeline/doc_urls.go index f7e6ed6d..bfc0532a 100644 --- a/internal/pipeline/doc_urls.go +++ b/internal/pipeline/doc_urls.go @@ -29,17 +29,18 @@ const PublisherTagReleasesDocURL = "https://docs.github.com/en/actions/how-tos/c const PublisherEscalationCopy = "Ask the action maintainer to tag releases from a branch" var docURLs = map[checks.Category]string{ - checks.NotPinned: securityHardeningBase + "#using-third-party-actions", - checks.ShaAsRef: securityHardeningBase + "#using-third-party-actions", - checks.RefChanged: securityHardeningBase + "#using-third-party-actions", - checks.Stale: securityHardeningBase + "#using-third-party-actions", - checks.MisleadingSHA: securityHardeningBase + "#using-third-party-actions", - checks.RefMoved: securityHardeningBase + "#using-third-party-actions", - checks.UnreachablePin: securityHardeningBase + "#using-third-party-actions", - checks.RepositoryChanged: securityHardeningBase + "#using-third-party-actions", - checks.OnboardingRequired: securityHardeningBase + "#using-third-party-actions", - checks.AncestryUnknown: securityHardeningBase + "#using-third-party-actions", - checks.ReachabilityUnknown: securityHardeningBase + "#using-third-party-actions", + checks.NotPinned: securityHardeningBase + "#using-third-party-actions", + checks.ShaAsRef: securityHardeningBase + "#using-third-party-actions", + checks.RefChanged: securityHardeningBase + "#using-third-party-actions", + checks.Stale: securityHardeningBase + "#using-third-party-actions", + checks.MisleadingSHA: securityHardeningBase + "#using-third-party-actions", + checks.RefMoved: securityHardeningBase + "#using-third-party-actions", + checks.UnreachablePin: securityHardeningBase + "#using-third-party-actions", + checks.RepositoryChanged: securityHardeningBase + "#using-third-party-actions", + checks.RepositoryIdentityUnknown: securityHardeningBase + "#using-third-party-actions", + checks.OnboardingRequired: securityHardeningBase + "#using-third-party-actions", + checks.AncestryUnknown: securityHardeningBase + "#using-third-party-actions", + checks.ReachabilityUnknown: securityHardeningBase + "#using-third-party-actions", } // DocURLFor returns the documentation URL for a finding category, or "" diff --git a/internal/pipeline/run.go b/internal/pipeline/run.go index e0267467..a512583f 100644 --- a/internal/pipeline/run.go +++ b/internal/pipeline/run.go @@ -78,7 +78,7 @@ func Run(ctx context.Context, opts RunOptions) (*RunResult, error) { len(parsed[i].SelfRepositoryRefErrs) == 0 && len(parsed[i].SelfRepositoryResolutionErrs) == 0 { fastPlans[i] = planFastPath(parsed[i]) - identityRefs[i] = repositoryIdentityRefs(parsed[i].Path, lockSnapshot, homeHostname) + identityRefs[i] = repositoryIdentityRefs(parsed[i], lockSnapshot, homeHostname) } } repositoryIdentities := lookupRepositoryIdentities(ctx, r, opts.Pool, identityRefs) @@ -206,7 +206,13 @@ type repositoryIdentityRef struct { RepoID int64 } -func repositoryIdentityRefs(path string, file parserlock.File, homeHostname string) []repositoryIdentityRef { +func repositoryIdentityRefs(pw checks.ParsedWorkflow, file parserlock.File, homeHostname string) []repositoryIdentityRef { + // Only verify roots the workflow still uses. A stale root (for example + // one whose repository was deleted) must not block the run that prunes it. + current := make(map[ghapi.NWORef]bool, len(pw.Refs)) + for _, ref := range pw.Refs { + current[ghapi.ForNWORef(ref.Owner, ref.Repo, ref.Ref)] = true + } var refs []repositoryIdentityRef index := make(map[ghapi.NWORef]int) add := func(ref parserlock.ActionRef, hostname, parent string, repoID int64) { @@ -247,7 +253,10 @@ func repositoryIdentityRefs(path string, file parserlock.File, homeHostname stri walk(child, pinKey) } } - for _, root := range file.Workflows[workflowfile.KeyFromPath(path)] { + for _, root := range file.Workflows[workflowfile.KeyFromPath(pw.Path)] { + if pin, ok := parserlock.ParsePin(root); !ok || !current[ghapi.ForNWORef(pin.Owner, pin.Repo, pin.Ref)] { + continue + } walk(root, "") } return refs @@ -256,6 +265,7 @@ func repositoryIdentityRefs(path string, file parserlock.File, homeHostname stri type repositoryIdentity struct { canonical string repoID int64 + err error } func (i repositoryIdentity) matches(nwo string, repoID int64) bool { @@ -302,14 +312,23 @@ func lookupRepositoryIdentities(ctx context.Context, r *resolve.Resolver, pool * func(indexedRef) string { return "" }, func(ctx context.Context, _ int, item indexedRef) error { ref := item.ref.Ref + // RepoIDs rejects a client routed to a host other than the + // one the lockfile records; CanonicalNWO then reads the same + // cached metadata from that client. + _, repoID, err := r.RepoIDs(ctx, item.ref.Hostname, ref.Owner, ref.Repo) + if err != nil { + results[item.idx].err = err + return nil + } canonical, err := r.CanonicalNWO(ctx, ref.Owner, ref.Repo) - if err == nil { - results[item.idx].canonical = canonical - _, repoID, idErr := r.RepoIDs(ctx, item.ref.Hostname, ref.Owner, ref.Repo) - if idErr == nil { - results[item.idx].repoID = repoID - } + if err == nil && canonical == "" { + err = fmt.Errorf("repos/%s returned no canonical name", ref.NWO()) + } + if err != nil { + results[item.idx].err = err + return nil } + results[item.idx] = repositoryIdentity{canonical: canonical, repoID: repoID} return nil }, ) @@ -327,21 +346,34 @@ func appendKnownRepositoryIdentityFindings(report *checks.Report, workflows [][] for _, item := range workflows[i] { ref := item.Ref identity := identities[repositoryIdentityKey(item.Hostname, ref)] + if identity.err != nil { + wr.Findings = append(wr.Findings, checks.Finding{ + WorkflowPath: wr.Path, + Category: checks.RepositoryIdentityUnknown, + Severity: checks.SeverityError, + Confidence: checks.ConfidenceHigh, + ActionRef: &ref, + Detail: fmt.Sprintf("couldn't verify the repository identity for %s: %v", ref.NWO(), identity.err), + Remediation: fmt.Sprintf("check that %s is reachable with your credentials, then run `gh actions-lock` again", ref.NWO()), + }) + continue + } if identity.canonical == "" { continue } + if item.RepoID != 0 && identity.repoID != item.RepoID { + wr.Findings = append(wr.Findings, checks.Finding{ + WorkflowPath: wr.Path, + Category: checks.RepositoryChanged, + Severity: checks.SeverityError, + Confidence: checks.ConfidenceHigh, + ActionRef: &ref, + Detail: fmt.Sprintf("repository identity changed for %s: the lockfile records repository ID %d, but the current repository ID is %d. This may indicate a namespace takeover", ref.NWO(), item.RepoID, identity.repoID), + Remediation: fmt.Sprintf("review %s before trusting it. If the replacement is expected, remove its lockfile entry and run `gh actions-lock` again", ref.NWO()), + }) + continue + } if strings.EqualFold(identity.canonical, ref.NWO()) { - if item.RepoID != 0 && identity.repoID != 0 && identity.repoID != item.RepoID { - wr.Findings = append(wr.Findings, checks.Finding{ - WorkflowPath: wr.Path, - Category: checks.RepositoryChanged, - Severity: checks.SeverityError, - Confidence: checks.ConfidenceHigh, - ActionRef: &ref, - Detail: fmt.Sprintf("repository identity changed for %s: the lockfile records repository ID %d, but the current repository ID is %d. This may indicate a namespace takeover", ref.NWO(), item.RepoID, identity.repoID), - Remediation: fmt.Sprintf("review %s before trusting it. If the replacement is expected, remove its lockfile entry and run `gh actions-lock` again", ref.NWO()), - }) - } continue } if resolvedTransfer(wr.ResolvedDeps, ref) { diff --git a/internal/pipeline/run_test.go b/internal/pipeline/run_test.go index 840e142e..611b6ea4 100644 --- a/internal/pipeline/run_test.go +++ b/internal/pipeline/run_test.go @@ -94,15 +94,25 @@ func TestPlanFastPath(t *testing.T) { func TestRepositoryIdentityRefsIncludesLockedClosure(t *testing.T) { file := parserlock.File{ Workflows: map[string][]string{ - ".github/workflows/ci.yml": {"root/composite@v1", "other/action@v1"}, + ".github/workflows/ci.yml": {"root/composite@v1", "other/action@v1", "deleted/action@v1"}, }, Dependencies: map[string]parserlock.Action{ "root/composite@v1": {RepoID: 10, Uses: []string{"old/action@v1"}}, "old/action@v1": {RepoID: 20}, "other/action@v1": {RepoID: 30}, + "deleted/action@v1": {RepoID: 40}, }, } - got := repositoryIdentityRefs(".github/workflows/ci.yml", file, "") + // deleted/action is a stale lockfile root the workflow no longer uses; it + // must not be looked up, or a deleted repository would block its pruning. + pw := checks.ParsedWorkflow{ + Path: ".github/workflows/ci.yml", + Refs: []parserlock.ActionRef{ + ref("root", "composite", "", "v1"), + ref("other", "action", "", "v1"), + }, + } + got := repositoryIdentityRefs(pw, file, "") assert.Len(t, got, 3) assert.Equal(t, "root/composite", got[0].Ref.NWO()) diff --git a/test/integration/run.rb b/test/integration/run.rb index a0a2a1ac..942a177a 100644 --- a/test/integration/run.rb +++ b/test/integration/run.rb @@ -90,6 +90,17 @@ def sso_403_all(srv) end end +# SSO 403 for everything except the actions/checkout identity lookup, so +# scenarios can exercise inconclusive resolution without failing closed on +# repository identity. +def sso_403_with_checkout_identity(srv) + srv.on(:GET, %r{/repos/actions/checkout$}) do |_req| + [200, { "Content-Type" => "application/json" }, + JSON.generate({ full_name: "actions/checkout", id: 197_814_629, owner: { id: 44_036_562 } })] + end + sso_403_all(srv) +end + # Register a catch-all that returns the given status with a JSON body. def error_all(srv, status, body) [:GET, :POST].each do |method| @@ -501,6 +512,7 @@ def checkout_repo_rest(srv) srv.on(:GET, %r{/repos/actions/checkout$}) do |_req| [200, { "Content-Type" => "application/json" }, JSON.generate({ + full_name: "actions/checkout", default_branch: "main", visibility: "public", pushed_at: "2024-01-01T00:00:00Z", @@ -706,6 +718,14 @@ def wire_checkout_fresh(s, token) end s.env("GH_TOKEN" => "gho_fake_dependabot_proxy_token") }, + verify_pinned_stub: ->(s) { + s.stub_server { |srv| sso_403_with_checkout_identity(srv) } + s.env("GH_TOKEN" => "gho_fake_verify_token") + }, + verify_json_combined: ->(s) { + s.stub_server { |srv| sso_403_with_checkout_identity(srv) } + s.env("GH_TOKEN" => "gho_fake_verify_json_token") + }, sso_dedup: ->(s) { s.stub_server { |srv| sso_403_all(srv) } s.env("GH_TOKEN" => "gho_fake_sso_test_token") diff --git a/test/scenarios/catalog.yml b/test/scenarios/catalog.yml index 2378f1b7..5deb98d8 100644 --- a/test/scenarios/catalog.yml +++ b/test/scenarios/catalog.yml @@ -708,7 +708,7 @@ scenarios: - name: verify_pinned_stub category: output_modes - description: "--verify with stub server — pinned lockfile passes (inconclusive is non-blocking in read-only mode)" + description: "--verify with stub server — pinned lockfile passes when identity verifies (inconclusive ancestry is non-blocking in read-only mode)" needs_stub: true tags: [stub] flags: ["--verify"] @@ -1958,7 +1958,7 @@ scenarios: valid: true - name: dbot_transient_403_drops_pin category: dependabot - description: "SSO 403 on a previously-pinned action — pin retained with an inconclusive warning" + description: "SSO 403 on a previously-pinned action — identity can't be verified, so the run fails closed and the lockfile is left untouched" needs_stub: true tags: [stub] flags: ["--no-onboard", "--no-narrow", "--no-interactive", "--json=valid,findings"] @@ -1969,16 +1969,14 @@ scenarios: actions: ["actions/checkout@v4"] lockfile_template: pinned_checkout expect: - exit: 0 + exit: 2 lockfile_deps_cover_direct: true stdout_is_json: true jq: - expr: '.valid' - equals: "true" - - expr: '.findings | length' + equals: "false" + - expr: '[.findings[] | select(.category == "repository-identity-unknown")] | length' equals: "1" - - expr: '.findings[0].category' - equals: "reachability-unknown" lockfile_contains: - "version: 'v0.0.2'" From 71845b7722d26ddedd2c0eea6ded0f071dc6a5ba Mon Sep 17 00:00:00 2001 From: Jeff Martin Date: Fri, 9 Oct 2026 10:02:23 -0700 Subject: [PATCH 3/4] Verify legacy host bindings by repository ID only A transfer changes owner_id while the repository keeps its identity, so comparing owner_id rejected transferred dependencies before the transfer rewrite could run. --- cmd/gh-actions-lock/proxima_test.go | 2 +- internal/lockfile/hosts_test.go | 24 ++++++++++++------------ internal/lockfile/state.go | 8 +++++--- 3 files changed, 18 insertions(+), 16 deletions(-) diff --git a/cmd/gh-actions-lock/proxima_test.go b/cmd/gh-actions-lock/proxima_test.go index 272260a1..37084618 100644 --- a/cmd/gh-actions-lock/proxima_test.go +++ b/cmd/gh-actions-lock/proxima_test.go @@ -159,7 +159,7 @@ jobs: "--hostname", "tenant.ghe.com", "--no-narrow", "--no-migrate-local-actions", "--json", path) require.Error(t, err) if status == 200 { - assert.ErrorContains(t, err, "does not match its tenant.ghe.com repository IDs") + assert.ErrorContains(t, err, "does not match its tenant.ghe.com repository ID") } else { assert.ErrorContains(t, err, "verifying repository identity") } diff --git a/internal/lockfile/hosts_test.go b/internal/lockfile/hosts_test.go index b6294c6a..0aa43c3b 100644 --- a/internal/lockfile/hosts_test.go +++ b/internal/lockfile/hosts_test.go @@ -134,18 +134,18 @@ func TestLegacyAndOmittedHostnames(t *testing.T) { assert.Contains(t, string(raw), "hostname: 'github.com'") } action := store.file.Dependencies["o/r@v1"] - for _, field := range []string{"owner", "repo"} { - t.Run("rejects changed "+field+" ID", func(t *testing.T) { - changed := action - if field == "owner" { - changed.OwnerID = 200 - } else { - changed.RepoID = 200 - } - store.file.Dependencies["o/r@v1"] = changed - require.ErrorContains(t, store.VerifyHosts(context.Background()), "regenerate the lockfile") - }) - } + t.Run("accepts changed owner ID", func(t *testing.T) { + changed := action + changed.OwnerID = 200 + store.file.Dependencies["o/r@v1"] = changed + require.NoError(t, store.VerifyHosts(context.Background())) + }) + t.Run("rejects changed repo ID", func(t *testing.T) { + changed := action + changed.RepoID = 200 + store.file.Dependencies["o/r@v1"] = changed + require.ErrorContains(t, store.VerifyHosts(context.Background()), "regenerate the lockfile") + }) }) } } diff --git a/internal/lockfile/state.go b/internal/lockfile/state.go index 6ad6fce0..63a7eba7 100644 --- a/internal/lockfile/state.go +++ b/internal/lockfile/state.go @@ -227,12 +227,14 @@ func (s *State) VerifyHosts(ctx context.Context) error { return fmt.Errorf("invalid dependency %q", key) } hostname := s.hostOrHome(action.Hostname) - ownerID, repoID, err := s.meta.RepoIDs(ctx, hostname, pin.Owner, pin.Repo) + // owner_id is compatibility metadata: a transfer changes it while the + // repository keeps its identity. + _, repoID, err := s.meta.RepoIDs(ctx, hostname, pin.Owner, pin.Repo) if err != nil { return fmt.Errorf("verifying repository identity for %s on %s: %w", key, hostname, err) } - if ownerID != action.OwnerID || repoID != action.RepoID { - return fmt.Errorf("dependency %s does not match its %s repository IDs; restore a lockfile for this host or regenerate the lockfile after reviewing the dependency", key, hostname) + if repoID != action.RepoID { + return fmt.Errorf("dependency %s does not match its %s repository ID; restore a lockfile for this host or regenerate the lockfile after reviewing the dependency", key, hostname) } } return nil From d54793da441d90764f0fa4c3171ed09d4e1ae20f Mon Sep 17 00:00:00 2001 From: Jeff Martin Date: Fri, 9 Oct 2026 10:07:46 -0700 Subject: [PATCH 4/4] Verify identities for pins carried over by renamed workflows Removing the global lockfile cache seed meant a renamed workflow's pins, which survive pruning but are not listed under the new key, were re-resolved live. Carry mutable pins over through the same repository identity check and seed only those that pass. --- cmd/gh-actions-lock/prune_workflow_test.go | 4 +-- internal/pipeline/run.go | 39 ++++++++++++++++++++-- internal/pipeline/run_test.go | 24 +++++++++++++ 3 files changed, 62 insertions(+), 5 deletions(-) diff --git a/cmd/gh-actions-lock/prune_workflow_test.go b/cmd/gh-actions-lock/prune_workflow_test.go index 2f4c9a26..56f403a8 100644 --- a/cmd/gh-actions-lock/prune_workflow_test.go +++ b/cmd/gh-actions-lock/prune_workflow_test.go @@ -288,7 +288,7 @@ func TestProximaPrunesBeforeVerifyingHosts(t *testing.T) { if tt.mismatch { id = 2 } - return httpmock.JSONResponse(map[string]any{"id": id, "owner": map[string]any{"id": 1}})(req) + return httpmock.JSONResponse(map[string]any{"full_name": "actions/checkout", "id": id, "owner": map[string]any{"id": 1}})(req) case "/repos/actions/setup-go": staleCalls++ return httpmock.StatusResponse(http.StatusNotFound)(req) @@ -308,7 +308,7 @@ func TestProximaPrunesBeforeVerifyingHosts(t *testing.T) { require.NoError(t, readErr) assert.Equal(t, before, string(after)) if tt.mismatch { - assert.ErrorContains(t, err, "does not match its tenant.ghe.com repository IDs") + assert.ErrorContains(t, err, "does not match its tenant.ghe.com repository ID") assert.Zero(t, staleCalls) } else { assert.ErrorContains(t, err, "verifying repository identity") diff --git a/internal/pipeline/run.go b/internal/pipeline/run.go index a512583f..b9acaf61 100644 --- a/internal/pipeline/run.go +++ b/internal/pipeline/run.go @@ -73,11 +73,26 @@ func Run(ctx context.Context, opts RunOptions) (*RunResult, error) { if r != nil { homeHostname = r.Hostname() } + lockDeps := make(map[string]dep.Dependency) + if opts.Store != nil { + for _, d := range opts.Store.AllDeps() { + lockDeps[strings.ToLower(d.NWO)+"@"+d.Ref] = d + } + } for i := range parsed { if len(parsed[i].LocalPaths) == 0 && len(parsed[i].SelfRepositoryRefErrs) == 0 && len(parsed[i].SelfRepositoryResolutionErrs) == 0 { fastPlans[i] = planFastPath(parsed[i]) + // A renamed workflow has no entry under its new key, but its + // pins survive pruning. Carry those mutable pins over through the + // same identity check instead of silently re-resolving them. + _, unrecorded := parsed[i].PartitionRefs() + for _, ref := range unrecorded { + if _, ok := lockDeps[lockDepKey(ref)]; ok && !checks.IsImmutableRef(ref.Ref) { + fastPlans[i].mutableRefs = append(fastPlans[i].mutableRefs, ref) + } + } identityRefs[i] = repositoryIdentityRefs(parsed[i], lockSnapshot, homeHostname) } } @@ -127,10 +142,11 @@ func Run(ctx context.Context, opts RunOptions) (*RunResult, error) { // Seed only the mutable recorded deps so they resolve from // the lockfile (trusted); immutable and unrecorded refs are // left to resolve live from the network. - rd := parsed[i].RecordedDeps(plan.mutableRefs) - seedDeps = append(seedDeps, rd...) for _, rr := range plan.mutableRefs { - recordedKeys[strings.ToLower(rr.Owner+"/"+rr.Repo)+"@"+rr.Ref] = true + if d, ok := lockDeps[lockDepKey(rr)]; ok { + seedDeps = append(seedDeps, d) + } + recordedKeys[lockDepKey(rr)] = true } } @@ -259,6 +275,19 @@ func repositoryIdentityRefs(pw checks.ParsedWorkflow, file parserlock.File, home } walk(root, "") } + // Pins carried over from a renamed workflow are not listed under the + // current key; verify them too. + pins := make(map[ghapi.NWORef]string, len(file.Dependencies)) + for raw := range file.Dependencies { + if pin, ok := parserlock.ParsePin(raw); ok { + pins[ghapi.ForNWORef(pin.Owner, pin.Repo, pin.Ref)] = raw + } + } + for _, ref := range pw.Refs { + if raw, ok := pins[ghapi.ForNWORef(ref.Owner, ref.Repo, ref.Ref)]; ok { + walk(raw, "") + } + } return refs } @@ -283,6 +312,10 @@ func lockedRepositoryIdentity(refs []repositoryIdentityRef, ref parserlock.Actio return repositoryIdentityRef{Ref: ref} } +func lockDepKey(ref parserlock.ActionRef) string { + return strings.ToLower(ref.Owner+"/"+ref.Repo) + "@" + ref.Ref +} + func repositoryIdentityKey(hostname string, ref parserlock.ActionRef) string { return strings.ToLower(hostname + "/" + ref.NWO()) } diff --git a/internal/pipeline/run_test.go b/internal/pipeline/run_test.go index 611b6ea4..096c6ede 100644 --- a/internal/pipeline/run_test.go +++ b/internal/pipeline/run_test.go @@ -126,6 +126,30 @@ func TestRepositoryIdentityRefsIncludesLockedClosure(t *testing.T) { assert.Empty(t, got[2].Parent) } +func TestRepositoryIdentityRefsIncludesRenamedWorkflowPins(t *testing.T) { + file := parserlock.File{ + Workflows: map[string][]string{ + ".github/workflows/old.yml": {"root/composite@v1"}, + }, + Dependencies: map[string]parserlock.Action{ + "root/composite@v1": {RepoID: 10, Uses: []string{"old/action@v1"}}, + "old/action@v1": {RepoID: 20}, + }, + } + pw := checks.ParsedWorkflow{ + Path: ".github/workflows/new.yml", + Refs: []parserlock.ActionRef{ref("root", "composite", "", "v1")}, + } + got := repositoryIdentityRefs(pw, file, "") + + assert.Len(t, got, 2) + assert.Equal(t, "root/composite", got[0].Ref.NWO()) + assert.EqualValues(t, 10, got[0].RepoID) + assert.Empty(t, got[0].Parent) + assert.Equal(t, "old/action", got[1].Ref.NWO()) + assert.Equal(t, "root/composite@v1", got[1].Parent) +} + func TestPartitionRefs(t *testing.T) { tests := []struct { name string