Refine CORS wildcard processing

This commit refines CORS wildcard processing in order to avoid sending a wildcard for Access-Control-Allow-Headers and Access-Control-Allow-Methods when credentials are enabled for non-preflight requests, as required by the specification. In that case, Access-Control-Allow-Headers and Access-Control-Allow-Methods values are copied from the request. For Access-Control-Expose-Headers, this is not possible since that would require to copy the response headers which are not available at the point when the CorsProcessor is invoked. Since all the major browsers seems to support wildcard including on requests with credentials, and since this is ultimately their responsibility to check on client-side what is authorized or not, Spring Framework continue to support this use case with proper mention in the Javadoc.
sdeleuze · Sep 11, 2023 · 35e590e · 35e590e
1 parent ed83461
commit 35e590e
Show file tree

Hide file tree

Showing 6 changed files with 114 additions and 11 deletions.
diff --git a/spring-web/src/main/java/org/springframework/web/cors/CorsConfiguration.java b/spring-web/src/main/java/org/springframework/web/cors/CorsConfiguration.java
@@ -412,8 +412,8 @@ public List<String> getExposedHeaders() {
 
 	/**
 	 * Add a response header to expose.
-	 * <p>The special value {@code "*"} allows all headers to be exposed for
-	 * non-credentialed requests.
+	 * <p>The special value {@code "*"} allows all headers to be exposed, including
+	 * for credentialed requests since major browsers support it.
 	 */
 	public void addExposedHeader(String exposedHeader) {
 		if (this.exposedHeaders == null) {

diff --git a/spring-web/src/main/java/org/springframework/web/cors/DefaultCorsProcessor.java b/spring-web/src/main/java/org/springframework/web/cors/DefaultCorsProcessor.java
@@ -139,19 +139,20 @@ protected boolean handleInternal(ServerHttpRequest request, ServerHttpResponse r
 
 		responseHeaders.setAccessControlAllowOrigin(allowOrigin);
 
-		if (preFlightRequest) {
+		boolean allowCredentials = Boolean.TRUE.equals(config.getAllowCredentials());
+		if (preFlightRequest || allowCredentials) {
 			responseHeaders.setAccessControlAllowMethods(allowMethods);
 		}
 
-		if (preFlightRequest && !allowHeaders.isEmpty()) {
+		if ((preFlightRequest || allowCredentials) && !CollectionUtils.isEmpty(allowHeaders)) {
 			responseHeaders.setAccessControlAllowHeaders(allowHeaders);
 		}
 
 		if (!CollectionUtils.isEmpty(config.getExposedHeaders())) {
 			responseHeaders.setAccessControlExposeHeaders(config.getExposedHeaders());
 		}
 
-		if (Boolean.TRUE.equals(config.getAllowCredentials())) {
+		if (allowCredentials) {
 			responseHeaders.setAccessControlAllowCredentials(true);
 		}
 
@@ -190,8 +191,7 @@ private HttpMethod getMethodToUse(ServerHttpRequest request, boolean isPreFlight
 
 	/**
 	 * Check the headers and determine the headers for the response of a
-	 * pre-flight request. The default implementation simply delegates to
-	 * {@link org.springframework.web.cors.CorsConfiguration#checkOrigin(String)}.
+	 * pre-flight or credentialed request.
 	 */
 	@Nullable
 	protected List<String> checkHeaders(CorsConfiguration config, List<String> requestHeaders) {

diff --git a/spring-web/src/main/java/org/springframework/web/cors/reactive/DefaultCorsProcessor.java b/spring-web/src/main/java/org/springframework/web/cors/reactive/DefaultCorsProcessor.java
@@ -137,19 +137,20 @@ protected boolean handleInternal(ServerWebExchange exchange,
 
 		responseHeaders.setAccessControlAllowOrigin(allowOrigin);
 
-		if (preFlightRequest) {
+		boolean allowCredentials = Boolean.TRUE.equals(config.getAllowCredentials());
+		if (preFlightRequest || allowCredentials) {
 			responseHeaders.setAccessControlAllowMethods(allowMethods);
 		}
 
-		if (preFlightRequest && !allowHeaders.isEmpty()) {
+		if ((preFlightRequest || allowCredentials) && !CollectionUtils.isEmpty(allowHeaders)) {
 			responseHeaders.setAccessControlAllowHeaders(allowHeaders);
 		}
 
 		if (!CollectionUtils.isEmpty(config.getExposedHeaders())) {
 			responseHeaders.setAccessControlExposeHeaders(config.getExposedHeaders());
 		}
 
-		if (Boolean.TRUE.equals(config.getAllowCredentials())) {
+		if (allowCredentials) {
 			responseHeaders.setAccessControlAllowCredentials(true);
 		}
 

diff --git a/spring-web/src/test/java/org/springframework/web/cors/CorsConfigurationTests.java b/spring-web/src/test/java/org/springframework/web/cors/CorsConfigurationTests.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2022 the original author or authors.
+ * Copyright 2002-2023 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.
@@ -19,6 +19,7 @@
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
+import java.util.List;
 
 import org.junit.jupiter.api.Test;
 
@@ -448,4 +449,13 @@ void permitDefaultDoesntSetOriginWhenPatternPresent() {
 		assertThat(config.getAllowedOrigins()).isNull();
 		assertThat(config.getAllowedOriginPatterns()).containsExactly("http://*.com");
 	}
+
+	@Test
+	void validateAllowCredentialsWithAllOrigins() {
+		CorsConfiguration config = new CorsConfiguration();
+		config.setAllowCredentials(true);
+		config.setAllowedOrigins(List.of(CorsConfiguration.ALL));
+		assertThatIllegalArgumentException().isThrownBy(config::validateAllowCredentials);
+	}
+
 }
diff --git a/spring-web/src/test/java/org/springframework/web/cors/DefaultCorsProcessorTests.java b/spring-web/src/test/java/org/springframework/web/cors/DefaultCorsProcessorTests.java
@@ -162,6 +162,50 @@ public void actualRequestCredentialsWithWildcardOrigin() throws Exception {
 		assertThat(this.response.getStatus()).isEqualTo(HttpServletResponse.SC_OK);
 	}
 
+	@Test
+	public void actualRequestCredentialsWithWildcardMethod() throws Exception {
+		this.request.setMethod(HttpMethod.GET.name());
+		this.request.addHeader(HttpHeaders.ORIGIN, "https://domain2.com");
+
+		this.conf.addAllowedOrigin("https://domain2.com");
+		this.conf.setAllowCredentials(true);
+		this.conf.addAllowedMethod(CorsConfiguration.ALL);
+
+		this.processor.processRequest(this.conf, this.request, this.response);
+		assertThat(this.response.containsHeader(HttpHeaders.ACCESS_CONTROL_ALLOW_ORIGIN)).isTrue();
+		assertThat(this.response.getHeader(HttpHeaders.ACCESS_CONTROL_ALLOW_ORIGIN)).isEqualTo("https://domain2.com");
+		assertThat(this.response.containsHeader(HttpHeaders.ACCESS_CONTROL_ALLOW_CREDENTIALS)).isTrue();
+		assertThat(this.response.getHeader(HttpHeaders.ACCESS_CONTROL_ALLOW_CREDENTIALS)).isEqualTo("true");
+		assertThat(this.response.containsHeader(HttpHeaders.ACCESS_CONTROL_ALLOW_METHODS)).isTrue();
+		assertThat(this.response.getHeader(HttpHeaders.ACCESS_CONTROL_ALLOW_METHODS)).isEqualTo(HttpMethod.GET.toString());
+		assertThat(this.response.getHeaders(HttpHeaders.VARY)).contains(HttpHeaders.ORIGIN,
+				HttpHeaders.ACCESS_CONTROL_REQUEST_METHOD, HttpHeaders.ACCESS_CONTROL_REQUEST_HEADERS);
+		assertThat(this.response.getStatus()).isEqualTo(HttpServletResponse.SC_OK);
+	}
+
+	@Test
+	public void actualRequestCredentialsWithWildcardAllowedHeader() throws Exception {
+		this.request.setMethod(HttpMethod.GET.name());
+		this.request.addHeader(HttpHeaders.ORIGIN, "https://domain2.com");
+		this.request.addHeader(HttpHeaders.AGE, "0");
+
+		this.conf.addAllowedOrigin("https://domain2.com");
+		this.conf.setAllowCredentials(true);
+		this.conf.addAllowedHeader(CorsConfiguration.ALL);
+
+		this.processor.processRequest(this.conf, this.request, this.response);
+		assertThat(this.response.containsHeader(HttpHeaders.ACCESS_CONTROL_ALLOW_ORIGIN)).isTrue();
+		assertThat(this.response.getHeader(HttpHeaders.ACCESS_CONTROL_ALLOW_ORIGIN)).isEqualTo("https://domain2.com");
+		assertThat(this.response.containsHeader(HttpHeaders.ACCESS_CONTROL_ALLOW_CREDENTIALS)).isTrue();
+		assertThat(this.response.getHeader(HttpHeaders.ACCESS_CONTROL_ALLOW_CREDENTIALS)).isEqualTo("true");
+		assertThat(this.response.containsHeader(HttpHeaders.ACCESS_CONTROL_ALLOW_HEADERS)).isTrue();
+		assertThat(this.response.getHeader(HttpHeaders.ACCESS_CONTROL_ALLOW_HEADERS))
+				.contains(HttpHeaders.ORIGIN + ", " + HttpHeaders.AGE);
+		assertThat(this.response.getHeaders(HttpHeaders.VARY)).contains(HttpHeaders.ORIGIN,
+				HttpHeaders.ACCESS_CONTROL_REQUEST_METHOD, HttpHeaders.ACCESS_CONTROL_REQUEST_HEADERS);
+		assertThat(this.response.getStatus()).isEqualTo(HttpServletResponse.SC_OK);
+	}
+
 	@Test
 	public void actualRequestCaseInsensitiveOriginMatch() throws Exception {
 		this.request.setMethod(HttpMethod.GET.name());

diff --git a/...ng-web/src/test/java/org/springframework/web/cors/reactive/DefaultCorsProcessorTests.java b/...ng-web/src/test/java/org/springframework/web/cors/reactive/DefaultCorsProcessorTests.java
@@ -165,6 +165,54 @@ public void actualRequestCredentialsWithWildcardOrigin() {
 		assertThat((Object) response.getStatusCode()).isNull();
 	}
 
+	@Test
+	public void actualRequestCredentialsWithWildcardMethod() {
+		ServerWebExchange exchange = MockServerWebExchange.from(MockServerHttpRequest
+				.method(HttpMethod.GET, "http://localhost/test.html")
+				.header(HttpHeaders.ORIGIN, "https://domain2.com"));
+
+		this.conf.addAllowedOrigin("https://domain2.com");
+		this.conf.setAllowCredentials(true);
+		this.conf.addAllowedMethod(CorsConfiguration.ALL);
+		this.processor.process(this.conf, exchange);
+
+		ServerHttpResponse response = exchange.getResponse();
+		assertThat(response.getHeaders().containsKey(HttpHeaders.ACCESS_CONTROL_ALLOW_ORIGIN)).isTrue();
+		assertThat(response.getHeaders().getFirst(HttpHeaders.ACCESS_CONTROL_ALLOW_ORIGIN)).isEqualTo("https://domain2.com");
+		assertThat(response.getHeaders().containsKey(HttpHeaders.ACCESS_CONTROL_ALLOW_CREDENTIALS)).isTrue();
+		assertThat(response.getHeaders().getFirst(HttpHeaders.ACCESS_CONTROL_ALLOW_CREDENTIALS)).isEqualTo("true");
+		assertThat(response.getHeaders().containsKey(HttpHeaders.ACCESS_CONTROL_ALLOW_METHODS)).isTrue();
+		assertThat(response.getHeaders().getFirst(HttpHeaders.ACCESS_CONTROL_ALLOW_METHODS)).isEqualTo(HttpMethod.GET.toString());
+		assertThat(response.getHeaders().get(HttpHeaders.VARY)).contains(HttpHeaders.ORIGIN,
+				HttpHeaders.ACCESS_CONTROL_REQUEST_METHOD, HttpHeaders.ACCESS_CONTROL_REQUEST_HEADERS);
+		assertThat((Object) response.getStatusCode()).isNull();
+	}
+
+	@Test
+	public void actualRequestCredentialsWithWildcardAllowedHeader() throws Exception {
+		ServerWebExchange exchange = MockServerWebExchange.from(MockServerHttpRequest
+				.method(HttpMethod.GET, "http://localhost/test.html")
+				.header(HttpHeaders.ORIGIN, "https://domain2.com")
+				.header(HttpHeaders.AGE, "0"));
+
+		this.conf.addAllowedOrigin("https://domain2.com");
+		this.conf.setAllowCredentials(true);
+		this.conf.addAllowedHeader(CorsConfiguration.ALL);
+		this.processor.process(this.conf, exchange);
+
+		ServerHttpResponse response = exchange.getResponse();
+		assertThat(response.getHeaders().containsKey(HttpHeaders.ACCESS_CONTROL_ALLOW_ORIGIN)).isTrue();
+		assertThat(response.getHeaders().getFirst(HttpHeaders.ACCESS_CONTROL_ALLOW_ORIGIN)).isEqualTo("https://domain2.com");
+		assertThat(response.getHeaders().containsKey(HttpHeaders.ACCESS_CONTROL_ALLOW_CREDENTIALS)).isTrue();
+		assertThat(response.getHeaders().getFirst(HttpHeaders.ACCESS_CONTROL_ALLOW_CREDENTIALS)).isEqualTo("true");
+		assertThat(response.getHeaders().containsKey(HttpHeaders.ACCESS_CONTROL_ALLOW_HEADERS)).isTrue();
+		assertThat(response.getHeaders().getFirst(HttpHeaders.ACCESS_CONTROL_ALLOW_HEADERS))
+				.contains(HttpHeaders.ORIGIN + ", " + HttpHeaders.AGE);
+		assertThat(response.getHeaders().get(HttpHeaders.VARY)).contains(HttpHeaders.ORIGIN,
+				HttpHeaders.ACCESS_CONTROL_REQUEST_METHOD, HttpHeaders.ACCESS_CONTROL_REQUEST_HEADERS);
+		assertThat((Object) response.getStatusCode()).isNull();
+	}
+
 	@Test
 	public void actualRequestCaseInsensitiveOriginMatch() {
 		ServerWebExchange exchange = actualRequest();