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

assert: validate input stricter #20481

Closed
wants to merge 1 commit into from
Closed

assert: validate input stricter #20481

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
16 changes: 10 additions & 6 deletions lib/assert.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ const {
const { codes: {
ERR_AMBIGUOUS_ARGUMENT,
ERR_INVALID_ARG_TYPE,
ERR_INVALID_ARG_VALUE,
ERR_INVALID_RETURN_VALUE
} } = require('internal/errors');
const { AssertionError, errorCache } = require('internal/assert');
Expand Down Expand Up @@ -414,12 +415,6 @@ function expectedException(actual, expected, msg) {
);
}

// TODO: Disallow primitives as error argument.
// This is here to prevent a breaking change.
if (typeof expected !== 'object') {
return true;
}

// Handle primitives properly.
if (typeof actual !== 'object' || actual === null) {
const err = new AssertionError({
Expand All @@ -438,6 +433,9 @@ function expectedException(actual, expected, msg) {
// as well.
if (expected instanceof Error) {
keys.push('name', 'message');
} else if (keys.length === 0) {
throw new ERR_INVALID_ARG_VALUE('error',
expected, 'may not be an empty object');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there any background on why this constraint was added?

It seems perfectly valid to me to do var sentinel = {}; assert.throws(() => { throw sentinel; }, sentinel, 'throws the sentinel');, for example.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before this change, nothing would have been validated as the signature was about validating the properties of the object and there are no properties to validate.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ahh ok, so there's no way to validate that a specific thing with identity is thrown?

Copy link
Member Author

@BridgeAR BridgeAR Jan 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's possible with the function validation: var sentinel = {}; assert.throws(() => { throw sentinel; }, (error) => { assert(error === sentinel, 'Sentinel should be thrown'); return true});

}
for (const key of keys) {
if (typeof actual[key] === 'string' &&
Expand Down Expand Up @@ -527,6 +525,12 @@ function expectsError(stackStartFn, actual, error, message) {
}
message = error;
error = undefined;
} else if (error != null &&
typeof error !== 'object' &&
typeof error !== 'function') {
throw new ERR_INVALID_ARG_TYPE('error',
['Object', 'Error', 'Function', 'RegExp'],
error);
}

if (actual === NO_EXCEPTION_SENTINEL) {
Expand Down
27 changes: 23 additions & 4 deletions test/parallel/test-assert.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -772,6 +772,21 @@ common.expectsError(
}
);

[
1,
false,
Symbol()
].forEach((input) => {
assert.throws(
() => assert.throws(() => {}, input),
{
code: 'ERR_INVALID_ARG_TYPE',
message: 'The "error" argument must be one of type Object, Error, ' +
`Function, or RegExp. Received type ${typeof input}`
}
);
});

{
const errFn = () => {
const err = new TypeError('Wrong value');
Expand Down Expand Up @@ -871,6 +886,14 @@ common.expectsError(
);
}

assert.throws(
() => assert.throws(() => { throw new Error(); }, {}),
{
message: "The argument 'error' may not be an empty object. Received {}",
code: 'ERR_INVALID_ARG_VALUE'
}
);

assert.throws(
() => a.throws(
// eslint-disable-next-line no-throw-literal
Expand Down Expand Up @@ -981,7 +1004,3 @@ assert.throws(
}
);
}

// TODO: This case is only there to make sure there is no breaking change.
// eslint-disable-next-line no-restricted-syntax, no-throw-literal
assert.throws(() => { throw 4; }, 4);