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.

Sign up for GitHub

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

Jump to bottom

[5.x] Decouple CSRF token from nocache script #11014

Open
wants to merge 13 commits into
base: 5.x
Choose a base branch
from
Open

[5.x] Decouple CSRF token from nocache script #11014

wants to merge 13 commits into from

Conversation

aerni
Copy link
Contributor

@aerni aerni commented Oct 25, 2024 •
edited
Loading

This PR takes another stab at #10306 which was reverted in #10898. The nocache_js_position config option introduced in the latter PR isn't optimal as you've got to pick your poison and choose between Livewire or nocache to work as expected.

This PR picks up on the idea pointed out here and extracts the CSRF token replacer script from the nocache replacer script. The CSRF replacer script is inserted as the first script in the head, while the nocache replacer script is placed at the end of the body. This way, you don't have to pick the script's position and can have both Livewire and nocache work alongside each other.

Introduced changes

The decoupled CSRF and nocache scripts are opt-in and can be activated in the static_caching.php config file:

decouple_nocache_scripts => true

If set to true, the CSRF and nocache replacer scripts will be decoupled as described above. The setting defaults to false, which enforces the current behaviour and will output one script containing both the CSRF and nocache replacer logic.

Events

When the decouple_nocache_scripts setting is activated, the statamic:nocache.replaced event is no longer dispatched when the CSRF token is replaced. It is now only dispatched by the nocache script. A new statamic:csrf.replaced event is dispatched instead.

Script replacement

When the decouple_nocache_scripts setting is activated, the StaticCache::nocacheJs($script) method now only replaces the nocache script. It doesn't touch the CSRF token script. A new StaticCache::csrfTokenJs($script) method is introduced to allow overriding the default CSRF script.

Testing

Here's a basic layout that you can use for testing. Note, that the CSRF token is replaced and nocache also works. Make sure to enable the new config option and full static caching.

<!doctype html>
<html>
    <head>
        <meta charset="utf-8">
        <meta http-equiv="X-UA-Compatible" content="IE=edge">
        <meta name="viewport" content="width=device-width, initial-scale=1">
    </head>
    <body>
        {{ nocache }}
            {{ now | iso_format('Y-m-d H:i:s') }}
        {{ /nocache }}
        <script data-csrf="{{ csrf_token }}"></script>
    </body>
</html>

Minor breaking change

I've removed the nocache_js_position config option introduced in #10898 as it is obsolete now. I don't think this should trip up too many people and it hasn't even been documented yet. If removing this setting was overzealous, we could add it back and make it, that if it's set to head it will decouple the scripts, and if it's set to body it will output the merged scripts. This setting was only ever useful if you're using Livewire. And in that case, I think it's safe to decouple the scripts.

For future simplicity, I also suggest removing the decouple_nocache_scripts feature flag for v6 and making this a breaking change.

Let me know what you think.

@aerni aerni force-pushed the feature/csrf-script branch from 4a73e63 to 13f1ad4 Compare October 31, 2024 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@aerni