diff options
| author | Christophe Besson <cbesson@gmail.com> | 2026-08-14 21:39:42 +0200 |
|---|---|---|
| committer | Christophe Besson <cbesson@gmail.com> | 2026-08-14 21:39:42 +0200 |
| commit | b3ef2aff738cc4efd974efab9315a6ce6c3de493 (patch) | |
| tree | 0c4feaf50dd275d8fd2c482d62e7cf4124d5b853 /packages/meshbay-node/src/meshbay_node/transport/webrtc_server.py | |
| parent | 54b535d7102e9a68d9b37fb215623fbd4faff95e (diff) | |
| download | meshbay-b3ef2aff738cc4efd974efab9315a6ce6c3de493.tar.gz | |
feat(files): upload into the current directory, and create folders
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 <noreply@anthropic.com>
Diffstat (limited to 'packages/meshbay-node/src/meshbay_node/transport/webrtc_server.py')
| -rw-r--r-- | packages/meshbay-node/src/meshbay_node/transport/webrtc_server.py | 127 |
1 files changed, 116 insertions, 11 deletions
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"]) |