refactor(shared, json-schema): reuse getOwn for own-property lookups - #2025
Merged
Merged
Conversation
Converge every remaining path walk over untrusted segments onto the shared getOwn/setOwn helpers, matching BracketNotationSerializer (middleapi#2024): the RPC and OpenAPI JSON deserializers, getRouter, getRouterContract, JsonSchemaCoercer, and get() in @orpc/shared. getRouter and getRouterContract previously resolved segments like __proto__ and constructor through the prototype chain; they now return undefined. The other sites are idiom convergence: they were own-only already, but only via the previous iteration's Object.hasOwn guard.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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: |
Contributor
dinwwwh
changed the title
![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
getRouterandgetRouterContractstop walking the prototype chain —current[segment]becomesgetOwn(current, segment), so__proto__,constructor,toString, etc. returnundefinedinstead ofObject.prototype/Object. This is the PR's real behavioral change; the new assertions inpackages/server/src/router-utils.test.ts:341-347andpackages/contract/src/router-utils.test.ts:199-205fail against the pre-PR code.- RPC and OpenAPI JSON deserializers switch to
getOwn/setOwn— behavior-identical: each read key was already validated as an own property by the previous loop iteration'sObject.hasOwnguard, sogetOwnand a raw index agree, andsetOwnis defensive hardening. The new__proto__/constructortests pass against pre-PR code too, which the PR body acknowledges. JsonSchemaCoercerproperty lookup and write —schema.properties ?? {}plusgetOwnandsetOwn, preserving the__proto__-as-own-property semantics already relied on bycoercer.test.ts.get()/set()in@orpc/shared—getOwnreplaces the inlineObject.hasOwnguard; semantically identical, just the unified pattern.
I independently traced the deserializer read/write ordering and agree with the "behavior-identical" claim, and a reference pass over the request pipeline (rpc-matcher, openapi-matcher, bracket-notation, the prototype-pollution plugin) found no remaining unguarded prototype-traversing read or non-setOwn write reachable from attacker-controlled segments.
ℹ️ Resolver traversal is now stricter than the for...in indexers
walkProcedureContractsSync / walkProcedureContractsAsync / augmentImplementedRouter iterate with for...in, which enumerates inherited enumerable keys, while getRouter / getRouterContract now reject anything not an own property. The only shape affected is Object.create(baseRouter)-style inheritance, which is neither type-supported nor documented as a router shape and has no test, doc, or first-party builder in the repo — so this is intended hardening, not a regression. Worth being aware of if that inheritance style is ever meant to be supported.
Technical details
# Traversal semantics asymmetry
## Affected sites
- `packages/server/src/router-utils.ts:268,303,340` — `for (const key in router)` indexes inherited enumerable entries
- `packages/server/src/router-utils.ts:204` — `getOwn` rejects inherited entries at resolve time
- `packages/contract/src/router-utils.ts:75` — same for contracts
## Why it is not a finding
- Routers/contracts are documented and typed as plain own-property records
(`apps/content/docs/router.mdx`, `packages/server/src/router.ts`), and every
first-party builder (`augmentRouter`, `augmentImplementedRouter`,
`augmentContractRouter`, `createRouterImplementerInternal`) materializes entries
as own keys, so indexing and resolving agree for every supported shape.
- `withHiddenRouterContract` uses a Proxy with only a `get` trap, so
`Object.hasOwn` forwards to the target and own-key resolution is unchanged.
- `getRouter` is fed `entry.path` derived from the developer router, not the
request pathname, so the hardening is defense-in-depth for the matchers; it is
directly reachable via `createRouterClient` property access and the dynamic-client
link codec.
## Open question for the human
- Is `Object.create(base)` router inheritance intended to be supported? If not,
no action; if yes, the `for...in` walkers and the `getOwn` resolvers need to be
reconciled.DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
Revert getOwn/setOwn in the RPC and OpenAPI deserializers, getRouter, getRouterContract and the coercer write. The deserializer loops verify each segment with Object.hasOwn before reading it, so the extra check only costs, and router paths come from the router definition, not the request. Keep getOwn in get() and the coercer property lookup, which were hand-rolled copies of it.
dinwwwh
changed the title
Contributor
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
This run re-reviewed the delta since the prior pullfrog review (987ad554): commit 5dabce22 reverts the prototype-chain hardening that commit introduced, leaving a behavior-preserving refactor.
- Reverted the router and deserializer hardening —
getRouter,getRouterContract, the RPC/OpenAPI deserializer walks and theJsonSchemaCoercerwrite are back to their pre-PR reads/writes; the removedgetOwn/setOwncalls and their router tests are gone. - Kept the
getOwnreuse as a pure refactor —get()/set()inpackages/shared/src/object.tsand thepropertySchemaslookup inpackages/json-schema/src/coercer.tsdelegate togetOwn; traced against the code they replace, observable behavior is identical for__proto__,constructor, and symbol keys. - Kept the deserializer regression tests — the new RPC/OpenAPI
__proto__and ownconstructor→prototypecases still pass on the reverted sources, so they pin existing behavior rather than the reverted change.
pnpm vitest run for the four touched packages (286 tests), pnpm type:check, and pnpm lint all pass.
ℹ️ The branch no longer carries the prototype-chain hardening
Reverting returns getRouter / getRouterContract to raw current[segment], so __proto__ / constructor segments resolve through the prototype chain again. This matches main, and every caller feeds a router- or developer-derived path (rpc-matcher.ts:224, openapi-matcher.ts:180, router-client.ts:66, openapi-link-codec.ts:422), so it is not a request-reachable regression — but if that hardening is still wanted, it now needs a separate change.
Technical details
# Removed router hardening
## Affected sites
- packages/server/src/router-utils.ts:204 — `current[segment]` resolves inherited members
- packages/contract/src/router-utils.ts:75 — same for contracts
- packages/client/src/rpc-json-serializer.ts:386,406 — raw reads, guarded by `Object.hasOwn`
- packages/openapi/src/openapi-json-serializer.ts:322 — same
- packages/json-schema/src/coercer.ts:232 — raw write into `{ ...coerced }`
## Why it is not a bug
- Behavior matches `main`, so this is a scope reduction, not a regression.
- The deserializer loops validate each segment with `Object.hasOwn` before
reading it, and only write to keys already proven own, so a `__proto__`
segment cannot re-parent the container.
- `coercedItems` is spread from an `isPlainObject`-checked source, so an own
`__proto__` stays an own data property and the raw write updates it.
## Open question for the human
- Is the `getRouter` / `getRouterContract` prototype-chain rejection still
desired? If so it needs a follow-up PR.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.
get()in@orpc/sharedand the property lookup inJsonSchemaCoercerwere hand-rolled copies ofgetOwn; they now call it. The coercer'sproperties !== undefinedternary is gone in favor of the same?? {}default itspatternPropertieslookup already uses.Testing
__proto__key, or behind an ownconstructor→prototypechain, is restored in place with the globalObject.prototypeuntouched and the container never re-parented.pnpm vitest runacross client, openapi, json-schema and shared,pnpm lint, andpnpm type:checkpass.Not changed
getRouter/getRouterContractkeep their existing reads and writes. The deserializers already verify every segment withObject.hasOwnbefore reading it, and router paths come from the router definition rather than the request, so switching them togetOwn/setOwnwould only add a check per segment.