fix(sandbox): address PR #309 review — TOCTOU direction, denial metadata, shared roots, docs
- fs-sandbox: delegate the mutation with the freshly re-canonicalized target (not the stale one), so the checked identity is the mutated identity — a symlink swapped in after resolve() can no longer escape workspace-write. - tool-fs: map a denial to an FsError carrying FS_SANDBOX_DENIED (not a plain Error), so ToolRegistry keeps the structured code on result.error for retry/observers while the message stays the shared marker. - sandbox-local: derive the Seatbelt writable set from the shared writableRoots() helper, so the profile and the fs fence cannot drift. - gen-doc-graphs: ctx.sandboxPolicy is owned by dsh-sandbox-policy and read only by the sandboxed executor/provider (the tool layers use the pure fold). - docs: bash-sandbox/bash/permission READMEs and bash.md reflect the relocated policy home and the sandbox/mode rename; drop the stale stdout.golden.jsonl.
This commit is contained in:
@@ -88,7 +88,7 @@ export class SandboxedFileSystem extends LocalFileSystem {
|
||||
|
||||
/**
|
||||
* Fence the write by the per-call mode, then delegate to the inherited
|
||||
* atomic write. See {@link assertWritable}.
|
||||
* atomic write. See {@link checkedTarget}.
|
||||
* @param target - the resolved target to write.
|
||||
* @param content - the full new file content.
|
||||
* @param expected - the write intent guarding the write; omit for unconditional.
|
||||
@@ -103,13 +103,12 @@ export class SandboxedFileSystem extends LocalFileSystem {
|
||||
signal?: AbortSignal,
|
||||
sandboxMode?: SandboxMode,
|
||||
): Promise<FsWriteOutcome> {
|
||||
await this.assertWritable(target, sandboxMode)
|
||||
return super.writeText(target, content, expected, signal)
|
||||
return super.writeText(await this.checkedTarget(target, sandboxMode), content, expected, signal)
|
||||
}
|
||||
|
||||
/**
|
||||
* Fence the edit by the per-call mode, then delegate to the inherited
|
||||
* atomic edit. See {@link assertWritable}.
|
||||
* atomic edit. See {@link checkedTarget}.
|
||||
* @param target - the resolved target to edit.
|
||||
* @param edit - the literal search/replace request.
|
||||
* @param expected - the version guard; omit for an unconditional edit.
|
||||
@@ -124,31 +123,34 @@ export class SandboxedFileSystem extends LocalFileSystem {
|
||||
signal?: AbortSignal,
|
||||
sandboxMode?: SandboxMode,
|
||||
): Promise<FsEditOutcome> {
|
||||
await this.assertWritable(target, sandboxMode)
|
||||
return super.editText(target, edit, expected, signal)
|
||||
return super.editText(await this.checkedTarget(target, sandboxMode), edit, expected, signal)
|
||||
}
|
||||
|
||||
/**
|
||||
* Enforce the per-call mode against `target` before delegating the mutation.
|
||||
* `read-only` denies; `workspace-write` re-canonicalizes the target NOW
|
||||
* (`resolve` realpaths the deepest existing ancestor, reflecting a
|
||||
* concurrently swapped symlink) and requires containment under a writable
|
||||
* root; `danger-full-access` allows. Throws the structured
|
||||
* `FS_SANDBOX_DENIED` on refusal — the tool layer maps it to the model-facing
|
||||
* `[sandbox: …]` marker and the escalation hint.
|
||||
* Enforce the per-call mode against `target` and return the EXACT target the
|
||||
* mutation must use, so the checked identity is the mutated one (no
|
||||
* check-here-write-there TOCTOU). `read-only` denies; `workspace-write`
|
||||
* re-canonicalizes NOW (`resolve` realpaths the deepest existing ancestor,
|
||||
* reflecting a concurrently swapped symlink), requires containment under a
|
||||
* writable root, and returns THAT fresh target; `danger-full-access` returns
|
||||
* the caller's target unfenced. Throws the structured `FS_SANDBOX_DENIED` on
|
||||
* refusal — the tool layer maps it to the model-facing `[sandbox: …]` marker
|
||||
* and the escalation hint.
|
||||
*/
|
||||
private async assertWritable(target: FsTarget, sandboxMode?: SandboxMode): Promise<void> {
|
||||
private async checkedTarget(target: FsTarget, sandboxMode?: SandboxMode): Promise<FsTarget> {
|
||||
const mode = sandboxMode ?? this.defaultMode
|
||||
if (mode === 'danger-full-access') return
|
||||
if (mode === 'danger-full-access') return target
|
||||
if (mode === 'read-only') {
|
||||
throw new FsError(`cannot write "${target.displayPath}": file access denied under read-only mode`, 'FS_SANDBOX_DENIED')
|
||||
}
|
||||
// workspace-write: containment on the FRESH canonical path (catches a
|
||||
// symlink ancestor swapped since the tool resolved this target).
|
||||
// symlink ancestor swapped since the tool resolved this target), and the
|
||||
// mutation delegates with THIS fresh target — never the stale one.
|
||||
const fresh = await this.resolve(target.displayPath)
|
||||
if (!this.writableRoots.some(root => isUnder(fresh.targetKey, root))) {
|
||||
throw new FsError(`cannot write "${target.displayPath}": file access denied under workspace-write mode`, 'FS_SANDBOX_DENIED')
|
||||
}
|
||||
return fresh
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -14,7 +14,7 @@ import { existsSync } from 'node:fs'
|
||||
import { homedir, tmpdir } from 'node:os'
|
||||
import { join } from 'node:path'
|
||||
import { Context } from 'cordis'
|
||||
import { FsError } from '@deepseek-ai/dsh-fs'
|
||||
import { FsError, FsTargetKey } from '@deepseek-ai/dsh-fs'
|
||||
import type { FsTarget } from '@deepseek-ai/dsh-fs'
|
||||
import SandboxPolicyService from '@deepseek-ai/dsh-sandbox-policy'
|
||||
import type { SandboxMode } from '@deepseek-ai/dsh-sandbox'
|
||||
@@ -146,6 +146,19 @@ describe('workspace-write containment', () => {
|
||||
expect(await readFile(path, 'utf8')).toBe('changed')
|
||||
})
|
||||
|
||||
it('mutates the freshly checked identity, not a stale outside targetKey (TOCTOU direction)', async () => {
|
||||
// A target whose displayPath is inside the workspace but whose targetKey is
|
||||
// a STALE outside path — as if an ancestor symlink pointed out at the tool's
|
||||
// resolve() and was swapped in before the write. The fence re-resolves
|
||||
// displayPath (now inside) AND delegates with that fresh target, so the byte
|
||||
// lands inside and the stale outside path is never written.
|
||||
const insidePath = join(workspace, 'landed.txt')
|
||||
const staleTarget: FsTarget = { displayPath: insidePath, targetKey: FsTargetKey(join(outside, 'escaped.txt')) }
|
||||
await fs.writeText(staleTarget, 'inside')
|
||||
expect(await readFile(insidePath, 'utf8')).toBe('inside')
|
||||
expect(existsSync(join(outside, 'escaped.txt'))).toBe(false)
|
||||
})
|
||||
|
||||
it('the workspace root itself passes the fence (path equal to a writable root), failing only on file type', async () => {
|
||||
// isUnder's path-equals-root branch: the fence allows the root, and the
|
||||
// write then fails because the root is a directory, not a regular file.
|
||||
|
||||
Reference in New Issue
Block a user