Skip to content

[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

Merged
merged 1 commit into from
Jan 6, 2025

Conversation

xabbuh
Copy link
Member

@xabbuh xabbuh commented Dec 24, 2024

Q A
Branch? 7.2
Bug fix? yes
New feature? no
Deprecations? no
Issues Fix #59294
License MIT

@nicolas-grekas
Copy link
Member

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.

@smnandre
Copy link
Member

smnandre commented Dec 24, 2024

Removing this will indeed lead to unsovable problems with Firefox. Full thread here with some recent updates. (updated)

Inputs without autocomplete=off attribute can lead to problems with Firefox. Full thread with details and recent updates: rails/rails#42610

@xabbuh
Copy link
Member Author

xabbuh commented Dec 24, 2024

But why was this not an issue on 7.1 and before?

@smnandre
Copy link
Member

smnandre commented Dec 25, 2024

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.

@nicolas-grekas
Copy link
Member

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.

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

@smnandre
Copy link
Member

But... autocomplete attribute is interpreted during page load, no?

Does this trick work everywhere and .. why ? (genuine question .. i would not be surprise of anything with FF)

@MatTheCat
Copy link
Contributor

MatTheCat commented Dec 30, 2024

Just checked, and dynamically adding the autocomplete attribute in the csrf_protection_controller has no effect on Firefox 133.0.3: if I submit a form and go back, the field keep the previously generated token as its value.

(BTW, it seems that setting a hidden input value property also updates its value attribute 😵)

@MatTheCat
Copy link
Contributor

MatTheCat commented Dec 30, 2024

From https://developer.mozilla.org/en-US/docs/Web/Security/Practical_implementation_guides/Turning_off_form_autocompletion we can read (emphasis mine)

Setting autocomplete="off" on fields […] stops the browser from caching form data in the session history. When form data is cached in session history, the information filled in by the user is shown in the case where the user has submitted the form and clicked the Back button to go back to the original form page.

A user input would update the value property, but not the value attribute. As such I tried to set the attribute instead of the property in the csrf_protection_controller, and it seems to work without an autocomplete attribute..!

- 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 value attribute can be achieved by setting the defaultValue property, so this also works:

- 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))));

@nicolas-grekas
Copy link
Member

@MatTheCat thanks for digging this topic. Can you please confirm that symfony/recipes#1371 would fix the issue?

@nicolas-grekas
Copy link
Member

Thank you @xabbuh.

@nicolas-grekas nicolas-grekas merged commit f6dcd9f into symfony:7.2 Jan 6, 2025
11 checks passed
@xabbuh xabbuh deleted the issue-59294 branch January 6, 2025 09:03
@fabpot fabpot mentioned this pull request Jan 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants