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)

RESOLVED FIXED in Firefox 60
(NeedInfo from)

Status

()

defect
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: zjz, Assigned: emk, NeedInfo)

Tracking

({regression})

Trunk
mozilla61
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox59 wontfix, firefox60 fixed, firefox61 fixed)

Details

Attachments

(5 attachments)

Reporter

Description

a year ago
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
Reporter

Comment 1

a year ago
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.
Reporter

Updated

a year ago
Summary: Statusbar should display a readable URL rather when the cursor is hovering over a link in an international domain name. → Statusbar should display a readable URL when the cursor is hovering over a link in an international domain name.
Reporter

Comment 2

a year ago
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.
Flags: needinfo?(past)
Could you please provide a URL when we can observe such a link?
Flags: needinfo?(past) → needinfo?(zjz)
Moving to Firefox::Security as there may be concerns around phishing with the proposed change.
Component: Untriaged → Security

Comment 5

a year ago
Posted image 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
Reporter

Comment 6

a year ago
(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.世界
Flags: needinfo?(zjz)
Reporter

Comment 7

a year ago
(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.
Reporter

Comment 8

a year ago
(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
Reporter

Comment 9

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

Comment 10

a year ago
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.
Reporter

Comment 11

a year ago
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.
Flags: needinfo?(dveditz)
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...
Reporter

Comment 14

a year ago
(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.
Reporter

Comment 15

a year ago
s/Unicode filename/Unicode domain name
Reporter

Comment 16

a year ago
(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.
Reporter

Updated

a year ago
Summary: Statusbar should display a readable URL when the cursor is hovering over a link in an international domain name. → 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)
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(dveditz)
Assignee

Comment 18

a year ago
(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.
Blocks: 945240
Component: Security → Document Navigation
Keywords: regression
Product: Firefox → Core
QA Whiteboard: :valen
QA Whiteboard: :valen
Comment hidden (mozreview-request)
Assignee

Updated

a year ago
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Reporter

Comment 21

a year ago
Glad to see someone has worked on this.

Thanks.
Reporter

Comment 22

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

Comment 23

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

https://reviewboard.mozilla.org/r/236612/#review242406
Attachment #8967915 - Flags: review?(valentin.gosu) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 27

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

Comment 28

a year ago
(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

a year ago
mozreview-review
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.
Attachment #8967953 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 30

a year ago
Pushed by VYV03354@nifty.ne.jp:
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
https://hg.mozilla.org/mozilla-central/rev/aee0f2f18cde
https://hg.mozilla.org/mozilla-central/rev/4213de0783d3
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Assignee

Comment 32

a year ago
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
Attachment #8967915 - Flags: approval-mozilla-beta?
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
Attachment #8967915 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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?
Flags: needinfo?(VYV03354)
Assignee

Comment 37

a year ago
Mmm, the status panel impl is completely incompatible due to de-XBL. Is it possible to land only non-test part?
Flags: needinfo?(VYV03354) → needinfo?(aryx.bugmail)
You need to log in before you can comment on or make changes to this bug.