--- id: review-001-schema-internal-visibility name: Enforce Internal-op invisibility on GET /schema (SRV-02, PRJ-06, GW-02) status: completed depends_on: [] scope: narrow risk: medium impact: component level: implementation tags: [server, gateway, review-001, security] --- ## Description Review 001 findings SRV-02 + PRJ-06 + GW-02 — three faces of one visibility gap on the discovery axis: - **SRV-02** (`src/gateway/routes.rs:120-137`): `schema_handler` runs only `access_check_for_op`; it skips the `is_internal_op` pre-check that `/call`, `/batch`, `/subscribe`, `/publish` all apply. Internal ops carry `AccessControl::default()`, so an unauthenticated caller can fetch the full spec (visibility, schemas, `access_control`) of an op it cannot call. `POST /call` on the same op correctly 404s (tested); `GET /schema` does not — the invisibility invariant (ADR-015 §2) is defeated on the discovery axis. - **PRJ-06** (`src/adapters/to_mcp.rs:148-174`): the MCP `schema` tool has the identical gap — no per-op pre-check, full spec including `required_scopes` returned for forbidden ops. The test `schema_returns_full_operation_spec` enshrines the leak. - **GW-02**: `/search` and `/schema` are per-identity GETs with no `Cache-Control: no-store` / `Vary: Authorization`, so shared caches may serve caller A's filtered response to caller B. Fix all three in one pass since they share the pre-check logic and test fixtures: add the 404 guard to `schema_handler`; run the same `access_check_for_op` pre-check in the MCP `schema` tool (or filter the returned spec per identity) and fix the enshrining test; add cache headers to the two per-identity GETs. ## Acceptance Criteria - [x] `GET /schema?name=` → 404 unauthenticated, for an unauthorized identity, and for an anonymous identity (test) - [x] MCP `schema` tool denies/404s unauthorized ops symmetrically with HTTP `/schema` (test); enshrining test fixed - [x] `/search` + `/schema` responses carry `Cache-Control: no-store` (and `Vary: Authorization` where a token can change the body) - [x] `cargo test` and `cargo clippy --all-targets -- -D warnings` pass - [x] `cargo test --all-features` passes (the MCP half is feature-gated) ## References - docs/reviews/001-initial-implementation-review.md (SRV-02, PRJ-06, GW-02) - docs/architecture/decisions/015-privilege-model-and-authority-context.md - docs/architecture/decisions/041-mcp-tool-gateway-pattern.md ## Notes - GW-02 headers are applied to **all** responses the two GET routes emit (200 and denial paths alike) — a 401/403/404 response is equally caller-specific and must not be cached. - `with_no_cache_headers` is a small helper applied inside `schema_handler`'s pre-check returns plus a `discovery_get_response` wrapper on the dispatch path; `/call`, `/batch`, `/subscribe`, `/publish` (non-GET, non-per-identity-response) are untouched. - The `/schema` 404 guard mirrors `call_handler` exactly: `is_internal_op` pre-check before `access_check_for_op`, same `not_found_response` shape, so a caller cannot distinguish internal from unknown ops. - MCP `schema` uses the registry-backed guard (`schema_visibility_and_access_denial`) rather than intercepting the `services/schema` dispatch — symmetric with HTTP (NOT_FOUND for Internal, FORBIDDEN for ACL) and it never reaches the discovery handler for forbidden names. - Notable: `registry.list_operations()` returns External-only and the test fixture built from it drops Internal ops from the dispatch registry — the new tests use the direct-registration fixture (`registry_with_internal_op`), mirroring `call_internal_op_returns_404`. ## Summary Fixed the three faces of the Internal-op invisibility gap (review-001 SRV-02, PRJ-06, GW-02): - SRV-02: `schema_handler` now applies the same `is_internal_op` → 404 pre-check as `/call`, `/batch`, `/subscribe`, `/publish`; tests cover unauthenticated, anonymous-token, and unauthorized-identity callers. - PRJ-06: the MCP `schema` tool runs the symmetric pre-check (`NOT_FOUND` for `Visibility::Internal`, `FORBIDDEN` for ACL-denied) before dispatching to `services/schema`; the enshrining `schema_returns_full_operation_spec` test no longer fetches with no identity and asserts `access_control` presence (spec now fetched for an unrestricted op, `access_control` asserted only via the new authorized-identity test). - GW-02: `/search` + `/schema` responses carry `Cache-Control: no-store` and `Vary: Authorization` on both success and error paths. Verification: cargo test 270 passed; cargo test --all-features 337+9+6+8+10 passed; clippy -D warnings clean (default and --all-features); fmt clean.