fix: reject a request body on a body-forbidden method (GET/HEAD/TRACE)

sdk-core/api/sdk-core.api

@@ -1026,6 +1026,7 @@ public final class org/dexpace/sdk/core/http/request/Method : java/lang/Enum {
 	public static final field TRACE Lorg/dexpace/sdk/core/http/request/Method;
 	public static fun getEntries ()Lkotlin/enums/EnumEntries;
 	public final fun getMethod ()Ljava/lang/String;
+	public final fun getPermitsRequestBody ()Z
 	public fun toString ()Ljava/lang/String;
 	public static fun valueOf (Ljava/lang/String;)Lorg/dexpace/sdk/core/http/request/Method;
 	public static fun values ()[Lorg/dexpace/sdk/core/http/request/Method;

sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/request/Method.kt

@@ -15,20 +15,27 @@ package org.dexpace.sdk.core.http.request
  * request line without translation.
  *
  * @property method Canonical uppercase method token sent in the request line.
+ * @property permitsRequestBody Whether HTTP allows this method to carry a request body. `false`
+ *   for `GET`, `HEAD`, and `TRACE`: GET/HEAD have no defined payload semantics (RFC 9110
+ *   §9.3.1/§9.3.2) and a TRACE request "MUST NOT send content" (§9.3.8). Transports disagree on
+ *   how to handle a body on these methods — OkHttp throws `IllegalArgumentException`, the JDK
+ *   builder silently ignores it — so a body on a body-forbidden method is rejected at request
+ *   construction (see `Request.RequestBuilder.build`) rather than left to diverge per transport.
  */
 @Suppress("unused")
 public enum class Method(
     public val method: String,
+    public val permitsRequestBody: Boolean,
 ) {
-    GET("GET"),
-    POST("POST"),
-    PUT("PUT"),
-    DELETE("DELETE"),
-    PATCH("PATCH"),
-    HEAD("HEAD"),
-    OPTIONS("OPTIONS"),
-    TRACE("TRACE"),
-    CONNECT("CONNECT"),
+    GET("GET", permitsRequestBody = false),
+    POST("POST", permitsRequestBody = true),
+    PUT("PUT", permitsRequestBody = true),
+    DELETE("DELETE", permitsRequestBody = true),
+    PATCH("PATCH", permitsRequestBody = true),
+    HEAD("HEAD", permitsRequestBody = false),
+    OPTIONS("OPTIONS", permitsRequestBody = true),
+    TRACE("TRACE", permitsRequestBody = false),
+    CONNECT("CONNECT", permitsRequestBody = false),
     ;
 
     override fun toString(): String = method

sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/request/Request.kt

@@ -37,7 +37,9 @@ import java.net.URL
  * @property method HTTP method on the wire.
  * @property url Fully-resolved target URL.
  * @property headers Request headers; may be empty but never `null`.
- * @property body Request body, or `null` for methods without a payload (typical for GET/HEAD).
+ * @property body Request body, or `null` for methods without a payload. A non-`null` body is
+ *   only valid on a method that permits one ([Method.permitsRequestBody]); building a `GET`,
+ *   `HEAD`, or `TRACE` request with a body throws `IllegalArgumentException`.
  */
 @ConsistentCopyVisibility
 public data class Request private constructor(
@@ -247,16 +249,30 @@ public data class Request private constructor(
         /**
          * Builds the [Request].
          *
+         * A body set on a method that forbids one ([Method.permitsRequestBody] is `false` —
+         * `GET`, `HEAD`, `TRACE`) is rejected here rather than passed to a transport: the two
+         * reference transports disagree on the case (OkHttp throws, the JDK builder drops the
+         * body and may consume a single-use stream for nothing), so the SDK fails fast at
+         * construction with one consistent error instead.
+         *
          * @return The built request.
          * @throws IllegalStateException If a required field is missing.
+         * @throws IllegalArgumentException If a body is set on a method that forbids one
+         *         ([Method.GET], [Method.HEAD], or [Method.TRACE]).
          */
-        override fun build(): Request =
-            Request(
-                method = checkRequired("method", method),
+        override fun build(): Request {
+            val resolvedMethod = checkRequired("method", method)
+            require(body == null || resolvedMethod.permitsRequestBody) {
+                "$resolvedMethod must not carry a request body; remove the body or use a " +
+                    "method that permits one (e.g. POST/PUT/PATCH)."
+            }
+            return Request(
+                method = resolvedMethod,
                 url = checkRequired("url", url),
                 headers = headersBuilder.build(),
                 body = body,
             )
+        }
     }
 
     public companion object {

sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/request/MethodTest.kt

@@ -49,4 +49,18 @@ class MethodTest {
     fun `entries enumerates exactly nine methods`() {
         assertEquals(9, Method.entries.size)
     }
+
+    @Test
+    fun `permitsRequestBody is false only for GET HEAD TRACE and CONNECT`() {
+        assertEquals(false, Method.GET.permitsRequestBody)
+        assertEquals(false, Method.HEAD.permitsRequestBody)
+        assertEquals(false, Method.TRACE.permitsRequestBody)
+        assertEquals(false, Method.CONNECT.permitsRequestBody)
+
+        assertEquals(true, Method.POST.permitsRequestBody)
+        assertEquals(true, Method.PUT.permitsRequestBody)
+        assertEquals(true, Method.PATCH.permitsRequestBody)
+        assertEquals(true, Method.DELETE.permitsRequestBody)
+        assertEquals(true, Method.OPTIONS.permitsRequestBody)
+    }
 }

sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/request/RequestTest.kt

@@ -219,6 +219,69 @@ class RequestTest {
         assertEquals("url is required", ex.message)
     }
 
+    // ---------------------------------------------------------------------
+    // Body / method compatibility — a body on a body-forbidden method is
+    // rejected at build time so transports never have to disagree on it.
+    // ---------------------------------------------------------------------
+
+    @Test
+    fun `build rejects a body on a body-forbidden method`() {
+        for (method in listOf(Method.GET, Method.HEAD, Method.TRACE)) {
+            val ex =
+                assertFailsWith<IllegalArgumentException>("expected rejection for $method") {
+                    Request.builder()
+                        .url("https://example.test")
+                        .method(method)
+                        .body(RequestBody.create("x", null))
+                        .build()
+                }
+            assertTrue(
+                ex.message!!.contains("$method must not carry a request body"),
+                "unexpected message for $method: ${ex.message}",
+            )
+        }
+    }
+
+    @Test
+    fun `build allows a body on a body-permitting method`() {
+        for (method in listOf(Method.POST, Method.PUT, Method.PATCH, Method.DELETE, Method.OPTIONS)) {
+            val payload = RequestBody.create("x", null)
+            val req =
+                Request.builder()
+                    .url("https://example.test")
+                    .method(method)
+                    .body(payload)
+                    .build()
+            assertEquals(payload, req.body, "body should be retained for $method")
+        }
+    }
+
+    @Test
+    fun `build allows a body-less body-forbidden method`() {
+        for (method in listOf(Method.GET, Method.HEAD, Method.TRACE)) {
+            val req =
+                Request.builder()
+                    .url("https://example.test")
+                    .method(method)
+                    .build()
+            assertNull(req.body, "body should stay null for $method")
+        }
+    }
+
+    @Test
+    fun `newBuilder switching to a body-forbidden method while a body is set is rejected`() {
+        val post =
+            Request.builder()
+                .url("https://example.test")
+                .method(Method.POST)
+                .body(RequestBody.create("x", null))
+                .build()
+
+        assertFailsWith<IllegalArgumentException> {
+            post.newBuilder().method(Method.GET).build()
+        }
+    }
+
     // ---------------------------------------------------------------------
     // Equality — compares url by external form, never resolving DNS.
     // ---------------------------------------------------------------------

sdk-transport-jdkhttp/src/main/kotlin/org/dexpace/sdk/transport/jdkhttp/internal/RequestAdapter.kt

@@ -61,10 +61,12 @@ internal class RequestAdapter(
      * that take a [HttpRequest.BodyPublisher] and those that force [HttpRequest.BodyPublishers.noBody]
      * reflects the HTTP semantics enforced by the JDK builder:
      *
-     *  - `GET` / `HEAD` / `TRACE` — no body. The JDK throws if a non-noBody publisher is supplied
-     *    on `HEAD`, so the adapter sends `noBody()` for these methods regardless of what the SDK
-     *    request carried. Callers passing a body to a GET/HEAD/TRACE request will see the body
-     *    silently dropped here.
+     *  - `GET` / `HEAD` / `TRACE` — no body. [Method.permitsRequestBody] is `false` for these, so
+     *    `Request.RequestBuilder.build` rejects a body on them at construction; an SDK request can
+     *    never carry one here. The adapter sends `noBody()` unconditionally rather than adapting
+     *    `request.body` — there is nothing to adapt, and `noBody()` consumes nothing (the previous
+     *    code adapted then discarded the publisher, which for a small body drained a consume-once
+     *    `writeTo` for nothing).
      *  - `POST` / `PUT` / `PATCH` / `DELETE` / `OPTIONS` — body publisher passed through. `DELETE`
      *    and `OPTIONS` with a body are unusual but permitted by HTTP and the JDK builder.
      *  - `CONNECT` — rejected; see [adapt]'s KDoc.
@@ -73,16 +75,15 @@ internal class RequestAdapter(
         builder: HttpRequest.Builder,
         request: SdkRequest,
     ) {
-        val publisher = BodyPublishers.adaptBody(request.body)
         when (request.method) {
             Method.GET -> builder.GET()
             Method.HEAD -> builder.method("HEAD", HttpRequest.BodyPublishers.noBody())
             Method.TRACE -> builder.method("TRACE", HttpRequest.BodyPublishers.noBody())
-            Method.POST -> builder.POST(publisher)
-            Method.PUT -> builder.PUT(publisher)
-            Method.DELETE -> builder.method("DELETE", publisher)
-            Method.PATCH -> builder.method("PATCH", publisher)
-            Method.OPTIONS -> builder.method("OPTIONS", publisher)
+            Method.POST -> builder.POST(BodyPublishers.adaptBody(request.body))
+            Method.PUT -> builder.PUT(BodyPublishers.adaptBody(request.body))
+            Method.DELETE -> builder.method("DELETE", BodyPublishers.adaptBody(request.body))
+            Method.PATCH -> builder.method("PATCH", BodyPublishers.adaptBody(request.body))
+            Method.OPTIONS -> builder.method("OPTIONS", BodyPublishers.adaptBody(request.body))
             Method.CONNECT -> throw IllegalArgumentException(
                 "java.net.http.HttpClient does not support user-issued CONNECT requests. " +
                     "Configure a proxy on the underlying HttpClient instead.",

sdk-transport-jdkhttp/src/test/kotlin/org/dexpace/sdk/transport/jdkhttp/JdkHttpTransportTest.kt

@@ -114,6 +114,43 @@ class JdkHttpTransportTest {
         assertEquals("/echo", recorded.url.encodedPath)
     }
 
+    // -------- body on a body-forbidden method (GET/HEAD/TRACE) --------
+
+    @Test
+    fun `bodyOnForbiddenMethodIsRejectedBeforeDispatch`() {
+        // The JDK builder ignores a body on GET/HEAD/TRACE, silently dropping it — and the eager
+        // BodyPublishers path would already have drained a small consume-once body into a byte
+        // array that is then discarded. The SDK rejects the body at request construction
+        // (Request.RequestBuilder.build), so such a request is never built and never reaches the
+        // transport, and no body is consumed for nothing.
+        for (method in listOf(Method.GET, Method.HEAD, Method.TRACE)) {
+            assertFailsWith<IllegalArgumentException>("expected rejection for $method") {
+                Request.builder()
+                    .method(method)
+                    .url(server.url("/forbidden-body").toUrl())
+                    .body(RequestBody.create("x", CommonMediaTypes.TEXT_PLAIN))
+                    .build()
+            }
+        }
+    }
+
+    @Test
+    fun `bodylessGetDispatchesWithNoBody`() {
+        server.enqueue(MockResponse.Builder().code(200).build())
+        val request =
+            Request.builder()
+                .method(Method.GET)
+                .url(server.url("/bodyless-get").toUrl())
+                .build()
+        transport.execute(request).use { response ->
+            assertEquals(200, response.status.code)
+        }
+        val recorded = server.takeRequest()
+        assertEquals("GET", recorded.method)
+        assertEquals("", recorded.body?.utf8() ?: "")
+        assertEquals("/bodyless-get", recorded.url.encodedPath)
+    }
+
     // -------- async golden paths --------
 
     @Test

sdk-transport-okhttp/src/main/kotlin/org/dexpace/sdk/transport/okhttp/internal/RequestAdapter.kt

@@ -32,7 +32,10 @@ import org.dexpace.sdk.core.http.request.RequestBody as SdkRequestBody
  *     makes a body optional for every method, so a body-less POST is a valid request; to keep
  *     it from crashing with `IllegalArgumentException`, the adapter substitutes a zero-length
  *     body for those methods (see [requiresRequestBody]). GET/HEAD/OPTIONS/TRACE/DELETE keep
- *     their `null` body.
+ *     their `null` body. The inverse case — a body on a body-forbidden method, which OkHttp's
+ *     `Request.Builder.method` rejects with `IllegalArgumentException("method GET must not have a
+ *     request body.")` — cannot reach here: `Request.RequestBuilder.build` rejects a body on
+ *     GET/HEAD/TRACE at construction, so `request.body` is always `null` for those methods.
  *
  * The adapter is stateless — the [logger] is the only field it carries so the DEBUG log
  * naming dropped headers attributes to the transport.

sdk-transport-okhttp/src/test/kotlin/org/dexpace/sdk/transport/okhttp/OkHttpTransportTest.kt

@@ -159,6 +159,43 @@ class OkHttpTransportTest {
         assertEquals("0", recorded.headers["Content-Length"], "empty body must report zero length")
     }
 
+    // -------- body on a body-forbidden method (GET/HEAD/TRACE) --------
+
+    @Test
+    fun bodyOnForbiddenMethodIsRejectedBeforeDispatch() {
+        // OkHttp's Request.Builder.method throws IllegalArgumentException("method GET must not
+        // have a request body.") for a GET carrying a body. That unchecked exception would escape
+        // execute's @Throws(IOException) contract. The SDK rejects the body at request
+        // construction (Request.RequestBuilder.build), so such a request is never built and never
+        // reaches the transport.
+        for (method in listOf(Method.GET, Method.HEAD, Method.TRACE)) {
+            assertFailsWith<IllegalArgumentException>("expected rejection for $method") {
+                Request.builder()
+                    .method(method)
+                    .url(server.url("/forbidden-body").toUrl())
+                    .body(RequestBody.create("x", null))
+                    .build()
+            }
+        }
+    }
+
+    @Test
+    fun bodylessGetDispatchesWithNoBody() {
+        server.enqueue(MockResponse.Builder().code(200).build())
+        val request =
+            Request.builder()
+                .method(Method.GET)
+                .url(server.url("/bodyless-get").toUrl())
+                .build()
+        transport.execute(request).use { response ->
+            assertEquals(200, response.status.code)
+        }
+        val recorded = server.takeRequest()
+        assertEquals("GET", recorded.method)
+        assertEquals("", recorded.body?.utf8() ?: "")
+        assertEquals("/bodyless-get", recorded.url.encodedPath)
+    }
+
     // -------- async golden paths --------
 
     @Test
@@ -195,15 +232,16 @@ class OkHttpTransportTest {
 
     @Test
     fun executeAsyncDeliversAdaptationFailureThroughFuture() {
-        // A body on a method OkHttp forbids one for (GET) makes request adaptation throw
-        // synchronously inside executeAsync. The contract is that executeAsync completes
-        // exceptionally on error, so the failure must arrive through the returned future — a
-        // future-composing caller's .exceptionally/.handle would never observe a synchronous throw.
+        // A non-http(s) URL scheme makes request adaptation throw synchronously inside
+        // executeAsync: java.net.URL accepts an ftp:// URL, but OkHttp's Request.Builder.url
+        // rejects any scheme other than http/https with IllegalArgumentException. The contract is
+        // that executeAsync completes exceptionally on error, so the failure must arrive through
+        // the returned future — a future-composing caller's .exceptionally/.handle would never
+        // observe a synchronous throw.
         val request =
             Request.builder()
                 .method(Method.GET)
-                .url(server.url("/async-adapt-fail").toUrl())
-                .body(RequestBody.create("x".toByteArray()))
+                .url(URL("ftp://example.test/async-adapt-fail"))
                 .build()
         // Must return a future rather than throwing on the caller's thread.
         val future = transport.executeAsync(request)