fix: remediate consumer findings CF-001..004 (alkhttp ledger)

- CF-004: services_schema_handler now applies the same visibility +
  AccessControl gates as invoke() (identity resolution mirrors invoke:
  handler_identity under internal). Restricted ops return spec-404 NOT_FOUND
  — matches "restricted ops don't exist" and leaks nothing about the
  restricted surface. Closes the unauthenticated /call-path disclosure.
- CF-003: publish_schema compiled at registration time (both
  OperationRegistry::register and OperationRegistryBuilder::store);
  un-compilable schemas are a registration error — an unvalidated ingest
  path can no longer be constructed. Compiled validator cached per-op
  (publish_validator) and consumed by dispatch; per-request compile gone.
  BEHAVIOR CHANGE: register/builder reject un-compilable publish_schema.
- CF-002: demux TooLarge skip streams through a fixed 64 KiB buffer
  instead of allocating the peer-declared length (u32, up to ~4 GiB);
  cumulative 256 MiB skipped-bytes budget tears down dribbling peers.
  Existing resync test passes unchanged.
- CF-001: new retryable CallError::connection_closed (CONNECTION_CLOSED)
  applied only where the call is provably undelivered — request-frame
  write failures on all consumer paths (call/subscribe/publish, both
  stream modes; publish pump tags write stages). Mid-publish failures and
  producer-side fail_all stay non-retryable INTERNAL (delivery ambiguous).
  New code string is additive; retryable flag is the machine-readable
  signal.

Verification: cargo test (558 pass, 15 new), clippy --all-targets -D
warnings, fmt --check, wasm32-unknown-unknown check.
This commit is contained in:
glm-5.3-flash committed 2026-08-31 08:22:55 +00:00
1 parent a2d72f9737
commit 8cb2a6eb6d
8 files changed
+613 -167

No files matched your search

+75 -115
View File
@@ -11,141 +11,101 @@ Format: date | found-in (alkhttp context) | severity | status.
## Open
### CF-004 — `services_schema_handler` discloses Internal/ACL-restricted op specs — no visibility or AccessControl check (2026-08-30)
(none)
---
## Resolved
### CF-004 — `services_schema_handler` discloses Internal/ACL-restricted op specs — no visibility or AccessControl check (2026-08-30) — RESOLVED 2026-08-31
- **Found in:** alkhttp Review 002, finding PRJ-16
(`alkhttp/docs/reviews/002-post-remediation-review.md`, Part F',
verified against tree `91483a7` + alkcall source). Filed to this
ledger 2026-08-30.
`src/registry/discovery.rs:327-343` (`services_schema_handler`) —
the handler does a bare `registry.registration(&name)` and returns
the handler did a bare `registry.registration(&name)` and returned
`spec_to_json(&reg.spec)` verbatim, with **no `Visibility` check and
no `AccessControl::check(peer_identity)`**. The built-in op itself is
External with default ACL, so alkhttp's per-route pre-checks (which
guard only the *outer* op name `services/schema`) cannot see the
inner name a caller smuggles in the input: `POST /call
{"operation":"services/schema","input":{"name":"secret/op"}}` returns
the full spec — input/output/error schemas and
`access_control.required_scopes` — of any Internal or admin-scoped
op, unauthenticated. The GET `/schema` route and the MCP `schema`
tool both enforce internal-op + ACL pre-checks on the same
underlying operation; the `/call` path (and the MCP `call` tool
dispatching through it) is the hole.
- **Impact:** unauthenticated disclosure of the complete contract of
every Internal operation, including the exact scopes needed for
escalation (the "reads the scopes needed to escalate" shape review
001's PRJ-06 called out). Disclosure only — the op's handler is not
executed.
- **Suggested direction:** enforce inside the handler (the complete
fix, covers every transport): resolve the peer identity from
`OperationContext`, then apply the same two checks the HTTP route
does — `is_internal_op`-equivalent visibility rejection (spec-404)
and `AccessControl::check` → FORBIDDEN envelope. Defensive note: some
existing wire tests exercise `services/schema` with default-ACL ops
and would still pass; the new behavior is only observable against
Internal/ACL-restricted registrations.
- **alkhttp side:** an alkhttp-local mitigation (input pre-check in
`call_handler`'s dispatch path) is planned as review-002 remediation
regardless — but it is a per-transport patch; the handler-level fix
here is the one that closes the hole for the wire and every future
transport.
- **Status:** open — no alkcall change made.
no `AccessControl::check(peer_identity)`**.
- **Fix:** the handler now applies the same two gates as
`OperationRegistry::invoke` — the Internal-visibility rejection
(`!ctx.internal` → spec-404) and `AccessControl::check` with the same
identity resolution invoke() uses (`handler_identity` when internal,
`identity` otherwise). Restricted ops return `NOT_FOUND` (spec-404)
rather than `FORBIDDEN` — no information leaks about the restricted
surface's existence or shape. The gate is in the handler itself, so
every transport (wire `/call`, HTTP routes, MCP tools) is covered.
- **Status:** resolved — 2026-08-30.
### CF-003 — `publish_schema` compile failure is fail-open on the wire dispatch path (2026-08-30)
### CF-003 — `publish_schema` compile failure is fail-open on the wire dispatch path (2026-08-30) — RESOLVED 2026-08-30
- **Found in:** alkhttp Review 001 post-remediation task
`review-001-publish-schema-validation-robust` (the GW-01/#1 follow-up
work). While fixing alkhttp's HTTP-side instance of the pattern, the
identical instance was verified on the wire dispatch path:
`src/protocol/dispatch.rs:352-366` (`Dispatcher::dispatch`,
`OperationType::Pub` arm) — when a Pub op's `publish_schema` fails to
**compile** (`jsonschema::options().build(&schema)` errors, e.g. a
non-array `"required"`, a non-string `"type"`, an invalid `pattern`
regex), the pump logs `warn!("publish_schema failed to compile;
chunks will not be validated")` and proceeds with `validator: None`.
- **Impact:** silent unvalidated ingest, per-request. A Pub op (local
registration or remote import) whose schema is un-compilable by the
pinned jsonschema dialect silently receives arbitrary JSON over the
wire that the registration *declares* validated — the handler's
schema guarantee is transport-dependent and quietly false. One broken
schema in the registry turns that op into an unvalidated ingest path
with only a warn log as the tell. (Per-request recompile cost is the
secondary issue; alkhttp's gateway cache eliminated both.)
- **Suggested direction:** fail-closed — on compile failure, reject the
chunk (or abort the sink dispatch) with `INTERNAL`-class
`CallError` naming the compile failure in the log only (untrusted
schema text off the wire), never a silent skip; cache the compiled
validator per registration (hot-reload by schema-value comparison;
see alkhttp's `src/gateway/schema_cache.rs` for the worked shape).
Complementary hygiene: reject un-compilable `publish_schema` values
at **registration time** (`OperationRegistry::register` observing
`spec.publish_schema`, or `with_publish_schema` validating eagerly)
so the failure never reaches traffic.
- **alkhttp side:** fixed in alkhttp `1572a9d` (gateway cache + lazy
resolution + stream-terminal fail-closed). The wire path is out of
this crate's scope (AGENTS.md: no forking alkcall types; the pump is
alkcall-internal), so the same invariant currently holds only on the
HTTP path.
- **Status:** open — no alkcall change made.
`OperationType::Pub` arm) — when a Pub op's `publish_schema` failed to
**compile**, the pump logged a warn and proceeded with
`validator: None` (silent unvalidated ingest, plus per-request
recompile cost).
- **Fix:** fail-closed at the source — `publish_schema` is now compiled
at **registration time** in both insertion points
(`OperationRegistry::register` and `OperationRegistryBuilder::store`);
an un-compilable schema is a registration error, so an
unvalidated-ingest path can never be constructed. The compiled
validator is cached per-op (`OperationRegistry::publish_validator`)
and the dispatch path consumes the cache — the per-request compile is
gone. The `from_call` import path inherits the guarantee (imports
register through the same choke point); forwarded-chunk validation on
the producer side is the remote peer's dispatch path and now inherits
the same fail-closed property.
- **Behavior change (semver-relevant):** `register`/builder methods
return `Err` for an un-compilable `publish_schema` (previously:
accepted). Code that registered garbage schemas will now get a
registration error instead of a silently-unvalidated op.
- **Status:** resolved — 2026-08-30.
### CF-002 — demux `TooLarge` skip allocates the full peer-declared length up front (up to ~4 GiB from an 8-byte header) (2026-08-30)
### CF-002 — demux `TooLarge` skip allocates the full peer-declared length up front (up to ~4 GiB from an 8-byte header) (2026-08-30) — RESOLVED 2026-08-30
- **Found in:** alkhttp Review 001, finding WS-12 (cross-crate;
`docs/reviews/001-initial-implementation-review.md`, Part B,
verified against tree `4a825d3`). Filed to this ledger 2026-08-30.
`src/channels/adapter.rs:144` (`run_demux_loop_for_client`) — when a
parsed chunk header carries `length > MAX_CHUNK_LEN`, the
`ChunkError::TooLarge` arm allocates
`src/channels/adapter.rs:144` (`run_demux_loop_for_client`) — the
`ChunkError::TooLarge` arm allocated
`let mut discard = vec![0u8; length as usize];` **before** reading:
the buffer is sized from the peer's untrusted 8-byte header, and
`length` is a `u32` (`0xFFFFFFFF` ≈ 4 GiB).
- **Impact:** memory-shape DoS, not a correctness bug (the resync/skip
itself is correct — alkcall's own resync test covers it). Any
authenticated peer can send header `[ch][len = 0xFFFFFFFF]` and then
dribble; the allocation is pinned for as long as the payload takes to
arrive (a dribbling peer can hold it indefinitely). Via the alkhttp WS
path this is trivially reachable by any authenticated browser; K
concurrent connections × 4 GiB headers → OOM before the dribble even
matters. The *normal* payload arm (`:159`, `vec![0u8;
header.length as usize]`) is safe — `parse_header` rejects
`length > MAX_CHUNK_LEN`, so it is bounded at 16 MiB; only the
`TooLarge` skip arm is unbounded.
- **Suggested direction:** stream-skip with a bounded fixed buffer
(e.g. a 64 KiB stack/fixed buffer, `read_exact` loop consuming
`length` bytes) instead of the pre-sized allocation. Optionally bound
cumulative skipped bytes per connection before tearing it down, so a
peer cannot loop `TooLarge` headers to burn bandwidth/CPU forever.
- **alkhttp side:** nothing to change there — this is demux-internal.
alkhttp's own read-side hardening (review-001-ws-pump-consolidation:
explicit inbound `max_message_size`/`max_frame_size`, idle-read
timeout) bounds the WS ingress, but a header that survives those caps
still hits this arm, so the fix belongs on the alkcall side.
- **Status:** open — no alkcall change made.
the buffer was sized from the peer's untrusted 8-byte header (`u32`
`length`, up to ~4 GiB).
- **Fix:** stream-skip with a fixed 64 KiB buffer + `read_exact` loop
consuming `length` bytes — no allocation sized from peer input.
Plus a cumulative skipped-bytes budget (256 MiB) that tears down the
connection when a peer loops `TooLarge` headers to burn bandwidth/CPU.
The budget resets on each valid chunk, so legitimate isolated
oversized chunks (the resync path, covered by the existing test) are
unaffected. The existing `demux_resyncs_after_oversized_chunk` test
passes unchanged.
- **Status:** resolved — 2026-08-30.
### CF-001 — `call_single_stream` write-failure maps to non-retryable `INTERNAL` (2026-08-29)
### CF-001 — `call_single_stream` write-failure maps to non-retryable `INTERNAL` (2026-08-29) — RESOLVED 2026-08-30
- **Found in:** alkhttp `from_wss` drop-monitor race tests (WS-02/CON-02,
review-001-ws-eof-signal). When the transport mux dies mid-call, the
write path fails fast and alkcall's `call_single_stream` write-failure
mapping produces a non-retryable
`INTERNAL: failed to write request frame` — even though the call never
reached the producer and a reconnect/retry would be safe.
- **Impact:** consumers that drop connections under load (WS EOF, network
flaps) get non-retryable errors for in-flight calls that are provably
not processed. Callers must either tolerate-or-discriminate two
envelope shapes for the same transport-death race, or miss retry
opportunities.
- **Suggested direction:** classify write failures that occur before any
response frame could arrive as retryable (`CONNECTION_CLOSED` or
equivalent) — or expose the distinction so consumers can decide.
- **alkhttp side:** the race test tolerates both outcomes for now
(asserts retryable only when the call is provably in-flight). If/when
alkcall fixes the mapping, alkhttp's ws-eof-signal test should tighten
back to retryable-only.
- **Status:** open — no alkcall change made.
---
## Resolved
(empty)
review-001-ws-eof-signal). When the transport mux died mid-call, the
write path failed fast and the write-failure mapping produced a
non-retryable `INTERNAL: failed to write request frame` — even though
the call never reached the producer and a reconnect/retry would be
safe.
- **Fix:** new retryable `CallError::connection_closed` constructor
(`CONNECTION_CLOSED` code, `retryable: true`), applied **only** to
write failures where the call is provably undelivered:
`call.requested` frame write failures on all consumer paths
(`call`/`subscribe`/`publish`, single-stream and stream-per-request;
the publish pump tags its write stages so only the request-frame
stage is retryable). Mid-publish and completed-frame write failures
stay `INTERNAL` (delivery ambiguous — retry unsafe), and the
producer-side `fail_all(...)` on connection close stays `INTERNAL`.
The new code string is additive to the wire error vocabulary;
`CONNECTION_CLOSED` is new — consumers should treat unknown codes per
their existing policy (the `retryable` flag is the machine-readable
signal).
- **Status:** resolved — 2026-08-30. alkhttp follow-up: the
`review-001-ws-eof-signal` race test can now tighten back to
retryable-only asserts.