fix(security): bound RESP processing and pipeline queues - #404
Merged
Conversation
Record the approved parser resource-limit design, TDD execution steps, and the transition from merged PR #388 to the RESP-LIMITS-001 implementation card. Constraint: This commit changes planning and recovery-facing documentation only; runtime behavior remains unchanged. Confidence: High; the scope follows exact main source inspection and Redis 8.8.1 parser evidence. Scope-risk: Low; only task-specific design, plan, STATE, and KANBAN files are included. Tested: git diff --cached --check; placeholder scan; Requirement and task identifier scan. Not-tested: Runtime tests are deferred to the implementation commit. Co-authored-by: OmX <omx@oh-my-codex.dev>
Reject aggregate declarations above the Redis 8.8.1 INT_MAX boundary and cap initial Array, Map, Set, and Push capacity at 1024 entries so unauthenticated input cannot directly request an unbounded Vec allocation. Add regression coverage for exact boundary overflow, i64 capacity overflow, and the maximum accepted declaration without changing existing null or incomplete-frame semantics. Constraint: This commit changes only the RESP parser and its in-file tests; bulk payload and connection buffer limits remain separate work. Confidence: High; the original test failed with capacity overflow and passed after the shared capacity guard was applied. Scope-risk: Low; normal aggregate parsing is unchanged and all four allocation sites use one private helper. Tested: Windows and WSL cargo test -p resp; Windows and WSL target Clippy; cargo fmt --all -- --check; git diff --check. Not-tested: Full workspace and process-level network suites were not rerun because no network, storage, Cargo, or server code changed. Co-authored-by: OmX <omx@oh-my-codex.dev>
|
Warning Review limit reached
Next review available in: 39 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis change adds bounded RESP parsing, authentication-aware network buffering, parser command-copy cleanup, bounded pipeline admission, shared timeout handling, regression coverage, and updated design and integration records. ChangesBounded request processing
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ConnectionHandler
participant parse_client_request
participant RespParse
participant CommandPipeline
Client->>ConnectionHandler: Send RESP bytes
ConnectionHandler->>parse_client_request: Pass parser, auth state, and bytes
parse_client_request->>RespParse: Parse with resource limits
RespParse-->>parse_client_request: Return parsed command or error
parse_client_request-->>ConnectionHandler: Return request result
ConnectionHandler->>CommandPipeline: Admit command
CommandPipeline-->>ConnectionHandler: Return response or timeout
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
Reject aggregate nesting beyond 128 levels before entering Array, Map, Set, or Push parsing so capped per-layer allocations and the recursive call stack cannot still grow without bound. Add deterministic coverage for the 1024-entry capacity policy and the 128/129 nesting boundary, and correct the implementation plan exact-test commands and validation record. Constraint: Bulk payload limits and total per-connection parser buffer limits remain separate work; no network, authentication, Cargo, storage, or command behavior is changed. Confidence: High; the new nesting regression failed on the previous PR Head, passed after the shared depth guard, and an independent second review found no remaining finding. Scope-risk: Low; the depth is threaded only through the four existing recursive aggregate parsers and the guard returns through the existing protocol-error path. Tested: Windows and WSL cargo test -p resp (65 unit, 20 integration); Windows and WSL target Clippy; cargo fmt --all -- --check; git diff --check. Not-tested: Full workspace and process-level network suites were not rerun because no network, storage, Cargo, server, or authentication code changed. Co-authored-by: OmX <omx@oh-my-codex.dev>
Constraint: Bound unauthenticated RESP parsing and optional pipeline resources without changing Redis command, storage, or Raft semantics. Confidence: High. Parser budgets, every active network consumer, legacy queue draining, and bounded pipeline admission have direct regression coverage. Scope-risk: Oversized or excessively expensive RESP frames now fail closed; queue admission shares the existing 30-second timeout. Red: Baseline parsing trusted attacker-controlled lengths, replayed incomplete aggregates without a cumulative budget, retained legacy command copies, and used an unbounded pipeline channel. Green: Resource-limit, parser-consumer, pipeline, and Linux TCP isolation regressions pass. Tested: Windows MSVC cargo test -p resp --all-features --locked; cargo test -p net --lib --all-features --locked; strict resp/net Clippy; fmt check; diff check. WSL/Linux exact oversized unauthenticated request and protocol-error isolation tests. Not-tested: Full WSL workspace suite. The earlier Windows workspace run reached all unit suites but retained the repository baseline of 19 TCP startup failures. Co-authored-by: OmX <omx@oh-my-codex.dev>
AlexStocks
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens Kiwi’s network-facing request ingestion by adding explicit resource limits to RESP parsing, enforcing a stricter pre-authentication buffering cap at the network boundary, draining legacy parser command history to avoid per-connection growth, and making the optional command pipeline’s queue truly bounded with backpressure.
Changes:
- Added RESP parser resource limits (inline/header length, bulk length, total buffered frame, aggregate length/depth, decoded-node budget, and cumulative incomplete-frame work budget) with targeted tests.
- Enforced a 1 MiB pre-authentication buffered-bytes cap in the active network parse path and unified active consumers behind shared parse helpers that also drain the legacy
next_command()queue. - Replaced the pipeline’s unbounded channel with a bounded Tokio mpsc channel (capacity normalized to ≥ 1) and added tests for bounded admission behavior.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/resp/src/parse.rs | Introduces RespLimits and enforces parse-time resource limits; adds extensive regression tests. |
| src/net/src/network_handle.rs | Adds shared parsing helpers, enforces pre-auth buffer cap, and drains legacy parsed-command copies. |
| src/net/src/optimized_handler.rs | Routes parsing through the new shared helper to enforce consistent limits and legacy-queue draining. |
| src/net/src/handle.rs | Routes parsing through the new shared helper to enforce consistent limits and legacy-queue draining. |
| src/net/src/async_resp_parser.rs | Uses shared parse helper to ensure legacy command copies are drained in this consumer too. |
| src/net/src/pipeline.rs | Switches to a bounded command channel and includes queue admission in timeout handling; adds bounded-channel tests. |
| src/net/tests/storage_command_e2e_tests.rs | Adds an e2e TCP test to validate oversized unauthenticated buffering is rejected without harming healthy connections. |
| docs/superpowers/specs/2026-07-31-resp-parser-resource-limits-design.md | Design record for RESP aggregate resource limiting (historical/archival per header note). |
| docs/superpowers/specs/2026-07-31-bounded-request-processing-design.md | Consolidated design for bounded request processing across parser + network + pipeline. |
| docs/superpowers/plans/2026-07-31-resp-parser-resource-limits.md | Implementation plan record for the earlier narrower scope (superseded per header note). |
| docs/superpowers/plans/2026-07-31-bounded-request-processing.md | Consolidated implementation plan record for the final scope. |
| .planning/STATE.md | Updates project state to reflect PR #404 scope, boundaries, and validation evidence. |
| .planning/KANBAN.md | Updates Kanban to reflect the current in-progress implementation card and recent merges. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
6 tasks
AlexStocks
added a commit
that referenced
this pull request
Jul 31, 2026
Update the active branch, PR number, publication history, and final-state instructions after the narrow verified fix moved to PR #406 instead of overwriting externally updated PR #404. Constraint: This commit changes only STATE and KANBAN publication records. Confidence: High; PR #406 and its initial Head were queried directly from GitHub. Scope-risk: None for runtime behavior; the two overlapping PRs still require maintainer reconciliation. Tested: cargo fmt --all -- --check; git diff --check; gh pr view 406. Not-tested: Runtime suites were not rerun because this commit changes planning documents only. Co-authored-by: OmX <omx@oh-my-codex.dev>
AlexStocks
added a commit
that referenced
this pull request
Jul 31, 2026
* docs(planning): define RESP allocation guardrails Record the approved parser resource-limit design, TDD execution steps, and the transition from merged PR #388 to the RESP-LIMITS-001 implementation card. Constraint: This commit changes planning and recovery-facing documentation only; runtime behavior remains unchanged. Confidence: High; the scope follows exact main source inspection and Redis 8.8.1 parser evidence. Scope-risk: Low; only task-specific design, plan, STATE, and KANBAN files are included. Tested: git diff --cached --check; placeholder scan; Requirement and task identifier scan. Not-tested: Runtime tests are deferred to the implementation commit. Co-authored-by: OmX <omx@oh-my-codex.dev> * fix(resp): bound aggregate parser allocations Reject aggregate declarations above the Redis 8.8.1 INT_MAX boundary and cap initial Array, Map, Set, and Push capacity at 1024 entries so unauthenticated input cannot directly request an unbounded Vec allocation. Add regression coverage for exact boundary overflow, i64 capacity overflow, and the maximum accepted declaration without changing existing null or incomplete-frame semantics. Constraint: This commit changes only the RESP parser and its in-file tests; bulk payload and connection buffer limits remain separate work. Confidence: High; the original test failed with capacity overflow and passed after the shared capacity guard was applied. Scope-risk: Low; normal aggregate parsing is unchanged and all four allocation sites use one private helper. Tested: Windows and WSL cargo test -p resp; Windows and WSL target Clippy; cargo fmt --all -- --check; git diff --check. Not-tested: Full workspace and process-level network suites were not rerun because no network, storage, Cargo, or server code changed. Co-authored-by: OmX <omx@oh-my-codex.dev> * fix(resp): bound aggregate nesting depth Reject aggregate nesting beyond 128 levels before entering Array, Map, Set, or Push parsing so capped per-layer allocations and the recursive call stack cannot still grow without bound. Add deterministic coverage for the 1024-entry capacity policy and the 128/129 nesting boundary, and correct the implementation plan exact-test commands and validation record. Constraint: Bulk payload limits and total per-connection parser buffer limits remain separate work; no network, authentication, Cargo, storage, or command behavior is changed. Confidence: High; the new nesting regression failed on the previous PR Head, passed after the shared depth guard, and an independent second review found no remaining finding. Scope-risk: Low; the depth is threaded only through the four existing recursive aggregate parsers and the guard returns through the existing protocol-error path. Tested: Windows and WSL cargo test -p resp (65 unit, 20 integration); Windows and WSL target Clippy; cargo fmt --all -- --check; git diff --check. Not-tested: Full workspace and process-level network suites were not rerun because no network, storage, Cargo, server, or authentication code changed. Co-authored-by: OmX <omx@oh-my-codex.dev> * fix(resp): avoid declaration-driven aggregate allocation Start aggregate containers empty so incomplete untrusted declarations cannot reserve element storage. Keep the Redis INT_MAX length check and the 128-level nesting guard, and measure parser-phase allocations in regression tests. Constraint: Scope remains limited to Issue #395 B1 and the five authorized RESP task paths. Confidence: High; the allocation regressions failed on the previous Head and pass with zero parser-phase allocation on Windows and WSL. Scope-risk: Actual payload size and total connection-buffer limits remain separate follow-up work. Tested: Windows and WSL cargo test -p resp; Windows and WSL cargo clippy -p resp --all-targets -- -D warnings -D clippy::unwrap_used; cargo fmt --all -- --check; git diff --check. Not-tested: Full workspace and native RocksDB suites because this change is confined to the resp crate. Co-authored-by: OmX <omx@oh-my-codex.dev> * docs(planning): record independent RESP allocation PR Update the active branch, PR number, publication history, and final-state instructions after the narrow verified fix moved to PR #406 instead of overwriting externally updated PR #404. Constraint: This commit changes only STATE and KANBAN publication records. Confidence: High; PR #406 and its initial Head were queried directly from GitHub. Scope-risk: None for runtime behavior; the two overlapping PRs still require maintainer reconciliation. Tested: cargo fmt --all -- --check; git diff --check; gh pr view 406. Not-tested: Runtime suites were not rerun because this commit changes planning documents only. Co-authored-by: OmX <omx@oh-my-codex.dev> --------- Co-authored-by: OmX <omx@oh-my-codex.dev>
Resolve PR #404 against main after PRs #402, #403, #405, and #406 while preserving RESP resource budgets, declaration-independent aggregate growth, bounded pipeline admission, and one shared request deadline. Constraint: Preserve the verified PR #404 scope and publish without force-push Tested: Windows resp 80 unit and 20 integration tests; net 35 lib tests; WSL resp and net suites, TCP regressions, strict Clippy; cargo fmt and diff checks Co-authored-by: OmX <omx@oh-my-codex.dev>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 @.planning/STATE.md:
- Line 148: Update the wording in the PR `#383` planning-history entry to
explicitly state that the frozen Oracle draft contains six files, replacing the
ambiguous “旧六文件 Oracle 草稿” phrase while preserving the rest of the scope and
meaning.
In `@docs/superpowers/plans/2026-07-31-resp-parser-resource-limits.md`:
- Around line 3-5: Keep the status notes in one continuous blockquote by adding
a quoted blank line before the next quoted paragraph in
docs/superpowers/plans/2026-07-31-resp-parser-resource-limits.md lines 3-5 and
docs/superpowers/specs/2026-07-31-resp-parser-resource-limits-design.md lines
3-5; alternatively remove the blank line at both sites.
In `@src/net/tests/storage_command_e2e_tests.rs`:
- Around line 484-490: Update the oversized unauthenticated payload write in the
test to ignore its result instead of panicking on expected BrokenPipe or
ConnectionReset errors. Preserve the subsequent read assertion as the check that
verifies the server closed the connection.
In `@src/resp/src/parse.rs`:
- Around line 593-625: Adjust the incremental parsing flow centered on
parse_resp_data and parse_array so already-decoded elements are not charged
repeatedly when process_buffer re-parses an incomplete frame. Track node/work
accounting from the current high-water mark, or derive the limit from buffered
bytes and max_decode_nodes, while still enforcing genuine payload resource
limits and accepting valid fragmented multi-bulk commands.
🪄 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 Plus
Run ID: 393843f3-5ccf-41d6-aee4-7c60609bca4f
📒 Files selected for processing (13)
.planning/KANBAN.md.planning/STATE.mddocs/superpowers/plans/2026-07-31-bounded-request-processing.mddocs/superpowers/plans/2026-07-31-resp-parser-resource-limits.mddocs/superpowers/specs/2026-07-31-bounded-request-processing-design.mddocs/superpowers/specs/2026-07-31-resp-parser-resource-limits-design.mdsrc/net/src/async_resp_parser.rssrc/net/src/handle.rssrc/net/src/network_handle.rssrc/net/src/optimized_handler.rssrc/net/src/pipeline.rssrc/net/tests/storage_command_e2e_tests.rssrc/resp/src/parse.rs
Allow the unauthenticated buffer-limit regression to accept an expected mid-write disconnect, clarify the frozen planning scope, and keep related status notes in valid Markdown blockquotes. Constraint: Preserve cumulative parser replay-work accounting and its CPU-denial-of-service boundary Tested: WSL oversized-request exact test; Windows target Clippy; cargo fmt and diff checks; parser replay-work exact test Co-authored-by: OmX <omx@oh-my-codex.dev>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
i32::MAX, nesting at 128, initial/incremental reservations at 1024, decoded nodes at 65,536, and cumulative incomplete-frame work at 1,000,000 node visitsRespParseconsumer through shared helpers and drain the legacynext_command()copy after complete framesIssue scope
Refs #395 (B1 only).
Refs #398.
This PR does not close #395. A source-level call-path audit did not support the issue's B2/B3/B4 P0 descriptions for the current code: normal commands are already serialized by
STORAGE_EXCLUSIVE, Raft apply is a single writer, and expiration currently sends no-op compaction work. This PR intentionally does not add record locks that could create a real Raft apply deadlock.#398 was manually closed while PR #403 remains open and independently changes
src/net/src/pipeline.rs. This PR does not claim to close #398. It contains the consolidated, tested queue implementation with normalized capacity, bounded admission, and timeout behavior; if #403 lands first, the overlapping implementation must be reconciled before #404 can merge.Requirements:
REQ-COMPAT-002,REQ-COMPAT-006,REQ-STABILITY-002,REQ-STABILITY-003,REQ-WORK-003.Validation
Windows MSVC, Rust 1.97.1:
cargo test -p resp --all-features --locked: 71 unit + 20 integration + doc tests, 0 failedcargo test -p net --lib --all-features --locked: 32 passed, 0 failedcargo clippy -p resp -p net --all-targets --all-features --locked -- -D warnings -D clippy::unwrap_used: exit 0cargo fmt --all -- --check: exit 0git diff --check: exit 0WSL/Linux, Rust 1.97.1, independent Linux target:
network_server_rejects_oversized_unauthenticated_request: 1 passed, 0 failednetwork_server_protocol_error_only_closes_the_bad_connection: 1 passed, 0 failedAn earlier Windows
cargo test --workspace --all-features --lockedrun on the same consolidated source reached all unit suites successfully. Its TCP collection had 4 passes and 19 failures, all with the repository's Windows baselineserver did not become connectable/connect-timeout symptom. The two changed/control TCP cases above pass under WSL/Linux. A full WSL workspace suite was not run.CI results must be evaluated against final Head
334a235a95c50ca1cdd71927e459a2c6ac5e5bb0; older green checks do not establish the final merge state. This PR has not been merged.Summary by CodeRabbit
New Features
Bug Fixes
Tests