Skip to content

fix(platforms): guard media redirects against SSRF - #55942

Open
necoweb3 wants to merge 1 commit into
NousResearch:mainfrom
necoweb3:fix/platform-media-redirect-ssrf-guard
Open

fix(platforms): guard media redirects against SSRF#55942
necoweb3 wants to merge 1 commit into
NousResearch:mainfrom
necoweb3:fix/platform-media-redirect-ssrf-guard

Conversation

@necoweb3

Copy link
Copy Markdown
Contributor

Summary

This closes redirect-based SSRF gaps in Feishu and WeCom remote media/document downloads.

Why

Both paths validated the initial URL with is_safe_url(), but then downloaded with follow_redirects=True.

That leaves a redirect gap: an attacker-controlled public URL can pass the initial safety check and then redirect to a private/internal address such as cloud metadata. The shared gateway redirect guard already exists for this exact class; these paths were not using it.

Affected paths:

  • plugins/platforms/feishu/adapter.py::_download_remote_document
  • plugins/platforms/wecom/adapter.py::_download_remote_bytes

Changes

  • Add _ssrf_redirect_guard to Feishu remote document download httpx.AsyncClient.
  • Add _ssrf_redirect_guard to owned WeCom download clients.
  • Add a final response.url safety check in WeCom so injected/shared clients that do not carry the event hook still cannot return redirected internal content.
  • Add regression coverage for Feishu redirect hook wiring and WeCom unsafe final URL rejection.

Tests

python -m pytest tests/gateway/test_feishu.py -k "download_remote_document" -q --timeout-method=thread
1 passed, 204 deselected

python -m pytest tests/gateway/test_wecom.py -k "download_remote_bytes" -q --timeout-method=thread
2 passed, 45 deselected

python -m pytest tests/gateway/test_wecom.py -q --timeout-method=thread
47 passed

@alt-glitch alt-glitch added type/security Security vulnerability or hardening comp/gateway Gateway runner, session dispatch, delivery platform/feishu Feishu / Lark adapter platform/wecom WeCom / WeChat Work adapter sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data P2 Medium — degraded but workaround exists labels Jun 30, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related to the SSRF-redirect-guard cluster: merged #54470 (yuanbao) and #7151 (Slack), and open #22340 / #43938 (yuanbao) and #24831 (Mattermost). Same vulnerability class (initial is_safe_url() then follow_redirects=True lets a redirect reach an internal address), but this PR covers previously-unguarded code sites — Feishu _download_remote_document and WeCom _download_remote_bytes — so it is related_to, not a duplicate.

@egilewski

Copy link
Copy Markdown
Contributor

looks mergeable

Security evidence:

  • trust boundary: model- or user-controlled remote Feishu document and WeCom media URLs must not be able to pass an initial public URL check and then follow redirects to private/internal metadata endpoints.
  • source/sink/invariant: FeishuAdapter._download_remote_document() now attaches the shared _ssrf_redirect_guard to its redirect-following httpx.AsyncClient, and WeComAdapter._download_remote_bytes() attaches the same hook for owned clients plus revalidates response.url before reading bytes from owned or injected clients.
  • current-main reproduction: a run-root probe imported the adapters from the current-main worktree and showed Feishu accepted bytes after a simulated redirect to 169.254.169.254, while WeCom returned secret from an injected client whose final response.url was 169.254.169.254.
  • PR-head or patch-replay validation: the same probe imported the adapters from the PR-head worktree and showed Feishu blocked the unsafe redirect via _ssrf_redirect_guard, while WeCom blocked the unsafe final response.url before reading bytes; local git merge-tree against current GitHub main succeeded despite the earlier setup advisory.
  • positive/negative cases: the PR's tests cover Feishu redirect-hook wiring, WeCom unsafe final-URL rejection for injected clients, and the existing WeCom size-limit path with a safe final URL; the affected Feishu and WeCom test files pass together with 252 tests.
  • residual bypass search: shared image/document cache downloads already use _ssrf_redirect_guard, Feishu's remote image path delegates to that helper, and WeCom's final-URL check covers the long-lived client used for media downloads even when the client was created without the new hook.
  • reviewer validation: CodeRabbit completed with no findings in the clean-pass flow.

