Skip to content

Commit

Permalink
vm: harden module type checks
Browse files Browse the repository at this point in the history 
Check if the value returned from user linker function is a null-ish
value.

`validateInternalField` should be preferred when checking `this`
argument to guard against null-ish `this`.

Co-authored-by: Mike Ralphson <[email protected]>
PR-URL: #52162
Backport-PR-URL: #53109
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
  • Loading branch information
@legendecas @MikeRalphson @targos
2 people authored and targos committed Oct 2, 2024
1 parent 1a038d8 commit 47be236
Show file tree
Hide file tree
Showing 5 changed files with 58 additions and 57 deletions.
66 changes: 24 additions & 42 deletions lib/internal/vm/module.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ const {
ArrayPrototypeSome,
ObjectDefineProperty,
ObjectGetPrototypeOf,
ObjectPrototypeHasOwnProperty,
ObjectSetPrototypeOf,
ReflectApply,
SafePromiseAllReturnVoid,
Expand Down Expand Up @@ -43,6 +44,7 @@ const {
validateObject,
validateUint32,
validateString,
validateInternalField,
} = require('internal/validators');

const binding = internalBinding('module_wrap');
Expand Down Expand Up @@ -75,6 +77,13 @@ const kLink = Symbol('kLink');

const { isContext } = require('internal/vm');

function isModule(object) {
if (typeof object !== 'object' || object === null || !ObjectPrototypeHasOwnProperty(object, kWrap)) {
return false;
}
return true;
}

class Module {
constructor(options) {
emitExperimentalWarning('VM Modules');
Expand Down Expand Up @@ -147,50 +156,38 @@ class Module {
}

get identifier() {
if (this[kWrap] === undefined) {
throw new ERR_VM_MODULE_NOT_MODULE();
}
validateInternalField(this, kWrap, 'Module');
return this[kWrap].url;
}

get context() {
if (this[kWrap] === undefined) {
throw new ERR_VM_MODULE_NOT_MODULE();
}
validateInternalField(this, kWrap, 'Module');
return this[kContext];
}

get namespace() {
if (this[kWrap] === undefined) {
throw new ERR_VM_MODULE_NOT_MODULE();
}
validateInternalField(this, kWrap, 'Module');
if (this[kWrap].getStatus() < kInstantiated) {
throw new ERR_VM_MODULE_STATUS('must not be unlinked or linking');
}
return this[kWrap].getNamespace();
}

get status() {
if (this[kWrap] === undefined) {
throw new ERR_VM_MODULE_NOT_MODULE();
}
validateInternalField(this, kWrap, 'Module');
return STATUS_MAP[this[kWrap].getStatus()];
}

get error() {
if (this[kWrap] === undefined) {
throw new ERR_VM_MODULE_NOT_MODULE();
}
validateInternalField(this, kWrap, 'Module');
if (this[kWrap].getStatus() !== kErrored) {
throw new ERR_VM_MODULE_STATUS('must be errored');
}
return this[kWrap].getError();
}

async link(linker) {
if (this[kWrap] === undefined) {
throw new ERR_VM_MODULE_NOT_MODULE();
}
validateInternalField(this, kWrap, 'Module');
validateFunction(linker, 'linker');
if (this.status === 'linked') {
throw new ERR_VM_MODULE_ALREADY_LINKED();
Expand All @@ -203,10 +200,7 @@ class Module {
}

async evaluate(options = kEmptyObject) {
if (this[kWrap] === undefined) {
throw new ERR_VM_MODULE_NOT_MODULE();
}

validateInternalField(this, kWrap, 'Module');
validateObject(options, 'options');

let timeout = options.timeout;
Expand All @@ -229,9 +223,7 @@ class Module {
}

[customInspectSymbol](depth, options) {
if (this[kWrap] === undefined) {
throw new ERR_VM_MODULE_NOT_MODULE();
}
validateInternalField(this, kWrap, 'Module');
if (typeof depth === 'number' && depth < 0)
return this;

Expand Down Expand Up @@ -306,7 +298,7 @@ class SourceTextModule extends Module {

const promises = this[kWrap].link(async (identifier, attributes) => {
const module = await linker(identifier, this, { attributes, assert: attributes });
if (module[kWrap] === undefined) {
if (!isModule(module)) {
throw new ERR_VM_MODULE_NOT_MODULE();
}
if (module.context !== this.context) {
Expand Down Expand Up @@ -337,19 +329,13 @@ class SourceTextModule extends Module {
}

get dependencySpecifiers() {
if (this[kWrap] === undefined) {
throw new ERR_VM_MODULE_NOT_MODULE();
}
if (this[kDependencySpecifiers] === undefined) {
this[kDependencySpecifiers] = this[kWrap].getStaticDependencySpecifiers();
}
validateInternalField(this, kDependencySpecifiers, 'SourceTextModule');
this[kDependencySpecifiers] ??= this[kWrap].getStaticDependencySpecifiers();
return this[kDependencySpecifiers];
}

get status() {
if (this[kWrap] === undefined) {
throw new ERR_VM_MODULE_NOT_MODULE();
}
validateInternalField(this, kDependencySpecifiers, 'SourceTextModule');
if (this.#error !== kNoError) {
return 'errored';
}
Expand All @@ -360,9 +346,7 @@ class SourceTextModule extends Module {
}

get error() {
if (this[kWrap] === undefined) {
throw new ERR_VM_MODULE_NOT_MODULE();
}
validateInternalField(this, kDependencySpecifiers, 'SourceTextModule');
if (this.#error !== kNoError) {
return this.#error;
}
Expand Down Expand Up @@ -415,9 +399,7 @@ class SyntheticModule extends Module {
}

setExport(name, value) {
if (this[kWrap] === undefined) {
throw new ERR_VM_MODULE_NOT_MODULE();
}
validateInternalField(this, kWrap, 'SyntheticModule');
validateString(name, 'name');
if (this[kWrap].getStatus() < kInstantiated) {
throw new ERR_VM_MODULE_STATUS('must be linked');
Expand All @@ -432,7 +414,7 @@ function importModuleDynamicallyWrap(importModuleDynamically) {
if (isModuleNamespaceObject(m)) {
return m;
}
if (!m || m[kWrap] === undefined) {
if (!isModule(m)) {
throw new ERR_VM_MODULE_NOT_MODULE();
}
if (m.status === 'errored') {
Expand Down
16 changes: 9 additions & 7 deletions test/parallel/test-vm-module-basic.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,13 +84,15 @@ const util = require('util');

assert.strictEqual(util.inspect(m, { depth: -1 }), '[SourceTextModule]');

assert.throws(
() => m[util.inspect.custom].call({ __proto__: null }),
{
code: 'ERR_VM_MODULE_NOT_MODULE',
message: 'Provided module is not an instance of Module'
},
);
for (const value of [null, { __proto__: null }, SourceTextModule.prototype]) {
assert.throws(
() => m[util.inspect.custom].call(value),
{
code: 'ERR_INVALID_ARG_TYPE',
message: /The "this" argument must be an instance of Module/,
},
);
}
}

{
Expand Down
8 changes: 4 additions & 4 deletions test/parallel/test-vm-module-errors.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -216,8 +216,8 @@ async function checkInvalidOptionForEvaluate() {
await assert.rejects(async () => {
await Module.prototype[method]();
}, {
code: 'ERR_VM_MODULE_NOT_MODULE',
message: /Provided module is not an instance of Module/
code: 'ERR_INVALID_ARG_TYPE',
message: /The "this" argument must be an instance of Module/
});
});
}
Expand All @@ -241,8 +241,8 @@ function checkInvalidCachedData() {

function checkGettersErrors() {
const expectedError = {
code: 'ERR_VM_MODULE_NOT_MODULE',
message: /Provided module is not an instance of Module/
code: 'ERR_INVALID_ARG_TYPE',
message: /The "this" argument must be an instance of (?:Module|SourceTextModule)/,
};
const getters = ['identifier', 'context', 'namespace', 'status', 'error'];
getters.forEach((getter) => {
Expand Down
17 changes: 17 additions & 0 deletions test/parallel/test-vm-module-link.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,22 @@ async function simple() {
delete globalThis.fiveResult;
}

async function invalidLinkValue() {
const invalidValues = [
undefined,
null,
{},
SourceTextModule.prototype,
];

for (const value of invalidValues) {
const module = new SourceTextModule('import "foo"');
await assert.rejects(module.link(() => value), {
code: 'ERR_VM_MODULE_NOT_MODULE',
});
}
}

async function depth() {
const foo = new SourceTextModule('export default 5');
await foo.link(common.mustNotCall());
Expand Down Expand Up @@ -143,6 +159,7 @@ const finished = common.mustCall();

(async function main() {
await simple();
await invalidLinkValue();
await depth();
await circular();
await circular2();
Expand Down
8 changes: 4 additions & 4 deletions test/parallel/test-vm-module-synthetic.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,12 +66,12 @@ const assert = require('assert');
});
}

{
for (const value of [null, {}, SyntheticModule.prototype]) {
assert.throws(() => {
SyntheticModule.prototype.setExport.call({}, 'foo');
SyntheticModule.prototype.setExport.call(value, 'foo');
}, {
code: 'ERR_VM_MODULE_NOT_MODULE',
message: /Provided module is not an instance of Module/
code: 'ERR_INVALID_ARG_TYPE',
message: /The "this" argument must be an instance of SyntheticModule/
});
}

Expand Down

0 comments on commit 47be236

Please sign in to comment.