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%"