From a76f5637580639a2e9519ee85d22227020282995 Mon Sep 17 00:00:00 2001 From: yawkat Date: Mon, 11 Apr 2022 10:50:40 +0200 Subject: [PATCH 1/3] Support absolute-form URI in HTTP requests This changeset drops the scheme and authority parts from the URI in two places where we interact with the netty uri. There is also a workaround for https://github.com/netty/netty/issues/12301 . Fixes #7193 --- .../http/netty/AbstractNettyHttpRequest.java | 29 +++-- .../http/server/netty/NettyHttpRequest.java | 2 +- .../http/server/netty/RequestLineSpec.groovy | 104 ++++++++++++++++++ 3 files changed, 127 insertions(+), 8 deletions(-) create mode 100644 http-server-netty/src/test/groovy/io/micronaut/http/server/netty/RequestLineSpec.groovy diff --git a/http-netty/src/main/java/io/micronaut/http/netty/AbstractNettyHttpRequest.java b/http-netty/src/main/java/io/micronaut/http/netty/AbstractNettyHttpRequest.java index db7cb5f22f0..3dc9935e97f 100644 --- a/http-netty/src/main/java/io/micronaut/http/netty/AbstractNettyHttpRequest.java +++ b/http-netty/src/main/java/io/micronaut/http/netty/AbstractNettyHttpRequest.java @@ -35,6 +35,7 @@ import io.netty.util.DefaultAttributeMap; import java.net.URI; +import java.net.URISyntaxException; import java.nio.charset.Charset; import java.util.Collection; import java.util.Locale; @@ -72,8 +73,22 @@ public abstract class AbstractNettyHttpRequest extends DefaultAttributeMap im public AbstractNettyHttpRequest(io.netty.handler.codec.http.HttpRequest nettyRequest, ConversionService conversionService) { this.nettyRequest = nettyRequest; this.conversionService = conversionService; - String fullUri = nettyRequest.uri(); - this.uri = URI.create(fullUri); + URI fullUri = URI.create(nettyRequest.uri()); + if (fullUri.getAuthority() != null || fullUri.getScheme() != null) { + // http://example.com/foo -> /foo + try { + fullUri = new URI( + null, // scheme + null, // authority + fullUri.getPath(), + fullUri.getQuery(), + fullUri.getFragment() + ); + } catch (URISyntaxException e) { + throw new IllegalArgumentException(e); + } + } + this.uri = fullUri; this.httpMethodName = nettyRequest.method().name(); this.httpMethod = HttpMethod.parse(httpMethodName); } @@ -153,7 +168,7 @@ public HttpParameters getParameters() { synchronized (this) { // double check httpParameters = this.httpParameters; if (httpParameters == null) { - httpParameters = decodeParameters(nettyRequest.uri()); + httpParameters = decodeParameters(); this.httpParameters = httpParameters; } } @@ -210,7 +225,7 @@ public String getPath() { synchronized (this) { // double check path = this.path; if (path == null) { - path = decodePath(nettyRequest.uri()); + path = decodePath(); this.path = path; } } @@ -228,17 +243,17 @@ public String getPath() { * @param uri The URI * @return The query string decoder */ - protected QueryStringDecoder createDecoder(String uri) { + protected final QueryStringDecoder createDecoder(URI uri) { Charset charset = getCharacterEncoding(); return charset != null ? new QueryStringDecoder(uri, charset) : new QueryStringDecoder(uri); } - private String decodePath(String uri) { + private String decodePath() { QueryStringDecoder queryStringDecoder = createDecoder(uri); return queryStringDecoder.rawPath(); } - private NettyHttpParameters decodeParameters(String uri) { + private NettyHttpParameters decodeParameters() { QueryStringDecoder queryStringDecoder = createDecoder(uri); return new NettyHttpParameters(queryStringDecoder.parameters(), conversionService, null); } diff --git a/http-server-netty/src/main/java/io/micronaut/http/server/netty/NettyHttpRequest.java b/http-server-netty/src/main/java/io/micronaut/http/server/netty/NettyHttpRequest.java index 2713acbcff3..ce683907670 100644 --- a/http-server-netty/src/main/java/io/micronaut/http/server/netty/NettyHttpRequest.java +++ b/http-server-netty/src/main/java/io/micronaut/http/server/netty/NettyHttpRequest.java @@ -665,7 +665,7 @@ public MutableHttpParameters getParameters() { synchronized (this) { // double check httpParameters = this.httpParameters; if (httpParameters == null) { - QueryStringDecoder queryStringDecoder = createDecoder(uri.toString()); + QueryStringDecoder queryStringDecoder = createDecoder(uri); httpParameters = new NettyHttpParameters(queryStringDecoder.parameters(), conversionService, null); this.httpParameters = httpParameters; } diff --git a/http-server-netty/src/test/groovy/io/micronaut/http/server/netty/RequestLineSpec.groovy b/http-server-netty/src/test/groovy/io/micronaut/http/server/netty/RequestLineSpec.groovy new file mode 100644 index 00000000000..c41d3da1ff2 --- /dev/null +++ b/http-server-netty/src/test/groovy/io/micronaut/http/server/netty/RequestLineSpec.groovy @@ -0,0 +1,104 @@ +package io.micronaut.http.server.netty + +import io.micronaut.context.ApplicationContext +import io.micronaut.context.annotation.Requires +import io.micronaut.http.annotation.Controller +import io.micronaut.http.annotation.Get +import io.micronaut.runtime.server.EmbeddedServer +import io.netty.channel.ChannelHandlerContext +import io.netty.channel.ChannelOutboundHandlerAdapter +import io.netty.channel.ChannelPromise +import io.netty.channel.embedded.EmbeddedChannel +import io.netty.handler.codec.http.DefaultHttpRequest +import io.netty.handler.codec.http.FullHttpResponse +import io.netty.handler.codec.http.HttpClientCodec +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.HttpObjectAggregator +import io.netty.handler.codec.http.HttpResponseStatus +import io.netty.handler.codec.http.HttpVersion +import jakarta.inject.Singleton +import spock.lang.Issue +import spock.lang.Specification + +import java.nio.charset.StandardCharsets + +class RequestLineSpec extends Specification { + @Issue('https://github.com/micronaut-projects/micronaut-core/issues/7193') + def 'test different request lines http 1'() { + given: + ApplicationContext ctx = ApplicationContext.run([ + 'spec.name': 'RequestLineSpec', + ]) + def embeddedServer = (NettyHttpServer) ctx.getBean(EmbeddedServer) + + def serverEmbeddedChannel = embeddedServer.buildEmbeddedChannel(false) + + def clientEmbeddedChannel = new EmbeddedChannel() + + serverEmbeddedChannel.pipeline() + .addFirst(new ChannelOutboundHandlerAdapter() { + @Override + void write(ChannelHandlerContext ctx_, Object msg, ChannelPromise promise) throws Exception { + // forward to client + clientEmbeddedChannel.writeOneInbound(msg) + } + + @Override + void flush(ChannelHandlerContext ctx_) throws Exception { + clientEmbeddedChannel.flushInbound() + } + }) + clientEmbeddedChannel.pipeline() + .addLast(new ChannelOutboundHandlerAdapter() { + @Override + void write(ChannelHandlerContext ctx_, Object msg, ChannelPromise promise) throws Exception { + // forward to server + serverEmbeddedChannel.writeOneInbound(msg) + } + + @Override + void flush(ChannelHandlerContext ctx_) throws Exception { + serverEmbeddedChannel.flushInbound() + } + }) + .addLast(new HttpClientCodec()) + .addLast(new HttpObjectAggregator(1024)) + + def request1 = new DefaultHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.GET, uri) + request1.headers().add(HttpHeaderNames.CONNECTION, HttpHeaderValues.CLOSE) + + when: + clientEmbeddedChannel.writeOneOutbound(request1) + clientEmbeddedChannel.flushOutbound() + serverEmbeddedChannel.runPendingTasks() + + then: + FullHttpResponse response = clientEmbeddedChannel.readInbound() + response.status() == HttpResponseStatus.OK + response.content().toString(StandardCharsets.UTF_8) == 'bar' + + cleanup: + response.release() + clientEmbeddedChannel.close() + serverEmbeddedChannel.close() + ctx.close() + + where: + uri << [ + '/foo', // origin-form + 'http://example.com/foo', // absolute-form + ] + } + + @Singleton + @Controller + @Requires(property = 'spec.name', value = 'RequestLineSpec') + public static class SimpleController { + @Get('/foo') + public String foo() { + return "bar" + } + } +} From 73c74a8bafd0946875177e42c85672732274f79e Mon Sep 17 00:00:00 2001 From: yawkat Date: Mon, 11 Apr 2022 11:15:59 +0200 Subject: [PATCH 2/3] work around infinite loop in BufferLeakDetection --- .../netty/fuzzing/BufferLeakDetection.groovy | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/http-server-netty/src/test/groovy/io/micronaut/http/server/netty/fuzzing/BufferLeakDetection.groovy b/http-server-netty/src/test/groovy/io/micronaut/http/server/netty/fuzzing/BufferLeakDetection.groovy index f3bfc65432a..74243117c16 100644 --- a/http-server-netty/src/test/groovy/io/micronaut/http/server/netty/fuzzing/BufferLeakDetection.groovy +++ b/http-server-netty/src/test/groovy/io/micronaut/http/server/netty/fuzzing/BufferLeakDetection.groovy @@ -16,6 +16,7 @@ class BufferLeakDetection extends ResourceLeakDetector { private static final List> allDetectors = new CopyOnWriteArrayList<>() + private static final String BASE_CANARY_STRING = "canary-" + UUID.randomUUID() + "-" private static volatile String canaryString = null private static volatile String currentTest = null @@ -54,13 +55,21 @@ class BufferLeakDetection extends ResourceLeakDetector { // adapted from https://github.com/netty/netty/blob/86603872776e3ff5a60dea3c104358e486eed588/common/src/test/java/io/netty/util/ResourceLeakDetectorTest.java private static synchronized void triggerGc() { + // timeout of last resort for the loop below. use nanoTime because it's monotonic + long startTime = System.nanoTime() + // need to randomize this every time, since ResourceLeakDetector will deduplicate leaks - canaryString = "canary-" + UUID.randomUUID() + canaryString = BASE_CANARY_STRING + UUID.randomUUID() canaryDetected = false leakCanary() do { + if (System.nanoTime() - startTime > 30_000_000_000L) { + LOG.warn("Canary leak detection failed.") + break + } + // Trigger GC. System.gc(); @@ -101,6 +110,10 @@ class BufferLeakDetection extends ResourceLeakDetector { canaryDetected = true return } + if (records.contains(BASE_CANARY_STRING)) { + // probably a canary from another run that ran into a timeout, drop + return + } leakDetected = true super.reportTracedLeak(resourceType, records) From 02281be0a01d14b1fccf194b020be1ec1e56a9af Mon Sep 17 00:00:00 2001 From: yawkat Date: Mon, 11 Apr 2022 12:04:03 +0200 Subject: [PATCH 3/3] add a url without authority to the test --- .../groovy/io/micronaut/http/server/netty/RequestLineSpec.groovy | 1 + 1 file changed, 1 insertion(+) diff --git a/http-server-netty/src/test/groovy/io/micronaut/http/server/netty/RequestLineSpec.groovy b/http-server-netty/src/test/groovy/io/micronaut/http/server/netty/RequestLineSpec.groovy index c41d3da1ff2..78e80d1f5da 100644 --- a/http-server-netty/src/test/groovy/io/micronaut/http/server/netty/RequestLineSpec.groovy +++ b/http-server-netty/src/test/groovy/io/micronaut/http/server/netty/RequestLineSpec.groovy @@ -89,6 +89,7 @@ class RequestLineSpec extends Specification { uri << [ '/foo', // origin-form 'http://example.com/foo', // absolute-form + 'http:///foo', // weird form ] }