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

CSRF Protection with Fetch Requests #1370

Open
hlecorche opened this issue Dec 24, 2024 · 1 comment
Open

CSRF Protection with Fetch Requests #1370

hlecorche opened this issue Dec 24, 2024 · 1 comment

Comments

@hlecorche
Copy link

First of all, thank you for the great work on the CSRF protection functionality in the csrf_protection_controller.js script. It’s a fantastic addition, and it works really well for handling traditional form submissions.

However, I’ve encountered an issue when using this CSRF protection in the context of fetch requests (without Turbo). Specifically, I found that the CSRF token is not always applied correctly when sending fetch requests, as the event listener for the form submission may be executed before the CSRF logic in csrf_protection_controller.js is applied.

In the current implementation, the CSRF token is injected into the form submission when the submit event is captured :

// csrf_protection_controller.js
document.addEventListener('submit', function (event) {
    // CSRF protection logic
});

This works fine when the form is submitted in the traditional way. However, when using fetch (with custom AJAX logic), I encountered an issue where the CSRF protection logic is executed after the fetch request is sent, which results in the CSRF token not being updated in time.

Here’s an example that illustrates the problem :

document.addEventListener('submit', function (event) {
	if (event.target.matches('form[data-toggle="ajax-form"]')) {
	  event.preventDefault()
	  // Send request with fetch
	}
})

Depending on the order of event listener registration, the event that triggers the fetch request can be executed before the csrf_protection_controller.js event listener. This is exactly what happens in my case, as my event listener is registered during the DOMContentLoaded event, which causes it to run before the CSRF protection logic is applied.

To fix this issue, I used the capture phase (true as the third argument in addEventListener) to ensure that the CSRF protection logic is executed before the fetch request is sent.

// csrf_protection_controller.js
document.addEventListener('submit', function (event) {
    // CSRF protection logic
}, true);  // Using the capture phase to ensure this runs first

Why is the capture phase not used by default in the script? I believe this would help ensure the CSRF token is updated before the form is submitted, especially in cases where fetch is used.

Additionally, I noticed that the CSRF logic in csrf_protection_controller.js is tightly coupled with form submissions with Turbo. However, when using fetch (without Turbo), the same functionality is required — updating the CSRF token — but the existing solution works specifically for Turbo requests. This creates the need for duplication of code to handle CSRF protection in the same way for fetch requests.

It would be more efficient to refactor the CSRF protection logic in csrf_protection_controller.js into reusable JavaScript functions that could be invoked depending on the environment (e.g., when handling fetch requests). This way, the CSRF protection logic for Turbo can be reused without duplicating code, allowing developers to call the relevant functions as needed based on their specific use case.

I’d love to hear your thoughts on whether this approach makes sense and if there are any suggestions for improving this workflow, particularly for fetch requests.

Happy Holidays !

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Dec 29, 2024

Thanks for the feedback.
Would you like to send a PR that'd achieve what you have in mind?
You might have noticed that the current source is at:
https://github.com/symfony/recipes/blob/main/symfony/stimulus-bundle/2.20/assets/controllers/csrf_protection_controller.js

it's open to improvements :)

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

No branches or pull requests

2 participants