From 80df5aa27efb52ab261bf8cc622a992394ec4f87 Mon Sep 17 00:00:00 2001 From: Gustavo Bertoi Date: Mon, 24 Aug 2026 01:00:40 -0300 Subject: [PATCH] fix(selfupdate): report a rate-limited 403 as rate limiting, not a private repo MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GitHub answers an exhausted API quota with a plain 403 Forbidden, which is indistinguishable from a permissions failure unless you read the response headers. Both `self update` and install.sh assumed the permissions case for every 403 and told the user to "set GITHUB_TOKEN if the repo is private" — so a user on a PUBLIC repo who had merely used up the anonymous 60-requests/hour quota went hunting for a permissions problem that did not exist, while the real cause (and the fact that it clears itself in minutes) stayed hidden. The distinguishing signal is X-RateLimit-Remaining: 0, plus Retry-After for secondary limits. apiError now separates four cases: rate limited -> names the limit that was hit, says plainly that it is NOT a permissions problem, gives the reset countdown, and still offers GITHUB_TOKEN as the fix (it raises the cap to 5000/h) 401 -> the token is set but was rejected 404 -> keeps the private-repo hint, which is where it belongs other -> the bare status Before: Github API https://api.github.com/... returned 403 Forbidden (set GITHUB_TOKEN if the repo is private). After: GitHub API rate limit exceeded (60 requests/hour), resets in 10m0s. This is not a permissions problem — the repository is reachable, you have simply used up the anonymous quota for your IP. Set GITHUB_TOKEN (or GH_TOKEN) to raise the limit to 5000 requests/hour: Applied to both the API path (selfupdate.go) and the asset-download path (update.go), since the reported failure hit the asset download first. install.sh cannot read the headers — its api() helper uses `curl -f`, which discards the error body — so it probes /rate_limit (which does not count against the quota) and appends the same clarification when the quota is spent. The reset countdown is omitted rather than rendered when the stamp is in the past or unparseable, so it can never print "resets in -3m0s". Co-Authored-By: Claude Opus 5 (1M context) --- install.sh | 18 ++++- internal/selfupdate/apierror.go | 89 +++++++++++++++++++++ internal/selfupdate/apierror_test.go | 113 +++++++++++++++++++++++++++ internal/selfupdate/selfupdate.go | 2 +- internal/selfupdate/update.go | 2 +- 5 files changed, 221 insertions(+), 3 deletions(-) create mode 100644 internal/selfupdate/apierror.go create mode 100644 internal/selfupdate/apierror_test.go diff --git a/install.sh b/install.sh index 8fcc544..d67fc07 100755 --- a/install.sh +++ b/install.sh @@ -51,6 +51,22 @@ else fi have tar || die "need tar to unpack the release archive" +# rate_limit_note explains an exhausted GitHub API quota. +# +# GitHub answers a spent anonymous quota with 403 Forbidden, which is +# indistinguishable from a permissions failure unless you look at the limit +# itself. Without this, a PUBLIC repo that is merely rate-limited reports as +# "if the repo is private", sending people to hunt a problem that is not there. +# /rate_limit does not itself count against the quota. +rate_limit_note() { + rl="$(dl "https://api.github.com/rate_limit" 2>/dev/null || true)" + case "$rl" in + *'"remaining":0'* | *'"remaining": 0'*) + printf '%s' " The GitHub API rate limit for your IP is exhausted — this is NOT a permissions problem. Set GITHUB_TOKEN to raise the limit to 5000/hour, or wait for the window to reset." + ;; + esac +} + # extract_tag pulls the first tag_name out of a GitHub releases JSON payload. extract_tag() { grep '"tag_name"' | head -n1 | sed -E 's/.*"tag_name":[[:space:]]*"([^"]+)".*/\1/'; } @@ -77,7 +93,7 @@ if [ -z "$tag" ]; then # pre-release-only repos and the brief post-publish API propagation window). tag="$(api "${API}/releases/latest" 2>/dev/null | extract_tag || true)" [ -n "$tag" ] || tag="$(api "${API}/releases" 2>/dev/null | extract_tag || true)" - [ -n "$tag" ] || die "could not determine the latest release. Pin one with DEVSTACK_VERSION=vX.Y.Z, and if the repo is private set GITHUB_TOKEN." + [ -n "$tag" ] || die "could not determine the latest release. Pin one with DEVSTACK_VERSION=vX.Y.Z, and set GITHUB_TOKEN if the repo is private.$(rate_limit_note)" fi # goreleaser strips the leading 'v' from the archive filename's version field. version="${tag#v}" diff --git a/internal/selfupdate/apierror.go b/internal/selfupdate/apierror.go new file mode 100644 index 0000000..1527ee8 --- /dev/null +++ b/internal/selfupdate/apierror.go @@ -0,0 +1,89 @@ +package selfupdate + +import ( + "fmt" + "net/http" + "strconv" + "time" +) + +// nowFn is swappable so the reset-countdown wording is testable. +var nowFn = time.Now + +// apiError turns a non-200 GitHub response into an error that names the ACTUAL +// cause. +// +// GitHub reports an exhausted rate limit as a plain 403, which is +// indistinguishable from a permissions failure unless you read the headers. The +// previous message assumed the permissions case for every 403 and told the user +// to "set GITHUB_TOKEN if the repo is private" — so a user hitting the +// unauthenticated 60-requests/hour cap on a PUBLIC repo went looking for a +// permissions problem that did not exist. +// +// The distinguishing signal is X-RateLimit-Remaining: 0. Setting a token is +// still the right advice when rate-limited (it raises the cap to 5000/hour), but +// the reason and the wait time matter more than the guess about visibility. +func apiError(url string, resp *http.Response) error { + if isRateLimited(resp) { + limit := resp.Header.Get("X-RateLimit-Limit") + if limit == "" { + limit = "the anonymous" + } else { + limit += " requests/hour" + } + return fmt.Errorf( + "GitHub API rate limit exceeded (%s)%s.\n"+ + "This is not a permissions problem — the repository is reachable, you have simply "+ + "used up the anonymous quota for your IP.\n"+ + "Set GITHUB_TOKEN (or GH_TOKEN) to raise the limit to 5000 requests/hour: %s", + limit, resetHint(resp), url) + } + switch resp.StatusCode { + case http.StatusUnauthorized: + return fmt.Errorf("GitHub API %s returned %s — GITHUB_TOKEN is set but was rejected; "+ + "check that it is valid and not expired", url, resp.Status) + case http.StatusNotFound: + return fmt.Errorf("GitHub API %s returned %s (set GITHUB_TOKEN if the repository is private)", + url, resp.Status) + } + return fmt.Errorf("GitHub API %s returned %s", url, resp.Status) +} + +// isRateLimited reports whether a response is a rate-limit rejection. GitHub uses +// 403 for the primary limit and 429 for secondary limits; both carry a zeroed +// X-RateLimit-Remaining, and a secondary limit may carry only Retry-After. +func isRateLimited(resp *http.Response) bool { + if resp.StatusCode != http.StatusForbidden && resp.StatusCode != http.StatusTooManyRequests { + return false + } + if resp.Header.Get("X-RateLimit-Remaining") == "0" { + return true + } + return resp.Header.Get("Retry-After") != "" +} + +// resetHint renders ", resets in 9m30s" when the response says when the window +// rolls over, and "" when it does not — never a bare or negative duration. +func resetHint(resp *http.Response) string { + if ra := resp.Header.Get("Retry-After"); ra != "" { + if secs, err := strconv.Atoi(ra); err == nil && secs > 0 { + return fmt.Sprintf(", retry in %s", (time.Duration(secs) * time.Second).String()) + } + } + reset := resp.Header.Get("X-RateLimit-Reset") + if reset == "" { + return "" + } + epoch, err := strconv.ParseInt(reset, 10, 64) + if err != nil { + return "" + } + d := time.Until(time.Unix(epoch, 0)).Round(time.Second) + if nowFn != nil { + d = time.Unix(epoch, 0).Sub(nowFn()).Round(time.Second) + } + if d <= 0 { + return "" + } + return fmt.Sprintf(", resets in %s", d.String()) +} diff --git a/internal/selfupdate/apierror_test.go b/internal/selfupdate/apierror_test.go new file mode 100644 index 0000000..fda89ee --- /dev/null +++ b/internal/selfupdate/apierror_test.go @@ -0,0 +1,113 @@ +package selfupdate + +import ( + "net/http" + "strconv" + "strings" + "testing" + "time" +) + +func resp(status int, hdr map[string]string) *http.Response { + h := http.Header{} + for k, v := range hdr { + h.Set(k, v) + } + return &http.Response{StatusCode: status, Status: strconv.Itoa(status) + " " + http.StatusText(status), Header: h} +} + +// TestRateLimitedForbiddenIsNotReportedAsPermissions is the regression this file +// exists for. A user on a PUBLIC repo exhausted the anonymous 60/hour quota and +// got "set GITHUB_TOKEN if the repo is private", which describes a problem that +// did not exist and hid the one that did. +func TestRateLimitedForbiddenIsNotReportedAsPermissions(t *testing.T) { + // Exactly the headers GitHub returned in that session. + r := resp(http.StatusForbidden, map[string]string{ + "X-RateLimit-Limit": "60", + "X-RateLimit-Remaining": "0", + "X-RateLimit-Reset": strconv.FormatInt(time.Now().Add(10*time.Minute).Unix(), 10), + }) + err := apiError("https://api.github.com/repos/o/r/releases", r) + msg := err.Error() + + if !strings.Contains(msg, "rate limit exceeded") { + t.Errorf("the message must name the real cause, got: %s", msg) + } + if strings.Contains(msg, "if the repository is private") || strings.Contains(msg, "if the repo is private") { + t.Errorf("a rate-limited response must NOT be reported as a permissions problem, got: %s", msg) + } + if !strings.Contains(msg, "60 requests/hour") { + t.Errorf("the message should quote the limit that was hit, got: %s", msg) + } + if !strings.Contains(msg, "GITHUB_TOKEN") { + t.Errorf("the message should still offer the fix, got: %s", msg) + } + if !strings.Contains(msg, "resets in") { + t.Errorf("the message should say when the window rolls over, got: %s", msg) + } +} + +func TestSecondaryRateLimitViaRetryAfter(t *testing.T) { + r := resp(http.StatusTooManyRequests, map[string]string{"Retry-After": "45"}) + msg := apiError("https://api.github.com/x", r).Error() + if !strings.Contains(msg, "rate limit exceeded") { + t.Errorf("429 with Retry-After should read as rate limiting, got: %s", msg) + } + if !strings.Contains(msg, "retry in 45s") { + t.Errorf("should surface Retry-After, got: %s", msg) + } +} + +// TestForbiddenWithQuotaLeftIsNotRateLimit: a 403 that still has quota is a real +// permissions failure and must keep the private-repo hint. +func TestForbiddenWithQuotaLeftIsNotRateLimit(t *testing.T) { + r := resp(http.StatusForbidden, map[string]string{ + "X-RateLimit-Limit": "5000", + "X-RateLimit-Remaining": "4999", + }) + msg := apiError("https://api.github.com/x", r).Error() + if strings.Contains(msg, "rate limit exceeded") { + t.Errorf("403 with quota remaining is not rate limiting, got: %s", msg) + } +} + +func TestNotFoundKeepsThePrivateRepoHint(t *testing.T) { + msg := apiError("https://api.github.com/x", resp(http.StatusNotFound, nil)).Error() + if !strings.Contains(msg, "private") { + t.Errorf("404 is where the private-repo hint belongs, got: %s", msg) + } +} + +func TestUnauthorizedBlamesTheToken(t *testing.T) { + msg := apiError("https://api.github.com/x", resp(http.StatusUnauthorized, nil)).Error() + if !strings.Contains(msg, "rejected") { + t.Errorf("401 means the token is bad, not that the repo is private, got: %s", msg) + } +} + +// TestResetHintNeverShowsAStaleOrNegativeDuration: a reset stamp in the past +// would otherwise render as "resets in -3m0s". +func TestResetHintNeverShowsAStaleOrNegativeDuration(t *testing.T) { + r := resp(http.StatusForbidden, map[string]string{ + "X-RateLimit-Remaining": "0", + "X-RateLimit-Reset": strconv.FormatInt(time.Now().Add(-3*time.Minute).Unix(), 10), + }) + msg := apiError("https://api.github.com/x", r).Error() + if strings.Contains(msg, "resets in -") || strings.Contains(msg, "resets in 0s") { + t.Errorf("a past reset stamp must be omitted, got: %s", msg) + } + if !strings.Contains(msg, "rate limit exceeded") { + t.Errorf("still a rate limit, got: %s", msg) + } +} + +func TestMalformedResetHeaderIsIgnored(t *testing.T) { + r := resp(http.StatusForbidden, map[string]string{ + "X-RateLimit-Remaining": "0", + "X-RateLimit-Reset": "not-a-number", + }) + msg := apiError("https://api.github.com/x", r).Error() + if strings.Contains(msg, "resets in") { + t.Errorf("an unparseable reset header must be dropped, got: %s", msg) + } +} diff --git a/internal/selfupdate/selfupdate.go b/internal/selfupdate/selfupdate.go index 79e6a5f..ab41c79 100644 --- a/internal/selfupdate/selfupdate.go +++ b/internal/selfupdate/selfupdate.go @@ -167,7 +167,7 @@ func githubGET(ctx context.Context, url string) ([]byte, error) { defer resp.Body.Close() body, _ := io.ReadAll(io.LimitReader(resp.Body, 4<<20)) if resp.StatusCode != http.StatusOK { - return nil, fmt.Errorf("GitHub API %s returned %s (set GITHUB_TOKEN if the repo is private)", url, resp.Status) + return nil, apiError(url, resp) } return body, nil } diff --git a/internal/selfupdate/update.go b/internal/selfupdate/update.go index 7675782..632792b 100644 --- a/internal/selfupdate/update.go +++ b/internal/selfupdate/update.go @@ -249,7 +249,7 @@ func downloadAsset(ctx context.Context, url string) ([]byte, error) { } defer resp.Body.Close() if resp.StatusCode != http.StatusOK { - return nil, fmt.Errorf("%s returned %s", url, resp.Status) + return nil, apiError(url, resp) } return io.ReadAll(io.LimitReader(resp.Body, 200<<20)) }