Stop syncing value attribute for controlled inputs
aweary opened this issue · 22 comments
Opening this as a follow up to some quick discussions in #11881. Syncing the value attribute has been a consistent source of bugs for us, and the benefits of doing so seem minimal. There's some previous discussion on the topic in #7359 and in other issues, I can't remember right now 😄
This would be a breaking change, so it would have to be done in a major release.
Reasons to keep syncing
- It prevents
form.reset()from putting controlled form inputs into a weird state - Some browser extensions (not sure which) read from the
valueattribute in some cases (not sure which) - It can be useful for querying inputs with a specific value using an attribute selector
Reasons to stop syncing
- It will reduce the complexity of
react-domin a non-trivial way - In turn, it will likely reduce bundle size as well
- We remove a whole class of bugs (fighting with browsers that want to be helpful about input values)
- Syncing the input value to the attribute potentially exposes sensitive data to third party tools (1)
What do we think? Are these reasons good enough to keep syncing the value attribute? Are there other more critical reasons we should keep doing so?
I think we should really try and make this happen, many of these bugs are intractable. Do any other frameworks try and keep them in sync? It seems like something React does out of a sense that it wouldn't be too hard for some benefit (which isn't the case it's hard).
Some browser extensions (not sure which) read from the value attribute in some cases (not sure which)
I don't think we've been able to verify that this is the case? even so tho i'd say that's the extensions's problem (which they should fix). I'll admit tho that stance is only tenable if there isn't a big problem with extensions do this.
The form.reset case seems to be the most compelling for keeping the behavior, but as a strike against it, folks should probably not be using form.reset with controlled components, are their compelling reasons for that? (I've done a lot of work with forms in React and never used form reset with controlled components so IDK)
If you are using controlled components, that means the state is kept somewhere, such as your form controller component. If you do want to be able to use form.reset(), then your component that owns the <form> should listen to onReset and reset its own state.
Stopping the syncing of value does not seem to do anything but enable anti-patterns; although I am curious about the "helpful browser" case.
EDIT: looks like I did confuse value attribute with property 😕
I think the original justification for that was that a lot of people felt that mismatch between value attribute and property was counter-intuitive. By that time many people learned React before the DOM APIs and weren’t aware of the distinction. So it “felt wrong” to them that the attribute in the browser DevTools (looks like HTML!) doesn’t match the “attribute” they see in JSX (looks like HTML too!)
I agree this was a poor justification in hindsight. We probably should’ve solved this by educating users instead (e.g. through documentation and blog posts). Unfortunately now that we’ve “fixed” this it’s likely more people rely on this behavior now. But ultimately I agree we should give up on this and change it back.
It prevents form.reset() from putting controlled form inputs into a weird state
I think we could do some neat stuff here. If the value attribute is not synchronized, we could allow controlled inputs to both control value and defaultValue. Then, on form reset, we could fire a synthetic change event on all of the associated inputs matching the defaultValue. Effectively controlling form.reset.
Some browser extensions (not sure which) read from the value attribute in some cases (not sure which)
It's possible I've forgotten already, but I feel like, every time this comes up, no-one is able to answer this question. I'd really love to nail this one down.
It can be useful for querying inputs with a specific value using an attribute selector
This is nice, but I think it is unique to React. I'd be okay with dropping this and conforming to the way standard HTML manages the value property/attribute natively. It's definitely simpler.
If I get it right, #10150 is an implementation. I'll close it as stale but reference this issue.
Oof. Should we send out a minor release that removes syncing of password inputs?
This is something that's been brought up before as a concern, and it's a good argument in favor of removing attribute syncing (added to the list).
Oof. Should we send out a minor release that removes syncing of password inputs?
It's potentially dangerous behavior, but I believe it's still expected behavior regardless. I'm not sure it's worth releasing a breaking change in a minor release. If anything, it's a good incentive to upgrade to the next major release that removes attribute syncing, assuming we agree to do so.
Relevant PoC exploit (relies on syncing) now making the rounds:
I wonder how much of this is over-focus on one particular vector when there are others similar ones such as :checked selectors, contentEditable content, just any HTML content in general visible on the page...
I don’t really have a good sense of how much worse it is with password fields.
However the use cases are also much more limited so it wouldn’t hurt that bad to do it for password fields in a minor if we’re going to do it for other things later.
However the use cases are also much more limited so it wouldn’t hurt that bad to do it for password fields in a minor if we’re going to do it for other things later.
I can get behind that. I think all defaultValue assignment pathways pass through here:
https://github.com/facebook/react/blob/master/packages/react-dom/src/client/ReactDOMFiberInput.js#L302-L318
It'd be pretty simple to add a case for passwords.
I think things like this are just part and parcel with the web platform like: https://github.com/maxchehab/CSS-Keylogging But i do think that syncing value is breaking expectations for folks more so than it's an attack vector.
I put up a demonstration site to easily see this in action:
https://no-csp-css-keylogger.badsite.io/
In my very high-level GitHub searches, it seems that there are many thousands of repositories that sync value on password fields, let alone all the other fields.
In the interim, might it be worthwhile to mention in the documentation:
https://reactjs.org/docs/forms.html#controlled-components
That it can be dangerous to sync value on input fields that contain sensitive information, if sites don't use Content Security Policy to prevent the injection of untrusted stylesheets?
Hello All,
This is Erez from Checkmarx.
We get a lot of requests from our customers to detect this issue as part of our static code analysis tool.
Please correct me if I'm wrong, but I understand from this thread that a solution to this issue (and related vulnerability) is not expected in the near future. So this is a heads-up that we will add the detection in one of the coming releases.
I totally agree with @april that a clear warning in the documentation will be very beneficial to the users.
Changing this can potentially be breaking so it's not clear to me what is better.
If somebody sends a PR I think we can take it for next minor release.
How about moving towards this in a graduated manner? For example:
- Update documentation to make clear that it's dangerous
- Warn about it in the console, unless
React.allowDangerousControlledComponents(or whatever) is enabled - At a future release, disable it unless
React.allowDangerousControlledComponentsis set - At a future release, disable it unless
React.allowDangerousControlledComponentsis set, warn unless CSP disables inline styles - At a major release, disable it
React.allowDangerousControlledComponentsis set and CSP disables inline styles
Or something along those lines. You could take it as fast or as slow as possible (and that might be more steps than necessary). Given how many sites I've seen on GitHub with this pattern, it seems reasonable likely that it will break things like tools that check for password complexity, etc.
We generally don't use global runtime feature flags like this because they break component ecosystem.
I can send out a PR to do this by mid-day Monday. Mechanically it's pretty simple.
Missed my own deadline! Just to give an update:
I have this working client-side. The complex part actually turns out to be how to handle server-side rendering. I'm trying to figure out the best place to put the password exception and make sure it doesn't impact hydration diffing.
Sent a PR out here:
#12722
I think the approach could use a little refinement, but I believe it covers all of the edge cases:
- Do not send down the value attribute server-side for password inputs
- Hydrate the value property client-side
- Do not set or update the value attribute during client-side rendering
- Respect the new value property for passwords if a user interacts with a password before hydration