From 5bb489addb76fa9b500234e1eaa0ec5d52b48b78 Mon Sep 17 00:00:00 2001 From: Randalix Date: Sat, 19 Sep 2026 11:39:50 +0200 Subject: [PATCH] fix(remote): authorize the attach wake first; tell the caller what happened to its bytes Review round 3 on #439. - The attachRemoteSession branch of POST /api/sessions ran `ensureHostAwake` before the multi-user gates, so a non-admin could have any configured host's `wakeCommand` spawned (or a packet broadcast) and the request held for the wake budget, then be refused for the workingDir. The admin gate now comes first, before the host is even looked up; remote hosts are admin-only infrastructure everywhere else. Route test: wake spy empty, 403. - The non-wait input route answers `{buffered:true}` when the registry took the chunk and `{buffered:true, dropped:true}` when it was over the cap and is gone (`RemoteInputOutcome` gains 'dropped'); additive to the bare `{}`. - The send-and-wait path answers OPERATION_FAILED when the host never comes back, like create and attach, instead of writing into the stalled pane and reporting delivered:true plus a timeout. - The flush writes with `fromUser: true`, so a first prompt buffered through a wake can still name the tab. Docs: api-reference (input route), remote-sessions.md (two invariants), CLAUDE.md key pattern. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01QdGP4jUTjc9J2RYYykDrCG --- CLAUDE.md | 2 +- docs/api-reference.md | 10 ++ docs/remote-sessions.md | 16 +++ src/remote-wake.ts | 30 +++-- src/web/routes/session-routes.ts | 29 ++++- test/remote-wake.test.ts | 18 +++ test/routes/session-remote-wake.test.ts | 146 +++++++++++++++++++++++- 7 files changed, 232 insertions(+), 19 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 3c269cfc..d30da59c 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -217,7 +217,7 @@ Codeman is a Claude Code session manager with web interface and autonomous Ralph **Remote sessions + remote SSH cases**: a case can point at a remote host. The agent runs inside a durable remote `tmux -L codeman-remote` (session name `codeman-ssh-`, deliberately failing the remote Codeman's `SAFE_MUX_NAME_PATTERN` so an instance on the target host never adopts it), fronted by a LOCAL tmux pane running `ssh`. Attached (`owned:false`) sessions **detach, never kill** on tab close; owned ones propagate `kill-session`. A bounded-backoff watcher auto-reconnects dropped sessions (`remoteAutoReconnect`, default ON). ⚠️ **It revives ONLY when the durable remote tmux session is verifiably still alive** (`remoteTmuxSessionAlive()`, a `has-session` probe over ssh, #355): a clean agent exit (Ctrl-C, Ctrl-D, `exit`) tears that session down, and `isPaneDead()` cannot tell it from a transport drop, so the watcher used to relaunch a FRESH agent after every clean exit (claude only looked fine because its `|| --resume` fallback masked it). An unreachable host answers `undefined`, which also means do not revive. ⚠️ `has-session` prints NOTHING on success, so the probe is classified by EXIT STATUS (`classifyRemoteAliveExit`: 0 alive, ssh's 255 or a timeout unknown, anything else gone); reading stdout classified every live session as gone and silently disabled transport-drop reconnects. The answer is cached per session and forgotten whenever the pane is seen alive again, or a stale `true` from one transport drop would revive the next clean exit. ⚠️ **File reads in a remote case are the second ssh surface** (#415, `src/remote-files.ts`): they go through `buildSshConnectionArgs()` as well, a browser-supplied path is only ever a `shellescape`d token, an unreachable host answers 502 (never 404), the size cap uses the REMOTE size, and no remote file is ever copied onto the server's disk — which is why writes, office previews and thumbnails are deliberately unsupported over ssh (the `PUT` guard sits BEFORE the local path validation, or a same-named local directory such as an sshfs mount takes the write). The probe's symlink resolution FAILS CLOSED (a path it cannot canonicalize is a 404, never its own unresolved string: the directory-only fallback let a `notes.txt -> ~/.ssh/id_rsa` link pass containment), and ssh children are BOUNDED by `src/remote-ssh-limiter.ts` plus one batched probe per attachment-history listing, because terminal output in a remote session is written on the remote host and a prompt-injected agent can print hundreds of `codeman://attach` links. The ATTACHMENT routes (a clicked path outside the case dir) go through the same layer, and which host a record is read from follows the SESSION, never the path string. ⚠️ **Command-injection surface: every ssh command line must flow through `buildSshConnectionArgs()`**, which `shellescape`s every user field. Never hand-build an ssh line elsewhere. ⚠️ Run flows must route remote cases through `POST /api/quick-start`, not `POST /api/sessions` (which stat-validates `workingDir` locally and has no `caseName`). → [architecture-invariants#remote-sessions-over-ssh](docs/architecture-invariants.md#remote-sessions-over-ssh), [#remote-ssh-cases](docs/architecture-invariants.md#remote-ssh-cases), `docs/remote-sessions.md` -**Wake-on-LAN (`remote-wake.ts`)**: an optional `RemoteHost.wakeMac` (Codeman builds the magic packet itself) or `RemoteHost.wakeCommand` (single executable path, run without a shell, takes precedence) lets the INPUT route, `POST /api/sessions/:id/wake`, and the user's own create/attach request (`POST /api/quick-start`, `POST /api/sessions` with `attachRemoteSession`, via `ensureHostAwake`) wake a sleeping host instead of writing into a stalled ssh pane. ⚠️ An explicit request — input, the wake button, or the user pressing Run/Attach — and NOTHING else may wake: the auto-reconnect watcher, `handleRemoteSessionDropped`, boot recovery and `cron-service.ts` have no access to the registry (a wake there would re-wake the host seconds after every suspend, and the create wake is wired in the route rather than the shared session service for exactly that reason), which `test/remote-wake.test.ts` asserts as two wiring guards — the second also pins that `server.ts` holds the registry for its LIFETIME only (`drop` on cleanup, `stop` on shutdown) and never calls a waking method. `GET /api/sessions/:id/reachability` merely probes and never wakes. Detection is a throttled bare TCP probe — deliberately no `ServerAliveInterval`, because keepalives move bytes into an idle connection every interval and that is what a byte-threshold idle detector must not read as activity. ⚠️ A host behind `jumpHost`/`socksProxy`/a `ProxyCommand` option is reachability-UNKNOWN (`isProbeable()`): the probe connects to `host:port`, which such a host does not answer even while ssh works, so the registry never buffers for it, never gates create/attach on it (`'unprobeable'`), and `/reachability` answers `reachable: null, probeable: false` — the banner keys on a PROVEN `false`, and the banner's 30 s poller runs only for a host with a wake target (a timer connecting to a host Codeman cannot wake is the same timer-driven traffic the keepalive rule forbids). Input arriving during a wake is buffered (a chunk over 4 KB is dropped whole, never delivered as a fragment) and flushed in order after `reattachRemote()` — a flush write that fails drops the rest (logged) rather than retaining it for a wake hours later; send-and-wait blocks instead. ⚠️ Browser keystrokes travel over the WebSocket, which deliberately does NOT pass through the registry (that is the hot path), so only the HTTP input path ever queues anything — the banner must not promise queued input for the Wake button. A request that waits on the wake (create/attach, and the button) uses the 40 s `REMOTE_WAKE_REQUEST_READY_TIMEOUT_MS`, not the 90 s session default, because the dashboard's reverse proxy cuts a request at its own 60 s `proxy_read_timeout`. The wake fields are re-read from `remote-hosts.json` on recovery and, throttled+cached via `RemoteWakeDeps.resolveRemote`, for a LIVE session, since the persisted `remote` snapshot never sees a field added later. UI: the amber `#hostWakeBanner` (`host-wake-ui.js`) with Wake / "Configure WoL" → `#wakeConfigModal`. The `remote:` SSE family is session-scoped in multi-user mode; a create/attach wake names its requester (`username`) since it has no session yet. `remote-wake.ts` refuses real IO under `VITEST` like `remote-files.ts`. +**Wake-on-LAN (`remote-wake.ts`)**: an optional `RemoteHost.wakeMac` (Codeman builds the magic packet itself) or `RemoteHost.wakeCommand` (single executable path, run without a shell, takes precedence) lets the INPUT route, `POST /api/sessions/:id/wake`, and the user's own create/attach request (`POST /api/quick-start`, `POST /api/sessions` with `attachRemoteSession`, via `ensureHostAwake`) wake a sleeping host instead of writing into a stalled ssh pane. ⚠️ An explicit request — input, the wake button, or the user pressing Run/Attach — and NOTHING else may wake: the auto-reconnect watcher, `handleRemoteSessionDropped`, boot recovery and `cron-service.ts` have no access to the registry (a wake there would re-wake the host seconds after every suspend, and the create wake is wired in the route rather than the shared session service for exactly that reason), which `test/remote-wake.test.ts` asserts as two wiring guards — the second also pins that `server.ts` holds the registry for its LIFETIME only (`drop` on cleanup, `stop` on shutdown) and never calls a waking method. `GET /api/sessions/:id/reachability` merely probes and never wakes. Detection is a throttled bare TCP probe — deliberately no `ServerAliveInterval`, because keepalives move bytes into an idle connection every interval and that is what a byte-threshold idle detector must not read as activity. ⚠️ A host behind `jumpHost`/`socksProxy`/a `ProxyCommand` option is reachability-UNKNOWN (`isProbeable()`): the probe connects to `host:port`, which such a host does not answer even while ssh works, so the registry never buffers for it, never gates create/attach on it (`'unprobeable'`), and `/reachability` answers `reachable: null, probeable: false` — the banner keys on a PROVEN `false`, and the banner's 30 s poller runs only for a host with a wake target (a timer connecting to a host Codeman cannot wake is the same timer-driven traffic the keepalive rule forbids). Input arriving during a wake is buffered (a chunk over 4 KB is dropped whole, never delivered as a fragment; the route answers `{buffered:true}` / `{buffered:true, dropped:true}` so the caller can tell) and flushed in order after `reattachRemote()` with `fromUser` — a flush write that fails drops the rest (logged) rather than retaining it for a wake hours later; send-and-wait blocks instead and answers `OPERATION_FAILED` when the host never returns. ⚠️ In multi-user mode the attach path 403s a non-admin BEFORE the host is looked up: the wake runs an executable, and remote hosts are admin-only infra everywhere else. ⚠️ Browser keystrokes travel over the WebSocket, which deliberately does NOT pass through the registry (that is the hot path), so only the HTTP input path ever queues anything — the banner must not promise queued input for the Wake button. A request that waits on the wake (create/attach, and the button) uses the 40 s `REMOTE_WAKE_REQUEST_READY_TIMEOUT_MS`, not the 90 s session default, because the dashboard's reverse proxy cuts a request at its own 60 s `proxy_read_timeout`. The wake fields are re-read from `remote-hosts.json` on recovery and, throttled+cached via `RemoteWakeDeps.resolveRemote`, for a LIVE session, since the persisted `remote` snapshot never sees a field added later. UI: the amber `#hostWakeBanner` (`host-wake-ui.js`) with Wake / "Configure WoL" → `#wakeConfigModal`. The `remote:` SSE family is session-scoped in multi-user mode; a create/attach wake names its requester (`username`) since it has no session yet. `remote-wake.ts` refuses real IO under `VITEST` like `remote-files.ts`. **Docker cases**: a case can point at a **container**, with any of the CLI run modes running inside it. Like remote-SSH this is a **LOCATION OVERLAY on cases, never a `SessionMode` of its own**. Exactly one long-lived container **per case**, shared by all its sessions, so killing a session kills only that session's in-container tmux and **never** `docker stop` while siblings remain. The workspace is a real host dir bind-mounted at the **same absolute path**, which is what keeps file-routes/watchers on real host bytes and makes the in-container transcript projHash match the host. Credentials are **seeded** (RO mount, copied into the container once) rather than shared RW, so in-container CLIs never write refreshed tokens back to the host, and bind mounts are excluded from `docker commit` so exports stay secret-free. **NEVER a create-time `-e` for secrets, NEVER `--privileged`, NEVER the docker socket.** Config drift is detected via a label hash and a drifted launch is REFUSED rather than silently launched with stale config. ⚠️ A case may instead **ADOPT** a container the user already runs (`DockerCase.owned === false`, mirror of remote-SSH's `owned:false`): Codeman only `exec`s into it and never creates, starts, stops, restarts or removes it, so a missing or stopped container FAILS CLOSED with an actionable message instead of being fixed. Absent = owned, so existing cases are byte-identical. ⚠️ An ADOPTED container may back SEVERAL cases at different in-container directories (`classifyAdoptContainerConflict` in `docker-hosts.ts`: an exact twin on the same container AND directory is refused, an owned container still backs exactly one case, and a container another user adopted is refused), which is what the Add Case panel's "copy an existing case" picker relies on; the wire carries `CaseInfo.docker.owned` ONLY when false, so the picker tests `=== false`, never truthiness. The guarantee is enforced at four independent layers because it cannot be observed by using the feature: `buildDockerStopCommand`/`buildDockerRemoveCommand` throw during pure STRING CONSTRUCTION, `removeDockerContainer` refuses again, drift reports "none" (an adopted container carries no `codeman.confighash` label, so a real comparison would 409 the launch forever), and the boot reaper skips it. ⚠️ Two lifecycle touches the original design missed and that are easy to re-introduce: the full-image export `docker commit`s the container (refused for an adopted case) and the workspace export `docker pause`s it first (skipped — it freezes the owner's processes for the length of the tar). ⚠️ `owned` is applied AFTER `dockerConfigHash`, which takes an explicit field list, or every pre-existing case would trip the drift gate at once. ⚠️ Run modes for a container case come from the CONTAINER (`availableModes`, live-probed): gating the run menu on HOST CLIs (#201) is right for local sessions and wrong here, since a host with no `claude` may run a container that ships one. ⚠️ **A failed probe means opposite things per ownership** — for an ADOPTED case it is a fault worth reporting, for an OWNED one it is the NORMAL state before the first session (the launch chain creates the container), so treating it as a fault hid every agent mode on every freshly linked Docker case behind "start it yourself first". That is why `CaseInfo.docker.owned` is on the wire. ⚠️ Claude is launched WITHOUT `--dangerously-skip-permissions` when the container's exec user is root (Claude Code refuses the flag as root and the refusal is visible only inside the container); which flag to drop is a per-CLI fact, so it is the registry's `overlays.docker.rootCommand`, never a branch. ⚠️ Adoption is **admin-only in multi-user mode**, unlike `docker-link`: linking creates OUR container, whose one bind mount `isWorkingDirAllowed` has already confined, while an adopted container's mounts belong to its owner and one mounting `/` hands the adopter the host. The same reasoning admin-gates the container listing and the in-container directory browser; the preflight instead admits a non-admin for a container already linked to a case they own, because the run menu probes it for every docker case. ⚠️ On the loopback-only prod bind a container cannot reach 127.0.0.1, so in-container hooks need `CODEMAN_DOCKER_BRIDGE_HOOKS=1`; otherwise idle detection falls back to output-based. → [architecture-invariants#docker-cases](docs/architecture-invariants.md#docker-cases), `docs/docker-cases.md` (user guide), `docs/docker-cases-plan.md` (design) diff --git a/docs/api-reference.md b/docs/api-reference.md index 53b38e8a..b1b87894 100644 --- a/docs/api-reference.md +++ b/docs/api-reference.md @@ -324,6 +324,16 @@ from the session's current state rather than requiring a new transition: the original turn may be long over. It comes back as `"delivered": false, "duplicate": true`. +**Wake-on-LAN hosts** (`docs/remote-sessions.md` §Wake-on-LAN): when the session's +remote host has a wake target and is asleep, the non-wait form answers `200` with +`{"buffered": true}` — the bytes are held and flushed after the host is back — or +`{"buffered": true, "dropped": true}` for a chunk over the 4 KB wake buffer, which +is gone (never delivered as a fragment). Both fields are additive to the historical +bare `{}`. With `wait`, the route blocks on the wake instead and answers +`422 OPERATION_FAILED` ("did not come back after a wake-on-LAN request — nothing was +sent") when the host never returns, rather than writing into the stalled pane and +reporting `delivered:true` plus a timeout. + ### Response All three nest the wait result under `data.wait`, so one client helper works against diff --git a/docs/remote-sessions.md b/docs/remote-sessions.md index 15dde0df..c10c6f07 100644 --- a/docs/remote-sessions.md +++ b/docs/remote-sessions.md @@ -430,6 +430,22 @@ command goes out and the response says only whether it did — no readiness poll The invariants worth keeping: +- **Authorization comes before the wake.** In multi-user mode the attach path + (`POST /api/sessions` + `attachRemoteSession`) answers `403` to a non-admin BEFORE the + host is looked up or probed: remote hosts are admin-only infrastructure everywhere else + (the list is `[]` for a non-admin, write and discovery routes are `adminOnly`), and the + wake spawns the host's `wakeCommand` or broadcasts a packet — a gate that came after the + wake handed an unprivileged account a way to run that executable for any configured + `hostId`, hold the request for the wake budget, and only then be refused for the + workingDir. The quick-start path resolves its remote case through `canAccessOwned` + first. Pinned in `test/routes/session-remote-wake.test.ts` (wake spy stays empty). +- **The caller is told what happened to its bytes.** The non-wait input route answers + `{buffered:true}` when the registry took the chunk and `{buffered:true, dropped:true}` + when it was over the cap and is gone; the send-and-wait route answers `OPERATION_FAILED` + when the host never comes back, like the create and attach paths, instead of writing + into the stalled pane and reporting `delivered:true` plus a timeout. Flushed chunks are + written with `fromUser`, so a first prompt that was buffered through a wake can still + name the tab. - **Only an EXPLICIT request may wake a host:** user input on an established session, the wake button, or the user's own session create/attach request (`ensureHostAwake`). Everything that runs on a TIMER must never wake one — the COD-108 watcher, the server's dropped-session diff --git a/src/remote-wake.ts b/src/remote-wake.ts index 170eff28..884a83cb 100644 --- a/src/remote-wake.ts +++ b/src/remote-wake.ts @@ -81,7 +81,7 @@ export type RemoteInputAction = 'deliver' | 'probe' | 'buffer'; * The caller-facing outcome of {@link RemoteWakeRegistry.handleInput}: either the * caller writes the bytes as usual, or the registry took ownership of them. */ -export type RemoteInputOutcome = 'deliver' | 'buffered'; +export type RemoteInputOutcome = 'deliver' | 'buffered' | 'dropped'; /** * Decide what to do with an input chunk on an input route. Mirrors @@ -240,8 +240,8 @@ export interface WakeableSession { readonly remote: WakeableRemote | undefined; /** COD-108 reattach: respawns the local ssh pane, idempotently attaching the durable remote tmux. */ reattachRemote(): Promise; - /** Write bytes to the session's pane. */ - writeViaMux(data: string): Promise; + /** Write bytes to the session's pane. `fromUser` marks input a person typed (or an agent sent for them). */ + writeViaMux(data: string, options?: { fromUser?: boolean }): Promise; } /** Injected IO so the registry holds no direct dependency on ssh/net/child_process in tests. */ @@ -442,12 +442,12 @@ export class RemoteWakeRegistry { if (action === 'deliver') return 'deliver'; if (action === 'buffer') { - this._enqueue(session.id, data); + const queued = this._enqueue(session.id, data); // A buffered verdict with no wake in flight still has to DRIVE a wake (the // previous one failed and reset the probe state, or the ladder landed here // directly) — otherwise the bytes would sit in the buffer forever. if (state.waking == null && target) void this.wake(session); - return 'buffered'; + return queued; } // action === 'probe' — the throttle window elapsed, so one TCP connect is owed. @@ -455,9 +455,9 @@ export class RemoteWakeRegistry { state.reachable = remote ? await this.deps.probe(remote) : true; if (state.reachable) return 'deliver'; - this._enqueue(session.id, data); + const queued = this._enqueue(session.id, data); void this.wake(session); - return 'buffered'; + return queued; } /** @@ -749,17 +749,19 @@ export class RemoteWakeRegistry { return state.resolvedRemote ?? session.remote; } - private _enqueue(sessionId: string, data: string): void { + /** Queue a chunk; `'dropped'` when it was over the cap and never entered the buffer. */ + private _enqueue(sessionId: string, data: string): 'buffered' | 'dropped' { const state = this._state(sessionId); const next = appendBoundedPending(state.pending, data); if (next === state.pending) { // Oversized chunk: dropped whole (see `appendBoundedPending`), so the buffer is - // untouched and nothing is delivered as a fragment. Logged because the user's - // paste is gone — the 200 the route returns cannot say so. + // untouched and nothing is delivered as a fragment. Logged, and reported to the + // route, which answers `dropped:true` — the user's paste is gone and a bare 200 + // could not say so. this.deps.log?.( `[RemoteWake] dropped a ${Buffer.byteLength(data)}-byte input chunk for session ${sessionId} — over the ${REMOTE_WAKE_PENDING_MAX_BYTES}-byte wake buffer, and a truncated paste must not be delivered as a fragment` ); - return; + return 'dropped'; } const before = state.pending.reduce((sum, chunk) => sum + Buffer.byteLength(chunk), 0); const after = next.reduce((sum, chunk) => sum + Buffer.byteLength(chunk), 0); @@ -767,6 +769,7 @@ export class RemoteWakeRegistry { this.deps.log?.(`[RemoteWake] pending buffer cap reached for session ${sessionId} — oldest input dropped`); } state.pending = next; + return 'buffered'; } private async _flush(state: WakeState, session: WakeableSession): Promise { @@ -779,7 +782,10 @@ export class RemoteWakeRegistry { // afterwards removed the NEXT chunk instead, so the drop-oldest bookkeeping lost a // chunk that was never written while the log line blamed the one that was. state.pending = state.pending.slice(1); - const ok = await session.writeViaMux(chunk).catch(() => false); + // `fromUser`: these bytes came through the input route as a person's prompt, so + // they may name the tab — without it a session whose FIRST prompt was buffered + // through a wake could never be auto-named. + const ok = await session.writeViaMux(chunk, { fromUser: true }).catch(() => false); if (!ok) { // Drop the rest, and say so. Retaining it looked safer but was worse: the wake // still resolves and marks the host reachable, so the NEXT input takes the diff --git a/src/web/routes/session-routes.ts b/src/web/routes/session-routes.ts index 51628f7c..641ca06c 100644 --- a/src/web/routes/session-routes.ts +++ b/src/web/routes/session-routes.ts @@ -888,6 +888,15 @@ export function registerSessionRoutes( // creation (owned durable sessions) is handled by the dedicated case-create // endpoint below, which #145 consolidated remote-host resolution into. if (body.attachRemoteSession) { + // Remote hosts are admin-only infrastructure everywhere else (the list answers + // `[]` to a non-admin; write and discovery routes are `adminOnly`), and the wake + // below spawns the host's `wakeCommand` or broadcasts a packet. So the gate comes + // FIRST — before the host is even looked up — or an unprivileged account could + // invoke that executable for any configured `hostId` and only then be told the + // workingDir was outside its workspace (reproduced upstream: wake spy fired, 403). + if (isMultiUserMode() && !isAdmin(req)) { + return createErrorResponse(ApiErrorCode.FORBIDDEN, 'Remote hosts are admin-only in multi-user mode'); + } const { hostId, remoteSessionName } = body.attachRemoteSession; const host = (await readRemoteHosts(CODEMAN_CONFIG_DIR)).find((item) => item.id === hostId); if (!host) return createErrorResponse(ApiErrorCode.NOT_FOUND, 'Remote host not found'); @@ -1645,12 +1654,26 @@ export function registerSessionRoutes( if (wantsWait) { // Send-and-wait keeps the response open anyway, so blocking on the wake is // simpler and more correct than buffering (buffering would break the wait). - await remoteWake.ensureAwake(session); - } else if ((await remoteWake.handleInput(session, inputStr)) === 'buffered') { + // A host that never comes back is an error here, as on the create/attach + // paths: writing into the stalled pane would answer `delivered:true` plus a + // timeout, which is the combination the API docs send callers to the wrong + // recovery for. + if (!(await remoteWake.ensureAwake(session))) { + return createErrorResponse( + ApiErrorCode.OPERATION_FAILED, + `${session.remote?.label ?? 'the remote host'} did not come back after a wake-on-LAN request — nothing was sent` + ); + } + } else { + const outcome = await remoteWake.handleInput(session, inputStr); // The registry holds the bytes and flushes them in order once the pane is // reattached. The client's ACK is this 200 — a tagged retry is deduped // (`shouldApplyInput` above already consumed the seq), so nothing is lost. - return {}; + // `buffered` is additive to the historical bare `{}`; `dropped` says the chunk + // was over the wake buffer's cap and is GONE (a 200 with no field could not + // tell delivered from buffered from dropped). + if (outcome === 'buffered') return { buffered: true }; + if (outcome === 'dropped') return { buffered: true, dropped: true }; } } diff --git a/test/remote-wake.test.ts b/test/remote-wake.test.ts index 36fc2da5..43b2f820 100644 --- a/test/remote-wake.test.ts +++ b/test/remote-wake.test.ts @@ -370,6 +370,24 @@ describe('RemoteWakeRegistry', () => { expect(h.events).not.toContain('remote:sessionReconnected'); }); + it('reports an oversized chunk as dropped, and flushes as user input so the tab can be named', async () => { + const h = harness(); + h.probe.mockResolvedValue(false); + let release: (() => void) | undefined; + h.waitUntilReady.mockImplementation(() => new Promise((resolve) => (release = () => resolve(true)))); + await expect(h.registry.handleInput(h.session, 'ok')).resolves.toBe('buffered'); + // Over the cap: never enters the buffer, and the caller is told — a bare 200 could + // not distinguish delivered from buffered from gone. + await expect(h.registry.handleInput(h.session, 'x'.repeat(REMOTE_WAKE_PENDING_MAX_BYTES + 1))).resolves.toBe( + 'dropped' + ); + expect(h.registry.pendingBytes('sess-1')).toBe(2); + release?.(); + await h.registry.wake(h.session); + // `fromUser`: a first prompt that was buffered through a wake may still name the tab. + expect(h.writeViaMux).toHaveBeenCalledWith('ok', { fromUser: true }); + }); + it('drops the buffer when a flush write fails, so nothing is replayed by a later wake', async () => { // Retaining the chunk was the earlier behaviour, and it was worse: the wake still // resolves and marks the host reachable, so the next input takes the deliver path diff --git a/test/routes/session-remote-wake.test.ts b/test/routes/session-remote-wake.test.ts index 017d573d..cf616032 100644 --- a/test/routes/session-remote-wake.test.ts +++ b/test/routes/session-remote-wake.test.ts @@ -10,17 +10,42 @@ * TCP connect, ssh, or WoL happens in CI. */ +import { mkdir, writeFile } from 'node:fs/promises'; +import { join } from 'node:path'; import { afterEach, describe, expect, it, vi } from 'vitest'; +import { getDataDir } from '../../src/config/instance.js'; import fastifyCookie from '@fastify/cookie'; import Fastify, { type FastifyInstance } from 'fastify'; import { registerSessionRoutes, _resetPaneLivenessState } from '../../src/web/routes/session-routes.js'; import { installRouteErrorHandler } from '../../src/web/route-error-handler.js'; import { createMockRouteContext } from '../mocks/index.js'; +import { httpStatusForErrorCode, type ApiErrorCode } from '../../src/types.js'; import { sessionWaits } from '../../src/web/session-wait-registry.js'; import { RemoteWakeRegistry, type RemoteWakeDeps } from '../../src/remote-wake.js'; import type { SessionRemote } from '../../src/types.js'; const SESSION_ID = 'remote-wake-session'; + +/** + * Mirror production's envelope + status mapping (as inbox-routes.test.ts does): a + * returned `createErrorResponse` carries its 4xx, a plain object is wrapped in + * `{success:true, data}`. Without it every error would read as a 200. + */ +function installEnvelope(app: FastifyInstance): void { + app.addHook('preSerialization', (req, reply, payload: unknown, done) => { + if (!req.url.startsWith('/api')) return done(null, payload); + if (payload === null || typeof payload !== 'object') return done(null, payload); + const p = payload as { success?: unknown; errorCode?: unknown }; + if (p.success === false) { + if (reply.statusCode === 200 && typeof p.errorCode === 'string') { + reply.code(httpStatusForErrorCode(p.errorCode as ApiErrorCode)); + } + return done(null, payload); + } + if (p.success === true) return done(null, payload); + return done(null, { success: true, data: payload }); + }); +} const URL = `/api/sessions/${SESSION_ID}/input`; afterEach(() => { @@ -48,9 +73,23 @@ const remoteSession: SessionRemote = { wakeCommand: '/home/joe/bin/whuff', }; -async function harness(opts: { remote?: SessionRemote; hostUp?: boolean; holdWake?: boolean } = {}): Promise { +async function harness( + opts: { + remote?: SessionRemote; + hostUp?: boolean; + holdWake?: boolean; + /** Stands in for the auth middleware (multi-user mode); absent = synthetic admin. */ + authUser?: { username: string; role: 'admin' | 'user' }; + } = {} +): Promise { const app = Fastify({ logger: false }); await app.register(fastifyCookie); + if (opts.authUser) { + const authUser = opts.authUser; + app.addHook('onRequest', async (req) => { + (req as unknown as { authUser: typeof authUser }).authUser = authUser; + }); + } const ctx = createMockRouteContext({ sessionId: SESSION_ID }); const session = ctx.sessions.get(SESSION_ID)!; session.remote = opts.remote ?? remoteSession; @@ -79,6 +118,7 @@ async function harness(opts: { remote?: SessionRemote; hostUp?: boolean; holdWak const registry = new RemoteWakeRegistry(deps); registerSessionRoutes(app, ctx as never, { remoteWake: registry }); + installEnvelope(app); installRouteErrorHandler(app); await app.ready(); return { app, ctx, registry, probe, wake, events, releaseWake: () => release?.() }; @@ -95,7 +135,7 @@ describe('POST /api/sessions/:id/input — wake-on-LAN', () => { const res = await send(h.app, { input: 'hallo', useMux: true }); expect(res.statusCode).toBe(200); - expect(res.json()).toEqual({}); + expect(res.json()).toEqual({ success: true, data: { buffered: true } }); // Nothing reached the pane: writing now would be swallowed by the stalled ssh. expect(session.writeBuffer).toEqual([]); expect(h.wake).toHaveBeenCalledWith({ kind: 'command', command: '/home/joe/bin/whuff' }); @@ -133,7 +173,7 @@ describe('POST /api/sessions/:id/input — wake-on-LAN', () => { const res = await send(h.app, { input: 'hallo', useMux: true }); - expect(res.json()).toEqual({}); + expect(res.json()).toEqual({ success: true, data: {} }); // the historical bare answer, untouched await vi.waitFor(() => expect(session.writeBuffer).toEqual(['hallo'])); expect(h.wake).not.toHaveBeenCalled(); expect(session.reattachRemote).not.toHaveBeenCalled(); @@ -248,3 +288,103 @@ describe('POST /api/sessions/:id/wake', () => { expect(h.wake).not.toHaveBeenCalled(); }); }); + +describe('POST /api/sessions + attachRemoteSession — authorization before the wake', () => { + // Remote hosts are admin-only infra everywhere else, and the attach wake spawns the + // host's `wakeCommand` (or broadcasts a packet). Before this gate a non-admin could + // post an attach for any configured hostId, have that executable run and the request + // held for the wake budget, and only THEN get a 403 for the workingDir (reproduced + // upstream: wake spy fired once, response 403). + it('403s a non-admin in multi-user mode without probing or waking the host', async () => { + const prev = process.env.CODEMAN_MULTIUSER; + process.env.CODEMAN_MULTIUSER = '1'; + try { + // `session-routes.ts` reads hosts from the sandboxed data dir (module-load-time + // constant), so a host with a wake command is written THERE: a regression would + // find it and fire the spy. + await mkdir(getDataDir(), { recursive: true }); + await writeFile( + join(getDataDir(), 'remote-hosts.json'), + JSON.stringify([ + { + id: 'hufflepuff', + label: 'Hufflepuff', + host: '192.168.50.137', + username: 'j', + wakeCommand: '/home/joe/bin/whuff', + }, + ]) + ); + const h = await harness({ hostUp: false, authUser: { username: 'mallory', role: 'user' } }); + const res = await h.app.inject({ + method: 'POST', + url: '/api/sessions', + payload: { attachRemoteSession: { hostId: 'hufflepuff', remoteSessionName: 'codeman-abc12345' } }, + }); + expect(res.statusCode).toBe(403); + expect(res.json().error).toMatch(/admin-only/); + expect(h.probe).not.toHaveBeenCalled(); + expect(h.wake).not.toHaveBeenCalled(); + expect(h.events).toEqual([]); + await h.app.close(); + } finally { + if (prev === undefined) delete process.env.CODEMAN_MULTIUSER; + else process.env.CODEMAN_MULTIUSER = prev; + } + }); +}); + +describe('POST /api/sessions/:id/input — what the caller is told', () => { + it('says buffered, and dropped for a chunk over the wake buffer cap', async () => { + // The non-wait branch always answered a bare `{}`; these fields are additive. Without + // them a prompt over 4 KB posted to a sleeping host was accepted and silently lost. + const h = await harness({ hostUp: false, holdWake: true }); + const small = await send(h.app, { input: 'hallo', useMux: true }); + expect(small.statusCode).toBe(200); + expect(small.json()).toEqual({ success: true, data: { buffered: true } }); + + const big = await send(h.app, { input: 'x'.repeat(5000), useMux: true }); + expect(big.statusCode).toBe(200); + expect(big.json()).toEqual({ success: true, data: { buffered: true, dropped: true } }); + expect(h.registry.pendingBytes(SESSION_ID)).toBe(5); + h.releaseWake(); + await h.registry.wake(h.ctx.sessions.get(SESSION_ID)!); + }); + + it('fails the send-and-wait path when the host never comes back, instead of writing into the stalled pane', async () => { + // Readiness never arrives: the wake resolves false. + const failing = await harnessWithFailingWake(); + const session = failing.ctx.sessions.get(SESSION_ID)!; + const res = await send(failing.app, { input: 'hallo', useMux: true, wait: true, waitTimeout: 1000 }); + expect(res.statusCode).toBe(422); + expect(res.json().errorCode).toBe('OPERATION_FAILED'); + expect(res.json().error).toMatch(/did not come back/); + expect(session.writeBuffer).toEqual([]); + await failing.app.close(); + }); +}); + +/** A harness whose readiness poll answers false: the wake command runs, the host stays down. */ +async function harnessWithFailingWake(): Promise { + const app = Fastify({ logger: false }); + await app.register(fastifyCookie); + const ctx = createMockRouteContext({ sessionId: SESSION_ID }); + ctx.sessions.get(SESSION_ID)!.remote = remoteSession; + const probe = vi.fn(async () => false); + const wake = vi.fn(async () => true); + const events: string[] = []; + const registry = new RemoteWakeRegistry({ + probe, + wake, + waitUntilReady: async () => false, + delay: async () => {}, + noteReconnected: () => {}, + broadcast: (event) => events.push(event), + log: () => {}, + }); + registerSessionRoutes(app, ctx as never, { remoteWake: registry }); + installEnvelope(app); + installRouteErrorHandler(app); + await app.ready(); + return { app, ctx, registry, probe, wake, events, releaseWake: () => {} }; +}