diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 3d77ca1cf9..0c559e5193 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -296,6 +296,9 @@ jobs: - run: cd cli && npm run lint && npm test - name: Verify CLI test temp cleanup run: cd cli && npm run test:temp-cleanup + # The seats package is bundled into the CLI, but its node:test suite sits + # outside the CLI's jest roots, so no other step runs it. No dependencies. + - run: cd packages/seats && npm test - run: cd workers/agent-runtime && npm run typecheck && npm test test-gate: diff --git a/packages/seats/src/valet/env.js b/packages/seats/src/valet/env.js index fe37dfefbe..507bcaa186 100644 --- a/packages/seats/src/valet/env.js +++ b/packages/seats/src/valet/env.js @@ -11,6 +11,7 @@ * (Seatbelt, bwrap) is what stops a process that goes looking. */ import { randomUUID } from 'node:crypto'; +import { realpathSync } from 'node:fs'; import { mkdtemp, mkdir, chmod, readFile, rm } from 'node:fs/promises'; import { homedir, tmpdir } from 'node:os'; import { delimiter, join, resolve, sep } from 'node:path'; @@ -35,6 +36,10 @@ const isInside = (path, dir) => { * read the user's login (~/.local/bin/claude). Adapters run their CLI by * absolute path, so the run loses nothing it needs. */ +// A HOME reached through a symlink (macOS /var -> /private/var) shows up in +// PATH under its resolved path too, e.g. npm's cwd-derived node_modules/.bin. +const spellings = (dir) => { try { return [dir, realpathSync(dir)]; } catch { return [dir]; } }; + const scrubPath = (value, realHomes) => value.split(delimiter) .filter((entry) => entry && !realHomes.some((home) => isInside(entry, home))) .join(delimiter); @@ -122,7 +127,7 @@ export const prepareRunEnv = async ({ const hook = trace && typeof trace.emit === 'function' ? trace : traceOf(ctx); const issued = grantMeta ? grantIssued(grantMeta) : null; if (issued) hook.emit('grant.issued', issued); - const realHomes = [...new Set([baseEnv.HOME, homedir()].filter(Boolean))]; + const realHomes = [...new Set([baseEnv.HOME, homedir()].filter(Boolean).flatMap(spellings))]; const DENIED = { not_granted: 'is not in the run grant', unresolved: 'could not be resolved', diff --git a/packages/seats/test/valet-env.test.js b/packages/seats/test/valet-env.test.js index 6218f3a5a2..aca618fff1 100644 --- a/packages/seats/test/valet-env.test.js +++ b/packages/seats/test/valet-env.test.js @@ -1,9 +1,9 @@ import { test } from 'node:test'; import assert from 'node:assert/strict'; import { spawnSync } from 'node:child_process'; -import { existsSync, mkdirSync, mkdtempSync, readFileSync, readdirSync as readdir, rmSync, statSync, writeFileSync } from 'node:fs'; +import { existsSync, mkdirSync, mkdtempSync, readFileSync, readdirSync as readdir, realpathSync, rmSync, statSync, symlinkSync, writeFileSync } from 'node:fs'; import { homedir, tmpdir } from 'node:os'; -import { join } from 'node:path'; +import { delimiter, join, sep } from 'node:path'; import { prepareRunEnv } from '../src/valet/env.js'; import { createTraceStore } from '../src/trace/store.js'; import { RunFailure } from '../src/failures.js'; @@ -12,6 +12,11 @@ const REAL_HOME = homedir(); const GRANTED = 'sk-granted-0123456789abcdef'; const WITHHELD = 'sk-withheld-fedcba9876543210'; +// True when value names dir or a path inside it. A bare substring check also matches a +// sibling that only shares the prefix: under HOME=/home/node, npm's own PATH entry +// /home/node_modules/.bin is outside HOME, and env.js rightly keeps it. +const namesPath = (value, dir) => new RegExp(`${dir.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}(?=$|[${sep}${delimiter}])`).test(value); + const recorder = () => { const events = []; return { events, emit: (type, data) => events.push({ type, data }) }; }; const withScratch = async (fn) => { const dir = mkdtempSync(join(tmpdir(), 'seats-valet-')); @@ -61,8 +66,8 @@ test('a real child sees only the granted key, a throwaway HOME, and no path to t assert.equal(seen.homedir, run.home); assert.ok(run.home.startsWith(root)); for (const [name, value] of Object.entries(seen.env)) { - assert.ok(!value.includes(loginHome), `${name} leaks the login HOME path: ${value}`); - assert.ok(!value.includes(REAL_HOME), `${name} leaks the real HOME path: ${value}`); + assert.ok(!namesPath(value, loginHome), `${name} leaks the login HOME path: ${value}`); + assert.ok(!namesPath(value, REAL_HOME), `${name} leaks the real HOME path: ${value}`); } assert.equal(seen.claude, false, '~/.claude is not reachable via HOME'); assert.equal(seen.codex, false, '~/.codex is not reachable via HOME'); @@ -94,6 +99,16 @@ test('the base env keeps only the allowlist, and PATH loses entries under the re } finally { await run.cleanup(); } })); +test('a HOME reached through a symlink loses PATH entries under its resolved path too', () => withScratch(async (root) => { + const real = join(realpathSync(root), 'real-home'); + mkdirSync(real); + const link = join(root, 'home-link'); + symlinkSync(real, link); + const baseEnv = { HOME: link, PATH: `/usr/bin:${real}/work/node_modules/.bin:${link}/.local/bin:/bin` }; + const run = await prepareRunEnv({ grant: {}, baseEnv, root }); + try { assert.equal(run.env.PATH, '/usr/bin:/bin'); } finally { await run.cleanup(); } +})); + test('file and resolver credentials resolve; resolveSecret sees the name and spec', () => withScratch(async (root) => { const keyFile = join(root, 'key.txt'); writeFileSync(keyFile, 'from-file-value\n', { mode: 0o600 });