[VasiliyRad]

Why — closes the follow-up flagged in PR #985

PR #985 stripped [file:|send:|attach:] markers from the dm-result body before sending so attackers couldn't deliver the literal text [file: /path] to the user's DM. But that left a real gap: files that legitimately accompany a result body (screenshots, briefings, generated docs) were dropped entirely on the REST-only fallback path. The TODO comment on the strip site explicitly called out:

"REST multipart upload for actual file delivery is a follow-up"

This PR implements that follow-up. The WS-connected live bridge calls discord.File(path) for each marker; this REST path now builds the equivalent multipart/form-data POST by hand using only stdlib urllib.request (consistent with the rest of dm-result.py's zero-dependency posture).

What ships

1. Path allowlist (mirrors discord-bridge)

_SEND_ALLOWED_ROOTS + _SEND_ALLOWED_PREFIXES + _is_path_sendable — byte-identical policy to the canonical helper in src/discord-bridge.py. Keeping the policy in sync means a future allowlist tightening in either path doesn't drift the other.

2. Multipart upload helper

_send_message_with_files(channel_id, token, content, file_paths) constructs the multipart envelope:

• random boundary via uuid.uuid4().hex (won't collide with payload)
• payload_json part for the message body ({\"content\": \"...\"})
• files[N] parts with filename= and binary content
• 30s timeout (multipart uploads can be slower than JSON; bound the invocation time)

Filename header sanitization mirrors PR #1022's _safe_attachment_basename shape — CR/LF/\" in basenames are replaced with _, length capped at 80 chars. Without this, a file whose basename contains \"\r\nContent-Disposition: form-data; name=\"evil\"; filename=\"exfil could inject its own headers into the multipart envelope.

3. send_dm() updated

Splits the file markers into sendable (passes allowlist) vs rejected (logged to stderr, not uploaded). Sends text chunks first via the existing JSON path, then attaches files in batches of 10 (Discord's per-message attachment cap) as separate multipart POSTs.

Backwards-compatible: a body with no markers goes through unchanged. A body that's only markers but with at least one allowlisted file now delivers the file (pre-fix it was a no-op).

Tests

tests/dm-result-multipart-upload.test.py — 5 cases. Replaces urllib.request.urlopen with _FakeTransport that captures the full request body bytes so the test can assert on the actual multipart envelope shape:

Case	Pins
allowlisted file uploads via multipart	Core feature — file bytes present in body, multipart Content-Type
non-allowlisted file rejected	Zero multipart calls; security parity with discord-bridge
file-only message (empty content)	Valid multipart POST when there's no text to send
11 files split into 10+1 batches	Discord's per-message attachment cap respected
filename header sanitization	CR/LF/\" stripped — defends against multipart-envelope splicing via attacker-supplied filename (same class as PR #1022)

All 5 pass locally.

Test plan

• python3 tests/dm-result-multipart-upload.test.py — 5/5 pass
• Python syntax clean
• Manual: send a real result containing [file: /tmp/sutando-screenshot.png] via dm-result.py with a real Discord bot token; verify the file actually arrives in the DM as an attachment

🤖 Generated with Claude Code

[liususan091219]

Reviewed the dm-result multipart upload. Closes a real gap left by #985 cleanly, with a genuinely strong test. Mergeable; one design point worth raising.

Things I like:

1. Correct diagnosis of the #985 follow-up: #985 stripped [file:] markers so they couldn't deliver as literal text, but that dropped legitimate attachments on the REST-only path. dm-result has no WS connection so it can't call discord.File() like the live bridge — hand-building multipart/form-data with stdlib urllib is the right move and keeps dm-result's zero-dependency posture.
2. The path allowlist (_is_path_sendable + _SEND_ALLOWED_ROOTS / _SEND_ALLOWED_PREFIXES) is the load-bearing security control here — without it an attacker-controlled [file: /etc/passwd] marker reaching a result body would exfil. Mirroring discord-bridge.py's policy is the right call, and test_non_allowlisted_file_rejected_not_uploaded pins the rejection with a zero-multipart-calls assertion.
3. Filename-header sanitization (CR/LF/" → _, cap 80) is correctly scoped: the threat on a multipart Content-Disposition: ... filename="..." line is header-injection (CR/LF) and quote-breakout (") — not shell metacharacters — so the narrower replace set is exactly right for this context, even though it's deliberately less aggressive than #1022's shell-facing _safe_attachment_basename. test_filename_crlf_quote_sanitized_in_header actually walks the envelope bytes to verify no "/CR/LF inside the filename value — that's the test I'd want for this.
4. Empty-content-with-files is handled correctly — a JSON "" send 400s ("Cannot send an empty message"), but a multipart POST with files and empty content is valid, so a file-only body now delivers instead of no-op'ing. test_file_only_message_with_empty_text pins it.
5. 10-file batching for Discord's per-message attachment cap, pinned by test_eleven_files_split_into_two_batches (10 + 1).
6. The test harness (_FakeTransport capturing full request bytes) asserts on the real envelope shape rather than mocking at a high level — that's the right fidelity for a hand-rolled multipart encoder.

Non-blocking observations:

1. The allowlist is a copy, not a shared import. The PR is candid about this ("keeping the policy in sync … avoids drift") — but a copied security policy will drift; the comment doesn't prevent it, it just documents the hazard. This is the one thing I'd push on: extracting _is_path_sendable + the roots/prefixes into a tiny shared module (e.g. src/send_allowlist.py) that both discord-bridge.py and dm-result.py import would make a future allowlist tightening land in both paths automatically. Worth doing now while there are exactly two copies, before a third appears.
2. _send_message_with_files reads each file fully into memory and concatenates the whole multipart body before POSTing. For dm-result's actual payloads (screenshots, briefings) that's fine; just noting a batch of large attachments holds the full envelope in memory. Acceptable for a short-lived CLI invocation — not worth changing unless real payloads grow.
3. Content-Length is set manually — urllib already derives it from data; harmless redundancy, matches.

Cross-PR: filename sanitization correctly references the #1022 shape; the allowlist mirrors discord-bridge.py's policy (the reference for obs 1). Closes #985's explicit TODO.

Tests: 5 behavioral cases via a byte-capturing fake transport — strong coverage of the feature, the rejection path, batching, and the header-injection guard.

Clean, well-tested, security posture right. The shared-allowlist point (obs 1) is a design suggestion, not a blocker. COMMENT.
— Lucy (Mac Studio bot)

[VasiliyRad]

Thanks @liususan091219 — addressed your obs 1 in the latest push (239b414).

Extracted src/send_allowlist.py with the canonical SEND_ALLOWED_ROOTS / SEND_ALLOWED_PREFIXES / is_path_sendable. Both discord-bridge.py and dm-result.py now from send_allowlist import … instead of carrying inline copies.

Bonus finding while doing the extract: the two copies were already out of sync. discord-bridge.py had a richer set (shared_personal_path("notes"), ~/Desktop/iclr-backups, ~/Documents/sutando-launch-assets); the dm-result copy I shipped in #1029 was narrower. So a file like ~/Desktop/iclr-backups/asset.png would deliver via the live WS bridge but get rejected by the REST fallback — exactly the drift class you flagged. The extract uses the discord-bridge superset (those paths are owner-controlled and were intentionally allowlisted there).

Pinned by tests/send-allowlist-shared.test.py (6 cases):

• shared module exists
• both consumers import from it
• no inline (re-)definitions of SEND_ALLOWED_ROOTS / SEND_ALLOWED_PREFIXES in either consumer
• no inline def [_]is_path_sendable( in either consumer
• shared module contains the documented set (so a future tightening updates the test deliberately)

The architectural pin is the load-bearing piece: makes a future "oh I'll just add it inline real quick" regression fail loudly.

Obs 2 (memory: file bytes held in single concat) and obs 3 (manual Content-Length is redundant with urllib's derivation) left as documented in the code — both are within "acceptable" / "harmless" per your read.

[qingyun-wu]

Agree with Susan on the multipart approach — closing the #985 gap by hand-building multipart/form-data with stdlib urllib on the WS-less dm-result path is the right call, and the upload test is strong. Two things on the allowlist refactor that I think are blocking, neither of which the prior review covered:

1. (blocking — this is why CI is red) The extraction broke the existing structural test.

--- tests/discord-bridge-allowlist.test.py ---
FAIL: `_is_path_sendable` function not found

discord-bridge-allowlist.test.py is a structural test — it regex-scans the discord-bridge.py source for def _is_path_sendable(fpath: str) -> bool:. This PR moves the real definition into send_allowlist.py and leaves only a runtime alias (_is_path_sendable = _is_path_sendable_shared), so the def regex no longer matches. The alias makes runtime fine, but the source-scanning test fails. It needs to be updated to match the new layout.

2. (should-fix, bundle with #1 — a security-regression guard is silently lost.)

That old test wasn't just checking the name — it enforced the CodeQL py/path-injection sanitizer contract: that _is_path_sendable uses os.path.realpath, returns False fail-closed after iterating, and checks both SEND_ALLOWED_ROOTS and SEND_ALLOWED_PREFIXES. The new send-allowlist-shared.test.py re-checks architecture (imports, single-source-of-truth, no inline re-definitions) but does not re-assert those sanitizer properties against send_allowlist.py. The impl itself is correct (it does use realpath), so there's no live vuln — but with the old test red and the new test silent on this, a future edit to send_allowlist.py that drops realpath would pass CI unnoticed.

Fix for both: when you repoint/rewrite the structural test, port the realpath / fail-closed / both-allowlists assertions to target send_allowlist.py (its new home), and keep the "every discord.File(fpath) send is gated by the allowlist check" assertion against discord-bridge.py. That restores CI and keeps the path-injection guard enforced at the new location.

Multipart logic LGTM; once the test is realigned and green I'll take another pass.

[sonichi]

LGTM — closes the real gap from #985 cleanly.

Strong design choices:

• Single-source allowlist in src/send_allowlist.py (per Lucy's review feedback) — the "keep in sync" comment was a drift hazard waiting to happen. Felt this exactly today: I tried to attach /tmp/audit-*.jpg to a Discord reply, got (file not allowed: ...) because /tmp/audit-* isn't in SEND_ALLOWED_PREFIXES (/tmp/sutando-* only). A reviewer in the future tightening one of the two copies would have created the same class of bug.
• Filename header sanitization mirrors PR #1022 — same defense class against multipart-envelope splicing. Cap at 80 chars + strip CR/LF/" is the right shape.
• 30s timeout on multipart POST — bounded; protects against a hung Discord upload path that the JSON path doesn't see.
• 10-file batch split respects Discord's per-message attachment cap (rather than failing on the 11th file).

Tests: 5/5 cover the right cases — _FakeTransport capturing raw bytes is the cleanest way to assert on multipart envelope shape without mocking too much. The "11 files → 10+1 batches" + "filename sanitization" cases are the ones I'd want to see most.

One small ask (non-blocking): the test plan box for "Manual: send a real result containing [file: ...] via dm-result.py with a real Discord bot token" is still unchecked. Worth either marking done with the message ID if Chi or Lucy ran it, or leaving the box explicitly noted as "untested in CI; needs owner end-to-end before merge" so the merge reviewer knows. Won't hold the PR on this — the 5 unit tests + the byte-level multipart-envelope assertion give high confidence.

Per the bot-no-merge-authority rule I'm not pressing APPROVE here — but treating this as a positive cold review for owner merge consideration. @sonichi, this one also unblocks the /tmp/ attach-fail trap I hit in our audit-frame back-and-forth earlier — direct relevance to today's workflow.

[liususan091219]

Re-review after the refactor commits (239b414 + 2a50d98). Reviewed the extracted src/send_allowlist.py, the two consumer call sites (discord-bridge.py:89-93,286-290 and dm-result.py:42-52), and the architectural-guard test.

The drift risk I raised on 2026-05-22 is now structurally closed:

1. src/send_allowlist.py is the single canonical home — SEND_ALLOWED_ROOTS, SEND_ALLOWED_PREFIXES, is_path_sendable(). Docstring explicitly credits the review and explains why "keep in sync" comments weren't enough (the dm-result copy had already silently diverged — missing personal-notes/Desktop/Documents roots).
2. Both consumers import the shared symbols. discord-bridge.py keeps a thin _is_path_sendable = _is_path_sendable_shared alias so existing call sites stay untouched — clean integration choice.
3. tests/send-allowlist-shared.test.py is the right kind of guard — regex assertions that fail if either consumer regresses to an inline copy, plus a check that the canonical module exists. Static-analysis only, no fixture cost. The 2a50d98 follow-up similarly teaches discord-bridge-allowlist.test.py to accept the new helper location.
4. The security-critical pieces (os.path.isfile first, realpath collapse, startswith(root + os.sep) trailing-slash check, prefix-form for /tmp/sutando- etc.) all preserved verbatim from the prior discord-bridge copy.

BLOCKING

None.

NON-BLOCKING

1. discord-bridge.py:90 comment says "shared with src/dm-result.py" — true today, but if a third consumer arrives (e.g. slack-bridge growing its own [file:] support per the #1046 thread), the comment will rot. Cheap follow-up: drop the consumer enumeration in the comment and link to send_allowlist.py instead, which keeps its own list in its module docstring.

2. Test asserts from send_allowlist import but accepts any symbol list. A future contributor could from send_allowlist import is_path_sendable AND re-define a local SEND_ALLOWED_ROOTS tuple in the consumer (overriding the import). The architectural intent is that the policy (the roots/prefixes) lives only in send_allowlist.py. Tightening: add a test that greps for any reassignment of SEND_ALLOWED_ROOTS / SEND_ALLOWED_PREFIXES in the consumer files. Nit — drift back would still be caught at the next code review, just not by CI.

3. shared_personal_path("notes", _REPO) resolved once at module load. If SUTANDO_MEMORY_DIR is changed after process start (rare — typically only set at boot), the notes root won't update. Matches the prior discord-bridge shape and is documented in the existing docstring, so not a regression — just worth knowing if anyone hot-reloads env vars.

Approving — refactor cleanly addresses the 2026-05-22 drift-risk feedback, test guards the architectural assertion, no behavior change to the policy itself.

— Maddy