fix: reject a request body on a body-forbidden method (GET/HEAD/TRACE)#135
Open
OmarAlJarrah wants to merge 1 commit into
Open
fix: reject a request body on a body-forbidden method (GET/HEAD/TRACE)#135OmarAlJarrah wants to merge 1 commit into
OmarAlJarrah wants to merge 1 commit into
Conversation
A `Request` could be built with a body on a method that forbids one (GET,
HEAD, TRACE). The two transports then handled it inconsistently and neither
was correct:
- OkHttp's `Request.Builder.method("GET", body)` throws
`IllegalArgumentException("method GET must not have a request body.")`.
That unchecked exception escaped the transport's `@Throws(IOException)`
contract (and the retry pipeline).
- The JDK transport built a `BodyPublisher` from the body and then called
`builder.GET()`, which ignores it. The body was silently dropped — and for
a small body the eager publisher path had already drained a consume-once
`writeTo` into a byte array that was then discarded.
Pick one consistent policy and reject at construction. `Method` now carries a
`permitsRequestBody` flag (`false` for GET/HEAD/TRACE/CONNECT), and
`Request.RequestBuilder.build()` throws `IllegalArgumentException` when a body
is set on a method that forbids one. Failing fast at the model boundary keeps
both transports from diverging: OkHttp can no longer receive a body it would
crash on, and the JDK transport no longer consumes a body it never sends. The
JDK adapter now sends `noBody()` unconditionally for GET/HEAD/TRACE instead of
adapting (and discarding) the publisher.
Adds symmetric coverage in both transport suites plus core tests for the new
validation and the `permitsRequestBody` flag. The new `Method.permitsRequestBody`
getter is the only public-API addition (api snapshot regenerated).
Closes #117
Member
Author
|
Nice cleanup. Centralizing the body-on-forbidden-method check at Worth double-checking
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The
Requestmodel madebodyoptional for every method and ran no method/body compatibility check, so aGET(orHEAD/TRACE) request carrying a body was publicly constructible. The two reference transports then handled that case inconsistently, and neither was correct:Request.Builder.method("GET", body), which throwsIllegalArgumentException("method GET must not have a request body."). That unchecked exception escaped the transport's@Throws(IOException)contract and the retry pipeline.BodyPublisherfrom the body and then calledbuilder.GET(), which ignores it. The body was silently dropped — and for a small body the eager publisher path had already drained a consume-oncewriteTointo a byte array that was then discarded.This is the inverse of the body-less POST/PUT/PATCH case fixed in #82: a body present on a method that forbids one, rather than a body absent on a method that requires one.
Change
Reject at construction, so the two transports never have to disagree:
Methodnow carries apermitsRequestBodyflag —falseforGET,HEAD,TRACE, andCONNECT;truefor the rest. The KDoc cites the relevant RFC 9110 sections.Request.RequestBuilder.build()throwsIllegalArgumentExceptionwhen a body is set on a method whosepermitsRequestBodyisfalse, with a message that names the method and points at the body-permitting alternatives.BodyPublishers.noBody()unconditionally forGET/HEAD/TRACErather than adapting (and discarding) the body — there is no body to adapt, so nothing is consumed for nothing.Rationale
Failing fast at the model boundary is the single consistent policy: OkHttp can no longer receive a body it would crash on, and the JDK transport no longer consumes a single-use body it never sends. The error is raised once, at build time, with a clear message — rather than surfacing differently (or not at all) depending on which transport is wired in.
Public API
One addition: the
Method.permitsRequestBodygetter.sdk-coreapi snapshot regenerated and committed.Tests
sdk-core:buildrejects a body on GET/HEAD/TRACE, allows it on POST/PUT/PATCH/DELETE/OPTIONS, allows a body-less forbidden method, and rejects anewBuilderthat switches a body-carrying request to GET; plus apermitsRequestBodyflag check inMethodTest.Gated build commands run (all passed)
./gradlew :sdk-core:test :sdk-core:ktlintCheck :sdk-core:detekt :sdk-core:apiCheck --no-daemon./gradlew :sdk-transport-okhttp:test :sdk-transport-okhttp:ktlintCheck :sdk-transport-okhttp:detekt :sdk-transport-okhttp:apiCheck --no-daemon./gradlew :sdk-transport-jdkhttp:test :sdk-transport-jdkhttp:ktlintCheck :sdk-transport-jdkhttp:apiCheck --no-daemon(detekt is skipped on this module)Closes #117