bsittler@ - can you take a look?

Oops, actually assign to bsittler@

Indeed, it looks like we are using the userinfo percent-encode set where we should use the path percent-encode set according to https://html.spec.whatwg.org/multipage/system-state.html#dom-navigator-registerprotocolhandler

Repro steps:

1. Navigate to https://example.org/robots.txt

2. Open JS console and run:

navigator.registerProtocolHandler('web+example', 'https://example.org/search?q=%s', 'web+example: example.org search')

3. "Allow"

4. Navigate to:

web+example://custom/protocol+handler|something%20else

5. Inspect the address bar

Actual:

https://example.org/search?q=web%2Bexample%3A%2F%2Fcustom%2Fprotocol%2Bhandler%7Csomething%2520else

Expected:

https://example.org/search?q=web+example://custom/protocol+handler|something%20else

Actually, it's even worse - we are also escaping % to %25, which even the userinfo percent-encode set would not escape

Misbehavior is not OS-specific

Hi Gyuyoung! I see you recently updated the protocol handler code to improve standards-conformance. Is it possible your change also changed escaping behavior, or would you know what might have changed it recently? Thanks, -Ben

Hi Kentaro, any idea whether recent protocol handler changes you reviewed might have changed this, and (if so) whether the change was intentional?

bsittler@, It might be caused by USVString - https://chromium-review.googlesource.com/c/chromium/src/+/781320

Let me take a look it today.

Hmm, I tried to reproduce this issue on Chrome 62 as well. But this issue was also reproduced. Besides, Firefox also changes the URL to https://example.org/search?Q=web%2Bexample%3A%2F%2Fcustom%2Fprotocol%2Bhandler%7Csomething%2520else.

rkilarski@, could you let me know what Chrome version worked you expected ?

Hello, the current Stable version 63.0.3239.108 and previous versions all work fine. But the current beta and dev versions encode the URI string in my custom protocol handler. 

Rich

bsittler@, Rich, it seems to me that this percent-encoded URL has been doing by net::EscapeQueryParamValue() in ProtocolHandler::TranslateUrl.

https://cs.chromium.org/chromium/src/chrome/common/custom_handlers/protocol_handler.cc?q=protocol_handler.cc&sq=package:chromium&dr&l=59

However, I think this conversion has been done for a long time. So I don't understand why this issue happened recently.

Anyway, if we don't call net::EscapeQueryParamValue(), it looks it works well.

   base::ReplaceFirstSubstringAfterOffset(
-      &translatedUrlSpec, 0, "%s",
-      net::EscapeQueryParamValue(url.spec(), true));
+      &translatedUrlSpec, 0, "%s", url.spec());

I upload a patch to fix this issue - https://chromium-review.googlesource.com/c/chromium/src/+/835908

However, I'm not 100% sure if this is correct fix or not yet. Let me take a look this further.

Rich, thanks for your issue report and follow-up. Can you provide any further details about the protocol handler you are registering?

I'm actually seeing the same behavior from #3 all the way back in Stable channel:

https://example.org/search?q=web%2Bexample%3A%2F%2Fcustom%2Fprotocol%2Bhandler%7Csomething%2520else

Google Chrome	63.0.3239.108 (Official Build) (64-bit)
Revision	d2626860fae283daee484943e6820af18fc73fd9-refs/branch-heads/3239@{#676}
OS	Linux
JavaScript	V8 6.3.292.48
Flash	24.0.0.189 internal-not-yet-present
User Agent	Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/63.0.3239.108 Safari/537.36

Thank you, Gyuyoung! I really appreciate your efforts to track down the bug and fix this, and I left a comment on that review. I don't see evidence with my own testing that this behavior changed recently, but it's still true that our current behavior does not seem in agreement with the spec.

Do you think it's worth doing interop testing to figure out whether it is the spec not reflecting reality or our implementation behavior being nonstandard here?

Firefox 52.5.0 (ESR, I believe) navigates to:

https://example.org/search?q=web%2Bexample%3A%2F%2Fcustom%2Fprotocol%2Bhandler%7Csomething%2520else

... although it is displayed with | rather than %7C in the Firefox address bar.

Given the apparent lack of spec-compliance here I'm suspecting that either my example is flawed (entirely possible!) or we need to do a round of interop testing, or both.

>Do you think it's worth doing interop testing to figure out whether it is the >spec not reflecting reality or our implementation behavior being nonstandard >here?

IMHO, we should follow the spec first. But, if it's hard to follow the spec in our implementation, we should ask it it the w3c spec group.

>Given the apparent lack of spec-compliance here I'm suspecting that either my >example is flawed (entirely possible!) or we need to do a round of interop >testing, or both.

Yes, when I tested your example, Chrome and Firefox had almost same result. So I wonder what use case are Rich using now. @Rich, could you let us know what is it?

BTW, bsittler@, I replied your comment in the patch. Could you take a look it?

Just now I followed the steps from @3 and I do get consistent results using those steps; stable/beta/dev all encode the URI in that manual "navigate to X" step. 

So I guess I'm doing something different. Maybe I'm doing things wrong, but here's what I'm doing:

0) I have a custom protocol handler that's registered with my OS.
1) In JS, I build a URI, including my parameters. This will launch my EXE to handle it. 
2) Using angular 1.x, I call $window.open, passing in the URI.
3) The exe launches and processes the parameters, etc.

I did this in Chrome stable (63.0.3239.108), beta (64.0.3282.39) and dev (65.0.3298.3).

Stable works fine, my exe is invoked with the URI passed in without encoding it. 

But beta and dev get the URI encoded, so now I have to decode it in my EXE. The ONLY thing that changed was the browser in my mix, everything else is constant (my code, angularjs, my exe, etc). 

Incidentally, I tried it in Firefox (57.0), and it did NOT encode my URI. So it worked as desired, even though we don't yet support Firefox so it's not part of our regular testing.

Thanks for the additional details! How did you register the protocol
handler with the OS?

Oh. Yeah. Found it. Thank you @bsittler for your question which triggered this.

Ever since we could now install Chrome Dev and Chrome Beta concurrently with Chrome Stable, we now have separate areas under \AppData\Local\Google for each of them.

So when I registered my protocol handler (via an installkit), I now need to register all three browsers. Once I did, Dev and Beta started working.

#eggmeetface

So... my issue is resolved. I'm sorry to have caused a to-do here.

I'm glad you solved the issue!

> So... my issue is resolved. I'm sorry to have caused a to-do here.

No, I think we should check if the percent-encoding is correct behavior against the spec. Thanks.

Having looked at the CL, I'm also of the opinion that this is a breaking change. (https://chromium-review.googlesource.com/c/chromium/src/+/835908#message-10c4b80153a13d645e241cece550433c40c47ff6).

Have we looked at how other UAs encode the custom URL? The most pernicious case is the handling of '%'. Perhaps if all/most UAs escape the '%' already in violation of the spec, then we could update the spec to match.

When I tested a below example in Firefox, Firefox changed '+' with "%2B" and ':' with %3A. According to my test both on Firefox and Chrome, they have done same behavior. If you see #c10 and #c16, you can know that Firefox has applied the percent-encode in the destination url.

===============================================================================
<script>
window.navigator.registerProtocolHandler("web+github",
                                         "https://gyuyoung.github.io/CustomSchemeExample/%s/url=%s",
                                         "github handler");
</script>

<a href="web+github:gyuyoung">link</a>
==============================================================================

Destination URL by Firefox
==============================================================================
https://gyuyoung.github.io/CustomSchemeExample/web%2Bgithub%3Agyuyoung/url=%s
==============================================================================

FYI, after CL 835908, the destination URL is - https://gyuyoung.github.io/CustomSchemeExample/web+github:gyuyoung/url=%s. It doesn't change '+', ':' with the matched percent-encoded codes.

Interesting. The "+" isn't a part of any encode sets, and its escaping would make the URL unusable without an additional percent decode step. I'd suspect that FF would need to escape "%" as well.

The destination URL is what's seen by the server, right? A good way to test this is to run it through a proxy and capture the URL as it is presented. I'm concerned that anything that parses the request may affect escaping.

Could you run a quick test with the following characters in the URL?
* U+0020 SPACE, U+0022 ("), U+003C (<), U+003E (>), and U+0060 (`) (fragment encode set)
* U+0023 (#), U+003F (?), U+007B ({), and U+007D (}) (path encode set)
* U+002F (/), U+003A (:), U+003B (;), U+003D (=), U+0040 (@), U+005B ([), U+005C (\), U+005D (]), U+005E (^), and U+007C (|) (userinfo encode set)

If you only do one, try "%" :-). If FF encodes "%" then I think we should go to the spec and change it.

.. by "go to the spec and change it" I mean we should request the spec be updated to match Chrome and FF behavior. Thus consumers of the escaped URLs would need to extract the encoded custom URL string and percent decode it prior to parsing it again as a URI. This process is incompatible with the current spec.

> If you only do one, try "%" :-). If FF encodes "%" then I think we should go to the spec and change it.

I think that we already checked it in #c10. When I tested below one on Firefox as well, it redirected to "https://example.org/search?Q=web%2Bexample%3A%2F%2Fcustom%2Fprotocol%2Bhandler%7Csomething%2520else.". Firefox changed % -> %25.

=============================================================================
navigator.registerProtocolHandler('web+example', 'https://example.org/search?q=%s', 'web+example: example.org search')

3. "Allow"

4. Navigate to:

web+example://custom/protocol+handler|something%20else
=============================================================================

Let's wait for a resolution to https://github.com/whatwg/html/issues/3377.

Thanks bsittler@ for filing the bug.

Ah I think I know "regressed". I fixed the opposite issue in https://crrev.com/c/778747. See  Issue 785809 . (Though that was for *external* protocol handlers, it may affect internal as well.)

TL;DR: The current behaviour (as of Chrome 63) is working as intended, despite disagreeing with the spec. There is a problem with the spec (https://github.com/whatwg/html/issues/3377) and I will be working to fix it.

Reading this thread, there seems to be a misunderstanding about what should happen here.

It is by design that registerProtocolHandler encodes all characters that could in any way be interpreted as part of the URL syntax (such as '?', '&', '=' and '+'). The encoded URL thus ends up being a URL-within-a-URL, making it unambiguous which characters form part of the "outer" URL and which form part of the "inner". Thus, the repro steps in #3, resulting in the following URL:

https://example.org/search?q=web%2Bexample%3A%2F%2Fcustom%2Fprotocol%2Bhandler%7Csomething%2520else

are working as intended.

Consider that on the server side, the URL would be parsed, with the query key-value pair ('q', 'web%2Bexample%3A%2F%2Fcustom%2Fprotocol%2Bhandler%7Csomething%2520else'). That value then gets decoded, e.g., by decodeURIComponent, as "web+example://custom/protocol+handler|something%20else", which is usable as a URL.

> Actually, it's even worse - we are also escaping % to %25, which even the userinfo percent-encode set would not escape

This is working as intended as well (and it's a major problem that the spec version of this API doesn't encode % to %25).

Otherwise, consider the inner-URL "web+example://abc/def%3Fghi". This should encode as "web%2Bexample%3A%2F%2Fabc%2Fdef%253Fghi", and decode back to "web+example://abc/def%3Fghi". If it were encoded according to the spec (path percent encode), it would be "q=web+example://abc/def%3Fghi", which then decodes as "web+example://abc/def?ghi", and the URL has mutated; "ghi" is now part of the query where it was originally part of the path.

(It may be unnecessary to escape all of those '/' characters, though.)

#3:
> Indeed, it looks like we are using the userinfo percent-encode set where we should use the path percent-encode set

We are actually using neither. We're encoding the complement of the RFC 2396 [1] "unreserved" set. This is equivalent to running the string through the encodeURIComponent JavaScript function.

Neither the userinfo percent-encode set nor the path percent-encode set escape enough characters. In particular, neither of them escape '%', '&' or '+', which can result in the meaning of the URL being changed, query parameter escapes, etc.

I'm going to mark this WontFix (Working as Intended) because it would be unacceptable for Chrome to implement the current spec. I will be working with spec authors on https://github.com/whatwg/html/issues/3377 and https://github.com/whatwg/url/issues/369 to fix the spec. It's possible we'll end up with something slightly different to what Chrome does now, and then I'll open a new bug to change Chrome's behaviour to line up with the spec.

[1] https://tools.ietf.org/html/rfc2396

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8332af677f16f911bc77e38886731a2930e6bb55

commit 8332af677f16f911bc77e38886731a2930e6bb55
Author: Gyuyoung Kim <gyuyoung.kim@lge.com>
Date: Thu Feb 08 02:13:53 2018

[registerProtocolHandler] Add a unit test for percent-encoding

Though registerProtocolHandler has been doing percent-encoding
when it merges a base URI and a custom URI based on the custom scheme,
it was not tested by any unit tests. So definitely we need to test
percent-encoding now. But if the spec of custom scheme handler is
updated, we should apply the change to this test as well.

Bug:  795919 
Change-Id: I35a7037b20216ed9fae35704e1c958318e53c1e1
Reviewed-on: https://chromium-review.googlesource.com/899893
Commit-Queue: Gyuyoung Kim <gyuyoung.kim@lge.com>
Reviewed-by: Ben Wells <benwells@chromium.org>
Reviewed-by: Matt Giuca <mgiuca@chromium.org>
Reviewed-by: Asanka Herath <asanka@chromium.org>
Cr-Commit-Position: refs/heads/master@{#535257}
[modify] https://crrev.com/8332af677f16f911bc77e38886731a2930e6bb55/chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc