fix: Address review comments

crates/tranquil-auth/src/compress.rs

@@ -7,7 +7,7 @@ const COMPRESSED_PREFIX: &str = "$br$";
 const QUALITY: u32 = 9;
 const WINDOW_BITS: u32 = 16;
 const BUFFER_SIZE: usize = 4096;
-const MAX_DECOMPRESSED_LEN: u64 = 64 * 1024;
+const MAX_SCOPE_LEN: u64 = 64 * 1024;
 
 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
 pub enum ScopeDecodeError {
@@ -28,6 +28,21 @@ impl fmt::Display for ScopeDecodeError {
 
 impl std::error::Error for ScopeDecodeError {}
 
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub enum ScopeEncodeError {
+    TooLarge,
+}
+
+impl fmt::Display for ScopeEncodeError {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        match self {
+            Self::TooLarge => write!(f, "Scope exceeds maximum length"),
+        }
+    }
+}
+
+impl std::error::Error for ScopeEncodeError {}
+
 fn brotli_compress(input: &str) -> Vec<u8> {
     let mut writer = CompressorWriter::new(Vec::new(), BUFFER_SIZE, QUALITY, WINDOW_BITS);
 
@@ -42,24 +57,28 @@ fn brotli_decompress(input: &[u8]) -> Result<String, ScopeDecodeError> {
     let mut output = String::new();
 
     Decompressor::new(input, BUFFER_SIZE)
-        .take(MAX_DECOMPRESSED_LEN + 1)
+        .take(MAX_SCOPE_LEN + 1)
         .read_to_string(&mut output)
         .map_err(|_| ScopeDecodeError::DecompressFailed)?;
 
-    if output.len() as u64 > MAX_DECOMPRESSED_LEN {
+    if output.len() as u64 > MAX_SCOPE_LEN {
         return Err(ScopeDecodeError::TooLarge);
     }
 
     Ok(output)
 }
 
-pub fn encode_scope(scope: &str) -> String {
+pub fn encode_scope(scope: &str) -> Result<String, ScopeEncodeError> {
+    if scope.len() as u64 > MAX_SCOPE_LEN {
+        return Err(ScopeEncodeError::TooLarge);
+    }
+
     let encoded = URL_SAFE_NO_PAD.encode(brotli_compress(scope));
 
     if COMPRESSED_PREFIX.len() + encoded.len() < scope.len() {
-        format!("{COMPRESSED_PREFIX}{encoded}")
+        Ok(format!("{COMPRESSED_PREFIX}{encoded}"))
     } else {
-        scope.to_owned()
+        Ok(scope.to_owned())
     }
 }
 
@@ -96,7 +115,7 @@ mod tests {
     #[test]
     fn long_scope_roundtrips_through_compression() {
         let scope = long_scope();
-        let encoded = encode_scope(&scope);
+        let encoded = encode_scope(&scope).unwrap();
 
         assert!(encoded.starts_with(COMPRESSED_PREFIX));
         assert!(encoded.len() < scope.len());
@@ -105,7 +124,7 @@ mod tests {
 
     #[test]
     fn short_scope_stays_plaintext() {
-        let encoded = encode_scope("com.atproto.access");
+        let encoded = encode_scope("com.atproto.access").unwrap();
 
         assert_eq!(encoded, "com.atproto.access");
         assert_eq!(decode_scope(&encoded).unwrap(), "com.atproto.access");
@@ -134,12 +153,19 @@ mod tests {
 
     #[test]
     fn compression_bomb_is_rejected() {
-        let bomb = encode_scope(&"a".repeat(MAX_DECOMPRESSED_LEN as usize * 2));
-        let encoded = bomb.strip_prefix(COMPRESSED_PREFIX).unwrap_or(&bomb);
+        let bomb = URL_SAFE_NO_PAD.encode(brotli_compress(&"a".repeat(MAX_SCOPE_LEN as usize * 2)));
 
         assert_eq!(
-            decode_scope(&format!("{COMPRESSED_PREFIX}{encoded}")),
+            decode_scope(&format!("{COMPRESSED_PREFIX}{bomb}")),
             Err(ScopeDecodeError::TooLarge)
         );
     }
+
+    #[test]
+    fn encode_rejects_oversized_scope() {
+        let oversized = "a".repeat(MAX_SCOPE_LEN as usize + 1);
+
+        assert_eq!(encode_scope(&oversized), Err(ScopeEncodeError::TooLarge));
+        assert!(encode_scope(&"a".repeat(MAX_SCOPE_LEN as usize)).is_ok());
+    }
 }

crates/tranquil-auth/src/lib.rs

@@ -13,7 +13,7 @@ pub use token::{
     create_service_token_hs256,
 };
 
-pub use compress::{ScopeDecodeError, decode_scope, encode_scope};
+pub use compress::{ScopeDecodeError, ScopeEncodeError, decode_scope, encode_scope};
 
 pub use totp::{
     TotpError, decrypt_totp_secret, encrypt_totp_secret, generate_backup_codes,

crates/tranquil-auth/src/token.rs

@@ -3,7 +3,7 @@ use crate::compress::encode_scope;
 use super::types::{
     ActClaim, Claims, Header, SigningAlgorithm, TokenScope, TokenType, TokenWithMetadata,
 };
-use anyhow::Result;
+use anyhow::{Context, Result};
 use base64::Engine as _;
 use base64::engine::general_purpose::URL_SAFE_NO_PAD;
 use chrono::{DateTime, Duration, Utc};
@@ -207,7 +207,7 @@ fn create_signed_token_pinned(
         aud: format!("did:web:{}", aud_hostname),
         exp: expiration,
         iat: Utc::now().timestamp(),
-        scope: Some(encode_scope(scope)),
+        scope: Some(encode_scope(scope).context("Scope too large to encode")?),
         lxm: None,
         jti: jti.clone(),
         act,
@@ -330,7 +330,7 @@ fn create_hs256_token_with_metadata(
         ),
         exp: expiration,
         iat: Utc::now().timestamp(),
-        scope: Some(encode_scope(scope)),
+        scope: Some(encode_scope(scope).context("Scope too large to encode")?),
         lxm: None,
         jti: jti.clone(),
         act: None,

crates/tranquil-oauth-server/src/endpoints/token/helpers.rs

@@ -43,7 +43,8 @@ pub fn create_access_token_with_delegation(
     let issuer = format!("https://{}", pds_hostname);
     let now = Utc::now().timestamp();
     let exp = now + ACCESS_TOKEN_EXPIRY_SECONDS;
-    let actual_scope = scope.unwrap_or("atproto");
+    let actual_scope = tranquil_pds::auth::encode_scope(scope.unwrap_or("atproto"))
+        .map_err(|_| OAuthError::InvalidScope("Scope too large".to_string()))?;
     let mut payload = json!({
         "iss": issuer,
         "sub": sub.as_str(),

crates/tranquil-pds/src/auth/mod.rs

@@ -43,14 +43,15 @@ pub use scope_verified::{
 pub use service::{ServiceTokenClaims, ServiceTokenError, ServiceTokenVerifier, is_service_token};
 
 pub use tranquil_auth::{
-    ActClaim, Claims, Header, SigningAlgorithm, TokenData, TokenDecodeError, TokenScope, TokenType,
-    TokenVerifyError, TokenWithMetadata, TotpError, UnsafeClaims, create_access_token,
-    create_access_token_hs256, create_access_token_hs256_with_metadata,
-    create_access_token_with_delegation, create_access_token_with_jti,
-    create_access_token_with_metadata, create_access_token_with_scope_metadata,
-    create_refresh_token, create_refresh_token_hs256, create_refresh_token_hs256_with_metadata,
-    create_refresh_token_with_jti, create_refresh_token_with_metadata, create_service_token,
-    create_service_token_hs256, generate_backup_codes, generate_qr_png_base64,
+    ActClaim, Claims, Header, ScopeDecodeError, ScopeEncodeError, SigningAlgorithm, TokenData,
+    TokenDecodeError, TokenScope, TokenType, TokenVerifyError, TokenWithMetadata, TotpError,
+    UnsafeClaims, create_access_token, create_access_token_hs256,
+    create_access_token_hs256_with_metadata, create_access_token_with_delegation,
+    create_access_token_with_jti, create_access_token_with_metadata,
+    create_access_token_with_scope_metadata, create_refresh_token, create_refresh_token_hs256,
+    create_refresh_token_hs256_with_metadata, create_refresh_token_with_jti,
+    create_refresh_token_with_metadata, create_service_token, create_service_token_hs256,
+    decode_scope, encode_scope, generate_backup_codes, generate_qr_png_base64,
     generate_totp_secret, generate_totp_uri, get_algorithm_from_token, get_did_from_token,
     get_jti_from_token, hash_backup_code, is_backup_code_format, verify_access_token,
     verify_access_token_hs256, verify_backup_code, verify_refresh_token,

crates/tranquil-pds/src/oauth/verify.rs

@@ -164,7 +164,9 @@ pub fn extract_oauth_token_info(token: &str) -> Result<OAuthTokenInfo, OAuthErro
     let scope = payload
         .get("scope")
         .and_then(|s| s.as_str())
-        .map(|s| s.to_string());
+        .map(crate::auth::decode_scope)
+        .transpose()
+        .map_err(|_| OAuthError::InvalidToken("Invalid scope claim encoding".to_string()))?;
     let controller_did = payload
         .get("act")
         .and_then(|a| a.get("sub"))

crates/tranquil-pds/tests/oauth_permission_sets.rs

@@ -505,6 +505,129 @@ async fn test_enforcement_uses_expanded_jwt_scope() {
     );
 }
 
+#[tokio::test]
+async fn test_long_expanded_scope_is_compressed_in_jwt() {
+    const BIG_SET_NSID: &str = "io.atcr.authBigApp";
+    let collections = [
+        "io.atcr.manifest",
+        "io.atcr.sailor.star",
+        "io.atcr.tag",
+        "io.atcr.blueprint",
+        "io.atcr.artifact",
+        "io.atcr.channel.read",
+    ];
+    let granular_scope = collections
+        .iter()
+        .map(|coll| format!("repo:{}?action=create&action=delete", coll))
+        .collect::<Vec<_>>()
+        .join(" ");
+    seed_permission_set(BIG_SET_NSID, &granular_scope).await;
+
+    let scope = format!("atproto include:{}", BIG_SET_NSID);
+    let (session, _consent_body, _mock) = create_delegated_session_with_scope(
+        "psc",
+        "https://example.com/permset-compress-callback",
+        &scope,
+    )
+    .await;
+
+    let payload = decode_jwt_payload(&session.access_token);
+    let jwt_scope = payload["scope"]
+        .as_str()
+        .expect("access token JWT should have a scope claim");
+    assert!(
+        jwt_scope.starts_with("$br$"),
+        "an expanded scope this long should be compressed in the JWT claim, got: {}",
+        jwt_scope
+    );
+
+    let decoded = tranquil_pds::auth::decode_scope(jwt_scope).expect("scope claim should decode");
+    for coll in collections {
+        assert!(
+            decoded.contains(&format!("repo:{}?action=create", coll)),
+            "decoded scope should carry {}, got: {}",
+            coll,
+            decoded
+        );
+    }
+    assert!(
+        !decoded.contains("include:"),
+        "decoded scope should not contain the raw include: token, got: {}",
+        decoded
+    );
+
+    let url = base_url().await;
+    let http_client = client();
+
+    let introspect_res = http_client
+        .post(format!("{}/oauth/introspect", url))
+        .form(&[("token", session.access_token.as_str())])
+        .send()
+        .await
+        .expect("introspect request failed");
+    assert_eq!(introspect_res.status(), StatusCode::OK);
+    let introspect_body: Value = introspect_res.json().await.unwrap();
+    let introspect_scope = introspect_body["scope"]
+        .as_str()
+        .expect("introspect response should have a scope string");
+    assert_eq!(
+        introspect_scope, decoded,
+        "introspect should report the decoded scope"
+    );
+
+    let collection = collections[0];
+    let create_res = http_client
+        .post(format!("{}/xrpc/com.atproto.repo.createRecord", url))
+        .bearer_auth(&session.access_token)
+        .json(&json!({
+            "repo": session.delegated_did,
+            "collection": collection,
+            "validate": false,
+            "record": {
+                "$type": collection,
+                "note": "compressed scope enforcement test",
+                "createdAt": Utc::now().to_rfc3339()
+            }
+        }))
+        .send()
+        .await
+        .expect("createRecord request failed");
+    assert_ne!(
+        create_res.status(),
+        StatusCode::FORBIDDEN,
+        "a compressed scope claim must still authorize the collections it covers. Got body: {:?}",
+        create_res.text().await
+    );
+
+    let refresh_res = http_client
+        .post(format!("{}/oauth/token", url))
+        .form(&[
+            ("grant_type", "refresh_token"),
+            ("refresh_token", session.refresh_token.as_str()),
+            ("client_id", session.client_id.as_str()),
+        ])
+        .send()
+        .await
+        .expect("Refresh request failed");
+    assert_eq!(refresh_res.status(), StatusCode::OK);
+    let refresh_body: Value = refresh_res.json().await.unwrap();
+    let refreshed_token = refresh_body["access_token"].as_str().unwrap();
+    let refreshed_claim = decode_jwt_payload(refreshed_token)["scope"]
+        .as_str()
+        .expect("refreshed JWT should have a scope claim")
+        .to_string();
+    assert!(
+        refreshed_claim.starts_with("$br$"),
+        "refreshed claim should also be compressed, got: {}",
+        refreshed_claim
+    );
+    assert_eq!(
+        tranquil_pds::auth::decode_scope(&refreshed_claim).expect("refreshed scope should decode"),
+        decoded,
+        "refresh must yield a byte-identical decoded scope"
+    );
+}
+
 #[tokio::test]
 async fn test_consent_post_errors_when_set_unresolvable() {
     const UNRESOLVABLE_NSID: &str = "io.atcr.authUnresolvableSet";

crates/tranquil-scopes/src/permission_set.rs

@@ -2,7 +2,7 @@ use hickory_resolver::TokioAsyncResolver;
 use hickory_resolver::config::{ResolverConfig, ResolverOpts};
 use reqwest::Client;
 use serde::{Deserialize, Serialize};
-use std::collections::HashMap;
+use std::collections::{BTreeMap, HashMap};
 use tracing::debug;
 use tranquil_types::{Did, Nsid};
 
@@ -334,13 +334,20 @@ fn is_under_authority(target_nsid: &str, authority: &str) -> bool {
 
 const DEFAULT_ACTIONS: &[&str] = &["create", "update", "delete"];
 
+fn action_rank(action: &str) -> usize {
+    DEFAULT_ACTIONS
+        .iter()
+        .position(|known| *known == action)
+        .unwrap_or(DEFAULT_ACTIONS.len())
+}
+
 fn build_expanded_scopes(
     permissions: &[PermissionEntry],
     default_aud: Option<&str>,
     namespace_authority: &str,
 ) -> String {
     // Key is `repo`, value is array of actions
-    let mut ungrouped_repo_scopes: HashMap<String, Vec<String>> = HashMap::new();
+    let mut ungrouped_repo_scopes: BTreeMap<String, Vec<String>> = BTreeMap::new();
     let mut rpc_scopes: Vec<String> = Vec::new();
 
     permissions
@@ -354,21 +361,21 @@ fn build_expanded_scopes(
                         .map(|a| a.iter().map(String::as_str).collect())
                         .unwrap_or_else(|| DEFAULT_ACTIONS.to_vec());
 
-                    collections
-                        .iter()
-                        .filter(|coll| is_under_authority(coll, namespace_authority))
-                        .for_each(|coll| {
-                            actions.iter().for_each(|action| {
-                                let existing = ungrouped_repo_scopes.get_mut(coll);
+                    if !actions.is_empty() {
+                        collections
+                            .iter()
+                            .filter(|coll| is_under_authority(coll, namespace_authority))
+                            .for_each(|coll| {
+                                let existing =
+                                    ungrouped_repo_scopes.entry(coll.to_string()).or_default();
 
-                                if existing.is_none() {
-                                    ungrouped_repo_scopes
-                                        .insert(coll.to_string(), vec![action.to_string()]);
-                                } else {
-                                    existing.unwrap().push(action.to_string());
-                                }
+                                actions.iter().for_each(|action| {
+                                    if !existing.iter().any(|seen| seen == action) {
+                                        existing.push(action.to_string());
+                                    }
+                                });
                             });
-                        });
+                    }
                 }
             }
             "rpc" => {
@@ -383,7 +390,9 @@ fn build_expanded_scopes(
                                 None => format!("rpc:{}", lxm),
                             };
 
-                            rpc_scopes.push(scope);
+                            if !rpc_scopes.contains(&scope) {
+                                rpc_scopes.push(scope);
+                            }
                         });
                 }
             }
@@ -392,7 +401,12 @@ fn build_expanded_scopes(
 
     let grouped_repo_scopes: Vec<String> = ungrouped_repo_scopes
         .iter()
-        .map(|(repo, actions)| format!("repo:{}?action={}", repo, actions.join("&action=")))
+        .map(|(repo, actions)| {
+            let mut actions = actions.clone();
+            actions.sort_by(|a, b| action_rank(a).cmp(&action_rank(b)).then_with(|| a.cmp(b)));
+
+            format!("repo:{}?action={}", repo, actions.join("&action="))
+        })
         .collect();
 
     let combined_repo_scopes = grouped_repo_scopes.join(" ");
@@ -479,9 +493,11 @@ mod tests {
         }];
 
         let expanded = build_expanded_scopes(&permissions, None, "io.atcr");
-        assert!(expanded.contains("repo:io.atcr.manifest?action=create&action=delete"));
-        assert!(expanded.contains("repo:io.atcr.sailor.star?action=create&action=delete"));
-        assert!(!expanded.contains("app.bsky.feed.post"));
+        assert_eq!(
+            expanded,
+            "repo:io.atcr.manifest?action=create&action=delete \
+             repo:io.atcr.sailor.star?action=create&action=delete"
+        );
     }
 
     #[test]
@@ -495,10 +511,121 @@ mod tests {
         }];
 
         let expanded = build_expanded_scopes(&permissions, None, "io.atcr");
-        assert!(expanded.contains("repo:io.atcr.manifest?action="));
-        assert!(expanded.contains("action=create"));
-        assert!(expanded.contains("action=update"));
-        assert!(expanded.contains("action=delete"));
+        assert_eq!(
+            expanded,
+            "repo:io.atcr.manifest?action=create&action=update&action=delete"
+        );
+    }
+
+    #[test]
+    fn test_build_expanded_scopes_repo_omitted_action_grants_all() {
+        let permissions = vec![PermissionEntry {
+            resource: "repo".to_string(),
+            action: None,
+            collection: Some(vec!["io.atcr.manifest".to_string()]),
+            lxm: None,
+            aud: None,
+        }];
+
+        let expanded = build_expanded_scopes(&permissions, None, "io.atcr");
+        assert_eq!(
+            expanded, "repo:io.atcr.manifest?action=create&action=update&action=delete",
+            "an omitted action list means all actions"
+        );
+    }
+
+    #[test]
+    fn test_build_expanded_scopes_repo_empty_action_list_skips_entry() {
+        let permissions = vec![PermissionEntry {
+            resource: "repo".to_string(),
+            action: Some(vec![]),
+            collection: Some(vec!["io.atcr.manifest".to_string()]),
+            lxm: None,
+            aud: None,
+        }];
+
+        let expanded = build_expanded_scopes(&permissions, None, "io.atcr");
+        assert!(
+            expanded.is_empty(),
+            "an explicitly empty action list is invalid, so the entry is skipped rather \
+             than expanded to all actions or emitted as a bare `?action=`, got: {expanded}"
+        );
+    }
+
+    #[test]
+    fn test_build_expanded_scopes_is_deterministic() {
+        let permissions = vec![
+            PermissionEntry {
+                resource: "repo".to_string(),
+                action: Some(vec!["create".to_string()]),
+                collection: Some(vec![
+                    "io.atcr.sailor.star".to_string(),
+                    "io.atcr.manifest".to_string(),
+                    "io.atcr.blob".to_string(),
+                ]),
+                lxm: None,
+                aud: None,
+            },
+            PermissionEntry {
+                resource: "rpc".to_string(),
+                action: None,
+                collection: None,
+                lxm: Some(vec![
+                    "io.atcr.getManifest".to_string(),
+                    "io.atcr.listTags".to_string(),
+                ]),
+                aud: Some("*".to_string()),
+            },
+        ];
+
+        let first = build_expanded_scopes(&permissions, None, "io.atcr");
+        assert_eq!(
+            first,
+            "repo:io.atcr.blob?action=create repo:io.atcr.manifest?action=create \
+             repo:io.atcr.sailor.star?action=create \
+             rpc:io.atcr.getManifest?aud=* rpc:io.atcr.listTags?aud=*"
+        );
+
+        for _ in 0..16 {
+            assert_eq!(build_expanded_scopes(&permissions, None, "io.atcr"), first);
+        }
+    }
+
+    #[test]
+    fn test_build_expanded_scopes_dedupes_and_canonicalizes_actions() {
+        let permissions = vec![
+            PermissionEntry {
+                resource: "repo".to_string(),
+                action: Some(vec!["delete".to_string(), "create".to_string()]),
+                collection: Some(vec!["io.atcr.manifest".to_string()]),
+                lxm: None,
+                aud: None,
+            },
+            PermissionEntry {
+                resource: "repo".to_string(),
+                action: Some(vec!["create".to_string(), "update".to_string()]),
+                collection: Some(vec!["io.atcr.manifest".to_string()]),
+                lxm: None,
+                aud: None,
+            },
+            PermissionEntry {
+                resource: "rpc".to_string(),
+                action: None,
+                collection: None,
+                lxm: Some(vec![
+                    "io.atcr.getManifest".to_string(),
+                    "io.atcr.getManifest".to_string(),
+                ]),
+                aud: Some("*".to_string()),
+            },
+        ];
+
+        let expanded = build_expanded_scopes(&permissions, None, "io.atcr");
+        assert_eq!(
+            expanded,
+            "repo:io.atcr.manifest?action=create&action=update&action=delete \
+             rpc:io.atcr.getManifest?aud=*"
+        );
     }
 
     #[test]