feat: add session.setCorsOriginAccessList API

[lishid]

Description of Change

Summary: A new "local" scheme privilege is added, which grants access to file:// resources without needing to disable webSecurity.

Rationale:

• It is common for electron apps to define a custom protocol to serve app resources (as seen from first example here).
• Apps that registers their custom protocol as secure cannot access files from the file:/// protocol.
• This rejection comes from deep within chromium and no Electron API seem to be able to override that, except webSecurity: false. Things I've tried:
  • registerFileProtocol('file'...)
  • interceptFileProtocol('file'...)
  • session.defaultSession.webRequest.onBeforeRequest
• webSecurity: false is not a recommended practice for security purposes by https://www.electronjs.org/docs/tutorial/security

This PR adds a local privilege to scheme registration, which adds the scheme to blink::SchemeRegistry::RegisterURLSchemeAsLocal.
Doing so marks the protocol as local, thus allowing it to access local resources via the pathway:
SecurityOrigin::CanDisplay()
SecurityOrigin::can_load_local_resources_
SecurityOrigin::IsLocal()

Checklist

• PR description included and stakeholders cc'd
• npm test passes
• tests are changed or added
• relevant documentation is changed or added
• PR title follows semantic commit guidelines
• PR release notes describe the change in a way relevant to app developers, and are capitalized, punctuated, and past tense.
• This is NOT A BREAKING CHANGE. Breaking changes may not be merged to master until 11-x-y is branched.

Release Notes

Notes: Added "local" scheme privilege, which grants access to file:// without the need to disable webSecurity.

[welcome]

💖 Thanks for opening this pull request! 💖

We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix.

Examples of commit messages with semantic prefixes:

• fix: don't overwrite prevent_default if default wasn't prevented
• feat: add app.isPackaged() method
• docs: app.isDefaultProtocolClient is now available on Linux

Things that will help get your PR across the finish line:

• Follow the JavaScript, C++, and Python coding style.
• Run npm run lint locally to catch formatting errors earlier.
• Document any user-facing changes you've made following the documentation styleguide.
• Include tests when adding/changing behavior.
• Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

[lishid]

The main issue we're trying to solve here is that webSecurity: false allows external embedded <iframe>s (think https://) to load file:/// URLs, which is a security issue.
Allowing local access for your custom app:// URL means the page on app://index.html can access file:/// resources, but an embedded <iframe> from https:// cannot access file:///.

[deepak1556]

Any reason you can't register another custom protocol that handles file paths ? what is the reason behind mixing file uri and custom protocol in the app ?

[lishid]

@deepak1556

Any reason you can't register another custom protocol that handles file paths ?

That's what I'm currently using, but it has some limitations and complications when we render end-user generated contents. It would be extremely convenient if file:/// paths just worked, rather than being blocked by Not allowed to load local resource.

For context, we render markdown content created by end users. We'd like to support

• file:/// image embeds
• Copying the rendered image to yield the file:/// link in the clipboard as HTML (rather than custom://)
• <iframe> a local .html file

I imagine it's possible to work around these issues with an HTML post processor that replaces file:/// links with custom://, then hook the copy, cut, dragstart events to replace the links back to file:///, but that's quite a poor solution with many easy-to-miss edge cases, for example, remembering to translate ALL src-like properties from <video>, <audio>, <source>, etc.

There's currently another bug which I have not yet filed, where registerFileProtocol('custom') does not work with the built-in PDF plugin using custom://path/to/file.pdf.

what is the reason behind mixing file uri and custom protocol in the app?

If I understand correctly, custom protocol provides many advantages over file:/// for serving app resources:

• It can be registered as a standard scheme to allow absolute paths to resolve properly
• It can be registered as a secure protocol to access various 'secure' gated features, such as SubtleCrypto
• It sets a Origin properly when connecting with remote resources behind https://, which can be used to manage CORS securely.

[lishid]

I'd like to add that while workarounds do exist, such as another custom protocol for files, developers using Electron are incentivized to take the easy route and just set webSecurity: false. (Related: #23757)

Most people don't really think about the implications of turning off webSecurity, other than that it enables the desired functionality. By principle we should provide a safer way to access/embed local resources.

[deepak1556]

Thanks for the added context, really helps!

• When you register a scheme with this new local privilege, it will get the same security policies as a file protocol, that means you can no longer access resources on this custom protocol from other less privileged schemes. Would this custom scheme still be useful for your app ?

• Have a scheme with secure, standard and local privileges is not a valid path in chromium, so there are more undefined behaviours if users arbitrarily configure these options.

