docs(review 002): post-remediation review — 40 findings across 6 passes + coverage analysis
Review 002 of the post-remediation tree (91483a7): six parallel
subsystem passes + coverage analysis, consolidated with direct source
re-verification of all consequential findings.
- Regression check: 76/82 review-001 findings verified FIXED; residuals
filed as new findings (SRV-11/SRV-12, WS-13, CON-14, OAI-10..19,
JS-01..03, PRJ-16..24, CLI-01..03, FWD-13..19, GW-15..17)
- Critical-class: GW-15 (unbounded /publish body), OAI-11 (exponential
$ref expansion hangs import)
- Security: PRJ-16 (Internal op schema disclosure via services/schema
over /call), FWD-13 (lone .. path escape with credentials)
- Coverage: 94.87% regions; redirect policy 100% dark (top gap),
forward_stream terminal arms dark, COV-13 dead code batch
Headline counts: 5 major security/availability, 6 major correctness,
23 minor, 2 info; 2 agent findings rejected at verification.
This commit is contained in:
@@ -0,0 +1,850 @@
|
||||
# Review 002 — Post-remediation review (after the review-001 sweep)
|
||||
|
||||
## Status
|
||||
|
||||
Verified, open for remediation.
|
||||
|
||||
## Scope
|
||||
|
||||
Consolidated review of the post-remediation tree at `91483a7` — the
|
||||
state after all 42 review-001 tasks (20 primary + 4 follow-up, plus
|
||||
re-scoped sub-task splits) were completed across the two remediation
|
||||
sessions. Review-001's findings clustered at the seams (auth/visibility,
|
||||
outbound forwarding, projection fidelity, WS edges); the remediation
|
||||
rewrote or heavily reworked most of that surface (~2.5k added lines
|
||||
net, large rewrites in `forward.rs`, `byte_adapter.rs`, `routes.rs`
|
||||
publish path, the `to_openapi` projection, and the WS pump
|
||||
consolidation). The purpose of this pass is threefold:
|
||||
|
||||
1. **Regression check**: verify every review-001 fix holds and did not
|
||||
introduce new defects.
|
||||
2. **New-code review**: the remediation created substantial new
|
||||
subsystems and rewrote existing ones under time pressure — the
|
||||
highest-risk code in the crate is code that has never been reviewed
|
||||
at all (schema_cache, BufferedLines/NdjsonChunkStream, the generic
|
||||
pump, the retry/redirect/timeout policy stack, the $ref resolver).
|
||||
3. **Fresh-eyes**: anything review-001's lens missed.
|
||||
|
||||
The review consolidates six passes (server+gateway, WebSocket+from_wss,
|
||||
forwarding core+client host, from_openapi/from_jsonschema/openapi_spec,
|
||||
to_openapi/to_mcp/from_mcp, and a coverage pass) plus a consolidating
|
||||
pass that re-verified the consequential findings directly in source
|
||||
(and where agents disagreed, read the code to decide). Cross-crate
|
||||
findings are flagged as such.
|
||||
|
||||
## Baseline verification (this pass)
|
||||
|
||||
```
|
||||
cargo test --all-features → 411 passed, 0 failed (370 unit + 41 integration)
|
||||
cargo clippy --all-features --all-targets -- -D warnings → clean
|
||||
cargo fmt --check → clean
|
||||
cargo llvm-cov --all-features → 94.87% regions / 94.63% lines / 92.22% functions
|
||||
```
|
||||
|
||||
The suite is green, hygiene gates (`deny(missing_docs)`, clippy, fmt)
|
||||
hold, and coverage is high. The findings below are overwhelmingly **new
|
||||
code under review** and **residual gaps in remediated code** — not a
|
||||
return of review-001's classes.
|
||||
|
||||
## Verdict
|
||||
|
||||
- **The remediation held.** Every review-001 finding was re-verified
|
||||
against its fix; 76 of 82 land FIXED-VERIFIED. The residuals are
|
||||
tracked below as new findings (SRV-12, WS-13, CON-14, OAI-11/12/13,
|
||||
JS-01, PRJ-16..17, CLI-01..03, FWD-13..14, GW-15..17, WS-14..19) —
|
||||
none is a reversion of the original defect; each is a gap one layer
|
||||
short of the fix's goal, or a new-mechanism bug.
|
||||
- **The one critical-class regression-adjacent defect:** GW-15 —
|
||||
`/publish` moved from a buffered extractor to a raw-`Body` NDJSON
|
||||
stream (the GW-06 streaming fix) and in doing so **lost both claimed
|
||||
memory bounds**: axum's `DefaultBodyLimit` does not apply to
|
||||
raw-`Body` handlers (it is an extractor-side extension), and the
|
||||
per-line cap check only runs when a `\n` arrives — a never-newline
|
||||
chunked upload grows the heap unboundedly (the SRV-03 class on a
|
||||
different route).
|
||||
- **One new borderline-critical defect:** OAI-11 — the OAI-01 fix
|
||||
(cycle guard + depth budget) bounds *stack*, not *work*; an acyclic
|
||||
shared-`$ref` chain (doubly-linked-list-shaped schema, valid and
|
||||
common) expands exponentially and can hang `import()` indefinitely —
|
||||
the same "valid spec kills the process" class OAI-01 found, slower.
|
||||
- **The dominant theme of the new findings is policy-vs-progress
|
||||
timeouts and terminality:** the WS idle timer resets on message
|
||||
arrival (so the dribble stall it was built for defeats it — WS-13);
|
||||
the SSE subscription path inherits a 30 s total-request timeout that
|
||||
kills healthy long-lived subscriptions (FWD-15) while its terminal
|
||||
arms are untested; `from_wss`'s new pending-sweep never exits (CON-14,
|
||||
a leak per dead session); and the MCP pagination loop trusts the
|
||||
remote to eventually stop (CON-14).
|
||||
- **Projection fidelity is much improved** (runtime-truthful `/call`,
|
||||
`/publish`, `/batch` verified against the wire) but two disclosure
|
||||
bugs remain: Internal op specs are readable through the
|
||||
`services/schema` op dispatched via `/call` (PRJ-16 — the GET
|
||||
`/schema` fix has a sibling hole through the op path), and the
|
||||
published document carries a dangling `$ref` (PRJ-16b) plus several
|
||||
wrong-status drifts (PRJ-17..21).
|
||||
|
||||
## Severity legend
|
||||
|
||||
- **[critical]** — the protocol cannot function end-to-end as committed;
|
||||
or a decided spec invariant is violated in a way that corrupts data or
|
||||
causes silent permanent state damage.
|
||||
- **[major]** — a decided behavior is missing, wrong, or a real
|
||||
reliability hazard; works in the happy path but fails a spec-required
|
||||
edge case, or is a realistic security weakness.
|
||||
- **[minor]** — drift, convention violation, dead code, or a doc/spec
|
||||
inconsistency with no correctness impact.
|
||||
|
||||
---
|
||||
|
||||
# Part A' — Server core (regression residuals + new findings)
|
||||
|
||||
**Regression check: SRV-01..SRV-10 all FIXED-VERIFIED** (evidence per
|
||||
finding consolidated in the remediation ledger below), with two
|
||||
residuals now filed as SRV-11/SRV-12.
|
||||
|
||||
## SRV-11 [minor] — Router-wide bearer layer double-wraps the WS upgrade route and `/mcp`; the SRV-10 comment claims the opposite
|
||||
|
||||
**Verified:** YES (empirically, scratch harness replicating
|
||||
`build_router` against axum 0.8.9 semantics). `src/server/adapter.rs:296-345`.
|
||||
The comment says the WS route "is registered BEFORE the router-wide
|
||||
bearer_auth_middleware route_layer below … so the WS path resolves the
|
||||
token exactly once." axum's `route_layer` wraps routes registered
|
||||
*before* the call — the router-wide layer at :328 is applied after the
|
||||
WS route was registered, so it wraps it: **both** layers run on
|
||||
`GET /alk/channels` and the token resolves exactly twice. Same for
|
||||
`/mcp` (own layer at :288-292 + router-wide at :328). Functionally
|
||||
benign today (both stashes agree), but a provider with token-use side
|
||||
effects double-counts and the code documents the opposite of the
|
||||
behavior. Ironically this reintroduces the minor SRV-10 fold-in that
|
||||
the remediation claimed to remove. **Fix:** drop one of the two layers
|
||||
per route (or place the WS/MCP registration after the router-wide
|
||||
layer) and correct the comment; add a test counting token resolutions.
|
||||
|
||||
## SRV-12 [minor] — Decoy 405 does not cover merged extra routes; wrong-method probes on custom paths return axum's bare 405
|
||||
|
||||
**Verified:** YES (empirically). `adapter.rs:314-332` —
|
||||
`method_not_allowed_fallback(decoy_method_not_allowed)` is registered on
|
||||
the default router, then extras are merged. axum applies the 405
|
||||
fallback only to method routers registered at the time of the call, so
|
||||
`DELETE /<extra-path>` returns a 405 with an empty body and **no
|
||||
`Server: nginx` header** in a decoy deployment with custom routes — the
|
||||
exact probe SRV-07's fix neutralized for the default surface (verified
|
||||
fixed there, `adapter.rs:1184-1215` test). **Fix:** re-apply
|
||||
`method_not_allowed_fallback` after the extras merge.
|
||||
|
||||
---
|
||||
|
||||
# Part B' — WebSocket subsystem + from_wss
|
||||
|
||||
**Regression check: WS-02/04/05/06/07/08/09/10/11 FIXED-VERIFIED.**
|
||||
WS-01 and the CON-02 sweep are the two residuals (WS-13, CON-14). The
|
||||
pump consolidation preserved behavior parity (diff-verified
|
||||
`4a825d3..HEAD`) and the inbound memory bound dropped from ~4 GiB to
|
||||
64 MiB/conn. No reversion found.
|
||||
|
||||
## WS-13 [major] — The idle timeout cannot fire in the WS-01 stall shape, and legitimately slow streams are killed instead
|
||||
|
||||
**Verified:** YES. `src/websocket/byte_adapter.rs:229-244`. The 60 s
|
||||
timer wraps only the *next-message await*. WS-01's attack — declare a
|
||||
16 MiB chunk, then deliver it one byte per WS message per minute —
|
||||
keeps arriving messages, so the deadline resets on every dribble while
|
||||
the demux still waits forever for the chunk to complete: the stall that
|
||||
WS-01's fix exists to bound persists, knob or no knob. Conversely,
|
||||
a legitimate >60 s-silent session (long compute, quiet subscription) is
|
||||
disconnected with 1001 mid-flight, because no keepalive/ping resets the
|
||||
timer and chunk-level progress does not. The integration test
|
||||
(`idle_read_timeout_resets_on_traffic`) enshrines reset-on-message as
|
||||
correct. **Fix:** drive the deadline off demux *progress* (reset when
|
||||
bytes are actually forwarded into `read_tx`), not message arrival; add
|
||||
a keepalive or document the knob as an application-silence bound, not
|
||||
an anti-dribble guarantee. Acceptance gates: a dribble-stall test that
|
||||
bounds demux progress, and a slow-but-alive session that survives.
|
||||
|
||||
## WS-14 [minor] — Byte-cap violation is detected only after the mux has committed the write
|
||||
|
||||
**Verified:** YES. `byte_adapter.rs:326-334, 585-586` — `poll_write`
|
||||
accepts into the bounded channel first; a single write above
|
||||
`PENDING_BUFFER_CAP` (only possible from a non-chunk-framed producer)
|
||||
is rejected *after* the mux believes it written → abrupt 1011 and a
|
||||
truncated chunk on the wire. Self-healing and unreachable for a correct
|
||||
mux. **Fix:** reject over-cap writes in `poll_write` before `try_send`.
|
||||
|
||||
## WS-15 [minor] — Close frames carry the hardcoded `"text messages not supported"` reason regardless of cause
|
||||
|
||||
**Verified:** YES. `byte_adapter.rs:316-324` — the same `WriteMsg::
|
||||
CloseWith(WS_GOING_AWAY)` is used for idle-timeout (1001), inbound-size
|
||||
cap (1011), and text rejection (1002); the reason string lies in two of
|
||||
three cases. Numeric codes (what clients key on) are correct. **Fix:**
|
||||
carry the reason in the enum variant.
|
||||
|
||||
## WS-16 [minor] — Withdrawn during verification (merged into CON-14)
|
||||
|
||||
## WS-17 [minor] — Bare-registry/custom WS routes get no idle-timeout knob surface
|
||||
|
||||
**Verified:** YES. `src/websocket/upgrade.rs:104-111` —
|
||||
`SessionState::from_registry` hardcodes `Some(DEFAULT_WS_IDLE_TIMEOUT)`
|
||||
and a private 64-session semaphore; a deployment assembling its own
|
||||
upgrade route cannot raise the cap or disable the timeout (no extension
|
||||
override, unlike `ChannelsPolicy`). Correct-but-inflexible; one
|
||||
authenticated client per custom route is bounded at 64 sessions/60 s. A
|
||||
long-lived silent subscription deployment needs `None` and cannot get
|
||||
it. **Fix:** a `WsIdleTimeout`/`WsSessions` request extension mirroring
|
||||
`ChannelsPolicy`, or document the fixed values.
|
||||
|
||||
## WS-18 [minor] — Write-side stall is unbounded: a peer that stops reading parks the mux indefinitely
|
||||
|
||||
**Verified:** YES. `byte_adapter.rs:584-597` — the idle knob covers
|
||||
reads only. A peer that keeps the WS alive but stops reading (full TCP
|
||||
window) blocks the write pump in `ws_sink.send`, fills the 64 WriteMsg
|
||||
slots (bounded ~1 GiB worst case), and `poll_write` returns Pending —
|
||||
the connection's mux and handlers park until the peer resumes or an
|
||||
admin aborts via `WsSessions`. Post-auth, per-connection, bounded
|
||||
memory; unbounded time. **Fix:** a write-progress timeout in the write
|
||||
pump (same knob family as the read one).
|
||||
|
||||
## WS-19 [minor] — Axum-flavor pump failure arms have no direct test; the cap-trip closes are verified on tungstenite only
|
||||
|
||||
**Verified:** YES. `byte_adapter.rs:782-1206` — all unit tests
|
||||
instantiate `TungsteniteFraming`; the axum flavor's text→1002 and
|
||||
size-cap close are covered end-to-end via `ws_upgrade_session.rs`, but
|
||||
the write-cap (WS-04/14) and idle-close arms are exercised on one flavor
|
||||
only. The shared pump makes regression risk low; the flavor adapters are
|
||||
the untested seam. **Fix:** mirror one text-rejection and one cap-trip
|
||||
test over `AxumFraming`.
|
||||
|
||||
## CON-14 [minor] — `from_wss` drop monitor loops forever after EOF: one 1 Hz task per dead session, permanently
|
||||
|
||||
**Verified:** YES. `src/adapters/from_wss.rs:254-272` — once
|
||||
`eof_observed` holds, the sweep branch runs `fail_all` every second
|
||||
forever; there is no exit, and naive `pending.is_empty()` early-exit
|
||||
*would* be wrong (post-EOF registrations race `fail_all` — the sweep
|
||||
test proves they register and would hang without it). Each fire-and-
|
||||
forget `import()` whose peer dies leaves a spawned task + pending map +
|
||||
watch receiver alive for the process lifetime. **Fix:** mark the
|
||||
connection dead so post-EOF registrations fail fast
|
||||
(`CONNECTION_CLOSED`), then let the sweep exit after a bounded grace
|
||||
period.
|
||||
|
||||
---
|
||||
|
||||
# Part C' — Gateway (regression residuals + new findings)
|
||||
|
||||
**Regression check: GW-01..GW-14 all FIXED-VERIFIED except GW-05/GW-06
|
||||
(PARTIAL) and the residuals below.** The publish path's true streaming
|
||||
landed (NdjsonChunkStream), publish_schema validation is fail-closed
|
||||
with a compile-once cache, batch cap/request-id/Retry-After/SSE
|
||||
terminality/keep-alive all verified with real tests.
|
||||
|
||||
## GW-15 [major] — `/publish` can buffer an unbounded request body despite two claimed caps
|
||||
|
||||
**Verified:** YES (source + axum-core semantics).
|
||||
`src/gateway/routes.rs:249-253, 425-448`.
|
||||
1. The handler takes raw `axum::body::Body` — axum's 2 MiB
|
||||
`DefaultBodyLimit` is implemented as a request extension that only
|
||||
*extractors* consult; a raw-Body handler passes the body through
|
||||
untouched. The doc's "in addition to axum's own 2 MiB default body
|
||||
limit" claim is false on this route.
|
||||
2. `BufferedLines::next_line` enforces `MAX_PUBLISH_LINE_BYTES` only
|
||||
when a `\n` is found; bytes accumulate in `self.buffer` unboundedly
|
||||
until then, and the trailing-EOF `::take(&mut self.buffer)` path has
|
||||
no cap check at all.
|
||||
Concrete failure: `POST /publish` (pre-dispatch, permissive path) with
|
||||
chunked transfer-encoding streaming `'a'` forever → heap grows with the
|
||||
upload until OOM; each poll also re-scans the whole buffer from index
|
||||
zero → O(n²) CPU on top. This is the SRV-03 class one route over.
|
||||
**Fix:** enforce the cap on `self.buffer` growth in the read loop
|
||||
(error once `len > MAX_PUBLISH_LINE_BYTES` even without a newline), and
|
||||
wrap the route in an explicit request-body-limit layer.
|
||||
|
||||
## GW-16 [minor] — `INVALID_INPUT` status drift persists on the new hand-rolled paths (400 vs the documented 422)
|
||||
|
||||
**Verified:** YES. `routes.rs:174-183, 257-278, 459-464` vs
|
||||
`docs/architecture/http-server.md:361`. The new pre-dispatch `/publish`
|
||||
and `/batch` rejection paths hand-build `INVALID_INPUT` responses with
|
||||
`StatusCode::BAD_REQUEST` (empty body, missing `operation`/`chunk`,
|
||||
invalid first line, line-cap, batch over-cap) while mid-stream chunk
|
||||
validation correctly emits the documented 422. Same error class, two
|
||||
statuses depending on where it fires — the GW-03 drift class, one table
|
||||
row down. **Fix:** route these through `call_error_to_http_response_*`,
|
||||
or sanction 400-for-body-shape explicitly in the doc.
|
||||
|
||||
## GW-17 [minor] — The documented 30 s deadline does not cover the `/publish` final envelope; a hung sink handler holds the request forever
|
||||
|
||||
**Verified:** YES. `src/gateway/dispatch.rs:132-145` — GW-05's timeout
|
||||
covers `invoke` only; `invoke_sink` has no deadline and the module doc
|
||||
justifies it ("bounded by the client's upload" — that bounds chunk
|
||||
arrival, not the handler's final await), while http-server.md:381-384
|
||||
explicitly includes the final envelope. **Fix:** wrap the sink
|
||||
handler's completion in the same 30 s timeout, or amend the doc.
|
||||
|
||||
## SRV-13 (recorded, non-finding) — `CachedOpenAPIDoc`/`PublishSchemaCache` staleness is structurally impossible
|
||||
|
||||
The registry is `&mut self`-registered before the `Arc` is shared, so
|
||||
post-construction mutation is impossible. The value-keyed cache
|
||||
invalidation is correct by inspection; the untested transition is
|
||||
Failed→corrected (see test gaps).
|
||||
|
||||
---
|
||||
|
||||
# Part D' — Outbound forwarding core + client host
|
||||
|
||||
**Regression check: FWD-02/03/04/05/06/08/09/10/11/12 FIXED-VERIFIED,**
|
||||
FWD-01/07 PARTIAL with residuals below. The URL pipeline is
|
||||
substantially hardened (single-pass render, encode-set covers `?`/`#`/
|
||||
`%`/`/`/`\`, origin equality check post-`set_path`, userinfo reject,
|
||||
redirect stop on scheme/host/port change, POST/PATCH not retried, 300 s
|
||||
Retry-After ceiling in both seconds and date forms, real incremental
|
||||
SSE parser with split-UTF8 and chunk-boundary unit tests).
|
||||
|
||||
## FWD-13 [major] — Lone `.`/`..` path values survive the encode set and are silently normalized by `Url::set_path` → path/tenant escape **with credentials attached**
|
||||
|
||||
**Verified:** YES (empirically against the locked `url 2.5.8`).
|
||||
`src/adapters/forward.rs:340-352` (encode set lacks `.`), `:519`
|
||||
(`set_path`), `:520-522` (origin check cannot catch it).
|
||||
`{tenant} = ".."` with template `/tenants/{tenant}/resources` renders
|
||||
`/tenants/../resources`, which `set_path` normalizes to `/resources` —
|
||||
a different upstream endpoint than the template describes (typically
|
||||
the less-scoped list-everything route), with the namespace's injected
|
||||
credential attached. The percent-encoded spelling is *also* normalized
|
||||
(`%2e%2e`), so extending the encode set does not fix it. The
|
||||
dot-segment test only covers the multi-segment `../../admin` form that
|
||||
the encoder *does* catch. **Fix:** reject rendered values that are
|
||||
exactly `.`/`..` in `value_to_path_segment` (the two normalized
|
||||
spellings), plus a post-`set_path` segment-count invariant test.
|
||||
|
||||
## FWD-14 [minor→major-class] — `forward_stream` has no response size accounting; a hostile SSE upstream is bounded only by the 30 s wall clock
|
||||
|
||||
**Verified:** YES. `forward.rs:880-934` — the streaming branch feeds
|
||||
`bytes_stream()` straight into `SseParser`; the 1 MiB cap bounds a
|
||||
single *line*, not the stream; the buffered branches carry caps, the
|
||||
streaming branch carries none (reqwest gzip off, so no decompression
|
||||
multiplier; total+read timeouts verified to apply). 30 s of 1-MiB-line
|
||||
events is still ~GB/s into envelope allocation. **Fix:** byte counter
|
||||
across `feed` on the stream path (total cap) and check `buf.len()`
|
||||
*before* extending so the overshoot can't exceed cap+chunk.
|
||||
|
||||
## FWD-15 [major] — SSE subscriptions inherit the 30 s total request timeout → healthy long-lived subscriptions are killed at 30 s
|
||||
|
||||
**Verified:** YES (verified against reqwest 0.13.4 source that
|
||||
`timeout()` rides into the response body stream). `forward.rs:824-845`
|
||||
sends the Sub forward through the shared client whose
|
||||
`DEFAULT_REQUEST_TIMEOUT` is 30 s; the gateway deliberately runs
|
||||
subscriptions unbounded (dispatch.rs:28, ADR-021) but the outbound half
|
||||
dies at 30 s with `SSE stream error: operation timed out` — a
|
||||
keep-alive-emitting source still dies. The direct dual of the read-
|
||||
timeout doc claim (`http_client.rs:149-150`). **Fix:** for
|
||||
`HandlerKind::Stream` forwards send with `request_timeout: None`
|
||||
(connect + read timeouts retained) via a per-request extension or a
|
||||
derived client.
|
||||
|
||||
## FWD-16 [major] — Missing capability ⇒ silently unauthenticated outbound request; only *malformed* credentials fail loudly
|
||||
|
||||
**Verified:** YES. `forward.rs:200-238` — every FWD-08 loud-error arm
|
||||
requires the capability to exist and be malformed; the
|
||||
`context.capabilities.get(namespace) == None` arm (the *most probable*
|
||||
misconfiguration shape — wrong capability key prefix, forgotten
|
||||
registration) silently sends the request unauthenticated. **Fix:** when
|
||||
`auth_scheme.is_some()` and the capability is absent, return `INTERNAL`
|
||||
("capability missing; refusing to send unauthenticated").
|
||||
|
||||
## CLI-01 [major] — `Retry-After` sleep sits outside the retry budget and re-arms a full ceiling on every throttled retry
|
||||
|
||||
**Verified:** YES (middleware order: `RetryGateMiddleware` outside
|
||||
`RetryAfterMiddleware`, `http_client.rs:511-520`). One logical request
|
||||
that gets `429`/`Retry-After: 300` sleeps to the deadline *per attempt*
|
||||
and each 429 re-arms `now + 300 s` (clamped to a full new ceiling);
|
||||
`TotalRetryBudget` gates only its own backoff sleeps, and the 30 s
|
||||
request timeout never starts (the sleep happens before the reqwest
|
||||
request is constructed) → up to ~15 min wall time inside one
|
||||
`forward()` call. **Fix:** sleep only when a retry will actually
|
||||
happen, and clamp the sleep by the remaining retry budget (check
|
||||
`max_total_retry_duration` inside `maybe_sleep_for`).
|
||||
|
||||
## FWD-17 [minor] — Non-JSON SSE payloads silently degrade to JSON strings; the parser's `event:` field is discarded
|
||||
|
||||
**Verified:** YES. `forward.rs:774-781` — `from_str().unwrap_or(String)`
|
||||
erases payload shape (`123` vs `"123"`) and an upstream's SSE
|
||||
`event: error` convention is indistinguishable from data. Documented
|
||||
JSON-or-string contract or carry the raw payload + event type. Minor.
|
||||
|
||||
## FWD-18 [minor] — Non-scalar path-placeholder values double-route: rendered into the path *and* emitted as query params
|
||||
|
||||
**Verified:** YES. `forward.rs:144-157` — `is_path_placeholder` skips
|
||||
query emission only while the *raw* input also renders into the path;
|
||||
for an object/array value under a placeholder key with an
|
||||
additionalProperties-allowing schema, the JSON blob is embedded in the
|
||||
path *and* the key is appended to the query. Inconsistent routing, no
|
||||
error. **Fix:** when the key matched a placeholder, never also emit it
|
||||
as query regardless of value shape.
|
||||
|
||||
## FWD-19 [info] — Preserved literal `%` in template/base text is upstream semantics-changing (documented trade-off)
|
||||
|
||||
The second encoding pass deliberately preserves `%` (so encoded values
|
||||
survive), which means literal `%2F` sequences in *template text* reach
|
||||
the upstream as `%2F` (different route per most stacks). Inputs are
|
||||
assembly-supplied (ADR-066 trust); document it.
|
||||
|
||||
## CLI-02 [minor] — The FWD-03 redirect fix has zero wire-level regression tests
|
||||
|
||||
No test in `tests/` sends any 3xx; the same-host/cross-host decision,
|
||||
hop cap, and "credentials do not ride a cross-host redirect" property
|
||||
are all regression-fragile (a future edit breaks the fix with all tests
|
||||
green). The harness seam already exists (`spawn_responder`). **Fix:**
|
||||
two wire tests (same-host 302 followed with header; cross-host 302
|
||||
surfaced, second endpoint never contacted).
|
||||
|
||||
## CLI-03 [minor] — FWD-04's method gate, budget, and attempt cap are untested on the wire
|
||||
|
||||
No test counts upstream hits on retries, no POST-bypass test, no
|
||||
budget-exhaustion test, no timeout-default behavioral test — review
|
||||
001's gap list flagged exactly these; the remediation added the code
|
||||
but not the tests. **Fix:** counting-responder tests (POST ⇒ 1 hit; GET
|
||||
500×2 ⇒ 3 hits; budget ⇒ ≤ budget + one attempt).
|
||||
|
||||
---
|
||||
|
||||
# Part E' — from_openapi / from_jsonschema / openapi_spec
|
||||
|
||||
**Regression check: OAI-01 PARTIAL (stack-safe, work-unbounded → OAI-11),
|
||||
OAI-03/04/05/07/08/09 FIXED-VERIFIED, OAI-02/06 PARTIAL with residuals.**
|
||||
The cycle guard is shape-safe (array-items, allOf/oneOf, additionalProperties,
|
||||
mid-object re-entry all verified by code-reading; the *tests* for those shapes
|
||||
are missing — OAI-16). No unwrap/expect remains in the three files' non-test
|
||||
code (one `assert_eq!` in `reject_collisions` → JS-03).
|
||||
|
||||
## OAI-11 [major, borderline critical] — The depth budget bounds stack, not work: acyclic shared-`$ref` chains expand exponentially and can hang `import()` indefinitely
|
||||
|
||||
**Verified:** YES (empirically, harness of the verbatim resolver).
|
||||
`openapi_spec.rs:382-437`. The OAI-01 fix bounds recursion depth and
|
||||
rejects cycles, but every `$ref` hop re-expands its target from a fresh
|
||||
clone — a shared-ref chain shaped like a doubly-linked list
|
||||
(`S_i = {a: $ref S_{i+1}, b: $ref S_{i+1}}`, valid, acyclic) expands as
|
||||
a full binary tree: measured 20 levels → 7.3M node visits / 3.6 s; ~2^N
|
||||
growth means 30 levels ≈ an hour and 40 ≈ days, with no stack overflow —
|
||||
just a wedged import (both guards never fire). `import()` is
|
||||
assembly-time today, but the same "valid document kills the process"
|
||||
class as OAI-01, one mitigation shy. **Fix:** memoize resolved cycle-
|
||||
free `$ref` targets per document (or cap total resolved node count with
|
||||
a clean error). Acceptance gate: a 30-level shared-chain import returns
|
||||
in bounded time.
|
||||
|
||||
## OAI-12 [major] — YAML path silently corrupts specs: `.inf`→`null` constraint, duplicate keys last-wins (vs JSON's hard error), merge keys unprocessed
|
||||
|
||||
**Verified:** YES (empirically through `yaml_serde 0.10.7`).
|
||||
`openapi_spec.rs:153-158` — a single `from_str::<Value>` with zero
|
||||
post-parse normalization: `maximum: .inf` becomes `Value::Null` (a
|
||||
declared constraint silently vanishes from the advertised schema);
|
||||
duplicate mapping keys silently last-win via the Value path (JSON
|
||||
errors on the same document — the two input formats disagree, YAML's
|
||||
failure mode is silent); `<<: *anchor` survives as a literal `"<<"`
|
||||
property in the advertised schema. Parser-level bounds (alias bombs,
|
||||
128-depth) verified working. **Fix:** post-parse walk on the YAML path:
|
||||
reject non-string keys, duplicate keys, and `.inf/.nan` numbers; invoke
|
||||
`apply_merge()` (or reject merge keys loudly).
|
||||
|
||||
## OAI-13 [major] — Path-item-level `parameters`, `2XX/4XX/5XX` wildcard responses, and `webhooks` are still silently mishandled
|
||||
|
||||
**Verified:** YES. `openapi_spec.rs:276-336` (only HTTP-method keys
|
||||
read — the skip filter explicitly whitelists `"parameters"` out of the
|
||||
unsupported-methods warning), `from_openapi.rs:136-145`.
|
||||
1. Shared path-item `parameters` never merge into operations → `{id}`
|
||||
unbound → the whole import fails, with a *misleading* diagnosis
|
||||
(nothing names the actual cause) against extremely common real specs.
|
||||
2. `"2XX"` success keys are missed by the SSE detection sweep → the
|
||||
OAI-06 misbehavior (stream imports as giant-text Mutation) survives
|
||||
one spelling away; `"4XX"/"5XX"` error keys drop silently with the
|
||||
generic warn.
|
||||
3. Top-level `webhooks` in mixed docs silently vanish. **Fix:** merge
|
||||
path-item parameters (operation overrides), extend both sweeps with
|
||||
the wildcard keys, import-or-reject `webhooks` loudly.
|
||||
|
||||
## OAI-18 [minor→major-class] — Enforcement is a key-allowlist: `required`, `enum`, `pattern`, and value types are advertised via `/schema` but never enforced at call time
|
||||
|
||||
**Verified:** YES. `forward.rs:288-329`. A `required: [id]` op accepts
|
||||
`{}`; an advertised-string property sent as an object serializes as
|
||||
JSON text into the query string; enum/min/pattern are never consulted.
|
||||
Advertise/enforce drift — the contract the gateway publishes is broader
|
||||
than the one it defends (the OAI-02 *unknown-key* vector is closed;
|
||||
this is the declared-but-unchecked remainder). The `jsonschema` crate
|
||||
is already a dependency (schema_cache uses it). **Fix:** compile-and-
|
||||
enforce the full input schema per op (reusing the cache pattern), or
|
||||
document the allowlist-only scope in ADR-066 and stop advertising what
|
||||
is not enforced.
|
||||
|
||||
## OAI-10 [minor] — `$ref` sibling keys are silently discarded (OpenAPI 3.1 divergence, no version gate)
|
||||
|
||||
**Verified:** YES. `openapi_spec.rs:396-410` — under 3.1, `{"$ref": …,
|
||||
"minLength": 3}` advertises a constraint through `/schema` that the
|
||||
resolved schema (used at call time) does not carry. There is no
|
||||
`openapi: 3.1` gate anywhere. **Fix:** warn on `$ref` siblings or gate
|
||||
the version, at minimum documented.
|
||||
|
||||
## OAI-14/OAI-15/OAI-16/OAI-17/OAI-19/JS-01/JS-02/JS-03 [minor] — cluster
|
||||
|
||||
- **OAI-14**: top-level `oneOf` requestBodies (unconstraining `body`
|
||||
contract), `discriminator`, `xml`, `callbacks`, `security` blocks are
|
||||
silently ignored — the OAI-06 loudness matrix has holes; loudness
|
||||
placement inconsistent (some features fail import, these don't).
|
||||
- **OAI-15**: a self-`$ref`'d requestBody resolves to content-less →
|
||||
body-less op registered silently (import succeeds; every call
|
||||
`INVALID_INPUT`s on `body`).
|
||||
- **OAI-16**: cycle-guard *tests* miss array-items/allOf/additionalProperties
|
||||
shapes (guard correct by inspection — insurance gap).
|
||||
- **OAI-17**: import errors echo unbounded spec-derived strings
|
||||
(100k-path servers lists → multi-megabyte error message); no
|
||||
credential material echoed (verified).
|
||||
- **OAI-19**: declared `in: header` params silently lose to
|
||||
`default_headers` and credential headers (last insert wins) —
|
||||
peer-supplied header values never reach the upstream when names
|
||||
collide; no import-time collision warning.
|
||||
- **JS-01 [major]**: `from_jsonschema` placeholder-binding check is
|
||||
skipped when `input_schema` has no `properties` key
|
||||
(`from_jsonschema.rs:166-188` — `is_empty → return false`), so
|
||||
`new(spec_without_properties, "/widgets/{id}")` **passes
|
||||
construction** and fails every call — the exact eager-validation
|
||||
promise OAI-09's fix made. The `from_openapi` equivalent handles the
|
||||
empty case correctly. **Fix:** drop the early return; a placeholder
|
||||
with no declared properties is unbound by definition.
|
||||
- **JS-02**: `from_openapi` templates are not validated for balanced
|
||||
braces at import; `/x{open` imports and fails per-call INTERNAL
|
||||
(the OAI-09 shape, one file over — reuse `from_jsonschema`'s check).
|
||||
- **JS-03**: `assert_eq!` in library code (`reject_collisions`,
|
||||
`from_openapi.rs:74-75`) — panic-family in non-test code.
|
||||
|
||||
---
|
||||
|
||||
# Part F' — Projections (regression residuals + new findings)
|
||||
|
||||
**Regression check: PRJ-01/03/04/05/07/08/09/10/12/13/14/15
|
||||
FIXED-VERIFIED.** The regenerated document's envelope shape, 422 body,
|
||||
error-status annotations, `/subscribe` contract, and securitySchemes
|
||||
were verified against the wire tests and are runtime-truthful except
|
||||
where noted below. The MCP gateway's search filter, Pub exclusion,
|
||||
batch shape, retryable, and wrapping are all pinned by tests. Two
|
||||
disclosure/correctness residuals (PRJ-16, PRJ-16b) and a drift cluster.
|
||||
|
||||
## PRJ-16 [major, security] — Internal/ACL-restricted op specs leak through `services/schema` invoked via `POST /call` (and the MCP `call` tool)
|
||||
|
||||
**Verified:** YES (alkcall source + alkhttp routes).
|
||||
`src/gateway/routes.rs:120-133` + alkcall `registry/discovery.rs:327-343`.
|
||||
`call_handler`'s pre-check guards only the *outer* op name
|
||||
(`services/schema` is External): `registry.invoke` dispatches to
|
||||
`services_schema_handler`, which does a bare
|
||||
`registry.registration(name)` → `spec_to_json` with **no visibility and
|
||||
no AccessControl check**. An unauthenticated caller reads the complete
|
||||
input/output/error schemas and `access_control.required_scopes` of
|
||||
every Internal op — the exact disclosure GET `/schema` (:151-156) and
|
||||
the MCP `schema` tool (to_mcp.rs:292-308) were fixed to prevent. The
|
||||
regenerated doc asserts "Internal (hidden from HTTP discovery, GW-02)"
|
||||
— currently false. (Disclosure only; the op's handler is not executed.)
|
||||
**Fix:** enforce the same internal+ACL pre-checks inside the
|
||||
`services/schema` dispatch path (alkcall-side handler fix is the
|
||||
complete one; an alkhttp-side input check is the local one).
|
||||
|
||||
## PRJ-16b [major] — The published document contains a dangling `$ref: #/components/schemas/CallError`
|
||||
|
||||
**Verified:** YES. `to_openapi.rs:602` (BatchResultEntry.error refs
|
||||
`CallError`) vs `:387-413` (components defines `CallFailure` + per-code
|
||||
variants, no `CallError`). The openapiv3 validation test runs on an
|
||||
**empty registry** and openapiv3 does not resolve refs on parse, so
|
||||
nothing trips. Any generated client or validator consuming the doc
|
||||
fails to resolve. **Fix:** define the generic `CallError` component or
|
||||
point at the existing per-code variants; re-run the parse test on a
|
||||
populated registry.
|
||||
|
||||
## PRJ-17 [minor] — `HTTP_`-prefixed op errors declared at protocol statuses clobber the shared protocol responses
|
||||
|
||||
**Verified:** YES. `to_openapi.rs:694, 237-239, 295-300` — an op
|
||||
declaring `HTTP_404@404` replaces `#/components/responses/NotFound` on
|
||||
`/call`, losing `NOT_FOUND` (still emitted by the runtime), so every
|
||||
real 404 violates the documented schema. **Fix:** merge op codes into
|
||||
the shared response (oneOf append) instead of overwriting.
|
||||
|
||||
## PRJ-18/PRJ-19/PRJ-20/PRJ-21/PRJ-23/PRJ-24 [minor] — projection drift cluster
|
||||
|
||||
- **PRJ-18**: `/publish` 400 over-declares `INVALID_OPERATION_TYPE`;
|
||||
the runtime maps that condition to **401** without a token (the same
|
||||
document's 401 entry is the true one; two docs, one scenario).
|
||||
- **PRJ-19**: extractor plain-text `415` and data-error `422` bodies
|
||||
are undocumented on `/call//batch//subscribe` (axum 0.8 emits them).
|
||||
- **PRJ-20**: `/call` 401 schema under-declares
|
||||
`INVALID_OPERATION_TYPE` (Sub/Pub called unauthenticated → real 401
|
||||
with a code the enum doesn't allow; `/publish` got this right).
|
||||
- **PRJ-21**: `/batch` documents a 500 the runtime never emits (all
|
||||
dispatch failures are in-band entries).
|
||||
- **PRJ-23**: `x-operation-error-statuses` is emitted *inside*
|
||||
`components.schemas` (extension keys are legal on `components`, not
|
||||
as a schema name) — OAS-invalid on any registry with op errors; the
|
||||
validator test only ever runs on an empty registry.
|
||||
- **PRJ-24**: dead `search_filter` parameter in `handle_batch`
|
||||
(computed then discarded — vestigial).
|
||||
|
||||
## PRJ-22 [minor] — MCP `batch` tool lacks the 100-call cap its HTTP counterpart enforces
|
||||
|
||||
**Verified:** YES. `to_mcp.rs:215-247` vs `routes.rs:173-184` — a
|
||||
10,000-entry `calls` array executes serially in the dispatch spine (up
|
||||
to the 30 s deadline each) while `/batch` rejects at 101. **Fix:**
|
||||
enforce `MAX_BATCH_OPERATIONS` in the MCP `batch` tool.
|
||||
|
||||
## CON-15 [minor] — `from_mcp` tool-name sanitization is not injective; collisions resolve by silent last-write
|
||||
|
||||
**Verified:** YES. `from_mcp/mod.rs:231-252` + alkcall
|
||||
`registration.rs:108-127` — `"a/b"`, `"a b"`, `"a\tb"` all sanitize to
|
||||
`"a b"`; `OperationRegistry::register` is `HashMap::insert`, so the
|
||||
later remote tool silently replaces the earlier one. A remote server
|
||||
can shadow its own tools with no diagnostic. **Fix:** fail import
|
||||
naming the colliding tools (or reject whitespace-containing names —
|
||||
current rejection is `/` + whitespace *after* trim; padded variants
|
||||
still collide with their trimmed selves).
|
||||
|
||||
## CON-16 [minor] — Dead `Capabilities` entry still carries the upstream MCP token; struct doc contradicts the corrected module doc
|
||||
|
||||
**Verified:** YES. `from_mcp/mod.rs:176, 393-400` — the import-time
|
||||
token is duplicated into `HandlerRegistration.capabilities` where no
|
||||
code reads it (the handler pins the transport header at import; CON-05
|
||||
fixed the module doc but `mod.rs:56`'s struct doc still claims per-call
|
||||
credential injection). Secret-minimization violation in spirit; the
|
||||
token also propagates into every `OperationContext` clone. **Fix:**
|
||||
pass `Capabilities::new()` or actually source per-call auth; fix the
|
||||
struct doc.
|
||||
|
||||
## CON-08 (status change) — the `from_mcp` session leak is now honestly documented, not fixed
|
||||
|
||||
`std::mem::forget(running)` remains (`mod.rs:126`); no close/teardown
|
||||
handle exists; `mod.rs:16-22` documents it as an explicit limitation
|
||||
(remote session + SSE stream live until the remote times out; repeated
|
||||
imports accumulate). Accepted-for-v1 with contract documented —
|
||||
recording as PARTIAL-DOCUMENTED, remediation optional (an explicit
|
||||
`close()` would be the v1.1 lever, same note as review-001 CON-09).
|
||||
|
||||
## CON-11 residual — JSON-RPC-path `MCP_JRPC_*` codes are not in the declared `error_schemas`; `Cancelled` → retryable=true is unsafe for Mutations
|
||||
|
||||
**Verified:** YES. `from_mcp/mod.rs:211-229, 377-389` — the new
|
||||
`MCP_TRANSPORT_ERROR` is declared (good), but JSON-RPC-error-derived
|
||||
codes carry `http_status: None` and appear in no declared schema
|
||||
(projects nowhere in to_openapi; surfaces as runtime 500 — defensible
|
||||
under the PRJ-04 annotation model, but undeclared). And a `Cancelled`
|
||||
remote error classified `retryable: true` invites duplicate side
|
||||
effects on Mutation retries. Minor.
|
||||
|
||||
---
|
||||
|
||||
# Part G' — Coverage (cargo-llvm-cov re-run + classification)
|
||||
|
||||
Overall: **94.87% regions / 94.63% lines / 92.22% functions** (JSON +
|
||||
HTML-verified per line). The prior pass's gaps mostly closed:
|
||||
tungstenite pumps have 8 dedicated tests (COV-03 resolved), TLS/mTLS
|
||||
success paths run through the real constructors (COV-02 resolved),
|
||||
forward.rs rose to 91%. The residue:
|
||||
|
||||
## COV-11 — `client/http_client.rs` 63.2% lines / 53.3% functions: the redirect policy closure is 100% dark
|
||||
|
||||
The FWD-03 fix (`same_host_redirect_policy`, :300-316) — the crate's
|
||||
headline credential-exfiltration defense — has **zero runtime
|
||||
exercise**; no test in the crate follows any 3xx. Also dark: the retry
|
||||
gate's POST-bypass (:362-366), the wall-clock budget stop (:381), the
|
||||
PEM parse-failure arms (:487-507), and the `config()` accessor
|
||||
(:279-281, public API, 0 uses — also the untested half of the FWD-12
|
||||
atomicity story). **This is the top coverage gap in the crate** — see
|
||||
COV-09 for the wire tests to add (shared with CLI-02/CLI-03).
|
||||
|
||||
## COV-10 — `forward_stream`'s terminal arms are dark end-to-end
|
||||
|
||||
`forward.rs:887-925` (SSE parse-overflow → terminal error, transport
|
||||
error mid-stream → terminal error, EOF-flush of the pending event) and
|
||||
the streaming build/send-failure arms (:811-814, :851-853) plus the
|
||||
Once-path transport-error and binary-decode arms (:638-643, :656-658,
|
||||
:719-722). The parser is unit-tested; the wiring around it never
|
||||
received multi-chunk or erroring input over wire. Subscriptions are
|
||||
unbounded by design, so these arms are the *only* thing that ends a
|
||||
stuck subscription — they are also untested. Fix with three responder
|
||||
tests (oversized line, aborted socket, EOF-with-pending) + dead-port
|
||||
transport test.
|
||||
|
||||
## COV-11b — Deployment knobs dark in all binaries
|
||||
|
||||
`with_ws_idle_timeout` (`adapter.rs:228-242`) — the WS-13-relevant
|
||||
server-side knob — is called by no test; a regression silently disables
|
||||
the idle timer on the default surface while tungstenite-side unit tests
|
||||
stay green. Add the 150 ms-knob 1001-close integration test + a `None`
|
||||
variant. Likewise `to_mcp.rs:459-468` (`call_tool`, the rmcp-entered
|
||||
production dispatch) is dark — everything MCP is tested through a
|
||||
bypass harness (`invoke_tool`), so the real routing shell has never
|
||||
run.
|
||||
|
||||
## COV-12 — Small dark arms worth folding into planned tests
|
||||
|
||||
- `openapi_spec.rs:428-433` — the array-recursion arm never runs in
|
||||
tests (the code is *correct* — verified — but arrays-of-`$ref`
|
||||
schemas have no test; one-line addition to the existing $ref tests).
|
||||
- `openapi_spec.rs:561-566` — `style: simple` + `explode: true`
|
||||
rejection arm (the one loudness gate with a test hole).
|
||||
- `from_mcp/mod.rs:405-408` — non-object tool arguments wrapped as
|
||||
`{"value": …}` (real wire behavior, untested).
|
||||
- `to_mcp.rs:175, 221` — missing-argument validation for `schema` and
|
||||
`batch` tools (two of the four tools' argument contracts untested).
|
||||
- `routes.rs:263-266` — first-line-invalid-JSON `/publish` 400 (the
|
||||
header-phase variant; only the stream-phase variant is tested).
|
||||
- `decoy.rs:133-138` — directory→index.html resolution.
|
||||
- `routes.rs:430-431, 551-553` — body-read-failure → chunk error
|
||||
(client reset mid-upload).
|
||||
|
||||
## COV-13 — Dead code confirmed by coverage (delete, don't test)
|
||||
|
||||
`server/state.rs:75-83` (both `FromRef` impls — nothing extracts those
|
||||
types), `gateway/dispatch.rs:74-76, 79-81` (two accessors — and
|
||||
`resolve_bearer`'s doc promises an auth hook the middleware never
|
||||
calls: wire it or delete it), `websocket/upgrade.rs:145-147`, and
|
||||
`to_mcp.rs`'s `PRJ-24` dead parameter.
|
||||
|
||||
Leave-uncovered (verified unreachable-by-construction): the reserved-
|
||||
path probe handler, `StreamError` glue, serialize-failure guards on
|
||||
acyclic inputs, depth-guard duplicates, `test-support` helper arms, and
|
||||
the stat/read race guards. Full per-range table in the coverage JSON
|
||||
export retained at `/tmp` during this session.
|
||||
|
||||
---
|
||||
|
||||
# Cross-cutting: what is solid (verified this pass)
|
||||
|
||||
- **Every review-001 security fix verified fixed with tests:** extra-
|
||||
route auth layering (now merged-before-layer, opt-out tested),
|
||||
`/schema` internal+ACL pre-checks (404/403, anonymous tested), MCP
|
||||
body limit (declared-CL and chunked 413 tests), hyper timer + header
|
||||
read timeout + keep-alive knobs, reservation-collision construction
|
||||
panics, decoy 405 + decode fixes (`%2B`, UTF-8), `/openapi.json`
|
||||
build-once cache with generic 500, gateway publish_schema fail-closed
|
||||
validation (schema-rejection, first-chunk, spy-handler-zero-chunks,
|
||||
hot-reload tests), no-store+Vary on per-identity GETs, SSE
|
||||
terminality + 15 s keep-alive + `retry:`, batch cap/request-ids,
|
||||
WS EOF watch signal (retention-tested), WS byte caps (accounting
|
||||
verified sound), 1 MiB inbound WS caps (integration-tested), shutdown
|
||||
close-frame cascade (acceptance-tested), session registry + 503 cap
|
||||
(slot-freeing tested), from_wss plaintext-`ws://` refusal (message-
|
||||
asserted), `Secret<String>` builders (redacted-Debug tested), typed
|
||||
401 classification (port-40101 false positive tested).
|
||||
- **Credential hygiene held:** no tracing of secrets anywhere in the
|
||||
forwarding path; loud (non-echoing) malformed-credential errors on
|
||||
all four value arms; redirect host-equality check compares
|
||||
scheme+host+port with normalized hosts; no upstream credential
|
||||
material appears in the served OpenAPI document (verified —
|
||||
`HandlerRegistration.capabilities` is invisible to `list_operations`).
|
||||
- **The no-env-vars invariant, Internal-by-default visibility, and
|
||||
`HTTP_<status>` discipline all still hold** (negative tests present).
|
||||
- **The to_openapi document is genuinely runtime-truthful now** except
|
||||
the specific drifts filed above (PRJ-16..23): envelope shape, 422
|
||||
body, error-status annotations, in-band SSE contract, and
|
||||
securitySchemes were each cross-checked against routes/error sources
|
||||
and their tests.
|
||||
- **The WS memory story is bounded in-crate:** read 64×1 MiB, write
|
||||
64×16 MiB + 16 MiB pending, no new unbounded accumulators (the two
|
||||
remaining >16 MiB buffers are alkcall-owned, unchanged).
|
||||
|
||||
---
|
||||
|
||||
# Test gaps that would have caught the findings (semantic)
|
||||
|
||||
1. A never-newline `/publish` body (GW-15's unbounded heap) — the
|
||||
existing line-cap test exercises the 400 short-circuit, not the
|
||||
stream path its name claims.
|
||||
2. A wrong-method probe on an **extra route** in decoy mode (SRV-12).
|
||||
3. A token-resolution count on the WS route (SRV-11's double-resolve).
|
||||
4. A path-param value of exactly `..`/`.` (FWD-13 — the whole
|
||||
dot-segment test family uses multi-segment forms the encoder
|
||||
catches).
|
||||
5. The dangling-`CallError` ref (PRJ-16b) — the openapiv3 parse test
|
||||
never runs on a populated registry.
|
||||
6. `HTTP_404@404`-style op errors (PRJ-17's clobber); a 401-body
|
||||
conformance test for `/call` (PRJ-20).
|
||||
7. A cycling-cursor MCP server (CON-14); a sanitize-collision pair
|
||||
(CON-15).
|
||||
8. The 30-level shared-`$ref` chain (OAI-11 — a bounded-time assert).
|
||||
9. YAML duplicate keys / `.inf` / merge keys at the `from_yaml` seam
|
||||
(OAI-12).
|
||||
10. Advertised-vs-enforced round trip: construct a doc, import, assert
|
||||
`/schema`'s input_schema is exactly what `build_request` enforces
|
||||
for every branch (JS-01 and OAI-18 both survived this missing
|
||||
chain test).
|
||||
11. The with-properties-absent placeholder case (JS-01) — the test
|
||||
covers only properties-present.
|
||||
12. `services/schema` invoked through `/call` with an Internal op name
|
||||
(PRJ-16) — the route-level tests cover GET `/schema` only.
|
||||
|
||||
---
|
||||
|
||||
# Remediation plan
|
||||
|
||||
Sequenced by dependency and severity; each unit independently
|
||||
shippable with its acceptance gate = the test that would have caught
|
||||
its worst finding.
|
||||
|
||||
## Unit 1 — Security-critical (GW-15, PRJ-16, FWD-13, FWD-16, OAI-11)
|
||||
|
||||
The five findings with real attack or availability impact.
|
||||
- GW-15: cap `BufferedLines` growth pre-newline + explicit body-limit
|
||||
layer on `/publish`. Gate: never-newline upload rejected at the cap.
|
||||
- PRJ-16: internal+ACL pre-check on the `services/schema` dispatch
|
||||
input (alkcall handler preferred; alkhttp-side input check as the
|
||||
local layer). Gate: `/call` on `services/schema{internal-op}` → 404.
|
||||
- FWD-13: reject `.`/`..` rendered path values (post-normalization
|
||||
segment-count invariant also acceptable). Gate: lone-`..` value →
|
||||
INVALID_INPUT.
|
||||
- FWD-16: missing capability + auth_scheme → INTERNAL. Gate: the
|
||||
silent arm's test.
|
||||
- OAI-11: memoize cycle-free `$ref` expansions (or node-count budget).
|
||||
Gate: 30-level chain imports in bounded time.
|
||||
|
||||
## Unit 2 — Timeout/terminality coherence (WS-13, FWD-14, FWD-15, CLI-01, GW-17, CON-14)
|
||||
|
||||
One coherent theme: the new policy knobs (idle, budget, ceiling,
|
||||
timeout) interact with progress and terminality incorrectly.
|
||||
- WS-13: progress-based deadline + keepalive decision + two acceptance
|
||||
tests (dribble bounded; silent-but-alive survives).
|
||||
- FWD-14/FWD-15: stream byte cap + subscription sends with
|
||||
`request_timeout: None`.
|
||||
- CLI-01: budget-aware Retry-After sleep.
|
||||
- GW-17: wrap sink completion in the 30 s deadline (or fix the doc).
|
||||
- CON-14: dead-connection fast-fail + bounded sweep exit.
|
||||
|
||||
## Unit 3 — Projection + doc truthfulness (PRJ-16b, PRJ-17..24, WS-15) + gateway status drift (GW-16)
|
||||
|
||||
Mostly mechanical; golden-doc work regains its review-001 lesson: the
|
||||
validation tests must run on a *populated* registry.
|
||||
|
||||
## Unit 4 — Spec-import gaps (OAI-12, OAI-13, OAI-10/14/15/16/17/19, JS-01/JS-02/JS-03, COV-12's openapi arms)
|
||||
|
||||
YAML normalization pass, path-item parameters + wildcard responses, and
|
||||
the loudness-matrix completion. JS-01 is a one-line fix with a
|
||||
one-line test — fold it into any passing commit.
|
||||
|
||||
## Unit 5 — WS polish (WS-14..19) + test-gaps batch (CLI-02/CLI-03, COV-09..12)
|
||||
|
||||
Wire tests for the redirect/retry/timeout policy stack, the streaming
|
||||
terminal arms, the idle-knob integration test, plus the small arm tests.
|
||||
Also delete the COV-13 dead code.
|
||||
|
||||
---
|
||||
|
||||
## Verification log (this pass)
|
||||
|
||||
- All findings carry `file:line` references verified against tree
|
||||
`91483a7`. The consolidating pass directly re-verified: GW-15
|
||||
(`BufferedLines` unbounded growth, raw-Body handler, extractor-limit
|
||||
bypass), PRJ-16 (alkcall `services_schema_handler` bare-registration
|
||||
disclosure + alkhttp outer-name-only pre-check), FWD-13 (encode set
|
||||
+ `Url::set_path` normalization against url 2.5.8), WS-13 (timer
|
||||
placement), JS-01 (`is_empty → false` early return), PRJ-16b (dangling
|
||||
ref vs components contents), and the array-`$ref` recursion arm
|
||||
(correct — one agent claim demoted to a coverage-only gap).
|
||||
- Empirical verifications (scratch harnesses outside the repo, no repo
|
||||
changes): SRV-11/SRV-12 router-shape probes (extra-route 405 shape,
|
||||
double token resolution), OAI-11 exponential-ref expansion (7.3M
|
||||
visits at 20 levels), OAI-12 YAML corruptions, FWD-13 dot-segment
|
||||
normalization, reqwest timeout-into-stream behavior.
|
||||
- Coverage figures: `cargo llvm-cov --all-features` (summary + JSON +
|
||||
HTML), 2026-08-30.
|
||||
- Six subsystem passes ran in parallel over disjoint file sets; two
|
||||
agent findings were rejected at consolidation (coverage-overreported
|
||||
dark array-`$ref` arm — code verified correct; WS-16 — duplicate of
|
||||
CON-14) and several "uncovered" flags were reclassified as
|
||||
cross-binary artifacts after HTML-line verification.
|
||||
Reference in New Issue
Block a user