fix(twilio): isolate locks and channels per conversation, restrict media downloads to the API origin (#235)

[patrick-chinchill]

Summary

Twilio conversations sharing one bot number are now isolated from each other, and authenticated media downloads are restricted to the configured Twilio API origin. Security fix, and breaking for Twilio: channel_id values change.

What changed

• Per-conversation locks (twilio/adapter.py): lock_scope changes from "channel" to "thread". Before this, different recipients texting the same bot number serialized behind each other's handlers, and under the default drop strategy the later one raised LockError.
• Per-conversation channels (twilio/thread.py): twilio_channel_id(thread_id) validates through decode_twilio_thread_id and returns the thread id unchanged. channel_id_from_thread_id, fetch_thread().channel_id and thread.channel.id all move from twilio:{sender} to the full twilio:{sender}:{recipient} id. Because channel_id == thread_id, persist_thread_history no longer appends a second copy of each inbound message to a shared channel list.
• Media origin check (twilio/api.py): fetch_twilio_media(url, *, api_url=None, api_base_url=None, ...) compares (scheme, lowercased host, effective port) against api_url → api_base_url → DEFAULT_API_URL, using is not None fallbacks. On a mismatch it raises TwilioApiError("Twilio media URL must match the configured Twilio API origin", status=0) before any credential is resolved or request sent. The adapter's downloader passes api_url=self._api_url, so every attachment download, both fresh webhook MediaUrlN and rehydrated, is constrained to that origin (upstream index.ts passes apiUrl for both as well).
• Layering: the existing Python-only Twilio host allowlist (_is_trusted_twilio_media_url) stays in front as defence in depth, as the issue recommended. Both checks must pass before Basic auth is sent.
• TwilioAdapterConfig.api_url comment notes that it is also the trusted media origin (upstream made the same docs change).

Upstream mapping

Upstream	Release	Ported as
28bc7768 fix(twilio): isolate message locks by conversation (vercel/chat#849)	chat@4.39.0	lock_scope = "thread" + concurrency regression test
b7c9316b fix(chat): tighten conversation boundaries (vercel/chat#875), Twilio part only	chat@4.40.0	twilio_channel_id returns the thread id. The rest of the commit is #195 / #194 / #229 / #230
d8103a10 fix(twilio): restrict authenticated media downloads (vercel/chat#831)	chat@4.38.1	origin check in fetch_twilio_media + adapter wiring

Tests ported

Twilio test files are not fidelity-mapped, so these were ported by hand:

• packages/adapter-twilio/src/index.test.ts

  • "uses the full DM thread id as its channel id" → TestThreadIds::test_uses_the_full_dm_thread_id_as_its_channel_id. The old channel assertion was removed from test_encodes_and_decodes_phone_and_channel_address_thread_ids.

  • "isolates concurrent recipients with thread-scoped locks" → TestThreadIds::test_isolates_concurrent_recipients_with_thread_scoped_locks. It uses a real Chat with create_mock_state(), the direct port of upstream createMockState. The first handler is held on an asyncio.Event with no sleeps. The test asserts:

    • the second recipient completes while the first is still held;
    • both thread ids were passed to acquire_lock;
    • the handler ran twice.

    A Python-specific addition asserts that persisted history is per conversation and that no shared twilio:{sender} channel list is written.

  • "rejects rehydrated media from an untrusted origin" → the existing test_media_downloader_refuses_untrusted_hosts was renamed to TestRehydrateAttachment::test_rejects_rehydrated_media_from_an_untrusted_origin and parametrized to cover both layers. A non-Twilio host is refused by the allowlist (ValidationError). A Twilio-owned host that is not the API origin is refused by the origin check (TwilioApiError).

• packages/adapter-twilio/src/api/index.test.ts
  • "fetches media from the configured regional api origin" → TestFetchTwilioMedia::test_fetches_media_from_the_configured_regional_api_origin
  • "rejects media outside the configured api origin" → TestFetchTwilioMedia::test_rejects_media_outside_the_configured_api_origin. It covers upstream's 3 cases, uses AsyncMock credential resolvers and transport, and asserts that none of them was awaited and that status == 0.
• Python-specific tests:
  • adapter api_url wiring: a regional origin is accepted and api.twilio.com is refused;
  • origin normalization and precedence: a default :443, a mixed-case host, and api_url over api_base_url;
  • malformed and ambiguous inputs;
  • channel_id_from_thread_id still rejects malformed ids;
  • updated pins: lock_scope == "thread" and fetch_thread().channel_id.

A mutation check against the pre-change source shows that the new tests fail on the old behavior. The second recipient raises LockError, and the channel/api_url/metadata assertions fail.

Verify first: redirect credential forwarding

api.twilio.com media answers with a redirect to a pre-signed CDN URL. The adapter's shared-session aiohttp transport follows it. aiohttp drops the Authorization header when the redirect changes origin, so Basic auth is not forwarded to the CDN. I checked client.py in aiohttp 3.9.0 (our >=3.9 floor) and in the locked 3.14.3. This PR only documents the finding (in docs/UPSTREAM_SYNC.md); redirect and byte-cap policy belongs to #204.

Divergences

No new rows in the non-parity table.

• Retained: the existing Twilio suffix allowlist, now layered in front of the upstream origin check. The rehydrate_attachment allowlist row is updated to say so. With the origin check in place, the allowlist only adds a refusal when api_url is a non-Twilio host. *.twiliocdn.com can no longer be reached, because it is never the API origin.
• Python-only strictness inside the origin check: URLs that contain whitespace, ASCII control characters or a backslash are refused outright. WHATWG strips or rewrites those characters and urlsplit does not match it exactly, so failing closed avoids a parser differential. The row records this.
• Parity: the channel-id and lock-scope change match 4.40. docs/UPSTREAM_SYNC.md has a new "Twilio conversation boundaries" subsection.

Consumer impact

• Twilio (breaking): channel_id / thread.channel.id now equal the thread id. Channel-scoped state and channel-history keys written under the old twilio:{sender} id are no longer read (they are orphaned, as upstream intends). Thread ids and thread history are unchanged, and channel_name is still the sender number. Concurrent conversations stop blocking or dropping each other.
• Twilio media (breaking for custom api_url only): every media URL, inbound webhook media included, must be on the configured api_url origin (default https://api.twilio.com). A regional api_url refuses inbound media on api.twilio.com, as upstream does. With a non-Twilio or http api_url (a proxy or mock), no media URL passes both layers, so attachment downloads always raise. The default config is unaffected.
• Slack / Teams: none.

Validation

ruff check ✅ · ruff format --check ✅ · audit_test_quality.py ✅ (0 hard failures) · verify_test_fidelity.py --strict @ chat@4.31.0 ✅ 732/732 · pytest ✅ 5168 passed, 13 skipped (baseline 5147).

Non-strict fidelity against chat@4.41.1: 705/823 matched, 118 missing. This PR doesn't change that, because the Twilio test files aren't mapped.

Environment note: uv.lock is gitignored, and a fresh uv sync now resolves microsoft-teams-* 2.1.0. That version removed App.activity_sender, which makes 3 Teams tests fail. The failures are unrelated to this PR and appear on main with a fresh resolve. The run above used the baseline microsoft-teams-*==2.0.13.4. Any CI run that resolves the Teams SDK fresh may hit the same failures; that needs a separate fix (a pin or a compat shim).

Closes #235
Part of #184

Review

Fixed in c37de73:

• Untested media_origin is None guard (api.py, minor). Confirmed: removing the guard passed every test. Added both-sides-opaque cases to test_media_origin_rejects_mismatched_and_malformed_urls: ("/2010-04-01/media/photo", "api.dublin.ie1.twilio.com"), ("", "") and ("/m", "/base"). The mutant now fails 3 tests.
• Surviving _url_origin mutants (tests/test_twilio_api.py, minor). Confirmed both. Added http://api.twilio.com:443/m, so dropping the scheme from the origin tuple now fails 1 test. The host-less pairs above cover the deleted if not scheme or not host guard, which now fails 3 tests.
• CHANGELOG understated the media-origin change (CHANGELOG.md, minor). Confirmed. Webhook MediaUrlN goes through the same _make_media_downloader, so the check covers fresh inbound media as well as rehydrated media (parity with upstream). The CHANGELOG entry now says this, is marked breaking (Twilio, custom api_url only), and spells out the proxy/http api_url dead end. The UPSTREAM_SYNC row and the Consumer impact section above now say the same. The new TestRehydrateAttachment::test_non_twilio_api_url_refuses_media_on_every_origin pins the dead end.

Declined: none.

Validation after the fixes: ruff, format, audit (0 hard failures) and fidelity --strict (732/732) all pass. pytest gives 5171 passed and 3 failed. All 3 failures are the Teams SDK 2.1.0 failures from the environment note above, and they also make CI test (3.12) / test (3.13) fail. They are outside this PR.

Merge gate

Final HEAD: b53f420. It merges origin/main, which brings in #251 (Teams SDK <2.1). The only conflict was in CHANGELOG.md, and it was resolved by keeping one ## Unreleased (4.41 wave) heading with the bullets from both sides. The 3 Teams failures from the environment note above no longer occur: #251 caps the SDK at 2.0.16.

gpt-6-astra review: 2 rounds.

• Round 1 (dd6525e, the merge commit): "No actionable regressions found. Twilio and related chat, channel, thread, and history tests passed (570 passed, 3 skipped); targeted lint, formatting, type checking, and test-quality checks also passed."
• Round 2 (b53f420, final HEAD): "No actionable regressions found. All 150 Twilio tests and 428 related core tests passed, with 3 skipped; targeted lint and formatting checks also passed."

Fixed in b53f420. These are comment-only changes that I found in self-review, not reviewer findings. Behavior is unchanged.

• TwilioAdapterConfig.api_url comment (types.py): it said only rehydrated media is origin-checked. Fresh webhook MediaUrlN goes through the same downloader, so the comment now names both.
• _TRUSTED_MEDIA_HOSTS comment (adapter.py): it said "upstream fetches blindly", which stopped being true with d8103a10. It now describes the allowlist as a Python-only layer in front of upstream's exact-origin check.

Rebutted: none.

Bot comments: CodeRabbit posted only its "Review limit reached" rate-limit notice, with no review content. gemini-code-assist did not post. There were no inline review comments and no reviews to address.

CI on b53f420: all green. That covers Lint & Type Check, test (3.12), test (3.13), CodeQL, Analyze (python) and Analyze (actions). The CodeRabbit status is "pass (rate limited)".

Local validation on b53f420: ruff check ✅ · ruff format --check ✅ · audit_test_quality.py ✅ (0 hard failures) · verify_test_fidelity.py --strict @ chat@4.31.0 ✅ · pytest ✅ 5174 passed, 13 skipped (Teams SDK 2.0.16).

[coderabbitai]

Warning

Review limit reached

Next included review available in 16 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 66712749-083f-40d0-aa54-c02e8d7327eb

📥 Commits

Reviewing files that changed from the base of the PR and between 8f959df and b53f420.

📒 Files selected for processing (8)

• CHANGELOG.md
• docs/UPSTREAM_SYNC.md
• src/chat_sdk/adapters/twilio/adapter.py
• src/chat_sdk/adapters/twilio/api.py
• src/chat_sdk/adapters/twilio/thread.py
• src/chat_sdk/adapters/twilio/types.py
• tests/test_twilio_adapter.py
• tests/test_twilio_api.py

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[patrick-chinchill]

Merge gate: CI green on b53f420 (Lint & Type Check, test (3.12), test (3.13), CodeQL, Analyze (python), Analyze (actions); CodeRabbit pass/rate-limited); origin/main (8f959df, #251) is already an ancestor of HEAD, so no further merge was needed. Local re-validation on b53f420: ruff, format, audit (0 hard failures), fidelity --strict @ chat@4.31.0, pytest 5174 passed / 13 skipped. Local Codex review (gpt-6-astra, xhigh, --base origin/main) on b53f420: PASS: "No actionable regressions found. All 150 Twilio tests and 428 related core tests passed, with 3 skipped; targeted lint and formatting checks also passed." 2 astra rounds (both clean). Bot reviews: CodeRabbit posted only its rate-limit notice; no human or inline reviews outstanding. Merging with --admin (Protect Main requires a code-owner approval).