JWE arbitrary content compression#937
Conversation
The JWE content was compressed only when claims was used as payload, but when using arbitrary content, the compression was omitted, while keeping the "zip" header field, leading to decompression failing.
mnylen
force-pushed
the
allow-compress-decompress-for-arbitrary-content
branch
from
700a92a to
c83fbe9
Compare
|
Excellent find!!! I can't believe this was missed, thank you so much 🙏 |
| plaintext = Streams.of(out.toByteArray()); | ||
| } else { | ||
| plaintext = content.toInputStream(); | ||
| writeAndClose("JWE Content", content, out); |
There was a problem hiding this comment.
We have to be careful here: if compression is not enabled, we don't want to write the plaintext Inputstream to an intermediate output stream (and then converted back into an input stream) which would be an unnecessary roundtrip/copy.
This logic should look more like that found in the sign method:
jjwt/impl/src/main/java/io/jsonwebtoken/impl/DefaultJwtBuilder.java
Lines 598 to 605 in c673b76
If possible, it'd be ideal if we can move this logic to a helper method that is shared by both sign and encrypt
There was a problem hiding this comment.
Makes sense.
I wonder though if we should also be a bit more clever with the initial byte array size for ByteArrayOutputStream. Seems a bit wasteful to do 8 KB byte array allocation if the content is only few hundred bytes. At least if the payload has byte array / string, we could use the initial payload size as the starting point, as even with compression, we shouldn't (at least usually) go above that size.
Edit: Well, maybe I'll try to make a different PR about the initial sizes later. Noticed that writeAndClose also creates a 4KB buffer, when it could also just use the initial payload size as the starting point.
lhazlewood
changed the title
| // No claims and not compressed, so just get the direct InputStream: | ||
| payloadStream = Assert.stateNotNull(payload.toInputStream(), "Payload InputStream cannot be null."); | ||
| } | ||
| payloadStream = convertPayloadToInputStream(payload); |
There was a problem hiding this comment.
I'd like to retain the existing names in exception messages so we don't lose context, so I think the helper method needs to also accept a String identifying what is being converted (e.g. JWS Unencoded Payload, JWE Claims), e.g.
private InputStream toInputStream(Payload payload, String name) {
...
}(also probably don't need the word Payload in the method name since it's redundant given it only accepts a Payload data type).
lhazlewood
left a comment
lhazlewood
left a comment
There was a problem hiding this comment.
Just a minor change remaining, and then I think we can merge. Thanks for your continued help!
2 participants
The JWE content was compressed only when claims was used as payload, but when using arbitrary content string/bytes/input stream, the compression was omitted, while keeping the "zip" header field, leading to un-parseable JWE as the decompression failed on non-compressed content.
I couldn't find any evidence that this was intended behavior from the documentation or the JWE spec, so made a fix for this.
See #936
Note that people might have used a similar workaround as I presented in the discussion to manually compress the payload, so fixing this might result in some code breaking somewhere. Might be good to mention in the changelog.