-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Form] do not render hidden CSRF token forms with autocomplete set to off #59296
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
Conversation
xabbuh
commented
Dec 24, 2024
Q | A |
---|---|
Branch? | 7.2 |
Bug fix? | yes |
New feature? | no |
Deprecations? | no |
Issues | Fix #59294 |
License | MIT |
I added it because my browser (Firefox) was actually auto filling it, which lead to broken UX. Maybe the conditions weren't exactly the same as I was developing the feature. We should first double confirm this. IIRC, this could matter when refreshing the page. |
Inputs without |
But why was this not an issue on 7.1 and before? |
After re-reading both Github thread and the changes from @nicolas-grekas, I’m starting to have doubts. Hidden fields tend to encounter issues in Firefox under two scenarios. The first is when their values or adjacent fields are manipulated via JavaScript. However, this doesn’t seem to be the case here, as the token is present in the HTML response. The second, which I’ve recently encountered on a project and affects all browsers, involves the infamous back-forward cache (bfc or bfcache). This browser feature stores entire pages in memory for faster navigation but can cause inconsistencies if JavaScript or state changes aren’t properly handled during navigation. Notably, commonly used cache headers have no effect on its activation—only no-store can disable it (for now). (This reminds me, I need to open a PR about the cache attribute!) I’m starting to think the second scenario might be more relevant in this case. |
The issue happens because the JS snippet attached to the new CSRF protection changes the value of the field before submitting the form. But then, the value shouldn't be memoized on page refresh. Still, that's what FF does when the attribute is not here. I guess we can update the snippet to make it set the attribute instead, so that the generated HTML is cleaner. Or we can keep as is: strictly valid HTML isn't a requirement in itself - the web is built on being able to adapt to various levels of undefined things. |
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... Does this trick work everywhere and .. why ? (genuine question .. i would not be surprise of anything with FF) |
Just checked, and dynamically adding the (BTW, it seems that setting a |
From https://developer.mozilla.org/en-US/docs/Web/Security/Practical_implementation_guides/Turning_off_form_autocompletion we can read (emphasis mine)
A user input would update the - csrfField.value = csrfToken = btoa(String.fromCharCode.apply(null, (window.crypto || window.msCrypto).getRandomValues(new Uint8Array(18))));
+ csrfField.setAttribute('value', csrfToken = btoa(String.fromCharCode.apply(null, (window.crypto || window.msCrypto).getRandomValues(new Uint8Array(18))))); Note that setting the - csrfField.value = csrfToken = btoa(String.fromCharCode.apply(null, (window.crypto || window.msCrypto).getRandomValues(new Uint8Array(18))));
+ csrfField.defaultValue = csrfToken = btoa(String.fromCharCode.apply(null, (window.crypto || window.msCrypto).getRandomValues(new Uint8Array(18)))); |
@MatTheCat thanks for digging this topic. Can you please confirm that symfony/recipes#1371 would fix the issue? |
Thank you @xabbuh. |