Re: Better build-time predefined protocol handlers

[Frédéric Wang]

Hello,

I'm forwarding this email from Delan about better build-time predefined
protocol handlers.

We already checked with API owners, and they don't think approval is
needed for already-allowed schemes (that's the case for ipfs and ipns
since
https://groups.google.com/u/1/a/chromium.org/g/blink-dev/c/7nHTRUP1EGY/m/xPtvFdWDAwAJ
)

Any thoughts about how we can proceed here ?

Thanks,

Le 07/05/2021 à 11:59, Delan Azabani a écrit :
> Custom protocol handlers allow URL schemes to be “redirected” to a
> registered handler over http:// or https://. They often come from web
> or extension content via Navigator#registerProtocolHandler, but some
> embedders and downstream browsers would benefit from a clear place to
> to supply their own default handlers at build time.
>
> For example, the easiest way to add support for ipfs:// and ipns://
> would be to predefine handlers pointing to public gateways, but if
> (say) Edge wanted to do so today, they would need to maintain patches
> that hardcode the handlers in their tree.
>
> We’ve written a prototype [0] showing one possible mechanism, based on
> the same approach that ash-chromeos uses for mailto: and webcal:, made
> configurable with a “global” constant array of scheme-handler pairs.
>
> Where should we go to propose a feature like this and figure out what
> mechanism is most appropriate for downstream customisation, seeing as
> this isn’t really a Blink or web-exposed feature in its own right? Is
> this small enough to discuss directly with OWNERS on a bug or CL?
>
> [0] https://crrev.com/c/2878143
>
> Cheers,
> Delan Azabani
> Web Platform @ Igalia

--
Frédéric Wang

[Yutaka Hirano]

+chrome/browser/custom_handlers OWNERS

 - Adding ipfs and ipns handlers to Chrome

 - Adding ipfs and ipns handlers to all the chromium-based browsers

 - Building a mechanism that each chromium-based browser can register its own handlers

Which is your priority?

[Frédéric Wang]

Hi,

I guess the ideal would be

"ipfs and ipns handlers to all the chromium-based browsers"

However, maybe there is not a consensus about this or it can be difficult to reach so it would instead be perfectly fine to start with

"a mechanism that each chromium-based browser can register its own handlers"

so that developers of each chromium-based browser can easily decide or not to register ipfs/ipns by default, without having to maintain patches to the Chromium code. About the details of such a mechanism, we are not familiar enough with how this kind of thing is generally done (build-time flag, runtime-flag, external configuration file, ??) and would welcome suggestions.

Our client mentioned that several Chromium-based browsers are interested in this feature and could join the discussion.

Thanks,

-- 
Frédéric Wang

[Ryan Sleevi]

So we already have an extensible way for embedders to indicate that they handle a particular scheme, and that exists at the //content layer.

ContentBrowserClient's HasCustomSchemeHandler and ContentBrowserClient's IsHandledURL, as well as AddAdditionalSchemes.

This question seems to be asking "How can embedders customize the //chrome layer", and I thought the explicit policy was that "//content is for embedders, //chrome is for Chrome and something you need to patch if you're diverging from Chrome"

Have I misunderstood the policies around //content and //chrome or the question here?

--
You received this message because you are subscribed to the Google Groups "net-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to net-dev+u...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/net-dev/16df3e7f-db23-b7f4-60b5-20f33e1c825e%40igalia.com.

[Dominick Ng]

As Ryan notes, the general approach is that diverging at the //chrome layer typically requires patches.

That being said, if there is a sensible refactor to the custom handler codebase that accomplishes the goals you want, I'm open to discussing whether that makes sense to go ahead.

The next step would likely be for you to write a design document explaining what the change looks like and what implications it would have for the Chromium codebase. For instance, if your proposal is an improvement over the current implementation (more maintainable, more performant, easier to understand, etc.), then that would help tip the scales towards accepting such a change.

[Javier Fernandez]

Hi Dominik,

Thanks for the feedback.

I've been analyzing Ryan's suggestion and I believe I have a better understanding of how embedders should proceed to support new schemes.  It seems that implementing the ContentClient and ContentBrowserClient is the right approach, as Ryan mentioned.

However, if I've understood it correctly, embedders should  be responsible of implementing their own protocol handlers logic, including registration and URLThroattle. I wonder whether it'd make sense to apply some refactoring to the current Chrome custom handler codebase and move it to the content layer, so that the embedders' implementation could benefit from it; as a matter of fact, I think there is already a bug to do this work, https://crbug.com/1139176. I'd like to get some opinions about this idea and, if possible, suggestions and/or ideas of possible approaches to do it as smoothly as possible.

Additionally, I think that moving the custom handlers logic to content would also have a positive impact on the security design, simplifying the security checks logic. Maybe Fred can comment further on this.

I'm far from being able see all the implications of this task, but if it sounds sensible, I'm willing to continue the analysis and provide a design doc we can iterate on and continue the discussion based on it.

[Luc Huynh]

Vào 18:25, Th 4, 26 thg 5, 2021 Javier Fernandez <jfern...@igalia.com> đã viết:

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/net-dev/1e981814-b206-76d0-e082-b05c31f532c9%40igalia.com.

[Kinuko Yasuda]

Hi Javier,

On Wed, May 26, 2021 at 8:25 PM Javier Fernandez <jfern...@igalia.com> wrote:

Hi Dominik,

Thanks for the feedback.

I've been analyzing Ryan's suggestion and I believe I have a better understanding of how embedders should proceed to support new schemes.  It seems that implementing the ContentClient and ContentBrowserClient is the right approach, as Ryan mentioned.

However, if I've understood it correctly, embedders should  be responsible of implementing their own protocol handlers logic, including registration and URLThroattle. I wonder whether it'd make sense to apply some refactoring to the current Chrome custom handler codebase and move it to the content layer, so that the embedders' implementation could benefit from it; as a matter of fact, I think there is already a bug to do this work, https://crbug.com/1139176. I'd like to get some opinions about this idea and, if possible, suggestions and/or ideas of possible approaches to do it as smoothly as possible.

There are things that can live in //content but are currently implemented outside //content for some historical reasons, and I agree that custom protocol handler hooks can be one of these.  Do you have some rough idea about how the //content API for embedders would look? It'd probably make this discussion easier.

Reg: the specific issue you cited, it's about navigator.registerProtocolHandler(), which is entirely implemented by embedders today via WebContentsDelegate::RegisterProtocolHandler.  This needs to be called by web pages (from the renderer) and I'm not sure if that's the hook you're looking for.

[Frédéric Wang]

> Additionally, I think that moving the custom handlers logic to content
> would also have a positive impact on the security design, simplifying
> the security checks logic. Maybe Fred can comment further on this.

To be more specific about this, below are the validations performed. For
security reasons, they have to be duplicated on both processes (*).
However, the browser process validation is split between //content and
//chrome. I think Javi's proposal to move things to //content would
allow to have browser process validation in one place.

Web process : Done in Blink

- custom scheme -- VerifyCustomHandlerScheme
 
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/navigatorcontentutils/navigator_content_utils.cc;l=107;drc=9fcc9d7b2915b6192ee6810eec54c50deb6313c6

- url syntax -- VerifyCustomHandlerURLSyntax
 
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/navigatorcontentutils/navigator_content_utils.cc;l=139;drc=9fcc9d7b2915b6192ee6810eec54c50deb6313c6

- url security (url's scheme and same origin) --
VerifyCustomHandlerURLSecurity
 
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/navigatorcontentutils/navigator_content_utils.cc;l=52;drc=9fcc9d7b2915b6192ee6810eec54c50deb6313c6

Browser process : Done in Content and Chrome.

- url security (same origin) -- AreValidRegisterProtocolHandlerArguments
 
https://source.chromium.org/chromium/chromium/src/+/main:content/browser/web_contents/web_contents_impl.cc;l=334;drc=9fcc9d7b2915b6192ee6810eec54c50deb6313c6

- url security (url's scheme) and custom scheme --
ProtocolHandler::IsValid()
 
https://source.chromium.org/chromium/chromium/src/+/main:chrome/common/custom_handlers/protocol_handler.cc;l=69;drc=46bbb9795fcc1934c6cfbec096764f888c4d400a

(*) I had changed this a bit so that both rely on a shared
IsValidCustomHandlerScheme from third_party/blink/common to implement
custom scheme validation.

--
Frédéric Wang

[Frédéric Wang]

Le 27/05/2021 à 02:50, Kinuko Yasuda a écrit :

Reg: the specific issue you cited, it's about navigator.registerProtocolHandler(), which is entirely implemented by embedders today via WebContentsDelegate::RegisterProtocolHandler.

I think the issue lacks clear description, but as I mentioned on the other email the whole implementation of "registerProtocolHandler" spans //blink //content and //chrome.

So for example, I'm not sure embedders relying exclusively on WebContentsDelegate::RegisterProtocolHandler would perform the full validations checks on the browser process? For example in CL 2287304 I only tested same-origin validation in web_contents_impl_unittest.cc (mock WebContentsDelegate) ; the whole test coverage had to be done via InProcessBrowserTest  (from //chrome).

Similarly, IIRC the substitution of the "%" sign is performed in //chrome's ProtocolHandler::TranslateUrl. So how can it even work for embedders ?

This needs to be called by web pages (from the renderer) and I'm not sure if that's the hook you're looking for.

Right, we don't need navigator.registerProtocolHandler() in our case, but I believe some of the logic in //content & //chrome would be still be shared for implementing custom handlers? And it does not seem core custom handler code should be in //chrome...

-- 
Frédéric Wang

[Javier Fernandez]

Hi,

On 27/5/21 2:50, Kinuko Yasuda wrote:

Hi Javier,

On Wed, May 26, 2021 at 8:25 PM Javier Fernandez <jfern...@igalia.com> wrote:

Hi Dominik,

Thanks for the feedback.

I've been analyzing Ryan's suggestion and I believe I have a better understanding of how embedders should proceed to support new schemes.  It seems that implementing the ContentClient and ContentBrowserClient is the right approach, as Ryan mentioned.

However, if I've understood it correctly, embedders should  be responsible of implementing their own protocol handlers logic, including registration and URLThroattle. I wonder whether it'd make sense to apply some refactoring to the current Chrome custom handler codebase and move it to the content layer, so that the embedders' implementation could benefit from it; as a matter of fact, I think there is already a bug to do this work, https://crbug.com/1139176. I'd like to get some opinions about this idea and, if possible, suggestions and/or ideas of possible approaches to do it as smoothly as possible.

There are things that can live in //content but are currently implemented outside //content for some historical reasons, and I agree that custom protocol handler hooks can be one of these.  Do you have some rough idea about how the //content API for embedders would look? It'd probably make this discussion easier.

I'm still catching up with the codebase and its design, but my preliminary rough idea would be to move part of the  ProtocolHandler, ProtocolHandlerRegistry and ProtocolHanderRegistryFactory to //content, so that embedders could implement abstract APIs to add default protocol handlers during the initialization. Ideally, embedders would rely on the ProtocolHandlerRegistry to implement IsHandlerdProtocol, like ChromeContentBrowserClient currently does. I think it'd be useful for embedders that they could use the ProtocolHandleThroattle class as well, especially the TranslateURL function.
 
Probably the OS integration and preference management should still live in //chrome, hence embedders must implement their specific subclasses of ProtocolHandler and ProtocolHandlerRegistry to handle this. appropriately.

Reg: the specific issue you cited, it's about navigator.registerProtocolHandler(), which is entirely implemented by embedders today via WebContentsDelegate::RegisterProtocolHandler.  This needs to be called by web pages (from the renderer) and I'm not sure if that's the hook you're looking for.

Sure, that's not the hooks I'm interested on. The basic use case I'm interested on, for now, is allow embbeders to declare additional default protocol handlers without asking users to install additional Web Extensions. The new design must also simplify the security and performing the full validations checks on the browser process.

--
javi

[Javier Fernandez]

Hi,

On 17/5/21 4:56, Dominick Ng wrote:

> As Ryan notes, the general approach is that diverging at the //chrome
> layer typically requires patches.
>
> That being said, if there is a sensible refactor to the custom handler
> codebase that accomplishes the goals you want, I'm open to discussing
> whether that makes sense to go ahead.
>
> The next step would likely be for you to write a design document
> explaining what the change looks like and what implications it would
> have for the Chromium codebase. For instance, if your proposal is an
> improvement over the current implementation (more maintainable, more
> performant, easier to understand, etc.), then that would help tip the
> scales towards accepting such a change.
>

This is my proposal for a design change to move the protocol handler
logic to //content layer.

https://docs.google.com/document/d/1eRAUFlNargC0GS9Zr9SC3bq_RK4Mt6rod5SU72hiRsg/edit?usp=sharing

I'd appreciate any feedback.
Thanks.

--
javi

[Kinuko Yasuda]

Hi,

Thanks for the doc, opinions from the custom_handlers owners (like Dominick) would be important here, but at a quick look it looks like at least some core parts can likely reasonably live in //content to me.

Could you maybe also open up comments permission on the doc for some of us?

[Javier Fernandez]

Hi,

On 9/6/21 15:05, Kinuko Yasuda wrote:

Hi,

Thanks for the doc, opinions from the custom_handlers owners (like Dominick) would be important here, but at a quick look it looks like at least some core parts can likely reasonably live in //content to me.

Could you maybe also open up comments permission on the doc for some of us?

I've sent invitation to edit to the people that contributed to this thread, so far.
I'd be glad to add anyone else interested on the issue.

--
javi

[Dominick Ng]

I took a look at the design doc and I haven't seen any major blocking concerns. I do know that Microsoft is currently doing a lot of work on the protocol handler code for PWAs in particular - +cc Fabio and Daniel who may also have thoughts on potentially moving the bulk of the custom handler code from //chrome to //content.

[Javier Fernandez]

Hi Dominick,

On 10/6/21 8:15, Dominick Ng wrote:
> I took a look at the design doc and I haven't seen any major blocking
> concerns. I do know that Microsoft is currently doing a lot of work on
> the protocol handler code for PWAs in particular - +cc Fabio and
> Daniel who may also have thoughts on potentially moving the bulk of
> the custom handler code from //chrome to //content.
>

Thanks for the feedback. It's great to know that there are no blockers
to the proposal; I understand that there are some issues that need to be
addressed, but I think we can do it as part of the regular review
process. I just wanted to know this work was useful and worth pursuing it.

I'd like to get feedback from Microsoft, as you mention (I'll give edit
permissions to those contacts you've suggested). I'd also be interested
to get opinions from other Chromium embedders, like Brave. As far as I
know Brave did a considerable work implementing custom protocol handlers
for their browser and this refactoring could have a relevant impact
(hopefully positive) on their design.

--
javi

[Fabio Rocha]

Thanks for the document Javier and Dominick for sharing.

I had a look at the document and there are also no concerns from our side.

[Daniel Buchner]

I'd be happy to provide feedback and collaborate on this from the Microsoft side (I'm the Daniel folks above were referring to)

[Daniel Buchner]

Turns out we may have gotten our Daniels crossed, but I would like to participate here, so I will take a look at the current bits and post about our use cases (if they are found to be relevant)

[Daniel Murphy]

[Javier Fernandez]

Hi,

Thanks everybody for the feedback and the positive signals about the
proposal.
I'll proceed with the work and we'll discuss the specific issues when
needed, either in the design doc of here, in a new thread.

Thanks,

javi

[Javier Fernandez]

+embedd...@chromium.org

Hi,

I've started to implement the design we discussed back in June and I
landed a few patches already. The Protocol Handler class has been moving
to //content already (see the CL [1] for details) but we've found some
obstacles with the refactoring to move the ProtocolHandlerRegistry class
(again, this is the CL [2] with the details).

I want to thank you for all the kind and useful reviews, which
eventually led to the conclusion that we may need a new iteration on the
discussion about the design document [3] I shared back then and the
approach I've followed to implement it. 

The main issue to discuss now is how to properly perform the refactoring
so that embedders can get advantage of the Protocol Handler logic while
still provide the implementation of the current //component interfaces.
Since //content can't depend on //component, the new design proposed to
define some specialized classes in //chrome to do it. However, this
seems not the ideal solution and it causes some layering issues
(explained in the design doc).

The alternate approach proposed in the CL reviews is basically to move
the Protocol Handler logic directly to //components.

Thanks.

[1] https://crrev.com/c/2923204
[2] https://crrev.com/c/2992306
[3]
https://docs.google.com/document/d/1eRAUFlNargC0GS9Zr9SC3bq_RK4Mt6rod5SU72hiRsg/edit?usp=sharing

--
javi

[Colin Blundell]

Thanks, Javier! I left comments on the doc.