Merge pull request #920 from roidelapluie/roidelapluie/cross-host-sticky

config: make isCrossHostRedirect sticky across the redirect chain

Author: roidelapluie

config/http_config.go

@@ -1135,18 +1135,29 @@ func mapToValues(m map[string]string) url.Values {
 	return v
 }
 
-// isCrossHostRedirect reports whether req is a redirect to a different host
-// than the original request. It detects this by walking the req.Response chain
-// (which Go's HTTP client populates on every redirect hop) to find the original
-// request's hostname, then comparing it to the current destination.
+// isCrossHostRedirect reports whether req is a redirect that has left the
+// original request's host at any point in the chain. It detects this by walking
+// the req.Response chain (which Go's HTTP client populates on every redirect
+// hop) to find the original request's hostname, then checking every hop in the
+// chain against it.
+//
+// The decision is sticky, mirroring net/http: once any hop leaves the original
+// host's domain, credentials and sensitive headers stay stripped for the rest
+// of the chain, even if a later hop redirects back to the original host.
+//
 // This works regardless of whether the caller uses NewClientFromConfig or a
 // custom http.Client built from NewRoundTripperFromConfigWithContext directly.
 func isCrossHostRedirect(req *http.Request) bool {
 	if req.Response == nil {
 		return false
 	}
 	originalHost := strings.ToLower(originalRequestHost(req))
-	return !isDomainOrSubdomain(strings.ToLower(req.URL.Hostname()), originalHost)
+	for r := req; r.Response != nil && r.Response.Request != nil; r = r.Response.Request {
+		if !isDomainOrSubdomain(strings.ToLower(r.URL.Hostname()), originalHost) {
+			return true
+		}
+	}
+	return false
 }
 
 func originalRequestHost(req *http.Request) string {
@@ -1158,8 +1169,10 @@ func originalRequestHost(req *http.Request) string {
 }
 
 // sensitiveHeadersOnRedirect lists the headers that must not be forwarded when
-// following a redirect to a different host, mirroring the list in
-// makeHeadersCopier in net/http/client.go.
+// following a redirect to a different host. The first four entries match the
+// list stripped by makeHeadersCopier in net/http/client.go; we additionally
+// strip the Proxy-* headers, which net/http does not, to avoid leaking proxy
+// credentials to an untrusted host.
 var sensitiveHeadersOnRedirect = map[string]struct{}{
 	"Authorization": {},
 	// "Www-Authenticate" is the canonical form produced by

config/http_config_test.go

@@ -1509,6 +1509,53 @@ func TestSameHostRedirectKeepsCredentials(t *testing.T) {
 	require.Truef(t, credsSeen, "credentials should be forwarded on same-host redirect")
 }
 
+func TestRedirectBackToOriginalHostKeepsCredentialsStripped(t *testing.T) {
+	// Mirror net/http's sticky behaviour: once a redirect chain leaves the
+	// original host, credentials must stay stripped for the rest of the chain,
+	// even when a later hop redirects back to the original host.
+	credsSeen := false
+	final := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if r.Header.Get("Authorization") != "" {
+			credsSeen = true
+		}
+		fmt.Fprint(w, ExpectedMessage)
+	}))
+	t.Cleanup(final.Close)
+
+	// middle listens on 127.0.0.1 but is reached via "localhost", so the hop
+	// from the origin (127.0.0.1) to middle is cross-host. It then redirects
+	// back to final, which shares the original "127.0.0.1" hostname.
+	middle := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		http.Redirect(w, r, final.URL, http.StatusFound)
+	}))
+	t.Cleanup(middle.Close)
+	middlePort := middle.Listener.Addr().(*net.TCPAddr).Port
+	middleLocalhostURL := fmt.Sprintf("http://localhost:%d", middlePort)
+
+	origin := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		http.Redirect(w, r, middleLocalhostURL, http.StatusFound)
+	}))
+	t.Cleanup(origin.Close)
+
+	cfg := HTTPClientConfig{
+		FollowRedirects: true,
+		Authorization: &Authorization{
+			Type:        "Bearer",
+			Credentials: "secret-token",
+		},
+	}
+	client, err := NewClientFromConfig(cfg, "test")
+	require.NoError(t, err)
+
+	resp, err := client.Get(origin.URL)
+	require.NoError(t, err)
+	defer resp.Body.Close()
+	_, err = io.ReadAll(resp.Body)
+	require.NoError(t, err)
+
+	require.Falsef(t, credsSeen, "credentials must stay stripped after leaving the original host, even when redirected back to it")
+}
+
 func TestRoundTripperCrossHostRedirectDropsCredentials(t *testing.T) {
 	// Verify that a custom http.Client built from NewRoundTripperFromConfig
 	// (not NewClientFromConfig) also strips credentials on cross-host redirects,