-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
fix: whitespace in server function .handler() not matching compiler regex #6216
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
📝 WalkthroughWalkthroughBroadened ServerFn pre-scan detection: the regex now matches Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/start-plugin-core/src/start-compiler-plugin/compiler.ts (1)
90-90: Critical security fix: LGTM with optional enhancement suggestion.The regex update correctly addresses the reported security issue where server code could leak to the client when whitespace appears between the dot and
handler(). The pattern\.\s*handler\s*\(properly matches method chains with whitespace/newlines, and addingcreateServerFn\s*\(provides broader coverage for edge cases in reformatted code.Optional: Add word boundary for more precise matching
Consider adding a word boundary
\bbeforecreateServerFnto prevent potential false positives likemycreateServerFn():- ServerFn: /createServerFn\s*\(|\.\s*handler\s*\(/, + ServerFn: /\bcreateServerFn\s*\(|\.\s*handler\s*\(/,Note: This is a minor improvement since the pre-scan is validated by AST parsing, so the current implementation is acceptable.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/start-plugin-core/src/start-compiler-plugin/compiler.tspackages/start-plugin-core/tests/compiler.test.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript strict mode with extensive type safety for all code
Files:
packages/start-plugin-core/src/start-compiler-plugin/compiler.tspackages/start-plugin-core/tests/compiler.test.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Implement ESLint rules for router best practices using the ESLint plugin router
Files:
packages/start-plugin-core/src/start-compiler-plugin/compiler.tspackages/start-plugin-core/tests/compiler.test.ts
🧬 Code graph analysis (1)
packages/start-plugin-core/tests/compiler.test.ts (1)
packages/start-plugin-core/src/start-compiler-plugin/compiler.ts (1)
detectKindsInCode(146-162)
🔇 Additional comments (2)
packages/start-plugin-core/tests/compiler.test.ts (2)
4-4: Good addition to test API.Importing
detectKindsInCodeenables direct testing of the detection patterns, which is appropriate for validating the regex fix.
204-231: Excellent test coverage for the security fix.These tests thoroughly validate the bug fix and prevent regressions:
- Lines 204-213: Directly test the reported issue (whitespace/newlines between
.andhandler)- Lines 215-222: Validate the complementary improvement (direct
createServerFn()detection)- Lines 224-231: Prevent false positives on similarly-named functions like
createServerFnExample()The test cases cover critical edge cases including various whitespace combinations (
\n,\t, multiple spaces) that could appear in reformatted code.
|
View your CI Pipeline Execution ↗ for commit 016fc61
☁️ Nx Cloud last updated this comment at |
fb7444c to
016fc61
Compare
with a server function like so:
for whatever reason, vite sometimes passes the transformed code to the start compiler like so:
when there's whitespace between
.andhandler(, the compiler doesn't detect the server function and this can leak server codeSummary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.