Repository navigation
verify jws and jwe bodies sent with DELETE and other bodyless methods - #3670
rootvector2 wants to merge 3 commits into
Conversation
The JOSE request filters returned before reading the entity for DELETE, GET, HEAD, OPTIONS and TRACE, and the client response filters did the same for responses to DELETE, HEAD and OPTIONS, so a body sent with those methods was never verified or decrypted. Skip them only when there is no entity.
coheigea
left a comment
There was a problem hiding this comment.
Findings, most severe first:
- Possible way to skip verification (needs checking per server). Whether a request body gets verified now depends on hasEntity(). On the server side, that method returns false as soon as the client’s own Content-Length header says 0, without looking at the stream. A request with Content-Length: 0 plus Transfer-Encoding: chunked and an unsigned body could skip verification, if the servlet container reads the chunked body but keeps the stale header. Whether this works depends on the container. The filters should check the stream itself, not the header.
- Subclasses that override isMethodWithNoContent change behaviour without warning. It’s protected and used to mean “never process this method”. Now it means “skip only if the body is also empty”. Overrides that relied on the old meaning will start failing with 400 or an exception.
- The same skip condition is copied into 8 classes. It would be safer as one helper in the abstract base classes. The method check is also mostly redundant now, because every filter already returns early on empty content.
- Test gaps. The client test fakes hasEntity() instead of using the real ClientResponseContextImpl, so the header shortcut is never exercised. Four of the changed filters have no tests: the JWE client response filter and the JWS JSON, JWE JSON client and JWE JSON server filters.
- Small extra cost on every GET. Those requests used to return immediately. Now each one buffers and probes the request stream through hasEntity().
isMethodWithNoContent now returns false by default and lives in the four abstract base classes, so every filter reads the entity whatever the method and relies on the existing empty-content return. Subclasses that override it keep its old meaning: the method is skipped entirely. The client tests use the real ClientRequestContextImpl and ClientResponseContextImpl, every one of the eight filters has a test, and a DELETE body sent with Content-Length: 0 is still verified.
|
Pushed a rework that takes
The JOSE systests in |
coheigea
left a comment
There was a problem hiding this comment.
New findings, most severe first:
- NPE when there is no entity stream. GET, HEAD and OPTIONS requests now reach IOUtils.readStringFromStream / readBytesFromStream, which reject a null stream. ContainerRequestContextImpl.getEntityStream() can return null: for example, a GET with no body over the local transport never sets an input stream on the server message. That request now fails with 500, where before it was skipped. A null check before reading, in all 8 filters, fixes it.
- super.isMethodWithNoContent() now returns false. A subclass that calls super to extend the old method list (for example, to also skip PATCH) quietly stops skipping GET/HEAD/OPTIONS/DELETE.
- The opt-in checkEmptyStream=true setting still skips on Content-Length: 0. It goes through hasEntity(), which trusts the header, for any method including POST. This was already true before the PR and isn’t tested.
- No test covers a missing stream. The test helpers always supply one, so finding 1 isn’t caught.
…he jose filters ContainerRequestContext.getEntityStream() and ClientResponseContext .getEntityStream() can return null, for example for a GET over the local transport or a response without an entity, and IOUtils rejects a null stream. All eight filters now return when there is no stream. checkEmptyStream went through hasEntity(), which returns false on a Content-Length: 0 header without looking at the stream. The filters already read the stream and return on empty content, so the hasEntity() shortcut is dropped and the setting no longer has any effect. The isMethodWithNoContent Javadoc now spells out the old default for subclasses that extend it through super.
|
Pushed 8b5911b.
|
coheigea
left a comment
There was a problem hiding this comment.
Thanks for the updates. A few remaining points:
1. An empty entity stream is closed and left on the message (server filters)
IOUtils.readStringFromStream / readBytesFromStream close the stream they read. When the content is empty, the filters return without putting a fresh stream back. Every body-less GET or DELETE now closes the underlying servlet input stream, which these filters never touched before. Anything that reads the body afterwards (a later filter, a ReaderInterceptor, an optional entity or form parameter) gets a closed stream. On some containers that throws ("Stream closed").
Suggested fix, in JwsContainerRequestFilter, JwsJsonContainerRequestFilter, JweContainerRequestFilter and JweJsonContainerRequestFilter:
if (StringUtils.isEmpty(content)) { // or encryptedContent.length == 0
context.setEntityStream(new ByteArrayInputStream(new byte[0]));
return;
}
2. Same issue in the client response filters
The 4 client filters now read and close the stream for empty DELETE/HEAD/OPTIONS responses too, and don't put a replacement back. A later response.readEntity(...) or another ClientResponseFilter can then hit an IOException where it used to get an empty entity. The same one-line reset fixes it.
3. checkEmptyStream is now a no-op
The getter and setter remain in the 4 abstract base classes, but the flag isn't read anywhere, and the only notice is in the Javadoc. Please add @Deprecated to the accessors so anyone setting it in Spring/Blueprint config gets some signal.
4. Test gap
None of the new tests checks that the entity stream can still be read after an empty body goes through a filter. Asserting that IOUtils.readStringFromStream(message.getContent(InputStream.class)) returns "" without throwing after filtering an empty GET/DELETE would cover points 1 and 2.
JwsContainerRequestFilter,JwsJsonContainerRequestFilter,JweContainerRequestFilterandJweJsonContainerRequestFilterreturn without reading the entity for DELETE, GET, HEAD, OPTIONS and TRACE, and the four client response filters do the same for responses to DELETE, HEAD and OPTIONS, but JAX-RS still reads a body on those methods, so a DELETE with an unsigned or wrongly signed body reaches the resource unverified while a correctly signed one arrives as the raw JWS string (found reviewing the jose-jaxrs filters, the same gap #3632 closed for http-signature); the filters now skip only whenhasEntity()is false.