Add {secure:} opt to protocol.registerStandardSchemes #7947
Conversation
|
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: So, for reference, I went back to my custom build of electron (in which I do have #7671 merged) and added 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? |
|
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 |
|
Ah, very good, thank you |
|
How should the deprecations be handled? Do I remove the code and put a deprecation notice on the docs? |
|
@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 |
|
Ok, this is RFR |
| // Register scheme to secure list (https, wss, data). | ||
| blink::WebSecurityPolicy::registerURLSchemeAsSecure( | ||
| blink::WebString::fromUTF8(scheme)); | ||
| std::cout << "webFrame.registerURLSchemeAsSecure is deprecated. Use protocol.registerStandardSchemes with {secure:true}" << std::endl; |
There was a problem hiding this comment.
You can move the deprecations to lib/renderer/api/web-frame.js
There was a problem hiding this comment.
So, wrap the methods there so that they log?
There was a problem hiding this comment.
@pfrazee can this be removed and just documented in the planned breaking changes doc?
| // Deprecated | ||
| webFrame.registerURLSchemeAsSecure('app') | ||
| // Replace with | ||
| protocol.registerStandardSchemes('app', {secure: true}) |
There was a problem hiding this comment.
protocol.registerStandardSchemes accepts only array of scheme(s).
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 Then when we are closer to having a 2.0 API plan, we can add deprecation logging to all the needed areas. |
|
Ok, I fixed the documentation. That should cover it. |
|
Yeah, I'll try to hit this soon
…On Tue, Nov 29, 2016 at 3:06 PM, Kevin Sawicki ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In atom/renderer/api/atom_api_web_frame.cc
<#7947>:
> // Register scheme to secure list (https, wss, data).
blink::WebSecurityPolicy::registerURLSchemeAsSecure(
blink::WebString::fromUTF8(scheme));
+ std::cout << "webFrame.registerURLSchemeAsSecure is deprecated. Use protocol.registerStandardSchemes with {secure:true}" << std::endl;
@pfrazee <https://github.com/pfrazee> can this be removed and just
documented in the planned breaking changes doc?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#7947>, or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABNhU8sjruwgLjdLuUy7TbMeJFacsJ7qks5rDJPjgaJpZM4KwDBh>
.
|
|
Ok, just had a moment to make the requested change. Done |
| @@ -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( | |||
There was a problem hiding this comment.
Is this still used? I'm not seeing references to it.
There was a problem hiding this comment.
I believe it's called by chromium or brightray code. I confirmed by removing it and running tests, which then failed.
There was a problem hiding this comment.
Sorry, I didn't see the override 👍
| @@ -4,6 +4,8 @@ | |||
|
|
|||
| #include "atom/renderer/api/atom_api_web_frame.h" | |||
|
|
|||
| E7F5 #include <iostream> | |||
| std::vector<std::string> ParseSchemesCLISwitch(const char* switch_name) { | ||
| base::CommandLine* command_line = base::CommandLine::ForCurrentProcess(); | ||
| std::string custom_schemes = command_line->GetSwitchValueASCII(switch_name); | ||
| if (!custom_schemes.empty()) { |
There was a problem hiding this comment.
base::SplitString should handle empty strings so I don't think we need this check here.
| std::vector<std::string> schemes; | ||
| ConvertStringWithSeparatorToVector(&schemes, ",", | ||
| switches::kSecureSchemes); | ||
| if (!schemes.empty()) { |
There was a problem hiding this comment.
I think this check would be okay to remove since the for loop should handle an empty schemes vector fine.
…rdScehesm, and add working test
fe15477 to
cac85d2
Compare
|
Fixed a few linter warnings and rebased to resolve the Thanks @pfrazee for this 👍 |
This PR replaces #7671. It will:
registerStandardSchemes, which is an optional object where you can specify{ secure: true }to enable secure registration. Updateprotocol.registerStandardSchemesto register a scheme as secure on the browser side, and to propagate that registration to renderers.webFrame.registerURLSchemeAsSecureandwebFrame.registerURLSchemeAsPrivilegedStill in progress. I'm opening this PR to discuss the changes while I work.