adding more security to api endpoints to require api token for all ac… #321
Conversation
…cess, no by passing
|
Caution Review failedThe pull request is closed. Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds multi-path authentication (API key, session token, session cookie, single-use short-lived wsTokens), login/logout/token routes, WS upgrade authentication, JSON-content middleware, dynamic CORS and cookie parsing, frontend auth/api clients, web login UI, and e2e test auth helpers. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Client as Browser/Electron
participant Server as Backend
participant Disk as Disk
rect rgba(220,240,255,0.9)
Note over User,Server: Web login → session creation and cookie
User->>Client: Submit API key via login form
Client->>Server: POST /api/auth/login { apiKey }
Server->>Server: validateApiKey(key) (timing-safe)
alt valid
Server->>Server: createSession() -> sessionToken
Server->>Disk: persist sessions file
Server->>Client: 200 { token } + Set-Cookie automaker_session (HttpOnly)
else invalid or rate-limited
Server->>Client: 401/429
end
end
sequenceDiagram
participant Client as Browser/Electron
participant Server as Backend
participant Auth as Auth module
participant WS as WebSocket
rect rgba(240,255,220,0.9)
Note over Client,Server: WebSocket upgrade authentication
Client->>Server: HTTP Upgrade (with X-API-Key or X-Session-Token or automaker_session cookie or wsToken query)
Server->>Auth: checkRawAuthentication(headers, query, cookies)
alt authenticated
Server->>Client: 101 Switching Protocols
Client->>WS: Open persistent WS
else unauthorized
Server->>Client: 403 Forbidden (upgrade rejected)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (50)
Comment |
Summary of ChangesHello @webdevcody, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the security of the API endpoints by introducing a dual-mode authentication system. For Electron environments, an API key is used, while web-based access now leverages secure HTTP-only session cookies, alongside new dedicated authentication routes. This change also refines CORS handling, secures WebSocket connections, and updates the UI to seamlessly integrate with these new authentication mechanisms, including a new login flow for web users. Additionally, the Claude usage service has been extended to support Linux. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This is a substantial and well-executed pull request that significantly enhances the application's security by mandating authentication for all API endpoints. The new system adeptly handles both API key-based authentication for Electron and session/cookie-based authentication for the web. The introduction of dedicated authentication routes and a centralized API fetching utility are excellent architectural improvements. My feedback focuses on a few key areas to further improve robustness, performance, and maintainability, including fixing a potential issue in cookie parsing, making file I/O asynchronous on the server, and ensuring consistency in the client-side authentication logic.
apps/server/src/index.ts
Outdated
| const cookies = cookieHeader.split(';').reduce( | ||
| (acc, cookie) => { | ||
| const [key, value] = cookie.trim().split('='); | ||
| acc[key] = value; | ||
| return acc; | ||
| }, | ||
| {} as Record<string, string> | ||
| ); |
There was a problem hiding this comment.
This manual cookie parsing logic is brittle and may fail with complex cookie values (e.g., values containing =). It's better to use a dedicated library for this. The cookie package is a standard for this and is already available as an indirect dependency via cookie-parser.
You can use it by adding import cookie from 'cookie'; at the top of the file and then simplifying this block.
const cookies = cookie.parse(cookieHeader);| // Add API key for Electron mode auth | ||
| const apiKey = getApiKey(); | ||
| if (apiKey) { | ||
| url += `&apiKey=${encodeURIComponent(apiKey)}`; | ||
| } | ||
| // In web mode, cookies are sent automatically with same-origin WebSocket |
There was a problem hiding this comment.
The WebSocket connection logic for the terminal is missing the sessionToken query parameter for web mode. While cookies might work for same-origin connections, this can be fragile. The server-side implementation supports sessionToken in the query, and the main event WebSocket client (http-api-client.ts) already sends it for robustness.
For consistency and to ensure authentication works reliably across different environments (e.g., different ports in development), you should also add the session token to the URL here. You'll need to import getSessionToken from @/lib/http-api-client.
| // Add API key for Electron mode auth | |
| const apiKey = getApiKey(); | |
| if (apiKey) { | |
| url += `&apiKey=${encodeURIComponent(apiKey)}`; | |
| } | |
| // In web mode, cookies are sent automatically with same-origin WebSocket | |
| // Add API key for Electron mode auth | |
| const apiKey = getApiKey(); | |
| if (apiKey) { | |
| url += `&apiKey=${encodeURIComponent(apiKey)}`; | |
| } else { | |
| // In web mode, add session token as query param for robustness | |
| const sessionToken = getSessionToken(); | |
| if (sessionToken) { | |
| url += `&sessionToken=${encodeURIComponent(sessionToken)}`; | |
| } | |
| } | |
| // In web mode, cookies are sent automatically with same-origin WebSocket |
apps/server/src/lib/auth.ts
Outdated
| function saveSessions(): void { | ||
| try { | ||
| fs.mkdirSync(path.dirname(SESSIONS_FILE), { recursive: true }); | ||
| const sessions = Array.from(validSessions.entries()); | ||
| fs.writeFileSync(SESSIONS_FILE, JSON.stringify(sessions), { encoding: 'utf-8', mode: 0o600 }); | ||
| } catch (error) { | ||
| console.error('[Auth] Failed to save sessions:', error); | ||
| } | ||
| } |
There was a problem hiding this comment.
Using fs.writeFileSync is a synchronous, blocking operation. In a server environment, this can block the event loop and degrade performance, especially if session creation/invalidation is frequent. It's better to use the asynchronous version, fs.promises.writeFile.
Note that you will also need to update the functions that call saveSessions() (i.e., createSession, validateSession, invalidateSession) to await this asynchronous operation.
| function saveSessions(): void { | |
| try { | |
| fs.mkdirSync(path.dirname(SESSIONS_FILE), { recursive: true }); | |
| const sessions = Array.from(validSessions.entries()); | |
| fs.writeFileSync(SESSIONS_FILE, JSON.stringify(sessions), { encoding: 'utf-8', mode: 0o600 }); | |
| } catch (error) { | |
| console.error('[Auth] Failed to save sessions:', error); | |
| } | |
| } | |
| async function saveSessions(): Promise<void> { | |
| try { | |
| await fs.promises.mkdir(path.dirname(SESSIONS_FILE), { recursive: true }); | |
| const sessions = Array.from(validSessions.entries()); | |
| await fs.promises.writeFile(SESSIONS_FILE, JSON.stringify(sessions), { encoding: 'utf-8', mode: 0o600 }); | |
| } catch (error) { | |
| console.error('[Auth] Failed to save sessions:', error); | |
| } | |
| } |
| export function isRequestAuthenticated(req: Request): boolean { | ||
| // Check API key header | ||
| const headerKey = req.headers['x-api-key'] as string | undefined; | ||
| if (headerKey && headerKey === API_KEY) { | ||
| return true; | ||
| } | ||
|
|
||
| // Check session token header | ||
| const sessionTokenHeader = req.headers['x-session-token'] as string | undefined; | ||
| if (sessionTokenHeader && validateSession(sessionTokenHeader)) { | ||
| return true; | ||
| } | ||
|
|
||
| // Check query parameter | ||
| const queryKey = req.query.apiKey as string | undefined; | ||
| if (queryKey && queryKey === API_KEY) { | ||
| return true; | ||
| } | ||
|
|
||
| // Check cookie | ||
| const sessionToken = req.cookies?.[SESSION_COOKIE_NAME] as string | undefined; | ||
| if (sessionToken && validateSession(sessionToken)) { | ||
| return true; | ||
| } | ||
|
|
||
| return false; | ||
| } |
There was a problem hiding this comment.
The logic in this function is very similar to the logic inside authMiddleware. This duplication can lead to maintenance issues, where a change in authentication logic might be applied in one place but not the other. Consider refactoring to reduce this duplication. For example, authMiddleware could potentially use isRequestAuthenticated for its checks, or both could delegate to a common private function to handle the core validation logic.
| const hasApiKey = !!apiKeys.anthropic || !!claudeAuthStatus?.hasEnvApiKey; | ||
| const showUsageTracking = !hasApiKey && !isWindows; |
There was a problem hiding this comment.
The logic to determine showUsageTracking here is inconsistent with the logic in board-header.tsx. The board-header.tsx component also checks isCliVerified, which seems more correct as the usage tracking section is only relevant if the user is authenticated via the Claude CLI. For consistency and correctness, I recommend using the same logic here.
| const hasApiKey = !!apiKeys.anthropic || !!claudeAuthStatus?.hasEnvApiKey; | |
| const showUsageTracking = !hasApiKey && !isWindows; | |
| const hasApiKey = !!apiKeys.anthropic || !!claudeAuthStatus?.hasEnvApiKey; | |
| const isCliVerified = | |
| claudeAuthStatus?.authenticated && claudeAuthStatus?.method === 'cli_authenticated'; | |
| const showUsageTracking = !hasApiKey && !isWindows && isCliVerified; |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
apps/ui/src/components/views/login-view.tsx (1)
21-39: Consider clearing the API key from state after successful login.The login flow is well-implemented with proper loading states and error handling. As a security hygiene measure, consider clearing the
apiKeyfrom component state after successful authentication to minimize the window where sensitive data resides in memory.🔎 Optional: Clear API key on success
const result = await login(apiKey.trim()); if (result.success) { + setApiKey(''); // Clear sensitive data from state // Redirect to home/board on success navigate({ to: '/' }); } else {apps/ui/src/routes/__root.tsx (1)
81-124: Consider removinglocation.pathnamefrom dependency array to prevent auth re-initialization on every navigation.The auth initialization effect runs on every route change due to
location.pathnamein the dependency array. This causes unnecessaryfetchSessionToken()andcheckAuthStatus()calls when navigating between authenticated routes. The redirect logic at lines 113-116 can be moved to a separate effect that depends onisAuthenticated.🔎 Proposed fix
useEffect(() => { const initAuth = async () => { try { // Initialize API key for Electron mode await initApiKey(); // In Electron mode, we're always authenticated via header if (isElectronMode()) { setIsAuthenticated(true); setAuthChecked(true); return; } // In web mode, try to fetch session token (works if cookie is valid) const tokenFetched = await fetchSessionToken(); if (tokenFetched) { setIsAuthenticated(true); setAuthChecked(true); return; } // Fallback: check auth status via cookie const status = await checkAuthStatus(); setIsAuthenticated(status.authenticated); setAuthChecked(true); - - // Redirect to login if not authenticated and not already on login page - if (!status.authenticated && location.pathname !== '/login') { - navigate({ to: '/login' }); - } } catch (error) { console.error('Failed to initialize auth:', error); setAuthChecked(true); } }; initAuth(); - }, [location.pathname, navigate]); + }, []); + + // Redirect to login if not authenticated (web mode only) + useEffect(() => { + if (!isElectronMode() && authChecked && !isAuthenticated && location.pathname !== '/login') { + navigate({ to: '/login' }); + } + }, [authChecked, isAuthenticated, location.pathname, navigate]);apps/server/src/index.ts (1)
222-272: Consider extracting cookie parsing to a shared utility.The manual cookie parsing (lines 257-264) duplicates logic that
cookie-parserhandles for HTTP requests. Since therequestobject in WebSocket upgrades doesn't go through Express middleware, this is necessary, but consider extracting the parsing logic to a shared utility function inlib/auth.tsfor consistency.apps/ui/src/lib/api-fetch.ts (1)
14-21: Consider extractinggetServerUrlto avoid duplication.This function is identical to the one in
http-api-client.ts(lines 36-42). Consider extracting it to a shared constants/config module to maintain DRY principle.apps/server/src/lib/auth.ts (1)
291-317:isRequestAuthenticatedduplicates logic fromauthMiddleware.This function replicates the authentication checking logic from
authMiddleware(lines 214-269). Consider refactoring to share the validation logic to reduce duplication and ensure consistency.🔎 Suggested refactor to reduce duplication
+/** + * Internal helper to check all authentication methods + */ +function checkAuthentication(req: Request): { authenticated: boolean; method?: string } { + const headerKey = req.headers['x-api-key'] as string | undefined; + if (headerKey && headerKey === API_KEY) { + return { authenticated: true, method: 'api_key_header' }; + } + + const sessionTokenHeader = req.headers['x-session-token'] as string | undefined; + if (sessionTokenHeader && validateSession(sessionTokenHeader)) { + return { authenticated: true, method: 'session_token_header' }; + } + + const queryKey = req.query.apiKey as string | undefined; + if (queryKey && queryKey === API_KEY) { + return { authenticated: true, method: 'api_key_query' }; + } + + const sessionToken = req.cookies?.[SESSION_COOKIE_NAME] as string | undefined; + if (sessionToken && validateSession(sessionToken)) { + return { authenticated: true, method: 'session_cookie' }; + } + + return { authenticated: false }; +} + export function authMiddleware(req: Request, res: Response, next: NextFunction): void { - // ... existing implementation + const { authenticated } = checkAuthentication(req); + if (authenticated) { + next(); + return; + } + // Return appropriate error... } export function isRequestAuthenticated(req: Request): boolean { - // ... existing implementation + return checkAuthentication(req).authenticated; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (25)
apps/server/package.jsonapps/server/src/index.tsapps/server/src/lib/auth.tsapps/server/src/routes/auth/index.tsapps/server/src/routes/health/index.tsapps/server/src/services/claude-usage-service.tsapps/ui/src/components/claude-usage-popover.tsxapps/ui/src/components/dialogs/file-browser-dialog.tsxapps/ui/src/components/views/board-view/board-header.tsxapps/ui/src/components/views/login-view.tsxapps/ui/src/components/views/settings-view.tsxapps/ui/src/components/views/terminal-view.tsxapps/ui/src/components/views/terminal-view/terminal-panel.tsxapps/ui/src/lib/api-fetch.tsapps/ui/src/lib/electron.tsapps/ui/src/lib/http-api-client.tsapps/ui/src/main.tsapps/ui/src/preload.tsapps/ui/src/routes/__root.tsxapps/ui/src/routes/login.tsxapps/ui/src/store/setup-store.tsapps/ui/src/types/electron.d.tsdocker-compose.ymlinit.mjspackage.json
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always import from shared packages (@automaker/*), never from old relative paths
Files:
apps/ui/src/types/electron.d.tsapps/ui/src/preload.tsapps/ui/src/components/views/login-view.tsxapps/server/src/routes/auth/index.tsapps/ui/src/components/dialogs/file-browser-dialog.tsxapps/ui/src/components/claude-usage-popover.tsxapps/ui/src/components/views/settings-view.tsxapps/ui/src/routes/__root.tsxapps/ui/src/components/views/terminal-view/terminal-panel.tsxapps/server/src/routes/health/index.tsapps/ui/src/lib/electron.tsapps/ui/src/components/views/board-view/board-header.tsxapps/ui/src/lib/http-api-client.tsapps/ui/src/components/views/terminal-view.tsxapps/server/src/lib/auth.tsapps/ui/src/lib/api-fetch.tsapps/server/src/services/claude-usage-service.tsapps/ui/src/routes/login.tsxapps/ui/src/store/setup-store.tsapps/server/src/index.tsapps/ui/src/main.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
resolveModelString()from @automaker/model-resolver to convert model aliases (haiku, sonnet, opus) to full model names
Files:
apps/ui/src/types/electron.d.tsapps/ui/src/preload.tsapps/ui/src/components/views/login-view.tsxapps/server/src/routes/auth/index.tsapps/ui/src/components/dialogs/file-browser-dialog.tsxapps/ui/src/components/claude-usage-popover.tsxapps/ui/src/components/views/settings-view.tsxapps/ui/src/routes/__root.tsxapps/ui/src/components/views/terminal-view/terminal-panel.tsxapps/server/src/routes/health/index.tsapps/ui/src/lib/electron.tsapps/ui/src/components/views/board-view/board-header.tsxapps/ui/src/lib/http-api-client.tsapps/ui/src/components/views/terminal-view.tsxapps/server/src/lib/auth.tsapps/ui/src/lib/api-fetch.tsapps/server/src/services/claude-usage-service.tsapps/ui/src/routes/login.tsxapps/ui/src/store/setup-store.tsapps/server/src/index.tsapps/ui/src/main.ts
apps/server/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
createEventEmitter()fromlib/events.tsfor all server operations to emit events that stream to frontend via WebSocket
Files:
apps/server/src/routes/auth/index.tsapps/server/src/routes/health/index.tsapps/server/src/lib/auth.tsapps/server/src/services/claude-usage-service.tsapps/server/src/index.ts
🧠 Learnings (2)
📚 Learning: 2025-12-28T05:07:48.138Z
Learnt from: CR
Repo: AutoMaker-Org/automaker PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-28T05:07:48.138Z
Learning: Applies to apps/server/src/**/*.{ts,tsx} : Use `createEventEmitter()` from `lib/events.ts` for all server operations to emit events that stream to frontend via WebSocket
Applied to files:
init.mjsapps/server/src/index.ts
📚 Learning: 2025-12-28T05:07:48.138Z
Learnt from: CR
Repo: AutoMaker-Org/automaker PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-28T05:07:48.138Z
Learning: Frontend UI must use TanStack Router for file-based routing, organize components in components/views/, implement stores with Zustand, and use custom hooks in hooks/ directory
Applied to files:
apps/ui/src/components/views/settings-view.tsxapps/ui/src/routes/login.tsx
🧬 Code graph analysis (10)
apps/ui/src/components/views/login-view.tsx (1)
apps/ui/src/lib/http-api-client.ts (1)
login(128-151)
apps/server/src/routes/auth/index.ts (1)
apps/server/src/lib/auth.ts (6)
isRequestAuthenticated(291-317)validateApiKey(175-177)createSession(137-146)getSessionCookieOptions(182-196)getSessionCookieName(201-203)invalidateSession(167-170)
apps/ui/src/components/dialogs/file-browser-dialog.tsx (1)
apps/ui/src/lib/api-fetch.ts (1)
apiPost(124-131)
apps/ui/src/components/claude-usage-popover.tsx (1)
apps/ui/src/store/setup-store.ts (1)
useSetupStore(125-183)
apps/ui/src/routes/__root.tsx (1)
apps/ui/src/lib/http-api-client.ts (4)
initApiKey(81-100)isElectronMode(71-73)fetchSessionToken(157-180)checkAuthStatus(105-123)
apps/ui/src/components/views/terminal-view/terminal-panel.tsx (1)
apps/ui/src/lib/http-api-client.ts (1)
getApiKey(53-53)
apps/ui/src/components/views/terminal-view.tsx (2)
apps/ui/src/lib/api-fetch.ts (4)
apiDeleteRaw(156-161)apiGet(113-119)apiPost(124-131)apiFetch(84-108)apps/ui/src/lib/http-api-client.ts (1)
getApiKey(53-53)
apps/server/src/lib/auth.ts (1)
init.mjs (1)
req(173-175)
apps/ui/src/lib/api-fetch.ts (1)
apps/ui/src/lib/http-api-client.ts (2)
getApiKey(53-53)getSessionToken(56-56)
apps/ui/src/routes/login.tsx (2)
apps/ui/src/routes/__root.tsx (1)
Route(267-269)apps/ui/src/components/views/login-view.tsx (1)
LoginView(15-104)
🔇 Additional comments (45)
apps/server/src/services/claude-usage-service.ts (3)
21-21: LGTM! Linux platform detection added.The
isLinuxflag follows the existing pattern and correctly detects the Linux platform usingos.platform().
52-56: LGTM! Platform routing updated to include Linux.The condition correctly routes both Windows and Linux to the PTY-based implementation, while macOS continues using the
expect-based approach. The method rename toexecuteClaudeUsageCommandPtyappropriately reflects its generalized usage.
159-166: LGTM! Platform-specific PTY configuration is correct.The implementation correctly handles platform differences:
- Working directory: Appropriate environment variables and fallbacks for each platform (USERPROFILE for Windows, HOME for Linux).
- Shell/args construction:
- Windows:
cmd.exe /c claude /usagewith separate arguments is correct for cmd.exe's behavior.- Linux:
/bin/sh -c "claude /usage"with the command string as a single argument is correct for POSIX shell's-coption.Both approaches correctly execute
claude /usageon their respective platforms.package.json (1)
25-25: LGTM! Clean Docker development workflow addition.The new
dev:dockerscript provides a convenient way to spin up the entire stack using Docker Compose, complementing the existing development scripts.apps/ui/src/lib/electron.ts (1)
434-434: LGTM! Clean API surface extension.The optional
getApiKeymethod integrates cleanly into the Electron API surface, supporting the dual-mode (Electron/web) authentication model introduced in this PR.apps/ui/src/store/setup-store.ts (1)
179-179: LGTM! Proper persistence of authentication state.Persisting
claudeAuthStatusensures that authentication state (including environment-based API keys) is retained across reloads, which improves the user experience by avoiding re-checking on every startup.apps/ui/src/components/views/terminal-view/terminal-panel.tsx (1)
944-957: WebSocket authentication approach is acceptable, with security caveat.The implementation correctly adds the API key as a query parameter for Electron mode, while web mode relies on cookie-based authentication. The logic is sound and properly encoded.
Security note: Query parameters can be logged by proxies, load balancers, and server access logs. However, this is a standard and acceptable approach for WebSocket authentication when cookies aren't available (Electron mode). The same-origin cookie approach for web mode is more secure and appropriate for that context.
apps/ui/src/types/electron.d.ts (1)
467-467: LGTM! Type definition is consistent.The type signature matches the implementation interface in
electron.tsand properly models the optional API key retrieval method.docker-compose.yml (1)
52-52: LGTM! Security improvement for Docker environment.Changing the default CORS origin from wildcard (
*) to a specific localhost origin (http://localhost:3007) is a security improvement that prevents unintended cross-origin access while still supporting the local Docker development workflow.init.mjs (1)
355-369: LGTM! Improved logging visibility.The change to pipe server output to both the console and log file is a UX improvement, making it easier to see important startup information (like the generated API key) in real-time while still preserving logs for troubleshooting.
apps/ui/src/components/views/settings-view.tsx (1)
59-67: LGTM! Comprehensive API key detection.The composite check for
hasApiKeynow correctly accounts for both user-provided API keys and environment-based keys (ANTHROPIC_API_KEY), ensuring that usage tracking is hidden appropriately regardless of how authentication is configured. The optional chaining onclaudeAuthStatusis properly defensive.apps/ui/src/routes/login.tsx (1)
1-6: LGTM!Clean route definition following TanStack Router's file-based routing pattern. The import path and route configuration align with the existing patterns in
__root.tsx. Based on learnings, this correctly uses TanStack Router for file-based routing.apps/ui/src/preload.ts (1)
22-24: LGTM!The new
getApiKeyIPC channel follows the established pattern for exposing secure IPC methods via the context bridge. The return typePromise<string | null>appropriately handles the case where no API key is configured.apps/server/src/routes/health/index.ts (1)
1-25: LGTM! Good security separation.Keeping the basic health check unauthenticated is the correct approach for load balancers and container orchestration (e.g., Kubernetes liveness/readiness probes), while moving the detailed health endpoint to require authentication prevents information disclosure. The re-export pattern allows flexible mounting with auth middleware at a higher level.
apps/ui/src/components/views/board-view/board-header.tsx (1)
38-49: LGTM! Clear authentication gating logic.The decomposition into
hasApiKey,isCliVerified, andshowUsageTrackingimproves readability. The logic correctly gates the usage popover to only CLI-authenticated users who don't have an API key configured, which aligns with the component inclaude-usage-popover.tsx(Lines 34-36) that uses the same verification pattern.apps/ui/src/components/claude-usage-popover.tsx (2)
34-37: LGTM!The
isCliVerifiedderivation is consistent withboard-header.tsx, ensuring unified authentication gating across the codebase.
77-106: LGTM! Proper conditional data fetching.The guards prevent unnecessary API calls when CLI authentication isn't verified. The dependency arrays correctly include
isCliVerifiedto ensure effects re-run when authentication state changes.apps/ui/src/components/dialogs/file-browser-dialog.tsx (1)
17-17: LGTM! Good refactor to centralized API utilities.Replacing the raw
fetchwithapiPostensures consistent authentication headers and base URL handling across the application. The error handling is preserved, and the typed responseapiPost<BrowseResult>maintains type safety.Also applies to: 96-118
apps/ui/src/components/views/login-view.tsx (1)
61-70: Good security practice using password input type.Using
type="password"for the API key input prevents shoulder-surfing and screen recording exposure. TheautoFocusandfont-monostyling choices are appropriate for this authentication flow.apps/server/package.json (1)
32-32: Dependencies look appropriate for cookie-based session handling.The
cookie-parsermiddleware is a well-established Express choice for cookie parsing. Version 1.4.7 is current and compatible with Express 5.2.1. Type definitions are correctly placed in devDependencies.apps/ui/src/routes/__root.tsx (2)
12-17: LGTM!The auth-related imports are correctly sourced from
@/lib/http-api-clientand properly organized.
203-228: LGTM!The conditional rendering for different auth states is well-structured - login route bypasses sidebar, loading state during auth check, and null return for redirect handling. The guards are properly ordered.
apps/server/src/routes/auth/index.ts (3)
78-84: Consider if returning the session token in the response body is necessary.The session token is already set as an HTTP-only cookie (line 78). Returning it in the response body (line 83) allows JavaScript to access and store it, which partially defeats the XSS protection that HTTP-only cookies provide. If this is intentional for cross-origin scenarios, consider documenting this trade-off.
Is the explicit token return required for cross-origin cookie issues, or can clients rely solely on the HTTP-only cookie for authentication?
30-46: LGTM!The status endpoint correctly uses
isRequestAuthenticatedto check auth state without requiring authentication, allowing the UI to determine if login is needed.
127-147: LGTM!The logout endpoint properly invalidates the server-side session and clears the cookie with appropriate security options.
apps/ui/src/main.ts (3)
63-101: LGTM!The API key management is well-implemented:
- Secure file permissions (0o600)
- Persistent storage in userData directory
- Loads existing key or generates new one
- Called synchronously before server startup ensuring key availability
375-376: LGTM!The API key is correctly passed to the server process via environment variable, ensuring both processes share the same key.
715-718: LGTM!The IPC handler correctly exposes the API key to the renderer process for authenticated HTTP requests.
apps/ui/src/lib/http-api-client.ts (4)
240-255: LGTM!WebSocket authentication correctly uses query parameters since browser WebSocket API doesn't support custom headers. The fallback chain (API key → session token) handles both Electron and web modes appropriately.
320-339: LGTM!Header construction properly prioritizes API key (Electron mode) and falls back to session token (web mode). The early return after setting API key header prevents accidentally including both.
341-376: LGTM!All HTTP methods correctly include
credentials: 'include'for cookie-based session auth fallback.
63-66: Remove unused parameter fromclearSessionTokenfunction.The function signature includes a parameter
token: string | nullthat is not used in the function body. This appears to be a copy-paste error fromsetSessionToken.🔎 Proposed fix
// Clear session token (called on logout) -export const clearSessionToken = (): void => { +export const clearSessionToken = (): void => { cachedSessionToken = null; };Note: The current code is actually correct as shown - the function has no parameters. This comment is invalid if the annotated code accurately reflects the file. Please verify the actual implementation.
apps/ui/src/components/views/terminal-view.tsx (3)
489-509: LGTM!The synchronous XMLHttpRequest during page unload correctly includes both authentication headers:
X-API-Keyfor API authenticationX-Terminal-Tokenfor terminal-specific authenticationwithCredentials = truefor cookie fallbackUsing sync XHR here is appropriate since async operations don't complete during
beforeunload.
436-453: LGTM!Migration to
apiGetwith proper typing and error handling. The dependency array correctly removedserverUrlsince it's now handled internally by the api helpers.
830-866: LGTM!The terminal creation correctly uses
apiFetchfor authenticated requests while preserving terminal-specificX-Terminal-Tokenheader. Error handling for session limits (429) is comprehensive.apps/server/src/index.ts (3)
115-149: LGTM!Dynamic CORS configuration correctly:
- Allows no-origin requests (Electron, curl)
- Supports configurable
CORS_ORIGINallowlist- Permits localhost for development
- Rejects unknown origins for security
- Enables credentials for cookie auth
274-297: LGTM!WebSocket upgrade handler correctly authenticates connections before upgrading. Unauthorized attempts receive proper 401 response and socket destruction.
180-188: LGTM!Route ordering is correct:
/api/healthand/api/authmounted beforeauthMiddleware(public endpoints)authMiddlewareapplied to all/api/*routes- Protected detailed health endpoint mounted after middleware
apps/ui/src/lib/api-fetch.ts (3)
34-57: LGTM!Header construction correctly prioritizes API key over session token and properly merges additional headers. The early return pattern prevents accidentally including both auth headers.
84-108: LGTM!The core fetch wrapper properly:
- Handles auth header injection with
skipAuthescape hatch- Stringifies non-string bodies
- Supports absolute URLs
- Always includes credentials
110-161: LGTM!Convenience wrappers provide good DX with proper TypeScript generics. The
apiDeleteRawvariant is useful for checking response status before parsing.apps/server/src/lib/auth.ts (4)
28-56: LGTM!Session loading correctly filters expired sessions during startup, preventing stale session restoration. Good logging of loaded vs expired counts for debugging.
61-69: LGTM!Session persistence with secure file permissions (0o600) ensures sessions survive server restarts while protecting the file from other users.
136-162: LGTM!Session management functions correctly handle creation, validation with expiration checking, and cleanup on expiration.
214-269: LGTM!The middleware correctly implements a priority chain for authentication methods:
- API key header (Electron)
- Session token header (Web explicit)
- API key query param (fallback)
- Session cookie (Web implicit)
Returns appropriate 401/403 status codes for missing vs invalid credentials.
apps/server/src/lib/auth.ts
Outdated
| // Print API key to console for web mode users | ||
| console.log(` | ||
| ╔═══════════════════════════════════════════════════════════════════════╗ | ||
| ║ 🔐 API Key for Web Mode Authentication ║ | ||
| ╠═══════════════════════════════════════════════════════════════════════╣ | ||
| ║ ║ | ||
| ║ When accessing via browser, you'll be prompted to enter this key: ║ | ||
| ║ ║ | ||
| ║ ${API_KEY} | ||
| ║ ║ | ||
| ║ In Electron mode, authentication is handled automatically. ║ | ||
| ╚═══════════════════════════════════════════════════════════════════════╝ | ||
| `); |
There was a problem hiding this comment.
Console banner box is misaligned and API key may be logged to files.
- The banner box doesn't account for the API key length (36 chars for UUID), causing visual misalignment.
- The API key printed to console may be captured in log files if stdout is redirected, which could be a security concern in production deployments.
Consider:
- Adding padding to align the box
- Only printing the key in development mode or when explicitly requested
🔎 Proposed fix for alignment and conditional logging
-// Print API key to console for web mode users
-console.log(`
+// Print API key to console for web mode users (development only or when requested)
+const shouldPrintApiKey = process.env.NODE_ENV !== 'production' || process.env.PRINT_API_KEY === 'true';
+if (shouldPrintApiKey) {
+ console.log(`
╔═══════════════════════════════════════════════════════════════════════╗
║ 🔐 API Key for Web Mode Authentication ║
╠═══════════════════════════════════════════════════════════════════════╣
║ ║
║ When accessing via browser, you'll be prompted to enter this key: ║
║ ║
-║ ${API_KEY}
+║ ${API_KEY.padEnd(67)}║
║ ║
-║ In Electron mode, authentication is handled automatically. ║
+║ In Electron mode, authentication is handled automatically. ║
╚═══════════════════════════════════════════════════════════════════════╝
`);
+}📝 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.
| // Print API key to console for web mode users | |
| console.log(` | |
| ╔═══════════════════════════════════════════════════════════════════════╗ | |
| ║ 🔐 API Key for Web Mode Authentication ║ | |
| ╠═══════════════════════════════════════════════════════════════════════╣ | |
| ║ ║ | |
| ║ When accessing via browser, you'll be prompted to enter this key: ║ | |
| ║ ║ | |
| ║ ${API_KEY} | |
| ║ ║ | |
| ║ In Electron mode, authentication is handled automatically. ║ | |
| ╚═══════════════════════════════════════════════════════════════════════╝ | |
| `); | |
| // Print API key to console for web mode users (development only or when requested) | |
| const shouldPrintApiKey = process.env.NODE_ENV !== 'production' || process.env.PRINT_API_KEY === 'true'; | |
| if (shouldPrintApiKey) { | |
| console.log(` | |
| ╔═══════════════════════════════════════════════════════════════════════╗ | |
| ║ 🔐 API Key for Web Mode Authentication ║ | |
| ╠═══════════════════════════════════════════════════════════════════════╣ | |
| ║ ║ | |
| ║ When accessing via browser, you'll be prompted to enter this key: ║ | |
| ║ ║ | |
| ║ ${API_KEY.padEnd(67)}║ | |
| ║ ║ | |
| ║ In Electron mode, authentication is handled automatically. ║ | |
| ╚═══════════════════════════════════════════════════════════════════════╝ | |
| `); | |
| } |
🤖 Prompt for AI Agents
In apps/server/src/lib/auth.ts around lines 113 to 125, the console banner
currently assumes a fixed width and prints the raw API key unconditionally;
update it to compute the banner inner width dynamically based on the API key
length (ensure at least the current minimum width), pad the API key line so the
box borders remain aligned, and rebuild the surrounding banner lines to match
that width; additionally, stop unguarded logging of the secret by only printing
the banner when running in development or when an explicit opt-in
flag/environment variable (e.g. SHOW_API_KEY or NODE_ENV === 'development') is
set, otherwise omit the API key output entirely.
Security improvements identified for the protect-api-with-api-key branch: - Use short-lived wsToken for WebSocket auth (not session tokens in URLs) - Add AUTOMAKER_HIDE_API_KEY env var to suppress console logging - Add rate limiting to login endpoint (5 attempts/min/IP) - Use timing-safe comparison for API key validation - Make WebSocket tokens single-use 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Use crypto.timingSafeEqual() for API key validation (prevents timing attacks) - Make WebSocket tokens single-use (invalidated after first validation) - Add AUTOMAKER_HIDE_API_KEY env var to suppress API key banner in logs - Add rate limiting to login endpoint (5 attempts/minute/IP) - Update client to fetch short-lived wsToken for WebSocket auth (session tokens no longer exposed in URLs) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Added NODE_ENV variable for development in docker-compose.override.yml.example. - Changed default NODE_ENV to development in Dockerfile. - Implemented fetchWsToken function to retrieve short-lived WebSocket tokens for secure authentication in TerminalPanel. - Updated connect function to use wsToken for WebSocket connections when API key is not available. - Introduced verifySession function to validate session status after login and on app load, ensuring session integrity. - Modified RootLayoutContent to verify session cookie validity and redirect to login if the session is invalid or expired. These changes improve the security and reliability of the authentication process.
|
@CodeRabbit re-review |
|
@webdevcody I'll re-review the changes in this PR for you. ✅ Actions performedReview triggered.
|
- Changed the resolved URL for the @electron/node-gyp module from SSH to HTTPS for improved accessibility and compatibility.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
apps/server/src/lib/auth.ts (2)
131-146: API key banner alignment issue remains.The banner box is misaligned because line 139 doesn't account for the API key length (36 characters for UUID) and is missing the closing
║. This was noted in a previous review.🔎 Proposed fix for alignment
-║ ${API_KEY} +║ ${API_KEY.padEnd(67)}║
391-398: Duplication withcheckAuthenticationnoted in previous review.This function is a thin wrapper that calls
checkAuthenticationand returns only the boolean result. While this creates some duplication with the middleware, it provides a cleaner API for callers that only need a boolean. The current approach is acceptable.
🧹 Nitpick comments (4)
apps/ui/src/lib/http-api-client.ts (1)
227-270: Consider making the verification endpoint more explicit.The
verifySession()function uses/api/settings/statusfor verification, which creates a tight coupling to a specific endpoint. While functional, this could be fragile if that endpoint changes or is unavailable.Consider one of these approaches:
- Use a dedicated
/api/auth/verifyendpoint for session verification- Document the dependency on
/api/settings/statusclearly in comments- Add fallback verification logic if the primary endpoint fails
This is a minor maintainability concern and can be addressed in a future refactor.
apps/ui/src/routes/__root.tsx (1)
214-220: LGTM! Proper loading state prevents unauthorized content flash.The loading state prevents users from briefly seeing protected content before authentication completes. The check for
!isElectronMode()correctly limits this behavior to web mode only.Minor UX enhancement: Consider making the loading message more specific:
- <div className="text-muted-foreground">Loading...</div> + <div className="text-muted-foreground">Verifying authentication...</div>This is purely cosmetic and can be deferred.
apps/server/src/routes/auth/index.ts (1)
27-45: Document the single-instance limitation of in-memory rate limiting.The in-memory rate limiting approach works well for single-instance deployments (local development, single-container Docker), but won't work correctly in multi-instance scenarios where each instance maintains its own
loginAttemptsmap.Consider adding a comment to document this limitation:
// Rate limiting configuration const RATE_LIMIT_WINDOW_MS = 60 * 1000; // 1 minute window const RATE_LIMIT_MAX_ATTEMPTS = 5; // Max 5 attempts per window -// In-memory rate limit tracking (resets on server restart) +// In-memory rate limit tracking (resets on server restart) +// NOTE: This is per-instance. For multi-instance deployments, use a shared store (Redis, etc.) const loginAttempts = new Map<string, { count: number; windowStart: number }>();For now, this is acceptable given the deployment scenarios, but should be addressed before horizontal scaling.
apps/server/src/lib/auth.ts (1)
309-316: API key in query parameter lacks timing-safe comparison for length check.When checking
queryKey, the flow goes tovalidateApiKeywhich is timing-safe. However, the existence checkif (queryKey)could theoretically leak timing information about whether the parameter was provided.In practice, this is a minor concern since the mere presence of a query parameter is observable in logs/network. The actual value validation is timing-safe.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
Dockerfileapps/server/package.jsonapps/server/src/index.tsapps/server/src/lib/auth.tsapps/server/src/routes/auth/index.tsapps/server/tests/unit/lib/auth.test.tsapps/ui/src/components/views/settings-view.tsxapps/ui/src/components/views/terminal-view/terminal-panel.tsxapps/ui/src/lib/http-api-client.tsapps/ui/src/routes/__root.tsxdocker-compose.override.yml.exampledocs/plans/2025-12-29-api-security-hardening-design.md
💤 Files with no reviewable changes (1)
- Dockerfile
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always import from shared packages (@automaker/*), never from old relative paths
Files:
apps/ui/src/components/views/settings-view.tsxapps/ui/src/components/views/terminal-view/terminal-panel.tsxapps/ui/src/routes/__root.tsxapps/server/src/index.tsapps/server/tests/unit/lib/auth.test.tsapps/server/src/lib/auth.tsapps/ui/src/lib/http-api-client.tsapps/server/src/routes/auth/index.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
resolveModelString()from @automaker/model-resolver to convert model aliases (haiku, sonnet, opus) to full model names
Files:
apps/ui/src/components/views/settings-view.tsxapps/ui/src/components/views/terminal-view/terminal-panel.tsxapps/ui/src/routes/__root.tsxapps/server/src/index.tsapps/server/tests/unit/lib/auth.test.tsapps/server/src/lib/auth.tsapps/ui/src/lib/http-api-client.tsapps/server/src/routes/auth/index.ts
apps/server/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
createEventEmitter()fromlib/events.tsfor all server operations to emit events that stream to frontend via WebSocket
Files:
apps/server/src/index.tsapps/server/src/lib/auth.tsapps/server/src/routes/auth/index.ts
🧠 Learnings (2)
📚 Learning: 2025-12-28T05:07:48.138Z
Learnt from: CR
Repo: AutoMaker-Org/automaker PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-28T05:07:48.138Z
Learning: Frontend UI must use TanStack Router for file-based routing, organize components in components/views/, implement stores with Zustand, and use custom hooks in hooks/ directory
Applied to files:
apps/ui/src/components/views/settings-view.tsx
📚 Learning: 2025-12-28T05:07:48.138Z
Learnt from: CR
Repo: AutoMaker-Org/automaker PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-28T05:07:48.138Z
Learning: Applies to apps/server/src/**/*.{ts,tsx} : Use `createEventEmitter()` from `lib/events.ts` for all server operations to emit events that stream to frontend via WebSocket
Applied to files:
apps/ui/src/components/views/terminal-view/terminal-panel.tsxapps/server/src/index.ts
🧬 Code graph analysis (5)
apps/ui/src/components/views/settings-view.tsx (2)
apps/ui/src/store/setup-store.ts (1)
useSetupStore(125-183)init.mjs (1)
isWindows(33-33)
apps/ui/src/components/views/terminal-view/terminal-panel.tsx (1)
apps/ui/src/lib/http-api-client.ts (3)
fetchWsToken(306-338)getSessionToken(56-56)getApiKey(53-53)
apps/ui/src/routes/__root.tsx (1)
apps/ui/src/lib/http-api-client.ts (3)
initApiKey(81-100)isElectronMode(71-73)verifySession(227-270)
apps/server/tests/unit/lib/auth.test.ts (1)
apps/server/src/lib/auth.ts (2)
isAuthEnabled(374-376)getAuthStatus(381-386)
apps/server/src/routes/auth/index.ts (1)
apps/server/src/lib/auth.ts (7)
isRequestAuthenticated(391-398)validateApiKey(233-247)createSession(158-167)getSessionCookieOptions(252-266)getSessionCookieName(271-273)createWsConnectionToken(199-207)invalidateSession(190-193)
🪛 LanguageTool
docs/plans/2025-12-29-api-security-hardening-design.md
[grammar] ~53-~53: Use a hyphen to join words.
Context: ...ver/src/routes/auth/index.ts` - Add rate limiting logic to login handler --- ##...
(QB_NEW_EN_HYPHEN)
⏰ 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). (1)
- GitHub Check: e2e
🔇 Additional comments (36)
apps/ui/src/components/views/settings-view.tsx (3)
3-3: LGTM!The import is necessary to access the Claude authentication status from the setup store.
59-59: LGTM!Correctly retrieves the Claude authentication status using the Zustand selector pattern.
62-70: Previous review concern resolved – logic looks correct.The implementation now includes the
isCliVerifiedcheck as suggested in the previous review, ensuring consistency withboard-header.tsx. The logic correctly:
- Checks for API keys from both user input and environment variables
- Verifies CLI authentication status and method
- Combines all conditions appropriately with Windows platform exclusion
docker-compose.override.yml.example (1)
11-11: LGTM! Appropriate development configuration.Adding
NODE_ENV=developmentaligns with the server-side changes that conditionally enable/disable features based on environment (e.g., secure cookies, CORS origins). This is a good practice for development setups.apps/ui/src/lib/http-api-client.ts (3)
44-73: LGTM! Clean dual-mode authentication state management.The module-level caching approach is appropriate for a singleton client, and the exported getters/setters provide controlled access to authentication state for both Electron (API key) and Web (session token) modes.
81-100: LGTM! Proper initialization with guard pattern.The initialization guard and error handling ensure robust startup behavior for both Electron and Web modes. The logging is helpful for debugging authentication issues.
306-371: LGTM! Well-structured WebSocket authentication flow.The separation of
fetchWsToken()andestablishWebSocket()provides good error handling and supports both Electron (API key) and Web (wsToken) modes. The fallback warning at line 363 is appropriate for debugging auth issues.apps/ui/src/components/views/terminal-view/terminal-panel.tsx (1)
977-1207: LGTM! WebSocket authentication properly implements short-lived wsToken flow.The connection logic correctly implements the security-hardened approach:
- Electron mode: Uses API key directly (line 982-984)
- Web mode: Fetches short-lived wsToken before connecting (line 987-990)
- Falls back to cookie-based auth if wsToken is unavailable (line 991-992)
This addresses the security concern from previous reviews about exposing session tokens in URLs. The wsToken approach is more secure and aligns with the design document.
Note: The code duplication issue raised in the previous comment should still be addressed to improve maintainability.
apps/ui/src/routes/__root.tsx (1)
79-121: Verify the redirect logic handles navigation race conditions.The authentication initialization effect depends on
location.pathnameandnavigate, which means it will re-run whenever the user navigates. While the guards at lines 107 and 114 prevent infinite loops, there could be edge cases:
- User navigates to another route while auth is being verified
- Multiple simultaneous navigation attempts during auth initialization
- Browser back/forward button during auth check
Test the following scenarios to ensure robust behavior:
- User navigates to a protected route before auth completes
- User clicks browser back button during auth initialization
- Rapid navigation between routes while
verifySession()is in flightConsider adding a ref to track if initialization is already in progress:
const authInitInProgressRef = useRef(false); useEffect(() => { if (authInitInProgressRef.current) return; // Prevent concurrent init authInitInProgressRef.current = true; const initAuth = async () => { try { // ... existing logic ... } finally { authInitInProgressRef.current = false; } }; initAuth(); }, [location.pathname, navigate]);This would prevent race conditions from concurrent auth checks.
apps/server/package.json (2)
32-32: LGTM! Standard dependency addition for cookie handling.Adding
cookie-parseris appropriate for the new cookie-based session authentication. The version (1.4.7) is current and stable.
41-42: LGTM! Type definitions for TypeScript support.Adding type definitions for
cookieandcookie-parserprovides proper TypeScript support for the new cookie handling functionality.apps/server/tests/unit/lib/auth.test.ts (3)
13-26: LGTM! Test correctly validates authentication requirement.The test suite correctly reflects the always-on authentication model. The updated error message "Authentication required." is clearer than the previous version.
59-64: LGTM! Test aligns with always-on authentication.The test correctly expects
isAuthEnabled()to always returntrue, matching the new authentication model where API key protection is always active.
67-76: LGTM! Test reflects multi-path authentication.The test correctly expects the
api_key_or_sessionmethod, which accurately describes the new authentication system that supports both API key headers and session cookies.docs/plans/2025-12-29-api-security-hardening-design.md (1)
1-94: LGTM! Comprehensive security hardening design.The design document clearly outlines five security improvements with specific implementation targets. The phased implementation order is logical, starting with simple isolated changes and progressing to more complex coordinated updates.
Based on my review of the other files in this PR:
- ✅ Fix #1 (wsToken): Implemented in
http-api-client.tsandterminal-panel.tsx- ✅ Fix #3 (rate limiting): Implemented in
apps/server/src/routes/auth/index.ts(next file to review)- ⏳ Fix #2, #4, #5: Should be in
apps/server/src/lib/auth.ts(not provided in this review)The implementation appears to follow the design document correctly.
apps/server/src/routes/auth/index.ts (5)
51-59: LGTM! Standard IP detection with appropriate fallbacks.The IP detection correctly handles reverse proxy scenarios with
X-Forwarded-Forand provides sensible fallbacks.Note on security:
X-Forwarded-Forcan be spoofed by clients if not behind a trusted proxy. However, this rate limiting is a defense-in-depth measure—the primary security is the timing-safe API key validation. The current implementation is acceptable for the intended deployment scenarios.
65-86: LGTM! Correct rate limit checking with proper cleanup.The rate limit logic correctly handles window expiration and provides accurate
retryAftervalues for clients to implement exponential backoff. The window-based approach is more user-friendly than a simple counter.
135-182: LGTM! Secure login implementation with proper rate limiting.The login endpoint correctly implements:
- ✅ Rate limiting before processing (prevents timing attacks)
- ✅ Records all attempts (including malformed requests)
- ✅ Timing-safe API key validation via
validateApiKey()- ✅ HTTP-only cookie for primary auth
- ✅ Returns session token for header-based auth (documented use case)
Security note: The session token is returned in the response body (line 180) to support explicit header-based authentication (
X-Session-Token). While this reduces some benefits of HTTP-only cookies, it's necessary for cross-origin scenarios and is documented in the comments. The primary session cookie remains HTTP-only.
191-209: LGTM! Secure wsToken generation for WebSocket authentication.The
/tokenendpoint correctly implements the security-hardened approach described in the design document:
- ✅ Validates existing session before issuing wsToken
- ✅ Generates a NEW short-lived token (not exposing the session cookie)
- ✅ Returns 401 for unauthenticated requests
- ✅ Documents the 5-minute expiry
This addresses the previous security concern about exposing session values. The wsToken is a separate, short-lived credential suitable for WebSocket URLs.
216-236: LGTM! Proper logout implementation with session cleanup.The logout endpoint correctly:
- ✅ Invalidates the server-side session
- ✅ Clears the HTTP-only cookie with matching options
- ✅ Handles missing session gracefully
The cookie clearing options (lines 225-230) match the creation options, ensuring proper cleanup.
apps/server/src/index.ts (7)
12-13: Good use of thecookielibrary for parsing.The import of
cookieaddresses the previous review feedback about brittle manual cookie parsing. This is the correct approach.
20-21: Imports align with the new authentication architecture.The new imports (
validateWsConnectionToken,checkRawAuthentication,createAuthRoutes,createDetailedHandler) support the multi-path authentication system being introduced.Also applies to: 23-23
116-142: CORS configuration correctly handles credentials with dynamic origins.The logic properly:
- Allows no-origin requests (Electron, curl, mobile apps)
- Respects
CORS_ORIGINenv var allowlist- Permits localhost for development
- Rejects other origins by default
One consideration: the
allowedOrigins[0] !== '*'check on line 125 allows*to bypass validation, which would then fall through to localhost-only. This is likely intentional but worth documenting.
176-178: Auth routes are correctly placed before the auth middleware.The
/api/healthand/api/authroutes are mounted beforeauthMiddlewareis applied, allowing unauthenticated access to health checks and login endpoints.
183-184: Detailed health endpoint is protected.The
/api/health/detailedendpoint is mounted afterauthMiddlewareis applied on line 181, ensuring it requires authentication.
218-253: WebSocket authentication function is well-structured.The function correctly:
- Parses URL and query parameters
- Uses the
cookielibrary for parsing (addressing past review)- Delegates to shared
checkRawAuthenticationfor consistency- Additionally validates short-lived
wsTokenfor WebSocket-specific authThe layered approach (shared auth logic + WebSocket-specific tokens) is appropriate for the security model.
259-265: WebSocket upgrade now requires authentication.All WebSocket connections are now authenticated before upgrade, with proper 401 response and socket destruction on failure. This closes the security gap mentioned in the PR objectives.
apps/server/src/lib/auth.ts (9)
29-37: Periodic cleanup prevents unbounded memory growth.The 60-second interval cleanup for expired WebSocket tokens is appropriate given their 5-minute lifetime. This prevents memory leaks from tokens that are created but never used.
42-70: Session loading handles expiration correctly.The function properly filters out expired sessions during load and logs helpful diagnostics. Using synchronous read at startup is acceptable since this runs once during initialization.
75-86: Session persistence is now async.This addresses the previous review feedback about blocking I/O. The file permissions (
0o600) correctly restrict access to the owner only.
115-124: Synchronous file write during key generation.The
fs.writeFileSyncat line 119 is acceptable here since it only runs once during startup when generating a new API key. The0o600permissions correctly restrict access.
173-185: Fire-and-forget pattern for session cleanup is appropriate.The synchronous return with async persistence (
saveSessions().catch(...)) ensures fast validation while still persisting changes. The error handling prevents unhandled rejections.
214-227: Single-use WebSocket tokens are correctly implemented.The token is deleted immediately upon lookup (line 219), before checking expiration. This ensures tokens can only be used once, even if validation fails due to expiration.
233-247: Timing-safe API key comparison is correctly implemented.The implementation:
- Validates input type and existence
- Uses
crypto.timingSafeEqualfor constant-time comparison- Handles length mismatch by performing a dummy comparison to maintain constant time
This prevents timing attacks that could leak information about the key.
259-266: Session cookie options are secure.The configuration correctly:
- Sets
httpOnly: trueto prevent XSS access- Enables
secureonly in production (allows HTTP in development)- Uses
sameSite: 'strict'for CSRF protection- Sets appropriate max age matching session lifetime
404-409:checkRawAuthenticationprovides clean interface for WebSocket auth.This wrapper appropriately exposes the authentication check for non-Express contexts like WebSocket upgrade handlers, returning only the boolean result needed.
| // Check for session cookie (web mode) | ||
| const sessionToken = cookies[SESSION_COOKIE_NAME]; | ||
| if (sessionToken && validateSession(sessionToken)) { | ||
| return { authenticated: true }; | ||
| } |
There was a problem hiding this comment.
Cookie session validation inconsistency.
Unlike other auth methods (lines 293-297, 302-307, 311-316), the cookie-based session check on line 320 doesn't return invalid_session error type when the token exists but is invalid. It falls through to no_auth.
This means an expired cookie session returns "Authentication required" (401) instead of "Invalid or expired session token" (403), which may confuse users.
🔎 Proposed fix for consistent error handling
// Check for session cookie (web mode)
const sessionToken = cookies[SESSION_COOKIE_NAME];
- if (sessionToken && validateSession(sessionToken)) {
- return { authenticated: true };
+ if (sessionToken) {
+ if (validateSession(sessionToken)) {
+ return { authenticated: true };
+ }
+ return { authenticated: false, errorType: 'invalid_session' };
}
return { authenticated: false, errorType: 'no_auth' };📝 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.
| // Check for session cookie (web mode) | |
| const sessionToken = cookies[SESSION_COOKIE_NAME]; | |
| if (sessionToken && validateSession(sessionToken)) { | |
| return { authenticated: true }; | |
| } | |
| // Check for session cookie (web mode) | |
| const sessionToken = cookies[SESSION_COOKIE_NAME]; | |
| if (sessionToken) { | |
| if (validateSession(sessionToken)) { | |
| return { authenticated: true }; | |
| } | |
| return { authenticated: false, errorType: 'invalid_session' }; | |
| } | |
| return { authenticated: false, errorType: 'no_auth' }; |
🤖 Prompt for AI Agents
In apps/server/src/lib/auth.ts around lines 318 to 322, the cookie session check
currently returns authenticated on a valid token but falls through to "no_auth"
when a session cookie exists but validateSession(token) returns false; change
the logic so that when a session cookie is present but
validateSession(sessionToken) fails you immediately return the same
invalid_session response shape used elsewhere (e.g., { authenticated: false,
errorType: 'invalid_session', message: 'Invalid or expired session token' }) so
cookie-based failures map to 403 like the other auth methods.
- Introduced `requireJsonContentType` middleware to ensure that all POST, PUT, and PATCH requests have the Content-Type set to application/json. - This enhancement improves security by preventing CSRF and content-type confusion attacks, ensuring only properly formatted requests are processed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/server/src/index.ts (1)
135-139: Consider allowing HTTPS localhost origins for local development.The current logic only allows
http://localhost:andhttp://127.0.0.1:origins. Developers using HTTPS for local development (e.g., with mkcert or self-signed certificates) would be blocked.🔎 Proposed fix to support HTTPS localhost
- // For local development, allow localhost origins - if (origin.startsWith('http://localhost:') || origin.startsWith('http://127.0.0.1:')) { + // For local development, allow localhost origins (HTTP and HTTPS) + if ( + origin.startsWith('http://localhost:') || + origin.startsWith('http://127.0.0.1:') || + origin.startsWith('https://localhost:') || + origin.startsWith('https://127.0.0.1:') + ) { callback(null, origin); return; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/server/src/index.tsapps/server/src/middleware/require-json-content-type.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always import from shared packages (@automaker/*), never from old relative paths
Files:
apps/server/src/middleware/require-json-content-type.tsapps/server/src/index.ts
apps/server/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
createEventEmitter()fromlib/events.tsfor all server operations to emit events that stream to frontend via WebSocket
Files:
apps/server/src/middleware/require-json-content-type.tsapps/server/src/index.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
resolveModelString()from @automaker/model-resolver to convert model aliases (haiku, sonnet, opus) to full model names
Files:
apps/server/src/middleware/require-json-content-type.tsapps/server/src/index.ts
🧠 Learnings (1)
📚 Learning: 2025-12-28T05:07:48.138Z
Learnt from: CR
Repo: AutoMaker-Org/automaker PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-28T05:07:48.138Z
Learning: Applies to apps/server/src/**/*.{ts,tsx} : Use `createEventEmitter()` from `lib/events.ts` for all server operations to emit events that stream to frontend via WebSocket
Applied to files:
apps/server/src/index.ts
⏰ 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). (1)
- GitHub Check: e2e
🔇 Additional comments (4)
apps/server/src/index.ts (3)
177-179: LGTM - Correct middleware positioning for API security.The
requireJsonContentTypemiddleware is correctly mounted before all API routes (including auth and health), ensuring consistent JSON content-type enforcement across the entire API surface for CSRF protection.
227-258: LGTM - Proper cookie parsing and multi-source authentication.The WebSocket authentication correctly uses
cookie.parse()from thecookiepackage (line 238) for robust cookie parsing, avoiding the brittleness of manual string manipulation. The function properly checks multiple authentication sources (API key, session token, session cookie, and wsToken) for flexible client support.
264-270: LGTM - WebSocket authentication properly enforced.The authentication check correctly protects all WebSocket upgrade requests, rejecting unauthorized connections with a 401 response before the upgrade completes. This aligns with the PR's security objectives.
apps/server/src/middleware/require-json-content-type.ts (1)
29-50: The middleware behavior is documented as intentional. The code comment inapps/server/src/index.ts(lines 177-178) explicitly states the requirement is to "prevent CSRF and content-type confusion attacks." This security rationale is sound and the strict enforcement is working as designed. No issues found in the codebase.
- Added comprehensive unit tests for authentication middleware, including session token validation, API key authentication, and cookie-based authentication. - Implemented tests for session management functions such as creating, updating, archiving, and deleting sessions. - Improved test coverage for queue management in session handling, ensuring robust error handling and validation. - Introduced checks for session metadata and working directory validation to ensure proper session creation.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
apps/server/tests/unit/services/agent-service.test.ts (1)
93-112: Consider testing behavior rather than implementation details.The assertion on line 111 checks for an exact count of
fs.readFilecalls, which tightly couples the test to the current implementation. If the service changes how it stores or retrieves session data (e.g., combining files, adding caching), this test will fail even if the public behavior remains correct.Consider refactoring to verify the observable behavior (e.g., that the second call returns cached data quickly, or that the session state is consistent) rather than counting internal I/O operations.
apps/server/tests/unit/lib/auth.test.ts (2)
3-4: Remove unused imports.The
fsandpathimports are not used anywhere in this test file.🔎 Proposed fix
import { describe, it, expect, beforeEach, vi } from 'vitest'; import { createMockExpressContext } from '../../utils/mocks.js'; -import fs from 'fs'; -import path from 'path'; /**
150-160: Add timer cleanup to prevent test pollution.If a test fails before reaching
vi.useRealTimers(), fake timers remain active and could affect subsequent tests. Move timer restoration to anafterEachhook within the relevant describe blocks or use try-finally.🔎 Example approach using afterEach
For the
validateSessionsuite:describe('validateSession', () => { + afterEach(() => { + vi.useRealTimers(); + }); + it('should validate a valid session token', async () => { // ... existing test }); it('should reject expired session token', async () => { vi.useFakeTimers(); const { createSession, validateSession } = await import('@/lib/auth.js'); const token = await createSession(); // Advance time past session expiration (30 days) vi.advanceTimersByTime(31 * 24 * 60 * 60 * 1000); expect(validateSession(token)).toBe(false); - vi.useRealTimers(); }); });Apply similar changes to the
validateWsConnectionTokensuite.Also applies to: 207-217
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/server/tests/unit/lib/auth.test.tsapps/server/tests/unit/services/agent-service.test.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always import from shared packages (@automaker/*), never from old relative paths
Files:
apps/server/tests/unit/services/agent-service.test.tsapps/server/tests/unit/lib/auth.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
resolveModelString()from @automaker/model-resolver to convert model aliases (haiku, sonnet, opus) to full model names
Files:
apps/server/tests/unit/services/agent-service.test.tsapps/server/tests/unit/lib/auth.test.ts
🧬 Code graph analysis (1)
apps/server/tests/unit/lib/auth.test.ts (1)
apps/server/src/lib/auth.ts (13)
authMiddleware(336-369)createSession(158-167)getSessionCookieName(271-273)validateSession(173-185)invalidateSession(190-193)createWsConnectionToken(199-207)validateWsConnectionToken(214-227)validateApiKey(233-247)getSessionCookieOptions(252-266)isRequestAuthenticated(391-398)checkRawAuthentication(404-410)isAuthEnabled(374-376)getAuthStatus(381-386)
⏰ 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). (1)
- GitHub Check: e2e
🔇 Additional comments (12)
apps/server/tests/unit/services/agent-service.test.ts (1)
239-264: Verify model string usage in tests.The test uses the hardcoded model string
'claude-sonnet-4-20250514'. According to the coding guidelines,resolveModelString()from@automaker/model-resolvershould be used to convert model aliases to full model names.While the guideline specifically mentions aliases like "haiku", "sonnet", and "opus", it's unclear whether tests should also use
resolveModelString()for consistency with production code, or if hardcoded strings are acceptable for test stability.Please verify whether test files should use
resolveModelString()or if hardcoded model strings are appropriate for testing purposes.Similar usage appears on:
- Line 420:
service.setSessionModel('session-1', 'claude-sonnet-4-20250514')- Line 426:
service.setSessionModel('nonexistent', 'claude-sonnet-4-20250514')- Line 609:
model: 'claude-sonnet-4-20250514'As per coding guidelines, please clarify the expected approach for model strings in tests.
apps/server/tests/unit/lib/auth.test.ts (11)
11-16: LGTM!The
beforeEachhook correctly resets modules and clears environment variables to ensure test isolation, which is essential when testing module-level initialization behavior.
18-115: LGTM!The
authMiddlewaretest suite provides comprehensive coverage of all authentication paths: API key (header and query), session tokens (header and cookie), and proper error responses (401 for missing auth, 403 for invalid credentials).
117-134: LGTM!Session creation tests properly verify that tokens are generated as non-empty strings and that each call produces unique tokens, which is critical for security.
136-161: LGTM!Session validation tests comprehensively cover valid tokens, invalid tokens, and expiration scenarios using fake timers.
163-172: LGTM!The session invalidation test properly verifies the complete lifecycle, ensuring that a valid session becomes invalid after invalidation.
174-191: LGTM!WebSocket token creation tests correctly verify token generation and uniqueness, matching the pattern established for session token creation.
193-227: LGTM!WebSocket token validation tests are comprehensive, including the critical single-use behavior where a token is consumed after first validation. The expiration test correctly verifies the 5-minute timeout.
229-272: LGTM!API key validation tests are thorough, covering correct keys, incorrect keys, empty strings, null/undefined inputs, and importantly, timing-safe comparison behavior. The use of
as anyin lines 259-260 is appropriate for testing invalid input types that TypeScript would normally reject.
274-302: LGTM!Cookie options tests properly verify all security-relevant properties including
httpOnly,sameSite, and environment-dependentsecureflag behavior.
304-311: LGTM!The cookie name test correctly verifies the expected constant value.
313-398: LGTM!The remaining test suites (
isRequestAuthenticated,checkRawAuthentication,isAuthEnabled,getAuthStatus) provide comprehensive coverage of authentication helper functions. Tests correctly verify all authentication methods (API key, session token, cookies) and properly assert that authentication is always enabled with the combinedapi_key_or_sessionmethod.
| describe('createSession', () => { | ||
| it('should create a new session and return token', async () => { | ||
| const { createSession } = await import('@/lib/auth.js'); | ||
| const token = await createSession(); | ||
|
|
||
| const { isAuthEnabled } = await import('@/lib/auth.js'); | ||
| expect(isAuthEnabled()).toBe(false); | ||
| expect(token).toBeDefined(); | ||
| expect(typeof token).toBe('string'); | ||
| expect(token.length).toBeGreaterThan(0); | ||
| }); | ||
|
|
||
| it('should return true when API key is set', async () => { | ||
| process.env.AUTOMAKER_API_KEY = 'test-key'; | ||
| it('should create unique tokens for each session', async () => { | ||
| const { createSession } = await import('@/lib/auth.js'); | ||
| const token1 = await createSession(); | ||
| const token2 = await createSession(); | ||
|
|
||
| const { isAuthEnabled } = await import('@/lib/auth.js'); | ||
| expect(isAuthEnabled()).toBe(true); | ||
| expect(token1).not.toBe(token2); | ||
| }); | ||
| }); | ||
|
|
||
| describe('getAuthStatus', () => { | ||
| it('should return disabled status when no API key', async () => { | ||
| delete process.env.AUTOMAKER_API_KEY; | ||
| describe('validateSession', () => { | ||
| it('should validate a valid session token', async () => { | ||
| const { createSession, validateSession } = await import('@/lib/auth.js'); | ||
| const token = await createSession(); | ||
|
|
||
| const { getAuthStatus } = await import('@/lib/auth.js'); | ||
| const status = getAuthStatus(); | ||
| expect(validateSession(token)).toBe(true); | ||
| }); | ||
|
|
||
| expect(status).toEqual({ | ||
| enabled: false, | ||
| method: 'none', | ||
| }); | ||
| it('should reject invalid session token', async () => { | ||
| const { validateSession } = await import('@/lib/auth.js'); | ||
|
|
||
| expect(validateSession('invalid-token')).toBe(false); | ||
| }); | ||
|
|
||
| it('should reject expired session token', async () => { | ||
| vi.useFakeTimers(); | ||
| const { createSession, validateSession } = await import('@/lib/auth.js'); | ||
| const token = await createSession(); | ||
|
|
||
| // Advance time past session expiration (30 days) | ||
| vi.advanceTimersByTime(31 * 24 * 60 * 60 * 1000); | ||
|
|
||
| expect(validateSession(token)).toBe(false); | ||
| vi.useRealTimers(); | ||
| }); | ||
| }); | ||
|
|
||
| describe('invalidateSession', () => { | ||
| it('should invalidate a session token', async () => { | ||
| const { createSession, validateSession, invalidateSession } = await import('@/lib/auth.js'); | ||
| const token = await createSession(); | ||
|
|
||
| expect(validateSession(token)).toBe(true); | ||
| await invalidateSession(token); | ||
| expect(validateSession(token)).toBe(false); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check the structure and setup of the test file
head -120 apps/server/tests/unit/lib/auth.test.ts | tail -30Repository: AutoMaker-Org/automaker
Length of output: 1147
🏁 Script executed:
# Check for any test setup, mocking, or teardown
rg -n "beforeEach|afterEach|vi\.mock|mock|setup|teardown" apps/server/tests/unit/lib/auth.test.ts -B 2 -A 2Repository: AutoMaker-Org/automaker
Length of output: 383
🏁 Script executed:
# Check the auth.ts implementation for saveSessions and loadSessions
rg -n "saveSessions|loadSessions|fs\." apps/server/src/lib/auth.ts -B 2 -A 5Repository: AutoMaker-Org/automaker
Length of output: 2569
🏁 Script executed:
# Check test utilities and mocks configuration
find apps/server/tests -name "*.ts" -type f | xargs rg -l "mock|Mock" | head -5Repository: AutoMaker-Org/automaker
Length of output: 322
🏁 Script executed:
# Check if there's a vitest config or setup file that might handle mocking
find apps/server -name "vitest.config.*" -o -name "setup*.ts" -o -name "*.setup.ts" | head -10Repository: AutoMaker-Org/automaker
Length of output: 122
🏁 Script executed:
cat -n apps/server/tests/setup.tsRepository: AutoMaker-Org/automaker
Length of output: 468
🏁 Script executed:
cat -n apps/server/vitest.config.tsRepository: AutoMaker-Org/automaker
Length of output: 2555
🏁 Script executed:
cat -n apps/server/tests/utils/mocks.tsRepository: AutoMaker-Org/automaker
Length of output: 3237
🏁 Script executed:
# Check if there's a .gitigore or similar that indicates session files are gitignored
find apps/server -name ".gitignore" -o -name "*.env*" | xargs grep -l "session\|\.auth\|\.key" 2>/dev/nullRepository: AutoMaker-Org/automaker
Length of output: 91
Mock the file system or implement cleanup in session tests.
Tests create and invalidate sessions, triggering real file writes via saveSessions() to /tmp/test-data. No mocks or cleanup are applied, leaving orphaned session files. A createMockFs() utility already exists in tests/utils/mocks.ts but isn't used here. Either mock fs using vi.mock() or apply createMockFs() with proper module isolation, or implement afterEach() cleanup to delete session files.
| it('should validate working directory', async () => { | ||
| // Set ALLOWED_ROOT_DIRECTORY to restrict paths | ||
| const originalAllowedRoot = process.env.ALLOWED_ROOT_DIRECTORY; | ||
| process.env.ALLOWED_ROOT_DIRECTORY = '/allowed/projects'; | ||
|
|
||
| // Re-import platform to initialize with new env var | ||
| vi.resetModules(); | ||
| const { initAllowedPaths } = await import('@automaker/platform'); | ||
| initAllowedPaths(); | ||
|
|
||
| const { AgentService } = await import('@/services/agent-service.js'); | ||
| const testService = new AgentService('/test/data', mockEvents as any); | ||
| vi.mocked(fs.readFile).mockResolvedValue('{}'); | ||
| vi.mocked(fs.writeFile).mockResolvedValue(undefined); | ||
|
|
||
| await expect( | ||
| testService.createSession('Test Session', undefined, '/invalid/path') | ||
| ).rejects.toThrow(); | ||
|
|
||
| // Restore original value | ||
| if (originalAllowedRoot) { | ||
| process.env.ALLOWED_ROOT_DIRECTORY = originalAllowedRoot; | ||
| } else { | ||
| delete process.env.ALLOWED_ROOT_DIRECTORY; | ||
| } | ||
| vi.resetModules(); | ||
| const { initAllowedPaths: reinit } = await import('@automaker/platform'); | ||
| reinit(); | ||
| }); |
There was a problem hiding this comment.
Isolate environment manipulation to prevent test interference.
This test modifies global state (process.env.ALLOWED_ROOT_DIRECTORY) and clears the module cache (vi.resetModules()), which can cause race conditions and flaky failures when tests run concurrently.
Vitest executes tests in parallel by default, so manipulating shared global state without proper isolation can affect other tests. Even though the test attempts cleanup, there's a timing window where concurrent tests may observe the modified environment.
🔎 Recommended approaches to fix
Option 1: Mark this test to run sequentially
- it('should validate working directory', async () => {
+ it.sequential('should validate working directory', async () => {Option 2: Use a separate test suite with sequential execution
- describe('createSession', () => {
+ describe('createSession', () => {
// ... other tests ...
+ });
+
+ describe.sequential('createSession - environment validation', () => {
it('should validate working directory', async () => {
// ... test body ...
});
});Option 3: Mock the validation function directly instead of manipulating the environment, if the architecture allows it.
📝 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.
| it('should validate working directory', async () => { | |
| // Set ALLOWED_ROOT_DIRECTORY to restrict paths | |
| const originalAllowedRoot = process.env.ALLOWED_ROOT_DIRECTORY; | |
| process.env.ALLOWED_ROOT_DIRECTORY = '/allowed/projects'; | |
| // Re-import platform to initialize with new env var | |
| vi.resetModules(); | |
| const { initAllowedPaths } = await import('@automaker/platform'); | |
| initAllowedPaths(); | |
| const { AgentService } = await import('@/services/agent-service.js'); | |
| const testService = new AgentService('/test/data', mockEvents as any); | |
| vi.mocked(fs.readFile).mockResolvedValue('{}'); | |
| vi.mocked(fs.writeFile).mockResolvedValue(undefined); | |
| await expect( | |
| testService.createSession('Test Session', undefined, '/invalid/path') | |
| ).rejects.toThrow(); | |
| // Restore original value | |
| if (originalAllowedRoot) { | |
| process.env.ALLOWED_ROOT_DIRECTORY = originalAllowedRoot; | |
| } else { | |
| delete process.env.ALLOWED_ROOT_DIRECTORY; | |
| } | |
| vi.resetModules(); | |
| const { initAllowedPaths: reinit } = await import('@automaker/platform'); | |
| reinit(); | |
| }); | |
| it.sequential('should validate working directory', async () => { | |
| // Set ALLOWED_ROOT_DIRECTORY to restrict paths | |
| const originalAllowedRoot = process.env.ALLOWED_ROOT_DIRECTORY; | |
| process.env.ALLOWED_ROOT_DIRECTORY = '/allowed/projects'; | |
| // Re-import platform to initialize with new env var | |
| vi.resetModules(); | |
| const { initAllowedPaths } = await import('@automaker/platform'); | |
| initAllowedPaths(); | |
| const { AgentService } = await import('@/services/agent-service.js'); | |
| const testService = new AgentService('/test/data', mockEvents as any); | |
| vi.mocked(fs.readFile).mockResolvedValue('{}'); | |
| vi.mocked(fs.writeFile).mockResolvedValue(undefined); | |
| await expect( | |
| testService.createSession('Test Session', undefined, '/invalid/path') | |
| ).rejects.toThrow(); | |
| // Restore original value | |
| if (originalAllowedRoot) { | |
| process.env.ALLOWED_ROOT_DIRECTORY = originalAllowedRoot; | |
| } else { | |
| delete process.env.ALLOWED_ROOT_DIRECTORY; | |
| } | |
| vi.resetModules(); | |
| const { initAllowedPaths: reinit } = await import('@automaker/platform'); | |
| reinit(); | |
| }); |
🤖 Prompt for AI Agents
apps/server/tests/unit/services/agent-service.test.ts lines 374-402: the test
mutates process.env.ALLOWED_ROOT_DIRECTORY and calls vi.resetModules(), which
can leak state and cause flakiness when Vitest runs tests in parallel; instead,
avoid changing global env and module cache by mocking the platform validation:
mock the function in '@automaker/platform' that checks/initializes allowed paths
(using vi.mock or vi.spyOn to return the desired allowed-roots behavior for this
test), remove process.env assignment and vi.resetModules calls, and assert
createSession rejects for the mocked invalid path; if mocking the platform
function is not feasible, mark this test (or its describe block) as serial
(describe.serial or test.serial) to ensure it runs isolated and keep cleanup
minimal.
- Added `authenticateForTests` utility to streamline API key authentication in tests, using a fallback for local testing. - Updated context image test to include authentication step before navigation, ensuring proper session handling. - Increased timeout for context view visibility to accommodate slower server responses. - Introduced a test API key in the Playwright configuration for consistent testing environments.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/ui/tests/context/add-context-image.spec.ts (1)
122-137: Refactor duplicate authentication logic for better maintainability.The test authenticates twice: first via API (line 123), then manually through the UI (lines 128-137). This redundancy makes the code harder to maintain and duplicates the hardcoded API key (
'test-api-key-for-e2e-tests') that already exists inclient.tsline 300.Consider refactoring to check the result of
authenticateForTestsand only fall back to manual login if it returnsfalse:🔎 Proposed refactor to eliminate duplication
+ // Authenticate with the server before navigating + const isAuthenticated = await authenticateForTests(page); + await page.goto('/'); await waitForNetworkIdle(page); - // Check if we're on the login screen and authenticate if needed - const loginInput = page.locator('input[type="password"][placeholder*="API key"]'); - const isLoginScreen = await loginInput.isVisible({ timeout: 2000 }).catch(() => false); - if (isLoginScreen) { - const apiKey = process.env.AUTOMAKER_API_KEY || 'test-api-key-for-e2e-tests'; - await loginInput.fill(apiKey); - await page.locator('button:has-text("Login")').click(); - await page.waitForURL('**/', { timeout: 5000 }); - await waitForNetworkIdle(page); - } + // If API authentication failed, try manual login as fallback + if (!isAuthenticated) { + const loginInput = page.locator('input[type="password"][placeholder*="API key"]'); + const isLoginScreen = await loginInput.isVisible({ timeout: 2000 }).catch(() => false); + if (isLoginScreen) { + const apiKey = process.env.AUTOMAKER_API_KEY || 'test-api-key-for-e2e-tests'; + await loginInput.fill(apiKey); + await page.locator('button:has-text("Login")').click(); + await page.waitForURL('**/', { timeout: 5000 }); + await waitForNetworkIdle(page); + } + }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/ui/playwright.config.tsapps/ui/tests/context/add-context-image.spec.tsapps/ui/tests/utils/api/client.tsapps/ui/tests/utils/navigation/views.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always import from shared packages (@automaker/*), never from old relative paths
Files:
apps/ui/playwright.config.tsapps/ui/tests/utils/api/client.tsapps/ui/tests/context/add-context-image.spec.tsapps/ui/tests/utils/navigation/views.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
resolveModelString()from @automaker/model-resolver to convert model aliases (haiku, sonnet, opus) to full model names
Files:
apps/ui/playwright.config.tsapps/ui/tests/utils/api/client.tsapps/ui/tests/context/add-context-image.spec.tsapps/ui/tests/utils/navigation/views.ts
🧬 Code graph analysis (2)
apps/ui/tests/context/add-context-image.spec.ts (3)
apps/ui/tests/utils/api/client.ts (1)
authenticateForTests(298-302)apps/ui/tests/utils/core/waiting.ts (1)
waitForNetworkIdle(9-11)apps/ui/tests/utils/navigation/views.ts (1)
navigateToContext(22-42)
apps/ui/tests/utils/navigation/views.ts (1)
apps/ui/tests/utils/core/waiting.ts (1)
waitForElement(16-27)
⏰ 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). (1)
- GitHub Check: e2e
🔇 Additional comments (4)
apps/ui/playwright.config.ts (1)
42-43: API key configuration properly integrated with test utilities.The
AUTOMAKER_API_KEYenvironment variable is correctly configured in playwright.config.ts with an appropriate test-only default. TheauthenticateForTestsutility inapps/ui/tests/utils/api/client.tsuses the same fallback pattern, and test files properly call this utility for authentication. Implementation is consistent and well-integrated.apps/ui/tests/utils/navigation/views.ts (1)
40-41: LGTM! Timeout increase is appropriate.The increased timeout (10s → 15s) appropriately accommodates the additional authentication overhead introduced by the API token requirements. The inline comment clearly documents the rationale.
apps/ui/tests/context/add-context-image.spec.ts (1)
141-143: LGTM! Good defensive programming to prevent race conditions.Adding the
toBeAttachedcheck ensures the file input element exists in the DOM before attempting to set files, preventing potential race condition failures.apps/ui/tests/utils/api/client.ts (1)
274-302: No action required—the authentication setup is correct.The backend does not have a hardcoded test API key. Instead, it reads the
AUTOMAKER_API_KEYenvironment variable, which the frontend test configuration (playwright.config.ts) properly sets to the same fallback value ('test-api-key-for-e2e-tests') across all frontend test utilities. The authentication mechanism is environment-based and consistent.
- Added `dev:test` script to package.json for streamlined testing without file watching. - Introduced `kill-test-servers` script to ensure no existing servers are running on test ports before executing tests. - Enhanced Playwright configuration to use mock agent for tests, ensuring consistent API responses and disabling rate limiting. - Updated various test files to include authentication steps and handle login screens, improving reliability and reducing flakiness in tests. - Added `global-setup` for e2e tests to ensure proper initialization before test execution.
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (11)
apps/ui/scripts/kill-test-servers.mjs (1)
37-44: Consider explicit exit code for test script clarity.The current error handling with
.catch(console.error)doesn't exit with a non-zero code on failure. While acceptable for a cleanup script (where it's reasonable to proceed even if cleanup fails), explicitly handling success/failure could improve clarity in the test workflow.🔎 Optional: Add explicit exit handling
-main().catch(console.error); +main() + .then(() => process.exit(0)) + .catch((error) => { + console.error(error); + process.exit(1); + });Note: The current implementation is acceptable since
killProcessOnPortcatches all errors internally, somain()won't reject. This suggestion is purely for explicitness.apps/ui/tests/global-setup.ts (1)
6-10: Consider removing or implementing actual setup logic.The
globalSetupfunction is currently a no-op that only logs a message. If no setup is required, this file and the global setup configuration can be removed to reduce unnecessary code. If setup will be added later, consider adding a TODO comment explaining what's planned.Suggested approach
If no setup is needed:
-/** - * Global setup for all e2e tests - * This runs once before all tests start - */ - -async function globalSetup() { - // Note: Server killing is handled by the pretest script in package.json - // GlobalSetup runs AFTER webServer starts, so we can't kill the server here - console.log('[GlobalSetup] Setup complete'); -} - -export default globalSetup;Or if setup is planned, add a TODO:
async function globalSetup() { + // TODO: Add any global test setup here (e.g., database seeding, global state initialization) // Note: Server killing is handled by the pretest script in package.json // GlobalSetup runs AFTER webServer starts, so we can't kill the server here console.log('[GlobalSetup] Setup complete'); }apps/ui/tests/utils/core/interactions.ts (3)
51-59: Consider using static import and adding error handling.The dynamic import of
authenticateForTestsis unnecessary here since it's a test utility that's always available. A static import would be clearer and more maintainable. Additionally, there's no error handling if authentication fails.Proposed improvement
Move to static import at the top of the file:
import { Page, expect } from '@playwright/test'; +import { authenticateForTests } from '../api/client'; import { getByTestId, getButtonByText } from './elements';Then simplify the function:
export async function gotoWithAuth(page: Page, url: string): Promise<void> { - const { authenticateForTests } = await import('../api/client'); await authenticateForTests(page); await page.goto(url); }
92-93: Avoid hardcoded timeout for UI state synchronization.The hardcoded 100ms timeout is fragile and may not be sufficient on slower systems. Consider waiting for the button's enabled state instead, which is already done on line 99.
Proposed fix
await loginInput.fill(apiKey); - // Wait a moment for the button to become enabled - await page.waitForTimeout(100); - // Wait for button to be enabled (it's disabled when input is empty) const loginButton = page .locator('[data-testid="login-submit-button"], button:has-text("Login")') .first(); await expect(loginButton).toBeEnabled({ timeout: 5000 });The
toBeEnabledassertion already waits up to 5 seconds, making the arbitrary 100ms delay unnecessary.
88-90: Consolidate API key retrieval logic.The API key retrieval logic on line 89 duplicates the same logic in
authenticateForTests(apps/ui/tests/utils/api/client.ts:358). Consider extracting this to a shared constant or function.Example consolidation
In a shared location (e.g.,
apps/ui/tests/utils/constants.ts):export function getTestApiKey(): string { return process.env.AUTOMAKER_API_KEY || 'test-api-key-for-e2e-tests'; }Then use it in both places:
+import { getTestApiKey } from '../constants'; + export async function handleLoginScreenIfPresent(page: Page): Promise<boolean> { // ... selector setup ... if (loginVisible) { - const apiKey = process.env.AUTOMAKER_API_KEY || 'test-api-key-for-e2e-tests'; + const apiKey = getTestApiKey(); await loginInput.fill(apiKey);apps/ui/tests/profiles/profiles-crud.spec.ts (1)
24-28: Consider consolidating authentication approach.Similar to the worktree test, this file uses both
authenticateForTestsandhandleLoginScreenIfPresent. If this dual approach is intentional (e.g., to handle edge cases where programmatic auth doesn't fully set up the session), consider:
- Documenting why both are needed
- Extracting this pattern into a helper like
authenticateAndNavigate(page, url)to reduce duplicationThis would centralize the logic and make the intent clearer.
Example helper function
In
apps/ui/tests/utils/core/interactions.ts:/** * Authenticate and navigate with login screen handling * Use this when tests need robust authentication that handles both programmatic and UI login */ export async function authenticateAndNavigateRobust(page: Page, url: string): Promise<void> { await authenticateForTests(page); await page.goto(url); await page.waitForLoadState('load'); await handleLoginScreenIfPresent(page); }Then use it:
- await authenticateForTests(page); - await page.goto('/'); - await page.waitForLoadState('load'); - await handleLoginScreenIfPresent(page); + await authenticateAndNavigateRobust(page, '/');apps/ui/tests/features/add-feature-to-backlog.spec.ts (2)
66-71: Authentication flow is correct, but consider extracting the pattern.This test correctly implements the dual authentication approach (programmatic + UI fallback) that's also used in the worktree and profiles tests. Since this pattern appears in multiple tests, consider extracting it into a shared helper function to reduce duplication and make the intent explicit.
See the earlier comment on
apps/ui/tests/profiles/profiles-crud.spec.ts(lines 24-28) for an example helper function implementation.
66-66: Remove obvious comment.The comment "// Authenticate before navigating" states what the code clearly shows on the next line. Consider removing it to reduce noise.
- // Authenticate before navigating await authenticateForTests(page);apps/ui/tests/context/add-context-image.spec.ts (2)
17-17: Refactor: Use importedauthenticateForTestsutility.The
authenticateForTestsfunction is imported but never used. This test should follow the same pattern as other test files by callingauthenticateForTests(page)before navigation.🔎 Suggested refactor
await setupProjectWithFixture(page, getFixturePath()); + await authenticateForTests(page);
125-134: Refactor: Replace inline login handling withhandleLoginScreenIfPresent.This inline login handling duplicates logic already available in the
handleLoginScreenIfPresentutility. For consistency with other test files and to reduce duplication, use the shared utility instead.🔎 Suggested refactor
Import the utility at the top of the file:
import { resetContextDirectory, setupProjectWithFixture, getFixturePath, navigateToContext, waitForContextFile, waitForNetworkIdle, authenticateForTests, + handleLoginScreenIfPresent, } from '../utils';Then replace the inline login handling:
await page.goto('/'); await waitForNetworkIdle(page); - - // Check if we're on the login screen and authenticate if needed - const loginInput = page.locator('input[type="password"][placeholder*="API key"]'); - const isLoginScreen = await loginInput.isVisible({ timeout: 2000 }).catch(() => false); - if (isLoginScreen) { - const apiKey = process.env.AUTOMAKER_API_KEY || 'test-api-key-for-e2e-tests'; - await loginInput.fill(apiKey); - await page.locator('button:has-text("Login")').click(); - await page.waitForURL('**/', { timeout: 5000 }); - await waitForNetworkIdle(page); - } + await handleLoginScreenIfPresent(page);apps/server/src/routes/auth/index.ts (1)
219-244: Logout implementation is correct and secure.The endpoint properly invalidates the session and clears the cookie with appropriate security options.
💡 Optional: Use getSessionCookieOptions() for consistency
For consistency with the login endpoint, consider using the helper function:
// Clear the cookie - res.clearCookie(cookieName, { - httpOnly: true, - secure: process.env.NODE_ENV === 'production', - sameSite: 'strict', - path: '/', - }); + const cookieOptions = getSessionCookieOptions(); + res.clearCookie(cookieName, cookieOptions);Note:
clearCookieignoresmaxAge, so this works fine.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
apps/server/package.jsonapps/server/src/routes/auth/index.tsapps/ui/package.jsonapps/ui/playwright.config.tsapps/ui/scripts/kill-test-servers.mjsapps/ui/src/components/views/login-view.tsxapps/ui/tests/agent/start-new-chat-session.spec.tsapps/ui/tests/context/add-context-image.spec.tsapps/ui/tests/context/context-file-management.spec.tsapps/ui/tests/context/delete-context-file.spec.tsapps/ui/tests/features/add-feature-to-backlog.spec.tsapps/ui/tests/features/edit-feature.spec.tsapps/ui/tests/features/feature-manual-review-flow.spec.tsapps/ui/tests/features/feature-skip-tests-toggle.spec.tsapps/ui/tests/git/worktree-integration.spec.tsapps/ui/tests/global-setup.tsapps/ui/tests/profiles/profiles-crud.spec.tsapps/ui/tests/projects/new-project-creation.spec.tsapps/ui/tests/projects/open-existing-project.spec.tsapps/ui/tests/utils/api/client.tsapps/ui/tests/utils/core/interactions.tsapps/ui/tests/utils/navigation/views.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always import from shared packages (@automaker/*), never from old relative paths
Files:
apps/ui/tests/features/feature-manual-review-flow.spec.tsapps/ui/tests/global-setup.tsapps/ui/tests/agent/start-new-chat-session.spec.tsapps/ui/tests/utils/api/client.tsapps/ui/tests/features/add-feature-to-backlog.spec.tsapps/ui/tests/profiles/profiles-crud.spec.tsapps/ui/tests/utils/core/interactions.tsapps/ui/tests/utils/navigation/views.tsapps/ui/tests/projects/new-project-creation.spec.tsapps/server/src/routes/auth/index.tsapps/ui/tests/features/feature-skip-tests-toggle.spec.tsapps/ui/tests/context/delete-context-file.spec.tsapps/ui/tests/projects/open-existing-project.spec.tsapps/ui/tests/git/worktree-integration.spec.tsapps/ui/src/components/views/login-view.tsxapps/ui/playwright.config.tsapps/ui/tests/context/context-file-management.spec.tsapps/ui/tests/features/edit-feature.spec.tsapps/ui/tests/context/add-context-image.spec.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
resolveModelString()from @automaker/model-resolver to convert model aliases (haiku, sonnet, opus) to full model names
Files:
apps/ui/tests/features/feature-manual-review-flow.spec.tsapps/ui/tests/global-setup.tsapps/ui/tests/agent/start-new-chat-session.spec.tsapps/ui/tests/utils/api/client.tsapps/ui/tests/features/add-feature-to-backlog.spec.tsapps/ui/tests/profiles/profiles-crud.spec.tsapps/ui/tests/utils/core/interactions.tsapps/ui/tests/utils/navigation/views.tsapps/ui/tests/projects/new-project-creation.spec.tsapps/server/src/routes/auth/index.tsapps/ui/tests/features/feature-skip-tests-toggle.spec.tsapps/ui/tests/context/delete-context-file.spec.tsapps/ui/tests/projects/open-existing-project.spec.tsapps/ui/tests/git/worktree-integration.spec.tsapps/ui/src/components/views/login-view.tsxapps/ui/playwright.config.tsapps/ui/tests/context/context-file-management.spec.tsapps/ui/tests/features/edit-feature.spec.tsapps/ui/tests/context/add-context-image.spec.ts
apps/server/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
createEventEmitter()fromlib/events.tsfor all server operations to emit events that stream to frontend via WebSocket
Files:
apps/server/src/routes/auth/index.ts
🧠 Learnings (1)
📚 Learning: 2025-12-30T01:02:07.114Z
Learnt from: illia1f
Repo: AutoMaker-Org/automaker PR: 324
File: apps/ui/src/components/views/board-view/components/kanban-card/kanban-card.tsx:122-131
Timestamp: 2025-12-30T01:02:07.114Z
Learning: Tailwind CSS v4 uses postfix syntax for the important modifier: append ! at the end of the utility class (e.g., backdrop-blur-[0px]! or hover:bg-red-500!). The older v3 style used a prefix (!) at the start (e.g., !backdrop-blur-[0px]); prefer the postfix form for consistency across TSX files.
Applied to files:
apps/ui/src/components/views/login-view.tsx
🧬 Code graph analysis (16)
apps/ui/tests/features/feature-manual-review-flow.spec.ts (2)
apps/ui/tests/utils/api/client.ts (1)
authenticateForTests(357-361)apps/ui/tests/utils/core/interactions.ts (1)
handleLoginScreenIfPresent(65-115)
apps/ui/scripts/kill-test-servers.mjs (2)
apps/server/src/services/dev-server-service.ts (1)
killProcessOnPort(55-98)init.mjs (1)
pid(88-88)
apps/ui/tests/agent/start-new-chat-session.spec.ts (1)
apps/ui/tests/utils/api/client.ts (1)
authenticateForTests(357-361)
apps/ui/tests/features/add-feature-to-backlog.spec.ts (2)
apps/ui/tests/utils/api/client.ts (1)
authenticateForTests(357-361)apps/ui/tests/utils/core/interactions.ts (1)
handleLoginScreenIfPresent(65-115)
apps/ui/tests/profiles/profiles-crud.spec.ts (3)
apps/ui/tests/utils/project/setup.ts (1)
setupMockProjectWithProfiles(777-882)apps/ui/tests/utils/api/client.ts (1)
authenticateForTests(357-361)apps/ui/tests/utils/core/interactions.ts (1)
handleLoginScreenIfPresent(65-115)
apps/ui/tests/utils/core/interactions.ts (1)
apps/ui/tests/utils/api/client.ts (1)
authenticateForTests(357-361)
apps/ui/tests/utils/navigation/views.ts (2)
apps/ui/tests/utils/api/client.ts (1)
authenticateForTests(357-361)apps/ui/tests/utils/core/waiting.ts (1)
waitForElement(16-27)
apps/server/src/routes/auth/index.ts (1)
apps/server/src/lib/auth.ts (7)
isRequestAuthenticated(391-398)validateApiKey(233-247)createSession(158-167)getSessionCookieOptions(252-266)getSessionCookieName(271-273)createWsConnectionToken(199-207)invalidateSession(190-193)
apps/ui/tests/features/feature-skip-tests-toggle.spec.ts (2)
apps/ui/tests/utils/api/client.ts (1)
authenticateForTests(357-361)apps/ui/tests/utils/core/interactions.ts (1)
handleLoginScreenIfPresent(65-115)
apps/ui/tests/context/delete-context-file.spec.ts (1)
apps/ui/tests/utils/api/client.ts (1)
authenticateForTests(357-361)
apps/ui/tests/projects/open-existing-project.spec.ts (2)
apps/ui/tests/utils/api/client.ts (1)
authenticateForTests(357-361)apps/ui/tests/utils/core/interactions.ts (1)
handleLoginScreenIfPresent(65-115)
apps/ui/tests/git/worktree-integration.spec.ts (2)
apps/ui/tests/utils/api/client.ts (1)
authenticateForTests(357-361)apps/ui/tests/utils/core/interactions.ts (1)
handleLoginScreenIfPresent(65-115)
apps/ui/src/components/views/login-view.tsx (1)
apps/ui/src/lib/http-api-client.ts (1)
login(130-164)
apps/ui/playwright.config.ts (1)
init.mjs (1)
require(17-17)
apps/ui/tests/context/context-file-management.spec.ts (1)
apps/ui/tests/utils/api/client.ts (1)
authenticateForTests(357-361)
apps/ui/tests/features/edit-feature.spec.ts (2)
apps/ui/tests/utils/api/client.ts (1)
authenticateForTests(357-361)apps/ui/tests/utils/core/interactions.ts (1)
handleLoginScreenIfPresent(65-115)
⏰ 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). (1)
- GitHub Check: e2e
🔇 Additional comments (27)
apps/ui/src/components/views/login-view.tsx (1)
15-110: Excellent implementation of the login component!The component demonstrates solid practices:
- Comprehensive error handling (server errors, network failures, loading states)
- Good UX with clear messaging, loading indicators, and disabled states during submission
- Proper accessibility with labels, autoFocus, and test IDs
- Security-conscious use of
type="password"for the API key input- Clean integration with the
loginfunction from http-api-client and navigation on successThe help text section is particularly helpful for guiding users to locate the API key.
apps/ui/package.json (1)
31-31: LGTM!Adding the kill-test-servers step before fixture setup ensures a clean test environment and prevents port conflicts from stale processes.
apps/ui/tests/agent/start-new-chat-session.spec.ts (1)
65-67: Authentication flow looks correct.The authentication step before navigation follows the intended pattern for the new security requirements. The sequence of authenticate → navigate → wait is appropriate.
apps/ui/tests/context/context-file-management.spec.ts (1)
35-37: Authentication integration is correct.The authentication call is properly placed after project setup and before navigation, maintaining the consistent pattern across the test suite.
apps/ui/tests/context/delete-context-file.spec.ts (1)
37-39: Authentication implementation is consistent.The authentication flow matches the pattern used across other test files in the suite.
apps/ui/tests/git/worktree-integration.spec.ts (1)
52-56: No action needed. This follows the standard pattern established throughout the test suite where bothauthenticateForTestsandhandleLoginScreenIfPresentare used together across all test files (new-project-creation, open-existing-project, profiles-crud, feature tests, etc.). This is an intentional, defensive approach, not redundancy.apps/ui/tests/projects/open-existing-project.spec.ts (1)
14-20: LGTM! Authentication flow properly integrated.The test correctly authenticates before navigation and handles potential login screens after page load, following the consistent pattern established across the test suite.
Also applies to: 83-86
apps/ui/tests/projects/new-project-creation.spec.ts (1)
10-16: LGTM! Consistent authentication pattern.The authentication flow is properly implemented and matches the pattern used across other test files.
Also applies to: 35-38
apps/ui/tests/features/edit-feature.spec.ts (1)
19-20: LGTM! Proper authentication sequence.The authentication and login handling are correctly integrated into the test flow.
Also applies to: 68-72
apps/ui/tests/features/feature-manual-review-flow.spec.ts (1)
22-23: LGTM! Authentication properly integrated.The test follows the established authentication pattern correctly.
Also applies to: 88-92
apps/ui/tests/features/feature-skip-tests-toggle.spec.ts (1)
22-23: LGTM! Authentication flow integrated correctly.The changes follow the standard pattern used across the test suite.
Also applies to: 70-74
apps/ui/tests/context/add-context-image.spec.ts (1)
138-140: Good practice: Explicit file input attachment check.Adding an explicit wait for the file input to be attached to the DOM before interacting with it is a good defensive pattern that prevents flakiness.
apps/ui/tests/utils/api/client.ts (1)
357-361: LGTM! Clean authentication wrapper.The
authenticateForTestsfunction provides a clean interface for tests to authenticate using environment configuration with a sensible fallback.apps/ui/tests/utils/navigation/views.ts (1)
11-12: LGTM! Pre-navigation authentication properly added.All navigation functions now correctly authenticate before navigating, ensuring tests have proper access to protected endpoints.
Also applies to: 44-45, 90-91, 123-124, 156-157, 184-185
apps/server/src/routes/auth/index.ts (5)
1-26: Clear documentation and appropriate imports.The file header provides excellent context about the security model and the authentication flow for both web and Electron modes.
64-89: Rate limit checking logic is correct.The function properly handles window expiration, calculates retry-after time, and cleans up expired entries.
91-105: Login attempt tracking logic is sound.The function correctly handles both new windows and incremental counting within the current window.
112-128: Status endpoint implementation is straightforward and correct.The endpoint properly delegates to
isRequestAuthenticatedand returns the expected payload.
192-217: WebSocket token endpoint correctly generates a separate short-lived token.The implementation properly creates a new, short-lived token (via
createWsConnectionToken) rather than exposing the session cookie value. This addresses the security concern from the previous review.apps/ui/playwright.config.ts (5)
6-7: LGTM! Hardcoding mock agent for tests is appropriate.This ensures consistent test behavior and prevents real API calls during testing, which aligns with the PR's security and testing goals.
13-14: Sequential execution is appropriate for auth-based tests.Setting
workers: 1prevents race conditions with the shared authenticated server state. Theretries: 0setting provides faster feedback but may require tests to be more robust against transient issues.
37-50: Well-configured test server environment.The changes ensure test isolation and proper authentication:
dev:testavoids file watching that could cause port conflictsreuseExistingServer: falseensures the test API key is used- Test API key with a clear default value supports reproducible test runs
- Hiding the API key banner reduces log noise
63-64: Appropriate to skip Electron in test environment.Forcing
VITE_SKIP_ELECTRON: 'true'prevents duplicate server spawning and simplifies the test environment since Electron functionality isn't needed for Playwright web tests.
22-23: This usage is correct. Usingrequire.resolve()in Playwright TypeScript configs is the standard and documented approach for resolving theglobalSetuppath. The file's TypeScript configuration enables CommonJS interoperability (esModuleInterop: true,allowSyntheticDefaultImports: true), makingrequireavailable in this context.apps/server/package.json (3)
12-12: Good addition of test-specific script.The
dev:testscript appropriately removes file watching (tsxwithoutwatch) to prevent server restarts during test execution, which could cause port conflicts or test flakiness.
42-43: Good practice to include TypeScript type definitions.Adding
@types/cookieand@types/cookie-parserprovides proper type safety for the new cookie handling functionality.
33-33: No security vulnerabilities found incookie-parser@1.4.7. Version 1.4.7 has no recorded CVEs and passes security database checks (Snyk). Dependency is safe to use.
| // Clean up old rate limit entries periodically (every 5 minutes) | ||
| setInterval( | ||
| () => { | ||
| const now = Date.now(); | ||
| loginAttempts.forEach((data, ip) => { | ||
| if (now - data.windowStart > RATE_LIMIT_WINDOW_MS * 2) { | ||
| loginAttempts.delete(ip); | ||
| } | ||
| }); | ||
| }, | ||
| 5 * 60 * 1000 | ||
| ); |
There was a problem hiding this comment.
The top-level setInterval is never cleared, causing a potential memory leak.
If this module is ever reloaded (e.g., during hot module replacement in development), the interval will continue running. This creates a memory leak as old intervals accumulate.
Consider one of these approaches:
- Store the interval ID and export a cleanup function
- Move the interval setup into a server lifecycle hook that can be properly torn down
- Use a WeakMap or other automatic cleanup mechanism
🔎 Recommended fix: store and export cleanup function
+let cleanupInterval: NodeJS.Timeout | null = null;
+
// Clean up old rate limit entries periodically (every 5 minutes)
-setInterval(
+cleanupInterval = setInterval(
() => {
const now = Date.now();
loginAttempts.forEach((data, ip) => {
if (now - data.windowStart > RATE_LIMIT_WINDOW_MS * 2) {
loginAttempts.delete(ip);
}
});
},
5 * 60 * 1000
);
+
+export function cleanup(): void {
+ if (cleanupInterval) {
+ clearInterval(cleanupInterval);
+ cleanupInterval = null;
+ }
+ loginAttempts.clear();
+}📝 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.
| // Clean up old rate limit entries periodically (every 5 minutes) | |
| setInterval( | |
| () => { | |
| const now = Date.now(); | |
| loginAttempts.forEach((data, ip) => { | |
| if (now - data.windowStart > RATE_LIMIT_WINDOW_MS * 2) { | |
| loginAttempts.delete(ip); | |
| } | |
| }); | |
| }, | |
| 5 * 60 * 1000 | |
| ); | |
| let cleanupInterval: NodeJS.Timeout | null = null; | |
| // Clean up old rate limit entries periodically (every 5 minutes) | |
| cleanupInterval = setInterval( | |
| () => { | |
| const now = Date.now(); | |
| loginAttempts.forEach((data, ip) => { | |
| if (now - data.windowStart > RATE_LIMIT_WINDOW_MS * 2) { | |
| loginAttempts.delete(ip); | |
| } | |
| }); | |
| }, | |
| 5 * 60 * 1000 | |
| ); | |
| export function cleanup(): void { | |
| if (cleanupInterval) { | |
| clearInterval(cleanupInterval); | |
| cleanupInterval = null; | |
| } | |
| loginAttempts.clear(); | |
| } |
🤖 Prompt for AI Agents
In apps/server/src/routes/auth/index.ts around lines 37 to 48, the top-level
setInterval used to clean up old rate-limit entries is never cleared which can
leak timers if the module is reloaded; capture the returned interval ID, export
(or otherwise expose) a cleanup function that calls clearInterval on that ID and
removes any other in-memory state as needed, and update any server lifecycle or
HMR code to call that cleanup during teardown; alternatively move the interval
registration into a lifecycle hook so it can be torn down cleanly.
| /** | ||
| * Get client IP address from request | ||
| * Handles X-Forwarded-For header for reverse proxy setups | ||
| */ | ||
| function getClientIp(req: Request): string { | ||
| const forwarded = req.headers['x-forwarded-for']; | ||
| if (forwarded) { | ||
| // X-Forwarded-For can be a comma-separated list; take the first (original client) | ||
| const forwardedIp = Array.isArray(forwarded) ? forwarded[0] : forwarded.split(',')[0]; | ||
| return forwardedIp.trim(); | ||
| } | ||
| return req.ip || req.socket.remoteAddress || 'unknown'; | ||
| } |
There was a problem hiding this comment.
X-Forwarded-For can be spoofed if not behind a trusted proxy.
The current implementation unconditionally trusts the X-Forwarded-For header, which can be set by any client. If the server is directly exposed to the internet (not behind a trusted reverse proxy), attackers can bypass rate limiting by spoofing this header with different IP addresses.
Consider:
- Only trust
X-Forwarded-Forwhen the request comes from a known proxy IP - Use Express's
trust proxysetting and rely onreq.ipinstead (which respects the trust proxy configuration) - Document the requirement that the server must be deployed behind a trusted reverse proxy
🔎 Recommended fix: use Express trust proxy and req.ip
In the main server setup (apps/server/src/index.ts), configure trust proxy:
// Trust proxy if behind reverse proxy (nginx, cloudflare, etc.)
app.set('trust proxy', true); // Or specify trusted proxy IPsThen simplify this function:
function getClientIp(req: Request): string {
- const forwarded = req.headers['x-forwarded-for'];
- if (forwarded) {
- // X-Forwarded-For can be a comma-separated list; take the first (original client)
- const forwardedIp = Array.isArray(forwarded) ? forwarded[0] : forwarded.split(',')[0];
- return forwardedIp.trim();
- }
- return req.ip || req.socket.remoteAddress || 'unknown';
+ // req.ip respects Express's trust proxy setting
+ return req.ip || req.socket.remoteAddress || 'unknown';
}📝 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 client IP address from request | |
| * Handles X-Forwarded-For header for reverse proxy setups | |
| */ | |
| function getClientIp(req: Request): string { | |
| const forwarded = req.headers['x-forwarded-for']; | |
| if (forwarded) { | |
| // X-Forwarded-For can be a comma-separated list; take the first (original client) | |
| const forwardedIp = Array.isArray(forwarded) ? forwarded[0] : forwarded.split(',')[0]; | |
| return forwardedIp.trim(); | |
| } | |
| return req.ip || req.socket.remoteAddress || 'unknown'; | |
| } | |
| /** | |
| * Get client IP address from request | |
| * Handles X-Forwarded-For header for reverse proxy setups | |
| */ | |
| function getClientIp(req: Request): string { | |
| // req.ip respects Express's trust proxy setting | |
| return req.ip || req.socket.remoteAddress || 'unknown'; | |
| } |
🤖 Prompt for AI Agents
In apps/server/src/routes/auth/index.ts around lines 50 to 62, the getClientIp
function unconditionally trusts the X-Forwarded-For header which can be spoofed;
update the app to use Express's trust proxy and change this function to rely on
req.ip (which respects trust proxy) or only parse X-Forwarded-For after
verifying the request came from a trusted proxy IP; specifically set
app.set('trust proxy', true) or a list of trusted proxy IPs in
apps/server/src/index.ts, replace the manual forwarded-header parsing with
returning req.ip || req.socket.remoteAddress || 'unknown', and add a short
comment documenting the requirement that the server must run behind a trusted
reverse proxy when relying on X-Forwarded-For.
| // Create session and set cookie | ||
| const sessionToken = await createSession(); | ||
| const cookieOptions = getSessionCookieOptions(); | ||
| const cookieName = getSessionCookieName(); | ||
|
|
||
| res.cookie(cookieName, sessionToken, cookieOptions); | ||
| res.json({ | ||
| success: true, | ||
| message: 'Logged in successfully.', | ||
| // Return token for explicit header-based auth (works around cross-origin cookie issues) | ||
| token: sessionToken, | ||
| }); |
There was a problem hiding this comment.
Returning the session token in the response body defeats HTTP-only cookie security.
Line 188 returns the session token in the response body with a comment about "cross-origin cookie issues." This completely undermines the HTTP-only cookie protection described in the file header (lines 8-10). An XSS attack can simply read this response and exfiltrate the session token, even though it cannot read the HTTP-only cookie directly.
The session token is meant to be opaque to the client when using cookies. If cross-origin scenarios require header-based authentication, the proper solution is:
- Use a separate authentication mechanism for cross-origin scenarios (e.g., a different login endpoint that returns a token meant for headers)
- Or accept that cookies don't work cross-origin and require users to use the API key directly in those scenarios
Returning both cookie and token from the same endpoint is the worst of both worlds - it has the complexity of cookies but none of the XSS protection.
🔎 Recommended fix: remove token from response
res.cookie(cookieName, sessionToken, cookieOptions);
res.json({
success: true,
message: 'Logged in successfully.',
- // Return token for explicit header-based auth (works around cross-origin cookie issues)
- token: sessionToken,
});If cross-origin scenarios truly need header-based auth, create a separate endpoint or login mode rather than compromising the security of cookie-based auth.
📝 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.
| // Create session and set cookie | |
| const sessionToken = await createSession(); | |
| const cookieOptions = getSessionCookieOptions(); | |
| const cookieName = getSessionCookieName(); | |
| res.cookie(cookieName, sessionToken, cookieOptions); | |
| res.json({ | |
| success: true, | |
| message: 'Logged in successfully.', | |
| // Return token for explicit header-based auth (works around cross-origin cookie issues) | |
| token: sessionToken, | |
| }); | |
| // Create session and set cookie | |
| const sessionToken = await createSession(); | |
| const cookieOptions = getSessionCookieOptions(); | |
| const cookieName = getSessionCookieName(); | |
| res.cookie(cookieName, sessionToken, cookieOptions); | |
| res.json({ | |
| success: true, | |
| message: 'Logged in successfully.', | |
| }); |
🤖 Prompt for AI Agents
In apps/server/src/routes/auth/index.ts around lines 178 to 189, the handler
currently returns the session token in the JSON body which defeats HTTP-only
cookie protection; remove the token from the response body (do not include
token: sessionToken), only set the cookie and return a success message/status,
ensure you do not log or expose the token anywhere, and if header-based auth for
cross-origin use is required implement a separate login endpoint or explicit
"token" mode rather than returning the cookie token here.
| async function killProcessOnPort(port) { | ||
| try { | ||
| const { stdout } = await execAsync(`lsof -ti:${port}`); | ||
| const pids = stdout.trim().split('\n').filter(Boolean); | ||
|
|
||
| if (pids.length > 0) { | ||
| console.log(`[KillTestServers] Found process(es) on port ${port}: ${pids.join(', ')}`); | ||
| for (const pid of pids) { | ||
| try { | ||
| await execAsync(`kill -9 ${pid}`); | ||
| console.log(`[KillTestServers] Killed process ${pid}`); | ||
| } catch (error) { | ||
| // Process might have already exited | ||
| } | ||
| } | ||
| // Wait a moment for the port to be released | ||
| await new Promise((resolve) => setTimeout(resolve, 500)); | ||
| } | ||
| } catch (error) { | ||
| // No process on port, which is fine | ||
| } | ||
| } |
There was a problem hiding this comment.
Add Windows support for cross-platform compatibility.
The script uses Unix-only commands (lsof, kill) and will fail on Windows. Developers on Windows won't be able to run the test suite.
🔎 Proposed cross-platform implementation
Based on the pattern in apps/server/src/services/dev-server-service.ts (lines 54-97), here's a cross-platform solution:
async function killProcessOnPort(port) {
try {
- const { stdout } = await execAsync(`lsof -ti:${port}`);
- const pids = stdout.trim().split('\n').filter(Boolean);
-
- if (pids.length > 0) {
- console.log(`[KillTestServers] Found process(es) on port ${port}: ${pids.join(', ')}`);
- for (const pid of pids) {
- try {
- await execAsync(`kill -9 ${pid}`);
- console.log(`[KillTestServers] Killed process ${pid}`);
- } catch (error) {
- // Process might have already exited
+ if (process.platform === 'win32') {
+ // Windows: find and kill process on port
+ const { stdout } = await execAsync(`netstat -ano | findstr :${port}`);
+ const lines = stdout.trim().split('\n');
+ const pids = new Set();
+ for (const line of lines) {
+ const parts = line.trim().split(/\s+/);
+ const pid = parts[parts.length - 1];
+ if (pid && pid !== '0') {
+ pids.add(pid);
+ }
+ }
+ if (pids.size > 0) {
+ console.log(`[KillTestServers] Found process(es) on port ${port}: ${[...pids].join(', ')}`);
+ for (const pid of pids) {
+ try {
+ await execAsync(`taskkill /F /PID ${pid}`);
+ console.log(`[KillTestServers] Killed process ${pid}`);
+ } catch (error) {
+ // Process might have already exited
+ }
}
+ await new Promise((resolve) => setTimeout(resolve, 500));
+ }
+ } else {
+ // macOS/Linux: use lsof
+ const { stdout } = await execAsync(`lsof -ti:${port}`);
+ const pids = stdout.trim().split('\n').filter(Boolean);
+
+ if (pids.length > 0) {
+ console.log(`[KillTestServers] Found process(es) on port ${port}: ${pids.join(', ')}`);
+ for (const pid of pids) {
+ try {
+ await execAsync(`kill -9 ${pid}`);
+ console.log(`[KillTestServers] Killed process ${pid}`);
+ } catch (error) {
+ // Process might have already exited
+ }
+ }
+ await new Promise((resolve) => setTimeout(resolve, 500));
}
- // Wait a moment for the port to be released
- await new Promise((resolve) => setTimeout(resolve, 500));
}
} catch (error) {
// No process on port, which is fine
}
}📝 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.
| async function killProcessOnPort(port) { | |
| try { | |
| const { stdout } = await execAsync(`lsof -ti:${port}`); | |
| const pids = stdout.trim().split('\n').filter(Boolean); | |
| if (pids.length > 0) { | |
| console.log(`[KillTestServers] Found process(es) on port ${port}: ${pids.join(', ')}`); | |
| for (const pid of pids) { | |
| try { | |
| await execAsync(`kill -9 ${pid}`); | |
| console.log(`[KillTestServers] Killed process ${pid}`); | |
| } catch (error) { | |
| // Process might have already exited | |
| } | |
| } | |
| // Wait a moment for the port to be released | |
| await new Promise((resolve) => setTimeout(resolve, 500)); | |
| } | |
| } catch (error) { | |
| // No process on port, which is fine | |
| } | |
| } | |
| async function killProcessOnPort(port) { | |
| try { | |
| if (process.platform === 'win32') { | |
| // Windows: find and kill process on port | |
| const { stdout } = await execAsync(`netstat -ano | findstr :${port}`); | |
| const lines = stdout.trim().split('\n'); | |
| const pids = new Set(); | |
| for (const line of lines) { | |
| const parts = line.trim().split(/\s+/); | |
| const pid = parts[parts.length - 1]; | |
| if (pid && pid !== '0') { | |
| pids.add(pid); | |
| } | |
| } | |
| if (pids.size > 0) { | |
| console.log(`[KillTestServers] Found process(es) on port ${port}: ${[...pids].join(', ')}`); | |
| for (const pid of pids) { | |
| try { | |
| await execAsync(`taskkill /F /PID ${pid}`); | |
| console.log(`[KillTestServers] Killed process ${pid}`); | |
| } catch (error) { | |
| // Process might have already exited | |
| } | |
| } | |
| await new Promise((resolve) => setTimeout(resolve, 500)); | |
| } | |
| } else { | |
| // macOS/Linux: use lsof | |
| const { stdout } = await execAsync(`lsof -ti:${port}`); | |
| const pids = stdout.trim().split('\n').filter(Boolean); | |
| if (pids.length > 0) { | |
| console.log(`[KillTestServers] Found process(es) on port ${port}: ${pids.join(', ')}`); | |
| for (const pid of pids) { | |
| try { | |
| await execAsync(`kill -9 ${pid}`); | |
| console.log(`[KillTestServers] Killed process ${pid}`); | |
| } catch (error) { | |
| // Process might have already exited | |
| } | |
| } | |
| await new Promise((resolve) => setTimeout(resolve, 500)); | |
| } | |
| } | |
| } catch (error) { | |
| // No process on port, which is fine | |
| } | |
| } |
🤖 Prompt for AI Agents
In apps/ui/scripts/kill-test-servers.mjs around lines 14 to 35, the current
implementation uses Unix-only commands (lsof, kill) and will fail on Windows;
replace it with a cross-platform implementation: detect process.platform, for
POSIX keep lsof/kilL flow, for Windows use a netstat/FindStr or PowerShell
command to find the PID(s) listening on the port (or use `netstat -ano | findstr
:PORT`), parse the PID list, then kill using `taskkill /PID <pid> /F` (or
Stop-Process in PowerShell) and handle errors similarly; ensure you trim and
filter PIDs, log found PIDs, attempt to terminate each PID with appropriate
platform-specific command, catch and ignore already-exited processes, and keep
the short post-kill delay to allow the port to be released.
| export async function authenticateWithApiKey(page: Page, apiKey: string): Promise<boolean> { | ||
| try { | ||
| // Ensure we're on a page (needed for cookies to work) | ||
| const currentUrl = page.url(); | ||
| if (!currentUrl || currentUrl === 'about:blank') { | ||
| await page.goto('http://localhost:3007', { waitUntil: 'domcontentloaded' }); | ||
| } | ||
|
|
||
| // Use browser context fetch to ensure cookies are set in the browser | ||
| const response = await page.evaluate( | ||
| async ({ url, apiKey }) => { | ||
| const res = await fetch(url, { | ||
| method: 'POST', | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| credentials: 'include', | ||
| body: JSON.stringify({ apiKey }), | ||
| }); | ||
| const data = await res.json(); | ||
| return { success: data.success, token: data.token }; | ||
| }, | ||
| { url: `${API_BASE_URL}/api/auth/login`, apiKey } | ||
| ); | ||
|
|
||
| if (response.success && response.token) { | ||
| // Manually set the cookie in the browser context | ||
| // The server sets a cookie named 'automaker_session' (see SESSION_COOKIE_NAME in auth.ts) | ||
| await page.context().addCookies([ | ||
| { | ||
| name: 'automaker_session', | ||
| value: response.token, | ||
| domain: 'localhost', | ||
| path: '/', | ||
| httpOnly: true, | ||
| sameSite: 'Lax', | ||
| }, | ||
| ]); | ||
|
|
||
| // Verify the session is working by polling auth status | ||
| // This replaces arbitrary timeout with actual condition check | ||
| let attempts = 0; | ||
| const maxAttempts = 10; | ||
| while (attempts < maxAttempts) { | ||
| const statusResponse = await page.evaluate( | ||
| async ({ url }) => { | ||
| const res = await fetch(url, { | ||
| credentials: 'include', | ||
| }); | ||
| return res.json(); | ||
| }, | ||
| { url: `${API_BASE_URL}/api/auth/status` } | ||
| ); | ||
|
|
||
| if (statusResponse.authenticated === true) { | ||
| return true; | ||
| } | ||
| attempts++; | ||
| // Use a very short wait between polling attempts (this is acceptable for polling) | ||
| await page.waitForFunction(() => true, { timeout: 50 }); | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| return false; | ||
| } catch (error) { | ||
| console.error('Authentication error:', error); | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Several improvements needed in authentication implementation.
The authentication flow is well-structured overall, but there are a few issues to address:
- Line 288: Hardcoded URL should use
API_BASE_URL - Line 340: Anti-pattern timeout hack
- Lines 322-341: Missing error logging when polling fails
🔎 Proposed fixes
const currentUrl = page.url();
if (!currentUrl || currentUrl === 'about:blank') {
- await page.goto('http://localhost:3007', { waitUntil: 'domcontentloaded' });
+ await page.goto(API_BASE_URL, { waitUntil: 'domcontentloaded' });
} if (statusResponse.authenticated === true) {
return true;
}
attempts++;
- // Use a very short wait between polling attempts (this is acceptable for polling)
- await page.waitForFunction(() => true, { timeout: 50 });
+ // Short wait between polling attempts
+ await page.waitForTimeout(50);
}
-
+
+ console.error('Authentication polling failed: max attempts reached');
return false;| await Promise.race([ | ||
| page.waitForURL((url) => !url.pathname.includes('/login'), { timeout: 10000 }), | ||
| appContent.first().waitFor({ state: 'visible', timeout: 10000 }), | ||
| ]).catch(() => {}); |
There was a problem hiding this comment.
Handle navigation errors explicitly.
The empty catch block silently suppresses navigation errors, which could hide real issues during test execution. Consider logging or handling specific error cases.
Proposed improvement
// Wait for navigation away from login - either to content or URL change
await Promise.race([
page.waitForURL((url) => !url.pathname.includes('/login'), { timeout: 10000 }),
appContent.first().waitFor({ state: 'visible', timeout: 10000 }),
- ]).catch(() => {});
+ ]).catch((err) => {
+ console.warn('[handleLoginScreenIfPresent] Navigation after login timed out:', err.message);
+ // Continue anyway - the subsequent load state wait will catch critical issues
+ });📝 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.
| await Promise.race([ | |
| page.waitForURL((url) => !url.pathname.includes('/login'), { timeout: 10000 }), | |
| appContent.first().waitFor({ state: 'visible', timeout: 10000 }), | |
| ]).catch(() => {}); | |
| await Promise.race([ | |
| page.waitForURL((url) => !url.pathname.includes('/login'), { timeout: 10000 }), | |
| appContent.first().waitFor({ state: 'visible', timeout: 10000 }), | |
| ]).catch((err) => { | |
| console.warn('[handleLoginScreenIfPresent] Navigation after login timed out:', err.message); | |
| // Continue anyway - the subsequent load state wait will catch critical issues | |
| }); |
🤖 Prompt for AI Agents
In apps/ui/tests/utils/core/interactions.ts around lines 103 to 106, the
Promise.race call ends with an empty catch that silently swallows navigation
errors; replace the empty catch with explicit handling: catch the error, if it's
a known/expected timeout (e.g. error.name === 'TimeoutError' or message includes
'Timeout') log a concise warning including the error details (console.warn or
the test logger) and allow the test to continue, otherwise rethrow the error so
real failures surface; ensure the logged message includes context (which wait
failed) and the original error object.
| // Check if we're on the login screen and handle it | ||
| const loginInput = page | ||
| .locator('[data-testid="login-api-key-input"], input[type="password"][placeholder*="API key"]') | ||
| .first(); | ||
| const isLoginScreen = await loginInput.isVisible({ timeout: 2000 }).catch(() => false); | ||
| if (isLoginScreen) { | ||
| const apiKey = process.env.AUTOMAKER_API_KEY || 'test-api-key-for-e2e-tests'; | ||
| await loginInput.fill(apiKey); | ||
| await page.waitForTimeout(100); | ||
| await page | ||
| .locator('[data-testid="login-submit-button"], button:has-text("Login")') | ||
| .first() | ||
| .click(); | ||
| await page.waitForURL('**/board', { timeout: 5000 }); | ||
| await page.waitForLoadState('load'); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Refactor: Eliminate duplicated login handling across navigation functions.
The login screen handling logic is duplicated across navigateToBoard, navigateToContext, navigateToAgent, and navigateToWelcome. This same logic already exists in the handleLoginScreenIfPresent utility function (in core/interactions.ts). This duplication creates a maintenance burden where login logic changes must be replicated in multiple places.
🔎 Recommended refactor
Import the utility at the top:
import { Page } from '@playwright/test';
-import { clickElement } from '../core/interactions';
+import { clickElement, handleLoginScreenIfPresent } from '../core/interactions';
import { waitForElement } from '../core/waiting';
import { authenticateForTests } from '../api/client';Then replace all inline login handling blocks with a single call. For example, in navigateToBoard:
await page.goto('/board');
await page.waitForLoadState('load');
-
- // Check if we're on the login screen and handle it
- const loginInput = page
- .locator('[data-testid="login-api-key-input"], input[type="password"][placeholder*="API key"]')
- .first();
- const isLoginScreen = await loginInput.isVisible({ timeout: 2000 }).catch(() => false);
- if (isLoginScreen) {
- const apiKey = process.env.AUTOMAKER_API_KEY || 'test-api-key-for-e2e-tests';
- await loginInput.fill(apiKey);
- await page.waitForTimeout(100);
- await page
- .locator('[data-testid="login-submit-button"], button:has-text("Login")')
- .first()
- .click();
- await page.waitForURL('**/board', { timeout: 5000 });
- await page.waitForLoadState('load');
- }
+ await handleLoginScreenIfPresent(page);Apply the same pattern to navigateToContext (lines 51-66), navigateToAgent (lines 130-145), and navigateToWelcome (lines 190-207).
Also applies to: 51-66, 130-145, 190-207
🤖 Prompt for AI Agents
In apps/ui/tests/utils/navigation/views.ts around lines 18-33 (and also replace
duplicated blocks at 51-66, 130-145, 190-207), the login-screen handling block
is duplicated; import the existing handleLoginScreenIfPresent utility from
core/interactions at the top of the file and replace each inline
login-detection-and-login sequence with a single await
handleLoginScreenIfPresent(page) call (keeping the same async/await flow and
timeouts where needed) so all navigation functions reuse the centralized login
logic.
|
@CodeRabbit re review this pr |
|
@webdevcody I'll perform a full re-review of all the changes in this PR for you. ✅ Actions performedFull review triggered. |
API requests were being made before initApiKey() completed, causing 401 Unauthorized errors on app startup in Electron mode. Changes: - Add waitForApiKeyInit() to track and await API key initialization - Make HTTP methods (get/post/put/delete) wait for auth before requests - Defer WebSocket connection until API key is ready - Add explicit auth wait in useSettingsMigration hook Fixes race condition introduced in PR AutoMaker-Org#321
API requests were being made before initApiKey() completed, causing 401 Unauthorized errors on app startup in Electron mode. Changes: - Add waitForApiKeyInit() to track and await API key initialization - Make HTTP methods (get/post/put/delete) wait for auth before requests - Defer WebSocket connection until API key is ready - Add explicit auth wait in useSettingsMigration hook Fixes race condition introduced in PR AutoMaker-Org#321
…cess, no by passing
Summary by CodeRabbit
New Features
Security
Platform
Tests
Other
✏️ Tip: You can customize this high-level summary in your review settings.