✨ server: process business onboarding approvals - #1209
Conversation
🦋 Changeset detectedLatest commit: 146f4d1 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 |
WalkthroughThe server adds business approval finalization, Panda company webhook handling, and credential-derived subtenant routing. Card, KYC, Persona, and Panda tests update their coverage for business credentials, card provisioning, verification, concurrency, and forwarded webhook requests. ChangesBusiness onboarding and card tenancy
Estimated code review effort: 4 (Complex) | ~60 minutes Mergeability Score: 🟠 High · up to The change can race during approval finalization, skip issuing a business card, or turn malformed or non-company webhook events into repeated 500 responses instead of safely acknowledging or falling back. Because these paths affect card provisioning and webhook authentication reliability, the PR is not merge-ready until the identified correctness and availability issues are addressed. Sequence Diagram(s)sequenceDiagram
participant PandaWebhook
participant PandaUtils
participant Database
PandaWebhook->>PandaUtils: process approved company application
PandaUtils->>Database: resolve business credential and cards
PandaUtils->>PandaUtils: finalizeBusinessApproval
PandaUtils->>Database: persist business user and card
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
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 |
5e369b4 to
c8abea1
Compare
14fbd8c to
9e408b8
Compare
9e408b8 to
146f4d1
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3c48c2d8-ea42-4d42-ae0d-110eb98c518e
📒 Files selected for processing (10)
.changeset/brown-heads-vanish.mdserver/api/card.tsserver/api/kyc.tsserver/hooks/panda.tsserver/hooks/persona.tsserver/test/api/card.test.tsserver/test/api/kyc.test.tsserver/test/hooks/panda.test.tsserver/test/hooks/persona.test.tsserver/utils/panda.ts
| if (application.applicationStatus === "approved") | ||
| await finalizeBusinessApproval(credentialId, application.id, account); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Run this finalization under the same account mutex as the webhook path.
server/hooks/panda.ts (Line 294) wraps finalizeBusinessApproval in (getMutex(account) ?? createMutex(account)).runExclusive(...). This call site does not. A company.updated webhook and this request can therefore finalize the same credential at the same time. Both runs can pass the pandaId resolution and the local active-card check before either writes, which produces two provider card requests and relies only on the Panda idempotency key for deduplication.
Also note that an error from finalizeBusinessApproval now propagates and replaces the approved application response, even though pandaCompanyId is already persisted.
🛡️ Proposed fix
if (application.applicationStatus === "approved")
- await finalizeBusinessApproval(credentialId, application.id, account);
+ await (getMutex(account) ?? createMutex(account)).runExclusive(() =>
+ finalizeBusinessApproval(credentialId, application.id, account),
+ );Import createMutex and getMutex from ../utils/panda.
📝 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.
| if (application.applicationStatus === "approved") | |
| await finalizeBusinessApproval(credentialId, application.id, account); | |
| if (application.applicationStatus === "approved") | |
| await (getMutex(account) ?? createMutex(account)).runExclusive(() => | |
| finalizeBusinessApproval(credentialId, application.id, account), | |
| ); |
| if (payload.resource === "company" || payload.resource === "application") { | ||
| if ( | ||
| payload.resource === "application" && | ||
| (await getCompanyApplicationStatus(payload.body.id).then(({ applicationStatus }) => applicationStatus)) !== | ||
| "approved" | ||
| ) | ||
| return c.json({ code: "ok" }); | ||
| if (payload.resource === "company" && payload.body.applicationStatus !== "approved") | ||
| return c.json({ code: "ok" }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
An application webhook for a non-company application produces repeated 500 responses.
The application variant accepts any action and any body.id. The handler then calls getCompanyApplicationStatus(payload.body.id), which requests /issuing/applications/company/{id}. For an individual user application id, Panda answers 404, request throws ServiceError, and the webhook returns 500. Panda then retries the same event, and every retry repeats the failing lookup.
The same call also invokes requireSubtenant() through the default in getCompanyApplicationStatus, so any deployment without PANDA_SUBTENANT_ID fails every application event with 500.
Treat a 404 as "not a company application" and acknowledge the event.
🛡️ Proposed fix
if (
payload.resource === "application" &&
- (await getCompanyApplicationStatus(payload.body.id).then(({ applicationStatus }) => applicationStatus)) !==
- "approved"
+ (await getCompanyApplicationStatus(payload.body.id)
+ .then(({ applicationStatus }) => applicationStatus)
+ .catch((error: unknown) => {
+ if (error instanceof ServiceError && error.status === 404) return undefined;
+ throw error;
+ })) !== "approved"
)
return c.json({ code: "ok" });📝 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.
| if (payload.resource === "company" || payload.resource === "application") { | |
| if ( | |
| payload.resource === "application" && | |
| (await getCompanyApplicationStatus(payload.body.id).then(({ applicationStatus }) => applicationStatus)) !== | |
| "approved" | |
| ) | |
| return c.json({ code: "ok" }); | |
| if (payload.resource === "company" && payload.body.applicationStatus !== "approved") | |
| return c.json({ code: "ok" }); | |
| if (payload.resource === "company" || payload.resource === "application") { | |
| if ( | |
| payload.resource === "application" && | |
| (await getCompanyApplicationStatus(payload.body.id) | |
| .then(({ applicationStatus }) => applicationStatus) | |
| .catch((error: unknown) => { | |
| if (error instanceof ServiceError && error.status === 404) return undefined; | |
| throw error; | |
| })) !== "approved" | |
| ) | |
| return c.json({ code: "ok" }); | |
| if (payload.resource === "company" && payload.body.applicationStatus !== "approved") | |
| return c.json({ code: "ok" }); |
| it("uses the company application status for business card provisioning", async () => { | ||
| const getApplicationStatus = vi.spyOn(panda, "getApplicationStatus"); | ||
| const getCompanyApplicationStatus = vi | ||
| .spyOn(panda, "getCompanyApplicationStatus") | ||
| .mockResolvedValueOnce({ id: "card-business-company", applicationStatus: "approved" }); | ||
| vi.spyOn(panda, "createCard").mockResolvedValueOnce({ | ||
| ...cardTemplate, | ||
| id: "00000000-0000-4000-8000-0000000000ab", | ||
| }); | ||
|
|
||
| const response = await appClient.index.$post({ header: { "test-credential-id": "card-business" } }); | ||
|
|
||
| expect(response.status).toBe(200); | ||
| expect(getCompanyApplicationStatus).toHaveBeenCalledExactlyOnceWith("card-business-company"); | ||
| expect(getApplicationStatus).not.toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Assert the subtenant propagation in the business provisioning test.
The test verifies the company status lookup, but it does not verify that createCard receives the derived subtenant. This is the core new behavior for business credentials, and the salt of card-business resolves to "subtenant".
💚 Proposed addition
- vi.spyOn(panda, "createCard").mockResolvedValueOnce({
+ const createCard = vi.spyOn(panda, "createCard").mockResolvedValueOnce({
...cardTemplate,
id: "00000000-0000-4000-8000-0000000000ab",
}); expect(getApplicationStatus).not.toHaveBeenCalled();
+ expect(createCard).toHaveBeenCalledWith("card-business-user", SIGNATURE_PRODUCT_ID, {
+ amount: undefined,
+ subtenantId: "subtenant",
+ });📝 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.
| it("uses the company application status for business card provisioning", async () => { | |
| const getApplicationStatus = vi.spyOn(panda, "getApplicationStatus"); | |
| const getCompanyApplicationStatus = vi | |
| .spyOn(panda, "getCompanyApplicationStatus") | |
| .mockResolvedValueOnce({ id: "card-business-company", applicationStatus: "approved" }); | |
| vi.spyOn(panda, "createCard").mockResolvedValueOnce({ | |
| ...cardTemplate, | |
| id: "00000000-0000-4000-8000-0000000000ab", | |
| }); | |
| const response = await appClient.index.$post({ header: { "test-credential-id": "card-business" } }); | |
| expect(response.status).toBe(200); | |
| expect(getCompanyApplicationStatus).toHaveBeenCalledExactlyOnceWith("card-business-company"); | |
| expect(getApplicationStatus).not.toHaveBeenCalled(); | |
| }); | |
| it("uses the company application status for business card provisioning", async () => { | |
| const getApplicationStatus = vi.spyOn(panda, "getApplicationStatus"); | |
| const getCompanyApplicationStatus = vi | |
| .spyOn(panda, "getCompanyApplicationStatus") | |
| .mockResolvedValueOnce({ id: "card-business-company", applicationStatus: "approved" }); | |
| const createCard = vi.spyOn(panda, "createCard").mockResolvedValueOnce({ | |
| ...cardTemplate, | |
| id: "00000000-0000-4000-8000-0000000000ab", | |
| }); | |
| const response = await appClient.index.$post({ header: { "test-credential-id": "card-business" } }); | |
| expect(response.status).toBe(200); | |
| expect(getCompanyApplicationStatus).toHaveBeenCalledExactlyOnceWith("card-business-company"); | |
| expect(getApplicationStatus).not.toHaveBeenCalled(); | |
| expect(createCard).toHaveBeenCalledWith("card-business-user", SIGNATURE_PRODUCT_ID, { | |
| amount: undefined, | |
| subtenantId: "subtenant", | |
| }); | |
| }); |
| const previousSubtenant = process.env.PANDA_SUBTENANT_ID; | ||
| process.env.PANDA_SUBTENANT_ID = "subtenant"; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Vitest vi.stubEnv vi.unstubAllEnvs usage
💡 Result:
In Vitest, vi.stubEnv and vi.unstubAllEnvs are used to manage environment variables during testing [1][2]. vi.stubEnv(name, value) This function updates an environment variable on both process.env and import.meta.env [1][3][4]. - name: The name of the environment variable (string) [1][3]. - value: The new value to set, or undefined to unset the variable [1][3][4]. Crucially, if you modify environment variables by directly assigning them (e.g., process.env.VAR = 'value'), Vitest will not be able to restore them later [1][3]. Using vi.stubEnv ensures that Vitest tracks the change, allowing for automatic or manual restoration [1]. vi.unstubAllEnvs This function restores all environment variables modified by vi.stubEnv to their original values that existed before any stubs were applied [1][3][4]. Automation To avoid calling vi.unstubAllEnvs manually in every test or hook, you can enable automatic resetting in your vitest.config.ts file by setting unstubEnvs to true [5][6]: // vitest.config.ts export default defineConfig({ test: { unstubEnvs: true, }, }) When this option is enabled, Vitest will automatically restore environment variables after each test [5][6]. Alternatively, you can continue to call vi.unstubAllEnvs manually, for example, within a beforeEach or afterEach hook [5][6][7].
Citations:
- 1: https://vitest.dev/api/vi
- 2: https://til.hashrocket.com/posts/ghwn4zzhj4-stub-environment-variables-in-vitest
- 3: https://github.com/vitest-dev/vitest/blob/206e8cff/docs/api/vi.md
- 4: https://github.com/vitest-dev/vitest/blob/v4.1.7/docs/api/vi.md
- 5: https://vitest.dev/guide/mocking
- 6: https://github.com/vitest-dev/vitest/blob/main/docs/guide/mocking.md
- 7: https://stackoverflow.com/questions/76768119/mocking-env-variables-with-vitest
🏁 Script executed:
#!/bin/bash
set -eu
file="server/test/hooks/panda.test.ts"
printf '%s\n' '--- relevant test setup and environment-variable usage ---'
sed -n '3160,3345p' "$file"
printf '%s\n' '--- Vitest imports and environment cleanup in this file ---'
rg -n -C 3 'from "vitest"|stubEnv|unstubAllEnvs|PANDA_SUBTENANT_ID|afterEach' "$file"
printf '%s\n' '--- repository Vitest configuration ---'
rg -n -C 3 'unstubEnvs|vitest|setupFiles' --glob '*.{ts,js,mjs,cjs,json}' --glob '!node_modules/**' .Repository: exactly/exa
Length of output: 37797
🏁 Script executed:
#!/bin/bash
set -eu
file="server/test/hooks/panda.test.ts"
printf '%s\n' '--- enclosing describe and setup ---'
sed -n '3000,3225p' "$file"
printf '%s\n' '--- all repeated subtenant cleanup blocks ---'
sed -n '3310,3530p' "$file"
printf '%s\n' '--- file tail and top-level hooks ---'
sed -n '4180,4240p' "$file"
printf '%s\n' '--- all Panda subtenant references in tracked files ---'
rg -n -C 2 'PANDA_SUBTENANT_ID' --glob '*.{ts,tsx,js,jsx,mjs,cjs,json}' .Repository: exactly/exa
Length of output: 23858
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
path = Path("server/test/hooks/panda.test.ts")
lines = path.read_text().splitlines()
start = next(i for i, line in enumerate(lines) if line == 'describe("webhooks", () => {')
end = next(i for i in range(start + 1, len(lines)) if lines[i] == "});" and i > 3510)
block = lines[start:end + 1]
save = sum("const previousSubtenant = process.env.PANDA_SUBTENANT_ID;" in line for line in block)
assign = sum('process.env.PANDA_SUBTENANT_ID = "subtenant";' in line for line in block)
restore = sum("if (previousSubtenant === undefined) delete process.env.PANDA_SUBTENANT_ID;" in line for line in block)
restore_assign = sum("else process.env.PANDA_SUBTENANT_ID = previousSubtenant;" in line for line in block)
after_each = next(
(i + 1 for i, line in enumerate(lines[start:end + 1], start) if "afterEach(() => vi.restoreAllMocks())" in line),
None,
)
print({
"manual_save_sequences": save,
"subtenant_assignments": assign,
"undefined_restore_branches": restore,
"value_restore_branches": restore_assign,
"webhooks_after_each_line": after_each,
})
assert save == assign == restore == restore_assign == 5
assert after_each == 3192
PYRepository: exactly/exa
Length of output: 391
Use vi.stubEnv for PANDA_SUBTENANT_ID.
Replace the four manual save-and-restore sequences with vi.stubEnv("PANDA_SUBTENANT_ID", "subtenant"). Add vi.unstubAllEnvs() to the existing afterEach at line 3192 so cleanup also runs when a test fails before its try block.
| "POST", | ||
| 10_000, | ||
| requireSubtenant(), | ||
| (options.subtenantId === "" ? undefined : options.subtenantId) ?? requireSubtenant(), |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Remove the dead empty-string branch.
(options.subtenantId === "" ? undefined : options.subtenantId) ?? requireSubtenant() produces the same result for "" and for undefined: both fall through to requireSubtenant(). The ternary therefore has no effect and suggests that "" opts out of subtenant routing. Line 205 in getCompanyApplicationStatus repeats the same pattern.
♻️ Proposed simplification
- (options.subtenantId === "" ? undefined : options.subtenantId) ?? requireSubtenant(),
+ options.subtenantId ?? requireSubtenant(),- (subtenantId === "" ? undefined : subtenantId) ?? requireSubtenant(),
+ subtenantId ?? requireSubtenant(),Also applies to: 205-205
| return database | ||
| .update(credentials) | ||
| .set({ pandaId: user.id }) | ||
| .where( | ||
| and(eq(credentials.id, credentialId), eq(credentials.pandaCompanyId, companyId), isNull(credentials.pandaId)), | ||
| ) | ||
| .returning({ id: credentials.id }) | ||
| .then(([updated]) => (updated ? user.id : undefined)); | ||
| })()); | ||
| if (!userId) return; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
The conditional pandaId update can silently skip card issuance.
The update is guarded by isNull(credentials.pandaId). If another writer sets pandaId between the initial findFirst and this update, the update matches zero rows, userId becomes undefined, and the function returns at Line 178 without creating or recording a card. The caller in server/api/kyc.ts (Line 587) and the webhook path both treat that as a completed finalization, so no retry occurs and the business credential stays without a card.
Re-read the row after a zero-row update and continue with the stored pandaId instead of returning.
🛡️ Proposed fix
.returning({ id: credentials.id })
- .then(([updated]) => (updated ? user.id : undefined));
+ .then(async ([updated]) => {
+ if (updated) return user.id;
+ const current = await database.query.credentials.findFirst({
+ columns: { pandaId: true },
+ where: eq(credentials.id, credentialId),
+ });
+ return current?.pandaId ?? undefined;
+ });📝 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.
| return database | |
| .update(credentials) | |
| .set({ pandaId: user.id }) | |
| .where( | |
| and(eq(credentials.id, credentialId), eq(credentials.pandaCompanyId, companyId), isNull(credentials.pandaId)), | |
| ) | |
| .returning({ id: credentials.id }) | |
| .then(([updated]) => (updated ? user.id : undefined)); | |
| })()); | |
| if (!userId) return; | |
| return database | |
| .update(credentials) | |
| .set({ pandaId: user.id }) | |
| .where( | |
| and(eq(credentials.id, credentialId), eq(credentials.pandaCompanyId, companyId), isNull(credentials.pandaId)), | |
| ) | |
| .returning({ id: credentials.id }) | |
| .then(async ([updated]) => { | |
| if (updated) return user.id; | |
| const current = await database.query.credentials.findFirst({ | |
| columns: { pandaId: true }, | |
| where: eq(credentials.id, credentialId), | |
| }); | |
| return current?.pandaId ?? undefined; | |
| }); | |
| })()); | |
| if (!userId) return; |
| if ( | ||
| (r.output.signature && verifySignature({ signature: r.output.signature, signingKey: key, payload })) || | ||
| (r.output["secondary-signature"] && | ||
| secondaryKey && | ||
| verifySignature({ | ||
| signature: r.output["secondary-signature"], | ||
| signingKey: secondaryKey, | ||
| payload, | ||
| })) | ||
| ) | ||
| return; | ||
| return c.text("unauthorized", 401); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
A malformed primary signature blocks the secondary signature check.
verifySignature calls timingSafeEqual(Buffer.from(signature, "hex"), Buffer.from(expectedSignature, "hex")). Node throws RangeError when the two buffers have different lengths. A signature header that is not a 32-byte hex string therefore throws instead of returning false. The short-circuit || then never evaluates the secondary-signature branch, and the handler fails with 500 instead of falling back to the secondary key or returning 401. This matters during key rotation, when Panda can send both headers.
Wrap each check so a malformed value is treated as invalid.
🛡️ Proposed fix
- if (
- (r.output.signature && verifySignature({ signature: r.output.signature, signingKey: key, payload })) ||
- (r.output["secondary-signature"] &&
- secondaryKey &&
- verifySignature({
- signature: r.output["secondary-signature"],
- signingKey: secondaryKey,
- payload,
- }))
- )
- return;
+ const valid = (signature: string | undefined, signingKey: string | undefined) => {
+ if (!signature || !signingKey) return false;
+ try {
+ return verifySignature({ signature, signingKey, payload });
+ } catch {
+ return false;
+ }
+ };
+ if (valid(r.output.signature, key) || valid(r.output["secondary-signature"], secondaryKey)) return;📝 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.
| if ( | |
| (r.output.signature && verifySignature({ signature: r.output.signature, signingKey: key, payload })) || | |
| (r.output["secondary-signature"] && | |
| secondaryKey && | |
| verifySignature({ | |
| signature: r.output["secondary-signature"], | |
| signingKey: secondaryKey, | |
| payload, | |
| })) | |
| ) | |
| return; | |
| return c.text("unauthorized", 401); | |
| const valid = (signature: string | undefined, signingKey: string | undefined) => { | |
| if (!signature || !signingKey) return false; | |
| try { | |
| return verifySignature({ signature, signingKey, payload }); | |
| } catch { | |
| return false; | |
| } | |
| }; | |
| if (valid(r.output.signature, key) || valid(r.output["secondary-signature"], secondaryKey)) return; | |
| return c.text("unauthorized", 401); |
45bf9bb to
11d5215
Compare
4216f04 to
76c713e
Compare
1 participant
summary
process panda business onboarding approvals.
stacked on #1203.
changes
test plan
notes
panda-b2bas its baseSummary by CodeRabbit
New Features
Bug Fixes