Downgrade CorsListener priority

[vincentchalamon]

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	yes
Deprecations?	no
Fixed tickets	#115

[vincentchalamon]

Ping @sroze

[sroze]

Sounds reasonable not to have it as 250. Though, that change is a massive BC-break. What's the reasoning behind 31 as a value?

[vincentchalamon]

That's totally a BC break and will require a major version tag. 31 is just under the RouterListener, you can check about the services listening to the kernel.request event ordered by priority here: https://github.com/nelmio/NelmioCorsBundle/issues/115#issuecomment-458269534

[nicolas-grekas]

What about 28, to be between the router listener and the next ones?

[ChristianVermeulen]

@nicolas-grekas I'm wondering why you proposed to put it behind the routerlistener. This causes nelmio to no longer handle options requests in our system because it is now handled by the routerlistener. This would mean we'd have to include the OPTIOS method in every route config (see issue #52).

I have set the priority to 38 to be just above the routerlistener and this fixes things. Are we doing something wrong or should this priority perhaps be revised?

[nicolas-grekas]

Ask @vincentchalamon not me :)

[vincentchalamon]

@ChristianVermeulen The original issue is explained here with a lot of details: https://github.com/nelmio/NelmioCorsBundle/issues/115

The bug was revealed by ApiPlatform which generates an absolute url on kernel.response event dispatched by the CorsListener on OPTIONS requests (for the preflight). But as the router weren't initialized yet, the url looked like http://localhost/foo instead of https://api.example.com/foo.

2 solutions were proposed: changing the service priority, or injecting the router.request_context in the CorsListener. The first one has been prefered.

By changing the priority to 38 in your tests, I'm not sure it fixed the original issue. Could you confirm it by generating an absolute url in a kernel.response event on OPTIONS requests?:

dump($this->router->generate('foo', [], Router::ABSOLUTE_URL));

If the original bug is still present, maybe the solution would be to rollback the priority to 250 and inject the router.request_context service in the CorsListener, as explained in the issue.

I'm gonna give it a try later by the day.

[ChristianVermeulen]

@vincent-chapron I'm not familiar with the api-framework repository, perhaps you could explain why an options-request needs to generate and return an absolute URL in the first place?

As far as I know an OPTIONS request is nothing more but a request to check which methods are allowed and if the request is allowed from the given origin and should not contain any body at all. The preflight request is meant to be very fast. Now it is being slowed down by e.g. the SessionListener and the LocaleListener. Which both are not needed to return the allowed methods.

• https://developer.mozilla.org/en-US/docs/Glossary/Preflight_request
• https://stackoverflow.com/questions/15381105/cors-what-is-the-motivation-behind-introducing-preflight-requests

[vincentchalamon]

In ApiPlatform, we need to add a _link response header with an absolute url. So, the preflight-response doesn't have any body, and the client can get this url even from an OPTIONS request.

[WebDevTmas]

Sounds like an issue with the API platform not the cors bundle.

[vincentchalamon]

@WebDevTmas This issue could happen to any Symfony listener on kernel.response which wants to generate an url on an OPTIONS request.

[ChristianVermeulen]

@vincent-chapron an options request should never be returning anything else besides the allowed methods... So basically you are "hacking" the options request to do something it is not meant to do. Therefor I think this should indeed be fixed in api-platform with a dedicated listener or something but not in Nelmio...

[ChristianVermeulen]

@Seldaek perhaps you can pitch in here as well? Wondering how you look at this... Right now we can not upgrade to v2.0.0 because of this.

[vincentchalamon]

@ChristianVermeulen According to the OPTIONS specification (RFC 7231):

A server generating a successful response to OPTIONS SHOULD send any header fields that might indicate optional features implemented by the server

In ApiPlatform, we're adding a _link header containing the url of the API documentation. As it's allowed by the spec, we're not hacking the OPTIONS request.

[ChristianVermeulen]

@vincentchalamon I see what you mean, so it is indeed not against spec to do so, my bad. I really thought the preflight was stricter then that.

How do you handle other preflight requests? Do you add the OPTIONS request to every path you have configured now? We simply can not call any OPTIONS requests any more at all now without explicitly adding this method to our endpoints because the RouterListener already picks it up before cors. (Hence issue #52).

I still think your problem should be solved with a dedicated listener in the api-framework specifically for your use case with a priority higher then CorsListener.

[vincentchalamon]

IMO the problem is still present in this bundle, as it could be reproduced on any Symfony project.

I don't have any sandbox-project to reproduce this bug, so if you have one, could you try the following solution?

1. Rollback the CorsListener priority to 250 and inject the router.request_context as last argument optionally:

   <!-- services.xml:L14-19-->
   <service id="nelmio_cors.cors_listener" class="%nelmio_cors.cors_listener.class%">
   <argument type="service" id="event_dispatcher" />
   <argument type="service" id="nelmio_cors.options_resolver" />
   <argument type="service" id="router.request_context" on-invalid="null" />
   <tag name="kernel.event_listener" event="kernel.request" method="onKernelRequest" priority="250" />
   </service>

2. Inject the context as argument on CorsListener:

   
   # CorsListener.php:L42-47
   /** @var RequestContext */
   private $context;

public function __construct(ResolverInterface $configurationResolver, RequestContext $context = null) { $this->configurationResolver = $configurationResolver; }

CorsListener.php:L72-76

if ('OPTIONS' === $request->getMethod()) { if ($this->context) { $this->context->fromRequest($request); } $event->setResponse($this->getPreflightResponse($request, $options));

return;

}

[nicolas-grekas]

@vincentchalamon how would you recommend @ChristianVermeulen to deal with the new behavior? See his questions in https://github.com/nelmio/NelmioCorsBundle/pull/125#issuecomment-553841597

[vincentchalamon]

@nicolas-grekas As moving the priority lower than RouterListener introduced an issue, I would suggest the solution explained in https://github.com/nelmio/NelmioCorsBundle/pull/125#issuecomment-553848344

[vincentchalamon]

Creating a dedicated listener on ApiPlatform only fixes the original issue on ApiPlatform, but not on any Symfony project with the same behavior.

[Seldaek]

IMO the reason it was a high priority before is exactly to prevent the problem @ChristianVermeulen describes, if you restricts the allowed methods on your controllers then you need to add OPTIONS everywhere otherwise the CorsListener doesn't have a chance to trigger.

If ApiPlatform needs a custom priority then maybe that's rather the special case which should do something different instead of breaking the bundle for everyone else?

So I would suggest rolling back to 250 so we bypass routing and session too as sessions aren't needed to serve CORS OPTIONS requests it's just overhead.

Then the question is what do we do to fix this in ApiPlatform, do we need to add a parameter in the bundle maybe that can be used by ApiPlatform to configure things correctly? It sounds to me like the problem will still persist in a project having ApiPlatform and non-ApiPlatform routes though, those routes will not be able to use CORS unless they allow OPTIONS requests I suppose?

So maybe ApiPlatform should run the router to figure out which links to add in a kernel.response event if the request has no _route? How about that? That sounds to me like it'd allow you to serve your _link headers without breaking anything else.

[nicolas-grekas]

Can't we split one listener in two, one callback being called earlier and the other later? Either CorsListener or RouterListener?

[Seldaek]

CORS needs to happen fully before the routing for OPTIONS requests, as it needs to return a response without hitting the controller, and/or the router will throw a Method Not Allowed.

Then ApiPlatform it seems needs the router to have happened so they know what to add on the response. Those are kinda fundamentally incompatible requirements.

[ChristianVermeulen]

Then ApiPlatform it seems needs the router to have happened so they know what to add on the response. Those are kinda fundamentally incompatible requirements.

That was kind of my thought all along. Though allowed according to the RFC, it seems very unconventional to add custom headers to an OPTIONS-response. Hence my personal feeling this should not be a responsibility of Nelmio at all.

A possible solution for ApiFramework might be to have the router generate a relative path instead and concat it to the full host which can be retrieved by injecting something like the request-stack perhaps?

[vincentchalamon]

Rollback of current PR: https://github.com/nelmio/NelmioCorsBundle/pull/140

I'll open an issue on ApiPlatform to fix the original issue

[dunglas]

@ChristianVermeulen @Seldaek the OPTIONS verb is not limited to CORS preflight requests. It's just one usage among others. For instance, the RFC defining the PATCH method states that OPTIONS should be used to retrieve the list of supported patch formats for a given resource.

This specification introduces a new response header Accept-Patch used to specify the patch document formats accepted by the server. Accept-Patch SHOULD appear in the OPTIONS response for any resource that supports the use of the PATCH method. The presence of the Accept-Patch header in response to any method is an implicit indication that PATCH is allowed on the resource identified by the Request-URI. The presence of a specific patch document format in this header indicates that that specific format is allowed on the resource identified by the Request-URI.

https://tools.ietf.org/html/rfc5789#section-3

Regarding API Platform, setting Link headers hinting the URLs of the Hydra doc (according to the W3C spec), or the one of the Mercure hub should be done for OPTIONS too. But I'm not even sure that will still face the original issue that was reported by @vincentchalamon because API Platform now delegates to the Symfony WebLink listener to generate these headers, and this listener is maybe not called anymore when NelmioCors is triggered (to be tested).

If we still face the issue, setting the Link headers for OPTIONS request (while valid, and fitting the original intent of this method) is probably useless for most users. Maybe could we just not set the headers if the method is OPTIONS, instead of trying to instantiate the router context "manually" etc.

That being said, if this problem hits API Platform, it may also hit other projects. In this case, maybe is it worth it to find a more generic solution. Also, having to implement specific logic because of NelmioCors (such as preventing to add Link headers for the OPTIONS method) in libs that shouldn't even be aware of its existence (in term of technical responsibilities) feels like a design issue to me.

[Seldaek]

@dunglas of course OPTIONS is legal outside of the CORS context and if the bundle interferes with non-CORS OPTIONS requests we should fix that.

[Seldaek]

Release 2.0.1 which rolls back the priority, thanks everyone for the discussion here.

I am on holidays atm so no time to dig in further but if someone has time to verify that regular OPTIONS requests are not intercepted (ideally in both 1.x and 2.x versions of the bundle) that'd be great.

[ChristianVermeulen]

if the bundle interferes with non-CORS OPTIONS requests we should fix that.

There is this in the CORS-listener:

        // skip if not a CORS request
        if (!$request->headers->has('Origin') || $request->headers->get('Origin') == $request->getSchemeAndHttpHost()) {
            return;
        }

So it should already disregard anything else. So I think ApiFramework is actually working with a CORS OPTIONS (preflight) request.

I also found release 1.5.6: https://github.com/nelmio/NelmioCorsBundle/releases/tag/1.5.6

Fixed preflight request handler hijacking regular non-CORS OPTIONS requests.

So I guess it should already be leaving regular OPTIONS requests alone?

Perhaps a solution could be to use compiler pass to load tagged services which can be called in the CorsListener to extend functionality when needed? Though I admit it is a quite big implementation.

[dunglas]

See https://github.com/api-platform/core/pull/3265#issuecomment-554304314

So actually having a special behavior for Preflight requests such as skipping other headers is a bad idea too and could lead to cache issues... It's a hard one 😄

[teohhanhui]

There is no such distinction between CORS (preflight) / non-CORS OPTIONS requests: https://github.com/nelmio/NelmioCorsBundle/pull/54#issuecomment-162838133

Any distinction we try to make is arbitrary and contrary to the spec.

[Seldaek]

@teohhanhui sure, but I don't see how to achieve what you described there.. How can we only generate a response if nothing else generated a response, as we can't really wait for everything else to be processed, unless we hook in kernel.exception and kernel.response and process CORS there by replacing routing errors if they happened or adding to existing response if there is one?

[teohhanhui]

Generating a response is not the problem. The problem is that doing so bypasses RouterListener and thus breaks the RequestContext. The solution is to un-break it: https://github.com/nelmio/NelmioCorsBundle/pull/125#issuecomment-553848344 - I don't think the suggested solution would cause any other problems.

[Seldaek]

Sorry but I don't understand how this solves anything. I am probably missing something.

[teohhanhui]

Sorry but I don't understand how this solves anything.

Stopping the kernel.request event propagation is unavoidable, and not a problem on its own. However, because CorsListener has higher priority than RouterListener (which is necessary, as you've explained above), the RequestContext is never populated with the correct values. The suggested solution (https://github.com/nelmio/NelmioCorsBundle/pull/125#issuecomment-553848344) populates the RequestContext, thus fixing the problem.

See the linked issue #115.

[Seldaek]

Ok thanks for explaining it again now this makes sense I agree that seems like a good solution although I am not sure if filling populating the request context will be enough to keep ApiPlatform working but I assume so from what @vincentchalamon wrote. If anyone feels like doing a PR that'd be great.

[joelwurtz]

Maybe the "best" solution would be to not use a listener and instead use a controller with custom routes added with the config of nelmio ? (So no more method not allowed / and we still have a request context for other third party lib)

[Seldaek]

As I understand it that means we'd hijack any valid OPTIONS route though, as we can not define a route that requires a given header to be present. So it still comes with a trade-off.

[joelwurtz]

It think this is possible by using the condition with a custom expression cf https://symfony.com/doc/current/routing.html#matching-expressions (but maybe there is a cleaner solution by providing a specific request matcher ?)