diff --git a/.gitignore b/.gitignore index 6f6f5e6..99214c6 100644 --- a/.gitignore +++ b/.gitignore @@ -20,3 +20,6 @@ # Go workspace file go.work go.work.sum + +# Claude Code local settings and scratch worktrees +.claude/ diff --git a/README.md b/README.md index 356dc5a..55b0720 100644 --- a/README.md +++ b/README.md @@ -95,11 +95,13 @@ Options combine in any order. An unparseable URL (or a nil client passed to `Wit ```go appTokenSource, err := githubauth.NewApplicationTokenSource(clientID, privateKey, - githubauth.WithApplicationTokenExpiration(1*time.Minute), - githubauth.WithExpirySkew(5*time.Second), // effective validity: 55s + githubauth.WithApplicationTokenExpiration(5*time.Minute), + githubauth.WithExpirySkew(5*time.Second), // effective validity: 3m55s ) ``` +Expiration is backdated 60s for clock drift, so effective validity is `expiration - 60s - skew`. Values at or below 90s (the backdate plus `DefaultExpirySkew`) are rejected and fall back to 10 minutes, because below that the cache can never hold the token and every call re-signs. + A zero or negative skew restores exact `oauth2.ReuseTokenSource` behavior. The wrapper is exported as `ReuseTokenSourceWithSkew` for use with any third-party `oauth2.TokenSource`, and is safe for concurrent use. ## Signing with external key stores (KMS, HSM, Vault) diff --git a/auth.go b/auth.go index d650b2d..c1dc337 100644 --- a/auth.go +++ b/auth.go @@ -9,6 +9,7 @@ import ( "encoding/base64" "errors" "net/http" + "reflect" "strconv" "sync" "time" @@ -25,10 +26,22 @@ const ( // DefaultExpirySkew is the default early-refresh window applied to cached // tokens returned by NewApplicationTokenSource and NewInstallationTokenSource. // At 30s the effective validity of a default 10-minute application JWT becomes - // 9m30s, which is acceptable and eliminates the common in-flight 401 caused + // 8m30s, which is acceptable and eliminates the common in-flight 401 caused // by a request starting near exp and arriving at GitHub after exp. DefaultExpirySkew = 30 * time.Second + // applicationTokenBackdate is how far into the past the iat claim of an + // application JWT is set, so a fast local clock cannot make GitHub see an + // iat in the future. It is subtracted from exp as well, so it is also the + // lower bound an expiration must exceed to yield a usable token. + applicationTokenBackdate = 60 * time.Second + + // minApplicationTokenExpiration is the smallest expiration that yields a + // token the cache can actually hold: it must clear the backdate (or the JWT + // is born expired) and then the refresh skew on top (or every Token() call + // re-signs). Expirations at or below it fall back to the default. + minApplicationTokenExpiration = applicationTokenBackdate + DefaultExpirySkew + // bearerTokenType is the token type used for OAuth2 Bearer tokens. bearerTokenType = "Bearer" ) @@ -110,11 +123,16 @@ type applicationTokenSource struct { type ApplicationTokenOpt func(*applicationTokenSource) // WithApplicationTokenExpiration sets the JWT expiration duration. -// Must be between 0 and 10 minutes per GitHub's JWT requirements. Invalid values default to 10 minutes. +// Must be greater than 90 seconds and at most 10 minutes. Ten minutes is +// GitHub's ceiling. The lower bound is the 60 second clock-drift backdate plus +// DefaultExpirySkew: at or below the backdate the JWT is already expired when +// minted, and within the skew above it the cache never holds the token, so +// every call re-signs — a remote round trip for a KMS, HSM or Vault signer. +// Invalid values default to 10 minutes. // See https://docs.github.com/en/apps/creating-github-apps/authenticating-with-a-github-app/generating-a-json-web-token-jwt-for-a-github-app#about-json-web-tokens-jwts func WithApplicationTokenExpiration(exp time.Duration) ApplicationTokenOpt { return func(a *applicationTokenSource) { - if exp > DefaultApplicationTokenExpiration || exp <= 0 { + if exp > DefaultApplicationTokenExpiration || exp <= minApplicationTokenExpiration { exp = DefaultApplicationTokenExpiration } a.expiration = exp @@ -129,8 +147,8 @@ func WithApplicationTokenExpiration(exp time.Duration) ApplicationTokenOpt { // // Tune this when your application token expiration (see // WithApplicationTokenExpiration) is short: the effective validity is -// expiration - skew, so with the default 10-minute expiration and 30s skew -// tokens are refreshed at 9m30s. +// expiration - 60s of clock-drift backdating - skew, so with the default +// 10-minute expiration and 30s skew tokens are refreshed at 8m30s. func WithExpirySkew(d time.Duration) ApplicationTokenOpt { return func(a *applicationTokenSource) { a.skew = d @@ -145,8 +163,9 @@ func WithExpirySkew(d time.Duration) ApplicationTokenOpt { // // The returned token source is wrapped in ReuseTokenSourceWithSkew with // DefaultExpirySkew (30s), so cached tokens are refreshed before exp rather -// than after. With the default 10-minute expiration the effective validity -// is 9m30s. Override with WithExpirySkew. +// than after. With the default 10-minute expiration, which is backdated 60s +// for clock drift, the effective validity is 8m30s. Override with +// WithExpirySkew. // // For KMS, HSM, Vault, or ssh-agent backed signing, use // NewApplicationTokenSourceFromSigner instead — the private key never leaves @@ -195,17 +214,23 @@ func NewApplicationTokenSourceFromSigner[T Identifier](id T, signer crypto.Signe // resolveIssuer converts a generic App ID / Client ID to its string form // and rejects zero values. func resolveIssuer[T Identifier](id T) (string, error) { - switch v := any(id).(type) { - case int64: - if v == 0 { + // Identifier is "~int64 | ~string", so defined types such as + // "type AppID int64" satisfy it. A type switch matches the exact dynamic + // type and would reject those, hence the switch on the underlying kind. + v := reflect.ValueOf(id) + switch v.Kind() { + case reflect.Int64: + n := v.Int() + if n == 0 { return "", errors.New("application identifier is required") } - return strconv.FormatInt(v, 10), nil - case string: - if v == "" { + return strconv.FormatInt(n, 10), nil + case reflect.String: + s := v.String() + if s == "" { return "", errors.New("application identifier is required") } - return v, nil + return s, nil default: return "", errors.New("unsupported identifier type") } @@ -230,7 +255,7 @@ func newApplicationTokenSource(issuer string, signer crypto.Signer, opts ...Appl // Generated JWTs can be used with "Authorization: Bearer" header for GitHub API requests. func (t *applicationTokenSource) Token() (*oauth2.Token, error) { // To protect against clock drift, set the issuance time 60 seconds in the past. - now := time.Now().Add(-60 * time.Second) + now := time.Now().Add(-applicationTokenBackdate) expiresAt := now.Add(t.expiration) token := jwt.NewWithClaims(jwt.SigningMethodRS256, jwt.RegisteredClaims{ @@ -344,14 +369,19 @@ func WithContext(ctx context.Context) InstallationTokenSourceOpt { } // WithRetryOnThrottle enables or disables a single automatic retry when -// GitHub returns a throttled response (HTTP 429, or 403 with rate-limit -// headers) for the installation token POST. Enabled by default. +// GitHub throttles the installation token POST. Enabled by default. +// +// Any 429 counts as throttled. A 403 counts only when GitHub identifies it as a +// rate limit — it carries Retry-After, reports an exhausted budget via +// X-RateLimit-Remaining: 0, or says so in its message. A permission failure such +// as "Resource not accessible by integration" is terminal and is retried by +// neither setting, despite carrying rate-limit headers. // -// On a throttled response the client sleeps the duration hinted by -// Retry-After or x-ratelimit-reset (capped at 60s, honoring ctx cancellation) -// and retries once. Subsequent failures bubble up unchanged. On a terminal -// throttle the returned error wraps ErrRateLimited so callers can branch with -// errors.Is. +// On a throttled response the client sleeps the duration hinted by Retry-After +// or x-ratelimit-reset, falling back to one minute for a hintless rate-limit 403 +// (capped at 60s, honoring ctx cancellation), and retries once. Subsequent +// failures bubble up unchanged. On a terminal throttle the returned error wraps +// ErrRateLimited so callers can branch with errors.Is. // // Disable this when the caller implements its own backoff or when deterministic // latency matters more than transient rate-limit resilience. diff --git a/auth_test.go b/auth_test.go index d733a09..fb26688 100644 --- a/auth_test.go +++ b/auth_test.go @@ -1323,3 +1323,315 @@ func Test_NewInstallationTokenSource_RejectsZeroID(t *testing.T) { }) } } + +// namedAppID and namedClientID model the idiomatic Go habit of giving an +// identifier its own defined type. The Identifier constraint is "~int64 | ~string", +// whose tildes promise both are accepted. +type namedAppID int64 + +type namedClientID string + +func Test_resolveIssuer_NamedIdentifierTypes(t *testing.T) { + tests := []struct { + name string + resolve func() (string, error) + want string + wantErr bool + wantErrMsg string + }{ + { + name: "named int64 identifier", + resolve: func() (string, error) { return resolveIssuer(namedAppID(123)) }, + want: "123", + }, + { + name: "named string identifier", + resolve: func() (string, error) { return resolveIssuer(namedClientID("Iv1.abc123")) }, + want: "Iv1.abc123", + }, + { + name: "plain int64 identifier", + resolve: func() (string, error) { return resolveIssuer(int64(456)) }, + want: "456", + }, + { + name: "plain string identifier", + resolve: func() (string, error) { return resolveIssuer("Iv23.def456") }, + want: "Iv23.def456", + }, + { + name: "named int64 zero value is rejected", + resolve: func() (string, error) { return resolveIssuer(namedAppID(0)) }, + wantErr: true, + wantErrMsg: "application identifier is required", + }, + { + name: "named string empty value is rejected", + resolve: func() (string, error) { return resolveIssuer(namedClientID("")) }, + wantErr: true, + wantErrMsg: "application identifier is required", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := tt.resolve() + if tt.wantErr { + if err == nil { + t.Fatalf("resolveIssuer() error = nil, want %q", tt.wantErrMsg) + } + if err.Error() != tt.wantErrMsg { + t.Fatalf("resolveIssuer() error = %q, want %q", err.Error(), tt.wantErrMsg) + } + return + } + if err != nil { + t.Fatalf("resolveIssuer() unexpected error = %v", err) + } + if got != tt.want { + t.Errorf("resolveIssuer() = %q, want %q", got, tt.want) + } + }) + } +} + +func Test_NewApplicationTokenSource_NamedIdentifierTypes(t *testing.T) { + rsaKey, err := rsa.GenerateKey(rand.Reader, 2048) + if err != nil { + t.Fatal(err) + } + pemBytes := pem.EncodeToMemory(&pem.Block{ + Type: "RSA PRIVATE KEY", + Bytes: x509.MarshalPKCS1PrivateKey(rsaKey), + }) + + cases := []struct { + name string + newSource func() (string, error) + wantIssuer string + }{ + { + name: "named int64 through pem constructor", + newSource: func() (string, error) { + ts, err := NewApplicationTokenSource(namedAppID(42), pemBytes) + if err != nil { + return "", err + } + tok, err := ts.Token() + if err != nil { + return "", err + } + return issuerFromJWT(tok.AccessToken) + }, + wantIssuer: "42", + }, + { + name: "named string through signer constructor", + newSource: func() (string, error) { + ts, err := NewApplicationTokenSourceFromSigner(namedClientID("Iv1.abc123"), rsaKey) + if err != nil { + return "", err + } + tok, err := ts.Token() + if err != nil { + return "", err + } + return issuerFromJWT(tok.AccessToken) + }, + wantIssuer: "Iv1.abc123", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got, err := tc.newSource() + if err != nil { + t.Fatalf("unexpected error = %v", err) + } + if got != tc.wantIssuer { + t.Errorf("iss = %q, want %q", got, tc.wantIssuer) + } + }) + } +} + +// issuerFromJWT extracts the iss claim from a signed JWT without verifying it. +func issuerFromJWT(token string) (string, error) { + parts := strings.Split(token, ".") + if len(parts) != 3 { + return "", fmt.Errorf("malformed JWT: %d parts", len(parts)) + } + payload, err := base64.RawURLEncoding.DecodeString(parts[1]) + if err != nil { + return "", err + } + var claims struct { + Iss string `json:"iss"` + } + if err := json.Unmarshal(payload, &claims); err != nil { + return "", err + } + return claims.Iss, nil +} + +// Test_applicationTokenSource_ShortExpirationIsNotAlreadyExpired pins the +// contract that a configured application token expiration never yields a dead +// JWT. +// +// Token() backdates issuance by 60s for clock-drift protection +// (exp = now - 60s + expiration), so any expiration <= 60s mints a token whose +// exp is already in the past (or exactly now). WithApplicationTokenExpiration +// clamps only the upper bound, so those values slip through and the caller gets +// a credential GitHub answers with 401. +// +// Assertions are on the observable oauth2.Token.Expiry and the signed exp +// claim, not on internal fields, so the test holds whether the lower bound is +// enforced in the option or in Token(). +func Test_applicationTokenSource_ShortExpirationIsNotAlreadyExpired(t *testing.T) { + rsaKey, err := rsa.GenerateKey(rand.Reader, 2048) + if err != nil { + t.Fatal(err) + } + + tests := []struct { + name string + // expiration is the value handed to WithApplicationTokenExpiration. + expiration time.Duration + // wantEffective is the expiration the source must actually apply: + // values at or below minApplicationTokenExpiration (the 60s backdate + // plus DefaultExpirySkew) are invalid and fall back to + // DefaultApplicationTokenExpiration, like every other invalid value the + // option already rejects. + wantEffective time.Duration + }{ + { + name: "30s is shorter than the clock-drift backdating", + expiration: 30 * time.Second, + wantEffective: DefaultApplicationTokenExpiration, + }, + { + name: "60s exactly cancels the clock-drift backdating", + expiration: 60 * time.Second, + wantEffective: DefaultApplicationTokenExpiration, + }, + { + // Clears the backdate but not the refresh skew on top of it, so the + // cache could never hold the token and every call would re-sign. + name: "90s is exactly the bound and is rejected", + expiration: 90 * time.Second, + wantEffective: DefaultApplicationTokenExpiration, + }, + { + name: "91s is the smallest honoured value", + expiration: 91 * time.Second, + wantEffective: 91 * time.Second, + }, + { + name: "5m is honoured", + expiration: 5 * time.Minute, + wantEffective: 5 * time.Minute, + }, + { + name: "10m is honoured", + expiration: DefaultApplicationTokenExpiration, + wantEffective: DefaultApplicationTokenExpiration, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + before := time.Now() + + ts, err := NewApplicationTokenSourceFromSigner(int64(42), rsaKey, WithApplicationTokenExpiration(tt.expiration)) + if err != nil { + t.Fatalf("construct: %v", err) + } + + tok, err := ts.Token() + if err != nil { + t.Fatalf("Token(): %v", err) + } + + // The token must be usable at all: a caller asking for a short + // expiration must not silently receive a dead credential. + if validFor := time.Until(tok.Expiry); validFor <= 0 { + t.Errorf("Expiry %v is not in the future (valid for %v); token is dead on arrival", tok.Expiry, validFor) + } + + // exp = issuance (now - 60s) + effective expiration. Tolerance + // absorbs execution time and the whole-second truncation of + // NumericDate. + want := before.Add(-60 * time.Second).Add(tt.wantEffective) + if diff := tok.Expiry.Sub(want); diff < -2*time.Second || diff > 2*time.Second { + t.Errorf("Expiry = %v, want ~%v (±2s): effective expiration is not %v", tok.Expiry, want, tt.wantEffective) + } + + // The signed JWT itself must verify and must not be expired: this + // is what GitHub sees. + parsed, err := jwt.ParseWithClaims(tok.AccessToken, &jwt.RegisteredClaims{}, func(_ *jwt.Token) (any, error) { + return &rsaKey.PublicKey, nil + }) + if err != nil { + t.Fatalf("parse minted JWT: %v", err) + } + + claims, ok := parsed.Claims.(*jwt.RegisteredClaims) + if !ok { + t.Fatalf("claims type = %T, want *jwt.RegisteredClaims", parsed.Claims) + } + if claims.ExpiresAt == nil { + t.Fatal("exp claim is missing") + } + if !claims.ExpiresAt.After(time.Now()) { + t.Errorf("exp claim %v is not in the future; GitHub rejects this JWT with 401", claims.ExpiresAt.Time) + } + }) + } +} + +// countingSigner records how many times the private key was used to sign. +type countingSigner struct { + crypto.Signer + signs atomic.Int32 +} + +func (c *countingSigner) Sign(rand io.Reader, digest []byte, opts crypto.SignerOpts) ([]byte, error) { + c.signs.Add(1) + return c.Signer.Sign(rand, digest, opts) +} + +// Test_WithApplicationTokenExpiration_TokenIsCacheable proves the accepted +// lower bound leaves a token the cache can actually hold. An expiration above +// the 60s backdate but within DefaultExpirySkew of it yields a token whose +// remaining life is at or below the skew, so ReuseTokenSourceWithSkew treats it +// as stale immediately and re-signs on every call — a remote round trip for the +// KMS, HSM and Vault signers NewApplicationTokenSourceFromSigner exists to serve. +func Test_WithApplicationTokenExpiration_TokenIsCacheable(t *testing.T) { + rsaKey, err := rsa.GenerateKey(rand.Reader, 2048) + if err != nil { + t.Fatal(err) + } + + for _, exp := range []time.Duration{61 * time.Second, 90 * time.Second, 5 * time.Minute} { + t.Run(exp.String(), func(t *testing.T) { + signer := &countingSigner{Signer: rsaKey} + + ts, err := NewApplicationTokenSourceFromSigner(int64(42), signer, + WithApplicationTokenExpiration(exp)) + if err != nil { + t.Fatalf("construct: %v", err) + } + + const calls = 5 + for i := range calls { + if _, err := ts.Token(); err != nil { + t.Fatalf("Token() #%d: %v", i+1, err) + } + } + + if got := signer.signs.Load(); got != 1 { + t.Errorf("signed %d times across %d calls, want 1; the token must survive in the cache", got, calls) + } + }) + } +} diff --git a/github.go b/github.go index 7bc35fc..bd4f8ed 100644 --- a/github.go +++ b/github.go @@ -18,6 +18,10 @@ const ( // defaultBaseURL is the default GitHub API base URL. defaultBaseURL = "https://api.github.com/" + // maxErrorBodyBytes caps how much of a non-2xx body is read into the error + // message, so a misbehaving or hostile server cannot exhaust memory. + maxErrorBodyBytes = 64 << 10 + // maxRetrySleep caps sleeps between retries so a misbehaving or hostile // server cannot stall callers indefinitely via Retry-After. maxRetrySleep = 60 * time.Second @@ -25,10 +29,21 @@ const ( // defaultThrottleBackoff is the fallback delay when GitHub returns 429 // without any retry hint header. defaultThrottleBackoff = 1 * time.Second + + // secondaryRateLimitBackoff is the fallback delay for a 403 identified as a + // rate limit that carries no usable hint. GitHub's guidance for that case is + // to wait at least one minute before retrying. + secondaryRateLimitBackoff = 60 * time.Second ) -// ErrRateLimited wraps errors returned when GitHub has throttled a request -// (HTTP 429 or 403 with rate-limit headers). Callers can branch with errors.Is. +// ErrRateLimited wraps errors returned when GitHub has throttled a request: +// any HTTP 429, or a 403 that GitHub identifies as a rate limit rather than a +// permission failure. A 403 counts when it carries Retry-After, reports an +// exhausted budget via X-RateLimit-Remaining: 0, or says so in its message. +// +// A 403 such as "Resource not accessible by integration" is a permission +// failure and is NOT wrapped, even though GitHub attaches rate-limit headers +// to it. Callers can branch with errors.Is. var ErrRateLimited = errors.New("github API rate limited") // InstallationTokenOptions specifies options for creating an installation token. @@ -154,8 +169,9 @@ func (c *githubClient) withBaseURL(baseURL string) (*githubClient, error) { // createInstallationToken creates an installation access token for a GitHub App. // When retryOnThrottle is enabled, a single retry is performed on 429 or on 403 -// responses that carry Retry-After / x-ratelimit-reset headers. The sleep is -// capped at maxRetrySleep and honors ctx cancellation. +// responses that are actually throttled — those carrying Retry-After, or +// x-ratelimit-remaining: 0. The sleep is capped at maxRetrySleep and honors ctx +// cancellation. // // API documentation: https://docs.github.com/en/rest/apps/apps?apiVersion=2022-11-28#create-an-installation-access-token-for-an-app func (c *githubClient) createInstallationToken(ctx context.Context, installationID int64, opts *InstallationTokenOptions) (*InstallationToken, error) { @@ -223,21 +239,20 @@ func (c *githubClient) doCreateInstallationToken(ctx context.Context, reqURL str return &token, 0, nil } - bodyResp, _ := io.ReadAll(resp.Body) - if delay, ok := c.throttleDelay(resp); ok { + bodyResp, _ := io.ReadAll(io.LimitReader(resp.Body, maxErrorBodyBytes)) + if delay, ok := c.throttleDelay(resp, bodyResp); ok { return nil, delay, fmt.Errorf("%w: GitHub API returned status %d: %s", ErrRateLimited, resp.StatusCode, string(bodyResp)) } return nil, 0, fmt.Errorf("GitHub API returned status %d: %s", resp.StatusCode, string(bodyResp)) } -// throttleDelay inspects a non-2xx response and reports the retry hint from -// GitHub's rate-limit headers. The bool is true when the response is considered -// retryable (429 always, 403 only when a parseable retry header is present). -// The returned duration is capped at maxRetrySleep. An unparseable header is -// treated as absent — a malformed hint must not silently flip a terminal 403 -// into a retry. -func (c *githubClient) throttleDelay(resp *http.Response) (time.Duration, bool) { +// throttleDelay inspects a non-2xx response and reports the retry hint GitHub +// supplied. The bool is true when the response is considered retryable: any 429, +// and a 403 only when it is a rate limit rather than a permission failure. The +// returned duration is capped at maxRetrySleep. An unparseable header is treated +// as absent — a malformed hint must not silently flip a terminal 403 into a retry. +func (c *githubClient) throttleDelay(resp *http.Response, body []byte) (time.Duration, bool) { if resp.StatusCode != http.StatusTooManyRequests && resp.StatusCode != http.StatusForbidden { return 0, false } @@ -248,6 +263,10 @@ func (c *githubClient) throttleDelay(resp *http.Response) (time.Duration, bool) } } + if resp.StatusCode == http.StatusForbidden && !isRateLimit403(resp, body) { + return 0, false + } + if v := resp.Header.Get("X-RateLimit-Reset"); v != "" { if reset, err := strconv.ParseInt(strings.TrimSpace(v), 10, 64); err == nil { return capDelay(time.Until(time.Unix(reset, 0))), true @@ -258,7 +277,28 @@ func (c *githubClient) throttleDelay(resp *http.Response) (time.Duration, bool) return defaultThrottleBackoff, true } - return 0, false + // A 403 that reached here is a rate limit with no usable hint. + return secondaryRateLimitBackoff, true +} + +// isRateLimit403 reports whether a 403 is a rate limit rather than a permission +// failure. GitHub attaches X-RateLimit-Limit / X-RateLimit-Remaining / +// X-RateLimit-Reset to essentially every authenticated response, terminal 403s +// such as "Resource not accessible by integration" included, so the presence of +// those headers proves nothing on its own: a permission failure carries a +// healthy remaining count and a reset epoch up to an hour out. +// +// Two things do distinguish one: an exhausted budget, or GitHub saying so in the +// message. The message matters because a secondary rate limit can arrive with a +// healthy primary budget and no Retry-After, and treating that as terminal would +// strip the ErrRateLimited callers branch on. +func isRateLimit403(resp *http.Response, body []byte) bool { + if v := strings.TrimSpace(resp.Header.Get("X-RateLimit-Remaining")); v != "" { + if remaining, err := strconv.ParseInt(v, 10, 64); err == nil && remaining <= 0 { + return true + } + } + return strings.Contains(strings.ToLower(string(body)), "rate limit") } // parseRetryAfter accepts either integer seconds ("30") or an HTTP-date diff --git a/github_test.go b/github_test.go index 05f8004..46226e1 100644 --- a/github_test.go +++ b/github_test.go @@ -544,11 +544,14 @@ func Test_createInstallationToken_RetryOnThrottle(t *testing.T) { func Test_createInstallationToken_XRateLimitReset(t *testing.T) { // x-ratelimit-reset is a Unix-second epoch, so the header value must be // computed at request-time to avoid sub-second truncation that makes the - // elapsed-time assertion flaky. + // elapsed-time assertion flaky. x-ratelimit-remaining is 0 because that is + // what marks a 403 as throttled rather than terminal; see + // Test_createInstallationToken_403PermissionNotThrottled. var attempts atomic.Int32 server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { n := attempts.Add(1) if n == 1 { + w.Header().Set("X-RateLimit-Remaining", "0") w.Header().Set("X-RateLimit-Reset", strconv.FormatInt(time.Now().Add(3*time.Second).Unix(), 10)) w.WriteHeader(http.StatusForbidden) _, _ = w.Write([]byte(`{"message":"rate limited"}`)) @@ -595,7 +598,7 @@ func Test_throttleDelay_CapsAtMaxRetrySleep(t *testing.T) { StatusCode: http.StatusTooManyRequests, Header: http.Header{"Retry-After": []string{"600"}}, } - d, ok := c.throttleDelay(resp) + d, ok := c.throttleDelay(resp, nil) if !ok { t.Fatalf("throttleDelay ok = false, want true") } @@ -607,7 +610,7 @@ func Test_throttleDelay_CapsAtMaxRetrySleep(t *testing.T) { StatusCode: http.StatusTooManyRequests, Header: http.Header{"Retry-After": []string{time.Now().Add(1 * time.Hour).UTC().Format(http.TimeFormat)}}, } - d, ok = c.throttleDelay(resp) + d, ok = c.throttleDelay(resp, nil) if !ok { t.Fatalf("throttleDelay ok = false, want true") } @@ -679,7 +682,7 @@ func Test_throttleDelay_NonThrottledStatus(t *testing.T) { c := newGitHubClient(&http.Client{}) for _, status := range []int{http.StatusOK, http.StatusCreated, http.StatusBadRequest, http.StatusInternalServerError} { resp := &http.Response{StatusCode: status, Header: http.Header{"Retry-After": []string{"5"}}} - if d, ok := c.throttleDelay(resp); ok || d != 0 { + if d, ok := c.throttleDelay(resp, nil); ok || d != 0 { t.Errorf("status %d: got (%v, %v), want (0, false)", status, d, ok) } } @@ -828,3 +831,217 @@ func Test_createInstallationToken_TokenFormats(t *testing.T) { }) } } + +// Test_createInstallationToken_403PermissionNotThrottled pins the distinction +// between a throttled 403 and a terminal permission 403. +// +// GitHub attaches X-RateLimit-Limit / X-RateLimit-Remaining / X-RateLimit-Reset +// to essentially every authenticated REST response, including terminal failures +// such as 403 {"message":"Resource not accessible by integration"}. Those carry +// a healthy X-RateLimit-Remaining and a reset epoch up to an hour out, so the +// reset header alone must never make a 403 retryable: doing so stalls the caller +// for up to maxRetrySleep and then reports ErrRateLimited for what is really a +// permissions problem. +// +// A 403 is throttled only when Retry-After is present or X-RateLimit-Remaining +// is exactly "0". 429 keeps its existing behaviour. +func Test_createInstallationToken_403PermissionNotThrottled(t *testing.T) { + // Computed once here; the server replays it milliseconds later. + futureReset := strconv.FormatInt(time.Now().Add(5*time.Second).Unix(), 10) + pastReset := strconv.FormatInt(time.Now().Add(-time.Minute).Unix(), 10) + + tests := []struct { + name string + responses []throttleResponse + wantAttempts int32 + wantErr bool + wantRateLimit bool + maxElapsed time.Duration + }{ + { + name: "403 permission error with healthy remaining is terminal", + responses: []throttleResponse{ + { + status: http.StatusForbidden, + headers: map[string]string{ + "X-RateLimit-Limit": "5000", + "X-RateLimit-Remaining": "4999", + "X-RateLimit-Reset": futureReset, + }, + body: `{"message":"Resource not accessible by integration"}`, + }, + }, + wantAttempts: 1, + wantErr: true, + wantRateLimit: false, + maxElapsed: time.Second, + }, + { + name: "403 with exhausted remaining is still throttled", + responses: []throttleResponse{ + { + status: http.StatusForbidden, + headers: map[string]string{ + "X-RateLimit-Limit": "5000", + "X-RateLimit-Remaining": "0", + "X-RateLimit-Reset": pastReset, + }, + body: `{"message":"API rate limit exceeded"}`, + }, + {status: http.StatusCreated, writeToken: true}, + }, + wantAttempts: 2, + wantErr: false, + maxElapsed: 5 * time.Second, + }, + { + name: "403 with Retry-After is still throttled", + responses: []throttleResponse{ + { + status: http.StatusForbidden, + headers: map[string]string{"Retry-After": "0"}, + body: `{"message":"You have exceeded a secondary rate limit"}`, + }, + {status: http.StatusCreated, writeToken: true}, + }, + wantAttempts: 2, + wantErr: false, + maxElapsed: 5 * time.Second, + }, + { + name: "429 without hint headers is still throttled", + responses: []throttleResponse{ + {status: http.StatusTooManyRequests, body: `{"message":"rate limited"}`}, + {status: http.StatusCreated, writeToken: true}, + }, + wantAttempts: 2, + wantErr: false, + maxElapsed: 5 * time.Second, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var attempts atomic.Int32 + server := httptest.NewServer(throttleHandler(t, tt.responses, &attempts)) + defer server.Close() + + client := newClientForServer(t, server) + + start := time.Now() + _, err := client.createInstallationToken(context.Background(), 12345, nil) + elapsed := time.Since(start) + + if got := attempts.Load(); got != tt.wantAttempts { + t.Errorf("attempts = %d, want %d", got, tt.wantAttempts) + } + if (err != nil) != tt.wantErr { + t.Errorf("err = %v, wantErr = %v", err, tt.wantErr) + } + if got := errors.Is(err, ErrRateLimited); got != tt.wantRateLimit { + t.Errorf("errors.Is(err, ErrRateLimited) = %v, want %v (err = %v)", got, tt.wantRateLimit, err) + } + if elapsed > tt.maxElapsed { + t.Errorf("elapsed = %v, want <= %v", elapsed, tt.maxElapsed) + } + }) + } +} + +// Test_throttleDelay_403Classification pins which 403s count as rate limiting. +// GitHub attaches X-RateLimit-* headers to essentially every authenticated +// response, so header presence alone proves nothing: a permission failure +// carries a healthy budget and a reset epoch up to an hour out. What does +// distinguish a rate limit is an exhausted budget, a Retry-After, or GitHub +// saying so in the message — the last matters because a secondary rate limit +// can arrive with a healthy primary budget and no Retry-After. +func Test_throttleDelay_403Classification(t *testing.T) { + future := strconv.FormatInt(time.Now().Add(30*time.Second).Unix(), 10) + + tests := []struct { + name string + headers map[string]string + body string + wantRetry bool + }{ + { + name: "permission failure with healthy budget is terminal", + headers: map[string]string{"X-RateLimit-Remaining": "4999", "X-RateLimit-Reset": future}, + body: `{"message":"Resource not accessible by integration"}`, + wantRetry: false, + }, + { + name: "secondary rate limit with healthy budget and no hint is throttled", + headers: map[string]string{"X-RateLimit-Remaining": "4321", "X-RateLimit-Reset": future}, + body: `{"message":"You have exceeded a secondary rate limit. Please wait a few minutes before you try again."}`, + wantRetry: true, + }, + { + name: "exhausted budget without a reset header is still throttled", + headers: map[string]string{"X-RateLimit-Remaining": "0"}, + body: `{"message":"API rate limit exceeded"}`, + wantRetry: true, + }, + { + name: "non-canonical zero budget is read numerically", + headers: map[string]string{"X-RateLimit-Remaining": "00", "X-RateLimit-Reset": future}, + body: `{"message":"Forbidden"}`, + wantRetry: true, + }, + { + name: "no rate-limit headers at all is terminal", + headers: map[string]string{}, + body: `{"message":"Resource not accessible by integration"}`, + wantRetry: false, + }, + { + name: "Retry-After wins regardless of budget or message", + headers: map[string]string{"Retry-After": "1", "X-RateLimit-Remaining": "4999"}, + body: `{"message":"Resource not accessible by integration"}`, + wantRetry: true, + }, + } + + c := newGitHubClient(&http.Client{}) + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + resp := &http.Response{StatusCode: http.StatusForbidden, Header: http.Header{}} + for k, v := range tt.headers { + resp.Header.Set(k, v) + } + + delay, retry := c.throttleDelay(resp, []byte(tt.body)) + if retry != tt.wantRetry { + t.Fatalf("throttleDelay() retryable = %v, want %v", retry, tt.wantRetry) + } + if retry && delay <= 0 { + t.Errorf("throttleDelay() delay = %v, want a positive wait when retryable", delay) + } + if !retry && delay != 0 { + t.Errorf("throttleDelay() delay = %v, want 0 when terminal", delay) + } + }) + } +} + +// Test_throttleDelay_HintlessRateLimit403WaitsAMinute pins the fallback for a +// 403 identified as a rate limit that carries no usable hint. GitHub's guidance +// for that case is to wait at least a minute, not the 1s used for a bare 429. +func Test_throttleDelay_HintlessRateLimit403WaitsAMinute(t *testing.T) { + resp := &http.Response{StatusCode: http.StatusForbidden, Header: http.Header{}} + resp.Header.Set("X-RateLimit-Remaining", "0") + + delay, retry := c403(t).throttleDelay(resp, []byte(`{"message":"API rate limit exceeded"}`)) + if !retry { + t.Fatal("throttleDelay() retryable = false, want true") + } + if delay != secondaryRateLimitBackoff { + t.Errorf("delay = %v, want %v", delay, secondaryRateLimitBackoff) + } +} + +func c403(t *testing.T) *githubClient { + t.Helper() + return newGitHubClient(&http.Client{}) +} diff --git a/llms.txt b/llms.txt index 01f9fbe..51d02d8 100644 --- a/llms.txt +++ b/llms.txt @@ -15,7 +15,7 @@ When to choose this library: Core API (package `githubauth`): -- `NewApplicationTokenSource(id, privateKeyPEM, opts...) (oauth2.TokenSource, error)` — App JWT source; `id` is a string Client ID or int64 App ID. Options: `WithApplicationTokenExpiration(d)` (max 10m), `WithExpirySkew(d)`. +- `NewApplicationTokenSource(id, privateKeyPEM, opts...) (oauth2.TokenSource, error)` — App JWT source; `id` is a string Client ID or int64 App ID. Options: `WithApplicationTokenExpiration(d)` (>90s, max 10m; outside that range falls back to 10m), `WithExpirySkew(d)`. - `NewApplicationTokenSourceFromSigner(id, signer crypto.Signer, opts...) (oauth2.TokenSource, error)` — App JWT source backed by an external RSA signer (KMS/HSM/Vault/ssh-agent). - `NewInstallationTokenSource(installationID int64, appSource oauth2.TokenSource, opts...) oauth2.TokenSource` — exchanges the App JWT for an installation token. Options: `WithEnterpriseURL(url)` (GHES, appends /api/v3/), `WithBaseURL(url)` (verbatim; GHEC data residency or httptest), `WithHTTPClient(c)`, `WithRetryOnThrottle(bool)`, `WithInstallationExpirySkew(d)`, `WithInstallationTokenOptions(o)`, `WithContext(ctx)`. - `NewPersonalAccessTokenSource(token string) oauth2.TokenSource` — classic (`ghp_...`) or fine-grained (`github_pat_...`) PATs.