mirror of
https://github.com/stablyai/orca.git
synced 2026-09-22 16:02:32 +00:00
fix(path): stop seeded user bin dirs from outranking the inherited PATH (#18265)
`patchPackagedProcessPath` prepends every seeded directory, so `~/bin` and `~/.local/bin` land ahead of the PATH a GUI-launched Electron inherited. That does more than make a tool findable, which is what seeding is for -- it re-ranks binaries the user already has, and those two directories are user-writable and can hold a wrapper for any system tool. On the #18234 reporter's box `~/.local/bin/gh` wraps `mise x gh -- gh`. Seeded ahead of /usr/bin we ran the wrapper where their own shell ran the real binary, and the wrapper's inner bare `gh` resolved back to itself. Measured in an Ubuntu 24.04 container: with their shell's ordering the chain exits in 22ms; with ours it never terminates and creates ~1,500 processes/second. Seed order now follows the rule the WSL twin already documents in posix-version-manager-bin-dirs.ts -- append, never prepend, because a login PATH that did resolve is authoritative. Version-manager shim dirs keep leading, since an nvm/mise/asdf user's runtime must still beat a system install; the generic user bin dirs move behind the inherited PATH. `getVersionManagerBinPaths` carries `~/bin` and `~/.local/bin` too (bun and pnpm install there), so they are filtered out of the leading group by name rather than by which list produced them.
This commit is contained in:
@@ -137,6 +137,66 @@ describe('patchPackagedProcessPath', () => {
|
||||
expect(segments).toContain('/usr/local/bin')
|
||||
})
|
||||
|
||||
// Why this ordering is load-bearing (#18234): a seed exists so a GUI-launched
|
||||
// Electron can *find* a tool, not to re-rank tools the user already has.
|
||||
// `~/.local/bin` is user-writable and can hold a wrapper for any system tool.
|
||||
// The reporter's `~/.local/bin/gh` wrapped `mise x gh -- gh`; seeded ahead of
|
||||
// /usr/bin it ran instead of the real gh, and the wrapper's inner bare `gh`
|
||||
// resolved back to itself. Measured in a container: with the login shell's
|
||||
// ordering that chain exits in 22ms, with the seeded ordering it never
|
||||
// terminates and creates ~1,300 processes/second.
|
||||
it('never lets a seeded user dir overtake a system dir already on PATH', async () => {
|
||||
const { app } = await import('electron')
|
||||
const { patchPackagedProcessPath } = await import('./configure-process')
|
||||
|
||||
setPlatform('linux')
|
||||
Object.defineProperty(app, 'isPackaged', { configurable: true, value: true })
|
||||
process.env.HOME = '/home/tester'
|
||||
process.env.PATH = '/usr/local/bin:/usr/bin:/bin'
|
||||
|
||||
patchPackagedProcessPath()
|
||||
|
||||
const segments = (process.env.PATH ?? '').split(':')
|
||||
const localBin = segments.indexOf(join('/home/tester', '.local/bin'))
|
||||
// Still reachable — that is what the seeding is for (#829).
|
||||
expect(localBin).toBeGreaterThan(-1)
|
||||
for (const systemDir of ['/usr/bin', '/bin', '/usr/local/bin']) {
|
||||
expect(segments.indexOf(systemDir)).toBeLessThan(localBin)
|
||||
}
|
||||
expect(segments.indexOf(join('/home/tester', 'bin'))).toBeGreaterThan(
|
||||
segments.indexOf('/usr/bin')
|
||||
)
|
||||
})
|
||||
|
||||
it('keeps version-manager shims ahead of the inherited PATH', async () => {
|
||||
const { app } = await import('electron')
|
||||
const { patchPackagedProcessPath } = await import('./configure-process')
|
||||
const { getVersionManagerBinPaths } = await import('../../shared/node-cli-command-resolution')
|
||||
|
||||
setPlatform('linux')
|
||||
Object.defineProperty(app, 'isPackaged', { configurable: true, value: true })
|
||||
process.env.HOME = '/home/tester'
|
||||
process.env.PATH = '/usr/bin:/bin'
|
||||
|
||||
patchPackagedProcessPath()
|
||||
|
||||
const segments = (process.env.PATH ?? '').split(':')
|
||||
const genericUserBinDirs = [join('/home/tester', 'bin'), join('/home/tester', '.local/bin')]
|
||||
const seeded = getVersionManagerBinPaths({ platform: 'linux', homePath: '/home/tester' })
|
||||
const shimDirs = seeded.filter((dir) => !genericUserBinDirs.includes(dir))
|
||||
expect(shimDirs).not.toHaveLength(0)
|
||||
// Why these keep leading: an nvm/mise/asdf user's runtime must beat a
|
||||
// system install, which is the reason this seeding is ordered at all.
|
||||
for (const dir of shimDirs) {
|
||||
expect(segments.indexOf(dir)).toBeLessThan(segments.indexOf('/usr/bin'))
|
||||
}
|
||||
// Why these do not: the same list carries the generic user bin dirs, which
|
||||
// hold whatever was last installed there rather than a managed toolchain.
|
||||
for (const dir of genericUserBinDirs) {
|
||||
expect(segments.indexOf(dir)).toBeGreaterThan(segments.indexOf('/usr/bin'))
|
||||
}
|
||||
})
|
||||
|
||||
it('leaves PATH untouched when the app is not packaged', async () => {
|
||||
const { app } = await import('electron')
|
||||
const { patchPackagedProcessPath } = await import('./configure-process')
|
||||
|
||||
@@ -117,20 +117,37 @@ export function patchPackagedProcessPath(): void {
|
||||
}
|
||||
|
||||
const home = process.env.HOME ?? ''
|
||||
const extraPaths: string[] = []
|
||||
// Why two lists: a seed exists so a GUI-launched Electron can *find* a tool
|
||||
// its minimal PATH omits. Putting one ahead of the inherited PATH does more
|
||||
// than that — it re-ranks binaries the user already has, and `~/bin` and
|
||||
// `~/.local/bin` are arbitrary user-writable directories that can shadow any
|
||||
// system tool. On the #18234 reporter's box `~/.local/bin/gh` is a wrapper
|
||||
// around `mise x gh -- gh`; hoisting it over /usr/bin/gh made us run the
|
||||
// wrapper where their own shell ran the real binary, and the inner bare `gh`
|
||||
// then resolved back to the wrapper. So: append these, and let a real
|
||||
// ordering opinion come from the login shell via mergePathSegments.
|
||||
const isGenericUserBinDir = (path: string): boolean =>
|
||||
process.platform !== 'win32' &&
|
||||
home !== '' &&
|
||||
(path === join(home, 'bin') || path === join(home, '.local/bin'))
|
||||
const appendPaths: string[] = []
|
||||
// Why these still lead: version-manager shims must beat a system install or
|
||||
// an nvm/mise/asdf user gets the wrong runtime, which is the whole reason
|
||||
// this seeding is ordered rather than appended (see hydrate-shell-path.ts).
|
||||
const prependPaths: string[] = []
|
||||
|
||||
if (process.platform !== 'win32') {
|
||||
extraPaths.push('/opt/homebrew/bin', '/opt/homebrew/sbin', '/usr/local/bin', '/usr/local/sbin')
|
||||
appendPaths.push('/opt/homebrew/bin', '/opt/homebrew/sbin', '/usr/local/bin', '/usr/local/sbin')
|
||||
|
||||
if (process.platform === 'linux') {
|
||||
// Why: snap and Linuxbrew ship on Linux only, so seeding them elsewhere adds phantom PATH entries every spawn must stat.
|
||||
extraPaths.push('/snap/bin', '/home/linuxbrew/.linuxbrew/bin')
|
||||
appendPaths.push('/snap/bin', '/home/linuxbrew/.linuxbrew/bin')
|
||||
}
|
||||
|
||||
extraPaths.push('/nix/var/nix/profiles/default/bin')
|
||||
appendPaths.push('/nix/var/nix/profiles/default/bin')
|
||||
|
||||
if (home) {
|
||||
extraPaths.push(
|
||||
appendPaths.push(
|
||||
join(home, 'bin'),
|
||||
join(home, '.local/bin'),
|
||||
join(home, '.nix-profile/bin'),
|
||||
@@ -142,18 +159,24 @@ export function patchPackagedProcessPath(): void {
|
||||
}
|
||||
|
||||
// Why: version-manager CLIs use env-node shebangs, so node must be on PATH or spawns fail (also seeds Windows user-local dirs).
|
||||
extraPaths.push(...getVersionManagerBinPaths())
|
||||
// Why the filter: that list carries `~/bin` and `~/.local/bin` too, because
|
||||
// bun/pnpm/npm --user also install there. Those two are generic user bin
|
||||
// directories, not a version manager's own shim directory, so they hold
|
||||
// whatever the user last dropped in them and must not outrank a system dir.
|
||||
// The specific dirs (.volta/bin, .asdf/shims, mise shims, .bun/bin, …) keep
|
||||
// leading, which is what the ordering was actually for.
|
||||
prependPaths.push(...getVersionManagerBinPaths().filter((path) => !isGenericUserBinDir(path)))
|
||||
|
||||
const pathKey = process.platform === 'win32' && process.env.Path !== undefined ? 'Path' : 'PATH'
|
||||
const currentPath = process.env[pathKey] ?? ''
|
||||
const pathDelimiter = getProcessPathDelimiter()
|
||||
const existing = new Set(currentPath.split(pathDelimiter))
|
||||
const missing = extraPaths.filter((path) => !existing.has(path))
|
||||
const currentSegments = currentPath.split(pathDelimiter).filter(Boolean)
|
||||
const existing = new Set(currentSegments)
|
||||
const prepend = prependPaths.filter((path) => !existing.has(path))
|
||||
const append = appendPaths.filter((path) => !existing.has(path) && !prepend.includes(path))
|
||||
|
||||
if (missing.length > 0) {
|
||||
process.env[pathKey] = [...missing, ...currentPath.split(pathDelimiter).filter(Boolean)].join(
|
||||
pathDelimiter
|
||||
)
|
||||
if (prepend.length > 0 || append.length > 0) {
|
||||
process.env[pathKey] = [...prepend, ...currentSegments, ...append].join(pathDelimiter)
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user