diff options
| -rw-r--r-- | docs/third-review.md | 61 | ||||
| -rw-r--r-- | packages/meshbay-hub/src/meshbay_hub/api/groups.py | 14 | ||||
| -rw-r--r-- | packages/meshbay-hub/tests/test_node_auth.py | 23 |
3 files changed, 61 insertions, 37 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` +> **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). -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. +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 |