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.

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

feat: remove BrowserWindow option inheritance #28550

Merged
merged 11 commits into from Apr 21, 2021
13 changes: 8 additions & 5 deletions docs/api/web-contents.md
Expand Up @@ -203,9 +203,9 @@ Returns:
* `frameName` String - Name given to the created window in the
`window.open()` call.
* `options` BrowserWindowConstructorOptions - The options used to create the
BrowserWindow. They are merged in increasing precedence: options inherited
from the parent, parsed options from the `features` string from
`window.open()`, and options given by
BrowserWindow. They are merged in increasing precedence: parsed options
from the `features` string from `window.open()`, security-related
webPreferences inherited from the parent, and options given by
[`webContents.setWindowOpenHandler`](web-contents.md#contentssetwindowopenhandlerhandler).
Unrecognized options are not filtered out.
* `referrer` [Referrer](structures/referrer.md) - The referrer that will be
Expand Down Expand Up @@ -1148,8 +1148,11 @@ Ignore application menu shortcuts while this web contents is focused.
without a recognized 'action' value will result in a console error and have
the same effect as returning `{action: 'deny'}`.

Called before creating a window when `window.open()` is called from the
renderer. See [`window.open()`](window-open.md) for more details and how to use this in conjunction with `did-create-window`.
Called before creating a window a new window is requested by the renderer, e.g.
by `window.open()`, a link with `target="_blank"`, shift+clicking on a link, or
submitting a form with `<form target="_blank">`. See
[`window.open()`](window-open.md) for more details and how to use this in
conjunction with `did-create-window`.

#### `contents.setAudioMuted(muted)`

Expand Down
5 changes: 2 additions & 3 deletions docs/api/window-open.md
Expand Up @@ -22,9 +22,8 @@ BrowserWindow in the main process by using `webContents.setWindowOpenHandler()`
for renderer-created windows.

BrowserWindow constructor options are set by, in increasing precedence
order: options inherited from the parent, parsed options
from the `features` string from `window.open()`, security-related webPreferences
inherited from the parent, and options given by
order: parsed options from the `features` string from `window.open()`,
security-related webPreferences inherited from the parent, and options given by
[`webContents.setWindowOpenHandler`](web-contents.md#contentssetwindowopenhandlerhandler).
Note that `webContents.setWindowOpenHandler` has final say and full privilege
because it is invoked in the main process.
Expand Down
21 changes: 21 additions & 0 deletions docs/breaking-changes.md
Expand Up @@ -38,6 +38,27 @@ to open synchronously scriptable child windows, among other incompatibilities.
See the documentation for [window.open in Electron](api/window-open.md)
for more details.

### Removed: BrowserWindowConstructorOptions inheriting from parent windows

Prior to Electron 14, windows opened with `window.open` would inherit
BrowserWindow constructor options such as `transparent` and `resizable` from
their parent window. Beginning with Electron 14, this behavior is removed, and
windows will not inherit any BrowserWindow constructor options from their
parents.

Instead, explicitly set options for the new window with `setWindowOpenHandler`:

```js
webContents.setWindowOpenHandler((details) => {
return {
action: 'allow',
overrideBrowserWindowOptions: {
// ...
}
}
})
```

### Removed: `additionalFeatures`

The deprecated `additionalFeatures` property in the `new-window` and
Expand Down
36 changes: 7 additions & 29 deletions lib/browser/guest-window-manager.ts
Expand Up @@ -198,39 +198,37 @@ const securityWebPreferences: { [key: string]: boolean } = {
enableWebSQL: false
};

function makeBrowserWindowOptions ({ embedder, features, overrideOptions, useDeprecatedBehaviorForOptionInheritance = true }: {
function makeBrowserWindowOptions ({ embedder, features, overrideOptions }: {
embedder: WebContents,
features: string,
overrideOptions?: BrowserWindowConstructorOptions,
useDeprecatedBehaviorForOptionInheritance?: boolean
}) {
const { options: parsedOptions, webPreferences: parsedWebPreferences } = parseFeatures(features);

const deprecatedInheritedOptions = getDeprecatedInheritedOptions(embedder);

return {
options: {
...(useDeprecatedBehaviorForOptionInheritance && deprecatedInheritedOptions),
show: true,
width: 800,
height: 600,
...parsedOptions,
...overrideOptions,
webPreferences: makeWebPreferences({ embedder, insecureParsedWebPreferences: parsedWebPreferences, secureOverrideWebPreferences: overrideOptions && overrideOptions.webPreferences, useDeprecatedBehaviorForOptionInheritance: true })
webPreferences: makeWebPreferences({
embedder,
insecureParsedWebPreferences: parsedWebPreferences,
secureOverrideWebPreferences: overrideOptions && overrideOptions.webPreferences
})
} as Electron.BrowserViewConstructorOptions
};
}

export function makeWebPreferences ({ embedder, secureOverrideWebPreferences = {}, insecureParsedWebPreferences: parsedWebPreferences = {}, useDeprecatedBehaviorForOptionInheritance = true }: {
export function makeWebPreferences ({ embedder, secureOverrideWebPreferences = {}, insecureParsedWebPreferences: parsedWebPreferences = {} }: {
embedder: WebContents,
insecureParsedWebPreferences?: ReturnType<typeof parseFeatures>['webPreferences'],
// Note that override preferences are considered elevated, and should only be
// sourced from the main process, as they override security defaults. If you
// have unvetted prefs, use parsedWebPreferences.
secureOverrideWebPreferences?: BrowserWindowConstructorOptions['webPreferences'],
useDeprecatedBehaviorForOptionInheritance?: boolean
}) {
const deprecatedInheritedOptions = getDeprecatedInheritedOptions(embedder);
const parentWebPreferences = embedder.getLastWebPreferences();
const securityWebPreferencesFromParent = (Object.keys(securityWebPreferences).reduce((map, key) => {
if (securityWebPreferences[key] === parentWebPreferences[key as keyof Electron.WebPreferences]) {
Expand All @@ -241,7 +239,6 @@ export function makeWebPreferences ({ embedder, secureOverrideWebPreferences = {
const openerId = parentWebPreferences.nativeWindowOpen ? null : embedder.id;

return {
...(useDeprecatedBehaviorForOptionInheritance && deprecatedInheritedOptions ? deprecatedInheritedOptions.webPreferences : null),
...parsedWebPreferences,
// Note that order is key here, we want to disallow the renderer's
// ability to change important security options but allow main (via
Expand All @@ -254,25 +251,6 @@ export function makeWebPreferences ({ embedder, secureOverrideWebPreferences = {
};
}

/**
* Current Electron behavior is to inherit all options from the parent window.
* In practical use, this is kind of annoying because consumers have to know
* about the parent window's preferences in order to unset them and makes child
* windows even more of an anomaly. In 11.0.0 we will remove this behavior and
* only critical security preferences will be inherited by default.
*/
function getDeprecatedInheritedOptions (embedder: WebContents) {
if (!embedder.browserWindowOptions) {
// If it's a webview, return just the webPreferences.
return {
webPreferences: embedder.getLastWebPreferences()
};
}

const { type, show, ...inheritableOptions } = embedder.browserWindowOptions;
return inheritableOptions;
}

function formatPostDataHeaders (postData: PostData) {
if (!postData) return;

Expand Down
2 changes: 1 addition & 1 deletion lib/browser/guest-window-proxy.ts
Expand Up @@ -135,7 +135,7 @@ handleMessage(

if (!windowMethods.has(method)) {
console.error(
`Blocked ${event.sender.getURL()} from calling method: ${method}`
`Blocked ${event.senderFrame.url} from calling method: ${method}`
);
throw new Error(`Invalid method: ${method}`);
}
Expand Down
19 changes: 18 additions & 1 deletion lib/renderer/window-setup.ts
Expand Up @@ -270,7 +270,24 @@ export const windowSetup = (
if (contextIsolationEnabled) internalContextBridge.overrideGlobalValueWithDynamicPropsFromIsolatedWorld(['open'], window.open);
}

if (openerId != null) {
// If this window uses nativeWindowOpen, but its opener window does not, we
// need to proxy window.opener in order to let the page communicate with its
// opener.
// Additionally, windows opened from a nativeWindowOpen child of a
// non-nativeWindowOpen parent will initially have their WebPreferences
// copied from their opener before having them updated, meaning openerId is
// initially incorrect. We detect this situation by checking for
// window.opener, which will be non-null for a natively-opened child, so we
// can ignore the openerId in that case, since it's incorrectly copied from
// the parent. This is, uh, confusing, so here's a diagram that will maybe
// help?
//
// [ grandparent window ] --> [ parent window ] --> [ child window ]
// n.W.O = false n.W.O = true n.W.O = true
// id = 1 id = 2 id = 3
// openerId = null openerId = 1 openerId = 1 <- !!wrong!!
// opener = null opener = null opener = [parent window]
if (openerId != null && !window.opener) {
window.opener = getOrCreateProxy(openerId);
if (contextIsolationEnabled) internalContextBridge.overrideGlobalValueWithDynamicPropsFromIsolatedWorld(['opener'], window.opener);
}
Expand Down
4 changes: 0 additions & 4 deletions shell/browser/api/electron_api_browser_window.cc
Expand Up @@ -78,10 +78,6 @@ BrowserWindow::BrowserWindow(gin::Arguments* args,
api_web_contents_->AddObserver(this);
Observe(api_web_contents_->web_contents());

// Keep a copy of the options for later use.
gin_helper::Dictionary(isolate, web_contents.ToV8().As<v8::Object>())
.Set("browserWindowOptions", options);

// Associate with BrowserWindow.
web_contents->SetOwnerWindow(window());

Expand Down
10 changes: 8 additions & 2 deletions spec-main/api-browser-window-spec.ts
Expand Up @@ -2372,7 +2372,7 @@ describe('BrowserWindow module', () => {
}
});
let childWc: WebContents | null = null;
w.webContents.setWindowOpenHandler(() => ({ action: 'allow', overrideBrowserWindowOptions: { webPreferences: { preload } } }));
w.webContents.setWindowOpenHandler(() => ({ action: 'allow', overrideBrowserWindowOptions: { webPreferences: { preload, contextIsolation: false } } }));

w.webContents.on('did-create-window', (win) => {
childWc = win.webContents;
Expand Down Expand Up @@ -2587,6 +2587,10 @@ describe('BrowserWindow module', () => {
preload
}
});
w.webContents.setWindowOpenHandler(() => ({
action: 'allow',
overrideBrowserWindowOptions: { show: false, webPreferences: { contextIsolation: false, webviewTag: true, nativeWindowOpen: true, nodeIntegrationInSubFrames: true } }
}));
w.webContents.once('new-window', (event, url, frameName, disposition, options) => {
options.show = false;
});
Expand Down Expand Up @@ -2665,7 +2669,9 @@ describe('BrowserWindow module', () => {
action: 'allow',
overrideBrowserWindowOptions: {
webPreferences: {
preload: path.join(fixtures, 'api', 'window-open-preload.js')
preload: path.join(fixtures, 'api', 'window-open-preload.js'),
contextIsolation: false,
nodeIntegrationInSubFrames: true
}
}
}));
Expand Down
2 changes: 1 addition & 1 deletion spec-main/chromium-spec.ts
Expand Up @@ -1042,7 +1042,7 @@ describe('chromium features', () => {

const parentCode = `new Promise((resolve) => {
// This is context (3), a child window of the WebView.
const child = window.open(${JSON.stringify(child)}, "", "show=no")
const child = window.open(${JSON.stringify(child)}, "", "show=no,contextIsolation=no,nodeIntegration=yes")
window.addEventListener("message", e => {
resolve(e.data)
})
Expand Down
89 changes: 36 additions & 53 deletions spec-main/fixtures/snapshots/native-window-open.snapshot.txt
Expand Up @@ -8,27 +8,23 @@
"frame-name",
"new-window",
{
"show": true,
"width": 800,
"title": "cool",
"backgroundColor": "blue",
"focusable": false,
"height": 600,
"top": 5,
"left": 10,
"resizable": false,
"x": 10,
"y": 5,
"webPreferences": {
"nativeWindowOpen": true,
"sandbox": true,
"backgroundColor": "blue",
"contextIsolation": true,
"nativeWindowOpen": true,
"nodeIntegration": false,
"sandbox": true,
"webviewTag": false,
"nodeIntegrationInSubFrames": false,
"openerId": null
},
"show": true,
"height": 600,
"top": 5,
"left": 10,
"resizable": false,
"x": 10,
"y": 5,
"webContents": "[WebContents]"
},
[],
Expand All @@ -47,26 +43,22 @@
"frame-name",
"new-window",
{
"show": true,
"width": 800,
"title": "cool",
"backgroundColor": "blue",
"focusable": false,
"height": 600,
"resizable": false,
"x": 0,
"y": 10,
"webPreferences": {
"nativeWindowOpen": true,
"sandbox": true,
"backgroundColor": "blue",
"zoomFactor": "2",
"contextIsolation": true,
"nativeWindowOpen": true,
"nodeIntegration": false,
"sandbox": true,
"webviewTag": false,
"nodeIntegrationInSubFrames": false,
"openerId": null
},
"show": true,
"height": 600,
"resizable": false,
"x": 0,
"y": 10,
"webContents": "[WebContents]"
},
[],
Expand All @@ -85,22 +77,20 @@
"frame-name",
"new-window",
{
"show": true,
"width": 800,
"title": "cool",
"height": 600,
"backgroundColor": "gray",
"focusable": false,
"webPreferences": {
"nativeWindowOpen": true,
"sandbox": true,
"backgroundColor": "gray",
"contextIsolation": true,
"nativeWindowOpen": true,
"nodeIntegration": false,
"sandbox": true,
"webviewTag": false,
"nodeIntegrationInSubFrames": false,
"openerId": null
"openerId": null,
"backgroundColor": "gray"
},
"show": true,
"height": 600,
"x": 100,
"y": 100,
"webContents": "[WebContents]"
Expand All @@ -121,24 +111,21 @@
"frame-name",
"new-window",
{
"show": true,
"width": 800,
"height": 600,
"x": 50,
"y": 20,
"title": "sup",
"backgroundColor": "blue",
"focusable": false,
"webPreferences": {
"nativeWindowOpen": true,
"sandbox": true,
"backgroundColor": "blue",
"contextIsolation": true,
"nativeWindowOpen": true,
"nodeIntegration": false,
"sandbox": true,
"webviewTag": false,
"nodeIntegrationInSubFrames": false,
"openerId": null
},
"show": true,
"height": 600,
"x": 50,
"y": 20,
"webContents": "[WebContents]"
},
[],
Expand All @@ -157,26 +144,22 @@
"frame-name",
"new-window",
{
"show": false,
"width": 800,
"title": "cool",
"backgroundColor": "blue",
"focusable": false,
"height": 600,
"top": 1,
"left": 1,
"x": 1,
"y": 1,
"webPreferences": {
"nativeWindowOpen": true,
"sandbox": true,
"backgroundColor": "blue",
"contextIsolation": true,
"nativeWindowOpen": true,
"nodeIntegration": false,
"sandbox": true,
"webviewTag": false,
"nodeIntegrationInSubFrames": false,
"openerId": null
},
"show": false,
"height": 600,
"top": 1,
"left": 1,
"x": 1,
"y": 1,
"webContents": "[WebContents]"
},
[],
Expand Down