Skip to content

Commit

Permalink
feat: remove BrowserWindow option inheritance (#28550)
Browse files Browse the repository at this point in the history
  • Loading branch information
@nornagon
nornagon committed Apr 21, 2021
1 parent c4931ff commit 4ca5184
Show file tree
Hide file tree
Showing 13 changed files with 121 additions and 111 deletions.
13 changes: 8 additions & 5 deletions docs/api/web-contents.md
View file
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
View file
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
View file
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
View file
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
View file
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
View file
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
View file
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
View file
Expand Up @@ -2383,7 +2383,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 @@ -2598,6 +2598,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 @@ -2676,7 +2680,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
View file
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
View file
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

0 comments on commit 4ca5184

Please sign in to comment.