Spring/spring-framework
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Suppress PREFLIGHT_AMBIGUOUS_MATCH if matches have no CORS config

Browse Source 
Closes gh-26490
This commit is contained in:
Rossen Stoyanchev
2021-02-03 10:43:08 +00:00
parent 0575e6637c
commit 996c86f448
5 changed files with 215 additions and 79 deletions
Download Patch File Download Diff File Expand all files Collapse all files

69
spring-webflux/src/main/java/org/springframework/web/reactive/result/method/AbstractHandlerMethodMapping.java
View File

@@ -330,19 +330,25 @@ public abstract class AbstractHandlerMethodMapping<T> extends AbstractHandlerMap
logger.trace(exchange.getLogPrefix() + matches.size() + " matching mappings: " + matches);
}
if (CorsUtils.isPreFlightRequest(exchange.getRequest())) {
return PREFLIGHT_AMBIGUOUS_MATCH;
for (Match match : matches) {
if (match.hasCorsConfig()) {
return PREFLIGHT_AMBIGUOUS_MATCH;
}
}
}
Match secondBestMatch = matches.get(1);
if (comparator.compare(bestMatch, secondBestMatch) == 0) {
Method m1 = bestMatch.handlerMethod.getMethod();
Method m2 = secondBestMatch.handlerMethod.getMethod();
RequestPath path = exchange.getRequest().getPath();
throw new IllegalStateException(
"Ambiguous handler methods mapped for '" + path + "': {" + m1 + ", " + m2 + "}");
else {
Match secondBestMatch = matches.get(1);
if (comparator.compare(bestMatch, secondBestMatch) == 0) {
Method m1 = bestMatch.getHandlerMethod().getMethod();
Method m2 = secondBestMatch.getHandlerMethod().getMethod();
RequestPath path = exchange.getRequest().getPath();
throw new IllegalStateException(
"Ambiguous handler methods mapped for '" + path + "': {" + m1 + ", " + m2 + "}");
}
}
}
handleMatch(bestMatch.mapping, bestMatch.handlerMethod, exchange);
return bestMatch.handlerMethod;
handleMatch(bestMatch.mapping, bestMatch.getHandlerMethod(), exchange);
return bestMatch.getHandlerMethod();
}
else {
return handleNoMatch(this.mappingRegistry.getRegistrations().keySet(), exchange);
@@ -353,8 +359,7 @@ public abstract class AbstractHandlerMethodMapping<T> extends AbstractHandlerMap
for (T mapping : mappings) {
T match = getMatchingMapping(mapping, exchange);
if (match != null) {
matches.add(new Match(match,
this.mappingRegistry.getRegistrations().get(mapping).getHandlerMethod()));
matches.add(new Match(match, this.mappingRegistry.getRegistrations().get(mapping)));
}
}
}
@@ -519,13 +524,14 @@ public abstract class AbstractHandlerMethodMapping<T> extends AbstractHandlerMap
this.pathLookup.add(path, mapping);
}
CorsConfiguration config = initCorsConfiguration(handler, method, mapping);
if (config != null) {
config.validateAllowCredentials();
this.corsLookup.put(handlerMethod, config);
CorsConfiguration corsConfig = initCorsConfiguration(handler, method, mapping);
if (corsConfig != null) {
corsConfig.validateAllowCredentials();
this.corsLookup.put(handlerMethod, corsConfig);
}
this.registry.put(mapping, new MappingRegistration<>(mapping, handlerMethod, directPaths));
this.registry.put(mapping,
new MappingRegistration<>(mapping, handlerMethod, directPaths, corsConfig != null));
}
finally {
this.readWriteLock.writeLock().unlock();
@@ -578,12 +584,17 @@ public abstract class AbstractHandlerMethodMapping<T> extends AbstractHandlerMap
private final Set<String> directPaths;
public MappingRegistration(T mapping, HandlerMethod handlerMethod, @Nullable Set<String> directPaths) {
private final boolean corsConfig;
public MappingRegistration(
T mapping, HandlerMethod handlerMethod, @Nullable Set<String> directPaths, boolean corsConfig) {
Assert.notNull(mapping, "Mapping must not be null");
Assert.notNull(handlerMethod, "HandlerMethod must not be null");
this.mapping = mapping;
this.handlerMethod = handlerMethod;
this.directPaths = (directPaths != null ? directPaths : Collections.emptySet());
this.corsConfig = corsConfig;
}
public T getMapping() {
@@ -597,6 +608,10 @@ public abstract class AbstractHandlerMethodMapping<T> extends AbstractHandlerMap
public Set<String> getDirectPaths() {
return this.directPaths;
}
public boolean hasCorsConfig() {
return this.corsConfig;
}
}
@@ -608,11 +623,23 @@ public abstract class AbstractHandlerMethodMapping<T> extends AbstractHandlerMap
private final T mapping;
private final HandlerMethod handlerMethod;
private final MappingRegistration<T> registration;
public Match(T mapping, HandlerMethod handlerMethod) {
public Match(T mapping, MappingRegistration<T> registration) {
this.mapping = mapping;
this.handlerMethod = handlerMethod;
this.registration = registration;
}
public T getMapping() {
return this.mapping;
}
public HandlerMethod getHandlerMethod() {
return this.registration.getHandlerMethod();
}
public boolean hasCorsConfig() {
return this.registration.hasCorsConfig();
}
@Override

58
spring-webflux/src/test/java/org/springframework/web/reactive/result/method/HandlerMethodMappingTests.java
View File

@@ -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.
@@ -28,12 +28,19 @@ import org.junit.jupiter.api.Test;
import reactor.core.publisher.Mono;
import reactor.test.StepVerifier;
import org.springframework.core.annotation.AnnotatedElementUtils;
import org.springframework.http.HttpHeaders;
import org.springframework.http.HttpMethod;
import org.springframework.http.HttpStatus;
import org.springframework.http.server.PathContainer;
import org.springframework.stereotype.Controller;
import org.springframework.web.bind.annotation.CrossOrigin;
import org.springframework.web.bind.annotation.RequestMapping;
import org.springframework.web.cors.CorsConfiguration;
import org.springframework.web.method.HandlerMethod;
import org.springframework.web.server.ServerWebExchange;
import org.springframework.web.testfixture.http.server.reactive.MockServerHttpRequest;
import org.springframework.web.testfixture.http.server.reactive.MockServerHttpResponse;
import org.springframework.web.testfixture.server.MockServerWebExchange;
import org.springframework.web.util.pattern.PathPattern;
import org.springframework.web.util.pattern.PathPatternParser;
@@ -105,6 +112,39 @@ public class HandlerMethodMappingTests {
StepVerifier.create(result).expectError(IllegalStateException.class).verify();
}
@Test // gh-26490
public void ambiguousMatchOnPreFlightRequestWithoutCorsConfig() throws Exception {
this.mapping.registerMapping("/f?o", this.handler, this.method1);
this.mapping.registerMapping("/fo?", this.handler, this.method2);
MockServerWebExchange exchange = MockServerWebExchange.from(
MockServerHttpRequest.options("http://example.org/foo")
.header(HttpHeaders.ORIGIN, "https://domain.com")
.header(HttpHeaders.ACCESS_CONTROL_REQUEST_METHOD, "GET"));
this.mapping.getHandler(exchange).block();
assertThat(exchange.getResponse().getStatusCode()).isEqualTo(HttpStatus.FORBIDDEN);
}
@Test // gh-26490
public void ambiguousMatchOnPreFlightRequestWithCorsConfig() throws Exception {
this.mapping.registerMapping("/f?o", this.handler, this.method1);
this.mapping.registerMapping("/fo?", this.handler, this.handler.getClass().getMethod("corsHandlerMethod"));
MockServerWebExchange exchange = MockServerWebExchange.from(
MockServerHttpRequest.options("http://example.org/foo")
.header(HttpHeaders.ORIGIN, "https://domain.com")
.header(HttpHeaders.ACCESS_CONTROL_REQUEST_METHOD, "GET"));
this.mapping.getHandler(exchange).block();
MockServerHttpResponse response = exchange.getResponse();
assertThat(response.getStatusCode()).isNull();
assertThat(response.getHeaders().getAccessControlAllowOrigin()).isEqualTo("https://domain.com");
assertThat(response.getHeaders().getAccessControlAllowMethods()).containsExactly(HttpMethod.GET);
}
@Test
public void registerMapping() {
String key1 = "/foo";
@@ -171,6 +211,17 @@ public class HandlerMethodMappingTests {
Collections.emptySet() : Collections.singleton(mapping));
}
@Override
protected CorsConfiguration initCorsConfiguration(Object handler, Method method, String mapping) {
CrossOrigin crossOrigin = AnnotatedElementUtils.findMergedAnnotation(method, CrossOrigin.class);
if (crossOrigin != null) {
CorsConfiguration corsConfig = new CorsConfiguration();
corsConfig.setAllowedOrigins(Collections.singletonList("http://domain.com"));
return corsConfig;
}
return null;
}
@Override
protected String getMatchingMapping(String pattern, ServerWebExchange exchange) {
PathContainer lookupPath = exchange.getRequest().getPath().pathWithinApplication();
@@ -200,6 +251,11 @@ public class HandlerMethodMappingTests {
@SuppressWarnings("unused")
public void handlerMethod2() {
}
@RequestMapping
@CrossOrigin(originPatterns = "*")
public void corsHandlerMethod() {
}
}
}

8
spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/GlobalCorsConfigIntegrationTests.java
View File

@@ -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.
@@ -160,9 +160,9 @@ class GlobalCorsConfigIntegrationTests extends AbstractRequestMappingIntegration
this.headers.add(HttpHeaders.ACCESS_CONTROL_REQUEST_METHOD, "GET");
ResponseEntity<String> entity = performOptions("/ambiguous", this.headers, String.class);
assertThat(entity.getStatusCode()).isEqualTo(HttpStatus.OK);
assertThat(entity.getHeaders().getAccessControlAllowOrigin()).isEqualTo("http://localhost:9000");
assertThat(entity.getHeaders().getAccessControlAllowMethods()).containsExactly(HttpMethod.GET);
assertThat(entity.getHeaders().getAccessControlAllowCredentials()).isEqualTo(true);
assertThat(entity.getHeaders().getAccessControlAllowOrigin()).isEqualTo("*");
assertThat(entity.getHeaders().getAccessControlAllowMethods()).containsExactly(HttpMethod.GET, HttpMethod.POST);
assertThat(entity.getHeaders().getAccessControlAllowCredentials()).isEqualTo(false);
assertThat(entity.getHeaders().get(HttpHeaders.VARY))
.containsExactly(HttpHeaders.ORIGIN, HttpHeaders.ACCESS_CONTROL_REQUEST_METHOD,
HttpHeaders.ACCESS_CONTROL_REQUEST_HEADERS);

67
spring-webmvc/src/main/java/org/springframework/web/servlet/handler/AbstractHandlerMethodMapping.java
View File

@@ -407,20 +407,26 @@ public abstract class AbstractHandlerMethodMapping<T> extends AbstractHandlerMap
logger.trace(matches.size() + " matching mappings: " + matches);
}
if (CorsUtils.isPreFlightRequest(request)) {
return PREFLIGHT_AMBIGUOUS_MATCH;
for (Match match : matches) {
if (match.hasCorsConfig()) {
return PREFLIGHT_AMBIGUOUS_MATCH;
}
}
}
Match secondBestMatch = matches.get(1);
if (comparator.compare(bestMatch, secondBestMatch) == 0) {
Method m1 = bestMatch.handlerMethod.getMethod();
Method m2 = secondBestMatch.handlerMethod.getMethod();
String uri = request.getRequestURI();
throw new IllegalStateException(
"Ambiguous handler methods mapped for '" + uri + "': {" + m1 + ", " + m2 + "}");
else {
Match secondBestMatch = matches.get(1);
if (comparator.compare(bestMatch, secondBestMatch) == 0) {
Method m1 = bestMatch.getHandlerMethod().getMethod();
Method m2 = secondBestMatch.getHandlerMethod().getMethod();
String uri = request.getRequestURI();
throw new IllegalStateException(
"Ambiguous handler methods mapped for '" + uri + "': {" + m1 + ", " + m2 + "}");
}
}
}
request.setAttribute(BEST_MATCHING_HANDLER_ATTRIBUTE, bestMatch.handlerMethod);
request.setAttribute(BEST_MATCHING_HANDLER_ATTRIBUTE, bestMatch.getHandlerMethod());
handleMatch(bestMatch.mapping, lookupPath, request);
return bestMatch.handlerMethod;
return bestMatch.getHandlerMethod();
}
else {
return handleNoMatch(this.mappingRegistry.getRegistrations().keySet(), lookupPath, request);
@@ -431,8 +437,7 @@ public abstract class AbstractHandlerMethodMapping<T> extends AbstractHandlerMap
for (T mapping : mappings) {
T match = getMatchingMapping(mapping, request);
if (match != null) {
matches.add(new Match(match,
this.mappingRegistry.getRegistrations().get(mapping).getHandlerMethod()));
matches.add(new Match(match, this.mappingRegistry.getRegistrations().get(mapping)));
}
}
}
@@ -630,13 +635,14 @@ public abstract class AbstractHandlerMethodMapping<T> extends AbstractHandlerMap
addMappingName(name, handlerMethod);
}
CorsConfiguration config = initCorsConfiguration(handler, method, mapping);
if (config != null) {
config.validateAllowCredentials();
this.corsLookup.put(handlerMethod, config);
CorsConfiguration corsConfig = initCorsConfiguration(handler, method, mapping);
if (corsConfig != null) {
corsConfig.validateAllowCredentials();
this.corsLookup.put(handlerMethod, corsConfig);
}
this.registry.put(mapping, new MappingRegistration<>(mapping, handlerMethod, directPaths, name));
this.registry.put(mapping,
new MappingRegistration<>(mapping, handlerMethod, directPaths, name, corsConfig != null));
}
finally {
this.readWriteLock.writeLock().unlock();
@@ -735,8 +741,10 @@ public abstract class AbstractHandlerMethodMapping<T> extends AbstractHandlerMap
@Nullable
private final String mappingName;
private final boolean corsConfig;
public MappingRegistration(T mapping, HandlerMethod handlerMethod,
@Nullable Set<String> directPaths, @Nullable String mappingName) {
@Nullable Set<String> directPaths, @Nullable String mappingName, boolean corsConfig) {
Assert.notNull(mapping, "Mapping must not be null");
Assert.notNull(handlerMethod, "HandlerMethod must not be null");
@@ -744,6 +752,7 @@ public abstract class AbstractHandlerMethodMapping<T> extends AbstractHandlerMap
this.handlerMethod = handlerMethod;
this.directPaths = (directPaths != null ? directPaths : Collections.emptySet());
this.mappingName = mappingName;
this.corsConfig = corsConfig;
}
public T getMapping() {
@@ -762,6 +771,10 @@ public abstract class AbstractHandlerMethodMapping<T> extends AbstractHandlerMap
public String getMappingName() {
return this.mappingName;
}
public boolean hasCorsConfig() {
return this.corsConfig;
}
}
@@ -773,11 +786,23 @@ public abstract class AbstractHandlerMethodMapping<T> extends AbstractHandlerMap
private final T mapping;
private final HandlerMethod handlerMethod;
private final MappingRegistration<T> registration;
public Match(T mapping, HandlerMethod handlerMethod) {
public Match(T mapping, MappingRegistration<T> registration) {
this.mapping = mapping;
this.handlerMethod = handlerMethod;
this.registration = registration;
}
public T getMapping() {
return this.mapping;
}
public HandlerMethod getHandlerMethod() {
return this.registration.getHandlerMethod();
}
public boolean hasCorsConfig() {
return this.registration.hasCorsConfig();
}
@Override

92
spring-webmvc/src/test/java/org/springframework/web/servlet/handler/HandlerMethodMappingTests.java
View File

@@ -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.
@@ -29,15 +29,22 @@ import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.springframework.context.support.StaticApplicationContext;
import org.springframework.core.annotation.AnnotatedElementUtils;
import org.springframework.http.HttpHeaders;
import org.springframework.stereotype.Controller;
import org.springframework.util.AntPathMatcher;
import org.springframework.util.PathMatcher;
import org.springframework.web.HttpRequestHandler;
import org.springframework.web.bind.annotation.CrossOrigin;
import org.springframework.web.bind.annotation.RequestMapping;
import org.springframework.web.context.support.StaticWebApplicationContext;
import org.springframework.web.cors.CorsConfiguration;
import org.springframework.web.method.HandlerMethod;
import org.springframework.web.servlet.HandlerExecutionChain;
import org.springframework.web.servlet.HandlerMapping;
import org.springframework.web.servlet.mvc.HttpRequestHandlerAdapter;
import org.springframework.web.testfixture.servlet.MockHttpServletRequest;
import org.springframework.web.testfixture.servlet.MockHttpServletResponse;
import org.springframework.web.util.UrlPathHelper;
import static org.assertj.core.api.Assertions.assertThat;
@@ -110,6 +117,46 @@ public class HandlerMethodMappingTests {
this.mapping.getHandlerInternal(new MockHttpServletRequest("GET", "/foo")));
}
@Test // gh-26490
public void ambiguousMatchOnPreFlightRequestWithoutCorsConfig() throws Exception {
this.mapping.registerMapping("/foo", this.handler, this.method1);
this.mapping.registerMapping("/f??", this.handler, this.method2);
MockHttpServletRequest request = new MockHttpServletRequest("OPTIONS", "/foo");
request.addHeader(HttpHeaders.ORIGIN, "https://domain.com");
request.addHeader(HttpHeaders.ACCESS_CONTROL_REQUEST_METHOD, "GET");
MockHttpServletResponse response = new MockHttpServletResponse();
HandlerExecutionChain chain = this.mapping.getHandler(request);
assertThat(chain).isNotNull();
assertThat(chain.getHandler()).isInstanceOf(HttpRequestHandler.class);
new HttpRequestHandlerAdapter().handle(request, response, chain.getHandler());
assertThat(response.getStatus()).isEqualTo(403);
}
@Test // gh-26490
public void ambiguousMatchOnPreFlightRequestWithCorsConfig() throws Exception {
this.mapping.registerMapping("/f?o", this.handler, this.method1);
this.mapping.registerMapping("/fo?", this.handler, this.handler.getClass().getMethod("corsHandlerMethod"));
MockHttpServletRequest request = new MockHttpServletRequest("OPTIONS", "/foo");
request.addHeader(HttpHeaders.ORIGIN, "https://domain.com");
request.addHeader(HttpHeaders.ACCESS_CONTROL_REQUEST_METHOD, "GET");
MockHttpServletResponse response = new MockHttpServletResponse();
HandlerExecutionChain chain = this.mapping.getHandler(request);
assertThat(chain).isNotNull();
assertThat(chain.getHandler()).isInstanceOf(HttpRequestHandler.class);
new HttpRequestHandlerAdapter().handle(request, response, chain.getHandler());
assertThat(response.getStatus()).isEqualTo(200);
assertThat(response.getHeader(HttpHeaders.ACCESS_CONTROL_ALLOW_ORIGIN)).isEqualTo("https://domain.com");
assertThat(response.getHeader(HttpHeaders.ACCESS_CONTROL_ALLOW_METHODS)).isEqualTo("GET");
}
@Test
public void detectHandlerMethodsInAncestorContexts() {
StaticApplicationContext cxt = new StaticApplicationContext();
@@ -131,7 +178,6 @@ public class HandlerMethodMappingTests {
@Test
public void registerMapping() {
String key1 = "/foo";
String key2 = "/foo*";
this.mapping.registerMapping(key1, this.handler, this.method1);
@@ -160,21 +206,10 @@ public class HandlerMethodMappingTests {
assertThat(handlerMethods).isNotNull();
assertThat(handlerMethods.size()).isEqualTo(1);
assertThat(handlerMethods.get(0)).isEqualTo(handlerMethod2);
// CORS lookup
CorsConfiguration config = this.mapping.getMappingRegistry().getCorsConfiguration(handlerMethod1);
assertThat(config).isNotNull();
assertThat(config.getAllowedOrigins().get(0)).isEqualTo(("http://" + handler.hashCode() + name1));
config = this.mapping.getMappingRegistry().getCorsConfiguration(handlerMethod2);
assertThat(config).isNotNull();
assertThat(config.getAllowedOrigins().get(0)).isEqualTo(("http://" + handler.hashCode() + name2));
}
@Test
public void registerMappingWithSameMethodAndTwoHandlerInstances() {
String key1 = "foo";
String key2 = "bar";
@@ -202,21 +237,10 @@ public class HandlerMethodMappingTests {
assertThat(handlerMethods.size()).isEqualTo(2);
assertThat(handlerMethods.get(0)).isEqualTo(handlerMethod1);
assertThat(handlerMethods.get(1)).isEqualTo(handlerMethod2);
// CORS lookup
CorsConfiguration config = this.mapping.getMappingRegistry().getCorsConfiguration(handlerMethod1);
assertThat(config).isNotNull();
assertThat(config.getAllowedOrigins().get(0)).isEqualTo(("http://" + handler1.hashCode() + name));
config = this.mapping.getMappingRegistry().getCorsConfiguration(handlerMethod2);
assertThat(config).isNotNull();
assertThat(config.getAllowedOrigins().get(0)).isEqualTo(("http://" + handler2.hashCode() + name));
}
@Test
public void unregisterMapping() throws Exception {
String key = "foo";
HandlerMethod handlerMethod = new HandlerMethod(this.handler, this.method1);
@@ -232,7 +256,6 @@ public class HandlerMethodMappingTests {
@Test
public void getCorsConfigWithBeanNameHandler() throws Exception {
String key = "foo";
String beanName = "handler1";
@@ -242,10 +265,6 @@ public class HandlerMethodMappingTests {
this.mapping.setApplicationContext(context);
this.mapping.registerMapping(key, beanName, this.method1);
HandlerMethod handlerMethod = this.mapping.getHandlerInternal(new MockHttpServletRequest("GET", key));
CorsConfiguration config = this.mapping.getMappingRegistry().getCorsConfiguration(handlerMethod);
assertThat(config).isNotNull();
assertThat(config.getAllowedOrigins().get(0)).isEqualTo(("http://" + beanName.hashCode() + this.method1.getName()));
}
@@ -284,9 +303,13 @@ public class HandlerMethodMappingTests {
@Override
protected CorsConfiguration initCorsConfiguration(Object handler, Method method, String mapping) {
CorsConfiguration corsConfig = new CorsConfiguration();
corsConfig.setAllowedOrigins(Collections.singletonList("http://" + handler.hashCode() + method.getName()));
return corsConfig;
CrossOrigin crossOrigin = AnnotatedElementUtils.findMergedAnnotation(method, CrossOrigin.class);
if (crossOrigin != null) {
CorsConfiguration corsConfig = new CorsConfiguration();
corsConfig.setAllowedOrigins(Collections.singletonList("http://domain.com"));
return corsConfig;
}
return null;
}
@Override
@@ -325,5 +348,10 @@ public class HandlerMethodMappingTests {
@RequestMapping
public void handlerMethod2() {
}
@RequestMapping
@CrossOrigin(originPatterns = "*")
public void corsHandlerMethod() {
}
}
}