diff --git a/crates/tinymcp/src/lib.rs b/crates/tinymcp/src/lib.rs index 0a1c6132..9a85d053 100644 --- a/crates/tinymcp/src/lib.rs +++ b/crates/tinymcp/src/lib.rs @@ -95,7 +95,8 @@ pub use config_servers::{ }; pub use error::{Error, Result}; pub use registry::{ - AppliedServer, ConfigApplyReport, Connections, McpRegistry, OAuthFlow, ProbeOutcome, + AppliedServer, ConfigApplyReport, Connections, McpRegistry, MemoryPendingStore, OAuthFlow, + PENDING_TTL_SECS, PendingAuthorization, PendingAuthorizationStore, ProbeOutcome, REMOTE_REQUEST_TIMEOUT, SecretRef, SecretVault, ServerRef, Store, SupervisedHost, Supervisor, SupervisorConfig, SupervisorEvent, TickReport, }; diff --git a/crates/tinymcp/src/registry/mod.rs b/crates/tinymcp/src/registry/mod.rs index e13f3ca6..bc61ec73 100644 --- a/crates/tinymcp/src/registry/mod.rs +++ b/crates/tinymcp/src/registry/mod.rs @@ -24,7 +24,9 @@ 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, DEFAULT_CLIENT_NAME, OAuthBundle, OAuthCredentialStore, OAuthFlow, + AuthDetection, AuthKind, DEFAULT_CLIENT_NAME, MemoryPendingStore, OAuthBundle, + OAuthCredentialStore, OAuthFlow, PENDING_TTL_SECS, PendingAuthorization, + PendingAuthorizationStore, }; pub use ops::McpRegistry; pub use setup::{SecretRef, SecretVault}; diff --git a/crates/tinymcp/src/registry/oauth/flow.rs b/crates/tinymcp/src/registry/oauth/flow.rs index 4747eab7..69eab413 100644 --- a/crates/tinymcp/src/registry/oauth/flow.rs +++ b/crates/tinymcp/src/registry/oauth/flow.rs @@ -1,18 +1,18 @@ //! Running the authorization-code flow. -use std::collections::HashMap; +use std::sync::Arc; use std::time::Duration; use base64::Engine as _; -use parking_lot::Mutex; use reqwest::Url; use serde_json::{Value, json}; use sha2::{Digest, Sha256}; use super::credentials::OAuthCredentialStore; use super::endpoint_guard::{guard_endpoint, guarded_client}; +use super::pending::{MemoryPendingStore, PendingAuthorization, PendingAuthorizationStore}; use super::tokens::{due_refresh, exchange_refresh, oauth_failure_reason, persist, post_form}; -use super::types::{AuthDetection, PendingAuthorization, TokenResponse}; +use super::types::{AuthDetection, TokenResponse}; use crate::error::{Error, Result}; use crate::transport::http::McpHttpClient; use tinymcp_bus::McpProxyConfig; @@ -21,13 +21,6 @@ use tinymcp_bus::McpProxyConfig; const B64: base64::engine::general_purpose::GeneralPurpose = base64::engine::general_purpose::URL_SAFE_NO_PAD; -/// How long an unfinished authorization stays parked. -/// -/// A user who starts a sign-in and closes the tab leaves an entry behind -/// holding a client secret. Ten minutes is longer than any real sign-in and -/// short enough that abandoned ones do not accumulate. -const PENDING_TTL: Duration = Duration::from_secs(10 * 60); - /// How long to wait on an authorization server. const AUTH_SERVER_TIMEOUT_SECS: u64 = 20; @@ -43,15 +36,16 @@ pub const DEFAULT_CLIENT_NAME: &str = "TinyMCP"; /// Runs browser sign-in for HTTP-remote servers. /// -/// Holds the authorizations parked between the redirect out and back. One -/// instance per host; two would not see each other's pending state, and a -/// redirect would come back to the wrong one. +/// Parks authorizations between the redirect out and back in a +/// [`PendingAuthorizationStore`]: this process's memory by default, so one +/// instance per host. Instances sharing a store with +/// [`Self::with_pending_store`] may each complete what another began. #[derive(Debug)] pub struct OAuthFlow { http: reqwest::Client, no_redirect_http: reqwest::Client, proxy: Option, - pending: Mutex>, + pending: Arc, public_endpoints_only: bool, client_name: String, } @@ -81,7 +75,7 @@ impl OAuthFlow { http, no_redirect_http, proxy, - pending: Mutex::new(HashMap::new()), + pending: Arc::new(MemoryPendingStore::new()), public_endpoints_only: false, client_name: DEFAULT_CLIENT_NAME.to_string(), }) @@ -103,6 +97,17 @@ impl OAuthFlow { self } + /// Parks authorizations in `store` instead of this process's memory. + /// + /// For a host running several instances behind one callback: with a store + /// they all share, the instance the redirect lands on completes a sign-in + /// another began. See [`PendingAuthorizationStore`]. + #[must_use] + pub fn with_pending_store(mut self, store: Arc) -> Self { + self.pending = store; + self + } + /// The client name sent during dynamic registration. #[must_use] pub fn client_name(&self) -> &str { @@ -270,11 +275,9 @@ impl OAuthFlow { let (code_verifier, code_challenge) = generate_pkce(); let state = uuid::Uuid::new_v4().to_string(); - { - let mut pending = self.pending.lock(); - prune_expired(&mut pending); - pending.insert( - state.clone(), + self.pending + .park( + &state, PendingAuthorization { server_id: server_id.to_string(), code_verifier, @@ -284,8 +287,8 @@ impl OAuthFlow { redirect_uri: redirect_uri.to_string(), started_at: super::tokens::now_unix(), }, - ); - } + ) + .await?; let authorize_url = Url::parse_with_params( &authorization_endpoint, @@ -313,22 +316,26 @@ impl OAuthFlow { /// /// The pending entry is consumed whether or not the exchange succeeds: a /// code is single-use, so a retry with the same state could never work and - /// keeping the entry would only hold a secret in memory. + /// keeping the entry would only hold a secret in the store. Taking it is + /// atomic in the store, so two completions of one state exchange the code + /// at most once. /// /// # Errors /// /// Returns [`Error::MalformedResponse`] when the state is unknown or - /// expired, plus whatever the token endpoint returns. + /// expired, whatever the pending store reports when it cannot be read, + /// plus whatever the token endpoint returns. 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); - pending.remove(state) - } - .ok_or_else(|| Error::malformed("unknown or expired oauth state"))?; + let now = super::tokens::now_unix(); + let pending = self + .pending + .take(state) + .await? + .filter(|pending| !pending.expired_at(now)) + .ok_or_else(|| Error::malformed("unknown or expired oauth state"))?; let mut form: Vec<(&str, &str)> = vec![ ("grant_type", "authorization_code"), @@ -394,19 +401,27 @@ impl OAuthFlow { /// 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()) + /// + /// # Errors + /// + /// Whatever the pending store reports when it cannot be read. + pub async fn pending_server(&self, state: &str) -> Result> { + let now = super::tokens::now_unix(); + Ok(self + .pending + .peek(state) + .await? + .filter(|parked| !parked.expired_at(now)) + .map(|parked| parked.server_id)) } /// How many authorizations are parked. For tests and diagnostics. - #[must_use] - pub fn pending_count(&self) -> usize { - let mut pending = self.pending.lock(); - prune_expired(&mut pending); - pending.len() + /// + /// # Errors + /// + /// Whatever the pending store reports when it cannot be read. + pub async fn pending_count(&self) -> Result { + self.pending.count().await } pub(super) async fn registration_client( @@ -546,11 +561,6 @@ fn generate_pkce() -> (String, String) { /// /// Each holds a client secret, so leaving them is a slow leak of credentials /// into a map nothing ever reads again. -fn prune_expired(pending: &mut HashMap) { - let cutoff = super::tokens::now_unix().saturating_sub(PENDING_TTL.as_secs()); - pending.retain(|_, authorization| authorization.started_at >= cutoff); -} - /// Exposes [`generate_pkce`] to the module's tests. #[cfg(test)] pub(super) fn generate_pkce_for_test() -> (String, String) { diff --git a/crates/tinymcp/src/registry/oauth/mod.rs b/crates/tinymcp/src/registry/oauth/mod.rs index 49921dd3..959aa035 100644 --- a/crates/tinymcp/src/registry/oauth/mod.rs +++ b/crates/tinymcp/src/registry/oauth/mod.rs @@ -11,7 +11,9 @@ //! is frequently wrong about this. //! 2. [`OAuthFlow::begin`] discovers the authorization server, registers a //! client dynamically (RFC 7591), generates a PKCE pair, parks the pending -//! state, and returns the authorize URL for a browser. +//! state in a [`PendingAuthorizationStore`] (memory by default; a shared +//! one for a host running several instances), and returns the authorize +//! URL for a browser. //! 3. [`OAuthFlow::complete`] takes the code and state from the redirect, //! exchanges the code, and persists the token. //! @@ -47,11 +49,15 @@ pub(crate) mod credentials; pub(crate) mod endpoint_guard; pub(crate) mod flow; +pub(crate) mod pending; pub(crate) mod tokens; pub(crate) mod types; pub use credentials::OAuthCredentialStore; pub use flow::{DEFAULT_CLIENT_NAME, OAuthFlow}; +pub use pending::{ + MemoryPendingStore, PENDING_TTL_SECS, PendingAuthorization, PendingAuthorizationStore, +}; pub use tokens::{OAUTH_BUNDLE_KEY, refresh_if_expired}; pub use types::{AuthDetection, AuthKind, OAuthBundle}; 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 a93463cf..db1f5260 100644 --- a/crates/tinymcp/src/registry/oauth/mod_host_store_tests.rs +++ b/crates/tinymcp/src/registry/oauth/mod_host_store_tests.rs @@ -114,9 +114,16 @@ async fn the_server_a_pending_state_belongs_to_can_be_read_before_completing() { .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); + assert_eq!( + flow.pending_server(&state).await.unwrap().as_deref(), + Some(TENANT_SERVER) + ); + assert_eq!( + flow.pending_count().await.unwrap(), + 1, + "peeking does not consume" + ); + assert_eq!(flow.pending_server("never-issued").await.unwrap(), None); } #[tokio::test] @@ -291,7 +298,7 @@ async fn a_guarded_flow_refuses_an_internal_authorization_server() { assert!(error.to_string().contains("https"), "{error}"); assert_eq!(state.registrations.load(Ordering::SeqCst), 0); - assert_eq!(flow.pending_count(), 0); + assert_eq!(flow.pending_count().await.unwrap(), 0); } #[tokio::test] @@ -357,3 +364,136 @@ async fn desktop_flow_keeps_the_default_clients() { .is_none() ); } + +// --------------------------------------------------------------------------- +// Several instances sharing one pending store +// --------------------------------------------------------------------------- + +/// A host running several instances behind one callback URL: the redirect +/// lands on whichever instance the load balancer picks, so the one that +/// completes is not the one that began. +#[tokio::test] +async fn a_sign_in_begun_on_one_instance_completes_on_another_sharing_its_store() { + let (endpoint, authority) = authority_with_challenge().await; + let host = HostSecrets::remote(&endpoint); + let shared: Arc = Arc::new(MemoryPendingStore::new()); + let first = flow().with_pending_store(shared.clone()); + let second = flow().with_pending_store(shared.clone()); + + let url = first + .begin(&host, TENANT_SERVER, "https://host.test/oauth/callback") + .await + .expect("begin"); + let state = authorize_param(&url, "state").unwrap(); + + assert_eq!( + second.pending_server(&state).await.unwrap().as_deref(), + Some(TENANT_SERVER) + ); + let server_id = second + .complete(&host, &state, "the-code") + .await + .expect("complete on the other instance"); + + assert_eq!(server_id, TENANT_SERVER); + assert_eq!( + host.get(TENANT_SERVER, "Authorization").as_deref(), + Some("Bearer at-1") + ); + assert_eq!(authority.exchanges.load(Ordering::SeqCst), 1); + assert_eq!(shared.count().await.unwrap(), 0); +} + +/// Two instances racing to complete the same redirect exchange the code once. +#[tokio::test] +async fn racing_completions_of_one_state_exchange_the_code_once() { + let (endpoint, authority) = authority_with_challenge().await; + let host = HostSecrets::remote(&endpoint); + let shared: Arc = Arc::new(MemoryPendingStore::new()); + let first = flow().with_pending_store(shared.clone()); + let second = flow().with_pending_store(shared); + + let url = first + .begin(&host, TENANT_SERVER, "https://host.test/oauth/callback") + .await + .unwrap(); + let state = authorize_param(&url, "state").unwrap(); + + let (a, b) = tokio::join!( + first.complete(&host, &state, "the-code"), + second.complete(&host, &state, "the-code"), + ); + + assert_eq!(usize::from(a.is_ok()) + usize::from(b.is_ok()), 1); + assert_eq!(authority.exchanges.load(Ordering::SeqCst), 1); +} + +/// A host store that keeps entries past their lifetime cannot complete a +/// stale sign-in: the flow checks the age itself. +#[tokio::test] +async fn an_expired_authorization_in_a_lazy_store_is_refused() { + #[derive(Debug, Default)] + struct NeverExpires(parking_lot::Mutex>); + + impl PendingAuthorizationStore for NeverExpires { + fn park<'a>( + &'a self, + state: &'a str, + pending: PendingAuthorization, + ) -> futures_util::future::BoxFuture<'a, crate::Result<()>> { + self.0.lock().insert(state.to_string(), pending); + Box::pin(std::future::ready(Ok(()))) + } + + fn take<'a>( + &'a self, + state: &'a str, + ) -> futures_util::future::BoxFuture<'a, crate::Result>> + { + Box::pin(std::future::ready(Ok(self.0.lock().remove(state)))) + } + + fn peek<'a>( + &'a self, + state: &'a str, + ) -> futures_util::future::BoxFuture<'a, crate::Result>> + { + Box::pin(std::future::ready(Ok(self.0.lock().get(state).cloned()))) + } + + fn count(&self) -> futures_util::future::BoxFuture<'_, crate::Result> { + Box::pin(std::future::ready(Ok(self.0.lock().len()))) + } + } + + let (endpoint, authority) = authority_with_challenge().await; + let host = HostSecrets::remote(&endpoint); + let lazy = Arc::new(NeverExpires::default()); + lazy.park( + "stale", + PendingAuthorization { + server_id: TENANT_SERVER.into(), + code_verifier: "v".into(), + client_id: "client-1".into(), + client_secret: None, + token_endpoint: format!("{endpoint}/token"), + redirect_uri: "https://host.test/oauth/callback".into(), + started_at: 1, + }, + ) + .await + .unwrap(); + let flow = flow().with_pending_store(lazy); + + assert_eq!(flow.pending_server("stale").await.unwrap(), None); + let error = flow + .complete(&host, "stale", "the-code") + .await + .expect_err("expired"); + + assert!( + matches!(error, Error::MalformedResponse { .. }), + "{error:?}" + ); + assert_eq!(authority.exchanges.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 7dbc8412..3ab8d1a0 100644 --- a/crates/tinymcp/src/registry/oauth/mod_tests.rs +++ b/crates/tinymcp/src/registry/oauth/mod_tests.rs @@ -20,6 +20,7 @@ use axum::{Json, Router}; use serde_json::{Value, json}; use super::flow::{DEFAULT_CLIENT_NAME, OAuthFlow}; +use super::pending::{MemoryPendingStore, PendingAuthorization, PendingAuthorizationStore}; use super::tokens::{OAUTH_BUNDLE_KEY, refresh_if_expired}; use super::types::{AuthKind, TokenResponse}; use crate::Error; @@ -561,7 +562,10 @@ async fn completing_with_an_unknown_state_is_refused() { #[tokio::test] async fn a_fresh_flow_has_nothing_parked() { - assert_eq!(OAuthFlow::new(None).unwrap().pending_count(), 0); + assert_eq!( + OAuthFlow::new(None).unwrap().pending_count().await.unwrap(), + 0 + ); } // --------------------------------------------------------------------------- @@ -910,7 +914,7 @@ async fn beginning_parks_exactly_one_pending_authorization() { .await .unwrap(); - assert_eq!(flow.pending_count(), 1); + assert_eq!(flow.pending_count().await.unwrap(), 1); } #[tokio::test] @@ -1317,7 +1321,7 @@ async fn a_state_is_consumed_even_when_the_exchange_fails() { let _ = flow.complete(&store, &returned_state, "the-code").await; - assert_eq!(flow.pending_count(), 0); + assert_eq!(flow.pending_count().await.unwrap(), 0); let error = flow .complete(&store, &returned_state, "the-code") .await diff --git a/crates/tinymcp/src/registry/oauth/pending.rs b/crates/tinymcp/src/registry/oauth/pending.rs new file mode 100644 index 00000000..72a81b17 --- /dev/null +++ b/crates/tinymcp/src/registry/oauth/pending.rs @@ -0,0 +1,200 @@ +//! Where a sign-in waits between the browser redirect out and back. +//! +//! [`OAuthFlow::begin`] parks the PKCE verifier and the registered client +//! under the authorization's `state`; [`OAuthFlow::complete`] takes them back +//! when the redirect returns. By default they wait in this process's memory +//! ([`MemoryPendingStore`]), which is right for a desktop host: the redirect +//! comes back to the same process that started it. +//! +//! A host running several instances behind one callback URL cannot rely on +//! that. The redirect lands on whichever instance the load balancer picks, so +//! the pending authorization has to live somewhere they all see — a shared +//! database, say. Such a host implements [`PendingAuthorizationStore`] and +//! hands it to [`OAuthFlow::with_pending_store`]. +//! +//! The store keeps entries for at least [`PENDING_TTL_SECS`]; the flow itself +//! refuses any entry older than that, so a store that expires lazily (or +//! never) still cannot complete a stale sign-in. +//! +//! [`OAuthFlow::begin`]: super::OAuthFlow::begin +//! [`OAuthFlow::complete`]: super::OAuthFlow::complete +//! [`OAuthFlow::with_pending_store`]: super::OAuthFlow::with_pending_store + +use std::collections::HashMap; + +use futures_util::future::BoxFuture; +use parking_lot::Mutex; +use serde::{Deserialize, Serialize}; + +use crate::error::Result; + +/// How long an unfinished authorization stays parked, in seconds. +/// +/// A user who starts a sign-in and closes the tab leaves an entry behind +/// holding a PKCE verifier and perhaps a client secret. Ten minutes is longer +/// than any real sign-in and short enough that abandoned ones do not +/// accumulate. A host store expiring entries itself (a database TTL index) +/// uses this as its lifetime. +pub const PENDING_TTL_SECS: u64 = 10 * 60; + +/// An authorization parked between the browser redirect out and back. +/// +/// Public so a host can keep it in its own [`PendingAuthorizationStore`]. It +/// holds the PKCE verifier and, for a confidential client, the client secret: +/// a host stores it sealed and never logs or displays it. Its `Debug` output +/// redacts both. +#[derive(Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct PendingAuthorization { + /// Which install this authorization is for: the host's opaque server id. + pub server_id: String, + /// The PKCE verifier, sent with the code exchange. + pub code_verifier: String, + /// The dynamically registered client. + pub client_id: String, + /// The secret, when the server issued a confidential client. + pub client_secret: Option, + /// Where to exchange the code. + pub token_endpoint: String, + /// The redirect the authorization was started with. + pub redirect_uri: String, + /// When this was parked, in Unix seconds. + pub started_at: u64, +} + +impl PendingAuthorization { + /// Whether this authorization is older than [`PENDING_TTL_SECS`] at `now` + /// (Unix seconds). + #[must_use] + pub fn expired_at(&self, now: u64) -> bool { + self.started_at < now.saturating_sub(PENDING_TTL_SECS) + } +} + +impl std::fmt::Debug for PendingAuthorization { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("PendingAuthorization") + .field("server_id", &self.server_id) + .field("code_verifier", &"[redacted]") + .field("client_id", &self.client_id) + .field( + "client_secret", + &self.client_secret.as_ref().map(|_| "[redacted]"), + ) + .field("token_endpoint", &self.token_endpoint) + .field("redirect_uri", &self.redirect_uri) + .field("started_at", &self.started_at) + .finish() + } +} + +/// Where [`OAuthFlow`](super::OAuthFlow) parks authorizations between the +/// redirect out and back. +/// +/// `state` is the random value the flow put in the authorize URL. It is a +/// bearer secret for the duration of the sign-in: a host persisting it may key +/// entries on a digest of it instead. +/// +/// Object-safe, so a flow holds any store behind one pointer. +pub trait PendingAuthorizationStore: Send + Sync + std::fmt::Debug { + /// Parks `pending` under `state`, replacing any entry already there. + /// + /// # Errors + /// + /// Whatever the store reports when it cannot be written. + fn park<'a>( + &'a self, + state: &'a str, + pending: PendingAuthorization, + ) -> BoxFuture<'a, Result<()>>; + + /// Removes and returns the entry parked under `state`, or `None` when there + /// is none. + /// + /// Must be atomic: when two callers take the same `state` at once, at most + /// one receives the entry. That is what stops one authorization code from + /// being exchanged twice. + /// + /// # Errors + /// + /// Whatever the store reports when it cannot be read or written. + fn take<'a>(&'a self, state: &'a str) -> BoxFuture<'a, Result>>; + + /// Returns the entry parked under `state` without removing it. + /// + /// # Errors + /// + /// Whatever the store reports when it cannot be read. + fn peek<'a>(&'a self, state: &'a str) -> BoxFuture<'a, Result>>; + + /// How many entries are parked, expired ones included unless the store has + /// already dropped them. For tests and diagnostics. + /// + /// # Errors + /// + /// Whatever the store reports when it cannot be read. + fn count(&self) -> BoxFuture<'_, Result>; +} + +/// The default store: pending authorizations in this process's memory. +/// +/// Right when the redirect returns to the process that started the sign-in. +/// Expired entries are dropped whenever the store is touched. Its `Debug` +/// output shows only how many are parked: each key is a `state`, a bearer +/// secret while the sign-in is open. +#[derive(Default)] +pub struct MemoryPendingStore { + entries: Mutex>, +} + +impl std::fmt::Debug for MemoryPendingStore { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("MemoryPendingStore") + .field("parked", &self.entries.lock().len()) + .finish() + } +} + +impl MemoryPendingStore { + /// An empty store. + #[must_use] + pub fn new() -> Self { + Self::default() + } + + fn pruned(&self) -> parking_lot::MutexGuard<'_, HashMap> { + let mut entries = self.entries.lock(); + let now = super::tokens::now_unix(); + entries.retain(|_, pending| !pending.expired_at(now)); + entries + } +} + +impl PendingAuthorizationStore for MemoryPendingStore { + fn park<'a>( + &'a self, + state: &'a str, + pending: PendingAuthorization, + ) -> BoxFuture<'a, Result<()>> { + self.pruned().insert(state.to_string(), pending); + Box::pin(std::future::ready(Ok(()))) + } + + fn take<'a>(&'a self, state: &'a str) -> BoxFuture<'a, Result>> { + let taken = self.pruned().remove(state); + Box::pin(std::future::ready(Ok(taken))) + } + + fn peek<'a>(&'a self, state: &'a str) -> BoxFuture<'a, Result>> { + let found = self.pruned().get(state).cloned(); + Box::pin(std::future::ready(Ok(found))) + } + + fn count(&self) -> BoxFuture<'_, Result> { + let count = self.pruned().len(); + Box::pin(std::future::ready(Ok(count))) + } +} + +#[cfg(test)] +#[path = "pending_tests.rs"] +mod tests; diff --git a/crates/tinymcp/src/registry/oauth/pending_tests.rs b/crates/tinymcp/src/registry/oauth/pending_tests.rs new file mode 100644 index 00000000..db0f4ca1 --- /dev/null +++ b/crates/tinymcp/src/registry/oauth/pending_tests.rs @@ -0,0 +1,119 @@ +//! The default in-memory pending store, and the redacted shape a host +//! persists. + +#![allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)] + +use super::*; + +fn parked(started_at: u64) -> PendingAuthorization { + PendingAuthorization { + server_id: "acme/linear".into(), + code_verifier: "verifier-1".into(), + client_id: "client-1".into(), + client_secret: Some("secret-1".into()), + token_endpoint: "https://auth.test/token".into(), + redirect_uri: "https://host.test/oauth/callback".into(), + started_at, + } +} + +fn now() -> u64 { + super::super::tokens::now_unix() +} + +#[tokio::test] +async fn a_parked_authorization_is_taken_once() { + let at = now(); + let store = MemoryPendingStore::new(); + store.park("state-2", parked(at)).await.unwrap(); + + assert_eq!(store.take("state-2").await.unwrap(), Some(parked(at))); + assert_eq!(store.take("state-2").await.unwrap(), None); +} + +#[tokio::test] +async fn peeking_leaves_the_authorization_parked() { + let store = MemoryPendingStore::new(); + store.park("state-1", parked(now())).await.unwrap(); + + let peeked = store.peek("state-1").await.unwrap().expect("parked"); + assert_eq!(peeked.server_id, "acme/linear"); + assert_eq!(store.count().await.unwrap(), 1); + assert!(store.take("state-1").await.unwrap().is_some()); +} + +#[tokio::test] +async fn an_unknown_state_finds_nothing() { + let store = MemoryPendingStore::new(); + + assert_eq!(store.peek("never-issued").await.unwrap(), None); + assert_eq!(store.take("never-issued").await.unwrap(), None); + assert_eq!(store.count().await.unwrap(), 0); +} + +#[tokio::test] +async fn an_expired_authorization_is_dropped() { + let store = MemoryPendingStore::new(); + store + .park("stale", parked(now() - PENDING_TTL_SECS - 1)) + .await + .unwrap(); + + assert_eq!(store.count().await.unwrap(), 0); + assert_eq!(store.take("stale").await.unwrap(), None); +} + +#[test] +fn expiry_is_measured_from_when_it_was_parked() { + let at = 10_000; + assert!(!parked(at).expired_at(at + PENDING_TTL_SECS)); + assert!(parked(at).expired_at(at + PENDING_TTL_SECS + 1)); + assert!( + !parked(at).expired_at(0), + "a clock behind the entry is not expiry" + ); +} + +#[test] +fn debug_output_redacts_the_verifier_and_secret() { + let rendered = format!("{:?}", parked(1)); + + assert!(!rendered.contains("verifier-1"), "{rendered}"); + assert!(!rendered.contains("secret-1"), "{rendered}"); + assert!(rendered.contains("acme/linear"), "{rendered}"); +} + +#[test] +fn the_serde_representation_is_pinned() { + let value = serde_json::to_value(parked(42)).unwrap(); + + assert_eq!( + value, + serde_json::json!({ + "server_id": "acme/linear", + "code_verifier": "verifier-1", + "client_id": "client-1", + "client_secret": "secret-1", + "token_endpoint": "https://auth.test/token", + "redirect_uri": "https://host.test/oauth/callback", + "started_at": 42, + }) + ); + let back: PendingAuthorization = serde_json::from_value(value).unwrap(); + assert_eq!(back, parked(42)); +} + +#[tokio::test] +async fn the_memory_stores_debug_output_never_shows_a_state() { + let store = MemoryPendingStore::new(); + store.park("state-secret-1", parked(now())).await.unwrap(); + let flow = super::super::OAuthFlow::new(None) + .unwrap() + .with_pending_store(std::sync::Arc::new(store)); + + let rendered = format!("{flow:?}"); + + assert!(!rendered.contains("state-secret-1"), "{rendered}"); + assert!(!rendered.contains("verifier-1"), "{rendered}"); + assert!(rendered.contains("parked: 1"), "{rendered}"); +} diff --git a/crates/tinymcp/src/registry/oauth/types.rs b/crates/tinymcp/src/registry/oauth/types.rs index 9176a49d..a2a5ee9c 100644 --- a/crates/tinymcp/src/registry/oauth/types.rs +++ b/crates/tinymcp/src/registry/oauth/types.rs @@ -9,28 +9,6 @@ use crate::error::{Error, Result}; // here so every path through this module keeps resolving. pub use tinymcp_bus::{AuthDetection, AuthKind}; -/// An authorization parked between the browser redirect out and back. -#[derive(Debug, Clone)] -pub(super) struct PendingAuthorization { - /// Which install this authorization is for. - pub(super) server_id: String, - /// The PKCE verifier, sent with the code exchange. - pub(super) code_verifier: String, - /// The dynamically registered client. - pub(super) client_id: String, - /// The secret, when the server issued a confidential client. - pub(super) client_secret: Option, - /// Where to exchange the code. - pub(super) token_endpoint: String, - /// The redirect the authorization was started with. - pub(super) redirect_uri: String, - /// When this was parked, in Unix seconds. - /// - /// Present so an authorization the user abandoned does not sit in memory - /// holding a client secret forever. - pub(super) started_at: u64, -} - /// The bookkeeping needed to mint a new access token without another sign-in. /// /// Stored as JSON beside the access token under diff --git a/crates/tinymcp/src/registry/ops/mod_tests.rs b/crates/tinymcp/src/registry/ops/mod_tests.rs index 2a4483c6..b3102e9a 100644 --- a/crates/tinymcp/src/registry/ops/mod_tests.rs +++ b/crates/tinymcp/src/registry/ops/mod_tests.rs @@ -1388,7 +1388,7 @@ async fn the_facade_hands_out_the_pieces_a_host_drives_directly() { let (handle, _receiver) = registry.vault().request("API_KEY").await; assert!(registry.vault().submit(&handle, "sekrit".into()).await); - assert_eq!(registry.oauth().pending_count(), 0); + assert_eq!(registry.oauth().pending_count().await.unwrap(), 0); assert_eq!(registry.connected_overview().await.len(), 0); } diff --git a/crates/tinymcp/tests/public_reexports.rs b/crates/tinymcp/tests/public_reexports.rs index fc0a789b..090498d1 100644 --- a/crates/tinymcp/tests/public_reexports.rs +++ b/crates/tinymcp/tests/public_reexports.rs @@ -1,5 +1,7 @@ //! Pins the crate-root paths a host imports the contract types from. +#![allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)] + use tinymcp::{ CONTRACT_VERSION, MCP_CALL_RESULT_KIND, McpAuthChallenge, McpCallError, McpCallOutcome, RegistryFreshness, @@ -25,3 +27,27 @@ fn the_contract_types_resolve_at_the_crate_root_to_the_bus_definitions() { let freshness: tinymcp_bus::RegistryFreshness = RegistryFreshness::LocalFallback; assert_ne!(freshness, RegistryFreshness::Live); } + +#[test] +fn the_pending_sign_in_store_api_resolves_at_the_crate_root() { + use std::sync::Arc; + use tinymcp::{ + MemoryPendingStore, OAuthFlow, PENDING_TTL_SECS, PendingAuthorization, + PendingAuthorizationStore, + }; + + let store: Arc = Arc::new(MemoryPendingStore::new()); + let _flow = OAuthFlow::new(None) + .expect("the flow builds") + .with_pending_store(store); + let parked = PendingAuthorization { + server_id: "docs".into(), + code_verifier: "v".into(), + client_id: "c".into(), + client_secret: None, + token_endpoint: "https://auth.test/token".into(), + redirect_uri: "https://host.test/cb".into(), + started_at: 0, + }; + assert!(parked.expired_at(PENDING_TTL_SECS + 1)); +}