From eebce2892688dd4f94b2e89ded31a3fb1a59ac06 Mon Sep 17 00:00:00 2001 From: Johanna Larsson Date: Tue, 22 Sep 2026 23:32:29 +0100 Subject: [PATCH] fix: handle wildcard + aud in OAuth scopes `rpc:*?aud=did:web:api.bsky.app#bsky_appview` fails in the permissions check even though it's a valid scope. This makes it resolve correctly. Also this commit fixes a bug where `rpc` as a bare scope is interpreted as all methods for all services, but it's really an invalid scope. Can drop it if it's a problem, but it just looks like a bug in the implementation rather than something intended? Note that the parser treats `rpc:*` as "allow everything for no services", but aud is actually required, so it's not a valid scope? Didn't mess with it though because delegation uses it and that part of the code treats it as allow everything for every service. We should maybe split the scope parsers so we have one for delegated scopes and one for regular? However, to avoid the same bug as bare `rpc`, I changed the enforcement step in "regular" OAuth to disallow `rpc:*` (this matches the existing behavior on main, no actual change, just a little acknowledgement). --- crates/tranquil-scopes/src/coverage.rs | 6 +---- crates/tranquil-scopes/src/parser.rs | 22 +++++++++------- crates/tranquil-scopes/src/permissions.rs | 31 +++++++++++++++++------ 3 files changed, 37 insertions(+), 22 deletions(-) diff --git a/crates/tranquil-scopes/src/coverage.rs b/crates/tranquil-scopes/src/coverage.rs index ea9e50e..db904ce 100644 --- a/crates/tranquil-scopes/src/coverage.rs +++ b/crates/tranquil-scopes/src/coverage.rs @@ -92,11 +92,7 @@ fn blob_covers(g: &BlobScope, r: &BlobScope) -> bool { } fn rpc_covers(g: &RpcScope, r: &RpcScope) -> bool { - let lxm_ok = match &g.lxm { - None => true, - Some(gl) if gl == "*" => true, - Some(gl) => r.lxm.as_deref() == Some(gl.as_str()), - }; + let lxm_ok = g.lxm == "*" || g.lxm == r.lxm; let aud_ok = match &g.aud { None => true, Some(ga) if ga == "*" => true, diff --git a/crates/tranquil-scopes/src/parser.rs b/crates/tranquil-scopes/src/parser.rs index 191dae1..949cd87 100644 --- a/crates/tranquil-scopes/src/parser.rs +++ b/crates/tranquil-scopes/src/parser.rs @@ -93,7 +93,7 @@ impl BlobScope { #[derive(Debug, Clone, PartialEq, Eq)] pub struct RpcScope { - pub lxm: Option, + pub lxm: String, pub aud: Option, } @@ -245,9 +245,10 @@ pub fn parse_scope(scope: &str) -> ParsedScope { let lxm = lxm_positional.or_else(|| params.get("lxm").and_then(|v| v.first().cloned())); let aud = params.get("aud").and_then(|v| v.first().cloned()); - let is_lxm_wildcard = lxm.as_deref() == Some("*") || lxm.is_none(); - let is_aud_wildcard = aud.as_deref() == Some("*"); - if is_lxm_wildcard && is_aud_wildcard { + let Some(lxm) = lxm else { + return ParsedScope::Unknown(scope.to_string()); + }; + if lxm == "*" && aud.as_deref() == Some("*") { return ParsedScope::Unknown(scope.to_string()); } @@ -400,7 +401,7 @@ mod tests { let scope = parse_scope("rpc:app.bsky.feed.getTimeline?aud=did:web:api.bsky.app"); match scope { ParsedScope::Rpc(r) => { - assert_eq!(r.lxm, Some("app.bsky.feed.getTimeline".to_string())); + assert_eq!(r.lxm, "app.bsky.feed.getTimeline"); assert_eq!(r.aud, Some("did:web:api.bsky.app".to_string())); } _ => panic!("Expected Rpc scope"), @@ -510,6 +511,12 @@ mod tests { assert!(matches!(scope4, ParsedScope::Rpc(_))); } + #[test] + fn test_rpc_lxm_required() { + let bare = parse_scope("rpc"); + assert!(matches!(bare, ParsedScope::Unknown(_))); + } + #[test] fn test_url_encoded_aud_with_fragment() { let scope = @@ -527,10 +534,7 @@ mod tests { ); match scope2 { ParsedScope::Rpc(r) => { - assert_eq!( - r.lxm, - Some("com.atproto.moderation.createReport".to_string()) - ); + assert_eq!(r.lxm, "com.atproto.moderation.createReport"); assert_eq!(r.aud, Some("did:web:api.bsky.app#bsky_appview".to_string())); } _ => panic!("Expected Rpc scope"), diff --git a/crates/tranquil-scopes/src/permissions.rs b/crates/tranquil-scopes/src/permissions.rs index 47b1189..7ebd281 100644 --- a/crates/tranquil-scopes/src/permissions.rs +++ b/crates/tranquil-scopes/src/permissions.rs @@ -49,10 +49,7 @@ impl ScopePermissions { pub fn superseded_by_transition_generic(scope: &ParsedScope) -> bool { match scope { ParsedScope::Repo(_) | ParsedScope::Blob(_) => true, - ParsedScope::Rpc(rpc) => !rpc - .lxm - .as_deref() - .is_some_and(|lxm| lxm == "*" || lxm.starts_with("chat.bsky.")), + ParsedScope::Rpc(rpc) => !(rpc.lxm == "*" || rpc.lxm.starts_with("chat.bsky.")), ParsedScope::Account(_) | ParsedScope::Identity(_) | ParsedScope::TransitionEmail @@ -193,9 +190,9 @@ impl ScopePermissions { let has_permission = self.find_rpc_scopes().any(|rpc_scope| { let lxm_matches = match &rpc_scope.lxm { - None => true, - Some(scope_lxm) if scope_lxm == lxm => true, - Some(scope_lxm) if scope_lxm.ends_with(".*") => { + scope_lxm if scope_lxm == "*" => true, + scope_lxm if scope_lxm == lxm => true, + scope_lxm if scope_lxm.ends_with(".*") => { let prefix = scope_lxm.strip_suffix(".*").unwrap(); lxm.starts_with(prefix) && lxm.chars().nth(prefix.len()) == Some('.') } @@ -203,7 +200,7 @@ impl ScopePermissions { }; let aud_matches = match &rpc_scope.aud { - None => true, + None => false, Some(scope_aud) if scope_aud == "*" => true, Some(scope_aud) => scope_aud == aud, }; @@ -474,6 +471,24 @@ mod tests { assert!(!perms.allows_rpc("did:web:api.bsky.app", &c("app.bsky.feed.getAuthorFeed"))); } + #[test] + fn test_rpc_wildcard_lxm() { + let perms = ScopePermissions::from_scope_string(Some( + "rpc:*?aud=did:web:api.bsky.app#bsky_appview", + )); + let aud = "did:web:api.bsky.app#bsky_appview"; + let other = "did:web:other.app#bsky_appview"; + assert!(perms.allows_rpc(aud, &c("app.bsky.feed.getTimeline"))); + assert!(!perms.allows_rpc(other, &c("app.bsky.feed.getTimeline"))); + } + + #[test] + fn test_rpc_wildcard_lxm_without_aud() { + let perms = ScopePermissions::from_scope_string(Some("rpc:*")); + let aud = "did:web:api.bsky.app#bsky_appview"; + assert!(!perms.allows_rpc(aud, &c("app.bsky.feed.getTimeline"))); + } + #[test] fn test_granular_account() { let perms = ScopePermissions::from_scope_string(Some("account:email?action=read"));