[engage-messaging-twilio] Validate twilioHostname against Twilio domain (SECOPS-25241)

[harsh-joshi99]

Summary

Fixes a credential-forwarding vulnerability (HackerOne AD-003 / SECOPS-25241 / STRATCONN-6910) in the Engage Messaging Twilio destination.

The customer-editable twilioHostname setting was interpolated, unvalidated, into request URLs that carry the Twilio API key via Authorization: Basic base64(twilioApiKeySID:twilioApiKeySecret). Setting it to a non-Twilio host causes Segment to forward the account's Twilio API key to that host.

Fix

Adds validateTwilioHostname() which allowlists hostnames under *.twilio.com (case-insensitive) and throws a PayloadValidationError before any credentialed request is made. The character class also blocks userinfo/path bypasses such as api.twilio.com@attacker.com.

Applied to all three credentialed call sites:

• utils/PhoneMessageSender.ts — SMS + WhatsApp (via inheritance)
• sendMobilePush/PushSender.ts — mobile push
• index.ts — testAuthentication

This matches the researcher's stated control: "The production Twilio API host should remain api.twilio.com or a validated Twilio-owned API host." All hosts this destination talks to today (api.twilio.com, content.twilio.com, push.ashburn.us1.twilio.com, regional api.<region>.twilio.com) pass the allowlist.

Testing

• Updated the existing "custom hostname" tests to use valid Twilio subdomains.
• Added rejection tests to send-sms / send-whatsapp / send-mobile-push asserting a PayloadValidationError is thrown and no outbound request is made for a non-Twilio host.
• 197 tests pass across the three suites.

End-to-end verification with the researcher's PoC

Ran the researcher's PoC harness (ad_003_engage_messaging_twilio_hostname_credential_forwarding) against the real Segment Actions runtime on both main and this branch. It uses nock.disableNetConnect() and synthetic credentials — no real Twilio/network access — setting twilioHostname to the attacker host twilio-credential-capture.example and invoking sendSms.

On main (unpatched) — credential leaked to attacker host:

Captured Twilio credential material at attacker-controlled host:
- request: https://twilio-credential-capture.example/2010-04-01/Accounts/AC00000000000000000000000000000000/Messages.json
- authorization: Basic U0tfQVRUQUNLRVJfVklTSUJMRV9LRVlfU0lEOlRXSUxJT19BUElfS0VZX1NFQ1JFVA==
- decoded: SK_ATTACKER_VISIBLE_KEY_SID:TWILIO_API_KEY_SECRET

{
  "success": true,
  "finding": "AD-003",
  "mode": "real-segment-actions-runtime",
  "primary_proof": "artifacts/control_comparison.txt",
  "captured_request": "https://twilio-credential-capture.example/2010-04-01/Accounts/AC00000000000000000000000000000000/Messages.json",
  "decoded_credential_material": "SK_ATTACKER_VISIBLE_KEY_SID:TWILIO_API_KEY_SECRET"
}

On this branch (fixed) — blocked before any request, no credential sent:

PayloadValidationError: Invalid Twilio hostname: "twilio-credential-capture.example". Hostname must be within the twilio.com domain.
    at validateTwilioHostname (.../engage-messaging-twilio/utils/TwilioMessageSender.ts:20:11)
    at SmsMessageSender.get twilioHostname [as twilioHostname] (.../engage-messaging-twilio/utils/PhoneMessageSender.ts:17:34)
    at SmsMessageSender.sendToRecepient (.../engage-messaging-twilio/utils/PhoneMessageSender.ts:64:23)

{
  "success": false,
  "finding": "AD-003",
  "error": "Invalid Twilio hostname: \"twilio-credential-capture.example\". Hostname must be within the twilio.com domain."
}

Rollout note

This enforces immediately (throws). The only regression vector is a live config using a non-twilio.com host that works today; every host in the current code path is under twilio.com. Flagging for reviewer awareness in case a measure-first / feature-flagged rollout is preferred for this high-volume destination.

🤖 Generated with Claude Code

[Copilot]

Pull request overview

Note

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

This PR mitigates a credential-forwarding vulnerability in the Engage Messaging Twilio destination by validating the customer-configurable twilioHostname against an allowlist of *.twilio.com before any credentialed requests are sent.

Changes:

• Added validateTwilioHostname() backed by a Twilio-domain allowlist regex.
• Applied hostname validation at all credentialed request call sites (SMS/WhatsApp sender, mobile push sender, and testAuthentication).
• Updated existing “custom hostname” tests to use valid Twilio subdomains and added rejection tests for non-Twilio hostnames.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
packages/destination-actions/src/destinations/engage-messaging-twilio/utils/TwilioMessageSender.ts	Introduces Twilio hostname allowlist validation and reusable helper.
packages/destination-actions/src/destinations/engage-messaging-twilio/utils/PhoneMessageSender.ts	Ensures SMS/WhatsApp hostname is validated before being used in requests.
packages/destination-actions/src/destinations/engage-messaging-twilio/sendMobilePush/PushSender.ts	Ensures mobile push hostname is validated before being used in requests.
packages/destination-actions/src/destinations/engage-messaging-twilio/index.ts	Validates hostname during testAuthentication to prevent credential forwarding.
packages/destination-actions/src/destinations/engage-messaging-twilio/tests/send-whatsapp.test.ts	Updates custom-host test data and adds rejection coverage for non-Twilio hostnames.
packages/destination-actions/src/destinations/engage-messaging-twilio/tests/send-sms.test.ts	Updates custom-host test data and adds rejection coverage for non-Twilio hostnames.
packages/destination-actions/src/destinations/engage-messaging-twilio/tests/send-mobile-push.test.ts	Updates custom-host test data and adds rejection coverage for non-Twilio hostnames.

[harsh-joshi99]

Note

Self-Review
Posted by PR author (harsh-joshi99) — self-reviews cannot apply the APPROVED label. Intended verdict: Manual Review Needed ⚠️. Ask a teammate to run plugin-review for the APPROVED label.

Code Review Personae Verdict: Manual Review Needed ⚠️

Three independent bug-finding passes and a policy scan found no critical, high, or medium issues. The credential-forwarding fix is complete: all three credentialed call sites route through the validated hostname getter, the regex resists the enumerated bypass vectors (userinfo/path/port/newline/suffix/uppercase), and the tests genuinely prove the security property (nock.disableNetConnect makes 'no interceptor' a hard guarantee).

One low-severity consistency nit: empty-string twilioHostname is handled differently across the three sites.

---

📝 Deep Code Review (3 Passes) — 0 Critical, 0 High, 0 Medium, 1 Low 🔵

Three independent bug-finder passes converged on the same conclusion: the fix is correct and not bypassable.

Consensus verifications (agreed by multiple passes):

• Coverage complete — the only two request sites interpolating the customer-controlled twilioHostname (PhoneMessageSender.ts:64 SMS/WhatsApp, PushSender.ts:90 push) both route through the validateTwilioHostname-guarded getter; testAuthentication (index.ts:171) validates inline. content.twilio.com (TwilioMessageSender.ts:93) and the profile-API host are hardcoded/framework-derived and correctly need no guarding. WhatsApp inherits the validated getter from PhoneMessageSender. grep confirmed no other settings.twilioHostname read bypasses the getter.
• Regex robust — /^([a-z0-9-]+\.)+twilio\.com$/ on .toLowerCase(), empirically tested in node by 2 passes: rejects userinfo (api.twilio.com@evil.com), port, path, suffix (api.twilio.com.evil.com), prefix (eviltwilio.com), bare twilio.com, and all newline-injection cases (JS $ without m flag anchors to absolute end-of-string, so api.twilio.com\n@evil.com is rejected).
• No NPE — all three call sites pass a guaranteed string (?? DEFAULT / ?.length ? : DEFAULT) to .toLowerCase().
• Throwing getter is safe — PayloadValidationError (non-retryable, correct class for a config error) is thrown only lazily inside the async perform lifecycle; not read in constructors/beforePerform/stats, so it is not swallowed and does not crash the process.
• No secret leakage — beforePerform logs only contentSid and twilioApiKeySID (the SID, not the secret); the error echoes the customer hostname, never the key.
• Tests prove the property — nock.disableNetConnect() (setup-after-env) makes any attempted external send throw NetConnectNotAllowed (which does not match /Invalid Twilio hostname/), so a regressed send fails the test loudly rather than passing silently.

Findings

• 🔵 LOW: Empty-string twilioHostname handled inconsistently across the three sites (packages/destination-actions/src/destinations/engage-messaging-twilio/sendMobilePush/PushSender.ts:35)
  [Found by 2/3 passes] PushSender uses twilioHostname?.length ? twilioHostname : DEFAULT (empty string falls back to the default and succeeds), while PhoneMessageSender (line 17) and index.ts testAuthentication use twilioHostname ?? DEFAULT (empty string is not nullish, so '' reaches validateTwilioHostname and throws Invalid Twilio hostname: ""). Not a new crash (pre-change an empty host produced a malformed https:///... URL that also failed) and not exploitable (an empty host cannot exfiltrate credentials), but the divergence is a latent inconsistency. Decision on this PR: left as-is to keep the security patch minimal.
  Suggestion: If normalizing later: resolve the host uniformly across all three sites, e.g. validateTwilioHostname(settings.twilioHostname?.length ? settings.twilioHostname : DEFAULT), so empty/whitespace consistently falls back to the safe default.

🛡️ Policy Compliance Review — No violations (3 policies evaluated)

Evaluated 3 policies (POL-001-port-forward-to-prod-db, POL-002-prod-db-write-from-laptop, POL-003-credentials-in-pr) across all review surfaces (PR body, commit messages, per-file code surfaces). No violations detected.

_{Generated by code-review-personae v0.8.15 | claude-opus-4-8 | deep code review (3 passes), policy review}

[harsh-joshi99]

Note

🔵 LOW: Empty-string twilioHostname handled inconsistently across the three sites

[Found by 2/3 passes] PushSender uses twilioHostname?.length ? twilioHostname : DEFAULT (empty string falls back to the default and succeeds), while PhoneMessageSender (line 17) and index.ts testAuthentication use twilioHostname ?? DEFAULT (empty string is not nullish, so '' reaches validateTwilioHostname and throws Invalid Twilio hostname: ""). Not a new crash (pre-change an empty host produced a malformed https:///... URL that also failed) and not exploitable (an empty host cannot exfiltrate credentials), but the divergence is a latent inconsistency. Decision on this PR: left as-is to keep the security patch minimal.

Suggestion: If normalizing later: resolve the host uniformly across all three sites, e.g. validateTwilioHostname(settings.twilioHostname?.length ? settings.twilioHostname : DEFAULT), so empty/whitespace consistently falls back to the safe default.