[web] Ingest Google OAuth avatars as same-origin#530
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds a new google-avatar module that validates, fetches, and uploads Google-hosted avatar images; integrates avatar resolution into Google OAuth flows (callback, native, one-tap, mobile exchange); sanitizes avatar src in the Avatar component; and nulls external HTTP images in the /api/auth/me response. Tests added/updated. Changes
Sequence DiagramsequenceDiagram
participant Client
participant AuthRoute as Auth Route
participant GoogleAvatar as Google Avatar Resolver
participant Processor as Processor Service
participant Database
Client->>AuthRoute: OAuth callback / token exchange (includes picture URL)
AuthRoute->>Database: createOrLinkUser(..., image: null)
Database-->>AuthRoute: user
AuthRoute->>GoogleAvatar: resolveGoogleAvatarImage(userId, pictureUrl, existingImage)
alt same-origin or preserve
GoogleAvatar-->>AuthRoute: return existing same-origin image (no fetch)
else allowlisted Google URL
GoogleAvatar->>GoogleAvatar: validate URL, fetch image, verify MIME/size
GoogleAvatar->>Processor: upload image with scoped token
Processor-->>GoogleAvatar: stored image URL
GoogleAvatar-->>AuthRoute: return processor URL
else disallowed/non-allowlisted
GoogleAvatar-->>AuthRoute: return null
end
AuthRoute->>Database: update user.image if resolved differs
Database-->>AuthRoute: updated user
AuthRoute-->>Client: respond with user (image may be null or resolved URL)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 2
🤖 Fix all issues with AI agents
In `@apps/web/src/app/api/auth/me/route.ts`:
- Around line 4-5: Remove the local duplicate isExternalHttpUrl implementation
and import the shared function from "@/lib/auth/google-avatar" instead; update
the top-level imports in route.ts to pull in isExternalHttpUrl, delete the local
const isExternalHttpUrl declaration, and ensure any call sites in this file use
the imported symbol so types/signature remain consistent with the exported
function from google-avatar.
In `@apps/web/src/lib/auth/google-avatar.ts`:
- Around line 120-156: In readResponseBodyWithLimit, when totalBytes exceeds
maxBytes you must cancel the underlying stream before throwing to release the
connection; call and await reader.cancel() (or try/catch around it) immediately
prior to throwing the 'Avatar file exceeds size limit' error so the reader is
properly closed and the network resource is freed.
🧹 Nitpick comments (4)
apps/web/src/lib/auth/google-avatar.ts (2)
286-291: Redundant buffer copy.
bufferreturned fromreadResponseBodyWithLimitis already a freshUint8Array. CreatingfileBytesas anotherUint8Arrayof the same size and copying into it serves no purpose — you can passbufferdirectly tonew File(...).Proposed simplification
const filename = `google-avatar.${MIME_EXTENSION_MAP[detectedMimeType]}`; - const fileBytes = new Uint8Array(buffer.byteLength); - fileBytes.set(buffer); - const file = new File([fileBytes], filename, { type: detectedMimeType }); + const file = new File([buffer], filename, { type: detectedMimeType }); return await uploadAvatarToProcessor(userId, file);
158-196: Processor upload: consider logging the returned avatar URL on success.Currently, success is silent — only failures are logged. Adding a debug-level log with the resulting path would help trace avatar ingestion end-to-end without adding noise.
apps/web/src/app/api/auth/google/one-tap/route.ts (1)
124-189: Avatar resolution adds latency to the login critical path.
resolveGoogleAvatarImageperforms an external fetch to Google (up to 5 s timeout) plus a processor upload, all inline before returning the login response. For existing users who already have a local avatar, the function short-circuits (good), but for new users or those with stale external URLs, this can add noticeable latency to the sign-in experience.If login responsiveness becomes a concern, consider deferring avatar resolution to a background job or post-response task (e.g., the
unstable_afterAPI or a queue) and returning the response immediately withimage: null, then patching the user record asynchronously.This applies equally to the callback, native, and mobile exchange routes.
apps/web/src/app/api/auth/google/native/route.ts (1)
108-108: Nit: The update guard doesn't coveremailVerifiedchanges.The condition
!user.googleId || !user.name || user.image !== resolvedImagedetermines whether to run the update, but the update body also setsemailVerified(line 116). If a user already hasgoogleId,name, and the same image, a change inemail_verifiedstatus won't trigger an update. This appears to be pre-existing behavior, but worth noting since the condition was modified in this PR.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 201c770ab4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const processorResponse = await fetch(`${PROCESSOR_URL}/api/avatar/upload`, { | ||
| method: 'POST', | ||
| headers: { | ||
| Authorization: `Bearer ${token}`, | ||
| }, | ||
| body: formData, | ||
| }); |
There was a problem hiding this comment.
uploadAvatarToProcessor performs an awaited fetch to the processor without any abort/timeout, so when the processor is slow or hangs, Google sign-in requests can stall for a long time before returning (or never complete within request budgets). This helper is called synchronously in all updated Google auth flows before completing login, so a processor outage now directly degrades authentication availability instead of just dropping avatar ingestion; add an AbortController timeout (like the download path already has) and treat timeout as a non-fatal avatar failure.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/web/src/lib/auth/google-avatar.ts`:
- Around line 163-206: The uploadAvatarToProcessor function currently awaits
fetch to PROCESSOR_URL without a timeout; add an AbortController with the same
FETCH_TIMEOUT_MS timeout used elsewhere (or a new constant) and pass
controller.signal into the fetch call inside uploadAvatarToProcessor, clear the
timeout on completion, and catch AbortError to log a warning via loggers.auth
(including userId and that the processor timed out) and return null; ensure you
do not modify the FormData usage or Authorization header in
createUserServiceToken, and keep existing error handling for non-ok responses.
🧹 Nitpick comments (4)
apps/web/src/lib/auth/google-avatar.ts (2)
188-205: Consider sanitizingpayload.filenamebefore embedding it in the URL path.
payload.filenamefrom the processor response is used directly in a URL path on line 200. While the processor is a trusted internal service, defense-in-depth would suggest a basic sanity check (e.g., no slashes or..sequences) to prevent accidental path traversal if the processor response is ever malformed.Proposed fix
+ const safeFilename = payload.filename.replace(/[^a-zA-Z0-9._-]/g, ''); + if (safeFilename !== payload.filename || safeFilename.length === 0) { + loggers.auth.warn('Processor returned suspicious avatar filename', { userId, filename: payload.filename }); + return null; + } + - const avatarUrl = `/api/avatar/${userId}/${payload.filename}?t=${Date.now()}`; + const avatarUrl = `/api/avatar/${userId}/${safeFilename}?t=${Date.now()}`;
296-301: Minor: double castbuffer as unknown as Uint8Array<ArrayBuffer>.
bufferis already aUint8Array(line 153). Theas unknown as Uint8Array<ArrayBuffer>cast is likely needed to satisfy a stricterFileconstructor overload in the current TypeScript target. This works, but a more transparent approach would be to typebufferasUint8Array<ArrayBuffer>at the allocation site (line 153), or usenew Uint8Array(buffer.buffer).apps/web/src/app/api/auth/google/native/route.ts (1)
129-161: New user path: extra DB round-trip can be avoided.Since the user ID is generated by
createId()at line 133 before the insert, you could pre-generate the ID, resolve the avatar, and insert with the resolved image in a single operation. This saves one DB update for every new user sign-up.The current two-step approach is safe (user is created even if avatar fails), but you can get the same safety with the single-insert pattern:
Proposed optimization
isNewUser = true; loggers.auth.info('Creating new user via native Google OAuth', { email, platform }); + const newUserId = createId(); + + // Resolve avatar before insert – userId is already known. + // On failure resolveGoogleAvatarImage returns null, which is fine as the default. + const resolvedImage = await resolveGoogleAvatarImage({ + userId: newUserId, + pictureUrl: picture, + existingImage: null, + }); + const [newUser] = await db.insert(users).values({ - id: createId(), + id: newUserId, name: name || email.split('@')[0], email, emailVerified: email_verified ? new Date() : null, - image: null, + image: resolvedImage, googleId, provider: 'google', tokenVersion: 0, role: 'user', storageUsedBytes: 0, subscriptionTier: 'free', }).returning(); user = newUser; - - const resolvedImage = await resolveGoogleAvatarImage({ - userId: user.id, - pictureUrl: picture, - existingImage: user.image, - }); - - if (resolvedImage !== (user.image ?? null)) { - await db.update(users) - .set({ image: resolvedImage }) - .where(eq(users.id, user.id)); - user = { ...user, image: resolvedImage }; - }This same pattern applies to the callback and one-tap routes as well.
apps/web/src/app/api/auth/google/callback/route.ts (1)
149-210: Avatar resolution logic mirrors the native route exactly — consider extracting a shared helper.The existing-user update (lines 150–177) and new-user create-then-resolve (lines 178–207) patterns are duplicated verbatim across
callback/route.ts,native/route.ts, andone-tap/route.ts. A shared helper (e.g.,upsertUserWithAvatar) would eliminate the three-way duplication and ensure any future changes to the avatar resolution flow are applied consistently.
1 participant
Summary
*.googleusercontent.comURLs over HTTPS, rejects redirects, enforces timeout/size limits, validates MIME + magic bytes, and uploads via processor using scoped service tokens/api/avatar/...URLs instead of hotlinking external avatar URLsimage: nullfrom/api/auth/mewhen an external URL is presentVerification
pnpm --filter web test src/app/api/auth/google/__tests__/one-tap.test.ts src/app/api/auth/google/__tests__/google-callback-redirect.test.ts src/app/api/auth/google/native/__tests__/route.test.ts src/app/api/auth/__tests__/google-callback-redirect.test.ts src/app/api/auth/google/__tests__/open-redirect-protection.test.ts src/app/api/auth/__tests__/mobile-oauth-google-exchange.test.ts src/app/api/auth/__tests__/me.test.ts src/lib/auth/__tests__/google-avatar.test.tspnpm --filter web typecheckNotes
SWARM-TRACKER.mduntouchedSummary by CodeRabbit