feat: Add Orchestrator Client Mode for Remote Access via Central Hub#319
Open
liamhelmer wants to merge 43 commits intositeboon:mainfrom
Open
feat: Add Orchestrator Client Mode for Remote Access via Central Hub#319liamhelmer wants to merge 43 commits intositeboon:mainfrom
liamhelmer wants to merge 43 commits intositeboon:mainfrom
Conversation
…or integration Add orchestrator client mode to claudecodeui that enables instances to connect to a central orchestrator server via WebSocket. Implements client registration, GitHub OAuth-based authentication, and real-time communication with the orchestrator backend for centralized instance management and monitoring. The orchestrator client creates a secure WebSocket connection using credentials defined in config, authenticates via GitHub OAuth tokens, and relays user traffic and application state back to the orchestrator for aggregation across multiple claudecodeui instances running on different machines.
…ient mode Add GitHub OAuth authentication and WebSocket proxy infrastructure to support claudecodeui instances connecting to the orchestrator. This enables client mode where instances authenticate via GitHub tokens and relay application data back to the central server for aggregation and management. The implementation provides the foundation for: - GitHub token validation on WebSocket connections - Proxying authenticated user traffic between clients and orchestrator - Secure credential exchange using OAuth flow - Per-client connection management with token-based authentication
…ient mode Implement client-mode support for claudecodeui to connect to a central orchestrator server. This enables multiple claudecodeui instances to register with and communicate through a central orchestrator using WebSocket connections authenticated via GitHub OAuth. The implementation includes: - GitHub OAuth authentication flow for secure client registration - WebSocket client that connects to orchestrator at configured URL - Request/response protocol for proxying user traffic through orchestrator - Middleware to authenticate incoming requests using GitHub tokens - Database layer to manage client sessions and authentication state This allows claudecodeui to operate in a distributed architecture where the orchestrator coordinates traffic and manages authentication across multiple connected instances.
…ient mode Configure claudecodeui to operate in client mode connecting to a central orchestrator via websocket. Client mode enables credential delegation through github oauth authentication, allowing the orchestrator to manage user traffic and forward app data back through the proxy connection. This architecture supports multi-instance orchestration where each claudecodeui client authenticates independently and streams results through a centralized gateway.
…ient mode Enable claudecodeui instances to operate in client mode, connecting to a central orchestrator server via websocket. Implements github oauth authentication for client identification and request proxying through the orchestrator, allowing distributed claude code instances to be managed centrally.
Contributor
WalkthroughAdds an Orchestrator client mode and subsystem: WS-based OrchestratorClient, message protocol, GitHub pass-through auth, status tracking, and HTTP proxying; integrates with CLI, env config, DB (github_id), auth middleware, service worker, and frontend routing/WebSocket flows. (<=50 words) Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Claude Code UI (Client)
participant Orch as Orchestrator Server
participant DB as Server DB
participant GH as GitHub API
UI->>Orch: WebSocket connect (client_id, token)
Orch->>UI: REGISTERED
UI->>Orch: sendStatusUpdate(IDLE/ACTIVE/BUSY)
Orch->>DB: getOrCreateOrchestratorUser(github_id, username)
Orch->>GH: validate GitHub token / membership
Orch->>UI: HTTP_PROXY_REQUEST (proxyBase, token, url)
UI->>UI: rewrite HTML/JS URLs (inject proxyBase & token)
UI->>Orch: HTTP_PROXY_RESPONSE (status, headers, body)
sequenceDiagram
participant Proxy as OrchestratorClient (server-side)
participant App as Proxied Claude Code UI
participant DB as Server DB
participant GH as GitHub API
Proxy->>GH: getGitHubUser / checkOrg/Team
GH-->>Proxy: user info
Proxy->>DB: getOrCreateOrchestratorUser(githubId, username)
DB-->>Proxy: user record
Proxy->>App: fetch proxied URL with generated JWT
App-->>Proxy: response (HTML/JS)
Proxy->>Proxy: rewrite URLs & inject fetch patch/token
Proxy->>Orch: HTTP_PROXY_RESPONSE (status, headers, body)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
- Create docs/Orchestrator-Client.md with comprehensive setup guide - Add orchestrator client mode section to README.md - Reference Duratii orchestrator project at github.com/Epiphytic/duratii Co-Authored-By: Claude Opus 4.5 <[email protected]>
…cket proxy Adds orchestrator client mode to claudecodeui allowing instances to connect to a central orchestrator server. Implements websocket connection with github oauth authentication and request proxying to enable the orchestrator to delegate user traffic through authenticated client instances.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/middleware/auth.js (1)
156-162: Remove unnecessary export ofJWT_SECRET.
JWT_SECRETis not imported or used anywhere outside ofserver/middleware/auth.js. Removing it from the export list (line 161) reduces unnecessary exposure of sensitive credentials without impacting functionality.
🤖 Fix all issues with AI agents
In @.fork-join/session-1768779775.json:
- Around line 1-21: Remove the internal session file
.fork-join/session-1768779775.json from the repo and ensure it won’t be
re-added: add the .fork-join/ pattern to .gitignore, remove the tracked
.fork-join/session-1768779775.json from the commit/index (so it’s no longer in
version control), then create a new commit that only updates .gitignore and
removes the file from the tree; if this was already pushed and must be purged
from history, coordinate a branch rewrite/force-push after consulting the team.
In @.fork-join/tracked_files.txt:
- Around line 1-9: The tracked file .fork-join/tracked_files.txt contains
absolute user-specific paths that leak local machine structure; update each
entry to use repository-relative paths (e.g., replace leading
/Users/.../repos/epiphytic/claudecodeui/ with relative paths like server/... or
src/...) and/or remove entries that are local-only, and then add the .fork-join/
directory to .gitignore so these artifacts are not committed; look for the file
name ".fork-join/tracked_files.txt" and the listed entries (db.js, auth.js,
client.js, github-auth.js, index.js, protocol.js, proxy.js, App.jsx, init.sql)
when making the changes.
In `@server/orchestrator/client.js`:
- Around line 628-639: The catch block that builds and sends the proxy error
response (using the error variable, createHttpProxyResponseMessage, request_id
and this.sendMessage) currently injects error.message into the response body,
risking information leakage; change it to send a generic, non-sensitive error
payload (e.g., "Proxy request failed") to the orchestrator/end-user while
logging the full error details internally via console.error or
processLogger.error, and ensure headers/status (502) and request_id remain
intact so callers can correlate the failure without exposing internal
diagnostics.
- Around line 166-178: The onRegistered listener is left attached if the socket
closes before registration; add a close handler that clears connectTimeout and
removes both onRegistered and onError listeners and rejects the promise to
prevent leaks. Specifically, after registering this.once("registered",
onRegistered) and this.once("error", onError), add a this.once("close", onClose)
(or similar) where onClose calls clearTimeout(connectTimeout),
this.removeListener("registered", onRegistered), this.removeListener("error",
onError), and rejects with an appropriate error; also ensure the onClose handler
removes itself after running so all listeners are cleaned up.
- Around line 732-762: The inline script building authScript is vulnerable
because orchestratorUsername (and to be safe orchestratorToken) are interpolated
directly into the JS string; update the code that constructs authScript so
user-controlled values are safely escaped before embedding (e.g., serialize
values with a safe encoder such as JSON.stringify or otherwise escape quotes and
closing script sequences) and then reference those sanitized variables in the
script; locate the authScript construction (variable authScript) and ensure
orchestratorUsername and orchestratorToken are sanitized/encoded before
interpolation to prevent breaking out of the quoted string or injecting
</script>.
In `@server/orchestrator/github-auth.js`:
- Around line 32-60: githubFetch currently assumes global fetch and
AbortController exist, which will break on Node <18; to fix, either declare Node
>=18 in package.json engines or add a runtime polyfill/fallback: detect
globalThis.fetch and globalThis.AbortController at startup and if missing
dynamically import a polyfill (e.g., undici) and assign globalThis.fetch and
globalThis.AbortController before using githubFetch (or refactor githubFetch to
accept an injected fetch/signal implementation); update package.json or CI docs
accordingly and ensure any new dependency is listed in package.json. Reference
symbols: githubFetch, fetch, AbortController, GITHUB_API_BASE.
In `@server/orchestrator/index.js`:
- Around line 198-208: The catch block logs “running in standalone mode” but the
function still returns { client, statusHooks, requestHandler }, which misleads
callers that an orchestrator is active; change the catch to return null (or
explicitly return null instead of the object) after logging an updated message
(e.g., "[ORCHESTRATOR] Orchestrator unavailable, running in standalone mode") so
callers can detect failure, and update any callers to handle a null return from
this routine rather than assuming a non‑null orchestrator (references: client,
statusHooks, requestHandler).
In `@server/orchestrator/proxy.js`:
- Around line 270-286: The current call to handlers.handleChatMessage proxyizes
the message with JSON.stringify({ type: action, ...payload }) which allows
payload.type to override action; instead ensure the routed action is
authoritative by removing or overriding any payload.type before stringifying.
Locate the call to handlers.handleChatMessage (using proxySocket, writer) and
change the construction so payload's fields are spread first and then type is
set to action (or explicitly delete payload.type prior to building the object)
so that the resulting JSON always has type === action.
- Around line 204-263: The follow-up routing is unreachable because
activeProxySockets is only closed over by createUserRequestHandler while
handleFollowUpMessage(activeProxySockets, requestId, payload) is never invoked;
fix by registering a follow-up listener inside createUserRequestHandler that
forwards follow-up messages to handleFollowUpMessage: after creating
activeProxySockets and/or after storing proxySocket, add an
orchestratorClient.ws?.on('message', ...) (or appropriate event hook) that
parses incoming messages, detects follow-up messages (e.g. message.type ===
'follow_up' or similar) and calls handleFollowUpMessage(activeProxySockets,
message.request_id, message.payload); ensure handleFollowUpMessage is
imported/available and remove the unused exported function only if you
intentionally choose not to support follow-ups.
🧹 Nitpick comments (6)
.fork-join/current_session (1)
1-1: Consider adding.fork-join/directory to.gitignore.This session tracking file appears to be a local development artifact that shouldn't be committed to the repository. Adding the entire
.fork-join/directory to.gitignorewould prevent accidental commits of session state.server/database/db.js (1)
287-294:bcrypt.hashSyncblocks the event loop; prefer asyncbcrypt.hash.With cost factor 12,
hashSynccan block ~250ms per call. During orchestrator authentication flows with concurrent users, this could degrade server responsiveness. Consider makinggetOrCreateOrchestratorUserasync and usingawait bcrypt.hash(...).♻️ Suggested async refactor
- getOrCreateOrchestratorUser: (githubId, githubUsername) => { + getOrCreateOrchestratorUser: async (githubId, githubUsername) => { try { // ... validation and lookup code ... if (existingByUsername) { if (existingByUsername.github_id && existingByUsername.github_id !== githubId) { const namespacedUsername = `orch_${githubId}_${githubUsername}`; const randomPassword = crypto.randomBytes(32).toString("hex"); - const passwordHash = bcrypt.hashSync(randomPassword, 12); + const passwordHash = await bcrypt.hash(randomPassword, 12); // ... } } // For new user creation: const randomPassword = crypto.randomBytes(32).toString("hex"); - const passwordHash = bcrypt.hashSync(randomPassword, 12); + const passwordHash = await bcrypt.hash(randomPassword, 12); // ... } },PR_DESCRIPTION.md (1)
27-42: Add language specifier to fenced code block.Per static analysis (MD040), fenced code blocks should have a language specified. For ASCII diagrams, use
textorplaintext:📝 Suggested fix
-``` +```text ┌─────────────────┐ ┌──────────────────────────────────────────────┐ │ claudecodeui │────▶│ Orchestrator Server │server/middleware/auth.js (1)
64-77: Debug logging exposes token payload in server logs.When
AUTH_DEBUG=true, the unverified token payload (includinguserIdandusername) is logged. While this is behind a flag, ensure logs with AUTH_DEBUG enabled are treated as sensitive and not persisted to log aggregation systems in production.server/orchestrator/client.js (2)
656-688: Static cache-busting version requires manual maintenance.The
cacheVersionconstant (line 658) is hardcoded and requires manual incrementing when URL rewriting logic changes. Consider deriving it from the package version or a build-time hash.💡 Alternative approaches
// Option 1: Use package version const cacheVersion = process.env.npm_package_version || "1.0.0"; // Option 2: Use a hash of the rewriting logic (computed once at module load) import { createHash } from "crypto"; const REWRITE_LOGIC_HASH = createHash("md5") .update(OrchestratorClient.prototype.rewriteHtmlUrls.toString()) .digest("hex") .slice(0, 8);
908-920: Method isasyncbut contains noawait.
getOrCreateOrchestratorTokenis declaredasyncbut neitheruserDb.getOrCreateOrchestratorUsernorgenerateTokenappear to be async operations based on typical usage patterns. This works correctly but theasynckeyword is unnecessary.This is a nitpick - keeping it
asyncprovides flexibility if the implementation changes in the future to require async operations.
Comment on lines
166
to
178
| const onRegistered = () => { | ||
| clearTimeout(connectTimeout); | ||
| resolve(); | ||
| }; | ||
|
|
||
| const onError = (error) => { | ||
| clearTimeout(connectTimeout); | ||
| this.removeListener("registered", onRegistered); | ||
| reject(error); | ||
| }; | ||
|
|
||
| this.once("registered", onRegistered); | ||
| this.once("error", onError); |
Contributor
There was a problem hiding this comment.
Listener cleanup missing on connection close.
If the WebSocket closes before registration completes (e.g., server rejects connection), the onRegistered listener remains attached to the EventEmitter, causing a potential memory leak on repeated connection attempts.
🔧 Suggested fix
const onRegistered = () => {
clearTimeout(connectTimeout);
+ this.removeListener("error", onError);
resolve();
};
const onError = (error) => {
clearTimeout(connectTimeout);
this.removeListener("registered", onRegistered);
reject(error);
};
this.once("registered", onRegistered);
this.once("error", onError);
+
+ // Also clean up on close before registration
+ this.ws.once("close", () => {
+ this.removeListener("registered", onRegistered);
+ this.removeListener("error", onError);
+ });🤖 Prompt for AI Agents
In `@server/orchestrator/client.js` around lines 166 - 178, The onRegistered
listener is left attached if the socket closes before registration; add a close
handler that clears connectTimeout and removes both onRegistered and onError
listeners and rejects the promise to prevent leaks. Specifically, after
registering this.once("registered", onRegistered) and this.once("error",
onError), add a this.once("close", onClose) (or similar) where onClose calls
clearTimeout(connectTimeout), this.removeListener("registered", onRegistered),
this.removeListener("error", onError), and rejects with an appropriate error;
also ensure the onClose handler removes itself after running so all listeners
are cleaned up.
Comment on lines
+32
to
+60
| async function githubFetch(endpoint, token) { | ||
| const controller = new AbortController(); | ||
| const timeoutId = setTimeout(() => controller.abort(), 8000); | ||
|
|
||
| try { | ||
| const response = await fetch(`${GITHUB_API_BASE}${endpoint}`, { | ||
| headers: { | ||
| Authorization: `Bearer ${token}`, | ||
| Accept: "application/vnd.github+json", | ||
| "X-GitHub-Api-Version": "2022-11-28", | ||
| "User-Agent": "claudecodeui-orchestrator", | ||
| }, | ||
| signal: controller.signal, | ||
| }); | ||
|
|
||
| if (!response.ok) { | ||
| const errorText = await response.text(); | ||
| throw new Error(`GitHub API error (${response.status}): ${errorText}`); | ||
| } | ||
|
|
||
| return response.json(); | ||
| } catch (error) { | ||
| if (error.name === "AbortError") { | ||
| throw new Error("GitHub API request timed out after 8 seconds"); | ||
| } | ||
| throw error; | ||
| } finally { | ||
| clearTimeout(timeoutId); | ||
| } |
Contributor
There was a problem hiding this comment.
Confirm runtime support for global fetch/AbortController.
githubFetch() assumes these globals exist. If you support Node <18, this will crash unless a polyfill (e.g., undici) is loaded or engines are pinned.
Please verify runtime expectations:
#!/bin/bash
rg -n "engines|node" package.json
rg -n "undici|node-fetch|fetch\\(" -S🤖 Prompt for AI Agents
In `@server/orchestrator/github-auth.js` around lines 32 - 60, githubFetch
currently assumes global fetch and AbortController exist, which will break on
Node <18; to fix, either declare Node >=18 in package.json engines or add a
runtime polyfill/fallback: detect globalThis.fetch and
globalThis.AbortController at startup and if missing dynamically import a
polyfill (e.g., undici) and assign globalThis.fetch and
globalThis.AbortController before using githubFetch (or refactor githubFetch to
accept an injected fetch/signal implementation); update package.json or CI docs
accordingly and ensure any new dependency is listed in package.json. Reference
symbols: githubFetch, fetch, AbortController, GITHUB_API_BASE.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@docs/Orchestrator-Client.md`:
- Around line 188-191: The fenced code block containing the orchestrator log
lines must include a language identifier to satisfy MD040; update the fence
around the two lines "[ORCHESTRATOR] Connected to orchestrator" and
"[ORCHESTRATOR] Successfully connected and registered" by changing the opening
triple backticks to include "text" (i.e., ```text) so the snippet is explicitly
marked as plain text.
- Around line 16-31: The fenced ASCII-art block in Orchestrator-Client.md is
missing a language identifier causing MD040 lint failures; update the opening
triple-backtick to include a language tag (e.g., replace ``` with ```text) for
the ASCII diagram code fence so the markdown linter accepts the block and
preserves formatting.
♻️ Duplicate comments (2)
.fork-join/tracked_files.txt (1)
1-11: Replace absolute paths with repo‑relative entries (or remove file if local-only).These entries expose a developer home directory and aren’t portable. Prefer repo‑relative paths or drop this file if it’s a local artifact.
🧭 Example rewrite to repo‑relative paths
-/Users/liam.helmer/repos/epiphytic/claudecodeui/server/database/db.js -/Users/liam.helmer/repos/epiphytic/claudecodeui/server/middleware/auth.js -/Users/liam.helmer/repos/epiphytic/claudecodeui/server/orchestrator/client.js -/Users/liam.helmer/repos/epiphytic/claudecodeui/server/orchestrator/github-auth.js -/Users/liam.helmer/repos/epiphytic/claudecodeui/server/orchestrator/index.js -/Users/liam.helmer/repos/epiphytic/claudecodeui/server/orchestrator/protocol.js -/Users/liam.helmer/repos/epiphytic/claudecodeui/server/orchestrator/proxy.js -/Users/liam.helmer/repos/epiphytic/claudecodeui/src/App.jsx -/Users/liam.helmer/repos/epiphytic/claudecodeui/server/database/init.sql -/Users/liam.helmer/repos/epiphytic/claudecodeui/docs/Orchestrator-Client.md -/Users/liam.helmer/repos/epiphytic/claudecodeui/README.md +server/database/db.js +server/middleware/auth.js +server/orchestrator/client.js +server/orchestrator/github-auth.js +server/orchestrator/index.js +server/orchestrator/protocol.js +server/orchestrator/proxy.js +src/App.jsx +server/database/init.sql +docs/Orchestrator-Client.md +README.md.fork-join/session-1768779775.json (1)
1-25: Remove internal session artifact from the repo.This file contains internal workflow prompts/metadata and shouldn’t be tracked.
🧹 Suggested follow-up in .gitignore
+.fork-join/
🧹 Nitpick comments (1)
README.md (1)
6-6: Tighten the opening sentence for clarity.Consider a shorter phrasing to reduce repetition.
✍️ Suggested rewrite
-A desktop and mobile UI for [Claude Code](https://docs.anthropic.com/en/docs/claude-code), [Cursor CLI](https://docs.cursor.com/en/cli/overview) and [Codex](https://developers.openai.com/codex). You can use it locally or remotely to view your active projects and sessions in Claude Code, Cursor, or Codex and make changes to them from everywhere (mobile or desktop). This gives you a proper interface that works everywhere. +A desktop and mobile UI for [Claude Code](https://docs.anthropic.com/en/docs/claude-code), [Cursor CLI](https://docs.cursor.com/en/cli/overview) and [Codex](https://developers.openai.com/codex). Use it locally or remotely to view active projects and sessions and make changes from anywhere (mobile or desktop).
Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Add USER_REQUEST_FOLLOW_UP to InboundMessageTypes in protocol.js - Add validation for follow-up message structure - Register follow-up listener in createUserRequestHandler - Handle follow-up events in client.js handleMessage switch - Auto-register listener on first handleUserRequest call The handleFollowUpMessage function was previously unreachable because activeProxySockets was only accessible inside createUserRequestHandler. Now the follow-up listener is registered on the orchestrator client and routes messages to existing proxy sockets by request_id. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Security fixes: - Add escapeForJs() to prevent XSS in orchestrator auth script - Escape orchestratorUsername and token before JS interpolation - Remove error.message from proxy error response (info leakage) - Fix payload type override by spreading payload before action Correctness fixes: - Add listener cleanup on WebSocket close before registration - Return null from initializeOrchestrator on connection failure - Add language specifiers to markdown code blocks (MD040) Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Use async bcrypt.hash() instead of blocking hashSync() - Add Node.js engine requirement >=18.0.0 for fetch/AbortController - Remove unused JWT_SECRET export from auth.js - Derive cacheVersion from npm_package_version instead of static string - Add await for async getOrCreateOrchestratorUser call Co-Authored-By: Claude Opus 4.5 <[email protected]>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@server/database/db.js`:
- Around line 239-341: In getOrCreateOrchestratorUser, avoid binding an
orchestrator GitHub identity to a local account that has no github_id; change
the branch that currently updates users when existingByUsername exists so that
you only bind when existingByUsername.github_id === githubId, and in the case
existingByUsername.github_id is falsy (or different) create a namespaced
orchestrator user (using namespacedUsername like
`orch_${githubId}_${githubUsername}` and a random password) instead of running
the UPDATE that assigns github_id; update logging accordingly and keep the
existing namespaced-creation flow in the username-collision case (refer to
existingByUsername, namespacedUsername, randomPassword, passwordHash, and the
INSERT stmt).
In `@server/orchestrator/client.js`:
- Around line 685-811: The proxyBase coming from requests is being interpolated
directly into HTML attributes and inline JS in rewriteHtmlUrls (used in
proxyPatchScript and authScript) which allows injection; validate and normalize
proxyBase first (e.g., ensure it starts with "/" and matches a tight whitelist
regex like only alphanumerics, -, _, ., :, / and %), then derive two sanitized
values: one escaped for HTML attributes (safeProxyBaseForAttr) and one escaped
for JS literals (safeProxyBaseForJs) using existing helpers (use
this.escapeForJs for JS and an HTML-escape helper or a simple replace of " and '
& < > for attributes). Replace all uses of proxyBase in the HTML replacements
and in proxyPatchScript/authScript with the appropriate sanitized variables (use
safeProxyBaseForAttr for attribute replacements and safeProxyBaseForJs inside JS
strings); keep original proxyBase only for validation logic, never embed the raw
value.
In `@server/orchestrator/proxy.js`:
- Around line 249-339: handleUserRequest can throw async errors and currently
leaves activeProxySockets and statusHooks in an inconsistent state; wrap the
main body of handleUserRequest in a try/catch (and optionally a finally) so any
exception during githubAuth.validate, proxy creation, or
handlers.handleChatMessage is caught, a generic error is sent via
orchestratorClient.sendError(requestId, "..."), and resources are cleaned up: if
proxySocket was created ensure proxySocket.close(...) is called, remove
requestId from activeProxySockets, and call
statusHooks?.onConnectionClose?.(connectionId); keep createAuthErrorMessage
handling for auth failures but use the same cleanup pattern for unexpected
errors. Ensure the function returns null after handling the error so callers
invoking handleUserRequest without await won't leave dangling state.
🧹 Nitpick comments (2)
package.json (1)
43-45: Consider requiring Node.js 20+ since Node 18 is past EOL.Node.js 18 reached end-of-life in April 2025. For better security and long-term support, consider bumping the minimum to
>=20.0.0(current LTS) or>=22.0.0.Suggested change
"engines": { - "node": ">=18.0.0" + "node": ">=20.0.0" },server/middleware/auth.js (1)
6-77: Constrain debug logging to non‑production and redact payload.
AUTH_DEBUGlogs the unverified token payload. If enabled in production, that can leak PII. Consider gating byNODE_ENVand logging only minimal fields.♻️ Suggested tweak
-const AUTH_DEBUG = process.env.AUTH_DEBUG === "true"; +const AUTH_DEBUG = + process.env.AUTH_DEBUG === "true" && + process.env.NODE_ENV !== "production"; @@ - console.log( - `[AUTH] Token payload (unverified):`, - JSON.stringify(unverified), - ); + console.log( + "[AUTH] Token payload (unverified):", + JSON.stringify({ + userId: unverified?.userId, + username: unverified?.username, + }), + );
Comment on lines
+239
to
+341
| // Get or create user for orchestrator authentication | ||
| // When connecting via orchestrator proxy, auto-create user if needed | ||
| getOrCreateOrchestratorUser: async (githubId, githubUsername) => { | ||
| try { | ||
| // Validate required parameters | ||
| if ( | ||
| !githubUsername || | ||
| typeof githubUsername !== "string" || | ||
| !githubUsername.trim() | ||
| ) { | ||
| throw new Error( | ||
| "githubUsername is required and must be a non-empty string", | ||
| ); | ||
| } | ||
| if (!githubId || typeof githubId !== "string" || !githubId.trim()) { | ||
| throw new Error("githubId is required and must be a non-empty string"); | ||
| } | ||
|
|
||
| // First, try to find user by github_id (most authoritative) | ||
| let user = db | ||
| .prepare( | ||
| "SELECT id, username, github_id, created_at, last_login FROM users WHERE github_id = ? AND is_active = 1", | ||
| ) | ||
| .get(githubId); | ||
|
|
||
| if (user) { | ||
| // Found user by github_id, update last_login | ||
| db.prepare( | ||
| "UPDATE users SET last_login = CURRENT_TIMESTAMP WHERE id = ?", | ||
| ).run(user.id); | ||
| return user; | ||
| } | ||
|
|
||
| // No user found by github_id, check if username exists | ||
| const existingByUsername = db | ||
| .prepare( | ||
| "SELECT id, username, github_id, created_at, last_login FROM users WHERE username = ? AND is_active = 1", | ||
| ) | ||
| .get(githubUsername); | ||
|
|
||
| if (existingByUsername) { | ||
| // Username exists - check if it has a different github_id | ||
| if ( | ||
| existingByUsername.github_id && | ||
| existingByUsername.github_id !== githubId | ||
| ) { | ||
| // Username collision: local account has different github_id | ||
| // Create namespaced orchestrator user instead of reusing | ||
| const namespacedUsername = `orch_${githubId}_${githubUsername}`; | ||
| const randomPassword = crypto.randomBytes(32).toString("hex"); | ||
| const passwordHash = await bcrypt.hash(randomPassword, 12); | ||
|
|
||
| const stmt = db.prepare( | ||
| "INSERT INTO users (username, password_hash, github_id, has_completed_onboarding, last_login) VALUES (?, ?, ?, 1, CURRENT_TIMESTAMP)", | ||
| ); | ||
| const result = stmt.run(namespacedUsername, passwordHash, githubId); | ||
| user = { | ||
| id: result.lastInsertRowid, | ||
| username: namespacedUsername, | ||
| github_id: githubId, | ||
| }; | ||
| console.log( | ||
| `[ORCHESTRATOR] Created namespaced user for orchestrator auth: ${namespacedUsername} (GitHub ID: ${githubId})`, | ||
| ); | ||
| return user; | ||
| } | ||
|
|
||
| // Username exists with no github_id or matching github_id - bind the github_id | ||
| db.prepare( | ||
| "UPDATE users SET github_id = ?, last_login = CURRENT_TIMESTAMP WHERE id = ?", | ||
| ).run(githubId, existingByUsername.id); | ||
| console.log( | ||
| `[ORCHESTRATOR] Bound github_id ${githubId} to existing user: ${githubUsername}`, | ||
| ); | ||
| return { ...existingByUsername, github_id: githubId }; | ||
| } | ||
|
|
||
| // No existing user found, create new one | ||
| const randomPassword = crypto.randomBytes(32).toString("hex"); | ||
| const passwordHash = await bcrypt.hash(randomPassword, 12); | ||
|
|
||
| const stmt = db.prepare( | ||
| "INSERT INTO users (username, password_hash, github_id, has_completed_onboarding, last_login) VALUES (?, ?, ?, 1, CURRENT_TIMESTAMP)", | ||
| ); | ||
| const result = stmt.run(githubUsername, passwordHash, githubId); | ||
| user = { | ||
| id: result.lastInsertRowid, | ||
| username: githubUsername, | ||
| github_id: githubId, | ||
| }; | ||
| console.log( | ||
| `[ORCHESTRATOR] Created new user for orchestrator auth: ${githubUsername} (GitHub ID: ${githubId})`, | ||
| ); | ||
|
|
||
| return user; | ||
| } catch (err) { | ||
| console.error( | ||
| "[ORCHESTRATOR] Error in getOrCreateOrchestratorUser:", | ||
| err, | ||
| ); | ||
| throw err; | ||
| } | ||
| }, |
Contributor
There was a problem hiding this comment.
Avoid binding orchestrator users to existing local usernames.
If a local account exists without github_id, a remote GitHub user with the same username can be auto‑bound and gain access. Prefer creating a namespaced orchestrator user unless the existing account is already linked to the same GitHub ID (or gate this behavior behind explicit config).
🔒 Suggested fix
if (existingByUsername) {
// Username exists - check if it has a different github_id
- if (
- existingByUsername.github_id &&
- existingByUsername.github_id !== githubId
- ) {
+ if (
+ !existingByUsername.github_id ||
+ existingByUsername.github_id !== githubId
+ ) {
// Username collision: local account has different github_id
// Create namespaced orchestrator user instead of reusing
const namespacedUsername = `orch_${githubId}_${githubUsername}`;
@@
- // Username exists with no github_id or matching github_id - bind the github_id
- db.prepare(
- "UPDATE users SET github_id = ?, last_login = CURRENT_TIMESTAMP WHERE id = ?",
- ).run(githubId, existingByUsername.id);
- console.log(
- `[ORCHESTRATOR] Bound github_id ${githubId} to existing user: ${githubUsername}`,
- );
- return { ...existingByUsername, github_id: githubId };
+ // Already linked to this GitHub ID; just update last_login
+ db.prepare(
+ "UPDATE users SET last_login = CURRENT_TIMESTAMP WHERE id = ?",
+ ).run(existingByUsername.id);
+ return existingByUsername;
}🤖 Prompt for AI Agents
In `@server/database/db.js` around lines 239 - 341, In
getOrCreateOrchestratorUser, avoid binding an orchestrator GitHub identity to a
local account that has no github_id; change the branch that currently updates
users when existingByUsername exists so that you only bind when
existingByUsername.github_id === githubId, and in the case
existingByUsername.github_id is falsy (or different) create a namespaced
orchestrator user (using namespacedUsername like
`orch_${githubId}_${githubUsername}` and a random password) instead of running
the UPDATE that assigns github_id; update logging accordingly and keep the
existing namespaced-creation flow in the username-collision case (refer to
existingByUsername, namespacedUsername, randomPassword, passwordHash, and the
INSERT stmt).
Comment on lines
685
to
811
| rewriteHtmlUrls( | ||
| html, | ||
| proxyBase, | ||
| orchestratorToken = null, | ||
| orchestratorUsername = null, | ||
| ) { | ||
| // Add a cache-busting version to force fresh fetches from Cloudflare edge | ||
| // Derived from package version to auto-increment on releases | ||
| const cacheVersion = process.env.npm_package_version || "1.0.0"; | ||
|
|
||
| let result = html | ||
| .replace(/src="\/(?!\/)/g, `src="${proxyBase}/`) | ||
| .replace(/href="\/(?!\/)/g, `href="${proxyBase}/`) | ||
| .replace(/action="\/(?!\/)/g, `action="${proxyBase}/`) | ||
| .replace(/src='\/(?!\/)/g, `src='${proxyBase}/`) | ||
| .replace(/href='\/(?!\/)/g, `href='${proxyBase}/`) | ||
| .replace(/action='\/(?!\/)/g, `action='${proxyBase}/`) | ||
| // Handle service worker registration: pass proxyBase as query param | ||
| // e.g., register('/sw.js') -> register('/clients/.../proxy/sw.js?proxyBase=/clients/.../proxy') | ||
| .replace( | ||
| /\.register\('\/sw\.js'\)/g, | ||
| `.register('${proxyBase}/sw.js?proxyBase=${encodeURIComponent(proxyBase)}')`, | ||
| ) | ||
| .replace( | ||
| /\.register\("\/sw\.js"\)/g, | ||
| `.register("${proxyBase}/sw.js?proxyBase=${encodeURIComponent(proxyBase)}")`, | ||
| ); | ||
|
|
||
| // Add cache-busting parameter to JS and CSS asset URLs | ||
| // Match patterns like src="/clients/.../proxy/assets/file.js" or href="/clients/.../proxy/assets/file.css" | ||
| result = result.replace( | ||
| /(<script[^>]+src="[^"]+\.js)(")/g, | ||
| `$1?_=${cacheVersion}$2`, | ||
| ); | ||
| result = result.replace( | ||
| /(<link[^>]+href="[^"]+\.css)(")/g, | ||
| `$1?_=${cacheVersion}$2`, | ||
| ); | ||
|
|
||
| // Inject scripts to: | ||
| // 1. Auto-authenticate via orchestrator token | ||
| // 2. Patch fetch() to redirect API calls through the proxy | ||
| // | ||
| // Note: React app uses 'auth-token' as the localStorage key | ||
| // We need to check if the existing token belongs to the same orchestrator user. | ||
| // If not, we update the token. We also store the orchestrator username separately | ||
| // to detect user changes without decoding the JWT on every request. | ||
| const proxyPatchScript = `<script> | ||
| // Patch fetch and WebSocket to redirect through the proxy | ||
| (function() { | ||
| const proxyBase = "${proxyBase}"; | ||
|
|
||
| // Make proxyBase available globally for React app components | ||
| window.__ORCHESTRATOR_PROXY_BASE__ = proxyBase; | ||
|
|
||
| const originalFetch = window.fetch; | ||
|
|
||
| window.fetch = function(url, options) { | ||
| // Convert URL to string if it's a Request object | ||
| let urlStr = url instanceof Request ? url.url : String(url); | ||
|
|
||
| // If it's an absolute path (starts with /) but not a full URL (no protocol) | ||
| // and not already going through the proxy, redirect it | ||
| if (urlStr.startsWith('/') && !urlStr.startsWith('//') && !urlStr.startsWith(proxyBase)) { | ||
| const newUrl = proxyBase + urlStr; | ||
| console.log('[ORCHESTRATOR] Redirecting fetch:', urlStr, '->', newUrl); | ||
|
|
||
| if (url instanceof Request) { | ||
| // Create a new Request with the modified URL | ||
| url = new Request(newUrl, url); | ||
| } else { | ||
| url = newUrl; | ||
| } | ||
| } | ||
|
|
||
| return originalFetch.call(this, url, options); | ||
| }; | ||
|
|
||
| console.log('[ORCHESTRATOR] Fetch patched for proxy base:', proxyBase); | ||
| })(); | ||
| </script>`; | ||
|
|
||
| // Escape user-controlled values to prevent XSS | ||
| const safeUsername = this.escapeForJs(orchestratorUsername || ""); | ||
| const safeToken = this.escapeForJs(orchestratorToken || ""); | ||
|
|
||
| const authScript = orchestratorToken | ||
| ? `<script> | ||
| // Auto-authenticate via orchestrator token | ||
| (function() { | ||
| const existingToken = localStorage.getItem('auth-token'); | ||
| const storedOrchestratorUser = localStorage.getItem('orchestrator-user'); | ||
| const orchestratorUsername = "${safeUsername}"; | ||
|
|
||
| // Check if we need to update the token: | ||
| // 1. No existing token | ||
| // 2. Orchestrator user changed (different GitHub user accessing via proxy) | ||
| // 3. Token exists but no stored orchestrator user (legacy direct login token) | ||
| const needsUpdate = !existingToken || | ||
| (orchestratorUsername && storedOrchestratorUser !== orchestratorUsername) || | ||
| (existingToken && !storedOrchestratorUser && orchestratorUsername); | ||
|
|
||
| if (needsUpdate) { | ||
| const token = "${safeToken}"; | ||
| localStorage.setItem('auth-token', token); | ||
| if (orchestratorUsername) { | ||
| localStorage.setItem('orchestrator-user', orchestratorUsername); | ||
| } | ||
| console.log('[ORCHESTRATOR] Auto-authenticated via orchestrator proxy for user:', orchestratorUsername || 'unknown'); | ||
| // Reload the page so the app initializes with the token | ||
| window.location.reload(); | ||
| return; | ||
| } | ||
| console.log('[ORCHESTRATOR] Already have valid auth token for user:', storedOrchestratorUser); | ||
| })(); | ||
| </script>` | ||
| : ""; | ||
|
|
||
| // Inject both scripts right after the opening <head> tag | ||
| // Proxy patch must come first so fetch is patched before any other scripts run | ||
| result = result.replace( | ||
| /<head>/i, | ||
| `<head>${proxyPatchScript}${authScript}`, | ||
| ); | ||
|
|
||
| return result; | ||
| } |
Contributor
There was a problem hiding this comment.
Sanitize proxyBase before embedding in HTML/JS.
proxy_base comes from the orchestrator request and is interpolated into HTML attributes and inline JS. If it contains quotes or script‑breaking sequences, it can inject markup/script. Validate it to a safe path and use a safe JS literal.
🔒 Suggested fix
rewriteHtmlUrls(
html,
proxyBase,
orchestratorToken = null,
orchestratorUsername = null,
) {
+ const normalizedProxyBase = typeof proxyBase === "string" ? proxyBase : "";
+ if (!/^\/[A-Za-z0-9/_\-.]+$/.test(normalizedProxyBase)) {
+ console.warn("[ORCHESTRATOR] Invalid proxyBase received, skipping rewrite");
+ return html;
+ }
+ const safeProxyBase = normalizedProxyBase;
+
// Add a cache-busting version to force fresh fetches from Cloudflare edge
// Derived from package version to auto-increment on releases
const cacheVersion = process.env.npm_package_version || "1.0.0";
let result = html
- .replace(/src="\/(?!\/)/g, `src="${proxyBase}/`)
- .replace(/href="\/(?!\/)/g, `href="${proxyBase}/`)
- .replace(/action="\/(?!\/)/g, `action="${proxyBase}/`)
- .replace(/src='\/(?!\/)/g, `src='${proxyBase}/`)
- .replace(/href='\/(?!\/)/g, `href='${proxyBase}/`)
- .replace(/action='\/(?!\/)/g, `action='${proxyBase}/`)
+ .replace(/src="\/(?!\/)/g, `src="${safeProxyBase}/`)
+ .replace(/href="\/(?!\/)/g, `href="${safeProxyBase}/`)
+ .replace(/action="\/(?!\/)/g, `action="${safeProxyBase}/`)
+ .replace(/src='\/(?!\/)/g, `src='${safeProxyBase}/`)
+ .replace(/href='\/(?!\/)/g, `href='${safeProxyBase}/`)
+ .replace(/action='\/(?!\/)/g, `action='${safeProxyBase}/`)
@@
- `.register('${proxyBase}/sw.js?proxyBase=${encodeURIComponent(proxyBase)}')`,
+ `.register('${safeProxyBase}/sw.js?proxyBase=${encodeURIComponent(
+ safeProxyBase,
+ )}')`,
)
.replace(
/\.register\("\/sw\.js"\)/g,
- `.register("${proxyBase}/sw.js?proxyBase=${encodeURIComponent(proxyBase)}")`,
+ `.register("${safeProxyBase}/sw.js?proxyBase=${encodeURIComponent(
+ safeProxyBase,
+ )}")`,
);
@@
(function() {
- const proxyBase = "${proxyBase}";
+ const proxyBase = ${JSON.stringify(safeProxyBase)};🤖 Prompt for AI Agents
In `@server/orchestrator/client.js` around lines 685 - 811, The proxyBase coming
from requests is being interpolated directly into HTML attributes and inline JS
in rewriteHtmlUrls (used in proxyPatchScript and authScript) which allows
injection; validate and normalize proxyBase first (e.g., ensure it starts with
"/" and matches a tight whitelist regex like only alphanumerics, -, _, ., :, /
and %), then derive two sanitized values: one escaped for HTML attributes
(safeProxyBaseForAttr) and one escaped for JS literals (safeProxyBaseForJs)
using existing helpers (use this.escapeForJs for JS and an HTML-escape helper or
a simple replace of " and ' & < > for attributes). Replace all uses of proxyBase
in the HTML replacements and in proxyPatchScript/authScript with the appropriate
sanitized variables (use safeProxyBaseForAttr for attribute replacements and
safeProxyBaseForJs inside JS strings); keep original proxyBase only for
validation logic, never embed the raw value.
Comment on lines
+249
to
+339
| async function handleUserRequest(orchestratorClient, message) { | ||
| const { request_id: requestId, action } = message; | ||
| // Initialize payload to prevent undefined mutation errors | ||
| let payload = message.payload || {}; | ||
|
|
||
| // Ensure follow-up listener is registered | ||
| setupFollowUpListener(orchestratorClient); | ||
|
|
||
| // Check if authentication is required (GitHub auth configured) | ||
| if (githubAuth.isConfigured) { | ||
| // Extract auth token from payload | ||
| const authToken = payload.auth_token; | ||
|
|
||
| // Validate the token | ||
| const authResult = await githubAuth.validate(authToken); | ||
|
|
||
| if (!authResult.authenticated) { | ||
| console.warn( | ||
| `[ORCHESTRATOR-PROXY] Authentication failed for request ${requestId}: ${authResult.error}`, | ||
| ); | ||
| // Send auth error back through orchestrator | ||
| const errorMsg = createAuthErrorMessage(requestId, authResult.error); | ||
| orchestratorClient.ws?.send(JSON.stringify(errorMsg)); | ||
| return null; | ||
| } | ||
|
|
||
| // Authentication succeeded - attach user info to payload | ||
| console.log( | ||
| `[ORCHESTRATOR-PROXY] Authenticated user: ${authResult.user.username} (${authResult.user.authMethod})`, | ||
| ); | ||
| // Use the validated user info | ||
| payload.user = authResult.user; | ||
| } | ||
|
|
||
| // Create proxy socket for this request | ||
| const proxySocket = new OrchestratorProxySocket( | ||
| orchestratorClient, | ||
| requestId, | ||
| { user: payload?.user }, | ||
| ); | ||
|
|
||
| // Track connection for status | ||
| const connectionId = `orchestrator-${requestId}`; | ||
| statusHooks?.onConnectionOpen?.(connectionId); | ||
|
|
||
| // Store in active map | ||
| activeProxySockets.set(requestId, proxySocket); | ||
|
|
||
| // Clean up on close | ||
| proxySocket.on("close", () => { | ||
| activeProxySockets.delete(requestId); | ||
| statusHooks?.onConnectionClose?.(connectionId); | ||
| }); | ||
|
|
||
| // Create writer wrapper | ||
| const writer = new OrchestratorProxyWriter(proxySocket); | ||
|
|
||
| // Route based on action | ||
| switch (action) { | ||
| case "claude-command": | ||
| case "cursor-command": | ||
| case "codex-command": | ||
| case "abort-session": | ||
| case "claude-permission-response": | ||
| case "cursor-abort": | ||
| case "check-session-status": | ||
| case "get-active-sessions": | ||
| // Inject the message as if it came from a real WebSocket | ||
| // The existing handler will process it via the 'message' event | ||
| if (handlers.handleChatMessage) { | ||
| handlers.handleChatMessage( | ||
| proxySocket, | ||
| writer, | ||
| JSON.stringify({ ...payload, type: action }), | ||
| ); | ||
| } | ||
| break; | ||
|
|
||
| case "close": | ||
| // Handle close request | ||
| proxySocket.close(payload?.code, payload?.reason); | ||
| break; | ||
|
|
||
| default: | ||
| console.warn(`[ORCHESTRATOR-PROXY] Unknown action: ${action}`); | ||
| orchestratorClient.sendError(requestId, `Unknown action: ${action}`); | ||
| proxySocket.close(4000, "Unknown action"); | ||
| } | ||
|
|
||
| return proxySocket; | ||
| } |
Contributor
There was a problem hiding this comment.
Handle async failures to avoid unhandled rejections.
handleUserRequest is async but invoked without await; any thrown error (auth validation, handler logic) becomes an unhandled rejection and can leave stale entries in activeProxySockets. Wrap the body in try/catch, close/cleanup, and send a generic error.
🧯 Suggested fix
async function handleUserRequest(orchestratorClient, message) {
const { request_id: requestId, action } = message;
// Initialize payload to prevent undefined mutation errors
- let payload = message.payload || {};
+ let payload = message.payload || {};
+ let proxySocket;
+
+ try {
// Ensure follow-up listener is registered
setupFollowUpListener(orchestratorClient);
@@
- const proxySocket = new OrchestratorProxySocket(
+ proxySocket = new OrchestratorProxySocket(
orchestratorClient,
requestId,
{ user: payload?.user },
);
@@
- return proxySocket;
+ return proxySocket;
+ } catch (error) {
+ console.error(
+ "[ORCHESTRATOR-PROXY] Failed to handle user request:",
+ error,
+ );
+ activeProxySockets.delete(requestId);
+ if (proxySocket) {
+ proxySocket.close(1011, "Internal error");
+ }
+ orchestratorClient.sendError?.(requestId, "Failed to handle request");
+ return null;
+ }
}🤖 Prompt for AI Agents
In `@server/orchestrator/proxy.js` around lines 249 - 339, handleUserRequest can
throw async errors and currently leaves activeProxySockets and statusHooks in an
inconsistent state; wrap the main body of handleUserRequest in a try/catch (and
optionally a finally) so any exception during githubAuth.validate, proxy
creation, or handlers.handleChatMessage is caught, a generic error is sent via
orchestratorClient.sendError(requestId, "..."), and resources are cleaned up: if
proxySocket was created ensure proxySocket.close(...) is called, remove
requestId from activeProxySockets, and call
statusHooks?.onConnectionClose?.(connectionId); keep createAuthErrorMessage
handling for auth failures but use the same cleanup pattern for unexpected
errors. Ensure the function returns null after handling the error so callers
invoking handleUserRequest without await won't leave dangling state.
…cket proxy Implement orchestrator client mode to allow claudecodeui instances to connect to a central orchestrator server. Instances authenticate using GitHub OAuth and establish a WebSocket connection to forward user traffic. The orchestrator can then route requests and aggregate responses from multiple connected clients, enabling centralized coordination of code execution across distributed Claude Code instances.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/utils/websocket.js (1)
12-19: Stale closure in cleanup function.The cleanup references
wsfrom the initial render, butwsstate may have changed. UsewsRefpattern or access the current state properly.Proposed fix
Consider using a ref to track the WebSocket instance for reliable cleanup:
+ const wsRef = useRef(null); const reconnectTimeoutRef = useRef(null); useEffect(() => { connect(); return () => { if (reconnectTimeoutRef.current) { clearTimeout(reconnectTimeoutRef.current); } - if (ws) { - ws.close(); - } + if (wsRef.current) { + wsRef.current.close(); + } }; }, []);Then update
wsRef.currentalongsidesetWs(websocket)in theonopenhandler.
🤖 Fix all issues with AI agents
In `@server/orchestrator/client.js`:
- Around line 214-225: The sendRegister() method currently includes
process.cwd() as the metadata.project value, which can leak local filesystem
paths; change this to a safe default and make sending the full cwd optional:
read a new config flag (e.g., this.config.includeProjectPath or
this.config.sendProjectPath) and if true include process.cwd(), otherwise use a
sanitized project identifier (e.g., this.config.projectName ||
path.basename(process.cwd()) || "unknown-project") or omit the field; update the
metadata construction in sendRegister() to reference these config values and add
documentation/comments describing the opt-in behavior.
- Around line 821-858: The rewriteJsUrls function uses the proxyBase parameter
directly in injected JS strings, so validate/sanitize proxyBase (e.g., allow
only a safe subset like protocol-relative/path characters and strip or
percent-encode quotes, backticks, newlines and regex metacharacters) before
using it; either add a shared sanitizer helper (e.g., sanitizeProxyBase) and
call it from handleHttpProxyRequest and rewriteJsUrls, or validate once in
handleHttpProxyRequest and pass a sanitizedProxyBase into rewriteJsUrls, then
use that sanitized value in the string replacements to prevent
malformed/exploitable JS injections (references: rewriteJsUrls, proxyBase,
rewriteHtmlUrls, handleHttpProxyRequest).
In `@server/routes/cli-auth.js`:
- Around line 14-18: The response currently hardcodes method: "credentials_file"
causing wrong reports for keychain credentials; update the response in the route
that uses checkClaudeCredentials() and credentialsResult to set method from
credentialsResult.method (fall back to "credentials_file" if absent) so the JSON
uses the actual method returned by checkClaudeCredentials()/credentialsResult
instead of the hardcoded string.
In `@src/components/Shell.jsx`:
- Around line 135-136: The console.log in Shell.jsx currently prints the full
wsUrl including the auth token (wsUrl =
`${wsInfo.ws_base}/shell?token=${encodeURIComponent(token)}` and
console.log("[ORCHESTRATOR] Shell connecting directly to:", wsUrl)), which
exposes credentials; change the logging to redact or omit the token by either
logging only wsInfo.ws_base or a masked URL (e.g., replace token value with
"[REDACTED]" or show a shortened/hashed token) so the auth token is never output
to logs.
In `@src/utils/websocket.js`:
- Around line 91-92: The console.log prints the full wsUrl including the auth
token—remove or redact the token before logging: in websocket.js where wsUrl is
built (wsUrl = `${wsInfo.ws_base}/ws?token=${encodeURIComponent(token)}`)
replace the debug log that references wsUrl with either no logging of the token
(log only wsInfo.ws_base or "Connecting directly to WebSocket (token redacted)")
or build a redactedUrl that omits or masks the token and log that instead;
update the console.log call accordingly (touch the lines that construct wsUrl,
reference token/encodeURIComponent, and the console.log).
- Around line 129-136: The setup path returns early on configuration/validation
failures (orchestrator proxy and OSS flows) without scheduling a reconnect,
causing asymmetrical behavior vs the websocket.onclose handler which uses
reconnectTimeoutRef.current to call connect() after 3s; update the setup error
branches to schedule retries instead of returning immediately by reusing the
same reconnect mechanism (e.g., set reconnectTimeoutRef.current to call
connect()), and add an exponential/backoff delay tracker (retryDelayRef or
similar) that increments on consecutive failures and resets on success so both
setup failures and onclose follow the same retry/backoff strategy.
♻️ Duplicate comments (1)
server/orchestrator/client.js (1)
732-767: XSS vulnerability:proxyBaseis interpolated without sanitization.The
proxyBaseparameter (from orchestrator request) is interpolated directly into the inline JavaScript at line 735 without escaping. If a malicious orchestrator sends aproxy_basecontaining"; alert(1); ", it would execute arbitrary JavaScript.While
orchestratorUsernameandorchestratorTokenare now properly escaped (lines 770-771),proxyBaseis still vulnerable.🔒 Suggested fix - validate and escape proxyBase
rewriteHtmlUrls( html, proxyBase, orchestratorToken = null, orchestratorUsername = null, ) { + // Validate proxyBase - must be a safe path pattern + const safeProxyBase = typeof proxyBase === "string" ? proxyBase : ""; + if (!/^\/[A-Za-z0-9/_\-.:]+$/.test(safeProxyBase)) { + console.warn("[ORCHESTRATOR] Invalid proxyBase received, skipping rewrite"); + return html; + } + // Add a cache-busting version to force fresh fetches from Cloudflare edge // ... const proxyPatchScript = `<script> // Patch fetch and WebSocket to redirect through the proxy (function() { - const proxyBase = "${proxyBase}"; + const proxyBase = ${JSON.stringify(safeProxyBase)};Using
JSON.stringify()ensures the value is properly escaped for JavaScript string context.
🧹 Nitpick comments (6)
server/routes/cli-auth.js (2)
174-183: Consider validating non-JSON keychain values before assuming authentication.If JSON parsing fails, the code assumes any non-empty stdout indicates valid authentication. This could lead to false positives if the keychain returns an unexpected format or error message that isn't caught by the exit code check. Consider adding basic validation (e.g., checking for token-like patterns or minimum length).
Proposed refinement
} catch { // Not JSON - might still be valid if it's just a token string - // Consider it authenticated if we got any non-empty value - resolve({ - authenticated: true, - email: "Keychain Auth", - method: "macos_keychain", - }); - return; + // Only consider it authenticated if it looks like a token (reasonable length, no spaces) + const value = stdout.trim(); + if (value.length >= 20 && !/\s/.test(value)) { + resolve({ + authenticated: true, + email: "Keychain Auth", + method: "macos_keychain", + }); + return; + } }
304-328: Add token expiration check for consistency with Claude credentials.The
checkClaudeCredentialsfunction checks theexpiresAtfield at lines 91 and 163 before returning authenticated. ThecheckCodexCredentialsfunction should similarly validate JWT token expiration by checking theexpclaim in the decoded payload. Even if Codex tokens don't include anexpclaim, checking for it is more defensive and maintains consistency with the Claude credentials handling pattern.src/utils/websocket.js (1)
36-99: Extract shared orchestrator proxy logic to reduce duplication.The orchestrator proxy connection logic (clientId extraction, ws-info fetch, URL construction) is nearly identical to
Shell.jsxlines 77-143. Consider extracting this into a shared utility function.Suggested extraction
Create a shared utility, e.g.,
src/utils/orchestratorWs.js:export async function getOrchestratorWsUrl(proxyBase, endpoint = "/ws") { const clientIdMatch = proxyBase.match(/\/clients\/([^/]+)\/proxy/); if (!clientIdMatch) { throw new Error(`Could not extract client ID from proxyBase: ${proxyBase}`); } const clientId = clientIdMatch[1]; const response = await fetch(`/api/clients/${clientId}/ws-info`, { credentials: "include", }); if (!response.ok) { throw new Error(`Failed to fetch WebSocket info: ${response.status}`); } const wsInfo = await response.json(); if (wsInfo.error) { throw new Error(wsInfo.error); } const token = localStorage.getItem("auth-token"); if (!token) { throw new Error("No authentication token found"); } return { wsUrl: `${wsInfo.ws_base}${endpoint}?token=${encodeURIComponent(token)}`, isLocal: wsInfo.is_local, }; }Then use in both files to reduce ~60 lines of duplication.
src/components/Shell.jsx (1)
77-143: Duplicated orchestrator proxy logic.This orchestrator proxy connection logic duplicates
src/utils/websocket.jslines 36-99. As noted in the websocket.js review, extracting this to a shared utility would improve maintainability.src/components/Onboarding.jsx (1)
87-175: Consider consolidating repetitive auth status check functions.
checkClaudeAuthStatus,checkCursorAuthStatus, andcheckCodexAuthStatushave nearly identical structure. A generic function could reduce duplication.Optional consolidation
const checkAuthStatus = async (provider, setStatus) => { try { const response = await authenticatedFetch(`/api/cli/${provider}/status`); if (response.ok) { const data = await response.json(); setStatus({ authenticated: data.authenticated, email: data.email, loading: false, error: data.error || null, }); } else { setStatus({ authenticated: false, email: null, loading: false, error: "Failed to check authentication status", }); } } catch (error) { console.error(`Error checking ${provider} auth status:`, error); setStatus({ authenticated: false, email: null, loading: false, error: error.message, }); } }; // Usage: const checkClaudeAuthStatus = () => checkAuthStatus("claude", setClaudeAuthStatus); const checkCursorAuthStatus = () => checkAuthStatus("cursor", setCursorAuthStatus); const checkCodexAuthStatus = () => checkAuthStatus("codex", setCodexAuthStatus);server/orchestrator/client.js (1)
543-544: Consider adding a timeout for the local HTTP request.The
fetch()call has no timeout, so if the local server is unresponsive, this proxy request will hang indefinitely, potentially exhausting resources if many requests accumulate.♻️ Suggested fix using AbortController
+ // Add timeout for local request to prevent indefinite hangs + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), 30000); // 30 second timeout + // Make the local HTTP request - const response = await fetch(url, fetchOptions); + const response = await fetch(url, { ...fetchOptions, signal: controller.signal }); + clearTimeout(timeoutId);
Comment on lines
+214
to
+225
| sendRegister() { | ||
| const metadata = { | ||
| hostname: os.hostname(), | ||
| platform: os.platform(), | ||
| project: process.cwd(), | ||
| status: this.status, | ||
| version: process.env.npm_package_version || "1.0.0", | ||
| // Include callback URL for HTTP proxy support | ||
| // This allows the orchestrator to proxy HTTP requests to this instance | ||
| callback_url: this.config.callbackUrl || null, | ||
| ...this.config.metadata, | ||
| }; |
Contributor
There was a problem hiding this comment.
Consider the information disclosed in registration metadata.
Line 218 sends process.cwd() as the project field, which exposes the local filesystem path to the orchestrator. Depending on the deployment environment, this could reveal sensitive information about the server's directory structure (e.g., /home/username/..., /var/www/...).
If this is intentional for debugging/identification purposes, consider documenting this behavior. Otherwise, consider using a sanitized project name or making it opt-in via configuration.
🤖 Prompt for AI Agents
In `@server/orchestrator/client.js` around lines 214 - 225, The sendRegister()
method currently includes process.cwd() as the metadata.project value, which can
leak local filesystem paths; change this to a safe default and make sending the
full cwd optional: read a new config flag (e.g., this.config.includeProjectPath
or this.config.sendProjectPath) and if true include process.cwd(), otherwise use
a sanitized project identifier (e.g., this.config.projectName ||
path.basename(process.cwd()) || "unknown-project") or omit the field; update the
metadata construction in sendRegister() to reference these config values and add
documentation/comments describing the opt-in behavior.
Comment on lines
+821
to
+858
| rewriteJsUrls(js, proxyBase) { | ||
| // Only rewrite specific path prefixes that are likely URLs, not regex patterns | ||
| // This is more selective than matching all "/" to avoid breaking regex literals | ||
| const urlPrefixes = [ | ||
| "api", | ||
| "assets", | ||
| "auth", | ||
| "ws", | ||
| "favicon", | ||
| "static", | ||
| "socket.io", | ||
| "sw.js", | ||
| "manifest.json", | ||
| "icons", | ||
| ]; | ||
|
|
||
| // Orchestrator API paths that should NOT be rewritten (they go directly to the orchestrator) | ||
| const orchestratorApiPaths = ["/api/clients/", "/api/tokens"]; | ||
|
|
||
| let result = js; | ||
| for (const prefix of urlPrefixes) { | ||
| // Escape regex metacharacters in the prefix to match literal characters | ||
| const escapedPrefix = prefix.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); | ||
| // Match "/${prefix}, '/${prefix}, `/${prefix} patterns | ||
| result = result | ||
| .replace( | ||
| new RegExp(`"\\/${escapedPrefix}(?=[\\/"])`, "g"), | ||
| `"${proxyBase}/${prefix}`, | ||
| ) | ||
| .replace( | ||
| new RegExp(`'\\/${escapedPrefix}(?=[\\/'])`, "g"), | ||
| `'${proxyBase}/${prefix}`, | ||
| ) | ||
| .replace( | ||
| new RegExp(`\`\\/${escapedPrefix}(?=[\\/\`])`, "g"), | ||
| `\`${proxyBase}/${prefix}`, | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
Same proxyBase sanitization issue applies here.
The proxyBase parameter is used directly in string replacements without validation. If it contains special characters, the resulting JavaScript could be malformed or exploitable. The same validation suggested for rewriteHtmlUrls should be applied here, or extract validation to a shared helper called at the entry point in handleHttpProxyRequest.
🤖 Prompt for AI Agents
In `@server/orchestrator/client.js` around lines 821 - 858, The rewriteJsUrls
function uses the proxyBase parameter directly in injected JS strings, so
validate/sanitize proxyBase (e.g., allow only a safe subset like
protocol-relative/path characters and strip or percent-encode quotes, backticks,
newlines and regex metacharacters) before using it; either add a shared
sanitizer helper (e.g., sanitizeProxyBase) and call it from
handleHttpProxyRequest and rewriteJsUrls, or validate once in
handleHttpProxyRequest and pass a sanitizedProxyBase into rewriteJsUrls, then
use that sanitized value in the string replacements to prevent
malformed/exploitable JS injections (references: rewriteJsUrls, proxyBase,
rewriteHtmlUrls, handleHttpProxyRequest).
Comment on lines
14
to
18
| return res.json({ | ||
| authenticated: true, | ||
| email: credentialsResult.email || 'Authenticated', | ||
| method: 'credentials_file' | ||
| email: credentialsResult.email || "Authenticated", | ||
| method: "credentials_file", | ||
| }); |
Contributor
There was a problem hiding this comment.
Bug: The method field from credentialsResult is not used.
The response hardcodes method: "credentials_file" but checkClaudeCredentials() returns the actual method ("credentials_file" or "macos_keychain"). This causes keychain-sourced credentials to be incorrectly reported.
Proposed fix
if (credentialsResult.authenticated) {
return res.json({
authenticated: true,
email: credentialsResult.email || "Authenticated",
- method: "credentials_file",
+ method: credentialsResult.method || "credentials_file",
});
}📝 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.
Suggested change
| return res.json({ | |
| authenticated: true, | |
| email: credentialsResult.email || 'Authenticated', | |
| method: 'credentials_file' | |
| email: credentialsResult.email || "Authenticated", | |
| method: "credentials_file", | |
| }); | |
| return res.json({ | |
| authenticated: true, | |
| email: credentialsResult.email || "Authenticated", | |
| method: credentialsResult.method || "credentials_file", | |
| }); |
🤖 Prompt for AI Agents
In `@server/routes/cli-auth.js` around lines 14 - 18, The response currently
hardcodes method: "credentials_file" causing wrong reports for keychain
credentials; update the response in the route that uses checkClaudeCredentials()
and credentialsResult to set method from credentialsResult.method (fall back to
"credentials_file" if absent) so the JSON uses the actual method returned by
checkClaudeCredentials()/credentialsResult instead of the hardcoded string.
Comment on lines
+135
to
+136
| wsUrl = `${wsInfo.ws_base}/shell?token=${encodeURIComponent(token)}`; | ||
| console.log("[ORCHESTRATOR] Shell connecting directly to:", wsUrl); |
Contributor
There was a problem hiding this comment.
Token exposed in console log.
Similar to websocket.js, the full WebSocket URL containing the auth token is logged. Redact the token from logs.
Proposed fix
wsUrl = `${wsInfo.ws_base}/shell?token=${encodeURIComponent(token)}`;
- console.log("[ORCHESTRATOR] Shell connecting directly to:", wsUrl);
+ console.log("[ORCHESTRATOR] Shell connecting directly to:", wsInfo.ws_base);📝 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.
Suggested change
| wsUrl = `${wsInfo.ws_base}/shell?token=${encodeURIComponent(token)}`; | |
| console.log("[ORCHESTRATOR] Shell connecting directly to:", wsUrl); | |
| wsUrl = `${wsInfo.ws_base}/shell?token=${encodeURIComponent(token)}`; | |
| console.log("[ORCHESTRATOR] Shell connecting directly to:", wsInfo.ws_base); |
🤖 Prompt for AI Agents
In `@src/components/Shell.jsx` around lines 135 - 136, The console.log in
Shell.jsx currently prints the full wsUrl including the auth token (wsUrl =
`${wsInfo.ws_base}/shell?token=${encodeURIComponent(token)}` and
console.log("[ORCHESTRATOR] Shell connecting directly to:", wsUrl)), which
exposes credentials; change the logging to redact or omit the token by either
logging only wsInfo.ws_base or a masked URL (e.g., replace token value with
"[REDACTED]" or show a shortened/hashed token) so the auth token is never output
to logs.
Comment on lines
+91
to
+92
| wsUrl = `${wsInfo.ws_base}/ws?token=${encodeURIComponent(token)}`; | ||
| console.log("[ORCHESTRATOR] Connecting directly to:", wsUrl); |
Contributor
There was a problem hiding this comment.
Avoid logging WebSocket URL containing auth token.
Line 92 logs the full wsUrl which includes the authentication token. This could expose sensitive credentials in browser console logs or log aggregation systems.
Proposed fix
- wsUrl = `${wsInfo.ws_base}/ws?token=${encodeURIComponent(token)}`;
- console.log("[ORCHESTRATOR] Connecting directly to:", wsUrl);
+ wsUrl = `${wsInfo.ws_base}/ws?token=${encodeURIComponent(token)}`;
+ console.log("[ORCHESTRATOR] Connecting directly to:", wsInfo.ws_base);📝 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.
Suggested change
| wsUrl = `${wsInfo.ws_base}/ws?token=${encodeURIComponent(token)}`; | |
| console.log("[ORCHESTRATOR] Connecting directly to:", wsUrl); | |
| wsUrl = `${wsInfo.ws_base}/ws?token=${encodeURIComponent(token)}`; | |
| console.log("[ORCHESTRATOR] Connecting directly to:", wsInfo.ws_base); |
🤖 Prompt for AI Agents
In `@src/utils/websocket.js` around lines 91 - 92, The console.log prints the full
wsUrl including the auth token—remove or redact the token before logging: in
websocket.js where wsUrl is built (wsUrl =
`${wsInfo.ws_base}/ws?token=${encodeURIComponent(token)}`) replace the debug log
that references wsUrl with either no logging of the token (log only
wsInfo.ws_base or "Connecting directly to WebSocket (token redacted)") or build
a redactedUrl that omits or masks the token and log that instead; update the
console.log call accordingly (touch the lines that construct wsUrl, reference
token/encodeURIComponent, and the console.log).
Comment on lines
129
to
136
| websocket.onclose = () => { | ||
| setIsConnected(false); | ||
| setWs(null); | ||
|
|
||
| // Attempt to reconnect after 3 seconds | ||
| reconnectTimeoutRef.current = setTimeout(() => { | ||
| connect(); | ||
| }, 3000); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files src/utils/ | head -20Repository: siteboon/claudecodeui
Length of output: 125
🏁 Script executed:
wc -l src/utils/websocket.jsRepository: siteboon/claudecodeui
Length of output: 91
🏁 Script executed:
cat -n src/utils/websocket.js | sed -n '1,50p'Repository: siteboon/claudecodeui
Length of output: 2082
🏁 Script executed:
cat -n src/utils/websocket.js | sed -n '40,110p'Repository: siteboon/claudecodeui
Length of output: 2805
🏁 Script executed:
cat -n src/utils/websocket.js | sed -n '120,145p'Repository: siteboon/claudecodeui
Length of output: 952
🏁 Script executed:
rg -n "reconnect" --type=js src/utils/websocket.jsRepository: siteboon/claudecodeui
Length of output: 321
🏁 Script executed:
cat -n src/utils/websocket.js | sed -n '110,161p'Repository: siteboon/claudecodeui
Length of output: 1636
🏁 Script executed:
cat -n src/utils/websocket.jsRepository: siteboon/claudecodeui
Length of output: 5895
Inconsistent reconnection behavior: setup failures don't retry, but connection closes do.
In orchestrator proxy mode (lines 54–98) and OSS mode (lines 102–109), configuration/validation failures return early without scheduling reconnection, whereas the onclose handler (lines 129–136) retries after 3 seconds. This creates an asymmetry where transient network errors or temporary API unavailability during connection setup would leave the app permanently disconnected. Consider whether failures to fetch WebSocket info, missing tokens, or malformed configs warrant retry logic (with backoff), or if they should remain fatal as permanent misconfiguration.
🤖 Prompt for AI Agents
In `@src/utils/websocket.js` around lines 129 - 136, The setup path returns early
on configuration/validation failures (orchestrator proxy and OSS flows) without
scheduling a reconnect, causing asymmetrical behavior vs the websocket.onclose
handler which uses reconnectTimeoutRef.current to call connect() after 3s;
update the setup error branches to schedule retries instead of returning
immediately by reusing the same reconnect mechanism (e.g., set
reconnectTimeoutRef.current to call connect()), and add an exponential/backoff
delay tracker (retryDelayRef or similar) that increments on consecutive failures
and resets on success so both setup failures and onclose follow the same
retry/backoff strategy.
Session work based on: Look in @~/repos/liamhelmer/ai-orchestrator/CLAUDE.md . I'm building an orchestrator that will act as a central server where instances of claudecodeui can connect to. We need to create a mode of claud
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@server/routes/cli-auth.js`:
- Around line 114-179: The checkMacOSKeychain() function currently spawns the
"security" process without a timeout; add the same 5000ms watchdog pattern used
in checkCursorStatus: create a timer (5000ms) after spawn that calls
childProcess.kill() (or childProcess.kill('SIGTERM')) and resolves with {
authenticated: false, email: null, error: "Keychain timeout" } if fired, clear
that timer in childProcess.on("close") and childProcess.on("error"), and ensure
you remove/ignore further stdout/stderr handlers after resolution so the promise
never hangs; reference the checkMacOSKeychain function, the childProcess
variable, and the stdout/stderr handling when implementing this.
♻️ Duplicate comments (1)
server/routes/cli-auth.js (1)
14-18: Use the actual credential method in the response.Line 17 hardcodes
"credentials_file", so keychain-sourced auth is misreported. UsecredentialsResult.methodwith a fallback.🛠️ Proposed fix
- method: "credentials_file", + method: credentialsResult.method || "credentials_file",
Comment on lines
+114
to
+179
| // Check macOS Keychain for Claude credentials | ||
| function checkMacOSKeychain() { | ||
| return new Promise((resolve) => { | ||
| const username = os.userInfo().username; | ||
|
|
||
| const childProcess = spawn("security", [ | ||
| "find-generic-password", | ||
| "-a", | ||
| username, | ||
| "-s", | ||
| "Claude Code-credentials", | ||
| "-w", // Output only the password | ||
| ]); | ||
|
|
||
| let stdout = ""; | ||
| let stderr = ""; | ||
|
|
||
| childProcess.stdout.on("data", (data) => { | ||
| stdout += data.toString(); | ||
| }); | ||
|
|
||
| childProcess.stderr.on("data", (data) => { | ||
| stderr += data.toString(); | ||
| }); | ||
|
|
||
| childProcess.on("close", (code) => { | ||
| if (code === 0 && stdout.trim()) { | ||
| // Found a credential - try to parse it as JSON | ||
| try { | ||
| const creds = JSON.parse(stdout.trim()); | ||
|
|
||
| // Check for expiry timestamp | ||
| if (creds.expiresAt && Date.now() >= creds.expiresAt) { | ||
| resolve({ | ||
| authenticated: false, | ||
| email: null, | ||
| error: "Credentials expired", | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| // Extract email/plan info if available | ||
| resolve({ | ||
| authenticated: true, | ||
| email: creds.email || creds.accountEmail || "Keychain Auth", | ||
| plan: creds.plan || null, | ||
| method: "macos_keychain", | ||
| }); | ||
| } catch { | ||
| // Not JSON - might still be valid if it's just a token string | ||
| resolve({ | ||
| authenticated: true, | ||
| email: "Keychain Auth", | ||
| method: "macos_keychain", | ||
| }); | ||
| } | ||
| } else { | ||
| resolve({ authenticated: false, email: null }); | ||
| } | ||
| }); | ||
|
|
||
| childProcess.on("error", () => { | ||
| resolve({ authenticated: false, email: null }); | ||
| }); | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, verify the file exists and check the function
git ls-files | grep -i cli-authRepository: siteboon/claudecodeui
Length of output: 90
🏁 Script executed:
# Get the line count to verify line range
wc -l server/routes/cli-auth.jsRepository: siteboon/claudecodeui
Length of output: 94
🏁 Script executed:
# Read the specific lines to verify the code matches the snippet
sed -n '114,179p' server/routes/cli-auth.jsRepository: siteboon/claudecodeui
Length of output: 1835
🏁 Script executed:
# Search for "Cursor check" reference mentioned in the review
rg -i "cursor.*check|checkCursor" server/routes/cli-auth.js -A 10 -B 2Repository: siteboon/claudecodeui
Length of output: 727
🏁 Script executed:
# Get the full checkCursorStatus implementation
rg -A 50 "function checkCursorStatus" server/routes/cli-auth.jsRepository: siteboon/claudecodeui
Length of output: 1304
🏁 Script executed:
# Search for all spawn calls in the file to see if others have timeouts
rg "spawn\(" server/routes/cli-auth.js -B 5 -A 20Repository: siteboon/claudecodeui
Length of output: 1245
🏁 Script executed:
# Check if there are any other process-spawning functions
rg "new Promise.*spawn|spawn.*\[" server/routes/cli-auth.js -B 3 -A 15Repository: siteboon/claudecodeui
Length of output: 892
Add a timeout to the macOS Keychain security call to prevent hanging requests.
The checkMacOSKeychain() function spawns a system process without timeout protection. If Keychain access is blocked, denied, or locked, the process will hang indefinitely since the security command can block on user interaction. The checkCursorStatus() function already implements this pattern with a 5000ms timeout and process termination—apply the same approach here.
The proposed fix is correct and follows the existing pattern in the codebase.
🤖 Prompt for AI Agents
In `@server/routes/cli-auth.js` around lines 114 - 179, The checkMacOSKeychain()
function currently spawns the "security" process without a timeout; add the same
5000ms watchdog pattern used in checkCursorStatus: create a timer (5000ms) after
spawn that calls childProcess.kill() (or childProcess.kill('SIGTERM')) and
resolves with { authenticated: false, email: null, error: "Keychain timeout" }
if fired, clear that timer in childProcess.on("close") and
childProcess.on("error"), and ensure you remove/ignore further stdout/stderr
handlers after resolution so the promise never hangs; reference the
checkMacOSKeychain function, the childProcess variable, and the stdout/stderr
handling when implementing this.
…cket proxy Adds orchestrator client mode functionality enabling claudecodeui instances to connect as clients to a central orchestrator server. Implements GitHub OAuth authentication for client identity verification and establishes WebSocket proxy connections for routing user traffic and application data between clients and the orchestrator, enabling centralized management of multiple claudecodeui instances.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@server/routes/mcp.js`:
- Around line 481-488: The server object currently exposes raw: config which may
leak sensitive headers/env values; remove the raw property or replace it with a
sanitized/redacted copy before returning to clients. Update the code that
constructs the server object (the server literal with
id/name/type/scope/config/raw) to either omit raw entirely or call a sanitizer
(e.g., sanitizeMCPConfig(config)) that strips or masks keys like Authorization,
API_KEY, TOKEN, SECRET, PASSWORD, ENV, headers, and other high-risk fields;
apply the same change to the other occurrence referenced in the comment (the
second server construction around lines 523-530) so no raw configs are returned.
Ensure the sanitizer returns a shallow copy to avoid mutating the original
config and keep non-sensitive metadata in config if needed.
- Line 120: The console.log calls that print cliArgs (and similar jsonString)
may expose secrets; update the logging in server/routes/mcp.js (references:
cliArgs, jsonString and the console.log statements around the Claude CLI command
and the other occurrences at the noted ranges) to either (a) gate these logs
behind a debug flag (e.g., only log when process.env.DEBUG_CLI or a configured
logger.debug is enabled) or (b) sanitize/redact sensitive tokens before logging
by mapping cliArgs/jsonString to a redacted version (replace values that look
like Authorization headers, Bearer tokens, API keys, or env tokens with
"[REDACTED]" or mask all but last 4 chars). Apply the same change to the other
console.log sites (around lines 227-236 and 614) so no full secrets are ever
emitted. Ensure the redaction/gate logic is reused (extract helper function like
redactArgs/redactString) to avoid duplication.
- Around line 300-305: The code in the name parsing logic (using
name.includes(":") and splitting into [prefix, serverName] to set actualName and
actualScope) incorrectly truncates legitimate names that contain colons; change
it to only treat the left-most segment as a scope when that segment matches a
whitelist of known scopes (e.g., KNOWN_SCOPES array) — if prefix is in that list
then set actualName = serverName and actualScope = actualScope || prefix,
otherwise preserve the full name (actualName = name) and do not modify
actualScope; update the block around the name parsing (the variables name,
actualName, actualScope) to implement this conditional check.
| console.log('🔧 Running Claude CLI command:', 'claude', cliArgs.join(' ')); | ||
|
|
||
| console.log("🔧 Running Claude CLI command:", "claude", cliArgs.join(" ")); |
Contributor
There was a problem hiding this comment.
Redact sensitive values in CLI logging.
cliArgs and jsonString can include headers/env tokens, and parsed server logs include full commands. This risks leaking secrets into logs. Please redact or gate behind a debug flag.
🔒 Proposed redaction of sensitive CLI/config logging
- console.log("🔧 Running Claude CLI command:", "claude", cliArgs.join(" "));
+ const redactedCliArgs = cliArgs.map((arg, i, arr) =>
+ arr[i - 1] === "--header" || arr[i - 1] === "-e" ? "[REDACTED]" : arg,
+ );
+ console.log(
+ "🔧 Running Claude CLI command:",
+ "claude",
+ redactedCliArgs.join(" "),
+ );
@@
- console.log(
- "🔧 Running Claude CLI command:",
- "claude",
- cliArgs[0],
- cliArgs[1],
- cliArgs[2],
- cliArgs[3],
- cliArgs[4],
- jsonString,
- );
+ console.log(
+ "🔧 Running Claude CLI command:",
+ "claude",
+ cliArgs.slice(0, 5).join(" "),
+ "[config redacted]",
+ );
@@
- console.log("🔍 Parsed Claude CLI servers:", servers);
+ console.log(
+ "🔍 Parsed Claude CLI servers:",
+ servers.map(({ id, type, status }) => ({ id, type, status })),
+ );Also applies to: 227-236, 614-614
🤖 Prompt for AI Agents
In `@server/routes/mcp.js` at line 120, The console.log calls that print cliArgs
(and similar jsonString) may expose secrets; update the logging in
server/routes/mcp.js (references: cliArgs, jsonString and the console.log
statements around the Claude CLI command and the other occurrences at the noted
ranges) to either (a) gate these logs behind a debug flag (e.g., only log when
process.env.DEBUG_CLI or a configured logger.debug is enabled) or (b)
sanitize/redact sensitive tokens before logging by mapping cliArgs/jsonString to
a redacted version (replace values that look like Authorization headers, Bearer
tokens, API keys, or env tokens with "[REDACTED]" or mask all but last 4 chars).
Apply the same change to the other console.log sites (around lines 227-236 and
614) so no full secrets are ever emitted. Ensure the redaction/gate logic is
reused (extract helper function like redactArgs/redactString) to avoid
duplication.
Comment on lines
300
to
305
| // If the name includes a scope prefix like "local:test", extract it | ||
| if (name.includes(':')) { | ||
| const [prefix, serverName] = name.split(':'); | ||
| if (name.includes(":")) { | ||
| const [prefix, serverName] = name.split(":"); | ||
| actualName = serverName; | ||
| actualScope = actualScope || prefix; // Use prefix as scope if not provided in query | ||
| } |
Contributor
There was a problem hiding this comment.
Preserve server names that contain colons.
Splitting on : assumes a scope prefix and will truncate names like plugin:context7:context7, making removal fail. Only treat the prefix as a scope when it matches known scopes.
🐛 Preserve colon-containing names
- if (name.includes(":")) {
- const [prefix, serverName] = name.split(":");
- actualName = serverName;
- actualScope = actualScope || prefix; // Use prefix as scope if not provided in query
- }
+ if (name.includes(":")) {
+ const [prefix, ...rest] = name.split(":");
+ if (prefix === "local" || prefix === "user") {
+ actualName = rest.join(":");
+ actualScope = actualScope || prefix; // Use prefix as scope if not provided in query
+ }
+ }📝 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.
Suggested change
| // If the name includes a scope prefix like "local:test", extract it | |
| if (name.includes(':')) { | |
| const [prefix, serverName] = name.split(':'); | |
| if (name.includes(":")) { | |
| const [prefix, serverName] = name.split(":"); | |
| actualName = serverName; | |
| actualScope = actualScope || prefix; // Use prefix as scope if not provided in query | |
| } | |
| // If the name includes a scope prefix like "local:test", extract it | |
| if (name.includes(":")) { | |
| const [prefix, ...rest] = name.split(":"); | |
| if (prefix === "local" || prefix === "user") { | |
| actualName = rest.join(":"); | |
| actualScope = actualScope || prefix; // Use prefix as scope if not provided in query | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@server/routes/mcp.js` around lines 300 - 305, The code in the name parsing
logic (using name.includes(":") and splitting into [prefix, serverName] to set
actualName and actualScope) incorrectly truncates legitimate names that contain
colons; change it to only treat the left-most segment as a scope when that
segment matches a whitelist of known scopes (e.g., KNOWN_SCOPES array) — if
prefix is in that list then set actualName = serverName and actualScope =
actualScope || prefix, otherwise preserve the full name (actualName = name) and
do not modify actualScope; update the block around the name parsing (the
variables name, actualName, actualScope) to implement this conditional check.
Comment on lines
481
to
488
| const server = { | ||
| id: name, | ||
| name: name, | ||
| type: 'stdio', // Default type | ||
| scope: 'user', // User scope - available across all projects | ||
| type: "stdio", // Default type | ||
| scope: "user", // User scope - available across all projects | ||
| config: {}, | ||
| raw: config // Include raw config for full details | ||
| raw: config, // Include raw config for full details | ||
| }; |
Contributor
There was a problem hiding this comment.
Avoid returning raw MCP config in API responses.
raw: config can include sensitive headers/env values. Consider omitting or redacting before returning to clients.
🔒 Proposed removal of raw config fields
- raw: config, // Include raw config for full details
@@
- raw: config, // Include raw config for full detailsAlso applies to: 523-530
🤖 Prompt for AI Agents
In `@server/routes/mcp.js` around lines 481 - 488, The server object currently
exposes raw: config which may leak sensitive headers/env values; remove the raw
property or replace it with a sanitized/redacted copy before returning to
clients. Update the code that constructs the server object (the server literal
with id/name/type/scope/config/raw) to either omit raw entirely or call a
sanitizer (e.g., sanitizeMCPConfig(config)) that strips or masks keys like
Authorization, API_KEY, TOKEN, SECRET, PASSWORD, ENV, headers, and other
high-risk fields; apply the same change to the other occurrence referenced in
the comment (the second server construction around lines 523-530) so no raw
configs are returned. Ensure the sanitizer returns a shallow copy to avoid
mutating the original config and keep non-sensitive metadata in config if
needed.
…cket proxy Add support for claudecodeui instances to connect to a central orchestrator server. Implements websocket connection to orchestrator with GitHub OAuth authentication, allowing instances to proxy user traffic back to the orchestrator while maintaining secure authentication. This enables orchestrator instances to manage and coordinate multiple claudecodeui deployments.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@server/orchestrator/client.js`:
- Around line 159-162: The close handler currently calls reject when
!wasConnected which can double-reject if the timeout or onError already
rejected; remove the duplicate rejection in the WebSocket "close" event (or
guard it with a settled flag) so only the timeout or the onError listener
performs rejection; look for the wasConnected variable, the reject callback
passed into the Promise, the WebSocket "close" event handler, and the existing
onError/timeout rejection path and eliminate the redundant reject call.
♻️ Duplicate comments (3)
server/orchestrator/client.js (3)
218-229: Information disclosure:process.cwd()sent to orchestrator.The
projectfield sends the full local filesystem path to the orchestrator server, which could reveal sensitive directory structure information (e.g.,/home/username/...). This was flagged in a previous review.Consider using
path.basename(process.cwd())or making this opt-in via configuration.
825-862: SameproxyBasesanitization issue applies here.The
proxyBaseparameter is interpolated into regex replacement strings without validation. Apply the same validation suggested forrewriteHtmlUrls, or extract validation to a shared helper called once inhandleHttpProxyRequestbefore passing to either rewrite function.
689-717:proxyBasenot sanitized before HTML/JS interpolation.The
proxyBaseparameter from the orchestrator is interpolated directly into HTML attributes (lines 700-705) and JavaScript strings (line 739) without validation. A maliciousproxyBasecould inject HTML or break out of JS string context.While past reviews flagged this, it remains unaddressed. Validate
proxyBaseagainst a strict pattern at the entry point.🔒 Suggested fix - add validation at the start of rewriteHtmlUrls
rewriteHtmlUrls( html, proxyBase, orchestratorToken = null, orchestratorUsername = null, ) { + // Validate proxyBase to prevent injection attacks + // Only allow safe path characters: alphanumerics, /, -, _, . + if (typeof proxyBase !== "string" || !/^\/[A-Za-z0-9/_\-.]*$/.test(proxyBase)) { + console.warn("[ORCHESTRATOR] Invalid proxyBase, skipping URL rewrite:", proxyBase); + return html; + } + // Add a cache-busting version to force fresh fetches from Cloudflare edge
Comment on lines
+159
to
+162
| if (!wasConnected) { | ||
| reject(new Error(`Connection failed: ${code}`)); | ||
| } | ||
| }); |
Contributor
There was a problem hiding this comment.
Potential double rejection after timeout.
If the connection times out (line 124 rejects), then the WebSocket close event fires and !wasConnected is true, causing a second rejection. This can lead to an unhandled promise rejection warning.
🔧 Suggested fix
this.ws.on("close", (code, reason) => {
clearTimeout(connectTimeout);
const wasConnected = this.isConnected;
this.isConnected = false;
this.isRegistered = false;
this.stopHeartbeat();
console.log(
`[ORCHESTRATOR] Connection closed: ${code} ${reason || ""}`,
);
this.emit("disconnected", { code, reason: reason?.toString() });
if (this.shouldReconnect) {
this.scheduleReconnect();
}
-
- if (!wasConnected) {
- reject(new Error(`Connection failed: ${code}`));
- }
});The rejection is already handled by the onError listener or the timeout. Removing this duplicate rejection path simplifies the flow.
🤖 Prompt for AI Agents
In `@server/orchestrator/client.js` around lines 159 - 162, The close handler
currently calls reject when !wasConnected which can double-reject if the timeout
or onError already rejected; remove the duplicate rejection in the WebSocket
"close" event (or guard it with a settled flag) so only the timeout or the
onError listener performs rejection; look for the wasConnected variable, the
reject callback passed into the Promise, the WebSocket "close" event handler,
and the existing onError/timeout rejection path and eliminate the redundant
reject call.
Collaborator
|
@liamhelmer If you have done any testing, can you list them in detail? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR adds orchestrator client mode to Claude Code UI, enabling instances to connect to a central orchestrator server (Duratii) for remote access and management. Users can now access their Claude Code UI instances from anywhere through a unified web interface, with automatic authentication pass-through and real-time status reporting.
Motivation
When running Claude Code UI on multiple machines (development servers, remote workstations, etc.), users need a way to:
Key Features
Architecture
Files Changed
New Files:
server/orchestrator/index.jsinitializeOrchestrator()factory functionclient.jsOrchestratorClientclass - WebSocket client with auto-reconnect, heartbeats, HTTP proxy handling, URL rewritingprotocol.jsgithub-auth.jsstatus-tracker.jsproxy.jsModified Files
server/cli.jsserver/middleware/auth.jsserver/database/db.jsgetOrCreateOrchestratorUser()for auto-creating users from orchestrator OAuth with github_id supportserver/database/init.sqlpublic/sw.jsproxyBasequery parameter support for PWA through proxy; URL normalization for cache keyssrc/App.jsxbasenamedetection for proxy access; fixed mediaQuery listener leaks.env.exampleNew Documentation
docs/Orchestrator-Client.mdConfiguration
Environment Variables
Technical Details
HTTP Proxy URL Rewriting
When requests come through the orchestrator proxy (
/clients/{id}/proxy/*), the client performs URL rewriting to ensure the React app works correctly:HTML rewriting (
rewriteHtmlUrls):src="/...",href="/...",action="/..."attributesfetch()patch script to redirect API calls through proxyproxyBasequery parameterJavaScript rewriting (
rewriteJsUrls):Binary content handling:
x-orch-encodingheaderService Worker (
sw.js):proxyBaseparameter to normalize cache keysAuto-Authentication Flow
X-Orchestrator-User-IdandX-Orchestrator-UsernameheadersOrchestratorClient.handleHttpProxyRequest()callsgetOrCreateOrchestratorToken()<script>that stores token in localStorageStatus Tracking
The
StatusTrackermonitors:Status values:
idle: No active connectionsactive: Browser connected, not generatingbusy: Claude is generating a responseMessage Protocol
Outbound (claudecodeui → Orchestrator):
register: Initial registration with metadatastatus_update: Status changes (idle/active/busy)ping: Heartbeathttp_proxy_response: Response to proxied HTTP requestInbound (Orchestrator → claudecodeui):
registered: Registration confirmationpong: Heartbeat responsecommand: Commands (disconnect, refresh_status)http_proxy_request: Proxied HTTP requestSecurity Improvements
AUTH_DEBUGenvironment variablegithub_idto prevent username collision attacksTest Plan
ORCHESTRATOR_MODE=clientwith URL and tokenBreaking Changes
None - orchestrator mode is opt-in via environment variables.
Migration Guide
Existing installations continue to work in standalone mode. To enable orchestrator mode:
Related
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes / Improvements
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.
Prompt History
📝 Prompt - 2026-01-20 22:48:22 UTC
📝 Prompt - 2026-01-20 22:49:41 UTC
📝 Prompt - 2026-01-20 22:53:16 UTC
📝 Prompt - 2026-01-20 22:57:38 UTC
📝 Prompt - 2026-01-21 03:07:41 UTC
📝 Prompt - 2026-01-21 06:50:50 UTC
📝 Prompt - 2026-01-21 07:05:55 UTC
📝 Prompt - 2026-01-21 18:45:15 UTC