diff --git a/src/databases/officer_db/src/api-keys/index.ts b/src/databases/officer_db/src/api-keys/index.ts index a0569aae..053d2d86 100644 --- a/src/databases/officer_db/src/api-keys/index.ts +++ b/src/databases/officer_db/src/api-keys/index.ts @@ -3,6 +3,7 @@ export { createApiKey, listApiKeys, revokeApiKey, + revokeApiKeysByName, deleteApiKey, touchApiKey, type ApiKeyIdentity, diff --git a/src/databases/officer_db/src/api-keys/queries.ts b/src/databases/officer_db/src/api-keys/queries.ts index ebe2d31b..49de7dd7 100644 --- a/src/databases/officer_db/src/api-keys/queries.ts +++ b/src/databases/officer_db/src/api-keys/queries.ts @@ -14,6 +14,8 @@ export type ApiKeyIdentity = { email: string; username: string | null; lastUsedAt: Date | null; + /** Permission keys this credential may reach, or null for the holder's full authority. */ + permissions: string[] | null; }; /** @@ -31,6 +33,7 @@ export async function findLiveApiKeyByHash(keyHash: string): Promise { @@ -62,6 +67,7 @@ export async function createApiKey(params: CreateApiKeyParams): Promise`, so deleting that commander revokes exactly its own and + * recreating one under a reused directory name cannot leave a live key nothing points at. + * + * Revokes rather than deletes, the same as the by-id path: the row is what the audit reads afterwards. + */ +export async function revokeApiKeysByName(userId: number, name: string): Promise { + const rows = await db + .update(apiKeys) + .set({ revokedAt: new Date() }) + .where(and(eq(apiKeys.userId, userId), eq(apiKeys.name, name), isNull(apiKeys.revokedAt))) + .returning({ id: apiKeys.id }); + return rows.length; +} + /** * Revoke one of the caller's own keys. Scoped by userId in the same statement as the id, so a caller * naming someone else's key id revokes nothing and is told the same "not found" as for an id that never diff --git a/src/databases/officer_db/src/api-keys/schema.ts b/src/databases/officer_db/src/api-keys/schema.ts index 5328643c..656b0e00 100644 --- a/src/databases/officer_db/src/api-keys/schema.ts +++ b/src/databases/officer_db/src/api-keys/schema.ts @@ -21,10 +21,17 @@ import { users } from '../auth/schema'; // // ── Authority ── // -// A key currently carries its owner's full authority — deliberately, as the first cut. That is not a -// hole: it is exactly what the password that made it could already do, so a key is never an escalation. -// A `scopes` column is the intended next step, and the resolver is written so adding one touches the -// resolver and this table, nothing else. +// A key carries its owner's authority, NARROWED by `permissions` when that column is set. Null means +// unnarrowed — what every key minted by a human still is, and what the mobile apps hold: exactly what the +// password that made it could already do, so a key is never an escalation. +// +// A list means the key may reach only those permission keys, and still only where the account itself +// could. Intersection, never union — see `permissions/authorize.ts` → `decideApiRequest`. +// +// **The owner is the reason this exists.** A Super Admin bypasses every role check, so a credential +// carrying the owner's identity is unbounded by construction, and a commander is an agent holding one +// unattended for months. The narrowing is applied BEFORE the owner bypass, which makes it the only thing +// in the platform that can bound them. export const apiKeys = pgTable( 'api_keys', { @@ -38,6 +45,13 @@ export const apiKeys = pgTable( keyHash: text('key_hash').notNull(), /** First characters of the secret, in the clear, for display only. */ prefix: text('prefix').notNull(), + /** + * Permission keys this credential may reach, or null for the holder's full authority. + * + * An empty array is NOT the same as null and is not a mistake to guard against: it is a key that can + * reach nothing, which is a coherent thing to mint and a safe thing to default to. + */ + permissions: text('permissions').array(), /** * Updated on use, but at most once a minute — see touchApiKey. Without the debounce every * authenticated request would carry a write, which is a lot of churn to answer "is this still in use". diff --git a/src/server.tsx b/src/server.tsx index 62ac2844..9ebf4a69 100644 --- a/src/server.tsx +++ b/src/server.tsx @@ -391,7 +391,7 @@ async function upgradeWs(req: Request, server: any, provider: UpgradableProvider // whole point: the two doors cannot disagree about what a role holds, because there is only one // declaration to read. `terminal`, `chat`, `task-runner`, `pipeline` and `desktop` are refused here // by being `execution` permissions, not by being absent from an array someone has to maintain. - if (!(await isWsProviderAllowed(user.id, provider))) { + if (!(await isWsProviderAllowed(user.id, provider, user.permissions ?? null))) { return new Response('Forbidden', { status: 403 }); } diff --git a/src/servers/_middlewares/permission-gate.ts b/src/servers/_middlewares/permission-gate.ts index 74db20fa..a8a0f433 100644 --- a/src/servers/_middlewares/permission-gate.ts +++ b/src/servers/_middlewares/permission-gate.ts @@ -44,8 +44,17 @@ export const permissionGateMiddleware: MiddlewareHandler = async function (ctx, // // Non-/api paths are not ours: the DAV sync door authenticates with its own app password and carries no // platform account, so there is nothing here to resolve. - if (payload && !isOwner && path.startsWith('/api') && !isExemptApiPath(path) && !isPluginPublicPath(path)) { - const { allowed, reason } = await isApiRequestAllowed(payload.id, ctx.req.method, path); + // A NARROWED credential is checked even for the owner — that narrowing is the only bound a Super Admin + // has, so skipping the check for them would make it decorative on the one account it exists for. An + // unnarrowed owner still short-circuits, which keeps the common request free of a database read. + const credential = payload?.permissions ?? null; + // Stashed for the routes that pass authority on rather than just using it. Set unconditionally, so a + // reader never has to distinguish "unnarrowed" from "the gate did not run". + ctx.set('credentialPermissions', credential); + const mustCheck = payload && (!isOwner || credential); + + if (mustCheck && path.startsWith('/api') && !isExemptApiPath(path) && !isPluginPublicPath(path)) { + const { allowed, reason } = await isApiRequestAllowed(payload.id, ctx.req.method, path, credential); if (!allowed) throw errors.FORBIDDEN(reason ?? 'Not permitted for this account'); } diff --git a/src/servers/api/command-hq/credential.ts b/src/servers/api/command-hq/credential.ts new file mode 100644 index 00000000..923dc1a7 --- /dev/null +++ b/src/servers/api/command-hq/credential.ts @@ -0,0 +1,98 @@ +import { mkdirSync, writeFileSync } from 'node:fs'; +import { join } from 'node:path'; +import { createApiKey, revokeApiKeysByName } from 'officerdb'; +import { mintApiKey } from '@@/auth-token'; +import { PERMISSION_BY_KEY } from '@@/permissions/registry'; +import { PORT, OFFICER_API_URL } from '../../officer-url.mjs'; +import { getCommanderDir } from '../../data-path'; + +/* + A commander's own credential. + + ── Why it has one at all ── + + A commander is an agent working while nobody watches, and it has to reach the platform to do anything + worth doing. Until now it could not: the only credential in the tree sits in the MCP tool server's + environment, which the agent's own shell cannot see. So the first commander built here did the + reasonable thing from where it was standing and curled the whisper container directly on localhost — + correct answer, wrong door, and a door the platform does not control. + + ── Why it is narrowed ── + + The owner bypasses every permission check, so a credential carrying the owner's identity is unbounded. + That is fine for a browser session somebody is sitting in front of and wrong for a process that runs + for months unattended. `api_keys.permissions` is the only thing that can bound a Super Admin — see + `permissions/authorize.ts` → `decideApiRequest`, which applies it BEFORE the owner short-circuit. + + ── Why the default is "everything except admin" ── + + A commander that can reach nothing is useless on the day it is created, and a default nobody can work + with gets widened in a hurry rather than thought about. So the default removes the class that is never + part of a watcher's job — installing plugins, managing users and roles, changing server settings — and + leaves the rest. + + It is a DEFAULT, not a policy: the list is stored on the key row and can be narrowed per commander. + And for a member it narrows itself, because the check is an intersection with what their role holds. +*/ + +/** Permission keys a commander gets unless told otherwise: everything the platform does that is not administration. */ +export function defaultCommanderPermissions(): string[] { + return [...PERMISSION_BY_KEY.values()].filter((p) => p.kind !== 'admin').map((p) => p.key); +} + +/** The env file's path, and the name the key is stored under — both derived, so revocation cannot miss one. */ +export const commanderKeyName = (dirName: string) => `commander:${dirName}`; +export const commanderEnvPath = (email: string, dirName: string) => + join(getCommanderDir(email, dirName), 'officer.env'); + +/** + * Mint a commander's key and write it into its directory. + * + * The secret is written to a FILE rather than injected into the agent's environment because the agent is + * not the only thing that needs it: a commander arms a detached loop, and that loop is spawned by the + * agent itself and inherits nothing the platform chose. A file both can read is the only shape that + * covers the interactive turn and the unattended loop with one credential. + * + * 0600, and `createCommanderDirectory` chowns the whole directory to the account afterwards — which is + * why this must run BEFORE that chown, not after. + */ +export async function writeCommanderCredential(params: { + userId: number; + email: string; + dirName: string; + permissions?: string[]; +}): Promise<{ path: string; permissions: string[] }> { + const permissions = params.permissions ?? defaultCommanderPermissions(); + const { secret, keyHash, prefix } = mintApiKey(); + + // Revoke first. A commander recreated under a directory name that has been used before would otherwise + // leave a live key nothing points at any more. + await revokeApiKeysByName(params.userId, commanderKeyName(params.dirName)).catch(() => {}); + await createApiKey({ + userId: params.userId, + name: commanderKeyName(params.dirName), + keyHash, + prefix, + permissions, + }); + + const path = commanderEnvPath(params.email, params.dirName); + mkdirSync(join(path, '..'), { recursive: true }); + writeFileSync( + path, + [ + "# This commander's own credential. Source it before calling Officer:", + '# set -a; . ./officer.env; set +a', + '#', + '# It is narrowed — see `permissions` on its api_keys row — and it is revoked when this commander is', + '# deleted. Do not copy it anywhere else; anything that needs one gets its own.', + `OFFICER_API_URL=${OFFICER_API_URL}`, + `OFFICER_API_HOST=${process.env.PUBLIC_URL ? new URL(process.env.PUBLIC_URL).host : `127.0.0.1:${PORT}`}`, + `OFFICER_API_KEY=${secret}`, + '', + ].join('\n'), + { mode: 0o600 }, + ); + + return { path, permissions }; +} diff --git a/src/servers/api/command-hq/directory.ts b/src/servers/api/command-hq/directory.ts index 4038ed36..c8731538 100644 --- a/src/servers/api/command-hq/directory.ts +++ b/src/servers/api/command-hq/directory.ts @@ -4,6 +4,7 @@ import { lookupOsUser } from '@@/os-user'; import { join } from 'node:path'; import { getCommanderDir, getCommanderStateDir } from '../../data-path'; import { loopPidPath, spawnedRecordPath } from './spawned'; +import { writeCommanderCredential } from './credential'; /* A commander's own directory, created once, up front — `docs/commander-directory.md`. @@ -22,6 +23,8 @@ export type CommanderClaudeMdParams = { referencePath: string; loopPidPath: string; spawnedPath: string; + /** Absolute path to this commander's `officer.env`, or null when minting it failed. */ + credentialPath: string | null; }; /** @@ -64,6 +67,37 @@ export function commanderClaudeMd(params: CommanderClaudeMdParams): string { 'The platform cannot find this process and cannot recover a spawned agent id after it exits, so this', 'record is the only one. Without it, deleting this commander leaves its loop running.', '', + '## Reaching the platform', + '', + ...(params.credentialPath + ? [ + 'This commander has its own credential, and it is the ONLY way it should reach anything:', + '', + '```', + `set -a; . ${params.credentialPath}; set +a`, + 'curl -H "Authorization: Bearer $OFFICER_API_KEY" -H "Host: $OFFICER_API_HOST" \\', + ' "$OFFICER_API_URL/api/..."', + '```', + '', + 'Use the platform, never the thing behind it. A sidecar — whisper, a media server, anything a', + 'plugin wraps — usually also listens on localhost with no authentication, so calling it directly', + 'works and looks correct. It is still wrong: it skips every permission, every setting the owner', + 'chose and every log, and it breaks silently the day that service moves.', + '', + 'Prefer a TASK over a raw call where one exists. `GET /api/tasks` lists them and', + '`POST /api/tasks//run` runs one and returns what it did — one reviewed definition on', + 'disk instead of a re-implementation living in a loop nobody reads.', + '', + 'The credential is narrowed and revoked when this commander is deleted. A 403 is an answer, not a', + 'problem to route around: it means this commander was not given that, and the owner is the one who', + 'decides otherwise.', + '', + ] + : [ + 'This commander has NO credential — minting one failed at creation. It cannot reach the platform', + 'API, and it must not work around that by calling a sidecar directly. Tell the owner.', + '', + ]), '## Before arming anything', '', 'Never arm a watcher on the first turn. The first turn is for settling what to watch and what to do', @@ -119,11 +153,24 @@ async function giveToAccount(email: string, dir: string): Promise export async function createCommanderDirectory( email: string, dirName: string, - params: { name: string; referencePath: string }, -): Promise<{ dir: string; ownershipError: string | null }> { + params: { name: string; referencePath: string; userId: number }, +): Promise<{ dir: string; ownershipError: string | null; credentialError: string | null }> { const dir = getCommanderDir(email, dirName); mkdirSync(dir, { recursive: true }); mkdirSync(getCommanderStateDir(email, dirName), { recursive: true }); + + // Before CLAUDE.md, because CLAUDE.md tells the commander where its credential is and has to say + // truthfully whether there is one. Best effort for the same reason the chown is: a commander with no + // credential is degraded, and refusing to create it would be the worse trade — but it must be TOLD, + // or it will find its own way to the machine, which is how this started. + let credentialPath: string | null = null; + let credentialError: string | null = null; + try { + ({ path: credentialPath } = await writeCommanderCredential({ userId: params.userId, email, dirName })); + } catch (err) { + credentialError = err instanceof Error ? err.message : 'could not mint a credential'; + } + writeFileSync( join(dir, 'CLAUDE.md'), commanderClaudeMd({ @@ -131,10 +178,11 @@ export async function createCommanderDirectory( referencePath: params.referencePath, loopPidPath: loopPidPath(email, dirName), spawnedPath: spawnedRecordPath(email, dirName), + credentialPath, }), ); - // Last, so it covers CLAUDE.md and state/ as well as the directory itself. + // Last, so it covers CLAUDE.md, officer.env and state/ as well as the directory itself. const ownershipError = await giveToAccount(email, dir); - return { dir, ownershipError }; + return { dir, ownershipError, credentialError }; } diff --git a/src/servers/api/command-hq/router.ts b/src/servers/api/command-hq/router.ts index 76bb2ff4..41441c90 100644 --- a/src/servers/api/command-hq/router.ts +++ b/src/servers/api/command-hq/router.ts @@ -6,12 +6,19 @@ import { checkBudget } from '../sub-agents/budget'; import * as errors from '@@/custom-errors'; import { resolveHomeDir } from '@@/user-home'; import { toShellUsername, getCommanderDir } from '@@/data-path'; -import { forgetChatSessions, getChatEventsSince, ingestSpawnedChildren, sessionKeyForTranscriptId } from 'officerdb'; +import { + forgetChatSessions, + getChatEventsSince, + ingestSpawnedChildren, + revokeApiKeysByName, + sessionKeyForTranscriptId, +} from 'officerdb'; import * as sidecar from '@@/sidecar-registry'; import { findSessionKeyByClaudeSession } from '@@/sidecar/claude/state'; import { sessionKeyForTranscript } from '../chat/websocket'; import { listSpawnedSessions, loopPidPath, spawnedRecordPath, stopCommanderProcesses, liveLoopPid } from './spawned'; import { createCommanderDirectory } from './directory'; +import { commanderKeyName } from './credential'; import type { ServerMessage } from '../chat/types'; import { loadClaudeSessionById, @@ -352,7 +359,18 @@ commandHqRouter.post('/commanders/new', async (ctx) => { // Row and directory first — see `POST /commanders` below for the same two calls with the full reasoning. const row = await createCommander({ userId: user.id, name, referencePath, categoryId: body.categoryId ?? null }); - const dir = createCommanderDirectory(who.email, row.dirName, { name, referencePath }); + // AWAITED. It was not, and that is the ordering this whole redesign exists to get right: Claude Code + // reads CLAUDE.md from the working directory at session start, so a turn spawned before the file is + // written is a commander that never learns what it is. The un-awaited call also made the `cwd` returned + // below a Promise, which serialised as `{}`. + const { dir, ownershipError, credentialError } = await createCommanderDirectory(who.email, row.dirName, { + name, + referencePath, + userId: user.id, + }); + if (ownershipError) + console.warn(`[command-hq] ${row.dirName}: directory not handed to the account — ${ownershipError}`); + if (credentialError) console.warn(`[command-hq] ${row.dirName}: no credential minted — ${credentialError}`); const sessionId = randomUUID(); const model = (await getUserDefaultModel(user.id)) || DEFAULT_MODEL; @@ -506,7 +524,12 @@ commandHqRouter.post('/commanders', async (ctx) => { if (!referencePath) throw errors.BAD_REQUEST('referencePath is required'); const row = await createCommander({ userId: user.id, name, referencePath, categoryId: body.categoryId ?? null }); - const { dir, ownershipError } = await createCommanderDirectory(who.email, row.dirName, { name, referencePath }); + const { dir, ownershipError, credentialError } = await createCommanderDirectory(who.email, row.dirName, { + name, + referencePath, + userId: user.id, + }); + if (credentialError) console.warn(`[command-hq] ${row.dirName}: no credential minted — ${credentialError}`); if (ownershipError) { console.warn(`[command-hq] ${row.dirName}: directory not handed to the account — ${ownershipError}`); } @@ -674,6 +697,17 @@ commandHqRouter.delete('/commanders/:sessionId', async (ctx) => { // Already gone, or never fully created. The row is still deleted below regardless. } + /* + And its credential, which the directory removal does NOT cover — the secret file goes with the + directory, but the row that makes it work lives in Postgres. A commander whose files are gone and whose + key still authenticates is the worst of both: nothing on disk to find it by, and a live credential. + + Revoked rather than deleted, so what it was and when it was last used survive the commander. + */ + await revokeApiKeysByName(user.id, commanderKeyName(row.dirName)).catch((err) => { + console.warn(`[command-hq] ${row.dirName}: credential not revoked — ${err}`); + }); + // 3. The commander itself. forgotten.push(...deleteClaudeSession(who, who.home, sessionId)); await deleteCommandHqSession(user.id, sessionId); diff --git a/src/servers/api/tasks/execute-script.ts b/src/servers/api/tasks/execute-script.ts index 0b3c956a..2c014a8a 100644 --- a/src/servers/api/tasks/execute-script.ts +++ b/src/servers/api/tasks/execute-script.ts @@ -1,10 +1,9 @@ -import { join, isAbsolute } from 'node:path'; -import { mkdirSync, writeFileSync, chmodSync, rmSync, createWriteStream } from 'node:fs'; -import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { mkdirSync, createWriteStream } from 'node:fs'; import { getTaskByDirName } from './task-files'; -import { getOwnerHomeDir, DATA_PATH } from '../../data-path'; +import { DATA_PATH } from '../../data-path'; import { killTree } from './process-tree'; -import { buildTaskApiEnv } from './task-api-env'; +import { prepareScriptRun } from './script-run'; // Script-job event stream. `stdout`/`stderr` are high-frequency (live-only + persisted to the log // file); `started`/`exit`/`error` are structural and get buffered for viewer replay. @@ -33,41 +32,6 @@ export type ExecuteScriptParams = { emit: (event: ScriptEvent) => void; }; -const getRunner = (language: string): string[] => - language === 'python' - ? ['python3'] - : language === 'typescript' - ? ['bun', 'run'] - : language === 'javascript' - ? ['node'] - : ['bash']; -const getFileName = (language: string): string => - language === 'python' - ? 'run.py' - : language === 'typescript' - ? 'index.ts' - : language === 'javascript' - ? 'index.js' - : 'run.sh'; - -function materializeScript(language: string, implementation: string): string { - const dir = join(tmpdir(), `officer-task-${Date.now()}-${Math.random().toString(36).slice(2)}`); - mkdirSync(dir, { recursive: true }); - const filePath = join(dir, getFileName(language)); - writeFileSync(filePath, implementation); - chmodSync(filePath, 0o755); - return filePath; -} - -const buildInputEnv = (inputs: Record): Record => { - const env: Record = {}; - for (const [key, value] of Object.entries(inputs)) env[`INPUT_${key.toUpperCase()}`] = value; - return env; -}; - -const buildArgs = (inputs: Record, argsOrder?: string[] | null): string[] => - !argsOrder || argsOrder.length === 0 ? [] : argsOrder.map((name) => inputs[name] ?? ''); - // The persisted per-job output log — served by GET /jobs/:id/log for backfill / unattended viewing. export const jobLogPath = (jobId: string) => join(DATA_PATH, 'jobs', `${jobId}.log`); @@ -79,22 +43,16 @@ export async function executeScript(params: ExecuteScriptParams): Promise<{ exit const task = await getTaskByDirName(params.taskDirName); if (!task) throw new Error(`Task not found: ${params.taskDirName}`); - if (task.mode !== 'script') throw new Error('Task is not a script-mode task'); - if (!task.implementation) throw new Error(`Task ${params.taskDirName} has no implementation`); - const language = task.language ?? 'bash'; - const scriptPath = materializeScript(language, task.implementation); - const inputEnv = buildInputEnv(inputs); - const positionalArgs = buildArgs(inputs, task.args); - const cmd = [...getRunner(language), scriptPath, ...positionalArgs]; - - const homeDir = getOwnerHomeDir(email); - const cwd = params.cwd ? (isAbsolute(params.cwd) ? params.cwd : join(homeDir, params.cwd)) : homeDir; - - const spawnCmd = cmd; - const apiEnv = await buildTaskApiEnv({ id: userId, email, username }); - const spawnEnv = { ...(process.env as Record), ...apiEnv, ...inputEnv }; - const spawnCwd = cwd; + // What the script sees is `script-run.ts`'s to decide, for all three runners. This one owns only how + // the output is reported: streamed to viewers and appended to a durable log. + const prepared = await prepareScriptRun({ + task, + inputs, + cwd: params.cwd, + user: { id: userId, email, username }, + }); + const { cmd: spawnCmd, env: spawnEnv, cwd: spawnCwd, cleanup } = prepared; mkdirSync(join(DATA_PATH, 'jobs'), { recursive: true }); const log = createWriteStream(jobLogPath(jobId), { flags: 'w' }); @@ -115,15 +73,7 @@ export async function executeScript(params: ExecuteScriptParams): Promise<{ exit /* a log-stream error must never kill the pump */ } }; - const cleanup = () => { - try { - rmSync(join(scriptPath, '..'), { recursive: true, force: true }); - } catch { - /* best effort */ - } - }; - - emit({ type: 'started', taskName: task.name }); + emit({ type: 'started', taskName: prepared.taskName }); const proc = Bun.spawn(spawnCmd, { cwd: spawnCwd, env: spawnEnv, stdout: 'pipe', stderr: 'pipe' }); diff --git a/src/servers/api/tasks/script-run.ts b/src/servers/api/tasks/script-run.ts new file mode 100644 index 00000000..d971772c --- /dev/null +++ b/src/servers/api/tasks/script-run.ts @@ -0,0 +1,169 @@ +import { join, isAbsolute } from 'node:path'; +import { mkdirSync, writeFileSync, chmodSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { getTaskByDirName, type TaskRecord } from './task-files'; +import { getOwnerHomeDir } from '../../data-path'; +import { buildTaskApiEnv } from './task-api-env'; + +/** + * Everything a script-mode task run needs before something spawns it. + * + * ── Why this is its own file ── + * + * Three runners spawn a script task and they must agree about HOW: the ephemeral socket + * (`task-executor.ts`), the background job (`execute-script.ts`) and the direct REST call below. Each + * carried its own copy of the same twenty lines — materialize the implementation into a temp dir, build + * `INPUT_` env vars, build positional args in `task.args` order, resolve a home-relative cwd, mint + * the API env. Identical line for line, and nothing said they had to stay that way. + * + * They differ in how they REPORT — streaming to a socket, appending to a durable log, or returning a + * string — and that difference is real, so it stays with each of them. What must not differ is what the + * script sees: a task that behaves one way from the context menu and another from a commander is a bug + * nobody can reproduce, because both callers are "running the task". + */ + +const getRunner = (language: string): string[] => + language === 'python' + ? ['python3'] + : language === 'typescript' + ? ['bun', 'run'] + : language === 'javascript' + ? ['node'] + : ['bash']; + +const getFileName = (language: string): string => + language === 'python' + ? 'run.py' + : language === 'typescript' + ? 'index.ts' + : language === 'javascript' + ? 'index.js' + : 'run.sh'; + +/** + * Only `index.ts` (or its sibling by language) is written out — a script task is materialized ALONE, so + * it has to be self-contained. That is a contract, not an oversight: see `platform/CLAUDE.md`. + */ +function materializeScript(language: string, implementation: string): string { + const dir = join(tmpdir(), `officer-task-${Date.now()}-${Math.random().toString(36).slice(2)}`); + mkdirSync(dir, { recursive: true }); + const filePath = join(dir, getFileName(language)); + writeFileSync(filePath, implementation); + chmodSync(filePath, 0o755); + return filePath; +} + +export type ScriptRunUser = { + id: number; + email: string; + username?: string | null; + /** The caller's credential narrowing, propagated to the script's own token. See `task-api-env.ts`. */ + permissions?: string[] | null; +}; + +export type PreparedScriptRun = { + taskName: string; + cmd: string[]; + cwd: string; + env: Record; + /** Removes the temp directory. Every caller must call it, on every path out. */ + cleanup: () => void; +}; + +type PrepareParams = { + task: TaskRecord; + inputs: Record; + /** Home-relative or absolute. Absent means the owner's home. */ + cwd?: string; + user: ScriptRunUser; +}; + +export async function prepareScriptRun({ task, inputs, cwd, user }: PrepareParams): Promise { + if (task.mode !== 'script') throw new Error('Task is not a script-mode task'); + if (!task.implementation) throw new Error(`Task ${task.dirName} has no implementation`); + + const language = task.language ?? 'bash'; + const scriptPath = materializeScript(language, task.implementation); + + // Inputs reach the script two ways, and both are load-bearing: named, as `INPUT_`, and + // positionally in the order `task.args` declares. + const inputEnv: Record = {}; + for (const [key, value] of Object.entries(inputs)) inputEnv[`INPUT_${key.toUpperCase()}`] = value; + const positionalArgs = task.args?.length ? task.args.map((name) => inputs[name] ?? '') : []; + + // A relative cwd is relative to the OWNER's real home, not the managed one — that is where task runs + // execute. A missing cwd surfaces as ENOENT naming the binary rather than the directory, which is why + // this resolves rather than passing it through. + const homeDir = getOwnerHomeDir(user.email); + + return { + taskName: task.name, + cmd: [...getRunner(language), scriptPath, ...positionalArgs], + cwd: cwd ? (isAbsolute(cwd) ? cwd : join(homeDir, cwd)) : homeDir, + env: { + ...(process.env as Record), + ...(await buildTaskApiEnv(user)), + ...inputEnv, + }, + cleanup: () => { + try { + rmSync(join(scriptPath, '..'), { recursive: true, force: true }); + } catch { + /* best effort */ + } + }, + }; +} + +export type DirectRunResult = { code: number; output: string; taskName: string }; + +/** + * Run a script task and wait for it. No job row, no socket, no streaming. + * + * This is the shape a caller with no browser needs — a commander's loop, a cron entry, anything holding a + * credential rather than a session. The two existing runners both require something to watch them: the + * socket needs a live connection, and the job needs somebody to open Jobs to see what happened. + * + * stdout and stderr are interleaved into one string in arrival order, which is what a caller reading it + * as "what happened" wants — the script's own narration and its errors in the order it produced them. + */ +export async function runScriptTaskDirect(params: { + taskDirName: string; + inputs: Record; + cwd?: string; + user: ScriptRunUser; +}): Promise { + const task = await getTaskByDirName(params.taskDirName); + if (!task) throw new Error(`Task not found: ${params.taskDirName}`); + + const prepared = await prepareScriptRun({ task, inputs: params.inputs, cwd: params.cwd, user: params.user }); + + try { + const proc = Bun.spawn(prepared.cmd, { + cwd: prepared.cwd, + env: prepared.env, + stdout: 'pipe', + stderr: 'pipe', + }); + + let output = ''; + const decoder = new TextDecoder(); + const drain = async (stream: ReadableStream) => { + const reader = stream.getReader(); + while (true) { + const { done, value } = await reader.read(); + if (done) break; + output += decoder.decode(value); + } + }; + + // Both streams are drained CONCURRENTLY with the wait. Reading them in sequence deadlocks the moment + // a script writes more to stderr than the pipe buffer holds while nothing is reading it. + await Promise.all([drain(proc.stdout), drain(proc.stderr)]); + const code = await proc.exited; + + return { code, output, taskName: prepared.taskName }; + } finally { + prepared.cleanup(); + } +} diff --git a/src/servers/api/tasks/task-api-env.ts b/src/servers/api/tasks/task-api-env.ts index fb7f51b9..26d3f649 100644 --- a/src/servers/api/tasks/task-api-env.ts +++ b/src/servers/api/tasks/task-api-env.ts @@ -11,7 +11,21 @@ const PUBLIC_HOST = (() => { } })(); -type TaskApiUser = { id: number; email: string; username?: string | null }; +type TaskApiUser = { + id: number; + email: string; + username?: string | null; + /** + * The narrowing carried by the credential that ASKED for this run, or null/absent for a full-authority + * caller (a browser session, the context menu, a job started by hand). + * + * It has to propagate or the narrowing is decorative. A commander holding a key limited to `tasks` can + * write a task and run it; if the token handed to that script were unnarrowed, the script would come + * back through the API with the owner's full authority and the limit would have bought nothing. The + * script gets exactly what its caller had — never more. + */ + permissions?: string[] | null; +}; // Environment that lets a script task call Officer's own API instead of reimplementing server-side // work (transcription, for one — `plugins/transcribe/tasks/transcribe-audio-file/index.ts` uses exactly @@ -24,7 +38,17 @@ type TaskApiUser = { id: number; email: string; username?: string | null }; // curl -H "Authorization: Bearer $OFFICER_AUTH_TOKEN" -H "Host: $OFFICER_API_HOST" \ // "$OFFICER_API_URL/api/transcribe/transcribe" export async function buildTaskApiEnv(user: TaskApiUser): Promise> { - const token = await sign({ id: user.id, email: user.email, username: user.username ?? '' }, '12h'); + const token = await sign( + { + id: user.id, + email: user.email, + username: user.username ?? '', + // Only when narrowed. An absent claim is what every ordinary run already signs, and `resolveAuthToken` + // spreads the payload onto the identity, so this reaches both doors with no further wiring. + ...(user.permissions ? { permissions: user.permissions } : {}), + }, + '12h', + ); return { OFFICER_API_URL, OFFICER_API_HOST: PUBLIC_HOST ?? `127.0.0.1:${PORT}`, diff --git a/src/servers/api/tasks/task-executor.ts b/src/servers/api/tasks/task-executor.ts index 10658499..b69f196f 100644 --- a/src/servers/api/tasks/task-executor.ts +++ b/src/servers/api/tasks/task-executor.ts @@ -1,9 +1,7 @@ import type { ServerWebSocket } from 'bun'; -import { join, isAbsolute } from 'node:path'; -import { mkdirSync, writeFileSync, chmodSync, rmSync, readdirSync, readFileSync } from 'node:fs'; +import { readdirSync, readFileSync } from 'node:fs'; import { getTaskByDirName } from './task-files'; -import { getOwnerHomeDir } from '../../data-path'; -import { buildTaskApiEnv } from './task-api-env'; +import { prepareScriptRun } from './script-run'; type WSData = { userId: number; @@ -94,123 +92,38 @@ function killTree(root: number) { }, 2000); } -import { tmpdir } from 'node:os'; - function send(ws: ServerWebSocket, msg: OutMessage) { if (ws.readyState === 1) ws.send(JSON.stringify(msg)); } -function getRunner(language: string): string[] { - switch (language) { - case 'bash': - return ['bash']; - case 'python': - return ['python3']; - case 'typescript': - return ['bun', 'run']; - case 'javascript': - return ['node']; - default: - return ['bash']; - } -} - -function getFileName(language: string): string { - switch (language) { - case 'bash': - return 'run.sh'; - case 'python': - return 'run.py'; - case 'typescript': - return 'index.ts'; - case 'javascript': - return 'index.js'; - default: - return 'run.sh'; - } -} - -// Write implementation to a temp file for execution, cleaned up after -function materializeScript(language: string, implementation: string): string { - const dir = join(tmpdir(), `officer-task-${Date.now()}-${Math.random().toString(36).slice(2)}`); - mkdirSync(dir, { recursive: true }); - - const fileName = getFileName(language); - const filePath = join(dir, fileName); - - writeFileSync(filePath, implementation); - chmodSync(filePath, 0o755); - - return filePath; -} - -function buildInputEnv(inputs: Record): Record { - const env: Record = {}; - for (const [key, value] of Object.entries(inputs)) { - env[`INPUT_${key.toUpperCase()}`] = value; - } - return env; -} - -function buildArgs(inputs: Record, argsOrder?: string[] | null): string[] { - if (!argsOrder || argsOrder.length === 0) return []; - return argsOrder.map((name) => inputs[name] ?? ''); -} - async function handleRun(ws: ServerWebSocket, msg: RunMessage) { const { userId, email, username } = ws.data; - // Resolve task from the file-backed store const task = await getTaskByDirName(msg.taskDirName); if (!task) { send(ws, { type: 'error', message: `Task not found: ${msg.taskDirName}` }); return; } - if (task.mode !== 'script') { - send(ws, { type: 'error', message: 'Task is not a script-mode task' }); + // What the script sees comes from `script-run.ts`, shared with the job runner and the direct REST call + // — a task must not behave differently depending on which door ran it. This file owns only the + // streaming: output to the socket, a keep-alive ping, and a kill that reaches the whole tree. + let prepared; + try { + prepared = await prepareScriptRun({ + task, + inputs: msg.inputs, + cwd: msg.cwd, + user: { id: userId, email, username }, + }); + } catch (err) { + send(ws, { type: 'error', message: err instanceof Error ? err.message : 'Task cannot run' }); return; } - if (!task.implementation) { - send(ws, { type: 'error', message: `Task ${msg.taskDirName} has no implementation` }); - return; - } + const { cmd: spawnCmd, env: spawnEnv, cwd: spawnCwd, cleanup } = prepared; - const language = task.language ?? 'bash'; - - // Write script to temp dir for execution - const scriptPath = materializeScript(language, task.implementation); - - // Build env vars from inputs - const inputEnv = buildInputEnv(msg.inputs); - - // Build positional args - const positionalArgs = buildArgs(msg.inputs, task.args); - - // Build the command - const runner = getRunner(language); - const cmd = [...runner, scriptPath, ...positionalArgs]; - - // msg.cwd arrives from the file browser relative to the user's home; Bun.spawn needs it absolute - // (a missing cwd surfaces as ENOENT naming the binary, not the directory) - const homeDir = getOwnerHomeDir(email); - const cwd = msg.cwd ? (isAbsolute(msg.cwd) ? msg.cwd : join(homeDir, msg.cwd)) : homeDir; - - const spawnCmd = cmd; - const apiEnv = await buildTaskApiEnv({ id: userId, email, username }); - const spawnEnv = { ...(process.env as Record), ...apiEnv, ...inputEnv }; - const spawnCwd = cwd; - - const cleanup = () => { - try { - rmSync(join(scriptPath, '..'), { recursive: true, force: true }); - } catch { - /* best effort */ - } - }; - - send(ws, { type: 'started', taskName: task.name }); + send(ws, { type: 'started', taskName: prepared.taskName }); try { const proc = Bun.spawn(spawnCmd, { diff --git a/src/servers/api/tasks/tasks.ts b/src/servers/api/tasks/tasks.ts index 1538a8ef..5a3b7ce0 100644 --- a/src/servers/api/tasks/tasks.ts +++ b/src/servers/api/tasks/tasks.ts @@ -1,5 +1,7 @@ import { createRouter } from '../../create-router'; +import * as errors from '../../custom-errors'; import { listTasks, getTaskByDirName, createTask, deleteTask, readCategoryOrder } from './task-files'; +import { runScriptTaskDirect } from './script-run'; type TriggerConfig = { type: 'file'; extensions: string[] } | { type: 'directory' }; @@ -54,6 +56,55 @@ tasksRouter.get('/:name', async (ctx) => { }); }); +/** + * POST /tasks/:name/run — run a script task, wait for it, return what it did. + * + * The entry point for a caller with no browser. Both existing runners need something watching them: the + * socket needs a live connection to stream into, and a job needs somebody to open Jobs and read it. A + * commander's loop has neither — it holds a credential, makes one call, and acts on the answer. + * + * Deliberately NOT a job. Transcribing a memo takes seconds and produces a file; wrapping it in a queue, + * a row and a progress bar puts a screen nobody opened between the request and the result. + * + * Synchronous, so the caller's own timeout is the bound on how long a task may take. That is the honest + * shape for "do this and tell me what happened" — a task that needs to outlive its caller should be a + * job, and `POST /jobs` already is one. + */ +tasksRouter.post('/:name/run', async (ctx) => { + const user = ctx.get('user'); + const name = ctx.req.param('name'); + + const task = await getTaskByDirName(name); + if (!task) throw errors.NOT_FOUND(`Task not found: ${name}`); + if (task.mode !== 'script') throw errors.BAD_REQUEST(`Task mode '${task.mode}' cannot run directly`); + + type RunBody = { inputs?: Record; cwd?: string }; + // A run with no inputs is a legitimate call, so an absent or unparseable body is an empty one rather + // than a 400 — the task's own required-input check is the place that knows what it needs. + const body: RunBody = await ctx.req.json().catch(() => ({}) as RunBody); + + try { + const { code, output, taskName } = await runScriptTaskDirect({ + taskDirName: name, + inputs: body.inputs ?? {}, + cwd: body.cwd, + // The script inherits the caller's narrowing, or the absence of one. Without this a narrowed + // credential could write a task and run it back at the API with full authority. + user: { + id: user.id, + email: user.email, + username: user.username ?? '', + permissions: ctx.get('credentialPermissions'), + }, + }); + // A non-zero exit is reported, not thrown: the script ran, and its output is the useful part of the + // answer either way. HTTP 200 means "the run happened"; `code` means "it worked". + return ctx.json({ ok: code === 0, code, output, taskName }); + } catch (err) { + throw errors.BAD_REQUEST(err instanceof Error ? err.message : 'Task failed to start'); + } +}); + tasksRouter.post('/', async (ctx) => { const body = await ctx.req.json<{ name: string; description?: string; mode?: string; language?: string }>(); diff --git a/src/servers/auth-token.ts b/src/servers/auth-token.ts index 1fe68ab9..fed317d3 100644 --- a/src/servers/auth-token.ts +++ b/src/servers/auth-token.ts @@ -68,6 +68,14 @@ export type AuthIdentity = { iat?: number; /** JWT only, seconds since the epoch. */ exp?: number; + /** + * API key only: the permission keys this credential may reach, or null for the holder's full authority. + * + * Carried on the identity rather than looked up again by the gate, because the key row was already read + * to answer "whose is this" — and because a second lookup is a second chance for the two doors to + * disagree about the same credential, which is the failure this whole file exists to prevent. + */ + permissions?: string[] | null; }; /** @@ -86,7 +94,7 @@ export async function resolveApiKey(secret: string): Promise {}); - return { id: row.userId, email: row.email, username: row.username, via: 'api-key' }; + return { id: row.userId, email: row.email, username: row.username, via: 'api-key', permissions: row.permissions }; } /** diff --git a/src/servers/create-router.ts b/src/servers/create-router.ts index 1dfe580a..b9fd4e87 100644 --- a/src/servers/create-router.ts +++ b/src/servers/create-router.ts @@ -14,6 +14,15 @@ export type HonoVariables = { * Carrying it on the object those call sites already receive means none of them can forget. */ user: User & { homeDir?: string }; + /** + * The narrowing carried by the credential that made this request — permission keys, or null when it + * carries its holder's full authority (every session JWT, every key a human minted). + * + * Set by the permission gate, which has already resolved the token and is the only place that should. + * A route reads it when it hands authority ONWARD — `POST /tasks/:name/run` mints a token for the + * script it spawns, and that token must not be wider than the credential that asked for the run. + */ + credentialPermissions: string[] | null; }; export const createRouter = () => new Hono<{ Variables: HonoVariables }>(); diff --git a/src/servers/permissions/authorize.test.ts b/src/servers/permissions/authorize.test.ts index 7d2985b1..c7e1d74d 100644 --- a/src/servers/permissions/authorize.test.ts +++ b/src/servers/permissions/authorize.test.ts @@ -214,3 +214,51 @@ describe('deciding a websocket', () => { `api/users/permissions-routes.ts` calls the invalidator — is a claim about call sites, not about logic, and a test here could only re-state it. */ + +/* + ── A credential narrower than its holder ── + + The owner bypasses every role check, so a token carrying the owner's identity is unbounded by + construction. That is right for a browser session somebody is sitting in front of, and wrong for an + agent holding one unattended for months — a commander. The narrowing on an API key is the only thing + that can bound a Super Admin, which is why it is checked BEFORE the bypass rather than folded into the + grants map (a Super Admin never reads that map). +*/ +describe('a narrowed credential', () => { + const owner: EffectivePermissions = { isOwner: true, grants: new Map() }; + + test('bounds the owner, who nothing else bounds', () => { + expect(decideApiRequest(owner, 'POST', '/api/tasks', ['tasks']).allowed).toBe(true); + expect(decideApiRequest(owner, 'GET', '/api/users', ['tasks']).allowed).toBe(false); + expect(decideWsProvider(owner, 'terminal', ['tasks'])).toBe(false); + }); + + test('null and undefined leave the holder unnarrowed', () => { + expect(decideApiRequest(owner, 'GET', '/api/users', null).allowed).toBe(true); + expect(decideApiRequest(owner, 'GET', '/api/users').allowed).toBe(true); + }); + + // An empty list is a coherent thing to mint, and the safe default for a credential nobody has decided + // about yet. It must not read as "no narrowing". + test('an empty list reaches nothing', () => { + expect(decideApiRequest(owner, 'GET', '/api/tasks', []).allowed).toBe(false); + }); + + // Intersection, never union: naming a permission does not grant it. + test('it cannot widen an account that was never granted it', () => { + const d = decideApiRequest(resolve(member()), 'GET', '/api/file-browser', ['files']); + expect(d.allowed).toBe(false); + }); + + test('it narrows an account that was granted it', () => { + const held = resolve(member(), grants(['files', 'write'])); + expect(decideApiRequest(held, 'GET', '/api/file-browser', ['files']).allowed).toBe(true); + expect(decideApiRequest(held, 'GET', '/api/file-browser', ['tasks']).allowed).toBe(false); + }); + + // Same rule the unnarrowed path already has, one step earlier: a path nothing serves must not be + // reachable by failing to resolve. + test('a path no permission covers is refused', () => { + expect(decideApiRequest(owner, 'GET', '/api/nothing-serves-this', ['tasks']).allowed).toBe(false); + }); +}); diff --git a/src/servers/permissions/authorize.ts b/src/servers/permissions/authorize.ts index 89fdbeb3..1ec56f84 100644 --- a/src/servers/permissions/authorize.ts +++ b/src/servers/permissions/authorize.ts @@ -141,17 +141,41 @@ export async function isApiRequestAllowed( userId: number | undefined, method: string, path: string, + credential?: string[] | null, ): Promise<{ allowed: boolean; reason?: string }> { - return decideApiRequest(await getEffectivePermissions(userId), method, path); + return decideApiRequest(await getEffectivePermissions(userId), method, path, credential); } -/** The decision, given an already-resolved account. Pure, and where every rule below actually lives. */ +/** + * The decision, given an already-resolved account. Pure, and where every rule below actually lives. + * + * `credential` is the narrowing carried by an API key — the permission keys THAT key may reach, or + * null/undefined for one that carries its holder's full authority (which is every key a human has minted + * and every session JWT). It is an intersection, never a union: a key cannot reach what its account + * could not, and its account cannot reach, through this key, what the key does not name. + * + * **It is checked before the owner bypass, and that is the whole point.** A Super Admin consults no + * grants table, so a credential carrying the owner's identity is unbounded by construction — which is + * fine for a browser session the owner is sitting in front of, and not fine for an agent holding one + * unattended for months. This line is the only thing in the platform that can bound them. + */ export function decideApiRequest( effective: EffectivePermissions, method: string, path: string, + credential?: string[] | null, ): { allowed: boolean; reason?: string } { const { isOwner, grants } = effective; + + if (credential) { + const scoped = permissionForApiPath(path); + // Same deny-the-unmapped rule as below, one step earlier: a narrowed credential naming nothing that + // covers this path cannot be allowed onto it by failing to resolve the path. + if (!scoped || !credential.includes(scoped.key)) { + return { allowed: false, reason: 'this credential is not permitted to use that' }; + } + } + if (isOwner) return { allowed: true }; const permission = permissionForApiPath(path); @@ -197,13 +221,29 @@ export function decideApiRequest( * provider reaching here today belongs to an `execution` permission and is refused above, structurally, * rather than by being left off a list. The rule stays because the first plugin to own a socket needs it. */ -export async function isWsProviderAllowed(userId: number | undefined, provider: string): Promise { - return decideWsProvider(await getEffectivePermissions(userId), provider); +export async function isWsProviderAllowed( + userId: number | undefined, + provider: string, + credential?: string[] | null, +): Promise { + return decideWsProvider(await getEffectivePermissions(userId), provider, credential); } /** The decision, given an already-resolved account. See `isWsProviderAllowed` for why level is not read. */ -export function decideWsProvider(effective: EffectivePermissions, provider: string): boolean { +export function decideWsProvider( + effective: EffectivePermissions, + provider: string, + credential?: string[] | null, +): boolean { const { isOwner, grants } = effective; + + // Before the owner bypass, for the reason given in `decideApiRequest`. A socket is the one door a + // narrowed credential must not walk through just because it authenticated. + if (credential) { + const scoped = permissionForWsProvider(provider); + if (!scoped || !credential.includes(scoped.key)) return false; + } + if (isOwner) return true; const permission = permissionForWsProvider(provider);