From b96f2a2d5a2bf662c00d3e1fe89a43cfc9f44ed6 Mon Sep 17 00:00:00 2001 From: jferrl Date: Tue, 30 Jun 2026 16:34:52 +0200 Subject: [PATCH] feat: add WithBaseURL option for custom API base URLs (#50) Add WithBaseURL, an InstallationTokenSourceOpt that sets the GitHub API base URL verbatim (only normalizing a trailing slash), closely mirroring how go-github lets callers target a custom endpoint. Unlike WithEnterpriseURL it does not append "/api/v3/", which unblocks: - GitHub Enterprise Cloud with data residency (https://api.SUBDOMAIN.ghe.com/) - pointing the client at an httptest server in tests Also harden the installation-token options so they cannot be misconfigured: - Option order no longer matters. WithHTTPClient previously rebuilt the client and silently discarded any base URL or retry setting applied before it; it now swaps only the underlying *http.Client. - Invalid base URLs and a nil HTTP client are reported by the first Token() call instead of silently falling back to the public GitHub API. And apply a few idiomatic, non-breaking cleanups: - WithHTTPClient operates on a shallow copy so the caller's *http.Client is no longer mutated. - Drop the deprecated, no-op net.Dialer.DualStack field. - Rename the unexported githubClient.client field to httpClient. README documents WithBaseURL vs WithEnterpriseURL; tests cover the new option, order-independence, fail-loud misconfiguration, and that the caller's HTTP client is not mutated. --- README.md | 32 ++++++++ auth.go | 78 +++++++++++++++--- auth_test.go | 183 +++++++++++++++++++++++++++++++++++++++--- github.go | 25 +++++- github_server_test.go | 7 +- github_test.go | 71 +++++++++++++++- http.go | 1 - 7 files changed, 368 insertions(+), 29 deletions(-) diff --git a/README.md b/README.md index 42d01e1..0304efe 100644 --- a/README.md +++ b/README.md @@ -278,6 +278,38 @@ func main() { } ``` +### GitHub Enterprise and Custom Base URLs + +The installation token source talks to `https://api.github.com/` by default. Two options point it elsewhere: + +- `WithEnterpriseURL` — for **GitHub Enterprise Server (GHES)**. The supplied URL is normalized the way GHES expects, appending `/api/v3/` when the host is not already an `api.` subdomain. +- `WithBaseURL` — uses the URL **verbatim** (only adding a trailing slash when missing), closely mirroring how `go-github` lets you set a custom endpoint. No `/api/v3/` suffix is added. Use it for **GitHub Enterprise Cloud (GHEC) with data residency** (`https://api.SUBDOMAIN.ghe.com/`) or to point the client at an `httptest` server in your tests. + +```go +// GitHub Enterprise Server (GHES) +installationTokenSource := githubauth.NewInstallationTokenSource( + installationID, + appTokenSource, + githubauth.WithEnterpriseURL("https://github.example.com"), +) + +// GitHub Enterprise Cloud (GHEC) with data residency +installationTokenSource = githubauth.NewInstallationTokenSource( + installationID, + appTokenSource, + githubauth.WithBaseURL("https://api.octocorp.ghe.com"), +) + +// Point at an httptest server in tests +installationTokenSource = githubauth.NewInstallationTokenSource( + installationID, + appTokenSource, + githubauth.WithBaseURL(server.URL), +) +``` + +Option order does not matter — `WithBaseURL`, `WithEnterpriseURL`, `WithHTTPClient`, and `WithRetryOnThrottle` can be combined in any order. If the provided URL cannot be parsed (or a `nil` HTTP client is passed), the misconfiguration is reported by the first call to `Token()` rather than silently falling back to the public GitHub API. + ### Proactive Token Refresh `oauth2.ReuseTokenSource` only refreshes a cached token *after* its expiry has passed. A request that starts at `T-100ms` with a token expiring at `T` can arrive at GitHub with an already-expired credential and receive a 401 that the caller must manually retry. This is especially painful with short application-token windows (default 10 min, optionally lower). diff --git a/auth.go b/auth.go index b1c677c..a2b604c 100644 --- a/auth.go +++ b/auth.go @@ -273,29 +273,69 @@ func WithInstallationTokenOptions(opts *InstallationTokenOptions) InstallationTo } } -// WithHTTPClient sets the HTTP client for the GitHub App installation token source. +// 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. +// +// A nil client is a configuration error reported by the first call to Token(). func WithHTTPClient(client *http.Client) InstallationTokenSourceOpt { return func(i *installationTokenSource) { - client.Transport = &oauth2.Transport{ + if client == nil { + i.setConfigErr(errors.New("WithHTTPClient: http client must not be nil")) + return + } + + // Work on a shallow copy so the caller's *http.Client is not mutated; + // only the Transport is swapped to inject GitHub App authentication. + authClient := *client + authClient.Transport = &oauth2.Transport{ Source: i.src, Base: client.Transport, } - i.client = newGitHubClient(client) + // Swap only the underlying *http.Client so the base URL and retry + // settings already configured on i.client survive, regardless of the + // order options are applied in. + i.client.httpClient = &authClient } } -// WithEnterpriseURL sets the base URL for GitHub Enterprise Server. -// This option should be used after WithHTTPClient to ensure the HTTP client is properly configured. -// If the provided base URL is invalid, the option is ignored and default GitHub base URL is used. +// WithEnterpriseURL sets the base URL for GitHub Enterprise Server (GHES). The +// URL is normalized the way GHES expects, appending the "/api/v3/" path when the +// 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. func WithEnterpriseURL(baseURL string) InstallationTokenSourceOpt { return func(i *installationTokenSource) { - enterpriseClient, err := i.client.withEnterpriseURL(baseURL) - if err != nil { - return + if _, err := i.client.withEnterpriseURL(baseURL); err != nil { + i.setConfigErr(err) } + } +} - i.client = enterpriseClient +// WithBaseURL sets the API base URL used to create installation tokens, closely +// mirroring how go-github lets callers point the client at a custom endpoint. +// Unlike WithEnterpriseURL, the URL is used verbatim — only a trailing slash is +// appended when missing, and no "/api/v3/" suffix is added. +// +// Use this for: +// - 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 +// parsed, the error is reported by the first call to Token() rather than +// silently falling back to the public GitHub API. +func WithBaseURL(baseURL string) InstallationTokenSourceOpt { + return func(i *installationTokenSource) { + if _, err := i.client.withBaseURL(baseURL); err != nil { + i.setConfigErr(err) + } } } @@ -347,6 +387,20 @@ type installationTokenSource struct { client *githubClient opts *InstallationTokenOptions skew time.Duration + + // configErr records the first invalid-configuration error encountered while + // applying options (e.g. an unparseable base URL or a nil HTTP client). It is + // surfaced by Token() so misconfiguration fails loudly instead of silently + // falling back to the public GitHub API. + configErr error +} + +// setConfigErr records err as the source's configuration error, keeping the +// first error so the reported failure is independent of option order. +func (t *installationTokenSource) setConfigErr(err error) { + if t.configErr == nil { + t.configErr = err + } } // NewInstallationTokenSource creates a GitHub App installation token source. @@ -384,6 +438,10 @@ func NewInstallationTokenSource(id int64, src oauth2.TokenSource, opts ...Instal // Token generates a new GitHub App installation token for authenticating as a GitHub App installation. func (t *installationTokenSource) Token() (*oauth2.Token, error) { + if t.configErr != nil { + return nil, t.configErr + } + token, err := t.client.createInstallationToken(t.ctx, t.id, t.opts) if err != nil { return nil, err diff --git a/auth_test.go b/auth_test.go index 55bd714..19abfc6 100644 --- a/auth_test.go +++ b/auth_test.go @@ -15,6 +15,7 @@ import ( "fmt" "io" "net/http" + "net/http/httptest" "reflect" "strings" "sync" @@ -225,8 +226,10 @@ func TestNewApplicationTokenSourceFromSigner(t *testing.T) { wantErr: true, }, { - name: "non-rsa signer is rejected", - new: func() (oauth2.TokenSource, error) { return NewApplicationTokenSourceFromSigner(int64(42), &stubSigner{pub: edPub}) }, + name: "non-rsa signer is rejected", + new: func() (oauth2.TokenSource, error) { + return NewApplicationTokenSourceFromSigner(int64(42), &stubSigner{pub: edPub}) + }, wantErr: true, }, { @@ -472,21 +475,179 @@ func TestWithEnterpriseURL_InvalidURL(t *testing.T) { t.Fatal(err) } - // Test with invalid URL - error is silently ignored in WithEnterpriseURL + // An invalid URL must not silently fall back to the public GitHub API; the + // error surfaces on the first Token() call instead. installationTokenSource := NewInstallationTokenSource( 1, appSrc, WithEnterpriseURL("ht\ntp://invalid"), ) - // The error is silently ignored in WithEnterpriseURL, so this should still work - // but will use the default URL if installationTokenSource == nil { - t.Error("Expected non-nil token source") + t.Fatal("Expected non-nil token source") + } + + if _, err := installationTokenSource.Token(); err == nil { + t.Error("Token() error = nil, want error for invalid enterprise URL") + } +} + +func TestWithBaseURL_InvalidURL(t *testing.T) { + privateKey, err := generatePrivateKey() + if err != nil { + t.Fatal(err) + } + + appSrc, err := NewApplicationTokenSource(int64(12345), privateKey) + if err != nil { + t.Fatal(err) + } + + // An invalid URL must not silently fall back to the public GitHub API; the + // error surfaces on the first Token() call instead. + installationTokenSource := NewInstallationTokenSource( + 1, + appSrc, + WithBaseURL("ht\ntp://invalid"), + ) + + if installationTokenSource == nil { + t.Fatal("Expected non-nil token source") } - // Test that the token source is created successfully - // The error is silently ignored, so the source uses the default URL + if _, err := installationTokenSource.Token(); err == nil { + t.Error("Token() error = nil, want error for invalid base URL") + } +} + +func TestWithBaseURL_DrivesTokenThroughServer(t *testing.T) { + now := time.Now().UTC() + expiration := now.Add(10 * time.Minute) + + // A bare httptest server URL (e.g. http://127.0.0.1:PORT) is exactly the + // case WithEnterpriseURL mishandles: it would append "/api/v3/" and break + // the mock. WithBaseURL uses the URL verbatim, so the installation-token + // POST lands on the expected path. + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/app/installations/1/access_tokens" { + t.Errorf("unexpected request path = %q", r.URL.Path) + w.WriteHeader(http.StatusNotFound) + return + } + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(InstallationToken{ + Token: "mocked-installation-token", + ExpiresAt: expiration, + }) + })) + defer server.Close() + + privateKey, err := generatePrivateKey() + if err != nil { + t.Fatal(err) + } + + appSrc, err := NewApplicationTokenSource(int64(34434), privateKey) + if err != nil { + t.Fatal(err) + } + + ts := NewInstallationTokenSource(1, appSrc, WithBaseURL(server.URL)) + + got, err := ts.Token() + if err != nil { + t.Fatalf("Token() error = %v", err) + } + + want := &oauth2.Token{ + AccessToken: "mocked-installation-token", + TokenType: "Bearer", + Expiry: expiration, + } + if !reflect.DeepEqual(got, want) { + t.Errorf("Token() = %v, want %v", got, want) + } +} + +// 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 +// public GitHub API instead of the test server. +func TestWithBaseURL_SurvivesHTTPClientOrder(t *testing.T) { + now := time.Now().UTC() + expiration := now.Add(10 * time.Minute) + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/app/installations/7/access_tokens" { + t.Errorf("request path = %q, want /app/installations/7/access_tokens", r.URL.Path) + w.WriteHeader(http.StatusNotFound) + return + } + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(InstallationToken{Token: "ordered-token", ExpiresAt: expiration}) + })) + defer server.Close() + + privateKey, err := generatePrivateKey() + if err != nil { + t.Fatal(err) + } + appSrc, err := NewApplicationTokenSource(int64(1), privateKey) + if err != nil { + t.Fatal(err) + } + + ts := NewInstallationTokenSource(7, appSrc, + WithBaseURL(server.URL), + WithHTTPClient(&http.Client{}), + ) + + got, err := ts.Token() + if err != nil { + t.Fatalf("Token() error = %v", err) + } + if got.AccessToken != "ordered-token" { + t.Errorf("AccessToken = %q, want %q", got.AccessToken, "ordered-token") + } +} + +func TestWithHTTPClient_NilClient(t *testing.T) { + privateKey, err := generatePrivateKey() + if err != nil { + t.Fatal(err) + } + appSrc, err := NewApplicationTokenSource(int64(1), privateKey) + if err != nil { + t.Fatal(err) + } + + // A nil HTTP client must not panic; the error surfaces on Token(). + ts := NewInstallationTokenSource(1, appSrc, WithHTTPClient(nil)) + if _, err := ts.Token(); err == nil { + t.Error("Token() error = nil, want error for nil http client") + } +} + +func TestWithHTTPClient_DoesNotMutateCallerClient(t *testing.T) { + privateKey, err := generatePrivateKey() + if err != nil { + t.Fatal(err) + } + appSrc, err := NewApplicationTokenSource(int64(1), privateKey) + if err != nil { + t.Fatal(err) + } + + originalTransport := http.DefaultTransport + client := &http.Client{Transport: originalTransport} + + _ = NewInstallationTokenSource(1, appSrc, WithHTTPClient(client)) + + // The option must operate on a copy; the caller's client (which may be + // shared elsewhere) must keep its original transport. + if client.Transport != originalTransport { + t.Errorf("WithHTTPClient mutated the caller's client Transport = %T, want it unchanged", client.Transport) + } } func Test_installationTokenSource_Token(t *testing.T) { @@ -1035,9 +1196,9 @@ func TestWithExpirySkew_Wiring(t *testing.T) { } tests := []struct { - name string - skewOpts []ApplicationTokenOpt - wantSkew bool // true → *reuseTokenSourceWithSkew, false → delegated + name string + skewOpts []ApplicationTokenOpt + wantSkew bool // true → *reuseTokenSourceWithSkew, false → delegated }{ { name: "default skew uses wrapper", diff --git a/github.go b/github.go index 739226a..7bc35fc 100644 --- a/github.go +++ b/github.go @@ -96,7 +96,7 @@ type Repository struct { // githubClient is a simple GitHub API client for creating installation tokens. type githubClient struct { baseURL *url.URL - client *http.Client + httpClient *http.Client retryOnThrottle bool } @@ -106,7 +106,7 @@ func newGitHubClient(httpClient *http.Client) *githubClient { return &githubClient{ baseURL: baseURL, - client: httpClient, + httpClient: httpClient, retryOnThrottle: true, } } @@ -133,6 +133,25 @@ func (c *githubClient) withEnterpriseURL(baseURL string) (*githubClient, error) return c, 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) { + base, err := url.Parse(baseURL) + if err != nil { + return nil, fmt.Errorf("failed to parse base URL: %w", err) + } + + if !strings.HasSuffix(base.Path, "/") { + base.Path += "/" + } + + c.baseURL = base + + return c, nil +} + // createInstallationToken creates an installation access token for a GitHub App. // When retryOnThrottle is enabled, a single retry is performed on 429 or on 403 // responses that carry Retry-After / x-ratelimit-reset headers. The sleep is @@ -188,7 +207,7 @@ func (c *githubClient) doCreateInstallationToken(ctx context.Context, reqURL str req.Header.Set("Accept", "application/vnd.github+json") req.Header.Set("Content-Type", "application/json") - resp, err := c.client.Do(req) + resp, err := c.httpClient.Do(req) if err != nil { return nil, 0, fmt.Errorf("failed to execute request: %w", err) } diff --git a/github_server_test.go b/github_server_test.go index be6c4bc..b628f88 100644 --- a/github_server_test.go +++ b/github_server_test.go @@ -89,9 +89,12 @@ func (t *mockTransport) RoundTrip(req *http.Request) (*http.Response, error) { // 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 + // 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 /app/installations/") && + return strings.HasPrefix(request, "POST ") && + strings.Contains(request, "/app/installations/") && strings.HasSuffix(request, "/access_tokens") } diff --git a/github_test.go b/github_test.go index b85c22d..5ec4007 100644 --- a/github_test.go +++ b/github_test.go @@ -100,6 +100,73 @@ 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: "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 with trailing slash", + baseURL: "https://api.octocorp.ghe.com/", + wantErr: false, + expectedBaseURL: "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: "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: "httptest-style host and port used verbatim", + baseURL: "http://127.0.0.1:8080", + wantErr: false, + expectedBaseURL: "http://127.0.0.1:8080/", + }, + { + name: "invalid URL with control characters", + baseURL: "ht\ntp://invalid", + wantErr: true, + expectedBaseURL: "", + }, + { + name: "URL with spaces", + baseURL: "http://invalid url with spaces", + wantErr: true, + expectedBaseURL: "", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + client := newGitHubClient(&http.Client{}) + githubClient, err := client.withBaseURL(tt.baseURL) + + if (err != nil) != tt.wantErr { + t.Errorf("withBaseURL(%v) error = %v", tt.baseURL, err) + } + + if err == nil && githubClient.baseURL.String() != tt.expectedBaseURL { + t.Errorf("withBaseURL(%v) expected = %v, received = %v", tt.baseURL, tt.expectedBaseURL, githubClient.baseURL) + } + }) + } +} + func Test_githubClient_createInstallationToken_ErrorCases(t *testing.T) { tests := []struct { name string @@ -291,8 +358,8 @@ func Test_createInstallationToken_ErrorPaths(t *testing.T) { t.Run("error parsing endpoint URL", func(t *testing.T) { // Create a client with an invalid base URL that will cause Parse to fail client := &githubClient{ - baseURL: &url.URL{Scheme: "http", Host: "example.com", Path: ":::invalid"}, - client: &http.Client{}, + baseURL: &url.URL{Scheme: "http", Host: "example.com", Path: ":::invalid"}, + httpClient: &http.Client{}, } _, err := client.createInstallationToken(context.Background(), 12345, nil) diff --git a/http.go b/http.go index f881654..6fd2c95 100644 --- a/http.go +++ b/http.go @@ -17,7 +17,6 @@ func cleanHTTPClient() *http.Client { DialContext: (&net.Dialer{ Timeout: 30 * time.Second, KeepAlive: 30 * time.Second, - DualStack: true, }).DialContext, MaxIdleConns: 100, IdleConnTimeout: 90 * time.Second,