328248 - JavaScript prevents saving passwords in the Keychain

Status: RESOLVED FIXED

(Camino Graveyard :: OS Integration, defect)

Description

[Shane Fenton]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.8) Gecko/20050511 Firefox/1.0.4
Build Identifier: http://www.mozilla.org/projects/camino/ [Version 2006021400 (1.0)]

I've  been unable to get autofill to work on howardforums.com, macosx.com/forums, and several other forums. It is specifically on forum sites.

Reproducible: Always

Steps to Reproduce:
1.Go to howardforums.com, and attempt to login and allow name/password save to keychain, then return to login.
2.Go to macosx.com/forums, and attempt to login and allow name/password save to keychain, then return to login.

3.

Actual Results:  
You have to login manually

Expected Results:  
Keychain/autofill fills it in

Comment 1

[Chris Lawson (gone)]

Both the links you provided are vBulletin-powered fora, which indicates to me that vBulletin is doing something that prevents our "detection" of username and password fields from working.

I'm tempted to call this tech evangelism or invalid, but I'll let some other folks take a look at it.

cl

Comment 2

[Stuart Morgan]

I can't get it to save passwords at all (unless I turn off JavaScript, at which point the form-fill works fine). When the form is submitted some JavaScript is running that takes the password, md5-hashes it, sticks the result in a hidden form field, and deletes the password from the password field.  By the time Camino's password-saving code has a chance to run, there is no password to save, so it gives up.  There's really nothing that can be done at the browser end if the page is erasing form data.

Comment 3

[Stuart Morgan]

Firefox actually fixed this case in bug 257781, so maybe we can get hooks in earlier at the point they (I haven't looked yet to see if it's a hook embedders have access to). Reopening for investigation based on that.

Comment 4

[Mardeg]

Greasemonkey workaround script from Mozillazine thread:
"I have identified the nasty bit of code responsible for this issue. It's in the vBulletin backwards compatibility engine, they had to leave in this automatic form reset for some reason (it just says "for backwards compatibility with old forum templates"). And although you are able to disable javascript and thereby login and save the password, I find that troublesome (going into Options and disabling it, then going back and enabling it). So instead, I decided to use Greasemonkey to inject a fix that bypasses the form clearing.

Get it from here: http://userscripts.org/scripts/show/8021

"It works seamlessly and transparently on all forums. And in case their particular url (the forum) does not have *forum* in their name, you just have to add it to the list of included sites."

Comment 5

[Stuart Morgan]

Greasemonkey is a Firefox extension; this is a Camino bug.

Comment 6

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Mass-reassign of bugs still assigned to pinkerton to nobody; filter on "NoMoPinkBugsInCamino".

Comment 8

[striovatti]

Worked for me:

Disable Javascript once and enter your username and password.

Thank you Mardeg!

Comment 10

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #3)
> Firefox actually fixed this case in bug 257781, so maybe we can get hooks in
> earlier at the point they (I haven't looked yet to see if it's a hook embedders
> have access to). Reopening for investigation based on that.

It's more complex than bug 257781 (given that bug 257781 just changed something in content/html/content/src/nsHTMLFormElement.cpp).

What happened is that because of regressions like bug 343182 (and bug 343264, which was a regression from fill-during-pageload, which landed at the same time), they started listening for NS_EARLYFORMSUBMIT_SUBJECT instead of NS_FORMSUBMIT_SUBJECT.

In simplest terms, that's a find/replace in http://mxr.mozilla.org/camino/source/camino/src/formfill/KeychainService.mm#137

However, bz warns in bug 343182 comment 6 and bug 343182 comment 14 about the action URI getting lost/being wrong.  I'm not sure how Firefox may have compensated for this when fixing bug 360493 (aka our bug 361463 and friends).  The Firefox current code still listens for NS_EARLYFORMSUBMIT_SUBJECT (or, rather, its JS equivalent, earlyformsubmit), but Stuart would have to look at the patch in bug 360493 to see if any compensation is needed for the action URL.

Aside from that, though, this looks like a trivial fix :P

Comment 11

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(If it does turn out to be that easy, or even just slightly more complex, this would be a good thing to get for 2.1 and for whatever branch release is current when we feel comfortable with the code.)

Comment 12

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #10)
> time), they started listening for NS_EARLYFORMSUBMIT_SUBJECT instead of
> NS_FORMSUBMIT_SUBJECT.
> 
> In simplest terms, that's a find/replace in
> http://mxr.mozilla.org/camino/source/camino/src/formfill/KeychainService.mm#137

FWIW, when I make that change, I successfully save a password at macosx.com, with 
	<key>macosx.com</key>
	<array>
		<string>macosx.com</string>
	</array>
as my AllowedActionHosts.plist <dict>.

Comment 13

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Doesn't appear to fix the NYU case (bug 457590), but

1) that appears to have changed, since disabling JS doesn't work now, either and 

2) Firefox doesn't work, either

(at least with dummy login info, which got me a prompt on both macosx.com and tvcampus.net).

Also, I notice we're passing PR_FALSE, whereas Firefox (and every other place bug 343182 patched) was using PR_TRUE; I have no idea if that's important, but I noticed it was different.

Comment 14

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Fix

Stuart had a look at this this morning, and his conclusion was that the action URI might be wrong on pages with JS submission hooks, but we only care about the action URI (host) for security, and that whole security model only works when the attacker can't run JS on the page.

So, it really is this simple.

Comment 15

[Stuart Morgan]

Comment on attachment 482278 [details] [diff] [review]
Fix

sr=smorgan

Comment 16

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

http://hg.mozilla.org/camino/rev/56b2d55697b8

I'm a bit wary about taking this on 2.0.x right before a release, in the event there's some case we've missed or misunderstood.

Comment 17

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

> I'm a bit wary about taking this on 2.0.x right before a release, in the event
> there's some case we've missed or misunderstood.

There was agreement with this sentiment at the meeting.  We'll renominate for 2.0.6 and look at it then when it will have been in use in the nightlies for a while.

Comment 18

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Per irc discussions, no one's seen/complained about regressions for the ~month this has been in 2.1a1pre nightlies, so we're taking this for 2.0.6.

Landed on CAMINO_2_0_BRANCH in advance of 2.0.6.