From 95752ef1c924e38eab5c370b8cee81898c7e83e3 Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Fri, 29 Jan 2021 22:10:03 +0000 Subject: [PATCH] Improve handling for pre-flight requests 1. Update the HandlerMapping contract to state that CORS checks are expected to be applied before returning a handler. 2. DispatcherHandler checks explicitly for pre-flight requests or CORS failed requests and skips handling for both. Technically no change since AbstractHandlerMapping already returns a NO_OP_HANDLER for those cases. The purpose however is for the DispatcherHandler to also guarantee more explicitly that no such handling can take place for such cases. As one consequence, this makes it possible to invoke the DispatcherHandler from anywhere in the WebFilter chain in order to "handle" a pre-flight request, and then skip the rest of the WebFilter chain. See gh-26257 --- .../web/reactive/DispatcherHandler.java | 11 +++++- .../web/reactive/HandlerMapping.java | 7 +++- .../handler/AbstractHandlerMapping.java | 9 +++-- .../web/reactive/DispatcherHandlerTests.java | 39 ++++++++++++++++--- 4 files changed, 55 insertions(+), 11 deletions(-) diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/DispatcherHandler.java b/spring-webflux/src/main/java/org/springframework/web/reactive/DispatcherHandler.java index 0bce233069..604b9e18a0 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/DispatcherHandler.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/DispatcherHandler.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2019 the original author or authors. + * Copyright 2002-2021 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -29,7 +29,10 @@ import org.springframework.context.ApplicationContext; import org.springframework.context.ApplicationContextAware; import org.springframework.core.annotation.AnnotationAwareOrderComparator; import org.springframework.http.HttpStatus; +import org.springframework.http.server.reactive.ServerHttpRequest; import org.springframework.lang.Nullable; +import org.springframework.util.ObjectUtils; +import org.springframework.web.cors.reactive.CorsUtils; import org.springframework.web.server.ResponseStatusException; import org.springframework.web.server.ServerWebExchange; import org.springframework.web.server.WebHandler; @@ -155,6 +158,12 @@ public class DispatcherHandler implements WebHandler, ApplicationContextAware { } private Mono invokeHandler(ServerWebExchange exchange, Object handler) { + // No handling for CORS rejected requests and pre-flight requests + ServerHttpRequest request = exchange.getRequest(); + HttpStatus status = exchange.getResponse().getStatusCode(); + if (ObjectUtils.nullSafeEquals(status, HttpStatus.FORBIDDEN) || CorsUtils.isPreFlightRequest(request)) { + return Mono.empty(); + } if (this.handlerAdapters != null) { for (HandlerAdapter handlerAdapter : this.handlerAdapters) { if (handlerAdapter.supports(handler)) { diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/HandlerMapping.java b/spring-webflux/src/main/java/org/springframework/web/reactive/HandlerMapping.java index b56f7872f4..ad61544e72 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/HandlerMapping.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/HandlerMapping.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2016 the original author or authors. + * Copyright 2002-2021 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -86,6 +86,11 @@ public interface HandlerMapping { /** * Return a handler for this request. + *

Before returning a handler, an implementing method should check for + * CORS configuration associated with the handler, apply validation checks + * based on it, and update the response accordingly. For pre-flight requests, + * the same should be done based on the handler matching to the expected + * actual request. * @param exchange current server exchange * @return a {@link Mono} that emits one value or none in case the request * cannot be resolved to a handler diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/handler/AbstractHandlerMapping.java b/spring-webflux/src/main/java/org/springframework/web/reactive/handler/AbstractHandlerMapping.java index 0c4464d3ae..4fea378fe9 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/handler/AbstractHandlerMapping.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/handler/AbstractHandlerMapping.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2020 the original author or authors. + * Copyright 2002-2021 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -49,7 +49,7 @@ import org.springframework.web.util.pattern.PathPatternParser; public abstract class AbstractHandlerMapping extends ApplicationObjectSupport implements HandlerMapping, Ordered, BeanNameAware { - private static final WebHandler REQUEST_HANDLED_HANDLER = exchange -> Mono.empty(); + private static final WebHandler NO_OP_HANDLER = exchange -> Mono.empty(); private final PathPatternParser patternParser; @@ -184,14 +184,15 @@ public abstract class AbstractHandlerMapping extends ApplicationObjectSupport } ServerHttpRequest request = exchange.getRequest(); if (hasCorsConfigurationSource(handler) || CorsUtils.isPreFlightRequest(request)) { - CorsConfiguration config = (this.corsConfigurationSource != null ? this.corsConfigurationSource.getCorsConfiguration(exchange) : null); + CorsConfiguration config = (this.corsConfigurationSource != null ? + this.corsConfigurationSource.getCorsConfiguration(exchange) : null); CorsConfiguration handlerConfig = getCorsConfiguration(handler, exchange); config = (config != null ? config.combine(handlerConfig) : handlerConfig); if (config != null) { config.validateAllowCredentials(); } if (!this.corsProcessor.process(config, exchange) || CorsUtils.isPreFlightRequest(request)) { - return REQUEST_HANDLED_HANDLER; + return NO_OP_HANDLER; } } return handler; diff --git a/spring-webflux/src/test/java/org/springframework/web/reactive/DispatcherHandlerTests.java b/spring-webflux/src/test/java/org/springframework/web/reactive/DispatcherHandlerTests.java index 637f102be3..c023f672a5 100644 --- a/spring-webflux/src/test/java/org/springframework/web/reactive/DispatcherHandlerTests.java +++ b/spring-webflux/src/test/java/org/springframework/web/reactive/DispatcherHandlerTests.java @@ -28,7 +28,10 @@ import org.springframework.core.MethodParameter; import org.springframework.core.Ordered; import org.springframework.core.io.buffer.DataBuffer; import org.springframework.core.io.buffer.DefaultDataBufferFactory; +import org.springframework.http.HttpHeaders; +import org.springframework.web.reactive.result.SimpleHandlerAdapter; import org.springframework.web.server.ServerWebExchange; +import org.springframework.web.server.WebHandler; import org.springframework.web.testfixture.http.server.reactive.MockServerHttpRequest; import org.springframework.web.testfixture.method.ResolvableMethod; import org.springframework.web.testfixture.server.MockServerWebExchange; @@ -37,6 +40,7 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.ArgumentMatchers.any; import static org.mockito.BDDMockito.given; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verifyNoInteractions; import static org.mockito.Mockito.withSettings; /** @@ -50,7 +54,7 @@ public class DispatcherHandlerTests { @Test - public void handlerMappingOrder() { + void handlerMappingOrder() { HandlerMapping hm1 = mock(HandlerMapping.class, withSettings().extraInterfaces(Ordered.class)); HandlerMapping hm2 = mock(HandlerMapping.class, withSettings().extraInterfaces(Ordered.class)); given(((Ordered) hm1).getOrder()).willReturn(1); @@ -65,13 +69,34 @@ public class DispatcherHandlerTests { context.registerBean(HandlerResultHandler.class, StringHandlerResultHandler::new); context.refresh(); - DispatcherHandler dispatcherHandler = new DispatcherHandler(context); - MockServerWebExchange exchange = MockServerWebExchange.from(MockServerHttpRequest.get("/")); - dispatcherHandler.handle(exchange).block(Duration.ofSeconds(0)); + new DispatcherHandler(context).handle(exchange).block(Duration.ofSeconds(0)); + assertThat(exchange.getResponse().getBodyAsString().block(Duration.ofSeconds(5))).isEqualTo("1"); } + @Test + void preFlightRequest() { + WebHandler webHandler = mock(WebHandler.class); + HandlerMapping handlerMapping = mock(HandlerMapping.class); + given((handlerMapping).getHandler(any())).willReturn(Mono.just(webHandler)); + + StaticApplicationContext context = new StaticApplicationContext(); + context.registerBean("handlerMapping", HandlerMapping.class, () -> handlerMapping); + context.registerBean(HandlerAdapter.class, SimpleHandlerAdapter::new); + context.registerBean(HandlerResultHandler.class, StringHandlerResultHandler::new); + context.refresh(); + + MockServerHttpRequest request = MockServerHttpRequest.options("/") + .header(HttpHeaders.ORIGIN, "https://domain.com") + .header(HttpHeaders.ACCESS_CONTROL_REQUEST_METHOD, "GET") + .build(); + + MockServerWebExchange exchange = MockServerWebExchange.from(request); + new DispatcherHandler(context).handle(exchange).block(Duration.ofSeconds(0)); + + verifyNoInteractions(webHandler); + } @SuppressWarnings("unused") private void handle() {} @@ -101,7 +126,11 @@ public class DispatcherHandlerTests { @Override public Mono handleResult(ServerWebExchange exchange, HandlerResult result) { - byte[] bytes = ((String) result.getReturnValue()).getBytes(StandardCharsets.UTF_8); + Object returnValue = result.getReturnValue(); + if (returnValue == null) { + return Mono.empty(); + } + byte[] bytes = ((String) returnValue).getBytes(StandardCharsets.UTF_8); DataBuffer dataBuffer = DefaultDataBufferFactory.sharedInstance.wrap(bytes); return exchange.getResponse().writeWith(Mono.just(dataBuffer)); }