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

feat: add protocol.handle #36674

Merged
merged 40 commits into from Mar 27, 2023
Merged

feat: add protocol.handle #36674

merged 40 commits into from Mar 27, 2023

Conversation

nornagon
Copy link
Member

@nornagon nornagon commented Dec 15, 2022

Description of Change

This adds a new method to the protocol module, protocol.handle, which replaces the existing register{Buffer,String,Stream,File,Http}Protocol and intercept{Buffer,String,Stream,File,Http}Protocol methods.

See electron/governance#368 for motivation and further details, though this PR has progressed the design slightly.

Checklist

Release Notes

Notes: Added protocol.handle, replacing and deprecating protocol.{register,intercept}{String,Buffer,Stream,Http,File}Protocol.

@nornagon nornagon added no-backport semver/minor backwards-compatible functionality labels Dec 15, 2022
@electron-cation electron-cation bot added new-pr 🌱 PR opened in the last 24 hours api-review/requested 🗳 labels Dec 15, 2022
@nornagon nornagon marked this pull request as draft December 15, 2022 23:38
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Dec 22, 2022
ABCDEV666

This comment was marked as spam.

@nornagon nornagon marked this pull request as ready for review March 8, 2023 22:56
@nornagon nornagon requested a review from a team as a code owner March 8, 2023 22:56
@nornagon nornagon requested a review from a team as a code owner March 8, 2023 23:55
Copy link
Contributor

@jkleinsc jkleinsc left a comment

Choose a reason for hiding this comment

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

API LGTM

Copy link
Member

@zcbenz zcbenz left a comment

Choose a reason for hiding this comment

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

API LGTM

@nornagon
Copy link
Member Author

Have been testing out various URL canonicalization things and discovered a few quirks that I think merit closer attention.

  1. protocol.handle() is currently unable to handle URLs with a username or password in them, e.g. http://user@host.com. This is because undici's implementation of Request, in line with the DOM implementation, refuses to be constructed with a URL that includes credentials.

    These kinds of URLs are being phased out, so this might not be too much of an issue in the long run, but right now attempting to load such a URL results in a TypeError being thrown in Electron's internals and the request hanging, which isn't ideal.

  2. URLs for "standard" protocols must have a host. That means if your app scheme is registered as "standard", then you must also have a host in your URL, e.g. app://host/foo.

    There are lots of weird side effects of not being a "standard" protocol. For instance, app:foo, app:/foo, app://foo and app:///foo all behave differently. app:///foo will load, but relative URLs won't work, because Chromium counts the slashes and decides that the URL is "authority based" and tries to interpret it as having a host name, which fails. app://foo will load, but in your handler when you try to parse the URL with new URL, you'll get foo as the hostname and / as the pathname, and relative URLs still won't work. app:/foo seems to work roughly correctly, except that the script-src 'self' CSP doesn't work as expected, I think because there is no "origin" to match. Setting the CSP to script-src app: seems to work, and even allows fetch() to work if enabled when registering the protocol. app:foo seems to behave the same as app:///foo, for related reasons.

    I will spare you the details of the way that c:/foo.txt is automatically interpreted as file://c:/foo.txt on Windows.

  3. Standard schemes are always registered as having a scheme type of SCHEME_WITH_HOST, meaning they cannot include a port. I'm not sure this matters terribly, but it might be something that would be useful if you were trying to create an "HTTP-like" scheme, or something of that nature (e.g. gopher://<host>:<port>). As such, if you register your custom protocol as a 'standard' scheme, any port info in URLs will be stripped. We could extend protocol.registerSchemesAsPrivileged to allow choosing other scheme types for standard schemes.

I'm not really sure what to do about all this. It seems pretty confusing, and I don't even think I've covered all the quirks of standard vs non-standard schemes. Perhaps it all just needs to be well documented?

Thoughts?

@nornagon
Copy link
Member Author

i noticed a lot of what i noted above is already documented at https://www.electronjs.org/docs/latest/api/protocol#protocolregisterschemesasprivilegedcustomschemes, so maybe this isn't as big of an issue as i thought.

@nornagon nornagon merged commit fda8ea9 into main Mar 27, 2023
11 checks passed
@nornagon nornagon deleted the protocol-handle branch March 27, 2023 17:00
@release-clerk
Copy link

release-clerk bot commented Mar 27, 2023

Release Notes Persisted

Added protocol.handle, replacing and deprecating protocol.{register,intercept}{String,Buffer,Stream,Http,File}Protocol.

@QiuFeng54321

This comment was marked as resolved.

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

Successfully merging this pull request may close these issues.

None yet

5 participants