From d4084ef803c4cfb9b241e5545b2ff236ceb2603e Mon Sep 17 00:00:00 2001 From: Jack Chen Date: Fri, 16 Oct 2026 10:42:32 +0800 Subject: [PATCH] fix(core): resolve dangling symlinks in the filesystem sandbox check (#626) * fix(core): resolve dangling symlinks in the filesystem sandbox check The sandbox resolver treated a dangling symlink as a path that does not exist yet, so the containment check passed on the link's own location and file_write then created the link's target, which could be outside the agent's working directory. The resolver now follows a dangling symlink to the target it names, rejects targets outside the root, keeps in-root targets writable, and bounds resolution at 40 hops so a loop fails the check. Addresses GHSA-h42w-g3ch-rjjm (CVE-2026-108600). * fix(core): fail the sandbox check when realpath reports ELOOP A symlink chain longer than the kernel's link limit made realpath fail with ELOOP, and the resolver then followed the first link by hand and resolved the rest, accepting a path the kernel itself refuses. ELOOP from realpath now fails the check, and the resolver's own hop limit stays as the bound for links it follows by hand. * fix(core): resolve symlink targets one component at a time Following a dangling symlink, the resolver normalized the target with path.resolve, which collapses a/.. lexically. When a is itself a symlink the kernel steps out of the directory a points to, so a target such as pivot/../created.txt resolved to a different file than a write through the link would create. The resolver now walks the target component by component, following symlinks before applying .., and fails when .. would step out of a missing directory, as the kernel does. The previous test for a self-referencing target built the link with path.join, which folded ghost/../self to self, so it only exercised a plain loop. Link targets in these tests are now joined without normalization. * fix(core): keep the directory requirement of a trailing separator in symlink targets A symlink target ending in a separator or "." names a directory, so the kernel refuses to open or create a file through it. The resolver dropped those components, and native realpath does the same on macOS, so a link to victim/ resolved to the file victim and file_write overwrote it. The resolver now requires an existing directory at the end of such a target, and confirms each realpath result with stat, which takes the same lookup as open, before trusting it. --- CHANGELOG.md | 9 + docs/sandbox-and-shell.md | 5 +- .../core/src/tool/built-in/path-safety.ts | 102 ++++++++- packages/core/tests/built-in-tools.test.ts | 202 +++++++++++++++++- 4 files changed, 308 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 57cac4c..4fb859a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,15 @@ ## Unreleased +### Security + +- `file_write` no longer writes outside the agent's working directory through a + dangling symlink (GHSA-h42w-g3ch-rjjm, CVE-2026-108600). The sandbox check + now resolves a dangling symlink to the target it names, so a link pointing + outside the root is rejected while one pointing inside it keeps working. The + check is shared by every built-in filesystem tool, and a symlink loop now + fails the check instead of reaching the filesystem call. + ## 1.21.2 - 2026-10-09 ### Changed diff --git a/docs/sandbox-and-shell.md b/docs/sandbox-and-shell.md index b4a3693..6ebbfee 100644 --- a/docs/sandbox-and-shell.md +++ b/docs/sandbox-and-shell.md @@ -39,7 +39,10 @@ Built-in filesystem tools are sandboxed to a per-agent working directory. Paths must be absolute and resolve inside that directory; symlinks are resolved before the check so they cannot escape the configured root. The resolved, symlink-free path is what reaches the `fs` API, which closes the window where a -symlink inside the candidate path could be swapped after the check. +symlink inside the candidate path could be swapped after the check. A dangling +symlink is resolved to the target it names, since writing through it would +create that target, so a link pointing outside the root is rejected like any +other outside path. A relative path is rejected outright, and a path that resolves outside the root returns an error `ToolResult` naming the root and the configuration fields that diff --git a/packages/core/src/tool/built-in/path-safety.ts b/packages/core/src/tool/built-in/path-safety.ts index d6e4616..3f9955a 100644 --- a/packages/core/src/tool/built-in/path-safety.ts +++ b/packages/core/src/tool/built-in/path-safety.ts @@ -1,5 +1,5 @@ -import { mkdir, realpath } from 'fs/promises' -import { basename, dirname, isAbsolute, join, relative, resolve } from 'path' +import { mkdir, readlink, realpath, stat } from 'fs/promises' +import { basename, dirname, isAbsolute, join, parse, relative, resolve, sep } from 'path' import type { ToolUseContext } from '../../types.js' /** @@ -84,7 +84,13 @@ export async function resolvePathWithinCwd( // Resolve symlinks all the way through the candidate. For paths that do // not yet exist (e.g. `file_write` creating a new file), resolve the // longest existing prefix and re-attach the non-existent suffix. - const realCandidate = await realpathTolerant(candidate) + let realCandidate: string + try { + realCandidate = await realpathTolerant(candidate, { hops: 0 }) + } catch (err) { + const message = err instanceof Error ? err.message : 'Unknown error' + return { ok: false, error: `Could not resolve path "${candidate}": ${message}` } + } if (!isWithin(realCandidate, realRoot)) { return outsideRoot(candidate, realRoot) @@ -101,14 +107,94 @@ function isWithin(candidate: string, root: string): boolean { return rel === '' || (!rel.startsWith('..') && !isAbsolute(rel)) } -async function realpathTolerant(path: string): Promise { +/** + * Bound on symlinks followed by hand, matching Linux MAXSYMLINKS. Chains that + * realpath walks itself are bounded by the kernel, which reports ELOOP. + */ +const MAX_SYMLINK_HOPS = 40 +const TOO_MANY_SYMLINKS = 'Too many levels of symbolic links' + +async function realpathTolerant(path: string, state: { hops: number }): Promise { try { - return await realpath(path) - } catch { + const real = await realpath(path) + // Native realpath accepts a symlink whose target ends in a separator even + // when that target is a file, which the kernel refuses to open. stat takes + // the same lookup as open, so a failure here sends the path through the + // manual resolution below. + await stat(path) + return real + } catch (err) { + // The kernel already refused this chain as too long or cyclic. Following + // it by hand would accept a path that no fs call could open as given. + if ((err as NodeJS.ErrnoException).code === 'ELOOP') { + throw new Error(TOO_MANY_SYMLINKS) + } const parent = dirname(path) if (parent === path) return path - const realParent = await realpathTolerant(parent) - return join(realParent, basename(path)) + const realParent = await realpathTolerant(parent, state) + const joined = join(realParent, basename(path)) + + // realpath also fails on a dangling symlink, which exists even though its + // target does not. Writing to it creates the target, so follow the link + // and resolve the target instead of treating the link as a new file. + let target: string + try { + target = await readlink(joined) + } catch { + return joined + } + if (++state.hops > MAX_SYMLINK_HOPS) { + throw new Error(TOO_MANY_SYMLINKS) + } + return resolveLinkTarget(realParent, target, state) + } +} + +const TARGET_SEPARATOR = sep === '\\' ? /[\\/]/ : /\// + +/** + * Resolve a symlink target the way the kernel does: one component at a time, + * starting from the directory holding the link when the target is relative. + * `path.resolve` would collapse `a/..` lexically, but when `a` is itself a + * symlink the kernel steps out of the directory `a` points to instead. + */ +async function resolveLinkTarget( + linkDir: string, + target: string, + state: { hops: number }, +): Promise { + const targetRoot = parse(target).root + const parts = target.slice(targetRoot.length).split(TARGET_SEPARATOR) + let current = targetRoot === '' ? linkDir : targetRoot + for (const part of parts) { + if (part === '' || part === '.') continue + if (part === '..') { + // `current` is symlink-free, so its lexical parent is its real parent, + // provided it exists. The kernel cannot step out of a missing directory. + await requireDirectory(current, target) + current = dirname(current) + continue + } + current = await realpathTolerant(join(current, part), state) + } + // A trailing separator or `.` names a directory, so the kernel refuses to + // open a file, or create one, through the link. + const last = parts[parts.length - 1] + if (last === '' || last === '.') { + await requireDirectory(current, target) + } + return current +} + +async function requireDirectory(path: string, target: string): Promise { + let isDirectory = false + try { + isDirectory = (await stat(path)).isDirectory() + } catch { + // Missing: rejected below. + } + if (!isDirectory) { + throw new Error(`Symlink target "${target}" requires "${path}" to be an existing directory`) } } diff --git a/packages/core/tests/built-in-tools.test.ts b/packages/core/tests/built-in-tools.test.ts index c9a3d37..4e2176b 100644 --- a/packages/core/tests/built-in-tools.test.ts +++ b/packages/core/tests/built-in-tools.test.ts @@ -1,6 +1,6 @@ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest' import { access, mkdir, mkdtemp, rm, symlink, writeFile, readFile } from 'fs/promises' -import { basename, join } from 'path' +import { basename, join, sep } from 'path' import { tmpdir } from 'os' import { detectSymlinkSupport } from './helpers/symlink-support.js' import { fileReadTool } from '../src/tool/built-in/file-read.js' @@ -562,6 +562,206 @@ describe('filesystem sandbox', () => { }) }) +describe.skipIf(!symlinksSupported)('filesystem sandbox symlinks', () => { + let root: string + let outside: string + let ctx: ToolUseContext + + beforeEach(async () => { + root = join(tmpDir, 'workspace') + outside = join(tmpDir, 'outside') + await mkdir(root) + await mkdir(outside) + ctx = { ...defaultContext, cwd: root } + }) + + async function write(path: string, content = 'escaped') { + return fileWriteTool.execute({ path, content }, ctx) + } + + it('rejects file_write through a dangling symlink that points outside the root', async () => { + const target = join(outside, 'planted.txt') + await symlink(target, join(root, 'dangling.txt')) + + const result = await write(join(root, 'dangling.txt')) + + expect(result.isError).toBe(true) + expect(result.data).toContain("outside the agent's working directory") + expect(await fileExists(target)).toBe(false) + }) + + it('rejects file_write through a relative dangling symlink that points outside the root', async () => { + await symlink(join('..', 'outside', 'planted.txt'), join(root, 'dangling.txt')) + + const result = await write(join(root, 'dangling.txt')) + + expect(result.isError).toBe(true) + expect(result.data).toContain("outside the agent's working directory") + expect(await fileExists(join(outside, 'planted.txt'))).toBe(false) + }) + + it('rejects file_write through a chain of dangling symlinks that ends outside the root', async () => { + await symlink(join(root, 'second'), join(root, 'first')) + await symlink(join(outside, 'planted.txt'), join(root, 'second')) + + const result = await write(join(root, 'first')) + + expect(result.isError).toBe(true) + expect(result.data).toContain("outside the agent's working directory") + expect(await fileExists(join(outside, 'planted.txt'))).toBe(false) + }) + + it('rejects file_write into a symlinked directory outside the root', async () => { + await symlink(outside, join(root, 'linked-dir')) + + const result = await write(join(root, 'linked-dir', 'new.txt')) + + expect(result.isError).toBe(true) + expect(result.data).toContain("outside the agent's working directory") + expect(await fileExists(join(outside, 'new.txt'))).toBe(false) + }) + + it('rejects file_write when a middle path segment is a symlink outside the root', async () => { + await mkdir(join(root, 'a')) + await symlink(outside, join(root, 'a', 'b')) + + const result = await write(join(root, 'a', 'b', 'c', 'new.txt')) + + expect(result.isError).toBe(true) + expect(result.data).toContain("outside the agent's working directory") + expect(await fileExists(join(outside, 'c'))).toBe(false) + }) + + it('rejects file_write when a middle path segment is a dangling symlink outside the root', async () => { + await mkdir(join(root, 'a')) + await symlink(join(outside, 'missing-dir'), join(root, 'a', 'b')) + + const result = await write(join(root, 'a', 'b', 'c', 'new.txt')) + + expect(result.isError).toBe(true) + expect(result.data).toContain("outside the agent's working directory") + expect(await fileExists(join(outside, 'missing-dir'))).toBe(false) + }) + + it('rejects file_write through a symlink loop', async () => { + await symlink(join(root, 'loop-b'), join(root, 'loop-a')) + await symlink(join(root, 'loop-a'), join(root, 'loop-b')) + + const result = await write(join(root, 'loop-a')) + + expect(result.isError).toBe(true) + expect(result.data).toContain('symbolic links') + }) + + it('rejects file_write through a symlink chain longer than the kernel allows', async () => { + // 41 links exceeds MAXSYMLINKS on Linux (40) and macOS (32), so the + // kernel refuses the chain even though every link resolves inside the root. + await writeFile(join(root, 'end.txt'), 'keep') + let next = join(root, 'end.txt') + for (let i = 0; i < 41; i++) { + const link = join(root, `chain-${i}`) + await symlink(next, link) + next = link + } + + const result = await write(next) + + expect(result.isError).toBe(true) + expect(result.data).toContain('symbolic links') + expect(await readFile(join(root, 'end.txt'), 'utf8')).toBe('keep') + }) + + it('rejects a dangling symlink whose target steps out of a missing directory', async () => { + // The kernel cannot resolve `ghost/..` because `ghost` dangles, so the + // write must fail rather than collapse the target to `self` lexically. + await symlink(join(root, 'missing-dir'), join(root, 'ghost')) + await symlink(['ghost', '..', 'self'].join(sep), join(root, 'self')) + + const result = await write(join(root, 'self')) + + expect(result.isError).toBe(true) + expect(result.data).toContain('to be an existing directory') + expect(await fileExists(join(root, 'missing-dir'))).toBe(false) + }) + + it.each([ + ['a trailing separator', sep], + ['a trailing .', `${sep}.`], + ])('rejects a symlink target with %s that names a file', async (_label, suffix) => { + // The suffix makes the kernel require a directory, so it refuses to open + // `victim.txt` through the link. + await writeFile(join(root, 'victim.txt'), 'keep') + await symlink(`victim.txt${suffix}`, join(root, 'link')) + + const result = await write(join(root, 'link')) + + expect(result.isError).toBe(true) + expect(result.data).toContain('to be an existing directory') + expect(await readFile(join(root, 'victim.txt'), 'utf8')).toBe('keep') + }) + + it('follows a symlink before applying .. in a dangling symlink target', async () => { + // `pivot/..` is `nested`, not the root, because `pivot` points into it. + await mkdir(join(root, 'nested', 'deep'), { recursive: true }) + await symlink(join('nested', 'deep'), join(root, 'pivot')) + await symlink(['pivot', '..', 'created.txt'].join(sep), join(root, 'link')) + await writeFile(join(root, 'created.txt'), 'unrelated') + + const result = await write(join(root, 'link'), 'through link') + + expect(result.isError).toBe(false) + expect(await readFile(join(root, 'nested', 'created.txt'), 'utf8')).toBe('through link') + expect(await readFile(join(root, 'created.txt'), 'utf8')).toBe('unrelated') + }) + + it('writes through a symlink to a file inside the root', async () => { + await writeFile(join(root, 'real.txt'), 'old') + await symlink(join(root, 'real.txt'), join(root, 'alias.txt')) + + const result = await write(join(root, 'alias.txt'), 'new') + + expect(result.isError).toBe(false) + expect(result.data).toContain('Updated') + expect(await readFile(join(root, 'real.txt'), 'utf8')).toBe('new') + }) + + it('writes through a symlinked directory inside the root', async () => { + await mkdir(join(root, 'real-dir')) + await symlink(join(root, 'real-dir'), join(root, 'dir-alias')) + + const result = await write(join(root, 'dir-alias', 'nested', 'new.txt'), 'inside') + + expect(result.isError).toBe(false) + expect(await readFile(join(root, 'real-dir', 'nested', 'new.txt'), 'utf8')).toBe('inside') + }) + + it('writes through a dangling symlink whose target is inside the root', async () => { + await mkdir(join(root, 'sub')) + // Relative targets resolve against the directory holding the link. + await symlink(join('..', 'later.txt'), join(root, 'sub', 'pending.txt')) + + const result = await write(join(root, 'sub', 'pending.txt'), 'created') + + expect(result.isError).toBe(false) + expect(result.data).toContain('Created') + expect(await readFile(join(root, 'later.txt'), 'utf8')).toBe('created') + }) + + it('rejects file_edit through a symlink to a file outside the root', async () => { + await writeFile(join(outside, 'secret.txt'), 'keep') + await symlink(join(outside, 'secret.txt'), join(root, 'linked.txt')) + + const result = await fileEditTool.execute( + { path: join(root, 'linked.txt'), old_string: 'keep', new_string: 'changed' }, + ctx, + ) + + expect(result.isError).toBe(true) + expect(result.data).toContain("outside the agent's working directory") + expect(await readFile(join(outside, 'secret.txt'), 'utf8')).toBe('keep') + }) +}) + // =========================================================================== // glob // ===========================================================================