Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion cmd/analyze.go
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
20 changes: 20 additions & 0 deletions internal/analyze/forge/auth.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ package forge

import (
"context"
"errors"
"fmt"
"net/http"
"os"
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
}

Expand All @@ -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)
}
}
Expand Down
45 changes: 45 additions & 0 deletions internal/analyze/forge/auth_check_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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})
Expand Down
14 changes: 4 additions & 10 deletions internal/analyze/forge/budget.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package forge
// reason. It is not a failure, and it is not a silence.

import (
"errors"
"fmt"
"sync"

Expand Down Expand Up @@ -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)
}
8 changes: 8 additions & 0 deletions internal/analyze/run.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

import (
"context"
"errors"
"fmt"
"net/url"
"os"
Expand Down Expand Up @@ -165,6 +166,13 @@
// 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
}
Expand All @@ -183,7 +191,7 @@

// Run produces the report. Every collector that runs, is skipped, or fails is recorded on the
// report — so a consumer can always tell a metric that is missing from a metric that is zero.
func Run(tool Tool, opts Options, pre *Preflight) (*Report, []byte, error) {

Check warning on line 194 in internal/analyze/run.go

View workflow job for this annotation

GitHub Actions / github-advisories

VNX-1048

This function calls 40 distinct functions or methods in 220 lines; a unit with that much fan-out does too many jobs to review or test as one, and a security check among them is easy to miss. Split it into smaller functions with one responsibility each

Check warning on line 194 in internal/analyze/run.go

View workflow job for this annotation

GitHub Actions / SAST

VNX-1048

This function calls 40 distinct functions or methods in 220 lines; a unit with that much fan-out does too many jobs to review or test as one, and a security check among them is easy to miss. Split it into smaller functions with one responsibility each
started := time.Now()
now := started

Expand Down
Loading