diff --git a/auth.go b/auth.go index d629de3..d650b2d 100644 --- a/auth.go +++ b/auth.go @@ -284,7 +284,10 @@ func WithHTTPClient(client *http.Client) InstallationTokenSourceOpt { // only the Transport is swapped to inject GitHub App authentication. authClient := *client authClient.Transport = &oauth2.Transport{ - Source: i.src, + // Reuse the JWT across requests exactly as the default transport + // does; otherwise supplying a custom client silently re-signs on + // every installation-token request. + Source: oauth2.ReuseTokenSource(nil, i.src), Base: client.Transport, } @@ -400,6 +403,9 @@ func (t *installationTokenSource) setConfigErr(err error) { // NewInstallationTokenSource creates a GitHub App installation token source. // Requires installation ID and a GitHub App JWT token source for authentication. // +// A non-positive installation ID is a configuration error, reported by the +// first call to Token() rather than sent to GitHub as a request that can only 404. +// // The returned token source is wrapped in ReuseTokenSourceWithSkew so cached // tokens are refreshed DefaultExpirySkew before their expiry, eliminating // in-flight 401s when a request starts close to exp and reaches GitHub after. @@ -423,6 +429,12 @@ func NewInstallationTokenSource(id int64, src oauth2.TokenSource, opts ...Instal skew: DefaultExpirySkew, } + // A non-positive ID can only produce /app/installations/0/access_tokens and + // a 404. Fail like a misconfiguration rather than like a GitHub outage. + if id <= 0 { + i.setConfigErr(errors.New("installation identifier is required")) + } + for _, opt := range opts { opt(i) } diff --git a/auth_test.go b/auth_test.go index 19abfc6..d733a09 100644 --- a/auth_test.go +++ b/auth_test.go @@ -19,6 +19,7 @@ import ( "reflect" "strings" "sync" + "sync/atomic" "testing" "time" @@ -1234,3 +1235,91 @@ func TestWithExpirySkew_Wiring(t *testing.T) { }) } } + +// countingTokenSource records how many times the application JWT was minted. +type countingTokenSource struct { + calls atomic.Int32 +} + +func (c *countingTokenSource) Token() (*oauth2.Token, error) { + c.calls.Add(1) + return &oauth2.Token{ + AccessToken: "app-jwt", + TokenType: bearerTokenType, + Expiry: time.Now().Add(time.Hour), + }, nil +} + +// Test_WithHTTPClient_CachesApplicationToken proves WithHTTPClient caches the +// application JWT the same way the default transport does. NewInstallationTokenSource +// wraps the source in oauth2.ReuseTokenSource, but WithHTTPClient wires the raw +// source, so supplying a custom client silently re-signs a JWT on every +// installation-token request. +func Test_WithHTTPClient_CachesApplicationToken(t *testing.T) { + src := &countingTokenSource{} + + // expires_at inside the skew window, so the installation-token cache + // refetches on every call and each one reaches the transport. + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusCreated) + _ = json.NewEncoder(w).Encode(map[string]any{ + "token": "ghs_installation", + "expires_at": time.Now().Add(time.Second).UTC().Format(time.RFC3339), + }) + })) + defer server.Close() + + ts := NewInstallationTokenSource(42, src, + WithBaseURL(server.URL), + WithHTTPClient(&http.Client{}), + ) + + const fetches = 3 + for i := range fetches { + if _, err := ts.Token(); err != nil { + t.Fatalf("Token() #%d err = %v", i+1, err) + } + } + + if got := src.calls.Load(); got != 1 { + t.Errorf("application token source consulted %d times across %d fetches, want 1; "+ + "WithHTTPClient must reuse the JWT like the default transport", got, fetches) + } +} + +// Test_NewInstallationTokenSource_RejectsZeroID proves a zero installation ID is +// refused as a configuration error instead of being sent to GitHub as +// /app/installations/0/access_tokens, which can only 404. A zero App ID is +// already rejected at construction; this is the same mistake one layer down. +func Test_NewInstallationTokenSource_RejectsZeroID(t *testing.T) { + var hits atomic.Int32 + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + hits.Add(1) + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"message":"Not Found"}`)) + })) + defer server.Close() + + for _, id := range []int64{0, -1} { + t.Run(fmt.Sprintf("id=%d", id), func(t *testing.T) { + hits.Store(0) + + ts := NewInstallationTokenSource(id, oauth2StaticSource{accessToken: "jwt"}, + WithBaseURL(server.URL), + ) + + _, err := ts.Token() + if err == nil { + t.Fatalf("Token() err = nil, want a configuration error for installation ID %d", id) + } + if got := hits.Load(); got != 0 { + t.Errorf("server received %d request(s), want 0; installation ID %d must fail before any network call", got, id) + } + if !strings.Contains(err.Error(), "installation") { + t.Errorf("err = %q, want it to name the installation identifier", err) + } + }) + } +} diff --git a/github_test.go b/github_test.go index 9e6a44e..05f8004 100644 --- a/github_test.go +++ b/github_test.go @@ -299,7 +299,7 @@ func Test_githubClient_createInstallationToken_ErrorCases(t *testing.T) { if tt.wantErr && tt.errorSubstring != "" { if err == nil { t.Errorf("expected error containing %q, got nil", tt.errorSubstring) - } else if !contains(err.Error(), tt.errorSubstring) { + } else if !strings.Contains(err.Error(), tt.errorSubstring) { t.Errorf("expected error containing %q, got %q", tt.errorSubstring, err.Error()) } } @@ -307,19 +307,6 @@ func Test_githubClient_createInstallationToken_ErrorCases(t *testing.T) { } } -func contains(s, substr string) bool { - return len(s) >= len(substr) && (s == substr || len(substr) == 0 || (len(s) > 0 && len(substr) > 0 && hasSubstring(s, substr))) -} - -func hasSubstring(s, substr string) bool { - for i := 0; i <= len(s)-len(substr); i++ { - if s[i:i+len(substr)] == substr { - return true - } - } - return false -} - func Test_Ptr(t *testing.T) { t.Run("string pointer", func(t *testing.T) { s := "test"