Mitigate fragment decompression DoS by rejecting lz codec#1
Mitigate fragment decompression DoS by rejecting lz codec#1
Conversation
Deploying agent-render with
|
| Latest commit: |
264d046
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://320d4752.agent-render.pages.dev |
| Branch Preview URL: | https://codex-propose-fix-for-lz-dec.agent-render.pages.dev |
📝 WalkthroughWalkthroughThe pull request removes support for the "lz" (compression) codec from the fragment encoding and decoding system. All encoding now defaults to plain base64 encoding via an updated Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Overview
The PR correctly removes LZ compression support to address the decompression DoS vulnerability (as suggested by the branch name). The changes:
Files Reviewed (2 files)
Other Observations (not in diff)Minor issue found in existing code (not part of this PR):
|
![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)
src/lib/payload/fragment.ts (1)
50-64:⚠️ Potential issue | 🟠 MajorReject oversized plain fragments at encode time.
After removing the compressed path, some bundles that previously fit as
v1.lzwill now serialize pastMAX_FRAGMENT_LENGTH. The currentviewer-shell.tsxcaller writes whatever this function returns, so we can generate links thatdecodeFragment()immediately rejects on reload. Please surface an explicit failure here instead of emitting an undecodable fragment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/payload/fragment.ts` around lines 50 - 64, The encodeEnvelope function must fail fast when a plain fragment would exceed MAX_FRAGMENT_LENGTH instead of returning an undecodable payload; after creating plainFragment via buildFragment(envelope, "plain") check its serialized length against MAX_FRAGMENT_LENGTH and throw a clear Error (or return a rejected result) if it exceeds the limit. Update encodeEnvelope to perform this check (use the existing MAX_FRAGMENT_LENGTH constant) before honoring options.preferCompressed or returning plainFragment so callers like viewer-shell.tsx never receive fragments that decodeFragment() will immediately reject. Ensure the thrown error message references encodeEnvelope and MAX_FRAGMENT_LENGTH for easier debugging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/payload/fragment.ts`:
- Around line 114-120: The branch that handles codec === "lz" should return a
distinct error code instead of "invalid-format" so legacy lz fragments are
distinguishable; update the return value in the codec === "lz" branch in
src/lib/payload/fragment.ts to use a new, specific code (e.g., "legacy-codec" or
"unsupported-legacy-fragment") while keeping a clear message, and update any
tests or callers that rely on the old "invalid-format" code to expect the new
code.
---
Outside diff comments:
In `@src/lib/payload/fragment.ts`:
- Around line 50-64: The encodeEnvelope function must fail fast when a plain
fragment would exceed MAX_FRAGMENT_LENGTH instead of returning an undecodable
payload; after creating plainFragment via buildFragment(envelope, "plain") check
its serialized length against MAX_FRAGMENT_LENGTH and throw a clear Error (or
return a rejected result) if it exceeds the limit. Update encodeEnvelope to
perform this check (use the existing MAX_FRAGMENT_LENGTH constant) before
honoring options.preferCompressed or returning plainFragment so callers like
viewer-shell.tsx never receive fragments that decodeFragment() will immediately
reject. Ensure the thrown error message references encodeEnvelope and
MAX_FRAGMENT_LENGTH for easier debugging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cc1d7cf3-396e-4b7c-b8e8-618d737fc3cc
📒 Files selected for processing (2)
src/lib/payload/fragment.tstests/fragment.test.ts
| if (codec === "lz") { | ||
| return { | ||
| ok: false, | ||
| code: "invalid-format", | ||
| message: 'Unsupported codec "lz". Please re-share using codec "plain".', | ||
| }; | ||
| } |
There was a problem hiding this comment.
Don’t report rejected lz links as generic format errors.
These are legacy fragments generated by older clients, not malformed input. Returning invalid-format makes old #agent-render=v1.lz... bookmarks indistinguishable from genuinely corrupt hashes, so the UI has no reliable way to show a targeted recovery message. Please give this branch its own result code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/payload/fragment.ts` around lines 114 - 120, The branch that handles
codec === "lz" should return a distinct error code instead of "invalid-format"
so legacy lz fragments are distinguishable; update the return value in the codec
=== "lz" branch in src/lib/payload/fragment.ts to use a new, specific code
(e.g., "legacy-codec" or "unsupported-legacy-fragment") while keeping a clear
message, and update any tests or callers that rely on the old "invalid-format"
code to expect the new code.
1 participant
Motivation
Description
v1.lzare rejected up-front and never decompressed bydecodeFragment.encodeEnvelopeto emitplain(base64url) payloads even whencodec: "lz"is requested so generated links remain decodable by current clients.src/lib/payload/fragment.ts.tests/fragment.test.tsto assert plain transport is emitted and to add a test thatv1.lzfragments are rejected.Testing
npm test -- tests/fragment.test.tsand all tests passed (6 passed).npm run lintand ESLint completed without errors.Codex Task
Summary by CodeRabbit