Closed Bug 427033 Opened 14 years ago Closed 14 years ago

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

Categories

(Toolkit :: Password Manager, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9

People

(Reporter: jaspermakkinje, Assigned: Dolske)

References

()

Details

(Keywords: regression)

Attachments

(1 file)

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.
Has this worked at one time (like in another beta) and recently stopped working?
Version: unspecified → Trunk
Can you attach some logs of what's happening? See:
http://wiki.mozilla.org/Firefox:Password_Manager_Debugging
@ 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]
@ 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
[...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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: wanted1.9.0.x?
OS: Windows XP → All
Hardware: PC → All
Summary: Firefox offers but does not save login information on http://www.spele.nl/ → Can't save form logins with action="javascript:..."
(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.
[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.
Keywords: regression
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.
Attached patch Patch v.1Splinter Review
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.
Assignee: nobody → dolske
Status: NEW → ASSIGNED
Attachment #314241 - Flags: review?(gavin.sharp)
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.
Flags: wanted1.9.0.x? → blocking-firefox3?
Target Milestone: --- → Firefox 3
Attachment #314241 - Flags: review?(gavin.sharp) → review+
Attachment #314241 - Flags: approval1.9?
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
Flags: blocking-firefox3? → blocking-firefox3-
Attachment #314241 - Flags: approval1.9? → approval1.9+
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
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
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.
Okay, that's fine... Will this be an update for FF2 or FF3? And where can I find it when it's online?
It should be fixed in tonight's nightly FF3 build, and will be included in the final FF3 release.
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...
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.
Duplicate of this bug: 429316
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.