diff --git a/cmd/analyze.go b/cmd/analyze.go index 9248e8b93..05a76b9ab 100644 --- a/cmd/analyze.go +++ b/cmd/analyze.go @@ -69,7 +69,9 @@ GitHub authentication The token's hourly quota is usually shared (in Actions, with every other job in the repository), so the GitHub pass stops while half the quota is still left and reports what it did not fetch. --forge-window-days reads a shorter window from GitHub than - --window-days reads from git, which costs fewer requests. + --window-days reads from git, which costs fewer requests. A token whose quota is already + spent skips GitHub, reporting those metrics as not measured; the rest of the report is + unaffected. A repository whose remote is not GitHub skips the check automatically.`, Args: cobra.MaximumNArgs(1), diff --git a/internal/analyze/forge/auth.go b/internal/analyze/forge/auth.go index 0e53efbf4..ca9e6206c 100644 --- a/internal/analyze/forge/auth.go +++ b/internal/analyze/forge/auth.go @@ -13,6 +13,7 @@ package forge import ( "context" + "errors" "fmt" "net/http" "os" @@ -75,6 +76,16 @@ var ErrNoCredentials = fmt.Errorf(`no GitHub credentials found. Or run with --no-forge to skip these metrics entirely. They will be reported as "not measured" rather than zero — the report never claims we looked when we did not`) +// ErrRateLimited means the preflight could not run because the token's quota is spent. It is +// not an access problem: inside GitHub Actions the job token's quota belongs to the whole +// repository, and other jobs can spend it before analyze starts. A caller should skip the +// forge pass and report its metrics as not measured, not fail the whole analysis. +var ErrRateLimited = errors.New("the GitHub API rate limit for this token is exhausted") + +func rateLimitedError(creds *Credentials, err error) error { + return fmt.Errorf("%w (token from %s): %v", ErrRateLimited, creds.Source, err) +} + // repoAccessError explains a token that works but cannot see this repository. // // The raw error is "404 Not Found", which is a lie of omission: the repository exists, and @@ -172,11 +183,17 @@ func Check(ctx context.Context, creds *Credentials, host string, repo Repo) (*Ch user, _, err := client.gh.Users.Get(ctx, "") if err != nil { + if IsRateLimit(err) { + return nil, rateLimitedError(creds, err) + } if repo.Owner == "" || repo.Name == "" { return nil, fmt.Errorf("GitHub rejected the token from %s: %w", creds.Source, err) } if _, resp, rerr := client.gh.Repositories.Get(ctx, repo.Owner, repo.Name); rerr != nil { + if IsRateLimit(rerr) { + return nil, rateLimitedError(creds, rerr) + } return nil, repoAccessError(creds, "repository-scoped token", repo, resp, rerr) } @@ -197,6 +214,9 @@ func Check(ctx context.Context, creds *Credentials, host string, repo Repo) (*Ch if repo.Owner != "" && repo.Name != "" { if _, resp, rerr := client.gh.Repositories.Get(ctx, repo.Owner, repo.Name); rerr != nil { + if IsRateLimit(rerr) { + return nil, rateLimitedError(creds, rerr) + } return nil, repoAccessError(creds, user.GetLogin(), repo, resp, rerr) } } diff --git a/internal/analyze/forge/auth_check_test.go b/internal/analyze/forge/auth_check_test.go index 95f58eb6a..9fde2f58f 100644 --- a/internal/analyze/forge/auth_check_test.go +++ b/internal/analyze/forge/auth_check_test.go @@ -20,11 +20,23 @@ import ( type fakeGitHub struct { userStatus, repoStatus, rateStatus int calls []string + + // exhausted answers every endpoint but /rate_limit the way GitHub does once a token's + // quota is spent: 403, X-RateLimit-Remaining: 0, "API rate limit exceeded". + exhausted bool } func (f *fakeGitHub) ServeHTTP(w http.ResponseWriter, r *http.Request) { f.calls = append(f.calls, r.URL.Path) w.Header().Set("Content-Type", "application/json") + if f.exhausted && r.URL.Path != "/api/v3/rate_limit" { + w.Header().Set("X-RateLimit-Limit", "1000") + w.Header().Set("X-RateLimit-Remaining", "0") + w.Header().Set("X-RateLimit-Reset", fmt.Sprint(time.Now().Add(time.Hour).Unix())) + w.WriteHeader(http.StatusForbidden) + _, _ = fmt.Fprint(w, `{"message":"API rate limit exceeded for installation."}`) + return + } reply := func(status int, body string) { w.WriteHeader(status) if status == http.StatusOK { @@ -125,6 +137,39 @@ func TestCheck_UserTokenCannotSeeTheRepo(t *testing.T) { require.Contains(t, msg, "almost certainly\n an access problem") } +// A spent quota is "come back later", not an access problem. In GitHub Actions the job token's +// quota is the repository's, and other jobs can spend it first; reporting that as "cannot read +// Vulnetix/cli" sent the reader to check permissions that were fine. +func TestCheck_ExhaustedQuotaIsNotAnAccessError(t *testing.T) { + for _, repo := range []Repo{testRepo, {}} { + host := startFakeGitHub(t, &fakeGitHub{exhausted: true}) + + _, err := Check(context.Background(), testCreds, host, repo) + require.ErrorIs(t, err, ErrRateLimited, "repo %+v", repo) + require.NotContains(t, err.Error(), "cannot read") + require.Contains(t, err.Error(), "GH_TOKEN") + } +} + +// GITHUB_TOKEN cannot read /user (403, not a rate limit); the repository read is then the one +// that meets the spent quota. +func TestCheck_RepositoryScopedTokenWithExhaustedQuota(t *testing.T) { + f := &fakeGitHub{userStatus: 403, rateStatus: 200} + host := startFakeGitHub(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + f.exhausted = r.URL.Path == "/api/v3/repos/acme/widgets" + f.ServeHTTP(w, r) + })) + + _, err := Check(context.Background(), testCreds, host, testRepo) + require.ErrorIs(t, err, ErrRateLimited) + require.NotContains(t, err.Error(), "cannot read") +} + +func TestIsRateLimit_Wrapped(t *testing.T) { + require.True(t, IsRateLimit(fmt.Errorf("fetch: %w", &github.RateLimitError{}))) + require.False(t, IsRateLimit(fmt.Errorf("fetch: %w", errors.New("boom")))) +} + // The quota is informational: failing to read it does not fail the check. func TestCheck_RateLimitFailureIsNotFatal(t *testing.T) { host := startFakeGitHub(t, &fakeGitHub{userStatus: 200, repoStatus: 200, rateStatus: 500}) diff --git a/internal/analyze/forge/budget.go b/internal/analyze/forge/budget.go index 2b070525a..13f61d13e 100644 --- a/internal/analyze/forge/budget.go +++ b/internal/analyze/forge/budget.go @@ -18,6 +18,7 @@ package forge // reason. It is not a failure, and it is not a silence. import ( + "errors" "fmt" "sync" @@ -126,15 +127,8 @@ func (b *budget) Reason() string { // a real failure matters: one means "come back later", the other means something is broken, // and treating them the same makes both harder to diagnose. func IsRateLimit(err error) bool { - if err == nil { - return false - } - if _, ok := err.(*github.RateLimitError); ok { - return true - } - if _, ok := err.(*github.AbuseRateLimitError); ok { - return true - } + var rl *github.RateLimitError + var abuse *github.AbuseRateLimitError - return false + return errors.As(err, &rl) || errors.As(err, &abuse) } diff --git a/internal/analyze/run.go b/internal/analyze/run.go index 0e3da9a10..b6813d16a 100644 --- a/internal/analyze/run.go +++ b/internal/analyze/run.go @@ -8,6 +8,7 @@ package analyze import ( "context" + "errors" "fmt" "net/url" "os" @@ -165,6 +166,13 @@ func Check(ctx context.Context, opts Options) (*Preflight, error) { // unable to read a private repo it was never granted — and GitHub reports that as a 404, so // the failure looks like a typo rather than a permission. result, err := forge.Check(ctx, creds, host, forgeRepo) + if errors.Is(err, forge.ErrRateLimited) { + // The token is fine; its quota is spent, often by other jobs sharing it. That costs the + // forge metrics, which are then not measured, and never the rest of the report. + p.ForgeStatus = fmt.Sprintf("GitHub: skipped (%v). Pull-request, review and issue metrics will be reported as not measured.", err) + + return p, nil + } if err != nil { return nil, err }