Repository navigation
fix(dm-result): honor tierMap + strip file markers from REST DM body - #985
Merged
sonichi merged 2 commits intoMay 22, 2026
Merged
Conversation
Three real bugs in the REST-only fallback delivery path used by
`poll_dm_fallback` when voice / the live bridge can't reach the user:
## Bug A: `_resolve_owner_id` ignored `tierMap`
`src/discord-bridge.py:poll_proactive` honors the documented priority
chain `$SUTANDO_DM_OWNER_ID` -> `tierMap[uid] == "owner"` -> bot-filter
fallback. `dm-result.py` only honored env override + bot-filter — drift
between the live bridge and the fallback path. An admin who tier-tagged
the owner in access.json saw the live bridge route correctly, but the
fallback could pick a different non-bot from allowFrom (or a bot, in
the degraded-network case where /users/{id} lookups all fail).
Fix: insert the tierMap branch between env override and bot-filter.
Defensive: only accept tier-tagged owner if still present in allowFrom
— stale tags from removed users must not resolve a delisted owner.
## Bug D: `send_dm` delivered `[file:|send:|attach:]` markers as
## literal text
`poll_dm_fallback` shells out to `dm-result.py --file results/task-X.txt`
to deliver result files when the live bridge can't. Result files often
embed file markers:
Here's the screenshot you asked about: [file: /tmp/sutando-x.png]
The live bridge attaches the file via `discord.File(path)` on the
WS-connected client. dm-result is REST-only and has no multipart
upload path — so the marker landed verbatim in the DM and the file
was silently dropped.
Fix: mirror discord-bridge's `_split_file_markers` helper. `send_dm`
now strips markers from the body before chunking and logs the dropped
file list. Actual REST multipart upload is a follow-up — flagged in
the inline comment.
## Bug C (secondary): empty-body-after-strip posted `""` -> Discord 400
Pre-fix: `chunks = list(_chunk_for_discord(text)) or [text]`. For a
whitespace-only or all-marker body, the chunker yields nothing ->
empty list -> falsy -> falls back to `[""]` and POSTs empty content.
Discord returns HTTP 400 / code 50006.
Fix: detect empty-after-strip and skip the /messages call entirely.
## Tests
`tests/dm-result-send-dm.test.py` (5 cases). Mocks
`urllib.request.urlopen` with a recording fake (`_FakeTransport`) and
probes the full REST flow. The fake raises `AssertionError` on any
unmocked request, so future regressions fail loudly.
- `test_tier_map_resolution_skips_bot_lookup` (Bug A regression
guard + perf assertion that the wasted /users/{id} call is gone)
- `test_bot_filter_fallback_still_works_without_tier_map`
(existing behavior preserved)
- `test_file_markers_stripped_from_body` (Bug D regression guard)
- `test_empty_body_after_marker_strip_does_not_post_messages`
(Bug C regression guard — the fake transport raises
AssertionError on an unmocked POST, so a future regression here
fails loudly)
- `test_env_override_skips_access_json_entirely` (priority-order
pin)
All 5 pass locally.
Co-Authored-By: Vasiliy's Sutando <noreply@anthropic.com>
|
|
Owner
|
LGTM. Three real bugs, three independent regression tests — the right granularity. Bug A is the most consequential — drift between Bug D (file markers landing as literal text) is the easiest to silently miss in normal use. Bug C (empty-after-strip → Discord 400) is the kind of post-fix-of-fix that's easy to skip but matters — all-marker bodies hit this. Returning If #990 lands first, this PR should rebase to import |
Merged
2 of 3 tasks
2 of 3 tasks
sonichi
added a commit
that referenced
this pull request
May 25, 2026
…achments (#1029) * feat(dm-result): REST multipart upload — actually deliver [file:] attachments ## 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 uuid4 (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. ## 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: - allowlisted file uploads via multipart (file bytes present) - non-allowlisted file rejected (zero multipart calls) - file-only message (empty content but valid multipart) - 11 files split into 10+1 batches - filename header sanitization (CR/LF/`"` stripped — defends against multipart-envelope splicing via attacker-supplied filename, same class as PR #1022) All 5 pass locally. Co-Authored-By: Vasiliy's Sutando <noreply@anthropic.com> * refactor(send-allowlist): extract shared module per @liususan091219 review Lucy's PR #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 — and noted that copies "will drift" regardless. Per her recommendation: extract to a tiny shared module that both consumers import. ## Pre-existing drift this also fixes The two copies were already out of sync: - `src/discord-bridge.py` had the richer set: `_REPO / "results"`, `_REPO / "notes"`, `shared_personal_path("notes", _REPO)`, `_REPO / "docs"`, `~/Desktop/iclr-backups`, `~/Documents/sutando-launch-assets`, plus the four `/tmp/*` prefixes. - `src/dm-result.py` (from PR #1029) had the narrower set: only `_REPO / "results"`, `_REPO / "notes"`, `_REPO / "docs"`, and the same four prefixes — missing the personal-notes / Desktop / Documents roots. So a file like `~/Desktop/iclr-backups/asset.png` would deliver via the live WS bridge but get rejected by the REST fallback. Same shape as the kinds of drift Lucy was guarding against. The extract makes the policy a single set; both paths now respect the same allowlist. ## What ships ### `src/send_allowlist.py` (new) - `SEND_ALLOWED_ROOTS` — the canonical superset (all 6 roots). - `SEND_ALLOWED_PREFIXES` — the four `/tmp/*` prefixes. - `is_path_sendable(fpath) -> bool` — the canonical predicate. Imports `resolve_workspace` and `shared_personal_path` at module- load so the per-user paths resolve correctly. Stays dep-light (stdlib only beyond those two existing helpers). ### `src/discord-bridge.py` Replaces 21-line inline definition + 22-line function with an import + a one-line alias (`_is_path_sendable = _is_path_sendable_shared`). The 7 existing call sites in this file don't need touching because the local name `_is_path_sendable` is preserved. ### `src/dm-result.py` Replaces the PR #1029 inline copy with the same import. Local names `_SEND_ALLOWED_ROOTS` / `_SEND_ALLOWED_PREFIXES` preserved via import-as alias so other tests that reference them keep working. ## Tests `tests/send-allowlist-shared.test.py` — 6 architectural assertions: - shared module exists - discord-bridge.py imports from it - dm-result.py imports 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 - the shared module contains the documented set (drops are deliberate, pinned by this test) The architectural pin is the load-bearing piece: it makes future "oh I'll just add it inline real quick" regressions fail loudly. All 6 tests pass locally. Existing `tests/dm-result-multipart-upload.test.py` (5 cases) and `tests/dm-result-send-dm.test.py` (5 cases) still pass — no behavior change for the live paths, just plumbing. Co-Authored-By: liususan091219 (review feedback) Co-Authored-By: Vasiliy's Sutando <noreply@anthropic.com> * test(discord-bridge-allowlist): accept send_allowlist.py as helper location Per @qingyun-wu PR #1029 review — CI red because the structural test in `tests/discord-bridge-allowlist.test.py` greps for `def _is_path_sendable(...)` in `src/discord-bridge.py`. The PR moves the canonical implementation into `src/send_allowlist.py` (per @liususan091219's drift-extraction review on #1029) and leaves only a runtime alias in discord-bridge.py, so the original regex misses. Fix: scan BOTH source files. Helper is found in either location: - Legacy inline form: `def _is_path_sendable(...)` in discord-bridge.py - Post-refactor form: `def is_path_sendable(...)` in send_allowlist.py If the helper lives in send_allowlist.py, the test additionally verifies discord-bridge.py imports it — without the import, the gate sites (`discord.File(fpath)`) would reference an undefined name. DOTALL match so multi-line `from send_allowlist import (… as …, ...)` parenthesized imports work. The contract this test guards (helper uses os.path.realpath for CodeQL sanitizer, checks both ROOTS and PREFIXES, fail-closed default, all sinks gated) is unchanged — only the location-discovery logic was updated. Co-Authored-By: qingyun-wu (review feedback) Co-Authored-By: Vasiliy's Sutando <noreply@anthropic.com> --------- Co-authored-by: Vasiliy's Sutando <noreply@anthropic.com> Co-authored-by: Chi Wang <4250911+sonichi@users.noreply.github.com>
This was referenced May 25, 2026
liususan091219
pushed a commit
to liususan091219/sutando-fork
that referenced
this pull request
May 28, 2026
…onichi#985) Three real bugs in the REST-only fallback delivery path used by `poll_dm_fallback` when voice / the live bridge can't reach the user: ## Bug A: `_resolve_owner_id` ignored `tierMap` `src/discord-bridge.py:poll_proactive` honors the documented priority chain `$SUTANDO_DM_OWNER_ID` -> `tierMap[uid] == "owner"` -> bot-filter fallback. `dm-result.py` only honored env override + bot-filter — drift between the live bridge and the fallback path. An admin who tier-tagged the owner in access.json saw the live bridge route correctly, but the fallback could pick a different non-bot from allowFrom (or a bot, in the degraded-network case where /users/{id} lookups all fail). Fix: insert the tierMap branch between env override and bot-filter. Defensive: only accept tier-tagged owner if still present in allowFrom — stale tags from removed users must not resolve a delisted owner. ## Bug D: `send_dm` delivered `[file:|send:|attach:]` markers as ## literal text `poll_dm_fallback` shells out to `dm-result.py --file results/task-X.txt` to deliver result files when the live bridge can't. Result files often embed file markers: Here's the screenshot you asked about: [file: /tmp/sutando-x.png] The live bridge attaches the file via `discord.File(path)` on the WS-connected client. dm-result is REST-only and has no multipart upload path — so the marker landed verbatim in the DM and the file was silently dropped. Fix: mirror discord-bridge's `_split_file_markers` helper. `send_dm` now strips markers from the body before chunking and logs the dropped file list. Actual REST multipart upload is a follow-up — flagged in the inline comment. ## Bug C (secondary): empty-body-after-strip posted `""` -> Discord 400 Pre-fix: `chunks = list(_chunk_for_discord(text)) or [text]`. For a whitespace-only or all-marker body, the chunker yields nothing -> empty list -> falsy -> falls back to `[""]` and POSTs empty content. Discord returns HTTP 400 / code 50006. Fix: detect empty-after-strip and skip the /messages call entirely. ## Tests `tests/dm-result-send-dm.test.py` (5 cases). Mocks `urllib.request.urlopen` with a recording fake (`_FakeTransport`) and probes the full REST flow. The fake raises `AssertionError` on any unmocked request, so future regressions fail loudly. - `test_tier_map_resolution_skips_bot_lookup` (Bug A regression guard + perf assertion that the wasted /users/{id} call is gone) - `test_bot_filter_fallback_still_works_without_tier_map` (existing behavior preserved) - `test_file_markers_stripped_from_body` (Bug D regression guard) - `test_empty_body_after_marker_strip_does_not_post_messages` (Bug C regression guard — the fake transport raises AssertionError on an unmocked POST, so a future regression here fails loudly) - `test_env_override_skips_access_json_entirely` (priority-order pin) All 5 pass locally. Co-authored-by: Vasiliy's Sutando <noreply@anthropic.com> Co-authored-by: Chi Wang <4250911+sonichi@users.noreply.github.com>
liususan091219
pushed a commit
to liususan091219/sutando-fork
that referenced
this pull request
May 28, 2026
…achments (sonichi#1029) * feat(dm-result): REST multipart upload — actually deliver [file:] attachments ## Why — closes the follow-up flagged in PR sonichi#985 PR sonichi#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 uuid4 (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 sonichi#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. ## 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: - allowlisted file uploads via multipart (file bytes present) - non-allowlisted file rejected (zero multipart calls) - file-only message (empty content but valid multipart) - 11 files split into 10+1 batches - filename header sanitization (CR/LF/`"` stripped — defends against multipart-envelope splicing via attacker-supplied filename, same class as PR sonichi#1022) All 5 pass locally. Co-Authored-By: Vasiliy's Sutando <noreply@anthropic.com> * refactor(send-allowlist): extract shared module per @liususan091219 review Lucy's 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 — and noted that copies "will drift" regardless. Per her recommendation: extract to a tiny shared module that both consumers import. ## Pre-existing drift this also fixes The two copies were already out of sync: - `src/discord-bridge.py` had the richer set: `_REPO / "results"`, `_REPO / "notes"`, `shared_personal_path("notes", _REPO)`, `_REPO / "docs"`, `~/Desktop/iclr-backups`, `~/Documents/sutando-launch-assets`, plus the four `/tmp/*` prefixes. - `src/dm-result.py` (from PR sonichi#1029) had the narrower set: only `_REPO / "results"`, `_REPO / "notes"`, `_REPO / "docs"`, and the same four prefixes — missing the personal-notes / Desktop / Documents roots. So a file like `~/Desktop/iclr-backups/asset.png` would deliver via the live WS bridge but get rejected by the REST fallback. Same shape as the kinds of drift Lucy was guarding against. The extract makes the policy a single set; both paths now respect the same allowlist. ## What ships ### `src/send_allowlist.py` (new) - `SEND_ALLOWED_ROOTS` — the canonical superset (all 6 roots). - `SEND_ALLOWED_PREFIXES` — the four `/tmp/*` prefixes. - `is_path_sendable(fpath) -> bool` — the canonical predicate. Imports `resolve_workspace` and `shared_personal_path` at module- load so the per-user paths resolve correctly. Stays dep-light (stdlib only beyond those two existing helpers). ### `src/discord-bridge.py` Replaces 21-line inline definition + 22-line function with an import + a one-line alias (`_is_path_sendable = _is_path_sendable_shared`). The 7 existing call sites in this file don't need touching because the local name `_is_path_sendable` is preserved. ### `src/dm-result.py` Replaces the PR sonichi#1029 inline copy with the same import. Local names `_SEND_ALLOWED_ROOTS` / `_SEND_ALLOWED_PREFIXES` preserved via import-as alias so other tests that reference them keep working. ## Tests `tests/send-allowlist-shared.test.py` — 6 architectural assertions: - shared module exists - discord-bridge.py imports from it - dm-result.py imports 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 - the shared module contains the documented set (drops are deliberate, pinned by this test) The architectural pin is the load-bearing piece: it makes future "oh I'll just add it inline real quick" regressions fail loudly. All 6 tests pass locally. Existing `tests/dm-result-multipart-upload.test.py` (5 cases) and `tests/dm-result-send-dm.test.py` (5 cases) still pass — no behavior change for the live paths, just plumbing. Co-Authored-By: liususan091219 (review feedback) Co-Authored-By: Vasiliy's Sutando <noreply@anthropic.com> * test(discord-bridge-allowlist): accept send_allowlist.py as helper location Per @qingyun-wu PR sonichi#1029 review — CI red because the structural test in `tests/discord-bridge-allowlist.test.py` greps for `def _is_path_sendable(...)` in `src/discord-bridge.py`. The PR moves the canonical implementation into `src/send_allowlist.py` (per @liususan091219's drift-extraction review on sonichi#1029) and leaves only a runtime alias in discord-bridge.py, so the original regex misses. Fix: scan BOTH source files. Helper is found in either location: - Legacy inline form: `def _is_path_sendable(...)` in discord-bridge.py - Post-refactor form: `def is_path_sendable(...)` in send_allowlist.py If the helper lives in send_allowlist.py, the test additionally verifies discord-bridge.py imports it — without the import, the gate sites (`discord.File(fpath)`) would reference an undefined name. DOTALL match so multi-line `from send_allowlist import (… as …, ...)` parenthesized imports work. The contract this test guards (helper uses os.path.realpath for CodeQL sanitizer, checks both ROOTS and PREFIXES, fail-closed default, all sinks gated) is unchanged — only the location-discovery logic was updated. Co-Authored-By: qingyun-wu (review feedback) Co-Authored-By: Vasiliy's Sutando <noreply@anthropic.com> --------- Co-authored-by: Vasiliy's Sutando <noreply@anthropic.com> Co-authored-by: Chi Wang <4250911+sonichi@users.noreply.github.com>
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
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
Three real bugs in
src/dm-result.py— the REST-only fallback delivery path used bypoll_dm_fallbackwhen voice / the live bridge can't reach the user:Bug A:
_resolve_owner_idignoredtierMapsrc/discord-bridge.py:poll_proactivehonors the documented priority chain$SUTANDO_DM_OWNER_ID→tierMap[uid] == "owner"→ bot-filter fallback.dm-result.pyonly honored env override + bot-filter — drift between the live bridge and the fallback path.Visible symptom: admin tier-tagged an owner in
access.json; live bridge routes proactive DMs correctly, but the fallback path could pick a different non-bot (or a bot, in the degraded-network case where/users/{id}lookups all fail and the function falls back toallow[0]).Fix: insert tierMap branch between env override and bot-filter. Defensive: only accept tier-tagged owner if still in
allowFrom.Bug D:
send_dmdelivered[file:|send:|attach:]markers as literal textpoll_dm_fallbackshells out todm-result.py --file results/task-X.txtto deliver result files. Result files often embed file markers:The live bridge attaches the file via
discord.File(path)on the WS-connected client. dm-result is REST-only and has no multipart upload path — so the marker landed verbatim in the DM and the file was silently dropped.Fix: mirror discord-bridge's
_split_file_markershelper.send_dmstrips markers before chunking and logs the dropped file list. Actual REST multipart upload is a follow-up — explicitly called out in the inline comment.Bug C (secondary): empty-body-after-strip posted
""→ Discord 400Pre-fix:
chunks = list(_chunk_for_discord(text)) or [text]. For a whitespace-only or all-marker body, the chunker yields nothing → empty list → falsy → falls back to[""]and POSTs empty content. Discord returns HTTP 400 / code 50006.Fix: detect empty-after-strip and skip the
/messagescall entirely. Reports no-op, returnsTrue(not an error).Tests
tests/dm-result-send-dm.test.py(5 cases). Mocksurllib.request.urlopenwith a recording fake (_FakeTransport) and probes the full REST flow. The fake raisesAssertionErroron any unmocked request, so future regressions fail loudly.test_tier_map_resolution_skips_bot_lookup/users/{id}calls.test_bot_filter_fallback_still_works_without_tier_maptest_file_markers_stripped_from_body[file:literal; surrounding text preserved.test_empty_body_after_marker_strip_does_not_post_messages/messagesPOSTs.test_env_override_skips_access_json_entirelyTest plan
python3 tests/dm-result-send-dm.test.py— 5/5 passtierMapset in access.json, verify dm-result picks the tier-tagged owner🤖 Generated with Claude Code