From 84406142ce1acdc1cd1ca1dc6e3304cc0a73852e Mon Sep 17 00:00:00 2001 From: jferrl Date: Sat, 31 Oct 2026 11:17:57 +0100 Subject: [PATCH 1/2] fix: correct the defects a Go style review found Two reviews against the Google Go Style Guide, by Codex and Claude Fable, found real defects alongside the style points: - oauth2.NewClient, the README's integration, wrapped this package's cache in its own, which refreshes 10s before expiry, so the 30s skew never applied. ReuseTokenSourceWithSkew is now oauth2's cache with the longer window, which NewClient reuses as is. It caches copies of the tokens, since oauth2 writes its window into each one and caches sharing a source's token raced on that. A non-positive skew is still exactly oauth2.ReuseTokenSource. - A huge Retry-After overflowed to a negative wait, read as retry now. - An ID past int64 in the environment saturated instead of being reported as malformed. - WithApplicationTokenExpiration wrote to its captured value when applied, racing when one option was shared. - --json or --agent after a malformed flag was never read, so the failure came out as prose. - version and help exited 0 when stdout failed; three CLI errors used %v, hiding their cause from errors.Is. - Identifier admits ~int, so NewApplicationTokenSource(12345, key) compiles. gofmt is now enforced by golangci-lint, CI runs the tests with -race, and doc comments that named missing types, overpromised option order or misdescribed oauth2's refresh are corrected. --- .github/workflows/test.yml | 2 +- .golangci.yml | 5 + CHANGELOG.md | 17 +++ README.md | 4 +- auth.go | 127 +++++++++++----------- auth_test.go | 206 ++++++++++++++++++++---------------- cmd/githubauth/exec.go | 2 +- cmd/githubauth/main.go | 66 ++++++++++-- cmd/githubauth/main_test.go | 84 +++++++++++++++ github.go | 12 ++- github_test.go | 3 + webhook/webhook.go | 20 ++-- 12 files changed, 368 insertions(+), 180 deletions(-) create mode 100644 .golangci.yml diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 04d0de7..174d32b 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -53,7 +53,7 @@ jobs: ${{ runner.os }}-go- - name: Run go test - run: go test ./... -coverprofile=coverage.out -covermode=atomic + run: go test -race ./... -coverprofile=coverage.out -covermode=atomic - name: Upload coverage to Codecov if: ${{ matrix.update-coverage }} diff --git a/.golangci.yml b/.golangci.yml new file mode 100644 index 0000000..366252d --- /dev/null +++ b/.golangci.yml @@ -0,0 +1,5 @@ +version: "2" + +formatters: + enable: + - gofmt diff --git a/CHANGELOG.md b/CHANGELOG.md index b9543d9..7df310c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -63,6 +63,8 @@ changes can affect existing code or scripts; they are listed under Changed. - An installation token carries what GitHub granted in its oauth2 Extra fields: `ExtraPermissions` and, for a token limited to some repositories, `ExtraRepositories`. `githubauth token --json` prints both (#90) +- `NewApplicationTokenSource(12345, key)` compiles: `Identifier` admits `~int`, so an + untyped constant needs no `int64(...)` conversion - The CLI is a GitHub CLI extension: `gh extension install jferrl/gh-githubauth`, run as `gh githubauth` (#91) @@ -84,6 +86,21 @@ changes can affect existing code or scripts; they are listed under Changed. ### Fixed +- **A source used through `oauth2.NewClient`, as the README shows, refreshed 10s before + expiry, not 30s.** `NewClient` added its own cache over this package's. + `ReuseTokenSourceWithSkew` is now oauth2's cache with the longer window, which + `NewClient` uses as is. It works on copies of the tokens it caches, so caches sharing a + source no longer race +- A `Retry-After` past about 292 years overflowed to a negative wait and retried at once. + It is capped at an hour, as documented +- `$GITHUB_APP_INSTALLATION_ID` or `$GITHUB_APP_ID` past the int64 range became the + largest int64 instead of being reported as malformed +- One `WithApplicationTokenExpiration` value applied by concurrent constructors raced +- `--json` or `--agent` after a malformed flag, as in `token --bogus --json`, still + produced a plain-text error; it now produces the JSON document +- `githubauth version` and `help` exited 0 when stdout could not be written +- Errors from a missing key file or a command that cannot start now wrap their cause, so + `errors.Is(err, fs.ErrNotExist)` and `exec.ErrNotFound` match - The exit-2 hint for a 422 names `--permissions` as well as `--repos`, and the exit-5 hint for a 404 names `--owner` (#88) diff --git a/README.md b/README.md index 7fd084c..c98ce1d 100644 --- a/README.md +++ b/README.md @@ -228,11 +228,11 @@ githubauth.NewInstallationTokenSource(installationID, appTokenSource, githubauth.WithBaseURL("https://api.octocorp.ghe.com")) ``` -Options combine in any order. An unparseable URL (or a nil client passed to `WithHTTPClient`) is reported by the first `Token()` call instead of silently falling back to the public GitHub API. +`WithHTTPClient` combines with either URL option in any order; given both `WithBaseURL` and `WithEnterpriseURL`, the last wins. An unparseable URL (or a nil client passed to `WithHTTPClient`) is reported by the first `Token()` call instead of silently falling back to the public GitHub API. ## Proactive token refresh -`oauth2.ReuseTokenSource` refreshes a cached token only *after* it expires, so a request that starts just before expiry can reach GitHub with a dead credential and 401. Both constructors instead wrap their sources in `ReuseTokenSourceWithSkew`, refreshing when `time.Until(exp) <= skew` (default `DefaultExpirySkew`, 30s). +`oauth2.ReuseTokenSource` refreshes a cached token 10s before it expires, which is not always enough for a request to reach GitHub in time. Both constructors instead wrap their sources in `ReuseTokenSourceWithSkew`, refreshing `skew` before expiry (default `DefaultExpirySkew`, 30s). It is oauth2's own cache with a longer window, so `oauth2.NewClient` uses it as is instead of adding its 10s one on top. ```go appTokenSource, err := githubauth.NewApplicationTokenSource(clientID, privateKey, diff --git a/auth.go b/auth.go index 7fc229a..6e887b2 100644 --- a/auth.go +++ b/auth.go @@ -13,7 +13,6 @@ import ( "reflect" "strconv" "strings" - "sync" "time" jwt "github.com/golang-jwt/jwt/v5" @@ -57,65 +56,49 @@ const ( bearerTokenType = "Bearer" ) -// ReuseTokenSourceWithSkew wraps src so cached tokens are refreshed proactively, -// skew before their expiry. oauth2.ReuseTokenSource refreshes only once exp has -// passed (via oauth2.Token.Valid), so a request that starts at T-100ms with a -// token expiring at T can arrive at GitHub already expired and yield a 401 the -// caller must manually retry. This wrapper refreshes when -// time.Until(t.Expiry) <= skew, cutting out that race. +// ReuseTokenSourceWithSkew wraps src so a cached token is refreshed skew before +// it expires, rather than oauth2's default of 10s, so a request that starts +// close to expiry does not reach GitHub with a dead token and earn a 401. // -// If skew is zero or negative the wrapper delegates to oauth2.ReuseTokenSource, -// preserving its exact behavior. An initial non-nil t is used until it needs -// refresh under the same rule. The returned source is safe for concurrent use; -// concurrent Token calls that find the cache stale collapse into a single -// upstream fetch. +// It is oauth2.ReuseTokenSourceWithExpiry, which oauth2.NewClient recognises +// and uses as is rather than adding a cache of its own that would refresh 10s +// before expiry whatever skew says. A zero or negative skew gives +// oauth2.ReuseTokenSource. An initial non-nil t is used until it needs refresh. +// The returned source is safe for concurrent use, and concurrent Token calls +// that find the cache stale collapse into a single upstream fetch. func ReuseTokenSourceWithSkew(t *oauth2.Token, src oauth2.TokenSource, skew time.Duration) oauth2.TokenSource { if skew <= 0 { return oauth2.ReuseTokenSource(t, src) } - return &reuseTokenSourceWithSkew{ - t: t, - src: src, - skew: skew, - } + // oauth2's cache writes its window into each token it holds. Copies keep + // that write off tokens the caller or src shares, such as a + // StaticTokenSource's, which two caches would otherwise race on. + return oauth2.ReuseTokenSourceWithExpiry(copyToken(t), copyingSource{src}, skew) } -type reuseTokenSourceWithSkew struct { - mu sync.Mutex - t *oauth2.Token - src oauth2.TokenSource - skew time.Duration -} +// copyingSource returns a copy of each token src returns. +type copyingSource struct{ src oauth2.TokenSource } -// Token returns the cached token if it is still valid beyond the configured -// skew, otherwise it calls the underlying source and caches the result. -func (r *reuseTokenSourceWithSkew) Token() (*oauth2.Token, error) { - r.mu.Lock() - defer r.mu.Unlock() - if r.valid() { - return r.t, nil - } - t, err := r.src.Token() +func (c copyingSource) Token() (*oauth2.Token, error) { + t, err := c.src.Token() if err != nil { return nil, err } - r.t = t - return t, nil + return copyToken(t), nil } -func (r *reuseTokenSourceWithSkew) valid() bool { - if r.t == nil || r.t.AccessToken == "" { - return false - } - if r.t.Expiry.IsZero() { - return true +func copyToken(t *oauth2.Token) *oauth2.Token { + if t == nil { + return nil } - return time.Until(r.t.Expiry) > r.skew + c := *t + return &c } -// Identifier constrains GitHub App identifiers to int64 (App ID) or string (Client ID). +// Identifier constrains GitHub App identifiers to an integer App ID or a string +// Client ID. int is admitted so an untyped constant such as 12345 is accepted. type Identifier interface { - ~int64 | ~string + ~int64 | ~int | ~string } // applicationTokenSource generates GitHub App JWTs for authentication. @@ -130,7 +113,8 @@ type applicationTokenSource struct { skew time.Duration } -// ApplicationTokenOpt is a functional option for configuring an applicationTokenSource. +// ApplicationTokenOpt configures NewApplicationTokenSource and +// NewApplicationTokenSourceFromSigner. type ApplicationTokenOpt func(*applicationTokenSource) // WithApplicationTokenExpiration sets the JWT expiration duration. @@ -142,19 +126,20 @@ type ApplicationTokenOpt func(*applicationTokenSource) // Invalid values default to 10 minutes. // See https://docs.github.com/en/apps/creating-github-apps/authenticating-with-a-github-app/generating-a-json-web-token-jwt-for-a-github-app#about-json-web-tokens-jwts func WithApplicationTokenExpiration(exp time.Duration) ApplicationTokenOpt { + // Normalized once, here: an option is a value callers may share across + // goroutines, so applying it must not write to what it captured. + if exp > DefaultApplicationTokenExpiration || exp <= minApplicationTokenExpiration { + exp = DefaultApplicationTokenExpiration + } return func(a *applicationTokenSource) { - if exp > DefaultApplicationTokenExpiration || exp <= minApplicationTokenExpiration { - exp = DefaultApplicationTokenExpiration - } a.expiration = exp } } // WithExpirySkew overrides the default early-refresh window (DefaultExpirySkew, // 30s) applied to the token cache returned by NewApplicationTokenSource. The -// cached token is refreshed when time.Until(exp) <= d. A zero or negative value -// disables the skew and falls back to oauth2.ReuseTokenSource behavior -// (refresh only after exp has passed). +// cached token is refreshed d before it expires. A zero or negative value falls +// back to oauth2.ReuseTokenSource and its 10s. // // Tune this when your application token expiration (see // WithApplicationTokenExpiration) is short: the effective validity is @@ -225,8 +210,8 @@ func NewApplicationTokenSourceFromSigner[T Identifier](id T, signer crypto.Signe // resolveIssuer converts a generic App ID / Client ID to its string form // and rejects zero values. func resolveIssuer[T Identifier](id T) (string, error) { - // Identifier is "~int64 | ~string", so both forms are comparable and their - // zero value — 0 or "" — is the one input GitHub cannot use. + // Every Identifier form is comparable, and its zero value, 0 or "", is the + // one input GitHub cannot use. var zero T if id == zero { return "", errors.New("application identifier is required") @@ -242,7 +227,7 @@ func resolveIssuer[T Identifier](id T) (string, error) { return v.String(), nil } - // Identifier admits no kind other than int64 here. + // Identifier admits only int and int64 kinds here; Int reads both. return strconv.FormatInt(v.Int(), 10), nil } @@ -259,12 +244,11 @@ func newApplicationTokenSource(issuer string, signer crypto.Signer, opts ...Appl return ReuseTokenSourceWithSkew(nil, t, t.skew) } -// Token generates a GitHub App JWT with required claims: iat, exp, iss, and alg. +// Token signs a GitHub App JWT with the iat, exp and iss claims, using RS256. // The iat claim is set 60 seconds in the past to account for clock drift. // Signing is routed through the configured crypto.Signer. // Generated JWTs can be used with "Authorization: Bearer" header for GitHub API requests. func (t *applicationTokenSource) Token() (*oauth2.Token, error) { - // To protect against clock drift, set the issuance time 60 seconds in the past. now := time.Now().Add(-applicationTokenBackdate) expiresAt := now.Add(t.expiration) @@ -295,7 +279,8 @@ func (t *applicationTokenSource) Token() (*oauth2.Token, error) { }, nil } -// InstallationTokenSourceOpt is a functional option for InstallationTokenSource. +// InstallationTokenSourceOpt configures NewInstallationTokenSource, and the +// subset RevokeInstallationToken and FindInstallationID document. type InstallationTokenSourceOpt func(*installationTokenSource) // WithInstallationTokenOptions sets the options for the GitHub App installation token. @@ -307,8 +292,8 @@ func WithInstallationTokenOptions(opts *InstallationTokenOptions) InstallationTo // WithHTTPClient sets the HTTP client used to call the GitHub API. Its transport // is wrapped so installation-token requests are authenticated with the GitHub -// App JWT; any base URL configured via WithBaseURL or WithEnterpriseURL is -// preserved, so these options may be combined in any order. +// App JWT; a base URL from WithBaseURL or WithEnterpriseURL is kept whichever +// comes first. // // A nil client is a configuration error reported by the first call to Token(). func WithHTTPClient(client *http.Client) InstallationTokenSourceOpt { @@ -347,10 +332,11 @@ func WithHTTPClient(client *http.Client) InstallationTokenSourceOpt { // host is not already an "api." subdomain. For GitHub Enterprise Cloud or a // verbatim URL (such as an httptest server), use WithBaseURL instead. // -// Option order does not matter; it may be combined with WithHTTPClient, -// WithBaseURL, and WithRetryOnThrottle in any order. If the URL cannot be -// parsed, the error is reported by the first call to Token() rather than -// silently falling back to the public GitHub API. +// It combines with WithHTTPClient and WithRetryOnThrottle in any order. Given +// with WithBaseURL, the last of the two wins. If the URL cannot be parsed, the +// error is reported by the first call to Token() rather than silently falling +// back to the public GitHub API; with several invalid options, the first +// one's error is reported. func WithEnterpriseURL(baseURL string) InstallationTokenSourceOpt { return func(i *installationTokenSource) { if _, err := i.client.withEnterpriseURL(baseURL); err != nil { @@ -368,10 +354,11 @@ func WithEnterpriseURL(baseURL string) InstallationTokenSourceOpt { // - GitHub Enterprise Cloud with data residency (https://api.SUBDOMAIN.ghe.com/) // - pointing the client at an httptest server in tests // -// Option order does not matter; it may be combined with WithHTTPClient, -// WithEnterpriseURL, and WithRetryOnThrottle in any order. If the URL cannot be +// It combines with WithHTTPClient and WithRetryOnThrottle in any order. Given +// with WithEnterpriseURL, the last of the two wins. If the URL cannot be // parsed, the error is reported by the first call to Token() rather than -// silently falling back to the public GitHub API. +// silently falling back to the public GitHub API; with several invalid +// 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 { @@ -380,7 +367,11 @@ func WithBaseURL(baseURL string) InstallationTokenSourceOpt { } } -// WithContext sets the context for the GitHub App installation token source. +// 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. func WithContext(ctx context.Context) InstallationTokenSourceOpt { return func(i *installationTokenSource) { i.ctx = ctx @@ -536,7 +527,9 @@ func newInstallationTokenSource(id int64, src oauth2.TokenSource, opts []Install // from NewApplicationTokenSource already caches with its own skew, and wrapping // it again would let the outer cache hold the JWT past that skew. func cachedSource(src oauth2.TokenSource) oauth2.TokenSource { - if _, ok := src.(*reuseTokenSourceWithSkew); ok { + // ReuseTokenSource hands src back only when it already is an oauth2 cache, + // as NewApplicationTokenSource returns; that cache keeps its own window. + if oauth2.ReuseTokenSource(nil, src) == src { return src } return ReuseTokenSourceWithSkew(nil, src, DefaultExpirySkew) diff --git a/auth_test.go b/auth_test.go index e8b0361..0c29975 100644 --- a/auth_test.go +++ b/auth_test.go @@ -565,11 +565,24 @@ func TestWithBaseURL_DrivesTokenThroughServer(t *testing.T) { TokenType: "Bearer", Expiry: expiration, } - if !reflect.DeepEqual(got, want) { + if !sameToken(got, want) { t.Errorf("Token() = %v, want %v", got, want) } } +// sameToken compares what a caller can see of a token: its public fields and +// the extras this package documents, not oauth2's unexported bookkeeping. +func sameToken(got, want *oauth2.Token) bool { + if got == nil || want == nil { + return got == want + } + return got.AccessToken == want.AccessToken && + got.TokenType == want.TokenType && + got.Expiry.Equal(want.Expiry) && + reflect.DeepEqual(got.Extra(ExtraPermissions), want.Extra(ExtraPermissions)) && + reflect.DeepEqual(got.Extra(ExtraRepositories), want.Extra(ExtraRepositories)) +} + // TestWithBaseURL_SurvivesHTTPClientOrder guards against the ordering footgun: // WithBaseURL is applied BEFORE WithHTTPClient, and the configured base URL must // survive the HTTP client swap. If it did not, the request would be sent to the @@ -752,7 +765,7 @@ func Test_installationTokenSource_Token(t *testing.T) { t.Errorf("installationTokenSource.Token() error = %v, wantErr %v", err, tt.wantErr) return } - if !reflect.DeepEqual(got, tt.want) { + if !sameToken(got, tt.want) { t.Errorf("installationTokenSource.Token() = %v, want %v", got, tt.want) } }) @@ -1190,105 +1203,60 @@ func TestReuseTokenSourceWithSkew(t *testing.T) { } } -// TestWithExpirySkew_Wiring verifies that WithExpirySkew threads through to -// the constructor-selected wrapper: a positive skew yields our -// *reuseTokenSourceWithSkew, a non-positive skew falls back to -// oauth2.ReuseTokenSource. We compare by type rather than by token-body -// equality because the JWT payload is deterministic within a single clock -// second, which would make a caching check ambiguous. -func TestWithExpirySkew_Wiring(t *testing.T) { - privateKey, err := generatePrivateKey() - if err != nil { - t.Fatal(err) - } +// TestReuseTokenSourceWithSkew_ThroughNewClient checks the skew survives +// oauth2.NewClient, the integration the README recommends. NewClient wraps any +// source it does not recognise in its own cache, which refreshes only 10s +// before expiry, so a token inside the skew was being reused. +func TestReuseTokenSourceWithSkew_ThroughNewClient(t *testing.T) { + t.Parallel() - tests := []struct { - name string - skewOpts []ApplicationTokenOpt - wantSkew bool // true → *reuseTokenSourceWithSkew, false → delegated - }{ - { - name: "default skew uses wrapper", - skewOpts: nil, - wantSkew: true, - }, - { - name: "explicit positive skew uses wrapper", - skewOpts: []ApplicationTokenOpt{WithExpirySkew(5 * time.Second)}, - wantSkew: true, - }, - { - name: "zero skew delegates to oauth2.ReuseTokenSource", - skewOpts: []ApplicationTokenOpt{WithExpirySkew(0)}, - wantSkew: false, - }, - { - name: "negative skew delegates to oauth2.ReuseTokenSource", - skewOpts: []ApplicationTokenOpt{WithExpirySkew(-1 * time.Second)}, - wantSkew: false, - }, + server := httptest.NewServer(http.HandlerFunc(func(http.ResponseWriter, *http.Request) {})) + defer server.Close() + + var fetched atomic.Int32 + // 20s left: past oauth2's 10s, inside the 30s skew, so it is stale here. + src := tokenSourceFunc(func() (*oauth2.Token, error) { + fetched.Add(1) + return &oauth2.Token{AccessToken: "t", Expiry: time.Now().Add(20 * time.Second)}, nil + }) + client := oauth2.NewClient(context.Background(), ReuseTokenSourceWithSkew(nil, src, 30*time.Second)) + + for range 2 { + resp, err := client.Get(server.URL) + if err != nil { + t.Fatalf("Get() error = %v", err) + } + _ = resp.Body.Close() } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - ts, err := NewApplicationTokenSource(int64(99), privateKey, tt.skewOpts...) - if err != nil { - t.Fatalf("NewApplicationTokenSource: %v", err) - } - _, isSkew := ts.(*reuseTokenSourceWithSkew) - if isSkew != tt.wantSkew { - t.Errorf("token source is *reuseTokenSourceWithSkew = %v, want %v (type=%T)", isSkew, tt.wantSkew, ts) - } - }) + if got := fetched.Load(); got != 2 { + t.Errorf("token fetches = %d, want 2: each request must refresh a token inside the skew", got) } } -// Test_WithInstallationExpirySkew_Wiring mirrors TestWithExpirySkew_Wiring one -// layer down: the option must select the early-refresh wrapper for a positive -// skew and fall back to oauth2.ReuseTokenSource for a non-positive one. -func Test_WithInstallationExpirySkew_Wiring(t *testing.T) { +// TestReuseTokenSourceWithSkew_SharedToken checks caches sharing one source's +// token do not write to it: oauth2's cache stores its window in each token it +// holds, which raced when two caches held the same one. Run with -race. +func TestReuseTokenSourceWithSkew_SharedToken(t *testing.T) { t.Parallel() - tests := []struct { - name string - opts []InstallationTokenSourceOpt - wantSkew bool // true → *reuseTokenSourceWithSkew, false → delegated - }{ - { - name: "default skew uses wrapper", - opts: nil, - wantSkew: true, - }, - { - name: "explicit positive skew uses wrapper", - opts: []InstallationTokenSourceOpt{WithInstallationExpirySkew(5 * time.Minute)}, - wantSkew: true, - }, - { - name: "zero skew delegates to oauth2.ReuseTokenSource", - opts: []InstallationTokenSourceOpt{WithInstallationExpirySkew(0)}, - wantSkew: false, - }, - { - name: "negative skew delegates to oauth2.ReuseTokenSource", - opts: []InstallationTokenSourceOpt{WithInstallationExpirySkew(-1 * time.Second)}, - wantSkew: false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - - ts := NewInstallationTokenSource(42, &countingTokenSource{}, tt.opts...) - - _, isSkew := ts.(*reuseTokenSourceWithSkew) - if isSkew != tt.wantSkew { - t.Errorf("token source is *reuseTokenSourceWithSkew = %v, want %v (type=%T)", isSkew, tt.wantSkew, ts) + shared := oauth2.StaticTokenSource(&oauth2.Token{AccessToken: "t", Expiry: time.Now().Add(time.Hour)}) + var wg sync.WaitGroup + for range 4 { + src := ReuseTokenSourceWithSkew(nil, shared, 30*time.Second) + wg.Go(func() { + if _, err := src.Token(); err != nil { + t.Errorf("Token() error = %v", err) } }) } + wg.Wait() } +// tokenSourceFunc adapts a function to oauth2.TokenSource. +type tokenSourceFunc func() (*oauth2.Token, error) + +func (f tokenSourceFunc) Token() (*oauth2.Token, error) { return f() } + // Test_WithInstallationExpirySkew_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 @@ -1801,3 +1769,63 @@ func Test_WithApplicationTokenExpiration_TokenIsCacheable(t *testing.T) { }) } } + +// TestNewApplicationTokenSource_IntegerLiteral checks an untyped constant, the +// form a first-time caller writes, is accepted as an App ID. +func TestNewApplicationTokenSource_IntegerLiteral(t *testing.T) { + t.Parallel() + + key, err := generatePrivateKey() + if err != nil { + t.Fatal(err) + } + src, err := NewApplicationTokenSource(12345, key) + if err != nil { + t.Fatalf("NewApplicationTokenSource(12345) error = %v", err) + } + tok, err := src.Token() + if err != nil { + t.Fatalf("Token() error = %v", err) + } + claims := jwt.RegisteredClaims{} + if _, _, err := jwt.NewParser().ParseUnverified(tok.AccessToken, &claims); err != nil { + t.Fatal(err) + } + if claims.Issuer != "12345" { + t.Errorf("iss = %q, want %q", claims.Issuer, "12345") + } +} + +// TestWithApplicationTokenExpiration_SharedOption checks one option value can +// be applied by concurrent constructors. Run with -race: applying it used to +// write the normalized expiration back into what the option captured. +func TestWithApplicationTokenExpiration_SharedOption(t *testing.T) { + t.Parallel() + + key, err := generatePrivateKey() + if err != nil { + t.Fatal(err) + } + opt := WithApplicationTokenExpiration(time.Hour) // out of range, so normalized + var wg sync.WaitGroup + for range 4 { + wg.Go(func() { + if _, err := NewApplicationTokenSource(int64(1), key, opt); err != nil { + t.Errorf("NewApplicationTokenSource() error = %v", err) + } + }) + } + wg.Wait() +} + +// TestReuseTokenSourceWithSkew_NonPositiveDelegates checks a zero skew is +// oauth2.ReuseTokenSource exactly: wrapping an existing oauth2 cache returns +// that cache, as oauth2 does, rather than caching twice. +func TestReuseTokenSourceWithSkew_NonPositiveDelegates(t *testing.T) { + t.Parallel() + + inner := oauth2.ReuseTokenSource(nil, oauth2.StaticTokenSource(&oauth2.Token{AccessToken: "t"})) + if got := ReuseTokenSourceWithSkew(nil, inner, 0); got != inner { + t.Errorf("ReuseTokenSourceWithSkew(nil, cache, 0) = %T wrapping the cache, want the cache itself", got) + } +} diff --git a/cmd/githubauth/exec.go b/cmd/githubauth/exec.go index 9ee5b66..e54165e 100644 --- a/cmd/githubauth/exec.go +++ b/cmd/githubauth/exec.go @@ -72,7 +72,7 @@ func runWithToken(argv []string, tok *oauth2.Token, host string, stdin io.Reader // The credential was minted, so a command that cannot start is the // invocation to fix, not something to retry. if err := cmd.Start(); err != nil { - return usagef("running %s: %v", argv[0], err) + return usagef("running %s: %w", argv[0], err) } done := make(chan struct{}) diff --git a/cmd/githubauth/main.go b/cmd/githubauth/main.go index abe64f9..be15834 100644 --- a/cmd/githubauth/main.go +++ b/cmd/githubauth/main.go @@ -98,11 +98,9 @@ func run(args []string, stdin io.Reader, stdout, stderr io.Writer) int { case "jwt": err = jwtCmd(args[1:], stdin, stdout, stderr, &out) case "version", "--version", "-version", "-v": - _, _ = fmt.Fprintln(stdout, version()) - return exitOK + _, err = fmt.Fprintln(stdout, version()) case "help", "--help", "-help", "-h": - _, _ = fmt.Fprint(stdout, rootHelp) - return exitOK + _, err = fmt.Fprint(stdout, rootHelp) default: err = unknownCommand(args[0]) } @@ -353,7 +351,6 @@ func tokenCmd(args []string, stdin io.Reader, stdout, stderr io.Writer, out *out 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 @@ -463,9 +460,12 @@ func parseFlags(fs *flag.FlagSet, args []string, out *output, stdout, stderr io. err := fs.Parse(args) switch { case errors.Is(err, flag.ErrHelp): - _, _ = diag.WriteTo(stdout) + if _, werr := diag.WriteTo(stdout); werr != nil { + return werr + } return err case err != nil: + scanOutputFlags(fs, args, out) if !out.json && !out.agent { _, _ = diag.WriteTo(stderr) } @@ -474,6 +474,46 @@ func parseFlags(fs *flag.FlagSet, args []string, out *output, stdout, stderr io. return nil } +// scanOutputFlags sets --json and --agent from the arguments the flag package +// never reached, because one before them failed to parse, so the failure is +// still rendered the way the caller asked. It stops where flag parsing would: +// at "--" or the first argument that is not a flag. +func scanOutputFlags(fs *flag.FlagSet, args []string, out *output) { + for i := 0; i < len(args); i++ { + arg := args[i] + if arg == "--" || arg == "-" || !strings.HasPrefix(arg, "-") { + return + } + name, value, hasValue := strings.Cut(strings.TrimPrefix(strings.TrimPrefix(arg, "-"), "-"), "=") + f := fs.Lookup(name) + if f == nil { + continue + } + if b, ok := f.Value.(interface{ IsBoolFlag() bool }); !ok || !b.IsBoolFlag() { + // Its value is the next argument, whatever it looks like. + if !hasValue { + i++ + } + continue + } + on := true + if hasValue { + // An explicit --json= is as invalid here as to the flag package. + b, err := strconv.ParseBool(value) + if err != nil { + continue + } + on = b + } + switch name { + case "json": + out.json = on + case "agent": + out.agent = on + } + } +} + // badEnvNumber explains a missing numeric flag whose environment fallback was // set but malformed, which would otherwise read as never set at all. func badEnvNumber(fs *flag.FlagSet, name, key string) error { @@ -561,9 +601,9 @@ func readKey(v string, stdin io.Reader) ([]byte, error) { // the value is named only when it is short enough to be a path. var pe *os.PathError if errors.As(err, &pe) && len(v) > maxKeyPathLen { - return nil, usagef("reading private key: --key is neither PEM, %q, nor a file it can open (%v); a base64-encoded key needs decoding first", "-", pe.Err) + return nil, usagef("reading private key: --key is neither PEM, %q, nor a file it can open (%w); a base64-encoded key needs decoding first", "-", pe.Err) } - return nil, usagef("reading private key: %v", err) + return nil, usagef("reading private key: %w", err) } return pem, nil } @@ -704,7 +744,11 @@ func parsePermissions(v string) (*githubauth.InstallationPermissions, error) { // envInt64 ignores a malformed value here, before the flags are parsed; // badEnvNumber reports it where the value would have been used. func envInt64(key string) int64 { - n, _ := strconv.ParseInt(os.Getenv(key), 10, 64) + n, err := strconv.ParseInt(os.Getenv(key), 10, 64) + if err != nil { + // ParseInt returns the nearest int64 on overflow; that is no ID either. + return 0 + } return n } @@ -750,8 +794,8 @@ func version() string { return "(devel) " + revision } -// nearest suggests a command for a typo, matching on a shared prefix or a -// single edit, which covers the realistic slips. +// nearest suggests a command for a typo, matching on a shared prefix or at most +// two edits, which covers the realistic slips, transpositions included. func nearest(in string) string { for _, cmd := range []string{"token", "jwt", "version", "help"} { if strings.HasPrefix(cmd, in) || strings.HasPrefix(in, cmd) || editDistance(in, cmd) <= 2 { diff --git a/cmd/githubauth/main_test.go b/cmd/githubauth/main_test.go index d615dca..05ab548 100644 --- a/cmd/githubauth/main_test.go +++ b/cmd/githubauth/main_test.go @@ -12,6 +12,7 @@ import ( "flag" "fmt" "io" + "io/fs" "net/http" "net/http/httptest" "os" @@ -588,6 +589,54 @@ func TestRun(t *testing.T) { wantCode: 0, wantStdout: contains("GH_HOST=ghe.example.com"), }, + { + // ParseInt saturates on overflow; that must not become an ID. + name: "an installation ID past int64 in the environment is malformed", + args: func(string) []string { + return []string{"token", "--client-id", "Iv1.abc", "--key", keyPath} + }, + env: map[string]string{"GITHUB_APP_INSTALLATION_ID": "9223372036854775808"}, + wantCode: exitUsage, + wantStdout: isEmpty, + wantStderr: "is not a number", + }, + { + // The flag package stops at --bogus and never reads --json. + name: "json after a malformed flag still renders the failure as JSON", + args: func(string) []string { + return []string{"token", "--bogus", "--json"} + }, + wantCode: exitUsage, + wantStdout: contains(`"type":"githubauth.error"`), + quietStderr: true, + }, + { + name: "json after a malformed value still renders the failure as JSON", + args: func(string) []string { + return []string{"token", "--installation", "nope", "--json"} + }, + wantCode: exitUsage, + wantStdout: contains(`"type":"githubauth.error"`), + quietStderr: true, + }, + { + name: "an explicit empty --json= is not on", + args: func(string) []string { + return []string{"token", "--json=", "--bogus"} + }, + wantCode: exitUsage, + wantStdout: isEmpty, + }, + { + // --json is the value of --key here, not a flag. + name: "a flag's value that looks like --json is a value", + args: func(string) []string { + return []string{"token", "--key", "--json", "--bogus"} + }, + wantCode: exitUsage, + wantStdout: isEmpty, + wantStderr: "flag provided but not defined", + }, { name: "keep without exec is misuse", args: func(string) []string { @@ -1519,3 +1568,38 @@ func TestOriginRepoHost(t *testing.T) { }) } } + +// failingWriter is a stdout that cannot be written, such as a closed pipe. +type failingWriter struct{} + +func (failingWriter) Write([]byte) (int, error) { return 0, errors.New("broken pipe") } + +// TestRunReportsAFailedWrite checks version and help do not claim success when +// stdout took nothing, as credential output already did not. +func TestRunReportsAFailedWrite(t *testing.T) { + clearEnv(t) + + for _, cmd := range []string{"version", "help", "token --help"} { + t.Run(cmd, func(t *testing.T) { + var stderr bytes.Buffer + if got := run(strings.Fields(cmd), strings.NewReader(""), failingWriter{}, &stderr); got != exitFailure { + t.Errorf("run(%q) = %d, want %d (stderr: %s)", cmd, got, exitFailure, stderr.String()) + } + }) + } +} + +// TestErrorsKeepTheirCause checks the CLI's wrapped errors still match the +// cause with errors.Is. +func TestErrorsKeepTheirCause(t *testing.T) { + t.Parallel() + + if _, err := readKey("no-such-app.pem", strings.NewReader("")); !errors.Is(err, fs.ErrNotExist) { + t.Errorf("readKey(missing) error = %v, want it to wrap fs.ErrNotExist", err) + } + tok := &oauth2.Token{AccessToken: "ghs_x"} + err := runWithToken([]string{"githubauth-no-such-command"}, tok, "", strings.NewReader(""), io.Discard, io.Discard) + if !errors.Is(err, exec.ErrNotFound) { + t.Errorf("runWithToken(missing command) error = %v, want it to wrap exec.ErrNotFound", err) + } +} diff --git a/github.go b/github.go index 0cd4e46..322256c 100644 --- a/github.go +++ b/github.go @@ -75,6 +75,7 @@ type RateLimitError struct { Message string } +// Error returns the status GitHub answered and its message. func (e *RateLimitError) Error() string { return fmt.Sprintf("%s: GitHub API returned status %d: %s", ErrRateLimited.Error(), e.StatusCode, e.Message) } @@ -112,6 +113,7 @@ type APIError struct { Message string } +// Error returns the status GitHub answered and its message. func (e *APIError) Error() string { return fmt.Sprintf("GitHub API returned status %d: %s", e.StatusCode, e.Message) } @@ -249,7 +251,8 @@ type Repository struct { Name *string `json:"name,omitempty"` } -// githubClient is a simple GitHub API client for creating installation tokens. +// githubClient makes the few GitHub API calls an App token needs: minting and +// revoking installation tokens, and finding an installation. type githubClient struct { baseURL *url.URL httpClient *http.Client @@ -523,8 +526,11 @@ func isRateLimit403(resp *http.Response, body []byte) bool { // "zero seconds". func parseRetryAfter(v string, now time.Time) (time.Duration, bool) { v = strings.TrimSpace(v) - if secs, err := strconv.Atoi(v); err == nil { - return time.Duration(secs) * time.Second, true + if secs, err := strconv.ParseInt(v, 10, 64); err == nil { + // Bounded before multiplying: a huge value would overflow, and a + // negative wait reads as "retry now". Callers clamp the rest. + limit := int64(maxRetryAfter / time.Second) + return time.Duration(min(max(secs, -limit), limit)) * time.Second, true } if t, err := http.ParseTime(v); err == nil { return t.Sub(now), true diff --git a/github_test.go b/github_test.go index 314d107..62c46de 100644 --- a/github_test.go +++ b/github_test.go @@ -866,6 +866,9 @@ func Test_parseRetryAfter(t *testing.T) { }{ {name: "integer seconds", value: "5", want: 5 * time.Second, wantOK: true}, {name: "integer seconds with whitespace", value: " 7 ", want: 7 * time.Second, wantOK: true}, + // Multiplied uncapped, these overflowed to a negative "retry now". + {name: "seconds past the cap", value: "9223372036854775807", want: maxRetryAfter, wantOK: true}, + {name: "negative seconds past the cap", value: "-9223372036854775808", want: -maxRetryAfter, wantOK: true}, {name: "zero seconds", value: "0", want: 0, wantOK: true}, {name: "negative seconds", value: "-3", want: -3 * time.Second, wantOK: true}, {name: "HTTP-date future", value: now.Add(10 * time.Second).Format(http.TimeFormat), want: 10 * time.Second, wantOK: true}, diff --git a/webhook/webhook.go b/webhook/webhook.go index 4da788f..ff942cd 100644 --- a/webhook/webhook.go +++ b/webhook/webhook.go @@ -22,9 +22,12 @@ import ( // Header names GitHub sets on every webhook delivery. const ( + // SignatureHeader carries the HMAC-SHA256 of the body, as "sha256=". SignatureHeader = "X-Hub-Signature-256" - EventHeader = "X-GitHub-Event" - DeliveryHeader = "X-GitHub-Delivery" + // EventHeader names the event, such as "push" or "pull_request". + EventHeader = "X-GitHub-Event" + // DeliveryHeader carries the delivery's unique ID. + DeliveryHeader = "X-GitHub-Delivery" ) // DefaultMaxPayloadSize matches GitHub's documented 25 MiB delivery cap. @@ -34,13 +37,18 @@ const signaturePrefix = "sha256=" // Sentinel errors returned by Verify. Callers can branch with errors.Is. var ( - ErrMissingSecret = errors.New("webhook: secret must not be empty") - ErrMissingSignature = errors.New("webhook: missing signature header") + // ErrMissingSecret means the secret was empty, which would accept any body. + ErrMissingSecret = errors.New("webhook: secret must not be empty") + // ErrMissingSignature means the delivery carried no signature. + ErrMissingSignature = errors.New("webhook: missing signature header") + // ErrInvalidSignatureFormat means the signature is not "sha256=". ErrInvalidSignatureFormat = errors.New("webhook: invalid signature format") - ErrSignatureMismatch = errors.New("webhook: signature mismatch") + // ErrSignatureMismatch means the signature does not match the body. + ErrSignatureMismatch = errors.New("webhook: signature mismatch") ) -// Verify reports whether signature is a valid HMAC-SHA256 of body using secret. +// Verify returns nil when signature is a valid HMAC-SHA256 of body using +// secret, and one of the Err sentinels otherwise. // signature must be in GitHub's "sha256=" form, as delivered in the // X-Hub-Signature-256 header. Comparison runs in constant time. // From 03a93a9f494a685821d787a00ee381d3281445d5 Mon Sep 17 00:00:00 2001 From: jferrl Date: Sat, 31 Oct 2026 11:36:49 +0100 Subject: [PATCH 2/2] test: cover WithExpirySkew and the output-flag scan PR1 deleted the wiring tests that were WithExpirySkew's only coverage, and left three branches of scanOutputFlags untested, which together dropped project coverage below main's. A behaviour test now shows the skew is the window the JWT cache refreshes on, and four CLI cases pin the scan: --agent after a bad flag, an explicit --json=true, a --json past a positional argument, and a --json value that is not a bool. --- auth_test.go | 43 +++++++++++++++++++++++++++++++++++++ cmd/githubauth/main_test.go | 36 +++++++++++++++++++++++++++++++ 2 files changed, 79 insertions(+) diff --git a/auth_test.go b/auth_test.go index 0c29975..19c175a 100644 --- a/auth_test.go +++ b/auth_test.go @@ -1829,3 +1829,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/main_test.go b/cmd/githubauth/main_test.go index 05ab548..aa46a33 100644 --- a/cmd/githubauth/main_test.go +++ b/cmd/githubauth/main_test.go @@ -619,6 +619,42 @@ func TestRun(t *testing.T) { wantStdout: contains(`"type":"githubauth.error"`), quietStderr: true, }, + { + name: "agent after a malformed flag still renders the failure as JSON", + args: func(string) []string { + return []string{"token", "--bogus", "--agent"} + }, + wantCode: exitUsage, + wantStdout: contains(`"type":"githubauth.error"`), + quietStderr: true, + }, + { + name: "an explicit --json=true after a malformed flag is on", + args: func(string) []string { + return []string{"token", "--bogus", "--json=true"} + }, + wantCode: exitUsage, + wantStdout: contains(`"type":"githubauth.error"`), + quietStderr: true, + }, + { + // The flag package stops at the first argument that is not a flag, + // so a --json past it is the command's, not githubauth's. + name: "a --json past a positional argument is not read", + args: func(string) []string { + return []string{"token", "--bogus", "gh", "--json"} + }, + wantCode: exitUsage, + wantStdout: isEmpty, + }, + { + 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 {