From 1b5a2b319c2687fc33a0486a8550becc70b50002 Mon Sep 17 00:00:00 2001 From: Louis Escher Date: Wed, 19 Aug 2026 14:28:52 +0200 Subject: [PATCH] fix: getServiceAuth aud parsing --- crates/tranquil-api/src/moderation/mod.rs | 4 +- .../tranquil-api/src/server/service_auth.rs | 6 +- crates/tranquil-auth/src/token.rs | 4 +- crates/tranquil-pds/src/api/proxy.rs | 4 +- crates/tranquil-pds/tests/jwt_security.rs | 4 +- crates/tranquil-pds/tests/oauth.rs | 46 ++++++ crates/tranquil-pds/tests/scope_edge_cases.rs | 32 ++++ crates/tranquil-pds/tests/server.rs | 40 +++++ crates/tranquil-types/src/lib.rs | 139 ++++++++++++++++++ 9 files changed, 269 insertions(+), 10 deletions(-) diff --git a/crates/tranquil-api/src/moderation/mod.rs b/crates/tranquil-api/src/moderation/mod.rs index 88045f1..27119bb 100644 --- a/crates/tranquil-api/src/moderation/mod.rs +++ b/crates/tranquil-api/src/moderation/mod.rs @@ -12,7 +12,7 @@ use tranquil_pds::api::ApiError; use tranquil_pds::api::proxy_client::{is_ssrf_safe, proxy_client}; use tranquil_pds::auth::{AnyUser, Auth}; use tranquil_pds::state::AppState; -use tranquil_pds::types::{Did, Nsid}; +use tranquil_pds::types::{Did, DidRef, Nsid}; static CREATE_REPORT_NSID: LazyLock = LazyLock::new(|| "com.atproto.moderation.createReport".parse().unwrap()); @@ -151,7 +151,7 @@ async fn proxy_to_report_service( let service_token = match tranquil_pds::auth::create_service_token( &auth_user.did, - service_did, + &DidRef::from(service_did), Some(&CREATE_REPORT_NSID), &key_bytes, ) { diff --git a/crates/tranquil-api/src/server/service_auth.rs b/crates/tranquil-api/src/server/service_auth.rs index da08672..5b66b27 100644 --- a/crates/tranquil-api/src/server/service_auth.rs +++ b/crates/tranquil-api/src/server/service_auth.rs @@ -11,7 +11,7 @@ use tracing::{error, info, warn}; use tranquil_pds::api::error::ApiError; use tranquil_pds::auth::extractor::{Auth, Permissive}; use tranquil_pds::state::AppState; -use tranquil_pds::types::Did; +use tranquil_pds::types::DidRef; use tranquil_types::Nsid; static CREATE_ACCOUNT_NSID: LazyLock = @@ -45,7 +45,7 @@ static PROTECTED_METHODS: LazyLock> = LazyLock::new(|| { #[derive(Deserialize)] pub struct GetServiceAuthParams { - pub aud: Did, + pub aud: DidRef, pub lxm: Option, pub exp: Option, } @@ -146,6 +146,8 @@ pub async fn get_service_auth( .into_response(); } + // NOTE: exp is validated here but never reaches create_service_token, which hardcodes a 60 + // second lifetime, so a client asking for longer silently gets 60 seconds if let Some(exp) = params.exp { let now = chrono::Utc::now().timestamp(); let diff = exp - now; diff --git a/crates/tranquil-auth/src/token.rs b/crates/tranquil-auth/src/token.rs index 9fee5fb..8a851cc 100644 --- a/crates/tranquil-auth/src/token.rs +++ b/crates/tranquil-auth/src/token.rs @@ -10,7 +10,7 @@ use chrono::{DateTime, Duration, Utc}; use hmac::{Hmac, Mac}; use k256::ecdsa::{Signature, SigningKey, signature::Signer}; use sha2::Sha256; -use tranquil_types::{Did, Jti, Nsid}; +use tranquil_types::{Did, DidRef, Jti, Nsid}; type HmacSha256 = Hmac; @@ -127,7 +127,7 @@ pub fn create_refresh_token_with_jti( pub fn create_service_token( did: &Did, - aud: &Did, + aud: &DidRef, lxm: Option<&Nsid>, key_bytes: &[u8], ) -> Result { diff --git a/crates/tranquil-pds/src/api/proxy.rs b/crates/tranquil-pds/src/api/proxy.rs index c51a2e3..5d5c638 100644 --- a/crates/tranquil-pds/src/api/proxy.rs +++ b/crates/tranquil-pds/src/api/proxy.rs @@ -5,7 +5,7 @@ use std::sync::LazyLock; use crate::api::error::ApiError; use crate::api::proxy_client::proxy_client; use crate::state::AppState; -use crate::types::{Did, Nsid}; +use crate::types::{Did, DidRef, Nsid}; use crate::util::get_header_str; use axum::{ body::Bytes, @@ -361,7 +361,7 @@ async fn proxy_handler( match crate::auth::create_service_token( &auth_user.did, - &token_aud, + &DidRef::from(&token_aud), Some(&token_lxm), &key_bytes, ) { diff --git a/crates/tranquil-pds/tests/jwt_security.rs b/crates/tranquil-pds/tests/jwt_security.rs index e462df3..c899b95 100644 --- a/crates/tranquil-pds/tests/jwt_security.rs +++ b/crates/tranquil-pds/tests/jwt_security.rs @@ -14,7 +14,7 @@ use tranquil_pds::auth::{ get_did_from_token, get_jti_from_token, verify_access_token, verify_refresh_token, verify_token, }; -use tranquil_types::{Did, Nsid}; +use tranquil_types::{Did, DidRef, Nsid}; fn generate_user_key() -> Vec { let secret_key = SecretKey::random(&mut OsRng); @@ -169,7 +169,7 @@ fn test_token_type_confusion() { let service_token = create_service_token( &did, - &Did::new("did:web:nel.pet").expect("valid DID"), + &DidRef::new("did:web:nel.pet").expect("valid DID reference"), Some(&Nsid::new("cafe.oyster.method").expect("valid NSID")), &key_bytes, ) diff --git a/crates/tranquil-pds/tests/oauth.rs b/crates/tranquil-pds/tests/oauth.rs index 284ac81..2a05cd3 100644 --- a/crates/tranquil-pds/tests/oauth.rs +++ b/crates/tranquil-pds/tests/oauth.rs @@ -1270,6 +1270,52 @@ async fn test_granular_scope_rpc_specific_method() { ); } +#[tokio::test] +async fn test_granular_scope_rpc_aud_with_service_id() { + let url = base_url().await; + let http_client = client(); + let (token, _, _) = + get_oauth_token_with_scope("rpc:app.bsky.feed.getTimeline?aud=did:web:api.bsky.app").await; + let allowed_res = http_client + .get(format!("{}/xrpc/com.atproto.server.getServiceAuth", url)) + .bearer_auth(&token) + .query(&[ + ("aud", "did:web:api.bsky.app#bsky_appview"), + ("lxm", "app.bsky.feed.getTimeline"), + ]) + .send() + .await + .unwrap(); + assert_eq!( + allowed_res.status(), + StatusCode::OK, + "A scope granted for a service must cover a request naming one of its service ids" + ); + let body: Value = allowed_res.json().await.unwrap(); + let service_token = body["token"].as_str().unwrap(); + let payload = service_token.split('.').nth(1).unwrap(); + let claims: Value = serde_json::from_slice(&URL_SAFE_NO_PAD.decode(payload).unwrap()).unwrap(); + assert_eq!( + claims["aud"], "did:web:api.bsky.app#bsky_appview", + "the service id must reach the signed claim even on the granular scope path" + ); + let blocked_res = http_client + .get(format!("{}/xrpc/com.atproto.server.getServiceAuth", url)) + .bearer_auth(&token) + .query(&[ + ("aud", "did:web:other.example#bsky_appview"), + ("lxm", "app.bsky.feed.getTimeline"), + ]) + .send() + .await + .unwrap(); + assert_eq!( + blocked_res.status(), + StatusCode::FORBIDDEN, + "A service id must not smuggle in a different audience" + ); +} + #[tokio::test] async fn test_oauth_metadata_includes_prompt_values_supported() { let url = base_url().await; diff --git a/crates/tranquil-pds/tests/scope_edge_cases.rs b/crates/tranquil-pds/tests/scope_edge_cases.rs index bdca3bb..9cffed4 100644 --- a/crates/tranquil-pds/tests/scope_edge_cases.rs +++ b/crates/tranquil-pds/tests/scope_edge_cases.rs @@ -181,6 +181,38 @@ fn test_permissions_rpc_lxm_wildcard_prefix() { assert!(!perms.allows_rpc("did:web:api.bsky.app", &c("app.bsky.actor.getProfile"))); } +#[test] +fn test_permissions_rpc_aud_service_id_is_normalized() { + let perms = + ScopePermissions::from_scope_string(Some("rpc:app.bsky.feed.*?aud=did:web:api.bsky.app")); + assert!( + perms.allows_rpc( + "did:web:api.bsky.app#bsky_appview", + &c("app.bsky.feed.getTimeline") + ), + "a scope granted for a service must cover a request naming one of its service ids" + ); + assert!( + perms.allows_rpc("did:web:api.bsky.app", &c("app.bsky.feed.getTimeline")), + "the bare form must keep working" + ); + assert!( + !perms.allows_rpc( + "did:web:other.example#bsky_appview", + &c("app.bsky.feed.getTimeline") + ), + "a service id must not smuggle in a different audience" + ); + + let fragment_scope = ScopePermissions::from_scope_string(Some( + "rpc:app.bsky.feed.*?aud=did:web:api.bsky.app%23bsky_appview", + )); + assert!( + fragment_scope.allows_rpc("did:web:api.bsky.app", &c("app.bsky.feed.getTimeline")), + "a scope granted with a service id must still cover the bare audience" + ); +} + #[test] fn test_delegation_intersect_mismatched_params_empty() { let result = intersect_scopes("repo:*?action=create", "repo:*?action=delete"); diff --git a/crates/tranquil-pds/tests/server.rs b/crates/tranquil-pds/tests/server.rs index 6cd861a..4951705 100644 --- a/crates/tranquil-pds/tests/server.rs +++ b/crates/tranquil-pds/tests/server.rs @@ -132,6 +132,46 @@ async fn test_service_auth() { let lxm_payload = URL_SAFE_NO_PAD.decode(lxm_parts[1]).unwrap(); let lxm_claims: Value = serde_json::from_slice(&lxm_payload).unwrap(); assert_eq!(lxm_claims["lxm"], "com.atproto.repo.getRecord"); + let fragment_res = client + .get(format!("{}/xrpc/com.atproto.server.getServiceAuth", base)) + .bearer_auth(&access_jwt) + .query(&[ + ("aud", "did:web:example.com#colibri_appview"), + ("lxm", "com.atproto.repo.getRecord"), + ]) + .send() + .await + .unwrap(); + assert_eq!(fragment_res.status(), StatusCode::OK); + let fragment_body: Value = fragment_res.json().await.unwrap(); + let fragment_token = fragment_body["token"].as_str().unwrap(); + let fragment_parts: Vec<&str> = fragment_token.split('.').collect(); + let fragment_payload = URL_SAFE_NO_PAD.decode(fragment_parts[1]).unwrap(); + let fragment_claims: Value = serde_json::from_slice(&fragment_payload).unwrap(); + assert_eq!( + fragment_claims["aud"], "did:web:example.com#colibri_appview", + "the service id must survive into the signed claim so the receiver can match it \ + against its own DID document" + ); + + let empty_fragment = client + .get(format!("{}/xrpc/com.atproto.server.getServiceAuth", base)) + .bearer_auth(&access_jwt) + .query(&[("aud", "did:web:example.com#")]) + .send() + .await + .unwrap(); + assert_eq!(empty_fragment.status(), StatusCode::BAD_REQUEST); + + let double_fragment = client + .get(format!("{}/xrpc/com.atproto.server.getServiceAuth", base)) + .bearer_auth(&access_jwt) + .query(&[("aud", "did:web:example.com#a#b")]) + .send() + .await + .unwrap(); + assert_eq!(double_fragment.status(), StatusCode::BAD_REQUEST); + let unauth = client .get(format!("{}/xrpc/com.atproto.server.getServiceAuth", base)) .query(&[("aud", "did:web:example.com")]) diff --git a/crates/tranquil-types/src/lib.rs b/crates/tranquil-types/src/lib.rs index 6fe70a8..eea83df 100644 --- a/crates/tranquil-types/src/lib.rs +++ b/crates/tranquil-types/src/lib.rs @@ -225,6 +225,54 @@ impl Did { } } +const DID_REF_MAX_LEN: usize = 2048; + +fn is_service_id(s: &str) -> bool { + !s.is_empty() + && !s + .chars() + .any(|c| c.is_whitespace() || c.is_control() || matches!(c, '#' | '/' | '?')) +} + +validated_string_newtype! { + pub struct DidRef; + error = DidRefError; + label = "DID reference"; + validator = |s| { + if s.len() > DID_REF_MAX_LEN { + return Err(()); + } + match s.split_once('#') { + None => jacquard_common::types::string::Did::new(s) + .map(|v| v.as_str().to_owned()) + .map_err(|_| ()), + Some((did, service_id)) => { + if !is_service_id(service_id) { + return Err(()); + } + let base = jacquard_common::types::string::Did::new(did).map_err(|_| ())?; + Ok(format!("{}#{}", base.as_str(), service_id)) + } + } + }; +} + +impl DidRef { + pub fn did(&self) -> &str { + self.0.split('#').next().unwrap_or(&self.0) + } + + pub fn service_id(&self) -> Option<&str> { + self.0.split_once('#').map(|(_, service_id)| service_id) + } +} + +impl From<&Did> for DidRef { + fn from(did: &Did) -> Self { + Self(did.0.clone()) + } +} + #[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash, Serialize, sqlx::Type)] #[serde(transparent)] #[sqlx(transparent)] @@ -1588,6 +1636,97 @@ mod validated_newtype_tests { ); } + #[test] + fn a_bare_did_ref_names_no_service() { + let aud = DidRef::new("did:plc:abc").unwrap(); + assert_eq!(aud.as_str(), "did:plc:abc"); + assert_eq!(aud.did(), "did:plc:abc"); + assert_eq!( + aud.service_id(), + None, + "an absent fragment is not the same as an empty one" + ); + } + + #[test] + fn a_did_ref_keeps_the_service_id_it_was_given() { + let aud = DidRef::new("did:web:api.colibri.social#colibri_appview").unwrap(); + assert_eq!( + aud.as_str(), + "did:web:api.colibri.social#colibri_appview", + "the fragment is what tells the receiver which of its services was audienced, \ + so it must survive entirely" + ); + assert_eq!(aud.did(), "did:web:api.colibri.social"); + assert_eq!(aud.service_id(), Some("colibri_appview")); + } + + #[test] + fn a_did_ref_normalizes_its_did_half_the_way_a_did_does() { + assert_eq!( + DidRef::new("at://did:plc:abc#colibri_appview") + .unwrap() + .as_str(), + "did:plc:abc#colibri_appview" + ); + assert_eq!( + DidRef::new("did:plc:def").unwrap().as_str(), + Did::new("did:plc:def").unwrap().as_str(), + "a fragmentless DidRef must be byte-identical to the Did it replaces" + ); + } + + #[test] + fn a_did_ref_rejects_anything_that_cannot_name_one_service() { + for bad in [ + "did:web:oyster.cafe#", + "did:web:oyster.cafe#a#b", + "did:web:oyster.cafe# whelk", + "did:web:oyster.cafe#a/b", + "did:web:oyster.cafe#a?b", + "not-a-did#colibri_appview", + "#colibri_appview", + ] { + assert!( + DidRef::new(bad).is_err(), + "{bad} should not parse as a DID reference" + ); + } + } + + #[test] + fn a_did_ref_does_not_second_guess_the_service_ids_it_has_not_seen() { + for good in [ + "did:web:oyster.cafe#atproto_pds", + "did:web:oyster.cafe#atproto_labeler", + "did:plc:abc#bsky_chat", + "did:web:oyster.cafe#whelk.v2", + ] { + assert!( + DidRef::new(good).is_ok(), + "{good} names a service the receiver resolves in its own DID document, \ + so rejecting it here would recreate the bug this type exists to fix" + ); + } + } + + #[test] + fn an_over_long_did_ref_is_rejected() { + let long = format!("did:web:{}#def", "a".repeat(2048)); + assert!( + DidRef::new(&long).is_err(), + "the lexicon bounds aud at 2048 bytes" + ); + } + + #[test] + fn a_did_ref_built_from_a_did_names_no_service() { + let did = Did::new("did:plc:def").unwrap(); + let aud = DidRef::from(&did); + assert_eq!(aud.as_str(), did.as_str()); + assert_eq!(aud.service_id(), None); + } + #[test] fn the_earliest_tid_sorts_below_every_generated_tid() { let earliest = Tid::earliest();