feat(security): prove a fresh passkey assertion on sensitive actions - #2463
Conversation
Card PAN/CVV, PIN, withdrawals and bank-account add were authorized by the session cookie alone. They now carry an x-step-up-token proving a WebAuthn assertion from the last five minutes. The proof is cached for its lifetime, so a multi-step flow (approve then prepare) costs one Face ID prompt rather than one per request, and is dropped on logout so it can't outlive the session it belongs to. Rain calls opt in with stepUp: true on the single rainRequest choke point instead of threading a header through every call site. Pairs with peanut-api-ts, where enforcement stays behind STEP_UP_ENFORCED until a build carrying this ships.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Code-analysis diffPainscore total: 6212.38 → 6218.98 (+6.6) 🆕 New findings (14)
✅ Resolved (13)
📈 Painscore deltas (top movers)
|
🧪 UI test report — ✅ all greenSuites
📊 Coverage (unit)
⏱ 10 slowest test cases
|
kushagrasarathe
left a comment
kushagrasarathe
left a comment
There was a problem hiding this comment.
Review — FE half of step-up (with peanut-api-ts#1217) — SAFE-with-conditions
Sound: routes step-up through the single rainRequest choke point (stepUp: true), sends a fresh passkey assertion, and the BE independently verifies it. No client-only gating, no backdoor. Reviewed together with the BE half.
Notes / conditions (mostly shared with #1217):
- Enforcement is contingent on #1217's
STEP_UP_ENFORCEDflag — until flipped, this ships the prompt UX with no server enforcement. - Native path untested (
@capgo/capacitor-passkey) — needs an on-device iOS+Android pass. - Confirm native carries the session:
rainRequestreadsCookies.get('jwt-token'), which may not resolve on native where the token lives in the native cookie jar / Preferences. Verify native Rain/card flows authenticate before relying on step-up there.
The challenge-TTL fix (the one real security gap) is BE-side — see #1217.
Reading the jwt cookie directly wrongly threw 'Authentication required' on native, where JS never holds the token and the native cookie jar authenticates requests. apiFetch is the path every other service uses: Authorization header on web, cookie jar on Capacitor, demo-mode routing included.
📝 WalkthroughWalkthroughAdds a WebAuthn-based step-up authentication service with cached proofs, expiry handling, header injection, and logout clearing. Sensitive Rain Card operations and bank-account creation now include step-up authentication. ChangesStep-up authentication
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant UserAction
participant rainApi
participant rainRequest
participant StepUpService
participant AuthAPI
participant RainAPI
UserAction->>rainApi: Start sensitive card operation
rainApi->>rainRequest: Request with stepUp=true
rainRequest->>StepUpService: Get step-up token
StepUpService->>AuthAPI: WebAuthn options and verification
AuthAPI-->>StepUpService: Proof token
StepUpService-->>rainRequest: x-step-up-token
rainRequest->>RainAPI: Protected request
RainAPI-->>UserAction: Operation response
Suggested labels: 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/services/step-up.ts`:
- Around line 40-42: Update getStepUpToken to maintain a single in-flight
proof-acquisition promise when the cache is empty, returning that promise to
concurrent callers instead of starting multiple WebAuthn ceremonies. Assign the
promise before acquisition begins and clear it when it settles, while preserving
the existing cached token and expiry behavior.
🪄 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: e4e4c3a3-ad18-4fa9-804c-db3791bc610c
📒 Files selected for processing (5)
src/app/actions/users.tssrc/context/authContext.tsxsrc/services/__tests__/step-up.test.tssrc/services/rain.tssrc/services/step-up.ts
| export async function getStepUpToken(): Promise<string> { | ||
| if (cached && cached.expiresAt - EXPIRY_MARGIN_MS > Date.now()) return cached.token | ||
| cached = null |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Deduplicate concurrent proof acquisition.
Concurrent callers both see an empty cache and each start a WebAuthn ceremony; the cache is only populated at Line 70. Keep one in-flight promise and share it until it settles, otherwise parallel sensitive requests can prompt twice or fail one assertion.
🤖 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/step-up.ts` around lines 40 - 42, Update getStepUpToken to
maintain a single in-flight proof-acquisition promise when the cache is empty,
returning that promise to concurrent callers instead of starting multiple
WebAuthn ceremonies. Assign the promise before acquisition begins and clear it
when it settles, while preserving the existing cached token and expiry behavior.
|
Both conditions in the review are now handled on this side; the rest live on the BE PR. Native session — fixed in b52b2bc. You were right to flag Challenge TTL — landed BE-side (peanut-api-ts#1217, Still outstanding, tracked in #1217's pre-flip checklist: the on-device iOS+Android pass through |
kushagrasarathe
left a comment
kushagrasarathe
left a comment
There was a problem hiding this comment.
Re-reviewed — native-auth condition RESOLVED. rainRequest no longer reads Cookies.get(jwt-token); it routes through apiFetch → getAuthHeaders → getAuthToken (localStorage on Capacitor, cookie on web). Residual guard is correctly !isCapacitor() && !token. Step-up options/verify go through the same native-safe path; headers preserved; cache cleared on logout. SAFE. (Follow-up, non-gating: card.ts still reads the cookie directly — separate ticket.)
2 participants
Why
Card PAN/CVV reveal, PIN read/set, withdrawals and bank-account add are authorized today by the session cookie alone. Sessions persist for weeks, so a borrowed unlocked phone or an exfiltrated web cookie reaches all of it with no further checks.
This is the client half of item A — the last of the session-security series and the one that closes the real gap. API half: peanut-api-ts#1217. Companions: app lock (#2461), report-only CSP (#2462).
What
src/services/step-up.tsruns a WebAuthn assertion against the user's own passkey and exchanges it for a 5-minute proof token, sent asx-step-up-token.Gated calls:
withdraw/session-approve,withdraw/prepare— opted in withstepUp: trueat the singlerainRequestchoke point rather than threading a header through every call site.POST /users/accounts(add bank account) viawithStepUpHeader.Prompt economy
The proof is cached for its lifetime with a 30s safety margin. A withdrawal (
session-approve→prepare) costs one Face ID prompt, not two. The cache is dropped on logout — a proof outliving its session would let the next person on the device skip verification.Safe to merge alone
The API accepts the header today but doesn't require it: enforcement sits behind
STEP_UP_ENFORCED, default off. So this can ship in either order, and the flag only flips once a build carrying it is out. Until then the extra prompt appears but nothing depends on it.Testing
8 unit tests, passing — ceremony, cache reuse, expiry-margin re-prompt, clear-on-logout, the no-passkey message, no caching on rejection, and that a cancelled prompt propagates rather than silently proceeding unverified. Full
src/servicessuite green (18). Typecheck clean.Not verified on a device. The web path is exercised by tests with
@simplewebauthn/browsermocked; the native path goes through the@capgo/capacitor-passkeyshim, which mocks can't stand in for. Worth a native pass on card reveal and a withdrawal beforeSTEP_UP_ENFORCEDis flipped.One thing worth a reviewer's eye
rainRequestreads the JWT withCookies.get('jwt-token')and throwsAuthentication requiredwhen it's absent. On native,getAuthToken()returns null by design — the token lives in the native cookie jar — so this path looks like it should already fail there. I left it alone since it's pre-existing and unrelated, but if Rain card flows do work on native today, I'm misreading something and the step-up wiring may need the same treatment.Summary by CodeRabbit
New Features
Bug Fixes