diff options
4 files changed, 107 insertions, 66 deletions
diff --git a/packages/meshbay-hub/src/meshbay_hub/static/app.js b/packages/meshbay-hub/src/meshbay_hub/static/app.js index def6c5b..d8b83ed 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/app.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/app.js @@ -1032,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, currentPath); + await transport.uploadChunk(file.name, i, totalChunks, buf); // Bytes actually acknowledged by the node, not bytes read locally. setUlState(prev => prev && { ...prev, sent: Math.min(file.size, (i + 1) * UPLOAD_CHUNK_SIZE) }); @@ -1050,7 +1050,7 @@ function GroupPage({ groupId, group, token, username, userId, onRefreshAuth }) { setUploading(false); setUlState(null); } - }, [currentPath]); + }, []); const makeDirectory = useCallback(async () => { const transport = transportRef.current; @@ -1760,10 +1760,14 @@ function ChatPanel({ transportRef, username, entries, gekRef, onRefreshIndex, on setAttaching(true); try { const totalChunks = Math.ceil(file.size / UPLOAD_CHUNK_SIZE); + let storedAs = file.name; 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); + const ack = await transport.uploadChunk(file.name, i, totalChunks, buf); + // 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. + if (ack && ack.stored_as) storedAs = ack.stored_as; } await new Promise(r => setTimeout(r, 2500)); if (onRefreshIndex) await onRefreshIndex(); @@ -1771,7 +1775,7 @@ function ChatPanel({ transportRef, username, entries, gekRef, onRefreshIndex, on const ftype = ['jpg','jpeg','png','gif','webp','svg'].includes(ext) ? 'image' : ['mp4','webm','mkv','mov','avi'].includes(ext) ? 'video' : 'file'; const structured = JSON.stringify({ - text: '', attachment: { filename: file.name, size: file.size, type: ftype }, + text: '', attachment: { filename: storedAs, size: file.size, type: ftype }, }); await transport.sendChat(structured, 0, null, username); setMessages(prev => [...prev, { diff --git a/packages/meshbay-hub/src/meshbay_hub/static/transport.js b/packages/meshbay-hub/src/meshbay_hub/static/transport.js index d2b24a3..0306a5c 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/transport.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/transport.js @@ -488,14 +488,13 @@ class MeshBayTransport { this._send({ type: 'stream_req', v: '0.1', file_id: fileId }); } - async uploadChunk(filename, chunkIndex, totalChunks, data, dir) { + async uploadChunk(filename, chunkIndex, totalChunks, data) { + // The node decides where this lands (uploads/) and under what name — it + // finds a free one rather than replacing anything. The ack says which. 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, 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 66ba7ef..483a6a7 100644 --- a/packages/meshbay-node/src/meshbay_node/transport/webrtc_server.py +++ b/packages/meshbay-node/src/meshbay_node/transport/webrtc_server.py @@ -100,12 +100,35 @@ MAX_JOIN_ATTEMPTS = 5 # failed pairings are also counted node-wide over a window. MAX_JOIN_FAILURES_WINDOW = 20 JOIN_FAILURE_WINDOW = 600 # seconds -UPLOAD_DIR_NAME = ".uploads" +# Everything a member sends lands here: files from the Files panel and +# 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" # Conservative allowlist: also what keeps markup out of filenames, which the node admin # UI used to render unescaped (finding H2). SAFE_UPLOAD_NAME = re.compile(r"^[A-Za-z0-9][A-Za-z0-9._ -]{0,127}$") +def _free_name(directory: Path, filename: str) -> str: + """ + `filename`, or the first "name (n).ext" that is not taken. + + Never returns the name of a file that exists, so an upload cannot replace + one — the property the per-user quarantine used to provide (C5a). + """ + if not (directory / filename).exists(): + return filename + stem, dot, ext = filename.rpartition(".") + if not dot: + stem, ext = filename, "" + for n in range(2, 1000): + candidate = f"{stem} ({n}){dot}{ext}" + if not (directory / candidate).exists(): + return candidate + raise FileExistsError(filename) + + def safe_subdir(shared_root: Path, rel: str) -> Path | None: """ Resolve a client-supplied directory under the shared root, or refuse. @@ -1215,30 +1238,31 @@ class WebRTCPeerSession: self._send({"type": "error", "detail": "No shared directory"}) return - # 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 + # One destination, chosen here and not by the client: uploads/ at the root + # of the shared directory. C5a is still honoured — the name passed the + # allowlist above, and an existing file is never replaced, which was the + # real defect (overwriting a file also made the attacker its recorded + # uploader, and therefore able to delete it). + rel_dir = UPLOAD_DIR_NAME + target_dir = shared_root / UPLOAD_DIR_NAME + target_dir.mkdir(parents=True, exist_ok=True) upload_key = f"{rel_dir}/{filename}" state = self._uploads.get(upload_key) + # A shared directory means two people can send the same name. Refusing the + # second is safe but silly — everyone's camera produces IMG_1234.jpg — so + # a free name is found instead. Never a replacement. + stored_name = state["stored_name"] if state else _free_name(target_dir, filename) + tmp_path = target_dir / f"{stored_name}.part" + final_path = target_dir / stored_name + if chunk_index == 0: + # Backstop: _free_name already guarantees this, and it stays because + # it asserts the invariant where the write happens. if final_path.exists(): self._send({"type": "error", "detail": "File already exists"}) return - state = {"next_index": 0, "bytes": 0} + state = {"next_index": 0, "bytes": 0, "stored_name": stored_name} self._uploads[upload_key] = state elif state is None: self._send({"type": "error", "detail": "Upload not started"}) @@ -1271,15 +1295,19 @@ class WebRTCPeerSession: "v": MNP_VERSION, "chunk_index": chunk_index, "filename": filename, + # What it is actually called on disk, which a chat attachment has to + # reference and the uploader deserves to be told. + "stored_as": stored_name, + "dir": rel_dir, }) if chunk_index + 1 >= total_chunks: 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"]) - self._audit("file_upload", f"{rel_dir}/{filename}") - self._register_uploader(ctx, rel_dir, filename) + stored_name, total_chunks, state["bytes"]) + self._audit("file_upload", f"{rel_dir}/{stored_name}") + self._register_uploader(ctx, rel_dir, stored_name) def _register_uploader(self, ctx: dict, rel_dir: str, filename: str) -> None: """ diff --git a/packages/meshbay-node/tests/test_security_regressions.py b/packages/meshbay-node/tests/test_security_regressions.py index 7627926..77d544b 100644 --- a/packages/meshbay-node/tests/test_security_regressions.py +++ b/packages/meshbay-node/tests/test_security_regressions.py @@ -132,7 +132,9 @@ def test_upload_cannot_overwrite_another_members_file(tmp_path): victim = _session(tmp_path, "victim-user") shared_root = victim._ctx["shared_root"] - original = shared_root / "important.mp4" + uploads = shared_root / "uploads" + uploads.mkdir() + original = uploads / "important.mp4" original.write_bytes(b"operator's original content") attacker = _session(tmp_path, "attacker-user") @@ -143,11 +145,9 @@ def test_upload_cannot_overwrite_another_members_file(tmp_path): "data": base64.b64encode(b"attacker content").decode(), }) - assert original.read_bytes() == b"operator's original content" - 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") + assert original.read_bytes() == b"operator's original content", ( + "an upload replaced an existing file (C5a)") + assert (uploads / "important (2).mp4").read_bytes() == b"attacker content" def test_upload_second_attempt_cannot_replace_own_completed_file(tmp_path): @@ -159,32 +159,10 @@ def test_upload_second_attempt_cannot_replace_own_completed_file(tmp_path): session.sent.clear() session._do_file_upload(dict(payload)) - assert any(m.get("type") == "error" for m in session.sent) - 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}" + uploads = session._ctx["shared_root"] / "uploads" + assert (uploads / "movie.mp4").read_bytes() == b"first", ( + "the first upload was replaced") + assert (uploads / "movie (2).mp4").read_bytes() == b"first" @pytest.mark.parametrize("bad", [ @@ -205,17 +183,49 @@ 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_still_refuses_unsafe_names_in_a_subdirectory(tmp_path): - """The name allowlist is not weakened by having somewhere to put the file.""" +def test_upload_ignores_any_directory_the_client_asks_for(tmp_path): + """ + Uploads land in uploads/, chosen by the node. A client that names somewhere + else — or nowhere at all — changes nothing, so the traversal surface that a + client-chosen destination would open does not exist on this path. + """ session = _session(tmp_path, "user-1") - (session._ctx["shared_root"] / "docs").mkdir() + shared_root = session._ctx["shared_root"] session._do_file_upload({ - "filename": "../escape.txt", "dir": "docs", + "filename": "note.txt", "dir": "../../etc", "chunk_index": 0, "total_chunks": 1, "data": base64.b64encode(b"x").decode(), }) - assert any(m.get("type") == "error" for m in session.sent) + + assert (shared_root / "uploads" / "note.txt").read_bytes() == b"x" + assert not (tmp_path / "etc").exists() + + +def test_two_members_can_send_the_same_filename(tmp_path): + """ + One shared uploads/ means collisions are ordinary — every camera produces + IMG_1234.jpg. The second gets a free name; neither replaces the other. + """ + first = _session(tmp_path, "user-1") + first._do_file_upload({ + "filename": "IMG_1234.jpg", "chunk_index": 0, "total_chunks": 1, + "data": base64.b64encode(b"first").decode(), + }) + second = _session(tmp_path, "user-2") + second._do_file_upload({ + "filename": "IMG_1234.jpg", "chunk_index": 0, "total_chunks": 1, + "data": base64.b64encode(b"second").decode(), + }) + + uploads = first._ctx["shared_root"] / "uploads" + assert (uploads / "IMG_1234.jpg").read_bytes() == b"first" + assert (uploads / "IMG_1234 (2).jpg").read_bytes() == b"second" + + ack = [m for m in second.sent if m.get("type") == "file_upload_ack"][-1] + assert ack["stored_as"] == "IMG_1234 (2).jpg", ( + "the sender must be told the name that was used, or a chat attachment " + "points at someone else's file") # ── H1: group isolation ────────────────────────────────────────────────────── |