diff --git a/src/adapters/to_mcp.rs b/src/adapters/to_mcp.rs index 8998cdc..f027f48 100644 --- a/src/adapters/to_mcp.rs +++ b/src/adapters/to_mcp.rs @@ -28,6 +28,8 @@ use std::sync::Arc; use alkcall::core::auth::Identity; use alkcall::protocol::wire::{CallError, ResponseEnvelope}; +use alkcall::registry::registration::OperationRegistry; +use alkcall::registry::spec::{AccessResult, Visibility}; use rmcp::model::{ CallToolRequestParams, CallToolResult, Implementation, JsonObject, ListToolsResult, PaginatedRequestParams, ServerCapabilities, ServerInfo, Tool, @@ -162,6 +164,11 @@ impl ToMcpGateway { })); } }; + if let Some(error) = + schema_visibility_and_access_denial(self.dispatch.registry(), &name, identity.as_ref()) + { + return call_error_to_structured_error(error); + } let response = self .dispatch .invoke( @@ -257,6 +264,28 @@ fn batch_error_value(result: CallToolResult) -> Value { }) } +/// Symmetric with HTTP `GET /schema` (PRJ-06): internal ops are +/// invisible (NOT_FOUND regardless of caller), ACL-forbidden ops +/// return FORBIDDEN. The MCP `schema` tool must never return the full +/// spec of an op the caller could not call. +fn schema_visibility_and_access_denial( + registry: &OperationRegistry, + operation: &str, + identity: Option<&Identity>, +) -> Option { + let name = operation.strip_prefix('/').unwrap_or(operation); + let registration = registry.registration(name)?; + if registration.spec.visibility == Visibility::Internal { + return Some(CallError::not_found(operation)); + } + if let AccessResult::Forbidden(message) = + registration.spec.access_control.check(identity, None, None) + { + return Some(CallError::forbidden(message)); + } + None +} + fn map_search_response(response: ResponseEnvelope) -> CallToolResult { match response.result { Ok(value) => { @@ -493,6 +522,19 @@ mod tests { ) } + fn internal_spec(name: &str) -> OperationSpec { + OperationSpec::new( + name, + OperationType::Query, + Visibility::Internal, + serde_json::json!({}), + serde_json::json!({}), + vec![], + AccessControl::default(), + None, + ) + } + fn make_echo_handler() -> alkcall::registry::registration::Handler { make_handler( |input, context| async move { ResponseEnvelope::ok(context.request_id, input) }, @@ -731,9 +773,151 @@ mod tests { assert!(structured.get("input_schema").is_some()); assert!(structured.get("output_schema").is_some()); assert!(structured.get("error_schemas").is_some()); + } + + #[tokio::test] + async fn schema_returns_full_spec_for_authorized_identity() { + let registry = full_registry_with_ops(vec![( + "fs/readFile".to_string(), + OperationType::Query, + AccessControl { + required_scopes: vec!["fs:read".to_string()], + ..Default::default() + }, + )]); + let gateway = ToMcpGateway::new(dispatch(registry, provider())); + + let mut args = Map::new(); + args.insert("name".to_string(), Value::String("fs/readFile".to_string())); + let result = invoke_tool( + &gateway, + "schema", + Some(args), + Some(identity_with_scopes("reader", &["fs:read"])), + ) + .await; + assert_eq!(result.is_error, Some(false)); + let structured = result.structured_content.expect("structured present"); + assert_eq!( + structured.get("name"), + Some(&Value::String("fs/readFile".to_string())) + ); assert!(structured.get("access_control").is_some()); } + #[tokio::test] + async fn schema_denies_acl_forbidden_op_symmetrically_with_http() { + let registry = full_registry_with_ops(vec![( + "admin/secret".to_string(), + OperationType::Query, + AccessControl { + required_scopes: vec!["admin".to_string()], + ..Default::default() + }, + )]); + let gateway = ToMcpGateway::new(dispatch(registry, provider())); + + let mut args = Map::new(); + args.insert( + "name".to_string(), + Value::String("admin/secret".to_string()), + ); + let result = invoke_tool( + &gateway, + "schema", + Some(args), + Some(identity_with_scopes("user", &["user"])), + ) + .await; + assert_eq!(result.is_error, Some(true)); + let structured = result.structured_content.expect("structured error present"); + assert_eq!( + structured.get("code"), + Some(&Value::String("FORBIDDEN".to_string())) + ); + assert!( + structured.get("input_schema").is_none(), + "a denied schema lookup must not leak the op's schemas" + ); + } + + #[tokio::test] + async fn schema_denies_internal_op_with_not_found_symmetrically_with_http() { + let mut inner = OperationRegistry::new(); + inner + .register(HandlerRegistration::new( + internal_spec("secret/op"), + HandlerKind::Once(make_echo_handler()), + OperationProvenance::Local, + None, + None, + Capabilities::new(), + )) + .unwrap(); + let inner = Arc::new(inner); + let mut dispatch_registry = OperationRegistry::new(); + dispatch_registry + .register(HandlerRegistration::new( + internal_spec("secret/op"), + HandlerKind::Once(make_echo_handler()), + OperationProvenance::Local, + None, + None, + Capabilities::new(), + )) + .unwrap(); + dispatch_registry + .register(HandlerRegistration::new( + services_list_spec(), + HandlerKind::Once(services_list_handler(Arc::clone(&inner))), + OperationProvenance::Local, + None, + Some(ScopedPeerEnv::empty()), + Capabilities::new(), + )) + .unwrap(); + dispatch_registry + .register(HandlerRegistration::new( + services_schema_spec(), + HandlerKind::Once(services_schema_handler(Arc::clone(&inner))), + OperationProvenance::Local, + None, + Some(ScopedPeerEnv::empty()), + Capabilities::new(), + )) + .unwrap(); + let gateway = ToMcpGateway::new(dispatch(Arc::new(dispatch_registry), provider())); + + let mut args = Map::new(); + args.insert("name".to_string(), Value::String("secret/op".to_string())); + let unauthenticated = invoke_tool(&gateway, "schema", Some(args.clone()), None).await; + assert_eq!(unauthenticated.is_error, Some(true)); + let structured = unauthenticated + .structured_content + .expect("structured error present"); + assert_eq!( + structured.get("code"), + Some(&Value::String("NOT_FOUND".to_string())) + ); + assert!(structured.get("input_schema").is_none()); + + let unauthorized = invoke_tool( + &gateway, + "schema", + Some(args), + Some(identity_with_scopes("user", &["user"])), + ) + .await; + assert_eq!(unauthorized.is_error, Some(true)); + let structured = unauthorized + .structured_content + .expect("structured error present"); + assert_eq!( + structured.get("code"), + Some(&Value::String("NOT_FOUND".to_string())) + ); + } + #[tokio::test] async fn call_returns_structured_for_success() { let registry = full_registry_with_ops(vec![( diff --git a/src/gateway/routes.rs b/src/gateway/routes.rs index 21f6bab..7cdc9bb 100644 --- a/src/gateway/routes.rs +++ b/src/gateway/routes.rs @@ -19,7 +19,8 @@ use alkcall::registry::registration::OperationRegistry; use alkcall::registry::spec::{AccessResult, Visibility}; use axum::body::Bytes; use axum::extract::{FromRef, Query, State}; -use axum::http::StatusCode; +use axum::http::header::{CACHE_CONTROL, VARY}; +use axum::http::{HeaderValue, StatusCode}; use axum::response::sse::{Event, KeepAlive}; use axum::response::{IntoResponse, Json, Response, Sse}; use axum::routing::{get, post}; @@ -127,7 +128,7 @@ pub(crate) async fn search_handler( let envelope = dispatch .invoke(identity.clone(), SERVICES_LIST, json!({})) .await; - envelope_to_response(envelope, identity.as_ref()) + discovery_get_response(envelope, identity.as_ref()) } pub(crate) async fn schema_handler( @@ -135,8 +136,11 @@ pub(crate) async fn schema_handler( ResolvedIdentity(identity): ResolvedIdentity, Query(query): Query, ) -> Response { + if is_internal_op(&state.registry, &query.name) { + return with_no_cache_headers(not_found_response(&query.name)); + } if let Some(forbidden) = access_check_for_op(&state.registry, &query.name, identity.as_ref()) { - return forbidden_response(forbidden, identity.as_ref()); + return with_no_cache_headers(forbidden_response(forbidden, identity.as_ref())); } let dispatch = state.dispatch(); let envelope = dispatch @@ -146,7 +150,7 @@ pub(crate) async fn schema_handler( json!({ "name": query.name }), ) .await; - envelope_to_response(envelope, identity.as_ref()) + discovery_get_response(envelope, identity.as_ref()) } pub(crate) async fn batch_handler( @@ -501,6 +505,22 @@ fn envelope_to_response(envelope: ResponseEnvelope, identity: Option<&Identity>) } } +/// The per-identity GET endpoints (`/search`, `/schema`) are +/// AccessControl-filtered per caller and auth-dependent (200 vs 403/404 +/// on the same name), so no shared cache may store or reuse the +/// response (GW-02). The header pair applies to every response these +/// routes emit, including the denial (error) paths. +fn discovery_get_response(envelope: ResponseEnvelope, identity: Option<&Identity>) -> Response { + with_no_cache_headers(envelope_to_response(envelope, identity)) +} + +fn with_no_cache_headers(mut response: Response) -> Response { + let headers = response.headers_mut(); + headers.insert(CACHE_CONTROL, HeaderValue::from_static("no-store")); + headers.insert(VARY, HeaderValue::from_static("Authorization")); + response +} + fn envelope_to_json(envelope: ResponseEnvelope) -> Value { match envelope.result { Ok(output) => envelope_to_ok_json(&envelope.request_id, &output), @@ -1071,6 +1091,51 @@ mod tests { assert_eq!(body.get("code"), Some(&json!("NOT_FOUND"))); } + #[tokio::test] + async fn schema_internal_op_returns_404_unauthenticated() { + let router = build_router(registry_with_internal_op(), unused_provider()); + let req = Request::builder() + .method("GET") + .uri("/schema?name=secret%2Fop") + .body(Body::empty()) + .unwrap(); + let (status, body) = send(router, req).await; + assert_eq!(status, StatusCode::NOT_FOUND); + assert_eq!(body.get("code"), Some(&json!("NOT_FOUND"))); + } + + #[tokio::test] + async fn schema_internal_op_returns_404_for_unauthorized_identity() { + let provider: Arc = Arc::new( + StaticIdentityProvider::new() + .with_token("user-tok", identity_with_scopes("user", &["user"])), + ); + let router = build_router(registry_with_internal_op(), provider); + let (k, v) = auth_header("user-tok"); + let req = Request::builder() + .method("GET") + .uri("/schema?name=secret%2Fop") + .header(k, v) + .body(Body::empty()) + .unwrap(); + let (status, _body) = send(router, req).await; + assert_eq!(status, StatusCode::NOT_FOUND); + } + + #[tokio::test] + async fn schema_internal_op_returns_404_for_anonymous_identity() { + let router = build_router(registry_with_internal_op(), unused_provider()); + let (k, v) = auth_header("unknown-tok"); + let req = Request::builder() + .method("GET") + .uri("/schema?name=secret%2Fop") + .header(k, v) + .body(Body::empty()) + .unwrap(); + let (status, _body) = send(router, req).await; + assert_eq!(status, StatusCode::NOT_FOUND); + } + #[tokio::test] async fn batch_returns_array_of_results_in_order() { let router = build_router(registry_with_echo(), unused_provider()); @@ -1615,6 +1680,89 @@ mod tests { let resp = router.oneshot(req).await.unwrap(); assert_eq!(resp.status(), StatusCode::OK); } + + #[tokio::test] + async fn search_and_schema_carry_no_store_and_vary_authorization() { + let discovery = registry_with_discovery_and_ops(vec![HandlerRegistration::new( + external_spec("echo/run", AccessControl::default()), + HandlerKind::Once(echo_handler()), + OperationProvenance::Local, + None, + None, + Capabilities::new(), + )]); + let router = build_router(discovery, unused_provider()); + for uri in ["/search", "/schema?name=echo%2Frun"] { + let req = Request::builder() + .method("GET") + .uri(uri) + .body(Body::empty()) + .unwrap(); + let resp = router.clone().oneshot(req).await.unwrap(); + assert_eq!(resp.status(), StatusCode::OK); + let cache_control = resp + .headers() + .get(axum::http::header::CACHE_CONTROL) + .map(|v| v.to_str().unwrap().to_string()); + assert_eq!( + cache_control.as_deref(), + Some("no-store"), + "GET {uri} must not be cacheable (GW-02)" + ); + let vary = resp + .headers() + .get(axum::http::header::VARY) + .map(|v| v.to_str().unwrap().to_string()); + assert_eq!( + vary.as_deref(), + Some("Authorization"), + "GET {uri} is per-identity; it must Vary on Authorization (GW-02)" + ); + } + } + + #[tokio::test] + async fn schema_denials_carry_no_store_and_vary_authorization() { + let ops = vec![HandlerRegistration::new( + external_spec( + "admin/secret", + AccessControl { + required_scopes: vec!["admin".to_string()], + ..Default::default() + }, + ), + HandlerKind::Once(echo_handler()), + OperationProvenance::Local, + None, + None, + Capabilities::new(), + )]; + let discovery = registry_with_discovery_and_ops(ops); + let router = build_router(discovery, unused_provider()); + let req = Request::builder() + .method("GET") + .uri("/schema?name=admin%2Fsecret") + .body(Body::empty()) + .unwrap(); + let resp = router.oneshot(req).await.unwrap(); + assert_eq!( + resp.status(), + StatusCode::UNAUTHORIZED, + "FORBIDDEN with no identity maps to 401 (gateway error mapping)" + ); + assert_eq!( + resp.headers() + .get(axum::http::header::CACHE_CONTROL) + .map(|v| v.to_str().unwrap()), + Some("no-store") + ); + assert_eq!( + resp.headers() + .get(axum::http::header::VARY) + .map(|v| v.to_str().unwrap()), + Some("Authorization") + ); + } // --- /publish (ADR-068) ------------------------------------------------- use alkcall::registry::registration::make_sink_handler; diff --git a/tasks/server/review-001-schema-internal-visibility.md b/tasks/server/review-001-schema-internal-visibility.md index 43f1d04..90c9dbf 100644 --- a/tasks/server/review-001-schema-internal-visibility.md +++ b/tasks/server/review-001-schema-internal-visibility.md @@ -1,7 +1,7 @@ --- id: review-001-schema-internal-visibility name: Enforce Internal-op invisibility on GET /schema (SRV-02, PRJ-06, GW-02) -status: pending +status: completed depends_on: [] scope: narrow risk: medium @@ -39,11 +39,11 @@ to the two per-identity GETs. ## Acceptance Criteria -- [ ] `GET /schema?name=` → 404 unauthenticated, for an unauthorized identity, and for an anonymous identity (test) -- [ ] MCP `schema` tool denies/404s unauthorized ops symmetrically with HTTP `/schema` (test); enshrining test fixed -- [ ] `/search` + `/schema` responses carry `Cache-Control: no-store` (and `Vary: Authorization` where a token can change the body) -- [ ] `cargo test` and `cargo clippy --all-targets -- -D warnings` pass -- [ ] `cargo test --all-features` passes (the MCP half is feature-gated) +- [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 @@ -53,8 +53,43 @@ to the two per-identity GETs. ## Notes -> Agent fills during implementation. +- 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 -> Filled on completion. \ No newline at end of file +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. \ No newline at end of file