fix(object): use own-property check so prototype keys aren't matched as entries (#1523)#1536
Open
sanjibani wants to merge 1 commit into
Open
Conversation
dosubot
Bot
added
size:S
WalkthroughAdds a shared 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@library/src/schemas/strictObject/strictObject.test.ts`:
- Line 124: The strictObject test case is using the object-literal prototype
setter instead of creating an own __proto__ property, so it does not exercise
the intended collision path. Update the sample in strictObject.test.ts to use a
computed __proto__ key (or define the property explicitly) within the relevant
test around strictObject so the regression covers an actual own-property
conflict.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0bd9b144-a530-42be-9d86-e79df0581677
📒 Files selected for processing (4)
library/src/schemas/strictObject/strictObject.test.tslibrary/src/utils/_hasOwnProperty/_hasOwnProperty.test.tslibrary/src/utils/_hasOwnProperty/_hasOwnProperty.tslibrary/src/utils/_hasOwnProperty/index.ts
| [ | ||
| { name: 'foo', toString: 'bar' }, | ||
| { name: 'foo', constructor: 1 }, | ||
| { name: 'foo', __proto__: 'x' }, |
There was a problem hiding this comment.
🎯 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 __proto__ key here — { name: 'foo', __proto__: 'x' } uses the special object-literal prototype setter, so it won’t create an own __proto__ property. Switch this sample to ['__proto__'] or Object.defineProperty so the regression covers the intended collision path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@library/src/schemas/strictObject/strictObject.test.ts` at line 124, The
strictObject test case is using the object-literal prototype setter instead of
creating an own __proto__ property, so it does not exercise the intended
collision path. Update the sample in strictObject.test.ts to use a computed
__proto__ key (or define the property explicitly) within the relevant test
around strictObject so the regression covers an actual own-property conflict.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="library/src/schemas/strictObject/strictObject.test.ts">
<violation number="1" location="library/src/schemas/strictObject/strictObject.test.ts:124">
P2: The object literal `{ name: 'foo', __proto__: 'x' }` uses JavaScript's special prototype-setter syntax rather than creating an own property named `__proto__`. This means the resulting object only has `name` as an own key, so this test case doesn't actually exercise the `__proto__` collision path.
Use a computed key to create a true own property:
```js
{ name: 'foo', ['__proto__']: 'x' }
```</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| [ | ||
| { name: 'foo', toString: 'bar' }, | ||
| { name: 'foo', constructor: 1 }, | ||
| { name: 'foo', __proto__: 'x' }, |
There was a problem hiding this comment.
P2: The object literal { name: 'foo', __proto__: 'x' } uses JavaScript's special prototype-setter syntax rather than creating an own property named __proto__. This means the resulting object only has name as an own key, so this test case doesn't actually exercise the __proto__ collision path.
Use a computed key to create a true own property:
{ name: 'foo', ['__proto__']: 'x' }Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At library/src/schemas/strictObject/strictObject.test.ts, line 124:
<comment>The object literal `{ name: 'foo', __proto__: 'x' }` uses JavaScript's special prototype-setter syntax rather than creating an own property named `__proto__`. This means the resulting object only has `name` as an own key, so this test case doesn't actually exercise the `__proto__` collision path.
Use a computed key to create a true own property:
```js
{ name: 'foo', ['__proto__']: 'x' }
```</comment>
<file context>
@@ -108,6 +108,24 @@ describe('strictObject', () => {
+ [
+ { name: 'foo', toString: 'bar' },
+ { name: 'foo', constructor: 1 },
+ { name: 'foo', __proto__: 'x' },
+ ]
+ );
</file context>
Suggested change
| { name: 'foo', __proto__: 'x' }, | |
| { name: 'foo', ['__proto__']: 'x' }, |
…as entries `strictObject`, `looseObject`, and `objectWithRest` (and their async variants) check whether an input key is a defined schema entry with `key in this.entries`. The `in` operator also matches inherited `Object.prototype` members, so an input key named like a prototype member was treated as a defined entry. Effect: - `strictObject` / `strictObjectAsync` failed to reject `__proto__`, `constructor`, `toString`, `valueOf`, and friends as unknown keys. A `__proto__` or `constructor` key passed a strict schema silently. - `looseObject` / `looseObjectAsync` dropped such keys instead of passing them through. - `objectWithRest` / `objectWithRestAsync` skipped such keys instead of validating them against the `rest` schema. Replace each `key in this.entries` membership check with a new `_hasOwnProperty(this.entries, key)` helper - the same `Object.prototype.hasOwnProperty.call` pattern already used by `_isValidObjectKey` for input keys and by the open-circle#1522 fixes. A schema that intentionally defines an entry named like a prototype member (`v.object({ toString: v.string() })`) still works, since the own-property check is precise. The new helper lives at `library/src/utils/_hasOwnProperty/` and is re-exported from the utils barrel alongside the existing `_isValidObjectKey` so the two prototype-aware checks sit side by side. Tests: - New `_hasOwnProperty` unit suite covering own properties, inherited `Object.prototype` members, and own properties that share a name with a prototype member. - New `strictObject` regression test asserting that an input carrying `toString` / `constructor` / `__proto__` is now rejected as an unknown key. The pre-fix code returned a clean parse for these inputs. Closes open-circle#1523
sanjibani
force-pushed
the
fix/issue-1523-object-prototype-keys
branch
from
July 9, 2026 09:40
a6d6f58 to
7e2ebb6
Compare
dosubot
Bot
added
size:M
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@library/src/schemas/looseObject/looseObject.ts`:
- Line 209: Add regression coverage in the `looseObject` and `objectWithRest`
test suites for prototype-collision keys. In `looseObject.test.ts`, add direct
cases showing `LooseObject` accepts and preserves `__proto__`, `constructor`,
`toString`, and `valueOf` on `this.entries` without treating them as collisions.
In `objectWithRest.test.ts`, add corresponding `ObjectWithRest` cases that
validate those keys against `rest` behavior, using the existing
`looseObject`/`objectWithRest` helpers and assertions to mirror the
`strictObject.test.ts` regression coverage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2d9eb938-8eb3-4805-8ec3-8455981d9a6a
📒 Files selected for processing (11)
library/src/schemas/looseObject/looseObject.tslibrary/src/schemas/looseObject/looseObjectAsync.tslibrary/src/schemas/objectWithRest/objectWithRest.tslibrary/src/schemas/objectWithRest/objectWithRestAsync.tslibrary/src/schemas/strictObject/strictObject.test.tslibrary/src/schemas/strictObject/strictObject.tslibrary/src/schemas/strictObject/strictObjectAsync.tslibrary/src/utils/_hasOwnProperty/_hasOwnProperty.test.tslibrary/src/utils/_hasOwnProperty/_hasOwnProperty.tslibrary/src/utils/_hasOwnProperty/index.tslibrary/src/utils/index.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- library/src/utils/_hasOwnProperty/_hasOwnProperty.test.ts
- library/src/utils/_hasOwnProperty/index.ts
- library/src/utils/_hasOwnProperty/_hasOwnProperty.ts
- library/src/schemas/strictObject/strictObject.test.ts
| if (!dataset.issues || !config.abortEarly) { | ||
| for (const key in input) { | ||
| if (_isValidObjectKey(input, key) && !(key in this.entries)) { | ||
| if (_isValidObjectKey(input, key) && !_hasOwnProperty(this.entries, key)) { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for prototype-collision test coverage in looseObject and objectWithRest test files
rg -n '__proto__\|constructor\|toString\|valueOf\|prototype' \
library/src/schemas/looseObject/looseObject.test.ts \
library/src/schemas/looseObject/looseObjectAsync.test.ts \
library/src/schemas/objectWithRest/objectWithRest.test.ts \
library/src/schemas/objectWithRest/objectWithRestAsync.test.ts \
library/src/schemas/looseObject/looseObjectAsync.test.ts 2>/dev/null || echo "No matches found"Repository: open-circle/valibot
Length of output: 174
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== test file locations =="
fd -a 'looseObject*.test.ts' library/src || true
fd -a 'objectWithRest*.test.ts' library/src || true
echo
echo "== repository-wide prototype-collision references =="
rg -n '__proto__|constructor|toString|valueOf|prototype' library/src --glob '*test.ts' --glob '*test.tsx' || true
echo
echo "== schema test names around looseObject/objectWithRest =="
rg -n 'looseObject|objectWithRest' library/src --glob '*test.ts' || trueRepository: open-circle/valibot
Length of output: 28050
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file sizes =="
wc -l library/src/schemas/looseObject/looseObject.test.ts library/src/schemas/objectWithRest/objectWithRest.test.ts
echo
echo "== looseObject test outline =="
ast-grep outline library/src/schemas/looseObject/looseObject.test.ts --view expanded || true
echo
echo "== objectWithRest test outline =="
ast-grep outline library/src/schemas/objectWithRest/objectWithRest.test.ts --view expanded || trueRepository: open-circle/valibot
Length of output: 584
Add prototype-collision coverage to looseObject and objectWithRest
strictObject.test.ts covers this regression, but library/src/schemas/looseObject/looseObject.test.ts and library/src/schemas/objectWithRest/objectWithRest.test.ts still lack cases for __proto__, constructor, toString, and valueOf. Add direct tests for loose objects passing these keys through and object-with-rest validating them against rest.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@library/src/schemas/looseObject/looseObject.ts` at line 209, Add regression
coverage in the `looseObject` and `objectWithRest` test suites for
prototype-collision keys. In `looseObject.test.ts`, add direct cases showing
`LooseObject` accepts and preserves `__proto__`, `constructor`, `toString`, and
`valueOf` on `this.entries` without treating them as collisions. In
`objectWithRest.test.ts`, add corresponding `ObjectWithRest` cases that validate
those keys against `rest` behavior, using the existing
`looseObject`/`objectWithRest` helpers and assertions to mirror the
`strictObject.test.ts` regression coverage.
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
1 participant
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.
Fixes #1523.
Bug
`strictObject`, `looseObject`, and `objectWithRest` (and their async variants) checked whether an input key was a defined schema entry with `key in this.entries`. The `in` operator also matches inherited `Object.prototype` members, so an input key named like a prototype member was treated as a defined entry.
Effect:
Before the fix:
```ts
v.parse(v.strictObject({ name: v.string() }), { name: 'a', toString: 'b' });
// returns { name: 'a' }; the unknown `toString` key is not reported
```
Fix
Replace each `key in this.entries` membership check with a new `_hasOwnProperty(this.entries, key)` helper - the same `Object.prototype.hasOwnProperty.call` pattern already used by `_isValidObjectKey` for input keys and by the #1522 fixes. A schema that intentionally defines an entry named like a prototype member (`v.object({ toString: v.string() })`) still works, since the own-property check is precise, not name-based.
The new helper lives at `library/src/utils/_hasOwnProperty/` and is re-exported from the utils barrel alongside the existing `_isValidObjectKey` so the two prototype-aware checks sit side by side.
Tests
8 files changed, +96/-8.
Summary by CodeRabbit
New Features
Bug Fixes
Tests