feat: implement allowFileAccess loadExtension option #25198

ChALkeR · 2020-08-29T07:48:11Z

Description of Change

Alternative to #25151.

Allows file access for extensions via an opt-in on per-extension basis.
Pre-9 this was essentially "enabled" by default, until 9.0.0 broke react devtools on local file:// urls (#24011).

API / doc is done.
Implementation works but is WiP (see below)
I don't like how flags are being passed this way, also a FIXME in LoadExtensionForReload() -- not sure yet when that is triggered. Probably replacing that with an additional method that sets the flags for a certain extension id and is stored in-memory would be a better approach.

Fixes: #24011.

Confirmed that react-devtools now works over file:// if allowFileAccess: true is passed and doesn't work otherwise.

cc @nornagon @samuelmaddock @RangerMauve

Thoughts? It would be helpful to hear which approach should be continued, this one or #25151.

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 allowFileAccess option to loadExtension() API.

samuelmaddock

I like the options argument for ses.loadExtension. Making use of ExtensionPrefs should help with the remaining work on this PR. 👍

shell/browser/api/electron_api_session.cc

samuelmaddock · 2020-08-29T08:19:45Z

shell/browser/extensions/electron_extension_loader.cc

@@ -155,9 +156,10 @@ void ElectronExtensionLoader::LoadExtensionForReload(
    LoadErrorBehavior load_error_behavior) {
  CHECK(!path.empty());

+  int load_flags = Extension::FOLLOW_SYMLINKS_ANYWHERE; // FIXME


I think this is a place where ExtensionPrefs would be useful. Looking at Chrome's installed extension loader, they use this to store global and extension-specific preferences at runtime—including methods for getting/setting file access.

In Session::LoadExtension, we could call ExtensionPrefs::SetAllowFileAccess with the value of the allowFileAccess option. Then ElectronExtensionLoader can derive the load_flags from ExtensionPrefs. Chrome's InstalledLoader::GetCreationFlags is a good example of how this could work.

@samuelmaddock thanks for the feedback!

I thought about that, but the thing I noticed while testing (#24011 (comment)) is that:

This preference doesn't appear to do anything on it's own, or it could do something I don't know yet.

In Chromium, this is apparently not what is used.

A call to ExtensionPrefs::SetAllowFileAccess writes to {profile}/Preferences.

But in Chromium that doesn't happen when file access is enabled in extension preferences.

Instead, Chromium stores that in{profile}/Current Session which is inexistent in Electron.

My concern here is that as Chromium apparently doesn't use it in that scenario, perhaps ExtensionPrefs::SetAllowFileAccess does something else and can have some other side effect?
Or is that not the case?

I can surely port to that if it would be safe to use it here.

After looking more into this, Chrome implements a private API on the chrome://extensions page with a function chrome.developerPrivate.updateExtensionConfiguration(config, callback). It's internal implementation calls a utility which in turn calls ExtensionPrefs::SetAllowFileAccess.

After this function is called, the value will stay in memory and (I think) persist to disk soon after. An extension loader implements a GetCreationFlags method to then derive flags from ExtensionPrefs.

In my opinion, sticking with this pattern close to Chrome will allow us to more naturally build on the extension support in Electron later on.

As for the the location of prefs ({profile}/Preferences), I'm not sure what the right path would be for us. I think we can go with the default and change it later if needed.

I don't believe Electron persists such preferences to disk. Additionally, I'd prefer that the extensions loading stuff doesn't persist anything to disk internally, as that's often surprising for developers (e.g. "i removed the loadExtension(...) line from my app, why is the react devtools extension still loading??" behavior that used to be present.)

Update on ExtensionPrefs, they're mostly used by Chrome to persist extension settings for a profile.

Whether an extension has file access or can run in incognito mode.

The extension installation time.

If an extension is disabled, the reason why.

And many other Chrome-specific settings.

These aren't too relevant to an Electron app and could be managed outside of Electron internal code. However, special cases such as chrome.runtime.reload() will cause this setting to be dropped without relying on storage such as ExtensionPrefs.

Additionally, ExtensionPrefs do end up persisting to disk using the same JsonPrefStore as UserPrefs which are setup in ElectronBrowserContext::InitPrefs.

To avoid this behavior, I believe it will be necessary to create an InMemoryPrefStore exclusively for ExtensionPrefs which would be returned from ElectronExtensionsBrowserClient::GetPrefServiceForContext. Possibly a command-line flag could be used if a developer wanted to opt-in to persisting these settings?

@ChALkeR to followup, we can simplify this by avoiding ExtensionPrefs altogether. Adding a map of
extensionId -> allowFileAccess could be added to ElectronExtensionLoader. When an extension is unloaded, we can clear it. That way we can allow persisting file access if an extension is reloaded.

Co-authored-by: Samuel Maddock <samuel.maddock@gmail.com>

docs/api/session.md

shell/browser/extensions/electron_extension_loader.cc

jkleinsc · 2020-09-22T00:21:53Z

The API WG reviewed this at the Sept 21, 2020 meeting

shell/browser/extensions/electron_extension_loader.cc

docs/api/session.md

nornagon · 2020-11-17T23:14:27Z

I'm happy to land this without the fix for remembering file access across extension reload. The latter is a smaller bug than what this fixes.

RobbieTheWagner · 2020-12-26T12:41:02Z

What's the status on this?

andrewschreiber · 2021-01-26T01:43:04Z

Very important PR to my project. What are the remaining blockers? I'd like to help if possible.

codebytere · 2021-01-26T17:23:24Z

@ChALkeR would you still like to move this forward?

RobbieTheWagner · 2021-01-26T17:57:21Z

This is a necessary feature, so hopefully it can be moved forward 😃. I'm willing to help too, if more help is needed.

safareli · 2021-01-30T20:01:46Z

We really want this to be merged and are ready to help move this forward!

Co-authored-by: Samuel Maddock <samuel.maddock@gmail.com> Co-authored-by: Jeremy Rose <jeremya@chromium.org>

trop · 2021-02-11T00:24:32Z

@nornagon has manually backported this PR to "12-x-y", please check out #27702

Co-authored-by: Samuel Maddock <samuel.maddock@gmail.com> Co-authored-by: Jeremy Rose <jeremya@chromium.org>

trop · 2021-02-11T00:26:02Z

@nornagon has manually backported this PR to "11-x-y", please check out #27703

bedney · 2021-02-11T01:41:51Z

@nornagon -

Thank you for going the extra mile here with the back ports! We really appreciate it!

benjaffe · 2021-02-11T03:16:04Z

Yes yes, thank you thank you! <3

twitharshil · 2021-02-11T04:36:38Z

@nornagon Will the fix for this issue can be backported to electron v9 which is still on the supported line versions?

nornagon · 2021-02-17T22:49:20Z

@twitharshil no, we are only backporting security fixes to v9.

Per electron/electron#23662 and electron/electron#25198 this is necessary to allow access to extensions via file:// Closes MarshallOfSound#161 Closes MarshallOfSound#162

feat: implement allow_file_access loadExtension option

6b95543

electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Aug 29, 2020

samuelmaddock suggested changes Aug 29, 2020

View reviewed changes

electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Aug 30, 2020

ChALkeR and others added 3 commits August 31, 2020 15:13

allow_file_access -> allowFileAccess

b048f32

Co-authored-by: Samuel Maddock <samuel.maddock@gmail.com>

Allow_file_access -> allowFileAccess

61c67f3

lint

d91e060

ChALkeR changed the title ~~feat: implement allow_file_access loadExtension option~~ feat: implement allowFileAccess loadExtension option Aug 31, 2020

nornagon approved these changes Sep 2, 2020

View reviewed changes

docs/api/session.md Show resolved Hide resolved

docs/api/session.md Show resolved Hide resolved

shell/browser/extensions/electron_extension_loader.cc Show resolved Hide resolved

nornagon added the api-review/requested 🗳 label Sep 2, 2020

jkleinsc added the api-review/reviewed label Sep 22, 2020

samuelmaddock mentioned this pull request Sep 30, 2020

fix(extensions): implement missing web_request hooks #22655

Merged

6 tasks

This was referenced Nov 10, 2020

docs: update devtools extension tutorial #26326

Merged

fix: allow extensions to access local files #25151

Closed

nornagon reviewed Nov 17, 2020

View reviewed changes

shell/browser/extensions/electron_extension_loader.cc Outdated Show resolved Hide resolved

nornagon reviewed Nov 17, 2020

View reviewed changes

docs/api/session.md Outdated Show resolved Hide resolved

Apply suggestions from code review

9a116b6

nornagon marked this pull request as ready for review November 17, 2020 23:12

nornagon and others added 2 commits November 30, 2020 14:48

lint

bafc433

Merge branch 'master' into chalker/extension-file-access-v2

5a8cb8b

schickm mentioned this pull request Dec 25, 2020

React DevTools and Redux DevTools tabs sometimes not appearing Electron@9.0.0 #23662

Closed

3 tasks

jkleinsc added the semver/minor backwards-compatible functionality label Jan 5, 2021

TomTomB mentioned this pull request Jan 16, 2021

React Developer Tools not appearing electron-react-boilerplate/electron-react-boilerplate#2725

Closed

5 tasks

nornagon added a commit that referenced this pull request Feb 11, 2021

feat: implement allowFileAccess loadExtension option (#25198)

1c8e0d6

Co-authored-by: Samuel Maddock <samuel.maddock@gmail.com> Co-authored-by: Jeremy Rose <jeremya@chromium.org>

trop bot added in-flight/12-x-y and removed needs-manual-bp/12-x-y labels Feb 11, 2021

nornagon added a commit that referenced this pull request Feb 11, 2021

feat: implement allowFileAccess loadExtension option (#25198)

659aaf2

Co-authored-by: Samuel Maddock <samuel.maddock@gmail.com> Co-authored-by: Jeremy Rose <jeremya@chromium.org>

nornagon mentioned this pull request Feb 11, 2021

feat: implement allowFileAccess loadExtension option #27703

Merged

trop bot added in-flight/11-x-y and removed needs-manual-bp/11-x-y labels Feb 11, 2021

codebytere mentioned this pull request Feb 11, 2021

Cherry pick squash commit instead of each individual commit electron/trop#185

Closed

trop bot added merged/12-x-y and removed in-flight/12-x-y labels Feb 16, 2021

shatyuka mentioned this pull request May 9, 2021

fix: increase corner radius for vibrancy view on big sur #29071

Merged

trop bot added the in-flight/12-x-y label May 9, 2021

shatyuka mentioned this pull request May 9, 2021

fix: increase corner radius for vibrancy view on big sur #29072

Merged

trop bot added the in-flight/11-x-y label May 9, 2021

codebytere removed in-flight/11-x-y labels May 10, 2021

electron deleted a comment from trop bot May 10, 2021

jamesmontalvo3 mentioned this pull request Jul 2, 2021

Only Redux Devtool Shows MarshallOfSound/electron-devtools-installer#162

Open

jamesmontalvo3 mentioned this pull request Jul 2, 2021

Add required allowFileAccess property to docs MarshallOfSound/electron-devtools-installer#191

Open

Nantris mentioned this pull request May 30, 2022

[Feature Request]: Allow access to Devtools Extensions when app is loaded over custom protocol (eg app://) #34386

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implement allowFileAccess loadExtension option #25198

feat: implement allowFileAccess loadExtension option #25198

ChALkeR commented Aug 29, 2020 •

edited by nornagon

samuelmaddock left a comment

samuelmaddock Aug 29, 2020 •

edited

ChALkeR Aug 31, 2020 •

edited

samuelmaddock Sep 4, 2020 •

edited

nornagon Sep 4, 2020

samuelmaddock Sep 12, 2020 •

edited

samuelmaddock Sep 22, 2020 •

edited

jkleinsc commented Sep 22, 2020

nornagon commented Nov 17, 2020

RobbieTheWagner commented Dec 26, 2020

andrewschreiber commented Jan 26, 2021

codebytere commented Jan 26, 2021

RobbieTheWagner commented Jan 26, 2021

safareli commented Jan 30, 2021

trop bot commented Feb 11, 2021

trop bot commented Feb 11, 2021

bedney commented Feb 11, 2021

benjaffe commented Feb 11, 2021

twitharshil commented Feb 11, 2021

nornagon commented Feb 17, 2021

feat: implement allowFileAccess loadExtension option #25198

feat: implement allowFileAccess loadExtension option #25198

Conversation

ChALkeR commented Aug 29, 2020 • edited by nornagon

Description of Change

Checklist

Release Notes

samuelmaddock left a comment

Choose a reason for hiding this comment

samuelmaddock Aug 29, 2020 • edited

Choose a reason for hiding this comment

ChALkeR Aug 31, 2020 • edited

Choose a reason for hiding this comment

samuelmaddock Sep 4, 2020 • edited

Choose a reason for hiding this comment

nornagon Sep 4, 2020

Choose a reason for hiding this comment

samuelmaddock Sep 12, 2020 • edited

Choose a reason for hiding this comment

samuelmaddock Sep 22, 2020 • edited

Choose a reason for hiding this comment

jkleinsc commented Sep 22, 2020

nornagon commented Nov 17, 2020

RobbieTheWagner commented Dec 26, 2020

andrewschreiber commented Jan 26, 2021

codebytere commented Jan 26, 2021

RobbieTheWagner commented Jan 26, 2021

safareli commented Jan 30, 2021

trop bot commented Feb 11, 2021

trop bot commented Feb 11, 2021

bedney commented Feb 11, 2021

benjaffe commented Feb 11, 2021

twitharshil commented Feb 11, 2021

nornagon commented Feb 17, 2021

ChALkeR commented Aug 29, 2020 •

edited by nornagon

samuelmaddock Aug 29, 2020 •

edited

ChALkeR Aug 31, 2020 •

edited

samuelmaddock Sep 4, 2020 •

edited

samuelmaddock Sep 12, 2020 •

edited

samuelmaddock Sep 22, 2020 •

edited