Repository: netty/netty
Issue: Support semicolon as a normal character in URI (no longer a parameter separator)

### Expected result
According to the 2014 W3C Recommendation, semicolon is now illegal as a parameter separator in a http URI.
`?foo=bar;baz` means the parameter `foo` will have the value `bar;baz`

see https://www.w3.org/TR/2014/REC-html5-20141028/forms.html#url-encoded-form-data
(thanks to https://stackoverflow.com/questions/3481664/semicolon-as-url-query-separator)

### Current result
The URI decoder now supports the obsolete 1999 W3C recommendation
 `?foo=bar;baz` is decoded as `?foo=bar & baz=`

(see QueryStringDecoder.java, line 230 :  `case '&': case ';':` both characters are handled equally)

### Netty version
tested (through vert.x) on 1.4.19

### JVM version (e.g. `java -version`)
1.8.0_60

### OS version (e.g. `uname -a`)
Windows i686-pc Intel

_Of course this issue is linked to  #3044 and #2896, but as the norm changed, I suppose it's better to open a new issue_

Issue comments:
@slandelle WDYT ?
👍 I've been parsing URIs this way (HTML5's form-url-encoded) for many years in both AHC and Gatling and I don't recall anyone ever complaining that semi-colon wasn't interpreted as a parameter separator.
Note also that when encoding an URL with java URI.tostring(), semicolons are _not_ encoded, so the client has extra replaces to do before sending the request
@slandelle @marcban so I am a bit confused what should be done here... Anyone interested in a PR ?
For me the correction would only be to delete the `case ‘;’ :` on line 230 of QueryStringDecoder.java. But maybe there are tests to correct also. Never done a PR but I can try...
> Note also that when encoding an URL with java URI.tostring(), semicolons are _not_ encoded, so the client has extra replaces to do before sending the request

I suppose you mean: if my server is Netty and uses `QueryStringDecoder`, I have to encode semicolons manually so `QueryStringDecoder` doesn't mess up with parsing, as URI.toString() won't encode them (which is expected).

> For me the correction would only be to delete the `case ‘;’ :` on line 230 of QueryStringDecoder.java. But maybe there are tests to correct also. Never done a PR but I can try...

This is a low hanging fruit so it would be a perfect ft for a first PR indeed :)
1/ exactly!
2/ I have some docs to read first... :-)

Pull request review comments:

