New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 818839 link

Starred by 3 users

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 2
Type: Bug


Participants' hotlists:
Url-Audit


Sign in to add a comment

URL is shown in punycode

Project Member Reported by mar...@mwiacek.com, Mar 5 2018

Issue description

Device name:s7

From "Settings > About Chrome"
Application version:66.0.3362.0
Operating system:7

URLs (if applicable):allestörungen.ch/

Steps to reproduce:
(1)open url
(2)open context menu on any element or page info

Expected result:
allestörungen.ch/ in url

Actual result:
url is modified and we seems to be doing it when FF doesn't

 
Screenshot_20180305-225553.png
319 KB View Download
Labels: Needs-triage-Mobile
Cc: pnangunoori@chromium.org
Labels: FoundIn-66 Target-67 FoundIn-67 M-67 Triaged-Mobile FoundIn-65 OS-Linux OS-Mac OS-Windows Type-Bug
Status: Untriaged (was: Unconfirmed)
Tested the issue in Android and able to reproduce the issue. Similar behavior is observed since Chrome #60.0.3072.0

Steps Followed:
1. Launched the Chrome Browser.
2. Navigate to the URL: allestörungen.ch
3. Tap on 'i' (Site info button).
4. Observed that "http://xn--allestrungen-9ib.ch" URL is displayed.

Chrome versions tested:
60.0.3072.0, 64.0.3282.137(Stable), 67.0.3363.3(Canary)

OS:
Android 7.0.0

Android Devices:
Samsung J7 Build/NRD90M

This seems to be a Non-Regression issue as same behavior is seen since M60.  Untriaged for further input's on this issue.

Please navigate to below link for log's and screen cast--
go/chrome-androidlogs/818839

Note: 
1. Similar behavior is observed in Desktop. It can be observed in Site Settings page. Eg.: chrome://settings/content/siteDetails?site=http%3A%2F%2Fxn--allestrungen-9ib.ch
2. Issue is not observed on FireFox mobile/desktop version.

Thanks!
818839 - Desktop.png
55.3 KB View Download
Cc: tedc...@chromium.org
Labels: android-fe-triaged
Status: Assigned (was: Untriaged)
Components: -UI
Labels: Team-Security-UX
Status: Untriaged (was: Assigned)
What language is your phone set to?  Does the URL appear correct in the Omnibox?

I don't know the expected behavior here so adding Team-Security-UX to weigh in.
Reply to C#4

Device is set to English in both Mobile and Desktop versions.

Yeah, in the omnobox URL seems to be correct.

Comment 6 by cthomp@chromium.org, Mar 21 2018

Cc: js...@chromium.org mgiuca@chromium.org
This appears to be triggering our punycode URL display due to unicode characters inconsistently. The original domain is displayed on M65 Desktop Linux, but the site settings shows the punycode. https://www.chromium.org/developers/design-documents/idn-in-google-chrome has some of our details on IDN display.

This conversion happens in https://cs.chromium.org/chromium/src/chrome/browser/ui/page_info/page_info.cc?l=314&gsn=GetSimpleSiteName, which calls url_formatter::FormatUrlForSecurityDisplay https://cs.chromium.org/chromium/src/components/url_formatter/elide_url.cc?l=371&gsn=FormatUrlForSecurityDisplay

Which ultimately calls HostForDisplay() to format the host piece https://cs.chromium.org/chromium/src/components/url_formatter/elide_url.cc?l=139&gsn=HostForDisplay

base::string16 HostForDisplay(base::StringPiece host_in_puny) {
  base::string16 host = url_formatter::IDNToUnicode(host_in_puny);
  return base::i18n::StringContainsStrongRTLChars(host) ?
      base::ASCIIToUTF16(host_in_puny) : host;
}

This is used throughout Chrome, but notably _not_ for the omnibox URL display, which is computed here: https://cs.chromium.org/chromium/src/components/toolbar/toolbar_model_impl.cc?gsn=GetCurrentPermanentUrlText&l=63

  base::string16 result = url_formatter::FormatUrl(GetURL(), format_types,
                                                   net::UnescapeRule::NORMAL,
                                                   nullptr, nullptr, nullptr);
  return gfx::TruncateString(result, max_url_display_chars_,
                             gfx::CHARACTER_BREAK);

Further digging turns up Issue 650760 which has more discussion about Format*ForSecurityDisplay methods vs. omnibox display. My brief understanding of Format*ForSecurityDisplay is that it should only be showing punycode for domains with RTL characters, which does not seem to be the case here.

