Issue metadata
Sign in to add a comment
|
Saved password doesn't work on m.facebook.com
Reported by
mcda...@amazon.com,
Jan 4 2017
|
||||||||||||||||||||||
Issue descriptionExample URL: m.facebook.com Steps to reproduce the problem: Chromium on any android device: 1. Go to m.facebook.com 2. Enter login info, tap "log in" 3. Wait for the "Do you want Chromium to save your password for this site?" infobar to appear, tap "Save" 4. Tap the facebook menu icon in the top right, scroll down to and tap "Log Out" 5. Wait for the username and password fields to autofill. Tap "Log In" What is the expected behavior? Facebook logs in with the saved username and password. What went wrong? "The password you entered is incorrect. Did you forget your password?" This only happens if you attempt to log in with the information autofilled at first page load. For example, if you tap into the password field and select "Use password for: xyz" ("manually" autofilling), logging in works as expected. I haven't seen this on any other sites, but it definitely used to work (bisected to https://chromium.googlesource.com/chromium/src/+/60b7754704a8fd81b0a010ce253b3754e72c50ec as the breaking commit) Does it occur on multiple sites: N/A Is it a problem with a plugin? No Did this work before? Yes v55 (Specifically, broke with this commit: https://chromium.googlesource.com/chromium/src/+/60b7754704a8fd81b0a010ce253b3754e72c50ec) Does this work in other browsers? Yes Chrome version: 57.0.2972.0 Channel: n/a OS Version: Any Flash Version: Shockwave Flash 24.0 r0
,
Jan 5 2017
I tracked down the root cause of this. Chromium doesn't actually autofill passwords until it detects a user gesture on the page. Facebook's login triggers off a touchstart event on the login button. In v55, this used to work because touchstart was considered a user gesture, so the password would be filled in in time. Now in v56, the login triggers before the user gesture so the password isn't yet autofilled. (https://www.chromestatus.com/feature/6131337345892352) Facebook will need to fix this on their end.
,
Jan 6 2017
Yikes this sounds bad, thanks for digging in! I verified I can reproduce the issue on Chrome M56. I'm having a bit of trouble because some of my Chrome instances have extra facebook accounts saved which means autofill doesn't show up until I tap anyway (and I can't find these extra one anywhere in my settings to clear them out). Anyway, if they're logging in on touchstart instead of touchend/click then I would consider that a bug (what if the user is just trying to zoom or pan the page). But it's not the behavior I'm seeing - I can touch on the login button and scroll the page without actually triggering a login. What makes you say they're triggering login on touchstart? I could request a bisect of this regression to confirm that it's really my touch event user-gesture change and not some other sort of autofill regression. I'll reach out to someone at Facebook ASAP to see if we can get their help. M56 is scheduled to hit stable around the end of January so time is pretty tight if we need to revert my change from issue 611981 .
,
Jan 6 2017
I wonder if it's possible there's a race of some sort in the autofill code here? Eg. if it was asyncronously committing the data on any input event which was considered a user gesture then that would tend to happen quickly enough back when touchstart was a user gesture, but would now be a tight race with touchend being the only user gesture (assuming login is being triggered by the touchend). +vasilii/vabr can you help?
,
Jan 6 2017
Proactively marking M56 blocker in case we do need to change something in Chrome to keep from breaking facebook.
,
Jan 6 2017
,
Jan 7 2017
"What makes you say they're triggering login on touchstart?" I deleted the touchstart dom event listeners on the page and that seemed to fix the issue. I also dug into the js code a bit and found what looked like a list of events they were listening on (included touchstart). Trying to debug minified js seemed like a nightmare so I stopped at that point. You're right, it could be something deeper than that. "I could request a bisect of this regression to confirm that it's really my touch event user-gesture change and not some other sort of autofill regression." I did my own bisect on builds from https://commondatastorage.googleapis.com/chromium-browser-snapshots/index.html?prefix=Android/ but feel free to confirm.
,
Jan 9 2017
Thanks for the details! Nope if you've confirmed with a bisect that this was my CL then I believe you, no need to re-confirm :-)
This minified JavaScript is indeed very hard to debug (Chrome pretty-printer doesn't seem to be working for some reason). I've done the following to shed some light on what's going on:
Object.defineProperty(document.querySelector("input[name=pass]"), 'value', {
get: function() {
var val = Object.getOwnPropertyDescriptor(HTMLInputElement.prototype, 'value').get.call(this);
console.log("value getter: " + val);
return val;
}, set: function(v) {
console.log("value setter: " + v);
var ret = Object.getOwnPropertyDescriptor(HTMLInputElement.prototype, 'value').set.call(this, v);
return ret;
}});
monitorEvents(window,"touch")
Doing that I see:
VM1680:1 touchstart TouchEvent {isTrusted: true, touches: TouchList, targetTouches: TouchList, changedTouches: TouchList, altKey: false…}
VM1681:4 value getter:
VM1680:1 touchend TouchEvent {isTrusted: true, touches: TouchList, targetTouches: TouchList, changedTouches: TouchList, altKey: false…}
VM1681:4 value getter: mypassword
VM1681:7 value setter:
VM1681:4 value getter:
VM1681:4 value getter:
So it looks like perhaps something is reading the password during the touchstart handler, then later during the touchend handler clobbering it.
I've tried confirming that there isn't a race in general with password autofill here but I haven't been able to figure out how exactly to trigger (without requiring a single tap) on a test page like https://output.jsbin.com/fafuca. I know password autofill relies on a lot of undocumented heuristics, so that's probably what's making this so annoying to debug :-(.
,
Jan 9 2017
It looks like it's not quite that simple. If I return a bogus password from my overridden getter, the facebook code still invokes the setter with the empty string (I see it hard-coded in the source). But it's definitely saving off the password from the touchstart listener even though login itself isn't triggered until touchend. I re-confirmed that if I remove the touchstart listeners the page seems to work just fine (login with the autofilled password works). I also verified that if my overridden value getter returns the true password, then login works fine. So the facebook code must just be stashing the password during touchstart and not updating that stashed copy when it's read again (this time getting the true value) on touchend. This seems like a pretty weird coding pattern probably limited to facebook. I'm inclined to say we should continue with shipping and leave it to Facebook to update their code to avoid the issue here. I haven't gotten a response from my contacts yet.
,
Jan 10 2017
Sorry for delay, we had holidays. Password Autofill has a feature that the password isn't shared with the web page until a user gesture happens. It's a security feature. After your changes the touchstart event isn't a user gesture. Therefore, the password isn't accessible to the page. I agree that their implementation is strange. - Has somebody already contacted the facebook developers on the issue? If there is a mail thread then I'd like to be CCed on it. - Is there a public documentation online about the project? I suspect there are more sites which may get broken.
,
Jan 10 2017
vasilii@ thanks, that matches my expectations. Any advice on how to reproduce this "autofill password without any tap" UX on a simple demo page like what I was trying to do at https://output.jsbin.com/fafuca? I couldn't find any documentation anywhere for how a developer should structure their page to ensure password autofill will kick in, did I miss it? > Has somebody already contacted the facebook developers on the issue? If there is a mail thread then I'd like to be CCed on it. Yes, I've added you. > Is there a public documentation online about the project? Some. Details are here: https://www.chromestatus.com/feature/6131337345892352, and the current HTML spec lists only touchend: https://html.spec.whatwg.org/multipage/interaction.html#triggered-by-user-activation > I suspect there are more sites which may get broken. If the breakage here is non-trivial (eg. because there are real legitimate use cases that can't be implemented any other way) then we may want to revert. Can you think of any reason why someone might actually want to do it this way? In general sites don't have "activation logic" on touchstart because they don't want to "activate" when the user does a scroll (only a tap). The best I can come up with is that perhaps sites like Facebook want to gain a few ms of response time by kicking off an auth XHR on touchstart, even though they'll only actually update the UI/navigate after the touchend. Unless we have evidence that this pattern is more widespread, or that there's a good reason Facebook needs to behave this way or needs more time to adapt, I think (for maximum security) we should continue with the plan to ship this in M56. WDYT?
,
Jan 10 2017
BTW I tested mobile Safari and there the password form data appears to be available immediately to JavaScript upon page load before the user has interacted with the page at all. vasilii@ Is there a design doc or anything you can point me to that explains in more detail the security benefit of waiting for a user gesture? Eg. perhaps this is designed primarily to help with malicious pages hosting forms inside an iframe? Any chance it's worth considering matching Safari for the main frame case by eliminating the gesture requirement?
,
Jan 11 2017
You can test autofill on https://rsolomakhin.github.io/autofill/. In general, there is nothing required from the developer to kick-in autofill on page load. It's helpful to use autocomplete attributes (https://www.chromium.org/developers/design-documents/form-styles-that-chromium-understands). I can't come up with a case where using touchstart is a must. Thus, I'd proceed for now. The "user gesture" feature exists since the inception of the password manager. It mitigates the XSS attack. So the password is really shared with the page when the user wants to log in (it's any user gesture because we don't know what triggers the submit for sure). I think the security team will be against relaxing this feature.
,
Jan 12 2017
It looks to me like Facebook just changed their code here. Now using the diagnostics in #8 I see simply the following:
touchstart TouchEvent {isTrusted: true, touches: TouchList, targetTouches: TouchList, changedTouches: TouchList, altKey: false…}
touchend TouchEvent {isTrusted: true, touches: TouchList, targetTouches: TouchList, changedTouches: TouchList, altKey: false…}
value getter: mypassword
value getter: mypassword
I'm talking with Facebook folks to try to confirm, but I think it's safe to assume this is now fixed in Facebook.
,
Jan 12 2017
> I can't come up with a case where using touchstart is a must. Thus, I'd proceed for now. By the way, thanks - that makes me feel better. I'll keep my eye out for reports of similar issues elsewhere.
,
Jan 12 2017
Ben Maurer from Facebook confirms this was fixed today. They didn't have a compelling reason for wanting to use touchstart they were just implementing the input handling themselves.
,
Jan 12 2017
I should note that Ben suggests we might want to emit a console warning when reading from password fields before autofill has made the data available (especially in a case like this where we're changing it). It's too late for M56, but if we get reports of other breakage it's worth considering trying to add for M57 (though could be spammy). I did have a warning for a milestone about relying on a user gesture that was going away, but I doubt it was getting triggered in the autofill case (since reading the form value doesn't itself do a UtilizeUserGesture call - autofill pushes the data via some other mechanism).
,
Jan 12 2017
Thanks for the idea. I'll bring it up with our team.
,
Nov 29
|
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by rsgav...@chromium.org
, Jan 4 2017Labels: triage-te