From e29a4ee3807f698ff576c9a9de8f745c2b9c79f8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Padez?= Date: Mon, 24 Aug 2026 13:40:42 +0000 Subject: [PATCH] a credential can be narrower than its holder MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit a commander is an agent working unattended for months, and the only credential in the tree lived in the MCP tool server's environment where the agent's own shell cannot see it. so the first one built here curled the whisper container directly on localhost: right answer, wrong door, and a door the platform does not control. api_keys.permissions. null is what every key a human minted still is — their full authority, exactly what the password that made it could do. a list narrows: the credential reaches only those permission keys, and still only where the account itself could. intersection, never union. checked BEFORE the owner bypass, which is the whole point. a Super Admin consults no grants table, so a token carrying the owner's identity is unbounded by construction — fine for a browser session somebody is sitting in front of, not fine for a process that runs for months. this is the only thing in the platform that can bound them. verified against the running server: an owner key scoped to [account] gets 200 on /api/user/settings and 403 on /api/users, /api/tasks and /api/plugins. it propagates. a narrowed credential can write a task and run it, so the token buildTaskApiEnv hands that script carries the same narrowing — otherwise the script comes back through the API with full authority and the limit bought nothing. the gate stashes it on the request for the one kind of route that passes authority onward. POST /tasks/:name/run: runs a script task, waits, returns code and output. both existing runners need something watching them — the socket needs a live connection, a job needs somebody to open Jobs. a commander has neither. deliberately not a job: transcribing a memo takes seconds and produces a file, and a queue plus a row plus a progress bar is a screen nobody opened between the request and the result. script-run.ts is the third thing this needed and the one worth having on its own. the socket runner, the job runner and now this each carried the same twenty lines — materialize, INPUT_ env, positional args, resolve cwd, mint the api env — identical, with nothing saying they had to stay that way. they differ in how they REPORT, which is real and stays with each of them. what the script SEES is now decided once. a commander gets its key at creation, in officer.env at 0600 inside its own directory, and CLAUDE.md tells it where and says plainly that a sidecar listening on localhost with no auth is still the wrong door. a file rather than an env var because the detached loop a commander arms is spawned by the agent and inherits nothing the platform chose. deleting the commander revokes the row — the directory removal takes the secret, not the thing that makes it work. default scope is everything except `admin`: a commander that can reach nothing gets widened in a hurry rather than thought about, and installing plugins, managing users and changing server settings are never part of a watcher's job. it is a default, stored per key, and for a member it narrows itself against their role. also fixed: POST /command-hq/commanders/new never awaited createCommanderDirectory, so a turn could spawn before its CLAUDE.md existed — the exact ordering that redesign was for — and the cwd it returned was a Promise serialising as {}. Co-Authored-By: Claude Opus 5 Officer-Session: 656f5c85-e49a-4f1f-b9f3-042a04b4d043 --- .../officer_db/src/api-keys/index.ts | 1 + .../officer_db/src/api-keys/queries.ts | 25 +++ .../officer_db/src/api-keys/schema.ts | 22 ++- src/server.tsx | 2 +- src/servers/_middlewares/permission-gate.ts | 13 +- src/servers/api/command-hq/credential.ts | 98 ++++++++++ src/servers/api/command-hq/directory.ts | 56 +++++- src/servers/api/command-hq/router.ts | 40 ++++- src/servers/api/tasks/execute-script.ts | 78 ++------ src/servers/api/tasks/script-run.ts | 169 ++++++++++++++++++ src/servers/api/tasks/task-api-env.ts | 28 ++- src/servers/api/tasks/task-executor.ts | 121 ++----------- src/servers/api/tasks/tasks.ts | 51 ++++++ src/servers/auth-token.ts | 10 +- src/servers/create-router.ts | 9 + src/servers/permissions/authorize.test.ts | 48 +++++ src/servers/permissions/authorize.ts | 50 +++++- 17 files changed, 631 insertions(+), 190 deletions(-) create mode 100644 src/servers/api/command-hq/credential.ts create mode 100644 src/servers/api/tasks/script-run.ts 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);