fix(sessions): tmux -t prefix-matching let orphan ops reach the user's own sessions

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 <name>` 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_<uuid>_mine` is refused by parseSessionList (its suffix is not a UUID) and so never appears in the listing — but `-t web_<uuid>` 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_<uuid> → succeeds against web_<uuid>_MINE has-session -t =web_<uuid> → "can't find session" capture-pane -p -t web_<uuid> → printed the DECOY's screen capture-pane -p -t =web_<uuid>: → "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 =<name>` 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.
2026-07-30 11:46:51 +02:00
parent 2bfc76b397
commit d39a0ab8d1
5 changed files with 204 additions and 42 deletions
--- 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<OrphanSessionInfo[]> {
@@ -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<string | null> {
-    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<string[]> {
+    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;
--- 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_<uuid>_mine` is refused by parseSessionList (its suffix is not a UUID) and so
+ * is never listed — but `-t web_<uuid>` 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_<uuid>` succeeds against `web_<uuid>_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_<uuid>_decoy`: with the real
+ * session gone, `-t web_<uuid>` printed the DECOY's screen while `-t =web_<uuid>:`
+ * 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<TmuxSessionSummary[]> {
 */
 export async function capturePane(name: string): Promise<string | null> {
  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,
--- 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_<uuid>_mine`
+    // is refused by parseSessionList (suffix is not a UUID) and so is never listed —
+    // but a bare `-t web_<uuid>` 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',
--- 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
--- 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')
  })