ostark / upper

Integrates Edge Caches like Fastly, KeyCDN, Cloudflare and Varnish with Craft.
MIT License
102 stars 22 forks source link

Make CP, Actions uncacheable #36

Closed timkelty closed 4 years ago

timkelty commented 4 years ago

Obviously, the CP needs to be uncacheable.

This happens somewhat by default, as PHP will set nocache headers whenever a session is started, backed on your session.cache_limiter (ini value)[https://www.php.net/manual/en/session.configuration.php#ini.session.cache-limiter].

However, the CP login page does not start a session, which can result in a cacheable login page, preventing the login from working (due to a cached CSRF).

So, how I'm getting around this is attaching my own behavior to craft\web\Response:

<?php
namespace modules\appmodule\behaviors;

use Craft;
use craft\web\Response;
use yii\base\Behavior;

class ResponseBehavior extends Behavior
{
    const UNCACHABLE_VALUE = 'private, no-cache';

    public function events()
    {
        return [
            Response::EVENT_BEFORE_SEND => 'makeCpUncacheable',
        ];
    }

    public function makeCpUncacheable()
    {
        if (Craft::$app->getRequest()->getIsCpRequest()) {
            $this->makeUncacheable();
        }
    }

    public function makeUncacheable()
    {
        $this->owner->getHeaders()->set('Cache-Control', self::UNCACHABLE_VALUE);
    }
}

This is also nice because in my twig templates I can call {% do craft.app.response.makeUncacheable() %}, and have the actual header values in one place. That is convenient, as depending on your cache service configuration, the required value of the cache-control header can be very specific (e.g. for Fastly it must be private to be uncacheable.

Anyway, I thought this could be a nice addition to CacheControlBehavior. Let me know and I'll send a PR.

ostark commented 4 years ago

@timkelty thanks for your suggestion. I thought all CP routes send private cache control headers. If not, maybe this should be added to craftcms core?

ostark commented 4 years ago

You are right! Feel free to send the PR. Not sure if I like the method name / interface ... What about?:

{% do upper.cache.never() %}
{% do upper.cache.for('12 hours') %}

instead of

{% do craft.app.response.makeUncacheable() %}

(when having a look at the code I noticed how ugly the 1.x branch is)

timkelty commented 4 years ago
{% do upper.cache.never() %}
{% do upper.cache.for('12 hours') %}

I like!

If not, maybe this should be added to craftcms core?

I considered that…but it seems like a pretty specific use of cache-control that really only applies when using a proxy cache. What do you think?

ostark commented 4 years ago

WIP https://github.com/ostark/upper/commit/5f61146a69705b88ecc44db1e1baba3f6dcadcab https://github.com/ostark/upper/commit/9f7bdd600865ad32c5c9b0a83bb62a0afe8d4b40

timkelty commented 4 years ago

@ostark actions are currently cacheable too, e.g. (actions/users/session-info, actions/commerce/cart/get-cart), which is probably not great.

I'm thinkin we should do the same for these by default? Seems like any direct controller action you wouldn't want cached.

timkelty commented 4 years ago

PR'd to expand cache exclusion for action requests.

ostark commented 4 years ago

Sorry for the late release https://github.com/ostark/upper/releases/tag/1.6.0