From 19f41990c14783f7ea1c4389cc1539aed9c4ed6b Mon Sep 17 00:00:00 2001
From: Trezy <tre@trezy.com>
Date: Tue, 7 Jul 2026 12:48:21 -0500
Subject: [PATCH] fix: prevent `read_self` from escalating during access merge

Signed-off-by: Trezy <tre@trezy.com>
---
 src/spaces/members.rs | 38 +++++++++++++++++++++++++++++++++++---
 src/spaces/types.rs   | 22 ++++++++++++++++++++++
 tests/spaces_db.rs    | 43 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 100 insertions(+), 3 deletions(-)

diff --git a/src/spaces/members.rs b/src/spaces/members.rs
index 2c3eceb..f355697 100644
--- a/src/spaces/members.rs
+++ b/src/spaces/members.rs
@@ -105,9 +105,12 @@ async fn resolve_delegation_target(
 }
 
 fn merge_access(resolved: &mut HashMap<String, SpaceAccess>, did: &str, access: SpaceAccess) {
-    let entry = resolved.entry(did.to_string()).or_insert(SpaceAccess::Read);
-    if access.can_write() {
-        *entry = SpaceAccess::Write;
+    // Seed with the member's real level (so `read_self` survives), then only
+    // ever upgrade to a higher privilege — never downgrade. Seeding at `Read`
+    // here would silently promote `read_self` members to full `read`.
+    let entry = resolved.entry(did.to_string()).or_insert(access);
+    if access.rank() > entry.rank() {
+        *entry = access;
     }
 }
 
@@ -129,6 +132,35 @@ mod tests {
         assert_eq!(map["did:plc:user1"], SpaceAccess::Write);
     }
 
+    #[test]
+    fn merge_access_preserves_read_self() {
+        // A read_self member must NOT be silently promoted to full read.
+        let mut map = HashMap::new();
+        merge_access(&mut map, "did:plc:user", SpaceAccess::ReadSelf);
+        assert_eq!(map["did:plc:user"], SpaceAccess::ReadSelf);
+    }
+
+    #[test]
+    fn merge_access_upgrades_but_never_downgrades() {
+        // read_self upgraded by a higher grant on another path.
+        let mut map = HashMap::new();
+        merge_access(&mut map, "u", SpaceAccess::ReadSelf);
+        merge_access(&mut map, "u", SpaceAccess::Read);
+        assert_eq!(map["u"], SpaceAccess::Read);
+        merge_access(&mut map, "u", SpaceAccess::Write);
+        assert_eq!(map["u"], SpaceAccess::Write);
+
+        // A lower grant on another path never downgrades.
+        let mut map2 = HashMap::new();
+        merge_access(&mut map2, "v", SpaceAccess::Read);
+        merge_access(&mut map2, "v", SpaceAccess::ReadSelf);
+        assert_eq!(map2["v"], SpaceAccess::Read);
+
+        merge_access(&mut map2, "v", SpaceAccess::Write);
+        merge_access(&mut map2, "v", SpaceAccess::ReadSelf);
+        assert_eq!(map2["v"], SpaceAccess::Write);
+    }
+
     #[test]
     fn merge_access_multiple_users() {
         let mut map = HashMap::new();
diff --git a/src/spaces/types.rs b/src/spaces/types.rs
index 622c536..a2c7d06 100644
--- a/src/spaces/types.rs
+++ b/src/spaces/types.rs
@@ -31,6 +31,20 @@ impl SpaceAccess {
         matches!(self, SpaceAccess::Write)
     }
 
+    /// Privilege rank used when merging memberships reached via multiple paths
+    /// (direct + delegation) — the highest rank wins. `read_self` is the most
+    /// restricted (own repo only), then `read`, then `write`.
+    ///
+    /// Note: the enum's declaration order (`Read`, `ReadSelf`, `Write`) does
+    /// **not** match privilege order, so `Ord` must not be derived — use this.
+    pub fn rank(&self) -> u8 {
+        match self {
+            SpaceAccess::ReadSelf => 0,
+            SpaceAccess::Read => 1,
+            SpaceAccess::Write => 2,
+        }
+    }
+
     pub fn can_read(&self) -> bool {
         true
     }
@@ -255,6 +269,14 @@ mod tests {
         assert!(SpaceAccess::Write.can_write());
     }
 
+    #[test]
+    fn space_access_rank_orders_by_privilege() {
+        // read_self is the most restricted, then read, then write — regardless
+        // of enum declaration order.
+        assert!(SpaceAccess::ReadSelf.rank() < SpaceAccess::Read.rank());
+        assert!(SpaceAccess::Read.rank() < SpaceAccess::Write.rank());
+    }
+
     #[test]
     fn mint_policy_roundtrip() {
         assert_eq!(
diff --git a/tests/spaces_db.rs b/tests/spaces_db.rs
index 345d923..7bdebd3 100644
--- a/tests/spaces_db.rs
+++ b/tests/spaces_db.rs
@@ -514,6 +514,49 @@ async fn add_and_get_member() {
     assert!(!fetched.is_delegation);
 }
 
+#[tokio::test]
+#[serial]
+async fn resolve_members_preserves_read_self() {
+    common::require_db!();
+    let pool = test_db::test_pool().await;
+    let backend = test_db::test_backend();
+    test_db::truncate_all(&pool).await;
+
+    let space_id = new_id();
+    let space = make_space(
+        &space_id,
+        "did:plc:rs-owner",
+        "com.example.readself",
+        "rs-skey",
+    );
+    spaces_db::create_space(&pool, backend, &space)
+        .await
+        .expect("create_space failed");
+
+    let member_did = "did:plc:rs-member";
+    spaces_db::add_member(
+        &pool,
+        backend,
+        &SpaceMember {
+            id: new_id(),
+            space_id: space_id.clone(),
+            did: member_did.to_string(),
+            access: SpaceAccess::ReadSelf,
+            is_delegation: false,
+            granted_by: Some("did:plc:rs-owner".to_string()),
+            created_at: now_rfc3339(),
+        },
+    )
+    .await
+    .expect("add_member failed");
+
+    // Resolution must not promote read_self to full read.
+    let access = happyview::spaces::members::is_member(&pool, backend, &space_id, member_did)
+        .await
+        .expect("is_member failed");
+    assert_eq!(access, Some(SpaceAccess::ReadSelf));
+}
+
 #[tokio::test]
 #[serial]
 async fn remove_member() {
-- 
2.51.2