cakephp / app

CakePHP application template
366 stars 390 forks source link

CSRF Token missing in error template #886

Closed calcosta closed 2 years ago

calcosta commented 2 years ago

Description

When CakePHP renders an error page (debug = false), e.g. 404 due to Missing Controller or a 500 due to an Internal error the Form Helper does not output the _csrfToken input field. However it does output the_Token[fields]and _Token[unlocked] field.

I noticed, that the _csrfToken input field is missing only when in Application.php the CSRF protection middleware is added to the queue after the ErrorHandlerMiddleware is added (like it is suggested in the cakephp/app repository).

Example:

_csrfToken input field missing:

public function middleware(MiddlewareQueue $middlewareQueue): MiddlewareQueue
    {
        $middlewareQueue
            ->add(new ErrorHandlerMiddleware(Configure::read('Error')))

            ->add(new AssetMiddleware([
                'cacheTime' => Configure::read('Asset.cacheTime'),
            ]))

            ->add(new RoutingMiddleware($this))

            ->add(new BodyParserMiddleware())

            ->add(new CsrfProtectionMiddleware([
                'httponly' => true,
            ]));

        return $middlewareQueue;
    }

_csrfToken input field is present:

public function middleware(MiddlewareQueue $middlewareQueue): MiddlewareQueue
    {
        $middlewareQueue
            ->add(new CsrfProtectionMiddleware([
                'httponly' => true,
            ]))

            ->add(new ErrorHandlerMiddleware(Configure::read('Error')))

            ->add(new AssetMiddleware([
                'cacheTime' => Configure::read('Asset.cacheTime'),
            ]))

            ->add(new RoutingMiddleware($this))

            ->add(new BodyParserMiddleware());

        return $middlewareQueue;
    } 

CakePHP Version

4.3.5

PHP Version

8.0.10

markstory commented 2 years ago

Does your error template include forms that require CSRF protection? If so then you need to reorder the middleware like you have noted.

The application skeleton is meant as a starting place and will need to be tweaked to fit the needs of your application. The proposed change will result in CSRF errors being handled by the global exception handlers which has other side effects.

ADmad commented 2 years ago

Having forms on error page is uncommon and the default order is good enough IMO. People can change it to suit their needs if needed.

calcosta commented 2 years ago

In my case the error layout includes the complete application header with the login form, so csrf protection is important. I was not aware that the order in which middlewares are added to the queue is of relevance. Thanks for the clarification.

markstory commented 2 years ago

I was not aware that the order in which middlewares are added to the queue is of relevance.

Yes, it matters very much. Is there something we could add either to the documentation in the book or in the comments to the application skeleton to make that more clear?

ADmad commented 2 years ago

I was not aware that the order in which middlewares are added to the queue is of relevance.

The term "queue" is indicative of its behavior. The order in which the middleware are added to the queue is the order in which they are run, FIFO.

calcosta commented 2 years ago

The term "queue" is indicative of its behavior. The order in which the middleware are added to the queue is the order in which they are run, FIFO.

Yes, of course you are right. But it may be difficult to estimate what effect the order has on the application. The behavior of the ErrorHandlerMiddleware in interaction with the CsrfProtectionMiddleware, for example, is difficult to understand without more detailed knowledge of the respective middlewares. A hint in the documentation would therefore be great.

github-actions[bot] commented 2 years ago

This issue is stale because it has been open for 120 days with no activity. Remove the stale label or comment or this will be closed in 15 days