feat(security): authenticate the charges websocket + drop the orphaned guest onramp action - #2534
Merged
Merged
Conversation
createOnrampForGuest called POST /bridge/onramp/create-for-guest, the unauthenticated route removed in peanut-api-ts #1247. Nothing in the UI ever called it — only its own two tests did — so this is dead code either way, but peanut-ui is a public repo and leaving it here keeps advertising the calling pattern it exploited: userId in the body, no auth header. Deleting the function orphans CreateOnrampGuestParams and three imports (CountryData, getCurrencyConfig, getCurrencyPrice), all of which existed solely to serve it. cancelOnramp and its serverFetch import are untouched. Generated API types are deliberately left alone: sync-openapi.yml pulls from staging on a weekday cron and owns them, and staging will not carry the route removal until the main->dev back-merge lands. Reported by Gibran, 27 Jul 2026.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughGuest onramp actions and demo routing were removed. API tests now cover direct web and native request routing. WebSocket connections authenticate with session tokens and stop reconnecting after credential rejection. The OpenAPI specification adds, removes, and updates endpoint contracts. ChangesOnramp API and request routing
WebSocket authentication
OpenAPI contract updates
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant HomeHistory
participant WebSocketClient
participant SessionToken
participant WebSocketServer
HomeHistory->>WebSocketClient: subscribe with authenticated username
WebSocketClient->>SessionToken: getSessionTokenForSocket()
SessionToken-->>WebSocketClient: session token or null
WebSocketClient->>WebSocketServer: auth frame
WebSocketServer-->>WebSocketClient: auth_ok or auth_error
WebSocketClient-->>HomeHistory: authentication event or rejected connection
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed: dependency version conflict. Check your lock file or package.json. Comment |
Contributor
Code-analysis diffPainscore total: 6279.56 → 6278.78 (-0.78) 🆕 New findings (371)
…and 351 more. ✅ Resolved (375)
…and 355 more. 📈 Painscore deltas (top movers)
|
Contributor
Author
|
@coderabbitai review |
Contributor
🧪 UI test report — ✅ all greenSuites
📊 Coverage (unit)
⏱ 10 slowest test cases
|
Contributor
✅ Action performedReview finished.
|
jjramirezn
marked this pull request as ready for review
July 27, 2026 18:18
Companion to peanut-api-ts #1247, which closes the PR #644 authorization fallout. The API now refuses to stream anything on /ws/charges/:username until the client proves who it is. The socket carries charge activity, Bridge/Manteca/Sumsub KYC status - including rejectLabels - and Rain card balance changes, and it was keyed on a public username with no credential at all. The frontend consumes those payloads (useWebSocket passes rejectLabels straight to the KYC handlers), so stripping them was not an option; the fix has to be auth. PeanutWebSocket now sends {type:'auth', token} as its first frame and the server subscribes to nothing until it verifies. A message frame rather than a header or query param because the browser WebSocket API cannot set headers, the jwt-token cookie is host-only on peanut.me and so never reaches api.peanut.me, and a token in a URL would land in every access log in between. It also suits native: getSessionTokenForSocket reads the token from the Capacitor cookie jar asynchronously, which a post-open frame can wait for. That helper is a deliberate, single-caller exception to "JS is not a token custodian on native" - the WebView's own WebSocket cannot see CapacitorHttp's jar, so nothing else can carry the credential. An auth rejection arrives as a non-clean close, which would otherwise fall into the 5x exponential-backoff loop and hammer the API with a credential already known to be bad. Close codes 4401/4408 and the auth_error frame now short-circuit that. HomeHistory subscribed to the `username` PROP, which on a public profile is the profile owner - so a logged-in viewer opened a socket on a stranger's channel and received their KYC and card events. It now always subscribes to the signed-in user's own channel, matching every other useWebSocket caller. The visible list is unchanged: it still comes from useTransactionHistory keyed on the viewed username with filterMutualTxs, which is what makes a profile show transactions mutual to the viewer. API types regenerated from the #1247 branch's openapi.json. Beyond the routes that PR deletes, this picks up four the committed spec had drifted behind (auth/step-up/*, unsubscribe, users/logout) - types only.
jjramirezn
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/services/websocket.ts (1)
191-197: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winInconsistent
auth_errorpayload shape across emission sites.
auth_erroris emitted with three different shapes:{ reason: 'no-session' }(handleOpen, line 165),message.data ?? null(here, line 196 — server-defined shape), and{ code, reason }(handleClose, line 278). A consumer reading.reasonvs.codeoff this single event name will getundefineddepending on which path fired it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/websocket.ts` around lines 191 - 197, Standardize the payload emitted by the auth_error event across handleOpen, the auth_error case in the message handler, and handleClose. Define and reuse one consistent payload shape containing the available code and reason fields, mapping server data into that shape and preserving null or missing values where unavailable.src/utils/auth-token.ts (1)
53-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDedupe with
hasNativeSessionCookie.This duplicates the exact
CapacitorCookies.getCookies+ try/catch logic already inhasNativeSessionCookie(lines 70-79). Extract a shared helper so the cookie-jar read logic exists once.♻️ Suggested refactor
Within the reviewed range:
+async function getNativeSessionCookie(): Promise<string | null> { + try { + const { CapacitorCookies } = await import('`@capacitor/core`') + const cookies = await CapacitorCookies.getCookies({ url: PEANUT_API_URL }) + return cookies?.[JWT_COOKIE_KEY] ?? null + } catch { + return null + } +} + export async function getSessionTokenForSocket(): Promise<string | null> { if (!isCapacitor()) return getAuthToken() - try { - const { CapacitorCookies } = await import('`@capacitor/core`') - const cookies = await CapacitorCookies.getCookies({ url: PEANUT_API_URL }) - return cookies?.[JWT_COOKIE_KEY] ?? null - } catch { - return null - } + return getNativeSessionCookie() }And update
hasNativeSessionCookie(outside this range) to reuse it:export async function hasNativeSessionCookie(): Promise<boolean> { if (!isCapacitor()) return false return !!(await getNativeSessionCookie()) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/auth-token.ts` around lines 53 - 63, Extract the shared Capacitor cookie lookup and error handling from getSessionTokenForSocket into a helper such as getNativeSessionCookie, returning the JWT cookie value or null. Update getSessionTokenForSocket and hasNativeSessionCookie to reuse this helper, preserving their existing non-Capacitor behavior and boolean result.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/Home/HomeHistory.tsx`:
- Around line 88-101: Update the history merge logic in HomeHistory so
wsHistoryEntries are included only when isViewingOwnHistory is true. Preserve
the existing useTransactionHistory results and filtering for public profiles,
preventing the viewer’s live websocket transactions from appearing in another
user’s history.
In `@src/services/websocket.ts`:
- Around line 146-173: Ensure the no-session branch in handleOpen emits a
matching disconnect event after auth failure, since disconnect() suppresses
handleClose by clearing the onclose handler. Preserve the existing auth_error
and no-reconnect behavior, and avoid emitting duplicate disconnect events when
normal close handling already runs.
---
Nitpick comments:
In `@src/services/websocket.ts`:
- Around line 191-197: Standardize the payload emitted by the auth_error event
across handleOpen, the auth_error case in the message handler, and handleClose.
Define and reuse one consistent payload shape containing the available code and
reason fields, mapping server data into that shape and preserving null or
missing values where unavailable.
In `@src/utils/auth-token.ts`:
- Around line 53-63: Extract the shared Capacitor cookie lookup and error
handling from getSessionTokenForSocket into a helper such as
getNativeSessionCookie, returning the JWT cookie value or null. Update
getSessionTokenForSocket and hasNativeSessionCookie to reuse this helper,
preserving their existing non-Capacitor behavior and boolean result.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fe99c5f3-9c1d-4fde-ae60-9d3f6b88849a
⛔ Files ignored due to path filters (1)
src/types/api.generated.tsis excluded by!**/*.generated.*
📒 Files selected for processing (4)
src/components/Home/HomeHistory.tsxsrc/services/websocket.tssrc/types/api.openapi.jsonsrc/utils/auth-token.ts
…onnect on the no-session path Two real bugs from the previous commit, both caught by CodeRabbit. Switching the socket to the signed-in user's own channel stopped the cross-user leak, but the merge downstream was left untouched - so on someone else's profile the VIEWER's own live transactions were still folded into the list being rendered, which is the profile owner's history. Derive liveEntries, empty unless viewing your own history, and merge that instead. The no-session branch emitted 'connect' (the socket did open), then 'auth_error', then called disconnect(). But disconnect() nulls onclose before closing, so handleClose - the only other place that emits 'disconnect' - never ran. useWebSocket maps those two events straight onto its status, so a client with no session sat there reporting "connected" forever with no reconnect scheduled to correct it. Emit 'disconnect' explicitly on that path.
Hugo0
approved these changes
Jul 28, 2026
Hugo0
left a comment
Hugo0
left a comment
Contributor
There was a problem hiding this comment.
Approving. Reviewed the diff rather than the description — the reasoning holds up.
The liveEntries memo is the catch I'd have most expected to be missed: once the socket is always your own channel, merging its entries into a profile you're viewing would inject your transactions into someone else's history. Good spot.
Verified independently:
useWebSocketalready no-ops on an undefined username (getWebSocketInstancereturns null), so theuser?.user.username ?? undefinedchange is safe for logged-out viewers on a public profile.- The
handleOpen→disconnect()→ manualemit('disconnect')path is correct —disconnect()nullsonclosefirst, so without it consumers would stay stuck reporting "connected".
Merge this one first, then peanut-api-ts#1247 — as your description says. Sending an auth frame is inert against today's API.
Two things carried forward, neither blocking:
- Old app-store builds don't send the auth frame and don't have the 4401/4408 short-circuit, so they'll fall into the 5× backoff loop — ~6 connections per session before giving up. Noise rather than danger, but it argues for landing the handshake on the
mobile-releasetrain with #2489. - The socket isn't re-authenticated after the handshake, so a live connection survives logout.
tokenVersionValidis the revocation gate and it only runs once. Cheap follow-up: re-check on the existing ping interval.
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
2 participants
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.
Summary
Frontend companion to peanut-api-ts #1247, which closes the authorization fallout from API PR #644.
Two things:
createOnrampForGuestserver action, whose route feat: handle onesignal push notification initialization #1247 deletes.The WebSocket
GET /ws/charges/:usernamehad no authentication of any kind and was keyed on a public username. Anyone could open it and stream a stranger's charge activity, Bridge/Manteca/Sumsub KYC status includingrejectLabels, deposits, pending perks and Rain card balance changes.Payload-stripping wasn't an option: the app genuinely consumes those payloads (
useWebSocketpassesrejectLabelsstraight into the Sumsub KYC handlers). So the fix is real auth, and that needs a client change — the API can't do it alone.PeanutWebSocketnow sends{type:'auth', token}as its first frame and the server subscribes to nothing until it verifies.A message frame rather than a header or query param because:
WebSocketAPI cannot set custom headers, ever;jwt-tokencookie is host-only onpeanut.me, so it is never sent toapi.peanut.me(and there's nocredentials: 'include'anywhere);It also suits native:
getSessionTokenForSocket()reads the token from the Capacitor cookie jar asynchronously, which a post-open frame can wait for but a connect-time URL cannot. That helper is a deliberate single-caller exception to the "JS is not a token custodian on native" rule inauth-token.ts— the WebView's ownWebSocketcannot see CapacitorHttp's jar, so nothing else can carry the credential. The token goes straight into a frame and is never stored.No reconnect loop on auth failure. A rejection arrives as a non-clean close, which would otherwise fall into the existing 5× exponential-backoff loop and hammer the API with a credential already known to be bad. Close codes
4401/4408and theauth_errorframe now short-circuit it.The public-profile leak
HomeHistorysubscribed to itsusernameprop. On a public profile that prop is the profile owner, so a logged-in viewer opened a socket on a stranger's channel and received their KYC and card-balance events. Every otheruseWebSocketcaller already passed the signed-in user's own username; this was the sole exception.It now always subscribes to the signed-in user's own channel. The visible list is unchanged — it still comes from
useTransactionHistory, keyed on the viewedusernamewithfilterMutualTxs, which is what makes a profile show transactions mutual to the viewer.(With the server-side check in #1247, passing someone else's username would simply fail to connect anyway.)
API types
Regenerated from #1247's
openapi.json. Beyond the routes that PR deletes, this picks up four the committed spec had drifted behind —/auth/step-up/options,/auth/step-up/verify,/unsubscribe,/users/logout. Types only, no runtime effect.CI
Green:
typecheck,unit,e2e,format,analyze,Deploy-Preview,ci-success.eslintis red — pre-existing and repo-wide, not from this PR. It fails onmainitself and on unrelated open PRs (#2528, #2533). The two files of mine it lists are flagged on lines this branch never touched (eventListeners: Map<…any…>, theon/offcasts,emit(…, data: any),useState<Array<any>>, an existingas any).Risk
npm run typecheckclean; test suite green apart from one pre-existing failure inadd-money-states.test.tsx(loaded EVM deposit shows QR code and address), which also fails on the base commit74ac9b107— verified in a throwaway worktree, unrelated to this change. CI'sunitjob passes it, so it looks environment-dependent locally.Known gap: native builds already in the app stores do not send the auth frame, so once #1247 deploys they lose live charge/KYC updates until users update. #2489 is on a
mobile-releasetrain — worth coordinating so the handshake rides it. Web is fully covered by the merge order below.Merge order
This inverts what #1247's original description said ("this PR must merge first"), which was written when this branch was a pure dead-code deletion with no runtime coupling.
Summary by CodeRabbit
Bug Fixes
API Updates