-
-
Notifications
You must be signed in to change notification settings - Fork 369
fix(object): use own-property check so prototype keys aren't matched as entries (#1523) #1536
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -108,6 +108,24 @@ describe('strictObject', () => { | |||||
| expectSchemaIssue(schema, baseIssue, [Symbol(), Symbol('foo')]); | ||||||
| }); | ||||||
|
|
||||||
| test('for input with a key that collides with Object.prototype', () => { | ||||||
| // Regression test for #1523. The pre-fix `key in this.entries` check | ||||||
| // matched inherited Object.prototype members, so a strict schema | ||||||
| // silently accepted `__proto__`, `constructor`, `toString`, and | ||||||
| // friends as defined entries. After the fix, those keys are | ||||||
| // reported as unknown and the schema rejects the input. | ||||||
| const strict = strictObject({ name: string() }, 'message'); | ||||||
| expectSchemaIssue( | ||||||
| strict, | ||||||
| { ...baseIssue, expected: 'never' }, | ||||||
| [ | ||||||
| { name: 'foo', toString: 'bar' }, | ||||||
| { name: 'foo', constructor: 1 }, | ||||||
| { name: 'foo', __proto__: 'x' }, | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '\n== Test file outline ==\n'
ast-grep outline library/src/schemas/strictObject/strictObject.test.ts --view expanded || true
printf '\n== Relevant lines ==\n'
nl -ba library/src/schemas/strictObject/strictObject.test.ts | sed -n '90,150p'
printf '\n== strictObject source search ==\n'
fd -a 'strictObject.ts' library/src || true
rg -n "strictObject|__proto__|proto" library/src/schemas/strictObject library/src -g '!**/*.map' || true
printf '\n== JS semantics probe ==\n'
node - <<'JS'
const samples = [
{ expr: "({ name: 'foo', __proto__: 'x' })" },
{ expr: "({ name: 'foo', ['__proto__']: 'x' })" },
{ expr: "Object.defineProperty({ name: 'foo' }, '__proto__', { value: 'x', enumerable: true })" },
{ expr: "({ __proto__: null, a: 1 })" },
];
for (const { expr } of samples) {
const obj = eval(expr);
console.log(expr);
console.log(' own keys:', Object.keys(obj));
console.log(' hasOwn __proto__:', Object.prototype.hasOwnProperty.call(obj, '__proto__'));
console.log(' proto is null:', Object.getPrototypeOf(obj) === null);
console.log(' value of obj.__proto__:', obj.__proto__);
}
JSRepository: open-circle/valibot Length of output: 333 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '\n== strictObject directory tree ==\n'
find library/src/schemas/strictObject -maxdepth 2 -type f | sort
printf '\n== strictObject test file ==\n'
nl -ba library/src/schemas/strictObject/strictObject.test.ts | sed -n '1,220p'
printf '\n== strictObject implementation search ==\n'
rg -n "strictObject|createStrictObject|Object.fromEntries|__proto__|prototype" library/src/schemas/strictObject library/src -g '!**/*.map' || true
printf '\n== JS semantics probe ==\n'
node - <<'JS'
const exprs = [
"({ name: 'foo', __proto__: 'x' })",
"({ name: 'foo', ['__proto__']: 'x' })",
"Object.defineProperty({ name: 'foo' }, '__proto__', { value: 'x', enumerable: true })",
"({ __proto__: null, a: 1 })",
];
for (const expr of exprs) {
const obj = eval(expr);
console.log(expr);
console.log(' own keys:', JSON.stringify(Object.keys(obj)));
console.log(' hasOwn __proto__:', Object.prototype.hasOwnProperty.call(obj, '__proto__'));
console.log(' proto is null:', Object.getPrototypeOf(obj) === null);
console.log(' obj.__proto__:', obj.__proto__);
}
JSRepository: open-circle/valibot Length of output: 679 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Minimal probe for object-literal `__proto__` behavior.
node - <<'JS'
const a = { name: 'foo', __proto__: 'x' };
const b = { name: 'foo', ['__proto__']: 'x' };
console.log(JSON.stringify({
aOwn: Object.prototype.hasOwnProperty.call(a, '__proto__'),
aKeys: Object.keys(a),
aProto: Object.getPrototypeOf(a) === Object.prototype,
aValue: a.__proto__,
bOwn: Object.prototype.hasOwnProperty.call(b, '__proto__'),
bKeys: Object.keys(b),
bProto: Object.getPrototypeOf(b) === Object.prototype,
bValue: b.__proto__,
}));
JSRepository: open-circle/valibot Length of output: 283 Use a computed 🤖 Prompt for AI AgentsThere was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: The object literal Use a computed key to create a true own property: { name: 'foo', ['__proto__']: 'x' }Prompt for AI agents
Suggested change
|
||||||
| ] | ||||||
| ); | ||||||
| }); | ||||||
|
|
||||||
| // Complex types | ||||||
|
|
||||||
| // TODO: Enable this test again in case we find a reliable way to check for | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| import { describe, expect, test } from 'vitest'; | ||
| import { _hasOwnProperty } from './_hasOwnProperty.ts'; | ||
|
|
||
| describe('_hasOwnProperty', () => { | ||
| test('returns true for own properties', () => { | ||
| expect(_hasOwnProperty({ name: 'a' }, 'name')).toBe(true); | ||
| }); | ||
|
|
||
| test('returns false for inherited Object.prototype members', () => { | ||
| // Regression coverage for the prototype-pollution class of bug that | ||
| // #1523 fixed in the object schemas. Keys that look like they are | ||
| // defined entries must not be matched via the `in` operator's | ||
| // prototype walk. | ||
| expect(_hasOwnProperty({}, 'toString')).toBe(false); | ||
| expect(_hasOwnProperty({}, 'valueOf')).toBe(false); | ||
| expect(_hasOwnProperty({}, 'hasOwnProperty')).toBe(false); | ||
| expect(_hasOwnProperty({}, 'constructor')).toBe(false); | ||
| expect(_hasOwnProperty({}, '__proto__')).toBe(false); | ||
| }); | ||
|
|
||
| test('returns true for own properties that share a name with a prototype member', () => { | ||
| // A schema that intentionally defines an entry named like a prototype | ||
| // member is still recognized as a defined entry. The own-property | ||
| // check is precise, not name-based. | ||
| expect(_hasOwnProperty({ toString: () => undefined }, 'toString')).toBe(true); | ||
| expect(_hasOwnProperty({ constructor: 1 }, 'constructor')).toBe(true); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| /** | ||
| * Own-property check that survives prototype pollution. Mirrors the call | ||
| * pattern already used by `_isValidObjectKey` so the entry-membership | ||
| * checks in object schemas can reject keys like `__proto__`, | ||
| * `constructor`, and `toString` instead of inheriting them from | ||
| * `Object.prototype`. | ||
| * | ||
| * @param object The object to check. | ||
| * @param key The key to check. | ||
| * | ||
| * @returns Whether the key is an own property of the object. | ||
| * | ||
| * @internal | ||
| */ | ||
| // @__NO_SIDE_EFFECTS__ | ||
| export function _hasOwnProperty(object: object, key: string): boolean { | ||
| return Object.prototype.hasOwnProperty.call(object, key); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| export * from './_hasOwnProperty.ts'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
Repository: open-circle/valibot
Length of output: 174
🏁 Script executed:
Repository: open-circle/valibot
Length of output: 28050
🏁 Script executed:
Repository: open-circle/valibot
Length of output: 584
Add prototype-collision coverage to
looseObjectandobjectWithReststrictObject.test.tscovers this regression, butlibrary/src/schemas/looseObject/looseObject.test.tsandlibrary/src/schemas/objectWithRest/objectWithRest.test.tsstill lack cases for__proto__,constructor,toString, andvalueOf. Add direct tests for loose objects passing these keys through and object-with-rest validating them againstrest.🤖 Prompt for AI Agents