feat(cdp): allowlist SNS TopicArn on the SES webhook#61833
Conversation
A verified SNS signature only proves the envelope was signed by *some* SNS topic, not that it came from our SES notification topic. An attacker could get their own topic auto-confirmed and then publish forged (e.g. Bounce) notifications that reach recipient opt-out. Reject envelopes whose TopicArn isn't in the new CDP_SES_WEBHOOK_ALLOWED_TOPIC_ARNS allowlist, after signature verification and before auto-confirming subscriptions or handling notifications. An empty allowlist keeps today's allow-all behaviour (with a one-time warning) so the check can ship before the env var is wired up per environment. Closes PostHog#61644
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds an SNS TopicArn allowlist to the SES webhook handler to prevent accepting (and auto-confirming) SNS messages from untrusted topics.
Changes:
- Introduces a configurable comma-separated SNS TopicArn allowlist and enforces it in
SesWebhookHandler. - Logs a warning when the allowlist is empty (permissive mode) and rejects non-allowlisted topics with
403. - Adds unit tests covering allowlisted, non-allowlisted, whitespace/comma parsing, and SubscriptionConfirmation behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| nodejs/src/cdp/services/messaging/helpers/ses.ts | Adds allowlist parsing, enforcement, and warning behavior in the webhook handler. |
| nodejs/src/cdp/services/messaging/helpers/ses.test.ts | Adds tests validating allowlist enforcement and confirmation-flow safety. |
| nodejs/src/cdp/config.ts | Adds new config key and default value for the allowlist. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
nodejs/src/cdp/services/messaging/helpers/ses.test.ts:310-330
The first two tests exercise the same code path with different inputs and expected outputs — the canonical case for a parameterized test. Per the project's style preference, these should be a single `it.each` block.
```suggestion
it.each([
{ label: 'allowlisted ARN', topicArn: allowedArn, expectedStatus: 200, expectedMetric: 'email_opened' as const },
{ label: 'foreign ARN', topicArn: foreignArn, expectedStatus: 403, expectedMetric: undefined },
])('$label: status=$expectedStatus', async ({ topicArn, expectedStatus, expectedMetric }) => {
const allowlisted = new SesWebhookHandler(allowedArn)
const result = await allowlisted.handleWebhook({
body: notification(topicArn),
headers: {},
verifySignature: false,
})
expect(result.status).toBe(expectedStatus)
expect(result.metrics?.[0]?.metricName).toBe(expectedMetric)
})
```
Reviews (1): Last reviewed commit: "feat(cdp): allowlist SNS TopicArn on the..." | Re-trigger Greptile |
- Make the empty-allowlist warning module-scoped so it fires at most once per process, not once per handler instance. - Gate the TopicArn check on parsed.mode === 'sns', matching the confirmation-flow guard below it. - Clarify the config comment: the allowlist is independent of SNS signature verification. - Parameterise the allowlisted/non-allowlisted Notification tests with it.each.
|
Thanks for the reviews — pushed a follow-up addressing the feedback:
On the allow-all-by-default point (hex-security): that's intentional and is the design requested in #61644 — "Empty default = allow-all (+ warn), so the code can ship before the env is configured; enforcement turns on once charts sets the var per env. No code redeploy needed to enable." The plan is to ship this, then wire |
Problem
The SES event webhook (
/public/m/ses_webhook) verifies the SNS message signature, which proves the envelope was signed by some SNS topic — but not that it came from our SES notification topic. The handler also auto-confirms anysns.<region>.amazonaws.comsubscription.That leaves a residual forged-event vector: an attacker can create their own SNS topic, get our endpoint to auto-confirm it, then publish a signed
Bouncenotification with a forged tracking tag, reachingrecipientsManager.optOutfor a team. A valid signature alone doesn't stop this.Closes #61644
Changes
CDP_SES_WEBHOOK_ALLOWED_TOPIC_ARNS(comma-separated) innodejs/src/cdp/config.ts, parsed into aSetinSesWebhookHandler.TopicArnisn't in the allowlist (403).Out of scope for this repo (tracked in the issue): wiring the env per environment in charts to the
sns_topic_arnoutput of theses-eventsTerraform module, and the separate follow-up to reject unsigned tracking codes.How did you test this code?
Added unit tests in
ses.test.tscovering: a Notification from an allowlisted ARN is processed; one from a non-allowlisted ARN is rejected with403; whitespace in the comma-separated list is trimmed; aSubscriptionConfirmationfrom a foreign topic is rejected without auto-confirming (nofetchTextcall); and an empty allowlist allows any topic.I did not run the plugin-server test suite locally — its toolchain requires native builds (rdkafka/cyclotron) and backing services — so the new tests have not been executed on my machine and are relying on CI. No manual testing was performed.