fix: validate server fn id during dev #6087
Conversation
WalkthroughIntroduces runtime validation for server function IDs via a virtual module and parsing helper, injects a dynamic import of that validator into the dev-mode getServerFnById flow, relaxes the directiveFnsById update guard to allow multiple updates, and adds assertion-based guards in the validation loader. Changes
Sequence Diagram(s)sequenceDiagram
participant DevImporter as Importer (dev code)
participant DevServer as Vite Dev Server / Plugin
participant VirtualModule as validate:server-fn-id?query=...
participant InMemoryMap as directiveFnsById
DevImporter->>DevServer: request getServerFnById(id)
DevServer-->>DevServer: construct validate virtual-module id (with query)
DevServer->>VirtualModule: dynamic import validate module (id)
VirtualModule->>InMemoryMap: check if id exists
alt id found
VirtualModule-->>DevServer: return empty export (validation passed)
DevServer-->>DevImporter: proceed to load target server fn
else id missing
VirtualModule-->>DevServer: throw error (invalid server fn id)
DevServer-->>DevImporter: propagate error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (2)**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{js,ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧬 Code graph analysis (1)packages/server-functions-plugin/src/index.ts (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (3)
Comment |
|
View your CI Pipeline Execution ↗ for commit f3c7ab2
☁️ Nx Cloud last updated this comment at |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/server-functions-plugin/src/index.ts (1)
224-230: Avoid relying on'/@id/__x00__'if possible; at least document why it’s required.
await import(/* @vite-ignore */ '/@id/__x00__' + validateIdImport)(Line 226) bakes in a Vite dev-server internal path encoding. If you can insteadresolveIdthe virtual module (or otherwise makeimport(validateIdImport)resolvable without the/@id/__x00__prefix), the validation mechanism will be less coupled to Vite internals. If that’s not possible in this plugin architecture, a short comment explaining the necessity would help prevent future “cleanup” regressions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/server-functions-plugin/src/index.ts(4 hunks)
🧰 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/server-functions-plugin/src/index.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Implement ESLint rules for router best practices using the ESLint plugin router
Files:
packages/server-functions-plugin/src/index.ts
🪛 ast-grep (0.40.0)
packages/server-functions-plugin/src/index.ts
[warning] 170-172: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(
^${resolveViteId(validateServerFnIdVirtualModule)}?.+,
)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test
- GitHub Check: Preview
| const validateServerFnIdVirtualModule = `virtual:tanstack-start-validate-server-fn-id` | ||
|
|
||
| function parseIdQuery(id: string): { | ||
| filename: string | ||
| query: { | ||
| [k: string]: string | ||
| } | ||
| } { | ||
| if (!id.includes('?')) return { filename: id, query: {} } | ||
| const [filename, rawQuery] = id.split(`?`, 2) as [string, string] | ||
| const query = Object.fromEntries(new URLSearchParams(rawQuery)) | ||
| return { filename, query } | ||
| } |
There was a problem hiding this comment.
parseIdQuery is fine, but it makes “missing id” look like an assertion failure.
parseIdQuery never returns falsy, so assert(parsed) (Line 177) is redundant, and assert(parsed.query.id) will throw an AssertionError instead of a clear Vite error. Prefer an explicit check and this.error('Missing ...') for consistency with the invalid-id path.
🤖 Prompt for AI Agents
In packages/server-functions-plugin/src/index.ts around lines 43 to 55, remove
the redundant assert(parsed) usage and replace the assertion that
parsed.query.id exists with an explicit runtime check that calls
this.error('Missing id in virtual module request') (or a message matching the
invalid-id path) when parsed.query.id is absent; since parseIdQuery always
returns an object, rely on that and use this.error to raise a clear Vite error
instead of throwing an AssertionError.
| { | ||
| name: 'tanstack-start-server-fn-vite-plugin-validate-serverfn-id', | ||
| apply: 'serve', | ||
| load: { | ||
| filter: { | ||
| id: new RegExp( | ||
| `^${resolveViteId(validateServerFnIdVirtualModule)}?.+`, | ||
| ), | ||
| }, | ||
| handler(id) { | ||
| const parsed = parseIdQuery(id) | ||
| assert(parsed) | ||
| assert(parsed.query.id) | ||
| if (directiveFnsById[parsed.query.id]) { | ||
| return `export {}` | ||
| } | ||
| this.error(`Invalid server function ID: ${parsed.query.id}`) | ||
| }, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Fix the load.filter.id regex pattern: the quantifier ? is positioned incorrectly and unescaped strings are unsafe in RegExp.
The current pattern `^${resolveViteId(validateServerFnIdVirtualModule)}?.+` makes the last character of the resolved ID optional rather than making the query string optional. This is a logic error that can silently match unintended IDs or fail to match valid ones. The unescaped dynamic string is also brittle; if special regex characters appear, the pattern breaks silently.
Additionally, the assertions at lines 177–178 (assert(parsed) and assert(parsed.query.id)) will throw cryptic errors instead of returning proper validation failures. Use explicit error checks instead:
id: new RegExp(
- `^${resolveViteId(validateServerFnIdVirtualModule)}?.+`,
+ `^${escapeRegExp(resolveViteId(validateServerFnIdVirtualModule))}\\?id=.+$`,
),
handler(id) {
const parsed = parseIdQuery(id)
- assert(parsed)
- assert(parsed.query.id)
- if (directiveFnsById[parsed.query.id]) {
+ const serverFnId = parsed?.query?.id
+ if (!serverFnId) {
+ this.error(`Missing server function ID in: ${id}`)
+ }
+ if (directiveFnsById[serverFnId]) {
return `export {}`
}
- this.error(`Invalid server function ID: ${parsed.query.id}`)
+ this.error(`Invalid server function ID: ${serverFnId}`)
}Also add the escaping helper at the top of the file:
+function escapeRegExp(s: string) {
+ return s.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')
+}Finally: verify that directiveFnsById is populated before this load hook can run in dev mode. If a module's load hook executes before its corresponding resolveId hook completes, all validations will fail. If that's possible, add a guard to detect uninitialized state.
🧰 Tools
🪛 ast-grep (0.40.0)
[warning] 170-172: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(
^${resolveViteId(validateServerFnIdVirtualModule)}?.+,
)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🤖 Prompt for AI Agents
In packages/server-functions-plugin/src/index.ts around lines 166 to 185, the
load.filter.id RegExp is incorrect and unsafe: the `?` is applied to the last
character of the resolved ID instead of making the query part optional, and the
dynamic string must be escaped before insertion into a RegExp to avoid
special-character injection; replace the pattern with a RegExp that escapes the
resolved ID and matches an optional query suffix (e.g. make the query portion
optional as a whole, not the last character). Replace assert(parsed) and
assert(parsed.query.id) with explicit runtime checks that call this.error with a
clear message when parsed or parsed.query.id are missing. Finally add a guard
that ensures directiveFnsById is initialized/populated before validating IDs in
the load hook and, if not ready, either skip validation or return a safe
fallback to avoid false negatives during dev-mode load ordering.
schiller-manuel
force-pushed
the
validate-serverfn-id
branch
from
f24c30a to
f3c7ab2
Compare
Summary by CodeRabbit
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.