Move URI validation step back to RoutingInBoundHandler (#10232)

Still configurable though.

The reason for this is that with the previous logic, filters could still see an invalid `request.getPath()`. This is a potential security issue because proxy filters that are built as described [in the guide](https://docs.micronaut.io/latest/guide/index.html#proxyClient) may forward the invalid user-controlled path without checking.

This patch moves the uri check back to its original place. This ensures it's checked before filters. It also replaces the request with a fixed path ("/") so that the filters cannot see a malicious user-controlled path.

http-server-netty/src/main/java/io/micronaut/http/server/netty/NettyRequestLifecycle.java

@@ -48,6 +48,7 @@ final class NettyRequestLifecycle extends RequestLifecycle {
 
     private final RoutingInBoundHandler rib;
     private final PipeliningServerHandler.OutboundAccess outboundAccess;
+    private final boolean validateUrl;
 
     /**
      * Should only be used where netty-specific stuff is needed, such as reading the body or
@@ -58,6 +59,7 @@ final class NettyRequestLifecycle extends RequestLifecycle {
     NettyRequestLifecycle(RoutingInBoundHandler rib, PipeliningServerHandler.OutboundAccess outboundAccess) {
         super(rib.routeExecutor);
         this.rib = rib;
+        this.validateUrl = rib.serverConfiguration.isValidateUrl();
         this.outboundAccess = outboundAccess;
     }
 

http-server-netty/src/main/java/io/micronaut/http/server/netty/RoutingInBoundHandler.java

@@ -195,27 +195,29 @@ public void handleUnboundError(Throwable cause) {
 
     @Override
     public void accept(ChannelHandlerContext ctx, io.netty.handler.codec.http.HttpRequest request, PipeliningServerHandler.OutboundAccess outboundAccess) {
-        NettyHttpRequest<Object> mnRequest;
-        try {
-            mnRequest = new NettyHttpRequest<>(request, ctx, conversionService, serverConfiguration);
-        } catch (IllegalArgumentException e) {
-            // invalid URI
-            NettyHttpRequest<Object> errorRequest = new NettyHttpRequest<>(
-                new DefaultFullHttpRequest(request.protocolVersion(), request.method(), "/", Unpooled.EMPTY_BUFFER),
-                ctx,
-                conversionService,
-                serverConfiguration
-            );
-            outboundAccess.attachment(errorRequest);
-            try (PropagatedContext.Scope ignore = PropagatedContext.getOrEmpty().plus(new ServerHttpRequestContext(errorRequest)).propagate()) {
-                new NettyRequestLifecycle(this, outboundAccess).handleException(errorRequest, e.getCause() == null ? e : e.getCause());
-            }
-            if (request instanceof StreamedHttpRequest streamed) {
-                streamed.closeIfNoSubscriber();
-            } else {
-                ((FullHttpRequest) request).release();
+        NettyHttpRequest<Object> mnRequest = new NettyHttpRequest<>(request, ctx, conversionService, serverConfiguration);
+        if (serverConfiguration.isValidateUrl()) {
+            try {
+                mnRequest.getUri();
+            } catch (IllegalArgumentException e) {
+                // invalid URI
+                NettyHttpRequest<Object> errorRequest = new NettyHttpRequest<>(
+                    new DefaultFullHttpRequest(request.protocolVersion(), request.method(), "/", Unpooled.EMPTY_BUFFER),
+                    ctx,
+                    conversionService,
+                    serverConfiguration
+                );
+                outboundAccess.attachment(errorRequest);
+                try (PropagatedContext.Scope ignore = PropagatedContext.getOrEmpty().plus(new ServerHttpRequestContext(errorRequest)).propagate()) {
+                    new NettyRequestLifecycle(this, outboundAccess).handleException(errorRequest, e.getCause() == null ? e : e.getCause());
+                }
+                if (request instanceof StreamedHttpRequest streamed) {
+                    streamed.closeIfNoSubscriber();
+                } else {
+                    ((FullHttpRequest) request).release();
+                }
+                return;
             }
-            return;
         }
         if (ctx.pipeline().get(ChannelPipelineCustomizer.HANDLER_ACCESS_LOGGER) != null) {
             // Micronaut Session needs this to extract values from the Micronaut Http Request for logging

http-server-netty/src/test/groovy/io/micronaut/http/server/netty/errors/MalformedUriSpec.groovy

@@ -10,6 +10,8 @@ import io.micronaut.http.annotation.Controller
 import io.micronaut.http.annotation.Error
 import io.micronaut.http.annotation.Filter
 import io.micronaut.http.annotation.Get
+import io.micronaut.http.annotation.RequestFilter
+import io.micronaut.http.annotation.ServerFilter
 import io.micronaut.http.client.HttpClient
 import io.micronaut.http.filter.HttpServerFilter
 import io.micronaut.http.filter.ServerFilterChain
@@ -54,6 +56,14 @@ class MalformedUriSpec extends Specification {
         result == 'Exception: Illegal character in path at index 11: /malformed/[]'
     }
 
+    void "test normal filter is not called for invalid uri"() {
+        when:
+        def result = new URL("$embeddedServer.URL/malformed-proxy/[]").text
+
+        then:
+        result == 'Exception: Illegal character in path at index 17: /malformed-proxy/[]'
+    }
+
     @Requires(property = "spec.name", value = "MalformedUriSpec")
     @Controller('/malformed')
     static class SomeController {
@@ -82,4 +92,15 @@ class MalformedUriSpec extends Specification {
             return chain.proceed(request)
         }
     }
+
+    @Requires(property = "spec.name", value = "MalformedUriSpec")
+    @Singleton
+    @ServerFilter("/malformed-proxy/**")
+    static class MalformedUriFilter {
+
+        @RequestFilter
+        HttpResponse<?> filter(HttpRequest<?> request) {
+            return HttpResponse.ok("ok: " + request.path)
+        }
+    }
 }

http-server/src/main/java/io/micronaut/http/server/RequestLifecycle.java

@@ -72,15 +72,13 @@ public class RequestLifecycle {
 
     private final RouteExecutor routeExecutor;
     private final boolean multipartEnabled;
-    private final boolean validateUrl;
     private HttpRequest<?> request;
 
     /**
      * @param routeExecutor The route executor to use for route resolution
      */
     protected RequestLifecycle(RouteExecutor routeExecutor) {
         this.routeExecutor = Objects.requireNonNull(routeExecutor, "routeExecutor");
-        this.validateUrl = routeExecutor.serverConfiguration.isValidateUrl();
         Optional<Boolean> isMultiPartEnabled = routeExecutor.serverConfiguration.getMultipart().getEnabled();
         this.multipartEnabled = isMultiPartEnabled.isEmpty() || isMultiPartEnabled.get();
     }
@@ -158,13 +156,6 @@ protected final ExecutionFlow<HttpResponse<?>> normalFlow(HttpRequest<?> request
 
             UriRouteMatch<Object, Object> routeMatch = routeExecutor.findRouteMatch(request);
             if (routeMatch == null) {
-                if (validateUrl) {
-                    try {
-                        request.getUri(); // Invalid url will throw an exception
-                    } catch (Throwable t) {
-                        return onError(request, t.getCause());
-                    }
-                }
                 //Check if there is a file for the route before returning route not found
                 FileCustomizableResponseType fileCustomizableResponseType = findFile(request);
                 if (fileCustomizableResponseType != null) {
@@ -187,16 +178,7 @@ protected final ExecutionFlow<HttpResponse<?>> normalFlow(HttpRequest<?> request
                     "Not a WebSocket request");
             }
 
-            return runWithFilters(request, (filteredRequest, propagatedContext) -> {
-                if (validateUrl) {
-                    try {
-                        request.getUri(); // Invalid url will throw an exception
-                    } catch (Throwable t) {
-                        return onError(filteredRequest, t.getCause());
-                    }
-                }
-                return executeRoute(filteredRequest, propagatedContext, routeMatch);
-            });
+            return runWithFilters(request, (filteredRequest, propagatedContext) -> executeRoute(filteredRequest, propagatedContext, routeMatch));
         } catch (Throwable t) {
             return onError(request, t);
         }