Verify locked manifests in online runs that keep recorded pins (#212) · github/gh-actions-lock@748cba9 · GitHub
Skip to content

Commit 748cba9

Browse files
nodeselectorCopilot
nodeselector
and
Copilot
authored
Verify locked manifests in online runs that keep recorded pins (#212)
* Verify locked manifests in online runs that keep recorded pins Read each locked action's manifest at its locked commit and block when a composite uses an action its lockfile entry does not record, or when the manifest is missing. Reads are batched across workflows, one request per nesting level, and skipped where this run already read the manifest live. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Block on an unread locked manifest as a retryable error A manifest that can't be read leaves its dependencies unverified, so it blocks like any unanswered check: exit 1 with retryable set in --json, and an abort behind the Dependabot proxy. Retryable is added with the same name and JSON key as the retry work that introduces it elsewhere. Locked-manifest reads carry their own GraphQL label. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Address locked-manifest review: Dockerfiles, seeds, category - Accept a Dockerfile, the runner's fallback, where a locked action has no action.yml or action.yaml. - Treat any failed read as unverified, including a missing commit under a reused seed that parity never checked. - Report manifest mismatches as incomplete-lockfile, which a plain run doesn't repair, with its own label and remediation. - Walk seeded children reached through a live composite. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
1 parent d2a970a commit 748cba9

25 files changed

Lines changed: 991 additions & 88 deletions

‎README.md‎

Lines changed: 7 additions & 0 deletions

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

Lines changed: 94 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -11,12 +11,14 @@ import (
1111
"os"
1212
"path/filepath"
1313
"strings"
14+
"sync/atomic"
1415
"testing"
1516

1617
parserlock "github.com/github/actions-lockfile/go/pkg/lockfile"
1718
"github.com/github/gh-actions-lock/cmd/gh-actions-lock/format"
1819
"github.com/github/gh-actions-lock/internal/ghapi/httpmock"
1920
"github.com/github/gh-actions-lock/internal/pinpool"
21+
"github.com/github/gh-actions-lock/internal/profile"
2022
"github.com/github/gh-actions-lock/internal/resolve"
2123
"github.com/stretchr/testify/assert"
2224
"github.com/stretchr/testify/require"
@@ -231,9 +233,90 @@ func testRepoResponse(nameWithOwner, oid, actionYAML string) map[string]any {
231233
}
232234
}
233235

234-
// registerParity stubs one parity query that reports every pin intact.
236+
// registerParity stubs one parity query that reports every pin intact, and
237+
// answers locked-manifest reads with node actions.
235238
func registerParity(reg *httpmock.Registry) {
236239
reg.Register(httpmock.GraphQL(`commit: object\(oid`), parityOK)
240+
registerLockedManifests(reg, nil)
241+
}
242+
243+
// lockedManifestRead matches a GraphQL read of locked manifests at their
244+
// commits.
245+
func lockedManifestRead(req *http.Request) bool {
246+
return req.Method == http.MethodPost && profile.GraphQLLabel(req.Context()) == "manifests"
247+
}
248+
249+
// registerLockedManifests answers locked-manifest reads from manifests,
250+
// keyed by owner/repo[/path]; any other action is a node action. It
251+
// returns the number of requests served.
252+
func registerLockedManifests(reg *httpmock.Registry, manifests map[string]string) *atomic.Int64 {
253+
var reads atomic.Int64
254+
respond := actionFiles(manifests, nil)
255+
reg.RegisterReusable(lockedManifestRead, func(req *http.Request) (*http.Response, error) {
256+
reads.Add(1)
257+
return respond(req)
258+
})
259+
return &reads
260+
}
261+
262+
// Manifests that stand for an action with only a Dockerfile, and for a
263+
// commit the repository no longer has.
264+
const (
265+
dockerfileOnly = "FROM alpine"
266+
missingCommit = "missing commit"
267+
)
268+
269+
// actionFiles answers action-file reads from manifests. A ref that is not a
270+
// full SHA resolves to commits[owner/repo].
271+
func actionFiles(manifests, commits map[string]string) httpmock.Responder {
272+
return func(req *http.Request) (*http.Response, error) {
273+
var body struct {
274+
Variables map[string]any `json:"variables"`
275+
}
276+
if err := json.NewDecoder(req.Body).Decode(&body); err != nil {
277+
return nil, err
278+
}
279+
data := map[string]any{}
280+
for i := 0; ; i++ {
281+
owner, ok := body.Variables[fmt.Sprintf("owner%d", i)].(string)
282+
if !ok {
283+
break
284+
}
285+
nwo := owner + "/" + body.Variables[fmt.Sprintf("name%d", i)].(string)
286+
name := nwo
287+
if yml := body.Variables[fmt.Sprintf("yml%d", i)].(string); yml != "action.yml" {
288+
name += "/" + strings.TrimSuffix(yml, "/action.yml")
289+
}
290+
text, ok := manifests[name]
291+
if !ok {
292+
text = nodeActionYAML
293+
}
294+
sha := strings.TrimSuffix(body.Variables[fmt.Sprintf("expr%d", i)].(string), "^{commit}")
295+
if !parserlock.IsFullSha(sha) {
296+
sha = commits[nwo]
297+
}
298+
resp := testRepoResponse(nwo, sha, text)
299+
switch text {
300+
case dockerfileOnly:
301+
resp["object"] = map[string]any{"oid": sha, "dockerfile": map[string]any{"oid": sha}}
302+
case missingCommit:
303+
resp["object"] = nil
304+
}
305+
data[fmt.Sprintf("a%d", i)] = resp
306+
}
307+
return httpmock.JSONResponse(map[string]any{"data": data})(req)
308+
}
309+
}
310+
311+
// registerLockedManifestsREST answers the REST-only reads of a manifest at
312+
// a full SHA: the commit lookup, then a node action's action.yml.
313+
func registerLockedManifestsREST(reg *httpmock.Registry) {
314+
reg.RegisterReusable(httpmock.REST("GET", `/commits/[0-9a-f]{40}$`), func(req *http.Request) (*http.Response, error) {
315+
return httpmock.JSONResponse(map[string]any{"sha": req.URL.Path[strings.LastIndex(req.URL.Path, "/")+1:]})(req)
316+
})
317+
reg.RegisterReusable(httpmock.RESTWithQuery("GET", `/contents/(.+/)?action\.yml$`, "ref="), func(*http.Request) (*http.Response, error) {
318+
return &http.Response{StatusCode: http.StatusOK, Body: io.NopCloser(strings.NewReader(nodeActionYAML)), Header: http.Header{}}, nil
319+
})
237320
}
238321

239322
// parityOK answers a parity query as if every locked pin still matches.
@@ -285,12 +368,18 @@ func parityOKWithIDs(ids map[string]int) httpmock.Responder {
285368
}
286369
}
287370

288-
// parityFallback answers parity queries the test did not stub as intact,
289-
// since fresh pins fail closed on an inconclusive check.
371+
// parityFallback answers parity queries and locked-manifest reads the test
372+
// did not stub as intact, since both fail closed when unanswered.
290373
type parityFallback struct{ rt http.RoundTripper }
291374

292375
func (p parityFallback) RoundTrip(req *http.Request) (*http.Response, error) {
293-
if req.Body == nil || !httpmock.GraphQL(`commit: object\(oid`)(req) {
376+
fallback := httpmock.Responder(parityOK)
377+
switch {
378+
case req.Body == nil:
379+
return p.rt.RoundTrip(req)
380+
case lockedManifestRead(req):
381+
fallback = actionFiles(nil, nil)
382+
case !httpmock.GraphQL(`commit: object\(oid`)(req):
294383
return p.rt.RoundTrip(req)
295384
}
296385
body, err := io.ReadAll(req.Body)
@@ -302,7 +391,7 @@ func (p parityFallback) RoundTrip(req *http.Request) (*http.Response, error) {
302391
return resp, nil
303392
}
304393
req.Body = io.NopCloser(bytes.NewReader(body))
305-
return parityOK(req)
394+
return fallback(req)
306395
}
307396

308397
// writeTempWorkflow writes a workflow YAML body to a scratch repo at

‎cmd/gh-actions-lock/format/json.go‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,9 @@ type Finding struct {
7171
Detail string `json:"detail"`
7272
Remediation string `json:"remediation,omitempty"`
7373
DocURL string `json:"doc_url,omitempty"`
74+
// Retryable marks a check GitHub didn't answer (an outage or rate
75+
// limit): the finding blocks, but re-running may clear it.
76+
Retryable bool `json:"retryable,omitempty"`
7477
}
7578

7679
// Dependency is the JSON-safe view of a resolved dependency, deduplicated
@@ -120,6 +123,7 @@ func findingFromReport(f checks.Finding) Finding {
120123
Detail: f.Detail,
121124
Remediation: f.Remediation,
122125
DocURL: f.DocURL,
126+
Retryable: f.Retryable,
123127
}
124128
if f.Dependency != nil {
125129
jf.Dependency = f.Dependency.Key()

‎cmd/gh-actions-lock/format/terminal.go‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -222,6 +222,8 @@ func categoryLabel(c checks.Category) string {
222222
return "Repository missing or inaccessible"
223223
case checks.Stale:
224224
return "Unused lockfile entry"
225+
case checks.IncompleteLockfile:
226+
return "Lockfile doesn't match locked manifest"
225227
}
226228
return strings.ReplaceAll(string(c), "-", " ")
227229
}
@@ -480,7 +482,7 @@ func renderRedirects(out *ui.UI, report *checks.Report, willRemediate bool) {
480482
// remediator should not re-print it in non-interactive mode).
481483
func IsAlertedCategory(c checks.Category) bool {
482484
switch c {
483-
case checks.UnreachablePin, checks.MisleadingSHA, checks.OnboardingRequired, checks.RepoRenamed, checks.RepoReplaced, checks.RepoUnavailable:
485+
case checks.UnreachablePin, checks.MisleadingSHA, checks.OnboardingRequired, checks.RepoRenamed, checks.RepoReplaced, checks.RepoUnavailable, checks.IncompleteLockfile:
484486
return true
485487
}
486488
return false

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

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -469,7 +469,8 @@ jobs:
469469

470470
// TestParity_UnchangedRerunRequestCount guards API kindness: an unchanged
471471
// rerun re-resolves nothing and costs one GraphQL request for the whole
472-
// closure, ref lookups included. A mutable ref that moved upstream stays
472+
// closure, ref lookups included, plus one batch reading the locked
473+
// manifests at their commits. A mutable ref that moved upstream stays
473474
// locked (sticky) and adds one compare call.
474475
func TestParity_UnchangedRerunRequestCount(t *testing.T) {
475476
reg := &httpmock.Registry{}
@@ -482,16 +483,18 @@ func TestParity_UnchangedRerunRequestCount(t *testing.T) {
482483
p, err := runParity(t, ct, args...)
483484
require.NoError(t, err)
484485
assert.True(t, p.Valid, "%+v", p.Findings)
485-
assert.Equal(t, 1, ct.n, "%v: one parity request for every pin", args)
486+
assert.Equal(t, 2, ct.n, "%v: one parity request and one manifest batch for every pin", args)
486487
}
487488
}
488489

489490
// TestParity_UnchangedRerunRequestCountRESTOnly covers the Dependabot proxy
490-
// path: one repos/{owner}/{repo} call per unique repository plus one commits
491-
// call per ref that still points at its locked commit.
491+
// path: one repos/{owner}/{repo} call per unique repository, one ref call per
492+
// ref that still points at its locked commit, and a commit and a contents
493+
// call per manifest at a distinct commit.
492494
func TestParity_UnchangedRerunRequestCountRESTOnly(t *testing.T) {
493495
t.Setenv("GH_ACTIONS_LOCK_DEPENDABOT_PROXY", "1")
494496
reg := &httpmock.Registry{}
497+
registerLockedManifestsREST(reg)
495498
defer reg.Verify(t)
496499
// Anonymous REST calls use http.DefaultClient.
497500
ct := &countingTransport{inner: reg}
@@ -512,7 +515,7 @@ func TestParity_UnchangedRerunRequestCountRESTOnly(t *testing.T) {
512515
p, err := runParity(t, ct, path)
513516
require.NoError(t, err)
514517
assert.True(t, p.Valid, "%+v", p.Findings)
515-
assert.Equal(t, 5, ct.n, "2 repos + 3 pins: %v", ct.urls)
518+
assert.Equal(t, 9, ct.n, "2 repos + 3 pins + 2 manifests: %v", ct.urls)
516519
}
517520

518521
// countingTransport counts every request, matched or not.
@@ -821,6 +824,7 @@ jobs:
821824
func TestParity_DependabotProxyKeepsRenamedName(t *testing.T) {
822825
t.Setenv("GH_ACTIONS_LOCK_DEPENDABOT_PROXY", "1")
823826
reg := &httpmock.Registry{}
827+
registerLockedManifestsREST(reg)
824828
reg.Register(httpmock.REST("GET", `^/repos/krzema12/github-actions-typing$`), httpmock.JSONResponse(map[string]any{
825829
"full_name": "typesafegithub/github-actions-typing", "id": 1, "owner": map[string]any{"id": 42},
826830
}))
@@ -897,6 +901,7 @@ func TestParity_VerifyMutableRefHistory(t *testing.T) {
897901
for _, proxy := range []bool{false, true} {
898902
t.Run(tt.name+"/"+map[bool]string{false: "graphql", true: "dependabot proxy"}[proxy], func(t *testing.T) {
899903
reg := &httpmock.Registry{}
904+
registerLockedManifestsREST(reg)
900905
if proxy {
901906
t.Setenv("GH_ACTIONS_LOCK_DEPENDABOT_PROXY", "1")
902907
useDefaultTransport(t, reg)
@@ -962,6 +967,7 @@ jobs:
962967
for _, proxy := range []bool{false, true} {
963968
t.Run("plain run makes one compare call/"+map[bool]string{false: "graphql", true: "dependabot proxy"}[proxy], func(t *testing.T) {
964969
reg := &httpmock.Registry{}
970+
registerLockedManifestsREST(reg)
965971
if proxy {
966972
t.Setenv("GH_ACTIONS_LOCK_DEPENDABOT_PROXY", "1")
967973
useDefaultTransport(t, reg)
@@ -1132,6 +1138,7 @@ func TestParity_VerifyRefCaseSensitive(t *testing.T) {
11321138
for _, proxy := range []bool{false, true} {
11331139
t.Run(map[bool]string{false: "graphql", true: "dependabot proxy"}[proxy], func(t *testing.T) {
11341140
reg := &httpmock.Registry{}
1141+
registerLockedManifestsREST(reg)
11351142
var heads []string
11361143
if proxy {
11371144
t.Setenv("GH_ACTIONS_LOCK_DEPENDABOT_PROXY", "1")

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -294,6 +294,9 @@ func TestProximaPrunesBeforeVerifyingHosts(t *testing.T) {
294294
staleCalls++
295295
return httpmock.StatusResponse(http.StatusNotFound)(req)
296296
case "/graphql":
297+
if lockedManifestRead(req) {
298+
return actionFiles(nil, nil)(req)
299+
}
297300
if tt.mismatch {
298301
return parityRepoID(req, http.StatusOK, 2)
299302
}

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

Lines changed: 1 addition & 0 deletions

0 commit comments

Comments
 (0)