Issue metadata
Sign in to add a comment
|
Certain unescaped characters in URL query string break PageInfo links
Reported by
13hu...@gmail.com,
Aug 15
|
||||||||||||||||||||||||
Issue description
Steps to reproduce the problem:
1. Visit a url containing a query string that has one of the following characters (not escaped): | { } %
e.g. https://www.example.com/index.htm?a|b
2. Touch page info (omnibox icon)
What is the expected behavior?
Link to site settings, and details if https
What went wrong?
Site settings and details links are missing
Did this work before? N/A
Chrome version: 68.0.3440.91 Channel: stable
OS Version: 6.0.1
Flash Version:
Other characters may also cause this.
,
Aug 15
Was able to reproduce on stable (only on Android).
,
Aug 15
,
Aug 15
Looks like the issue is around here https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chrome/browser/page_info/PageInfoController.java?rcl=82b85f89b3cc8affd99138fee566b7a17a6ffb59&l=208, if the URL fails to parse the button does not show up.
,
Aug 16
Looks like the issue here is that the URI constructor requires the input to already be sanitized if using the single argument string constructor, otherwise it throws an exception.
,
Aug 16
tedchoc: Is there a preferred way of sanitizing URLs in Clank for a case like this? It looks like the parsed URI is not being displayed, it is just used for deciding when to show the site settings button and the connection details link, so just replacing the invalid characters before calling new URI() would do the trick, but I'm not too familiar with Clank so I don't know what the best way to do this would be. Happy to take this bug and implement the fix once I figure that out.
,
Oct 7
Perhaps you could use URLEncoder (https://developer.android.com/reference/java/net/URLEncoder) to encode the URL first? That should percent-escape special characters like |{}%. It has pretty sparse use in Clank though....
,
Oct 9
Assigning to huayinz@ to make the switch. We likely should just call: https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chrome/browser/util/UrlUtilities.java?q=UrlUtilities.java&sq=package:chromium&g=0&l=113 to get the scheme. They mention the exact issue here with invalid characters. Alternatively, we could use this too: https://cs.chromium.org/chromium/src/net/android/java/src/org/chromium/net/GURLUtils.java?q=UrlUtils.java&sq=package:chromium&g=0&l=32 The former is nicer as it doesn't have native dependencies for robolectric, but we really should converge.
,
Oct 9
,
Oct 12
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c84187c1507e410bbab87c7c18bb49455e04cdb9 commit c84187c1507e410bbab87c7c18bb49455e04cdb9 Author: Becky Zhou <huayinz@chromium.org> Date: Fri Oct 12 19:04:21 2018 Fix site settings button and detail link not shown on page info Fix site settings and detail link not shown on page info under URL with certain unescaped characters. Bug: 874455 Change-Id: Icf31bee1b644136d285c5286938b16bc2b3de88c Reviewed-on: https://chromium-review.googlesource.com/c/1278205 Commit-Queue: Becky Zhou <huayinz@chromium.org> Reviewed-by: Ted Choc <tedchoc@chromium.org> Cr-Commit-Position: refs/heads/master@{#599315} [modify] https://crrev.com/c84187c1507e410bbab87c7c18bb49455e04cdb9/chrome/android/java/src/org/chromium/chrome/browser/page_info/PageInfoController.java
,
Oct 12
|
|||||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||||
Comment 1 by jdonnelly@chromium.org
, Aug 15