diff --git a/crates/agent/src/scopes.rs b/crates/agent/src/scopes.rs index 7ebc9c34..33efe75e 100644 --- a/crates/agent/src/scopes.rs +++ b/crates/agent/src/scopes.rs @@ -5,19 +5,32 @@ use std::collections::HashSet; +/// URI of the join-links resource. The `link_id` values surfaced through this +/// resource are single-step bearer credentials for `WireMessage::JoinRequest`, +/// so any scope below `Full`/`Admin` MUST be denied access. See issue #436 +/// (audit finding AUD-2). +const JOIN_LINKS_URI: &str = "willow://server/join-links"; + /// Scope of a bearer token, controlling tool and resource access. +/// +/// Resource access is **not** uniform across all scopes: `willow://server/join-links` +/// exposes bearer credentials usable by `WireMessage::JoinRequest`, so it is +/// restricted to the privileged scopes (`Full`, `Admin`) and to `Custom` token +/// allowlists that explicitly include the URI. #[derive(Debug, Clone, Default)] pub enum TokenScope { /// All tools, all resources. #[default] Full, - /// No tools, all resources. + /// No tools, all resources except `willow://server/join-links`. ReadOnly, - /// Messaging tools only, all resources. + /// Messaging tools only, all resources except `willow://server/join-links`. Messaging, /// All tools, all resources (semantically distinct from Full for audit). Admin, - /// Explicit allowlist of tool names. + /// Explicit allowlist of tool *and* resource names. A name in the set + /// authorises either a tool call or a resource read with that exact + /// identifier. Custom(HashSet), } @@ -45,9 +58,16 @@ impl TokenScope { } /// Returns true if the given resource URI is allowed. - pub fn allows_resource(&self, _uri: &str) -> bool { - // All scopes allow all resources. - true + /// + /// `willow://server/join-links` exposes single-step bearer credentials and + /// is therefore gated to `Full`/`Admin` (or a `Custom` set that lists it). + /// All other URIs are allowed under `ReadOnly` and `Messaging`. + pub fn allows_resource(&self, uri: &str) -> bool { + match self { + Self::Full | Self::Admin => true, + Self::ReadOnly | Self::Messaging => !uri.starts_with(JOIN_LINKS_URI), + Self::Custom(set) => set.contains(uri), + } } } @@ -73,6 +93,45 @@ mod tests { assert!(scope.allows_resource("willow://server/channels")); } + #[test] + fn readonly_scope_denies_join_links() { + let scope = TokenScope::ReadOnly; + assert!( + !scope.allows_resource("willow://server/join-links"), + "ReadOnly scope must NOT expose join-links: link_id is a bearer credential (issue #436)" + ); + } + + #[test] + fn messaging_scope_denies_join_links() { + let scope = TokenScope::Messaging; + assert!( + !scope.allows_resource("willow://server/join-links"), + "Messaging scope must NOT expose join-links: link_id is a bearer credential (issue #436)" + ); + } + + #[test] + fn full_and_admin_allow_join_links() { + assert!(TokenScope::Full.allows_resource("willow://server/join-links")); + assert!(TokenScope::Admin.allows_resource("willow://server/join-links")); + } + + #[test] + fn custom_resource_allowlist_gates_join_links() { + // Custom set without the URI denies it. + let mut set = HashSet::new(); + set.insert("send_message".to_string()); + let scope = TokenScope::Custom(set); + assert!(!scope.allows_resource("willow://server/join-links")); + + // Custom set including the URI grants it. + let mut set = HashSet::new(); + set.insert("willow://server/join-links".to_string()); + let scope = TokenScope::Custom(set); + assert!(scope.allows_resource("willow://server/join-links")); + } + #[test] fn messaging_allows_only_messaging_tools() { let scope = TokenScope::Messaging; @@ -88,6 +147,7 @@ mod tests { assert!(!scope.allows_tool("kick_member")); assert!(!scope.allows_tool("create_server")); assert!(scope.allows_resource("willow://identity")); + assert!(scope.allows_resource("willow://server/channels")); } #[test] @@ -95,12 +155,17 @@ mod tests { let mut set = HashSet::new(); set.insert("send_message".to_string()); set.insert("react".to_string()); + // Custom is also an explicit resource allowlist now (issue #436), + // so include the identity URI to keep its access in this test. + set.insert("willow://identity".to_string()); let scope = TokenScope::Custom(set); assert!(scope.allows_tool("send_message")); assert!(scope.allows_tool("react")); assert!(!scope.allows_tool("create_channel")); assert!(!scope.allows_tool("kick_member")); assert!(scope.allows_resource("willow://identity")); + // URI not in the set is denied. + assert!(!scope.allows_resource("willow://server/channels")); } #[test] diff --git a/crates/agent/tests/e2e.rs b/crates/agent/tests/e2e.rs index 7591cf44..3c012ea3 100644 --- a/crates/agent/tests/e2e.rs +++ b/crates/agent/tests/e2e.rs @@ -573,17 +573,72 @@ async fn readonly_token_hides_tools() { visible.iter().map(|t| &t.name).collect::>() ); - // Resources should all be visible + // All resources except `willow://server/join-links` are visible. The + // join-links resource exposes single-step bearer credentials usable by + // `WireMessage::JoinRequest`, so it is restricted to `Full`/`Admin` + // (audit finding AUD-2, issue #436). let resources = willow_agent::resources::list_resources(); for r in &resources { - assert!( - server.scope.allows_resource(&r.raw.uri), - "ReadOnly should allow resource: {}", - r.raw.uri - ); + if r.raw.uri == "willow://server/join-links" { + assert!( + !server.scope.allows_resource(&r.raw.uri), + "ReadOnly must NOT expose {}: link_id is a bearer credential", + r.raw.uri + ); + } else { + assert!( + server.scope.allows_resource(&r.raw.uri), + "ReadOnly should allow resource: {}", + r.raw.uri + ); + } } } +/// Integration-tier check: under `ReadOnly`, the same filter pipeline used by +/// `WillowMcpServer::list_resources` must omit the join-links entry. +/// +/// `ServerHandler::list_resources` cannot be invoked directly from outside +/// rmcp (the `RequestContext` is not externally constructible), +/// so we replicate the exact filter pipeline that the handler runs: +/// `resources::list_resources().filter(|r| scope.allows_resource(&r.raw.uri))`. +#[tokio::test] +async fn readonly_list_resources_omits_join_links() { + use willow_agent::scopes::TokenScope; + use willow_agent::server::WillowMcpServer; + + let (client, _broker) = test_client(); + let server = WillowMcpServer::with_scope(client.clone(), TokenScope::ReadOnly); + + let visible: Vec = willow_agent::resources::list_resources() + .into_iter() + .filter(|r| server.scope.allows_resource(&r.raw.uri)) + .map(|r| r.raw.uri) + .collect(); + + assert!( + !visible.iter().any(|u| u == "willow://server/join-links"), + "ReadOnly list_resources must omit join-links, got: {visible:?}" + ); + // Sanity: other resources are still listed. + assert!(visible.iter().any(|u| u == "willow://identity")); + assert!(visible.iter().any(|u| u == "willow://server/channels")); + + // `Full` scope must still see join-links (positive control). + let full = WillowMcpServer::with_scope(client, TokenScope::Full); + let visible_full: Vec = willow_agent::resources::list_resources() + .into_iter() + .filter(|r| full.scope.allows_resource(&r.raw.uri)) + .map(|r| r.raw.uri) + .collect(); + assert!( + visible_full + .iter() + .any(|u| u == "willow://server/join-links"), + "Full scope should still list join-links" + ); +} + #[tokio::test] async fn messaging_scope_restricts_tools() { use willow_agent::scopes::TokenScope; @@ -733,31 +788,33 @@ async fn read_resource_allowed_uri_returns_resource() { ); } -/// Test that `WillowMcpServer::read_resource` would reject a URI that the scope -/// denies, with the same error code (`INVALID_REQUEST`) that `call_tool` uses. +/// Test that `WillowMcpServer::read_resource` rejects `willow://server/join-links` +/// for a `ReadOnly`-scoped token with `INVALID_REQUEST`, matching the error +/// code `call_tool` uses when scope blocks a tool. /// -/// `read_resource` on `WillowMcpServer` mirrors `call_tool`'s gate: +/// `read_resource` on `WillowMcpServer` has two layers: /// 1. Scope check — returns `Err(ErrorData)` immediately if the URI is blocked /// 2. Dispatch to `resources::read_resource` — only reached if scope allows /// -/// Today no built-in `TokenScope` variant denies any resource, so we drive the -/// negative path with a stub closure that mimics a future scope returning false -/// for one URI. This pins the contract: blocked reads return `INVALID_REQUEST`, -/// not silently dispatch. Pattern mirrors `readonly_scope_rejects_send_message`. +/// Layer 1 is what this test pins: under `ReadOnly`, join-links is now denied +/// (audit finding AUD-2, issue #436). The `RequestContext` needed +/// by the `ServerHandler` trait method cannot be constructed from outside the +/// rmcp crate, so we replicate the exact gate that `read_resource` runs. #[tokio::test] -async fn denied_uri_rejects_with_invalid_request() { +async fn readonly_scope_rejects_join_links_read() { use rmcp::model::ErrorCode; + use willow_agent::scopes::TokenScope; + use willow_agent::server::WillowMcpServer; + + let (client, _broker) = test_client(); + let server = WillowMcpServer::with_scope(client, TokenScope::ReadOnly); - // Stub scope that denies one specific URI — mimics what a future tightened - // scope (e.g. hiding `willow://server/join-links` from messaging tokens) - // would do once `TokenScope::allows_resource` returns something other - // than `true`. let denied_uri = "willow://server/join-links"; - let scope_allows_resource = |uri: &str| uri != denied_uri; + // The real `ReadOnly` scope must deny the join-links URI now. assert!( - !scope_allows_resource(denied_uri), - "stub scope must deny {denied_uri}" + !server.scope.allows_resource(denied_uri), + "ReadOnly must deny {denied_uri} (link_id is a bearer credential)" ); // Replicate exactly what WillowMcpServer::read_resource does before @@ -783,10 +840,10 @@ async fn denied_uri_rejects_with_invalid_request() { "error message should mention the blocked URI" ); - // Sanity: a URI the stub allows must pass the gate. + // Sanity: non-credential URIs still pass under ReadOnly. assert!( - scope_allows_resource("willow://identity"), - "stub scope must allow non-denied URIs" + server.scope.allows_resource("willow://identity"), + "ReadOnly must still allow non-credential URIs" ); }