From bbf23af10f08d256901f4bb8acf39ed22bb021af Mon Sep 17 00:00:00 2001 From: Kushal Pisavadia Date: Thu, 9 Jul 2026 06:22:31 +0100 Subject: [PATCH] Reject rewrite HTTPS downgrades; close intermediate redirect responses Both issues are in `RequestRewritingClient.doSend`. It follows redirects by hand now that the JDK client uses `Redirect.NEVER`. ## Downgrade guard The guard read the raw `Location` scheme before applying rewrites. So a rewrite from `https` to `http` slipped a cleartext request past it. Now the code rewrites first, then checks the scheme. ## Connection leak The loop dropped each intermediate `3xx` response without closing it. That leaked one `BodyHandlers.ofInputStream()` connection per hop. Now the code closes the body before following the redirect. --- .../pkl/core/http/RequestRewritingClient.java | 11 +++- .../core/http/RequestRewritingClientTest.kt | 65 +++++++++++++++++++ .../src/test/kotlin/org/pkl/core/http/util.kt | 23 +++++++ 3 files changed, 96 insertions(+), 3 deletions(-) diff --git a/pkl-core/src/main/java/org/pkl/core/http/RequestRewritingClient.java b/pkl-core/src/main/java/org/pkl/core/http/RequestRewritingClient.java index cd99616a4..38741cb65 100644 --- a/pkl-core/src/main/java/org/pkl/core/http/RequestRewritingClient.java +++ b/pkl-core/src/main/java/org/pkl/core/http/RequestRewritingClient.java @@ -16,6 +16,7 @@ package org.pkl.core.http; import com.google.errorprone.annotations.ThreadSafe; +import java.io.Closeable; import java.io.IOException; import java.net.URI; import java.net.URISyntaxException; @@ -134,12 +135,16 @@ private HttpResponse doSend( throw new HttpClientException( ErrorMessages.create("httpRedirectInvalidUri", currentRequestUri, location.get())); } + var nextRequestUri = rewriteUri(redirectUri); if (currentRequestUri.getScheme().equalsIgnoreCase("https") - && redirectUri.getScheme().equalsIgnoreCase("http")) { + && nextRequestUri.getScheme().equalsIgnoreCase("http")) { throw new HttpClientException( - ErrorMessages.create("httpRedirectCannotDowngrade", currentRequestUri, redirectUri)); + ErrorMessages.create("httpRedirectCannotDowngrade", currentRequestUri, nextRequestUri)); } - currentRequestUri = rewriteUri(redirectUri); + if (response.body() instanceof Closeable closeable) { + closeable.close(); + } + currentRequestUri = nextRequestUri; currentRequest = rewriteRequest(request, currentRequestUri); redirectCount++; } diff --git a/pkl-core/src/test/kotlin/org/pkl/core/http/RequestRewritingClientTest.kt b/pkl-core/src/test/kotlin/org/pkl/core/http/RequestRewritingClientTest.kt index 6e8809c79..bd39bbaa7 100644 --- a/pkl-core/src/test/kotlin/org/pkl/core/http/RequestRewritingClientTest.kt +++ b/pkl-core/src/test/kotlin/org/pkl/core/http/RequestRewritingClientTest.kt @@ -15,6 +15,8 @@ */ package org.pkl.core.http +import java.io.ByteArrayInputStream +import java.io.InputStream import java.net.URI import java.net.http.HttpClient as JdkHttpClient import java.net.http.HttpRequest @@ -23,8 +25,10 @@ import java.net.http.HttpResponse.BodyHandlers import java.time.Duration import java.util.Locale import org.assertj.core.api.Assertions.assertThat +import org.assertj.core.api.Assertions.assertThatCode import org.assertj.core.api.Assertions.assertThatList import org.junit.jupiter.api.Test +import org.pkl.commons.test.FakeHttpResponse import org.pkl.core.util.GlobResolver import org.pkl.core.util.IoUtils @@ -441,4 +445,65 @@ class RequestRewritingClientTest { assertThatList(captured.request.headers().allValues("user-agent")) .containsExactly("My User Agent") } + + @Test + fun `rejects downgrade when a rewrite maps an https redirect target to http`() { + val delegate = + ReplayingClient( + FakeHttpResponse().apply { + statusCode = 302 + headers = httpHeaders("Location", "https://redirect-target/bar.pkl") + }, + FakeHttpResponse(), + ) + val client = + RequestRewritingClient( + "Pkl", + Duration.ofSeconds(42), + -1, + delegate, + mapOf(URI("https://redirect-target/") to URI("http://downgraded/")), + mapOf(), + ) + val request = HttpRequest.newBuilder(URI("https://start.example/foo.pkl")).build() + + assertThatCode { client.send(request, BodyHandlers.discarding(), NoopChecker) } + .`as`("following an https redirect whose rewritten target is http must be rejected") + .hasMessageContaining("Cannot follow redirect from 'https:' URL to 'http:' URL") + assertThat(delegate.requestedUris.map { it.scheme }).doesNotContain("http") + } + + @Test + fun `closes intermediate redirect response body before following the redirect`() { + val intermediateBody = CloseTrackingInputStream() + val delegate = + ReplayingClient( + FakeHttpResponse().apply { + statusCode = 302 + headers = httpHeaders("Location", "/bar.pkl") + body = intermediateBody + }, + FakeHttpResponse(), + ) + val client = + RequestRewritingClient("Pkl", Duration.ofSeconds(42), -1, delegate, mapOf(), mapOf()) + val request = HttpRequest.newBuilder(URI("https://example.com/foo.pkl")).build() + + client.send(request, BodyHandlers.ofInputStream(), NoopChecker) + + assertThat(intermediateBody.closed) + .`as`("intermediate 3xx response body must be closed to release its connection") + .isTrue + } + + /** @ByteArrayInputStream that records whether it was closed. */ + private class CloseTrackingInputStream : ByteArrayInputStream(ByteArray(0)) { + var closed = false + private set + + override fun close() { + closed = true + super.close() + } + } } diff --git a/pkl-core/src/test/kotlin/org/pkl/core/http/util.kt b/pkl-core/src/test/kotlin/org/pkl/core/http/util.kt index ed51b474f..587468980 100644 --- a/pkl-core/src/test/kotlin/org/pkl/core/http/util.kt +++ b/pkl-core/src/test/kotlin/org/pkl/core/http/util.kt @@ -16,6 +16,9 @@ package org.pkl.core.http import java.net.URI +import java.net.http.HttpHeaders +import java.net.http.HttpRequest +import java.net.http.HttpResponse import java.util.regex.Pattern data class HttpSettings( @@ -32,3 +35,23 @@ fun HttpClient.getConfiguredSettings(): HttpSettings { object NoopChecker : HttpClient.HttpRequestChecker { override fun check(uri: URI) {} } + +fun httpHeaders(name: String, value: String): HttpHeaders = + HttpHeaders.of(mapOf(name to listOf(value))) { _, _ -> true } + +class ReplayingClient(vararg responses: HttpResponse<*>) : HttpClient { + val requestedUris = mutableListOf() + private val responses = ArrayDeque(responses.toList()) + + @Suppress("UNCHECKED_CAST") + override fun send( + request: HttpRequest, + responseBodyHandler: HttpResponse.BodyHandler, + httpRequestChecker: HttpClient.HttpRequestChecker, + ): HttpResponse { + requestedUris.add(request.uri()) + return responses.removeFirst() as HttpResponse + } + + override fun close() {} +}