URL is shown in punycode |
||||||||||||
Issue descriptionDevice 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
,
Mar 7 2018
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!
,
Mar 9 2018
,
Mar 9 2018
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.
,
Mar 10 2018
Reply to C#4 Device is set to English in both Mobile and Desktop versions. Yeah, in the omnobox URL seems to be correct.
,
Mar 21 2018
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.
,
Mar 21 2018
,
Mar 31 2018
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.
,
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.
,
Apr 17 2018
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
,
Apr 18 2018
Looks like the crash I ran into in #10 is already tracked in Issue 825911 . I'll tackle that separately.
,
Apr 18 2018
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).
,
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
,
May 3 2018
Taking ownership of this as I plan to continue working on the related punycode/unicode display bugs.
,
May 3 2018
,
May 3 2018
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?
,
May 3 2018
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.
,
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
,
May 8 2018
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!
,
May 8 2018
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.
,
Jun 25 2018
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.)
,
Jun 26 2018
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.
,
Jun 27 2018
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!
,
Aug 1
The omnibox url is intentionally displayed as punycode to prevent abuse/spoofing ( Issue 683314 ). see https://www.xudongz.com/blog/2017/idn-phishing/
,
Nov 29
Issue 910371 has been merged into this issue. |
||||||||||||
►
Sign in to add a comment |
||||||||||||
Comment 1 by pnangunoori@chromium.org
, Mar 6 2018