From 4d4a253f72704b5f0c7ec9e6ed149b4920b33c87 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Padez?= Date: Tue, 11 Aug 2026 19:03:00 +0000 Subject: [PATCH] terminal runs as the member; chat is grantable and still refused MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TERMINAL is confined now, and the shell is genuinely theirs. The pty sidecar spawns it through sudo setpriv as their own account, in their own home, with the platform's environment cleared. Verified end to end against the sidecar's own socket: id -u 1001, not 1000 file the shell wrote owned by ptyprobe ps -o user=,args= ptyprobe /bin/zsh -i env | grep -c POSTGRES 0 osUser and home are resolved in upgradeWs from the authenticated account, and whatever the browser sent under those names is DELETED first. The bridge forwards the query string to the sidecar untouched and the sidecar starts a shell from what it finds there, so trusting the client for either would let a member ask for the owner's uid in a query parameter. node-pty does support uid/gid, unlike Bun.spawn, and they are deliberately unused: they set the ids without applying the account's groups or resetting the environment, so the shell would keep the owner's groups and everything Bun loaded from .env. Also closes the pty identity blindness in TODO.md. Sessions record whose they are, list and kill scope to the caller, and re-attaching to a session belonging to another account is refused — otherwise a member resumes someone else's shell by guessing an id that travels in a query string. Measured: member killing the owner's session -> ok:false, owner killing it -> ok:true. CHAT is confined so the owner can grant it and the route resolves, and both execution doors refuse a non-owner: the router wholesale, and the socket in server.tsx. The agent has not moved — the SDK spawns claude itself with nowhere to put a uid, and every transcript path resolves through the owner's home, so a member would read the owner's session list and run an agent as the owner. Reads are refused too, because listClaudePwds returns the names of the owner's projects. A deliberate, temporary gap at the owner's request: permission and route now, function when a turn can be spawned under runAs with the member's own HOME. Both guards say so, and the registry test names them so a future edit cannot move one without the other. Co-Authored-By: Claude Opus 5 --- src/server.tsx | 38 ++++++- src/servers/api/chat/chat.ts | 23 +++++ src/servers/capabilities/registry.test.ts | 18 ++-- src/servers/capabilities/registry.ts | 21 +++- src/servers/sidecar/pty/server.mjs | 14 ++- src/servers/sidecar/pty/sessions.mjs | 119 ++++++++++++++++++---- 6 files changed, 199 insertions(+), 34 deletions(-) diff --git a/src/server.tsx b/src/server.tsx index 6c652330..ac2ca1a1 100644 --- a/src/server.tsx +++ b/src/server.tsx @@ -4,9 +4,10 @@ import { serve } from 'bun'; import { honoServer, PROTECTED_API_PREFIXES, UNPROTECTED_API_PREFIXES } from './servers/hono'; import { assertCapabilityTotality } from './servers/capabilities/totality'; import { assertSecretsClosed } from './servers/os-user'; +import { resolveHomeDir } from './servers/user-home'; import { resolveAuthToken } from './servers/auth-token'; import { isWsProviderAllowed } from './servers/capabilities/authorize'; -import { isTokenBlacklisted } from 'officerdb'; +import { isTokenBlacklisted, getUserById } from 'officerdb'; import { terminalWebsocket } from './servers/api/terminal/websocket'; import { chatWebsocket } from './servers/api/chat/websocket'; import { taskRunnerWebsocket } from './servers/api/tasks/task-executor'; @@ -193,6 +194,41 @@ async function upgradeWs( const command = url.searchParams.get('command') ?? undefined; const cols = url.searchParams.get('cols') ? Number(url.searchParams.get('cols')) : undefined; const rows = url.searchParams.get('rows') ? Number(url.searchParams.get('rows')) : undefined; + + // ── Whose shell is this ── + // + // The terminal bridge forwards this query string to the pty sidecar untouched, and the sidecar starts a + // shell from what it finds there. So `osUser` and `home` are resolved HERE, from the authenticated + // account, and any values the browser sent are deleted first. Trusting the client for either would let a + // member ask for the owner's uid in a query parameter. + // + // Absent for the owner: no `osUser` means the sidecar runs the shell as itself, which is the behaviour + // this has always had. + url.searchParams.delete('osUser'); + url.searchParams.delete('home'); + + // The other half of the temporary chat gap — see api/chat/chat.ts for the whole reasoning. The capability + // is grantable so the route resolves, but a turn would spawn `claude` as the OWNER, so the transport is + // owner-only until agents run under `runAs`. Refusing the socket is what makes that true rather than + // documented. + if (provider === 'chat') { + const chatUser = await getUserById(user.id); + if (chatUser?.role !== 'Super Admin') return new Response('Forbidden', { status: 403 }); + } + + if (provider === 'terminal') { + const resolved = await resolveHomeDir(user.id); + if (!resolved.ok) return new Response('Forbidden', { status: 403 }); + if (!resolved.isOwner) { + const dbUser = await getUserById(user.id); + // A confined capability is only granted to an account with an OS user, so this should not happen — + // and if it ever does, refusing beats opening the owner's shell. + if (!dbUser?.osUser) return new Response('Forbidden', { status: 403 }); + url.searchParams.set('osUser', dbUser.osUser); + url.searchParams.set('home', resolved.home); + } + } + const ok = server.upgrade(req, { data: { userId: user.id, diff --git a/src/servers/api/chat/chat.ts b/src/servers/api/chat/chat.ts index efffece4..fd2b8bef 100644 --- a/src/servers/api/chat/chat.ts +++ b/src/servers/api/chat/chat.ts @@ -1,5 +1,7 @@ import type { Context } from 'hono'; import { createRouter } from '../../create-router'; +import { isSuperAdmin } from '@@/super-admin'; +import * as errors from '@@/custom-errors'; import * as sidecar from '@@/sidecar-registry'; import { getUserSettings } from 'officerdb'; import { @@ -30,6 +32,27 @@ import { registerAgentPanelRoutes } from './agent-panels-routes'; export const chatRouter = createRouter(); +// ── Chat is grantable, and its machinery is not ready for a member. This is that gap, held open on purpose ── +// +// The `chat` capability moved from `execution` to `confined` so the owner can grant it and the route resolves. +// The agent underneath has NOT moved: `claude-manager.ts` drives turns through the Agent SDK, which spawns +// `claude` itself with no way to hand it a uid, and every transcript path here resolves through the owner's +// home. So a member reaching this router would read the owner's session list and run an agent as the owner — +// which is the whole thing the confinement work exists to prevent. +// +// Refused wholesale rather than per-route, and reads rather than just writes: `listClaudePwds` returns the +// directory names of the owner's projects, which is not a member's business either. +// +// What lifts this is per-user agents: the turn becomes its own process under `runAs`, with the member's own +// HOME so `~/.claude` and their transcripts are theirs. docs/per-user-linux-accounts.md § stage 5. Delete this +// middleware then — it is the only thing standing between a granted member and the owner's agent. +chatRouter.use(async (ctx, next) => { + if (!(await isSuperAdmin(ctx.get('user')))) { + throw errors.FORBIDDEN('Chat is not available to members yet — the agent still runs as the server owner.'); + } + return next(); +}); + // The working directory a request operates on: an explicit ?cwd= (a chosen pwd), else the default // general_chat_sessions dir. Claude groups sessions by cwd, so this selects which project group we read. // (OpenCode sessions all live in the one fixed server and ignore cwd.) diff --git a/src/servers/capabilities/registry.test.ts b/src/servers/capabilities/registry.test.ts index 64f39e61..cb472a29 100644 --- a/src/servers/capabilities/registry.test.ts +++ b/src/servers/capabilities/registry.test.ts @@ -154,9 +154,10 @@ describe('self-service routes', () => { describe('kinds', () => { test('execution capabilities are never grantable', () => { const grantable = new Set(GRANTABLE_CAPABILITIES.map((c) => c.key)); - // `files` left this list on 2026-08-11 when it became `confined` — see the test below and - // docs/per-user-linux-accounts.md. Everything still here runs as the OWNER in the owner's home. - for (const key of ['terminal', 'chat', 'tasks', 'desktop', 'browser', 'items']) { + // `files` left this list on 2026-08-11, then `terminal` and `chat` the same day — see the tests below and + // docs/per-user-linux-accounts.md. Everything still here runs as the OWNER in the owner's home with no + // per-caller resolution at all. + for (const key of ['tasks', 'desktop', 'browser', 'items']) { expect(CAPABILITY_BY_KEY.get(key)?.kind).toBe('execution'); expect(grantable.has(key)).toBe(false); } @@ -166,7 +167,7 @@ describe('kinds', () => { // authorize.ts, which is where the rule can cover routes, sockets and the dock at once. test('confined capabilities are grantable', () => { const grantable = new Set(GRANTABLE_CAPABILITIES.map((c) => c.key)); - for (const key of ['files']) { + for (const key of ['files', 'terminal', 'chat']) { expect(CAPABILITY_BY_KEY.get(key)?.kind).toBe('confined'); expect(grantable.has(key)).toBe(true); } @@ -174,9 +175,14 @@ describe('kinds', () => { // The claim `confined` makes is that every path it reaches resolves its directory from the CALLER. That // cannot be asserted from the registry, so this pins the inverse: nothing becomes confined without a - // deliberate edit here, and the list is short enough to audit by eye. + // deliberate edit here, and the list stays short enough to audit by eye. + // + // `chat` is on it ahead of its implementation, deliberately and temporarily: the capability is grantable so + // the route resolves, while api/chat/chat.ts and the chat socket both refuse a non-owner because a turn + // would still spawn the agent as the owner. If you are here because this test failed, check that those two + // guards moved together with whatever you changed. test('confined is a short, deliberate list', () => { - expect(CAPABILITIES.filter((c) => c.kind === 'confined').map((c) => c.key)).toEqual(['files']); + expect(CAPABILITIES.filter((c) => c.kind === 'confined').map((c) => c.key)).toEqual(['terminal', 'chat', 'files']); }); test('admin capabilities are never grantable', () => { diff --git a/src/servers/capabilities/registry.ts b/src/servers/capabilities/registry.ts index 6f8db18b..4ab75659 100644 --- a/src/servers/capabilities/registry.ts +++ b/src/servers/capabilities/registry.ts @@ -255,20 +255,33 @@ export const CAPABILITIES: Capability[] = [ }, // ── execution: never grantable ────────────────────────────────────────────────────────────────── + // Confined since 2026-08-11. A member's shell is spawned by the pty sidecar through `sudo setpriv` as their + // own Linux account, in their own home, with the platform's environment cleared — so it is their shell, and + // the kernel decides what it can reach. The sidecar also records whose each session is, so `list` and `kill` + // scope to the caller instead of every shell on the box. + // + // What this is NOT is a jail. A member with a shell can `cd /` and read whatever the system leaves + // world-readable, like any account on any machine. It isolates members from each other and from the owner's + // files, which is the promise `confined` makes. { key: 'terminal', label: 'Terminal', - description: 'A real shell as the server owner', - kind: 'execution', + description: 'A shell on this machine, as your own user', + kind: 'confined', api: ['/terminal'], ws: ['terminal'], routes: ['/terminal'], }, + // Confined so the owner can grant it and the route resolves — but the agent underneath still runs as the + // OWNER, so `api/chat/chat.ts` refuses a non-owner outright and the chat socket is refused in server.tsx. + // A deliberate, temporary gap: the permission exists, the functionality follows when a turn can be spawned + // under `runAs` with the member's own HOME. Until then this grant buys a route and a refusal, and the + // comments at both guards say so. { key: 'chat', label: 'Chat', - description: 'The agent, running unsandboxed as the server owner', - kind: 'execution', + description: 'The agent', + kind: 'confined', api: ['/chat'], ws: ['chat'], routes: ['/chat'], diff --git a/src/servers/sidecar/pty/server.mjs b/src/servers/sidecar/pty/server.mjs index cf640952..2b039ba3 100644 --- a/src/servers/sidecar/pty/server.mjs +++ b/src/servers/sidecar/pty/server.mjs @@ -27,13 +27,19 @@ export function startServer() { const url = new URL(req.url ?? '/', 'http://127.0.0.1'); // Officer-owned routes, reached through the platform's authenticated proxy. + // + // `osUser` scopes both to one account's sessions. The platform sends it for a member and omits it for the + // owner; absent means unscoped. Until this existed these two listed and killed EVERY shell on the box for + // anyone who could reach them, which was safe only because the terminal was owner-only. + const scope = url.searchParams.has('osUser') ? (url.searchParams.get('osUser') || null) : undefined; + if (url.pathname === '/_officer/sessions' && req.method === 'GET') { - return json(res, 200, { sessions: store.list() }); + return json(res, 200, { sessions: store.list(scope) }); } const killMatch = url.pathname.match(/^\/_officer\/sessions\/([^/]+)$/); if (killMatch && req.method === 'DELETE') { - const killed = store.kill(decodeURIComponent(killMatch[1])); + const killed = store.kill(decodeURIComponent(killMatch[1]), scope); return json(res, killed ? 200 : 404, { ok: killed }); } @@ -55,6 +61,10 @@ export function startServer() { cwd: url.searchParams.get('cwd') ?? undefined, cols: Number(url.searchParams.get('cols')) || 0, rows: Number(url.searchParams.get('rows')) || 0, + // Injected by the platform's bridge from the authenticated account, after stripping whatever the + // browser sent. Absent for the owner, whose shell runs as this process. + osUser: url.searchParams.get('osUser') || undefined, + home: url.searchParams.get('home') || undefined, }); if (!session) { socket.close(1011, 'failed to start terminal'); diff --git a/src/servers/sidecar/pty/sessions.mjs b/src/servers/sidecar/pty/sessions.mjs index 4935175f..a34792e0 100644 --- a/src/servers/sidecar/pty/sessions.mjs +++ b/src/servers/sidecar/pty/sessions.mjs @@ -28,12 +28,21 @@ const SHELL = { command: process.env.SHELL ?? '/bin/zsh', args: ['-i'] }; /** @type {Map} */ const sessions = new Map(); -const resolveCwd = (cwd) => { - if (!cwd || cwd === '~') return HOME_DIR; - if (cwd.startsWith('~/')) return join(HOME_DIR, cwd.slice(2)); +/** + * Where a shell opens. `base` is the requesting account's home; without one this is the owner's HOME_DIR, as + * it always was. + * + * An absolute `cwd` is honoured as given, for the owner and for a member alike. That is not a hole and not an + * oversight: the member's shell runs as their uid, so the kernel decides what they can see once they get + * there. A path check here would be theatre — `cd /` is one keystroke away in any shell. + */ +const resolveCwd = (cwd, base) => { + const home = base || HOME_DIR; + if (!cwd || cwd === '~') return home; + if (cwd.startsWith('~/')) return join(home, cwd.slice(2)); if (cwd.startsWith('/')) return cwd; // Relative paths have no meaning here — this process's cwd is the repo, not the user's folder. - return HOME_DIR; + return home; }; const appendBuffer = (session, data) => { @@ -58,11 +67,59 @@ const broadcast = (session, msg) => { } }; -/** Attach a client to a session, spawning the shell if this is the first time we've seen the id. */ -export function attach(sessionId, client, { cwd, cols, rows } = {}) { +/** + * The argv that opens a shell as `osUser`, or as this process when there is none. + * + * Mirrors `runAs` in src/servers/os-user.ts, and cannot import it: this sidecar runs under node (node-pty + * binds a native addon against node's ABI) while that file is Bun/TypeScript. If you change one, change both + * — the flags are load-bearing and the reasoning for each is documented there. + * + * node-pty does support `uid`/`gid` options, unlike Bun.spawn. They are deliberately NOT used: they set the + * ids without applying the account's supplementary groups or resetting the environment, so the shell would + * keep the OWNER's groups and the platform's entire env — including everything Bun loaded from `.env`. + * + * `--reset-env` keeps TERM (per setpriv(1)) and sets HOME/SHELL/USER/LOGNAME/PATH from the target's passwd + * entry. COLORTERM does not survive it, so it is re-stated through `env` — it is how programs decide they may + * emit 24-bit colour. + */ +function shellArgv(osUser) { + if (!osUser) return { command: SHELL.command, args: SHELL.args }; + return { + command: 'sudo', + args: [ + '-n', + 'setpriv', + `--reuid=${osUser}`, + `--regid=${osUser}`, + '--init-groups', + '--reset-env', + '--', + 'env', + 'COLORTERM=truecolor', + SHELL.command, + ...SHELL.args, + ], + }; +} + +/** + * Attach a client to a session, spawning the shell if this is the first time we've seen the id. + * + * `osUser` and `home` come from the PLATFORM, which resolves them from the authenticated account — never from + * anything the browser sent. This socket binds loopback and only the platform's bridge reaches it, which is + * the same trust model as the `X-Officer-User` header on the HTTP sidecars. + */ +export function attach(sessionId, client, { cwd, cols, rows, osUser, home } = {}) { let session = sessions.get(sessionId); if (session) { + // A session belongs to whoever started it. Re-attaching is only allowed for the same account, or this is + // how one member resumes another's shell by guessing a session id — and the id is in a query string. + if ((session.osUser ?? null) !== (osUser ?? null)) { + client.send({ type: 'output', data: '\r\n[Terminal error] that session belongs to another account\r\n' }); + client.send({ type: 'exit', exitCode: 1 }); + return null; + } session.clients.add(client); // Scrollback goes out as `replay`, not as ordinary output: the client may already be showing some of // it, so it resets and rebuilds from this rather than appending a second copy. @@ -76,12 +133,16 @@ export function attach(sessionId, client, { cwd, cols, rows } = {}) { let term; try { - term = pty.spawn(SHELL.command, SHELL.args, { + const { command, args } = shellArgv(osUser); + term = pty.spawn(command, args, { name: 'xterm-256color', cols: spawnCols, rows: spawnRows, - cwd: resolveCwd(cwd), - // COLORTERM is how programs decide they may emit 24-bit colour — TERM only advertises 256. + // A member's shell starts in THEIR home, not the owner's. `resolveCwd` resolves `~` and relative paths + // against HOME_DIR, which is the owner's — so the base has to be passed in for anyone else. + cwd: resolveCwd(cwd, home), + // For a member this env reaches `sudo` and `setpriv`, and `--reset-env` clears it before the shell: + // see shellArgv. For the owner it is the shell's environment as before. env: { ...process.env, TERM: 'xterm-256color', COLORTERM: 'truecolor' }, }); } catch (err) { @@ -101,6 +162,9 @@ export function attach(sessionId, client, { cwd, cols, rows } = {}) { lastActivityAt: now, title: '', pid: term.pid, + /** Whose shell this is, or null for the owner. Read by `list` and `kill` so one account cannot see or + * end another's — the gap TODO.md called the pty sidecar's identity blindness. */ + osUser: osUser ?? null, clients: new Set([client]), }; sessions.set(sessionId, session); @@ -145,9 +209,18 @@ export function resize(sessionId, cols, rows) { } } -export function kill(sessionId) { +/** + * End a session. `osUser` scopes it: pass an account name and only that account's sessions can be killed; + * pass nothing and it is the owner, who may kill any. + * + * Undefined and null mean different things here, which is why the check is explicit. `undefined` is "the + * caller did not scope this" (the owner). `null` is "the caller is the owner's own shell", which is a real + * value a member must not match. + */ +export function kill(sessionId, osUser) { const session = sessions.get(sessionId); if (!session) return false; + if (osUser !== undefined && (session.osUser ?? null) !== osUser) return false; try { session.term.kill(); } catch { @@ -157,17 +230,21 @@ export function kill(sessionId) { return true; } -export function list() { - return [...sessions.entries()].map(([sessionId, s]) => ({ - sessionId, - cols: s.cols, - rows: s.rows, - createdAt: s.createdAt, - lastActivityAt: s.lastActivityAt, - title: s.title || undefined, - pid: s.pid, - clients: s.clients.size, - })); +/** Sessions, scoped the same way as `kill`: an account name sees only its own, nothing sees everything. */ +export function list(osUser) { + return [...sessions.entries()] + .filter(([, s]) => osUser === undefined || (s.osUser ?? null) === osUser) + .map(([sessionId, s]) => ({ + sessionId, + cols: s.cols, + rows: s.rows, + createdAt: s.createdAt, + lastActivityAt: s.lastActivityAt, + title: s.title || undefined, + pid: s.pid, + clients: s.clients.size, + osUser: s.osUser ?? undefined, + })); } export function killAll() {