makeImmutable() only makes the children immutable, not the target object #865
Description
makeImmutable only makes the children immutable, not the given object
it('Correctly unable to change an immutable configuration', function() {
config = requireUncached(__dirname + '/../lib/config');
config.util.makeImmutable(config, 'TestModule');
assert.throws(() => config.TestModule.newField = "setToThis",
/Cannot assign to read only property/);
});
AssertionError [ERR_ASSERTION] [ERR_ASSERTION]: Missing expected exception.
at TestContext.<anonymous> (/Users/jasonmarshall/Projects/cobbler/node-config/test/2-config-test.js:282:14)
at Test.runInAsyncScope (node:async_hooks:214:14)
at Test.run (node:internal/test_runner/test:1047:25)
at Suite.processPendingSubtests (node:internal/test_runner/test:744:7)
{
generatedMessage: false,
code: 'ERR_ASSERTION',
actual: undefined,
expected: /Cannot assign to read only property/,
operator: 'throws'
}
This was partially fixed for #505 but only for the children and not the object itself. I think the recursion is done wrong here.
There's a second problem that shows up in withModuleDefaults that is a regression not caught by the tests. The PR to address this issue includes tests with more realistic misuse scenarios that should catch this.
Another consequence is that any objects in arrays also don't have their top-level fields guarded, only children:
object = {
item1: 23,
subObject: {
item2: "hello",
entries: [ { a: "b"}, { b: "c", c: { d: "e" } }],
}
};
Util.makeImmutable(object);
object.subObject.entries[0].a = "c"; // works, but should throw error
Additionally, CodeRabbit is making the following suggestion, which I don't think I can disagree with:
Array branch in Util.makeImmutable must set writable: false, configurable: false.
The Object.defineProperty call at lib/util.js lines 159-161 in the Array branch only passes { value: Object.freeze(value) } without writable: false, configurable: false. This creates an inconsistency: the RawConfig branch (lines 146-150) and the regular object branch (lines 196-200) both set these flags, but the Array branch does not.
This means array properties can be reassigned even after makeImmutable: