Cap mobile app#1845
Conversation
superagent-security
Bot
added
the
contributor:verified
|
Paragon Review Skipped Hi @richiemcilroy! Your Polarity credit balance is insufficient to complete this review. Please visit https://app.paragon.run to finish your review. |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| { concurrency: 5 }, | ||
| ); | ||
| }); | ||
|
|
||
| const getCapDetail = Effect.fn("Mobile.getCapDetail")(function* ( | ||
| videoId: Video.VideoId, | ||
| ) { | ||
| const { row, cap } = yield* getCapById(videoId); | ||
| const metadata = getMetadataRecord(row.metadata); | ||
| const comments = yield* getComments(videoId); | ||
|
|
||
| return { | ||
| cap, | ||
| summary: getMetadataString(metadata, "summary"), |
There was a problem hiding this comment.
The verification token is deleted from the database before the expiry check. Two concurrent requests carrying the same valid code will both SELECT the token (both rows are returned), both issue the DELETE (the second silently deletes 0 rows, not an error), both pass the expiry check, and both receive a freshly-minted API key. Moving the DELETE to after the expiry check (or wrapping both in a transaction / using DELETE … RETURNING in a single statement) closes the window.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/app/api/mobile/[...route]/route.ts
Line: 856-869
Comment:
**TOCTOU race: same OTP accepted twice**
The verification token is deleted from the database _before_ the expiry check. Two concurrent requests carrying the same valid code will both `SELECT` the token (both rows are returned), both issue the `DELETE` (the second silently deletes 0 rows, not an error), both pass the expiry check, and both receive a freshly-minted API key. Moving the `DELETE` to _after_ the expiry check (or wrapping both in a transaction / using `DELETE … RETURNING` in a single statement) closes the window.
How can I resolve this? If you propose a fix, please make it concise.| .from(Db.comments) | ||
| .leftJoin(Db.users, eq(Db.comments.authorId, Db.users.id)) | ||
| .where(eq(Db.comments.videoId, videoId)) | ||
| .orderBy(Db.comments.createdAt), | ||
| ); | ||
|
|
||
| return yield* Effect.forEach( | ||
| rows, | ||
| (row) => | ||
| Effect.gen(function* () { | ||
| const imageUrl = row.authorImage | ||
| ? yield* imageUploads | ||
| .resolveImageUrl(row.authorImage) | ||
| .pipe(Effect.catchAll(() => Effect.succeed(null))) | ||
| : null; | ||
|
|
||
| return { | ||
| id: row.id, | ||
| videoId: row.videoId, | ||
| type: row.type, | ||
| content: row.content, | ||
| timestamp: row.timestamp, | ||
| parentCommentId: row.parentCommentId, | ||
| createdAt: toIsoString(row.createdAt), | ||
| updatedAt: toIsoString(row.updatedAt), | ||
| author: { | ||
| id: row.authorId, | ||
| name: row.authorName, | ||
| imageUrl, | ||
| }, | ||
| }; | ||
| }), | ||
| { concurrency: 5 }, | ||
| ); | ||
| }); | ||
|
|
||
| const getCapDetail = Effect.fn("Mobile.getCapDetail")(function* ( | ||
| videoId: Video.VideoId, | ||
| ) { | ||
| const { row, cap } = yield* getCapById(videoId); | ||
| const metadata = getMetadataRecord(row.metadata); | ||
| const comments = yield* getComments(videoId); | ||
|
|
||
| return { | ||
| cap, | ||
| summary: getMetadataString(metadata, "summary"), | ||
| chapters: getMetadataChapters(metadata), | ||
| transcriptionStatus: row.transcriptionStatus, | ||
| comments, | ||
| shareUrl: `${serverEnv().WEB_URL}/s/${videoId}`, | ||
| }; | ||
| }); | ||
|
|
||
| const createMobileComment = Effect.fn("Mobile.createComment")(function* ({ |
There was a problem hiding this comment.
verifyEmailSession accepts any 6-digit code (10⁶ possibilities) with no per-email attempt counter or exponential back-off. An attacker who receives the requestEmailCode confirmation for a target email address can cycle through all ~10⁶ codes within minutes. Middleware-level rate limiting alone is often insufficient here; a counter stored alongside the token record (or simply deleting the token on any failed attempt) would make brute-forcing impractical.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/app/api/mobile/[...route]/route.ts
Line: 824-877
Comment:
**No brute-force protection on the OTP endpoint**
`verifyEmailSession` accepts any 6-digit code (10⁶ possibilities) with no per-email attempt counter or exponential back-off. An attacker who receives the `requestEmailCode` confirmation for a target email address can cycle through all ~10⁶ codes within minutes. Middleware-level rate limiting alone is often insufficient here; a counter stored alongside the token record (or simply deleting the token on any failed attempt) would make brute-forcing impractical.
How can I resolve this? If you propose a fix, please make it concise.![tembo[bot]](https://avatars.githubusercontent.com/in/1223475?s=60&v=4){kind=small}
| const session = yield* createMobileApiKey(user.value.id); | ||
|
|
||
| if (urlParams.redirectUri) { | ||
| const redirectUrl = new URL(urlParams.redirectUri); |
There was a problem hiding this comment.
redirectUri here is effectively untrusted input. As-is, a logged-in user hitting /api/mobile/session/request?redirectUri=https://evil.com would get redirected with api_key in the query string.
Consider allowlisting schemes/hosts before appending secrets:
| const redirectUrl = new URL(urlParams.redirectUri); | |
| if (urlParams.redirectUri) { | |
| const redirectUrl = new URL(urlParams.redirectUri); | |
| if ( | |
| redirectUrl.protocol !== "cap:" && | |
| redirectUrl.protocol !== "exp+cap:" | |
| ) { | |
| return yield* Effect.fail(new HttpApiError.BadRequest()); | |
| } | |
| redirectUrl.searchParams.set("api_key", session.apiKey); | |
| redirectUrl.searchParams.set("user_id", user.value.id); | |
| return HttpServerResponse.redirect(redirectUrl.toString()); | |
| } |
| .digest("hex"); | ||
|
|
||
| const sendMobileEmailCode = async (email: string, code: string) => { | ||
| if (!serverEnv().RESEND_API_KEY) { |
There was a problem hiding this comment.
Logging OTP codes to server logs is risky if RESEND_API_KEY is accidentally unset in prod. I’d consider failing hard in production (and only logging in non-prod):
| if (!serverEnv().RESEND_API_KEY) { | |
| if (!serverEnv().RESEND_API_KEY) { | |
| if (process.env.NODE_ENV === "production") { | |
| throw new Error("RESEND_API_KEY is required to send mobile email codes"); | |
| } | |
| console.log(""); | |
| console.log("Cap mobile verification code"); | |
| console.log(`Email: ${email}`); | |
| console.log(`Code: ${code}`); | |
| console.log("Expires in: 10 minutes"); | |
| console.log(""); | |
| return; | |
| } |
| } | ||
| }; | ||
|
|
||
| const parseJson = async (response: Response) => { |
There was a problem hiding this comment.
parseJson will throw if the backend ever returns non-JSON (common for proxies / HTML error pages), which bypasses your MobileApiError path. Small hardening to keep errors actionable:
| const parseJson = async (response: Response) => { | |
| const parseJson = async (response: Response) => { | |
| const text = await response.text(); | |
| if (text.length === 0) return null; | |
| try { | |
| return JSON.parse(text) as unknown; | |
| } catch { | |
| return text; | |
| } | |
| }; |
1 participant
Implements v1 of the Cap mobile app for iOS
Greptile Summary
This PR introduces the first version of the Cap mobile iOS app — an Expo/React Native application backed by a new Next.js API surface at
/api/mobile. It adds OAuth and email-OTP sign-in, a cap library browser, video upload with progress tracking, playback, and cap management (titles, passwords, sharing, comments).apps/mobile): Full Expo Router app with typed API client, secure-store session management, upload queue reducer, and comprehensive vitest coverage.apps/web/app/api/mobile): 1,400-line Effect-based handler covering auth (session request, email OTP, revoke), bootstrap, cap CRUD, comments, reactions, and upload lifecycle.packages/web-domain/src/Mobile.ts): New Effect schemas defining the mobile API contract, with a matching contract test suite.Confidence Score: 2/5
The mobile app code itself is well-structured and tested, but the new server-side API handler introduces multiple auth security gaps that should be addressed before this ships to users.
The web API route has three compounding auth issues: the OAuth session endpoint appends a permanent API key to any caller-supplied redirect URL with no scheme or domain validation; the email OTP verification deletes the token before checking expiry, allowing the same code to authenticate two concurrent requests simultaneously; and no attempt counter guards the 6-digit code endpoint against exhaustive search. All three affect the authentication path that every mobile user will go through on first sign-in.
apps/web/app/api/mobile/[...route]/route.ts needs the most attention — specifically the requestSession redirect, the verifyEmailSession token deletion order, and the missing rate-limit guard on OTP verification.
Security Review
apps/web/app/api/mobile/[...route]/route.ts,requestSessionhandler): TheredirectUriquery parameter is appended withapi_keyanduser_idwithout any allowlist validation. A crafted link can redirect an authenticated user's permanent API key to an attacker-controlled URL.verifyEmailSession): The verification token is deleted fromverificationTokensbefore the expiry timestamp is checked. Two simultaneous requests carrying the same valid code can both succeed and both receive independent API keys.verifyEmailSession): No per-email attempt counter or back-off is implemented against the 6-digit code endpoint, making exhaustive search feasible.Important Files Changed
Comments Outside Diff (3)
apps/web/app/api/mobile/[...route]/route.ts, line 1586-1591 (link)redirectUrileaks API key to arbitrary URLsThe
requestSessionhandler appends the freshly-mintedapi_keyanduser_idto whateverredirectUrithe caller provides, with no allowlist check. An attacker who tricks an authenticated user into openinghttps://cap.so/api/mobile/session/request?redirectUri=https://attacker.comwill receive that user's permanent API key in the redirect. TheredirectUrishould be validated against the app's registered deep-link scheme (e.g., onlycap://or the deployment-specific scheme fromapp.config.js) before any credentials are appended.Prompt To Fix With AI
apps/web/app/api/mobile/[...route]/route.ts, line 1413-1450 (link)getPlaybackhas no ownership check on the videogetCapByIdenforceseq(Db.videos.ownerId, user.id), butgetPlaybackbypasses it and callsvideos.getByIdForViewingdirectly. IfgetByIdForViewingreturns records for public or org-shared videos that do not belong to the authenticated user, any authenticated mobile user can retrieve signed playback URLs for those videos. Confirm whether this broader access is intentional for the mobile app, or whether an ownership/membership check should be added here.Prompt To Fix With AI
apps/web/app/(org)/login/form.tsx, line 143-162 (link)searchParamsidentity changeThe new
useEffectcallshandleGoogleSignIn()whenevermobileProvider=googleis present in the query string. BecausesearchParamsis a new reference on every navigation update, this effect can fire multiple times during the session redirect lifecycle, potentially triggering extrasignIn("google", ...)calls. Guard the effect with auseRefflag (or router-replace the URL after the first trigger) so it fires exactly once per page load.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "chore(web): refresh workflow step manife..." | Re-trigger Greptile