glm-5.3-flash
2ec02fd578
Decision: advertise == enforce. The key allowlist (OAI-02) stays as the first gate with its established unknown-key message; a compiled leaf validator now runs second, so required/type/enum/pattern/bounds violations surface as INVALID_INPUT 422 naming the keyword — not as upstream round-trips. - new src/adapters/input_validation.rs: CompiledInputSchema compiles an op's input_schema once at import with the jsonschema crate (same 2020-12 dialect publish_schema uses) and validates peer input at call time; the compile-time copy is hardened closed-by-default (additionalProperties: false injected when absent) so the validator reproduces the allowlist's unknown-key semantics; explicit additionalProperties:true catch-all and schema values are preserved; the original spec value is never mutated - from_openapi/from_jsonschema import(): compile per registration, capture the validator in the handler closure (re-import recompiles — the closure capture is the invalidation story); a non-compilable input schema fails import loudly (AdapterError::SchemaParse naming the operation), matching the publish_schema fail-closed precedent - from_openapi generated input schemas now carry additionalProperties:false explicitly, so the /schema advert states the enforced rule and external schema-driven validators reach the same verdicts - forward/forward_stream/build_request gain an Option<&CompiledInputSchema> parameter; enforcement runs after the allowlist - round-trip test (review 002 Test-gap 10): the /schema-exported input_schema is compiled with the same validator and driven against build_request over a 10-input violation matrix — accept-sets exactly equal in both directions; the chain-test that lets advertise/enforce drift surface as a CI failure - ADR-066: new decision section (advertise==enforce) with the trust- boundary reasoning and the rejected option (b) rationale - module + enforce_input_schema docs updated to the two-gate shape cargo test --all-features 596 pass; clippy --all-features/-D warnings, fmt, doc gates clean. docs(tasks): mark review-002-fu-oai18-decision done
111 lines
5.7 KiB
Markdown
111 lines
5.7 KiB
Markdown
---
|
|
id: review-002-fu-oai18-decision
|
|
name: OAI-18 advertise-vs-enforce decision — enforce leaf constraints or scope the advert (OAI-18)
|
|
status: done
|
|
depends_on: []
|
|
scope: moderate
|
|
risk: medium
|
|
impact: component
|
|
level: implementation
|
|
tags: [adapters, review-002, follow-up, from-openapi, from-jsonschema]
|
|
---
|
|
|
|
## Description
|
|
|
|
Bracketed follow-up item 1 — the one OAI finding the review-002 bulk
|
|
deliberately deferred (Unit 4 landed OAI-12/13 and the loudness
|
|
cluster; OAI-18 was re-evaluated afterwards per the bracket plan). The
|
|
decision was re-validated against the post-bulk tree (43 commits,
|
|
`0a932e5`+): the deferred status still stands, and this task now
|
|
carries it.
|
|
|
|
**The gap (review 002 OAI-18, verified at tree `91483a7`):**
|
|
`enforce_input_schema` (`src/adapters/forward.rs:455-499`) enforces a
|
|
**key allowlist** only. The `/schema` surface advertises full JSON
|
|
Schema (`required`, `enum`, `pattern`, `minLength`, value types) via
|
|
`OperationSpec.input_schema`, but at call time:
|
|
|
|
- `required: [id]` accepts `{}` (the missing key forwards → upstream
|
|
422 instead of the gateway's own `INVALID_INPUT 422`)
|
|
- a `{"type": "string"}` property sent as an object serializes as JSON
|
|
text into the query string (or a header value)
|
|
- `enum`/`minimum`/`pattern` are never consulted
|
|
|
|
Advertise/enforce drift: the contract the gateway publishes is broader
|
|
than the one it defends. The review-001 OAI-02 *unknown-key* vector
|
|
(undeclared keys shaping the request) is closed; this is the
|
|
declared-but-unchecked remainder. Nothing changed this in the bulk —
|
|
re-verified `forward.rs:455-499` is still membership-only at `0a932e5`.
|
|
|
|
**The decision this task must make** (implement with the decision, or
|
|
record its explicit rejection):
|
|
|
|
- **(a) Enforce leaf constraints**: compile the full input schema per
|
|
op with the `jsonschema` crate (already a dependency via
|
|
`gateway/schema_cache.rs`) and validate peer input at call time,
|
|
reusing the `PublishSchemaCache` compile-once pattern. This makes
|
|
advertise == enforce exactly, at the cost of a per-op compile and
|
|
schema-dialect semantics (2020-12; same dialect publish_schema
|
|
already uses, so the crate is consistent).
|
|
- **(b) Scope the advert**: instead of enforcing, project a
|
|
reduced input schema into `/schema` — describe only what is enforced
|
|
(keys + types, drop `required`/`enum`/`pattern` from what
|
|
`spec_to_json` advertises) — so the contract and the enforcement
|
|
match while the *spec* stays the configuration source. (Downside:
|
|
information loss for well-behaved consumers; the spec-derived
|
|
detail is genuinely useful documentation for humans.)
|
|
|
|
The review-002 assessment leaned (a) — the crate already pays the
|
|
compile-cache complexity for `publish_schema`, and the advertise/enforce
|
|
asymmetry is exactly the class of drift review 002 filed as
|
|
[minor→major-class]. Option (b) is legitimate if the assembly-layer
|
|
trust boundary (ADR-066's "specs are trusted configuration") is judged
|
|
to make full-schema enforcement unnecessary belt — but note OAI-02's
|
|
rationale cut the other way: peer *input* is never trusted, and the
|
|
advertised schema is exactly what a peer reads before crafting input.
|
|
|
|
## Acceptance Criteria
|
|
|
|
- [ ] A decision recorded (implement one option, or document a third)
|
|
in ADR-066 with the FWD-17/18/19 decision-section style
|
|
- [ ] If (a): input schemas compile once per registration (cache
|
|
invalidation mirrors `schema_cache.rs`'s value-keyed scheme);
|
|
peer input failing leaf constraints → `INVALID_INPUT` 422
|
|
naming the violated keyword — NOT an upstream round-trip; a spec
|
|
whose input schema fails to compile fails *import* loudly
|
|
(fail-closed, matching the publish_schema precedent from
|
|
review-001's follow-up task)
|
|
- [ ] If (a): the advertised-vs-enforced round-trip test from the
|
|
review-002 test-gap list lands: construct a doc, import, assert
|
|
the `/schema`-exported `input_schema` accepts exactly what
|
|
`build_request` accepts (the missing chain-test that let JS-01
|
|
and OAI-18 survive two reviews)
|
|
- [ ] If (b): the advert-stripping behavior is tested (required/enum/
|
|
pattern absent from `/schema` output while the original spec
|
|
retains them internally) and the module doc + ADR-066 say so
|
|
- [ ] Either way: `cargo test`, `cargo clippy --all-targets --
|
|
-D warnings`, `cargo fmt --check` pass (`--all-features` if
|
|
mcp-touched)
|
|
|
|
## References
|
|
|
|
- docs/reviews/002-post-remediation-review.md (Part E', OAI-18)
|
|
- src/adapters/forward.rs:455-499 (the key-allowlist), src/gateway/schema_cache.rs (the compile-once pattern to reuse)
|
|
- docs/architecture/decisions/066-from-jsonschema-as-http-adapter.md (the section to extend), /decisions/023-operation-error-schemas.md
|
|
- tasks/adapters/review-002-yaml-normalization.md and review-002-oai13-path-item-wildcards.md (the post-bulk advertised-schema surface this decision was deferred against)
|
|
- docs/reviews/002-post-remediation-review.md Test-gap 10 (the round-trip test)
|
|
|
|
## Notes
|
|
|
|
This was bracketed explicitly because review-002-yaml-normalization
|
|
and review-002-oai13-path-item-wildcards both change the
|
|
advertised-schema surface (wildcards now project with payload schemas;
|
|
path-item parameters merge in). Those landed; the decision is now
|
|
unblocked and the surface is stable. The jsonschema dialect question
|
|
is settled by precedent (`PublishSchemaCache` compiles against the
|
|
same crate). If (a) is chosen, watch one interaction: the
|
|
`HEADER_PARAM_IN_MARKER`-decorated and `body` properties are
|
|
peer-visible schema extensions — the compiled validator must accept
|
|
them as ordinary properties (they are), and `additionalProperties:
|
|
true` semantics must keep working (opt-in catch-all — the compiled
|
|
validator gives you this for free). |