Copilot commented on code in PR #2726:
URL: https://github.com/apache/groovy/pull/2726#discussion_r3620831463
##########
subprojects/groovy-http-builder/src/main/groovy/groovy/http/HttpBuilder.groovy:
##########
@@ -381,9 +501,62 @@ final class HttpBuilder {
}
target = baseUri.resolve(target.toString())
}
+ enforceConfinement(target)
return appendQuery(target, query)
}
+ /**
+ * When {@code confineToBaseUri} is enabled, rejects any request whose
+ * resolved URI leaves the {@link Config#baseUri} namespace: a different
+ * origin (scheme or authority), or a path that escapes the base path
+ * prefix (e.g. an absolute {@code /other} path or {@code ..} traversal).
+ * <p>
+ * The permitted set is exactly the URIs reachable by resolving a
+ * non-escaping relative reference against {@code baseUri}. Comparison uses
+ * raw (percent-encoded) path segments; a server that decodes {@code
%2e%2e}
+ * itself is outside the scope of this check.
+ *
+ * @param target the fully resolved request URI
+ * @throws SecurityException if the target escapes the configured base URI
+ */
+ private void enforceConfinement(final URI target) {
+ if (!confineToBaseUri || baseUri == null) {
+ return
+ }
+ URI normalized = target.normalize()
+ boolean sameScheme =
baseUri.scheme?.equalsIgnoreCase(normalized.scheme)
+ boolean sameAuthority = baseUri.authority != null &&
+ baseUri.authority.equalsIgnoreCase(normalized.authority)
+ if (!sameScheme || !sameAuthority || !isPathWithin(baseUri,
normalized)) {
+ throw new SecurityException(
+ "Request URI '" + target + "' is not confined to baseUri
'" + baseUri + "'")
+ }
Review Comment:
`URI.getAuthority()` string equality is too strict for “same origin” checks:
it can differ while still being the same origin (e.g., explicit default ports
like `:80`/`:443`, or presence/absence of user-info). This will incorrectly
reject otherwise-confined redirects/requests; compare normalized `host`
(case-insensitive) and an “effective port” (explicit port if present, otherwise
default for the scheme) instead of `authority` string equality.
##########
subprojects/groovy-http-builder/src/main/groovy/groovy/http/HttpBuilder.groovy:
##########
@@ -317,60 +339,158 @@ 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.
+ RequestSpec requestSpec = evalSpec(spec)
+ URI resolvedUri = resolveUri(uri, requestSpec.queryParameters)
+ return buildHopRequest(method, resolvedUri,
combinedHeaders(requestSpec), requestSpec.body, requestSpec.timeout)
+ }
+
+ private RequestSpec evalSpec(final Closure<?> spec) {
RequestSpec requestSpec = new RequestSpec()
if (spec != null) {
Closure<?> code = (Closure<?>) spec.clone()
code.resolveStrategy = Closure.DELEGATE_FIRST
code.delegate = requestSpec
code.call()
}
+ return requestSpec
+ }
- URI resolvedUri = resolveUri(uri, requestSpec.queryParameters)
- HttpRequest.Builder requestBuilder =
HttpRequest.newBuilder(resolvedUri)
+ private Map<String, String> combinedHeaders(final RequestSpec requestSpec)
{
+ Map<String, String> headers = new LinkedHashMap<>(defaultHeaders)
+ headers.putAll(requestSpec.headers)
+ return headers
+ }
- Duration timeout = requestSpec.timeout ?: defaultRequestTimeout
+ private HttpRequest buildHopRequest(final String method, final URI uri,
+ final Map<String, String> headers,
+ final Object body, final Duration
requestTimeout) {
+ HttpRequest.Builder requestBuilder = HttpRequest.newBuilder(uri)
+ Duration timeout = requestTimeout ?: defaultRequestTimeout
if (timeout != null) {
requestBuilder.timeout(timeout)
}
-
- defaultHeaders.each { String name, String value ->
- requestBuilder.header(name, value)
- }
- requestSpec.headers.each { String name, String value ->
+ headers.each { String name, String value ->
requestBuilder.setHeader(name, value)
}
-
- requestBuilder.method(method, bodyPublisher(method, requestSpec.body))
+ requestBuilder.method(method, bodyPublisher(method, body))
return requestBuilder.build()
}
- private List buildRequest(final String method, final Object uri, final
Closure<?> spec) {
- RequestSpec requestSpec = new RequestSpec()
- if (spec != null) {
- Closure<?> code = (Closure<?>) spec.clone()
- code.resolveStrategy = Closure.DELEGATE_FIRST
- code.delegate = requestSpec
- code.call()
+ private <T> HttpResponse<T> send(final String method, final HttpRequest
httpRequest,
+ final HttpResponse.BodyHandler<T>
bodyHandler) {
+ try {
+ return client.send(httpRequest, bodyHandler)
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt()
+ throw new RuntimeException("HTTP request " + method + " " +
httpRequest.uri() + " was interrupted", e)
+ } catch (IOException e) {
+ throw new RuntimeException("I/O error during HTTP request " +
method + " " + httpRequest.uri(), e)
}
+ }
- URI resolvedUri = resolveUri(uri, requestSpec.queryParameters)
- HttpRequest.Builder requestBuilder =
HttpRequest.newBuilder(resolvedUri)
+ /**
+ * 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.
+ */
+ private HttpResponse<String> followRedirects(String method, URI
currentUri, HttpResponse<String> response,
+ Map<String, String> headers,
Object body,
+ Duration timeout,
HttpResponse.BodyHandler<String> bodyHandler) {
+ 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)
+ }
+ String nextMethod = redirectMethod(method, response.statusCode())
+ boolean sameMethod = nextMethod == method
+ Object nextBody = sameMethod ? body : null
+ Map<String, String> nextHeaders = sameMethod ? headers :
withoutBodyHeaders(headers)
+ HttpRequest httpRequest = buildHopRequest(nextMethod, target,
nextHeaders, nextBody, timeout)
+ response = send(nextMethod, httpRequest, bodyHandler)
+ currentUri = target
+ method = nextMethod
+ body = nextBody
+ headers = nextHeaders
+ }
+ }
- Duration timeout = requestSpec.timeout ?: defaultRequestTimeout
- if (timeout != null) {
- requestBuilder.timeout(timeout)
+ /**
+ * Asynchronous counterpart of {@link #followRedirects}. A confinement
+ * violation on a hop surfaces as a failed future carrying the
+ * {@link SecurityException} thrown by {@link #enforceConfinement}.
+ */
+ 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) {
+ 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))
+ 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)
+ 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)
}
+ }
- defaultHeaders.each { String name, String value ->
- requestBuilder.header(name, value)
+ /**
+ * Returns the confinement-checked redirect target for a response, or
+ * {@code null} if the response is not a followable redirect (non-3xx, or a
+ * 3xx with no {@code Location}). Throws {@link SecurityException} via
+ * {@link #enforceConfinement} if the target escapes the base URI.
+ */
+ private URI redirectTarget(final URI currentUri, final HttpResponse<?>
response) {
+ int status = response.statusCode()
+ if (status != 301 && status != 302 && status != 303 && status != 307
&& status != 308) {
+ return null
+ }
+ String location =
response.headers().firstValue('Location').orElse(null)
+ if (location == null || location.isEmpty()) {
+ return null
+ }
+ URI target = currentUri.resolve(location).normalize()
+ enforceConfinement(target)
+ return target
+ }
+
+ private static String redirectMethod(final String method, final int
statusCode) {
+ if (statusCode == 303) {
+ return 'GET'
}
- requestSpec.headers.each { String name, String value ->
- requestBuilder.setHeader(name, value)
+ if ((statusCode == 301 || statusCode == 302) &&
+ !('GET'.equalsIgnoreCase(method) ||
'HEAD'.equalsIgnoreCase(method))) {
+ return 'GET'
}
+ return method // 307/308 preserve method and body
+ }
Review Comment:
When `followRedirectsManually` is enabled (confined + redirects), redirect
semantics diverge from the previous behavior that delegated to
`HttpClient.Redirect.NORMAL`. In particular, this code converts non-GET/HEAD
methods on 301/302 into GET and proceeds to follow, which `Redirect.NORMAL`
does not do. To avoid surprising behavior changes triggered solely by enabling
confinement, align manual redirect-following rules with the JDK client’s
redirect policy (or explicitly document this behavior change and consider a
separate option controlling the policy).
##########
subprojects/groovy-http-builder/src/main/groovy/groovy/http/HttpBuilder.groovy:
##########
@@ -381,9 +501,62 @@ final class HttpBuilder {
}
target = baseUri.resolve(target.toString())
}
+ enforceConfinement(target)
return appendQuery(target, query)
}
+ /**
+ * When {@code confineToBaseUri} is enabled, rejects any request whose
+ * resolved URI leaves the {@link Config#baseUri} namespace: a different
+ * origin (scheme or authority), or a path that escapes the base path
+ * prefix (e.g. an absolute {@code /other} path or {@code ..} traversal).
+ * <p>
+ * The permitted set is exactly the URIs reachable by resolving a
+ * non-escaping relative reference against {@code baseUri}. Comparison uses
+ * raw (percent-encoded) path segments; a server that decodes {@code
%2e%2e}
+ * itself is outside the scope of this check.
+ *
+ * @param target the fully resolved request URI
+ * @throws SecurityException if the target escapes the configured base URI
+ */
+ private void enforceConfinement(final URI target) {
+ if (!confineToBaseUri || baseUri == null) {
+ return
+ }
+ URI normalized = target.normalize()
+ boolean sameScheme =
baseUri.scheme?.equalsIgnoreCase(normalized.scheme)
+ boolean sameAuthority = baseUri.authority != null &&
+ baseUri.authority.equalsIgnoreCase(normalized.authority)
+ if (!sameScheme || !sameAuthority || !isPathWithin(baseUri,
normalized)) {
Review Comment:
The newly introduced origin-matching logic (scheme/authority and port
handling) and redirect-following semantics aren’t fully covered by tests. Add
targeted tests for (1) redirects/requests that differ only by explicit default
port (e.g., `http://host/` → `http://host:80/...`) and (2) confined redirect
behavior for non-GET methods on 301/302 to ensure the intended policy is
enforced and stable.
##########
subprojects/groovy-http-builder/src/main/java/groovy/http/HttpBuilderClient.java:
##########
@@ -61,4 +61,13 @@
/** Whether to follow HTTP redirects. Default is false. */
boolean followRedirects() default false;
+
+ /**
+ * Whether requests are confined to the base URL. When {@code true}, a
+ * request whose resolved URI leaves the base URL's origin or path prefix
+ * (for example via an absolute path, {@code ..} traversal, or a {@code
create}
+ * override to another host) is rejected with a {@link SecurityException}.
Review Comment:
The phrase “a {@code create} override to another host” is ambiguous in
Javadoc (it’s not clear what “create” refers to or what is being overridden).
Consider clarifying the exact mechanism (e.g., overriding the generated
`create(...)` factory / supplying an alternate base URL) so annotation users
understand what scenarios are blocked.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
