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. |
![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; | |
| } | |
| }; |
superagent-security
Bot
removed
the
contributor:verified
| .pipe(Effect.map(() => ({ success: true as const }))), | ||
| ), | ||
| ) | ||
| .handle("getPlayback", ({ path }) => |
There was a problem hiding this comment.
getPlayback returns signed URLs but doesn’t appear to enforce the same access checks as getCapById. If getByIdForViewing is broader than owner-only, this could let any authed user fetch playback for other users’ videos.
| .handle("getPlayback", ({ path }) => | |
| .handle("getPlayback", ({ path }) => | |
| withMappedErrors( | |
| Effect.gen(function* () { | |
| yield* getCapById(path.id); | |
| return yield* getPlayback(path.id); | |
| }), | |
| ), | |
| ) |
| [next], | ||
| ); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
This effect can re-run multiple times because searchParams changes identity across navigations. For mobileProvider=google, that can trigger multiple signIn("google") calls.
One option is to remove the param right after triggering so it’s one-shot:
| useEffect(() => { | |
| useEffect(() => { | |
| if (searchParams?.get("mobileProvider") === "google") { | |
| handleGoogleSignIn(); | |
| const params = new URLSearchParams(searchParams.toString()); | |
| params.delete("mobileProvider"); | |
| router.replace(`?${params.toString()}`); | |
| } | |
| if (searchParams?.get("mobileProvider") !== "workos") return; | |
| const mobileOrganizationId = searchParams.get("organizationId"); | |
| if (!mobileOrganizationId) { | |
| setShowOrgInput(true); | |
| return; | |
| } | |
| let active = true; | |
| handleWorkosSignIn(mobileOrganizationId).catch(() => { | |
| if (!active) return; | |
| setOrganizationId(mobileOrganizationId); | |
| setShowOrgInput(true); | |
| toast.error("Organization not found or SSO not configured"); | |
| }); | |
| return () => { | |
| active = false; | |
| }; | |
| }, [handleGoogleSignIn, handleWorkosSignIn, router, searchParams]); |
superagent-security
Bot
added
the
pr:flagged
|
hey @greptileai please re-review the pr |
superagent-security
Bot
removed
the
pr:flagged
| const result = await FileSystem.downloadAsync(download.url, target); | ||
| await MediaLibrary.saveToLibraryAsync(result.uri); | ||
|
|
||
| return download.fileName; |
There was a problem hiding this comment.
documentDirectory is never deleted after it is saved to the photo library. Each saveCapVideoToPhotos call leaves a copy behind, so the app's documents folder grows unboundedly whenever users download caps.
| const result = await FileSystem.downloadAsync(download.url, target); | |
| await MediaLibrary.saveToLibraryAsync(result.uri); | |
| return download.fileName; | |
| const result = await FileSystem.downloadAsync(download.url, target); | |
| try { | |
| await MediaLibrary.saveToLibraryAsync(result.uri); | |
| } finally { | |
| await FileSystem.deleteAsync(result.uri, { idempotent: true }); | |
| } | |
| return download.fileName; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/mobile/src/caps/saveCapVideo.ts
Line: 26-29
Comment:
The temporary file downloaded to `documentDirectory` is never deleted after it is saved to the photo library. Each `saveCapVideoToPhotos` call leaves a copy behind, so the app's documents folder grows unboundedly whenever users download caps.
```suggestion
const result = await FileSystem.downloadAsync(download.url, target);
try {
await MediaLibrary.saveToLibraryAsync(result.uri);
} finally {
await FileSystem.deleteAsync(result.uri, { idempotent: true });
}
return download.fileName;
```
How can I resolve this? If you propose a fix, please make it concise.1 participant
Implements v1 of the Cap mobile app for iOS
Greptile Summary
This PR introduces a full v1 iOS mobile app for Cap, along with the backend mobile API (
/api/mobile/[...route]), a shared domain schema package, and mobile-aware changes to the web login form. The mobile client handles auth (email OTP, Google OAuth, WorkOS SSO), cap browsing/management, and video uploads via a queue-based system.route.ts, 1 452 lines): email OTP verification uses an atomicDELETE … AND token = tokento prevent double-use;getPlaybackadds an explicitownerIdguard; therequestSessionredirect URI is now validated against an allowlist of custom-scheme protocols (cap:,exp+cap:).useRefone-shot guard but the WorkOS path is currently missing an equivalent.saveCapVideo.ts: downloads a cap todocumentDirectoryfor photo-library saving but does not clean up the temporary file afterwards, causing permanent storage accumulation per download.Confidence Score: 4/5
Safe to merge once the missing WorkOS idempotency guard and the temp-file cleanup in saveCapVideo are addressed.
The core security concerns from the first review round (redirect URI validation, OTP race) have been fixed. The remaining issues are a missing one-shot guard on the WorkOS SSO trigger and a permanent file-storage leak in the download-to-photos flow — both real defects, neither blocking auth or data integrity.
apps/mobile/src/caps/saveCapVideo.ts for the temp-file leak, and apps/web/app/(org)/login/form.tsx for the WorkOS guard gap.
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 (2): Last reviewed commit: "Mobile: handle non-JSON errors and redir..." | Re-trigger Greptile