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

Change autocomplete value to new-password #2258

Closed

Conversation

nitriques
Copy link
Member

This was a breaking change in Chrome M34 that has been fixed in M37: The password manager would completely ignore the autocomplete="off" directive and would still fill up the first input[type=password] it could find.

This was making me nervous since I spotted my password getting saved in plain text in the config file.

The only was to fix (without any hacks) is to use the new value "new-password" (See https://code.google.com/p/chromium/issues/detail?id=370363#c8 and http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2014-March/253895.html)

I've only changed the password field and left the username field untouched.

I've tested it in Chrome and Firefox (both do well).

This would need testing in Safari.

This was a breaking change in Chrome M34 that has been fixed in M37: The password manager would completely ignore the autocomplete="off" directive and would still fill up the first input[type=password] it could find.

This was making me nervous since I spotted my password getting saved in plain text in the config file.

The only was to fix (without any hacks) is to use the new value "new-password" (See https://code.google.com/p/chromium/issues/detail?id=370363#c8 and http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2014-March/253895.html)

I've only changed the password field and left the username field untouched.

I've tested it in Chrome and Firefox (both do well).
@michael-e
Copy link
Member

This looks like a hack. Do I understand it right, this is only intended to work around a browser bug which has been fixed in the meantime?

@nitriques
Copy link
Member Author

This looks like a hack. Do I understand it right, this is only intended to work around a browser bug which has been fixed in the meantime?

Definitivly not. 'new-password' is the new value in the spec. The "bug" was that starting in M34, Chrome completely (and still does) ignore auto-complete=off for password inputs. The "correct" way to prevent this is by using the new value, as per the whatwg spec and M37 implementation states.

See http://stackoverflow.com/questions/12374442/chrome-browser-ignoring-autocomplete-off for the hacks :)

@michael-e
Copy link
Member

Aha, I see. Still, according to the whatwg specs, the new-password value is meant for:

A new password (e.g. when creating an account or changing a password)

which isn't the case here.

Isn't the real problem is that the user has activated the browser's autofill feature (even for passwords)? (The Symphony form is correct, in my eyes.)

@nitriques
Copy link
Member Author

which isn't the case here.

I do not agree. They meant, IMO, that "it's not a login password field". It fits into the "changing a password" category. You're not creating an account when you're setting that value. You're changing it from what it is right now (which can be nothing) to a new value.

Isn't the real problem is that the user has activated the browser's autofill feature

I do not think that this is the problem. It's part of it. But I think that our UI should accommodate for this kind of user preference.

Would you tell a blind user to not use a screen reader? You would try to prevent the zoom feature of the browsers? Password managers are a standard feature of all recent browsers.

@nitriques
Copy link
Member Author

In that regards, we should also apply the same value when changing an author password (and should allow it for the current user's password).

@michael-e
Copy link
Member

In that regards, we should also apply the same value when changing an author password (and should allow it for the current user's password).

Yep, exactly. When a password is changed, we should apply new-password. In other cases, we shouldn't work against the browser's behaviour.

It fits into the "changing a password" category.

That is the only point which I don't understand. We are talking about the preferences page, right? Why should this setting be a "new password"?

@nitriques
Copy link
Member Author

That is the only point which I don't understand. We are talking about the preferences page, right?

Yes!

Why should this setting be a "new password"?

Because you're not creating the password. You're setting it.

@michael-e
Copy link
Member

Hmmm, what's the difference between creating and setting?

Wouldn't it be much more logical to apply autocomplete="off" to the whole preferences form, as mentioned in http://stackoverflow.com/a/16130452 ?

@nitriques
Copy link
Member Author

Hmmm, what's the difference between creating and setting?

I my mind, creating would be actually allowing the user to use that password (e.g. user account creation) where here, it's more of a configuration setting, which should never be auto-filled.

Wouldn't it be much more logical to apply autocomplete="off"

username and password field already have it, but Chrome ignores it...

@michael-e
Copy link
Member

The Stackoverflow comment says that it must be applied to the <form> tag. Have you tried that?

@nitriques
Copy link
Member Author

Have you tried that?

Yeah, not working with M38 nor M41.

@michael-e
Copy link
Member

How bad. But I am a bit lost now – in your first post you said that the main bug (honoring autocomplete="off to the password input itself) has been fixed in M37. So I assumed that you are trying to fix "outdated" Chrome versions.

it's more of a configuration setting, which should never be auto-filled

Yes, I agree on that. Still, using autocomplete="new-password" to achieve this looks wrong to me.

@nitriques
Copy link
Member Author

has been fixed in M37. So I assumed that you are trying to fix "outdated" Chrome versions.

No, sorry, I may have not be clear on that. It has been "fixed" by introducing the new value.

looks wrong to me.

Why? Because they choose a sucky name ?

@michael-e
Copy link
Member

No, because the specs say that it should be used for:

A new password (e.g. when creating an account or changing a password)

which isn't the case here. I really wonder if there is no better solution. :-(

@jensscherbl
Copy link
Member

whatwg spec

I'm for sticking with W3C spec. WHATWG is more experimental and changes more frequently...

@michael-e
Copy link
Member

The W3C specs say:

The attribute takes two values, on and off.

@michael-e
Copy link
Member

Honestly, I would rather stick with these two values, too. It's not Symphony's fault if browsers are doing this wrong. (What's so difficult about on and off?)

@nitriques
Copy link
Member Author

What's so difficult about on and off?

I totally agree. But I was really pissed off to see my password in the config file this morning.

All of your concerns are perfectly valid. My proposed solution was the only one that did not felt hacky.

@jensscherbl
Copy link
Member

The W3C specs say

I mean the most current HTML5 spec, but it says on or off only as well...

Edit

To be honest, I really don't care about this particular thing, but I think it would be a good idea in general to decide and communicate what standard we're following for backend and extensions.

@michael-e
Copy link
Member

@nitriques: I understand that this feels terrible, but should we really implement "experimental stuff" to solve this? In my eyes the risk of breaking other clients/implementations (which stick to the specs) is too high (just for the sake of fixing a single browser).

@nitriques
Copy link
Member Author

(just for the sake of fixing a single browser).

Both firefox and chrome did implemented it. That's why I was curious Safari. If the three browsers agree on the implementation, I do not really care if it's in the spec or not.

If it does break things in one browser, I agree, we should trash this PR and let the autocomplete value to off.

@michael-e
Copy link
Member

I am not only thinking about current browsers, but also about future versions/browsers. If you leave the specs, you can never be sure that things won't break tomorrow.

@nitriques
Copy link
Member Author

you can never be sure that things won't break tomorrow

That's how I feel about anything frontend related 😄

The

element was part of the spec. And then, it got rip apart...

@michael-e
Copy link
Member

@brendo should decide what to do here. (In my eyes it should not be pulled.)

@nitriques
Copy link
Member Author

@michael-e Yeah I'd love to have more opinions (and more test) on this topic too. (That what PR are for right?)

More information on the topic (going both ways)

https://groups.google.com/a/chromium.org/forum/#!msg/security-dev/wYGThW5WRrE/qiWrKwJ79S4J

http://www.theregister.co.uk/2014/04/09/chrome_makes_new_password_grab_in_version_34/

And even, what WP did: https://core.trac.wordpress.org/ticket/24364

@brendo
Copy link
Member

brendo commented Nov 25, 2014

So aside from why they have settled on an odd name for the new autocomplete value, is there any reason why this patch is only applied to the single field and not other password fields (such as Author?)?

@nitriques
Copy link
Member Author

So aside from why they have settled on an odd name for the new autocomplete value, is there any reason why this patch is only applied to the single field and not other password fields (such as Author?)?

No!

See my previous comment

In that regards, we should also apply the same value when changing an author password (and should allow it for the current user's password).

I just never pushed it because the debate was far from over. Please weigh in Brendan!

@jensscherbl
Copy link
Member

@nitriques I still think we should stick to the HTML5 spec and don't merge this. What do you think?

@brendo
Copy link
Member

brendo commented Mar 24, 2015

Agree, Chrome 37 is a thing of the past now as well.

@brendo brendo closed this Mar 24, 2015
@nitriques
Copy link
Member Author

I agree, but

This was making me nervous since I spotted my password getting saved in plain text in the config file.

I still get this. The SMTP fields are auto-completed even when not visible, which saves my symphony password into it. (with both M41 and M43)

I agree that including non-standard stuff sucks. But I still feels that this is a problem.

@jensscherbl
Copy link
Member

I still get this. The SMTP fields are auto-completed even when not visible, which saves my symphony password into it.

Maybe you should file a bug report on the chromium issue tracker then. If the field has autocompletion turned off and the browser simply ignores it, it's clearly a browser problem.

@nitriques
Copy link
Member Author

If the field has autocompletion turned off and the browser simply ignores it, it's clearly a browser problem.

No. It's "by design"

https://code.google.com/p/chromium/issues/detail?id=352347
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/zhhj7hCip5c/discussion

And it turns out I could run chrome with a special flag... Which may not what I want.... I like to have auto-completion... But not everywhere.

@jensscherbl
Copy link
Member

No. It's "by design"

A browser problem "by design" is still a browser problem.

I like to have auto-completion... But not everywhere.

Maybe you should use a browser where it works as expected then ;)

@nitriques
Copy link
Member Author

A browser problem "by design" is still a browser problem.

I agree. But this cancels all my arguments.

Maybe you should use a browser where it works as expected then

Yeah, I'll use Spartan 😄

nitriques added a commit that referenced this pull request Jun 7, 2016
Not visible element should be made readonly, since the user can't edit
them anyways.

This fixes a problem where Chrome would auto-fill hidden input and the
value would be saved without the user ever seeing it.

We already tried to fix the problem usign a standard solution
(auto-complete="off", #1843 and #1841) but it does not work. We also tried a non-standard
solution (#2258) which was rejected.

This change only uses standard solution, even thought it relies on
javascript to make things works (it should not be a problem since the
backend now heavily relies on javascript)
jensscherbl pushed a commit that referenced this pull request May 28, 2017
Not visible element should be made readonly, since the user can't edit
them anyways.

This fixes a problem where Chrome would auto-fill hidden input and the
value would be saved without the user ever seeing it.

We already tried to fix the problem usign a standard solution
(auto-complete="off", #1843 and #1841) but it does not work. We also tried a non-standard
solution (#2258) which was rejected.

This change only uses standard solution, even thought it relies on
javascript to make things works (it should not be a problem since the
backend now heavily relies on javascript)
nitriques added a commit that referenced this pull request Jun 16, 2017
Not visible element should be made readonly, since the user can't edit
them anyways.

This fixes a problem where Chrome would auto-fill hidden input and the
value would be saved without the user ever seeing it.

We already tried to fix the problem usign a standard solution
(auto-complete="off", #1843 and #1841) but it does not work. We also tried a non-standard
solution (#2258) which was rejected.

This change only uses standard solution, even thought it relies on
javascript to make things works (it should not be a problem since the
backend now heavily relies on javascript)

Picked from 301d2b7
nitriques added a commit that referenced this pull request Jun 16, 2017
Not visible element should be made readonly, since the user can't edit
them anyways.

This fixes a problem where Chrome would auto-fill hidden input and the
value would be saved without the user ever seeing it.

We already tried to fix the problem usign a standard solution
(auto-complete="off", #1843 and #1841) but it does not work. We also tried a non-standard
solution (#2258) which was rejected.

This change only uses standard solution, even thought it relies on
javascript to make things works (it should not be a problem since the
backend now heavily relies on javascript)

Picked from 301d2b7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants