From 2d7f5bdd630af770800182dc8732c59608213eee Mon Sep 17 00:00:00 2001 From: River Date: Wed, 29 Jun 2022 12:08:35 -0700 Subject: [PATCH 1/4] Add Better Path Handling Directly forward the uri when it's a path instead of trying to parse it. Also clean up isOriginForm/isAsteriskForm as their logic seems to be incorrect (for example, URI#getSchemeSpecificPart() never returns null so they both return false trivially). They should probably be edited themselves in a later commit (and possibly used again here when correct). Since this was always returning true, it's unclear if authority logic is still correct after this change. --- .../codec/http2/HttpConversionUtil.java | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) 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 edb54dca1d7..b64511c7592 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 @@ -49,8 +49,6 @@ import static io.netty.handler.codec.http.HttpResponseStatus.parseLine; import static io.netty.handler.codec.http.HttpScheme.HTTP; import static io.netty.handler.codec.http.HttpScheme.HTTPS; -import static io.netty.handler.codec.http.HttpUtil.isAsteriskForm; -import static io.netty.handler.codec.http.HttpUtil.isOriginForm; import static io.netty.handler.codec.http2.Http2Error.PROTOCOL_ERROR; import static io.netty.handler.codec.http2.Http2Exception.connectionError; import static io.netty.handler.codec.http2.Http2Exception.streamError; @@ -435,16 +433,19 @@ public static Http2Headers toHttp2Headers(HttpMessage in, boolean validateHeader final Http2Headers out = new DefaultHttp2Headers(validateHeaders, inHeaders.size()); if (in instanceof HttpRequest) { HttpRequest request = (HttpRequest) in; - URI requestTargetUri = URI.create(request.uri()); - out.path(toHttp2Path(requestTargetUri)); - out.method(request.method().asciiName()); - setHttp2Scheme(inHeaders, requestTargetUri, out); - - if (!isOriginForm(requestTargetUri) && !isAsteriskForm(requestTargetUri)) { + if (request.uri().startsWith("/") || "*".equals(request.uri())) { + // Origin or asterisk form + out.path(request.uri()); + setHttp2Scheme(inHeaders, out); + } else { + URI requestTargetUri = URI.create(request.uri()); + out.path(toHttp2Path(requestTargetUri)); // Attempt to take from HOST header before taking from the request-line String host = inHeaders.getAsString(HttpHeaderNames.HOST); - setHttp2Authority(host == null || host.isEmpty() ? requestTargetUri.getAuthority() : host, out); + setHttp2Authority(isNullOrEmpty(host) ? requestTargetUri.getAuthority() : host, out); + setHttp2Scheme(inHeaders, requestTargetUri, out); } + out.method(request.method().asciiName()); } else if (in instanceof HttpResponse) { HttpResponse response = (HttpResponse) in; out.status(response.status().codeAsText()); From 3f18a02d536c81c93cdbff77a8d0c9af71a9af4e Mon Sep 17 00:00:00 2001 From: River Date: Wed, 29 Jun 2022 12:18:02 -0700 Subject: [PATCH 2/4] Add Tests Add tests to ensure request conversion behaves as expected. --- .../codec/http2/HttpConversionUtilTest.java | 34 +++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/HttpConversionUtilTest.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/HttpConversionUtilTest.java index debfc565a77..7b2a30b2e3f 100644 --- a/codec-http2/src/test/java/io/netty/handler/codec/http2/HttpConversionUtilTest.java +++ b/codec-http2/src/test/java/io/netty/handler/codec/http2/HttpConversionUtilTest.java @@ -172,6 +172,40 @@ public void stripConnectionNomineesWithCsv() { HttpConversionUtil.toHttp2Headers(inHeaders, out); assertEquals(1, out.size()); assertSame("world", out.get("hello")); + } + + @Test + public void handlesRequest() throws Exception { + boolean validateHeaders = true; + HttpRequest msg = new DefaultHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.GET, "http://example.com/path/to/something", validateHeaders); + HttpHeaders inHeaders = msg.headers(); + inHeaders.add(CONNECTION, "foo, bar"); + inHeaders.add("foo", "baz"); + inHeaders.add("bar", "qux"); + inHeaders.add("hello", "world"); + Http2Headers out = HttpConversionUtil.toHttp2Headers(msg, validateHeaders); + assertEquals("/path/to/something", out.path()); + assertEquals("http", out.scheme()); + assertEquals("example.com", out.authority()); + assertEquals(HttpMethod.GET..asciiName(), out.method()); + } + + @Test + public void handlesRequestWithDoubleSlashPath() throws Exception { + boolean validateHeaders = true; + HttpRequest msg = new DefaultHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.GET, "//path/to/something", validateHeaders); + HttpHeaders inHeaders = msg.headers(); + inHeaders.add(CONNECTION, "foo, bar"); + inHeaders.add(HOST, "example.com"); + inHeaders.add(HttpConversionUtil.ExtensionHeaderNames.SCHEME.text(), "http"); + inHeaders.add("foo", "baz"); + inHeaders.add("bar", "qux"); + inHeaders.add("hello", "world"); + Http2Headers out = HttpConversionUtil.toHttp2Headers(msg, validateHeaders); + assertEquals("//path/to/something", out.path()); + assertEquals("http", out.scheme()); + assertEquals("example.com", out.authority()); + assertEquals(HttpMethod.GET..asciiName(), out.method()); } @Test From e7e2643278cda5c925717c6a7a90d20e891fe2db Mon Sep 17 00:00:00 2001 From: River Date: Wed, 29 Jun 2022 12:43:54 -0700 Subject: [PATCH 3/4] Fix Typo in Test --- .../io/netty/handler/codec/http2/HttpConversionUtilTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/HttpConversionUtilTest.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/HttpConversionUtilTest.java index 7b2a30b2e3f..d8fad1bf96f 100644 --- a/codec-http2/src/test/java/io/netty/handler/codec/http2/HttpConversionUtilTest.java +++ b/codec-http2/src/test/java/io/netty/handler/codec/http2/HttpConversionUtilTest.java @@ -187,7 +187,7 @@ public void handlesRequest() throws Exception { assertEquals("/path/to/something", out.path()); assertEquals("http", out.scheme()); assertEquals("example.com", out.authority()); - assertEquals(HttpMethod.GET..asciiName(), out.method()); + assertEquals(HttpMethod.GET.asciiName(), out.method()); } @Test @@ -205,7 +205,7 @@ public void handlesRequestWithDoubleSlashPath() throws Exception { assertEquals("//path/to/something", out.path()); assertEquals("http", out.scheme()); assertEquals("example.com", out.authority()); - assertEquals(HttpMethod.GET..asciiName(), out.method()); + assertEquals(HttpMethod.GET.asciiName(), out.method()); } @Test From b705c3bf41012945c418fb13f3a674871bee1c98 Mon Sep 17 00:00:00 2001 From: johnnyjacobs Date: Wed, 29 Jun 2022 20:59:27 -0700 Subject: [PATCH 4/4] Fix Test Failures Fix test failures. This requires refactoring the authority pathway to reflect what it was in practice before (though not what the logic implied). Specifically, the logic before never set the HTTP2 Authority if the path was in origin or asterisk form (although due to those methods always returning false this never affected output). --- .../codec/http2/HttpConversionUtil.java | 15 +++++++---- .../codec/http2/HttpConversionUtilTest.java | 26 +++++++++---------- 2 files changed, 23 insertions(+), 18 deletions(-) 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 b64511c7592..9b677b5170b 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 @@ -433,18 +433,19 @@ public static Http2Headers toHttp2Headers(HttpMessage in, boolean validateHeader final Http2Headers out = new DefaultHttp2Headers(validateHeaders, inHeaders.size()); if (in instanceof HttpRequest) { HttpRequest request = (HttpRequest) in; + String host = inHeaders.getAsString(HttpHeaderNames.HOST); if (request.uri().startsWith("/") || "*".equals(request.uri())) { // Origin or asterisk form - out.path(request.uri()); + out.path(new AsciiString(request.uri())); setHttp2Scheme(inHeaders, out); } else { URI requestTargetUri = URI.create(request.uri()); out.path(toHttp2Path(requestTargetUri)); - // Attempt to take from HOST header before taking from the request-line - String host = inHeaders.getAsString(HttpHeaderNames.HOST); - setHttp2Authority(isNullOrEmpty(host) ? requestTargetUri.getAuthority() : host, out); + // Take from the request-line if HOST header was empty + host = isNullOrEmpty(host) ? requestTargetUri.getAuthority() : host; setHttp2Scheme(inHeaders, requestTargetUri, out); } + setHttp2Authority(host, out); out.method(request.method().asciiName()); } else if (in instanceof HttpResponse) { HttpResponse response = (HttpResponse) in; @@ -603,9 +604,13 @@ static void setHttp2Authority(String authority, Http2Headers out) { } } + private static void setHttp2Scheme(HttpHeaders in, Http2Headers out) { + setHttp2Scheme(in, URI.create(""), out); + } + private static void setHttp2Scheme(HttpHeaders in, URI uri, Http2Headers out) { String value = uri.getScheme(); - if (value != null) { + if (!isNullOrEmpty(value)) { out.scheme(new AsciiString(value)); return; } diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/HttpConversionUtilTest.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/HttpConversionUtilTest.java index d8fad1bf96f..bd442c36086 100644 --- a/codec-http2/src/test/java/io/netty/handler/codec/http2/HttpConversionUtilTest.java +++ b/codec-http2/src/test/java/io/netty/handler/codec/http2/HttpConversionUtilTest.java @@ -16,6 +16,7 @@ package io.netty.handler.codec.http2; import io.netty.handler.codec.http.DefaultHttpHeaders; +import io.netty.handler.codec.http.DefaultHttpRequest; import io.netty.handler.codec.http.HttpHeaders; import io.netty.handler.codec.http.HttpMethod; import io.netty.handler.codec.http.HttpRequest; @@ -172,39 +173,38 @@ public void stripConnectionNomineesWithCsv() { HttpConversionUtil.toHttp2Headers(inHeaders, out); assertEquals(1, out.size()); assertSame("world", out.get("hello")); - } + } @Test public void handlesRequest() throws Exception { boolean validateHeaders = true; - HttpRequest msg = new DefaultHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.GET, "http://example.com/path/to/something", validateHeaders); + HttpRequest msg = new DefaultHttpRequest( + HttpVersion.HTTP_1_1, HttpMethod.GET, "http://example.com/path/to/something", validateHeaders); HttpHeaders inHeaders = msg.headers(); inHeaders.add(CONNECTION, "foo, bar"); - inHeaders.add("foo", "baz"); - inHeaders.add("bar", "qux"); inHeaders.add("hello", "world"); Http2Headers out = HttpConversionUtil.toHttp2Headers(msg, validateHeaders); - assertEquals("/path/to/something", out.path()); - assertEquals("http", out.scheme()); - assertEquals("example.com", out.authority()); + assertEquals(new AsciiString("/path/to/something"), out.path()); + assertEquals(new AsciiString("http"), out.scheme()); + assertEquals(new AsciiString("example.com"), out.authority()); assertEquals(HttpMethod.GET.asciiName(), out.method()); + assertEquals("world", out.get("hello")); } @Test public void handlesRequestWithDoubleSlashPath() throws Exception { boolean validateHeaders = true; - HttpRequest msg = new DefaultHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.GET, "//path/to/something", validateHeaders); + HttpRequest msg = new DefaultHttpRequest( + HttpVersion.HTTP_1_1, HttpMethod.GET, "//path/to/something", validateHeaders); HttpHeaders inHeaders = msg.headers(); inHeaders.add(CONNECTION, "foo, bar"); inHeaders.add(HOST, "example.com"); inHeaders.add(HttpConversionUtil.ExtensionHeaderNames.SCHEME.text(), "http"); - inHeaders.add("foo", "baz"); - inHeaders.add("bar", "qux"); inHeaders.add("hello", "world"); Http2Headers out = HttpConversionUtil.toHttp2Headers(msg, validateHeaders); - assertEquals("//path/to/something", out.path()); - assertEquals("http", out.scheme()); - assertEquals("example.com", out.authority()); + assertEquals(new AsciiString("//path/to/something"), out.path()); + assertEquals(new AsciiString("http"), out.scheme()); + assertEquals(new AsciiString("example.com"), out.authority()); assertEquals(HttpMethod.GET.asciiName(), out.method()); }