Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Jul 2, 2024

Conversation

timkelty
Copy link
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

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
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

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
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

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
Member

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

@paulwaddington
Copy link

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
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