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 --- docs/meshbay-draft-v6.md | 37 ++++++++++++++++++++++++++++++++----- 1 file changed, 32 insertions(+), 5 deletions(-) (limited to 'docs/meshbay-draft-v6.md') diff --git a/docs/meshbay-draft-v6.md b/docs/meshbay-draft-v6.md index 180050c..4439cd1 100644 --- a/docs/meshbay-draft-v6.md +++ b/docs/meshbay-draft-v6.md @@ -57,7 +57,7 @@ | 6 | Portability | exFAT/NTFS and Windows are the **common** case. Case folding and Unicode normalization become correctness requirements, not compatibility notes | E8 / decision 12 | | 7 | Accounts | Native registration is **hybrid**: passphrase-derived `auth_key` (the recovery path) plus a device Ed25519 key for day-to-day authentication | E3 / decision 4 | | 8 | Authorship | Chat senders are **cryptographically authenticated to each other**; an upload has a **provable owner** who may delete it, as the operator may. v5's node-asserted attribution is replaced | operator decision, §2.4b | -| 9 | Node authority | The operator may **close uploading to everyone but themselves**, per group. Signed MNP op, stored on the node, enforced by the node — the hidden button is a courtesy, the refusal is the control | §2.1b | +| 9 | Node authority | The operator decides **which directories accept uploads**, per root. Signed MNP op, stored on the node, enforced by the node — the hidden button is a courtesy, the refusal is the control. **Superseded 2026-09-06** by `docs/refactor-groups.md` §1.1: the group-wide `member_upload` switch this section described is replaced by RO/RW per root, and the "everyone but the operator" carve-out is gone | §2.1b | | 10 | Client | A group's UI is a **set of pluggable applications** (Chat, Files today), not one monolithic page. Which are shown is a per-group, operator-signed setting on the same pattern as change 9 | §2.7 | | 11 | Hub role | The hub gains a **runtime instance-policy store** (`hub_settings`). First policy: an admin switches **public groups off** hub-wide, enforced server-side on every hub-mediated path. `suspend` vs `revoke` on a group are now written down as the distinct things they are | §2.8 | | 12 | Group registry | A group name is **unique per owner account**, not globally; the group's identity is still its UUID. Listed everywhere as `name@owner` | §2.9 | @@ -74,10 +74,12 @@ 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`. **The operator designates one root as the upload - destination**; the quarantine lives inside it. If that root is unavailable the upload - fails with a stated reason and never falls back to another; if none is designated, - uploads are refused rather than guessed. +- 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 + 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.) - **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 @@ -91,6 +93,31 @@ device that asked, so the node keeps no thumbnail store. ### 2.1b §5.2 Uploads — the operator may close them +> **Superseded 2026-09-06.** `member_upload` is gone; the mechanism is `writable` on +> each root. What the three load-bearing properties below say is *unchanged* — read +> "the root's `writable` flag" for "`member_upload`" and every word of them still +> holds, which is why they are kept rather than deleted. What did change: +> +> - **It is per root, not per group.** A group can publish one library read-only and +> accept uploads into another, which the single switch could not express. +> - **There is no carve-out for the operator.** Read-only means read-only for +> everyone, because a published library that quietly accepts writes from whoever +> holds admin authority is not one. The paragraph below justifying the setting by +> "the only way to get a curated library was to designate no upload root at all, +> which refuses the operator too" is therefore the reasoning that was reversed: that +> *is* the model now, and refusing the operator is the point rather than the defect. +> - **The client names the destination root.** With several writable roots the node +> cannot choose without guessing, and a guess sends a member's file to a disk the +> operator did not intend. It names a root, never a path; everything below the root +> is still decided by the node. +> - The signed op is `OP_ROOT_UPDATE` (plus `OP_ROOT_EJECT` / `OP_ROOT_PLUG`) rather +> than `OP_MEMBER_UPLOAD`, and the flags live in `node.toml` — they are +> configuration — while the *ejected* runtime state lives in `roster.db`. +> `member_upload` survives on the handshake ack alone, computed as "any root is +> writable", for MNP 1.0 clients that read no other field. +> +> See `docs/refactor-groups.md` §1.1 and §1.5b. + New. A group where every member may add files is the default and stays the default; some groups want a library the operator curates, and until now the only way to get one was to designate no upload root at all, which refuses the operator too. -- 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 'docs/meshbay-draft-v6.md') 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