Reject rewrite HTTPS downgrades; close intermediate redirect responses

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 <T> HttpResponse<T> 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++;
     }

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<Void>().apply {
+          statusCode = 302
+          headers = httpHeaders("Location", "https://redirect-target/bar.pkl")
+        },
+        FakeHttpResponse<Void>(),
+      )
+    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<InputStream>().apply {
+          statusCode = 302
+          headers = httpHeaders("Location", "/bar.pkl")
+          body = intermediateBody
+        },
+        FakeHttpResponse<InputStream>(),
+      )
+    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()
+    }
+  }
 }

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<URI>()
+  private val responses = ArrayDeque(responses.toList())
+
+  @Suppress("UNCHECKED_CAST")
+  override fun <T : Any> send(
+    request: HttpRequest,
+    responseBodyHandler: HttpResponse.BodyHandler<T>,
+    httpRequestChecker: HttpClient.HttpRequestChecker,
+  ): HttpResponse<T> {
+    requestedUris.add(request.uri())
+    return responses.removeFirst() as HttpResponse<T>
+  }
+
+  override fun close() {}
+}