Review setup note: I reviewed a run-owned local merge/replay setup against current GitHub main because the automation reported a stale/conflict setup advisory; this does not by itself prove the submitted branch merges cleanly.

Signed: GPT-5.5-xhigh in Codex

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for covering the Feishu download path and for retaining a defensive final-URL check for injected WeCom clients.

Problems

  • Blocking: The WeCom hook only applies to the fallback client at plugins/platforms/wecom/adapter.py:1082-1086 in PR 6ba1accba85e. In normal operation, connect() creates and stores self._http_client with follow_redirects=True but no hook at plugins/platforms/wecom/adapter.py:222-224; _download_remote_bytes() prefers that client. The new check at PR line 1098 runs only after httpx has followed the redirect and received the final response, so it cannot prevent the SSRF request itself.

Suggested changes

  • Attach _ssrf_redirect_guard to the persistent client created by connect() and add a test for that construction path. Keep the final-URL check as defense for injected clients that do not carry the hook.

Automated hermes-sweeper review.

client = self._http_client or httpx.AsyncClient(
timeout=30.0,
follow_redirects=True,
event_hooks={"response": [_ssrf_redirect_guard]},

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This hook only protects the fallback client. A connected adapter uses self._http_client, which connect() constructs with follow_redirects=True and no event hook (adapter.py:222-224 on current main). The final-URL check below happens after the redirected request, so please install this guard on that persistent client as well.

@teknium1 teknium1 added the sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform label Jul 15, 2026
@egilewski

Copy link
Copy Markdown
Contributor

superseded

Current main already contains a broader fix from #70193 (0cd4afeafd) for these Feishu and WeCom fetches. This PR's normal WeCom connection still creates a persistent client with follow_redirects=True and no response hook, so the final-URL check happens only after that client has already followed the redirect. The merged implementation creates that persistent client, the fallback client, and the Feishu document client through create_ssrf_safe_async_client with _ssrf_redirect_guard, covering both redirects and connect-time DNS rebinding.

Security evidence:

  • trust boundary: user-controlled Feishu document and WeCom media URLs must not reach private/internal addresses through redirects or DNS rebinding.
  • source/sink/invariant: the old head leaves the normal WeCom persistent client unguarded; current main applies the SSRF-safe factory and redirect guard to every affected client construction path.
  • current-main reproduction: the persistent-client probe observed the safe factory and response hook, and the focused WeCom and Feishu DNS-rebinding regressions passed.
  • PR-head or patch-replay validation: the exact-head probe observed follow_redirects=True with no response hook on the persistent WeCom client; the PR's affected tests pass but do not cover that construction path.
  • positive/negative cases: the old head protects the fallback-client and final-URL cases, while current main additionally blocks the normal persistent-client request before unsafe network I/O.
  • residual bypass search: fix(security): DNS-pinned SSRF-safe fetches + Slack CDN allowlist, backfill injection, token perms #70193 also replaces the Feishu and WeCom clients with DNS-pinned safe clients, so it is a strict superset of this redirect-only patch.
  • reviewer validation: source inspection, exact-head/current-main probes, merged-commit ancestry, and focused tests all support closing this PR in favor of fix(security): DNS-pinned SSRF-safe fetches + Slack CDN allowlist, backfill injection, token perms #70193.

Not checked:

  • CodeRabbit review

Signed: GPT-5.6-sol-xhigh in Codex

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

Labels

comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists platform/feishu Feishu / Lark adapter platform/wecom WeCom / WeChat Work adapter sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/security Security vulnerability or hardening

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants