Skip to content
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

feat: add app.disablePluginSandbox(mimeType) #18939

Closed
wants to merge 1 commit into from
Closed

feat: add app.disablePluginSandbox(mimeType) #18939

wants to merge 1 commit into from

Conversation

miniak
Copy link
Contributor

@miniak miniak commented Jun 22, 2019 •
edited

Description of Change

Added app.disablePluginSandbox(mimeType) that allows to disable sandboxing of the ppapi plugin host process, which is per-plugin and shared by multiple BrowserWindow instances.

Address #18516. Follow up to #15894 (comment).

/cc @nornagon

Checklist

Release Notes

Notes: Added app.disablePluginSandbox(mimeType), which disables sandbox for the plugin identified by mimeType.

@miniak miniak requested a review from nornagon June 22, 2019 10:51
@miniak miniak requested a review from a team as a code owner June 22, 2019 10:51
@miniak miniak self-assigned this Jun 22, 2019
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Jun 22, 2019
@miniak
Copy link
Contributor Author

miniak commented Jun 22, 2019

@nornagon Should we maybe handle #17823 in AtomBrowserClient::IsPluginSandboxed() now?

@miniak miniak mentioned this pull request Jun 22, 2019
Closed
3 tasks
@miniak
Copy link
Contributor Author

miniak commented Jun 22, 2019

cc @Feverqwe

@miniak miniak changed the title Add app.disablePluginSandbox(mimeType) feat: add app.disablePluginSandbox(mimeType) Jun 22, 2019
@miniak miniak force-pushed the miniak/disable-plugin-sandbox branch 3 times, most recently from 8a51acb to 46d8df8 Compare June 22, 2019 14:32
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Jun 23, 2019
@MarshallOfSound MarshallOfSound requested a review from a team June 24, 2019 21:49
@MarshallOfSound
Copy link
Member

As a feature being targetted at 6-0-x this late in the beta cycle it needs approval from @electron/wg-releases. Please add this PR to the WG agenda with reasoning for it's addition

@miniak
Copy link
Contributor Author

miniak commented Jun 25, 2019

@MarshallOfSound added to the agenda. Regardless of whether it should be backported or not, how does the PR look to you?

@miniak miniak force-pushed the miniak/disable-plugin-sandbox branch from 46d8df8 to f0a64c5 Compare June 27, 2019 07:48
nornagon
nornagon reviewed Jun 27, 2019
View reviewed changes
Copy link
Member

@nornagon nornagon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious why the choice of mimeType as the filter. It seems like it might be better to filter on plugin path, or name? But I don't have all the info :)

patches/chromium/add_contentbrowserclient_ispluginsandboxdisabled_callback.patch
Date: Sat, 22 Jun 2019 12:23:04 +0200
Subject: Add ContentBrowserClient::IsPluginSandboxDisabled() callback

Allows the embedder to decide whether the plugin should be sandboxed.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please upstream this patch & add a link to the chromium codereview :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have never upstreamed stuff to Chromium so far, I need to try it for the first time. Any tips?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, I meant to respond to this ages ago. The best place to start is https://chromium.googlesource.com/chromium/src/+/master/docs/contributing.md

tldr: get a chromium checkout, make a branch, and run git cl upload.

shell/browser/atom_browser_client.cc
@@ -988,6 +989,17 @@ bool AtomBrowserClient::PreSpawnRenderer(sandbox::TargetPolicy* policy) {
}
#endif // defined(OS_WIN)

bool AtomBrowserClient::IsPluginSandboxDisabled(
const content::PepperPluginInfo& info) {
const auto& unsandboxed_plugins = Browser::Get()->unsandboxed_plugins();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be reasonable to implement this as a callback into JS? I'm imagining an API like

app.setPluginSandboxEnabledHandler((mimeType) => {
  return mimeType !== 'foo/bar'
})

As written, the API doesn't allow for re-enabling the sandbox after it's disabled, or for choosing which webContents are allowed to launch unsandboxed plugins. I'm not sure that it'd be likely to need such features, just asking whether we should consider them.

Copy link
Contributor Author

@miniak miniak Jun 28, 2019 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we can easily map the plugin to the webContents. Also the plugin hosting process is instantiated per plugin. Multiple webContents can share the same one.

My thinking was that there are plugins that just don't work with sandbox at all, so we just need a way to opt-out to make them work. I don't think it's necessary to do it with more granularity.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the JS handler on the other hand could get more information from content::PepperPluginInfo, including the name, path and MIME types

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Feverqwe what do you think as the person experiencing the issue that we are trying to solve here?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that any api will be fine. setPluginSandboxEnabledHandler is more flexible, but and disablePluginSandbox looks good.

@miniak miniak requested a review from deepak1556 July 5, 2019 14:42
@miniak miniak force-pushed the miniak/disable-plugin-sandbox branch from 2efa235 to 9db5b3b Compare July 7, 2019 11:28
@codebytere
Copy link
Member

@miniak do you still want to merge this? it's needing rebase

@codebytere
Copy link
Member

Closing for now given no action and no response to the above but we can re-open should there be a path forward :)

@codebytere codebytere closed this Nov 7, 2019
@miniak miniak mentioned this pull request Dec 9, 2019
Closed
3 tasks
@fp07
Copy link

fp07 commented Dec 26, 2019

Can we re-open this?

@zSoulweaver zSoulweaver mentioned this pull request Apr 26, 2020
Closed
@zcbenz zcbenz deleted the miniak/disable-plugin-sandbox branch June 1, 2020 10:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nornagon nornagon nornagon left review comments

@Feverqwe Feverqwe Feverqwe left review comments

@deepak1556 deepak1556 Awaiting requested review from deepak1556

Assignees

@miniak miniak

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@miniak @MarshallOfSound @codebytere @fp07 @nornagon @Feverqwe