119 lines
6.5 KiB
Markdown
119 lines
6.5 KiB
Markdown
---
|
|
id: review-001-forward-response-fidelity
|
|
name: Upstream response decode — vendored JSON, size caps, loud auth errors, error bodies (FWD-07, FWD-08, FWD-10, FWD-12)
|
|
status: completed
|
|
depends_on: []
|
|
scope: moderate
|
|
risk: medium
|
|
impact: component
|
|
level: implementation
|
|
tags: [adapters, review-001]
|
|
---
|
|
|
|
## Description
|
|
|
|
Review 001 findings on `src/adapters/forward.rs`'s response-decode paths
|
|
(`forward`, `forward_stream`), minus the SSE parser (its own task):
|
|
|
|
- **FWD-07**: `content_type.contains("application/json")`
|
|
(`forward.rs:236`) misses `application/vnd.api+json`,
|
|
`application/problem+json`, etc. → vendor JSON decoded as a `Value::Array`
|
|
of one `Number` per byte. No response size limit on any read path
|
|
(`json()/text()/bytes()`) — a hostile upstream controls caller memory.
|
|
`forward_stream` never checks content-type (200 HTML → empty stream, no
|
|
error). Fix: mime-essence matching (`application/.*+json` suffix
|
|
semantics), response size caps on all read paths, and a loud error for
|
|
non-SSE content on a Sub op.
|
|
- **FWD-08** (`forward.rs:104-126`, `:87-94`): invalid credential values
|
|
(control characters, typo'd header name) silently drop the header —
|
|
the request goes out unauthenticated and the caller only sees the
|
|
eventual upstream 401. Fail loudly at call time. (No leak risk today —
|
|
nothing is logged; keep it that way.)
|
|
- **FWD-10** (`forward.rs:215-227`, `:333-346`): upstream error bodies
|
|
discarded — surfaced message is only `"HTTP {status}: {reason}"`;
|
|
upstream diagnostics never reach the caller and unconsumed bodies
|
|
hinder connection reuse. Surface a bounded error-body echo on the error
|
|
path; keep the `HTTP_<status>` mapping as is (correct).
|
|
- **FWD-12**: byte-identical duplicates `value_to_path_segment`/
|
|
`value_to_query` (`:133-151`); `forward`'s `op_type` parameter only
|
|
toggles `ACCEPT` and its `text/` branch would buffer an entire SSE
|
|
stream if a Sub were ever routed here (currently unreachable, inviting
|
|
misuse — remove or make safe); the `unwrap_or_else(|_| "null")` at
|
|
`:197` masks an unreachable serialization failure with a null body;
|
|
`SharedHttpClient::reload` performs two separate `ArcSwap::store` calls.
|
|
|
|
## Acceptance Criteria
|
|
|
|
- [x] `application/vnd.api+json` (and `application/problem+json`) decode as JSON, not byte arrays (test)
|
|
- [x] Response size cap enforced on JSON/text/binary read paths (test)
|
|
- [x] Non-SSE content-type on a Sub op produces a loud error, not an empty stream (test)
|
|
- [x] Invalid credential values fail loudly at call time (test), no credential material logged
|
|
- [x] Upstream 4xx/5xx bodies surface bounded in the error envelope (test)
|
|
- [x] Duplicate `value_to_*` helpers collapsed; `text/` buffering branch removed or safe; reload atomic
|
|
- [x] `forward.rs` COV-01 uncovered regions (binary branch, auth arms, non-2xx mapping, content-type branches) substantially covered
|
|
- [x] `cargo test` and `cargo clippy --all-targets -- -D warnings` pass
|
|
|
|
## References
|
|
|
|
- docs/reviews/001-initial-implementation-review.md (Part D, FWD-07, FWD-08, FWD-10, FWD-12; Part I, COV-01)
|
|
|
|
## Notes
|
|
|
|
> Agent fills during implementation. Shares forward.rs with
|
|
> review-001-forward-url-safety (request side) and
|
|
> review-001-sse-parser — sequence those first or coordinate.
|
|
|
|
FWD-07: JSON dispatch is now mime-essence based
|
|
(`is_json_content_type`: type `application`, subtype `json` or `+json`
|
|
suffix; hand-rolled — `mime` is in the lock file only as a transitive
|
|
dep of reqwest, not a direct dependency, so no new crate). Read paths
|
|
(JSON/text/binary) are byte-capped by `read_body_capped` at
|
|
`RESPONSE_BODY_CAP` = 16 MiB; over-cap surfaces `HTTP_413`.
|
|
`forward_stream` checks the `Content-Type` essence (`text/event-stream`)
|
|
and a non-SSE 2xx yields one `INVALID_RESPONSE_TYPE` error envelope
|
|
instead of an empty stream. FWD-08: every silently-dropped header —
|
|
default headers (`build_request`), and the Bearer/ApiKey/Basic
|
|
credential arms — now returns a loud `CallError::internal` ("refusing
|
|
to send the request unauthenticated"); error text names the header or
|
|
namespace but never echoes credential material (tested). FWD-10:
|
|
non-2xx in both `forward` and `forward_stream` goes through
|
|
`error_envelope`: `HTTP_<status>` mapping unchanged (ADR-023) plus a
|
|
bounded echo of the upstream body — read once, capped at
|
|
`ERROR_BODY_ECHO_CAP` = 4 KiB, control characters stripped (newline/tab
|
|
kept), `[truncated]` marker when the read hit `STATUS_BODY_DRAIN`
|
|
(64 KiB) without EOF, in which case the connection is dropped rather
|
|
than drained further (bounded connection-reuse trade). FWD-12:
|
|
`value_to_path_segment`/`value_to_query` collapsed onto one
|
|
`scalar_value_to_string` extractor (the raw scalar is shared; path
|
|
rendering applies `PATH_VALUE_ENCODE_SET`, query emission stays raw for
|
|
`query_pairs_mut` — they were byte-identical pre-FWD-01 but the roles
|
|
diverged, so a single raw-then-encode function was wrong for the query
|
|
arm and the pre-existing `+`-encoding test proved it); `forward` lost
|
|
its `op_type` parameter (the Sub branch only toggled ACCEPT and the
|
|
`text/` buffering branch is gone — Sub ops route through
|
|
`forward_stream`); request-body serialization failure is a loud
|
|
`CallError::internal` instead of a silent `null` body (both handlers);
|
|
`SharedHttpClient` now holds client+config in one
|
|
`ArcSwap<SharedHttpInner>` so `reload` swaps both in a single atomic
|
|
store (minimal touch to `src/client/http_client.rs`, public API
|
|
`client()`/`config()` unchanged). COV-01 regions covered: vendor/`+json`
|
|
content-type branches, binary branch, all three auth arms (loud error
|
|
paths + the pre-existing success tests), non-2xx mapping with error-body
|
|
echo (2/404/429/500, truncated and exact), cap trips on all three read
|
|
paths, non-SSE Sub stream, SSE happy path through `forward_stream`.
|
|
|
|
## Summary
|
|
|
|
FWD-07/08/10/12 remediated in `src/adapters/forward.rs` (+
|
|
one-spot `SharedHttpClient::reload` atomicity fix in
|
|
`src/client/http_client.rs`): vendor-JSON mime-essence decode, 16 MiB
|
|
`RESPONSE_BODY_CAP` on all buffered reads (`HTTP_413` on breach),
|
|
loud `INVALID_RESPONSE_TYPE` on non-SSE Sub-op streams, loud call-time
|
|
errors for invalid credential/default-header values (no secret echo),
|
|
bounded 4 KiB error-body echo on non-2xx with a 64 KiB
|
|
`STATUS_BODY_DRAIN` read cap, duplicate scalar helpers collapsed,
|
|
`forward`'s dead `op_type` Sub branch removed, serialization failure
|
|
made loud, and the hot-reload swap made atomic (joint client+config
|
|
holder). 23 tests in `forward.rs::tests` (was 9) covering the previously
|
|
uncovered binary/auth/non-2xx/content-type regions (COV-01); full
|
|
suite 265 passing. Commits 9bb8487 (core) and 7ac2dc5 (tests). |