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

Issue metadata

Status: Fixed
Owner:
Closed: Dec 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 1
Type: Bug-Regression



Sign in to add a comment
link

Issue 789163: Omnibox does not unescape URL fragment

Reported by factormy...@gmail.com, Nov 28 2017

Issue description

UserAgent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/62.0.3202.29 Safari/537.36

Steps to reproduce the problem:
1. open example webpage
2. observe the browser url and the devtools console
3. the end of the url should look like: ∗#∗ but instead comes out as: ∗#%E2%88%97

Example:
(also live at https://factormystic.net/crbug/bad-hash-escaping.html)

```
<!DOCTYPE html>
<html>
<head>
<script type="text/javascript">
window.history.replaceState(null, null, "\u2217");
window.history.replaceState(null, null, "#\u2217");
if (document.location.hash != "#\u2217") {
    console.error("we have a problem");
}
</script>
</head>
</html>

```

BTW, I don't think this is specifically about window.history.replaceState, as document.location.hash = "#\u2217" appears to be equivalently regressed.

What is the expected behavior?
I do not expect that setting the document location hash will be escaped. "\u2217" was not escaped in prior versions of chrome, or in other browsers, or other portions of the url.

What went wrong?
"\u2217" (as a demo) is escaped in the hash portion of the url. This is behavior that has recently changed, and I do not expect or want it to be escaped in the hash or anywhere.

Did this work before? Yes it works correctly in 62.0.3202.29 (beta) for sure, not sure about between that and 64.0.3269.3 (dev)

Chrome version: 64.0.3269.3  Channel: dev
OS Version: 10.0
Flash Version:
 
crbug-hash-broken.png
159 KB View Download
crbug-hash-ok.png
53.0 KB View Download

Comment 1 by krajshree@chromium.org, Nov 29 2017

Labels: Needs-Triage-M64

Comment 2 by divya.pa...@techmahindra.com, Nov 29 2017

Cc: ranjitkan@chromium.org divya.pa...@techmahindra.com
Labels: -Pri-2 hasbisect-per-revision ReleaseBlock-Stable Triaged-ET M-63 OS-Linux OS-Mac Pri-1
Owner: mkwst@chromium.org
Status: Assigned (was: Unconfirmed)
Able to reproduce the issue on reported version 64.0.3269.3, beta 63.0.3239.59 and Dev 64.0.3280.0 using Windows 10, Ubuntu 14.04, Win10 hence providing bisect info

Bisect Info:
================
Good build: 63.0.3236.0
Bad build: 63.0.3237.7

You are probably looking for a change made after 507480 (known good), but no later than 507481 (first known bad).
CHANGELOG URL:
https://chromium.googlesource.com/chromium/src/+log/0a918e900a759b04f177cc5b8bca668140751136..f8f6ed59949be4451ee2f5443d8a313f102fde60

Reviewed-on: https://chromium-review.googlesource.com/668363

@Mike West: Please confirm the issue and help in re-assigning if it is not related to your change.

Note: Feel free to adjust the blocker and milestone if this should not be blocking for M-63

Thanks!

Comment 3 by ranjitkan@chromium.org, Nov 29 2017

Cc: pbomm...@chromium.org gov...@chromium.org

Comment 4 by gov...@chromium.org, Nov 29 2017

M63 Stable promotion is coming VERY soon. Your bug is labelled as Stable ReleaseBlock, pls make sure to land the fix and request a merge  into the release branch ASAP. Thank you.

Comment 5 by gov...@chromium.org, Nov 29 2017

Plan is to cut M63 Stable RC on this Friday (12/01) @ 5:00 PM PT. Please plan to have fix landed/merged to M63 before then. Thank you.

Comment 6 by mkwst@chromium.org, Nov 30 2017

Cc: elawrence@chromium.org est...@chromium.org
We intentionally changed our encoding of fragment identifiers to align with the spec and other vendors (and will be encoding more characters as discussed in https://github.com/whatwg/url/issues/344 and https://github.com/whatwg/url/pull/347).

* Chrome 63+, Firefox, and Safari all echo "we have a problem", as `window.location.hash` is populated with the percent-encoded values rather than the non-encoded values. I don't have Edge sitting in front of me to test, but I can check it when I get home.

* You're correct, though, that we're showing percent-encoded values in the fragment, but not the path. That's strange.

I don't feel like the change in the way that fragment identifiers are displayed is a stable blocker, and would prefer that we keep the web-facing change to align our behavior with other vendors. That said, it's not pretty, and it would be nice to fix it. CCing some folks who know more about URL display in the omnibox than I do, and who might have opinions.

If this is deemed a blocker, then reverting https://chromium-review.googlesource.com/c/668363/ would fix the display at the expense of regressing the percent-escaping.

Comment 7 by factormy...@gmail.com, Nov 30 2017

> Chrome 63+, Firefox, and Safari all echo "we have a problem", as `window.location.hash` is populated with the percent-encoded values rather than the non-encoded values. I don't have Edge sitting in front of me to test, but I can check it when I get home.

I see. However there is still a variation. The url bar ui in other browsers does NOT display the percent encoded hash. Only Chrome 63 does. This still seems like a bug.
crbug-ui-edge.png
108 KB View Download
crbug-ui-firefox.png
86.6 KB View Download

Comment 8 by gov...@chromium.org, Nov 30 2017

Cc: candr...@chromium.org
+candrada@, could you pls check whether this repros on Android or not?

Comment 9 by elawrence@chromium.org, Nov 30 2017

Re #7: 

> "I see. However there is still a variation"

Yes, this is as Mike notes when he says " You're correct, though, that we're showing percent-encoded values in the fragment, but not the path. That's strange. "

The fix for that in Chrome is small in size, but carries unknown risk at this late date.

--- a/components/url_formatter/url_formatter.cc
+++ b/components/url_formatter/url_formatter.cc
@@ -586,11 +586,10 @@ base::string16 FormatUrlWithAdjustments(
                              NonHostComponentTransform(unescape_rules),
                              &url_string, &new_parsed->query, adjustments);

-    // Ref.  This is valid, unescaped UTF-8, so we can just convert.
     if (parsed.ref.is_valid())
       url_string.push_back('#');
     AppendFormattedComponent(spec, parsed.ref,
-                             NonHostComponentTransform(net::UnescapeRule::NONE),
+                             NonHostComponentTransform(unescape_rules),
                              &url_string, &new_parsed->ref, adjustments);
   }

Comment 10 by elawrence@chromium.org, Nov 30 2017

Labels: OS-Android
Re #8, yes, this reproduces on Android Chrome 64 (and does not repro on Android Chrome 62).

Comment 11 by gov...@chromium.org, Nov 30 2017

Cc: cma...@chromium.org gkihumba@chromium.org
Labels: OS-Chrome
Thank you  elawrence@.

Adding Chrome OS as well assuming this repros there as well.

Comment 12 by elawrence@chromium.org, Nov 30 2017

Cc: mkwst@chromium.org pkasting@chromium.org
 Issue 790688  has been merged into this issue.

Comment 13 by elawrence@chromium.org, Nov 30 2017

Labels: -ReleaseBlock-Stable
Status: Untriaged (was: Assigned)
I don't think this should be considered a Release blocker, but assigning to the proper node for triage.

Comment 14 by elawrence@chromium.org, Nov 30 2017

Summary: Omnibox does not unescape URL fragment (was: document location hash incorrectly escaped)

Comment 15 by cbiesin...@chromium.org, Nov 30 2017

Components: -Blink Blink>Network

Comment 16 by elawrence@chromium.org, Nov 30 2017

Components: -Blink>Network UI>Browser>Omnibox

Comment 17 by mpear...@chromium.org, Dec 12 2017

mkwst@ and elawrence@ - are you planning to make this fix?  I know you decided not to merge a fix, but I got the impression that you would fix it.

Comment 18 by elawrence@chromium.org, Dec 12 2017

RE #17: I think we /should/ take a fix here, but I'd like to hear from Omnibox owners who are experts in this space before we change the behavior. I suppose we could just try the CL in #9 and wait for LGTMs?

Comment 19 by mpear...@chromium.org, Dec 12 2017

As an omnibox owner, fixing this sgtm.

Comment 20 by elawrence@chromium.org, Dec 12 2017

Looking to see how many tests I break. :) 
https://chromium-review.googlesource.com/c/chromium/src/+/822819

