Spell full SHAs one way in the lockfile (#213) · github/gh-actions-lock@d2a970a · GitHub
Skip to content

Commit d2a970a

Browse files
Spell full SHAs one way in the lockfile (#213)
* Spell full SHAs one way in the lockfile and workflows The Actions backend matches a `uses:` ref to its lockfile key exactly, so a full SHA must be spelled the same in both. Lockfile loads lowercase full SHAs in keys, refs, commits, `uses:` and workflow roots, merging duplicate spellings and rejecting ones that disagree. A workflow `uses:` with an uppercase full SHA is a fixable finding, and fix runs lowercase it. A remote composite that spells a child SHA in uppercase can't be locked. Also reject what the backend can't run: a ref containing `@` anywhere, and one repository locked on two hosts. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Match uppercase full-SHA uses to lowercase keys without rewriting them The Actions backend lowercases full-SHA refs before it looks them up, so a workflow can keep the spelling it was written with. Workflow refs are lowercased once, where they are parsed, and a planned rewrite of a full-SHA use is applied to the spelling the source has. Drop the uppercase finding and the uppercase-composite-child rejection. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Never record a ref with @, and merge spellings whatever their child order Reverse lookup could pick a tag or branch containing @, which the Actions backend rejects in `uses:`. Duplicate spellings of one entry now compare their children as the set Save writes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
1 parent 2a30e48 commit d2a970a

16 files changed

Lines changed: 368 additions & 50 deletions

File tree

‎cmd/gh-actions-lock/command_test.go‎

Lines changed: 84 additions & 1 deletion

‎internal/ghapi/refname.go‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,8 @@ func fullSHAShaped(s string) bool {
7272
}
7373

7474
// AcceptedRefName reports whether name is a ref name the Actions backend
75-
// accepts: a valid ref name that isn't shaped like `git describe` output.
75+
// accepts: a valid ref name that isn't shaped like `git describe` output and
76+
// has no @, which the backend's `uses:` grammar rejects.
7677
func AcceptedRefName(name string) bool {
77-
return ValidRefName(name) && !describeSuffix.MatchString(name)
78+
return ValidRefName(name) && !describeSuffix.MatchString(name) && !strings.Contains(name, "@")
7879
}

‎internal/ghapi/refname_test.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ func TestAcceptedRefName(t *testing.T) {
3535
{"v1-G1234", true}, // uppercase G
3636
{"v1-g1234-rc", true},
3737
{"main~1", false},
38+
{"rel@1", false},
3839
}
3940
for _, tt := range tests {
4041
assert.Equal(t, tt.want, AcceptedRefName(tt.name), tt.name)

‎internal/lockfile/convertor.go‎

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -11,9 +11,8 @@ import (
1111
"github.com/github/gh-actions-lock/internal/ghapi"
1212
)
1313

14-
// depToPin converts a dep.Dependency into a parserlock.Pin without any
15-
// case-normalization — callers should rely on Pin.String / Pin.Canonical for
16-
// the canonical form. Sub-action path is dropped: the lockfile pin grammar
14+
// depToPin converts a dep.Dependency into a parserlock.Pin with its full-SHA
15+
// ref spelled canonically; Pin.String folds owner/repo. Sub-action path is dropped: the lockfile pin grammar
1716
// identifies a downloaded tarball at repo+sha granularity, and distinct
1817
// subpaths in the same repo+ref collapse to one entry.
1918
func depToPin(d dep.Dependency) (parserlock.Pin, error) {
@@ -30,10 +29,20 @@ func depToPin(d dep.Dependency) (parserlock.Pin, error) {
3029
return parserlock.Pin{
3130
Owner: owner,
3231
Repo: repo,
33-
Ref: d.Ref,
32+
Ref: CanonicalRef(d.Ref),
3433
}, nil
3534
}
3635

36+
// CanonicalRef spells a full commit SHA in lowercase and returns any other
37+
// ref unchanged. The Actions backend matches a `uses:` ref to its lockfile
38+
// key exactly, so a full SHA has one spelling in both.
39+
func CanonicalRef(ref string) string {
40+
if parserlock.IsFullSha(ref) {
41+
return strings.ToLower(ref)
42+
}
43+
return ref
44+
}
45+
3746
// ValidEntryRef reports whether ref is a lock entry ref the Actions backend
3847
// resolver accepts for pin: a short tag or branch name it accepts (never a
3948
// SHA, a refs/ name, revision syntax like main~1, or a `git describe` name),

‎internal/lockfile/hosts_test.go‎

Lines changed: 26 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,7 @@ func TestHostScopedMetadataAndPinCollisions(t *testing.T) {
7373
require.NoError(t, store.SetHostname("tenant.ghe.com"))
7474
sha := strings.Repeat("a", 40)
7575
tenant := dep.Dependency{Hostname: "tenant.ghe.com", NWO: "o/r", Ref: "tenant", SHA: sha}
76-
public := dep.Dependency{Hostname: "github.com", NWO: "o/r", Ref: "public", SHA: sha}
76+
public := dep.Dependency{Hostname: "github.com", NWO: "o/q", Ref: "public", SHA: sha}
7777
require.NoError(t, store.Set(context.Background(), ".github/workflows/ci.yml", []dep.Dependency{tenant, public}, nil, nil))
7878
require.NoError(t, store.Save())
7979
reloaded, err := LoadState(dir, nil)
@@ -82,20 +82,37 @@ func TestHostScopedMetadataAndPinCollisions(t *testing.T) {
8282
file := reloaded.File()
8383
assert.Empty(t, file.Dependencies["o/r@tenant"].Hostname)
8484
assert.EqualValues(t, 20, file.Dependencies["o/r@tenant"].RepoID)
85-
assert.Equal(t, "github.com", file.Dependencies["o/r@public"].Hostname)
86-
assert.EqualValues(t, 2, file.Dependencies["o/r@public"].RepoID)
85+
assert.Equal(t, "github.com", file.Dependencies["o/q@public"].Hostname)
86+
assert.EqualValues(t, 2, file.Dependencies["o/q@public"].RepoID)
8787
deps, err := reloaded.Get(".github/workflows/ci.yml")
8888
require.NoError(t, err)
8989
assert.ElementsMatch(t, []string{"github.com", "tenant.ghe.com"}, []string{deps[0].Hostname, deps[1].Hostname})
9090
require.NoError(t, reloaded.Set(context.Background(), ".github/workflows/ci.yml", deps, nil, nil))
9191
assert.EqualValues(t, 20, reloaded.File().Dependencies["o/r@tenant"].RepoID)
92-
assert.EqualValues(t, 2, reloaded.File().Dependencies["o/r@public"].RepoID)
92+
assert.EqualValues(t, 2, reloaded.File().Dependencies["o/q@public"].RepoID)
9393

94-
public.Ref = tenant.Ref
95-
require.ErrorContains(t, store.Set(context.Background(), ".github/workflows/other.yml", []dep.Dependency{public}, nil, nil), "conflicting hosts")
96-
fresh, err := LoadState(t.TempDir(), hostMetadata{})
97-
require.NoError(t, err)
98-
require.ErrorContains(t, fresh.Set(context.Background(), ".github/workflows/ci.yml", []dep.Dependency{tenant, public}, nil, nil), "conflicting hosts")
94+
t.Run("rejects one pin on two hosts", func(t *testing.T) {
95+
moved := public
96+
moved.NWO, moved.Ref = tenant.NWO, tenant.Ref
97+
require.ErrorContains(t, store.Set(context.Background(), ".github/workflows/other.yml", []dep.Dependency{moved}, nil, nil), "conflicting hosts")
98+
})
99+
t.Run("rejects one repository on two hosts in a write", func(t *testing.T) {
100+
fresh, err := LoadState(t.TempDir(), hostMetadata{})
101+
require.NoError(t, err)
102+
other := public
103+
other.NWO = tenant.NWO
104+
require.ErrorContains(t, fresh.Set(context.Background(), ".github/workflows/ci.yml", []dep.Dependency{tenant, other}, nil, nil), "conflicting hosts")
105+
})
106+
t.Run("rejects a lockfile locking one repository on two hosts", func(t *testing.T) {
107+
raw, err := os.ReadFile(filepath.Join(dir, parserlock.Path))
108+
require.NoError(t, err)
109+
require.NoError(t, os.WriteFile(filepath.Join(dir, parserlock.Path), []byte(strings.ReplaceAll(string(raw), "o/q@", "o/r@")), 0o600))
110+
forged, err := LoadState(dir, nil)
111+
require.NoError(t, err)
112+
err = forged.SetHostname("tenant.ghe.com")
113+
require.ErrorIs(t, err, ErrCorruptLockfile)
114+
assert.ErrorContains(t, err, "o/r is locked on both github.com and tenant.ghe.com")
115+
})
99116
}
100117

101118
func TestLegacyAndOmittedHostnames(t *testing.T) {

‎internal/lockfile/state.go‎

Lines changed: 72 additions & 27 deletions

0 commit comments

Comments
 (0)