errors: validate constructor name - #65607
Open
christianaurichzm wants to merge 1 commit into
Open
Conversation
determineSpecificType() checks for a usable constructor name with `'name' in value.constructor`. That accepts an empty name, and the `in` operator requires its right-hand side to be an object. Anonymous classes own a `name` that is the empty string, so they produce messages ending in a dangling "Received an instance of ". A truthy primitive `constructor` reaches the `in` operator and throws while the message is being built, so ERR_INVALID_ARG_TYPE is replaced by a TypeError carrying no `code`. The latter is reachable from untrusted input, since `constructor` is an ordinary JSON key. The check changed in nodejs#49696, while this function was rewritten as a switch. That pull request updated test/common's invalidArgTypeHelper to match its deliberate change to the `function` branch, but left the helper's `object` branch on the original truthy check, so the two have disagreed since. Read `constructor` and its `name` once and use the name only when it is a non-empty string. Requiring a string also stops meaningless names from being interpolated: 42 currently yields "an instance of 42", and a symbol name throws outright. The current check reads `constructor` three times, which an accessor can observe. Move the helper to the same check so the two cannot drift apart again. Signed-off-by: Christian Aurich <christian.aurichzm@gmail.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #65607 +/- ##
==========================================
- Coverage 90.07% 90.05% -0.03%
==========================================
Files 751 751
Lines 254875 254921 +46
Branches 48108 48124 +16
==========================================
- Hits 229579 229566 -13
- Misses 16466 16530 +64
+ Partials 8830 8825 -5
🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
determineSpecificType()decides whether an error message can name the constructor of a value. The check isvalue.constructor && 'name' in value.constructor: theinoperator requires an object on its right-hand side, and an empty name passes it.On v22.23.1, and the same on
main:The first one is the one that matters.
constructoris an ordinary JSON key, so untrusted input can make the construction of the message throw: theERR_INVALID_ARG_TYPEnever reaches the caller, it is replaced by a plainTypeErrorwith nocode, anderr.code === 'ERR_INVALID_ARG_TYPE'at the call site stops matching. The second is cosmetic: an anonymous class owns anamethat is the empty string, so the message ends in a danglingReceived an instance of.Representative cases:
valuenew (class {})()an instance of{}JSON.parse('{"constructor": 5}')TypeError[Object]{ constructor: { name: Symbol('x') } }TypeError[Object]{ constructor: { name: 42 } }an instance of 42[Object]This reads
constructorand itsnameonce, and uses the name only when it is a non-empty string. The single read is also a change in behaviour: the current check readsconstructorthree times, which an accessor can observe. Tests pinboth reads.
test/common'sinvalidArgTypeHelpermoves to the same check. #49696 updated it to match its deliberate change to thefunctionbranch, but left theobjectbranch on the original truthy check, so the two have disagreed since.Scope
Requiring a string intentionally also changes non-string constructor names.
{ constructor: { name: 42 } }produces a well-formedan instance of 42today and now falls back to[Object], the same fallback already used when no name is available.The
functionbranch is unchanged. Its handling of anonymous function names was changed deliberately in #49696.Testing
test/parallel/test-error-value-type-detection.mjs: extended with the cases in the table, assertions thatconstructorand itsnameare each read once, and a check that building the error keeps itscodebenchmark/error/determine-specific-type.js, baseline versus patch on Linux x64, three comparisons, the last with--runs 60 --analyze: the configurations flagged at the 5% threshold were not consistent between runs.compare.jsreports 1.70 expected false positives at that threshold across its 34 configurationsmake -j16 testandmake lint: passRefs: #49696