Trust canonical host alongside API host

pkg/api/api_host_test.go

@@ -69,7 +69,7 @@ type apiHostTestHarness struct {
 	thirdPartyRequests requestRecorder
 }
 
-func newAPIHostTestHarness(t *testing.T, apiHost string) *apiHostTestHarness {
+func newAPIHostTestHarness(t *testing.T, host, apiHost string) *apiHostTestHarness {
 	t.Helper()
 
 	harness := &apiHostTestHarness{}
@@ -117,6 +117,12 @@ func newAPIHostTestHarness(t *testing.T, apiHost string) *apiHostTestHarness {
 		case "/redirect-to-subdomain":
 			http.Redirect(w, req, "https://subdomain.gw.example.net/redirected", http.StatusFound)
 			return
+		case "/redirect-to-canonical-host":
+			http.Redirect(w, req, "https://"+host+"/redirected", http.StatusFound)
+			return
+		case "/redirect-to-canonical-subdomain":
+			http.Redirect(w, req, "https://subdomain."+host+"/redirected", http.StatusFound)
+			return
 		}
 		proxy.ServeHTTP(w, req)
 	}))
@@ -141,6 +147,8 @@ func newAPIHostTestHarness(t *testing.T, apiHost string) *apiHostTestHarness {
 		"ghes.example.com:443":         fakeAddress,
 		"gw.example.net:443":           gatewayAddress,
 		"subdomain.gw.example.net:443": gatewayAddress,
+		host + ":443":                  fakeAddress,
+		"subdomain." + host + ":443":   fakeAddress,
 		thirdPartyHost + ":443":        thirdParty.Listener.Addr().String(),
 	}
 	harness.transport = &http.Transport{
@@ -161,7 +169,7 @@ func newAPIHostTestHarness(t *testing.T, apiHost string) *apiHostTestHarness {
 func newConfiguredAPIHostTest(t *testing.T, host, apiHost string) (*apiHostTestHarness, ClientOptions) {
 	t.Helper()
 
-	harness := newAPIHostTestHarness(t, apiHost)
+	harness := newAPIHostTestHarness(t, host, apiHost)
 	testutils.StubConfig(t, fmt.Sprintf("hosts:\n  %s:\n    api_host: %q\n", host, apiHost))
 	return harness, ClientOptions{
 		Host:      host,
@@ -173,7 +181,7 @@ func newConfiguredAPIHostTest(t *testing.T, host, apiHost string) (*apiHostTestH
 func newCanonicalAPIHostTest(t *testing.T, apiHost string) (*apiHostTestHarness, ClientOptions) {
 	t.Helper()
 
-	harness := newAPIHostTestHarness(t, apiHost)
+	harness := newAPIHostTestHarness(t, "github.com", apiHost)
 	testutils.StubConfig(t, "")
 	return harness, ClientOptions{
 		Host:      "github.com",
@@ -379,9 +387,44 @@ func TestAPIHostRouting(t *testing.T) {
 					authorization: "token test-token",
 				})
 				requireRequest(t, &harness.gatewayRequests, recordedRequest{
-					method: http.MethodGet,
-					path:   "/redirected",
-					host:   "subdomain." + apiHost,
+					method:        http.MethodGet,
+					path:          "/redirected",
+					host:          "subdomain." + apiHost,
+					authorization: "",
+				})
+			})
+
+			t.Run("provides token when redirected to the canonical 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-canonical-host")
+				require.NoError(t, err)
+				require.NoError(t, response.Body.Close())
+
+				requireRequest(t, &harness.githubAPIRequests, recordedRequest{
+					method:        http.MethodGet,
+					path:          "/redirected",
+					host:          tt.host,
+					authorization: "token test-token",
+				})
+			})
+
+			t.Run("provides token when redirected to a canonical subdomain", 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-canonical-subdomain")
+				require.NoError(t, err)
+				require.NoError(t, response.Body.Close())
+
+				requireRequest(t, &harness.githubAPIRequests, recordedRequest{
+					method:        http.MethodGet,
+					path:          "/redirected",
+					host:          "subdomain." + tt.host,
+					authorization: "token test-token",
 				})
 			})
 		})

pkg/api/client_options.go

@@ -25,12 +25,12 @@ type ClientOptions struct {
 	//
 	// If AuthToken is not provided, the Host will be used for token lookup,
 	// and requests to APIHost will be allowed to include that token. APIHost
-	// must therefore be a trusted endpoint. When APIHost is configured, the
-	// token is not sent to Host or its subdomains.
+	// must therefore be a trusted endpoint. Configuring APIHost does not prevent
+	// the token from being sent to Host or its subdomains.
 	//
 	// Absolute URLs passed to RESTClient methods are requested as given
-	// and are never rewritten to APIHost. They are authenticated only when
-	// they target APIHost.
+	// and are never rewritten to APIHost. They are authenticated when they
+	// target Host or one of its subdomains, or the exact APIHost.
 	APIHost string
 
 	// AuthToken is the authorization token that will be used

pkg/api/http_client.go

@@ -46,10 +46,10 @@ func DefaultHTTPClient() (*http.Client, error) {
 // 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 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.
+// request, as it is the responsibility of the consumer to configure this. The auth
+// token is only added to requests targeting opts.Host or one of its subdomains, or
+// the exact opts.APIHost when configured. This prevents tokens from being sent to
+// arbitrary hosts.
 func NewHTTPClient(opts ClientOptions) (*http.Client, error) {
 	var err error
 	if optionsNeedResolution(opts) {
@@ -215,15 +215,12 @@ 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 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.
+		// the request targets the canonical host or one of its subdomains, or the
+		// exact configured API host.
 		requestHost := req.URL.Hostname()
 		if k == authorization {
-			if hrt.apiHost != "" && !strings.EqualFold(requestHost, hrt.apiHost) {
-				continue
-			}
-
-			if hrt.apiHost == "" && !isSameDomain(requestHost, hrt.host) {
+			isAPIHost := hrt.apiHost != "" && strings.EqualFold(requestHost, hrt.apiHost)
+			if !isSameDomain(requestHost, hrt.host) && !isAPIHost {
 				continue
 			}
 		}

pkg/api/http_client_test.go

@@ -141,15 +141,18 @@ func TestNewHTTPClient(t *testing.T) {
 			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:        "adds authorization for canonical host when API host is configured",
+			host:        "test.com",
+			apiHost:     "gateway.example",
+			reqURL:      "https://test.com",
+			wantHeaders: defaultHeaders(),
+		},
+		{
+			name:        "adds authorization for canonical subdomain when API host is configured",
+			host:        "test.com",
+			apiHost:     "gateway.example",
+			reqURL:      "https://api.test.com",
+			wantHeaders: defaultHeaders(),
 		},
 		{
 			name:    "withholds authorization from an API host subdomain",