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: Add --chromium-pref flag #2912

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open
76 changes: 56 additions & 20 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@
"addons-linter": "6.17.0",
"bunyan": "1.8.15",
"camelcase": "8.0.0",
"chrome-launcher": "0.15.1",
"chrome-launcher": "1.1.0",
"debounce": "1.2.1",
"decamelize": "6.0.0",
"es6-error": "4.1.1",
Expand All @@ -82,6 +82,7 @@
"open": "9.1.0",
"parse-json": "7.1.1",
"promise-toolbox": "0.21.0",
"set-value": "4.1.0",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's not add the set-value module. It appears to be unmaintained (no responses from the project maintainer on several comments from the past year), and upon inspection I also see the lack of safeguards against pollution of built-in prototype members.

"source-map-support": "0.5.21",
"strip-bom": "5.0.0",
"strip-json-comments": "5.0.1",
Expand Down
6 changes: 6 additions & 0 deletions src/cmd/run.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ export default async function run(
firefoxApkComponent,
// Chromium CLI options.
chromiumBinary,
chromiumPref,
chromiumProfile,
},
{
Expand Down Expand Up @@ -86,6 +87,10 @@ export default async function run(
customPrefs['extensions.manifestV3.enabled'] = true;
}

// Create an alias for --chromium-pref since it has been transformed into an
// object containing one or more preferences.
const customChromiumPrefs = { ...chromiumPref };
Copy link
Member

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:

Suggested change
const customChromiumPrefs = { ...chromiumPref };
const customChromiumPrefs = chromiumPref;


const manifestData = await getValidatedManifest(sourceDir);

const profileDir = firefoxProfile || chromiumProfile;
Expand Down Expand Up @@ -193,6 +198,7 @@ export default async function run(
...commonRunnerParams,
chromiumBinary,
chromiumProfile,
customChromiumPrefs,
};

const chromiumRunner = await createExtensionRunner({
Expand Down
20 changes: 20 additions & 0 deletions src/extension-runners/chromium.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
launch as defaultChromiumLaunch,
} from 'chrome-launcher';
import WebSocket, { WebSocketServer } from 'ws';
import set from 'set-value';

import { createLogger } from '../util/logger.js';
import { TempDir } from '../util/temp-dir.js';
Expand All @@ -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.
*/
Expand Down Expand Up @@ -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', () => {
Expand Down Expand Up @@ -414,4 +420,18 @@ export class ChromiumExtensionRunner {
}
}
}

