Adds Zod validation for webhook payloads #377

infomiho · 2025-02-19T13:59:54Z

Adds runtime validation for all webhook payloads instead of relying on type assertions.

The idea was:

write Zod schemas for all the data we 100% need
parse the data and return it in format { eventName, data } which helps when you check for eventName you are sure of the type of the data object

Signed-off-by: Mihovil Ilakovac <[email protected]>

infomiho · 2025-02-20T09:17:09Z

template/app/src/payment/lemonSqueezy/webhook.ts

  } catch (err) {
+    if (err instanceof UnhandledWebhookEventError) {


I wasn't sure if we wanted to return something other than 200 if we receive a request for a webhook event we don't handle.

I guess we could, but we shouldn't be receiving any webhooks we don't explicitly request from the Stripe dashboard settings. Maybe the console.error is enough?

we shouldn't be receiving any webhooks we don't explicitly request from the Stripe

Yes, I understand but I kept seeing errors for some of the hooks in the e2e tests so I implemented this bit - this way we are just "ignoring" the extra webhook calls.

infomiho · 2025-02-20T09:17:48Z

template/app/src/payment/stripe/webhook.ts

-      // In development, it is likely that you will receive other events that you are not handling, and that's fine. These can be ignored without any issues.
-      console.error('Unhandled event type: ', event.type);
+    if (err instanceof UnhandledWebhookEventError) {
+      return response.status(200).json({ received: true });


Same thing as in https://github.com/wasp-lang/open-saas/pull/377/files#r1963148873

infomiho · 2025-02-20T09:33:37Z

template/app/src/payment/stripe/webhookPayload.ts

+    throw new Error('Invalid Stripe Event');
+  });
+  switch (event.type) {
+    case 'checkout.session.completed':


I tried doing stuff like this to reduce the code duplication, but I couldn't get the exact types (discriminate unions):

const eventToSchema = { 'checkout.session.completed': sessionCompletedDataSchema, 'invoice.paid': invoicePaidDataSchema, 'customer.subscription.updated': subscriptionUpdatedDataSchema, 'customer.subscription.deleted': subscriptionDeletedDataSchema, } as const; function isSchemaDefinedForEvent(eventType: string): eventType is keyof typeof eventToSchema { return eventType in eventToSchema; } if (!isSchemaDefinedForEvent(event.type)) { throw new UnhandledWebhookEventError(event.type); } const schema = eventToSchema[event.type]; const data = await schema.parseAsync(event.data.object).catch(handleParsingError); return { eventName: event.type, data };

this is starting to look more complicated than it needs to be.

But wouldn't using a Record type be clearer, where the key type is Stripe.Event? That way you wouldn't have to use eventType is keyof typeof eventToSchema

this is starting to look more complicated than it needs to be.

You mean the code in the PR or the code in this comment?

@infomiho Yeah, this is a classic problem. Once you get down to a union type, you can't deunionize it (if it's unknown at compile time).

You could make it work by passing the event.type into a function, and then querying for the correct schema inside. At least I think this would work, I could try tomorrow. On second thought, maybe not. I'd have to play around.

But, I think I agree with @vincanger - this is a lot for an average OpenSaas user.

@vincanger Btw, to answer your question, he can't use Record<Stripe.Event, ...> because Stripe.Event is an alias for a string (they underspecified the type).

For reducing duplication, we could focus on each case's body and perhaps extract a function:

switch (event.type) { // ... case 'payment_intent.succeeded': const paymentIntent = await parseStripeEventData(paymentIntentSucceededDataSchema, event.data.object); return { eventName: event.type, data: paymentIntent }; // option 1 case 'customer.subscription.updated': return { eventName: event.type, data: parseStripeEventData(subscriptionDeletedDataSchema, event.data.object), }; // option 2 case 'customer.subscription.deleted': const deletedSubscription = await parseStripeEventData( subscriptionDeletedDataSchema, event.data.object ); return { eventName: event.type, data: deletedSubscription }; } function parseStripeEventData<Z extends z.AnyZodObject>( schema: Z, rawStripeEvent: unknown ): Promise<z.infer<Z>> { return schema.parseAsync(rawStripeEvent).catch((e) => { console.error(e); throw new Error('Error parsing Stripe event object'); }); }

But again, since this is a template, and the types here are pretty scary, I'd probably keep the duplication.

I am even on the edge of suggesting we hold off with introducing Zod to webhooks, but I can't judge is it to HC or not. I'll leave that to @vincanger.

vincanger

Looks good. I'd say we merge the payment_intent.succeeded webhook addition and add that first, before merging though.

vincanger · 2025-02-20T09:10:51Z

template/app/src/payment/stripe/webhook.ts

@@ -10,47 +10,75 @@ import { emailSender } from 'wasp/server/email';
 import { assertUnreachable } from '../../shared/utils';
 import { requireNodeEnvVar } from '../../server/utils';
 import { z } from 'zod';
+import {
+  InvoicePaidData,
+  parseWebhookPayload,


should we separate and import types at the type of the file, as we've been doing?

You mean import the types at the top of the file? I don't see that we did that in this file e.g.

import { type MiddlewareConfigFn, HttpError } from 'wasp/server';

is on top.

I've added the import type bit for the types.

ah true, this won't apply for zod types as they're runtime specific, right?

this won't apply for zod types as they're runtime specific, right?

I'm not following sorry :) What do mean exactly that won't apply to Zod types?

vincanger · 2025-02-20T09:11:29Z

template/app/src/payment/stripe/webhook.ts

+      case 'customer.subscription.deleted':
+        await handleCustomerSubscriptionDeleted(payload.data, prismaUserDelegate);
+        break;
+      default:


We're missing payment_intent.succeeded but I think that's still awaiting to be merged in another PR...

I've added the handling of the payment_intent.succeeded hook 👍

Just to double check. How come this was missing and is it possible we missed something else?

It was a parallel PR of introducing payment_intent.succeeded event #375

vincanger · 2025-02-20T09:43:30Z

template/app/src/payment/lemonSqueezy/webhook.ts

  } catch (err) {
+    if (err instanceof UnhandledWebhookEventError) {


I guess we could, but we shouldn't be receiving any webhooks we don't explicitly request from the Stripe dashboard settings. Maybe the console.error is enough?

vincanger

tested it out and looking good.

vincanger · 2025-02-20T19:40:31Z

template/app/src/payment/stripe/webhook.ts

@@ -10,47 +10,75 @@ import { emailSender } from 'wasp/server/email';
 import { assertUnreachable } from '../../shared/utils';
 import { requireNodeEnvVar } from '../../server/utils';
 import { z } from 'zod';
+import {
+  InvoicePaidData,
+  parseWebhookPayload,


ah true, this won't apply for zod types as they're runtime specific, right?

sodic

Nice work!

I left some comments. Don't think I found anything wrong, but I had some questions.

Note

Btw, in the future, I recommend doing refactors/non-functional changes in a separate PR. Git often doesn't realize something was moved and important changes can slip through unnoticed.

I've only started doing this very recently after reading this great article: https://mtlynch.io/code-review-love. It's a must-read. Although I was and still am guilty of some of the things he mentions

sodic · 2025-02-25T22:34:37Z

template/app/src/payment/lemonSqueezy/webhookPayload.ts

+import * as z from 'zod';
+import { UnhandledWebhookEventError } from '../errors';
+
+export async function parseWebhookPayload(rawPayload: string) {


This one's missing a return type and is a module boundary.

Btw, it's a complicated type. Should we maybe name it?

Given that this is a template, I wouldn't type the return type. It feels complex and users will need to update multiple places just to satisfy the Typescript compiler.

sodic · 2025-02-25T22:35:41Z

template/app/src/payment/lemonSqueezy/webhookPayload.ts

+import { UnhandledWebhookEventError } from '../errors';
+
+export async function parseWebhookPayload(rawPayload: string) {
+  const rawEvent = JSON.parse(rawPayload) as unknown;


No difference in this case, but it's better to keep it clean:

const rawEvent: unknown = JSON.parse(rawPayload);

sodic · 2025-02-25T22:36:23Z

template/app/src/payment/lemonSqueezy/webhookPayload.ts

+    console.error(e);
+    throw new Error('Invalid Lemon Squeezy Webhook Event');


Why are we both logging the error and rethrowing it? Same question applies to other places too.

I've cleaned up this code now. I log errors in this file to get the full Zod error message for the developer to debug if needed.

sodic · 2025-02-25T22:41:44Z

template/app/src/payment/lemonSqueezy/webhook.ts

    } else {
-      response.status(400).json({ error: 'Error Processing Lemon Squeezy Webhook Event' });
+      return response.status(400).json({ error: 'Error Processing Lemon Squeezy Webhook Event' });


How did this work without return before?

Express sends a response as soon as you call json, but I've added return here so the other code doesn't execute needlessly. It just makes the code more precise.

sodic · 2025-02-25T22:43:40Z

template/app/src/payment/lemonSqueezy/webhook.ts

        break;
      default:
-        console.error('Unhandled event type: ', event.meta.event_name);
+        // If you'd like to handle more events, you can add more cases above.
+        assertUnreachable(payload);


I don't recommend changing this in Open Saas, just FYI for future projects.

There's now a new way of doing this which is arguably simpler:

payload satisfies never

I only learned about it recently (possibly from you, can't remember).

@vincanger Also please double check whether we truly want exhaustiveness here.

From what I remember, I think suggested this too. Then @Martinsos and @vincanger explained why it's undesirable.

Perhaps lemonSqueezy can send an event we don't know about, which shouldn't be that big of a deal to us? Something like that.

In the webhookPayload.ts file:

we parse the event name as a string with Zod (we allow all event names)

we use a switch statement to go through the event names (returning an exact eventName and typed data)

in the end, in the default block we throw an UnhandledWebhookEventError

This enables the code in the webhook.ts ability to be exhaustive with all the eventNames, but catch this specific error UnhandledWebhookEventError and handle it in any way needed, for example, I'm returning a 200 response in that case.

sodic · 2025-02-25T23:42:41Z

template/app/src/payment/stripe/webhookPayload.ts

+    throw new Error('Invalid Stripe Event');
+  });
+  switch (event.type) {
+    case 'checkout.session.completed':


@infomiho Yeah, this is a classic problem. Once you get down to a union type, you can't deunionize it (if it's unknown at compile time).

You could make it work by passing the event.type into a function, and then querying for the correct schema inside. At least I think this would work, I could try tomorrow. On second thought, maybe not. I'd have to play around.

But, I think I agree with @vincanger - this is a lot for an average OpenSaas user.

@vincanger Btw, to answer your question, he can't use Record<Stripe.Event, ...> because Stripe.Event is an alias for a string (they underspecified the type).

sodic · 2025-02-25T23:46:31Z

template/app/src/payment/stripe/webhookPayload.ts

+    throw new Error('Invalid Stripe Event');
+  });
+  switch (event.type) {
+    case 'checkout.session.completed':


For reducing duplication, we could focus on each case's body and perhaps extract a function:

switch (event.type) { // ... case 'payment_intent.succeeded': const paymentIntent = await parseStripeEventData(paymentIntentSucceededDataSchema, event.data.object); return { eventName: event.type, data: paymentIntent }; // option 1 case 'customer.subscription.updated': return { eventName: event.type, data: parseStripeEventData(subscriptionDeletedDataSchema, event.data.object), }; // option 2 case 'customer.subscription.deleted': const deletedSubscription = await parseStripeEventData( subscriptionDeletedDataSchema, event.data.object ); return { eventName: event.type, data: deletedSubscription }; } function parseStripeEventData<Z extends z.AnyZodObject>( schema: Z, rawStripeEvent: unknown ): Promise<z.infer<Z>> { return schema.parseAsync(rawStripeEvent).catch((e) => { console.error(e); throw new Error('Error parsing Stripe event object'); }); }

But again, since this is a template, and the types here are pretty scary, I'd probably keep the duplication.

I am even on the edge of suggesting we hold off with introducing Zod to webhooks, but I can't judge is it to HC or not. I'll leave that to @vincanger.

sodic · 2025-02-25T23:52:10Z

template/app/src/payment/stripe/webhook.ts

+    const secret = requireNodeEnvVar('STRIPE_WEBHOOK_SECRET');
+    const sig = request.headers['stripe-signature'];
+    if (!sig) {
+      throw new HttpError(400, 'Stripe Webhook Signature Not Provided');
+    }


If you're up for a refactor - this can easily be a checkSignatureOrThrowHttpError function.

Lemon has something similar I believe, but it's in the old code.

Extract this into a parseRequestBody fn.

sodic · 2025-02-25T23:53:50Z

template/app/src/payment/stripe/webhook.ts

@@ -103,7 +133,7 @@ export async function handlePaymentIntentSucceeded(
    return;
  }

-  const userStripeId = validateUserStripeIdOrThrow(paymentIntent.customer);


How come we could kick this out?

Zod now validates that the customer is a string :)

sodic · 2025-02-25T23:55:52Z

template/app/src/payment/stripe/webhook.ts

 };

+function ensureStripeEvent(request: express.Request, sig: string | string[], secret: string): Stripe.Event {


Why ensure? Why not construct?

Maybe ensureStripeEventIsSomething would be even better

Good point, I'll think about renaming this fn 👍

Renamed into constructStripeEvent

infomiho force-pushed the miho-webhooks-runtime-validations branch from 06458e3 to 4dc0c4b Compare February 19, 2025 14:02

infomiho marked this pull request as ready for review February 19, 2025 14:06

infomiho force-pushed the miho-webhooks-runtime-validations branch from 7760552 to 4cc1b39 Compare February 19, 2025 15:31

Adds Zod validation for webhook payloads

a8f52f2

Signed-off-by: Mihovil Ilakovac <[email protected]>

infomiho force-pushed the miho-webhooks-runtime-validations branch from 5750330 to a8f52f2 Compare February 19, 2025 15:57

infomiho requested review from vincanger and sodic February 20, 2025 09:16

infomiho commented Feb 20, 2025

View reviewed changes

Cleanup

20ee20a

infomiho commented Feb 20, 2025

View reviewed changes

infomiho added 2 commits February 20, 2025 10:38

Update Lemon Squeezy validation

ad41d60

Update Lemon Squeezy validation #2

b7de406

vincanger requested changes Feb 20, 2025

View reviewed changes

infomiho added 4 commits February 20, 2025 10:46

Add extra comments above the validation schemas

ef496af

Merge branch 'main' into miho-webhooks-runtime-validations

a969544

Drop redundant validation

77ad7a4

Adds type imports

e40c1e4

vincanger approved these changes Feb 20, 2025

View reviewed changes

infomiho mentioned this pull request Feb 21, 2025

Dry up subscription status #382

Merged

Merge branch 'main' into miho-webhooks-runtime-validations

40afc5e

sodic reviewed Feb 26, 2025

View reviewed changes

infomiho added 2 commits February 28, 2025 12:36

Merge branch 'main' into miho-webhooks-runtime-validations

e628733

PR comments

98047b7

infomiho requested a review from sodic February 28, 2025 17:13

		} catch (err) {
		if (err instanceof UnhandledWebhookEventError) {

		console.error(e);
		throw new Error('Invalid Lemon Squeezy Webhook Event');

		};

		function ensureStripeEvent(request: express.Request, sig: string \| string[], secret: string): Stripe.Event {

Adds Zod validation for webhook payloads #377

Are you sure you want to change the base?

Adds Zod validation for webhook payloads #377

Conversation

infomiho commented Feb 19, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

infomiho Feb 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincanger Feb 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sodic Feb 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincanger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincanger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sodic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

infomiho Feb 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

infomiho Feb 28, 2025 • edited Loading

Choose a reason for hiding this comment

sodic Feb 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

infomiho commented Feb 19, 2025 •

edited

Loading

infomiho Feb 20, 2025 •

edited

Loading

vincanger Feb 20, 2025 •

edited

Loading

sodic Feb 25, 2025 •

edited

Loading

infomiho Feb 28, 2025 •

edited

Loading

infomiho Feb 28, 2025 •

edited

Loading

sodic Feb 25, 2025 •

edited

Loading