summaryrefslogtreecommitdiffstats
path: root/packages/meshbay-node/src/meshbay_node
diff options
context:
space:
mode:
authorChristophe Besson <cbesson@gmail.com>2026-08-14 22:01:51 +0200
committerChristophe Besson <cbesson@gmail.com>2026-08-14 22:01:51 +0200
commit38f91818f876c51dcd7eb7911b65fc7bf5154c83 (patch)
tree4b8af2755796711a2d09f2750a0c263372e6b4cc /packages/meshbay-node/src/meshbay_node
parentb3ef2aff738cc4efd974efab9315a6ce6c3de493 (diff)
downloadmeshbay-38f91818f876c51dcd7eb7911b65fc7bf5154c83.tar.gz
feat(files): one uploads/ directory, for files and chat alike
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 <noreply@anthropic.com>
Diffstat (limited to 'packages/meshbay-node/src/meshbay_node')
-rw-r--r--packages/meshbay-node/src/meshbay_node/transport/webrtc_server.py70
1 files changed, 49 insertions, 21 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 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:
"""