Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

A different approach to chromium accepting my responses #3

Merged
merged 1 commit into from
Feb 21, 2023

Conversation

John-LittleBearLabs
Copy link
Collaborator

I wonder if adding something into AddAddtionalSchemes in //chrome might be more palatable than the change I was making in navigation_request.cc down in render_host.

⬆️ this is very much the question I want to ask.

It is worth noting, some of the other things in AddAdditionalSchemes are behind build flags, which was something raised as a possibility for this project. So, if we wanted to go that way, it's nice to know there's precedent.

The biggest downside is that this does mean CIDs are getting lowercased again, as they were with the http redirect. So the CIDv0 style (Qm...) are a non-starter as they're case-sensitive. And now that I'm going block-by-block (for ipfs:// anyhow), this is an even bigger problem as, at least for the moment, I'm resolving the binary CIDs in the PB-DAG links as CIDv0. So, if we go this route that's something that must be addressed.

But before I do that, I want to get a consensus on whether this is a worthwhile and good approach.

not navigation_request,
more palatable?
+ for ( const char* ip_s : {"ipfs", "ipns"} ) {
+ schemes->standard_schemes.push_back(ip_s);
+ schemes->cors_enabled_schemes.push_back(ip_s);
+ schemes->secure_schemes.push_back(ip_s);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could also be worth questioning how many of these categories IPFS/IPNS really need to be in.

For my purposes I'm pretty sure they need to be in standard_schemes at the very least.

If they're in secure_schemes, then perhaps we have to ban insecure http gateways (or maybe just non-localhost http?). FWIW, right now I don't believe I have any in the list anyhow.

@John-LittleBearLabs
Copy link
Collaborator Author

@javifernandez - I floated an idea in our Slack, and I'd like to get your take on it. The hope is to handle various CIDs (including those that can't be lowercased and/or include characters that would be illegal in a host), while also not conflicting on an existing scheme (like http, particularly for URLLoaderFactory maps), and not changing anything in render_host.

  • Don't do this kind of registry for ipfs: or ipns schemes. Let them hit the interceptor just as they did (case in-tact).
  • Do register some other scheme that's not already handled in chromium. Perhaps ipfs-http:// or something.
    • Assume its "host" will always be an all-lowercase (probably base32) CID that's the root of a pb-dag.
  • When intercepting ipns, resolve the name, then
  • When intercepting ipfs (or if you just resolved ipns to ipfs), convert whatever CID you have to bafy...
  • Issue an HTTP redirect to ipfs-http

What do you think of this idea?

@javifernandez
Copy link
Collaborator

Hi, at a first shot, using the Content Public API for adding new schemes seems the right approach. When I started working on implementing support for IPFS and IPNS through the Custom Handlers API I've been told that this would be the preferred approach for Embedders.

It seems that Chrome already added the 'isolated-app' scheme to the standard_schemes set. This could be perhaps the model we could get some inspiration from to propose adding IPFS/IPNS as an standard scheme. I find the isolated-app scheme explainer quite interesting.

@John-LittleBearLabs
Copy link
Collaborator Author

Hi, at a first shot, using the Content Public API for adding new schemes seems the right approach. When I started working on implementing support for IPFS and IPNS through the Custom Handlers API I've been told that this would be the preferred approach for Embedders.

It seems that Chrome already added the 'isolated-app' scheme to the standard_schemes set. This could be perhaps the model we could get some inspiration from to propose adding IPFS/IPNS as an standard scheme. I find the isolated-app scheme explainer quite interesting.

@javifernandez - am I to understand you think this PR is a good change? But perhaps doesn't go far enough?

You bring up isolated-app, which is also added to various categories in ChromeContentClient::AddAdditionalSchemes, like my change here.
Whereas the Google Groups comment you link to reads:

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

So maybe I should also modify chrome_content_browser_client.cc's HasCustomSchemeHandler and IsHandledURL?

@javifernandez
Copy link
Collaborator

am I to understand you think this PR is a good change? But perhaps doesn't go far enough?

I believe so. From the technical point of view it made sense to add IPFS / IPNS to some of the scheme lists handled by Chrome. Having said that, adding a Chrome specific scheme is not the same than adding an external one. Our acceptance bar will be much higher, so we must think it carefully. IMHO, it's a line of work worth prototyping, at least to figure out how far we would need to go.

So maybe I should also modify chrome_content_browser_client.cc's HasCustomSchemeHandler and IsHandledURL?

HasCustomSchemeHandler is intended to be used with the ProtocolHandlerRegistry, so unless we want to explore that path, it wouldn't be useful for the approach you are proposing now.

However, IsHandledURL is implemented by the ProfileIOData by checking a hardcoded list of "known schemes". The NavigationRequests uses this API to determine whether it's should be treated as an external protocol. It's also used by the Process Security Policies, but I don't have a clear idea of the implications of this logic for our use case.

@John-LittleBearLabs John-LittleBearLabs merged commit 79a0d59 into main Feb 21, 2023
@John-LittleBearLabs John-LittleBearLabs deleted the planb branch March 6, 2023 00:02
John-LittleBearLabs added a commit that referenced this pull request Nov 8, 2023
# This is the 1st commit message:

Fixes #39

# This is the commit message #2:

covg

# This is the commit message #3:

groundwork
John-LittleBearLabs added a commit that referenced this pull request Nov 15, 2023
# This is the 1st commit message:

Fixes #39

# This is the commit message #2:

covg

# This is the commit message #3:

groundwork
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants