[Spring MVC] OpenApiValidationInterceptor interferes with CORS config

[Gediminas Rimša]

Our service has to return CORS headers with every response.

We are using standard Spring MVC CORS configuration, which adds a CorsFilter that adds CORS headers to all responses (this happens in the Filter before continuing with the filter chain - i.e. before actually handing the request in the Controller).

If some exception is thrown during request processing (e.g. NPE), CORS response headers are already present on the HTTP response object at that point in time.

However, when swagger-request-validator is in use and OpenAPI RESPONSE validation fails, OpenApiValidationInterceptor resets the response:

try {
   validationReportHandler.handleResponseReport(requestLoggingKey, validationReport);
} catch (final InvalidResponseException e) {
   // as an exception will rewrite the current, cached response it has to be reset
   cachedResponse.reset();     // <-- This clears CORS headers
   throw e;
}

It seems to me that just throwing the exception would suffice here, because the framework that handles the exception could then decide which parts of the response to clear (or to build a completely new response object).

[Sven Döring]

Did I understand this correctly, that this only happens if there was an Exception before? In normal cases that does not happen?

‌

Which version you are using? With the latest version 2.11.1, six days ago, there was a subtle change to the response validation.

[Gediminas Rimša]

We are using 2.9.0, but I have just confirmed it affects 2.11.1 as well.

The scenarios are the following:

1. Both request and response OpenAPI validation is successful → response includes CORS headers
2. Request OpenAPI validation fails → response includes CORS headers
3. Application-specific logic fails with some unhandled exception (e.g. a DB connection fails, or some NPE is thrown) → response includes CORS headers
4. Response OpenAPI validation fails → response does not include CORS headers ( ← this bug )

‌

[Sven Döring]

Do you configure a CORSFilter? Something like that?

@Bean
public CORSFilter corsFilter() {
  return new CORSFilter();
}

Or do you use the WebMvcConfigurer?

[Gediminas Rimša]

There are two ways to do that in Spring:

Without spring-security - implement WebMvcConfigurer and add:

    @Override
    public void addCorsMappings(CorsRegistry registry) {
        registry.addMapping("/**")
                .allowedOrigins("*")
                .allowedHeaders("*")
                .allowedMethods("*")
                .allowCredentials(false)
                .maxAge(Duration.ofHours(12).getSeconds());
    }

Or alternatively with spring-security - extend WebSecurityConfigurerAdapter and add something like this:

    @Override
    protected void configure(HttpSecurity httpSecurity) throws Exception {
        httpSecurity
                .cors(cors -> cors.configurationSource(corsConfigurationSource()));
    }

    private CorsConfigurationSource corsConfigurationSource() {
        CorsConfiguration configuration = new CorsConfiguration();
        configuration.setAllowedOrigins(Collections.singletonList("*"));
        configuration.setAllowedMethods(Collections.singletonList("*"));
        configuration.setAllowedHeaders(Collections.singletonList("*"));
        configuration.setAllowCredentials(false);
        configuration.setMaxAge(Duration.ofHours(12).getSeconds());
        UrlBasedCorsConfigurationSource source = new UrlBasedCorsConfigurationSource();
        source.registerCorsConfiguration("/**", configuration);
        return source;
    }

Both should have the same effect of configuring Spring’s standard org.springframework.web.filter.CorsFilter

[Sven Döring]

The CorsInterceptor defined in the AbstractHandlerMapping does that. Maybe the CorsFilter is involved with spring-security.

‌

As I never used the response validation after implementing it some years ago I tried removing this line cachedResponse.reset(); - as you suggested - and all the @SpringBootTest with invalid responses still work. I’ll take that chance and will prepare a change for this issue.

[Gediminas Rimša]

Yes, sorry, I was mistaken - it is CorsInterceptor that Spring configures by default.

Thanks!

[Sven Döring]

I’ve looked at it again.

There are two things I see critical about not resetting the response:
(1) The response headers might contain sensitive data of the original response that will still be there although the response body will be overwritten with the response validation error.
(2) A @ControllerAdvice for a InvalidResponseException does not work if the response has not been resetted. Even one of the integration tests fails: com.atlassian.oai.validator.springmvc.example.exceptionhandler.RestRequestValidationTest

‌

