Add "Vary: Origin" header; do not set "Access-Control-Allow-Origin" h…

[teohhanhui]

…eader when origin is not allowed

Fixes #42, #50

[teohhanhui]

Failing tests are likely due to Travis changes...

[Seldaek]

This changes many things that are unrelated to your changes and it makes it quite hard to review and understand what you actually changed. The whole event dispatcher removal makes no sense to me, I don't think it's necessary, and the way we had it we avoided triggering the response listener unless necessary, which IMO is better. Could you please open a new PR (or two) with the individual changes you want to do?

[teohhanhui]

The refactoring is necessary because the Vary: Origin header should be set on all (potentially CORS) requests.

In any case, I think injecting the event dispatcher and adding the event listener that way is an anti-pattern.

[Seldaek]

Well that's your opinion but IMO it's less wasteful to call less listeners.

The main issue though is that changing it all at once makes it really hard to review so I'd be happy to check things one by one.

[teohhanhui]

Let us first agree on the intended outcomes...

Precondition: The request MUST have matching options (as obtained from the options resolver), which tells us that the path has been configured for CORS (edit: looks like this assumption is incorrect; there's a design flaw in the config). Otherwise we SHOULD skip it, be it during kernel.request or kernel.response.

1. The Access-Control-Allow-Origin header MUST be set to the value of the Origin header (actually this is not so straightforward according to the spec, but let's forget about that for now...), or * if the applicable allow_origin option is true; unless the origin is not allowed, in which case it MUST not be set. [ Fixes #50 ]
2. The Vary: Origin header MUST be set in the response; unless the applicable allow_origin option is true, in which case it SHOULD not be set (as it is unnecessary). [ Fixes #42 ]

This has helped me clarify my thoughts and the above requirements will serve as guidance for me to improve this PR (or smaller PRs). Let me know if I've missed anything...

[Seldaek]

Precondition sounds good except that with the current model, if kernel.response is called we know that we have to act since it is only registered in case the path is under CORS config.

1. Sounds good to me to skip instead of sending "null", it definitely should do the job. The rest I think is already the correct behavior.
2. Makes sense as well.

Thanks for laying it down in clear terms :)

[teohhanhui]

@Seldaek I wasn't talking about implementation. The precondition is for requirements 1 and 2 so that I don't have to repeat myself :stuck_out_tongue:

with the current model, if kernel.response is called we know that we have to act since it is only registered in case the path is under CORS config.

Are you sure? That assumption is incorrect because we still provide the defaults even when the path doesn't match: https://github.com/nelmio/NelmioCorsBundle/blob/de48ff53f478fdfca308307a5cf25ea56928e7a7/Options/ConfigProvider.php#L53

I think this is undesirable behaviour.

[Seldaek]

You are right about the defaults I think, I just hope it doesn't create a BC issue where if people had no paths configured it would work before but not anymore if we remove that return $this->defaults; you linked.

So if we remove that, the kernel.request handler would return early, and the framework would probably throw a 405 because OPTIONS is not an allowed method, unless people did not configure the required request methods for a route in which case the route will be executed which I think is pretty bad as that could allow CORS-execution of a resource even without having access to it. I think in case there are no $options returned we should check if the method is OPTIONS and if so just return a 400 or 405 response straight away.

[teohhanhui]

There are further requirements regarding the pre-flight request that I left out...

I've been trying to figure out what's so special about them, but this comment seems to be spot on:

Preflight requests are designed to be compatible with regular OPTIONS requests. Just respond to all OPTIONS requests in the same way. http://stackoverflow.com/questions/32331737/how-can-i-identify-a-cors-preflight-request#comment52538208_32331737

If we follow that line of thought, we should refer to RFC7231 on the OPTIONS method, which suggests that the response might include the Allow header. But perhaps what this bundle needs to do is just respect the response set by a controller or other listeners, and only modify it to add the appropriate CORS-related headers. In case no response has been generated, we should generate one.

[teohhanhui]

Routes can be dynamic (for example, in Symfony CMF), so we can't just inject OPTIONS as allowed method via a compiler pass. Hmm...

[Seldaek]

I'd rather keep this simple really, either we ignore OPTIONS that don't match this bundle's URL, or we treat them all as invalid and return early to protect the users (with the side effect that users can't write controllers accepting OPTIONS anymore, but is this really a problem I don't know).

[teohhanhui]

we treat them all as invalid and return early to protect the users (with the side effect that users can't write controllers accepting OPTIONS anymore

Not a problem since we should be able to change CorsListener to a very low priority now.

[Seldaek]

We can't have a lower prio than the router AFAIK, but maybe this isn't true anymore with latest symfony

[Seldaek]

Closing as this does more changes than I have time to review right now.. https://github.com/nelmio/NelmioCorsBundle/pull/67 and allows you to fix the cache/vary use case at least.