GROOVY-12274: HttpBuilder: drop default headers on a redirect to anot…

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
+    }
+}