Perhaps the problem can be solved differently.
There is already an exceptional behaviour for CORS preflight requests. Those will not be validated. What if the vary header is not gonna reset but the rest?
Is the vary header the only header you miss?

[Gediminas Rimša]

There are two things I see critical about not resetting the response:
(1) The response headers might contain sensitive data of the original response that will still be there although the response body will be overwritten with the response validation error.

The way I see it, OpenApiValidationInterceptor should not assume how some other part of the application (that is outside of swagger-request-validator will handle the exception).

Currently, the interceptor does not modify the response itself, it only throws the exception. How this exception will be handled later does depend on the application. In theory, an application could decide to just catch the response validation exception, log it, but return the original response body unchanged (even if it is not valid). It is at that point - where InvalidResponseException is caught and handled - where any modification to response body/headers should take place.

(2) A @ControllerAdvice for a InvalidResponseException does not work if the response has not been resetted. Even one of the integration tests fails: com.atlassian.oai.validator.springmvc.example.exceptionhandler.RestRequestValidationTest

I checked out the repo, removed the cachedResponse.reset(); statement and ran mvn clean test . 3 tests failed, all in com.atlassian.oai.validator.springmvc.example.exceptionhandler.RestRequestValidationTest (9 passed in the same class).

However, in all those failing cases, the custom RestServiceExceptionHandler is invoked, and it returns the correct ResponseEntity<RestValidationReport> containing the appropriate report object. It is only after it goes through Spring MVC response serialization and deserialization when an empty response body is observed in RestRequestValidationTest#assertBadResponse, which then causes a NPE.

I assumed it could be some bug in Spring Boot, so I tried upgrading it to the latest 1.5 version, but got the same result.

After that I tested the fix in our app, and ended up with the correct response (HTTP 500 with CORS headers and proper response body) when I also added response buffer clearing in ExceptionHandler like this:

    @ExceptionHandler(InvalidResponseException.class)
    public ResponseEntity<String> handleOpenApiResponseValidationExceptions(RuntimeException ex, HttpServletResponse response) {
        response.resetBuffer();     // had to add this line
        return ResponseEntity.status(INTERNAL_SERVER_ERROR).body(ex.getMessage());
    }

Honestly, I did not expect this - I assumed Spring clears the response automatically.

There is already an exceptional behaviour for CORS preflight requests. Those will not be validated. What if the vary header is not gonna reset but the rest?
Is the vary header the only header you miss?

In our application validation success response has the following headers:

Vary: Origin
Vary: Access-Control-Request-Method
Vary: Access-Control-Request-Headers
Access-Control-Allow-Origin: http://test-domain.com
Access-Control-Allow-Credentials: true
X-Content-Type-Options: nosniff
X-XSS-Protection: 1; mode=block
Cache-Control: no-cache, no-store, max-age=0, must-revalidate
Pragma: no-cache
Expires: 0
X-Frame-Options: DENY
Content-Type: application/json
Content-Length: 130
Date: Wed, 09 Dec 2020 20:08:04 GMT
Keep-Alive: timeout=60
Connection: keep-alive

While validation failure response (with 2.9.0 or 2.11.1) has the following headers:

X-Content-Type-Options: nosniff
X-XSS-Protection: 1; mode=block
Cache-Control: no-cache, no-store, max-age=0, must-revalidate
Pragma: no-cache
Expires: 0
X-Frame-Options: DENY
Content-Type: text/plain;charset=UTF-8
Content-Length: 400
Date: Wed, 09 Dec 2020 20:09:15 GMT
Connection: close

i.e. not only Vary, but Access-Control-Allow-* headers are missing as well.

[Gediminas Rimša]

I was thinking a bit about possible solutions here. So far a few came to my mind:

• Remove response.reset() from OpenApiValidationInterceptor and let the app using the lib determine how to proceed. Two ways it could do that:

  • App has to implement exception handler that decides which parts of response should be reset (in theory the app may only want to log the error and still return the invalid response anyway) - this could be a breaking change for lib users

• Provide alternative implementation allowing the App to choose which behavior they want.

  • For example, the App can decorate OpenApiValidationInterceptor when registering it with Spring, something like

    public void addInterceptors(final InterceptorRegistry registry) { registry.addInterceptor( new InvalidResponseResettingOpenApiValidationInterceptorDecorator( new OpenApiValidationInterceptor( ... ) ) ); }

    This would mean that lib users would have to change their config slightly.

  • Another alternative could be to extract a non-resetting version of the interceptor into something like InvalidResponseRetainingOpenApiValidationInterceptor (implemented in the lib), while making OpenApiValidationInterceptor delegate to it. This way existing lib users would not have to change, yet others like me could switch to the alternative providing manual control.

