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

wants to merge 1 commit into from
+33 −10

Conversation

BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented May 2, 2018 •
edited
Loading

This makes sure invalid error objects are not ignored when using
assert.throws and assert.rejects.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the assert Issues and PRs related to the assert subsystem. label May 2, 2018
@BridgeAR BridgeAR added the semver-major PRs that contain breaking changes and should be released in the next major version. label May 2, 2018
@BridgeAR BridgeAR requested a review from apapirovski May 2, 2018 22:03
@BridgeAR
Copy link
Member Author

BridgeAR commented May 2, 2018 •
edited
Loading

CI https://ci.nodejs.org/job/node-test-pull-request/14638/

@BridgeAR BridgeAR requested review from a team and removed request for apapirovski May 2, 2018 22:04
@BridgeAR BridgeAR force-pushed the assert-stricter-input-validation branch from 719f3af to 5eff6a5 Compare May 2, 2018 22:26
@BridgeAR
Copy link
Member Author

BridgeAR commented May 2, 2018 •
edited
Loading

Seems like this detected an issue in our tests where n-api passed through a regular expression from a different context. So the error was actually never validated so far.

@BridgeAR
Copy link
Member Author

BridgeAR commented May 2, 2018

It also detected this error: bdf5be9#r28831219

@BridgeAR
Copy link
Member Author

BridgeAR commented May 3, 2018 •
edited
Loading

This depends on #20487 to land first.
Update: it landed.

@Trott
Copy link
Member

Trott commented May 3, 2018

@nodejs/testing

@Trott
Copy link
Member

Trott commented May 4, 2018

(I imagine this should get a CITGM run? I mean, even if it only uncovers bugs in the packages, we probably want to get issues and PRs filed before releasing this.)

@BridgeAR BridgeAR force-pushed the assert-stricter-input-validation branch from 5eff6a5 to d1c2c46 Compare May 7, 2018 11:28
@BridgeAR
Copy link
Member Author

BridgeAR commented May 7, 2018 •
edited
Loading

I rebased, since this is now unblocked.

CI https://ci.nodejs.org/job/node-test-pull-request/14697/ ✔️
CITGM https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1401/ ✔️
CITGM master https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1402/ ✅

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 7, 2018
@BridgeAR BridgeAR requested a review from a team May 7, 2018 23:08
mcollina
mcollina approved these changes May 8, 2018
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@BridgeAR
assert: validate input stricter
f8a9023 
This makes sure invalid `error` objects are not ignored when using
`assert.throws` and `assert.rejects`.
@BridgeAR BridgeAR force-pushed the assert-stricter-input-validation branch from d1c2c46 to f8a9023 Compare May 10, 2018 12:17
@BridgeAR
Copy link
Member Author

Rebased due to conflicts.

CI before landing: https://ci.nodejs.org/job/node-test-pull-request/14784/

@BridgeAR
Copy link
Member Author

Landed in 21c3a40

@BridgeAR BridgeAR closed this May 10, 2018
BridgeAR added a commit to BridgeAR/node that referenced this pull request May 10, 2018
@BridgeAR
assert: validate input stricter
21c3a40 
This makes sure invalid `error` objects are not ignored when using
`assert.throws` and `assert.rejects`.

PR-URL: nodejs#20481
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@BridgeAR BridgeAR deleted the assert-stricter-input-validation branch January 20, 2020 11:32
ljharb
ljharb reviewed Jan 13, 2023
View reviewed changes
lib/assert.js
@@ -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});

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ljharb ljharb ljharb left review comments

@targos targos targos approved these changes

@jasnell jasnell jasnell approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
assert Issues and PRs related to the assert subsystem. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@BridgeAR @Trott @ljharb @mcollina @jasnell @targos @nodejs-github-bot