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

Add {secure:} opt to protocol.registerStandardSchemes #7947

Merged
merged 9 commits into from Dec 12, 2016

Conversation

Projects
None yet
4 participants
@pfrazee
Contributor

pfrazee commented Nov 11, 2016

This PR replaces #7671. It will:

  • Add a second arg to registerStandardSchemes, which is an optional object where you can specify { secure: true } to enable secure registration. Update protocol.registerStandardSchemes to register a scheme as secure on the browser side, and to propagate that registration to renderers.
  • Add deprecation notices to webFrame.registerURLSchemeAsSecure and webFrame.registerURLSchemeAsPrivileged

Still in progress. I'm opening this PR to discuss the changes while I work.

@pfrazee

This comment has been minimized.

Show comment
Hide comment
@pfrazee

pfrazee Nov 11, 2016

Contributor

I'll need some help on this -

I was having difficulty writing the tests, so I decided to run a demo app and just check the DevTools. I looked at the Security tab and got:

screen shot 2016-11-11 at 1 13 38 pm

So, for reference, I went back to my custom build of electron (in which I do have #7671 merged) and added webFrame.registerURLSchemeAsPrivileged to the demo. However, the devtools security tab still shows Insecure.

So, I'm wondering if even the current master, and my custom build, are missing a step for correctly registering a protocol as secure. Any thoughts?

Contributor

pfrazee commented Nov 11, 2016

I'll need some help on this -

I was having difficulty writing the tests, so I decided to run a demo app and just check the DevTools. I looked at the Security tab and got:

screen shot 2016-11-11 at 1 13 38 pm

So, for reference, I went back to my custom build of electron (in which I do have #7671 merged) and added webFrame.registerURLSchemeAsPrivileged to the demo. However, the devtools security tab still shows Insecure.

So, I'm wondering if even the current master, and my custom build, are missing a step for correctly registering a protocol as secure. Any thoughts?

@deepak1556

This comment has been minimized.

Show comment
Hide comment
@deepak1556

deepak1556 Nov 12, 2016

Member

The devtools panel security info is not standard, currently only valid HTTPS schemes are considered secure https://cs.chromium.org/chromium/src/components/security_state/security_state_model.h?l=38-66 .

You can test if origin is registered as secure by trying to use cachestorage api, the renderer will terminate throw error if the origin is not secure.

Member

deepak1556 commented Nov 12, 2016

The devtools panel security info is not standard, currently only valid HTTPS schemes are considered secure https://cs.chromium.org/chromium/src/components/security_state/security_state_model.h?l=38-66 .

You can test if origin is registered as secure by trying to use cachestorage api, the renderer will terminate throw error if the origin is not secure.

@pfrazee

This comment has been minimized.

Show comment
Hide comment
@pfrazee

pfrazee Nov 12, 2016

Contributor

Ah, very good, thank you

Contributor

pfrazee commented Nov 12, 2016

Ah, very good, thank you

@pfrazee

This comment has been minimized.

Show comment
Hide comment
@pfrazee

pfrazee Nov 14, 2016

Contributor

How should the deprecations be handled? Do I remove the code and put a deprecation notice on the docs?

Contributor

pfrazee commented Nov 14, 2016

How should the deprecations be handled? Do I remove the code and put a deprecation notice on the docs?

@MarshallOfSound

This comment has been minimized.

Show comment
Hide comment
@MarshallOfSound

MarshallOfSound Nov 14, 2016

Member

@pfrazee Someone can confirm this ( @kevinsawicki ) but I believe the way we currently do it is leave the deprecated method in. Add a console warning that it is deprecated. Remove it from the docs. And add a // TODO in the code to remove it entirely in the next major bump.

Member

MarshallOfSound commented Nov 14, 2016

@pfrazee Someone can confirm this ( @kevinsawicki ) but I believe the way we currently do it is leave the deprecated method in. Add a console warning that it is deprecated. Remove it from the docs. And add a // TODO in the code to remove it entirely in the next major bump.

@pfrazee

This comment has been minimized.

Show comment
Hide comment
@pfrazee

pfrazee Nov 14, 2016

Contributor

Ok, this is RFR

Contributor

pfrazee commented Nov 14, 2016

Ok, this is RFR

@pfrazee pfrazee changed the title from Add {secure:} opt to protocol.registerStandardSchemes (WIP) to Add {secure:} opt to protocol.registerStandardSchemes Nov 14, 2016

@kevinsawicki

This comment has been minimized.

Show comment
Hide comment
@kevinsawicki

kevinsawicki Nov 15, 2016

Contributor

Add deprecation notices to

We haven't really formally deprecated (meaning adding logged messages) any 2.0 APIs yet.

I think it is okay for now to just add a TODO comment like this one and add an entry to planned-breaking-changes.md.

Then when we are closer to having a 2.0 API plan, we can add deprecation logging to all the needed areas.

Contributor

kevinsawicki commented Nov 15, 2016

Add deprecation notices to

We haven't really formally deprecated (meaning adding logged messages) any 2.0 APIs yet.

I think it is okay for now to just add a TODO comment like this one and add an entry to planned-breaking-changes.md.

Then when we are closer to having a 2.0 API plan, we can add deprecation logging to all the needed areas.

@pfrazee

This comment has been minimized.

Show comment
Hide comment
@pfrazee

pfrazee Nov 16, 2016

Contributor

Ok, I fixed the documentation. That should cover it.

Contributor

pfrazee commented Nov 16, 2016

Ok, I fixed the documentation. That should cover it.

@pfrazee

This comment has been minimized.

Show comment
Hide comment
@pfrazee

pfrazee Dec 1, 2016

Contributor
Contributor

pfrazee commented Dec 1, 2016

@pfrazee

This comment has been minimized.

Show comment
Hide comment
@pfrazee

pfrazee Dec 11, 2016

Contributor

Ok, just had a moment to make the requested change. Done

Contributor

pfrazee commented Dec 11, 2016

Ok, just had a moment to make the requested change. Done

@kevinsawicki kevinsawicki self-assigned this Dec 12, 2016

@@ -31,6 +31,9 @@ class AtomContentClient : public brightray::ContentClient {
std::vector<content::PepperPluginInfo>* plugins) override;
void AddServiceWorkerSchemes(
std::set<std::string>* service_worker_schemes) override;
void AddSecureSchemesAndOrigins(

This comment has been minimized.

@kevinsawicki

kevinsawicki Dec 12, 2016

Contributor

Is this still used? I'm not seeing references to it.

@kevinsawicki

kevinsawicki Dec 12, 2016

Contributor

Is this still used? I'm not seeing references to it.

This comment has been minimized.

@pfrazee

pfrazee Dec 12, 2016

Contributor

I believe it's called by chromium or brightray code. I confirmed by removing it and running tests, which then failed.

@pfrazee

pfrazee Dec 12, 2016

Contributor

I believe it's called by chromium or brightray code. I confirmed by removing it and running tests, which then failed.

This comment has been minimized.

@kevinsawicki

kevinsawicki Dec 12, 2016

Contributor

Sorry, I didn't see the override 👍

@kevinsawicki

kevinsawicki Dec 12, 2016

Contributor

Sorry, I didn't see the override 👍

@kevinsawicki

This comment has been minimized.

Show comment
Hide comment
@kevinsawicki

kevinsawicki Dec 12, 2016

Contributor

Fixed a few linter warnings and rebased to resolve the planned-breaking-changes.md conflict.

Thanks @pfrazee for this 👍

Contributor

kevinsawicki commented Dec 12, 2016

Fixed a few linter warnings and rebased to resolve the planned-breaking-changes.md conflict.

Thanks @pfrazee for this 👍

@kevinsawicki kevinsawicki merged commit 1d288b6 into electron:master Dec 12, 2016

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment