Apply paired review changes

Author: williammartin

pkg/api/client_options.go

@@ -16,16 +16,18 @@ 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
+	// sent to instead of the default 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.
+	// If AuthToken is not provided, the Host will be used for token lookup,
+	// and requests to APIHost will be allowed to include that token.
+	//
+	// 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
@@ -40,9 +42,8 @@ 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.
+	// CheckRedirect specifies the policy for handling redirects.
+	// If nil, the default http.Client CheckRedirect policy is used.
 	//
 	// This matters for requests where following a redirect silently changes
 	// the meaning of the request. For example, Go's default policy converts a
@@ -140,25 +141,33 @@ func resolveAPIHost(opts ClientOptions) (ClientOptions, error) {
 		opts.APIHost = configuredAPIHost
 	}
 
-	if !validAPIHost(opts.APIHost) {
+	if err := validAPIHost(opts.APIHost); err != nil {
 		return ClientOptions{}, fmt.Errorf(
-			`invalid api_host for %s: %q must be a hostname without a scheme or port, for example "api.example.com"`,
+			`invalid api_host for %s: %v`,
 			opts.Host,
-			opts.APIHost,
+			err,
 		)
 	}
 
 	return opts, nil
 }
 
-func validAPIHost(apiHost string) bool {
+func validAPIHost(apiHost string) error {
+	invalidHostnameError := fmt.Errorf(`%q must be a hostname without a scheme or port, for example "api.example.com"`, apiHost)
+
 	// A bare hostname has no surrounding whitespace, and no port or IPv6 literal.
 	if apiHost == "" || strings.TrimSpace(apiHost) != apiHost || strings.Contains(apiHost, ":") {
-		return false
+		return invalidHostnameError
 	}
 
 	// 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() != ""
+	if err != nil {
+		return invalidHostnameError
+	}
+	if u.Host != apiHost || u.Hostname() == "" {
+		return invalidHostnameError
+	}
+	return nil
 }

pkg/api/client_options_test.go

@@ -162,29 +162,33 @@ func TestValidAPIHost(t *testing.T) {
 	}{
 		{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"},
+		{name: "empty", value: "", valid: false},
+		{name: "scheme", value: "https://gw.example.net", valid: false},
+		{name: "path", value: "gw.example.net/api", valid: false},
+		{name: "query", value: "gw.example.net?trace=1", valid: false},
+		{name: "fragment", value: "gw.example.net#fragment", valid: false},
+		{name: "userinfo", value: "user@gw.example.net", valid: false},
+		{name: "leading whitespace", value: " gw.example.net", valid: false},
+		{name: "trailing whitespace", value: "gw.example.net ", valid: false},
+		{name: "empty host", value: ":8443", valid: false},
+		{name: "missing port", value: "gw.example.net:", valid: false},
+		{name: "non-numeric port", value: "gw.example.net:http", valid: false},
+		{name: "minimum port", value: "gw.example.net:1", valid: false},
+		{name: "typical port", value: "gw.example.net:8443", valid: false},
+		{name: "maximum port", value: "gw.example.net:65535", valid: false},
+		{name: "zero port", value: "gw.example.net:0", valid: false},
+		{name: "out-of-range port", value: "gw.example.net:65536", valid: false},
+		{name: "IPv6 literal", value: "[::1]", valid: false},
+		{name: "bare IPv6 address", value: "::1", valid: false},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			assert.Equal(t, tt.valid, validAPIHost(tt.value))
+			if tt.valid {
+				require.NoError(t, validAPIHost(tt.value))
+			} else {
+				require.Error(t, validAPIHost(tt.value))
+			}
 		})
 	}
 }

pkg/api/http_client.go

