Add per-host API endpoint overrides and a configurable redirect policy

[williammartin]

Related: cli/cli#13717, cli/cli#13991

Description

GitHub CLI derives REST and GraphQL endpoints from the authenticated GitHub host. Organizations that require API traffic to pass through a corporate gateway currently cannot configure that routing in gh or extensions built on go-gh.

This adds a per-host api_host setting in hosts.yml and an exported ClientOptions.APIHost override. Authentication and token selection still use the canonical GitHub host; only the host of endpoints constructed by go-gh changes. Canonical schemes and paths are preserved, including GHES /api/v3/ and /api/graphql paths and github.localhost HTTP endpoints.

All three client constructors resolve the override even when callers already supply Host, AuthToken, and Transport. The REST and GraphQL constructors swap the host only after deriving their canonical endpoints. REST absolute URLs supplied by callers or servers remain unchanged. Authorization is permitted only for the canonical domain or one exact configured API host.

The second change here is a CheckRedirect field on ClientOptions, matching the field of the same name on http.Client. NewHTTPClient built its client without it, so a caller had no way to control redirect handling. The policy matters because Go's default converts a DELETE into a GET when it follows a 301, so deleting a repository that has been renamed reports success having deleted nothing. Behaviour is unchanged when CheckRedirect is nil.

How did you test this change?

See all the test harnessing on cli/cli#14104

Key points

• api_host accepts a bare hostname only. It intentionally rejects ports, schemes, and path prefixes, so the mapping is direct and canonical endpoint layout remains authoritative.
• A non-empty ClientOptions.APIHost takes precedence over hosts.yml; environment-variable configuration is intentionally out of scope.
• Server-provided absolute URLs are not rewritten. A gateway is responsible for rewriting pagination or asset URLs when those requests must remain on its route.
• Config-read failures are treated as no configured API host, preserving existing behavior for callers that fully specify client options. A successfully read but invalid api_host still fails client construction.
• CheckRedirect is a general field mirroring http.Client rather than a narrow "do not follow redirects" flag. go-gh is a library and the policy is the caller's to choose; cli/cli wraps it in a narrow option of its own, where exactly one use case is known.
• Strictly, a caller can honour api_host without this field by resolving the API host itself and continuing to build its own client. cli/cli does this in one place, gh api, which needs to set Content-Length for --input uploads and so cannot use the shared request path. That is an escape hatch rather than a pattern: it duplicates resolution and leaves each call site able to forget.

Notes for reviewers

None

Authorship and follow-up

Who wrote this:

• A human wrote it.
• An agent wrote it under close human direction.
• An agent wrote it independently, and no human has guided the implementation beyond the initial prompt.

Who answers review comments:

• @williammartin will read and reply directly. Name the account.
• An agent will draft replies and @username will read them before they are posted.
• Nobody has explicitly committed to replying.

[Copilot]

Pull request overview

Adds support for per-host API endpoint overrides in go-gh so REST/GraphQL requests can be routed through a corporate gateway while authentication and token selection remain tied to the canonical GitHub host.

Changes:

• Introduces ClientOptions.APIHost and hosts.yml api_host lookup/validation to override only the API request destination host.
• Updates HTTP auth-header behavior to allow Authorization on the canonical domain or the configured API host override.
• Adds end-to-end acceptance tests (with a real reverse-proxy gateway) plus focused unit tests covering endpoint derivation and authorization boundaries.

Show a summary per file

File	Description
pkg/api/client_options.go	Adds APIHost option plus config-based resolution and hostname-only validation.
pkg/api/client_options_test.go	Adds tests for API host validation, precedence, constructor behavior, and config read error handling.
pkg/api/host.go	Adds config lookup helper for per-host api_host in hosts.yml.
pkg/api/host_test.go	Adds unit coverage for hosts.yml api_host lookup and normalization.
pkg/api/http_client.go	Extends Authorization header allowlist logic to include exact configured API host; adds host-swapping helper.
pkg/api/http_client_test.go	Adds coverage for API host allowlist behavior (including empty-host URL edge cases).
pkg/api/rest_client.go	Switches REST client to store a resolved endpoint (canonical + optional API host override) and uses it to build request URLs.
pkg/api/rest_client_test.go	Stabilizes tests by stubbing config; adds coverage for API-host endpoint swapping and URL joining behavior.
pkg/api/graphql_client.go	Applies API host override after deriving canonical GraphQL endpoint; ensures options are resolved consistently.
pkg/api/graphql_client_test.go	Stabilizes tests by stubbing config; adds coverage for API-host endpoint swapping.
pkg/api/cache_test.go	Stubs config to keep cache tests isolated from API-host config lookups.
pkg/api/api_host_test.go	Adds acceptance tests verifying routing through a gateway for HTTP/REST/GraphQL plus pagination/direct-URL behaviors.

Review details

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

• Files reviewed: 12/12 changed files
• Comments generated: 2
• Review effort level: Lite

[babakks]

Thanks for putting this together, @williammartin! 🙏

I reviewed the whole change set, and overall the design looks solid to me. The separation between authentication (keyed on Host) and routing (keyed on APIHost), the decision to never rewrite absolute URLs, and the exact-match token boundary for the override host all feel well reasoned. I didn't find any correctness or security issues with the system itself.

I left a handful of inline comments, mostly naming, godoc, and test-coverage nitpicks, plus a couple of small refactor suggestions.

The one thing I'd like to see landed is a test asserting that the token is withheld when a request is redirected to a subdomain of the configured api_host. Right now the suite proves that canonical-host subdomains receive the token and that unrelated third parties don't, but the "exact match, no parent-domain pass" guarantee for api_host is only covered indirectly. I've added an inline suggestion with a passing patch that wires this case in, so it should be a quick add.

[babakks]

nitpick: this error message is about the logic in the validAPIHost function, so it's best to move the message there and change the validAPIHost signature to return an error instead of a boolean. Then this call site will be changed to:

if err := validAPIHost(opts.APIHost); err != nil {
	return ClientOptions{}, fmt.Errorf("invalid api_host for %s: %w", opts.Host, err)
}

[babakks]

To further reinforce this, we need the same test cases for the configuration path:

Suggested change

	{
	name: "invalid explicit option fails",
	host: "example.ghe.com",
	apiHost: "https://explicit.example.net",
	wantErr: `invalid api_host for example.ghe.com: "https://explicit.example.net" must be a hostname without a scheme or port, for example "api.example.com"`,
	},
	{
	name: "explicit port fails",
	host: "example.ghe.com",
	apiHost: "explicit.example.net:8443",
	wantErr: `invalid api_host for example.ghe.com: "explicit.example.net:8443" must be a hostname without a scheme or port, for example "api.example.com"`,
	},
	{
	name: "invalid explicit option fails",
	host: "example.ghe.com",
	apiHost: "https://explicit.example.net",
	wantErr: `invalid api_host for example.ghe.com: "https://explicit.example.net" must be a hostname without a scheme or port, for example "api.example.com"`,
	},
	{
	name: "explicit port fails",
	host: "example.ghe.com",
	apiHost: "explicit.example.net:8443",
	wantErr: `invalid api_host for example.ghe.com: "explicit.example.net:8443" must be a hostname without a scheme or port, for example "api.example.com"`,
	},
	{
	name: "invalid configured option fails",
	host: "example.ghe.com",
	config: "hosts:\n example.ghe.com:\n api_host: https://configured.example.net\n",
	wantErr: `invalid api_host for example.ghe.com: "https://configured.example.net" must be a hostname without a scheme or port, for example "api.example.com"`,
	},
	{
	name: "configured port fails",
	host: "example.ghe.com",
	config: "hosts:\n example.ghe.com:\n api_host: configured.example.net:8443\n",
	wantErr: `invalid api_host for example.ghe.com: "configured.example.net:8443" must be a hostname without a scheme or port, for example "api.example.com"`,
	},

[babakks]

Let's use heredoc.Doc for all these test cases.

[babakks]

We should include the API host among these:

Suggested change

	// and unix domain socket are resolved from the gh environment configuration.
	// As part of the configuration a hostname, auth token, default set of headers,
	// API host, and unix domain socket are resolved from the gh environment configuration.

[babakks]

This endpoint field is actually used as a URL prefix. So, I agree that host wasn't a good name, but endpoint is not really clear, too. My suggestion is to rename it to baseURL or even just prefix.

If we do this, we also need to change lots of other semantic references (e.g. in function parameters or tests).

[babakks]

Let's add a time saver comment here:

Suggested change

	endpoint := restPrefix(opts.Host)
	if opts.APIHost != "" {
	endpoint = swapHost(endpoint, opts.APIHost)
	}
	endpoint := restPrefix(opts.Host)
	if opts.APIHost != "" {
	// The endpoint comes with a trailing slash and swapping the host should preserve it.
	endpoint = swapHost(endpoint, opts.APIHost)
	}

