aboutsummaryrefslogtreecommitdiffstats
path: root/packages/meshbay-node/tests/test_security_regressions.py
diff options
context:
space:
mode:
authorChristophe Besson <cbesson@gmail.com>2026-09-06 17:48:36 +0200
committerChristophe Besson <cbesson@gmail.com>2026-09-06 17:48:36 +0200
commitea56b8c79538323875c00db2e7006b255f7cd494 (patch)
treeee08835bc190a75e49a6a8e78755111aef0e678f /packages/meshbay-node/tests/test_security_regressions.py
parente76e27868b30a2b00b1ba42dd8e7ee6071e0c0d7 (diff)
downloadmeshbay-ea56b8c79538323875c00db2e7006b255f7cd494.tar.gz
fix(groups): finish Phase 1 — MNP root management, upload targets, eject state
Review of the Phase 1 commit found the RO/RW model sound but three paths unfinished, each of which broke the flow the phase exists to deliver. Plus 29 test failures it introduced and no coverage for anything it added. Uploads went to the wrong directory. The node read a `root` field on file_upload that no client ever sent, so every upload landed in the first writable root while the Files toolbar offered its button based on the root being browsed — with two writable roots, uploading from one wrote into the other. Files now names the root it is showing; Chat names one chosen in the shell (an operator-configured directory arrives in Phase 2); the node refuses an unknown name rather than falling back, and refuses read-only and ejected roots by code. Shared directories were unreachable on the web. The table read its roots only from the loopback API, which resolves to "not available" in a browser, so the section rendered for nobody there — while the Uploads controls it replaced had worked — and the transport.updateRoot/ejectRoot/plugRoot methods beside it were dead. MNP is now the path, loopback the fallback for a local node with no live connection, and adding a root over MNP takes a typed path since no web page can browse a remote disk. Ejecting updated nobody's screen. transport.js resolves an admin ack against the pending request and returns, which is right for every op whose caller knows the value it chose; the root acks carry state only the node can compute, so the operator who clicked Eject was the one client that never saw it happen. And the ejected flag reached roster.db but was never read back, so a restart undid it and the next scan read an empty mount point as an erased library. Also: the member-upload endpoint answered 200 and did nothing (removed); the wizard ignored the first root's RW switch; reload compared roots on name and path, so editing writable in node.toml did nothing; the table had no path column, which is the only thing separating two libraries sharing a basename; apps_enabled normalisation differed between the two sides of a signed subject. Tests: eject/plug, per-root upload refusal and the node.toml rewrite had no coverage at all. test_member_upload_policy.py is replaced by test_root_writable_policy.py — it tested a removed feature — and every property worth keeping from it moved rather than being dropped. Docs: draft-v6 structural decision 9 is annotated as superseded (the operator can no longer have a directory only they may write to — a real capability removed, flagged rather than hidden), the man page documents the root verb and the RO/RW fields, and refactor-groups.md §7b records what the plan got wrong. Suite: 41 failures before, 13 after — all 13 pre-existing on main. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011pvMdvLBG92jyhvD5pD6us
Diffstat (limited to 'packages/meshbay-node/tests/test_security_regressions.py')
-rw-r--r--packages/meshbay-node/tests/test_security_regressions.py145
1 files changed, 137 insertions, 8 deletions
diff --git a/packages/meshbay-node/tests/test_security_regressions.py b/packages/meshbay-node/tests/test_security_regressions.py
index 7f71da5..9db8ac1 100644
--- a/packages/meshbay-node/tests/test_security_regressions.py
+++ b/packages/meshbay-node/tests/test_security_regressions.py
@@ -18,6 +18,7 @@ from cryptography.hazmat.primitives.asymmetric.ed25519 import Ed25519PrivateKey
from meshbay_common.protocol import IndexEntry
from meshbay_node.indexer.group_index import GroupIndex
+from meshbay_node.roots import RootSet
from conftest import one_root
from meshbay_node.transport.webrtc_server import WebRTCPeerSession
@@ -132,14 +133,14 @@ def test_the_node_never_generates_a_name_it_would_refuse(tmp_path):
def _uploads_dir(session) -> Path:
"""
- Where this session's uploads land: uploads/ inside the group's upload root.
+ Where this session's uploads land: uploads/ inside its first writable root.
Asked of the root set rather than assembled by hand, so a test cannot pass
while agreeing with a wrong answer the code also produced.
"""
- root = session._ctx["roots"].upload_root
- assert root is not None, "the fixture must designate an upload root"
- return root.path / "uploads"
+ writable = session._ctx["roots"].writable_roots
+ assert writable, "the fixture must give the group a writable root"
+ return writable[0].path / "uploads"
def _session(tmp_path: Path, user_id: str) -> WebRTCPeerSession:
@@ -227,14 +228,17 @@ def test_dir_create_cannot_escape_the_shared_root(tmp_path, bad):
def test_upload_ignores_any_directory_the_client_asks_for(tmp_path):
"""
- Uploads land in uploads/, chosen by the node. A client that names somewhere
- else — or nowhere at all — changes nothing, so the traversal surface that a
- client-chosen destination would open does not exist on this path.
+ The destination inside a root is the node's decision, and stays so.
+
+ A client now names the *root* it is uploading into — it has to, once a group
+ can have several writable ones — but that is a name looked up in the root
+ table, never a path. Everything below the root is still chosen here, so the
+ traversal surface a client-chosen destination would open does not exist.
"""
session = _session(tmp_path, "user-1")
session._do_file_upload({
- "filename": "note.txt", "dir": "../../etc",
+ "filename": "note.txt", "dir": "../../etc", "path": "/etc",
"chunk_index": 0, "total_chunks": 1,
"data": base64.b64encode(b"x").decode(),
})
@@ -243,6 +247,131 @@ def test_upload_ignores_any_directory_the_client_asks_for(tmp_path):
assert not (tmp_path / "etc").exists()
+@pytest.mark.parametrize("named_root", [
+ "../../etc", "/etc", "shared/../..", "Shared/uploads", "nope",
+])
+def test_a_root_name_is_looked_up_never_joined(tmp_path, named_root):
+ """
+ The name the client sends is matched against the group's root table and
+ refused when it matches nothing. A version that joined it to a path — or
+ that quietly fell back to the first writable root — would turn "which
+ directory" into either a traversal or a file on a disk the operator did
+ not intend, and the second is discovered weeks later.
+ """
+ session = _session(tmp_path, "user-1")
+ before = set(tmp_path.rglob("*"))
+
+ session._do_file_upload({
+ "filename": "note.txt", "root": named_root,
+ "chunk_index": 0, "total_chunks": 1,
+ "data": base64.b64encode(b"x").decode(),
+ })
+
+ refusal = [m for m in session.sent if m.get("type") == "error"]
+ assert refusal and refusal[0].get("code") == "no_such_root", named_root
+ assert set(tmp_path.rglob("*")) == before, f"wrote something via {named_root!r}"
+
+
+def test_an_upload_goes_to_the_root_it_names(tmp_path):
+ """
+ With two writable roots there is no defensible default, and the client is
+ the only party that knows which directory the person is looking at. The
+ node picking one meant a file uploaded from a folder on screen landed in a
+ different one — the same "uploads went somewhere else" the single upload
+ root was never allowed to guess about.
+ """
+ media = tmp_path / "Media"
+ incoming = tmp_path / "Incoming"
+ media.mkdir()
+ incoming.mkdir()
+ session = _session(tmp_path, "user-1")
+ session._ctx["roots"] = RootSet.build([
+ {"path": str(media), "writable": True},
+ {"path": str(incoming), "writable": True},
+ ])
+
+ session._do_file_upload({
+ "filename": "note.txt", "root": "Incoming",
+ "chunk_index": 0, "total_chunks": 1,
+ "data": base64.b64encode(b"x").decode(),
+ })
+
+ assert (incoming / "uploads" / "note.txt").read_bytes() == b"x"
+ assert not (media / "uploads").exists(), "it went to the first root instead"
+
+
+def test_a_read_only_root_refuses_an_upload(tmp_path):
+ """
+ RO is the mechanism now, not a hidden button. It binds the operator too:
+ "read-only for everyone" is what makes a published library one, and an
+ exception for whoever happens to hold admin authority is the sort of
+ carve-out that later reads as the rule.
+ """
+ published = tmp_path / "Published"
+ published.mkdir()
+ session = _session(tmp_path, "user-1")
+ session._ctx["roots"] = RootSet.build([{"path": str(published)}])
+ session._is_node_admin = lambda: True
+
+ session._do_file_upload({
+ "filename": "note.txt", "root": "Published",
+ "chunk_index": 0, "total_chunks": 1,
+ "data": base64.b64encode(b"x").decode(),
+ })
+
+ refusal = [m for m in session.sent if m.get("type") == "error"]
+ assert refusal and refusal[0].get("code") == "root_read_only"
+ assert not (published / "uploads").exists()
+
+
+def test_a_fully_read_only_group_refuses_an_unaddressed_upload(tmp_path):
+ """
+ An MNP 1.0 client names no root, so the node falls back to the first
+ writable one. There isn't one here, and the fallback must refuse rather
+ than write into whatever root happens to come first.
+ """
+ published = tmp_path / "Published"
+ published.mkdir()
+ session = _session(tmp_path, "user-1")
+ session._ctx["roots"] = RootSet.build([{"path": str(published)}])
+
+ session._do_file_upload({
+ "filename": "note.txt",
+ "chunk_index": 0, "total_chunks": 1,
+ "data": base64.b64encode(b"x").decode(),
+ })
+
+ refusal = [m for m in session.sent if m.get("type") == "error"]
+ assert refusal and refusal[0].get("code") == "no_writable_root"
+ assert not (published / "uploads").exists()
+
+
+def test_an_ejected_root_refuses_an_upload(tmp_path):
+ """
+ Writing to a drive somebody has their hand on is the thing eject exists to
+ stop. `writable` is still true — that is configuration — so availability
+ has to be checked separately, which is what an earlier version conflated.
+ """
+ usb = tmp_path / "USB"
+ usb.mkdir()
+ session = _session(tmp_path, "user-1")
+ roots = RootSet.build([{"path": str(usb), "writable": True,
+ "removable": True}])
+ roots.roots[0].ejected = True
+ roots.roots[0].available = False
+ session._ctx["roots"] = roots
+
+ session._do_file_upload({
+ "filename": "note.txt", "root": "USB",
+ "chunk_index": 0, "total_chunks": 1,
+ "data": base64.b64encode(b"x").decode(),
+ })
+
+ refusal = [m for m in session.sent if m.get("type") == "error"]
+ assert refusal and refusal[0].get("code") == "root_unavailable"
+ assert not (usb / "uploads").exists()
+
+
def test_two_members_can_send_the_same_filename(tmp_path):
"""
One shared uploads/ means collisions are ordinary — every camera produces