diff --git a/cmd/gh-actions-lock/command_test.go b/cmd/gh-actions-lock/command_test.go index 11cc69a0..f3ce3387 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" @@ -67,6 +68,458 @@ 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, _, err := runCommandWithHTTP(t, reg, args...) + + require.NoError(t, err, "stdout:\n%s", stdout) + 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_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": 2, + "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) +} + const nodeActionYAML = "name: Test Action\nruns:\n using: node20\n" func testRepoResponse(nameWithOwner, oid, actionYAML string) map[string]any { @@ -652,9 +1105,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) @@ -662,8 +1115,12 @@ 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"}), + ) + // 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{ @@ -716,7 +1173,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 818bee11..ae9965b8 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) @@ -436,8 +436,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/run.go b/cmd/gh-actions-lock/run.go index 0b7a3d32..ffd5ee48 100644 --- a/cmd/gh-actions-lock/run.go +++ b/cmd/gh-actions-lock/run.go @@ -176,18 +176,9 @@ func runCheck(cmd *cobra.Command, opts *checkOptions, newResolver resolverFunc) if err != nil { return err } - // Pre-warm resolver caches from the lockfile so repeat runs skip - // redundant GraphQL and REST calls. Skipped when --rescan is set: - // a full re-verification must hit the network to detect ref movement. - // --accept-moved and --relock both imply --rescan (must detect what - // moved before re-pinning). if opts.acceptMoved || opts.relock { opts.rescan = true } - trustLockfileCaches := !opts.rescan - if trustLockfileCaches { - r.SeedFromLockfile(store.AllDeps()) - } endSetup() opts.workflowPaths = paths diff --git a/internal/dep/dependency.go b/internal/dep/dependency.go index 86dc7f03..122448b5 100644 --- a/internal/dep/dependency.go +++ b/internal/dep/dependency.go @@ -15,7 +15,8 @@ import ( // resolver traversal, and lockfile serialization — never persisted on disk // and not part of any public API. type Dependency struct { - NWO string // owner/repo (no path) + 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 @@ -75,14 +76,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 f64df865..e2cee1cc 100644 --- a/internal/ghapi/graphql_action_files.go +++ b/internal/ghapi/graphql_action_files.go @@ -27,13 +27,14 @@ 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 { - Owner string - Repo string - Path string - Ref string - CommitOID string - ActionYML string - Err error + 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. @@ -260,6 +261,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) @@ -289,6 +296,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 cdbf9c78..5513a23b 100644 --- a/internal/ghapi/graphql_action_files_test.go +++ b/internal/ghapi/graphql_action_files_test.go @@ -101,6 +101,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 cd2c8883..9f4ef596 100644 --- a/internal/ghapi/repos.go +++ b/internal/ghapi/repos.go @@ -126,6 +126,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 @@ -134,9 +135,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. @@ -150,6 +152,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"` @@ -169,6 +172,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, @@ -184,6 +188,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 8fa8f4d1..dd8b26b3 100644 --- a/internal/ghapi/rest_fallback.go +++ b/internal/ghapi/rest_fallback.go @@ -252,13 +252,37 @@ func (c *Client) resolveAnonymous(ctx context.Context, ref ActionFileRequest) Ac Ref: ref.Ref, } + var canonical string + 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 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) @@ -276,10 +300,10 @@ 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 { // 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 { // Not fatal — some actions don't have action.yml (reusable workflows). return result 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 6dcc3eef..0c8e502a 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 aac509e9..d51dd608 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" @@ -128,13 +129,28 @@ func planWorkflow(ctx context.Context, wr checks.WorkflowReport, opts PlanOption rewriteRefs = wr.ActionRefs } rewriteRefKeys := actionRefKeys(rewriteRefs) + 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, rewriteRefKeys) wplans = append(wplans, WorkflowPlan{Path: wr.Path, Rewrites: rw, SelfActionFiles: wr.SelfActionFiles}) @@ -168,6 +184,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, rewriteRefKeys) @@ -179,6 +199,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) + } entries = append(entries, unresolvedEntries(wr, unrecordedRefs, deps, resolveErr)...) if len(deps) == 0 { wplans = append(wplans, WorkflowPlan{Path: wr.Path, SelfActionFiles: wr.SelfActionFiles}) @@ -194,6 +222,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 patch tags, and resolve bare-SHA refs // to a symbolic tag when one exists. @@ -233,12 +266,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) @@ -267,9 +302,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 { // No rewrites and no plan entry yet — still include the workflow @@ -286,24 +322,73 @@ func planWorkflow(ctx context.Context, wr checks.WorkflowReport, opts PlanOption return planResult{entries: entries, wplans: wplans}, 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 +} + // unresolvedEntries flags findings whose refs were attempted but failed to // resolve. On a partial failure deps holds the refs that did resolve, so only // the genuine misses (attempted and not in deps) are marked Unresolved. func unresolvedEntries(wr checks.WorkflowReport, unrecordedRefs []parserlock.ActionRef, deps []dep.Dependency, resolveErr error) []Entry { - resolved := make(map[string]bool, len(deps)) + resolved := make(map[ghapi.NWORef]bool, len(deps)) for _, d := range deps { - resolved[strings.ToLower(d.NWO+"@"+d.Ref)] = true + owner, repo := d.OwnerRepo() + resolved[ghapi.ForNWORef(owner, repo, d.Ref)] = true + for _, ref := range d.OriginalRefs { + resolved[ghapi.ForNWORef(ref.Owner, ref.Repo, ref.Ref)] = true + } } - attempted := make(map[string]bool, len(unrecordedRefs)) + attempted := make(map[ghapi.NWORef]bool, len(unrecordedRefs)) for _, ref := range unrecordedRefs { - attempted[strings.ToLower(ref.Owner+"/"+ref.Repo+"@"+ref.Ref)] = true + attempted[ghapi.ForNWORef(ref.Owner, ref.Repo, ref.Ref)] = true } var out []Entry for _, f := range wr.Findings { if f.ActionRef == nil { continue } - key := strings.ToLower(f.ActionRef.Owner + "/" + f.ActionRef.Repo + "@" + f.ActionRef.Ref) + key := ghapi.ForNWORef(f.ActionRef.Owner, f.ActionRef.Repo, f.ActionRef.Ref) if !attempted[key] || resolved[key] { continue } diff --git a/internal/pin/plan_test.go b/internal/pin/plan_test.go index 2c3955f5..6b4393e8 100644 --- a/internal/pin/plan_test.go +++ b/internal/pin/plan_test.go @@ -2,9 +2,13 @@ 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" @@ -16,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) +} + // TestPlanWorkflow_PartialResolutionFailure verifies that when one ref in a // workflow fails resolution (e.g. repo not found), only the failed ref is // marked Unresolved. The successful ref proceeds through reachability and @@ -26,14 +150,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"}}, @@ -52,7 +176,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, @@ -65,7 +189,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"}, }, } @@ -95,12 +219,31 @@ func TestPlanWorkflow_PartialResolutionFailure(t *testing.T) { assert.Equal(t, "main", unresolved[0].Ref) assert.Contains(t, unresolved[0].Reason, "not found") - // good/action must be pinned (not poisoned by the bad ref). + // The redirected action must be pinned under its canonical NWO, not + // misclassified as unresolved because the sibling failed. require.Len(t, pinned, 1, "expected exactly one pinned entry") - assert.Equal(t, "good/action", pinned[0].NWO) + assert.Equal(t, "new/action", pinned[0].NWO) assert.Equal(t, goodSHA, pinned[0].SHA) } +func TestUnresolvedEntriesPreservesRefCase(t *testing.T) { + resolvedRef := parserlock.ActionRef{Owner: "old", Repo: "action", Ref: "Release"} + failedRef := parserlock.ActionRef{Owner: "old", Repo: "action", Ref: "release"} + wr := checks.WorkflowReport{Findings: []checks.Finding{ + {ActionRef: &resolvedRef, Category: checks.NotPinned}, + {ActionRef: &failedRef, Category: checks.NotPinned}, + }} + + got := unresolvedEntries(wr, []parserlock.ActionRef{resolvedRef, failedRef}, []dep.Dependency{{ + NWO: "new/action", + Ref: resolvedRef.Ref, + OriginalRefs: []parserlock.ActionRef{resolvedRef}, + }}, assert.AnError) + + require.Len(t, got, 1) + assert.Equal(t, failedRef.Ref, got[0].Ref) +} + // TestPlanWorkflow_AllResolutionsFail verifies that when ALL refs in a // workflow fail resolution, every finding is marked Unresolved and no // reachability is attempted. diff --git a/internal/pin/record.go b/internal/pin/record.go index ad32246a..02d0b199 100644 --- a/internal/pin/record.go +++ b/internal/pin/record.go @@ -48,8 +48,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/finding.go b/internal/pipeline/checks/finding.go index 48c7f6af..2d228a65 100644 --- a/internal/pipeline/checks/finding.go +++ b/internal/pipeline/checks/finding.go @@ -73,6 +73,10 @@ type WorkflowReport struct { SelfActionFiles []string // 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 337f0ffd..be41f168 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 @@ -159,6 +162,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 df2a1999..89ac2b30 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/run.go b/internal/pipeline/run.go index 46aa908a..582337c6 100644 --- a/internal/pipeline/run.go +++ b/internal/pipeline/run.go @@ -6,11 +6,13 @@ 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" "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 +58,25 @@ 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() + } + 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, fastPlans[i], lockSnapshot) + } + } + canonicalRepos := lookupRepositoryIdentities(ctx, r, opts.Pool, identityRefs) var seedDeps []dep.Dependency recordedKeys := make(map[string]bool) for i := range parsed { @@ -73,9 +91,28 @@ 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 { + canonical := canonicalRepos[ghapi.ForRepo(ref.Owner, ref.Repo)] + if canonical != "" && strings.EqualFold(canonical, ref.NWO()) { + trustedMutable = append(trustedMutable, ref) + } + } + for _, item := range identityRefs[i] { + if item.Parent == "" { + continue + } + canonical := canonicalRepos[ghapi.ForRepo(item.Ref.Owner, item.Ref.Repo)] + if canonical == "" || !strings.EqualFold(canonical, item.Ref.NWO()) { + 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 +182,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) + appendKnownTransferFindings(report, identityRefs, canonicalRepos) endDiag() valid := report.IsValid() @@ -155,6 +193,135 @@ func Run(ctx context.Context, opts RunOptions) (*RunResult, error) { }, nil } +type repositoryIdentityRef struct { + Ref parserlock.ActionRef + Parent string +} + +func repositoryIdentityRefs(path string, plan fastPathPlan, file parserlock.File) []repositoryIdentityRef { + refs := make([]repositoryIdentityRef, 0, len(plan.mutableRefs)) + index := make(map[ghapi.NWORef]int) + mutable := make(map[ghapi.NWORef]bool, len(plan.mutableRefs)) + add := func(ref parserlock.ActionRef, parent string) { + key := ghapi.ForNWORef(ref.Owner, ref.Repo, ref.Ref) + if i, ok := index[key]; ok { + if parent != "" { + refs[i].Parent = parent + } + return + } + index[key] = len(refs) + refs = append(refs, repositoryIdentityRef{Ref: ref, Parent: parent}) + } + + for _, ref := range plan.mutableRefs { + mutable[ghapi.ForNWORef(ref.Owner, ref.Repo, ref.Ref)] = true + } + 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 + } + add(parserlock.ActionRef{Owner: pin.Owner, Repo: pin.Repo, Ref: pin.Ref}, parent) + for _, child := range file.Dependencies[pinKey].Uses { + walk(child, pinKey) + } + } + for _, root := range file.Workflows[workflowfile.KeyFromPath(path)] { + pin, ok := parserlock.ParsePin(root) + if !ok || !mutable[ghapi.ForNWORef(pin.Owner, pin.Repo, pin.Ref)] { + continue + } + for _, child := range file.Dependencies[root].Uses { + walk(child, root) + } + } + for _, ref := range plan.mutableRefs { + add(ref, "") + } + return refs +} + +func lookupRepositoryIdentities(ctx context.Context, r *resolve.Resolver, pool *pinpool.Pool, workflows [][]repositoryIdentityRef) map[ghapi.Repo]string { + type indexedRef struct { + idx int + ref parserlock.ActionRef + } + var repos []indexedRef + seen := make(map[ghapi.Repo]bool) + for _, identities := range workflows { + for _, item := range identities { + ref := item.Ref + key := ghapi.ForRepo(ref.Owner, ref.Repo) + if !seen[key] { + seen[key] = true + repos = append(repos, indexedRef{idx: len(repos), ref: ref}) + } + } + } + results := make([]string, len(repos)) + if r != nil { + _ = pinpool.RunTyped(pool, ctx, "", repos, + func(indexedRef) string { return "" }, + func(ctx context.Context, _ int, item indexedRef) error { + canonical, err := r.CanonicalNWO(ctx, item.ref.Owner, item.ref.Repo) + if err == nil { + results[item.idx] = canonical + } + return nil + }, + ) + } + canonical := make(map[ghapi.Repo]string, len(repos)) + for _, item := range repos { + canonical[ghapi.ForRepo(item.ref.Owner, item.ref.Repo)] = results[item.idx] + } + return canonical +} + +func appendKnownTransferFindings(report *checks.Report, workflows [][]repositoryIdentityRef, canonicalRepos map[ghapi.Repo]string) { + for i := range report.Workflows { + wr := &report.Workflows[i] + for _, item := range workflows[i] { + ref := item.Ref + canonical := canonicalRepos[ghapi.ForRepo(ref.Owner, ref.Repo)] + if canonical == "" || strings.EqualFold(canonical, ref.NWO()) || resolvedTransfer(wr.ResolvedDeps, ref) { + continue + } + known := dep.Dependency{ + NWO: 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 +329,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..4f8c030d 100644 --- a/internal/pipeline/run_test.go +++ b/internal/pipeline/run_test.go @@ -91,6 +91,31 @@ func TestPlanFastPath(t *testing.T) { } } +func TestRepositoryIdentityRefsIncludesMutableClosureInPartialWorkflow(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": {Uses: []string{"old/action@v1"}}, + "old/action@v1": {}, + "other/action@v1": {}, + }, + } + plan := fastPathPlan{ + resolved: false, + mutableRefs: []parserlock.ActionRef{ref("root", "composite", "", "v1")}, + } + + got := repositoryIdentityRefs(".github/workflows/ci.yml", plan, file) + + assert.Len(t, got, 2) + assert.Equal(t, "old/action", got[0].Ref.NWO()) + assert.Equal(t, "root/composite@v1", got[0].Parent) + assert.Equal(t, "root/composite", got[1].Ref.NWO()) + assert.Empty(t, got[1].Parent) +} + func TestPartitionRefs(t *testing.T) { tests := []struct { name string diff --git a/internal/resolve/discovery.go b/internal/resolve/discovery.go index 0665d7c9..a812f2ba 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,11 +396,16 @@ 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{ - NWO: res[j].Owner + "/" + res[j].Repo, - Path: res[j].Path, - Ref: ref.Ref, - SHA: res[j].CommitOID, + 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 24776e06..bc2d79fe 100644 --- a/internal/resolve/resolver.go +++ b/internal/resolve/resolver.go @@ -160,6 +160,11 @@ func (r *Resolver) RepoIDs(ctx context.Context, owner, repo string) (int64, int6 return r.gh.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 de308d29..237e1ce0 100644 --- a/internal/workflowfile/rewrite.go +++ b/internal/workflowfile/rewrite.go @@ -103,6 +103,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 00391db9..375ffc68 100644 --- a/test/integration/run.rb +++ b/test/integration/run.rb @@ -347,6 +347,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" @@ -441,7 +442,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 } @@ -675,7 +676,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 396407df..3e1e389f 100644 --- a/test/scenarios/catalog.yml +++ b/test/scenarios/catalog.yml @@ -1956,7 +1956,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"] @@ -1974,7 +1974,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'" @@ -1982,11 +1984,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" @@ -2008,6 +2005,31 @@ scenarios: - expr: '.findings[0].category' equals: 'reachability-unknown' + - name: transferred_repository_rewritten + category: security + description: "Real compiled binary and GitHub API redirect an old repository NWO, rewrite writable workflow source, and emit canonical lock metadata" + needs_token: true + tags: [smoke] + flags: ["--no-narrow", "--no-interactive"] + fixtures: + workflows: + ci.yml: + name: CI + actions: ["krzema12/github-actions-typing@v2.2.2"] + expect: + exit: 0 + lockfile_exists: true + lockfile_contains: + - "'typesafegithub/github-actions-typing@v2.2.2':" + lockfile_excludes: + - "krzema12/github-actions-typing" + files_contain: + .github/workflows/ci.yml: + - "uses: typesafegithub/github-actions-typing@v2.2.2" + files_exclude: + .github/workflows/ci.yml: + - "uses: krzema12/github-actions-typing@v2.2.2" + - name: dbot_forgery_blocks category: dependabot description: "Stale pin (lockfile SHA not reachable from the ref head) produces unreachable-pin/error finding"