Patch that resolved the issue:
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/QueryStringDecoder.java b/codec-http/src/main/java/io/netty/handler/codec/http/QueryStringDecoder.java
index a20126dbbb8..01b747b3f23 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/QueryStringDecoder.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/QueryStringDecoder.java
@@ -68,6 +68,7 @@ public class QueryStringDecoder {
     private final Charset charset;
     private final String uri;
     private final int maxParams;
+    private final boolean semicolonIsNormalChar;
     private int pathEndIdx;
     private String path;
     private Map<String, List<String>> params;
@@ -109,9 +110,19 @@ public QueryStringDecoder(String uri, Charset charset, boolean hasPath) {
      * specified charset.
      */
     public QueryStringDecoder(String uri, Charset charset, boolean hasPath, int maxParams) {
+        this(uri, charset, hasPath, maxParams, false);
+    }
+
+    /**
+     * Creates a new decoder that decodes the specified URI encoded in the
+     * specified charset.
+     */
+    public QueryStringDecoder(String uri, Charset charset, boolean hasPath,
+                              int maxParams, boolean semicolonIsNormalChar) {
         this.uri = checkNotNull(uri, "uri");
         this.charset = checkNotNull(charset, "charset");
         this.maxParams = checkPositive(maxParams, "maxParams");
+        this.semicolonIsNormalChar = semicolonIsNormalChar;
 
         // `-1` means that path end index will be initialized lazily
         pathEndIdx = hasPath ? -1 : 0;
@@ -138,6 +149,14 @@ public QueryStringDecoder(URI uri, Charset charset) {
      * specified charset.
      */
     public QueryStringDecoder(URI uri, Charset charset, int maxParams) {
+        this(uri, charset, maxParams, false);
+    }
+
+    /**
+     * Creates a new decoder that decodes the specified URI encoded in the
+     * specified charset.
+     */
+    public QueryStringDecoder(URI uri, Charset charset, int maxParams, boolean semicolonIsNormalChar) {
         String rawPath = uri.getRawPath();
         if (rawPath == null) {
             rawPath = EMPTY_STRING;
@@ -147,6 +166,7 @@ public QueryStringDecoder(URI uri, Charset charset, int maxParams) {
         this.uri = rawQuery == null? rawPath : rawPath + '?' + rawQuery;
         this.charset = checkNotNull(charset, "charset");
         this.maxParams = checkPositive(maxParams, "maxParams");
+        this.semicolonIsNormalChar = semicolonIsNormalChar;
         pathEndIdx = rawPath.length();
     }
 
@@ -177,7 +197,7 @@ public String path() {
      */
     public Map<String, List<String>> parameters() {
         if (params == null) {
-            params = decodeParams(uri, pathEndIdx(), charset, maxParams);
+            params = decodeParams(uri, pathEndIdx(), charset, maxParams, semicolonIsNormalChar);
         }
         return params;
     }
@@ -204,7 +224,8 @@ private int pathEndIdx() {
         return pathEndIdx;
     }
 
-    private static Map<String, List<String>> decodeParams(String s, int from, Charset charset, int paramsLimit) {
+    private static Map<String, List<String>> decodeParams(String s, int from, Charset charset, int paramsLimit,
+                                                          boolean semicolonIsNormalChar) {
         int len = s.length();
         if (from >= len) {
             return Collections.emptyMap();
@@ -226,8 +247,12 @@ private static Map<String, List<String>> decodeParams(String s, int from, Charse
                     valueStart = i + 1;
                 }
                 break;
-            case '&':
             case ';':
+                if (semicolonIsNormalChar) {
+                    continue;
+                }
+                // fall-through
+            case '&':
                 if (addParam(s, nameStart, valueStart, i, params, charset)) {
                     paramsLimit--;
                     if (paramsLimit == 0) {


Test patch:
diff --git a/codec-http/src/test/java/io/netty/handler/codec/http/QueryStringDecoderTest.java b/codec-http/src/test/java/io/netty/handler/codec/http/QueryStringDecoderTest.java
index a0071c4a70f..5937d66bf24 100644
--- a/codec-http/src/test/java/io/netty/handler/codec/http/QueryStringDecoderTest.java
+++ b/codec-http/src/test/java/io/netty/handler/codec/http/QueryStringDecoderTest.java
@@ -129,6 +129,13 @@ public void testExotic() {
         assertQueryString("/foo?a=1&a=&a=", "/foo?a=1&a&a=");
     }
 
+    @Test
+    public void testSemicolon() {
+        assertQueryString("/foo?a=1;2", "/foo?a=1;2", false);
+        // ";" should be treated as a normal character, see #8855
+        assertQueryString("/foo?a=1;2", "/foo?a=1%3B2", true);
+    }
+
     @Test
     public void testPathSpecific() {
         // decode escaped characters
@@ -225,8 +232,14 @@ public void testUrlDecoding() throws Exception {
     }
 
     private static void assertQueryString(String expected, String actual) {
-        QueryStringDecoder ed = new QueryStringDecoder(expected, CharsetUtil.UTF_8);
-        QueryStringDecoder ad = new QueryStringDecoder(actual, CharsetUtil.UTF_8);
+        assertQueryString(expected, actual, false);
+    }
+
+    private static void assertQueryString(String expected, String actual, boolean semicolonIsNormalChar) {
+        QueryStringDecoder ed = new QueryStringDecoder(expected, CharsetUtil.UTF_8, true,
+                1024, semicolonIsNormalChar);
+        QueryStringDecoder ad = new QueryStringDecoder(actual, CharsetUtil.UTF_8, true,
+                1024, semicolonIsNormalChar);
         Assert.assertEquals(ed.path(), ad.path());
         Assert.assertEquals(ed.parameters(), ad.parameters());
     }