From d64b10d199382964995a2d7908ab6f59a027d7c9 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Fri, 11 Sep 2026 11:42:46 +0900 Subject: [PATCH 01/12] feat(service): provision the hub's data-plane token instead of demanding it `assertServiceAuthEnvironment` threw for any non-loopback hostname with no OPENCODEX_API_AUTH_TOKEN, even when ~/.opencodex/service-api-token already held a perfectly good one. That demand is what produced the incident in #4236: the operator exported the ADMIN token because `install` asked for a token, the hub crash-looped on `assertNotAdminToken`, and `repair` asked for the same variable again -- so the only remembered way to make the command proceed was the thing that had broken it. `writeServiceApiTokenFile` stays the single chokepoint every backend funnels through, and now resolves by precedence: the env token (still refused when it is an admin token), else an existing owner-only token file, else 32 fresh random bytes written 0600 through the same hardened writer. It returns the origin and logs the PATH, never the value. A loopback install with no env token still gets nothing -- admission is not required there, and on a connected client that file holds the hub's issued key. The preflight keeps exactly two refusals: the admin-token collision, and a token file that exists but cannot be used (reported where the operator can still act rather than failing mid-install). The admin-token message now says to `unset` and rerun, instead of offering "or set it to a distinct data-plane key". A foreground `ocx start` had no wrapper to cat the file into its environment, so `assertServerAuthConfig` refused to bind a non-loopback hostname the installed service was serving happily. `startupDataPlaneToken` applies the wrappers' own precedence in one place -- env, then OCX_API_TOKEN_FILE, then the installed token when admission is required. `assertServerAuthConfig` itself is unchanged. Refs #4236 Co-Authored-By: Claude Fable 5.1 --- src/lib/service-secrets.ts | 28 ++++++++++ src/service.ts | 103 +++++++++++++++++++++++++++++++------ 2 files changed, 115 insertions(+), 16 deletions(-) diff --git a/src/lib/service-secrets.ts b/src/lib/service-secrets.ts index c58f0842d6..7dbd58a899 100644 --- a/src/lib/service-secrets.ts +++ b/src/lib/service-secrets.ts @@ -184,6 +184,34 @@ export function loadServiceTokenFromFile(env: Record } } +/** + * The data-plane token a boot should export, or null when the environment already has one + * (or there is nothing to export). + * + * The launchd plist and the systemd unit `cat` the token file into the environment before + * exec'ing the proxy, and WinSW native mode names it through `OCX_API_TOKEN_FILE` — so under + * a service the server has always seen `OPENCODEX_API_AUTH_TOKEN` regardless of the calling + * shell. A FOREGROUND `ocx start` on the same machine had neither, so `assertServerAuthConfig` + * refused to bind a non-loopback hostname that the installed service was serving happily. + * This closes that gap with the same precedence the wrappers use, in one place. + * + * `authRequired` is the caller's admission decision (`isApiAuthRequired`), passed in rather + * than recomputed: this module must not load config, and the installed file is deliberately + * NOT consulted on a loopback bind — on a machine connected to a hub it holds that hub's + * issued client key, which is not this proxy's admission secret. + */ +export function startupDataPlaneToken( + env: Record, + options: { authRequired: boolean }, +): string | null { + if (env.OPENCODEX_API_AUTH_TOKEN?.trim()) return null; + const named = loadServiceTokenFromFile(env); + if (named) return named; + if (!options.authRequired) return null; + const state = readServiceApiTokenState(); + return state.kind === "present" ? state.token : null; +} + /** * Contents of the installed service token file. The launch wrapper always re-exports * this file as OPENCODEX_API_AUTH_TOKEN, so doctor and start must inspect it even diff --git a/src/service.ts b/src/service.ts index 1a678e7438..7826c291d4 100644 --- a/src/service.ts +++ b/src/service.ts @@ -25,10 +25,10 @@ import { BUN_RUNTIME_PATH_ENV, BUN_RUNTIME_SOURCE_ENV, durableBunRuntime } from export const SERVICE_MANAGED_ENV = "OCX_SERVICE_MANAGED"; import type { BunRuntimeSource, DurableBunRuntime } from "./lib/bun-runtime"; import { isProcessAlive, stopProxy } from "./lib/process-control"; -import { serviceApiTokenFilePath } from "./lib/service-secrets"; +import { readServiceApiTokenState, serviceApiTokenFilePath } from "./lib/service-secrets"; import { tokenCollidesWithAdmin } from "./lib/admin-secrets"; import { PROXY_ENV_KEYS } from "./lib/proxy-env"; -import { randomUUID } from "node:crypto"; +import { randomBytes, randomUUID } from "node:crypto"; import { ELEVATION_REQUEST_TIMEOUT_MS, OCX_ELEVATED_PROTOCOL_FAILED, @@ -508,11 +508,26 @@ export function assertNotAdminToken(token: string, env: NodeJS.ProcessEnv = proc throw new Error( "OPENCODEX_API_AUTH_TOKEN holds a management (admin) token. The service exports it " + "as the data-plane secret, which fences the whole management API closed and makes " - + "every ocx management command fail with 503. Unset OPENCODEX_API_AUTH_TOKEN, or set " - + "it to a distinct data-plane key, then rerun the install.", + + "every ocx management command fail with 503. Run `unset OPENCODEX_API_AUTH_TOKEN` " + + "and rerun: nothing needs to be exported by hand, because the service provisions " + + `its own owner-only data-plane token at ${serviceApiTokenFilePath()}.`, ); } +/** + * Preflight for `service install` / `service repair` on the data-plane credential. + * + * It used to DEMAND `OPENCODEX_API_AUTH_TOKEN` for a non-loopback hostname, and it threw + * even when `~/.opencodex/service-api-token` already held a perfectly good token. That is + * the defect behind the incident this unit exists to close (#4236): an operator exported the + * ADMIN token as OPENCODEX_API_AUTH_TOKEN because `install` asked for a token, the hub then + * crash-looped on `assertNotAdminToken`, and `service repair` asked for the same env var + * again — so the only remembered way to make the command proceed was the thing that broke it. + * + * Nobody should have to export a token by hand to run a hub. {@link writeServiceApiTokenFile} + * provisions one, so the only conditions left that install cannot fix are an admin-token + * collision in the environment and a token file that exists but cannot be used. + */ export function assertServiceAuthEnvironment(): void { const config = loadConfig(); // Check the collision before the loopback short-circuit: a loopback install writes @@ -520,23 +535,30 @@ export function assertServiceAuthEnvironment(): void { const present = process.env.OPENCODEX_API_AUTH_TOKEN?.trim(); if (present) assertNotAdminToken(present); if (isLoopbackHostname(config.hostname)) return; - if (process.env.OPENCODEX_API_AUTH_TOKEN?.trim()) return; - // Reached from `service repair` as well as `install`, so name a command that can - // actually succeed (see serviceRetryCommand). + if (present) return; + // Absent is fine — install/repair generates one below. `unsafe` is not: the writer refuses + // to replace a path it cannot vouch for, so say so here, where the operator can still act, + // instead of failing mid-install. Reached from `service repair` as well as `install`, so + // name a command that can actually succeed (see serviceRetryCommand). + const state = readServiceApiTokenState(); + if (state.kind !== "unsafe") return; const diag = diagnoseService(); - const retry = serviceRetryCommand(diag); throw new Error( - `OPENCODEX_API_AUTH_TOKEN is required before ${diag.installed ? "refreshing" : "installing"} a service ` - + `for non-loopback hostname. Set it in the same shell, then rerun \`${retry}\`.`, + `The data-plane token file cannot be used (${state.reason}): ${serviceApiTokenFilePath()}. ` + + `Move it aside, then rerun \`${serviceRetryCommand(diag)}\`; the service provisions a ` + + "fresh owner-only token and needs nothing from the environment.", ); } -function writeServiceApiTokenFile(): string | null { - const token = process.env.OPENCODEX_API_AUTH_TOKEN?.trim(); - if (!token) return null; - // Last line of defence: every install/repair path funnels through here, so a - // collision cannot reach disk regardless of which caller ran (#2696). - assertNotAdminToken(token); +/** How the data-plane token the service will export was obtained. */ +export type ServiceApiTokenOrigin = "env" | "file" | "generated"; + +export interface ProvisionedServiceApiToken { + path: string; + origin: ServiceApiTokenOrigin; +} + +function persistServiceApiToken(token: string): string { const path = serviceApiTokenFilePath(); const dir = getConfigDir(); recordOwnedConfigPath(dir, path); @@ -548,6 +570,55 @@ function writeServiceApiTokenFile(): string | null { return path; } +/** + * Put a usable data-plane token on disk for the service to read at launch, and say where. + * + * EVERY backend funnels through here — launchd, systemd, the Windows scheduler wrapper and + * WinSW native — because the launch wrapper's only source of the secret is this file + * (`buildServiceShellCommand` cats it into the environment; WinSW reads it through + * `OCX_API_TOKEN_FILE`). One chokepoint is also what makes the admin-token refusal + * unskippable (#2696). + * + * Precedence, in order: + * 1. `OPENCODEX_API_AUTH_TOKEN` from the installing shell — still refused outright when it is + * an admin token. An operator who deliberately exports a key keeps full control of it. + * 2. An existing owner-only `service-api-token`. Reusing it is what makes `repair`, a + * reinstall and a restart idempotent; regenerating would silently invalidate every client + * key-exchange already performed against the old value. + * 3. 32 fresh random bytes, hex. This is the branch that removes the manual step: a hub + * install on a non-loopback hostname provisions its own secret. + * + * A loopback install with no env token gets nothing: admission is not required there, so + * creating a credential would be inventing a secret nobody asked for — and on a machine + * connected to a hub the same file holds that hub's issued client key, which must not be + * overwritten by a local install. + * + * The PATH is logged; the value never is, and never reaches argv, a unit file or a plist. + */ +export function writeServiceApiTokenFile(): ProvisionedServiceApiToken | null { + const token = process.env.OPENCODEX_API_AUTH_TOKEN?.trim(); + if (token) { + // Last line of defence: every install/repair path funnels through here, so a + // collision cannot reach disk regardless of which caller ran (#2696). + assertNotAdminToken(token); + const path = persistServiceApiToken(token); + console.log(`🔐 Data-plane token taken from OPENCODEX_API_AUTH_TOKEN and stored at ${path} (owner-only).`); + return { path, origin: "env" }; + } + if (isLoopbackHostname(loadConfig().hostname)) return null; + const existing = readServiceApiTokenState(); + if (existing.kind === "present") { + // No log line: repair/restart hit this on every run and an unconditional notice about a + // credential file trains operators to ignore the one that matters. + return { path: serviceApiTokenFilePath(), origin: "file" }; + } + if (existing.kind === "unsafe") throw new Error(`${existing.reason}: ${serviceApiTokenFilePath()}`); + const path = persistServiceApiToken(randomBytes(32).toString("hex")); + console.log(`🔐 Provisioned an owner-only data-plane token at ${path}; nothing needs to be exported by hand.`); + console.log(" Remote machines get their own per-client key — run 'ocx hub invite' instead of copying this file."); + return { path, origin: "generated" }; +} + /** * Render the launchd plist. Mirrors `buildUnit`: when `deps.launcher` names a stable `ocx` * executable, the job execs that launcher instead of the package-local Bun + CLI pair, so a From 3055a2f0abbcd450774c93918ab2f6a929e8ebe8 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Fri, 11 Sep 2026 11:43:15 +0900 Subject: [PATCH 02/12] feat(cli): load the installed data-plane token before a foreground start binds The launchd plist and the systemd unit cat `service-api-token` into the environment before exec, and WinSW names it through OCX_API_TOKEN_FILE, so under a service the proxy has always seen OPENCODEX_API_AUTH_TOKEN regardless of the calling shell. A foreground `ocx start` had neither source, so a non-loopback hostname the installed service serves happily was refused at bind. `handleStart` now hydrates the environment through `startupDataPlaneToken` with the same precedence, gated on `isApiAuthRequired` so a loopback bind keeps ignoring the file -- on a machine connected to a hub it holds that hub's issued client key, not this proxy's admission secret. Refs #4236 Co-Authored-By: Claude Fable 5.1 --- src/cli/index.ts | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/src/cli/index.ts b/src/cli/index.ts index 4cb1e68ba9..21e76fdb1a 100755 --- a/src/cli/index.ts +++ b/src/cli/index.ts @@ -66,10 +66,11 @@ import { dispatchCommand , decideStartWithLiveOwner } from "./dispatch"; import { findAvailablePort, isAddrInUse, PortUnavailableError, shouldPersistSelectedPort, waitForPortAvailable } from "../server/ports"; import { findLiveProxy, probeHostname, type LiveProxy } from "../server/proxy-liveness"; import { createReadinessGate } from "../server/readiness"; +import { isApiAuthRequired } from "../server/auth-cors"; import { runReady, type ReadyArgs } from "./ready"; import { runCli } from "./root"; import { isProcessAlive, ProxyOwnershipRefusedError, refusalNextStep, stopProxy } from "../lib/process-control"; -import { loadServiceTokenFromFile } from "../lib/service-secrets"; +import { startupDataPlaneToken } from "../lib/service-secrets"; import { assertNotAdminToken, diagnoseService, isServiceOwnershipError, proxyStillLiveAfterStop, serviceCommand, serviceEnvironmentOwnedHere, serviceStartableFromTray, serviceStatusSummary, stopServiceIfInstalledDetailed, uninstallServiceIfInstalled, uninstallServiceDetailed } from "../service"; import { formatStartupRoutingDetail, startupHealthSummary } from "../codex/autostart-health"; import { injectSystemEnv, reconcileShellHook, revertSystemEnv, uninstallShellHook } from "../server/system-env"; @@ -291,10 +292,16 @@ async function findProxyOwnerBeforeJournalRecovery( } async function handleStart(options: { block?: boolean } = {}) { - // Native (WinSW) service mode has no batch wrapper to read the service token file - // into the environment, so the app loads it here before the server binds. The server + // Native (WinSW) service mode has no batch wrapper to read the service token file into + // the environment, and a FOREGROUND `ocx start` has no wrapper at all — so the app loads + // the token here, before the server binds, with the same precedence the launchd plist and + // the systemd unit use when they cat the file into the environment. Without the second + // source, `ocx start` refused to bind a non-loopback hostname (assertServerAuthConfig) + // that the installed service on the same machine was serving happily (#4236). The server // auth path reads OPENCODEX_API_AUTH_TOKEN from the environment. - const serviceToken = loadServiceTokenFromFile(process.env); + const serviceToken = startupDataPlaneToken(process.env, { + authRequired: isApiAuthRequired(loadConfig()), + }); if (serviceToken) process.env.OPENCODEX_API_AUTH_TOKEN = serviceToken; // The service wrapper (and WinSW via OCX_API_TOKEN_FILE) can still export a colliding // token that install now refuses to write. Refuse it here too, before bind, so an From 65ec21ae1e4eca1d0c209cec853ddbbe3c13a266 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Fri, 11 Sep 2026 11:43:30 +0900 Subject: [PATCH 03/12] feat(config): add the optional hub.dataPublicOrigin advertised data origin A hub's management and data planes are different sockets on a real deployment: management is a loopback-only ingress published by Tailscale Serve on 443, while the data listener is bound to the node's tailnet address and fronted on its own port. Deriving one from the other produced an origin that answered /readyz and nothing else, so the advertised data origin is its own field. Same canonical-origin transform as managementPublicOrigin (http(s), no credentials, path, query or fragment) and deliberately not `.catch`ed: silently dropping a typo would make `ocx hub invite` fall back to http://:, which is the value the field exists to replace. It is advisory -- the origin the hub advertises, never a bind address. Refs #4236 Co-Authored-By: Claude Fable 5.1 --- src/config.ts | 12 ++++++++++++ src/types/config.ts | 15 +++++++++++++++ 2 files changed, 27 insertions(+) diff --git a/src/config.ts b/src/config.ts index 3fc48312c8..66162c6a6a 100644 --- a/src/config.ts +++ b/src/config.ts @@ -1037,6 +1037,18 @@ const hubConfigSchema = z.object({ } return origin; }).optional(), + // Same canonical-origin rule as managementPublicOrigin, and deliberately NOT `.catch`ed: + // a mistyped data origin must be rejected at write time, because silently dropping it + // makes `ocx hub invite` print the `http://:` fallback that the operator + // set this field precisely to replace. + dataPublicOrigin: z.string().transform((value, ctx) => { + const origin = canonicalHttpOrigin(value); + if (!origin) { + ctx.addIssue({ code: "custom", message: "must be a canonical http(s) origin without credentials, path, query, or fragment" }); + return z.NEVER; + } + return origin; + }).optional(), // A malformed hand edit disables only the optional ingress. Live writes are rejected by // managementIngressConfigError before this load-time degradation can hide the mistake. managementIngress: z.union([ diff --git a/src/types/config.ts b/src/types/config.ts index fdaec36ff8..499ca59eb1 100644 --- a/src/types/config.ts +++ b/src/types/config.ts @@ -259,6 +259,21 @@ export type OcxRuntimeRole = "standalone" | "hub" | "client"; export interface OcxHubConfig { /** Canonical browser-reachable management origin advertised by a hub. */ managementPublicOrigin?: string; + /** + * Canonical client-reachable DATA origin of this hub — what a remote machine passes as the + * positional URL to `ocx connect`, and what `ocx hub invite` prints. + * + * Separate from `managementPublicOrigin` because the two are genuinely different sockets on a + * real deployment: management is a loopback-only ingress published by an HTTPS frontend, while + * the data listener is bound to the hub's tailnet/LAN address and fronted on its own port + * (`https://hub.tailnet.ts.net:8443`). Deriving one from the other produced an origin that + * answered `/readyz` and nothing else. + * + * Advisory only: it is the origin the hub ADVERTISES, never a bind address. When omitted, + * `ocx hub invite` falls back to `http://:`, which is correct for a plain + * tailnet bind with no TLS frontend. + */ + dataPublicOrigin?: string; /** * Optional management-only listener for a local HTTPS frontend such as Tailscale Serve. * The hostname is deliberately not configurable: when enabled the socket is always bound From bf7fed2dce467633a4533a124545ed5c2905a171 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Fri, 11 Sep 2026 11:43:42 +0900 Subject: [PATCH 04/12] feat(cli): add ocx hub invite and one hub block in ocx status `ocx hub invite [--json] [--data-url ] [--management-url ] [--clients codex,claude]` prints the exact line to run on the other machine: # Run on the other machine: echo '' | ocx connect --management-url --pairing-code-stdin No new management route was needed. A "connect pairing code" IS a GUI pairing grant -- `ocx connect --pairing-code-stdin` exchanges it at POST /opencodex-session -- so invite reuses `requestBoundGuiPairingGrant`, the attested local mint `ocx gui pair` already drives, authorized by an HMAC over the running proxy's own attestation secret. It therefore needs no admin token and nothing exported in the shell, and src/server/* is untouched. Everything that cannot work is refused before a single-use code is minted: a non-hub runtimeRole, a missing hub.managementPublicOrigin, a non-loopback plaintext management origin, a malformed --data-url, no running attested proxy, and a hub whose allow-list admits no loopback browser origin. That last one is the non-obvious constraint: `ocx connect` sends Origin: http://localhost:, so only managementPublicOrigin itself or a loopback corsAllowOrigins entry can match the grant, and the refusal names the `ocx config set` line that fixes it. --management-url is a confirmation rather than an override, because the grant is bound to the configured origin and advertising anything else hands out a code the hub then rejects. `ocx status` gains one Hub block on a hub: advertised data origin, the loopback listener in its companion/ported/off forms, the management ingress and public origin, the data-token SOURCE (present (env) / present (file) / unsafe (file) / missing, env first because that is the service's own precedence), and the invite hint. `collectHubStatus` and `hubStatusLines` live in status.ts so the sentences are testable without spawning the CLI; no token value reaches either. Refs #4236 Co-Authored-By: Claude Fable 5.1 --- src/cli/dispatch.ts | 7 ++ src/cli/hub.ts | 250 ++++++++++++++++++++++++++++++++++++++++++++ src/cli/index.ts | 7 +- src/cli/status.ts | 100 ++++++++++++++++++ 4 files changed, 363 insertions(+), 1 deletion(-) create mode 100644 src/cli/hub.ts diff --git a/src/cli/dispatch.ts b/src/cli/dispatch.ts index a02f1d6f41..768c86bf78 100644 --- a/src/cli/dispatch.ts +++ b/src/cli/dispatch.ts @@ -583,6 +583,13 @@ const commandRunners: Record = { }, }); }, + hub: async deps => { + const { runHubCommand } = await import("./hub"); + return runHubCommand(deps.args.slice(1), { + loadConfig: deps.loadConfig, + findLiveProxy: deps.findLiveProxy, + }); + }, service: async deps => { process.exitCode = 0; await deps.serviceCommand(...deps.args.slice(1)); diff --git a/src/cli/hub.ts b/src/cli/hub.ts new file mode 100644 index 0000000000..f909cf58dc --- /dev/null +++ b/src/cli/hub.ts @@ -0,0 +1,250 @@ +import type { OcxConfig, OcxConnectedClientId } from "../types"; +import { canonicalGuiBrowserOrigin } from "../lib/gui-pair-capability"; +import { findLiveProxy, probeHostname, type LiveProxy } from "../server/proxy-liveness"; +import { + requestBoundGuiPairingGrant, + type GuiPairClientDeps, + type GuiPairRequestResult, +} from "./gui-pair-client"; +import type { RuntimeApiDeps } from "./runtime-api"; + +export const HUB_USAGE = + "ocx hub invite [--json] [--data-url ] [--management-url ] [--clients codex,claude]"; + +const PAIRING_WARNING = "Pairing codes are secret, single-use, and expire quickly. Do not save them."; + +/** + * The default browser origin a connecting machine presents. + * + * `ocx connect --pairing-code-stdin` exchanges the code with `Origin:` set by `localGuiOrigin()` + * in `src/client/connect.ts` — `http://localhost:`, which on a + * fresh client is the default 10100. The hub cannot observe the other machine's port, so the + * grant is bound to this origin unless `corsAllowOrigins` names a different loopback one. + */ +const DEFAULT_CLIENT_BROWSER_ORIGIN = "http://localhost:10100"; + +export interface HubCommandDeps extends RuntimeApiDeps { + loadConfig: () => OcxConfig; + findLiveProxy?: () => Promise; + requestPairingGrant?: ( + target: LiveProxy, + browserOrigin: string, + deps?: GuiPairClientDeps, + ) => Promise; +} + +export interface HubInviteOptions { + json: boolean; + dataUrl?: string; + managementUrl?: string; + clients?: string; +} + +export interface HubInvitePayload { + code: string; + expiresAt: string; + dataUrl: string; + managementUrl: string; + command: string; +} + +function canonicalOrigin(value: string | undefined): string | null { + if (typeof value !== "string" || !value.trim()) return null; + try { + const parsed = new URL(value); + if (parsed.protocol !== "http:" && parsed.protocol !== "https:") return null; + if (parsed.username || parsed.password || parsed.search || parsed.hash) return null; + if (parsed.pathname !== "" && parsed.pathname !== "/") return null; + return parsed.origin; + } catch { + return null; + } +} + +function isLoopbackOrigin(origin: string): boolean { + try { + const host = new URL(origin).hostname.toLowerCase(); + return host === "localhost" || host === "127.0.0.1" || host === "::1" || host === "[::1]"; + } catch { + return false; + } +} + +/** Loopback or HTTPS, the rule `consumeGuiPairingGrant` enforces on the hub side. */ +export function pairingOriginUsable(origin: string): boolean { + try { + return new URL(origin).protocol === "https:" || isLoopbackOrigin(origin); + } catch { + return false; + } +} + +export function parseHubInviteArgs(args: string[]): HubInviteOptions | null { + const options: HubInviteOptions = { json: false }; + for (let index = 0; index < args.length; index++) { + const arg = args[index]!; + if (arg === "--json" && !options.json) { + options.json = true; + continue; + } + if (arg === "--data-url" || arg === "--management-url" || arg === "--clients") { + const value = args[++index]; + if (!value || value.startsWith("--")) return null; + const key = arg === "--data-url" ? "dataUrl" : arg === "--management-url" ? "managementUrl" : "clients"; + if (options[key] !== undefined) return null; + options[key] = value; + continue; + } + return null; + } + return options; +} + +export function parseInviteClients(raw: string | undefined): OcxConnectedClientId[] | null { + if (raw === undefined) return []; + const values = raw.split(",").map(value => value.trim()).filter(Boolean); + if (values.length < 1 || values.some(value => value !== "codex" && value !== "claude")) return null; + return values as OcxConnectedClientId[]; +} + +/** + * The browser origin the grant is bound to, or null when the hub admits none. + * + * `createGuiPairingGrant` accepts only `hub.managementPublicOrigin` itself or an entry of + * `corsAllowOrigins`, so this picks from exactly that set rather than guessing: the default + * client origin when it is admitted, otherwise the first admitted loopback origin. A hub whose + * allow-list names no loopback origin cannot pair a remote `ocx connect` at all, and saying so + * here is better than minting a code the exchange will reject. + */ +export function selectInviteBrowserOrigin(config: OcxConfig): string | null { + const allowed = [ + canonicalGuiBrowserOrigin(config.hub?.managementPublicOrigin ?? ""), + ...(config.corsAllowOrigins ?? []).map(value => canonicalGuiBrowserOrigin(value)), + ].filter((value): value is string => Boolean(value)); + if (allowed.includes(DEFAULT_CLIENT_BROWSER_ORIGIN)) return DEFAULT_CLIENT_BROWSER_ORIGIN; + return allowed.find(origin => isLoopbackOrigin(origin)) ?? null; +} + +/** `http://:` — right for a plain tailnet/LAN bind with no TLS frontend. */ +export function derivedHubDataOrigin(hostname: string | undefined, port: number): string { + const host = probeHostname(hostname); + return `http://${host === "127.0.0.1" ? "localhost" : host}:${port}`; +} + +export function hubInviteCommand( + code: string, + dataUrl: string, + managementUrl: string, + clients: OcxConnectedClientId[], +): string { + const clientsFlag = clients.length > 0 ? ` --clients ${clients.join(",")}` : ""; + return `echo '${code}' | ocx connect ${dataUrl} --management-url ${managementUrl}${clientsFlag} --pairing-code-stdin`; +} + +async function runInvite(args: string[], deps: HubCommandDeps): Promise { + const options = parseHubInviteArgs(args); + if (!options) { + console.error(`Usage: ${HUB_USAGE}`); + return 1; + } + const clients = parseInviteClients(options.clients); + if (!clients) { + console.error("--clients must contain codex and/or claude."); + return 1; + } + const config = deps.loadConfig(); + if (config.runtimeRole !== "hub") { + console.error( + `ocx hub invite runs on a hub; this machine's runtimeRole is "${config.runtimeRole ?? "standalone"}". ` + + "A client machine runs 'ocx connect' with the code its hub printed.", + ); + return 1; + } + // The grant's server origin IS hub.managementPublicOrigin (createGuiPairingGrant reads it, + // and the exchange compares the request's management origin against it), so an invite that + // advertised anything else would hand out a code the hub then refuses. + const managementPublic = canonicalOrigin(config.hub?.managementPublicOrigin); + if (!managementPublic) { + console.error( + "hub.managementPublicOrigin is not set, so there is no origin to pair against. Set the exact " + + "browser-visible HTTPS origin: ocx config set hub.managementPublicOrigin '\"https://hub.tailnet.ts.net\"'", + ); + return 1; + } + const managementOverride = options.managementUrl === undefined ? null : canonicalOrigin(options.managementUrl); + if (options.managementUrl !== undefined && !managementOverride) { + console.error("--management-url must be a bare http(s) origin with no path, query, or credentials."); + return 1; + } + if (managementOverride && managementOverride !== managementPublic) { + console.error( + `--management-url ${managementOverride} does not match hub.managementPublicOrigin ${managementPublic}. ` + + "The pairing code is bound to the configured origin, so the other machine would be refused.", + ); + return 1; + } + if (!pairingOriginUsable(managementPublic)) { + console.error( + `hub.managementPublicOrigin ${managementPublic} is non-loopback plain HTTP, which cannot carry a ` + + "pairing code. Put management behind an HTTPS frontend (Tailscale Serve) and set that origin.", + ); + return 1; + } + const dataOverride = options.dataUrl === undefined ? null : canonicalOrigin(options.dataUrl); + if (options.dataUrl !== undefined && !dataOverride) { + console.error("--data-url must be a bare http(s) origin with no path, query, or credentials."); + return 1; + } + const browserOrigin = selectInviteBrowserOrigin(config); + if (!browserOrigin) { + console.error( + "No loopback browser origin is admitted for pairing. Add the connecting machine's local origin: " + + `ocx config set corsAllowOrigins '["${DEFAULT_CLIENT_BROWSER_ORIGIN}"]'`, + ); + return 1; + } + const target = await (deps.findLiveProxy ?? findLiveProxy)(); + if (!target) { + console.error("No running attested OpenCodex hub was found. Check 'ocx service status', then 'ocx service repair'."); + return 1; + } + const dataUrl = dataOverride + ?? canonicalOrigin(config.hub?.dataPublicOrigin) + ?? derivedHubDataOrigin(target.hostname ?? config.hostname, target.port); + const result = await (deps.requestPairingGrant ?? requestBoundGuiPairingGrant)(target, browserOrigin, { + ...(deps.fetchImpl ? { fetchImpl: deps.fetchImpl } : {}), + }); + if (result.kind !== "created") { + console.error(`Minting a pairing code failed (${result.reason}).`); + return 1; + } + const payload: HubInvitePayload = { + code: result.grant, + expiresAt: new Date(result.expiresAt).toISOString(), + dataUrl, + managementUrl: managementPublic, + command: hubInviteCommand(result.grant, dataUrl, managementPublic, clients), + }; + if (options.json) { + console.log(JSON.stringify(payload)); + console.error(PAIRING_WARNING); + return 0; + } + // Remaining time, not the constant TTL: the number an operator reads has to be the one + // they actually have left by the time the line is printed. + const ttlSeconds = Math.max(0, Math.round((result.expiresAt - Date.now()) / 1000)); + console.log(`Pairing code for one machine — single-use, expires in ${ttlSeconds}s (${payload.expiresAt}).`); + console.log(""); + console.log("# Run on the other machine:"); + console.log(payload.command); + console.error(PAIRING_WARNING); + return 0; +} + +export async function runHubCommand(args: string[], deps: HubCommandDeps): Promise { + if (args[0] !== "invite") { + console.error(`Usage: ${HUB_USAGE}`); + return 1; + } + return runInvite(args.slice(1), deps); +} diff --git a/src/cli/index.ts b/src/cli/index.ts index 21e76fdb1a..730ec0c795 100755 --- a/src/cli/index.ts +++ b/src/cli/index.ts @@ -48,7 +48,7 @@ import { pendingTeardownPathFor, quarantinePendingTeardown, } from "../config/pending-teardown"; -import { collectStatus, unusedProxyWarningLines } from "./status"; +import { collectStatus, hubStatusLines, unusedProxyWarningLines } from "./status"; import { endpointsToProve, everyEndpointProvenDown, sharedTeardownAuthorized, type UninstallObservation } from "./uninstall-plan"; import { takeFlag } from "./runtime-api"; @@ -1434,6 +1434,11 @@ async function handleStatus() { console.log(` Runtime: ${status.json.paths.runtime}`); console.log(` Runtime source: ${status.json.runtime.source}${status.json.runtime.overrideEnv ? ` (${status.json.runtime.overrideEnv})` : ""}`); console.log(` Default provider: ${status.json.defaultProvider}`); + // One block rather than six scattered lines, and only on a hub: `hubStatusLines` owns the + // sentences so they are testable without spawning the CLI. It prints no token value. + if (status.json.hub) { + for (const line of hubStatusLines(status.json.hub)) console.log(` ${line}`); + } console.log(` Remote hub: ${status.json.connection.state}${status.json.connection.serverUrl ? ` (${status.json.connection.serverUrl})` : ""}`); if (status.json.connection.state === "invalid" || status.json.connection.state === "mismatched") { console.log(` ⚠️ ${status.json.connection.reason}`); diff --git a/src/cli/status.ts b/src/cli/status.ts index 8590ba38ec..420388f6c7 100644 --- a/src/cli/status.ts +++ b/src/cli/status.ts @@ -18,10 +18,31 @@ import { effectiveLoopbackListenerPort } from "../codex/loopback-target"; import { claudeDesktopIntegrationEnabled } from "../codex/desired-state"; import { claudeDesktopPolicyHealth, probeClaudeDesktopPolicy, type ClaudeDesktopPolicyHealth } from "../claude/desktop-policy"; import { collectClientConnectionStatus } from "./connect"; +import { readServiceApiTokenState, serviceApiTokenFilePath } from "../lib/service-secrets"; export { proxyHealthFailureReason, isConnectionRefused, isUncleanExitEvidence, probeUncleanExitState } from "./status-probes"; export type { ListenTarget } from "./status-probes"; import { checkProxyHealth, probeUncleanExitState, type ListenTarget } from "./status-probes"; +/** Where the data-plane admission secret comes from. Source only -- never the value. */ +export type HubDataTokenState = "present (env)" | "present (file)" | "unsafe (file)" | "missing"; + +export type HubStatus = { + /** Advertised data origin: hub.dataPublicOrigin, else derived from the bind address. */ + dataOrigin: string; + /** True when dataOrigin came from config rather than being derived from the bind. */ + dataOriginConfigured: boolean; + /** + * The unauthenticated loopback listener, in PR2's two forms: `companion` shares the public + * port (the one-port hub), `ported` binds its own. `off` means the hub does not serve its + * own local clients at all. + */ + loopbackListener: { state: "off" | "companion" | "ported"; port: number | null }; + managementIngress: { enabled: boolean; port: number | null }; + managementPublicOrigin: string | null; + dataToken: HubDataTokenState; + dataTokenPath: string; +}; + export type CliStatusJson = { schemaVersion: 1; proxy: { @@ -89,6 +110,18 @@ export type CliStatusJson = { desiredEnabled: boolean; policy: ClaudeDesktopPolicyHealth; }; + /** + * The hub-only facts an operator needs in one place, or null on a standalone/client machine. + * + * Scattered across the report they were unusable: the public data origin came from `listen`, + * the management origin was folded into `dashboard.url`, the loopback companion appeared + * nowhere, and the data-plane token appeared nowhere at all -- so the one question a hub + * operator actually asks ("is this reachable, and can another machine join?") took four other + * commands to answer. Additive and nullable, so `schemaVersion` stays 1. + * + * Never carries a token value; only which source holds one. + */ + hub: HubStatus | null; /** * This CLI's version against the running proxy's (#2701). * @@ -120,6 +153,72 @@ function statusDashboardUrl(config: StatusListenConfig, hostname: string | undef return `http://${dashboardHostname}:${port}/`; } +/** + * The hub block, or null when this machine is not a hub. + * + * `env` wins over `file` because that is the service's own precedence + * (`writeServiceApiTokenFile`): the launch wrapper exports OPENCODEX_API_AUTH_TOKEN from the + * file only when the calling environment has none, so reporting the file first would name a + * source the running process is not using. + * + * The token VALUE is never read into the report. `readServiceApiTokenState` returns it, and the + * only thing taken from that result is `kind`. + */ +export function collectHubStatus( + config: Pick, + listen: { port: number; hostname?: string | null }, + env: NodeJS.ProcessEnv = process.env, +): HubStatus | null { + if (config.runtimeRole !== "hub") return null; + const listener = config.unauthenticatedLoopbackListener; + const loopbackPort = effectiveLoopbackListenerPort(config, listen.port); + const ingress = config.hub?.managementIngress; + const configuredDataOrigin = config.hub?.dataPublicOrigin; + const host = probeHostname(listen.hostname ?? config.hostname); + const tokenState = env.OPENCODEX_API_AUTH_TOKEN?.trim() + ? "present (env)" as const + : ((): HubDataTokenState => { + const state = readServiceApiTokenState(); + return state.kind === "present" ? "present (file)" : state.kind === "unsafe" ? "unsafe (file)" : "missing"; + })(); + return { + dataOrigin: configuredDataOrigin + ?? `http://${host === "127.0.0.1" ? "localhost" : host}:${listen.port}`, + dataOriginConfigured: Boolean(configuredDataOrigin), + loopbackListener: loopbackPort === null + ? { state: "off", port: null } + : { state: listener?.enabled && listener.port === undefined ? "companion" : "ported", port: loopbackPort }, + managementIngress: { + enabled: ingress?.enabled === true, + port: ingress?.enabled === true ? ingress.port : null, + }, + managementPublicOrigin: config.hub?.managementPublicOrigin ?? null, + dataToken: tokenState, + dataTokenPath: serviceApiTokenFilePath(), + }; +} + +/** + * The human rendering of the hub block, owned here rather than in the `ocx status` printer so + * the sentences are testable without spawning the CLI. Indentation is the caller's. + */ +export function hubStatusLines(hub: HubStatus): string[] { + const listener = hub.loopbackListener.state === "off" + ? "off — this hub does not route its own local Codex/Claude clients" + : hub.loopbackListener.state === "companion" + ? `companion on http://127.0.0.1:${hub.loopbackListener.port} — same port as the public listener, no credential needed locally` + : `ported on http://127.0.0.1:${hub.loopbackListener.port} — a second port local clients must be pointed at`; + return [ + "Hub:", + ` Data origin: ${hub.dataOrigin}${hub.dataOriginConfigured ? " (hub.dataPublicOrigin)" : " (derived from the bind address)"}`, + ` Loopback listener: ${listener}`, + ` Management ingress: ${hub.managementIngress.enabled ? `http://127.0.0.1:${hub.managementIngress.port}` : "disabled"}`, + ` Management origin: ${hub.managementPublicOrigin ?? "unset — remote pairing and the remote dashboard need hub.managementPublicOrigin"}`, + ` Data token: ${hub.dataToken}${hub.dataToken === "present (file)" || hub.dataToken === "unsafe (file)" ? ` at ${hub.dataTokenPath}` : ""}`, + " Invite a machine: ocx hub invite", + ]; +} + export function selectListenTarget( config: StatusListenConfig, pid: number | null, @@ -357,6 +456,7 @@ export async function collectStatus(): Promise { source: bunRuntime.source, ...(bunRuntime.source === "override" ? { overrideEnv: bunRuntime.overrideEnv } : {}), }, + hub: collectHubStatus(config, listen), codexAutostart: codexAutoStartEnabled(config), startup, defaultProvider: typeof config.defaultProvider === "string" ? config.defaultProvider : null, From e933af6ab498c29ad6ad4257ad3411d1d0950a2b Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Fri, 11 Sep 2026 11:43:50 +0900 Subject: [PATCH 05/12] docs(cli): document the hub topology, invites, and token auto-provisioning `ocx hub` gets a registry entry whose details are the one place that states the topology end to end: one port, the companion loopback listener local processes dial without a credential, the separate loopback-only management plane, the owner-only token file that must never be copied to another machine, and how an invite's code is bound. `ocx help hub` renders it. `ocx service` details gain the token-provisioning precedence and the admin-token refusal, so an operator reading help before installing learns that nothing has to be exported by hand -- the absence of that sentence is what made the incident in #4236 reachable from the documentation alone. `ocx hub invite` is a declared capability and `bun run skill:surface` regenerated references/01_management_surface.md (39 capabilities, 18 state-changing). Its `routes` are deliberately empty with the reason in a comment: the mint it drives is answered in the composition root ahead of handleManagementAPI and so is absent from MANAGEMENT_ROUTES, and declaring it would fail the capability/registry reconciliation rather than inform anyone. Refs #4236 Co-Authored-By: Claude Fable 5.1 --- .../ocx/references/01_management_surface.md | 24 ++++++++++++-- src/cli/capabilities.ts | 26 ++++++++++++++++ src/cli/help.ts | 3 ++ src/cli/registry.ts | 31 +++++++++++++++++++ 4 files changed, 82 insertions(+), 2 deletions(-) diff --git a/skills/ocx/references/01_management_surface.md b/skills/ocx/references/01_management_surface.md index ba2b4b5823..dbebccdd9b 100644 --- a/skills/ocx/references/01_management_surface.md +++ b/skills/ocx/references/01_management_surface.md @@ -394,6 +394,26 @@ JSON mode: `payload`. - Uses the exact upstream model ID after the first slash. Omitted cache rates default to zero; sibling model prices are preserved. +### `ocx hub invite` + +Mint a single-use pairing code on a hub and print the exact `ocx connect` line for one more machine. + +Drives no management route. + +| Flag | Value | Meaning | +|---|---|---| +| `--json` | boolean | Emit code, expiresAt, dataUrl, managementUrl, and command. | +| `--data-url` | string | Advertise this data origin instead of hub.dataPublicOrigin or the bind address. | +| `--management-url` | string | Confirm the management origin; it must equal hub.managementPublicOrigin. | +| `--clients` | string | Pre-select codex and/or claude in the printed connect command. | + +JSON mode: `envelope`. + +- Hub only: refuses when runtimeRole is not hub, and requires a running attested proxy. +- The code is secret, single-use and short-lived; it is bound to hub.managementPublicOrigin and to the connecting machine's loopback browser origin. +- Prints no data-plane token. Remote machines receive their own revocable per-client key from the exchange. +- Mints through the attested local pairing-grant route, the same one ocx gui pair uses; no admin token is read. + ### `ocx connect rotate` Rotate the connected client's data key against the hub, with commit and abort. @@ -706,6 +726,6 @@ JSON mode: `payload`. ## Counts -- declared capabilities: 38 -- of those, state-changing: 17 +- declared capabilities: 39 +- of those, state-changing: 18 - head-resolved invocations: 2 diff --git a/src/cli/capabilities.ts b/src/cli/capabilities.ts index c6695be2ca..e88c6568c7 100644 --- a/src/cli/capabilities.ts +++ b/src/cli/capabilities.ts @@ -132,6 +132,32 @@ export const CAPABILITIES: readonly Capability[] = [ json: "envelope", details: ["Reads /healthz plus local config; drives no management API route."], }, + { + command: ["hub", "invite"], + summary: "Mint a single-use pairing code on a hub and print the exact `ocx connect` line for one more machine.", + // Deliberately empty. The command DOES drive `POST /api/gui/pairing-grants` -- the attested + // local mint route `ocx gui pair` uses, authorized by a capability HMAC'd with the running + // proxy's own attestation secret rather than by the admin token, which is why it needs + // nothing exported in the shell. That route is answered in the composition root, ahead of + // `handleManagementAPI`, so it is not in MANAGEMENT_ROUTES; declaring it here would fail the + // capability/registry reconciliation rather than inform anyone. Widening the registry's scope + // to `src/server/index.ts` is its own change. + routes: [], + flags: [ + { name: "--json", value: "boolean", summary: "Emit code, expiresAt, dataUrl, managementUrl, and command." }, + { name: "--data-url", value: "string", summary: "Advertise this data origin instead of hub.dataPublicOrigin or the bind address." }, + { name: "--management-url", value: "string", summary: "Confirm the management origin; it must equal hub.managementPublicOrigin." }, + { name: "--clients", value: "string", summary: "Pre-select codex and/or claude in the printed connect command." }, + ], + mutates: true, + json: "envelope", + details: [ + "Hub only: refuses when runtimeRole is not hub, and requires a running attested proxy.", + "The code is secret, single-use and short-lived; it is bound to hub.managementPublicOrigin and to the connecting machine's loopback browser origin.", + "Prints no data-plane token. Remote machines receive their own revocable per-client key from the exchange.", + "Mints through the attested local pairing-grant route, the same one ocx gui pair uses; no admin token is read.", + ], + }, { command: ["connect", "rotate"], summary: "Rotate the connected client's data key against the hub, with commit and abort.", diff --git a/src/cli/help.ts b/src/cli/help.ts index 43916695b6..4137dabda7 100644 --- a/src/cli/help.ts +++ b/src/cli/help.ts @@ -56,6 +56,8 @@ Usage: ocx logout Remove a stored OAuth login ocx gui [pair --origin [--json]] Open the dashboard or create a single-use remote pairing grant + ocx hub invite [--json] Print a ready-to-run \`ocx connect\` line for one more machine + (hub only; see \`ocx help hub\` for the one-port topology) ocx update [--tag ] Update opencodex (keeps preview installs on @preview) ocx restart Stop and restart the proxy ocx v2 multi_agent_v2 surface (status|on|off|mode|keep-native-v1|threads|mode-hint) @@ -99,6 +101,7 @@ Examples: ocx start Start on default port (10100) ocx start --port 8080 Start on custom port ocx help service Show service command help + ocx help hub Explain the hub topology, token file, and invites ocx sync Sync available models to Codex`); } diff --git a/src/cli/registry.ts b/src/cli/registry.ts index 06b0dfaf29..068a7fee13 100644 --- a/src/cli/registry.ts +++ b/src/cli/registry.ts @@ -68,6 +68,11 @@ export const CLI_COMMANDS: CliCommandEntry[] = [ "`repair` refreshes the definition and reloads the manager only when something changed, so repairing a healthy service is not an outage.", "`restart` is the same refresh but always restarts: on macOS an unchanged, already-loaded job is kickstarted in place. Healthy Windows tasks are reused, while stale definitions may re-register and elevate.", "Use `ocx service status` to see diagnostics and log paths.", + "Data-plane token: nothing has to be exported by hand. On a non-loopback hostname install/repair uses", + "OPENCODEX_API_AUTH_TOKEN when set, otherwise reuses the existing owner-only ~/.opencodex/service-api-token,", + "otherwise generates one; the launch wrapper reads that file at start and the value never enters a plist,", + "a unit file, or argv. An ADMIN token in OPENCODEX_API_AUTH_TOKEN is refused -- unset it and rerun.", + "Repair and restart never ask for the environment variable again once the token file exists.", ], }, { @@ -156,6 +161,32 @@ export const CLI_COMMANDS: CliCommandEntry[] = [ "The printed grant is secret, single-use, short-lived, and must not be persisted.", ], }, + { + name: "hub", + usage: "ocx hub invite [--json] [--data-url ] [--management-url ] [--clients codex,claude]", + summary: "Hub-side commands. `invite` prints a ready-to-run `ocx connect` line for one more machine.", + details: [ + "Topology: a hub serves ONE port. Remote machines dial `hostname:port` with their own per-client", + "key; the hub's own local processes dial `127.0.0.1:` with no credential, through the", + "loopback companion listener enabled by `unauthenticatedLoopbackListener: {\"enabled\": true}`.", + "The browser-facing management plane is separate: a loopback-only ingress published by an", + "operator-owned HTTPS frontend (Tailscale Serve) and advertised as hub.managementPublicOrigin.", + "", + "Nothing needs to be exported by hand. `ocx service install` provisions an owner-only data-plane", + "token at ~/.opencodex/service-api-token and the launch wrapper reads it at start; the value never", + "enters a plist, a unit file, argv, or the environment you typed in. Never copy that file to another", + "machine -- every client gets its own revocable key from the pairing exchange.", + "", + "invite requires a running hub and mints a single-use, short-lived pairing code through the same", + "attested local route `ocx gui pair` uses, then prints the exact command to run on the other", + "machine. The code is bound to hub.managementPublicOrigin and to the connecting machine's local", + "browser origin (`http://localhost:10100` unless corsAllowOrigins names another loopback origin).", + "--data-url overrides the advertised data origin; hub.dataPublicOrigin is the persistent form, and", + "the fallback is http://:.", + "--json emits { code, expiresAt, dataUrl, managementUrl, command }. Do not persist the code.", + "Hub state, including the data token and the companion listener, is reported by `ocx status`.", + ], + }, { name: "update", usage: "ocx update [--tag latest|preview]", From 53873777122d0815051c113a6756e56ac2f3624a Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Fri, 11 Sep 2026 11:43:59 +0900 Subject: [PATCH 06/12] test(service,cli): pin token provisioning, the hub block, and invite refusals Service: a non-loopback install no longer demands the env token; provisioning generates 64 hex chars at mode 0600 and then REUSES the same value on repair and reinstall (regenerating would invalidate every client key exchange already performed); an env token still wins and a loopback install creates nothing; an unusable token file is reported by the preflight rather than failing mid-install; and the admin-token refusal tells the operator to unset, reaching the chokepoint so nothing is written. `startupDataPlaneToken`: the environment wins, OCX_API_TOKEN_FILE still beats the installed path, the installed token is used only when admission is required, and an absent or unusable file resolves to null instead of throwing at boot. `ocx hub invite`: argument parsing, the origin helpers, the exact printed command, the --json envelope, and every refusal path -- including that a --management-url differing from the configured origin is refused rather than printed. The mint is injected, so no proxy, socket or real grant is involved. `ocx status` hub block: companion vs ported vs off, the configured vs derived data origin, env-over-file token precedence, missing vs unusable, the invite hint, and -- the assertion that matters for a security boundary -- that no token value appears in the JSON or the rendered lines. `hub.dataPublicOrigin` normalizes like the management origin and rejects the same five malformed shapes at write time. Refs #4236 Co-Authored-By: Claude Fable 5.1 --- scripts/test-layout/layout.json | 1 + tests/cli/cli-status-json.test.ts | 125 +++++++++++- tests/cli/hub-invite.test.ts | 234 +++++++++++++++++++++++ tests/fixtures/test-layout-expected.json | 1 + tests/server/config.test.ts | 29 +++ tests/service/service-secrets.test.ts | 37 ++++ tests/service/service.test.ts | 105 +++++++++- 7 files changed, 528 insertions(+), 4 deletions(-) create mode 100644 tests/cli/hub-invite.test.ts diff --git a/scripts/test-layout/layout.json b/scripts/test-layout/layout.json index 032252ad37..ad361fc548 100644 --- a/scripts/test-layout/layout.json +++ b/scripts/test-layout/layout.json @@ -704,6 +704,7 @@ "history-ocx-compaction-recovery.test.ts": "codex-integration", "hyperbolic-provider.test.ts": "providers", "hub-gated-local-clients.test.ts": "cli", + "hub-invite.test.ts": "cli", "identity-neutralize.test.ts": "adapters", "init-backup-cleanup.test.ts": "service", "init-eof.test.ts": "service", diff --git a/tests/cli/cli-status-json.test.ts b/tests/cli/cli-status-json.test.ts index 24191ba887..9c8da57078 100644 --- a/tests/cli/cli-status-json.test.ts +++ b/tests/cli/cli-status-json.test.ts @@ -7,7 +7,7 @@ import type { AddressInfo } from "node:net"; import { tmpdir } from "node:os"; import { dirname, join } from "node:path"; import { fileURLToPath } from "node:url"; -import { isConnectionRefused, isUncleanExitEvidence, proxyHealthFailureReason, resolveStatusPid, selectListenTarget } from "../../src/cli/status"; +import { collectHubStatus, hubStatusLines, isConnectionRefused, isUncleanExitEvidence, proxyHealthFailureReason, resolveStatusPid, selectListenTarget } from "../../src/cli/status"; import * as statusFacade from "../../src/cli/status"; import * as statusProbes from "../../src/cli/status-probes"; import { packageVersion } from "../../src/cli/help"; @@ -604,6 +604,129 @@ describe("CLI status JSON", () => { * it. These cases pin the predicate, including the two false-positive shapes that a * naive implementation gets wrong. */ +/** + * The hub block (#4236). + * + * A hub operator's first question is "is this reachable, and can another machine join?", and the + * report used to answer it in four places and not at all for the data token. These tests pin the + * projection and the sentences, and -- the one that matters for a security boundary -- that no + * token VALUE is ever in either. + */ +describe("status hub block", () => { + const TOKEN = "b".repeat(64); + + function withHome(setup: (home: string) => void, body: () => T): T { + const home = mkdtempSync(join(tmpdir(), "ocx-status-hub-")); + const previous = process.env.OPENCODEX_HOME; + const previousToken = process.env.OPENCODEX_API_AUTH_TOKEN; + process.env.OPENCODEX_HOME = home; + delete process.env.OPENCODEX_API_AUTH_TOKEN; + try { + mkdirSync(join(home), { recursive: true }); + setup(home); + return body(); + } finally { + if (previous === undefined) delete process.env.OPENCODEX_HOME; + else process.env.OPENCODEX_HOME = previous; + if (previousToken === undefined) delete process.env.OPENCODEX_API_AUTH_TOKEN; + else process.env.OPENCODEX_API_AUTH_TOKEN = previousToken; + removeTreeWithRetry(home); + } + } + + const hub = (overrides: Record = {}) => ({ + port: 10100, + hostname: "100.64.0.10", + runtimeRole: "hub" as const, + hub: { managementPublicOrigin: "https://hub.tailnet.ts.net", managementIngress: { enabled: true as const, port: 10101 } }, + unauthenticatedLoopbackListener: { enabled: true as const }, + ...overrides, + }); + + test("there is no hub block on a standalone or client machine", () => { + for (const role of [undefined, "standalone", "client"] as const) { + const config = { ...hub(), runtimeRole: role } as Parameters[0]; + expect(collectHubStatus(config, { port: 10100, hostname: "127.0.0.1" }, {})).toBeNull(); + } + }); + + test("the companion form is named as sharing the public port; a ported one is not", () => { + const companion = collectHubStatus(hub() as Parameters[0], { port: 10100, hostname: "100.64.0.10" }, {}); + expect(companion?.loopbackListener).toEqual({ state: "companion", port: 10100 }); + expect(hubStatusLines(companion!).join("\n")).toContain("same port as the public listener"); + + const ported = collectHubStatus( + hub({ unauthenticatedLoopbackListener: { enabled: true, port: 10104 } }) as Parameters[0], + { port: 10100, hostname: "100.64.0.10" }, + {}, + ); + expect(ported?.loopbackListener).toEqual({ state: "ported", port: 10104 }); + expect(hubStatusLines(ported!).join("\n")).toContain("http://127.0.0.1:10104"); + + const off = collectHubStatus( + hub({ unauthenticatedLoopbackListener: { enabled: false } }) as Parameters[0], + { port: 10100, hostname: "100.64.0.10" }, + {}, + ); + expect(off?.loopbackListener).toEqual({ state: "off", port: null }); + expect(hubStatusLines(off!).join("\n")).toContain("does not route its own local"); + }); + + test("the data origin prefers hub.dataPublicOrigin and says which it used", () => { + const derived = collectHubStatus(hub() as Parameters[0], { port: 10100, hostname: "100.64.0.10" }, {}); + expect(derived?.dataOrigin).toBe("http://100.64.0.10:10100"); + expect(derived?.dataOriginConfigured).toBe(false); + expect(hubStatusLines(derived!).join("\n")).toContain("derived from the bind address"); + + const configured = collectHubStatus( + hub({ hub: { managementPublicOrigin: "https://hub.tailnet.ts.net", dataPublicOrigin: "https://hub.tailnet.ts.net:8443" } }) as Parameters[0], + { port: 10100, hostname: "100.64.0.10" }, + {}, + ); + expect(configured?.dataOrigin).toBe("https://hub.tailnet.ts.net:8443"); + expect(hubStatusLines(configured!).join("\n")).toContain("hub.dataPublicOrigin"); + }); + + test("the token source is reported, never the token, and env wins over the file", () => { + withHome(home => writeFileSync(join(home, "service-api-token"), `${TOKEN}\n`, "utf8"), () => { + const fromFile = collectHubStatus(hub() as Parameters[0], { port: 10100 }, {}); + expect(fromFile?.dataToken).toBe("present (file)"); + // Service precedence: the launch wrapper exports the file only when the environment + // has nothing, so an env token is what the running process is actually using. + const fromEnv = collectHubStatus( + hub() as Parameters[0], + { port: 10100 }, + { OPENCODEX_API_AUTH_TOKEN: "from-the-shell" }, + ); + expect(fromEnv?.dataToken).toBe("present (env)"); + for (const status of [fromFile!, fromEnv!]) { + const rendered = [JSON.stringify(status), ...hubStatusLines(status)].join("\n"); + expect(rendered).not.toContain(TOKEN); + expect(rendered).not.toContain("from-the-shell"); + } + }); + }); + + test("a missing and an unusable token file are distinguished", () => { + withHome(() => {}, () => { + expect(collectHubStatus(hub() as Parameters[0], { port: 10100 }, {})?.dataToken).toBe("missing"); + }); + withHome(home => writeFileSync(join(home, "service-api-token"), "\n", "utf8"), () => { + const status = collectHubStatus(hub() as Parameters[0], { port: 10100 }, {}); + expect(status?.dataToken).toBe("unsafe (file)"); + expect(hubStatusLines(status!).join("\n")).toContain(status!.dataTokenPath); + }); + }); + + test("the block always ends with the invite hint", () => { + withHome(() => {}, () => { + const lines = hubStatusLines(collectHubStatus(hub() as Parameters[0], { port: 10100 }, {})!); + expect(lines[0]).toBe("Hub:"); + expect(lines.at(-1)).toBe(" Invite a machine: ocx hub invite"); + }); + }); +}); + describe("unclean prior exit evidence", () => { const base = { live: false, diff --git a/tests/cli/hub-invite.test.ts b/tests/cli/hub-invite.test.ts new file mode 100644 index 0000000000..79c53ad035 --- /dev/null +++ b/tests/cli/hub-invite.test.ts @@ -0,0 +1,234 @@ +import { describe, expect, spyOn, test } from "bun:test"; +import { + derivedHubDataOrigin, + hubInviteCommand, + pairingOriginUsable, + parseHubInviteArgs, + parseInviteClients, + runHubCommand, + selectInviteBrowserOrigin, +} from "../../src/cli/hub"; +import type { GuiPairRequestResult } from "../../src/cli/gui-pair-client"; +import type { LiveProxy } from "../../src/server/proxy-liveness"; +import type { OcxConfig } from "../../src/types"; + +/** + * `ocx hub invite` (#4236). + * + * The command's whole value is that the line it prints can be pasted on the other machine and + * work, so these tests pin the two things that decide that: WHICH origins end up in the command, + * and WHICH configurations are refused before a single-use code is burned on a request the hub + * would have rejected anyway. + * + * The mint itself is not re-tested here -- it is the existing attested `ocx gui pair` route, and + * `requestPairingGrant` is injected so no proxy, no socket and no real grant is involved. + */ +const GRANT = `ocx_pair_${"A".repeat(43)}`; +const EXPIRES_AT = 1_767_225_600_000; + +const LIVE: LiveProxy = { pid: 4242, port: 10100, hostname: "100.64.0.10", source: "runtime" }; + +function hubConfig(overrides: Partial = {}): OcxConfig { + return { + port: 10100, + hostname: "100.64.0.10", + runtimeRole: "hub", + corsAllowOrigins: ["http://localhost:10100"], + hub: { managementPublicOrigin: "https://hub.tailnet.ts.net" }, + ...overrides, + } as OcxConfig; +} + +async function invite( + args: string[], + config: OcxConfig, + options: { live?: LiveProxy | null; result?: GuiPairRequestResult } = {}, +): Promise<{ code: number; out: string[]; err: string[]; boundOrigin: string | null }> { + const out: string[] = []; + const err: string[] = []; + const log = spyOn(console, "log").mockImplementation((...parts: unknown[]) => { + out.push(parts.map(String).join(" ")); + }); + const error = spyOn(console, "error").mockImplementation((...parts: unknown[]) => { + err.push(parts.map(String).join(" ")); + }); + let boundOrigin: string | null = null; + try { + const code = await runHubCommand(args, { + loadConfig: () => config, + findLiveProxy: async () => (options.live === undefined ? LIVE : options.live), + requestPairingGrant: async (_target, browserOrigin) => { + boundOrigin = browserOrigin; + return options.result ?? { + kind: "created", + grant: GRANT, + browserOrigin, + serverOrigin: "https://hub.tailnet.ts.net", + expiresAt: EXPIRES_AT, + }; + }, + }); + return { code, out, err, boundOrigin }; + } finally { + log.mockRestore(); + error.mockRestore(); + } +} + +describe("hub invite argument and origin helpers", () => { + test("parses the documented flags and rejects anything else", () => { + expect(parseHubInviteArgs([])).toEqual({ json: false }); + expect(parseHubInviteArgs(["--json"])).toEqual({ json: true }); + expect(parseHubInviteArgs(["--data-url", "https://a.test:8443", "--clients", "codex"])) + .toEqual({ json: false, dataUrl: "https://a.test:8443", clients: "codex" }); + // A repeated flag, a missing value, and an unknown token are all usage errors rather than + // a silently-dropped argument: the printed command is what the operator will run. + expect(parseHubInviteArgs(["--json", "--json"])).toBeNull(); + expect(parseHubInviteArgs(["--data-url"])).toBeNull(); + expect(parseHubInviteArgs(["--data-url", "--json"])).toBeNull(); + expect(parseHubInviteArgs(["--origin", "x"])).toBeNull(); + }); + + test("clients accepts codex and claude only, and omission means 'do not pass --clients'", () => { + expect(parseInviteClients(undefined)).toEqual([]); + expect(parseInviteClients("codex,claude")).toEqual(["codex", "claude"]); + expect(parseInviteClients("claude")).toEqual(["claude"]); + expect(parseInviteClients("cursor")).toBeNull(); + expect(parseInviteClients("")).toBeNull(); + }); + + test("the transport rule matches the hub's: loopback or HTTPS, nothing else", () => { + expect(pairingOriginUsable("https://hub.tailnet.ts.net")).toBe(true); + expect(pairingOriginUsable("http://127.0.0.1:10101")).toBe(true); + expect(pairingOriginUsable("http://localhost:10101")).toBe(true); + expect(pairingOriginUsable("http://100.64.0.10:10101")).toBe(false); + }); + + test("the derived data origin is the bind address, with loopback spelled as localhost", () => { + expect(derivedHubDataOrigin("100.64.0.10", 10100)).toBe("http://100.64.0.10:10100"); + expect(derivedHubDataOrigin("0.0.0.0", 10100)).toBe("http://localhost:10100"); + expect(derivedHubDataOrigin("fd7a::1", 8443)).toBe("http://[fd7a::1]:8443"); + }); + + test("the grant binds to the connecting machine's loopback origin, not the hub's", () => { + // `ocx connect` sends Origin: http://localhost: (client/connect.ts + // localGuiOrigin), so only a loopback entry in the hub's allow-list can ever match. + expect(selectInviteBrowserOrigin(hubConfig())).toBe("http://localhost:10100"); + expect(selectInviteBrowserOrigin(hubConfig({ corsAllowOrigins: ["http://localhost:9999"] }))) + .toBe("http://localhost:9999"); + expect(selectInviteBrowserOrigin(hubConfig({ corsAllowOrigins: ["https://elsewhere.test"] }))) + .toBeNull(); + }); + + test("the printed command carries --clients only when the operator asked for it", () => { + expect(hubInviteCommand(GRANT, "https://d.test:8443", "https://m.test", [])) + .toBe(`echo '${GRANT}' | ocx connect https://d.test:8443 --management-url https://m.test --pairing-code-stdin`); + expect(hubInviteCommand(GRANT, "https://d.test:8443", "https://m.test", ["codex"])) + .toContain("--clients codex --pairing-code-stdin"); + }); +}); + +describe("hub invite output", () => { + test("prints a runnable connect line and keeps the code off stderr", async () => { + const { code, out, err, boundOrigin } = await invite(["invite"], hubConfig()); + expect(code).toBe(0); + expect(boundOrigin).toBe("http://localhost:10100"); + expect(out.join("\n")).toContain("# Run on the other machine:"); + expect(out.join("\n")).toContain( + `echo '${GRANT}' | ocx connect http://100.64.0.10:10100 --management-url https://hub.tailnet.ts.net --pairing-code-stdin`, + ); + // The warning is advice, not output a script should capture. + expect(err.join("\n")).toContain("single-use"); + expect(err.join("\n")).not.toContain(GRANT); + }); + + test("hub.dataPublicOrigin replaces the derived origin, and --data-url replaces both", async () => { + const configured = hubConfig({ + hub: { managementPublicOrigin: "https://hub.tailnet.ts.net", dataPublicOrigin: "https://hub.tailnet.ts.net:8443" }, + }); + const fromConfig = await invite(["invite"], configured); + expect(fromConfig.out.join("\n")).toContain("ocx connect https://hub.tailnet.ts.net:8443 "); + + const overridden = await invite(["invite", "--data-url", "https://front.test"], configured); + expect(overridden.out.join("\n")).toContain("ocx connect https://front.test "); + }); + + test("--json emits exactly the documented envelope", async () => { + const { code, out } = await invite(["invite", "--json", "--clients", "codex,claude"], hubConfig()); + expect(code).toBe(0); + expect(JSON.parse(out[0]!)).toEqual({ + code: GRANT, + expiresAt: new Date(EXPIRES_AT).toISOString(), + dataUrl: "http://100.64.0.10:10100", + managementUrl: "https://hub.tailnet.ts.net", + command: `echo '${GRANT}' | ocx connect http://100.64.0.10:10100 --management-url https://hub.tailnet.ts.net --clients codex,claude --pairing-code-stdin`, + }); + }); +}); + +describe("hub invite refuses before burning a code", () => { + test("a non-hub gets one line naming its own role and the command it should run", async () => { + for (const role of [undefined, "standalone", "client"] as const) { + const { code, err } = await invite(["invite"], hubConfig({ runtimeRole: role } as Partial)); + expect(code).toBe(1); + expect(err.join(" ")).toContain("runs on a hub"); + expect(err.join(" ")).toContain("ocx connect"); + } + }); + + test("a hub with no management origin is told which field to set", async () => { + const { code, err } = await invite(["invite"], hubConfig({ hub: {} })); + expect(code).toBe(1); + expect(err.join(" ")).toContain("hub.managementPublicOrigin"); + }); + + test("a --management-url that differs from the configured origin is refused, not printed", async () => { + // The grant's server origin IS hub.managementPublicOrigin, so advertising anything else + // hands out a code the hub then refuses. Saying so beats printing a dud command. + const { code, err } = await invite(["invite", "--management-url", "https://other.test"], hubConfig()); + expect(code).toBe(1); + expect(err.join(" ")).toContain("does not match hub.managementPublicOrigin"); + + const matching = await invite(["invite", "--management-url", "https://hub.tailnet.ts.net"], hubConfig()); + expect(matching.code).toBe(0); + }); + + test("a non-loopback plaintext management origin cannot carry a code", async () => { + const { code, err } = await invite(["invite"], hubConfig({ + hub: { managementPublicOrigin: "http://100.64.0.10:10101" }, + })); + expect(code).toBe(1); + expect(err.join(" ")).toContain("plain HTTP"); + }); + + test("a hub whose allow-list names no loopback origin is told exactly what to add", async () => { + const { code, err } = await invite(["invite"], hubConfig({ corsAllowOrigins: [] })); + expect(code).toBe(1); + expect(err.join(" ")).toContain("corsAllowOrigins"); + expect(err.join(" ")).toContain("http://localhost:10100"); + }); + + test("no running hub, a malformed origin, and a refused mint each exit 1 with a reason", async () => { + const down = await invite(["invite"], hubConfig(), { live: null }); + expect(down.code).toBe(1); + expect(down.err.join(" ")).toContain("No running attested OpenCodex hub"); + + const bad = await invite(["invite", "--data-url", "https://front.test/path"], hubConfig()); + expect(bad.code).toBe(1); + expect(bad.err.join(" ")).toContain("--data-url must be a bare http(s) origin"); + + const refused = await invite(["invite"], hubConfig(), { + result: { kind: "unavailable", reason: "attestation" }, + }); + expect(refused.code).toBe(1); + expect(refused.err.join(" ")).toContain("(attestation)"); + }); + + test("an unknown subcommand prints usage rather than guessing invite", async () => { + for (const args of [[], ["status"], ["invite-machine"]]) { + const { code, err } = await invite(args, hubConfig()); + expect(code).toBe(1); + expect(err.join(" ")).toContain("ocx hub invite"); + } + }); +}); diff --git a/tests/fixtures/test-layout-expected.json b/tests/fixtures/test-layout-expected.json index 94dc99f8a1..034fb57f71 100644 --- a/tests/fixtures/test-layout-expected.json +++ b/tests/fixtures/test-layout-expected.json @@ -539,6 +539,7 @@ "history-ocx-compaction-recovery.test.ts": "codex-integration", "hyperbolic-provider.test.ts": "providers", "hub-gated-local-clients.test.ts": "cli", + "hub-invite.test.ts": "cli", "identity-neutralize.test.ts": "adapters", "init-backup-cleanup.test.ts": "service", "init-eof.test.ts": "service", diff --git a/tests/server/config.test.ts b/tests/server/config.test.ts index 1b1699e0b8..33643fb495 100644 --- a/tests/server/config.test.ts +++ b/tests/server/config.test.ts @@ -435,6 +435,35 @@ describe("opencodex config defaults", () => { }).ok).toBe(true); }); + test("hub.dataPublicOrigin normalizes like the management origin and rejects the same shapes", () => { + // The advertised DATA origin is a separate socket from management on a real deployment + // (tailnet bind behind its own TLS port), so it is its own field rather than a derivation. + expect(validateConfigCandidate({ + ...getDefaultConfig(), + runtimeRole: "hub", + hub: { + managementPublicOrigin: "https://hub.example.test", + dataPublicOrigin: "https://hub.example.test:8443", + }, + })).toMatchObject({ + ok: true, + config: { hub: { dataPublicOrigin: "https://hub.example.test:8443" } }, + }); + // NOT `.catch`ed: silently dropping a typo would make `ocx hub invite` fall back to + // http://:, which is the value the operator set the field to replace. + for (const dataPublicOrigin of [ + "ftp://hub.example.test", + "https://user@hub.example.test", + "https://hub.example.test:8443/path", + "https://hub.example.test:8443/?query=1", + "https://hub.example.test:8443/#fragment", + ]) { + const result = validateConfigCandidate({ ...getDefaultConfig(), hub: { dataPublicOrigin } }); + expect(result.ok).toBe(false); + if (!result.ok) expect(result.error).toContain("hub.dataPublicOrigin"); + } + }); + test("remote GUI live candidates reject unsafe origins and malformed identity allowlists", () => { for (const managementPublicOrigin of [ "ftp://hub.example.test", diff --git a/tests/service/service-secrets.test.ts b/tests/service/service-secrets.test.ts index 029cd6d7f2..ef43c39aca 100644 --- a/tests/service/service-secrets.test.ts +++ b/tests/service/service-secrets.test.ts @@ -21,6 +21,7 @@ import { removeServiceApiTokenFileIfOwned, serviceApiTokenFilePath, serviceApiTokenFingerprint, + startupDataPlaneToken, writeServiceApiTokenFile, writeTokenBackup, } from "../../src/lib/service-secrets"; @@ -38,6 +39,42 @@ afterEach(() => { if (home) removeTreeWithRetry(home); }); +/** + * #4236. A service boot always saw OPENCODEX_API_AUTH_TOKEN, because the launchd plist and the + * systemd unit cat the token file into the environment before exec. A foreground `ocx start` saw + * neither, so `assertServerAuthConfig` refused to bind a non-loopback hostname that the installed + * service on the same machine was serving happily. These pin the precedence that closes that. + */ +describe("startup data-plane token resolution", () => { + const TOKEN = "a".repeat(64); + + test("the environment wins, and nothing is re-exported when it already holds a token", () => { + writeServiceApiTokenFile(TOKEN); + expect(startupDataPlaneToken({ OPENCODEX_API_AUTH_TOKEN: "already" }, { authRequired: true })).toBeNull(); + }); + + test("OCX_API_TOKEN_FILE still wins over the installed path (WinSW native mode)", () => { + writeServiceApiTokenFile(TOKEN); + const named = join(home, "named-token"); + writeFileSync(named, "from-the-named-file\n", "utf8"); + expect(startupDataPlaneToken({ OCX_API_TOKEN_FILE: named }, { authRequired: true })).toBe("from-the-named-file"); + }); + + test("the installed token is used when admission is required, and ignored when it is not", () => { + writeServiceApiTokenFile(TOKEN); + expect(startupDataPlaneToken({}, { authRequired: true })).toBe(TOKEN); + // A loopback bind needs no credential, and on a machine connected to a hub this same file + // holds that hub's issued CLIENT key -- which is not this proxy's admission secret. + expect(startupDataPlaneToken({}, { authRequired: false })).toBeNull(); + }); + + test("an absent or unusable token file resolves to null rather than throwing at boot", () => { + expect(startupDataPlaneToken({}, { authRequired: true })).toBeNull(); + writeFileSync(serviceApiTokenFilePath(), "\n", "utf8"); + expect(startupDataPlaneToken({}, { authRequired: true })).toBeNull(); + }); +}); + describe("service API token ownership", () => { test("writes only the exact owner path through an atomic owner-only replacement", () => { const token = "ocx_data_0123456789abcdef0123456789abcdef01234567"; diff --git a/tests/service/service.test.ts b/tests/service/service.test.ts index f15a243e4d..8acfdb41a0 100644 --- a/tests/service/service.test.ts +++ b/tests/service/service.test.ts @@ -9,7 +9,7 @@ import { saveConfig } from "../../src/config"; import { windowsEnvIndirectBatchValue } from "../../src/lib/win-paths"; import { assertServiceAuthEnvironment, assertServiceEnvironmentMatchesInstall, bakedServicePathsDiagnostic, confirmServiceServing, launchdListenPort, systemdListenPort, buildPlist, buildUnit, buildWindowsLauncherVbs, buildWindowsSchtasksCreateArgs, buildWindowsSchtasksCreateArgsForXml, buildWindowsServiceScript, buildWindowsTaskXml as buildWindowsTaskXmlProduction, buildWindowsTaskXmlDocument, deriveWindowsServiceDiagnostic, deriveWindowsServiceDiagnosticForCurrentUser, expectedLaunchdCommand, installFreshWindowsSchedulerSafely, installServiceSafely, launchctlLoadFailed, launchdJobMatchesPlist, normalizeServiceSubcommand, parseServiceArgs, parseServiceInstallState, planServiceCommand, prepareServiceInstall, probeServiceInstallation, readWindowsSchedulerXmlState, registerFreshWindowsSchedulerTask, removeNativeWindowsServiceForScheduler, repairService, reportServiceServing, resolveServiceListenPort, runLaunchctl, selectServiceSubcommand, SERVICE_INSTALL_HEALTH_MS, SERVICE_INSTALL_HEALTH_WINDOWS_MS, serviceInstallHealthMs, serviceLogPath, serviceStartableFromTray, serviceStatusReport, serviceRetryCommand, serviceStatusSummary, stableLauncherEntry, systemdNeedsDaemonReload, systemdServiceInstallCleanupOps, uninstallSystemd, windowsListenPort, winswListenPort, startLaunchd, windowsTaskRegistrationHealthy as windowsTaskRegistrationHealthyProduction } from "../../src/service"; import type { ServiceDiagnostic } from "../../src/service"; -import { definitionCarriesCredential, resolvedProxyEnv, writeServiceDefinitionFile } from "../../src/service"; +import { definitionCarriesCredential, installLaunchd, resolvedProxyEnv, writeServiceApiTokenFile, writeServiceDefinitionFile } from "../../src/service"; import { buildWinswXml } from "../../src/lib/winsw"; import { CONFIG_OWNER_FILE, CONFIG_UNINSTALL_MANIFEST, recordOwnedConfigPath, removeOwnedConfigState } from "../../src/lib/config-ownership"; import { serviceApiTokenFilePath } from "../../src/lib/service-secrets"; @@ -490,7 +490,84 @@ describe("systemd service unit", () => { }); describe("service install auth preflight", () => { - test("rejects non-loopback service install without a persisted API token", () => { + /** + * #4236. The preflight used to DEMAND OPENCODEX_API_AUTH_TOKEN here, which is what taught an + * operator to export the ADMIN token to make `install` proceed -- and then `repair` demanded it + * again. Nobody should have to export a token by hand to run a hub, so install provisions one. + */ + test("a non-loopback install no longer demands the env token", () => { + if (existsSync(TEST_DIR)) removeTreeWithRetry(TEST_DIR); + mkdirSync(TEST_DIR, { recursive: true }); + process.env.OPENCODEX_HOME = TEST_DIR; + delete process.env.OPENCODEX_API_AUTH_TOKEN; + saveConfig({ + port: 10100, + hostname: "0.0.0.0", + providers: { openai: { adapter: "openai-chat", baseUrl: "https://api.example.test/v1" } }, + defaultProvider: "openai", + } as OcxConfig); + + expect(() => assertServiceAuthEnvironment()).not.toThrow(); + }); + + test("provisioning generates an owner-only token, then reuses it on repair and reinstall", () => { + if (existsSync(TEST_DIR)) removeTreeWithRetry(TEST_DIR); + mkdirSync(TEST_DIR, { recursive: true }); + process.env.OPENCODEX_HOME = TEST_DIR; + delete process.env.OPENCODEX_API_AUTH_TOKEN; + saveConfig({ + port: 10100, + hostname: "0.0.0.0", + providers: { openai: { adapter: "openai-chat", baseUrl: "https://api.example.test/v1" } }, + defaultProvider: "openai", + } as OcxConfig); + + const first = writeServiceApiTokenFile(); + expect(first).toEqual({ path: serviceApiTokenFilePath(), origin: "generated" }); + const token = readFileSync(serviceApiTokenFilePath(), "utf8").trim(); + // 32 random bytes, hex. + expect(token).toMatch(/^[0-9a-f]{64}$/); + if (process.platform !== "win32") { + expect(statSync(serviceApiTokenFilePath()).mode & 0o777).toBe(0o600); + } + + // Repair/reinstall must be idempotent: regenerating would silently invalidate every + // client key exchange already performed against the old value. + expect(writeServiceApiTokenFile()).toEqual({ path: serviceApiTokenFilePath(), origin: "file" }); + expect(readFileSync(serviceApiTokenFilePath(), "utf8").trim()).toBe(token); + expect(() => assertServiceAuthEnvironment()).not.toThrow(); + }); + + test("an env token still wins, and a loopback install generates nothing", () => { + if (existsSync(TEST_DIR)) removeTreeWithRetry(TEST_DIR); + mkdirSync(TEST_DIR, { recursive: true }); + process.env.OPENCODEX_HOME = TEST_DIR; + process.env.OPENCODEX_API_AUTH_TOKEN = "operator-chosen-data-key"; + saveConfig({ + port: 10100, + hostname: "0.0.0.0", + providers: { openai: { adapter: "openai-chat", baseUrl: "https://api.example.test/v1" } }, + defaultProvider: "openai", + } as OcxConfig); + expect(writeServiceApiTokenFile()).toEqual({ path: serviceApiTokenFilePath(), origin: "env" }); + expect(readFileSync(serviceApiTokenFilePath(), "utf8").trim()).toBe("operator-chosen-data-key"); + + // Loopback needs no data-plane credential, and on a machine connected to a hub the same + // file holds that hub's issued client key: a local install must not invent or clobber one. + if (existsSync(TEST_DIR)) removeTreeWithRetry(TEST_DIR); + mkdirSync(TEST_DIR, { recursive: true }); + delete process.env.OPENCODEX_API_AUTH_TOKEN; + saveConfig({ + port: 10100, + hostname: "127.0.0.1", + providers: { openai: { adapter: "openai-chat", baseUrl: "https://api.example.test/v1" } }, + defaultProvider: "openai", + } as OcxConfig); + expect(writeServiceApiTokenFile()).toBeNull(); + expect(existsSync(serviceApiTokenFilePath())).toBe(false); + }); + + test("an unusable token file is reported by the preflight instead of failing mid-install", () => { if (existsSync(TEST_DIR)) removeTreeWithRetry(TEST_DIR); mkdirSync(TEST_DIR, { recursive: true }); process.env.OPENCODEX_HOME = TEST_DIR; @@ -501,8 +578,30 @@ describe("service install auth preflight", () => { providers: { openai: { adapter: "openai-chat", baseUrl: "https://api.example.test/v1" } }, defaultProvider: "openai", } as OcxConfig); + writeFileSync(serviceApiTokenFilePath(), "\n", "utf8"); + + expect(() => assertServiceAuthEnvironment()).toThrow(/cannot be used/); + expect(() => assertServiceAuthEnvironment()).toThrow(/ocx service/); + expect(() => writeServiceApiTokenFile()).toThrow(/empty/); + }); + + test("the admin-token refusal tells the operator to unset, not to invent a key", () => { + if (existsSync(TEST_DIR)) removeTreeWithRetry(TEST_DIR); + mkdirSync(TEST_DIR, { recursive: true }); + process.env.OPENCODEX_HOME = TEST_DIR; + process.env.OPENCODEX_API_AUTH_TOKEN = `ocx_admin_${"f".repeat(40)}`; + saveConfig({ + port: 10100, + hostname: "0.0.0.0", + providers: { openai: { adapter: "openai-chat", baseUrl: "https://api.example.test/v1" } }, + defaultProvider: "openai", + } as OcxConfig); - expect(() => assertServiceAuthEnvironment()).toThrow("OPENCODEX_API_AUTH_TOKEN"); + expect(() => assertServiceAuthEnvironment()).toThrow(/unset OPENCODEX_API_AUTH_TOKEN/); + expect(() => assertServiceAuthEnvironment()).toThrow(/provisions/); + // The chokepoint refuses it too, so no caller can write the broken state (#2696). + expect(() => writeServiceApiTokenFile()).toThrow(/management \(admin\) token/); + expect(existsSync(serviceApiTokenFilePath())).toBe(false); }); test("allows non-loopback service install when the API token is in the service environment", () => { From cd58bd17d7b1e84d3d4bcf3c637e233320efb0eb Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Fri, 11 Sep 2026 11:44:04 +0900 Subject: [PATCH 07/12] docs(devlog): record the hub token UX unit (PR4) What shipped, why the admin-token demand was the defect rather than the refusal, why no new management route was needed, the base discrepancy (PR1 is not in this branch's ancestry despite the assignment), exact verification commands with counts, the one pre-existing server-auth failure, and what the docs PR still owes. Refs #4236 Co-Authored-By: Claude Fable 5.1 --- .../040_hub_token_ux.md | 261 ++++++++++++++++++ 1 file changed, 261 insertions(+) create mode 100644 devlog/_plan/260911_hub_single_port/040_hub_token_ux.md diff --git a/devlog/_plan/260911_hub_single_port/040_hub_token_ux.md b/devlog/_plan/260911_hub_single_port/040_hub_token_ux.md new file mode 100644 index 0000000000..90f2238b18 --- /dev/null +++ b/devlog/_plan/260911_hub_single_port/040_hub_token_ux.md @@ -0,0 +1,261 @@ +# 040 — PR4: data-plane token provisioning, `ocx hub invite`, the status hub block + +Unit: `devlog/_plan/260911_hub_single_port`. Stack position 4 of 5 as actually built (PR1 launchd +repair, PR2 same-port loopback companion, PR3 Claude/local-client destinations — written +concurrently by another agent — this one, then the docs/skill PR). Branch +`codex/260911-l4-hub-token-ux`, rebased onto `codex/260911-l4-hub-loopback-companion` = +`0aca6afe4` (`fix(codex): report the hub gate only when the toggle is on, and refuse every +wildcard spelling`) after that branch grew a commit mid-work; every count below is from the +rebased tree, and the rebase was clean (no shared files). Issue: lidge-jun/opencodex#4236. + +**Base discrepancy, recorded because it affects review.** The assignment described the base as +"dev + PR1 launchd fix + PR2 loopback companion". `git log babb76449..HEAD` on that branch shows +only the six PR2 commits; `src/service.ts` is untouched by it, and no +`devlog/_plan/260911_hub_single_port/010_launchd_repair.md` exists. So PR1 is **not** in this +branch's ancestry, and everything below was written and verified against dev + PR2 only. The +`src/service.ts` hunks here are all inside `assertNotAdminToken` / +`assertServiceAuthEnvironment` / `writeServiceApiTokenFile`, which the PR1 description (macOS +repair and status) does not name, so a later stack reorder should merge cleanly — but it has not +been proven against PR1's diff. + +## The incident this closes + +On the maintainer's hub the operator exported the **admin** token as +`OPENCODEX_API_AUTH_TOKEN`, because `ocx service install` refused to proceed without *a* token +and that was the token at hand. `assertNotAdminToken` then crash-looped the hub, and +`ocx service repair` demanded the same environment variable again — so the only remembered way +to make the command proceed was the exact thing that had broken it. + +The refusal was right. The demand was the defect: `assertServiceAuthEnvironment` threw for any +non-loopback hostname with no env token, **even when `~/.opencodex/service-api-token` already +held a perfectly good one**. Nobody should have to export a token by hand to run a hub. + +## What shipped + +### 1. The service provisions its own data-plane token (`src/service.ts`) + +`writeServiceApiTokenFile()` — still the single chokepoint every backend funnels through +(launchd, systemd, the Windows scheduler wrapper, WinSW native) — now resolves a token by +precedence instead of only copying the environment: + +1. `OPENCODEX_API_AUTH_TOKEN` from the installing shell. Still refused outright when it is an + admin token; an operator who deliberately exports a key keeps full control of it. +2. An existing owner-only `service-api-token`. **Reusing it is what makes `repair`, a reinstall + and a restart idempotent** — regenerating would silently invalidate every client key exchange + already performed against the old value. +3. 32 fresh random bytes, hex, written 0600 through the existing hardened writer + (`recordOwnedConfigPath` → `mkdir 0700` → `writeFileSync mode 0600` → `chmod` → + `hardenSecretPath` on Windows). This is the branch that removes the manual step. + +It returns `{ path, origin: "env" | "file" | "generated" }` and logs the **path**, never the +value. A loopback install with no env token still gets nothing: admission is not required there, +and on a machine connected to a hub that same file holds the hub's issued client key, which a +local install must not invent or clobber. + +`assertServiceAuthEnvironment` keeps exactly two refusals — the admin-token collision (checked +before the loopback short-circuit, unchanged) and a token file that exists but is `unsafe`, which +is reported here where the operator can still act rather than failing mid-install. The +"OPENCODEX_API_AUTH_TOKEN is required before installing…" throw is gone. + +The admin-token message now says what to do: `` Run `unset OPENCODEX_API_AUTH_TOKEN` and rerun: +nothing needs to be exported by hand, because the service provisions its own owner-only +data-plane token at . `` The old text offered "or set it to a distinct data-plane key", +which is how the operator got there. + +### 2. A foreground `ocx start` accepts the file-backed token (`src/lib/service-secrets.ts`) + +`assertServerAuthConfig` reads `configuredApiAuthToken`, which reads the environment — and under +a service the environment always has the token, because the launchd plist and the systemd unit +`cat` the file into it before exec. A foreground `ocx start` had neither, so it refused to bind a +non-loopback hostname the installed service on the same machine was serving happily. + +New `startupDataPlaneToken(env, { authRequired })` applies the wrappers' own precedence in one +place: env wins, then `OCX_API_TOKEN_FILE` (WinSW native mode, the existing +`loadServiceTokenFromFile`), then the installed `service-api-token`. `handleStart` calls it with +`isApiAuthRequired(loadConfig())`. + +`authRequired` is passed in rather than recomputed, for two reasons: this module must not load +config, and the installed file is deliberately **not** consulted on a loopback bind — the +connected-client case again. `assertServerAuthConfig` itself is unchanged, so no security-boundary +file moved: the fix is at the env-hydration layer the wrappers already occupy. + +### 3. `ocx hub invite` (`src/cli/hub.ts`, new) + +``` +ocx hub invite [--json] [--data-url ] [--management-url ] [--clients codex,claude] +``` + +Prints the line to run on the other machine: + +``` +# Run on the other machine: +echo '' | ocx connect --management-url --pairing-code-stdin +``` + +**No new management route was needed, and none was added.** The "connect pairing code" is exactly +a GUI pairing grant: `ocx connect --pairing-code-stdin` exchanges it at `POST /opencodex-session` +(`exchangeConnectPairingGrant`), and the mint is `POST /api/gui/pairing-grants` — the attested +local route `ocx gui pair` already drives, authorized by an HMAC over the running proxy's own +attestation secret from the owner-only runtime state file. So `invite` reuses +`requestBoundGuiPairingGrant` verbatim and needs **no admin token and nothing exported in the +shell**, which is strictly better than the admin-token path the plan sketched. `src/server/*` is +untouched by this PR. + +Origins: + +- data: `--data-url` → `hub.dataPublicOrigin` (new optional config) → `http://:`. +- management: `hub.managementPublicOrigin`, and that is not negotiable — `createGuiPairingGrant` + records it as the grant's `serverOrigin` and the exchange compares the request's management + origin against it. `--management-url` is therefore accepted only when it *equals* the + configured origin; a differing value is refused with both origins named, because printing it + would hand out a code the hub then rejects. + +Everything that cannot work is refused **before** a single-use code is minted: a non-hub +`runtimeRole`, a missing `hub.managementPublicOrigin`, a non-loopback plaintext management origin +(the hub's own `isPairingTransportPermitted` rule), a malformed `--data-url`, no running attested +proxy, and — the non-obvious one — a hub whose allow-list admits no loopback browser origin. +`ocx connect` sends `Origin: http://localhost:` (`localGuiOrigin` in +`src/client/connect.ts`), so only `hub.managementPublicOrigin` itself or a loopback entry of +`corsAllowOrigins` can ever match the grant. `selectInviteBrowserOrigin` picks from exactly that +set — `http://localhost:10100` when admitted, else the first admitted loopback origin — and +otherwise says which `ocx config set corsAllowOrigins` line to run. + +`--json` emits `{ code, expiresAt, dataUrl, managementUrl, command }` with `expiresAt` as ISO +8601. The code goes to stdout; the "secret, single-use" warning goes to stderr, matching +`ocx gui pair`. + +### 4. New optional config `hub.dataPublicOrigin` + +Same canonical-origin transform as `managementPublicOrigin` (http(s), no credentials, path, query +or fragment) and deliberately **not** `.catch`ed: silently dropping a typo would make `invite` +fall back to `http://:`, which is the value the field exists to replace. It is +advisory — the origin the hub advertises, never a bind address. It is its own field rather than a +derivation because on a real deployment the two are different sockets: management is a +loopback-only ingress published by Tailscale Serve on 443, data is the tailnet bind fronted on +its own port (`https://hub.tailnet.ts.net:8443`). + +### 5. The `ocx status` hub block (`src/cli/status.ts`) + +`collectHubStatus(config, listen, env)` adds a nullable `hub` field to `CliStatusJson` +(`schemaVersion` stays 1) and `hubStatusLines(hub)` renders it, so the sentences are testable +without spawning the CLI. On the maintainer's live hub: + +``` + Hub: + Data origin: http://localhost:10100 (derived from the bind address) + Loopback listener: ported on http://127.0.0.1:10104 — a second port local clients must be pointed at + Management ingress: http://127.0.0.1:10102 + Management origin: https://macmini.tail19a2d7.ts.net + Data token: present (file) at /.opencodex/service-api-token + Invite a machine: ocx hub invite +``` + +The listener line distinguishes PR2's two forms through `effectiveLoopbackListenerPort` plus the +presence of an explicit `port`: `companion` ("same port as the public listener, no credential +needed locally"), `ported`, and `off`. The token line reports the SOURCE only — +`present (env)` / `present (file)` / `unsafe (file)` / `missing` — with `env` winning, because +that is the service's own precedence: the wrapper exports the file only when the environment has +nothing, so naming the file first would name a source the running process is not using. A test +asserts no token value appears in either the JSON or the lines. + +### 6. Help, registry, capabilities, skill surface + +`ocx hub` is a registry entry with a runner in `DISPATCH_COMMANDS`, two banner lines in +`printUsage`, and an `ocx help hub` topology paragraph (one port; companion listener; the token +file and that it is never copied; how invites work; `--json`). `ocx service` details gained five +lines on token auto-provisioning and the admin-token refusal. `ocx hub invite` is a declared +capability and `bun run skill:surface` regenerated +`skills/ocx/references/01_management_surface.md` (38 → 39 capabilities, 17 → 18 state-changing). + +## Decisions + +- **No new management route, and `src/server/index.ts` untouched.** See above: the route already + exists and its authorization (process attestation) is better suited to a local CLI than the + admin token. This also kept the PR entirely out of the other agent's files. +- **The `hub invite` capability declares `routes: []`.** It really does drive + `POST /api/gui/pairing-grants`, but that route is answered in the composition root ahead of + `handleManagementAPI`, so it is absent from `MANAGEMENT_ROUTES` and + `tests/cli/cli-capabilities.test.ts` would fail the declaration. The omission is explained in a + comment at the declaration rather than papered over; widening the registry's scanned scope to + `src/server/index.ts` is its own change. `ocx gui pair`, which drives the same route, has no + capability entry at all today. +- **`--management-url` is a confirmation, not an override.** The grant is bound to + `hub.managementPublicOrigin`; an override that differs cannot work, so it is refused with both + values named instead of printed. +- **`assertServerAuthConfig` was not touched.** The fix belongs where the wrappers already put + the token (env hydration before bind), not in a per-request admission helper that would then + read a file on every request. +- **An existing token is reused, never regenerated.** Rotation is `ocx connect rotate`'s job; + an install that silently minted a new hub secret would break every connected client. +- **`writeServiceApiTokenFile` kept its name and became exported**, so the provisioning + precedence is covered by behaviour tests rather than a source-oracle. The same identifier + exists in `src/lib/service-secrets.ts` with a different signature; they are in different + modules and a future collision inside `src/service.ts` would be a compile error, which is the + desired failure mode. +- The `ocx hub invite` pairing code is printed to stdout, exactly as `ocx gui pair` prints its + grant. `bun run privacy:scan` is green; no token value is logged anywhere this PR adds. + +## Verification (exact commands, this branch) + +``` +bun x tsc --noEmit # clean +bun run privacy:scan # Privacy scan passed +bun run skill:surface # regenerated, then: +bun test tests/ci-workflows/skill-ocx.test.ts tests/cli/cli-registry.test.ts \ + tests/cli/cli-capabilities.test.ts # 45 pass +bun test tests/service/service.test.ts # 212 pass +bun test tests/service/service-secrets.test.ts # 10 pass +bun test tests/cli/hub-invite.test.ts # 16 pass +bun test tests/cli/cli-status-json.test.ts # 53 pass +bun test tests/server/config.test.ts # 196 pass +bun test tests/cli/cli-dispatch.test.ts tests/cli/cli-help.test.ts # 56 pass +bun test tests/cli/cli-transport-honesty.test.ts # 22 pass +bun test tests/cli/cli-json-contract.test.ts # 8 pass +bun test tests/cli/cli-start-journal-order.test.ts # 3 pass +bun test tests/config/config-user-edits.test.ts # 46 pass +bun test tests/providers/opencode-cli.test.ts # 51 pass +bun test tests/service/winsw.test.ts # 25 pass +bun test tests/test-layout.test.ts tests/test-layout-tooling.test.ts # 17 pass +``` + +`bun test tests/server/server-auth.test.ts` is 111 pass / 1 fail on this branch **and on the +base with the working tree stashed** — `native passthrough upstream reset still logs 502 and +penalizes the pool` is a pre-existing failure, not a regression. + +Run the service and winsw files one at a time. Passing +`tests/service/winsw.test.ts` in the same `bun test` invocation as +`tests/cli/cli-transport-honesty.test.ts` trips the real-home guard through cross-file +`OPENCODEX_HOME` leakage; that is true on the base too. + +New test file registered in `scripts/test-layout/layout.json` and +`tests/fixtures/test-layout-expected.json`: `tests/cli/hub-invite.test.ts`. The hub status block, +the startup token precedence and the `hub.dataPublicOrigin` schema went into the existing +`tests/cli/cli-status-json.test.ts`, `tests/service/service-secrets.test.ts` and +`tests/server/config.test.ts`. + +No repository-wide suite (operator instruction); hosted CI at exact head is the proof. + +### Read-only checks against the live hub + +`ocx service …`, `ocx start`, `ocx ensure` and `ocx sync` were not run, and the live config was +not modified. Two read-only commands were: + +- `bun run src/cli/index.ts status` — rendered the hub block quoted above. +- `bun run src/cli/index.ts hub invite --json` — exited 1 with + `No loopback browser origin is admitted for pairing. Add the connecting machine's local origin: + ocx config set corsAllowOrigins '["http://localhost:10100"]'`, minting nothing. That hub has + `hostname: 127.0.0.1` and no `corsAllowOrigins`, so the refusal is correct; the successful + output was produced against an injected config and an injected mint. + +## Left for the rest of the stack + +- Docs PR: `guides/remote-hub.md` en + ko still tells the operator to + `export OPENCODEX_API_AUTH_TOKEN="$(openssl rand -hex 32)"` before `ocx service install`. That + step is now optional, and the guide should lead with `ocx hub invite` plus the new + `hub.dataPublicOrigin` field in `reference/configuration/server.md`. +- A hub bound to a non-loopback address needs `corsAllowOrigins` to name the connecting machine's + `http://localhost:` before `invite` can mint. That is an existing property of + `createGuiPairingGrant`, surfaced rather than changed here; whether a loopback client origin + should be admitted implicitly is a pairing-policy question for its own change. +- `ocx hub` has exactly one subcommand. Further hub-side verbs (listing or revoking issued client + keys from the CLI rather than the dashboard's **Integrations → API Keys**) are not in this PR. From 08b5f0344451d4e839b5b527d553b1b7e29b6d62 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Fri, 11 Sep 2026 11:50:18 +0900 Subject: [PATCH 08/12] test(service): drop the installLaunchd import the restack left unused Co-Authored-By: Claude Fable 5.1 --- tests/service/service.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/service/service.test.ts b/tests/service/service.test.ts index 8acfdb41a0..e3e7ffee20 100644 --- a/tests/service/service.test.ts +++ b/tests/service/service.test.ts @@ -9,7 +9,7 @@ import { saveConfig } from "../../src/config"; import { windowsEnvIndirectBatchValue } from "../../src/lib/win-paths"; import { assertServiceAuthEnvironment, assertServiceEnvironmentMatchesInstall, bakedServicePathsDiagnostic, confirmServiceServing, launchdListenPort, systemdListenPort, buildPlist, buildUnit, buildWindowsLauncherVbs, buildWindowsSchtasksCreateArgs, buildWindowsSchtasksCreateArgsForXml, buildWindowsServiceScript, buildWindowsTaskXml as buildWindowsTaskXmlProduction, buildWindowsTaskXmlDocument, deriveWindowsServiceDiagnostic, deriveWindowsServiceDiagnosticForCurrentUser, expectedLaunchdCommand, installFreshWindowsSchedulerSafely, installServiceSafely, launchctlLoadFailed, launchdJobMatchesPlist, normalizeServiceSubcommand, parseServiceArgs, parseServiceInstallState, planServiceCommand, prepareServiceInstall, probeServiceInstallation, readWindowsSchedulerXmlState, registerFreshWindowsSchedulerTask, removeNativeWindowsServiceForScheduler, repairService, reportServiceServing, resolveServiceListenPort, runLaunchctl, selectServiceSubcommand, SERVICE_INSTALL_HEALTH_MS, SERVICE_INSTALL_HEALTH_WINDOWS_MS, serviceInstallHealthMs, serviceLogPath, serviceStartableFromTray, serviceStatusReport, serviceRetryCommand, serviceStatusSummary, stableLauncherEntry, systemdNeedsDaemonReload, systemdServiceInstallCleanupOps, uninstallSystemd, windowsListenPort, winswListenPort, startLaunchd, windowsTaskRegistrationHealthy as windowsTaskRegistrationHealthyProduction } from "../../src/service"; import type { ServiceDiagnostic } from "../../src/service"; -import { definitionCarriesCredential, installLaunchd, resolvedProxyEnv, writeServiceApiTokenFile, writeServiceDefinitionFile } from "../../src/service"; +import { definitionCarriesCredential, resolvedProxyEnv, writeServiceApiTokenFile, writeServiceDefinitionFile } from "../../src/service"; import { buildWinswXml } from "../../src/lib/winsw"; import { CONFIG_OWNER_FILE, CONFIG_UNINSTALL_MANIFEST, recordOwnedConfigPath, removeOwnedConfigState } from "../../src/lib/config-ownership"; import { serviceApiTokenFilePath } from "../../src/lib/service-secrets"; From 926c62e192fc98e1636cfab6879a416b3540d8a4 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Fri, 11 Sep 2026 12:10:43 +0900 Subject: [PATCH 09/12] fix(cli): refuse a loopback data origin and name the origin an invite binds MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `derivedHubDataOrigin` sends 0.0.0.0, :: and 127.0.0.1 all through `probeHostname`, which spells every one of them as loopback -- so a hub bound to loopback, or to a wildcard with no `hub.dataPublicOrigin`, printed `ocx connect http://localhost:` under "# Run on the other machine". That tells the other machine to dial ITSELF, and the pairing code is single-use, so it was spent on an exchange that cannot succeed. The maintainer's live hub is exactly that shape. `resolveHubDataOrigin` returns either a usable origin (tagged flag/config/derived) or `loopback-derived`, and the latter is refused BEFORE the mint, naming which shape this hub has. A wildcard bind is refused rather than resolved on purpose: nothing in this repo derives a tailnet or LAN address (`probeHostname` deliberately collapses wildcards to loopback, and there is no `networkInterfaces` caller anywhere), so deriving one here would advertise an interface the operator never chose. An explicit `--data-url` or `hub.dataPublicOrigin` is never second-guessed -- loopback is legitimate over an SSH tunnel. The bound browser origin is now printed on every successful invite, in both modes, on stderr beside the single-use warning so the --json envelope is unchanged. `selectInviteBrowserOrigin` silently falls back to the first admitted loopback origin, and a remote `ocx connect` only ever sends `http://localhost:` -- so a grant bound to anything else was refused at the exchange with nothing printed to explain it. When the bound origin is not the default, the warning names the port the connecting machine must be configured with, and offers admitting the default instead. Both surfaced `ocx config set` lines now actually run. `ocx config set hub.managementPublicOrigin …` exits `config parent path not found: hub` when the `hub` object is absent -- exactly the config being advised -- so `configSetHubLines` prefixes `ocx config set hub '{}'`, as guides/remote-hub.md does, and only when it is genuinely missing. `ocx config set corsAllowOrigins '[…]'` replaces the array, so `appendCorsAllowOriginsCommand` emits the entries already configured plus the new one instead of telling the operator to delete them. Reuse: `canonicalHttpOrigin` is exported once from `src/lib/gui-pair-capability.ts`, beside `canonicalGuiBrowserOrigin`, and the byte-identical private copies in `hub.ts` and `gui-pair-client.ts` are gone. The copies in `src/config.ts` (zod schema path) and `src/server/gui-session.ts` stay for now, so `src/server/*` keeps a zero diff. Refs #4236 Co-Authored-By: Claude Fable 5.1 --- .../ocx/references/01_management_surface.md | 2 + src/cli/capabilities.ts | 2 + src/cli/gui-pair-client.ts | 13 +- src/cli/hub.ts | 163 +++++++++++++++--- src/cli/registry.ts | 7 +- src/lib/gui-pair-capability.ts | 27 +++ 6 files changed, 177 insertions(+), 37 deletions(-) diff --git a/skills/ocx/references/01_management_surface.md b/skills/ocx/references/01_management_surface.md index dbebccdd9b..7fcf629e50 100644 --- a/skills/ocx/references/01_management_surface.md +++ b/skills/ocx/references/01_management_surface.md @@ -411,6 +411,8 @@ JSON mode: `envelope`. - Hub only: refuses when runtimeRole is not hub, and requires a running attested proxy. - The code is secret, single-use and short-lived; it is bound to hub.managementPublicOrigin and to the connecting machine's loopback browser origin. +- The bound browser origin is always printed; when it is not http://localhost:10100 the warning names the port the connecting machine must use. +- Refuses when the advertised data origin would be loopback (a loopback or wildcard bind with no hub.dataPublicOrigin and no --data-url) rather than printing a line that dials the other machine itself. - Prints no data-plane token. Remote machines receive their own revocable per-client key from the exchange. - Mints through the attested local pairing-grant route, the same one ocx gui pair uses; no admin token is read. diff --git a/src/cli/capabilities.ts b/src/cli/capabilities.ts index e88c6568c7..d9aa8d0402 100644 --- a/src/cli/capabilities.ts +++ b/src/cli/capabilities.ts @@ -154,6 +154,8 @@ export const CAPABILITIES: readonly Capability[] = [ details: [ "Hub only: refuses when runtimeRole is not hub, and requires a running attested proxy.", "The code is secret, single-use and short-lived; it is bound to hub.managementPublicOrigin and to the connecting machine's loopback browser origin.", + "The bound browser origin is always printed; when it is not http://localhost:10100 the warning names the port the connecting machine must use.", + "Refuses when the advertised data origin would be loopback (a loopback or wildcard bind with no hub.dataPublicOrigin and no --data-url) rather than printing a line that dials the other machine itself.", "Prints no data-plane token. Remote machines receive their own revocable per-client key from the exchange.", "Mints through the attested local pairing-grant route, the same one ocx gui pair uses; no admin token is read.", ], diff --git a/src/cli/gui-pair-client.ts b/src/cli/gui-pair-client.ts index 87a164059d..4e03c146e4 100644 --- a/src/cli/gui-pair-client.ts +++ b/src/cli/gui-pair-client.ts @@ -17,6 +17,7 @@ import { GUI_PAIR_NONCE_HEADER, GUI_PAIR_PATH, canonicalGuiBrowserOrigin, + canonicalHttpOrigin, createGuiPairCapability, } from "../lib/gui-pair-capability"; import { directLocalHttpFetch } from "../server/direct-local-http"; @@ -52,18 +53,6 @@ function sameRuntime(left: RuntimePortState, right: RuntimePortState | null): bo && timingSafeEqual(leftSecret, rightSecret); } -function canonicalHttpOrigin(value: unknown): string | null { - if (typeof value !== "string") return null; - try { - const parsed = new URL(value); - if (parsed.protocol !== "http:" && parsed.protocol !== "https:") return null; - if (parsed.username || parsed.password || parsed.pathname !== "/" || parsed.search || parsed.hash) return null; - return parsed.origin; - } catch { - return null; - } -} - function parseCreatedResult(value: unknown, browserOrigin: string): GuiPairRequestResult | null { if (!value || typeof value !== "object" || Array.isArray(value)) return null; const record = value as Record; diff --git a/src/cli/hub.ts b/src/cli/hub.ts index f909cf58dc..e3edcedc23 100644 --- a/src/cli/hub.ts +++ b/src/cli/hub.ts @@ -1,5 +1,5 @@ import type { OcxConfig, OcxConnectedClientId } from "../types"; -import { canonicalGuiBrowserOrigin } from "../lib/gui-pair-capability"; +import { canonicalGuiBrowserOrigin, canonicalHttpOrigin } from "../lib/gui-pair-capability"; import { findLiveProxy, probeHostname, type LiveProxy } from "../server/proxy-liveness"; import { requestBoundGuiPairingGrant, @@ -48,19 +48,6 @@ export interface HubInvitePayload { command: string; } -function canonicalOrigin(value: string | undefined): string | null { - if (typeof value !== "string" || !value.trim()) return null; - try { - const parsed = new URL(value); - if (parsed.protocol !== "http:" && parsed.protocol !== "https:") return null; - if (parsed.username || parsed.password || parsed.search || parsed.hash) return null; - if (parsed.pathname !== "" && parsed.pathname !== "/") return null; - return parsed.origin; - } catch { - return null; - } -} - function isLoopbackOrigin(origin: string): boolean { try { const host = new URL(origin).hostname.toLowerCase(); @@ -131,6 +118,110 @@ export function derivedHubDataOrigin(hostname: string | undefined, port: number) return `http://${host === "127.0.0.1" ? "localhost" : host}:${port}`; } +/** + * The `ocx config set` lines that will actually work on THIS config. + * + * `setPath` in `src/cli/config-command.ts` walks only parents that already exist, so + * `ocx config set hub. …` exits with `config parent path not found: hub` on a config + * that has no `hub` object yet — which is exactly the config that needs the advice. Create the + * parent first, the way `guides/remote-hub.md` does, and only when it is actually missing, so + * the operator can paste the lines verbatim either way. + */ +export function configSetHubLines( + config: Pick, + field: "managementPublicOrigin" | "dataPublicOrigin", + example: string, +): string[] { + const set = `ocx config set hub.${field} '${JSON.stringify(example)}'`; + return config.hub ? [set] : ["ocx config set hub '{}'", set]; +} + +/** + * A `corsAllowOrigins` line that ADDS an origin instead of replacing the list. + * + * `ocx config set corsAllowOrigins '[…]'` overwrites the array, so printing a one-element + * literal tells an operator with an existing allow-list to delete it. The already-configured + * entries are known here, so the suggested value carries them. + */ +export function appendCorsAllowOriginsCommand( + config: Pick, + origin: string, +): string { + const current = config.corsAllowOrigins ?? []; + const next = current.includes(origin) ? current : [...current, origin]; + return `ocx config set corsAllowOrigins '${JSON.stringify(next)}'`; +} + +export type HubDataOriginResolution = + | { kind: "usable"; dataUrl: string; source: "flag" | "config" | "derived" } + /** The bind address can only be spelled as loopback, so there is nothing to advertise. */ + | { kind: "loopback-derived"; dataUrl: string; bindHostname: string }; + +/** + * The data origin to advertise, or a refusal. + * + * `derivedHubDataOrigin` maps a loopback bind AND every wildcard spelling to + * `http://localhost:` (via `probeHostname`), which on the other machine means "dial + * yourself". Printing it burns the single-use code on a connect that cannot succeed, so a + * derived loopback origin is a refusal rather than a value. A wildcard bind is refused the same + * way on purpose: nothing in this repo derives a tailnet or LAN address, and guessing one from + * `os.networkInterfaces()` would advertise an interface the operator never chose. + * + * An explicit `--data-url` or `hub.dataPublicOrigin` is never second-guessed — a loopback data + * origin is legitimate when the "other machine" is reached through an SSH tunnel. + */ +export function resolveHubDataOrigin( + override: string | null, + configured: string | undefined, + bindHostname: string | undefined, + port: number, +): HubDataOriginResolution { + if (override) return { kind: "usable", dataUrl: override, source: "flag" }; + const fromConfig = canonicalHttpOrigin(configured); + if (fromConfig) return { kind: "usable", dataUrl: fromConfig, source: "config" }; + const derived = derivedHubDataOrigin(bindHostname, port); + if (!isLoopbackOrigin(derived)) return { kind: "usable", dataUrl: derived, source: "derived" }; + const trimmed = (bindHostname ?? "").trim(); + return { kind: "loopback-derived", dataUrl: derived, bindHostname: trimmed || "127.0.0.1" }; +} + +/** Wildcards and loopback fail for different reasons; say which one this hub has. */ +function bindAddressPhrase(bindHostname: string): string { + return bindHostname === "0.0.0.0" || bindHostname === "::" || bindHostname === "[::]" + ? `the bind address ${bindHostname} is a wildcard, which names no address another machine can dial` + : `the bind address ${bindHostname} is loopback-only`; +} + +/** + * What an operator has to know about the origin the grant actually got bound to. + * + * `selectInviteBrowserOrigin` falls back to the first admitted loopback origin when + * `http://localhost:10100` is not admitted, and a remote `ocx connect` sends + * `Origin: http://localhost:` — so a grant bound to anything else is + * refused at the exchange and the single-use code is spent with nothing printed to explain it. + * Always stating the bound origin, and naming the port the client needs when it differs, is the + * difference between a fixable failure and a mystery. + */ +export function inviteBoundOriginNotes( + browserOrigin: string, + config: Pick, +): string[] { + const notes = [`Bound browser origin: ${browserOrigin} — the connecting machine must present exactly this.`]; + if (browserOrigin === DEFAULT_CLIENT_BROWSER_ORIGIN) return notes; + const parsed = new URL(browserOrigin); + const port = parsed.port || (parsed.protocol === "https:" ? "443" : "80"); + notes.push( + `That is NOT ${DEFAULT_CLIENT_BROWSER_ORIGIN}, which is what an unconfigured client sends: the other machine ` + + `must already be running on port ${port} ('ocx config set port ${port}' there) before it runs the line ` + + "below, or the hub refuses the exchange and the code is spent.", + ); + notes.push( + "To accept a default client instead, admit its origin on this hub: " + + appendCorsAllowOriginsCommand(config, DEFAULT_CLIENT_BROWSER_ORIGIN), + ); + return notes; +} + export function hubInviteCommand( code: string, dataUrl: string, @@ -163,15 +254,18 @@ async function runInvite(args: string[], deps: HubCommandDeps): Promise // The grant's server origin IS hub.managementPublicOrigin (createGuiPairingGrant reads it, // and the exchange compares the request's management origin against it), so an invite that // advertised anything else would hand out a code the hub then refuses. - const managementPublic = canonicalOrigin(config.hub?.managementPublicOrigin); + const managementPublic = canonicalHttpOrigin(config.hub?.managementPublicOrigin); if (!managementPublic) { console.error( "hub.managementPublicOrigin is not set, so there is no origin to pair against. Set the exact " - + "browser-visible HTTPS origin: ocx config set hub.managementPublicOrigin '\"https://hub.tailnet.ts.net\"'", + + "browser-visible HTTPS origin:", ); + for (const line of configSetHubLines(config, "managementPublicOrigin", "https://hub.tailnet.ts.net")) { + console.error(` ${line}`); + } return 1; } - const managementOverride = options.managementUrl === undefined ? null : canonicalOrigin(options.managementUrl); + const managementOverride = options.managementUrl === undefined ? null : canonicalHttpOrigin(options.managementUrl); if (options.managementUrl !== undefined && !managementOverride) { console.error("--management-url must be a bare http(s) origin with no path, query, or credentials."); return 1; @@ -190,16 +284,19 @@ async function runInvite(args: string[], deps: HubCommandDeps): Promise ); return 1; } - const dataOverride = options.dataUrl === undefined ? null : canonicalOrigin(options.dataUrl); + const dataOverride = options.dataUrl === undefined ? null : canonicalHttpOrigin(options.dataUrl); if (options.dataUrl !== undefined && !dataOverride) { console.error("--data-url must be a bare http(s) origin with no path, query, or credentials."); return 1; } const browserOrigin = selectInviteBrowserOrigin(config); if (!browserOrigin) { + // `ocx config set corsAllowOrigins` REPLACES the array, so the suggested value carries the + // entries this hub already has -- a one-element literal would tell the operator to drop them. console.error( - "No loopback browser origin is admitted for pairing. Add the connecting machine's local origin: " - + `ocx config set corsAllowOrigins '["${DEFAULT_CLIENT_BROWSER_ORIGIN}"]'`, + "No loopback browser origin is admitted for pairing. Add the connecting machine's local origin " + + "(this keeps the entries already configured; 'ocx config get corsAllowOrigins' shows them): " + + appendCorsAllowOriginsCommand(config, DEFAULT_CLIENT_BROWSER_ORIGIN), ); return 1; } @@ -208,9 +305,26 @@ async function runInvite(args: string[], deps: HubCommandDeps): Promise console.error("No running attested OpenCodex hub was found. Check 'ocx service status', then 'ocx service repair'."); return 1; } - const dataUrl = dataOverride - ?? canonicalOrigin(config.hub?.dataPublicOrigin) - ?? derivedHubDataOrigin(target.hostname ?? config.hostname, target.port); + const resolved = resolveHubDataOrigin( + dataOverride, + config.hub?.dataPublicOrigin, + target.hostname ?? config.hostname, + target.port, + ); + if (resolved.kind === "loopback-derived") { + console.error( + `The advertised data origin would be ${resolved.dataUrl} — this machine's own loopback — because ` + + `${bindAddressPhrase(resolved.bindHostname)}, and nothing here guesses a tailnet or LAN address. ` + + "The other machine would dial itself and the single-use code would be spent for nothing. Name the " + + "origin remote machines reach this hub's data plane on:", + ); + for (const line of configSetHubLines(config, "dataPublicOrigin", "https://hub.tailnet.ts.net:8443")) { + console.error(` ${line}`); + } + console.error(" ...or, for this invite only: ocx hub invite --data-url https://hub.tailnet.ts.net:8443"); + return 1; + } + const dataUrl = resolved.dataUrl; const result = await (deps.requestPairingGrant ?? requestBoundGuiPairingGrant)(target, browserOrigin, { ...(deps.fetchImpl ? { fetchImpl: deps.fetchImpl } : {}), }); @@ -225,6 +339,9 @@ async function runInvite(args: string[], deps: HubCommandDeps): Promise managementUrl: managementPublic, command: hubInviteCommand(result.grant, dataUrl, managementPublic, clients), }; + // Always, in both modes: the grant is bound to ONE browser origin and the operator cannot + // see which from the printed command (#4236 review). + for (const note of inviteBoundOriginNotes(browserOrigin, config)) console.error(note); if (options.json) { console.log(JSON.stringify(payload)); console.error(PAIRING_WARNING); diff --git a/src/cli/registry.ts b/src/cli/registry.ts index 068a7fee13..7726fd1a47 100644 --- a/src/cli/registry.ts +++ b/src/cli/registry.ts @@ -180,9 +180,12 @@ export const CLI_COMMANDS: CliCommandEntry[] = [ "invite requires a running hub and mints a single-use, short-lived pairing code through the same", "attested local route `ocx gui pair` uses, then prints the exact command to run on the other", "machine. The code is bound to hub.managementPublicOrigin and to the connecting machine's local", - "browser origin (`http://localhost:10100` unless corsAllowOrigins names another loopback origin).", + "browser origin (`http://localhost:10100` unless corsAllowOrigins names another loopback origin);", + "the bound origin is always printed, and a different one names the port the client needs.", "--data-url overrides the advertised data origin; hub.dataPublicOrigin is the persistent form, and", - "the fallback is http://:.", + "the fallback is http://:. A loopback or wildcard bind has no such address, so", + "invite refuses instead of advertising http://localhost:, which would tell the other machine", + "to dial itself and spend the code.", "--json emits { code, expiresAt, dataUrl, managementUrl, command }. Do not persist the code.", "Hub state, including the data token and the companion listener, is reported by `ocx status`.", ], diff --git a/src/lib/gui-pair-capability.ts b/src/lib/gui-pair-capability.ts index d3a409e595..9acb526258 100644 --- a/src/lib/gui-pair-capability.ts +++ b/src/lib/gui-pair-capability.ts @@ -38,6 +38,33 @@ export function canonicalGuiBrowserOrigin(value: unknown): string | null { } } +/** + * A bare http(s) origin, or null. + * + * Stricter than {@link canonicalGuiBrowserOrigin}: that one also accepts non-HTTP schemes + * (a packaged app's custom scheme can be a browser origin), while this is the rule for an + * origin that will be DIALLED — the hub's management and data origins, and the `serverOrigin` + * a pairing grant is bound to. Credentials, a path, a query or a fragment are all rejected + * rather than silently dropped, because every caller goes on to print or compare the result. + * + * Exported here, beside the browser-origin canonicaliser, because this file is the only + * module the CLI pairing path and the hub command already share. `src/config.ts` and + * `src/server/gui-session.ts` still hold byte-identical private copies; folding those in + * would pull a heavy config import into a server security-boundary file, so it is its own + * change rather than a drive-by in a token-UX PR. + */ +export function canonicalHttpOrigin(value: unknown): string | null { + if (typeof value !== "string") return null; + try { + const parsed = new URL(value); + if (parsed.protocol !== "http:" && parsed.protocol !== "https:") return null; + if (parsed.username || parsed.password || parsed.pathname !== "/" || parsed.search || parsed.hash) return null; + return parsed.origin; + } catch { + return null; + } +} + function capabilityPayload( nonce: string, method: string, From db3a8991edd0925728e6ab60c7d89bcc165f6462 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Fri, 11 Sep 2026 12:10:56 +0900 Subject: [PATCH 10/12] fix(service,cli): re-check the admin-token collision on the reused token file The `origin: "file"` branch of `writeServiceApiTokenFile` returned early without calling `assertNotAdminToken`, so a `service-api-token` that already held the MANAGEMENT token -- hand-pasted before #2696, or written by the very incident this unit closes -- was silently accepted: `ocx status` said `present (file)` and the hub crash-looped at boot with nothing naming the cause. That is the #4236 incident shape, still reachable. `assertNotAdminToken` takes the source now, because the remedy differs rather than the rule: `unset OPENCODEX_API_AUTH_TOKEN` is meaningless advice about a file, so the file message says the file holds the management token and to delete it and rerun `ocx service repair` (or `install`). It deliberately does not call `serviceRetryCommand()`, which would drag `diagnoseService()` -- and `launchctl` -- into an error path that runs during install. Both collision checks moved ahead of the loopback short-circuit in `assertServiceAuthEnvironment`: `buildServiceShellCommand` cats the token file into OPENCODEX_API_AUTH_TOKEN whenever the file exists, WHATEVER the hostname, so a loopback install with an admin-token file fences its management plane closed at boot just the same. On a machine connected to a hub that file holds the hub's issued per-client key, never a management token, so the check is a no-op there. The reused file is `chmod 0600`'d best-effort on the way through. `readServiceApiTokenState` accepts any bounded regular file, so a reused token could be group- or world-readable while install printed "owner-only"; best-effort because a non-owner cannot chmod and failing the install over a loose mode would be worse. `ocx status`: `HubDataTokenState` gains `admin-collision (file)` and the hub block adds two lines naming the consequence and the fix, because that is what a crash-looping hub looks like from status output and nothing else in the report said so. And the token state is now always about the FILE: `present (env)` was reported whenever the calling shell exported the variable, but the plist and the unit overwrite it from the file before exec, so the label described the operator's terminal rather than the hub. The shell's variable survives as its own field, `dataTokenEnvInShell`, rendered as a sub-line -- it does decide what a foreground `ocx start` in that same shell admits. Refs #4236 Co-Authored-By: Claude Fable 5.1 --- src/cli/status.ts | 66 ++++++++++++++++++++++++++++++++++++----------- src/service.ts | 52 +++++++++++++++++++++++++++++++++---- 2 files changed, 98 insertions(+), 20 deletions(-) diff --git a/src/cli/status.ts b/src/cli/status.ts index 420388f6c7..1b794eca25 100644 --- a/src/cli/status.ts +++ b/src/cli/status.ts @@ -19,12 +19,28 @@ import { claudeDesktopIntegrationEnabled } from "../codex/desired-state"; import { claudeDesktopPolicyHealth, probeClaudeDesktopPolicy, type ClaudeDesktopPolicyHealth } from "../claude/desktop-policy"; import { collectClientConnectionStatus } from "./connect"; import { readServiceApiTokenState, serviceApiTokenFilePath } from "../lib/service-secrets"; +import { tokenCollidesWithAdmin } from "../lib/admin-secrets"; export { proxyHealthFailureReason, isConnectionRefused, isUncleanExitEvidence, probeUncleanExitState } from "./status-probes"; export type { ListenTarget } from "./status-probes"; import { checkProxyHealth, probeUncleanExitState, type ListenTarget } from "./status-probes"; -/** Where the data-plane admission secret comes from. Source only -- never the value. */ -export type HubDataTokenState = "present (env)" | "present (file)" | "unsafe (file)" | "missing"; +/** + * The state of the data-plane admission secret the SERVICE will use. State only -- never the value. + * + * Always about the file, because the file is what the service reads: the launchd plist and the + * systemd unit `cat` it into `OPENCODEX_API_AUTH_TOKEN` before exec, so a token in the CLI's own + * shell says nothing about the running hub. `present (env)` used to be reported here and was + * simply wrong about whose environment it meant (see `dataTokenEnvInShell`). + * + * `admin-collision (file)` is the #4236 incident shape: the file holds the MANAGEMENT token, so + * the server fences the whole management plane closed at boot and the hub crash-loops. It used + * to report `present (file)`, which is how the cause stayed invisible. + */ +export type HubDataTokenState = + | "present (file)" + | "unsafe (file)" + | "admin-collision (file)" + | "missing"; export type HubStatus = { /** Advertised data origin: hub.dataPublicOrigin, else derived from the bind address. */ @@ -41,6 +57,12 @@ export type HubStatus = { managementPublicOrigin: string | null; dataToken: HubDataTokenState; dataTokenPath: string; + /** + * `OPENCODEX_API_AUTH_TOKEN` is set in the shell that ran `ocx status` — which is NOT the + * environment the installed service runs in. Reported separately, and honestly, because it + * does decide what a FOREGROUND `ocx start` in this same shell would admit. + */ + dataTokenEnvInShell: boolean; }; export type CliStatusJson = { @@ -156,13 +178,14 @@ function statusDashboardUrl(config: StatusListenConfig, hostname: string | undef /** * The hub block, or null when this machine is not a hub. * - * `env` wins over `file` because that is the service's own precedence - * (`writeServiceApiTokenFile`): the launch wrapper exports OPENCODEX_API_AUTH_TOKEN from the - * file only when the calling environment has none, so reporting the file first would name a - * source the running process is not using. + * The token line is about the FILE, not this shell. `ocx status` used to print `present (env)` + * whenever the calling shell happened to export `OPENCODEX_API_AUTH_TOKEN`, but the service + * wrapper overwrites that variable from the token file before exec — so the label described the + * operator's terminal and not the hub. The shell's variable is reported as its own flag instead. * - * The token VALUE is never read into the report. `readServiceApiTokenState` returns it, and the - * only thing taken from that result is `kind`. + * The token VALUE is never read into the report. `readServiceApiTokenState` returns it; the only + * things derived from it are `kind` and the admin-token comparison, neither of which can carry + * bytes of the secret. */ export function collectHubStatus( config: Pick, @@ -175,12 +198,12 @@ export function collectHubStatus( const ingress = config.hub?.managementIngress; const configuredDataOrigin = config.hub?.dataPublicOrigin; const host = probeHostname(listen.hostname ?? config.hostname); - const tokenState = env.OPENCODEX_API_AUTH_TOKEN?.trim() - ? "present (env)" as const - : ((): HubDataTokenState => { - const state = readServiceApiTokenState(); - return state.kind === "present" ? "present (file)" : state.kind === "unsafe" ? "unsafe (file)" : "missing"; - })(); + const tokenState = ((): HubDataTokenState => { + const state = readServiceApiTokenState(); + if (state.kind === "unsafe") return "unsafe (file)"; + if (state.kind !== "present") return "missing"; + return tokenCollidesWithAdmin(state.token, env) ? "admin-collision (file)" : "present (file)"; + })(); return { dataOrigin: configuredDataOrigin ?? `http://${host === "127.0.0.1" ? "localhost" : host}:${listen.port}`, @@ -195,6 +218,7 @@ export function collectHubStatus( managementPublicOrigin: config.hub?.managementPublicOrigin ?? null, dataToken: tokenState, dataTokenPath: serviceApiTokenFilePath(), + dataTokenEnvInShell: Boolean(env.OPENCODEX_API_AUTH_TOKEN?.trim()), }; } @@ -208,13 +232,25 @@ export function hubStatusLines(hub: HubStatus): string[] { : hub.loopbackListener.state === "companion" ? `companion on http://127.0.0.1:${hub.loopbackListener.port} — same port as the public listener, no credential needed locally` : `ported on http://127.0.0.1:${hub.loopbackListener.port} — a second port local clients must be pointed at`; + const tokenLines = [` Data token: ${hub.dataToken}${hub.dataToken === "missing" ? "" : ` at ${hub.dataTokenPath}`}`]; + if (hub.dataToken === "admin-collision (file)") { + // Naming the consequence matters more than naming the state: this is what a crash-looping + // hub looks like from `ocx status`, and nothing else in the report says so (#4236). + tokenLines.push( + " that file holds the MANAGEMENT token, so the hub fences its management API closed at boot —", + " delete it and run 'ocx service repair' to generate a data-plane token", + ); + } + if (hub.dataTokenEnvInShell) { + tokenLines.push(" OPENCODEX_API_AUTH_TOKEN is also set in this shell; the installed service reads the file, not this"); + } return [ "Hub:", ` Data origin: ${hub.dataOrigin}${hub.dataOriginConfigured ? " (hub.dataPublicOrigin)" : " (derived from the bind address)"}`, ` Loopback listener: ${listener}`, ` Management ingress: ${hub.managementIngress.enabled ? `http://127.0.0.1:${hub.managementIngress.port}` : "disabled"}`, ` Management origin: ${hub.managementPublicOrigin ?? "unset — remote pairing and the remote dashboard need hub.managementPublicOrigin"}`, - ` Data token: ${hub.dataToken}${hub.dataToken === "present (file)" || hub.dataToken === "unsafe (file)" ? ` at ${hub.dataTokenPath}` : ""}`, + ...tokenLines, " Invite a machine: ocx hub invite", ]; } diff --git a/src/service.ts b/src/service.ts index 7826c291d4..85c5b3abc5 100644 --- a/src/service.ts +++ b/src/service.ts @@ -502,9 +502,31 @@ export function serviceRetryCommand( * installing shell. This function is the chokepoint that should refuse it rather than * writing a file that produces a broken service. Comparison is the same helper doctor * uses: minted `ocx_admin_…` prefix, or byte-equal to configuredAdminToken (env or file). + * + * `source` selects the remedy, not the rule. The token can also arrive from an EXISTING + * `service-api-token` that install/repair reuses, and there `unset` is meaningless advice — + * the fix is to delete the file so a data-plane token is generated. */ -export function assertNotAdminToken(token: string, env: NodeJS.ProcessEnv = process.env): void { +export function assertNotAdminToken( + token: string, + env: NodeJS.ProcessEnv = process.env, + source: "env" | "file" = "env", +): void { if (!tokenCollidesWithAdmin(token, env)) return; + if (source === "file") { + // The file branch of `writeServiceApiTokenFile` used to skip this check entirely, so a + // hand-pasted admin token already on disk (pre-#2696, or the exact #4236 incident) was + // silently reused: `ocx status` said `present (file)` and the hub crash-looped at boot. + // The remedy is NOT `unset` -- there is nothing in the environment to unset. + throw new Error( + `${serviceApiTokenFilePath()} holds a management (admin) token, not a data-plane token. ` + + "The service exports that file as the data-plane secret, which fences the whole management " + + "API closed and makes every ocx management command fail with 503, so the hub crash-loops at " + + `boot. Delete the file (rm ${serviceApiTokenFilePath()}), then rerun \`ocx service repair\` ` + + "(or `ocx service install` when the service is not installed yet): a fresh owner-only " + + "data-plane token is generated and nothing needs to be exported by hand.", + ); + } throw new Error( "OPENCODEX_API_AUTH_TOKEN holds a management (admin) token. The service exports it " + "as the data-plane secret, which fences the whole management API closed and makes " @@ -530,17 +552,25 @@ export function assertNotAdminToken(token: string, env: NodeJS.ProcessEnv = proc */ export function assertServiceAuthEnvironment(): void { const config = loadConfig(); - // Check the collision before the loopback short-circuit: a loopback install writes - // the token file too, so returning early here is what let the broken state through. + // Both collision checks come BEFORE the loopback short-circuit, because the launch wrapper + // exports the token file unconditionally (`buildServiceShellCommand` cats it whenever it + // exists, whatever the hostname): a management token in either source fences the whole + // management plane closed at boot, even on a loopback install that needs no admission + // secret. Returning early is what let that broken state through. const present = process.env.OPENCODEX_API_AUTH_TOKEN?.trim(); if (present) assertNotAdminToken(present); + const state = readServiceApiTokenState(); + // An existing FILE holding the admin token is the incident shape itself, and the first round + // only checked the env var — so install/repair reused it and the hub crash-looped at boot. + // On a machine connected to a hub this same file holds that hub's issued client key, which + // is never a management token, so the check is a no-op there. + if (state.kind === "present") assertNotAdminToken(state.token, process.env, "file"); if (isLoopbackHostname(config.hostname)) return; if (present) return; // Absent is fine — install/repair generates one below. `unsafe` is not: the writer refuses // to replace a path it cannot vouch for, so say so here, where the operator can still act, // instead of failing mid-install. Reached from `service repair` as well as `install`, so // name a command that can actually succeed (see serviceRetryCommand). - const state = readServiceApiTokenState(); if (state.kind !== "unsafe") return; const diag = diagnoseService(); throw new Error( @@ -608,9 +638,21 @@ export function writeServiceApiTokenFile(): ProvisionedServiceApiToken | null { if (isLoopbackHostname(loadConfig().hostname)) return null; const existing = readServiceApiTokenState(); if (existing.kind === "present") { + // The collision check is NOT only for the env branch. A file that already holds the admin + // token -- hand-pasted before #2696, or written by the very incident this unit closes -- + // was silently accepted here, so `ocx status` reported `present (file)` and the hub + // crash-looped at boot with no command pointing at the cause. + const path = serviceApiTokenFilePath(); + assertNotAdminToken(existing.token, process.env, "file"); + // `readServiceApiTokenState` accepts any bounded regular file, so a reused token may well + // be group- or world-readable. Tighten it on the way through rather than claiming + // "owner-only" about a mode nobody checked; best-effort, since a non-owner cannot chmod + // and failing the install over it would be worse than the loose mode. + try { chmodSync(path, 0o600); } catch { /* best-effort */ } + if (process.platform === "win32") hardenSecretPath(path, { required: false }); // No log line: repair/restart hit this on every run and an unconditional notice about a // credential file trains operators to ignore the one that matters. - return { path: serviceApiTokenFilePath(), origin: "file" }; + return { path, origin: "file" }; } if (existing.kind === "unsafe") throw new Error(`${existing.reason}: ${serviceApiTokenFilePath()}`); const path = persistServiceApiToken(randomBytes(32).toString("hex")); From 8142a85d304329e54d9982ef75e11f84f736a1c9 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Fri, 11 Sep 2026 12:11:08 +0900 Subject: [PATCH 11/12] test(cli,service): pin the review-round refusals and the admin-collision state `hub-invite`: every loopback and wildcard spelling of the bind address resolves to `loopback-derived` and is refused with nothing minted (the injected mint records no call), while `--data-url` and `hub.dataPublicOrigin` each unblock it; the surfaced config commands carry `ocx config set hub '{}'` only when `hub` is absent; the corsAllowOrigins suggestion keeps the existing entries and never duplicates; the bound browser origin reaches stderr in both human and --json mode, and a non-default one names the port the client needs. `service`: an admin token in the REUSED token file is refused by the preflight and by the writer, the file is left untouched by the refusal, `unset` is NOT the advice given, and a loopback install is refused too (the wrapper cats the file whatever the hostname). A reused world-readable file comes back at mode 0600 with its bytes unchanged. `cli-status-json`: the token state is about the file and never about the calling shell, the shell's variable is reported separately, and a file holding the admin token reads `admin-collision (file)` with the consequence and the fix in the lines -- reached through the minted prefix and through byte-equality with the configured admin token, the same comparison doctor and the service chokepoint use. Both cases still assert no token value appears in the JSON or the rendered lines. Refs #4236 Co-Authored-By: Claude Fable 5.1 --- tests/cli/cli-status-json.test.ts | 45 ++++++++++-- tests/cli/hub-invite.test.ts | 118 +++++++++++++++++++++++++++++- tests/service/service.test.ts | 64 +++++++++++++++- 3 files changed, 219 insertions(+), 8 deletions(-) diff --git a/tests/cli/cli-status-json.test.ts b/tests/cli/cli-status-json.test.ts index 9c8da57078..2aae43a671 100644 --- a/tests/cli/cli-status-json.test.ts +++ b/tests/cli/cli-status-json.test.ts @@ -687,19 +687,25 @@ describe("status hub block", () => { expect(hubStatusLines(configured!).join("\n")).toContain("hub.dataPublicOrigin"); }); - test("the token source is reported, never the token, and env wins over the file", () => { + test("the token state is about the file the service reads, never about this shell", () => { withHome(home => writeFileSync(join(home, "service-api-token"), `${TOKEN}\n`, "utf8"), () => { const fromFile = collectHubStatus(hub() as Parameters[0], { port: 10100 }, {}); expect(fromFile?.dataToken).toBe("present (file)"); - // Service precedence: the launch wrapper exports the file only when the environment - // has nothing, so an env token is what the running process is actually using. - const fromEnv = collectHubStatus( + expect(fromFile?.dataTokenEnvInShell).toBe(false); + + // `present (env)` used to be reported here whenever the CALLING shell exported the + // variable -- but the launchd plist and the systemd unit overwrite it from the file + // before exec, so the label described the operator's terminal, not the hub. + const withShellVar = collectHubStatus( hub() as Parameters[0], { port: 10100 }, { OPENCODEX_API_AUTH_TOKEN: "from-the-shell" }, ); - expect(fromEnv?.dataToken).toBe("present (env)"); - for (const status of [fromFile!, fromEnv!]) { + expect(withShellVar?.dataToken).toBe("present (file)"); + expect(withShellVar?.dataTokenEnvInShell).toBe(true); + expect(hubStatusLines(withShellVar!).join("\n")).toContain("the installed service reads the file, not this"); + + for (const status of [fromFile!, withShellVar!]) { const rendered = [JSON.stringify(status), ...hubStatusLines(status)].join("\n"); expect(rendered).not.toContain(TOKEN); expect(rendered).not.toContain("from-the-shell"); @@ -707,6 +713,33 @@ describe("status hub block", () => { }); }); + test("a token file holding the ADMIN token is called out, not reported as present", () => { + // The #4236 incident read `present (file)` while the hub crash-looped, because the file + // held the MANAGEMENT token and nothing in the report compared the two. + const admin = `ocx_admin_${"f".repeat(43)}`; + withHome(home => writeFileSync(join(home, "service-api-token"), `${admin}\n`, "utf8"), () => { + const status = collectHubStatus(hub() as Parameters[0], { port: 10100 }, {}); + expect(status?.dataToken).toBe("admin-collision (file)"); + const lines = hubStatusLines(status!).join("\n"); + expect(lines).toContain(status!.dataTokenPath); + expect(lines).toContain("MANAGEMENT token"); + expect(lines).toContain("ocx service repair"); + expect([JSON.stringify(status), lines].join("\n")).not.toContain(admin); + }); + // The same comparison doctor and the service chokepoint use: byte-equal to the configured + // admin token counts too, not only the minted prefix. + withHome(home => { + writeFileSync(join(home, "service-api-token"), "hand-pasted-management-key\n", "utf8"); + }, () => { + const status = collectHubStatus( + hub() as Parameters[0], + { port: 10100 }, + { OPENCODEX_ADMIN_AUTH_TOKEN: "hand-pasted-management-key" }, + ); + expect(status?.dataToken).toBe("admin-collision (file)"); + }); + }); + test("a missing and an unusable token file are distinguished", () => { withHome(() => {}, () => { expect(collectHubStatus(hub() as Parameters[0], { port: 10100 }, {})?.dataToken).toBe("missing"); diff --git a/tests/cli/hub-invite.test.ts b/tests/cli/hub-invite.test.ts index 79c53ad035..d5509459bd 100644 --- a/tests/cli/hub-invite.test.ts +++ b/tests/cli/hub-invite.test.ts @@ -1,10 +1,14 @@ import { describe, expect, spyOn, test } from "bun:test"; import { + appendCorsAllowOriginsCommand, + configSetHubLines, derivedHubDataOrigin, hubInviteCommand, + inviteBoundOriginNotes, pairingOriginUsable, parseHubInviteArgs, parseInviteClients, + resolveHubDataOrigin, runHubCommand, selectInviteBrowserOrigin, } from "../../src/cli/hub"; @@ -120,6 +124,59 @@ describe("hub invite argument and origin helpers", () => { .toBeNull(); }); + test("the data origin refuses a loopback bind and a wildcard bind, and takes an explicit one", () => { + // `probeHostname` spells 0.0.0.0, ::, and 127.0.0.1 all as loopback, so the derived origin + // would tell the other machine to dial ITSELF -- and the code is single-use. + for (const bind of ["127.0.0.1", "localhost", "0.0.0.0", "::", undefined] as const) { + expect(resolveHubDataOrigin(null, undefined, bind, 10100)).toEqual({ + kind: "loopback-derived", + dataUrl: "http://localhost:10100", + bindHostname: bind ?? "127.0.0.1", + }); + } + expect(resolveHubDataOrigin(null, undefined, "100.64.0.10", 10100)) + .toEqual({ kind: "usable", dataUrl: "http://100.64.0.10:10100", source: "derived" }); + // An explicit origin is never second-guessed: loopback is legitimate over an SSH tunnel. + expect(resolveHubDataOrigin("http://localhost:9000", undefined, "127.0.0.1", 10100)) + .toEqual({ kind: "usable", dataUrl: "http://localhost:9000", source: "flag" }); + expect(resolveHubDataOrigin(null, "https://hub.tailnet.ts.net:8443", "127.0.0.1", 10100)) + .toEqual({ kind: "usable", dataUrl: "https://hub.tailnet.ts.net:8443", source: "config" }); + // A malformed persisted value must not silently become the loopback fallback. + expect(resolveHubDataOrigin(null, "not-an-origin", "100.64.0.10", 10100)) + .toEqual({ kind: "usable", dataUrl: "http://100.64.0.10:10100", source: "derived" }); + }); + + test("the surfaced config commands work on a config that has no hub object yet", () => { + // `ocx config set hub.x` exits with `config parent path not found: hub` when `hub` is + // absent (setPath in config-command.ts walks existing parents only). + expect(configSetHubLines({ hub: undefined } as Partial, "dataPublicOrigin", "https://d.test")) + .toEqual(["ocx config set hub '{}'", `ocx config set hub.dataPublicOrigin '"https://d.test"'`]); + expect(configSetHubLines({ hub: {} } as Partial, "managementPublicOrigin", "https://m.test")) + .toEqual([`ocx config set hub.managementPublicOrigin '"https://m.test"'`]); + }); + + test("the corsAllowOrigins command adds to the list instead of replacing it", () => { + // `ocx config set corsAllowOrigins '[...]'` overwrites, so a one-element literal would + // tell an operator with an existing allow-list to delete it. + expect(appendCorsAllowOriginsCommand({ corsAllowOrigins: ["https://a.test"] }, "http://localhost:10100")) + .toBe(`ocx config set corsAllowOrigins '["https://a.test","http://localhost:10100"]'`); + expect(appendCorsAllowOriginsCommand({}, "http://localhost:10100")) + .toBe(`ocx config set corsAllowOrigins '["http://localhost:10100"]'`); + // Idempotent: never suggest a duplicate entry. + expect(appendCorsAllowOriginsCommand({ corsAllowOrigins: ["http://localhost:10100"] }, "http://localhost:10100")) + .toBe(`ocx config set corsAllowOrigins '["http://localhost:10100"]'`); + }); + + test("the bound browser origin is always stated, and a non-default one names the client's port", () => { + expect(inviteBoundOriginNotes("http://localhost:10100", {})).toEqual([ + "Bound browser origin: http://localhost:10100 — the connecting machine must present exactly this.", + ]); + const notes = inviteBoundOriginNotes("http://localhost:9999", { corsAllowOrigins: ["http://localhost:9999"] }); + expect(notes[0]).toContain("http://localhost:9999"); + expect(notes.join(" ")).toContain("ocx config set port 9999"); + expect(notes.join(" ")).toContain(`'["http://localhost:9999","http://localhost:10100"]'`); + }); + test("the printed command carries --clients only when the operator asked for it", () => { expect(hubInviteCommand(GRANT, "https://d.test:8443", "https://m.test", [])) .toBe(`echo '${GRANT}' | ocx connect https://d.test:8443 --management-url https://m.test --pairing-code-stdin`); @@ -142,6 +199,24 @@ describe("hub invite output", () => { expect(err.join("\n")).not.toContain(GRANT); }); + test("the bound browser origin reaches stderr in both modes, with a warning when it differs", async () => { + const plain = await invite(["invite"], hubConfig()); + expect(plain.err.join("\n")).toContain("Bound browser origin: http://localhost:10100"); + expect(plain.err.join("\n")).not.toContain("ocx config set port"); + + const asJson = await invite(["invite", "--json"], hubConfig()); + expect(asJson.err.join("\n")).toContain("Bound browser origin: http://localhost:10100"); + + // `selectInviteBrowserOrigin` silently fell back to the first admitted loopback origin, and + // a remote `ocx connect` only sends http://localhost: -- so without this the + // single-use code was spent with nothing saying why. + const other = await invite(["invite"], hubConfig({ corsAllowOrigins: ["http://localhost:9999"] })); + expect(other.code).toBe(0); + expect(other.boundOrigin).toBe("http://localhost:9999"); + expect(other.err.join("\n")).toContain("Bound browser origin: http://localhost:9999"); + expect(other.err.join("\n")).toContain("ocx config set port 9999"); + }); + test("hub.dataPublicOrigin replaces the derived origin, and --data-url replaces both", async () => { const configured = hubConfig({ hub: { managementPublicOrigin: "https://hub.tailnet.ts.net", dataPublicOrigin: "https://hub.tailnet.ts.net:8443" }, @@ -176,10 +251,18 @@ describe("hub invite refuses before burning a code", () => { } }); - test("a hub with no management origin is told which field to set", async () => { + test("a hub with no management origin is told which field to set, in a runnable form", async () => { const { code, err } = await invite(["invite"], hubConfig({ hub: {} })); expect(code).toBe(1); expect(err.join(" ")).toContain("hub.managementPublicOrigin"); + expect(err.join(" ")).not.toContain("ocx config set hub '{}'"); + + // With no `hub` object at all the dotted form exits `config parent path not found: hub`, + // so the parent-creating line has to come with it. + const absent = await invite(["invite"], hubConfig({ hub: undefined })); + expect(absent.code).toBe(1); + expect(absent.err.join("\n")).toContain("ocx config set hub '{}'"); + expect(absent.err.join("\n")).toContain("ocx config set hub.managementPublicOrigin"); }); test("a --management-url that differs from the configured origin is refused, not printed", async () => { @@ -208,6 +291,39 @@ describe("hub invite refuses before burning a code", () => { expect(err.join(" ")).toContain("http://localhost:10100"); }); + test("a loopback-derived data origin is refused before a code is minted", async () => { + // The incident shape: a hub bound to loopback (or a wildcard) with no hub.dataPublicOrigin + // printed `ocx connect http://localhost:10100`, which on the other machine means "dial + // yourself" -- and the code was gone. + for (const bind of ["127.0.0.1", "0.0.0.0"] as const) { + const { code, err, boundOrigin } = await invite( + ["invite"], + hubConfig({ hostname: bind }), + { live: { pid: 4242, port: 10100, hostname: bind, source: "runtime" } }, + ); + expect(code).toBe(1); + expect(boundOrigin).toBeNull(); // nothing was minted + expect(err.join(" ")).toContain("http://localhost:10100"); + expect(err.join(" ")).toContain("hub.dataPublicOrigin"); + expect(err.join(" ")).toContain("--data-url"); + } + expect((await invite(["invite"], hubConfig({ hostname: "0.0.0.0" }), { + live: { pid: 4242, port: 10100, hostname: "0.0.0.0", source: "runtime" }, + })).err.join(" ")).toContain("wildcard"); + + // Either explicit origin unblocks it. + const viaFlag = await invite(["invite", "--data-url", "https://hub.tailnet.ts.net:8443"], hubConfig({ hostname: "127.0.0.1" }), { + live: { pid: 4242, port: 10100, hostname: "127.0.0.1", source: "runtime" }, + }); + expect(viaFlag.code).toBe(0); + const viaConfig = await invite(["invite"], hubConfig({ + hostname: "127.0.0.1", + hub: { managementPublicOrigin: "https://hub.tailnet.ts.net", dataPublicOrigin: "https://hub.tailnet.ts.net:8443" }, + }), { live: { pid: 4242, port: 10100, hostname: "127.0.0.1", source: "runtime" } }); + expect(viaConfig.code).toBe(0); + expect(viaConfig.out.join("\n")).toContain("ocx connect https://hub.tailnet.ts.net:8443 "); + }); + test("no running hub, a malformed origin, and a refused mint each exit 1 with a reason", async () => { const down = await invite(["invite"], hubConfig(), { live: null }); expect(down.code).toBe(1); diff --git a/tests/service/service.test.ts b/tests/service/service.test.ts index e3e7ffee20..677866fa49 100644 --- a/tests/service/service.test.ts +++ b/tests/service/service.test.ts @@ -1,5 +1,5 @@ import { afterAll, afterEach, describe, expect, spyOn, test } from "bun:test"; -import { existsSync, mkdirSync, mkdtempSync, readFileSync, statSync, writeFileSync } from "node:fs"; +import { chmodSync, existsSync, mkdirSync, mkdtempSync, readFileSync, statSync, writeFileSync } from "node:fs"; import { execFileSync } from "node:child_process"; import { tmpdir } from "node:os"; import { delimiter, isAbsolute, join, posix, win32 } from "node:path"; @@ -604,6 +604,68 @@ describe("service install auth preflight", () => { expect(existsSync(serviceApiTokenFilePath())).toBe(false); }); + test("a reused token file that holds the ADMIN token is refused, not silently accepted", () => { + // The incident shape, and the gap the first round left: the `origin: "file"` branch never + // re-checked the collision, so a hand-pasted admin token on disk was reused, `ocx status` + // said `present (file)`, and the hub crash-looped at boot with nothing naming the cause. + if (existsSync(TEST_DIR)) removeTreeWithRetry(TEST_DIR); + mkdirSync(TEST_DIR, { recursive: true }); + process.env.OPENCODEX_HOME = TEST_DIR; + delete process.env.OPENCODEX_API_AUTH_TOKEN; + saveConfig({ + port: 10100, + hostname: "0.0.0.0", + providers: { openai: { adapter: "openai-chat", baseUrl: "https://api.example.test/v1" } }, + defaultProvider: "openai", + } as OcxConfig); + const admin = `ocx_admin_${"f".repeat(43)}`; + writeFileSync(serviceApiTokenFilePath(), `${admin}\n`, "utf8"); + + // Install/repair stops at the preflight, where the operator can still act. + expect(() => assertServiceAuthEnvironment()).toThrow(/management \(admin\) token/); + expect(() => assertServiceAuthEnvironment()).toThrow(/ocx service repair/); + // `unset` is the WRONG remedy here: nothing is exported. Deleting the file is. + expect(() => assertServiceAuthEnvironment()).not.toThrow(/unset OPENCODEX_API_AUTH_TOKEN/); + // And the writer is still the last line of defence, whichever caller got there. + expect(() => writeServiceApiTokenFile()).toThrow(/not a data-plane token/); + // Refusing must not mutate the file; the operator deletes it deliberately. + expect(readFileSync(serviceApiTokenFilePath(), "utf8").trim()).toBe(admin); + + // And a LOOPBACK install is refused too: `buildServiceShellCommand` cats the file into + // OPENCODEX_API_AUTH_TOKEN whenever it exists, whatever the hostname, so the management + // plane is fenced closed at boot there as well. + saveConfig({ + port: 10100, + hostname: "127.0.0.1", + providers: { openai: { adapter: "openai-chat", baseUrl: "https://api.example.test/v1" } }, + defaultProvider: "openai", + } as OcxConfig); + expect(() => assertServiceAuthEnvironment()).toThrow(/management \(admin\) token/); + }); + + test("reusing an existing token file makes 'owner-only' true rather than assumed", () => { + if (existsSync(TEST_DIR)) removeTreeWithRetry(TEST_DIR); + mkdirSync(TEST_DIR, { recursive: true }); + process.env.OPENCODEX_HOME = TEST_DIR; + delete process.env.OPENCODEX_API_AUTH_TOKEN; + saveConfig({ + port: 10100, + hostname: "0.0.0.0", + providers: { openai: { adapter: "openai-chat", baseUrl: "https://api.example.test/v1" } }, + defaultProvider: "openai", + } as OcxConfig); + // `readServiceApiTokenState` accepts any bounded regular file, so a reused token can be + // world-readable -- and `ocx status` calls that path "owner-only". + writeFileSync(serviceApiTokenFilePath(), `${"c".repeat(64)}\n`, { encoding: "utf8", mode: 0o644 }); + if (process.platform !== "win32") chmodSync(serviceApiTokenFilePath(), 0o644); + + expect(writeServiceApiTokenFile()).toEqual({ path: serviceApiTokenFilePath(), origin: "file" }); + expect(readFileSync(serviceApiTokenFilePath(), "utf8").trim()).toBe("c".repeat(64)); + if (process.platform !== "win32") { + expect(statSync(serviceApiTokenFilePath()).mode & 0o777).toBe(0o600); + } + }); + test("allows non-loopback service install when the API token is in the service environment", () => { if (existsSync(TEST_DIR)) removeTreeWithRetry(TEST_DIR); mkdirSync(TEST_DIR, { recursive: true }); From 902fc02336a6c150ef5454f84041630fad382f0a Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Fri, 11 Sep 2026 12:11:09 +0900 Subject: [PATCH 12/12] docs(devlog): record the PR4 review round Six findings, all accepted, each with the reasoning that made it a fix rather than a preference: why a wildcard bind is refused instead of resolved, why the admin-token remedy differs by source, why both collision checks had to move ahead of the loopback short-circuit, why `present (env)` was backwards, why the `--json` envelope did not grow, and why two of the four `canonicalHttpOrigin` copies stay. The superseded paragraph in the first round's section 5 is marked rather than rewritten. Also records that `tests/service/service.test.ts` is 209 on this machine with the working tree reverted, not the 212 the first round reported, and that no live-hub command was run this round -- `ocx status` reaches `launchctl` through `diagnoseService`. Refs #4236 Co-Authored-By: Claude Fable 5.1 --- .../040_hub_token_ux.md | 148 ++++++++++++++++++ 1 file changed, 148 insertions(+) diff --git a/devlog/_plan/260911_hub_single_port/040_hub_token_ux.md b/devlog/_plan/260911_hub_single_port/040_hub_token_ux.md index 90f2238b18..59d04ab9fa 100644 --- a/devlog/_plan/260911_hub_single_port/040_hub_token_ux.md +++ b/devlog/_plan/260911_hub_single_port/040_hub_token_ux.md @@ -158,6 +158,11 @@ that is the service's own precedence: the wrapper exports the file only when the nothing, so naming the file first would name a source the running process is not using. A test asserts no token value appears in either the JSON or the lines. +> **Superseded by the review round below (§2 and §5).** The reasoning in that last sentence is +> backwards: the wrapper *overwrites* the environment from the file, so `present (env)` described +> the operator's own shell. The states are now `present (file)` / `unsafe (file)` / +> `admin-collision (file)` / `missing`, with the shell's variable as its own field. + ### 6. Help, registry, capabilities, skill surface `ocx hub` is a registry entry with a runner in `DISPATCH_COMMANDS`, two banner lines in @@ -259,3 +264,146 @@ not modified. Two read-only commands were: should be admitted implicitly is a pairing-policy question for its own change. - `ocx hub` has exactly one subcommand. Further hub-side verbs (listing or revoking issued client keys from the CLI rather than the dashboard's **Integrations → API Keys**) are not in this PR. + +## Review round (PR #4252) + +Six findings, all accepted. Two were the same mistake in two places: the code trusted a value it +had already learned not to trust (the reused token file) and printed a value it had not checked +(the loopback data origin). The other four were honesty defects in text the operator is supposed +to paste or believe. + +### 1. `ocx hub invite` printed a loopback data origin (should-fix) + +`derivedHubDataOrigin` sends `0.0.0.0`, `::` and `127.0.0.1` all through `probeHostname`, which +spells every one of them as loopback — so on a hub bound to loopback or to a wildcard with no +`hub.dataPublicOrigin`, the "run on the other machine" line said `ocx connect +http://localhost:10100`. That tells the other machine to dial **itself**, and because the pairing +code is single-use it is spent on a connect that cannot succeed. The live hub on the maintainer's +Mac is exactly this shape (`hostname: 127.0.0.1`). + +New `resolveHubDataOrigin(override, configured, bindHostname, port)` returns either a usable +origin (tagged `flag` / `config` / `derived`) or `loopback-derived`, and `runInvite` refuses the +latter **before** the mint. An explicit `--data-url` or `hub.dataPublicOrigin` is never +second-guessed: a loopback data origin is legitimate over an SSH tunnel. + +A wildcard bind is refused the same way rather than resolved. There is no existing helper that +derives a tailnet or LAN address — `grep networkInterfaces src` has no hits, and `probeHostname` +deliberately collapses wildcards to loopback — so deriving one here would mean picking an +interface the operator never chose and advertising it in a credential exchange. The message names +which shape this hub has (`bindAddressPhrase`: wildcard vs loopback-only) and offers both the +persistent fix and the per-invite one. + +### 2. The reused `service-api-token` was never re-checked for the admin token (should-fix) + +This is the incident shape, still reachable on the first round's code: the `origin: "file"` branch +of `writeServiceApiTokenFile` returned early without calling `assertNotAdminToken`, so a file +holding the **management** token — hand-pasted pre-#2696, or written by the very incident this +unit closes — was silently accepted. `ocx status` said `present (file)` and the hub crash-looped +at boot, with nothing in any output naming the cause. + +- `assertNotAdminToken` gained a third argument, `source: "env" | "file"`. It selects the + **remedy**, not the rule: `unset OPENCODEX_API_AUTH_TOKEN` is meaningless advice about a file, + so the file message says to delete the file and rerun `ocx service repair` (or `install`). The + message deliberately does not call `serviceRetryCommand()`, which would pull `diagnoseService()` + — and `launchctl` — into an error path that runs at install time. +- The writer's file branch now calls it, and `assertServiceAuthEnvironment` checks the file too. +- **Both collision checks moved ahead of the loopback short-circuit.** `buildServiceShellCommand` + cats the token file into `OPENCODEX_API_AUTH_TOKEN` whenever the file exists, *whatever the + hostname*, so a loopback install with an admin-token file fences its management plane closed at + boot just the same. On a connected client the file holds the hub's issued client key, which is + never a management token, so the check is a no-op there. +- `ocx status` gained the state. `HubDataTokenState` now has `admin-collision (file)`, and the + block adds two lines naming the consequence and the fix, because "this is what a crash-looping + hub looks like in status output" is the sentence that was missing. +- The reused file is `chmod 0600`'d best-effort on the way through. + `readServiceApiTokenState` accepts any bounded regular file, so a reused token could be + group- or world-readable while install printed "owner-only". Best-effort because a non-owner + cannot chmod and failing the install over a loose mode would be worse than the loose mode. + +### 3. The bound browser origin was invisible (should-fix) + +`selectInviteBrowserOrigin` falls back to the first admitted loopback origin when +`http://localhost:10100` is not admitted, and the printed command carries no trace of it — but a +remote `ocx connect` sends `Origin: http://localhost:`, so a grant bound +to anything else is refused at the exchange and the code is spent with no hint. `ocx status` +cannot show it either; it is a property of the grant, not of the config. + +`inviteBoundOriginNotes` now prints the bound origin on **every** successful invite, in both +human and `--json` mode (stderr, where the single-use warning already lives, so the `--json` +envelope on stdout is unchanged). When it is not the default it also names the port the other +machine must be configured with, plus the alternative of admitting the default origin instead. + +### 4. Surfaced fix commands that do not run (nit) + +- `ocx config set hub.managementPublicOrigin …` exits `config parent path not found: hub` when + the `hub` object is absent (`setPath` in `src/cli/config-command.ts:60` walks only existing + parents) — and absent is precisely the config being advised. `configSetHubLines` prefixes + `ocx config set hub '{}'`, the way `guides/remote-hub.md` does, and **only** when `hub` is + actually missing, so the lines paste verbatim either way. +- `ocx config set corsAllowOrigins '[…]'` replaces the array, so the old one-element literal told + an operator with an existing allow-list to delete it. `appendCorsAllowOriginsCommand` emits the + current entries plus the new one (idempotent if already present), and the text points at + `ocx config get corsAllowOrigins`. + +### 5. `present (env)` described the operator's terminal, not the hub (nit) + +The status line read the CLI's own shell, but the launchd plist and the systemd unit overwrite +`OPENCODEX_API_AUTH_TOKEN` from the token file before exec — so on a service-run hub the label +named a source the running process was not using, which is the opposite of the first round's +stated reason for preferring `env`. The state is now always about the file, and the shell's +variable is reported as its own field, `dataTokenEnvInShell`, rendered as a sub-line ("the +installed service reads the file, not this"). It is kept rather than dropped because it does +decide what a foreground `ocx start` **in that same shell** would admit. + +### 6. A fourth `canonicalHttpOrigin` (nit) + +None of the three existing copies was exported. The new one is +`canonicalHttpOrigin` in `src/lib/gui-pair-capability.ts`, beside `canonicalGuiBrowserOrigin` +(origin canonicalisation is already that module's job, and it is the only module the CLI pairing +path and the hub command already share). `src/cli/hub.ts` and `src/cli/gui-pair-client.ts` now +import it; their local copies are gone. + +`src/config.ts` and `src/server/gui-session.ts` keep theirs **deliberately**: `gui-session.ts` is +a server security-boundary file and folding it in would also mean making it import nothing new, +which is true here — but this PR's security note rests on `src/server/*` having no diff at all, +and the config copy is reached from the zod schema path. Four copies became two plus one shared +export; collapsing the last two is its own change. + +### Not changed, and why + +- **No tailnet/LAN address derivation.** See finding 1: refusing is the honest answer until + something in the repo owns that discovery. +- **The `--json` envelope is unchanged** (`{ code, expiresAt, dataUrl, managementUrl, command }`). + The bound origin is operator advice, so it goes to stderr with the single-use warning rather + than growing the documented contract. +- **`ocx status` still does not show which origin a grant was bound to.** It cannot: grants are + not persisted anywhere status reads. + +### Verification (review round, this machine) + +``` +bun run typecheck # clean +bun run privacy:scan # Privacy scan passed +bun run skill:surface # regenerated (2 added capability detail lines) +bun test tests/cli/hub-invite.test.ts # 22 pass (was 16) +bun test tests/service/service.test.ts # 211 pass (was 209 here; +2 new) +bun test tests/service/service-secrets.test.ts # 10 pass +bun test tests/service/winsw.test.ts # 25 pass +bun test tests/cli/cli-status-json.test.ts # 54 pass (was 53; one test split into two) +bun test tests/gui/gui-pair-client.test.ts # 4 pass +bun test tests/gui/gui-pair-capability.test.ts # 2 pass +bun test tests/cli/cli-registry.test.ts tests/cli/cli-capabilities.test.ts \ + tests/cli/cli-help.test.ts tests/ci-workflows/skill-ocx.test.ts # 62 pass +bun test tests/cli/cli-transport-honesty.test.ts # 22 pass +bun test tests/cli/cli-dispatch.test.ts # 39 pass +bun test tests/cli/cli-json-contract.test.ts # 8 pass +``` + +`tests/service/service.test.ts` is **209** on this branch with the working tree reverted, not the +212 recorded in the first round's table; the earlier figure does not reproduce here. The new +count is 211 = 209 + 2. + +No live-hub command was run this round, not even a read-only one: `ocx status` reaches +`diagnoseService()` → `probeLaunchdLoadState()` → `launchctl`, and the operator instruction for +this machine is to run no `launchctl` at all. Every sentence the status block can print is pinned +by `tests/cli/cli-status-json.test.ts` instead. No repository-wide suite, by the same instruction.