Api: Repsonse objects returned by exception handlers are processed twice, losing data

[Blizzke]

I have several registered exception handlers that return a Response-object.

After that Response gets back up to Illuminate\Routing\Pipeline::handleException(), that sameexception will be attached via a call to the withException()-function.

On its way back up to the router, it passes through the other active middlewares.
If you have CORS enabled for example, the correct Accept-*-headers are added.

So then we arrive back in Dingo\Api\Routing\Router::dispatch() with a Response as intended.
Unfortunately, there it is detected that since an exception is attached it should be re-thrown.
This causes the exception handler (which already ran for this particular exception) to be executed again, returning yet another Response-object. Unfortunately this time we already lost the middleware-chain and this response does not receive its CORS headers.

Result: the frontend receives a kaputt response.

I am well aware this is an exceptional situation, yet it was created while just using the existing & provided functionality. I've solved it by returning a derivation of the Response class now that blocks the withException-functionality, but somehow there must be a better/cleaner way to prevent exceptions being fed twice through the handler.

[nschirmer]

I'm actually doing something much simpler than you, but having the same issue.

I do not have any registered exception handlers, but I am using a CORS middleware. All I'm doing, from my controllers, is either throwing an exception (that Dingo catches) or using one of Dingo's helper functions such as errorUnauthorized.

Even with such a simple setup, the response does not include any of my CORS headers. The only solution I've thought of so far is putting this inside of my middleware, above $response = $next($request);

app('Dingo\Api\Exception\Handler')->register(function (\Symfony\Component\HttpKernel\Exception\HttpException $exception) {
        // Generate and return a response, attaching CORS headers...
        // Unfortunately, this means recreating a lot of code Dingo already has in its generic handler
        // Simpler solution would be to say:
        //     $response = app('Dingo\Api\Exception\Handler')->genericResponse($exception);
        // and then attach the headers to $response... but genericResponse is a protected function.
});

Any thoughts, @jasonlewis? Thanks.

[hskrasek]

This does sound like a bug, will look into this and see what I can do to solve it.

[hskrasek]

@Blizzke and @nschirmer I just pushed up a change that should fix this. Can you install dev-master or https://github.com/dingo/api/commit/0c7e146f1dd48116e897e9387fa33c1628362578 and give it a whirl?

[JonasDoebertin]

Having the same issue with serving an API with dingo/api and barryvdh/laravel-cors. As soon as I throw an exception or an error response from my controllers, the CORS headers are missing from the response that get's actually send in the end.

Since the change in 0c7e146f1dd48116e897e9387fa33c1628362578 was reverted, is there any other way of getting this worked out or is there a fix in sight?

@nschirmer How exactly did you solve this if I may ask?

[nschirmer]

Sorry @hskrasek -- I haven't had a chance to test what you pushed up.

@JonasDoebertin -- My current fix is basically as follows...

So the Cors middleware I'm using would normally just do this (before putting in the Exception handler fix):

// Note that all of the following lines are wrapped in another if that actually checks
// if we're dealing with a CORS request from another domain ... if we're not, then 
// it just returns $next($request) and doesn't add any CORS headers

if($request->isMethod('options')) {
    $response = response()->json(['options' => true], 200);
} else {
    // Fix code below is going to go HERE
    $response = $next($request);
}

return $response
    ->header('Access-Control-Allow-Methods', 'POST, GET, OPTIONS, PUT, DELETE')
    ->header('Access-Control-Allow-Headers', 'Origin, X-Requested-With, Content-Type, Accept, Authorization')
    ->header('Access-Control-Allow-Origin', $origin);

