diff --git a/README.md b/README.md index 3408047c..54bd4fff 100644 --- a/README.md +++ b/README.md @@ -179,16 +179,51 @@ Enable the `tools` feature to expose each server tool as a - **The generic bridge** is three tools over one configured-server registry: `McpListServersTool`, `McpListToolsTool` and `McpCallTool` (`mcp_list_servers`, `mcp_list_tools`, `mcp_call_tool`). `McpCallTool` asks a - host-supplied `ActGate` before it sends anything. + host-supplied `ActGate` before it sends anything. Server and tool names lose + the backticks and asterisks a model wraps them in (`` `docs` ``, `**docs**`, + `` `docs`. ``); `_`, `.` and other characters are kept. +- **The call outcome.** Every `mcp_call_tool` result for a call the act gate + allowed and that names a server, a tool and `arguments` carries a + `tinymcp_bus::McpCallOutcome` in `ToolResult::metadata`, with + `kind` set to `MCP_CALL_RESULT_KIND` (`"mcp_call"`): the server and tool, + `ok` when the server answered, and otherwise the error's wire name and + whether it was a 401 that advertised OAuth. It is host-only — never rendered + to the model — so a host meters answered calls and surfaces failures from it + instead of parsing text. A call missing one of those fields, or refused by + the gate, fails before a result exists and carries no outcome. The server and + tool are reported as the caller named them, unscrubbed. Read it back with + `McpCallOutcome::from_metadata`; decoding rejects a wrong `kind` or an `ok` + that disagrees with `error`. - **`SecretScrubber`** removes a server's own credentials (tokens, basic-auth pairs, header and query values, URL userinfo) from whatever it echoes back; every bridge tool applies it, and a host wraps its own invokers with it. + `with_secrets` adds values the host keeps outside the server's config, each + redacted as a strict credential: anywhere in the text, even when short. - **`tool_result`** maps a rendered MCP result onto `tinytools::ToolResult`, bounding oversized blocks. `tinytools` is a git dependency so a host that links another checkout of it can `[patch]` the two into one package. +## `mcp.json` and OAuth for hosts with their own store + +`registry::config_doc` reads and writes the `{ "mcpServers": { … } }` document. +`parse` and `render` are the install store's reading and projection. +A host that keeps declarations itself uses `parse_with` and `render_declared` +instead: `parse_with` also reads `allowedTools`, `disallowedTools` and +`timeoutSecs`, carries the fields the host registers in +`ParseOptions::host_fields` through verbatim in `Declared::host_fields`, and +with `lenient` set drops a bad entry into `ParseReport::rejected` rather than +refusing the document. Credentials are write-only in both. + +`registry::OAuthFlow::refresh` refreshes an expired token from a host's +`OAuthCredentialStore`, re-checking the token endpoint when +`require_public_endpoints()` is set; the free `refresh_if_expired` does not +check. `registry::OAuthBundle` is the stored refresh bundle's shape. +`OAuthFlow::with_client_name` sets the name dynamic registration sends, which +the authorization server shows on its consent screen; it defaults to +`DEFAULT_CLIENT_NAME` (`TinyMCP`). + ## Static linking Enable the `static-link` feature when compiling this module into a Rust host. It diff --git a/ROADMAP.md b/ROADMAP.md index 11340249..50dd0ebd 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -14,6 +14,10 @@ out of scope. A roadmap that lists everything is a roadmap nobody trusts. supply-chain checks - a manual release workflow that versions, tags, publishes to crates.io, and creates a GitHub release with crate and TinyBus runtime/module assets +- a structured `mcp_call` outcome on every bridge call that reaches a result, + for host metering and failure surfacing (contract 1.3) +- `mcp.json` reading for hosts with their own store (`parse_with`), and a + guarded OAuth refresh on the flow ## Next diff --git a/crates/tinymcp-bus/README.md b/crates/tinymcp-bus/README.md index 11e25752..04fbf154 100644 --- a/crates/tinymcp-bus/README.md +++ b/crates/tinymcp-bus/README.md @@ -12,7 +12,7 @@ vocabulary has to be published as an ordinary library. This is it. | ---------- | ------------------------------------------------------------ | | `names` | interface name, object path, one constant per member | | `greeting` | the value vocabulary: the `Greet` request and response | -| `agent_tools` | tool specs a host exposes to a model, and argument normalization | +| `agent_tools` | tool specs a host exposes to a model, argument normalization, and the structured call outcome | | `version` | `CONTRACT_VERSION` and the bind rule a host applies to it | Two dependencies, both pure Rust: `serde` and `serde_json`. diff --git a/crates/tinymcp-bus/src/agent_tools/mod.rs b/crates/tinymcp-bus/src/agent_tools/mod.rs index 147a2b30..9f303dae 100644 --- a/crates/tinymcp-bus/src/agent_tools/mod.rs +++ b/crates/tinymcp-bus/src/agent_tools/mod.rs @@ -11,6 +11,11 @@ //! `arguments` as the object MCP requires, and every path that forwards a call //! reads them through it. //! +//! [`McpCallOutcome`] is what a forwarded call reports back to the host, beside +//! the text the model reads: which server and tool, whether the server +//! answered, and the classified error when it did not. It travels as a tool +//! result's metadata tagged with [`MCP_CALL_RESULT_KIND`]. +//! //! # Why this is in the contract crate //! //! A tool's name, description and schema are prompt-cache and transcript @@ -31,7 +36,9 @@ mod types; pub use arguments::normalize_tool_arguments; pub use registry_tools::{RegistryTool, registry_tool_specs}; -pub use types::{AgentToolEffect, AgentToolSpec, ArgsError}; +pub use types::{ + AgentToolEffect, AgentToolSpec, ArgsError, MCP_CALL_RESULT_KIND, McpCallError, McpCallOutcome, +}; #[cfg(test)] #[path = "mod_tests.rs"] diff --git a/crates/tinymcp-bus/src/agent_tools/mod_tests.rs b/crates/tinymcp-bus/src/agent_tools/mod_tests.rs index 0baea29c..556dda57 100644 --- a/crates/tinymcp-bus/src/agent_tools/mod_tests.rs +++ b/crates/tinymcp-bus/src/agent_tools/mod_tests.rs @@ -309,3 +309,146 @@ fn effects_serialize_in_snake_case() { assert_eq!(serde_json::to_value(effect).unwrap(), json!(wire)); } } + +// --------------------------------------------------------------------------- +// Call outcome +// --------------------------------------------------------------------------- + +#[test] +fn the_call_outcome_kind_is_pinned() { + assert_eq!(super::MCP_CALL_RESULT_KIND, "mcp_call"); +} + +#[test] +fn an_answered_outcome_pins_its_wire_form_and_omits_the_error() { + let outcome = super::McpCallOutcome::answered("docs", "search"); + let wire = serde_json::to_value(&outcome).unwrap(); + assert_eq!( + wire, + json!({ "kind": "mcp_call", "server": "docs", "tool": "search", "ok": true }) + ); + assert_eq!( + serde_json::from_value::(wire).unwrap(), + outcome + ); +} + +#[test] +fn a_failed_outcome_pins_its_wire_form_and_round_trips() { + let outcome = super::McpCallOutcome::failed( + "docs", + "search", + super::McpCallError { + code: crate::errors::UNAUTHORIZED.into(), + unauthorized: true, + advertises_oauth: true, + }, + ); + let wire = serde_json::to_value(&outcome).unwrap(); + assert_eq!( + wire, + json!({ + "kind": "mcp_call", + "server": "docs", + "tool": "search", + "ok": false, + "error": { + "code": "ai.tinyhumans.tinymcp.Error.Unauthorized", + "unauthorized": true, + "advertises_oauth": true, + }, + }) + ); + assert_eq!( + serde_json::from_value::(wire).unwrap(), + outcome + ); +} + +#[test] +fn a_plain_error_carries_no_authorization_signal() { + let error = super::McpCallError::new(crate::errors::TRANSPORT); + assert_eq!(error.code, crate::errors::TRANSPORT); + assert!(!error.unauthorized); + assert!(!error.advertises_oauth); +} + +#[test] +fn an_outcome_is_read_back_only_from_metadata_of_its_kind() { + let outcome = super::McpCallOutcome::answered("docs", "search"); + let metadata = serde_json::to_value(&outcome).unwrap(); + assert_eq!( + super::McpCallOutcome::from_metadata(&metadata), + Some(outcome) + ); + + assert_eq!( + super::McpCallOutcome::from_metadata(&json!({ "kind": "web_search" })), + None + ); + assert_eq!(super::McpCallOutcome::from_metadata(&json!([1, 2])), None); + assert_eq!( + super::McpCallOutcome::from_metadata(&json!({ "kind": "mcp_call", "ok": "yes" })), + None + ); +} + +#[test] +fn decoding_an_outcome_rejects_what_the_constructors_never_produce() { + let decode = |value: serde_json::Value| serde_json::from_value::(value); + let error = json!({ "code": crate::errors::TRANSPORT, "unauthorized": false, "advertises_oauth": false }); + let base = + |ok: bool| json!({ "kind": "mcp_call", "server": "docs", "tool": "search", "ok": ok }); + + let mut answered_with_error = base(true); + answered_with_error["error"] = error.clone(); + assert!(decode(answered_with_error).is_err()); + + assert!(decode(base(false)).is_err()); + let mut failed_null_error = base(false); + failed_null_error["error"] = serde_json::Value::Null; + assert!(decode(failed_null_error).is_err()); + + let mut missing_kind = base(true); + missing_kind.as_object_mut().unwrap().remove("kind"); + assert!(decode(missing_kind).is_err()); + let mut wrong_kind = base(true); + wrong_kind["kind"] = json!("web_search"); + assert!(decode(wrong_kind).is_err()); + + assert_eq!( + decode(base(true)).unwrap(), + super::McpCallOutcome::answered("docs", "search") + ); + let mut failed = base(false); + failed["error"] = error; + assert_eq!( + decode(failed).unwrap(), + super::McpCallOutcome::failed( + "docs", + "search", + super::McpCallError::new(crate::errors::TRANSPORT) + ) + ); +} + +#[test] +fn decoding_an_error_rejects_an_oauth_advert_without_a_401() { + let decode = |unauthorized: bool, advertises_oauth: bool| { + serde_json::from_value::(json!({ + "code": "x", + "unauthorized": unauthorized, + "advertises_oauth": advertises_oauth, + })) + }; + assert!(decode(false, true).is_err()); + assert!(decode(false, false).is_ok()); + assert!(decode(true, false).is_ok()); + assert!(decode(true, true).is_ok()); + + let nested = json!({ + "kind": "mcp_call", "server": "docs", "tool": "t", "ok": false, + "error": { "code": "x", "unauthorized": false, "advertises_oauth": true }, + }); + assert_eq!(super::McpCallOutcome::from_metadata(&nested), None); +} diff --git a/crates/tinymcp-bus/src/agent_tools/types.rs b/crates/tinymcp-bus/src/agent_tools/types.rs index 82d4e0f0..6681dae1 100644 --- a/crates/tinymcp-bus/src/agent_tools/types.rs +++ b/crates/tinymcp-bus/src/agent_tools/types.rs @@ -78,3 +78,159 @@ pub struct AgentToolSpec { /// A deferred tool costs nothing in the prompt until a model looks for it. pub deferred: bool, } + +/// The `kind` discriminator an [`McpCallOutcome`] carries. +/// +/// A host that receives a tool result's metadata as an untyped JSON object +/// tells this payload apart from other tools' by this value. +pub const MCP_CALL_RESULT_KIND: &str = "mcp_call"; + +/// What one `mcp_call_tool` call did, as structured data for the host. +/// +/// Attached to the call's result as host-only metadata, never rendered to the +/// model. A host reads it instead of parsing the result text to meter calls +/// that reached a server and to surface the ones that did not. +/// +/// `ok` says whether the server answered the call. A tool that answered with +/// its own error result still has `ok: true`: the server was reached and the +/// failure is the tool's, reported in the result itself. +/// +/// Decoding rejects a payload whose `kind` is not [`MCP_CALL_RESULT_KIND`] or +/// whose `ok` and `error` disagree, so a decoded outcome is one the +/// constructors could have produced. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(try_from = "McpCallOutcomeWire")] +pub struct McpCallOutcome { + /// Always [`MCP_CALL_RESULT_KIND`]. + pub kind: String, + /// The server the call was aimed at, as the caller named it. + pub server: String, + /// The tool the call was aimed at, as the caller named it. + pub tool: String, + /// Whether the server answered the call. + pub ok: bool, + /// Why the call did not reach an answer. `None` when `ok` is `true`. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub error: Option, +} + +impl McpCallOutcome { + /// A call the server answered. + #[must_use] + pub fn answered(server: impl Into, tool: impl Into) -> Self { + Self { + kind: MCP_CALL_RESULT_KIND.to_string(), + server: server.into(), + tool: tool.into(), + ok: true, + error: None, + } + } + + /// A call that failed before the server answered it. + #[must_use] + pub fn failed(server: impl Into, tool: impl Into, error: McpCallError) -> Self { + Self { + kind: MCP_CALL_RESULT_KIND.to_string(), + server: server.into(), + tool: tool.into(), + ok: false, + error: Some(error), + } + } + + /// Reads an outcome out of a tool result's metadata. + /// + /// `None` when `metadata` is not an object whose `kind` is + /// [`MCP_CALL_RESULT_KIND`], or does not decode as an outcome. + #[must_use] + pub fn from_metadata(metadata: &Value) -> Option { + serde_json::from_value(metadata.clone()).ok() + } +} + +#[derive(Deserialize)] +struct McpCallOutcomeWire { + kind: String, + server: String, + tool: String, + ok: bool, + #[serde(default)] + error: Option, +} + +impl TryFrom for McpCallOutcome { + type Error = String; + + fn try_from(wire: McpCallOutcomeWire) -> Result { + if wire.kind != MCP_CALL_RESULT_KIND { + return Err(format!( + "expected kind `{MCP_CALL_RESULT_KIND}`, got `{}`", + wire.kind + )); + } + if wire.ok != wire.error.is_none() { + return Err(if wire.ok { + "an answered call cannot carry an error".to_string() + } else { + "a failed call must carry an error".to_string() + }); + } + Ok(Self { + kind: wire.kind, + server: wire.server, + tool: wire.tool, + ok: wire.ok, + error: wire.error, + }) + } +} + +/// Why an MCP call failed, classified for a host. +/// +/// Decoding rejects `advertises_oauth: true` without `unauthorized: true`. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(try_from = "McpCallErrorWire")] +pub struct McpCallError { + /// The error's wire name: one of the constants in [`crate::errors`]. + pub code: String, + /// Whether the server answered HTTP 401 and wants credentials. + pub unauthorized: bool, + /// Whether that 401 advertised OAuth, so the host offers a sign-in rather + /// than a token field. Always `false` when `unauthorized` is `false`. + pub advertises_oauth: bool, +} + +#[derive(Deserialize)] +struct McpCallErrorWire { + code: String, + unauthorized: bool, + advertises_oauth: bool, +} + +impl TryFrom for McpCallError { + type Error = &'static str; + + fn try_from(wire: McpCallErrorWire) -> Result { + if wire.advertises_oauth && !wire.unauthorized { + return Err("`advertises_oauth` requires `unauthorized`"); + } + Ok(Self { + code: wire.code, + unauthorized: wire.unauthorized, + advertises_oauth: wire.advertises_oauth, + }) + } +} + +impl McpCallError { + /// An error classified under `code` with no authorization signal. + #[must_use] + pub fn new(code: impl Into) -> Self { + Self { + code: code.into(), + unauthorized: false, + advertises_oauth: false, + } + } +} diff --git a/crates/tinymcp-bus/src/lib.rs b/crates/tinymcp-bus/src/lib.rs index e0be61c8..2126ad2f 100644 --- a/crates/tinymcp-bus/src/lib.rs +++ b/crates/tinymcp-bus/src/lib.rs @@ -25,8 +25,9 @@ //! records. //! - [`audit`] — the durable record of every write an MCP tool performed. //! - [`sanitize`] — the stripping pipeline applied to untrusted remote text. -//! - [`agent_tools`] — the tool specs a host exposes to a model, and the -//! normalization every forwarded call's `arguments` goes through. +//! - [`agent_tools`] — the tool specs a host exposes to a model, the +//! normalization every forwarded call's `arguments` goes through, and the +//! structured outcome a forwarded call reports to the host. //! - [`version`] — [`CONTRACT_VERSION`] and the [`is_compatible`] bind rule. //! //! # What is deliberately not here @@ -129,8 +130,8 @@ pub mod transport; pub mod version; pub use agent_tools::{ - AgentToolEffect, AgentToolSpec, ArgsError, RegistryTool, normalize_tool_arguments, - registry_tool_specs, + AgentToolEffect, AgentToolSpec, ArgsError, MCP_CALL_RESULT_KIND, McpCallError, McpCallOutcome, + RegistryTool, normalize_tool_arguments, registry_tool_specs, }; pub use audit::{ DEFAULT_LIST_LIMIT, ERROR_MESSAGE_MAX_BYTES, MAX_LIST_LIMIT, McpWriteListQuery, McpWriteRecord, diff --git a/crates/tinymcp-bus/src/version/mod.rs b/crates/tinymcp-bus/src/version/mod.rs index ecfe484d..ad809726 100644 --- a/crates/tinymcp-bus/src/version/mod.rs +++ b/crates/tinymcp-bus/src/version/mod.rs @@ -9,8 +9,9 @@ /// The wire contract version this crate defines. /// -/// 1.1 added agent tools; 1.2 added registry and directory members. -pub const CONTRACT_VERSION: (u32, u32) = (1, 2); +/// 1.1 added agent tools; 1.2 added registry and directory members; 1.3 added +/// the structured call outcome and the credential-store error name. +pub const CONTRACT_VERSION: (u32, u32) = (1, 3); /// Returns whether a host holding [`CONTRACT_VERSION`] can bind to a module /// reporting `module`. diff --git a/crates/tinymcp-bus/src/version/mod_tests.rs b/crates/tinymcp-bus/src/version/mod_tests.rs index 10211e88..e7ab0980 100644 --- a/crates/tinymcp-bus/src/version/mod_tests.rs +++ b/crates/tinymcp-bus/src/version/mod_tests.rs @@ -4,13 +4,14 @@ use super::{CONTRACT_VERSION, binds, is_compatible}; #[test] fn the_shipped_contract_version_is_pinned() { - assert_eq!(CONTRACT_VERSION, (1, 2)); + assert_eq!(CONTRACT_VERSION, (1, 3)); } #[test] fn a_host_on_this_contract_refuses_a_module_from_before_it() { - // 1.1 added agent tools; a 1.0 module does not carry them. + // 1.3 added the call outcome; a 1.2 module does not report it. assert!(!is_compatible((1, 0))); + assert!(!is_compatible((1, 2))); } #[test] @@ -20,7 +21,7 @@ fn the_contract_binds_to_itself() { #[test] fn a_newer_minor_on_the_module_side_binds() { - assert!(is_compatible((1, 2))); + assert!(is_compatible((1, 3))); assert!(is_compatible((1, 97))); } diff --git a/crates/tinymcp/src/lib.rs b/crates/tinymcp/src/lib.rs index a7fbb11a..16c3b252 100644 --- a/crates/tinymcp/src/lib.rs +++ b/crates/tinymcp/src/lib.rs @@ -115,8 +115,9 @@ pub use tinymcp_bus::{ AuthDetection, AuthKind, AuthorizationServerMetadata, CONTRACT_VERSION, ChatTurn, CommandKind, ConnStatus, ConnectedServerOverview, DEFAULT_LIST_LIMIT, ERROR_MESSAGE_MAX_BYTES, ExtraFields, HEADER_PROTOCOL_VERSION, HEADER_SESSION_ID, HttpHeader, INTERFACE, InstalledServer, - LATEST_PROTOCOL_VERSION, MAX_DESCRIPTION_BYTES, MAX_LIST_LIMIT, MAX_TITLE_BYTES, METHODS, - McpAuthChallenge, McpAuthConfig, McpAuthHint, McpAuthorizationContext, McpClientConfig, + LATEST_PROTOCOL_VERSION, MAX_DESCRIPTION_BYTES, MAX_LIST_LIMIT, MAX_TITLE_BYTES, + MCP_CALL_RESULT_KIND, METHODS, McpAuthChallenge, McpAuthConfig, McpAuthHint, + McpAuthorizationContext, McpCallError, McpCallOutcome, McpClientConfig, McpClientIdentityConfig, McpClientInfo, McpInitializeResult, McpProxyConfig, McpRegistryAuthConfig, McpRemoteTool, McpServerConfig, McpServerToolResult, McpSseEvent, McpTool, McpToolContent, McpToolResult, McpWriteListQuery, McpWriteRecord, NewMcpWriteRecord, diff --git a/crates/tinymcp/src/registry/config_doc/document.rs b/crates/tinymcp/src/registry/config_doc/document.rs index 486a5c95..9f5eee46 100644 --- a/crates/tinymcp/src/registry/config_doc/document.rs +++ b/crates/tinymcp/src/registry/config_doc/document.rs @@ -4,7 +4,7 @@ use std::collections::BTreeMap; use serde_json::{Map, Value, json}; -use super::types::Declared; +use super::types::{Declared, ParseOptions, ParseReport, RejectedEntry}; use crate::error::{Error, Result}; use tinymcp_bus::{InstalledServer, Transport}; @@ -17,6 +17,21 @@ const ECHOED_FIELDS: &[&str] = &["envKeys", "authConfigured"]; /// Fields a write accepts without acting on, because other clients emit them. const TOLERATED_FIELDS: &[&str] = &["type", "transport"]; +/// The fields every entry may carry. +const STANDARD_FIELDS: &[&str] = &[ + "url", + "command", + "args", + "env", + "headers", + "description", + "enabled", +]; + +/// The fields [`parse_with`] reads and [`parse`] refuses, because the install +/// store has nowhere to keep them. +const EXTENSION_FIELDS: &[&str] = &["allowedTools", "disallowedTools", "timeoutSecs"]; + /// Credential names beginning this way are the store's own bookkeeping (the /// OAuth refresh bundle) and are neither rendered nor overwritten from here. const INTERNAL_KEY_PREFIX: &str = "__"; @@ -86,6 +101,51 @@ pub fn parse(doc: &Value) -> Result> { /// [`parse`], with the refusal as the bare sentence. fn parse_servers(doc: &Value) -> std::result::Result, String> { + let mut declared = Vec::new(); + for (name, entry) in server_entries(doc)? { + declared.push(read_entry(name, entry, None)?); + } + Ok(declared) +} + +/// Reads a document into declarations for a host that keeps its own store. +/// +/// As [`parse`], plus `allowedTools`, `disallowedTools` and `timeoutSecs`, the +/// host's own fields from [`ParseOptions::host_fields`], and a refusal of two +/// keys that name one server once trimmed. Under [`ParseOptions::lenient`] a +/// refused entry is dropped and reported in [`ParseReport::rejected`] rather +/// than refusing the document. +/// +/// # Errors +/// +/// Returns [`Error::ConfigDoc`] when the document's root is unreadable, and, +/// unless lenient, for the first refused entry. +pub fn parse_with(doc: &Value, options: &ParseOptions<'_>) -> Result { + let config_doc = |detail| Error::ConfigDoc { detail }; + let mut report = ParseReport::default(); + for (name, entry) in server_entries(doc).map_err(config_doc)? { + let read = if report.declared.iter().any(|seen| seen.name == name) { + Err(format!("`{name}` is declared twice")) + } else { + read_entry(name, entry, Some(options)) + }; + match read { + Ok(declared) => report.declared.push(declared), + Err(detail) if options.lenient => { + tracing::debug!(name, "dropping a refused mcp.json entry: {detail}"); + report.rejected.push(RejectedEntry { + name: name.to_string(), + detail, + }); + } + Err(detail) => return Err(config_doc(detail)), + } + } + Ok(report) +} + +/// The document's entries by trimmed key, refusing an unreadable root. +fn server_entries(doc: &Value) -> std::result::Result, String> { let Some(root) = doc.as_object() else { return Err(format!("mcp.json holds an object with an `{ROOT_KEY}` key")); }; @@ -95,35 +155,46 @@ fn parse_servers(doc: &Value) -> std::result::Result, String> { let Some(servers) = servers.as_object() else { return Err(format!("`{ROOT_KEY}` maps a server name to its settings")); }; + Ok(servers + .iter() + .map(|(name, entry)| (name.trim(), entry)) + .collect()) +} - let mut declared = Vec::with_capacity(servers.len()); - for (name, entry) in servers { - let name = name.trim(); - if name.is_empty() { - return Err("a server needs a name — one entry's key is empty".to_string()); - } - let Some(entry) = entry.as_object() else { - return Err(format!( - "`{name}` holds an object, e.g. {{ \"command\": \"npx\", \"args\": [\"-y\", \"…\"] }} or {{ \"url\": \"https://…\" }}" - )); - }; - declared.push(parse_entry(name, entry)?); +/// Reads one keyed entry, refusing an empty key or a non-object value. +fn read_entry( + name: &str, + entry: &Value, + extension: Option<&ParseOptions<'_>>, +) -> std::result::Result { + if name.is_empty() { + return Err("a server needs a name — one entry's key is empty".to_string()); } - Ok(declared) + let Some(entry) = entry.as_object() else { + return Err(format!( + "`{name}` holds an object, e.g. {{ \"command\": \"npx\", \"args\": [\"-y\", \"…\"] }} or {{ \"url\": \"https://…\" }}" + )); + }; + parse_entry(name, entry, extension) } -/// Reads one entry. -fn parse_entry(name: &str, entry: &Map) -> std::result::Result { +/// Reads one entry. `extension` is `None` for [`parse`], which refuses the +/// extension and host fields. +fn parse_entry( + name: &str, + entry: &Map, + extension: Option<&ParseOptions<'_>>, +) -> std::result::Result { + let host_fields = extension.map_or(&[][..], |options| options.host_fields); for key in entry.keys() { - let known = matches!( - key.as_str(), - "url" | "command" | "args" | "env" | "headers" | "description" | "enabled" - ) || ECHOED_FIELDS.contains(&key.as_str()) - || TOLERATED_FIELDS.contains(&key.as_str()); + let key = key.as_str(); + let known = STANDARD_FIELDS.contains(&key) + || ECHOED_FIELDS.contains(&key) + || TOLERATED_FIELDS.contains(&key) + || (extension.is_some() && EXTENSION_FIELDS.contains(&key)) + || host_fields.contains(&key); if !known { - return Err(format!( - "`{name}` has a `{key}` field this host doesn't understand; it accepts url, headers, command, args, env, description and enabled" - )); + return Err(unknown_field(name, key, extension.is_some(), host_fields)); } } @@ -180,9 +251,141 @@ fn parse_entry(name: &str, entry: &Map) -> std::result::Result String { + let mut accepted: Vec<&str> = vec![ + "url", + "headers", + "command", + "args", + "env", + "description", + "enabled", + ]; + if extended { + accepted.extend(EXTENSION_FIELDS); + accepted.extend( + host_fields.iter().filter(|field| { + !STANDARD_FIELDS.contains(field) && !EXTENSION_FIELDS.contains(field) + }), + ); + } + let (last, rest) = accepted.split_last().unwrap_or((&"", &[])); + format!( + "`{name}` has a `{key}` field this host doesn't understand; it accepts {} and {last}", + rest.join(", ") + ) +} + +/// A list of tool names, trimmed, refusing a blank or non-string one. Absent +/// reads as empty. +fn tool_list( + name: &str, + entry: &Map, + key: &str, +) -> std::result::Result, String> { + match entry.get(key) { + None | Some(Value::Null) => Ok(Vec::new()), + Some(Value::Array(items)) => items + .iter() + .map(|item| { + item.as_str() + .map(str::trim) + .filter(|tool| !tool.is_empty()) + .map(str::to_string) + .ok_or_else(|| format!("`{name}`.{key} holds tool names only")) + }) + .collect(), + Some(_) => Err(format!("`{name}`.{key} is a list of tool names")), + } +} + +/// `timeoutSecs`, refusing anything but a whole number of seconds above zero. +fn timeout_secs( + name: &str, + entry: &Map, +) -> std::result::Result, String> { + match entry.get("timeoutSecs") { + None | Some(Value::Null) => Ok(None), + Some(value) => value + .as_u64() + .filter(|secs| *secs > 0) + .map(Some) + .ok_or_else(|| format!("`{name}`.timeoutSecs is a whole number of seconds above zero")), + } +} + +/// The host's registered fields an entry carries, verbatim, skipping nulls +/// and any name that is a standard field. +fn host_field_values(entry: &Map, host_fields: &[&str]) -> BTreeMap { + host_fields + .iter() + .filter(|field| { + !STANDARD_FIELDS.contains(field) + && !EXTENSION_FIELDS.contains(field) + && !ECHOED_FIELDS.contains(field) + && !TOLERATED_FIELDS.contains(field) + }) + .filter_map(|field| { + let value = entry.get(*field).filter(|value| !value.is_null())?; + Some(((*field).to_string(), value.clone())) + }) + .collect() +} + +/// Writes declarations back as the document [`parse_with`] reads. +/// +/// For a host that keeps declarations itself; [`render`] is the install +/// store's projection. Servers are sorted by name, the extension and host +/// fields are emitted when set, and credentials never are: a declaration's +/// `env` or `headers` is write-only, as everywhere else in this module. +#[must_use] +pub fn render_declared(declared: &[Declared]) -> Value { + let mut entries: Vec<&Declared> = declared.iter().collect(); + entries.sort_by(|a, b| a.name.cmp(&b.name)); + + let mut out = Map::new(); + for server in entries { + let mut entry = Map::new(); + for (key, value) in &server.host_fields { + entry.insert(key.clone(), value.clone()); + } + if let Transport::HttpRemote { url } = &server.transport { + entry.insert("url".into(), json!(url)); + } else { + entry.insert("command".into(), json!(server.command)); + if !server.args.is_empty() { + entry.insert("args".into(), json!(server.args)); + } + } + if let Some(description) = server.description.as_deref().filter(|d| !d.is_empty()) { + entry.insert("description".into(), json!(description)); + } + if !server.enabled { + entry.insert("enabled".into(), json!(false)); + } + if !server.allowed_tools.is_empty() { + entry.insert("allowedTools".into(), json!(server.allowed_tools)); + } + if !server.disallowed_tools.is_empty() { + entry.insert("disallowedTools".into(), json!(server.disallowed_tools)); + } + if let Some(secs) = server.timeout_secs { + entry.insert("timeoutSecs".into(), json!(secs)); + } + out.insert(server.name.clone(), Value::Object(entry)); + } + + json!({ ROOT_KEY: out }) +} + /// Reads an entry's credential block: `env` for a stdio server, `headers` for /// an HTTP one, refusing the other spelling. /// diff --git a/crates/tinymcp/src/registry/config_doc/mod.rs b/crates/tinymcp/src/registry/config_doc/mod.rs index 6973c65a..f1126103 100644 --- a/crates/tinymcp/src/registry/config_doc/mod.rs +++ b/crates/tinymcp/src/registry/config_doc/mod.rs @@ -14,6 +14,17 @@ //! applies the difference. Neither is an import format that can drift from //! what is configured. //! +//! # A host that keeps its own store +//! +//! A host that holds declarations itself rather than in the install store +//! reads the same document through [`parse_with`], which also accepts +//! `allowedTools`, `disallowedTools` and `timeoutSecs`, carries fields the +//! host registers in [`ParseOptions::host_fields`] through verbatim, and can +//! drop a bad entry instead of refusing the document. [`render_declared`] +//! writes those declarations back. [`parse`] keeps refusing the extension +//! fields, because the install store has nowhere to put them and accepting +//! them there would drop them silently. +//! //! # Credentials are write-only //! //! A stdio server's `env` and an HTTP server's `headers` are secrets. They are @@ -35,9 +46,18 @@ mod apply; mod document; mod types; -pub use document::{ROOT_KEY, merge_credentials, parse, render, same_dial, to_installed}; -pub use types::{AppliedServer, ConfigApplyReport, Declared}; +pub use document::{ + ROOT_KEY, merge_credentials, parse, parse_with, render, render_declared, same_dial, + to_installed, +}; +pub use types::{ + AppliedServer, ConfigApplyReport, Declared, ParseOptions, ParseReport, RejectedEntry, +}; #[cfg(test)] #[path = "mod_tests.rs"] mod test; + +#[cfg(test)] +#[path = "mod_parse_with_tests.rs"] +mod parse_with_test; diff --git a/crates/tinymcp/src/registry/config_doc/mod_parse_with_tests.rs b/crates/tinymcp/src/registry/config_doc/mod_parse_with_tests.rs new file mode 100644 index 00000000..ffdfe68a --- /dev/null +++ b/crates/tinymcp/src/registry/config_doc/mod_parse_with_tests.rs @@ -0,0 +1,304 @@ +//! [`parse_with`] and [`render_declared`]: the reading of `mcp.json` for a +//! host that keeps its own store — extension fields, host fields, lenient +//! reads — and writing those declarations back. + +#![allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)] + +use std::collections::BTreeMap; + +use serde_json::{Value, json}; +use tinymcp_bus::Transport; + +use super::*; +use crate::Error; + +const HOST_FIELDS: &[&str] = &["readOnlyTools", "authSecret"]; + +fn host() -> ParseOptions<'static> { + ParseOptions { + host_fields: HOST_FIELDS, + lenient: false, + } +} + +fn lenient() -> ParseOptions<'static> { + ParseOptions { + lenient: true, + ..host() + } +} + +fn refusal(doc: &Value, options: &ParseOptions<'_>) -> String { + match parse_with(doc, options).unwrap_err() { + Error::ConfigDoc { detail } => detail, + other => panic!("expected a config-doc refusal, got {other:?}"), + } +} + +#[test] +fn the_extension_fields_are_read() { + let report = parse_with( + &json!({ "mcpServers": { "notion": { + "url": "https://mcp.notion.test/mcp", + "allowedTools": [" search ", "fetch"], + "disallowedTools": ["delete"], + "timeoutSecs": 45, + } } }), + &ParseOptions::default(), + ) + .unwrap(); + assert_eq!(report.rejected, Vec::new()); + let notion = &report.declared[0]; + assert_eq!(notion.allowed_tools, ["search", "fetch"]); + assert_eq!(notion.disallowed_tools, ["delete"]); + assert_eq!(notion.timeout_secs, Some(45)); + assert!(notion.host_fields.is_empty()); +} + +#[test] +fn the_strict_parse_still_refuses_the_extension_fields() { + let error = parse(&json!({ "mcpServers": { "a": { "command": "npx", "timeoutSecs": 5 } } })) + .unwrap_err() + .to_string(); + assert_eq!( + error, + "`a` has a `timeoutSecs` field this host doesn't understand; it accepts url, headers, command, args, env, description and enabled" + ); + let declared = parse(&json!({ "mcpServers": { "a": { "command": "npx" } } })).unwrap(); + assert_eq!(declared[0].allowed_tools, Vec::::new()); + assert_eq!(declared[0].timeout_secs, None); + assert!(declared[0].host_fields.is_empty()); +} + +#[test] +fn registered_host_fields_are_carried_verbatim() { + let report = parse_with( + &json!({ "mcpServers": { "a": { + "url": "https://a.test/mcp", + "readOnlyTools": ["list"], + "authSecret": null, + } } }), + &host(), + ) + .unwrap(); + assert_eq!( + report.declared[0].host_fields, + BTreeMap::from([("readOnlyTools".to_string(), json!(["list"]))]) + ); +} + +#[test] +fn an_unregistered_field_is_refused_naming_what_is_accepted() { + let doc = json!({ "mcpServers": { "a": { "url": "https://a.test/mcp", "cwd": "/tmp" } } }); + assert_eq!( + refusal(&doc, &host()), + "`a` has a `cwd` field this host doesn't understand; it accepts url, headers, command, args, env, description, enabled, allowedTools, disallowedTools, timeoutSecs, readOnlyTools and authSecret" + ); +} + +#[test] +fn a_host_field_named_like_a_standard_one_reads_as_the_standard_one() { + let report = parse_with( + &json!({ "mcpServers": { "a": { "url": "https://a.test/mcp", "enabled": false } } }), + &ParseOptions { + host_fields: &["enabled", "url"], + lenient: false, + }, + ) + .unwrap(); + assert!(!report.declared[0].enabled); + assert!(report.declared[0].host_fields.is_empty()); +} + +#[test] +fn extension_field_refusals_name_the_entry_and_the_field() { + let cases = [ + ( + json!({ "allowedTools": "search" }), + "`a`.allowedTools is a list of tool names", + ), + ( + json!({ "allowedTools": [1] }), + "`a`.allowedTools holds tool names only", + ), + ( + json!({ "disallowedTools": [" "] }), + "`a`.disallowedTools holds tool names only", + ), + ( + json!({ "timeoutSecs": 0 }), + "`a`.timeoutSecs is a whole number of seconds above zero", + ), + ( + json!({ "timeoutSecs": "30" }), + "`a`.timeoutSecs is a whole number of seconds above zero", + ), + ( + json!({ "timeoutSecs": -1 }), + "`a`.timeoutSecs is a whole number of seconds above zero", + ), + ]; + for (extra, expected) in cases { + let mut entry = json!({ "url": "https://a.test/mcp" }); + entry + .as_object_mut() + .unwrap() + .extend(extra.as_object().unwrap().clone()); + let doc = json!({ "mcpServers": { "a": entry } }); + assert_eq!(refusal(&doc, &host()), expected); + } +} + +#[test] +fn null_extension_fields_read_as_absent() { + let report = parse_with( + &json!({ "mcpServers": { "a": { + "url": "https://a.test/mcp", + "allowedTools": null, + "timeoutSecs": null, + } } }), + &host(), + ) + .unwrap(); + assert_eq!(report.declared[0].allowed_tools, Vec::::new()); + assert_eq!(report.declared[0].timeout_secs, None); +} + +#[test] +fn a_strict_read_refuses_the_document_on_the_first_bad_entry() { + let doc = json!({ "mcpServers": { + "bad": { "url": "https://b.test/mcp", "command": "npx" }, + "good": { "url": "https://g.test/mcp" }, + } }); + assert_eq!( + refusal(&doc, &host()), + "`bad` names both a `url` and a `command`; a server is dialled one way" + ); +} + +#[test] +fn a_lenient_read_drops_bad_entries_and_keeps_the_rest() { + let doc = json!({ "mcpServers": { + "bad": { "url": "https://b.test/mcp", "command": "npx" }, + "good": { "url": "https://g.test/mcp" }, + "list": [1], + " ": { "url": "https://e.test/mcp" }, + "odd": { "url": "https://o.test/mcp", "cwd": "/" }, + } }); + let report = parse_with(&doc, &lenient()).unwrap(); + assert_eq!( + report + .declared + .iter() + .map(|d| d.name.as_str()) + .collect::>(), + ["good"] + ); + let rejected: Vec<&str> = report.rejected.iter().map(|r| r.name.as_str()).collect(); + assert_eq!(rejected, ["", "bad", "list", "odd"]); + assert_eq!( + report.rejected[0].detail, + "a server needs a name — one entry's key is empty" + ); + assert!( + report.rejected[2] + .detail + .starts_with("`list` holds an object") + ); +} + +#[test] +fn two_keys_naming_one_server_are_refused_or_the_second_dropped() { + let doc = json!({ "mcpServers": { + "a": { "url": "https://one.test/mcp" }, + "a ": { "url": "https://two.test/mcp" }, + } }); + assert_eq!(refusal(&doc, &host()), "`a` is declared twice"); + + let report = parse_with(&doc, &lenient()).unwrap(); + assert_eq!(report.declared.len(), 1); + assert_eq!( + report.declared[0].transport, + Transport::HttpRemote { + url: "https://one.test/mcp".into() + } + ); + assert_eq!(report.rejected[0].detail, "`a` is declared twice"); +} + +#[test] +fn an_unreadable_root_is_refused_even_when_lenient() { + for doc in [json!([]), json!({}), json!({ "mcpServers": [] })] { + assert!(matches!( + parse_with(&doc, &lenient()), + Err(Error::ConfigDoc { .. }) + )); + } +} + +#[test] +fn a_rendered_declaration_reads_back_as_itself_without_credentials() { + let doc = json!({ "mcpServers": { + "notion": { + "url": "https://mcp.notion.test/mcp", + "headers": { "Authorization": "Bearer secret" }, + "description": "Notes", + "enabled": false, + "allowedTools": ["search"], + "disallowedTools": ["delete"], + "timeoutSecs": 30, + "readOnlyTools": ["search"], + }, + "fs": { "command": "npx", "args": ["-y", "fs"], "env": { "ROOT": "/" } }, + } }); + let declared = parse_with(&doc, &host()).unwrap().declared; + let rendered = render_declared(&declared); + + assert_eq!( + rendered, + json!({ "mcpServers": { + "fs": { "command": "npx", "args": ["-y", "fs"] }, + "notion": { + "url": "https://mcp.notion.test/mcp", + "description": "Notes", + "enabled": false, + "allowedTools": ["search"], + "disallowedTools": ["delete"], + "timeoutSecs": 30, + "readOnlyTools": ["search"], + }, + } }) + ); + assert!(!rendered.to_string().contains("secret")); + + let reread = parse_with(&rendered, &host()).unwrap().declared; + let without_credentials: Vec = declared + .into_iter() + .map(|mut d| { + d.credentials = None; + d + }) + .collect(); + let mut expected = without_credentials; + expected.sort_by(|a, b| a.name.cmp(&b.name)); + assert_eq!(reread, expected); +} + +#[test] +fn a_command_without_args_renders_without_an_args_list() { + let declared = parse_with( + &json!({ "mcpServers": { "a": { "command": "uvx", "description": "" } } }), + &ParseOptions::default(), + ); + assert!(declared.is_err(), "a blank description is refused"); + let declared = parse_with( + &json!({ "mcpServers": { "a": { "command": "uvx" } } }), + &ParseOptions::default(), + ) + .unwrap() + .declared; + assert_eq!( + render_declared(&declared), + json!({ "mcpServers": { "a": { "command": "uvx" } } }) + ); +} diff --git a/crates/tinymcp/src/registry/config_doc/types.rs b/crates/tinymcp/src/registry/config_doc/types.rs index d3d659c3..96e5e7a1 100644 --- a/crates/tinymcp/src/registry/config_doc/types.rs +++ b/crates/tinymcp/src/registry/config_doc/types.rs @@ -2,6 +2,7 @@ use std::collections::BTreeMap; +use serde_json::Value; use tinymcp_bus::Transport; /// One server as the document declares it, after validation. @@ -23,6 +24,56 @@ pub struct Declared { pub description: Option, /// Whether the server is brought up and exposed. pub enabled: bool, + /// `allowedTools`: the only tools the server may expose. Empty means every + /// tool. Read by [`parse_with`](super::parse_with) only. + pub allowed_tools: Vec, + /// `disallowedTools`: tools that are always blocked. Read by + /// [`parse_with`](super::parse_with) only. + pub disallowed_tools: Vec, + /// `timeoutSecs`: how long a call may take, when the entry says. Read by + /// [`parse_with`](super::parse_with) only. + pub timeout_secs: Option, + /// The host's own fields, by document key, verbatim. + /// + /// Only the names a host registers in [`ParseOptions::host_fields`] land + /// here; a `null` value reads as absent. + pub host_fields: BTreeMap, +} + +/// How [`parse_with`](super::parse_with) reads a document. +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] +pub struct ParseOptions<'a> { + /// Entry fields the host understands beyond the standard ones, by + /// document key (`"readOnlyTools"`, say). Their values are carried in + /// [`Declared::host_fields`] untouched; validating them is the host's. A + /// name that is already a standard field is read as that field. + pub host_fields: &'a [&'a str], + /// Whether a refused entry is dropped and reported instead of refusing + /// the whole document. + /// + /// For documents a user did not write against this host, such as a + /// bundle copied from a vendor's instructions. A document whose root is + /// unreadable is still refused. + pub lenient: bool, +} + +/// What [`parse_with`](super::parse_with) read. +#[derive(Debug, Clone, Default, PartialEq, Eq)] +pub struct ParseReport { + /// The entries that were read, in document-key order. + pub declared: Vec, + /// The entries that were dropped, in document-key order. Always empty + /// unless [`ParseOptions::lenient`] is set. + pub rejected: Vec, +} + +/// One entry a lenient read dropped. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct RejectedEntry { + /// The entry's key, trimmed. + pub name: String, + /// Why it was dropped, naming the entry and the field. + pub detail: String, } /// One server a document application touched. diff --git a/crates/tinymcp/src/registry/mod.rs b/crates/tinymcp/src/registry/mod.rs index 8aad1335..b0250d03 100644 --- a/crates/tinymcp/src/registry/mod.rs +++ b/crates/tinymcp/src/registry/mod.rs @@ -23,7 +23,9 @@ pub mod supervisor; pub use boot::{BootOutcome, connect_installed_servers}; pub use config_doc::{AppliedServer, ConfigApplyReport}; pub use connections::{Connections, ProbeOutcome, REMOTE_REQUEST_TIMEOUT}; -pub use oauth::{AuthDetection, AuthKind, OAuthCredentialStore, OAuthFlow}; +pub use oauth::{ + AuthDetection, AuthKind, DEFAULT_CLIENT_NAME, OAuthBundle, OAuthCredentialStore, OAuthFlow, +}; pub use ops::McpRegistry; pub use setup::{SecretRef, SecretVault}; pub use sources::{Registries, RegistrySource}; diff --git a/crates/tinymcp/src/registry/oauth/flow.rs b/crates/tinymcp/src/registry/oauth/flow.rs index f33d43dd..86e76381 100644 --- a/crates/tinymcp/src/registry/oauth/flow.rs +++ b/crates/tinymcp/src/registry/oauth/flow.rs @@ -11,7 +11,7 @@ use sha2::{Digest, Sha256}; use super::credentials::OAuthCredentialStore; use super::endpoint_guard::{guard_endpoint, guarded_client}; -use super::tokens::{oauth_failure_reason, persist, post_form}; +use super::tokens::{due_refresh, exchange_refresh, oauth_failure_reason, persist, post_form}; use super::types::{AuthDetection, PendingAuthorization, TokenResponse}; use crate::error::{Error, Result}; use crate::transport::http::McpHttpClient; @@ -34,12 +34,12 @@ const AUTH_SERVER_TIMEOUT_SECS: u64 = 20; /// How long to wait when probing a server for what it wants. const PROBE_TIMEOUT_SECS: u64 = 20; -/// The client name presented during dynamic registration. +/// The client name presented during dynamic registration when the host sets +/// none with [`OAuthFlow::with_client_name`]. /// -/// A user sees this on the consent screen, so it names the library rather than -/// leaving it blank. A host that wants its own name there registers its own -/// client; that is a larger change than a string. -const CLIENT_NAME: &str = "TinyMCP"; +/// A user sees it on the consent screen, so it names the library rather than +/// leaving it blank. +pub const DEFAULT_CLIENT_NAME: &str = "TinyMCP"; /// Runs browser sign-in for HTTP-remote servers. /// @@ -53,6 +53,7 @@ pub struct OAuthFlow { proxy: Option, pending: Mutex>, public_endpoints_only: bool, + client_name: String, } impl OAuthFlow { @@ -82,9 +83,32 @@ impl OAuthFlow { proxy, pending: Mutex::new(HashMap::new()), public_endpoints_only: false, + client_name: DEFAULT_CLIENT_NAME.to_string(), }) } + /// Sets the client name sent during dynamic registration, which the + /// authorization server shows the user on its consent screen. + /// + /// A blank name keeps [`DEFAULT_CLIENT_NAME`]. Surrounding whitespace is + /// trimmed. Clients already registered keep the name they were registered + /// with. + #[must_use] + pub fn with_client_name(mut self, name: impl Into) -> Self { + let name = name.into(); + let name = name.trim(); + if !name.is_empty() { + self.client_name = name.to_string(); + } + self + } + + /// The client name sent during dynamic registration. + #[must_use] + pub fn client_name(&self) -> &str { + &self.client_name + } + /// Refuses authorization, registration and token endpoints that are not /// `https` or that resolve to an internal address. /// @@ -93,8 +117,9 @@ impl OAuthFlow { /// trust turns this on. Off by default: a desktop host signs in to loopback /// development servers. The token endpoint is re-checked when a code is /// exchanged, so a host re-pointed between `begin` and `complete` is - /// refused too. [`refresh_if_expired`](super::refresh_if_expired) does not - /// re-check: it only ever posts to an endpoint this guard already passed. + /// refused too, and again by [`Self::refresh`] before each refresh. The + /// free [`refresh_if_expired`](super::refresh_if_expired) does not + /// re-check; a host that turns this on refreshes through [`Self::refresh`]. #[must_use] pub fn require_public_endpoints(mut self) -> Self { self.public_endpoints_only = true; @@ -331,6 +356,33 @@ impl OAuthFlow { Ok(pending.server_id) } + /// Mints a new access token when the stored one has expired, or is about + /// to, as [`refresh_if_expired`](super::refresh_if_expired) does. + /// + /// Returns whether a refresh happened. Under + /// [`Self::require_public_endpoints`] the bundle's token endpoint is + /// checked again, and the refresh posted over a client pinned to the + /// addresses it was checked against, so a bundle stored by an older build + /// or edited in a host's store cannot aim a refresh at an internal address. + /// + /// # Errors + /// + /// Returns [`Error::MalformedResponse`] when the stored bundle cannot be + /// read, when the token endpoint is refused by the guard, or when it answers + /// with something unusable; [`Error::Http`] when it answers with a failure + /// status; and [`Error::Transport`] when it cannot be reached. + pub async fn refresh(&self, store: &S, server_id: &str) -> Result + where + S: OAuthCredentialStore + ?Sized, + { + let Some(bundle) = due_refresh(store, server_id).await? else { + return Ok(false); + }; + let guarded_http = self.token_client(&bundle.token_endpoint).await?; + let http = guarded_http.as_ref().unwrap_or(&self.http); + exchange_refresh(store, http, server_id, &bundle).await + } + /// The server a parked authorization is for, without consuming it. /// /// A browser redirect carries no session, so a host keeping credentials @@ -391,7 +443,7 @@ impl OAuthFlow { redirect_uri: &str, ) -> Result<(String, Option)> { let body = json!({ - "client_name": CLIENT_NAME, + "client_name": self.client_name, "redirect_uris": [redirect_uri], "grant_types": ["authorization_code", "refresh_token"], "response_types": ["code"], diff --git a/crates/tinymcp/src/registry/oauth/mod.rs b/crates/tinymcp/src/registry/oauth/mod.rs index 49649612..c6b38269 100644 --- a/crates/tinymcp/src/registry/oauth/mod.rs +++ b/crates/tinymcp/src/registry/oauth/mod.rs @@ -40,7 +40,7 @@ //! The access token is stored as the server's `Authorization` header value, so //! the ordinary connect path picks it up with no special case. The bookkeeping //! needed to mint a new one — refresh token, client credentials, token endpoint, -//! expiry — is stored beside it under [`OAUTH_BUNDLE_KEY`]. That key begins with +//! expiry — is an [`OAuthBundle`] stored beside it under [`OAUTH_BUNDLE_KEY`]. That key begins with //! two underscores, which is the marker meaning "never send this as a request //! header and never show it in a credential list". @@ -51,9 +51,9 @@ pub(crate) mod tokens; pub(crate) mod types; pub use credentials::OAuthCredentialStore; -pub use flow::OAuthFlow; +pub use flow::{DEFAULT_CLIENT_NAME, OAuthFlow}; pub use tokens::{OAUTH_BUNDLE_KEY, refresh_if_expired}; -pub use types::{AuthDetection, AuthKind}; +pub use types::{AuthDetection, AuthKind, OAuthBundle}; #[cfg(test)] #[path = "mod_tests.rs"] diff --git a/crates/tinymcp/src/registry/oauth/mod_refresh_tests.rs b/crates/tinymcp/src/registry/oauth/mod_refresh_tests.rs new file mode 100644 index 00000000..4a85403e --- /dev/null +++ b/crates/tinymcp/src/registry/oauth/mod_refresh_tests.rs @@ -0,0 +1,249 @@ +//! [`OAuthFlow::refresh`]: the flow's own refresh, which re-applies the +//! public-endpoint guard, and the public [`OAuthBundle`] wire form. + +use super::*; +use crate::registry::OAuthBundle; + +#[test] +fn the_bundle_wire_form_is_pinned_and_round_trips() { + let bundle = OAuthBundle { + refresh_token: Some("r1".into()), + client_id: "cli-1".into(), + client_secret: None, + token_endpoint: "https://auth.example/token".into(), + expires_at: 42, + }; + let wire = serde_json::to_value(&bundle).unwrap(); + assert_eq!( + wire, + json!({ + "refresh_token": "r1", + "client_id": "cli-1", + "client_secret": null, + "token_endpoint": "https://auth.example/token", + "expires_at": 42, + }) + ); + assert_eq!(serde_json::from_value::(wire).unwrap(), bundle); +} + +#[test] +fn the_bundle_debug_output_hides_its_secrets() { + let bundle = OAuthBundle { + refresh_token: Some("refresh-secret".into()), + client_id: "cli-1".into(), + client_secret: Some("client-secret".into()), + token_endpoint: "https://auth.example/token".into(), + expires_at: 42, + }; + let shown = format!("{bundle:?} {bundle:#?}"); + assert!(!shown.contains("refresh-secret"), "{shown}"); + assert!(!shown.contains("client-secret"), "{shown}"); + assert!( + shown.contains("cli-1") && shown.contains("[redacted]"), + "{shown}" + ); + + let bare = OAuthBundle { + refresh_token: None, + client_secret: None, + ..bundle + }; + assert!(format!("{bare:?}").contains("refresh_token: None")); +} + +#[test] +fn a_stored_bundle_reads_back_as_the_public_type() { + let store = store_with_remote("https://example.test/mcp"); + store_expired_bundle(&store, "https://auth.example/token", Some("r1")); + let env = store.load_env_values("srv-1").unwrap(); + let bundle: OAuthBundle = serde_json::from_str(&env[OAUTH_BUNDLE_KEY]).unwrap(); + assert_eq!(bundle.client_secret.as_deref(), Some("sec-1")); + assert_eq!(bundle.refresh_token.as_deref(), Some("r1")); +} + +#[tokio::test] +async fn the_flow_refreshes_an_expired_token_when_unguarded() { + let requests = TokenRequests::default(); + let base = serve(token_endpoint( + requests.clone(), + json!({ "access_token": "fresh", "expires_in": 3600 }), + )) + .await; + let store = store_with_remote("https://example.test/mcp"); + store_expired_bundle(&store, &format!("{base}/token"), Some("r1")); + + let flow = OAuthFlow::new(None).unwrap(); + assert!(flow.refresh(&store, "srv-1").await.unwrap()); + assert_eq!(requests.count.load(Ordering::SeqCst), 1); + assert_eq!( + store.load_env_values("srv-1").unwrap()["Authorization"], + "Bearer fresh" + ); + + assert!(!flow.refresh(&store, "srv-1").await.unwrap()); + assert_eq!(requests.count.load(Ordering::SeqCst), 1); +} + +#[tokio::test] +async fn the_guarded_flow_refuses_a_loopback_token_endpoint_and_sends_nothing() { + let requests = TokenRequests::default(); + let base = serve(token_endpoint( + requests.clone(), + json!({ "access_token": "fresh" }), + )) + .await; + let store = store_with_remote("https://example.test/mcp"); + store_expired_bundle(&store, &format!("{base}/token"), Some("r1")); + + let flow = OAuthFlow::new(None).unwrap().require_public_endpoints(); + let error = flow.refresh(&store, "srv-1").await.unwrap_err(); + assert!( + matches!(&error, Error::MalformedResponse { detail } if detail.starts_with("token endpoint refused")), + "{error}" + ); + assert_eq!(requests.count.load(Ordering::SeqCst), 0); + assert_eq!( + store.load_env_values("srv-1").unwrap()["Authorization"], + "Bearer stale" + ); +} + +#[tokio::test] +async fn the_guarded_flow_refuses_an_https_endpoint_on_an_internal_address() { + let store = store_with_remote("https://example.test/mcp"); + store_expired_bundle(&store, "https://127.0.0.1:1/token", Some("r1")); + + let flow = OAuthFlow::new(None).unwrap().require_public_endpoints(); + let error = flow.refresh(&store, "srv-1").await.unwrap_err(); + assert!(error.to_string().contains("disallowed address"), "{error}"); +} + +#[tokio::test] +async fn the_guarded_flow_leaves_a_token_that_needs_no_refresh_alone() { + let store = store_with_remote("https://example.test/mcp"); + store_expired_bundle(&store, "http://127.0.0.1:1/token", None); + + let flow = OAuthFlow::new(None).unwrap().require_public_endpoints(); + assert!(!flow.refresh(&store, "srv-1").await.unwrap()); + assert!( + !flow + .refresh(&store_with_remote("https://x.test/mcp"), "srv-1") + .await + .unwrap() + ); +} + +#[tokio::test] +async fn an_unreadable_bundle_is_reported_by_the_flow() { + let store = store_with_remote("https://example.test/mcp"); + store + .set_env_values( + "srv-1", + &BTreeMap::from([(OAUTH_BUNDLE_KEY.to_string(), "not json".to_string())]), + ) + .unwrap(); + let error = OAuthFlow::new(None) + .unwrap() + .refresh(&store, "srv-1") + .await + .unwrap_err(); + assert!( + error + .to_string() + .contains("stored oauth bundle is unreadable") + ); +} + +/// A store whose bundle is replaced by a newer sign-in after the first read. +struct SupersededStore { + inner: Store, + reads: AtomicUsize, + writes: AtomicUsize, +} + +#[allow(clippy::unused_async_trait_impl)] +impl crate::registry::OAuthCredentialStore for SupersededStore { + async fn remote_url(&self, server_id: &str) -> crate::Result> { + self.inner.remote_url(server_id).await + } + + async fn load_credentials(&self, server_id: &str) -> crate::Result> { + let mut env = self.inner.load_credentials(server_id).await?; + if self.reads.fetch_add(1, Ordering::SeqCst) >= 1 { + let newer = json!({ + "refresh_token": "r-newer", + "client_id": "cli-2", + "client_secret": null, + "token_endpoint": "https://auth.example/token", + "expires_at": u64::MAX / 2, + }); + env.insert(OAUTH_BUNDLE_KEY.to_string(), newer.to_string()); + env.insert("Authorization".to_string(), "Bearer newer".to_string()); + } + Ok(env) + } + + async fn store_credentials( + &self, + server_id: &str, + credentials: &BTreeMap, + ) -> crate::Result<()> { + self.writes.fetch_add(1, Ordering::SeqCst); + self.inner.store_credentials(server_id, credentials).await + } +} + +#[tokio::test] +async fn a_refresh_does_not_overwrite_a_sign_in_stored_while_it_waited() { + let requests = TokenRequests::default(); + let base = serve(token_endpoint( + requests.clone(), + json!({ "access_token": "fresh", "expires_in": 3600 }), + )) + .await; + let inner = store_with_remote("https://example.test/mcp"); + store_expired_bundle(&inner, &format!("{base}/token"), Some("r1")); + let store = SupersededStore { + inner, + reads: AtomicUsize::new(0), + writes: AtomicUsize::new(0), + }; + + let flow = OAuthFlow::new(None).unwrap(); + assert!(!flow.refresh(&store, "srv-1").await.unwrap()); + assert_eq!(requests.count.load(Ordering::SeqCst), 1); + assert_eq!(store.writes.load(Ordering::SeqCst), 0); + assert_eq!( + store.inner.load_env_values("srv-1").unwrap()["Authorization"], + "Bearer stale" + ); +} + +#[tokio::test] +async fn exchanging_a_bundle_without_a_refresh_token_sends_nothing() { + let requests = TokenRequests::default(); + let base = serve(token_endpoint( + requests.clone(), + json!({ "access_token": "fresh", "expires_in": 3600 }), + )) + .await; + let store = store_with_remote("https://example.test/mcp"); + store_expired_bundle(&store, &format!("{base}/token"), Some("r1")); + let http = reqwest::Client::new(); + for refresh_token in [None, Some(" ".to_string())] { + let bundle = OAuthBundle { + refresh_token, + client_id: "cli-1".into(), + client_secret: None, + token_endpoint: format!("{base}/token"), + expires_at: 1, + }; + assert!( + !crate::registry::oauth::tokens::exchange_refresh(&store, &http, "srv-1", &bundle) + .await + .unwrap() + ); + } + assert_eq!(requests.count.load(Ordering::SeqCst), 0); +} diff --git a/crates/tinymcp/src/registry/oauth/mod_tests.rs b/crates/tinymcp/src/registry/oauth/mod_tests.rs index abba73c7..6ffb6dac 100644 --- a/crates/tinymcp/src/registry/oauth/mod_tests.rs +++ b/crates/tinymcp/src/registry/oauth/mod_tests.rs @@ -19,7 +19,7 @@ use axum::routing::post; use axum::{Json, Router}; use serde_json::{Value, json}; -use super::flow::OAuthFlow; +use super::flow::{DEFAULT_CLIENT_NAME, OAuthFlow}; use super::tokens::{OAUTH_BUNDLE_KEY, refresh_if_expired}; use super::types::{AuthKind, TokenResponse}; use crate::Error; @@ -579,6 +579,8 @@ use axum::routing::get; #[derive(Debug, Default)] struct Authority { registrations: AtomicUsize, + /// The body the registration endpoint last received. + last_registration: parking_lot::Mutex>, exchanges: AtomicUsize, /// The form the token endpoint last received. last_form: parking_lot::Mutex>, @@ -600,13 +602,16 @@ fn endpoints() -> Router> { Router::new() .route( "/register", - post(|State(state): State>| async move { - state.registrations.fetch_add(1, Ordering::SeqCst); - axum::Json(json!({ - "client_id": "client-1", - "client_secret": "client-secret-1", - })) - }), + post( + |State(state): State>, Json(body): Json| async move { + state.registrations.fetch_add(1, Ordering::SeqCst); + *state.last_registration.lock() = Some(body); + axum::Json(json!({ + "client_id": "client-1", + "client_secret": "client-secret-1", + })) + }, + ), ) .route( "/token", @@ -791,6 +796,42 @@ async fn beginning_a_sign_in_registers_a_client_and_returns_an_authorize_url() { assert!(url.contains("/authorize"), "{url}"); } +async fn registered_client_name(flow: OAuthFlow) -> Option { + let (endpoint, state) = authority_with_challenge().await; + let store = store_with_remote(&endpoint); + flow.begin(&store, "srv-1", "http://127.0.0.1:7788/callback") + .await + .expect("begin"); + let body = state.last_registration.lock().clone(); + body.and_then(|body| body.get("client_name").cloned()) +} + +#[tokio::test] +async fn registration_names_the_library_when_the_host_sets_no_name() { + assert_eq!( + registered_client_name(flow()).await, + Some(json!(DEFAULT_CLIENT_NAME)) + ); +} + +#[tokio::test] +async fn registration_carries_the_client_name_the_host_sets() { + let flow = flow().with_client_name(" OpenCompany "); + assert_eq!(flow.client_name(), "OpenCompany"); + assert_eq!( + registered_client_name(flow).await, + Some(json!("OpenCompany")) + ); +} + +#[test] +fn a_blank_client_name_keeps_the_default() { + assert_eq!( + flow().with_client_name(" ").client_name(), + DEFAULT_CLIENT_NAME + ); +} + #[tokio::test] async fn the_authorize_url_carries_the_pkce_challenge_and_its_method() { // Without PKCE an intercepted code is enough to mint a token. The method @@ -1229,3 +1270,10 @@ fn a_kind_serializes_as_the_same_spelling_it_reports() { #[path = "mod_host_store_tests.rs"] mod host_store; + +// --------------------------------------------------------------------------- +// The flow's own refresh +// --------------------------------------------------------------------------- + +#[path = "mod_refresh_tests.rs"] +mod flow_refresh; diff --git a/crates/tinymcp/src/registry/oauth/tokens.rs b/crates/tinymcp/src/registry/oauth/tokens.rs index 954c7100..91da8aeb 100644 --- a/crates/tinymcp/src/registry/oauth/tokens.rs +++ b/crates/tinymcp/src/registry/oauth/tokens.rs @@ -51,21 +51,60 @@ pub async fn refresh_if_expired( http: &reqwest::Client, server_id: &str, ) -> Result +where + S: OAuthCredentialStore + ?Sized, +{ + let Some(bundle) = due_refresh(store, server_id).await? else { + return Ok(false); + }; + exchange_refresh(store, http, server_id, &bundle).await +} + +/// The stored bundle when its access token is due for a refresh and it holds a +/// refresh token to do it with; `None` otherwise. +/// +/// # Errors +/// +/// Returns [`Error::MalformedResponse`] when the stored bundle cannot be read, +/// and whatever the store reports when it cannot be read at all. +pub(super) async fn due_refresh(store: &S, server_id: &str) -> Result> where S: OAuthCredentialStore + ?Sized, { let env = store.load_credentials(server_id).await?; let Some(raw_bundle) = env.get(OAUTH_BUNDLE_KEY) else { - return Ok(false); + return Ok(None); }; let bundle: OAuthBundle = serde_json::from_str(raw_bundle) .map_err(|error| Error::malformed(format!("stored oauth bundle is unreadable: {error}")))?; if bundle.expires_at > now_unix() + REFRESH_SKEW_SECS { - return Ok(false); + return Ok(None); } + let has_refresh_token = bundle + .refresh_token + .as_deref() + .is_some_and(|token| !token.trim().is_empty()); + Ok(has_refresh_token.then_some(bundle)) +} + +/// Exchanges `bundle`'s refresh token at its token endpoint over `http` and +/// stores what comes back. +/// +/// # Errors +/// +/// As [`refresh_if_expired`]. +pub(super) async fn exchange_refresh( + store: &S, + http: &reqwest::Client, + server_id: &str, + bundle: &OAuthBundle, +) -> Result +where + S: OAuthCredentialStore + ?Sized, +{ let Some(refresh_token) = bundle .refresh_token .as_deref() @@ -73,7 +112,6 @@ where else { return Ok(false); }; - let mut form: Vec<(&str, &str)> = vec![ ("grant_type", "refresh_token"), ("refresh_token", refresh_token), @@ -92,6 +130,11 @@ where tokens.refresh_token.clone_from(&bundle.refresh_token); } + if superseded(store, server_id, bundle).await? { + tracing::info!(server_id, "dropped a refresh superseded by a newer sign-in"); + return Ok(false); + } + persist( store, server_id, @@ -106,6 +149,22 @@ where Ok(true) } +/// Whether the stored bundle is no longer the `bundle` a refresh started from, +/// because a sign-in stored a newer one while the token endpoint was answering. +/// +/// A store offers no compare-and-swap, so this narrows the window rather than +/// closing it. +async fn superseded(store: &S, server_id: &str, bundle: &OAuthBundle) -> Result +where + S: OAuthCredentialStore + ?Sized, +{ + let env = store.load_credentials(server_id).await?; + let current = env + .get(OAUTH_BUNDLE_KEY) + .and_then(|raw| serde_json::from_str::(raw).ok()); + Ok(current.as_ref() != Some(bundle)) +} + /// Stores an access token and its refresh bundle. /// /// Merged over the existing credentials rather than replacing them. Storing is diff --git a/crates/tinymcp/src/registry/oauth/types.rs b/crates/tinymcp/src/registry/oauth/types.rs index 821fe1af..9176a49d 100644 --- a/crates/tinymcp/src/registry/oauth/types.rs +++ b/crates/tinymcp/src/registry/oauth/types.rs @@ -33,21 +33,38 @@ pub(super) struct PendingAuthorization { /// The bookkeeping needed to mint a new access token without another sign-in. /// -/// Stored beside the access token under the reserved bundle key. The access -/// token itself is the `Authorization` header value, so the ordinary connect -/// path needs no special case for an OAuth server. -#[derive(Debug, Clone, Serialize, Deserialize)] -pub(super) struct OAuthBundle { +/// Stored as JSON beside the access token under +/// [`OAUTH_BUNDLE_KEY`](super::OAUTH_BUNDLE_KEY). The access token itself is +/// the `Authorization` header value, so the ordinary connect path needs no +/// special case for an OAuth server. Public so a host keeping credentials in +/// its own store can read the bundle it holds — for an expiry it shows, or to +/// migrate one — without restating its shape. It holds a client secret and a +/// refresh token: never log or display it. +#[derive(Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct OAuthBundle { /// The refresh token, when the server issued one. - pub(super) refresh_token: Option, + pub refresh_token: Option, /// The registered client. - pub(super) client_id: String, + pub client_id: String, /// The client secret, when there is one. - pub(super) client_secret: Option, + pub client_secret: Option, /// Where to refresh. - pub(super) token_endpoint: String, + pub token_endpoint: String, /// When the current access token expires, in Unix seconds. Best effort. - pub(super) expires_at: u64, + pub expires_at: u64, +} + +impl std::fmt::Debug for OAuthBundle { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + let redacted = |secret: &Option| secret.as_ref().map(|_| "[redacted]"); + f.debug_struct("OAuthBundle") + .field("refresh_token", &redacted(&self.refresh_token)) + .field("client_id", &self.client_id) + .field("client_secret", &redacted(&self.client_secret)) + .field("token_endpoint", &self.token_endpoint) + .field("expires_at", &self.expires_at) + .finish() + } } /// A parsed token-endpoint reply. diff --git a/crates/tinymcp/src/tools/bridge.rs b/crates/tinymcp/src/tools/bridge.rs index ba08d66b..a6aa7d2c 100644 --- a/crates/tinymcp/src/tools/bridge.rs +++ b/crates/tinymcp/src/tools/bridge.rs @@ -9,6 +9,14 @@ //! //! What stays with the host is the decision to allow a call at all: //! [`McpCallTool`] takes an [`ActGate`] it runs before anything is sent. +//! +//! Every result [`McpCallTool`] returns once the act gate has allowed the call +//! and it has a server, a tool and an `arguments` value carries a +//! [`McpCallOutcome`] as its metadata. A call missing one of those, or refused +//! by the gate, fails before any result exists and carries none. The outcome +//! says whether the server answered and, when it did not, the error's wire +//! name and whether it was a 401 that advertised OAuth. The model never sees it; a host reads it to meter calls +//! and to surface failures without parsing the result text. // The tool names and descriptions are fixed strings, and the rendered Markdown // is built a line at a time; both are clearer as written. @@ -18,7 +26,7 @@ use std::sync::Arc; use async_trait::async_trait; use serde_json::{Value, json}; -use tinymcp_bus::McpAuthConfig; +use tinymcp_bus::{McpAuthConfig, McpCallError, McpCallOutcome}; use tinytools::{PermissionLevel, Tool, ToolCallOptions, ToolResult}; use super::scrub::SecretScrubber; @@ -296,6 +304,7 @@ impl Tool for McpCallTool { let server = required_string_arg(&args, "server")?; let tool = required_string_arg(&args, "tool")?; + let scrubber = SecretScrubber::for_server(&self.registry, &server); let arguments = args .get("arguments") .cloned() @@ -304,15 +313,32 @@ impl Tool for McpCallTool { // cannot hold one is refused naming what arrived. let arguments = match tinymcp_bus::normalize_tool_arguments(arguments) { Ok(arguments) => Value::Object(arguments), - Err(error) => return Ok(ToolResult::error(format!("`arguments`: {error}"))), + Err(error) => { + let outcome = McpCallOutcome::failed( + &server, + &tool, + McpCallError::new(tinymcp_bus::errors::INVALID_ARGUMENTS), + ); + return Ok(with_outcome( + ToolResult::error(format!("`arguments`: {error}")), + &outcome, + )); + } }; - let scrubber = SecretScrubber::for_server(&self.registry, &server); - let mut result = match self.registry.call_tool(&server, &tool, arguments).await { - Ok(result) => result.rendered, + let (mut result, outcome) = match self.registry.call_tool(&server, &tool, arguments).await { + Ok(result) => (result.rendered, McpCallOutcome::answered(&server, &tool)), Err(err) => { - return Ok(ToolResult::error( - scrubber.scrub(&format!("mcp_call_tool failed: {err}")), + let outcome = McpCallOutcome::failed(&server, &tool, call_error(&err)); + tracing::debug!( + server = %scrubber.scrub(&server), + tool = %scrubber.scrub(&tool), + code = err.wire_name(), + "[mcp] mcp_call_tool failed" + ); + return Ok(with_outcome( + ToolResult::error(scrubber.scrub(&format!("mcp_call_tool failed: {err}"))), + &outcome, )); } }; @@ -320,7 +346,10 @@ impl Tool for McpCallTool { if options.prefer_markdown && result.markdown_formatted.is_none() { result.markdown_formatted = Some(result.output()); } - Ok(scrubber.scrub_result(super::tool_result(result))) + Ok(with_outcome( + scrubber.scrub_result(super::tool_result(result)), + &outcome, + )) } async fn execute(&self, args: Value) -> anyhow::Result { @@ -329,6 +358,21 @@ impl Tool for McpCallTool { } } +/// `result` carrying `outcome` as its host-only metadata. +fn with_outcome(mut result: ToolResult, outcome: &McpCallOutcome) -> ToolResult { + result.metadata = serde_json::to_value(outcome).ok(); + result +} + +/// The host-facing classification of a failed call. +fn call_error(error: &crate::Error) -> McpCallError { + McpCallError { + code: error.wire_name().to_string(), + unauthorized: error.is_unauthorized(), + advertises_oauth: error.advertises_oauth(), + } +} + fn auth_kind(auth: &McpAuthConfig) -> &'static str { match auth { McpAuthConfig::None => "none", @@ -355,12 +399,29 @@ pub(super) fn endpoint_without_query(endpoint: &str) -> String { .to_string() } -fn required_string_arg(args: &Value, key: &str) -> anyhow::Result { +/// A required identifier argument, trimmed and without the markdown a model +/// wraps it in when it answers in prose (`` `docs` ``, `*docs*`, `` `docs`. ``). +/// +/// Backticks and asterisks go from either end, and sentence punctuation goes +/// only when it follows one of them. Every other character, `_` and `.` +/// included, reaches the server as typed. +pub(super) fn required_string_arg(args: &Value, key: &str) -> anyhow::Result { + let missing = || anyhow::anyhow!("missing required `{key}`"); let value = args .get(key) .and_then(Value::as_str) .map(str::trim) .filter(|s| !s.is_empty()) - .ok_or_else(|| anyhow::anyhow!("missing required `{key}`"))?; - Ok(value.to_string()) + .ok_or_else(missing)?; + let unpunctuated = value.trim_end_matches(['.', ',', ';', ':', '!']); + let fenced = if unpunctuated.ends_with(['`', '*']) { + unpunctuated + } else { + value + }; + let cleaned = fenced.trim_matches(['`', '*']); + if cleaned.is_empty() { + return Err(missing()); + } + Ok(cleaned.to_string()) } diff --git a/crates/tinymcp/src/tools/bridge_outcome_tests.rs b/crates/tinymcp/src/tools/bridge_outcome_tests.rs new file mode 100644 index 00000000..ce4a2772 --- /dev/null +++ b/crates/tinymcp/src/tools/bridge_outcome_tests.rs @@ -0,0 +1,231 @@ +//! Tests for the structured outcome `mcp_call_tool` attaches to its results: +//! what it reports for an answered call and for each way a call fails, that +//! the model-facing rendering never carries it, and that scrubbing keeps it. + +#![allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)] + +use std::sync::Arc; + +use axum::http::{HeaderMap, HeaderValue, StatusCode}; +use axum::response::IntoResponse as _; +use axum::routing::post; +use axum::{Json, Router}; +use serde_json::{Value, json}; +use tinymcp_bus::{ + MCP_CALL_RESULT_KIND, McpAuthConfig, McpCallOutcome, McpClientConfig, McpServerConfig, errors, +}; +use tinytools::{Tool, ToolResult}; + +use super::{ActGate, McpCallTool}; +use crate::McpServerRegistry; + +const SECRET: &str = "outcome-Secret_77"; + +fn registry(endpoint: &str, auth: McpAuthConfig, disallowed: &[&str]) -> Arc { + Arc::new( + McpServerRegistry::from_config(&McpClientConfig { + servers: vec![McpServerConfig { + name: "docs".into(), + endpoint: endpoint.into(), + auth, + disallowed_tools: disallowed.iter().map(|tool| (*tool).to_string()).collect(), + ..McpServerConfig::default() + }], + ..McpClientConfig::default() + }) + .unwrap(), + ) +} + +fn call_tool(registry: Arc) -> McpCallTool { + let gate: ActGate = Arc::new(|_| Ok(())); + McpCallTool::new(registry, gate) +} + +fn outcome_of(result: &ToolResult) -> McpCallOutcome { + McpCallOutcome::from_metadata(result.metadata.as_ref().expect("metadata is attached")) + .expect("metadata decodes as a call outcome") +} + +async fn answering_server() -> String { + async fn handle(Json(body): Json) -> axum::response::Response { + let result = match body["method"].as_str().unwrap_or_default() { + "initialize" => json!({ + "protocolVersion": tinymcp_bus::LATEST_PROTOCOL_VERSION, + "capabilities": { "tools": {} }, + "serverInfo": { "name": "echo", "version": "1.0.0" }, + }), + "notifications/initialized" => return StatusCode::ACCEPTED.into_response(), + "tools/list" => json!({ + "tools": [{ "name": "whoami", "inputSchema": { "type": "object" } }] + }), + "tools/call" => json!({ + "content": [{ "type": "text", "text": format!("you sent {SECRET}") }], + "isError": false, + }), + _ => json!({}), + }; + Json(json!({ "jsonrpc": "2.0", "id": body["id"].clone(), "result": result })) + .into_response() + } + serve(Router::new().route("/mcp", post(handle))).await +} + +async fn unauthorized_server(resource_metadata: Option<&'static str>) -> String { + let app = Router::new().route( + "/mcp", + post(move || async move { + let mut headers = HeaderMap::new(); + let challenge = resource_metadata.map_or_else( + || "Bearer realm=\"mcp\"".to_string(), + |url| format!("Bearer resource_metadata=\"{url}\""), + ); + headers.insert( + "www-authenticate", + HeaderValue::from_str(&challenge).unwrap(), + ); + (StatusCode::UNAUTHORIZED, headers, "").into_response() + }), + ); + serve(app).await +} + +async fn serve(app: Router) -> String { + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let addr = listener.local_addr().unwrap(); + tokio::spawn(async move { axum::serve(listener, app).await.unwrap() }); + format!("http://{addr}/mcp") +} + +async fn closed_endpoint() -> String { + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let addr = listener.local_addr().unwrap(); + drop(listener); + format!("http://{addr}/mcp") +} + +fn args(tool: &str) -> Value { + json!({ "server": "docs", "tool": tool, "arguments": {} }) +} + +#[tokio::test] +async fn an_answered_call_reports_ok_with_no_error() { + let registry = registry(&answering_server().await, McpAuthConfig::None, &[]); + let result = call_tool(registry).execute(args("whoami")).await.unwrap(); + assert!(!result.is_error, "{}", result.output()); + assert_eq!( + outcome_of(&result), + McpCallOutcome::answered("docs", "whoami") + ); + assert_eq!( + result.metadata.as_ref().unwrap()["kind"], + json!(MCP_CALL_RESULT_KIND) + ); +} + +#[tokio::test] +async fn the_outcome_survives_scrubbing_and_never_reaches_the_rendering() { + let registry = registry( + &answering_server().await, + McpAuthConfig::BearerToken { + token: SECRET.into(), + }, + &[], + ); + let result = call_tool(registry).execute(args("whoami")).await.unwrap(); + assert!(result.output().contains(super::scrub::REDACTED)); + assert!(!result.output().contains(SECRET)); + assert!(outcome_of(&result).ok); + assert!(!result.output().contains(MCP_CALL_RESULT_KIND)); +} + +#[tokio::test] +async fn a_blocked_tool_reports_tool_not_allowed() { + let registry = registry(&answering_server().await, McpAuthConfig::None, &["wipe"]); + let result = call_tool(registry).execute(args("wipe")).await.unwrap(); + assert!(result.is_error); + let outcome = outcome_of(&result); + assert!(!outcome.ok); + assert_eq!(outcome.tool, "wipe"); + let error = outcome.error.unwrap(); + assert_eq!(error.code, errors::TOOL_NOT_ALLOWED); + assert!(!error.unauthorized); + assert!(!error.advertises_oauth); +} + +#[tokio::test] +async fn a_401_advertising_oauth_reports_both_flags() { + let endpoint = unauthorized_server(Some("https://auth.example/.well-known/res")).await; + let result = call_tool(registry(&endpoint, McpAuthConfig::None, &[])) + .execute(args("whoami")) + .await + .unwrap(); + assert!(result.is_error); + let error = outcome_of(&result).error.unwrap(); + assert_eq!(error.code, errors::UNAUTHORIZED); + assert!(error.unauthorized); + assert!(error.advertises_oauth); +} + +#[tokio::test] +async fn a_401_without_resource_metadata_is_unauthorized_without_oauth() { + let endpoint = unauthorized_server(None).await; + let result = call_tool(registry(&endpoint, McpAuthConfig::None, &[])) + .execute(args("whoami")) + .await + .unwrap(); + let error = outcome_of(&result).error.unwrap(); + assert_eq!(error.code, errors::UNAUTHORIZED); + assert!(error.unauthorized); + assert!(!error.advertises_oauth); +} + +#[tokio::test] +async fn an_unknown_server_reports_unknown_server() { + let registry = registry(&closed_endpoint().await, McpAuthConfig::None, &[]); + let result = call_tool(registry) + .execute(json!({ "server": "nope", "tool": "whoami", "arguments": {} })) + .await + .unwrap(); + let outcome = outcome_of(&result); + assert_eq!(outcome.server, "nope"); + assert_eq!(outcome.error.unwrap().code, errors::UNKNOWN_SERVER); +} + +#[tokio::test] +async fn an_unreachable_server_reports_a_transport_error() { + let registry = registry(&closed_endpoint().await, McpAuthConfig::None, &[]); + let result = call_tool(registry).execute(args("whoami")).await.unwrap(); + assert!(result.is_error); + assert_eq!(outcome_of(&result).error.unwrap().code, errors::TRANSPORT); +} + +#[tokio::test] +async fn arguments_that_are_not_an_object_report_invalid_arguments() { + let registry = registry(&closed_endpoint().await, McpAuthConfig::None, &[]); + let result = call_tool(registry) + .execute(json!({ "server": "docs", "tool": "whoami", "arguments": [1] })) + .await + .unwrap(); + assert!(result.is_error); + let outcome = outcome_of(&result); + assert_eq!(outcome.tool, "whoami"); + assert_eq!(outcome.error.unwrap().code, errors::INVALID_ARGUMENTS); +} + +#[tokio::test] +async fn the_outcome_names_the_caller_s_server_and_tool_even_when_they_match_a_secret() { + let registry = registry( + &answering_server().await, + McpAuthConfig::BearerToken { + token: SECRET.into(), + }, + &[SECRET], + ); + let result = call_tool(registry).execute(args(SECRET)).await.unwrap(); + assert!(result.is_error); + assert!(!result.output().contains(SECRET)); + let outcome = outcome_of(&result); + assert_eq!(outcome.tool, SECRET); + assert_eq!(outcome.error.unwrap().code, errors::TOOL_NOT_ALLOWED); +} diff --git a/crates/tinymcp/src/tools/bridge_tests.rs b/crates/tinymcp/src/tools/bridge_tests.rs index 5fd27c8a..0b30cd14 100644 --- a/crates/tinymcp/src/tools/bridge_tests.rs +++ b/crates/tinymcp/src/tools/bridge_tests.rs @@ -487,3 +487,39 @@ fn an_endpoint_that_is_not_a_url_is_cut_at_its_query_or_fragment() { "https://example.com/mcp" ); } + +#[test] +fn identifiers_lose_the_markdown_a_model_wraps_them_in() { + use super::bridge::required_string_arg; + let parsed = |value: &str| required_string_arg(&json!({ "server": value }), "server").unwrap(); + assert_eq!(parsed("docs"), "docs"); + assert_eq!(parsed("docs`"), "docs"); + assert_eq!(parsed("`docs`"), "docs"); + assert_eq!(parsed("*docs*"), "docs"); + assert_eq!(parsed("`docs`."), "docs"); + assert_eq!(parsed(" **docs**: "), "docs"); + assert_eq!(parsed("***docs***!"), "docs"); + assert_eq!(parsed("my_docs-v2"), "my_docs-v2"); + assert_eq!(parsed("get_data_"), "get_data_"); + assert_eq!(parsed("__init"), "__init"); + assert_eq!(parsed("__init__"), "__init__"); + assert_eq!(parsed("_docs_"), "_docs_"); + assert_eq!(parsed("docs."), "docs."); + assert_eq!(parsed("v1.2"), "v1.2"); + assert_eq!(parsed("`get_data_`"), "get_data_"); + let error = required_string_arg(&json!({ "server": "```" }), "server").unwrap_err(); + assert_eq!(error.to_string(), "missing required `server`"); + assert!(required_string_arg(&json!({ "server": 7 }), "server").is_err()); +} + +#[tokio::test] +async fn a_fenced_server_name_reaches_the_configured_server() { + let (base, calls) = echoing_server("plain", None).await; + let registry = registry_with(&format!("{base}/mcp"), McpAuthConfig::None); + let result = call_tool(registry) + .execute(json!({ "server": "`docs`", "tool": "whoami`", "arguments": {} })) + .await + .unwrap(); + assert!(!result.is_error, "{}", full_output(&result)); + assert_eq!(calls.lock().len(), 1); +} diff --git a/crates/tinymcp/src/tools/mod.rs b/crates/tinymcp/src/tools/mod.rs index 1fa439c8..11b63643 100644 --- a/crates/tinymcp/src/tools/mod.rs +++ b/crates/tinymcp/src/tools/mod.rs @@ -102,6 +102,9 @@ pub fn tools_for( #[path = "mod_tests.rs"] mod test; +#[cfg(test)] +#[path = "bridge_outcome_tests.rs"] +mod bridge_outcome_test; #[cfg(test)] #[path = "bridge_tests.rs"] mod bridge_test; diff --git a/crates/tinymcp/src/tools/scrub.rs b/crates/tinymcp/src/tools/scrub.rs index f57133bd..4616d1b8 100644 --- a/crates/tinymcp/src/tools/scrub.rs +++ b/crates/tinymcp/src/tools/scrub.rs @@ -30,12 +30,21 @@ const CREDENTIAL_QUERY_PARAM_NEEDLES: [&str; 7] = [ ]; /// Every secret one configured server was dialled with, ready to scrub. -#[derive(Debug, Clone)] +#[derive(Clone)] pub struct SecretScrubber { pub(super) secrets: Vec, strict: Vec, } +impl std::fmt::Debug for SecretScrubber { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("SecretScrubber") + .field("secrets", &self.secrets.len()) + .field("strict", &self.strict.len()) + .finish() + } +} + impl SecretScrubber { /// The scrubber for the server named `server`, or an empty one (which /// scrubs nothing) when `registry` has no such server. @@ -132,11 +141,35 @@ impl SecretScrubber { } secrets.push(value.to_string()); } - secrets.sort_by(|a, b| b.len().cmp(&a.len()).then_with(|| a.cmp(b))); - secrets.dedup(); + sort_longest_first(&mut secrets); Self { secrets, strict } } + /// This scrubber, also replacing each of `secrets`. + /// + /// For credentials the server's own configuration does not carry: a token + /// a host keeps in its own secret store, or a value it injected some other + /// way. Each is matched as typed and URL-encoded, anywhere in the text even + /// when short; blank values are skipped. + #[must_use] + pub fn with_secrets(mut self, secrets: impl IntoIterator) -> Self { + for secret in secrets { + let secret = secret.trim(); + if secret.is_empty() { + continue; + } + let encoded = urlencoding::encode(secret).into_owned(); + if encoded != secret { + self.strict.push(encoded.clone()); + self.secrets.push(encoded); + } + self.strict.push(secret.to_string()); + self.secrets.push(secret.to_string()); + } + sort_longest_first(&mut self.secrets); + self + } + /// `text` with every known secret replaced. #[must_use] pub fn scrub(&self, text: &str) -> String { @@ -255,6 +288,12 @@ impl SecretScrubber { } } +/// Longest first, so a secret that contains another is replaced whole. +fn sort_longest_first(secrets: &mut Vec) { + secrets.sort_by(|a, b| b.len().cmp(&a.len()).then_with(|| a.cmp(b))); + secrets.dedup(); +} + fn endpoint_query(endpoint: &str) -> Option<&str> { let (_, rest) = endpoint.split_once('?')?; let query = rest.split('#').next().unwrap_or_default(); diff --git a/crates/tinymcp/src/tools/scrub_tests.rs b/crates/tinymcp/src/tools/scrub_tests.rs index 6ba8aa94..9e0a2631 100644 --- a/crates/tinymcp/src/tools/scrub_tests.rs +++ b/crates/tinymcp/src/tools/scrub_tests.rs @@ -125,3 +125,64 @@ fn scrub_value_keeps_both_entries_when_keys_collide_after_redaction() { assert_eq!(map.get("[redacted]"), Some(&json!("a"))); assert_eq!(map.get("[redacted] (2)"), Some(&json!("b"))); } + +#[test] +fn extra_secrets_are_redacted_as_typed_and_url_encoded() { + let scrubber = SecretScrubber::new(&McpAuthConfig::None, "https://example.com/mcp") + .with_secrets(["host token/1".to_string(), " ".to_string()]); + assert_eq!( + scrubber.scrub("a host token/1 b host%20token%2F1 c"), + "a [redacted] b [redacted] c" + ); + assert_eq!(scrubber.secrets.len(), 2); +} + +#[test] +fn extra_secrets_join_the_configured_ones_longest_first() { + let scrubber = SecretScrubber::new( + &McpAuthConfig::BearerToken { + token: "configured-token".into(), + }, + "https://example.com/mcp", + ) + .with_secrets(["configured-token-extended".to_string()]); + assert_eq!( + scrubber.scrub("configured-token-extended and configured-token"), + "[redacted] and [redacted]" + ); + let result = scrubber.scrub_result(tinytools::ToolResult::json( + json!({ "echo": "configured-token-extended" }), + )); + assert!(!format!("{:?}", result.content).contains("configured-token")); +} + +#[test] +fn a_short_extra_secret_is_redacted_inside_larger_text() { + let scrubber = SecretScrubber::new(&McpAuthConfig::None, "https://example.com/mcp") + .with_secrets(["k9".to_string()]); + assert_eq!( + scrubber.scrub("token=xk9y and k9"), + "token=x[redacted]y and [redacted]" + ); + let mut value = json!({ "k9": "a", "tool_k9x": "b" }); + scrubber.scrub_value(&mut value); + let map = value.as_object().unwrap(); + assert!(map.contains_key("[redacted]"), "{value}"); + assert!(map.contains_key("tool_k9x"), "{value}"); +} + +#[test] +fn debug_output_never_contains_a_secret() { + let scrubber = SecretScrubber::new( + &McpAuthConfig::BearerToken { + token: "configured-token".into(), + }, + "https://example.com/mcp", + ) + .with_secrets(["host token/1".to_string()]); + let shown = format!("{scrubber:?} {scrubber:#?}"); + assert!(!shown.contains("configured-token"), "{shown}"); + assert!(!shown.contains("host"), "{shown}"); + assert!(!shown.contains("%2F"), "{shown}"); + assert!(shown.contains("SecretScrubber"), "{shown}"); +} diff --git a/crates/tinymcp/tests/public_reexports.rs b/crates/tinymcp/tests/public_reexports.rs new file mode 100644 index 00000000..7dcd3e2c --- /dev/null +++ b/crates/tinymcp/tests/public_reexports.rs @@ -0,0 +1,23 @@ +//! Pins the crate-root paths a host imports the contract types from. + +use tinymcp::{ + CONTRACT_VERSION, MCP_CALL_RESULT_KIND, McpAuthChallenge, McpCallError, McpCallOutcome, +}; + +#[test] +fn the_contract_types_resolve_at_the_crate_root_to_the_bus_definitions() { + let challenge: tinymcp_bus::McpAuthChallenge = McpAuthChallenge { + scheme: "Bearer".into(), + realm: None, + resource_metadata: None, + }; + assert_eq!(challenge.scheme, "Bearer"); + + let outcome: tinymcp_bus::McpCallOutcome = McpCallOutcome::failed( + "docs", + "search", + McpCallError::new(tinymcp_bus::errors::TRANSPORT), + ); + assert_eq!(outcome.kind, MCP_CALL_RESULT_KIND); + assert_eq!(CONTRACT_VERSION, tinymcp_bus::CONTRACT_VERSION); +}