From 98dec88254822263ebd70d522e1f68e42a95e690 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Thu, 13 Aug 2026 22:31:58 -0700 Subject: [PATCH] fix(terminal): pin shim tombstone shell state and directory identity Three fixes, each proven before and after. Delayed expansion: bare setlocal inherits the caller's state. Under a parent shell started with /V:ON, a literal !CD! PATH entry became the current directory and a planted git.cmd ran (exit 66), and a legitimate directory containing ! stopped resolving (exit 127). Both on Windows 11, both gone with setlocal DisableDelayedExpansion. Directory identity: the POSIX filter compared the legacy shim directory lexically while comparing its own directory with -ef, so a symlink or a /../ spelling escaped the filter and the live attribution wrapper won the lookup. It now tests both. The env scrub had the same gap and now normalizes before its suffix test. Retained POSIX wrappers: with no absolute bash verifiable the wrapper was deleted, which strands a shell that already hashed the path on 127 instead of falling through to PATH. It now reuses the shebang of the wrapper it replaces, which is known to work on that host, and rejects /usr/bin/env so the ambient lookup stays closed. Deleting is the last resort. Two Windows test pins matched the wrong occurrence and stayed green with the guard they claimed to protect removed; they now assert the subroutine body. All five fixes were mutation-tested. Co-authored-by: Orca --- .../pty/legacy-terminal-posix-tombstone.ts | 38 +++++++- src/main/pty/legacy-terminal-shim-dir.test.ts | 95 ++++++++++++++++++- src/main/pty/legacy-terminal-shim-dir.ts | 30 +++--- .../pty/legacy-terminal-windows-tombstone.ts | 7 +- 4 files changed, 152 insertions(+), 18 deletions(-) diff --git a/src/main/pty/legacy-terminal-posix-tombstone.ts b/src/main/pty/legacy-terminal-posix-tombstone.ts index 09afc5937e5..2f838ceee3e 100644 --- a/src/main/pty/legacy-terminal-posix-tombstone.ts +++ b/src/main/pty/legacy-terminal-posix-tombstone.ts @@ -1,5 +1,5 @@ -import { accessSync, constants, statSync } from 'node:fs' -import { join } from 'node:path' +import { accessSync, constants, readFileSync, statSync } from 'node:fs' +import { basename, join } from 'node:path' const SHELL_DOLLAR = '$' @@ -70,6 +70,34 @@ export function resolvePosixTombstoneInterpreter( return null } +/** + * Returns the absolute interpreter the wrapper being replaced already used, or null. + * + * Why: when this process cannot see an absolute bash, deleting the wrapper strands a shell that + * has the path hashed -- it reports 127 rather than falling through to the next PATH entry. The + * file about to be overwritten ran on this host, so its own shebang names an interpreter known to + * work here, and reusing it keeps the compatibility promise without an ambient lookup. + */ +export function readVerifiedShebangInterpreter(filePath: string): string | null { + let firstLine: string + try { + firstLine = readFileSync(filePath, 'utf8').split('\n', 1)[0] ?? '' + } catch { + return null + } + const match = /^#!\s*(\S+)/.exec(firstLine) + const interpreter = match?.[1] + if (!interpreter?.startsWith('/')) { + return null + } + // Why: `#!/usr/bin/env bash` is absolute but defers the real lookup to PATH, which is the + // current-directory exposure this whole path exists to avoid. + if (basename(interpreter) === 'env') { + return null + } + return isExecutable(interpreter) ? interpreter : null +} + const POSIX_TOMBSTONE = String.raw`#!__ORCA_INTERPRETER__ set -u @@ -116,7 +144,11 @@ filter_path() { # Why: an empty or relative PATH element resolves against the current directory, so keeping # it would let a repository-local git/gh win the lookup below. : - elif [[ -n "$legacy_target" && "$normalized" == "$legacy_target" ]]; then + elif [[ -n "$legacy_target" && ( "$normalized" == "$legacy_target" || "$entry" -ef "$legacy_target" ) ]]; then + # Why both tests: -ef compares filesystem identity, so it catches the same directory reached + # through a symlink or a /legacy/../legacy spelling that the lexical compare misses. It is + # false when either path no longer exists, which is exactly when the lexical compare still + # holds, so neither alone is enough. : elif [[ "$entry" -ef "$wrapper_dir" ]]; then : diff --git a/src/main/pty/legacy-terminal-shim-dir.test.ts b/src/main/pty/legacy-terminal-shim-dir.test.ts index 0207fd09df7..41f287b3cc1 100644 --- a/src/main/pty/legacy-terminal-shim-dir.test.ts +++ b/src/main/pty/legacy-terminal-shim-dir.test.ts @@ -13,9 +13,13 @@ import { import { tmpdir } from 'node:os' import { join } from 'node:path' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' -import { resolvePosixTombstoneInterpreter } from './legacy-terminal-posix-tombstone' +import { + readVerifiedShebangInterpreter, + resolvePosixTombstoneInterpreter +} from './legacy-terminal-posix-tombstone' import { __resetLegacyTerminalShimNeutralizationForTests, + isLegacyTerminalShimPathEntry, neutralizeLegacyTerminalShimDir, stripLegacyTerminalShimEnv } from './legacy-terminal-shim-dir' @@ -168,13 +172,24 @@ describe('legacy terminal shim neutralization', () => { // separator lives in a variable because a literal backslash before the closing quote // breaks cmd's parser, and a sentinel character would corrupt paths containing it. expect(cmd).toContain('set "orca_sep=\\"') + // Why: bare setlocal inherits the caller's delayed-expansion state. Under a parent shell + // with /V:ON, a literal !CD! PATH entry became the cwd and ran a planted git.cmd (exit 66), + // and a legitimate directory containing ! stopped resolving (exit 127). Both on Windows 11. + expect(cmd).toContain('setlocal DisableDelayedExpansion') + expect(cmd).not.toContain('\r\nsetlocal\r\n') // Why: a captured ORCA_REAL_* may be relative, and both the existence test and the // invocation resolve it against the cwd. expect(cmd).toContain('if defined orca_real call :orca_check_rooted') expect(cmd).toContain('if defined orca_real if not defined orca_rooted set "orca_real="') // Why: the exported PATH is inherited by the real command; a relative entry left in it lets // the cwd select the tools that command spawns. - expectOrdered(cmd, ':orca_append_path', 'if not defined orca_rooted exit /b') + // Why the exact block, not an ordering pin: the first ':orca_append_path' in the file is + // the top-level CALL, and the first 'if not defined orca_rooted' belongs to a different + // subroutine, so an ordering pin stayed green with this subroutine's guard deleted. + expect(cmd).toContain(':orca_append_path\r\nif not defined orca_entry exit /b\r\n') + expect(cmd).toContain( + 'set "orca_probe=%orca_entry%"\r\ncall :orca_check_rooted\r\nif not defined orca_rooted exit /b\r\nfor %%G in ("%orca_entry%") do set "orca_path_entry_dir=%%~fG"' + ) // Why: CALL re-expands its own command line, so path data passed as an argument gets a // second round of percent expansion. A PATH entry holding a literal %CD% became the // current directory before the rooted check saw it and the planted git.cmd ran (rc=66 on @@ -443,6 +458,82 @@ describe('legacy terminal shim neutralization', () => { expect(run.stdout).not.toContain('HOSTILE') }) + itOnPosix('rejects the legacy shim directory reached by a different spelling', () => { + // Why: the legacy-dir compare was lexical while the self-compare used -ef, so a PATH entry + // spelled `/../` or reached through a symlink named the same directory but + // survived the filter, and the still-live attribution wrapper won the lookup. + const userData = makeUserDataDir() + const posixDir = join(userData, 'orca-terminal-attribution', 'posix') + const legacyDir = join(userData, 'legacy-shim') + const realBin = join(userData, 'real-bin') + for (const dir of [posixDir, legacyDir, realBin]) { + mkdirSync(dir, { recursive: true }) + } + writeFileSync(join(posixDir, 'git'), 'legacy attribution wrapper') + + neutralizeLegacyTerminalShimDir(userData) + + writeFileSync(join(legacyDir, 'git'), "#!/bin/bash\nprintf 'HOSTILE\\n'\n", { mode: 0o755 }) + writeFileSync(join(realBin, 'git'), "#!/bin/bash\nprintf 'REAL\\n'\n", { mode: 0o755 }) + const legacyLink = join(userData, 'legacy-link') + symlinkSync(legacyDir, legacyLink) + + for (const spelling of [join(legacyDir, '..', 'legacy-shim'), legacyLink]) { + const run = spawnSync(join(posixDir, 'git'), ['--version'], { + env: { + ...process.env, + ORCA_ATTRIBUTION_SHIM_DIR: legacyDir, + PATH: `${spelling}:${realBin}:/usr/bin:/bin` + }, + encoding: 'utf8', + timeout: 20_000 + }) + expect(run.stdout, `spelling ${spelling}`).toContain('REAL') + expect(run.stdout, `spelling ${spelling}`).not.toContain('HOSTILE') + } + }) + + itOnPosix('reuses the replaced wrapper shebang when no absolute bash is verifiable', () => { + // Why: deleting the wrapper strands a shell that already hashed the path -- it reports 127 + // rather than falling through to PATH. The file being replaced ran on this host, so its own + // shebang names an interpreter known to work here. + const userData = makeUserDataDir() + const wrapper = join(userData, 'git') + writeFileSync(wrapper, '#!/bin/bash\nexit 0\n', { mode: 0o755 }) + expect(readVerifiedShebangInterpreter(wrapper)).toBe('/bin/bash') + + // Why: `env` is absolute but defers the lookup to PATH, which is the cwd exposure this exists + // to close. Accepting it would silently reinstate the hijack the null branch prevents. + writeFileSync(wrapper, '#!/usr/bin/env bash\nexit 0\n', { mode: 0o755 }) + expect(readVerifiedShebangInterpreter(wrapper)).toBeNull() + + writeFileSync(wrapper, '#!bash\nexit 0\n', { mode: 0o755 }) + expect(readVerifiedShebangInterpreter(wrapper)).toBeNull() + + writeFileSync(wrapper, '#!/nonexistent/bash\nexit 0\n', { mode: 0o755 }) + expect(readVerifiedShebangInterpreter(wrapper)).toBeNull() + + writeFileSync(wrapper, 'no shebang at all\n', { mode: 0o755 }) + expect(readVerifiedShebangInterpreter(wrapper)).toBeNull() + + expect(readVerifiedShebangInterpreter(join(userData, 'absent'))).toBeNull() + }) + + it('treats a shim directory reached through .. as a shim PATH entry', () => { + // Why: a plain suffix test missed `/orca-terminal-attribution/posix/../posix`, so the + // entry survived the scrub and the shim stayed reachable. + expect(isLegacyTerminalShimPathEntry('/tmp/old/orca-terminal-attribution/posix/../posix')).toBe( + true + ) + expect(isLegacyTerminalShimPathEntry('/tmp/old/orca-terminal-attribution/win32/../win32')).toBe( + true + ) + expect(isLegacyTerminalShimPathEntry('/tmp/old/orca-terminal-attribution/posix/../other')).toBe( + false + ) + expect(isLegacyTerminalShimPathEntry('/usr/local/bin')).toBe(false) + }) + itOnPosix('does not fall back to the cwd when every PATH entry is filtered out', () => { // Why: with the shim dir as the only entry the cleaned PATH is empty; without the // path_entry_kept guard the lookup runs against that empty PATH and finds a cwd-local git. diff --git a/src/main/pty/legacy-terminal-shim-dir.ts b/src/main/pty/legacy-terminal-shim-dir.ts index 08f4a7d19b0..02b2b8dc5d0 100644 --- a/src/main/pty/legacy-terminal-shim-dir.ts +++ b/src/main/pty/legacy-terminal-shim-dir.ts @@ -1,6 +1,7 @@ import { chmodSync, existsSync, mkdirSync, renameSync, rmSync, writeFileSync } from 'node:fs' -import { join } from 'node:path' +import { join, normalize } from 'node:path' import { + readVerifiedShebangInterpreter, renderLegacyTerminalPosixTombstone, resolvePosixTombstoneInterpreter } from './legacy-terminal-posix-tombstone' @@ -98,19 +99,18 @@ function writeNeutralWrappers(rootDir: string): void { const win32Dir = join(rootDir, 'win32') mkdirSync(posixDir, { recursive: true }) mkdirSync(win32Dir, { recursive: true }) - // Why: with no absolute interpreter verifiable, a tombstone would need an ambient shebang and - // could take bash from the current directory. Deleting the legacy wrapper is the safe outcome: - // the command simply falls through to the real one on PATH. + // Why: a tombstone with no verifiable absolute interpreter would need an ambient shebang and + // could take bash from the current directory. Falling back to the shebang of the wrapper being + // replaced keeps a shell that has the path hashed working -- deleting the file strands it on + // 127 rather than falling through to PATH. Deleting is the last resort, not the first. const interpreter = resolvePosixTombstoneInterpreter() for (const command of ['git', 'gh'] as const) { - if (interpreter === null) { - rmSync(join(posixDir, command), { force: true }) + const posixPath = join(posixDir, command) + const resolved = interpreter ?? readVerifiedShebangInterpreter(posixPath) + if (resolved === null) { + rmSync(posixPath, { force: true }) } else { - writeFileAtomically( - join(posixDir, command), - renderLegacyTerminalPosixTombstone(command, interpreter), - 0o755 - ) + writeFileAtomically(posixPath, renderLegacyTerminalPosixTombstone(command, resolved), 0o755) } writeFileAtomically( join(win32Dir, `${command}.cmd`), @@ -158,7 +158,13 @@ function pathEntrySpellings(dir: string, windows: boolean): string[] { } export function isLegacyTerminalShimPathEntry(entry: string): boolean { - const normalized = entry.replaceAll('\\', '/').replace(/\/+$/, '').toLowerCase() + // Why normalize first: a `.../orca-terminal-attribution/posix/../posix` spelling names the shim + // directory but fails a plain suffix test, so the entry survived the scrub and the shim stayed + // reachable. normalize collapses `..` textually, which is what a suffix test needs. + const normalized = normalize(entry.replaceAll('\\', '/')) + .replaceAll('\\', '/') + .replace(/\/+$/, '') + .toLowerCase() return ( normalized.endsWith(`/${LEGACY_SHIM_ROOT_DIR}/posix`) || normalized.endsWith(`/${LEGACY_SHIM_ROOT_DIR}/win32`) diff --git a/src/main/pty/legacy-terminal-windows-tombstone.ts b/src/main/pty/legacy-terminal-windows-tombstone.ts index 415cb1374aa..124b930952e 100644 --- a/src/main/pty/legacy-terminal-windows-tombstone.ts +++ b/src/main/pty/legacy-terminal-windows-tombstone.ts @@ -13,7 +13,12 @@ // script died with "The syntax of the command is incorrect." const WIN32_PASSTHROUGH_WRAPPER = String.raw`@echo off -setlocal +rem Why explicit: bare setlocal inherits the caller's delayed-expansion state, and a parent shell +rem started with /V:ON made every PATH entry undergo ! expansion inside the loops below. A literal +rem !CD! entry then became the current directory and a planted git.cmd ran (exit 66), and a +rem legitimate directory whose name contains ! stopped resolving (exit 127). Both proven on +rem Windows 11; both disappear when the wrapper pins its own state. +setlocal DisableDelayedExpansion set "orca_real=%ORCA_REAL___ORCA_UPPER_COMMAND__%" set "orca_wrapper_dir=%~dp0" set "orca_legacy_wrapper_dir=%ORCA_ATTRIBUTION_SHIM_DIR%"