Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -130,21 +130,31 @@ describe('sendMobilePush action', () => {

it('should send notification for custom hostname', async () => {
const externalId = defaultExternalId
const notifyReqUrl = `https://my-api.com/v1/Services/${pushServiceSid}/Notifications`
const customHostname = 'push.custom.twilio.com'
const notifyReqUrl = `https://${customHostname}/v1/Services/${pushServiceSid}/Notifications`
const notifyReqBody = getDefaultExpectedNotifyApiReq(externalId)

nock(`https://content.twilio.com`).get(`/v1/Content/${contentSid}`).reply(200, defaultTemplate)
nock(notifyReqUrl).post('', notifyReqBody.toString()).reply(201, externalId)

const responses = await testAction({
settingsOverrides: {
twilioHostname: 'my-api.com'
twilioHostname: customHostname
}
})
expect(responses[1].url).toStrictEqual(notifyReqUrl)
expect(responses[1].status).toEqual(201)
expect(responses[1].data).toMatchObject(externalId)
})

it('should reject a non-Twilio custom hostname without sending credentials', async () => {
nock(`https://content.twilio.com`).get(`/v1/Content/${contentSid}`).reply(200, defaultTemplate)

// No nock interceptor for the notify request: it must never be made, so credentials are never forwarded.
await expect(testAction({ settingsOverrides: { twilioHostname: 'my-api.com' } })).rejects.toThrow(
/Invalid Twilio hostname/
)
})
Comment thread
harsh-joshi99 marked this conversation as resolved.
})

describe('error handling', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -391,7 +391,7 @@ describe.each(['stage', 'production'])('%s environment', (environment) => {
})
})

const twilioHostname = 'api.nottwilio.com'
const twilioHostname = 'api.custom.twilio.com'

const twilioRequest = nock(`https://${twilioHostname}/2010-04-01/Accounts/a`)
.post('/Messages.json', expectedTwilioRequest.toString())
Expand All @@ -409,6 +409,20 @@ describe.each(['stage', 'production'])('%s environment', (environment) => {
expect(twilioRequest.isDone()).toEqual(true)
})

it('should reject a non-Twilio custom hostname without sending credentials', async () => {
const twilioHostname = 'api.nottwilio.com'

// No nock interceptor: the request must never be made, so credentials are never forwarded.
await expect(
testAction({
settingsOverrides: { twilioHostname },
mappingOverrides: {
externalIds: [{ type: 'phone', id: '+1 (505) 555-4555', subscriptionStatus: true, channelType: 'sms' }]
}
})
).rejects.toThrow(/Invalid Twilio hostname/)
})

it('should send SMS with custom metadata', async () => {
const expectedTwilioRequest = new URLSearchParams({
Body: 'Hello world, jane!',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,7 @@ describe.each(['stage', 'production'])('%s environment', (environment) => {
Tags: defaultTags
})

const twilioHostname = 'api.nottwilio.com'
const twilioHostname = 'api.custom.twilio.com'

const twilioRequest = nock(`https://${twilioHostname}/2010-04-01/Accounts/a`)
.post('/Messages.json', expectedTwilioRequest.toString())
Expand All @@ -262,6 +262,13 @@ describe.each(['stage', 'production'])('%s environment', (environment) => {
expect(twilioRequest.isDone()).toEqual(true)
})

it('should reject a non-Twilio custom hostname without sending credentials', async () => {
// No nock interceptor: the request must never be made, so credentials are never forwarded.
await expect(testAction({ settingsOverrides: { twilioHostname: 'api.nottwilio.com' } })).rejects.toThrow(
/Invalid Twilio hostname/
)
})

it('should send WhatsApp with custom metadata', async () => {
const expectedTwilioRequest = new URLSearchParams({
ContentSid: defaultTemplateSid,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import type { Settings } from './generated-types'
import { actionDefinition as sendSms } from './sendSms'
import { actionDefinition as sendWhatsApp } from './sendWhatsApp'
import { actionDefinition as sendMobilePush } from './sendMobilePush'
import { validateTwilioHostname } from './utils'

const getRange = (val: number): { value: number; label: string }[] => {
return Array(val)
Expand Down Expand Up @@ -167,7 +168,7 @@ export const destinationDefinition: DestinationDefinition<Settings> = {
}
},
testAuthentication: (request, options) => {
const hostName = options.settings.twilioHostname ?? 'api.twilio.com'
const hostName = validateTwilioHostname(options.settings.twilioHostname ?? 'api.twilio.com')
return request(`https://${hostName}/2010-04-01`)
}
},
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/* eslint-disable @typescript-eslint/no-explicit-any */
/* eslint-disable @typescript-eslint/no-unsafe-call */
import { TwilioMessageSender } from '../utils'
import { TwilioMessageSender, validateTwilioHostname } from '../utils'
import { ExtId, track } from '@segment/actions-shared'
import type { Payload as PushPayload } from './generated-types'
import { ContentTemplateTypes } from '../utils/types'
Expand Down Expand Up @@ -31,7 +31,9 @@ export class PushSender extends TwilioMessageSender<PushPayload> {
private DEFAULT_HOSTNAME = 'push.ashburn.us1.twilio.com'

get twilioHostname() {
return this.settings.twilioHostname?.length ? this.settings.twilioHostname : this.DEFAULT_HOSTNAME
return validateTwilioHostname(
this.settings.twilioHostname?.length ? this.settings.twilioHostname : this.DEFAULT_HOSTNAME
)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
get twilioToken() {
//TODO cache this (lazy load)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/* eslint-disable @typescript-eslint/no-non-null-assertion */
/* eslint-disable @typescript-eslint/no-unsafe-call */
import { TwilioMessageSender, TwilioPayloadBase } from './TwilioMessageSender'
import { TwilioMessageSender, TwilioPayloadBase, validateTwilioHostname } from './TwilioMessageSender'
import { OperationDecorator, TrackedError, OperationContext, ExtId } from '@segment/actions-shared'

/**
Expand All @@ -14,7 +14,7 @@ export abstract class PhoneMessageSender<Payload extends PhoneMessagePayload> ex
abstract getBody(phone: string): Promise<URLSearchParams>

get twilioHostname() {
return this.settings.twilioHostname ?? this.DEFAULT_HOSTNAME
return validateTwilioHostname(this.settings.twilioHostname ?? this.DEFAULT_HOSTNAME)
}
get twilioToken() {
return Buffer.from(`${this.settings.twilioApiKeySID}:${this.settings.twilioApiKeySecret}`).toString('base64')
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,24 @@ import { track, MessageSendPerformer, MessagePayloadBase } from '@segment/action

const Liquid = new LiquidJs()

// Only hostnames within Twilio's domain may receive requests, which carry the
// account's Basic-auth API key. The `twilioHostname` setting is customer-editable,
// so without this allowlist an arbitrary host could be used to exfiltrate the
// credentials (SECOPS-25241). Disallowing '@', '/' and ':' also blocks userinfo
// and path tricks such as "api.twilio.com@attacker.com".
const TWILIO_HOSTNAME_REGEX = /^([a-z0-9-]+\.)+twilio\.com$/
Comment thread
harsh-joshi99 marked this conversation as resolved.

export function validateTwilioHostname(hostname: string): string {
// Hostnames are case-insensitive, so normalize before matching to avoid
// rejecting valid hosts like "Api.Twilio.Com".
if (!TWILIO_HOSTNAME_REGEX.test(hostname.toLowerCase())) {
throw new PayloadValidationError(
`Invalid Twilio hostname: "${hostname}". Hostname must be within the twilio.com domain.`
)
Comment on lines +22 to +24
}
return hostname
}

export interface TwilioPayloadBase extends MessagePayloadBase {
contentSid?: string
}
Expand Down
Loading