Multipart boundary should strip quotes

[Thorn1089]

Affects: 5.3.3
Library: spring-web

Although uncommon, some HTTP clients will quote the multipart boundary value. This does appear to be acceptable based on a reading of the RFC. As a specific example, the .NET SDK's HttpClient class will generate a quoted UUID to use as the boundary:

POST /foo HTTP/1.1
Content-Type: multipart/form-data; boundary="7e296554-91ca-4075-ada1-c72043296dd7"
Host: foo.bar.example
Content-Length: <snip>
Expect: 100-continue

--7e296554-91ca-4075-ada1-c72043296dd7
Content-Type: text/plain; charset=utf-8
Content-Disposition: form-data; name=Foo

BAR
--7e296554-91ca-4075-ada1-c72043296dd7--

The problem is the codec shipped with spring-web does not handle this case:

	@Nullable
	private static byte[] boundary(HttpMessage message) {
		MediaType contentType = message.getHeaders().getContentType();
		if (contentType != null) {
			String boundary = contentType.getParameter("boundary");
			if (boundary != null) {
				return boundary.getBytes(StandardCharsets.ISO_8859_1);
			}
		}
		return null;
	}

The code should check the boundary string to see if it starts and ends with an ASCII double-quote ("). If so, it should strip them before creating the byte array to be used later.

See #26615 which led to me discovering this issue.

[michael-o]

This is a non-compliant implementation of MediaType where no difference is made between token and quoted string.

[Thorn1089]

Hi @michael-o -- can you clarify your comment please?

Are you suggesting a quoted boundary value should not be allowed? Based on the longstanding behavior of the .NET HttpClient it seems that it is legal, though uncommon.

Or are you saying that the fix should be in the MediaType class to consistently unquote parameters? That does seem to be how the quality parameter (q) is treated, looking at the code.

[michael-o]

No, Iam not suggesting that: Here is how it should look like by the RFC: https://tools.ietf.org/html/rfc7231#appendix-C

     quoted-string = <quoted-string, see [RFC7230], Section 3.2.6>
     token         = <token, see [RFC7230], Section 3.2.6>

When reading a paremeter MediaType must handle both cases. The quoted string has it purpose to contain characters token cannot contain. I dealing with this in multipart/related. Same rules apply here.

[Thorn1089]

OK, so we agree the current handling is not compliant with the spec then. :)

I can take a crack at a PR for this over the weekend.

[poutsma]

@TomRK1089 No need for a PR, I'll pick this up. Unless it's already done, of course :).

[michael-o]

I believe that the fix is in the wrong place. It should have happened on the media type because many other media types us paremters as well and they are subject to this.

[poutsma]

I believe that the fix is in the wrong place. It should have happened on the media type because many other media types us paremters as well and they are subject to this.

You are right in theory, but such a change could break any application that depends on the current behavior of MediaType, including any subtypes. And that is not a change that I feel comfortable making for the 5.3.5 release.

[Thorn1089]

@poutsma Thank you! When do you think a new SNAPSHOT build will go out for the library? I'm comfortable adding the Spring snapshot repos in the short term to get this fix into my application.

[poutsma]

I think it’s out already, see https://repo.spring.io/snapshot/org/springframework/spring/5.3.5-SNAPSHOT/