Skip to content

Commit

Permalink
refactor: improve feature string parsing (#23130)
Browse files Browse the repository at this point in the history
* test: add pre-change snapshot of new-window event

* move to .ts file for easier diff

* refactor: improve feature string parsing logic

* test: update snapshots

* update type names per review

* update comma-separated parse test

* use for loop instead of reduce per review

* tighten up types

* avoid variable guest contents id returnValue in test snapshot
  • Loading branch information
loc committed Apr 21, 2020
1 parent b3909f5 commit aca2e4f
Show file tree
Hide file tree
Showing 10 changed files with 589 additions and 108 deletions.
2 changes: 1 addition & 1 deletion filenames.auto.gni
Original file line number Diff line number Diff line change
Expand Up @@ -274,7 +274,7 @@ auto_filenames = {
"lib/common/define-properties.ts",
"lib/common/electron-binding-setup.ts",
"lib/common/init.ts",
"lib/common/parse-features-string.js",
"lib/common/parse-features-string.ts",
"lib/common/reset-search-paths.ts",
"lib/common/type-utils.ts",
"lib/common/web-view-methods.ts",
Expand Down
10 changes: 7 additions & 3 deletions lib/browser/api/web-contents.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ const { internalWindowOpen } = require('@electron/internal/browser/guest-window-
const NavigationController = require('@electron/internal/browser/navigation-controller');
const { ipcMainInternal } = require('@electron/internal/browser/ipc-main-internal');
const ipcMainUtils = require('@electron/internal/browser/ipc-main-internal-utils');
const { convertFeaturesString } = require('@electron/internal/common/parse-features-string');
const { parseFeatures } = require('@electron/internal/common/parse-features-string');
const { MessagePortMain } = require('@electron/internal/browser/message-port-main');

// session is not used here, the purpose is to make sure session is initalized
Expand Down Expand Up @@ -511,11 +511,13 @@ WebContents.prototype._init = function () {
// Make new windows requested by links behave like "window.open".
this.on('-new-window', (event, url, frameName, disposition,
rawFeatures, referrer, postData) => {
const { options, additionalFeatures } = convertFeaturesString(rawFeatures, frameName);
const { options, webPreferences, additionalFeatures } = parseFeatures(rawFeatures);
const mergedOptions = {
show: true,
width: 800,
height: 600,
title: frameName,
webPreferences,
...options
};

Expand All @@ -533,12 +535,14 @@ WebContents.prototype._init = function () {
return;
}

const { options, additionalFeatures } = convertFeaturesString(rawFeatures, frameName);
const { options, webPreferences, additionalFeatures } = parseFeatures(rawFeatures);
const mergedOptions = {
show: true,
width: 800,
height: 600,
webContents,
title: frameName,
webPreferences,
...options
};

Expand Down
24 changes: 10 additions & 14 deletions lib/browser/guest-view-manager.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
const { webContents } = require('electron');
const { ipcMainInternal } = require('@electron/internal/browser/ipc-main-internal');
const ipcMainUtils = require('@electron/internal/browser/ipc-main-internal-utils');
const { parseFeaturesString } = require('@electron/internal/common/parse-features-string');
const { parseWebViewWebPreferences } = require('@electron/internal/common/parse-features-string');
const { syncMethods, asyncMethods, properties } = require('@electron/internal/common/web-view-methods');
const { serialize } = require('@electron/internal/common/type-utils');

Expand Down Expand Up @@ -184,6 +184,13 @@ const attachGuest = function (event, embedderFrameId, elementInstanceId, guestIn
}
}

// parse the 'webpreferences' attribute string, if set
// this uses the same parsing rules as window.open uses for its features
const parsedWebPreferences =
typeof params.webpreferences === 'string'
? parseWebViewWebPreferences(params.webpreferences)
: null;

const webPreferences = {
guestInstanceId: guestInstanceId,
nodeIntegration: params.nodeintegration != null ? params.nodeintegration : false,
Expand All @@ -194,21 +201,10 @@ const attachGuest = function (event, embedderFrameId, elementInstanceId, guestIn
disablePopups: !params.allowpopups,
webSecurity: !params.disablewebsecurity,
enableBlinkFeatures: params.blinkfeatures,
disableBlinkFeatures: params.disableblinkfeatures
disableBlinkFeatures: params.disableblinkfeatures,
...parsedWebPreferences
};

// parse the 'webpreferences' attribute string, if set
// this uses the same parsing rules as window.open uses for its features
if (typeof params.webpreferences === 'string') {
parseFeaturesString(params.webpreferences, function (key, value) {
if (value === undefined) {
// no value was specified, default it to true
value = true;
}
webPreferences[key] = value;
});
}

if (params.preload) {
webPreferences.preloadURL = params.preload;
}
Expand Down
7 changes: 5 additions & 2 deletions lib/browser/guest-window-manager.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ const { BrowserWindow } = electron;
const { isSameOrigin } = process.electronBinding('v8_util');
const { ipcMainInternal } = require('@electron/internal/browser/ipc-main-internal');
const ipcMainUtils = require('@electron/internal/browser/ipc-main-internal-utils');
const { convertFeaturesString } = require('@electron/internal/common/parse-features-string');
const { parseFeatures } = require('@electron/internal/common/parse-features-string');

const hasProp = {}.hasOwnProperty;
const frameToGuest = new Map();
Expand Down Expand Up @@ -224,7 +224,10 @@ ipcMainInternal.on('ELECTRON_GUEST_WINDOW_MANAGER_WINDOW_OPEN', (event, url, fra
if (features == null) features = '';

const disposition = 'new-window';
const { options, additionalFeatures } = convertFeaturesString(features, frameName);
const { options, webPreferences, additionalFeatures } = parseFeatures(features);
if (!options.title) options.title = frameName;
options.webPreferences = webPreferences;

const referrer = { url: '', policy: 'default' };
internalWindowOpen(event, url, referrer, frameName, disposition, options, additionalFeatures, null);
});
Expand Down
85 changes: 0 additions & 85 deletions lib/common/parse-features-string.js

This file was deleted.

110 changes: 110 additions & 0 deletions lib/common/parse-features-string.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
/**
* Utilities to parse comma-separated key value pairs used in browser APIs.
* For example: "x=100,y=200,width=500,height=500"
*/
import { BrowserWindowConstructorOptions } from 'electron';

type RequiredBrowserWindowConstructorOptions = Required<BrowserWindowConstructorOptions>;
type IntegerBrowserWindowOptionKeys = {
[K in keyof RequiredBrowserWindowConstructorOptions]:
RequiredBrowserWindowConstructorOptions[K] extends number ? K : never
}[keyof RequiredBrowserWindowConstructorOptions];

// This could be an array of keys, but an object allows us to add a compile-time
// check validating that we haven't added an integer property to
// BrowserWindowConstructorOptions that this module doesn't know about.
const keysOfTypeNumberCompileTimeCheck: { [K in IntegerBrowserWindowOptionKeys] : true } = {
x: true,
y: true,
width: true,
height: true,
minWidth: true,
maxWidth: true,
minHeight: true,
maxHeight: true,
opacity: true
};
// Note `top` / `left` are special cases from the browser which we later convert
// to y / x.
const keysOfTypeNumber = ['top', 'left', ...Object.keys(keysOfTypeNumberCompileTimeCheck)];

/**
* Note that we only allow "0" and "1" boolean conversion when the type is known
* not to be an integer.
*
* The coercion of yes/no/1/0 represents best effort accordance with the spec:
* https://html.spec.whatwg.org/multipage/window-object.html#concept-window-open-features-parse-boolean
*/
type CoercedValue = string | number | boolean;
function coerce (key: string, value: string): CoercedValue {
if (keysOfTypeNumber.includes(key)) {
return Number(value);
}

switch (value) {
case 'true':
case '1':
case 'yes':
case undefined:
return true;
case 'false':
case '0':
case 'no':
return false;
default:
return value;
}
}

export function parseCommaSeparatedKeyValue (source: string, useSoonToBeDeprecatedBehaviorForBareKeys: boolean) {
const bareKeys = [] as string[];
const parsed = {} as { [key: string]: any };
for (const keyValuePair of source.split(',')) {
const [key, value] = keyValuePair.split('=').map(str => str.trim());
if (useSoonToBeDeprecatedBehaviorForBareKeys && value === undefined) {
bareKeys.push(key);
continue;
}
parsed[key] = coerce(key, value);
}

return { parsed, bareKeys };
}

export function parseWebViewWebPreferences (preferences: string) {
return parseCommaSeparatedKeyValue(preferences, false).parsed;
}

const allowedWebPreferences = ['zoomFactor', 'nodeIntegration', 'enableRemoteModule', 'preload', 'javascript', 'contextIsolation', 'webviewTag'] as const;
type AllowedWebPreference = (typeof allowedWebPreferences)[number];

/**
* Parses a feature string that has the format used in window.open().
*
* `useSoonToBeDeprecatedBehaviorForBareKeys` — In the html spec, windowFeatures keys
* without values are interpreted as `true`. Previous versions of Electron did
* not respect this. In order to not break any applications, this will be
* flipped in the next major version.
*/
export function parseFeatures (
features: string,
useSoonToBeDeprecatedBehaviorForBareKeys: boolean = true
) {
const { parsed, bareKeys } = parseCommaSeparatedKeyValue(features, useSoonToBeDeprecatedBehaviorForBareKeys);

const webPreferences: { [K in AllowedWebPreference]?: any } = {};
allowedWebPreferences.forEach((key) => {
if (parsed[key] === undefined) return;
webPreferences[key] = parsed[key];
delete parsed[key];
});

if (parsed.left !== undefined) parsed.x = parsed.left;
if (parsed.top !== undefined) parsed.y = parsed.top;

return {
options: parsed as Omit<BrowserWindowConstructorOptions, 'webPreferences'> & { [key: string]: CoercedValue },
webPreferences,
additionalFeatures: bareKeys
};
}

0 comments on commit aca2e4f

Please sign in to comment.