Add improvements to API endpoint auth checks#1151
Merged
Merged
Conversation
- /kyb/attempt-status now requires Supabase auth (CRITICAL-1: was unauthenticated, leaked KYB attempt data by subAccountId). - /getUser, /getUserRemainingLimit, /validatePixKey accept partner API key OR Supabase token via requirePartnerOrUserAuth, so SDK partners can reach them during ramp pre-flight.
CRITICAL-2: createSubaccount unconditionally overwrote the TaxId.subAccountId row on any taxId match, letting an attacker (anonymous or authenticated) hijack another user's subaccount by reusing their taxId and pointing it at a freshly created BRLA subaccount they control. - Look up the existing TaxId row before calling the BRLA API. If it is owned by a different userId, or owned by no one while the caller is authenticated, reject with 409 instead of overwriting. Overwrite is still allowed when the row is in the safe Consulted state (no real subaccount bound yet). - Surface APIError status codes from handleApiError so the 409 reaches clients. - Add controller tests covering getAveniaUser query handling and the new createSubaccount ownership cases.
The shared BrlaGetUserRequest type no longer carries quoteId, and /brla/getUser now authenticates via partner key or user token instead of a quote-ownership check. Update the SDK call sites accordingly so BRL pre-flight KYC validation works for partner SDK callers without a stale quoteId being appended to the query string.
Several BRLA endpoints accepted any authenticated (or even anonymous) caller and operated on taxId/subAccountId without checking that the row belongs to the caller. That let one Supabase user read or progress another user's KYC. - getAveniaUser: when authenticated as a Supabase user, reject with 403 if the taxId row's userId differs (HIGH-1). Partner SDK callers are unaffected. - /getUploadUrls: switch to requireAuth and require taxIdRecord.userId to match the caller (HIGH-3). - /newKyc: switch to requireAuth, validate subAccountId, and require the bound taxId row's userId to match the caller (HIGH-2). - /kyb/new-level-1/web-sdk: switch to requireAuth and require taxIdRecord.userId to match before initiating KYB (HIGH-4). - Extend getAveniaUser tests with a cross-user 403 case.
Before this, BRL ramp registration only validated KYC level. Invalid pix keys and over-limit amounts were only surfaced deep in the phase processor, with opaque errors. - Add ApiService.validateBrlPixKey and ApiService.getBrlRemainingLimit, both hitting the partner-or-user-authenticated BRLA endpoints. - registerBrlOfframp now rejects invalid pix destinations up front with InvalidPixKeyError. - Both BRL register flows fetch the quote, look up the user's remaining BRL limit for the matching ramp direction, and throw AmountExceedsLimitError before any ephemeral generation or ramp registration when the BRL amount on the quote exceeds the remaining limit.
✅ Deploy Preview for vortexfi ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for vortex-sandbox ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR tightens authentication/access control around BRLA endpoints, adds a validated DEPLOYMENT_ENV configuration knob, and updates BRL KYC SDK calls to remove an unused quoteId parameter. It also adds controller-level ownership checks for BRLA subaccount creation and introduces/extends tests around the new behaviors.
Changes:
- Switch select BRLA routes to dual-mode auth (
requirePartnerOrUserAuth) and require user auth for/kyb/attempt-status. - Add
DEPLOYMENT_ENVwith validation and enforce (intended) constraints withSANDBOX_ENABLED. - Remove
quoteIdfrom SDK BRL KYC status calls; add tests for BRLA controllers and new subaccount ownership logic.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/sdk/src/services/ApiService.ts | Simplifies getBrlKycStatus signature and request params (removes quoteId). |
| packages/sdk/src/handlers/BrlHandler.ts | Updates handler to match new KYC status API (no quoteId). |
| apps/api/src/config/vars.ts | Introduces DEPLOYMENT_ENV parsing/validation and updates environment-dependent settings/guards. |
| apps/api/src/api/routes/v1/brla.route.ts | Applies dual auth middleware to specific BRLA routes; adds requireAuth to KYB attempt status route. |
| apps/api/src/api/controllers/brla.controller.ts | Improves APIError-to-HTTP mapping and adds pre-create ownership checks for subaccounts. |
| apps/api/src/api/controllers/brla.controller.test.ts | Adds Bun tests covering getAveniaUser and createSubaccount edge cases. |
| apps/api/.env.example | Documents new DEPLOYMENT_ENV variable. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| if (error instanceof Error && error.message.includes("status '400'")) { | ||
| const splitError = error.message.split("Error: ", 1); |
Comment on lines
+322
to
+326
| if (existingTaxId && existingTaxId.internalStatus !== TaxIdInternalStatus.Consulted) { | ||
| const ownedByAnotherUser = existingTaxId.userId !== null && existingTaxId.userId !== (req.userId ?? null); | ||
| const ownedByAnonymousAndCallerIsUser = existingTaxId.userId === null && !!req.userId; | ||
| if (ownedByAnotherUser || ownedByAnonymousAndCallerIsUser) { | ||
| res.status(httpStatus.CONFLICT).json({ |
Comment on lines
+326
to
+329
| res.status(httpStatus.CONFLICT).json({ | ||
| error: "A subaccount already exists for this taxId" | ||
| } as unknown as BrlaCreateSubaccountResponse); | ||
| return; |
Comment on lines
+217
to
+218
| if (config.deploymentEnv === "production" && config.sandboxEnabled) { | ||
| throw new Error("SANDBOX_ENABLED=true requires DEPLOYMENT_ENV=sandbox; refusing to start production deployment"); |
Security hardening for BRL endpoints
- Fix handleApiError split bug: use limit 2 so JSON parsing branch is reachable - Fix subaccount ownership: allow authenticated users to claim anonymous TaxId records - Widen createSubaccount response type to BrlaCreateSubaccountResponse | BrlaErrorResponse - Enforce sandboxEnabled => deploymentEnv === "sandbox" for all environments - Require partner isActive flag in ownsPartnerRecord ownership check - Fail-closed on non-finite BRL amount in assertWithinBrlLimit - Update InvalidPixKeyError message to be pixKey-specific - Move newKyc validation before 5-second sleep to reject invalid requests early - Fix misleading "scoped by API key" comment in getAveniaUser Agent-Logs-Url: https://github.com/pendulum-chain/vortex/sessions/64b0a8ee-18e3-4c9b-a61b-973f3ef3a63b Co-authored-by: ebma <6690623+ebma@users.noreply.github.com>
…ment Agent-Logs-Url: https://github.com/pendulum-chain/vortex/sessions/64b0a8ee-18e3-4c9b-a61b-973f3ef3a63b Co-authored-by: ebma <6690623+ebma@users.noreply.github.com>
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
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.
Authentication and Access Control:
/getUser,/getUserRemainingLimit,/validatePixKey) to use a new middlewarerequirePartnerOrUserAuth, allowing both partner API keys and authenticated users to access these routes. The/kyb/attempt-statusendpoint now requires user authentication. [1] [2] [3]Environment Configuration and Validation:
DEPLOYMENT_ENVvariable with strict validation, added to.env.exampleand read inconfig/vars.ts. The code now enforces correct combinations ofDEPLOYMENT_ENVandSANDBOX_ENABLED, and provides clearer error messages if misconfigured. [1] [2] [3] [4] [5]Subaccount Creation and Ownership Logic:
createSubaccountcontroller to perform ownership checks before creating a new subaccount, preventing accidental overwrites and account-takeover scenarios. The logic now rejects requests if a subaccount already exists for the tax ID and is owned by another user or by an anonymous user when the caller is authenticated.getAveniaUserandcreateSubaccountcontrollers, covering new and existing edge cases.API Error Handling and Documentation:
SDK Simplification:
quoteIdparameter fromgetBrlKycStatusand related calls inBrlHandlerandApiService. [1] [2] [3] [4]