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

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.

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

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

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.

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.

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?

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

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:

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.

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__":[]}');

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.

Suggesting a few changes to avoid prototype pollution.

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