Conversation
- Store tokens per-account in data/google-accounts/{email}.json
- Aggregate emails/events across all connected accounts in parallel
- Add accountEmail field to EmailMessage and CalendarEvent types
- Connect banner shows account strip when connected with add/remove
- Auto-migrate from legacy single-account data/google-auth.json
- exchangeCode fetches user email via Gmail/OAuth2 API after auth
✅ Deploy Preview for agent-native-fw ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![builder-io-integration[bot]](https://avatars.githubusercontent.com/in/353721?s=60&v=4){kind=small}
There was a problem hiding this comment.
Builder has reviewed your changes and found 6 potential issues.
Review Details
Code Review Summary — PR #28: Multi-account Google OAuth
This PR migrates both mail and calendar templates from single-account to multi-account Google OAuth. Tokens are stored per-account as data/google-accounts/{email}.json with auto-migration from a legacy single file. Emails and calendar events are fetched from all connected accounts in parallel, and a new account management UI lets users add/remove accounts.
Risk Level: 🔴 HIGH — This PR touches authentication, token storage, and introduces new security-sensitive paths derived from user input.
The overall architecture is reasonable: using files as the account store fits the agent-native framework, and the parallel fetch pattern is correct. However, several security and correctness bugs were found by all 5 parallel review agents.
🔴 HIGH — Path Traversal in disconnect() (mail + calendar)
The user-supplied email from req.body is passed directly to path.join(ACCOUNTS_DIR, \${email}.json`)and then deleted — no bounds check. Sending{"email": "../../settings"}deletesdata/settings.json. Any .json` file reachable by the process is at risk. Found by 5/5 agents.
🔴 HIGH — XSS in OAuth Callback HTML (mail + calendar)
The email value from exchangeCode() is interpolated unescaped into an inline <script> block as a JS string literal that is also assigned to innerHTML. RFC 5321 permits ' in quoted local-parts; a quote terminates the string and executes arbitrary JS in the OAuth popup origin. Found by 5/5 agents.
🟡 MEDIUM — Race condition in migrateLegacyTokens(), stale token closure, and error masking in multi-account operations (see comments).
Reviewed using 5 parallel code-review agents with randomized file ordering to counter position bias.
Code review by Builder.io
- Path traversal: disconnect() now validates email against known account files instead of joining raw input into path - XSS: callback page uses JSON.stringify + textContent instead of interpolating email into innerHTML - Race condition: migrateLegacyTokens() uses module-level promise to deduplicate concurrent calls - Stale closure: token refresh handler re-reads file from disk instead of merging onto captured snapshot
There was a problem hiding this comment.
Builder has reviewed your changes and found 5 potential issues.
Review Details
Code Review Summary — PR #28 Update: Multi-account Google OAuth
The previous 6 issues (path traversal ×2, XSS ×2, migration race, stale token closure) are all cleanly fixed in this update — good work. The getAccountFiles() lookup pattern for disconnect, JSON.stringify + textContent for the callback page, module-level migrationPromise, and re-reading the file inside the refresh handler are all correct approaches.
Risk Level: 🔴 HIGH — still touches auth and credential storage.
Three new issues were found by 2-4 of 4 parallel agents.
🔴 HIGH — Calendar OAuth completely broken (missing email scope)
The calendar template's SCOPES only includes calendar.readonly and calendar.events. The new exchangeCode() now calls oauth2.userinfo.get() to resolve the email for the filename, but that endpoint requires openid or userinfo.email scope. Connecting a Google Calendar account will throw a 403 from the userinfo call and return HTTP 500 to the user — no account is stored. The mail template uses gmail.users.getProfile() which works with its existing Gmail scope, so mail is unaffected.
🔴 HIGH — Silent token overwrite via "unknown" fallback (mail + calendar)
Both exchangeCode() implementations fall back to "unknown" when Google returns no email, writing unknown.json. A second OAuth flow that also fails email resolution silently overwrites the first account's tokens — permanent credential loss with no error surfaced.
🟡 MEDIUM — getEmail/markRead swallow auth errors and markRead targets wrong account
All errors (401, 403, 429) are caught as "try next account". markRead doesn't even destructure email from the client loop, making it unable to route by accountEmail even if the client sends it — instead firing N Gmail API calls on every read-toggle.
Reviewed with 4 parallel code-review agents on the full PR diff.
Code review by Builder.io
- Add userinfo.email scope to calendar OAuth - Throw on missing email instead of silent "unknown" fallback - getEmail: only continue to next account on 404, surface other errors - markRead: route via accountEmail instead of iterating all clients - Delete corrupt legacy token file to prevent infinite retry loop
There was a problem hiding this comment.
Builder has reviewed your changes and found 6 potential issues.
Review Details
Code Review Summary (Round 3)
PR #28 adds multi-account Google OAuth to both mail and calendar templates, with per-account token files in data/google-accounts/{email}.json, legacy auto-migration, and parallel aggregated fetching. Risk: HIGH (OAuth flows, token storage, credential routing). All 5 issues from round 2 were correctly fixed. Reviewed using 4 parallel agents with randomized file ordering.
Key Findings
🔴 HIGH:
getClientremoved from import but still called inmarkRead— compile error + runtimeReferenceError, breaking all read/unread toggling for Gmail accounts
🟡 MEDIUM:
listGmailMessages()andlistEvents()swallow all per-account errors (401, 429, network failures), silently returning[]and turning auth failures into an empty inbox/calendarupdateEventingoogle-calendar.tsnow routes byevent.accountEmail, butroutes/events.tsstill passesreq.body(which never includesaccountEmail), so every calendar event update routes to the first connected account- Migration fallback filenames (
"migrated-account","unknown-account") become permanent fake email addresses since the filename is the source of truth for account identity throughout the codebase error?.code(a Node.js string like'ECONNRESET') passed tores.status(), triggeringERR_HTTP_INVALID_STATUS_CODEand dropping the connection on network errors
Found using 4 parallel code-review agents with randomized file ordering.
Code review by Builder.io
| try { | ||
| const client = await getClient(); | ||
| // Route to specific account if provided, otherwise try first client | ||
| const client = await getClient(accountEmail); |
There was a problem hiding this comment.
getClient called but not imported — ReferenceError crashes markRead
🔴 High Priority
The PR replaced getClient with getClients in the import but markRead still calls getClient(accountEmail) on this line, which is not in scope — confirmed by pnpm typecheck failing with TS2552: Cannot find name 'getClient'. Every PATCH /api/emails/:id/read request while Google is connected throws a ReferenceError, making read/unread toggling completely broken. Fix: add getClient back to the import alongside getClients.
How did I do? React with 👍 or 👎 to help me improve.
There was a problem hiding this comment.
Fixed — added getClient back to the import.
| `[listGmailMessages] Error fetching from ${email}:`, | ||
| error.message, | ||
| ); | ||
| return []; |
There was a problem hiding this comment.
listGmailMessages swallows 401/429 errors — auth failures silently appear as empty inbox
🟡 Medium Priority
The catch block returns [] for all errors including 401 (expired/revoked token) and 429 (rate limit), so a single expired account causes the entire inbox to return a successful 200 [] response with no indication of failure. Fix: only suppress 404-style not-found conditions; rethrow 401/429/5xx so the route can return an appropriate error status.
How did I do? React with 👍 or 👎 to help me improve.
There was a problem hiding this comment.
Partially agreed — with multiple accounts, one expired token shouldn't blank the whole inbox. Changed listGmailMessages to return { messages, errors }. The route now returns 502 when ALL accounts fail, but returns results with an X-Account-Errors header when only some fail. This way partial data still shows while surfacing the issue.
| `[listEvents] Error fetching from ${email}:`, | ||
| error.message, | ||
| ); | ||
| return []; |
There was a problem hiding this comment.
listEvents swallows 401/429 errors — auth failures silently appear as empty calendar
🟡 Medium Priority
Same pattern as listGmailMessages: the per-account catch in listEvents returns [] for all errors, turning a 401/429 on the only connected account into an empty calendar with a 200 status rather than an actionable error. Fix: propagate non-recoverable Google API errors (auth failures, rate limits) to the route instead of suppressing them.
How did I do? React with 👍 or 👎 to help me improve.
There was a problem hiding this comment.
Fixed — same approach as mail. Returns 502 when all accounts fail, partial results + X-Account-Errors header otherwise.
| event: Partial<CalendarEvent>, | ||
| ): Promise<void> { | ||
| const client = await getClient(); | ||
| const client = await getClient(event.accountEmail); |
There was a problem hiding this comment.
updateEvent routes by event.accountEmail but callers pass req.body — updates wrong account
🟡 Medium Priority
updateEvent now calls getClient(event.accountEmail), but routes/events.ts passes raw req.body to this function — and req.body never includes accountEmail (confirmed by EventDialog.tsx which sends only { id, title, description, location, start, end } and drag-drop handlers which send only { id, start, end }). With two accounts, getClient(undefined) falls back to files[0], so every calendar event update writes to the first connected account regardless of which account owns the event. Fix: pass the merged updated object (which has accountEmail from the stored event) instead of req.body.
How did I do? React with 👍 or 👎 to help me improve.
There was a problem hiding this comment.
Fixed — now passes the merged updated object (which inherits accountEmail from the stored event) instead of raw req.body.
| } | ||
| } catch { | ||
| // If we can't reach Gmail (offline, expired), just move with a fallback name | ||
| writeJsonFile(path.join(ACCOUNTS_DIR, "migrated-account.json"), tokens); |
There was a problem hiding this comment.
Migration fallback filenames become permanent fake account emails in the UI
🟡 Medium Priority
When migration runs while offline or Google returns no email, tokens are written as migrated-account.json or unknown-account.json; since path.basename(file, '.json') is the sole source of truth for account email throughout the codebase, these synthetic names surface permanently in the UI account strip and can cause phantom duplicate entries when the user later reconnects the same address. The same issue exists in templates/calendar/server/lib/google-calendar.ts line 78. Fix: on migration failure, delete the legacy file without writing a fallback rather than persisting an unattributable token.
How did I do? React with 👍 or 👎 to help me improve.
There was a problem hiding this comment.
Fixed — migration no longer writes fallback files. If email can't be resolved (offline/no email returned), the legacy file is deleted without persisting an unattributable token.
| if (status === 404) continue; | ||
| // Surface auth/rate-limit/other errors immediately | ||
| console.error("[getEmail] Gmail error:", error.message); | ||
| res.status(status || 500).json({ error: error.message }); |
There was a problem hiding this comment.
error?.code (Node.js string) passed to res.status() — crashes response on network errors
🟡 Medium Priority
error?.code for network-level failures (ECONNRESET, ETIMEDOUT, ENOTFOUND, etc.) is a truthy Node.js string constant, so status || 500 evaluates to the string rather than 500; res.status('ECONNRESET') then throws ERR_HTTP_INVALID_STATUS_CODE and drops the connection instead of returning a clean error response. Fix: guard with typeof error?.response?.status === 'number' before using it, and fall back to 502 for network-level errors.
How did I do? React with 👍 or 👎 to help me improve.
There was a problem hiding this comment.
Fixed — now checks typeof error?.response?.status === 'number' and falls back to 502 for network-level errors.
- Add getClient to emails.ts import (was missing, crashed markRead) - Fix error?.code string passed to res.status() — use numeric check + 502 fallback - Migration: don't write phantom fallback files, just delete legacy on failure - updateEvent: pass merged object (has accountEmail) instead of raw req.body - listGmailMessages/listEvents: return errors alongside results, surface as 502 when all accounts fail, X-Account-Errors header for partial failures
1 participant
Summary
data/google-accounts/{email}.jsonwith auto-migration from legacy single-account fileaccountEmailfield identifying which account it came fromTest plan
data/google-auth.jsonauto-migrates to new format🤖 Generated with Claude Code