Quote the multipart boundary when it isn't a token#9527
Conversation
| if (TOKEN.matches(this)) return this | ||
| require('"' !in this && '\\' !in this) { "boundary contains a character that can't be quoted: $this" } |
There was a problem hiding this comment.
I don't really like that this does three separate passes over the string.
And then if it has to quote it creates a string builder to produce a string only to append that to a string builder at the sole call site.
This code isn't in the hot path or anything, but it irks me a little how inefficient it is.
There was a problem hiding this comment.
I figured because the current happy path is 1 pass, then it wasn't too bad.
But 1 pass and terminating early should be possible.
There was a problem hiding this comment.
Changed it to a single pass that writes straight into the buildString, so there's no intermediate string. It stops the token check once it knows the boundary needs quoting, and only keeps scanning to reject a " or \.
| MultipartBody | ||
| .Builder("a\"; charset=\"evil") | ||
| .addPart("Hello, World!".toRequestBody(null)) | ||
| .build() |
There was a problem hiding this comment.
We should only wrap the function which can fail and we need to assert on the message to ensure the code path we want to test is the one being hit (as opposed to some other validation now or in the future).
There was a problem hiding this comment.
Now only build() is wrapped, and it checks the message so the test can't pass on a different IllegalArgumentException.
MultipartBody built its Content-Type without quoting the boundary, so a boundary with a non-token character like ':' failed toMediaType() and threw. Quote it per RFC 2045 section 5.1. A boundary with '"' or '\' can't round-trip through MediaType's quoted-string, so reject those. Fixes square#8068.
eyupcanakman
force-pushed
the
fix/quote-multipart-boundary
branch
from
f88e43f to
6f25d58
Compare
3 participants
Fixes #8068.
MultipartBodybuilds itsContent-Typewithout quoting the boundary. A boundary with a non-token character, like the colon inabc:def, isn't a valid token, sotoMediaType()rejects it andbuild()throws.This quotes the boundary when it isn't a token, per RFC 2045 section 5.1, so the Content-Type parses and round-trips through
parameter("boundary"). A boundary containing a quote or backslash can't round-trip through the quoted-string, so it's rejected instead. Added a test for the colon boundary and one for the rejected case.