mirror of
https://github.com/stablyai/orca.git
synced 2026-10-22 08:03:37 +00:00
fix(ssh): fence a recycled relay PTY id on the shell's own identity
A reset relay restarts its ids at pty-1, so an id alone can name somebody else's shell: a pane still bound to pty-1 could attach to a shell another pane was already driving, and the two would share keystrokes and output. The guard that used to catch this compared the paneKey and tabId frozen at spawn. It was removed for good reason — a pane moved to another tab was refused its own live shell, and refused as "not found", which read as death and resumed the agent a second time. So it traded a cross-attach for a double resume. The incarnation is the shell's OWN identity, so it discriminates a recycled id without caring where the pane lives: both failures close at once. The relay refuses a mismatch and is deliberately not worded "not found" — that phrasing is what the client maps to an expired session, and expiry authorizes a respawn onto a shell this branch just proved is alive. Only a host-attested expectation is sent. The locally synthesized stand-in is not stable across reconnects and would refuse a pane its own shell. An older relay ignores the field and an older client sends none, so both stay permissive. The tab-keyed comparator is deleted rather than left dormant, and its suite is inverted onto the new identity: a recycled id is still refused, and a pane that moved tabs now attaches instead of being told its shell is gone.
This commit is contained in:
@@ -109,7 +109,9 @@ describe('a genuine absence is still a death', () => {
|
||||
// for a new shell, and in exactly that case the pane and tab both still match,
|
||||
// so it accepted the wrong shell anyway.
|
||||
describe('reattach does not ask the relay to police pane identity', () => {
|
||||
async function attachParams(): Promise<Record<string, unknown>> {
|
||||
async function attachParams(
|
||||
extraOptions: Record<string, unknown> = {}
|
||||
): Promise<Record<string, unknown>> {
|
||||
const request = vi.fn().mockRejectedValue(new Error('boom'))
|
||||
try {
|
||||
await reattachSshPtySession({
|
||||
@@ -121,7 +123,8 @@ describe('reattach does not ask the relay to police pane identity', () => {
|
||||
rows: 24,
|
||||
paneKey: 'tab-new:leaf-1',
|
||||
tabId: 'tab-new',
|
||||
env: { ORCA_PANE_KEY: 'tab-old:leaf-1', ORCA_TAB_ID: 'tab-old' }
|
||||
env: { ORCA_PANE_KEY: 'tab-old:leaf-1', ORCA_TAB_ID: 'tab-old' },
|
||||
...extraOptions
|
||||
} as never
|
||||
})
|
||||
} catch {
|
||||
@@ -138,4 +141,26 @@ describe('reattach does not ask the relay to police pane identity', () => {
|
||||
it('sends no expected tab id', async () => {
|
||||
expect(await attachParams()).not.toHaveProperty('expectedTabId')
|
||||
})
|
||||
|
||||
// The producer pin for the relay's incarnation guard. The guard is only worth having if the
|
||||
// expectation actually leaves the client, and nothing else here would notice if it stopped:
|
||||
// the relay stays permissive on an absent field, so a silent regression reads as "all green".
|
||||
it('sends the expected incarnation when the host attested one', async () => {
|
||||
expect(await attachParams({ expectedIncarnationId: 'inc-host-7' })).toMatchObject({
|
||||
expectedIncarnationId: 'inc-host-7'
|
||||
})
|
||||
})
|
||||
|
||||
// A stand-in minted locally when the host reported none. It is first-write-wins and is dropped
|
||||
// when provider state resets, so the same live shell can present a different one after a
|
||||
// reconnect — sending it would make the relay refuse the pane its own shell.
|
||||
it('sends no expected incarnation when the value was synthesized locally', async () => {
|
||||
expect(await attachParams({ expectedIncarnationId: 'legacy:23:0:pty-7' })).not.toHaveProperty(
|
||||
'expectedIncarnationId'
|
||||
)
|
||||
})
|
||||
|
||||
it('sends no expected incarnation when there is none to send', async () => {
|
||||
expect(await attachParams()).not.toHaveProperty('expectedIncarnationId')
|
||||
})
|
||||
})
|
||||
|
||||
@@ -1,5 +1,9 @@
|
||||
import type { SshChannelMultiplexer } from '../ssh/ssh-channel-multiplexer'
|
||||
import { isPtyIncarnationId, type PtyIncarnationId } from '../../shared/pty-incarnation'
|
||||
import {
|
||||
isPtyIncarnationId,
|
||||
isRelayAttestedPtyIncarnationId,
|
||||
type PtyIncarnationId
|
||||
} from '../../shared/pty-incarnation'
|
||||
import {
|
||||
SSH_PTY_IDENTITY_MISMATCH_ERROR,
|
||||
SSH_SESSION_EXPIRED_ERROR,
|
||||
@@ -197,7 +201,14 @@ export async function reattachSshPtySession(args: {
|
||||
id: relaySessionId,
|
||||
cols: args.options.cols,
|
||||
rows: args.options.rows,
|
||||
suppressReplayNotification: true
|
||||
suppressReplayNotification: true,
|
||||
// The shell's own identity, so it survives a pane moving between tabs — unlike the pane
|
||||
// identity this replaced. Sent only when the host attested it: a locally synthesized
|
||||
// stand-in is not stable across reconnects and would refuse the pane its own shell. An
|
||||
// older relay ignores the field, which leaves today's permissive behaviour.
|
||||
...(isRelayAttestedPtyIncarnationId(args.options.expectedIncarnationId)
|
||||
? { expectedIncarnationId: args.options.expectedIncarnationId }
|
||||
: {})
|
||||
},
|
||||
installSourceActivation: args.installSourceActivation,
|
||||
rememberPtyIncarnation: args.rememberPtyIncarnation
|
||||
|
||||
@@ -42,7 +42,6 @@ import {
|
||||
IMMEDIATE_PTY_EXIT_TIMEOUT_MS,
|
||||
MAX_RELAY_PTY_SESSIONS,
|
||||
PtyHandler,
|
||||
attachIdentityMismatches,
|
||||
formatNodePtyUnavailableMessage
|
||||
} from './pty-handler'
|
||||
import { RelayDispatcher } from './dispatcher'
|
||||
@@ -1168,6 +1167,68 @@ describe('PtyHandler', () => {
|
||||
|
||||
expect(mockKill).toHaveBeenCalledWith('SIGKILL')
|
||||
expect(handler.activePtyCount).toBe(0)
|
||||
})
|
||||
|
||||
// INVERTED from the deleted `attachIdentityMismatches` suite. That guard compared the paneKey and
|
||||
// tabId frozen at spawn; it caught a recycled id, but it also refused a pane that had merely moved
|
||||
// to another tab — and refused as "not found", which the client read as death and answered by
|
||||
// resuming the agent a second time. The property worth keeping is "a recycled id must not attach";
|
||||
// the property that had to go is "identity is where the pane lives". Both are pinned below, now
|
||||
// against the shell's own incarnation.
|
||||
it('refuses an attach whose expected incarnation names a different shell', async () => {
|
||||
const first = await spawnPty({})
|
||||
|
||||
await expect(
|
||||
dispatcher.callRequest('pty.attach', {
|
||||
id: first.id,
|
||||
expectedIncarnationId: 'incarnation-from-a-previous-relay'
|
||||
})
|
||||
).rejects.toThrow(/identity mismatch/i)
|
||||
})
|
||||
|
||||
// The wording matters as much as the refusal: "not found" is what the client maps to an expired
|
||||
// session, and expiry authorizes a respawn onto the very shell this branch proved is alive.
|
||||
it('does not word an incarnation refusal as not-found', async () => {
|
||||
const first = await spawnPty({})
|
||||
|
||||
await expect(
|
||||
dispatcher.callRequest('pty.attach', {
|
||||
id: first.id,
|
||||
expectedIncarnationId: 'incarnation-from-a-previous-relay'
|
||||
})
|
||||
).rejects.toThrow(expect.not.stringMatching(/not found/i))
|
||||
})
|
||||
|
||||
it('attaches when the expected incarnation is the shell it names', async () => {
|
||||
const first = await spawnPty({})
|
||||
|
||||
await expect(
|
||||
attachPty({ id: first.id, expectedIncarnationId: first.incarnationId })
|
||||
).resolves.toMatchObject({ incarnationId: first.incarnationId })
|
||||
})
|
||||
|
||||
// An older client sends no expectation. Refusing there would strand every pane it owns.
|
||||
it('stays permissive when the caller supplies no expected incarnation', async () => {
|
||||
const first = await spawnPty({})
|
||||
|
||||
await expect(attachPty({ id: first.id })).resolves.toMatchObject({
|
||||
incarnationId: first.incarnationId
|
||||
})
|
||||
})
|
||||
|
||||
// The regression the old tab-keyed guard caused. A pane carries its shell to a new tab; the
|
||||
// relay still holds the paneKey/tabId frozen at spawn. That must not refuse the attach.
|
||||
it('attaches a pane that has moved to another tab', async () => {
|
||||
const first = await spawnPty({ paneKey: 'tab-a:0', tabId: 'tab-a' })
|
||||
|
||||
await expect(
|
||||
attachPty({
|
||||
id: first.id,
|
||||
expectedIncarnationId: first.incarnationId,
|
||||
expectedPaneKey: 'tab-b:0',
|
||||
expectedTabId: 'tab-b'
|
||||
})
|
||||
).resolves.toMatchObject({ incarnationId: first.incarnationId })
|
||||
expect(vi.getTimerCount()).toBe(0)
|
||||
})
|
||||
|
||||
@@ -1876,13 +1937,19 @@ describe('PtyHandler', () => {
|
||||
expect(spawnOptions.env.ORCA_PANE_KEY).toBeUndefined()
|
||||
expect(spawnOptions.env.ORCA_TAB_ID).toBeUndefined()
|
||||
|
||||
// INVERTED: this used to require a differing pane identity to be REFUSED. That refusal is the
|
||||
// defect — the paneKey and tabId are frozen at spawn, so it also refused a pane that had merely
|
||||
// moved tabs, and it refused as "not found", which read as death and resumed the agent twice.
|
||||
// The metadata is still recorded and still kept out of the shell env (asserted above); it just
|
||||
// no longer gates an attach. A recycled id is rejected on the incarnation instead, which is
|
||||
// pinned separately.
|
||||
await expect(
|
||||
attachPty({
|
||||
id: 'pty-1',
|
||||
expectedPaneKey: 'tab-b:leaf-b',
|
||||
expectedTabId: 'tab-b'
|
||||
})
|
||||
).rejects.toThrow('PTY "pty-1" not found')
|
||||
).resolves.toEqual({ incarnationId: spawn.incarnationId })
|
||||
|
||||
await expect(
|
||||
attachPty({
|
||||
@@ -3116,13 +3183,17 @@ describe('PtyHandler', () => {
|
||||
expect(callArgs.env.ORCA_PANE_KEY).toBeUndefined()
|
||||
expect(callArgs.env.ORCA_TAB_ID).toBeUndefined()
|
||||
|
||||
// INVERTED, same reason as the spawn-side clause: the revived PTY still carries the pane
|
||||
// identity it was serialized with, and that identity no longer gates an attach. What the
|
||||
// clause still proves is that revive PRESERVES the metadata and keeps it out of the shell env
|
||||
// (asserted above) — it just may not refuse a pane for living somewhere else now.
|
||||
await expect(
|
||||
dispatcher.callRequest('pty.attach', {
|
||||
id: 'pty-1',
|
||||
expectedPaneKey: 'tab-other:leaf',
|
||||
expectedTabId: 'tab-other'
|
||||
})
|
||||
).rejects.toThrow('PTY "pty-1" not found')
|
||||
).resolves.toMatchObject({ incarnationId: expect.any(String) })
|
||||
})
|
||||
|
||||
it('invokes the exit listener with the spawn-time paneKey', async () => {
|
||||
@@ -3593,39 +3664,3 @@ describe('PtyHandler', () => {
|
||||
expect(handler.pendingPtyCreationCount).toBe(0)
|
||||
})
|
||||
})
|
||||
|
||||
describe('attachIdentityMismatches', () => {
|
||||
it('rejects a paneKey collision across relay generations', () => {
|
||||
// Old lease expects tab-a's pane; the reset relay's pty-1 belongs to tab-b.
|
||||
expect(
|
||||
attachIdentityMismatches({ paneKey: 'tab-a:0' }, { paneKey: 'tab-b:0', tabId: 'tab-b' })
|
||||
).toBe(true)
|
||||
})
|
||||
|
||||
it('rejects a tabId collision when only tab identity is known', () => {
|
||||
expect(attachIdentityMismatches({ tabId: 'tab-a' }, { tabId: 'tab-b' })).toBe(true)
|
||||
})
|
||||
|
||||
it('accepts a matching identity', () => {
|
||||
expect(
|
||||
attachIdentityMismatches(
|
||||
{ paneKey: 'tab-a:0', tabId: 'tab-a' },
|
||||
{ paneKey: 'tab-a:0', tabId: 'tab-a' }
|
||||
)
|
||||
).toBe(false)
|
||||
})
|
||||
|
||||
it('stays permissive when the caller supplies no identity', () => {
|
||||
expect(attachIdentityMismatches({}, { paneKey: 'tab-a:0', tabId: 'tab-a' })).toBe(false)
|
||||
})
|
||||
|
||||
it('stays permissive when the managed PTY predates identity capture', () => {
|
||||
expect(attachIdentityMismatches({ paneKey: 'tab-a:0', tabId: 'tab-a' }, {})).toBe(false)
|
||||
})
|
||||
|
||||
it('does not reject on tabId when paneKey matches (split panes share a tab)', () => {
|
||||
expect(
|
||||
attachIdentityMismatches({ paneKey: 'tab-a:1' }, { paneKey: 'tab-a:1', tabId: 'tab-a' })
|
||||
).toBe(false)
|
||||
})
|
||||
})
|
||||
|
||||
+19
-21
@@ -329,17 +329,13 @@ export type PtyExitListener = (event: { id: string; paneKey?: string }) => void
|
||||
|
||||
type PtyIdentity = { paneKey?: string; tabId?: string }
|
||||
|
||||
/**
|
||||
* True when a reattach's expected pane identity contradicts the target PTY's own.
|
||||
* Rejects cross-relay-generation id collisions (a reset relay reuses `pty-N`).
|
||||
* Only compares fields present on both sides; absent identity stays permissive.
|
||||
*/
|
||||
export function attachIdentityMismatches(expected: PtyIdentity, managed: PtyIdentity): boolean {
|
||||
return Boolean(
|
||||
(expected.paneKey && managed.paneKey && expected.paneKey !== managed.paneKey) ||
|
||||
(expected.tabId && managed.tabId && expected.tabId !== managed.tabId)
|
||||
)
|
||||
}
|
||||
/* Why there is no attach-time pane-identity comparison here any more: it keyed on the paneKey and
|
||||
* tabId frozen at spawn, so moving a pane to another tab made the relay refuse its own live shell,
|
||||
* and refuse by saying "not found" — which the client read as death and answered by resuming the
|
||||
* agent a second time. Cross-generation id collisions, the thing it was actually for, are rejected
|
||||
* by the incarnation comparison in `attach` instead: that is the shell's own identity, so it
|
||||
* follows the pane wherever it moves. `attachIdentity` is still recorded and still round-trips
|
||||
* through serialize/revive; it just no longer gates an attach. */
|
||||
/** Returns env to merge into the PTY's spawn env. Receives spawn context so augmenters can derive per-PTY identity from paneKey.
|
||||
* `command` is the renderer-chosen agent launch command (`pi`, `omp`, …); undefined for CLI-launched bare shells. */
|
||||
export type PtyEnvAugmenter = (ctx: {
|
||||
@@ -1611,16 +1607,18 @@ export class PtyHandler {
|
||||
throw new Error(`PTY "${id}" not found`)
|
||||
}
|
||||
|
||||
// Why: generation resets can reuse PTY IDs; reject conflicting identities.
|
||||
const mismatch = attachIdentityMismatches(
|
||||
{
|
||||
paneKey: typeof params.expectedPaneKey === 'string' ? params.expectedPaneKey : undefined,
|
||||
tabId: typeof params.expectedTabId === 'string' ? params.expectedTabId : undefined
|
||||
},
|
||||
managed.attachIdentity ?? { paneKey: managed.paneKey, tabId: managed.tabId }
|
||||
)
|
||||
if (mismatch) {
|
||||
throw new Error(`PTY "${id}" not found (identity mismatch)`)
|
||||
// Why: a reset relay restarts its ids at pty-1, so an id alone can name somebody else's shell.
|
||||
// The incarnation is the shell's OWN identity, so it discriminates a recycled id without
|
||||
// caring where the pane lives. The pane identity this replaced froze the TAB at spawn, so a
|
||||
// pane moved to another tab was refused — and refused as "not found", which read as death and
|
||||
// resumed the agent a second time. Absent expectation stays permissive: an older client sends
|
||||
// none, and refusing there would strand every pane it owns.
|
||||
const expectedIncarnationId =
|
||||
typeof params.expectedIncarnationId === 'string' ? params.expectedIncarnationId : undefined
|
||||
if (expectedIncarnationId && expectedIncarnationId !== managed.incarnationId) {
|
||||
// Deliberately NOT worded "not found": that phrasing is what the client maps to an expired
|
||||
// session, and expiry authorizes a respawn onto a shell this branch just proved is alive.
|
||||
throw new Error(`PTY "${id}" identity mismatch`)
|
||||
}
|
||||
|
||||
managed.startupIngress?.snapshotBarrier()
|
||||
|
||||
Reference in New Issue
Block a user