fix(core): fix websockets

[NathanFlurry]

No description provided.

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Preview	Comments	Updated (UTC)
rivetkit-express	Ready	Preview	Comment	Sep 15, 2025 8:55pm

[claude]

Claude finished @NathanFlurry's task —— View job

---

PR Review: fix(core): fix websockets

Reviewing this websocket fix PR and providing feedback on code quality, potential bugs, performance, security, and test coverage.

Todo List:

• Read and analyze the changed files
• Review code quality and best practices
• Check for potential bugs or issues
• Evaluate performance considerations
• Assess security concerns
• Review test coverage
• Provide comprehensive feedback

Changes Summary:

File 1: packages/rivetkit/src/manager/router.ts (+10/-3 lines)

• Added header extraction for WebSocket connections (X-RivetKit-Encoding, X-RivetKit-Conn-Params, X-RivetKit-Auth-Data)
• Added fallback to lowercase headers (x-rivet-encoding, x-rivet-conn-params, x-rivet-auth-data)
• Updated managerDriver.proxyWebSocket call to include these extracted parameters

File 2: packages/rivetkit/src/remote-manager-driver/actor-websocket-client.ts (+3/-0 lines)

• Added x-rivet-port header with value "main" in buildGuardHeadersForWebSocket

---

Review Feedback

✅ Code Quality & Best Practices

• Good: Header extraction logic follows the existing pattern in the codebase
• Good: Fallback to lowercase headers improves compatibility with various WebSocket clients
• Good: Uses existing constants for header names in actor-websocket-client.ts
• Good: Consistent with RivetKit naming conventions and structure

⚠️ Potential Issues

1. JSON Parsing Without Error Handling (router.ts:119-120)

   connParams ? JSON.parse(connParams) : undefined,
   authData ? JSON.parse(authData) : undefined,

   • Risk: If headers contain malformed JSON, this will throw uncaught exceptions
   • Recommendation: Add try-catch blocks or use a safe JSON parser

2. Type Safety Concern (router.ts:118)

   encoding as any, // Will be validated by driver

   • Issue: Type casting defeats TypeScript's safety guarantees
   • Recommendation: Use proper type validation (e.g., with Zod schema) instead of as any

3. Missing Port Header in Router (router.ts)

   • The client now sends x-rivet-port: "main" but the router doesn't extract or validate this header
   • Risk: Inconsistency between client expectations and server handling

🔒 Security Considerations

• Medium Risk: JSON parsing of connParams and authData without validation could lead to prototype pollution or DoS attacks with deeply nested objects
• Recommendation: Validate JSON structure and depth before parsing

⚡ Performance Considerations

• Good: Header extraction is minimal overhead
• Good: JSON parsing only happens when headers are present
• Minor: Multiple header lookups could be optimized by caching header object

🧪 Test Coverage

• Existing Coverage: Found comprehensive WebSocket tests in examples (raw-websocket-handler-proxy/tests/)
• Gap: No specific tests for the new header extraction logic
• Recommendation: Add unit tests to verify header parsing behavior, especially error cases

📝 Suggestions for Improvement

1. Add Safe JSON Parsing (router.ts:103-121):

   const safeParseJson = (value: string | undefined) => {
     if (!value) return undefined;
     try {
       return JSON.parse(value);
     } catch (error) {
       logger().warn("invalid json in header", { value, error });
       return undefined;
     }
   };
   
   const connParams = safeParseJson(
     c.req.header("X-RivetKit-Conn-Params") || c.req.header("x-rivet-conn-params")
   );
   const authData = safeParseJson(
     c.req.header("X-RivetKit-Auth-Data") || c.req.header("x-rivet-auth-data")
   );

2. Use Proper Type Validation:

   import { EncodingSchema } from "@/actor/protocol/serde";
   
   const encodingResult = EncodingSchema.safeParse(encoding);
   if (!encodingResult.success) {
     throw new InvalidEncodingError(encoding);
   }

3. Extract Port Header Consistently:

   const port = c.req.header("X-RivetKit-Port") || c.req.header("x-rivet-port") || "main";

✅ Overall Assessment

This is a solid fix that addresses WebSocket header handling inconsistencies. The changes are minimal, focused, and follow existing patterns. The main concerns are around error handling and type safety, which are important for production reliability.

Recommendation: Approve with minor improvements - Consider implementing the JSON parsing safety measures before merging.

---

[NathanFlurry]

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

• fix(core): fix websockets #1229 👈 (View in Graphite)
• fix(inspector): make inspector work again #1228
• chore(core): remove manager driver #1227
• fix(core): fix issue deserializing WebSocket message with BARE #1226
• chore(core): fix simple engine test #1225
• chore(core): simplify engine keys #1224
• chore(core): remove engine runner addresses config #1223
• chore(core): add welcome message #1222
• chore(fs): remove unneeded warning for onRunnerStart #1221
• chore(core): add readme #1220
• chore(core): add logger to engine runner #1219
• chore(core): update engine runner #1218
• main

---

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

• merge queue - adds this PR to the back of the merge queue
• hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

[graphite-app]

Inconsistent CORS middleware application. The root route handler manually applies CORS middleware, but this creates inconsistent behavior since CORS is already applied globally in the proxy middleware. This could lead to duplicate CORS headers or conflicts. Remove the manual 'cors' parameter from this route handler.

Suggested change

	router.get("/", cors, (c) => {
	router.get("/", (c) => {

Spotted by Diamond



Is this helpful? React 👍 or 👎 to let us know.