feat(middleware): inspect HTTP responses before return - #3070
Conversation
Remove the request write-prefix restriction and route request and response mutations through one direction-aware atomic applicator.\n\nRefs #2691 Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Add the finalized bidirectional response protocol, separate gRPC service transport, manifest binding, and ordered headers/whole-body/streaming/trailer session runner.\n\nRefs #2691 Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
|
🌿 Preview your docs: https://nvidia-preview-pr-3070.docs.buildwithfern.com/openshell |
drew
left a comment
drew
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
This accepted-issue implementation has four blocking findings in response buffering, HTTP framing, connection lifecycle, and fail-open observability. The Fern documentation covers the new user-facing contract; the stale agent-skill guidance is a non-blocking follow-up under Gator's review calibration.
Action required: @pimlock, please address GATOR-8768b149-01 through GATOR-8768b149-04 and push an updated head for a focused follow-up review.
Blocking findings:
GATOR-8768b149-01: fail-open whole-body failure can retain an unbounded responseGATOR-8768b149-02: middleware can emit chunked framing on an HTTP/1.0 responseGATOR-8768b149-03: middleware reverses established relay connection-lifecycle outcomesGATOR-8768b149-04: fail-open response bypasses omit the required detection finding
Carried findings:
- None
Non-blocking suggestions:
- Update the mapped policy-generation, CLI, and cluster-debugging skills for
HTTP_RESPONSE/PRE_RETURNbefore release so agent guidance matches the new capability.
Gator metadata
- Validation: Project-valid implementation of accepted issue #2691
- Docs: Fern docs and architecture/RFC guidance updated; existing navigation already covers the edited pages
- Checks: Current Branch Checks and Helm Lint statuses are green; required E2E dispatch follows resolution of review blockers
- E2E:
test:e2erequired for network proxy, policy, and response-delivery behavior; not yet dispatched while code feedback is open - Head SHA:
8768b1498a8c4c16e461b1737bed50b61795dd04 - Base SHA:
8ffc6c2a13cd482dea9e2195058309344c274756 - Merge base SHA:
8ffc6c2a13cd482dea9e2195058309344c274756 - Patch ID:
97aabced6b999ee511cd1920996930217f7a2e23 - Gator payload:
7 - Review mode:
initial - Previous reviewed SHA: none
- Review budget exhausted: no
- Maintainer decision required: no
- Next state:
gator:in-review
| let deadline = Instant::now() + MAX_MIDDLEWARE_CHAIN_TIMEOUT; | ||
| let output = self.process_units_from(0, vec![data], deadline).await?; | ||
| if self.defer_output_until_finish { | ||
| self.deferred_output.extend(output); |
There was a problem hiding this comment.
gator-agent
Critical — GATOR-8768b149-01 · A failed whole-body barrier retains an unbounded response
Summary: With a fail-open whole-body stage on a chunked, close-delimited, or streaming response, crossing the configured limit disables that stage but leaves deferred output enabled. Every later upstream unit is accumulated until EOF, so an unbounded upstream can exhaust supervisor memory despite the documented payload cap.
Fix: When the last whole-body barrier fails open, release retained original bytes and explicitly transition the relay to bounded committed streaming; cover timeout, malformed-result, stream-closure, and overflow exits.
Verify: Configure a 4 KiB fail-open whole-body stage, push 4097 bytes and then repeated 64 KiB units. After the overflow, units must be released with bounded retention instead of accumulating until finish.
Agent context
- Location:
crates/openshell-supervisor-middleware/src/response.rs:215 - Ownership: This PR introduces the whole-body barrier and deferred accumulator.
There was a problem hiding this comment.
Addressed in draft PR #3074 (e6761464). When the final active whole-body stage fails open, the session now stops deferring output, releases all retained original bytes immediately, and disables the failed stage. The relay commits streaming-compatible framing at that transition, so later units pass through without retaining the rest of the response. The regression test crosses the 4 KiB stage limit at 4,097 bytes, then pushes two additional 64 KiB units and verifies that every unit is released immediately.
| let head = serialize_response_head( | ||
| &status_line, | ||
| &preflight.headers, | ||
| ResponseFraming::Chunked, |
There was a problem hiding this comment.
gator-agent
Warning — GATOR-8768b149-02 · HTTP/1.0 responses receive invalid chunked framing
Summary: When response middleware selects headers-only or streaming inspection, OpenShell preserves an upstream HTTP/1.0 status line but unconditionally rewrites the response as Transfer-Encoding: chunked. HTTP/1.0 recipients do not support chunked coding, so clients can expose chunk markers, truncate the body, or wait for close-delimited completion. Whole-body responses with trailers reach the same invalid combination.
Fix: Track the applicable HTTP version and choose compatible downstream framing; never emit chunked transfer coding with an HTTP/1.0 status line.
Verify: Relay HTTP/1.0 200 OK with a five-byte body through headers-only, streaming, and trailer-producing modes. Each result must use valid HTTP/1.0 framing and deliver exactly those body bytes without chunk markers.
Agent context
- Location:
crates/openshell-supervisor-network/src/l7/rest.rs:3530 - Sibling: The trailer path selects chunked framing at
rest.rs:3671.
There was a problem hiding this comment.
Addressed in draft PR #3074 (e6761464). The relay now detects an HTTP/1.0 status line. Header-only and streaming responses use raw close-delimited output with Connection: close; buffered whole-body responses use Content-Length. HTTP/1.0 output never sends Transfer-Encoding: chunked or a Trailer header. The regression covers header-only, streaming, and whole-body-with-upstream-trailer paths.
| } | ||
| client.flush().await.into_diagnostic()?; | ||
| Ok(Some( | ||
| if server_wants_close || matches!(body_length, BodyLength::None) { |
There was a problem hiding this comment.
gator-agent
Warning — GATOR-8768b149-03 · Middleware reverses relay connection-lifecycle semantics
Summary: After a successful middleware response, this branch returns Consumed for every Connection: close response and every response without body framing. The baseline relay deliberately returns Reusable for explicitly framed close responses so the next upstream write observes closure, and for unframed keep-alive responses treated as empty. The new result can tear down the enclosing tunnel, reintroduce long client retry delays, and lose supported reuse.
Fix: Mirror the baseline outcome rules: consume only close-delimited or event-stream unframed bodies; preserve reuse for explicit framing and unframed keep-alive responses.
Verify: With headers-only middleware, relay an explicitly framed Connection: close response and an unframed keep-alive empty response. Both must match their no-middleware Reusable outcomes.
Agent context
- Location:
crates/openshell-supervisor-network/src/l7/rest.rs:3704 - Ownership: This PR adds the separate middleware completion calculation.
There was a problem hiding this comment.
Addressed in draft PR #3074 (e6761464). Response completion now preserves the baseline connection outcome: explicitly framed Content-Length or chunked responses remain Reusable even when Connection: close is present, and an unframed keep-alive response with an empty body is also Reusable. Only an actual close-delimited response or event stream consumes the connection. The regression covers both reported cases.
| ) -> Vec<openshell_ocsf::OcsfEvent> { | ||
| invocations | ||
| .iter() | ||
| .map(|invocation| { |
There was a problem hiding this comment.
gator-agent
Warning — GATOR-8768b149-04 · Fail-open response bypasses lose the alert signal
Summary: A response stage that fails open after capacity exhaustion, timeout, stream closure, malformed output, or whole-body overflow emits only an HTTP activity record. Existing middleware paths and the published contract also emit a detection finding for reduced enforcement, which is the signal operators use to alert when inspection was bypassed. Successful delivery here can therefore hide degraded response controls.
Fix: Dual-emit a platform-owned detection finding for every failed FailOpen response invocation, including session-capacity exhaustion, using only validated policy, implementation, host, phase, and failure-category fields.
Verify: Exercise preflight failure, runtime timeout, whole-body overflow, and session-capacity exhaustion with fail-open stages. Each case must emit both its HTTP activity and one sanitized detection finding.
Agent context
- Location:
crates/openshell-supervisor-network/src/l7/rest.rs:3733
There was a problem hiding this comment.
Addressed in draft PR #3074 (e6761464). Every failed fail-open response invocation now emits both the HTTP activity event and a medium-severity detection finding. The finding carries only validated policy/config/implementation/host/phase fields plus a platform-owned failure category; it excludes free-form middleware reasons. Categories cover preflight/invalid results, timeouts, transport failures, payload capacity, and session capacity. The regression verifies the dual emission and that the service-provided reason is absent.
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
drew
added
gator:blocked
Monitoring CompleteMonitoring is complete because this PR has been closed without merge. Thanks @pimlock. I checked your note that this change was superseded by the reviewable stack in #3072, #3073, #3074, and #3075, and confirmed PR #3070 is closed with no merge commit. Final status: the last Gator review had four open findings on head I removed the active Gator metadata
|
|
Also addressed the review-level maintenance suggestion in draft PR #3075 ( |
2 participants
Summary
Add the finalized
HTTP_RESPONSE/PRE_RETURNmiddleware contract and enforce it on HTTP/1.x responses before sandbox delivery. The implementation supports headers-only, bounded whole-body, and normalized lockstep streaming modes while preserving explicit fail-open/fail-closed delivery semantics.Related Issue
Closes #2691
Changes
HttpResponsePreReturn.EvaluateAPI, manifest binding, remote transport, and ordered response session runtime.1xxresponses, repair downstream framing, normalize trailers, bypass upgrades, and distinguish failures before and after commitment.Testing
mise run pre-commitpassesmise run testpassesmise run cipassesmise run e2epasses (Rust, 89 Python scenarios, and MCP conformance)Checklist