From a137c2cf4ecc1678ab2d6b21ac74779cf976d9bc Mon Sep 17 00:00:00 2001 From: ayush-that Date: Wed, 21 Oct 2026 11:14:21 +0530 Subject: [PATCH 1/4] fix(ci): reject workflow symlinks that resolve outside the repository --- src/commands/agent.ts | 47 +------ src/commands/ci.spec.ts | 10 ++ src/commands/ci.ts | 43 ++++++- src/lib/target-path.ts | 46 +++++++ test/e2e/ci-init-symlinks.e2e.test.ts | 170 ++++++++++++++++++++++++++ 5 files changed, 267 insertions(+), 49 deletions(-) create mode 100644 src/lib/target-path.ts create mode 100644 test/e2e/ci-init-symlinks.e2e.test.ts diff --git a/src/commands/agent.ts b/src/commands/agent.ts index 9e202e2f..1afb1933 100644 --- a/src/commands/agent.ts +++ b/src/commands/agent.ts @@ -6,6 +6,7 @@ import type { CommonOptions as FactoryCommonOptions } from '../lib/client-factor import { CLIError, localValidationError } from '../lib/errors.js'; import type { OutputMode } from '../lib/output.js'; import { GLOBAL_OPTS_HINT, Output, resolveOutputMode } from '../lib/output.js'; +import { inspectTargetPath } from '../lib/target-path.js'; import { promptText } from '../lib/prompt.js'; import { FALLBACK_TARGET, resolveAgentTargets, type DetectAgentDeps } from '../lib/agent-detect.js'; import { @@ -81,52 +82,6 @@ const defaultAgentFs: AgentFs = { // Path safety // --------------------------------------------------------------------------- -/** - * Walk each component of `relPath` beneath `root`, refusing to traverse or - * write through a symlink. `fs.mkdir`/`writeFile` follow symlinks, so a planted - * symlink at any existing path component (e.g. `.claude` -> /etc, or the final - * `SKILL.md` -> ~/.bashrc) could place or clobber files outside `--dir`. The - * lexical containment guard in `runInstall` is a string compare and cannot see - * this; only an `lstat`-per-component walk can. Fail-closed: any symlink is - * rejected (exit 5). - * - * Returns the target's `{ isFile }` when it already exists, or `null` when it - * (or any ancestor) does not yet exist — in which case the missing tail is - * created fresh and cannot be a pre-planted symlink. A small TOCTOU window - * remains between this check and the write; that is acceptable for a local, - * single-user CLI and avoids non-portable O_NOFOLLOW / rename gymnastics. - */ -async function inspectTargetPath( - agentFs: AgentFs, - root: string, - relPath: string, -): Promise<{ isFile: boolean } | null> { - const segments = relPath.split(/[/\\]+/).filter(Boolean); - let current = root; - let finalIsFile = false; - for (const [i, seg] of segments.entries()) { - current = path.join(current, seg); - const ls = await agentFs.lstat(current); - if (ls === null) { - // This component and everything below it does not exist yet. - return null; - } - if (ls.isSymbolicLink) { - const shown = segments.slice(0, i + 1).join('/'); - throw new CLIError( - `refusing to write through a symlink: "${shown}" — installing here could place files outside --dir. Remove the symlink or choose a different --dir.`, - 5, - ); - } - if (i < segments.length - 1 && ls.isFile) { - const shown = segments.slice(0, i + 1).join('/'); - throw new CLIError(`cannot create ${relPath}: "${shown}" exists and is not a directory.`, 5); - } - finalIsFile = ls.isFile; - } - return { isFile: finalIsFile }; -} - /** * Back up the current bytes at `abs` next to it without clobbering any existing * backup or writing through a symlink. Exclusive create (`wx`) fails with diff --git a/src/commands/ci.spec.ts b/src/commands/ci.spec.ts index 0a50862c..8f6f43c9 100644 --- a/src/commands/ci.spec.ts +++ b/src/commands/ci.spec.ts @@ -52,6 +52,16 @@ function makeFakeFs(seed: Record = {}): CiFs & { files: Map(Object.entries(seed)); return { files, + async lstat(target) { + if (files.has(target)) return { isFile: true, isSymbolicLink: false }; + if ([...files.keys()].some(file => file.startsWith(`${target}${sep}`))) { + return { isFile: false, isSymbolicLink: false }; + } + return null; + }, + async realpath(target) { + return target; + }, async writeFile(target, data, opts) { if (opts?.exclusive && files.has(target)) { const err = new Error('EEXIST') as NodeJS.ErrnoException; diff --git a/src/commands/ci.ts b/src/commands/ci.ts index 9e3eec2e..d428a3a3 100644 --- a/src/commands/ci.ts +++ b/src/commands/ci.ts @@ -12,6 +12,7 @@ import { loadConfig, resolveProfileName } from '../lib/config.js'; import { type FetchImpl, type HttpClient } from '../lib/http.js'; import { GLOBAL_OPTS_HINT, Output, resolveOutputMode } from '../lib/output.js'; import type { Page } from '../lib/pagination.js'; +import { inspectTargetPath, type TargetPathFs } from '../lib/target-path.js'; import { assertNotLocal } from '../lib/target-url.js'; import { classifyLinkage, @@ -67,10 +68,11 @@ const DEFAULT_API_URL = 'https://api.testsprite.com'; /** * Minimal fs seam (mirrors `agent.ts`'s `AgentFs` semantics: exclusive writes to - * refuse clobbering, recursive mkdir to create parent dirs). Kept local so `ci` - * doesn't couple to the agent-install internals. + * refuse clobbering, recursive mkdir to create parent dirs). Path inspection + * shares the agent installer's component walk. */ -export interface CiFs { +export interface CiFs extends TargetPathFs { + realpath(target: string): Promise; writeFile(target: string, data: string, opts?: { exclusive?: boolean }): Promise; mkdir(dir: string): Promise; readFile(target: string): Promise; @@ -79,6 +81,18 @@ export interface CiFs { } const defaultCiFs: CiFs = { + async lstat(target) { + try { + // eslint-disable-next-line security/detect-non-literal-fs-filename -- inspect each component before writing the caller-selected workflow path. + const stat = await nodeFs.lstat(target); + return { isFile: stat.isFile(), isSymbolicLink: stat.isSymbolicLink() }; + } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'ENOENT') return null; + throw error; + } + }, + // eslint-disable-next-line security/detect-non-literal-fs-filename -- resolve the repository and workflow symlink for containment validation. + realpath: target => nodeFs.realpath(target), // `target`/`dir` are the workflow output path (default `.github/workflows/testsprite.yml`, // or the caller's `--path`), resolved from cwd — a scaffold target the user is writing into // their own repo, never external/network input. Same risk profile as agent-install scaffolding. @@ -628,6 +642,29 @@ async function runCiInitScaffold( if (opts.project) assertSafeWorkflowValue('project', opts.project); if (opts.env) assertSafeWorkflowValue('env', opts.env); const { relPath, absPath } = resolveWorkflowPath(opts.workflowPath, cwd); + const allowInternalSymlink = async (target: string): Promise => { + let resolved: string; + let root: string; + try { + [root, resolved] = await Promise.all([fs.realpath(cwd), fs.realpath(target)]); + } catch { + throw new CLIError( + `cannot resolve symlink at "${target}"; refusing to write the workflow.`, + 5, + ); + } + const relative = path.relative(root, resolved); + if (relative === '..' || relative.startsWith(`..${path.sep}`) || path.isAbsolute(relative)) { + throw new CLIError( + `refusing to write through symlink "${target}": "${resolved}" is outside the repository.`, + 5, + ); + } + }; + await inspectTargetPath(fs, cwd, relPath, allowInternalSymlink); + if (opts.force) { + await inspectTargetPath(fs, cwd, `${relPath}.bak`, allowInternalSymlink); + } const projectId = await resolveProjectId(opts, deps); const resolvedEnv = await resolveEnvironment(opts, deps, projectId); diff --git a/src/lib/target-path.ts b/src/lib/target-path.ts new file mode 100644 index 00000000..a8b5aebe --- /dev/null +++ b/src/lib/target-path.ts @@ -0,0 +1,46 @@ +import path from 'node:path'; +import { CLIError } from './errors.js'; + +export interface TargetPathFs { + lstat(p: string): Promise<{ isFile: boolean; isSymbolicLink: boolean } | null>; +} + +/** + * Inspect existing components before creating a target. Symlinks are refused + * unless the caller supplies a policy that validates their destination. + * Returns null at the first missing component. A race between inspection and + * writing remains, as with the agent installer's original path guard. + */ +export async function inspectTargetPath( + agentFs: TargetPathFs, + root: string, + relPath: string, + allowSymlink?: (target: string) => Promise, +): Promise<{ isFile: boolean } | null> { + const segments = relPath.split(process.platform === 'win32' ? /[/\\]+/ : /\/+/).filter(Boolean); + let current = root; + let finalIsFile = false; + for (const [i, seg] of segments.entries()) { + current = path.join(current, seg); + const ls = await agentFs.lstat(current); + if (ls === null) { + // This component and everything below it does not exist yet. + return null; + } + if (ls.isSymbolicLink && allowSymlink) { + await allowSymlink(current); + } else if (ls.isSymbolicLink) { + const shown = segments.slice(0, i + 1).join('/'); + throw new CLIError( + `refusing to write through a symlink: "${shown}" — installing here could place files outside --dir. Remove the symlink or choose a different --dir.`, + 5, + ); + } + if (i < segments.length - 1 && ls.isFile) { + const shown = segments.slice(0, i + 1).join('/'); + throw new CLIError(`cannot create ${relPath}: "${shown}" exists and is not a directory.`, 5); + } + finalIsFile = ls.isFile; + } + return { isFile: finalIsFile }; +} diff --git a/test/e2e/ci-init-symlinks.e2e.test.ts b/test/e2e/ci-init-symlinks.e2e.test.ts new file mode 100644 index 00000000..71be477f --- /dev/null +++ b/test/e2e/ci-init-symlinks.e2e.test.ts @@ -0,0 +1,170 @@ +/* eslint-disable security/detect-non-literal-fs-filename -- Every filesystem path belongs to the isolated mkdtemp fixture; symlinks deliberately exercise the real CLI boundary. */ +import { execFile } from 'node:child_process'; +import { createServer } from 'node:http'; +import { + mkdtemp, + mkdir, + readFile, + readdir, + realpath, + rm, + symlink, + writeFile, +} from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { dirname, join, resolve } from 'node:path'; +import { promisify } from 'node:util'; +import { afterEach, describe, expect, it } from 'vitest'; + +const binary = resolve('dist/index.js'); +const execute = promisify(execFile); +let fixture: string; + +afterEach(async () => { + if (fixture) await rm(fixture, { recursive: true, force: true }); +}); + +// Cover escaping ancestors, final files and backups, internal links, dangling +// links, and a symlinked cwd. All requests stay on loopback with dummy credentials. +describe.skipIf(process.platform === 'win32')('ci init symlink containment', () => { + async function setup() { + fixture = await realpath(await mkdtemp(join(tmpdir(), 'testsprite-ci-path-'))); + const repo = join(fixture, 'repo'); + const outside = join(fixture, 'outside'); + const home = join(fixture, 'home'); + await Promise.all([repo, outside, home].map(dir => mkdir(dir))); + return { repo, outside, home }; + } + + async function run(cwd: string, home: string, force = false, workflowPath?: string) { + const server = createServer((_req, res) => { + res.setHeader('content-type', 'application/json'); + res.end(JSON.stringify({ environments: [{ name: 'staging', isDefault: true }] })); + }); + await new Promise(done => server.listen(0, '127.0.0.1', done)); + const address = server.address() as { port: number }; + try { + const env = { ...process.env, HOME: home, USERPROFILE: home }; + for (const key of Object.keys(env)) { + if (key.startsWith('TESTSPRITE_')) delete env[key as keyof typeof env]; + } + return await execute( + process.execPath, + [ + binary, + '--endpoint-url', + `http://127.0.0.1:${address.port}`, + 'ci', + 'init', + 'github', + '--project', + 'dummy-project', + '--env', + 'staging', + ...(force ? ['--force'] : []), + ...(workflowPath ? ['--path', workflowPath] : []), + ], + { + cwd, + env: { + ...env, + TESTSPRITE_API_KEY: 'sk-user-local-only', + TESTSPRITE_NO_TELEMETRY: '1', + TESTSPRITE_NO_UPDATE_NOTIFIER: '1', + }, + timeout: 10000, + }, + ).then( + result => ({ ...result, code: 0 }), + error => ({ + stdout: error.stdout as string, + stderr: error.stderr as string, + code: error.code as number, + }), + ); + } finally { + await new Promise((done, reject) => + server.close(error => (error ? reject(error) : done())), + ); + } + } + + for (const location of [ + '.github', + '.github/workflows', + '.github/workflows/testsprite.yml', + '.github/workflows/testsprite.yml.bak', + ]) { + it(`refuses external ${location} before writing`, async () => { + const { repo, outside, home } = await setup(); + const target = + location.endsWith('.yml') || location.endsWith('.bak') + ? join(outside, 'sentinel') + : outside; + if (target !== outside) await writeFile(target, 'unchanged'); + const link = join(repo, location); + await mkdir(dirname(link), { recursive: true }); + if (location.endsWith('.bak')) + await writeFile(join(repo, '.github/workflows/testsprite.yml'), 'original workflow'); + await symlink(target, link); + const result = await run(repo, home, true); + expect(result.code, result.stderr).toBe(5); + expect(result.stderr).toContain(target); + expect(await readdir(outside)).toEqual(target === outside ? [] : ['sentinel']); + if (target !== outside) expect(await readFile(target, 'utf8')).toBe('unchanged'); + if (location.endsWith('.bak')) + expect(await readFile(join(repo, '.github/workflows/testsprite.yml'), 'utf8')).toBe( + 'original workflow', + ); + }); + } + + for (const location of ['.github', '.github/workflows', '.github/workflows/testsprite.yml']) { + it(`allows internal ${location}`, async () => { + const { repo, home } = await setup(); + const target = join(repo, 'internal'); + if (location.endsWith('.yml')) await writeFile(target, 'original workflow'); + else await mkdir(target); + const link = join(repo, location); + await mkdir(dirname(link), { recursive: true }); + await symlink(target, link); + const result = await run(repo, home, true); + expect(result.code, result.stderr).toBe(0); + expect(await readFile(join(repo, '.github/workflows/testsprite.yml'), 'utf8')).toContain( + 'dummy-project', + ); + }); + } + + it('refuses a dangling external ancestor without creating its target', async () => { + const { repo, outside, home } = await setup(); + await symlink(join(outside, 'missing'), join(repo, '.github')); + const result = await run(repo, home); + expect(result.code, result.stderr).toBe(5); + expect(await readdir(outside)).toEqual([]); + }); + + it('checks a literal backslash in a POSIX workflow filename', async () => { + const { repo, outside, home } = await setup(); + const workflowPath = '.github/workflows/a\\b.yml'; + const target = join(outside, 'sentinel'); + await mkdir(join(repo, '.github/workflows'), { recursive: true }); + await writeFile(target, 'unchanged'); + await symlink(target, join(repo, workflowPath)); + const result = await run(repo, home, true, workflowPath); + expect(result.code, result.stderr).toBe(5); + expect(result.stderr).toContain(target); + expect(await readFile(target, 'utf8')).toBe('unchanged'); + }); + + it('allows a symlinked working directory', async () => { + const { repo, home } = await setup(); + const alias = join(fixture, 'alias'); + await symlink(repo, alias); + const result = await run(alias, home); + expect(result.code, result.stderr).toBe(0); + expect(await readFile(join(repo, '.github/workflows/testsprite.yml'), 'utf8')).toContain( + 'dummy-project', + ); + }); +}); From ea78a028dbe56d71c42f50a5ba367ce0143699f4 Mon Sep 17 00:00:00 2001 From: ayush-that Date: Wed, 21 Oct 2026 11:19:11 +0530 Subject: [PATCH 2/4] test(ci): simplify symlink regression setup --- test/e2e/ci-init-symlinks.e2e.test.ts | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/test/e2e/ci-init-symlinks.e2e.test.ts b/test/e2e/ci-init-symlinks.e2e.test.ts index 71be477f..275ed4d0 100644 --- a/test/e2e/ci-init-symlinks.e2e.test.ts +++ b/test/e2e/ci-init-symlinks.e2e.test.ts @@ -24,8 +24,6 @@ afterEach(async () => { if (fixture) await rm(fixture, { recursive: true, force: true }); }); -// Cover escaping ancestors, final files and backups, internal links, dangling -// links, and a symlinked cwd. All requests stay on loopback with dummy credentials. describe.skipIf(process.platform === 'win32')('ci init symlink containment', () => { async function setup() { fixture = await realpath(await mkdtemp(join(tmpdir(), 'testsprite-ci-path-'))); @@ -44,9 +42,9 @@ describe.skipIf(process.platform === 'win32')('ci init symlink containment', () await new Promise(done => server.listen(0, '127.0.0.1', done)); const address = server.address() as { port: number }; try { - const env = { ...process.env, HOME: home, USERPROFILE: home }; + const env: NodeJS.ProcessEnv = { ...process.env, HOME: home, USERPROFILE: home }; for (const key of Object.keys(env)) { - if (key.startsWith('TESTSPRITE_')) delete env[key as keyof typeof env]; + if (key.startsWith('TESTSPRITE_')) delete env[key]; } return await execute( process.execPath, From 6c37aa51d248fc64791d9722d971f660121c2a1f Mon Sep 17 00:00:00 2001 From: ayush-that Date: Thu, 22 Oct 2026 10:23:37 +0530 Subject: [PATCH 3/4] test(ci): cover symlinked workflow ancestors without force --- test/e2e/ci-init-symlinks.e2e.test.ts | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/test/e2e/ci-init-symlinks.e2e.test.ts b/test/e2e/ci-init-symlinks.e2e.test.ts index 275ed4d0..40bee42e 100644 --- a/test/e2e/ci-init-symlinks.e2e.test.ts +++ b/test/e2e/ci-init-symlinks.e2e.test.ts @@ -87,13 +87,15 @@ describe.skipIf(process.platform === 'win32')('ci init symlink containment', () } } - for (const location of [ - '.github', - '.github/workflows', - '.github/workflows/testsprite.yml', - '.github/workflows/testsprite.yml.bak', - ]) { - it(`refuses external ${location} before writing`, async () => { + for (const [location, force] of [ + ['.github', false], + ['.github', true], + ['.github/workflows', false], + ['.github/workflows', true], + ['.github/workflows/testsprite.yml', true], + ['.github/workflows/testsprite.yml.bak', true], + ] as const) { + it(`refuses external ${location} ${force ? 'with' : 'without'} --force before writing`, async () => { const { repo, outside, home } = await setup(); const target = location.endsWith('.yml') || location.endsWith('.bak') @@ -105,7 +107,7 @@ describe.skipIf(process.platform === 'win32')('ci init symlink containment', () if (location.endsWith('.bak')) await writeFile(join(repo, '.github/workflows/testsprite.yml'), 'original workflow'); await symlink(target, link); - const result = await run(repo, home, true); + const result = await run(repo, home, force); expect(result.code, result.stderr).toBe(5); expect(result.stderr).toContain(target); expect(await readdir(outside)).toEqual(target === outside ? [] : ['sentinel']); From 1e3eb63f8f18a2eba98626c393c5cbefeadf85b0 Mon Sep 17 00:00:00 2001 From: ayush-that Date: Thu, 22 Oct 2026 14:19:29 +0530 Subject: [PATCH 4/4] test(ci): run workflow symlink checks without sockets --- test/e2e/ci-init-symlinks.e2e.test.ts | 92 ++++++++++++--------------- test/helpers/ci-init-fetch.mjs | 12 ++++ 2 files changed, 53 insertions(+), 51 deletions(-) create mode 100644 test/helpers/ci-init-fetch.mjs diff --git a/test/e2e/ci-init-symlinks.e2e.test.ts b/test/e2e/ci-init-symlinks.e2e.test.ts index 40bee42e..acd205ab 100644 --- a/test/e2e/ci-init-symlinks.e2e.test.ts +++ b/test/e2e/ci-init-symlinks.e2e.test.ts @@ -1,6 +1,5 @@ /* eslint-disable security/detect-non-literal-fs-filename -- Every filesystem path belongs to the isolated mkdtemp fixture; symlinks deliberately exercise the real CLI boundary. */ import { execFile } from 'node:child_process'; -import { createServer } from 'node:http'; import { mkdtemp, mkdir, @@ -17,6 +16,7 @@ import { promisify } from 'node:util'; import { afterEach, describe, expect, it } from 'vitest'; const binary = resolve('dist/index.js'); +const fetchFixture = resolve('test/helpers/ci-init-fetch.mjs'); const execute = promisify(execFile); let fixture: string; @@ -34,57 +34,47 @@ describe.skipIf(process.platform === 'win32')('ci init symlink containment', () return { repo, outside, home }; } - async function run(cwd: string, home: string, force = false, workflowPath?: string) { - const server = createServer((_req, res) => { - res.setHeader('content-type', 'application/json'); - res.end(JSON.stringify({ environments: [{ name: 'staging', isDefault: true }] })); - }); - await new Promise(done => server.listen(0, '127.0.0.1', done)); - const address = server.address() as { port: number }; - try { - const env: NodeJS.ProcessEnv = { ...process.env, HOME: home, USERPROFILE: home }; - for (const key of Object.keys(env)) { - if (key.startsWith('TESTSPRITE_')) delete env[key]; - } - return await execute( - process.execPath, - [ - binary, - '--endpoint-url', - `http://127.0.0.1:${address.port}`, - 'ci', - 'init', - 'github', - '--project', - 'dummy-project', - '--env', - 'staging', - ...(force ? ['--force'] : []), - ...(workflowPath ? ['--path', workflowPath] : []), - ], - { - cwd, - env: { - ...env, - TESTSPRITE_API_KEY: 'sk-user-local-only', - TESTSPRITE_NO_TELEMETRY: '1', - TESTSPRITE_NO_UPDATE_NOTIFIER: '1', - }, - timeout: 10000, - }, - ).then( - result => ({ ...result, code: 0 }), - error => ({ - stdout: error.stdout as string, - stderr: error.stderr as string, - code: error.code as number, - }), - ); - } finally { - await new Promise((done, reject) => - server.close(error => (error ? reject(error) : done())), - ); + function run(cwd: string, home: string, force = false, workflowPath?: string) { + const env: NodeJS.ProcessEnv = { ...process.env, HOME: home, USERPROFILE: home }; + for (const key of Object.keys(env)) { + if (key.startsWith('TESTSPRITE_')) delete env[key]; } + return execute( + process.execPath, + [ + '--import', + fetchFixture, + binary, + '--endpoint-url', + 'http://127.0.0.1:9', + 'ci', + 'init', + 'github', + '--project', + 'dummy-project', + '--env', + 'staging', + ...(force ? ['--force'] : []), + ...(workflowPath ? ['--path', workflowPath] : []), + ], + { + cwd, + env: { + ...env, + TESTSPRITE_API_KEY: 'sk-user-local-only', + TESTSPRITE_NO_TELEMETRY: '1', + TESTSPRITE_NO_UPDATE_NOTIFIER: '1', + }, + timeout: 10000, + }, + ).then( + result => ({ ...result, code: 0 }), + error => ({ + stdout: error.stdout as string, + stderr: error.stderr as string, + code: error.code as number, + }), + ); } for (const [location, force] of [ diff --git a/test/helpers/ci-init-fetch.mjs b/test/helpers/ci-init-fetch.mjs new file mode 100644 index 00000000..18c8409b --- /dev/null +++ b/test/helpers/ci-init-fetch.mjs @@ -0,0 +1,12 @@ +// Preload in the CLI child so the real command runs without opening API sockets. +globalThis.fetch = async (url, init) => { + if ( + url !== 'http://127.0.0.1:9/api/cli/v1/projects/dummy-project/env' || + init?.method !== 'GET' + ) { + throw new Error(`Unexpected request in ci init fixture: ${init?.method} ${url}`); + } + return new Response(JSON.stringify({ environments: [{ name: 'staging', isDefault: true }] }), { + headers: { 'content-type': 'application/json' }, + }); +};