refactor: migrate account emails to dedicated api#1652
refactor: migrate account emails to dedicated api#1652arpitgupta1214 wants to merge 2 commits intotestfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 2 minutes and 53 seconds. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✨ 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 |
arpitgupta1214
force-pushed
the
codex/account-emails-chat
branch
from
a356f2b to
d354453
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
11 issues found across 35 files
Confidence score: 1/5
- High-risk security exposure:
lib/api/elysia/routes/youtube/channelInfo.tsacceptsartist_account_idfrom query params without ownership checks, enabling IDOR access to other users' accounts. - API-key auth can fall through to cookie auth in
lib/api/elysia/plugins/auth.ts, so failed API-key validation may still authenticate, which is a serious auth bypass risk. - Given the two concrete auth/authorization flaws (both high severity), this is likely to break security expectations and is not safe to merge yet.
- Pay close attention to
lib/api/elysia/routes/youtube/channelInfo.tsandlib/api/elysia/plugins/auth.ts- IDOR risk and API-key auth fallback need to be fixed.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="lib/api/elysia/routes/youtube/channelInfo.ts">
<violation number="1" location="lib/api/elysia/routes/youtube/channelInfo.ts:9">
P2: Custom agent: **Module should export a single primary function whose name matches the filename**
Exported name `youtubeChannelInfoRoute` doesn't match the file basename `channelInfo`. Since the file already lives under `routes/youtube/`, the `youtube` prefix is redundant. Rename the export to `channelInfoRoute` (and update the import in the sibling `index.ts`).</violation>
<violation number="2" location="lib/api/elysia/routes/youtube/channelInfo.ts:15">
P0: IDOR vulnerability: `artist_account_id` from query params is passed directly to `validateYouTubeTokens` without verifying the authenticated user owns or has access to that account. The `user` object from `authPlugin` is available but never checked. Any authenticated user can fetch YouTube tokens for any account.</violation>
<violation number="3" location="lib/api/elysia/routes/youtube/channelInfo.ts:19">
P2: Passing an empty string `""` for `refreshToken` when `tokens.refresh_token` is null/undefined. Since the parameter is optional (`refreshToken?: string`), pass `undefined` instead to avoid the downstream OAuth client treating an empty string as a valid refresh token.</violation>
</file>
<file name="hooks/useYoutubeStatus.ts">
<violation number="1" location="hooks/useYoutubeStatus.ts:16">
P1: Custom agent: **Flag AI Slop and Fabricated Changes**
This change drops the `tokenStatus === "valid"` check, so any non-empty channel array now counts as `"valid"` — even when the underlying token is expired or revoked. No test covers the new behavior, and a regression assertion (e.g., verifying that an array of channels with an invalid token still returns `"invalid"`) would be straightforward to add.</violation>
</file>
<file name="lib/api/elysia/routes/agentTemplates/favorites.ts">
<violation number="1" location="lib/api/elysia/routes/agentTemplates/favorites.ts:22">
P2: Remove `console.error` — the project's "No Production Logging" principle (CLAUDE.md) disallows console statements in merged code.</violation>
</file>
<file name="lib/youtube/token-refresher.ts">
<violation number="1" location="lib/youtube/token-refresher.ts:77">
P1: Specific errors thrown inside the `try` block (`REFRESH_INCOMPLETE_CREDENTIALS`, `DB_UPDATE_FAILED`) are caught by the `catch` and silently replaced with `REFRESH_GENERAL_FAILURE`. Add a re-throw guard at the top of the catch block to preserve errors already thrown with specific messages.</violation>
</file>
<file name="lib/supabase/account_api_keys/getAccountIdFromApiKey.ts">
<violation number="1" location="lib/supabase/account_api_keys/getAccountIdFromApiKey.ts:21">
P2: `.limit(1)` before `.maybeSingle()` silently masks duplicate `key_hash` rows. Without `limit(1)`, Supabase would error on >1 matching rows, surfacing a data-integrity problem. Drop `limit(1)` so duplicates are caught instead of hidden.</violation>
</file>
<file name="components/Agents/useAgentToggleFavorite.ts">
<violation number="1" location="components/Agents/useAgentToggleFavorite.ts:12">
P2: Removing the user-authentication guard means unauthenticated calls now hit the network and surface a generic error toast ("Failed to update favorite") instead of being silently skipped. Consider adding a session/auth check (e.g., from your auth provider) so the function early-returns before making a pointless request.</violation>
</file>
<file name="lib/api/elysia/plugins/auth.ts">
<violation number="1" location="lib/api/elysia/plugins/auth.ts:18">
P1: Security: failed API-key auth silently falls through to cookie auth. When a caller explicitly provides `x-api-key`, a validation failure should return 401 immediately rather than trying the privy-token cookie. Otherwise a request intended for one account could be authenticated as a different account via the cookie, creating a confused-deputy scenario.</violation>
</file>
<file name="lib/youtube/token-validator.ts">
<violation number="1" location="lib/youtube/token-validator.ts:42">
P2: Intentional validation throws (`NO_TOKENS`, `EXPIRED_TOKENS_NO_REFRESH`) are inside the `try` block and will be caught, misleadingly logged as errors via `console.error`, then re-thrown. This pollutes error logs with expected validation outcomes and double-logs refresh failures (since `refreshStoredYouTubeToken` already logs its own errors). Narrow the `try` to only wrap the database call, or re-throw known errors before logging.</violation>
</file>
<file name="lib/youtube/channel-fetcher.ts">
<violation number="1" location="lib/youtube/channel-fetcher.ts:36">
P2: This `throw` is inside the `try` block and gets caught by the function's own `catch` handler. The error is logged via `console.error` (misleading for a business-logic "no channels" case) and then re-wrapped into a new `Error`, losing the original stack trace. Move this throw before the try block, or restructure so this case isn't caught by the generic error handler — e.g., check for empty items after the try/catch, or re-throw known errors in the catch without wrapping.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
| "/channel-info", | ||
| async ({ query, status }) => { | ||
| try { | ||
| const tokens = await validateYouTubeTokens(query.artist_account_id); |
There was a problem hiding this comment.
P0: IDOR vulnerability: artist_account_id from query params is passed directly to validateYouTubeTokens without verifying the authenticated user owns or has access to that account. The user object from authPlugin is available but never checked. Any authenticated user can fetch YouTube tokens for any account.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/api/elysia/routes/youtube/channelInfo.ts, line 15:
<comment>IDOR vulnerability: `artist_account_id` from query params is passed directly to `validateYouTubeTokens` without verifying the authenticated user owns or has access to that account. The `user` object from `authPlugin` is available but never checked. Any authenticated user can fetch YouTube tokens for any account.</comment>
<file context>
@@ -0,0 +1,48 @@
+ "/channel-info",
+ async ({ query, status }) => {
+ try {
+ const tokens = await validateYouTubeTokens(query.artist_account_id);
+
+ return await fetchYouTubeChannelInfo({
</file context>
hooks/useYoutubeStatus.ts
Outdated
| return channelResponse.tokenStatus === "valid" | ||
| ? "valid" | ||
| : "invalid"; | ||
| if (Array.isArray(channelResponse) && channelResponse.length > 0) { |
There was a problem hiding this comment.
P1: Custom agent: Flag AI Slop and Fabricated Changes
This change drops the tokenStatus === "valid" check, so any non-empty channel array now counts as "valid" — even when the underlying token is expired or revoked. No test covers the new behavior, and a regression assertion (e.g., verifying that an array of channels with an invalid token still returns "invalid") would be straightforward to add.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At hooks/useYoutubeStatus.ts, line 16:
<comment>This change drops the `tokenStatus === "valid"` check, so any non-empty channel array now counts as `"valid"` — even when the underlying token is expired or revoked. No test covers the new behavior, and a regression assertion (e.g., verifying that an array of channels with an invalid token still returns `"invalid"`) would be straightforward to add.</comment>
<file context>
@@ -13,10 +13,8 @@ const useYoutubeStatus = (artistAccountId?: string) => {
- return channelResponse.tokenStatus === "valid"
- ? "valid"
- : "invalid";
+ if (Array.isArray(channelResponse) && channelResponse.length > 0) {
+ return "valid";
}
</file context>
lib/youtube/token-refresher.ts
Outdated
| 'REFRESH_GENERAL_FAILURE', | ||
| YouTubeErrorMessages.REFRESH_GENERAL_FAILURE | ||
| ); | ||
| throw new Error(YouTubeErrorMessages.REFRESH_GENERAL_FAILURE); |
There was a problem hiding this comment.
P1: Specific errors thrown inside the try block (REFRESH_INCOMPLETE_CREDENTIALS, DB_UPDATE_FAILED) are caught by the catch and silently replaced with REFRESH_GENERAL_FAILURE. Add a re-throw guard at the top of the catch block to preserve errors already thrown with specific messages.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/youtube/token-refresher.ts, line 77:
<comment>Specific errors thrown inside the `try` block (`REFRESH_INCOMPLETE_CREDENTIALS`, `DB_UPDATE_FAILED`) are caught by the `catch` and silently replaced with `REFRESH_GENERAL_FAILURE`. Add a re-throw guard at the top of the catch block to preserve errors already thrown with specific messages.</comment>
<file context>
@@ -92,16 +70,10 @@ export async function refreshStoredYouTubeToken(
- 'REFRESH_GENERAL_FAILURE',
- YouTubeErrorMessages.REFRESH_GENERAL_FAILURE
- );
+ throw new Error(YouTubeErrorMessages.REFRESH_GENERAL_FAILURE);
}
-}
</file context>
lib/api/elysia/plugins/auth.ts
Outdated
| identifier: apiKey, | ||
| }, | ||
| }; | ||
| } catch (error) { |
There was a problem hiding this comment.
P1: Security: failed API-key auth silently falls through to cookie auth. When a caller explicitly provides x-api-key, a validation failure should return 401 immediately rather than trying the privy-token cookie. Otherwise a request intended for one account could be authenticated as a different account via the cookie, creating a confused-deputy scenario.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/api/elysia/plugins/auth.ts, line 18:
<comment>Security: failed API-key auth silently falls through to cookie auth. When a caller explicitly provides `x-api-key`, a validation failure should return 401 immediately rather than trying the privy-token cookie. Otherwise a request intended for one account could be authenticated as a different account via the cookie, creating a confused-deputy scenario.</comment>
<file context>
@@ -0,0 +1,37 @@
+ identifier: apiKey,
+ },
+ };
+ } catch (error) {
+ console.error("Elysia auth apiKey validation failed:", error);
+ }
</file context>
|
|
||
| const errorResponseSchema = t.String(); | ||
|
|
||
| export const youtubeChannelInfoRoute = new Elysia() |
There was a problem hiding this comment.
P2: Custom agent: Module should export a single primary function whose name matches the filename
Exported name youtubeChannelInfoRoute doesn't match the file basename channelInfo. Since the file already lives under routes/youtube/, the youtube prefix is redundant. Rename the export to channelInfoRoute (and update the import in the sibling index.ts).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/api/elysia/routes/youtube/channelInfo.ts, line 9:
<comment>Exported name `youtubeChannelInfoRoute` doesn't match the file basename `channelInfo`. Since the file already lives under `routes/youtube/`, the `youtube` prefix is redundant. Rename the export to `channelInfoRoute` (and update the import in the sibling `index.ts`).</comment>
<file context>
@@ -0,0 +1,48 @@
+
+const errorResponseSchema = t.String();
+
+export const youtubeChannelInfoRoute = new Elysia()
+ .use(authPlugin)
+ .get(
</file context>
| .from("account_api_keys") | ||
| .select("account") | ||
| .eq("key_hash", keyHash) | ||
| .limit(1) |
There was a problem hiding this comment.
P2: .limit(1) before .maybeSingle() silently masks duplicate key_hash rows. Without limit(1), Supabase would error on >1 matching rows, surfacing a data-integrity problem. Drop limit(1) so duplicates are caught instead of hidden.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/supabase/account_api_keys/getAccountIdFromApiKey.ts, line 21:
<comment>`.limit(1)` before `.maybeSingle()` silently masks duplicate `key_hash` rows. Without `limit(1)`, Supabase would error on >1 matching rows, surfacing a data-integrity problem. Drop `limit(1)` so duplicates are caught instead of hidden.</comment>
<file context>
@@ -0,0 +1,36 @@
+ .from("account_api_keys")
+ .select("account")
+ .eq("key_hash", keyHash)
+ .limit(1)
+ .maybeSingle();
+
</file context>
|
|
||
| return await fetchYouTubeChannelInfo({ | ||
| accessToken: tokens.access_token, | ||
| refreshToken: tokens.refresh_token || "", |
There was a problem hiding this comment.
P2: Passing an empty string "" for refreshToken when tokens.refresh_token is null/undefined. Since the parameter is optional (refreshToken?: string), pass undefined instead to avoid the downstream OAuth client treating an empty string as a valid refresh token.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/api/elysia/routes/youtube/channelInfo.ts, line 19:
<comment>Passing an empty string `""` for `refreshToken` when `tokens.refresh_token` is null/undefined. Since the parameter is optional (`refreshToken?: string`), pass `undefined` instead to avoid the downstream OAuth client treating an empty string as a valid refresh token.</comment>
<file context>
@@ -0,0 +1,48 @@
+
+ return await fetchYouTubeChannelInfo({
+ accessToken: tokens.access_token,
+ refreshToken: tokens.refresh_token || "",
+ includeBranding: true,
+ });
</file context>
| nextFavourite: boolean | ||
| ) => { | ||
| if (!userData?.id || !templateId) return; | ||
| if (!templateId) return; |
There was a problem hiding this comment.
P2: Removing the user-authentication guard means unauthenticated calls now hit the network and surface a generic error toast ("Failed to update favorite") instead of being silently skipped. Consider adding a session/auth check (e.g., from your auth provider) so the function early-returns before making a pointless request.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At components/Agents/useAgentToggleFavorite.ts, line 12:
<comment>Removing the user-authentication guard means unauthenticated calls now hit the network and surface a generic error toast ("Failed to update favorite") instead of being silently skipped. Consider adding a session/auth check (e.g., from your auth provider) so the function early-returns before making a pointless request.</comment>
<file context>
@@ -1,22 +1,19 @@
nextFavourite: boolean
) => {
- if (!userData?.id || !templateId) return;
+ if (!templateId) return;
try {
</file context>
lib/youtube/token-validator.ts
Outdated
| } catch (error) { | ||
| console.error('Error validating YouTube tokens:', error); | ||
| return YouTubeErrorBuilder.createUtilityError('FETCH_ERROR', YouTubeErrorMessages.FETCH_ERROR); | ||
| console.error("Error validating YouTube tokens:", error); |
There was a problem hiding this comment.
P2: Intentional validation throws (NO_TOKENS, EXPIRED_TOKENS_NO_REFRESH) are inside the try block and will be caught, misleadingly logged as errors via console.error, then re-thrown. This pollutes error logs with expected validation outcomes and double-logs refresh failures (since refreshStoredYouTubeToken already logs its own errors). Narrow the try to only wrap the database call, or re-throw known errors before logging.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/youtube/token-validator.ts, line 42:
<comment>Intentional validation throws (`NO_TOKENS`, `EXPIRED_TOKENS_NO_REFRESH`) are inside the `try` block and will be caught, misleadingly logged as errors via `console.error`, then re-thrown. This pollutes error logs with expected validation outcomes and double-logs refresh failures (since `refreshStoredYouTubeToken` already logs its own errors). Narrow the `try` to only wrap the database call, or re-throw known errors before logging.</comment>
<file context>
@@ -1,61 +1,50 @@
} catch (error) {
- console.error('Error validating YouTube tokens:', error);
- return YouTubeErrorBuilder.createUtilityError('FETCH_ERROR', YouTubeErrorMessages.FETCH_ERROR);
+ console.error("Error validating YouTube tokens:", error);
+
+ if (error instanceof Error) {
</file context>
lib/youtube/channel-fetcher.ts
Outdated
| "NO_CHANNELS", | ||
| YouTubeErrorMessages.NO_CHANNELS | ||
| ); | ||
| throw new Error(YouTubeErrorMessages.NO_CHANNELS); |
There was a problem hiding this comment.
P2: This throw is inside the try block and gets caught by the function's own catch handler. The error is logged via console.error (misleading for a business-logic "no channels" case) and then re-wrapped into a new Error, losing the original stack trace. Move this throw before the try block, or restructure so this case isn't caught by the generic error handler — e.g., check for empty items after the try/catch, or re-throw known errors in the catch without wrapping.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/youtube/channel-fetcher.ts, line 36:
<comment>This `throw` is inside the `try` block and gets caught by the function's own `catch` handler. The error is logged via `console.error` (misleading for a business-logic "no channels" case) and then re-wrapped into a new `Error`, losing the original stack trace. Move this throw before the try block, or restructure so this case isn't caught by the generic error handler — e.g., check for empty items after the try/catch, or re-throw known errors in the catch without wrapping.</comment>
<file context>
@@ -39,10 +33,7 @@ export async function fetchYouTubeChannelInfo({
- "NO_CHANNELS",
- YouTubeErrorMessages.NO_CHANNELS
- );
+ throw new Error(YouTubeErrorMessages.NO_CHANNELS);
}
</file context>
![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: a356f2bc5d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
lib/api/elysia/plugins/auth.ts
Outdated
| const apiKey = request.headers.get("x-api-key"); | ||
| const privyToken = cookie["privy-token"]?.value as string | undefined; |
There was a problem hiding this comment.
The new auth resolver only reads x-api-key and a privy-token cookie, so requests that authenticate with Authorization: Bearer ... cannot be resolved to a user. This is a regression risk for the app’s existing Privy pattern (client code typically fetches an access token and sends it as a Bearer header), and it can make /api/youtube/channel-info and /api/agent-templates/favorites return 401 for logged-in users in environments where that cookie is not present.
Useful? React with 👍 / 👎.
|
Closing this PR to reopen from a fresh branch so the review thread starts clean without the stale Elysia-related comments. |
1 participant
Summary
/api/account-emailsrouteaccount_idcontract from the frontendTesting
Summary by cubic
Migrated account email lookups to the external accounts API and removed the local
/api/account-emailsroute. Added a typed fetcher and a simplified hook that uses a Privy bearer token; updatedFileInfoDialogandTasksListto use it.app/api/account-emails/route.ts.lib/accounts/fetchAccountEmailsandhooks/useAccountEmailsto callGET /api/accounts/emailswithAuthorization: Bearer <token>and repeatedaccount_idparams.useAccountEmails: takesaccountIds(and optionalenabled), gates on Privy authentication, and uses cleaner query keys; updatedFileInfoDialogandTasksListaccordingly.Written for commit 130d00f. Summary will update on new commits.