fix(openapi): prevent prototype pollution through object values in bracket notation - #2024
Merged
dinwwwh merged 3 commits intoSep 15, 2026
Conversation
…acket notation The deserializer creates its own containers as NullProtoObj or arrays, but a value coming from the input that passes isPlainObject was kept and traversed. A `__proto__` segment on such a value resolved to Object.prototype through the prototype chain, and isPlainObject accepts it (its own proto is null), so the final write landed on the global prototype. Read and write own properties only: getOwn for reads, setOwn for writes, and Object.hasOwn instead of `in`. Export setOwn from @orpc/shared, which already used it internally for set() and clone().
More templates
@orpc/ai-sdk
@orpc/arktype
@orpc/bun
@orpc/client
@orpc/cloudflare
@orpc/contract
@orpc/experimental-effect
@orpc/evlog
@orpc/hibernation
@orpc/json-schema
@orpc/experimental-lock
@orpc/experimental-msw
@orpc/nest
@orpc/next
@orpc/node
@orpc/openapi
@orpc/opentelemetry
@orpc/pinia-colada
@orpc/pino
@orpc/publisher
@orpc/ratelimit
@orpc/server
@orpc/shared
@orpc/swr
@orpc/tanstack-query
@orpc/trpc
@orpc/valibot
@orpc/zod
commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Contributor
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Own-property-only traversal in
deserialize—getOwn/setOwn/Object.hasOwnreplace directcurrentRef[nextSegment]reads, direct assignment, andinchecks, so a__proto__orconstructorsegment on an input-provided object value can no longer resolve intoObject.prototype. The container is now built in a localchildand written back once per step. setOwnexported from@orpc/shared— promoted from module-private (packages/shared/src/object.ts:124); already re-exported viaexport * from './object'and used byset()/clone().- Regression test — bracket-notation.test.ts:414 covers
__proto__andconstructor[prototype]reached through object values and asserts global prototypes stay clean.
I verified the new test is not a tautology: reverting the fix reproduces Object.prototype.polluted === '1' and fails the test. pnpm vitest run packages/openapi (456 tests) plus tsc -b for @orpc/shared and @orpc/openapi all pass. The documented behavior change (a[toString] on an object value now assigns rather than merging the inherited method) is intentional and does not affect NullProtoObj containers.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
Collapse the four array-conversion branches into a single decision, so the
array-or-object rule and its two converters live in one place instead of five.
Drop the `ref: { value }` wrapper: the root is always a NullProtoObj, so the
first loop iteration could only ever write the root back onto itself. The walk
now starts at the first segment and returns the root directly.
Write the container back only when it changed, which keeps re-walked paths
free of pointless stores and stops a literal `__proto__` segment from re-running
Object.defineProperty on every entry.
setOwn is now exported, so test it directly: ordinary writes, symbol and number keys, array indexes, and the __proto__ case, including that the defined property stays writable, enumerable and configurable. Enumerability is what lets a deserialized __proto__ key survive a for...in round trip. Also feed findDeepMatches a value that is neither a match, an array nor a plain object, bringing object.ts to full branch coverage.
Contributor
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Reviewed the delta since the prior pullfrog review (790eb17a): the deserializer walk rewrite in a326989b plus the new tests in 79dad15c.
- Reworked the
deserializetraversal —rootis now the realNullProtoObjcontainer;nextSegmentstarts atsegments[0]and the loop runs fromi = 1, dropping the{ value }wrapper and the empty-input early return. - Collapsed array / push-style conversion — the split
i !== last/i === lastbranches are replaced by a singlecanStayArraypredicate with identical semantics, including the push-style marker lifecycle. - Conditional write-back —
setOwnnow runs only when the child reference changes, so an input-provided object value already at that key is no longer rewritten on every step. This also retires the frozen/non-writable-object concern noted in the previous review. setOwnunit tests — newdescribe('setOwn')block in@orpc/shared; thefindDeepMatchesfixture gains a non-matching property to exercise the skip branch.
Verification: bracket-notation.test.ts (36) and object.test.ts (62) pass, and a 50,000-case differential fuzz of the new deserialize against the previously reviewed implementation (790eb17a) produced identical output and identical error behavior on every case.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
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.
BracketNotationSerializer.deserializecould write to the globalObject.prototypewhen an entry's value was a plain object and a later path walked through it with__proto__. It now reads and writes own properties only, so dangerous segments land as plain data instead of touching a real prototype.Containers the deserializer builds itself were already safe. The hole was containers it adopts: a value from the input that passes
isPlainObjectbecame one, and reading__proto__off it resolved through the prototype chain toObject.prototype, whichisPlainObjectaccepts because its own prototype isnull.Fixes
__proto__on the containers it creates.URLSearchParamsandFormDataentries, so values arestring | Fileand get replaced before traversal. This hardens the public API surface, which accepts[string, unknown][]and backsparseFormData.setOwnis now exported from@orpc/shared. It already existed there, module private, backingset()andclone().Behavior
One intended change: a path like
a[toString]against an object value no longer resolves the inherited method and merges into[fn, value], it simply assigns. Array index handling, append style[]entries, duplicate key merging and the index cap are all unchanged.The walk is also simpler and does less work: the array or object rule now lives in one decision instead of five, a redundant wrapper object is gone, and containers are only written back when they actually changed, so re-walked paths and repeated
__proto__segments no longer re-store on every entry.Testing
Regression tests for object values reached via
__proto__andconstructor[prototype], plus direct tests for the newly exportedsetOwn. The pollution test fails onmain, where the leak also breaks two unrelated tests later in the same process.packages/openapi,packages/sharedandpackages/nextpass (826 tests), along withpnpm lintandpnpm type:check. Both changed source files are at 100% statement, branch, function and line coverage from their own unit tests.