diff --git a/pkg/api/api_host_test.go b/pkg/api/api_host_test.go index 48a55e8..a30fae5 100644 --- a/pkg/api/api_host_test.go +++ b/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", }) }) }) diff --git a/pkg/api/client_options.go b/pkg/api/client_options.go index b60b486..6e7e910 100644 --- a/pkg/api/client_options.go +++ b/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 diff --git a/pkg/api/http_client.go b/pkg/api/http_client.go index ec7bc92..4319373 100644 --- a/pkg/api/http_client.go +++ b/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 } } diff --git a/pkg/api/http_client_test.go b/pkg/api/http_client_test.go index 17da140..557e1c6 100644 --- a/pkg/api/http_client_test.go +++ b/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",