fix(gateway): fail-closed publish_schema compile + compile-once cache (GW-01 follow-up)
- new gateway::schema_cache — PublishSchemaCache: compile the op's publish_schema once per registration (value-keyed invalidation for hot reload), cache compile failures (logged once at error level, never retried per request) - /publish compile failure is now fail-closed: the chunk stream terminates with INTERNAL (500), the error text stays in the log (no schema internals on the wire) — the per-request warn-and-skip unvalidated ingest path is removed - schema resolution is lazy (first chunk poll, after invoke_sink's 404/403/422 pre-checks — GW-11 order preserved) and keyed by schema value, so re-registration/hot reload is picked up (test) - NdjsonChunkStream: first Err item is terminal (done + stream end), mirroring the wire pump's send(Err) + break — Ok chunks can never follow an error on the HTTP path either (found by spy-handler test) Verified: cargo test (308), cargo test --all-features, clippy --all-targets -D warnings (default + all-features), fmt --check. Tasks: review-001-publish-schema-validation-robust
This commit is contained in:
@@ -1,7 +1,7 @@
|
||||
---
|
||||
id: review-001-publish-schema-validation-robust
|
||||
name: Fix /publish schema validation fail-open + per-request recompilation (post-remediation)
|
||||
status: pending
|
||||
status: completed
|
||||
depends_on: []
|
||||
scope: narrow
|
||||
risk: high
|
||||
@@ -41,11 +41,11 @@ schema originates), with the gateway cache as defense in depth.
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
- [ ] Compiled validator (or error) cached per registration; no per-request recompile (test: compile count / perf shape not asserted, but code path is registration-keyed)
|
||||
- [ ] Un-compilable `publish_schema` → request fails loudly (500/INTERNAL, error logged), chunks never flow unvalidated (test)
|
||||
- [ ] Schema re-registration (hot reload) picks up the new schema (test)
|
||||
- [ ] A `publish_schema`-registered Pub op still rejects an invalid chunk (existing GW-01 gate stays green)
|
||||
- [ ] `cargo test` and `cargo clippy --all-targets -- -D warnings` pass
|
||||
- [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
|
||||
|
||||
@@ -55,13 +55,78 @@ schema originates), with the gateway cache as defense in depth.
|
||||
|
||||
## Notes
|
||||
|
||||
> Agent fills during implementation. Found in the post-remediation
|
||||
> sweep — not a Review 001 finding itself. Fail-open is the priority
|
||||
> half; caching is the efficiency half. Registration-side (adapter)
|
||||
> rejection at import is the preferred end state if alkcall's
|
||||
> registration surface allows observing `publish_schema` at
|
||||
> HandlerRegistration time.
|
||||
Mechanism: new `src/gateway/schema_cache.rs` —
|
||||
`PublishSchemaCache`, a cloneable `Arc<RwLock<HashMap<String,
|
||||
CacheEntry>>>` held on `RouterState` (built empty per
|
||||
`HttpAdapter`/router construction; `GatewayState` extracts it via
|
||||
`FromRef`). Each entry stores the compiled `Arc<jsonschema::Validator>`
|
||||
(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
|
||||
|
||||
> Filled on completion.
|
||||
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.
|
||||
Reference in New Issue
Block a user