Skip to content

Commit 996c86f

Browse files
rstoyanchevrstoyanchev
committed
Suppress PREFLIGHT_AMBIGUOUS_MATCH if matches have no CORS config
Closes gh-26490
1 parent 0575e66 commit 996c86f

File tree

5 files changed

+215
-79
lines changed

5 files changed

+215
-79
lines changed

‎spring-webflux/src/main/java/org/springframework/web/reactive/result/method/AbstractHandlerMethodMapping.java

+48-21
Original file line numberDiff line numberDiff line change
@@ -330,19 +330,25 @@ protected HandlerMethod lookupHandlerMethod(ServerWebExchange exchange) throws E
330330
logger.trace(exchange.getLogPrefix() + matches.size() + " matching mappings: " + matches);
331331
}
332332
if (CorsUtils.isPreFlightRequest(exchange.getRequest())) {
333-
return PREFLIGHT_AMBIGUOUS_MATCH;
333+
for (Match match : matches) {
334+
if (match.hasCorsConfig()) {
335+
return PREFLIGHT_AMBIGUOUS_MATCH;
336+
}
337+
}
334338
}
335-
Match secondBestMatch = matches.get(1);
336-
if (comparator.compare(bestMatch, secondBestMatch) == 0) {
337-
Method m1 = bestMatch.handlerMethod.getMethod();
338-
Method m2 = secondBestMatch.handlerMethod.getMethod();
339-
RequestPath path = exchange.getRequest().getPath();
340-
throw new IllegalStateException(
341-
"Ambiguous handler methods mapped for '" + path + "': {" + m1 + ", " + m2 + "}");
339+
else {
340+
Match secondBestMatch = matches.get(1);
341+
if (comparator.compare(bestMatch, secondBestMatch) == 0) {
342+
Method m1 = bestMatch.getHandlerMethod().getMethod();
343+
Method m2 = secondBestMatch.getHandlerMethod().getMethod();
344+
RequestPath path = exchange.getRequest().getPath();
345+
throw new IllegalStateException(
346+
"Ambiguous handler methods mapped for '" + path + "': {" + m1 + ", " + m2 + "}");
347+
}
342348
}
343349
}
344-
handleMatch(bestMatch.mapping, bestMatch.handlerMethod, exchange);
345-
return bestMatch.handlerMethod;
350+
handleMatch(bestMatch.mapping, bestMatch.getHandlerMethod(), exchange);
351+
return bestMatch.getHandlerMethod();
346352
}
347353
else {
348354
return handleNoMatch(this.mappingRegistry.getRegistrations().keySet(), exchange);
@@ -353,8 +359,7 @@ private void addMatchingMappings(Collection<T> mappings, List<Match> matches, Se
353359
for (T mapping : mappings) {
354360
T match = getMatchingMapping(mapping, exchange);
355361
if (match != null) {
356-
matches.add(new Match(match,
357-
this.mappingRegistry.getRegistrations().get(mapping).getHandlerMethod()));
362+
matches.add(new Match(match, this.mappingRegistry.getRegistrations().get(mapping)));
358363
}
359364
}
360365
}
@@ -519,13 +524,14 @@ public void register(T mapping, Object handler, Method method) {
519524
this.pathLookup.add(path, mapping);
520525
}
521526

522-
CorsConfiguration config = initCorsConfiguration(handler, method, mapping);
523-
if (config != null) {
524-
config.validateAllowCredentials();
525-
this.corsLookup.put(handlerMethod, config);
527+
CorsConfiguration corsConfig = initCorsConfiguration(handler, method, mapping);
528+
if (corsConfig != null) {
529+
corsConfig.validateAllowCredentials();
530+
this.corsLookup.put(handlerMethod, corsConfig);
526531
}
527532

528-
this.registry.put(mapping, new MappingRegistration<>(mapping, handlerMethod, directPaths));
533+
this.registry.put(mapping,
534+
new MappingRegistration<>(mapping, handlerMethod, directPaths, corsConfig != null));
529535
}
530536
finally {
531537
this.readWriteLock.writeLock().unlock();
@@ -578,12 +584,17 @@ static class MappingRegistration<T> {
578584

579585
private final Set<String> directPaths;
580586

581-
public MappingRegistration(T mapping, HandlerMethod handlerMethod, @Nullable Set<String> directPaths) {
587+
private final boolean corsConfig;
588+
589+
public MappingRegistration(
590+
T mapping, HandlerMethod handlerMethod, @Nullable Set<String> directPaths, boolean corsConfig) {
591+
582592
Assert.notNull(mapping, "Mapping must not be null");
583593
Assert.notNull(handlerMethod, "HandlerMethod must not be null");
584594
this.mapping = mapping;
585595
this.handlerMethod = handlerMethod;
586596
this.directPaths = (directPaths != null ? directPaths : Collections.emptySet());
597+
this.corsConfig = corsConfig;
587598
}
588599

589600
public T getMapping() {
@@ -597,6 +608,10 @@ public HandlerMethod getHandlerMethod() {
597608
public Set<String> getDirectPaths() {
598609
return this.directPaths;
599610
}
611+
612+
public boolean hasCorsConfig() {
613+
return this.corsConfig;
614+
}
600615
}
601616

602617

@@ -608,11 +623,23 @@ private class Match {
608623

609624
private final T mapping;
610625

611-
private final HandlerMethod handlerMethod;
626+
private final MappingRegistration<T> registration;
612627

613-
public Match(T mapping, HandlerMethod handlerMethod) {
628+
public Match(T mapping, MappingRegistration<T> registration) {
614629
this.mapping = mapping;
615-
this.handlerMethod = handlerMethod;
630+
this.registration = registration;
631+
}
632+
633+
public T getMapping() {
634+
return this.mapping;
635+
}
636+
637+
public HandlerMethod getHandlerMethod() {
638+
return this.registration.getHandlerMethod();
639+
}
640+
641+
public boolean hasCorsConfig() {
642+
return this.registration.hasCorsConfig();
616643
}
617644

618645
@Override

‎spring-webflux/src/test/java/org/springframework/web/reactive/result/method/HandlerMethodMappingTests.java

+57-1
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2002-2020 the original author or authors.
2+
* Copyright 2002-2021 the original author or authors.
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
@@ -28,12 +28,19 @@
2828
import reactor.core.publisher.Mono;
2929
import reactor.test.StepVerifier;
3030

31+
import org.springframework.core.annotation.AnnotatedElementUtils;
32+
import org.springframework.http.HttpHeaders;
33+
import org.springframework.http.HttpMethod;
34+
import org.springframework.http.HttpStatus;
3135
import org.springframework.http.server.PathContainer;
3236
import org.springframework.stereotype.Controller;
37+
import org.springframework.web.bind.annotation.CrossOrigin;
3338
import org.springframework.web.bind.annotation.RequestMapping;
39+
import org.springframework.web.cors.CorsConfiguration;
3440
import org.springframework.web.method.HandlerMethod;
3541
import org.springframework.web.server.ServerWebExchange;
3642
import org.springframework.web.testfixture.http.server.reactive.MockServerHttpRequest;
43+
import org.springframework.web.testfixture.http.server.reactive.MockServerHttpResponse;
3744
import org.springframework.web.testfixture.server.MockServerWebExchange;
3845
import org.springframework.web.util.pattern.PathPattern;
3946
import org.springframework.web.util.pattern.PathPatternParser;
@@ -105,6 +112,39 @@ public void ambiguousMatch() {
105112
StepVerifier.create(result).expectError(IllegalStateException.class).verify();
106113
}
107114

115+
@Test // gh-26490
116+
public void ambiguousMatchOnPreFlightRequestWithoutCorsConfig() throws Exception {
117+
this.mapping.registerMapping("/f?o", this.handler, this.method1);
118+
this.mapping.registerMapping("/fo?", this.handler, this.method2);
119+
120+
MockServerWebExchange exchange = MockServerWebExchange.from(
121+
MockServerHttpRequest.options("http://example.org/foo")
122+
.header(HttpHeaders.ORIGIN, "https://domain.com")
123+
.header(HttpHeaders.ACCESS_CONTROL_REQUEST_METHOD, "GET"));
124+
125+
this.mapping.getHandler(exchange).block();
126+
127+
assertThat(exchange.getResponse().getStatusCode()).isEqualTo(HttpStatus.FORBIDDEN);
128+
}
129+
130+
@Test // gh-26490
131+
public void ambiguousMatchOnPreFlightRequestWithCorsConfig() throws Exception {
132+
this.mapping.registerMapping("/f?o", this.handler, this.method1);
133+
this.mapping.registerMapping("/fo?", this.handler, this.handler.getClass().getMethod("corsHandlerMethod"));
134+
135+
MockServerWebExchange exchange = MockServerWebExchange.from(
136+
MockServerHttpRequest.options("http://example.org/foo")
137+
.header(HttpHeaders.ORIGIN, "https://domain.com")
138+
.header(HttpHeaders.ACCESS_CONTROL_REQUEST_METHOD, "GET"));
139+
140+
this.mapping.getHandler(exchange).block();
141+
142+
MockServerHttpResponse response = exchange.getResponse();
143+
assertThat(response.getStatusCode()).isNull();
144+
assertThat(response.getHeaders().getAccessControlAllowOrigin()).isEqualTo("https://domain.com");
145+
assertThat(response.getHeaders().getAccessControlAllowMethods()).containsExactly(HttpMethod.GET);
146+
}
147+
108148
@Test
109149
public void registerMapping() {
110150
String key1 = "/foo";
@@ -171,6 +211,17 @@ protected Set<String> getDirectPaths(String mapping) {
171211
Collections.emptySet() : Collections.singleton(mapping));
172212
}
173213

214+
@Override
215+
protected CorsConfiguration initCorsConfiguration(Object handler, Method method, String mapping) {
216+
CrossOrigin crossOrigin = AnnotatedElementUtils.findMergedAnnotation(method, CrossOrigin.class);
217+
if (crossOrigin != null) {
218+
CorsConfiguration corsConfig = new CorsConfiguration();
219+
corsConfig.setAllowedOrigins(Collections.singletonList("http://domain.com"));
220+
return corsConfig;
221+
}
222+
return null;
223+
}
224+
174225
@Override
175226
protected String getMatchingMapping(String pattern, ServerWebExchange exchange) {
176227
PathContainer lookupPath = exchange.getRequest().getPath().pathWithinApplication();
@@ -200,6 +251,11 @@ public void handlerMethod1() {
200251
@SuppressWarnings("unused")
201252
public void handlerMethod2() {
202253
}
254+
255+
@RequestMapping
256+
@CrossOrigin(originPatterns = "*")
257+
public void corsHandlerMethod() {
258+
}
203259
}
204260

205261
}

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

+4-4
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2002-2019 the original author or authors.
2+
* Copyright 2002-2021 the original author or authors.
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
@@ -160,9 +160,9 @@ void preFlightRequestWithAmbiguousMapping(HttpServer httpServer) throws Exceptio
160160
this.headers.add(HttpHeaders.ACCESS_CONTROL_REQUEST_METHOD, "GET");
161161
ResponseEntity<String> entity = performOptions("/ambiguous", this.headers, String.class);
162162
assertThat(entity.getStatusCode()).isEqualTo(HttpStatus.OK);
163-
assertThat(entity.getHeaders().getAccessControlAllowOrigin()).isEqualTo("http://localhost:9000");
164-
assertThat(entity.getHeaders().getAccessControlAllowMethods()).containsExactly(HttpMethod.GET);
165-
assertThat(entity.getHeaders().getAccessControlAllowCredentials()).isEqualTo(true);
163+
assertThat(entity.getHeaders().getAccessControlAllowOrigin()).isEqualTo("*");
164+
assertThat(entity.getHeaders().getAccessControlAllowMethods()).containsExactly(HttpMethod.GET, HttpMethod.POST);
165+
assertThat(entity.getHeaders().getAccessControlAllowCredentials()).isEqualTo(false);
166166
assertThat(entity.getHeaders().get(HttpHeaders.VARY))
167167
.containsExactly(HttpHeaders.ORIGIN, HttpHeaders.ACCESS_CONTROL_REQUEST_METHOD,
168168
HttpHeaders.ACCESS_CONTROL_REQUEST_HEADERS);

‎spring-webmvc/src/main/java/org/springframework/web/servlet/handler/AbstractHandlerMethodMapping.java

+46-21
Original file line numberDiff line numberDiff line change
@@ -407,20 +407,26 @@ protected HandlerMethod lookupHandlerMethod(String lookupPath, HttpServletReques
407407
logger.trace(matches.size() + " matching mappings: " + matches);
408408
}
409409
if (CorsUtils.isPreFlightRequest(request)) {
410-
return PREFLIGHT_AMBIGUOUS_MATCH;
410+
for (Match match : matches) {
411+
if (match.hasCorsConfig()) {
412+
return PREFLIGHT_AMBIGUOUS_MATCH;
413+
}
414+
}
411415
}
412-
Match secondBestMatch = matches.get(1);
413-
if (comparator.compare(bestMatch, secondBestMatch) == 0) {
414-
Method m1 = bestMatch.handlerMethod.getMethod();
415-
Method m2 = secondBestMatch.handlerMethod.getMethod();
416-
String uri = request.getRequestURI();
417-
throw new IllegalStateException(
418-
"Ambiguous handler methods mapped for '" + uri + "': {" + m1 + ", " + m2 + "}");
416+
else {
417+
Match secondBestMatch = matches.get(1);
418+
if (comparator.compare(bestMatch, secondBestMatch) == 0) {
419+
Method m1 = bestMatch.getHandlerMethod().getMethod();
420+
Method m2 = secondBestMatch.getHandlerMethod().getMethod();
421+
String uri = request.getRequestURI();
422+
throw new IllegalStateException(
423+
"Ambiguous handler methods mapped for '" + uri + "': {" + m1 + ", " + m2 + "}");
424+
}
419425
}
420426
}
421-
request.setAttribute(BEST_MATCHING_HANDLER_ATTRIBUTE, bestMatch.handlerMethod);
427+
request.setAttribute(BEST_MATCHING_HANDLER_ATTRIBUTE, bestMatch.getHandlerMethod());
422428
handleMatch(bestMatch.mapping, lookupPath, request);
423-
return bestMatch.handlerMethod;
429+
return bestMatch.getHandlerMethod();
424430
}
425431
else {
426432
return handleNoMatch(this.mappingRegistry.getRegistrations().keySet(), lookupPath, request);
@@ -431,8 +437,7 @@ private void addMatchingMappings(Collection<T> mappings, List<Match> matches, Ht
431437
for (T mapping : mappings) {
432438
T match = getMatchingMapping(mapping, request);
433439
if (match != null) {
434-
matches.add(new Match(match,
435-
this.mappingRegistry.getRegistrations().get(mapping).getHandlerMethod()));
440+
matches.add(new Match(match, this.mappingRegistry.getRegistrations().get(mapping)));
436441
}
437442
}
438443
}
@@ -630,13 +635,14 @@ public void register(T mapping, Object handler, Method method) {
630635
addMappingName(name, handlerMethod);
631636
}
632637

633-
CorsConfiguration config = initCorsConfiguration(handler, method, mapping);
634-
if (config != null) {
635-
config.validateAllowCredentials();
636-
this.corsLookup.put(handlerMethod, config);
638+
CorsConfiguration corsConfig = initCorsConfiguration(handler, method, mapping);
639+
if (corsConfig != null) {
640+
corsConfig.validateAllowCredentials();
641+
this.corsLookup.put(handlerMethod, corsConfig);
637642
}
638643

639-
this.registry.put(mapping, new MappingRegistration<>(mapping, handlerMethod, directPaths, name));
644+
this.registry.put(mapping,
645+
new MappingRegistration<>(mapping, handlerMethod, directPaths, name, corsConfig != null));
640646
}
641647
finally {
642648
this.readWriteLock.writeLock().unlock();
@@ -735,15 +741,18 @@ static class MappingRegistration<T> {
735741
@Nullable
736742
private final String mappingName;
737743

744+
private final boolean corsConfig;
745+
738746
public MappingRegistration(T mapping, HandlerMethod handlerMethod,
739-
@Nullable Set<String> directPaths, @Nullable String mappingName) {
747+
@Nullable Set<String> directPaths, @Nullable String mappingName, boolean corsConfig) {
740748

741749
Assert.notNull(mapping, "Mapping must not be null");
742750
Assert.notNull(handlerMethod, "HandlerMethod must not be null");
743751
this.mapping = mapping;
744752
this.handlerMethod = handlerMethod;
745753
this.directPaths = (directPaths != null ? directPaths : Collections.emptySet());
746754
this.mappingName = mappingName;
755+
this.corsConfig = corsConfig;
747756
}
748757

749758
public T getMapping() {
@@ -762,6 +771,10 @@ public Set<String> getDirectPaths() {
762771
public String getMappingName() {
763772
return this.mappingName;
764773
}
774+
775+
public boolean hasCorsConfig() {
776+
return this.corsConfig;
777+
}
765778
}
766779

767780

@@ -773,11 +786,23 @@ private class Match {
773786

774787
private final T mapping;
775788

776-
private final HandlerMethod handlerMethod;
789+
private final MappingRegistration<T> registration;
777790

778-
public Match(T mapping, HandlerMethod handlerMethod) {
791+
public Match(T mapping, MappingRegistration<T> registration) {
779792
this.mapping = mapping;
780-
this.handlerMethod = handlerMethod;
793+
this.registration = registration;
794+
}
795+
796+
public T getMapping() {
797+
return this.mapping;
798+
}
799+
800+
public HandlerMethod getHandlerMethod() {
801+
return this.registration.getHandlerMethod();
802+
}
803+
804+
public boolean hasCorsConfig() {
805+
return this.registration.hasCorsConfig();
781806
}
782807

783808
@Override

0 commit comments

Comments
 (0)