Preserve historical SHA pins during branch normalization (#165) · github/gh-actions-lock@d14124e · GitHub
Skip to content

Commit d14124e

Browse files
bayandinnodeselectorCopilot
authored
Preserve historical SHA pins during branch normalization (#165)
* Preserve historical SHA pins during branch normalization Only replace a bare SHA with a branch name when its head matches the pinned commit. Keep containing branches as metadata for older commits, both during reverse lookup and when repairing existing lockfile entries. * Test historical SHA pins during branch normalization Update the existing branch-repair test fixture to provide the branch head. Cover distinct historical commits on the same branch in fresh and existing lockfiles, including adding a workflow to existing inventory. Check that branch heads and exact tags still normalize to symbolic refs. --------- Co-authored-by: Alexander Bayandin <864213+bayandin@users.noreply.github.com> Co-authored-by: Jeff Martin <nodeselector@github.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
1 parent 9d7e72c commit d14124e

5 files changed

Lines changed: 146 additions & 7 deletions

File tree

‎internal/pin/normalization_test.go‎

Lines changed: 4 additions & 0 deletions

‎internal/pin/plan.go‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -638,8 +638,12 @@ func narrowVerifiedEntries(ctx context.Context, entries []Entry, opts PlanOption
638638
}
639639
if parserlock.IsFullSha(e.Ref) {
640640
newRef := e.Tag
641-
if newRef == "" {
642-
newRef = e.OnBranch
641+
if newRef == "" && e.OnBranch != "" && opts.Resolver != nil {
642+
// A containing branch cannot identify an older pinned commit.
643+
owner, repo := splitNWO(e.NWO)
644+
if head, ok := opts.Resolver.GetBranchHead(ctx, owner, repo, e.OnBranch); ok && strings.EqualFold(head.SHA, e.SHA) {
645+
newRef = e.OnBranch
646+
}
643647
}
644648
if newRef == "" {
645649
continue

‎internal/pin/plan_test.go‎

Lines changed: 76 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -418,7 +418,13 @@ func TestNarrowVerifiedEntries_StickyPrecision(t *testing.T) {
418418
})
419419

420420
t.Run("SHA metadata repairs to branch", func(t *testing.T) {
421-
tagger, _ := newTagger(t)
421+
tagger, reg := newTagger(t)
422+
reg.Register(httpmock.REST("GET", `repos/actions/checkout/git/ref/heads/main`),
423+
httpmock.JSONResponse(map[string]any{
424+
"ref": "refs/heads/main", "object": map[string]any{"sha": sha, "type": "commit"},
425+
}))
426+
resolver, err := resolve.New("github.com", pinpool.New(2, nil), resolve.WithTransport(reg))
427+
require.NoError(t, err)
422428
report := fastPathReport(sha)
423429
report.Inventory[0].Dep.Branch = "main"
424430
report.ActionRefs = []parserlock.ActionRef{{
@@ -427,7 +433,7 @@ func TestNarrowVerifiedEntries_StickyPrecision(t *testing.T) {
427433
Ref: sha,
428434
}}
429435

430-
result, err := planWorkflow(context.Background(), report, PlanOptions{Tagger: tagger}, func(string) {})
436+
result, err := planWorkflow(context.Background(), report, PlanOptions{Tagger: tagger, Resolver: resolver}, func(string) {})
431437
require.NoError(t, err)
432438

433439
require.Len(t, result.entries, 1)
@@ -506,6 +512,74 @@ func TestNarrowVerifiedEntries_StickyPrecision(t *testing.T) {
506512
})
507513
}
508514

515+
func TestPlanPreservesDifferentHistoricalSHAsOnSameBranch(t *testing.T) {
516+
const headSHA = "cccccccccccccccccccccccccccccccccccccccc"
517+
shas := []string{
518+
"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
519+
"bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb",
520+
}
521+
for _, tt := range []struct {
522+
name string
523+
locked int
524+
}{
525+
{name: "fresh lockfile"},
526+
{name: "existing lockfile", locked: 2},
527+
{name: "new workflow with existing lockfile", locked: 1},
528+
} {
529+
t.Run(tt.name, func(t *testing.T) {
530+
reg := &httpmock.Registry{}
531+
defer reg.Verify(t)
532+
reg.Register(httpmock.REST("GET", `repos/owner/action/git/ref/heads/main`),
533+
httpmock.JSONResponse(map[string]any{
534+
"ref": "refs/heads/main", "object": map[string]any{"sha": headSHA, "type": "commit"},
535+
}))
536+
if tt.locked < len(shas) {
537+
reg.Register(httpmock.REST("GET", `repos/owner/action$`),
538+
httpmock.JSONResponse(map[string]any{"default_branch": "main"}))
539+
reg.Register(httpmock.REST("GET", `repos/owner/action/tags`),
540+
httpmock.JSONResponse(httpmock.TagListResponse()))
541+
for _, sha := range shas[tt.locked:] {
542+
reg.Register(httpmock.REST("GET", `repos/owner/action/compare/`+sha+`\.\.\.`+headSHA),
543+
httpmock.JSONResponse(httpmock.CompareAncestorResponse(sha)))
544+
}
545+
}
546+
pool := pinpool.New(2, nil)
547+
resolver, err := resolve.New("github.com", pool, resolve.WithTransport(reg))
548+
require.NoError(t, err)
549+
report := &checks.Report{}
550+
for i, sha := range shas {
551+
d := dep.Dependency{Hostname: "github.com", NWO: "owner/action", Ref: sha, SHA: sha, Branch: "main"}
552+
resolver.SeedFromLockfile([]dep.Dependency{d})
553+
wr := checks.WorkflowReport{
554+
Path: ".github/workflows/" + sha + ".yml",
555+
ActionRefs: []parserlock.ActionRef{{Owner: "owner", Repo: "action", Ref: sha}},
556+
}
557+
if i < tt.locked {
558+
wr.Inventory = []checks.InventoryEntry{{Dep: d, File: wr.Path, Direct: true}}
559+
} else {
560+
wr.Findings = []checks.Finding{{Category: checks.NotPinned, Severity: checks.SeverityError}}
561+
}
562+
report.Workflows = append(report.Workflows, wr)
563+
}
564+
565+
record, err := Plan(context.Background(), report, PlanOptions{
566+
Resolver: resolver, Tagger: tag.NewListerForTest(t, reg), Pool: pool,
567+
})
568+
require.NoError(t, err)
569+
require.Len(t, record.Entries, len(shas))
570+
for i, entry := range record.Entries {
571+
assert.Equal(t, shas[i], entry.Ref)
572+
assert.Equal(t, shas[i], entry.SHA)
573+
assert.Equal(t, "main", entry.OnBranch)
574+
}
575+
require.Len(t, record.Workflows, len(shas))
576+
for _, wp := range record.Workflows {
577+
assert.Empty(t, wp.Rewrites)
578+
}
579+
})
580+
}
581+
}
582+
509583
func TestPlanNormalizationRespectsLockedTarget(t *testing.T) {
510584
const (
511585
oldSHA = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"

‎internal/resolve/reverse_lookup.go‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -252,8 +252,9 @@ type LookupIssue struct {
252252

253253
// ReverseLookup leaves symbolic refs unchanged and reverse-lookups bare SHA
254254
// refs via DiscoverContaining. For bare SHAs it populates dep.Tag and dep.Branch
255-
// and computes the canonical @ref. The ref priority is:
256-
// tag (semver-ish release) > protected branch > default branch > any branch.
255+
// and computes the canonical @ref. Exact tags take precedence over branch
256+
// heads. A containing branch remains metadata when its head differs from SHA,
257+
// so historical pins on the same branch keep distinct refs.
257258
// When the canonical ref differs from dep.Ref the change is recorded in
258259
// the returned rewrites map and dep.Ref is updated in place.
259260
//
@@ -300,7 +301,9 @@ func (r *Resolver) ReverseLookup(ctx context.Context, deps []dep.Dependency) (ma
300301
newRef = branch
301302
}
302303
if newRef == "" {
303-
newRef = branch
304+
if head, ok := r.GetBranchHead(ctx, owner, repo, branch); ok && strings.EqualFold(head.SHA, d.SHA) {
305+
newRef = branch
306+
}
304307
}
305308
if newRef == "" || newRef == d.Ref {
306309
continue

‎internal/resolve/reverse_lookup_test.go‎

Lines changed: 54 additions & 0 deletions

0 commit comments

Comments
 (0)