diff --git a/cmd/gh-actions-lock/command_test.go b/cmd/gh-actions-lock/command_test.go index 11cc69a0..f50cc39f 100644 --- a/cmd/gh-actions-lock/command_test.go +++ b/cmd/gh-actions-lock/command_test.go @@ -1056,7 +1056,11 @@ func TestCheckCommand_LoadErrorFailsFixMode(t *testing.T) { args := append(tt.args, workflowPath) _, stderr, err := runCommandWithHTTP(t, reg, args...) require.Error(t, err) - assert.Contains(t, err.Error(), "parsing workflow YAML") + if tt.name == "migration enabled" { + assert.Contains(t, err.Error(), "parsing workflow YAML") + } else { + require.ErrorIs(t, err, errSilent) + } assert.NotContains(t, stderr, "All 1 workflow valid") }) } diff --git a/cmd/gh-actions-lock/migrate_test.go b/cmd/gh-actions-lock/migrate_test.go index fe1028ef..7237eb59 100644 --- a/cmd/gh-actions-lock/migrate_test.go +++ b/cmd/gh-actions-lock/migrate_test.go @@ -173,9 +173,7 @@ jobs: t.Chdir(dir) - _, _, err := runCommandWithHTTP(t, reg, - ".github/workflows/workflow.yml", - ) + _, _, err := runCommandWithHTTP(t, reg) require.NoError(t, err) read := func(rel string) string { @@ -267,3 +265,58 @@ jobs: assert.NotContains(t, got, "$/", "opt-out means no migration to $/") }) } + +func TestNoMigrateLocalActions_PreservesExistingPins(t *testing.T) { + const workflowWithRemoteAction = `name: CI +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - uses: ./.github/actions/foo + - uses: actions/checkout@v4 +` + const workflowWithOnlyLocalAction = `name: CI +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - uses: ./.github/actions/foo +` + const lock = `version: 'v0.0.2' +dependencies: + 'actions/checkout@v4': + ref: 'v4' + commit: 'sha1-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa' + owner_id: 1 + repo_id: 1 +workflows: + '.github/workflows/ci.yml': + - 'actions/checkout@v4' +` + + for name, workflow := range map[string]string{ + "remote and local actions": workflowWithRemoteAction, + "only local action": workflowWithOnlyLocalAction, + "malformed workflow": "name: [", + } { + t.Run(name, func(t *testing.T) { + dir := t.TempDir() + t.Chdir(dir) + workflowPath := filepath.Join(".github", "workflows", "ci.yml") + require.NoError(t, os.MkdirAll(filepath.Dir(workflowPath), 0o755)) + require.NoError(t, os.WriteFile(workflowPath, []byte(workflow), 0o600)) + lockPath := filepath.Join(".github", "workflows", "actions.lock") + require.NoError(t, os.WriteFile(lockPath, []byte(lock), 0o600)) + + transport := &requestCountingTransport{} + _, _, err := runCommandWithHTTP(t, transport, "--rescan", "--no-migrate-local-actions") + require.Error(t, err) + got, readErr := os.ReadFile(lockPath) + require.NoError(t, readErr) + assert.Equal(t, 2, strings.Count(string(got), "'actions/checkout@v4'")) + assert.NotContains(t, string(got), "'.github/workflows/ci.yml': []") + }) + } +} diff --git a/cmd/gh-actions-lock/pin_summary.go b/cmd/gh-actions-lock/pin_summary.go index c7f9bc28..cdd8375e 100644 --- a/cmd/gh-actions-lock/pin_summary.go +++ b/cmd/gh-actions-lock/pin_summary.go @@ -176,10 +176,9 @@ func renderCooldownFindings(console *ui.UI, report *checks.Report) { } } -// renderNarrowedEntries shows refs that were upgraded from mutable (main, v4) -// to full semver (v6.0.2) on already-pinned workflows. +// renderNarrowedEntries shows refs updated on already-pinned workflows. func renderNarrowedEntries(console *ui.UI, narrowed []pin.Entry) { - console.TermSuccess("Narrowed %d %s to full semver", + console.TermSuccess("Updated %d %s", len(narrowed), ui.Pluralize(len(narrowed), "ref", "refs")) for _, e := range narrowed { console.TermDetail(" %s@%s → %s", e.NWO, e.AutoFixedRef, e.Ref) diff --git a/cmd/gh-actions-lock/pin_summary_test.go b/cmd/gh-actions-lock/pin_summary_test.go index 836383df..8e257bbb 100644 --- a/cmd/gh-actions-lock/pin_summary_test.go +++ b/cmd/gh-actions-lock/pin_summary_test.go @@ -11,6 +11,7 @@ import ( "github.com/github/gh-actions-lock/internal/pipeline/checks" "github.com/github/gh-actions-lock/internal/resolve" "github.com/github/gh-actions-lock/internal/ui" + "github.com/stretchr/testify/assert" ) // A workflow shared between two different actions must still be listed under @@ -39,6 +40,23 @@ func TestRenderPinnedEntries_WorkflowSharedAcrossActions(t *testing.T) { } } +func TestRenderNarrowedEntries_BranchRepair(t *testing.T) { + var buf bytes.Buffer + console := ui.NewPlain(&buf) + + renderNarrowedEntries(console, []pin.Entry{{ + NWO: "owner/action", + Ref: "main", + AutoFixedRef: strings.Repeat("a", 40), + }}) + + out := buf.String() + assert.Contains(t, out, "Updated 1 ref") + assert.NotContains(t, out, "semver") + assert.Contains(t, out, "owner/action@aaaaaaaa") + assert.Contains(t, out, "→ main") +} + func TestReportHasUnfixableErrors_ClassifiesWorkflowNotPinned(t *testing.T) { for _, tt := range []struct { name string diff --git a/cmd/gh-actions-lock/run.go b/cmd/gh-actions-lock/run.go index 0b7a3d32..547d5882 100644 --- a/cmd/gh-actions-lock/run.go +++ b/cmd/gh-actions-lock/run.go @@ -380,6 +380,7 @@ func runCheck(cmd *cobra.Command, opts *checkOptions, newResolver resolverFunc) NoNarrow: opts.noNarrow, AcceptMoved: opts.acceptMoved, Relock: opts.relock, + PartialScan: !fullScan, }) endPlan() if planErr != nil { diff --git a/cmd/gh-actions-lock/selfrepository_test.go b/cmd/gh-actions-lock/selfrepository_test.go index 80c5ec00..84eb3faf 100644 --- a/cmd/gh-actions-lock/selfrepository_test.go +++ b/cmd/gh-actions-lock/selfrepository_test.go @@ -97,3 +97,105 @@ jobs: _, readErr = os.Stat(lockPath) assert.ErrorIs(t, readErr, os.ErrNotExist) } + +func TestExistingSHARefRewritesSelfRepositoryAction(t *testing.T) { + const sha = "bcd2ba49218906704ab6c1aa796996da409d3eb1" + transport := &requestCountingTransport{} + + dir := t.TempDir() + require.NoError(t, os.Mkdir(filepath.Join(dir, ".git"), 0o755)) + actionPath := filepath.Join(dir, ".github", "actions", "local", "action.yml") + require.NoError(t, os.MkdirAll(filepath.Dir(actionPath), 0o755)) + require.NoError(t, os.WriteFile(actionPath, []byte(`name: Local +runs: + using: composite + steps: + - uses: actions/create-github-app-token@`+sha+` +`), 0o600)) + + workflowPath := filepath.Join(dir, ".github", "workflows", "ci.yml") + require.NoError(t, os.MkdirAll(filepath.Dir(workflowPath), 0o755)) + workflow := []byte(`name: CI +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - uses: $/.github/actions/local +`) + require.NoError(t, os.WriteFile(workflowPath, workflow, 0o600)) + require.NoError(t, os.WriteFile(filepath.Join(filepath.Dir(workflowPath), "other.yml"), []byte(`name: Other +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - uses: actions/create-github-app-token@v3.2.0 +`), 0o600)) + lockPath := filepath.Join(dir, ".github", "workflows", "actions.lock") + originalLock := []byte(`version: 'v0.0.2' +workflows: + '.github/workflows/ci.yml': + - 'actions/create-github-app-token@` + sha + `' + '.github/workflows/other.yml': + - 'actions/create-github-app-token@v3.2.0' +dependencies: + 'actions/create-github-app-token@` + sha + `': + ref: 'v3.2.0' + commit: 'sha1-` + sha + `' + owner_id: 44036562 + repo_id: 642580244 + uses: + - 'actions/checkout@v4' + 'actions/create-github-app-token@v3.2.0': + ref: 'v3.2.0' + commit: 'sha1-` + sha + `' + owner_id: 44036562 + repo_id: 642580244 + uses: + - 'actions/setup-go@v5' + 'actions/checkout@v4': + ref: 'v4' + commit: 'sha1-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa' + owner_id: 44036562 + repo_id: 197814629 + 'actions/setup-go@v5': + ref: 'v5' + commit: 'sha1-bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb' + owner_id: 44036562 + repo_id: 485264523 +`) + require.NoError(t, os.WriteFile(lockPath, originalLock, 0o600)) + t.Chdir(dir) + + _, _, err := runCommandWithHTTP(t, transport, "--no-narrow", filepath.Join(".github", "workflows", "ci.yml")) + require.NoError(t, err) + lockAfterNoNarrow, err := os.ReadFile(lockPath) + require.NoError(t, err) + + _, _, err = runCommandWithHTTP(t, transport, filepath.Join(".github", "workflows", "ci.yml")) + require.ErrorContains(t, err, "partial workflow scan") + action, readErr := os.ReadFile(actionPath) + require.NoError(t, readErr) + assert.Contains(t, string(action), "uses: actions/create-github-app-token@"+sha) + lock, readErr := os.ReadFile(lockPath) + require.NoError(t, readErr) + assert.Equal(t, lockAfterNoNarrow, lock) + + _, _, err = runCommandWithHTTP(t, transport) + require.NoError(t, err) + assert.Zero(t, transport.calls.Load()) + + action, err = os.ReadFile(actionPath) + require.NoError(t, err) + assert.Contains(t, string(action), "uses: actions/create-github-app-token@v3.2.0") + + lock, err = os.ReadFile(lockPath) + require.NoError(t, err) + assert.Contains(t, string(lock), "'actions/create-github-app-token@v3.2.0'") + assert.Contains(t, string(lock), "'actions/checkout@v4'") + assert.Contains(t, string(lock), "'actions/setup-go@v5'") + assert.Contains(t, string(lock), "uses:\n - 'actions/checkout@v4'") + assert.Contains(t, string(lock), "- 'actions/setup-go@v5'") + assert.NotContains(t, string(lock), "'actions/create-github-app-token@"+sha+"'") +} diff --git a/internal/dep/dependency.go b/internal/dep/dependency.go index 86dc7f03..6fd82fb1 100644 --- a/internal/dep/dependency.go +++ b/internal/dep/dependency.go @@ -27,6 +27,8 @@ type Dependency struct { Ref string // resolved ref as given in uses: SHA string // full commit hash HashAlgo string // "sha1" or "sha256" + // OriginalRef is the lockfile key ref before an in-memory rewrite. + OriginalRef string // Tag is the discovered release/tag pointing at SHA, if any. Optional. // Populated by the pin-time discovery pass; not read from `uses:`. Tag string diff --git a/internal/lockfile/state.go b/internal/lockfile/state.go index 00f96ec3..93860cff 100644 --- a/internal/lockfile/state.go +++ b/internal/lockfile/state.go @@ -406,7 +406,8 @@ func (s *State) Set(ctx context.Context, workflowKey string, deps []dep.Dependen ref = d.Branch } } - if existing, ok := s.file.Dependencies[pinKey]; ok { + existing, ok := s.file.Dependencies[pinKey] + if ok { if ref == "" { ref = existing.Ref } @@ -414,6 +415,19 @@ func (s *State) Set(ctx context.Context, workflowKey string, deps []dep.Dependen usesSet[u] = true } } + oldRef := d.OriginalRef + if !isSHARef(d.Ref) && isSHARef(oldRef) { + shaPin := pin + shaPin.Ref = oldRef + if shaExisting, shaOK := s.file.Dependencies[shaPin.String()]; shaOK { + if !ok && ref == "" { + ref = shaExisting.Ref + } + for _, u := range shaExisting.Uses { + usesSet[u] = true + } + } + } var uses []string if len(usesSet) > 0 { uses = make([]string, 0, len(usesSet)) diff --git a/internal/lockfile/state_test.go b/internal/lockfile/state_test.go index 39630d3d..af7b70f4 100644 --- a/internal/lockfile/state_test.go +++ b/internal/lockfile/state_test.go @@ -111,6 +111,47 @@ func actionKeys[V any](m map[string]V) []string { return keys } +func TestState_DoesNotMergeImplicitSHAEdges(t *testing.T) { + dir := t.TempDir() + store, err := LoadState(dir, fakeMetadataResolver{}) + if err != nil { + t.Fatal(err) + } + + const sha = "1111111111111111111111111111111111111111" + symbolic := dep.Dependency{NWO: "owner/action", Ref: "v1", SHA: sha, HashAlgo: "sha1"} + shaParent := dep.Dependency{NWO: "owner/action", Ref: sha, SHA: sha, HashAlgo: "sha1"} + child := dep.Dependency{ + NWO: "actions/checkout", Ref: "v4", + SHA: "2222222222222222222222222222222222222222", HashAlgo: "sha1", + } + ctx := context.Background() + if err := store.Set(ctx, "symbolic.yml", []dep.Dependency{symbolic}, nil, map[string]bool{symbolic.Key(): true}); err != nil { + t.Fatal(err) + } + if err := store.Set(ctx, "sha.yml", []dep.Dependency{shaParent, child}, + map[string][]string{child.Key(): {shaParent.Key()}}, + map[string]bool{shaParent.Key(): true}); err != nil { + t.Fatal(err) + } + + if err := store.Set(ctx, "symbolic.yml", []dep.Dependency{symbolic}, nil, map[string]bool{symbolic.Key(): true}); err != nil { + t.Fatal(err) + } + store.PruneWorkflows(map[string]bool{"symbolic.yml": true}) + if err := store.Save(); err != nil { + t.Fatal(err) + } + + action := store.file.Dependencies["owner/action@v1"] + if len(action.Uses) != 0 { + t.Fatalf("symbolic dependency retained unrelated SHA edges: %v", action.Uses) + } + if _, ok := store.file.Dependencies[child.Key()]; ok { + t.Fatalf("stale child %q was not garbage-collected", child.Key()) + } +} + // TestState_SetAcceptsEmptyBranch verifies that deps with no discovered // branch are accepted — the new schema makes ref optional. func TestState_SetAcceptsEmptyBranch(t *testing.T) { diff --git a/internal/pin/commit.go b/internal/pin/commit.go index 6dcc3eef..9d48eece 100644 --- a/internal/pin/commit.go +++ b/internal/pin/commit.go @@ -156,11 +156,12 @@ func groupPinnedByWorkflow(rec *Record) map[string][]dep.Dependency { } for _, wf := range e.Workflows { result[wf] = append(result[wf], dep.Dependency{ - NWO: e.NWO, - Ref: e.Ref, - SHA: e.SHA, - Branch: e.OnBranch, - Tag: e.Tag, + NWO: e.NWO, + Ref: e.Ref, + SHA: e.SHA, + OriginalRef: e.AutoFixedRef, + Branch: e.OnBranch, + Tag: e.Tag, }) } } diff --git a/internal/pin/commit_test.go b/internal/pin/commit_test.go index 9abe31bf..e2c23abd 100644 --- a/internal/pin/commit_test.go +++ b/internal/pin/commit_test.go @@ -142,6 +142,7 @@ jobs: }}, Workflows: []WorkflowPlan{{Path: workflowPath}}, } + require.NoError(t, Commit(context.Background(), rec, store, nil)) got, err := os.ReadFile(filepath.Join(dir, ".github", "workflows", "actions.lock")) @@ -150,3 +151,60 @@ jobs: assert.NotContains(t, string(got), "owner/action@main") assert.NotContains(t, string(got), "actions/setup-go@v5") } + +func TestCommitRekeysAnnotatedTagObjectWithUses(t *testing.T) { + const tagObjectSHA = "1111111111111111111111111111111111111111" + const commitSHA = "2222222222222222222222222222222222222222" + workflowPath := filepath.Join(".github", "workflows", "ci.yml") + dir := t.TempDir() + require.NoError(t, os.MkdirAll(filepath.Join(dir, filepath.Dir(workflowPath)), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(dir, workflowPath), []byte(`on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - uses: owner/action@`+tagObjectSHA+` +`), 0o644)) + t.Chdir(dir) + + store, err := lockfile.LoadState(dir, fakeMeta{}) + require.NoError(t, err) + parent := dep.Dependency{ + NWO: "owner/action", Ref: tagObjectSHA, SHA: tagObjectSHA, + HashAlgo: "sha1", Tag: "v3.2.0", + } + child := dep.Dependency{ + NWO: "actions/checkout", Ref: "v4", SHA: strings.Repeat("3", 40), + HashAlgo: "sha1", + } + require.NoError(t, store.Set(context.Background(), workflowPath, + []dep.Dependency{parent, child}, + map[string][]string{child.Key(): {parent.Key()}}, + map[string]bool{parent.Key(): true})) + require.NoError(t, store.Save()) + + rec := &Record{ + Entries: []Entry{{ + NWO: parent.NWO, + Ref: "v3.2.0", + SHA: commitSHA, + Resolution: Verified, + AutoFixedRef: tagObjectSHA, + Direct: true, + Workflows: []string{workflowPath}, + }}, + Workflows: []WorkflowPlan{{ + Path: workflowPath, + Rewrites: map[string]string{ + parent.NWO + "@" + tagObjectSHA: parent.NWO + "@v3.2.0", + }, + }}, + } + require.NoError(t, Commit(context.Background(), rec, store, nil)) + + got, err := os.ReadFile(filepath.Join(dir, ".github", "workflows", "actions.lock")) + require.NoError(t, err) + assert.Contains(t, string(got), "'owner/action@v3.2.0'") + assert.Contains(t, string(got), "uses:\n - 'actions/checkout@v4'") + assert.NotContains(t, string(got), "'owner/action@"+tagObjectSHA+"'") +} diff --git a/internal/pin/plan.go b/internal/pin/plan.go index aac509e9..6287cb05 100644 --- a/internal/pin/plan.go +++ b/internal/pin/plan.go @@ -41,6 +41,10 @@ type PlanOptions struct { // findings untouched so possible tampering stays a hard error. Relock bool + // PartialScan reports that explicit workflow paths, rather than the full + // workflow directory, define this run's mutation authority. + PartialScan bool + // prevImpreciseNWO is computed once in Plan() from the global lockfile // state. It holds lowercased NWOs that are already recorded with a // non-full-semver ref anywhere in the lockfile. Narrowing is skipped @@ -80,7 +84,6 @@ func Plan(ctx context.Context, report *checks.Report, opts PlanOptions) (*Record if opts.prevImpreciseNWO == nil && opts.Store != nil { opts.prevImpreciseNWO = impreciseDirectNWOs(opts.Store) } - results := make([]planResult, len(report.Workflows)) var planErr error poolErr := pinpool.RunTyped(opts.Pool, ctx, "Planning pins", @@ -102,7 +105,20 @@ func Plan(ctx context.Context, report *checks.Report, opts PlanOptions) (*Record planErr = poolErr } + targetSHAs := make(map[string]string) for _, pr := range results { + for _, entry := range pr.entries { + if planErr != nil || entry.SHA == "" || + entry.Resolution != Pinned && entry.Resolution != Verified { + continue + } + key := strings.ToLower(entry.NWO) + "@" + entry.Ref + if sha, ok := targetSHAs[key]; ok && !strings.EqualFold(sha, entry.SHA) { + planErr = fmt.Errorf("conflicting planned target %s resolves to both %s and %s", key, sha, entry.SHA) + continue + } + targetSHAs[key] = entry.SHA + } rec.Entries = append(rec.Entries, pr.entries...) rec.Workflows = append(rec.Workflows, pr.wplans...) } @@ -118,6 +134,9 @@ type planResult struct { func planWorkflow(ctx context.Context, wr checks.WorkflowReport, opts PlanOptions, status func(string)) (planResult, error) { var entries []Entry var wplans []WorkflowPlan + if wr.SkipCommit { + return planResult{entries: verifiedEntries(wr.Inventory, wr.Path)}, nil + } for _, finding := range wr.Findings { if finding.Category == checks.InvalidSelfRepositoryRef { return planResult{}, nil @@ -127,8 +146,6 @@ func planWorkflow(ctx context.Context, wr checks.WorkflowReport, opts PlanOption if rewriteRefs == nil { rewriteRefs = wr.ActionRefs } - rewriteRefKeys := actionRefKeys(rewriteRefs) - // 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) @@ -136,7 +153,10 @@ func planWorkflow(ctx context.Context, wr checks.WorkflowReport, opts PlanOption if !wr.NeedsAttention() && !repinMoved { entries = verifiedEntries(inventory, wr.Path) - rw := narrowVerifiedEntries(ctx, entries, opts, rewriteRefKeys) + rw := narrowVerifiedEntries(ctx, entries, opts, rewriteRefs) + if err := rejectPartialSelfActionRewrites(opts, wr.SelfActionRefs, rw); err != nil { + return planResult{}, err + } wplans = append(wplans, WorkflowPlan{Path: wr.Path, Rewrites: rw, SelfActionFiles: wr.SelfActionFiles}) return planResult{entries: entries, wplans: wplans}, nil } @@ -170,7 +190,10 @@ func planWorkflow(ctx context.Context, wr checks.WorkflowReport, opts PlanOption } if len(unrecordedRefs) == 0 { - rw := narrowVerifiedEntries(ctx, entries, opts, rewriteRefKeys) + rw := narrowVerifiedEntries(ctx, entries, opts, rewriteRefs) + if err := rejectPartialSelfActionRewrites(opts, wr.SelfActionRefs, rw); err != nil { + return planResult{}, err + } wplans = append(wplans, WorkflowPlan{Path: wr.Path, Rewrites: rw, SelfActionFiles: wr.SelfActionFiles}) return planResult{entries: entries, wplans: wplans}, nil } @@ -260,11 +283,14 @@ func planWorkflow(ctx context.Context, wr checks.WorkflowReport, opts PlanOption // Record workflow plan if there are rewrites. // Also narrow any verified (already-recorded) entries that have imprecise refs. - if verifiedRW := narrowVerifiedEntries(ctx, entries, opts, rewriteRefKeys); len(verifiedRW) > 0 { + if verifiedRW := narrowVerifiedEntries(ctx, entries, opts, rewriteRefs); len(verifiedRW) > 0 { for k, v := range verifiedRW { rewrites[k] = v } } + if err := rejectPartialSelfActionRewrites(opts, wr.SelfActionRefs, rewrites); err != nil { + return planResult{}, err + } if len(rewrites) > 0 { wplans = append(wplans, WorkflowPlan{ Path: wr.Path, @@ -286,6 +312,24 @@ func planWorkflow(ctx context.Context, wr checks.WorkflowReport, opts PlanOption return planResult{entries: entries, wplans: wplans}, nil } +func rejectPartialSelfActionRewrites(opts PlanOptions, selfActionRefs []parserlock.ActionRef, rewrites map[string]string) error { + if !opts.PartialScan || len(rewrites) == 0 { + return nil + } + selfKeys := actionRefKeys(selfActionRefs) + for oldUses := range rewrites { + ref := parserlock.ParseActionRef(oldUses) + if ref == nil { + continue + } + key := strings.ToLower(ref.Owner+"/"+ref.Repo) + "@" + ref.Ref + if selfKeys[key] { + return fmt.Errorf("cannot rewrite %s in a shared local action during a partial workflow scan; scan all workflows", key) + } + } + return nil +} + // unresolvedEntries flags findings whose refs were attempted but failed to // resolve. On a partial failure deps holds the refs that did resolve, so only // the genuine misses (attempted and not in deps) are marked Unresolved. @@ -616,6 +660,8 @@ func verifiedEntries(inventory []checks.InventoryEntry, path string) []Entry { Ref: inv.Dep.Ref, SHA: inv.Dep.SHA, Resolution: Verified, + OnBranch: inv.Dep.Branch, + Tag: inv.Dep.Tag, Workflows: []string{path}, Direct: inv.Direct, RequiredBy: inv.Parents, @@ -627,10 +673,11 @@ func verifiedEntries(inventory []checks.InventoryEntry, path string) []Entry { // narrowVerifiedEntries upgrades already-recorded direct deps to full semver // tags when possible, returning the workflow-YAML rewrites. Skipped for // --no-narrow, transitive deps, and refs the user kept imprecise (sticky v4). -func narrowVerifiedEntries(ctx context.Context, entries []Entry, opts PlanOptions, rewriteRefKeys map[string]bool) map[string]string { +func narrowVerifiedEntries(ctx context.Context, entries []Entry, opts PlanOptions, rewriteRefs []parserlock.ActionRef) map[string]string { if opts.NoNarrow || opts.Tagger == nil { return nil } + rewriteRefKeys := actionRefKeys(rewriteRefs) rewrites := make(map[string]string) for i := range entries { e := &entries[i] @@ -640,6 +687,27 @@ func narrowVerifiedEntries(ctx context.Context, entries []Entry, opts PlanOption if !rewriteRefKeys[strings.ToLower(e.NWO)+"@"+e.Ref] { continue } + if parserlock.IsFullSha(e.Ref) { + newRef := e.Tag + if newRef == "" { + newRef = e.OnBranch + } + if newRef == "" { + continue + } + if parserlock.IsFullSha(newRef) || hasConflictingLockTarget(opts.Store, e.NWO, newRef, e.SHA) { + continue + } + oldRef := e.Ref + for _, ref := range rewriteRefs { + if strings.EqualFold(ref.Owner+"/"+ref.Repo, e.NWO) && ref.Ref == oldRef { + rewrites[ref.FullName()+"@"+oldRef] = ref.FullName() + "@" + newRef + } + } + e.Ref = newRef + e.AutoFixedRef = oldRef + continue + } owner, repo := splitNWO(e.NWO) if owner == "" { continue @@ -686,6 +754,18 @@ func narrowVerifiedEntries(ctx context.Context, entries []Entry, opts PlanOption return rewrites } +func hasConflictingLockTarget(store *lockfile.State, nwo, ref, sha string) bool { + if store == nil { + return false + } + action, ok := store.File().Dependencies[nwo+"@"+ref] + if !ok { + return false + } + _, targetSHA, ok := strings.Cut(action.Commit, "-") + return ok && !strings.EqualFold(targetSHA, sha) +} + func actionRefKeys(refs []parserlock.ActionRef) map[string]bool { keys := make(map[string]bool, len(refs)) for _, ref := range refs { diff --git a/internal/pin/plan_test.go b/internal/pin/plan_test.go index 2c3955f5..a290ab5e 100644 --- a/internal/pin/plan_test.go +++ b/internal/pin/plan_test.go @@ -5,6 +5,7 @@ import ( "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" @@ -297,6 +298,125 @@ func TestNarrowVerifiedEntries_StickyPrecision(t *testing.T) { assert.Empty(t, result.wplans[0].Rewrites, "no workflow rewrite for a sticky entry") }) + t.Run("sticky sibling does not suppress SHA metadata repair", func(t *testing.T) { + tagger, _ := newTagger(t) + report := fastPathReport(sha) + report.Inventory[0].Dep.Tag = "v4.2.1" + report.ActionRefs = []parserlock.ActionRef{{ + Owner: "actions", + Repo: "checkout", + Ref: sha, + }} + opts := PlanOptions{ + Tagger: tagger, + prevImpreciseNWO: map[string]bool{"actions/checkout": true}, + } + + result, err := planWorkflow(context.Background(), report, opts, func(string) {}) + require.NoError(t, err) + + require.Len(t, result.entries, 1) + assert.Equal(t, "v4.2.1", result.entries[0].Ref) + assert.Equal(t, sha, result.entries[0].AutoFixedRef) + require.Len(t, result.wplans, 1) + assert.Equal(t, + map[string]string{"actions/checkout@" + sha: "actions/checkout@v4.2.1"}, + result.wplans[0].Rewrites, + ) + }) + + t.Run("SHA metadata does not rewrite to itself", func(t *testing.T) { + tagger, _ := newTagger(t) + report := fastPathReport(sha) + report.Inventory[0].Dep.Branch = sha + report.ActionRefs = []parserlock.ActionRef{{ + Owner: "actions", + Repo: "checkout", + Ref: sha, + }} + + result, err := planWorkflow(context.Background(), report, PlanOptions{Tagger: tagger}, func(string) {}) + require.NoError(t, err) + + require.Len(t, result.entries, 1) + assert.Equal(t, sha, result.entries[0].Ref) + assert.Empty(t, result.entries[0].AutoFixedRef) + assert.Empty(t, result.wplans[0].Rewrites) + }) + + t.Run("SHA metadata repairs to branch", func(t *testing.T) { + tagger, _ := newTagger(t) + report := fastPathReport(sha) + report.Inventory[0].Dep.Branch = "main" + report.ActionRefs = []parserlock.ActionRef{{ + Owner: "actions", + Repo: "checkout", + Ref: sha, + }} + + result, err := planWorkflow(context.Background(), report, PlanOptions{Tagger: tagger}, func(string) {}) + require.NoError(t, err) + + require.Len(t, result.entries, 1) + assert.Equal(t, "main", result.entries[0].Ref) + assert.Equal(t, sha, result.entries[0].AutoFixedRef) + assert.Equal(t, + map[string]string{"actions/checkout@" + sha: "actions/checkout@main"}, + result.wplans[0].Rewrites, + ) + }) + + t.Run("repair preserves source NWO spelling", func(t *testing.T) { + tagger, _ := newTagger(t) + report := fastPathReport(sha) + report.Inventory[0].Dep.Tag = "v4.2.1" + report.ActionRefs = []parserlock.ActionRef{{ + Owner: "Actions", + Repo: "Checkout", + Ref: sha, + }} + + result, err := planWorkflow(context.Background(), report, PlanOptions{Tagger: tagger}, func(string) {}) + require.NoError(t, err) + + require.Len(t, result.entries, 1) + assert.Equal(t, "v4.2.1", result.entries[0].Ref) + assert.Equal(t, + map[string]string{"Actions/Checkout@" + sha: "Actions/Checkout@v4.2.1"}, + result.wplans[0].Rewrites, + ) + }) + + t.Run("repair declines conflicting symbolic target", func(t *testing.T) { + tagger, _ := newTagger(t) + store, err := lockfile.LoadState(t.TempDir(), fakeMeta{}) + require.NoError(t, err) + target := dep.Dependency{ + NWO: "actions/checkout", + Ref: "v4.2.1", + SHA: "def4560000000000000000000000000000000000", + HashAlgo: "sha1", + } + require.NoError(t, store.Set(context.Background(), "other.yml", + []dep.Dependency{target}, nil, map[string]bool{target.Key(): true})) + + report := fastPathReport(sha) + report.Inventory[0].Dep.Tag = target.Ref + report.ActionRefs = []parserlock.ActionRef{{ + Owner: "actions", + Repo: "checkout", + Ref: sha, + }} + + result, err := planWorkflow(context.Background(), report, PlanOptions{Tagger: tagger, Store: store}, func(string) {}) + require.NoError(t, err) + + require.Len(t, result.entries, 1) + assert.Equal(t, sha, result.entries[0].Ref) + assert.Empty(t, result.entries[0].AutoFixedRef) + assert.Empty(t, result.wplans[0].Rewrites) + }) + t.Run("branch ref main is NOT narrowed", func(t *testing.T) { // main is not version-shaped, so narrowing must not touch it. // Non-version refs are intentional choices (e.g. vercel/next.js@canary). @@ -313,6 +433,92 @@ func TestNarrowVerifiedEntries_StickyPrecision(t *testing.T) { }) } +func TestPlanRejectsConflictingSameRunRepairs(t *testing.T) { + const firstSHA = "abc1230000000000000000000000000000000000" + const secondSHA = "def4560000000000000000000000000000000000" + report := func(path, sha string) checks.WorkflowReport { + return checks.WorkflowReport{ + Path: path, + ActionRefs: []parserlock.ActionRef{{ + Owner: "actions", Repo: "checkout", Ref: sha, + }}, + Inventory: []checks.InventoryEntry{{ + Dep: dep.Dependency{NWO: "actions/checkout", Ref: sha, SHA: sha, Tag: "v4.2.1"}, + File: path, + Direct: true, + }}, + } + } + + _, err := Plan(context.Background(), &checks.Report{ + Workflows: []checks.WorkflowReport{ + report(".github/workflows/first.yml", firstSHA), + report(".github/workflows/second.yml", secondSHA), + }, + }, PlanOptions{ + Tagger: new(tag.Lister), + Pool: pinpool.New(2, nil), + }) + + require.ErrorContains(t, err, "conflicting planned target actions/checkout@v4.2.1") +} + +func TestPlanRejectsRepairConflictingWithSymbolicEntry(t *testing.T) { + const repairedSHA = "abc1230000000000000000000000000000000000" + const symbolicSHA = "def4560000000000000000000000000000000000" + repair := checks.WorkflowReport{ + Path: ".github/workflows/repair.yml", + ActionRefs: []parserlock.ActionRef{{ + Owner: "actions", Repo: "checkout", Ref: repairedSHA, + }}, + Inventory: []checks.InventoryEntry{{ + Dep: dep.Dependency{NWO: "actions/checkout", Ref: repairedSHA, SHA: repairedSHA, Tag: "v4.2.1"}, + File: ".github/workflows/repair.yml", + Direct: true, + }}, + } + symbolic := checks.WorkflowReport{ + Path: ".github/workflows/symbolic.yml", + Inventory: []checks.InventoryEntry{{ + Dep: dep.Dependency{NWO: "actions/checkout", Ref: "v4.2.1", SHA: symbolicSHA}, + File: ".github/workflows/symbolic.yml", + Direct: true, + }}, + } + + _, err := Plan(context.Background(), &checks.Report{ + Workflows: []checks.WorkflowReport{repair, symbolic}, + }, PlanOptions{ + Tagger: new(tag.Lister), + Pool: pinpool.New(2, nil), + }) + + require.ErrorContains(t, err, "conflicting planned target actions/checkout@v4.2.1") +} + +func TestPlanExcludesLoadFailuresFromCommit(t *testing.T) { + const sha = "abc1230000000000000000000000000000000000" + blocked := checks.WorkflowReport{ + Path: ".github/workflows/broken.yml", + SkipCommit: true, + Inventory: []checks.InventoryEntry{{ + Dep: dep.Dependency{NWO: "actions/checkout", Ref: "v4", SHA: sha}, + File: ".github/workflows/broken.yml", + }}, + } + valid := checks.WorkflowReport{Path: ".github/workflows/valid.yml"} + + record, err := Plan(context.Background(), &checks.Report{ + Workflows: []checks.WorkflowReport{blocked, valid}, + }, PlanOptions{Pool: pinpool.New(2, nil)}) + require.NoError(t, err) + + require.Len(t, record.Workflows, 1) + assert.Equal(t, valid.Path, record.Workflows[0].Path) + require.Len(t, record.Entries, 1) + assert.Equal(t, blocked.Path, record.Entries[0].Workflows[0]) +} + func TestPlanWorkflow_SelfRepositoryDependencyIsNotRewrittenOnFastPath(t *testing.T) { const sha = "abc1230000000000000000000000000000000000" @@ -495,6 +701,20 @@ func TestNoNarrow_BareSHA(t *testing.T) { "actions/checkout@"+sha, "rewrite map should record the original SHA ref") }) + + t.Run("partial scan rejects unrecorded shared action rewrite", func(t *testing.T) { + resolver, tagger, wr, _ := newSlowPathFixtures(t, false) + wr.SelfActionRefs = append([]parserlock.ActionRef(nil), wr.ActionRefs...) + + _, err := planWorkflow(context.Background(), wr, PlanOptions{ + Resolver: resolver, + Tagger: tagger, + Pool: pinpool.New(2, nil), + PartialScan: true, + }, func(string) {}) + + require.ErrorContains(t, err, "shared local action during a partial workflow scan") + }) } // TestPlanWorkflow_CrossRefTransitiveClosure verifies that a composite at diff --git a/internal/pipeline/checks/finding.go b/internal/pipeline/checks/finding.go index 48c7f6af..0d82da92 100644 --- a/internal/pipeline/checks/finding.go +++ b/internal/pipeline/checks/finding.go @@ -63,6 +63,8 @@ type InventoryEntry struct { type WorkflowReport struct { Path string Findings []Finding + // SkipCommit prevents terminal parse failures from entering the write phase. + SkipCommit bool // ActionRefs are all remote dependency roots attributed to the workflow, // including refs found inside in-repo `$/…` actions. ActionRefs []parserlock.ActionRef @@ -71,6 +73,8 @@ type WorkflowReport struct { RewriteRefs []parserlock.ActionRef // SelfActionFiles are in-repo action definition files reached via `$/…`. SelfActionFiles []string + // SelfActionRefs are the remote refs found specifically inside SelfActionFiles. + SelfActionRefs []parserlock.ActionRef // Deps are the existing pinned dependencies (nil if not pinned). Deps []dep.Dependency // Inventory lists all dependencies with direct/transitive classification. diff --git a/internal/pipeline/checks/parsed.go b/internal/pipeline/checks/parsed.go index de73caa5..b25feb6d 100644 --- a/internal/pipeline/checks/parsed.go +++ b/internal/pipeline/checks/parsed.go @@ -20,7 +20,9 @@ type ParsedWorkflow struct { RewriteRefs []parserlock.ActionRef // SelfActionFiles are the in-repo action definition files reached through // step-level `$/…` refs. They are rewritten alongside the workflow. - SelfActionFiles []string + SelfActionFiles []string + // SelfActionRefs are the remote refs found specifically inside SelfActionFiles. + SelfActionRefs []parserlock.ActionRef LocalPaths []string SelfRepositoryRefs []string // SelfRepositoryRefErrs holds malformed `$/…@ref` values (the invalid form). diff --git a/internal/pipeline/diagnose.go b/internal/pipeline/diagnose.go index 337f0ffd..ac715eba 100644 --- a/internal/pipeline/diagnose.go +++ b/internal/pipeline/diagnose.go @@ -178,6 +178,7 @@ func precheckWorkflow(pw checks.ParsedWorkflow, store *lockfile.State) (checks.W wr := checks.WorkflowReport{Path: pw.Path} if pw.LoadErr != nil { + wr.SkipCommit = true wr.Findings = append(wr.Findings, checks.Finding{ WorkflowPath: pw.Path, Category: checks.NotPinned, @@ -187,6 +188,7 @@ func precheckWorkflow(pw checks.ParsedWorkflow, store *lockfile.State) (checks.W Detail: fmt.Sprintf("failed to load workflow: %s", pw.LoadErr), DocURL: DocURLFor(checks.NotPinned), }) + preserveExistingInventory(&wr, pw) return wr, true } @@ -196,6 +198,7 @@ func precheckWorkflow(pw checks.ParsedWorkflow, store *lockfile.State) (checks.W wr.RewriteRefs = pw.Refs } wr.SelfActionFiles = pw.SelfActionFiles + wr.SelfActionRefs = pw.SelfActionRefs wr.ParseWarnings = pw.ParseWarnings hasTerminalFinding := false @@ -248,6 +251,7 @@ func precheckWorkflow(pw checks.ParsedWorkflow, store *lockfile.State) (checks.W } if hasTerminalFinding { + preserveExistingInventory(&wr, pw) return wr, true } @@ -283,6 +287,17 @@ func precheckWorkflow(pw checks.ParsedWorkflow, store *lockfile.State) (checks.W return wr, false } +func preserveExistingInventory(wr *checks.WorkflowReport, pw checks.ParsedWorkflow) { + wr.Deps = pw.ExistingDeps + for _, d := range pw.ExistingDeps { + wr.Inventory = append(wr.Inventory, checks.InventoryEntry{ + Dep: d, + File: pw.Path, + Direct: true, + }) + } +} + func indexDeps(deps []dep.Dependency) map[string]dep.Dependency { out := make(map[string]dep.Dependency, len(deps)) for _, dep := range deps { diff --git a/internal/pipeline/parse.go b/internal/pipeline/parse.go index ac9e7b1a..cae4b3f1 100644 --- a/internal/pipeline/parse.go +++ b/internal/pipeline/parse.go @@ -38,6 +38,15 @@ func ParseAll(paths []string, store *lockfile.State) []checks.ParsedWorkflow { out := make([]checks.ParsedWorkflow, 0, total) for _, path := range paths { pw := checks.ParsedWorkflow{Path: path} + if store != nil { + wfKey := workflowfile.KeyFromPath(path) + deps, depsErr := store.Get(wfKey) + if depsErr != nil { + pw.DepsErr = depsErr + } else { + pw.ExistingDeps = deps + } + } wf, err := workflowfile.Load(path) if err != nil { pw.LoadErr = err @@ -52,20 +61,12 @@ func ParseAll(paths []string, store *lockfile.State) []checks.ParsedWorkflow { // rewritten alongside the workflow, so narrowing stays in sync. pw.RewriteRefs = pw.Refs pw.SelfActionFiles = selfScan.ActionFiles + pw.SelfActionRefs = selfScan.Refs pw.LocalPaths = mergeStrings(scan.LocalPaths, selfScan.LocalPaths) pw.SelfRepositoryRefs = mergeStrings(scan.SelfRepositoryRefs, selfScan.SelfRepositoryRefs) pw.SelfRepositoryRefErrs = mergeStrings(scan.SelfRepositoryRefErrs, selfScan.SelfRepositoryRefErrs) pw.SelfRepositoryResolutionErrs = selfScan.Errors pw.ParseWarnings = append(scan.Warnings, selfScan.Warnings...) - if len(pw.Refs) > 0 && store != nil { - wfKey := workflowfile.KeyFromPath(path) - deps, depsErr := store.Get(wfKey) - if depsErr != nil { - pw.DepsErr = depsErr - } else { - pw.ExistingDeps = deps - } - } out = append(out, pw) } return out diff --git a/internal/pipeline/verify_local.go b/internal/pipeline/verify_local.go index b9be0768..f92514b8 100644 --- a/internal/pipeline/verify_local.go +++ b/internal/pipeline/verify_local.go @@ -29,6 +29,7 @@ func VerifyLocalCoverage(parsed []checks.ParsedWorkflow, store *lockfile.State) return pw.Refs }(), SelfActionFiles: pw.SelfActionFiles, + SelfActionRefs: pw.SelfActionRefs, Deps: pw.ExistingDeps, }