Closed Bug 1391421 Opened 7 years ago Closed 7 years ago

Fix unicode domain support regressions

Categories

(Firefox for Android Graveyard :: General, defect)

Product:
Firefox for Android Graveyard
Component:
General
Version:
57 Branch
Platform:
All
Android
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(fennec+, firefox55 unaffected, firefox56 unaffected, firefox57 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 57
Tracking Flags:
Tracking Status
fennec + ---
firefox55 --- unaffected
firefox56 --- unaffected
firefox57 --- verified

People

(Reporter: mcomella, Assigned: JanH)

References

Details

(Keywords: regression)

Attachments

(9 files, 1 obsolete file)

Bug 1391421 - Part 1 - Switch nsAndroidHistory to Unicode domains.
59 bytes, text/x-review-board-request
esawin
: review+
Details
Bug 1391421 - Part 2 - Make script dialogues use the Unicode domain as title.
59 bytes, text/x-review-board-request
baku
: review+
Details
Bug 1391421 - Part 3 - Switch various places that can end up being user-visible to use Unicode domains.
59 bytes, text/x-review-board-request
esawin
: review+
jwu
: review+
Details
Bug 1391421 - Part 4 - Switch Session Store to save the "display" URL.
59 bytes, text/x-review-board-request
mikedeboer
: review+
Details
Bug 1391421 - Part 5 - Normalise the saved "appOrigin" to Unicode.
59 bytes, text/x-review-board-request
jwu
: review+
Details
Bug 1391421 - Part 6 - Switch context menus to Unicode domains.
59 bytes, text/x-review-board-request
jwu
: review+
Details
Bug 1391421 - Part 7 - Switch addon/theme install prompts to Unicode domains.
59 bytes, text/x-review-board-request
jwu
: review+
Details
Bug 1391421 - Part 8 - Fix site identity handling.
59 bytes, text/x-review-board-request
jwu
: review+
Details
Bug 1391421 - Part 9 - Add a basic Robocop test for IDN domain support.
59 bytes, text/x-review-board-request
gbrown
: review+
Details 
STR:
0) With photon fennec
1) visit http://faß.de

Expected: url bar displays ^
Actual: url bar displays "xn--fa-hia.de"

In Firefox Beta, this works as expected.

We should consider adding some tests so this support doesn't regress again in the future.
This looks like it might be a change in gecko: see bug 1391422 comment 9 (that bug is for unicode domains appearing as punycode in top sites).
Hi Jing Wei
Please help take a look. Thanks!
Flags: needinfo?(wehuang)
Flags: needinfo?(topwu.tw)
Keywords: regression
FYI: I'm trying to get help from snorp in bug 1391422 so unless someone here is already experienced with gecko, it may not be worth duplicating our efforts.
we should prioritize this one in 57.3, added to tomorrow's planning doc.
Flags: needinfo?(wehuang)
FYI: the related bug 1391422 is low priority against the other AS bugs so I've unassigned. This seems to have been caused by gecko changes and the next recommended step is to identify a regression window.

Kevin, could someone determine a regression window on this bug? It might be related to the photon changes but it might not be (as determined in bug 1391422).
Flags: needinfo?(kbrosnan)
Keywords: regressionwindow-wanted
Whiteboard: [FNC][SPT57.3][BL]
Bogdan, if you don't have too much on your plate, could you determine the regression window from comment 5? It's related to AS (as bug 1391422) but it's the lowest possible priority to finish by the 57 merge so focus on other things first! :)
Flags: needinfo?(bogdan.surd)
Hello,

Here is that regression-window:
 Last Known Good Build: 2017-08-09
 First Known Bad Build: 2017-08-10
 Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=4c5fbf49376351679dcc49f4cff26c3c2e055ccc&tochange=4d54ac07b8c97f0e6713dab2ba694023b5b2f3b5
Flags: needinfo?(bogdan.surd)
Keywords: regressionwindow-wanted
Looks like this change was made intentionally as part of 1380617. I guess Firefox displays unicode by accessing uri.displaySpec instead of .spec. Jing Wei, you should be able to make a similar change in browser.js if you want similar behavior.
Flags: needinfo?(kbrosnan)
FYI: I'm not sure what the proper way to handle this is because bug 1397842 was filed and describes the case where:
- www.аррӏе.com/ could be a scam site
- www.apple.com/ is the real Apple

but they look identical here and in the url bar on desktop. How do we do this on desktop and why? Is it the right approach?

---

Affected/unaffected flags set based on the fact that bug 1380617, the regressing bug, landed in Firefox 57.
status-firefox56: --- → unaffected
status-firefox57: --- → affected
See Also: → 1397842
Summary: Photon url bar has regressed support for unicode domains → URL bar has regressed support for unicode domains
(In reply to Michael Comella (:mcomella) from comment #9)
> but they look identical here and in the url bar on desktop. How do we do
> this on desktop and why? Is it the right approach?

The scam apple.com links to a blog post about this issue [1], which links to bug 1332714 where it was discussed on desktop. The blog post quotes the bug:

(In reply to Gervase Markham [:gerv] from bug 1332714 comment #5)
> (In reply to Simon Montagu :smontagu from comment #4)
> > https://www.аррӏе.com/ is an example of a whole-script homograph, which our
> > IDN display code is not designed to protect against -- for example
> > https://www.асе.com/ spoofs https://www.ace.com/
> 
> Indeed. Our IDN threat model specifically excludes whole-script homographs,
> because they can't be detected programmatically and our "TLD whitelist"
> approach didn't scale in the face of a large number of new TLDs. If you are
> buying a domain in a registry which does not have proper anti-spoofing
> protections (like .com), it is sadly the responsibility of domain owners to
> check for whole-script homographs and register them.
> 
> We can't go blacklisting standard Cyrillic letters.

Which is unfortunate because end users are still vulnerable. I wonder if we can do better here?

[1]: https://www.xudongz.com/blog/2017/idn-phishing/
already in our plan so t+
Assignee: nobody → topwu.tw
tracking-fennec: ? → +
Joe, I think we'll need a direction from Product for this. Should we

1. simply fix this bug mentioned in comment 0 with the concern pointed out by comment 9, which is the same as Desktop's today?

2. have overall design including UX consideration to lower the risk of Phishing URL. (maybe need to align w/ Desktop's solution, once they have)

Maybe to adapt (1) for now, and monitoring 1332714 to make a mobile-version solution in the future?
Flags: needinfo?(jcheng)
agree with your assessment on comment 12 but let's make sure we have a follow up bug to track point #2
Flags: needinfo?(jcheng)
The patch only converts the URL content from ASCII Compatible Encoding (ACE) to Unicode, and the idea is based on 2 factors:

1. I would like to keep the patch as simple as possible because we might need some visual redesign on URL bar in near future to prevent IDN Phishing discussed in bug 1332714.
2. I'm not familiar with the logic in browser.js, I don't know if there is any side effect from replacing `uri.spec` with `uri.displaySpec`.
Flags: needinfo?(topwu.tw)
Comment on attachment 8907438 [details]
Bug 1391421 - Translates URL from ASCII Compatible Encoding (ACE) to Unicode.

https://reviewboard.mozilla.org/r/179130/#review184254

There are more places where we're handling URLs than just the URL bar.
Won't this mean that e.g. we're now still storing punycode domains into our history/bookmarks/... DB tables, whereas previously we stored the Unicode URL?
This also e.g. leaves our "Switch to tab" functionality broken. I haven't looked how that works, but it probably takes the URL bar user input as it and then searches through all open tabs, which won't work if those store their URL as Punycode.

As a quick fix, I think we should rather override ["network.standard-url.punycode-host"](https://dxr.mozilla.org/mozilla-central/rev/f9a5e9ed62103c84e4cde915f4d08f1ce71be83e/modules/libpref/init/all.js#2102) to `false` to restore the previous behaviour for now and then think this through carefully which places need the `spec` and which ones the `displaySpec`
Attachment #8907438 - Flags: review-
Blocks: 945240
(In reply to Jan Henning [:JanH] from comment #16)
> Comment on attachment 8907438 [details]
> Bug 1391421 - Translates URL from ASCII Compatible Encoding (ACE) to Unicode.
> 
> https://reviewboard.mozilla.org/r/179130/#review184254
> 
> There are more places where we're handling URLs than just the URL bar.
> Won't this mean that e.g. we're now still storing punycode domains into our
> history/bookmarks/... DB tables, whereas previously we stored the Unicode
> URL?
> This also e.g. leaves our "Switch to tab" functionality broken. I haven't
> looked how that works, but it probably takes the URL bar user input as it
> and then searches through all open tabs, which won't work if those store
> their URL as Punycode.
> 
> As a quick fix, I think we should rather override
> ["network.standard-url.punycode-host"](https://dxr.mozilla.org/mozilla-
> central/rev/f9a5e9ed62103c84e4cde915f4d08f1ce71be83e/modules/libpref/init/
> all.js#2102) to `false` to restore the previous behaviour for now and then
> think this through carefully which places need the `spec` and which ones the
> `displaySpec`

This behaviour is web-observable. Flipping the pref would mean Firefox on desktop and android behave differently. That doesn't seem desirable to me... See also bug 1380617 where we fixed the desktop UI to deal with this.
Actually, the regression started when we flipped the pref in bug 1380617.

(In reply to Jan Henning [:JanH] from comment #16)
> As a quick fix, I think we should rather override
> ["network.standard-url.punycode-host"](https://dxr.mozilla.org/mozilla-
> central/rev/f9a5e9ed62103c84e4cde915f4d08f1ce71be83e/modules/libpref/init/
> all.js#2102) to `false` to restore the previous behaviour for now and then
> think this through carefully which places need the `spec` and which ones the
> `displaySpec`

This would negate the web-compat benefits we get from bug 1380617.
The correct fix would be to use displaySpec. I also suggest adding unit tests to make sure we don't regress this.
Blocks: 1380617
No longer blocks: 945240
(In reply to :Gijs from comment #17)
> This behaviour is web-observable. Flipping the pref would mean Firefox on
> desktop and android behave differently. That doesn't seem desirable to me...
> See also bug 1380617 where we fixed the desktop UI to deal with this.

Yes, I didn't mean that as a long-term solution, just that if we absolutely want a super-quick fix for our UI, temporarily flipping that pref back for mobile for one release is in my opinion still better than an incomplete workaround in our Java UI code.
But if we can get the spec/displaySpec situation sorted out in our JS code in time for 57 then that's of course the better solution.
Comment on attachment 8907438 [details]
Bug 1391421 - Translates URL from ASCII Compatible Encoding (ACE) to Unicode.

https://reviewboard.mozilla.org/r/179130/#review184276
Attachment #8907438 - Flags: review?(cnevinchen) → review+
Comment on attachment 8907438 [details]
Bug 1391421 - Translates URL from ASCII Compatible Encoding (ACE) to Unicode.

https://reviewboard.mozilla.org/r/179130/#review184278
Attachment #8907438 - Flags: review+
Attachment #8907438 - Flags: review?(s.kaspari)
Stealing this from you - I need to check one or two edge cases, but other than that, I've looked through our usage of uri.spec
Assignee: topwu.tw → jh+bugzilla
Attachment #8907438 - Attachment is obsolete: true
See Also: → 1400023
See Also: → 1400033
(In reply to Jan Henning [:JanH] from comment #22)
> I need to check one or two edge cases

Such a statement is of course tempting fate, because after actually building and running my initial patch, a number of additional locations where Punycode usage or not matters have promptly come up. But I'm working my way through them...
Summary: URL bar has regressed support for unicode domains → Fix unicode domain support regressions
Version: unspecified → 57 Branch
Blocks: 1400544
Comment on attachment 8909414 [details]
Bug 1391421 - Part 1 - Switch nsAndroidHistory to Unicode domains.

https://reviewboard.mozilla.org/r/180576/#review186108
Attachment #8909414 - Flags: review?(esawin) → review+
Comment on attachment 8909416 [details]
Bug 1391421 - Part 3 - Switch various places that can end up being user-visible to use Unicode domains.

https://reviewboard.mozilla.org/r/180590/#review186116

LGTM, although I'm missing some context here, what are the remaining use cases for .spec?

::: mobile/android/chrome/content/browser.js:3586
(Diff revision 1)
>      // Only set tab uri if uri is valid
>      let uri = null;
> +    let uriSpec = null;
>      let title = aParams.title || aURL;
>      try {
> -      uri = Services.io.newURI(aURL).spec;
> +      uri = Services.io.newURI(aURL);

Do we need uri and uriSpec? Why adding the new variable?
Attachment #8909416 - Flags: review?(esawin) → review+
Comment on attachment 8909416 [details]
Bug 1391421 - Part 3 - Switch various places that can end up being user-visible to use Unicode domains.

https://reviewboard.mozilla.org/r/180590/#review186116

> Do we need uri and uriSpec? Why adding the new variable?

Hmm yes, that probably only makes sense in conjunction with [Part 5](https://reviewboard.mozilla.org/r/180586/diff/1#index_header). Although that in turn is debatable as well, since I guess I could equally settle on normalising the stored appOrigin as Unicode (i.e. with displaySpec). Since that would be the only use of spec here (and therefore the only reason to keep the parsed Uri around), I guess I should change that bit, too.
Comment on attachment 8909416 [details]
Bug 1391421 - Part 3 - Switch various places that can end up being user-visible to use Unicode domains.

https://reviewboard.mozilla.org/r/180590/#review186116

I guess either if you explicitly want Punycode, or else probably mostly a case of not touching things if not necessary because - as far as I could tell - they don't end up in the UI, because we're e.g. just getting the spec to check whether it looks like a reader-mode url, starts with "about:", etc., or we're just taking the hosts of two URIs and comparing them with each other and things like that.
Comment on attachment 8909415 [details]
Bug 1391421 - Part 2 - Make script dialogues use the Unicode domain as title.

https://reviewboard.mozilla.org/r/180578/#review186210
Attachment #8909415 - Flags: review?(amarchesini) → review+
Comment on attachment 8909416 [details]
Bug 1391421 - Part 3 - Switch various places that can end up being user-visible to use Unicode domains.

https://reviewboard.mozilla.org/r/180590/#review186294
Attachment #8909416 - Flags: review?(topwu.tw) → review+
Comment on attachment 8909418 [details]
Bug 1391421 - Part 5 - Normalise the saved "appOrigin" to Unicode.

https://reviewboard.mozilla.org/r/180586/#review186304
Attachment #8909418 - Flags: review?(topwu.tw) → review+
Comment on attachment 8909419 [details]
Bug 1391421 - Part 6 - Switch context menus to Unicode domains.

https://reviewboard.mozilla.org/r/180588/#review186306
Attachment #8909419 - Flags: review?(topwu.tw) → review+
Comment on attachment 8909420 [details]
Bug 1391421 - Part 7 - Switch addon/theme install prompts to Unicode domains.

https://reviewboard.mozilla.org/r/180594/#review186308
Attachment #8909420 - Flags: review?(topwu.tw) → review+
Comment on attachment 8909421 [details]
Bug 1391421 - Part 8 - Fix site identity handling.

https://reviewboard.mozilla.org/r/180596/#review186310
Attachment #8909421 - Flags: review?(topwu.tw) → review+
Comment on attachment 8909417 [details]
Bug 1391421 - Part 4 - Switch Session Store to save the "display" URL.

https://reviewboard.mozilla.org/r/180580/#review186470
Attachment #8909417 - Flags: review?(mdeboer) → review+
Whiteboard: [FNC][SPT57.3][BL]
Comment on attachment 8909422 [details]
Bug 1391421 - Part 9 - Add a basic Robocop test for IDN domain support.

https://reviewboard.mozilla.org/r/180598/#review187154

::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/testIdnSupport.java:10
(Diff revision 2)
> +
> +public class testIdnSupport extends UITest {
> +    public void testToolbarIdnSupport() {
> +        GeckoHelper.blockForReady();
> +
> +        mBaseHostnameUrl = "http://exämple.test/tests";

Won't this hit the non-local-connections assertion when run in automation?

Do you have results from try?
(In reply to Geoff Brown [:gbrown] from comment #51)
> Comment on attachment 8909422 [details]
> Bug 1391421 - Part 9 - Add a basic Robocop test for IDN domain support.
> 
> https://reviewboard.mozilla.org/r/180598/#review187154
> 
> :::
> mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/
> testIdnSupport.java:10
> (Diff revision 2)
> > +
> > +public class testIdnSupport extends UITest {
> > +    public void testToolbarIdnSupport() {
> > +        GeckoHelper.blockForReady();
> > +
> > +        mBaseHostnameUrl = "http://exämple.test/tests";
> 
> Won't this hit the non-local-connections assertion when run in automation?

exämple.test should be OK
http://searchfox.org/mozilla-central/rev/d08b24e613cac8c9c5a4131452459241010701e0/build/pgo/server-locations.txt#135
Comment on attachment 8909422 [details]
Bug 1391421 - Part 9 - Add a basic Robocop test for IDN domain support.

https://reviewboard.mozilla.org/r/180598/#review187178

::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/testIdnSupport.java:10
(Diff revision 2)
> +
> +public class testIdnSupport extends UITest {
> +    public void testToolbarIdnSupport() {
> +        GeckoHelper.blockForReady();
> +
> +        mBaseHostnameUrl = "http://exämple.test/tests";

From irc:

<JanH|mobile> gbrown: I've picked the names from there [list of allowed server aliases], and yes, it works on Try
Attachment #8909422 - Flags: review?(gbrown) → review+
Pushed by mozilla@buttercookie.de:
https://hg.mozilla.org/integration/autoland/rev/c9bfc364426e
Part 1 - Switch nsAndroidHistory to Unicode domains. r=esawin
https://hg.mozilla.org/integration/autoland/rev/2394625be483
Part 2 - Make script dialogues use the Unicode domain as title. r=baku
https://hg.mozilla.org/integration/autoland/rev/458c41e00670
Part 3 - Switch various places that can end up being user-visible to use Unicode domains. r=esawin,jwu
https://hg.mozilla.org/integration/autoland/rev/89aa265c6be9
Part 4 - Switch Session Store to save the "display" URL. r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/0f3603bb2045
Part 5 - Normalise the saved "appOrigin" to Unicode. r=jwu
https://hg.mozilla.org/integration/autoland/rev/7707e5ec573c
Part 6 - Switch context menus to Unicode domains. r=jwu
https://hg.mozilla.org/integration/autoland/rev/77084cd51869
Part 7 - Switch addon/theme install prompts to Unicode domains. r=jwu
https://hg.mozilla.org/integration/autoland/rev/3cb0b289475c
Part 8 - Fix site identity handling. r=jwu
https://hg.mozilla.org/integration/autoland/rev/65537efd1f72
Part 9 - Add a basic Robocop test for IDN domain support. r=gbrown
Verified in 57.0b3 the url bar displays http://faß.de.
Status: RESOLVED → VERIFIED
status-firefox57: fixed → verified
Depends on: 1403693
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: