Password manager shows IDN domains with punycode

VERIFIED FIXED in Firefox 57

Status

()

defect
P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: JanH, Assigned: JanH)

Tracking

({regression})

57 Branch
mozilla58
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 unaffected, firefox56 unaffected, firefox57+ verified, firefox58 verified)

Details

Attachments

(2 attachments)

[Tracking Requested - why for this release]: Regression in Login manager UI

Because of bug 1380617, nsUri.prePath now returns the domain name in punycode form, which means that
- logins for IDN domains created on previous Firefox versions (where prePath returned the domain with Unicode characters where allowed) won't work and have to be recreated
- the punycode domain names show up in the UI
Replacing uri.prePath with uri.displayPrePath should do the job.
If we want to do it that way, I can try taking a look at it, since I'm doing the same thing for Android right now anyway.
Assignee: nobody → jh+bugzilla
Comment hidden (mozreview-request)
FWIW, tests are passing (https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c6e8d8ba83fc97a5449fb5e9455575d60e921f1) and a basic check on my Windows and Android builds seems to work as well, although I cannot rule out that I might have overlooked something.
Comment on attachment 8909410 [details]
Bug 1400023 - Switch logins handling to use "display" URIs.

Bouncing to MattN because there's no way I'll get to this quickly.
Attachment #8909410 - Flags: review?(dolske) → review?(MattN+bmo)
Comment on attachment 8909410 [details]
Bug 1400023 - Switch logins handling to use "display" URIs.

https://reviewboard.mozilla.org/r/180612/#review188630

I'm confused (which is why I've taken a while to get to this)… `displayPrePath` seems like the wrong thing to use internally since it's return value will change if the user flips the "network.IDN_show_punycode" pref. It seems like display* should only be used for display purposes.

Can you explain to me why it's correct to change:

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:1378
(Diff revision 1)
>     * Get the parts of the URL we want for identification.
>     * Strip out things like the userPass portion
>     */
>    _getPasswordOrigin(uriString, allowJS) {
>      var realm = "";
>      try {
>        var uri = Services.io.newURI(uriString);
>  
>        if (allowJS && uri.scheme == "javascript")
>          return "javascript:";
>  
>        // Build this manually instead of using prePath to avoid including the userPass portion.
> -      realm = uri.scheme + "://" + uri.hostPort;
> +      realm = uri.scheme + "://" + uri.displayHostPort;

IIRC this gives the result we use for storage. Shouldn't it store a consistent value regardless of the pref? I guess that would mean punycode?

::: toolkit/components/passwordmgr/LoginManagerContextMenu.jsm:95
(Diff revision 1)
>      let searchParams = {
> -      hostname: documentURI.prePath,
> +      hostname: documentURI.displayPrePath,
>        schemeUpgrades: LoginHelper.schemeUpgrades,
>      };
>      let logins = LoginHelper.searchLoginsWithObject(searchParams);
>      let resolveBy = [
>        "scheme",
>        "timePasswordChanged",
>      ];
> -    logins = LoginHelper.dedupeLogins(logins, ["username", "password"], resolveBy, documentURI.prePath);
> +    logins = LoginHelper.dedupeLogins(logins, ["username", "password"], resolveBy, documentURI.displayPrePath);

These are querying storage so I think these shouldn't use an API which can change based on a pref.
Attachment #8909410 - Flags: review?(MattN+bmo)
Assignee

Comment 7

2 years ago
mozreview-review-reply
Comment on attachment 8909410 [details]
Bug 1400023 - Switch logins handling to use "display" URIs.

https://reviewboard.mozilla.org/r/180612/#review188630

You're right that this is not the perfect long-term solution, but short term we'll be no worse off than before bug 1380617 landed, when then non-"display" URI properties exhibited the same behaviour (Unicode or Punycode depending on that pref setting).

Because a full fix means
- separating out all places where a URI ends up user-visible and adding a conversion step (and another one on the way back, e.g. for editing logins)
- dealing with the fact that save for those few users that did flip that pref, all existing logins on IDN domains will have made it into the storage in Unicode form, so we'd have to either make all places where we're comparing something with a URI coming from storage Punycode-insensitive, or else do a database upgrade and convert all existing logins to Punycode

and I don't think I'll have the time to understand the login manager well enough for *that* to still fix this for 57, I chose the simpler path of just mimicking the pre-bug 1380617 behaviour for now. But if so, I guess I should add at least a follow-up bug for the full solution...
^^
Flags: needinfo?(MattN+bmo)
See Also: → 1407146
Comment hidden (mozreview-request)
Flags: needinfo?(MattN+bmo)

Comment 11

2 years ago
Pushed by mozilla@buttercookie.de:
https://hg.mozilla.org/integration/autoland/rev/c479510810b7
Switch logins handling to use "display" URIs. r=MattN
Comment on attachment 8909410 [details]
Bug 1400023 - Switch logins handling to use "display" URIs.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1380617
[User impact if declined]: IDN domains will display with Punycode within the login manager UI and existing logins on IDN domains created before bug 1380617 won't be recognised.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Basic functionality (storing, editing (not on Android because of bug 1400373), form filling from saved logins) tested and found working on Windows and Android, but an additional pair of eyes would be appreciated.
[Needs manual test from QE? If yes, steps to reproduce]: Nothing specific, just make sure that there's nothing unexpected when using the login manager on IDN domains.
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: Lowish.
[Why is the change risky/not risky?]: This basically restores the behaviour pre bug 1380617, where converting an URI to a string automatically used the Unicode form where permissible. While as far as I can tell everything works fine and tests are passing, it is possible that I might have overlooked a code site that additionally needs fixing, though.
[String changes made/needed]: none
Attachment #8909410 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/c479510810b7
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Comment on attachment 8909410 [details]
Bug 1400023 - Switch logins handling to use "display" URIs.

Recent regression, beta57+
Attachment #8909410 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
Posted image domain-beta.png
I have not been able to reproduced this issue. On Firefox nightly 2017.09.14 and on 57.0b14 prefs were set to: network.IDN_show_punycode=false and network.standard-url.punycode-host=true. Using nightly (2017.09.14) and 57.0b14 I see that the password manager page shows IDN domains with unicode on both builds. I attached a screenshot from beta build, please let me know if I am wrong and give me more steps to reproduced this issue.
Flags: needinfo?(jh+bugzilla)
Not sure if that is causing the confusion, but the problems is only visible for saved logins created after bug 1380617 landed.

STR on Nightly:
1. Visit http://äöü.buttercookie.de/temp/formtest.html
2. Domain should display with umlauts in the address bar
3. Enter anything into the form, hit "Submit" and accept the the login manager's prompt to save the data.
4. Go into the options to look at your Saved Logins.
5. The domain is displayed as http://xn--4ca0bs.buttercookie.de
Flags: needinfo?(jh+bugzilla)
Thanks! I managed to reproduced this issue on nightly (build ID=20170914220209) on Win 8.1 x64. I can confirm this issue is fixed, I verified using Firefox 57.0 (build ID=20171102181127) and latest nightly build 58.0a1 on Windows 8.1 x64, Ubuntu 14.04 x64 and Mac OS X 10.12.6.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.