mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-10-05 23:19:43 +02:00
Fifteen findings from two independent reviews of #442, three of them blocking. Every one is addressed here. The three blockers all sat in the restore route. A rebuild that threw after addSession left a registered session with no pane behind it, visible on the board, holding a layout slot and written to state.json, with its plan entry already spent; the catch now cleans the session up and puts the entry back. The loop checked neither the global nor the per-user session cap, so one click could take a board past a documented limit; capacity is now re-checked per iteration, because the loop is itself creating the sessions it counts. Worst of the three, a rebuilt session carried none of the state its constructor has no parameter for and then persisted itself over the record that held it, zeroing token and cost totals and dropping the pin. The pin matters most: pruning keeps a record only while it is pinned, so discarding it handed the record to the next stale sweep. A new reapplyPersistedSessionState() on the session port restores the pin, the token totals, auto-compact, auto-clear, auto-resume, nice priority, the flicker filter and the custom-model selection, and it runs before both startInteractive and the first persist. The rest, in the order they bite a user. Every rebuild failure was reported as workspace-missing, so the banner told users their repo was gone when the agent had simply failed to start; there are now distinct reasons, and the toast names each one. The client read restored and skipped off the outer response object rather than through the uniform envelope, so every count came back zero and neither toast ever fired. A board left open across the reboot never learned an offer existed, because the banner was seeded only on the page-load path; it now re-reads on every SSE init. The workspace check was existence-only, skipping the multi-user confinement that the create route applies, so a withdrawn grant would not be noticed. The banner had no phone breakpoint while its text was nowrap and its buttons could not shrink. Smaller: a missing workspace is now re-offered rather than dropped, while an already-open conversation is dropped rather than re-offered forever; a throw anywhere in the route returns the unspent entries instead of discarding the plan; the single flight is keyed by owner, since take() already stops two callers receiving one entry; the env clamp's header no longer claims a protection it cannot provide on this path today, and names the check that does bite; the three endpoints are documented in docs/api-reference.md; and the module header now says that os.uptime() reads the host's clock, so the feature is effectively off inside a container. The review also explained why the tests missed all of this: they proved the construction claim through their own copy of the construction rather than through the route, and the route tests used workspaces that did not exist, so no Session was ever built. test/routes/reboot-restore-rebuild-failure.ts mocks the Session module to drive the route's real path, and covers the cleanup, the reason reported, the re-application ordering, the broadcast and the caps. The mock route context gains the port method and the mux call the route needs. Refs #411 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
98 lines
5.3 KiB
TypeScript
98 lines
5.3 KiB
TypeScript
/**
|
|
* @fileoverview The env-var half of the multi-user privilege clamp.
|
|
*
|
|
* A session's `envOverrides` can hand back privilege that the per-CLI config
|
|
* clamp removed, so a non-granted owner's overrides get the privileged keys
|
|
* 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.
|
|
*
|
|
* 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
|
|
* precedent by importing `canUsernameRunPrivilegedCommands` from `user-store.ts`
|
|
* directly and re-resolving the owner's grant when a job fires. Every caller here
|
|
* re-resolves the grant at the moment it builds a session, for the same reason.
|
|
*
|
|
* @dependencies user-store (canUsernameRunPrivilegedCommands), config/cli-registry
|
|
* @consumedby web/routes/session-routes, web/routes/reboot-restore-routes
|
|
*
|
|
* @module session-env-clamp
|
|
*/
|
|
|
|
import { canUsernameRunPrivilegedCommands } from './user-store.js';
|
|
import { enabledClis } from './config/cli-registry/registry.js';
|
|
|
|
/**
|
|
* Env-var keys a non-granted owner must not be able to set, because each one
|
|
* hands back privilege `clampExternalCliBypassForOwner()` just removed, or redirects a
|
|
* credential-resolution endpoint.
|
|
*
|
|
* 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.
|
|
*
|
|
* - `DSH_PERMISSION_MODE` IS the harness's permission switch. Every other CLI's
|
|
* bypass is a command-line FLAG, reachable only through the per-CLI config the
|
|
* clamp already owns; this one is an env var, so the config clamp alone is
|
|
* half a gate.
|
|
* - `DSH_HOME` points the launcher at a profile tree, and a profile's plugin code
|
|
* executes at BOOT, before any approval row can apply. A user who can write a
|
|
* workspace can put a profile in it, so this is the wider of the two.
|
|
* - `DEEPSEEK_BASE_URL` aims the provider endpoint, and `_configureCliEnv()`
|
|
* forwards the SERVER's own `DEEPSEEK_API_KEY` into every dsh pane before
|
|
* `applyEnvOverrides()` runs — so a non-granted owner who could set the base
|
|
* 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()).
|
|
*/
|
|
export function ownerClampedEnvKeys(): string[] {
|
|
return enabledClis().flatMap((entry) => entry.capabilities.privilegedEnvKeys);
|
|
}
|
|
|
|
/**
|
|
* Env-var half of the multi-user bypass clamp.
|
|
*
|
|
* `clampExternalCliBypassForOwner()` in `web/routes/session-routes.ts` clamps the
|
|
* per-CLI CONFIG, and for every CLI
|
|
* but DeepSeek that is the whole story. Here it is not: `applyEnvOverrides()` runs
|
|
* AFTER `_configureCliEnv()` in tmux-manager, so an override sent on the SAME
|
|
* request lands last and wins, and a non-granted owner could restore
|
|
* `danger-full-access` on the very request the config clamp downgraded.
|
|
*
|
|
* Keys are DROPPED rather than rewritten: dropping falls through to what
|
|
* `_configureCliEnv()` exports, which is the clamped config and the server's own
|
|
* `DSH_HOME`, i.e. exactly the intended state. No-op in single-user mode and for a
|
|
* granted owner, like every other clamp here
|
|
* (`canUsernameRunPrivilegedCommands()` returns true when `!isMultiUserMode()`),
|
|
* and it returns the caller's own object untouched when there is nothing to strip.
|
|
*/
|
|
export async function clampEnvOverridesForOwner(
|
|
owner: string | undefined,
|
|
envOverrides: Record<string, string> | undefined
|
|
): Promise<Record<string, string> | undefined> {
|
|
if (!envOverrides) return envOverrides;
|
|
const keys = ownerClampedEnvKeys();
|
|
if (!keys.some((key) => key in envOverrides)) return envOverrides;
|
|
if (await canUsernameRunPrivilegedCommands(owner)) return envOverrides;
|
|
const clamped = { ...envOverrides };
|
|
for (const key of keys) delete clamped[key];
|
|
return clamped;
|
|
}
|