From 1e6db7d23c70b7bd7e1422f09911b3645f0fb2e2 Mon Sep 17 00:00:00 2001
From: Christophe Besson <cbesson@gmail.com>
Date: Tue, 1 Sep 2026 20:50:45 +0200
Subject: revert(hub): M6 — add_group_member must keep accepting node tokens
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

M6 in the third review was a misread. `add_group_member` accepting a
node-scoped token is deliberate (commit 0443cf8): the node calls
POST /v1/groups/{id}/members/{username} after a CLI `member invite` so
the group shows up in the invitee's SPA, authenticating with a
node-scoped token. `group.admin_id == caller` is the real guard.

An older test (`test_node_scope_blocks_add_member`) asserted the
opposite and had been left red on main; the M6 "fix" (commit 6b38704)
satisfied that test by switching the dependency to `require_user_scope`
— which made `ops.create_invite`'s hub-membership call 403. That
exception is swallowed with a log.warning, so an invited user silently
never lands in group_members and the group is invisible to them.
Reported from live testing (CLI `member invite grenet`, grenet saw
nothing).

Dependency back to `get_current_user`. The stale test now asserts the
intended behaviour: a node token may add a member to its own
operator's group (201) but not to a group it does not own (403).
Third-review M6 marked WITHDRAWN.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011pG75yGK3NthNfyjH74omG
---
 docs/third-review.md                               | 63 +++++++++++++---------
 packages/meshbay-hub/src/meshbay_hub/api/groups.py | 14 ++---
 packages/meshbay-hub/tests/test_node_auth.py       | 23 +++++---
 3 files changed, 62 insertions(+), 38 deletions(-)

diff --git a/docs/third-review.md b/docs/third-review.md
index 6839ef7..531f73e 100644
--- a/docs/third-review.md
+++ b/docs/third-review.md
@@ -75,7 +75,7 @@ and a few of the second-review fixes did not reach every path.
   running SPA.
 
 None of this breaks the architecture. The cryptographic core and the trust model
-are unchanged and still sound. Every finding in this review (H1, H2, M1–M6) was
+are unchanged and still sound. H1, H2 and M1–M5 were
 fixed on 2026-09-01; what is left is the L-list — opportunistic hardening, not a
 hole — plus verifying the SPA CSP (M5) against the live app.
 
@@ -502,24 +502,32 @@ hashes to the `<script>` tags in `_HTML` (the content hash is already computed).
 
 ---
 
-### M6 — `add_group_member` accepts node-scoped tokens
+### M6 — `add_group_member` accepts node-scoped tokens — WITHDRAWN
 
-> **Fixed 2026-09-01.** Dependency changed to `require_user_scope`. The
-> regression test `test_node_auth.py::test_node_scope_blocks_add_member` already
-> existed and was red on `main`; it now passes.
-
-**Location:** `api/groups.py:585-619`
-
-Every mutating group endpoint depends on `require_user_scope` — except
-`POST /v1/groups/{group_id}/members/{username}`, which depended on
-`get_current_user`. `deps.py` states plainly: "Node-scoped tokens CANNOT
-create/delete groups or manage membership." This one could: a node-scoped daemon
-token (or a stolen one) whose subject owns the group could add any existing user
-to it. Impact was bounded (own groups, existing accounts only), but it
-contradicted NS7 ("operator manages groups from the browser only") and was the
-same class as the second review's M9. The test asserting the intended behaviour
-was already in the tree — the endpoint had simply drifted off the shared
-dependency.
+> **Not a finding. Fixed then reverted 2026-09-01.**
+>
+> `add_group_member` accepting a node-scoped token is **deliberate** (commit
+> `0443cf8`): the node calls `POST /v1/groups/{id}/members/{username}` after a
+> CLI `member invite` so the group becomes visible in the invitee's SPA, and it
+> authenticates with a node-scoped token. The `group.admin_id == caller` check
+> is the real guard — a node can only touch its own operator's groups, adding an
+> already-registered account.
+>
+> This review misread that as drift (a stale test,
+> `test_node_scope_blocks_add_member`, asserted the opposite and had been left
+> red on `main`). Tightening the dependency to `require_user_scope` **broke the
+> CLI invite → hub-membership flow**: `ops.create_invite` swallows the resulting
+> 403 with a `log.warning`, so an invited user silently never appears in
+> `group_members` and the group is invisible to them. Found in live testing.
+> The dependency is back on `get_current_user` and the stale test now asserts
+> the intended behaviour (node token may add to its own group, 403 for a group
+> it does not own).
+
+The original M6 concern — a stolen node token adding accounts to the operator's
+own groups — is real but low: bounded to the operator's groups, existing
+accounts only, and it is now a *required* capability. If it is ever worth
+constraining, it needs a dedicated node→membership path, not a scope block on
+this shared endpoint.
 
 ---
 
@@ -636,9 +644,9 @@ Against the v6 §4 claims, updated for this review:
 
 **One-sentence version:** *the E2E story between browser and node is now
 genuinely mutual and covers every path, the second review's critical gaps are
-closed, and every finding in this review (H1, H2, M1–M6) was fixed the day it was
-written — leaving the L-list as opportunistic hardening and one thing to verify:
-the SPA's new CSP against the live app.*
+closed, and H1/H2/M1–M5 were fixed the day this was written (M6 was withdrawn as
+a misread of a deliberate design) — leaving the L-list as opportunistic
+hardening and one thing to verify: the SPA's new CSP against the live app.*
 
 ---
 
@@ -653,7 +661,7 @@ the SPA's new CSP against the live app.*
 | M3 | Link-preview SSRF: no rate limit, ports open, rebinding | Medium | M | ✅ **fixed 2026-09-01** — rate limit + port allowlist + connect-address re-check + bomb guard |
 | M4 | Federation: peer over-trust, `aud` unchecked, revoke no-op | Medium | M | ✅ **fixed 2026-09-01** — source bound to signer, push capped, revoke prunes the peer's own entries, replay rejected |
 | M5 | No CSP / security headers on the SPA | Medium | S | ✅ **fixed 2026-09-01** — CSP + `nosniff` + `frame-ancestors` middleware; verify against the live SPA |
-| M6 | `add_group_member` accepts node tokens | Medium | S | ✅ **fixed 2026-09-01** — dependency → `require_user_scope` |
+| M6 | `add_group_member` accepts node tokens | — | — | ⛔ **withdrawn** — deliberate (CLI invite flow, commit 0443cf8); the "fix" broke it and was reverted |
 | L1 | Relay registration no PoP | Low | S | If/when the relay registry is used |
 | L2 | Orphaned `replication.py` / `revocation.py` | Low | S | Delete now |
 | L3–L11 | See §5 | Low | S | Opportunistic |
@@ -695,8 +703,11 @@ federation peer, and a missing header policy. None of them required exotic
 capability, and none of them were architectural — they were the cost of adding
 six subsystems faster than the authorization model grew to cover them.
 
-Every finding in this review (H1, H2, M1–M6) was fixed the day it was written.
-What is left is the L-list — opportunistic hardening — and one verification: the
-SPA's new CSP (M5) against the running app, since a mis-tuned CSP shows as a
-blank page. On a build whose honest claims are now strong and largely
+H1, H2 and M1–M5 were fixed the day this was written; M6 was withdrawn — it
+misread the node registering a hub membership during the CLI invite flow
+(deliberate, commit `0443cf8`) as authorization drift, and the "fix" broke that
+flow in live testing. What is left is the L-list — opportunistic hardening — and
+one verification: the SPA's new CSP (M5) against the running app, since a
+mis-tuned CSP shows as a blank page. On a build whose honest claims are now
+strong and largely
 defensible.
diff --git a/packages/meshbay-hub/src/meshbay_hub/api/groups.py b/packages/meshbay-hub/src/meshbay_hub/api/groups.py
index 15c7a5d..07d3ec0 100644
--- a/packages/meshbay-hub/src/meshbay_hub/api/groups.py
+++ b/packages/meshbay-hub/src/meshbay_hub/api/groups.py
@@ -586,14 +586,16 @@ async def update_group(
 async def add_group_member(
     group_id: str,
     username: str,
-    current_user: User = Depends(require_user_scope),
+    current_user: User = Depends(get_current_user),
     db: AsyncSession = Depends(get_db),
 ):
-    # `require_user_scope`, like every other mutating group endpoint: a
-    # node-scoped daemon token must not manage membership (NS7 — the operator
-    # manages groups from the browser). This was the one membership endpoint
-    # still on `get_current_user`, so a node token could add members to its
-    # operator's own groups.
+    # `get_current_user`, not `require_user_scope`: the node calls this after a
+    # CLI `member invite` so the group becomes visible in the invitee's SPA
+    # (commit 0443cf8). The node authenticates with a node-scoped token, and the
+    # `group.admin_id == current_user.id` check below is the real guard — a node
+    # can only touch its own operator's groups, adding an already-registered
+    # account. (Third-review M6 proposed tightening this to `require_user_scope`;
+    # that broke the CLI invite flow and was reverted — see the review doc.)
     group = await db.get(Group, group_id)
     if not group:
         raise HTTPException(status_code=404, detail="Group not found")
diff --git a/packages/meshbay-hub/tests/test_node_auth.py b/packages/meshbay-hub/tests/test_node_auth.py
index 72ce412..a104a72 100644
--- a/packages/meshbay-hub/tests/test_node_auth.py
+++ b/packages/meshbay-hub/tests/test_node_auth.py
@@ -148,24 +148,35 @@ async def test_node_scope_blocks_group_create(client):
 
 
 @pytest.mark.asyncio
-async def test_node_scope_blocks_add_member(client):
-    sk_node, user_token = await _setup_node_user(client, "op1")
+async def test_node_token_may_add_a_member_to_its_own_operators_group(client):
+    """The node calls this after a CLI `member invite` so the group shows up in
+    the invitee's SPA (commit 0443cf8). A node-scoped token is accepted here —
+    the `group.admin_id == caller` check is the guard — but only for a group the
+    node's operator owns."""
+    sk_op, op_token = await _setup_node_user(client, "op1")
 
     r = await client.post("/v1/groups", json={
         "name": "mygroup", "visibility": "private", "join_policy": "invite",
-    }, headers={"Authorization": f"Bearer {user_token}"})
-    assert r.status_code == 201
+    }, headers={"Authorization": f"Bearer {op_token}"})
     gid = r.json()["group_id"]
 
     _, pk2 = _gen_ed25519()
     _, px2 = _gen_x25519()
     await _register(client, "member1", pk2, px2)
 
-    r = await _node_auth(client, "op1", sk_node)
-    node_token = r.json()["access_token"]
+    node_token = (await _node_auth(client, "op1", sk_op)).json()["access_token"]
 
     r = await client.post(f"/v1/groups/{gid}/members/member1",
                           headers={"Authorization": f"Bearer {node_token}"})
+    assert r.status_code == 201
+
+    # …but not to a group it does not own.
+    sk_other, other_token = await _setup_node_user(client, "op2")
+    r = await client.post("/v1/groups", json={"name": "theirs", "visibility": "private"},
+                          headers={"Authorization": f"Bearer {other_token}"})
+    other_gid = r.json()["group_id"]
+    r = await client.post(f"/v1/groups/{other_gid}/members/member1",
+                          headers={"Authorization": f"Bearer {node_token}"})
     assert r.status_code == 403
 
 
-- 
cgit v1.2.3