terminal runs as the member; chat is grantable and still refused
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 <noreply@anthropic.com>
This commit is contained in:
+37
-1
@@ -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,
|
||||
|
||||
Reference in New Issue
Block a user