Comment 21 by jack.who...@gmail.com, Dec 13 2017

Hello! I'm a Wikipedia contributor, and last year there was a community survey among Wikipedians where changing the fragment appearance from #R.C3.A9sum.C3.A9 to #Résumé and from #.D0.9D.D0.BE.D0.B2.D1.8B.D0.B9... to #Новый... was the #3 wish out of 265: https://meta.wikimedia.org/wiki/2016_Community_Wishlist_Survey/Results.

This was implemented successfully by Wikimedia developers, only to be nullified by the recent Chrome 63.0.3239.84 release (https://chromereleases.googleblog.com/2017/12/stable-channel-update-for-desktop.html). And now #Новый looks like #%D0%9D%D0%BE%D0%B2%D1%8B%D0%B9.

According to the linked page, this was done out of some security considerations. This is fine, but it's unclear why the fragment can't be _displayed_ decoded in the address bar, as is the case with the path. This behaviour doesn't seem consistent.

So, in actuality (when sent to server, as well as copied to clipboard):
- path is encoded
- fragment is encoded

In the address bar:
- path is decoded
- fragment is encoded

For example (see the attachment):
- in actuality: https://en.wikipedia.org/wiki/R%C3%A9sum%C3%A9#R%C3%A9sum%C3%A9_evaluation
- in the address bar: https://en.wikipedia.org/wiki/Résumé#R%C3%A9sum%C3%A9_evaluation

In Wikipedia (which is, let me remind you, the 5th top site in the world according to Alexa), fragment links appear in the table of contents on Wikipedia, as well as the image links, so they are widespread there.

Firefox has fragment prettification in the address bar for a long time already, while Chrome has no.
Résumé - Wikipedia.png
19.9 KB View Download

Comment 22 by elawrence@chromium.org, Dec 13 2017

Re #21: I expect that display will show decided values in 65 or even 64. See #20.

Comment 23 by d.j.hart...@gmail.com, Dec 13 2017

Re #22: Thank you for taking the time to answer that question.

Comment 24 by bugdroid1@chromium.org, Dec 14 2017

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

commit f8625da2e7215aaac6c0c76a636ab8483090943d
Author: Eric Lawrence <elawrence@chromium.org>
Date: Thu Dec 14 18:36:37 2017

Unescape fragment for display in Omnibox

A change in Chrome 63 causes Chrome to urlencode the fragment component
of URLs. The Chrome omnibox should decode the fragment component as it
does for other URL components and matching Firefox.

Bug:  789163 
Change-Id: If82d72445d904495c8d3e4129ab63be141745328
Reviewed-on: https://chromium-review.googlesource.com/822819
Commit-Queue: Eric Lawrence <elawrence@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#524123}
[modify] https://crrev.com/f8625da2e7215aaac6c0c76a636ab8483090943d/chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc
[modify] https://crrev.com/f8625da2e7215aaac6c0c76a636ab8483090943d/components/url_formatter/elide_url_unittest.cc
[modify] https://crrev.com/f8625da2e7215aaac6c0c76a636ab8483090943d/components/url_formatter/url_formatter.cc
[modify] https://crrev.com/f8625da2e7215aaac6c0c76a636ab8483090943d/components/url_formatter/url_formatter_unittest.cc

Comment 25 by bugdroid1@chromium.org, Dec 14 2017

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

commit e486ef03b1649a5749cd875b03898b2b1600c055
Author: Peter Laurens <peterlaurens@chromium.org>
Date: Thu Dec 14 22:59:00 2017

Revert "Unescape fragment for display in Omnibox"

This reverts commit f8625da2e7215aaac6c0c76a636ab8483090943d.

Reason for revert: This change breaks EGTests for iOS Simulator and Device, see:

https://logs.chromium.org/v/?s=chromium%2Fbb%2Fchromium.mac%2Fios-simulator-full-configs%2F1434%2F%2B%2Frecipes%2Fsteps%2Fios_chrome_web_egtests__iPhone_6s_Plus_iOS_10.0_%2F0%2Fstdout

Original change's description:
> Unescape fragment for display in Omnibox
> 
> A change in Chrome 63 causes Chrome to urlencode the fragment component
> of URLs. The Chrome omnibox should decode the fragment component as it
> does for other URL components and matching Firefox.
> 
> Bug:  789163 
> Change-Id: If82d72445d904495c8d3e4129ab63be141745328
> Reviewed-on: https://chromium-review.googlesource.com/822819
> Commit-Queue: Eric Lawrence <elawrence@chromium.org>
> Reviewed-by: Peter Kasting <pkasting@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#524123}

TBR=pkasting@chromium.org,elawrence@chromium.org

Change-Id: I9572e8a5d66381478c1faca0f78a66b878a4b059
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  789163 
Reviewed-on: https://chromium-review.googlesource.com/828080
Reviewed-by: Eugene But <eugenebut@chromium.org>
Commit-Queue: Eugene But <eugenebut@chromium.org>
Cr-Commit-Position: refs/heads/master@{#524221}
[modify] https://crrev.com/e486ef03b1649a5749cd875b03898b2b1600c055/chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc
[modify] https://crrev.com/e486ef03b1649a5749cd875b03898b2b1600c055/components/url_formatter/elide_url_unittest.cc
[modify] https://crrev.com/e486ef03b1649a5749cd875b03898b2b1600c055/components/url_formatter/url_formatter.cc
[modify] https://crrev.com/e486ef03b1649a5749cd875b03898b2b1600c055/components/url_formatter/url_formatter_unittest.cc

Comment 26 by mpear...@chromium.org, Dec 15 2017

Cc: brajkumar@chromium.org
 Issue 795164  has been merged into this issue.

Comment 27 by bugdroid1@chromium.org, Dec 16 2017

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/03f9a90d8a783f9d1a94935ac298338a1e694380

commit 03f9a90d8a783f9d1a94935ac298338a1e694380
Author: Eric Lawrence <elawrence@chromium.org>
Date: Sat Dec 16 04:48:11 2017

Reland of 'Unescape fragment for display in Omnibox'

The original landing broke EGTests for iOS Simulator and Device which
were not run by the CQ. This change includes updated EGTests.

TBR=pkasting@chromium.org

Bug:  789163 , 643458
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: Ie30afcb7bcba5affc7266d72a10b7f8dc0074314
Reviewed-on: https://chromium-review.googlesource.com/830093
Reviewed-by: Eric Lawrence <elawrence@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Eugene But <eugenebut@chromium.org>
Commit-Queue: Eric Lawrence <elawrence@chromium.org>
Cr-Commit-Position: refs/heads/master@{#524591}
[modify] https://crrev.com/03f9a90d8a783f9d1a94935ac298338a1e694380/chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc
[modify] https://crrev.com/03f9a90d8a783f9d1a94935ac298338a1e694380/components/url_formatter/elide_url_unittest.cc
[modify] https://crrev.com/03f9a90d8a783f9d1a94935ac298338a1e694380/components/url_formatter/url_formatter.cc
[modify] https://crrev.com/03f9a90d8a783f9d1a94935ac298338a1e694380/components/url_formatter/url_formatter_unittest.cc
[modify] https://crrev.com/03f9a90d8a783f9d1a94935ac298338a1e694380/ios/chrome/browser/web/push_and_replace_state_navigation_egtest.mm

Comment 28 by jdonnelly@chromium.org, Dec 22 2017

Owner: elawrence@chromium.org
Status: Assigned (was: Untriaged)

Comment 29 by elawrence@chromium.org, Dec 22 2017

Status: Fixed (was: Assigned)
Fixed in m65 via cl in c27.

Comment 30 by elawrence@chromium.org, Jan 2 2018

Labels: Merge-Request-64
Should we merge this fix to M-64?

Comment 31 by sheriffbot@chromium.org, Jan 2 2018

Project Member
Labels: -Merge-Request-64 Hotlist-Merge-Review Merge-Review-64
This bug requires manual review: Reverts referenced in bugdroid comments after merge request.
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 32 by abdulsyed@google.com, Jan 4 2018

My recommendation is to wait until M65. If it's critical, can you please provide more justification for why this is needed for 64, and how safe this merge is overall? Since this has been present in M62 and change seems quite complex, my recommendation is to take it for M65.

Comment 33 by elawrence@chromium.org, Jan 5 2018

Re #32: The regression was introduced in M63 and discovered late in M63. It should be a fairly safe merge for 64, but since we don't seem to be getting a ton of complaints about this, I don't object to waiting to M65.

Comment 34 by cma...@chromium.org, Jan 5 2018

+1 on letting it M65 since it does not seem critical.

Comment 35 by abdulsyed@google.com, Jan 9 2018

Labels: -Merge-Review-64 Merge-Rejected-64

Comment 36 by mpear...@chromium.org, Jan 24 2018

 Issue 804867  has been merged into this issue.

Comment 37 by jack.who...@gmail.com, Feb 18 2018

So, it will be in 65, right?

Comment 38 by elawrence@chromium.org, Feb 18 2018

Re #37, as noted in #29, yes, the fix landed in 65.0.3297.0.

Sign in to add a comment