fix(router-core): safely format Standard Schema validation issues#7782
Open
TemRevil wants to merge 1 commit into
Open
fix(router-core): safely format Standard Schema validation issues#7782TemRevil wants to merge 1 commit into
TemRevil wants to merge 1 commit into
Conversation
Standard Schema validators can return issue objects that JSON.stringify cannot serialize, for example when a path segment is a symbol or the issue holds a circular reference. Both the server-function validator in start-client-core and router search validation stringified the raw issues array, so a value that could not be serialized threw and buried the real validation failure. Add a shared internal formatter in router-core that only reads each issue's message and path, so it never throws on unserializable values. Paths are rendered with bracketed, quoted segments (for example ["user"]["name"][0]) so a literal key such as "a.b" cannot be confused with a nested path, and a prototype-named key such as "__proto__" is only ever printed as text, never used to index into an object. Root issues render as (root). Wire the formatter into both call sites and cover root issues, nested paths, numeric keys, symbol and prototype-named keys, and a circular issue object with unit tests.
Contributor
📝 WalkthroughWalkthroughAdds a shared Standard Schema validation-error formatter, exposes it from ChangesValidation error formatting
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 `@packages/start-client-core/src/createServerFn.ts`:
- Line 904: Update the if statement handling result.issues to use curly braces
around the throw statement, following the project’s control-flow style
guidelines.
🪄 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: 7833e68a-61cf-4f6c-8e09-e95351fc1014
📒 Files selected for processing (6)
packages/router-core/src/index.tspackages/router-core/src/router.tspackages/router-core/src/validationError.tspackages/router-core/src/validators.tspackages/router-core/tests/validationError.test.tspackages/start-client-core/src/createServerFn.ts
|
|
||
| if (result.issues) | ||
| throw new Error(JSON.stringify(result.issues, undefined, 2)) | ||
| if (result.issues) throw new Error(formatValidationError(result.issues)) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add curly braces to the if statement.
Per the project's coding guidelines for **/*.{ts,tsx,js,jsx}: "Always use curly braces for if, else, loops, and similar control statements. Never write one-line bodies like if (foo) x = 1."
♻️ Proposed fix
- if (result.issues) throw new Error(formatValidationError(result.issues))
+ if (result.issues) {
+ throw new Error(formatValidationError(result.issues))
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (result.issues) throw new Error(formatValidationError(result.issues)) | |
| if (result.issues) { | |
| throw new Error(formatValidationError(result.issues)) | |
| } |
🤖 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 `@packages/start-client-core/src/createServerFn.ts` at line 904, Update the if
statement handling result.issues to use curly braces around the throw statement,
following the project’s control-flow style guidelines.
Source: Coding guidelines
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.
Closes #7779.
Problem
Two places serialize Standard Schema validator issues the same way:
execValidatorinstart-client-core/src/createServerFn.tsdidthrow new Error(JSON.stringify(result.issues, undefined, 2)).validateSearchinrouter-core/src/router.tsdidthrow new SearchParamError(JSON.stringify(result.issues, undefined, 2), { cause: result }).The Standard Schema spec allows an issue
pathto contain anyPropertyKey, and a segment can be either a raw key or a{ key }object.JSON.stringifythrows on asymbolkey or abigint, and on any circular reference an issue object happens to carry. When that happens the thrown error is the serializer's error, not the validation failure, so the actual reason the input was rejected is lost right at the point a developer needs it.Approach
I started by looking at what a "safe" version of the existing output would be, and a plain
JSON.stringifywith a replacer that drops non-serializable values felt wrong: it keeps the JSON shape but still leaks the whole internal issue object, and the output is noisy. The useful information in an issue is really just the message and the path, so I settled on a small formatter that reads only those two fields and never touches anything else on the object. That also sidesteps the circular-reference case for free, because it never walks into arbitrary properties.The path rendering took a couple of tries. A dot path like
user.namereads well until a key legitimately contains a dot or a bracket, at which pointa.bis ambiguous between one key"a.b"and the nested patha -> b. So each segment is bracketed and quoted withJSON.stringifyof the key:["user"]["name"][0]. Numbers render as[0]so array indices stay readable, and symbols render viaString(key)instead of throwing. Because every string key goes throughJSON.stringifyrather than being used to index into an object, a key named__proto__is only ever printed as text and can never walk the prototype chain. Root issues with no path render as(root).The formatter lives in
router-core(src/validationError.ts) so both call sites share one implementation. The issue asks to avoid growing the publicrouter-coresurface unless there is a supported use case.start-client-coreconsumes it across the package boundary throughworkspace:*, which resolves through the publishedexportsmap, so it does need to be reachable. I exported it from the main entry to match how existing shared helpers likeisRedirectandparseRedirectare shared today. If you would rather keep it off the primary barrel and expose it through a dedicated subpath instead, I am happy to move it.I also added an optional
pathfield toAnyStandardSchemaValidateIssueinvalidators.tsso the formatter can read it in a typed way. It is optional, so it does not change any existing usage.Tests
packages/router-core/tests/validationError.test.tscovers:{ key }object segments,__proto__rendered as text without prototype access,I have not added the server-function E2E scenarios across multiple Standard Schema libraries or the bundle-size run mentioned in the issue, since those touch a wider surface. Happy to follow up in a separate PR if you would like them in here.
Summary by CodeRabbit
New Features
Bug Fixes