fix(assertString): throw typeError on objects with constructor and name property#2692
fix(assertString): throw typeError on objects with constructor and name property#2692newtondividedbysqm wants to merge 2 commits intovalidatorjs:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Updates assertString to correctly reject non-string objects that spoof constructor.name as "String", and expands tests to cover the spoofing cases (and boxed strings).
Changes:
- Tighten
assertStringtype checking to allow primitives/boxed strings while rejecting constructor-name spoofed objects. - Add unit tests for spoofed
{ constructor: { name: "String" } }inputs. - Add a unit test ensuring boxed strings (
new String(...)) are accepted.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/lib/util/assertString.js |
Reworks the validation logic to avoid constructor-name spoofing. |
test/util.test.js |
Adds regression tests for spoofed objects and boxed strings. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export default function assertString(input) { | ||
| if (input === undefined || input === null) throw new TypeError(`Expected a string but received a ${input}`); | ||
| if (input.constructor.name !== 'String') throw new TypeError(`Expected a string but received a ${input.constructor.name}`); | ||
| if (typeof input !== 'string' && !(input instanceof String)) throw new TypeError(`Expected a string but received a ${input?.constructor?.name ?? typeof input}`) |
There was a problem hiding this comment.
instanceof String will return false for boxed strings created in a different JS realm (e.g., iframe/window), which can cause valid String objects to be rejected. To avoid that cross-realm pitfall while still blocking { constructor: { name: 'String' } } spoofing, consider checking Object.prototype.toString.call(input) === '[object String]' (or reusing the existing typeOf util) alongside the primitive typeof input === 'string' check.
| if (typeof input !== 'string' && !(input instanceof String)) throw new TypeError(`Expected a string but received a ${input?.constructor?.name ?? typeof input}`) | |
| if (typeof input !== 'string' && Object.prototype.toString.call(input) !== '[object String]') throw new TypeError(`Expected a string but received a ${input?.constructor?.name ?? typeof input}`) |
| export default function assertString(input) { | ||
| if (input === undefined || input === null) throw new TypeError(`Expected a string but received a ${input}`); | ||
| if (input.constructor.name !== 'String') throw new TypeError(`Expected a string but received a ${input.constructor.name}`); | ||
| if (typeof input !== 'string' && !(input instanceof String)) throw new TypeError(`Expected a string but received a ${input?.constructor?.name ?? typeof input}`) |
There was a problem hiding this comment.
Missing semicolon at the end of this throw statement will fail the repo's ESLint (airbnb-base defaults to semi: always). Add a trailing semicolon (and keep statement formatting consistent with the previous line).
| if (typeof input !== 'string' && !(input instanceof String)) throw new TypeError(`Expected a string but received a ${input?.constructor?.name ?? typeof input}`) | |
| if (typeof input !== 'string' && !(input instanceof String)) throw new TypeError(`Expected a string but received a ${input?.constructor?.name ?? typeof input}`); |
| assert.throws(() => { assertString({constructor: {name: "string"}}); }, TypeError); | ||
| }); | ||
|
|
||
| it('Should throw an error if argument provided is an Object pretending to be a \'String\'', () => { | ||
| assert.throws(() => { assertString({constructor: {name: "String"}}); }, TypeError); |
There was a problem hiding this comment.
These new test inputs use double quotes ("string", "String") while the surrounding tests consistently use single quotes, and the repo ESLint config extends airbnb-base (default quotes: single). Switching these to single quotes will prevent lint failures.
| assert.throws(() => { assertString({constructor: {name: "string"}}); }, TypeError); | |
| }); | |
| it('Should throw an error if argument provided is an Object pretending to be a \'String\'', () => { | |
| assert.throws(() => { assertString({constructor: {name: "String"}}); }, TypeError); | |
| assert.throws(() => { assertString({constructor: {name: 'string'}}); }, TypeError); | |
| }); | |
| it('Should throw an error if argument provided is an Object pretending to be a \'String\'', () => { | |
| assert.throws(() => { assertString({constructor: {name: 'String'}}); }, TypeError); |
| assert.doesNotThrow(() => { assertString(''); }); | ||
| }); | ||
|
|
||
There was a problem hiding this comment.
This blank line contains trailing whitespace, which will fail no-trailing-spaces under airbnb-base ESLint. Remove the spaces so the line is truly empty.
|
Please fix the tests and look through the Copilot comments before we review this |
assertString is currently validating objects with constructor property like
{constructor: {name: "String"}}