diff --git a/cmd/gh-actions-lock/format/terminal.go b/cmd/gh-actions-lock/format/terminal.go index 39bfeafc..5095c6f7 100644 --- a/cmd/gh-actions-lock/format/terminal.go +++ b/cmd/gh-actions-lock/format/terminal.go @@ -122,7 +122,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.Category) && !f.RefMissing { hasFixable = true } else if f.Severity == checks.SeverityError { blocked = true @@ -141,7 +141,7 @@ func PresentReadOnlyFailures(out *ui.UI, report *checks.Report) (hasFixable bool // prints a single non-valid finding to the terminal (bypassing the // narration log) so read-only modes surface the same detail as fix mode. func renderTermFindingDetail(out *ui.UI, f checks.Finding, dep string) { - label := categoryLabel(f.Category) + label := findingLabel(f) icon := out.TermYellow(ui.IconWarning) if f.Severity == checks.SeverityError { icon = out.TermRed(ui.IconError) @@ -154,7 +154,7 @@ func renderTermFindingDetail(out *ui.UI, f checks.Finding, dep string) { out.TermDetail(" ↳ %s", out.TermDim(DepReleaseURL(f.Dependency.Hostname, owner+"/"+repo, nil))) } } - if IsAlertedCategory(f.Category) && f.Remediation != "" { + if (IsAlertedCategory(f.Category) || f.RefMissing) && f.Remediation != "" { out.TermDetail(" %s %s", out.TermYellow(ui.IconWarning), f.Remediation) } if f.RecommendedTag != "" { @@ -174,6 +174,15 @@ func renderTermFindingDetail(out *ui.UI, f checks.Finding, dep string) { } } +// findingLabel is categoryLabel with a terminal-only title for refs that +// don't exist upstream, which share the not-pinned category in JSON. +func findingLabel(f checks.Finding) string { + if f.RefMissing { + return "Ref does not exist" + } + return categoryLabel(f.Category) +} + // categoryLabel returns a human-readable label for a finding category, // e.g. "repo-renamed" -> "Repository renamed". Falls back to a // hyphen-to-space title for unmapped categories so new slugs still read @@ -278,7 +287,7 @@ func renderErrorFindings(out *ui.UI, report *checks.Report, failedCount, checked // renderFindingDetail prints a single non-valid finding with its category // icon, detail text, remediation, and doc links. func renderFindingDetail(out *ui.UI, f checks.Finding, dep string) { - label := categoryLabel(f.Category) + label := findingLabel(f) icon := out.Yellow(ui.IconWarning) if f.Severity == checks.SeverityError { icon = out.Red(ui.IconError) @@ -291,7 +300,7 @@ func renderFindingDetail(out *ui.UI, f checks.Finding, dep string) { out.Detail(" ↳ %s", out.Dim(DepReleaseURL(f.Dependency.Hostname, owner+"/"+repo, nil))) } } - if IsAlertedCategory(f.Category) && f.Remediation != "" { + if (IsAlertedCategory(f.Category) || f.RefMissing) && f.Remediation != "" { out.Detail(" %s %s", out.Yellow(ui.IconWarning), f.Remediation) } if f.RecommendedTag != "" { diff --git a/cmd/gh-actions-lock/format/terminal_test.go b/cmd/gh-actions-lock/format/terminal_test.go index ae399808..a4aa0de3 100644 --- a/cmd/gh-actions-lock/format/terminal_test.go +++ b/cmd/gh-actions-lock/format/terminal_test.go @@ -661,6 +661,31 @@ func TestPresentReadOnlyFailures_LocalActionNotFixable(t *testing.T) { } } +// TestPresentReadOnlyFailures_MissingRefNotFixable keeps the "Re-run +// without --no-fix" hint off a ref that doesn't exist: it shares the fixable +// not-pinned category, but a re-run can't create the ref. +func TestPresentReadOnlyFailures_MissingRefNotFixable(t *testing.T) { + u, _ := newTestUI() + report := &checks.Report{ + Workflows: []checks.WorkflowReport{{ + Path: ".github/workflows/ci.yml", + Findings: []checks.Finding{{ + WorkflowPath: ".github/workflows/ci.yml", + Category: checks.NotPinned, + Severity: checks.SeverityError, + Confidence: checks.ConfidenceHigh, + Dependency: &dep.Dependency{NWO: "actions/checkout", Ref: "v999.0.0"}, + Detail: "ref v999.0.0 does not exist in actions/checkout", + RefMissing: true, + }}, + }}, + } + + if PresentReadOnlyFailures(u, report) { + t.Error("a missing ref is not auto-fixable; hasFixable should be false") + } +} + // TestPresentReadOnlyFailures_UnreachablePinShowsReleases verifies the // read-only renderer includes the upstream releases link for an // unreachable-pin finding, matching the detailed (fix-mode) renderer. diff --git a/cmd/gh-actions-lock/missing_ref_test.go b/cmd/gh-actions-lock/missing_ref_test.go new file mode 100644 index 00000000..1f6ca386 --- /dev/null +++ b/cmd/gh-actions-lock/missing_ref_test.go @@ -0,0 +1,117 @@ +package main + +import ( + "encoding/json" + "net/http" + "os" + "regexp" + "strings" + "testing" + + "github.com/github/gh-actions-lock/internal/ghapi/httpmock" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestMissingRefNamesTheCause proves a ref that does not exist fails with an +// error finding that names it, rather than a fixable ref-changed plus a +// reachability warning. +func TestMissingRefNamesTheCause(t *testing.T) { + refs := []struct { + name, ref, detail, remedy string + }{ + {"tag", "v999.0.0", "ref v999.0.0 does not exist in actions/checkout", + "point `uses:` at a tag or branch that exists"}, + {"commit", "7b403c9ec14bd3ae0bbf793c2bee8815a7ac920b", "commit 7b403c9ec14b does not exist or is not reachable in actions/checkout", + "pin a commit that exists in actions/checkout, or point `uses:` at a tag or branch"}, + {"short sha", "7b403c9", "ref 7b403c9 does not resolve in actions/checkout", + "if this is a commit, use the full 40-character SHA; otherwise point `uses:` at a tag or branch that exists"}, + } + for _, tt := range refs { + for _, proxy := range []bool{false, true} { + t.Run(tt.name+"/"+map[bool]string{false: "graphql", true: "dependabot proxy"}[proxy], func(t *testing.T) { + testMissingRef(t, tt.ref, tt.detail, tt.remedy, proxy) + }) + } + } +} + +func testMissingRef(t *testing.T, ref, detail, remedy string, proxy bool) { + t.Helper() + reg := &httpmock.Registry{} + // Each resolve pass (plan, then diagnosis) asks again, for both + // the JSON and the terminal run. + for range 6 { + registerMissingRef(reg, ref, proxy) + } + if proxy { + t.Setenv("GH_ACTIONS_LOCK_DEPENDABOT_PROXY", "1") + useDefaultTransport(t, reg) + } + path := writeTempWorkflow(t, ` +name: ci +on: push +jobs: + test: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4.2.2 +`, "actions/checkout@v4.2.2") + wf, err := os.ReadFile(path) + require.NoError(t, err) + wf = []byte(strings.Replace(string(wf), "@v4.2.2", "@"+ref, 1)) + require.NoError(t, os.WriteFile(path, wf, 0o600)) + lockBefore := readTempLockfilePins(t) + + stdout, _, err := runCommandWithHTTP(t, reg, + "--no-onboard", "--no-narrow", "--no-interactive", "--json=findings", path) + require.ErrorIs(t, err, errSilent) + var out struct { + Findings []struct { + Category string `json:"category"` + Severity string `json:"severity"` + Detail string `json:"detail"` + Remediation string `json:"remediation"` + DocURL string `json:"doc_url"` + } `json:"findings"` + } + require.NoError(t, json.Unmarshal([]byte(stdout), &out), stdout) + found := false + for _, f := range out.Findings { + if f.Category == "not-pinned" && f.Severity == "error" { + found = true + assert.Equal(t, detail, f.Detail) + assert.Equal(t, remedy, f.Remediation) + assert.Empty(t, f.DocURL, "the security-hardening page doesn't explain a missing ref") + } + } + assert.True(t, found, stdout) + assert.Equal(t, lockBefore, readTempLockfilePins(t)) + after, err := os.ReadFile(path) + require.NoError(t, err) + assert.Equal(t, string(wf), string(after)) + + _, stderr, err := runCommandWithHTTP(t, reg, + "--no-onboard", "--no-narrow", "--no-interactive", path) + require.ErrorIs(t, err, errSilent) + assert.Contains(t, stderr, "Ref does not exist actions/checkout@"+ref) + assert.Contains(t, stderr, detail) + assert.Contains(t, stderr, remedy) + assert.NotContains(t, stderr, "Re-run without") + assert.NotContains(t, stderr, "Not pinned") + assert.NotContains(t, stderr, "security-hardening") +} + +func registerMissingRef(reg *httpmock.Registry, ref string, proxy bool) { + if !proxy { + reg.Register(httpmock.GraphQL(`repository\(owner:`), httpmock.JSONResponse(map[string]any{ + "data": map[string]any{"a0": map[string]any{"nameWithOwner": "actions/checkout", "object": nil}}, + })) + return + } + 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}, + })) + reg.Register(httpmock.REST("GET", `^/repos/actions/checkout/commits/`+regexp.QuoteMeta(ref)+`$`), + httpmock.StatusResponse(http.StatusUnprocessableEntity)) +} diff --git a/cmd/gh-actions-lock/parity_test.go b/cmd/gh-actions-lock/parity_test.go index 37bc2823..e7780cac 100644 --- a/cmd/gh-actions-lock/parity_test.go +++ b/cmd/gh-actions-lock/parity_test.go @@ -584,3 +584,12 @@ jobs: assert.Equal(t, 2, countGraphQL(ct), "resolve + parity: %v", ct.urls) assert.Contains(t, readTempLockfilePins(t), "actions/checkout@v4.2.1") } + +// useDefaultTransport routes anonymous REST calls, which use +// http.DefaultTransport, through rt for the rest of the test. +func useDefaultTransport(t *testing.T, rt http.RoundTripper) { + t.Helper() + orig := http.DefaultTransport + http.DefaultTransport = rt + t.Cleanup(func() { http.DefaultTransport = orig }) +} diff --git a/internal/ghapi/errors.go b/internal/ghapi/errors.go index 7ae05407..b1b8370a 100644 --- a/internal/ghapi/errors.go +++ b/internal/ghapi/errors.go @@ -52,3 +52,13 @@ func IsRateLimited(err error) bool { return httpErr.StatusCode == http.StatusForbidden && httpErr.Headers.Get("X-RateLimit-Remaining") == "0" } + +// RefNotFoundError reports a ref that does not exist in a repository that +// does. Unlike a transport or rate-limit failure, retrying will not help. +type RefNotFoundError struct { + Owner, Repo, Ref string + Err error +} + +func (e *RefNotFoundError) Error() string { return e.Err.Error() } +func (e *RefNotFoundError) Unwrap() error { return e.Err } diff --git a/internal/ghapi/graphql_action_files.go b/internal/ghapi/graphql_action_files.go index 18598b0c..2b503a20 100644 --- a/internal/ghapi/graphql_action_files.go +++ b/internal/ghapi/graphql_action_files.go @@ -310,17 +310,19 @@ func parseActionFileResponse(data map[string]json.RawMessage, refs []ActionFileR if repo.Object == nil || repo.Object.OID == "" { n := len(ref.Ref) + var err error switch { case isHexString(ref.Ref) && (n == 40 || n == 64): // Full SHA that doesn't resolve — commit is unreachable/orphaned - results[idx].Err = fmt.Errorf("commit %s does not exist or is not reachable in %s/%s", + err = fmt.Errorf("commit %s does not exist or is not reachable in %s/%s", ref.Ref[:12], ref.Owner, ref.Repo) case isHexString(ref.Ref): // Short hex — ambiguous, might be a truncated SHA - results[idx].Err = fmt.Errorf("version %q does not resolve — if this is a commit, use the full 40-character SHA", ref.Ref) + err = fmt.Errorf("version %q does not resolve — if this is a commit, use the full 40-character SHA", ref.Ref) default: - results[idx].Err = fmt.Errorf("version %q does not exist", ref.Ref) + err = fmt.Errorf("version %q does not exist", ref.Ref) } + results[idx].Err = &RefNotFoundError{Owner: ref.Owner, Repo: ref.Repo, Ref: ref.Ref, Err: err} continue } diff --git a/internal/ghapi/rest_fallback.go b/internal/ghapi/rest_fallback.go index 261ac476..81f8cfa4 100644 --- a/internal/ghapi/rest_fallback.go +++ b/internal/ghapi/rest_fallback.go @@ -3,6 +3,7 @@ package ghapi import ( "context" "encoding/json" + "errors" "fmt" "io" "net/http" @@ -338,6 +339,10 @@ func (c *Client) resolveAnonymous(ctx context.Context, ref ActionFileRequest) Ac ) sha, err := c.anonGetCommitSHA(ctx, commitURL) if err != nil { + var missing *RefNotFoundError + if errors.As(err, &missing) { + missing.Owner, missing.Repo, missing.Ref = ref.Owner, ref.Repo, ref.Ref + } result.Err = fmt.Errorf("anonymous fallback: %w", err) return result } @@ -386,6 +391,9 @@ func (c *Client) anonGetCommitSHA(ctx context.Context, url string) (string, erro } defer resp.Body.Close() + if resp.StatusCode == http.StatusNotFound || resp.StatusCode == http.StatusUnprocessableEntity { + return "", &RefNotFoundError{Err: fmt.Errorf("HTTP %d resolving commit", resp.StatusCode)} + } if resp.StatusCode != http.StatusOK { return "", fmt.Errorf("HTTP %d resolving commit", resp.StatusCode) } diff --git a/internal/pipeline/checks/finding.go b/internal/pipeline/checks/finding.go index b33807d1..d30e159e 100644 --- a/internal/pipeline/checks/finding.go +++ b/internal/pipeline/checks/finding.go @@ -47,6 +47,10 @@ type Finding struct { // finding (regardless of outcome). Lets renderers distinguish // "we didn't look" from "we looked and found nothing." RecommendedSearched bool + // RefMissing marks a not-pinned finding whose ref definitively does not + // exist upstream. Terminal output titles it separately; the JSON + // category stays not-pinned. + RefMissing bool } // InventoryEntry describes a single dependency with context. diff --git a/internal/pipeline/diagnose.go b/internal/pipeline/diagnose.go index 2e42412b..4ad89d4f 100644 --- a/internal/pipeline/diagnose.go +++ b/internal/pipeline/diagnose.go @@ -101,6 +101,20 @@ func diagnoseOneParsed(ctx context.Context, pw checks.ParsedWorkflow, r *resolve }) blockingResolverError = true } + for _, missing := range missingRefs(resolveErr) { + nwo := missing.Owner + "/" + missing.Repo + wr.Findings = append(wr.Findings, checks.Finding{ + WorkflowPath: pw.Path, + Category: checks.NotPinned, + Severity: checks.SeverityError, + Confidence: checks.ConfidenceHigh, + Dependency: &dep.Dependency{NWO: nwo, Ref: missing.Ref}, + Detail: missingRefDetail(missing.Ref, nwo), + Remediation: missingRefRemediation(missing.Ref, nwo), + RefMissing: true, + }) + blockingResolverError = true + } if blockingResolverError { wr.BlockingResolverError = true return wr @@ -332,3 +346,48 @@ func hasIssues(ff []checks.Finding) bool { } return false } + +// missingRefs collects every ref the resolver definitively failed to find. +func missingRefs(err error) []*ghapi.RefNotFoundError { + switch e := err.(type) { + case nil: + return nil + case *ghapi.RefNotFoundError: + return []*ghapi.RefNotFoundError{e} + case interface{ Unwrap() []error }: + var out []*ghapi.RefNotFoundError + for _, inner := range e.Unwrap() { + out = append(out, missingRefs(inner)...) + } + return out + } + return missingRefs(errors.Unwrap(err)) +} + +// missingRefDetail names a full commit SHA by its short form so the message +// reads like the rest of the CLI's commit references. +func missingRefDetail(ref, nwo string) string { + switch { + case isFullSHA(ref): + return fmt.Sprintf("commit %s does not exist or is not reachable in %s", ref[:12], nwo) + case isHex(ref): + return fmt.Sprintf("ref %s does not resolve in %s", ref, nwo) + } + return fmt.Sprintf("ref %s does not exist in %s", ref, nwo) +} + +// missingRefRemediation keeps the short-SHA hint: a truncated commit is a +// different mistake from a tag that was never published. +func missingRefRemediation(ref, nwo string) string { + switch { + case isFullSHA(ref): + return fmt.Sprintf("pin a commit that exists in %s, or point `uses:` at a tag or branch", nwo) + case isHex(ref): + return "if this is a commit, use the full 40-character SHA; otherwise point `uses:` at a tag or branch that exists" + } + return "point `uses:` at a tag or branch that exists" +} + +func isFullSHA(ref string) bool { + return (len(ref) == 40 || len(ref) == 64) && isHex(ref) +} diff --git a/test/scenarios/catalog.yml b/test/scenarios/catalog.yml index f2197774..f8769729 100644 --- a/test/scenarios/catalog.yml +++ b/test/scenarios/catalog.yml @@ -1965,7 +1965,7 @@ scenarios: - name: dbot_impostor_blocks category: dependabot - description: "Orphaned commit (no tag or branch) produces reachability-unknown warning in JSON findings" + description: "Commit that does not exist upstream blocks with a not-pinned error" needs_token: true tags: [smoke] flags: ["--no-onboard", "--no-narrow", "--no-interactive", "--no-fix", "--json=findings"] @@ -1975,14 +1975,16 @@ scenarios: name: CI actions: ["nodeselector/actions-test-fixtures@7b403c9ec14bd3ae0bbf793c2bee8815a7ac920b"] expect: - exit: 0 + exit: 1 stdout_is_json: true - stdout_contains: ["does not exist or is not reachable"] + stdout_contains: ["does not exist or is not reachable in nodeselector/actions-test-fixtures"] jq: - expr: '.findings | length' gt: 0 - expr: '.findings[0].category' - equals: 'reachability-unknown' + equals: 'not-pinned' + - expr: '.findings[0].severity' + equals: 'error' - name: dbot_forgery_blocks category: dependabot