fix(security): DNS-pinned SSRF-safe fetches + Slack CDN allowlist, backfill injection, token perms - #70193
Merged
Conversation
Install connect-time DNS validation for Hermes-owned direct httpx clients so SSRF-sensitive fetch paths dial a vetted IP instead of re-resolving after preflight. This preserves Host/SNI semantics for direct HTTP(S) connections and keeps proxy routing as an explicit trusted egress boundary. Wire the guarded clients into media cache downloads, vision downloads, Skills Hub direct/raw fetches, and platform attachment fetch paths that already perform SSRF preflight and redirect validation. Fixes #8033 Co-authored-by: Tom Qiao <zqiao@microsoft.com>
Several platform fetch paths called is_safe_url before constructing ordinary httpx clients, leaving a second DNS lookup at connection time. This preserved the rebinding window for Slack batch images, Feishu documents, Telegram URL-photo fallback, and WeCom remote media. Route each path through create_ssrf_safe_async_client and the shared redirect guard so direct connections validate and dial vetted IPs while configured proxies remain an explicit trusted egress boundary. Add per-path regressions that change DNS from public at preflight to metadata at connect time. The Skills Hub provenance fixture intentionally serves content over loopback. Opt that test-scoped server into private-address access so it keeps exercising the real HTTP transport without weakening production blocking. Related #8033 Co-authored-by: teknium1 <127238744+teknium1@users.noreply.github.com>
_download_slack_file and _download_slack_file_bytes fetched Slack-supplied URLs with the bot token attached and follow_redirects=True, but without the is_safe_url pre-flight check or per-redirect guard that the outbound send_image path already uses. A URL that resolves to (or 3xx-redirects into) a private/internal address could reach internal services and leak the bot token (CWE-918). Add the same pre-flight + _ssrf_redirect_guard hook to both inbound sibling paths.
`SlackAdapter._fetch_thread_context` formats each prior thread message as
`{name}: {msg_text}` and joins them with newlines into the block the call
site prepends *raw* into the model turn (`text = thread_context + text`).
Both fields are attacker-influenceable — any thread participant sets their
own Slack display name and message text — and neither was neutralized, so
an embedded newline let a thread message break out of its line and pose as
a fresh markdown section (a fake "## SYSTEM" / "## Override" heading) inside
the context the model reads when first mentioned mid-thread.
This is the same indirect-prompt-injection vector already closed for the
sibling untrusted sinks: the sender-name prefix
(`neutralize_untrusted_inline_text`), the reply quote, and the relay
channel-context renderer. The Slack thread-context backfill — the default
whenever the bot is mentioned in a thread with no active session — was the
missed sink. (The existing `[unverified]` tagging marks *who* a message is
from; it does nothing about newline structure, so an authorized sender can
inject just as easily.)
Flatten both fields with `neutralize_untrusted_inline_text` before
interpolation. The body uses `max_chars=0` so message text is not truncated
(thread context caps the message *count*, never per-message length); the
display name keeps the default bound. `parent_text` keeps the raw message
(its own reply-context sink neutralizes separately). A well-behaved message
is preserved byte-for-byte.
Adds a regression test covering a hostile display name, a hostile message
body, benign passthrough, and the no-truncation guarantee.
The OAuth multi-workspace token file contains plaintext bot tokens for all saved Slack workspaces. Unlike the Google Chat adapter which sets 0o600 when writing credentials, the Slack token file has no permission enforcement — a default umask 022 makes it world-readable. Fix: check file permissions on read and emit a warning log with remediation instructions if the file is group- or world-readable.
…ds, widen token-file perms warning Follow-up hardening on top of the C14 cherry-picks (#57860/#44026/#66742/#60009): - Slack file downloads (_download_slack_file/_download_slack_file_bytes) now require an https URL on a Slack CDN host (files.slack.com, *.slack.com Enterprise Grid, *.slack-files.com legacy shares) before attaching the bot token. url_private/url_private_download only ever point at the Slack CDN, so a forged file object from a malicious workspace app or compromised event stream pointing the Bearer-token download at an arbitrary PUBLIC host (token exfiltration) is now refused — a hole #44026's generic private-IP SSRF check alone could not close. - The same two download paths now use create_ssrf_safe_async_client (from #57860) so the preflight-validated hostname is resolved once, validated, and dialed by IP — closing the DNS-rebinding TOCTOU window for the token-bearing inbound fetches as well. - #60009's slack_tokens.json permission warning is generalized into utils.warn_if_credential_file_broadly_readable() (POSIX-only, fail-quiet) and wired into the other read path with the same gap: google_chat's load_user_credentials(). google_chat already writes 0o600 via _write_private_json; the read-time warning covers hand-provisioned/legacy files. Nothing in-repo writes slack_tokens.json (user/OAuth-provisioned), so there is no write path to chmod for Slack. Security tests both directions: non-CDN/lookalike/http URLs and connect-time DNS rebinds are blocked before any TCP connect; real files.slack.com, Enterprise Grid, and slack-files.com URLs still reach the network layer; 0o600 files stay silent while 0o644/0o640 warn with a chmod hint. A/B: all 10 new download-guard tests fail with the hardening reverted and pass with it applied.
Contributor
૮ >ﻌ< ა ci reviewran on 1508858 all good! |
alt-glitch
added
type/security
This was referenced Jul 23, 2026
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
6 participants
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.
Summary
Four security hardenings: DNS-pinned SSRF-safe fetches gateway-wide (kills the rebinding TOCTOU that URL validation alone misses), Slack file URLs allowlisted to Slack CDN domains, prompt-injection neutralization in thread backfill on top of wave-1's trust-tags, and token files locked to 0600 with warnings on loose permissions.
Changes
create_ssrf_safe_(async_)clientintools/url_safety.py— resolve once, validate IP, connect to the validated IP (Host/SNI preserved); wired into base.py cache helpers, vision, skills hub, and 8 platform adapters. Proxies remain an explicit trusted egress boundary (fix(security): pin DNS resolutions for SSRF-safe fetches #57860, fixes Security: DNS rebinding TOCTOU bypasses SSRF protection in url_safety.py #8033).*.slack.com/*.slack-files.com(fix(security): validate inbound Slack file URLs against SSRF #44026, hand-ported to the plugin adapter). Note: extend the suffix tuple if a GovCloud/China variant host surfaces._format_thread_context).slack_tokens.jsonchmod 600 on write + warn on read; widened to the token-file class (fix(slack): warn when slack_tokens.json is group/world-readable #60009).Credits
Salvaged with authorship preserved: #57860 (@egilewski), #44026 (@zapabob), #66742 (@Frowtek), #60009 (@AlexFucuson9).
Validation
tests/gateway/ -q -k slackInfographic