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

[stimulus-bundle] Export CSRF protection helpers #1375

Merged
merged 1 commit into from
Jan 23, 2025
Merged

[stimulus-bundle] Export CSRF protection helpers #1375

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 40 additions & 22 deletions symfony/stimulus-bundle/2.20/assets/controllers/csrf_protection_controller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,61 +1,79 @@
var nameCheck = /^[-_a-zA-Z0-9]{4,22}$/;
var tokenCheck = /^[-_/+a-zA-Z0-9]{24,}$/;
const nameCheck = /^[-_a-zA-Z0-9]{4,22}$/;
const tokenCheck = /^[-_\/+a-zA-Z0-9]{24,}$/;

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe escaping a slash inside [] isn't needed and it's triggering no-useless-escape in eslint.

// Generate and double-submit a CSRF token in a form field and a cookie, as defined by Symfony's SameOriginCsrfTokenManager
document.addEventListener('submit', function (event) {
var csrfField = event.target.querySelector('input[data-controller="csrf-protection"], input[name="_csrf_token"]');
generateCsrfToken(event.target);
}, true);

Copy link
Member

@nicolas-grekas nicolas-grekas Jan 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry I missed this during the review: why adding true as third argument here @hlecorche ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing that in the capture phase means that this listener (attached on the root of the document) runs before the listeners attached directly on the form. This ensures that listeners attached on a form to perform custom submission logic (using Ajax for instance) will have the value for the generated CSRF token (instead of generating it too late): https://developer.mozilla.org/en-US/docs/Learn_web_development/Core/Scripting/Event_bubbling#event_capture

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and this fixes symfony/symfony#59571

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// When @hotwired/turbo handles form submissions, send the CSRF token in a header in addition to a cookie
// The `framework.csrf_protection.check_header` config option needs to be enabled for the header to be checked
document.addEventListener('turbo:submit-start', function (event) {
const h = generateCsrfHeaders(event.detail.formSubmission);
Object.keys(h).map(function (k) {
event.detail.formSubmission.fetchRequest.headers[k] = h[k];
});
});

// When @hotwired/turbo handles form submissions, remove the CSRF cookie once a form has been submitted
document.addEventListener('turbo:submit-end', function (event) {
removeCsrfToken(event.detail.formSubmission.formElement);
});

export function generateCsrfToken (formElement) {
const csrfField = formElement.querySelector('input[data-controller="csrf-protection"], input[name="_csrf_token"]');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why exporting them ? If the goal is to make them reusable elsewhere, this seems a bad idea to me to mix side effects with reusable utilities: code wanting to use those utilities would trigger the side effects of the file (and if you use a bundler able to perform tree-shaking, side effects are bad as they prevent tree shaking).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's for this reason that the first version of my pull request included two files (Stimulus and utilities)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But yes @stof , the goal is to make them reusable elsewhere :

import { generateCsrfHeaders, removeCsrfToken } from '../controllers/csrf_protection_controller';

// ...

document.addEventListener('before-event-with-jquery-or-custom-fetch-event', function (event) {
	Object.entries(generateCsrfHeaders(event.detail.form)).forEach(([name, value]) => {
	    // add header
	});
});

document.addEventListener('after-event-with-jquery-or-custom-fetch-event', function (event) {
	removeCsrfToken(event.detail.form);
});

It can also be used without relying on the form's submit event :

import { generateCsrfToken, generateCsrfHeaders, removeCsrfToken } from '../controllers/csrf_protection_controller';

// ...

link.addEventListener('click', function (event) {
	const form = document.getElementById('my-form');
	generateCsrfToken(form);
	// Send form with fetch (or jquery) with generateCsrfHeaders and removeCsrfToken functions
});

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we can do something about by leveraging the export default entry to register the listeners when stimulus loads the controller?
having one file is desirable to me


if (!csrfField) {
return;
}

var csrfCookie = csrfField.getAttribute('data-csrf-protection-cookie-value');
var csrfToken = csrfField.value;
let csrfCookie = csrfField.getAttribute('data-csrf-protection-cookie-value');
let csrfToken = csrfField.value;

if (!csrfCookie && nameCheck.test(csrfToken)) {
csrfField.setAttribute('data-csrf-protection-cookie-value', csrfCookie = csrfToken);
csrfField.defaultValue = csrfToken = btoa(String.fromCharCode.apply(null, (window.crypto || window.msCrypto).getRandomValues(new Uint8Array(18))));
csrfField.dispatchEvent(new Event('change', {bubbles: true}));
csrfField.dispatchEvent(new Event('change', { bubbles: true }));
}

if (csrfCookie && tokenCheck.test(csrfToken)) {
var cookie = csrfCookie + '_' + csrfToken + '=' + csrfCookie + '; path=/; samesite=strict';
const cookie = csrfCookie + '_' + csrfToken + '=' + csrfCookie + '; path=/; samesite=strict';
document.cookie = window.location.protocol === 'https:' ? '__Host-' + cookie + '; secure' : cookie;
}
});
}

// When @hotwired/turbo handles form submissions, send the CSRF token in a header in addition to a cookie
// The `framework.csrf_protection.check_header` config option needs to be enabled for the header to be checked
document.addEventListener('turbo:submit-start', function (event) {
var csrfField = event.detail.formSubmission.formElement.querySelector('input[data-controller="csrf-protection"], input[name="_csrf_token"]');
export function generateCsrfHeaders (formElement) {
const headers = {};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we export functions, we should write some JSDoc on them to provide better DX when using them.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More an more I'm thinking such method / exports should not be there.

We provide "plug n play" CSRF here, but we should probably not encourage using this recipe file as vendor/ lib code.

We can remove it / change it at any time in future versions

const csrfField = formElement.querySelector('input[data-controller="csrf-protection"], input[name="_csrf_token"]');

if (!csrfField) {
return;
return headers;
}

var csrfCookie = csrfField.getAttribute('data-csrf-protection-cookie-value');
const csrfCookie = csrfField.getAttribute('data-csrf-protection-cookie-value');

if (tokenCheck.test(csrfField.value) && nameCheck.test(csrfCookie)) {
event.detail.formSubmission.fetchRequest.headers[csrfCookie] = csrfField.value;
headers[csrfCookie] = csrfField.value;
}
});

// When @hotwired/turbo handles form submissions, remove the CSRF cookie once a form has been submitted
document.addEventListener('turbo:submit-end', function (event) {
var csrfField = event.detail.formSubmission.formElement.querySelector('input[data-controller="csrf-protection"], input[name="_csrf_token"]');
return headers;
}

export function removeCsrfToken (formElement) {
const csrfField = formElement.querySelector('input[data-controller="csrf-protection"], input[name="_csrf_token"]');

if (!csrfField) {
return;
}

var csrfCookie = csrfField.getAttribute('data-csrf-protection-cookie-value');
const csrfCookie = csrfField.getAttribute('data-csrf-protection-cookie-value');

if (tokenCheck.test(csrfField.value) && nameCheck.test(csrfCookie)) {
var cookie = csrfCookie + '_' + csrfField.value + '=0; path=/; samesite=strict; max-age=0';
const cookie = csrfCookie + '_' + csrfField.value + '=0; path=/; samesite=strict; max-age=0';

document.cookie = window.location.protocol === 'https:' ? '__Host-' + cookie + '; secure' : cookie;
}
});
}

/* stimulusFetch: 'lazy' */
export default 'csrf-protection-controller';
Loading