Repository: netty/netty
Issue: StringIndexOutOfBoundsException thrown by HttpPostRequestDecoder.splitHeaderContentType() when Content-Type header starts with a semicolon

### Expected behavior
I'm not sure what the desired behavior should be for `HttpPostRequestDecoder.splitHeaderContentType()` when it finds a Content-Type header that starts with a semicolon, but I'm assuming `StringIndexOutOfBoundsException` is not intentional.

### Actual behavior
`HttpPostRequestDecoder.splitHeaderContentType()` throws a `StringIndexOutOfBoundsException` when it parses a Content-Type header that starts with a semicolon `;`. Specifically this line, because the `aEnd` variable is 0 when the Content-Type header starts with a semicolon: https://github.com/netty/netty/blob/00afb19d7a37de21b35ce4f6cb3fa7f74809f2ab/codec-http/src/main/java/io/netty/handler/codec/http/multipart/HttpPostRequestDecoder.java#L278.

### Steps to reproduce
1. Make a request to a Netty HTTP server and pass a Content-Type header that starts with a semicolon `;`. I'm not sure if there are HTTP clients that would sanitize this for you and prevent the problem, but I was able to reproduce this with `RestAssured` and a Netty `Bootstrap` acting as a HTTP Client via `HttpClientCodec`, so there are at least a few clients you can use to reproduce.
2. In the Netty server that receives the request, call `HttpPostRequestDecoder.isMultipart(HttpRequest)` or any other code path that ultimately causes `HttpPostRequestDecoder.splitHeaderContentType(String)` to be called with the request's Content-Type header.
3. You'll see a `StringIndexOutOfBoundsException` get thrown.

### Netty version
`4.1.30.Final` (probably others as well)

Issue comments:
@nicmunroe yeah this looks like a bug... Can you provide a fix ?
> @nicmunroe yeah this looks like a bug... Can you provide a fix ?

Possibly - I'm working through CLA issues to make sure I'm cleared to contribute.
Thanks a lot !

> Am 15.11.2018 um 00:22 schrieb Nic Munroe <notifications@github.com>:
> 
> @nicmunroe yeah this looks like a bug... Can you provide a fix ?
> 
> Possibly - I'm working through CLA issues to make sure I'm cleared to contribute.
> 
> —
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub, or mute the thread.

@normanmaurer @nicmunroe Hi guys , ```Content-Type = media-type``` follows from RFC:
```media-type = type "/" subtype *( OWS ";" OWS parameter )```
        ```type  = token```
       ``` subtype  = token```
I think starting content-type value with ';' is not correct, and we should handle this like "unsupported media type".

