From 2ee2eacb4b3d19b1aad3450b3ab3fa0cf47e1a0d Mon Sep 17 00:00:00 2001 From: timkjr Date: Fri, 28 Aug 2026 13:45:12 -0500 Subject: [PATCH] fix(omp): clamp OMP_AUTH_BROKER_URL/TOKEN, correct the env-allowlist docs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The docs claimed omp "has no documented vendor-key namespace of its own" and "the multi-user clamp has nothing to gate" for omp — both false. Per omp's own docs/environment-variables.md, it reads ~40 provider keys from env (pi's known 34-key problem in the same shape), and its own knobs are mostly PI_* (already globally allowlisted): PI_CONFIG_DIR, PI_CODING_AGENT_DIR, PI_CODING_AGENT_SESSION_DIR, PI_SUBPROCESS_CMD, PI_SHELL_PREFIX. The first three also move the ~/.omp tree omp-session-resolver.ts/omp-transcript.ts hardcode, silently degrading pinning/history — a known gap shared with pi, documented but not fixed here. The OMP_* prefix this PR adds brings in OMP_AUTH_BROKER_URL/ OMP_AUTH_BROKER_TOKEN, where omp resolves credentials from — the same shape DEEPSEEK_BASE_URL is already dropped for in clampEnvOverridesForOwner(). Add both to OWNER_CLAMPED_ENV_KEYS so a non-granted owner in multi-user mode can't redirect them, and correct the false claims in CLAUDE.md, docs/omp-integration.md, and the stale resolveOmpHome() comment. Also documents omp's default tools.approvalMode: yolo, which was previously unstated. --- CLAUDE.md | 2 +- docs/omp-integration.md | 28 +++++++++++++++++++++----- src/utils/omp-session-resolver.ts | 8 +++++++- src/web/routes/session-routes.ts | 24 ++++++++++++++++++---- test/omp-mode.test.ts | 33 ++++++++++++++++++++++++++++++- 5 files changed, 83 insertions(+), 12 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 62ae926e..802ed5d5 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -205,7 +205,7 @@ Codeman is a Claude Code session manager with web interface and autonomous Ralph **Docker cases**: a case can point at a **container**, with any of the CLI run modes running inside it. Like remote-SSH this is a **LOCATION OVERLAY on cases, never a `SessionMode` of its own**. Exactly one long-lived container **per case**, shared by all its sessions, so killing a session kills only that session's in-container tmux and **never** `docker stop` while siblings remain. The workspace is a real host dir bind-mounted at the **same absolute path**, which is what keeps file-routes/watchers on real host bytes and makes the in-container transcript projHash match the host. Credentials are **seeded** (RO mount, copied into the container once) rather than shared RW, so in-container CLIs never write refreshed tokens back to the host, and bind mounts are excluded from `docker commit` so exports stay secret-free. **NEVER a create-time `-e` for secrets, NEVER `--privileged`, NEVER the docker socket.** Config drift is detected via a label hash and a drifted launch is REFUSED rather than silently launched with stale config. ⚠️ On the loopback-only prod bind a container cannot reach 127.0.0.1, so in-container hooks need `CODEMAN_DOCKER_BRIDGE_HOOKS=1`; otherwise idle detection falls back to output-based. → [architecture-invariants#docker-cases](docs/architecture-invariants.md#docker-cases), `docs/docker-cases.md` (user guide), `docs/docker-cases-plan.md` (design) -**External CLI modes (OpenCode, Codex, Gemini, Antigravity, Pi, Grok, DeepSeek, OMP)**: `isExternalCliMode()` in `session.ts` gates Claude-specific behavior off (Ralph tracker, BashToolParser, token/CLI-info parsing, ❯-prompt readiness); these CLIs render their own TUIs, so readiness is output stabilization instead. All eight **require tmux with no direct PTY fallback**, because secrets are injected via socket-scoped `tmux setenv` and never on the spawn command line. ⚠️ `run*()` in `session-ui.js` MUST unwrap the `{success,data}` envelope; reading the raw shape silently breaks the run. ⚠️ **Codex sessions use PREDICTIVE WRITE-THROUGH echo, never the buffer overlay** (`_localEchoPolicy` in `_updateLocalEchoState`, terminal-ui.js): codex's composer reacts per keystroke ("/" pops a live-filtering picker, arrows edit server-side state, the composer grows as it wraps), so buffer-until-Enter starved it into issues #218/#219/#220/#222 and stays disabled (`_localEchoEnabled` remains false for codex). Instead, `PredictiveEchoAddon` (separate `vendor/xterm-predictive-echo.js` bundle) paints each keystroke at the predicted cell while the wire path stays BYTE-IDENTICAL: the onData hook (`_predictHookOnData`) is a plain statement with no `return`, so control always falls through into the untouched send path — pinned by vm and E2E byte-identity tests. Predictions reconcile against the parsed buffer and only while the cursor sits on the measured composer row (`isCodexComposerRow`, `/^› /`). Codex also **drops keystrokes that share a PTY read with a bracketed paste**, so flushed text and the paste sequence must go out as separate delayed writes (mirroring the Enter branch's delayed `\r`). Tests: `test/local-echo-codex-gating.test.ts`, `test/codex-predictive-echo.test.ts` (E2E vs real codex), `packages/xterm-zerolag-input/test/codex-replay.test.ts`. ⚠️ **Pi is the opposite kind of CLI and needs the opposite instincts**: it has NO permission prompts and no sandbox, so there is no bypass flag to send and Codeman must not invent one; its privileged knob is the tri-state `approveProjectTrust` (`--approve`/`--no-approve`), which makes pi EXECUTE repo-local `.pi/extensions` TypeScript, so the multi-user clamp puts pi in the **materialize** branch (an absent config still yields `--no-approve` for a non-granted owner) and `--api-key` is never wired. Pi stays OUT of `isAltScreenStripMode()` (main-screen TUI, and its 0.84.0 fullscreen mode is runtime-switchable via `/settings`, where the alt screen is load-bearing), and lands on the `'buffer'` echo policy via the `_updateLocalEchoState` fallthrough. Pi's own tests: `test/pi-mode.test.ts`, `test/routes/external-cli-bypass-clamp.test.ts`; user guide `docs/pi-integration.md`. ⚠️ **Grok is codex-shaped on permissions but opencode-shaped on rendering**: its bypass switch is `alwaysApprove` (`--always-approve`, grok's `bypassPermissions` mode — the Run button sends it `true` like antigravity's, and the clamp's only-if-sent branch strips it for non-granted owners), while its fullscreen alt-screen TUI keeps it OUT of `isAltScreenStripMode()`; the resolver version-probes `grok --version` like pi's (npm squatters exist for the name — `GET /api/grok/status` surfaces path + version), and grok lands on the `'buffer'` echo policy via the fallthrough (UNMEASURED against a live authenticated session; if its composer turns out per-keystroke-reactive like codex, flip it to the `'off'` branch). Grok's own tests: `test/grok-mode.test.ts`, `test/grok-cli-resolver.test.ts`; user guide `docs/grok-integration.md`. ⚠️ **DeepSeek breaks three of this family's assumptions, so do not pattern-match it onto its siblings.** (1) The agent is a **PROFILE, not the binary**: `dsh` is a launcher over `$DSH_HOME/profiles/` and DeepSeek ships only `web`/`headless`/`base`, so the terminal front door is ALWAYS third-party and "installed" ≠ "runnable" — the Run button gates on `isDeepSeekRunnable()` (binary AND a pane-capable profile) while `isDeepSeekAvailable()` gates the "add a profile" affordance; a `web`/`headless` profile is refused at spawn because it cannot drive a pane. (2) The permission switch is the **`DSH_PERMISSION_MODE` env export, not a flag** (`read-only`/`workspace-write`/`danger-full-access`) — the harness has none, and this is the one legitimate exception to the effort-style env-var ban because it is read with `??` as a boot-time default, so it stays soft; absent = `workspace-write`, which asks, hence the only-if-sent clamp branch, clamping to `workspace-write` (never `read-only`, which would break the workspace). ⚠️ **That clamp needs a second half no other CLI needs**, because the switch is an env var and `DSH_*` is an allowlisted `envOverrides` prefix: `applyEnvOverrides()` runs AFTER `_configureDeepSeek()` in tmux-manager, so a non-granted owner sending `DSH_PERMISSION_MODE` on the SAME request would land last and hand back exactly the privilege the config clamp removed. `clampEnvOverridesForOwner()` (session-routes.ts) DROPS `DSH_PERMISSION_MODE`, `DSH_HOME` and `DEEPSEEK_BASE_URL` for a non-granted owner (the last because `_configureDeepSeek()` forwards the SERVER's own `DEEPSEEK_API_KEY` into the pane, so a redirected base URL would send it to a foreign host) (dropping falls through to what `_configureDeepSeek()` exports, which is the clamped value); `DSH_HOME` is there because it points the launcher at a profile tree whose plugin code runs at BOOT, before any approval row applies. Every OTHER CLI's bypass is a command-line flag reachable only through its config, which is why the config clamp alone is the whole gate for them. (3) It is the **only non-claude mode that passes `hooksAvailableForMode()`**, and for it alone that predicate is a per-SESSION question rather than a per-mode one (`deepSeekConfig.statusReporting: false` disarms the bridge, so every call site passes `sessionHookOptions(session)`; answering from the mode there re-creates the infinite-wait-dressed-as-a-timeout the guard exists to prevent). It passes because the terminal front door reports idle/working/blocked to a supervisor over a generic env-gated contract and `deepseek-status-shim.ts` makes Codeman that supervisor — real `stop`/`blocked` signals, real Approvals Inbox items, plus the `agent_working` event that clears an alert answered in the terminal. ⚠️ The resolver needs the strictest identity probe of the family (`dsh --help` must say `DeepSeek Harness`) because Debian ships an unrelated `dsh` (dancer's shell) that would pass a version probe. Model is NOT a session field (it is a profile composition entry). ⚠️ `hooksAvailableForMode()` is about hook SIGNALS and is not a stand-in for "is this a claude session": Read My Mind and intent capture read Claude's own transcript and compare `mode === 'claude'` directly, because when `deepseek` earned a yes the shared predicate silently widened both to a mode with no transcript to read (pinned by a static check in `test/deepseek-mode.test.ts`). ⚠️ **It is also the only external CLI whose answers are READ FROM DISK rather than scraped off the pane**: `deepseek-transcript.ts` reads `$DSH_HOME/sessions///session.jsonl.zstd` and backs the `last-response` route for dsh, because the pane segmenter served dsh-TUI's ASCII-art SPLASH as the worker's answer (measured), which anything polling for a first answer reads as an answer. Three traps live in that file: dsh appends **one zstd FRAME per write** and Node's `zlib` zstd decoder stops at the first (a real 56-line transcript decoded as 1 line, so the module walks frame headers itself; a Node older than 22.15 has no zstd and falls back to the pane); every turn also records a **plugin-sourced `user/message`** (the runtime-context snapshot) that must not render as the user's words; and a failed `turn/end` is surfaced as `Turn error: …` rather than as an empty string that reads as "still thinking". ⚠️ Session→transcript pairing is by the header's own `cwd` plus a ±60 s boot window, never by reproducing dsh's directory mangling (which has already changed form once) — and NEVER by newest-mtime alone, which handed a fresh worker its predecessor's answer in the same case dir. DeepSeek's own tests: `test/deepseek-mode.test.ts`, `test/deepseek-cli-resolver.test.ts`, `test/deepseek-transcript.test.ts`; user guide `docs/deepseek-integration.md`. OMP (`omp`) is architecturally the simplest of the family: it needs no bypass flag (the CLI's own `~/.omp` config governs trust/model routing), so `buildOmpCommand()` only ever passes `--model`/`--resume`, and the multi-user clamp has nothing to gate. → [architecture-invariants#external-cli-modes-opencode-codex-gemini-antigravity-pi-grok-deepseek-omp](docs/architecture-invariants.md#external-cli-modes-opencode-codex-gemini-antigravity-pi-grok-deepseek-omp) +**External CLI modes (OpenCode, Codex, Gemini, Antigravity, Pi, Grok, DeepSeek, OMP)**: `isExternalCliMode()` in `session.ts` gates Claude-specific behavior off (Ralph tracker, BashToolParser, token/CLI-info parsing, ❯-prompt readiness); these CLIs render their own TUIs, so readiness is output stabilization instead. All eight **require tmux with no direct PTY fallback**, because secrets are injected via socket-scoped `tmux setenv` and never on the spawn command line. ⚠️ `run*()` in `session-ui.js` MUST unwrap the `{success,data}` envelope; reading the raw shape silently breaks the run. ⚠️ **Codex sessions use PREDICTIVE WRITE-THROUGH echo, never the buffer overlay** (`_localEchoPolicy` in `_updateLocalEchoState`, terminal-ui.js): codex's composer reacts per keystroke ("/" pops a live-filtering picker, arrows edit server-side state, the composer grows as it wraps), so buffer-until-Enter starved it into issues #218/#219/#220/#222 and stays disabled (`_localEchoEnabled` remains false for codex). Instead, `PredictiveEchoAddon` (separate `vendor/xterm-predictive-echo.js` bundle) paints each keystroke at the predicted cell while the wire path stays BYTE-IDENTICAL: the onData hook (`_predictHookOnData`) is a plain statement with no `return`, so control always falls through into the untouched send path — pinned by vm and E2E byte-identity tests. Predictions reconcile against the parsed buffer and only while the cursor sits on the measured composer row (`isCodexComposerRow`, `/^› /`). Codex also **drops keystrokes that share a PTY read with a bracketed paste**, so flushed text and the paste sequence must go out as separate delayed writes (mirroring the Enter branch's delayed `\r`). Tests: `test/local-echo-codex-gating.test.ts`, `test/codex-predictive-echo.test.ts` (E2E vs real codex), `packages/xterm-zerolag-input/test/codex-replay.test.ts`. ⚠️ **Pi is the opposite kind of CLI and needs the opposite instincts**: it has NO permission prompts and no sandbox, so there is no bypass flag to send and Codeman must not invent one; its privileged knob is the tri-state `approveProjectTrust` (`--approve`/`--no-approve`), which makes pi EXECUTE repo-local `.pi/extensions` TypeScript, so the multi-user clamp puts pi in the **materialize** branch (an absent config still yields `--no-approve` for a non-granted owner) and `--api-key` is never wired. Pi stays OUT of `isAltScreenStripMode()` (main-screen TUI, and its 0.84.0 fullscreen mode is runtime-switchable via `/settings`, where the alt screen is load-bearing), and lands on the `'buffer'` echo policy via the `_updateLocalEchoState` fallthrough. Pi's own tests: `test/pi-mode.test.ts`, `test/routes/external-cli-bypass-clamp.test.ts`; user guide `docs/pi-integration.md`. ⚠️ **Grok is codex-shaped on permissions but opencode-shaped on rendering**: its bypass switch is `alwaysApprove` (`--always-approve`, grok's `bypassPermissions` mode — the Run button sends it `true` like antigravity's, and the clamp's only-if-sent branch strips it for non-granted owners), while its fullscreen alt-screen TUI keeps it OUT of `isAltScreenStripMode()`; the resolver version-probes `grok --version` like pi's (npm squatters exist for the name — `GET /api/grok/status` surfaces path + version), and grok lands on the `'buffer'` echo policy via the fallthrough (UNMEASURED against a live authenticated session; if its composer turns out per-keystroke-reactive like codex, flip it to the `'off'` branch). Grok's own tests: `test/grok-mode.test.ts`, `test/grok-cli-resolver.test.ts`; user guide `docs/grok-integration.md`. ⚠️ **DeepSeek breaks three of this family's assumptions, so do not pattern-match it onto its siblings.** (1) The agent is a **PROFILE, not the binary**: `dsh` is a launcher over `$DSH_HOME/profiles/` and DeepSeek ships only `web`/`headless`/`base`, so the terminal front door is ALWAYS third-party and "installed" ≠ "runnable" — the Run button gates on `isDeepSeekRunnable()` (binary AND a pane-capable profile) while `isDeepSeekAvailable()` gates the "add a profile" affordance; a `web`/`headless` profile is refused at spawn because it cannot drive a pane. (2) The permission switch is the **`DSH_PERMISSION_MODE` env export, not a flag** (`read-only`/`workspace-write`/`danger-full-access`) — the harness has none, and this is the one legitimate exception to the effort-style env-var ban because it is read with `??` as a boot-time default, so it stays soft; absent = `workspace-write`, which asks, hence the only-if-sent clamp branch, clamping to `workspace-write` (never `read-only`, which would break the workspace). ⚠️ **That clamp needs a second half no other CLI needs**, because the switch is an env var and `DSH_*` is an allowlisted `envOverrides` prefix: `applyEnvOverrides()` runs AFTER `_configureDeepSeek()` in tmux-manager, so a non-granted owner sending `DSH_PERMISSION_MODE` on the SAME request would land last and hand back exactly the privilege the config clamp removed. `clampEnvOverridesForOwner()` (session-routes.ts) DROPS `DSH_PERMISSION_MODE`, `DSH_HOME` and `DEEPSEEK_BASE_URL` for a non-granted owner (the last because `_configureDeepSeek()` forwards the SERVER's own `DEEPSEEK_API_KEY` into the pane, so a redirected base URL would send it to a foreign host) (dropping falls through to what `_configureDeepSeek()` exports, which is the clamped value); `DSH_HOME` is there because it points the launcher at a profile tree whose plugin code runs at BOOT, before any approval row applies. Every OTHER CLI's bypass is a command-line flag reachable only through its config, which is why the config clamp alone is the whole gate for them. (3) It is the **only non-claude mode that passes `hooksAvailableForMode()`**, and for it alone that predicate is a per-SESSION question rather than a per-mode one (`deepSeekConfig.statusReporting: false` disarms the bridge, so every call site passes `sessionHookOptions(session)`; answering from the mode there re-creates the infinite-wait-dressed-as-a-timeout the guard exists to prevent). It passes because the terminal front door reports idle/working/blocked to a supervisor over a generic env-gated contract and `deepseek-status-shim.ts` makes Codeman that supervisor — real `stop`/`blocked` signals, real Approvals Inbox items, plus the `agent_working` event that clears an alert answered in the terminal. ⚠️ The resolver needs the strictest identity probe of the family (`dsh --help` must say `DeepSeek Harness`) because Debian ships an unrelated `dsh` (dancer's shell) that would pass a version probe. Model is NOT a session field (it is a profile composition entry). ⚠️ `hooksAvailableForMode()` is about hook SIGNALS and is not a stand-in for "is this a claude session": Read My Mind and intent capture read Claude's own transcript and compare `mode === 'claude'` directly, because when `deepseek` earned a yes the shared predicate silently widened both to a mode with no transcript to read (pinned by a static check in `test/deepseek-mode.test.ts`). ⚠️ **It is also the only external CLI whose answers are READ FROM DISK rather than scraped off the pane**: `deepseek-transcript.ts` reads `$DSH_HOME/sessions///session.jsonl.zstd` and backs the `last-response` route for dsh, because the pane segmenter served dsh-TUI's ASCII-art SPLASH as the worker's answer (measured), which anything polling for a first answer reads as an answer. Three traps live in that file: dsh appends **one zstd FRAME per write** and Node's `zlib` zstd decoder stops at the first (a real 56-line transcript decoded as 1 line, so the module walks frame headers itself; a Node older than 22.15 has no zstd and falls back to the pane); every turn also records a **plugin-sourced `user/message`** (the runtime-context snapshot) that must not render as the user's words; and a failed `turn/end` is surfaced as `Turn error: …` rather than as an empty string that reads as "still thinking". ⚠️ Session→transcript pairing is by the header's own `cwd` plus a ±60 s boot window, never by reproducing dsh's directory mangling (which has already changed form once) — and NEVER by newest-mtime alone, which handed a fresh worker its predecessor's answer in the same case dir. DeepSeek's own tests: `test/deepseek-mode.test.ts`, `test/deepseek-cli-resolver.test.ts`, `test/deepseek-transcript.test.ts`; user guide `docs/deepseek-integration.md`. OMP (`omp`) needs no bypass flag (the CLI's own `~/.omp` config governs trust/model routing, defaulting to `tools.approvalMode: yolo`), so `buildOmpCommand()` only ever passes `--model`/`--resume`/`--continue` — but the multi-user clamp is NOT a no-op for it: `OMP_*` is an allowlisted `envOverrides` prefix, and the two credential-resolution keys it admits, `OMP_AUTH_BROKER_URL`/`OMP_AUTH_BROKER_TOKEN`, are clamped in `clampEnvOverridesForOwner()` for a non-granted owner, the same shape as `DEEPSEEK_BASE_URL`. Separately, `PI_*` is already allowlisted (pi needs it) and omp reads several of its knobs too (`PI_CONFIG_DIR`, `PI_CODING_AGENT_DIR`, `PI_CODING_AGENT_SESSION_DIR`, `PI_SUBPROCESS_CMD`, `PI_SHELL_PREFIX`) — a redirected `PI_CONFIG_DIR` moves the `~/.omp` tree `omp-session-resolver.ts`/`omp-transcript.ts` hardcode, silently breaking pinning/history; this is a known gap shared with pi, not fixed here. → [architecture-invariants#external-cli-modes-opencode-codex-gemini-antigravity-pi-grok-deepseek-omp](docs/architecture-invariants.md#external-cli-modes-opencode-codex-gemini-antigravity-pi-grok-deepseek-omp) **Run launch synchronization**: the Run entrypoint holds an in-flight lock and disables `#runBtn` for the whole launch (≥500ms), so a double click cannot create duplicate sessions with the same `w-` name. `_ensureCreatedSessionVisible()` runs before `selectSession()`, and `_onSessionCreated()` stays an idempotent upsert, so POST-first and SSE-first ordering both produce exactly one rendered tab. ⚠️ **Closing has the mirror-image race and one owner**: `closeSession()` reads `wasActive` BEFORE its `await` and announces the delete via `_closingSessions`, while `_onSessionDeleted` skips the active-session handoff for an id in that set. Both used to read `activeSessionId` after the fact, so the `session_deleted` broadcast for your own delete could null it first and closing the tab you were on landed on the welcome screen instead of the next session, on the same build, depending on timing. The fallback also picks the first order entry that is still in `sessions` (a dead id can linger in `sessionOrder`, same reason Alt+N indexes a live-filtered list). A delete from ANOTHER client still shows the welcome screen, which is the honest answer when what you were looking at was taken away. Tests: `test/session-close-fallback.test.ts`. → [architecture-invariants#run-launch-synchronization](docs/architecture-invariants.md#run-launch-synchronization) diff --git a/docs/omp-integration.md b/docs/omp-integration.md index ae89ba99..79c3fea6 100644 --- a/docs/omp-integration.md +++ b/docs/omp-integration.md @@ -47,12 +47,30 @@ result is interpolated into the pane's spawn command. **omp reads its own model routing and hooks from `~/.omp`, so no trust or permission flags are needed** — unlike every sibling CLI in this family, there is no -bypass-permissions equivalent to wire up, and the multi-user owner clamp has nothing -to gate for `omp` (no branch needed, no privileged flag exists to strip). +bypass-permissions equivalent to wire up, so `buildOmpCommand()` only ever passes +`--model`/`--resume`/`--continue`. ⚠️ That does NOT mean omp is unrestricted: its +documented default `tools.approvalMode` is `yolo`, so an omp pane auto-approves exec +with no flag from Codeman — the CLI's own config, not Codeman, is what would need to +change that. -Env overrides: the `OMP_*` prefix is allowlisted. omp has no documented vendor-key -namespace of its own (its provider credentials live in `~/.omp` config files, not -environment variables), so nothing beyond `OMP_*` is admitted. +Env overrides: the `OMP_*` prefix is allowlisted, and per omp's own +`docs/environment-variables.md` it is not the narrow surface it looks like. omp reads +roughly 40 provider keys from the environment (`ANTHROPIC_API_KEY`, `OPENAI_API_KEY`, +`XAI_API_KEY`, `HF_TOKEN`, ...) — pi's 34-key problem in the same shape — which is why +none of those get a dedicated allowlist entry; a session authenticates from `~/.omp` +config or the server process's own env instead, like pi. omp's own documented knobs +are mostly `PI_*`, not `OMP_*` (`PI_CONFIG_DIR`, `PI_CODING_AGENT_DIR`, +`PI_CODING_AGENT_SESSION_DIR`, `PI_SUBPROCESS_CMD`, `PI_SHELL_PREFIX`, +`OMP_PROFILE`/`PI_PROFILE`), and `PI_*` is already allowlisted globally because pi +mode needs it — so an omp session today already accepts all of those. The first three +also move the tree `omp-session-resolver.ts` and `omp-transcript.ts` hardcode +(`resolveOmpHome()` assumes `~/.omp` unconditionally), so pinning and history quietly +stop working under a redirected config root; this is a known gap, not fixed here. + +The `OMP_` prefix itself brings in `OMP_AUTH_BROKER_URL` / `OMP_AUTH_BROKER_TOKEN`, +where omp resolves credentials from — the same shape `DEEPSEEK_BASE_URL` is dropped +for in `clampEnvOverridesForOwner()` (session-routes.ts), so both are clamped there +for a non-granted owner in multi-user mode. None of this matters in single-user mode. ## Exact-id pinning: why `--resume`, not just `--continue` diff --git a/src/utils/omp-session-resolver.ts b/src/utils/omp-session-resolver.ts index 7b1a5ed7..e01ac00a 100644 --- a/src/utils/omp-session-resolver.ts +++ b/src/utils/omp-session-resolver.ts @@ -50,7 +50,13 @@ export function mangleOmpWorkingDir(workingDir: string): string { return relative.replace(/\//g, '-'); } -/** `~/.omp` — no known env override exists (unlike DSH_HOME); revisit if omp adds one. */ +/** + * `~/.omp` — omp's own env overrides are mostly `PI_*` (shared with pi mode, already + * allowlisted in schemas.ts), and `PI_CONFIG_DIR` in particular can move this root. + * That is not honored here: a session with a redirected `PI_CONFIG_DIR` silently + * degrades pinning/history to omp's own ambiguous `--continue` instead of erroring, + * a known gap (found in Ark0N/Codeman#353 review) shared with pi and not fixed here. + */ function resolveOmpHome(): string { return join(homedir(), '.omp'); } diff --git a/src/web/routes/session-routes.ts b/src/web/routes/session-routes.ts index 7d5a7cf0..fe50bd92 100644 --- a/src/web/routes/session-routes.ts +++ b/src/web/routes/session-routes.ts @@ -399,10 +399,10 @@ export const _clampExternalCliBypassForOwner = clampExternalCliBypassForOwner; /** * Env-var keys a non-granted owner must not be able to set, because each one - * hands back privilege the config clamp above just removed — or, for the last, - * redirects a credential the server injects. + * hands back privilege the config clamp above just removed, or redirects a + * credential-resolution endpoint. * - * All are DeepSeek's, and all are reachable because `DSH_*` and `DEEPSEEK_*` are + * The DeepSeek three are reachable because `DSH_*` and `DEEPSEEK_*` are * allowlisted `envOverrides` prefixes (schemas.ts) — which they have to be, since * that is also how a user configures the harness's non-privileged knobs. * @@ -419,8 +419,24 @@ export const _clampExternalCliBypassForOwner = clampExternalCliBypassForOwner; * URL would have the operator's API key sent as a bearer credential to a host * of their choosing. (`DEEPSEEK_API_KEY` itself stays overridable: supplying * your OWN key removes privilege rather than granting it.) + * - `OMP_AUTH_BROKER_URL`/`OMP_AUTH_BROKER_TOKEN` are where omp resolves + * credentials from — the same shape as `DEEPSEEK_BASE_URL` above, reachable + * because `OMP_*` is an allowlisted prefix. Unlike DeepSeek, Codeman does not + * forward any operator-held key into an omp pane today (omp's provider + * credentials live in `~/.omp` config files, not env vars), so there is no + * known concrete exfiltration path yet — clamped defensively anyway, since a + * non-granted owner redirecting where a shared multi-tenant deployment + * resolves auth from is not something to allow silently (found in + * Ark0N/Codeman#353 review; omp's own knobs are otherwise mostly `PI_*`, + * already allowlisted for pi and not addressed here — see resolveOmpHome()). */ -const OWNER_CLAMPED_ENV_KEYS = ['DSH_PERMISSION_MODE', 'DSH_HOME', 'DEEPSEEK_BASE_URL'] as const; +const OWNER_CLAMPED_ENV_KEYS = [ + 'DSH_PERMISSION_MODE', + 'DSH_HOME', + 'DEEPSEEK_BASE_URL', + 'OMP_AUTH_BROKER_URL', + 'OMP_AUTH_BROKER_TOKEN', +] as const; /** * Env-var half of the multi-user bypass clamp. diff --git a/test/omp-mode.test.ts b/test/omp-mode.test.ts index 941b8cd0..dd7aa5a3 100644 --- a/test/omp-mode.test.ts +++ b/test/omp-mode.test.ts @@ -1,9 +1,10 @@ -import { describe, expect, it } from 'vitest'; +import { describe, expect, it, beforeEach, afterEach } from 'vitest'; import { CreateSessionSchema, QuickStartSchema } from '../src/web/schemas.js'; import { buildSpawnCommand } from '../src/tmux-manager.js'; import { defaultDockerCommandForMode } from '../src/docker-hosts.js'; import { defaultRemoteCommandForMode } from '../src/remote-hosts.js'; import { isExternalCliMode, isAltScreenStripMode } from '../src/session.js'; +import { _clampEnvOverridesForOwner } from '../src/web/routes/session-routes.js'; describe('OMP mode schemas', () => { it('accepts OMP session creation config', () => { @@ -132,3 +133,33 @@ describe('OMP mode gates', () => { expect(defaultRemoteCommandForMode('omp')).toBe('exec "${SHELL:-/bin/sh}" -i -l -c \'omp\''); }); }); + +describe('OMP multi-user clamp: the env-var half', () => { + // Unlike DeepSeek, omp has no permission FLAG or CONFIG for the clamp to + // gate (buildOmpCommand() only ever emits --model/--resume/--continue), so + // the only privilege surface is the two credential-resolution env vars the + // OMP_* prefix admits. + const ORIGINAL = process.env.CODEMAN_MULTIUSER; + beforeEach(() => { + process.env.CODEMAN_MULTIUSER = '1'; + }); + afterEach(() => { + if (ORIGINAL === undefined) delete process.env.CODEMAN_MULTIUSER; + else process.env.CODEMAN_MULTIUSER = ORIGINAL; + }); + + it('strips OMP_AUTH_BROKER_URL and OMP_AUTH_BROKER_TOKEN, leaving unrelated overrides alone', async () => { + const out = await _clampEnvOverridesForOwner('nobody', { + OMP_AUTH_BROKER_URL: 'https://attacker.example/broker', + OMP_AUTH_BROKER_TOKEN: 'stolen-token', + OMP_PROFILE: 'default', + }); + expect(out).toEqual({ OMP_PROFILE: 'default' }); + }); + + it('is a no-op in single-user mode', async () => { + delete process.env.CODEMAN_MULTIUSER; + const input = { OMP_AUTH_BROKER_URL: 'https://attacker.example/broker' }; + expect(await _clampEnvOverridesForOwner(undefined, input)).toBe(input); + }); +});