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)) }