From 387d08de2773da81e7379ba9f0ae213a4fbff538 Mon Sep 17 00:00:00 2001 From: Ali Mohammad Date: Fri, 16 Oct 2026 23:45:28 +0300 Subject: [PATCH] fix(workspace): check the deny list through links; write files atomically A symlink or junction inside the workspace pointing at .git let edit_file rewrite .git/config and write_file create files in .git: the deny check only looked at the typed path. The file tools now also check where a path really leads, via the new optional FsProvider.realPath (NodeWorkspace has it), for reads, writes, edits, list_dir and glob/grep link paths. NodeWorkspace.writeFile now writes a temp file and renames it into place, keeping the replaced file's mode, so a killed process cannot leave a truncated file. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 2 + api/index.api.md | 3 +- api/tools.api.md | 3 +- docs/workspace-tools.md | 22 +++- llms-full.txt | 22 +++- src/storage/atomicFile.ts | 14 ++- src/tools/workspace/NodeWorkspace.test.ts | 146 ++++++++++++++++++++++ src/tools/workspace/NodeWorkspace.ts | 26 +++- src/tools/workspace/fsTools.ts | 59 ++++++++- src/tools/workspace/paths.ts | 15 +++ src/tools/workspace/types.ts | 9 ++ 11 files changed, 298 insertions(+), 23 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 953774a7..43d3d23d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,8 @@ opt-out. - `lousho eval` without vitest installed now says it runs `*.eval.ts` files through vitest, the directory it searched from, and the install command; [Evals](docs/evals.md) states the requirement up front. ### Fixed +- The file tools' `deny` list (`.git` by default) is now also checked where a path really leads: a symlink or Windows junction inside the workspace pointing at `.git` or into it (`gitj -> .git`) let `edit_file gitj/config` rewrite `.git/config` (for example `core.hooksPath`, which runs code on the next git command) and `write_file gitj/PWNED` create files in `.git`. Reads, writes, edits, `list_dir`, and `glob`/`grep` given such a link as `path` are now refused. Providers with links can implement the new optional `FsProvider.realPath(path)`; `NodeWorkspace` does. See [Workspace tools](docs/workspace-tools.md#security-model). +- `NodeWorkspace.writeFile` (and so `write_file` / `edit_file`) writes a temp file in the same directory and renames it into place, so a process killed mid-write no longer leaves a truncated file. A replaced file keeps its permission bits; a read-only file is still refused. - MCP tool schemas with OpenAPI's `nullable: true` (any type) now accept `null`; the model's valid `null` was rejected with `ToolArgumentsValidationError`. - A failed MCP tool call (server exited, timeout, protocol error) now rejects with `LOUSHO_MCP_CALL_FAILED`, naming `__` with the MCP error as `cause`, instead of a bare `MCP error -32000: Connection closed`. [MCP](docs/mcp.md) now says stdio `env` is merged onto the MCP SDK's default allow-list, not `process.env`. - Flows (consumer harness): a `{{variable}}` holding an object (a `toolCall` result, an MCP result) no longer reaches a prompt as `[object Object]`: an object whose only field is `text` (an MCP tool result) is that text, a raw MCP `{ content }` result its text parts, and any other object or array its JSON. Placeholders take dotted paths (`{{res.user.name}}`, `{{list.0}}`, own properties only), also as whole tool-argument values. An `evaluator` step stores its result in `outputVariable`. See [Template values](docs/flows.md#template-values). diff --git a/api/index.api.md b/api/index.api.md index 0e7b670f..0facb001 100644 --- a/api/index.api.md +++ b/api/index.api.md @@ -2578,6 +2578,7 @@ export interface FsProvider { readdir(path: string): Promise; readFile(path: string): Promise; readFileBytes?(path: string): Promise; + realPath?(path: string): Promise; rm(path: string, options?: { recursive?: boolean; }): Promise; @@ -4098,6 +4099,7 @@ export class NodeWorkspace implements Workspace { readFile(path: string): Promise; // (undocumented) readFileBytes(path: string): Promise; + realPath(path: string): Promise; rm(path: string, options?: { recursive?: boolean; }): Promise; @@ -4105,7 +4107,6 @@ export class NodeWorkspace implements Workspace { readonly shell: string; // (undocumented) stat(path: string): Promise; - // (undocumented) writeFile(path: string, content: string): Promise; } diff --git a/api/tools.api.md b/api/tools.api.md index 4803f284..c1e70dd9 100644 --- a/api/tools.api.md +++ b/api/tools.api.md @@ -857,6 +857,7 @@ export interface FsProvider { readdir(path: string): Promise; readFile(path: string): Promise; readFileBytes?(path: string): Promise; + realPath?(path: string): Promise; rm(path: string, options?: { recursive?: boolean; }): Promise; @@ -1531,6 +1532,7 @@ export class NodeWorkspace implements Workspace { readFile(path: string): Promise; // (undocumented) readFileBytes(path: string): Promise; + realPath(path: string): Promise; rm(path: string, options?: { recursive?: boolean; }): Promise; @@ -1538,7 +1540,6 @@ export class NodeWorkspace implements Workspace { readonly shell: string; // (undocumented) stat(path: string): Promise; - // (undocumented) writeFile(path: string, content: string): Promise; } diff --git a/docs/workspace-tools.md b/docs/workspace-tools.md index c06251d6..2b59e242 100644 --- a/docs/workspace-tools.md +++ b/docs/workspace-tools.md @@ -241,6 +241,12 @@ path. This means: - `glob` and `grep` never follow links while walking the tree. - `rm` on a link removes the link itself and leaves its target alone. +**Writes are atomic.** `NodeWorkspace` writes a file (for `write_file` and +`edit_file` too) to a temp file in the same directory, flushes it and renames +it over the target, so a process killed mid-write leaves the old file or the +new one, never half of it. A replaced file keeps its permission bits, and a +read-only file is still refused. + **The file tools keep out of `.git`.** A checkout often has credentials in `.git/config`: `actions/checkout` keeps the job's token there as `http.https://github.com/.extraheader = AUTHORIZATION: basic ...`, and a @@ -264,9 +270,19 @@ const workspace = new NodeWorkspace({ root: '.' }); const tools = createFsTools(workspace, { readOnly: true, deny: ['.git', '.env'] }); ``` -The check is by name, so a symlink elsewhere in the workspace that points -into `.git` is not caught (`glob` and `grep` never follow links, but -`read_file` on the link does). **The `shell` tool is not covered:** a command +Links into `.git` are refused too. With `NodeWorkspace` (or any provider that +implements `realPath`), the check also runs on where a path really leads, so a +symlink or Windows junction elsewhere in the workspace that points at `.git` +or into it (`gitj -> .git`, `hooks -> .git/hooks`, `cfg -> .git/config`) +cannot be used to read, write or edit what is behind it: `read_file +gitj/config`, `edit_file gitj/config` and `write_file gitj/PWNED` are tool +errors, and so are `list_dir`, `glob` and `grep` given the link as their +`path` (`grep` with `path: 'gitj'` or `path: 'gitj/config'`). `glob` and +`grep` never follow links while walking the tree, so a search from the root +does not reach `.git` through a link either. A provider without `realPath` +(`MemoryWorkspace` has no links) gets the name check only. + +**The `shell` tool is not covered:** a command is free text, so it cannot be path-filtered, and `cat .git/config` or `git config --get-regexp extraheader` still works. Git commands that coding agents need (`git status`, `git diff`) go through the shell tool, so keep its diff --git a/llms-full.txt b/llms-full.txt index e55d7f23..32a6eb06 100644 --- a/llms-full.txt +++ b/llms-full.txt @@ -20017,6 +20017,12 @@ path. This means: - `glob` and `grep` never follow links while walking the tree. - `rm` on a link removes the link itself and leaves its target alone. +**Writes are atomic.** `NodeWorkspace` writes a file (for `write_file` and +`edit_file` too) to a temp file in the same directory, flushes it and renames +it over the target, so a process killed mid-write leaves the old file or the +new one, never half of it. A replaced file keeps its permission bits, and a +read-only file is still refused. + **The file tools keep out of `.git`.** A checkout often has credentials in `.git/config`: `actions/checkout` keeps the job's token there as `http.https://github.com/.extraheader = AUTHORIZATION: basic ...`, and a @@ -20040,9 +20046,19 @@ const workspace = new NodeWorkspace({ root: '.' }); const tools = createFsTools(workspace, { readOnly: true, deny: ['.git', '.env'] }); ``` -The check is by name, so a symlink elsewhere in the workspace that points -into `.git` is not caught (`glob` and `grep` never follow links, but -`read_file` on the link does). **The `shell` tool is not covered:** a command +Links into `.git` are refused too. With `NodeWorkspace` (or any provider that +implements `realPath`), the check also runs on where a path really leads, so a +symlink or Windows junction elsewhere in the workspace that points at `.git` +or into it (`gitj -> .git`, `hooks -> .git/hooks`, `cfg -> .git/config`) +cannot be used to read, write or edit what is behind it: `read_file +gitj/config`, `edit_file gitj/config` and `write_file gitj/PWNED` are tool +errors, and so are `list_dir`, `glob` and `grep` given the link as their +`path` (`grep` with `path: 'gitj'` or `path: 'gitj/config'`). `glob` and +`grep` never follow links while walking the tree, so a search from the root +does not reach `.git` through a link either. A provider without `realPath` +(`MemoryWorkspace` has no links) gets the name check only. + +**The `shell` tool is not covered:** a command is free text, so it cannot be path-filtered, and `cat .git/config` or `git config --get-regexp extraheader` still works. Git commands that coding agents need (`git status`, `git diff`) go through the shell tool, so keep its diff --git a/src/storage/atomicFile.ts b/src/storage/atomicFile.ts index dea8a82a..40c928f1 100644 --- a/src/storage/atomicFile.ts +++ b/src/storage/atomicFile.ts @@ -73,10 +73,11 @@ async function syncDirectory(dir: string): Promise { } } -/** Writes and flushes `content` to a new file `temp`. */ -async function writeSynced(temp: string, content: string): Promise { +/** Writes and flushes `content` to a new file `temp`, with permission bits `mode` when given. */ +async function writeSynced(temp: string, content: string, mode: number | undefined): Promise { const handle = await open(temp, 'wx'); try { + if (mode !== undefined) await handle.chmod(mode); await handle.writeFile(content, 'utf8'); await handle.sync(); } finally { @@ -86,17 +87,18 @@ async function writeSynced(temp: string, content: string): Promise { /** * Writes `content` to `file` atomically and durably (see the module comment); - * creates the directory when it is missing. + * creates the directory when it is missing. `mode` sets the new file's + * permission bits (to keep those of the file it replaces). */ -export async function writeFileAtomic(file: string, content: string): Promise { +export async function writeFileAtomic(file: string, content: string, { mode }: { mode?: number } = {}): Promise { const temp = `${file}.${process.pid}.${randomUUID()}.tmp`; try { try { - await writeSynced(temp, content); + await writeSynced(temp, content, mode); } catch (error) { if ((error as NodeJS.ErrnoException).code !== 'ENOENT') throw error; await mkdir(dirname(file), { recursive: true }); - await writeSynced(temp, content); + await writeSynced(temp, content, mode); } await renameWithRetry(temp, file); } catch (error) { diff --git a/src/tools/workspace/NodeWorkspace.test.ts b/src/tools/workspace/NodeWorkspace.test.ts index b38d148c..21471c65 100644 --- a/src/tools/workspace/NodeWorkspace.test.ts +++ b/src/tools/workspace/NodeWorkspace.test.ts @@ -233,6 +233,152 @@ describe('NodeWorkspace file system', () => { expect(fs.existsSync(path.join(rootDir, 'src', 'a.ts'))).toBe(true); }); }); + + // A link inside the workspace pointing at .git let edit_file set core.hooksPath in .git/config. + describe('deny list through links', () => { + const CONFIG = '[core]\n\tbare = false\n'; + let gitRoot: string; + let gitWs: NodeWorkspace; + let gitTools: Record; + + beforeAll(() => { + gitRoot = path.join(tmp, 'git-root'); + fs.mkdirSync(path.join(gitRoot, '.git', 'hooks'), { recursive: true }); + fs.writeFileSync(path.join(gitRoot, '.git', 'config'), CONFIG); + fs.writeFileSync(path.join(gitRoot, '.git', 'hooks', 'pre-commit.sample'), 'TOKEN\n'); + fs.writeFileSync(path.join(gitRoot, 'readme.md'), 'hello\n'); + gitWs = new NodeWorkspace({ root: gitRoot }); + gitTools = Object.fromEntries(createFsTools(gitWs).map((t) => [t.name, t])); + }); + + const config = () => fs.readFileSync(path.join(gitRoot, '.git', 'config'), 'utf8'); + + /** Every way the file tools could reach `.git` through the directory link `name` (to `.git`) or `sub` (to `.git/hooks`). */ + async function expectRefused(name: string, sub: string): Promise { + const denied = /is denied: it resolves through a link/; + await expect(exec(gitTools.read_file, { path: `${name}/config` })).rejects.toThrow(denied); + await expect(exec(gitTools.edit_file, { path: `${name}/config`, old_string: 'bare = false', new_string: 'hooksPath = /tmp/x' })).rejects.toThrow(denied); + await expect(exec(gitTools.write_file, { path: `${name}/PWNED`, content: 'x' })).rejects.toThrow(denied); + await expect(exec(gitTools.write_file, { path: `${name}/new/dir/PWNED`, content: 'x' })).rejects.toThrow(denied); + await expect(exec(gitTools.write_file, { path: `${sub}/pre-commit`, content: 'x' })).rejects.toThrow(denied); + await expect(exec(gitTools.list_dir, { path: name })).rejects.toThrow(denied); + await expect(exec(gitTools.glob, { pattern: '**/*', path: name })).rejects.toThrow(denied); + await expect(exec(gitTools.grep, { pattern: 'TOKEN', path: name })).rejects.toThrow(denied); + await expect(exec(gitTools.grep, { pattern: 'bare', path: `${name}/config` })).rejects.toThrow(denied); + await expect(exec(gitTools.grep, { pattern: 'TOKEN', path: sub })).rejects.toThrow(denied); + // A walk from the root never follows the link. + expect(await exec(gitTools.grep, { pattern: 'TOKEN' })).toBe('No matches for "TOKEN".'); + expect(config()).toBe(CONFIG); + expect(fs.existsSync(path.join(gitRoot, '.git', 'PWNED'))).toBe(false); + expect(fs.existsSync(path.join(gitRoot, '.git', 'new'))).toBe(false); + expect(fs.existsSync(path.join(gitRoot, '.git', 'hooks', 'pre-commit'))).toBe(false); + } + + it('refuses reads, writes, edits, listings and searches through a junction to .git (or a directory symlink elsewhere)', async () => { + const type = IS_WINDOWS ? 'junction' : 'dir'; + fs.symlinkSync(path.join(gitRoot, '.git'), path.join(gitRoot, 'gitj'), type); + fs.symlinkSync(path.join(gitRoot, '.git', 'hooks'), path.join(gitRoot, 'hooksj'), type); + try { + await expectRefused('gitj', 'hooksj'); + } finally { + fs.rmSync(path.join(gitRoot, 'gitj')); + fs.rmSync(path.join(gitRoot, 'hooksj')); + } + }); + + it('refuses the same through a directory symlink', async (ctx) => { + const refused = trySymlink(path.join(gitRoot, '.git'), path.join(gitRoot, 'gits'), 'dir'); + if (refused) { + console.warn(`Skipping .git directory-symlink test: the OS refused to create a symlink (${refused}).`); + ctx.skip(); + return; + } + fs.symlinkSync(path.join(gitRoot, '.git', 'hooks'), path.join(gitRoot, 'hookss'), 'dir'); + try { + await expectRefused('gits', 'hookss'); + } finally { + fs.rmSync(path.join(gitRoot, 'gits')); + fs.rmSync(path.join(gitRoot, 'hookss')); + } + }); + + it('refuses a file symlink to .git/config', async (ctx) => { + const refused = trySymlink(path.join(gitRoot, '.git', 'config'), path.join(gitRoot, 'cfg'), 'file'); + if (refused) { + console.warn(`Skipping .git file-symlink test: the OS refused to create a symlink (${refused}).`); + ctx.skip(); + return; + } + try { + await expect(exec(gitTools.read_file, { path: 'cfg' })).rejects.toThrow(/is denied/); + await expect(exec(gitTools.edit_file, { path: 'cfg', old_string: 'bare', new_string: 'x' })).rejects.toThrow(/is denied/); + await expect(exec(gitTools.write_file, { path: 'cfg', content: 'x' })).rejects.toThrow(/is denied/); + expect(config()).toBe(CONFIG); + } finally { + fs.rmSync(path.join(gitRoot, 'cfg')); + } + }); + + it.runIf(IS_WINDOWS)('refuses a junction whose target is spelled .GIT', async () => { + fs.symlinkSync(path.join(gitRoot, '.GIT'), path.join(gitRoot, 'upper'), 'junction'); + try { + await expect(exec(gitTools.write_file, { path: 'upper/PWNED', content: 'x' })).rejects.toThrow(/is denied/); + await expect(exec(gitTools.read_file, { path: 'upper/config' })).rejects.toThrow(/is denied/); + } finally { + fs.rmSync(path.join(gitRoot, 'upper')); + } + }); + + it('keeps links to allowed places working, and deny: [] opens .git through a link too', async () => { + const type = IS_WINDOWS ? 'junction' : 'dir'; + fs.symlinkSync(path.join(gitRoot, '.git'), path.join(gitRoot, 'gitj'), type); + try { + expect(await gitWs.realPath('gitj/config')).toBe('.git/config'); + expect(await gitWs.realPath('gitj/not/yet')).toBe('.git/not/yet'); + expect(await gitWs.realPath('.')).toBe('.'); + const open = Object.fromEntries(createFsTools(gitWs, { deny: [] }).map((t) => [t.name, t])); + expect(await exec(open.read_file, { path: 'gitj/config' })).toMatch(/bare = false/); + expect(await exec(gitTools.read_file, { path: 'readme.md' })).toBe(' 1\thello'); + } finally { + fs.rmSync(path.join(gitRoot, 'gitj')); + } + }); + }); + + describe('writeFile replaces the file atomically', () => { + it('renames a new file into place: a hard link to the old file keeps the old content, and no temp file is left', async () => { + await ws.writeFile('atomic/a.txt', 'old'); + fs.linkSync(path.join(rootDir, 'atomic', 'a.txt'), path.join(rootDir, 'atomic', 'old-link.txt')); + await ws.writeFile('atomic/a.txt', 'new'); + expect(fs.readFileSync(path.join(rootDir, 'atomic', 'a.txt'), 'utf8')).toBe('new'); + expect(fs.readFileSync(path.join(rootDir, 'atomic', 'old-link.txt'), 'utf8')).toBe('old'); + expect(fs.readdirSync(path.join(rootDir, 'atomic')).sort()).toEqual(['a.txt', 'old-link.txt']); + }); + + it.skipIf(IS_WINDOWS)('keeps the permission bits of the file it replaces', async () => { + const file = path.join(rootDir, 'atomic', 'run.sh'); + await ws.writeFile('atomic/run.sh', 'echo 1'); + fs.chmodSync(file, 0o750); + await ws.writeFile('atomic/run.sh', 'echo 2'); + expect(fs.statSync(file).mode & 0o777).toBe(0o750); + }); + + it('still refuses a read-only file and a directory', async () => { + const file = path.join(rootDir, 'atomic', 'locked.txt'); + await ws.writeFile('atomic/locked.txt', 'keep'); + fs.chmodSync(file, 0o444); + try { + // root may write any file, so there is nothing to refuse. + if (process.getuid?.() !== 0) { + await expect(ws.writeFile('atomic/locked.txt', 'x')).rejects.toThrow(/Permission denied|not permitted/); + expect(fs.readFileSync(file, 'utf8')).toBe('keep'); + } + } finally { + fs.chmodSync(file, 0o644); + } + await expect(ws.writeFile('atomic', 'x')).rejects.toThrow('atomic is a directory, not a file.'); + }); + }); }); describe('NodeWorkspace shell', () => { diff --git a/src/tools/workspace/NodeWorkspace.ts b/src/tools/workspace/NodeWorkspace.ts index c7c25d18..4073f600 100644 --- a/src/tools/workspace/NodeWorkspace.ts +++ b/src/tools/workspace/NodeWorkspace.ts @@ -1,7 +1,7 @@ /** * NodeWorkspace: a Workspace over a real directory. */ -import { promises as fsp, realpathSync, statSync, type Dirent, type Stats } from 'node:fs'; +import { constants as fsConstants, promises as fsp, realpathSync, statSync, type Dirent, type Stats } from 'node:fs'; import * as nodePath from 'node:path'; import type { ShellExecOptions, @@ -15,6 +15,7 @@ import { normalizeWorkspacePath, WorkspaceError } from './paths'; import { runShellCommand } from './nodeProcess'; import { commandEnv } from '../../security/commandEnv'; import { ConfigurationError } from '../../execution/errors'; +import { writeFileAtomic } from '../../storage/atomicFile'; const DEFAULT_MAX_OUTPUT_BYTES = 1_000_000; @@ -139,17 +140,36 @@ export class NodeWorkspace implements Workspace { return fsp.readFile(abs).catch((error) => Promise.reject(toWorkspaceError(error, rel))); } + /** + * Writes through a temp file in the same directory that is renamed over the + * target, so a killed process never leaves a truncated file. An existing + * file keeps its permission bits, and one that is not writable is refused + * as before. + */ async writeFile(path: string, content: string): Promise { const { rel, abs } = await this.resolve(path); if (abs === this.root) throw new WorkspaceError('Cannot write a file at the workspace root path ".".'); try { + const existing = await fsp.stat(abs).catch((error: unknown) => (isMissing(error) ? undefined : Promise.reject(error))); + if (existing?.isDirectory()) throw Object.assign(new Error('directory'), { code: 'EISDIR' }); + if (existing) await fsp.access(abs, fsConstants.W_OK); await fsp.mkdir(nodePath.dirname(abs), { recursive: true }); - await fsp.writeFile(abs, content, 'utf8'); + await writeFileAtomic(abs, content, { mode: existing ? existing.mode & 0o7777 : undefined }); } catch (error) { - throw toWorkspaceError(error, rel); + throw toWorkspaceError((error as { cause?: unknown } | undefined)?.cause ?? error, rel); } } + /** + * The workspace-relative path `path` resolves to once symlinks and + * junctions are followed (`'.'` for the root); see `FsProvider.realPath`. + */ + async realPath(path: string): Promise { + const { abs } = await this.resolve(path); + const relative = nodePath.relative(this.root, abs); + return relative === '' ? '.' : relative.split(nodePath.sep).join('/'); + } + async stat(path: string): Promise { const { rel, abs } = await this.resolve(path); try { diff --git a/src/tools/workspace/fsTools.ts b/src/tools/workspace/fsTools.ts index 9ab2d3d1..ca570a95 100644 --- a/src/tools/workspace/fsTools.ts +++ b/src/tools/workspace/fsTools.ts @@ -4,7 +4,7 @@ import { z } from 'zod'; import { defineTool, type DefinedTool } from '../defineTool'; import type { FsProvider } from './types'; -import { createDenyTest } from './paths'; +import { assertResolvedPathAllowed, createDenyTest, type DeniedSegment } from './paths'; import { allowedPath, editFile, globFiles, grepFiles, listDirectory, prepareEdit, readNumberedLines, type FsLimits } from './fsOperations'; import { ConfigurationError } from '../../execution/errors'; import type { WorkspaceCheckpoints } from './checkpoints'; @@ -102,7 +102,10 @@ export interface FsToolsOptions { * (`http..extraheader = AUTHORIZATION: basic ...`), and a worktree's * `.git` file names a host path. A path with a denied segment (`.git/config`, * `./.GIT`, `a/../.git/`, `sub\.git\HEAD`) is a `WorkspaceError`; `glob`, - * `grep` and `list_dir` leave such entries out. Names compare + * `grep` and `list_dir` leave such entries out. With a provider that has + * `realPath` (`NodeWorkspace`), a path that resolves through a symlink or + * junction to a denied entry (`gitj/config` with `gitj -> .git`) is refused + * too, for reads, writes and edits alike. Names compare * case-insensitively. Pass `[]` to allow everything. The `shell` tool is not * covered: a command can still run `cat .git/config`. */ @@ -140,6 +143,46 @@ function resolveDeny(deny: readonly string[] | undefined): readonly string[] { return deny; } +/** A provider whose every path is also checked where it really leads, and that check on its own. */ +interface LinkGuard { + fs: FsProvider; + /** Rejects when `path` resolves through a link to a denied entry. */ + check(path: string): Promise; +} + +/** + * `fs` with each path also checked against the deny list where it really + * leads: the lexical check sees `gitj/config`, but a symlink or junction + * `gitj -> .git` makes that `.git/config`. Only a provider with `realPath` + * (one that has links) needs it; `fs` itself is used otherwise, or when + * nothing is denied. + */ +function guardLinks(fs: FsProvider, isDenied: DeniedSegment, denyNames: readonly string[]): LinkGuard { + const realPath = fs.realPath?.bind(fs); + if (!realPath || denyNames.length === 0) return { fs, check: async () => undefined }; + const check = async (path: string) => assertResolvedPathAllowed(path, await realPath(path), isDenied); + const guard = + (op: (path: string, ...rest: A) => Promise) => + async (path: string, ...rest: A): Promise => { + await check(path); + return op.call(fs, path, ...rest); + }; + const guarded: FsProvider = { + readFile: guard(fs.readFile), + writeFile: guard(fs.writeFile), + stat: guard(fs.stat), + readdir: guard(fs.readdir), + mkdir: guard(fs.mkdir), + rm: guard(fs.rm), + glob: fs.glob?.bind(fs), + getMode: fs.getMode && guard(fs.getMode), + chmod: fs.chmod && guard(fs.chmod), + readFileBytes: fs.readFileBytes && guard(fs.readFileBytes), + realPath, + }; + return { fs: guarded, check }; +} + /** Caps a tool's text output at `maxOutputChars`, saying so. */ function capChars(text: string, limits: FsLimits): string { if (text.length <= limits.maxOutputChars) return text; @@ -186,7 +229,8 @@ function readOnlyTools(fs: FsProvider, limits: FsLimits, approvals: FsToolApprov ]; } -function mutatingTools(fs: FsProvider, limits: FsLimits, approvals: FsToolApprovals, checkpoints: WorkspaceCheckpoints | undefined): DefinedTool[] { +function mutatingTools(links: LinkGuard, limits: FsLimits, approvals: FsToolApprovals, checkpoints: WorkspaceCheckpoints | undefined): DefinedTool[] { + const fs = links.fs; return [ defineTool({ name: 'write_file', @@ -196,6 +240,8 @@ function mutatingTools(fs: FsProvider, limits: FsLimits, approvals: FsToolApprov needsApproval: approvals.write_file, async execute({ path, content }, ctx) { const target = allowedPath(path, limits); + // Before `track`, which would read the old content into a backup. + await links.check(target); const write = async () => { await fs.writeFile(target, content); return content; @@ -212,7 +258,7 @@ function mutatingTools(fs: FsProvider, limits: FsLimits, approvals: FsToolApprov input: editFileInput, needsApproval: approvals.edit_file, async execute(args, ctx) { - allowedPath(args.path, limits); + await links.check(allowedPath(args.path, limits)); if (!checkpoints) return editFile(fs, args); let message = ''; await checkpoints.track(ctx, args.path, async () => { @@ -251,7 +297,8 @@ function mutatingTools(fs: FsProvider, limits: FsLimits, approvals: FsToolApprov */ export function createFsTools(fs: FsProvider, options: FsToolsOptions = {}): DefinedTool[] { const limits = resolveLimits(options); + const links = guardLinks(fs, limits.isDenied ?? (() => undefined), resolveDeny(options.deny)); const approvals = options.needsApproval ?? {}; - const tools = readOnlyTools(fs, limits, approvals); - return options.readOnly ? tools : [tools[0], ...mutatingTools(fs, limits, approvals, options.checkpoints), ...tools.slice(1)]; + const tools = readOnlyTools(links.fs, limits, approvals); + return options.readOnly ? tools : [tools[0], ...mutatingTools(links, limits, approvals, options.checkpoints), ...tools.slice(1)]; } diff --git a/src/tools/workspace/paths.ts b/src/tools/workspace/paths.ts index 035b78f6..9af8720a 100644 --- a/src/tools/workspace/paths.ts +++ b/src/tools/workspace/paths.ts @@ -153,3 +153,18 @@ export function normalizeAllowedPath(path: string, isDenied: DeniedSegment): str } return normalized; } + +/** + * Refuses `path` when the location it resolves to (`realPath`, the + * workspace-relative path after following symlinks and junctions) has a denied + * segment: a link `gitj -> .git` must not open `.git/config` as `gitj/config`. + */ +export function assertResolvedPathAllowed(path: string, realPath: string, isDenied: DeniedSegment): void { + const segment = isDenied(realPath); + if (segment !== undefined) { + throw new WorkspaceError( + `Access to ${JSON.stringify(path)} is denied: it resolves through a link to ${JSON.stringify(realPath)}, and the file tools never open ` + + `${JSON.stringify(segment)}, which can hold credentials (for example CI checkout tokens in .git/config). Use files elsewhere in the workspace.` + ); + } +} diff --git a/src/tools/workspace/types.ts b/src/tools/workspace/types.ts index 5fd96170..5c9dcbef 100644 --- a/src/tools/workspace/types.ts +++ b/src/tools/workspace/types.ts @@ -84,6 +84,15 @@ export interface FsProvider { * U+FFFD in place of every byte it could not decode. `NodeWorkspace` has it. */ readFileBytes?(path: string): Promise; + /** + * Optional: the workspace-relative, `/`-separated path `path` resolves to + * once symlinks and junctions are followed (`'.'` for the root); for a path + * that does not exist yet, its deepest existing ancestor's real path plus + * the rest. A backend with links should implement it: the file tools check + * their `deny` list against it too, so a link `gitj -> .git` cannot open + * `.git/config` as `gitj/config`. `NodeWorkspace` has it. + */ + realPath?(path: string): Promise; } /** Options for {@link ShellProvider.exec}. */