• Keep resetting the response, but add original response headers and (invalid) body to InvalidResponseException so that the app handling the exception could re-add them to the response in the exception handler.

  • I quite like this idea - it seems it would be a non-breaking change, at the same time allowing for more control in the app.

What do you think? Would any of these work?

[Sven Döring]

@Gediminas Rimša Are you able to use a snapshot version of the swagger-request-validator?
I’ve prepared a fix for this issue but instead of creating a not completely working pull-request you maybe can test it beforehand: https://bitbucket.org/sdoeringNew/swagger-request-validator/src/issue_310/

I’ve tried it with an WebSecurityConfigurerAdapter and the three mentioned headers are still available after an invalid response.

[Gediminas Rimša]

@Sven Döring Built the snapshot version from that branch and tried it. Got the following exception:

o.a.c.c.C.[.[localhost].[/].[dispatcherServlet] - Servlet.service() for servlet [dispatcherServlet] in context with path [] threw exception [Request processing failed; nested exception is java.lang.IllegalStateException: Duplicate key Vary (attempted merging values [Origin, Access-Control-Request-Method, Access-Control-Request-Headers] and [Origin, Access-Control-Request-Method, Access-Control-Request-Headers])] with root causejava.lang.IllegalStateException: Duplicate key Vary (attempted merging values [Origin, Access-Control-Request-Method, Access-Control-Request-Headers] and [Origin, Access-Control-Request-Method, Access-Control-Request-Headers])
    at java.base/java.util.stream.Collectors.duplicateKeyException(Collectors.java:133) ~[na:na]
    at java.base/java.util.stream.Collectors.lambda$uniqKeysMapAccumulator$1(Collectors.java:180) ~[na:na]
    at java.base/java.util.stream.ReduceOps$3ReducingSink.accept(ReduceOps.java:169) ~[na:na]
    at java.base/java.util.ArrayList$ArrayListSpliterator.forEachRemaining(ArrayList.java:1625) ~[na:na]
    at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:484) ~[na:na]
    at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:474) ~[na:na]
    at java.base/java.util.stream.ReduceOps$ReduceOp.evaluateSequential(ReduceOps.java:913) ~[na:na]
    at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234) ~[na:na]
    at java.base/java.util.stream.ReferencePipeline.collect(ReferencePipeline.java:578) ~[na:na]
    at com.atlassian.oai.validator.springmvc.OpenApiValidationService.lambda$resolveHeadersOnResponse$1(OpenApiValidationService.java:157) ~[swagger-request-validator-springmvc-2.12.2-SNAPSHOT.jar:na]
    at java.base/java.util.Optional.map(Optional.java:258) ~[na:na]
    at com.atlassian.oai.validator.springmvc.OpenApiValidationService.resolveHeadersOnResponse(OpenApiValidationService.java:155) ~[swagger-request-validator-springmvc-2.12.2-SNAPSHOT.jar:na]
    at com.atlassian.oai.validator.springmvc.OpenApiValidationInterceptor.preHandle(OpenApiValidationInterceptor.java:106) ~[swagger-request-validator-springmvc-2.12.2-SNAPSHOT.jar:na]
    ...

Response at the time of failure returns:

servletResponse.getHeaderNames().toString() => [Vary, Vary, Vary, Access-Control-Allow-Origin]

I tried adding .distinct() step in here:

    Map<String, List<String>> resolveHeadersOnResponse(final HttpServletResponse servletResponse) {
        return Optional.ofNullable(servletResponse.getHeaderNames())
                .map(headerNames ->
                        headerNames.stream()
                                .distinct()          // <-- this
                                .collect(toMap(identity(), headerName -> (List<String>) newArrayList(servletResponse.getHeaders(headerName))))
                ).orElse(emptyMap());
    }

And after that, I was able to receive the expected response - OpenAPI response validation failure with all expected CORS headers.

[Sven Döring]

That’s strange. Why do you have duplicate header names?

However I ignore those duplicates now and added a PR #213.

[James Navin]

Available in 2.32.1