--- id: review-001-publish-schema-validation-robust name: Fix /publish schema validation fail-open + per-request recompilation (post-remediation) status: completed depends_on: [] scope: narrow risk: high impact: component level: implementation tags: [gateway, review-001, follow-up] --- ## Description Follow-up to review-001-gateway-publish-semantics (GW-01): the landed fix compiles the op's `publish_schema` **on every `/publish` request** (`src/gateway/routes.rs:254-268`) and, when compilation fails, logs a warning and proceeds with **no validation** — chunks flow to the handler unvalidated. Two remediation-introduced problems: - **Fail-open on compile failure** — this reintroduces, conditionally, the exact transport-dependent invariant GW-01 closed: a handler that registered a `publish_schema` (or a remote import whose schema is un-compilable by jsonschema 0.46's dialect) silently receives arbitrary JSON over the HTTP path that the wire path would have aborted. A registry with one broken schema becomes an unvalidated ingest path with only a `tracing::warn` as the tell. - **Per-request compile cost** — `jsonschema::options().build()` on the hot path is CPU + allocation per publish call; a hostile or merely busy caller multiplies it. Fix direction (implementer's choice on mechanism): compile **once per registration** — cache the compiled `Validator` (or the compile error) against the op in the gateway state, invalidated on re-registration — and make compile failure **loud and closed**: the request path surfaces a server-fault (500, `INTERNAL`-class with the compile error logged at error level), never a silent skip. Consider whether adapters can reject un-compilable `publish_schema` values at import/registration time so the failure lands before any traffic (adapter-side validation where the schema originates), with the gateway cache as defense in depth. ## Acceptance Criteria - [x] Compiled validator (or error) cached per registration; no per-request recompile (test: compile count / perf shape not asserted, but code path is registration-keyed) - [x] Un-compilable `publish_schema` → request fails loudly (500/INTERNAL, error logged), chunks never flow unvalidated (test) - [x] Schema re-registration (hot reload) picks up the new schema (test) - [x] A `publish_schema`-registered Pub op still rejects an invalid chunk (existing GW-01 gate stays green) - [x] `cargo test` and `cargo clippy --all-targets -- -D warnings` pass ## References - docs/reviews/001-initial-implementation-review.md (Part C, GW-01) - tasks/gateway/review-001-gateway-publish-semantics.md (the landed fix + rationale) - src/gateway/routes.rs:254-268 (the fail-open) ## Notes Mechanism: new `src/gateway/schema_cache.rs` — `PublishSchemaCache`, a cloneable `Arc>>` held on `RouterState` (built empty per `HttpAdapter`/router construction; `GatewayState` extracts it via `FromRef`). Each entry stores the compiled `Arc` (or a `Failed` marker) **plus the raw schema `Value`** that produced it. Lookup compares the registry's current `publish_schema` value against the cached one: equal → serve the cached validator/failure; different → recompile. This gives hot-reload correctness by value equality without lifetime-coupling to registry internals; under the documented assemble-then-serve invariant (the same one `CachedOpenAPIDoc` relies on) each schema compiles exactly once per process, and the value check is defense in depth for re-assembly paths. Fail-closed semantics: a failed compile is **cached as a failure** — the error is logged once at `error` level (never retried or re-logged per request), and the wire message is a generic INTERNAL naming the compile failure without echoing schema internals (an untrusted schema's error text must not reach the wire; same discipline as the `/openapi.json` cache-miss 500). Resolution is **lazy**, resolved against the cache inside the chunk stream's first poll, not in the handler prologue: `invoke_sink` owns the 404/403/422 pre-checks (GW-11), so an unknown/internal/wrong-type op is rejected before any schema lookup or compile happens — no cache population or error-log spam from ops a caller cannot reach. Stream terminality fix (found by the spy-handler test): the old `NdjsonChunkStream` yielded `Err` items but kept streaming — a handler that drained the stream (instead of aborting on first `Err`) would have kept receiving **unvalidated** chunks after a violation or a compile failure. The wire pump (`alkcall dispatch.rs:599-600`) does `send(Err)` + `break`, i.e. the error item is terminal and the channel closes. `NdjsonChunkStream` now mirrors that exactly: the first `Err` item (schema violation, bad JSON, read failure, or compile failure) sets `done` and the stream ends — `Ok` chunks can never flow after an error on this transport either. Adapter-side registration-time rejection was considered and **not** pursued here: alkcall's `OperationRegistry::register` does not observe `publish_schema` compilability, and adding that would be an alkcall surface change (this crate consumes `alkcall = "0.1.1"` from crates- io) — the gateway cache is the defense in depth the task prescribed. Worth noting for alkcall: the identical compile-fail-open pattern exists on the wire path (`alkcall/src/protocol/dispatch.rs:356-366`, warn-and-skip) — upstream fix candidate, out of scope for this crate. ## Summary Implemented in three parts: - **`src/gateway/schema_cache.rs`**: `PublishSchemaCache` — compile- once, value-keyed invalidation, cached failures, `error!`-level compile logging, generic non-leaking wire error (`CompileFailed::call_error` → `CallError::internal`). 5 unit tests (no-schema/unknown-op → `Ok(None)`; compile+validate; `Arc::ptr_eq` cache identity; failure stays failed). - **`src/gateway/routes.rs`**: `publish_handler` uses the cache; `PublishSchemaState` (Unresolved → Unvalidated/Validated/Failed) resolves lazily on first chunk poll, after `invoke_sink`'s pre-checks; `NdjsonChunkStream` is terminal-on-first-`Err` (wire parity). Route tests: uncompilable schema → 500 `INTERNAL` with non-echoing message; spy handler receives 0 `Ok` chunks and exactly one terminal `INTERNAL` error; hot-reload re-registration picks up the replacement schema (validator identity + semantics asserted); all pre-existing GW-01 gates (reject-invalid, accept-valid, first-chunk validation, no-schema passthrough) stay green. - **`src/server/{state,adapter}.rs`** + `gateway/mod.rs`: cache plumbed through `RouterState`/`GatewayState` at all construction sites. Verification: `cargo test` 308 passed (default), `--all-features` all suites green (379+), `cargo clippy --all-targets -- -D warnings` clean (default + all-features), `cargo fmt --check` clean. `--no-default-features` warnings are pre-existing on the base commit.