fix(voice): strip padding from packets and add guards#11449
fix(voice): strip padding from packets and add guards#11449kodiakhq[bot] merged 4 commits intodiscordjs:mainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #11449 +/- ##
=======================================
Coverage 31.61% 31.61%
=======================================
Files 387 388 +1
Lines 13995 14004 +9
Branches 1100 1105 +5
=======================================
+ Hits 4424 4427 +3
- Misses 9437 9440 +3
- Partials 134 137 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
📝 WalkthroughWalkthroughAdds RTP packet validation and RFC3550 padding stripping to VoiceReceiver. The changes include guard clauses to reject non-voice packets and non-RTP version 2 packets early in the encryption path, and post-decryption padding removal based on RTP header flags. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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.
Actionable comments posted: 2
🤖 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/voice/src/receive/VoiceReceiver.ts`:
- Line 190: Extract the hardcoded Opus payload byte (0x78) into a shared
constant (e.g., RTP_OPUS_PAYLOAD_TYPE) and replace the literal check in
VoiceReceiver (the conditional using msg[1] !== 0x78) with that constant; also
update the corresponding usage in Networking.ts to reference the same constant
so both modules use a single source of truth and avoid protocol divergence.
Ensure the constant is exported from a shared module (e.g., util/Constants or
similar) and imported where needed.
- Around line 188-194: The payload-type check in VoiceReceiver.ts is brittle
because it compares msg[1] directly to 0x78 and ignores the RTP marker bit;
change the check to mask out the marker bit (e.g., compute payloadType = msg[1]
& 0x7F and compare payloadType !== 0x78) so packets with the M bit set (0xF8)
are still accepted, keeping the existing RTP version guard that reads rtpVersion
= msg[0] >> 6. Ensure you update the conditional that currently reads `if
(msg[1] !== 0x78) return;` to use the masked payloadType instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: df7e2abb-3ba2-4de8-ab38-aecc42d0a0e1
📒 Files selected for processing (1)
packages/voice/src/receive/VoiceReceiver.ts
📜 Review details
⏰ 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). (1)
- GitHub Check: Tests
🔇 Additional comments (1)
packages/voice/src/receive/VoiceReceiver.ts (1)
140-147: Padding stripping implementation looks correct for RFC3550 compliance.The logic correctly:
- Reads the padding flag from the unencrypted RTP header (
buffer[0])- Reads the padding count from the last byte of the decrypted payload (
packet)- Guards against malformed packets where
paddingAmount >= packet.lengthOne minor consideration: per RFC3550 section 5.1, when the padding bit is set, the padding count must be at least 1 (since it includes itself). If
paddingAmountis 0 with padding flag set, this indicates a malformed packet. The current code handles this gracefully (no-op), but you may want to add explicit validation or logging for protocol compliance.
Packets from Discord were not being stripped from padding according to RFC3550. This also adds some guards so that ONLY voice packets are handled and that the RTP version must be 2 in order to be parsed.
Fixes #11419 and all other linked issues.