diff --git a/CHANGELOG.md b/CHANGELOG.md index 7df310c..6e905a7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -77,6 +77,9 @@ changes can affect existing code or scripts; they are listed under Changed. under `token --exec`, before anything is minted. It used to be kept, which v1.11.0 suggested for a `--base-url` proxy in front of github.com; set it inside the command instead: `--exec -- env GH_HOST=github.com gh …` +- `NewPersonalAccessTokenSource("")`'s error now reads "personal access token is + required", like the package's other missing-argument errors. Match errors with + `errors.Is`/`errors.As`, not their text - **`InstallationPermissions` is no longer comparable**, because `Other` is a map. Code that compares two values with `==` or uses one as a map key no longer compiles (#88) - **Comparing installation token values, `*a == *b`, now panics** when GitHub's answer diff --git a/auth.go b/auth.go index 6e887b2..636e09c 100644 --- a/auth.go +++ b/auth.go @@ -15,7 +15,7 @@ import ( "strings" "time" - jwt "github.com/golang-jwt/jwt/v5" + "github.com/golang-jwt/jwt/v5" "golang.org/x/oauth2" ) @@ -339,7 +339,7 @@ func WithHTTPClient(client *http.Client) InstallationTokenSourceOpt { // one's error is reported. func WithEnterpriseURL(baseURL string) InstallationTokenSourceOpt { return func(i *installationTokenSource) { - if _, err := i.client.withEnterpriseURL(baseURL); err != nil { + if err := i.client.withEnterpriseURL(baseURL); err != nil { i.setConfigErr(err) } } @@ -361,7 +361,7 @@ func WithEnterpriseURL(baseURL string) InstallationTokenSourceOpt { // options, the first one's error is reported. func WithBaseURL(baseURL string) InstallationTokenSourceOpt { return func(i *installationTokenSource) { - if _, err := i.client.withBaseURL(baseURL); err != nil { + if err := i.client.withBaseURL(baseURL); err != nil { i.setConfigErr(err) } } @@ -370,8 +370,8 @@ func WithBaseURL(baseURL string) InstallationTokenSourceOpt { // WithContext sets the context every token request runs under, retry wait // included; the default is context.Background. Once it is done, every later // request fails with its error, so the source stops refreshing once its cached -// token goes stale; prefer a context that lives as long as the source. It is stored on the source because oauth2.TokenSource.Token takes -// no context. +// token goes stale; prefer a context that lives as long as the source. It is +// stored on the source because oauth2.TokenSource.Token takes no context. func WithContext(ctx context.Context) InstallationTokenSourceOpt { return func(i *installationTokenSource) { i.ctx = ctx @@ -469,8 +469,9 @@ func NewInstallationTokenSource(id int64, src oauth2.TokenSource, opts ...Instal // RevokeInstallationToken revokes an installation access token before its hour // is up, so a copy that leaked stops working. A source from // NewInstallationTokenSource that handed the token out does not know, and keeps -// returning it from its cache until it expires; build a new source instead. WithBaseURL, WithEnterpriseURL and -// WithHTTPClient apply as they do to NewInstallationTokenSource. The other +// returning it from its cache until it expires; build a new source instead. +// WithBaseURL, WithEnterpriseURL and WithHTTPClient apply as they do to +// NewInstallationTokenSource. The other // options are ignored. // // See https://docs.github.com/en/rest/apps/installations?apiVersion=2022-11-28#revoke-an-installation-access-token @@ -535,7 +536,8 @@ func cachedSource(src oauth2.TokenSource) oauth2.TokenSource { return ReuseTokenSourceWithSkew(nil, src, DefaultExpirySkew) } -// Token generates a new GitHub App installation token for authenticating as a GitHub App installation. +// Token mints an installation access token, carrying what GitHub granted it in +// its Extra fields. func (t *installationTokenSource) Token() (*oauth2.Token, error) { if t.configErr != nil { return nil, t.configErr @@ -598,7 +600,7 @@ func NewPersonalAccessTokenSource(token string) oauth2.TokenSource { // remain valid until manually revoked or expired by GitHub. func (t *personalAccessTokenSource) Token() (*oauth2.Token, error) { if t.token == "" { - return nil, errors.New("token not provided") + return nil, errors.New("personal access token is required") } return &oauth2.Token{ diff --git a/auth_test.go b/auth_test.go index 19c175a..a1b8845 100644 --- a/auth_test.go +++ b/auth_test.go @@ -23,15 +23,12 @@ import ( "testing" "time" - jwt "github.com/golang-jwt/jwt/v5" + "github.com/golang-jwt/jwt/v5" "golang.org/x/oauth2" ) func TestNewApplicationTokenSource(t *testing.T) { - privateKey, err := generatePrivateKey() - if err != nil { - t.Fatal(err) - } + privateKey := testKeyPEM(t) tests := []struct { name string @@ -93,29 +90,26 @@ func TestNewApplicationTokenSource(t *testing.T) { } func TestApplicationTokenSource_Token(t *testing.T) { - privateKey, err := generatePrivateKey() - if err != nil { - t.Fatal(err) - } + privateKey := testKeyPEM(t) tests := []struct { - name string - new func() (oauth2.TokenSource, error) - expectedIss string + name string + new func() (oauth2.TokenSource, error) + wantIss string }{ { name: "numeric app id token generation", new: func() (oauth2.TokenSource, error) { return NewApplicationTokenSource(int64(12345), privateKey) }, - expectedIss: "12345", + wantIss: "12345", }, { name: "client id token generation", new: func() (oauth2.TokenSource, error) { return NewApplicationTokenSource("Iv1.1234567890abcdef", privateKey) }, - expectedIss: "Iv1.1234567890abcdef", + wantIss: "Iv1.1234567890abcdef", }, } @@ -123,22 +117,22 @@ func TestApplicationTokenSource_Token(t *testing.T) { t.Run(tt.name, func(t *testing.T) { tokenSource, err := tt.new() if err != nil { - t.Fatalf("Failed to create token source: %v", err) + t.Fatalf("NewApplicationTokenSource() error = %v", err) } token, err := tokenSource.Token() if err != nil { - t.Fatalf("Failed to generate token: %v", err) + t.Fatalf("Token() error = %v", err) } if token.AccessToken == "" { - t.Error("Token access token is empty") + t.Error("Token().AccessToken is empty") } if token.TokenType != "Bearer" { - t.Errorf("Expected token type 'Bearer', got %s", token.TokenType) + t.Errorf("Token().TokenType = %q, want %q", token.TokenType, "Bearer") } if token.Expiry.IsZero() { - t.Error("Token expiry is not set") + t.Error("Token().Expiry is zero, want it set") } // Parse and verify JWT claims @@ -150,32 +144,21 @@ func TestApplicationTokenSource_Token(t *testing.T) { return &privKey.PublicKey, nil }) if err != nil { - t.Fatalf("Failed to parse JWT token: %v", err) + t.Fatalf("parsing the JWT: %v", err) } claims, ok := jwtToken.Claims.(*jwt.RegisteredClaims) if !ok { - t.Fatal("Failed to get JWT claims") + t.Fatalf("JWT claims are %T, want *jwt.RegisteredClaims", jwtToken.Claims) } - if claims.Issuer != tt.expectedIss { - t.Errorf("Expected issuer %s, got %s", tt.expectedIss, claims.Issuer) + if claims.Issuer != tt.wantIss { + t.Errorf("iss = %q, want %q", claims.Issuer, tt.wantIss) } }) } } -func TestApplicationTokenSource_Token_SigningError(t *testing.T) { - // Create an invalid private key that will cause signing to fail - invalidKey := []byte("invalid key") - - // This should fail at NewApplicationTokenSource due to invalid PEM - _, err := NewApplicationTokenSource(int64(12345), invalidKey) - if err == nil { - t.Fatal("Expected error for invalid private key, got nil") - } -} - // stubSigner is a test double implementing crypto.Signer. type stubSigner struct { pub crypto.PublicKey @@ -326,9 +309,8 @@ func TestApplicationTokenSource_FromSigner_PropagatesError(t *testing.T) { } } -// TestApplicationTokenSource_JWTContract is a senior-grade contract test that -// asserts the full JWT protocol compliance across both the PEM-encoded path -// and the crypto.Signer path, using the same underlying RSA key. +// TestApplicationTokenSource_JWTContract checks the PEM path and the +// crypto.Signer path mint the same JWT, from the same RSA key. // // It covers the three gaps previously defended only by reasoning: // @@ -466,10 +448,7 @@ func TestApplicationTokenSource_JWTContract(t *testing.T) { } func TestWithEnterpriseURL_InvalidURL(t *testing.T) { - privateKey, err := generatePrivateKey() - if err != nil { - t.Fatal(err) - } + privateKey := testKeyPEM(t) appSrc, err := NewApplicationTokenSource(int64(12345), privateKey) if err != nil { @@ -485,7 +464,7 @@ func TestWithEnterpriseURL_InvalidURL(t *testing.T) { ) if installationTokenSource == nil { - t.Fatal("Expected non-nil token source") + t.Fatal("NewInstallationTokenSource() = nil, want a source") } if _, err := installationTokenSource.Token(); err == nil { @@ -494,10 +473,7 @@ func TestWithEnterpriseURL_InvalidURL(t *testing.T) { } func TestWithBaseURL_InvalidURL(t *testing.T) { - privateKey, err := generatePrivateKey() - if err != nil { - t.Fatal(err) - } + privateKey := testKeyPEM(t) appSrc, err := NewApplicationTokenSource(int64(12345), privateKey) if err != nil { @@ -513,7 +489,7 @@ func TestWithBaseURL_InvalidURL(t *testing.T) { ) if installationTokenSource == nil { - t.Fatal("Expected non-nil token source") + t.Fatal("NewInstallationTokenSource() = nil, want a source") } if _, err := installationTokenSource.Token(); err == nil { @@ -543,10 +519,7 @@ func TestWithBaseURL_DrivesTokenThroughServer(t *testing.T) { })) defer server.Close() - privateKey, err := generatePrivateKey() - if err != nil { - t.Fatal(err) - } + privateKey := testKeyPEM(t) appSrc, err := NewApplicationTokenSource(int64(34434), privateKey) if err != nil { @@ -602,10 +575,7 @@ func TestWithBaseURL_SurvivesHTTPClientOrder(t *testing.T) { })) defer server.Close() - privateKey, err := generatePrivateKey() - if err != nil { - t.Fatal(err) - } + privateKey := testKeyPEM(t) appSrc, err := NewApplicationTokenSource(int64(1), privateKey) if err != nil { t.Fatal(err) @@ -626,10 +596,7 @@ func TestWithBaseURL_SurvivesHTTPClientOrder(t *testing.T) { } func TestWithHTTPClient_NilClient(t *testing.T) { - privateKey, err := generatePrivateKey() - if err != nil { - t.Fatal(err) - } + privateKey := testKeyPEM(t) appSrc, err := NewApplicationTokenSource(int64(1), privateKey) if err != nil { t.Fatal(err) @@ -643,10 +610,7 @@ func TestWithHTTPClient_NilClient(t *testing.T) { } func TestWithHTTPClient_DoesNotMutateCallerClient(t *testing.T) { - privateKey, err := generatePrivateKey() - if err != nil { - t.Fatal(err) - } + privateKey := testKeyPEM(t) appSrc, err := NewApplicationTokenSource(int64(1), privateKey) if err != nil { t.Fatal(err) @@ -665,85 +629,57 @@ func TestWithHTTPClient_DoesNotMutateCallerClient(t *testing.T) { } func Test_installationTokenSource_Token(t *testing.T) { - now := time.Now().UTC() - expiration := now.Add(10 * time.Minute) + expiration := time.Now().UTC().Add(10 * time.Minute) - mockedHTTPClient, cleanupSuccess := newMockedHTTPClient( - withRequestMatch( - postAppInstallationsAccessTokensByInstallationID, - InstallationToken{ - Token: "mocked-installation-token", - ExpiresAt: expiration, - Permissions: &InstallationPermissions{ - PullRequests: Ptr("read"), - Other: map[string]string{"installation_repositories": "write"}, - }, - Repositories: []Repository{ - { - Name: Ptr("mocked-repo-1"), - ID: Ptr(int64(1)), - }, - {ID: Ptr(int64(2))}, - }, + // An Enterprise Server, so the request has to keep the /api/v3/ path. + ok := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodPost || r.URL.Path != "/api/v3/app/installations/1/access_tokens" { + http.NotFound(w, r) + return + } + w.WriteHeader(http.StatusCreated) + _ = json.NewEncoder(w).Encode(InstallationToken{ + Token: "mocked-installation-token", + ExpiresAt: expiration, + Permissions: &InstallationPermissions{ + PullRequests: Ptr("read"), + Other: map[string]string{"installation_repositories": "write"}, }, - ), - ) - defer cleanupSuccess() - - errMockedHTTPClient, cleanupError := newMockedHTTPClient( - withRequestMatchHandler( - postAppInstallationsAccessTokensByInstallationID, - http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(http.StatusInternalServerError) - _, _ = w.Write([]byte(`{"message":"Internal Server Error"}`)) - }), - )) - defer cleanupError() - - privateKey, err := generatePrivateKey() - if err != nil { - t.Fatal(err) - } + Repositories: []Repository{ + {Name: Ptr("mocked-repo-1"), ID: Ptr(int64(1))}, + {ID: Ptr(int64(2))}, + }, + }) + })) + defer ok.Close() + failing := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + _, _ = w.Write([]byte(`{"message":"Internal Server Error"}`)) + })) + defer failing.Close() - appSrc, err := NewApplicationTokenSource(int64(34434), privateKey, WithApplicationTokenExpiration(5*time.Minute)) + appSrc, err := NewApplicationTokenSource(int64(34434), testKeyPEM(t), WithApplicationTokenExpiration(5*time.Minute)) if err != nil { t.Fatal(err) } - type fields struct { - id int64 - src oauth2.TokenSource - opts []InstallationTokenSourceOpt - } tests := []struct { name string - fields fields + opts []InstallationTokenSourceOpt want *oauth2.Token wantErr bool }{ { - name: "error getting installation token", - fields: fields{ - id: 1, - src: appSrc, - opts: []InstallationTokenSourceOpt{ - WithInstallationTokenOptions(&InstallationTokenOptions{}), - WithHTTPClient(errMockedHTTPClient), - }, - }, + name: "error getting installation token", + opts: []InstallationTokenSourceOpt{WithInstallationTokenOptions(&InstallationTokenOptions{}), WithEnterpriseURL(failing.URL)}, wantErr: true, }, { name: "generate a new installation token", - fields: fields{ - id: 1, - src: appSrc, - opts: []InstallationTokenSourceOpt{ - WithInstallationTokenOptions(&InstallationTokenOptions{}), - WithContext(context.Background()), - WithEnterpriseURL("https://github.example.com"), - WithHTTPClient(mockedHTTPClient), - }, + opts: []InstallationTokenSourceOpt{ + WithInstallationTokenOptions(&InstallationTokenOptions{}), + WithContext(context.Background()), + WithEnterpriseURL(ok.URL), }, // GitHub's answer rides along, including a permission with no field. want: (&oauth2.Token{ @@ -758,48 +694,12 @@ func Test_installationTokenSource_Token(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - tr := NewInstallationTokenSource(tt.fields.id, tt.fields.src, tt.fields.opts...) - - got, err := tr.Token() + got, err := NewInstallationTokenSource(1, appSrc, tt.opts...).Token() if (err != nil) != tt.wantErr { - t.Errorf("installationTokenSource.Token() error = %v, wantErr %v", err, tt.wantErr) - return + t.Fatalf("Token() error = %v, wantErr %v", err, tt.wantErr) } if !sameToken(got, tt.want) { - t.Errorf("installationTokenSource.Token() = %v, want %v", got, tt.want) - } - }) - } -} - -func TestNewPersonalAccessTokenSource(t *testing.T) { - tests := []struct { - name string - token string - want oauth2.TokenSource - }{ - { - name: "empty token", - token: "", - want: &personalAccessTokenSource{token: ""}, - }, - { - name: "classic personal access token", - token: "ghp_1234567890abcdefghijklmnopqrstuvwxyz123456", - want: &personalAccessTokenSource{token: "ghp_1234567890abcdefghijklmnopqrstuvwxyz123456"}, - }, - { - name: "fine-grained personal access token", - token: "github_pat_11ABCDEFG0123456789_abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ", - want: &personalAccessTokenSource{token: "github_pat_11ABCDEFG0123456789_abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ"}, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got := NewPersonalAccessTokenSource(tt.token) - if !reflect.DeepEqual(got, tt.want) { - t.Errorf("NewPersonalAccessTokenSource() = %v, want %v", got, tt.want) + t.Errorf("Token() = %v, want %v", got, tt.want) } }) } @@ -874,18 +774,14 @@ func TestPersonalAccessTokenSource_Token(t *testing.T) { } } -func generatePrivateKey() ([]byte, error) { - privateKey, err := rsa.GenerateKey(rand.Reader, 2048) +// testKeyPEM returns a fresh RSA private key in PEM form. +func testKeyPEM(t testing.TB) []byte { + t.Helper() + key, err := rsa.GenerateKey(rand.Reader, 2048) if err != nil { - return nil, err - } - // Encode the private key to the PEM format - privateKeyPEM := &pem.Block{ - Type: "RSA PRIVATE KEY", - Bytes: x509.MarshalPKCS1PrivateKey(privateKey), + t.Fatalf("generating a test key: %v", err) } - - return pem.EncodeToMemory(privateKeyPEM), nil + return pem.EncodeToMemory(&pem.Block{Type: "RSA PRIVATE KEY", Bytes: x509.MarshalPKCS1PrivateKey(key)}) } // countingSource is a test oauth2.TokenSource that counts Token() calls, @@ -935,272 +831,251 @@ func newShortLivedSource(validFor time.Duration) *countingSource { } func TestReuseTokenSourceWithSkew(t *testing.T) { - tests := []struct { - name string - run func(t *testing.T) - }{ - { - // Token valid for 1s, skew=500ms. First Token() caches; after 600ms - // the cached token is within the skew window and the next call - // must refresh. - name: "refreshes within skew window", - run: func(t *testing.T) { - src := newShortLivedSource(1 * time.Second) - ts := ReuseTokenSourceWithSkew(nil, src, 500*time.Millisecond) - - first, err := ts.Token() - if err != nil { - t.Fatalf("first Token(): %v", err) - } - if got := src.callCount(); got != 1 { - t.Fatalf("upstream calls after first Token() = %d, want 1", got) - } + // Token valid for 1s, skew=500ms. First Token() caches; after 600ms + // the cached token is within the skew window and the next call + // must refresh. + t.Run("refreshes within skew window", func(t *testing.T) { + src := newShortLivedSource(1 * time.Second) + ts := ReuseTokenSourceWithSkew(nil, src, 500*time.Millisecond) + + first, err := ts.Token() + if err != nil { + t.Fatalf("first Token(): %v", err) + } + if got := src.callCount(); got != 1 { + t.Fatalf("upstream calls after first Token() = %d, want 1", got) + } - // Cached call while still outside the skew window: no refresh. - cached, err := ts.Token() - if err != nil { - t.Fatalf("cached Token(): %v", err) - } - if got := src.callCount(); got != 1 { - t.Fatalf("upstream calls after cached Token() = %d, want 1", got) - } - if cached.AccessToken != first.AccessToken { - t.Errorf("cached token = %q, want %q", cached.AccessToken, first.AccessToken) - } + // Cached call while still outside the skew window: no refresh. + cached, err := ts.Token() + if err != nil { + t.Fatalf("cached Token(): %v", err) + } + if got := src.callCount(); got != 1 { + t.Fatalf("upstream calls after cached Token() = %d, want 1", got) + } + if cached.AccessToken != first.AccessToken { + t.Errorf("cached token = %q, want %q", cached.AccessToken, first.AccessToken) + } - // Enter the skew window (600ms elapsed, 400ms to exp ≤ 500ms skew). - time.Sleep(600 * time.Millisecond) + // Enter the skew window (600ms elapsed, 400ms to exp ≤ 500ms skew). + time.Sleep(600 * time.Millisecond) - refreshed, err := ts.Token() - if err != nil { - t.Fatalf("refresh Token(): %v", err) - } - if got := src.callCount(); got != 2 { - t.Errorf("upstream calls after refresh = %d, want 2", got) - } - if refreshed.AccessToken == first.AccessToken { - t.Errorf("refreshed token = %q, want a new token", refreshed.AccessToken) - } - }, - }, - { - // Skew 2s, tokens valid for 1s. Every call lands inside the skew - // window, so the cache never hits. - name: "skew exceeds validity always refreshes", - run: func(t *testing.T) { - src := newShortLivedSource(1 * time.Second) - ts := ReuseTokenSourceWithSkew(nil, src, 2*time.Second) - - const iterations = 5 - var last string - for i := range iterations { - tok, err := ts.Token() - if err != nil { - t.Fatalf("Token() #%d: %v", i, err) - } - if tok.AccessToken == last { - t.Errorf("Token() #%d returned cached %q, want refresh", i, tok.AccessToken) + refreshed, err := ts.Token() + if err != nil { + t.Fatalf("refresh Token(): %v", err) + } + if got := src.callCount(); got != 2 { + t.Errorf("upstream calls after refresh = %d, want 2", got) + } + if refreshed.AccessToken == first.AccessToken { + t.Errorf("refreshed token = %q, want a new token", refreshed.AccessToken) + } + }) + + // Skew 2s, tokens valid for 1s. Every call lands inside the skew + // window, so the cache never hits. + t.Run("skew exceeds validity always refreshes", func(t *testing.T) { + src := newShortLivedSource(1 * time.Second) + ts := ReuseTokenSourceWithSkew(nil, src, 2*time.Second) + + const iterations = 5 + var last string + for i := range iterations { + tok, err := ts.Token() + if err != nil { + t.Fatalf("Token() #%d: %v", i, err) + } + if tok.AccessToken == last { + t.Errorf("Token() #%d returned cached %q, want refresh", i, tok.AccessToken) + } + last = tok.AccessToken + } + if got := src.callCount(); got != iterations { + t.Errorf("upstream calls = %d, want %d", got, iterations) + } + }) + + // Regression: with skew<=0 the wrapper must match + // oauth2.ReuseTokenSource exactly. We run identical scenarios + // through both implementations and assert the same upstream call + // counts and the same AccessTokens at every step. + t.Run("zero skew matches ReuseTokenSource", func(t *testing.T) { + scenarios := []struct { + name string + seed *oauth2.Token + makeFn func(call int) *oauth2.Token + calls int + }{ + { + name: "nil seed, long-lived tokens, repeated calls cache", + seed: nil, + makeFn: func(call int) *oauth2.Token { + return &oauth2.Token{ + AccessToken: fmt.Sprintf("long-%d", call), + TokenType: "Bearer", + Expiry: time.Now().Add(1 * time.Hour), } - last = tok.AccessToken - } - if got := src.callCount(); got != iterations { - t.Errorf("upstream calls = %d, want %d", got, iterations) - } + }, + calls: 5, }, - }, - { - // Regression: with skew<=0 the wrapper must match - // oauth2.ReuseTokenSource exactly. We run identical scenarios - // through both implementations and assert the same upstream call - // counts and the same AccessTokens at every step. - name: "zero skew matches ReuseTokenSource", - run: func(t *testing.T) { - scenarios := []struct { - name string - seed *oauth2.Token - makeFn func(call int) *oauth2.Token - calls int - }{ - { - name: "nil seed, long-lived tokens, repeated calls cache", - seed: nil, - makeFn: func(call int) *oauth2.Token { - return &oauth2.Token{ - AccessToken: fmt.Sprintf("long-%d", call), - TokenType: "Bearer", - Expiry: time.Now().Add(1 * time.Hour), - } - }, - calls: 5, - }, - { - name: "pre-seeded long-lived token is reused", - seed: &oauth2.Token{ - AccessToken: "seed", - TokenType: "Bearer", - Expiry: time.Now().Add(1 * time.Hour), - }, - makeFn: func(call int) *oauth2.Token { - return &oauth2.Token{ - AccessToken: fmt.Sprintf("fresh-%d", call), - TokenType: "Bearer", - Expiry: time.Now().Add(1 * time.Hour), - } - }, - calls: 3, - }, - } - for _, s := range scenarios { - t.Run(s.name, func(t *testing.T) { - skewSrc := &countingSource{mkToken: s.makeFn} - plainSrc := &countingSource{mkToken: s.makeFn} - - skewTS := ReuseTokenSourceWithSkew(s.seed, skewSrc, 0) - plainTS := oauth2.ReuseTokenSource(s.seed, plainSrc) - - for i := 0; i < s.calls; i++ { - skewTok, skewErr := skewTS.Token() - plainTok, plainErr := plainTS.Token() - - if (skewErr == nil) != (plainErr == nil) { - t.Fatalf("call %d error mismatch: skew=%v plain=%v", i, skewErr, plainErr) - } - if skewErr != nil { - continue - } - if skewTok.AccessToken != plainTok.AccessToken { - t.Errorf("call %d token mismatch: skew=%q plain=%q", i, skewTok.AccessToken, plainTok.AccessToken) - } - } - if skewSrc.callCount() != plainSrc.callCount() { - t.Errorf("upstream call counts differ: skew=%d plain=%d", skewSrc.callCount(), plainSrc.callCount()) - } - }) - } + { + name: "pre-seeded long-lived token is reused", + seed: &oauth2.Token{ + AccessToken: "seed", + TokenType: "Bearer", + Expiry: time.Now().Add(1 * time.Hour), + }, + makeFn: func(call int) *oauth2.Token { + return &oauth2.Token{ + AccessToken: fmt.Sprintf("fresh-%d", call), + TokenType: "Bearer", + Expiry: time.Now().Add(1 * time.Hour), + } + }, + calls: 3, }, - }, - { - // 100 goroutines fire Token() against a slow upstream that mints a - // long-lived token. The mutex must funnel them so only the first - // reaches upstream; the rest return the cached value. - name: "concurrent Token calls collapse to one upstream fetch", - run: func(t *testing.T) { - src := &countingSource{ - delay: 50 * time.Millisecond, - mkToken: func(call int) *oauth2.Token { - return &oauth2.Token{ - AccessToken: fmt.Sprintf("token-%d", call), - TokenType: "Bearer", - Expiry: time.Now().Add(1 * time.Hour), - } - }, - } - ts := ReuseTokenSourceWithSkew(nil, src, 30*time.Second) - - const goroutines = 100 - var ( - wg sync.WaitGroup - mu sync.Mutex - tokens = make(map[string]int, goroutines) - errs []error - ) - wg.Add(goroutines) - start := make(chan struct{}) - for range goroutines { - go func() { - defer wg.Done() - <-start - tok, err := ts.Token() - mu.Lock() - defer mu.Unlock() - if err != nil { - errs = append(errs, err) - return - } - tokens[tok.AccessToken]++ - }() - } - close(start) - wg.Wait() + } + for _, s := range scenarios { + t.Run(s.name, func(t *testing.T) { + skewSrc := &countingSource{mkToken: s.makeFn} + plainSrc := &countingSource{mkToken: s.makeFn} - if len(errs) != 0 { - t.Fatalf("Token() errors: %v", errs) - } - if got := src.callCount(); got != 1 { - t.Errorf("upstream calls = %d, want 1", got) - } - if len(tokens) != 1 { - t.Errorf("distinct tokens = %d, want 1 (%v)", len(tokens), tokens) + skewTS := ReuseTokenSourceWithSkew(s.seed, skewSrc, 0) + plainTS := oauth2.ReuseTokenSource(s.seed, plainSrc) + + for i := range s.calls { + skewTok, skewErr := skewTS.Token() + plainTok, plainErr := plainTS.Token() + + if (skewErr == nil) != (plainErr == nil) { + t.Fatalf("call %d error mismatch: skew=%v plain=%v", i, skewErr, plainErr) + } + if skewErr != nil { + continue + } + if skewTok.AccessToken != plainTok.AccessToken { + t.Errorf("call %d token mismatch: skew=%q plain=%q", i, skewTok.AccessToken, plainTok.AccessToken) + } } - if tokens["token-1"] != goroutines { - t.Errorf("goroutines that got token-1 = %d, want %d", tokens["token-1"], goroutines) + if skewSrc.callCount() != plainSrc.callCount() { + t.Errorf("upstream call counts differ: skew=%d plain=%d", skewSrc.callCount(), plainSrc.callCount()) } - }, - }, - { - // A valid seed token passed into the wrapper must be served - // without touching upstream. This is the hot-start path: callers - // rehydrating a cached token from disk or config should not incur - // an extra round trip. - name: "valid seed token served without upstream call", - run: func(t *testing.T) { - seed := &oauth2.Token{ - AccessToken: "seeded", + }) + } + }) + + // 100 goroutines fire Token() against a slow upstream that mints a + // long-lived token. The mutex must funnel them so only the first + // reaches upstream; the rest return the cached value. + t.Run("concurrent Token calls collapse to one upstream fetch", func(t *testing.T) { + src := &countingSource{ + delay: 50 * time.Millisecond, + mkToken: func(call int) *oauth2.Token { + return &oauth2.Token{ + AccessToken: fmt.Sprintf("token-%d", call), TokenType: "Bearer", Expiry: time.Now().Add(1 * time.Hour), } - src := &countingSource{ - mkToken: func(call int) *oauth2.Token { - return &oauth2.Token{ - AccessToken: fmt.Sprintf("fresh-%d", call), - TokenType: "Bearer", - Expiry: time.Now().Add(1 * time.Hour), - } - }, - } - ts := ReuseTokenSourceWithSkew(seed, src, 30*time.Second) - + }, + } + ts := ReuseTokenSourceWithSkew(nil, src, 30*time.Second) + + const goroutines = 100 + var ( + wg sync.WaitGroup + mu sync.Mutex + tokens = make(map[string]int, goroutines) + errs []error + ) + wg.Add(goroutines) + start := make(chan struct{}) + for range goroutines { + go func() { + defer wg.Done() + <-start tok, err := ts.Token() + mu.Lock() + defer mu.Unlock() if err != nil { - t.Fatalf("Token(): %v", err) - } - if tok.AccessToken != "seeded" { - t.Errorf("AccessToken = %q, want %q (seed)", tok.AccessToken, "seeded") + errs = append(errs, err) + return } - if got := src.callCount(); got != 0 { - t.Errorf("upstream calls = %d, want 0 (seed should short-circuit)", got) - } - }, - }, - { - // Upstream errors must surface to the caller verbatim (wrappable - // with errors.Is) and must not be cached — the next call has to - // retry the upstream. Caching a failure would turn a transient - // hiccup into a permanent outage. - name: "upstream error is propagated and not cached", - run: func(t *testing.T) { - sentinel := errors.New("upstream unavailable") - src := &countingSource{ - err: sentinel, - mkToken: func(int) *oauth2.Token { return nil }, - } - ts := ReuseTokenSourceWithSkew(nil, src, 30*time.Second) + tokens[tok.AccessToken]++ + }() + } + close(start) + wg.Wait() - for i := range 3 { - _, err := ts.Token() - if !errors.Is(err, sentinel) { - t.Errorf("call %d: err = %v, want errors.Is(%v)", i, err, sentinel) - } - } - if got := src.callCount(); got != 3 { - t.Errorf("upstream calls = %d, want 3 (errors must not be cached)", got) + if len(errs) != 0 { + t.Fatalf("Token() errors: %v", errs) + } + if got := src.callCount(); got != 1 { + t.Errorf("upstream calls = %d, want 1", got) + } + if len(tokens) != 1 { + t.Errorf("distinct tokens = %d, want 1 (%v)", len(tokens), tokens) + } + if tokens["token-1"] != goroutines { + t.Errorf("goroutines that got token-1 = %d, want %d", tokens["token-1"], goroutines) + } + }) + + // A valid seed token passed into the wrapper must be served + // without touching upstream. This is the hot-start path: callers + // rehydrating a cached token from disk or config should not incur + // an extra round trip. + t.Run("valid seed token served without upstream call", func(t *testing.T) { + seed := &oauth2.Token{ + AccessToken: "seeded", + TokenType: "Bearer", + Expiry: time.Now().Add(1 * time.Hour), + } + src := &countingSource{ + mkToken: func(call int) *oauth2.Token { + return &oauth2.Token{ + AccessToken: fmt.Sprintf("fresh-%d", call), + TokenType: "Bearer", + Expiry: time.Now().Add(1 * time.Hour), } }, - }, - } - for _, tt := range tests { - t.Run(tt.name, tt.run) - } + } + ts := ReuseTokenSourceWithSkew(seed, src, 30*time.Second) + + tok, err := ts.Token() + if err != nil { + t.Fatalf("Token(): %v", err) + } + if tok.AccessToken != "seeded" { + t.Errorf("AccessToken = %q, want %q (seed)", tok.AccessToken, "seeded") + } + if got := src.callCount(); got != 0 { + t.Errorf("upstream calls = %d, want 0 (seed should short-circuit)", got) + } + }) + + // Upstream errors must surface to the caller verbatim (wrappable + // with errors.Is) and must not be cached — the next call has to + // retry the upstream. Caching a failure would turn a transient + // hiccup into a permanent outage. + t.Run("upstream error is propagated and not cached", func(t *testing.T) { + sentinel := errors.New("upstream unavailable") + src := &countingSource{ + err: sentinel, + mkToken: func(int) *oauth2.Token { return nil }, + } + ts := ReuseTokenSourceWithSkew(nil, src, 30*time.Second) + + for i := range 3 { + _, err := ts.Token() + if !errors.Is(err, sentinel) { + t.Errorf("call %d: err = %v, want errors.Is(%v)", i, err, sentinel) + } + } + if got := src.callCount(); got != 3 { + t.Errorf("upstream calls = %d, want 3 (errors must not be cached)", got) + } + }) } // TestReuseTokenSourceWithSkew_ThroughNewClient checks the skew survives @@ -1257,11 +1132,11 @@ type tokenSourceFunc func() (*oauth2.Token, error) func (f tokenSourceFunc) Token() (*oauth2.Token, error) { return f() } -// Test_WithInstallationExpirySkew_AppliesToCache proves the configured window is +// TestWithInstallationExpirySkew_AppliesToCache proves the configured window is // the one the cache actually refreshes on, not just the branch it selects. The // installation token expires in 2s and the skew is 10s, so every Token() lands // inside the window and must refetch. -func Test_WithInstallationExpirySkew_AppliesToCache(t *testing.T) { +func TestWithInstallationExpirySkew_AppliesToCache(t *testing.T) { t.Parallel() var minted atomic.Int32 @@ -1312,11 +1187,11 @@ func (z *zeroExpiryTokenSource) Token() (*oauth2.Token, error) { }, nil } -// Test_ReuseTokenSourceWithSkew_ZeroExpiry proves a token with no expiry is +// TestReuseTokenSourceWithSkew_ZeroExpiry proves a token with no expiry is // cached indefinitely. The skew is a comparison against exp, so with no exp // there is nothing to refresh early against: treating the zero time as "already // expired" would re-mint on every single call. -func Test_ReuseTokenSourceWithSkew_ZeroExpiry(t *testing.T) { +func TestReuseTokenSourceWithSkew_ZeroExpiry(t *testing.T) { t.Parallel() src := &zeroExpiryTokenSource{} @@ -1360,12 +1235,12 @@ func (c *countingTokenSource) Token() (*oauth2.Token, error) { }, nil } -// Test_WithHTTPClient_CachesApplicationToken proves WithHTTPClient caches the +// TestWithHTTPClient_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) { +func TestWithHTTPClient_CachesApplicationToken(t *testing.T) { src := &countingTokenSource{} // expires_at inside the skew window, so the installation-token cache @@ -1422,11 +1297,11 @@ func Test_cachedSource_KeepsTheSkew(t *testing.T) { } } -// Test_NewInstallationTokenSource_RejectsZeroID proves a zero installation ID is +// TestNewInstallationTokenSource_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) { +func TestNewInstallationTokenSource_RejectsZeroID(t *testing.T) { var hits atomic.Int32 server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { @@ -1529,7 +1404,7 @@ func Test_resolveIssuer_NamedIdentifierTypes(t *testing.T) { } } -func Test_NewApplicationTokenSource_NamedIdentifierTypes(t *testing.T) { +func TestNewApplicationTokenSource_NamedIdentifierTypes(t *testing.T) { rsaKey, err := rsa.GenerateKey(rand.Reader, 2048) if err != nil { t.Fatal(err) @@ -1613,10 +1488,9 @@ func issuerFromJWT(token string) (string, error) { // 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. +// (exp = now - 60s + expiration), so an expiration of 60s or less would mint a +// token whose exp has already passed. WithApplicationTokenExpiration therefore +// sends such values to the default, and this test keeps it doing so. // // 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 @@ -1734,13 +1608,13 @@ func (c *countingSigner) Sign(rand io.Reader, digest []byte, opts crypto.SignerO return c.Signer.Sign(rand, digest, opts) } -// Test_WithApplicationTokenExpiration_TokenIsCacheable proves the accepted +// TestWithApplicationTokenExpiration_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) { +func TestWithApplicationTokenExpiration_TokenIsCacheable(t *testing.T) { rsaKey, err := rsa.GenerateKey(rand.Reader, 2048) if err != nil { t.Fatal(err) @@ -1775,10 +1649,7 @@ func Test_WithApplicationTokenExpiration_TokenIsCacheable(t *testing.T) { func TestNewApplicationTokenSource_IntegerLiteral(t *testing.T) { t.Parallel() - key, err := generatePrivateKey() - if err != nil { - t.Fatal(err) - } + key := testKeyPEM(t) src, err := NewApplicationTokenSource(12345, key) if err != nil { t.Fatalf("NewApplicationTokenSource(12345) error = %v", err) @@ -1802,10 +1673,7 @@ func TestNewApplicationTokenSource_IntegerLiteral(t *testing.T) { func TestWithApplicationTokenExpiration_SharedOption(t *testing.T) { t.Parallel() - key, err := generatePrivateKey() - if err != nil { - t.Fatal(err) - } + key := testKeyPEM(t) opt := WithApplicationTokenExpiration(time.Hour) // out of range, so normalized var wg sync.WaitGroup for range 4 { diff --git a/cmd/githubauth/exec.go b/cmd/githubauth/exec.go index e54165e..7fa1fa6 100644 --- a/cmd/githubauth/exec.go +++ b/cmd/githubauth/exec.go @@ -39,6 +39,12 @@ var execEnterpriseEnv = []string{"GH_ENTERPRISE_TOKEN", "GITHUB_ENTERPRISE_TOKEN // The child's exit status becomes githubauth's, so --exec is transparent to // whatever inspects the result. func runWithToken(argv []string, tok *oauth2.Token, host string, stdin io.Reader, stdout, stderr io.Writer) error { + return runChild(argv, tok, host, inForeground, stdin, stdout, stderr) +} + +// runChild is runWithToken with the foreground check passed in, which a test +// cannot leave to whatever terminal it happens to run under. +func runChild(argv []string, tok *oauth2.Token, host string, inFg func(pid int) bool, stdin io.Reader, stdout, stderr io.Writer) error { all := append(slices.Clone(execTokenEnv), execEnterpriseEnv...) keys := execTokenEnv if isEnterpriseHost(host) { @@ -64,7 +70,6 @@ func runWithToken(argv []string, tok *oauth2.Token, host string, stdin io.Reader // its environment. A Ctrl-C at the keyboard reaches the child itself when // the child is in the terminal's foreground job, so then SIGINT is only // held, to keep githubauth alive until the child exits. - inFg := foreground sigs := make(chan os.Signal, 1) signal.Notify(sigs, os.Interrupt, syscall.SIGTERM) defer signal.Stop(sigs) @@ -118,7 +123,7 @@ const revokeTimeout = 10 * time.Second // returned func runs. Held from before the command starts until the token is // revoked, a signal in between cannot leave the token valid. revokeTimeout // bounds how long they stay held once the command has exited. -func holdSignals() (release func()) { +func holdSignals() func() { held := make(chan os.Signal, 1) signal.Notify(held, os.Interrupt, syscall.SIGTERM) return func() { signal.Stop(held) } @@ -137,7 +142,7 @@ func revoke(err error, tok *oauth2.Token, opts []githubauth.InstallationTokenSou // A 401 means the token is already dead: past its hour, or revoked by the // command itself. var apiErr *githubauth.APIError - if revokeErr == nil || errors.As(revokeErr, &apiErr) && apiErr.StatusCode == http.StatusUnauthorized { + if revokeErr == nil || (errors.As(revokeErr, &apiErr) && apiErr.StatusCode == http.StatusUnauthorized) { return err } @@ -176,10 +181,6 @@ func ghHost(apiURL string) string { return host } -// foreground is inForeground, replaceable by the tests, whose process group a -// developer's terminal may or may not hold. -var foreground = inForeground - // isEnterpriseHost mirrors gh's own test for whether a host reads the // enterprise variables: anything but github.com and data residency (*.ghe.com). func isEnterpriseHost(host string) bool { @@ -189,20 +190,18 @@ func isEnterpriseHost(host string) bool { // childEnviron is githubauth's environment without the App's private key or // any token already exported, for every process githubauth starts. func childEnviron() []string { - return environWithout(os.Environ(), append(append(slices.Clone(execTokenEnv), execEnterpriseEnv...), "GITHUB_APP_PRIVATE_KEY")...) + // Windows ignores case in variable names, so github_app_private_key is + // the same variable there. + return environWithout(os.Environ(), runtime.GOOS == "windows", append(append(slices.Clone(execTokenEnv), execEnterpriseEnv...), "GITHUB_APP_PRIVATE_KEY")...) } -// foldEnvNames is whether environment variable names ignore case, as on -// Windows, where github_app_private_key is the same variable. -var foldEnvNames = runtime.GOOS == "windows" - // environWithout drops keys from env so the token cannot be shadowed by one -// the caller already exported. -func environWithout(env []string, keys ...string) []string { +// the caller already exported. foldCase matches names ignoring case. +func environWithout(env []string, foldCase bool, keys ...string) []string { out := make([]string, 0, len(env)) for _, kv := range env { name, _, _ := strings.Cut(kv, "=") - if !slices.ContainsFunc(keys, func(k string) bool { return k == name || foldEnvNames && strings.EqualFold(k, name) }) { + if !slices.ContainsFunc(keys, func(k string) bool { return k == name || (foldCase && strings.EqualFold(k, name)) }) { out = append(out, kv) } } diff --git a/cmd/githubauth/exec_test.go b/cmd/githubauth/exec_test.go index 49c8801..3376eb3 100644 --- a/cmd/githubauth/exec_test.go +++ b/cmd/githubauth/exec_test.go @@ -9,6 +9,7 @@ import ( "runtime" "strconv" "strings" + "sync" "syscall" "testing" "time" @@ -232,57 +233,65 @@ func TestRunWithToken(t *testing.T) { // A supervisor stopping githubauth with SIGINT, with no terminal to have // sent the child its own, would otherwise leave the child running. - t.Run("SIGINT is forwarded to the child outside the terminal's foreground", func(t *testing.T) { - if runtime.GOOS == "windows" { - t.Skip("no signals on windows") - } - foreground = func(int) bool { return false } - t.Cleanup(func() { foreground = inForeground }) - t.Setenv("GITHUBAUTH_EXEC_HELPER", "1") - t.Setenv("GITHUBAUTH_EXEC_SLEEP", "1") - - go func() { - time.Sleep(500 * time.Millisecond) - p, _ := os.FindProcess(os.Getpid()) - _ = p.Signal(os.Interrupt) - }() - - var stdout, stderr bytes.Buffer - err := runWithToken(helperArgv(), tok, "", strings.NewReader(""), &stdout, &stderr) + // A supervisor stopping githubauth with SIGINT, with no terminal to have + // sent the child its own, would otherwise leave the child running. A + // SIGTERM, usually sent to githubauth alone, likewise. + signals := []struct { + name string + sig syscall.Signal + inForeground bool + }{ + {name: "SIGINT is forwarded to the child outside the terminal's foreground", sig: syscall.SIGINT}, + {name: "SIGTERM is forwarded to the child", sig: syscall.SIGTERM, inForeground: true}, + } + for _, tt := range signals { + t.Run(tt.name, func(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("no signals on windows") + } + t.Setenv("GITHUBAUTH_EXEC_HELPER", "1") + t.Setenv("GITHUBAUTH_EXEC_SLEEP", "1") + + // The helper prints before it sleeps, so its first output means it + // is running and runChild is already holding signals. + out := &readyWriter{ready: make(chan struct{})} + sent := make(chan struct{}) + go func() { + defer close(sent) + select { + case <-out.ready: + case <-time.After(5 * time.Second): + return + } + p, _ := os.FindProcess(os.Getpid()) + _ = p.Signal(tt.sig) + }() - var status exitStatus - if !errors.As(err, &status) || int(status) != 128+int(syscall.SIGINT) { - t.Errorf("runWithToken() error = %v, want exitStatus %d", err, 128+int(syscall.SIGINT)) - } - }) + var stderr bytes.Buffer + err := runChild(helperArgv(), tok, "", func(int) bool { return tt.inForeground }, strings.NewReader(""), out, &stderr) + <-sent - // A SIGTERM sent to githubauth alone would otherwise leave the child - // running with the credential in its environment. - t.Run("SIGTERM is forwarded to the child", func(t *testing.T) { - if runtime.GOOS == "windows" { - t.Skip("no signals on windows") - } - t.Setenv("GITHUBAUTH_EXEC_HELPER", "1") - t.Setenv("GITHUBAUTH_EXEC_SLEEP", "1") - - go func() { - time.Sleep(500 * time.Millisecond) - p, _ := os.FindProcess(os.Getpid()) - _ = p.Signal(syscall.SIGTERM) - }() + var status exitStatus + if !errors.As(err, &status) || int(status) != 128+int(tt.sig) { + t.Errorf("runChild() error = %v, want exitStatus %d", err, 128+int(tt.sig)) + } + }) + } +} - start := time.Now() - var stdout, stderr bytes.Buffer - err := runWithToken(helperArgv(), tok, "", strings.NewReader(""), &stdout, &stderr) +// readyWriter closes ready on the first write it receives. +type readyWriter struct { + once sync.Once + ready chan struct{} + mu sync.Mutex + buf bytes.Buffer +} - var status exitStatus - if !errors.As(err, &status) || int(status) != 128+int(syscall.SIGTERM) { - t.Errorf("runWithToken() error = %v, want exitStatus %d", err, 128+int(syscall.SIGTERM)) - } - if elapsed := time.Since(start); elapsed > 5*time.Second { - t.Errorf("elapsed = %v, want the child stopped by the forwarded signal", elapsed) - } - }) +func (w *readyWriter) Write(p []byte) (int, error) { + w.once.Do(func() { close(w.ready) }) + w.mu.Lock() + defer w.mu.Unlock() + return w.buf.Write(p) } func TestGhHost(t *testing.T) { @@ -324,7 +333,7 @@ func TestEnvironWithout(t *testing.T) { env := []string{"PATH=/bin", "GITHUB_TOKEN=stale", "GH_TOKEN=stale", "GITHUB_TOKEN_EXTRA=kept", "HOME=/home/x"} want := []string{"PATH=/bin", "GITHUB_TOKEN_EXTRA=kept", "HOME=/home/x"} - got := environWithout(env, "GITHUB_TOKEN", "GH_TOKEN") + got := environWithout(env, false, "GITHUB_TOKEN", "GH_TOKEN") if len(got) != len(want) { t.Fatalf("environWithout() = %q, want %q", got, want) } @@ -338,11 +347,7 @@ func TestEnvironWithout(t *testing.T) { // TestEnvironWithoutFoldsWhereTheOSDoes checks the private key is dropped // whatever its case on Windows, where the OS reads it case-insensitively. func TestEnvironWithoutFoldsWhereTheOSDoes(t *testing.T) { - was := foldEnvNames - foldEnvNames = true - t.Cleanup(func() { foldEnvNames = was }) - - got := environWithout([]string{"github_app_private_key=pem", "PATH=/bin"}, "GITHUB_APP_PRIVATE_KEY") + got := environWithout([]string{"github_app_private_key=pem", "PATH=/bin"}, true, "GITHUB_APP_PRIVATE_KEY") if len(got) != 1 || got[0] != "PATH=/bin" { t.Errorf("environWithout() = %q, want only PATH", got) } diff --git a/cmd/githubauth/main.go b/cmd/githubauth/main.go index be15834..d7b7894 100644 --- a/cmd/githubauth/main.go +++ b/cmd/githubauth/main.go @@ -188,8 +188,9 @@ func (a *appFlags) checkExec(argv []string) error { // checkGHHost refuses an exported GH_HOST that names another instance than // the one an installation token is for, before anything is minted: gh would // send the token there. The JWT is not checked: it is signed locally, and only -// the caller knows which instance its App lives on. A caller whose proxy really serves it sets GH_HOST inside -// the command instead, where nothing else in the environment can choose it. +// the caller knows which instance its App lives on. A caller whose proxy +// really serves it sets GH_HOST inside the command instead, where nothing else +// in the environment can choose it. func (a *appFlags) checkGHHost() error { want := cmp.Or(a.host, "github.com") // Compared exactly: gh treats github.com:443 as another host than github.com. @@ -205,7 +206,7 @@ func (a *appFlags) emit(tok *oauth2.Token, argv []string, stdin io.Reader, stdou if a.exec { return runWithToken(argv, tok, a.host, stdin, stdout, stderr) } - if err := write(stdout, tok, a.out.json); err != nil { + if err := writeToken(stdout, tok, a.out.json); err != nil { return err } if a.out.agent { @@ -268,130 +269,174 @@ func tokenCmd(args []string, stdin io.Reader, stdout, stderr io.Writer, out *out # GitHub Enterprise Server. Under --exec, $GH_HOST points gh at it. githubauth token --installation 12345 --enterprise-url https://github.example.com`) - var app appFlags - app.bind(fs, out) - - installation := fs.Int64("installation", envInt64("GITHUB_APP_INSTALLATION_ID"), "installation ID to mint the token for ($GITHUB_APP_INSTALLATION_ID)") - repos := fs.String("repos", "", "comma-separated repository names to scope the token to; owner/repo also finds the installation, like --owner") - owner := fs.String("owner", "", "user or organization the App is installed on, to look up the installation instead of passing --installation") - permissions := fs.String("permissions", "", "comma-separated name:level pairs to scope the token to, e.g. contents:read,pull_requests:write") - keep := fs.Bool("keep", false, "under --exec, leave the token valid when the command exits instead of revoking it") - baseURL := fs.String("base-url", "", "API base URL, used verbatim (Enterprise Cloud with data residency)") - enterpriseURL := fs.String("enterprise-url", "", "GitHub Enterprise Server URL, normalized the way GHES expects") - + var f tokenFlags + f.bind(fs, out) if err := parseFlags(fs, args, out, stdout, stderr); err != nil { return err } - if err := app.checkExec(fs.Args()); err != nil { + req, err := f.resolve() + if err != nil { + return err + } + appSrc, err := f.source(stdin) + if err != nil { + return err + } + + urlOpts := f.urlOptions() + tok, err := mint(appSrc, req, urlOpts) + if err != nil { return err } - if *keep && !app.exec { - return usagef("--keep applies only under --exec, which revokes the token when the command exits") + if f.exec && !f.keep { + defer holdSignals()() + } + err = f.emit(tok, fs.Args(), stdin, stdout, stderr) + // Even a command that failed to start leaves a minted token behind. + if f.exec && !f.keep { + err = revoke(err, tok, urlOpts, stderr) + } + return err +} + +// tokenFlags are the token command's flags, beside the ones every command has. +type tokenFlags struct { + appFlags + installation int64 + repos string + owner string + permissions string + keep bool + baseURL string + enterpriseURL string +} + +func (f *tokenFlags) bind(fs *flag.FlagSet, out *output) { + f.appFlags.bind(fs, out) + fs.Int64Var(&f.installation, "installation", envInt64("GITHUB_APP_INSTALLATION_ID"), "installation ID to mint the token for ($GITHUB_APP_INSTALLATION_ID)") + fs.StringVar(&f.repos, "repos", "", "comma-separated repository names to scope the token to; owner/repo also finds the installation, like --owner") + fs.StringVar(&f.owner, "owner", "", "user or organization the App is installed on, to look up the installation instead of passing --installation") + fs.StringVar(&f.permissions, "permissions", "", "comma-separated name:level pairs to scope the token to, e.g. contents:read,pull_requests:write") + fs.BoolVar(&f.keep, "keep", false, "under --exec, leave the token valid when the command exits instead of revoking it") + fs.StringVar(&f.baseURL, "base-url", "", "API base URL, used verbatim (Enterprise Cloud with data residency)") + fs.StringVar(&f.enterpriseURL, "enterprise-url", "", "GitHub Enterprise Server URL, normalized the way GHES expects") +} + +// tokenRequest is what the token flags resolve to: the installation, or the +// owner to look it up by, and how far to narrow the token. +type tokenRequest struct { + installation int64 + owner string + repos []string + permissions *githubauth.InstallationPermissions +} + +// resolve checks the flags against each other and works out the request, +// before any key is read or request made. It also sets f.host. +func (f *tokenFlags) resolve() (tokenRequest, error) { + if err := f.checkExec(f.fs.Args()); err != nil { + return tokenRequest{}, err + } + if f.keep && !f.exec { + return tokenRequest{}, usagef("--keep applies only under --exec, which revokes the token when the command exits") } // An empty scope is no scope: GitHub would mint a token for every // repository, the opposite of what passing --repos asked for. - names, repoOwner, err := splitOwner(splitRepos(*repos)) + names, repoOwner, err := splitOwner(splitList(f.repos)) if err != nil { - return err + return tokenRequest{}, err } - if isSet(fs, "repos") && len(names) == 0 { - return usagef("--repos names no repository; omit it for a token covering the whole installation") + if isSet(f.fs, "repos") && len(names) == 0 { + return tokenRequest{}, usagef("--repos names no repository; omit it for a token covering the whole installation") } + owner := f.owner // An owner/repo name finds the installation the way --owner does. if repoOwner != "" { switch { - case isSet(fs, "installation"): - return usagef("--repos names the owner %s, which finds the installation: drop --installation, or the %s/ prefix", repoOwner, repoOwner) - case *owner != "" && !strings.EqualFold(*owner, repoOwner): - return usagef("--repos names repositories of %s, but --owner is %s", repoOwner, *owner) + case isSet(f.fs, "installation"): + return tokenRequest{}, usagef("--repos names the owner %s, which finds the installation: drop --installation, or the %s/ prefix", repoOwner, repoOwner) + case owner != "" && !strings.EqualFold(owner, repoOwner): + return tokenRequest{}, usagef("--repos names repositories of %s, but --owner is %s", repoOwner, owner) } - *owner = repoOwner + owner = repoOwner } - if *baseURL != "" && *enterpriseURL != "" { - return usagef("set --base-url or --enterprise-url, not both") + if f.baseURL != "" && f.enterpriseURL != "" { + return tokenRequest{}, usagef("set --base-url or --enterprise-url, not both") } - if err := checkURL("--base-url", *baseURL); err != nil { - return err + if err := checkURL("--base-url", f.baseURL); err != nil { + return tokenRequest{}, err } - if err := checkURL("--enterprise-url", *enterpriseURL); err != nil { - return err + if err := checkURL("--enterprise-url", f.enterpriseURL); err != nil { + return tokenRequest{}, err } - app.host = ghHost(*baseURL + *enterpriseURL) // at most one is set - if err := app.checkGHHost(); err != nil { - return err + f.host = ghHost(cmp.Or(f.baseURL, f.enterpriseURL)) + if err := f.checkGHHost(); err != nil { + return tokenRequest{}, err } // Nothing names the installation, so the clone the command runs in does, // the way actions/create-github-app-token defaults to the current // repository. The token covers that repository alone. // An --owner given empty, as from an unset script variable, is not // nothing: falling back would mint for a repository the script never named. - if !isSet(fs, "owner") && !isSet(fs, "installation") && !isSet(fs, "repos") && os.Getenv("GITHUB_APP_INSTALLATION_ID") == "" { - if o, r, ok := originRepo(app.host); ok { - *owner, names = o, []string{r} + if !isSet(f.fs, "owner") && !isSet(f.fs, "installation") && !isSet(f.fs, "repos") && os.Getenv("GITHUB_APP_INSTALLATION_ID") == "" { + if o, r, ok := originRepo(f.host); ok { + owner, names = o, []string{r} } } switch { - case *owner != "" && isSet(fs, "installation"): - return usagef("set --installation or --owner, not both") - case strings.Contains(*owner, "/"): - return usagef("--owner %q is a repository: pass its account as --owner and the repository as --repos", *owner) - case *owner == "." || *owner == "..": - return usagef("--owner %q is not an account: pass a user or organization login", *owner) - case *owner == "" && *installation <= 0: - if err := badEnvNumber(fs, "installation", "GITHUB_APP_INSTALLATION_ID"); err != nil { - return err + case owner != "" && isSet(f.fs, "installation"): + return tokenRequest{}, usagef("set --installation or --owner, not both") + case strings.Contains(owner, "/"): + return tokenRequest{}, usagef("--owner %q is a repository: pass its account as --owner and the repository as --repos", owner) + case owner == "." || owner == "..": + return tokenRequest{}, usagef("--owner %q is not an account: pass a user or organization login", owner) + case owner == "" && f.installation <= 0: + if err := badEnvNumber(f.fs, "installation", "GITHUB_APP_INSTALLATION_ID"); err != nil { + return tokenRequest{}, err } - return usagef("missing installation ID: set --installation, --owner to look it up, or run inside a clone of a repository the App is installed on") + return tokenRequest{}, usagef("missing installation ID: set --installation, --owner to look it up, or run inside a clone of a repository the App is installed on") } - perms, err := parsePermissions(*permissions) + perms, err := parsePermissions(f.permissions) if err != nil { - return err + return tokenRequest{}, err } - if isSet(fs, "permissions") && perms == nil { - return usagef("--permissions names no permission; omit it for a token with every permission the App has") - } - - appSrc, err := app.source(stdin) - if err != nil { - return err + if isSet(f.fs, "permissions") && perms == nil { + return tokenRequest{}, usagef("--permissions names no permission; omit it for a token with every permission the App has") } + return tokenRequest{installation: f.installation, owner: owner, repos: names, permissions: perms}, nil +} - var urlOpts []githubauth.InstallationTokenSourceOpt +// urlOptions turns --base-url or --enterprise-url, at most one of which is +// set, into the options every request to that instance needs. +func (f *tokenFlags) urlOptions() []githubauth.InstallationTokenSourceOpt { + var opts []githubauth.InstallationTokenSourceOpt switch { - case *baseURL != "": - urlOpts = append(urlOpts, githubauth.WithBaseURL(*baseURL)) - case *enterpriseURL != "": - urlOpts = append(urlOpts, githubauth.WithEnterpriseURL(*enterpriseURL)) + case f.baseURL != "": + opts = append(opts, githubauth.WithBaseURL(f.baseURL)) + case f.enterpriseURL != "": + opts = append(opts, githubauth.WithEnterpriseURL(f.enterpriseURL)) } + return opts +} - id := *installation - if *owner != "" { - if id, err = githubauth.FindInstallationID(context.Background(), appSrc, *owner, urlOpts...); err != nil { - return err +// mint looks the installation up when only its owner is known, then mints a +// token scoped as req asks. +func mint(appSrc oauth2.TokenSource, req tokenRequest, urlOpts []githubauth.InstallationTokenSourceOpt) (*oauth2.Token, error) { + id := req.installation + if req.owner != "" { + var err error + if id, err = githubauth.FindInstallationID(context.Background(), appSrc, req.owner, urlOpts...); err != nil { + return nil, err } } - opts := slices.Clone(urlOpts) - if len(names) > 0 || perms != nil { + if len(req.repos) > 0 || req.permissions != nil { opts = append(opts, githubauth.WithInstallationTokenOptions(&githubauth.InstallationTokenOptions{ - Repositories: names, - Permissions: perms, + Repositories: req.repos, + Permissions: req.permissions, })) } - - tok, err := githubauth.NewInstallationTokenSource(id, appSrc, opts...).Token() - if err != nil { - return err - } - if app.exec && !*keep { - defer holdSignals()() - } - err = app.emit(tok, fs.Args(), stdin, stdout, stderr) - // Even a command that failed to start leaves a minted token behind. - if app.exec && !*keep { - err = revoke(err, tok, urlOpts, stderr) - } - return err + return githubauth.NewInstallationTokenSource(id, appSrc, opts...).Token() } func jwtCmd(args []string, stdin io.Reader, stdout, stderr io.Writer, out *output) error { @@ -608,7 +653,7 @@ func readKey(v string, stdin io.Reader) ([]byte, error) { return pem, nil } -func write(w io.Writer, tok *oauth2.Token, asJSON bool) error { +func writeToken(w io.Writer, tok *oauth2.Token, asJSON bool) error { if !asJSON { _, err := fmt.Fprintln(w, tok.AccessToken) return err @@ -625,7 +670,8 @@ func write(w io.Writer, tok *oauth2.Token, asJSON bool) error { }{tok.AccessToken, tok.Expiry, perms, repos}) } -func splitRepos(v string) []string { +// splitList splits a comma-separated flag value, dropping empty entries. +func splitList(v string) []string { var out []string for name := range strings.SplitSeq(v, ",") { if name = strings.TrimSpace(name); name != "" { @@ -697,8 +743,11 @@ func parseRemote(remote string) (host, owner, repo string, ok bool) { host, path = u.Hostname(), u.Path // An http(s) remote is served by the instance itself, so it carries its // port, default included; see originRepo. - if u.Scheme == "http" || u.Scheme == "https" { - host = net.JoinHostPort(host, cmp.Or(u.Port(), map[string]string{"http": "80", "https": "443"}[u.Scheme])) + switch u.Scheme { + case "http": + host = net.JoinHostPort(host, cmp.Or(u.Port(), "80")) + case "https": + host = net.JoinHostPort(host, cmp.Or(u.Port(), "443")) } } else if userHost, p, found := strings.Cut(remote, ":"); found && !strings.Contains(userHost, "/") { _, host, _ = strings.Cut(userHost, "@") @@ -725,7 +774,7 @@ func parseRemote(remote string) (host, owner, repo string, ok bool) { // does not know with a 422, even beside valid ones, rather than ignoring it and // minting a token with every permission. func parsePermissions(v string) (*githubauth.InstallationPermissions, error) { - pairs := splitRepos(v) + pairs := splitList(v) if len(pairs) == 0 { return nil, nil } diff --git a/cmd/githubauth/main_test.go b/cmd/githubauth/main_test.go index aa46a33..03ce3b9 100644 --- a/cmd/githubauth/main_test.go +++ b/cmd/githubauth/main_test.go @@ -18,6 +18,7 @@ import ( "os" "os/exec" "path/filepath" + "reflect" "runtime" "strings" "sync" @@ -112,71 +113,66 @@ func statusHandler(code int) func() *httptest.Server { } // Stdout matchers, so one table can assert on a fixed token, a signed JWT -// whose bytes differ every run, and a JSON envelope. - -func isEmpty(t *testing.T, got string) { - t.Helper() +// whose bytes differ every run, and a JSON envelope. Each returns what is +// wrong, for TestRun to report with the invocation that produced it. +func isEmpty(got string) error { if got != "" { - t.Errorf("stdout = %q, want it empty", got) + return fmt.Errorf("stdout = %q, want it empty", got) } + return nil } -func isNotEmpty(t *testing.T, got string) { - t.Helper() - +func isNotEmpty(got string) error { if got == "" { - t.Error("stdout is empty, want output") + return errors.New("stdout is empty, want output") } + return nil } -func isExactly(want string) func(*testing.T, string) { - return func(t *testing.T, got string) { - t.Helper() - +func isExactly(want string) func(string) error { + return func(got string) error { if got != want { - t.Errorf("stdout = %q, want %q", got, want) + return fmt.Errorf("stdout = %q, want %q", got, want) } + return nil } } -func contains(want string) func(*testing.T, string) { - return func(t *testing.T, got string) { - t.Helper() - +func contains(want string) func(string) error { + return func(got string) error { if !strings.Contains(got, want) { - t.Errorf("stdout = %q, want it to contain %q", got, want) + return fmt.Errorf("stdout = %q, want it to contain %q", got, want) } + return nil } } -func isJWT(t *testing.T, got string) { - t.Helper() - +func isJWT(got string) error { if n := len(strings.Split(got, ".")); n != 3 { - t.Errorf("stdout has %d JWT segments, want 3", n) + return fmt.Errorf("stdout has %d JWT segments, want 3", n) } + return nil } // isJSONToken checks the --json envelope: the token, and an expiry that is in // the future but no further out than max. -func isJSONToken(want string, max time.Duration) func(*testing.T, string) { - return func(t *testing.T, got string) { - t.Helper() - +func isJSONToken(want string, max time.Duration) func(string) error { + return func(got string) error { var out struct { Token string `json:"token"` ExpiresAt time.Time `json:"expires_at"` } if err := json.Unmarshal([]byte(got), &out); err != nil { - t.Fatalf("decoding %q: %v", got, err) + return fmt.Errorf("decoding stdout %q: %w", got, err) } if want != "" && out.Token != want { - t.Errorf("token = %q, want %q", out.Token, want) + return fmt.Errorf("token = %q, want %q", out.Token, want) } if d := time.Until(out.ExpiresAt); d <= 0 || d > max { - t.Errorf("expires in %v, want a positive duration under %v", d, max) + return fmt.Errorf("expires in %v, want a positive duration under %v", d, max) } + return nil } } @@ -192,7 +188,7 @@ func TestRun(t *testing.T) { env map[string]string setupServer func() *httptest.Server wantCode int - wantStdout func(*testing.T, string) + wantStdout func(string) error wantStderr string quietStderr bool }{ @@ -898,12 +894,15 @@ func TestRun(t *testing.T) { var stdout, stderr bytes.Buffer - got := run(tt.args(serverURL), strings.NewReader(tt.stdin), &stdout, &stderr) + args := tt.args(serverURL) + got := run(args, strings.NewReader(tt.stdin), &stdout, &stderr) if got != tt.wantCode { - t.Errorf("run() = %d, want %d (stderr: %s)", got, tt.wantCode, stderr.String()) + t.Errorf("run(%q) = %d, want %d (stderr: %s)", args, got, tt.wantCode, stderr.String()) } if tt.wantStdout != nil { - tt.wantStdout(t, strings.TrimSpace(stdout.String())) + if err := tt.wantStdout(strings.TrimSpace(stdout.String())); err != nil { + t.Errorf("run(%q): %v", args, err) + } } if tt.wantStderr != "" && !strings.Contains(stderr.String(), tt.wantStderr) { t.Errorf("stderr = %q, want it to contain %q", stderr.String(), tt.wantStderr) @@ -1118,10 +1117,10 @@ func TestTokenRequests(t *testing.T) { if tt.wantMint != "" && mint != tt.wantMint { t.Errorf("minted at %s, want %s", mint, tt.wantMint) } - if !strings.Contains(stdout.String(), tt.wantStdout) { + if tt.wantStdout != "" && !strings.Contains(stdout.String(), tt.wantStdout) { t.Errorf("stdout = %q, want it to contain %q", stdout.String(), tt.wantStdout) } - if tt.wantBody != "" && body != tt.wantBody { + if tt.wantBody != "" && !sameJSON(body, tt.wantBody) { t.Errorf("request body = %s, want %s", body, tt.wantBody) } if revoked := revokedWith != ""; revoked != tt.wantRevoke { @@ -1270,7 +1269,7 @@ func TestReadKey(t *testing.T) { } } -func TestSplitRepos(t *testing.T) { +func TestSplitList(t *testing.T) { t.Parallel() tests := []struct { @@ -1289,13 +1288,13 @@ func TestSplitRepos(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() - got := splitRepos(tt.in) + got := splitList(tt.in) if len(got) != len(tt.want) { - t.Fatalf("splitRepos(%q) = %v, want %v", tt.in, got, tt.want) + t.Fatalf("splitList(%q) = %v, want %v", tt.in, got, tt.want) } for i := range got { if got[i] != tt.want[i] { - t.Errorf("splitRepos(%q)[%d] = %q, want %q", tt.in, i, got[i], tt.want[i]) + t.Errorf("splitList(%q)[%d] = %q, want %q", tt.in, i, got[i], tt.want[i]) } } }) @@ -1383,7 +1382,7 @@ type errWriter struct{} func (errWriter) Write([]byte) (int, error) { return 0, errors.New("pipe closed") } -func TestWrite(t *testing.T) { +func TestWriteToken(t *testing.T) { t.Parallel() tok := &oauth2.Token{AccessToken: "ghs_x", Expiry: time.Now().Add(time.Hour)} @@ -1411,18 +1410,18 @@ func TestWrite(t *testing.T) { w = &buf } - err := write(w, tok, tt.asJSON) + err := writeToken(w, tok, tt.asJSON) if tt.wantErr { if err == nil { - t.Fatal("write() = nil, want the writer's error") + t.Fatal("writeToken() = nil, want the writer's error") } return } if err != nil { - t.Fatalf("write() error = %v", err) + t.Fatalf("writeToken() error = %v", err) } if !strings.Contains(buf.String(), tt.want) { - t.Errorf("write() wrote %q, want it to contain %q", buf.String(), tt.want) + t.Errorf("writeToken() wrote %q, want it to contain %q", buf.String(), tt.want) } }) } @@ -1560,7 +1559,12 @@ func TestOriginRepoKeepsTheKeyFromGit(t *testing.T) { if owner, repo, ok := originRepo(""); !ok || owner != "acme" || repo != "api" { t.Fatalf("originRepo() = %q, %q, %v, want acme, api, true", owner, repo, ok) } - if got, _ := os.ReadFile(seen); len(got) != 0 { + // The fake git always writes the file, so a missing one means it never ran. + got, err := os.ReadFile(seen) + if err != nil { + t.Fatalf("reading what git saw: %v", err) + } + if len(got) != 0 { t.Errorf("git saw $GITHUB_APP_PRIVATE_KEY = %q, want it empty", got) } } @@ -1639,3 +1643,10 @@ func TestErrorsKeepTheirCause(t *testing.T) { t.Errorf("runWithToken(missing command) error = %v, want it to wrap exec.ErrNotFound", err) } } + +// sameJSON reports whether a and b are the same JSON value: key order and +// spacing are not what a request body promises. +func sameJSON(a, b string) bool { + var av, bv any + return json.Unmarshal([]byte(a), &av) == nil && json.Unmarshal([]byte(b), &bv) == nil && reflect.DeepEqual(av, bv) +} diff --git a/github.go b/github.go index 322256c..7af05a1 100644 --- a/github.go +++ b/github.go @@ -172,7 +172,8 @@ type InstallationPermissions struct { Other map[string]string `json:"-"` } -// MarshalJSON sends Other alongside the fields. +// MarshalJSON sends Other alongside the fields. The value receiver lets a +// non-pointer InstallationPermissions marshal Other too. func (p InstallationPermissions) MarshalJSON() ([]byte, error) { type fields InstallationPermissions // Only *string fields, which always marshal. @@ -257,6 +258,9 @@ type githubClient struct { baseURL *url.URL httpClient *http.Client retryOnThrottle bool + // sleep waits out a throttle before the retry. It is sleepCtx; tests + // record the wait instead of spending it. + sleep func(context.Context, time.Duration) error } // newGitHubClient creates a new GitHub API client. @@ -267,14 +271,15 @@ func newGitHubClient(httpClient *http.Client) *githubClient { baseURL: baseURL, httpClient: httpClient, retryOnThrottle: true, + sleep: sleepCtx, } } // withEnterpriseURL sets the base URL for GitHub Enterprise Server. -func (c *githubClient) withEnterpriseURL(baseURL string) (*githubClient, error) { +func (c *githubClient) withEnterpriseURL(baseURL string) error { base, err := url.Parse(baseURL) if err != nil { - return nil, fmt.Errorf("failed to parse base URL: %w", err) + return fmt.Errorf("failed to parse base URL: %w", err) } if !strings.HasSuffix(base.Path, "/") { @@ -288,18 +293,17 @@ func (c *githubClient) withEnterpriseURL(baseURL string) (*githubClient, error) } c.baseURL = base - - return c, nil + return nil } // withBaseURL sets the API base URL verbatim, applying none of the GitHub // Enterprise Server path rewriting that withEnterpriseURL performs. Only a // trailing slash is appended when missing so that endpoint resolution via // baseURL.Parse works correctly. -func (c *githubClient) withBaseURL(baseURL string) (*githubClient, error) { +func (c *githubClient) withBaseURL(baseURL string) error { base, err := url.Parse(baseURL) if err != nil { - return nil, fmt.Errorf("failed to parse base URL: %w", err) + return fmt.Errorf("failed to parse base URL: %w", err) } if !strings.HasSuffix(base.Path, "/") { @@ -307,8 +311,7 @@ func (c *githubClient) withBaseURL(baseURL string) (*githubClient, error) { } c.baseURL = base - - return c, nil + return nil } // createInstallationToken creates an installation access token for a GitHub App. @@ -345,7 +348,7 @@ func (c *githubClient) createInstallationToken(ctx context.Context, installation } // Both are wrapped so a cancelled wait still reads as rate limited. - if sleepErr := sleepCtx(ctx, throttled.RetryAfter); sleepErr != nil { + if sleepErr := c.sleep(ctx, throttled.RetryAfter); sleepErr != nil { return nil, fmt.Errorf("%w: %w", sleepErr, err) } diff --git a/github_server_test.go b/github_server_test.go deleted file mode 100644 index b628f88..0000000 --- a/github_server_test.go +++ /dev/null @@ -1,108 +0,0 @@ -// Package githubauth provides utilities for GitHub authentication, -// including generating and using GitHub App tokens and installation tokens. -// -// This file contains local mock implementations for testing purposes. -package githubauth - -import ( - "encoding/json" - "net/http" - "net/http/httptest" - "strings" -) - -// mockedHTTPClient represents a mock HTTP client for testing GitHub API interactions. -type mockedHTTPClient struct { - server *httptest.Server - handlers map[string]http.HandlerFunc -} - -// mockOption is a functional option for configuring mockedHTTPClient. -type mockOption func(*mockedHTTPClient) - -// withRequestMatch configures the mock to return a specific response for a given endpoint pattern. -func withRequestMatch(endpoint string, response any) mockOption { - return func(m *mockedHTTPClient) { - m.handlers[endpoint] = func(w http.ResponseWriter, _ *http.Request) { - w.Header().Set("Content-Type", "application/json") - _ = json.NewEncoder(w).Encode(response) - } - } -} - -// withRequestMatchHandler configures the mock to use a custom handler for a given endpoint pattern. -func withRequestMatchHandler(endpoint string, handler http.HandlerFunc) mockOption { - return func(m *mockedHTTPClient) { - m.handlers[endpoint] = handler - } -} - -// newMockedHTTPClient creates a new mock HTTP client with the provided options. -// Returns the HTTP client and a cleanup function that should be called to close the test server. -func newMockedHTTPClient(opts ...mockOption) (*http.Client, func()) { - m := &mockedHTTPClient{ - handlers: make(map[string]http.HandlerFunc), - } - - for _, opt := range opts { - opt(m) - } - - m.server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - key := r.Method + " " + r.URL.Path - - if handler, exists := m.handlers[key]; exists { - handler(w, r) - return - } - - for pattern, handler := range m.handlers { - if matchesPattern(key, pattern) { - handler(w, r) - return - } - } - - w.WriteHeader(http.StatusNotFound) - _, _ = w.Write([]byte(`{"message":"Not Found"}`)) - })) - - client := &http.Client{ - Transport: &mockTransport{ - server: m.server, - }, - } - - return client, m.server.Close -} - -// mockTransport implements http.RoundTripper to redirect requests to our mock server. -type mockTransport struct { - server *httptest.Server -} - -func (t *mockTransport) RoundTrip(req *http.Request) (*http.Response, error) { - req.URL.Scheme = "http" - req.URL.Host = t.server.URL[7:] - return http.DefaultTransport.RoundTrip(req) -} - -// matchesPattern performs simple pattern matching for GitHub API endpoints. -func matchesPattern(request, pattern string) bool { - // Handle the specific case used in tests: POST /app/installations/{installation_id}/access_tokens. - // Tolerate an Enterprise base-path prefix (e.g. /api/v3/) so requests routed - // through WithEnterpriseURL still match regardless of option ordering. - if pattern == "POST /app/installations/{installation_id}/access_tokens" { - return strings.HasPrefix(request, "POST ") && - strings.Contains(request, "/app/installations/") && - strings.HasSuffix(request, "/access_tokens") - } - - // For other patterns, use exact matching - return request == pattern -} - -// Common GitHub API endpoint patterns used in tests -const ( - postAppInstallationsAccessTokensByInstallationID = "POST /app/installations/{installation_id}/access_tokens" -) diff --git a/github_test.go b/github_test.go index 62c46de..df242fb 100644 --- a/github_test.go +++ b/github_test.go @@ -8,6 +8,8 @@ import ( "maps" "net/http" "net/http/httptest" + "reflect" + "slices" "strconv" "strings" "sync/atomic" @@ -19,84 +21,83 @@ import ( func Test_githubClient_withEnterpriseURL(t *testing.T) { tests := []struct { - name string - baseURL string - wantErr bool - expectedBaseURL string + name string + baseURL string + wantErr bool + wantBaseURL string }{ { - name: "valid URL with first subdomain", - baseURL: "https://api.github.example.com", - wantErr: false, - expectedBaseURL: "https://api.github.example.com/", + name: "valid URL with first subdomain", + baseURL: "https://api.github.example.com", + wantErr: false, + wantBaseURL: "https://api.github.example.com/", }, { - name: "valid URL with first subdomain and trailing slash", - baseURL: "https://api.github.example.com/", - wantErr: false, - expectedBaseURL: "https://api.github.example.com/", + name: "valid URL with first subdomain and trailing slash", + baseURL: "https://api.github.example.com/", + wantErr: false, + wantBaseURL: "https://api.github.example.com/", }, { - name: "valid URL with second subdomain", - baseURL: "https://ghes.api.example.com", - wantErr: false, - expectedBaseURL: "https://ghes.api.example.com/", + name: "valid URL with second subdomain", + baseURL: "https://ghes.api.example.com", + wantErr: false, + wantBaseURL: "https://ghes.api.example.com/", }, { - name: "valid URL with second subdomain and trailing slash", - baseURL: "https://ghes.api.example.com/", - wantErr: false, - expectedBaseURL: "https://ghes.api.example.com/", + name: "valid URL with second subdomain and trailing slash", + baseURL: "https://ghes.api.example.com/", + wantErr: false, + wantBaseURL: "https://ghes.api.example.com/", }, { - name: "valid URL with path", - baseURL: "https://github.example.com/api/v3", - wantErr: false, - expectedBaseURL: "https://github.example.com/api/v3/", + name: "valid URL with path", + baseURL: "https://github.example.com/api/v3", + wantErr: false, + wantBaseURL: "https://github.example.com/api/v3/", }, { - name: "valid URL with path and trailing slash", - baseURL: "https://github.example.com/api/v3/", - wantErr: false, - expectedBaseURL: "https://github.example.com/api/v3/", + name: "valid URL with path and trailing slash", + baseURL: "https://github.example.com/api/v3/", + wantErr: false, + wantBaseURL: "https://github.example.com/api/v3/", }, { - name: "valid URL without path", - baseURL: "https://github.example.com", - wantErr: false, - expectedBaseURL: "https://github.example.com/api/v3/", + name: "valid URL without path", + baseURL: "https://github.example.com", + wantErr: false, + wantBaseURL: "https://github.example.com/api/v3/", }, { - name: "valid URL without path but with trailing slash", - baseURL: "https://github.example.com/", - wantErr: false, - expectedBaseURL: "https://github.example.com/api/v3/", + name: "valid URL without path but with trailing slash", + baseURL: "https://github.example.com/", + wantErr: false, + wantBaseURL: "https://github.example.com/api/v3/", }, { - name: "invalid URL with control characters", - baseURL: "ht\ntp://invalid", - wantErr: true, - expectedBaseURL: "", + name: "invalid URL with control characters", + baseURL: "ht\ntp://invalid", + wantErr: true, + wantBaseURL: "", }, { - name: "URL with spaces", - baseURL: "http://invalid url with spaces", - wantErr: true, - expectedBaseURL: "", + name: "URL with spaces", + baseURL: "http://invalid url with spaces", + wantErr: true, + wantBaseURL: "", }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { client := newGitHubClient(&http.Client{}) - githubClient, err := client.withEnterpriseURL(tt.baseURL) + err := client.withEnterpriseURL(tt.baseURL) if (err != nil) != tt.wantErr { - t.Errorf("withEnterpriseURL(%v) error = %v", tt.baseURL, err) + t.Errorf("withEnterpriseURL(%q) error = %v, wantErr %v", tt.baseURL, err, tt.wantErr) } - - if err == nil && githubClient.baseURL.String() != tt.expectedBaseURL { - t.Errorf("withEnterpriseURL(%v) expected = %v, received = %v", tt.baseURL, tt.expectedBaseURL, githubClient.baseURL) + if err == nil && client.baseURL.String() != tt.wantBaseURL { + t.Errorf("withEnterpriseURL(%q).baseURL = %q, want %q", tt.baseURL, client.baseURL, tt.wantBaseURL) } }) } @@ -104,66 +105,65 @@ func Test_githubClient_withEnterpriseURL(t *testing.T) { func Test_githubClient_withBaseURL(t *testing.T) { tests := []struct { - name string - baseURL string - wantErr bool - expectedBaseURL string + name string + baseURL string + wantErr bool + wantBaseURL string }{ { - name: "GHEC data residency URL is used verbatim", - baseURL: "https://api.octocorp.ghe.com", - wantErr: false, - expectedBaseURL: "https://api.octocorp.ghe.com/", + name: "GHEC data residency URL is used verbatim", + baseURL: "https://api.octocorp.ghe.com", + wantErr: false, + wantBaseURL: "https://api.octocorp.ghe.com/", }, { - name: "GHEC data residency URL with trailing slash", - baseURL: "https://api.octocorp.ghe.com/", - wantErr: false, - expectedBaseURL: "https://api.octocorp.ghe.com/", + name: "GHEC data residency URL with trailing slash", + baseURL: "https://api.octocorp.ghe.com/", + wantErr: false, + wantBaseURL: "https://api.octocorp.ghe.com/", }, { - name: "plain host without /api/v3 suffix appended", - baseURL: "https://github.example.com", - wantErr: false, - expectedBaseURL: "https://github.example.com/", + name: "plain host without /api/v3 suffix appended", + baseURL: "https://github.example.com", + wantErr: false, + wantBaseURL: "https://github.example.com/", }, { - name: "existing path is preserved without /api/v3 munging", - baseURL: "https://github.example.com/api/v3", - wantErr: false, - expectedBaseURL: "https://github.example.com/api/v3/", + name: "existing path is preserved without /api/v3 munging", + baseURL: "https://github.example.com/api/v3", + wantErr: false, + wantBaseURL: "https://github.example.com/api/v3/", }, { - name: "httptest-style host and port used verbatim", - baseURL: "http://127.0.0.1:8080", - wantErr: false, - expectedBaseURL: "http://127.0.0.1:8080/", + name: "httptest-style host and port used verbatim", + baseURL: "http://127.0.0.1:8080", + wantErr: false, + wantBaseURL: "http://127.0.0.1:8080/", }, { - name: "invalid URL with control characters", - baseURL: "ht\ntp://invalid", - wantErr: true, - expectedBaseURL: "", + name: "invalid URL with control characters", + baseURL: "ht\ntp://invalid", + wantErr: true, + wantBaseURL: "", }, { - name: "URL with spaces", - baseURL: "http://invalid url with spaces", - wantErr: true, - expectedBaseURL: "", + name: "URL with spaces", + baseURL: "http://invalid url with spaces", + wantErr: true, + wantBaseURL: "", }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { client := newGitHubClient(&http.Client{}) - githubClient, err := client.withBaseURL(tt.baseURL) + err := client.withBaseURL(tt.baseURL) if (err != nil) != tt.wantErr { - t.Errorf("withBaseURL(%v) error = %v", tt.baseURL, err) + t.Errorf("withBaseURL(%q) error = %v, wantErr %v", tt.baseURL, err, tt.wantErr) } - - if err == nil && githubClient.baseURL.String() != tt.expectedBaseURL { - t.Errorf("withBaseURL(%v) expected = %v, received = %v", tt.baseURL, tt.expectedBaseURL, githubClient.baseURL) + if err == nil && client.baseURL.String() != tt.wantBaseURL { + t.Errorf("withBaseURL(%q).baseURL = %q, want %q", tt.baseURL, client.baseURL, tt.wantBaseURL) } }) } @@ -178,7 +178,7 @@ func Test_githubClient_createInstallationToken_ErrorCases(t *testing.T) { errorSubstring string }{ { - name: "invalid JSON in options", + name: "options are marshaled into a successful request", setupServer: func() *httptest.Server { return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusCreated) @@ -299,16 +299,16 @@ 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) + t.Errorf("createInstallationToken() error = nil, want one containing %q", tt.errorSubstring) } else if !strings.Contains(err.Error(), tt.errorSubstring) { - t.Errorf("expected error containing %q, got %q", tt.errorSubstring, err.Error()) + t.Errorf("createInstallationToken() error = %q, want it to contain %q", err, tt.errorSubstring) } } }) } } -func Test_Ptr(t *testing.T) { +func TestPtr(t *testing.T) { t.Run("string pointer", func(t *testing.T) { s := "test" p := Ptr(s) @@ -446,7 +446,7 @@ func Test_createInstallationToken_EndpointURL(t *testing.T) { defer server.Close() c := newGitHubClient(&http.Client{}) - if _, err := c.withBaseURL(server.URL + tt.basePath); err != nil { + if err := c.withBaseURL(server.URL + tt.basePath); err != nil { t.Fatalf("withBaseURL(%q): %v", tt.basePath, err) } @@ -537,8 +537,9 @@ func Test_createInstallationToken_SendsOptionsBody(t *testing.T) { select { case got := <-gotBody: - if got != tt.wantBody { - t.Errorf("request body = %q, want %q", got, tt.wantBody) + // Compared as JSON: key order and spacing are not the contract. + if !sameJSON(got, tt.wantBody) { + t.Errorf("request body = %s, want %s", got, tt.wantBody) } default: t.Fatal("server received no request") @@ -556,8 +557,9 @@ type throttleResponse struct { writeToken bool // serialize an InstallationToken JSON body instead of body } -func throttleHandler(t *testing.T, responses []throttleResponse, attempts *atomic.Int32) http.HandlerFunc { - t.Helper() +// throttleHandler answers each request with the next of responses, starting +// over once they run out, and counts the requests in attempts. +func throttleHandler(responses []throttleResponse, attempts *atomic.Int32) http.HandlerFunc { return func(w http.ResponseWriter, _ *http.Request) { n := attempts.Add(1) idx := int(n-1) % len(responses) @@ -584,14 +586,23 @@ func throttleHandler(t *testing.T, responses []throttleResponse, attempts *atomi func newClientForServer(t *testing.T, server *httptest.Server) *githubClient { t.Helper() c := newGitHubClient(&http.Client{Timeout: 10 * time.Second}) - baseURL, err := c.baseURL.Parse(server.URL + "/") - if err != nil { - t.Fatalf("parse server URL: %v", err) + if err := c.withBaseURL(server.URL); err != nil { + t.Fatalf("withBaseURL(%q) error = %v", server.URL, err) } - c.baseURL = baseURL return c } +// recordSleeps makes c note each throttle wait instead of sleeping through it, +// so a test asserts the wait it asked for rather than the time that passed. +func recordSleeps(c *githubClient) *[]time.Duration { + var waits []time.Duration + c.sleep = func(_ context.Context, d time.Duration) error { + waits = append(waits, d) + return nil + } + return &waits +} + func Test_createInstallationToken_RetryOnThrottle(t *testing.T) { tests := []struct { name string @@ -600,8 +611,7 @@ func Test_createInstallationToken_RetryOnThrottle(t *testing.T) { wantAttempts int32 wantErr bool wantRateLimit bool - minElapsed time.Duration - maxElapsed time.Duration + wantWaits []time.Duration }{ { name: "429 with Retry-After=1 retries and succeeds", @@ -612,8 +622,7 @@ func Test_createInstallationToken_RetryOnThrottle(t *testing.T) { retryEnabled: true, wantAttempts: 2, wantErr: false, - minElapsed: 900 * time.Millisecond, - maxElapsed: 3 * time.Second, + wantWaits: []time.Duration{time.Second}, }, { name: "429 without Retry-After uses default 1s backoff", @@ -624,8 +633,7 @@ func Test_createInstallationToken_RetryOnThrottle(t *testing.T) { retryEnabled: true, wantAttempts: 2, wantErr: false, - minElapsed: 900 * time.Millisecond, - maxElapsed: 3 * time.Second, + wantWaits: []time.Duration{time.Second}, }, { name: "two consecutive 429s retry once then fail", @@ -637,6 +645,7 @@ func Test_createInstallationToken_RetryOnThrottle(t *testing.T) { wantAttempts: 2, wantErr: true, wantRateLimit: true, + wantWaits: []time.Duration{0}, }, { name: "WithRetryOnThrottle(false) disables retry", @@ -672,15 +681,14 @@ func Test_createInstallationToken_RetryOnThrottle(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { var attempts atomic.Int32 - server := httptest.NewServer(throttleHandler(t, tt.responses, &attempts)) + server := httptest.NewServer(throttleHandler(tt.responses, &attempts)) defer server.Close() client := newClientForServer(t, server) client.retryOnThrottle = tt.retryEnabled + waits := recordSleeps(client) - 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) @@ -691,22 +699,17 @@ func Test_createInstallationToken_RetryOnThrottle(t *testing.T) { if tt.wantRateLimit && !errors.Is(err, ErrRateLimited) { t.Errorf("err = %v, want errors.Is(err, ErrRateLimited)", err) } - if tt.minElapsed > 0 && elapsed < tt.minElapsed { - t.Errorf("elapsed = %v, want >= %v", elapsed, tt.minElapsed) - } - if tt.maxElapsed > 0 && elapsed > tt.maxElapsed { - t.Errorf("elapsed = %v, want <= %v", elapsed, tt.maxElapsed) + if !slices.Equal(*waits, tt.wantWaits) { + t.Errorf("waits = %v, want %v", *waits, tt.wantWaits) } }) } } 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. x-ratelimit-remaining is 0 because that is - // what marks a 403 as throttled rather than terminal; see - // Test_createInstallationToken_403PermissionNotThrottled. + // x-ratelimit-reset is a Unix-second epoch, computed at request time. + // 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) @@ -727,10 +730,9 @@ func Test_createInstallationToken_XRateLimitReset(t *testing.T) { defer server.Close() client := newClientForServer(t, server) + waits := recordSleeps(client) - start := time.Now() tok, err := client.createInstallationToken(context.Background(), 12345, nil) - elapsed := time.Since(start) if err != nil { t.Fatalf("createInstallationToken err = %v", err) @@ -741,12 +743,9 @@ func Test_createInstallationToken_XRateLimitReset(t *testing.T) { if got := attempts.Load(); got != 2 { t.Errorf("attempts = %d, want 2", got) } - // Reset is now+3, Unix() can truncate up to 1s, so the floor is ~2s. - if elapsed < 1500*time.Millisecond { - t.Errorf("elapsed = %v, want >= 1.5s (reset hint was now+3s)", elapsed) - } - if elapsed > 5*time.Second { - t.Errorf("elapsed = %v, want <= 5s (bounded by reset hint)", elapsed) + // Reset is now+3s, and Unix() truncates up to 1s, so the wait is 2-3s. + if len(*waits) != 1 || (*waits)[0] < 1500*time.Millisecond || (*waits)[0] > 3*time.Second { + t.Errorf("waits = %v, want one wait of 2-3s, until the reset", *waits) } } @@ -795,7 +794,7 @@ func Test_throttleDelay_KeepsGitHubsFullWait(t *testing.T) { func Test_createInstallationToken_LongWaitIsNotSleptThrough(t *testing.T) { var attempts atomic.Int32 reset := strconv.FormatInt(time.Now().Add(40*time.Minute).Unix(), 10) - server := httptest.NewServer(throttleHandler(t, []throttleResponse{ + server := httptest.NewServer(throttleHandler([]throttleResponse{ { status: http.StatusForbidden, headers: map[string]string{"X-RateLimit-Remaining": "0", "X-RateLimit-Reset": reset}, @@ -804,8 +803,9 @@ func Test_createInstallationToken_LongWaitIsNotSleptThrough(t *testing.T) { }, &attempts)) defer server.Close() - start := time.Now() - _, err := newClientForServer(t, server).createInstallationToken(context.Background(), 12345, nil) + client := newClientForServer(t, server) + waits := recordSleeps(client) + _, err := client.createInstallationToken(context.Background(), 12345, nil) var rle *RateLimitError if !errors.As(err, &rle) { @@ -817,29 +817,29 @@ func Test_createInstallationToken_LongWaitIsNotSleptThrough(t *testing.T) { if got := attempts.Load(); got != 1 { t.Errorf("attempts = %d, want 1", got) } - if elapsed := time.Since(start); elapsed > time.Second { - t.Errorf("elapsed = %v, want an immediate return", elapsed) + if len(*waits) != 0 { + t.Errorf("waits = %v, want none: a wait past the cap is returned, not slept", *waits) } } func Test_createInstallationToken_ContextCancelledDuringSleep(t *testing.T) { var attempts atomic.Int32 - server := httptest.NewServer(throttleHandler(t, []throttleResponse{ + server := httptest.NewServer(throttleHandler([]throttleResponse{ {status: http.StatusTooManyRequests, headers: map[string]string{"Retry-After": "30"}, body: `{"message":"rate limited"}`}, }, &attempts)) defer server.Close() client := newClientForServer(t, server) + // Cancelled from inside the wait, so the cancellation always lands there + // rather than before or after it, as a timer would allow. ctx, cancel := context.WithCancel(context.Background()) - go func() { - time.Sleep(150 * time.Millisecond) + client.sleep = func(ctx context.Context, d time.Duration) error { cancel() - }() + return sleepCtx(ctx, d) + } - start := time.Now() _, err := client.createInstallationToken(ctx, 12345, nil) - elapsed := time.Since(start) if !errors.Is(err, context.Canceled) { t.Errorf("err = %v, want context.Canceled", err) @@ -847,9 +847,6 @@ func Test_createInstallationToken_ContextCancelledDuringSleep(t *testing.T) { if !errors.Is(err, ErrRateLimited) { t.Errorf("err = %v, want it to still wrap ErrRateLimited", err) } - if elapsed > 5*time.Second { - t.Errorf("elapsed = %v, expected ctx cancellation to abort sleep quickly", elapsed) - } if got := attempts.Load(); got != 1 { t.Errorf("attempts = %d, want 1 (no retry after cancellation)", got) } @@ -897,55 +894,45 @@ func Test_throttleDelay_NonThrottledStatus(t *testing.T) { } } -func Test_WithRetryOnThrottle_AffectsSource(t *testing.T) { - // Drive a full installationTokenSource.Token() through a throttled server - // to verify the option propagates from the source down to the client. - var attempts atomic.Int32 - server := httptest.NewServer(throttleHandler(t, []throttleResponse{ - {status: http.StatusTooManyRequests, headers: map[string]string{"Retry-After": "0"}, body: `{"message":"first"}`}, - {status: http.StatusCreated, writeToken: true}, - }, &attempts)) - defer server.Close() - - src := oauth2StaticSource{accessToken: "jwt"} - - ts := NewInstallationTokenSource(42, src, - WithEnterpriseURL(server.URL), - WithRetryOnThrottle(true), - ) - - tok, err := ts.Token() - if err != nil { - t.Fatalf("Token() err = %v", err) - } - if tok.AccessToken != "test-token" { - t.Errorf("AccessToken = %q, want %q", tok.AccessToken, "test-token") - } - if got := attempts.Load(); got != 2 { - t.Errorf("attempts = %d, want 2", got) +func TestWithRetryOnThrottle_AffectsSource(t *testing.T) { + // A full installationTokenSource.Token() through a throttled server shows + // the option reaches the client. + tests := []struct { + name string + retry bool + wantAttempts int32 + wantErr bool + }{ + {name: "retry on", retry: true, wantAttempts: 2}, + {name: "retry off", retry: false, wantAttempts: 1, wantErr: true}, } - var attempts2 atomic.Int32 - server2 := httptest.NewServer(throttleHandler(t, []throttleResponse{ - {status: http.StatusTooManyRequests, headers: map[string]string{"Retry-After": "0"}, body: `{"message":"first"}`}, - {status: http.StatusCreated, writeToken: true}, - }, &attempts2)) - defer server2.Close() + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var attempts atomic.Int32 + server := httptest.NewServer(throttleHandler([]throttleResponse{ + {status: http.StatusTooManyRequests, headers: map[string]string{"Retry-After": "0"}, body: `{"message":"first"}`}, + {status: http.StatusCreated, writeToken: true}, + }, &attempts)) + defer server.Close() - ts2 := NewInstallationTokenSource(42, src, - WithEnterpriseURL(server2.URL), - WithRetryOnThrottle(false), - ) + ts := NewInstallationTokenSource(42, oauth2StaticSource{accessToken: "jwt"}, + WithEnterpriseURL(server.URL), + WithRetryOnThrottle(tt.retry), + ) + tok, err := ts.Token() - _, err = ts2.Token() - if err == nil { - t.Fatalf("Token() err = nil, want ErrRateLimited") - } - if !errors.Is(err, ErrRateLimited) { - t.Errorf("err = %v, want errors.Is(err, ErrRateLimited)", err) - } - if got := attempts2.Load(); got != 1 { - t.Errorf("attempts = %d, want 1", got) + if tt.wantErr { + if !errors.Is(err, ErrRateLimited) { + t.Errorf("Token() error = %v, want errors.Is(err, ErrRateLimited)", err) + } + } else if err != nil || tok.AccessToken != "test-token" { + t.Errorf("Token() = %v, %v, want test-token", tok, err) + } + if got := attempts.Load(); got != tt.wantAttempts { + t.Errorf("attempts = %d, want %d", got, tt.wantAttempts) + } + }) } } @@ -1065,7 +1052,6 @@ func Test_createInstallationToken_403PermissionNotThrottled(t *testing.T) { wantAttempts int32 wantErr bool wantRateLimit bool - maxElapsed time.Duration }{ { name: "403 permission error with healthy remaining is terminal", @@ -1083,7 +1069,6 @@ func Test_createInstallationToken_403PermissionNotThrottled(t *testing.T) { wantAttempts: 1, wantErr: true, wantRateLimit: false, - maxElapsed: time.Second, }, { name: "403 with exhausted remaining is still throttled", @@ -1101,7 +1086,6 @@ func Test_createInstallationToken_403PermissionNotThrottled(t *testing.T) { }, wantAttempts: 2, wantErr: false, - maxElapsed: 5 * time.Second, }, { name: "403 with Retry-After is still throttled", @@ -1115,7 +1099,6 @@ func Test_createInstallationToken_403PermissionNotThrottled(t *testing.T) { }, wantAttempts: 2, wantErr: false, - maxElapsed: 5 * time.Second, }, { name: "429 without hint headers is still throttled", @@ -1125,21 +1108,19 @@ func Test_createInstallationToken_403PermissionNotThrottled(t *testing.T) { }, 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)) + server := httptest.NewServer(throttleHandler(tt.responses, &attempts)) defer server.Close() client := newClientForServer(t, server) + recordSleeps(client) - 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) @@ -1150,9 +1131,6 @@ func Test_createInstallationToken_403PermissionNotThrottled(t *testing.T) { 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) - } }) } } @@ -1241,7 +1219,7 @@ 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"}`)) + delay, retry := newGitHubClient(&http.Client{}).throttleDelay(resp, []byte(`{"message":"API rate limit exceeded"}`)) if !retry { t.Fatal("throttleDelay() retryable = false, want true") } @@ -1250,15 +1228,10 @@ func Test_throttleDelay_HintlessRateLimit403WaitsAMinute(t *testing.T) { } } -func c403(t *testing.T) *githubClient { - t.Helper() - return newGitHubClient(&http.Client{}) -} - -// Test_RateLimitError_CarriesRetryHint covers the reason the type exists: a +// TestRateLimitError_CarriesRetryHint covers the reason the type exists: a // caller running its own backoff can read the wait the client already computed // from GitHub's headers, instead of re-parsing the response it never sees. -func Test_RateLimitError_CarriesRetryHint(t *testing.T) { +func TestRateLimitError_CarriesRetryHint(t *testing.T) { tests := []struct { name string status int @@ -1330,9 +1303,9 @@ func Test_RateLimitError_CarriesRetryHint(t *testing.T) { } } -// Test_RateLimitError_MessageFormat pins the rendered message. Callers log and +// TestRateLimitError_MessageFormat pins the rendered message. Callers log and // match on it, so introducing the type must not reword it. -func Test_RateLimitError_MessageFormat(t *testing.T) { +func TestRateLimitError_MessageFormat(t *testing.T) { err := &RateLimitError{ StatusCode: http.StatusForbidden, RetryAfter: time.Second, @@ -1349,9 +1322,9 @@ func Test_RateLimitError_MessageFormat(t *testing.T) { } } -// Test_RateLimitError_NotReturnedForTerminalFailures guards the boundary: a +// TestRateLimitError_NotReturnedForTerminalFailures guards the boundary: a // permission failure or a plain 404 must not be surfaced as rate limiting. -func Test_RateLimitError_NotReturnedForTerminalFailures(t *testing.T) { +func TestRateLimitError_NotReturnedForTerminalFailures(t *testing.T) { tests := []struct { name string status int @@ -1410,9 +1383,9 @@ func Test_RateLimitError_NotReturnedForTerminalFailures(t *testing.T) { } } -// Test_APIError_MessageFormat pins the rendered message, which predates the +// TestAPIError_MessageFormat pins the rendered message, which predates the // type: it was a fmt.Errorf string callers already log and match on. -func Test_APIError_MessageFormat(t *testing.T) { +func TestAPIError_MessageFormat(t *testing.T) { err := &APIError{StatusCode: http.StatusNotFound, Message: `{"message":"Not Found"}`} want := `GitHub API returned status 404: {"message":"Not Found"}` @@ -1738,3 +1711,51 @@ func TestInstallationPermissions_MarshalJSONRefusesCaseVariant(t *testing.T) { t.Error("Marshal() err = nil, want the permission named twice refused") } } + +// sameJSON reports whether a and b are the same JSON value, or both empty. +func sameJSON(a, b string) bool { + if a == "" || b == "" { + return a == b + } + var av, bv any + return json.Unmarshal([]byte(a), &av) == nil && json.Unmarshal([]byte(b), &bv) == nil && reflect.DeepEqual(av, bv) +} + +func TestSleepCtx(t *testing.T) { + t.Parallel() + + // The retry tests record waits instead of spending them, so this is what + // shows the wait is real: it takes at least d, and ends early, mid-wait, + // when the context does. + tests := []struct { + name string + d time.Duration + cancelAfter time.Duration // 0: never cancelled + minElapsed time.Duration + wantErr error + }{ + {name: "no wait", d: 0}, + {name: "a wait lasts its duration", d: 20 * time.Millisecond, minElapsed: 20 * time.Millisecond}, + {name: "a context done mid-wait ends it", d: time.Hour, cancelAfter: 10 * time.Millisecond, wantErr: context.Canceled}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + if tt.cancelAfter > 0 { + time.AfterFunc(tt.cancelAfter, cancel) + } + + start := time.Now() + err := sleepCtx(ctx, tt.d) + if !errors.Is(err, tt.wantErr) { + t.Errorf("sleepCtx(%v) = %v, want %v", tt.d, err, tt.wantErr) + } + if elapsed := time.Since(start); elapsed < tt.minElapsed { + t.Errorf("sleepCtx(%v) returned after %v, want at least %v", tt.d, elapsed, tt.minElapsed) + } + }) + } +} diff --git a/http.go b/http.go index 9738816..38a8c04 100644 --- a/http.go +++ b/http.go @@ -1,7 +1,7 @@ package githubauth import ( - "errors" + "fmt" "net" "net/http" "runtime" @@ -36,14 +36,18 @@ func cleanHTTPClient() *http.Client { } } +// maxRedirects is how many redirects a request follows, as http.Client does by +// default. +const maxRedirects = 10 + // sameHostRedirect refuses a redirect that leaves the original host or drops // TLS. oauth2.Transport sets Authorization on every hop, which defeats the // header stripping http.Client does across hosts, so following one would hand // the App JWT to whoever the server points at. The refused 3xx is returned as // the response. func sameHostRedirect(req *http.Request, via []*http.Request) error { - if len(via) >= 10 { - return errors.New("stopped after 10 redirects") + if len(via) >= maxRedirects { + return fmt.Errorf("stopped after %d redirects", maxRedirects) } orig := via[0].URL if req.URL.Host != orig.Host || (orig.Scheme == "https" && req.URL.Scheme != "https") { diff --git a/http_test.go b/http_test.go index 8dbb4bd..96588f8 100644 --- a/http_test.go +++ b/http_test.go @@ -2,7 +2,6 @@ package githubauth import ( "errors" - "fmt" "net/http" "net/http/httptest" "testing" @@ -119,11 +118,16 @@ func TestSameHostRedirect(t *testing.T) { from string to string hops int - want error + // want is a sentinel matched with errors.Is; wantMsg an error of + // this package's own, matched by its message. + want error + wantMsg string }{ - {name: "same host", from: "https://ghe.example.com/a", to: "https://ghe.example.com/b", want: nil}, - {name: "a tenth hop", from: "https://ghe.example.com/a", to: "https://ghe.example.com/b", hops: 10, want: errors.New("stopped after 10 redirects")}, - {name: "same host upgraded to TLS", from: "http://ghe.example.com/a", to: "https://ghe.example.com/a", want: nil}, + {name: "same host", from: "https://ghe.example.com/a", to: "https://ghe.example.com/b"}, + {name: "a tenth hop", from: "https://ghe.example.com/a", to: "https://ghe.example.com/b", hops: 10, wantMsg: "stopped after 10 redirects"}, + {name: "same host upgraded to TLS", from: "http://ghe.example.com/a", to: "https://ghe.example.com/a"}, + // net/http stops on this sentinel itself, so an error that merely + // reads the same would not do. {name: "another host", from: "https://ghe.example.com/a", to: "https://evil.example.com/a", want: http.ErrUseLastResponse}, {name: "TLS dropped", from: "https://ghe.example.com/a", to: "http://ghe.example.com/a", want: http.ErrUseLastResponse}, } @@ -136,7 +140,13 @@ func TestSameHostRedirect(t *testing.T) { for len(via) < tt.hops { via = append(via, from) } - if got := sameHostRedirect(to, via); fmt.Sprint(got) != fmt.Sprint(tt.want) { + got := sameHostRedirect(to, via) + switch { + case tt.wantMsg != "": + if got == nil || got.Error() != tt.wantMsg { + t.Errorf("sameHostRedirect() = %v, want %q", got, tt.wantMsg) + } + case !errors.Is(got, tt.want) || (tt.want == nil && got != nil): t.Errorf("sameHostRedirect() = %v, want %v", got, tt.want) } }) diff --git a/webhook/webhook.go b/webhook/webhook.go index ff942cd..08b4125 100644 --- a/webhook/webhook.go +++ b/webhook/webhook.go @@ -31,7 +31,7 @@ const ( ) // DefaultMaxPayloadSize matches GitHub's documented 25 MiB delivery cap. -const DefaultMaxPayloadSize int64 = 25 * 1024 * 1024 +const DefaultMaxPayloadSize int64 = 25 << 20 const signaturePrefix = "sha256=" diff --git a/webhook/webhook_test.go b/webhook/webhook_test.go index a7e2f12..95cb852 100644 --- a/webhook/webhook_test.go +++ b/webhook/webhook_test.go @@ -64,7 +64,7 @@ func TestMiddleware_Valid(t *testing.T) { Middleware(secret)(next).ServeHTTP(rec, req) if rec.Code != http.StatusNoContent { - t.Fatalf("status = %d, want %d", rec.Code, http.StatusNoContent) + t.Errorf("status = %d, want %d", rec.Code, http.StatusNoContent) } if !bytes.Equal(gotBody, body) { t.Fatalf("body restored incorrectly: got %q, want %q", gotBody, body) @@ -88,7 +88,7 @@ func TestMiddleware_InvalidSignature(t *testing.T) { t.Fatal("next handler was called on invalid signature") } if rec.Code != http.StatusUnauthorized { - t.Fatalf("status = %d, want %d", rec.Code, http.StatusUnauthorized) + t.Errorf("status = %d, want %d", rec.Code, http.StatusUnauthorized) } } @@ -108,7 +108,7 @@ func TestMiddleware_MissingSignature(t *testing.T) { t.Fatal("next handler was called without a signature") } if rec.Code != http.StatusUnauthorized { - t.Fatalf("status = %d, want %d", rec.Code, http.StatusUnauthorized) + t.Errorf("status = %d, want %d", rec.Code, http.StatusUnauthorized) } } @@ -129,7 +129,7 @@ func TestMiddleware_MaxPayloadSize(t *testing.T) { t.Fatal("next handler was called on oversized body") } if rec.Code != http.StatusRequestEntityTooLarge { - t.Fatalf("status = %d, want %d", rec.Code, http.StatusRequestEntityTooLarge) + t.Errorf("status = %d, want %d", rec.Code, http.StatusRequestEntityTooLarge) } } @@ -191,7 +191,7 @@ func TestMiddleware_CustomErrorHandler(t *testing.T) { handler.ServeHTTP(rec, req) if rec.Code != http.StatusTeapot { - t.Fatalf("status = %d, want %d", rec.Code, http.StatusTeapot) + t.Errorf("status = %d, want %d", rec.Code, http.StatusTeapot) } if !errors.Is(captured, ErrSignatureMismatch) { t.Fatalf("captured err = %v, want errors.Is(ErrSignatureMismatch)", captured)