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 fdeba99d..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" @@ -12,6 +13,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 +87,626 @@ 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 len(tt.pins) > 0 { + 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...) + 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...) + + 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, "id": 1, "owner": map[string]any{"id": 1}}), + ) + 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_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" + 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, "id": 1, "owner": map[string]any{"id": 1}}), + ) + 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, "id": 1, "owner": map[string]any{"id": 1}}), + ) + reg.Register( + httpmock.REST("GET", `repos/old/action$`), + httpmock.JSONResponse(map[string]any{"full_name": newNWO, "id": 2, "owner": map[string]any{"id": 2}}), + ) + 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 +871,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. @@ -300,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() @@ -309,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) @@ -787,9 +1431,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 +1441,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 +1503,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..244e1bea 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,10 @@ func categoryLabel(c checks.Category) string { return "Misleading SHA" case checks.UnreachablePin: return "Unreachable pin" + case checks.RepositoryChanged: + return "Repository identity changed" + case checks.RepositoryIdentityUnknown: + return "Repository identity unverified" case checks.Stale: return "Unused lockfile entry" } @@ -250,6 +254,7 @@ func renderErrorFindings(out *ui.UI, report *checks.Report, failedCount, checked parts := []string{} for _, cat := range []checks.Category{ checks.UnreachablePin, + checks.RepositoryChanged, checks.RepositoryIdentityUnknown, checks.RefChanged, checks.NotPinned, checks.OnboardingRequired, checks.LocalAction, checks.InvalidSelfRepositoryRef, checks.Stale, checks.MisleadingSHA, @@ -424,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.OnboardingRequired: + case checks.UnreachablePin, checks.MisleadingSHA, checks.RepositoryChanged, checks.RepositoryIdentityUnknown, checks.OnboardingRequired: return true } return false @@ -436,8 +441,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/proxima_test.go b/cmd/gh-actions-lock/proxima_test.go index 6b7d234e..37084618 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 { @@ -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/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/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..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)) @@ -181,7 +194,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/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 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..d6b0625b 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 || finding.Category == checks.RepositoryIdentityUnknown { + 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..f0b03021 100644 --- a/internal/pipeline/checks/category.go +++ b/internal/pipeline/checks/category.go @@ -33,6 +33,14 @@ 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" + // 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 f1ddc200..242195be 100644 --- a/internal/pipeline/checks/category_test.go +++ b/internal/pipeline/checks/category_test.go @@ -18,6 +18,8 @@ func TestCategoryStringsAreFrozen(t *testing.T) { {Stale, "stale"}, {MisleadingSHA, "misleading-sha"}, {UnreachablePin, "unreachable-pin"}, + {RepositoryChanged, "repository-changed"}, + {RepositoryIdentityUnknown, "repository-identity-unknown"}, {Valid, "valid"}, {RunOnly, "run-only"}, {AncestryUnknown, "ancestry-unknown"}, @@ -50,7 +52,7 @@ func TestCategoryIsInconclusive(t *testing.T) { } blocking := []Category{ NotPinned, ShaAsRef, RefChanged, RefMoved, Stale, - MisleadingSHA, UnreachablePin, + MisleadingSHA, UnreachablePin, RepositoryChanged, RepositoryIdentityUnknown, 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..bfc0532a 100644 --- a/internal/pipeline/doc_urls.go +++ b/internal/pipeline/doc_urls.go @@ -29,16 +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.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 46aa908a..b9acaf61 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,44 @@ 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() + } + 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) + } + } + repositoryIdentities := lookupRepositoryIdentities(ctx, r, opts.Pool, identityRefs) var seedDeps []dep.Dependency recordedKeys := make(map[string]bool) for i := range parsed { @@ -73,9 +111,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 @@ -84,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 } } @@ -145,6 +204,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 +215,232 @@ func Run(ctx context.Context, opts RunOptions) (*RunResult, error) { }, nil } +type repositoryIdentityRef struct { + Ref parserlock.ActionRef + Hostname string + Parent string + RepoID int64 +} + +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) { + 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(pw.Path)] { + if pin, ok := parserlock.ParsePin(root); !ok || !current[ghapi.ForNWORef(pin.Owner, pin.Repo, pin.Ref)] { + continue + } + 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 +} + +type repositoryIdentity struct { + canonical string + repoID int64 + err error +} + +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 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()) +} + +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 + // 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 && 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 + }, + ) + } + 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.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()) { + 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 +448,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..096c6ede 100644 --- a/internal/pipeline/run_test.go +++ b/internal/pipeline/run_test.go @@ -91,6 +91,65 @@ 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", "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}, + }, + } + // 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()) + 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 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 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..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| @@ -343,6 +354,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 +449,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 } @@ -500,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", @@ -647,6 +660,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 +704,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 } })] @@ -685,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 79377cbd..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, clean exit" + 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,14 +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: "0" + equals: "false" + - expr: '[.findings[] | select(.category == "repository-identity-unknown")] | length' + equals: "1" lockfile_contains: - "version: 'v0.0.2'" @@ -1984,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" @@ -2010,6 +2005,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"