From 5fc391a47ce51de9d7c51601e64280913ed6fe58 Mon Sep 17 00:00:00 2001 From: Devvyn <22340871+opticon454@users.noreply.github.com> Date: Sat, 19 Sep 2026 18:05:51 +0800 Subject: [PATCH] fix(custom-model): address fourth pre-merge review + merge upstream master (Ark0N) Merged upstream/master (22 commits: reboot-restore recovery feature, terminal keycode229 recovery work, install.sh/CLI-catalog generator changes, CHANGELOG/version bump to 1.30.0) into this branch. No conflicts; git auto-merged every overlapping file (CLAUDE.md, docs/api-reference.md, app.js, index.html, styles.css, routes/index.ts, session-routes.ts, schemas.ts, server.ts). Two required fixes from the latest review: 1. privilegedEnvKeys widening (stock.ts) changes behaviour outside this feature. The reviewer decided to keep both CLAUDE_CODE_MAX_CONTEXT_TOKENS and CLAUDE_CONFIG_DIR listed (types.ts's rule that every traffic- redirecting var this feature introduces must appear there stays literally true), and asked for the real consequences documented instead of hidden: - Corrected session-env-clamp.ts's fileoverview, which stated the opposite of what the code now does (reboot-restore's clamp call used to be able to strip nothing for claude; it now strips a persisted CLAUDE_CONFIG_DIR for a non-granted owner). - Corrected the rationale comments in stock.ts: privilegedEnvKeys has exactly one consumer (ownerClampedEnvKeys, feeding the generic envOverrides clamp on create/quick-start/reboot-restore), not the custom-model routes. - Added a CLAUDE.md line to the CLAUDE_CONFIG_DIR gotcha covering the admin-only-in-multi-user-mode and reboot-restore-strips-it consequences. - Added a "Claude multi-user clamp" test next to the existing DeepSeek/OMP ones, pinning the new stripping behaviour. 2. GET .../running-status (custom-model-routes.ts) no longer passes the raw llama-swap `cmd` field (the literal launch line, which can carry model paths and --api-key) to the browser -- the frontend only ever reads model/state, cmd exists solely for server-side parseCtxFromCmd() during discovery. Added a test asserting the response never contains cmd or a planted secret. Also regenerated config/clis.stock.json and install.sh's catalogue block (npm run generate:cli-catalog) to clear drift introduced by the upstream merge, since it was failing the sync check. Left to the reviewer, as they said they'd take at merge: the two "comments pointing at removed code" cleanups, the two stale CLAUDE.md counts, and the small items list (mode==='claude' frontend branch, isCliAvailable() unknown-id gap, shared confirmed flag ordering, one-shot cancel toast severity, pumpLlamaSwapLogTail buffer cap). Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01Ea59JhUmHBm1gRCsiYF33R --- CLAUDE.md | 2 +- src/config/cli-registry/stock.ts | 20 +++++++++---- src/session-env-clamp.ts | 19 +++++++----- src/web/routes/custom-model-routes.ts | 17 +++++++++-- test/routes/custom-model-routes.test.ts | 28 +++++++++++++++++ test/routes/session-custom-model.test.ts | 38 +++++++++++++++++++++++- 6 files changed, 108 insertions(+), 16 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index c9e0b1cb..ac85ef29 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -132,7 +132,7 @@ Codeman is a Claude Code session manager with web interface and autonomous Ralph - **ESM only** — Never `require()`, use `await import()`. `tsx` masks CJS/ESM issues in dev but production breaks - **Package ≠ product name** — npm: `aicodeman`, product: **Codeman**. Release renames tags accordingly. Both `aicodeman` and `codeman` bin aliases are installed (`package.json` `bin`) - **Global regex `lastIndex`** — Shared `g`-flag patterns in loops must reset `lastIndex = 0` first, or use the `execPattern()` helper in `utils/regex-patterns.ts` (resets automatically) -- **`envOverrides` flow `CLAUDE_CODE_*` / `OPENCODE_*` / `CODEX_*` / `GEMINI_*` / `GOOGLE_*` / `ANTIGRAVITY_*` / `PI_*` / `GROK_*` / `XAI_*` / `DSH_*` / `DEEPSEEK_*` env vars, plus exact-key `CLAUDE_CONFIG_DIR`** — Set via `POST /api/sessions { envOverrides }`, stored on `Session._envOverrides`, exported by `tmux-manager.buildEnvExports()` at spawn time, persisted in `SessionState.envOverrides`. **Do NOT** write these to `/.claude/settings.local.json` — that's the old path and creates UI/disk drift. (`GOOGLE_*` is the deliberately-broad Vertex-AI namespace for Gemini — see Multi-CLI prefix discipline.) `CLAUDE_CONFIG_DIR` (#255, exact match via `ALLOWED_ENV_KEYS` in `schemas.ts`) points a session at a separate Claude account/config dir for per-client subscriptions; it persists to state.json (a path, not a secret; losing it on restart would silently switch accounts). ⚠️ A relocated config dir writes transcripts outside `~/.claude/projects`, so the response viewer, subagent windows, ultracode panel and Read My Mind capture go blind for that session unless the user symlinks `projects` back into the shared tree (`ln -s ~/.claude/projects /projects`). → [architecture-invariants#per-session-env-overrides-exact-key-allowlist-and-claude_config_dir](docs/architecture-invariants.md#per-session-env-overrides-exact-key-allowlist-and-claude_config_dir) +- **`envOverrides` flow `CLAUDE_CODE_*` / `OPENCODE_*` / `CODEX_*` / `GEMINI_*` / `GOOGLE_*` / `ANTIGRAVITY_*` / `PI_*` / `GROK_*` / `XAI_*` / `DSH_*` / `DEEPSEEK_*` env vars, plus exact-key `CLAUDE_CONFIG_DIR`** — Set via `POST /api/sessions { envOverrides }`, stored on `Session._envOverrides`, exported by `tmux-manager.buildEnvExports()` at spawn time, persisted in `SessionState.envOverrides`. **Do NOT** write these to `/.claude/settings.local.json` — that's the old path and creates UI/disk drift. (`GOOGLE_*` is the deliberately-broad Vertex-AI namespace for Gemini — see Multi-CLI prefix discipline.) `CLAUDE_CONFIG_DIR` (#255, exact match via `ALLOWED_ENV_KEYS` in `schemas.ts`) points a session at a separate Claude account/config dir for per-client subscriptions; it persists to state.json (a path, not a secret; losing it on restart would silently switch accounts). ⚠️ A relocated config dir writes transcripts outside `~/.claude/projects`, so the response viewer, subagent windows, ultracode panel and Read My Mind capture go blind for that session unless the user symlinks `projects` back into the shared tree (`ln -s ~/.claude/projects /projects`). ⚠️ It is also one of claude's `privilegedEnvKeys` (Custom Model Endpoint Profiles, since it can redirect a session's traffic same as any other injected var), so in multi-user mode setting it via `envOverrides` is admin-only, and a non-granted owner's already-persisted `CLAUDE_CONFIG_DIR` is stripped on reboot-restore — silently returning that session to the default Claude account rather than the one it was pointed at (see `session-env-clamp.ts`). → [architecture-invariants#per-session-env-overrides-exact-key-allowlist-and-claude_config_dir](docs/architecture-invariants.md#per-session-env-overrides-exact-key-allowlist-and-claude_config_dir) - **Effort is NOT an env var** — never carry effort as `CLAUDE_CODE_EFFORT_LEVEL`: the env var hard-locks effort and blocks in-session `/effort` switching (incl. ultracode). It flows as the dedicated `effort` payload field → `Session._effort` → `claude --effort ` for regular levels incl. `max` (the settings `effortLevel` key is `enum(["low","medium","high","xhigh"]).catch(undefined)` — `max` gets SILENTLY dropped there), or `claude --settings '{"ultracode":true}'` for ultracode (rejected by `--effort`). Both are soft defaults the user can override anytime. Legacy env-var entries are auto-migrated by the Session constructor and unset from tmux sessions in `applyEnvOverrides()`. See `buildEffortCliArgs()` in `session-cli-builder.ts`, tests in `test/effort-injection.test.ts` - **Model choice flows via `settings.local.json`, NOT `--model` or env** — the App Settings **Claude Model** picker (`claudeModel` in `settings.json`) is read by `session-ui.js` at session create (wins over the legacy 1M-Opus toggles `opusContext1m`/`opusContext1mEnabled`), sent as the `modelOverride` payload field, and `updateCaseModel()` (`hooks-config.ts`) writes/deletes the `model` key in `/.claude/settings.local.json`. This is the intended exception to the envOverrides rule above: model legitimately lives in `settings.local.json` (a soft default — in-session `/model` still works); env vars do not - **Multi-CLI prefix discipline** — env-var prefix is CLI-specific (`CLAUDE_CODE_*` vs `OPENCODE_*` vs `CODEX_*` vs `GEMINI_*` vs `ANTIGRAVITY_*` vs `PI_*` vs `GROK_*` vs `DSH_*`) and the `ALLOWED_ENV_PREFIXES` allowlist in `schemas.ts` enforces this; non-prefix exceptions are exact keys in `ALLOWED_ENV_KEYS` (currently only `CLAUDE_CONFIG_DIR`), never a widened prefix. Gemini additionally allowlists the **broad `GOOGLE_*`** namespace (intentional: Vertex AI auth needs `GOOGLE_CLOUD_PROJECT`/`GOOGLE_APPLICATION_CREDENTIALS`/`GOOGLE_GENAI_USE_VERTEXAI`; it is the loosest allowlist entry, affecting only the user's own spawned CLI), and Grok allowlists **`XAI_*`** for the same vendor-namespace reason (`XAI_API_KEY` is grok's documented auth var). When adding a setting, decide which CLI(s) it applies to and gate the env export accordingly. Never blanket-forward all prefixes. ⚠️ Pi is the case that proves the rule: its ~34 provider keys (`ANTHROPIC_API_KEY`, `OPENAI_API_KEY`, `HF_TOKEN`, …) share NO prefix, and the allowlist is one GLOBAL list applied by a refine with no mode context, so admitting them for pi would widen it for every mode at once — they stay out, and pi users authenticate via `/login` or the server process's own env. ⚠️ DeepSeek repeats pi's lesson exactly: a dsh `settings.yaml` can nominate ANY env var as a provider credential (`apiKeyEnv`), so only the vendor namespaces `DSH_*` (launcher inputs incl. `DSH_PERMISSION_MODE`) and `DEEPSEEK_*` (`DEEPSEEK_API_KEY`/`DEEPSEEK_BASE_URL`) are admitted; foreign provider keys authenticate from dsh's own files or the server env. Resolver design pattern: `docs/opencode-integration.md`, `docs/pi-integration.md`, `docs/grok-integration.md`, `docs/deepseek-integration.md` diff --git a/src/config/cli-registry/stock.ts b/src/config/cli-registry/stock.ts index 4a357eda..1e455192 100644 --- a/src/config/cli-registry/stock.ts +++ b/src/config/cli-registry/stock.ts @@ -228,9 +228,11 @@ const CLAUDE: CliEntry = { privilegedParams: [], // ANTHROPIC_* is NOT in allowedPrefixes/allowedKeys above (deliberately — see the // allowedPrefixes comment nearby), so these are unreachable via plain envOverrides - // today; listed here only so the dedicated custom-model route (docs/custom-model-endpoints-plan.md - // chunk 5) clamps them for a non-granted multi-user owner the same way every other - // CLI's injection vars are clamped, the day that route widens who can set them. + // today. privilegedEnvKeys has exactly one consumer, ownerClampedEnvKeys() in + // session-env-clamp.ts, which feeds the generic envOverrides clamp on + // POST /api/sessions, POST /api/quick-start and reboot-restore — no custom-model + // route reads this field at all, and the values it injects are merged in AFTER + // that clamp runs regardless of what's listed here. privilegedEnvKeys: [ 'ANTHROPIC_BASE_URL', 'ANTHROPIC_API_KEY', @@ -239,8 +241,16 @@ const CLAUDE: CliEntry = { 'ANTHROPIC_DEFAULT_OPUS_MODEL', // CLAUDE_CODE_MAX_CONTEXT_TOKENS already matches the CLAUDE_CODE_* allowedPrefix, and // CLAUDE_CONFIG_DIR is already an allowed exact key (docs/wiki/Agent-CLIs.md), so both - // were already reachable via plain envOverrides before this pair existed — listed here - // only so the custom-model route clamps them the same way as every other injected var. + // were already reachable via plain envOverrides before this pair existed and this + // feature does not strictly need either listed. They stay listed anyway, because + // types.ts's rule ("every traffic-redirecting var this feature introduces MUST also + // appear in privilegedEnvKeys") is meant to hold literally, not with an exception + // carved out for the two vars that happen not to need it today. The real + // consequence lands on the GENERIC envOverrides clamp above, not on this feature: + // a non-granted multi-user owner can no longer set CLAUDE_CONFIG_DIR through + // envOverrides at all (the per-client-account override, #255), and a PERSISTED one + // is now stripped on reboot-restore for such an owner too — see + // session-env-clamp.ts's own fileoverview. 'CLAUDE_CODE_MAX_CONTEXT_TOKENS', 'CLAUDE_CONFIG_DIR', ], diff --git a/src/session-env-clamp.ts b/src/session-env-clamp.ts index b9abf3a6..788020a3 100644 --- a/src/session-env-clamp.ts +++ b/src/session-env-clamp.ts @@ -6,13 +6,18 @@ * stripped before the session is built. The create and resume routes are what * this bites on: they clamp what a request asked for. * - * The reboot-restore route calls it as defence in depth, and today it can strip - * nothing. `Session.getEnvOverridesForPersist()` keeps only `CLAUDE_CODE_*` and - * `CLAUDE_CONFIG_DIR` out of a session's overrides, claude's `privilegedEnvKeys` - * are the five `ANTHROPIC_*` names, and that pass admits claude alone — so a - * persisted record cannot carry a clamped key. The call is there for the day the - * persisted set widens. The grant re-resolution that does bite on that path is - * `resolveClaudeModeForUsername`, which recomputes the permission mode. + * The reboot-restore route calls it as defence in depth, and it CAN strip + * something today: `Session.getEnvOverridesForPersist()` keeps only + * `CLAUDE_CODE_*` and `CLAUDE_CONFIG_DIR` out of a session's overrides, and + * claude's `privilegedEnvKeys` now includes both `CLAUDE_CODE_MAX_CONTEXT_TOKENS` + * and `CLAUDE_CONFIG_DIR` (Custom Model Endpoint Profiles, since both can + * redirect a claude session's traffic — see stock.ts's own comment on why they + * are listed despite not needing the clamp for that feature). So a non-granted + * owner's persisted `CLAUDE_CONFIG_DIR` (the per-client-account override, #255) + * is now stripped on reboot-restore, silently returning that session to the + * default Claude account rather than the account it was pointed at. The grant + * re-resolution that ALSO bites on that path is `resolveClaudeModeForUsername`, + * which recomputes the permission mode. * * This lives outside `web/routes` on purpose. The question it answers is about * session privilege rather than about HTTP, and `cron/cron-service.ts` sets the diff --git a/src/web/routes/custom-model-routes.ts b/src/web/routes/custom-model-routes.ts index 2e9ee1cb..8c49b046 100644 --- a/src/web/routes/custom-model-routes.ts +++ b/src/web/routes/custom-model-routes.ts @@ -790,7 +790,15 @@ export function registerCustomModelRoutes(app: FastifyInstance): void { // can equally ask what it currently has loaded, before or while that apply is pending. app.get( '/api/model-endpoints/:id/running-status', - async (req): Promise> => { + async ( + req + ): Promise< + ApiResponse<{ + isLlamaSwap: boolean; + running: Array>; + logLine?: string; + }> + > => { const { id } = req.params as { id: string }; const hosts = await readCustomModelHosts(CODEMAN_CONFIG_DIR); const host = hosts.find((item) => item.id === id); @@ -802,7 +810,12 @@ export function registerCustomModelRoutes(app: FastifyInstance): void { // Only worth tailing /logs once llama-swap is actually confirmed — a plain // llama.cpp/OpenAI-compatible server has no such endpoint at all. const logLine = status.isLlamaSwap ? getLatestLlamaSwapLogLine(host) : undefined; - return { success: true, data: { ...status, logLine } }; + // `cmd` (the literal llama-server launch line, which can carry model paths and + // --api-key) exists only so parseCtxFromCmd() can read it server-side during + // discovery — this un-gated, polled-every-second route has no reason to hand it + // to the browser, which only ever reads `model`/`state`. + const running = status.running.map(({ model, state }) => ({ model, state })); + return { success: true, data: { isLlamaSwap: status.isLlamaSwap, running, logLine } }; } ); } diff --git a/test/routes/custom-model-routes.test.ts b/test/routes/custom-model-routes.test.ts index 16b7011f..80d5a5de 100644 --- a/test/routes/custom-model-routes.test.ts +++ b/test/routes/custom-model-routes.test.ts @@ -181,6 +181,34 @@ describe('custom model endpoint CRUD', () => { expect(res.json().error).toMatch(/refused.*169\.254\.169\.254/); }); + it('running-status never hands the browser the raw llama-swap launch command (cmd)', async () => { + const { app } = await setup(); + await app.inject({ + method: 'POST', + url: '/api/model-endpoints', + payload: { id: 'ep-running', label: 'A', baseUrl: 'http://localhost:8080', apiKey: 'k' }, + }); + fetchMock.mockImplementation(async (url: URL) => { + if (url.pathname === '/running') { + return new Response( + JSON.stringify({ + running: [ + { model: 'qwen3', state: 'ready', cmd: 'llama-server -m /models/qwen3.gguf --api-key sk-secret' }, + ], + }), + { status: 200 } + ); + } + throw new Error(`unexpected request in this test: ${url.href}`); + }); + + const res = await app.inject({ method: 'GET', url: '/api/model-endpoints/ep-running/running-status' }); + const body = res.json(); + expect(body.data.running).toEqual([{ model: 'qwen3', state: 'ready' }]); + expect(JSON.stringify(body)).not.toContain('sk-secret'); + expect(JSON.stringify(body)).not.toContain('cmd'); + }); + it('refuses a baseUrl with embedded credentials or a non-http scheme at save time', async () => { const { app } = await setup(); for (const baseUrl of ['http://user:pw@host:8080', 'ftp://host/models', 'http://169.254.169.254']) { diff --git a/test/routes/session-custom-model.test.ts b/test/routes/session-custom-model.test.ts index 1a918e78..32cbc7e7 100644 --- a/test/routes/session-custom-model.test.ts +++ b/test/routes/session-custom-model.test.ts @@ -4,7 +4,7 @@ * Port: N/A (app.inject, no real port needed) */ import { describe, it, expect, beforeEach, vi } from 'vitest'; -import { registerSessionRoutes } from '../../src/web/routes/session-routes.js'; +import { registerSessionRoutes, _clampEnvOverridesForOwner } from '../../src/web/routes/session-routes.js'; import { createRouteTestHarness } from './_route-test-utils.js'; import { createMockSession } from '../mocks/index.js'; import { getDataDir } from '../../src/config/instance.js'; @@ -620,3 +620,39 @@ describe('POST /api/sessions/:id/custom-model', () => { expect(session.setCustomModel).not.toHaveBeenCalled(); }); }); + +describe('Claude multi-user clamp: the env-var half', () => { + // CLAUDE_CODE_MAX_CONTEXT_TOKENS and CLAUDE_CONFIG_DIR were already reachable via + // plain envOverrides before claude's privilegedEnvKeys existed (the first already + // matches the CLAUDE_CODE_* allowedPrefix, the second is an allowed exact key), so + // listing them here is not what makes this route safe — no custom-model route reads + // privilegedEnvKeys at all. What it DOES do: ownerClampedEnvKeys() feeds the generic + // envOverrides clamp on create/quick-start/reboot-restore, so a non-granted owner can + // no longer set CLAUDE_CONFIG_DIR that way (the per-client-account feature, #255), and + // a PERSISTED one is now stripped on reboot-restore for such an owner too — see + // session-env-clamp.ts's own fileoverview for why that pass used to be a no-op for + // claude specifically. + 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 CLAUDE_CONFIG_DIR and CLAUDE_CODE_MAX_CONTEXT_TOKENS for a non-granted owner, leaving unrelated CLAUDE_CODE_* keys alone', async () => { + const out = await _clampEnvOverridesForOwner('nobody', { + CLAUDE_CONFIG_DIR: '/home/attacker/fake-claude-config', + CLAUDE_CODE_MAX_CONTEXT_TOKENS: '999999', + CLAUDE_CODE_EXPERIMENTAL_AGENT_TEAMS: '1', + }); + expect(out).toEqual({ CLAUDE_CODE_EXPERIMENTAL_AGENT_TEAMS: '1' }); + }); + + it('is a no-op in single-user mode', async () => { + delete process.env.CODEMAN_MULTIUSER; + const input = { CLAUDE_CONFIG_DIR: '/home/attacker/fake-claude-config' }; + expect(await _clampEnvOverridesForOwner(undefined, input)).toBe(input); + }); +});