Skip to content

[FUNK-2000] Fixed extensible webhook shared secret issue #3316

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

[FUNK-2000] Fixed extensible webhook shared secret issue #3316

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8781252
fixed extensible webhook shared secret issue
vaibhav-nanda Sep 25, 2025
d0e3291
updated test
vaibhav-nanda Sep 26, 2025
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
94 changes: 94 additions & 0 deletions packages/destination-actions/src/destinations/webhook-extensible/__test__/webhook.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
DestinationDefinition
} from '@segment/actions-core'
import Webhook from '../index'
import { createHmac, timingSafeEqual } from 'crypto'

const settings = {
oauth: {},
Expand Down Expand Up @@ -261,6 +262,99 @@ export const baseWebhookTests = (def: DestinationDefinition<any>) => {
})
).rejects.toThrow(PayloadValidationError)
})

it('supports request signing for no auth', async () => {
const url = 'https://example.com'
const event = createTestEvent({
properties: { cool: true }
})
const payload = JSON.stringify(event.properties)
const sharedSecret = 'sharedSecret123'

nock(url)
.post('/', payload)
.reply(async function (_uri, body) {
// Normally you should use the raw body but nock automatically
// deserializes it (and doesn't allow us to access the raw request
// body) so we re-serialize the body here so that we can demonstrate
// signture validation.
const bodyString = JSON.stringify(body)

// Validate the signature
const expectSignature = this.req.headers['x-signature'][0]
const actualSignature = createHmac('sha1', sharedSecret).update(bodyString).digest('hex')

// Use constant-time comparison to avoid timing attacks
if (
expectSignature.length !== actualSignature.length ||
!timingSafeEqual(Buffer.from(actualSignature, 'hex'), Buffer.from(expectSignature, 'hex'))
) {
return [400, 'Invalid signature']
}

return [200, 'OK']
})

const responses = await testDestination.testAction('send', {
event,
mapping: {
url,
data: { '@path': '$.properties' }
},
settings: { sharedSecret, ...noAuthSettings },
useDefaultMappings: true
})

expect(responses.length).toBe(1)
expect(responses[0].status).toBe(200)
})

it('supports request signing for bearer token', async () => {
const url = 'https://example.com'
const event = createTestEvent({
properties: { cool: true }
})
const payload = JSON.stringify(event.properties)
const sharedSecret = 'sharedSecret123'

nock(url)
.post('/', payload)
.matchHeader('authorization', 'Bearer BearerToken1')
.reply(async function (_uri, body) {
// Normally you should use the raw body but nock automatically
// deserializes it (and doesn't allow us to access the raw request
// body) so we re-serialize the body here so that we can demonstrate
// signture validation.
const bodyString = JSON.stringify(body)

// Validate the signature
const expectSignature = this.req.headers['x-signature'][0]
const actualSignature = createHmac('sha1', sharedSecret).update(bodyString).digest('hex')

// Use constant-time comparison to avoid timing attacks
if (
expectSignature.length !== actualSignature.length ||
!timingSafeEqual(Buffer.from(actualSignature, 'hex'), Buffer.from(expectSignature, 'hex'))
) {
return [400, 'Invalid signature']
}

return [200, 'OK']
})

const responses = await testDestination.testAction('send', {
event,
mapping: {
url,
data: { '@path': '$.properties' }
},
settings: { sharedSecret, ...bearerTypeSettings },
useDefaultMappings: true
})

expect(responses.length).toBe(1)
expect(responses[0].status).toBe(200)
})
})

describe('refreshAccessToken', () => {
Expand Down
20 changes: 17 additions & 3 deletions packages/destination-actions/src/destinations/webhook-extensible/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import type { DestinationDefinition } from '@segment/actions-core'
import type { Settings } from './generated-types'
import { sendRefreshTokenReq } from './auth-utils'
import { createHmac } from 'crypto'

import send from './send'

Expand All @@ -26,12 +27,24 @@ const destination: DestinationDefinition<SettingsWithDynamicAuth> = {
return res
}
},
extendRequest: ({ settings, auth }) => {
extendRequest: ({ settings, auth, payload }) => {
const { dynamicAuthSettings } = settings
let xSignatureHeader

if (payload) {
const payloadData = payload.length ? payload[0]['data'] : payload['data']
if (settings.sharedSecret && payloadData) {
const digest = createHmac('sha1', settings.sharedSecret)
.update(JSON.stringify(payloadData), 'utf8')
.digest('hex')
xSignatureHeader = { 'X-Signature': digest }
}
}
Comment on lines +34 to +42
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @vaibhav-nanda I have a few questions about this:

  1. Do we need the xSignatureHeader to always be present if the customer provides a sharedSecret?

  2. How do you know that you are generating the xSignatureHeader correctly?

  3. This Action implements the performBatch() function. If we send a batch of events then I assume the xSignatureHeader needs to created based off of all the data being sent - is that correct?

  4. Shouldn't the xSignatureHeader value be generated from the actual payload being sent to the destination platform, rather than the input payload values? I can see that the data object gets encoded before it gets sent. if (data) return encodeBody(data, contentType)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @joe-ayoub-segment

  1. Yes, the x-signature header is expected to be present always, if the customer has provided a sharedSecret.

  2. This was confirmed by verifying the header being sent on the destination. The header received on destination was matched with the value calculated using the code on Line 37, and it was the same. This was also confirmed by sending the same request on Actions Webhook destination (it has the same functionality for x-signature header) and it also had the same header as the extensible webhooks.

  3. As per the current implementation in Actions Webhook destination, only the first event in the payload is being used to generate the xSignatureHeader. The same is being followed over here as well.

  4. It has been confirmed by comparing the xSignatureHeader values from Actions webhook and Extensible Webhook destinations. Both are sending out the same headers for the same request to both of them. The xSignatureHeader value is same for both the scenarios.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @vaibhav-nanda,

Regarding 1. Makes sense.

Regarding 2. Makes sense.

Regarding 3 and 4: This doesn't make sense to me. Shouldn't the entire body be used? How will the receiving side know how to check which part of the body was used when it's checking if the request is valid? Because of this I think we should ask @varadarajan-tw to take a look. I'm not qualified ;)!

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like for webhook action destination, we call out the signature computation logic in the docs.
https://segment.com/docs/connections/destinations/catalog/actions-webhook/#shared-secret-with-batching. The customer should be able to validate by parsing the body, computing signature and validating it against the the header sent by Segment.

However, the general practice is to compute it for the entire request body. I don't see a standard at Segment but Twilio's signature signing standard seems to be a good one to follow. We can implement this and call this out in our docs so that customers know how to validate.

My only concern with batch request signing is performance when dealing with large payloads. If it doesn't add too much overhead for a batch of 1000 requests, we should consider signing the entire payload.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @varadarajan-tw - makes sense.


let accessToken
let tokenPrefix = 'Bearer'
if (dynamicAuthSettings?.oauth?.type === 'noAuth') {
return {}
return xSignatureHeader ? { headers: xSignatureHeader } : {}
}
if (dynamicAuthSettings?.bearer) {
accessToken = dynamicAuthSettings?.bearer?.bearerToken
Expand All @@ -41,7 +54,8 @@ const destination: DestinationDefinition<SettingsWithDynamicAuth> = {
}
return {
headers: {
authorization: `${tokenPrefix} ${accessToken}`
authorization: `${tokenPrefix} ${accessToken}`,
...(xSignatureHeader || {})
}
}
},
Expand Down
Loading