feat: Add --chromium-pref flag

[aklinker1]

Overview

This closes #2899.

Add a new parameter, --chromium-pref, which lets users configure some profile settings when running the web-ext run command. For default preferences, I set extensions.ui.developer_mode=true so you don't have to toggle the developer mode switch every time you run an extension on chromium.

web-ext/src/extension-runners/chromium.js

Lines 30 to 32 in 9d0512a

	const DEFAULT_PREFS = {
	'extensions.ui.developer_mode': true,
	};

Manual Testing

Here's the commands I ran to do manual testing:

# Default, dev mode should be enabled at chrome://extensions
node .\bin\web-ext.js run -s .\tests\fixtures\minimal-web-ext\ -t chromium

# Disable dev mode, change download dir (chrome://settings/downloads)
node .\bin\web-ext.js run -s .\tests\fixtures\minimal-web-ext\ -t chromium --chromium-pref extensions.ui.developer_mode=false --chromium-pref download.default_directory="C:\Users\Aaron\Downloads\test"

[aklinker1]

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

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.31%. Comparing base (17d1c4b) to head (b1779d9).

❗ Current head b1779d9 differs from pull request most recent head 61e8a86

Please upload reports for the commit 61e8a86 to get more accurate results.

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #2912       +/-   ##
===========================================
+ Coverage        0   99.31%   +99.31%     
===========================================
  Files           0       32       +32     
  Lines           0     1759     +1759     
===========================================
+ Hits            0     1747     +1747     
- Misses          0       12       +12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[aklinker1]

Rebased and fixed merge conflicts

[aklinker1]

Rebased and fixed conflicts

[VersoriumX]

No conflicts present with Branch

[VersoriumX]

add

[pftom]

@aklinker1 why not merge this pr? I still encounter the content script source map issues

[aklinker1]

Only maintainers can merge PRs, and this hasn't been approved by one.

[VersoriumX]

Thanks

[aklinker1]

@VersoriumX I appreciate your approvals, but since you're not a maintainer, you approving this more than once doesn't really help

[Rob--W]

Sorry for the delay here - I've put this on my list of tasks to review. We are supportive of the requested capability here.

[aklinker1]

@Rob--W no worries! Thanks for the update

[Rob--W]

This PR contains unrelated code (maybe partially due to a merge conflict).

Since you need to fix up the PR, could you split this PR in two parts:

• chore: Update chrome-launcher (this can be merged quickly)
• feat: Add --chromium-pref flag (can be reviewed and merged separately).

If it makes it easier, you could create a new branch branched off the main branch that updates chrome-launcher (and create a PR for that), and then rebase the current branch on top of that. Once the chrome-launcher PR is merged into main, this PR can be rebased on top of the main branch.

[Rob--W]

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.

[Rob--W]

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

[Rob--W]

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);

[aklinker1]

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.

[Rob--W]

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.

[aklinker1]

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?

[aklinker1]

@Rob--W Thanks for the review! I'll update this branch and make the suggested changes once chrome-launcher is updated. I created a PR: #3200

[Rob--W]

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

Suggested change

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

[Rob--W]

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;

[Rob--W]

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.

[Rob--W]

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 Object.getPrototypeOf(actual) and confirm that it is Object.prototype.

And let's also have the same test, but with the following:

const input = JSON.parse('{"__proto__":[]}');
const expected = JSON.parse('{"__proto__":[]}');

[Rob--W]

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.

[Rob--W]

Suggested change

	const customPrefs = {};
	const customPrefs = new Map();

Suggesting a few changes to avoid prototype pollution.

[Rob--W]

Suggested change

	customPrefs[`${key}`] = value;
	customPrefs.set(`${key}`, value);

[Rob--W]

Suggested change

	return customPrefs;
	return Object.fromEntries(customPrefs);

[Rob--W]

Is there any point in this check or can it be dropped?

\w means [A-Za-z0-9_], so what your code here does is rejecting anything other than alphanumeric characters and period.

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 : characters which are rejected by your current code), e.g. as seen at:

• example of file: https://source.chromium.org/chromium/chromium/src/+/main:chrome/test/data/diagnostics/user/Default/Preferences;l=58;drc=f0f6afc99205acab3f069d915cdad756a52d8809
• example in code comment: https://source.chromium.org/chromium/chromium/src/+/main:net/http/http_server_properties_manager.cc;l=245-256;drc=4787fce6c51383f5631643ac3d14cc512d656de6

[VersoriumX]

@VersoriumX squssh and merge