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

pkg/api/cache_test.go

@@ -9,10 +9,13 @@ import (
 	"testing"
 	"time"
 
+	"github.com/cli/go-gh/v2/internal/testutils"
 	"github.com/stretchr/testify/assert"
 )
 
 func TestCacheResponse(t *testing.T) {
+	testutils.StubConfig(t, "")
+
 	counter := 0
 	fakeHTTP := tripper{
 		roundTrip: func(req *http.Request) (*http.Response, error) {
@@ -97,6 +100,8 @@ func TestCacheResponse(t *testing.T) {
 }
 
 func TestCacheResponseRequestCacheOptions(t *testing.T) {
+	testutils.StubConfig(t, "")
+
 	counter := 0
 	fakeHTTP := tripper{
 		roundTrip: func(req *http.Request) (*http.Response, error) {

pkg/api/client_options.go

@@ -5,6 +5,8 @@ import (
 	"fmt"
 	"io"
 	"net/http"
+	"net/url"
+	"strings"
 	"time"
 
 	"github.com/cli/go-gh/v2/pkg/auth"
@@ -13,6 +15,19 @@ import (
 
 // ClientOptions holds available options to configure API clients.
 type ClientOptions struct {
+	// APIHost overrides the hostname that REST and GraphQL API requests are
+	// sent to while authentication continues to use Host. It must be a bare
+	// hostname, without a scheme or port, for example "api.example.com".
+	//
+	// When empty, the api_host value configured for Host in gh config is used,
+	// if there is one. Client construction fails when the resulting value,
+	// whether set here or read from gh config, is not a bare hostname.
+	//
+	// The auth token is sent to APIHost as well as to Host, so it must be a
+	// trusted endpoint. Absolute URLs passed to RESTClient methods are
+	// requested as given and are never rewritten to APIHost.
+	APIHost string
+
 	// AuthToken is the authorization token that will be used
 	// to authenticate against API endpoints.
 	AuthToken string
@@ -25,6 +40,16 @@ type ClientOptions struct {
 	// Default is 24 hours.
 	CacheTTL time.Duration
 
+	// CheckRedirect specifies the policy for handling redirects, matching the
+	// field of the same name on http.Client. If nil, the default policy of
+	// following up to 10 redirects is used.
+	//
+	// 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
+
 	// EnableCache specifies if API requests will be cached or not.
 	// Default is no caching.
 	EnableCache bool
@@ -104,3 +129,36 @@ func resolveOptions(opts ClientOptions) (ClientOptions, error) {
 	}
 	return opts, nil
 }
+
+func resolveAPIHost(opts ClientOptions) (ClientOptions, error) {
+	if opts.APIHost == "" {
+		configuredAPIHost, ok := apiHost(opts.Host)
+		if !ok {
+			return opts, nil
+		}
+
+		opts.APIHost = configuredAPIHost
+	}
+
+	if !validAPIHost(opts.APIHost) {
+		return ClientOptions{}, fmt.Errorf(
+			`invalid api_host for %s: %q must be a hostname without a scheme or port, for example "api.example.com"`,
+			opts.Host,
+			opts.APIHost,
+		)
+	}
+
+	return opts, nil
+}
+
+func validAPIHost(apiHost string) bool {
+	// A bare hostname has no surrounding whitespace, and no port or IPv6 literal.
+	if apiHost == "" || strings.TrimSpace(apiHost) != apiHost || strings.Contains(apiHost, ":") {
+		return false
+	}
+
+	// Parsing as a scheme relative URL rejects userinfo, paths, queries and fragments,
+	// since any of those make the parsed host differ from the input.
+	u, err := url.Parse("//" + apiHost)
+	return err == nil && u.Host == apiHost && u.Hostname() != ""
+}

pkg/api/client_options_test.go

@@ -1,11 +1,14 @@
 package api
 
 import (
+	"errors"
 	"net/http"
 	"testing"
 
 	"github.com/cli/go-gh/v2/internal/testutils"
+	"github.com/cli/go-gh/v2/pkg/config"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestResolveOptions(t *testing.T) {
@@ -150,6 +153,220 @@ func TestOptionsNeedResolution(t *testing.T) {
 	}
 }
 
+func TestValidAPIHost(t *testing.T) {
+	tests := []struct {
+		name  string
+		value string
+		valid bool
+	}{
+		{name: "bare hostname", value: "gw.example.net", valid: true},
+		{name: "mixed-case hostname", value: "GW.Example.NET", valid: true},
+		{name: "empty"},
+		{name: "scheme", value: "https://gw.example.net"},
+		{name: "path", value: "gw.example.net/api"},
+		{name: "query", value: "gw.example.net?trace=1"},
+		{name: "fragment", value: "gw.example.net#fragment"},
+		{name: "userinfo", value: "user@gw.example.net"},
+		{name: "leading whitespace", value: " gw.example.net"},
+		{name: "trailing whitespace", value: "gw.example.net "},
+		{name: "empty host", value: ":8443"},
+		{name: "missing port", value: "gw.example.net:"},
+		{name: "non-numeric port", value: "gw.example.net:http"},
+		{name: "minimum port", value: "gw.example.net:1"},
+		{name: "typical port", value: "gw.example.net:8443"},
+		{name: "maximum port", value: "gw.example.net:65535"},
+		{name: "zero port", value: "gw.example.net:0"},
+		{name: "out-of-range port", value: "gw.example.net:65536"},
+		{name: "IPv6 literal", value: "[::1]"},
+		{name: "bare IPv6 address", value: "::1"},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			assert.Equal(t, tt.valid, validAPIHost(tt.value))
+		})
+	}
+}
+
+func TestResolveAPIHost(t *testing.T) {
+	tests := []struct {
+		name    string
+		host    string
+		apiHost string
+		config  string
+		want    string
+		wantErr string
+	}{
+		{
+			name:    "explicit option takes precedence",
+			host:    "example.ghe.com",
+			apiHost: "explicit.example.net",
+			config:  "hosts:\n  example.ghe.com:\n    api_host: configured.example.net\n",
+			want:    "explicit.example.net",
+		},
+		{
+			name:   "config fills an empty option",
+			host:   "example.ghe.com",
+			config: "hosts:\n  example.ghe.com:\n    api_host: configured.example.net\n",
+			want:   "configured.example.net",
+		},
+		{
+			name: "no option or config stays empty",
+			host: "example.ghe.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"`,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			testutils.StubConfig(t, tt.config)
+			opts := ClientOptions{Host: tt.host, APIHost: tt.apiHost}
+
+			got, err := resolveAPIHost(opts)
+
+			if tt.wantErr != "" {
+				require.EqualError(t, err, tt.wantErr)
+			} else {
+				require.NoError(t, err)
+			}
+			assert.Equal(t, tt.want, got.APIHost)
+		})
+	}
+}
+
+func TestAPIClientConstructorsRejectInvalidAPIHost(t *testing.T) {
+	constructors := []struct {
+		name      string
+		construct func(ClientOptions) error
+	}{
+		{
+			name: "HTTP",
+			construct: func(opts ClientOptions) error {
+				_, err := NewHTTPClient(opts)
+				return err
+			},
+		},
+		{
+			name: "REST",
+			construct: func(opts ClientOptions) error {
+				_, err := NewRESTClient(opts)
+				return err
+			},
+		},
+		{
+			name: "GraphQL",
+			construct: func(opts ClientOptions) error {
+				_, err := NewGraphQLClient(opts)
+				return err
+			},
+		},
+	}
+
+	for _, constructor := range constructors {
+		t.Run(constructor.name+" configured value", func(t *testing.T) {
+			testutils.StubConfig(t, "hosts:\n  example.ghe.com:\n    api_host: gw.example.net:8443\n")
+			opts := ClientOptions{
+				Host:      "example.ghe.com",
+				AuthToken: "token",
+				Transport: http.DefaultTransport,
+			}
+
+			err := constructor.construct(opts)
+
+			require.EqualError(
+				t,
+				err,
+				`invalid api_host for example.ghe.com: "gw.example.net:8443" must be a hostname without a scheme or port, for example "api.example.com"`,
+			)
+		})
+
+		t.Run(constructor.name+" explicit value", func(t *testing.T) {
+			testutils.StubConfig(t, "")
+			opts := ClientOptions{
+				Host:      "example.ghe.com",
+				APIHost:   "gw.example.net:8443",
+				AuthToken: "token",
+				Transport: http.DefaultTransport,
+			}
+
+			err := constructor.construct(opts)
+
+			require.EqualError(
+				t,
+				err,
+				`invalid api_host for example.ghe.com: "gw.example.net:8443" must be a hostname without a scheme or port, for example "api.example.com"`,
+			)
+		})
+	}
+}
+
+func TestAPIClientConstructorsIgnoreConfigReadErrors(t *testing.T) {
+	oldRead := config.Read
+	config.Read = func(*config.Config) (*config.Config, error) {
+		return nil, &config.InvalidConfigFileError{
+			Path: "hosts.yml",
+			Err:  errors.New("invalid YAML"),
+		}
+	}
+	t.Cleanup(func() {
+		config.Read = oldRead
+	})
+
+	opts := ClientOptions{
+		Host:      "example.ghe.com",
+		AuthToken: "token",
+		Transport: http.DefaultTransport,
+	}
+
+	resolved, err := resolveAPIHost(opts)
+	require.NoError(t, err)
+	assert.Equal(t, opts, resolved)
+
+	constructors := []struct {
+		name      string
+		construct func(ClientOptions) error
+	}{
+		{
+			name: "HTTP",
+			construct: func(opts ClientOptions) error {
+				_, err := NewHTTPClient(opts)
+				return err
+			},
+		},
+		{
+			name: "REST",
+			construct: func(opts ClientOptions) error {
+				_, err := NewRESTClient(opts)
+				return err
+			},
+		},
+		{
+			name: "GraphQL",
+			construct: func(opts ClientOptions) error {
+				_, err := NewGraphQLClient(opts)
+				return err
+			},
+		},
+	}
+
+	for _, constructor := range constructors {
+		t.Run(constructor.name, func(t *testing.T) {
+			require.NoError(t, constructor.construct(opts))
+		})
+	}
+}
+
 func testConfig() string {
 	return `
 hosts:

pkg/api/graphql_client.go

@@ -32,20 +32,28 @@ func DefaultGraphQLClient() (*GraphQLClient, error) {
 // and unix domain socket are resolved from the gh environment configuration.
 // These behaviors can be overridden using the opts argument.
 func NewGraphQLClient(opts ClientOptions) (*GraphQLClient, error) {
+	var err error
 	if optionsNeedResolution(opts) {
-		var err error
 		opts, err = resolveOptions(opts)
 		if err != nil {
 			return nil, err
 		}
 	}
 
+	opts, err = resolveAPIHost(opts)
+	if err != nil {
+		return nil, err
+	}
+
 	httpClient, err := NewHTTPClient(opts)
 	if err != nil {
 		return nil, err
 	}
 
 	endpoint := graphQLEndpoint(opts.Host)
+	if opts.APIHost != "" {
+		endpoint = swapHost(endpoint, opts.APIHost)
+	}
 
 	return &GraphQLClient{
 		client:     graphql.NewClient(endpoint, httpClient),