Refine CORS documentation for wildcard processing

[lowcasz]

In CORS configuration we don't have access for request and response, e.g. by servlet filter
We have allowedOriginsPattern to override * wildcard when we are using allowCredentials=true
In this situation we overwrite allowed-origin header by request origin, but heders and methods are still not overwrite wildcard which is not allowed by documentation when we allow credentials.

When some configuration for requestHeaders, exposedHeaders, allowedMethods is null then all is clear,
but when we have allowed credentials and some list is defined with one value with asterisk then it should be overwritten according to CORS documentation.

It is problematically for preflighted requests when we don't know all request headers or exposed headers,
but in single request it should be overwritten according to headers from request and response when we allow credentials,
or we should have possibility to calculate cors headers dynamically based on request and response to fix it manually.

[bclozel]

From the description of this issue, it's not clear whether you are reporting a bug, requesting an enhancement, or asking a question.

Can we take a step back and start over with:

• what are you trying to achieve
• what have you tried (a code snippet of the CORS configuration you are using)
• what's the behavior your are getting
• what behavior you were expecting itself

You can read up good advice on how to create a minimal sample if this is a better option for you.

[lowcasz]

What am I trying to achieve?

• I want extend default CORS configuration to pass everything with any origin, method, requestHeaders, exposedHeaders, allowedCredentials.

What am I tried?
cors-demo.zip

@Configuration
@EnableWebMvc
@RequiredArgsConstructor
public class CorsConfig implements WebMvcConfigurer {

    private final CorsProperties corsProperties;

    @Override
    public void addCorsMappings(CorsRegistry registry) {
        registry.addMapping("/**").combine(corsProperties.applyPermitDefaultValues());
    }

}

package example;

import static java.util.Objects.isNull;
import static org.springframework.util.CollectionUtils.isEmpty;

import org.springframework.boot.context.properties.ConfigurationProperties;
import org.springframework.web.cors.CorsConfiguration;

@ConfigurationProperties("cors")
public class CorsProperties extends CorsConfiguration {

    @Override
    public CorsConfiguration applyPermitDefaultValues() {
        if (isEmpty(getAllowedOrigins()) && isEmpty(getAllowedOrigins())) {
            addAllowedOriginPattern(ALL);
        }
        if (isEmpty(getAllowedMethods())) {
            addAllowedMethod(ALL);
        }
        if (isEmpty(getAllowedHeaders())) {
            addAllowedHeader(ALL);
        }
        if (isEmpty(getExposedHeaders())) {
            addExposedHeader(ALL);
        }
        if (isNull(getMaxAge())) {
            setMaxAge(1800L);
        }
        if (isNull(getAllowCredentials())) {
            setAllowCredentials(true);
            //todo owerwrite "*" (ALL="*") in all fields dynamically based on request and response, when allow credentials
        }
        return this;
    }

}

Whats behaviour am I gets?

What is expected bahaviour?
CORS documentation don't allow * wildcard (in allowOrigin, methods, requestHeaders, exposedHeaders) when allowCredentials is true.
In this situation, when wildcard was declared in configuration, all response headers should contains listed all needed values instead of *, but response header contains *:
Access-Control-Expose-Headers: *
instead of: Access-Control-Expose-Headers: Location

[sdeleuze]

Looks like there is potentially 2 things here:

• A potential bug related to the fact that we are not preventing users to specify * for Control-Allow-Headers, Control-Allow-Methods and Control-Expose-Headers like we do for Origin when credentials are enabled. Both Mozilla documentation related section and WHATWG specs are mentioning that (could have been updated or we missed that, not sure)
• A feature request to manage patterns for allowed methods, request headers, exposed headers like we do for allowed origins (see related Support for pattern based origin cors configuration #25016 issue).

@lowcasz Could you please confirm your are raising both the described potential bug and enhancement request?

[lowcasz]

Sure ;) It can be like that. We are talking about situation when allowCredentials (Access-Control-Allow-Credentials) is true only. Without credentials actual version is ok.

[lowcasz]

I'm sorry for late answer. I tested it on chrome only, and this browser actually works with asterisk even if credentials are allowed.
So in this situation maybe it is not the best way to totally block it even if documentation requires other behavior.
In this situation I think bugfix as restriction is no needed and only pattern feauture is needed.

[sdeleuze]

After a discussion with the team, for headers and methods, we are not going to introduce pattern management.

You can watch this draft commit to see the direction I am leaning towards.

I have implemented support for copying the request method and headers but I will likely not be able to support exposed headers as that would require, if I am not mistaken, to copy the response headers which are not available when the CorsProcessor is invoked. Could be in theory implemented by deferring this part of the processing after all filters + handler have been invoked, but that's out of the scope of this issue, and I am not sure it is worth the effort.

I tested it on chrome only, and this browser actually works with asterisk even if credentials are allowed.
So in this situation maybe it is not the best way to totally block it even if documentation requires other behavior.
In this situation I think bugfix as restriction is no needed and only pattern feauture is needed.

Are you talking about allowed methods, allowed headers or exposed headers here? If this is about exposed headers, did you checked headers were accessible from JavaScript with Access-Control-Expose-Headers: true when credentials are enabled?

[lowcasz]

I have read actual Bean defining CORS and I know it is difficult to change. It is reason why I wrote it instead extend this class or create additional filters, becase o lot of people have the same problem and should be resolved on library level.
I don't understand your question because is wrong. :/ I hope you mean: Access-Control-Expose-Headers: *
So yes, I tested it and was problem without Access-Control-Expose-Headers, but add it with * resolve my problem on chrome which uses JS for requests. I don't know yet how many browsers support * with enabled credentials.
Documentation of CORS describe the *, cannot be used in any headers like allowed origins, methods, headers, exposed headers when credentials are enabled. Some day a lot of users using spring configured with credentials and * will have to make a complicated hotfixes when any browser will start support this documentation requirements. There is a lot of ways where application is not vulnerable for this kind of attack and CORS porotection is no needed, but we cannot just switch off it when browser will require it.

It is why I think it is important issue.

I know other problem without answer: How to process preflight requests?
So overwrite it by library in normally response is especially important.

Instead of copy response headers, we can solve this problem in other way which support preflight requests too. We can create new issue when it is needed.
We can replace * on all known headers/methods specified in documentations and add all custom headers specified in application property file, but it have one disadvantage.
It will be generate big response with a lot of boiler plate, that can be a overkill on every request.

I think that will be very important and hot feature, when any popular browser starts support this restrictions.
This feature will open way to other possibility: "disable" CORS protection. It could be great feauture for beginners in spring and applications/services without sensitive data. I can start new issue to connect it, when you think it is valuable feature.

[rstoyanchev]

It is reason why I wrote it instead extend this class or create additional filters, becase o lot of people have the same problem and should be resolved on library level.

Your example in #31143 (comment) shows overriding applyDefaultPermits. This method is used on startup to switch from more conservative defaults (for general purpose use) to more permissive, opinionated defaults with @CrossOrigin. At that stage, there is no way to know what request or response headers will be used at runtime, and no practical way to replace * from there.

At best we can make decisions at runtime, in DefaultCorsProcessor, as in the commit @sdeleuze pointed to where we can use the actual HTTP method, and the headers from "access-control-request-headers".

For expose headers, there isn't anything straight forward we can do to replace *. This is something that I believe an application would have to do, explicitly listing the headers it wants to expose, which can be done in global CORS config, and/or via annotations where needed.

[sdeleuze]

After some tests, I can confirm that Chrome, Safari and Firefox in practice all allow using Access-Control-Expose-Headers: * with enabled credentials (it is possible to get headers value from JavaScript), so it might potentially be relevant to not reject this combination since it works in practice and it is up to the browser's implementation to follow the spec or not. But we need to evaluate if that's OK from a security perspective or not for Spring.

[sdeleuze]

Please keep this issue open as we will do some refinements.