feat(server,contract): guard against primitive values in router tree traversal#1522
Conversation
`enhanceRouter`, `traverseContractProcedures`, and `unlazyRouter` all use `for (const key in router)` without verifying the value is an object. When a router module re-exports a primitive (e.g. `export const FOO = 'bar'`), the `for...in` loop iterates string character indices, eventually hitting a single-character string that recurses on itself infinitely — crashing with RangeError: Maximum call stack size exceeded. Add type guards before the `for...in` fallback in all three functions so that non-object values are returned/skipped instead of iterated. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Use .toEqual() with expected structure instead of .toBeDefined() for the unlazyRouter primitive-value test case. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
dosubot
bot
added
size:L
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces safety guards in packages/server/src/router-utils.ts to prevent infinite recursion when traversing router trees that contain non-object primitive values. It also adds descriptive JSDoc comments to several utility functions and includes a new suite of tests to verify correct handling of various primitive types. Feedback suggests that similar logic in the contract package should also be updated to prevent potential crashes in those modules.
jameskranz
force-pushed
the
fix/enhance-router-primitive-recursion-v2
branch
from
32952fd to
92ffa80
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded runtime guards to server and contract router utilities to prevent traversal into non-object (primitive or null) exports; added tests in both packages verifying procedures are augmented while primitive exports are left unchanged, including single-character string edge cases. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
jameskranz
force-pushed
the
fix/enhance-router-primitive-recursion-v2
branch
from
92ffa80 to
24438df
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/contract/src/router-utils.test.ts (2)
111-129: Consider adding boolean and null/undefined tests for consistency.
enhanceContractRoutertests cover boolean and null/undefined values (lines 98-108), but this suite doesn't. While the guard implementation handles all primitive types uniformly, adding these tests would ensure consistent coverage across all three functions.💡 Suggested additional test cases
it('handles number values without infinite recursion', () => { const routerWithNumber = { ping, VERSION: 42 } as any const minified = minifyContractRouter(routerWithNumber) expect(isContractProcedure((minified as any).ping)).toBe(true) }) + + it('handles boolean values without infinite recursion', () => { + const routerWithBool = { ping, ENABLED: true } as any + const minified = minifyContractRouter(routerWithBool) + expect(isContractProcedure((minified as any).ping)).toBe(true) + }) + + it('handles null and undefined values without infinite recursion', () => { + const routerWithNullish = { ping, NIL: null, UNDEF: undefined } as any + const minified = minifyContractRouter(routerWithNullish) + expect(isContractProcedure((minified as any).ping)).toBe(true) + }) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/contract/src/router-utils.test.ts` around lines 111 - 129, Add tests to the "minifyContractRouter with primitive values" suite to cover boolean and null/undefined cases similar to the existing string/number tests: create router objects like { ping, FLAG: true } and { ping, MISSING: null } (or undefined), call minifyContractRouter on each, and assert isContractProcedure((minified as any).ping) is true; place the tests as additional it(...) blocks alongside the existing ones to ensure parity with enhanceContractRouter coverage.
131-149: Same suggestion: consider adding boolean and null/undefined tests.For the same consistency reason as
minifyContractRouter, adding tests for boolean and null/undefined would complete the coverage parity withenhanceContractRouter.💡 Suggested additional test cases
it('handles number values without infinite recursion', () => { const routerWithNumber = { ping: oc.input(inputSchema), VERSION: 42 } as any const populated = populateContractRouterPaths(routerWithNumber) expect(isContractProcedure(populated.ping)).toBe(true) }) + + it('handles boolean values without infinite recursion', () => { + const routerWithBool = { ping: oc.input(inputSchema), ENABLED: true } as any + const populated = populateContractRouterPaths(routerWithBool) + expect(isContractProcedure(populated.ping)).toBe(true) + }) + + it('handles null and undefined values without infinite recursion', () => { + const routerWithNullish = { ping: oc.input(inputSchema), NIL: null, UNDEF: undefined } as any + const populated = populateContractRouterPaths(routerWithNullish) + expect(isContractProcedure(populated.ping)).toBe(true) + }) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/contract/src/router-utils.test.ts` around lines 131 - 149, Add tests to populateContractRouterPaths to cover boolean and null/undefined primitive values similar to the existing string/number tests: create router variations like { ping: oc.input(inputSchema), FLAG: true } and { ping: oc.input(inputSchema), MISSING: null } (and undefined), call populateContractRouterPaths on each, and assert expect(isContractProcedure(populated.ping)).toBe(true). Mirror the style and naming of the current cases so coverage matches minifyContractRouter and enhanceContractRouter.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/contract/src/router-utils.test.ts`:
- Around line 111-129: Add tests to the "minifyContractRouter with primitive
values" suite to cover boolean and null/undefined cases similar to the existing
string/number tests: create router objects like { ping, FLAG: true } and { ping,
MISSING: null } (or undefined), call minifyContractRouter on each, and assert
isContractProcedure((minified as any).ping) is true; place the tests as
additional it(...) blocks alongside the existing ones to ensure parity with
enhanceContractRouter coverage.
- Around line 131-149: Add tests to populateContractRouterPaths to cover boolean
and null/undefined primitive values similar to the existing string/number tests:
create router variations like { ping: oc.input(inputSchema), FLAG: true } and {
ping: oc.input(inputSchema), MISSING: null } (and undefined), call
populateContractRouterPaths on each, and assert
expect(isContractProcedure(populated.ping)).toBe(true). Mirror the style and
naming of the current cases so coverage matches minifyContractRouter and
enhanceContractRouter.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b4523cde-6f58-4035-85e1-37ca5c843057
📒 Files selected for processing (2)
packages/contract/src/router-utils.test.tspackages/contract/src/router-utils.ts
Apply the same primitive type guards to enhanceContractRouter, minifyContractRouter, and populateContractRouterPaths in the contract package. These functions have the same infinite recursion vulnerability when router modules export string values alongside contract procedures. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
jameskranz
force-pushed
the
fix/enhance-router-primitive-recursion-v2
branch
from
9167d2b to
f2fc836
Compare
jameskranz
changed the title
|
But why is this necessary? Why doesn't TypeScript warn you that the oRPC router is invalid when it contains non-procedure properties? |
The Router mapped type does map non-procedure properties to never, so in theory TypeScript should catch this. But it doesn't when you use import * as module to compose routers -- the namespace type is wide enough to slip past the constraint, and a few internal .router() paths cast through as any. We've hit this twice in our codebase. Both times we were using import * as namespace imports, which is pretty much the standard way to compose routers from module files. Once it was a string constant exported alongside procedures, and once it was an exported object with circular internals. No TypeScript errors either time. Since the type system has this gap with namespace imports, it seems like the runtime should just skip non-procedure values instead of blowing up the stack. The change itself is pretty small -- just a typeof check before the for...in fallback, same pattern traverseContractProcedures already uses. |
|
@jameskranz This PR doesn't actually fix the circular object case mentioned. The The A cleaner approach would be filtering non-procedure exports at the boundary before they enter the router at all: function filterInternalExport<T extends Record<any, any>>(module: T): T {
return Object.fromEntries(
Object.entries(module).filter(([, v]) => return false for internal exports)
)
}
// Usage
import * as userModule from './user'
const router = { user: filterInternalExport(userModule) } |
You're right that this doesn't handle circular objects -- that would need visited-set tracking, which is a bigger change. But those are pretty different problems. Circular references in plain objects are rare and arguably a bug in the consumer's code. Exporting a string constant next to procedures is just normal module authoring, and it shouldn't take down the process. On the 'as any' -- those casts are all pre-existing in router-utils.ts, not something this PR introduces. Every branch in enhanceRouter already returns as any (lazy, procedure, object iteration). The primitive guard just follows the same convention. Definitely worth cleaning up but feels like separate work. The filter approach works but it means every consumer needs to know that namespace imports are unsafe and wrap them before passing to .router(). That filter also needs to know what to keep, which basically means checking isProcedure, at which point it makes more sense to do that inside the library where you already have access to those checks. If you'd rather go that route -- skipping non-procedure values with isProcedure/isContractProcedure checks instead of the primitive guard -- I'm open to reworking the PR around that. It'd handle both primitives and unexpected objects, and it's more precise about what actually gets iterated. |
export a primitive value and import it as a router is rare too, so both cases are equally edge cases. For the filter approach, oRPC exports almost everything already so this use case is too specific to justify adding it to the library. Honestly I think the real fix is just being explicit about what goes into the router rather than relying on whole module exports: import * as userModule from './user'
const router = { user: { procedure1: userModule.procedure1, procedure2: userModule.procedure2 } }If your module exports more than what the router needs, that's a signal to be explicit rather than relying on the library to silently filter things out. |
I hear where you're coming from on keeping the library core lean, but I think "Explicit Property Mapping" shifts a heavy maintenance burden onto the consumer that scales poorly. In our project, we’re managing over 50 modules and hundreds of procedures. If we have to manually map every single one in both the main contract and the implementation router, we’re essentially creating a redundant registry that has to be kept in sync by hand. It’s a constant point of failure—every time a dev adds a procedure, they have to remember to update two other files, or the client-side types just silently break. Namespace imports are a standard way to compose large systems in TypeScript. The fact that oRPC hits a stack overflow because of a stray constant suggests the traversal logic is a bit too trusting of the input shape. Instead of the library crashing because it found something it didn't expect, it'd be much more robust to just ignore anything that isn't a procedure or a nested router. By using isProcedure or isContractProcedure during traversal, we move to an allow-list approach. The library becomes defensive against any non-oRPC exports—whether that's a version string, a helper function, or even a circular reference—without forcing users into hundreds of lines of boilerplate. It seems like a win for stability without adding meaningful complexity to the codebase. |
dinwwwh
changed the title
|
I believe to support your case we need improve https://github.com/middleapi/orpc/blob/main/packages/server/src/router-utils.ts#L31 too |
More templates
@orpc/ai-sdk
@orpc/arktype
@orpc/client
@orpc/contract
@orpc/experimental-durable-iterator
@orpc/hey-api
@orpc/interop
@orpc/json-schema
@orpc/nest
@orpc/openapi
@orpc/openapi-client
@orpc/otel
@orpc/experimental-pino
@orpc/experimental-publisher
@orpc/experimental-publisher-durable-object
@orpc/experimental-ratelimit
@orpc/react
@orpc/react-query
@orpc/experimental-react-swr
@orpc/server
@orpc/shared
@orpc/solid-query
@orpc/standard-server
@orpc/standard-server-aws-lambda
@orpc/standard-server-fastify
@orpc/standard-server-fetch
@orpc/standard-server-node
@orpc/standard-server-peer
@orpc/svelte-query
@orpc/tanstack-query
@orpc/trpc
@orpc/valibot
@orpc/vue-colada
@orpc/vue-query
@orpc/zod
commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
- Add typeof guard to getRouter and getContractRouter, mirroring the
pattern used by enhance/minify/populate/unlazy in this PR. Without
it, traversal walks character indices ('v'[0] === 'v') and returns
garbage instead of undefined.
- Replace inferred-union test bindings with precise type casts so
property assertions are statically checked instead of any-cast.
- Add primitive-traversal coverage for getRouter / getContractRouter.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/server/src/router-utils.test.ts (1)
253-266: Primitive passthrough assertions are incomplete.
moduleWithPrimitivesincludesDEPRECATEDandOPTIONAL_FEATURE, but these are not asserted inenhanceRouterandunlazyRoutertests. Add explicit checks to lock in null/undefined preservation behavior.Proposed test assertions
@@ expect(enhanced.API_VERSION).toBe('v2') expect(enhanced.MAX_PAGE_SIZE).toBe(100) expect(enhanced.ENABLE_CACHE).toBe(true) + expect(enhanced.DEPRECATED).toBeNull() + expect(enhanced.OPTIONAL_FEATURE).toBeUndefined() @@ expect(result.getUser).toEqual(pong) expect(result.listUsers).toEqual(pong) expect(result.API_VERSION).toBe('v2') expect(result.MAX_PAGE_SIZE).toBe(100) + expect(result.ENABLE_CACHE).toBe(true) + expect(result.DEPRECATED).toBeNull() + expect(result.OPTIONAL_FEATURE).toBeUndefined()Also applies to: 312-318
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server/src/router-utils.test.ts` around lines 253 - 266, Update the tests for enhanceRouter and unlazyRouter to assert that primitive exports with null/undefined are preserved: locate the test cases using moduleWithPrimitives and add explicit expectations that DEPRECATED is null and OPTIONAL_FEATURE is undefined (or the exact values present in moduleWithPrimitives) after calling enhanceRouter and unlazyRouter; reference the enhanced variables (e.g., enhanced.DEPRECATED, enhanced.OPTIONAL_FEATURE and similarly for the result of unlazyRouter) so the tests lock in null/undefined passthrough behavior alongside the existing API_VERSION / MAX_PAGE_SIZE / ENABLE_CACHE assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/server/src/router-utils.test.ts`:
- Around line 294-305: Test is excluding null/undefined crash path; update
traverseContractProcedures test to include cases where exported values can be
null/undefined and then fix the traversal order in
packages/server/src/router-utils.ts so the type guard runs before accessing
hidden contract data. Specifically, in traverseContractProcedures ensure the
guard that checks for non-null/undefined and proper shape runs before any call
to getHiddenRouterContract (or any access of hiddenContract properties) and
adjust the iteration order in the traverseContractProcedures implementation so
getHiddenRouterContract is only invoked after the guard passes (refer to
function names traverseContractProcedures and getHiddenRouterContract to locate
the logic). Ensure tests reflect the real failure mode by adding an export like
export const X = null and asserting traversal skips or handles it without
throwing.
---
Nitpick comments:
In `@packages/server/src/router-utils.test.ts`:
- Around line 253-266: Update the tests for enhanceRouter and unlazyRouter to
assert that primitive exports with null/undefined are preserved: locate the test
cases using moduleWithPrimitives and add explicit expectations that DEPRECATED
is null and OPTIONAL_FEATURE is undefined (or the exact values present in
moduleWithPrimitives) after calling enhanceRouter and unlazyRouter; reference
the enhanced variables (e.g., enhanced.DEPRECATED, enhanced.OPTIONAL_FEATURE and
similarly for the result of unlazyRouter) so the tests lock in null/undefined
passthrough behavior alongside the existing API_VERSION / MAX_PAGE_SIZE /
ENABLE_CACHE assertions.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5cd64dea-2ff2-4a19-8ac3-a8db5d04912d
📒 Files selected for processing (4)
packages/contract/src/router-utils.test.tspackages/contract/src/router-utils.tspackages/server/src/router-utils.test.tspackages/server/src/router-utils.ts
✅ Files skipped from review due to trivial changes (1)
- packages/contract/src/router-utils.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/contract/src/router-utils.test.ts
- packages/server/src/router-utils.ts
|
Addressed the getRouter improvement. Same typeof guard pattern, plus the parity fix on getContractRouter. Tests cover the worst-case single-character export for both. |
…exports getHiddenRouterContract reads a symbol property and throws on null or undefined. Recursing into a child export of `null` (e.g. `export const X = null`) crashed before any guard ran. Move the typeof/null guard above the hidden-contract lookup, and update the test to use a fixture containing null/undefined so the crash path is covered. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…sertions The shape assertions on the return values of enhanceContractRouter, populateContractRouterPaths, and enhanceRouter target a concrete object shape, but the functions return a union with the procedure type, which doesn't structurally overlap. tsc rejects the direct cast (TS2352) and suggests routing through unknown — applying that at the three sites. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
hopefully final fix for lint. all pass locally |
2 participants
Summary
for (const key in router)loops without validating that values are objects crash withRangeError: Maximum call stack size exceededwhen a router module exports primitives (e.g.,export const FOO = "bar").enhanceRouter,traverseContractProcedures, andunlazyRouterinpackages/server/src/router-utils.ts.enhanceContractRouter,minifyContractRouter, andpopulateContractRouterPathsinpackages/contract/src/router-utils.ts.Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests