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.

Sign up for GitHub

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

Jump to bottom

feat: Add --chromium-pref flag #2912

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
+242 −1
Open

feat: Add --chromium-pref flag #2912

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
4557df4
Upgrade to `chrome-launcher@1.1.0`
aklinker1 Oct 15, 2023
584b9e6
Add custom prefs to chromium runner
aklinker1 Oct 15, 2023
73ae575
Add CLI flag and docs
aklinker1 Oct 15, 2023
9df53ce
Correct param name
aklinker1 Oct 15, 2023
0f6a879
Fix formatting
aklinker1 Oct 15, 2023
caa193a
Re-run CI
aklinker1 Oct 15, 2023
c8dea96
revert changelog diff
aklinker1 Oct 31, 2023
b1779d9
Update comment
aklinker1 Jan 29, 2024
61e8a86
Merge branch 'master' into enable-developer-mode
aklinker1 May 30, 2024
ed9a645
Merge branch 'master' into enable-developer-mode
aklinker1 Jul 20, 2024
caad914
remove set-value
aklinker1 Jul 20, 2024
6123e12
cleanup
aklinker1 Jul 20, 2024
02476f5
Implement and test new expand-prefs function
aklinker1 Jul 20, 2024
eefc4ad
Add more tests
aklinker1 Jul 20, 2024
7281ae1
cleanup tests
aklinker1 Jul 20, 2024
035bf98
Implement coersion for chromium
aklinker1 Jul 20, 2024
3bac4fc
fix allowed name regex
aklinker1 Jul 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 56 additions & 20 deletions package-lock.json
View file
Open in desktop

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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@
"addons-linter": "6.28.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 @@ -81,6 +81,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
11 changes: 11 additions & 0 deletions src/cmd/run.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ export default async function run(
firefoxApkComponent,
// Chromium CLI options.
chromiumBinary,
chromiumPref,
chromiumProfile,
},
{
Expand Down Expand Up @@ -80,6 +81,15 @@ export default async function run(
// Create an alias for --pref since it has been transformed into an
// object containing one or more preferences.
const customPrefs = { ...pref };
if (firefoxPreview.includes('mv3')) {
log.info('Configuring Firefox preferences for Manifest V3');
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 };

const manifestData = await getValidatedManifest(sourceDir);

const profileDir = firefoxProfile || chromiumProfile;
Expand Down Expand Up @@ -187,6 +197,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
View file
Open in desktop
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 || {}),
}).reduce((prefs, [key, value]) => {
set(prefs, key, value);
return prefs;
}, {});
}
}
12 changes: 12 additions & 0 deletions src/program.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -616,6 +616,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
57 changes: 55 additions & 2 deletions tests/unit/test-extension-runners/test.chromium.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,9 @@ 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.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,
},
},
});

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
View file
Open in desktop
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