From f82947ab8ff608a44a065e3ea475b1206e71ab95 Mon Sep 17 00:00:00 2001 From: yawkat Date: Wed, 12 Jan 2022 11:49:55 +0100 Subject: [PATCH] Do not apply filters when RouteExecutor decides not to handle an error For certain errors (`isIgnorable`), the `RouteExecutor` decides not to write a response for the error, and returns an empty flux. However, without this patch, the `RoutingInBoundHandler` would still apply filters. For example, the micronaut-security filter will check access permissions for the error request, even though those routes would never be executed anyway (`RouteExecutor` decided not to). That filter may then return a response. This patch adds a short-circuit check for these 'closed connection' errors to `RoutingInBoundHandler`, and avoids running downstream processing altogether. Fixes #6723 --- gradle/libs.versions.toml | 2 + http-server-netty/build.gradle | 3 +- .../server/netty/RoutingInBoundHandler.java | 14 +++- .../server/netty/ImmediateCloseSpec.groovy | 77 +++++++++++++++++++ .../micronaut/http/server/RouteExecutor.java | 3 + 5 files changed, 97 insertions(+), 2 deletions(-) create mode 100644 http-server-netty/src/test/groovy/io/micronaut/http/server/netty/ImmediateCloseSpec.groovy diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml index 281e26ee576..17bc171ab0c 100644 --- a/gradle/libs.versions.toml +++ b/gradle/libs.versions.toml @@ -11,6 +11,7 @@ hibernate = "5.5.3.Final" hibernate-validator = "6.1.6.Final" htmlSanityCheck = "1.1.6" htmlunit = "2.47.1" +httpcomponents-client = "4.5.13" jakarta-inject-api = "2.0.1" jakarta-inject-tck = "2.0.1" javax-inject = "1" @@ -524,6 +525,7 @@ testcontainers-spock = { module = "org.testcontainers:spock", version.ref = "man vertx = { module = "io.vertx:vertx-core", version.ref = "vertx" } vertx-webclient = { module = "io.vertx:vertx-web-client", version.ref = "vertx" } +httpcomponents-client = { module = "org.apache.httpcomponents:httpclient", version.ref = "httpcomponents-client" } wiremock = { module = "com.github.tomakehurst:wiremock-jre8", version.ref = "wiremock" } diff --git a/http-server-netty/build.gradle b/http-server-netty/build.gradle index e983352fab2..85df94f5743 100644 --- a/http-server-netty/build.gradle +++ b/http-server-netty/build.gradle @@ -42,9 +42,10 @@ dependencies { } testImplementation libs.managed.jackson.databind - // following 3 dependencies needed for Http2PostTest + // http impls for tests testImplementation libs.vertx testImplementation libs.vertx.webclient + testImplementation libs.httpcomponents.client testImplementation libs.jetty.alpn.openjdk8.client testImplementation libs.managed.groovy.json diff --git a/http-server-netty/src/main/java/io/micronaut/http/server/netty/RoutingInBoundHandler.java b/http-server-netty/src/main/java/io/micronaut/http/server/netty/RoutingInBoundHandler.java index 1673f2fcd47..700ef406237 100644 --- a/http-server-netty/src/main/java/io/micronaut/http/server/netty/RoutingInBoundHandler.java +++ b/http-server-netty/src/main/java/io/micronaut/http/server/netty/RoutingInBoundHandler.java @@ -144,6 +144,9 @@ class RoutingInBoundHandler extends SimpleChannelInboundHandler> { private static final Logger LOG = LoggerFactory.getLogger(RoutingInBoundHandler.class); + /** + * Also present in {@link RouteExecutor} + */ private static final Pattern IGNORABLE_ERROR_MESSAGE = Pattern.compile( "^.*(?:connection.*(?:reset|closed|abort|broken)|broken.*pipe).*$", Pattern.CASE_INSENSITIVE); private static final Argument ARGUMENT_PART_DATA = Argument.of(PartData.class); @@ -245,9 +248,18 @@ public void userEventTriggered(ChannelHandlerContext ctx, Object evt) throws Exc @Override public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) { + // short-circuit ignorable exceptions: This is also handled by RouteExecutor, but handling this early avoids + // running any filters + if (isIgnorable(cause)) { + if (LOG.isDebugEnabled()) { + LOG.debug("Swallowed an IOException caused by client connectivity: " + cause.getMessage(), cause); + } + return; + } + NettyHttpRequest nettyHttpRequest = NettyHttpRequest.remove(ctx); if (nettyHttpRequest == null) { - if (cause instanceof SSLException || cause.getCause() instanceof SSLException || isIgnorable(cause)) { + if (cause instanceof SSLException || cause.getCause() instanceof SSLException) { if (LOG.isDebugEnabled()) { LOG.debug("Micronaut Server Error - No request state present. Cause: " + cause.getMessage(), cause); } diff --git a/http-server-netty/src/test/groovy/io/micronaut/http/server/netty/ImmediateCloseSpec.groovy b/http-server-netty/src/test/groovy/io/micronaut/http/server/netty/ImmediateCloseSpec.groovy new file mode 100644 index 00000000000..19eeb459f7c --- /dev/null +++ b/http-server-netty/src/test/groovy/io/micronaut/http/server/netty/ImmediateCloseSpec.groovy @@ -0,0 +1,77 @@ +package io.micronaut.http.server.netty + +import io.micronaut.context.ApplicationContext +import io.micronaut.context.annotation.Requires +import io.micronaut.core.async.publisher.Publishers +import io.micronaut.http.HttpRequest +import io.micronaut.http.HttpResponse +import io.micronaut.http.MutableHttpResponse +import io.micronaut.http.annotation.Controller +import io.micronaut.http.annotation.Get +import io.micronaut.http.filter.HttpServerFilter +import io.micronaut.http.filter.ServerFilterChain +import io.micronaut.runtime.server.EmbeddedServer +import jakarta.inject.Inject +import jakarta.inject.Singleton +import org.apache.http.client.methods.HttpGet +import org.apache.http.impl.client.CloseableHttpClient +import org.apache.http.impl.client.HttpClients +import org.reactivestreams.Publisher +import spock.lang.Issue +import spock.lang.Specification + +class ImmediateCloseSpec extends Specification { + @Issue('https://github.com/micronaut-projects/micronaut-core/issues/6723') + def 'immediate close of client connection should not lead to log message'() { + given: + def ctx = ApplicationContext.run([ + 'spec.name': 'ImmediateCloseSpec', + //'micronaut.server.port': 8080 + ]) + def embeddedServer = ctx.getBean(EmbeddedServer) + embeddedServer.start() + + when: + HttpGet get = new HttpGet(embeddedServer.URI.toString() + '/empty') + CloseableHttpClient httpClient = HttpClients.createDefault() + def response = httpClient.execute(get) + httpClient.close() + embeddedServer.close() // wait for connection handling to finish + + then: + response.getStatusLine().statusCode == 401 + ctx.getBean(Holder).handleCount == 1 + // can't check for log messages :( + + cleanup: + embeddedServer.close() + httpClient.close() + } + + @Requires(property = 'spec.name', value = 'ImmediateCloseSpec') + @Controller('/empty') + static class EmptyController { + @Get + HttpResponse get() { + return HttpResponse.ok() + } + } + + @Requires(property = 'spec.name', value = 'ImmediateCloseSpec') + @Singleton + static class Holder { + int handleCount = 0 + } + + @Requires(property = 'spec.name', value = 'ImmediateCloseSpec') + @io.micronaut.http.annotation.Filter('/**') + static class Filter implements HttpServerFilter { + @Inject Holder holder + + @Override + Publisher> doFilter(HttpRequest request, ServerFilterChain chain) { + holder.handleCount++ + return Publishers.just(HttpResponse.unauthorized()) + } + } +} diff --git a/http-server/src/main/java/io/micronaut/http/server/RouteExecutor.java b/http-server/src/main/java/io/micronaut/http/server/RouteExecutor.java index b6a5fd1711a..14abceccdfc 100644 --- a/http-server/src/main/java/io/micronaut/http/server/RouteExecutor.java +++ b/http-server/src/main/java/io/micronaut/http/server/RouteExecutor.java @@ -92,6 +92,9 @@ public final class RouteExecutor { private static final Logger LOG = LoggerFactory.getLogger(RouteExecutor.class); + /** + * Also present in netty RoutingInBoundHandler + */ private static final Pattern IGNORABLE_ERROR_MESSAGE = Pattern.compile( "^.*(?:connection.*(?:reset|closed|abort|broken)|broken.*pipe).*$", Pattern.CASE_INSENSITIVE);