tuupola / cors-middleware

PSR-7 and PSR-15 CORS middleware
MIT License
132 stars 16 forks source link

Exception causes CORS Error #2

Open ghost opened 8 years ago

ghost commented 8 years ago

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 commented 8 years ago

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

ghost commented 8 years ago

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 commented 7 years ago

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

edsonmedina commented 7 years ago

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 commented 7 years ago

@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 commented 6 years ago

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 commented 6 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

@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 commented 4 years ago

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