Uh oh!
There was an error while loading. Please reload this page.
feat: Add multiplexed RPC stream transport message handling - #40
Conversation
📝 WalkthroughWalkthroughAdds a new ChangesMultiplexRpcTransporter
PHPDoc nullable type standardization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Transporter/MultiplexRpcTransporter.php`:
- Around line 25-31: Add a maximum frame-size validation check in the
MultiplexRpcTransporter.php file after unpacking the length and the existing
check for length < 4. Before calling readBytes with the $length value, add an
additional condition that validates the length does not exceed a protocol-safe
maximum threshold. If the length exceeds this maximum, throw a
RecvFailedException with an appropriate error message indicating the frame size
is too large. This prevents malicious or buggy peers from forcing excessive
memory consumption by advertising unreasonably large frame sizes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ce7594d9-ef51-4036-8d67-39ce5c2e3182
📒 Files selected for processing (1)
src/Transporter/MultiplexRpcTransporter.php
| $unpacked = unpack('Nlength', $header); | ||
| $length = $unpacked['length']; | ||
| if ($length < 4) { | ||
| throw new RecvFailedException(sprintf('Invalid package length: %d', $length)); | ||
| } | ||
| $body = $this->readBytes($length); |
There was a problem hiding this comment.
Add a maximum frame-size guard before reading the body.
Line 31 reads a peer-declared $length with no upper bound. A malicious or buggy peer can advertise a huge size and force excessive memory/read pressure before failure. Please reject frames above a protocol-safe max.
Suggested patch
class MultiplexRpcTransporter extends StreamSocketTransporter
{
public const PING = 'ping';
public const PONG = 'pong';
+ private const MAX_FRAME_LENGTH = 8 * 1024 * 1024; // adjust to protocol limit
@@
$unpacked = unpack('Nlength', $header);
$length = $unpacked['length'];
if ($length < 4) {
throw new RecvFailedException(sprintf('Invalid package length: %d', $length));
}
+ if ($length > self::MAX_FRAME_LENGTH) {+ throw new RecvFailedException(sprintf('Package too large: %d', $length));+ }
$body = $this->readBytes($length);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $unpacked = unpack('Nlength', $header); | |
| $length = $unpacked['length']; | |
| if ($length < 4) { | |
| thrownewRecvFailedException(sprintf('Invalid package length: %d', $length)); | |
| } | |
| $body = $this->readBytes($length); | |
| class MultiplexRpcTransporter extends StreamSocketTransporter | |
| { | |
| publicconstPING = 'ping'; | |
| publicconstPONG = 'pong'; | |
| privateconstMAX_FRAME_LENGTH = 8 * 1024 * 1024; // adjust to protocol limit | |
| // ... other class members ... | |
| // In the receive() method or equivalent: | |
| $unpacked = unpack('Nlength', $header); | |
| $length = $unpacked['length']; | |
| if ($length < 4) { | |
| thrownewRecvFailedException(sprintf('Invalid package length: %d', $length)); | |
| } | |
| if ($length > self::MAX_FRAME_LENGTH) { | |
| thrownewRecvFailedException(sprintf('Package too large: %d', $length)); | |
| } | |
| $body = $this->readBytes($length); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Transporter/MultiplexRpcTransporter.php` around lines 25 - 31, Add a
maximum frame-size validation check in the MultiplexRpcTransporter.php file
after unpacking the length and the existing check for length < 4. Before calling
readBytes with the $length value, add an additional condition that validates the
length does not exceed a protocol-safe maximum threshold. If the length exceeds
this maximum, throw a RecvFailedException with an appropriate error message
indicating the frame size is too large. This prevents malicious or buggy peers
from forcing excessive memory consumption by advertising unreasonably large
frame sizes.
Uh oh!
There was an error while loading. Please reload this page.
Co-Authored-By: siam <hzh@addcn.com> Co-Authored-By: Deeka Wong <8337659+huangdijia@users.noreply.github.com>
Summary by CodeRabbit