GROOVY-12274: HttpBuilder: drop default headers on a redirect to another origin

With followRedirects and no confinement, redirect following was delegated to
the JDK client, which re-issued the request to the Location target carrying
the headers configured on the builder. Those headers are applied to every
request, so a token or key among them was sent to whatever origin a
redirect named.

The platform cannot be relied on for this. Measured against a cross-host
redirect, on a chain that ends at a server which records what it received:

                        JDK 17.0.20    JDK 21.0.6    JDK 23.0.2
                        (2026-07)      (2025-01)     (2025-01)
  Authorization         stripped       forwarded     forwarded
  Cookie                stripped       forwarded     forwarded
  Proxy-Authorization   stripped       stripped      stripped
  X-Api-Key             forwarded      forwarded     forwarded

Two things follow. Whether the well known credential headers are protected
depends on the update level of the JDK in use, which an application cannot
choose. And a header the platform does not recognise is forwarded on every
JDK, while the builder's headers may hold anything the caller put there, so
a policy naming header names would repeat the same mistake at one remove.

Follow redirects here in every case rather than only under confinement, and
drop the caller's headers for good once a hop leaves the origin the request
started from. Same-origin redirects are unaffected, as is confinement, which
already rejected a cross-origin hop outright. A chain which returns to the
original origin does not get the headers back, since by then they have been
seen by another server.

Note this drops all of the caller's headers rather than a chosen few: which
of them carry credentials is not something this class can know.

Author: paulk-asert

subprojects/groovy-http-builder/src/main/groovy/groovy/http/HttpBuilder.groovy

@@ -54,20 +54,21 @@ final class HttpBuilder {
         if (config.connectTimeout != null) {
             clientBuilder.connectTimeout(config.connectTimeout)
         }
-        // When confinement is active we must see every 3xx ourselves so the base-URI
-        // gate can be applied to each hop; the JDK client would otherwise follow
-        // redirects internally, escaping confinement. Only the unconfined case
-        // delegates redirect-following to the JDK.
-        followRedirectsManually = config.confineToBaseUri && config.followRedirects
-        if (config.followRedirects && !followRedirectsManually) {
-            clientBuilder.followRedirects(HttpClient.Redirect.NORMAL)
-        }
+        // Every 3xx is seen here rather than followed inside the JDK client, so that each hop
+        // can be gated: a confined hop against the base URI, and any hop against the origin the
+        // caller's headers were set for.
+        followRedirectsManually = config.followRedirects
         if (config.clientConfigurer != null) {
             Closure<?> code = (Closure<?>) config.clientConfigurer.clone()
             code.resolveStrategy = Closure.DELEGATE_FIRST
             code.delegate = clientBuilder
             code.call(clientBuilder)
         }
+        // Redirect policy is owned by the followRedirects flag above. A redirect policy set on
+        // the raw builder (via clientConfig) would make the JDK client follow hops internally,
+        // where no hop can be gated and the caller's headers travel to whatever origin a
+        // Location names — so any such setting is overridden, after the configurer has run.
+        clientBuilder.followRedirects(HttpClient.Redirect.NEVER)
         client = clientBuilder.build()
         baseUri = config.baseUri
         defaultHeaders = Collections.unmodifiableMap(new LinkedHashMap<>(config.headers))
@@ -217,7 +218,8 @@ final class HttpBuilder {
         if (followRedirectsManually) {
             future = future.thenCompose { HttpResponse<String> response ->
                 followRedirectsAsync(method, httpRequest.uri(), response, headers,
-                        requestSpec.body, requestSpec.timeout, requestSpec.bodyHandler, 0)
+                        requestSpec.body, requestSpec.timeout, requestSpec.bodyHandler, 0,
+                        httpRequest.uri(), false)
             }
         }
         return future.thenApply { HttpResponse<String> response -> new HttpResult(response) }
@@ -339,10 +341,10 @@ final class HttpBuilder {
     }
 
     private HttpRequest buildStreamRequest(final String method, final Object uri, final Closure<?> spec) {
-        // Note: streaming does not auto-follow redirects under confinement. Because
-        // the body is an unbuffered publisher, a 3xx is returned to the caller as-is
-        // rather than followed. This is safe (no bypass) but not transparent; callers
-        // who need confined streaming redirects should resolve the Location themselves.
+        // Note: streaming never auto-follows redirects. Because the response body is an
+        // unbuffered publisher, a 3xx is returned to the caller as-is rather than followed;
+        // callers who need streaming redirects should resolve the Location themselves. This
+        // also means no hop can escape the gates above, at the cost of transparency.
         RequestSpec requestSpec = evalSpec(spec)
         URI resolvedUri = resolveUri(uri, requestSpec.queryParameters)
         return buildHopRequest(method, resolvedUri, combinedHeaders(requestSpec), requestSpec.body, requestSpec.timeout)
@@ -393,27 +395,32 @@ final class HttpBuilder {
     }
 
     /**
-     * Synchronously follows redirects while confinement is active, applying
-     * {@link #enforceConfinement} to every hop. Because a confined hop must share
-     * the base URI's origin, a redirect to another host is rejected outright — so
-     * sensitive headers can never leak across origins here.
+     * Synchronously follows redirects, applying {@link #enforceConfinement} to every hop while
+     * confinement is active, and shedding the caller's headers on any hop that leaves the
+     * origin the request started from.
      */
     private HttpResponse<String> followRedirects(String method, URI currentUri, HttpResponse<String> response,
                                                  Map<String, String> headers, Object body,
                                                  Duration timeout, HttpResponse.BodyHandler<String> bodyHandler) {
+        URI origin = currentUri
+        boolean leftOrigin = false
         int redirects = 0
         while (true) {
             URI target = redirectTarget(currentUri, response)
             if (target == null) {
                 return response
             }
             if (++redirects > MAX_REDIRECTS) {
-                throw new RuntimeException("Too many redirects (> " + MAX_REDIRECTS + ") for request confined to " + baseUri)
+                throw new RuntimeException("Too many redirects (> " + MAX_REDIRECTS + ") for request to " + origin)
             }
             String nextMethod = redirectMethod(method, response.statusCode())
             boolean sameMethod = nextMethod == method
             Object nextBody = sameMethod ? body : null
             Map<String, String> nextHeaders = sameMethod ? headers : withoutBodyHeaders(headers)
+            leftOrigin = leftOrigin || !sameOrigin(origin, target)
+            if (leftOrigin) {
+                nextHeaders = [:]
+            }
             HttpRequest httpRequest = buildHopRequest(nextMethod, target, nextHeaders, nextBody, timeout)
             response = send(nextMethod, httpRequest, bodyHandler)
             currentUri = target
@@ -431,24 +438,29 @@ final class HttpBuilder {
     private CompletableFuture<HttpResponse<String>> followRedirectsAsync(
             String method, URI currentUri, HttpResponse<String> response,
             Map<String, String> headers, Object body, Duration timeout,
-            HttpResponse.BodyHandler<String> bodyHandler, int redirects) {
+            HttpResponse.BodyHandler<String> bodyHandler, int redirects, URI origin, boolean leftOrigin) {
         URI target = redirectTarget(currentUri, response)
         if (target == null) {
             return CompletableFuture.completedFuture(response)
         }
         if (redirects + 1 > MAX_REDIRECTS) {
             CompletableFuture<HttpResponse<String>> failed = new CompletableFuture<>()
             failed.completeExceptionally(
-                    new RuntimeException("Too many redirects (> " + MAX_REDIRECTS + ") for request confined to " + baseUri))
+                    new RuntimeException("Too many redirects (> " + MAX_REDIRECTS + ") for request to " + origin))
             return failed
         }
         String nextMethod = redirectMethod(method, response.statusCode())
         boolean sameMethod = nextMethod == method
         Object nextBody = sameMethod ? body : null
         Map<String, String> nextHeaders = sameMethod ? headers : withoutBodyHeaders(headers)
+        boolean nextLeftOrigin = leftOrigin || !sameOrigin(origin, target)
+        if (nextLeftOrigin) {
+            nextHeaders = [:]
+        }
         HttpRequest httpRequest = buildHopRequest(nextMethod, target, nextHeaders, nextBody, timeout)
         return client.sendAsync(httpRequest, bodyHandler).thenCompose { HttpResponse<String> next ->
-            followRedirectsAsync(nextMethod, target, next, nextHeaders, nextBody, timeout, bodyHandler, redirects + 1)
+            followRedirectsAsync(nextMethod, target, next, nextHeaders, nextBody, timeout, bodyHandler,
+                    redirects + 1, origin, nextLeftOrigin)
         }
     }
 
@@ -784,6 +796,10 @@ final class HttpBuilder {
         /**
          * Provides direct access to the underlying {@code HttpClient.Builder}
          * for advanced configuration (authenticator, SSL context, proxy, cookie handler, etc.).
+         * <p>
+         * Redirect policy is the exception: it is owned by {@link #followRedirects} so that
+         * every hop can be gated against confinement and against the origin the caller's
+         * headers were set for. A redirect policy set on the raw builder here is overridden.
          *
          * @param configurer a closure taking an {@code HttpClient.Builder}
          */

subprojects/groovy-http-builder/src/test/groovy/groovy/http/HttpBuilderRedirectHeaderTest.groovy

@@ -0,0 +1,182 @@
+/*
+ *  Licensed to the Apache Software Foundation (ASF) under one
+ *  or more contributor license agreements.  See the NOTICE file
+ *  distributed with this work for additional information
+ *  regarding copyright ownership.  The ASF licenses this file
+ *  to you under the Apache License, Version 2.0 (the
+ *  "License"); you may not use this file except in compliance
+ *  with the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *  Unless required by applicable law or agreed to in writing,
+ *  software distributed under the License is distributed on an
+ *  "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ *  KIND, either express or implied.  See the License for the
+ *  specific language governing permissions and limitations
+ *  under the License.
+ */
+package groovy.http
+
+import com.sun.net.httpserver.HttpExchange
+import com.sun.net.httpserver.HttpServer
+
+import java.net.http.HttpClient
+import java.util.concurrent.ConcurrentHashMap
+import java.util.concurrent.atomic.AtomicInteger
+import org.junit.jupiter.api.AfterAll
+import org.junit.jupiter.api.BeforeAll
+import org.junit.jupiter.api.BeforeEach
+import org.junit.jupiter.api.Test
+
+/**
+ * GROOVY-12274: headers the caller configured for one origin must not be carried to another by
+ * following a redirect. The JDK client cannot be relied on for this. It protects only the header
+ * names it knows about, so a credential in any other header is forwarded regardless; and whether
+ * it protects even those depends on the update level of the JDK in use.
+ */
+class HttpBuilderRedirectHeaderTest {
+
+    static HttpServer origin
+    static HttpServer elsewhere
+    static int originPort
+    static int elsewherePort
+    /** Headers seen by whichever endpoint served the final hop; written on server threads. */
+    static final Map<String, String> received = new ConcurrentHashMap<>()
+    /** Counts final-hop arrivals, so a negative assertion cannot pass by the hop never happening. */
+    static final AtomicInteger arrivals = new AtomicInteger()
+
+    @BeforeAll
+    static void setUpClass() {
+        origin = HttpServer.create(new InetSocketAddress('127.0.0.1', 0), 0)
+        originPort = origin.address.port
+        elsewhere = HttpServer.create(new InetSocketAddress('127.0.0.1', 0), 0)
+        elsewherePort = elsewhere.address.port
+
+        // The redirect target is passed as the query so one handler serves every case.
+        origin.createContext('/redirect') { HttpExchange exchange ->
+            exchange.responseHeaders.add('Location', exchange.requestURI.query)
+            exchange.sendResponseHeaders(302, -1)
+            exchange.close()
+        }
+        [origin, elsewhere].each { server ->
+            server.createContext('/target') { HttpExchange exchange ->
+                arrivals.incrementAndGet()
+                record(exchange)
+                byte[] body = 'ok'.bytes
+                exchange.sendResponseHeaders(200, body.length)
+                exchange.responseBody.withStream { it.write(body) }
+            }
+        }
+        // Second hop for the return-to-origin case.
+        elsewhere.createContext('/bounce') { HttpExchange exchange ->
+            exchange.responseHeaders.add('Location', URLDecoder.decode(exchange.requestURI.query, 'UTF-8'))
+            exchange.sendResponseHeaders(302, -1)
+            exchange.close()
+        }
+        origin.start()
+        elsewhere.start()
+    }
+
+    @AfterAll
+    static void tearDownClass() {
+        origin?.stop(0)
+        elsewhere?.stop(0)
+    }
+
+    @BeforeEach
+    void setUp() {
+        received.clear()
+        arrivals.set(0)
+    }
+
+    private static void record(HttpExchange exchange) {
+        ['Authorization', 'Cookie', 'X-Api-Key', 'Accept'].each { name ->
+            def values = exchange.requestHeaders.get(name)
+            if (values) received.put(name, values.first())
+        }
+    }
+
+    private static HttpBuilder builderWithHeaders() {
+        HttpBuilder.http {
+            baseUri "http://127.0.0.1:${originPort}"
+            followRedirects true
+            headers([
+                    'Authorization': 'Bearer SECRET-TOKEN',
+                    'Cookie'       : 'session=SECRET-COOKIE',
+                    'X-Api-Key'    : 'SECRET-KEY',
+                    'Accept'       : 'text/plain',
+            ])
+        }
+    }
+
+    @Test
+    void testHeadersAreNotCarriedToAnotherOrigin() {
+        builderWithHeaders().get("/redirect?http://127.0.0.1:${elsewherePort}/target")
+
+        assert arrivals.get() == 1, 'the redirect was not followed, so the test proves nothing'
+        assert received.isEmpty(),
+                "a redirect to another origin received ${received.keySet()}"
+    }
+
+    @Test
+    void testCustomHeaderIsNotCarriedEither() {
+        // The header the JDK never protects, on any update level: the whole reason this is not
+        // left to the platform.
+        builderWithHeaders().get("/redirect?http://127.0.0.1:${elsewherePort}/target")
+
+        assert arrivals.get() == 1, 'the redirect was not followed, so the test proves nothing'
+        assert received['X-Api-Key'] == null
+    }
+
+    @Test
+    void testHeadersSurviveARedirectWithinTheSameOrigin() {
+        builderWithHeaders().get("/redirect?http://127.0.0.1:${originPort}/target")
+
+        assert received['Authorization'] == 'Bearer SECRET-TOKEN'
+        assert received['X-Api-Key'] == 'SECRET-KEY'
+        assert received['Accept'] == 'text/plain'
+    }
+
+    @Test
+    void testHeadersAreNotCarriedToAnotherOriginAsynchronously() {
+        builderWithHeaders()
+                .requestAsync('GET', "/redirect?http://127.0.0.1:${elsewherePort}/target")
+                .join()
+
+        assert arrivals.get() == 1, 'the redirect was not followed, so the test proves nothing'
+        assert received.isEmpty(),
+                "an asynchronous redirect to another origin received ${received.keySet()}"
+    }
+
+    @Test
+    void testClientConfigCannotHandRedirectsBackToTheJdkClient() {
+        // A redirect policy set on the raw builder would make the JDK client follow hops
+        // internally, bypassing the header shedding; it is overridden, so the manual loop
+        // still follows the redirect and the other origin still sees no caller headers.
+        def builder = HttpBuilder.http {
+            baseUri "http://127.0.0.1:${originPort}"
+            followRedirects true
+            // X-Api-Key is the discriminating probe: a JDK-followed hop forwards it on every
+            // JDK version, so this test fails without the override no matter the platform
+            headers(['Authorization': 'Bearer SECRET-TOKEN', 'X-Api-Key': 'SECRET-KEY'])
+            clientConfig { it.followRedirects(HttpClient.Redirect.NORMAL) }
+        }
+        builder.get("/redirect?http://127.0.0.1:${elsewherePort}/target")
+
+        assert arrivals.get() == 1, 'the redirect was not followed, so the test proves nothing'
+        assert received.isEmpty(),
+                "a redirect to another origin received ${received.keySet()}"
+    }
+
+    @Test
+    void testHeadersAreNotRestoredByReturningToTheOrigin() {
+        // Once a chain has left the origin the caller's headers are gone for good; a hop back
+        // does not bring them out again.
+        String back = URLEncoder.encode("http://127.0.0.1:${originPort}/target", 'UTF-8')
+        builderWithHeaders().get("/redirect?http://127.0.0.1:${elsewherePort}/bounce?${back}")
+
+        assert arrivals.get() == 1, 'the chain did not reach the origin again, so the test proves nothing'
+        assert received['Authorization'] == null
+    }
+}