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.

I think we should probably return an error code, right? This way, we explicitly tell Stripe (hey, we couldn't handle this). It probably makes things easier for people requesting refunds etc.

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.

I didn't get this part. Why would they be sending events we didn't request? If that's the case, all the more reason to return 400 or something similar (e.g., 422 - unprocessable content).

I've went with explicitly returning a 422 for the unhandled events. I don't have much experience with Stripe or Lemon Squeezy web hook events so I thought they might not like a non-200 response.

I'll see what happens in the e2e tests in the CI and post a screenshot just to be extra clear on what I meant earlier.

In the CI, we are getting the 422 error meaning there are some unhandled events.

Should we log them and find out? I'll leave it to @vincanger

Based on latest CI run: https://github.com/wasp-lang/open-saas/actions/runs/14514923688/job/40721696044

The unhandled events are:

customer.created

charge.succeeded

payment_method.attached

customer.updated

customer.subscription.created

payment_intent.created

invoice.created

invoice.finalized

invoice.updated

invoice.payment_succeeded

I think that's fine that we don't handle all the events Stripe send to us. If it's okay to have the 422 status code with Stripe, then I think we are okay with the current state of things. @vincanger

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

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

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?

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

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

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

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

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

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

sodic

Nice work, left some more comments but most big stufff is resolved.

@vincanger Please come weigh in on the unresolved threads that require your expertise.

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

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

sodic · 2025-04-15T10:45:04Z

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

+
+export type OrderData = z.infer<typeof orderDataSchema>;
+
+const genericEventSchema = z.object({


Should we document where this type comes from as well?

Can we link the type like we did for the others?

The type is not exported unfortunately, so I get a Typescript error when I try to link to the type.

sodic · 2025-04-15T10:48:53Z

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

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


I think we should probably return an error code, right? This way, we explicitly tell Stripe (hey, we couldn't handle this). It probably makes things easier for people requesting refunds etc.

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.

I didn't get this part. Why would they be sending events we didn't request? If that's the case, all the more reason to return 400 or something similar (e.g., 422 - unprocessable content).

sodic · 2025-04-15T10:51:14Z

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

+    const lineItems = await subscriptionItemsSchema.parseAsync(lineItemsRaw);
+
+    return lineItems;


I advise just returning it directly.

I have to await for the error handling to kick in. If I return directly, the code looks like:

return await subscriptionItemsSchema.parseAsync(lineItemsRaw);

which is a common footgun - maybe somebody removes the await (thinking, I've heard I can just return the promise directly) and then the error handling changes. I'd like to keep the current impl for better clarity around what's going on.

Got it. Didn't notice the catch block.

maybe somebody removes the await (thinking, I've heard I can just return the promise directly)

Hm, you could say the same thing about removing the extra variable 😄
Inlining a expression into the return statement is arguably even a bigger footgun than return await because more people understand it.

For example, if I saw return await, I would think it's weird, probably break out of autopilot, and notice the catch block. With the current code, I thought it was an oversight.

The best solution, IMO, is picking whichever option you prefer and leaving a comment warning the reader about the error handling. Like we did here.

I prefer the current option because it looks dead simple to me. It might not look that clean to some, but to me it's still an okay way of writing that logic.

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

…alue

sodic

Nice work, all good on my side.

I left two unresolved threads but you won't need me for that so I'm approving.

Please have @vincanger check the other stuff.

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

template/app/src/payment/stripe/webhook.ts Outdated Show resolved Hide resolved

Cleanup

20ee20a

infomiho commented Feb 20, 2025

View reviewed changes

template/app/src/payment/stripe/webhookPayload.ts Outdated Show resolved Hide resolved

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

infomiho self-assigned this Apr 7, 2025

sodic reviewed Apr 15, 2025

View reviewed changes

infomiho added 3 commits April 16, 2025 12:17

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

a537d00

PR comments

85ff1af

Destructure event payload right away

8d016dd

infomiho added 2 commits April 16, 2025 13:47

Adds debug log

8ac1924

Adjust payment_intent.succeeded data schema to match the expected v…

8b0eae5

…alue

infomiho requested review from sodic and vincanger April 16, 2025 12:29

sodic approved these changes Apr 16, 2025

View reviewed changes

Adds extra logging for unhandled webhook events

4575008

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds Zod validation for webhook payloads #377

Adds Zod validation for webhook payloads #377

infomiho commented Feb 19, 2025 •

edited

Loading

infomiho Feb 20, 2025

vincanger Feb 20, 2025

infomiho Feb 20, 2025 •

edited

Loading

sodic Apr 15, 2025

infomiho Apr 16, 2025

infomiho Apr 16, 2025

sodic Apr 16, 2025

infomiho Apr 17, 2025 •

edited

Loading

vincanger left a comment

vincanger Feb 20, 2025

infomiho Feb 20, 2025

vincanger Feb 20, 2025

infomiho Feb 21, 2025

vincanger Feb 20, 2025

vincanger left a comment

vincanger Feb 20, 2025

sodic left a comment

sodic left a comment

sodic Apr 15, 2025

sodic Apr 16, 2025

infomiho Apr 17, 2025

sodic Apr 15, 2025

sodic Apr 15, 2025

infomiho Apr 16, 2025 •

edited

Loading

sodic Apr 16, 2025

infomiho Apr 17, 2025

sodic left a comment

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


		export type OrderData = z.infer<typeof orderDataSchema>;

		const genericEventSchema = z.object({

		const lineItems = await subscriptionItemsSchema.parseAsync(lineItemsRaw);

		return lineItems;

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

infomiho Apr 17, 2025 • edited Loading

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

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

sodic 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

infomiho Apr 16, 2025 • edited Loading

Choose a reason for hiding this 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

infomiho commented Feb 19, 2025 •

edited

Loading

infomiho Feb 20, 2025 •

edited

Loading

infomiho Apr 17, 2025 •

edited

Loading

infomiho Apr 16, 2025 •

edited

Loading