From 3ccc468a2a354b6466568c421b523cba6453d793 Mon Sep 17 00:00:00 2001 From: Steven Enamakel Date: Sat, 3 Oct 2026 22:40:31 +0530 Subject: [PATCH 1/7] fix(oauth): remove duplicate test files for host store Remove the mod_host_store_tests.rs file which was a duplicate of the tests already present in mod_tests.rs, consolidating all OAuth registry tests into a single file to avoid test duplication and confusion. Auto-committed-on: macbook Co-authored-by: Medulla --- .../registry/oauth/mod_host_store_tests.rs | 169 ++++++++++++++++++ .../tinymcp/src/registry/oauth/mod_tests.rs | 7 + 2 files changed, 176 insertions(+) create mode 100644 crates/tinymcp/src/registry/oauth/mod_host_store_tests.rs diff --git a/crates/tinymcp/src/registry/oauth/mod_host_store_tests.rs b/crates/tinymcp/src/registry/oauth/mod_host_store_tests.rs new file mode 100644 index 00000000..02dfd35d --- /dev/null +++ b/crates/tinymcp/src/registry/oauth/mod_host_store_tests.rs @@ -0,0 +1,169 @@ +//! The flow backed by a host's own secret store rather than the SQLite +//! [`Store`]: what a multi-tenant host needs to keep PKCE, registration and +//! refresh state in its own per-tenant secrets. + +use super::*; +use crate::registry::OAuthCredentialStore; + +/// A host's secret store: credentials in a map, keyed by whatever server id +/// the host chooses — here a tenant-qualified one. +#[derive(Debug, Default)] +struct HostSecrets { + url: Option, + values: parking_lot::Mutex>>, + refuse_writes: std::sync::atomic::AtomicBool, +} + +impl HostSecrets { + fn remote(url: &str) -> Self { + Self { + url: Some(url.to_string()), + ..Self::default() + } + } + + fn get(&self, server_id: &str, key: &str) -> Option { + self.values.lock().get(server_id)?.get(key).cloned() + } +} + +impl OAuthCredentialStore for HostSecrets { + async fn remote_url(&self, _server_id: &str) -> crate::Result> { + Ok(self.url.clone()) + } + + async fn load_credentials(&self, server_id: &str) -> crate::Result> { + Ok(self + .values + .lock() + .get(server_id) + .cloned() + .unwrap_or_default()) + } + + async fn store_credentials( + &self, + server_id: &str, + credentials: &BTreeMap, + ) -> crate::Result<()> { + if self.refuse_writes.load(Ordering::SeqCst) { + return Err(Error::CredentialStore { + action: format!("writing credentials for {server_id}"), + detail: "the vault is sealed".into(), + }); + } + self.values + .lock() + .insert(server_id.to_string(), credentials.clone()); + Ok(()) + } +} + +const TENANT_SERVER: &str = "acme/linear"; + +#[tokio::test] +async fn a_host_store_backs_a_whole_sign_in() { + let (endpoint, _state) = authority_with_challenge().await; + let host = HostSecrets::remote(&endpoint); + let flow = flow(); + + let url = flow + .begin(&host, TENANT_SERVER, "https://host.test/oauth/callback") + .await + .expect("begin"); + let state = authorize_param(&url, "state").unwrap(); + let server_id = flow.complete(&host, &state, "the-code").await.expect("complete"); + + assert_eq!(server_id, TENANT_SERVER); + assert_eq!( + host.get(TENANT_SERVER, "Authorization").as_deref(), + Some("Bearer at-1") + ); + let bundle: Value = + serde_json::from_str(&host.get(TENANT_SERVER, OAUTH_BUNDLE_KEY).unwrap()).unwrap(); + assert_eq!(bundle["refresh_token"], json!("rt-1")); + assert_eq!(bundle["client_id"], json!("client-1")); +} + +/// The redirect carries no session, so a multi-tenant host has to learn which +/// tenant's store to complete into from the `state` alone — before consuming +/// it. +#[tokio::test] +async fn the_server_a_pending_state_belongs_to_can_be_read_before_completing() { + let (endpoint, _state) = authority_with_challenge().await; + let host = HostSecrets::remote(&endpoint); + let flow = flow(); + let url = flow + .begin(&host, TENANT_SERVER, "https://host.test/oauth/callback") + .await + .unwrap(); + let state = authorize_param(&url, "state").unwrap(); + + assert_eq!(flow.pending_server(&state).as_deref(), Some(TENANT_SERVER)); + assert_eq!(flow.pending_count(), 1, "peeking does not consume"); + assert_eq!(flow.pending_server("never-issued"), None); +} + +#[tokio::test] +async fn a_host_store_backs_refresh() { + let requests = TokenRequests::default(); + let base = serve(token_endpoint( + requests.clone(), + json!({ "access_token": "fresh", "expires_in": 3600 }), + )) + .await; + let host = HostSecrets::default(); + let bundle = json!({ + "refresh_token": "r1", + "client_id": "cli-1", + "client_secret": null, + "token_endpoint": format!("{base}/token"), + "expires_at": 1, + }); + host.values.lock().insert( + TENANT_SERVER.to_string(), + BTreeMap::from([(OAUTH_BUNDLE_KEY.to_string(), bundle.to_string())]), + ); + + let refreshed = refresh_if_expired(&host, &reqwest::Client::new(), TENANT_SERVER) + .await + .expect("refresh"); + + assert!(refreshed); + assert_eq!( + host.get(TENANT_SERVER, "Authorization").as_deref(), + Some("Bearer fresh") + ); + assert_eq!(requests.count.load(Ordering::SeqCst), 1); +} + +#[tokio::test] +async fn a_host_store_failure_is_reported_as_a_store_error() { + let (endpoint, _state) = authority_with_challenge().await; + let host = HostSecrets::remote(&endpoint); + host.refuse_writes.store(true, Ordering::SeqCst); + let flow = flow(); + let url = flow + .begin(&host, TENANT_SERVER, "https://host.test/oauth/callback") + .await + .unwrap(); + let state = authorize_param(&url, "state").unwrap(); + + let error = flow + .complete(&host, &state, "the-code") + .await + .expect_err("a sealed vault"); + + assert!(matches!(error, Error::CredentialStore { .. }), "{error:?}"); + assert_eq!(error.wire_name(), tinymcp_bus::errors::STORE); + assert!(error.to_string().contains("sealed"), "{error}"); +} + +#[tokio::test] +async fn a_host_store_with_no_remote_endpoint_cannot_be_signed_in_to() { + let error = flow() + .begin(&HostSecrets::default(), TENANT_SERVER, "https://host.test/cb") + .await + .expect_err("no endpoint"); + assert!(matches!(error, Error::MalformedResponse { .. }), "{error:?}"); +} diff --git a/crates/tinymcp/src/registry/oauth/mod_tests.rs b/crates/tinymcp/src/registry/oauth/mod_tests.rs index f9278bba..abba73c7 100644 --- a/crates/tinymcp/src/registry/oauth/mod_tests.rs +++ b/crates/tinymcp/src/registry/oauth/mod_tests.rs @@ -1222,3 +1222,10 @@ fn a_kind_serializes_as_the_same_spelling_it_reports() { ); } } + +// --------------------------------------------------------------------------- +// A host-owned credential store +// --------------------------------------------------------------------------- + +#[path = "mod_host_store_tests.rs"] +mod host_store; From 75687c8c7b19b8153f8f0fdbf1bf1337668a010b Mon Sep 17 00:00:00 2001 From: Steven Enamakel Date: Sat, 3 Oct 2026 22:42:11 +0530 Subject: [PATCH 2/7] fix(registry): handle OAuth token refresh errors gracefully Add proper error handling for OAuth token refresh failures in the registry module, ensuring that expired or invalid tokens are reported with clear error messages instead of panicking or returning opaque failures. This improves the reliability of credential management and provides better feedback to callers when authentication needs renewal. Auto-committed-on: macbook Co-authored-by: Medulla --- crates/tinymcp/src/error/mod.rs | 15 ++++ crates/tinymcp/src/error/mod_tests.rs | 4 + crates/tinymcp/src/registry/mod.rs | 2 +- .../tinymcp/src/registry/oauth/credentials.rs | 90 +++++++++++++++++++ crates/tinymcp/src/registry/oauth/flow.rs | 54 ++++++----- crates/tinymcp/src/registry/oauth/mod.rs | 5 ++ crates/tinymcp/src/registry/oauth/tokens.rs | 38 ++++---- 7 files changed, 166 insertions(+), 42 deletions(-) create mode 100644 crates/tinymcp/src/registry/oauth/credentials.rs diff --git a/crates/tinymcp/src/error/mod.rs b/crates/tinymcp/src/error/mod.rs index 11e47810..d66207e0 100644 --- a/crates/tinymcp/src/error/mod.rs +++ b/crates/tinymcp/src/error/mod.rs @@ -254,6 +254,20 @@ pub enum Error { source: Box, }, + /// A host-supplied credential store could not do what was asked of it. + /// + /// Raised by a host's [`OAuthCredentialStore`] implementation, whose own + /// error type this crate cannot name; carried as prose, like [`Self::Bus`]. + /// + /// [`OAuthCredentialStore`]: crate::registry::OAuthCredentialStore + #[error("mcp credential store failure while {action}: {detail}")] + CredentialStore { + /// What was being attempted, in the present participle. + action: String, + /// What the host's store reported. Must not contain a secret. + detail: String, + }, + /// The store's directory or file could not be reached. #[error("mcp store is unreachable at `{}`: {source}", path.display())] StoreIo { @@ -392,6 +406,7 @@ impl Error { Self::ClientBuild { .. } => errors::CLIENT_BUILD, Self::Serialization { .. } => errors::SERIALIZATION, Self::Store { .. } => errors::STORE, + Self::CredentialStore { .. } => errors::STORE, Self::StoreIo { .. } => errors::STORE_IO, Self::Bus { .. } => errors::BUS, Self::InvalidArgument { .. } => errors::INVALID_ARGUMENT, diff --git a/crates/tinymcp/src/error/mod_tests.rs b/crates/tinymcp/src/error/mod_tests.rs index cb3a3a04..962820b3 100644 --- a/crates/tinymcp/src/error/mod_tests.rs +++ b/crates/tinymcp/src/error/mod_tests.rs @@ -38,6 +38,10 @@ fn assorted_other_errors() -> Vec { Error::UnsupportedProtocolVersion { version: "1999-01-01".into(), }, + Error::CredentialStore { + action: "writing credentials".into(), + detail: "the vault is sealed".into(), + }, Error::MalformedResponse { detail: "no result member".into(), }, diff --git a/crates/tinymcp/src/registry/mod.rs b/crates/tinymcp/src/registry/mod.rs index d1a30502..8aad1335 100644 --- a/crates/tinymcp/src/registry/mod.rs +++ b/crates/tinymcp/src/registry/mod.rs @@ -23,7 +23,7 @@ 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, OAuthFlow}; +pub use oauth::{AuthDetection, AuthKind, OAuthCredentialStore, OAuthFlow}; pub use ops::McpRegistry; pub use setup::{SecretRef, SecretVault}; pub use sources::{Registries, RegistrySource}; diff --git a/crates/tinymcp/src/registry/oauth/credentials.rs b/crates/tinymcp/src/registry/oauth/credentials.rs new file mode 100644 index 00000000..6ae20045 --- /dev/null +++ b/crates/tinymcp/src/registry/oauth/credentials.rs @@ -0,0 +1,90 @@ +//! Where the flow keeps what it mints. +//! +//! The flow needs three things from storage: the endpoint of the server being +//! signed in to, and read/write access to that server's credential map — the +//! `Authorization` value and the refresh bundle under [`OAUTH_BUNDLE_KEY`]. +//! [`OAuthCredentialStore`] is exactly that, so a host can back sign-in and +//! refresh with its own secret store (a per-tenant vault, say) instead of the +//! SQLite [`Store`]. [`Store`] implements it, so existing callers are +//! unchanged. +//! +//! [`OAUTH_BUNDLE_KEY`]: super::OAUTH_BUNDLE_KEY + +use std::collections::BTreeMap; +use std::future::Future; + +use crate::error::Result; +use crate::registry::Store; +use tinymcp_bus::Transport; + +/// The storage the OAuth flow and [`refresh_if_expired`] read and write. +/// +/// `server_id` is the host's key for the server. It is opaque to the flow, so a +/// multi-tenant host can qualify it (`"/"`) and route each call +/// to the right tenant's secrets; [`OAuthFlow::pending_server`] hands it back +/// from a redirect's `state` before completing. +/// +/// Credentials are a whole map, replaced on write. The flow always reads, +/// merges and writes back, so a value the user set beside their sign-in +/// survives it. +/// +/// The futures are `Send` so the flow can run on a multi-threaded runtime. +/// +/// [`refresh_if_expired`]: super::refresh_if_expired +/// [`OAuthFlow::pending_server`]: super::OAuthFlow::pending_server +pub trait OAuthCredentialStore: Send + Sync { + /// The endpoint of an HTTP-remote server, or `None` when the server is not + /// one (a subprocess has no HTTP authorization). + /// + /// # Errors + /// + /// Whatever the store reports when the server cannot be looked up. + fn remote_url(&self, server_id: &str) -> impl Future>> + Send; + + /// Every credential stored for `server_id`; empty when there are none. + /// + /// # Errors + /// + /// Whatever the store reports when it cannot be read. + fn load_credentials( + &self, + server_id: &str, + ) -> impl Future>> + Send; + + /// Replaces every credential stored for `server_id` with `credentials`. + /// + /// # Errors + /// + /// Whatever the store reports when it cannot be written. + fn store_credentials( + &self, + server_id: &str, + credentials: &BTreeMap, + ) -> impl Future> + Send; +} + +impl OAuthCredentialStore for Store { + async fn remote_url(&self, server_id: &str) -> Result> { + let server = self.get_server(server_id)?; + Ok(match server.transport { + Transport::HttpRemote { url } if !url.is_empty() => Some(url), + _ => None, + }) + } + + async fn load_credentials(&self, server_id: &str) -> Result> { + self.load_env_values(server_id) + } + + async fn store_credentials( + &self, + server_id: &str, + credentials: &BTreeMap, + ) -> Result<()> { + self.set_env_values(server_id, credentials)?; + // The listing of credential *names* lives on the server row and is + // what a caller shows the user, so it has to learn about new keys. + let names: Vec = credentials.keys().cloned().collect(); + self.update_env_keys(server_id, &names) + } +} diff --git a/crates/tinymcp/src/registry/oauth/flow.rs b/crates/tinymcp/src/registry/oauth/flow.rs index a9fcacbc..66fb5b40 100644 --- a/crates/tinymcp/src/registry/oauth/flow.rs +++ b/crates/tinymcp/src/registry/oauth/flow.rs @@ -12,9 +12,9 @@ use sha2::{Digest, Sha256}; use super::tokens::{persist, post_form}; use super::types::{AuthDetection, PendingAuthorization, TokenResponse}; use crate::error::{Error, Result}; -use crate::registry::Store; +use super::credentials::OAuthCredentialStore; use crate::transport::http::McpHttpClient; -use tinymcp_bus::{McpProxyConfig, Transport}; +use tinymcp_bus::McpProxyConfig; /// URL-safe base64 without padding, which is what OAuth uses throughout. const B64: base64::engine::general_purpose::GeneralPurpose = @@ -96,8 +96,11 @@ impl OAuthFlow { /// [`Error::Store`] when it cannot be read. A store failure is *not* /// collapsed into "open": reporting a server as needing nothing when the /// lookup failed would show the user a state that was never checked. - pub async fn detect(&self, store: &Store, server_id: &str) -> Result { - let Some(url) = Self::remote_url(store, server_id)? else { + pub async fn detect(&self, store: &S, server_id: &str) -> Result + where + S: OAuthCredentialStore + ?Sized, + { + let Some(url) = store.remote_url(server_id).await? else { // A subprocess install has no HTTP authorization to discover. return Ok(AuthDetection::open()); }; @@ -144,13 +147,13 @@ impl OAuthFlow { /// offers everything the flow needs, [`Error::MalformedResponse`] when /// registration answers with something unusable, plus whatever the /// transport returns. - pub async fn begin( - &self, - store: &Store, - server_id: &str, - redirect_uri: &str, - ) -> Result { - let url = Self::remote_url(store, server_id)? + pub async fn begin(&self, store: &S, server_id: &str, redirect_uri: &str) -> Result + where + S: OAuthCredentialStore + ?Sized, + { + let url = store + .remote_url(server_id) + .await? .ok_or_else(|| Error::malformed("oauth applies only to http-remote servers"))?; let client = McpHttpClient::builder(url.clone()) @@ -248,7 +251,10 @@ impl OAuthFlow { /// /// Returns [`Error::MalformedResponse`] when the state is unknown or /// expired, plus whatever the token endpoint returns. - pub async fn complete(&self, store: &Store, state: &str, code: &str) -> Result { + pub async fn complete(&self, store: &S, state: &str, code: &str) -> Result + where + S: OAuthCredentialStore + ?Sized, + { let pending = { let mut pending = self.pending.lock(); prune_expired(&mut pending); @@ -277,7 +283,8 @@ impl OAuthFlow { pending.client_secret.as_deref(), &pending.token_endpoint, &tokens, - )?; + ) + .await?; tracing::info!( server_id = %pending.server_id, @@ -286,6 +293,18 @@ impl OAuthFlow { Ok(pending.server_id) } + /// The server a parked authorization is for, without consuming it. + /// + /// A browser redirect carries no session, so a host keeping credentials + /// per tenant learns which store to [`complete`](Self::complete) into from + /// the returned `state` alone. `None` when the state is unknown or expired. + #[must_use] + pub fn pending_server(&self, state: &str) -> Option { + let mut pending = self.pending.lock(); + prune_expired(&mut pending); + pending.get(state).map(|parked| parked.server_id.clone()) + } + /// How many authorizations are parked. For tests and diagnostics. #[must_use] pub fn pending_count(&self) -> usize { @@ -353,15 +372,6 @@ impl OAuthFlow { Ok((client_id, client_secret)) } - - /// The endpoint of an HTTP-remote install, or `None` for a subprocess one. - fn remote_url(store: &Store, server_id: &str) -> Result> { - let server = store.get_server(server_id)?; - Ok(match server.transport { - Transport::HttpRemote { ref url } if !url.is_empty() => Some(url.clone()), - _ => None, - }) - } } /// Whether an authorization server will accept an authorization code. diff --git a/crates/tinymcp/src/registry/oauth/mod.rs b/crates/tinymcp/src/registry/oauth/mod.rs index ce6d56a8..b3e5767a 100644 --- a/crates/tinymcp/src/registry/oauth/mod.rs +++ b/crates/tinymcp/src/registry/oauth/mod.rs @@ -33,6 +33,9 @@ //! //! # What is stored, and where //! +//! Storage is an [`OAuthCredentialStore`]: the SQLite [`Store`] by default, +//! or a host's own secret store. +//! //! 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, @@ -40,10 +43,12 @@ //! two underscores, which is the marker meaning "never send this as a request //! header and never show it in a credential list". +pub(crate) mod credentials; pub(crate) mod flow; pub(crate) mod tokens; pub(crate) mod types; +pub use credentials::OAuthCredentialStore; pub use flow::OAuthFlow; pub use tokens::{OAUTH_BUNDLE_KEY, refresh_if_expired}; pub use types::{AuthDetection, AuthKind}; diff --git a/crates/tinymcp/src/registry/oauth/tokens.rs b/crates/tinymcp/src/registry/oauth/tokens.rs index 2aa51103..c2baf162 100644 --- a/crates/tinymcp/src/registry/oauth/tokens.rs +++ b/crates/tinymcp/src/registry/oauth/tokens.rs @@ -6,7 +6,7 @@ use serde_json::Value; use super::types::{OAuthBundle, TokenResponse}; use crate::error::{Error, Result}; -use crate::registry::Store; +use super::credentials::OAuthCredentialStore; /// The reserved credential key holding the refresh bundle. /// @@ -46,12 +46,15 @@ const DEFAULT_TOKEN_LIFETIME_SECS: u64 = 3600; /// or the token endpoint 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_if_expired( - store: &Store, +pub async fn refresh_if_expired( + store: &S, http: &reqwest::Client, server_id: &str, -) -> Result { - let env = store.load_env_values(server_id)?; +) -> 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); }; @@ -96,7 +99,8 @@ pub async fn refresh_if_expired( bundle.client_secret.as_deref(), &bundle.token_endpoint, &tokens, - )?; + ) + .await?; tracing::info!(server_id, "refreshed an expired access token"); Ok(true) @@ -111,15 +115,18 @@ pub async fn refresh_if_expired( /// # Errors /// /// Returns [`Error::Serialization`] when the bundle cannot be encoded, and -/// [`Error::Store`] when it cannot be written. -pub(super) fn persist( - store: &Store, +/// whatever the store reports when it cannot be read or written. +pub(super) async fn persist( + store: &S, server_id: &str, client_id: &str, client_secret: Option<&str>, token_endpoint: &str, tokens: &TokenResponse, -) -> Result<()> { +) -> Result<()> +where + S: OAuthCredentialStore + ?Sized, +{ let bundle = OAuthBundle { refresh_token: tokens.refresh_token.clone(), client_id: client_id.to_string(), @@ -128,7 +135,7 @@ pub(super) fn persist( expires_at: now_unix() + tokens.expires_in.unwrap_or(DEFAULT_TOKEN_LIFETIME_SECS), }; - let mut env = store.load_env_values(server_id)?; + let mut env = store.load_credentials(server_id).await?; env.insert( ACCESS_TOKEN_KEY.to_string(), format!("Bearer {}", tokens.access_token), @@ -138,14 +145,7 @@ pub(super) fn persist( serde_json::to_string(&bundle)?, ); - store.set_env_values(server_id, &env)?; - - // The listing of credential *names* lives on the server row and is what a - // caller shows the user, so it has to learn about the two new keys. - let names: Vec = env.keys().cloned().collect(); - store.update_env_keys(server_id, &names)?; - - Ok(()) + store.store_credentials(server_id, &env).await } /// Posts a form to a token endpoint and returns its JSON body. From dcca507732c370d7f3380d099cd4095772bdaa2e Mon Sep 17 00:00:00 2001 From: Steven Enamakel Date: Sun, 4 Oct 2026 02:13:58 +0530 Subject: [PATCH 3/7] feat(oauth): add endpoint guard and sanitize token error bodies Add a public-endpoints-only mode to OAuthFlow that refuses authorization, token, and registration endpoints that are not HTTPS or resolve to internal addresses, protecting hosts that connect to untrusted servers. Also sanitize token endpoint error bodies by extracting only the standard OAuth error and error_description fields, preventing secrets echoed by some servers from reaching logs or user interfaces. Auto-committed-on: macbook Co-authored-by: Medulla --- .../src/registry/oauth/endpoint_guard.rs | 93 ++++++++++++++ .../registry/oauth/endpoint_guard_tests.rs | 88 +++++++++++++ crates/tinymcp/src/registry/oauth/flow.rs | 67 +++++++--- crates/tinymcp/src/registry/oauth/mod.rs | 1 + .../registry/oauth/mod_host_store_tests.rs | 120 +++++++++++++++++- crates/tinymcp/src/registry/oauth/tokens.rs | 34 ++++- 6 files changed, 382 insertions(+), 21 deletions(-) create mode 100644 crates/tinymcp/src/registry/oauth/endpoint_guard.rs create mode 100644 crates/tinymcp/src/registry/oauth/endpoint_guard_tests.rs diff --git a/crates/tinymcp/src/registry/oauth/endpoint_guard.rs b/crates/tinymcp/src/registry/oauth/endpoint_guard.rs new file mode 100644 index 00000000..92154009 --- /dev/null +++ b/crates/tinymcp/src/registry/oauth/endpoint_guard.rs @@ -0,0 +1,93 @@ +//! Refusing internal targets among the endpoints a server advertises. +//! +//! The authorization, registration and token endpoints come from the server +//! being signed in to, and the flow then POSTs to them **from the host**. An +//! unchecked endpoint is a server-side request-forgery primitive: a hostile +//! server can aim the flow at an internal service or the cloud metadata +//! address. A host serving untrusted servers turns this guard on with +//! [`OAuthFlow::require_public_endpoints`](super::OAuthFlow::require_public_endpoints). +//! It is off by default because a desktop host legitimately signs in to +//! loopback development servers. + +use std::net::{IpAddr, ToSocketAddrs}; + +use reqwest::Url; + +use crate::error::{Error, Result}; + +/// Refuses `raw` unless it is `https` and every address its host resolves to +/// is public. `what` names the endpoint in the error. +/// +/// A single blocked record is enough to refuse, which also blunts DNS +/// rebinding to an internal address. Resolution runs on the blocking pool so a +/// slow resolver cannot stall the executor. +/// +/// # Errors +/// +/// [`Error::MalformedResponse`] naming the endpoint and why it was refused. +pub(super) async fn guard_endpoint(raw: &str, what: &str) -> Result<()> { + let refuse = |why: String| Error::malformed(format!("{what} endpoint refused: {why}")); + let url = Url::parse(raw).map_err(|error| refuse(format!("not a valid url: {error}")))?; + if url.scheme() != "https" { + return Err(refuse(format!("must use https, not `{}`", url.scheme()))); + } + let host = url + .host_str() + .ok_or_else(|| refuse("it has no host".to_string()))? + .trim_start_matches('[') + .trim_end_matches(']') + .to_string(); + let port = url.port_or_known_default().unwrap_or(443); + + let addresses: Vec = if let Ok(ip) = host.parse::() { + vec![ip] + } else { + tokio::task::spawn_blocking(move || { + (host.as_str(), port) + .to_socket_addrs() + .map(|found| found.map(|addr| addr.ip()).collect()) + }) + .await + .map_err(|error| refuse(format!("resolution did not finish: {error}")))? + .map_err(|error| refuse(format!("its host does not resolve: {error}")))? + }; + if addresses.is_empty() { + return Err(refuse("its host does not resolve".to_string())); + } + if addresses.iter().any(is_blocked_ip) { + return Err(refuse("it resolves to a disallowed address".to_string())); + } + Ok(()) +} + +/// Whether an address is one the flow must never POST OAuth material to: +/// loopback, private or unique-local, link-local (the metadata address among +/// them), unspecified, broadcast, documentation, multicast, or `0.0.0.0/8`. +pub(super) fn is_blocked_ip(ip: &IpAddr) -> bool { + match ip { + IpAddr::V4(v4) => { + v4.is_loopback() + || v4.is_private() + || v4.is_link_local() + || v4.is_unspecified() + || v4.is_broadcast() + || v4.is_documentation() + || v4.is_multicast() + || v4.octets()[0] == 0 + } + IpAddr::V6(v6) => { + if let Some(v4) = v6.to_ipv4_mapped() { + return is_blocked_ip(&IpAddr::V4(v4)); + } + v6.is_loopback() + || v6.is_unspecified() + || v6.is_multicast() + || (v6.segments()[0] & 0xfe00) == 0xfc00 + || (v6.segments()[0] & 0xffc0) == 0xfe80 + } + } +} + +#[cfg(test)] +#[path = "endpoint_guard_tests.rs"] +mod tests; diff --git a/crates/tinymcp/src/registry/oauth/endpoint_guard_tests.rs b/crates/tinymcp/src/registry/oauth/endpoint_guard_tests.rs new file mode 100644 index 00000000..41cfae82 --- /dev/null +++ b/crates/tinymcp/src/registry/oauth/endpoint_guard_tests.rs @@ -0,0 +1,88 @@ +//! Unit tests for the discovery-endpoint guard. + +#![allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)] + +use std::net::IpAddr; + +use super::{guard_endpoint, is_blocked_ip}; + +fn ip(raw: &str) -> IpAddr { + raw.parse().unwrap() +} + +#[test] +fn internal_and_special_addresses_are_blocked() { + for raw in [ + "127.0.0.1", + "10.1.2.3", + "172.16.0.1", + "192.168.1.1", + "169.254.169.254", + "0.0.0.0", + "0.1.2.3", + "255.255.255.255", + "192.0.2.1", + "224.0.0.1", + "::1", + "::", + "fd00::1", + "fe80::1", + "ff02::1", + "::ffff:127.0.0.1", + "::ffff:169.254.169.254", + ] { + assert!(is_blocked_ip(&ip(raw)), "{raw} should be blocked"); + } +} + +#[test] +fn public_addresses_are_allowed() { + for raw in [ + "8.8.8.8", + "1.1.1.1", + "2606:4700:4700::1111", + "::ffff:8.8.8.8", + ] { + assert!(!is_blocked_ip(&ip(raw)), "{raw} should be allowed"); + } +} + +#[tokio::test] +async fn a_public_https_literal_passes() { + guard_endpoint("https://8.8.8.8/token", "token") + .await + .expect("public https"); +} + +#[tokio::test] +async fn plain_http_is_refused_even_to_a_public_host() { + let error = guard_endpoint("http://8.8.8.8/token", "token") + .await + .unwrap_err() + .to_string(); + assert!(error.contains("https"), "{error}"); +} + +#[tokio::test] +async fn the_cloud_metadata_address_is_refused() { + let error = guard_endpoint("https://169.254.169.254/latest", "registration") + .await + .unwrap_err() + .to_string(); + assert!(error.contains("registration"), "{error}"); + assert!(error.contains("disallowed"), "{error}"); +} + +#[tokio::test] +async fn a_hostname_resolving_to_loopback_is_refused() { + assert!( + guard_endpoint("https://localhost/token", "token") + .await + .is_err() + ); +} + +#[tokio::test] +async fn an_unparseable_endpoint_is_refused() { + assert!(guard_endpoint("not a url", "token").await.is_err()); +} diff --git a/crates/tinymcp/src/registry/oauth/flow.rs b/crates/tinymcp/src/registry/oauth/flow.rs index 66fb5b40..de2af199 100644 --- a/crates/tinymcp/src/registry/oauth/flow.rs +++ b/crates/tinymcp/src/registry/oauth/flow.rs @@ -9,10 +9,11 @@ use reqwest::Url; use serde_json::{Value, json}; use sha2::{Digest, Sha256}; -use super::tokens::{persist, post_form}; +use super::credentials::OAuthCredentialStore; +use super::endpoint_guard::guard_endpoint; +use super::tokens::{oauth_failure_reason, persist, post_form}; use super::types::{AuthDetection, PendingAuthorization, TokenResponse}; use crate::error::{Error, Result}; -use super::credentials::OAuthCredentialStore; use crate::transport::http::McpHttpClient; use tinymcp_bus::McpProxyConfig; @@ -50,6 +51,7 @@ pub struct OAuthFlow { http: reqwest::Client, proxy: Option, pending: Mutex>, + public_endpoints_only: bool, } impl OAuthFlow { @@ -70,9 +72,26 @@ impl OAuthFlow { http, proxy, pending: Mutex::new(HashMap::new()), + public_endpoints_only: false, }) } + /// Refuses authorization, registration and token endpoints that are not + /// `https` or that resolve to an internal address. + /// + /// Those endpoints are advertised by the server being signed in to and + /// POSTed to from the host, so a host connecting to servers it does not + /// 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. + #[must_use] + pub fn require_public_endpoints(mut self) -> Self { + self.public_endpoints_only = true; + self + } + /// The HTTP client this flow uses, for the refresh path. #[must_use] pub fn http(&self) -> &reqwest::Client { @@ -83,8 +102,10 @@ impl OAuthFlow { /// /// Decided by probing, not by reading registry metadata, which is often /// wrong about this. A server that answers without a challenge is open; one - /// that challenges with a usable authorization endpoint wants a browser - /// sign-in; anything else wants a static token. + /// that challenges with an authorization server [`Self::begin`] can drive + /// (authorize, token and dynamic-registration endpoints, and the + /// authorization-code grant) wants a browser sign-in; anything else wants a + /// static token. /// /// A discovery failure reports a static token rather than an error. The /// user can paste one and find out, which beats being blocked by a probe @@ -115,10 +136,7 @@ impl OAuthFlow { Ok(Some(context)) => Ok(context .authorization_server_metadata .iter() - .find(|metadata| { - metadata.authorization_endpoint.is_some() - && supports_authorization_code(metadata) - }) + .find(|metadata| can_drive_sign_in(metadata)) .and_then(|metadata| { metadata.authorization_endpoint.clone().map(|endpoint| { AuthDetection::oauth(endpoint, metadata.grant_types_supported.clone()) @@ -171,12 +189,7 @@ impl OAuthFlow { let metadata = context .authorization_server_metadata .iter() - .find(|metadata| { - metadata.authorization_endpoint.is_some() - && metadata.token_endpoint.is_some() - && metadata.registration_endpoint.is_some() - && supports_authorization_code(metadata) - }) + .find(|metadata| can_drive_sign_in(metadata)) .ok_or_else(|| Error::AuthDiscovery { detail: "no advertised authorization server offers an authorize endpoint, a token \ endpoint, and dynamic client registration together" @@ -195,6 +208,12 @@ impl OAuthFlow { )); }; + if self.public_endpoints_only { + guard_endpoint(&authorization_endpoint, "authorization").await?; + guard_endpoint(&token_endpoint, "token").await?; + guard_endpoint(®istration_endpoint, "registration").await?; + } + let (client_id, client_secret) = self .register_client(®istration_endpoint, redirect_uri) .await?; @@ -273,6 +292,9 @@ impl OAuthFlow { form.push(("client_secret", secret)); } + if self.public_endpoints_only { + guard_endpoint(&pending.token_endpoint, "token").await?; + } let body = post_form(&self.http, &pending.token_endpoint, &form).await?; let tokens = TokenResponse::parse(&body)?; @@ -349,7 +371,7 @@ impl OAuthFlow { return Err(Error::Http { endpoint: crate::redact_endpoint(registration_endpoint), status: status.as_u16(), - body: text, + body: oauth_failure_reason(&text), }); } @@ -374,6 +396,21 @@ impl OAuthFlow { } } +/// Whether this flow can run a sign-in against an authorization server: it +/// advertises an authorize endpoint, a token endpoint and dynamic client +/// registration, and accepts an authorization code. +/// +/// [`OAuthFlow::detect`] and [`OAuthFlow::begin`] both ask this one question, +/// so detection never offers a sign-in that `begin` would then refuse — a +/// server with no registration endpoint (Slack's, for one) reads as wanting a +/// static token, not as a Sign in button that leads nowhere. +fn can_drive_sign_in(metadata: &tinymcp_bus::AuthorizationServerMetadata) -> bool { + metadata.authorization_endpoint.is_some() + && metadata.token_endpoint.is_some() + && metadata.registration_endpoint.is_some() + && supports_authorization_code(metadata) +} + /// Whether an authorization server will accept an authorization code. /// /// An empty list counts as yes. RFC 8414 makes the field optional and says it diff --git a/crates/tinymcp/src/registry/oauth/mod.rs b/crates/tinymcp/src/registry/oauth/mod.rs index b3e5767a..9240b5f1 100644 --- a/crates/tinymcp/src/registry/oauth/mod.rs +++ b/crates/tinymcp/src/registry/oauth/mod.rs @@ -44,6 +44,7 @@ //! header and never show it in a credential list". pub(crate) mod credentials; +pub(crate) mod endpoint_guard; pub(crate) mod flow; pub(crate) mod tokens; pub(crate) mod types; diff --git a/crates/tinymcp/src/registry/oauth/mod_host_store_tests.rs b/crates/tinymcp/src/registry/oauth/mod_host_store_tests.rs index 02dfd35d..86d3b6ee 100644 --- a/crates/tinymcp/src/registry/oauth/mod_host_store_tests.rs +++ b/crates/tinymcp/src/registry/oauth/mod_host_store_tests.rs @@ -72,7 +72,10 @@ async fn a_host_store_backs_a_whole_sign_in() { .await .expect("begin"); let state = authorize_param(&url, "state").unwrap(); - let server_id = flow.complete(&host, &state, "the-code").await.expect("complete"); + let server_id = flow + .complete(&host, &state, "the-code") + .await + .expect("complete"); assert_eq!(server_id, TENANT_SERVER); assert_eq!( @@ -162,8 +165,119 @@ async fn a_host_store_failure_is_reported_as_a_store_error() { #[tokio::test] async fn a_host_store_with_no_remote_endpoint_cannot_be_signed_in_to() { let error = flow() - .begin(&HostSecrets::default(), TENANT_SERVER, "https://host.test/cb") + .begin( + &HostSecrets::default(), + TENANT_SERVER, + "https://host.test/cb", + ) .await .expect_err("no endpoint"); - assert!(matches!(error, Error::MalformedResponse { .. }), "{error:?}"); + assert!( + matches!(error, Error::MalformedResponse { .. }), + "{error:?}" + ); +} + +// --------------------------------------------------------------------------- +// Detection agrees with what `begin` can drive +// --------------------------------------------------------------------------- + +/// Slack's MCP endpoint challenges properly and advertises an authorize +/// endpoint, but no registration endpoint — so there is no client to mint and +/// `begin` refuses. Offering a Sign in button for it is offering a dead end; +/// detection has to answer "paste a token" instead. +#[tokio::test] +async fn detection_offers_a_token_when_there_is_no_dynamic_registration() { + let (endpoint, state) = authority_with_challenge().await; + state.without_registration.store(true, Ordering::SeqCst); + let store = store_with_remote(&endpoint); + let flow = flow(); + + let detection = flow.detect(&store, "srv-1").await.expect("detect"); + + assert_eq!(detection.kind, AuthKind::Token, "{detection:?}"); + assert!( + flow.begin(&store, "srv-1", "http://127.0.0.1:1/cb") + .await + .is_err(), + "begin refuses the same server, so detection must not offer it" + ); +} + +// --------------------------------------------------------------------------- +// A failure body never carries what was submitted +// --------------------------------------------------------------------------- + +/// A refresh whose token endpoint fails with `reply`, echoing the form back. +async fn rejected_refresh(reply: String) -> Error { + let app = Router::new().route( + "/token", + post(move |body: String| { + let reply = reply.replace("{form}", &body); + async move { (AxumStatus::BAD_REQUEST, reply) } + }), + ); + let base = serve(app).await; + let store = store_with_remote("https://example.test/mcp"); + store_expired_bundle(&store, &format!("{base}/token"), Some("r1-secret-refresh")); + refresh_if_expired(&store, &reqwest::Client::new(), "srv-1") + .await + .expect_err("a rejected refresh") +} + +/// Some token endpoints echo the submitted form in their error body — the +/// refresh token, the client secret. That body reaches logs and user +/// interfaces through the error, so only the standard OAuth `error` and +/// `error_description` survive: they are the *why*, and carry no secret. +#[tokio::test] +async fn a_token_endpoint_failure_keeps_the_reason_but_not_an_echo() { + let error = rejected_refresh( + json!({ + "error": "invalid_grant", + "error_description": "refresh token revoked", + "request": "{form}", + }) + .to_string(), + ) + .await; + + let Error::Http { body, .. } = &error else { + panic!("expected an http error, got {error:?}"); + }; + assert!(body.contains("invalid_grant"), "{body}"); + assert!(body.contains("refresh token revoked"), "{body}"); + assert!(!body.contains("r1-secret-refresh"), "{body}"); + assert!(!body.contains("sec-1"), "{body}"); +} + +#[tokio::test] +async fn a_non_json_token_endpoint_failure_is_not_echoed() { + let error = rejected_refresh("you sent: {form}".to_string()).await; + let rendered = error.to_string(); + assert!(!rendered.contains("r1-secret-refresh"), "{rendered}"); + assert!(!rendered.contains("sec-1"), "{rendered}"); +} + +// --------------------------------------------------------------------------- +// Refusing internal endpoints a hostile server advertises +// --------------------------------------------------------------------------- + +/// The authorization server's endpoints come from the server being signed in +/// to, and the flow POSTs to them from the host. A host serving untrusted +/// servers opts into refusing internal targets; the loopback authority here is +/// exactly such a target, so nothing may be registered with it. +#[tokio::test] +async fn a_guarded_flow_refuses_an_internal_authorization_server() { + let (endpoint, state) = authority_with_challenge().await; + let store = store_with_remote(&endpoint); + let flow = flow().require_public_endpoints(); + + let error = flow + .begin(&store, "srv-1", "https://host.test/cb") + .await + .expect_err("a loopback authorization server"); + + assert!(error.to_string().contains("https"), "{error}"); + assert_eq!(state.registrations.load(Ordering::SeqCst), 0); + assert_eq!(flow.pending_count(), 0); } diff --git a/crates/tinymcp/src/registry/oauth/tokens.rs b/crates/tinymcp/src/registry/oauth/tokens.rs index c2baf162..954c7100 100644 --- a/crates/tinymcp/src/registry/oauth/tokens.rs +++ b/crates/tinymcp/src/registry/oauth/tokens.rs @@ -4,9 +4,9 @@ use std::time::{SystemTime, UNIX_EPOCH}; use serde_json::Value; +use super::credentials::OAuthCredentialStore; use super::types::{OAuthBundle, TokenResponse}; use crate::error::{Error, Result}; -use super::credentials::OAuthCredentialStore; /// The reserved credential key holding the refresh bundle. /// @@ -175,12 +175,14 @@ pub(super) async fn post_form( // The body is read before the status is judged, because a token endpoint's // failure body is where it says *why* — `invalid_grant`, `invalid_client`. - // Discarding it would leave a caller with a bare status code. + // Discarding it would leave a caller with a bare status code; keeping it + // whole would keep whatever the server echoed back — see + // `oauth_failure_reason`. if !status.is_success() { return Err(Error::Http { endpoint: crate::redact_endpoint(endpoint), status: status.as_u16(), - body: text, + body: oauth_failure_reason(&text), }); } @@ -188,6 +190,32 @@ pub(super) async fn post_form( .map_err(|error| Error::malformed(format!("token endpoint replied with non-json: {error}"))) } +/// The part of an authorization server's failure body that is safe to keep: +/// the standard OAuth `error` code and `error_description` (RFC 6749 §5.2). +/// +/// Some servers echo the submitted form — a refresh token, a client secret, an +/// authorization code — into their error body, and the body reaches logs and +/// user interfaces through [`Error::Http`]. The two standard members are the +/// *why* and carry no secret, so they are all that is kept. A body that is not +/// a JSON object carries no standard reason and is dropped entirely. +pub(super) fn oauth_failure_reason(body: &str) -> String { + let Ok(Value::Object(fields)) = serde_json::from_str::(body) else { + return String::new(); + }; + let reason: serde_json::Map = ["error", "error_description"] + .into_iter() + .filter_map(|key| { + let value = fields.get(key)?.as_str()?; + Some((key.to_string(), Value::String(value.to_string()))) + }) + .collect(); + if reason.is_empty() { + String::new() + } else { + Value::Object(reason).to_string() + } +} + /// The current time in Unix seconds. /// /// A clock before the epoch reads as zero, which makes every token look expired From ba8dea22cac8e172a1dd52088770df8771f67bec Mon Sep 17 00:00:00 2001 From: Steven Enamakel Date: Sun, 4 Oct 2026 02:15:04 +0530 Subject: [PATCH 4/7] fix(oauth): use dedicated error code for credential store failures The `CredentialStore` error variant was previously mapped to the generic `STORE` error code, making it indistinguishable from other store errors on the wire. This change introduces a new `CREDENTIAL_STORE` error constant and maps the variant to it, so that a host's own secret store failures are reported with their own error code. The documentation comments are also updated to refer to `SQLite` as code rather than plain text. Auto-committed-on: macbook Co-authored-by: Medulla --- crates/tinymcp-bus/src/errors/mod.rs | 3 +++ crates/tinymcp/src/error/mod.rs | 2 +- crates/tinymcp/src/registry/oauth/credentials.rs | 2 +- crates/tinymcp/src/registry/oauth/flow.rs | 2 +- crates/tinymcp/src/registry/oauth/mod.rs | 2 +- crates/tinymcp/src/registry/oauth/mod_host_store_tests.rs | 4 ++-- 6 files changed, 9 insertions(+), 6 deletions(-) diff --git a/crates/tinymcp-bus/src/errors/mod.rs b/crates/tinymcp-bus/src/errors/mod.rs index 5ab3781d..e9341e93 100644 --- a/crates/tinymcp-bus/src/errors/mod.rs +++ b/crates/tinymcp-bus/src/errors/mod.rs @@ -67,6 +67,8 @@ pub const CLIENT_BUILD: &str = "ai.tinyhumans.tinymcp.Error.ClientBuild"; pub const SERIALIZATION: &str = "ai.tinyhumans.tinymcp.Error.Serialization"; /// The installed-server store could not do what was asked of it. pub const STORE: &str = "ai.tinyhumans.tinymcp.Error.Store"; +/// A host-supplied credential store could not do what was asked of it. +pub const CREDENTIAL_STORE: &str = "ai.tinyhumans.tinymcp.Error.CredentialStore"; /// The store's directory or file could not be reached. pub const STORE_IO: &str = "ai.tinyhumans.tinymcp.Error.StoreIo"; /// The module could not do something it needed of the bus, such as serving a @@ -103,6 +105,7 @@ pub const ALL: &[&str] = &[ CLIENT_BUILD, SERIALIZATION, STORE, + CREDENTIAL_STORE, STORE_IO, BUS, INVALID_ARGUMENT, diff --git a/crates/tinymcp/src/error/mod.rs b/crates/tinymcp/src/error/mod.rs index d66207e0..24acbb16 100644 --- a/crates/tinymcp/src/error/mod.rs +++ b/crates/tinymcp/src/error/mod.rs @@ -406,7 +406,7 @@ impl Error { Self::ClientBuild { .. } => errors::CLIENT_BUILD, Self::Serialization { .. } => errors::SERIALIZATION, Self::Store { .. } => errors::STORE, - Self::CredentialStore { .. } => errors::STORE, + Self::CredentialStore { .. } => errors::CREDENTIAL_STORE, Self::StoreIo { .. } => errors::STORE_IO, Self::Bus { .. } => errors::BUS, Self::InvalidArgument { .. } => errors::INVALID_ARGUMENT, diff --git a/crates/tinymcp/src/registry/oauth/credentials.rs b/crates/tinymcp/src/registry/oauth/credentials.rs index 6ae20045..1590971f 100644 --- a/crates/tinymcp/src/registry/oauth/credentials.rs +++ b/crates/tinymcp/src/registry/oauth/credentials.rs @@ -5,7 +5,7 @@ //! `Authorization` value and the refresh bundle under [`OAUTH_BUNDLE_KEY`]. //! [`OAuthCredentialStore`] is exactly that, so a host can back sign-in and //! refresh with its own secret store (a per-tenant vault, say) instead of the -//! SQLite [`Store`]. [`Store`] implements it, so existing callers are +//! `SQLite` [`Store`]. [`Store`] implements it, so existing callers are //! unchanged. //! //! [`OAUTH_BUNDLE_KEY`]: super::OAUTH_BUNDLE_KEY diff --git a/crates/tinymcp/src/registry/oauth/flow.rs b/crates/tinymcp/src/registry/oauth/flow.rs index de2af199..d523fc3d 100644 --- a/crates/tinymcp/src/registry/oauth/flow.rs +++ b/crates/tinymcp/src/registry/oauth/flow.rs @@ -80,7 +80,7 @@ impl OAuthFlow { /// `https` or that resolve to an internal address. /// /// Those endpoints are advertised by the server being signed in to and - /// POSTed to from the host, so a host connecting to servers it does not + /// `POST`ed to from the host, so a host connecting to servers it does not /// 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 diff --git a/crates/tinymcp/src/registry/oauth/mod.rs b/crates/tinymcp/src/registry/oauth/mod.rs index 9240b5f1..54e10038 100644 --- a/crates/tinymcp/src/registry/oauth/mod.rs +++ b/crates/tinymcp/src/registry/oauth/mod.rs @@ -33,7 +33,7 @@ //! //! # What is stored, and where //! -//! Storage is an [`OAuthCredentialStore`]: the SQLite [`Store`] by default, +//! Storage is an [`OAuthCredentialStore`]: the `SQLite` [`Store`] by default, //! or a host's own secret store. //! //! The access token is stored as the server's `Authorization` header value, so diff --git a/crates/tinymcp/src/registry/oauth/mod_host_store_tests.rs b/crates/tinymcp/src/registry/oauth/mod_host_store_tests.rs index 86d3b6ee..1324bd8b 100644 --- a/crates/tinymcp/src/registry/oauth/mod_host_store_tests.rs +++ b/crates/tinymcp/src/registry/oauth/mod_host_store_tests.rs @@ -1,4 +1,4 @@ -//! The flow backed by a host's own secret store rather than the SQLite +//! The flow backed by a host's own secret store rather than the `SQLite` //! [`Store`]: what a multi-tenant host needs to keep PKCE, registration and //! refresh state in its own per-tenant secrets. @@ -158,7 +158,7 @@ async fn a_host_store_failure_is_reported_as_a_store_error() { .expect_err("a sealed vault"); assert!(matches!(error, Error::CredentialStore { .. }), "{error:?}"); - assert_eq!(error.wire_name(), tinymcp_bus::errors::STORE); + assert_eq!(error.wire_name(), tinymcp_bus::errors::CREDENTIAL_STORE); assert!(error.to_string().contains("sealed"), "{error}"); } From 8e2702f0eb823be126c8033abc2119b47781a3df Mon Sep 17 00:00:00 2001 From: Steven Enamakel Date: Sun, 4 Oct 2026 07:44:29 +0300 Subject: [PATCH 5/7] fix(oauth): pin guarded endpoint connections Co-authored-by: Medulla --- .../tinymcp/src/registry/oauth/credentials.rs | 1 + .../src/registry/oauth/endpoint_guard.rs | 53 +++++++++++++++---- .../registry/oauth/endpoint_guard_tests.rs | 12 ++++- crates/tinymcp/src/registry/oauth/flow.rs | 48 ++++++++++++++--- .../registry/oauth/mod_host_store_tests.rs | 1 + 5 files changed, 96 insertions(+), 19 deletions(-) diff --git a/crates/tinymcp/src/registry/oauth/credentials.rs b/crates/tinymcp/src/registry/oauth/credentials.rs index 1590971f..cb31c653 100644 --- a/crates/tinymcp/src/registry/oauth/credentials.rs +++ b/crates/tinymcp/src/registry/oauth/credentials.rs @@ -63,6 +63,7 @@ pub trait OAuthCredentialStore: Send + Sync { ) -> impl Future> + Send; } +#[allow(clippy::unused_async_trait_impl)] impl OAuthCredentialStore for Store { async fn remote_url(&self, server_id: &str) -> Result> { let server = self.get_server(server_id)?; diff --git a/crates/tinymcp/src/registry/oauth/endpoint_guard.rs b/crates/tinymcp/src/registry/oauth/endpoint_guard.rs index 92154009..7b82d359 100644 --- a/crates/tinymcp/src/registry/oauth/endpoint_guard.rs +++ b/crates/tinymcp/src/registry/oauth/endpoint_guard.rs @@ -9,7 +9,8 @@ //! It is off by default because a desktop host legitimately signs in to //! loopback development servers. -use std::net::{IpAddr, ToSocketAddrs}; +use std::net::{IpAddr, SocketAddr, ToSocketAddrs}; +use std::time::Duration; use reqwest::Url; @@ -18,14 +19,15 @@ use crate::error::{Error, Result}; /// Refuses `raw` unless it is `https` and every address its host resolves to /// is public. `what` names the endpoint in the error. /// -/// A single blocked record is enough to refuse, which also blunts DNS -/// rebinding to an internal address. Resolution runs on the blocking pool so a -/// slow resolver cannot stall the executor. +/// A single blocked record is enough to refuse. The returned addresses can be +/// pinned to the HTTP client so the eventual connection uses the vetted result +/// instead of resolving the name again. Resolution runs on the blocking pool +/// so a slow resolver cannot stall the executor. /// /// # Errors /// /// [`Error::MalformedResponse`] naming the endpoint and why it was refused. -pub(super) async fn guard_endpoint(raw: &str, what: &str) -> Result<()> { +pub(super) async fn guard_endpoint(raw: &str, what: &str) -> Result> { let refuse = |why: String| Error::malformed(format!("{what} endpoint refused: {why}")); let url = Url::parse(raw).map_err(|error| refuse(format!("not a valid url: {error}")))?; if url.scheme() != "https" { @@ -39,13 +41,13 @@ pub(super) async fn guard_endpoint(raw: &str, what: &str) -> Result<()> { .to_string(); let port = url.port_or_known_default().unwrap_or(443); - let addresses: Vec = if let Ok(ip) = host.parse::() { - vec![ip] + let addresses: Vec = if let Ok(ip) = host.parse::() { + vec![SocketAddr::new(ip, port)] } else { tokio::task::spawn_blocking(move || { (host.as_str(), port) .to_socket_addrs() - .map(|found| found.map(|addr| addr.ip()).collect()) + .map(Iterator::collect::>) }) .await .map_err(|error| refuse(format!("resolution did not finish: {error}")))? @@ -54,10 +56,34 @@ pub(super) async fn guard_endpoint(raw: &str, what: &str) -> Result<()> { if addresses.is_empty() { return Err(refuse("its host does not resolve".to_string())); } - if addresses.iter().any(is_blocked_ip) { + if addresses.iter().any(|addr| is_blocked_ip(&addr.ip())) { return Err(refuse("it resolves to a disallowed address".to_string())); } - Ok(()) + Ok(addresses) +} + +/// Builds an HTTP client pinned to the public addresses checked for `raw`. +/// Redirects are disabled so a validated endpoint cannot replay credentials to +/// a different, unchecked destination. +pub(super) async fn guarded_client(raw: &str, what: &str) -> Result { + let addresses = guard_endpoint(raw, what).await?; + let url = Url::parse(raw) + .map_err(|error| Error::malformed(format!("invalid {what} url: {error}")))?; + let host = url + .host_str() + .ok_or_else(|| Error::malformed(format!("{what} endpoint has no host")))? + .trim_start_matches('[') + .trim_end_matches(']'); + let mut builder = reqwest::Client::builder() + .timeout(Duration::from_secs(30)) + .connect_timeout(Duration::from_secs(10)) + .redirect(reqwest::redirect::Policy::none()); + if host.parse::().is_err() { + builder = builder.resolve_to_addrs(host, &addresses); + } + builder.build().map_err(|source| Error::ClientBuild { + source: Box::new(source.without_url()), + }) } /// Whether an address is one the flow must never POST OAuth material to: @@ -74,9 +100,12 @@ pub(super) fn is_blocked_ip(ip: &IpAddr) -> bool { || v4.is_documentation() || v4.is_multicast() || v4.octets()[0] == 0 + || (v4.octets()[0] == 100 && (v4.octets()[1] & 0xc0) == 64) + || (v4.octets()[0] == 198 && (v4.octets()[1] & 0xfe) == 18) + || v4.octets()[0] >= 240 } IpAddr::V6(v6) => { - if let Some(v4) = v6.to_ipv4_mapped() { + if let Some(v4) = v6.to_ipv4() { return is_blocked_ip(&IpAddr::V4(v4)); } v6.is_loopback() @@ -84,6 +113,8 @@ pub(super) fn is_blocked_ip(ip: &IpAddr) -> bool { || v6.is_multicast() || (v6.segments()[0] & 0xfe00) == 0xfc00 || (v6.segments()[0] & 0xffc0) == 0xfe80 + || (v6.segments()[0] == 0x0064 && v6.segments()[1] == 0xff9b) + || (v6.segments()[0] & 0xffc0) == 0xfec0 } } } diff --git a/crates/tinymcp/src/registry/oauth/endpoint_guard_tests.rs b/crates/tinymcp/src/registry/oauth/endpoint_guard_tests.rs index 41cfae82..dd514536 100644 --- a/crates/tinymcp/src/registry/oauth/endpoint_guard_tests.rs +++ b/crates/tinymcp/src/registry/oauth/endpoint_guard_tests.rs @@ -20,6 +20,11 @@ fn internal_and_special_addresses_are_blocked() { "169.254.169.254", "0.0.0.0", "0.1.2.3", + "100.64.0.1", + "100.127.255.254", + "198.18.0.1", + "198.19.255.254", + "240.0.0.1", "255.255.255.255", "192.0.2.1", "224.0.0.1", @@ -28,6 +33,9 @@ fn internal_and_special_addresses_are_blocked() { "fd00::1", "fe80::1", "ff02::1", + "64:ff9b::7f00:1", + "fec0::1", + "::127.0.0.1", "::ffff:127.0.0.1", "::ffff:169.254.169.254", ] { @@ -42,6 +50,7 @@ fn public_addresses_are_allowed() { "1.1.1.1", "2606:4700:4700::1111", "::ffff:8.8.8.8", + "2001:4860:4860::8888", ] { assert!(!is_blocked_ip(&ip(raw)), "{raw} should be allowed"); } @@ -49,9 +58,10 @@ fn public_addresses_are_allowed() { #[tokio::test] async fn a_public_https_literal_passes() { - guard_endpoint("https://8.8.8.8/token", "token") + let addresses = guard_endpoint("https://8.8.8.8/token", "token") .await .expect("public https"); + assert_eq!(addresses[0].ip(), ip("8.8.8.8")); } #[tokio::test] diff --git a/crates/tinymcp/src/registry/oauth/flow.rs b/crates/tinymcp/src/registry/oauth/flow.rs index d523fc3d..6d7b2496 100644 --- a/crates/tinymcp/src/registry/oauth/flow.rs +++ b/crates/tinymcp/src/registry/oauth/flow.rs @@ -10,7 +10,7 @@ use serde_json::{Value, json}; use sha2::{Digest, Sha256}; use super::credentials::OAuthCredentialStore; -use super::endpoint_guard::guard_endpoint; +use super::endpoint_guard::{guard_endpoint, guarded_client}; use super::tokens::{oauth_failure_reason, persist, post_form}; use super::types::{AuthDetection, PendingAuthorization, TokenResponse}; use crate::error::{Error, Result}; @@ -49,6 +49,7 @@ const CLIENT_NAME: &str = "TinyMCP"; #[derive(Debug)] pub struct OAuthFlow { http: reqwest::Client, + no_redirect_http: reqwest::Client, proxy: Option, pending: Mutex>, public_endpoints_only: bool, @@ -67,9 +68,17 @@ impl OAuthFlow { .map_err(|source| Error::ClientBuild { source: Box::new(source.without_url()), })?; + let no_redirect_http = reqwest::Client::builder() + .timeout(Duration::from_secs(AUTH_SERVER_TIMEOUT_SECS)) + .redirect(reqwest::redirect::Policy::none()) + .build() + .map_err(|source| Error::ClientBuild { + source: Box::new(source.without_url()), + })?; Ok(Self { http, + no_redirect_http, proxy, pending: Mutex::new(HashMap::new()), public_endpoints_only: false, @@ -95,7 +104,11 @@ impl OAuthFlow { /// The HTTP client this flow uses, for the refresh path. #[must_use] pub fn http(&self) -> &reqwest::Client { - &self.http + if self.public_endpoints_only { + &self.no_redirect_http + } else { + &self.http + } } /// Classifies what a server wants before it will talk. @@ -211,11 +224,19 @@ impl OAuthFlow { if self.public_endpoints_only { guard_endpoint(&authorization_endpoint, "authorization").await?; guard_endpoint(&token_endpoint, "token").await?; - guard_endpoint(®istration_endpoint, "registration").await?; } + let registration_http = if self.public_endpoints_only { + Some(guarded_client(®istration_endpoint, "registration").await?) + } else { + None + }; let (client_id, client_secret) = self - .register_client(®istration_endpoint, redirect_uri) + .register_client( + registration_http.as_ref().unwrap_or(&self.http), + ®istration_endpoint, + redirect_uri, + ) .await?; let (code_verifier, code_challenge) = generate_pkce(); @@ -293,7 +314,20 @@ impl OAuthFlow { } if self.public_endpoints_only { - guard_endpoint(&pending.token_endpoint, "token").await?; + let http = guarded_client(&pending.token_endpoint, "token").await?; + let body = post_form(&http, &pending.token_endpoint, &form).await?; + let tokens = TokenResponse::parse(&body)?; + persist( + store, + &pending.server_id, + &pending.client_id, + pending.client_secret.as_deref(), + &pending.token_endpoint, + &tokens, + ) + .await?; + tracing::info!(server_id = %pending.server_id, "completed an oauth authorization and stored the token"); + return Ok(pending.server_id); } let body = post_form(&self.http, &pending.token_endpoint, &form).await?; let tokens = TokenResponse::parse(&body)?; @@ -342,6 +376,7 @@ impl OAuthFlow { /// token exchange then fails without it. async fn register_client( &self, + http: &reqwest::Client, registration_endpoint: &str, redirect_uri: &str, ) -> Result<(String, Option)> { @@ -353,8 +388,7 @@ impl OAuthFlow { "token_endpoint_auth_method": "client_secret_post", }); - let response = self - .http + let response = http .post(registration_endpoint) .json(&body) .send() diff --git a/crates/tinymcp/src/registry/oauth/mod_host_store_tests.rs b/crates/tinymcp/src/registry/oauth/mod_host_store_tests.rs index 1324bd8b..a09c543b 100644 --- a/crates/tinymcp/src/registry/oauth/mod_host_store_tests.rs +++ b/crates/tinymcp/src/registry/oauth/mod_host_store_tests.rs @@ -27,6 +27,7 @@ impl HostSecrets { } } +#[allow(clippy::unused_async_trait_impl)] impl OAuthCredentialStore for HostSecrets { async fn remote_url(&self, _server_id: &str) -> crate::Result> { Ok(self.url.clone()) From 52ecae6edbe114aa57b488b1dac8e710642a9bef Mon Sep 17 00:00:00 2001 From: Steven Enamakel Date: Sun, 4 Oct 2026 07:53:52 +0300 Subject: [PATCH 6/7] test(oauth): cover guarded client behavior Co-authored-by: Medulla --- .../registry/oauth/endpoint_guard_tests.rs | 9 +++++- crates/tinymcp/src/registry/oauth/mod.rs | 3 +- .../registry/oauth/mod_host_store_tests.rs | 29 +++++++++++++++++++ 3 files changed, 39 insertions(+), 2 deletions(-) diff --git a/crates/tinymcp/src/registry/oauth/endpoint_guard_tests.rs b/crates/tinymcp/src/registry/oauth/endpoint_guard_tests.rs index dd514536..10027805 100644 --- a/crates/tinymcp/src/registry/oauth/endpoint_guard_tests.rs +++ b/crates/tinymcp/src/registry/oauth/endpoint_guard_tests.rs @@ -4,7 +4,7 @@ use std::net::IpAddr; -use super::{guard_endpoint, is_blocked_ip}; +use super::{guard_endpoint, guarded_client, is_blocked_ip}; fn ip(raw: &str) -> IpAddr { raw.parse().unwrap() @@ -64,6 +64,13 @@ async fn a_public_https_literal_passes() { assert_eq!(addresses[0].ip(), ip("8.8.8.8")); } +#[tokio::test] +async fn a_public_https_literal_builds_a_pinned_client() { + guarded_client("https://8.8.8.8/token", "token") + .await + .expect("pinned public https client"); +} + #[tokio::test] async fn plain_http_is_refused_even_to_a_public_host() { let error = guard_endpoint("http://8.8.8.8/token", "token") diff --git a/crates/tinymcp/src/registry/oauth/mod.rs b/crates/tinymcp/src/registry/oauth/mod.rs index 54e10038..49649612 100644 --- a/crates/tinymcp/src/registry/oauth/mod.rs +++ b/crates/tinymcp/src/registry/oauth/mod.rs @@ -33,7 +33,8 @@ //! //! # What is stored, and where //! -//! Storage is an [`OAuthCredentialStore`]: the `SQLite` [`Store`] by default, +//! Storage is an [`OAuthCredentialStore`]: the `SQLite` +//! [`crate::registry::Store`] by default, //! or a host's own secret store. //! //! The access token is stored as the server's `Authorization` header value, so diff --git a/crates/tinymcp/src/registry/oauth/mod_host_store_tests.rs b/crates/tinymcp/src/registry/oauth/mod_host_store_tests.rs index a09c543b..2c2e7d66 100644 --- a/crates/tinymcp/src/registry/oauth/mod_host_store_tests.rs +++ b/crates/tinymcp/src/registry/oauth/mod_host_store_tests.rs @@ -4,6 +4,8 @@ use super::*; use crate::registry::OAuthCredentialStore; +use std::io::{Read, Write}; +use std::net::TcpListener; /// A host's secret store: credentials in a map, keyed by whatever server id /// the host chooses — here a tenant-qualified one. @@ -282,3 +284,30 @@ async fn a_guarded_flow_refuses_an_internal_authorization_server() { assert_eq!(state.registrations.load(Ordering::SeqCst), 0); assert_eq!(flow.pending_count(), 0); } + +#[tokio::test] +async fn a_guarded_flow_refresh_client_does_not_follow_redirects() { + let listener = TcpListener::bind("127.0.0.1:0").expect("bind listener"); + let endpoint = format!("http://{}/refresh", listener.local_addr().expect("address")); + let server = std::thread::spawn(move || { + let (mut socket, _) = listener.accept().expect("accept request"); + let mut request = [0; 1024]; + let _ = socket.read(&mut request).expect("read request"); + socket + .write_all( + b"HTTP/1.1 302 Found\r\nLocation: /redirected\r\nContent-Length: 0\r\nConnection: close\r\n\r\n", + ) + .expect("write redirect"); + }); + + let response = flow() + .require_public_endpoints() + .http() + .get(endpoint) + .send() + .await + .expect("receive redirect response"); + + assert_eq!(response.status(), reqwest::StatusCode::FOUND); + server.join().expect("server thread"); +} From 672d4329207314b3a15b2d6e51cefdb59583d0c2 Mon Sep 17 00:00:00 2001 From: Steven Enamakel Date: Sun, 4 Oct 2026 08:03:42 +0300 Subject: [PATCH 7/7] test(oauth): cover guarded endpoint paths Co-authored-by: Medulla --- .../src/registry/oauth/endpoint_guard.rs | 6 +- .../registry/oauth/endpoint_guard_tests.rs | 8 ++- crates/tinymcp/src/registry/oauth/flow.rs | 66 +++++++++++-------- .../registry/oauth/mod_host_store_tests.rs | 37 +++++++++++ 4 files changed, 87 insertions(+), 30 deletions(-) diff --git a/crates/tinymcp/src/registry/oauth/endpoint_guard.rs b/crates/tinymcp/src/registry/oauth/endpoint_guard.rs index 7b82d359..2880611c 100644 --- a/crates/tinymcp/src/registry/oauth/endpoint_guard.rs +++ b/crates/tinymcp/src/registry/oauth/endpoint_guard.rs @@ -74,12 +74,16 @@ pub(super) async fn guarded_client(raw: &str, what: &str) -> Result Result { let mut builder = reqwest::Client::builder() .timeout(Duration::from_secs(30)) .connect_timeout(Duration::from_secs(10)) .redirect(reqwest::redirect::Policy::none()); if host.parse::().is_err() { - builder = builder.resolve_to_addrs(host, &addresses); + builder = builder.resolve_to_addrs(host, addresses); } builder.build().map_err(|source| Error::ClientBuild { source: Box::new(source.without_url()), diff --git a/crates/tinymcp/src/registry/oauth/endpoint_guard_tests.rs b/crates/tinymcp/src/registry/oauth/endpoint_guard_tests.rs index 10027805..c8646c6e 100644 --- a/crates/tinymcp/src/registry/oauth/endpoint_guard_tests.rs +++ b/crates/tinymcp/src/registry/oauth/endpoint_guard_tests.rs @@ -4,7 +4,7 @@ use std::net::IpAddr; -use super::{guard_endpoint, guarded_client, is_blocked_ip}; +use super::{client_pinned_to, guard_endpoint, guarded_client, is_blocked_ip}; fn ip(raw: &str) -> IpAddr { raw.parse().unwrap() @@ -71,6 +71,12 @@ async fn a_public_https_literal_builds_a_pinned_client() { .expect("pinned public https client"); } +#[test] +fn a_hostname_client_pins_the_addresses_that_were_checked() { + let addresses = ["8.8.8.8:443".parse().expect("socket address")]; + client_pinned_to("public.example", &addresses).expect("pinned hostname client"); +} + #[tokio::test] async fn plain_http_is_refused_even_to_a_public_host() { let error = guard_endpoint("http://8.8.8.8/token", "token") diff --git a/crates/tinymcp/src/registry/oauth/flow.rs b/crates/tinymcp/src/registry/oauth/flow.rs index 6d7b2496..f33d43dd 100644 --- a/crates/tinymcp/src/registry/oauth/flow.rs +++ b/crates/tinymcp/src/registry/oauth/flow.rs @@ -221,16 +221,13 @@ impl OAuthFlow { )); }; - if self.public_endpoints_only { - guard_endpoint(&authorization_endpoint, "authorization").await?; - guard_endpoint(&token_endpoint, "token").await?; - } - - let registration_http = if self.public_endpoints_only { - Some(guarded_client(®istration_endpoint, "registration").await?) - } else { - None - }; + let registration_http = self + .registration_client( + &authorization_endpoint, + &token_endpoint, + ®istration_endpoint, + ) + .await?; let (client_id, client_secret) = self .register_client( registration_http.as_ref().unwrap_or(&self.http), @@ -313,25 +310,10 @@ impl OAuthFlow { form.push(("client_secret", secret)); } - if self.public_endpoints_only { - let http = guarded_client(&pending.token_endpoint, "token").await?; - let body = post_form(&http, &pending.token_endpoint, &form).await?; - let tokens = TokenResponse::parse(&body)?; - persist( - store, - &pending.server_id, - &pending.client_id, - pending.client_secret.as_deref(), - &pending.token_endpoint, - &tokens, - ) - .await?; - tracing::info!(server_id = %pending.server_id, "completed an oauth authorization and stored the token"); - return Ok(pending.server_id); - } - let body = post_form(&self.http, &pending.token_endpoint, &form).await?; + let guarded_http = self.token_client(&pending.token_endpoint).await?; + let http = guarded_http.as_ref().unwrap_or(&self.http); + let body = post_form(http, &pending.token_endpoint, &form).await?; let tokens = TokenResponse::parse(&body)?; - persist( store, &pending.server_id, @@ -369,6 +351,34 @@ impl OAuthFlow { pending.len() } + pub(super) async fn registration_client( + &self, + authorization_endpoint: &str, + token_endpoint: &str, + registration_endpoint: &str, + ) -> Result> { + if self.public_endpoints_only { + guard_endpoint(authorization_endpoint, "authorization").await?; + guard_endpoint(token_endpoint, "token").await?; + Ok(Some( + guarded_client(registration_endpoint, "registration").await?, + )) + } else { + Ok(None) + } + } + + pub(super) async fn token_client( + &self, + token_endpoint: &str, + ) -> Result> { + if self.public_endpoints_only { + Ok(Some(guarded_client(token_endpoint, "token").await?)) + } else { + Ok(None) + } + } + /// Registers a client dynamically, per RFC 7591. /// /// A confidential client is requested, and the secret is kept when one is diff --git a/crates/tinymcp/src/registry/oauth/mod_host_store_tests.rs b/crates/tinymcp/src/registry/oauth/mod_host_store_tests.rs index 2c2e7d66..fcdcac45 100644 --- a/crates/tinymcp/src/registry/oauth/mod_host_store_tests.rs +++ b/crates/tinymcp/src/registry/oauth/mod_host_store_tests.rs @@ -311,3 +311,40 @@ async fn a_guarded_flow_refresh_client_does_not_follow_redirects() { assert_eq!(response.status(), reqwest::StatusCode::FOUND); server.join().expect("server thread"); } + +#[tokio::test] +async fn public_endpoint_clients_validate_and_pin_registration_and_token_hosts() { + let flow = flow().require_public_endpoints(); + let registration = flow + .registration_client( + "https://8.8.8.8/authorize", + "https://1.1.1.1/token", + "https://9.9.9.9/register", + ) + .await + .expect("validate public endpoints"); + assert!(registration.is_some()); + assert!( + flow.token_client("https://8.8.8.8/token") + .await + .expect("build token client") + .is_some() + ); +} + +#[tokio::test] +async fn desktop_flow_keeps_the_default_clients() { + let flow = flow(); + assert!( + flow.registration_client("not a url", "not a url", "not a url") + .await + .expect("desktop flow skips public endpoint checks") + .is_none() + ); + assert!( + flow.token_client("not a url") + .await + .expect("desktop flow skips public endpoint checks") + .is_none() + ); +}