From b3ef2aff738cc4efd974efab9315a6ce6c3de493 Mon Sep 17 00:00:00 2001 From: Christophe Besson Date: Fri, 14 Aug 2026 21:39:42 +0200 Subject: feat(files): upload into the current directory, and create folders MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The per-user quarantine is gone. `.uploads/{user_id}/` was the fix for C5a, and it worked, but it made the shared directory something nobody could organise: every file landed under a uuid nobody recognises. Files now go where the member is looking, most often the root. What the quarantine actually bought is kept, and is now what the tests assert rather than the location: - an existing file is never replaced. That was the real defect — overwriting a file also made the attacker its recorded uploader, and therefore able to delete it through the uploader path - the name allowlist is unchanged - the destination is confined under the shared root That last one is new surface: the directory arrives from the client. safe_subdir() is the single place that decides, with two independent guards — every segment against the name allowlist, and the resolved result under the root — because one of them will eventually be refactored by someone who does not know why it is there. Ten traversal cases are covered, and they fail if both guards go. Also adds `dir_create` (any member may organise a shared directory; audited like anything that writes to the operator's disk) and makes the node report its real directory list in index_sync — folders were inferred from file paths, so a new empty one, or one that had been emptied, simply did not exist as far as the UI was concerned. Two C5a tests changed their assertions deliberately, as C5b's did before: they encoded the quarantine path, which is the thing being removed. The property they existed for is asserted more directly than before. Co-Authored-By: Claude Opus 5 --- .../meshbay-common/src/meshbay_common/protocol.py | 2 + packages/meshbay-hub/src/meshbay_hub/static/app.js | 124 ++++++++++++-------- .../meshbay-hub/src/meshbay_hub/static/i18n.js | 2 + .../src/meshbay_hub/static/transport.js | 15 ++- .../src/meshbay_node/transport/webrtc_server.py | 127 +++++++++++++++++++-- .../tests/test_security_regressions.py | 86 +++++++++++--- 6 files changed, 280 insertions(+), 76 deletions(-) (limited to 'packages') diff --git a/packages/meshbay-common/src/meshbay_common/protocol.py b/packages/meshbay-common/src/meshbay_common/protocol.py index 50e8663..08dc47b 100644 --- a/packages/meshbay-common/src/meshbay_common/protocol.py +++ b/packages/meshbay-common/src/meshbay_common/protocol.py @@ -35,6 +35,8 @@ class MNP: # the wire contract looking as though the endpoint still existed. FILE_UPLOAD = "file_upload" # client pushes file chunk to node FILE_UPLOAD_ACK = "file_upload_ack" # node acknowledges chunk receipt + DIR_CREATE = "dir_create" # client → node: make a directory + DIR_CREATE_ACK = "dir_create_ack" # node → client: created FILE_DELETE = "file_delete" # client requests file deletion FILE_DELETE_ACK = "file_delete_ack" # node confirms deletion STREAM_REQUEST = "stream_req" # client requests MSE video stream diff --git a/packages/meshbay-hub/src/meshbay_hub/static/app.js b/packages/meshbay-hub/src/meshbay_hub/static/app.js index cff001b..def6c5b 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/app.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/app.js @@ -824,6 +824,9 @@ function GroupPage({ groupId, group, token, username, userId, onRefreshAuth }) { const [tab, setTab] = useState('files'); const [uploading, setUploading] = useState(false); const [ulState, setUlState] = useState(null); + // Directories are not index entries, so a new empty one needs a nudge + // to appear in the breadcrumb listing. + const [nodeDirs, setNodeDirs] = useState([]); const [menuOpen, setMenuOpen] = useState(null); const [isNodeAdmin, setIsNodeAdmin] = useState(false); const [needsCode, setNeedsCode] = useState(false); @@ -916,6 +919,7 @@ function GroupPage({ groupId, group, token, username, userId, onRefreshAuth }) { if (cancelled) return; const synced = msg.entries || []; setEntries(synced); + if (msg.dirs) setNodeDirs(msg.dirs); setCached(false); cacheGroupIndex(groupId, group ? group.name : groupId, synced); }; @@ -924,6 +928,7 @@ function GroupPage({ groupId, group, token, username, userId, onRefreshAuth }) { if (cancelled) return; const freshEntries = indexMsg.entries || []; setEntries(freshEntries); + setNodeDirs(indexMsg.dirs || []); setCached(false); setStatus('connected'); @@ -1027,7 +1032,7 @@ function GroupPage({ groupId, group, token, username, userId, onRefreshAuth }) { for (let i = 0; i < totalChunks; i++) { const slice = file.slice(i * UPLOAD_CHUNK_SIZE, (i + 1) * UPLOAD_CHUNK_SIZE); const buf = new Uint8Array(await slice.arrayBuffer()); - await transport.uploadChunk(file.name, i, totalChunks, buf); + await transport.uploadChunk(file.name, i, totalChunks, buf, currentPath); // Bytes actually acknowledged by the node, not bytes read locally. setUlState(prev => prev && { ...prev, sent: Math.min(file.size, (i + 1) * UPLOAD_CHUNK_SIZE) }); @@ -1038,13 +1043,29 @@ function GroupPage({ groupId, group, token, username, userId, onRefreshAuth }) { await new Promise(r => setTimeout(r, 2500)); const indexMsg = await transport.fetchIndex(); if (indexMsg.entries) setEntries(indexMsg.entries); + if (indexMsg.dirs) setNodeDirs(indexMsg.dirs); } catch (err) { setError(err.message); } finally { setUploading(false); setUlState(null); } - }, []); + }, [currentPath]); + + const makeDirectory = useCallback(async () => { + const transport = transportRef.current; + if (!transport || !transport.connected) return; + const name = prompt(t('group.mkdir_prompt')); + if (!name || !name.trim()) return; + try { + await transport.createDirectory(currentPath, name.trim()); + const indexMsg = await transport.fetchIndex(); + if (indexMsg.entries) setEntries(indexMsg.entries); + if (indexMsg.dirs) setNodeDirs(indexMsg.dirs); + } catch (err) { + setError(err.message); + } + }, [currentPath]); const deleteFile = useCallback(async (entry) => { const transport = transportRef.current; @@ -1104,6 +1125,15 @@ function GroupPage({ groupId, group, token, username, userId, onRefreshAuth }) { return sortAsc ? cmp : -cmp; }); + // The node's own listing, so an empty folder is visible, plus anything implied + // by a file path in case the two ever disagree. + for (const d of nodeDirs) { + if (!currentPath && !d.includes('/')) dirs.add(d); + else if (currentPath && d.startsWith(currentPath + '/')) { + const rest = d.slice(currentPath.length + 1); + if (!rest.includes('/')) dirs.add(rest); + } + } const subdirs = [...dirs].sort(); const baseLabel = { @@ -1205,6 +1235,8 @@ function GroupPage({ groupId, group, token, username, userId, onRefreshAuth }) { +
setCurrentPath('')}>/ ${breadcrumbs.map((seg, i) => { @@ -1578,49 +1610,6 @@ function MembersPanel({ groupId, group, token, transportRef, gekRef, return html`
- ${isAdmin && html` -
-

${t('members.invite_title')}

- ${error && html`

${error}

`} - ${inviteCode && html` -
-

${t('members.invite_code_ready', { user: inviteCode.username })}

-

- ${inviteCode.code} -

-

${t('members.invite_code_hint')}

-
- `} -
- setInviteUser(e.target.value)} required /> - -
-
- `} - - - - - - - - - ${members.map(m => html` - - - - - `)} - -
${t('admin.col_username')}${t('members.group_role')}
${m.username} - ${m.user_id === adminId - ? html`${t('members.owner')}` - : html`${t('members.member')}` - } -
${isNodeAdmin && html`

${t('members.pair_title')}

@@ -2443,7 +2432,50 @@ function AdminPage({ token }) { value=${userSearch} onInput=${e => { setUserSearch(e.target.value); loadUsers(e.target.value); }} /> ${usersTotal} total
- + ${isAdmin && html` + +

${t('members.invite_title')}

+ ${error && html`

${error}

`} + ${inviteCode && html` +
+

${t('members.invite_code_ready', { user: inviteCode.username })}

+

+ ${inviteCode.code} +

+

${t('members.invite_code_hint')}

+
+ `} +
+ setInviteUser(e.target.value)} required /> + +
+ + `} +
+ + + + + + + + ${members.map(m => html` + + + + + `)} + +
${t('admin.col_username')}${t('members.group_role')}
${m.username} + ${m.user_id === adminId + ? html`${t('members.owner')}` + : html`${t('members.member')}` + } +
+ diff --git a/packages/meshbay-hub/src/meshbay_hub/static/i18n.js b/packages/meshbay-hub/src/meshbay_hub/static/i18n.js index e3e0def..c19411b 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/i18n.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/i18n.js @@ -89,6 +89,8 @@ const en = { 'group.offline_title': 'No nodes are currently online for this group.', 'group.offline_hint': 'Files will appear when a node hosting this group connects.', 'group.upload': 'Upload', + 'group.mkdir': 'New folder', + 'group.mkdir_prompt': 'Name of the new folder:', 'group.uploading': 'Uploading...', 'group.view': 'View', 'group.delete': 'Delete', diff --git a/packages/meshbay-hub/src/meshbay_hub/static/transport.js b/packages/meshbay-hub/src/meshbay_hub/static/transport.js index 0a8796e..d2b24a3 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/transport.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/transport.js @@ -488,15 +488,28 @@ class MeshBayTransport { this._send({ type: 'stream_req', v: '0.1', file_id: fileId }); } - async uploadChunk(filename, chunkIndex, totalChunks, data) { + async uploadChunk(filename, chunkIndex, totalChunks, data, dir) { const msg = await this._sendAndWait({ type: 'file_upload', v: '0.1', filename, + // Where the member is looking. The node confines it under the shared root + // and refuses to overwrite, so this is a destination, not a licence. + dir: dir || '', chunk_index: chunkIndex, total_chunks: totalChunks, data: data, }); + if (msg.type === 'error') throw new Error(msg.detail); + return msg; + } + + /** Create a directory under the current one. Any member may. */ + async createDirectory(dir, name) { + const msg = await this._sendAndWait({ + type: 'dir_create', v: '0.1', dir: dir || '', name, + }); + if (msg.type === 'error') throw new Error(msg.detail); return msg; } 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 fe4e3c2..66ba7ef 100644 --- a/packages/meshbay-node/src/meshbay_node/transport/webrtc_server.py +++ b/packages/meshbay-node/src/meshbay_node/transport/webrtc_server.py @@ -106,6 +106,37 @@ UPLOAD_DIR_NAME = ".uploads" SAFE_UPLOAD_NAME = re.compile(r"^[A-Za-z0-9][A-Za-z0-9._ -]{0,127}$") +def safe_subdir(shared_root: Path, rel: str) -> Path | None: + """ + Resolve a client-supplied directory under the shared root, or refuse. + + Uploads land where the member is looking now rather than in a per-user + quarantine, so the path arrives from the wire and every part of it has to be + checked: each segment against the same allowlist as filenames, and the + resolved result against the root. `..`, absolute paths, symlinks pointing + out, and anything with a separator in a segment are all refused here rather + than in the caller, so there is one place to get it right. + + The quarantine was the fix for C5a; what actually mattered in it — no + overwrite, a name allowlist, and confinement — is kept by this plus the + caller's existing checks. + """ + rel = (rel or "").strip().strip("/") + if not rel: + return shared_root + parts = [seg for seg in rel.split("/") if seg not in ("", ".")] + if any(seg == ".." or not SAFE_UPLOAD_NAME.match(seg) for seg in parts): + return None + try: + target = (shared_root / Path(*parts)).resolve() + root = shared_root.resolve() + except OSError: + return None + if target != root and root not in target.parents: + return None + return target + + def _extract_dtls_fingerprint(sdp: str) -> bytes: """Extract the DTLS SHA-256 fingerprint from SDP as raw 32 bytes.""" for line in sdp.splitlines(): @@ -301,6 +332,8 @@ class WebRTCPeerSession: self._do_chat_history(msg) elif mtype == MNP.FILE_UPLOAD: self._do_file_upload(msg) + elif mtype == MNP.DIR_CREATE: + self._do_dir_create(msg) elif mtype == MNP.FILE_DELETE: self._do_file_delete(msg) elif mtype == MNP.ADMIN_RESPONSE: @@ -834,6 +867,48 @@ class WebRTCPeerSession: self._send(reply) self._audit_join("gek_wrapped", f"group={group_id[:8]}") + def _do_dir_create(self, msg: dict) -> None: + """ + Create a directory, for any member of the group. + + Same confinement as an upload: every segment passes the name allowlist and + the result must resolve under the shared root. Making a directory is not a + privileged act — a member who can add a file can organise where it goes — + but it writes to the operator's disk, so it is audited like one. + """ + ctx = self._group_ctx() + shared_root = ctx.get("shared_root") + if not shared_root: + self._send({"type": "error", "detail": "No shared directory"}) + return + + name = str(msg.get("name", "")).strip() + if not SAFE_UPLOAD_NAME.match(name): + self._send({"type": "error", "detail": "Invalid directory name"}) + return + + parent = safe_subdir(shared_root, msg.get("dir") or "") + if parent is None or not parent.is_dir(): + self._send({"type": "error", "detail": "Invalid directory"}) + return + + target = safe_subdir(shared_root, f"{(msg.get('dir') or '').strip('/')}/{name}") + if target is None: + self._send({"type": "error", "detail": "Invalid directory"}) + return + if target.exists(): + self._send({"type": "error", "detail": "Already exists"}) + return + + target.mkdir(parents=False) + log.info("Directory created by %s: %s", self._user_id[:8], + target.relative_to(shared_root)) + self._audit("dir_create", str(target.relative_to(shared_root))) + self._send({ + "type": MNP.DIR_CREATE_ACK, "v": MNP_VERSION, + "dir": str(target.relative_to(shared_root)), + }) + async def _do_keypair_bundle_delete(self) -> None: """ Withdraw our own key backup from this node. @@ -918,8 +993,28 @@ class WebRTCPeerSession: "group_id": idx.group_id, "version": idx.version, "entries": entries, + # Directories are not index entries, so the client used to infer them + # from file paths — which means a folder someone just created, or one + # they emptied, simply did not exist as far as the UI was concerned. + "dirs": self._list_dirs(ctx.get("shared_root")), }) + @staticmethod + def _list_dirs(shared_root: Path | None) -> list[str]: + """Directories under the shared root, relative and sorted.""" + if not shared_root: + return [] + out = [] + try: + for path in sorted(shared_root.rglob("*")): + if path.is_dir() and not path.name.startswith("."): + rel = path.relative_to(shared_root) + if not any(part.startswith(".") for part in rel.parts): + out.append(str(rel)) + except OSError: + return [] + return out[:2000] + def _do_file_request(self, msg: dict) -> None: ctx = self._group_ctx() file_id = msg["file_id"] @@ -1120,21 +1215,31 @@ class WebRTCPeerSession: self._send({"type": "error", "detail": "No shared directory"}) return - # Per-user quarantine: a member can only ever write inside their own directory, - # so they cannot overwrite the operator's files or another member's (C5a). - rel_dir = f"{UPLOAD_DIR_NAME}/{self._user_id}" - user_dir = shared_root / UPLOAD_DIR_NAME / self._user_id - user_dir.mkdir(parents=True, exist_ok=True) - tmp_path = user_dir / f"{filename}.part" - final_path = user_dir / filename + # Where the member is looking, not a quarantine named after their user id. + # C5a is still honoured by what follows: the path is confined under the + # shared root (safe_subdir), the name passed the allowlist above, and an + # existing file is never overwritten — which is what made the quarantine + # necessary, since overwriting a file also made the attacker its recorded + # uploader and therefore able to delete it. + rel_dir = (msg.get("dir") or "").strip().strip("/") + target_dir = safe_subdir(shared_root, rel_dir) + if target_dir is None: + self._send({"type": "error", "detail": "Invalid directory"}) + return + if not target_dir.is_dir(): + self._send({"type": "error", "detail": "No such directory"}) + return + tmp_path = target_dir / f"{filename}.part" + final_path = target_dir / filename - state = self._uploads.get(filename) + upload_key = f"{rel_dir}/{filename}" + state = self._uploads.get(upload_key) if chunk_index == 0: if final_path.exists(): self._send({"type": "error", "detail": "File already exists"}) return state = {"next_index": 0, "bytes": 0} - self._uploads[filename] = state + self._uploads[upload_key] = state elif state is None: self._send({"type": "error", "detail": "Upload not started"}) return @@ -1151,7 +1256,7 @@ class WebRTCPeerSession: chunk_bytes = bytes(data) if state["bytes"] + len(chunk_bytes) > MAX_UPLOAD_BYTES: - self._uploads.pop(filename, None) + self._uploads.pop(upload_key, None) tmp_path.unlink(missing_ok=True) self._send({"type": "error", "detail": "Upload exceeds size limit"}) return @@ -1169,7 +1274,7 @@ class WebRTCPeerSession: }) if chunk_index + 1 >= total_chunks: - self._uploads.pop(filename, None) + self._uploads.pop(upload_key, None) tmp_path.rename(final_path) log.info("Upload complete: %s (%d chunks, %d bytes)", filename, total_chunks, state["bytes"]) diff --git a/packages/meshbay-node/tests/test_security_regressions.py b/packages/meshbay-node/tests/test_security_regressions.py index dcd9cf6..7627926 100644 --- a/packages/meshbay-node/tests/test_security_regressions.py +++ b/packages/meshbay-node/tests/test_security_regressions.py @@ -120,9 +120,14 @@ def _session(tmp_path: Path, user_id: str) -> WebRTCPeerSession: def test_upload_cannot_overwrite_another_members_file(tmp_path): """ C5a: uploads used to land in the shared root under a client-chosen name and - overwrite whatever was there. That let any member destroy the operator's files, - and — by becoming the recorded uploader of the replaced file — delete them - through the uploader path, bypassing the Ed25519 admin challenge entirely. + overwrite whatever was there. That let any member destroy the operator's + files, and — by becoming the recorded uploader of the replaced file — delete + them through the uploader path, bypassing the Ed25519 admin challenge. + + The per-user quarantine that fixed it was removed on 2026-08-14: files now go + where the member is looking, because a shared directory nobody can organise is + not a shared directory. What made the quarantine work is kept, and is what + this test now asserts — an existing file is never replaced. """ victim = _session(tmp_path, "victim-user") shared_root = victim._ctx["shared_root"] @@ -139,23 +144,14 @@ def test_upload_cannot_overwrite_another_members_file(tmp_path): }) assert original.read_bytes() == b"operator's original content" - uploaded = shared_root / ".uploads" / "attacker-user" / "important.mp4" - assert uploaded.exists(), "upload should be quarantined, not dropped" - assert uploaded.read_bytes() == b"attacker content" - - -def test_upload_rejects_out_of_order_chunks(tmp_path): - """C5a: chunk_index > 0 used to append blindly to any .part file on disk.""" - session = _session(tmp_path, "user-1") - session._do_file_upload({ - "filename": "movie.mp4", "chunk_index": 3, "total_chunks": 5, - "data": base64.b64encode(b"spliced").decode(), - }) - assert any(m.get("type") == "error" for m in session.sent) + assert any(m.get("type") == "error" for m in attacker.sent), ( + "the upload must be refused outright, not silently dropped") + assert not (shared_root / "important.mp4.part").exists(), ( + "a refused upload must leave nothing behind") def test_upload_second_attempt_cannot_replace_own_completed_file(tmp_path): - """C5a: even the original uploader goes through a fresh name, not an overwrite.""" + """C5a: even the original uploader does not get to overwrite.""" session = _session(tmp_path, "user-1") payload = {"filename": "movie.mp4", "chunk_index": 0, "total_chunks": 1, "data": base64.b64encode(b"first").decode()} @@ -164,10 +160,64 @@ def test_upload_second_attempt_cannot_replace_own_completed_file(tmp_path): session._do_file_upload(dict(payload)) assert any(m.get("type") == "error" for m in session.sent) - stored = session._ctx["shared_root"] / ".uploads" / "user-1" / "movie.mp4" + stored = session._ctx["shared_root"] / "movie.mp4" assert stored.read_bytes() == b"first" +@pytest.mark.parametrize("bad_dir", [ + "..", "../..", "/etc", "a/../../b", "./../x", "sub/../../..", + "\\..\\..", "~", "a/./../..", +]) +def test_upload_cannot_escape_the_shared_root(tmp_path, bad_dir): + """ + The destination now arrives from the client, which is a path the node did not + choose. Every segment goes through the same allowlist as a filename and the + result must resolve inside the shared root. + """ + session = _session(tmp_path, "user-1") + outside = tmp_path / "outside.txt" + + session._do_file_upload({ + "filename": "outside.txt", "dir": bad_dir, + "chunk_index": 0, "total_chunks": 1, + "data": base64.b64encode(b"escaped").decode(), + }) + + assert any(m.get("type") == "error" for m in session.sent), bad_dir + assert not outside.exists(), f"upload escaped the shared root via {bad_dir!r}" + + +@pytest.mark.parametrize("bad", [ + {"dir": "..", "name": "evil"}, + {"dir": "", "name": ".."}, + {"dir": "", "name": "a/b"}, + {"dir": "/etc", "name": "evil"}, + {"dir": "", "name": ".hidden"}, +]) +def test_dir_create_cannot_escape_the_shared_root(tmp_path, bad): + """Creating a directory is not privileged, but it still writes to a disk.""" + session = _session(tmp_path, "user-1") + before = set(tmp_path.rglob("*")) + + session._do_dir_create(bad) + + assert any(m.get("type") == "error" for m in session.sent), bad + assert set(tmp_path.rglob("*")) == before, f"created something via {bad!r}" + + +def test_upload_still_refuses_unsafe_names_in_a_subdirectory(tmp_path): + """The name allowlist is not weakened by having somewhere to put the file.""" + session = _session(tmp_path, "user-1") + (session._ctx["shared_root"] / "docs").mkdir() + + session._do_file_upload({ + "filename": "../escape.txt", "dir": "docs", + "chunk_index": 0, "total_chunks": 1, + "data": base64.b64encode(b"x").decode(), + }) + assert any(m.get("type") == "error" for m in session.sent) + + # ── H1: group isolation ────────────────────────────────────────────────────── def test_chat_store_and_peers_are_per_group(tmp_path): -- cgit v1.2.3
${t('admin.col_username')} ${t('admin.col_role')}