[babakks]

It actually makes more sense as endpoint here in the GraphQLClient struct as the API endpoint is always the same for GraphQL.

[babakks]

remark/suggestion: Both NewRESTClient and NewGraphQLClient already call resolveAPIHost on the opts value. The resolveAPIHost function is carefully implemented so that the second call doesn't try reading the config, but the validation part happens twice (obviously, the second time is always a pass).

I think this implies we should take out the validation from resolveAPIHost and do it here as a separate/explicit call. 🤔

[babakks]

We need another test case to confirm the token is not sent when redirecting to subdomains of the API host. Here's the diff to add that (and it's passing):

diff --git a/pkg/api/api_host_test.go b/pkg/api/api_host_test.go
index 3b481b3..5687ede 100644
--- a/pkg/api/api_host_test.go
+++ b/pkg/api/api_host_test.go
@@ -113,6 +113,9 @@ func newAPIHostTestHarness(t *testing.T, apiHost string) *apiHostTestHarness {
 		if req.URL.Path == "/redirect-to-third-party" {
 			http.Redirect(w, req, "https://"+thirdPartyHost+"/redirected", http.StatusFound)
 			return
+		} else if req.URL.Path == "/redirect-to-subdomain" {
+			http.Redirect(w, req, "https://subdomain.gw.example.net/redirected", http.StatusFound)
+			return
 		}
 		proxy.ServeHTTP(w, req)
 	}))
@@ -132,11 +135,12 @@ func newAPIHostTestHarness(t *testing.T, apiHost string) *apiHostTestHarness {
 	fakeAddress := fakeGitHub.Listener.Addr().String()
 	gatewayAddress := gateway.Listener.Addr().String()
 	dialMap := map[string]string{
-		"api.github.com:443":      fakeAddress,
-		"api.example.ghe.com:443": fakeAddress,
-		"ghes.example.com:443":    fakeAddress,
-		"gw.example.net:443":      gatewayAddress,
-		thirdPartyHost + ":443":   thirdParty.Listener.Addr().String(),
+		"api.github.com:443":           fakeAddress,
+		"api.example.ghe.com:443":      fakeAddress,
+		"ghes.example.com:443":         fakeAddress,
+		"gw.example.net:443":           gatewayAddress,
+		"subdomain.gw.example.net:443": gatewayAddress,
+		thirdPartyHost + ":443":        thirdParty.Listener.Addr().String(),
 	}
 	harness.transport = &http.Transport{
 		// We must turn off TLS verification because our test servers use self-signed certs.
@@ -357,6 +361,28 @@ func TestAPIHostRouting(t *testing.T) {
 					authorization: "",
 				})
 			})
+
+			t.Run("does not provide token when redirected to a subdomain of the api_host", func(t *testing.T) {
+				harness, opts := newConfiguredAPIHostTest(t, tt.host, apiHost)
+				httpClient, err := NewHTTPClient(opts)
+				require.NoError(t, err)
+
+				response, err := httpClient.Get("https://" + apiHost + "/redirect-to-subdomain")
+				require.NoError(t, err)
+				require.NoError(t, response.Body.Close())
+
+				requireRequest(t, &harness.gatewayRequests, recordedRequest{
+					method:        http.MethodGet,
+					path:          "/redirect-to-subdomain",
+					host:          apiHost,
+					authorization: "token test-token",
+				})
+				requireRequest(t, &harness.gatewayRequests, recordedRequest{
+					method: http.MethodGet,
+					path:   "/redirected",
+					host:   "subdomain." + apiHost,
+				})
+			})
 		})
 	}
 

[babakks]

nitpick: to be clear it's not just DELETE:

Suggested change

	// This matters for requests where following a redirect silently changes
	// the meaning of the request. Go's default policy converts a DELETE into a
	// GET when it follows a 301, so a caller deleting a renamed resource can
	// receive a success response having deleted nothing.
	CheckRedirect func(*http.Request, []*http.Request) error
	// This matters for requests where following a redirect silently changes
	// the meaning of the request. For example, Go's default policy converts a DELETE into a
	// GET when it follows a 301, so a caller deleting a renamed resource can
	// receive a success response having deleted nothing.
	CheckRedirect func(*http.Request, []*http.Request) error