8000 Add protocol.registerSecureSchemes by pfrazee · Pull Request #7671 · electron/electron · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Add protocol.registerSecureSchemes #7671

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Add protocol.registerSecureSchemes #7671

wants to merge 1 commit into from

Conversation

pfrazee
Copy link
Contributor
@pfrazee pfrazee commented Oct 19, 2016

Solves #7670

Per @deepak1556's advice, I added an interface to the ContentClient:: AddSecureSchemesAndOrigins method. Due to the timing in my tests, I found it had to be called before the 'ready' event, as with registerStandardSchemes.

I'm not sure what the best way is to automatically test this. Suggestions? I was able to confirm it works in my bug demo, https://github.com/pfrazee/electron-bug-securewebviewcrash.

@deepak1556
Copy link
Member

There is no need for a separate api, standard schemes can be registered as secure by default (should be done on both browser and renderer). We have already disabled storage for non-standard schemes, so there is no use of making them secure.

A bit unrelated to the PR, @zcbenz can we deprecate and remove protocol.registerServiceWorkerSchemes ? We can enable service workers for standard schemes by default, again doesn't make sense for non-standard schemes.

@pfrazee
Copy link
Contributor Author
pfrazee commented Oct 20, 2016

Are you sure? Wouldn't a scheme that proxies to http, but https, be standard but not secure?

@deepak1556
Copy link
Member

It doesn't matter whether the scheme is authenticated or not. localhost, file are secure schemes and origins. Custom schemes are declared by users and they consider it trust worthy https://www.w3.org/TR/secure-contexts/#is-origin-trustworthy . Is there a scenario where custom scheme cannot be treated secure ?

@pfrazee
Copy link
Contributor Author
pfrazee commented Oct 20, 2016

If the custom scheme is a proxy for HTTP and not HTTPS then it wouldn't be secure.

On Oct 20, 2016, at 4:45 AM, Robo notifications@github.com wrote:

It doesn't matter whether the scheme is authenticated or not. localhost, file are secure schemes and origins. Custom schemes are declared by users and they consider it trust worthy https://www.w3.org/TR/secure-contexts/#is-origin-trustworthy . Is there a scenario where custom scheme cannot be treated secure ?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.

@deepak1556
Copy link
Member
deepak1556 commented Oct 20, 2016 •
edited
Loading

If the custom scheme is a proxy for HTTP and not HTTPS then it wouldn't be secure.

👍

Calling protocol.registerSecureSchemes should also register on renderer side, we should not expect users to take care of registering in both places. I propose removal of secure and serviceWorker options from webframe.registerURLSchemeAsPrivileged and instead register those whenever appropriate browser apis are called. Thoughts ?

@pfrazee
Copy link
Contributor Author
pfrazee commented Oct 21, 2016

@deepak1556 I'm cool with that. Are other Electron members on board?

@zcbenz zcbenz self-assigned this Oct 24, 2016
@zcbenz
Copy link
Contributor
zcbenz commented Oct 24, 2016

Calling protocol.registerSecureSchemes should also register on renderer side, we should not expect users to take care of registering in both places.

👍

I propose removal of secure and serviceWorker options from webframe.registerURLSchemeAsPrivileged and instead register those whenever appropriate browser apis are called.

This would break apps that use registerURLSchemeAsPrivileged to register secure schemes, I prefer to keep it unchanged.

@deepak1556
Copy link
Member

@zcbenz This patch will still crash for non-standard schemes because accessing cache storage apis check for uniqueness of origin and it will fail. The only way I could find to disable cache storage for non-standard schemes is not let them be registered as secure unlike how we disabled other storage apis. If we are disabling cache storage, we should also disable service workers for non-standard schemes.
We can check if origin is standard before registering in protocol.registerSecureSchemes and protocol.registerServiceWorkerSchemes. This should also be carried out in webFrame.registerURLSchemeAsPrivileged but will definitely break existing apps that expect non-standard schemes to be secure, not sure how to fix this problem.

@zcbenz
Copy link
Contributor
zcbenz commented Oct 25, 2016

How about making registerStandardSchemes also register secure schemes by default, and provide an extra options argument for turning that off? Just like what registerURLSchemeAsPrivileged does.

@deepak1556
Copy link
Member

How about making registerStandardSchemes also register secure schemes by default, and provide an extra options argument for turning that off? Just like what registerURLSchemeAsPrivileged does.

That should work! Any thoughts on disabling secure option it in webFrame.registerURLSchemesAsPrivileged for non-standard schemes ?

@zcbenz
Copy link
Contributor
zcbenz commented Oct 25, 2016

That should work! Any thoughts on disabling secure option it in webFrame.registerURLSchemesAsPrivileged for non-standard schemes ?

Sounds good to me.

@pfrazee
Copy link
Contributor Author
pfrazee commented Oct 25, 2016

Ok, to summarize the changes:

  • Update protocol. registerStandardSchemes to register a scheme as secure on the browser side, and to propagate that registration to renderers.
  • Add a second arg to registerStandardSchemes, which is an optional object where you can specify { secure: false } to disable secure registration.
  • Remove secure registration from registerURLSchemeAsPrivileged
  • Deprecate webframe. registerURLSchemeAsSecure
  • Probably should close this PR, as we wont need protocol.registerSecureSchemes.

Correct?

@zcbenz
Copy link
Contributor
zcbenz commented Oct 31, 2016

Correct?

👍

@pfrazee
Copy link
Contributor Author
pfrazee commented Nov 2, 2016

I'll get started on this next week.

@kevinsawicki
Copy link
Contributor

Probably should close this PR, as we wont need protocol.registerSecureSchemes.

Closing this out 👍

@pfrazee pfrazee mentioned this pull request Nov 11, 2016
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@zcbenz zcbenz

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pfrazee @deepak1556 @zcbenz @kevinsawicki
0