cc-ing mgiuca@ and jshin@ on this as it seems closely related to the work on Issue 650760, but not quite a duplicate. It sounds like the intended behavior would be to just display this as unicode everywhere to match the omnibox display (since there are no RTL characters). I can poke some more at why this is happening.

Comment 7 by cthomp@chromium.org, Mar 21 2018

Cc: cthomp@chromium.org

Comment 8 by est...@chromium.org, Mar 31 2018

Components: UI>Security>UrlFormatting
Status: Available (was: Untriaged)
Thanks for the great investigation, cthomp! It does sound like we shouldn't be display punycode in Page Info and other surfaces if we didn't deem it necessary to show punycode in the omnibox.

Comment 9 by cthomp@chromium.org, Apr 16 2018

Took a little more time to dig into this. There are a couple distinct, but related, issues here. From what I can tell, all of the C++ UI displaying URLs are using those functions, which ultimately call `IDNToUnicode(host)`, correctly converting hosts like this into Unicode. But, non-C++ logic appears to consistently not mirror the logic of `elide_url.cc` and `url_formatter` correctly.

1) [The reported bug] The Android Java code tries to separately handle eliding URLs to keep the origin visible, but never converts to Unicode. I'm not sure how well it's Unicode handling is in general, as I'm not familiar with Java's String/Unicode machinery. See: https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chrome/browser/page_info/PageInfoPopup.java

Interestingly, I think that ConnectionInfoPopup.java does not suffer from these problems since it sets the section headline from the IdentityInfo struct in the native C++ ConnectionInfoPopup code: https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chrome/browser/page_info/ConnectionInfoPopup.java?l=71 (It calls `nativeInit()`, which results in setting up the correct IdentityInfo and calling back into the Java code here https://cs.chromium.org/chromium/src/chrome/browser/ui/android/page_info/connection_info_popup_android.cc?l=117.)

2) Javascript code, particularly in DevTools. You can see the same punycode show up in the origins listed in the Security panel, and throughout the Network panel details.

It looks like origins in the Security panel sidebar are added in https://cs.chromium.org/chromium/src/third_party/blink/renderer/devtools/front_end/security/SecurityPanel.js?l=444

This is made trickier because I think there's less of a distinction between "URL string for displaying" and "URL string as datatype" in JS.

This also comes up in console warning source URLs.


For the Android PageInfoPopup, I think it should be calling UrlFormatter.formatUrlForSecurityDisplay [1] to be in line with the behavior of PageInfo on Desktop. Here's a quick CL that adds that: 

https://chromium-review.googlesource.com/c/chromium/src/+/1015229

I'm tracking down a crash that occurs in the ConnectionInfoPopup for https://expired.badssl.com (after clicking through the warning), and may update that CL.

Handling the formatting issue in Javascript/Blink code may be more involved.

[1] https://cs.chromium.org/chromium/src/components/url_formatter/android/java/src/org/chromium/components/url_formatter/UrlFormatter.java?l=89
Looks like the crash I ran into in #10 is already tracked in  Issue 825911 . I'll tackle that separately.
One more fun bit: Android Site Settings sometimes shows the correct Unicode (when going Page -> Page Info -> Site Settings), but sometimes shows the punycode (when going Settings -> All sites/single site settings).
Project Member

Comment 13 by bugdroid1@chromium.org, Apr 28 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/266018787285e7cd288826fe8113a5d323d45e7e

commit 266018787285e7cd288826fe8113a5d323d45e7e
Author: Christopher Thompson <cthomp@chromium.org>
Date: Sat Apr 28 02:35:00 2018

Use FormatUrl in Android PageInfoPopup

This adds a new UrlFormatter.formatUrlForDisplayWithNormalEscaping()
function, and uses it in the Android PageInfoPopup to make its URL
display correctly de-punycode URLs that are safe to convert back to
Unicode for display.

This also adds url_formatter unittests for the specific usage of
FormatUrl that the new function uses under a variety of URL
conditions.

Bug: 818839
Change-Id: Id0f50a6a37be7309c684ef180acc78f84895d729
Reviewed-on: https://chromium-review.googlesource.com/1015229
Commit-Queue: Christopher Thompson <cthomp@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Emily Stark <estark@chromium.org>
Reviewed-by: Ted Choc <tedchoc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#554611}
[modify] https://crrev.com/266018787285e7cd288826fe8113a5d323d45e7e/chrome/android/java/src/org/chromium/chrome/browser/page_info/PageInfoPopup.java
[modify] https://crrev.com/266018787285e7cd288826fe8113a5d323d45e7e/chrome/android/java/src/org/chromium/chrome/browser/page_info/PageInfoView.java
[modify] https://crrev.com/266018787285e7cd288826fe8113a5d323d45e7e/chrome/android/javatests/DEPS
[modify] https://crrev.com/266018787285e7cd288826fe8113a5d323d45e7e/chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/TrustedCdnPublisherUrlTest.java
[modify] https://crrev.com/266018787285e7cd288826fe8113a5d323d45e7e/chrome/android/javatests/src/org/chromium/chrome/browser/page_info/PageInfoPopupTest.java
[modify] https://crrev.com/266018787285e7cd288826fe8113a5d323d45e7e/components/url_formatter/android/java/src/org/chromium/components/url_formatter/UrlFormatter.java
[modify] https://crrev.com/266018787285e7cd288826fe8113a5d323d45e7e/components/url_formatter/url_formatter_android.cc

Cc: -cthomp@chromium.org
Owner: cthomp@chromium.org
Taking ownership of this as I plan to continue working on the related punycode/unicode display bugs.
Status: Started (was: Available)
Can we have the same function used for displaying URL in the context menu and page info?

We've got UrlFormatter.formatUrlForDisplayWithNormalEscaping() like introduced in #13, I've tried to use it for Context Menu but still have problems with tests. I jave also asked SWE for some advice here and so far nobody was interested.

More info: https://chromium-review.googlesource.com/c/chromium/src/+/1037123

cthomp@, could you help here?
Sure I can take a look at the CL. That seems like an easy fix to improve URL display.

I also want to take a broader look sometime at URL display on Android in general to try to think about how different UI surfaces should behave (and potentially push for simplify/improving URL display across many of them), but I likely won't have engineering cycles to get very far on that this quarter.
Project Member

Comment 18 by bugdroid1@chromium.org, May 7 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a81cc134860d246ffe09cee1074737c1ffe23ec1

commit a81cc134860d246ffe09cee1074737c1ffe23ec1
Author: Christopher Thompson <cthomp@chromium.org>
Date: Mon May 07 17:27:25 2018

Improve URL formatting in Android Site Settings

This changes the display of a WebsiteAddress (in getTitle()) to use
FormatUrlForSecurityDisplay(), so that domains are correctly converted
to Unicode when it is safe to do so (instead of being displayed as
punycode if any non-ASCII characters exist).

This will additionally strip the path/query/fragment, but the current
WebsiteAddress implementation should not include those.

Bug: 818839
Change-Id: I763c81d8a7fb25d66563c14f6a50a93cf16aa494
Reviewed-on: https://chromium-review.googlesource.com/1022967
Reviewed-by: Ted Choc <tedchoc@chromium.org>
Commit-Queue: Christopher Thompson <cthomp@chromium.org>
Cr-Commit-Position: refs/heads/master@{#556483}
[modify] https://crrev.com/a81cc134860d246ffe09cee1074737c1ffe23ec1/chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsiteAddress.java

Labels: Needs-Feedback
Tested on latest Chrome Canary #68.0.3324.0 on Mac 10.13.3, Windows 10 and Debian Rodete and still observed the URL as  http://xn--allestrungen-9ib.ch in site settings page. By following the steps and comments provided in the Comment #2

cthomp@ -- Could you please let us know whether it is the intended behavior or what exactly the fix is. It would help us in verifying and adding the TE Verified labels.

Attached the screenshot for reference.

Thanks!
818839.png
88.2 KB View Download
The fix in #18 is only for Android site settings unfortunately. I'll need to work on another CL for the Desktop site settings formatting issue.
Components: UI>Browser>SiteSettings
Labels: OS-Chrome
Summary: URL is shown in punycode (was: url is reformatted)
Hi cthomp@, thanks very much for working on this! I'm working on desktop All Sites, which is part of the desktop site settings, and having this be consistent with the omnibox would be awesome. What's your timeline / priority for a fix? (I'm not blocked or anything, just curious if you were planning to work on this any time soon.)
Status: Assigned (was: Started)
Unfortunately I don't have any plans to work on this in the near future. Fixing this for desktop site settings has gotten pushed aside as this was already somewhat of a side project for me (and that I don't know much about WebUI/desktop site settings). As a rough estimate: I might try to work on it in Q3, likely as part of our work to audit URL display surfaces in Chrome.

If some of this is relevant to your All Sites work, I'd be happy to coordinate or recommend how to fix it, but may not have cycles to actually work on the CLs.
No worries, fixing this may be in scope for All Sites as well, though a P3 - will revisit and prioritise closer to the end of the project. Thanks!
The omnibox url is intentionally displayed as punycode to prevent abuse/spoofing ( Issue 683314 ).
see https://www.xudongz.com/blog/2017/idn-phishing/
 Issue 910371  has been merged into this issue.

Sign in to add a comment