fix(rest): prevent credential and default header leaks on cross-origin redirects

catalog/rest/rest.go

@@ -234,10 +234,21 @@ type sessionTransport struct {
 	authManager    AuthManager
 	defaultHeaders http.Header
 	signer         RequestSigner
-	// signingOrigin is the configured catalog origin. A request to a different
-	// origin (e.g. a redirect hop) is not signed, so the signer's Authorization
-	// header and any session token never reach an unconfigured host.
-	signingOrigin *url.URL
+
+	// builtinHeaders is the subset of defaultHeaders under the built-in keys
+	// (with any operator override applied), which identify the client and
+	// carry no credentials. It is all a request to an origin other than
+	// catalogOrigin or authOrigin (e.g. a redirect hop) receives.
+	builtinHeaders http.Header
+	// catalogOrigin is the configured catalog origin: the only origin that
+	// receives the auth header and the signer's signature. authOrigin is the
+	// configured OAuth token endpoint, if any, which also receives
+	// user-supplied default headers.
+	// These are compared against the request URL rather than derived from
+	// the redirect chain, so a transport that omits Response.Request cannot
+	// widen them. A nil catalogOrigin disables the check.
+	catalogOrigin *url.URL
+	authOrigin    *url.URL
 }
 
 // sameOrigin reports whether two URLs share scheme, host, and effective port.
@@ -262,12 +273,22 @@ func defaultedPort(u *url.URL) string {
 }
 
 func (s *sessionTransport) RoundTrip(r *http.Request) (*http.Response, error) {
+	// net/http strips Authorization on redirect only for a new hostname (a
+	// port or scheme change, or a hop to a subdomain, keeps it), but this
+	// runs after that stripping, so credentials and user-supplied headers are
+	// only applied for the configured origins.
+	toCatalog := s.catalogOrigin == nil || sameOrigin(s.catalogOrigin, r.URL)
+	defaults := s.builtinHeaders
+	if toCatalog || (s.authOrigin != nil && sameOrigin(s.authOrigin, r.URL)) {
+		defaults = s.defaultHeaders
+	}
+
 	// A session default is applied unless the request already carries that
 	// header (a per-request override of any default, not just Content-Type
 	// wins) or explicitly opted out of it via withSuppressedHeaders (carried on
 	// the context as an explicit set, never inferred from header values).
 	suppressed := suppressedHeadersFrom(r.Context())
-	for k, v := range s.defaultHeaders {
+	for k, v := range defaults {
 		ck := http.CanonicalHeaderKey(k)
 		if _, ok := r.Header[ck]; ok {
 			continue
@@ -286,7 +307,7 @@ func (s *sessionTransport) RoundTrip(r *http.Request) (*http.Response, error) {
 	// session default of the same key. A caller cannot suppress or spoof the
 	// Authorization header by supplying its own. Do not reorder this before the
 	// default-header loop.
-	if s.authManager != nil && r.Context().Value(skipOAuth) == nil {
+	if s.authManager != nil && toCatalog && r.Context().Value(skipOAuth) == nil {
 		var (
 			k, v string
 			err  error
@@ -305,7 +326,12 @@ func (s *sessionTransport) RoundTrip(r *http.Request) (*http.Response, error) {
 		r.Header.Set(k, v)
 	}
 
-	if s.signer != nil && (s.signingOrigin == nil || sameOrigin(s.signingOrigin, r.URL)) {
+	// A signer only signs requests to the configured catalog origin: a request
+	// to a different origin (e.g. a redirect hop) is left unsigned, so the
+	// signer's Authorization header and any session token never reach an
+	// unconfigured host. The guard lives here, in core, so it covers every
+	// signer, including one installed verbatim via WithSigner.
+	if s.signer != nil && toCatalog {
 		if err := s.signer.SignRequest(r); err != nil {
 			return nil, err
 		}
@@ -1046,6 +1072,8 @@ func (r *Catalog) createSession(ctx context.Context, opts *options) (*http.Clien
 	session := &sessionTransport{
 		RoundTripper:   baseTransport,
 		defaultHeaders: http.Header{},
+		catalogOrigin:  r.baseURI,
+		authOrigin:     opts.authUri,
 	}
 	cl := &http.Client{Transport: session}
 
@@ -1076,6 +1104,13 @@ func (r *Catalog) createSession(ctx context.Context, opts *options) (*http.Clien
 		}
 	}
 
+	session.builtinHeaders = http.Header{}
+	for _, k := range []string{"X-Client-Version", "Content-Type", "User-Agent", headerIcebergAccessDelegation} {
+		if v := session.defaultHeaders.Values(k); len(v) > 0 {
+			session.builtinHeaders[http.CanonicalHeaderKey(k)] = v
+		}
+	}
+
 	if authManager != nil {
 		session.authManager = authManager
 	}
@@ -1087,14 +1122,6 @@ func (r *Catalog) createSession(ctx context.Context, opts *options) (*http.Clien
 		return nil, nil, err
 	}
 	session.signer = signer
-	// A signer only signs requests to the configured catalog origin: a request
-	// to a different origin (e.g. a redirect hop) is left unsigned, so the
-	// signer's Authorization header and any session token never reach an
-	// unconfigured host. The guard lives here, in core, so it covers every
-	// signer, including one installed verbatim via WithSigner.
-	if signer != nil {
-		session.signingOrigin = r.baseURI
-	}
 
 	return cl, cleanup, nil
 }

catalog/rest/rest_internal_test.go

@@ -102,6 +102,207 @@ func TestSignerDoesNotSignCrossOriginRedirect(t *testing.T) {
 	require.Empty(t, gotToken, "the redirect target must not receive the session token")
 }
 
+// TestCredentialsNotSentOnCrossOriginRedirect pins that the OAuth bearer token
+// and user-supplied default headers stay on the origin that started the request,
+// while a same-origin redirect keeps them.
+func TestCredentialsNotSentOnCrossOriginRedirect(t *testing.T) {
+	type seen struct {
+		hit                         bool
+		auth, apiKey, custom, agent string
+	}
+	record := func(s *seen, r *http.Request) {
+		s.hit = true
+		s.auth = r.Header.Get("Authorization")
+		s.apiKey = r.Header.Get("X-Api-Key")
+		s.custom = r.Header.Get("X-Custom")
+		s.agent = r.Header.Get("User-Agent")
+	}
+
+	var first, other, sameLanding seen
+	second := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		record(&other, r)
+		w.WriteHeader(http.StatusOK)
+	}))
+	defer second.Close()
+
+	mux := http.NewServeMux()
+	mux.HandleFunc("/v1/config", func(w http.ResponseWriter, r *http.Request) {
+		json.NewEncoder(w).Encode(map[string]any{"defaults": map[string]any{}, "overrides": map[string]any{}})
+	})
+	mux.HandleFunc("/cross", func(w http.ResponseWriter, r *http.Request) {
+		record(&first, r)
+		http.Redirect(w, r, second.URL+"/landing", http.StatusTemporaryRedirect)
+	})
+	mux.HandleFunc("/same", func(w http.ResponseWriter, r *http.Request) {
+		http.Redirect(w, r, "/landing", http.StatusTemporaryRedirect)
+	})
+	mux.HandleFunc("/landing", func(w http.ResponseWriter, r *http.Request) {
+		record(&sameLanding, r)
+		w.WriteHeader(http.StatusOK)
+	})
+	srv := httptest.NewServer(mux)
+	defer srv.Close()
+
+	cat, err := NewCatalog(context.Background(), "rest", srv.URL,
+		WithOAuthToken("SECRET-CATALOG-TOKEN"),
+		WithHeaders(map[string]string{"X-Custom": "SECRET-CUSTOM"}),
+		WithAdditionalProps(iceberg.Properties{"header.X-Api-Key": "SECRET-API-KEY"}))
+	require.NoError(t, err)
+
+	get := func(path string) {
+		req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, srv.URL+path, nil)
+		require.NoError(t, err)
+		resp, err := cat.cl.Do(req)
+		require.NoError(t, err)
+		require.NoError(t, resp.Body.Close())
+	}
+
+	t.Run("cross origin", func(t *testing.T) {
+		get("/cross")
+
+		require.Equal(t, "Bearer SECRET-CATALOG-TOKEN", first.auth, "the configured origin must still be authenticated")
+		require.Equal(t, "SECRET-API-KEY", first.apiKey)
+		require.Equal(t, "SECRET-CUSTOM", first.custom)
+
+		require.True(t, other.hit, "the redirect target must be reached")
+		assert.Empty(t, other.auth, "the redirect target must not receive the bearer token")
+		assert.Empty(t, other.apiKey, "the redirect target must not receive header.* defaults")
+		assert.Empty(t, other.custom, "the redirect target must not receive WithHeaders defaults")
+		assert.Equal(t, "GoIceberg/"+iceberg.Version(), other.agent, "built-in client headers are still sent")
+	})
+
+	t.Run("same origin", func(t *testing.T) {
+		get("/same")
+
+		require.True(t, sameLanding.hit)
+		assert.Equal(t, "Bearer SECRET-CATALOG-TOKEN", sameLanding.auth)
+		assert.Equal(t, "SECRET-API-KEY", sameLanding.apiKey)
+		assert.Equal(t, "SECRET-CUSTOM", sameLanding.custom)
+	})
+}
+
+// TestOverriddenBuiltinHeadersSentOnCrossOriginRedirect pins that a
+// cross-origin hop receives the operator's value for a built-in header it
+// overrode, not the built-in default.
+func TestOverriddenBuiltinHeadersSentOnCrossOriginRedirect(t *testing.T) {
+	var otherHit bool
+	var otherAgent, otherDelegation, otherCustom string
+	second := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		otherHit = true
+		otherAgent = r.Header.Get("User-Agent")
+		otherDelegation = r.Header.Get(headerIcebergAccessDelegation)
+		otherCustom = r.Header.Get("X-Custom")
+		w.WriteHeader(http.StatusOK)
+	}))
+	defer second.Close()
+
+	mux := http.NewServeMux()
+	mux.HandleFunc("/v1/config", func(w http.ResponseWriter, r *http.Request) {
+		json.NewEncoder(w).Encode(map[string]any{"defaults": map[string]any{}, "overrides": map[string]any{}})
+	})
+	mux.HandleFunc("/cross", func(w http.ResponseWriter, r *http.Request) {
+		http.Redirect(w, r, second.URL+"/landing", http.StatusTemporaryRedirect)
+	})
+	srv := httptest.NewServer(mux)
+	defer srv.Close()
+
+	cat, err := NewCatalog(context.Background(), "rest", srv.URL,
+		WithHeaders(map[string]string{"User-Agent": "corp-agent/1", "X-Custom": "SECRET-CUSTOM"}),
+		WithAdditionalProps(iceberg.Properties{"header." + headerIcebergAccessDelegation: "remote-signing"}))
+	require.NoError(t, err)
+
+	req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, srv.URL+"/cross", nil)
+	require.NoError(t, err)
+	resp, err := cat.cl.Do(req)
+	require.NoError(t, err)
+	require.NoError(t, resp.Body.Close())
+
+	require.True(t, otherHit, "the redirect target must be reached")
+	assert.Equal(t, "corp-agent/1", otherAgent)
+	assert.Equal(t, "remote-signing", otherDelegation)
+	assert.Empty(t, otherCustom, "the redirect target must not receive WithHeaders defaults")
+}
+
+// TestCredentialsNotSentOnSyntheticRedirect pins that the redirect guard does
+// not depend on the transport linking Response.Request: a custom transport that
+// returns a bare 307 must not cause credentials to follow it.
+func TestCredentialsNotSentOnSyntheticRedirect(t *testing.T) {
+	var otherHit bool
+	var otherAuth, otherAPIKey string
+	transport := roundTripFunc(func(r *http.Request) (*http.Response, error) {
+		switch {
+		case r.URL.Host == "other.test":
+			otherHit = true
+			otherAuth = r.Header.Get("Authorization")
+			otherAPIKey = r.Header.Get("X-Api-Key")
+
+			return &http.Response{StatusCode: http.StatusOK, Body: http.NoBody}, nil
+		case r.URL.Path == "/v1/config":
+			return &http.Response{
+				StatusCode: http.StatusOK,
+				Body:       io.NopCloser(bytes.NewReader([]byte(`{"defaults":{},"overrides":{}}`))),
+			}, nil
+		default:
+			// A synthetic redirect: no Request backlink on the response.
+			return &http.Response{
+				StatusCode: http.StatusTemporaryRedirect,
+				Header:     http.Header{"Location": {"http://other.test/landing"}},
+				Body:       http.NoBody,
+			}, nil
+		}
+	})
+
+	cat, err := NewCatalog(context.Background(), "rest", "http://catalog.test",
+		WithCustomTransport(transport),
+		WithOAuthToken("SECRET-CATALOG-TOKEN"),
+		WithAdditionalProps(iceberg.Properties{"header.X-Api-Key": "SECRET-API-KEY"}))
+	require.NoError(t, err)
+
+	req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, "http://catalog.test/redirect", nil)
+	require.NoError(t, err)
+	resp, err := cat.cl.Do(req)
+	require.NoError(t, err)
+	require.NoError(t, resp.Body.Close())
+
+	require.True(t, otherHit, "the redirect target must be reached")
+	assert.Empty(t, otherAuth, "the redirect target must not receive the bearer token")
+	assert.Empty(t, otherAPIKey, "the redirect target must not receive header.* defaults")
+}
+
+// TestHeaderDefaultsReachSeparateTokenEndpoint pins that the configured OAuth
+// token endpoint is trusted for header.* defaults even on a different origin
+// from the catalog, so the redirect guard does not break IdPs that need them.
+func TestHeaderDefaultsReachSeparateTokenEndpoint(t *testing.T) {
+	var tokenAPIKey string
+	tokenSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		tokenAPIKey = r.Header.Get("X-Api-Key")
+		w.Header().Set("Content-Type", "application/json")
+		json.NewEncoder(w).Encode(map[string]any{"access_token": "TOKEN", "token_type": "Bearer", "expires_in": 3600})
+	}))
+	defer tokenSrv.Close()
+
+	var catalogAuth string
+	mux := http.NewServeMux()
+	mux.HandleFunc("/v1/config", func(w http.ResponseWriter, r *http.Request) {
+		catalogAuth = r.Header.Get("Authorization")
+		json.NewEncoder(w).Encode(map[string]any{"defaults": map[string]any{}, "overrides": map[string]any{}})
+	})
+	srv := httptest.NewServer(mux)
+	defer srv.Close()
+
+	authURI, err := url.Parse(tokenSrv.URL + "/token")
+	require.NoError(t, err)
+
+	_, err = NewCatalog(context.Background(), "rest", srv.URL,
+		WithCredential("client:secret"),
+		WithAuthURI(authURI),
+		WithAdditionalProps(iceberg.Properties{"header.X-Api-Key": "SECRET-API-KEY"}))
+	require.NoError(t, err)
+
+	assert.Equal(t, "Bearer TOKEN", catalogAuth)
+	assert.Equal(t, "SECRET-API-KEY", tokenAPIKey, "the configured token endpoint must still receive header.* defaults")
+}
+
 func TestSplitIdentForPathRequiresNamespaceAndName(t *testing.T) {
 	cat := &Catalog{}