-
Notifications
You must be signed in to change notification settings - Fork 133
chore(rivetkit): add required zod validation for json encoding #3431
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
|
The latest updates on your projects. Learn more about Vercel for GitHub.
3 Skipped Deployments
|
Code Review: Add Required Zod Validation for JSON EncodingSummaryThis PR adds Zod schema validation to the JSON encoding path in RivetKit's serialization/deserialization flow. The changes ensure type safety when using JSON encoding for client-server communication, while maintaining backward compatibility with CBOR and bare encodings. Positive Aspects✅ Strong Type Safety: The new Zod schemas provide runtime validation for JSON-encoded messages, catching malformed data early Issues & Concerns🔴 Critical: ArrayBuffer Schema Validation IssueLocation: const ArrayBufferSchema = z.instanceof(ArrayBuffer);Problem: This schema will fail validation for JSON-encoded data because:
Expected flow: But validation happens on the raw parsed JSON (where ArrayBuffers are still Impact: All JSON-encoded messages with ArrayBuffer fields will fail validation, breaking JSON encoding completely. Recommended Fix: The schemas should match the intermediate JSON representation: const ArrayBufferSchema = z.tuple([z.literal(""), z.string()]);
const UintSchema = z.tuple([z.literal(""), z.string()]);Or alternatively, validate after custom parsing, not before.
|
More templates
@rivetkit/actor
@rivetkit/cloudflare-workers
@rivetkit/core
@rivetkit/db
@rivetkit/framework-base
@rivetkit/next-js
@rivetkit/react
rivetkit
@rivetkit/sql-loader
@rivetkit/engine-runner
@rivetkit/engine-runner-protocol
commit: |
PR Review: Add Required Zod Validation for JSON EncodingOverviewThis PR adds Zod schema validation for JSON encoding in RivetKit's client-server protocol, ensuring type safety and runtime validation when using JSON as the encoding format. The implementation is generally solid but there are a few concerns to address. Strengths
Issues and Concerns1. Security: Use of z.any() in API Utils (Priority: High)Location: src/remote-manager-driver/api-utils.ts:55-56 Using z.any() completely bypasses validation, defeating the purpose of this PR. This creates a security and data integrity risk. Recommendation: Replace with z.unknown() at minimum, or better yet, define proper Zod schemas for the API endpoints that use this utility. 2. ArrayBuffer Schema Validation (Priority: Medium)Location: src/actor/client-protocol-schema-json/mod.ts:4 z.instanceof(ArrayBuffer) may not work correctly across different execution contexts or after JSON serialization where ArrayBuffers are converted to base64 strings. Consider using a custom Zod schema that validates the array format that jsonStringifyCompat produces. 3. Missing Test Coverage (Priority: Medium)No new tests were added for the Zod validation logic. Tests should verify that validation catches malformed JSON messages, invalid discriminated union tags, missing required fields, and wrong types for bigint/ArrayBuffer. 4. Error Handling (Priority: Low)Using .parse() throws errors synchronously. Consider using .safeParse() and handling validation errors more gracefully with better context about what was being validated. 5. Type Complexity (Priority: Low)The dual generic types add complexity. Add JSDoc comments explaining when T !== TJson and why, documenting that TJson represents the JSON-serializable form while T represents the internal representation. Additional Observations
Checklist for Author
VerdictApprove with changes requested. The core implementation is solid and adds valuable type safety. However, the use of z.any() in API utils undermines the security benefits and should be addressed before merging. The missing tests are also concerning for such a critical change to the serialization layer. Once the z.any() issue is resolved and basic tests are added, this will be a strong improvement to the codebase. |
d27a9f0 to
be3dc7d
Compare
Code Review - PR #3431This PR adds Zod schema validation for JSON-encoded protocol messages in RivetKit. Positive Aspects
Critical Issues
Recommendations Before MergeCritical:
Important:
See full details in src/serde.ts, src/actor/protocol/serde.ts, and src/remote-manager-driver/api-utils.ts |
51507e2 to
a8f3a9c
Compare
be3dc7d to
1784d8f
Compare
|
Deployment failed with the following error: Learn More: https://vercel.com/docs/limits#rate-limits |
Code Review - PR 3431: Add Required Zod Validation for JSON EncodingSummaryThis PR adds Zod schema validation for JSON encoding/decoding in the RivetKit TypeScript client protocol. The changes introduce runtime type validation when using JSON encoding, while leaving CBOR and bare encodings unvalidated. Strengths
Issues and Concerns1. Performance Impact Not Assessed (Severity: Medium)The PR adds Zod validation to every JSON serialization/deserialization call, which introduces runtime overhead. For high-frequency message passing:
Recommendation: Add benchmarks, consider development-only validation mode, and document performance implications. 2. Inconsistent Validation Across Encodings (Severity: Medium)Only JSON encoding gets validation, while CBOR and bare encodings remain unvalidated. Either document why JSON needs validation but CBOR doesn't, or add validation for all encodings. 3. z.any() Defeats the Purpose (Severity: High)In remote-manager-driver/api-utils.ts:55-56, using z.any() completely bypasses validation. Consider defining proper schemas or making the schema parameter optional. 4. ArrayBuffer Validation May Fail (Severity: High)The schema uses z.instanceof(ArrayBuffer) but validation may run before jsonParseCompat converts base64 strings back to ArrayBuffers. Need integration tests to verify the order of operations in the deserialization pipeline. 5. Missing Test Coverage (Severity: Medium)No tests verify validation errors, malformed data rejection, or integration with the full pipeline. Testing RecommendationsBefore merging:
Summary RecommendationRequest Changes - The core idea is sound, but needs:
The PR improves type safety which is valuable, but the issues above need addressing to ensure correctness and avoid performance regressions. |
Merge activity
|

No description provided.