Skip to content

Commit 428856f

Browse files
committed
config: make isCrossHostRedirect sticky across the redirect chain
Walk every hop in the redirect chain rather than only checking the current request's host. Once any hop leaves the original host, the function returns true for all subsequent requests in that chain, even if a later hop redirects back to the original host. This mirrors net/http's own behaviour and closes the bypass window where a cross→original→cross chain could slip credentials through. Signed-off-by: Julien Pivotto <291750+roidelapluie@users.noreply.github.com>
1 parent 0d8de87 commit 428856f

2 files changed

Lines changed: 67 additions & 7 deletions

File tree

config/http_config.go

Lines changed: 20 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1117,18 +1117,29 @@ func mapToValues(m map[string]string) url.Values {
11171117
return v
11181118
}
11191119

1120-
// isCrossHostRedirect reports whether req is a redirect to a different host
1121-
// than the original request. It detects this by walking the req.Response chain
1122-
// (which Go's HTTP client populates on every redirect hop) to find the original
1123-
// request's hostname, then comparing it to the current destination.
1120+
// isCrossHostRedirect reports whether req is a redirect that has left the
1121+
// original request's host at any point in the chain. It detects this by walking
1122+
// the req.Response chain (which Go's HTTP client populates on every redirect
1123+
// hop) to find the original request's hostname, then checking every hop in the
1124+
// chain against it.
1125+
//
1126+
// The decision is sticky, mirroring net/http: once any hop leaves the original
1127+
// host's domain, credentials and sensitive headers stay stripped for the rest
1128+
// of the chain, even if a later hop redirects back to the original host.
1129+
//
11241130
// This works regardless of whether the caller uses NewClientFromConfig or a
11251131
// custom http.Client built from NewRoundTripperFromConfigWithContext directly.
11261132
func isCrossHostRedirect(req *http.Request) bool {
11271133
if req.Response == nil {
11281134
return false
11291135
}
11301136
originalHost := strings.ToLower(originalRequestHost(req))
1131-
return !isDomainOrSubdomain(strings.ToLower(req.URL.Hostname()), originalHost)
1137+
for r := req; r.Response != nil && r.Response.Request != nil; r = r.Response.Request {
1138+
if !isDomainOrSubdomain(strings.ToLower(r.URL.Hostname()), originalHost) {
1139+
return true
1140+
}
1141+
}
1142+
return false
11321143
}
11331144

11341145
func originalRequestHost(req *http.Request) string {
@@ -1140,8 +1151,10 @@ func originalRequestHost(req *http.Request) string {
11401151
}
11411152

11421153
// sensitiveHeadersOnRedirect lists the headers that must not be forwarded when
1143-
// following a redirect to a different host, mirroring the list in
1144-
// makeHeadersCopier in net/http/client.go.
1154+
// following a redirect to a different host. The first four entries match the
1155+
// list stripped by makeHeadersCopier in net/http/client.go; we additionally
1156+
// strip the Proxy-* headers, which net/http does not, to avoid leaking proxy
1157+
// credentials to an untrusted host.
11451158
var sensitiveHeadersOnRedirect = map[string]struct{}{
11461159
"Authorization": {},
11471160
// "Www-Authenticate" is the canonical form produced by

config/http_config_test.go

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1515,6 +1515,53 @@ func TestSameHostRedirectKeepsCredentials(t *testing.T) {
15151515
require.Truef(t, credsSeen, "credentials should be forwarded on same-host redirect")
15161516
}
15171517

1518+
func TestRedirectBackToOriginalHostKeepsCredentialsStripped(t *testing.T) {
1519+
// Mirror net/http's sticky behaviour: once a redirect chain leaves the
1520+
// original host, credentials must stay stripped for the rest of the chain,
1521+
// even when a later hop redirects back to the original host.
1522+
credsSeen := false
1523+
final := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
1524+
if r.Header.Get("Authorization") != "" {
1525+
credsSeen = true
1526+
}
1527+
fmt.Fprint(w, ExpectedMessage)
1528+
}))
1529+
t.Cleanup(final.Close)
1530+
1531+
// middle listens on 127.0.0.1 but is reached via "localhost", so the hop
1532+
// from the origin (127.0.0.1) to middle is cross-host. It then redirects
1533+
// back to final, which shares the original "127.0.0.1" hostname.
1534+
middle := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
1535+
http.Redirect(w, r, final.URL, http.StatusFound)
1536+
}))
1537+
t.Cleanup(middle.Close)
1538+
middlePort := middle.Listener.Addr().(*net.TCPAddr).Port
1539+
middleLocalhostURL := fmt.Sprintf("http://localhost:%d", middlePort)
1540+
1541+
origin := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
1542+
http.Redirect(w, r, middleLocalhostURL, http.StatusFound)
1543+
}))
1544+
t.Cleanup(origin.Close)
1545+
1546+
cfg := HTTPClientConfig{
1547+
FollowRedirects: true,
1548+
Authorization: &Authorization{
1549+
Type: "Bearer",
1550+
Credentials: "secret-token",
1551+
},
1552+
}
1553+
client, err := NewClientFromConfig(cfg, "test")
1554+
require.NoError(t, err)
1555+
1556+
resp, err := client.Get(origin.URL)
1557+
require.NoError(t, err)
1558+
defer resp.Body.Close()
1559+
_, err = io.ReadAll(resp.Body)
1560+
require.NoError(t, err)
1561+
1562+
require.Falsef(t, credsSeen, "credentials must stay stripped after leaving the original host, even when redirected back to it")
1563+
}
1564+
15181565
func TestRoundTripperCrossHostRedirectDropsCredentials(t *testing.T) {
15191566
// Verify that a custom http.Client built from NewRoundTripperFromConfig
15201567
// (not NewClientFromConfig) also strips credentials on cross-host redirects,

0 commit comments

Comments
 (0)