@@ -45,11 +45,11 @@ func DefaultHTTPClient() (*http.Client, error) {
 // As part of the configuration a hostname, auth token, default set of headers,
 // and unix domain socket are resolved from the gh environment configuration.
 // These behaviors can be overridden using the opts argument. In this instance
-// providing opts.Host will not change the destination of your request as it is
-// the responsibility of the consumer to configure this. However, if opts.Host
-// does not match the request host, the auth token will not be added to the headers.
-// This is to protect against the case where tokens could be sent to an arbitrary
-// host.
+// providing opts.Host or opts.APIHost will not change the destination of your
+// request, as it is the responsibility of the consumer to configure this. When
+// opts.APIHost is configured, the auth token is only added to requests targeting
+// that exact host. Otherwise, it is only added to requests targeting opts.Host or
+// one of its subdomains. This prevents tokens from being sent to arbitrary hosts.
 func NewHTTPClient(opts ClientOptions) (*http.Client, error) {
 	var err error
 	if optionsNeedResolution(opts) {
@@ -139,12 +139,6 @@ func isSameDomain(requestHost, domain string) bool {
 	return (requestHost == domain) || strings.HasSuffix(requestHost, "."+domain)
 }
 
-// isAPIHost reports whether requestHost is the configured API host override.
-// An unset override matches nothing, including an empty request host.
-func isAPIHost(requestHost, apiHost string) bool {
-	return apiHost != "" && strings.EqualFold(requestHost, apiHost)
-}
-
 // swapHost returns rawURL with its host replaced by apiHost.
 func swapHost(rawURL, apiHost string) string {
 	if apiHost == "" {
@@ -220,14 +214,18 @@ func newHeaderRoundTripper(host string, apiHost string, authToken string, header
 
 func (hrt headerRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) {
 	for k, v := range hrt.headers {
-		// If the authorization header has been set and the request
-		// host is not in the same domain that was specified in the ClientOptions
-		// then do not add the authorization header to the request.
+		// If the default headers include an authorization header, only add it when
+		// the request targets the configured API host. When no API host is configured,
+		// allow the canonical host and its subdomains.
 		requestHost := req.URL.Hostname()
-		if k == authorization &&
-			!isSameDomain(requestHost, hrt.host) &&
-			!isAPIHost(requestHost, hrt.apiHost) {
-			continue
+		if k == authorization {
+			if hrt.apiHost != "" && !strings.EqualFold(requestHost, hrt.apiHost) {
+				continue
+			}
+
+			if hrt.apiHost == "" && !isSameDomain(requestHost, hrt.host) {
+				continue
+			}
 		}
 
 		// If the header is already set in the request, don't overwrite it.

pkg/api/http_client_test.go

@@ -32,46 +32,6 @@ func TestHTTPClient(t *testing.T) {
 	assert.Equal(t, 200, res.StatusCode)
 }
 
-func TestIsAPIHost(t *testing.T) {
-	tests := []struct {
-		name        string
-		requestHost string
-		apiHost     string
-		want        bool
-	}{
-		{
-			name:        "matches exact host",
-			requestHost: "gateway.example",
-			apiHost:     "gateway.example",
-			want:        true,
-		},
-		{
-			name:        "matches host ignoring case",
-			requestHost: "GATEWAY.example",
-			apiHost:     "gateway.example",
-			want:        true,
-		},
-		{
-			name:        "unset override matches nothing",
-			requestHost: "",
-			apiHost:     "",
-			want:        false,
-		},
-		{
-			name:        "empty request host does not match configured override",
-			requestHost: "",
-			apiHost:     "gateway.example",
-			want:        false,
-		},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			assert.Equal(t, tt.want, isAPIHost(tt.requestHost, tt.apiHost))
-		})
-	}
-}
-
 func TestNewHTTPClient(t *testing.T) {
 	testutils.StubConfig(t, "")
 
@@ -180,6 +140,17 @@ func TestNewHTTPClient(t *testing.T) {
 			reqURL:      "https://GATEWAY.example",
 			wantHeaders: defaultHeaders(),
 		},
+		{
+			name:    "withholds authorization from canonical host when API host is configured",
+			host:    "test.com",
+			apiHost: "gateway.example",
+			reqURL:  "https://test.com",
+			wantHeaders: func() http.Header {
+				h := defaultHeaders()
+				h.Del(authorization)
+				return h
+			}(),
+		},
 		{
 			name:    "withholds authorization from an API host subdomain",
 			host:    "test.com",