feat(desktop): add GitHub OAuth and API-level token exchange#428
feat(desktop): add GitHub OAuth and API-level token exchange#428saddlepaddle merged 12 commits intomainfrom
Conversation
- Simplify /api/auth/desktop/[provider] to redirect to OAuth page - Create /auth/desktop/[provider] page that triggers OAuth immediately - Create /api/auth/desktop/callback to handle post-OAuth auth code creation - Add desktop auth routes to public routes in middleware - Add server-side redirect in (auth) layout for authenticated users - Remove unnecessary sso-callback layout This ensures desktop users always go through OAuth flow regardless of existing web session, allowing them to choose which account to use. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Desktop app now calls Clerk's /oauth/authorize directly (bypasses web app) - Uses PKCE for secure token exchange at Clerk's /oauth/token endpoint - API accepts OAuth tokens via acceptsToken: 'oauth_token' - Added PROTOCOL_SCHEMES to shared package - Removed old desktop auth routes and token exchange endpoints This eliminates the web sign-in page from desktop auth flow entirely. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add prompt=login to force fresh auth (allows switching accounts) - Redirect OAuth callback through web app for branded UI - Create web callback page that redirects to desktop deep link - Update token exchange redirect_uri to match authorize request 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add GitHub OAuth support alongside Google for desktop auth - Implement JWT token exchange at API level (/api/auth/desktop/google, /api/auth/desktop/github) - Add refresh token endpoint for token rotation - Route OAuth through web callbacks for consistent UX - Update CI/CD workflows with OAuth environment variables - Add GITHUB_CLIENT_ID and GITHUB_CLIENT_SECRET to env schemas 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Warning Rate limit exceeded@saddlepaddle has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 47 seconds before requesting another review. ⌛ 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. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
WalkthroughThis pull request implements OAuth-based desktop authentication via Google and GitHub, replacing the previous PKCE-based token exchange flow. It introduces new OAuth callback handlers, token management utilities, state-based CSRF protection, and updates the desktop application's authentication service and UI accordingly. The Clerk webhook is removed in favor of on-demand user synchronization. Changes
Sequence Diagram(s)sequenceDiagram
participant User as Desktop User
participant Desktop as Desktop App
participant Browser as System Browser
participant OAuth as OAuth Provider<br/>(Google/GitHub)
participant WebApp as Web App
participant API as Backend API
participant Clerk as Clerk Service
participant DB as Database
User->>Desktop: Click Sign In (Google/GitHub)
Desktop->>Desktop: Generate state & store in stateStore
Desktop->>Browser: Open OAuth URL with state parameter
Browser->>OAuth: Request authorization
OAuth->>Browser: User grants permission
Browser->>WebApp: Redirect to /api/auth/desktop/{provider}<br/>with code & state
WebApp->>API: POST /api/auth/desktop/{provider}<br/>(code, redirectUri)
API->>OAuth: Exchange code for access token
OAuth-->>API: Return access token & user info
API->>Clerk: Find or create user by email
Clerk-->>API: User ID
API->>API: Generate JWT tokens<br/>(access + refresh)
API-->>WebApp: Return tokens & expirations
WebApp->>Browser: Redirect to superset-dev://auth/callback<br/>?accessToken=...&state=...
Browser->>Desktop: Trigger deep link handler
Desktop->>Desktop: Parse tokens from URL
Desktop->>Desktop: Verify state matches stored state
Desktop->>Desktop: Clear state from stateStore
Desktop->>Desktop: Store tokens in secure storage
Desktop->>Desktop: Emit authentication success
rect rgb(200, 220, 250)
Note over Desktop: Subsequent API Calls
Desktop->>API: Request with Bearer token
API->>API: Verify JWT token
API-->>Desktop: Return authenticated response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas requiring extra attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
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 |
saddlepaddle
had a problem deploying
to
preview
GitHub Actions
Failure
saddlepaddle
had a problem deploying
to
preview
GitHub Actions
Failure
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (18)
.env.example (1)
38-44: Optional: Consider alphabetically ordering OAuth credential keys.The static analysis tool suggests ordering
GITHUB_CLIENT_IDandGITHUB_CLIENT_SECRETbeforeGOOGLE_CLIENT_IDto maintain alphabetical order. However, the current ordering (Google before GitHub) may be intentional based on implementation priority.🔎 Alternative ordering (alphabetical by provider)
# ----------------------------------------------------------------------------- # OAuth Credentials (for Desktop App direct auth) # ----------------------------------------------------------------------------- +GITHUB_CLIENT_ID= +GITHUB_CLIENT_SECRET= GOOGLE_CLIENT_ID= GOOGLE_CLIENT_SECRET= -GITHUB_CLIENT_ID= -GITHUB_CLIENT_SECRET=apps/desktop/src/renderer/screens/sign-in/index.tsx (1)
10-13: Consider adding minimal error feedback for failed sign-in attempts.The mutation no longer has error handling callbacks. While the OAuth flow opens in an external browser (so loading state may be less critical), users won't receive feedback if the browser fails to open or the mutation errors.
Suggested improvement
-const signInMutation = trpc.auth.signIn.useMutation(); +const signInMutation = trpc.auth.signIn.useMutation({ + onError: (error) => { + console.error("[sign-in] Failed to initiate OAuth:", error.message); + // Optionally show a toast or error message to user + }, +});.github/workflows/release-desktop.yml (1)
61-64: Inconsistent secret naming between Google and GitHub OAuth credentials.Line 62 maps
GITHUB_CLIENT_IDtosecrets.GH_CLIENT_ID, while Line 61 consistently mapsGOOGLE_CLIENT_IDtosecrets.GOOGLE_CLIENT_ID. This naming inconsistency could cause confusion during maintenance and secret management.🔎 Recommended fix for consistency
Consider using consistent naming:
- GITHUB_CLIENT_ID: ${{ secrets.GH_CLIENT_ID }} + GITHUB_CLIENT_ID: ${{ secrets.GITHUB_CLIENT_ID }}Or update the environment variable name to match the secret:
- GITHUB_CLIENT_ID: ${{ secrets.GH_CLIENT_ID }} + GH_CLIENT_ID: ${{ secrets.GH_CLIENT_ID }}Note: This change would require updating the corresponding secret name in the repository settings or updating code that reads this environment variable.
.github/workflows/deploy-preview.yml (1)
130-133: Inconsistent secret naming between Google and GitHub OAuth credentials.Lines 132-133 and 149-150 map
GITHUB_CLIENT_IDandGITHUB_CLIENT_SECRETto secrets prefixed withGH_, while Lines 130-131 and 147-148 consistently mapGOOGLE_CLIENT_IDandGOOGLE_CLIENT_SECRETto secrets with the fullGOOGLE_prefix. This naming inconsistency appears in both the environment declaration and the Vercel deploy command, increasing the risk of configuration errors.🔎 Recommended fix for consistency
Consider standardizing the naming pattern. Option 1 - Use full names consistently:
- GITHUB_CLIENT_ID: ${{ secrets.GH_CLIENT_ID }} - GITHUB_CLIENT_SECRET: ${{ secrets.GH_CLIENT_SECRET }} + GITHUB_CLIENT_ID: ${{ secrets.GITHUB_CLIENT_ID }} + GITHUB_CLIENT_SECRET: ${{ secrets.GITHUB_CLIENT_SECRET }}And in the deploy command:
- --env GITHUB_CLIENT_ID=$GITHUB_CLIENT_ID \ - --env GITHUB_CLIENT_SECRET=$GITHUB_CLIENT_SECRET) + --env GITHUB_CLIENT_ID=$GITHUB_CLIENT_ID \ + --env GITHUB_CLIENT_SECRET=$GITHUB_CLIENT_SECRET)Note: This requires updating the secret names in repository settings to use
GITHUB_CLIENT_IDandGITHUB_CLIENT_SECRETinstead ofGH_CLIENT_IDandGH_CLIENT_SECRET.Also applies to: 146-150
packages/trpc/src/router/user/user.ts (1)
11-19: Consider explicit error handling for user sync failures.The current implementation returns
nullifsyncUserFromClerkfails (e.g., when no primary email is found). For a "me" endpoint, consider throwing an explicit error to communicate the failure reason to the client, rather than silently returningnull.🔎 Example with explicit error handling
me: protectedProcedure.query(async ({ ctx }) => { const existingUser = await db.query.users.findFirst({ where: eq(users.clerkId, ctx.userId), }); if (existingUser) { return existingUser; } - return syncUserFromClerk(ctx.userId); + const syncedUser = await syncUserFromClerk(ctx.userId); + + if (!syncedUser) { + throw new Error("Failed to sync user from Clerk. No primary email found."); + } + + return syncedUser; }),This makes failures more explicit and provides better debugging information.
apps/api/src/app/api/auth/desktop/github/route.ts (1)
44-48: Use proper input validation instead of type assertion.The
body as { code: string; redirectUri: string }bypasses type safety. While the subsequentif (!code || !redirectUri)check helps, consider using Zod for consistent validation.🔎 Proposed fix using Zod validation
+import { z } from "zod"; + +const RequestSchema = z.object({ + code: z.string().min(1), + redirectUri: z.string().url(), +}); + export async function POST(request: Request) { try { - const body = await request.json(); - const { code, redirectUri } = body as { - code: string; - redirectUri: string; - }; - - if (!code || !redirectUri) { - return Response.json( - { error: "Missing code or redirectUri" }, - { status: 400 }, - ); - } + const body = await request.json(); + const parsed = RequestSchema.safeParse(body); + + if (!parsed.success) { + return Response.json( + { error: "Invalid request body", details: parsed.error.flatten() }, + { status: 400 }, + ); + } + + const { code, redirectUri } = parsed.data;apps/api/src/trpc/context.ts (1)
17-22: Validatepayload.subbefore returning.The JWT
subclaim is optional per spec. If a malformed token lackssub, the function returnsundefinedcast tostring, which could cause downstream issues.🔎 Proposed fix
// Only accept access tokens for API authentication if (payload.type !== "access") { return null; } - return payload.sub as string; + if (typeof payload.sub !== "string") { + return null; + } + + return payload.sub; } catch {apps/api/src/app/api/auth/desktop/tokens.ts (1)
69-77: Validate payload claims before type assertion.Similar to
verifyDesktopTokenin context.ts,payload.subandpayload.emailare optional per JWT spec. Validate their presence and type to avoid returningundefinedcast tostring.🔎 Proposed fix
// Ensure it's a refresh token if (payload.type !== "refresh") { return null; } + if (typeof payload.sub !== "string" || typeof payload.email !== "string") { + return null; + } + return { - userId: payload.sub as string, - email: payload.email as string, + userId: payload.sub, + email: payload.email, }; } catch {apps/api/src/app/api/auth/desktop/google/route.ts (1)
146-163: Consider sanitizing error details before returning to client.Returning raw Clerk error details (line 159) may expose internal implementation information to clients. Log the full details server-side but return a generic message to the client.
🔎 Proposed fix
} catch (clerkError: unknown) { // Log and return detailed Clerk error const errorDetails = clerkError && typeof clerkError === "object" && "errors" in clerkError ? (clerkError as { errors: unknown[] }).errors : clerkError; console.error( "[auth/google] Clerk createUser failed:", JSON.stringify(errorDetails, null, 2), ); return Response.json( { error: "Failed to create user account", - details: errorDetails, }, { status: 400 }, ); }apps/web/src/app/auth/desktop/success/page.tsx (1)
102-109: Redundant null check fordesktopUrl.At this point in the render,
hasAllTokensis truthy (checked at line 67), which meansdesktopUrlis guaranteed to be non-null per line 31-33. The conditional on line 102 is unnecessary.🔎 Proposed simplification
- {desktopUrl && ( - <Link - href={desktopUrl} - className="text-muted-foreground/70 underline decoration-muted-foreground/40 underline-offset-4 transition-colors hover:text-muted-foreground" - > - If you weren't redirected, click here. - </Link> - )} + <Link + href={desktopUrl} + className="text-muted-foreground/70 underline decoration-muted-foreground/40 underline-offset-4 transition-colors hover:text-muted-foreground" + > + If you weren't redirected, click here. + </Link>apps/web/src/app/api/auth/desktop/github/route.ts (1)
36-41: Consider extracting shared token response type.The
tokenDatatype is duplicated between the Google and GitHub routes (seeapps/web/src/app/api/auth/desktop/google/route.tslines 37-41). Extract to a shared types file to maintain consistency.// In a shared types file, e.g., apps/web/src/app/api/auth/desktop/types.ts export interface DesktopTokenResponse { accessToken: string; accessTokenExpiresAt: number; refreshToken: string; refreshTokenExpiresAt: number; }packages/trpc/src/router/user/utils/sync-user-from-clerk.ts (1)
13-30: Avatar upload assumes PNG format regardless of source image type.The avatar is saved as
avatar.png(line 22) regardless of the actual image format from Clerk. While most browsers/services handle this gracefully, consider:
- Preserving the original extension, or
- Using a generic name without extension and relying on content-type headers
🔎 Proposed fix to detect content type
async function uploadAvatar( imageUrl: string, userId: string, ): Promise<string | null> { try { const response = await fetch(imageUrl); if (!response.ok) return null; const blob = await response.blob(); - const { url } = await put(`users/${userId}/avatar.png`, blob, { + const contentType = response.headers.get("content-type") || "image/png"; + const ext = contentType.includes("jpeg") || contentType.includes("jpg") ? "jpg" : "png"; + const { url } = await put(`users/${userId}/avatar.${ext}`, blob, { access: "public", token: env.BLOB_READ_WRITE_TOKEN, + contentType, }); return url; } catch { return null; } }apps/api/src/app/api/auth/desktop/refresh/route.ts (2)
15-16: Type assertion without validation can lead to runtime errors.The type assertion
as { refreshToken: string }doesn't provide runtime safety. If the request body has a different shape orrefreshTokenis present but not a string, the code will proceed with an invalid value.Consider validating the type before use:
🔎 Suggested validation approach
const body = await request.json(); - const { refreshToken } = body as { refreshToken: string }; + const { refreshToken } = body; - if (!refreshToken) { + if (!refreshToken || typeof refreshToken !== "string") { return Response.json({ error: "Missing refresh token" }, { status: 400 }); }
10-11: Doc comment is provider-specific but endpoint is generic.The comment mentions "Google OAuth" specifically, but this refresh endpoint works for both Google and GitHub providers as indicated by the PR objectives.
🔎 Suggested fix
- * This endpoint allows the desktop app to get new tokens without - * requiring the user to re-authenticate through Google OAuth. + * This endpoint allows the desktop app to get new tokens without + * requiring the user to re-authenticate through OAuth.apps/desktop/src/main/lib/auth/auth-service.ts (4)
39-39: Import statement placed in middle of file.The
tokenStorageimport is placed between the CSRF functions and the class definition, breaking the conventional import grouping at the top of the file.🔎 Suggested fix
Move this import to the top with the other imports:
import crypto from "node:crypto"; import { EventEmitter } from "node:events"; import { type BrowserWindow, shell } from "electron"; import { env } from "main/env.main"; import type { AuthProvider, AuthSession, SignInResult } from "shared/auth"; +import { tokenStorage } from "./token-storage";And remove line 39.
134-158: Add timeout to prevent indefinite hanging on network issues.The fetch call has no timeout, which could cause the refresh operation to hang indefinitely if the server is unresponsive. Additionally, the type assertion at line 153 lacks runtime validation.
🔎 Suggested fix with timeout and validation
try { + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), 10000); + const response = await fetch( `${env.NEXT_PUBLIC_API_URL}/api/auth/desktop/refresh`, { method: "POST", headers: { "Content-Type": "application/json", }, body: JSON.stringify({ refreshToken: this.session.refreshToken, }), + signal: controller.signal, }, ); + + clearTimeout(timeoutId); if (!response.ok) { console.error("[auth] Token refresh failed:", response.status); return false; } - const tokens = (await response.json()) as { - accessToken: string; - accessTokenExpiresAt: number; - refreshToken: string; - refreshTokenExpiresAt: number; - }; + const tokens = await response.json(); + + if ( + typeof tokens.accessToken !== "string" || + typeof tokens.refreshToken !== "string" || + typeof tokens.accessTokenExpiresAt !== "number" || + typeof tokens.refreshTokenExpiresAt !== "number" + ) { + console.error("[auth] Invalid token response shape"); + return false; + }
256-258: Log message is provider-specific but method handles both providers.The log message at line 258 says "via Google OAuth" but
handleAuthCallbackis called for both Google and GitHub flows.🔎 Suggested fix
- console.log("[auth] Signed in via Google OAuth with refresh token"); + console.log("[auth] Signed in via OAuth with refresh token");
97-124: Potential race condition if getAccessToken is called concurrently.If multiple callers invoke
getAccessToken()while the access token is expired, each could triggerrefreshTokens()concurrently, leading to multiple refresh requests and potential token invalidation issues if the server invalidates the old refresh token on use.Consider adding a refresh lock or promise deduplication.
🔎 Suggested approach
class AuthService extends EventEmitter { private session: AuthSession | null = null; + private refreshPromise: Promise<boolean> | null = null; // ... in refreshTokens(): private async refreshTokens(): Promise<boolean> { if (!this.session?.refreshToken) { return false; } + // Deduplicate concurrent refresh calls + if (this.refreshPromise) { + return this.refreshPromise; + } + + this.refreshPromise = this.doRefreshTokens(); + try { + return await this.refreshPromise; + } finally { + this.refreshPromise = null; + } + } + + private async doRefreshTokens(): Promise<boolean> { try { // ... existing fetch logic
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (43)
.env.example(1 hunks).github/workflows/deploy-preview.yml(2 hunks).github/workflows/deploy-production.yml(2 hunks).github/workflows/release-desktop.yml(1 hunks)apps/api/src/app/api/auth/desktop/github/route.ts(1 hunks)apps/api/src/app/api/auth/desktop/google/route.ts(1 hunks)apps/api/src/app/api/auth/desktop/refresh/route.ts(1 hunks)apps/api/src/app/api/auth/desktop/token/route.ts(0 hunks)apps/api/src/app/api/auth/desktop/tokens.ts(1 hunks)apps/api/src/app/api/webhooks/clerk/route.ts(0 hunks)apps/api/src/env.ts(1 hunks)apps/api/src/trpc/context.ts(1 hunks)apps/api/src/trpc/utils/verifyDesktopToken.ts(0 hunks)apps/desktop/scripts/patch-dev-protocol.ts(1 hunks)apps/desktop/src/main/env.main.ts(1 hunks)apps/desktop/src/main/index.ts(1 hunks)apps/desktop/src/main/lib/auth/auth-service.ts(4 hunks)apps/desktop/src/main/lib/auth/deep-link-handler.ts(3 hunks)apps/desktop/src/renderer/screens/main/index.tsx(4 hunks)apps/desktop/src/renderer/screens/sign-in/index.tsx(2 hunks)apps/desktop/src/shared/constants.ts(2 hunks)apps/web/src/app/(auth)/layout.tsx(1 hunks)apps/web/src/app/(auth)/sign-in/[[...sign-in]]/page.tsx(2 hunks)apps/web/src/app/(auth)/sso-callback/layout.tsx(0 hunks)apps/web/src/app/api/auth/desktop/[provider]/route.ts(0 hunks)apps/web/src/app/api/auth/desktop/github/route.ts(1 hunks)apps/web/src/app/api/auth/desktop/google/route.ts(1 hunks)apps/web/src/app/auth/desktop/success/page.tsx(5 hunks)apps/web/src/proxy.ts(1 hunks)packages/shared/src/constants.ts(1 hunks)packages/trpc/package.json(1 hunks)packages/trpc/src/env.ts(1 hunks)packages/trpc/src/router/admin/admin.ts(1 hunks)packages/trpc/src/router/admin/index.ts(1 hunks)packages/trpc/src/router/organization/index.ts(1 hunks)packages/trpc/src/router/organization/organization.ts(1 hunks)packages/trpc/src/router/repository/index.ts(1 hunks)packages/trpc/src/router/repository/repository.ts(1 hunks)packages/trpc/src/router/task/index.ts(1 hunks)packages/trpc/src/router/task/task.ts(1 hunks)packages/trpc/src/router/user/index.ts(1 hunks)packages/trpc/src/router/user/user.ts(1 hunks)packages/trpc/src/router/user/utils/sync-user-from-clerk.ts(1 hunks)
💤 Files with no reviewable changes (5)
- apps/web/src/app/(auth)/sso-callback/layout.tsx
- apps/api/src/app/api/auth/desktop/token/route.ts
- apps/api/src/app/api/webhooks/clerk/route.ts
- apps/api/src/trpc/utils/verifyDesktopToken.ts
- apps/web/src/app/api/auth/desktop/[provider]/route.ts
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Avoid using any type in TypeScript - maintain type safety unless absolutely necessary
Files:
packages/trpc/src/router/task/index.tspackages/trpc/src/router/admin/index.tspackages/trpc/src/router/admin/admin.tspackages/trpc/src/router/repository/repository.tspackages/trpc/src/router/user/index.tspackages/trpc/src/router/repository/index.tspackages/trpc/src/router/task/task.tsapps/api/src/app/api/auth/desktop/github/route.tsapps/api/src/app/api/auth/desktop/google/route.tsapps/web/src/app/api/auth/desktop/google/route.tsapps/web/src/proxy.tsapps/api/src/app/api/auth/desktop/tokens.tsapps/web/src/app/api/auth/desktop/github/route.tsapps/api/src/env.tspackages/shared/src/constants.tsapps/desktop/src/renderer/screens/main/index.tsxapps/desktop/src/shared/constants.tsapps/desktop/scripts/patch-dev-protocol.tsapps/api/src/app/api/auth/desktop/refresh/route.tspackages/trpc/src/router/user/utils/sync-user-from-clerk.tspackages/trpc/src/router/user/user.tspackages/trpc/src/router/organization/organization.tsapps/desktop/src/renderer/screens/sign-in/index.tsxpackages/trpc/src/router/organization/index.tsapps/desktop/src/main/lib/auth/deep-link-handler.tsapps/api/src/trpc/context.tsapps/web/src/app/(auth)/layout.tsxapps/web/src/app/(auth)/sign-in/[[...sign-in]]/page.tsxapps/web/src/app/auth/desktop/success/page.tsxapps/desktop/src/main/lib/auth/auth-service.tsapps/desktop/src/main/env.main.tspackages/trpc/src/env.tsapps/desktop/src/main/index.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Run Biome for formatting, linting, import organization, and safe fixes at the root level using bun run lint:fix
Files:
packages/trpc/src/router/task/index.tspackages/trpc/src/router/admin/index.tspackages/trpc/src/router/admin/admin.tspackages/trpc/src/router/repository/repository.tspackages/trpc/src/router/user/index.tspackages/trpc/src/router/repository/index.tspackages/trpc/src/router/task/task.tsapps/api/src/app/api/auth/desktop/github/route.tsapps/api/src/app/api/auth/desktop/google/route.tsapps/web/src/app/api/auth/desktop/google/route.tsapps/web/src/proxy.tsapps/api/src/app/api/auth/desktop/tokens.tsapps/web/src/app/api/auth/desktop/github/route.tsapps/api/src/env.tspackages/shared/src/constants.tsapps/desktop/src/renderer/screens/main/index.tsxapps/desktop/src/shared/constants.tsapps/desktop/scripts/patch-dev-protocol.tsapps/api/src/app/api/auth/desktop/refresh/route.tspackages/trpc/src/router/user/utils/sync-user-from-clerk.tspackages/trpc/src/router/user/user.tspackages/trpc/src/router/organization/organization.tsapps/desktop/src/renderer/screens/sign-in/index.tsxpackages/trpc/src/router/organization/index.tsapps/desktop/src/main/lib/auth/deep-link-handler.tsapps/api/src/trpc/context.tsapps/web/src/app/(auth)/layout.tsxapps/web/src/app/(auth)/sign-in/[[...sign-in]]/page.tsxapps/web/src/app/auth/desktop/success/page.tsxapps/desktop/src/main/lib/auth/auth-service.tsapps/desktop/src/main/env.main.tspackages/trpc/src/env.tsapps/desktop/src/main/index.ts
apps/api/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Drizzle ORM for all database operations
Files:
apps/api/src/app/api/auth/desktop/github/route.tsapps/api/src/app/api/auth/desktop/google/route.tsapps/api/src/app/api/auth/desktop/tokens.tsapps/api/src/env.tsapps/api/src/app/api/auth/desktop/refresh/route.tsapps/api/src/trpc/context.ts
apps/desktop/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx,js,jsx}: For Electron interprocess communication, ALWAYS use trpc as defined insrc/lib/trpc
Use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary.
Files:
apps/desktop/src/renderer/screens/main/index.tsxapps/desktop/src/shared/constants.tsapps/desktop/scripts/patch-dev-protocol.tsapps/desktop/src/renderer/screens/sign-in/index.tsxapps/desktop/src/main/lib/auth/deep-link-handler.tsapps/desktop/src/main/lib/auth/auth-service.tsapps/desktop/src/main/env.main.tsapps/desktop/src/main/index.ts
apps/desktop/src/renderer/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Never import Node.js modules in renderer process or shared code - use only in main process (src/main/)
Files:
apps/desktop/src/renderer/screens/main/index.tsxapps/desktop/src/renderer/screens/sign-in/index.tsx
apps/desktop/src/{main,renderer,preload}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use type-safe IPC communication - define channel types in apps/desktop/src/shared/ipc-channels.ts before implementing handlers
Files:
apps/desktop/src/renderer/screens/main/index.tsxapps/desktop/src/renderer/screens/sign-in/index.tsxapps/desktop/src/main/lib/auth/deep-link-handler.tsapps/desktop/src/main/lib/auth/auth-service.tsapps/desktop/src/main/env.main.tsapps/desktop/src/main/index.ts
**/*.{tsx,css}
📄 CodeRabbit inference engine (AGENTS.md)
Use React + TailwindCSS v4 + shadcn/ui for UI development
Files:
apps/desktop/src/renderer/screens/main/index.tsxapps/desktop/src/renderer/screens/sign-in/index.tsxapps/web/src/app/(auth)/layout.tsxapps/web/src/app/(auth)/sign-in/[[...sign-in]]/page.tsxapps/web/src/app/auth/desktop/success/page.tsx
apps/desktop/src/main/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Accept object parameters in IPC handlers - do not use positional parameters in ipcMain.handle()
Files:
apps/desktop/src/main/lib/auth/deep-link-handler.tsapps/desktop/src/main/lib/auth/auth-service.tsapps/desktop/src/main/env.main.tsapps/desktop/src/main/index.ts
apps/desktop/src/main/index.ts
📄 CodeRabbit inference engine (AGENTS.md)
Load environment variables in src/main/index.ts and electron.vite.config.ts with override: true before other imports
Files:
apps/desktop/src/main/index.ts
🧠 Learnings (6)
📚 Learning: 2025-12-18T17:26:38.664Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-12-18T17:26:38.664Z
Learning: Applies to apps/desktop/**/*.{ts,tsx,js,jsx} : For Electron interprocess communication, ALWAYS use trpc as defined in `src/lib/trpc`
Applied to files:
packages/trpc/src/router/admin/admin.tsapps/desktop/scripts/patch-dev-protocol.ts
📚 Learning: 2025-12-18T23:19:10.405Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-18T23:19:10.405Z
Learning: Applies to apps/api/src/**/*.{ts,tsx} : Use Drizzle ORM for all database operations
Applied to files:
packages/trpc/src/router/admin/admin.ts
📚 Learning: 2025-12-18T23:19:10.405Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-18T23:19:10.405Z
Learning: Applies to packages/db/src/**/*.{ts,tsx} : Store database schema in packages/db/src/ and use Drizzle ORM for schema management
Applied to files:
packages/trpc/src/router/admin/admin.ts
📚 Learning: 2025-12-18T23:19:10.405Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-18T23:19:10.405Z
Learning: Applies to apps/*/app/**/proxy.ts : Never create middleware.ts in Next.js 16 - always use proxy.ts for request interception
Applied to files:
apps/web/src/proxy.ts
📚 Learning: 2025-12-18T23:19:10.405Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-18T23:19:10.405Z
Learning: Applies to apps/desktop/src/main/index.ts : Load environment variables in src/main/index.ts and electron.vite.config.ts with override: true before other imports
Applied to files:
apps/desktop/scripts/patch-dev-protocol.tsapps/desktop/src/main/env.main.ts
📚 Learning: 2025-12-18T23:19:10.405Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-18T23:19:10.405Z
Learning: Applies to apps/desktop/src/main/**/*.ts : Accept object parameters in IPC handlers - do not use positional parameters in ipcMain.handle()
Applied to files:
apps/desktop/src/main/index.ts
🧬 Code graph analysis (12)
apps/api/src/app/api/auth/desktop/github/route.ts (3)
apps/api/src/app/api/auth/desktop/google/route.ts (1)
POST(42-174)apps/api/src/env.ts (1)
env(4-28)apps/api/src/app/api/auth/desktop/tokens.ts (1)
generateTokens(18-54)
apps/web/src/app/api/auth/desktop/google/route.ts (1)
apps/web/src/app/api/auth/desktop/github/route.ts (1)
GET(14-93)
apps/api/src/app/api/auth/desktop/tokens.ts (1)
apps/api/src/env.ts (1)
env(4-28)
apps/web/src/app/api/auth/desktop/github/route.ts (1)
apps/web/src/app/api/auth/desktop/google/route.ts (1)
GET(14-93)
packages/shared/src/constants.ts (1)
apps/desktop/src/shared/constants.ts (1)
PROTOCOL_SCHEMES(30-30)
apps/desktop/src/renderer/screens/main/index.tsx (3)
apps/desktop/src/renderer/screens/main/components/Background.tsx (1)
Background(1-39)apps/desktop/src/renderer/screens/main/components/AppFrame.tsx (1)
AppFrame(5-7)apps/desktop/src/renderer/screens/sign-in/index.tsx (1)
SignInScreen(9-80)
apps/api/src/app/api/auth/desktop/refresh/route.ts (1)
apps/api/src/app/api/auth/desktop/tokens.ts (2)
verifyRefreshToken(59-81)generateTokens(18-54)
packages/trpc/src/router/user/user.ts (3)
packages/db/src/index.ts (1)
eq(1-1)packages/db/src/schema/schema.ts (1)
users(15-35)packages/trpc/src/router/user/utils/sync-user-from-clerk.ts (1)
syncUserFromClerk(36-83)
apps/desktop/src/renderer/screens/sign-in/index.tsx (1)
apps/desktop/src/main/lib/auth/auth-service.ts (1)
signIn(181-228)
apps/api/src/trpc/context.ts (4)
apps/api/src/env.ts (1)
env(4-28)apps/web/src/env.ts (1)
env(5-44)packages/trpc/src/trpc.ts (1)
createTRPCContext(20-24)packages/trpc/src/index.ts (1)
createTRPCContext(9-9)
packages/trpc/src/env.ts (4)
apps/api/src/env.ts (1)
env(4-28)apps/admin/src/env.ts (1)
env(5-41)apps/marketing/src/env.ts (1)
env(5-34)apps/web/src/env.ts (1)
env(5-44)
apps/desktop/src/main/index.ts (2)
apps/desktop/src/main/lib/auth/auth-service.ts (1)
authService(308-308)apps/desktop/src/main/lib/auth/index.ts (1)
authService(1-1)
🪛 dotenv-linter (4.0.0)
.env.example
[warning] 43-43: [UnorderedKey] The GITHUB_CLIENT_ID key should go before the GOOGLE_CLIENT_ID key
(UnorderedKey)
[warning] 44-44: [UnorderedKey] The GITHUB_CLIENT_SECRET key should go before the GOOGLE_CLIENT_ID key
(UnorderedKey)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Deploy Marketing
- GitHub Check: Deploy API
- GitHub Check: Deploy Admin
- GitHub Check: Deploy Docs
- GitHub Check: Deploy Web
- GitHub Check: Build
🔇 Additional comments (44)
packages/trpc/src/router/repository/repository.ts (1)
6-6: LGTM! Import path refactoring aligns with centralized TRPC structure.The updated import path from
"../trpc"to"../../trpc"is consistent with the broader refactoring across all TRPC router modules to use a centralized export surface.packages/trpc/src/router/task/task.ts (1)
7-7: LGTM! Import path refactoring aligns with centralized TRPC structure.The updated import path is consistent with the broader refactoring across all TRPC router modules.
apps/web/src/app/(auth)/sign-in/[[...sign-in]]/page.tsx (1)
7-7: LGTM! Dynamic redirect URL support for desktop OAuth flow.The addition of query parameter-based redirect URL handling enables the desktop OAuth flow to redirect users back to the desktop app after authentication. The implementation correctly:
- Extracts
redirect_urlfrom query parameters- Provides a sensible default of
"/"- Passes the redirect URL to Clerk's OAuth flow
Also applies to: 14-14, 18-19, 31-31
packages/trpc/src/env.ts (1)
2-2: LGTM! Runtime environment validation for critical secrets.The addition of Zod-based validation for
CLERK_SECRET_KEYandBLOB_READ_WRITE_TOKENenforces that these critical secrets are present and non-empty at runtime, preventing deployment issues.Also applies to: 5-8
apps/web/src/proxy.ts (1)
8-9: LGTM! Public routes for desktop OAuth authentication flow.The addition of
/auth/desktop(.*)and/api/auth/desktop(.*)as public routes correctly allows unauthenticated access to the desktop OAuth callback and token exchange endpoints, which is necessary for the authentication flow to complete.apps/desktop/scripts/patch-dev-protocol.ts (1)
16-22: LGTM! Pragmatic solution to avoid env validation during build scripts.Defining
PROTOCOL_SCHEMESlocally in the predev script avoids triggering environment validation at build time, which would otherwise fail when environment variables are not yet configured. The clear comment explains the rationale, and the minimal code duplication is justified for this build-time tooling context..github/workflows/deploy-production.yml (1)
98-102: OAuth credentials are correctly propagated to Vercel deployment.All OAuth environment variables (DESKTOP_AUTH_SECRET, GOOGLE_CLIENT_ID, GOOGLE_CLIENT_SECRET, GITHUB_CLIENT_ID, GITHUB_CLIENT_SECRET) are properly sourced from GitHub repository secrets and passed to the Vercel deploy command via
--envflags. Ensure the corresponding GitHub repository secrets are configured with the exact names referenced in the workflow.apps/desktop/src/renderer/screens/sign-in/index.tsx (1)
35-53: LGTM - Static icons are appropriate for this flow.Since the OAuth flow opens in an external browser via
shell.openExternal(), showing a loading spinner in the desktop app would be misleading. The simplified UI makes sense for this authentication pattern.apps/desktop/src/main/lib/auth/deep-link-handler.ts (3)
6-14: Interface clearly models the token-based callback response.The updated interface properly captures all token fields with appropriate types. The optional
statefield for CSRF verification is included.
113-127: LGTM - Helper correctly identifies auth deep links.The function properly validates both protocol schemes and the "auth" host.
41-101: Token validation logic is thorough and correct.The extraction and validation properly handles:
- Missing token/expiration checks with specific error messages
- Safe integer parsing with
Number.parseIntandNumber.isNaNvalidation- State parameter presence check
State validation is correctly implemented in the caller:
processDeepLink()passes the extracted state toauthService.handleAuthCallback(), which validates it viaverifyState()before processing the authentication. The CSRF protection flow is complete.packages/trpc/src/router/task/index.ts (1)
1-1: LGTM - Consistent barrel export pattern.This follows the established convention for router index files in the codebase.
packages/trpc/src/router/admin/admin.ts (1)
7-7: LGTM - Import path correctly updated for module reorganization.The path change aligns with the barrel export structure introduced in sibling router modules.
packages/trpc/src/router/repository/index.ts (1)
1-1: LGTM - Consistent barrel export pattern.Mirrors the structure of other router index files (
admin/index.ts,task/index.ts).packages/trpc/src/router/organization/organization.ts (1)
6-6: LGTM!The import path update correctly reflects the module reorganization and aligns with the centralized TRPC helpers pattern.
packages/trpc/src/router/user/index.ts (1)
1-1: LGTM!The barrel export establishes a clean public API surface for the user router, consistent with the module organization pattern used across other routers.
packages/trpc/src/router/organization/index.ts (1)
1-1: LGTM!The barrel export follows the same pattern established for other routers, providing a consistent public API surface.
packages/trpc/src/router/user/user.ts (1)
6-7: LGTM!The import path update aligns with the module reorganization, and the new
syncUserFromClerkimport correctly enables Clerk-based user synchronization.packages/trpc/src/router/admin/index.ts (1)
1-1: LGTM!The barrel export maintains consistency with the module organization pattern established across all router modules.
packages/trpc/package.json (1)
22-22: No issues found. @vercel/blob@2.0.0 is the latest stable version with no known security vulnerabilities, and the caret range allows appropriate patch and minor updates.packages/shared/src/constants.ts (1)
5-9: LGTM!The
PROTOCOL_SCHEMESconstant is well-structured with theas constassertion for type safety, follows the existing file conventions, and appropriately centralizes the deep-link protocol configuration for desktop OAuth callbacks.apps/desktop/src/main/env.main.ts (1)
19-22: Verify required OAuth client IDs won't break development environments.These environment variables are marked as required with
z.string().min(1), meaning the app will fail to start if they're missing. Ensure all development, CI, and production environments have these variables configured.If OAuth is optional for local development, consider adding defaults or making them optional:
🔎 Optional: Make OAuth client IDs optional for development
// Google OAuth for direct desktop authentication - GOOGLE_CLIENT_ID: z.string().min(1), + GOOGLE_CLIENT_ID: z.string().min(1).optional(), // GitHub OAuth for direct desktop authentication - GITHUB_CLIENT_ID: z.string().min(1), + GITHUB_CLIENT_ID: z.string().min(1).optional(),apps/api/src/env.ts (1)
11-16: LGTM!OAuth credentials are correctly placed in the server schema, keeping secrets server-side only. The
min(1)validation ensures non-empty values are provided.apps/web/src/app/(auth)/layout.tsx (1)
7-17: LGTM!The async layout correctly redirects authenticated users away from auth pages. Using
await auth()followed byredirect()follows the standard Next.js App Router pattern for server components.apps/api/src/app/api/auth/desktop/github/route.ts (2)
110-130: Verify email verification status for emails from the user profile.When
githubUser.emailis present (line 111), it's used directly without checking if it's verified. Only emails fetched from/user/emailsare checked forverifiedstatus. GitHub typically only exposes verified emails publicly, but consider explicitly verifying the status.🔎 Proposed fix to always verify email status
// Get user's email - might need to fetch from emails endpoint if private - let email = githubUser.email; - - if (!email) { - // Fetch emails from the emails endpoint - const emailsResponse = await fetch("https://api.github.com/user/emails", { - headers: { - Authorization: `Bearer ${tokenData.access_token}`, - Accept: "application/vnd.github+json", - "X-GitHub-Api-Version": "2022-11-28", - }, - }); - - if (emailsResponse.ok) { - const emails: GitHubEmail[] = await emailsResponse.json(); - // Find primary verified email - const primaryEmail = emails.find((e) => e.primary && e.verified); - const verifiedEmail = emails.find((e) => e.verified); - email = primaryEmail?.email || verifiedEmail?.email || null; - } + // Always fetch from emails endpoint to ensure we get a verified email + const emailsResponse = await fetch("https://api.github.com/user/emails", { + headers: { + Authorization: `Bearer ${tokenData.access_token}`, + Accept: "application/vnd.github+json", + "X-GitHub-Api-Version": "2022-11-28", + }, + }); + + let email: string | null = null; + if (emailsResponse.ok) { + const emails: GitHubEmail[] = await emailsResponse.json(); + // Find primary verified email + const primaryEmail = emails.find((e) => e.primary && e.verified); + const verifiedEmail = emails.find((e) => e.verified); + email = primaryEmail?.email || verifiedEmail?.email || null; }
144-199: LGTM!The Clerk user provisioning logic correctly mirrors the Google OAuth route pattern. Error handling is comprehensive with detailed logging, and the token generation follows the established pattern using the shared
generateTokensutility.apps/api/src/trpc/context.ts (1)
43-67: Auth flow implementation looks correct.The multi-method authentication approach with proper precedence (Clerk session → Clerk OAuth → Desktop JWT) is well-structured. The Bearer token extraction and fallback to
userId: nullare handled appropriately.apps/desktop/src/shared/constants.ts (1)
1-1: Good refactor to centralize protocol schemes.Importing from the shared package and re-exporting maintains backward compatibility for existing consumers while consolidating the source of truth.
Also applies to: 29-34
apps/desktop/src/renderer/screens/main/index.tsx (2)
35-36: Clean auth gating implementation.The auth state derivation with safe defaults (
isSignedIn ?? false) and the early returns for loading/sign-in states are well-structured. TheDndProvideris correctly excluded from auth screens where drag-and-drop isn't needed.Also applies to: 161-185
52-54: Workspace query correctly gated by auth state.Enabling the query only when
isSignedInis true prevents unnecessary API calls and potential auth errors for unauthenticated users.apps/api/src/app/api/auth/desktop/tokens.ts (2)
5-6: Token expiry durations are reasonable.1-hour access tokens and 30-day refresh tokens follow common security practices, balancing security with user experience.
18-54: Token generation implementation is well-structured.Good practices observed:
- Distinct
typeclaim differentiates access from refresh tokens- Proper use of
setExpirationTimewith Unix timestamp (seconds)- Issuer claim enables token validation scoping
apps/api/src/app/api/auth/desktop/google/route.ts (1)
166-169: LGTM!Token generation and response structure are correct.
apps/web/src/app/auth/desktop/success/page.tsx (2)
30-33: Tokens in URL may be logged or exposed — verify this is acceptable.Passing
accessTokenandrefreshTokenas URL query parameters exposes them to:
- Browser history
- Any analytics/logging on the success page
- Potential referrer headers if the page has external resources
Since this redirects to a custom protocol (
superset://orsuperset-dev://) and the page auto-redirects immediately, the risk is minimized. However, ensure no client-side analytics capture these parameters.
40-45: LGTM!The auto-redirect effect properly guards against double execution and handles the error/missing-URL cases correctly.
apps/web/src/app/api/auth/desktop/github/route.ts (2)
44-77: LGTM!Error handling is correctly structured. The comment on line 72 properly explains why the redirect is outside the try-catch block (Next.js
redirectthrows a special error).
79-92: LGTM!Success redirect correctly passes all tokens and preserves the OAuth state for CSRF verification downstream.
packages/trpc/src/router/user/utils/sync-user-from-clerk.ts (1)
52-83: LGTM!The Drizzle ORM upsert pattern is correct, using email as the source of truth for conflict resolution. Avatar upload is efficiently deferred until after user creation and only when needed. As per coding guidelines, Drizzle ORM is used for all database operations.
apps/desktop/src/main/index.ts (2)
36-57: LGTM!The implementation correctly uses object parameters for
handleAuthCallbackas per the coding guidelines. The success condition properly validates all required token fields before proceeding. Based on learnings, IPC handlers should accept object parameters rather than positional parameters, which is correctly followed here.
69-79: LGTM!The
focusMainWindowhelper correctly handles the window restore flow, including the minimized state check.apps/api/src/app/api/auth/desktop/refresh/route.ts (1)
32-37: LGTM on token rotation implementation.The token refresh correctly rotates both access and refresh tokens, which is a security best practice. The debug logging is appropriate for troubleshooting.
apps/desktop/src/main/lib/auth/auth-service.ts (3)
7-26: LGTM on CSRF state mechanism.The state generation uses cryptographically secure random bytes and includes automatic cleanup of stale states. The 10-minute expiry is reasonable for OAuth flows.
51-82: Good defensive initialization logic.The startup flow correctly handles expired tokens by attempting refresh before clearing the session. This ensures users aren't unnecessarily logged out when only the access token has expired.
181-228: LGTM on OAuth URL construction for both providers.The sign-in flow correctly builds provider-specific OAuth URLs with appropriate scopes and CSRF state parameters. Google's
prompt: select_accountensures users can switch accounts.
| GOOGLE_CLIENT_ID: ${{ secrets.GOOGLE_CLIENT_ID }} | ||
| GOOGLE_CLIENT_SECRET: ${{ secrets.GOOGLE_CLIENT_SECRET }} | ||
| GITHUB_CLIENT_ID: ${{ secrets.GH_CLIENT_ID }} | ||
| GITHUB_CLIENT_SECRET: ${{ secrets.GH_CLIENT_SECRET }} |
There was a problem hiding this comment.
Critical: Secret name mismatch for GitHub OAuth credentials.
Lines 84-85 reference secrets.GH_CLIENT_ID and secrets.GH_CLIENT_SECRET, but the environment variable names are GITHUB_CLIENT_ID and GITHUB_CLIENT_SECRET. This inconsistency will cause the deployment to fail if the GitHub secrets are named GITHUB_CLIENT_ID and GITHUB_CLIENT_SECRET (as suggested by the variable names elsewhere).
🔎 Recommended fix to align secret names
- GITHUB_CLIENT_ID: ${{ secrets.GH_CLIENT_ID }}
- GITHUB_CLIENT_SECRET: ${{ secrets.GH_CLIENT_SECRET }}
+ GITHUB_CLIENT_ID: ${{ secrets.GITHUB_CLIENT_ID }}
+ GITHUB_CLIENT_SECRET: ${{ secrets.GITHUB_CLIENT_SECRET }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| GOOGLE_CLIENT_ID: ${{ secrets.GOOGLE_CLIENT_ID }} | |
| GOOGLE_CLIENT_SECRET: ${{ secrets.GOOGLE_CLIENT_SECRET }} | |
| GITHUB_CLIENT_ID: ${{ secrets.GH_CLIENT_ID }} | |
| GITHUB_CLIENT_SECRET: ${{ secrets.GH_CLIENT_SECRET }} | |
| GOOGLE_CLIENT_ID: ${{ secrets.GOOGLE_CLIENT_ID }} | |
| GOOGLE_CLIENT_SECRET: ${{ secrets.GOOGLE_CLIENT_SECRET }} | |
| GITHUB_CLIENT_ID: ${{ secrets.GITHUB_CLIENT_ID }} | |
| GITHUB_CLIENT_SECRET: ${{ secrets.GITHUB_CLIENT_SECRET }} |
🤖 Prompt for AI Agents
In .github/workflows/deploy-production.yml around lines 82 to 85, the workflow
uses secrets.GH_CLIENT_ID and secrets.GH_CLIENT_SECRET for the GITHUB_CLIENT_ID
and GITHUB_CLIENT_SECRET environment variables which mismatches expected secret
names; update the workflow to reference secrets.GITHUB_CLIENT_ID and
secrets.GITHUB_CLIENT_SECRET to match the environment variable names (and verify
those secrets exist in the repository settings).
| const API_URL = process.env.NEXT_PUBLIC_API_URL || "http://localhost:3001"; | ||
| const WEB_URL = process.env.NEXT_PUBLIC_WEB_URL || "http://localhost:3000"; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check env.ts file and route.ts file
echo "=== Checking apps/web/src/env.ts ==="
cat -n apps/web/src/env.ts | head -100
echo ""
echo "=== Checking apps/web/src/app/api/auth/desktop/google/route.ts ==="
cat -n apps/web/src/app/api/auth/desktop/google/route.ts | head -30Repository: superset-sh/superset
Length of output: 3157
Use validated environment schema instead of raw process.env.
The web app has a validated env schema at apps/web/src/env.ts using @t3-oss/env-nextjs. Import env and use it instead of accessing process.env directly, which bypasses validation.
Note: NEXT_PUBLIC_WEB_URL is not currently defined in the env schema (line 20-28 in env.ts). You'll need to add it to the client schema before using it, or update the fallback logic. For example:
+import { env } from "@/env";
import { redirect } from "next/navigation";
-const API_URL = process.env.NEXT_PUBLIC_API_URL || "http://localhost:3001";
-const WEB_URL = process.env.NEXT_PUBLIC_WEB_URL || "http://localhost:3000";
+const API_URL = env.NEXT_PUBLIC_API_URL;
+const WEB_URL = process.env.NEXT_PUBLIC_WEB_URL || "http://localhost:3000";Then add NEXT_PUBLIC_WEB_URL to the client schema in env.ts with appropriate validation.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const API_URL = process.env.NEXT_PUBLIC_API_URL || "http://localhost:3001"; | |
| const WEB_URL = process.env.NEXT_PUBLIC_WEB_URL || "http://localhost:3000"; | |
| import { env } from "@/env"; | |
| import { redirect } from "next/navigation"; | |
| const API_URL = env.NEXT_PUBLIC_API_URL; | |
| const WEB_URL = process.env.NEXT_PUBLIC_WEB_URL || "http://localhost:3000"; |
🤖 Prompt for AI Agents
In apps/web/src/app/api/auth/desktop/google/route.ts around lines 3-4, the code
reads raw process.env variables instead of using the validated env schema;
replace direct process.env usage with the exported env from apps/web/src/env.ts
(e.g. use env.NEXT_PUBLIC_API_URL and env.NEXT_PUBLIC_WEB_URL) and remove the
raw fallbacks, and also update apps/web/src/env.ts to add NEXT_PUBLIC_WEB_URL to
the client schema with appropriate validation (or adjust fallback behavior
inside env.ts) so the value is validated at build/runtime before use.
| // Validate required params | ||
| if (!code || !state) { | ||
| const errorUrl = new URL("/auth/desktop/success", WEB_URL); | ||
| errorUrl.searchParams.set("error", "Missing authentication parameters"); | ||
| redirect(errorUrl.toString()); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read the full file to understand context
echo "=== Full file content ==="
cat -n "apps/web/src/app/api/auth/desktop/google/route.ts"
echo -e "\n=== Search for state validation in web auth routes ==="
rg -n "state" apps/web/src/app/api/auth/ -A 3 -B 1 -i
echo -e "\n=== Search for state validation in API routes ==="
rg -n "state" apps/api/src/app/api/auth/ -A 3 -B 1 -i 2>/dev/null || echo "apps/api path not found or no matches"Repository: superset-sh/superset
Length of output: 7323
🏁 Script executed:
#!/bin/bash
# Search for state storage/validation patterns
echo "=== Looking for stored state comparisons ==="
rg -n "stored.*state|state.*verify|state.*match|state.*compare" -i
echo -e "\n=== Search for oauth callback handling ==="
rg -n "redirect_uri|callback" apps/web/src/app/api/auth/ -A 2 -B 1
echo -e "\n=== Check for desktop-specific auth logic ==="
fd -t f "." apps/web/src/app/api/auth/desktop/ | head -20Repository: superset-sh/superset
Length of output: 1401
🏁 Script executed:
#!/bin/bash
# Read the PKCE file to understand state validation
cat -n "apps/desktop/src/main/lib/auth/pkce.ts" | head -100
echo -e "\n=== State validation logic ==="
sed -n '70,100p' "apps/desktop/src/main/lib/auth/pkce.ts"Repository: superset-sh/superset
Length of output: 3816
🏁 Script executed:
#!/bin/bash
# Find where state is generated and stored in desktop app
rg -n "state" apps/desktop/src/main/lib/auth/ -B 2 -A 2 | head -80
# Check the success page handler
cat -n "apps/web/src/app/auth/desktop/success/page.tsx" 2>/dev/null || echo "File not found"Repository: superset-sh/superset
Length of output: 9879
State parameter must be validated against the original value at the authorization callback.
The current code only checks if state exists but doesn't verify it against the state generated when initiating the OAuth flow. State should be validated before exchanging the authorization code for tokens, not after. Implement state validation by:
- Storing the state value (in session, cache, or secure cookie) when initiating the OAuth request
- Comparing the returned state against the stored value before proceeding with token exchange at line 45
- Rejecting the request if the state values don't match
🤖 Prompt for AI Agents
In apps/web/src/app/api/auth/desktop/google/route.ts around lines 28-33 (and the
token exchange at ~line 45), the code only checks presence of state but does not
validate it against the original value; persist the generated state when
initiating the OAuth flow (session/cookie/secure cache), then at this callback
retrieve the stored state and compare it to the returned state before performing
the token exchange at line 45; if they do not match, reject the request
immediately (redirect to the error URL with a clear message or return an error
response) and do not proceed to exchange the code for tokens.
| // Redirect to success page with all tokens | ||
| const successUrl = new URL("/auth/desktop/success", WEB_URL); | ||
| successUrl.searchParams.set("accessToken", tokenData.accessToken); | ||
| successUrl.searchParams.set( | ||
| "accessTokenExpiresAt", | ||
| tokenData.accessTokenExpiresAt.toString(), | ||
| ); | ||
| successUrl.searchParams.set("refreshToken", tokenData.refreshToken); | ||
| successUrl.searchParams.set( | ||
| "refreshTokenExpiresAt", | ||
| tokenData.refreshTokenExpiresAt.toString(), | ||
| ); | ||
| successUrl.searchParams.set("state", state); | ||
| redirect(successUrl.toString()); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the success page and examine its implementation
fd -t f "page.tsx" apps/web/src/app/auth/desktop/successRepository: superset-sh/superset
Length of output: 110
🏁 Script executed:
#!/bin/bash
# Find the GitHub auth route
fd -t f "route.ts" apps/web/src/app/api/auth/desktop/ | head -20Repository: superset-sh/superset
Length of output: 163
🏁 Script executed:
#!/bin/bash
# Read the success page to see how it handles tokens
wc -l apps/web/src/app/auth/desktop/success/page.tsxRepository: superset-sh/superset
Length of output: 114
🏁 Script executed:
#!/bin/bash
# Read the GitHub route to compare patterns
wc -l apps/web/src/app/api/auth/desktop/github/route.tsRepository: superset-sh/superset
Length of output: 116
🏁 Script executed:
#!/bin/bash
# Read the success page implementation
cat -n apps/web/src/app/auth/desktop/success/page.tsxRepository: superset-sh/superset
Length of output: 4968
🏁 Script executed:
#!/bin/bash
# Read the GitHub route for comparison
cat -n apps/web/src/app/api/auth/desktop/github/route.tsRepository: superset-sh/superset
Length of output: 3693
🏁 Script executed:
#!/bin/bash
# Search for any history.replaceState calls in auth-related files
rg "history\.replaceState|replaceState" apps/web/src/app/auth/ apps/web/src/app/api/auth/Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
#!/bin/bash
# Check if there's any client-side token cleanup in the success page
rg "useEffect|history" apps/web/src/app/auth/desktop/success/page.tsxRepository: superset-sh/superset
Length of output: 150
Add history.replaceState() to remove tokens from browser history after extraction.
The success page correctly extracts tokens from query parameters and avoids rendering them in the UI, but it's missing a critical security control. After extracting the tokens in the useEffect hook, call history.replaceState({}, '', '/auth/desktop/success') to remove them from the browser history. This prevents users from accessing tokens via the back button and protects against history-based token leakage.
🤖 Prompt for AI Agents
In apps/web/src/app/api/auth/desktop/google/route.ts around lines 79-92 the
server redirects to /auth/desktop/success with tokens in the query string;
update the client-side success page: in the useEffect that parses tokens from
location.search, after extracting and storing the tokens call
history.replaceState({}, '', '/auth/desktop/success') to remove query params
from the browser history so tokens cannot be retrieved via the back button;
ensure this runs only after tokens are parsed and stored (guarded) and do not
change server redirect behavior.
| export async function syncUserFromClerk(clerkId: string) { | ||
| const clerkUser = await clerkClient.users.getUser(clerkId); | ||
|
|
||
| const primaryEmail = clerkUser.emailAddresses.find( | ||
| (email) => email.id === clerkUser.primaryEmailAddressId, | ||
| )?.emailAddress; | ||
|
|
||
| if (!primaryEmail) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Missing error handling for Clerk API call.
clerkClient.users.getUser(clerkId) on line 37 throws if the user doesn't exist or if there's a network/API error. This will propagate as an unhandled exception. Consider wrapping in try-catch to return null or handle gracefully.
🔎 Proposed fix
export async function syncUserFromClerk(clerkId: string) {
- const clerkUser = await clerkClient.users.getUser(clerkId);
+ let clerkUser;
+ try {
+ clerkUser = await clerkClient.users.getUser(clerkId);
+ } catch (error) {
+ console.error("[syncUserFromClerk] Failed to fetch Clerk user:", clerkId, error);
+ return null;
+ }
const primaryEmail = clerkUser.emailAddresses.find(📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export async function syncUserFromClerk(clerkId: string) { | |
| const clerkUser = await clerkClient.users.getUser(clerkId); | |
| const primaryEmail = clerkUser.emailAddresses.find( | |
| (email) => email.id === clerkUser.primaryEmailAddressId, | |
| )?.emailAddress; | |
| if (!primaryEmail) { | |
| return null; | |
| } | |
| export async function syncUserFromClerk(clerkId: string) { | |
| let clerkUser; | |
| try { | |
| clerkUser = await clerkClient.users.getUser(clerkId); | |
| } catch (error) { | |
| console.error("[syncUserFromClerk] Failed to fetch Clerk user:", clerkId, error); | |
| return null; | |
| } | |
| const primaryEmail = clerkUser.emailAddresses.find( | |
| (email) => email.id === clerkUser.primaryEmailAddressId, | |
| )?.emailAddress; | |
| if (!primaryEmail) { | |
| return null; | |
| } |
🤖 Prompt for AI Agents
In packages/trpc/src/router/user/utils/sync-user-from-clerk.ts around lines 36
to 45, wrap the clerkClient.users.getUser(clerkId) call in a try-catch so
API/network errors or missing users don't throw; on error (or if clerkClient
throws) return null (optionally log the error) and proceed, then continue the
existing primaryEmail check and return flow.
- Verify Google ID tokens with proper JWT signature verification using jose - Only trust verified emails from GitHub /user/emails endpoint - Replace raw process.env with validated env schemas in web routes - Remove unused handleDeepLinkAuth method and verifyState export - Remove PROTOCOL_SCHEMES re-export from shared/constants - Simplify desktop success page (remove unnecessary Suspense wrapper) - Clean up comments and unused code 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
saddlepaddle
had a problem deploying
to
preview
GitHub Actions
Failure
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
apps/desktop/src/main/lib/auth/auth-service.ts (3)
7-37: State cleanup runs only on generateState calls—consider periodic cleanup.The state store cleanup (lines 18-24) only runs when
generateState()is called. If sign-in attempts are infrequent, expired states may accumulate. Consider either:
- Running cleanup periodically (e.g., setInterval), or
- Running cleanup on both
generateState()andverifyState()calls.Also, verify that
verifyStateis exported for use by other modules (the export is not visible in the provided code but mentioned in the AI summary).🔎 Alternative: cleanup in verifyState too
function verifyState(state: string): boolean { + // Clean up old states + const tenMinutesAgo = Date.now() - 10 * 60 * 1000; + for (const [s, timestamp] of stateStore) { + if (timestamp < tenMinutesAgo) { + stateStore.delete(s); + } + } + if (!stateStore.has(state)) { return false; } stateStore.delete(state); return true; }
97-124: Add null check after refreshTokens for type safety.Line 113 returns
this.session.accessTokenafter a successful refresh, but TypeScript may flag this if strict null checks are enabled sincethis.sessioncould theoretically be null. WhilerefreshTokens()does setthis.session, an explicit null check would improve type safety and make the code more defensive.🔎 More defensive pattern
const refreshed = await this.refreshTokens(); if (refreshed) { - return this.session.accessToken; + return this.session?.accessToken ?? null; } }
230-267: Update log message to be provider-agnostic.Line 258 logs "Signed in via Google OAuth with refresh token" but this callback handler is used for both Google and GitHub OAuth flows. The log message should be provider-agnostic or include the actual provider information.
🔎 Suggested fix
- console.log("[auth] Signed in via Google OAuth with refresh token"); + console.log("[auth] Signed in successfully with refresh token");Or if you want to preserve provider info, pass it as a parameter:
-async handleAuthCallback(params: { +async handleAuthCallback(params: { + provider?: AuthProvider; accessToken: string; accessTokenExpiresAt: number; refreshToken: string; refreshTokenExpiresAt: number; state: string; }): Promise<SignInResult> { // ... - console.log("[auth] Signed in via Google OAuth with refresh token"); + console.log(`[auth] Signed in via ${params.provider || "OAuth"} with refresh token`);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
apps/api/src/app/api/auth/desktop/github/route.ts(1 hunks)apps/api/src/app/api/auth/desktop/google/route.ts(1 hunks)apps/api/src/env.ts(1 hunks)apps/desktop/src/main/env.main.ts(1 hunks)apps/desktop/src/main/index.ts(1 hunks)apps/desktop/src/main/lib/auth/auth-service.ts(3 hunks)apps/desktop/src/main/lib/auth/deep-link-handler.ts(3 hunks)apps/desktop/src/shared/constants.ts(2 hunks)apps/web/src/app/api/auth/desktop/github/route.ts(1 hunks)apps/web/src/app/api/auth/desktop/google/route.ts(1 hunks)apps/web/src/app/auth/desktop/success/page.tsx(3 hunks)apps/web/src/env.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/desktop/src/main/env.main.ts
- apps/desktop/src/shared/constants.ts
- apps/web/src/app/api/auth/desktop/github/route.ts
- apps/desktop/src/main/index.ts
- apps/web/src/app/api/auth/desktop/google/route.ts
🧰 Additional context used
📓 Path-based instructions (7)
apps/api/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Drizzle ORM for all database operations
Files:
apps/api/src/app/api/auth/desktop/github/route.tsapps/api/src/app/api/auth/desktop/google/route.tsapps/api/src/env.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Avoid using any type in TypeScript - maintain type safety unless absolutely necessary
Files:
apps/api/src/app/api/auth/desktop/github/route.tsapps/web/src/app/auth/desktop/success/page.tsxapps/web/src/env.tsapps/desktop/src/main/lib/auth/deep-link-handler.tsapps/desktop/src/main/lib/auth/auth-service.tsapps/api/src/app/api/auth/desktop/google/route.tsapps/api/src/env.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Run Biome for formatting, linting, import organization, and safe fixes at the root level using bun run lint:fix
Files:
apps/api/src/app/api/auth/desktop/github/route.tsapps/web/src/app/auth/desktop/success/page.tsxapps/web/src/env.tsapps/desktop/src/main/lib/auth/deep-link-handler.tsapps/desktop/src/main/lib/auth/auth-service.tsapps/api/src/app/api/auth/desktop/google/route.tsapps/api/src/env.ts
**/*.{tsx,css}
📄 CodeRabbit inference engine (AGENTS.md)
Use React + TailwindCSS v4 + shadcn/ui for UI development
Files:
apps/web/src/app/auth/desktop/success/page.tsx
apps/desktop/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx,js,jsx}: For Electron interprocess communication, ALWAYS use trpc as defined insrc/lib/trpc
Use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary.
Files:
apps/desktop/src/main/lib/auth/deep-link-handler.tsapps/desktop/src/main/lib/auth/auth-service.ts
apps/desktop/src/{main,renderer,preload}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use type-safe IPC communication - define channel types in apps/desktop/src/shared/ipc-channels.ts before implementing handlers
Files:
apps/desktop/src/main/lib/auth/deep-link-handler.tsapps/desktop/src/main/lib/auth/auth-service.ts
apps/desktop/src/main/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Accept object parameters in IPC handlers - do not use positional parameters in ipcMain.handle()
Files:
apps/desktop/src/main/lib/auth/deep-link-handler.tsapps/desktop/src/main/lib/auth/auth-service.ts
🧠 Learnings (2)
📚 Learning: 2025-12-18T17:26:38.664Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-12-18T17:26:38.664Z
Learning: Applies to apps/desktop/**/*.{ts,tsx,js,jsx} : Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary.
Applied to files:
apps/web/src/app/auth/desktop/success/page.tsx
📚 Learning: 2025-12-18T23:19:10.405Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-18T23:19:10.405Z
Learning: Applies to apps/desktop/src/main/**/*.ts : Accept object parameters in IPC handlers - do not use positional parameters in ipcMain.handle()
Applied to files:
apps/desktop/src/main/lib/auth/deep-link-handler.ts
🧬 Code graph analysis (1)
apps/desktop/src/main/lib/auth/auth-service.ts (4)
apps/desktop/src/main/lib/auth/pkce.ts (1)
generateState(29-31)apps/desktop/src/main/lib/auth/token-storage.ts (1)
tokenStorage(46-46)packages/shared/src/constants.ts (1)
AuthProvider(3-3)apps/desktop/src/shared/auth.ts (3)
AuthProvider(1-1)SignInResult(13-16)AuthSession(6-11)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Deploy Marketing
- GitHub Check: Deploy Admin
- GitHub Check: Deploy API
- GitHub Check: Deploy Docs
- GitHub Check: Build
🔇 Additional comments (21)
apps/web/src/env.ts (1)
22-22: I need the review comment to rewrite. Please provide it within <review_comment> tags along with any context about the code under review.apps/api/src/app/api/auth/desktop/github/route.ts (5)
1-33: LGTM! Type definitions are thorough and type-safe.The imports are minimal and the interface definitions properly model the GitHub API responses with full type safety.
42-89: LGTM! Token exchange logic is robust.The code properly validates inputs, handles GitHub's token exchange flow, and includes comprehensive error handling with appropriate logging.
110-140: LGTM! Email verification is handled correctly.The implementation properly fetches verified emails from GitHub's
/user/emailsendpoint and only accepts verified emails (preferring primary+verified). This correctly addresses the security concern from the previous review.
142-145: LGTM! Name parsing handles edge cases correctly.The logic properly handles null/empty name values by using
|| undefinedto ensure Clerk receives proper undefined values instead of empty strings.
147-202: LGTM! User management and token generation are well-implemented.The code properly handles existing users, creates new users when needed, marks emails as verified for OAuth-verified emails, and includes detailed error handling for Clerk operations.
apps/api/src/env.ts (1)
11-14: LGTM! OAuth credentials are properly validated.The new environment variables use appropriate
z.string().min(1)validation to ensure non-empty OAuth credentials. The validation syntax is correct for Zod 4.apps/api/src/app/api/auth/desktop/google/route.ts (4)
1-39: LGTM! JWT verification infrastructure is properly set up.The code imports
josefor proper JWT verification and creates a cached JWKS instance. Type definitions are complete and type-safe.
56-87: LGTM! Google OAuth token exchange is correctly implemented.The code properly exchanges the authorization code for tokens using the correct content type and parameters, with appropriate error handling.
89-111: LGTM! ID token verification is now properly implemented.The code uses
joseto verify the ID token signature against Google's JWKS, validates issuer and audience claims, and checks email verification. This fully addresses the security concern from the previous review.
113-168: LGTM! User management follows consistent patterns.The Clerk user creation and management logic mirrors the GitHub OAuth route, providing consistency across OAuth providers. Email verification is properly handled.
apps/web/src/app/auth/desktop/success/page.tsx (3)
11-31: LGTM! Token extraction and URL construction are secure.The code properly extracts all required token parameters and uses
encodeURIComponentfor safe URL construction, preventing injection issues.
33-42: LGTM! useEffect is appropriately used for navigation side effect.The auto-redirect logic properly uses
useEffectfor the one-time navigation side effect. ThehasAttemptedguard prevents double-redirects, and dependency array is correct. This is an appropriate use case foruseEffect(browser navigation) rather than state management.
44-107: LGTM! Error handling and UI states are comprehensive.The component properly handles error states, invalid requests, and success cases with clear user-facing messages and fallback options. The conditional rendering of the manual link is defensive and appropriate.
apps/desktop/src/main/lib/auth/deep-link-handler.ts (3)
1-14: LGTM! Clean interface definition with proper typing.The import uses the configured alias and the interface correctly defines all token-related fields with appropriate optional modifiers and types.
78-92: LGTM! Proper timestamp parsing with NaN validation.The expiration timestamp parsing correctly uses base-10 parseInt and validates against NaN, with clear error messages for invalid values.
113-124: LGTM! Robust URL validation with proper error handling.The function correctly validates both protocol schemes and host, with appropriate try-catch for malformed URLs.
apps/desktop/src/main/lib/auth/auth-service.ts (4)
51-82: LGTM! Proper session restoration with token refresh fallback.The initialization logic correctly handles all session states. Line 65 temporarily sets
this.sessionto enable the refresh flow, which is necessary but slightly awkward. This is acceptable since initialization runs before any concurrent operations.
269-288: LGTM! Clean session management and service export.The sign-out flow, session clearing, and state change emission are straightforward and correct. The singleton pattern for
authServiceis appropriate.
1-288: Coding guidelines: Well-structured with proper object parameters.The code properly follows the coding guidelines:
- ✓ Uses object parameters for
handleAuthCallback(line 233-239)- ✓ Maintains type safety throughout (no
anytypes)- ✓ Proper TypeScript typing for all interfaces and methods
The past review concern about the unused
handleDeepLinkAuthmethod appears to have been addressed by its removal from this implementation.
181-228: Environment variables are already validated at startup via t3-env.The
envobject frommain/env.main.tsis validated when the module is first imported byauth-service.ts. Sincet3-oss/env-core'screateEnv()validates the schema at import time, missing or invalid values forGH_CLIENT_IDandGOOGLE_CLIENT_ID(both required without defaults) will throw aZodErrorduring app startup—before any sign-in attempt. The.envfile is loaded inelectron.vite.config.tswithoverride: true, ensuring variables are available at runtime.Likely an incorrect or invalid review comment.
| /** | ||
| * Refresh tokens using the refresh token | ||
| */ | ||
| private async refreshTokens(): Promise<boolean> { | ||
| if (!this.session?.refreshToken) { | ||
| return false; | ||
| } | ||
|
|
||
| return this.session?.accessToken ?? null; | ||
| try { | ||
| const response = await fetch( | ||
| `${env.NEXT_PUBLIC_API_URL}/api/auth/desktop/refresh`, | ||
| { | ||
| method: "POST", | ||
| headers: { | ||
| "Content-Type": "application/json", | ||
| }, | ||
| body: JSON.stringify({ | ||
| refreshToken: this.session.refreshToken, | ||
| }), | ||
| }, | ||
| ); | ||
|
|
||
| if (!response.ok) { | ||
| console.error("[auth] Token refresh failed:", response.status); | ||
| return false; | ||
| } | ||
|
|
||
| const tokens = (await response.json()) as { | ||
| accessToken: string; | ||
| accessTokenExpiresAt: number; | ||
| refreshToken: string; | ||
| refreshTokenExpiresAt: number; | ||
| }; | ||
|
|
||
| // Update session with new tokens | ||
| this.session = { | ||
| accessToken: tokens.accessToken, | ||
| accessTokenExpiresAt: tokens.accessTokenExpiresAt, | ||
| refreshToken: tokens.refreshToken, | ||
| refreshTokenExpiresAt: tokens.refreshTokenExpiresAt, | ||
| }; | ||
|
|
||
| await tokenStorage.save(this.session); | ||
| console.log("[auth] Tokens refreshed successfully"); | ||
| return true; | ||
| } catch (err) { | ||
| console.error("[auth] Token refresh error:", err); | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for existing token response schemas or types
rg -n "RefreshToken|refreshToken.*Response|desktop.*refresh|TokenSchema" --type=ts -g "!node_modules" -g "!dist" -A 3 -B 3Repository: superset-sh/superset
Length of output: 2349
🏁 Script executed:
#!/bin/bash
# Search for Zod usage in the project
rg -n "from.*zod|import.*zod" --type=ts -g "!node_modules" -g "!dist" | head -20Repository: superset-sh/superset
Length of output: 1391
🏁 Script executed:
#!/bin/bash
# Look at the refresh endpoint implementation to see the response structure
cat -n apps/api/src/app/api/auth/desktop/refresh/route.tsRepository: superset-sh/superset
Length of output: 1651
🏁 Script executed:
#!/bin/bash
# Look at the generateTokens function to understand the response structure
cat -n apps/api/src/app/api/auth/desktop/tokens.tsRepository: superset-sh/superset
Length of output: 2581
🏁 Script executed:
#!/bin/bash
# Check if there are any API type definitions in the desktop app
find apps/desktop -name "*.ts" -type f -exec grep -l "auth.*types\|api.*types\|ipc.*channel" {} \; | head -10Repository: superset-sh/superset
Length of output: 97
🏁 Script executed:
#!/bin/bash
# Check the IPC channels file mentioned in the coding guidelines
cat -n apps/desktop/src/shared/ipc-channels.ts | head -100Repository: superset-sh/superset
Length of output: 135
🏁 Script executed:
#!/bin/bash
# List what's in the shared directory
ls -la apps/desktop/src/shared/Repository: superset-sh/superset
Length of output: 891
🏁 Script executed:
#!/bin/bash
# Check the shared auth.ts file
cat -n apps/desktop/src/shared/auth.tsRepository: superset-sh/superset
Length of output: 545
🏁 Script executed:
#!/bin/bash
# Check the auth-service.ts to see if it imports the AuthSession type
cat -n apps/desktop/src/main/lib/auth/auth-service.ts | head -180Repository: superset-sh/superset
Length of output: 5784
Validate API response shape before type assertion.
Lines 153-158 use a type assertion without validating the response structure. If the API returns an unexpected shape, this could cause runtime errors later. The AuthSession type already exists in shared/auth.ts and matches the expected response perfectly. Consider using Zod (already a project dependency and used extensively in the codebase) to validate the response:
import { z } from "zod";
import type { AuthSession } from "shared/auth";
const RefreshResponseSchema = z.object({
accessToken: z.string(),
accessTokenExpiresAt: z.number(),
refreshToken: z.string(),
refreshTokenExpiresAt: z.number(),
});
// In refreshTokens:
const tokens = RefreshResponseSchema.parse(await response.json());Or use safeParse() for error handling that doesn't throw.
🤖 Prompt for AI Agents
In apps/desktop/src/main/lib/auth/auth-service.ts around lines 126 to 175, the
code asserts the refresh response shape without validation which can lead to
runtime errors; update the file to import z (and optionally AuthSession type)
and define a Zod schema matching accessToken, accessTokenExpiresAt,
refreshToken, refreshTokenExpiresAt, then validate the parsed JSON using
schema.parse (or schema.safeParse and handle failures) before assigning to
this.session and saving to tokenStorage so only validated token objects are
used.
| // Get all tokens and metadata | ||
| const accessToken = parsedUrl.searchParams.get("accessToken"); | ||
| const accessTokenExpiresAtStr = parsedUrl.searchParams.get( | ||
| "accessTokenExpiresAt", | ||
| ); | ||
| const refreshToken = parsedUrl.searchParams.get("refreshToken"); | ||
| const refreshTokenExpiresAtStr = parsedUrl.searchParams.get( | ||
| "refreshTokenExpiresAt", | ||
| ); | ||
| const state = parsedUrl.searchParams.get("state"); | ||
|
|
||
| if (!code) { | ||
| pkceStore.clear(); | ||
| return { success: false, error: "No auth code in callback" }; | ||
| if (!accessToken) { | ||
| return { success: false, error: "No access token in callback" }; | ||
| } | ||
|
|
||
| if (!accessTokenExpiresAtStr) { | ||
| return { | ||
| success: false, | ||
| error: "No access token expiration in callback", | ||
| }; | ||
| } | ||
|
|
||
| if (!refreshToken) { | ||
| return { success: false, error: "No refresh token in callback" }; | ||
| } | ||
|
|
||
| if (!refreshTokenExpiresAtStr) { | ||
| return { | ||
| success: false, | ||
| error: "No refresh token expiration in callback", | ||
| }; | ||
| } | ||
|
|
||
| if (!state) { | ||
| pkceStore.clear(); | ||
| return { success: false, error: "No state in callback" }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Validate state parameter earlier for fail-fast CSRF protection.
The state parameter is validated after all tokens are parsed and validated (line 74). For security and efficiency, CSRF state validation should occur earlier—ideally right after checking for errors and before parsing any tokens. This fails fast on invalid states and avoids unnecessary processing.
🔎 Suggested reordering
// Check for error response
const error = parsedUrl.searchParams.get("error");
if (error) {
const errorDescription = parsedUrl.searchParams.get("error_description");
return { success: false, error: errorDescription || error };
}
+ // Verify state first for CSRF protection
+ const state = parsedUrl.searchParams.get("state");
+ if (!state) {
+ return { success: false, error: "No state in callback" };
+ }
+
// Get all tokens and metadata
const accessToken = parsedUrl.searchParams.get("accessToken");
const accessTokenExpiresAtStr = parsedUrl.searchParams.get(
"accessTokenExpiresAt",
);
const refreshToken = parsedUrl.searchParams.get("refreshToken");
const refreshTokenExpiresAtStr = parsedUrl.searchParams.get(
"refreshTokenExpiresAt",
);
- const state = parsedUrl.searchParams.get("state");
if (!accessToken) {
return { success: false, error: "No access token in callback" };
}
// ... rest of validations
-
- if (!state) {
- return { success: false, error: "No state in callback" };
- }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Get all tokens and metadata | |
| const accessToken = parsedUrl.searchParams.get("accessToken"); | |
| const accessTokenExpiresAtStr = parsedUrl.searchParams.get( | |
| "accessTokenExpiresAt", | |
| ); | |
| const refreshToken = parsedUrl.searchParams.get("refreshToken"); | |
| const refreshTokenExpiresAtStr = parsedUrl.searchParams.get( | |
| "refreshTokenExpiresAt", | |
| ); | |
| const state = parsedUrl.searchParams.get("state"); | |
| if (!code) { | |
| pkceStore.clear(); | |
| return { success: false, error: "No auth code in callback" }; | |
| if (!accessToken) { | |
| return { success: false, error: "No access token in callback" }; | |
| } | |
| if (!accessTokenExpiresAtStr) { | |
| return { | |
| success: false, | |
| error: "No access token expiration in callback", | |
| }; | |
| } | |
| if (!refreshToken) { | |
| return { success: false, error: "No refresh token in callback" }; | |
| } | |
| if (!refreshTokenExpiresAtStr) { | |
| return { | |
| success: false, | |
| error: "No refresh token expiration in callback", | |
| }; | |
| } | |
| if (!state) { | |
| pkceStore.clear(); | |
| return { success: false, error: "No state in callback" }; | |
| } | |
| // Verify state first for CSRF protection | |
| const state = parsedUrl.searchParams.get("state"); | |
| if (!state) { | |
| return { success: false, error: "No state in callback" }; | |
| } | |
| // Get all tokens and metadata | |
| const accessToken = parsedUrl.searchParams.get("accessToken"); | |
| const accessTokenExpiresAtStr = parsedUrl.searchParams.get( | |
| "accessTokenExpiresAt", | |
| ); | |
| const refreshToken = parsedUrl.searchParams.get("refreshToken"); | |
| const refreshTokenExpiresAtStr = parsedUrl.searchParams.get( | |
| "refreshTokenExpiresAt", | |
| ); | |
| if (!accessToken) { | |
| return { success: false, error: "No access token in callback" }; | |
| } | |
| if (!accessTokenExpiresAtStr) { | |
| return { | |
| success: false, | |
| error: "No access token expiration in callback", | |
| }; | |
| } | |
| if (!refreshToken) { | |
| return { success: false, error: "No refresh token in callback" }; | |
| } | |
| if (!refreshTokenExpiresAtStr) { | |
| return { | |
| success: false, | |
| error: "No refresh token expiration in callback", | |
| }; | |
| } |
🤖 Prompt for AI Agents
In apps/desktop/src/main/lib/auth/deep-link-handler.ts around lines 41 to 76,
the state (CSRF) parameter is validated too late after token parsing; move the
state retrieval and validation immediately after parsing the URL/searchParams
and basic error checks so we fail fast on invalid/missing state before
extracting or validating access/refresh tokens and expirations; update the
control flow so the function returns an error for a missing/invalid state right
away, then proceed to read and validate accessToken, accessTokenExpiresAt,
refreshToken, and refreshTokenExpiresAt only if state is present and valid.
- Add GitHub OAuth option to sign-in and sign-up pages - Use react-icons (FaGithub, FcGoogle) instead of SVG files - Fix PostHog /ingest routes being blocked by Clerk middleware - Remove unused google.svg asset 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
saddlepaddle
had a problem deploying
to
preview
GitHub Actions
Failure
1 participant
Summary
Changes
/api/auth/desktop/google,/api/auth/desktop/github,/api/auth/desktop/refreshGOOGLE_CLIENT_ID,GOOGLE_CLIENT_SECRET,GITHUB_CLIENT_ID,GITHUB_CLIENT_SECRETto deploy workflowsTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.