Skip to content

refactor(network): unify buffered HTTP request handling #2284

Description

@pimlock

Problem Statement

OpenShell currently buffers and normalizes HTTP/1.1 request bodies in several places:

  • REST middleware and body credential rewriting in crates/openshell-supervisor-network/src/l7/rest.rs
  • JSON-RPC and MCP inspection through crates/openshell-supervisor-network/src/l7/http.rs
  • GraphQL inspection in crates/openshell-supervisor-network/src/l7/graphql.rs
  • inference request parsing in crates/openshell-supervisor-network/src/l7/inference.rs

These paths independently parse chunked framing, enforce limits, consume trailers, and rebuild headers. Their behavior has drifted. PR #2027 exposed one example: REST buffering discarded actual trailers but left the Trailer declaration on the normalized request, while JSON-RPC, GraphQL, and inference removed it.

Independent implementations also make it harder to verify consistent handling of malformed framing, chunk extensions, trailers, byte limits, and pipelined request boundaries.

Transparent request relay is a different case: it intentionally preserves chunked wire framing and trailers without buffering or transforming the body. That path should remain separate.

Proposed Design

Introduce a shared buffered-request framing and normalization core with caller-specific adapters:

  1. Define a common chunked decoder/state machine that produces a structured result containing the decoded body, consumed wire bytes, trailers as separate fields, and original framing metadata.
  2. Centralize framing validation and configurable limits, including decoded bytes, wire bytes, chunk count, and trailer line/byte limits.
  3. Centralize header normalization: remove Transfer-Encoding, Trailer, and the old Content-Length; set the decoded Content-Length; and make the trailer policy explicit (drop, reject, or a future field-aware retention mode).
  4. Provide an incremental async adapter for REST, middleware, credential rewriting, JSON-RPC, and GraphQL. Keep caller-specific behavior such as Expect: 100-continue, policy-generation guards, and fail-open recovery outside the shared decoder.
  5. Provide a slice-based adapter for inference, which parses already-buffered bytes and must return the exact number of bytes consumed for keep-alive requests.
  6. Move GraphQL onto the shared HTTP inspection helper where its requirements already match JSON-RPC/MCP.
  7. Add a shared conformance matrix covering content-length bodies, chunk extensions, trailers, malformed or ambiguous framing, limits, incomplete input, pipelined boundaries, and exact consumed-byte accounting.
  8. Document that buffered transformation paths currently discard trailers. Preserve the raw relay_chunked path for requests that do not require inspection or body rewriting.

The goal is one framing/parser contract and one normalization contract, not necessarily one monolithic I/O function.

Alternatives Considered

Use one async buffering function everywhere

Inference parses an existing byte slice and needs exact consumed-byte accounting, while the other paths read incrementally from a stream. REST also has policy-generation checks and fail-open recovery concerns. Forcing all callers through one async API would add coupling without removing the important differences.

Keep the implementations separate and add tests

Tests help, but duplicated parsing and header normalization can still drift as limits and protocol behavior evolve. A shared conformance suite alone would not eliminate that maintenance and security risk.

Preserve all trailers after buffering

Buffered paths may transform the body or regenerate the upstream request. HTTP field semantics determine whether a trailer can safely become a header, so universal preservation or merging is unsafe. Keeping trailers separate in the shared result allows an explicit future policy without silently changing semantics.

Agent Investigation

Reviewed the request flows associated with PR #2027 and traced chunked bodies through REST credential injection, supervisor middleware, JSON-RPC/MCP, GraphQL, and inference routing. REST and JSON-RPC use incremental readers with different surrounding control flow; GraphQL duplicates much of the JSON-RPC buffering behavior; inference uses a synchronous slice parser for keep-alive framing. Searched existing OpenShell issues and found no issue directly tracking consolidation of buffered HTTP request handling.

  • I reviewed existing issues and architecture documentation before proposing this design.
  • This issue includes a concrete design proposal and alternatives.

Activity

  1. pimlock commented on Jul 15, 2026

    @pimlock
    CollaboratorAuthor

    Follow-up context from PR #2027 review

    The review confirmed that GraphQL and JSON-RPC/MCP already decoded chunked request bodies, removed Transfer-Encoding and Trailer, and forwarded a Content-Length body without the actual trailers before PR #2027. This is therefore a pre-existing limitation in those inspection paths.

    PR #2027 newly exposed the same semantic risk in REST middleware buffering and changed REST request-body credential rewriting from preserving raw chunked framing to decoding and normalizing it. The scoped decision for PR #2027 is to reject any request with non-empty chunked trailers in those two newly affected REST transformation paths. Transparent relay_chunked behavior remains unchanged and continues to preserve framing and trailers when no buffering or body transformation is required.

    The follow-up in this issue should move GraphQL, JSON-RPC/MCP, REST middleware, credential rewriting, and inference onto a structured buffered-framing result that keeps trailers separate and makes each caller choose an explicit reject, drop, or validated-retention policy. The conformance matrix should include declared-but-empty trailers, one or multiple actual trailers, trailer byte/count limits, malformed trailer fields, and exact pipelined-byte consumption.

  2. github-actions commented on Jul 29, 2026

    @github-actions

    This issue has had no activity for 14 days and is now marked stale. It may be closed in 7 days if there is no further activity. Comment or remove the state:stale label to keep it open.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    state:staleInactive item at risk of automatic closure.tech-debt

    Type

    No type

    Projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions