Skip to content

Ensure response is not cached when generating a CSRF token#15281

Merged
brandonkelly merged 2 commits into4.xfrom
feature/no-cache-csrf
Jul 2, 2024
Merged

Ensure response is not cached when generating a CSRF token#15281
brandonkelly merged 2 commits into4.xfrom
feature/no-cache-csrf

Conversation

@timkelty
Copy link
Copy Markdown
Contributor

@timkelty timkelty commented Jul 2, 2024

Description

Prevents accidental caching when generating CSRF. Technically we could now remove this explicit call to setNoCacheHeaders, but I left it for now.

Related issues

@brandonkelly brandonkelly merged commit 0e1a998 into 4.x Jul 2, 2024
@brandonkelly brandonkelly deleted the feature/no-cache-csrf branch July 2, 2024 14:24
@mikesnoeren
Copy link
Copy Markdown

Just to make sure I understand this correctly, does this mean that when the code snippet below is added to your _layout.twig file (which every page is extending), all pages will have a no cache header set?

<script>
    window.csrfTokenName = "{{ craft.app.config.general.csrfTokenName|e('js') }}";
    window.csrfTokenValue = "{{ craft.app.request.csrfToken|e('js') }}";
</script>

If so, that might be problematic for a lot of websites.

@brandonkelly
Copy link
Copy Markdown
Member

@mikesnoeren Basically yes. (Actually, your comment made us realize this PR didn’t go quite far enough, and we should be doing the same thing from Request::getCsrfToken() as well. Addressing that in the next release.)

There are two reasons we made this change:

  • CSRF tokens are ephemeral, and will be cycled whenever the user session changes. If you load a page and the browser caches it with the current CSRF token, then come back to it later and the browser decides to load the cached version, forms on the page will likely break because they’re stuck with an outdated CSRF token value.
  • They’re unique to each browser session as well. Without setting no-cache headers, the Craft response is giving the green light to services like Cloudflare to cache the response for all visitors, when they should be each getting their own CSRF token values.

Craft 4.9 and 5.2 added the asyncCsrfInputs config setting, which makes it easy to include CSRF inputs in forms without sacrificing cachability. When that’s enabled, csrfInput() will return a <craft-csrf-input> placeholder element rather than an input, and registers JS code that fetches a CSRF token via Ajax and swaps out the placeholder with an input on page load.

You could take the same Ajax approach with your JS code:

<script>
  (function() {
    fetch("{{ actionUrl('users/session-info') }}", {
      headers: {
        'Accept': 'application/json',
      }
    })
      .then(response => response.json())
      .then(data => {
        window.csrfTokenName = data.csrfTokenName;
        window.csrfTokenValue = data.csrfTokenValue;
      });
  })();
</script>

@mikesnoeren
Copy link
Copy Markdown

That makes total sense, and it seems like a good change, especialy the asyncCsrfInputs config setting is a very welcome change.

However, this update could have significant performance implications for sites that rely heavily on craft native caching. Would it be possible to include a notice in the release notes or update documentation highlighting this change? This would allow developers to assess the potential impact on their sites and make any necessary adjustments before updating.

Perhaps something like:
'Note: This update modifies how CSRF tokens are handled, which may affect caching behavior on some pages. Please review your site's caching strategy after updating.'

@brandonkelly
Copy link
Copy Markdown
Member

To be clear, this only impacts the cache response headers, e.g. Cache-Control. It won’t impact any of Craft’s own caching features, like {% cache %} tags.

@mikesnoeren
Copy link
Copy Markdown

Ah yes, I apologize for the confusion in my last message. However, the core intention remains the same. We have a couple of sites cached behind Cloudflare to significantly reduce the load on their servers. Updating to this release without thoroughly reviewing all the update logs could potentially lead to issues or even server crashes.

brandonkelly added a commit that referenced this pull request Jul 9, 2024
@brandonkelly
Copy link
Copy Markdown
Member

Fair enough, added a note to the release notes for the next release (be75da6).

@paulwaddington
Copy link
Copy Markdown

We're applying this to a multi-site setup and running into a CORS issue, with the error "No 'Access-Control-Allow-Origin' header is present on the requested resource".

I assume we need to allow the origin URL as below, though we've not established how to do that yet!

Access-Control-Allow-Origin: www.example.com

@timkelty
Copy link
Copy Markdown
Contributor Author

@paulwaddington the upcoming Craft 4.11/5.3 release will provide a easy way to do this via config/app.php. See details in #15397

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

Successfully merging this pull request may close these issues.

4 participants