https://codereview.chromium.org/2641783003/

contains proposed patch.

Screenshot shows version after change (top app) and before change (bottom app)

Deleted: Screenshot_20170118-221751[1].png 223 KB

	Screenshot_20170118-221751[1].png 223 KB View Download

Note: ktam@ in the https://bugs.chromium.org/p/chromium/issues/detail?id=680673#c11 mentioned, that such format for URLs (but in ContextMenu) is resonable

palmer: Could you weigh in on whether this is something we want or not?

+emily

I can imagine this could be nice for readability but I don't know if there are any other implications that URL decoding may involve

#3: I'll defer to Team-Security-UX, of which I am no longer a member. :)

Patch changes displayed URL only, URL available for copying is not affected, UI/UX shouldn't be affected in any way too

estark, I'm assigning to you to evaluate.

+lgarron to help triage

What does the omnibox show? 
There are a lot of URL formatting Unicode edge cases, and if the URL bar shows percent-encoded then I think we should also do that here.

> What does the omnibox show? 
> There are a lot of URL formatting Unicode edge cases, and if the URL bar shows > percent-encoded then I think we should also do that here.

I propose here displaying in human readable format (and displaying the same in context menu with https://bugs.chromium.org/p/chromium/issues/detail?id=680673) and copying "as is" (it can be in both formats). The same in URL TextView it can be in both formats (it's sensible).

Is there any security issue here ? Or not ? Can be patch accepted ?

Human readable format is much easier/shorter and Chrome should be light and easy, right ?

hi,

can I ask for any decision/comment/update here ?

Hi,

Since 18 Jan I don't see strong voices against proposed change. It's using Uri.decode available since API 1 and doesn't change any deep functionality, just only string displayed to user.

I see seven people involved, can somebody give LGTM ?

I'm still against this change unless you can demonstrate that it is not vulnerable to spoofing/RTL issues like the omnibox (we have a whole bunch of random stuff like not allowing Unicode lock icons).

If you're confident that there are no such issues, could you point us at relevant documentation for it?

> I'm still against this change unless you can demonstrate
> that it is not vulnerable to spoofing/RTL issues like the
> omnibox (we have a whole bunch of random stuff like not
> allowing Unicode lock icons).
> If you're confident that there are no such issues,
> could you point us at relevant documentation for it?

Internal processing URLs - not affected (the same URLs are inside Chrome and the same are copied to user)

Creating decoded URL string and displaying text in our TextView - affected

Used function - https://developer.android.com/reference/android/net/Uri.html#decode(java.lang.String)

This function is ALREADY used in code (ExternalNavigationHandler.java)

Do you expect me to provide some JUnit tests ? If yes, on what URLs list ? Or do you expect me to provide some Uri.decode or TextView analysis ? Could you be a little bit precise ?

I don't understand why you still speak for example about spoofing - we just take different string and display it. No scripts or web processing.

We need to be confident that this will not have Unicode bugs like:

https://bugs.chromium.org/p/chromium/issues/list?can=1&q=unicode+component%3AUI%3EBrowser%3EOmnibox+&colspec=ID+Pri+M+Status+Owner+Summary+OS+Modified&x=m&y=releaseblock&cells=tiles
https://bugs.chromium.org/p/chromium/issues/list?can=2&q=RTL+component%3AUI%3EBrowser%3EOmnibox+&colspec=ID+Pri+M+Status+Owner+Summary+OS+Modified&x=m&y=releaseblock&cells=tiles

Showing a URL in Unicode instead of ASCII lends itself to spoofing unless you take specific precautions, and precautions in the omnibox are a result of a lot of years of finding edge cases.

So, for example, if the decoded URL contains RTL characters, will those be decoded? Will RTL runs be displayed in RTL? Will they flip any text around them? Can they affect the origin? What about whitespace characters?
If the URL is not truncated, can you spoof some of the UI using a long URL? (Do you show Unicode emoji? Can you spoof the lock icon?) If the URL is truncated, is it possible to use RTL to truncate in a way that appears to display the wrong origin?

Perhaps the answers to those are simple, but I don't have enough information in front of me to make that judgment. :-(

> We need to be confident that this will not have Unicode bugs like:
[...]
> Perhaps the answers to those are simple, but I don't have enough information
> in front of me to make that judgment. :-(

Short answer:

currently: get URL & process it for displaying

proposed: get URL & (decode/replace % with Unicode chars) & process it for displaying

I don't cut-off any processing (it handles Unicode chars like currently), just add one function in the middle: "Decodes '%'-escaped octets in the given string using the UTF-8 scheme. Replaces invalid octets with the unicode replacement character ("\\uFFFD")."

If current processing doesn't handle something correctly, it's done even now and additional function in the middle doesn't change anything.

Long answer: is still required ?

Looks like there's already a Java function for what you're trying to do: https://cs.chromium.org/chromium/src/components/url_formatter/android/java/src/org/chromium/components/url_formatter/UrlFormatter.java?type=cs&q=formatUrlForDisplay+java+package:%5Echromium$&l=48

We currently use the url_formatter for formatting the text in the status bubble on desktop (which is the URL shown in the bottom left corner when you hover over a link): https://cs.chromium.org/chromium/src/chrome/browser/ui/views/status_bubble_views.cc?q=statusbubble+package:%5Echromium$&dr=CSs&l=765

So I could see this working there and it includes the necessary Unicode logic already.

> We currently use the url_formatter for formatting the text in the status bubble on desktop (which is the URL shown in the bottom left corner when you hover over a link)

Note that the status is not a security surface [1][2], since it can be easily spoofed. Its current behaviour should not be taken as security-approved behaviour for displaying URLs.

However, if the Java URL formatter is used in the omnibox on Android, that would probably be enough to support using it here in the same way, because then Unicode bugs (that don't depend on multiple lines) would also manifest in the URL bar. Do you know if that's the case?

[1] https://sites.google.com/a/chromium.org/dev/Home/chromium-security/security-faq?pli=1#TOC-Where-are-the-security-indicators-located-in-the-browser-window-
[2] https://sites.google.com/a/chromium.org/dev/user-experience/status-bubble

1. on desktop it's probably wrong implementation of the status bubble rendering.
2. I've put 2nd patch version with using url_formatter. It seems to work fine and of course display correctly URLs (it seems to be totally not connected with status bubble behaviors)

LGTM?

No, I don't believe this is used in omnibox, but it is used on a variety of other surfaces such as infobars, notifications, and such.

Marcin, what does the omnibox look like on this url page? Are the characters shown decoded there?

Patch was discussed from every possible side and is using Chrome function, but nobody want to LGTM.

Should I provide something else or what ?

marcin: thanks very much for the patch. Please bear with us a little longer while we figure out the right thing to do here.

Re comment 23: In the omnibox %-encoded characters are decoded (see screenshot). I think we should match the omnibox; however, it seems that using formatUrlForDisplay will have other possibly unintended consequences like stripping the protocol from http:// URLs and stripping embedded credentials. (To me it looks like formatUrlForDisplay isn't actually used anywhere right now?)

It looks like the code that formats the omnibox URL is here: https://cs.chromium.org/chromium/src/components/toolbar/toolbar_model_impl.cc?type=cs&l=38

But I'm not sure at the moment how to best reuse that code for the Android page info bubble.

Sidenote: why do we show the full URL in the page info bubble? Could we get away with just showing the origin?

Deleted: Screenshot_20170221-132950.png 332 KB

	Screenshot_20170221-132950.png 332 KB View Download

> Please bear with us a little longer while
> we figure out the right thing to do here.

[...]

> Sidenote: why do we show the full URL in the 
> page info bubble? Could we get away with just showing the origin?

There are two places: this menu and context menu (see https://bugs.chromium.org/p/chromium/issues/detail?id=680673). Let's make them consistent and:

displaying URLs in user friendly form AND

copying URLs "as is"

Users will have something easy to read if necessary + exact URLs when needed.

WDYT? 

For hiding the path from the URL, see Issue 567469.

> copying URLs "as is"

The URL in Page Info is not copyable, though?

> For hiding the path from the URL, see Issue 567469.

I don't know, what 567469 has to current patch, but current "short press for showing/hiding" full url is optimal.

> The URL in Page Info is not copyable, though?

It is, press longer on it. Patch doesn't change it and you copy url "as is"

because of it I really don't see any problem and need of discussing it ;)

Any updates here?

Sorry, I lost track of this bug.

The omnibox code that I pointed to in Comment 25 uses url_formatter::FormatUrl, with net::UnescapeRule::NORMAL. It seems to me that we should use that for the Page Info URL, to match the omnibox. lgarron, WDYT?

(Note though it might be a bit of extra work to expose that function to Java.)

I see big misunderstanding here.

In #1 there is proposed, that popup info should try to decode %-sequences to give user nice info. It's Jan 18.

Later used in patch function is criticized and replaced with something used to display URLs in status bar in desktop.

In mean time we see few comments suggesting, that people are not sure what is implemented (for example - they don't know, that URL can be expanded)

And now on May 1 we see comment, that omnibox and page info URL should be same.

Did I miss something?

They're the same NOW and patch is about making them different to help user.

Re #31: I don't understand your comment. Right now it seems that the omnibox shows %-decoded characters (see my screenshot in comment 25). Why do you say they are the same, and why would we want to make them decode differently?

> They're the same NOW and patch is about making them different to help user.

Then I think that the current behaviour *is* reasonable. The URL bar is already supposed to be helpful the user (to the extent that anything is visible in the URL bar on the device), while balancing certain security requirements to protect against spoofing.

If there is a way to unescape some characters that is both safe and useful to the user, we should probably also do it in the omnibox (inc. on desktop). If not, I think we need a very good reason for them to be different, and solid documentation to go with it.

The patch doesn't contain a TEST= case, and I only see one example in a screenshot in the bug. Do we have compelling examples or use cases? Any stats on how often this would affect the URL?
Are international users frequently running into this issue and want this change?

OK, I will explain it on example.

Now when I do search for "ł" in Google:

In URL in omnibox I can put "ł" or "%C5%82", in popup info I get always "%C5%82"

I propose:

In URL in omnibox I can put "ł" or "%C5%82", in popup info I get always "ł"

Patch is working and it's using already used in Chrome function, there are not known any problems.

Is there something what we need to discuss more in this case?

> Right now it seems that the omnibox shows %-decoded characters (see my screenshot in comment 25).

I thought this wasn't the case, but it seems I'm wrong. I can get URLs to keep percent-encoded characters, but not for anything I know to be a print character.

(We still need to be careful about e.g. emoji.)
I'll try to figure out what the Android URL bar is currently using to format the URL.

I pointed to the android omnibox url formatting code in comment 25. I think we should use that, as I proposed in comment 30.

I think, that we are going a little bit outside main topic, which is making Chrome more user friendly...

Once again:

Now when I do search for "ł" in Google:

In URL in omnibox I can put "ł" or "%C5%82" & in popup info I get always "%C5%82"

I propose:

In URL in omnibox I can put "ł" or "%C5%82" & in popup info I get always "ł"

This is quite consistent with desktop behavior. Changing URL in omnibox is probably not good idea, but displaying something nice in popup URL - why not ? (especially that we do it on desktop)

Any sensible arguments against ?

Re #37: I think lgarron has already made several sensible arguments against doing this haphazardly for user-friendliness. Displaying decoded characters can become a security problem. We are not looking to making any changes to the URL display in the omnibox. Instead we are looking to reuse the code that the omnibox uses, so that Page Info and the omnibox display the same URL.

estark@: My bad, I didn't re-read the thread thoroughly enough. :-(

I dug into the code to check a bit, and you're right about everything. I support your proposal.

We need to expose something new for this from C++ to Java, right?

> I think lgarron has already made several sensible 
> arguments against doing this haphazardly for user-friendliness. 
> Displaying decoded characters can become a security problem. 

We have seen, that the way of displaying status bar in desktop Chrome is allowing to display something else. The function UrlFormatter.formatUrlForDisplay itself is not problem, you can easy enter mentioned by lgarron pages and see it.

> We are not looking to making any changes to the URL display in the omnibox. 

Good

> Instead we are looking to reuse the code that the omnibox uses, 
> so that Page Info and the omnibox display the same URL.

Bad, Chrome should be user friendly and innovative.

Re #40:
> We need to expose something new for this from C++ to Java, right?

Yes, I think maybe a new function in components/url_formatter/url_formatter_android.cc that sets the correct options.

Re #41:

> We have seen, that the way of displaying status bar in desktop Chrome is allowing to display something else.

As explained in comment 21, the status bubble is well-documented as *not* a security UI surface and so it doesn't have the same considerations. For Page Info, we want to use the same function (with the same arguments) as the omnibox uses, which is a security surface -- not the status bubble, which is not a security surface.

> Bad, Chrome should be user friendly and innovative.

I think you are misunderstanding the discussion. We are trying to figure out exactly how to safely implement the behavior that you would like Chrome to have, which is %-decoded characters in Page Info.

> I think you are misunderstanding the discussion. 
> We are trying to figure out exactly how to safely implement 
> the behavior that you would like Chrome to have, which 
> is %-decoded characters in Page Info.

"ł" = "%C5%82"

when I have URL in omnibox with "ł", will I see "ł" or "%C5%82" in popup info ?

when I have URL in omnibox with "%C5%82", will I see "ł" or "%C5%82" in popup info ?

I'm not actively working on this, but I think Comment 30 is what we should do here.

 Issue 705233  has been merged into this issue.

682393 and 705233 are related to totally different app places and can be differently handled, please unduplicate.