Conversation
🦋 Changeset detectedLatest commit: 2598ece The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a webhook subsystem: documentation, a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as Exactly API
participant DB as Database (sources)
participant Panda as Panda Publisher
participant Webhook as External Endpoint
participant Sentry
Client->>API: trigger event (e.g., transaction)
API->>DB: persist state/event
API->>Panda: invoke publish(payload[, receipt])
Panda->>DB: load org sources.config
Panda->>Panda: build JSON body & compute HMAC-SHA256(secret, body)
loop retry attempts (exponential backoff)
Panda->>Webhook: POST body with Signature header
alt 2xx response
Webhook-->>Panda: response (text/JSON)
Panda->>Panda: debug log response
else timeout or non-2xx
Webhook-->>Panda: error/timeout
Panda->>Sentry: captureException(error)
Note right of Panda: wait backoff then retry
end
end
Panda-->>API: publish completed (errors logged)
API-->>Client: reply to original request
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive webhook system, including a new API for managing webhook configurations, database schema updates, and a publishing mechanism integrated into the transaction lifecycle. The implementation features HMAC SHA256 signing for security, an exponential backoff retry policy, and extensive documentation. Review feedback identifies a critical validation error in the card status schema where the 'INACTIVE' state was omitted, as well as several documentation improvements including a broken markdown tag, a typo, and the need for clearer example URLs.
|
✅ All tests passed. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2d79b368-fdf5-44b0-be33-4c4d1d3a8448
📒 Files selected for processing (19)
.changeset/dry-peas-ring.md.changeset/fifty-friends-bet.md.changeset/long-moons-brake.md.changeset/loud-shoes-visit.md.changeset/olive-onions-tan.md.changeset/quick-ants-write.md.changeset/violet-plums-move.mdcspell.jsondocs/astro.config.tsdocs/src/content/docs/organization-authentication.mddocs/src/content/docs/webhooks.mdserver/api/index.tsserver/api/webhook.tsserver/database/schema.tsserver/hooks/panda.tsserver/test/api/webhook.test.tsserver/test/hooks/panda.test.tsserver/utils/auth.tsserver/utils/keeper.ts
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
server/hooks/panda.ts (1)
1322-1328:⚠️ Potential issue | 🟠 MajorThe outbound card webhook schema is narrower than the values you emit.
publish()mapsnotActivatedto"INACTIVE", and the inboundCardschema also allowsallTimeandperAuthorization, butWebhookonly acceptsACTIVE|FROZEN|DELETEDand four frequency values. Those legitimate Panda updates will failv.parse(Webhook, ...)and no webhook will be sent.🛠️ Proposed fix
limit: v.object({ amount: v.number(), - frequency: v.picklist(["per24HourPeriod", "per7DayPeriod", "per30DayPeriod", "perYearPeriod"]), + frequency: v.picklist([ + "per24HourPeriod", + "per7DayPeriod", + "per30DayPeriod", + "perYearPeriod", + "allTime", + "perAuthorization", + ]), }), - status: v.picklist(["ACTIVE", "FROZEN", "DELETED"]), + status: v.picklist(["ACTIVE", "FROZEN", "DELETED", "INACTIVE"]),Also applies to: 1439-1447
server/api/webhook.ts (1)
14-21:⚠️ Potential issue | 🟠 MajorValidate webhook URLs before storing them.
These fields accept arbitrary strings, and
server/hooks/panda.tslater passes them straight tofetch(). That makes it possible to persist malformed or internal destinations and turn webhook delivery into SSRF. Require a validhttps://URL and reject local/private hosts.Also applies to: 149-161
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 04b5f99d-29af-4493-b6f7-dafe9ae83cba
📒 Files selected for processing (16)
.changeset/dry-peas-ring.md.changeset/fifty-friends-bet.md.changeset/long-moons-brake.md.changeset/loud-shoes-visit.md.changeset/olive-onions-tan.md.changeset/quick-ants-write.mddocs/astro.config.tsdocs/src/content/docs/organization-authentication.mddocs/src/content/docs/webhooks.mdserver/api/index.tsserver/api/webhook.tsserver/hooks/panda.tsserver/test/api/webhook.test.tsserver/test/hooks/panda.test.tsserver/utils/auth.tsserver/utils/keeper.ts
nfmelendez
force-pushed
the
webhook
branch
2 times, most recently
from
a45122d to
149c02f
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (7)
docs/src/content/docs/organization-authentication.md (1)
163-209:⚠️ Potential issue | 🟡 MinorShow the organization prerequisite in this example.
This flow authenticates and then calls
GET /api/webhook/POST /api/webhook, but it never creates or selects an organization. Copied in isolation, it will hit403 { code: "no organization" }. Either add the organization step here or call out the prerequisite explicitly.server/utils/keeper.ts (1)
151-153:⚠️ Potential issue | 🟠 MajorDefer
onReceiptbefore wrapping it in a promise.
Promise.resolve(options?.onReceipt?.(receipt))evaluates the callback before the promise exists, so a synchronous throw escapes this.catch()and can still failexaSend()after the receipt was already obtained. Queue the callback first, then attach the error handler.🛠️ Proposed fix
- Promise.resolve(options?.onReceipt?.(receipt)).catch((error: unknown) => - captureException(error, { level: "error" }), - ); + void Promise.resolve() + .then(() => options?.onReceipt?.(receipt)) + .catch((error: unknown) => captureException(error, { level: "error" }));Run this to confirm the synchronous-throw behavior difference:
#!/bin/bash node <<'NODE' const cb = () => { throw new Error("sync throw"); }; try { Promise.resolve(cb()).catch(() => console.log("wrapped")); } catch (error) { console.log("escaped:", error.message); } Promise.resolve() .then(() => cb()) .catch((error) => console.log("deferred:", error.message)); NODEserver/test/hooks/panda.test.ts (1)
2806-2808: 🧹 Nitpick | 🔵 TrivialMatch the
debugmock by namespace instead of call order.This
mockReturnValueOnce()chain only works whileserver/hooks/panda.tscreates exactly two debug instances in the current order. Adding another namespace or reordering them will silently wirewebhookLoggerto the wrong logger.♻️ Suggested refactor
vi.mock("debug", () => { - const createDebug = vi.fn().mockReturnValueOnce(vi.fn()).mockReturnValueOnce(webhookLogger); + const createDebug = vi.fn().mockImplementation((namespace: string) => + namespace === "exa:webhook" ? webhookLogger : vi.fn(), + ); return { default: createDebug }; });Run this to inspect the current namespace/count assumption:
#!/bin/bash sed -n '70,75p' server/hooks/panda.ts sed -n '2804,2809p' server/test/hooks/panda.test.tsdocs/src/content/docs/webhooks.md (1)
393-423:⚠️ Potential issue | 🟠 MajorRemove
authorizedAmountfrom the force-capture payload.Force capture is the
completedflow without a prior authorization. KeepingauthorizedAmounthere documents the settlement/over-capture contract instead, so consumers may build the wrong parser.server/hooks/panda.ts (1)
1249-1257:⚠️ Potential issue | 🟠 MajorBound the retry window here.
delay: ({ count }) => 2^count * 500mswithretryCount: 20pushes the last retry out to ~72 hours and keeps a failed delivery alive for nearly a week. At the same time, plainfetch()network failures still skip retries because only"WebhookFailed"and"TimeoutError"are whitelisted. Cap the delay and include transient network errors if this path is meant to be resilient.server/api/webhook.ts (2)
14-21:⚠️ Potential issue | 🟠 MajorValidate webhook targets before persisting them.
These schemas accept any string, and
server/hooks/panda.tslater passes the stored value straight tofetch(). That allows webhook configs pointing at localhost, link-local, or other internal hosts. Parse the URL here and reject non-HTTPS/private targets before storing them.Also applies to: 149-160
175-186:⚠️ Potential issue | 🟠 MajorThis read-modify-write still races across instances.
The in-memory mutex only serializes one process. Two app instances can both read
sources.config, apply different mutations, and write whole-document updates that clobber each other. Move this into a DB transaction with row locking or an atomicjsonbupdate.Also applies to: 255-266
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d26e278a-58bf-48d3-9902-656e2f07f45c
📒 Files selected for processing (16)
.changeset/dry-peas-ring.md.changeset/fifty-friends-bet.md.changeset/long-moons-brake.md.changeset/loud-shoes-visit.md.changeset/olive-onions-tan.md.changeset/quick-ants-write.mddocs/astro.config.tsdocs/src/content/docs/organization-authentication.mddocs/src/content/docs/webhooks.mdserver/api/index.tsserver/api/webhook.tsserver/hooks/panda.tsserver/test/api/webhook.test.tsserver/test/hooks/panda.test.tsserver/utils/auth.tsserver/utils/keeper.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
server/hooks/panda.ts (2)
1250-1251:⚠️ Potential issue | 🟠 MajorCap the retry backoff.
With
retryCount: 20, this reaches262,144,000mson the last delay, which is roughly 72 hours. That makes failed deliveries linger for days and ties the retry window to process lifetime.🛠️ Suggested cap
- delay: ({ count }) => Math.trunc(1 << count) * 500, + delay: ({ count }) => Math.min(Math.trunc(1 << count) * 500, 60_000),
1442-1445:⚠️ Potential issue | 🟠 MajorAllow the full upstream limit frequency set.
Lines 176-183 accept
"allTime"and"perAuthorization"on inboundcard.updatedevents, but the outbound webhook schema rejects both here.v.parse(Webhook, ...)will fail and skip delivery for valid card limit updates.🛠️ Suggested fix
limit: v.object({ amount: v.number(), - frequency: v.picklist(["per24HourPeriod", "per7DayPeriod", "per30DayPeriod", "perYearPeriod"]), + frequency: v.picklist([ + "per24HourPeriod", + "per7DayPeriod", + "per30DayPeriod", + "perYearPeriod", + "allTime", + "perAuthorization", + ]), }),docs/src/content/docs/webhooks.md (3)
29-33:⚠️ Potential issue | 🟠 MajorUse the webhook
secretin the verification snippet.Outbound signatures are generated with the per-webhook
secret, not a generic API key. Copying this example as-is will fail against real deliveries.🛠️ Suggested correction
-const signature = createHmac("sha256", <YOUR_API_KEY>) +const signature = createHmac("sha256", "<YOUR_WEBHOOK_SECRET>")
393-420:⚠️ Potential issue | 🟠 MajorMake the force-capture example a true no-authorization settlement.
The force-capture path is the
completedflow with no prior authorized hold. KeepingauthorizedAmounthere models a settled authorization instead of a force capture.🛠️ Suggested correction
- "authorizedAmount": 10000,
641-653:⚠️ Potential issue | 🟠 MajorDocument
card.updatedas a general lifecycle event.
server/hooks/panda.tsforwards generic card updates, including non-wallet states likecanceled. Describing this as only digital-wallet provisioning will make consumers ignore valid events.🛠️ Suggested wording
-This webhook is currently triggered when a user adds their card to a digital wallet. +This webhook is sent whenever Exa receives a card lifecycle update, including wallet provisioning and status changes such as cancellation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 84db6815-a17c-450d-9e1c-a996d1ceb7db
📒 Files selected for processing (6)
.changeset/dry-peas-ring.md.changeset/fifty-friends-bet.mddocs/src/content/docs/webhooks.mdserver/hooks/panda.tsserver/test/api/webhook.test.tsserver/test/hooks/panda.test.ts
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
server/hooks/panda.ts (1)
1252-1254:⚠️ Potential issue | 🟠 MajorCap the backoff before one bad webhook hangs around for days.
With
retryCount: 20,Math.trunc(1 << count) * 500reaches262,144,000ms(~72h) on the last delay. That is long enough to keep this fire-and-forget delivery task alive for multiple days.⏱️ Minimal fix
{ - delay: ({ count }) => Math.trunc(1 << count) * 500, + delay: ({ count }) => Math.min(Math.trunc(1 << count) * 500, 60_000), retryCount: domain === "base-sepolia.exactly.app" ? 3 : 20, shouldRetry: ({ error }) => {server/api/webhook.ts (1)
29-34:⚠️ Potential issue | 🟠 MajorThis read-modify-write still loses webhook updates across instances.
The
Map<string, Mutex>only serializes requests handled by one Node process. Two instances can both read the samesources.config, apply different mutations, and the last write wins; if both see no row, one of the inserts can also fail on thesources.idprimary key. Use a database transaction with row locking or an atomicjsonbupdate instead.Also applies to: 206-224, 286-300
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 60b544ed-273f-4d14-a251-3274e2013603
📒 Files selected for processing (6)
server/api/webhook.tsserver/hooks/panda.tsserver/test/api/webhook.test.tsserver/test/hooks/panda.test.tsserver/test/utils/webhook.test.tsserver/utils/webhook.ts
3 participants
Summary by CodeRabbit
New Features
Security
Reliability
Documentation
Tests
Chores