From d39a0ab8d1dc2afea1cc33e131b10598878862d2 Mon Sep 17 00:00:00 2001 From: Yaojia Wang Date: Thu, 30 Jul 2026 11:46:51 +0200 Subject: [PATCH] fix(sessions): tmux -t prefix-matching let orphan ops reach the user's own sessions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round two of adversarial review. The skeptic confirmed the session_activity finding with extra evidence from this host and raised three things the first pass missed. SECURITY — `tmux -t ` resolves exact name, then NAME PREFIX, then fnmatch. The whole module rests on "we only ever touch `web_` + a UUID v4, so a tmux session the user made for their own work is never enumerated and never offered for deletion". Prefix matching goes around that: a session named `web__mine` is refused by parseSessionList (its suffix is not a UUID) and so never appears in the listing — but `-t web_` matches it. So a DELETE aimed at an id that does not exist would end the user's session, and a preview would print its screen. Verified on tmux 3.6a, isolated socket: has-session -t web_ → succeeds against web__MINE has-session -t =web_ → "can't find session" capture-pane -p -t web_ → printed the DECOY's screen capture-pane -p -t =web_: → "can't find session" Two target forms are needed, because `=` only qualifies the session part of a target: session targets (has-session, kill-session) take `=name`, while capture-pane takes a PANE and rejects a bare `=name` outright ("can't find pane"), so it needs `=name:`. Both are now the only way a name reaches tmux, with an integration test that stands up a real decoy and asserts the listing omits it and both the preview and the DELETE report 404 while it stays alive. CORRECTNESS — take max(window_activity, session_activity), not window_activity alone. The previous commit swapped one clock for the other, but they are not ordered: window_activity tracks pane output while an attach with no output bumps only session_activity. On this host one session's session_activity was 4 s NEWER than its window_activity (and another's window_activity was 25 days newer). The question is "has anything happened here", so the answer is the later of the two. Also documents the caveat that window_activity resolves against the session's current window only — exact for the single-window sessions this app creates, and bounded by the max for anything else. CORRECTNESS — parseSessionList accepted an empty numeric field. Number('') is 0 and 0 is finite, so a blank clock parsed as "created at the epoch, idle ever since": instantly eligible for the idle cleanup. Fields must now be actual digits. SAFETY — bulk cleanup re-reads the world immediately before killing. The candidate list is a snapshot and the kill loop takes time, so a session could be attached, or adopted into the table, inside that window. This is the irreversible path; it now verifies each candidate against a fresh listing rather than trusting the snapshot. PERFORMANCE — captureOrphan no longer probes with hasSession first. That was a second tmux spawn per thumbnail, and a SYNCHRONOUS one on the path a whole grid refreshes, buying nothing: capture-pane already fails cleanly on a missing session and we already turn that into null. The guard splits into a pure half (mayActOnOrphan: UUID shape + not in the table) used by both paths, and the existence probe, which only the destructive path needs. Tests: 2213 unit, 9 orphan integration (incl. the decoy regression). Confirmed `has-session -t =` still resolves the 69 real sessions, so the Case 3.5 re-attach path is unaffected. Note: test/integration/server.test.ts "H1 shell state survives a server restart" flakes ~1 run in 3 when all 27 run together on a loaded machine, and passes 4/4 in isolation. That is the pre-existing real-PTY timing flake already recorded in PROGRESS_LOG, not this change — it exercises Case 3.5, which is verified above. --- src/session/manager.ts | 39 +++++++--- src/session/tmux.ts | 95 +++++++++++++++++++----- test/integration/orphan-sessions.test.ts | 36 +++++++++ test/manager-orphans.test.ts | 23 +++++- test/tmux.test.ts | 53 ++++++++++--- 5 files changed, 204 insertions(+), 42 deletions(-) diff --git a/src/session/manager.ts b/src/session/manager.ts index 3f86574..6c3f9f9 100644 --- a/src/session/manager.ts +++ b/src/session/manager.ts @@ -431,12 +431,18 @@ export function createSessionManager( hostile or malformed name reaching `tmux -t`; the second keeps a live session's shell from being killed behind its own PTY's back. */ - /** True when `id` is a real, untracked tmux session of ours that we may act on. */ - function isActionableOrphan(id: string): boolean { + /** The pure half of the guard: is this an id we are allowed to act on at all? + * No tmux call, so it is free to run on a polled path. */ + function mayActOnOrphan(id: string): boolean { if (!cfg.useTmux) return false; if (!SESSION_ID_RE.test(id)) return false; - if (sessions.has(id)) return false; // tracked ⇒ use get()/killById instead - return hasSession(tmuxName(id)); + return !sessions.has(id); // tracked ⇒ use get()/killById instead + } + + /** …plus an existence probe. Only for the DESTRUCTIVE path, which wants to + * distinguish "already gone" (report false) from "killed it". */ + function isActionableOrphan(id: string): boolean { + return mayActOnOrphan(id) && hasSession(tmuxName(id)); } async function listOrphans(): Promise { @@ -454,9 +460,14 @@ export function createSessionManager( })); } - /** The orphan's current screen, or null. Read-only: never attaches. */ + /** The orphan's current screen, or null. Read-only: never attaches. + * + * Deliberately does NOT probe with hasSession first. That was a second tmux spawn + * per thumbnail — and a SYNCHRONOUS one, on the path a grid of cards refreshes — + * buying nothing: `capture-pane` against a missing session already fails cleanly + * and we already turn that into null. */ async function captureOrphan(id: string): Promise { - if (!isActionableOrphan(id)) return null; + if (!mayActOnOrphan(id)) return null; return capturePane(tmuxName(id)); } @@ -476,10 +487,20 @@ export function createSessionManager( * "this server does not track it" is not evidence that it is abandoned. */ async function killOrphansIdleSince(cutoffMs: number): Promise { + const candidates = (await listOrphans()).filter( + (o) => !o.attached && o.lastActivityAt < cutoffMs, + ); + if (candidates.length === 0) return []; + + // Re-read the world immediately before killing. The list above is a snapshot, + // and the loop that follows takes time: a session can be attached, or adopted + // into our table, in that window — and this is the irreversible path, so it + // re-checks rather than trusting the snapshot. + const fresh = new Map((await listOrphans()).map((o) => [o.id, o])); const killed: string[] = []; - for (const o of await listOrphans()) { - if (o.attached) continue; - if (o.lastActivityAt >= cutoffMs) continue; + for (const o of candidates) { + const now = fresh.get(o.id); + if (now === undefined || now.attached) continue; // gone, adopted, or now in use if (killOrphan(o.id)) killed.push(o.id); } return killed; diff --git a/src/session/tmux.ts b/src/session/tmux.ts index d4d23e1..37a386a 100644 --- a/src/session/tmux.ts +++ b/src/session/tmux.ts @@ -28,19 +28,35 @@ const TMUX_TIMEOUT_MS = 3000 /** * Tab-separated, machine-readable. tmux reports both clocks in unix SECONDS. * - * The activity clock is `window_activity`, NOT `session_activity`. That is not a - * detail: `session_activity` tracks CLIENT activity — it advances on attach and on - * keypresses — and never moves for a detached session no matter how much its shell - * is printing. Measured on tmux 3.6a with a detached session in a `while true; do - * echo tick; sleep 1; done` loop: after 6s, session_activity was still equal to - * session_created while window_activity had advanced. Using session_activity as the - * liveness signal would make the idle-cleanup kill precisely the long-running, - * unattended, actively-working sessions this whole feature exists to protect. + * BOTH activity clocks are requested, because neither alone answers "has anything + * happened here" and they are not ordered: + * + * - `session_activity` tracks CLIENT activity — it advances on attach and on + * keypresses, and never moves for a detached session no matter how much its shell + * is printing. Measured on tmux 3.6a with a detached session in a `while true; do + * echo tick; sleep 1; done` loop: after 6 s it still equalled session_created + * while window_activity had advanced. Using it alone as the liveness signal makes + * idle-cleanup kill precisely the long-running, unattended, actively-working + * sessions this feature exists to protect — on one real host, 3 of them. + * - `window_activity` tracks pane OUTPUT, but an attach with no output bumps only + * session_activity. Observed on the same host: one session whose session_activity + * was 4 s newer than its window_activity, another whose window_activity was 25 + * days newer. + * + * So the parser takes max(window_activity, session_activity). + * + * CAVEAT: in `list-sessions`, `#{window_activity}` resolves against the session's + * CURRENT window only. Every session this app creates is single-window (plain + * `new-session`), so it is exact here; a user who opens a second window inside one + * (Ctrl-b c) and leaves output only in a background window could still read as + * quieter than it is. Taking the max limits that to "as stale as session_activity", + * never staler. */ const LIST_FORMAT = [ '#{session_name}', '#{session_created}', '#{window_activity}', + '#{session_activity}', '#{session_attached}', '#{window_width}', '#{window_height}', @@ -78,10 +94,38 @@ export function tmuxName(sessionId: string): string { return `web_${sessionId}` } +/** + * A tmux `-t` target that matches ONE session by exact name. + * + * Without the `=`, tmux resolves a target as exact name, then NAME PREFIX, then + * fnmatch. That is a hole in this module's central promise: a session named + * `web__mine` is refused by parseSessionList (its suffix is not a UUID) and so + * is never listed — but `-t web_` prefix-matches it, so a kill aimed at a + * non-existent id would end the user's own session instead. Verified on tmux 3.6a: + * `has-session -t web_` succeeds against `web__MINE`, and fails with the + * `=` prefix. Every -t in this file goes through here. + */ +function exact(name: string): string { + return `=${name}` +} + +/** + * The same, for a target-PANE. `capture-pane` takes a pane, and tmux rejects a bare + * `=name` there outright ("can't find pane: =web_x") — the `=` only qualifies the + * session part of a target, so the session has to be named as such with a trailing + * `:`. Verified on tmux 3.6a against a decoy named `web__decoy`: with the real + * session gone, `-t web_` printed the DECOY's screen while `-t =web_:` + * failed with "can't find session". Without this the preview leaks the contents of a + * session the user created for their own work. + */ +function exactPane(name: string): string { + return `=${name}:` +} + /** Does a tmux session with this name already exist (e.g. after a restart)? */ export function hasSession(name: string): boolean { try { - execFileSync('tmux', ['has-session', '-t', name], { + execFileSync('tmux', ['has-session', '-t', exact(name)], { stdio: 'ignore', timeout: TMUX_TIMEOUT_MS, }) @@ -94,7 +138,7 @@ export function hasSession(name: string): boolean { /** Kill a tmux session (ends the shell). No-op if it's already gone. */ export function killSession(name: string): void { try { - execFileSync('tmux', ['kill-session', '-t', name], { + execFileSync('tmux', ['kill-session', '-t', exact(name)], { stdio: 'ignore', timeout: TMUX_TIMEOUT_MS, }) @@ -120,9 +164,10 @@ export function parseSessionList(stdout: string): TmuxSessionSummary[] { for (const line of stdout.split('\n')) { if (line === '') continue const parts = line.split('\t') - if (parts.length < 6) continue + if (parts.length < 7) continue - const [name, created, activity, attached, width, height] = parts as [ + const [name, created, winActivity, sessActivity, attached, width, height] = parts as [ + string, string, string, string, @@ -134,24 +179,36 @@ export function parseSessionList(stdout: string): TmuxSessionSummary[] { const id = name.slice('web_'.length) if (!SESSION_ID_RE.test(id)) continue - const createdSec = Number(created) - const activitySec = Number(activity) - const cols = Number(width) - const rows_ = Number(height) + // Number('') is 0, which IS finite — so an empty field would sail through as + // "created at the epoch, idle ever since", i.e. instantly eligible for the idle + // cleanup. Require actual digits. + const num = (v: string): number => (/^\d+$/.test(v) ? Number(v) : NaN) + const createdSec = num(created) + const winSec = num(winActivity) + const sessSec = num(sessActivity) + const cols = num(width) + const rows_ = num(height) if ( !Number.isFinite(createdSec) || - !Number.isFinite(activitySec) || + !Number.isFinite(winSec) || + !Number.isFinite(sessSec) || !Number.isFinite(cols) || !Number.isFinite(rows_) ) { continue } + // The two clocks are NOT ordered: window_activity tracks pane output, while an + // attach bumps session_activity without producing any. Observed on a real host: + // one session with session_activity 4s NEWER than window_activity, another with + // window_activity 25 DAYS newer. The question being asked is "has anything + // happened here", so take whichever is later. + const activitySec = Math.max(winSec, sessSec) rows.push({ id, createdAtMs: createdSec * 1000, lastActivityAtMs: activitySec * 1000, - attached: attached !== '0' && attached !== '', + attached: attached !== '0', cols, rows: rows_, }) @@ -192,7 +249,7 @@ export async function listSessions(): Promise { */ export async function capturePane(name: string): Promise { try { - const { stdout } = await execFileAsync('tmux', ['capture-pane', '-p', '-e', '-t', name], { + const { stdout } = await execFileAsync('tmux', ['capture-pane', '-p', '-e', '-t', exactPane(name)], { encoding: 'utf8', maxBuffer: CAPTURE_MAX_BUFFER, timeout: TMUX_TIMEOUT_MS, diff --git a/test/integration/orphan-sessions.test.ts b/test/integration/orphan-sessions.test.ts index 5d5aa6f..afb8815 100644 --- a/test/integration/orphan-sessions.test.ts +++ b/test/integration/orphan-sessions.test.ts @@ -187,6 +187,42 @@ describe('orphan sessions — tmux enabled', () => { ).toContain(name) }) + itTmux('cannot reach a look-alike session by tmux prefix matching', async () => { + // tmux -t resolves exact name, then NAME PREFIX, then fnmatch. `web__mine` + // is refused by parseSessionList (suffix is not a UUID) and so is never listed — + // but a bare `-t web_` prefix-matches it, which would let a DELETE aimed at + // a non-existent id end a session the user created for their own work. + const decoyId = randomUUID() + const decoy = `web_${decoyId}_mine` + execFileSync('tmux', ['new-session', '-d', '-s', decoy, 'sleep 300'], { stdio: 'ignore' }) + try { + // It must not appear in the listing… + const list = (await (await fetch(`${origin()}/orphan-sessions`)).json()) as OrphanSessionInfo[] + expect(list.some((o) => o.id === decoyId)).toBe(false) + + // …and the exact id must be reported as absent, not silently matched to it. + const preview = await fetch(`${origin()}/orphan-sessions/${decoyId}/preview`) + expect(preview.status).toBe(404) + + const del = await fetch(`${origin()}/orphan-sessions/${decoyId}`, { + method: 'DELETE', + headers: { Origin: origin() }, + }) + expect(del.status).toBe(404) + + // The decoy is still alive — that is the whole point. + expect( + execFileSync('tmux', ['ls', '-F', '#{session_name}'], { encoding: 'utf8' }), + ).toContain(decoy) + } finally { + try { + execFileSync('tmux', ['kill-session', '-t', `=${decoy}`], { stdio: 'ignore' }) + } catch { + // already gone + } + } + }) + itTmux('kills it, and 404s the second time', async () => { const first = await fetch(`${origin()}/orphan-sessions/${id}`, { method: 'DELETE', diff --git a/test/manager-orphans.test.ts b/test/manager-orphans.test.ts index 928c07f..a91dfa6 100644 --- a/test/manager-orphans.test.ts +++ b/test/manager-orphans.test.ts @@ -177,12 +177,16 @@ describe('captureOrphan', () => { expect(tmuxCapture).not.toHaveBeenCalled(); }); - it('refuses when no such tmux session exists', async () => { + it('returns null when the session is gone, without a second probe spawn', async () => { + // No hasSession pre-check here on purpose: it was a second tmux spawn per + // thumbnail — a SYNCHRONOUS one, on the polled grid-refresh path — and + // capture-pane already fails cleanly on a missing session. tmuxHas.mockReturnValue(false); + tmuxCapture.mockResolvedValue(null); const mgr = createSessionManager(CFG); expect(await mgr.captureOrphan(U1)).toBeNull(); - expect(tmuxCapture).not.toHaveBeenCalled(); + expect(tmuxHas).not.toHaveBeenCalled(); }); it('refuses an id the table owns — a live session previews from its ring buffer', async () => { @@ -250,6 +254,21 @@ describe('killOrphansIdleSince', () => { expect(tmuxKill).toHaveBeenCalledTimes(1); }); + it('re-checks attachment right before killing, not just in the snapshot', async () => { + // The candidate list is a snapshot and the kill loop takes time; a session can be + // attached in that window. This is the irreversible path, so it re-reads. + let call = 0; + tmuxList.mockImplementation(async () => { + call += 1; + // first read: idle and unattached → a candidate. second read: now attached. + return [summary(U1, 1_000, call > 1)]; + }); + const mgr = createSessionManager(CFG); + + expect(await mgr.killOrphansIdleSince(5_000)).toEqual([]); + expect(tmuxKill).not.toHaveBeenCalled(); + }); + it('never kills a session someone is attached to from a terminal', async () => { // "Untracked by this server" is not "abandoned" — a plain `tmux attach` or a // second server process both show up as attached, and bulk cleanup must not diff --git a/test/tmux.test.ts b/test/tmux.test.ts index 626dd90..a4f2eaf 100644 --- a/test/tmux.test.ts +++ b/test/tmux.test.ts @@ -54,7 +54,7 @@ describe('hasSession', () => { expect(hasSession('web_x')).toBe(true) expect(mockExec).toHaveBeenCalledWith( 'tmux', - ['has-session', '-t', 'web_x'], + ['has-session', '-t', '=web_x'], expect.objectContaining({ stdio: 'ignore' }), ) }) @@ -68,12 +68,12 @@ describe('hasSession', () => { }) describe('killSession', () => { - it('invokes tmux kill-session for the name', () => { + it('targets the EXACT name so it cannot prefix-match another session', () => { mockExec.mockReturnValue(Buffer.from('')) killSession('web_x') expect(mockExec).toHaveBeenCalledWith( 'tmux', - ['kill-session', '-t', 'web_x'], + ['kill-session', '-t', '=web_x'], expect.objectContaining({ stdio: 'ignore' }), ) }) @@ -93,7 +93,7 @@ const U2 = '3a63f3c7-acfe-4081-992f-c4dbd55b3d6b' describe('parseSessionList', () => { it('parses tmux rows into summaries, converting seconds to ms', () => { - const out = `web_${U1}\t1783911991\t1783911992\t0\t80\t23\n` + const out = `web_${U1}\t1783911991\t1783911992\t1783911990\t0\t80\t23\n` expect(parseSessionList(out)).toEqual([ { id: U1, @@ -107,32 +107,53 @@ describe('parseSessionList', () => { }) it('sorts most-recently-active first', () => { - const out = [`web_${U1}\t100\t100\t0\t80\t24`, `web_${U2}\t100\t900\t1\t80\t24`].join('\n') + const out = [`web_${U1}\t100\t100\t100\t0\t80\t24`, `web_${U2}\t100\t900\t100\t1\t80\t24`].join('\n') expect(parseSessionList(out).map((s) => s.id)).toEqual([U2, U1]) }) it('reports a session attached from a real terminal', () => { - const out = `web_${U1}\t100\t100\t1\t80\t24` + const out = `web_${U1}\t100\t100\t100\t1\t80\t24` expect(parseSessionList(out)[0]!.attached).toBe(true) }) it('ignores sessions that are not ours — never touch the user’s own tmux', () => { - const out = ['mywork\t100\t100\t0\t80\t24', `web_${U1}\t100\t100\t0\t80\t24`].join('\n') + const out = ['mywork\t100\t100\t100\t0\t80\t24', `web_${U1}\t100\t100\t100\t0\t80\t24`].join('\n') expect(parseSessionList(out).map((s) => s.id)).toEqual([U1]) }) it('ignores a web_-prefixed name whose suffix is not a UUID v4', () => { // Only ids the protocol could have produced are actionable: anything else is // someone else's session that merely starts with web_. - const out = ['web_../../etc\t100\t100\t0\t80\t24', 'web_-t\t100\t100\t0\t80\t24'].join('\n') + const out = ['web_../../etc\t100\t100\t100\t0\t80\t24', 'web_-t\t100\t100\t100\t0\t80\t24'].join('\n') expect(parseSessionList(out)).toEqual([]) }) it('skips malformed and blank rows instead of throwing', () => { - const out = ['', 'garbage', `web_${U1}\tNaN\t100\t0\t80\t24`, `web_${U2}\t100\t100\t0\t80\t24`].join('\n') + const out = [ + '', + 'garbage', + `web_${U1}\tNaN\t100\t100\t0\t80\t24`, + `web_${U2}\t100\t100\t100\t0\t80\t24`, + ].join('\n') expect(parseSessionList(out).map((s) => s.id)).toEqual([U2]) }) + it('rejects an EMPTY numeric field instead of reading it as 0', () => { + // Number('') is 0 and 0 is finite, so a blank clock would parse as "created at + // the epoch, idle ever since" — instantly eligible for the idle cleanup. + const out = `web_${U1}\t\t100\t100\t0\t80\t24` + expect(parseSessionList(out)).toEqual([]) + }) + + it('takes the NEWER of the two activity clocks', () => { + // They are not ordered: window_activity tracks pane output, session_activity is + // bumped by an attach that produces none. "Has anything happened" is the max. + const winNewer = `web_${U1}\t100\t900\t100\t0\t80\t24` + const sessNewer = `web_${U2}\t100\t100\t900\t0\t80\t24` + expect(parseSessionList(winNewer)[0]!.lastActivityAtMs).toBe(900_000) + expect(parseSessionList(sessNewer)[0]!.lastActivityAtMs).toBe(900_000) + }) + it('returns an empty list for empty output (no tmux server)', () => { expect(parseSessionList('')).toEqual([]) }) @@ -140,7 +161,7 @@ describe('parseSessionList', () => { describe('listSessions', () => { it('asks tmux for a machine-readable list', async () => { - mockExecAsync.mockResolvedValue({ stdout: `web_${U1}\t100\t100\t0\t80\t24\n`, stderr: '' }) + mockExecAsync.mockResolvedValue({ stdout: `web_${U1}\t100\t100\t100\t0\t80\t24\n`, stderr: '' }) expect((await listSessions()).map((s) => s.id)).toEqual([U1]) const [file, args] = mockExecAsync.mock.calls[0] as [string, string[]] expect(file).toBe('tmux') @@ -157,7 +178,9 @@ describe('listSessions', () => { const [, args] = mockExecAsync.mock.calls[0] as [string, string[]] const fmt = args[args.indexOf('-F') + 1]! expect(fmt).toContain('#{window_activity}') - expect(fmt).not.toContain('#{session_activity}') + // session_activity is also requested — the parser takes the max — but + // window_activity is the one that must never be missing. + expect(fmt).toContain('#{session_activity}') }) it('bounds the call so a wedged tmux cannot hang the server', async () => { @@ -183,7 +206,13 @@ describe('capturePane', () => { expect(args[0]).toBe('capture-pane') expect(args).toContain('-p') // print to stdout — read-only, no client expect(args).toContain('-e') // keep escape sequences so colour survives - expect(args).toContain('web_x') + // '=web_x:' forces an EXACT session match for a target-PANE. A bare target + // prefix-matches, which leaks the screen of a session parseSessionList + // deliberately refuses to enumerate; a plain '=web_x' is rejected by tmux + // outright ("can't find pane"), because '=' only qualifies the session part. + expect(args).toContain('=web_x:') + expect(args).not.toContain('web_x') + expect(args).not.toContain('=web_x') // Attaching would resize the session and SIGWINCH whatever is running in it. expect(args).not.toContain('attach-session') })