From 38f91818f876c51dcd7eb7911b65fc7bf5154c83 Mon Sep 17 00:00:00 2001 From: Christophe Besson Date: Fri, 14 Aug 2026 22:01:51 +0200 Subject: feat(files): one uploads/ directory, for files and chat alike MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Correction to the previous commit. Uploads went wherever the member happened to be looking, which spreads chat attachments through the tree and makes the destination a client-supplied path — surface that had to be defended. Everything a member sends now lands in `uploads/` at the root of the shared directory: visible, one place, easy for the operator to look into or empty. Chat attachments go there too, so the separate out-of-tree thumbs directory is not needed and is not built. They were already ordinary uploads; now they are ordinary uploads that land somewhere sensible. The destination is chosen by the node, so a client naming somewhere else changes nothing — the traversal surface simply is not there on this path. safe_subdir() remains for dir_create, where the path genuinely does come from the client, and keeps its tests. One shared directory means name collisions are ordinary rather than adversarial: every camera produces IMG_1234.jpg. The node finds a free name — "IMG_1234 (2).jpg" — and reports it in the ack, because a chat message has to point at the file that was actually written and not at someone else's. Nothing is ever replaced, which is the property the per-user quarantine existed for (C5a) and the one the tests assert; they fail if the free-name search is removed. Co-Authored-By: Claude Opus 5 --- .../tests/test_security_regressions.py | 84 ++++++++++++---------- 1 file changed, 47 insertions(+), 37 deletions(-) (limited to 'packages/meshbay-node/tests') 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 ────────────────────────────────────────────────────── -- cgit v1.2.3