Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7d1b65ce95
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7ca123cd6c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ff4d79461e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Extract auth-token resolution into a pure helper with deterministic generated-token testing coverage for CLI/env precedence and no-auth behavior.
Route server startup/shutdown through ServerService startServer/stopServer, enable secure-by-default auth token resolution with --no-auth and --print-auth-token options, and refresh startup output with connection helpers. Also update mux server --help assertions for the new flags.
Address Codex review: check for an existing server BEFORE serviceContainer.initialize() to prevent orphaned task resumptions. ServerService.startServer() still re-checks as defense-in-depth.
- Print networkBaseUrls[0] instead of baseUrl in 'Connect from another machine' instructions, since baseUrl is loopback for wildcard binds. - Shell-quote the auth token to handle metacharacters. - URL-encode the token in the browser query string.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 42c6a33e83
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…rverService (#2425) ## Summary Make `mux server` **secure-by-default**: when no auth token is provided via `--auth-token` or `MUX_SERVER_AUTH_TOKEN`, the CLI now auto-generates a random 32-byte hex token. This matches the desktop app's existing behavior (`randomBytes(32).toString("hex")`). Simultaneously consolidates duplicated server lifecycle logic by delegating to `ServerService.startServer()` / `stopServer()`, eliminating ~70 lines of manual lockfile, mDNS, and `createOrpcServer()` wiring. ## Background Previously, `mux server` with no flags ran **unauthenticated** — anyone who could reach the network interface had full API access. The desktop app was already secure-by-default, but the CLI server was not. The CLI also duplicated server startup responsibilities (lockfile check/acquire, `createOrpcServer()` call, mDNS advertising, `isLoopbackHost()`) that `ServerService` already handles, creating maintenance burden. ## Implementation ### New: `resolveServerAuthToken()` helper (`src/cli/serverAuthToken.ts`) Pure function with clear precedence: `--no-auth` > `--auth-token` > `MUX_SERVER_AUTH_TOKEN` > auto-generated. Returns a discriminated union (`ResolvedAuthToken`) with mode + source metadata. ### Consolidated `src/cli/server.ts` - **Removed:** direct `ServerLockfile`, `createOrpcServer`, `MdnsAdvertiserService`, `isLoopbackHost()` usage - **Added:** single `serverService.startServer()` call + `serverService.stopServer()` for cleanup - **New flags:** `--no-auth` (explicit insecure escape hatch) and `--print-auth-token` - **Startup output:** rich banner with base URL, LAN URLs, docs URL, auth status, copy-paste env vars, browser URL with token, lockfile path ### `ServerService.getLockfilePath()` accessor Small addition so the CLI can print where the lockfile lives without re-instantiating `ServerLockfile`. ## Validation - `make typecheck` ✅ - `bun test src/cli/serverAuthToken.test.ts` — 9/9 ✅ - `bun test src/cli/run.test.ts` — 17/17 ✅ - `make static-check` ✅ --- <details> <summary>📋 Implementation Plan</summary> # Plan: Secure-by-default `mux server` + consolidate startup via `ServerService` ## Context / Why The standalone CLI server (`mux server`) can currently run **unauthenticated** when no token is provided. That’s unsafe by default. Separately, `src/cli/server.ts` duplicates server-startup responsibilities that already exist in `ServerService` (lockfile discovery/acquire, network base URL computation, mDNS advertising, stop/cleanup). Consolidating onto `ServerService.startServer()` / `stopServer()` reduces long-term maintenance and keeps the desktop + server codepaths consistent. ## Evidence (repo) - Unauthenticated-by-default today: - `src/cli/server.ts` derives `AUTH_TOKEN` from `--auth-token` / `MUX_SERVER_AUTH_TOKEN`, and converts blank/missing values to `undefined`. - `src/node/orpc/authMiddleware.ts` bypasses auth when token is missing/blank: `if (!authToken?.trim()) return passThrough`. - Desktop is already secure-by-default: - `src/desktop/main.ts`: `process.env.MUX_SERVER_AUTH_TOKEN ?? randomBytes(32).toString('hex')`. - Consolidation target exists: - `src/node/services/serverService.ts` already implements `startServer()` / `stopServer()` with lockfile, mDNS, and `computeNetworkBaseUrls()`. <details> <summary>What `src/cli/server.ts` duplicates today (high-level)</summary> - Checking for an existing server via `ServerLockfile` - Calling `createOrpcServer()` directly - Manually acquiring/releasing `server.lock` - Manually starting/stopping `MdnsAdvertiserService` - Maintaining local loopback detection logic `ServerService.startServer()` / `stopServer()` already handle these. </details> ## Recommended approach: Refactor `mux server` to use `ServerService.startServer()` and always provide an auth token **Net LoC (product code only): ~-20 LoC** (delete duplicated lockfile+mDNS code; add token resolution + a couple flags) ### Acceptance criteria 1. `mux server` with no flags/env vars generates a random auth token and requires it for HTTP/WS API. 2. `mux server --auth-token …` and `MUX_SERVER_AUTH_TOKEN=… mux server` still work. 3. Provide an explicit insecure escape hatch: `mux server --no-auth` (disables auth; writes empty token to lockfile). 4. Startup output makes it easy to connect (baseUrl, docs/UI URLs, where to find/copy the token). 5. `src/cli/server.ts` no longer contains duplicated lockfile + mDNS logic. ### Implementation details (ordered) #### 1) Extract token resolution into a tiny helper (unit-testable) Files: - `src/cli/serverAuthToken.ts` (new) - `src/cli/serverAuthToken.test.ts` (new) Suggested API: ```ts import { randomBytes } from 'crypto'; export type ResolvedAuthToken = | { mode: 'disabled'; token: '' } | { mode: 'enabled'; token: string; source: 'cli' | 'env' | 'generated' }; export function resolveServerAuthToken(opts: { noAuth: boolean; cliToken: string | undefined; envToken: string | undefined; randomBytesFn?: (n: number) => Buffer; }): ResolvedAuthToken { // precedence: --no-auth > cli token > env token > generated // invariant: when mode==='enabled', token must be non-empty after trim } ``` Key test cases: - `noAuth=true` → `{ mode: 'disabled', token: '' }` - `cliToken=' abc '` → `{ mode: 'enabled', token: 'abc', source: 'cli' }` - env token used when CLI token absent - neither provided → deterministic generated token (inject `randomBytesFn`) #### 2) Consolidate server startup/shutdown in `src/cli/server.ts` **File:** `src/cli/server.ts` - Commander flags: - Keep `--auth-token <token>`, but update help text (no longer “optional”; default is generated) - Add `--no-auth` - Add `--print-auth-token` (optional; forces printing even on loopback) - Replace the manual wiring: - `ServerLockfile` pre-check + `lockfile.acquire(...)` - direct `createOrpcServer(...)` - manual `MdnsAdvertiserService` handling - local loopback detection helper with a single call: ```ts const resolved = resolveServerAuthToken({ noAuth: options.noAuth === true, cliToken: options.authToken as string | undefined, envToken: process.env.MUX_SERVER_AUTH_TOKEN, }); const info = await serviceContainer.serverService.startServer({ muxHome: serviceContainer.config.rootDir, context: serviceContainer.toORPCContext(), host: HOST, port: PORT, authToken: resolved.token, serveStatic: true, }); ``` - Keep the existing startup keepalive interval, but clear it immediately after `startServer()` resolves. - Cleanup path should use `await serviceContainer.serverService.stopServer()` (then `await serviceContainer.dispose()`). #### 3) Startup output / UX improvements **File:** `src/cli/server.ts` Use `ServerInfo` returned by `startServer()`: - Always print: - `info.baseUrl` - docs URL: `${info.baseUrl}/api/docs` - whether auth is enabled (`info.token.length > 0`) - If auth is enabled: - Print the token only when `--print-auth-token` is set **or** when `info.networkBaseUrls.length > 0` (strong signal user intends LAN/VPN access). - Always print the lockfile path for local retrieval (see next step). - Print copy/paste helpers: - `MUX_SERVER_URL=...` - `MUX_SERVER_AUTH_TOKEN=...` - Web UI URL with token query param: `/?token=...` - If `--no-auth`, print a loud warning that the server is open to anyone who can reach it. #### 4) Avoid re-instantiating `ServerLockfile` just to print the lockfile path (optional consolidation) **File:** `src/node/services/serverService.ts` Add a small accessor: ```ts getLockfilePath(): string | null { return this.lockfile?.getLockPath() ?? null; } ``` Then `src/cli/server.ts` can print the lockfile path via `serviceContainer.serverService.getLockfilePath()`. (If we want zero churn outside CLI, the fallback is to instantiate `new ServerLockfile(serviceContainer.config.rootDir)` purely for printing; but the accessor is cleaner.) #### 5) Update tests - `src/cli/run.test.ts`: update `mux server --help` expectations to include `--no-auth` / `--print-auth-token` and updated `--auth-token` description. - Add `serverAuthToken` unit tests (fast; no real server process). #### 6) (Optional) Clarify the browser auth prompt **File:** `src/browser/components/AuthTokenModal.tsx` Tweak the description to mention the token comes from startup output or the `server.lock` file. ### Validation - `make test` - `make typecheck` ## Alternative (smaller churn, more duplication): secure-by-default without consolidation **Net LoC (product code only): ~+40 LoC** Only change `src/cli/server.ts` to generate a token when missing, and keep the existing direct `createOrpcServer()` + lockfile + mDNS logic. This is lower refactor risk, but keeps two server-start implementations that must be maintained together. </details> --- _Generated with `mux` • Model: `anthropic:claude-opus-4-6` • Thinking: `xhigh` • Cost: `$0.94`_ <!-- mux-attribution: model=anthropic:claude-opus-4-6 thinking=xhigh costs=0.94 -->
Summary
Make
mux serversecure-by-default: when no auth token is provided via--auth-tokenorMUX_SERVER_AUTH_TOKEN, the CLI now auto-generates a random 32-byte hex token. This matches the desktop app's existing behavior (randomBytes(32).toString("hex")).Simultaneously consolidates duplicated server lifecycle logic by delegating to
ServerService.startServer()/stopServer(), eliminating ~70 lines of manual lockfile, mDNS, andcreateOrpcServer()wiring.Background
Previously,
mux serverwith no flags ran unauthenticated — anyone who could reach the network interface had full API access. The desktop app was already secure-by-default, but the CLI server was not.The CLI also duplicated server startup responsibilities (lockfile check/acquire,
createOrpcServer()call, mDNS advertising,isLoopbackHost()) thatServerServicealready handles, creating maintenance burden.Implementation
New:
resolveServerAuthToken()helper (src/cli/serverAuthToken.ts)Pure function with clear precedence:
--no-auth>--auth-token>MUX_SERVER_AUTH_TOKEN> auto-generated. Returns a discriminated union (ResolvedAuthToken) with mode + source metadata.Consolidated
src/cli/server.tsServerLockfile,createOrpcServer,MdnsAdvertiserService,isLoopbackHost()usageserverService.startServer()call +serverService.stopServer()for cleanup--no-auth(explicit insecure escape hatch) and--print-auth-tokenServerService.getLockfilePath()accessorSmall addition so the CLI can print where the lockfile lives without re-instantiating
ServerLockfile.Validation
make typecheck✅bun test src/cli/serverAuthToken.test.ts— 9/9 ✅bun test src/cli/run.test.ts— 17/17 ✅make static-check✅📋 Implementation Plan
Plan: Secure-by-default
mux server+ consolidate startup viaServerServiceContext / Why
The standalone CLI server (
mux server) can currently run unauthenticated when no token is provided. That’s unsafe by default.Separately,
src/cli/server.tsduplicates server-startup responsibilities that already exist inServerService(lockfile discovery/acquire, network base URL computation, mDNS advertising, stop/cleanup). Consolidating ontoServerService.startServer()/stopServer()reduces long-term maintenance and keeps the desktop + server codepaths consistent.Evidence (repo)
src/cli/server.tsderivesAUTH_TOKENfrom--auth-token/MUX_SERVER_AUTH_TOKEN, and converts blank/missing values toundefined.src/node/orpc/authMiddleware.tsbypasses auth when token is missing/blank:if (!authToken?.trim()) return passThrough.src/desktop/main.ts:process.env.MUX_SERVER_AUTH_TOKEN ?? randomBytes(32).toString('hex').src/node/services/serverService.tsalready implementsstartServer()/stopServer()with lockfile, mDNS, andcomputeNetworkBaseUrls().What `src/cli/server.ts` duplicates today (high-level)
ServerLockfilecreateOrpcServer()directlyserver.lockMdnsAdvertiserServiceServerService.startServer()/stopServer()already handle these.Recommended approach: Refactor
mux serverto useServerService.startServer()and always provide an auth tokenNet LoC (product code only): ~-20 LoC (delete duplicated lockfile+mDNS code; add token resolution + a couple flags)
Acceptance criteria
mux serverwith no flags/env vars generates a random auth token and requires it for HTTP/WS API.mux server --auth-token …andMUX_SERVER_AUTH_TOKEN=… mux serverstill work.mux server --no-auth(disables auth; writes empty token to lockfile).src/cli/server.tsno longer contains duplicated lockfile + mDNS logic.Implementation details (ordered)
1) Extract token resolution into a tiny helper (unit-testable)
Files:
src/cli/serverAuthToken.ts(new)src/cli/serverAuthToken.test.ts(new)Suggested API:
Key test cases:
noAuth=true→{ mode: 'disabled', token: '' }cliToken=' abc '→{ mode: 'enabled', token: 'abc', source: 'cli' }randomBytesFn)2) Consolidate server startup/shutdown in
src/cli/server.tsFile:
src/cli/server.tsCommander flags:
--auth-token <token>, but update help text (no longer “optional”; default is generated)--no-auth--print-auth-token(optional; forces printing even on loopback)Replace the manual wiring:
ServerLockfilepre-check +lockfile.acquire(...)createOrpcServer(...)MdnsAdvertiserServicehandlingwith a single call:
startServer()resolves.await serviceContainer.serverService.stopServer()(thenawait serviceContainer.dispose()).3) Startup output / UX improvements
File:
src/cli/server.tsUse
ServerInforeturned bystartServer():info.baseUrl${info.baseUrl}/api/docsinfo.token.length > 0)--print-auth-tokenis set or wheninfo.networkBaseUrls.length > 0(strong signal user intends LAN/VPN access).MUX_SERVER_URL=...MUX_SERVER_AUTH_TOKEN=.../?token=...--no-auth, print a loud warning that the server is open to anyone who can reach it.4) Avoid re-instantiating
ServerLockfilejust to print the lockfile path (optional consolidation)File:
src/node/services/serverService.tsAdd a small accessor:
Then
src/cli/server.tscan print the lockfile path viaserviceContainer.serverService.getLockfilePath().(If we want zero churn outside CLI, the fallback is to instantiate
new ServerLockfile(serviceContainer.config.rootDir)purely for printing; but the accessor is cleaner.)5) Update tests
src/cli/run.test.ts: updatemux server --helpexpectations to include--no-auth/--print-auth-tokenand updated--auth-tokendescription.serverAuthTokenunit tests (fast; no real server process).6) (Optional) Clarify the browser auth prompt
File:
src/browser/components/AuthTokenModal.tsxTweak the description to mention the token comes from startup output or the
server.lockfile.Validation
make testmake typecheckAlternative (smaller churn, more duplication): secure-by-default without consolidation
Net LoC (product code only): ~+40 LoC
Only change
src/cli/server.tsto generate a token when missing, and keep the existing directcreateOrpcServer()+ lockfile + mDNS logic. This is lower refactor risk, but keeps two server-start implementations that must be maintained together.Generated with
mux• Model:anthropic:claude-opus-4-6• Thinking:xhigh• Cost:$0.94