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

Enable/Disable CORS revisited [SPR-17598] #22130

Open
spring-issuemaster opened this issue Dec 13, 2018 · 2 comments

Comments

@spring-issuemaster
Copy link
Collaborator

@spring-issuemaster spring-issuemaster commented Dec 13, 2018

ayush-finix opened SPR-17598 and commented

Similar issue to #18266, however, disabling the cors processor doesn't work well enough.

We're building a proxy server where even the options requests should just be forwarded to the upstream, and the cors settings are somewhat interfering with this since there's CorsUtils.isPreflightRequest calls peppered around in various place (mostly referenced in the above ticket).

A few things I'd like to bring up:

After overriding RequestMappingHandlerMapping like

public class CorsNoopHandlerMapping extends RequestMappingHandlerMapping {

  public CorsNoopHandlerMapping() {
    setOrder(0);  // Make it override the default handler mapping.
  }

  @Override
  protected HandlerExecutionChain getCorsHandlerExecutionChain(HttpServletRequest request,
      HandlerExecutionChain chain, CorsConfiguration config) {
    return chain;  // Return the same chain it uses for everything else.
  }
}

in X extends WebMvcConfigurationSupport

@Override
protected RequestMappingHandlerMapping createRequestMappingHandlerMapping() {
  return new CorsNoopHandlerMapping();
}

Disabling the cors processor and setting spring.mvc.dispatch-options-request=true still resolves a controller like the below to method 2

 

@RequestMapping(path = "/**", method = {RequestMethod.OPTIONS})
 method1() {}

@RequestMapping(path = "/**", method = {RequestMethod.DELETE, RequestMethod.GET, RequestMethod.HEAD, RequestMethod.POST, RequestMethod.PATCH, RequestMethod.PUT, RequestMethod.TRACE})
method2() {}

because the default RequestMethodsRequestCondition still checks preflight requests

https://github.com/spring-projects/spring-framework/blob/v5.1.2.RELEASE/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/condition/RequestMethodsRequestCondition.java#L108

Aside: Why are all the condition classes final?

 

There's seems to be a lot of hoops to jump through to just make any OPTIONS call (with Origin header for example) resolve to something like method1, and it's really unfortunate that there's no sane way to override the CorsUtils methods or just disable the feature entirely. Sorry if this isn't the right place to discuss this, and please let me know if there's any more information necessary.


Affects: 5.1.2, 5.1.3

Issue Links:

  • #18266 Enable/Disable Spring CORS option
@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Dec 13, 2018

Brian Clozel commented

Could you elaborate on why overriding the cors processor is not enough? Could you provide a concise sample that shows that?

I'm not sure Spring MVC is the best choice for building a gateway; as you can see, the annotation model is less explicit and delegates the route matching to the engine, so you're not in full control of that part. The same thing will apply for content-negotiation, decoding the request body and error handling.

It looks like you're spending more time undoing the web support in Spring MVC than building your own app.

Have you considered using Spring WebFlux.fn or Spring Cloud Gateway?

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Dec 13, 2018

ayush-finix commented

I'll get a github example as soon as I can, but in the meanwhile, I'll try and explain it.

If CorsUtil.isPreflight request returns true, then overriding just the cors processor isn't good enough (in my case) for only two reasons (that I've run into so far).

  1. The default RequestMappingHandlerMapping throws out the normal handler execution chain:
  2. RequestMethodsRequestCondition returns null for a handler that should match: https://github.com/spring-projects/spring-framework/blob/a89e716cc71a8741385a0224b5d7eb7ce009e11a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/condition/RequestMethodsRequestCondition.java#L103 
  1. was fixable with the new HandlerMapping that I posted above, but 2) was significantly more complicated to try and fix through the same new HandlerMapping because RequestMappingInfo and RequestMethodsRequestCondition are both final. The current "fix" for 2) I've done is just to map a single method and check the http method to branch.
@RequestMapping(path = "/**")
public asdf(HttpServletRequest request) {
if (request.getMethod().equals("OPTIONS")) {...}

Spring WebFlux.fn seems like a better solution, but there don't seem to be many issues to get this working as a fairly simple mvc project, and I'm just working with what exists in our code base. From a cursory reading, everything in webflux looks almost fairly identical to an mvc app?

I'm not sure about the other points you mention. Content negotiation and decoding stay as is while errors mostly map to 407 or 502s or just pass back whatever the target server responds with (which from this server's perspective is a success).

 

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant
You can’t perform that action at this time.