diff --git a/CLAUDE.md b/CLAUDE.md index 31b0be65..09f77a6a 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -172,7 +172,7 @@ Codeman is a Claude Code session manager with web interface and autonomous Ralph **CLI registry** (`src/config/cli-registry/`): every run mode is a `CliEntry` (discovery, launch argv template, env handling, `capabilities`, and the `overlays` behind remote/docker pane commands). **No code outside `stock.ts` may branch on a CLI id**: use a capability field or a NAMED PROFILE (`profiles.ts`); `test/cli-registry-no-id-branching.test.ts` and `test/frontend-cli-no-id-branching.test.ts` enforce it. ⚠️ Config holds typed argv tokens, never shell text; literals are validated at LOAD time and a bad one rejects the whole entry. ⚠️ Keep `external`, `hooks` and `altScreen` independent; never derive one from another. ⚠️ Config regexes (`discovery.version.regex`, `capabilities.workDetect.workingLine`, `capabilities.workDetect.watchingLine`, `capabilities.workDetect.awaitingLine`, `capabilities.workDetect.blockedLine`, `capabilities.modelDetect.screenLine`) must compile through `compileVersionRegex()`. ⚠️ `privilegedParams[].param` names a LAUNCH PARAM, not the legacy `Config` field (bridged only by `launch.legacyConfigAliases`); a wrong name silently clamps nothing. ⚠️ Resolve the registry AT CALL TIME, never in a module-level const. ⚠️ Remote claude/omp arms of `buildRemoteLaunchCommand` are not covered by the pane-command golden. `~/.codeman/clis.json` overrides entries. It is WRITTEN only by the opt-in CLI management routes (`cliManagementEnabled`, default OFF; `/api/clis`, `cli-registry-routes.ts`), and only through `mutateRegistryFile()` in `registry-writer.ts`, which serializes mutations and refuses (409) a file that does not parse or has group/world permission bits rather than overwriting it. Importing the registry still writes nothing. → [architecture-invariants#cli-registry](docs/architecture-invariants.md#cli-registry), `docs/cli-registry.md` -**External CLI modes (OpenCode, Codex, Gemini, Antigravity, Pi, Grok, DeepSeek, OMP, Copilot)**: `isExternalCliMode()` in `session.ts` gates Claude-specific behavior off (Ralph tracker, BashToolParser, token parsing, ❯ readiness; readiness is output stabilization); work detection is per-CLI `capabilities.workDetect` data, not this gate. All nine **require tmux, no direct PTY fallback** (secrets go via socket-scoped `tmux setenv`, never the command line). ⚠️ `run*()` in `session-ui.js` MUST unwrap the `{success,data}` envelope. ⚠️ **Codex uses predictive write-through echo, never the buffer overlay**: `_predictHookOnData` must never `return` (wire path stays byte-identical), and flushed text and a bracketed paste must go out as separate delayed writes. ⚠️ **Pi**: no bypass flag, never invent one; `approveProjectTrust` executes repo code, so it is in the clamp's **materialize** branch; never wire `--api-key`. ⚠️ **Grok**: `alwaysApprove` is stripped for non-granted owners (only-if-sent). ⚠️ **DeepSeek**: the agent is a PROFILE (Run gates on `isDeepSeekRunnable()`); the permission switch is the `DSH_PERMISSION_MODE` env var, so `clampEnvOverridesForOwner()` must DROP `DSH_PERMISSION_MODE`, `DSH_HOME` and `DEEPSEEK_BASE_URL` for non-granted owners; `hooksAvailableForMode()` is per-SESSION for it (pass `sessionHookOptions(session)`) and is never a stand-in for `mode === 'claude'`; answers come from `deepseek-transcript.ts`, paired by header `cwd` + boot window, never newest-mtime. ⚠️ **OMP**: `OMP_AUTH_BROKER_URL`/`_TOKEN` are clamped the same way. ⚠️ **Copilot**: `allowAll` (`--yolo`) is a `privilegedParam` and `COPILOT_HOME`/`COPILOT_ALLOW_ALL`/`COPILOT_PROVIDER_*`/`COPILOT_MODEL` are privileged env keys; the resume id must start with a letter or digit (`path-segment`), since `--resume`'s value is optional and `--resume --yolo` would otherwise make `--yolo` its own flag; `--name` is dropped next to `--resume`/`--continue` (Copilot refuses the pair); its mouse DECSETs are stripped (`strip-mux-and-mouse`) so a drag selects and the wheel is forwarded by `_shouldForwardWheelToApp` while `cliMouseTracking` is on; `composerReadyMark` is the footer hint `/ commands · ? help`, NOT `❯` (the folder-trust dialog draws `❯ 1. Yes`); `last-response` reads the pane's visible screen (`transcriptNeedsScreenCapture`) through a per-CLI `PANE_DIALECTS` row, because its stream is cursor moves, not lines (a prompt row scrolled off by a tall answer reads as the answer's tail unless a banner/trust dialog is on screen, and typed composer text is not a prompt); Docker seeds no Copilot credential (`config.json` holds a plain-text token without a keyring). → [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, Copilot)**: `isExternalCliMode()` in `session.ts` gates Claude-specific behavior off (Ralph tracker, BashToolParser, token parsing, ❯ readiness; readiness is output stabilization); work detection is per-CLI `capabilities.workDetect` data, not this gate. All nine **require tmux, no direct PTY fallback** (secrets go via socket-scoped `tmux setenv`, never the command line). ⚠️ `run*()` in `session-ui.js` MUST unwrap the `{success,data}` envelope. ⚠️ **Codex uses predictive write-through echo, never the buffer overlay**: `_predictHookOnData` must never `return` (wire path stays byte-identical), and flushed text and a bracketed paste must go out as separate delayed writes. ⚠️ **Pi**: no bypass flag, never invent one; `approveProjectTrust` executes repo code, so it is in the clamp's **materialize** branch; never wire `--api-key`. ⚠️ **Grok**: `alwaysApprove` is stripped for non-granted owners (only-if-sent). ⚠️ **DeepSeek**: the agent is a PROFILE (Run gates on `isDeepSeekRunnable()`); the permission switch is the `DSH_PERMISSION_MODE` env var, so `clampEnvOverridesForOwner()` must DROP `DSH_PERMISSION_MODE`, `DSH_HOME` and `DEEPSEEK_BASE_URL` for non-granted owners; `hooksAvailableForMode()` is per-SESSION for it (pass `sessionHookOptions(session)`) and is never a stand-in for `mode === 'claude'`; answers come from `deepseek-transcript.ts`, paired by header `cwd` + boot window, never newest-mtime. ⚠️ **OMP**: `OMP_AUTH_BROKER_URL`/`_TOKEN` are clamped the same way. ⚠️ **Copilot**: `allowAll` (`--yolo`) is a `privilegedParam` and `COPILOT_HOME`/`COPILOT_ALLOW_ALL`/`COPILOT_PROVIDER_*`/`COPILOT_MODEL` are privileged env keys, and so is every other `COPILOT_*` variable the CLI reads that switches approvals, runs code with no approval row or sends its sign-in token elsewhere (the 1.0.98 list is in `stock.ts`; the prefix admits whatever a later release adds, so re-read the bundle whenever the measured version moves); the resume id must start with a letter or digit (`path-segment`), since `--resume`'s value is optional and `--resume --yolo` would otherwise make `--yolo` its own flag; `--name` is dropped next to `--resume`/`--continue` (Copilot refuses the pair); its mouse DECSETs are stripped (`strip-mux-and-mouse`) so a drag selects and the wheel is forwarded by `_shouldForwardWheelToApp` while `cliMouseTracking` is on; `composerReadyMark` is the footer hint `/ commands · ? help`, NOT `❯` (the folder-trust dialog draws `❯ 1. Yes`); `last-response` reads the pane's visible screen (`transcriptNeedsScreenCapture`) through a per-CLI `PANE_DIALECTS` row, because its stream is cursor moves, not lines (a prompt row scrolled off by a tall answer reads as the answer's tail unless a banner/trust dialog is on screen, and typed composer text is not a prompt); Docker seeds no Copilot credential (`config.json` holds a plain-text token without a keyring). → [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) **DeepSeek web UI** (`POST`/`GET`/`DELETE /api/deepseek/web`, `deepseek-web-server.ts`): the Run menu's "DeepSeek web UI..." entry supervises ONE background `dsh web` child process, deliberately **NOT a shell session**. ⚠️ Every piece is load-bearing: exactly one server (a second click REUSES it), restarted when the browser authority changes (`--trusted-host`, last asker wins), killed on shutdown via `stopDeepSeekWeb()` (the detached child would otherwise outlive Codeman and hold its port), and failures returned to the caller. ⚠️ Never hardcode the port: search from 3080 across 40, detect free ports by BINDING, then wait for the server to really answer. ⚠️ Both `POST` and `DELETE` must stay behind `canUsernameRunPrivilegedCommands` (booting a profile runs its plugin code; the server is shared). → [architecture-invariants#deepseek-web-ui](docs/architecture-invariants.md#deepseek-web-ui) diff --git a/docs/cli-registry.md b/docs/cli-registry.md index fba15d6a..359ef643 100644 --- a/docs/cli-registry.md +++ b/docs/cli-registry.md @@ -352,4 +352,4 @@ The rules the module keeps and the tests pin: it only ADDS (a name already defin `copilot` (npm `@github/copilot`) is a stock entry. A bare launch is `copilot`, in its own Manual Approval mode. The launch parameters follow the grok/omp shape: `model` (`--model`), `resumeId` (`--resume `, an id, id prefix or session name: a single word that starts with a letter or digit, so a flag-shaped value such as `--yolo` is dropped rather than read as its own flag) and `continueSession` (`--continue`, skipped when a resume id is set), plus `allowAll` (`--yolo`: allow every tool, path and URL). The legacy config object is `copilotConfig` (`{ model, allowAll, resumeSessionId, continueSession }`). The Run button sends `allowAll: true` like the other agent CLIs' bypass switches, and `allowAll` is declared in `privilegedParams`, so a non-granted multi-user owner has it forced back to `false` (an absent config is never materialized: a bare spawn is already safe). Session history resume goes through `copilotConfig.resumeSessionId` / `continueSession`. The tab names the model it reads from the footer (`modelDetect`). Its `altScreen` is `strip-mux-and-mouse`, like opencode: Copilot turns mouse tracking on itself, and left in, a plain drag is reported to the TUI instead of selecting (Auto Copy and copy-on-select did nothing); stripped, a drag is a local selection, clicks go through the hand-encoded tap, and the wheel is forwarded as SGR reports while `cliMouseTracking` is on. It declares `mcpConfig` (`~/.copilot/mcp-config.json`, relocated by `COPILOT_HOME`) and no `credStore`: its token sits in `config.json` in plain text on a host without a keyring, so a Docker case is not seeded and you sign in inside the container. `env.allowedKeys` is empty on purpose: a headless host sets `COPILOT_GITHUB_TOKEN`, which the `COPILOT_` prefix admits, and `GH_TOKEN`/`GITHUB_TOKEN` stay out of the one global allowlist. -Measured on 1.0.94 inside tmux: an alternate-screen TUI with mouse tracking on, whose composer is the `❯` between two rules. While a turn runs the footer reads `◉ Working esc edit prompt` (the dot alternates `◉`/`◎`), which is the `workDetect` working line. Keystrokes sent with `tmux send-keys` reach the composer, Shift+Enter inserts a newline, and a shell tool call completes inside tmux; github/copilot-cli#4180 (a driven PTY ignores input) and #4223 (a shell command never completes under tmux) report both failing on 1.0.70 to 1.0.74 and neither reproduces on 1.0.94. `COPILOT_HOME` and `COPILOT_ALLOW_ALL` are privileged env keys, so a non-granted multi-user owner cannot set them through `envOverrides`. +Measured on 1.0.94 inside tmux: an alternate-screen TUI with mouse tracking on, whose composer is the `❯` between two rules. While a turn runs the footer reads `◉ Working esc edit prompt` (the dot alternates `◉`/`◎`), which is the `workDetect` working line. Keystrokes sent with `tmux send-keys` reach the composer, Shift+Enter inserts a newline, and a shell tool call completes inside tmux; github/copilot-cli#4180 (a driven PTY ignores input) and #4223 (a shell command never completes under tmux) report both failing on 1.0.70 to 1.0.74 and neither reproduces on 1.0.94. `COPILOT_HOME` and `COPILOT_ALLOW_ALL` are privileged env keys, so a non-granted multi-user owner cannot set them through `envOverrides`, and so are the other `COPILOT_*` variables (read from the 1.0.98 bundle, far more than `copilot help environment` lists) that switch approvals, run code with no approval row or send the sign-in token elsewhere. The `COPILOT_` prefix admits whatever a later release adds, so the list needs a look whenever the measured version moves. diff --git a/docs/copilot-integration.md b/docs/copilot-integration.md index 54a00335..c9c64a86 100644 --- a/docs/copilot-integration.md +++ b/docs/copilot-integration.md @@ -48,7 +48,7 @@ Codeman never logs Copilot in or stores its credentials. | --- | --- | | Launch | `copilot`, in Copilot's own Manual Approval mode. | | Run button | `copilot --yolo` (allow every tool, path and URL), like the other CLIs' bypass switches, so an unattended session never stops on an approval. | -| Permission clamp | `allowAll` is a `privilegedParam`. A multi-user owner without the bypass grant has it forced back to `false`; an absent config is never materialized (a bare `copilot` is already safe). `COPILOT_HOME`, `COPILOT_ALLOW_ALL`, `COPILOT_PROVIDER_*` and `COPILOT_MODEL` are privileged env keys. | +| Permission clamp | `allowAll` is a `privilegedParam`. A multi-user owner without the bypass grant has it forced back to `false`; an absent config is never materialized (a bare `copilot` is already safe). `COPILOT_HOME`, `COPILOT_ALLOW_ALL`, `COPILOT_PROVIDER_*` and `COPILOT_MODEL` are privileged env keys, and so is every other `COPILOT_*` variable the CLI reads that switches approvals (`COPILOT_PLAN_THEN_AUTOPILOT`), runs code with no approval row (`COPILOT_CLI_DIST_DIR`, the package caches, `COPILOT_EDITOR`, `COPILOT_PROVIDER_API_KEY_COMMAND`, skills, hooks) or sends the sign-in token to another host (`COPILOT_API_URL`, `COPILOT_GH_HOST`, ...): the list is in `stock.ts`, read from the 1.0.98 bundle. A variable a later Copilot release adds is not covered until it is listed there. | | Model | `--model `, sent only by an API caller that sets `copilotConfig.model` (the value must start with a letter or digit, like `resumeSessionId`). Nothing in the UI sets it: a saved custom endpoint sets `COPILOT_MODEL` instead (see below). | | Resume | `--resume `, sent only by an API caller that sets `copilotConfig.resumeSessionId` (Past Sessions does not). The value (an id, an id prefix or a session name) must be one plain word starting with a letter or digit: `--resume`'s value is optional, so `--resume --yolo` would otherwise make `--yolo` its own flag and bypass the clamp. | | Continue | `--continue`, from **Past Sessions**: it resumes the most recent conversation Copilot has for that directory. Nothing sets it on a respawn. Skipped when a resume value is set. | diff --git a/src/config/cli-registry/schema.ts b/src/config/cli-registry/schema.ts index 541a5e10..de4a5542 100644 --- a/src/config/cli-registry/schema.ts +++ b/src/config/cli-registry/schema.ts @@ -464,8 +464,10 @@ const capabilitiesSchema = z ) .max(8), // Exact env var NAMES, not prefixes: this list is a targeted deny, and a prefix here - // would let one entry silently strip a whole namespace off every owner's overrides. - privilegedEnvKeys: z.array(envName).max(8), + // would let one entry silently strip a whole namespace off every owner's overrides. The + // bound is generous because a longer list only takes more away from non-granted owners: + // copilot's COPILOT_ prefix covers dozens of variables that run code or redirect its token. + privilegedEnvKeys: z.array(envName).max(48), gates: z.record(z.string(), z.object({ minVersion: z.string().max(20), failClosed: z.boolean() }).strict()), maxFrameBytes: z.number().int().positive().optional(), newline: z.enum(['line-feed', 'esc-enter']).optional(), diff --git a/src/config/cli-registry/stock.ts b/src/config/cli-registry/stock.ts index 1f0b641a..ae0c133c 100644 --- a/src/config/cli-registry/stock.ts +++ b/src/config/cli-registry/stock.ts @@ -1795,14 +1795,54 @@ const COPILOT: CliEntry = { format: 'copilot-json', relocation: { envVar: 'COPILOT_HOME', path: 'mcp-config.json' }, }, - // COPILOT_HOME can restate permissions and COPILOT_ALLOW_ALL turns every one on; both - // already match the COPILOT_ allowedPrefix, so a non-granted multi-user owner must not - // be able to set them through envOverrides. + // Every one of these matches the COPILOT_ allowedPrefix, so a non-granted multi-user owner + // must not be able to set it through envOverrides. Read from the 1.0.98 bundle (copilot reads + // far more COPILOT_* variables than `copilot help environment` lists), by what each hands back: + // - approval: COPILOT_ALLOW_ALL turns every tool on; COPILOT_PLAN_THEN_AUTOPILOT switches a + // plan into autopilot; COPILOT_ASSISTED_APPROVAL; COPILOT_HOME restates the permission config. + // - code at boot or without an approval row: COPILOT_CLI_DIST_DIR runs `/index.js` in place + // of the CLI, the package caches pick which CLI build runs, COPILOT_EDITOR, + // COPILOT_PROVIDER_API_KEY_COMMAND and COPILOT_AHP_HOST_BIN are run as commands, skills and + // provider configs are loaded from the named paths, and the hook switches widen what hooks run. + // - credential redirects: the Copilot and GitHub API bases receive the CLI's sign-in token, which + // in multi-user mode is the operator's (every user shares the OS account and ~/.copilot), and + // the BYOK provider family aims a Codeman custom endpoint elsewhere. + // The prefix stays because COPILOT_GITHUB_TOKEN and the harmless knobs live in it; a variable a + // later release adds is NOT covered until it is listed here. privilegedEnvKeys: [ 'COPILOT_HOME', 'COPILOT_ALLOW_ALL', + 'COPILOT_PLAN_THEN_AUTOPILOT', + 'COPILOT_ASSISTED_APPROVAL', + 'COPILOT_CLI_DIST_DIR', + 'COPILOT_CLI_RESOLVED_DIST_DIR', + 'COPILOT_PKG_CACHE_HOME', + 'COPILOT_CACHE_HOME', + 'COPILOT_EDITOR', + 'COPILOT_AHP_HOST_BIN', + 'COPILOT_DEBUG_VOICE_ENGINE_SCRIPT', + 'COPILOT_SKILLS_DIRS', + 'COPILOT_PROVIDERS_CONFIG', + 'COPILOT_HOOK_ALLOW_HTTP_AUTH_HOOKS', + 'COPILOT_HOOK_ALLOW_LOCALHOST', + 'COPILOT_E2E_TEST_HOOKS', + 'COPILOT_API_URL', + 'COPILOT_DEBUG_GITHUB_API_URL', + 'COPILOT_GH_HOST', + 'COPILOT_MC_BASE_URL', + 'COPILOT_AHP_URL', + 'COPILOT_DEBUG_ENTRA_BROKER_URL', + 'COPILOT_CAFE_METRICS_URL', 'COPILOT_PROVIDER_BASE_URL', 'COPILOT_PROVIDER_API_KEY', + 'COPILOT_PROVIDER_API_KEY_COMMAND', + 'COPILOT_PROVIDER_BEARER_TOKEN', + 'COPILOT_PROVIDER_HEADERS', + 'COPILOT_PROVIDER_TYPE', + 'COPILOT_PROVIDER_WIRE_MODEL', + 'COPILOT_PROVIDER_MODEL_ID', + 'COPILOT_PROVIDER_GHES_HOST', + 'COPILOT_PROVIDER_GHES_TOKEN', 'COPILOT_MODEL', ], // Grok/codex-shaped clamp: a bare spawn is already Manual Approval, so the multi-user clamp diff --git a/test/copilot-custom-endpoint.test.ts b/test/copilot-custom-endpoint.test.ts index dee7611c..a59967f0 100644 --- a/test/copilot-custom-endpoint.test.ts +++ b/test/copilot-custom-endpoint.test.ts @@ -1,5 +1,7 @@ -import { describe, it, expect } from 'vitest'; +import { afterEach, beforeEach, describe, it, expect } from 'vitest'; import { STOCK_CLIS } from '../src/config/cli-registry/stock.js'; +import { _clampEnvOverridesForOwner } from '../src/web/routes/session-routes.js'; +import { CreateSessionSchema } from '../src/web/schemas.js'; const copilot = STOCK_CLIS.find((c) => (c.id as string) === 'copilot')!; @@ -44,3 +46,49 @@ describe('GitHub Copilot environment and sync declarations', () => { expect(copilot.overlays?.credStore).toBeUndefined(); }); }); + +describe('GitHub Copilot multi-user clamp: the env-var half', () => { + // COPILOT_ is an allowlisted prefix, so every variable below would otherwise pass the schema. + // Each one hands back what the allowAll clamp removed: an approval switch, code that runs with no + // approval row (a CLI build, an editor, a key command, skills, hooks), or the sign-in token sent to + // a host of the caller's choosing. + const DANGEROUS: Record = { + COPILOT_ALLOW_ALL: 'true', + COPILOT_PLAN_THEN_AUTOPILOT: '1', + COPILOT_HOME: '/tmp/planted-copilot-home', + COPILOT_CLI_DIST_DIR: '/tmp/planted-dist', + COPILOT_PKG_CACHE_HOME: '/tmp/planted-cache', + COPILOT_CACHE_HOME: '/tmp/planted-cache', + COPILOT_EDITOR: 'sh -c planted', + COPILOT_PROVIDER_API_KEY_COMMAND: 'sh -c planted', + COPILOT_SKILLS_DIRS: '/tmp/planted-skills', + COPILOT_PROVIDERS_CONFIG: '/tmp/planted-providers.json', + COPILOT_API_URL: 'https://attacker.example', + COPILOT_GH_HOST: 'attacker.example', + COPILOT_PROVIDER_BASE_URL: 'https://attacker.example/v1', + COPILOT_PROVIDER_HEADERS: 'X-Planted: 1', + }; + 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('passes the env allowlist, so only the clamp stands between these and the pane', () => { + expect(() => + CreateSessionSchema.parse({ workingDir: '/tmp', mode: 'copilot', envOverrides: DANGEROUS }) + ).not.toThrow(); + }); + + it("strips every one of them from a non-granted owner's overrides and keeps the harmless knobs", async () => { + const out = await _clampEnvOverridesForOwner('nobody', { + ...DANGEROUS, + COPILOT_GITHUB_TOKEN: 'own-token', + COPILOT_AUTO_UPDATE: 'false', + }); + expect(out).toEqual({ COPILOT_GITHUB_TOKEN: 'own-token', COPILOT_AUTO_UPDATE: 'false' }); + }); +});