From aac6f2818b76dff22524ce4bd59931c6e3e9083c Mon Sep 17 00:00:00 2001 From: Lewis Date: Sat, 21 Mar 2026 11:29:50 +0200 Subject: [PATCH] fix(oauth): fail properly on non-expanded include scopes --- .../src/endpoints/authorize.rs | 11 ++++- .../src/endpoints/token/grants.rs | 6 ++- crates/tranquil-pds/src/oauth/scopes/mod.rs | 5 ++- crates/tranquil-scopes/src/lib.rs | 2 +- crates/tranquil-scopes/src/permission_set.rs | 45 ++++++++++++++----- 5 files changed, 52 insertions(+), 17 deletions(-) diff --git a/crates/tranquil-oauth-server/src/endpoints/authorize.rs b/crates/tranquil-oauth-server/src/endpoints/authorize.rs index 7f7bef6..76a0648 100644 --- a/crates/tranquil-oauth-server/src/endpoints/authorize.rs +++ b/crates/tranquil-oauth-server/src/endpoints/authorize.rs @@ -1432,7 +1432,16 @@ pub async fn consent_get( requested_scope_str.to_string() }; - let expanded_scope_str = expand_include_scopes(&effective_scope_str).await; + let expanded_scope_str = match expand_include_scopes(&effective_scope_str).await { + Ok(s) => s, + Err(e) => { + return json_error( + StatusCode::BAD_REQUEST, + "invalid_scope", + &format!("Failed to expand permission set: {e}"), + ); + } + }; let requested_scopes: Vec<&str> = expanded_scope_str.split_whitespace().collect(); let consent_client_id = ClientId::from(request_data.parameters.client_id.clone()); let preferences = state diff --git a/crates/tranquil-oauth-server/src/endpoints/token/grants.rs b/crates/tranquil-oauth-server/src/endpoints/token/grants.rs index 25ae0f1..82d790a 100644 --- a/crates/tranquil-oauth-server/src/endpoints/token/grants.rs +++ b/crates/tranquil-oauth-server/src/endpoints/token/grants.rs @@ -155,7 +155,11 @@ pub async fn handle_authorization_code_grant( let final_scope = if let Some(ref scope) = raw_scope { if scope.contains("include:") { - Some(expand_include_scopes(scope).await) + Some( + expand_include_scopes(scope) + .await + .map_err(|e| OAuthError::InvalidScope(format!("Failed to expand permission set: {e}")))?, + ) } else { raw_scope } diff --git a/crates/tranquil-pds/src/oauth/scopes/mod.rs b/crates/tranquil-pds/src/oauth/scopes/mod.rs index 5c1c24b..944eda7 100644 --- a/crates/tranquil-pds/src/oauth/scopes/mod.rs +++ b/crates/tranquil-pds/src/oauth/scopes/mod.rs @@ -1,6 +1,7 @@ pub use tranquil_scopes::{ AccountAction, AccountAttr, AccountScope, BlobScope, IdentityAttr, IdentityScope, IncludeScope, ParsedScope, RepoAction, RepoScope, RpcScope, SCOPE_DEFINITIONS, ScopeCategory, - ScopeDefinition, ScopeError, ScopePermissions, expand_include_scopes, format_scope_for_display, - get_required_scopes, get_scope_definition, is_valid_scope, parse_scope, parse_scope_string, + ScopeDefinition, ScopeError, ScopeExpansionError, ScopePermissions, expand_include_scopes, + format_scope_for_display, get_required_scopes, get_scope_definition, is_valid_scope, + parse_scope, parse_scope_string, }; diff --git a/crates/tranquil-scopes/src/lib.rs b/crates/tranquil-scopes/src/lib.rs index 463daf2..9b9a5c9 100644 --- a/crates/tranquil-scopes/src/lib.rs +++ b/crates/tranquil-scopes/src/lib.rs @@ -13,5 +13,5 @@ pub use parser::{ AccountAction, AccountAttr, AccountScope, BlobScope, IdentityAttr, IdentityScope, IncludeScope, ParsedScope, RepoAction, RepoScope, RpcScope, parse_scope, parse_scope_string, }; -pub use permission_set::expand_include_scopes; +pub use permission_set::{ScopeExpansionError, expand_include_scopes}; pub use permissions::ScopePermissions; diff --git a/crates/tranquil-scopes/src/permission_set.rs b/crates/tranquil-scopes/src/permission_set.rs index 7bec965..a103e8b 100644 --- a/crates/tranquil-scopes/src/permission_set.rs +++ b/crates/tranquil-scopes/src/permission_set.rs @@ -4,7 +4,7 @@ use serde::Deserialize; use std::collections::HashMap; use std::sync::LazyLock; use tokio::sync::RwLock; -use tracing::{debug, warn}; +use tracing::debug; #[derive(Debug, thiserror::Error)] pub enum ScopeExpansionError { @@ -73,26 +73,27 @@ struct PermissionEntry { aud: Option, } -pub async fn expand_include_scopes(scope_string: &str) -> String { +pub async fn expand_include_scopes( + scope_string: &str, +) -> Result { let futures: Vec<_> = scope_string .split_whitespace() .map(|scope| async move { match scope.strip_prefix("include:") { Some(rest) => { let (nsid_base, aud) = parse_include_scope(rest); - expand_permission_set(nsid_base, aud) - .await - .unwrap_or_else(|e| { - warn!(nsid = nsid_base, error = %e, "Failed to expand permission set, keeping original"); - scope.to_string() - }) + expand_permission_set(nsid_base, aud).await } - None => scope.to_string(), + None => Ok(scope.to_string()), } }) .collect(); - futures::future::join_all(futures).await.join(" ") + futures::future::join_all(futures) + .await + .into_iter() + .collect::, ScopeExpansionError>>() + .map(|v| v.join(" ")) } fn parse_include_scope(rest: &str) -> (&str, Option<&str>) { @@ -553,17 +554,37 @@ mod tests { #[tokio::test] async fn test_expand_include_scopes_passthrough_non_include() { - let result = expand_include_scopes("atproto transition:generic").await; + let result = expand_include_scopes("atproto transition:generic") + .await + .unwrap(); assert_eq!(result, "atproto transition:generic"); } #[tokio::test] async fn test_expand_include_scopes_mixed_with_regular() { - let result = expand_include_scopes("atproto repo:app.bsky.feed.post?action=create").await; + let result = expand_include_scopes("atproto repo:app.bsky.feed.post?action=create") + .await + .unwrap(); assert!(result.contains("atproto")); assert!(result.contains("repo:app.bsky.feed.post?action=create")); } + #[tokio::test] + async fn test_expand_include_scopes_fails_on_unresolvable_nsid() { + let result = + expand_include_scopes("atproto include:nonexistent.fake.permissionSet").await; + assert!(result.is_err()); + } + + #[tokio::test] + async fn test_expand_include_scopes_fails_even_with_valid_scopes_present() { + let result = expand_include_scopes( + "atproto include:nonexistent.fake.permissionSet repo:app.bsky.feed.post?action=create", + ) + .await; + assert!(result.is_err()); + } + #[tokio::test] async fn test_cache_population_and_retrieval() { let cache_key = "test.cached.scope";