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.

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

Stop syncing value attribute for controlled inputs #11896

Open
aweary opened this issue Dec 20, 2017 · 22 comments

Comments

@aweary
Copy link
Member

commented Dec 20, 2017

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 value attribute 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-dom in 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?

cc @nhunzaker @jquense @gaearon

@jquense

This comment has been minimized.

Copy link
Collaborator

commented Dec 20, 2017

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)

@Jessidhia

This comment has been minimized.

Copy link
Contributor

commented Dec 22, 2017

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 😕

@gaearon

This comment has been minimized.

Copy link
Member

commented Dec 22, 2017

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.

@nhunzaker

This comment has been minimized.

Copy link
Collaborator

commented Dec 22, 2017

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.

@gaearon gaearon added this to the 17.0.0 milestone Jan 5, 2018

@gaearon

This comment has been minimized.

Copy link
Member

commented Jan 5, 2018

If I get it right, #10150 is an implementation. I'll close it as stale but reference this issue.

@sebmarkbage

This comment has been minimized.

@nhunzaker

This comment has been minimized.

Copy link
Collaborator

commented Feb 5, 2018

Oof. Should we send out a minor release that removes syncing of password inputs?

@aweary

This comment has been minimized.

Copy link
Member Author

commented Feb 5, 2018

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.

@noinkling

This comment has been minimized.

Copy link

commented Feb 21, 2018

Relevant PoC exploit (relies on syncing) now making the rounds:

@sebmarkbage

This comment has been minimized.

Copy link
Member

commented Feb 21, 2018

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.

@nhunzaker

This comment has been minimized.

Copy link
Collaborator

commented Feb 21, 2018

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.

@jquense

This comment has been minimized.

Copy link
Collaborator

commented Feb 21, 2018

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.

@april

This comment has been minimized.

Copy link

commented Feb 23, 2018

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.

@april

This comment has been minimized.

Copy link

commented Feb 23, 2018

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?

@ErezYalon

This comment has been minimized.

Copy link

commented Apr 27, 2018

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.

@gaearon

This comment has been minimized.

Copy link
Member

commented Apr 27, 2018

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.

@april

This comment has been minimized.

Copy link

commented Apr 27, 2018

How about moving towards this in a graduated manner? For example:

  1. Update documentation to make clear that it's dangerous
  2. Warn about it in the console, unless React.allowDangerousControlledComponents (or whatever) is enabled
  3. At a future release, disable it unless React.allowDangerousControlledComponents is set
  4. At a future release, disable it unless React.allowDangerousControlledComponents is set, warn unless CSP disables inline styles
  5. At a major release, disable it React.allowDangerousControlledComponents is 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.

@gaearon

This comment has been minimized.

Copy link
Member

commented Apr 27, 2018

We generally don't use global runtime feature flags like this because they break component ecosystem.

@nhunzaker

This comment has been minimized.

Copy link
Collaborator

commented Apr 27, 2018

I can send out a PR to do this by mid-day Monday. Mechanically it's pretty simple.

@nhunzaker

This comment has been minimized.

Copy link
Collaborator

commented Apr 30, 2018

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.

@nhunzaker

This comment has been minimized.

Copy link
Collaborator

commented May 1, 2018

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
@Jessidhia

This comment has been minimized.

Copy link
Contributor

commented Oct 4, 2018

Does #13526 replace this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.