I think it should be hard to configure bad custom schemes in electron, if the api were to make local as exclusive from other privileges (i-e) throw proper error messages, it should be fine.

With the above conditions, an example app scheme configured with local privilege will behave exactly like another file scheme. Would this still serve the purpose for your app ?

[lishid]

• When you register a scheme with this new local privilege, it will get the same security policies as a file protocol, that means you can no longer access resources on this custom protocol from other privileged schemes. Would this custom scheme still be useful for your app ?

I think that should be fine for most use cases. I imagine in the most common case, it would disallow https:// pages access to include things from my-app://, which may even be considered a desirable behavior. It should be blocked by CORS anyway assuming the developer hasn't disabled CORS.

• Have a scheme with secure, standard and local privileges is not a valid path in chromium, so there are more undefined behaviours if users arbitrarily configure these options.

That's interesting, definitely something I wasn't aware of. I must have missed some of the checks when browsing the code (please excuse my first time working with chromium source; would also appreciate if you can give some pointers to how chromium checks for such an invalid scheme).
I don't mind trying another approach, since I did notice there might be side effects from marking the custom scheme as local.

I think it should be hard to configure bad custom schemes in electron, if the api were to make local as exclusive from other privileges (i-e) throw proper error messages, it should be fine.

With the above conditions, an example app scheme configured with local privilege will behave exactly like another file scheme. Would this still serve the purpose for your app ?

Given your insight that local must be exclusive from standard and secure, I would say that indeed makes this solution rather useless.

While digging, another possible solution I think could work is calling WebSecurityPolicy::AddOriginAccessAllowListEntry(), which would satisfy this part of the CanDisplay() check. The only problem is AddOriginAccessAllowListEntry requires an Origin (scheme + host) rather than just a scheme.

[deepak1556]

While digging, another possible solution I think could work is calling WebSecurityPolicy::AddOriginAccessAllowListEntry(), which would satisfy this part of the CanDisplay() check

that sounds better providing more finer control, it can be a new api on session module. I think the api works well with empty hosts and file:// origin, can you test if this works for your case ?

[lishid]

Sure, I'll give it a try. Thanks for the tip!

[lishid]

Sorry for the delay - I haven't been able to get Electron to build. After several attempts it seems to constantly get stuck on
[14424/14849] STAMP obj/tools/v8_context_snapshot/v8_context_snapshot.stamp

Is there any resources I can look for to debug this issue?

[deepak1556]

you can run the ninja command with -v flag to get more verbose output. Also does your system have a minimum of 8GB RAM, it is bare minimum required for linking step.

[lishid]

My VM has 16GB RAM, not sure if that's enough, I'll reboot it with 48GB after to see.

In my search, I found this thread https://groups.google.com/a/chromium.org/d/msg/chromium-dev/0g7D-ubb87c/dbpGxBSoBQAJ where this user ran into something similar.
According to his log, he's stuck on the LINK step, but I think my setup got stuck before that.
His log from the google groups link:

[4/5] STAMP obj/tools/v8_context_snapshot/v8_context_snapshot.stamp
[5/5] LINK ./chrome

---

I've restarted with e build -v, somehow it seems to get stuck on a touch command (makes sense that's what STAMP does) but I can run it just fine manually

[deepak1556]

Is it still stuck at that step after you ran it manually ? Can you try removing that file and re-try again.

[lishid]

That's right, I've tried a couple of times removing that file and resuming build - suspecting it may be a hardware issue, I'm formatting a new SSD to install a fresh OS and setup from scratch. Will report back once that's done.

[lishid]

I figured it out (it was a hard drive issue) and I'm now able to build & run.

Quick question while I'm at it:
The session module seems to belong to the main process, so I'm unsure what pathway this should be communicated to renderer processes. I've concluded that since I'm not seeing any reference from electron_api_session to the blink namespace.

[lishid]

I've also confirmed that the WebSecurityPolicy::AddOriginAccessAllowListEntry() indeed works when placed in renderer_client_base.cc around this area:

electron/shell/renderer/renderer_client_base.cc

Line 230 in c6c022d

blink::WebSecurityPolicy::RegisterURLSchemeAsAllowingServiceWorkers("file");

blink::WebURL webUrl(GURL("app://./"));
blink::WebSecurityPolicy::AddOriginAccessAllowListEntry(
  webUrl, "file", "", 
  network::mojom::CorsDomainMatchMode::kAllowSubdomains,
  network::mojom::CorsPortMatchMode::kAllowAnyPort,
  network::mojom::CorsOriginAccessMatchPriority::kDefaultPriority
);

There's a few sanitization I'll need to perform (ex: input url must contain host) to avoid crashes, but adding this indeed solves the Not allowed to load local resource issue.
EDIT: It runs into #23757 but there's a workaround to fix that protocol.registerFileProtocol("file", ...).

[lishid]

After much exploring in the source code, it still seems the only place that uses WebSecurityPolicy is from renderer_client_base

electron/shell/renderer/renderer_client_base.cc

Line 39 in c6c022d

#include "third_party/blink/public/web/web_security_policy.h"

I haven't yet found a good example of an Electron API that accesses something similar from the main process. I'm hoping someone can point me to one such example so I can copy paste get some inspirations.

[deepak1556]

Sorry for the delayed response

Yes WebSecurityPolicy is only meant to be accessed on the renderer side and the RendererClientBase::RenderThreadStarted is ideal point to invoke it, mainly because the policies need to be configured before the document starts loading.

Currently we have protocol module which configures something similar on the renderer side via RegisterSchemeAsPrivileged through command line ElectronBrowserClient::AppendExtraCommandLineSwitches

Reason I asked the api to be on session is because the origin list can be shared across different webContents using similar session. Also we don't want this api on the renderer side (for ex: via webFrame) because the renderer contents can be untrusted and should not get access to this powerful api.

For this particular api the CORS check is eventually performed by the network service process https://source.chromium.org/chromium/chromium/src/+/master:third_party/blink/renderer/platform/weborigin/security_policy.cc;l=58 , so we don't have to pass this to the renderer and call the WebSecurityPolicy api.

There is way to configure the origin list per network context via the context create param https://source.chromium.org/chromium/chromium/src/+/master:services/network/public/mojom/network_context.mojom;l=429 , all we need to do is setup ElectronBrowserContext::GetSharedCorsOriginAccessList and the session module api can call SharedCorsOriginAccessList::SetForOrigin

[deepak1556]

Since this api might require a patch to network_context.mojom for updating the access list dynamically and also the api form needs a bit more discussion, looping in @electron/wg-api members

[lishid]

Sorry for the lack of activity - shifting priorities. Though I'll get back to more experimenting and come up with a new commit soon. Thanks again for all the assistance.

[lishid]

Ok so I've hooked up a method in the session API that calls browser_context()->SetCorsOriginAccessListForOrigin.
Next, I've modified ElectronBrowserContext::SetCorsOriginAccessListForOrigin and GetSharedCorsOriginAccessList using the source in Chromium (https://source.chromium.org/chromium/chromium/src/+/master:chrome/browser/profiles/profile_impl.cc;l=1275?q=SetCorsOriginAccessListForOrigin&ss=chromium%2Fchromium%2Fsrc)

If I understand the code correctly, SetCorsOriginAccessListForOrigin calls CorsOriginPatternSetter::SetLists on each partition, which proxies to partition->GetNetworkContext()->SetCorsOriginAccessListsForOrigin.

This doesn't seem to do anything in my testing just yet, so I'm not too sure what the next step is.
In your last reply, you mentioned that we'll need to patch and use network_context.mojom, so maybe that's what I'm missing? I was under the impression that partition->GetNetworkContext()->SetCorsOriginAccessListsForOrigin would apply it to the NetworkContext.

Perhaps I should gather and commit my changes just so we can look at them? Let me know.

[deepak1556]

Yup it would be better to push your changes, thanks!

[lishid]

The API is still there just for testing purpose, will change it to whatever makes sense once the team approves the method and agrees on an API design.

[deepak1556]

I haven't gotten around to this PR yet, will have a look at the implementation tomorrow. Thanks!

[lishid]

Oops wrong branch... Sorry about that

[lishid]

Quick ping and also to clarify this code doesn't actually achieve the intended behavior just yet. I have not yet find out why that is.

[jkleinsc]

The @electron/wg-api reviewed this at the August 24, 2020 meeting.

[lishid]

Thank you for the response. It's understandable given you guys have a lot on your plates!

Let me know if there's anything I can do to help 😄

[deepak1556]

@lishid i have cleaned up the PR a bit, the api now looks like

await session.setCorsOriginAccessList(
   "custom://abc/hello.html",
   [ "https://*.github.com/*", "*://electron.github.io", "file" ], /* allow list */
   [ "http://*/*" ] /* block list */);

can you work on writing some tests and adding documentation, so that it can be reviewed again by api-wg next week. Thanks!

[lishid]

Will do, thanks!

[lishid]

Just added docs, will work on tests tomorrow when my updated sources builds all 14k files again 😂

[bpasero]

Hey, is this PR moving on?

[lishid]

Doesn't seem like the API has been reviewed a second time yet, probably fallen through the cracks - @jkleinsc any updates?

[lishid]

Hey @deepak1556 @jkleinsc is there anything we can do to push this PR forward?

IMO it's still a serious security issue for Electron because a lot of Electron apps use webSecurity: false and/or bypassCSP: true as the current workaround for the lack of this API.

[jkleinsc]

@lishid there are a couple things that are needed for this PR to move forward:

1. Update your branch to the latest from master to resolve the conflicting files:
2. Resolve the lint errors - you can verify linting locally by running npm run lint
3. The API WG needs to approve this PR. I'll ping the group today on this.

[lishid]

I could not run lint because cpplint.py: not found. Tried looking it up and I don't see any cpplint.py in the repo.

I don't see any instructions for cpplint anywhere in any of these docs pages either:

• https://www.electronjs.org/docs/development/pull-requests
• https://www.electronjs.org/docs/development/build-instructions-linux
• https://www.electronjs.org/docs/development/coding-style
• https://github.com/electron/build-tools/search?q=cpplint

Saw a package named cpplint on pip but not sure if that's compatible with the current python 2.7 setup.

Please advise.

EDIT: I got access to the CI lint results instead!

[lishid]

@deepak1556 I seem to be getting an exception for "conversion failure from" (there's nothing after "from") when I specify the allow list to be [ "file" ]. That can't seem right? I'm not familiar with the conversion process, hopefully someone can help. 🙏

[lishid]

@deepak1556 Upon further debugging, I believe that the CanDisplay() check only uses the local WebSecurityPolicy as a shortcut for checking, and the request only makes it to the shared network manager process if this check passes in the first place. Thus we're pretty much stuck passing it through AppendExtraCommandLineSwitches like we do for schemes.

@jkleinsc Given this scenario, I'd propose a new API inspired by protocol.registerSchemesAsPrivileged(customSchemes), let's say protocol.registerFileAccessibleOrigin(origin), which would similarly be added to as an extra command line switch.

I'd argue that making this API more general purpose (i.e. not just allowing access to file:/// protocol) doesn't make much sense, because there's little need to have fine grained control over CORS for any other scheme/protocol.

[zcbenz]

@lishid Do you mean setCorsOriginAccessList(origin, ['file'], []) is not enough to make origin access file: and we have to add a new registerFileAccessibleOrigin API for that purpose?

[lishid]

@zcbenz That is mostly correct. In my experiments, only applying the WebSecurityPolicy API directly from the renderer's side had an impact over the file:/// block. (As described by #24849 (comment))

In addition, in my testing, I was able to allow access to file:/// by only using the WebSecurityPolicy API in the renderer process, without making use of the shared CORS allow list that is currently in this PR.

EDIT: I want to clarify that setCorsOriginAccessList(origin, ['file'], []) may not be necessary to achieve the purposes of this proposal, and only registerFileAccessibleOrigin will be necessary.

Some more observations that may be relevant:

• The error for file:/// protocol access is different from regular CORS errors. It specifically says Not allowed to load local resource: file:///path/to/resource
• In the network tab, the request lacks any kind of information, including headers, and the timing tab calculates it to be 20us.
• I've attempted to use any API available for intercept the request, such as registering a protocol handler, and using webRequests, but none of it ever catches the request.

When I traced through the specific error message in Chromium's source, it seemed to be produced through a code path that had very basic checks, which was specially coded for protocols marked as local. The only two ways to bypass this check was to either 1) mark the origin protocol as local as well, or 2) have file: in the WebSecurityPolicy allow list for that origin. EDIT: Details attached

[zcbenz]

Thanks for explaining, I'm good with either setCorsOriginAccessList or registerFileAccessibleOrigin API.

But if we go with setCorsOriginAccessList, it should handle setCorsOriginAccessList(origin, ['file'], []) as a special case under the hood without involving another API, since most users do not understand the internal security model of blink and it would be very confusing to have to use 2 APIs together to achieve one thing.

[lishid]

Sounds good. I will attempt a prototype with just registerFileAccessibleOrigin because I think setCorsOriginAccessList will most likely go unused, and will just be additional maintenance surface without clear benefit or use-case.

[codebytere]

@lishid is this PR still something you're planning on pursuing?

[lishid]

Yeah... I haven't had time to follow up but it should definitely be implemented at some point so that people don't just turn off webSecurity to achieve the desired effect. I will catch up once I get some more free time on my hands.

[miniak]

@lishid do you have any updates on the PR?

[lishid]

Apologies, I still have my hands full and haven't been able to make any progress. Feel free to close until someone more capable can implement this.

[nornagon]

I'm going to close this for now as there's not been much activity lately. If there's renewed interest in this we can open a new PR.