security: don't pass preloadPath via ELECTRON_BROWSER_SANDBOX_LOAD #13031
Conversation
174d709 to
2c79eb5
Compare
2c79eb5 to
2da10ee
Compare
04dd752 to
708457e
Compare
|
@zcbenz When I preserve the (buggy?) preload script behavior by adding |
|
@miniak I'm not familiar with the code path in sandboxed renderer at all, I don't have an answer after looking at the code. We need a redesign of the whole WebPreferences system, currently it is inherited but with different fields filtered at different places, and some places are expecting the latest values while some places are keeping a copy of modified values. |
284f0fc to
77498fb
Compare
37d502e to
5b37766
Compare
| command_line->AppendSwitchPath(switches::kPreloadScript, preload_path); | ||
| else | ||
| LOG(ERROR) << "preload url must be file:// protocol."; | ||
| if (!command_line->HasSwitch(switches::kEnableSandbox)) { |
There was a problem hiding this comment.
kEnableSandbox is about enabling the chromium sandbox, but it is not necessary for enabling sandbox mode
There was a problem hiding this comment.
@tarruda I can revert this part. I just wanted to avoid passing the preload script path via commandline as the sandboxed renderer is not going to use it anyway.
docs/api/browser-window.md
Outdated
| @@ -263,6 +263,8 @@ It creates a new `BrowserWindow` with native properties as set by the `options`. | |||
| When node integration is turned off, the preload script can reintroduce | |||
| Node global symbols back to the global scope. See example | |||
| [here](process.md#event-loaded). | |||
| * `inheritPreload` Boolean (optional) - Whether the preload script is inherited | |||
There was a problem hiding this comment.
I thought we were supposed to add a more generic mechanism, like a callback that can modify webPreferences and add the preload path to the parent window.
There was a problem hiding this comment.
@tarruda yes, but I didn't have time yet to investigate how to do it :(
5b37766 to
85730fb
Compare
76e4bcd to
77970f7
Compare
|
@tarruda I finally had time to investigate how to set the preload script for the popup. I've updated the PR. |
77970f7 to
91cf65a
Compare
2a1e7bc to
82a84cf
Compare
|
@tarruda I had to tweak it a bit more as |
82a84cf to
db1b937
Compare
lib/browser/rpc-server.js
Outdated
| platform: process.platform, | ||
| env: process.env | ||
| } | ||
| }) | ||
|
|
||
| const getPreloadPath = function (webContents) { |
There was a problem hiding this comment.
equivalent of the code in https://github.com/electron/electron/blob/master/atom/browser/web_contents_preferences.cc
// The preload script.
base::FilePath::StringType preload;
if (GetAsString(&preference_, options::kPreloadScript, &preload)) {
if (base::FilePath(preload).IsAbsolute())
command_line->AppendSwitchNative(switches::kPreloadScript, preload);
else
LOG(ERROR) << "preload script must have absolute path.";
} else if (GetAsString(&preference_, options::kPreloadURL, &preload)) {
// Translate to file path if there is "preload-url" option.
base::FilePath preload_path;
if (net::FileURLToFilePath(GURL(preload), &preload_path))
command_line->AppendSwitchPath(switches::kPreloadScript, preload_path);
else
LOG(ERROR) << "preload url must be file:// protocol.";
}
29af450 to
7c719a6
Compare
|
@MarshallOfSound is anything preventing us from merging this PR? |
7c719a6 to
294d3fb
Compare
| @@ -1409,6 +1409,8 @@ describe('BrowserWindow module', () => { | |||
| preload: preload | |||
| } | |||
| }) | |||
| ipcRenderer.send('set-web-preferences-on-next-new-window', w.webContents.id, 'preload', preload) | |||
There was a problem hiding this comment.
other tests are already using this IPC to configure popups
There was a problem hiding this comment.
in spec/static/main.js
ipcMain.on('set-web-preferences-on-next-new-window', (event, id, key, value) => {
webContents.fromId(id).once('new-window', (event, url, frameName, disposition, options) => {
options.webPreferences[key] = value
})
})
There was a problem hiding this comment.
|
@codebytere, @ckerr can you please review? |
| v8::Local<v8::Value> WebContents::GetPreloadPath(v8::Isolate* isolate) const { | ||
| base::FilePath::StringType preload; | ||
| if (auto* web_preferences = WebContentsPreferences::From(web_contents())) { | ||
| web_preferences->GetPreloadPath(&preload); |
There was a problem hiding this comment.
should check the retval here, something more like
if (auto* web_preferences = WebContentsPreferences::From(web_contents()) {
if (web_preferences->GetPreloadPath(&preload)) {
return mate::ConvertToV8(isolate, preload);
}
}
return v8::Null(isolate);| LOG(ERROR) << "preload url must be file:// protocol."; | ||
| } | ||
| if (GetPreloadPath(&preload)) | ||
| command_line->AppendSwitchNative(switches::kPreloadScript, preload); |
There was a problem hiding this comment.
I'm not 100% sure about this? -- the removed code appends with either AppendSwitchNative or AppendSwitchPath depending on the code branch, but the new code always uses AppendSwitchNative. Is this intentional?
There was a problem hiding this comment.
@ckerr it's ok, in preloadURL case, I return preload_path.value()
void CommandLine::AppendSwitchPath(const std::string& switch_string,
const FilePath& path) {
AppendSwitchNative(switch_string, path.value());
}
294d3fb to
c5efac7
Compare
| @@ -1884,6 +1884,16 @@ void WebContents::OnGetZoomLevel(content::RenderFrameHost* rfh, | |||
| rfh->Send(reply_msg); | |||
| } | |||
|
|
|||
| v8::Local<v8::Value> WebContents::GetPreloadPath(v8::Isolate* isolate) const { | |||
| base::FilePath::StringType preload; | |||
| if (auto* web_preferences = WebContentsPreferences::From(web_contents())) { | |||
There was a problem hiding this comment.
(trivial) declaration of preload could be moved inside this if { clause
| @@ -440,7 +440,8 @@ ipcMain.on('ELECTRON_BROWSER_WINDOW_CLOSE', function (event) { | |||
| event.returnValue = null | |||
| }) | |||
|
|
|||
| ipcMain.on('ELECTRON_BROWSER_SANDBOX_LOAD', function (event, preloadPath) { | |||
| ipcMain.on('ELECTRON_BROWSER_SANDBOX_LOAD', function (event) { | |||
| const preloadPath = event.sender._getPreloadPath() | |||
There was a problem hiding this comment.
While we're in the neighborhood, the code below this
event.returnValue = {
preloadSrc: preloadSrc,
preloadError: preloadError,
...could be updated to use ES6 enhanced object literals:
event.returnValue = {
preloadSrc,
preloadError,
...c5efac7 to
3fa4294
Compare
|
@miniak, thanks for the quick changes! Somewhere along the line |
3fa4294 to
5636536
Compare
|
@ckerr rebased + conflict resolved |
|
An error occurred while attempting to backport this PR to "3-0-x", you will need to perform this backport manually |
Follow up to #12877.
@tarruda and @MarshallOfSound please review.
It also allows different sandboxed webContents to share the renderer process (affinity) even if they have different preload scripts (as it does not have to be passed on the command line).
Checklist
npm testpasses