fix(realtime): decode edge cases#2130
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughSerializer.decode now wraps JSON.parse(rawPayload) in try/catch and validates that the parsed value is an array with at least five elements; on parse error it logs "Error parsing Realtime message" and returns an empty object via callback, and on validation failure it logs "Malformed Realtime message" and returns an empty object via callback. Binary decode paths return early (logging "Failed to decode binary Realtime message") when decoding yields undefined. _decodeUserBroadcast and binary decoding now defensively handle JSON parse errors and may return undefined on failure. Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
|
I typically use zod in my own project for validation but not sure the dependency policy here. |
@supabase/auth-js
@supabase/functions-js
@supabase/postgrest-js
@supabase/realtime-js
@supabase/storage-js
@supabase/supabase-js
commit: |
|
Hey @yukimotochern, thanks for the PR! Defensive error handling in the serializer is a good idea. A few things to address before we merge: callback({}) causes a downstream crash: The main issue is that calling callback({}) on a parse error doesn't actually make things safer, it moves the crash downstream. In _onConnMessage, the decoded message is used like this: const { topic, event, payload, ref } = msg
const status = payload.status || '' // TypeError if payload is undefinedSince {} destructures to payload = undefined, you'd get a TypeError there instead. I'd suggest either skipping the callback entirely on bad input (just return after logging), or guarding _onConnMessage against payload being undefined. Binary decode path has the same issue. _decodeUserBroadcast has an unguarded JSON.parse(decoder.decode(payload)) too, worth addressing for consistency while you're in here. Use the client's logger instead of console.error. RealtimeClient has a log() method — using that keeps log handling consistent and respects any logging config the user has set. A couple of unit cases covering malformed JSON and a short array would go a long way here. I have a question regarding the On Zod: Good instinct to flag it. We haven't fully decided on a validation library yet, so the manual checks are fine for now. If/when we adopt something like Zod across the codebase, this would be a natural place to update. |
|
@mandarini fixed |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/realtime-js/src/lib/serializer.ts (1)
210-213:⚠️ Potential issue | 🔴 CriticalUnguarded
JSON.parse(metadata)can still crash on malformed input.The payload parsing at lines 194-199 was correctly wrapped in try/catch, but the metadata parsing at line 212 remains unguarded. If
metadataSize > 0butmetadatacontains invalid JSON, this will throw and crash—defeating the purpose of this PR's defensive handling.Proposed fix
// Metadata is optional and always JSON encoded if (metadataSize > 0) { - data['meta'] = JSON.parse(metadata) + try { + data['meta'] = JSON.parse(metadata) + } catch (error) { + console.error('Error decoding metadata:', error, metadata) + return undefined + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/realtime-js/src/lib/serializer.ts` around lines 210 - 213, The unguarded JSON.parse(metadata) when metadataSize > 0 can throw on malformed JSON; update the metadata parsing in serializer.ts to mirror the earlier payload parsing's defensive pattern by wrapping JSON.parse(metadata) in a try/catch, assigning data['meta'] to the parsed object on success and to a safe fallback (e.g., null or {}) on failure, and log or handle the parse error (using the same logger/format as the payload catch) so a malformed metadata field does not crash the process; locate the code around metadataSize, metadata and data['meta'] to implement this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/realtime-js/src/lib/serializer.ts`:
- Line 197: The error path in serializer.ts currently uses console.log for the
"Error decoding JSON payload:" message which is inconsistent with other error
paths; replace that console.log call with console.error (the call that logs
"Error decoding JSON payload:" along with error and textPayload) so it matches
the other error handling in this module (search for the "Error decoding JSON
payload:" string in serializer.ts to locate the exact statement).
---
Outside diff comments:
In `@packages/core/realtime-js/src/lib/serializer.ts`:
- Around line 210-213: The unguarded JSON.parse(metadata) when metadataSize > 0
can throw on malformed JSON; update the metadata parsing in serializer.ts to
mirror the earlier payload parsing's defensive pattern by wrapping
JSON.parse(metadata) in a try/catch, assigning data['meta'] to the parsed object
on success and to a safe fallback (e.g., null or {}) on failure, and log or
handle the parse error (using the same logger/format as the payload catch) so a
malformed metadata field does not crash the process; locate the code around
metadataSize, metadata and data['meta'] to implement this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: a0484d77-a2cb-4f3f-9537-dee3d4078e9f
📒 Files selected for processing (1)
packages/core/realtime-js/src/lib/serializer.ts
mandarini
left a comment
mandarini
left a comment
There was a problem hiding this comment.
Hey @yukimotochern, thanks for the update, a couple of things still need to be addressed before this is ready:
-
PR title: The title needs to follow the conventional commit format. Please change it from
Fix realtime-js: decode edge casesto follow our guidelines. -
Use the client's logger, not console.error: All the new error paths still call console.error directly. As mentioned in my previous review, please use RealtimeClient's log() method instead. This keeps log handling consistent and respects any logging configuration the user has set.
-
Tests are still missing: Only serializer.ts was changed. Please add unit cases covering at least: malformed JSON input and a valid JSON array with fewer than 5 elements.
yukimotochern
changed the title
3 participants
🔍 Description
What changed?
Why was this change needed?
Closes #(issue_number)
📸 Screenshots/Examples
🔄 Breaking changes
📋 Checklist
<type>(<scope>): <description>npx nx formatto ensure consistent code formatting📝 Additional notes