Closed Bug 451155 Opened 12 years ago Closed 12 years ago

Password manager does not work correctly on IDN site whose name contains any character over U+0100


(Toolkit :: Password Manager, defect)

1.9.0 Branch
Not set





(Reporter: dev-null, Assigned: Dolske)




(4 keywords, Whiteboard: [MU?])


(2 files, 1 obsolete file)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; ja; rv:1.9.1a2pre) Gecko/20080818113442 Minefield/3.1a2pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv: Gecko/2008070208 Firefox/3.0.1

Password manager does not work correctly on IDN site (for example, Japanese domain name) whose name contains any character over U+0100.

Reproducible: Always

Steps to Reproduce:
1. Start Fx2.0.0.16.
2. Go to the URL (and accept security error).
3. Enter username and password.
4. Click ログイン button to submit.
5. Click Remember button in Confirm dialog.
6. Upgrade to Fx3.0.1.
7. Go to the URL again (and add exeption).

Actual Results:  
Username and password are NOT filled in automatically.

Expected Results:  
Username and password SHOULD BE filled in automatically.

The Site is garbled in
Tools -> Options -> Security -> Saved Passwords
after upgrade to Fx3.
Keywords: regression
Whiteboard: [MU?]
Version: unspecified → 1.9.0 Branch
Ugh. I looked into IDN issues back in bug 396316, and they seemed to work fine. I suspect the testcase I used was insufficient -- the UCS2 representation of the non-ascii characters was probably still U+00xx.

The storage-Legacy.js module probably needs to use nsIScriptableUnicodeConverter (as it already does in _encrypt()/_decrypt() for usernames and passwords) for every string that might be written to disk. This would mean hostname, formSubmitURL, httpRealm, and usernameField/passwordField. Oh, and the disabled-hosts list too.

One nice thing: this bug is already fixed on 3.1, as a result of switching to mozStorage. Yay!

Also, for the hostname/formSubmitURL, it would probably be best to store as punycode version (which should just be plain ascii), and convert to IDN in the UI. This keeps the stored value the same, even when the IDN whitelist changes -- see bug 400097. Would still need to support extensions using arbitrary UCS2 strings, though. Changing the format could break existing stored IDN logs (when they are not already broken by this bug), so this might incur a storage file version bump or DB scheme version bump. Probably best to spin these issues off into a separate bug...
Assignee: nobody → dolske
Attached patch Patch v.1 (obsolete) — Splinter Review
Passes existing tests, need to add tests to see if it actually fixes this problem. :)
Target Milestone: --- → mozilla1.9.1a2
Requesting blocking because this should be fixed before major update. Otherwise, stored password for some site will be corrupted.

(In reply to comment #2)
> Created an attachment (id=334422) [details]
> Patch v.1

This patch works fine for me with form and also with HTTP authentication.
Flags: blocking1.9.0.3?
Flags: blocking1.9.0.2?
Attached patch Patch v.2Splinter Review
Same fix, added tests.

I also modified the mozStorage module with the move of nsIScriptableUnicodeConverter into a getter... It's ok without the move, but I think this will be a performance win -- we're not creating an instance of it every time we decrypt a string.
Attachment #334422 - Attachment is obsolete: true
Attachment #334548 - Flags: review?(
Adding "dataloss" keyword because stored data is garbled and can not be recovered, even if this bug is fixed the after data is garbled.
Keywords: dataloss
We need to block on this. Gavin, can you review this patch asap?
Flags: blocking1.9.0.3?
Flags: blocking1.9.0.2?
Flags: blocking1.9.0.2+
Comment on attachment 334548 [details] [diff] [review]
Patch v.2

>+ * UCS2 characteres above U+00FF.

comentarios en inglés, por favor (dos casos)
Attachment #334548 - Flags: review?( → review+
I'm not sure this is reaaaaally a blocker, just because it's been broken since the new password manager landed back in May 2007, and this is the first report. But in any case, I'm happy to fix it ASAP. :)
Pushed changeset 442ac72e88f1.
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: mozilla1.9.1a2 → mozilla1.9.1
Attachment #334548 - Flags: approval1.9.0.2?
Comment on attachment 334548 [details] [diff] [review]
Patch v.2

Approved for Please land in CVS. a=ss
Attachment #334548 - Flags: approval1.9.0.2? → approval1.9.0.2+
Checked in on FF3.0 branch:

Checking in toolkit/components/passwordmgr/src/storage-Legacy.js;
  new revision: 1.28; previous revision: 1.27
Checking in toolkit/components/passwordmgr/test/unit/test_storage_legacy_3.js;
  new revision: 1.7; previous revision: 1.6
Keywords: fixed1.9.0.2
Duplicate of this bug: 453098
We need a username and password to verify this fix or the reporter needs to do so for 3.02.
(In reply to comment #13)
> We need a username and password to verify this fix or the reporter needs to do
> so for 3.02.

Verified with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv: Gecko/2008090213 Firefox/3.0.2.

I don't know username and password either.
But password can be stored to password manager even if it's wrong.
Ah, that makes sense. Marking it as verified.
Depends on: 454708
Depends on: 454993
This caused a regression in Firefox 3.0.2 - see bug 454708
No longer depends on: 454993
Depends on: 454993
No longer depends on: 454993
relnote: passwords entries of this type created in the broken builds (3.0, 3.0.1) will not be corrected in 3.0.3, they will have to be created and saved again by the user. The invalid entry will be there in PWM and should probably also just be removed by the user.
Keywords: relnote
You need to log in before you can comment on or make changes to this bug.