Feat/standard webhooks - #61
Merged
Merged
Conversation
|
Warning Review limit reached
More reviews will be available in 37 minutes and 56 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds end-to-end webhook support: DB schema and relations, Ed25519 keypair generation, storage helpers, a signing+forwarding engine that records deliveries, HTTP handlers/routes (create/get/delete/send-test/public-key), auth enforcement for specific RPCs, integration into payment webhook flow, and a proto submodule pointer update. ChangesWebhook Endpoint Management and Event Forwarding
Proto Submodule Update
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
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: 6
🤖 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 `@src/routes/http/api/webhookEndpoints.ts`:
- Around line 19-21: The createEndpointSchema currently only does syntactic
validation; tighten it by requiring the URL use the https scheme and adding a
Zod refinement on createEndpointSchema that parses the URL (new URL(value)) and
rejects hostnames or IPs that are loopback (127.0.0.0/8, ::1), link-local
(169.254.0.0/16, fe80::/10), or private ranges (10.0.0.0/8, 172.16.0.0/12,
192.168.0.0/16, fc00::/7), and disallow plain "localhost"; and then implement a
runtime re-check in your forwarding code in forwardWebhook.ts (the code that
performs the server-side fetch) to perform DNS resolution
(dns.promises.resolve4/resolve6 or lookup) of the target host, verify each
resolved IP is not in those disallowed ranges before calling fetch, and
throw/reject if any resolved address is disallowed so SSRF attempts are blocked
server-side as well.
In `@src/routes/http/forwardWebhook.ts`:
- Around line 49-53: Replace the built-in Date usage with Luxon UTC APIs: import
{ DateTime } from 'luxon', compute the integer seconds with const timestamp =
Math.floor(DateTime.utc().toSeconds()), and generate the ISO timestamp with
DateTime.fromSeconds(timestamp).toUTC().toISO() (use this in the body where new
Date(timestamp * 1000).toISOString() was used). Update the body construction in
forwardWebhook (variables timestamp and body) to use these Luxon values.
In `@src/storage/db/postgres/helpers/webhookEndpoints.ts`:
- Around line 38-69: The current get -> conditional update/insert in the webhook
endpoint logic (using getWebhookEndpointByApiKeyId then db.update or db.insert
on webhookEndpointsTable) is non-atomic and can race on the unique apiKeyId;
change it to an atomic upsert by using Drizzle's insert ... on conflict do
update (e.g., db.insert(webhookEndpointsTable).values(...).onConflictDoUpdate({
target: webhookEndpointsTable.apiKeyId, set: { url, privateKey, publicKey,
updatedAt: new Date().toISOString() } }).returning()) so a single query handles
insert-or-update deterministically, validate inputs before calling the DB, and
still throw StorageError.emptyResult if the returning result is missing;
alternatively wrap the existing logic in a transaction and explicitly catch
unique-constraint violations when performing the insert and then retry the
update path.
- Line 47: Replace the built-in Date usage for updatedAt with Luxon's UTC
timestamp: import DateTime from Luxon (import { DateTime } from 'luxon') in
webhookEndpoints.ts and set updatedAt to DateTime.utc().toISO(); update any
related create/update blocks in the same module that use new Date() to use
DateTime.utc().toISO() to comply with the repo's UTC/Luxon requirement.
- Line 79: Replace the name-based error check with a typed instanceof check: in
the catch handling where you currently do `if (e instanceof Error && (e as
any).name === "StorageError")`, change it to `if (e instanceof StorageError) {
throw e; }` so the code uses the StorageError class directly (referencing the
StorageError type and the caught variable `e`) and remove the `(e as any).name`
usage.
In `@src/storage/db/postgres/schema.ts`:
- Around line 301-303: The foreign key for webhook_deliveries.endpointId (the
line using uuid("endpoint_id").references(() => webhookEndpointsTable.id)) needs
an explicit delete policy to avoid referential integrity errors when removing
endpoints; update that FK definition to include an onDelete behavior (for
example .onDelete("CASCADE")) so deleting a webhook endpoint either cascades to
its deliveries or uses your chosen policy, keeping the .references(() =>
webhookEndpointsTable.id) and .notNull() intact.
🪄 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 Plus
Run ID: 5dc98042-b65c-4f0f-99ee-c1916d1be754
📒 Files selected for processing (8)
protosrc/routes/http/api/registerApiRoutes.tssrc/routes/http/api/webhookEndpoints.tssrc/routes/http/createdCheckout.tssrc/routes/http/forwardWebhook.tssrc/storage/db/postgres/helpers/webhookEndpoints.tssrc/storage/db/postgres/schema.tssrc/utils/generateWebhookKeyPair.ts
SteakFisher
self-requested a review
May 28, 2026 20:40
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/utils/generateWebhookKeyPair.ts (1)
34-37: 💤 Low valueRemove unused
prefixedToPublicKeyPemfromsrc/utils/generateWebhookKeyPair.ts(lines 34-37).The function is defined but never referenced anywhere in the repo (only its declaration is found), so it’s dead/unreachable code and can be deleted (or wired into the intended verification path).
🤖 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 `@src/utils/generateWebhookKeyPair.ts` around lines 34 - 37, Remove the dead helper prefixedToPublicKeyPem by deleting its declaration from the module, or if intended to be used, wire it into the verification flow: locate the verification function(s) that accept a prefixed public key (e.g., any verifyWebhookSignature or similar routines in the generateWebhookKeyPair module) and replace inline base64-to-PEM logic with a call to prefixedToPublicKeyPem, or simply remove prefixedToPublicKeyPem if it is unused; ensure imports/exports are updated accordingly and run tests to confirm nothing references prefixedToPublicKeyPem.
🤖 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.
Nitpick comments:
In `@src/utils/generateWebhookKeyPair.ts`:
- Around line 34-37: Remove the dead helper prefixedToPublicKeyPem by deleting
its declaration from the module, or if intended to be used, wire it into the
verification flow: locate the verification function(s) that accept a prefixed
public key (e.g., any verifyWebhookSignature or similar routines in the
generateWebhookKeyPair module) and replace inline base64-to-PEM logic with a
call to prefixedToPublicKeyPem, or simply remove prefixedToPublicKeyPem if it is
unused; ensure imports/exports are updated accordingly and run tests to confirm
nothing references prefixedToPublicKeyPem.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4568906d-d893-442c-bbfe-0f13362cd5ef
📒 Files selected for processing (2)
src/storage/db/postgres/schema.tssrc/utils/generateWebhookKeyPair.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/storage/db/postgres/schema.ts
feat: add POST /webhook-endpoint/send-test for test API keys
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/routes/http/api/webhookEndpoints.ts (1)
22-36:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftSSRF surface remains only partially mitigated.
Production now enforces
https://, but there is still no protection against private/loopback/link-local targets or DNS-rebinding, and test mode accepts any valid URL (includinghttp://to internal hosts). This value is later used for a server-sidefetchinforwardWebhook.ts. Consider rejecting private/internal address ranges and re-validating the resolved IP at send time.🤖 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 `@src/routes/http/api/webhookEndpoints.ts` around lines 22 - 36, The current validation in webhookEndpoints.ts (mode variable and the z.object/url/refine chain) only enforces HTTPS and doesn't block private/loopback/link-local hosts or DNS-rebinding; update validation to reject IP literals in private ranges and common local hostnames (e.g., 127.0.0.1, ::1, localhost, 169.254.x.x, 10.x.x.x, 192.168.x.x, 172.16-31.x.x) by adding an extra refine that parses the URL host and fails for those patterns, and in forwardWebhook.ts re-validate at send time by resolving the hostname (DNS lookup) and checking the resolved IP(s) against the same private/CIDR lists before performing fetch to prevent DNS-rebinding; reference the schema in webhookEndpoints.ts and the send/fetch logic in forwardWebhook.ts when implementing these checks.
🧹 Nitpick comments (1)
src/routes/http/api/webhookEndpoints.ts (1)
21-21: 💤 Low valueAdd an explicit return type to
getCreateEndpointSchema.The two branches return different inferred Zod shapes; declare the contract explicitly.
♻️ Suggested signature
-function getCreateEndpointSchema(mode: "test" | "production" | null) { +function getCreateEndpointSchema( + mode: "test" | "production" | null +): z.ZodType<{ url: string }> {As per coding guidelines: "Always use explicit types for function parameters and return values".
🤖 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 `@src/routes/http/api/webhookEndpoints.ts` at line 21, The function getCreateEndpointSchema currently relies on inferred Zod return types from two different branches; add an explicit return type such as ZodTypeAny (imported from 'zod') to its signature — e.g. declare getCreateEndpointSchema(mode: "test" | "production" | null): ZodTypeAny — and ensure you import the type (import type { ZodTypeAny } from 'zod'); this makes the contract explicit and avoids mismatched inferred shapes between the branches (verify the returned zod schemas from both branches still satisfy the declared type).
🤖 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 `@src/interceptors/auth.ts`:
- Around line 79-98: The webhookEndpointCache TTL causes stale auth decisions
because checkWebhookEndpoint caches existence and upsert/delete flows never
invalidate it; update the upsertWebhookEndpoint and deleteWebhookEndpoint code
paths (or the HTTP handlers that call them) to call
webhookEndpointCache.delete(apiKeyId) after a successful upsert or delete so
checkWebhookEndpoint will re-query the DB, referencing the webhookEndpointCache
symbol and the checkWebhookEndpoint, upsertWebhookEndpoint,
deleteWebhookEndpoint functions (and if easier, add the same invalidation in the
RegisterEvent/StreamEvents/CreateCheckoutLink handlers after changes).
---
Duplicate comments:
In `@src/routes/http/api/webhookEndpoints.ts`:
- Around line 22-36: The current validation in webhookEndpoints.ts (mode
variable and the z.object/url/refine chain) only enforces HTTPS and doesn't
block private/loopback/link-local hosts or DNS-rebinding; update validation to
reject IP literals in private ranges and common local hostnames (e.g.,
127.0.0.1, ::1, localhost, 169.254.x.x, 10.x.x.x, 192.168.x.x, 172.16-31.x.x) by
adding an extra refine that parses the URL host and fails for those patterns,
and in forwardWebhook.ts re-validate at send time by resolving the hostname (DNS
lookup) and checking the resolved IP(s) against the same private/CIDR lists
before performing fetch to prevent DNS-rebinding; reference the schema in
webhookEndpoints.ts and the send/fetch logic in forwardWebhook.ts when
implementing these checks.
---
Nitpick comments:
In `@src/routes/http/api/webhookEndpoints.ts`:
- Line 21: The function getCreateEndpointSchema currently relies on inferred Zod
return types from two different branches; add an explicit return type such as
ZodTypeAny (imported from 'zod') to its signature — e.g. declare
getCreateEndpointSchema(mode: "test" | "production" | null): ZodTypeAny — and
ensure you import the type (import type { ZodTypeAny } from 'zod'); this makes
the contract explicit and avoids mismatched inferred shapes between the branches
(verify the returned zod schemas from both branches still satisfy the declared
type).
🪄 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 Plus
Run ID: aad1ed74-d34f-459b-be5e-23fd01458a5e
📒 Files selected for processing (5)
src/interceptors/auth.tssrc/routes/http/api/registerApiRoutes.tssrc/routes/http/api/webhookEndpoints.tssrc/routes/http/forwardWebhook.tssrc/storage/db/postgres/helpers/webhookEndpoints.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/routes/http/api/registerApiRoutes.ts
- src/routes/http/forwardWebhook.ts
This was referenced May 30, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit