fix(selfupdate): report a rate-limited 403 as rate limiting, not a private repo

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

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

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

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
 }

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