From 90269207ed5dfb1dafcb4af7a3a8b2d5a2650ce4 Mon Sep 17 00:00:00 2001 From: yawkat Date: Fri, 6 Sep 2024 17:54:40 +0200 Subject: [PATCH] Don't run error routes when initial processing of malformed request fails When netty reports a decode failure, we run the status routes and error response processor. If *that* fails, we would run onError. onError will run filters which can occlude the error because the filters will not see the full headers (decoding failed after all) and may e.g. fail authentication. This change removes the onError fallback from the decode failure branch. --- .../server/netty/NettyRequestLifecycle.java | 42 +++++++------ .../netty/errors/HeaderTooLongSpec.groovy | 60 +++++++++++++++++++ .../netty/errors/MalformedUriSpec.groovy | 33 ++++++++++ 3 files changed, 117 insertions(+), 18 deletions(-) create mode 100644 http-server-netty/src/test/groovy/io/micronaut/http/server/netty/errors/HeaderTooLongSpec.groovy diff --git a/http-server-netty/src/main/java/io/micronaut/http/server/netty/NettyRequestLifecycle.java b/http-server-netty/src/main/java/io/micronaut/http/server/netty/NettyRequestLifecycle.java index f285a2c22db..9fee366399f 100644 --- a/http-server-netty/src/main/java/io/micronaut/http/server/netty/NettyRequestLifecycle.java +++ b/http-server-netty/src/main/java/io/micronaut/http/server/netty/NettyRequestLifecycle.java @@ -74,31 +74,37 @@ void handleNormal(NettyHttpRequest request) { ExecutionFlow> result; - try { - // handle decoding failure - DecoderResult decoderResult = request.getNativeRequest().decoderResult(); - if (decoderResult.isFailure()) { - Throwable cause = decoderResult.cause(); - HttpStatus status = cause instanceof TooLongFrameException ? HttpStatus.REQUEST_ENTITY_TOO_LARGE : HttpStatus.BAD_REQUEST; + // handle decoding failure + DecoderResult decoderResult = request.getNativeRequest().decoderResult(); + if (decoderResult.isFailure()) { + Throwable cause = decoderResult.cause(); + HttpStatus status = cause instanceof TooLongFrameException ? HttpStatus.REQUEST_ENTITY_TOO_LARGE : HttpStatus.BAD_REQUEST; + try { result = onStatusError( request, HttpResponse.status(status), status.getReason() ); - } else { - result = normalFlow(request); + } catch (Exception e) { + result = ExecutionFlow.error(e); } - ImperativeExecutionFlow> imperativeFlow = result.tryComplete(); - if (imperativeFlow != null) { - Object value = ((ImperativeExecutionFlow) imperativeFlow).getValue(); - // usually this is a MutableHttpResponse, avoid scalability issues here - HttpResponse response = value instanceof NettyMutableHttpResponse mut ? mut : (HttpResponse) value; - rib.writeResponse(outboundAccess, request, response, imperativeFlow.getError()); - } else { - result.onComplete((response, throwable) -> rib.writeResponse(outboundAccess, request, response, throwable)); + } else { + try { + result = normalFlow(request); + } catch (Exception e) { + handleException(request, e); + return; } - } catch (Exception e) { - handleException(request, e); + } + + ImperativeExecutionFlow> imperativeFlow = result.tryComplete(); + if (imperativeFlow != null) { + Object value = ((ImperativeExecutionFlow) imperativeFlow).getValue(); + // usually this is a MutableHttpResponse, avoid scalability issues here + HttpResponse response = value instanceof NettyMutableHttpResponse mut ? mut : (HttpResponse) value; + rib.writeResponse(outboundAccess, request, response, imperativeFlow.getError()); + } else { + result.onComplete((response, throwable) -> rib.writeResponse(outboundAccess, request, response, throwable)); } } diff --git a/http-server-netty/src/test/groovy/io/micronaut/http/server/netty/errors/HeaderTooLongSpec.groovy b/http-server-netty/src/test/groovy/io/micronaut/http/server/netty/errors/HeaderTooLongSpec.groovy new file mode 100644 index 00000000000..748cd43e788 --- /dev/null +++ b/http-server-netty/src/test/groovy/io/micronaut/http/server/netty/errors/HeaderTooLongSpec.groovy @@ -0,0 +1,60 @@ +package io.micronaut.http.server.netty.errors + +import io.micronaut.context.ApplicationContext +import io.micronaut.context.annotation.Requires +import io.micronaut.core.annotation.NonNull +import io.micronaut.http.HttpRequest +import io.micronaut.http.MutableHttpResponse +import io.micronaut.http.annotation.RequestFilter +import io.micronaut.http.annotation.ServerFilter +import io.micronaut.http.server.exceptions.response.ErrorContext +import io.micronaut.http.server.exceptions.response.ErrorResponseProcessor +import io.micronaut.runtime.server.EmbeddedServer +import jakarta.inject.Singleton +import spock.lang.AutoCleanup +import spock.lang.Shared +import spock.lang.Specification + +class HeaderTooLongSpec extends Specification { + + @Shared @AutoCleanup EmbeddedServer embeddedServer = ApplicationContext.run(EmbeddedServer, [ + 'spec.name': 'HeaderTooLongSpec', + 'micronaut.server.netty.log-level': 'info' + ]) + + def 'header too long'() { + given: + def connection = new URL("$embeddedServer.URL/malformed-proxy/xyz").openConnection() + connection.setRequestProperty("foo", "b".repeat(9000)) + + def myFilter = embeddedServer.applicationContext.getBean(MyFilter) + + when: + connection.inputStream + then: + thrown IOException + ((HttpURLConnection) connection).errorStream == null + myFilter.filteredRequest == null + } + + @Singleton + @Requires(property = "spec.name", value = "HeaderTooLongSpec") + static class BrokenProcessor implements ErrorResponseProcessor { + @Override + MutableHttpResponse processResponse(@NonNull ErrorContext errorContext, @NonNull MutableHttpResponse baseResponse) { + throw new Exception("This processor is intentionally broken") + } + } + + @Singleton + @Requires(property = "spec.name", value = "HeaderTooLongSpec") + @ServerFilter("/**") + static class MyFilter { + HttpRequest filteredRequest + + @RequestFilter + void requestFilter(HttpRequest request) { + filteredRequest = request + } + } +} diff --git a/http-server-netty/src/test/groovy/io/micronaut/http/server/netty/errors/MalformedUriSpec.groovy b/http-server-netty/src/test/groovy/io/micronaut/http/server/netty/errors/MalformedUriSpec.groovy index d87d5d1df4e..0fcf909108c 100644 --- a/http-server-netty/src/test/groovy/io/micronaut/http/server/netty/errors/MalformedUriSpec.groovy +++ b/http-server-netty/src/test/groovy/io/micronaut/http/server/netty/errors/MalformedUriSpec.groovy @@ -15,6 +15,7 @@ import io.micronaut.http.annotation.ServerFilter import io.micronaut.http.client.HttpClient import io.micronaut.http.filter.HttpServerFilter import io.micronaut.http.filter.ServerFilterChain +import io.micronaut.http.server.annotation.PreMatching import io.micronaut.runtime.server.EmbeddedServer import jakarta.inject.Singleton import org.reactivestreams.Publisher @@ -64,6 +65,31 @@ class MalformedUriSpec extends Specification { result == 'Exception: Illegal character in path at index 17: /malformed-proxy/[]' } + void "header too long"() { + given: + OncePerFilter filter = embeddedServer.applicationContext.getBean(OncePerFilter) + filter.filterCalled = false + + MalformedUriFilter newFilter = embeddedServer.applicationContext.getBean(MalformedUriFilter) + newFilter.preMatchingCalled = false + + def connection = new URL("$embeddedServer.URL/malformed-proxy/xyz").openConnection() + connection.setRequestProperty("foo", "b".repeat(9000)) + + when: + connection.inputStream + then: + thrown IOException + + when: + def result = ((HttpURLConnection) connection).errorStream.text + + then: + result == '{"message":"Request Entity Too Large","_links":{"self":{"href":"/malformed-proxy/xyz","templated":false}},"_embedded":{"errors":[{"message":"Request Entity Too Large"}]}}' + !filter.filterCalled + !newFilter.preMatchingCalled + } + @Requires(property = "spec.name", value = "MalformedUriSpec") @Controller('/malformed') static class SomeController { @@ -97,10 +123,17 @@ class MalformedUriSpec extends Specification { @Singleton @ServerFilter("/malformed-proxy/**") static class MalformedUriFilter { + boolean preMatchingCalled @RequestFilter HttpResponse filter(HttpRequest request) { return HttpResponse.ok("ok: " + request.path) } + + @RequestFilter + @PreMatching + void preMatching(HttpRequest request) { + preMatchingCalled = false + } } }