1450538 - Statusbar should display a readable URL when the cursor is hovering over a link in an international domain name(At least we can do it for some kinds of URLs which look very different from English characters, E.g., CJK domain name)

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[張俊芝(ZHANG Junzhi)]

Attachment: Statusbar showing a URL with an unreadable Punycode domain name.png

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:58.0) Gecko/20100101 Firefox/58.0
Build ID: 20180123231643

Steps to reproduce:

See the screenshot I uploaded.


Actual results:

The text in the statusbar was a URL with an unreadable Punycode domain name xn--g6w.xn--8pv/測.html


Expected results:

The text in the statusbar should have been a URL with a readable Unicode domain name, that is, 測.本/測.html

Comment 1

[張俊芝(ZHANG Junzhi)]

I would like to fix this bug, but since I still have two bugs under reviewing and I am afraid that I won't have much time these days, so if anyone would like to work on this bug, please feel free to take it. I would like to see this bug get fixed as soon as possible.

Comment 2

[張俊芝(ZHANG Junzhi)]

Please see the uploaded attachment, the bottomleft status bar shows an unreadable Punycode domain name, it should show a readable Unicode domain name.

Please help confirm this bug.

Since it doesn't seem to require too much code, and correctly showing a link address is a basic feature for a browser, so I think we might prioritize this bug.

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

Could you please provide a URL when we can observe such a link?

Comment 4

[Panos Astithas (he/him) [:past] (please ni?)]

Moving to Firefox::Security as there may be concerns around phishing with the proposed change.

Comment 5

[u554753]

Attachment: URL on 61.png

I get the URL seen in attachment, but not sure if it's intended.

Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:61.0) Gecko/20100101 Firefox/61.0

Comment 6

[張俊芝(ZHANG Junzhi)]

(In reply to Panos Astithas [:past] (please ni?) from comment #3)
> Could you please provide a URL when we can observe such a link?

For example, http://nic.世界/
After opening the homepage, move the cursor to any internal link, then you can see that the domain name is displayed as the unreadable nic.xn--rhqv96g/ , not the readable nic.世界

Comment 7

[張俊芝(ZHANG Junzhi)]

(In reply to Grover Wimberly IV [:Grover-QA] from comment #5)
> Created attachment 8964982 [details]
> URL on 61.png
> 
> I get the URL seen in attachment, but not sure if it's intended.
> 
> Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:61.0) Gecko/20100101
> Firefox/61.0

測.本 is my local domain name which is used to illustrate the bug. comment 6 provides an actual Unicode domain name.

Comment 8

[張俊芝(ZHANG Junzhi)]

(In reply to Panos Astithas [:past] (please ni?) from comment #4)
> Moving to Firefox::Security as there may be concerns around phishing with
> the proposed change.

It's not phishing.

Because of the historical reason, only ASCII characters are valid in the domain name, so Punycode is invented to encode a Unicode domain name in ASCII characters.
Punycode is a standard, please refer to:

https://en.wikipedia.org/wiki/Punycode

So, the underlying data transferring actually goes through the Punycode domain name, but from the prespective of the user, they obviously don't want to remember a Punycode domain name

Comment 9

[張俊芝(ZHANG Junzhi)]

So the bug here is talking about showing a friendly Unicode domain name, not the hard-no-remembered Punycode domain name. Nothing about security.

Comment 10

[張俊芝(ZHANG Junzhi)]

If you use Chrome to browse http://nic.世界/ You can see that it shows a friendly Unicode domain name in the status bar when you are hovering over a link in the website.

Comment 11

[張俊芝(ZHANG Junzhi)]

Maybe it needs someone who have knowledge on Punycode to have a look at this bug. It's fiarly easy to confirm.

Daniel, could you confirm this bug and prioritize it?

I wish I could work on this, but I am afraid I won't have much time in the near future, so I really wish someone would kindly fix it as soon as possible.

Comment 12

[張俊芝(ZHANG Junzhi)]

Attachment: status bar showing a Punycode domain URL when cursor is hovering over a link to a Unicode domain name URL.png

Comment 13

[Johann Hofmann [:johannh]]

Some people have traditionally pointed at the fact that we show punycode in that tooltip to say "look, the phishing issue is not so bad", so this might be a WONTFIX...

Comment 14

[張俊芝(ZHANG Junzhi)]

(In reply to Johann Hofmann [:johannh] from comment #13)
> Some people have traditionally pointed at the fact that we show punycode in
> that tooltip to say "look, the phishing issue is not so bad", so this might
> be a WONTFIX...

I propose we can show both the two forms of the URLs, or by some other way, at least let the user know the Unicode filename isn't a bad idea.

Comment 15

[張俊芝(ZHANG Junzhi)]

s/Unicode filename/Unicode domain name

Comment 16

[張俊芝(ZHANG Junzhi)]

(In reply to Johann Hofmann [:johannh] from comment #13)
> Some people have traditionally pointed at the fact that we show punycode in
> that tooltip to say "look, the phishing issue is not so bad", so this might
> be a WONTFIX...

And moreover, we don't need to show both the two forms of the URLs for some kinds of the URL, for example, CJK Unicode domain, they look so different from the English characters, so it's not possible to make a phishing attack by a Chinese domain name.

Comment 17

[Daniel Veditz [:dveditz]]

I can confirm the bug, but "Firefox Security" seems the wrong place for it. Doing it badly might be a security problem, but it's not up to the security team to fix it.

> > there may be concerns around phishing with the proposed change.
>
> It's not phishing.

There are many "look-alike" characters in Unicode and they can be used for _spoofing_, which has been taken advantage of to perform _phishing_.

However, we do display many hosts as IDN in the addressbar where it is most important to prevent spoofing. We sometimes have missed things and those are bugs that we fix. Over all it should be safe to show the hover text as IDN if (and ONLY if) we will display it as such in the addressbar. And then we should display it exactly as we would display it in the addressbar.

(In reply to Johann Hofmann [:johannh] from comment #13)
> Some people have traditionally pointed at the fact that we show punycode in
> that tooltip to say "look, the phishing issue is not so bad"

You can't trust link hovertext in many situations. "Check the hovertext for punycode to avoid spoofing" is a terrible lesson for people to learn. Either we (as an organization) support IDN or we don't; this looks like a bug to me.

Comment 18

[Masatoshi Kimura [:emk]]

(In reply to Johann Hofmann [:johannh] from comment #13)
> Some people have traditionally pointed at the fact that we show punycode in
> that tooltip to say "look, the phishing issue is not so bad", so this might
> be a WONTFIX...

Actually we used to display Unicode (non-Punycode) host names. You can confirm it by using ESR 52. This is a regression of bug 945240. We should use GetDisplaySpec() instead of GetSpec() here:
https://dxr.mozilla.org/mozilla-central/rev/30d72755b1749953d438199456f1524ce84ab5e5/docshell/base/nsDocShell.cpp#13786

And yes, we can't consider the status panel as a security UI. It is too easy to spoof the content.

Comment 19

[Masatoshi Kimura [:emk]]

Attachment: Bug 1450538 - Use nsIURI::GetDisplaySpec to compensate a change to nsIURI::GetSpec.

Review commit: https://reviewboard.mozilla.org/r/236612/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/236612/

Comment 20

[Masatoshi Kimura [:emk]]

Comment on attachment 8967915 [details]
Bug 1450538 - Use nsIURI::GetDisplaySpec to compensate a change to nsIURI::GetSpec.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/236612/diff/1-2/

Comment 21

[張俊芝(ZHANG Junzhi)]

Glad to see someone has worked on this.

Thanks.

Comment 22

[張俊芝(ZHANG Junzhi)]

Could we have a mochitest to make sure it wont regress again?

Comment 23

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8967915 [details]
Bug 1450538 - Use nsIURI::GetDisplaySpec to compensate a change to nsIURI::GetSpec.

https://reviewboard.mozilla.org/r/236612/#review242406

Comment 24

[Masatoshi Kimura [:emk]]

Attachment: Bug 1450538 - Browser test to ensure that the StatusPanel displays an IDN.

Review commit: https://reviewboard.mozilla.org/r/236644/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/236644/

Comment 25

[Masatoshi Kimura [:emk]]

Comment on attachment 8967915 [details]
Bug 1450538 - Use nsIURI::GetDisplaySpec to compensate a change to nsIURI::GetSpec.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/236612/diff/2-3/

Comment 26

[Masatoshi Kimura [:emk]]

Comment on attachment 8967953 [details]
Bug 1450538 - Browser test to ensure that the StatusPanel displays an IDN.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/236644/diff/1-2/

Comment 27

[Masatoshi Kimura [:emk]]

I could not make this test work with TV+webrender. What is wrong? I suspect a bug of webrender.

Comment 28

[:Gijs (he/him)]

(In reply to Masatoshi Kimura [:emk] from comment #27)
> I could not make this test work with TV+webrender. What is wrong? I suspect
> a bug of webrender.

I expect the same as https://bugzilla.mozilla.org/show_bug.cgi?id=1358712#c30 .

Comment 29

[:Gijs (he/him)]

Comment on attachment 8967953 [details]
Bug 1450538 - Browser test to ensure that the StatusPanel displays an IDN.

https://reviewboard.mozilla.org/r/236644/#review242646

::: browser/base/content/test/statuspanel/browser_show_statuspanel_idn.js:14
(Diff revision 2)
> +  let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, TEST_PAGE_URL);
> +
> +  let promise = promiseStatusPanelShown(window, TEST_STATUS_TEXT);
> +  ContentTask.spawn(tab.linkedBrowser, null, async () => {
> +    content.document.links[0].focus();
> +  });
> +  await promise;
> +
> +  await BrowserTestUtils.removeTab(tab);

For future reference, you could also have used `withNewForegroundTab` with an (async) callback for this type of "open a tab, test stuff, close the tab" test. Doesn't need to block landing this though, this is also fine.

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/aee0f2f18cde
Use nsIURI::GetDisplaySpec to compensate a change to nsIURI::GetSpec. r=valentin
https://hg.mozilla.org/integration/autoland/rev/4213de0783d3
Browser test to ensure that the StatusPanel displays an IDN. r=Gijs

Comment 31

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/aee0f2f18cde
https://hg.mozilla.org/mozilla-central/rev/4213de0783d3

Comment 32

[Masatoshi Kimura [:emk]]

Comment on attachment 8967915 [details]
Bug 1450538 - Use nsIURI::GetDisplaySpec to compensate a change to nsIURI::GetSpec.

Approval Request Comment
[Feature/Bug causing the regression]: 945240
[User impact if declined]: Users cannot read IDNs in the status panel.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: The change just reverts the behavior of Firefox 56 or earlier.
[String changes made/needed]: none

Comment 33

[Julien Cristau [:jcristau]]

Comment on attachment 8967915 [details]
Bug 1450538 - Use nsIURI::GetDisplaySpec to compensate a change to nsIURI::GetSpec.

display idn instead of punycode in status bar, approved for 60.0b14

Comment 34

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/8b59be7c1ce6
https://hg.mozilla.org/releases/mozilla-beta/rev/fd3897b47e12

Comment 35

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/e6cdf63c59a4

Comment 36

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out from beta for failing browser_show_statuspanel_idn.js:

https://hg.mozilla.org/releases/mozilla-beta/rev/f453337749a3fd288e5591e804c9fdb24fe41afd

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=174583240&repo=mozilla-beta

[task 2018-04-19T17:27:33.525Z] 17:27:33     INFO - Buffered messages finished
[task 2018-04-19T17:27:33.525Z] 17:27:33     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/statuspanel/browser_show_statuspanel_idn.js | Uncaught exception - at chrome://mochitests/content/browser/browser/base/content/test/statuspanel/head.js:16 - TypeError: win.StatusPanel is undefined
[task 2018-04-19T17:27:33.526Z] 17:27:33     INFO - Stack trace:
[task 2018-04-19T17:27:33.526Z] 17:27:33     INFO -     promiseStatusPanelShown@chrome://mochitests/content/browser/browser/base/content/test/statuspanel/head.js:16:7
[task 2018-04-19T17:27:33.526Z] 17:27:33     INFO -     test_show_statuspanel_twice@chrome://mochitests/content/browser/browser/base/content/test/statuspanel/browser_show_statuspanel_idn.js:16:17
[task 2018-04-19T17:27:33.526Z] 17:27:33     INFO -     Tester_execTest@chrome://mochikit/content/browser-test.js:1058:9
[task 2018-04-19T17:27:33.527Z] 17:27:33     INFO -     Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:958:9
[task 2018-04-19T17:27:33.527Z] 17:27:33     INFO -     SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:795:59

Does it more from bug 1445455 and others?

Comment 37

[Masatoshi Kimura [:emk]]

Mmm, the status panel impl is completely incompatible due to de-XBL. Is it possible to land only non-test part?

Comment 38

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/455ece02d8d2