Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CorsRegistration#combine is a noop #26877

Closed
bertwin opened this issue Apr 29, 2021 · 1 comment
Closed

CorsRegistration#combine is a noop #26877

bertwin opened this issue Apr 29, 2021 · 1 comment
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: bug A general bug
Milestone

Comments

@bertwin
Copy link

bertwin commented Apr 29, 2021

The call to CorsConfiguration::combine returns a new configuration object, which is ignored.

public CorsRegistration combine(CorsConfiguration other) {
this.config.combine(other);
return this;
}

public CorsConfiguration combine(@Nullable CorsConfiguration other) {
if (other == null) {
return this;
}
// Bypass setAllowedOrigins to avoid re-compiling patterns
CorsConfiguration config = new CorsConfiguration(this);
List<String> origins = combine(getAllowedOrigins(), other.getAllowedOrigins());
List<OriginPattern> patterns = combinePatterns(this.allowedOriginPatterns, other.allowedOriginPatterns);
config.allowedOrigins = (origins == DEFAULT_PERMIT_ALL && !CollectionUtils.isEmpty(patterns) ? null : origins);
config.allowedOriginPatterns = patterns;
config.setAllowedMethods(combine(getAllowedMethods(), other.getAllowedMethods()));
config.setAllowedHeaders(combine(getAllowedHeaders(), other.getAllowedHeaders()));
config.setExposedHeaders(combine(getExposedHeaders(), other.getExposedHeaders()));
Boolean allowCredentials = other.getAllowCredentials();
if (allowCredentials != null) {
config.setAllowCredentials(allowCredentials);
}
Long maxAge = other.getMaxAge();
if (maxAge != null) {
config.setMaxAge(maxAge);
}
return config;
}

Affects: 5.3

I was expecting the following to work:

@Configuration
class CorsGlobalConfiguration : WebFluxConfigurer {

    override fun addCorsMappings(corsRegistry: CorsRegistry) {
        val config = CorsConfiguration()
        config.addAllowedOrigin("http://localhost:3000")
        config.addAllowedMethod("*")
        config.applyPermitDefaultValues()

        corsRegistry.addMapping("/api/**").combine(config)
    }
}

This works:

@Configuration
class CorsGlobalConfiguration : WebFluxConfigurer {

    override fun addCorsMappings(corsRegistry: CorsRegistry) {
          corsRegistry.addMapping("/api/**")
                .allowedOrigins("http://localhost:3000")
                .allowedMethods("*")

    }
}

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Apr 29, 2021
@sbrannen sbrannen added in: web Issues in web modules (web, webmvc, webflux, websocket) type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Apr 29, 2021
@sbrannen sbrannen added this to the 5.3.7 milestone Apr 29, 2021
@sbrannen
Copy link
Member

Thanks for bringing this to our attention... as your first issue raised against the Spring Framework!

This applies to both Web MVC and WebFlux:

  • org.springframework.web.reactive.config.CorsRegistration.combine(CorsConfiguration)
  • org.springframework.web.servlet.config.annotation.CorsRegistration.combine(CorsConfiguration)

See also: #25716

@rstoyanchev rstoyanchev self-assigned this May 4, 2021
@rstoyanchev rstoyanchev changed the title CorsRegistration::combine is a noop CorsRegistration#combine is a noop May 5, 2021
Zoran0104 pushed a commit to Zoran0104/spring-framework that referenced this issue Aug 20, 2021
lxbzmy pushed a commit to lxbzmy/spring-framework that referenced this issue Mar 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: bug A general bug
Projects
None yet
Development

No branches or pull requests

4 participants