fix(whatsapp): allow Twilio's media CDN as a validated redirect target (#1932) - #1937
Conversation
#1932) Inbound WhatsApp media has never worked. Twilio serves message media from mms.twiliocdn.com, reached via a 302 from api.twilio.com, but the SSRF allowlist accepted only the .twilio.com suffix — so every attachment was refused at the redirect gate and download_file returned None (since #463). Widen only the gate that had the bug. The allowlist is now two tiers: _TWILIO_MEDIA_SOURCE_HOST_SUFFIXES (*.twilio.com) — webhook MediaUrl{N} parse gate + hop 1, the hop that carries the tenant's Basic auth _TWILIO_MEDIA_ALLOWED_HOST_SUFFIXES (+ *.twiliocdn.com) — validated redirect targets only, always unauthenticated The credentialed hop stays exactly as narrow as before, so the widening doesn't depend on the webhook HMAC gate holding forever. s3-external-1 .amazonaws.com (the target for accounts without media auth) stays out: it is path-style, so allowlisting it admits arbitrary buckets under an allowlisted host. Also: follow_redirects=False stays and every hop is still re-validated, but the single manual follow becomes a bounded budget (_MAX_MEDIA_REDIRECTS=3) so a future 302→302 chain degrades to "still works"; a transport size cap (_WA_MEDIA_DOWNLOAD_MAX_BYTES, parity with Telegram/Slack); the off-domain refusal logs at ERROR; and the generic handler logs the exception type only (some httpx exceptions embed the URL, and the signed CDN URL is a ~4h bearer capability). download_file's signature and its None-on-failure contract are unchanged (#1933 depends on them). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…1932) New tests/unit/test_whatsapp_inbound_media.py — all download_file coverage lives here, NOT in tests/test_whatsapp_adapter.py whose module-level `db = MagicMock()` makes the credentials guard pass vacuously. download_file returns None on every failure path and its bare `except Exception` swallows AssertionError, so `result is None` alone is a vacuous assertion. Every test here asserts the recorded (url, auth) call list — a broken harness (unpatched db seam, capital-L Location key, assert inside the fake) shows up as a wrong hop count instead of a green pass. TDD evidence: against the un-widened constant 6 of the 23 fail, including the AC #5 headline test_redirect_to_cdn_returns_bytes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…1932) Extends TestTwilioMediaUrlAllowlist in place. The six #463 spoof tests keep byte-identical bodies and node-ids — the only removed line in this file is the class docstring, which claimed "only *.twilio.com hosts allowed" and is now false. Adds: mms./media.twiliocdn.com accepted, bare apex, case-insensitivity; and the CDN-shaped spoofs — eviltwiliocdn.com, twiliocdn.com.evil.com, non-https, userinfo @, trailing-dot FQDN, punycode lookalike, scheme-relative. Pins D2 (S3 refused, path-style rationale in the docstring) and D3 (the source predicate excludes the CDN, and inherits every spoof guard). Pins both named constants, incl. source ⊆ allowed. Also pins the parse gate at the narrow tier: a twiliocdn MediaUrl0 off the wire yields msg.files == []. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- architecture.md: the catalog entry claimed a single `*.twilio.com` gate; name both tiers in one clause. - feature-flows/whatsapp-integration.md: rewrite "SSRF defense on media downloads" — the actual two-hop chain, the two-tier allowlist table, the bounded follow budget, the no-env-override decision and why, the S3 exclusion and why, and the transport size cap. Status line notes #1932. - requirements/public-access.md: ":338" asserted "*.twilio.com only, no redirects to other hosts" — after this change BOTH clauses are false, and it is a live security claim, so it is corrected rather than left standing. ":309" goes stale the same way. - user-docs/integrations/whatsapp-integration.md: the mitigation the no-env-override decision actually leans on. Prerequisites gains the opt-in Twilio Console setting inbound media requires; Troubleshooting gains "Inbound images arrive as — download failed" (with the exact log line and what each host means) and distinguishes it from the separate "unsupported format" policy rejection. docs/security-reports/ is untouched — historical point-in-time report. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
/sync-feature-flows: the only changed code file is src/backend/adapters/whatsapp_adapter.py, which maps to the existing whatsapp-integration.md flow (already rewritten in the previous commit). No new flow doc — this is a bug fix inside an existing adapter, not a new feature. Adds the Recent Updates row, matching the fix(...) rows that already point at existing flow docs (#1445, #1444, #903). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…line (#1932) /review follow-ups on the #1932 SSRF fix. No behaviour change. 1. The two-tier constant comment cited `transports/twilio_webhook.py:123-128` and `:140-142` for the HMAC gate its security reasoning leans on. Both were accurate at 8e92452, but that file is live and the citation is exactly the kind a future reader must actually follow to re-verify the argument. Anchored on symbols instead (`TwilioWebhookTransport.handle_webhook`, `RequestValidator(...).validate(...)`, `raw_event = dict(params)`), which survives line drift. 2. The user-doc troubleshooting entry enumerated three of the media log lines an operator can hit and missed two — including the generic handler, which is now the LEAST self-explanatory path: narrowing it to `type(e).__name__` (so a signed CDN URL embedded in an httpx message can never reach the log) collapses DNS, TLS and connection-refused into a bare `ConnectError`. Replaced the prose with a table covering all five, and said plainly why the type name is all there is, so nobody reads it as a bug. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Resolve by running |
AndriiPasternak31
left a comment
There was a problem hiding this comment.
Ran /review + /validate-pr + /cso --diff against 8946bf70.
Verdict: 0 critical, 0 blocking security findings. Not approving yet — only because AC #1's operator check hasn't run and AC #9 is still open. Both are yours to close; neither needs a code change. Details below, plus one two-word fix I'd take before merge and some vendor evidence that materially de-risks AC #1.
The diff is genuinely clean and I'd rather say so than manufacture objections. The two-tier split is the right call: the credentialed hop's blast radius is byte-for-byte what it was, and only the unauthenticated redirect-target tier widened.
What I verified independently (rather than trusting the PR body)
Anti-vacuity — reproduced first-hand. Since download_file returns None on every path, I re-ran two of your deliberate breaks rather than taking the table on trust:
| Break | Your claim | My result |
|---|---|---|
| B — revert the widening | 12 red across both files | 12 red ✅ |
C — re-attach auth= to the redirect hop |
exactly 2 red | exactly 2 red ✅ |
Tree restored clean after each (git diff --stat empty). The hop-count assertion discipline does what you say it does.
35-vector urlparse ↔ httpx.URL differential corpus → 0 exploitable bypasses. I scored by parser disagreement (matcher reads host X, socket goes to host Y), since that's the only place a real bypass can hide. Three vectors disagree; all three fail closed — httpx raises InvalidURL or yields an empty host before any connection, and the raise lands inside your try → return None:
| Vector | urlparse host |
httpx | Outcome |
|---|---|---|---|
https://mms.twiliocdn\t.com/x |
mms.twiliocdn.com (allowed) |
InvalidURL |
no request |
https://ｍms.twiliocdn.com/x (full-width m) |
ｍms.twiliocdn.com (allowed) |
InvalidURL |
no request |
␣https://mms.twiliocdn.com/x |
mms.twiliocdn.com (allowed) |
empty host | no request |
You named the leading-space case. The tab and full-width cases are two you didn't — both land in the same safe direction, and all three are unreachable from a parsed Location header anyway.
Correctly refused: prefix-not-suffix, suffix-append, userinfo (@ and %40), backslash authority, CR/LF/NUL, Unicode dot variants (U+3002 / U+FF0E / U+FF61 in both positions), Cyrillic homoglyph (IDNA → mms.xn--twilicdn-rbh.com), Turkish dotted-İ, trailing-dot FQDN, http://, IP literals incl. link-local metadata and [::1], query/fragment decoys, and both S3 forms.
Multi-Location re-measured on real httpx.Headers: attacker-first → denied; CDN-first → httpx still resolves mms.twiliocdn.com, trailing attacker URL degrades to path bytes. No attacker host is ever contacted. Matches your finding.
The newly-live downstream. This is the angle I'd flag as most under-examined, and it holds up. The fix resurrects a path dead since #463, so nothing after download_file has been exercised in prod for three months. Traced it:
upload_service.py:190—actual_size = len(data), declared size explicitly advisory. This one matters:whatsapp_adapter.py:327hardcodessize=0, so a declared-size-trusting sink would have had no effective size limit. It doesn't.sanitize_filename()on every branch;[^\w.\-()]strips separators, so a malformedMediaContentTypecan't traverse via the synthesizedmedia_{i}.{ext}.- Magic-byte MIME cross-check rejects declared/detected mismatches outside the image↔image and text↔text families.
AC #1 — better than "unverifiable"
You flagged this as resting on one empirical vendor claim. It's actually vendor-documented:
"Requests to fetch your media will redirect you to a secure URL that is only valid for 4 hours… The domain of the unsecured media URL is
s3-external-1.amazonaws.comand secured media URL ismms.twiliocdn.com."
— Twilio: Protect Media Access with HTTP Basic Authentication
One line confirming both the fix and the S3 exclusion. And the July 2023 changelog adds the part I'd fold into the docs:
"Newly-created main accounts will have HTTP Basic Authentication enabled without the option to disable it." (effective 2023-07-31)
So media-auth isn't just "the supported configuration" — it's mandatory for every account created in the last three years. The S3 residual only reaches accounts grandfathered before July 2023. That's a stronger position than the PR body currently claims, and worth stating that way in the user doc.
Also worth noting: a 2016 report of Twilio's chain landing on media.twiliocdn.com.s3-external-1.amazonaws.com in two redirects — real-world evidence that your bounded budget (rather than a single follow) was the right call. That host is correctly refused by the matcher (prefix, not suffix); I tested it.
Still run the curl — the docs establish Twilio's behaviour, not Trinity's end-to-end delivery. Cheaper alternative: send a PDF. If the placeholder flips from — download failed to — unsupported format, the redirect chain provably worked and bytes were returned (upload_service.py:166 vs :174). Your PR body frames the PDF case defensively — it's actually the crispest cheap end-to-end signal available.
One thing I'd fix before merge
The fix leaves its own root cause half-repaired. #1932's root cause wasn't the allowlist — it was that a fail-closed security gate logged its rejection at WARNING, hiding a 100% outage for three months. This PR raises the redirect gate to logger.error (:764), but both SOURCE-tier gates stay at WARNING:
:314 if not _is_twilio_media_source_url(media_url):
:315 logger.warning("[WHATSAPP] Rejecting non-Twilio media URL at parse time: %s", ...)
:728 if not _is_twilio_media_source_url(file.url):
:729 logger.warning("[WHATSAPP] Refusing to download non-Twilio media URL (host=%s)", ...)Before the split these shared one constant and couldn't diverge. The two-tier design makes MediaUrl{N} strictly stricter than the redirect target — a new asymmetry, guarded by the quieter level. If Twilio ever puts a CDN-form URL in MediaUrl{N} (exactly the class of vendor change that caused #1932), every attachment fails again at WARNING. Empirical detection latency for that signature: three months.
8946bf70's message says it covers "every media log line" — these two are the exceptions. Two-word change, no behaviour change.
AC #9 is still open
#1315 is CLOSED with 2 comments (/claim + the bot); no correction posted. The AC says "a comment on the closed issue is enough" — the correction currently lives only in a PR body that nobody reading #1315 will encounter, which is the information loss the AC existed to prevent. Same for #468 item 3d.
On the Slack finding
Confirmed at slack_service.py:739. Agree it's out of scope here, and I'd keep the severity honest — the URL comes from an HMAC-verified Slack event and httpx strips Authorization cross-origin, so the bot token doesn't travel. But the trend is the argument for filing it:
| Audit | Cited as | Status |
|---|---|---|
cso-2026-06-21.md:159 |
slack_service.py:639 |
VERIFIED |
cso-2026-07-13.md:31 |
slack_service.py:702 |
PERSISTENT |
| today | slack_service.py:739 |
PERSISTENT — 3rd cycle |
Six weeks, three audits, no tracker entry — and the citation is drifting as the file grows. A finding that lives only in dated report files is losing its own address.
One footnote I found bleakly instructive: the 2026-06-21 report cited the WhatsApp adapter as the good example Slack should copy — "unlike the Telegram and WhatsApp adapters that validate host." At that moment this allowlist was rejecting 100% of legitimate media. "Allowlist present" scored as correct without anyone checking it admitted the legitimate host.
Minor
- Stale claim, two places.
feature-flows/whatsapp-integration.md:17andrequirements/public-access.md:309both still summarise inbound as "images/audio/PDFs". You corrected the SSRF clause on:309but left the media-types clause your own analysis contradicts (upload_service.py:45-49rejectsaudio/andapplication/pdf). - 16 MB cap doesn't bound memory —
len(resp.content)runs post-read. Your comment at:92-96says this outright so it's honest; noting only that the constant's name reads like a DoS control. Filed follow-up is the right home. - Port unconstrained — confirmed
https://mms.twiliocdn.com:8443/is accepted. Named as a residual; needs an attacker to already hold a*.twiliocdn.comname. - Confirmed
--skip-agentwas correct here: nodocker/base-image/file and no new backend module in the diff. Andtest_1069_voip_call_path_paramis pre-existingfastapi>=0.115.0floor drift — should not be attributed to this PR.
To convert this to an approval: the AC #1 check (or the PDF placeholder-flip signal) and AC #9's comment on #1315. The F1 log-level change is a strong suggestion, not a gate.
Process surface is complete — all four memory surfaces plus user docs, closing keyword present, full label set, 23/23 CI green across six pytest seeds, and a named regression test whose non-vacuity I confirmed myself.
🤖 Review assisted by Claude Code
# Conflicts: # docs/memory/feature-flows.md
…iew) #1932's root cause was never the allowlist — it was that a fail-closed gate logged a 100% inbound-media outage at WARNING, where it sat unnoticed for three months. The fix raised only the redirect gate to ERROR; both SOURCE-tier gates (webhook `MediaUrl{N}` parse, credentialed hop 1) stayed at WARNING. Before the two-tier split these shared one constant and could not diverge. The split makes SOURCE strictly *stricter* than the redirect tier, so it is now the asymmetry most likely to break on the next vendor change — the exact #1932 signature — and it was the half still guarded at the quieter level. Both are now ERROR, pinned by `TestSourceTierRejectionsAreVisible` (mutation- proven: reverting either level REDs the pair on `{30} != {40}`), since the level is a one-word revert with a three-month observed detection latency. Also corrects two stale doc claims flagged in review. `feature-flows/ whatsapp-integration.md` and `requirements/public-access.md` both described inbound media as "images/audio/PDFs", which `upload_service.UNSUPPORTED_MIMES` contradicts — it rejects `application/pdf`, `audio/`, `video/` and archives for every channel, so only images (plus text/CSV/JSON) reach a workspace. A third claim was wrong in the same place and is corrected here: WhatsApp has no voice-transcription route at all. `message_router._maybe_transcribe_voice` returns early for `channel != "telegram"` and calls `telegram_media .process_voice` — it never touches `adapter.download_file`, so a WhatsApp voice note is fetched and then rejected as "unsupported format", full stop. User doc: media auth has been mandatory on accounts created since 2023-07-31 (per Twilio's changelog), so the S3 residual reaches only pre-July-2023 grandfathered accounts — most operators need no action. Refs #1932 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
trinity-ability
left a comment
There was a problem hiding this comment.
/validate-pr: PASS.
- Base
dev✅ · 8 files ✅ ·Fixes #1932closing keyword present, issue auto-promotes ✅ - Security scan clean. The
api.twilio.com@evil.com/mms.twiliocdn.com@evil.comstrings are userinfo-spoof test vectors, and_host_matches_suffixesreadsparsed.hostname, which resolves them toevil.com— correctly rejected ✅ - No new top-level backend module, no new
os.getenv()✅ - Docs: requirements + architecture + 2 feature flows + user doc ✅
- Test adequacy: redirect-chain coverage, CDN-shaped spoof vectors, two-tier allowlist pins — non-happy-path, named to #1932 ✅
Design call worth recording: splitting SOURCE (credentialed hop 1, stays .twilio.com) from ALLOWED (unauthenticated redirect targets, adds .twiliocdn.com) widens only the gate that had the bug, so the tenant AuthToken's blast radius is unchanged. The redirect budget, the transport size cap, and dropping the exception body (the signed CDN URL is a ~4h bearer capability) are all correct.
Approving from trinity-ability — GitHub does not permit self-approval and the author is vybe.
dev has since taken #1913/#1937/#1947/#1949/#1899. Seven conflicts, resolved so that no side's change is lost: SOURCE - static_checks.py the one real semantic conflict. ent#128 (#1899) flipped the per-check swallow from _skip to _fail so a crashed check is counted by _counts; ent#89 kept _skip and added logging. Taking this branch's side verbatim would have silently reverted the HARD-count fix. Merged: _fail from #1899 + logger.error(exc_info=True) from ent#89, which is strictly more diagnostic than the logger.warning it replaces. The docstring directly above already asserts "a check that could not evaluate is not a check that passed". - template_service three hunks, all adjacent additions: both import blocks kept (template_schedules got its own statement — the two sides shared a closing paren), both new functions kept, and both pre-literal computations kept at each of the two call sites. - crud.py import list, both symbols kept. DOCS - architecture.md two hunks where BOTH sides had edited the same three bullets (template_service / fork_to_own / crud). Not a pick — each line was 3-way merged at word granularity against the merge base; no edit pairs overlapped, so both sides' text survives verbatim. dev's bullet order preserved, ent#89's new template_schedules.py bullet appended. - feature-flows.md all rows kept, table stays reverse-chronological. - learnings.md both sets of entries kept. - registry.json both entry lists kept. The conflict opened after a bare '{' and closed before a bare '}', so each side was an object BODY -- a naive concatenation produced invalid JSON. Re-added the '},{' separator. 109 -> 112 entries, none dropped. Verified: zero markers tree-wide, registry.json parses (112 entries), all three touched modules compile, and every deletion vs origin/dev is one of ent#89's own intended replacements (crud docstring three->four, the cron helper replaced by the shared validator, T-018 wired into the dispatch map).
Fixes #1932
What was broken
Twilio never serves inbound WhatsApp media from
api.twilio.comdirectly — it 302s every attachment to its media CDN (mms.twiliocdn.com). The SSRF allowlist only permitted*.twilio.com, sodownload_filerefused the redirect and returnedNone.Every inbound WhatsApp attachment has failed since #463 (2026-04-23) — roughly three months on a feature that looked shipped. The pre-existing
WARNINGfor the refusal surfaced nothing in that entire window (which is why it is nowERROR).The fix — a two-tier allowlist, deliberately narrower than the AC asked for
The AC said "widen the allowlist". Widening the single existing constant would also have widened the credentialed hop — the request that carries the tenant's
AccountSid:AuthTokenas HTTP Basic auth. Three independent reviewers (/autoplanstrategy,/autoplansecurity, and the Gemini second voice) called that ~3× wider than the bug. So the constant splits:_TWILIO_MEDIA_SOURCE_HOST_SUFFIXES.twilio.comMediaUrl{N}parse gate + credentialed hop 1_TWILIO_MEDIA_ALLOWED_HOST_SUFFIXES.twilio.com,.twiliocdn.comAC #2 is satisfied literally — the AC-named constant
_TWILIO_MEDIA_ALLOWED_HOST_SUFFIXESstill receives.twiliocdn.com. The split only means the credentialed blast radius stays exactly as narrow as it was before this PR. Verified narrow by grep: exactly three call sites —:310(parse) and:724(hop 1) use the narrow predicate; only:759(redirect target) uses the wide one.test_allowlist_constants_are_the_documented_tierspinsSOURCE ⊆ ALLOWED.Also in the diff, each a direct consequence of making this path reachable for the first time:
follow_redirects=Falsestays — every hop is re-validated before it is issued, nothing is blind-followed. Single-follow became a bounded budget (_MAX_MEDIA_REDIRECTS = 3) so a future Twilio 302→302 chain degrades to "still works" instead of silently reproducing this exact bug.16 MB) — Telegram (20 MB) and Slack (10 MB) both cap; WhatsApp's read was unbounded, and this fix makes that unbounded read reachable for the first time.type(e).__name__only. Somehttpxexceptions (UnsupportedProtocol,InvalidURL) embed the URL, and the signed CDN URL is a live ~4-hour bearer capability to the media.Two reasons, and the second one matters more:
(a) No live-sender verification was possible in this run. It needs a real Twilio account with HTTP Basic Authentication for media enabled, a publicly reachable webhook URL, and a provisioned WhatsApp sender.
(b) The AC's premise is partly wrong.
services/upload_service.py:45-49UNSUPPORTED_MIMESrejectsapplication/pdfandaudio/*for every channel, enforced channel-agnostically atupload_service.py:177. So of the AC's three attachment types, only the image can actually land as bytes.A PDF or a voice note will now be fetched successfully by this fix and then surface as "unsupported format" — that is a separate, deliberate policy gate, not a regression from this PR. Expanding scope into
UNSUPPORTED_MIMESwas explicitly rejected as out of fence.How a human should verify on a deployed instance:
Refusing off-domain media redirectwarning.*.amazonaws.comwas rejected — and it is worse than it looksAccounts without media auth enabled get redirected to
s3-external-1.amazonaws.com. That host is path-style (/<any-bucket>/<key>), so allowlisting it — even as a single exact hostname — admits arbitrary attacker-controlled content under an allowlisted name. That is a bypass primitive, not a widening.The supported configuration is therefore media-auth-enabled accounts. An account without it will still fail — with the
Refusing off-domain media redirect to host=s3-external-1.amazonaws.comlog line as the diagnostic, and a new operator-facing troubleshooting table in the user docs that names exactly this case and how to fix it in the Twilio Console.This is test-pinned, not prose:
test_s3_redirect_refused.Env override: NO — recorded decision (AC #4)
Rationale, recorded as a comment at the constant so it survives without this PR body:
EXTRA_CORS_ORIGINSis inbound CORS, not an outbound fetch gate;telegram_media.ALLOWED_DOWNLOAD_HOSTandurl_validation's github.com lock are both hard-coded;base_image_allowlistis an admin DB setting with an audit trail.)agent_service/helpers.py::validate_base_image) — never env.Anti-vacuity evidence (this is what makes the suite trustworthy)
download_filereturnsNoneon every failure path, so a negative test passes even against a completely unpatched seam. Every negative test therefore asserts the recorded call log / hop count, not justis None. Four deliberate breaks were introduced and all four were detected (tree restored clean after each):httpx.Headers→ plaindicttest_non_200_after_redirect_returns_noneandtest_redirect_budget_exhaustedwent red purely on hop count — captured output showsassert result is Nonepassing, thenassert len(calls) == 2→assert 1 == 2. Proof the suite is not carried by the all-paths-return-Nonetrap.auth=to the redirect hopA
TestInboundMediaHarnessIntegrityclass fails loudly if the DB seam or thehttpx.Headersfake ever regresses./csoCLEAN with adversarial evidence: a 37-vector corpus against the widened tier with aurlparse↔httpx.URLcross-check found no bypass and no exploitable differential (the single disagreement — a leading-space URL — is unreachable from a parsed HTTP header and fails closed). Multi-Locationjoin measured on realhttpx.Headers: attacker-first → denied; twiliocdn-first → httpx still resolveshost='mms.twiliocdn.com', so no attacker host is ever contacted.A second-voice claim was tested and refuted, twice, by two different workers: the alleged hop-1 cookie leak to the CDN does not occur — with a hop-1
Set-Cookie: Domain=.twilio.com, hop 2 carriesauthorization=Noneandcookie=None(httpx enforces RFC 6265 domain matching).AC #3 — the #463 spoof tests
The six #463 spoof tests are byte-identical to
origin/dev(verified by per-function diff — all six IDENTICAL), node-ids unchanged._is_twilio_media_url's name and signature are preserved._host_matches_suffixestakessuffixesas a required positional — no permissive default that a future call site could silently inherit.CDN-shaped spoof vectors added on top (
mms.twiliocdn.com@evil.com,mms.twiliocdn.com.evil.com, and the scheme/userinfo family).Accepted residuals — named so none of them reads as an oversight
_process_updateis a background task, not a request hold.upload_service's policy caps on purpose so the policy layer keeps producing its honest message; 16 MB is itself above Twilio's own 16 MB WhatsApp media ceiling, so this is the pathological case only._host_matches_suffixeskeeps the pre-existinghost == s.lstrip(".") or host.endswith(s)shape, so the widened tier also admits baretwiliocdn.com. It is the line the two-tier split rests on.Pre-merge operator check (2 minutes)
Confirmation, not a blocker — the bounded budget already covers a chain. Please run against a real account and paste the output:
Then follow the
Locationonce. This confirms the redirect target host and that hop 2 answers200.Corrections to other issues (recorded here as text; no comments were posted)
download_fileas a working prerequisite. Same correction.#1933 relationship
Unblocked — but only for media-auth-enabled accounts.
download_file's signature and itsNone-on-failure contract are byte-stable (verified by diff), so #1933 can build on it unchanged.NEW security finding — follow-up recommendation (deliberately not filed)
Slack's
download_filehas no SSRF allowlist at all.slack_service.py:739fetches an inbound-event URL withfollow_redirects=True(:754) — no allowlist, no scheme check. Same bug class as this issue, different channel.Deliberately not widened into this PR. The eventual shape is a shared
channel_media_fetch(url, allowlist, auth, max_bytes)covering WhatsApp / Telegram / Slack uniformly.Other follow-ups (listed, not filed)
UNSUPPORTED_MIMESresidual — if PDFs should reach agent workspaces, that is a deliberate policy change, not a bug fix.fastapi>=0.115.0is a floor, not a pin (see Verification below).Verification — stated honestly
/verify-localran with--skip-agent. This was forced by the host: the operator's live dev stack owns the globaltrinity-agent-network, so the full agent stage hard-refuses. This is a host-environment constraint, not a passing agent stage. The skipped stages (3:import agent_serverinside the built base image; 5: real agent/health) cover no surface this PR touches — the diff contains nodocker/base-image/file and no new backend module.WAVE-4 result:
The single red is proven pre-existing, not assumed.
tests/unit/test_1069_voip_call_path_param.py::TestVoipCallPathParam::test_flat_path_params_are_agent_name_not_namewas run from a pristinegit archive origin/dev@8e924526extracted into a temp tree, using the same verify venv via the harness's own invocation → identicalImportError: cannot import name 'get_flat_dependant'. The file is byte-identical across this branch anddev.Repo-level root cause:
fastapi>=0.115.0is a floor, not a pin, so any fresh venv now resolves 0.141.1, whereget_flat_dependantno longer exists. At the prod pinfastapi==0.115.6the file is4 passed. This will red for every contributor building a clean environment and deserves its own issue — it must not be attributed to this PR.Targeted suite, re-derived first-hand at ship time (explicit collected counts; no
-m unitanywhere — that flag silently deselects: measured65 collected / 42 deselected / 23 selected):Notes for the reviewer
transports/twilio_webhook.pydrift; the comment namesTwilioWebhookTransport.handle_webhookandRequestValidator(...).validate(...)instead. Please do not "restore" line numbers there.type(e).__name__log narrowing — the narrowed log is deliberately less informative, and the table is what makes it actionable..claude/agents/test-runner.mdcatalog sync is intentionally left uncommitted (it lives in the privatetrinity-devsubmodule) and needs a separate commit there. It is not part of this PR.docs/memory/learnings.mdnortests/registry.json, so it has no conflict with the sibling PRs in this batch and can merge in any order relative to them.🤖 Generated with Claude Code
Review round 2 (2026-08-03) — merge + reviewer follow-ups
Rebased onto
devand applied the reviewer's pre-merge item. Head is now33c28d64; PR is MERGEABLE again.Merged
origin/dev(c55ff804). One conflict, indocs/memory/feature-flows.md— both sides prepended a row to the Recent Updates table (#1932 here, #1931 ondev). Both rows kept, #1932 above #1931.whatsapp_adapter.pyand both test files were untouched by the merge (verified by emptygit diff HEAD -- <paths>), and the #1932architecture.mdline survived the auto-merge intact.F1 applied — both SOURCE-tier gates now log at ERROR (
33c28d64). The reviewer's framing is right and worth restating: #1932's root cause was never the allowlist, it was a fail-closed gate reporting a 100% outage at a level nobody reads. Raising only the redirect gate left the stricter half — the one the two-tier split newly made most likely to break on the next vendor change — guarded at WARNING.Not left as prose, because the level is a one-word revert with a three-month observed detection latency. New
TestSourceTierRejectionsAreVisiblepins both, and is mutation-proven: reverting eitherlogger.errorREDs the pair onassert {30} == {40}, and both go green on restore.Both stale doc claims fixed.
feature-flows/whatsapp-integration.md:17andrequirements/public-access.md:309described inbound media as "images/audio/PDFs";upload_service.UNSUPPORTED_MIMESrejectsapplication/pdf,audio/,video/and archives channel-agnostically, so only images (plus text/CSV/JSON) reach a workspace. Chasing that down is what surfaced the_maybe_transcribe_voiceerror corrected above.Vendor evidence folded into the user doc. Media auth has been mandatory on every main account created since 2023-07-31 (Twilio changelog, linked in the doc), so the S3 residual reaches only pre-July-2023 grandfathered accounts. The prerequisite now opens with "most accounts already have this and need no action" instead of "opt-in and off by default" — materially less alarming, and accurate.
Verification
test_whatsapp_inbound_media.py+test_whatsapp_adapter.py(-p no:randomly)test_whatsapp_outbound_media.pytests/unit(-m "not slow",-p no:randomly){30} != {40}; green on restoreThe 8
tests/unit/test_1771*.pyproperty files are excluded from that count — they fail collection withModuleNotFoundError: No module named 'hypothesis'on this host's interpreter (system Python 3.14, no venv). No1771file is touched by this branch. Notetest_1069_voip_call_path_param.pypassed in this run, so thefastapifloor-drift red reported in the original body is environment-dependent, not a property of the branch.Still open — both yours, neither needs a code change
— unsupported formatrather than— download failed, the redirect chain provably worked and bytes came back.