Skip to content

fix(discord): harden outbound image URL fetches - #96540

Closed
big-mon wants to merge 6 commits into
NousResearch:mainfrom
big-mon:fix/dev-77-discord-safe-image-fetch
Closed

big-mon wants to merge 6 commits into
NousResearch:mainfrom
big-mon:fix/dev-77-discord-safe-image-fetch

Conversation

@big-mon

@big-mon big-mon commented Aug 27, 2026 •

Copy link
Copy Markdown

Superseded by #96543 after the source branch was renamed. Please continue review on #96543.

isheng-eqi and others added 6 commits August 28, 2026 00:15
The recent Discord resource-bounding pass (NousResearch#60122, NousResearch#60112, NousResearch#60113)
added limits for REST JSON/error response bodies and component label
UTF-16 lengths. Four HTTP response reads for image/animation/attachment
downloads were left unbounded — an oversized response from a CDN or
external URL could OOM the bot.

Add _DISCORD_IMAGE_DOWNLOAD_MAX_BYTES (50 MB) and
_DISCORD_ATTACHMENT_DOWNLOAD_MAX_BYTES (100 MB) constants, a shared
_read_response_bytes_bounded() helper, and apply bounds to:
- Batch image download (adapter.py ~2467)
- Single image download (adapter.py ~3555)
- Animation/GIF download (adapter.py ~3634)
- Attachment download (adapter.py ~5799)

Refs: NousResearch#60122, NousResearch#60112
@alt-glitch alt-glitch added type/security Security vulnerability or hardening comp/plugins Plugin system and bundled plugins platform/discord Discord bot adapter P3 Low — cosmetic, nice to have needs-repro Bug needs reproduction steps labels Aug 27, 2026
@big-mon big-mon closed this Aug 27, 2026
@big-mon
big-mon deleted the fix/dev-77-discord-safe-image-fetch branch August 27, 2026 17:51

@andrexibiza andrexibiza left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact head 0c7f18e341f130f3a27102baf2673539b9d2055f against its live base main@0dfba37b11ff2ca908ae2df85b55f4f4c9b7fd8b. I read the complete three-file diff, the full current Discord adapter around the affected paths, the canonical tools.url_safety transport contract, both new regression files, the exact-head workflow state, the open predecessor/sibling graph, and the Discord decomposition record. There were no existing reviews or PR comments when I started.

The security mechanism itself is careful work. In particular:

  • direct HTTP(S) goes through the repository's create_ssrf_safe_async_client, whose contract resolves/validates at connect time and dials the vetted IP while preserving Host/SNI;
  • redirects are manual and re-enter the same boundary instead of inheriting one hostname check;
  • decoded response bytes are streamed through a hard per-response aggregate limit rather than trusting Content-Length;
  • the 100 MiB batch budget survives Discord's 10-file chunking and the native→base fallback through one task-local budget object;
  • rejected/non-200 bodies count toward the batch budget, while an overdeclared-but-unread response does not falsely consume bytes;
  • attachment type is derived from PNG/JPEG/GIF/WebP bytes, with GIF required for animation;
  • the DNS-rebind regression reaches the real SSRF-safe httpx transport seam and proves that the private second resolution is refused before the raw backend connect.

I did not find a second security bypass in those mechanics. There is, however, one hard landing blocker and two important composition edges.

P1 / hard architecture gate — this security fix regrows the Discord godfile instead of using its accepted media shard

plugins/platforms/discord/adapter.py is not an unowned legacy file. #78634 is the live decomposition owner and explicitly records the repo-wide rule that this ~10K-line adapter is sharded and never reverted/regrown. The Discord decomposition campaign likewise says new behavior must land in a <=2K module/shard, and the media-send surface already has a concrete owner: #79652 extracts send_multiple_images, send_image, send_animation, and the neighboring media/typing cluster into DiscordMediaSendMixin.

This head instead adds the new fetch authority in several places directly in the monolith: the ContextVar/redirect client helpers near the module top, the batch-budget wrapper plus send_multiple_images changes, the send_image/send_animation changes, and _DiscordImageDownloadBudget / _read_response_bytes_bounded around the ~9960-line region. The implementation is good, but placing it here makes the killed file the authority root again and creates exactly the future collision surface the shard train exists to remove.

Required repair: preserve these semantics but move the new outbound-fetch/budget machinery into a focused <=2K Discord media/fetch module, then compose the three sender overrides through the #79652 media-send seam (or first rebase that extraction onto current main and apply this security delta there). adapter.py should be reduced to the minimum delegate/re-export compatibility surface required by existing callers/tests. Keep seam-identity/monkeypatch compatibility explicit when retargeting tests; do not solve this by copying the helpers into both modules.

A clean restack should rerun the 42 focused tests, the 83 URL-safety/attachment/document neighbors, the Discord media shard seam tests, git diff --check, attribution, Windows footguns, and exact-head hosted CI.

Interlocks / contributor ownership

  • #60395 / isheng-eqi — superseded source, credit preserved. This head correctly carries the two original commits as a030399d… and 4d629a71… with isheng still the Git author, then adds big-mon's hardening. That is the right salvage shape. Once this replacement is ready, #60395 should be marked/closed as superseded rather than left as a second implementation candidate.
  • #79652 — structural owner of this exact media-send neighborhood. It is complementary architecture, not competing behavior. The security delta belongs on that seam; preserve its extraction/identity contract rather than landing new bytes back into adapter.py.
  • #40255 / nftpoetrist — complementary async-DNS fix. It already owns the known event-loop blocking class caused by synchronous is_safe_url() inside Discord's async media handlers, including these same call sites. #96540 should not silently supersede it: when this work is rehomed, compose its async_is_safe_url policy into the new media/fetch owner (or declare the landing order) so the SSRF hardening does not retain the synchronous-DNS stall class.
  • #96526 and #96527 — adjacent fresh Discord voice work. Both currently touch adapter.py but own voice doctor/policy, not outbound image fetches. Rehoming this media security work materially reduces that collision surface; no reason to fold their behavior together.
  • #62932 → merged #69482 — historical redirect-validation lineage. This PR correctly builds on that provenance rather than cherry-picking another redirect implementation.

Exact-head evidence

The local focused/adjacent receipts in the body are useful development evidence, but this exact GitHub object is not hosted-green yet. CI 33099887439, Docker 33099885299, Nix 33099885438, and the current label-rerun workflows are all action_required; the CI run currently exposes zero jobs, so there is no failing test result to attribute and no exact-head acceptance receipt to inherit from another commit.

Once the media authority is moved behind the accepted shard boundary, the overlap graph is reconciled, and the exact restacked head gets real hosted execution, the actual SSRF/body-budget/content-validation work looks strong. The negative tests here are doing useful security work rather than merely mirroring implementation details. 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/plugins Plugin system and bundled plugins needs-repro Bug needs reproduction steps P3 Low — cosmetic, nice to have platform/discord Discord bot adapter type/security Security vulnerability or hardening

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants