✨ server: encrypted kyc - #948
Conversation
🦋 Changeset detectedLatest commit: f57da90 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 KYC application workflow: new authenticated POST/PATCH/GET /application endpoints with SIWE verification and payload hashing (encrypted/plaintext), Panda submit/get/update wrappers and schemas, org role and ACL updates, DB column for organization role, docs for encrypted KYC payload, dependency add, and expanded tests/mocks. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as Client
participant KYC as KYC API
participant DB as Database
participant Auth as Auth Service
participant Panda as Panda Service
Client->>KYC: POST /application (SIWE + payload {ciphertext|JSON})
KYC->>KYC: Parse SIWE, verify signature & chainId
KYC->>KYC: Compute sha256(ciphertext or canonicalized JSON)
KYC->>DB: Read credential & org membership
KYC->>Auth: Verify org role includes kyc:create
KYC->>Panda: submitApplication(payload, encrypted?)
Panda-->>KYC: { id, applicationStatus }
KYC->>DB: Persist credentials.pandaId & source
KYC-->>Client: { status: applicationStatus }
Client->>KYC: GET /application
KYC->>DB: Read credentials.pandaId
KYC->>Panda: getApplicationStatus(pandaId)
Panda-->>KYC: { id, applicationStatus, reason? }
KYC-->>Client: { code:"ok", status, reason }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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)
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 |
There was a problem hiding this comment.
Code Review
This pull request implements a new KYC application submission and management flow, including support for encrypted payloads and organization-based permissions. The review identified potential issues with the current organization membership check, the SIWE message parsing, and the payload hashing logic. I have provided suggestions to improve the robustness of the membership lookup, handle potential parsing errors, and correct the hashing implementation.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #948 +/- ##
==========================================
+ Coverage 72.81% 73.21% +0.40%
==========================================
Files 241 241
Lines 10013 10245 +232
Branches 3265 3362 +97
==========================================
+ Hits 7291 7501 +210
- Misses 2429 2440 +11
- Partials 293 304 +11
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/utils/auth.ts (1)
20-64:⚠️ Potential issue | 🟡 MinorRemove unused
deleteandreadkyc actions or implement permission enforcement for them.Lines 20 define
kyc: ["create", "delete", "read"], but onlycreateis granted toadmin/ownerroles (lines 58, 63), and no KYC endpoints enforce these actions viahasPermission(). The global action definitions are unused dead code—either removedeleteandreadto match the intended role permissions, or add permission checks to enforce them in KYC endpoints (currently they bypass the access control system).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 12eeea5f-41ad-44e3-98ac-07be8bf882b0
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (15)
.changeset/cuddly-streets-like.md.changeset/four-numbers-worry.md.changeset/sharp-squids-push.md.changeset/upset-seas-sink.mdcspell.jsondocs/src/content/docs/organization-authentication.mdserver/api/card.tsserver/api/kyc.tsserver/database/schema.tsserver/package.jsonserver/test/api/card.test.tsserver/test/api/kyc.test.tsserver/test/e2e.tsserver/utils/auth.tsserver/utils/panda.ts
125a498 to
aeaa4ee
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
server/test/api/kyc.test.ts (1)
1440-1440:⚠️ Potential issue | 🟠 MajorThese test signatures still hash the wrong bytes.
This is the same canonicalize bug flagged earlier:
canonicalize()already returns the canonical JSON string, so wrapping it inJSON.stringify()changes the bytes being signed. The plaintext application tests no longer match the server-side hash computation.💡 Proposed fix
- sha256(Buffer.from(JSON.stringify(canonicalize(applicationPayload)), "utf8")) + sha256(Buffer.from(canonicalize(applicationPayload) ?? "", "utf8"))Also applies to: 1504-1504, 1557-1557, 1739-1739, 1776-1776, 1813-1813, 1842-1842
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4b887372-b7ce-4860-af1c-ac3e93fea439
📒 Files selected for processing (10)
.changeset/cuddly-streets-like.md.changeset/sharp-squids-push.md.changeset/upset-seas-sink.mdcspell.jsondocs/src/content/docs/organization-authentication.mdserver/api/kyc.tsserver/database/schema.tsserver/test/api/kyc.test.tsserver/utils/auth.tsserver/utils/panda.ts
aeaa4ee to
7fafac5
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c1ad266c-f3b0-4847-9461-6c6e351826af
📒 Files selected for processing (10)
.changeset/cuddly-streets-like.md.changeset/sharp-squids-push.md.changeset/upset-seas-sink.mdcspell.jsondocs/src/content/docs/organization-authentication.mdserver/api/kyc.tsserver/database/schema.tsserver/test/api/kyc.test.tsserver/utils/auth.tsserver/utils/panda.ts
7fafac5 to
9058e28
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
server/api/card.ts (1)
334-338:⚠️ Potential issue | 🟠 MajorMove the KYC lookup behind the local
already createdbranch.This preflight now runs before we decide whether the user already has an active/frozen card, so duplicate
POST /cardcalls can regress from400 { code: "already created" }to403/500when Panda is denied, stale, or unavailable. PuttinggetApplicationStatus(...)inside the existingtryafterif (cardCount > 0)keeps the endpoint idempotent and reuses the currentnoUser(...)mapping for stalepandaIds.♻️ Proposed fix
- const kyc = await getApplicationStatus(credential.pandaId); - if (kyc.applicationStatus !== "approved") { - return c.json({ code: "kyc not approved" }, 403); - } - let isUpgradeFromPlatinum = credential.cards.some( ({ status, productId }) => status === "DELETED" && productId === PLATINUM_PRODUCT_ID, ); @@ } if (cardCount > 0) return c.json({ code: "already created" }, 400); try { + const kyc = await getApplicationStatus(credential.pandaId); + if (kyc.applicationStatus !== "approved") return c.json({ code: "kyc not approved" }, 403); const card = await createCard(credential.pandaId, SIGNATURE_PRODUCT_ID);server/utils/panda.ts (1)
425-431:⚠️ Potential issue | 🟡 Minor
new Date(...)still accepts impossible calendar dates.Values like
2026-02-31pass this check because JavaScript normalizes overflowed dates instead of rejecting them. Round-trip the parsed year/month/day withDate.UTC(...)so only real calendar dates reach Panda.🛠️ Proposed fix
birthDate: pipe( string(), regex(/^\d{4}-\d{2}-\d{2}$/, "must be YYYY-MM-DD format"), check((value) => { - const date = new Date(value); - return !Number.isNaN(date.getTime()); + const [year, month, day] = value.split("-").map(Number); + if (year === undefined || month === undefined || day === undefined) return false; + const date = new Date(Date.UTC(year, month - 1, day)); + return date.getUTCFullYear() === year && date.getUTCMonth() === month - 1 && date.getUTCDate() === day; }, "must be a valid date"),Does JavaScript `new Date("2026-02-31")` normalize overflowed ISO dates instead of returning `Invalid Date`?
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1fb0a294-b798-4017-b6f4-d185461cd025
📒 Files selected for processing (15)
.changeset/calm-tigers-stop.md.changeset/cuddly-streets-like.md.changeset/four-numbers-worry.md.changeset/sharp-squids-push.md.changeset/upset-seas-sink.mdcspell.jsondocs/src/content/docs/organization-authentication.mdserver/api/card.tsserver/api/kyc.tsserver/database/schema.tsserver/test/api/card.test.tsserver/test/api/kyc.test.tsserver/test/e2e.tsserver/utils/auth.tsserver/utils/panda.ts
9058e28 to
98f2903
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
docs/src/content/docs/organization-authentication.md (1)
226-327:⚠️ Potential issue | 🟡 MinorAdd the actual
/applicationsubmit example withencrypted: "true"header.The sample currently stops at payload logging (Line 323). For encrypted KYC, readers also need the submission contract; otherwise this is easy to copy into a failing integration.
📄 Suggested doc patch
owner.signMessage({ message }) - .then((signature) => { + .then(async (signature) => { const verify = { message, signature, walletAddress: owner.address, chainId, }; const { hash, ...payload } = encryptedPayload; - console.log("application payload", { ...payload, verify }); + const applicationPayload = { ...payload, verify }; + console.log("application payload", applicationPayload); + + const response = await fetch("https://sandbox.exactly.app/api/application", { + method: "POST", + headers: { "content-type": "application/json", encrypted: "true" }, + body: JSON.stringify(applicationPayload), + }); + console.log("application submit status", response.status); })
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8500d69f-ac57-471b-95c2-d4e1e46f3edb
📒 Files selected for processing (11)
.changeset/calm-tigers-stop.md.changeset/cuddly-streets-like.md.changeset/sharp-squids-push.md.changeset/upset-seas-sink.mdcspell.jsondocs/src/content/docs/organization-authentication.mdserver/api/kyc.tsserver/database/schema.tsserver/test/api/kyc.test.tsserver/utils/auth.tsserver/utils/panda.ts
8c026bc to
fc57eea
Compare
98f2903 to
de72eb9
Compare
c985bf7 to
43dc358
Compare
b095e06 to
987fa1c
Compare
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
14b6c44 to
d7fa395
Compare
295a971 to
6842c4b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f57da9051f
ℹ️ 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 account = await database.query.walletAddresses.findFirst({ | ||
| where: eq(walletAddresses.address, address), |
There was a problem hiding this comment.
Use the request chain context when resolving the signing wallet. The new KYC submit flow queries wallet_addresses with findFirst on address only, even though the table stores chainId and the payload includes verify.chainId; if the same address exists in multiple rows (e.g., different chain/user records), this can select the wrong membership arbitrarily and either deny valid submissions or authorize against the wrong organization membership.
Useful? React with 👍 / 👎.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores