thephpleague / route

Fast PSR-7 based routing and dispatch component including PSR-15 middleware, built on top of FastRoute.
http://route.thephpleague.com
MIT License
651 stars 126 forks source link

CORS - routes not found getting 405 method not allowed instead? #257

Closed lautiamkok closed 4 years ago

lautiamkok commented 5 years ago

How can we implement CORS correctly with the router? Can't find any guide from the documentation. So I imitate Slim's as follows:

    use Psr\Http\Message\ResponseInterface;
    use Psr\Http\Message\ServerRequestInterface;
    use function Zend\Stratigility\middleware;

    $router->map('OPTIONS', '/{routes:.+}', function (ServerRequestInterface $request) : ResponseInterface {
        $response = new Zend\Diactoros\Response;
        return $response;
    });

    $router->middleware(middleware(function ($request, $handler) {
        $response = $handler->handle($request);
        return $response
            ->withHeader('Access-Control-Allow-Origin', 'http://mysite')
            ->withHeader('Access-Control-Allow-Headers', 'X-Requested-With, Content-Type, Accept, Origin, Authorization')
            ->withHeader('Access-Control-Allow-Methods', 'GET, POST, PUT, DELETE, PATCH, OPTIONS');
    }));

But I get 405 method not allowed for the routes that are supposed to be 404 not found, for example:

    http://myapi.com/xxx
    http://myapi.com/zzz/xxx

These routes are not set so they should be returnng 404 but I get 405 instead. It works as it is expected if I remove $router->map('OPTIONS',...) from the route.

Any ideas?

In Slim's guide, we just have to add this to the last route:

$app->map(['GET', 'POST', 'PUT', 'DELETE', 'PATCH'], '/{routes:.+}', function ($request, $response) {
    throw new HttpNotFoundException($request);
});

But the League's router does not allow array in the first param in the router. So if we use do this in League:

$router->map(['GET', 'POST', 'PUT', 'DELETE', 'PATCH'], ....)

We will get an error for that.

Any ideas how we should do this properly?

philipobenito commented 4 years ago

Added this to my list for the release I'm planning for early January

philipobenito commented 4 years ago

Been looking at this for the latest release. The behaviour is technically correct, and actually coming from FastRoute.

You're defining a catch-all route, even though it's OPTIONS, the router is matching a route but returning 405 because the incoming request method isn't OPTIONS.

OPTIONS routes ideally should be set per route, but I'm working on a solution to make that a bit more automatic.

That being said, if you really do want to do it this way, you can, just not with an array.

$notFoundCallable = function ($request) {
    throw new NotFoundException();
};

$router->get('/{routes:.+}', $notFoundCallable);
$router->post('/{routes:.+}', $notFoundCallable);
// etc...

I think these routes would need to be registered after any literal routes but you'd need to test.

TCB13 commented 4 years ago

@philipobenito is it possible to query the router inside a middleware to find out registered http verbs for some path passed in the request?

philipobenito commented 4 years ago

That functionality doesn’t actually exist so although there are ways to get access to the Router in a middleware, there isn’t an easy way to get all the verbs.

I’ve implemented auto options routes in v5 that will respond with basic required headers with the ability to customise the handler for the OPTIONS routes.

TCB13 commented 4 years ago

That functionality doesn’t actually exist so although there are ways to get access to the Router in a middleware, there isn’t an easy way to get all the verbs.

I’ve implemented auto options routes in v5 that will respond with basic required headers with the ability to customise the handler for the OPTIONS routes.

Interesting, my idea was to build something like:

$router->map("OPTIONS", '/{routes:.+}', function(ResponseInterface $response) { return $response; })->middleware(new CorsRequest());
$router->get("/test", ... );
$router->post("/test", ... );

Then inside the CorsRequest middleware I would:

function process(.....) {

// Find allowed verbs/methods for the requested router and add them to the response header
$verbs = $router->findVerbsForRequest($request->getPath());
$response = $response->withHeader("Access-Control-Allow-Methods", implode(", ", $verbs));

// Send the request headers back to the client as allowed (avoids pre-flight fail) + some extras
$headers  = array_merge(array_keys(getallheaders()), self::$alwaysAllowedHeaders);
$response = $response->withHeader("Access-Control-Allow-Headers", strtolower(implode(", ", $headers)));

return $response;
}

I've done this in the past in Slim, it worked fine.

philipobenito commented 4 years ago

Yeah, v5 just does that for you for any explicitly defined routes.

Take a look at the open PR for the new version if you like, aiming to release in the next few days.

Sent from ProtonMail Mobile

On Tue, May 19, 2020 at 19:46, Tadeu Bento notifications@github.com wrote:

That functionality doesn’t actually exist so although there are ways to get access to the Router in a middleware, there isn’t an easy way to get all the verbs.

I’ve implemented auto options routes in v5 that will respond with basic required headers with the ability to customise the handler for the OPTIONS routes.

Interesting, my idea was to build something like:

$router->map("OPTIONS", '/{routes:.+}', function(ResponseInterface $response) { return $response; })->middleware(new CorsRequest()); $router->get("/test", ... ); $router->post("/test", ... );

Then inside the CorsRequest middleware I would:

function process(.....) {

// Find allowed verbs/methods for the requested router and add them to the response header $verbs = $router->findVerbsForRequest($request->getPath()); $response = $response->withHeader("Access-Control-Allow-Methods", implode(", ", $verbs));

// Send the request headers back to the client as allowed (avoids pre-flight fail) + some extras $headers = array_merge(array_keys(getallheaders()), self::$alwaysAllowedHeaders); $response = $response->withHeader("Access-Control-Allow-Headers", strtolower(implode(", ", $headers)));

return $response; }

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

TCB13 commented 4 years ago

Yeah, v5 just does that for you for any explicitly defined routes

Thank you very much :)

I was checking out the PR and it indeed adds the HTTP Verbs at getOptionsCallable(), unfortunately it doesn't seem to have a way to deal with Access-Control-Allow-Credentials or Access-Control-Allow-Headers headers also required for CORS in some situations...

the cross-domain server can permit reading of the response when credentials are passed to it by setting the CORS Access-Control-Allow-Credentials header to true.

Under certain circumstances, when a cross-domain request includes a non-standard HTTP method or headers, the cross-origin request is preceded by a request using the OPTIONS method, and the CORS protocol necessitates an initial check on what methods and headers are permitted prior to allowing the cross-origin request.

I'm not sure how this last part can be implemented because anyone can have their own custom headers and needs them in the CORS request. Maybe I'll just override your generic strategy to deal with this.

philipobenito commented 4 years ago

Yeah that’s exactly the intent, to provide the basics and to use a custom strategy for what you specifically need

Sent from ProtonMail Mobile

On Tue, May 19, 2020 at 20:07, Tadeu Bento notifications@github.com wrote:

Yeah, v5 just does that for you for any explicitly defined routes

I was checking out the PR and it indeed adds the HTTP Verbs at getOptionsCallable(), unfortunately it doesn't seem to have a way to deal with Access-Control-Allow-Credentials or Access-Control-Allow-Headers headers also required for CORS in some situations...

the cross-domain server can permit reading of the response when credentials are passed to it by setting the CORS Access-Control-Allow-Credentials header to true.

Under certain circumstances, when a cross-domain request includes a non-standard HTTP method or headers, the cross-origin request is preceded by a request using the OPTIONS method, and the CORS protocol necessitates an initial check on what methods and headers are permitted prior to allowing the cross-origin request.

I'm not sure how this last part could be implemented because anyone can have their own custom headers and needs them in the CORS request. Maybe I'll just override your generic strategy to deal with this.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

philipobenito commented 4 years ago

Also keep in mind that a lot of the CORS headers can be set on every response, so in those cases you can apply a global middleware, that will be applied to every response, including the one from the options handler, that handler only needs to be concerned with setting headers that only apply to specific routes.

Sent from ProtonMail Mobile

On Tue, May 19, 2020 at 20:07, Tadeu Bento notifications@github.com wrote:

Yeah, v5 just does that for you for any explicitly defined routes

I was checking out the PR and it indeed adds the HTTP Verbs at getOptionsCallable(), unfortunately it doesn't seem to have a way to deal with Access-Control-Allow-Credentials or Access-Control-Allow-Headers headers also required for CORS in some situations...

the cross-domain server can permit reading of the response when credentials are passed to it by setting the CORS Access-Control-Allow-Credentials header to true.

Under certain circumstances, when a cross-domain request includes a non-standard HTTP method or headers, the cross-origin request is preceded by a request using the OPTIONS method, and the CORS protocol necessitates an initial check on what methods and headers are permitted prior to allowing the cross-origin request.

I'm not sure how this last part could be implemented because anyone can have their own custom headers and needs them in the CORS request. Maybe I'll just override your generic strategy to deal with this.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.