Skip to content

Commit abe5cf3

Browse files
MarshallOfSoundMarshallOfSound
authored
refactor: port window.open and window.opener to use ctx bridge instead of hole punching (electron#23235)
* refactor: port window.open and window.opener to use ctx bridge instead of hole punching * refactor: only run the isolated init bundle when webview is enabled
1 parent c68589f commit abe5cf3

File tree

11 files changed

+178
-85
lines changed

11 files changed

+178
-85
lines changed

‎filenames.auto.gni

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -182,12 +182,8 @@ auto_filenames = {
182182
isolated_bundle_deps = [
183183
"lib/common/electron-binding-setup.ts",
184184
"lib/isolated_renderer/init.js",
185-
"lib/renderer/api/context-bridge.ts",
186-
"lib/renderer/ipc-renderer-internal-utils.ts",
187-
"lib/renderer/ipc-renderer-internal.ts",
188185
"lib/renderer/web-view/web-view-constants.ts",
189186
"lib/renderer/web-view/web-view-element.ts",
190-
"lib/renderer/window-setup.ts",
191187
"package.json",
192188
"tsconfig.electron.json",
193189
"tsconfig.json",

‎lib/browser/api/web-contents.js

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -548,6 +548,11 @@ WebContents.prototype._init = function () {
548548

549549
internalWindowOpen(event, url, referrer, frameName, disposition, mergedOptions, additionalFeatures, postData);
550550
});
551+
552+
const prefs = this.getWebPreferences() || {};
553+
if (prefs.webviewTag && prefs.contextIsolation) {
554+
electron.deprecate.log('Security Warning: A WebContents was just created with both webviewTag and contextIsolation enabled. This combination is fundamentally less secure and effectively bypasses the protections of contextIsolation. We strongly recommend you move away from webviews to OOPIF or BrowserView in order for your app to be more secure');
555+
}
551556
}
552557

553558
this.on('login', (event, ...args) => {

‎lib/isolated_renderer/init.js

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -6,22 +6,10 @@ process.electronBinding = require('@electron/internal/common/electron-binding-se
66

77
const v8Util = process.electronBinding('v8_util');
88

9-
// The `lib/renderer/ipc-renderer-internal.js` module looks for the ipc object in the
10-
// "ipc-internal" hidden value
11-
v8Util.setHiddenValue(global, 'ipc-internal', v8Util.getHiddenValue(isolatedWorld, 'ipc-internal'));
12-
139
const webViewImpl = v8Util.getHiddenValue(isolatedWorld, 'web-view-impl');
1410

1511
if (webViewImpl) {
1612
// Must setup the WebView element in main world.
1713
const { setupWebView } = require('@electron/internal/renderer/web-view/web-view-element');
1814
setupWebView(v8Util, webViewImpl);
1915
}
20-
21-
const isolatedWorldArgs = v8Util.getHiddenValue(isolatedWorld, 'isolated-world-args');
22-
23-
if (isolatedWorldArgs) {
24-
const { guestInstanceId, isHiddenPage, openerId, usesNativeWindowOpen, rendererProcessReuseEnabled } = isolatedWorldArgs;
25-
const { windowSetup } = require('@electron/internal/renderer/window-setup');
26-
windowSetup(guestInstanceId, openerId, isHiddenPage, usesNativeWindowOpen, rendererProcessReuseEnabled);
27-
}

‎lib/renderer/api/context-bridge.ts

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,8 +21,11 @@ export default contextBridge;
2121

2222
export const internalContextBridge = {
2323
contextIsolationEnabled,
24-
overrideGlobalMethodFromIsolatedWorld: (keys: string[], method: Function) => {
25-
return binding._overrideGlobalMethodFromIsolatedWorld(keys, method);
24+
overrideGlobalValueFromIsolatedWorld: (keys: string[], value: any) => {
25+
return binding._overrideGlobalValueFromIsolatedWorld(keys, value, false);
26+
},
27+
overrideGlobalValueWithDynamicPropsFromIsolatedWorld: (keys: string[], value: any) => {
28+
return binding._overrideGlobalValueFromIsolatedWorld(keys, value, true);
2629
},
2730
overrideGlobalPropertyFromIsolatedWorld: (keys: string[], getter: Function, setter?: Function) => {
2831
return binding._overrideGlobalPropertyFromIsolatedWorld(keys, getter, setter || null);

‎lib/renderer/window-setup.ts

Lines changed: 82 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -2,24 +2,18 @@ import { ipcRendererInternal } from '@electron/internal/renderer/ipc-renderer-in
22
import * as ipcRendererUtils from '@electron/internal/renderer/ipc-renderer-internal-utils';
33
import { internalContextBridge } from '@electron/internal/renderer/api/context-bridge';
44

5-
const inMainWorld = internalContextBridge.isInMainWorld();
65
const { contextIsolationEnabled } = internalContextBridge;
76

8-
// Should we inject APIs into this world, if ctx isolation is enabled then only inject in the isolated world
9-
// else inject everywhere
10-
const shouldInjectGivenContextIsolationIsMaybeEnabled = contextIsolationEnabled ? !inMainWorld : true;
11-
12-
// This file implements the following APIs Directly:
7+
// This file implements the following APIs over the ctx bridge:
138
// - window.open()
149
// - window.opener.blur()
1510
// - window.opener.close()
1611
// - window.opener.eval()
1712
// - window.opener.focus()
1813
// - window.opener.location
1914
// - window.opener.print()
15+
// - window.opener.closed
2016
// - window.opener.postMessage()
21-
22-
// And the following APIs over the ctx bridge:
2317
// - window.history.back()
2418
// - window.history.forward()
2519
// - window.history.go()
@@ -40,13 +34,13 @@ const toString = (value: any) => {
4034

4135
const windowProxies = new Map<number, BrowserWindowProxy>();
4236

43-
const getOrCreateProxy = (guestId: number) => {
37+
const getOrCreateProxy = (guestId: number): SafelyBoundBrowserWindowProxy => {
4438
let proxy = windowProxies.get(guestId);
4539
if (proxy == null) {
4640
proxy = new BrowserWindowProxy(guestId);
4741
windowProxies.set(guestId, proxy);
4842
}
49-
return proxy;
43+
return proxy.getSafe();
5044
};
5145

5246
const removeProxy = (guestId: number) => {
@@ -74,6 +68,8 @@ class LocationProxy {
7468
*/
7569
private static ProxyProperty<T> (target: LocationProxy, propertyKey: LocationProperties) {
7670
Object.defineProperty(target, propertyKey, {
71+
enumerable: true,
72+
configurable: true,
7773
get: function (this: LocationProxy): T | string {
7874
const guestURL = this.getGuestURL();
7975
const value = guestURL ? guestURL[propertyKey] : '';
@@ -92,6 +88,30 @@ class LocationProxy {
9288
});
9389
}
9490

91+
public getSafe = () => {
92+
const that = this;
93+
return {
94+
get href () { return that.href; },
95+
set href (newValue) { that.href = newValue; },
96+
get hash () { return that.hash; },
97+
set hash (newValue) { that.hash = newValue; },
98+
get host () { return that.host; },
99+
set host (newValue) { that.host = newValue; },
100+
get hostname () { return that.hostname; },
101+
set hostname (newValue) { that.hostname = newValue; },
102+
get origin () { return that.origin; },
103+
set origin (newValue) { that.origin = newValue; },
104+
get pathname () { return that.pathname; },
105+
set pathname (newValue) { that.pathname = newValue; },
106+
get port () { return that.port; },
107+
set port (newValue) { that.port = newValue; },
108+
get protocol () { return that.protocol; },
109+
set protocol (newValue) { that.protocol = newValue; },
110+
get search () { return that.search; },
111+
set search (newValue) { that.search = newValue; }
112+
};
113+
}
114+
95115
constructor (guestId: number) {
96116
// eslint will consider the constructor "useless"
97117
// unless we assign them in the body. It's fine, that's what
@@ -124,6 +144,17 @@ class LocationProxy {
124144
}
125145
}
126146

147+
interface SafelyBoundBrowserWindowProxy {
148+
location: WindowProxy['location'];
149+
blur: WindowProxy['blur'];
150+
close: WindowProxy['close'];
151+
eval: typeof eval; // eslint-disable-line no-eval
152+
focus: WindowProxy['focus'];
153+
print: WindowProxy['print'];
154+
postMessage: WindowProxy['postMessage'];
155+
closed: boolean;
156+
}
157+
127158
class BrowserWindowProxy {
128159
public closed: boolean = false
129160

@@ -134,7 +165,7 @@ class BrowserWindowProxy {
134165
// so for now, we'll have to make do with an "any" in the mix.
135166
// https://github.com/Microsoft/TypeScript/issues/2521
136167
public get location (): LocationProxy | any {
137-
return this._location;
168+
return this._location.getSafe();
138169
}
139170

140171
public set location (url: string | any) {
@@ -152,27 +183,48 @@ class BrowserWindowProxy {
152183
});
153184
}
154185

155-
public close () {
186+
public getSafe = (): SafelyBoundBrowserWindowProxy => {
187+
const that = this;
188+
return {
189+
postMessage: this.postMessage,
190+
blur: this.blur,
191+
close: this.close,
192+
focus: this.focus,
193+
print: this.print,
194+
eval: this.eval,
195+
get location () {
196+
return that.location;
197+
},
198+
set location (url: string | any) {
199+
that.location = url;
200+
},
201+
get closed () {
202+
return that.closed;
203+
}
204+
};
205+
}
206+
207+
public close = () => {
156208
this._invokeWindowMethod('destroy');
157209
}
158210

159-
public focus () {
211+
public focus = () => {
160212
this._invokeWindowMethod('focus');
161213
}
162214

163-
public blur () {
215+
public blur = () => {
164216
this._invokeWindowMethod('blur');
165217
}
166218

167-
public print () {
219+
public print = () => {
168220
this._invokeWebContentsMethod('print');
169221
}
170222

171-
public postMessage (message: any, targetOrigin: string) {
223+
public postMessage = (message: any, targetOrigin: string) => {
172224
ipcRendererInternal.invoke('ELECTRON_GUEST_WINDOW_MANAGER_WINDOW_POSTMESSAGE', this.guestId, message, toString(targetOrigin), window.location.origin);
173225
}
174226

175-
public eval (code: string) {
227+
public eval = (code: string) => {
176228
this._invokeWebContentsMethod('executeJavaScript', code);
177229
}
178230

@@ -188,12 +240,12 @@ class BrowserWindowProxy {
188240
export const windowSetup = (
189241
guestInstanceId: number, openerId: number, isHiddenPage: boolean, usesNativeWindowOpen: boolean, rendererProcessReuseEnabled: boolean
190242
) => {
191-
if (!process.sandboxed && guestInstanceId == null && shouldInjectGivenContextIsolationIsMaybeEnabled) {
243+
if (!process.sandboxed && guestInstanceId == null) {
192244
// Override default window.close.
193245
window.close = function () {
194246
ipcRendererInternal.send('ELECTRON_BROWSER_WINDOW_CLOSE');
195247
};
196-
if (contextIsolationEnabled) internalContextBridge.overrideGlobalMethodFromIsolatedWorld(['close'], window.close);
248+
if (contextIsolationEnabled) internalContextBridge.overrideGlobalValueFromIsolatedWorld(['close'], window.close);
197249
}
198250

199251
if (!usesNativeWindowOpen) {
@@ -210,23 +262,21 @@ export const windowSetup = (
210262
return null;
211263
}
212264
};
265+
if (contextIsolationEnabled) internalContextBridge.overrideGlobalValueWithDynamicPropsFromIsolatedWorld(['open'], window.open);
213266
}
214267

215268
if (openerId != null) {
216-
// TODO(MarshallOfSound): Make compatible with ctx isolation without hole-punch
217269
window.opener = getOrCreateProxy(openerId);
270+
if (contextIsolationEnabled) internalContextBridge.overrideGlobalValueWithDynamicPropsFromIsolatedWorld(['opener'], window.opener);
218271
}
219272

220273
// But we do not support prompt().
221-
if (shouldInjectGivenContextIsolationIsMaybeEnabled) {
222-
window.prompt = function () {
223-
throw new Error('prompt() is and will not be supported.');
224-
};
225-
if (contextIsolationEnabled) internalContextBridge.overrideGlobalMethodFromIsolatedWorld(['prompt'], window.prompt);
226-
}
274+
window.prompt = function () {
275+
throw new Error('prompt() is and will not be supported.');
276+
};
277+
if (contextIsolationEnabled) internalContextBridge.overrideGlobalValueFromIsolatedWorld(['prompt'], window.prompt);
227278

228279
if (!usesNativeWindowOpen || openerId != null) {
229-
// TODO(MarshallOfSound): Make compatible with ctx isolation without hole-punch
230280
ipcRendererInternal.on('ELECTRON_GUEST_WINDOW_POSTMESSAGE', function (
231281
_event, sourceId: number, message: any, sourceOrigin: string
232282
) {
@@ -247,21 +297,21 @@ export const windowSetup = (
247297
});
248298
}
249299

250-
if (!process.sandboxed && !rendererProcessReuseEnabled && shouldInjectGivenContextIsolationIsMaybeEnabled) {
300+
if (!process.sandboxed && !rendererProcessReuseEnabled) {
251301
window.history.back = function () {
252302
ipcRendererInternal.send('ELECTRON_NAVIGATION_CONTROLLER_GO_BACK');
253303
};
254-
if (contextIsolationEnabled) internalContextBridge.overrideGlobalMethodFromIsolatedWorld(['history', 'back'], window.history.back);
304+
if (contextIsolationEnabled) internalContextBridge.overrideGlobalValueFromIsolatedWorld(['history', 'back'], window.history.back);
255305

256306
window.history.forward = function () {
257307
ipcRendererInternal.send('ELECTRON_NAVIGATION_CONTROLLER_GO_FORWARD');
258308
};
259-
if (contextIsolationEnabled) internalContextBridge.overrideGlobalMethodFromIsolatedWorld(['history', 'forward'], window.history.forward);
309+
if (contextIsolationEnabled) internalContextBridge.overrideGlobalValueFromIsolatedWorld(['history', 'forward'], window.history.forward);
260310

261311
window.history.go = function (offset: number) {
262312
ipcRendererInternal.send('ELECTRON_NAVIGATION_CONTROLLER_GO_TO_OFFSET', +offset);
263313
};
264-
if (contextIsolationEnabled) internalContextBridge.overrideGlobalMethodFromIsolatedWorld(['history', 'go'], window.history.go);
314+
if (contextIsolationEnabled) internalContextBridge.overrideGlobalValueFromIsolatedWorld(['history', 'go'], window.history.go);
265315

266316
const getHistoryLength = () => ipcRendererInternal.sendSync('ELECTRON_NAVIGATION_CONTROLLER_LENGTH') + 104;
267317
Object.defineProperty(window.history, 'length', {
@@ -271,7 +321,7 @@ export const windowSetup = (
271321
if (contextIsolationEnabled) internalContextBridge.overrideGlobalPropertyFromIsolatedWorld(['history', 'length'], getHistoryLength);
272322
}
273323

274-
if (guestInstanceId != null && shouldInjectGivenContextIsolationIsMaybeEnabled) {
324+
if (guestInstanceId != null) {
275325
// Webview `document.visibilityState` tracks window visibility (and ignores
276326
// the actual <webview> element visibility) for backwards compatibility.
277327
// See discussion in #9178.

0 commit comments

Comments
 (0)