[
https://issues.apache.org/jira/browse/GROOVY-12182?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18097789#comment-18097789
]
ASF GitHub Bot commented on GROOVY-12182:
-----------------------------------------
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.
> groovy-http-builder: confine requests (and redirects) to the base URI
> ----------------------------------------------------------------------
>
> Key: GROOVY-12182
> URL: https://issues.apache.org/jira/browse/GROOVY-12182
> Project: Groovy
> Issue Type: Improvement
> Components: groovy-http-builder
> Reporter: Paul King
> Assignee: Paul King
> Priority: Major
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
