Conversation
🦋 Changeset detectedLatest commit: 7672e26 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new webhook handler for the 'Bridge' service, enabling the server to process various events such as customer status updates, fund receipts, and liquidation address drains. It includes robust signature validation for security, integrates with notification and analytics platforms, and standardizes the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
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 new Bridge webhook POST endpoint with signed-payload validation, credential lookup, user context resolution, push notifications, and analytics tracking; updates Onramp event property Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Bridge (Webhook Sender)
participant Server as Bridge Hook Handler
participant DB as Database
participant Sentry as Sentry
participant OneSignal as Push Notification Service
participant Segment as Analytics Service
Client->>Server: POST webhook payload + signature header
Server->>Server: Validate signature (RSA‑SHA256)
alt Signature invalid
Server-->>Client: 401 { code: "unauthorized" }
else Signature valid
Server->>Server: Validate payload schema
alt Payload invalid
Server-->>Client: 200 { code: "bad bridge" }
else Payload valid
Server->>DB: Lookup credential by bridgeId
alt Credential not found
Server->>Sentry: Capture context
Server-->>Client: 200 { code: "credential not found" }
else Credential found
Server->>Server: Establish user context
alt Event = payment_submitted or drain.payment_submitted
Server->>OneSignal: Send deposit notification
Server->>Segment: Track Onramp event (amount, currency, source, usdcAmount)
Server-->>Client: 200 { code: "ok" }
else Event = customer.updated.status_transitioned
alt status = "active"
Server->>Segment: Track RampAccount event
Server->>OneSignal: Send fiat onramp activation notification
Server-->>Client: 200 { code: "ok" }
else
Server-->>Client: 200 { code: "ok" }
end
else
Server-->>Client: 200 { code: "ok" }
end
Note over OneSignal,Segment: Errors captured in Sentry but do not block response
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
mainqueg
force-pushed
the
bridge-hook
branch
2 times, most recently
from
468bb15 to
d67d97d
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #861 +/- ##
==========================================
+ Coverage 71.13% 71.94% +0.81%
==========================================
Files 211 212 +1
Lines 8349 8530 +181
Branches 2727 2813 +86
==========================================
+ Hits 5939 6137 +198
+ Misses 2132 2123 -9
+ Partials 278 270 -8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
server/hooks/bridge.ts (1)
125-177:⚠️ Potential issue | 🟠 MajorGuard side effects with webhook idempotency.
track()andsendPushNotification()run on every delivery in this block. Retries/duplicates will emit repeated analytics and notifications unless you dedupe on a stable event key before side effects.server/test/hooks/bridge.test.ts (1)
25-36:⚠️ Potential issue | 🟠 MajorAdd teardown for inserted credential fixture.
The suite inserts persistent DB state in
beforeAllbut never deletes it. This can leak state across reruns and make the suite flaky.Suggested cleanup
-import { afterEach, beforeAll, describe, expect, inject, it, vi } from "vitest"; +import { eq } from "drizzle-orm"; +import { afterAll, afterEach, beforeAll, describe, expect, inject, it, vi } from "vitest"; @@ beforeAll(async () => { await database.insert(credentials).values([ { id: "bridge-test", publicKey: new Uint8Array(hexToBytes(owner)), account, factory, pandaId: "bridgePandaId", bridgeId: "bridgeCustomerId", }, ]); }); + + afterAll(async () => { + await database.delete(credentials).where(eq(credentials.id, "bridge-test")); + });
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1f375179-b8c8-43dc-9548-2da431b5b274
📒 Files selected for processing (9)
.changeset/thin-pumas-brush.mdserver/hooks/bridge.tsserver/hooks/manteca.tsserver/index.tsserver/test/hooks/bridge.test.tsserver/test/hooks/manteca.test.tsserver/utils/ramps/bridge.tsserver/utils/segment.tsserver/vitest.config.mts
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
mainqueg
force-pushed
the
bridge-hook
branch
2 times, most recently
from
333ef5d to
c8ea3f5
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| const digest = createHash("sha256").update(`${timestamp}.${body}`).digest(); | ||
| const verifier = createVerify("RSA-SHA256"); | ||
| verifier.update(digest); |
There was a problem hiding this comment.
Bug: The Bridge webhook handler uses RSA-SHA256 for signature verification, but the Bridge API requires HMAC-SHA256. This will cause all legitimate webhooks to fail verification.
Severity: CRITICAL
Suggested Fix
Replace the RSA-SHA256 verification logic with HMAC-SHA256. Use crypto.createHmac("sha256", webhookSecret) to generate a signature from the request body and timestamp. Compare this generated signature against the one provided in the bridge-signature header. Update the environment variable from BRIDGE_WEBHOOK_PUBLIC_KEY to a shared secret provided by Bridge.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: server/hooks/bridge.ts#L193-L195
Potential issue: The Bridge webhook signature verification is incorrectly implemented
using `RSA-SHA256` with a public key, as seen with `createVerify("RSA-SHA256")`.
According to Bridge's API documentation, webhooks must be verified using `HMAC-SHA256`
with a shared secret. This mismatch will cause the verification to fail for all incoming
webhooks from Bridge, resulting in a `401 Unauthorized` response. Consequently, critical
functionalities like customer activation, onramp transaction tracking, and deposit
notifications will be completely broken. The implementation appears to be an incorrect
adaptation from a Stripe webhook handler.
Did we get this right? 👍 / 👎 to inform future reviews.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| if (!match) return c.json({ code: "unauthorized" }, 401); | ||
| const [, timestamp, base64Signature] = match; | ||
| if (!timestamp || !base64Signature) return c.json({ code: "unauthorized" }, 401); | ||
| if (Math.abs(Date.now() - Number(timestamp)) > 600_000) return c.json({ code: "unauthorized" }, 401); |
There was a problem hiding this comment.
🚩 Timestamp validation assumes millisecond precision
The timestamp comparison at line 191 uses Math.abs(Date.now() - Number(timestamp)) > 600_000, treating the bridge timestamp as milliseconds (matching Date.now()). The test at server/test/hooks/bridge.test.ts:83 confirms this assumption by using Date.now() - 600_001 as an expired timestamp. If Bridge's webhook actually sends timestamps in Unix seconds (not milliseconds), the 10-minute window (600_000 ms) would be compared against a value ~1000x larger, causing every request to be rejected. This depends on Bridge's actual API behavior which should be verified against their documentation.
Was this helpful? React with 👍 or 👎 to provide feedback.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 16b4784071
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const bridgeId = | ||
| payload.event_type === "customer.updated.status_transitioned" | ||
| ? payload.event_object.id | ||
| : payload.event_object.customer_id; | ||
| const credential = await database.query.credentials.findFirst({ | ||
| columns: { account: true, source: true }, | ||
| where: eq(credentials.bridgeId, bridgeId), | ||
| }); |
There was a problem hiding this comment.
The handler fetches credentials before checking subtype/state-specific no-op paths, so events you intentionally ignore (for example virtual_account.activity.created with type !== "payment_submitted" or drain transitions not in payment_submitted) still hit the database and can emit credential not found errors for unknown customer_ids. In production webhook traffic where Bridge sends many non-actionable transitions, this creates avoidable DB load and noisy Sentry errors even though the final behavior is just 200 { code: "ok" }.
Useful? React with 👍 / 👎.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| | { | ||
| event: "Onramp"; | ||
| properties: { | ||
| amount: number; |
There was a problem hiding this comment.
🚩 fiatAmount → amount rename is complete and consistent
The property rename from fiatAmount to amount in the Segment Onramp event type (server/utils/segment.ts:45) is consistently applied in the only two callers: server/hooks/manteca.ts:204 and server/hooks/bridge.ts:143,164. The test assertions in server/test/hooks/manteca.test.ts:349 and server/test/hooks/bridge.test.ts:149,290 also use the new amount name. This is a breaking change for any downstream Segment consumers (dashboards, analytics pipelines) that reference the old fiatAmount property name.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
closes #436
Summary by CodeRabbit
New Features
Tests
Chores