-
Notifications
You must be signed in to change notification settings - Fork 341
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: Add --chromium-pref
flag
#2912
base: master
Are you sure you want to change the base?
Changes from all commits
4557df4
584b9e6
73ae575
9df53ce
0f6a879
caa193a
c8dea96
b1779d9
61e8a86
ed9a645
caad914
6123e12
02476f5
eefc4ad
7281ae1
035bf98
3bac4fc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -17,6 +17,7 @@ import { createLogger } from '../util/logger.js'; | |||||
import { TempDir } from '../util/temp-dir.js'; | ||||||
import isDirectory from '../util/is-directory.js'; | ||||||
import fileExists from '../util/file-exists.js'; | ||||||
import expandPrefs from '../util/expand-prefs.js'; | ||||||
|
||||||
const log = createLogger(import.meta.url); | ||||||
|
||||||
|
@@ -26,6 +27,10 @@ export const DEFAULT_CHROME_FLAGS = ChromeLauncher.defaultFlags().filter( | |||||
(flag) => !EXCLUDED_CHROME_FLAGS.includes(flag), | ||||||
); | ||||||
|
||||||
const DEFAULT_PREFS = { | ||||||
'extensions.ui.developer_mode': true, | ||||||
}; | ||||||
|
||||||
/** | ||||||
* Implements an IExtensionRunner which manages a Chromium instance. | ||||||
*/ | ||||||
|
@@ -210,6 +215,7 @@ export class ChromiumExtensionRunner { | |||||
userDataDir, | ||||||
// Ignore default flags to keep the extension enabled. | ||||||
ignoreDefaultFlags: true, | ||||||
prefs: this.getPrefs(), | ||||||
}); | ||||||
|
||||||
this.chromiumInstance.process.once('close', () => { | ||||||
|
@@ -414,4 +420,15 @@ export class ChromiumExtensionRunner { | |||||
} | ||||||
} | ||||||
} | ||||||
|
||||||
/** | ||||||
* Returns a deep preferences object based on a set of flat preferences, like | ||||||
* "extensions.ui.developer_mode". | ||||||
*/ | ||||||
getPrefs() { | ||||||
return expandPrefs({ | ||||||
...DEFAULT_PREFS, | ||||||
...(this.params.customChromiumPrefs || {}), | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Spread syntax with
Suggested change
|
||||||
}); | ||||||
} | ||||||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,33 @@ | ||||||
import { UsageError } from '../errors.js'; | ||||||
|
||||||
export function coerceCLICustomChromiumPreference(cliPrefs) { | ||||||
const customPrefs = {}; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Suggesting a few changes to avoid prototype pollution. |
||||||
|
||||||
for (const pref of cliPrefs) { | ||||||
const prefsAry = pref.split('='); | ||||||
|
||||||
if (prefsAry.length < 2) { | ||||||
throw new UsageError( | ||||||
`Incomplete custom preference: "${pref}". ` + | ||||||
'Syntax expected: "prefname=prefvalue".', | ||||||
); | ||||||
} | ||||||
|
||||||
const key = prefsAry[0]; | ||||||
let value = prefsAry.slice(1).join('='); | ||||||
|
||||||
if (/[^\w_.]/.test(key)) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there any point in this check or can it be dropped?
It is stricter than the actual values permitted by Chromium's Preferences keys, which is quite permissive actually. For example it can contain URLs (including
|
||||||
throw new UsageError(`Invalid custom preference name: ${key}`); | ||||||
} | ||||||
|
||||||
if (value === `${parseInt(value)}`) { | ||||||
value = parseInt(value, 10); | ||||||
} else if (value === 'true' || value === 'false') { | ||||||
value = value === 'true'; | ||||||
} | ||||||
|
||||||
customPrefs[`${key}`] = value; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
} | ||||||
|
||||||
return customPrefs; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
/** | ||
* Given an object where the keys are the flattened path to a | ||
* preference, and the value is the value to set at that path, return | ||
* an object where the paths are fully expanded. | ||
*/ | ||
export default function expandPrefs(prefs) { | ||
const prefsMap = new Map(); | ||
for (const [key, value] of Object.entries(prefs)) { | ||
let submap = prefsMap; | ||
const props = key.split('.'); | ||
const lastProp = props.pop(); | ||
for (const prop of props) { | ||
if (!submap.has(prop)) { | ||
submap.set(prop, new Map()); | ||
} | ||
submap = submap.get(prop); | ||
if (!(submap instanceof Map)) { | ||
throw new Error( | ||
`Cannot set ${key} because a value already exists at ${prop}`, | ||
); | ||
} | ||
} | ||
submap.set(lastProp, value); | ||
} | ||
return mapToObject(prefsMap); | ||
} | ||
|
||
function mapToObject(map) { | ||
return Object.fromEntries( | ||
Array.from(map, ([k, v]) => [k, v instanceof Map ? mapToObject(v) : v]), | ||
); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -47,7 +47,7 @@ function prepareExtensionRunnerParams({ params } = {}) { | |
|
||
describe('util/extension-runners/chromium', async () => { | ||
it('uses the expected chrome flags', () => { | ||
// Flags from chrome-launcher v0.14.0 | ||
// Flags from chrome-launcher v1.1.2 | ||
const expectedFlags = [ | ||
'--disable-features=Translate,OptimizationHints,MediaRouter,DialMediaRouteProvider,CalculateNativeWinOcclusion,InterestFeedContentSuggestions,CertificateTransparencyComponentUpdater,AutofillServerCommunication,PrivacySandboxSettings4', | ||
'--disable-component-extensions-with-background-pages', | ||
|
@@ -619,6 +619,56 @@ describe('util/extension-runners/chromium', async () => { | |
}), | ||
); | ||
|
||
it('does pass default prefs to chrome', async () => { | ||
const { params } = prepareExtensionRunnerParams(); | ||
|
||
const runnerInstance = new ChromiumExtensionRunner(params); | ||
await runnerInstance.run(); | ||
|
||
sinon.assert.calledOnce(params.chromiumLaunch); | ||
sinon.assert.calledWithMatch(params.chromiumLaunch, { | ||
prefs: { | ||
extensions: { | ||
ui: { | ||
developer_mode: true, | ||
}, | ||
}, | ||
}, | ||
}); | ||
|
||
await runnerInstance.exit(); | ||
}); | ||
|
||
it('does pass custom prefs to chrome', async () => { | ||
const { params } = prepareExtensionRunnerParams({ | ||
params: { | ||
customChromiumPrefs: { | ||
'download.default_directory': '/some/directory', | ||
'extensions.ui.developer_mode': false, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's split the test in two: that a custom pref can be passed, and that a default pref can be overridden. We want to make sure that custom prefs are merged with the default prefs, and not changed. The current implementation is correct, but the test as constructed would still pass if the implementation failed to merge, which is a sign of an incomplete test. |
||
}, | ||
}, | ||
}); | ||
|
||
const runnerInstance = new ChromiumExtensionRunner(params); | ||
await runnerInstance.run(); | ||
|
||
sinon.assert.calledOnce(params.chromiumLaunch); | ||
sinon.assert.calledWithMatch(params.chromiumLaunch, { | ||
prefs: { | ||
download: { | ||
default_directory: '/some/directory', | ||
}, | ||
extensions: { | ||
ui: { | ||
developer_mode: false, | ||
}, | ||
}, | ||
}, | ||
}); | ||
|
||
await runnerInstance.exit(); | ||
}); | ||
|
||
describe('reloadAllExtensions', () => { | ||
let runnerInstance; | ||
let wsClient; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,68 @@ | ||
import { assert, expect } from 'chai'; | ||
import { describe, it } from 'mocha'; | ||
|
||
import expandPrefs from '../../../src/util/expand-prefs.js'; | ||
|
||
describe('utils/expand-prefs', () => { | ||
it('expands dot-deliminated preferences into a deep object', () => { | ||
const input = { | ||
a: 'a', | ||
'b.c': 'c', | ||
'd.e.f': 'f', | ||
}; | ||
const expected = { | ||
a: 'a', | ||
b: { | ||
c: 'c', | ||
}, | ||
d: { | ||
e: { | ||
f: 'f', | ||
}, | ||
}, | ||
}; | ||
const actual = expandPrefs(input); | ||
|
||
assert.deepEqual(actual, expected); | ||
}); | ||
|
||
it("doesn't pollute the object prototype", () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The test case here is a specific one that failed the original library that you proposed. Let's also prepend a test for a more obvious test case: const input = {
'__proto__.x': 1
};
// Using JSON.parse because an object literal with __proto__
// would be mistaken for an object with a different prototype.
const expected = JSON.parse('{"__proto__":{"x":1}}'); Besides the obvious comparison like above, you should also add a test And let's also have the same test, but with the following: const input = JSON.parse('{"__proto__":[]}');
const expected = JSON.parse('{"__proto__":[]}'); |
||
const call = 'overriden'; | ||
const input = { | ||
'hasOwnProperty.call': call, | ||
}; | ||
const expected = { | ||
hasOwnProperty: { | ||
call, | ||
}, | ||
}; | ||
const actual = expandPrefs(input); | ||
|
||
assert.notEqual(Object.prototype.hasOwnProperty.call, call); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of notEqual, save the original value before modifying the test, then do a strict comparison. A unit test that tests for equality is stronger than one that tests for inequality. |
||
assert.deepEqual(actual, expected); | ||
}); | ||
|
||
it('throws an error when setting the child property of an already set parent', () => { | ||
const input = { | ||
a: 'a', | ||
'a.b': 'b', | ||
}; | ||
|
||
expect(() => expandPrefs(input)).to.throw( | ||
'Cannot set a.b because a value already exists at a', | ||
); | ||
}); | ||
|
||
it('allows overriding a parent even if a child has already been set', () => { | ||
const input = { | ||
'a.b': 'b', | ||
a: 'a', | ||
}; | ||
const expected = { | ||
a: 'a', | ||
}; | ||
const actual = expandPrefs(input); | ||
|
||
assert.deepEqual(actual, expected); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that you basically copied the previous few lines. The logic there did not completely make sense, I've sent a PR to fix that up: #3218
Let's simplify to the following: