This repository was archived by the owner on Jul 16, 2026. It is now read-only.
Repository navigation
feat(dm-result): REST multipart upload — actually deliver [file:] attachments - #57
Open
VasiliyRad wants to merge 2 commits into
Open
VasiliyRad wants to merge 2 commits into
VasiliyRad wants to merge 2 commits into
Conversation
…achments Closes the TODO flagged in PR #26 (fork) / sonichi#985 (upstream): dm-result was stripping `[file:|send:|attach:]` markers from the body and logging them, instead of actually delivering the attached files via REST. The WS-connected live bridge calls `discord.File(path)`; this REST path now builds the equivalent `multipart/form-data` POST by hand using only stdlib `urllib.request`. ## What ships 1. Path allowlist mirrors discord-bridge's `_is_path_sendable` (zero drift between live + fallback delivery paths). 2. New `_send_message_with_files(channel_id, token, content, file_paths)` helper builds multipart envelope using stdlib only. uuid4-derived boundary + `payload_json` + `files[N]` parts. 3. send_dm() now filters markers via allowlist, sends text chunks first, then attaches files in batches of 10 (Discord's per- message attachment cap). Filename header sanitization mirrors PR #52's `_safe_attachment_basename` shape — CR/LF/`"` in basenames are replaced with `_`, length capped at 80 chars. Without this, an attacker-supplied filename could inject its own Content-Disposition headers into the multipart envelope. Backwards-compatible: bodies with no markers go through unchanged. ## Tests `tests/dm-result-multipart-upload.test.py` — 5 cases via the recording-fake-transport pattern from PR #26: - allowlisted file uploads via multipart (file bytes in body) - non-allowlisted file rejected (zero multipart calls) - file-only message with empty text - 11 files split into 10+1 batches - filename header sanitization regression guard All 5 pass locally. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…view Lucy's upstream PR sonichi#1029 review flagged the file-attachment allowlist as a copy between discord-bridge.py and dm-result.py with a "keep in sync" comment that wouldn't prevent drift. Extracted to a shared `src/send_allowlist.py` module that both consumers import. ## Pre-existing drift this also fixes The two copies were already out of sync: - `discord-bridge.py` had the richer set (personal-notes, Desktop, Documents roots, plus the 4 tmp prefixes) - `dm-result.py` (from PR #57) had the narrower set (3 roots, 4 prefixes) — missing the personal-notes / Desktop / Documents The extract uses the discord-bridge superset. ## Tests `tests/send-allowlist-shared.test.py` — 6 architectural assertions pinning no inline re-definitions of `SEND_ALLOWED_ROOTS` / `SEND_ALLOWED_PREFIXES` / `is_path_sendable`. Both consumers must import from the shared module. All tests pass locally. Co-Authored-By: liususan091219 (review feedback) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
bassilkhilo-ag2
approved these changes
May 24, 2026
bassilkhilo-ag2
left a comment
bassilkhilo-ag2
left a comment
There was a problem hiding this comment.
LGTM — cold review. Port of sutando sonichi#1029 (Susan APPROVED + Chi LGTM). Two main changes:
-
Shared allowlist (
src/send_allowlist.py): single source of truth for path security policy. The old copy indiscord-bridge.pydeleted, dm-result.py now imports from the shared module. Prevents policy drift (liususan091219's comment in upstream review).os.path.realpathbefore prefix comparison is the correct CodeQL-satisfying pattern. -
REST multipart delivery: dm-result.py can now deliver
[file:]/[send:]/[attach:]markers viamultipart/form-dataPOST using only stdliburllib.request— consistent with dm-result.py's zero-dependency posture. Falls back gracefully if file doesn't exist or isn't in the allowlist.
Tests: dm-result-multipart-upload.test.py + send-allowlist-shared.test.py. Merge when ready.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why — closes the follow-up flagged in PR #26
PR #26 stripped
[file:|send:|attach:]markers from the dm-result body. The WS-connected live bridge callsdiscord.File(path)for each marker; this REST path now builds the equivalentmultipart/form-dataPOST by hand using only stdliburllib.request(consistent with the rest ofdm-result.py's zero-dependency posture).What ships
_SEND_ALLOWED_ROOTS+_SEND_ALLOWED_PREFIXES+_is_path_sendable_send_message_with_files(channel_id, token, content, file_paths)payload_json+files[N]partssend_dm()updatedFilename header sanitization mirrors PR #52's
_safe_attachment_basename— CR/LF/\"in basenames are replaced with_, length capped at 80 chars. Defends against multipart-envelope splicing via attacker-supplied filename (same class as PR #52).Tests
tests/dm-result-multipart-upload.test.py— 5 cases. Same fake-transport pattern as PR #26. All 5 pass locally.🤖 Generated with Claude Code