427033 - Can't save form logins with action="javascript:..."

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect)

Description

[Jasper Makkinje]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; nl; rv:1.9b5) Gecko/2008032620 Firefox/3.0b5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; nl; rv:1.9b5) Gecko/2008032620 Firefox/3.0b5

If I enter my username and password for the first time, the Password Manager-bar does come up, but the "Remember Password" button doesn't work; nothing happens when you click it. The other buttons, "Not now" and "Never for this website" do work, and do make the bar disappear.

The password really isn't saved, it's not in the password manager menu, and when I remove my cookies, I'm logged out of the website and it doesn't prompt my username when I start typing it in the Login-field.

Reproducible: Always

Steps to Reproduce:
1. Go to http://www.spele.nl/
2. Create an account
3. Log in to your account, and try to save the password.
Actual Results:  
Nothing, the "Remember Password" button doesn't seem to do anyting at all.

Expected Results:  
The bar should have disappeared, and my username and password should have been remembered the next time I visited this site.

Comment 1

[u88484]

Has this worked at one time (like in another beta) and recently stopped working?

Comment 2

[Justin Dolske [:Dolske]]

Can you attach some logs of what's happening? See:
http://wiki.mozilla.org/Firefox:Password_Manager_Debugging

Comment 3

[Jasper Makkinje]

@ Kurt Schultz: I don't remember trying it in Firefox 2, I'll install it and try...

@ Justin Dolske:

Hmmm, I did find something strange in the logs here:
[list]
[*]Login Manager: observer notified for form submission.
[*]Login Manager: Couldn't parse origin for javascript:inloggen(document.getElementById('gebruikersnaam').value,document.getElementById('wachtwoord').value,'NL');
[*]Login Manager: Checking if logins to http://www.spele.nl can be saved.
[*]Login Manager: Searching for logins matching host: http://www.spele.nl, formSubmitURL: null, httpRealm: null
[*]Pwmgr Prompter: ===== initialized =====
[*]Pwmgr Prompter: Adding new password-save notification bar
[*]Login Manager: Saving logins for http://www.spele.nl enabled? false
[*]PwMgr Storage: Writing passwords to C:\Documents and Settings\Jasper\Application Data\Mozilla\Firefox\Profiles\ek5f6ip9.default\signons3.txt
[/list]

Comment 4

[Jasper Makkinje]

@ Kurt Schultz: I don't remember trying it in Firefox 2, I'll install it and
try...

@ Justin Dolske:

Hmmm, I did find something strange in the logs here:
<UL>
<LI>Login Manager: observer notified for form submission.
<LI>Login Manager: Couldn't parse origin for
javascript:inloggen(document.getElementById('gebruikersnaam').value,document.getElementById('wachtwoord').value,'NL');
[*]Login Manager: Checking if logins to http://www.spele.nl can be saved.
<LI>Login Manager: Searching for logins matching host: http://www.spele.nl,
formSubmitURL: null, httpRealm: null
<LI>Pwmgr Prompter: ===== initialized =====
<LI>Pwmgr Prompter: Adding new password-save notification bar
<LI>Login Manager: Saving logins for http://www.spele.nl enabled? false
<LI>PwMgr Storage: Writing passwords to C:\Documents and
Settings\Jasper\Application
Data\Mozilla\Firefox\Profiles\ek5f6ip9.default\signons3.txt
</UL>

EDIT: Oops, no BBCode, I guess :P

Comment 5

[Justin Dolske [:Dolske]]

[...No HTML, either. :-)]

Ah, yes, this is a bug.

The login page uses <form action="javascript:inloggen(...)"> for the login form. We're trying to save the hostname for where the login will be sent (good ole bug 360493), can't parse the URL, and thus set newLogin.formSubmitURL to null. When we try to save the login (when you click "Remember"), addLogin() will throw because null isn't a legal value for the formSubmitURL.

Possible fixes:

* Treat a javascript URL as a wildcard (newLogin.formSubmitURL = ""). This would be the most flexible, and would allow the login to form if a site has other login forms with a hardcoded action URL. OTOH, this would mean the site's users have greater exposure to the XSS attack described in bug 360493.

* Save the URL in a special format (newLogin.formSubmitURL = "javascript:"). This would mitigate the XSS concerns (because if an attacker can inject a JS URL into the form action, the user is screwed anyway). Downside is it's not quite as flexible (see above). I'd also want to thing through any tricky security issues with that.

I'm leaning towards the latter.

Comment 6

[Justin Dolske [:Dolske]]

(In reply to comment #1)
> Has this worked at one time (like in another beta) and recently stopped
> working?

This shouldn't have worked since the new password manager landed a year ago [so I think this isn't blocking-ff3, but I'd like to fix it for FF 3.0.1]. Not sure what FF2 did -- could you test? It would be good to know what it saves, and see if we migrate old logins correctly.

Comment 7

[Jasper Makkinje]

[Oops, I didn't notice HTML didn't work either... :-) ]

Anyway, I just tested it in Firefox 2.0.0.13, and it DOES work there, no problems at all.

Comment 8

[Justin Dolske [:Dolske]]

Interesting. It's not supposed to work in FF2 either! When I test this locally (on FF2), the login I end up with has a form action origin of "javascript://javascript". This happens because of a bug in GetPasswordRealm()...

http://mxr.mozilla.org/mozilla1.8/source/toolkit/components/passwordmgr/base/nsPasswordManager.cpp#2172

aURI->GetHostPort() fails, but the code only checks for a failure by looking for an empty buffer. Unfortunately, the buffer is being reused, and still contains the previous value, so the failure isn't caught. Doh!

I think the original security problem this was for isn't a problem anymore, so this is mostly harmless... The testcase in bug 159484 tries to create a tricky-looking URI by doing something akin to:

var www = {};
www.tricky = {};
www.tricky.org = {};
location = "javascript:www.tricky.org";

The ancient wallet code could apparently be convinced to save a login for "www.tricky.org" that way, but we run everything though nsIURI.newURI now, which isn't fooled by that.

We should probably go ahead and fix this, if only to make sure that we handle from signons.txt upgrade correctly.

Comment 9

[Justin Dolske [:Dolske]]

Attachment: Patch v.1

Basic approach here is simple and safe... When parsing something that's a form |action|, add a flag to allow "javascript:".

A more liberal patch would be to allow any protocol, but I'm not sure if there are other use-cases where this is both useful and safe.

Comment 10

[Justin Dolske [:Dolske]]

Renomming for blocking, since this is a regression. It wasn't supposed to work before, but people might be using it. Patch is simple and safe, and has -- omgomg -- tests.

Comment 11

[Mike Beltzner [:beltzner, not reading bugmail]]

I don't think this blocks (as in, I'd back it out if it caused regressions) but tests and reviews result in a=beltzner

Comment 12

[Justin Dolske [:Dolske]]

Checking in toolkit/components/passwordmgr/src/nsLoginManager.js;
  new revision: 1.28; previous revision: 1.27
Checking in toolkit/components/passwordmgr/src/storage-Legacy.js;
  new revision: 1.26; previous revision: 1.25
Checking in toolkit/components/passwordmgr/test/Makefile.in;
  new revision: 1.15; previous revision: 1.14
Checking in toolkit/components/passwordmgr/test/test_bug_360493_1.html;
  new revision: 1.6; previous revision: 1.5
Checking in toolkit/components/passwordmgr/test/test_bug_427033.html;
  initial revision: 1.1
Checking in toolkit/components/passwordmgr/test/unit/test_storage_legacy_1.js;
  new revision: 1.10; previous revision: 1.9
Checking in toolkit/components/passwordmgr/test/unit/data/signons-427033-1.txt;
  initial revision: 1.1

Comment 13

[u88484]

Jasper: Could you download tomorrow's nightly build and see if this works for you now?  If so mark this bug as verified and add a comment saying which build id you used to verify.

Comment 14

[Jasper Makkinje]

Okay, that's fine... Will this be an update for FF2 or FF3? And where can I find it when it's online?

Comment 15

[Justin Dolske [:Dolske]]

It should be fixed in tonight's nightly FF3 build, and will be included in the final FF3 release.

Comment 16

[:Gavin Sharp [email: [e-mail]]]

Jasper: you can use a build from ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-trunk/ to test.

Comment 17

[Jasper Makkinje]

Hmm, I downloaded the Nightly Build and tried it, and kinda works, but not really:

It does save my password and username, it's in the list of saved passwords, but when I visit the site later, after removing my cookies, it still doesn't "fill in" my username and password, so I have to type them again...

Comment 18

[Justin Dolske [:Dolske]]

It seems the page is adding the loging form dynamically with javascript... The password manager only fills in logins when the page first loads, and the form not there yet. That's bug 355063.

Sorry this didn't entirely solve your problem.