From 6faafc48a025957e745fc5d2a40ccae35051efbc Mon Sep 17 00:00:00 2001 From: "@permadeath.com" Date: Tue, 1 Sep 2026 15:02:01 -0400 Subject: [PATCH] fix(pr): one reader for whose record an at-uri names MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit "Is this record mine" was written four ways, and one of them tested `rest.starts_with(me)` with no trailing `/`, so any account whose DID merely begins with yours read as you — and that reader promotes a record it believes is yours from "state unknown" to "open". `did:plc` is fixed-width so it could not bite there, but a `did:web` is as long as its domain: standing as `did:web:example.com`, a pull at `at://did:web:example.com.evil.net/...` was yours. `model::record`'s `authority_of` is the one reader now, and `resolve.rs` follows the `is_did` rule its own doc comment already claimed it followed. Change-Id: I20a52c8bd517fdc11e9955bc3f6b764986fbbad9 --- src/clients/tangled/resolve.rs | 8 +++-- src/cmd/issue/read.rs | 5 +-- src/cmd/pr/read/labels.rs | 14 ++------ src/cmd/pr/read/mod.rs | 4 +-- src/cmd/pr/read/sources.rs | 5 +-- src/cmd/stack/mod.rs | 13 ++----- src/cmd/stack/read.rs | 4 +-- src/cmd/stack/write.rs | 3 +- src/model/record.rs | 62 ++++++++++++++++++++++++++++++++++ 9 files changed, 79 insertions(+), 39 deletions(-) diff --git a/src/clients/tangled/resolve.rs b/src/clients/tangled/resolve.rs index 9302021..134ba59 100644 --- a/src/clients/tangled/resolve.rs +++ b/src/clients/tangled/resolve.rs @@ -268,8 +268,12 @@ pub async fn owner_of(repo_did: &str) -> Result> { let Some(owner) = path.split('/').find(|seg| !seg.is_empty()) else { return Ok(None); }; - // Already a DID in the path is possible and needs no second lookup. - if owner.starts_with("did:") { + // Already a DID in the path is possible and needs no second lookup — + // judged by [`crate::lexicon::identity::is_did`], the rule this file's + // own `classify_path_did` follows and this had drifted from. A + // `starts_with("did:")` test hands back `did:plc:short` as an identity; + // a malformed one is not one, and falls through to the probe below. + if crate::lexicon::identity::is_did(owner) { return Ok(Some(owner.to_string())); } match crate::clients::atproto::did::resolve_handle(owner).await { diff --git a/src/cmd/issue/read.rs b/src/cmd/issue/read.rs index 2b2c00b..d1cf6f0 100644 --- a/src/cmd/issue/read.rs +++ b/src/cmd/issue/read.rs @@ -564,10 +564,7 @@ impl Listed { /// safe only while every row came from one account's PDS. A repo-scoped /// index listing spans authors, which is the whole point of it. fn author_did(&self) -> &str { - self.uri - .strip_prefix("at://") - .and_then(|rest| rest.split('/').next()) - .unwrap_or_default() + crate::model::record::authority_of(&self.uri).unwrap_or_default() } fn title(&self) -> &str { diff --git a/src/cmd/pr/read/labels.rs b/src/cmd/pr/read/labels.rs index 29e9e19..362796b 100644 --- a/src/cmd/pr/read/labels.rs +++ b/src/cmd/pr/read/labels.rs @@ -96,12 +96,7 @@ pub(in crate::cmd::pr) fn target_repo_did(item: &serde_json::Value) -> Option` for every row it can place, across every repo on the @@ -290,12 +285,7 @@ fn label_from_location(location: &str) -> Option { pub(super) fn author_did(item: &serde_json::Value) -> Option { // at://did:plc:xyz/sh.tangled.repo.pull/rkey - item["uri"] - .as_str()? - .strip_prefix("at://")? - .split('/') - .next() - .map(String::from) + crate::model::record::authority_of(item["uri"].as_str()?).map(String::from) } /// Split `at:///sh.tangled.repo/` into its owner and name. /// The collection sits between them, hence the skip. diff --git a/src/cmd/pr/read/mod.rs b/src/cmd/pr/read/mod.rs index 71f2873..0c3a22d 100644 --- a/src/cmd/pr/read/mod.rs +++ b/src/cmd/pr/read/mod.rs @@ -494,7 +494,7 @@ pub(super) async fn list(args: ListArgs) -> Result<()> { /// asked about `@bob.example.com` is owed sentences about `@bob.example.com`; /// answering them about `did:plc:…` is correct and unreadable. fn author_label(input: &str) -> String { - match input.starts_with("did:") { + match crate::lexicon::identity::looks_like_a_did(input) { true => input.to_string(), false => format!("@{}", input.trim_start_matches('@')), } @@ -507,7 +507,7 @@ fn author_label(input: &str) -> String { /// record. `None` for anything that is not one, which then matches no /// filter. fn uri_author(uri: &str) -> Option<&str> { - uri.strip_prefix("at://")?.split('/').next() + crate::model::record::authority_of(uri) } async fn in_this_repo(args: ListArgs) -> Result<()> { diff --git a/src/cmd/pr/read/sources.rs b/src/cmd/pr/read/sources.rs index c06c459..4c7fb5c 100644 --- a/src/cmd/pr/read/sources.rs +++ b/src/cmd/pr/read/sources.rs @@ -1218,10 +1218,7 @@ fn settle_own_open(items: &mut [Listed], me: Option<&str>, complete: bool) { return; }; for item in items.iter_mut() { - let mine = item - .uri - .strip_prefix("at://") - .is_some_and(|rest| rest.starts_with(me)); + let mine = crate::model::record::is_authored_by(&item.uri, me); if mine && item.state == State::Unknown { item.state = State::Known(PullState::Open.label().to_string()); } diff --git a/src/cmd/stack/mod.rs b/src/cmd/stack/mod.rs index 96d817b..ea4a40f 100644 --- a/src/cmd/stack/mod.rs +++ b/src/cmd/stack/mod.rs @@ -160,16 +160,10 @@ pub(in crate::cmd) fn chain_for_branch<'a>( none_advice: &str, flat_advice: &str, ) -> Result> { - let own_prefix = format!("at://{me}/"); let own: Vec<&serde_json::Value> = items .iter() .copied() - .filter(|i| { - i["uri"] - .as_str() - .unwrap_or_default() - .starts_with(&own_prefix) - }) + .filter(|i| crate::model::record::is_authored_by(i["uri"].as_str().unwrap_or_default(), me)) .collect(); let mine = match (entry_for_branch(&own, branch, closed), whose) { @@ -189,8 +183,7 @@ pub(in crate::cmd) fn chain_for_branch<'a>( crate::cmd::pr::read::for_branch(std::iter::once(**i), branch).is_some() }) .filter_map(|i| i["uri"].as_str()) - .filter_map(|uri| uri.strip_prefix("at://")) - .filter_map(|rest| rest.split('/').next()) + .filter_map(crate::model::record::authority_of) .collect(); if authors.len() > 1 { let named: Vec<&str> = authors.into_iter().collect(); @@ -232,7 +225,7 @@ pub(in crate::cmd) fn chain_for_branch<'a>( if whose == Whose::Mine { for member in &chain.members { let uri = member["uri"].as_str().unwrap_or_default(); - if !uri.starts_with(&own_prefix) { + if !crate::model::record::is_authored_by(uri, me) { // What `pr` already answers for a round, an edit or a close // against somebody else's record: there is a session and it // is the wrong one. diff --git a/src/cmd/stack/read.rs b/src/cmd/stack/read.rs index 8d5a7f8..c3fd81c 100644 --- a/src/cmd/stack/read.rs +++ b/src/cmd/stack/read.rs @@ -504,9 +504,7 @@ async fn commits_per_member(chain: &Chain<'_>) -> std::collections::HashMap) -> Option { .members .first() .and_then(|m| m["uri"].as_str()) - .and_then(|uri| uri.strip_prefix("at://")) - .and_then(|rest| rest.split('/').next()) + .and_then(crate::model::record::authority_of) .map(str::to_string) } diff --git a/src/model/record.rs b/src/model/record.rs index cfc9cc0..f9d5f7e 100644 --- a/src/model/record.rs +++ b/src/model/record.rs @@ -125,3 +125,65 @@ mod tests { ); } } + +/// The account whose repository a record lives in: the authority segment of +/// `at:////`. +/// +/// A record's authority *is* its author — an at-uri names the repository the +/// record sits in, and only that account can write there — so this answers +/// "whose is it" without touching the record. +/// +/// `None` for anything that is not an at-uri, which then matches no account +/// rather than matching every one. +pub(crate) fn authority_of(uri: &str) -> Option<&str> { + uri.strip_prefix("at://")?.split('/').next() +} + +/// Whether `uri` names a record in `did`'s repository. +/// +/// **Split on the delimiter, never `starts_with` the DID.** One copy of this +/// tested `rest.starts_with(me)` with no trailing `/`, which makes any +/// account whose DID begins with yours read as you. `did:plc` is +/// fixed-width so it could not bite there, but a `did:web` is as long as its +/// domain: standing as `did:web:example.com`, a record at +/// `at://did:web:example.com.evil.net/...` came back as yours — and the one +/// caller promotes a record it believes is yours from "state unknown" to +/// "open". +pub(crate) fn is_authored_by(uri: &str, did: &str) -> bool { + authority_of(uri) == Some(did) +} + +#[cfg(test)] +mod authority_tests { + use super::*; + + #[test] + fn the_authority_is_the_first_segment() { + assert_eq!( + authority_of("at://did:plc:abc/sh.tangled.repo.pull/3k"), + Some("did:plc:abc") + ); + } + + #[test] + fn anything_that_is_not_an_at_uri_has_no_authority() { + assert_eq!(authority_of("https://example.com/x"), None); + assert_eq!(authority_of(""), None); + } + + #[test] + fn an_account_whose_did_merely_begins_with_mine_is_not_me() { + // The whole reason this is a function: `starts_with` says yes here. + let theirs = "at://did:web:example.com.evil.net/sh.tangled.repo.pull/3k"; + assert!(!is_authored_by(theirs, "did:web:example.com")); + assert!(is_authored_by( + "at://did:web:example.com/sh.tangled.repo.pull/3k", + "did:web:example.com" + )); + } + + #[test] + fn a_bare_authority_with_no_collection_still_reads() { + assert_eq!(authority_of("at://did:plc:abc"), Some("did:plc:abc")); + } +} -- 2.51.2