-
-
Notifications
You must be signed in to change notification settings - Fork 487
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
[stimulus-bundle] Export CSRF protection helpers #1375
Conversation
Thanks for the PR 😍 How to test these changes in your application
Diff between recipe versionsIn order to help with the review stage, I'm in charge of computing the diff between the various versions of patched recipes. symfony/stimulus-bundle2.8 vs 2.9diff --git a/symfony/stimulus-bundle/2.9/assets/bootstrap.js b/symfony/stimulus-bundle/2.9/assets/bootstrap.js
new file mode 100644
index 0000000..2689398
--- /dev/null
+++ b/symfony/stimulus-bundle/2.9/assets/bootstrap.js
@@ -0,0 +1,2 @@
+// register any custom, 3rd party controllers here
+// app.register('some_controller_name', SomeImportedController);
diff --git a/symfony/stimulus-bundle/2.9/assets/controllers/hello_controller.js b/symfony/stimulus-bundle/2.9/assets/controllers/hello_controller.js
new file mode 100644
index 0000000..e847027
--- /dev/null
+++ b/symfony/stimulus-bundle/2.9/assets/controllers/hello_controller.js
@@ -0,0 +1,16 @@
+import { Controller } from '@hotwired/stimulus';
+
+/*
+ * This is an example Stimulus controller!
+ *
+ * Any element with a data-controller="hello" attribute will cause
+ * this controller to be executed. The name "hello" comes from the filename:
+ * hello_controller.js -> "hello"
+ *
+ * Delete this file or adapt it for your use!
+ */
+export default class extends Controller {
+ connect() {
+ this.element.textContent = 'Hello Stimulus! Edit me in assets/controllers/hello_controller.js';
+ }
+}
diff --git a/symfony/stimulus-bundle/2.9/assets/controllers.json b/symfony/stimulus-bundle/2.9/assets/controllers.json
new file mode 100644
index 0000000..a1c6e90
--- /dev/null
+++ b/symfony/stimulus-bundle/2.9/assets/controllers.json
@@ -0,0 +1,4 @@
+{
+ "controllers": [],
+ "entrypoints": []
+}
diff --git a/symfony/stimulus-bundle/2.8/manifest.json b/symfony/stimulus-bundle/2.9/manifest.json
index ff66e87..60e0ddb 100644
--- a/symfony/stimulus-bundle/2.8/manifest.json
+++ b/symfony/stimulus-bundle/2.9/manifest.json
@@ -2,5 +2,46 @@
"bundles": {
"Symfony\\UX\\StimulusBundle\\StimulusBundle": ["all"]
},
- "aliases": ["stimulus", "stimulus-bundle"]
+ "copy-from-recipe": {
+ "assets/": "assets/"
+ },
+ "aliases": ["stimulus", "stimulus-bundle"],
+ "conflict": {
+ "symfony/webpack-encore-bundle": "<2.0",
+ "symfony/flex": "<1.20.0 || >=2.0.0,<2.3.0"
+ },
+ "add-lines": [
+ {
+ "file": "webpack.config.js",
+ "content": "\n // enables the Symfony UX Stimulus bridge (used in assets/bootstrap.js)\n .enableStimulusBridge('./assets/controllers.json')",
+ "position": "after_target",
+ "target": ".splitEntryChunks()"
+ },
+ {
+ "file": "assets/app.js",
+ "content": "import './bootstrap.js';",
+ "position": "top",
+ "warn_if_missing": true
+ },
+ {
+ "file": "assets/bootstrap.js",
+ "content": "import { startStimulusApp } from '@symfony/stimulus-bridge';\n\n// Registers Stimulus controllers from controllers.json and in the controllers/ directory\nexport const app = startStimulusApp(require.context(\n '@symfony/stimulus-bridge/lazy-controller-loader!./controllers',\n true,\n /\\.[jt]sx?$/\n));",
+ "position": "top",
+ "requires": "symfony/webpack-encore-bundle"
+ },
+ {
+ "file": "assets/bootstrap.js",
+ "content": "import { startStimulusApp } from '@symfony/stimulus-bundle';\n\nconst app = startStimulusApp();",
+ "position": "top",
+ "requires": "symfony/asset-mapper"
+ },
+ {
+ "file": "templates/base.html.twig",
+ "content": " {{ ux_controller_link_tags() }}",
+ "position": "after_target",
+ "target": "{% block stylesheets %}",
+ "warn_if_missing": true,
+ "requires": "symfony/asset-mapper"
+ }
+ ]
} 2.9 vs 2.13diff --git a/symfony/stimulus-bundle/2.9/manifest.json b/symfony/stimulus-bundle/2.13/manifest.json
index 60e0ddb..4701215 100644
--- a/symfony/stimulus-bundle/2.9/manifest.json
+++ b/symfony/stimulus-bundle/2.13/manifest.json
@@ -34,14 +34,6 @@
"content": "import { startStimulusApp } from '@symfony/stimulus-bundle';\n\nconst app = startStimulusApp();",
"position": "top",
"requires": "symfony/asset-mapper"
- },
- {
- "file": "templates/base.html.twig",
- "content": " {{ ux_controller_link_tags() }}",
- "position": "after_target",
- "target": "{% block stylesheets %}",
- "warn_if_missing": true,
- "requires": "symfony/asset-mapper"
}
]
} 2.13 vs 2.20diff --git a/symfony/stimulus-bundle/2.20/assets/controllers/csrf_protection_controller.js b/symfony/stimulus-bundle/2.20/assets/controllers/csrf_protection_controller.js
new file mode 100644
index 0000000..05e46b7
--- /dev/null
+++ b/symfony/stimulus-bundle/2.20/assets/controllers/csrf_protection_controller.js
@@ -0,0 +1,79 @@
+const nameCheck = /^[-_a-zA-Z0-9]{4,22}$/;
+const tokenCheck = /^[-_\/+a-zA-Z0-9]{24,}$/;
+
+// 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) {
+ generateCsrfToken(event.target);
+}, true);
+
+// 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"]');
+
+ if (!csrfField) {
+ return;
+ }
+
+ 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 }));
+ }
+
+ if (csrfCookie && tokenCheck.test(csrfToken)) {
+ const cookie = csrfCookie + '_' + csrfToken + '=' + csrfCookie + '; path=/; samesite=strict';
+ document.cookie = window.location.protocol === 'https:' ? '__Host-' + cookie + '; secure' : cookie;
+ }
+}
+
+export function generateCsrfHeaders (formElement) {
+ const headers = {};
+ const csrfField = formElement.querySelector('input[data-controller="csrf-protection"], input[name="_csrf_token"]');
+
+ if (!csrfField) {
+ return headers;
+ }
+
+ const csrfCookie = csrfField.getAttribute('data-csrf-protection-cookie-value');
+
+ if (tokenCheck.test(csrfField.value) && nameCheck.test(csrfCookie)) {
+ headers[csrfCookie] = csrfField.value;
+ }
+
+ return headers;
+}
+
+export function removeCsrfToken (formElement) {
+ const csrfField = formElement.querySelector('input[data-controller="csrf-protection"], input[name="_csrf_token"]');
+
+ if (!csrfField) {
+ return;
+ }
+
+ const csrfCookie = csrfField.getAttribute('data-csrf-protection-cookie-value');
+
+ if (tokenCheck.test(csrfField.value) && nameCheck.test(csrfCookie)) {
+ 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';
diff --git a/symfony/stimulus-bundle/2.13/manifest.json b/symfony/stimulus-bundle/2.20/manifest.json
index 4701215..4289495 100644
--- a/symfony/stimulus-bundle/2.13/manifest.json
+++ b/symfony/stimulus-bundle/2.20/manifest.json
@@ -7,6 +7,8 @@
},
"aliases": ["stimulus", "stimulus-bundle"],
"conflict": {
+ "symfony/framework-bundle": "<7.2",
+ "symfony/security-csrf": "<7.2",
"symfony/webpack-encore-bundle": "<2.0",
"symfony/flex": "<1.20.0 || >=2.0.0,<2.3.0"
}, |
I've tested it in a POST form in a turbo-frame that was failing with the current csrf_protection_controller.js and it worked flawlessly. Thanks @hlecorche. |
Do we really need two distinct files for this ? :| |
Definitely one file please here. |
Head branch was pushed to by a user without write access
Pull request was closed
@nicolas-grekas I have merged everything into a single file. Here are the modified examples : This code can be used without Turbo. Here's an example: 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
}); |
// 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) { | ||
Object.entries(generateCsrfHeaders(event.detail.formSubmission.formElement)).forEach(([name, value]) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Object.entries isn't available with IE11, while I think the previous script was working with it.
export
is also a feature not compatible with IE11 so that maybe the script didn't work with it already, but maybe the eshim made it work in the end?
What I mean is: if the snippet worked with IE11, let's not break IE11 support. Any idea of the status of this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turbo and Stimulus do not support IE 11, so I believe it is fine to use more modern API's
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No IE support for Stimulus, Turbo, import map, nor the importmap polyfill (see here)
No consequence / difference with today
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this comment is old, the polyfill does support IE11 (at least the .dist version, I've tried already)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dist
file of es-module-shims
uses async
/await
among other things. So it definitely does not support IE11 without transpiling. And it also uses some modern methods that won't work without polyfiling them.
Projects needing to support IE11 probably have an infrastructure to transpile code to ES5 and load polyfills, which would work for this file as well.
2a4fbea
to
5c9bc98
Compare
5c9bc98
to
2c0d32f
Compare
Thank you @hlecorche |
Thanks @nicolas-grekas |
|
||
// 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); |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
removeCsrfToken(event.detail.formSubmission.formElement); | ||
}); | ||
|
||
export function generateCsrfToken (formElement) { |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
});
There was a problem hiding this comment.
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
// 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"]'); | ||
export function generateCsrfHeaders (formElement) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
|
||
// 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); |
There was a problem hiding this comment.
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
Here is my first proposal addressing #1370:
var
byconst
orlet
for better modern JavaScript practices.This code can be used without Turbo. Here's an example:
It can also be used without relying on the form's submit event :
I have made these changes in version 2.20 (the existing version). However, since the behavior of the files is modified, I assume it is necessary to create a new version.