/**
* Returns a deep preferences object based on a set of flat preferences, like
* "extensions.ui.developer_mode".
*/
getPrefs() {
return Object.entries({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid the dependency of set-value and avoid overwriting built-in instance members, let's roll (y)our own. The expected input is a list of dotted paths + values, currently stored as a flat object (but originally a string of dotted.key=value pairs). The expected output is an object with nested objects.

Here is an example (not tested):

function mapToObject(map) {
  return Object.fromEntries(
    Array.from(
      map,
      ([k, v]) => [k, v instanceof Map ? mapToObject(v) : v]
    )
  );
}

// Logic for getPrefs(), given prefs = DEFAUL_PREFS etc.
const prefsMap = new Map();
for (let [key, value] of prefs) {
  let submap = prefsMap;
  const props = key.split(".");
  const lastProp = props.pop();
  for (let 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}`);
    }
  }
  // TODO: Consider log.warn() if submap.has(lastProp) before overwriting.
  submap.set(lastProp, value);
}

return mapToObject(prefsMap);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to base the function off this implementation. It's very compact and doesn't require using maps: https://youmightnotneed.com/lodash/#set

Will add some basic tests too.

Why do you want to avoid overwriting existing values? None of the default prefs I set in this PR are necessary for running the extension, they're just nice defaults that someone could turn off if they wanted. I also can't think of a use-case in the future where that would be useful.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to base the function off this implementation. It's very compact and doesn't require using maps: https://youmightnotneed.com/lodash/#set

This snippet you've selected and lodash (_.set) are both vulnerable to a Prototype pollution vulnerability. The set-value library that you used before fixed that in response to a report (GHSA-4g88-fppr-53pp). Despite the fix, it is still possible to overwrite properties on inherit built-in prototypes. For example:

var setValue = require('set-value');
setValue({}, 'hasOwnProperty.call', 1);
// Expected: true. Actual: TypeError: Object.prototype.hasOwnProperty.call is not a function
console.log(Object.prototype.hasOwnProperty.call(Math, "min"));

Why do you want to avoid overwriting existing values?

Are you referring to "TODO: Consider log.warn() if submap.has(lastProp) before overwriting."? The purpose of that is to let the user know that a built-in value has been overwritten. Not that big of a deal. I'm also fine with omitting it. Overwriting the value when there is a Map at that point may be an error though, consider the following:

download.download_path=/path/to/somewhere
download=1

It is not meaningful to set "download" to 1, and a warning could be nice. Not required though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see now. I misunderstood lots of things about the original comment lol. But now that I've implemented and tested your original suggestion, we're on the same page.

Only comment is about throwing the error. See these test cases:

https://github.com/aklinker1/web-ext/blob/eefc4adeea0b3930adcdf69b7b92a751e81eddc7/tests/unit/test-util/test.expand-prefs.js#L39-L61

We throw an error on the first, but not the second. I think we should make both behave the same way, either throw an error or allow the later value to override the earlier. Thoughts?

...DEFAULT_PREFS,
...(this.params.customChromiumPrefs || {}),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spread syntax with ...undefined and ...null works, you don't need to fall back to || {}.

Suggested change
...(this.params.customChromiumPrefs || {}),
...this.params.customChromiumPrefs,

}).reduce((prefs, [key, value]) => {
set(prefs, key, value);
return prefs;
}, {});
}
}
12 changes: 12 additions & 0 deletions src/program.js
Original file line number Diff line number Diff line change
Expand Up @@ -630,6 +630,18 @@ Example: $0 --help run.
demandOption: false,
type: 'string',
},
'chromium-pref': {
describe:
'Launch chromium with a custom preference ' +
'(example: --chromium-pref=browser.theme.follows_system_colors=false). ' +
'You can repeat this option to set more than one ' +
'preference.',
demandOption: false,
requiresArg: true,
type: 'array',
coerce: (arg) =>
arg != null ? coerceCLICustomPreference(arg) : undefined,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

coerceCLICustomPreference is Firefox-specific, and includes Firefox-specific logic such as preferences that cannot be overridden. You should use a custom one for Chromium.

},
'chromium-profile': {
describe: 'Path to a custom Chromium profile',
demandOption: false,
Expand Down
55 changes: 54 additions & 1 deletion tests/unit/test-extension-runners/test.chromium.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ describe('util/extension-runners/chromium', async () => {
it('uses the expected chrome flags', () => {
// Flags from chrome-launcher v0.14.0
const expectedFlags = [
'--disable-features=Translate',
'--disable-features=Translate,OptimizationHints,MediaRouter,DialMediaRouteProvider,CalculateNativeWinOcclusion,InterestFeedContentSuggestions,CertificateTransparencyComponentUpdater,AutofillServerCommunication',
'--disable-component-extensions-with-background-pages',
'--disable-background-networking',
'--disable-component-update',
Expand All @@ -66,6 +66,9 @@ describe('util/extension-runners/chromium', async () => {
'--password-store=basic',
'--use-mock-keychain',
'--force-fieldtrials=*BackgroundTracing/default/',
'--disable-hang-monitor',
'--disable-prompt-on-repost',
'--disable-domain-reliability',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes were required after upgrading chrome-launcher, similar to #2825 (comment)

];

assert.deepEqual(DEFAULT_CHROME_FLAGS, expectedFlags);
Expand Down Expand Up @@ -615,6 +618,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,
Copy link
Member

Choose a reason for hiding this comment

The 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;
Expand Down
21 changes: 21 additions & 0 deletions tests/unit/test.program.js
Original file line number Diff line number Diff line change
Expand Up @@ -627,6 +627,27 @@ describe('program.main', () => {
});
});

it('converts custom chromium preferences into an object', () => {
const fakeCommands = fake(commands, {
run: () => Promise.resolve(),
});
return execProgram(
[
'run',
'--chromium-pref',
'prop=true',
'--chromium-pref',
'prop2=value2',
],
{ commands: fakeCommands },
).then(() => {
const { chromiumPref } = fakeCommands.run.firstCall.args[0];
assert.isObject(chromiumPref);
assert.equal(chromiumPref.prop, true);
assert.equal(chromiumPref.prop2, 'value2');
});
});

it('passes shouldExitProgram option to commands', () => {
const fakeCommands = fake(commands, {
lint: () => Promise.resolve(),
Expand Down