My fix is actually pretty messy, since I sort of rushed it until a real fix was available. Basically, I have this within the else block above (before setting $response variable).

    app('Dingo\Api\Exception\Handler')->register(function (\Symfony\Component\HttpKernel\Exception\HttpException $exception) use($origin) {
        $statusCode = $exception->getStatusCode();

        if (! $message = $exception->getMessage()) {
            $message = sprintf('%d %s', $statusCode, \Illuminate\Http\Response::$statusTexts[$statusCode]);
        }

        $data = [
            'message' => $message,
            'status_code' => $statusCode,
        ];

        if ($exception instanceof \Dingo\Api\Contract\Debug\MessageBagErrors && $exception->hasErrors()) {
            $data['errors'] = $exception->getErrors();
        }

        if ($code = $exception->getCode()) {
            $data['code'] = $code;
        }

        $headers = [
            'Access-Control-Allow-Methods' => 'POST, GET, OPTIONS, PUT, DELETE',
            'Access-Control-Allow-Headers' => 'Origin, X-Requested-With, Content-Type, Accept, Authorization',
            'Access-Control-Allow-Origin' => $origin
        ];

        $response = new \Illuminate\Http\Response($data, $statusCode, $headers);
        return $response;
    });

This binds to the exception handler, catching them, and then formatting the output myself... and also binding the CORS headers.

Hope this helps, sorry for the mess. If you haven't yet, maybe give dev-master a try to see if the fix @hskrasek put in works for you.

[JonasDoebertin]

Unfortunately, the fix @hskrasek did in 0c7e146f1dd48116e897e9387fa33c1628362578 was reverted a little later in 5fbfb5d590ed4e65dcacd30b68c21c27e5066dca since it seemed to have raised other issues.

[JonasDoebertin]

Thanks so much for your help, I'll see if I can get it sorted out this way until there's a real fix.

[Blizzke]

Don't really get the reasoning behind the re-throwing of that exception. I mean, it gets attached to the response by the exception handler after all. That means it was thrown before already...

The simplest thing I can think of to circumvent this is to add a Middleware that just sees if the returned response has a withException() function and if so, nulls out the exception property. It is public after all.

[JonasDoebertin]

From a first very quick test, the solution @Blizzke suggested seems to be working.

I added a new global middleware:

class FixCors
{
    /**
     * Handle an incoming request.
     *
     * @param  \Illuminate\Http\Request  $request
     * @param  \Closure  $next
     * @return mixed
     */
    public function handle($request, Closure $next)
    {
        $response = $next($request);

        if (method_exists($response, 'withException')) {
            $response->exception = null;
        }

        return $response;
    }
}

[hskrasek]

@Blizzke The answer is simply, Laravel 5.1. Laravel 5.1 sends the request through a very generic pipeline that is missing the logic to catch exceptions during each slice and properly handle it, which prevents middleware from being able to operate on an error response. Meanwhile, 5.2 and 5.3 have a Routing specific pipeline that catches exceptions on each slice. Response goes through all the middleware, everything is great, and then the dingo router re-does everything.

The fun in supporting 3 different versions of laravel and possibly soon 3 versions of lumen. Still trying to think of a way that this can be solved on our end, but for the time being if missing CORS headers are the only things getting mangled, I'd suggest using an approach outlined here. The second example, not the first. You may not be using that particular package for CORS, but the idea is the same.

[skyrpex]

he fun in supporting 3 different versions of laravel

But why should the same version of Dingo support different Laravel versions?

I'd suggest using an approach outlined here

It doesn't work for me in Laravel 5.2 because the render method of the exception handler is never called.

[colindecarlo]

I ran into this not long ago on a Laravel 5.3 project. I've 'fixed' the problem by transferring headers from the original response onto the new exception sourced response. Makes me sad, but just maybe an actual solution swimming around in there.

            $originalHeaders = $response->headers;
            $response = $this->exception->handle($exception);

            collect($originalHeaders->keys())
                ->reject(function ($headerName) use ($response) {
                    return $response->headers->has($headerName);
                })
                ->each(function ($headerName) use ($originalHeaders, $response) {
                    $response->headers->set($headerName, $originalHeaders->get($headerName));
                });

I crammed it all in a service provider which is included after the Dingo LaravelServiceProvider

<?php

namespace App\Providers;

use Exception;
use Dingo\Api\Http\Request;
use Dingo\Api\Routing\Router;
use Dingo\Api\Contract\Routing\Adapter;
use Dingo\Api\Provider\ServiceProvider;
use Dingo\Api\Contract\Debug\ExceptionHandler;

class DingoRouterWorkAroundProvider extends ServiceProvider
{
    public function boot()
    {
    }

    public function register()
    {
        $this->app->singleton('api.router', function ($app) {
            $router = new WorkAroundRouter(
                $app[Adapter::class],
                $app[ExceptionHandler::class],
                $app,
                $this->config('domain'),
                $this->config('prefix')
            );

            $router->setConditionalRequest($this->config('conditionalRequest'));

            return $router;
        });
    }
}

class WorkAroundRouter extends Router
{
    public function dispatch(Request $request)
    {
        $this->currentRoute = null;

        $this->container->instance(Request::class, $request);

        $this->routesDispatched++;

        try {
            $response = $this->adapter->dispatch($request, $request->version());

            if (property_exists($response, 'exception') && $response->exception instanceof Exception) {
                throw $response->exception;
            }
        } catch (Exception $exception) {
            if ($request instanceof InternalRequest) {
                throw $exception;
            }

            $this->exception->report($exception);

            // This is where things go squirrely
            // The response which goes through all the middleware is tossed
            // away and a new response is created based on the exception
            // this causes any additional headers which may have existed on the
            // original response to be lost
            //
            // soooooo ....
            // I'm taking the headers that existed on the original response which
            // don't exist on the new response and adding them.
            $originalHeaders = $response->headers;
            $response = $this->exception->handle($exception);

            collect($originalHeaders->keys())
                ->reject(function ($headerName) use ($response) {
                    return $response->headers->has($headerName);
                })
                ->each(function ($headerName) use ($originalHeaders, $response) {
                    $response->headers->set($headerName, $originalHeaders->get($headerName));
                });
        }

        return $this->prepareResponse($response, $request, $request->format());
    }
}

[skys215]

In my case, CORS header was removed when throwing Exception from my own controller.
After debugging 2 days with no efforts and debugging one day with xdebug, I found where the bug was.

The problem is where the author pointed to us on this commit: 0c7e146.
The new Response was made to make a generic error response.
At here, you can see that it's trying to add headers from exception.

return new Response($response, $this->getStatusCode($exception), $this->getHeaders($exception));

So the headers wouldn't be reset, but why can't we get the headers we set?
Let's keep analyzing it.
The headers is set here(from Symfony):

class HttpException extends \RuntimeException implements HttpExceptionInterface
{
    private $headers;
    public function __construct($statusCode, $message = null, \Exception $previous = null, array $headers = array(), $code = 0)
    {
        $this->statusCode = $statusCode;
        $this->headers = $headers;
        parent::__construct($message, $code, $previous);
    }
...

When we called it? Here:

/**
     * Return an error response.
     *
     * @param string $message
     * @param int    $statusCode
     *
     * @throws \Symfony\Component\HttpKernel\Exception\HttpException
     *
     * @return void
     */
    public function error($message, $statusCode)
    {
        throw new HttpException($statusCode, $message);
    }
...
    /**
     * Return a 400 bad request error.
     *
     * @param string $message
     *
     * @throws \Symfony\Component\HttpKernel\Exception\HttpException
     *
     * @return void
     */
    public function errorBadRequest($message = 'Bad Request')
    {
        $this->error($message, 400);
    }
....

In my case, I was calling errorBadRequest in the controller.

        try{
                .....
        } catch (\Exception $e) {
            $this->response->errorBadRequest($e->getMessage());
        }

Now we can know that the headers must be set in the controller in order to set the header when creating the new Response for generic response. But actually, we can't get response header here in the controller.
The only place that can get original headers is here.

        try {
            $response = $this->adapter->dispatch($request, $request->version());
            if (property_exists($response, 'exception') && $response->exception instanceof Exception) {
                throw $response->exception;
            }
        } catch (Exception $exception) {
            if ($request instanceof InternalRequest) {
                throw $exception;
            }
            $this->exception->report($exception);
            $response = $this->exception->handle($exception);
        }

Setting the header to new response here is not possible due to no setHeaders method on Response class and the headers property is private.
Also, handle method is a registered handler in DingoServiceProvider, I'm not sure we can change the signature of handle method or not to pass the header with getHeaders method as second parameter.
After all, I think that instead of creating the new response object, changing the content of the response is the better solution.
ping @hskrasek