Keychain can fill in username/password that doesn't match data prefilled by JavaScript



Camino Graveyard
OS Integration
11 years ago
10 years ago


(Reporter: Smokey Ardisson (offline for a while; not following bugs - do not email), Assigned: Stuart Morgan)


Mac OS X




(1 attachment)

878 bytes, patch
Mike Pinkerton (not reading bugmail)
: superreview+
Details | Diff | Splinter Review
In another bug, we mentioned that we shouldn't let the Keychain fill in data when what it wants to fill doesn't match the existing username.

I've just run into a scenario where this happens:

I just a got a new account ID/password at investools, so yesterday I logged in with the new details.  My cookie got updated with new values, and I told Camino to update the existing Keychain entry with the new values.  Today I came back to log-in, and the cookie prefilled the new username/password, and then I got the "Allow Camino to access the Keychain" prompt, and after allowing, the entries in the login form were now replaced by the old username/password (which were somehow preserved in the Keychain instead of updated; bug to follow on that).

Comment 1

11 years ago
As I suspected, they are doing the fill with JS, rather than the more usual technique of inserting it directly into the HTML when creating the page. Still, it's odd that you could see the username and password when we prompted, so I'll have to play around in gdb to see what's happening.

The fact that they store the password in a cookie in plaintext is really a nice touch.

Comment 2

11 years ago
Can you still repro this? I'm not seeing it, but I may not be following the steps correctly.

Comment 3

10 years ago
I see what's happening now. We check for a "value" attribute, which is what would be used to prefill during content generation, not the content. Tweaking summary slightly to clarify.
Summary: Keychain can fill in username/password that doesn't match site/cookie-prefilled data → Keychain can fill in username/password that doesn't match data prefilled by JavaScript

Comment 4

10 years ago
Created attachment 314262 [details] [diff] [review]

The fix is trivial; we just need to check the value, which reflects user edits, rather than the value attribute, which doesn't.
Attachment #314262 - Flags: superreview?(mikepinkerton)
Comment on attachment 314262 [details] [diff] [review]

Attachment #314262 - Flags: superreview?(mikepinkerton) → superreview+

Comment 6

10 years ago
Landed on trunk.
Last Resolved: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Camino2.0
You need to log in before you can comment on or make changes to this bug.