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

PR: #135

Author: OmarAlJarrah

sdk-core/api/sdk-core.api

@@ -1033,6 +1033,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,30 @@ 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 this SDK permits the method to carry a request body.
+ *   `false` for `GET`, `HEAD`, `TRACE`, and `CONNECT`. Of these only `TRACE` is forbidden a body
+ *   outright by HTTP — a TRACE request "MUST NOT send content" (RFC 9110 §9.3.8); for `GET`/`HEAD`
+ *   (§9.3.1/§9.3.2) and `CONNECT` (§9.3.6) a request payload has no generally defined semantics
+ *   and is discouraged rather than illegal. The SDK rejects a body on all four as one consistent
+ *   policy, because the reference transports otherwise diverge on the case — OkHttp throws
+ *   `IllegalArgumentException`, the JDK builder silently ignores the body — so it is rejected once
+ *   at request construction (see `Request.RequestBuilder.build`) rather than left to behave
+ *   differently 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`, `TRACE`, or `CONNECT` request with a body throws `IllegalArgumentException`.
  */
 @ConsistentCopyVisibility
 public data class Request private constructor(
@@ -117,12 +119,16 @@ public data class Request private constructor(
             }
 
         /**
-         * Sets the request body.
+         * Sets the request body, or clears it when [body] is `null`.
          *
-         * @param body The request body.
+         * Passing `null` is the way to drop a body carried over by [newBuilder] — for example
+         * when downgrading a body-carrying request to `GET`, `HEAD`, `TRACE`, or `CONNECT`, whose
+         * [build] rejects a non-`null` body ([Method.permitsRequestBody] is `false` for them).
+         *
+         * @param body The request body, or `null` to clear any previously-set body.
          * @return This builder.
          */
-        public fun body(body: RequestBody): RequestBuilder =
+        public fun body(body: RequestBody?): RequestBuilder =
             apply {
                 this.body = body
             }
@@ -247,16 +253,32 @@ 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`, `CONNECT`) 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. To downgrade a body-carrying request to
+         * one of these methods, clear the body first with `body(null)`.
+         *
          * @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], [Method.TRACE], or [Method.CONNECT]).
          */
-        override fun build(): Request =
-            Request(
-                method = checkRequired("method", method),
-                url = checkRequired("url", url),
+        override fun build(): Request {
+            val resolvedMethod = checkRequired("method", method)
+            val resolvedUrl = checkRequired("url", url)
+            require(body == null || resolvedMethod.permitsRequestBody) {
+                "$resolvedMethod must not carry a request body; remove the body or use a " +
+                    "method that permits one (POST, PUT, PATCH, DELETE, or OPTIONS)."
+            }
+            return Request(
+                method = resolvedMethod,
+                url = resolvedUrl,
                 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,102 @@ 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, Method.CONNECT)) {
+            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, Method.CONNECT)) {
+            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()
+        }
+    }
+
+    @Test
+    fun `body null clears a previously-set body`() {
+        val req =
+            Request.builder()
+                .url("https://example.test")
+                .method(Method.POST)
+                .body(RequestBody.create("x", null))
+                .body(null)
+                .build()
+        assertNull(req.body, "body(null) should clear the previously-set body")
+    }
+
+    @Test
+    fun `newBuilder downgrade to a body-forbidden method succeeds after clearing the body`() {
+        val post =
+            Request.builder()
+                .url("https://example.test")
+                .method(Method.POST)
+                .body(RequestBody.create("x", null))
+                .build()
+
+        // Clearing the body with body(null) is the supported way to downgrade a body-carrying
+        // request to a method that forbids one.
+        val get =
+            post.newBuilder()
+                .method(Method.GET)
+                .body(null)
+                .build()
+
+        assertEquals(Method.GET, get.method)
+        assertNull(get.body, "downgraded GET must not retain the original body")
+    }
+
     // ---------------------------------------------------------------------
     // 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,13 @@ 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 (and
+     *    for `CONNECT`, which is rejected outright in its own branch below), 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 +76,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

@@ -123,6 +123,43 @@ class JdkHttpTransportTest {
         assertEquals("/echo", recorded.url.encodedPath)
     }
 
+    // -------- body on a body-forbidden method (GET/HEAD/TRACE/CONNECT) --------
+
+    @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, Method.CONNECT)) {
+            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

@@ -31,8 +31,11 @@ import org.dexpace.sdk.core.http.request.RequestBody as SdkRequestBody
  *     body for POST/PUT/PATCH (the methods OkHttp treats as requiring one). The SDK model
  *     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.
+ *     body for those methods (see [requiresRequestBody]). GET/HEAD/OPTIONS/TRACE/DELETE/CONNECT
+ *     keep 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/CONNECT 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.