From ea56b8c79538323875c00db2e7006b255f7cd494 Mon Sep 17 00:00:00 2001 From: Christophe Besson Date: Sun, 6 Sep 2026 17:48:36 +0200 Subject: fix(groups): finish Phase 1 — MNP root management, upload targets, eject state MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_011pvMdvLBG92jyhvD5pD6us --- .../tests/test_root_writable_policy.py | 203 +++++++++++++++++++++ 1 file changed, 203 insertions(+) create mode 100644 packages/meshbay-node/tests/test_root_writable_policy.py (limited to 'packages/meshbay-node/tests/test_root_writable_policy.py') diff --git a/packages/meshbay-node/tests/test_root_writable_policy.py b/packages/meshbay-node/tests/test_root_writable_policy.py new file mode 100644 index 0000000..da95032 --- /dev/null +++ b/packages/meshbay-node/tests/test_root_writable_policy.py @@ -0,0 +1,203 @@ +""" +Who may write to the operator's disk, now that RO/RW on the root decides it. + +This replaces `test_member_upload_policy.py`. The old model had two orthogonal +controls — one root designated as the upload target, and a group-wide +`member_upload` switch — and collapsed into one property per root: `writable`. +The properties worth keeping from the old file survive the change unaltered: + +* the interface hiding a control is a courtesy to the people who are not + trying; **the node refusing is the part that holds** against someone who is. + A member with an old tab open, or one speaking MNP directly, gets the same + answer. That half is pinned in `test_security_regressions.py`, next to the + overwrite properties it belongs with; +* the setting is changed by a **signed** operator instruction, or it is a + suggestion any member can undo; +* it is stored on the **node**, never the hub. A hub that could decide who + writes to the operator's disk would have authority over the node. + +And one that is new: the *old* message must no longer be able to change +anything. A deprecated instruction that still works is not deprecated, and this +one would reopen uploads group-wide. +""" + +import base64 +from pathlib import Path + +import pytest +from cryptography.hazmat.primitives.asymmetric.ed25519 import Ed25519PrivateKey + +from meshbay_common.adminop import OP_ROOT_UPDATE, OP_ROOT_EJECT, OP_ROOT_PLUG +from meshbay_common.protocol import MNP +from meshbay_node.indexer.group_index import GroupIndex +from meshbay_node.roots import RootSet +from meshbay_node.transport.webrtc_server import WebRTCPeerSession + +pytestmark = pytest.mark.asyncio + + +def _session(tmp_path: Path, user_id: str, *, + writable: bool = True, + operator: str | None = None) -> WebRTCPeerSession: + shared_root = tmp_path / "shared" + shared_root.mkdir(exist_ok=True) + index = GroupIndex(group_id="g" * 32, sk_node=Ed25519PrivateKey.generate()) + ctx = { + "roots": RootSet.build([{"path": str(shared_root), "writable": writable}]), + "index": index, + "sk_node": index.sk_node, + "node_user_id": operator, + } + session = WebRTCPeerSession.__new__(WebRTCPeerSession) + session._ctx = ctx + session._group_id = "g" * 32 + session._user_id = user_id + session._pk_user = "" + session._uploads = {} + session.sent = [] + session._send = session.sent.append + session._audit = lambda *a, **k: None + return session + + +def _upload(session, filename="clip.mp4", body=b"bytes"): + session._do_file_upload({ + "filename": filename, "root": "shared", + "chunk_index": 0, "total_chunks": 1, + "data": base64.b64encode(body).decode(), + }) + + +def _uploads_dir(session) -> Path: + return session._ctx["roots"].roots[0].path / "uploads" + + +# ── The door, not the button ───────────────────────────────────────────────── + +async def test_a_member_cannot_upload_to_a_read_only_root(tmp_path): + session = _session(tmp_path, "member-1", writable=False) + _upload(session) + + refusal = [m for m in session.sent if m.get("type") == "error"] + assert refusal and refusal[0].get("code") == "root_read_only" + assert not _uploads_dir(session).exists() + + +async def test_members_upload_normally_to_a_writable_root(tmp_path): + session = _session(tmp_path, "member-1", writable=True) + _upload(session) + + assert not [m for m in session.sent if m.get("type") == "error"] + assert (_uploads_dir(session) / "clip.mp4").read_bytes() == b"bytes" + + +async def test_read_only_binds_the_operator_too(tmp_path): + """ + The old model exempted the operator, because the switch was about *members*. + RO is about the directory: a published library is read-only for everyone, and + an exception for admin authority is how a rule turns into a default. + """ + session = _session(tmp_path, "the-operator", writable=False, + operator="the-operator") + session._is_node_admin = lambda: True + _upload(session) + + refusal = [m for m in session.sent if m.get("type") == "error"] + assert refusal and refusal[0].get("code") == "root_read_only" + + +# ── Signed, or it is a suggestion ──────────────────────────────────────────── + +def _capture_challenges(session) -> list[tuple[str, str]]: + issued: list[tuple[str, str]] = [] + + def issue(op, subject, **kw): + issued.append((op, subject)) + + session._issue_admin_challenge = issue + session._has_admin_authority = lambda: True + return issued + + +async def test_changing_a_roots_flags_needs_a_signature(tmp_path): + """The flags are not applied by the request — only by the signed response.""" + session = _session(tmp_path, "the-operator", operator="the-operator") + issued = _capture_challenges(session) + + session._do_root_update({"group_id": "g" * 32, "root_name": "shared", + "writable": False}) + + assert [op for op, _ in issued] == [OP_ROOT_UPDATE] + assert session._ctx["roots"].roots[0].writable is True, ( + "applied before it was signed") + + +async def test_the_subject_names_the_outcome_not_the_operation(tmp_path): + """ + The operator is shown the subject before signing, so it has to say what will + be true afterwards. "shared" alone would have them authorize a change they + cannot see the direction of. + """ + session = _session(tmp_path, "op", operator="op") + issued = _capture_challenges(session) + + session._do_root_update({"group_id": "g" * 32, "root_name": "shared", + "writable": True, "removable": True}) + + assert issued == [(OP_ROOT_UPDATE, "shared:rw=on,rem=on")] + + +async def test_eject_and_plug_are_signed_too(tmp_path): + """ + Hiding a group's whole library from every member is not a lesser act than + changing a flag. An unsigned one would let any member black out a group. + """ + session = _session(tmp_path, "op", operator="op") + issued = _capture_challenges(session) + + session._do_root_eject({"group_id": "g" * 32, "root_name": "shared"}) + session._do_root_plug({"group_id": "g" * 32, "root_name": "shared"}) + + assert issued == [(OP_ROOT_EJECT, "shared"), (OP_ROOT_PLUG, "shared")] + + +async def test_a_request_with_nobody_to_authorize_it_is_refused(tmp_path): + """ + An unpaired node has no key to check a signature against, so the challenge + is never issued rather than issued and then unverifiable. + """ + session = _session(tmp_path, "member-1") + issued = _capture_challenges(session) + session._has_admin_authority = lambda: False + + session._do_root_update({"group_id": "g" * 32, "root_name": "shared", + "writable": True}) + + assert issued == [] + assert [m for m in session.sent if m.get("type") == "error"] + + +# ── The deprecated message must not still work ─────────────────────────────── + +async def test_the_old_member_upload_message_changes_nothing(tmp_path): + """ + MNP still parses `member_upload` so an old client gets an answer instead of + a dropped request. What it must not do is act: this instruction could + reopen uploads for a whole group, and a client old enough to send it is + exactly one that knows nothing about read-only roots. + """ + session = _session(tmp_path, "member-1", writable=False) + session._has_admin_authority = lambda: True + issued = _capture_challenges(session) + + session._do_member_upload({"allowed": True}) + + assert issued == [], "a deprecated instruction asked to be signed" + assert session._ctx["roots"].roots[0].writable is False + acks = [m for m in session.sent if m.get("type") == MNP.MEMBER_UPLOAD_ACK] + assert acks and acks[0].get("deprecated") is True + + # And the door is still shut. + _upload(session) + refusal = [m for m in session.sent if m.get("type") == "error"] + assert refusal and refusal[0].get("code") == "root_read_only" -- cgit v1.2.3 From f3fb449f3a943096a2569dc383f2819a612bccd5 Mon Sep 17 00:00:00 2001 From: Christophe Besson Date: Sun, 6 Sep 2026 22:06:45 +0200 Subject: feat(node): an upload lands in the folder it was sent to MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There is no `uploads/` subdirectory any more, and the client names the folder rather than the root. It was the last of v5's quarantine — the per-user layer went on 2026-08-14 for the same reason — and it goes on the same grounds: a folder appearing beside the operator's library because somebody sent a file is the node deciding how their disk is arranged. Somebody dropping a file into the folder they are looking at expects it to be in that folder. **What made the quarantine worth having was never the subdirectory.** It is the filename allowlist, the size cap, the chunk ordering and the no-overwrite rule, and all four are untouched: an existing file is never replaced, the second sender of IMG_1234.jpg gets a free name, and the check still sits at the write. Letting the client choose the destination is safe for one reason and only one: it is resolved through `RootSet.resolve()`, which refuses `..`, absolute segments and anything whose resolved form escapes its root, symlinks included. A member answers "which of this group's folders", never "which path on the operator's disk" — and the test that used to assert the node chose now asserts that, with six shapes of escape. `direct` goes with it. Its only job was to say "no subdirectory for this root", which is now every root, and a config flag that does nothing is worse than none. Chat's attachment folder finally does something: the directory the operator picks in the Chat settings pane is where attachments are written, falling back to the first writable root while they have not chosen one, or if the one they chose has since been made read-only or ejected — a stale choice should not become a refusal at send time. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011pvMdvLBG92jyhvD5pD6us --- docs/meshbay-draft-v6.md | 14 +++- .../meshbay-hub/src/meshbay_hub/static/chat-app.js | 10 ++- .../src/meshbay_hub/static/files-app.js | 13 ++-- .../src/meshbay_hub/static/group-page.js | 13 +++- .../src/meshbay_hub/static/transport.js | 18 +++-- .../tests/test_upload_controls_hidden.py | 7 +- packages/meshbay-node/src/meshbay_node/config.py | 3 +- packages/meshbay-node/src/meshbay_node/ops.py | 3 +- packages/meshbay-node/src/meshbay_node/roots.py | 6 +- .../src/meshbay_node/transport/webrtc_server.py | 55 +++++++++---- .../tests/test_root_writable_policy.py | 8 +- .../tests/test_security_regressions.py | 90 +++++++++++++--------- 12 files changed, 156 insertions(+), 84 deletions(-) (limited to 'packages/meshbay-node/tests/test_root_writable_policy.py') diff --git a/docs/meshbay-draft-v6.md b/docs/meshbay-draft-v6.md index 4439cd1..28aea0c 100644 --- a/docs/meshbay-draft-v6.md +++ b/docs/meshbay-draft-v6.md @@ -74,12 +74,22 @@ v5 confines uploads to `shared_root/uploads/` with a filename allowlist, no overwrite, chunk ordering and a size cap. All four protections stand. Two amendments: -- There is no single `shared_root`. **Each root is read-only or read-write**, and the - quarantine lives inside whichever writable root the upload is addressed to. If that +- There is no single `shared_root`. **Each root is read-only or read-write**, and an + upload goes to the folder the sender is looking at, inside a writable root. If that root is unavailable the upload fails with a stated reason and never falls back to another; if the group has no writable root, uploads are refused rather than guessed. (Amended 2026-09-06 — the original text designated *one* root as the upload destination, and the client named none. See `docs/refactor-groups.md` §1.1.) +- **There is no `uploads/` quarantine directory any more** (2026-09-06). It was the + last of v5's, the per-user layer having gone on 2026-08-14, and it went for the same + reason: a folder appearing beside the operator's library because somebody sent a + file is the node deciding how their disk is arranged. **What made the quarantine + worth having was never the subdirectory** — it is the filename allowlist, the size + cap, the chunk ordering and the no-overwrite rule, and all four are unchanged. + The client now names the destination folder, which is safe for one reason and only + one: it is resolved through `RootSet.resolve()`, which refuses `..`, absolute + segments and anything escaping its root, symlinks included. A member answers "which + of this group's folders", never "which path on the operator's disk". - **The no-overwrite rule is unchanged and still holds on exFAT/NTFS.** An earlier draft claimed a string comparison let `README.TXT` land on `readme.txt` there. It does not: the check is `Path.exists()`, and `stat()` is itself case-insensitive on those diff --git a/packages/meshbay-hub/src/meshbay_hub/static/chat-app.js b/packages/meshbay-hub/src/meshbay_hub/static/chat-app.js index 4714674..0a7ef26 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/chat-app.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/chat-app.js @@ -204,7 +204,8 @@ function ChatImage({ filename, entries, transportRef, gekRef }) { // root is read-only, or the one drive that was writable is unplugged — and the // paperclip says so rather than producing a refusal from the node. function ChatPanel({ transportRef, username, entries, gekRef, onRefreshIndex, - onPreview, attachRoot = '', onActivity, status }) { + onPreview, attachRoot = '', attachDir = '', + onActivity, status }) { const [messages, setMessages] = useState([]); const [hasMore, setHasMore] = useState(false); const [loadingOlder, setLoadingOlder] = useState(false); @@ -484,7 +485,10 @@ function ChatPanel({ transportRef, username, entries, gekRef, onRefreshIndex, try { // Two people sending IMG_1234.jpg both succeed; the node picks a free name // and the message has to point at the one it chose. - const ack = await transport.uploadFile(file, { root: attachRoot }); + // `attachDir` is the folder the operator chose in Settings; `attachRoot` + // is the fallback for a group where they have not chosen one yet. + const ack = await transport.uploadFile( + file, { root: attachRoot, dir: attachDir || undefined }); const storedAs = (ack && ack.stored_as) || file.name; await new Promise(r => setTimeout(r, 2500)); if (onRefreshIndex) await onRefreshIndex(); @@ -506,7 +510,7 @@ function ChatPanel({ transportRef, username, entries, gekRef, onRefreshIndex, } finally { setAttaching(false); } - }, [username, onRefreshIndex, jumpToBottom, attachRoot]); + }, [username, onRefreshIndex, jumpToBottom, attachRoot, attachDir]); const onKeyDown = useCallback((e) => { if (e.key === 'Enter' && !e.shiftKey) { diff --git a/packages/meshbay-hub/src/meshbay_hub/static/files-app.js b/packages/meshbay-hub/src/meshbay_hub/static/files-app.js index a6a3ca4..9ca3aae 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/files-app.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/files-app.js @@ -75,12 +75,12 @@ function FilesPanel({ e.target.value = ''; const transport = transportRef.current; if (!files.length || !transport || !transport.connected) return; - // The root being browsed is the destination. A group can have several - // writable roots, so leaving the node to pick one means a file uploaded - // from a folder the operator is looking at lands in a different one — - // which is only noticed much later, if at all. - const uploadRoot = currentPath ? currentPath.split('/')[0] : ''; - if (!uploadRoot) return; + // The folder on screen is the destination — not its root, and not a + // subdirectory of the node's invention. Somebody dropping a file into the + // folder they are looking at expects it to be in that folder. + const uploadDir = currentPath; + if (!uploadDir) return; + const uploadRoot = uploadDir.split('/')[0]; setError(''); for (const file of files) { @@ -92,6 +92,7 @@ function FilesPanel({ onProgress: (sent) => onProgress(sent, file.size), signal, root: uploadRoot, + dir: uploadDir, }); // The node re-indexes on a filesystem event, so there is nothing to // wait on but the clock. Refreshing here means the file appears in diff --git a/packages/meshbay-hub/src/meshbay_hub/static/group-page.js b/packages/meshbay-hub/src/meshbay_hub/static/group-page.js index 513792b..a1e6411 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/group-page.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/group-page.js @@ -590,7 +590,16 @@ function GroupPage({ groupId, group, token, username, userId, userPrefs, [nodeRoots]); const legacyNode = nodeRoots.length > 0 && nodeRoots.every((r) => r.writable === undefined); - const attachRoot = writableRoots.length ? writableRoots[0].name + // The operator's chosen attachment folder wins where there is one — that is + // what the Chat settings pane is for. Its root has to be writable and + // present, or the choice is stale (they made it read-only, or ejected the + // drive) and the fallback is better than a refusal at send time. + const chatDirRoot = chatDirectory ? chatDirectory.split('/')[0] : ''; + const chatDirUsable = Boolean( + chatDirRoot && writableRoots.some((r) => r.name === chatDirRoot)); + const attachDir = chatDirUsable ? chatDirectory : ''; + const attachRoot = chatDirUsable ? chatDirRoot + : writableRoots.length ? writableRoots[0].name : (legacyNode && memberUpload ? (nodeRoots.find((r) => r.upload) || nodeRoots[0]).name : ''); @@ -655,7 +664,7 @@ function GroupPage({ groupId, group, token, username, userId, userPrefs, groupId, transportRef, gekRef, status, username, entries, availableEntries, nodeDirs, nodeRoots, setEntries, setNodeDirs, setNodeRoots, applyIndex, - isNodeAdmin, operatorPaired, attachRoot, userId, setError, onPreview, + isNodeAdmin, operatorPaired, attachRoot, attachDir, userId, setError, onPreview, onRefreshIndex: refreshIndex, onActivity: touchActivity, // Plural everywhere: Videos and Music read a list now, and Photos always // did. The scalar `videoRoot`/`audioRoot` shapes survive only on the wire, diff --git a/packages/meshbay-hub/src/meshbay_hub/static/transport.js b/packages/meshbay-hub/src/meshbay_hub/static/transport.js index 32f8539..3acfd20 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/transport.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/transport.js @@ -1778,13 +1778,18 @@ class MeshBayTransport { * free one rather than replacing anything. The ack says which, and that is what * this returns. * - * `root` names which shared directory to upload into — a name, never a path; - * the node picks the destination inside it. Since a group can have several - * writable roots, leaving it out is a guess, and the node's fallback ("the - * first writable one") exists only for MNP 1.0 clients, which had exactly one - * destination. Every caller here browses a root and knows which one it is. + * `dir` names the folder to upload into, as a virtual path + * (`Media/Films/1999`) — where the sender is actually looking. The node + * resolves it against the group's own roots, which refuses `..`, absolute + * segments and anything escaping its root; it is a place among the group's + * folders, never a path on the operator's filesystem. + * + * `root` is the older, coarser form: the root's name and nothing below it. + * Kept because a node that predates `dir` reads it, and because Chat has no + * folder on screen to name. Omitting both leaves the node to pick, which it + * only does for a client old enough to have had one destination. */ - async uploadFile(file, { chunkSize, onProgress, signal, root } = {}) { + async uploadFile(file, { chunkSize, onProgress, signal, root, dir } = {}) { // The same file twice at once would confuse the node, which keys its own // upload state by name — and would race for the same destination. if (this._uploaders.has(file.name)) { @@ -1836,6 +1841,7 @@ class MeshBayTransport { total_chunks: total, data: buf, ...(root ? { root } : {}), + ...(dir ? { dir } : {}), }); } while (acked < total) { diff --git a/packages/meshbay-hub/tests/test_upload_controls_hidden.py b/packages/meshbay-hub/tests/test_upload_controls_hidden.py index ae1f444..7b8b0e3 100644 --- a/packages/meshbay-hub/tests/test_upload_controls_hidden.py +++ b/packages/meshbay-hub/tests/test_upload_controls_hidden.py @@ -125,8 +125,11 @@ def test_files_uploads_into_the_root_it_is_showing(): page = _component(FILES_APP.read_text(encoding="utf-8"), "FilesPanel") upload = page[page.index("const uploadFile"):] upload = upload[:upload.index("const makeDirectory")] - assert "root: uploadRoot" in upload, "the node is left to choose" - assert "currentPath.split('/')[0]" in upload + assert "dir: uploadDir" in upload, "the node is left to choose the folder" + assert "const uploadDir = currentPath" in upload, ( + "the destination is not the folder on screen") + assert "root: uploadRoot" in upload, ( + "a node too old for `dir` reads `root`, and gets nothing without it") # ── Learning the answer ───────────────────────────────────────────────────── diff --git a/packages/meshbay-node/src/meshbay_node/config.py b/packages/meshbay-node/src/meshbay_node/config.py index 4712312..7673a51 100644 --- a/packages/meshbay-node/src/meshbay_node/config.py +++ b/packages/meshbay-node/src/meshbay_node/config.py @@ -184,7 +184,6 @@ class RootSpec: kind: str = "generic" # generic|video|audio|photo — a view hint, unused for now writable: bool = False # RW roots accept uploads from group members removable: bool = False # operator can eject this root before unplugging the device - direct: bool = False # uploads land at root path, not in a subdirectory @dataclass @@ -225,7 +224,7 @@ class GroupConfig: for r in self.roots: r.writable = False self.roots.append(RootSpec( - path=self.upload_dir.strip(), writable=True, direct=True)) + path=self.upload_dir.strip(), writable=True)) @dataclass diff --git a/packages/meshbay-node/src/meshbay_node/ops.py b/packages/meshbay-node/src/meshbay_node/ops.py index 5b10452..3dfdc23 100644 --- a/packages/meshbay-node/src/meshbay_node/ops.py +++ b/packages/meshbay-node/src/meshbay_node/ops.py @@ -679,8 +679,7 @@ async def add_root(state: dict, group_id: str, path: str, *, from meshbay_node.config import RootSpec cfg.roots.append(RootSpec( path=str(added.path), name=added.name, kind=added.kind, - writable=added.writable, removable=added.removable, - direct=added.direct)) + writable=added.writable, removable=added.removable)) # Deliberately *not* mutating the live RootSet in place. # diff --git a/packages/meshbay-node/src/meshbay_node/roots.py b/packages/meshbay-node/src/meshbay_node/roots.py index 9d3f7cb..ffe801c 100644 --- a/packages/meshbay-node/src/meshbay_node/roots.py +++ b/packages/meshbay-node/src/meshbay_node/roots.py @@ -122,7 +122,6 @@ class Root: kind: str = "generic" writable: bool = False removable: bool = False - direct: bool = False ejected: bool = False available: bool = True @@ -226,8 +225,7 @@ class RootSet: writable=writable, removable=bool(spec.get("removable", False)), ejected=bool(spec.get("ejected", False)), - available=not bool(spec.get("ejected", False)), - direct=bool(spec.get("direct", False))) + available=not bool(spec.get("ejected", False))) _refuse_nesting(root, roots) roots.append(root) by_folded[root.folded] = root @@ -370,8 +368,6 @@ class RootSet: "ejected": r.ejected, # Backward compat for MNP 1.0 clients "upload": r.writable} - if r.direct: - d["direct"] = True out.append(d) return out diff --git a/packages/meshbay-node/src/meshbay_node/transport/webrtc_server.py b/packages/meshbay-node/src/meshbay_node/transport/webrtc_server.py index d341d8c..61458f2 100644 --- a/packages/meshbay-node/src/meshbay_node/transport/webrtc_server.py +++ b/packages/meshbay-node/src/meshbay_node/transport/webrtc_server.py @@ -214,7 +214,6 @@ JOIN_FAILURE_WINDOW = 600 # seconds # attachments from the chat alike. One visible directory the operator can look # into, back up or empty — rather than a hidden tree of per-user uuids that # nobody could read, or files scattered wherever someone happened to be looking. -UPLOAD_DIR_NAME = "uploads" def _extract_dtls_fingerprint(sdp: str) -> bytes: @@ -4016,7 +4015,12 @@ class WebRTCPeerSession: # later — the same reason the old single upload root was never guessed. # A client that names nothing is an MNP 1.0 one, and there was exactly # one destination in its world: the first writable root. - target_root_name = str(msg.get("root") or "").strip() + # `dir` is the folder being browsed, as a virtual path + # (`Media/Films/1999`); `root` is the older, coarser form and is what + # its first segment means on its own. + target_rel = str(msg.get("dir") or "").strip().strip("/") + target_root_name = (target_rel.split("/")[0] if target_rel + else str(msg.get("root") or "").strip()) upload_root = None if target_root_name: upload_root = roots.by_name(target_root_name) @@ -4052,18 +4056,43 @@ class WebRTCPeerSession: "filename": filename}) return - if upload_root.direct: - rel_dir = upload_root.name - target_dir = upload_root.path + # The folder the sender is looking at, and no subdirectory of the node's + # invention. + # + # Uploads used to be confined to `/uploads/`, created on demand. + # That was the last of v5's quarantine (the per-user layer went on + # 2026-08-14, for the same reason): a shared directory nobody can + # organise is not a shared directory, and a folder appearing beside the + # operator's library because somebody sent a file is the node deciding + # how their disk is arranged. + # + # What made the quarantine worth having is not the subdirectory — it is + # the filename allowlist, the size cap, the chunk ordering, and the + # no-overwrite rule below. All four are unchanged. + # + # `resolve()` and not a join: it refuses `..`, absolute segments and + # anything whose resolved form escapes its root, symlinks included. The + # client names *where among the group's own folders*, never a path on + # the operator's filesystem. + if target_rel: + target_dir = roots.resolve(target_rel) + if target_dir is None or not target_dir.is_dir(): + self._send({"type": "error", + "detail": "Not a directory in this group", + "code": "no_such_directory", + "filename": filename}) + return + rel_dir = target_rel else: - rel_dir = f"{upload_root.name}/{UPLOAD_DIR_NAME}" - target_dir = upload_root.path / UPLOAD_DIR_NAME - try: - target_dir.mkdir(parents=True, exist_ok=True) - except OSError as e: - log.warning("Cannot create upload folder in root %r: %s", - upload_root.name, e) - self._send({"type": "error", "detail": "Upload folder unavailable", + # An MNP 1.0 client names nothing; the root itself is where its one + # destination now is. + target_dir = upload_root.path + rel_dir = upload_root.name + if not target_dir.is_dir(): + self._send({"type": "error", + "detail": f"Directory '{upload_root.name}' is " + f"currently unavailable", + "code": "root_unavailable", "filename": filename}) return diff --git a/packages/meshbay-node/tests/test_root_writable_policy.py b/packages/meshbay-node/tests/test_root_writable_policy.py index da95032..d7f2666 100644 --- a/packages/meshbay-node/tests/test_root_writable_policy.py +++ b/packages/meshbay-node/tests/test_root_writable_policy.py @@ -62,14 +62,16 @@ def _session(tmp_path: Path, user_id: str, *, def _upload(session, filename="clip.mp4", body=b"bytes"): session._do_file_upload({ - "filename": filename, "root": "shared", + "filename": filename, "dir": "shared", "chunk_index": 0, "total_chunks": 1, "data": base64.b64encode(body).decode(), }) def _uploads_dir(session) -> Path: - return session._ctx["roots"].roots[0].path / "uploads" + # The root itself: the `uploads/` subdirectory the node used to create is + # gone (see test_security_regressions._uploads_dir for why). + return session._ctx["roots"].roots[0].path # ── The door, not the button ───────────────────────────────────────────────── @@ -80,7 +82,7 @@ async def test_a_member_cannot_upload_to_a_read_only_root(tmp_path): refusal = [m for m in session.sent if m.get("type") == "error"] assert refusal and refusal[0].get("code") == "root_read_only" - assert not _uploads_dir(session).exists() + assert not (_uploads_dir(session) / "clip.mp4").exists() async def test_members_upload_normally_to_a_writable_root(tmp_path): diff --git a/packages/meshbay-node/tests/test_security_regressions.py b/packages/meshbay-node/tests/test_security_regressions.py index 9db8ac1..1a318f7 100644 --- a/packages/meshbay-node/tests/test_security_regressions.py +++ b/packages/meshbay-node/tests/test_security_regressions.py @@ -133,14 +133,22 @@ 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 its first writable root. + Where an unaddressed upload lands: the first writable root itself. + + There is no `uploads/` subdirectory any more. It was the last of v5's + quarantine — the per-user layer went on 2026-08-14 — and it went for the + same reason: a folder appearing beside the operator's library because + somebody sent a file is the node deciding how their disk is arranged. The + protections that made the quarantine worth having are the allowlist, the + size cap, the chunk ordering and the no-overwrite rule, and every one of + them is asserted below, unchanged. 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. """ writable = session._ctx["roots"].writable_roots assert writable, "the fixture must give the group a writable root" - return writable[0].path / "uploads" + return writable[0].path def _session(tmp_path: Path, user_id: str) -> WebRTCPeerSession: @@ -176,7 +184,6 @@ def test_upload_cannot_overwrite_another_members_file(tmp_path): """ victim = _session(tmp_path, "victim-user") uploads = _uploads_dir(victim) - uploads.mkdir() original = uploads / "important.mp4" original.write_bytes(b"operator's original content") @@ -226,50 +233,57 @@ def test_dir_create_cannot_escape_the_shared_root(tmp_path, bad): assert set(tmp_path.rglob("*")) == before, f"created something via {bad!r}" -def test_upload_ignores_any_directory_the_client_asks_for(tmp_path): +def test_the_client_names_a_folder_and_never_a_filesystem_path(tmp_path): """ - The destination inside a root is the node's decision, and stays so. + The destination is now the folder the sender is looking at, which means the + client does choose it — and the whole of what keeps that safe is that the + choice is *resolved against the group's own roots* rather than joined to + one. - 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. + `RootSet.resolve()` refuses `..`, absolute segments and anything whose + resolved form escapes its root, symlinks included. So "which of this + group's folders" is answerable by a member and "which path on the + operator's disk" is not. """ session = _session(tmp_path, "user-1") + (session._ctx["roots"].roots[0].path / "sub").mkdir() + before = set(tmp_path.rglob("*")) - session._do_file_upload({ - "filename": "note.txt", "dir": "../../etc", "path": "/etc", - "chunk_index": 0, "total_chunks": 1, - "data": base64.b64encode(b"x").decode(), - }) + for bad in ("../../etc", "/etc", "shared/../..", "shared/../../etc", + "nope", "shared/missing"): + session.sent.clear() + session._do_file_upload({ + "filename": "note.txt", "dir": bad, + "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, f"{bad!r} was accepted" + assert refusal[0].get("code") in ("no_such_root", "no_such_directory"), bad - assert (_uploads_dir(session) / "note.txt").read_bytes() == b"x" - assert not (tmp_path / "etc").exists() + assert set(tmp_path.rglob("*")) == before, "a refused upload still wrote" -@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): +def test_an_upload_lands_in_the_folder_it_names(tmp_path): """ - 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. + And in that folder itself — the `uploads/` subdirectory the node used to + create is gone. Somebody dropping a file into the folder they are looking + at expects it to be in that folder. """ session = _session(tmp_path, "user-1") - before = set(tmp_path.rglob("*")) + root = session._ctx["roots"].roots[0] + (root.path / "Albums").mkdir() session._do_file_upload({ - "filename": "note.txt", "root": named_root, + "filename": "note.txt", "dir": f"{root.name}/Albums", "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}" + assert (root.path / "Albums" / "note.txt").read_bytes() == b"x" + assert not (root.path / "Albums" / "uploads").exists(), ( + "the node invented a subdirectory in the operator's library") + assert not (root.path / "uploads").exists() def test_an_upload_goes_to_the_root_it_names(tmp_path): @@ -291,13 +305,13 @@ def test_an_upload_goes_to_the_root_it_names(tmp_path): ]) session._do_file_upload({ - "filename": "note.txt", "root": "Incoming", + "filename": "note.txt", "dir": "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" + assert (incoming / "note.txt").read_bytes() == b"x" + assert not (media / "note.txt").exists(), "it went to the first root instead" def test_a_read_only_root_refuses_an_upload(tmp_path): @@ -314,14 +328,14 @@ def test_a_read_only_root_refuses_an_upload(tmp_path): session._is_node_admin = lambda: True session._do_file_upload({ - "filename": "note.txt", "root": "Published", + "filename": "note.txt", "dir": "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() + assert not (published / "note.txt").exists() def test_a_fully_read_only_group_refuses_an_unaddressed_upload(tmp_path): @@ -343,7 +357,7 @@ def test_a_fully_read_only_group_refuses_an_unaddressed_upload(tmp_path): 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() + assert not (published / "note.txt").exists() def test_an_ejected_root_refuses_an_upload(tmp_path): @@ -362,14 +376,14 @@ def test_an_ejected_root_refuses_an_upload(tmp_path): session._ctx["roots"] = roots session._do_file_upload({ - "filename": "note.txt", "root": "USB", + "filename": "note.txt", "dir": "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() + assert not (usb / "note.txt").exists() def test_two_members_can_send_the_same_filename(tmp_path): -- cgit v1.2.3 From 920284009d634cb568f95b3e93b93012c4b803bb Mon Sep 17 00:00:00 2001 From: Christophe Besson Date: Sun, 6 Sep 2026 23:42:59 +0200 Subject: feat(client): bring back New folder, icon-only — and close the hole it opened MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The control was hidden and its `canCreateDir` left computed and unused. It is back in the Files toolbar as an icon: the toolbar already carries one labelled primary action, and a second beside it competes for the width the breadcrumb trail needs. The name is in `title` *and* `aria-label` — a title is invisible to a screen reader on a button with no text, so an icon-only control without both is simply unnamed for anyone not reading with their eyes. Its gate changes. It required `isNodeAdmin`, which contradicted the node's own rule — "making a directory is not a privileged act; a member who can add a file can organise where it goes" — and hid the control from everyone who could have used it. It now follows the Upload button: a writable root, and not at the top of a group, where the level is the set of roots rather than a directory on anyone's disk. Restoring it surfaced a real gap. `_do_dir_create` never learned about RO/RW: `_do_file_upload` gained the `writable` check with the model and this one did not, so a member refused a file in a published library could still leave empty directories all through it, and could write to a drive mid-eject. Read-only has to mean read-only for every way of writing, not just for files. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011pvMdvLBG92jyhvD5pD6us --- .../src/meshbay_hub/static/files-app.js | 19 +++++++++- .../meshbay-hub/src/meshbay_hub/static/style.css | 4 +++ .../tests/test_upload_controls_hidden.py | 30 ++++++++++++++++ .../src/meshbay_node/transport/webrtc_server.py | 22 ++++++++++++ .../tests/test_root_writable_policy.py | 41 ++++++++++++++++++++++ 5 files changed, 115 insertions(+), 1 deletion(-) (limited to 'packages/meshbay-node/tests/test_root_writable_policy.py') diff --git a/packages/meshbay-hub/src/meshbay_hub/static/files-app.js b/packages/meshbay-hub/src/meshbay_hub/static/files-app.js index 9ca3aae..fb57a0f 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/files-app.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/files-app.js @@ -234,7 +234,13 @@ function FilesPanel({ // set of roots, which is the operator's configuration and not a directory on // anyone's disk. The node refuses it, so offering it would only produce an // error nobody can act on. - const canCreateDir = Boolean(currentPath) && isNodeAdmin; + // + // Otherwise the rule is the same as the Upload button's, and for the same + // reason the node gives: "making a directory is not a privileged act — a + // member who can add a file can organise where it goes". It used to require + // `isNodeAdmin`, which contradicted the node and hid the control from + // everyone who could actually use it. + const canCreateDir = Boolean(currentPath) && currentRootWritable && !readOnly; const breadcrumbs = currentPath ? currentPath.split('/') : []; @@ -363,6 +369,17 @@ function FilesPanel({ onChange=${uploadFile} /> `} + ${/* Icon only: the toolbar already carries a labelled primary + action, and a second one beside it competes with it for the + width a breadcrumb trail needs. The name lives in the tooltip + and in aria-label, so it is not lost to anyone reading with + something other than their eyes. */''} + ${canCreateDir && html` + + `}
diff --git a/packages/meshbay-hub/src/meshbay_hub/static/style.css b/packages/meshbay-hub/src/meshbay_hub/static/style.css index 0441e14..4647209 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/style.css +++ b/packages/meshbay-hub/src/meshbay_hub/static/style.css @@ -833,6 +833,10 @@ button:disabled { opacity: 0.5; cursor: not-allowed; } .tb-btn:disabled { opacity: 0.45; cursor: not-allowed; } .tb-btn:disabled:hover { border-color: var(--border); color: var(--text); } .tb-btn .icon { width: 15px; height: 15px; } +/* Square, for a toolbar button whose name is in its tooltip rather than + beside it. The horizontal padding goes; the height does not, so it lines up + with the labelled buttons next to it. */ +.tb-btn-icon { padding: 0; width: 32px; justify-content: center; gap: 0; } .tb-btn.primary { background: var(--accent); border-color: var(--accent); diff --git a/packages/meshbay-hub/tests/test_upload_controls_hidden.py b/packages/meshbay-hub/tests/test_upload_controls_hidden.py index 7b8b0e3..a61de1f 100644 --- a/packages/meshbay-hub/tests/test_upload_controls_hidden.py +++ b/packages/meshbay-hub/tests/test_upload_controls_hidden.py @@ -73,6 +73,36 @@ def test_the_files_upload_button_is_not_offered_at_the_top_of_a_group(): assert "currentPath &&" in toolbar +def test_the_new_folder_button_follows_the_same_rule_as_upload(): + """ + Both write to the operator's disk, so both need a writable root — the node + refuses either otherwise. It used to require `isNodeAdmin`, which + contradicted the node ("a member who can add a file can organise where it + goes") and hid the control from everyone who could have used it. + """ + page = _component(FILES_APP.read_text(encoding="utf-8"), "FilesPanel") + decl = page[page.index("const canCreateDir"):] + decl = decl[:decl.index(";") + 1] + assert "currentRootWritable" in decl + assert "currentPath" in decl, ( + "the top of a group is the set of roots, not a directory to create in") + assert "isNodeAdmin" not in decl + + +def test_an_icon_only_button_still_says_what_it_is(): + """ + The name moved into a tooltip to save toolbar width. A `title` is invisible + to a screen reader on a button with no text, so the label has to be there + as well — otherwise the control is simply unnamed for anyone not reading + with their eyes. + """ + page = _component(FILES_APP.read_text(encoding="utf-8"), "FilesPanel") + block = page[page.index("canCreateDir && html`"):] + block = block[:block.index("")] + assert "title=" in block and "aria-label=" in block + assert "group.mkdir" in block + + def test_the_chat_composer_hides_its_paperclip(): chat = _component(CHAT_APP.read_text(encoding="utf-8"), "ChatPanel") composer = chat[chat.index("chat-input-row"):] diff --git a/packages/meshbay-node/src/meshbay_node/transport/webrtc_server.py b/packages/meshbay-node/src/meshbay_node/transport/webrtc_server.py index 61458f2..0e2d418 100644 --- a/packages/meshbay-node/src/meshbay_node/transport/webrtc_server.py +++ b/packages/meshbay-node/src/meshbay_node/transport/webrtc_server.py @@ -1595,6 +1595,28 @@ class WebRTCPeerSession: "detail": "Choose a folder to create this in"}) return + # Read-only means read-only, and creating a folder writes to the + # operator's disk. `_do_file_upload` gained this check with the RO/RW + # model and this one did not — so a member could not add a file to a + # published library but could still leave empty directories in it. + owner = roots.split(parent_rel) + if owner is None: + self._send({"type": "error", "detail": "Invalid directory"}) + return + parent_root, _tail = owner + if not parent_root.writable: + self._send({"type": "error", + "detail": f"Directory '{parent_root.name}' is read-only", + "code": "root_read_only"}) + self._audit("dir_create_refused", parent_rel[:64]) + return + if not parent_root.available: + self._send({"type": "error", + "detail": f"Directory '{parent_root.name}' is " + f"currently unavailable", + "code": "root_unavailable"}) + return + parent = safe_subdir(roots, parent_rel) if parent is None or not parent.is_dir(): self._send({"type": "error", "detail": "Invalid directory"}) diff --git a/packages/meshbay-node/tests/test_root_writable_policy.py b/packages/meshbay-node/tests/test_root_writable_policy.py index d7f2666..7eb75fd 100644 --- a/packages/meshbay-node/tests/test_root_writable_policy.py +++ b/packages/meshbay-node/tests/test_root_writable_policy.py @@ -108,6 +108,47 @@ async def test_read_only_binds_the_operator_too(tmp_path): assert refusal and refusal[0].get("code") == "root_read_only" +async def test_a_member_cannot_create_a_folder_in_a_read_only_root(tmp_path): + """ + Read-only has to mean read-only for every way of writing, not just for + files. `_do_file_upload` gained this check with the RO/RW model and + `_do_dir_create` did not, so a member refused a file in a published library + could still leave empty directories all through it. + + Creating a folder stays unprivileged — the node's own words: "a member who + can add a file can organise where it goes". What changed is that it now + requires the same root to be writable that adding the file would have. + """ + session = _session(tmp_path, "member-1", writable=False) + session._do_dir_create({"dir": "shared", "name": "New folder"}) + + refusal = [m for m in session.sent if m.get("type") == "error"] + assert refusal and refusal[0].get("code") == "root_read_only" + assert not (tmp_path / "shared" / "New folder").exists() + + +async def test_a_member_can_create_a_folder_in_a_writable_root(tmp_path): + """The counter-property: it must stay unprivileged where it is allowed.""" + session = _session(tmp_path, "member-1", writable=True) + session._do_dir_create({"dir": "shared", "name": "New folder"}) + + assert not [m for m in session.sent if m.get("type") == "error"] + assert (tmp_path / "shared" / "New folder").is_dir() + + +async def test_an_ejected_root_refuses_a_new_folder(tmp_path): + """Writing to a drive somebody has their hand on, one level up from a file.""" + session = _session(tmp_path, "member-1", writable=True) + roots = session._ctx["roots"] + roots.roots[0].ejected = True + roots.roots[0].available = False + + session._do_dir_create({"dir": "shared", "name": "New folder"}) + refusal = [m for m in session.sent if m.get("type") == "error"] + assert refusal and refusal[0].get("code") == "root_unavailable" + assert not (tmp_path / "shared" / "New folder").exists() + + # ── Signed, or it is a suggestion ──────────────────────────────────────────── def _capture_challenges(session) -> list[tuple[str, str]]: -- cgit v1.2.3