From a5953e278c74dde41d5e1a30ce4bc8c9973c59b5 Mon Sep 17 00:00:00 2001 From: jferrl Date: Sat, 31 Oct 2026 11:33:08 +0100 Subject: [PATCH 1/2] refactor: apply the Go style review's structure and test findings No behaviour changes but one error message. Following the Google Go Style Guide findings from the Codex and Claude Fable reviews: - tokenCmd was 145 lines mixing two flag styles. It is now tokenFlags, bound like appFlags, with resolve (validation, in the same order), urlOptions (the one place the URL flags become options) and mint. - The two package-level test hooks are parameters: runChild takes the foreground probe, environWithout whether to fold case. - withEnterpriseURL/withBaseURL return only their error; githubClient holds its throttle sleep, so retry tests record the wait they asked for instead of measuring wall-clock time, and the cancellation test cancels from inside the wait. TestSleepCtx keeps checking the real wait. The library suite went from 11s to under 6s with -race. - Signal tests signal once the child has written, and join the sender. - The mock transport, which rewrote the caller's request, is replaced by httptest; tokens are compared by public fields; sentinels with errors.Is; request bodies as JSON; stdout matchers return errors. - Naming and consistency: writeToken, splitList, maxRedirects, Test_ only for unexported symbols, got-before-want messages, testKeyPEM, rewrapped and corrected comments. The empty-PAT error now reads "personal access token is required". --- CHANGELOG.md | 3 + auth.go | 20 +- auth_test.go | 801 ++++++++++++++++-------------------- cmd/githubauth/exec.go | 29 +- cmd/githubauth/exec_test.go | 111 ++--- cmd/githubauth/main.go | 226 ++++++---- cmd/githubauth/main_test.go | 111 ++--- github.go | 23 +- github_server_test.go | 108 ----- github_test.go | 441 ++++++++++---------- http.go | 10 +- http_test.go | 22 +- webhook/webhook.go | 2 +- webhook/webhook_test.go | 10 +- 14 files changed, 917 insertions(+), 1000 deletions(-) delete mode 100644 github_server_test.go 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 0c29975..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") - } - 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 }, + errs = append(errs, err) + return } - 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 { @@ -1829,3 +1697,46 @@ func TestReuseTokenSourceWithSkew_NonPositiveDelegates(t *testing.T) { t.Errorf("ReuseTokenSourceWithSkew(nil, cache, 0) = %T wrapping the cache, want the cache itself", got) } } + +// TestWithExpirySkew_SetsTheWindow checks the option is the window the JWT +// cache refreshes on: a 2-minute JWT, live for 60s after the backdate, sits +// inside a 9-minute skew, so every Token call signs anew, and outside the +// default 30s one, so it is reused. +func TestWithExpirySkew_SetsTheWindow(t *testing.T) { + t.Parallel() + + rsaKey, err := rsa.GenerateKey(rand.Reader, 2048) + if err != nil { + t.Fatal(err) + } + tests := []struct { + name string + opts []ApplicationTokenOpt + wantSigns int + }{ + {name: "default skew reuses it", opts: nil, wantSigns: 1}, + {name: "a skew past its life refreshes it", opts: []ApplicationTokenOpt{WithExpirySkew(9 * time.Minute)}, wantSigns: 2}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + stub := &stubSigner{pub: &rsaKey.PublicKey, signFn: func(r io.Reader, digest []byte, opts crypto.SignerOpts) ([]byte, error) { + return rsaKey.Sign(r, digest, opts) + }} + opts := append([]ApplicationTokenOpt{WithApplicationTokenExpiration(2 * time.Minute)}, tt.opts...) + src, err := NewApplicationTokenSourceFromSigner(int64(1), stub, opts...) + if err != nil { + t.Fatalf("NewApplicationTokenSourceFromSigner() error = %v", err) + } + for range 2 { + if _, err := src.Token(); err != nil { + t.Fatalf("Token() error = %v", err) + } + } + if stub.gotCalls != tt.wantSigns { + t.Errorf("signs = %d, want %d", stub.gotCalls, tt.wantSigns) + } + }) + } +} 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..c4db9a0 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,173 @@ 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 { switch { - case *baseURL != "": - urlOpts = append(urlOpts, githubauth.WithBaseURL(*baseURL)) - case *enterpriseURL != "": - urlOpts = append(urlOpts, githubauth.WithEnterpriseURL(*enterpriseURL)) + case f.baseURL != "": + return []githubauth.InstallationTokenSourceOpt{githubauth.WithBaseURL(f.baseURL)} + case f.enterpriseURL != "": + return []githubauth.InstallationTokenSourceOpt{githubauth.WithEnterpriseURL(f.enterpriseURL)} } + return nil +} - 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 +652,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 +669,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 +742,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 +773,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 05ab548..bc1b865 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 }{ @@ -619,6 +615,14 @@ func TestRun(t *testing.T) { wantStdout: contains(`"type":"githubauth.error"`), quietStderr: true, }, + { + name: "a --json value that is not a bool is not on", + args: func(string) []string { + return []string{"token", "--json=maybe", "--bogus"} + }, + wantCode: exitUsage, + wantStdout: isEmpty, + }, { name: "an explicit empty --json= is not on", args: func(string) []string { @@ -862,12 +866,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) @@ -1082,10 +1089,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 { @@ -1234,7 +1241,7 @@ func TestReadKey(t *testing.T) { } } -func TestSplitRepos(t *testing.T) { +func TestSplitList(t *testing.T) { t.Parallel() tests := []struct { @@ -1253,13 +1260,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]) } } }) @@ -1347,7 +1354,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)} @@ -1375,18 +1382,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) } }) } @@ -1524,7 +1531,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) } } @@ -1603,3 +1615,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) From a654d709e089b512048eb2b6c9a92de134b64a28 Mon Sep 17 00:00:00 2001 From: jferrl Date: Sat, 31 Oct 2026 11:39:32 +0100 Subject: [PATCH 2/2] refactor: give urlOptions a single return The no-URL return was its own statement, which no test can reach without minting against api.github.com. Accumulating into one slice, as the code did before the split, keeps the function fully covered. --- cmd/githubauth/main.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/cmd/githubauth/main.go b/cmd/githubauth/main.go index c4db9a0..d7b7894 100644 --- a/cmd/githubauth/main.go +++ b/cmd/githubauth/main.go @@ -409,13 +409,14 @@ func (f *tokenFlags) resolve() (tokenRequest, error) { // 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 f.baseURL != "": - return []githubauth.InstallationTokenSourceOpt{githubauth.WithBaseURL(f.baseURL)} + opts = append(opts, githubauth.WithBaseURL(f.baseURL)) case f.enterpriseURL != "": - return []githubauth.InstallationTokenSourceOpt{githubauth.WithEnterpriseURL(f.enterpriseURL)} + opts = append(opts, githubauth.WithEnterpriseURL(f.enterpriseURL)) } - return nil + return opts } // mint looks the installation up when only its owner is known, then mints a