Omnibox does not unescape URL fragment
Reported by
factormy...@gmail.com,
Nov 28 2017
|
||||||||||||||||
Issue descriptionUserAgent: 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:
,
Nov 29 2017
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!
,
Nov 29 2017
,
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.
,
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.
,
Nov 30 2017
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.
,
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.
,
Nov 30 2017
+candrada@, could you pls check whether this repros on Android or not?
,
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);
}
,
Nov 30 2017
Re #8, yes, this reproduces on Android Chrome 64 (and does not repro on Android Chrome 62).
,
Nov 30 2017
Thank you elawrence@. Adding Chrome OS as well assuming this repros there as well.
,
Nov 30 2017
,
Nov 30 2017
I don't think this should be considered a Release blocker, but assigning to the proper node for triage.
,
Nov 30 2017
,
Nov 30 2017
,
Nov 30 2017
,
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.
,
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?
,
Dec 12 2017
As an omnibox owner, fixing this sgtm.
,
Dec 12 2017
Looking to see how many tests I break. :) https://chromium-review.googlesource.com/c/chromium/src/+/822819
,
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.
,
Dec 13 2017
Re #21: I expect that display will show decided values in 65 or even 64. See #20.
,
Dec 13 2017
Re #22: Thank you for taking the time to answer that question.
,
Dec 14 2017
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
,
Dec 14 2017
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
,
Dec 15 2017
,
Dec 16 2017
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
,
Dec 22 2017
,
Dec 22 2017
Fixed in m65 via cl in c27.
,
Jan 2 2018
Should we merge this fix to M-64?
,
Jan 2 2018
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
,
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.
,
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.
,
Jan 5 2018
+1 on letting it M65 since it does not seem critical.
,
Jan 9 2018
,
Jan 24 2018
Issue 804867 has been merged into this issue.
,
Feb 18 2018
So, it will be in 65, right?
,
Feb 18 2018
Re #37, as noted in #29, yes, the fix landed in 65.0.3297.0. |
||||||||||||||||
►
Sign in to add a comment |
||||||||||||||||
Comment 1 by krajshree@chromium.org
, Nov 29 2017