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

Exception causes CORS Error #2

Open
ghost opened this issue Jul 11, 2016 · 15 comments
Open

Exception causes CORS Error #2

ghost opened this issue Jul 11, 2016 · 15 comments

Comments

@ghost
Copy link

ghost commented Jul 11, 2016

Whenever an exception is thrown by any of my classes, response loses cors headers. Setting CORS headers manualy within the slim 'errorHandler', solves the issue, but does not feel right.

@tuupola
Copy link
Owner

tuupola commented Aug 12, 2016

Middleware can only set headers if it is called. The error in your classes most likely causes middleware stack not to run.

@ghost
Copy link
Author

ghost commented Aug 12, 2016

The exception is thrown after cors middleware has been invoked. Somehow response object falls back to its initial state which leads to the cors error.

@edsonmedina
Copy link

edsonmedina commented May 9, 2017

Same issue here. Exceptions handled by errorHandler are losing CORS headers.

@edsonmedina
Copy link

Apparently it's a slim issue (App.php method process()):

        // Traverse middleware stack
        try {
            $response = $this->callMiddlewareStack($request, $response);
        } catch (Exception $e) {
            $response = $this->handleException($e, $request, $response);
        } catch (Throwable $e) {
            $response = $this->handlePhpError($e, $request, $response);
        }

It does either one or the other (handle errors or middleware), not both.

@tuupola
Copy link
Owner

tuupola commented May 10, 2017

@edsonmedina You are correct. Currently only way I know how to handle this is to set the CORS headers again in the error handler. This is Slim issue, not middleware issue.

@neomerx
Copy link

neomerx commented Oct 30, 2017

A typical solution for this issue would be saving CORS headers before handling next middleware/Controller. When an unexpected exception occurs it will be handled by an Exception Handler. This handler has to take the saved headers and add them to response.

So it's a responsibility of 2 parties: CORS middleware (to save the headers) and exception handler (to use them).

@neomerx
Copy link

neomerx commented Oct 30, 2017

Also, there is another error handling strategy: final handler (controller) should not throw any exceptions (it just catches them all) so middleware can expect to always have a chance to add the headers. If that's the case then no 'special' storage for CORS headers is needed and the exception handler is not needed to know about CORS existence.

@marc-mabe
Copy link

marc-mabe commented Feb 22, 2019

This is for sure not an issue of this middleware.
I had the same issue with ZF expressive as the error handler middleware is the first one and catches all exceptions happened before = means the CORS middleware does not respond anything as a thrown exception stops executing the current program flow until it gets catched.

As a solution you need to handle exceptions twice and the middleware pipeline looks like this:

  1. first exception handler (needs to be the first one to handle really all exceptions)
  2. required middlewars for CORS like routing
  3. CORS middleware
  4. second exception handler
  5. all the rest

Now your CORS middleware can act an most exception based error responses except the once hapend before the CORS middleware

@tuupola
Copy link
Owner

tuupola commented Feb 22, 2019

As a sidenote. I know some people use exceptions for program flow, for example to return a 404 response. I do not see a 404 as an exceptional situation and tend to use special NotFoundResponse classes instead. This also works as a solution to above described problem with middlewares and exceptions.

@neomerx
Copy link

neomerx commented Feb 22, 2019

@tuupola A developer will have to deal with exceptions in this case or another anyway.

IMHO, a controller should handle all errors from itself and below and returns only responses (2XX, 4XX or 5XX). Otherwise, the controller must know about cors middleware and its needs to add headers, which is odd.

@marcguyer
Copy link

If a response is generated before the CORS middleware is invoked, the headers are added to the response. When an exception is thrown in a middleware stack, it's possible that the exception breaks out of the stack, depending on your setup. As long as you can catch that exception and generate a response before the CORS middleware is invoked, you're all set. So, generally speaking, you can include an exception handler middleware that generates a response before the CORS middleware in the stack. Here's a basic example pipeline:

  1. exception handler (most outer middleware to handle any exception still unhandled)
  2. routing
  3. CORS
  4. exception handler (handle any exceptions from "upstream")
  5. request hander

In a normal request without any exception, the response is generated by (5), (4) is bypassed, (3) adds relevant CORS headers to the response and ultimately the response is rendered.

If an exception is thrown in (5), it's handled by (4) which generates a response based on the exception, then (3) adds the headers.

@jseparovic1
Copy link

@marcguyer Could we just pipe CorsMiddleware before ProblemDetailsMiddleware or in your example 4. before 3.

This is example from Zend Expressive project.

$app->pipe(ErrorHandler::class);
$app->pipe(RequestIdMiddleware::class);
$app->pipe(ServerUrlMiddleware::class);
$app->pipe(CorsMiddleware::class);

// Always use Problem Details format for calls to the API.
$app->pipe('/api', ProblemDetailsMiddleware::class);

$app->pipe(RouteMiddleware::class);
...

@marcguyer
Copy link

Yes since ProblemDetailsMiddleware behaves like an exception handler. As long as the response is already produced before it's passed back through CorsMiddleware then it can do it's work on that response.

@jseparovic1
Copy link

@marcguyer I just wanted to confirm that there are no any drawbacks for this approach since I'm using it.
It might also be use-full for others since this is quite common problem when building API-s and solution is simple.

@babarinde
Copy link

Hi,
Thanks @tuupola for the cool implementation of CORS,
I think it will be a great idea to have this expressive (mezzio) issues taken care of in the docs as it will save a lot of people stress. i.e.

install mezzio-problem-details with:
composer require mezzio/mezzio-problem-details
and add
$app->pipe(ProblemDetailsMiddleware::class);
to the pipeline after
$app->pipe(CorsMiddleware::class);

Thanks @marcguyer and @jseparovic1 and everyone

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants