Feat/implement webhook verification#11
Conversation
|
Warning Review limit reached
More reviews will be available in 28 minutes and 59 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds Stripe billing support: a ChangesStripe Billing Infrastructure
Sequence DiagramsequenceDiagram
rect rgba(173, 216, 230, 0.5)
note over Client,MongoDB: Stripe Billing: /me Endpoint & Checkout
end
participant Client as Client
participant MeRoute as GET /api/me
participant CheckoutRoute as POST /api/stripe/create-checkout
participant getCurrentUser as getCurrentUser()
participant MongoDB as MongoDB
participant StripeAPI as Stripe API
Client->>MeRoute: GET /api/me
MeRoute->>getCurrentUser: verify auth cookie
getCurrentUser-->>MeRoute: JWTPayload | null (401 if null)
MeRoute->>MongoDB: find User by id
MeRoute->>MongoDB: find active Membership
MeRoute->>MongoDB: find Tenant
MongoDB-->>MeRoute: user + membership + tenant
MeRoute-->>Client: { user, membership: { role, tenant } }
Client->>CheckoutRoute: POST { priceId }
CheckoutRoute->>getCurrentUser: verify auth cookie
getCurrentUser-->>CheckoutRoute: JWTPayload | null (401 if null)
CheckoutRoute->>MongoDB: verify active owner Membership (403 if missing)
CheckoutRoute->>MongoDB: load Tenant by id (404 if missing)
CheckoutRoute->>CheckoutRoute: compute planType from priceId
alt tenant.stripeCustomerId missing
CheckoutRoute->>StripeAPI: customers.create(email, name, metadata)
StripeAPI-->>CheckoutRoute: customer.id
CheckoutRoute->>MongoDB: persist stripeCustomerId to Tenant
end
CheckoutRoute->>StripeAPI: checkout.sessions.create(subscription, priceId, metadata)
StripeAPI-->>CheckoutRoute: session with url
CheckoutRoute-->>Client: { url: session.url }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (1)
docs/guides/billing.md (1)
1-3: ⚡ Quick winReplace placeholder with minimum operator setup steps before release.
This doc currently provides no actionable setup/verification guidance for new billing flows (env vars, webhook setup, checkout route behavior).
I can draft a concise v1 billing runbook (env matrix + Stripe webhook setup + local test checklist) if you want me to open a follow-up issue.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/guides/billing.md` around lines 1 - 3, The billing.md file contains only a placeholder TODO comment with no actionable setup guidance. Replace the TODO comment with concrete documentation covering the minimum operator setup steps required before release: document the required environment variables and how to configure them, provide step-by-step instructions for setting up Stripe webhooks including webhook endpoint configuration and secret management, describe the checkout route behavior and expected flow, and include a local testing checklist to verify the billing integration works correctly end-to-end.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/api/me/route.ts`:
- Around line 19-24: The issue is that tenant data is being fetched and returned
from tenantModel.findById without verifying that the user has an active
membership first, potentially exposing tenant details to unauthorized users. Add
a check after the membershipModel.findOne call to verify the membership exists
and is not null before proceeding to fetch the tenant with tenantModel.findById.
Only return the tenant data in the NextResponse.json response if the membership
check passes, otherwise return an appropriate error response such as a 403
Forbidden or 404 Not Found status.
- Around line 16-21: The code dereferences user._id in the
membershipModel.findOne query without first verifying that the user variable is
not null. After the dashboardUserModel.findById call returns, add a null check
to ensure the user exists before attempting to access user._id. If the user is
null, return an appropriate error response (such as a 404 Not Found or 401
Unauthorized) to prevent a 500 error when a user no longer exists in the
database.
In `@app/api/stripe/create-checkout/route.ts`:
- Around line 67-68: Replace all instances of process.env.NEXT_PUBLIC_APP_URL
with process.env.APP_URL in the Stripe session creation configuration on lines
67-68 where success_url and cancel_url are being set. Since this is a
server-side route file, use the validated server-side APP_URL environment
variable instead of the public-facing NEXT_PUBLIC_APP_URL to ensure the Stripe
redirect URLs are properly constructed even if the public environment variable
is unset.
- Around line 15-16: The priceId extracted from the request body at line 15 is
not validated against allowed values before being passed to
stripe.checkout.sessions.create() at line 65, which creates a security
vulnerability allowing arbitrary price IDs. Add validation logic after
extracting priceId that checks if it matches either NEXT_PUBLIC_PRO_PRICE_ID or
NEXT_PUBLIC_ENTERPRISE_PRICE_ID constants, and throw an error or return early if
the priceId is not in this allowlist. This validation must occur before the
priceId is used in any Stripe API calls or plan type determination logic.
In `@lib/config/index.ts`:
- Around line 19-21: The webhook secrets for GitHub, Slack, and Shopify are
currently defined as required strings in the config schema, which causes startup
failures in environments that only use certain webhook providers (like Stripe).
Make the GITHUB_WEBHOOK_SECRET, SLACK_WEBHOOK_SECRET, and SHOPIFY_WEBHOOK_SECRET
definitions optional by chaining `.optional()` to each schema definition,
allowing applications to boot successfully without providing unused webhook
provider credentials.
In `@lib/stripe/stripe.ts`:
- Around line 2-13: The server-side Stripe initialization in the stripe.ts file
is importing clientConfig from the shared config module, which can cause server
imports to fail if client environment variables are missing, even though only
the server STRIPE_SECRET_KEY is needed. Remove the clientConfig import and
refactor the isStripeTestMode function to determine test mode by checking the
serverConfig STRIPE_SECRET_KEY instead (which starts with "sk_test_" in test
mode), eliminating the dependency on client config being available during server
initialization.
In `@lib/webhook-verification.ts`:
- Line 26: The log.warn call in the webhook verification code is logging the raw
signature value, which is sensitive security material and should not be exposed
in logs. Modify the log.warn statement to redact or hash the signature before
logging it. Instead of passing the signature parameter directly, compute a hash
of the signature or show only a truncated portion of it (e.g., first few
characters followed by ellipsis) to aid in debugging without exposing the full
sensitive value.
- Around line 57-88: The getVerifier function incorrectly routes Slack and
Shopify to the generic HMACVerifier, which cannot satisfy their distinct
requirements. Slack requires signing the concatenated string
"v0:{timestamp}:{rawBody}" and accessing the X-Slack-Request-Timestamp header
for replay protection, while Shopify requires Base64-encoded (not hex) HMAC
digests. Create two new verifier classes, SlackVerifier and ShopifyVerifier,
that implement WebhookVerifier with their provider-specific signing logic. To
support header access, update the WebhookVerifier interface's verify method
signature to accept a headers parameter (or Headers object) in addition to the
existing parameters. Then update the case statements for "slack" and "shopify"
in the getVerifier function to return instances of the new SlackVerifier and
ShopifyVerifier respectively.
- Around line 13-31: The signature parsing reduce function overwrites duplicate
keys, so when Stripe provides multiple v1 signatures during secret rotation,
only the last one is retained. Refactor the reduce logic to collect all v1
signature values into an array instead of a single string. Additionally, the
parseInt(timestamp) call lacks validation, allowing non-numeric timestamps to
return NaN and bypass the timestamp check. Add validation to ensure timestamp is
a valid numeric string using Number.isInteger or similar checks before parsing
it, then update the timestamp comparison logic to work with the validated
numeric timestamp and ensure all collected v1 signatures are verified against
the expected signature.
In `@types/types.ts`:
- Around line 8-24: The WebhookPayload and WebhookJob interfaces use the `any`
type which bypasses type safety. In the WebhookPayload interface, change the
default type parameter from `any` to `unknown` to force consumers to narrow
types explicitly. In the WebhookJob interface, change the `payload` field from
`any` to use a generic type parameter (similar to WebhookPayload's approach) or
at least use `unknown` instead of `any` to maintain type safety throughout the
webhook contract definitions.
---
Nitpick comments:
In `@docs/guides/billing.md`:
- Around line 1-3: The billing.md file contains only a placeholder TODO comment
with no actionable setup guidance. Replace the TODO comment with concrete
documentation covering the minimum operator setup steps required before release:
document the required environment variables and how to configure them, provide
step-by-step instructions for setting up Stripe webhooks including webhook
endpoint configuration and secret management, describe the checkout route
behavior and expected flow, and include a local testing checklist to verify the
billing integration works correctly end-to-end.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a89184be-116e-4bcb-b718-0c9f9a373be5
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
app/api/me/route.tsapp/api/stripe/create-checkout/route.tsdocs/guides/billing.mdlib/auth/jwt.tslib/config/index.tslib/stripe/stripe.tslib/webhook-verification.tspackage.jsontypes/types.ts
| const { priceId } = await req.json(); | ||
| const tenantId = user.tenantId; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether checkout route enforces a strict allowlist before Stripe session creation.
rg -nP 'priceId|line_items|NEXT_PUBLIC_(PRO|ENTERPRISE)_PRICE_ID|Invalid priceId' app/api/stripe/create-checkout/route.ts lib/config/index.tsRepository: arxja/saasify
Length of output: 521
🏁 Script executed:
cat -n app/api/stripe/create-checkout/route.tsRepository: arxja/saasify
Length of output: 3197
🏁 Script executed:
cat -n lib/config/index.ts | head -50Repository: arxja/saasify
Length of output: 2031
Reject unapproved priceId values before creating Stripe sessions.
Line 15 accepts priceId directly from the request body without validation. The route only uses it to determine plan type metadata (line 40) but passes it unchecked to stripe.checkout.sessions.create() at line 65. This allows clients to specify arbitrary price IDs, bypassing your intended plan constraints.
Implement an allowlist check against NEXT_PUBLIC_PRO_PRICE_ID and NEXT_PUBLIC_ENTERPRISE_PRICE_ID before creating the session:
Proposed fix
const { priceId } = await req.json();
+const proPriceId = process.env.NEXT_PUBLIC_PRO_PRICE_ID;
+const enterprisePriceId = process.env.NEXT_PUBLIC_ENTERPRISE_PRICE_ID;
+const allowedPriceIds = [proPriceId, enterprisePriceId].filter(
+ (v): v is string => Boolean(v),
+);
+if (typeof priceId !== "string" || !allowedPriceIds.includes(priceId)) {
+ return NextResponse.json({ error: "Invalid priceId" }, { status: 400 });
+}
@@
let planType = "pro";
-if (priceId === process.env.NEXT_PUBLIC_ENTERPRISE_PRICE_ID) {
+if (priceId === enterprisePriceId) {
planType = "enterprise";
}Also applies to lines 39-42 (plan type logic) and 65-66 (Stripe API call).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { priceId } = await req.json(); | |
| const tenantId = user.tenantId; | |
| const { priceId } = await req.json(); | |
| const proPriceId = process.env.NEXT_PUBLIC_PRO_PRICE_ID; | |
| const enterprisePriceId = process.env.NEXT_PUBLIC_ENTERPRISE_PRICE_ID; | |
| const allowedPriceIds = [proPriceId, enterprisePriceId].filter( | |
| (v): v is string => Boolean(v), | |
| ); | |
| if (typeof priceId !== "string" || !allowedPriceIds.includes(priceId)) { | |
| return NextResponse.json({ error: "Invalid priceId" }, { status: 400 }); | |
| } | |
| const tenantId = user.tenantId; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/api/stripe/create-checkout/route.ts` around lines 15 - 16, The priceId
extracted from the request body at line 15 is not validated against allowed
values before being passed to stripe.checkout.sessions.create() at line 65,
which creates a security vulnerability allowing arbitrary price IDs. Add
validation logic after extracting priceId that checks if it matches either
NEXT_PUBLIC_PRO_PRICE_ID or NEXT_PUBLIC_ENTERPRISE_PRICE_ID constants, and throw
an error or return early if the priceId is not in this allowlist. This
validation must occur before the priceId is used in any Stripe API calls or plan
type determination logic.
| success_url: `${process.env.NEXT_PUBLIC_APP_URL}/${tenant.subdomain}/dashboard?session_id={CHECKOUT_SESSION_ID}`, | ||
| cancel_url: `${process.env.NEXT_PUBLIC_APP_URL}/${tenant.subdomain}/settings/billing?canceled=true`, |
There was a problem hiding this comment.
Use the validated server app URL for Stripe redirects.
Line 67-Line 68 use NEXT_PUBLIC_APP_URL, but this config file validates APP_URL on the server side. If NEXT_PUBLIC_APP_URL is unset, Stripe redirect URLs become invalid and session creation fails.
Proposed fix
- success_url: `${process.env.NEXT_PUBLIC_APP_URL}/${tenant.subdomain}/dashboard?session_id={CHECKOUT_SESSION_ID}`,
- cancel_url: `${process.env.NEXT_PUBLIC_APP_URL}/${tenant.subdomain}/settings/billing?canceled=true`,
+ success_url: `${process.env.APP_URL}/${tenant.subdomain}/dashboard?session_id={CHECKOUT_SESSION_ID}`,
+ cancel_url: `${process.env.APP_URL}/${tenant.subdomain}/settings/billing?canceled=true`,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| success_url: `${process.env.NEXT_PUBLIC_APP_URL}/${tenant.subdomain}/dashboard?session_id={CHECKOUT_SESSION_ID}`, | |
| cancel_url: `${process.env.NEXT_PUBLIC_APP_URL}/${tenant.subdomain}/settings/billing?canceled=true`, | |
| success_url: `${process.env.APP_URL}/${tenant.subdomain}/dashboard?session_id={CHECKOUT_SESSION_ID}`, | |
| cancel_url: `${process.env.APP_URL}/${tenant.subdomain}/settings/billing?canceled=true`, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/api/stripe/create-checkout/route.ts` around lines 67 - 68, Replace all
instances of process.env.NEXT_PUBLIC_APP_URL with process.env.APP_URL in the
Stripe session creation configuration on lines 67-68 where success_url and
cancel_url are being set. Since this is a server-side route file, use the
validated server-side APP_URL environment variable instead of the public-facing
NEXT_PUBLIC_APP_URL to ensure the Stripe redirect URLs are properly constructed
even if the public environment variable is unset.
| const signatureParts = signature.split(",").reduce( | ||
| (acc, part) => { | ||
| const [key, value] = part.split("="); | ||
| acc[key] = value; | ||
| return acc; | ||
| }, | ||
| {} as Record<string, string>, | ||
| ); | ||
|
|
||
| const timestamp = signatureParts["t"]; | ||
| const expectedSignature = signatureParts["v1"]; | ||
|
|
||
| if (!timestamp || !expectedSignature) { | ||
| log.warn("Invalid Stripe signature format", { signature }); | ||
| return false; | ||
| } | ||
|
|
||
| const currentTimestamp = Math.floor(Date.now() / 1000); | ||
| if (Math.abs(currentTimestamp - parseInt(timestamp)) > 300) { |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Can Stripe-Signature include multiple v1 signatures during webhook secret rotation, and what is Stripe's recommended validation approach?
💡 Result:
Yes, the Stripe-Signature header can and will include multiple v1 signatures simultaneously during a webhook secret rotation [1][2]. When you roll a webhook signing secret in the Stripe Dashboard, you can choose to keep the previous secret active for a grace period (up to 24 hours) [1][3]. During this overlap window, Stripe signs every webhook event with both the old and the new secret, resulting in a header that contains multiple v1 signatures (e.g., t=...,v1=sig_old,v1=sig_new) [1][4][5]. Stripe's recommended validation approach is to use their official SDKs, which are designed to handle this scenario automatically [1][6]. Official Validation Recommendations: 1. Use Official Libraries: Always use the Stripe SDK's constructEvent or equivalent verification method [7][6][8]. These libraries are built to parse the header, identify all provided signatures, and verify the payload against your configured secret(s) [9][10]. 2. Support Multiple Secrets: If you are not using an SDK, or to ensure smooth rotation, your verification logic must be able to accept any valid signature provided in the header [4][11][2]. Specifically, you should: - Extract all signatures associated with the v1 scheme from the header [9][10]. - Attempt to verify the payload against each of your currently active signing secrets [11][8]. - Accept the event if at least one combination of secret and provided signature produces a match [9][4][11]. 3. Maintain Proper Rotation Workflow: - Add the new secret to your application environment alongside the existing one [5][8]. - Deploy your code (which must now be capable of verifying with either secret) [4][5][8]. - Trigger a test event in the Stripe Dashboard to confirm that your endpoint successfully verifies the signatures [3][5]. - Once verified, you can finalize the secret rotation in the Stripe Dashboard to expire the old secret, and subsequently remove it from your application configuration [5][8]. Never ignore the signature or attempt to implement custom HMAC verification if you can avoid it; the official SDKs handle constant-time comparisons and other security best practices that protect against common vulnerabilities like timing attacks [11][5][8].
Citations:
- 1: https://docs.stripe.com/webhooks?snapshot-or-thin=thin
- 2: https://quantlabusa.dev/tools/stripe-webhook-tester
- 3: https://github.com/SoapyRED/freighttools/blob/main/docs/runbooks/stripe-webhook.md
- 4: https://gethook.to/blog/zero-downtime-webhook-secret-rotation
- 5: https://www.hooklistener.com/learn/stripe-webhook-security-guide
- 6: https://docs.stripe.com/webhooks
- 7: https://docs.stripe.com/webhooks/signature
- 8: https://quantlabusa.dev/blog/stripe-webhook-security-best-practices
- 9: https://github.com/stripe/stripe-go/blob/master/webhooks.go
- 10: https://github.com/stripe/stripe-php/blob/master/lib/WebhookSignature.php
- 11: https://devtoys.pro/blog/hmac-webhook-signatures
🏁 Script executed:
cat -n lib/webhook-verification.ts | head -60Repository: arxja/saasify
Length of output: 2244
Support multiple v1 signatures and add strict timestamp validation for Stripe webhook verification.
The current implementation (lines 13-20) uses a reduce pattern that overwrites duplicate keys, discarding earlier v1 signatures. During Stripe secret rotation, webhooks are signed with both old and new secrets simultaneously (t=...,v1=sig_old,v1=sig_new), so the code must verify against all provided signatures, not just the last one.
Additionally, line 31 uses parseInt(timestamp) without validation. If timestamp is non-numeric (e.g., "abc"), parseInt returns NaN, and Math.abs(currentTimestamp - NaN) evaluates to NaN, which fails the > 300 check, allowing invalid timestamps to pass verification.
Extract all v1 signatures and validate timestamp format before parsing:
Suggested parsing hardening
-const signatureParts = signature.split(",").reduce(
- (acc, part) => {
- const [key, value] = part.split("=");
- acc[key] = value;
- return acc;
- },
- {} as Record<string, string>,
-);
-
-const timestamp = signatureParts["t"];
-const expectedSignature = signatureParts["v1"];
+const parts = signature.split(",").map((p) => p.trim());
+const timestamp = parts.find((p) => p.startsWith("t="))?.slice(2);
+const v1Signatures = parts
+ .filter((p) => p.startsWith("v1="))
+ .map((p) => p.slice(3));
-if (!timestamp || !expectedSignature) {
+if (!timestamp || v1Signatures.length === 0 || !/^\d+$/.test(timestamp)) {
log.warn("Invalid Stripe signature format", { signature });
return false;
}
const currentTimestamp = Math.floor(Date.now() / 1000);
-if (Math.abs(currentTimestamp - parseInt(timestamp)) > 300) {
+if (Math.abs(currentTimestamp - Number(timestamp)) > 300) {
...
}
-return crypto.timingSafeEqual(
- Buffer.from(hmac),
- Buffer.from(expectedSignature),
-);
+return v1Signatures.some((sig) =>
+ sig.length === hmac.length &&
+ crypto.timingSafeEqual(Buffer.from(hmac), Buffer.from(sig)),
+);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/webhook-verification.ts` around lines 13 - 31, The signature parsing
reduce function overwrites duplicate keys, so when Stripe provides multiple v1
signatures during secret rotation, only the last one is retained. Refactor the
reduce logic to collect all v1 signature values into an array instead of a
single string. Additionally, the parseInt(timestamp) call lacks validation,
allowing non-numeric timestamps to return NaN and bypass the timestamp check.
Add validation to ensure timestamp is a valid numeric string using
Number.isInteger or similar checks before parsing it, then update the timestamp
comparison logic to work with the validated numeric timestamp and ensure all
collected v1 signatures are verified against the expected signature.
| const expectedSignature = signatureParts["v1"]; | ||
|
|
||
| if (!timestamp || !expectedSignature) { | ||
| log.warn("Invalid Stripe signature format", { signature }); |
There was a problem hiding this comment.
Avoid logging raw webhook signatures.
Line 26 logs the full signature header value. Treat signature material as sensitive and redact/hash it before writing logs.
Suggested change
- log.warn("Invalid Stripe signature format", { signature });
+ log.warn("Invalid Stripe signature format", {
+ signaturePreview: signature.slice(0, 8),
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| log.warn("Invalid Stripe signature format", { signature }); | |
| log.warn("Invalid Stripe signature format", { | |
| signaturePreview: signature.slice(0, 8), | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/webhook-verification.ts` at line 26, The log.warn call in the webhook
verification code is logging the raw signature value, which is sensitive
security material and should not be exposed in logs. Modify the log.warn
statement to redact or hash the signature before logging it. Instead of passing
the signature parameter directly, compute a hash of the signature or show only a
truncated portion of it (e.g., first few characters followed by ellipsis) to aid
in debugging without exposing the full sensitive value.
| class HMACVerifier implements WebhookVerifier { | ||
| verify(rawBody: Buffer, signature: string, secret: string): boolean { | ||
| try { | ||
| const hmac = crypto | ||
| .createHmac("sha256", secret) | ||
| .update(rawBody) | ||
| .digest("hex"); | ||
|
|
||
| const expectedSignature = signature.startsWith("sha256=") | ||
| ? `sha256=${hmac}` | ||
| : hmac; | ||
|
|
||
| return crypto.timingSafeEqual( | ||
| Buffer.from(signature), | ||
| Buffer.from(expectedSignature), | ||
| ); | ||
| } catch (error) { | ||
| log.error("HMAC verification failed", error as Error); | ||
| return false; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Factory to get the right verifier | ||
| function getVerifier(provider: string): WebhookVerifier { | ||
| switch (provider.toLowerCase()) { | ||
| case "stripe": | ||
| return new StripeVerifier(); | ||
| case "github": | ||
| case "slack": | ||
| case "shopify": | ||
| return new HMACVerifier(); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is Slack's official request signature verification algorithm, including required headers and replay protection requirements?
💡 Result:
To verify that a request originated from Slack, your application must perform signature verification using your app's unique "Signing Secret," which is available in your Slack app management dashboard [1][2]. Required Headers The verification process requires two specific HTTP headers sent by Slack with every request [1][3]: 1. X-Slack-Request-Timestamp: A Unix timestamp (in seconds) indicating when the request was sent [1][3]. 2. X-Slack-Signature: The HMAC-SHA256 signature, formatted as v0=<hex_digest> [1][3]. Verification Algorithm 1. Replay Protection: Compare the X-Slack-Request-Timestamp header to your server's current time. If the difference is greater than 5 minutes, reject the request as stale to prevent replay attacks [4][5][3]. 2. Base String Construction: Concatenate the version number (currently v0), the timestamp, and the raw request body using a colon as a delimiter: v0:{timestamp}:{raw_request_body} [1][5][3]. 3. HMAC Calculation: Compute an HMAC-SHA256 hash of this base string using your Signing Secret as the key [1][6][3]. 4. Signature Comparison: Prepend the version prefix (v0=) to your calculated hex-encoded hash and compare it to the X-Slack-Signature header [1][5][3]. It is critical to use a timing-safe comparison function to prevent timing attacks [5][7][3]. Important Implementation Notes - Raw Body: You must use the exact raw request body (before any JSON parsing or modification) to generate the hash [7][3]. - Timing-Safe Comparison: Always use a constant-time comparison (e.g., crypto.timingSafeEqual in Node.js or hmac.compare_digest in Python) when checking the signature [4][5][7][3]. - Versioning: The current signature version is v0 [1][5]. Your implementation should check that the signature version in the header matches this expected version [5].
Citations:
- 1: https://medium.com/slack-developer-blog/securing-your-slack-apps-abd4d733b158
- 2: https://medium.com/slack-developer-blog/build-more-secure-apps-with-request-signing-mutual-tls-9e73f19cc9dd
- 3: https://hookdeck.com/webhooks/platforms/how-to-secure-and-verify-slack-webhooks-with-hookdeck
- 4: https://docs.slack.dev/tools/python-slack-sdk/reference/signature/index.html
- 5: https://github.com/slackapi/bolt-js/blob/main/src/receivers/verify-request.ts
- 6: https://github.com/slackapi/python-slack-sdk/blob/main/slack_sdk/signature/__init__.py
- 7: https://www.hooksbase.com/guides/receive-slack-events
🌐 Web query:
What is Shopify's official X-Shopify-Hmac-Sha256 verification format (digest encoding and comparison method)?
💡 Result:
To verify a Shopify webhook, you must compute the HMAC-SHA256 signature of the raw request body using your app's client secret as the key, then compare it to the signature provided in the X-Shopify-Hmac-Sha256 header [1][2]. Verification format and comparison details: - Digest Encoding: The HMAC-SHA256 digest must be encoded as a Base64 string [3][4][5]. Using Hex encoding, which is common for other services, will cause verification to fail [3][5]. - Input Data: Verification must be performed on the raw, unparsed request body [1][3]. Parsing the body into JSON (e.g., using express.json) before verification often modifies whitespace or key ordering, which alters the resulting hash and breaks verification [3][4]. - Comparison Method: The comparison should be performed using a constant-time comparison function (such as crypto.timingSafeEqual in Node.js or hmac.compare_digest in Python) to prevent timing attacks [1][3][5][2]. To implement this, ensure your middleware captures the raw request body (e.g., as a Buffer) before any body-parsing middleware runs [1][4][6]. If the computed Base64 digest does not match the value in the X-Shopify-Hmac-Sha256 header, the request should be rejected [1][2].
Citations:
- 1: https://shopify.dev/docs/apps/build/webhooks/verify-deliveries
- 2: https://kolachitech.com/shopify-webhook-verification/
- 3: https://www.hookbase.app/blog/shopify-webhook-signature-verification
- 4: https://hookray.com/blog/webhooks/shopify/express
- 5: https://webhookwhisper.com/webhook-errors/shopify-hmac-validation-failed
- 6: https://shopify-shopify-app-js.mintlify.app/api-reference/shopify-api/webhooks
🏁 Script executed:
# First, locate and examine the webhook verification file
find . -name "webhook-verification.ts" -type fRepository: arxja/saasify
Length of output: 87
🏁 Script executed:
# Check the full implementation of the file
if [ -f "lib/webhook-verification.ts" ]; then
cat -n lib/webhook-verification.ts
fiRepository: arxja/saasify
Length of output: 5063
🏁 Script executed:
# Search for any other webhook verifier implementations
fd -e ts -e js | xargs grep -l "VerifySlack\|SlackVerifier\|ShopifyVerifier" 2>/dev/null || echo "No provider-specific verifiers found"Repository: arxja/saasify
Length of output: 94
Do not use the generic HMAC verifier for Slack and Shopify.
Lines 86-88 route Slack and Shopify to HMACVerifier, but this verifier cannot satisfy either provider's requirements:
-
Slack: The verifier only hashes
rawBodydirectly, but Slack requires signing a base string ofv0:{timestamp}:{rawBody}and needs access to theX-Slack-Request-Timestampheader for mandatory replay protection (5-minute window). Valid Slack requests will be rejected with the current implementation. -
Shopify: The verifier uses hex encoding (
.digest("hex")), but Shopify requires Base64-encoded HMAC digests. Verification will always fail due to encoding mismatch.
Additionally, the WebhookVerifier interface only accepts a single signature string parameter and cannot pass request headers, making it impossible to implement provider-specific logic that requires header access (like Slack's timestamp).
Implement provider-specific verifiers (SlackVerifier and ShopifyVerifier) that can handle each provider's unique requirements, or update the interface to pass Headers instead of a single signature string.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/webhook-verification.ts` around lines 57 - 88, The getVerifier function
incorrectly routes Slack and Shopify to the generic HMACVerifier, which cannot
satisfy their distinct requirements. Slack requires signing the concatenated
string "v0:{timestamp}:{rawBody}" and accessing the X-Slack-Request-Timestamp
header for replay protection, while Shopify requires Base64-encoded (not hex)
HMAC digests. Create two new verifier classes, SlackVerifier and
ShopifyVerifier, that implement WebhookVerifier with their provider-specific
signing logic. To support header access, update the WebhookVerifier interface's
verify method signature to accept a headers parameter (or Headers object) in
addition to the existing parameters. Then update the case statements for "slack"
and "shopify" in the getVerifier function to return instances of the new
SlackVerifier and ShopifyVerifier respectively.
| export interface WebhookPayload<T = any> { | ||
| id: string; | ||
| type: string; | ||
| data: T; | ||
| createdAt: string; | ||
| provider: string; | ||
| signature: string; | ||
| idempotencyKey: string; | ||
| } | ||
|
|
||
| export interface WebhookJob { | ||
| id: string; | ||
| eventId: string; | ||
| provider: string; | ||
| eventType: string; | ||
| payload: any; | ||
| attempts: number; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify remaining explicit any usages in TS files
rg -nP --type=ts '\bany\b'Repository: arxja/saasify
Length of output: 1121
🏁 Script executed:
cat -n types/types.ts | head -40Repository: arxja/saasify
Length of output: 1101
🏁 Script executed:
rg -nA 5 'export interface WebhookPayload' types/types.tsRepository: arxja/saasify
Length of output: 198
🏁 Script executed:
rg -nA 15 'export interface WebhookJob' types/types.tsRepository: arxja/saasify
Length of output: 450
🏁 Script executed:
rg 'WebhookPayload|WebhookJob' --type=ts -g '!types/types.ts' -cRepository: arxja/saasify
Length of output: 39
🏁 Script executed:
rg 'WebhookPayload|WebhookJob' --type=ts --type=tsx --type=jsx --type=js -lRepository: arxja/saasify
Length of output: 85
🏁 Script executed:
cat -n tsconfig.json | head -30Repository: arxja/saasify
Length of output: 912
🏁 Script executed:
find . -name '.eslintrc*' -o -name 'eslint.config.*' | head -5Repository: arxja/saasify
Length of output: 379
🏁 Script executed:
rg 'WebhookPayload|WebhookJob' -t ts -t jsRepository: arxja/saasify
Length of output: 160
🏁 Script executed:
cat -n eslint.config.mjs | head -80Repository: arxja/saasify
Length of output: 648
🏁 Script executed:
rg '`@typescript-eslint`' eslint.config.mjsRepository: arxja/saasify
Length of output: 39
🏁 Script executed:
# Check if there's a rule against 'any' type
rg 'no-explicit-any|no-any' .Repository: arxja/saasify
Length of output: 39
🏁 Script executed:
# Check if TypeScript strict mode reports issues with explicit any
cat > /tmp/test.ts << 'EOF'
export interface Test<T = any> {
value: any;
}
EOF
cd /tmp && npx tsc --strict --noEmit test.ts 2>&1 || trueRepository: arxja/saasify
Length of output: 39
🏁 Script executed:
# Check package.json for scripts and dependencies
cat -n package.json | head -40Repository: arxja/saasify
Length of output: 1446
Use typed payload generics instead of any in webhook contracts.
Line 8 (WebhookPayload<T = any>) and Line 23 (payload: any) bypass type safety. Prefer unknown defaults and generic payload typing to force consumers to narrow types explicitly rather than disabling checks.
Suggested change
-export interface WebhookPayload<T = any> {
+export interface WebhookPayload<T = unknown> {
id: string;
type: string;
data: T;
createdAt: string;
provider: string;
signature: string;
idempotencyKey: string;
}
-export interface WebhookJob {
+export interface WebhookJob<TPayload = unknown> {
id: string;
eventId: string;
provider: string;
eventType: string;
- payload: any;
+ payload: TPayload;
attempts: number;
maxAttempts: number;
nextRetry?: Date;
status: "pending" | "processing" | "completed" | "failed" | "dead";
createdAt: Date;
updatedAt: Date;
error?: string;
}🧰 Tools
🪛 ESLint
[error] 8-8: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 23-23: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@types/types.ts` around lines 8 - 24, The WebhookPayload and WebhookJob
interfaces use the `any` type which bypasses type safety. In the WebhookPayload
interface, change the default type parameter from `any` to `unknown` to force
consumers to narrow types explicitly. In the WebhookJob interface, change the
`payload` field from `any` to use a generic type parameter (similar to
WebhookPayload's approach) or at least use `unknown` instead of `any` to
maintain type safety throughout the webhook contract definitions.
Source: Linters/SAST tools
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 30-43: The environment variables block is currently scoped only to
the build step, but the test:ci step runs without them. Since
lib/config/index.ts validates these keys and lib/stripe/stripe.ts reads
serverConfig.STRIPE_SECRET_KEY at import time, tests can fail before assertions
when these modules are imported. Move the entire env block containing
DATABASE_URL, JWT_SECRET, STRIPE_SECRET_KEY, and all other variables from under
the build step to the job level (as jobs.test.env) so both the test:ci and build
steps share access to the same environment variables.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: edfb084b-cfbd-4d28-9cce-467dd8b5777a
📒 Files selected for processing (3)
.github/workflows/ci.ymlapp/api/me/route.tslib/config/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- app/api/me/route.ts
- lib/config/index.ts
| - run: pnpm run test:ci | ||
| - run: pnpm run build | ||
| env: | ||
| DATABASE_URL: ${{ secrets.DATABASE_URL }} | ||
| JWT_SECRET: ${{ secrets.JWT_SECRET }} | ||
| STRIPE_SECRET_KEY: ${{ secrets.STRIPE_SECRET_KEY }} | ||
| STRIPE_WEBHOOK_SECRET: ${{ secrets.STRIPE_WEBHOOK_SECRET }} | ||
| APP_URL: ${{ secrets.APP_URL }} | ||
| NODE_ENV: "development" | ||
| LOG_LEVEL: "info" | ||
| NEXT_PUBLIC_STRIPE_CHECKOUT_URL: "https://checkout.stripe.com" | ||
| NEXT_PUBLIC_STRIPE_PUBLISHABLE_KEY: ${{ secrets.NEXT_PUBLIC_STRIPE_PUBLISHABLE_KEY }} | ||
| NEXT_PUBLIC_PRO_PRICE_ID: ${{ secrets.NEXT_PUBLIC_PRO_PRICE_ID }} | ||
| NEXT_PUBLIC_ENTERPRISE_PRICE_ID: ${{ secrets.NEXT_PUBLIC_ENTERPRISE_PRICE_ID }} |
There was a problem hiding this comment.
Scope required env vars at the test job level, not only the build step.
pnpm run test:ci (Line 30) runs without the env keys added on Lines 32-43. Given lib/config/index.ts validates these keys and lib/stripe/stripe.ts reads serverConfig.STRIPE_SECRET_KEY at import time, tests can fail before assertions when they touch config-dependent modules. Move this env block to jobs.test.env so both test and build share the same contract.
Suggested change
jobs:
test:
runs-on: ubuntu-latest
+ env:
+ DATABASE_URL: ${{ secrets.DATABASE_URL }}
+ JWT_SECRET: ${{ secrets.JWT_SECRET }}
+ STRIPE_SECRET_KEY: ${{ secrets.STRIPE_SECRET_KEY }}
+ STRIPE_WEBHOOK_SECRET: ${{ secrets.STRIPE_WEBHOOK_SECRET }}
+ APP_URL: ${{ secrets.APP_URL }}
+ NODE_ENV: "development"
+ LOG_LEVEL: "info"
+ NEXT_PUBLIC_STRIPE_CHECKOUT_URL: "https://checkout.stripe.com"
+ NEXT_PUBLIC_STRIPE_PUBLISHABLE_KEY: ${{ secrets.NEXT_PUBLIC_STRIPE_PUBLISHABLE_KEY }}
+ NEXT_PUBLIC_PRO_PRICE_ID: ${{ secrets.NEXT_PUBLIC_PRO_PRICE_ID }}
+ NEXT_PUBLIC_ENTERPRISE_PRICE_ID: ${{ secrets.NEXT_PUBLIC_ENTERPRISE_PRICE_ID }}
permissions:
contents: read
@@
- - run: pnpm run build
- env:
- DATABASE_URL: ${{ secrets.DATABASE_URL }}
- JWT_SECRET: ${{ secrets.JWT_SECRET }}
- STRIPE_SECRET_KEY: ${{ secrets.STRIPE_SECRET_KEY }}
- STRIPE_WEBHOOK_SECRET: ${{ secrets.STRIPE_WEBHOOK_SECRET }}
- APP_URL: ${{ secrets.APP_URL }}
- NODE_ENV: "development"
- LOG_LEVEL: "info"
- NEXT_PUBLIC_STRIPE_CHECKOUT_URL: "https://checkout.stripe.com"
- NEXT_PUBLIC_STRIPE_PUBLISHABLE_KEY: ${{ secrets.NEXT_PUBLIC_STRIPE_PUBLISHABLE_KEY }}
- NEXT_PUBLIC_PRO_PRICE_ID: ${{ secrets.NEXT_PUBLIC_PRO_PRICE_ID }}
- NEXT_PUBLIC_ENTERPRISE_PRICE_ID: ${{ secrets.NEXT_PUBLIC_ENTERPRISE_PRICE_ID }}
+ - run: pnpm run build🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yml around lines 30 - 43, The environment variables
block is currently scoped only to the build step, but the test:ci step runs
without them. Since lib/config/index.ts validates these keys and
lib/stripe/stripe.ts reads serverConfig.STRIPE_SECRET_KEY at import time, tests
can fail before assertions when these modules are imported. Move the entire env
block containing DATABASE_URL, JWT_SECRET, STRIPE_SECRET_KEY, and all other
variables from under the build step to the job level (as jobs.test.env) so both
the test:ci and build steps share access to the same environment variables.
1 participant
Summary by CodeRabbit