close
atproto made easy crates.io/crates/jacquard
atproto rust
136

Configure Feed

Select the types of activity you want to include in your feed.

oauth: parse repo scopes that name collections with the collection parameter #15

Open opened by okami.mom targeting main

[code generated by opus 5.5, reviewed by me, i found the tests maybe excessive but not useless]

Scopes::new and Scope::parse reject valid repo scopes that name their collection with the collection query parameter instead of positionally. my project sister-radio failed to start with these scopes, which @atcute/oauth-types generates:

  • repo?collection=fm.teal.feed.play&collection=app.bsky.feed.post&action=create
  • repo?collection=fm.teal.actor.status&action=create&action=update

the error was invalid OAuth scopes: Parse error: error in ``: regex failure - invalid. the parser treated the empty text before "?" as the collection and failed NSID validation, and never read the collection parameter.

the patch reads the collection either from after the colon or from the collection query parameters. it also rejects no collection at all and ones that give the collection both after the colon and as a parameter, like repo:a?collection=b per spec https://github.com/bluesky-social/atproto-website/blob/main/src/app/%5Blocale%5D/specs/permission/en.mdx#repo

Labels

None yet.

assignee

None yet.

Participants 1
AT URI
at://did:plc:3rwz3xfw2crswgifqgc3g7zh/sh.tangled.repo.pull/3mwdlad6ft522
+249 -95
Diff #0
+249 -95
crates/jacquard-oauth/src/scopes.rs
··· 1293 1293 let start = pos as u16; 1294 1294 let end = start + token.len() as u16; 1295 1295 1296 - let inner = parse_scope_indices(token, start)?; 1297 - indices.push(ScopeIndices { start, end, inner }); 1296 + // A token usually yields one scope, but `repo?collection=a&collection=b` 1297 + // yields one per collection; each shares the token's byte range. 1298 + for inner in parse_scope_indices(token, start)? { 1299 + indices.push(ScopeIndices { start, end, inner }); 1300 + } 1298 1301 1299 1302 pos = end as usize + 1; // +1 for the space delimiter. 1300 1303 } ··· 1655 1658 /// `base` is the byte offset of `token` within the outer buffer. 1656 1659 /// All `(u16, u16)` ranges in the returned indices are absolute 1657 1660 /// offsets into the outer buffer, NOT relative to `token`. 1658 - fn parse_scope_indices(token: &str, base: u16) -> Result<ScopeInnerIndices, ParseError> { 1661 + /// 1662 + /// Returns one entry per scope the token denotes. That is exactly one for 1663 + /// every resource except `repo` in query form, which yields one entry per 1664 + /// `collection` parameter, matching how permission sets expand. 1665 + fn parse_scope_indices( 1666 + token: &str, 1667 + base: u16, 1668 + ) -> Result<SmallVec<[ScopeInnerIndices; 1]>, ParseError> { 1659 1669 // Determine the prefix by checking for known prefixes. 1660 1670 let prefixes = [ 1661 1671 "account", ··· 1695 1705 ParseError::UnknownPrefix(token[..token.find(':').unwrap_or(token.len())].to_smolstr()) 1696 1706 })?; 1697 1707 1698 - match prefix { 1708 + let single = match prefix { 1709 + "repo" => return parse_repo_indices(token, suffix, base), 1699 1710 "account" => parse_account_indices(suffix), 1700 1711 "identity" => parse_identity_indices(suffix), 1701 1712 "blob" => parse_blob_indices(token, suffix, base), 1702 - "repo" => parse_repo_indices(token, suffix, base), 1703 1713 "rpc" => parse_rpc_indices(token, suffix, base), 1704 1714 "space" => parse_space_indices(token, suffix, base), 1705 1715 "atproto" => parse_atproto_indices(suffix), ··· 1709 1719 "profile" => parse_profile_indices(suffix), 1710 1720 "email" => parse_email_indices(suffix), 1711 1721 _ => Err(ParseError::UnknownPrefix(prefix.to_smolstr())), 1712 - } 1722 + }?; 1723 + let mut parsed = SmallVec::new(); 1724 + parsed.push(single); 1725 + Ok(parsed) 1713 1726 } 1714 1727 1715 1728 /// Parse account scope indices. ··· 1856 1869 Ok(ScopeInnerIndices::Blob { accept }) 1857 1870 } 1858 1871 1859 - /// Parse repo scope indices, storing byte range of collection NSID if present. 1860 - fn parse_repo_indices( 1861 - token: &str, 1872 + /// Split a `repo` scope suffix into its collection values and its `action` 1873 + /// parameter values. 1874 + /// 1875 + /// See <https://github.com/bluesky-social/atproto-website/blob/main/src/app/%5Blocale%5D/specs/permission/en.mdx#repo>. 1876 + fn split_repo_suffix( 1862 1877 suffix: Option<&str>, 1863 - base: u16, 1864 - ) -> Result<ScopeInnerIndices, ParseError> { 1865 - let (collection_str, params) = match suffix { 1866 - Some(s) => { 1867 - if let Some(pos) = s.find('?') { 1868 - (Some(&s[..pos]), Some(&s[pos + 1..])) 1869 - } else { 1870 - (Some(s), None) 1871 - } 1872 - } 1873 - None => (None, None), 1878 + ) -> Result<(SmallVec<[&str; 2]>, Option<Vec<&str>>), ParseError> { 1879 + let Some(suffix) = suffix else { 1880 + return Ok((SmallVec::new(), None)); 1874 1881 }; 1882 + let (positional, mut params) = match suffix.find('?') { 1883 + Some(pos) => (&suffix[..pos], parse_query_string(&suffix[pos + 1..])), 1884 + None => (suffix, BTreeMap::new()), 1885 + }; 1886 + let named = params.remove("collection").unwrap_or_default(); 1887 + let actions = params.remove("action"); 1875 1888 1876 - let collection = match collection_str { 1877 - Some("*") | None => None, 1878 - Some(nsid_str) => { 1879 - jacquard_common::types::nsid::validate_nsid(nsid_str)?; 1880 - // Find position of the NSID in the token. 1881 - if let Some(pos) = token.find(nsid_str) { 1882 - let start = base + pos as u16; 1883 - let end = start + nsid_str.len() as u16; 1884 - Some((start, end)) 1885 - } else { 1886 - return Err(ParseError::InvalidResource(nsid_str.to_smolstr())); 1887 - } 1889 + let values: SmallVec<[&str; 2]> = match (positional.is_empty(), named.is_empty()) { 1890 + (false, true) => SmallVec::from_slice(&[positional]), 1891 + (true, false) => named.into_iter().collect(), 1892 + (false, false) => { 1893 + return Err(ParseError::InvalidResource( 1894 + "repo collection given both positionally and as a parameter".to_smolstr(), 1895 + )); 1888 1896 } 1897 + // `repo:`, `repo?`, or `repo?action=create`: a suffix but no collection. 1898 + (true, true) => return Err(ParseError::MissingResource), 1889 1899 }; 1890 1900 1891 - let mut actions = RepoActionFlags(RepoActionFlags::ALL); 1901 + for value in &values { 1902 + if *value != "*" { 1903 + jacquard_common::types::nsid::validate_nsid(value)?; 1904 + } 1905 + } 1906 + Ok((values, actions)) 1907 + } 1892 1908 1893 - if let Some(params) = params { 1894 - let parsed_params = parse_query_string(params); 1895 - if let Some(values) = parsed_params.get("action") { 1896 - let mut flags = 0u8; 1897 - for value in values { 1898 - match value.as_ref() { 1899 - "create" => flags |= RepoActionFlags::CREATE, 1900 - "update" => flags |= RepoActionFlags::UPDATE, 1901 - "delete" => flags |= RepoActionFlags::DELETE, 1902 - "*" => flags = RepoActionFlags::ALL, 1903 - other => return Err(ParseError::InvalidAction(other.to_smolstr())), 1904 - } 1905 - } 1906 - actions = RepoActionFlags(flags); 1909 + /// Parse the `action` values of a `repo` scope, defaulting to all actions. 1910 + fn parse_repo_action_flags(values: Option<Vec<&str>>) -> Result<RepoActionFlags, ParseError> { 1911 + let Some(values) = values else { 1912 + return Ok(RepoActionFlags(RepoActionFlags::ALL)); 1913 + }; 1914 + let mut flags = 0u8; 1915 + for value in values { 1916 + match value { 1917 + "create" => flags |= RepoActionFlags::CREATE, 1918 + "update" => flags |= RepoActionFlags::UPDATE, 1919 + "delete" => flags |= RepoActionFlags::DELETE, 1920 + "*" => flags = RepoActionFlags::ALL, 1921 + other => return Err(ParseError::InvalidAction(other.to_smolstr())), 1907 1922 } 1908 1923 } 1924 + Ok(RepoActionFlags(flags)) 1925 + } 1909 1926 1910 - Ok(ScopeInnerIndices::Repo { 1911 - collection, 1912 - actions, 1913 - }) 1927 + /// Parse repo scope indices, one entry per collection, storing the byte range 1928 + /// of each collection NSID (`None` for the wildcard). 1929 + fn parse_repo_indices( 1930 + token: &str, 1931 + suffix: Option<&str>, 1932 + base: u16, 1933 + ) -> Result<SmallVec<[ScopeInnerIndices; 1]>, ParseError> { 1934 + let (collections, actions) = split_repo_suffix(suffix)?; 1935 + let actions = parse_repo_action_flags(actions)?; 1936 + 1937 + let mut parsed = SmallVec::new(); 1938 + if collections.is_empty() { 1939 + parsed.push(ScopeInnerIndices::Repo { 1940 + collection: None, 1941 + actions, 1942 + }); 1943 + return Ok(parsed); 1944 + } 1945 + for value in collections { 1946 + let collection = if value == "*" { 1947 + None 1948 + } else { 1949 + // Any occurrence of the same text reconstructs the same NSID. 1950 + let pos = token 1951 + .find(value) 1952 + .ok_or_else(|| ParseError::InvalidResource(value.to_smolstr()))?; 1953 + let start = base + pos as u16; 1954 + Some((start, start + value.len() as u16)) 1955 + }; 1956 + parsed.push(ScopeInnerIndices::Repo { 1957 + collection, 1958 + actions, 1959 + }); 1960 + } 1961 + Ok(parsed) 1914 1962 } 1915 1963 1916 1964 /// Parse RPC scope indices, storing byte ranges of lexicon and audience values. ··· 2798 2846 where 2799 2847 S: FromStr, 2800 2848 { 2801 - let (collection_str, params) = match suffix { 2802 - Some(s) => { 2803 - if let Some(pos) = s.find('?') { 2804 - (Some(&s[..pos]), Some(&s[pos + 1..])) 2805 - } else { 2806 - (Some(s), None) 2849 + let (mut collections, actions) = split_repo_suffix(suffix)?; 2850 + // Normalize as `Scopes` does: the wildcard absorbs every other 2851 + // collection, and repeats count once. 2852 + let collection = if collections.is_empty() || collections.contains(&"*") { 2853 + RepoCollection::All 2854 + } else { 2855 + collections.sort_unstable(); 2856 + collections.dedup(); 2857 + match collections.as_slice() { 2858 + [nsid] => RepoCollection::Nsid(Nsid::from_str(nsid)?), 2859 + // A single `Scope` holds one collection; `Scopes` expands these. 2860 + _ => { 2861 + return Err(ParseError::InvalidResource( 2862 + "repo scope names several collections; parse it with Scopes".to_smolstr(), 2863 + )); 2807 2864 } 2808 2865 } 2809 - None => (None, None), 2810 - }; 2811 - 2812 - let collection = match collection_str { 2813 - Some("*") | None => RepoCollection::All, 2814 - Some(nsid) => RepoCollection::Nsid(Nsid::from_str(nsid)?), 2815 2866 }; 2816 - 2817 - let mut actions = BTreeSet::new(); 2818 - if let Some(params) = params { 2819 - let parsed_params = parse_query_string(params); 2820 - if let Some(values) = parsed_params.get("action") { 2821 - for value in values { 2822 - match value.as_ref() { 2823 - "create" => { 2824 - actions.insert(RepoAction::Create); 2825 - } 2826 - "update" => { 2827 - actions.insert(RepoAction::Update); 2828 - } 2829 - "delete" => { 2830 - actions.insert(RepoAction::Delete); 2831 - } 2832 - "*" => { 2833 - actions.insert(RepoAction::Create); 2834 - actions.insert(RepoAction::Update); 2835 - actions.insert(RepoAction::Delete); 2836 - } 2837 - other => return Err(ParseError::InvalidAction(other.to_smolstr())), 2838 - } 2839 - } 2840 - } 2841 - } 2842 - 2843 - if actions.is_empty() { 2844 - actions.insert(RepoAction::Create); 2845 - actions.insert(RepoAction::Update); 2846 - actions.insert(RepoAction::Delete); 2847 - } 2867 + let actions = parse_repo_action_flags(actions)?.to_actions(); 2848 2868 2849 2869 Ok(Scope::Repo(RepoScope { 2850 2870 collection, ··· 3804 3824 ); 3805 3825 } 3806 3826 3827 + #[test] 3828 + fn test_repo_scope_collection_param_parsing() { 3829 + // A single collection in query form is the same scope as the 3830 + // positional form. 3831 + let named: Scope = 3832 + Scope::parse("repo?collection=app.bsky.feed.post&action=create").unwrap(); 3833 + let positional: Scope = Scope::parse("repo:app.bsky.feed.post?action=create").unwrap(); 3834 + assert_eq!(named, positional); 3835 + 3836 + let named: Scope = Scope::parse("repo?collection=*").unwrap(); 3837 + assert_eq!(named, Scope::parse("repo:*").unwrap()); 3838 + 3839 + // Normalized like `Scopes`: the wildcard absorbs, repeats count once. 3840 + let absorbed: Scope = 3841 + Scope::parse("repo?collection=*&collection=app.bsky.feed.post").unwrap(); 3842 + assert_eq!(absorbed, Scope::parse("repo:*").unwrap()); 3843 + let repeated: Scope = 3844 + Scope::parse("repo?collection=app.bsky.feed.post&collection=app.bsky.feed.post") 3845 + .unwrap(); 3846 + assert_eq!(repeated, Scope::parse("repo:app.bsky.feed.post").unwrap()); 3847 + 3848 + // A single `Scope` can hold only one collection; `Scopes` expands these. 3849 + let err = Scope::<SmolStr>::parse( 3850 + "repo?collection=app.bsky.feed.post&collection=app.bsky.feed.like", 3851 + ) 3852 + .unwrap_err(); 3853 + assert!(matches!(err, ParseError::InvalidResource(_))); 3854 + } 3855 + 3807 3856 #[test] 3808 3857 fn test_rpc_scope_parsing() { 3809 3858 let scope: Scope = Scope::parse("rpc:*").unwrap(); ··· 4553 4602 } 4554 4603 } 4555 4604 4605 + #[test] 4606 + fn test_scopes_repo_collection_param() { 4607 + // The permission spec lets `collection` be given as repeated query 4608 + // parameters instead of positionally; each one becomes its own scope, 4609 + // the same way permission sets expand. Expected strings mirror the 4610 + // consistency cases in the reference implementation 4611 + // (bluesky-social/atproto packages/oauth/oauth-scopes), except that 4612 + // several collections normalize to one scope per collection. 4613 + let test_cases = [ 4614 + ("repo?collection=*", "repo:*"), 4615 + ("repo?collection=*&action=update", "repo:*?action=update"), 4616 + ( 4617 + "repo?collection=*&collection=com.example.foo&action=update", 4618 + "repo:*?action=update", 4619 + ), 4620 + ("repo?collection=*&collection=com.example.foo", "repo:*"), 4621 + ( 4622 + "repo?action=create&collection=com.example.foo", 4623 + "repo:com.example.foo?action=create", 4624 + ), 4625 + ( 4626 + "repo?collection=com.example.foo&action=create&action=update&action=delete", 4627 + "repo:com.example.foo", 4628 + ), 4629 + ( 4630 + "repo?action=create&collection=com.example.foo&collection=com.example.bar", 4631 + "repo:com.example.bar?action=create repo:com.example.foo?action=create", 4632 + ), 4633 + // Duplicates collapse. 4634 + ( 4635 + "repo?collection=com.example.foo&collection=com.example.foo", 4636 + "repo:com.example.foo", 4637 + ), 4638 + // One collection is a prefix of another; each keeps its own NSID. 4639 + ( 4640 + "repo?collection=com.example.foo.bar&collection=com.example.foo", 4641 + "repo:com.example.foo repo:com.example.foo.bar", 4642 + ), 4643 + ]; 4644 + 4645 + for (input, expected) in test_cases { 4646 + let scopes = Scopes::new(SmolStr::new_static(input)) 4647 + .unwrap_or_else(|e| panic!("failed to parse {input}: {e}")); 4648 + assert_eq!( 4649 + scopes.to_normalized_string(), 4650 + expected, 4651 + "failed for: {input}" 4652 + ); 4653 + } 4654 + } 4655 + 4656 + #[test] 4657 + fn test_scopes_repo_collection_param_grants() { 4658 + // As generated by @atcute/oauth-types for a client requesting two 4659 + // collections, alongside other scopes in the same string. 4660 + let scopes = Scopes::new(SmolStr::new_static( 4661 + "atproto repo?collection=fm.teal.feed.play&collection=app.bsky.feed.post&action=create blob:image/*", 4662 + )) 4663 + .unwrap(); 4664 + assert_eq!(scopes.len(), 4); 4665 + 4666 + for collection in ["fm.teal.feed.play", "app.bsky.feed.post"] { 4667 + let create: Scope = Scope::repo_create(collection).unwrap(); 4668 + let delete: Scope = Scope::repo_delete(collection).unwrap(); 4669 + assert!( 4670 + scopes.grants(&create), 4671 + "should grant create on {collection}" 4672 + ); 4673 + assert!( 4674 + !scopes.grants(&delete), 4675 + "should not grant delete on {collection}" 4676 + ); 4677 + } 4678 + let other: Scope = Scope::repo_create("app.bsky.feed.like").unwrap(); 4679 + assert!(!scopes.grants(&other)); 4680 + } 4681 + 4682 + #[test] 4683 + fn test_scopes_repo_collection_param_invalid() { 4684 + let invalid = [ 4685 + // The spec requires a collection. 4686 + "repo?action=create", 4687 + "repo?", 4688 + "repo:", 4689 + "repo?collection=", 4690 + // A parameter can't be both positional and named. 4691 + "repo:app.bsky.feed.post?collection=app.bsky.feed.like", 4692 + "repo:*?collection=app.bsky.feed.post", 4693 + // Values are still validated. 4694 + "repo?collection=invalid", 4695 + "repo?collection=app.bsky.feed.post&action=invalid", 4696 + ]; 4697 + 4698 + for input in invalid { 4699 + assert!( 4700 + Scopes::new(SmolStr::new_static(input)).is_err(), 4701 + "should reject: {input}" 4702 + ); 4703 + assert!( 4704 + Scope::<SmolStr>::parse(input).is_err(), 4705 + "should reject: {input}" 4706 + ); 4707 + } 4708 + } 4709 + 4556 4710 #[test] 4557 4711 fn test_scopes_rpc_scope_parsing() { 4558 4712 // Test rpc scopes parse correctly.

History

1 round 1 comment
Sign up or Login to add to the discussion
okami.mom submitted #0
1 commit
Expand
961f9076
oauth: parse repo scopes that name collections with the collection parameter
Checking mergeability…
Expand 1 comment

theres also a regression in 0.12 where the generated code produces unused warnings