-
-
Notifications
You must be signed in to change notification settings - Fork 1k
Adds Zod validation for webhook payloads #377
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.
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
Changes from all commits
a8f52f2
20ee20a
ad41d60
b7de406
ef496af
a969544
77ad7a4
e40c1e4
40afc5e
e628733
98047b7
a537d00
85ff1af
8d016dd
8ac1924
8b0eae5
4575008
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
export class UnhandledWebhookEventError extends Error { | ||
constructor(eventType: string) { | ||
super(`Unhandled event type: ${eventType}`); | ||
this.name = 'UnhandledWebhookEventError'; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,77 @@ | ||
import * as z from 'zod'; | ||
import { UnhandledWebhookEventError } from '../errors'; | ||
import { HttpError } from 'wasp/server'; | ||
|
||
export async function parseWebhookPayload(rawPayload: string) { | ||
sodic marked this conversation as resolved.
Show resolved
Hide resolved
|
||
try { | ||
const rawEvent: unknown = JSON.parse(rawPayload); | ||
const { meta, data } = await genericEventSchema.parseAsync(rawEvent); | ||
switch (meta.event_name) { | ||
case 'order_created': | ||
const orderData = await orderDataSchema.parseAsync(data); | ||
return { eventName: meta.event_name, meta, data: orderData }; | ||
case 'subscription_created': | ||
case 'subscription_updated': | ||
case 'subscription_cancelled': | ||
case 'subscription_expired': | ||
const subscriptionData = await subscriptionDataSchema.parseAsync(data); | ||
return { eventName: meta.event_name, meta, data: subscriptionData }; | ||
default: | ||
// If you'd like to handle more events, you can add more cases above. | ||
throw new UnhandledWebhookEventError(meta.event_name); | ||
} | ||
} catch (e: unknown) { | ||
if (e instanceof UnhandledWebhookEventError) { | ||
throw e; | ||
} else { | ||
console.error(e); | ||
throw new HttpError(400, 'Error parsing Lemon Squeezy webhook payload'); | ||
} | ||
} | ||
} | ||
|
||
export type SubscriptionData = z.infer<typeof subscriptionDataSchema>; | ||
|
||
export type OrderData = z.infer<typeof orderDataSchema>; | ||
|
||
/** | ||
* This schema is based on LemonSqueezyResponse type | ||
*/ | ||
const genericEventSchema = z.object({ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we document where this type comes from as well? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we link the type like we did for the others? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The type is not exported unfortunately, so I get a Typescript error when I try to link to the type. |
||
meta: z.object({ | ||
event_name: z.string(), | ||
custom_data: z.object({ | ||
user_id: z.string(), | ||
}), | ||
}), | ||
data: z.unknown(), | ||
}); | ||
|
||
/** | ||
* This schema is based on | ||
* @type import('@lemonsqueezy/lemonsqueezy.js').Order | ||
* specifically Order['data']. | ||
*/ | ||
const orderDataSchema = z.object({ | ||
attributes: z.object({ | ||
customer_id: z.number(), | ||
status: z.string(), | ||
first_order_item: z.object({ | ||
variant_id: z.number(), | ||
}), | ||
order_number: z.number(), | ||
}), | ||
}); | ||
|
||
/** | ||
* This schema is based on | ||
* @type import('@lemonsqueezy/lemonsqueezy.js').Subscription | ||
* specifically Subscription['data']. | ||
*/ | ||
const subscriptionDataSchema = z.object({ | ||
attributes: z.object({ | ||
customer_id: z.number(), | ||
status: z.string(), | ||
variant_id: z.number(), | ||
}), | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the CI, we are getting the
422
error meaning there are some unhandled events.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we log them and find out? I'll leave it to @vincanger
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on latest CI run: https://github.com/wasp-lang/open-saas/actions/runs/14514923688/job/40721696044
The unhandled events are:
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is totally fine according to Stripe. I've read through the docs and am making sure to handle the most important ones for most SaaS apps (plus a couple more, I think).