Sounds good to me @amizurov ... @nicmunroe WDYT ?
The relevant RFC sections (I'm assuming you're referring to RFC 7231 [Section 3.1.1.5](https://tools.ietf.org/html/rfc7231#section-3.1.1.5) and [Section 3.1.1.1](https://tools.ietf.org/html/rfc7231#section-3.1.1.1)) don't specify that you must respond with an error when encountering invalid content-type. In fact when talking about Content-Type in section 3.1.1.5 it says:

> In practice, resource owners do not always properly configure their origin server to provide the correct Content-Type for a given representation, with the result that some clients will examine a payload's content and override the specified type.

It goes on to say this is might be a bad idea and `Implementers are encouraged to provide a means of disabling such "content sniffing" when it is used`, but the point is that the RFC seems to be indicating that it should be expected for Content-Type to sometimes be bad/broken/incorrect.

So I think we could choose to gracefully handle badly-formatted Content-Type here without throwing an error, and it wouldn't go against the RFC (and might even be more in line with the RFC, depending on how you read it).

From a non-RFC perspective it feels weird to me to throw an exception of any sort when calling `HttpPostRequestDecoder.isMultipart(HttpRequest)` (i.e. a malformed content-type header is clearly not multipart - so the method should just return false), and I'm not sure the `splitHeaderContentType(...)` method would be the right place to throw it even if we decided throwing an exception was warranted.

(Unless throwing an exception is not what you meant by "handle this like unsupported media type"? If so I'd need more info on what you meant.)

Ultimately my gut instinct is that this doesn't feel like a fail-fast situation. Thoughts?
@nicmunroe Thanks for your thoughts. 

For clarity, i meant that ``content-type`` value starting with **';'** is incorrect.  And I think when we expect the concrete ``media-type``: (application/json, multipart/form-data ... etc) we should handle request like a ```400 Bad Request``` and responding to client with reason: "unsupported media type" for example. 

This is make sense to the client side to knowns what happens and fix issue also we prevent problems on server.  But how you said - it's depending how we read RFC and what we wont to fix only one method or situation in common ?
That's the thing though - at this point in the code there's no concrete media-type expectation. It's just "parse content-type into an array of multipart stuff and return empty if you can't parse it", or "is this multipart?". This isn't the portion of code where you say "I *expect* this to be multipart, and you should fail with a 400 for me if it's not".

There may be a spot in the code where that expectation is either explicit or implicit in the contract of the method, and in that case I'm all on board with failing fast. I just don't think this is that spot.
@nicmunroe My point of view is how to do this method more informative if it make sense. If no, then you are right - ```isMultipart("bad content") -> false```. 
That's fair. I have no problem with making this method more informative in principal. I'd just prefer a separate issue if there's a desire to make more sweeping functional changes, as adjusting method signatures and such is going to have a ripple effect and end up touching a bunch of code. Instead, at least for this issue, I'd like to just make a targeted fix for the `StringIndexOutOfBoundsException`.
No problem, do that
@amizurov @nicmunroe any update here ?
Hi, @normanmaurer. Oh what the long story, i'll try to fix it soon if @nicmunroe is not opposed to check PR.
Sorry it took a long time to get approval to do a PR, by then I was buried in other stuff, and I was never able to unbury my priority list enough to get to this. :( 

Yeah, I'm happy to check a PR. And/or I still intend to get to this ... someday - I just can't guarantee when that would be.
@nicmunroe could you please check fix
Sorry I was out all last week, but your solution looks good to me! Thank you @amizurov !

Pull request review comments:

Patch that resolved the issue:
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/multipart/HttpPostRequestDecoder.java b/codec-http/src/main/java/io/netty/handler/codec/http/multipart/HttpPostRequestDecoder.java
index 0c106264063..b430471248c 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/multipart/HttpPostRequestDecoder.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/multipart/HttpPostRequestDecoder.java
@@ -140,11 +140,11 @@ protected enum MultiPartStatus {
      * @return True if the request is a Multipart request
      */
     public static boolean isMultipart(HttpRequest request) {
-        if (request.headers().contains(HttpHeaderNames.CONTENT_TYPE)) {
-            return getMultipartDataBoundary(request.headers().get(HttpHeaderNames.CONTENT_TYPE)) != null;
-        } else {
-            return false;
+        String mimeType = request.headers().get(HttpHeaderNames.CONTENT_TYPE);
+        if (mimeType != null && mimeType.startsWith(HttpHeaderValues.MULTIPART_FORM_DATA.toString())) {
+            return getMultipartDataBoundary(mimeType) != null;
         }
+        return false;
     }
 
     /**


Test patch:
diff --git a/codec-http/src/test/java/io/netty/handler/codec/http/multipart/HttpPostRequestDecoderTest.java b/codec-http/src/test/java/io/netty/handler/codec/http/multipart/HttpPostRequestDecoderTest.java
index 6e6e3cbf359..2c3bfe7ed80 100644
--- a/codec-http/src/test/java/io/netty/handler/codec/http/multipart/HttpPostRequestDecoderTest.java
+++ b/codec-http/src/test/java/io/netty/handler/codec/http/multipart/HttpPostRequestDecoderTest.java
@@ -29,6 +29,7 @@
 import io.netty.handler.codec.http.HttpHeaderNames;
 import io.netty.handler.codec.http.HttpHeaderValues;
 import io.netty.handler.codec.http.HttpMethod;
+import io.netty.handler.codec.http.HttpRequest;
 import io.netty.handler.codec.http.HttpVersion;
 import io.netty.handler.codec.http.LastHttpContent;
 import io.netty.util.CharsetUtil;
@@ -734,4 +735,17 @@ public void testNotLeak() {
             assertTrue(request.release());
         }
     }
+
+    @Test
+    public void testMultipartFormDataContentType() {
+        HttpRequest request = new DefaultHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.POST, "/");
+        assertFalse(HttpPostRequestDecoder.isMultipart(request));
+
+        String multipartDataValue = HttpHeaderValues.MULTIPART_FORM_DATA + ";" + "boundary=gc0p4Jq0M2Yt08jU534c0p";
+        request.headers().set(HttpHeaderNames.CONTENT_TYPE, ";" + multipartDataValue);
+        assertFalse(HttpPostRequestDecoder.isMultipart(request));
+
+        request.headers().set(HttpHeaderNames.CONTENT_TYPE, multipartDataValue);
+        assertTrue(HttpPostRequestDecoder.isMultipart(request));
+    }
 }