CORS combine - override specific host defined in global conf with * in controller/method [SPR-15772]

[spring-projects-issues]

Cyprian Gracz opened SPR-15772 and commented

In my Spring Boot I have global CORS config:

@Configuration
@EnableWebMvc
public class CorsConfig extends WebMvcConfigurerAdapter {

	@Override
	public void addCorsMappings(CorsRegistry registry) {
		CorsRegistration config = registry.addMapping("/**");
		config.allowedOrigins("http://domain.com");
	}
}

Then I try to allow any origin to one of controllers:

@RestController
@CrossOrigin("*")
public class OpenController {
}

Because of this line:

	private List<String> combine(@Nullable List<String> source, @Nullable List<String> other) {
		if (other == null || other.contains(ALL)) {
			return (source != null ? source : Collections.emptyList());
}

in https://github.com/spring-projects/spring-framework/blob/master/spring-web/src/main/java/org/springframework/web/cors/CorsConfiguration.java#L373

CorsConfiguration gets "http://domain.com" as allowed domain because controller specifies ALL ( * ) as allowed origin, so it's "ignored".

Is this desired behaviour? Shouldn't controllers/methods always override global config?

---

Issue Links:

• DATAREST-1184 Adapt to CORS changes in Spring Framework 5.0.3
• Add Vary: Access-Control-Request-Method/Headers CORS headers [SPR-16413] #20959 Add Vary: Access-Control-Request-Method/Headers CORS headers

Referenced from: commits 0075f13

[spring-projects-issues]

Sébastien Deleuze commented

Thanks for your feedback, I will try to see if we can improve the CORS combining logic to handle this use case.

[spring-projects-issues]

Sébastien Deleuze commented

Cyprian Gracz While combination logic for lists is additive rather than override (and this is not something likely to change because it would be breaking), your use case highlights a problem in current algorithm which is not able to differentiate default permit values from a * explicitly set at controller or handler method level.

This commit differentiates these 2 use cases and now allows to use additive combination logic for lists even when * is configured, and also documents combination logic more extensively.

Could you please check with 5.0.3.BUILD-SNAPSHOT builds that you now get * and not http://domain.com with your use case ?

[spring-projects-issues]

Ladislav Sopko commented

Hello,
I meet some problems with new combine logic. I Override
org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerMapping
and I override method:

@Override
protected CorsConfiguration getCorsConfiguration(Object handler, HttpServletRequest request) {
	// This call return every time AbstractHandlerMethodMapping.ALLOW_CORS_CONFIG
	CorsConfiguration corsConfiguration = super.getCorsConfiguration(handler, request);
	
	// Here we take our store path specific cfg
	String lookupPath = getUrlPathHelper().getLookupPathForRequest(request);
	ContentStoreInfo info = ContentStoreUtils.findStore(contentStores, lookupPath);
        CorsConfigurationBuilder builder = new CorsConfigurationBuilder();
	CorsConfiguration storeCorsConfiguration = builder.build(info);  
	
	// and here we have problem, since corsConfiguration is 
	// AbstractHandlerMethodMapping.ALLOW_CORS_CONFIG our sotre config is lost
        // cause it will just force {"*"} for all
	return corsConfiguration == null ? storeCorsConfiguration
			: corsConfiguration.combine(storeCorsConfiguration);
}

Do I some mistake? Or there must be some fix done also in
org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerMapping
in order to proper use of new combine?

Thank You.

[spring-projects-issues]

Sébastien Deleuze commented

super.getCorsConfiguration(handler, request) returns AbstractHandlerMethodMapping.ALLOW_CORS_CONFIG when it is not able to determine CORS configuration in case of ambiguous match during preflight request, in order to defer that to the actual request where it will have the required information.
So with current implementation, you're config is not taken in account during preflight request, but is likely to be taken in account during the actual request.
Could you check it is the case?

[spring-projects-issues]

Ladislav Sopko commented

Hello,
The "Cors" things are a bit new for me. Actually ( followed your indication) I saw now, that test case which fail
is for "options" method, it actually (saw it while did debug) do preflight. I found this, cause I'm doing port of Paul Warren's
spring-content from spring boot 1.5 to spring-boot 2.0.x. In version 1.5.x it went well, now it fail cause test is awaiting
allowOrigin some url but it return "". In preflight request it return every time "" Is it correct?

Then what You said is true! In regular GET it return my CorsConfig cause super call return null and not AbstractHandlerMethodMapping.ALLOW_CORS_CONFIG so everything is working well.

It remain my doubt: is it correct return in preflight "*" in any case?
Is it not dangerous?
Or I just don't understand well Cors ? :)
Excuse me if I steal Your time, but I lost 2 days of debug, and this place was only one where I find relevant responses :9

Thank You.

[spring-projects-issues]

Ladislav Sopko commented

It strips * chars... There are 2 times " * ". in text :)

[spring-projects-issues]

Sébastien Deleuze commented

Preflight requests return "*" only when ambiguous matching is detected because @RequestMapping is richer than what preflight request defines.
It seems to me that in your case, your override with the new combine logic is fine (even if I can understand it is surprising): the CorsConfiguration is the combination of yours and Spring MVC one, and since Spring MVC can't determine it during preflight request, it is necessary to wait the actual request to perform such check with or without your custom config.

As described here, I think preflight requests have been introduced in CORS to avoid changing the rules for non-CORS aware servers, and not really to provide more security. In all cases, the relevant checks with the proper CorsConfiguration will be performed during the actual request that happen after each successful preflight request. The only improvement I could think about would be to return the combination of the various CorsConfiguration associated with the ambiguous handlers, but that's not trivial and IMO not worth the effort given what explained previously.

[spring-projects-issues]

Ladislav Sopko commented

Hi,
nice, I start to understand it better.
So I see 2 different solutions for me:

1: just modify test case to accept " * " for "options" method
2: detect preflight request in my override method getCorsConfiguration, and in that case return my crafted config

What is better in Your opinion?

Thank You .

[spring-projects-issues]

Sébastien Deleuze commented

If your CORS config is designed to be merged with Spring MVC one, solution 1 I guess.

[spring-projects-issues]

Sébastien Deleuze commented

FYI, I have added a CORS support section in Spring Framework 5 upgrade documentation.

[spring-projects-issues]

Ladislav Sopko commented

Thank You for your time.
I'll do as You point me :)