From 3ab5e7e56f89138071d5fe3a1f43f24e9d066011 Mon Sep 17 00:00:00 2001 From: Trezy Date: Mon, 11 May 2026 14:17:34 -0500 Subject: [PATCH] fix: prevent creation of API clients that would break HappyView core client Signed-off-by: Trezy --- src/admin/api_clients.rs | 7 +++++ src/auth/client_registry.rs | 56 +++++++++++++++++++++++++++++++++++++ 2 files changed, 63 insertions(+) diff --git a/src/admin/api_clients.rs b/src/admin/api_clients.rs index 6da0c41..f2188a1 100644 --- a/src/admin/api_clients.rs +++ b/src/admin/api_clients.rs @@ -32,6 +32,13 @@ pub(super) async fn create_api_client( ) -> Result<(StatusCode, Json), AppError> { auth.require(Permission::ApiClientsCreate).await?; + if state.oauth.is_domain_client_id(&body.client_id_url) { + return Err(AppError::Conflict(format!( + "client_id_url '{}' conflicts with a registered domain's OAuth client", + body.client_id_url + ))); + } + // Generate the client key: "hvc_" + 32 random hex chars. let mut random_bytes = [0u8; 16]; rand::rng().fill(&mut random_bytes); diff --git a/src/auth/client_registry.rs b/src/auth/client_registry.rs index 78cf1d9..0b1f821 100644 --- a/src/auth/client_registry.rs +++ b/src/auth/client_registry.rs @@ -130,6 +130,20 @@ impl OAuthClientRegistry { self.primary_client.store(client); } + /// 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). + 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 + }) + } + /// Build and register a single OAuth client from API client metadata. /// Used when creating or updating an API client via the admin UI. pub fn register_api_client( @@ -140,6 +154,12 @@ impl OAuthClientRegistry { scopes_str: &str, params: &ApiClientOAuthParams, ) -> Result<(), String> { + if self.is_domain_client_id(client_id_url) { + return Err(format!( + "client_id_url '{}' conflicts with a registered domain's OAuth client", + client_id_url + )); + } let ApiClientOAuthParams { plc_url, state_store, @@ -232,6 +252,14 @@ impl OAuthClientRegistry { }; for (client_id_url, client_uri, redirect_uris_json, scopes_str) in rows { + if self.is_domain_client_id(&client_id_url) { + tracing::warn!( + client_id = %client_id_url, + "Skipping API client that conflicts with a domain OAuth client" + ); + continue; + } + let redirect_uris: Vec = serde_json::from_str(&redirect_uris_json).unwrap_or_default(); @@ -332,4 +360,32 @@ mod tests { assert_eq!(map.get("key1").unwrap().value(), "val2"); } + + #[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(), + ); + + 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 + }) + }; + + assert!(matches("https://example.com/oauth-client-metadata.json")); + assert!(matches( + "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")); + } } -- 2.51.2