From 4f5a8ef572613f0db314dbb774d8dd8ae622193a Mon Sep 17 00:00:00 2001 From: Trezy Date: Mon, 11 May 2026 15:00:39 -0500 Subject: [PATCH] fix: prevent base path collision Signed-off-by: Trezy --- src/admin/domains.rs | 29 ++++++--- src/auth/client_registry.rs | 120 ++++++++++++++++++++++++------------ src/main.rs | 18 +++++- 3 files changed, 118 insertions(+), 49 deletions(-) diff --git a/src/admin/domains.rs b/src/admin/domains.rs index e91b1a5..4a205a2 100644 --- a/src/admin/domains.rs +++ b/src/admin/domains.rs @@ -123,17 +123,25 @@ pub(super) async fn create( updated_at: now, }; + // Build the base-path-aware URLs for this domain + let domain_base_url = state.config.url_with_base_path(&url); + let client_id_url = format!( + "{}/oauth-client-metadata.json", + domain_base_url.trim_end_matches('/') + ); + // Register the OAuth client for this domain - state - .oauth - .register_domain_client(url.clone(), state.oauth.primary_client()); + state.oauth.register_domain_client( + url.clone(), + client_id_url.clone(), + state.oauth.primary_client(), + ); // Build a proper OAuth client if not loopback let domain_is_loopback = url.contains("127.0.0.1") || url.contains("[::1]") || url.contains("localhost"); if !domain_is_loopback { - let client_id_url = format!("{}/oauth-client-metadata.json", url.trim_end_matches('/')); - let callback = format!("{}/auth/callback", url.trim_end_matches('/')); + let callback = format!("{}/auth/callback", domain_base_url.trim_end_matches('/')); if let Err(e) = state.oauth.register_api_client( &client_id_url, &url, @@ -151,7 +159,9 @@ pub(super) async fn create( // Move from `clients` (where register_api_client puts it) to domain_clients + clients if let Some(client) = state.oauth.get(&client_id_url) { state.oauth.remove(&client_id_url); - state.oauth.register_domain_client(url.clone(), client); + state + .oauth + .register_domain_client(url.clone(), client_id_url, client); } } } @@ -211,7 +221,12 @@ pub(super) async fn delete( .map_err(|e| AppError::Internal(format!("failed to delete domain: {e}")))?; // Remove OAuth client and cache entry - state.oauth.remove_domain_client(&url); + let domain_base_url = state.config.url_with_base_path(&url); + let client_id_url = format!( + "{}/oauth-client-metadata.json", + domain_base_url.trim_end_matches('/') + ); + state.oauth.remove_domain_client(&url, &client_id_url); let host = url .strip_prefix("https://") .or_else(|| url.strip_prefix("http://")) diff --git a/src/auth/client_registry.rs b/src/auth/client_registry.rs index 0b1f821..995496b 100644 --- a/src/auth/client_registry.rs +++ b/src/auth/client_registry.rs @@ -91,23 +91,26 @@ impl OAuthClientRegistry { /// Register a domain-specific OAuth client. /// Inserts into both `domain_clients` (keyed by domain URL, for `get_for_domain`) /// and `clients` (keyed by client_id_url, for `get_or_default`). - pub fn register_domain_client(&self, domain_url: String, client: Arc) { - let client_id_url = format!( - "{}/oauth-client-metadata.json", - domain_url.trim_end_matches('/') - ); + /// + /// `client_id_url` must be the base-path-aware client ID + /// (e.g. `{domain_url}{base_path}/oauth-client-metadata.json`). + pub fn register_domain_client( + &self, + domain_url: String, + client_id_url: String, + client: Arc, + ) { self.domain_clients.insert(domain_url, Arc::clone(&client)); self.clients.insert(client_id_url, client); } /// Remove a domain-specific OAuth client from both maps. - pub fn remove_domain_client(&self, domain_url: &str) { + /// + /// `client_id_url` must be the same base-path-aware client ID that was + /// passed to `register_domain_client`. + pub fn remove_domain_client(&self, domain_url: &str, client_id_url: &str) { self.domain_clients.remove(domain_url); - let client_id_url = format!( - "{}/oauth-client-metadata.json", - domain_url.trim_end_matches('/') - ); - self.clients.remove(&client_id_url); + self.clients.remove(client_id_url); } /// Look up a domain-specific OAuth client. @@ -131,17 +134,18 @@ impl OAuthClientRegistry { } /// Returns true if the given `client_id_url` is already claimed by a domain - /// client (i.e. matches `{domain_url}/oauth-client-metadata.json` for any - /// registered domain). + /// client. Checks by comparing against the actual client instances stored by + /// domain registrations, so it works correctly regardless of `BASE_PATH`. pub fn is_domain_client_id(&self, client_id_url: &str) -> bool { - self.domain_clients.iter().any(|entry| { - let domain_url = entry.key(); - let expected = format!( - "{}/oauth-client-metadata.json", - domain_url.trim_end_matches('/') - ); - expected == client_id_url - }) + // A client_id_url belongs to a domain client if any domain_clients entry + // has the same Arc as the one stored in `clients` under that key. + if let Some(candidate) = self.clients.get(client_id_url) { + self.domain_clients + .iter() + .any(|entry| Arc::ptr_eq(entry.value(), candidate.value())) + } else { + false + } } /// Build and register a single OAuth client from API client metadata. @@ -363,29 +367,67 @@ mod tests { #[test] fn test_domain_client_id_collision_detection() { - let domains: DashMap = DashMap::new(); - domains.insert("https://example.com".to_string(), "client".to_string()); - domains.insert( - "https://other.example.com/".to_string(), - "client".to_string(), + // Simulate the is_domain_client_id logic using raw DashMaps and Arc pointer equality, + // mirroring the real OAuthClientRegistry implementation. + let domain_clients: DashMap> = DashMap::new(); + let clients: DashMap> = DashMap::new(); + + // Register domain "https://example.com" with base-path-aware client_id_url + let client_a = Arc::new("client_a".to_string()); + domain_clients.insert("https://example.com".to_string(), Arc::clone(&client_a)); + clients.insert( + "https://example.com/hv/oauth-client-metadata.json".to_string(), + client_a, ); - let matches = |client_id_url: &str| -> bool { - domains.iter().any(|entry| { - let domain_url = entry.key(); - let expected = format!( - "{}/oauth-client-metadata.json", - domain_url.trim_end_matches('/') - ); - expected == client_id_url - }) + // Register domain "https://other.example.com" without base path + let client_b = Arc::new("client_b".to_string()); + domain_clients.insert( + "https://other.example.com".to_string(), + Arc::clone(&client_b), + ); + clients.insert( + "https://other.example.com/oauth-client-metadata.json".to_string(), + client_b, + ); + + // Also register a non-domain API client + let api_client = Arc::new("api_client".to_string()); + clients.insert( + "https://api.example.com/oauth-client-metadata.json".to_string(), + api_client, + ); + + let is_domain_client_id = |client_id_url: &str| -> bool { + if let Some(candidate) = clients.get(client_id_url) { + domain_clients + .iter() + .any(|entry| Arc::ptr_eq(entry.value(), candidate.value())) + } else { + false + } }; - assert!(matches("https://example.com/oauth-client-metadata.json")); - assert!(matches( + // Base-path-aware key is detected as a domain client + assert!(is_domain_client_id( + "https://example.com/hv/oauth-client-metadata.json" + )); + // Non-base-path key is also detected + assert!(is_domain_client_id( "https://other.example.com/oauth-client-metadata.json" )); - assert!(!matches("https://unrelated.com/oauth-client-metadata.json")); - assert!(!matches("https://example.com/other-path.json")); + // Unrelated URLs are not detected + assert!(!is_domain_client_id( + "https://unrelated.com/oauth-client-metadata.json" + )); + assert!(!is_domain_client_id("https://example.com/other-path.json")); + // The old (wrong) key without base path is not detected + assert!(!is_domain_client_id( + "https://example.com/oauth-client-metadata.json" + )); + // API client is not detected as a domain client + assert!(!is_domain_client_id( + "https://api.example.com/oauth-client-metadata.json" + )); } } diff --git a/src/main.rs b/src/main.rs index 8145cf4..33b998a 100644 --- a/src/main.rs +++ b/src/main.rs @@ -519,7 +519,15 @@ async fn main() { // Register the primary domain's OAuth client in domain_clients if let Some(ref pd) = domain_cache.primary().await { - oauth_registry.register_domain_client(pd.url.clone(), Arc::clone(&oauth_client_arc)); + let primary_client_id_url = format!( + "{}/oauth-client-metadata.json", + config.url_with_base_path(&pd.url).trim_end_matches('/') + ); + oauth_registry.register_domain_client( + pd.url.clone(), + primary_client_id_url, + Arc::clone(&oauth_client_arc), + ); } // Build OAuth clients for all non-primary domains @@ -553,7 +561,7 @@ async fn main() { match atrium_oauth::OAuthClient::new(OAuthClientConfig { client_metadata: AtprotoClientMetadata { - client_id: domain_client_id, + client_id: domain_client_id.clone(), client_uri: Some(domain_base_url.clone()), redirect_uris: vec![domain_callback_url], token_endpoint_auth_method: AuthMethod::None, @@ -569,7 +577,11 @@ async fn main() { }) { Ok(client) => { info!(domain = %domain.url, "Registered domain OAuth client"); - oauth_registry.register_domain_client(domain.url.clone(), Arc::new(client)); + oauth_registry.register_domain_client( + domain.url.clone(), + domain_client_id, + Arc::new(client), + ); } Err(e) => { tracing::error!(domain = %domain.url, error = %e, "Failed to create domain OAuth client"); -- 2.51.2