feat(payments-next): Add FxA Webhook support#20300
Conversation
There was a problem hiding this comment.
Pull request overview
Adds Firefox Accounts (FxA) Security Event Token (SET) webhook handling to the payments-next API by introducing a new FxA webhook service/controller pair, associated config/types/errors, and a local test script to generate and send signed events.
Changes:
- Introduces
FxaWebhookService+FxaWebhooksControllerto authenticate and dispatch FxA webhook events. - Adds
FxaWebhookConfigand wires it into payments-apiRootConfig/AppModuleto enable configuration-driven validation. - Adds unit tests and a local integration script (
test-fxa-webhook.ts) to exercise the endpoint.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| libs/payments/webhooks/src/lib/fxa-webhooks.types.ts | Defines FxA event URIs and SET payload types. |
| libs/payments/webhooks/src/lib/fxa-webhooks.service.ts | Implements SET bearer token extraction, signature verification, and event dispatch. |
| libs/payments/webhooks/src/lib/fxa-webhooks.service.spec.ts | Adds tests for auth validation, event dispatch, and unhandled event reporting. |
| libs/payments/webhooks/src/lib/fxa-webhooks.error.ts | Adds structured errors for auth failures and unhandled event types. |
| libs/payments/webhooks/src/lib/fxa-webhooks.controller.ts | Exposes POST /webhooks/fxa endpoint. |
| libs/payments/webhooks/src/lib/fxa-webhooks.controller.spec.ts | Tests controller-to-service wiring. |
| libs/payments/webhooks/src/lib/fxa-webhooks.config.ts | Adds typed config for issuer/audience/public JWK (with env JSON parsing). |
| libs/payments/webhooks/src/index.ts | Exports new FxA webhook modules from the webhooks library. |
| apps/payments/api/src/scripts/test-fxa-webhook.ts | Local script to sign and POST a SET to the webhook endpoint. |
| apps/payments/api/src/config/index.ts | Adds FxA webhook config to the API root typed config schema. |
| apps/payments/api/src/app/app.module.ts | Registers the new FxA controller/service in the payments API module. |
| apps/payments/api/.env | Adds FxA webhook env var placeholders for local config. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const signed = match[1] + '.' + match[2]; | ||
| const signature = Buffer.from(match[3], 'base64'); | ||
| const verifier = crypto.createVerify('RSA-SHA256'); | ||
| verifier.update(signed); | ||
|
|
||
| if (!verifier.verify(this.publicPem, signature)) { | ||
| return null; | ||
| } | ||
|
|
||
| const payload = JSON.parse( | ||
| Buffer.from(match[2], 'base64').toString() | ||
| ) as FxaSecurityEventTokenPayload; |
There was a problem hiding this comment.
JWTs use base64url encoding for the header/payload/signature segments, but this code decodes them with Buffer.from(..., 'base64'). Since the regex explicitly allows '-' and '_' (base64url alphabet), using 'base64' here can lead to incorrect decoding and failed signature verification/parsing in some Node versions. Consider decoding with 'base64url' (or normalizing base64url to base64) for both the signature and payload segments.
| this.log.log('handlePasswordChange', { sub, event }); | ||
| this.statsd.increment('fxa.webhook.event', { | ||
| eventType: 'password-change', | ||
| }); | ||
| } | ||
|
|
||
| private async handleProfileChange( | ||
| sub: string, | ||
| event: FxaProfileChangeEvent | ||
| ): Promise<void> { | ||
| this.log.log('handleProfileChange', { sub, event }); | ||
| this.statsd.increment('fxa.webhook.event', { | ||
| eventType: 'profile-change', | ||
| }); |
There was a problem hiding this comment.
These handlers log the full event payload (and sub). For profile-change this can include email and other account state fields, which is likely PII/sensitive and could end up in centralized logs. Consider logging only a minimal, non-PII subset (e.g., event type + uid hash/last4) or redacting specific fields before logging.
| @IsString() | ||
| public readonly fxaWebhookAudience!: string; | ||
|
|
||
| @Transform(({ value }) => (typeof value === 'string' ? JSON.parse(value) : value)) |
There was a problem hiding this comment.
The JSON.parse() in this @Transform will throw a raw SyntaxError when the env var is malformed (or an empty string), which can make configuration failures harder to diagnose. Consider catching parse errors and surfacing a clearer configuration/validation error message for FXA_WEBHOOK_PUBLIC_JWK.
| @Transform(({ value }) => (typeof value === 'string' ? JSON.parse(value) : value)) | |
| @Transform(({ value }) => { | |
| if (typeof value !== 'string') { | |
| return value; | |
| } | |
| try { | |
| return JSON.parse(value); | |
| } catch (err: any) { | |
| const message = | |
| 'Invalid JSON provided for FXA_WEBHOOK_PUBLIC_JWK environment variable: ' + | |
| (err && err.message ? err.message : String(err)); | |
| throw new Error(message); | |
| } | |
| }) |
| #!/usr/bin/env ts-node | ||
| /* This Source Code Form is subject to the terms of the Mozilla Public | ||
| * License, v. 2.0. If a copy of the MPL was not distributed with this | ||
| * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ | ||
|
|
||
| /** | ||
| * Local integration test script for the FxA webhook endpoint. | ||
| * | ||
| * Generates a signed JWT Security Event Token and POSTs it to the | ||
| * payments API webhook route. Uses the test RSA key pair from the | ||
| * event-broker test suite. | ||
| * | ||
| * Usage: | ||
| * npx tsx apps/payments/api/src/scripts/test-fxa-webhook.ts [options] | ||
| * |
There was a problem hiding this comment.
The script header says to run with npx tsx ..., but the shebang is #!/usr/bin/env ts-node. This mismatch can confuse users and may fail depending on what runtime is installed. Consider aligning the shebang and the documented invocation (either tsx everywhere or ts-node everywhere).
| this.statsd.increment('fxa.webhook.error'); | ||
| } | ||
| this.log.error(error); | ||
| Sentry.captureException(error); |
There was a problem hiding this comment.
handleWebhookEvent captures all errors (including expected auth failures like missing/invalid Authorization) to Sentry. This can create noisy alerting and higher ingestion costs if the endpoint receives routine invalid traffic. Consider skipping Sentry.captureException for FxaWebhookAuthError (while still incrementing StatsD) or capturing it at a lower severity/sampled rate.
| Sentry.captureException(error); | |
| if (!(error instanceof FxaWebhookAuthError)) { | |
| Sentry.captureException(error); | |
| } |
df13e09 to
83b6e3a
Compare
| } | ||
| this.log.error(error); | ||
| Sentry.captureException(error); | ||
| // Swallow error to avoid retries |
There was a problem hiding this comment.
Until we have a queue in place, we probably do not want to swallow so that we do get retries
| payload: FxaSecurityEventTokenPayload | ||
| ): Promise<void> { | ||
| for (const eventUri of Object.keys(payload.events)) { | ||
| const eventData = payload.events[eventUri]; |
There was a problem hiding this comment.
We should use zod to enforce shape here, which can also infer the type so that the casts below aren't necessary.
There was a problem hiding this comment.
I'm using type-dependent safeParse(eventData) calls below this to ensure the type is what we expect. A top-level globalFooPattern.safeParse(eventData) isn't viable with how different their structures are.
| events: { | ||
| [uri: string]: Record<string, any>; | ||
| }; | ||
| } |
There was a problem hiding this comment.
We can replace almost all of the types above with zod schemas and then zod infer the type from those schemas.
There was a problem hiding this comment.
question: Is this a temporary file?
There was a problem hiding this comment.
Its useful for testing. We can either leave it as a script for manual local testing, or remove it before merge
There was a problem hiding this comment.
question: is it possible to test this locally with the fxa-event-broker service?
If the fxa-event-broker service works in the local stack, and the events can be tested in that way, then I'm leaning towards not merging the script. It adds code that might need to be maintained in future, etc. But that's my 2 cents, also happy to merge if it makes sense.
| changeTime: number; | ||
| } | ||
|
|
||
| export interface FxaProfileChangeEvent { |
There was a problem hiding this comment.
issue: oops, this doesn't seem to match the documented payload
https://github.com/mozilla/fxa/blob/main/packages/fxa-event-broker/README.md#profile-change
There was a problem hiding this comment.
Agreed, but it does match the actual implementation of their generateProfileSET method
the pubsub proxy controller calls jwtset.generateProfileSET with
clientId,
uid: message.uid,
email: message.email,
locale: message.locale,
metricsEnabled: message.metricsEnabled,
totpEnabled: message.totpEnabled,
accountDisabled: message.accountDisabled,
accountLocked: message.accountLocked,
and only these fields make its way to the events field of the SET:
email: proEvent.email,
locale: proEvent.locale,
metricsEnabled: proEvent.metricsEnabled,
totpEnabled: proEvent.totpEnabled,
accountDisabled: proEvent.accountDisabled,
accountLocked: proEvent.accountLocked,
which matches the FxaProfileChangeEvent type
There was a problem hiding this comment.
This code doesn't look particularly volatile either. Last change was by Danny Coates 4 years ago, I think.
There was a problem hiding this comment.
I've updated the typing to be more in line with what the docs have, but in a way that still works with the actual structure theyre sending
There was a problem hiding this comment.
Oh nice, well spotted. As an optional task, it'd be good to update the docs.
| @Inject(StatsDService) private statsd: StatsD, | ||
| @Inject(Logger) private log: LoggerService | ||
| ) { | ||
| this.publicPem = jwk2pem(fxaWebhookConfig.fxaWebhookPublicJwk); |
There was a problem hiding this comment.
issue: fetch public keys from FxA public url instead of providing via env var.
FxA docs recommend fetching the public keys
Verify them using FxA's public keys from the JWKS endpoint
| } | ||
| } | ||
|
|
||
| private async handlePasswordChange( |
There was a problem hiding this comment.
question: should the handle* methods be moved to their own class?
Moving the handle methods to their own class shows a clear separation in purpose of each class.
- The
fxa-webhooks.servicereceives the event, authenticates it and dispatches it to the correct handler. - The "handler" class, implements the logic for how the events should be processed.
There was a problem hiding this comment.
As it stands right now, the FxaWebhooksService class is responsible for two things: event authentication, and event handling. I think that the handle* methods should stay in this service layer. They are where (eventually) the bulk of the business business logic for the webhooks will live, and that feels in line with the goal of a service.
If the goal is to streamline this class, I'd maybe suggest we bump out the authentication methods into the fxa webhooks controller layer, or another new class. What do you think?
There was a problem hiding this comment.
That sounds good to me.
Just to share my thinking, for the Stripe Webhook logic I opted to have the StripeWebhookService to handle just the logic around decoding the event and making sure it goes to the right place, and then have the SubscriptionEventsService that contains the business logic for each event. IMO it creates a nice separation of concerns, so I figured I'd just suggest it.
0eef349 to
2c2fc49
Compare
| const issuer = this.fxaWebhookConfig.fxaWebhookIssuer.endsWith('/') | ||
| ? this.fxaWebhookConfig.fxaWebhookIssuer | ||
| : this.fxaWebhookConfig.fxaWebhookIssuer + '/'; | ||
| const discoveryUrl = `${issuer}.well-known/openid-configuration`; | ||
| const discoveryResponse = await fetch(discoveryUrl); | ||
| if (!discoveryResponse.ok) { | ||
| throw new FxaWebhookJwksError( | ||
| `Failed to fetch OIDC discovery document: ${discoveryResponse.status}` | ||
| ); | ||
| } | ||
| const { jwks_uri: jwksUri } = await discoveryResponse.json(); | ||
| if (!jwksUri) { | ||
| throw new FxaWebhookJwksError( | ||
| 'OIDC discovery document missing jwks_uri' | ||
| ); | ||
| } |
There was a problem hiding this comment.
suggestion: replace this logic with a config var for the jwks endpoint
I don't think the JWKS endpoint changes often, so it should be fine to provide it via an env var instead of fetching it every time.
| ); | ||
| } | ||
|
|
||
| const jwksResponse = await fetch(jwksUri); |
There was a problem hiding this comment.
issue: do not fetch jwks on every request
This webhook handler will be receiving quite a few requests. I believe the suggestion in the FxA docs was to fetch the jwk once on startup, although an alternative maybe better solution might be have a in memory cache.
suggestion: use in memory cache decorator (currently being used used by Strapi CMS)
StaberindeZA
left a comment
There was a problem hiding this comment.
r+wc. Looks good, lets get this into Stage. Just some small remaining changes.
Also it'd be great if we could test this locally with auth-server and event-broker if it's simple enough to setup.
| ttlSeconds: DEFAULT_JWKS_FALLBACK_TTL_SECONDS, | ||
| client: (_: any, context: FxaWebhookService) => | ||
| context.fallbackCacheAdapter, | ||
| }) |
There was a problem hiding this comment.
suggestion: remove fallback. Just having the memory cache adapter should be good enough.
There was a problem hiding this comment.
I'd like to keep it, if that's alright. Its the same caching pattern as we use elsewhere and I don't see much of a drawback. I'm planning to merge without, but let me know if you'd like me to file a followup!
| const DEFAULT_JWKS_CACHE_TTL_SECONDS = 300; // 300 seconds is 5 minutes. | ||
| const DEFAULT_JWKS_FALLBACK_TTL_SECONDS = 1800; // 1800 seconds is 30 minutes. |
There was a problem hiding this comment.
question(non-blocking): should these be configurable?
There was a problem hiding this comment.
I don't think its necessary, it's the same pattern as elsewhere and I don't expect these to change
Because: * FxA has several webhooks that SubPlat can make use of This commit: * Adds a FxaWebhookService class * Adds routes to the payments-api service to receive webhooks * Adds validations to only handle valid webhook requests Closes #PAY-3464
Because:
This commit:
Closes #PAY-3464
Checklist
Put an
xin the boxes that applyHow to review (Optional)
To verify, take a look at apps/payments/api/src/scripts/test-fxa-webhook.ts