[
https://issues.apache.org/jira/browse/GROOVY-12274?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18105729#comment-18105729
]
ASF GitHub Bot commented on GROOVY-12274:
-----------------------------------------
Copilot commented on code in PR #2811:
URL: https://github.com/apache/groovy/pull/2811#discussion_r3809055977
##########
subprojects/groovy-http-builder/src/main/groovy/groovy/http/HttpBuilder.groovy:
##########
@@ -54,14 +54,11 @@ 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. The client is therefore left at its
default of not
+ // following redirects at all.
+ followRedirectsManually = config.followRedirects
Review Comment:
With redirects no longer configured on the underlying `HttpClient.Builder`,
`streamAsync`/`getStreamAsync` will not follow redirects even when
`followRedirects` is enabled. This appears to contradict the existing
`buildStreamRequest` note (“does not auto-follow redirects under confinement”),
and it changes behavior for the unconfined streaming case. Consider
implementing manual redirect-following for streaming requests when
`followRedirects` is true (including the cross-origin header-shedding policy),
or explicitly document/encode that streaming never follows redirects.
##########
subprojects/groovy-http-builder/src/test/groovy/groovy/http/HttpBuilderRedirectHeaderTest.groovy:
##########
@@ -0,0 +1,157 @@
+/*
+ * 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 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. */
+ static Map<String, String> received = [:]
+ /** Counts final-hop arrivals, so a negative assertion cannot pass by the
hop never happening. */
+ static int arrivals = 0
+
+ @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 += 1
+ 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 = 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 == 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 == 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 received.isEmpty(),
+ "an asynchronous redirect to another origin received
${received.keySet()}"
+ }
Review Comment:
`testHeadersAreNotCarriedToAnotherOriginAsynchronously` can pass even if
redirects are *not* being followed: if the `/target` hop is never reached,
`received` remains empty and the assertion still succeeds. Add an `arrivals ==
1` assertion (matching the synchronous tests) to ensure the negative assertion
is meaningful.
##########
subprojects/groovy-http-builder/src/test/groovy/groovy/http/HttpBuilderRedirectHeaderTest.groovy:
##########
@@ -0,0 +1,157 @@
+/*
+ * 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 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. */
+ static Map<String, String> received = [:]
+ /** Counts final-hop arrivals, so a negative assertion cannot pass by the
hop never happening. */
+ static int arrivals = 0
+
+ @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 += 1
+ record(exchange)
+ byte[] body = 'ok'.bytes
+ exchange.sendResponseHeaders(200, body.length)
+ exchange.responseBody.withStream { it.write(body) }
+ }
+ }
Review Comment:
The test uses static mutable state (`received` map and `arrivals` counter)
written from the `HttpServer` handler threads and read from the test thread
without any synchronization. This is a data race and can lead to flaky
assertions (especially for the async case). Prefer returning the observed
headers in the HTTP response (and asserting on the response body), or make the
shared state thread-safe (e.g., `ConcurrentHashMap` + `AtomicInteger`, plus a
latch to await `/target` execution).
> HttpBuilder: drop default headers on a redirect to another origin
> -----------------------------------------------------------------
>
> Key: GROOVY-12274
> URL: https://issues.apache.org/jira/browse/GROOVY-12274
> Project: Groovy
> Issue Type: Improvement
> Reporter: Paul King
> Priority: Major
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
