Repository: netty/netty
Issue: HttpConversionUtil.toHttp2Headers converts uri to path header unproperly
Labels: defect

Netty version: 4.1.0.Beta6

Context:

When we use HttpConversionUtil.toHttp2Headers, if uri has urlencoded values, they are decoded unproperly in `:path` header. 

I could't find any spec but below in https://httpwg.github.io/specs/rfc7540.html#HttpHeaders

> The :path pseudo-header field includes the path and query parts of the target URI (the path-absolute production and optionally a '?' character followed by the query production (see Sections 3.3 and 3.4 of [RFC3986]). A request in asterisk form includes the value '_' for the :path pseudo-header field.
> This pseudo-header field MUST NOT be empty for http or https URIs; http or https URIs that do not contain a path component MUST include a value of '/'. The exception to this rule is an OPTIONS request for an http or https URI that does not include a path component; these MUST include a :path pseudo-header field with a value of '_' (see [RFC7230], Section 5.3.4).

Steps to reproduce:
1. Create HttpMessage:
   
   ``` java
   String uri = "https://www.anydomain.com/path%2B1?q=query%2B1&q2=query%2B2#fragment%2B1";
   FullHttpRequest request = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.GET, uri);
   ```
2. convert using `HttpConversionUtil.toHttp2Headers`
   
   ``` java
   Http2Headers http2Headers = HttpConversionUtil.toHttp2Headers(request);
   ```
3. get path by `http2Headers.get(HttpNames.PATH)`
   
   ``` java
   assertThat(http2Headers.get(HttpNames.PATH), is("/path%2B1?q=query%2B1&q2=query%2B2#fragment%2B1");
   //it fails, actual value is "/path+1?q=query+1&q2=query+2#fragment+1"
   ```

Opinion:

change `HttpConversionUtil.toHttp2Path` to use `getRawXXX()`

``` java
    private static AsciiString toHttp2Path(URI uri) {
        StringBuilder pathBuilder = new StringBuilder(length(uri.getRawPath()) +
                length(uri.getRawQuery()) + length(uri.getRawFragment()) + 2);
        if (!isNullOrEmpty(uri.getRawPath())) {
            pathBuilder.append(uri.getRawPath());
        }
        if (!isNullOrEmpty(uri.getRawQuery())) {
            pathBuilder.append('?');
            pathBuilder.append(uri.getRawQuery());
        }
        if (!isNullOrEmpty(uri.getRawFragment())) {
            pathBuilder.append('#');
            pathBuilder.append(uri.getRawFragment());
        }
        String path = pathBuilder.toString();
        return path.isEmpty() ? EMPTY_REQUEST_PATH : new AsciiString(path);
    }
```


Issue comments:
@Scottmitch @nmittler can you check?

fixed by https://github.com/netty/netty/pull/4245


Pull request review comments:

Patch that resolved the issue:
diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/HttpConversionUtil.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/HttpConversionUtil.java
index 7163f69f550..3231c50a926 100644
--- a/codec-http2/src/main/java/io/netty/handler/codec/http2/HttpConversionUtil.java
+++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/HttpConversionUtil.java
@@ -353,18 +353,18 @@ public static Http2Headers toHttp2Headers(HttpHeaders inHeaders, boolean validat
      * <a href="https://tools.ietf.org/html/rfc7230#section-5.3">rfc7230, 5.3</a>.
      */
     private static AsciiString toHttp2Path(URI uri) {
-        StringBuilder pathBuilder = new StringBuilder(length(uri.getPath()) +
-                length(uri.getQuery()) + length(uri.getFragment()) + 2);
-        if (!isNullOrEmpty(uri.getPath())) {
-            pathBuilder.append(uri.getPath());
+        StringBuilder pathBuilder = new StringBuilder(length(uri.getRawPath()) +
+                length(uri.getRawQuery()) + length(uri.getRawFragment()) + 2);
+        if (!isNullOrEmpty(uri.getRawPath())) {
+            pathBuilder.append(uri.getRawPath());
         }
-        if (!isNullOrEmpty(uri.getQuery())) {
+        if (!isNullOrEmpty(uri.getRawQuery())) {
             pathBuilder.append('?');
-            pathBuilder.append(uri.getQuery());
+            pathBuilder.append(uri.getRawQuery());
         }
-        if (!isNullOrEmpty(uri.getFragment())) {
+        if (!isNullOrEmpty(uri.getRawFragment())) {
             pathBuilder.append('#');
-            pathBuilder.append(uri.getFragment());
+            pathBuilder.append(uri.getRawFragment());
         }
         String path = pathBuilder.toString();
         return path.isEmpty() ? EMPTY_REQUEST_PATH : new AsciiString(path);


Test patch:
diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/HttpToHttp2ConnectionHandlerTest.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/HttpToHttp2ConnectionHandlerTest.java
index 0377788887d..9d10e0d3bca 100644
--- a/codec-http2/src/test/java/io/netty/handler/codec/http2/HttpToHttp2ConnectionHandlerTest.java
+++ b/codec-http2/src/test/java/io/netty/handler/codec/http2/HttpToHttp2ConnectionHandlerTest.java
@@ -157,6 +157,23 @@ public void testOriginFormRequestTargetHandled() throws Exception {
         verifyHeadersOnly(http2Headers, writePromise, clientChannel.writeAndFlush(request, writePromise));
     }
 
+    @Test
+    public void testOriginFormRequestTargetHandledFromUrlencodedUri() throws Exception {
+        bootstrapEnv(2, 1, 0);
+        final FullHttpRequest request = new DefaultFullHttpRequest(
+                HTTP_1_1, GET, "/where%2B0?q=now%2B0&f=then%2B0#section1%2B0");
+        final HttpHeaders httpHeaders = request.headers();
+        httpHeaders.setInt(HttpConversionUtil.ExtensionHeaderNames.STREAM_ID.text(), 5);
+        httpHeaders.set(HttpConversionUtil.ExtensionHeaderNames.SCHEME.text(), "http");
+        final Http2Headers http2Headers =
+                new DefaultHttp2Headers().method(new AsciiString("GET"))
+                                         .path(new AsciiString("/where%2B0?q=now%2B0&f=then%2B0#section1%2B0"))
+                                         .scheme(new AsciiString("http"));
+
+        ChannelPromise writePromise = newPromise();
+        verifyHeadersOnly(http2Headers, writePromise, clientChannel.writeAndFlush(request, writePromise));
+    }
+
     @Test
     public void testAbsoluteFormRequestTargetHandledFromHeaders() throws Exception {
         bootstrapEnv(2, 1, 0);