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

Downgrade CorsListener priority #125

Merged
merged 2 commits into from Nov 6, 2019

Conversation

@vincentchalamon
Copy link
Contributor

vincentchalamon commented Apr 5, 2019

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

This comment has been minimized.

Copy link
Contributor Author

vincentchalamon commented Apr 6, 2019

Ping @sroze

@sroze

This comment has been minimized.

Copy link
Contributor

sroze commented Apr 6, 2019

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

This comment has been minimized.

Copy link
Contributor Author

vincentchalamon commented Apr 6, 2019

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: #115 (comment)

@nicolas-grekas

This comment has been minimized.

Copy link

nicolas-grekas commented Nov 5, 2019

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

@Seldaek Seldaek merged commit f612991 into nelmio:master Nov 6, 2019
1 check was pending
1 check was pending
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@ChristianVermeulen

This comment has been minimized.

Copy link

ChristianVermeulen commented Nov 14, 2019

@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

This comment has been minimized.

Copy link

nicolas-grekas commented Nov 14, 2019

Ask @vincentchalamon not me :)

@vincentchalamon

This comment has been minimized.

Copy link
Contributor Author

vincentchalamon commented Nov 14, 2019

@ChristianVermeulen The original issue is explained here with a lot of details: #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

This comment has been minimized.

Copy link

ChristianVermeulen commented Nov 14, 2019

@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.

@vincentchalamon

This comment has been minimized.

Copy link
Contributor Author

vincentchalamon commented Nov 14, 2019

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

This comment has been minimized.

Copy link

WebDevTmas commented Nov 14, 2019

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

@vincentchalamon

This comment has been minimized.

Copy link
Contributor Author

vincentchalamon commented Nov 14, 2019

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

@ChristianVermeulen

This comment has been minimized.

Copy link

ChristianVermeulen commented Nov 14, 2019

@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

This comment has been minimized.

Copy link

ChristianVermeulen commented Nov 14, 2019

@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

This comment has been minimized.

Copy link
Contributor Author

vincentchalamon commented Nov 14, 2019

@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

This comment has been minimized.

Copy link

ChristianVermeulen commented Nov 14, 2019

@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

This comment has been minimized.

Copy link
Contributor Author

vincentchalamon commented Nov 14, 2019

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>
  1. 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

This comment has been minimized.

Copy link

nicolas-grekas commented Nov 14, 2019

@vincentchalamon how would you recommend @ChristianVermeulen to deal with the new behavior? See his questions in #125 (comment)

@vincentchalamon

This comment has been minimized.

Copy link
Contributor Author

vincentchalamon commented Nov 14, 2019

@nicolas-grekas As moving the priority lower than RouterListener introduced an issue, I would suggest the solution explained in #125 (comment)

@vincentchalamon

This comment has been minimized.

Copy link
Contributor Author

vincentchalamon commented Nov 14, 2019

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

@Seldaek

This comment has been minimized.

Copy link
Member

Seldaek commented Nov 14, 2019

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

This comment has been minimized.

Copy link

nicolas-grekas commented Nov 14, 2019

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

@Seldaek

This comment has been minimized.

Copy link
Member

Seldaek commented Nov 14, 2019

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

This comment has been minimized.

Copy link

ChristianVermeulen commented Nov 14, 2019

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 added a commit to vincentchalamon/NelmioCorsBundle that referenced this pull request Nov 14, 2019
@vincentchalamon

This comment has been minimized.

Copy link
Contributor Author

vincentchalamon commented Nov 14, 2019

Rollback of current PR: #140

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

@dunglas

This comment has been minimized.

Copy link
Collaborator

dunglas commented Nov 14, 2019

@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

This comment has been minimized.

Copy link
Member

Seldaek commented Nov 15, 2019

@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

This comment has been minimized.

Copy link
Member

Seldaek commented Nov 15, 2019

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

This comment has been minimized.

Copy link

ChristianVermeulen commented Nov 15, 2019

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

This comment has been minimized.

Copy link
Collaborator

dunglas commented Nov 15, 2019

See api-platform/core#3265 (comment)

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

This comment has been minimized.

Copy link

teohhanhui commented Nov 15, 2019

There is no such distinction between CORS (preflight) / non-CORS OPTIONS requests: #54 (comment)

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

@Seldaek

This comment has been minimized.

Copy link
Member

Seldaek commented Nov 15, 2019

@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

This comment has been minimized.

Copy link

teohhanhui commented Nov 15, 2019

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: #125 (comment) - I don't think the suggested solution would cause any other problems.

@Seldaek

This comment has been minimized.

Copy link
Member

Seldaek commented Nov 15, 2019

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

@teohhanhui

This comment has been minimized.

Copy link

teohhanhui commented Nov 15, 2019

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 (#125 (comment)) populates the RequestContext, thus fixing the problem.

See the linked issue #115.

@Seldaek

This comment has been minimized.

Copy link
Member

Seldaek commented Nov 15, 2019

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

This comment has been minimized.

Copy link

joelwurtz commented Nov 15, 2019

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

This comment has been minimized.

Copy link
Member

Seldaek commented Nov 15, 2019

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

This comment has been minimized.

Copy link

joelwurtz commented Nov 15, 2019

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 ?)

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