feat(dm-result): multipart upload + shared send_allowlist (Phase 5.9)

src/discord-bridge.py

@@ -138,26 +138,18 @@ def _push_vision_image(path: str, source: str = "discord") -> bool:  # type: ign
 ARCHIVE_RESULTS_DIR = REPO / "results" / "archive"
 OWNER_ACTIVITY_FILE = STATE_DIR / "last-owner-activity.json"
 
-# Allowlist for paths that may be attached to outgoing Discord messages.
-# Result text can embed `[file: /path]` / `[send: /path]` / `[attach: /path]`
-# markers; we only forward paths that resolve under one of these roots.
-# Fail-closed: a non-matching path is reported inline rather than sent.
-SEND_ALLOWED_ROOTS = (
-    str(REPO / "results"),
-    str(REPO / "notes"),
-    # Notes canonical home (private dir) — once saved by save_note, paths
-    # reference the private location. Both old and new paths allowed during
-    # the transition; the resolver picks whichever exists.
-    str(shared_personal_path("notes", REPO)),
-    str(REPO / "docs"),
-    str(Path.home() / "Desktop" / "iclr-backups"),
-    str(Path.home() / "Documents" / "sutando-launch-assets"),
-)
-SEND_ALLOWED_PREFIXES = (
-    "/tmp/sutando-",
-    "/private/tmp/sutando-",
-    "/tmp/echo-",
-    "/private/tmp/echo-",
+# Allowlist for paths attached via `[file:|send:|attach:]` markers.
+# Single source of truth is `src/send_allowlist.py` — shared with
+# `src/dm-result.py`'s REST-fallback delivery path. Per the OSS
+# liususan091219 review on PR #1029: keeping the policy as a copy in
+# each file will drift even with "keep in sync" comments. The extract
+# removes that hazard at the boundary. The policy values are byte-
+# identical to the pre-extract inline copy here (this is a pure
+# refactor, not a tightening/loosening of allowed roots).
+from send_allowlist import (  # noqa: E402
+    SEND_ALLOWED_PREFIXES,
+    SEND_ALLOWED_ROOTS,
+    is_path_sendable as _is_path_sendable_shared,
 )
 
 
@@ -347,27 +339,12 @@ def _split_file_markers(text: str) -> tuple[str, list[str]]:
     return clean_text, files
 
 
-def _is_path_sendable(fpath: str) -> bool:
-    """True iff `fpath` is a real file AND resolves under an allowed root.
-
-    Uses os.path.realpath to collapse symlinks / `..` segments before the
-    prefix comparison, matching the sanitizer pattern used across the
-    codebase for CodeQL py/path-injection resolution.
-    """
-    if not os.path.isfile(fpath):
-        return False
-    try:
-        real = os.path.realpath(fpath)
-    except OSError:
-        return False
-    for root in SEND_ALLOWED_ROOTS:
-        root_real = os.path.realpath(root)
-        if real == root_real or real.startswith(root_real + os.sep):
-            return True
-    for prefix in SEND_ALLOWED_PREFIXES:
-        if real.startswith(prefix):
-            return True
-    return False
+# Public name preserved (`_is_path_sendable`) so all existing call sites
+# in this bridge (write_owner_activity downstream + the four send paths)
+# continue to work unchanged. The shared helper lives in
+# `src/send_allowlist.py`; this alias is the single boundary between the
+# bridge and the shared policy module.
+_is_path_sendable = _is_path_sendable_shared
 
 
 def write_owner_activity(channel: str, summary: str) -> None:

src/dm-result.py

@@ -38,18 +38,32 @@
 ACCESS_JSON = Path.home() / ".claude" / "channels" / "discord" / "access.json"
 SSE_STATUS_URL = "http://localhost:8080/sse-status"
 
+# Path allowlist for `[file:|send:|attach:]` markers — sourced from
+# `src/send_allowlist.py` so this REST-fallback path uses the SAME
+# policy as the WS-connected live bridge (`src/discord-bridge.py`).
+# Per @liususan091219 review on the OSS PR #1029: a copied allowlist
+# will drift even with a comment claiming they're in sync — the
+# extract removes that hazard at the boundary. Ported in stando
+# Phase 5.9 of the OSS→private sync.
+from send_allowlist import (  # noqa: E402
+    is_path_sendable as _is_path_sendable,
+    SEND_ALLOWED_PREFIXES as _SEND_ALLOWED_PREFIXES,  # re-exported for tests
+    SEND_ALLOWED_ROOTS as _SEND_ALLOWED_ROOTS,        # re-exported for tests
+)
+
 
 _FENCE_LINE = re.compile(r"^\s{0,3}(`{3,}|~{3,})\s*([^\s`~][^`~]*)?\s*$")
 
 
 # Mirror of discord-bridge.py's `_FILE_MARKER_RE` — agent-emitted file
 # attachment markers embedded in result bodies. dm-result.py is the
-# REST-only fallback delivery path used when voice isn't connected;
-# without parsing these markers it would deliver the literal text
-# `[file: /tmp/sutando-x.png]` in the DM and silently drop the
-# attachment. PR limitation: REST multipart upload for actual file
-# delivery is a follow-up — this commit strips the markers from the
-# body so the user doesn't see the literal text. See dm-result_TODO.
+# REST-only fallback delivery path used when voice isn't connected.
+# The WS-connected live bridge attaches files via `discord.File(path)`;
+# this REST path builds the equivalent multipart upload (see
+# `_send_message_with_files`). Each marker path is allowlist-checked
+# against `_is_path_sendable` — same policy as discord-bridge.py via
+# the shared `send_allowlist` import — to bound exfil if an attacker-
+# controlled marker ever reaches a result body.
 _FILE_MARKER_RE = re.compile(r'\[(?:file|send|attach):\s*((?:/|~/)[^\]:]+)\]')
 
 
@@ -196,6 +210,79 @@ def _discord_api(method, path, token, body=None):
         return json.loads(raw) if raw else None
 
 
+def _send_message_with_files(channel_id: str, token: str, content: str,
+                             file_paths: "list[str]"):
+    """POST a message to a channel as multipart/form-data with attached
+    files. Used by `send_dm()` when an agent-emitted result body
+    contains `[file:|send:|attach:]` markers — the WS-connected live
+    bridge calls `discord.File(path)` directly; this REST path needs
+    to assemble the same multipart payload by hand.
+
+    `content` may be empty (file-only message). `file_paths` are the
+    already-allowlisted absolute paths. Raises on non-2xx; caller
+    handles per-file errors.
+
+    Discord docs: POST /channels/{id}/messages with
+    multipart/form-data, parts named `payload_json` (the message body)
+    and `files[0]`, `files[1]`, ... each with a `filename=` in the
+    `Content-Disposition`.
+    """
+    # Random boundary that won't appear in any payload we send.
+    import uuid
+    boundary = f"----SutandoBoundary{uuid.uuid4().hex}"
+    parts: list[bytes] = []
+    # payload_json part
+    payload = {"content": content} if content else {}
+    parts.append(
+        f"--{boundary}\r\n"
+        f'Content-Disposition: form-data; name="payload_json"\r\n'
+        f"Content-Type: application/json\r\n\r\n".encode()
+    )
+    parts.append(json.dumps(payload).encode())
+    parts.append(b"\r\n")
+    # files[N] parts
+    for i, fpath in enumerate(file_paths):
+        filename = os.path.basename(fpath)
+        # Sanitize filename header — same shape as discord-bridge.py's
+        # `_safe_attachment_basename`. A filename containing CR/LF
+        # here would let the file inject its own headers into the
+        # multipart envelope.
+        safe_name = (
+            filename
+            .replace("\r", "_")
+            .replace("\n", "_")
+            .replace('"', "_")
+        )[:80] or f"file-{i}"
+        parts.append(
+            f"--{boundary}\r\n"
+            f'Content-Disposition: form-data; name="files[{i}]"; '
+            f'filename="{safe_name}"\r\n'
+            f"Content-Type: application/octet-stream\r\n\r\n".encode()
+        )
+        with open(fpath, "rb") as fh:
+            parts.append(fh.read())
+        parts.append(b"\r\n")
+    parts.append(f"--{boundary}--\r\n".encode())
+    body = b"".join(parts)
+    url = f"https://discord.com/api/v10/channels/{channel_id}/messages"
+    req = urllib.request.Request(
+        url,
+        data=body,
+        headers={
+            "Authorization": f"Bot {token}",
+            "Content-Type": f"multipart/form-data; boundary={boundary}",
+            "Content-Length": str(len(body)),
+            "User-Agent": "Sutando/1.0",
+        },
+        method="POST",
+    )
+    # 30s timeout — multipart uploads can be slower than JSON; cap to
+    # bound the dm-result.py invocation time.
+    with urllib.request.urlopen(req, timeout=30) as resp:
+        raw = resp.read()
+        return json.loads(raw) if raw else None
+
+
 def _resolve_owner_id(token):
     """Return the Discord user ID for the human owner.
 
@@ -284,32 +371,42 @@ def send_dm(text: str) -> bool:
         print(f"dm-result: failed to open DM channel with {owner_id}: {e}", file=sys.stderr)
         return False
 
-    # Strip [file:|send:|attach:] markers from the body — the bridge's
-    # WS-connected client would attach the file directly, but dm-result
-    # is REST-only and has no multipart upload path yet, so the markers
-    # would otherwise deliver as literal text in the DM. We log the
-    # dropped files so the lossy delivery is visible in the dm-result
-    # output rather than silent.
-    clean_text, dropped_files = _split_file_markers(text)
-    if dropped_files:
+    # Extract [file:|send:|attach:] markers. The WS-connected live
+    # bridge calls `discord.File(path)` for each marker; this REST
+    # path now builds the equivalent multipart upload (see
+    # `_send_message_with_files`). Each marker path is allowlist-
+    # checked against `_is_path_sendable` — same policy as
+    # discord-bridge.py via the shared `send_allowlist` import — to
+    # bound exfil if an attacker-controlled marker ever reaches a
+    # result body.
+    clean_text, marker_files = _split_file_markers(text)
+    expanded_files = [os.path.expanduser(p.strip()) for p in marker_files]
+    sendable_files = [p for p in expanded_files if _is_path_sendable(p)]
+    rejected_files = [p for p in expanded_files if not _is_path_sendable(p)]
+    if rejected_files:
+        # Same security signal as discord-bridge: rejected paths log
+        # but don't leak the failure to the user.
         print(
-            f"dm-result: {len(dropped_files)} file marker(s) stripped from body "
-            f"(REST multipart upload not implemented): {dropped_files}",
+            f"dm-result: {len(rejected_files)} file marker(s) rejected by "
+            f"allowlist (would deliver via [file:] but path is outside "
+            f"_SEND_ALLOWED_ROOTS / _SEND_ALLOWED_PREFIXES): {rejected_files}",
             file=sys.stderr,
         )
 
     # An all-marker / all-whitespace body becomes empty after strip.
     # Sending `""` to Discord returns 400 ("Cannot send an empty
-    # message"); skip the send and report the no-op instead.
-    if not clean_text:
+    # message") for a text-only request. For a multipart upload with
+    # files, an empty `content` is valid.
+    if not clean_text and not sendable_files:
         print(
-            f"dm-result: body is empty after marker-strip; nothing to send "
-            f"(channel {channel_id})"
+            f"dm-result: body is empty after marker-strip and no sendable "
+            f"files; nothing to send (channel {channel_id})"
         )
-        return True  # not an error — the input had no deliverable text
+        return True  # not an error — the input had no deliverable payload
 
-    # Chunk into Discord-safe pieces, preserving code fences across boundaries.
-    chunks = list(_chunk_for_discord(clean_text))
+    # Chunk text into Discord-safe pieces, preserving code fences
+    # across boundaries.
+    chunks = list(_chunk_for_discord(clean_text)) if clean_text else []
     for i, chunk in enumerate(chunks):
         try:
             _discord_api("POST", f"/channels/{channel_id}/messages", token, {"content": chunk})
@@ -320,8 +417,27 @@ def send_dm(text: str) -> bool:
             )
             return False
 
+    # Attach files in batches of 10 (Discord per-message attachment
+    # cap). Each batch goes as a separate multipart message with
+    # empty content — the text was already delivered as chunks above.
+    DISCORD_FILES_PER_MESSAGE = 10
+    for batch_start in range(0, len(sendable_files), DISCORD_FILES_PER_MESSAGE):
+        batch = sendable_files[batch_start : batch_start + DISCORD_FILES_PER_MESSAGE]
+        try:
+            _send_message_with_files(channel_id, token, "", batch)
+        except Exception as e:
+            print(
+                f"dm-result: failed to upload file batch "
+                f"{batch_start // DISCORD_FILES_PER_MESSAGE + 1} "
+                f"({len(batch)} file(s)) to channel {channel_id}: {e}",
+                file=sys.stderr,
+            )
+            return False
+
+    file_summary = f", {len(sendable_files)} file(s)" if sendable_files else ""
     print(
-        f"dm-result: sent to DM ({len(clean_text)} chars in {len(chunks)} chunk(s)) via channel {channel_id}"
+        f"dm-result: sent to DM ({len(clean_text)} chars in {len(chunks)} chunk(s)"
+        f"{file_summary}) via channel {channel_id}"
     )
     return True
 

src/send_allowlist.py

@@ -0,0 +1,109 @@
+"""Shared file-attachment allowlist for `[file:|send:|attach:]` markers.
+
+Single source of truth for the policy that decides whether an agent-
+emitted file marker can be delivered to the owner's Discord DM /
+channel. Used by:
+
+  - ``src/discord-bridge.py`` — live WS-connected bridge
+    (``discord.File(path)``).
+  - ``src/dm-result.py`` — REST-only fallback when the bridge isn't
+    available (``multipart/form-data`` upload, see PR #1029).
+
+Per @liususan091219 review on PR #1029: keeping the policy as a copy
+in each file *will* drift even with the "keep in sync" comment, so
+extracting here gives both consumers a shared import. A tightening
+of the allowlist now lands in both paths automatically; a broadening
+is a single edit.
+
+Ported from OSS sutando in Phase 5.9 of the OSS→private sync. The
+policy values are byte-identical to the pre-port inline copy in
+``src/discord-bridge.py`` — this extract is a pure refactor that
+removes the drift hazard between the bridge and ``dm-result.py``.
+
+The policy:
+
+  * Files must be regular files (`os.path.isfile`) so symlinks
+    pointing at non-existent destinations don't qualify.
+  * `realpath` collapses the path before the prefix check — defeats
+    symlink-to-`/etc/passwd` and `..` escapes.
+  * Either the realpath equals an allowed-root or starts with
+    ``<root>/`` (the trailing-slash check prevents
+    ``/private/tmp/sutando-xyz`` from being treated as a child of
+    ``/private/tmp/sutando``).
+  * Prefix matches use `startswith` against the realpath so the
+    same anti-symlink/anti-traversal property holds.
+
+Per-machine paths (`~/Desktop/iclr-backups`,
+`~/Documents/sutando-launch-assets`) resolve at module-load time
+based on the current `Path.home()`; if the user's home dir changes
+between processes, the resolved root differs accordingly — that's
+the same shape the discord-bridge copy already had and matches
+expectations for owner-local file roots.
+"""
+
+from __future__ import annotations
+
+import os
+import sys
+from pathlib import Path
+
+sys.path.insert(0, str(Path(__file__).resolve().parent))
+from workspace_default import resolve_workspace  # noqa: E402
+from util_paths import shared_personal_path  # noqa: E402
+
+_REPO = resolve_workspace()
+
+# Owner-relative + machine-local roots. Files under these roots are
+# delivered to Discord as attachments without further checks.
+SEND_ALLOWED_ROOTS: tuple[str, ...] = (
+    str(_REPO / "results"),
+    str(_REPO / "notes"),
+    # Notes canonical home (private dir) — once saved by save_note,
+    # paths reference the private location. Both old and new paths
+    # allowed during the transition; resolver picks whichever exists.
+    str(shared_personal_path("notes", _REPO)),
+    str(_REPO / "docs"),
+    str(Path.home() / "Desktop" / "iclr-backups"),
+    str(Path.home() / "Documents" / "sutando-launch-assets"),
+)
+
+# Prefix forms — files whose realpath starts with any of these strings
+# are deliverable. Covers temp-file artifacts the agent generates
+# (`/tmp/sutando-recording-*.mov`, `/tmp/echo-screenshot-*.png`, etc.)
+# without needing to enumerate every filename.
+SEND_ALLOWED_PREFIXES: tuple[str, ...] = (
+    "/tmp/sutando-",
+    "/private/tmp/sutando-",
+    "/tmp/echo-",
+    "/private/tmp/echo-",
+)
+
+
+def is_path_sendable(fpath: str) -> bool:
+    """True iff `fpath` is a regular file AND its `realpath` resolves
+    under one of ``SEND_ALLOWED_ROOTS`` or starts with one of
+    ``SEND_ALLOWED_PREFIXES``.
+
+    Single source of truth for the file-attachment-delivery policy.
+    Mirrors the shape used by ``_is_path_sendable`` in both call sites
+    pre-extract.
+
+    A non-existent path, a symlink to outside the allowlist (caught by
+    realpath collapse), or a path that simply doesn't match any
+    allowed root/prefix all return False. Callers should fail-closed
+    on False (log + skip; never deliver the file).
+    """
+    if not os.path.isfile(fpath):
+        return False
+    try:
+        real = os.path.realpath(fpath)
+    except OSError:
+        return False
+    for root in SEND_ALLOWED_ROOTS:
+        root_real = os.path.realpath(root)
+        if real == root_real or real.startswith(root_real + os.sep):
+            return True
+    for prefix in SEND_ALLOWED_PREFIXES:
+        if real.startswith(prefix):
+            return True
+    return False