mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-10-08 08:29:42 +02:00
fix(omp): clamp OMP_AUTH_BROKER_URL/TOKEN, correct the env-allowlist docs
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.
This commit is contained in:
+23
-5
@@ -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
|
**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
|
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
|
bypass-permissions equivalent to wire up, so `buildOmpCommand()` only ever passes
|
||||||
to gate for `omp` (no branch needed, no privileged flag exists to strip).
|
`--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
|
Env overrides: the `OMP_*` prefix is allowlisted, and per omp's own
|
||||||
namespace of its own (its provider credentials live in `~/.omp` config files, not
|
`docs/environment-variables.md` it is not the narrow surface it looks like. omp reads
|
||||||
environment variables), so nothing beyond `OMP_*` is admitted.
|
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`
|
## Exact-id pinning: why `--resume`, not just `--continue`
|
||||||
|
|
||||||
|
|||||||
@@ -50,7 +50,13 @@ export function mangleOmpWorkingDir(workingDir: string): string {
|
|||||||
return relative.replace(/\//g, '-');
|
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 {
|
function resolveOmpHome(): string {
|
||||||
return join(homedir(), '.omp');
|
return join(homedir(), '.omp');
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -399,10 +399,10 @@ export const _clampExternalCliBypassForOwner = clampExternalCliBypassForOwner;
|
|||||||
|
|
||||||
/**
|
/**
|
||||||
* Env-var keys a non-granted owner must not be able to set, because each one
|
* 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,
|
* hands back privilege the config clamp above just removed, or redirects a
|
||||||
* redirects a credential the server injects.
|
* 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
|
* allowlisted `envOverrides` prefixes (schemas.ts) — which they have to be, since
|
||||||
* that is also how a user configures the harness's non-privileged knobs.
|
* 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
|
* 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
|
* of their choosing. (`DEEPSEEK_API_KEY` itself stays overridable: supplying
|
||||||
* your OWN key removes privilege rather than granting it.)
|
* 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.
|
* Env-var half of the multi-user bypass clamp.
|
||||||
|
|||||||
+32
-1
@@ -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 { CreateSessionSchema, QuickStartSchema } from '../src/web/schemas.js';
|
||||||
import { buildSpawnCommand } from '../src/tmux-manager.js';
|
import { buildSpawnCommand } from '../src/tmux-manager.js';
|
||||||
import { defaultDockerCommandForMode } from '../src/docker-hosts.js';
|
import { defaultDockerCommandForMode } from '../src/docker-hosts.js';
|
||||||
import { defaultRemoteCommandForMode } from '../src/remote-hosts.js';
|
import { defaultRemoteCommandForMode } from '../src/remote-hosts.js';
|
||||||
import { isExternalCliMode, isAltScreenStripMode } from '../src/session.js';
|
import { isExternalCliMode, isAltScreenStripMode } from '../src/session.js';
|
||||||
|
import { _clampEnvOverridesForOwner } from '../src/web/routes/session-routes.js';
|
||||||
|
|
||||||
describe('OMP mode schemas', () => {
|
describe('OMP mode schemas', () => {
|
||||||
it('accepts OMP session creation config', () => {
|
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\'');
|
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);
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|||||||
Reference in New Issue
Block a user