fix(slack): gate inbound file downloads behind a host allowlist (#1951)

[dolho]

Closes #1951

Problem

SlackService.download_file fetched url_private_download with no host allowlist, no scheme check, and follow_redirects=True — redirects blind-followed to any host. Both siblings already validate; Slack was the one that never did, across three consecutive CSO audits.

Channel	Host allowlist	Redirects
Telegram	api.telegram.org, exact	n/a
WhatsApp (#1932)	two-tier suffix	follow_redirects=False, each hop re-validated
Slack (before)	none	follow_redirects=True

Why it mattered despite two mitigations

The issue is honest that the URL is HMAC-verified Slack input and that httpx strips Authorization cross-origin. Both hold — and both are transitive: one trusts Slack's field, the other trusts httpx's behaviour. Neither is a property of Trinity's code. What is ours:

• the fetch is issued from the backend, which is on both Docker networks — so an arbitrary-host fetch reaches internal services agents deliberately cannot route to
• the bytes are returned to message_router and written into an agent workspace, so this is a non-blind primitive, stronger than a blind SSRF

Fix — the #1932 shape, not a third bespoke allowlist

_SLACK_FILE_SOURCE_HOST_SUFFIXES  = (".slack.com",)                                    # hop 1: carries the bot token
_SLACK_FILE_ALLOWED_HOST_SUFFIXES = (".slack.com", ".slack-edge.com", ".slack-files.com")  # validated 30x targets, no token
_MAX_FILE_REDIRECTS = 3

follow_redirects=False, every hop re-validated before it is issued, https-only, refusals at ERROR.

*.slack.com rather than an exact files.slack.com, deliberately. #1932's real lesson is that an allowlist which rejects the legitimate host scores as "allowlist present" in an audit while being 100% broken — WhatsApp media was dead for three months that way. A Slack-apex suffix can't break on a files-N.slack.com variant and still confines the token to Slack. If Slack ever redirects somewhere outside the tiers, it fails loudly at ERROR with the host named, and the fix is one allowlist line.

Verification

Before/after against the pre-fix module through the same fake transport:

BEFORE (origin/dev)  https://169.254.169.254/latest/meta-data/  requests=1  follow_redirects=True  bytes=b'INTERNAL-RESPONSE'
BEFORE (origin/dev)  http://172.29.0.2:5432/                    requests=1  follow_redirects=True  bytes=b'INTERNAL-RESPONSE'
BEFORE (origin/dev)  https://evil.example.com/x                 requests=1  follow_redirects=True  bytes=b'INTERNAL-RESPONSE'

AFTER  (this branch) all three                                  requests=0                         bytes=None

172.29.0.2:5432 is this machine's actual trinity-postgres on the platform network — the reach the issue describes, demonstrated rather than asserted.

• 26 new tests, plus all 355 existing Slack unit tests green
• Every negative test asserts the recorded call log, never is None alone — download_file returns None on every failure path and its bare except Exception swallows AssertionError, so a return-value assertion passes against a completely unpatched seam (the AC's point, and test_whatsapp_inbound_media.py's documented lesson)
• The legitimate files.slack.com download still succeeds end-to-end, and the CDN-redirect test asserts the second hop carries no Authorization

One test earns its place specially: evil-slack-files.com passes a dotless endswith("slack-files.com") check. Every allowlist entry must therefore start with a dot, pinned by test_every_allowlist_entry_starts_with_a_dot.

Acceptance criteria

• Host allowlist validated before any request; non-https refused
• No blind redirect following — follow_redirects=False, per-hop re-validation, bounded budget
• Refusals log at ERROR
• Regression tests assert the call log / hop count
• A legitimate Slack download still succeeds

Deliberately not folded in

The shared channel_media_fetch helper (would touch two channels that were just fixed — better as its own change with all three test suites as the safety net), the private-IP/DNS-rebinding resolution check, and a true streaming size bound. The latter two are accepted residuals on #1932 and belong with that helper; the size cap here stays honest about bounding what is accepted, not what is buffered, exactly like both siblings.

Related to #1951

🤖 Generated with Claude Code

[vybe]

Validated via /validate-pr. Closes #1951 (closing keyword added so the issue auto-promotes).

Security fix, no schema/API/config surface. The two flagged strings in my scan are test fixtures (xoxb-test-token, 169.254.169.254 as a redirect target), not credentials.

What I checked and agreed with:

• Reuses the #1932 two-tier shape rather than inventing a third bespoke allowlist — source tier (.slack.com, carries the bot token) strictly narrower than the redirect tier, follow_redirects=False, every hop re-validated before it is issued, bounded budget, https-only.
• *.slack.com over an exact files.slack.com is the right call. #1932's actual lesson is that an allowlist rejecting the legitimate host scores as "allowlist present" in an audit while being 100% broken — WhatsApp media was dead three months that way. A too-narrow list here fails silently; a too-broad-within-Slack one fails loudly at ERROR with the host named.
• Every negative test asserts the recorded call log, never is None. download_file returns None on every failure path and its bare except Exception swallows AssertionError, so a return-value assertion passes against a completely unpatched seam. This is the vacuous-negative class from #1932/#1917, and it's handled.
• test_every_allowlist_entry_starts_with_a_dot pins the evil-slack-files.com dotless-suffix bypass — the kind of guard that stops the next entry from reintroducing it.

The before/after demonstration against the real trinity-postgres on the platform network makes the reachability argument concrete rather than theoretical.

Merged dev and resolved the tests/registry.json append conflict (both entries kept; JSON re-parsed, no new duplicates, touched-file set unchanged).