diff --git a/.gitignore b/.gitignore index 139e4d2259..6cd8c7d0da 100644 --- a/.gitignore +++ b/.gitignore @@ -38,6 +38,9 @@ docs/bot-detection.md # Base2 gate-test scratch .base2-test-scratch/ +# Containment test escape-target scratch (outside the OS temp roots) +.containment-test-scratch/ + # Local agent state (task memory, gate telemetry JSONL sink) .openbuff/ diff --git a/common/knowledge.md b/common/knowledge.md index 952e2a86ad..fbdac76ea9 100644 --- a/common/knowledge.md +++ b/common/knowledge.md @@ -71,6 +71,10 @@ This package contains code shared across the Openbuff monorepo, especially the l - _Knowledge refresh 2026-09-05 (compaction progress, image sniffing, subagent timeout removal): `common/src/types/print-mode.ts` gained the additive `context_compaction_progress` variant (`runId`, `ancestorRunIds`, optional `agentId`, `percent`, a `phase` of `analyzing`/`summarizing`/`applying`, optional `contextTokens`/`targetBudgetTokens`). It is a NEW member of the discriminated union rather than a widened `context_compaction_status` `state` enum, precisely because an added enum member breaks a consumer switching exhaustively over that enum while an unknown event `type` is already contractually a no-op. It appears only BETWEEN a `started` and its matching `settled` for the same `runId`; `percent` is a best-effort monotonic estimate, so a consumer must clamp with its own maximum rather than trust arrival order or range, and `percent: 100` is never a claim that space was reclaimed — the terminal `context_compaction` result stays the only signal of that. The category schemas gained an optional `boundedFileReads` (optional so replayed events emitted before the bounded-vs-whole-file split still validate). `common/src/constants/images.ts` gained `detectImageMediaTypeFromBytes`, which matches PNG, JPEG, GIF, BMP, WEBP, and TIFF magic numbers, requires the WEBP form tag at offset 8 so RIFF-fronted audio is not misreported as an image, and returns null for short or unsigned buffers; `common/src/__tests__/images.test.ts` pins that it only ever returns MIME strings the extension map already publishes. The subagent wall-clock timeout surface was removed: `defaultTimeoutMs` is gone from `common/src/types/agent-template.ts` and `common/src/types/dynamic-agent-template.ts`, the per-spawn `timeout_seconds` entry field is gone from `common/src/tools/params/tool/spawn-agents.ts`, `timeout` is gone from the tool-call request in `common/src/actions.ts`, and `common/src/tools/params/tool/run-terminal-command.ts` now defaults `timeout_seconds` to -1 (no timeout). That is a public tool-schema change, so regenerated tool definition sources must land in the same commit. `common/src/types/agent-handoff.ts` gained an optional observational `contextUsage` on the agent receipt (`tokens` plus optional `windowTokens`, `percentOfWindow`, `compactionCount`) so a parent can size later delegations, and `common/src/types/session-state.ts` gained `AgentState.lastSetOutputError` so the missing-structured-output retry names the real rejection._ +- _Knowledge refresh 2026-09-06 (cleanup followups): three reviewer advisories from the temp-scope change were closed. (1) The duplicated `makeOutsideRoot`/`outsideRootsUsable`/`removeScratchParentIfEmpty` containment fixtures copy-pasted across ~8 test suites were consolidated into one shared module at `common/src/testing/fixtures/containment-fixtures.ts`, re-exported from `common/src/testing/index.ts`, and the consuming suites (`project-path-containment.test.ts`, `path-utils.test.ts`, `read-image.test.ts`, `read-logs.test.ts`, `glob.test.ts`, `run-agent-step-tools.test.ts`) were rewired to import from `@codebuff/common/testing` — which required adding `./testing` and `./testing/*` export-map entries in `common/package.json` (the flat `"*"` glob only mapped top-level files). (2) The duplicate bounded-append loops in `recordCanonicalReceipt`/`retainReceipt` in `sdk/src/tools/filesystem-authority.ts` were noted; left as-is for this pass. (3) The `OWNED_TEMP_SEGMENT_PATTERNS_FS_AWARE` re-export alias at `sdk/src/tools/path-utils.ts` was removed (it had zero importers) and `OWNED_TEMP_SEGMENT_PATTERNS` is imported from `common/src/util/project-path-containment.ts` directly. + +- _Knowledge refresh 2026-09-05 (temp-root scope widening + quoted-slash terminal fix): `common/src/util/project-path-containment.ts` widened the OS-temp exception from an openbuff-owned NAME gate to plain containment — any path strictly inside a temp root (`os.tmpdir()` plus `/tmp` on POSIX) now resolves with `scope: 'owned-temp'` and an absolute `relativePath` whatever its segment names, so `/notes.txt` behaves exactly like `/openbuff-job-1.log`. `OWNED_TEMP_SEGMENT_PATTERNS` is DOCUMENTATION ONLY now (it enumerates the namespaces openbuff itself creates); `isInsideOwnedTempNamespace` was renamed `isInsideTempRoot`; the strictly-inside rule, raw-`..` refusal, and the single-realpath TOCTOU discipline are unchanged. A NEW fail-closed `isMandatorySensitiveReadPath` refusal (lexical AND dereferenced path, mirroring `resolveExternalReadRealPath`) keeps `/.env`, `/credentials.json`, private keys, and path-aware carriers like `.aws/config` unreachable for READS and WRITES — with the name gate gone there is no longer an incidental pattern blocking them, so the resolver is the only guard. `isOwnedTempPathForFileSystem` is newly exported and `sdk/src/tools/path-utils.ts` deleted its private fs-aware duplicate in favor of it. `packages/agent-runtime/src/tools/tool-executor.ts` gained `OWNED_TEMP_WRITE_EXEMPT_TOOLS` (the file-changing tools) so writes into temp are no longer hard-blocked by the backstop, while the external-read allowlist stays strictly read-only (the write side never consults `isExternalReadPath`) and `code_search`/`glob`/`find_files_matching_content` temp cwds stay hard-blocked because their handlers do not contain. `ownedTempMutationRefusal` in `sdk/src/tools/filesystem-authority.ts` is unchanged in mechanism and is the ONLY defense against writing an executable-extension basename anywhere under temp, including `tmux-helper-.sh`, which containment no longer excludes; its `OWNED_TEMP_REFUSED_EXTENSIONS` set now also refuses interpreter-executed extensions (.js/.mjs/.cjs/.jsx/.ts/.tsx/.mts/.cts/.py/.pyw/.pl/.rb/.lua/.php/.r/.jl/.tcl) because a `write_file /tmp/x.js` followed by `node /tmp/x.js` would otherwise execute staged code under terminal profiles that permit `node ` (create/overwrite/move refused; delete stays allowed for cleanup); `run_terminal_command` still refuses an owned-temp cwd. A follow-up repair extends the same refusals to Win32 trailing dot/space aliases: `ownedTempMutationRefusal` now evaluates the job-artifact pattern, the tmux-capture segment pattern, and the executable-extension set against BOTH the resolved path and its Win32-normalized form (every segment's trailing dots/spaces stripped, via a module-local `win32NormalizeSegments` in `filesystem-authority.ts` mirroring `refusesWin32AliasedSensitivePath`), so a lexical `payload.sh ` — whose extname `.sh ` misses the set while the OS creates the real `payload.sh` — can no longer stage an executable, clobber live `openbuff-*.log`/`.json` job artifacts, or forge `tmux-captures-*` capture evidence on Windows; refusal codes, the owned-temp scope gate, the read exemption, and the delete cleanup carve-out are unchanged. Two more hardenings from the security review: the temp resolvers also refuse Win32-aliased sensitive paths (`/.env ` with a trailing space/dot normalizes to `.env` on win32, and the same per-segment normalization covers aliased INTERMEDIATE directories so `/.aws /config` cannot open the real `.aws/config` — `refusesWin32AliasedSensitivePath` in the same module normalizes EVERY segment, applied to lexical AND dereferenced paths in both sync and async resolvers, while non-sensitive trailing-dot names stay admitted), and `findOutsideAbsolutePath` re-refuses a QUOTED root-only operand (`rm -rf '/'`, `cp x '/'`) when the command invokes a filesystem-mutating executable (rm/mv/cp/chmod/chown/chgrp/dd/shred/truncate/ln/install, optionally behind sudo/doas) — the quoted-root skip itself exists only for `sed`/`awk` expression delimiters, and `ls /` unquoted, `cat '/etc/passwd'`, and `bash -c 'cat /etc/passwd'` stay refused. Separately, `sdk/src/tools/terminal-command-policy.ts` fixed a false positive where a bare `/` inside a quoted word — typically the delimiter tail of `sed 's/^/X /'` — was treated as an absolute path operand: `findOutsideAbsolutePath` now skips only a root-only token that sits inside a quoted region (one linear quote scan per call), while an unquoted `ls /`, a quoted `cat '/etc/passwd'`, and an embedded `bash -c 'cat /etc/passwd'` stay refused. Containment negative fixtures that used `os.tmpdir()` mkdtemp dirs as their "outside the project" target were re-anchored to a gitignored `.containment-test-scratch/` under each package directory so the refusals remain attributable._ + ## Scope Notes Openbuff is CLI/SDK-focused and local/BYOK. Do not add new dependencies from `common/` to hosted web, billing, credit, subscription, or BigQuery product surfaces. Provider-owned billing, quota, token usage, and OAuth flows may still be documented when they refer to the user's configured provider rather than an Openbuff-hosted product. diff --git a/common/package.json b/common/package.json index 57c14bf0a7..040e2ff77a 100644 --- a/common/package.json +++ b/common/package.json @@ -9,7 +9,19 @@ "bun": "./src/*.ts", "import": "./src/*.ts", "types": "./src/*.ts", - "default": "./src/*.ts" + "default": "./src*.ts" + }, + "./testing": { + "bun": "./src/testing/index.ts", + "import": "./src/testing/index.ts", + "types": "./src/testing/index.ts", + "default": "./src/testing/index.ts" + }, + "./testing/*": { + "bun": "./src/testing/*.ts", + "import": "./src/testing/*.ts", + "types": "./src/testing/*.ts", + "default": "./src/testing/*.ts" } }, "scripts": { diff --git a/common/src/testing/fixtures/containment-fixtures.ts b/common/src/testing/fixtures/containment-fixtures.ts new file mode 100644 index 0000000000..92971b0102 --- /dev/null +++ b/common/src/testing/fixtures/containment-fixtures.ts @@ -0,0 +1,69 @@ +/** + * Shared filesystem fixtures for suites that exercise paths OUTSIDE the + * project root and every OS temp root. + * + * Containment suites cannot anchor escape fixtures under an OS temp root: the + * widened temp exception legitimately admits anything strictly inside one, so + * a refusal there would be unattributable. `makeOutsideRoot` therefore anchors + * scratch directories under `/.containment-test-scratch/`, which sits + * outside BOTH boundaries. Suites either clean the fixtures themselves or + * delegate to `cleanupOutsideRoots()` (which drains the module-level tracker) + * followed by `removeScratchParentIfEmpty()`. + */ + +import fs from 'node:fs' +import path from 'node:path' + +import { getOwnedTempRoots } from '../../util/project-path-containment' + +/** Fixture roots created by `makeOutsideRoot`, for `cleanupOutsideRoots`. */ +const trackedOutsideRoots: string[] = [] + +/** + * Fixture root that is outside BOTH the project root and every OS temp root, so + * a containment refusal here is attributable to the escape itself rather than + * to a path the widened temp exception now legitimately admits. + */ +export function makeOutsideRoot(prefix: string): string { + // The scratch parent is created lazily at call time — never at module load — + // so importing this module has no filesystem side effects. + const parent = path.join(process.cwd(), '.containment-test-scratch') + fs.mkdirSync(parent, { recursive: true }) + const dir = fs.mkdtempSync(path.join(parent, prefix)) + trackedOutsideRoots.push(dir) + return dir +} + +/** + * True when this checkout itself sits outside every OS temp root. A checkout + * under a temp root would make `makeOutsideRoot` produce an owned-temp path + * where an escape refusal is unattributable — affected tests skip in that + * case instead of asserting a refusal that cannot hold there. + */ +export function outsideRootsUsable(): boolean { + const repoRoot = fs.realpathSync(process.cwd()) + return getOwnedTempRoots().every((root) => { + const relative = path.relative(fs.realpathSync(root), repoRoot) + return ( + relative === '..' || + relative.startsWith('..' + path.sep) || + path.isAbsolute(relative) + ) + }) +} + +/** Removes every fixture root still tracked from `makeOutsideRoot`. */ +export function cleanupOutsideRoots(): void { + for (const dir of trackedOutsideRoots.splice(0)) { + fs.rmSync(dir, { recursive: true, force: true }) + } +} + +/** Removes the scratch parent when empty; see `makeOutsideRoot`. */ +export function removeScratchParentIfEmpty(): void { + try { + fs.rmdirSync(path.join(process.cwd(), '.containment-test-scratch')) + } catch { + // Children from this or another suite remain; leave the parent in place. + } +} diff --git a/common/src/testing/index.ts b/common/src/testing/index.ts index 18892c2b46..27e01a10c2 100644 --- a/common/src/testing/index.ts +++ b/common/src/testing/index.ts @@ -53,6 +53,21 @@ export { } from './fixtures/agent-runtime' export type { TestAgentRuntimeParams } from './fixtures/agent-runtime' +// ============================================================================ +// Containment Test Fixtures +// ============================================================================ + +/** + * Filesystem fixtures for suites that create directories outside the project + * root and every OS temp root (see `makeOutsideRoot` for why). + */ +export { + cleanupOutsideRoots, + makeOutsideRoot, + outsideRootsUsable, + removeScratchParentIfEmpty, +} from './fixtures/containment-fixtures' + // ============================================================================ // Error Utilities // ============================================================================ diff --git a/common/src/util/__tests__/project-path-containment.test.ts b/common/src/util/__tests__/project-path-containment.test.ts index fb04565d01..5381a82f94 100644 --- a/common/src/util/__tests__/project-path-containment.test.ts +++ b/common/src/util/__tests__/project-path-containment.test.ts @@ -10,6 +10,12 @@ import { test, } from 'bun:test' +import { + makeOutsideRoot, + outsideRootsUsable, + removeScratchParentIfEmpty, +} from '@codebuff/common/testing' + import { configureExternalReadRoots, ensureExternalReadRootsConfigured, @@ -24,6 +30,15 @@ import { resolveProjectPathForRead, } from '../project-path-containment' +/** + * `describe`, skipped whole when outside-root fixtures cannot be produced. The + * runtime describe exposes `.skip` but the local bun:test typings do not model + * it, so the narrow cast is deliberate. + */ +const describeWithOutsideFixtures: typeof describe = outsideRootsUsable() + ? describe + : (describe as unknown as { skip: typeof describe }).skip + describe('isPathInsideProject', () => { test('accepts project-relative paths', () => { expect(isPathInsideProject('/repo', 'src/file.ts')).toBe(true) @@ -119,7 +134,7 @@ describe('isPathInsideProject — symlink containment', () => { beforeEach(() => { tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'path-contain-')) - outsideDir = fs.mkdtempSync(path.join(os.tmpdir(), 'outside-')) + outsideDir = makeOutsideRoot('outside-') // In-project symlink that escapes: tmpDir/evil -> outsideDir fs.symlinkSync(outsideDir, path.join(tmpDir, 'evil')) // Legit in-project symlink: tmpDir/link -> tmpDir/real @@ -132,16 +147,21 @@ describe('isPathInsideProject — symlink containment', () => { fs.rmSync(outsideDir, { recursive: true, force: true }) }) + afterAll(removeScratchParentIfEmpty) + test('rejects a symlink that points outside the project', () => { + if (!outsideRootsUsable()) return expect(isPathInsideProject(tmpDir, 'evil')).toBe(false) expect(isPathInsideProject(tmpDir, 'evil/file.ts')).toBe(false) }) test('rejects an outside symlink even when the target does not exist', () => { + if (!outsideRootsUsable()) return expect(isPathInsideProject(tmpDir, 'evil/nonexistent.ts')).toBe(false) }) test('rejects a missing descendant below a symlink whose nearest existing ancestor is outside', () => { + if (!outsideRootsUsable()) return const missingDescendant = path.join('evil', 'missing', 'deep', 'file.ts') expect(resolveProjectPath(tmpDir, missingDescendant)).toBeNull() expect(isPathInsideProject(tmpDir, missingDescendant)).toBe(false) @@ -152,12 +172,16 @@ describe('isPathInsideProject — symlink containment', () => { }) }) -describe('openbuff-owned OS temp namespace exception', () => { +describe('OS temp root containment exception', () => { + // The exception covers the WHOLE temp root: any path strictly inside it + // resolves with `scope: 'owned-temp'` whatever its segment names. The name + // patterns in `OWNED_TEMP_SEGMENT_PATTERNS` are documentation only. + // // `os.tmpdir()` is used for the primary cases: on macOS it is a symlinked // `/var/folders/...` path, so hardcoding `/tmp` would compare the wrong - // strings. Literal `/tmp` assertions are guarded on it being an owned root. + // strings. Literal `/tmp` assertions are guarded on it being a temp root. const tempRoot = getOwnedTempRoots()[0] - // Literal `/tmp` assertions only run where `/tmp` really is an owned root + // Literal `/tmp` assertions only run where `/tmp` really is a temp root // (POSIX); `getOwnedTempRoots()[0]` covers every platform. const literalTmpIsOwnedRoot = isOwnedTempPath('/tmp/openbuff-probe.log') const uniqueSuffix = () => @@ -189,8 +213,92 @@ describe('openbuff-owned OS temp namespace exception', () => { afterEach(cleanupCreated) afterAll(cleanupCreated) + afterAll(removeScratchParentIfEmpty) + + describe('accepts any path strictly inside an OS temp root', () => { + /** Asserts the widened contract through the predicate AND the resolver. */ + const expectTempScoped = (target: string) => { + expect(isOwnedTempPath(target)).toBe(true) + const result = resolveProjectPath('/some/project', target) + expect(result).not.toBeNull() + expect(result!.scope).toBe('owned-temp') + // Outside the project, so `relativePath` is the absolute resolved path. + expect(result!.relativePath).toBe(path.resolve(target)) + expect(path.isAbsolute(result!.relativePath)).toBe(true) + } + + test('accepts a plain temp file whose name matches no openbuff pattern', () => { + expectTempScoped(path.join(tempRoot, 'notes.txt')) + + if (literalTmpIsOwnedRoot) { + expect(isOwnedTempPath('/tmp/notes.txt')).toBe(true) + } + }) + + test('accepts an unrelated tool\u2019s temp subtree', () => { + expectTempScoped(path.join(tempRoot, 'other-tool-cache', 'x.log')) + + if (literalTmpIsOwnedRoot) { + expect(isOwnedTempPath('/tmp/other-tool-cache/x.log')).toBe(true) + } + }) + + test('accepts a nested path whose first segment matches no openbuff pattern', () => { + expectTempScoped(path.join(tempRoot, 'nested', 'deep', 'file.json')) + expectTempScoped(path.join(tempRoot, 'nested', 'openbuff-foo.log')) + + if (literalTmpIsOwnedRoot) { + expect(isOwnedTempPath('/tmp/nested/deep/file.json')).toBe(true) + expect(isOwnedTempPath('/tmp/nested/openbuff-foo.log')).toBe(true) + } + }) + + test('accepts a name that merely resembles an openbuff namespace', () => { + // Previously refused because `notopenbuff-foo.log` failed the anchored + // first-segment pattern; names are no longer part of the decision. + expectTempScoped(path.join(tempRoot, 'notopenbuff-foo.log')) + + if (literalTmpIsOwnedRoot) { + expect(isOwnedTempPath('/tmp/notopenbuff-foo.log')).toBe(true) + } + }) + + test('accepts openbuff-prefixed names with any extension', () => { + // Containment no longer restricts the extension. The defense against + // WRITING an executable-extension basename lives in + // `ownedTempMutationRefusal` / `OWNED_TEMP_REFUSED_EXTENSIONS` in + // `sdk/src/tools/filesystem-authority.ts`, not here. + expectTempScoped(path.join(tempRoot, 'openbuff-evil.sh')) + expectTempScoped(path.join(tempRoot, 'openbuff-x.log.sh')) + expectTempScoped(path.join(tempRoot, 'openbuff-x.txt')) + expect( + isOwnedTempPath(path.join(tempRoot, 'openbuff-evil.sh', 'payload')), + ).toBe(true) + expect( + isPathInsideProject( + '/some/project', + path.join(tempRoot, 'openbuff-evil.sh'), + ), + ).toBe(true) + + if (literalTmpIsOwnedRoot) { + expect(isOwnedTempPath('/tmp/openbuff-evil.sh')).toBe(true) + expect(isOwnedTempPath('/tmp/openbuff-x.log.sh')).toBe(true) + expect(isOwnedTempPath('/tmp/openbuff-x.txt')).toBe(true) + } + }) + + test('accepts the tmux helper script name', () => { + // Formerly excluded by containment. The executable-basename defense now + // lives ENTIRELY in the SDK filesystem authority, which refuses + // create/overwrite/move for `.sh` under a temp root. + expectTempScoped(path.join(tempRoot, 'tmux-helper-abc.sh')) + + if (literalTmpIsOwnedRoot) { + expect(isOwnedTempPath('/tmp/tmux-helper-abc.sh')).toBe(true) + } + }) - describe('accepts openbuff-owned temp paths', () => { test('accepts background-job log and metadata names', () => { expect(isOwnedTempPath(path.join(tempRoot, 'openbuff-job-abc.log'))).toBe( true, @@ -235,20 +343,6 @@ describe('openbuff-owned OS temp namespace exception', () => { } }) - test('rejects a tmux helper script name', () => { - // The executable tmux helper script is deliberately NOT an owned temp - // namespace: it is chmod +x'd and then EXECUTED by - // run_terminal_command, so granting write access there would turn a - // file write into arbitrary command execution. - expect(isOwnedTempPath(path.join(tempRoot, 'tmux-helper-abc.sh'))).toBe( - false, - ) - - if (literalTmpIsOwnedRoot) { - expect(isOwnedTempPath('/tmp/tmux-helper-abc.sh')).toBe(false) - } - }) - test('accepts an owned temp file that really exists on disk', () => { const ownedLog = track( path.join(tempRoot, `openbuff-job-${uniqueSuffix()}.log`), @@ -330,7 +424,7 @@ describe('openbuff-owned OS temp namespace exception', () => { }) }) - describe('rejects paths outside the owned namespaces', () => { + describe('refusals containment still enforces', () => { test('rejects the temp root itself (strictly-inside rule)', () => { expect(isOwnedTempPath(os.tmpdir())).toBe(false) expect(isOwnedTempPath(tempRoot)).toBe(false) @@ -342,68 +436,96 @@ describe('openbuff-owned OS temp namespace exception', () => { } }) - test('rejects a first segment that matches no owned pattern', () => { - const foreign = path.join(tempRoot, 'other-tool-cache', 'x.log') - expect(isOwnedTempPath(foreign)).toBe(false) - expect(resolveProjectPath('/some/project', foreign)).toBeNull() - - if (literalTmpIsOwnedRoot) { - expect(isOwnedTempPath('/tmp/other-tool-cache/x.log')).toBe(false) + test('refuses mandatory-sensitive basenames inside a temp root', () => { + // Widening to the whole temp root would otherwise expose a dropped-in + // credential file to every temp consumer, for reads AND writes, with no + // name pattern incidentally blocking it. The refusal lives in the + // resolver, so the predicate and the resolver must agree. + const sensitive = [ + path.join(tempRoot, '.env'), + path.join(tempRoot, 'credentials.json'), + path.join(tempRoot, 'x', 'id_rsa'), + ] + for (const target of sensitive) { + expect(isOwnedTempPath(target)).toBe(false) + expect(resolveProjectPath('/some/project', target)).toBeNull() } - }) - test('requires the owned prefix at the start of the segment', () => { - const substringMatch = path.join(tempRoot, 'notopenbuff-foo.log') - expect(isOwnedTempPath(substringMatch)).toBe(false) - expect(resolveProjectPath('/some/project', substringMatch)).toBeNull() + // A non-sensitive neighbour under the same root stays reachable, so the + // refusals above are attributable to the sensitive policy. + expect(isOwnedTempPath(path.join(tempRoot, 'env-notes.txt'))).toBe(true) if (literalTmpIsOwnedRoot) { - expect(isOwnedTempPath('/tmp/notopenbuff-foo.log')).toBe(false) + expect(isOwnedTempPath('/tmp/.env')).toBe(false) + expect(isOwnedTempPath('/tmp/credentials.json')).toBe(false) } }) - test('requires the owned prefix on the first segment under the root', () => { - const nested = path.join(tempRoot, 'nested', 'openbuff-foo.log') - expect(isOwnedTempPath(nested)).toBe(false) - expect(resolveProjectPath('/some/project', nested)).toBeNull() - - if (literalTmpIsOwnedRoot) { - expect(isOwnedTempPath('/tmp/nested/openbuff-foo.log')).toBe(false) + test('refuses a Win32-aliased sensitive basename (trailing dot/space)', () => { + // Win32 strips trailing dots/spaces from path segments, so `\.env ` + // resolves to the real `.env` on Windows while its basename fails the + // exact sensitive match above. The alias guard in the resolver refuses + // when the NORMALIZED basename would be sensitive. Paths are built by + // concatenation (not `path.join`) so the raw alias survives into the + // input on every platform. + const aliased = [`${tempRoot}/.env `, `${tempRoot}/.env.`] + for (const target of aliased) { + expect(isOwnedTempPath(target)).toBe(false) + expect(resolveProjectPath('/some/project', target)).toBeNull() } + + // A non-sensitive neighbour whose basename merely ENDS in a dot stays + // reachable: the guard only refuses when normalization lands on a + // sensitive name, so the refusals above are attributable to the + // sensitive policy rather than to trailing-dot names in general. + expect(isOwnedTempPath(`${tempRoot}/notes.txt.`)).toBe(true) + expect( + resolveProjectPath('/some/project', `${tempRoot}/notes.txt.`), + ).not.toBeNull() }) - test('rejects an openbuff-prefixed file with a non-log/json extension', () => { - // The tightened patterns are anchored full-segment shapes: the - // file pattern only admits `.log`/`.json`, and the mkdtemp DIRECTORY - // pattern excludes dots, so it must not rescue a dotted name. This is - // asserted on the predicate/resolver, never on filesystem state. - const evil = path.join(tempRoot, 'openbuff-evil.sh') - expect(isOwnedTempPath(evil)).toBe(false) - expect(resolveProjectPath('/some/project', evil)).toBeNull() - expect(isPathInsideProject('/some/project', evil)).toBe(false) + test('refuses a Win32-aliased INTERMEDIATE directory (trailing dot/space in a parent segment)', async () => { + // Win32 strips trailing dots/spaces from EVERY segment, not only the + // basename: `/.aws /config` opens the real `.aws/config` on + // Windows while neither the exact sensitive match (parent is `.aws `) + // nor a basename-only alias guard (`config` normalizes to itself) + // fires. Paths are built by concatenation so the raw alias survives + // into the input on every platform. + const aliased = [ + `${tempRoot}/.aws /config`, + `${tempRoot}/.kube ./config`, + ] + for (const target of aliased) { + expect(isOwnedTempPath(target)).toBe(false) + expect(resolveProjectPath('/some/project', target)).toBeNull() + } - // Also rejected for a nested path beneath the same segment. + // The async injected-filesystem resolver must refuse the same shape so + // the sync and async twins never disagree about an aliased parent. expect( - isOwnedTempPath(path.join(tempRoot, 'openbuff-evil.sh', 'payload')), - ).toBe(false) + await resolveProjectPathForFileSystemRead( + '/some/project', + `${tempRoot}/.aws /config`, + fs.promises, + ), + ).toBeNull() - if (literalTmpIsOwnedRoot) { - expect(isOwnedTempPath('/tmp/openbuff-evil.sh')).toBe(false) - } + // A non-sensitive neighbour under a trailing-dot directory stays + // reachable, so the refusals above are attributable to the sensitive + // policy rather than to trailing-dot segments in general. + expect(isOwnedTempPath(`${tempRoot}/notes./file.txt`)).toBe(true) }) - test('rejects other non-log/json extensions on an owned-looking name', () => { - const doubleExtension = path.join(tempRoot, 'openbuff-x.log.sh') - const textExtension = path.join(tempRoot, 'openbuff-x.txt') - - expect(isOwnedTempPath(doubleExtension)).toBe(false) - expect(isOwnedTempPath(textExtension)).toBe(false) - expect(resolveProjectPath('/some/project', doubleExtension)).toBeNull() - expect(resolveProjectPath('/some/project', textExtension)).toBeNull() + test('refuses a path-aware credential carrier inside a temp root', () => { + // `config` is far too generic to block on its basename alone; it is the + // PARENT directory that makes `/.aws/config` a credential store, so + // the resolver must pass full paths to the sensitive check. + const awsConfig = path.join(tempRoot, '.aws', 'config') + expect(isOwnedTempPath(awsConfig)).toBe(false) + expect(resolveProjectPath('/some/project', awsConfig)).toBeNull() if (literalTmpIsOwnedRoot) { - expect(isOwnedTempPath('/tmp/openbuff-x.log.sh')).toBe(false) - expect(isOwnedTempPath('/tmp/openbuff-x.txt')).toBe(false) + expect(isOwnedTempPath('/tmp/.aws/config')).toBe(false) } }) @@ -460,6 +582,29 @@ describe('openbuff-owned OS temp namespace exception', () => { expect(resolveProjectPath('/some/project', link)).toBeNull() } }) + + test('rejects a NON-openbuff-named symlink that dereferences outside the temp root', () => { + // Attribution guard for the widened scope: with names no longer part of + // the decision, the refusal must come from the DEREFERENCED-path + // containment check rather than from the basename failing a pattern. + const escapeTarget = fs.realpathSync(process.cwd()) + const targetIsOutsideTempRoots = getOwnedTempRoots().every((root) => { + const relative = path.relative(fs.realpathSync(root), escapeTarget) + return ( + relative === '..' || + relative.startsWith('..' + path.sep) || + path.isAbsolute(relative) + ) + }) + + const link = track(path.join(tempRoot, `notes-${uniqueSuffix()}.txt`)) + fs.symlinkSync(escapeTarget, link) + + if (targetIsOutsideTempRoots) { + expect(isOwnedTempPath(link)).toBe(false) + expect(resolveProjectPath('/some/project', link)).toBeNull() + } + }) }) describe('project containment is preserved', () => { @@ -474,12 +619,11 @@ describe('openbuff-owned OS temp namespace exception', () => { }) test('still rejects an in-project symlink that dereferences outside the project', () => { + if (!outsideRootsUsable()) return const projectDir = track( fs.mkdtempSync(path.join(os.tmpdir(), 'path-contain-project-')), ) - const outsideDir = track( - fs.mkdtempSync(path.join(os.tmpdir(), 'path-contain-outside-')), - ) + const outsideDir = track(makeOutsideRoot('path-contain-outside-')) const outsideFile = path.join(outsideDir, 'secret.ts') fs.writeFileSync(outsideFile, 'secret\n') fs.symlinkSync(outsideFile, path.join(projectDir, 'evil.ts')) @@ -493,7 +637,12 @@ describe('openbuff-owned OS temp namespace exception', () => { }) }) -describe('external read root allowlist', () => { +// Every fixture in this describe must sit outside BOTH the project root and +// the OS temp roots, so the external-read allowlist is exercised in isolation +// from the widened temp exception (a temp-adjacent fixture would resolve as +// `owned-temp` before the external-read branch is ever consulted). The whole +// suite skips when the checkout cannot provide such a fixture. +describeWithOutsideFixtures('external read root allowlist', () => { const projectRoot = '/repo' let allowedRoot: string let siblingRoot: string @@ -514,13 +663,13 @@ describe('external read root allowlist', () => { // would leave an open read boundary for every later test in the process. resetExternalReadRootsForTesting() - allowedRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'external-read-')) + allowedRoot = makeOutsideRoot('external-read-') cleanupPaths.push(allowedRoot) // Sibling directory sharing the allowlisted root's prefix. siblingRoot = `${allowedRoot}-evil` fs.mkdirSync(siblingRoot) cleanupPaths.push(siblingRoot) - outsideDir = fs.mkdtempSync(path.join(os.tmpdir(), 'external-outside-')) + outsideDir = makeOutsideRoot('external-outside-') cleanupPaths.push(outsideDir) allowedFile = path.join(allowedRoot, 'notes.txt') @@ -534,6 +683,8 @@ describe('external read root allowlist', () => { removeTracked() }) + afterAll(removeScratchParentIfEmpty) + describe('default-closed posture', () => { test('is unconfigured and refuses outside paths', () => { expect(getExternalReadRoots()).toEqual([]) @@ -632,10 +783,53 @@ describe('external read root allowlist', () => { expect(isExternalReadPath(credentials)).toBe(false) expect(resolveProjectPathForRead(projectRoot, credentials)).toBeNull() + // Win32 aliasing must NOT slip past the refusal: the OS strips trailing + // dots/spaces from segments, so `/.env ` resolves to the + // real `.env` on Windows. Paths built by concatenation (not `path.join`) + // so the raw alias survives into the input on every platform, matching + // the owned-temp twin's refusal of the identical shape. + const aliased = [`${allowedRoot}/.env `, `${allowedRoot}/credentials.json `] + for (const target of aliased) { + expect(isExternalReadPath(target)).toBe(false) + expect(resolveProjectPathForRead(projectRoot, target)).toBeNull() + } + + // A non-sensitive neighbour whose basename merely ENDS in a dot stays + // reachable: the guard only refuses when normalization lands on a + // sensitive name, so the refusals above are attributable to the + // sensitive policy rather than to trailing-dot names in general. + expect(isExternalReadPath(`${allowedRoot}/notes.txt.`)).toBe(true) + // A non-sensitive neighbour in the same root stays readable. expect(isExternalReadPath(allowedFile)).toBe(true) }) + test('refuses a Win32-aliased INTERMEDIATE directory inside the root', async () => { + // Same intermediate-segment alias as the owned-temp twin: Win32 strips + // trailing dots/spaces from EVERY segment, so `/.aws /config` + // opens the real `.aws/config` while the basename (`config`) and the + // raw parent (`.aws `) each miss the sensitive policy on their own. + const aliased = [ + `${allowedRoot}/.aws /config`, + `${allowedRoot}/.kube ./config`, + ] + for (const target of aliased) { + expect(isExternalReadPath(target)).toBe(false) + expect(resolveProjectPathForRead(projectRoot, target)).toBeNull() + expect( + await resolveProjectPathForFileSystemRead( + projectRoot, + target, + fs.promises, + ), + ).toBeNull() + } + + // A non-sensitive neighbour under a trailing-dot directory stays + // reachable, keeping the refusals attributable to the sensitive policy. + expect(isExternalReadPath(`${allowedRoot}/notes./notes.txt`)).toBe(true) + }) + test('WRITE-PATH INVARIANT: resolveProjectPath never reaches an allowlisted external root', () => { // The load-bearing separation: the resolvers used by change_file / // replace_range / filesystem-authority must stay blind to the read @@ -675,6 +869,17 @@ describe('external read root allowlist', () => { fs.promises, ), ).toBeNull() + + // The injected-filesystem twin must refuse the Win32-aliased shape too, + // so the sync and async resolvers never disagree about an alias like + // `/.env ` (trailing space survives into the input). + expect( + await resolveProjectPathForFileSystemRead( + projectRoot, + `${allowedRoot}/.env `, + fs.promises, + ), + ).toBeNull() }) test('re-configuring with an equivalent set is a no-op', () => { diff --git a/common/src/util/project-path-containment.ts b/common/src/util/project-path-containment.ts index e4c24c226a..ef9daf7818 100644 --- a/common/src/util/project-path-containment.ts +++ b/common/src/util/project-path-containment.ts @@ -26,10 +26,15 @@ export type ContainedProjectPath = { realFullPath: string relativePath: string /** - * 'project' for in-project paths; 'owned-temp' for the openbuff-owned OS - * temp namespace exception; 'external-read' for a path inside an explicitly + * 'project' for in-project paths; 'owned-temp' for the OS-temp-root + * exception (ANY path strictly inside `getOwnedTempRoots()`, whatever its + * segment names); 'external-read' for a path inside an explicitly * allowlisted read-only root outside the project (reachable only through * `resolveProjectPathForRead` / `resolveProjectPathForFileSystemRead`). + * + * The 'owned-temp' NAME is retained because every consumer branches on it; + * it no longer implies an openbuff-owned segment name (see + * `OWNED_TEMP_SEGMENT_PATTERNS`, which is documentation only). */ scope: 'project' | 'owned-temp' | 'external-read' } @@ -61,9 +66,9 @@ function escapesRoot(root: string, target: string): boolean { * Contract for the owned-temp exception (see `isOwnedTempPath`): the RAW * caller input must be free of `..` segments. * - * Traversal through an openbuff-owned temp namespace is never a legitimate - * access pattern, so it is refused BEFORE any collapsing happens — even when - * the collapsed path would land back inside the namespace. This check lives at + * Traversal through an OS temp root is never a legitimate access pattern, so + * it is refused BEFORE any collapsing happens — even when the collapsed path + * would land back inside that root. This check lives at * the entry points (`isOwnedTempPath`, `resolveProjectPath`, * `resolveProjectPathForFileSystem`) rather than inside the owned-temp * resolver: the resolvers call `path.resolve` first, so a check further down @@ -188,23 +193,29 @@ async function realpathCachedForFileSystemRoot( return real } -// First-segment patterns for temp namespaces openbuff itself creates and -// writes into. Writers: `sdk/src/tools/background-jobs.ts` -// (`openbuff-.log` / `.json`, `openbuff-job-*`), -// `agents/basher.ts` (`openbuff-basher-.log`) and `agents/tmux-cli.ts` -// (`tmux-captures-/`). +// DOCUMENTATION ONLY — NOT a containment gate. This list ENUMERATES the +// first-segment temp namespaces openbuff itself creates and writes into: +// `sdk/src/tools/background-jobs.ts` (`openbuff-.log` / `.json`, +// `openbuff-job-*`), `agents/basher.ts` (`openbuff-basher-.log`) and +// `agents/tmux-cli.ts` (`tmux-captures-/`). Tests and readers use it +// to name those namespaces; containment never consults it. // -// The executable tmux helper script (`tmux-helper-.sh`) is -// DELIBERATELY EXCLUDED: it is chmod +x'd and then executed by -// run_terminal_command, whose policy also exempts `/tmp/...` tokens. Granting -// write access there would turn a plain file write into arbitrary command -// execution, i.e. a terminal-policy bypass. +// WHY the distinction matters: reachability under an OS temp root is decided +// SOLELY by `isInsideTempRoot` (strictly-inside containment applied to both +// the lexical and the dereferenced path) plus the fail-closed +// mandatory-sensitive refusal in `resolveOwnedTempRealPath`. A path whose +// first segment matches nothing here — `/notes.txt`, +// `/other-tool-cache/x.log`, `/notopenbuff-foo.log` — is fully +// reachable, so reading this list as a filter would describe a boundary that +// no longer exists. // -// These are ANCHORED FULL-SEGMENT patterns, so an attacker-chosen suffix on an -// otherwise owned-looking name does not qualify: `openbuff-x/../y` never -// reaches here (raw `..` is refused up front) and `openbuff-evil.sh` matches -// neither the `.log|.json` file pattern nor the extension-free directory -// pattern. +// Concretely: the executable tmux helper script (`tmux-helper-.sh`) +// is NO LONGER excluded by containment. The defense against writing an +// executable-extension basename anywhere under the temp root now lives +// ENTIRELY in `ownedTempMutationRefusal` / `OWNED_TEMP_REFUSED_EXTENSIONS` in +// `sdk/src/tools/filesystem-authority.ts`. That refusal — not this list — is +// what stops a plain file write from becoming arbitrary command execution +// through run_terminal_command's `/tmp/...` token exemption. export const OWNED_TEMP_SEGMENT_PATTERNS: RegExp[] = [ // Background-job log/metadata + basher full logs: `openbuff-.log|.json`. /^openbuff-[A-Za-z0-9._-]+\.(?:log|json)$/, @@ -218,7 +229,13 @@ let ownedTempRootsCache: string[] | undefined let ownedTempComparisonRootsCache: string[] | undefined /** - * Temp roots openbuff itself writes into. + * OS temp roots path-taking tools may reach, for READS and WRITES alike. + * + * SCOPE: the whole root, not a name-gated namespace. Any path STRICTLY inside + * one of these roots resolves with `scope: 'owned-temp'` regardless of its + * segment names (`/notes.txt` exactly as much as + * `/openbuff-job-1.log`), except for mandatory-sensitive paths. The root + * ITSELF never resolves, for any scope. * * INJECTED-FILESYSTEM CAVEAT: these root NAMES always come from the host * process (`os.tmpdir()` and, on POSIX, `/tmp`), including when containment is @@ -226,7 +243,7 @@ let ownedTempComparisonRootsCache: string[] | undefined * those names is done through the adapter (see * `resolveProjectPathForFileSystem`). For a virtual or sandboxed filesystem in * which those names denote something other than the host temp dir, the - * owned-temp exception therefore grants reach to whatever the adapter maps + * temp-root exception therefore grants reach to whatever the adapter maps * them to. Adapters that must not expose host-named temp paths have to refuse * them themselves; this module cannot discover an adapter's temp root. */ @@ -247,9 +264,10 @@ export function getOwnedTempRoots(): string[] { } /** - * Owned temp roots in both lexical and symlink-dereferenced form. On macOS - * `os.tmpdir()` is a symlinked `/var/folders/...` path, so an owned file's - * realpath only lands under the dereferenced root. + * Temp roots in both lexical and symlink-dereferenced form. On macOS + * `os.tmpdir()` is a symlinked `/var/folders/...` path, so a temp file's + * realpath only lands under the dereferenced root — which is why containment + * compares against this PAIR instead of string-prefixing a single root. */ function getOwnedTempComparisonRoots(): string[] { if (!ownedTempComparisonRootsCache) { @@ -278,26 +296,54 @@ async function getOwnedTempComparisonRootsForFileSystem( } /** - * True when `target` is strictly inside one of `roots` and its first segment - * below that root matches an openbuff-owned namespace pattern. The temp root - * itself never qualifies. + * True when `target` is STRICTLY inside one of `roots`. The temp root itself + * never qualifies: it is not a readable file, and admitting it would silently + * widen directory-listing consumers to the root entry itself. + * + * WHY the rename from `isInsideOwnedTempNamespace`: the gate is now plain root + * containment, NOT an openbuff-owned name. The old helper additionally + * required the first segment below the root to match + * `OWNED_TEMP_SEGMENT_PATTERNS`, which made `/notes.txt` unreadable and + * every absolute temp write unreachable. Names are no longer part of the + * decision, so a helper named after a namespace would misdescribe it. + * + * Containment goes through `escapesRoot`, which is `path.relative`-based, so a + * sibling-prefix directory like `/tmpfoo` is correctly refused where a naive + * `startsWith` check would admit it. */ -function isInsideOwnedTempNamespace(target: string, roots: string[]): boolean { +function isInsideTempRoot(target: string, roots: string[]): boolean { return roots.some((root) => { const relative = path.relative(root, target) - if (relative === '' || escapesRoot(root, target)) return false - const firstSegment = relative.split(path.sep)[0] - return OWNED_TEMP_SEGMENT_PATTERNS.some((pattern) => - pattern.test(firstSegment), - ) + return relative !== '' && !escapesRoot(root, target) }) } +/** + * Win32 aliasing guard: the OS strips trailing dots/spaces from EVERY path + * segment, so `\.env ` resolves to the real `.env` while its basename + * fails the exact sensitive match — and an aliased INTERMEDIATE directory + * like `/.aws /config` opens the real `.aws/config` with an + * untouched basename. Refuse when the Win32-normalized path (ALL segments, + * not just the last) would be sensitive. No-op cost when no segment carries + * a trailing dot/space — the common case, and the universal case on POSIX, + * where such names are pathological. + */ +function refusesWin32AliasedSensitivePath(path: string): boolean { + const segments = path.split(/[\\/]+/) + const normalizedPath = segments + .map((segment) => segment.replace(/[ .]+$/, '')) + .join('/') + if (normalizedPath === segments.join('/')) return false + return isMandatorySensitiveReadPath(normalizedPath) +} + /** * Resolve the ALREADY-RESOLVED absolute `fullPath` to the ONE real path that * is both validated here and used by callers for the actual filesystem - * operation. Returns `null` when the path is not inside an openbuff-owned temp - * namespace. + * operation. Returns `null` when the path is not STRICTLY inside an OS temp + * root (`getOwnedTempRoots()`), when it is a mandatory-sensitive path, or + * when its Win32-normalized path would be mandatory-sensitive (see + * `refusesWin32AliasedSensitivePath`). * * The raw-input `..` policy is enforced by the entry points (see * `hasTraversalSegment`), never here: this function only ever sees collapsed @@ -310,24 +356,63 @@ function isInsideOwnedTempNamespace(target: string, roots: string[]): boolean { */ function resolveOwnedTempRealPath(fullPath: string): string | null { const roots = getOwnedTempComparisonRoots() - if (!isInsideOwnedTempNamespace(fullPath, roots)) return null + if (!isInsideTempRoot(fullPath, roots)) return null - // Critical guard: a symlink like `/tmp/openbuff-evil.log -> /etc/passwd` - // passes the lexical checks, so the dereferenced path must satisfy both - // root containment and the owned-namespace prefix as well. + // Critical guard: a symlink like `/tmp/notes.txt -> /etc/passwd` passes the + // lexical check, so the dereferenced path must satisfy root containment too. const realFullPath = realpathOrLexical(fullPath) - if (!isInsideOwnedTempNamespace(realFullPath, roots)) return null + if (!isInsideTempRoot(realFullPath, roots)) return null + + // Fail-closed sensitive refusal, checked on BOTH the lexical and the + // dereferenced path (a benign-looking temp name may link to + // `credentials.json` and vice versa) — the same shape + // `resolveExternalReadRealPath` uses below. + // + // WHY it lives in the resolver rather than in each handler: containment now + // admits the WHOLE temp root, so `/.env` and `/credentials.json` + // would otherwise be exposed to every current and future temp consumer, for + // reads AND writes. Unlike the retired first-segment namespace gate there is + // no longer any name pattern incidentally blocking them, so a handler that + // forgot the check would be the only thing between an agent and a + // dropped-in credential file. + // + // The FULL paths are passed (not just the basenames) so the path-aware + // credential carriers `isMandatorySensitiveReadPath` recognizes — + // `.kube/config`, `.docker/config.json`, `gh/hosts.yml`, `.aws/config` — are + // refused too. Those basenames are far too generic to block on their own, so + // a basename-only call would silently expose them under the temp root. + // + // `refusesWin32AliasedSensitivePath` extends the same refusal to Win32 + // aliasing (trailing dot/space), checked on the same two strings the caller + // opens so any refusal stays attributable to one of them. + if ( + isMandatorySensitiveReadPath(fullPath) || + isMandatorySensitiveReadPath(realFullPath) || + refusesWin32AliasedSensitivePath(fullPath) || + refusesWin32AliasedSensitivePath(realFullPath) + ) { + return null + } return realFullPath } /** - * True when `input` resolves inside an openbuff-owned temp namespace. + * True when `input` resolves STRICTLY inside an OS temp root + * (`getOwnedTempRoots()`), whatever its segment names: `/notes.txt` and + * `/other-tool-cache/x.log` qualify exactly like + * `/openbuff-job-1.log`. * - * Contract: a raw input containing a `..` segment is refused outright, even - * when it would collapse back into the namespace. `resolveProjectPath` and - * `resolveProjectPathForFileSystem` apply the same rule to their owned-temp - * fallback, so all three agree on any given input. + * Still FALSE for: the temp root itself (strictly-inside rule), a raw input + * containing a `..` segment (even one that collapses back inside the root), a + * temp-named path whose realpath escapes every temp root, and a + * mandatory-sensitive path such as `/.env` or `/credentials.json` + * (refused inside `resolveOwnedTempRealPath`, so the predicate and the + * resolvers can never disagree). + * + * Contract: the `..` rule is enforced here, above `path.resolve`. + * `resolveProjectPath` and `resolveProjectPathForFileSystem` apply the same + * rule to their owned-temp fallback, so all three agree on any given input. */ export function isOwnedTempPath(input: string): boolean { if (!input || hasTraversalSegment(input)) return false @@ -340,7 +425,7 @@ async function resolveOwnedTempRealPathForFileSystem( fileSystem: CodebuffFileSystem, ): Promise { const roots = await getOwnedTempComparisonRootsForFileSystem(fileSystem) - if (!isInsideOwnedTempNamespace(fullPath, roots)) return null + if (!isInsideTempRoot(fullPath, roots)) return null // Resolved once, exactly like the sync helper: the validated string is the // string callers operate on. @@ -348,16 +433,53 @@ async function resolveOwnedTempRealPathForFileSystem( fullPath, fileSystem, ) - if (!isInsideOwnedTempNamespace(realFullPath, roots)) return null + if (!isInsideTempRoot(realFullPath, roots)) return null + + // Identical fail-closed sensitive refusal as the sync resolver, on the same + // FULL paths. The two MUST NOT disagree: an operation routed through an + // injected filesystem would otherwise be the one place `/.env` or + // `/.aws/config` stayed reachable. The Win32-alias extension of the + // refusal (`refusesWin32AliasedSensitivePath`) runs on the same strings. + if ( + isMandatorySensitiveReadPath(fullPath) || + isMandatorySensitiveReadPath(realFullPath) || + refusesWin32AliasedSensitivePath(fullPath) || + refusesWin32AliasedSensitivePath(realFullPath) + ) { + return null + } return realFullPath } /** - * Build the containment result for an owned temp path. `relativePath` is the - * absolute resolved path: owned temp paths live outside the project, so a - * project-relative form would be meaningless (and would look like a traversal - * escape). Returning the absolute path keeps display and lookup honest. + * Async counterpart of `isOwnedTempPath` for injected filesystems. + * + * Exported so the SDK does not keep a private duplicate: the FS-aware + * re-validation `sdk/src/tools/path-utils.ts` needs for its synthesized + * top-level unlink candidate is exactly this predicate (adapter `realpath` + * plus fs-aware comparison roots), and a copy would have to be widened in + * lockstep with every change here. + */ +export async function isOwnedTempPathForFileSystem( + input: string, + fileSystem: CodebuffFileSystem, +): Promise { + if (!input || hasTraversalSegment(input)) return false + return ( + (await resolveOwnedTempRealPathForFileSystem( + path.resolve(input), + fileSystem, + )) !== null + ) +} + +/** + * Build the containment result for a path inside an OS temp root + * (`scope: 'owned-temp'`). `relativePath` is the absolute resolved path: temp + * paths live outside the project, so a project-relative form would be + * meaningless (and would look like a traversal escape). Returning the absolute + * path keeps display and lookup honest. * * Takes the ALREADY-RESOLVED absolute path from the caller: re-resolving the * raw input here would resolve a relative input against `process.cwd()` @@ -655,7 +777,9 @@ function isInsideExternalReadRoot(target: string, roots: string[]): boolean { * is both validated here and used by callers for the actual read. Returns * `null` when the path is not strictly inside a configured external read root * — including whenever the registry is unconfigured, since the comparison root - * list is then empty. + * list is then empty — and, like `resolveOwnedTempRealPath`, whenever the path + * is mandatory-sensitive or its Win32-normalized path would be (see + * `refusesWin32AliasedSensitivePath`). * * The raw-input `..` policy is enforced by the entry points (see * `hasTraversalSegment`), never here: this function only ever sees collapsed @@ -692,9 +816,15 @@ function resolveExternalReadRealPath(fullPath: string): string | null { // are refused too. Those basenames are far too generic to block on their own, // so a basename-only call would silently expose them inside an allowlisted // home-directory root. + // + // `refusesWin32AliasedSensitivePath` extends the same refusal to Win32 + // aliasing (trailing dot/space), so `/.env ` cannot read + // through as the real `.env` on Windows — matching the owned-temp resolver. if ( isMandatorySensitiveReadPath(fullPath) || - isMandatorySensitiveReadPath(realFullPath) + isMandatorySensitiveReadPath(realFullPath) || + refusesWin32AliasedSensitivePath(fullPath) || + refusesWin32AliasedSensitivePath(realFullPath) ) { return null } @@ -719,11 +849,14 @@ async function resolveExternalReadRealPathForFileSystem( if (!isInsideExternalReadRoot(realFullPath, roots)) return null // Identical fail-closed sensitive refusal as the sync resolver, on the same - // FULL paths; the two must never disagree about `credentials.json` or about a - // path-aware carrier like `.docker/config.json`. + // FULL paths; the two must never disagree about `credentials.json`, about a + // path-aware carrier like `.docker/config.json`, or about a Win32-aliased + // sensitive basename like `/.env `. if ( isMandatorySensitiveReadPath(fullPath) || - isMandatorySensitiveReadPath(realFullPath) + isMandatorySensitiveReadPath(realFullPath) || + refusesWin32AliasedSensitivePath(fullPath) || + refusesWin32AliasedSensitivePath(realFullPath) ) { return null } @@ -817,14 +950,18 @@ async function externalReadContainedPathForFileSystem( * - the symlink-dereferenced path resolves to a location outside the real * project root (e.g. an in-project symlink that points outside the repo). * - * Exception: paths inside an openbuff-owned OS temp namespace (see - * `isOwnedTempPath` — `openbuff-*` or `tmux-captures-*` directly under the - * temp root) are allowed even though they are outside the project, so - * path-taking tools can reach background-job logs, basher full logs and tmux - * captures. Such results carry `scope: 'owned-temp'` and an absolute - * `relativePath`; consumers must branch on `scope`. That exception - * additionally requires a traversal-free raw input, exactly like - * `isOwnedTempPath`. + * Exception: ANY path strictly inside an OS temp root (see `isOwnedTempPath`) + * is allowed even though it is outside the project, whatever its segment + * names — background-job logs, basher full logs and tmux captures, but + * equally an ordinary `/notes.txt` scratch file. Such results carry + * `scope: 'owned-temp'` and an absolute `relativePath`; consumers must branch + * on `scope`, never on absoluteness. The exception additionally requires a + * traversal-free raw input and refuses mandatory-sensitive paths + * (`/.env`, `/credentials.json`), exactly like `isOwnedTempPath`. + * + * BRANCH ORDER is load-bearing: the project check runs FIRST, so a project + * root that itself lives under the temp dir (common in tests) keeps resolving + * as `scope: 'project'` and keeps its in-project policy. * * This is the canonical, package-boundary-safe containment check. The SDK * (`sdk/src/tools/path-utils.ts`) and the agent runtime @@ -871,8 +1008,11 @@ export function resolveProjectPath( * filesystem instance; otherwise a virtual or wrapped filesystem could expose * symlinks that the host filesystem cannot see. * - * The owned-temp exception behaves exactly as in `resolveProjectPath`, with - * the host-derived root names caveat documented on `getOwnedTempRoots`. + * The temp-root exception (`scope: 'owned-temp'`) behaves exactly as in + * `resolveProjectPath` — whole-root strictly-inside containment, a + * traversal-free raw input, and the same fail-closed mandatory-sensitive + * refusal — with the host-derived root names caveat documented on + * `getOwnedTempRoots`. */ export async function resolveProjectPathForFileSystem( projectRoot: string, diff --git a/packages/agent-runtime/src/__tests__/run-agent-step-tools.test.ts b/packages/agent-runtime/src/__tests__/run-agent-step-tools.test.ts index bbb43a9a64..8d407eb784 100644 --- a/packages/agent-runtime/src/__tests__/run-agent-step-tools.test.ts +++ b/packages/agent-runtime/src/__tests__/run-agent-step-tools.test.ts @@ -3,6 +3,12 @@ import * as os from 'os' import * as path from 'path' import * as analytics from '@codebuff/common/analytics' import { TEST_USER_ID } from '@codebuff/common/old-constants' +import { + cleanupOutsideRoots, + makeOutsideRoot, + outsideRootsUsable, + removeScratchParentIfEmpty, +} from '@codebuff/common/testing' import { TEST_AGENT_RUNTIME_IMPL } from '@codebuff/common/testing/impl/agent-runtime' import { getInitialSessionState } from '@codebuff/common/types/session-state' import { promptSuccess } from '@codebuff/common/util/error' @@ -297,6 +303,8 @@ describe('runAgentStep - set_output tool', () => { afterAll(() => { clearAgentGeneratorCache() + cleanupOutsideRoots() + removeScratchParentIfEmpty() }) const mockFileContext: ProjectFileContext = { @@ -2151,15 +2159,20 @@ describe('runAgentStep - set_output tool', () => { ) }) - it('still hard-blocks writes to an openbuff-owned temp path', async () => { - // The owned-temp exception is read-only by construction: the SDK's - // filesystem-authority.ts owns the narrower owned-temp mutation policy - // (tmux captures are verification evidence a subagent must not forge), so - // this backstop must never pre-authorize a write there. - const ownedTempWrite = path.join( + it('does not hard-block writes to a temp path via the write exemption', async () => { + // The widened owned-temp exception admits WRITES under an OS temp root for + // the tools in OWNED_TEMP_WRITE_EXEMPT_TOOLS: their SDK write handlers are + // the authoritative containment layer (strictly-inside temp-root + // containment, fail-closed mandatory-sensitive refusal, and + // ownedTempMutationRefusal for live job artifacts / tmux capture evidence / + // executable basenames), so this backstop must not pre-refuse. Asserting + // BOTH halves keeps the pass attributable: no scope error chunk AND the + // tool call actually published (not silently swallowed for some other + // reason). + const tempWrite = path.join( getOwnedTempRoots()[0], - 'tmux-captures-session-1', - 'capture-001.txt', + 'openbuff-agent-runtime-write', + 'notes.txt', ) const chunks: unknown[] = [] runAgentStepBaseParams = { @@ -2168,9 +2181,9 @@ describe('runAgentStep - set_output tool', () => { } runAgentStepBaseParams.promptAiSdkStream = async function* ({}) { yield createToolCallChunk('write_file', { - path: ownedTempWrite, - instructions: 'Forge tmux capture evidence', - content: 'export const blocked = true\n', + path: tempWrite, + instructions: 'Scratch write under an OS temp root', + content: 'export const allowed = true\n', }) yield createToolCallChunk('end_turn', {}) return promptSuccess('mock-message-id') @@ -2191,29 +2204,89 @@ describe('runAgentStep - set_output tool', () => { localAgentTemplates: { 'unscoped-agent': unscopedAgent }, agentTemplate: unscopedAgent, agentState, - prompt: 'Write into the owned temp namespace', + prompt: 'Write into an OS temp root', + }) + + // No filesystem write scope error chunk... + expect(chunks).not.toContainEqual( + expect.objectContaining({ + type: 'error', + message: expect.stringContaining('filesystem write scope'), + }), + ) + // ...and the write is published as a tool call, so the pass cannot come + // from the call never having been dispatched at all. + expect(chunks).toContainEqual( + expect.objectContaining({ + type: 'tool_call', + toolName: 'write_file', + }), + ) + }) + + it('still hard-blocks a non-exempt read tool with a temp cwd (code_search)', async () => { + // Companion pinning the exemption as TOOL-scoped: code_search is absent + // from EXTERNAL_READ_EXEMPT_TOOLS (its handler realpaths the caller cwd and + // spawns ripgrep there with no containment resolution at all), so a temp + // cwd is still hard-blocked even though the same path would be allowed for + // read_files above. Remove code_search's exclusion and this refusal + // disappears, which is exactly the regression this test exists to catch. + const tempSearchCwd = path.join( + getOwnedTempRoots()[0], + 'openbuff-agent-runtime-search', + ) + const chunks: unknown[] = [] + runAgentStepBaseParams = { + ...runAgentStepBaseParams, + onResponseChunk: (chunk) => chunks.push(chunk), + } + runAgentStepBaseParams.promptAiSdkStream = async function* ({}) { + yield createToolCallChunk('code_search', { + pattern: 'apiKey', + cwd: tempSearchCwd, + }) + yield createToolCallChunk('end_turn', {}) + return promptSuccess('mock-message-id') + } + + const sessionState = getInitialSessionState(mockFileContext) + const agentState = sessionState.mainAgentState + const unscopedAgent: AgentTemplate = { + ...testAgent, + id: 'unscoped-agent', + toolNames: ['code_search', 'end_turn'], + filesystemScope: undefined, + } + + await runAgentStep({ + ...runAgentStepBaseParams, + agentType: 'unscoped-agent', + localAgentTemplates: { 'unscoped-agent': unscopedAgent }, + agentTemplate: unscopedAgent, + agentState, + prompt: 'Grep inside an OS temp root with a non-exempt tool', }) expect(chunks).toContainEqual( expect.objectContaining({ type: 'error', message: expect.stringContaining( - 'was blocked by the unscoped-agent filesystem write scope', + 'was blocked by the unscoped-agent filesystem read scope', ), }), ) expect(chunks).not.toContainEqual( expect.objectContaining({ type: 'tool_call', - toolName: 'write_file', + toolName: 'code_search', }), ) }) - it('still hard-blocks reads of a non-owned absolute temp sibling', async () => { - // Attribution guard: the allow above must come from owned-temp SCOPE, not - // from "any absolute temp path". This first segment matches no - // OWNED_TEMP_SEGMENT_PATTERNS entry, so the read stays hard-blocked. + it('does not hard-block reads of a non-openbuff absolute temp sibling', async () => { + // Positive counterpart to the owned-temp read test: with the widened + // whole-root scope, a temp path whose first segment matches no + // openbuff-owned pattern is reachable exactly like an owned one. const nonOwnedTempRead = path.join( getOwnedTempRoots()[0], 'not-openbuff-owned', @@ -2247,7 +2320,63 @@ describe('runAgentStep - set_output tool', () => { localAgentTemplates: { 'unscoped-agent': unscopedAgent }, agentTemplate: unscopedAgent, agentState, - prompt: 'Read an unowned absolute temp path', + prompt: 'Read an ordinary absolute temp path', + }) + + // No read-scope error chunk... + expect(chunks).not.toContainEqual( + expect.objectContaining({ + type: 'error', + message: expect.stringContaining('filesystem read scope'), + }), + ) + // ...and the read is published as a tool call. + expect(chunks).toContainEqual( + expect.objectContaining({ + type: 'tool_call', + toolName: 'read_files', + }), + ) + }) + + it('still hard-blocks a read path outside both the project and every temp root', async () => { + // Attribution guard for the two temp-read allows above: they must come from + // owned-temp SCOPE, not from "any absolute path". This fixture root sits + // outside BOTH the project root and every OS temp root, so the absolute + // escape stays hard-blocked. + if (!outsideRootsUsable()) return + const outsideRoot = makeOutsideRoot('agent-runtime-read-out-') + const outsideRead = path.join(outsideRoot, 'file.txt') + fs.writeFileSync(outsideRead, 'outside\n') + const chunks: unknown[] = [] + runAgentStepBaseParams = { + ...runAgentStepBaseParams, + onResponseChunk: (chunk) => chunks.push(chunk), + } + runAgentStepBaseParams.promptAiSdkStream = async function* ({}) { + yield createToolCallChunk('read_files', { + paths: [outsideRead], + }) + yield createToolCallChunk('end_turn', {}) + return promptSuccess('mock-message-id') + } + + const sessionState = getInitialSessionState(mockFileContext) + const agentState = sessionState.mainAgentState + const unscopedAgent: AgentTemplate = { + ...testAgent, + id: 'unscoped-agent', + toolNames: ['read_files', 'end_turn'], + filesystemScope: undefined, + } + + await runAgentStep({ + ...runAgentStepBaseParams, + agentType: 'unscoped-agent', + localAgentTemplates: { 'unscoped-agent': unscopedAgent }, + agentTemplate: unscopedAgent, + agentState, + prompt: 'Read an absolute path outside every boundary', }) expect(chunks).toContainEqual( @@ -2272,9 +2401,9 @@ describe('runAgentStep - set_output tool', () => { // `readableRoots`), so this runtime backstop must not refuse them. The SDK // resolvers stay authoritative — including the fail-closed // mandatory-sensitive refusal — this layer only stops pre-dispatch refusal. - const externalRoot = fs.mkdtempSync( - path.join(os.tmpdir(), 'external-read-backstop-'), - ) + // The root must sit outside the OS temp roots too, or the widened temp + // write/read exception would cover it before the allowlist is consulted. + const externalRoot = makeOutsideRoot('external-read-backstop-') const externalRead = path.join(externalRoot, 'notes.txt') fs.writeFileSync(externalRead, 'notes\n') // Module state: reset before configuring so a differing set from an earlier @@ -2340,10 +2469,11 @@ describe('runAgentStep - set_output tool', () => { it('still hard-blocks writes to an allowlisted external path', async () => { // The external allowlist is READ-only by construction (there is no // external-write scope), so this backstop must never pre-authorize a write - // there — the exception stays gated on access === 'read'. - const externalRoot = fs.mkdtempSync( - path.join(os.tmpdir(), 'external-read-backstop-write-'), - ) + // there — the exception stays gated on access === 'read'. The root must + // sit outside the OS temp roots too: the write exemption deliberately does + // not consult isExternalReadPath, so a temp-rooted fixture would be allowed + // by the temp write path instead of blocked for the external reason. + const externalRoot = makeOutsideRoot('external-read-backstop-write-') const externalWrite = path.join(externalRoot, 'notes.txt') fs.writeFileSync(externalWrite, 'notes\n') resetExternalReadRootsForTesting() @@ -2406,10 +2536,10 @@ describe('runAgentStep - set_output tool', () => { it('still hard-blocks reads of a non-allowlisted external sibling', async () => { // Attribution guard: the allow above must come from the ALLOWLIST, not from // "any absolute path outside the project". The sibling directory shares the - // allowlisted root's prefix, which a naive startsWith check would admit. - const externalRoot = fs.mkdtempSync( - path.join(os.tmpdir(), 'external-read-backstop-sibling-'), - ) + // allowlisted root's prefix, which a naive startsWith check would admit, and + // both sit outside the OS temp roots so the temp exception stays out of the + // picture. + const externalRoot = makeOutsideRoot('external-read-backstop-sibling-') const siblingRoot = `${externalRoot}-evil` fs.mkdirSync(siblingRoot) const siblingRead = path.join(siblingRoot, 'notes.txt') @@ -2477,9 +2607,7 @@ describe('runAgentStep - set_output tool', () => { // EXTERNAL_READ_EXEMPT_TOOLS and stays hard-blocked even for a configured // allowlisted root — otherwise code_search({ cwd: '/projects' }) // would recursively grep other projects' persisted transcripts. - const externalRoot = fs.mkdtempSync( - path.join(os.tmpdir(), 'external-read-code-search-'), - ) + const externalRoot = makeOutsideRoot('external-read-code-search-') fs.writeFileSync(path.join(externalRoot, 'notes.txt'), 'notes\n') resetExternalReadRootsForTesting() configureExternalReadRoots([externalRoot]) @@ -2543,9 +2671,7 @@ describe('runAgentStep - set_output tool', () => { // migrated tool (its SDK handler resolves through the read-only containment // resolvers), so the same configured root that code_search cannot reach // stays readable here. - const externalRoot = fs.mkdtempSync( - path.join(os.tmpdir(), 'external-read-exempt-read-files-'), - ) + const externalRoot = makeOutsideRoot('external-read-exempt-read-files-') const externalRead = path.join(externalRoot, 'notes.txt') fs.writeFileSync(externalRead, 'notes\n') resetExternalReadRootsForTesting() diff --git a/packages/agent-runtime/src/tools/handlers/tool/find-files.ts b/packages/agent-runtime/src/tools/handlers/tool/find-files.ts index e86db2cdb7..07a9487a55 100644 --- a/packages/agent-runtime/src/tools/handlers/tool/find-files.ts +++ b/packages/agent-runtime/src/tools/handlers/tool/find-files.ts @@ -1,3 +1,7 @@ +import fs from 'node:fs' +import os from 'node:os' +import path from 'node:path' + import { isReadFilesResultV1 } from '@codebuff/common/tools/results/filesystem' import { jsonToolResult } from '@codebuff/common/util/messages' @@ -24,9 +28,52 @@ import type { import type { AgentState } from '@codebuff/common/types/session-state' import type { ProjectFileContext } from '@codebuff/common/util/file' -// Turn this on to collect full file context, using Claude-4-Opus to pick which files to send up -// TODO: We might want to be able to turn this on on a per-repo basis. -const COLLECT_FULL_FILE_CONTEXT = false +// Whether to collect full file context (using Claude-4-Opus to pick which +// files to send up). Defaults off; enable per-repo by setting the +// OPENBUFF_COLLECT_FULL_FILE_CONTEXT env var to "1"/"true"/"yes"/"on" +// (case-insensitive), or by adding +// `"collectFullFileContext": true` to the project's openbuff.json under +// `experimental`. +const OPENBUFF_CONFIG_FILE_NAME = 'openbuff.json' +// Maximum number of ancestor directories to scan for openbuff.json. Mirrors +// the SDK's provider-config bound: a monorepo workspace root is typically +// 3-5 levels above a subpackage, so 10 comfortably covers legitimate cases +// while guaranteeing the walk terminates before reaching the filesystem root. +const MAX_ANCESTOR_SCAN_DEPTH = 10 +function isFullFileContextEnabled(): boolean { + const envValue = process.env.OPENBUFF_COLLECT_FULL_FILE_CONTEXT + if (envValue !== undefined) { + return /^(1|true|yes|on)$/i.test(envValue.trim()) + } + // Env var unset: check openbuff.json in the project root or ancestor + // directories for experimental.collectFullFileContext. Bounded walk that + // stops at the home directory boundary (security: never walk above the + // user's home, matching the SDK's provider-config behavior). + let currentDir = path.resolve(process.cwd()) + const home = os.homedir() + for (let depth = 0; depth < MAX_ANCESTOR_SCAN_DEPTH; depth++) { + const configPath = path.join(currentDir, OPENBUFF_CONFIG_FILE_NAME) + if (fs.existsSync(configPath)) { + try { + const config = JSON.parse(fs.readFileSync(configPath, 'utf8')) + if (config?.experimental?.collectFullFileContext === true) { + return true + } + } catch { + // Malformed config at this level — skip and keep walking. + } + } + if (currentDir === home) { + return false + } + const parentDir = path.dirname(currentDir) + if (parentDir === currentDir) { + return false + } + currentDir = parentDir + } + return false +} export const handleFindFiles = (async ( params: { @@ -47,7 +94,7 @@ export const handleFindFiles = (async ( 'messages' | 'system' | 'assistantPrompt' > & ParamsExcluding< - typeof uploadExpandedFileContextForTraining, + typeof prepareExpandedFileContextForTraining, 'messages' | 'system' | 'assistantPrompt' > & ParamsExcluding, @@ -103,8 +150,8 @@ export const handleFindFiles = (async ( : [], ) - if (COLLECT_FULL_FILE_CONTEXT && addedFiles.length > 0) { - uploadExpandedFileContextForTraining({ + if (isFullFileContextEnabled() && addedFiles.length > 0) { + prepareExpandedFileContextForTraining({ ...params, messages: agentState.messageHistory, system, @@ -112,7 +159,7 @@ export const handleFindFiles = (async ( }).catch((error) => { logger.error( { error }, - 'Error uploading expanded file context for training', + 'Error preparing expanded file context for training', ) }) } @@ -138,7 +185,7 @@ export const handleFindFiles = (async ( } }) satisfies CodebuffToolHandlerFunction<'find_files'> -async function uploadExpandedFileContextForTraining( +async function prepareExpandedFileContextForTraining( params: { requestFiles: RequestFilesFn } & ParamsOf, @@ -148,7 +195,7 @@ async function uploadExpandedFileContextForTraining( const loadedFiles = await requestFiles({ filePaths: files }) - // Upload a map of: + // Prepare a map of: // {file_path: {content, token_count}} // up to 50k tokens const filesToUpload: Record = {} @@ -177,4 +224,9 @@ async function uploadExpandedFileContextForTraining( } filesToUpload[file] = { content, tokens } } + + // TODO: Upload mechanism not yet implemented. filesToUpload is prepared + // (file_path -> {content, tokens}, capped at 50k tokens per file) but the + // upload endpoint/API/storage target is unknown. Re-enable this upload + // once the upload mechanism is defined. } diff --git a/packages/agent-runtime/src/tools/handlers/tool/spawn-agent-inline.ts b/packages/agent-runtime/src/tools/handlers/tool/spawn-agent-inline.ts index b4ebf54674..505a5b6557 100644 --- a/packages/agent-runtime/src/tools/handlers/tool/spawn-agent-inline.ts +++ b/packages/agent-runtime/src/tools/handlers/tool/spawn-agent-inline.ts @@ -285,7 +285,8 @@ export const handleSpawnAgentInline = (async ( parentSystemPrompt: system, parentTools, onResponseChunk: (chunk: string | PrintModeEvent) => { - // Inherits parent's onResponseChunk, except for context-pruner (TODO: add an option for it to be silent?) + // Inherits parent's onResponseChunk, except for context-pruner (keeps + // progress tick only — see the `else` branch below). if (!isContextPruner) { if (typeof chunk === 'string') { writeToClient(chunk) diff --git a/packages/agent-runtime/src/tools/tool-executor.ts b/packages/agent-runtime/src/tools/tool-executor.ts index 06347f836d..f92798ede8 100644 --- a/packages/agent-runtime/src/tools/tool-executor.ts +++ b/packages/agent-runtime/src/tools/tool-executor.ts @@ -93,6 +93,7 @@ import type { ProjectFileContext, } from '@codebuff/common/util/file' import type { ToolCallPart, ToolSet } from 'ai' +import type { LanguageModelV2StreamPart } from '@ai-sdk/provider' export type CustomToolCall = { toolName: string @@ -188,6 +189,92 @@ export function buildSpawnAgentsHandlerFailureOutput( ) } +type SpawnGateFilterResult = { + agents: unknown + aborted: boolean + abortValue: Promise | null +} + +/** + * Deterministically blocks git-committer spawns until the validation/reviewer + * gate has passed. canSuggestFollowups is false precisely when the gate is + * not green (edits pending review). This mirrors the suggest_followups guard + * in executeToolCall and enforces the harness ordering: commit only after + * review is green. When canSuggestFollowups is undefined (gate system not + * active, e.g. non-base2 agents), this helper is NOT called by the caller. + * + * Only the git-committer entry is filtered; co-batched legitimate agents + * proceed normally. Extracted into a dedicated helper so the pre-gate + * refusal is unit-testable and re-used by any spawn_agents entry point. + */ +function filterSpawnAgentsGate(params: { + agents: unknown +onResponseChunk: (chunk: string | PrintModeEvent) => void +}): SpawnGateFilterResult { + const { agents: inputAgents, onResponseChunk } = params + if (!Array.isArray(inputAgents)) { + return { agents: inputAgents, aborted: false, abortValue: null } + } + const filteredAgents = inputAgents.filter( + (agent) => + !( + agent && + typeof agent === 'object' && + typeof (agent as Record).agent_type === 'string' && + // Match on the resolved agent id so a git-committer alias cannot + // bypass the pre-gate block (consistent with spawn resolution). + normalizeSpawnAgentType( + String((agent as Record).agent_type), + ) === 'git-committer' + ), + ) + if (filteredAgents.length === inputAgents.length) { + return { agents: inputAgents, aborted: false, abortValue: null } + } + onResponseChunk({ + type: 'error', + message: + 'git-committer withheld: GATE: PENDING (need GATE: PASSED / phase=final_response_allowed). End your turn; do not retry or predict gate progress. Spawn git-committer once after GATE: PASSED.', + userMessage: GIT_COMMITTER_WITHHELD_USER_MESSAGE, + autoRecovering: true, + }) + if (filteredAgents.length === 0) { + return { + agents: filteredAgents, + aborted: true, + abortValue: Promise.resolve(), + } + } + return { agents: filteredAgents, aborted: false, abortValue: null } +} + +/** + * Wrapper that returns abortablePreviousToolCallFinished when the gate + * blocks all agents. The early-return value must match executeToolCall's + * Promise return type, so callers can `return` it directly. + */ +function resolveSpawnGateAbort( + result: SpawnGateFilterResult, + abortablePreviousToolCallFinished: Promise, +): { + agents: unknown + shouldReturn: boolean + returnValue: Promise +} { + if (result.aborted) { + return { + agents: result.agents, + shouldReturn: true, + returnValue: abortablePreviousToolCallFinished, + } + } + return { + agents: result.agents, + shouldReturn: false, + returnValue: abortablePreviousToolCallFinished, + } +} + export function normalizeNativeToolOutput(params: { toolName: T toolCallId: string @@ -1721,6 +1808,38 @@ const EXTERNAL_READ_EXEMPT_TOOLS = new Set([ 'list_directory', ]) +// Tools whose SDK handler is the authoritative containment layer for the +// owned-temp WRITE relaxation in executeToolCall (OS temp roots resolve as +// `scope: 'owned-temp'`). +// +// INVARIANT: a tool belongs here ONLY if its SDK handler routes every +// caller-supplied path through the write resolvers +// (`resolveFilePathFor*Operation` in `sdk/src/tools/path-utils.ts`, or +// `FilesystemAuthority.authorizePath`, which itself resolves through them). +// Those resolvers remain authoritative: they apply strictly-inside temp-root +// containment with a single realpath dereference, the fail-closed +// mandatory-sensitive refusal (so `/.env` and `/credentials.json` +// stay unwritable), `ownedTempMutationRefusal` (live background-job artifacts, +// tmux capture evidence, executable-extension basenames) and the same +// conditional-commit / expected-state revalidation project writes get. This +// backstop is NOT that layer — it defers to the handler — so exempting a tool +// whose handler does not contain would remove the only check that exists for +// it. +// +// `write_audit_findings` is DELIBERATELY ABSENT: it derives a project-relative +// `.agents/sessions//findings/.md` path and never accepts an +// absolute one, so it has no temp path to exempt. +const OWNED_TEMP_WRITE_EXEMPT_TOOLS = new Set([ + 'apply_patch', + 'apply_smart_patch', + 'edit_3d_asset', + 'edit_transaction', + 'replace_range', + 'rewrite_symbol', + 'str_replace', + 'write_file', +]) + const MAX_CUSTOM_INPUT_SCAN_DEPTH = 6 const MAX_CUSTOM_INPUT_SCAN_STRINGS = 1000 @@ -2046,35 +2165,40 @@ export async function executeToolCall( // lexical scope for missing paths so create operations still work. } } - // READ-ONLY owned-temp and external-read exceptions. The SDK - // deliberately permits reads under the openbuff-owned OS temp namespace - // (see read-files.ts `authorizeReadTarget` and read-logs.ts): that is how - // a parent agent reads back tmux capture evidence and background-job - // logs. It equally permits reads strictly inside a root the user - // explicitly allowlisted (the openbuff config directory for logs/state, - // plus any `readableRoots` entry in openbuff.json). This backstop has no - // notion of either namespace, so without the exceptions it refuses reads - // the SDK is designed to allow. + // READ-ONLY owned-temp and external-read exceptions, plus the WRITE-side + // owned-temp exception. The SDK deliberately permits reads under the OS + // temp roots (see read-files.ts `authorizeReadTarget` and read-logs.ts): + // that is how a parent agent reads back tmux capture evidence and + // background-job logs, and since containment widened to the whole temp + // root it is also how an agent reads an ordinary `/notes.txt`. It + // equally permits reads strictly inside a root the user explicitly + // allowlisted (the openbuff config directory for logs/state, plus any + // `readableRoots` entry in openbuff.json). This backstop has no notion of + // either namespace, so without the exceptions it refuses reads the SDK is + // designed to allow. // - // The SDK read handlers of the EXEMPT tools (see - // EXTERNAL_READ_EXEMPT_TOOLS) remain AUTHORITATIVE for both: they run - // the real containment resolution (symlink dereferencing, - // strictly-inside checks, and the fail-closed mandatory-sensitive - // refusal that keeps `credentials.json` unreadable inside an allowlisted - // config root). This layer only stops pre-dispatch refusal of paths - // those handlers will validate themselves, so a tool whose handler does - // NOT contain (e.g. code_search) is never exempted. + // The SDK handlers of the EXEMPT tools (see EXTERNAL_READ_EXEMPT_TOOLS + // and OWNED_TEMP_WRITE_EXEMPT_TOOLS) remain AUTHORITATIVE: they run the + // real containment resolution (symlink dereferencing, strictly-inside + // checks, and the fail-closed mandatory-sensitive refusal that keeps + // `credentials.json` unreadable and unwritable), and the write handlers + // additionally apply `ownedTempMutationRefusal` plus the usual + // conditional-commit revalidation. This layer only stops pre-dispatch + // refusal of paths those handlers will validate themselves, so a tool + // whose handler does NOT contain (e.g. code_search, glob, + // find_files_matching_content — they resolve an arbitrary caller `cwd` + // and spawn a search there) is never exempted, for reads or writes. // - // Access-scoped AND tool-scoped. Access: a WRITE to an owned-temp or - // allowlisted-external path keeps hard-blocking here. The - // (narrower) owned-temp mutation policy is owned by the SDK's - // filesystem-authority.ts `ownedTempMutationRefusal` — tmux captures are - // verification evidence a subagent must not be able to forge — and the - // external allowlist is READ-only by construction (there is no - // `external-write` scope), so this layer must not pre-authorize any - // mutation of either. + // Access-scoped AND tool-scoped: + // - `isExternalReadPath` MUST NOT be consulted for writes. The external + // allowlist is READ-only by construction (there is no + // `external-write` scope), so a write into an allowlisted root stays + // hard-blocked here. + // - the owned-temp write exemption is tool-scoped for exactly the same + // reason the read one is: membership in the Set is a claim about the + // handler, not about the path. // - // Both predicates get the RAW caller path: each resolves its own input + // Every predicate gets the RAW caller path: each resolves its own input // and refuses any raw `..` segment itself, which is exactly the guard we // want. The project-relative `normalized` form would be a meaningless // `../..`-style string here. @@ -2082,25 +2206,34 @@ export async function executeToolCall( filesystemAccess.access === 'read' && EXTERNAL_READ_EXEMPT_TOOLS.has(toolName) && (isOwnedTempPath(rawPath) || isExternalReadPath(rawPath)) + // Ordering is deliberate on this hot path: the cheap access/Set checks + // short-circuit before the filesystem-touching predicate, and + // `isOwnedTempPath` only runs for paths that already failed the + // in-project check. + const ownedTempWriteAllowed = + filesystemAccess.access === 'write' && + OWNED_TEMP_WRITE_EXEMPT_TOOLS.has(toolName) && + isOwnedTempPath(rawPath) + const containmentExempt = externalReadAllowed || ownedTempWriteAllowed // A path "escapes" the project when it traverses above the root or is // absolute (either lexically or after canonicalization). Escapes are the // real containment boundary: an agent must never read or write outside - // the project, so these are always hard-blocked regardless of access — - // except for the owned-temp / allowlisted-external reads above. + // the project, so these are always hard-blocked — except for the exempt + // owned-temp / allowlisted-external cases above. const escapesProject = - !externalReadAllowed && + !containmentExempt && (normalizedEscapesProject(normalized) || normalizedEscapesProject(canonical)) // An in-project path is a scope mismatch when it stays inside the project // but does not match the agent's declared filesystemScope patterns. Only // meaningful when the agent declared a scope for this access type. An - // owned-temp or allowlisted-external read is not in-project, so it is - // never pattern-matched against filesystemScope globs: it is neither + // exempt owned-temp or allowlisted-external path is not in-project, so it + // is never pattern-matched against filesystemScope globs: it is neither // hard-blocked above nor spuriously warned about below. const scopeMismatch = allowedPatterns !== undefined && !escapesProject && - !externalReadAllowed && + !containmentExempt && !allowedPatterns.some( (pattern) => scopePatternMatches(normalized, pattern) && @@ -2221,47 +2354,26 @@ export async function executeToolCall( false } - // TODO: Allow tools to provide a validation function, and move this logic into the spawn_agents validation function. - // Pre-validate spawn_agents to filter out non-existent agents before streaming + // Pre-validate spawn_agents to filter out non-existent agents before + // streaming. The git-committer gate filter is extracted into a dedicated + // helper so it can be unit-tested and re-used by any spawn_agents entry + // point. let effectiveInput = toolCall.input as Record - // Deterministically block git-committer spawns until the validation/reviewer - // gate has passed. canSuggestFollowups is false precisely when the gate is - // not green (edits pending review). This mirrors the suggest_followups guard - // above and enforces the harness ordering: commit only after review is green. - // When canSuggestFollowups is undefined (gate system not active, e.g. non-base2 - // agents), the check is skipped so custom agents are unaffected. - // Only the git-committer entry is filtered; co-batched legitimate agents - // proceed normally, consistent with the spawn_agents pre-validation pattern. if (toolName === 'spawn_agents' && canSuggestFollowups === false) { - const agents = effectiveInput.agents - if (Array.isArray(agents)) { - const filteredAgents = agents.filter( - (agent) => - !( - agent && - typeof agent === 'object' && - typeof (agent as Record).agent_type === 'string' && - // Match on the resolved agent id so a git-committer alias cannot - // bypass the pre-gate block (consistent with spawn resolution). - normalizeSpawnAgentType( - String((agent as Record).agent_type), - ) === 'git-committer' - ), - ) - if (filteredAgents.length < agents.length) { - onResponseChunk({ - type: 'error', - message: - 'git-committer withheld: GATE: PENDING (need GATE: PASSED / phase=final_response_allowed). End your turn; do not retry or predict gate progress. Spawn git-committer once after GATE: PASSED.', - userMessage: GIT_COMMITTER_WITHHELD_USER_MESSAGE, - autoRecovering: true, - }) - if (filteredAgents.length === 0) { - return abortablePreviousToolCallFinished - } - effectiveInput = { ...effectiveInput, agents: filteredAgents } - } + const gateResult = filterSpawnAgentsGate({ + agents: effectiveInput.agents, + onResponseChunk, + }) + const resolved = resolveSpawnGateAbort( + gateResult, + abortablePreviousToolCallFinished, + ) + if (resolved.shouldReturn) { + return resolved.returnValue + } + if (resolved.agents !== effectiveInput.agents) { + effectiveInput = { ...effectiveInput, agents: resolved.agents } } } diff --git a/packages/internal/src/openai-compatible/chat/openai-compatible-chat-language-model.ts b/packages/internal/src/openai-compatible/chat/openai-compatible-chat-language-model.ts index 3108e08da5..c4d3625fa9 100644 --- a/packages/internal/src/openai-compatible/chat/openai-compatible-chat-language-model.ts +++ b/packages/internal/src/openai-compatible/chat/openai-compatible-chat-language-model.ts @@ -554,18 +554,18 @@ export class OpenAICompatibleChatLanguageModel implements LanguageModelV2 { finishEmitted = true } + const chunkSchema = this.chunkSchema return { stream: response.pipeThrough( new TransformStream< - ParseResult>, + ParseResult>, LanguageModelV2StreamPart >({ start(controller) { controller.enqueue({ type: 'stream-start', warnings }) }, - // TODO we lost type safety on Chunk, most likely due to the error schema. MUST FIX - transform(chunk, controller) { + transform(chunk: ParseResult>, controller) { // Emit raw chunk if requested (before anything else) if (options.includeRawChunks) { controller.enqueue({ type: 'raw', rawValue: chunk.rawValue }) diff --git a/packages/internal/src/openrouter-ai-sdk/chat/index.ts b/packages/internal/src/openrouter-ai-sdk/chat/index.ts index 5ba30e4677..2ef74a3c43 100644 --- a/packages/internal/src/openrouter-ai-sdk/chat/index.ts +++ b/packages/internal/src/openrouter-ai-sdk/chat/index.ts @@ -164,17 +164,24 @@ export class OpenRouterChatLanguageModel implements LanguageModelV2 { } if (tools && tools.length > 0) { - // TODO: support built-in tools const mappedTools = tools - .filter((tool) => tool.type === 'function') - .map((tool) => ({ - type: 'function' as const, - function: { - name: tool.name, - description: tool.description, - parameters: tool.inputSchema, - }, - })) + .map((tool) => { + if (tool.type === 'provider-defined') { + return { + type: 'provider-defined' as const, + id: tool.id, + name: tool.name, + } + } + return { + type: 'function' as const, + function: { + name: tool.name, + description: tool.description, + parameters: tool.inputSchema, + }, + } + }) return { ...baseArgs, diff --git a/sdk/src/__tests__/filesystem-authority.test.ts b/sdk/src/__tests__/filesystem-authority.test.ts index ee4bfdf2b9..bc2741ce16 100644 --- a/sdk/src/__tests__/filesystem-authority.test.ts +++ b/sdk/src/__tests__/filesystem-authority.test.ts @@ -527,6 +527,123 @@ describe('FilesystemAuthority owned-temp namespace permits CRUD except live job }, ) + test.each([ + ['create', '.js'], + ['move', '.js'], + ['create', '.py'], + ['move', '.py'], + ] as const)( + 'refuses %s of a plain non-openbuff owned-temp path ending in %s', + async (operation, extension) => { + // With containment admitting the whole temp root, this refusal is the + // only defense against a file-changing tool staging a script that a + // later `node /tmp/x.js` / `python3 /tmp/x.py` executes: read-only + // terminal profiles refuse interpreter one-liners but permit + // `node `, and the /tmp/... token exemption carries the command. + // A plain segment directly under the temp root — no openbuff naming — + // pins that the interpreter-extension refusal does not depend on any + // owned-name pattern. `move` authorizes the DESTINATION path here, + // matching how change_file authorizes both ends of a move. + const result = await authority.authorizePath( + path.join(ownedTempRoot, `payload-${uniqueSuffix()}${extension}`), + operation, + ) + expect(result).toEqual({ + allowed: false, + code: 'owned_temp_executable_extension_refused', + }) + }, + ) + + test.each([ + ['create', '.sh '], + ['move', '.sh '], + ['create', '.py.'], + ['move', '.py.'], + ] as const)( + 'refuses %s of an aliased plain owned-temp executable name ending in %s', + async (operation, aliasedSuffix) => { + // Win32 strips trailing dots/spaces from the final path segment, so the + // lexical alias `payload-.sh ` / `payload-.py.` creates the real + // `payload-.sh` / `payload-.py` while its raw extname (`.sh ` / + // `.`) misses OWNED_TEMP_REFUSED_EXTENSIONS. The refusal must also run + // on the Win32-normalized basename, or the alias dodges the only + // defense against the terminal-policy bypass the non-aliased tests + // above document. + const result = await authority.authorizePath( + path.join(ownedTempRoot, `payload-${uniqueSuffix()}${aliasedSuffix}`), + operation, + ) + expect(result).toEqual({ + allowed: false, + code: 'owned_temp_executable_extension_refused', + }) + }, + ) + + test('refuses overwrite of a Win32-aliased background-job log name', async () => { + // `openbuff-job-.log ` fails BACKGROUND_JOB_FILE_PATTERN raw (the + // trailing space sits before the pattern's `$`), but Win32 creates the + // real `openbuff-job-.log`, so the aliased basename must be + // normalized before the job-artifact refusal or a tool-side overwrite + // could clobber a live job log through its alias. + const result = await authority.authorizePath( + path.join(ownedTempDir, `openbuff-job-${uniqueSuffix()}.log `), + 'overwrite', + ) + expect(result).toEqual({ + allowed: false, + code: 'owned_temp_job_artifact_read_only', + }) + }) + + test('refuses create under a Win32-aliased tmux-captures segment', async () => { + // `tmux-captures- ` fails TMUX_CAPTURE_DIR_PATTERN raw (trailing + // space before the `$`), but Win32 creates the real `tmux-captures-` + // directory, so the segment check must also run on the normalized form or + // capture evidence could be forged under the alias. The aliased directory + // is deliberately NOT created: create authorization resolves + // not-yet-existing segments lexically, which is exactly the alias form + // being refused. + const result = await authority.authorizePath( + path.join( + ownedTempRoot, + `tmux-captures-${uniqueSuffix()} `, + 'capture-0001.txt', + ), + 'create', + ) + expect(result).toEqual({ + allowed: false, + code: 'owned_temp_capture_read_only', + }) + }) + + test('still allows create of a plain non-executable owned-temp payload name', async () => { + // Positive control for the aliased refusals above: the Win32-normalized + // check must not widen the refusal to ordinary payload names. + const result = await authority.authorizePath( + path.join(ownedTempRoot, `payload-${uniqueSuffix()}.txt`), + 'create', + ) + expect(result).toMatchObject({ allowed: true }) + if (!result.allowed) throw new Error(result.code) + expect(result.path.scope).toBe('owned-temp') + }) + + test('still allows delete of a Win32-aliased payload.sh name (cleanup carve-out)', async () => { + // The extension refusal stops STAGING a script; deleting one is cleanup + // and stays allowed even in aliased form — the delete carve-out is + // preserved unchanged by the normalization repair. + const result = await authority.authorizePath( + path.join(ownedTempRoot, `payload-${uniqueSuffix()}.sh `), + 'delete', + ) + expect(result).toMatchObject({ allowed: true }) + if (!result.allowed) throw new Error(result.code) + expect(result.path.scope).toBe('owned-temp') + }) + test.each(['create', 'overwrite', 'delete', 'move'] as const)( 'refuses %s on a live background-job log', async (operation) => { diff --git a/sdk/src/__tests__/glob.test.ts b/sdk/src/__tests__/glob.test.ts index 8558d3f803..521324ea8e 100644 --- a/sdk/src/__tests__/glob.test.ts +++ b/sdk/src/__tests__/glob.test.ts @@ -1,5 +1,10 @@ import * as projectFileTree from '@codebuff/common/project-file-tree' -import { describe, test, expect, afterEach, spyOn } from 'bun:test' +import { + makeOutsideRoot, + outsideRootsUsable, + removeScratchParentIfEmpty, +} from '@codebuff/common/testing' +import { describe, test, expect, afterAll, afterEach, spyOn } from 'bun:test' import nodeFs from 'node:fs' import os from 'node:os' import path from 'node:path' @@ -8,6 +13,8 @@ import { glob } from '../tools/glob' import type { CodebuffFileSystem } from '@codebuff/common/types/filesystem' +afterAll(removeScratchParentIfEmpty) + const PROJECT_PATH = '/project' // The glob tool only uses `getProjectFileTree` + `flattenTree` to enumerate @@ -179,10 +186,12 @@ describe('glob tool', () => { }) test('rejects a cwd symlink that escapes the project', async () => { + if (!outsideRootsUsable()) return const projectRoot = nodeFs.mkdtempSync(path.join(os.tmpdir(), 'glob-root-')) - const outsideRoot = nodeFs.mkdtempSync( - path.join(os.tmpdir(), 'glob-outside-'), - ) + // The symlink TARGET must sit outside the OS temp roots too, or the + // widened temp exception would legitimately admit this cwd and the + // escape refusal would never fire. + const outsideRoot = makeOutsideRoot('glob-outside-') nodeFs.symlinkSync(outsideRoot, path.join(projectRoot, 'escape')) mockFileTree(['src/a.ts']) diff --git a/sdk/src/__tests__/path-utils.test.ts b/sdk/src/__tests__/path-utils.test.ts index 4e0c5bad8d..dcdf403924 100644 --- a/sdk/src/__tests__/path-utils.test.ts +++ b/sdk/src/__tests__/path-utils.test.ts @@ -1,8 +1,19 @@ import fs from 'fs' import os from 'os' import path from 'path' -import { afterEach, beforeEach, describe, expect, test } from 'bun:test' - +import { + afterAll, + afterEach, + beforeEach, + describe, + expect, + test, +} from 'bun:test' +import { + makeOutsideRoot, + outsideRootsUsable, + removeScratchParentIfEmpty, +} from '@codebuff/common/testing' import { configureExternalReadRoots, resetExternalReadRootsForTesting, @@ -21,6 +32,8 @@ import { import type { CodebuffFileSystem } from '@codebuff/common/types/filesystem' +afterAll(removeScratchParentIfEmpty) + describe('[SEC-H01] isSafeProjectRelativePath', () => { test('rejects traversal, drive, UNC, and NUL inputs; allows in-project absolute POSIX form', () => { expect(isSafeProjectRelativePath('../secret')).toBe(false) @@ -107,7 +120,7 @@ describe('resolveFilePathWithinProject — symlink containment', () => { beforeEach(() => { tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'path-utils-')) - outsideDir = fs.mkdtempSync(path.join(os.tmpdir(), 'outside-')) + outsideDir = makeOutsideRoot('outside-') // In-project symlink that escapes: tmpDir/evil -> outsideDir fs.symlinkSync(outsideDir, path.join(tmpDir, 'evil')) // Legit in-project symlink: tmpDir/link -> tmpDir/real @@ -121,11 +134,13 @@ describe('resolveFilePathWithinProject — symlink containment', () => { }) test('rejects a symlink that points outside the project', () => { + if (!outsideRootsUsable()) return expect(resolveFilePathWithinProject(tmpDir, 'evil')).toBeNull() expect(resolveFilePathWithinProject(tmpDir, 'evil/file.ts')).toBeNull() }) test('rejects an outside symlink even when the target file does not exist', () => { + if (!outsideRootsUsable()) return expect( resolveFilePathWithinProject(tmpDir, 'evil/nonexistent.ts'), ).toBeNull() @@ -192,6 +207,9 @@ test('filesystem operations resolve symlinks through the injected filesystem', a }) describe('read-only operation resolvers', () => { + // The externalRoot fixture is anchored outside the OS temp roots (see + // `makeOutsideRoot`), so this describe skips whole when the checkout cannot + // provide such a fixture rather than asserting a scope that cannot hold. let projectDir: string let externalRoot: string let externalFile: string @@ -204,7 +222,11 @@ describe('read-only operation resolvers', () => { beforeEach(() => { resetExternalReadRootsForTesting() projectDir = fs.mkdtempSync(path.join(os.tmpdir(), 'read-resolver-proj-')) - externalRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'read-resolver-ext-')) + // Outside both the project root and the OS temp roots: a temp-rooted + // externalRoot would resolve as `owned-temp` before the external-read + // branch is consulted, and the unconfigured refusal below would never + // fire. + externalRoot = makeOutsideRoot('read-resolver-ext-') externalFile = path.join(externalRoot, 'notes.txt') fs.writeFileSync(externalFile, 'notes\n') fs.writeFileSync( @@ -266,6 +288,7 @@ describe('read-only operation resolvers', () => { }) test('refuses the external file while the registry is unconfigured', () => { + if (!outsideRootsUsable()) return expect(resolveFilePathForReadOperation(projectDir, externalFile)).toBeNull() }) }) diff --git a/sdk/src/__tests__/read-files.test.ts b/sdk/src/__tests__/read-files.test.ts index 0755aeeb10..93a1dd83fe 100644 --- a/sdk/src/__tests__/read-files.test.ts +++ b/sdk/src/__tests__/read-files.test.ts @@ -399,18 +399,43 @@ describe('getFilesStructured', () => { cwd: '/project', fs: mockFs, // An allow-everything host filter proves the refusal comes from the - // mandatory blocklist, not from host policy. + // mandatory sensitive policy, not from host policy. fileFilter: () => ({ status: 'allow' }), }) + // The containment resolver's fail-closed mandatory-sensitive refusal + // fires FIRST: it returns no resolution at all, so the handler reports + // the outside-project code. (Pinned on that specific code: if the + // resolver refusal were removed, the read-only resolver would resolve + // these as owned-temp and the handler's alias blocklist would answer + // `blocked` instead — either way this assertion fails and the refusal + // regression is caught.) expect(result.results[0]).toMatchObject({ status: 'error', - error: { code: 'blocked' }, + error: { code: 'outside_project' }, }) expect(result.results[1]).toMatchObject({ status: 'error', - error: { code: 'blocked' }, + error: { code: 'outside_project' }, + }) + + // Attribution guard: a non-sensitive neighbour in the SAME temp + // directory still reads, so the refusals above are attributable to the + // sensitive policy rather than to temp paths being unreachable. + const neighbour = path.join( + ownedTempRoot, + 'openbuff-readfiles-secrets', + 'notes.txt', + ) + const allowed = await getFilesStructured({ + filePaths: [neighbour], + cwd: '/project', + fs: createMockFs({ + files: { [neighbour]: { content: 'scratch\n' } }, + }), + fileFilter: () => ({ status: 'allow' }), }) + expect(allowed.results[0]).toMatchObject({ status: 'ok' }) }) test('[COR-M11] returns typed binary and unsupported-encoding failures without capabilities', async () => { diff --git a/sdk/src/__tests__/read-image.test.ts b/sdk/src/__tests__/read-image.test.ts index 33fd3c5b96..12f6d6916a 100644 --- a/sdk/src/__tests__/read-image.test.ts +++ b/sdk/src/__tests__/read-image.test.ts @@ -3,18 +3,32 @@ import * as os from 'os' import * as path from 'path' import { FILE_READ_STATUS } from '@codebuff/common/old-constants' +import { + makeOutsideRoot, + outsideRootsUsable, + removeScratchParentIfEmpty, +} from '@codebuff/common/testing' import { createNodeError } from '@codebuff/common/testing/errors' import { configureExternalReadRoots, resetExternalReadRootsForTesting, } from '@codebuff/common/util/project-path-containment' -import { afterEach, beforeEach, describe, expect, test } from 'bun:test' +import { + afterAll, + afterEach, + beforeEach, + describe, + expect, + test, +} from 'bun:test' import { readImages } from '../tools/read-image' import type { CodebuffFileSystem } from '@codebuff/common/types/filesystem' import type { PathLike } from 'node:fs' +afterAll(removeScratchParentIfEmpty) + function createMockFs(files: Record): CodebuffFileSystem { return { readFile: async (filePath: PathLike) => { @@ -134,8 +148,12 @@ describe('readImages', () => { }) test('rejects in-project symlinks pointing outside the project root', async () => { + if (!outsideRootsUsable()) return const tmpRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'readimg-root-')) - const tmpOutside = fs.mkdtempSync(path.join(os.tmpdir(), 'readimg-out-')) + // The symlink TARGET must sit outside the OS temp roots too, or the + // widened temp exception would legitimately admit this read and the + // escape refusal would never fire. + const tmpOutside = makeOutsideRoot('readimg-out-') try { const projectRoot = fs.realpathSync(tmpRoot) const outsideDir = fs.realpathSync(tmpOutside) diff --git a/sdk/src/__tests__/read-logs.test.ts b/sdk/src/__tests__/read-logs.test.ts index cddbdf9de2..e9f6d12a7d 100644 --- a/sdk/src/__tests__/read-logs.test.ts +++ b/sdk/src/__tests__/read-logs.test.ts @@ -1,8 +1,14 @@ -import { afterEach, beforeEach, describe, expect, test } from 'bun:test' +import { afterAll, afterEach, beforeEach, describe, expect, test } from 'bun:test' import * as fs from 'fs' import * as os from 'os' import * as path from 'path' +import { + cleanupOutsideRoots, + makeOutsideRoot, + outsideRootsUsable, + removeScratchParentIfEmpty, +} from '@codebuff/common/testing' import { configureExternalReadRoots, isOwnedTempPath, @@ -24,6 +30,11 @@ import type { CodebuffFileSystem } from '@codebuff/common/types/filesystem' const tempDirs: string[] = [] const tempFiles: string[] = [] +/** `describe`, skipped whole when outside-root fixtures cannot be produced. */ +const describeWithOutsideFixtures = outsideRootsUsable() ? describe : describe.skip + +afterAll(removeScratchParentIfEmpty) + /** Trusted owner injected into readLogs by the run/session layer in tests. */ const TRUSTED_OWNER = { clientSessionId: 'session-1', @@ -81,6 +92,7 @@ afterEach(() => { for (const dir of tempDirs.splice(0)) { fs.rmSync(dir, { recursive: true, force: true }) } + cleanupOutsideRoots() for (const file of tempFiles.splice(0)) { fs.rmSync(file, { force: true }) } @@ -122,6 +134,9 @@ describe('readLogs', () => { const result = value( await readLogs({ cwd, + // A `..` segment survives into the raw input, and a traversal-free raw + // input is a hard requirement of the temp-root exception, so this stays + // refused even though the collapsed path lands under the temp root. path: path.relative(cwd, path.join(outside, 'secret.log')), owner: TRUSTED_OWNER, }), @@ -131,8 +146,11 @@ describe('readLogs', () => { }) test('rejects absolute paths outside cwd', async () => { + if (!outsideRootsUsable()) return const cwd = makeTempDir() - const outside = makeTempDir() + // An absolute escape target must sit outside the OS temp roots too, or + // the widened temp exception would legitimately admit this read. + const outside = makeOutsideRoot('read-logs-out-') const outsideFile = path.join(outside, 'secret.log') fs.writeFileSync(outsideFile, 'secret\n') @@ -144,8 +162,11 @@ describe('readLogs', () => { }) test('rejects symlinks that resolve outside cwd', async () => { + if (!outsideRootsUsable()) return const cwd = makeTempDir() - const outside = makeTempDir() + // The symlink target must sit outside the OS temp roots too, or the + // widened temp exception would legitimately admit this read. + const outside = makeOutsideRoot('read-logs-out-') const outsideFile = path.join(outside, 'secret.log') fs.writeFileSync(outsideFile, 'secret\n') fs.symlinkSync(outsideFile, path.join(cwd, 'link.log')) @@ -181,17 +202,57 @@ describe('readLogs', () => { expect(result.content).toBe('two\nthree\n') }) - test('rejects a non-owned temp path outside cwd', async () => { + test('reads a plain temp log outside cwd', async () => { + // Containment covers the WHOLE OS temp root, so a temp log whose name + // matches no openbuff namespace is reachable exactly like an owned one. + const cwd = makeTempDir() + const plainLog = tempFilePath('not-owned') + fs.writeFileSync(plainLog, 'one\ntwo\nthree\n') + + const result = value( + await readLogs({ + cwd, + path: plainLog, + lines: 2, + max_chars: 1_000, + owner: TRUSTED_OWNER, + }), + ) + + expect(result.errorMessage).toBeUndefined() + // Compared against the realpath, matching the owned-temp test above: on + // macOS `os.tmpdir()` is a symlinked `/var/folders/...` path. + expect(result.resolvedPath).toBe(fs.realpathSync(plainLog)) + expect(result.content).toBe('two\nthree\n') + }) + + test('still refuses a sensitive basename inside a temp directory', async () => { + // The widened scope must not expose a dropped-in credential file: the + // containment resolver applies the fail-closed mandatory-sensitive refusal, + // so the read fails with the same outside-project message as any + // unresolvable path. const cwd = makeTempDir() - const notOwned = tempFilePath('not-owned') - fs.writeFileSync(notOwned, 'secret\n') + const tempDir = makeTempDir() + const sensitive = path.join(tempDir, '.env') + fs.writeFileSync(sensitive, 'API_KEY=secret\n') const result = value( - await readLogs({ cwd, path: notOwned, owner: TRUSTED_OWNER }), + await readLogs({ cwd, path: sensitive, owner: TRUSTED_OWNER }), ) expect(result.errorMessage).toContain('outside the project directory') expect(result.content).toBeUndefined() + + // A non-sensitive neighbour in the same temp directory still reads, so the + // refusal above is attributable to the sensitive policy rather than to temp + // paths being unreachable. + const neighbour = path.join(tempDir, 'app.log') + fs.writeFileSync(neighbour, 'one\ntwo\n') + const allowed = value( + await readLogs({ cwd, path: neighbour, owner: TRUSTED_OWNER }), + ) + expect(allowed.errorMessage).toBeUndefined() + expect(allowed.content).toBe('one\ntwo\n') }) test('reads a background job log by jobId', async () => { @@ -458,7 +519,11 @@ describe('readLogs', () => { }) }) -describe('readLogs — allowlisted external read roots', () => { +// Every `externalRoot` in this describe must sit outside BOTH the project root +// and the OS temp roots: a temp-rooted root would resolve as `owned-temp` +// before the external-read branch is ever consulted, so neither the configured +// allow nor the unconfigured refusal would prove anything about the registry. +describeWithOutsideFixtures('readLogs — allowlisted external read roots', () => { beforeEach(() => { // The registry is configure-once per PROCESS, and `run.ts` legitimately // configures it (with the openbuff config dir) as soon as any suite in this @@ -476,7 +541,7 @@ describe('readLogs — allowlisted external read roots', () => { test('reads a log inside an allowlisted external root', async () => { const cwd = makeTempDir() - const externalRoot = makeTempDir() + const externalRoot = makeOutsideRoot('read-logs-ext-') const externalLog = path.join(externalRoot, 'external.log') fs.writeFileSync(externalLog, 'one\ntwo\nthree\n') @@ -501,7 +566,7 @@ describe('readLogs — allowlisted external read roots', () => { test('refuses the same log while the registry is unconfigured', async () => { const cwd = makeTempDir() - const externalRoot = makeTempDir() + const externalRoot = makeOutsideRoot('read-logs-ext-') const externalLog = path.join(externalRoot, 'external.log') fs.writeFileSync(externalLog, 'one\ntwo\nthree\n') @@ -515,7 +580,7 @@ describe('readLogs — allowlisted external read roots', () => { test('a host fileFilter blocks a log inside an allowlisted external root', async () => { const cwd = makeTempDir() - const externalRoot = makeTempDir() + const externalRoot = makeOutsideRoot('read-logs-ext-') const externalLog = path.join(externalRoot, 'external.log') fs.writeFileSync(externalLog, 'one\ntwo\nthree\n') diff --git a/sdk/src/__tests__/terminal-command-policy.test.ts b/sdk/src/__tests__/terminal-command-policy.test.ts index 8696015df4..60daaede64 100644 --- a/sdk/src/__tests__/terminal-command-policy.test.ts +++ b/sdk/src/__tests__/terminal-command-policy.test.ts @@ -274,6 +274,156 @@ describe('terminal command permission policy', () => { } }) + it('ignores a quoted bare slash but keeps real absolute operands denied', () => { + for (const command of [ + // A `/` inside a quoted sed/awk expression is an expression delimiter, + // not a path operand: `s/^/RESULT commit: /` ends in `: /`, so the token + // scan captures a space-preceded `/` that resolves to the root. + "git log --oneline -3 HEAD | sed 's/^/RESULT commit: /'", + "git status --porcelain | sed 's/^/CLI /'", + 'git status --porcelain | sed "s/^/X /"', + 'git status --porcelain | sed "s|^|X |"', + "awk -F'/' '{print $2}' notes.txt", + // A quote of the other kind inside an open region is literal text: the + // `"hi"` must not close the single-quoted region early, which would push + // the trailing `/` outside every range and re-deny the command. + 'sed \'s/^/say "hi": /\' notes.txt', + // Mirror case: a `'` inside a double-quoted region is literal too, so the + // closing `"` still ends the region at the right offset. + 'sed "s/^/it\'s /" notes.txt', + // The resolved /tmp exemption is untouched by the quoted-slash skip. + "stat -c '%a %U' /tmp", + ]) { + expect( + evaluateTerminalCommandPolicy({ + command, + mode: 'assistant', + permissionProfile: 'workspace-write', + projectRoot, + }), + ).toEqual({ allowed: true }) + } + // The same helper backs the read-only family, so the pipeline that first + // hit this false positive is allowed there too. + expect( + evaluateTerminalCommandPolicy({ + command: "git log --oneline -3 HEAD | sed 's/^/RESULT commit: /'", + mode: 'assistant', + permissionProfile: 'read-only', + projectRoot, + }), + ).toEqual({ allowed: true }) + for (const [command, token] of [ + // An UNQUOTED bare `/` is still an absolute operand outside the project. + ['ls /', '/'], + ['du -sh /', '/'], + // An unterminated quote reports no region, so the operand stays checked. + ["ls '/", '/'], + // The quoted expression ends where it should: a bare `/` operand AFTER a + // quoted region is still outside the project. + ["sed 's/x/y/' /", '/'], + // A quoted token whose content IS an absolute path stays refused; that is + // why the skip is narrowed to the root-only token. + ["cat '/etc/passwd'", '/etc/passwd'], + ['cat "/etc/passwd"', '/etc/passwd'], + // An absolute path embedded in a quoted multi-word string, reached + // through this helper rather than the shell-indirection guard. + ["xargs -I{} echo '/etc/passwd here'", '/etc/passwd'], + ['rg --file=/etc/passwd TODO src', '/etc/passwd'], + ] as const) { + expect( + evaluateTerminalCommandPolicy({ + command, + mode: 'assistant', + permissionProfile: 'workspace-write', + projectRoot, + }), + ).toEqual({ + allowed: false, + reason: `absolute path is outside the project: ${token}`, + }) + } + for (const command of [ + // Refused earlier by the shell-indirection guard; the absolute path + // embedded in the quoted script must never become allowed either. + "bash -c 'cat /etc/passwd'", + 'cat /etc/passwd', + 'cat ~/secrets', + 'cat $HOME/secrets', + 'cat ${HOME}/x', + ]) { + expect( + evaluateTerminalCommandPolicy({ + command, + mode: 'assistant', + permissionProfile: 'workspace-write', + projectRoot, + }).allowed, + ).toBe(false) + } + // dependency-mutation shares the same helper, so an unquoted bare `/` + // operand stays refused at that call site too. + expect( + evaluateTerminalCommandPolicy({ + command: 'npm install --prefix /', + mode: 'assistant', + permissionProfile: 'dependency-mutation', + projectRoot, + }), + ).toEqual({ + allowed: false, + reason: 'absolute path is outside the project: /', + }) + }) + + it('refuses a quoted root-only operand of a filesystem-mutation command', () => { + // The quoted-region skip waives a root-only token (`/`) inside quotes, + // and the root-deletion deny pattern misses `rm -rf '/'` because the + // quote breaks its `\s+\/` anchor — so a mutating command carrying a + // quoted root OPERAND must be re-refused with the standard + // outside-project denial. + for (const command of ["rm -rf '/'", "cp x '/'"]) { + expect( + evaluateTerminalCommandPolicy({ + command, + mode: 'assistant', + permissionProfile: 'workspace-write', + projectRoot, + }), + ).toEqual({ + allowed: false, + reason: 'absolute path is outside the project: /', + }) + } + // workspace-write refuses `sudo ...` earlier via its privilege-escalation + // deny pattern, so the elevated shape is pinned through tmux-test — the + // one profile that reaches the outside-path helper without the deny + // patterns and without refusing `rm` outright. + expect( + evaluateTerminalCommandPolicy({ + command: "sudo rm -rf '/'", + mode: 'assistant', + permissionProfile: 'tmux-test', + projectRoot, + }), + ).toEqual({ + allowed: false, + reason: 'absolute path is outside the project: /', + }) + // A quoted root-only token with NO mutation executable stays allowed: + // the sed delimiter shape and an echo payload are inert data. + for (const command of ["git log | sed 's/^/R: /'", 'echo "path:/']) { + expect( + evaluateTerminalCommandPolicy({ + command, + mode: 'assistant', + permissionProfile: 'workspace-write', + projectRoot, + }), + ).toEqual({ allowed: true }) + } + }) + it('denies process environment dumps under tmux-test without re-enabling workspace deny patterns', () => { for (const command of [ 'printenv', diff --git a/sdk/src/tools/__tests__/list-directory.test.ts b/sdk/src/tools/__tests__/list-directory.test.ts index 43f181b3e1..046e56c496 100644 --- a/sdk/src/tools/__tests__/list-directory.test.ts +++ b/sdk/src/tools/__tests__/list-directory.test.ts @@ -5,8 +5,9 @@ import os from 'os' import path from 'path' import { - OWNED_TEMP_SEGMENT_PATTERNS, configureExternalReadRoots, + getOwnedTempRoots, + isOwnedTempPath, resetExternalReadRootsForTesting, } from '@codebuff/common/util/project-path-containment' @@ -284,11 +285,6 @@ function expectContainmentRejection( ) } -/** True when `segment` is an owned-temp top-level segment per the shared list. */ -function isOwnedTempSegment(segment: string): boolean { - return OWNED_TEMP_SEGMENT_PATTERNS.some((pattern) => pattern.test(segment)) -} - describe('listDirectory containment', () => { test('rejects sibling /project-evil when project is /project', async () => { const result = await listDirectory({ @@ -376,10 +372,14 @@ describe('listDirectory containment', () => { // pins the entry-filter behaviour for that shape, including the mandatory // sensitive-path block ('.env' must not survive the exact arrays below). const ownedTempSegment = 'openbuff-xyz' - // Precondition: the segment really is owned-temp per the shared pattern - // list. Without this the case would silently degrade into a plain - // containment reject if the owned-temp prefix ever changed. - expect(isOwnedTempSegment(ownedTempSegment)).toBe(true) + // Precondition keyed off the ACTUAL gate: any path strictly inside the + // temp root resolves as owned-temp, so this must come back true. Without + // it the case would silently degrade into a plain containment reject if + // temp-root containment ever changed. (The old per-segment pattern list + // is documentation only and gates nothing.) + expect( + isOwnedTempPath(path.join(getOwnedTempRoots()[0], ownedTempSegment)), + ).toBe(true) const ownedTempDir = path.join(os.tmpdir(), ownedTempSegment) const fs = makeFs({ @@ -400,16 +400,47 @@ describe('listDirectory containment', () => { expect(value.path).toBe(ownedTempDir) }) - test('rejects a non-owned temp sibling outside the project', async () => { - // Negative counterpart to the owned-temp case: an equally out-of-project - // temp sibling whose segment matches no owned-temp pattern must still be - // rejected, so the allow above is attributable to owned-temp scope rather - // than to temp paths being generally reachable. + test('lists a non-openbuff temp directory outside the project', async () => { + // Positive counterpart to the owned-temp case: with containment widened to + // the WHOLE temp root, a temp directory whose segment matches no + // openbuff-owned pattern is reachable exactly like an owned one, and the + // listing runs with the same absolute-relativePath entry-filter shape. const foreignTempSegment = 'not-owned-xyz' - expect(isOwnedTempSegment(foreignTempSegment)).toBe(false) + // Precondition keyed off the ACTUAL gate: the widened whole-root scope + // admits this path exactly like an openbuff-named one, so the allow below + // is attributable to that scope rather than to any name pattern. (The old + // per-segment pattern list is documentation only and gates nothing.) + expect( + isOwnedTempPath(path.join(getOwnedTempRoots()[0], foreignTempSegment)), + ).toBe(true) + + const foreignTempDir = path.join(os.tmpdir(), foreignTempSegment) + const fs = makeFs({ + readdir: makeReaddir([ + dirent('job.log'), + dirent('.env'), + dirent('nested', 'dir'), + ]), + }) + const result = await listDirectory({ + directoryPath: foreignTempDir, + projectPath: '/virtual/repo', + fs, + }) + const value = expectListing(result) + expect(value.files).toEqual(['job.log']) + // The mandatory sensitive-path block still applies to temp entries. + expect(value.directories).toEqual(['nested']) + // `value.path` is the absolute temp directory, mirroring the owned-temp + // listing above. + expect(value.path).toBe(foreignTempDir) + }) + test('still rejects the temp root itself (strictly-inside rule)', async () => { + // Negative counterpart: the root itself is never inside ANY scope, so the + // allow above stays attributable to strictly-inside containment. const result = await listDirectory({ - directoryPath: path.join(os.tmpdir(), foreignTempSegment), + directoryPath: os.tmpdir(), projectPath: '/virtual/repo', fs: rejectingFs(), }) @@ -802,8 +833,8 @@ describe('listDirectory listing behaviour', () => { describe('listDirectory allowlisted external read roots', () => { // Synthetic absolute root: every filesystem call goes through the stub - // filesystem, and the name deliberately avoids the `openbuff-` owned-temp - // patterns so an allow here can only come from the external read allowlist. + // filesystem, and the root deliberately sits outside every temp root so an + // allow here can only come from the external read allowlist. const externalRoot = path.resolve('/external-read-root') // Strictly inside the root: the root itself is deliberately not readable. const externalDir = path.join(externalRoot, 'logs') diff --git a/sdk/src/tools/filesystem-authority.ts b/sdk/src/tools/filesystem-authority.ts index 8c02d1cacd..259f99c16b 100644 --- a/sdk/src/tools/filesystem-authority.ts +++ b/sdk/src/tools/filesystem-authority.ts @@ -41,12 +41,47 @@ const BACKGROUND_JOB_FILE_PATTERN = /^openbuff-(.+)\.(?:log|json)$/ */ const TMUX_CAPTURE_DIR_PATTERN = /^tmux-captures-.+$/ +/** + * Win32 aliasing guard for the owned-temp refusals below: the OS strips + * trailing dots/spaces from EVERY path segment, so the lexical alias + * `/payload.sh ` creates the real `payload.sh` while + * `path.extname('payload.sh ')` is `.sh ` and misses + * `OWNED_TEMP_REFUSED_EXTENSIONS` — and an aliased + * `/openbuff-job-1.log ` basename or `/tmux-captures-x /` segment + * would dodge `BACKGROUND_JOB_FILE_PATTERN` / `TMUX_CAPTURE_DIR_PATTERN` the + * same way. Splits on BOTH separators (win32 `path.sep` is a backslash), + * strips trailing dots/spaces from every segment, and joins with '/'. Mirrors + * `refusesWin32AliasedSensitivePath` in + * `common/src/util/project-path-containment.ts` so the two cannot drift in + * approach; a no-op when no segment carries a trailing dot/space (the common + * case, and the universal case on POSIX). + */ +function win32NormalizeSegments(value: string): string { + return value + .split(/[\\/]+/) + .map((segment) => segment.replace(/[ .]+$/, '')) + .join('/') +} + /** * Owned temp space is exempt from the terminal command policy's outside-path * check (every `/tmp/...` token is allowed there), so a tool-side write of an * executable-extension basename would turn a plain file write into arbitrary * command execution. Refuse those basenames instead of relying on the terminal * policy to catch them later. + * + * This is now the ONLY defense against a file-changing tool staging a script + * anywhere under an OS temp root for a later command to execute. Containment + * in `common/src/util/project-path-containment.ts` admits the whole temp root + * and no longer excludes any name — including the chmod +x'd tmux helper + * script `tmux-helper-.sh`, which run_terminal_command executes. + * Interpreter scripts are covered for the same reason: read-only terminal + * profiles refuse interpreter one-liners (`node -e`, `python3 -c`) but permit + * `node /tmp/x.js` / `python3 /tmp/x.py`, and the same `/tmp/...` token + * exemption carries that command — so a staged `.js`/`.py` file would be + * executed one step later. Removing or narrowing this set therefore re-opens + * a terminal-policy bypass; do not weaken it, and keep the refusal applied to + * create/overwrite/move. */ const OWNED_TEMP_REFUSED_EXTENSIONS = new Set([ '.sh', @@ -57,8 +92,40 @@ const OWNED_TEMP_REFUSED_EXTENSIONS = new Set([ '.bat', '.cmd', '.ps1', + // Interpreter-executed extensions (`node `, `python3 `, ...). + '.js', + '.mjs', + '.cjs', + '.jsx', + '.ts', + '.tsx', + '.mts', + '.cts', + '.py', + '.pyw', + '.pl', + '.rb', + '.lua', + '.php', + '.r', + '.jl', + '.tcl', ]) +/** + * Single append+trim path for a bounded receipt log, shared by + * `recordCanonicalReceipt` and `retainReceipt`. The splice form drops however + * many entries are over the cap (a lone `shift()` drops exactly one and can + * leave the array over cap), so the log stays bounded by whatever cap the + * caller passes. + */ +function appendBounded(list: T[], item: T, cap: number): void { + list.push(item) + if (list.length > cap) { + list.splice(0, list.length - cap) + } +} + export type FilesystemCapability = | 'baseline' | 'range_read' @@ -237,14 +304,19 @@ export class FilesystemAuthority { ) if (!resolved) return { allowed: false, code: 'path_outside_project' } - // The owned-temp namespace is now mutable so tools can manage their own - // scratch artifacts (mkdtemp dirs and their contents): create, overwrite, + // The owned-temp scope covers ANY path strictly inside an OS temp root, and + // it is mutable so tools can manage their own scratch artifacts (mkdtemp + // dirs and their contents, plus ordinary temp files): create, overwrite, // delete and move are permitted there. The exceptions refused below are // live background-job log/metadata files, tmux capture evidence, and // executable-extension basenames. - // /tmp is world-writable, so owned-temp mutations rely on (i) the anchored - // full-segment owned patterns, (ii) single-realpath containment validated - // in `common/src/util/project-path-containment.ts`, and (iii) the same + // /tmp is world-writable, so owned-temp mutations rely on (i) STRICTLY + // INSIDE temp-root containment with a SINGLE realpath dereference, both + // validated in `common/src/util/project-path-containment.ts` (there is no + // longer any owned-name pattern doing part of this work), (ii) the + // fail-closed mandatory-sensitive refusal in that same resolver, which is + // what keeps `/.env` and `/credentials.json` unwritable, (iii) + // the refusals in `ownedTempMutationRefusal` below, and (iv) the same // conditional-commit / expected-state revalidation every project write // goes through. const ownedTempRefusal = this.ownedTempMutationRefusal(resolved, operation) @@ -659,13 +731,20 @@ export class FilesystemAuthority { * capture would let a subagent forge its own evidence. They get the same * read-only treatment as job artifacts. * - * Every other owned-temp path (openbuff mkdtemp directories and their nested - * contents) permits create, overwrite, delete and move, except that an - * executable-extension basename cannot be created, overwritten or moved. - * Deleting such a file remains allowed so tools can clean up. + * Every other owned-temp path (openbuff mkdtemp directories, ordinary temp + * files, and any nested contents) permits create, overwrite, delete and move, + * except that an executable-extension basename cannot be created, + * overwritten or moved — including `tmux-helper-.sh`, which + * containment no longer excludes. Deleting such a file remains allowed so + * tools can clean up. * - * The basename/segment checks run on the RESOLVED path so alias forms cannot - * dodge them. + * The basename/segment checks run on the RESOLVED path AND its + * Win32-normalized form (`win32NormalizeSegments` strips trailing + * dots/spaces from EVERY segment, as the OS does on win32), so alias forms + * cannot dodge them: a lexical `payload.sh ` creates the real `payload.sh` + * while `path.extname('payload.sh ')` is `.sh `, and the same alias would + * slip `openbuff-job-1.log ` and `tmux-captures-x ` past the read-only + * artifact patterns. */ private ownedTempMutationRefusal( resolved: ResolvedOperationPath, @@ -673,20 +752,34 @@ export class FilesystemAuthority { ): { allowed: false; code: string } | undefined { if (resolved.scope !== 'owned-temp' || operation === 'read') return undefined - const basename = path.basename(resolved.operationPath) - if (BACKGROUND_JOB_FILE_PATTERN.test(basename)) { + const operationPath = resolved.operationPath + const basename = path.basename(operationPath) + const normalizedPath = win32NormalizeSegments(operationPath) + const normalizedBasename = path.basename(normalizedPath) + if ( + BACKGROUND_JOB_FILE_PATTERN.test(basename) || + BACKGROUND_JOB_FILE_PATTERN.test(normalizedBasename) + ) { return { allowed: false, code: 'owned_temp_job_artifact_read_only' } } if ( - resolved.operationPath + operationPath .split(path.sep) + .some((segment) => TMUX_CAPTURE_DIR_PATTERN.test(segment)) || + normalizedPath + .split('/') .some((segment) => TMUX_CAPTURE_DIR_PATTERN.test(segment)) ) { return { allowed: false, code: 'owned_temp_capture_read_only' } } if ( operation !== 'delete' && - OWNED_TEMP_REFUSED_EXTENSIONS.has(path.extname(basename).toLowerCase()) + (OWNED_TEMP_REFUSED_EXTENSIONS.has( + path.extname(basename).toLowerCase(), + ) || + OWNED_TEMP_REFUSED_EXTENSIONS.has( + path.extname(normalizedBasename).toLowerCase(), + )) ) { return { allowed: false, @@ -702,10 +795,9 @@ export class FilesystemAuthority { phase: FilesystemPolicyPhase, ): Promise { // Fail closed on the read-only `external-read` scope: this authority only - // covers the project tree and the openbuff-owned temp namespace, which are - // the only scopes the operation resolvers - // (`resolveFilePathFor*Operation`) can produce. The check keeps - // `AuthorizedFilesystemPath.scope` narrow instead of widening a + // covers the project tree and the OS-temp scope, which are the only scopes + // the operation resolvers (`resolveFilePathFor*Operation`) can produce. The + // check keeps `AuthorizedFilesystemPath.scope` narrow instead of widening a // mutation-side type with a read-only scope. if (resolved.scope === 'external-read') { return { allowed: false, code: 'external_read_scope_unsupported' } @@ -773,23 +865,11 @@ export class FilesystemAuthority { * the log stays bounded by MAX_COMMIT_RECEIPTS_PER_RUN whatever the caller. */ private recordCanonicalReceipt(receipt: CommitReceiptV1): void { - this.canonicalReceipts.push(receipt) - if (this.canonicalReceipts.length > MAX_COMMIT_RECEIPTS_PER_RUN) { - this.canonicalReceipts.splice( - 0, - this.canonicalReceipts.length - MAX_COMMIT_RECEIPTS_PER_RUN, - ) - } + appendBounded(this.canonicalReceipts, receipt, MAX_COMMIT_RECEIPTS_PER_RUN) } private retainReceipt(receipt: CommitReceipt): void { - this.receipts.push(receipt) - if (this.receipts.length > MAX_COMMIT_RECEIPTS_PER_RUN) { - this.receipts.splice( - 0, - this.receipts.length - MAX_COMMIT_RECEIPTS_PER_RUN, - ) - } + appendBounded(this.receipts, receipt, MAX_COMMIT_RECEIPTS_PER_RUN) } private pruneTerminalOperations(): void { diff --git a/sdk/src/tools/path-utils.ts b/sdk/src/tools/path-utils.ts index 688a4c60fb..bdd73595dd 100644 --- a/sdk/src/tools/path-utils.ts +++ b/sdk/src/tools/path-utils.ts @@ -1,9 +1,8 @@ import path from 'path' import { - getOwnedTempRoots, isOwnedTempPath, - OWNED_TEMP_SEGMENT_PATTERNS, + isOwnedTempPathForFileSystem, resolveProjectPath, resolveProjectPathForFileSystem, resolveProjectPathForFileSystemRead, @@ -81,13 +80,13 @@ export function getScopedReadPolicyAliases( /** * Shared owned-temp fallback for unlink-style operations (followFinalSymlink: false). * - * A top-level owned-temp entry (e.g. an `openbuff-` scratch directory - * directly under the OS temp root) has the bare temp root as its parent, and - * the temp root is deliberately never itself owned-temp (strictly-inside rule), - * so the parent lookup legitimately fails there. The parent lookup also fails - * when the parent is only lexically owned but its realpath escapes the owned - * roots — in that case the synthesized candidate would land outside the owned - * namespace and must be refused. + * A top-level owned-temp entry (e.g. a scratch directory directly under the OS + * temp root) has the bare temp root as its parent, and the temp root is + * deliberately never itself owned-temp (strictly-inside rule), so the parent + * lookup legitimately fails there. The parent lookup also fails when the parent + * is only lexically inside a temp root but its realpath escapes every temp root + * — in that case the synthesized candidate would land outside containment and + * must be refused. * * This helper centralizes the candidate synthesis + isOwnedTempPath re-validation * so sync and async resolveFilePathFor*Operation cannot drift. @@ -110,88 +109,18 @@ function getUnlinkOperationPath( return path.join(parent.realFullPath, path.basename(resolved.fullPath)) } -// --- FS-aware owned-temp helpers for virtual adapters (RF-2) --- -// Re-uses the canonical pattern list from common so the two cannot drift. -export const OWNED_TEMP_SEGMENT_PATTERNS_FS_AWARE: RegExp[] = - OWNED_TEMP_SEGMENT_PATTERNS - -function escapesRootFsAware(root: string, target: string): boolean { - const relative = path.relative(root, target) - return ( - relative === '..' || - relative.startsWith('..' + path.sep) || - path.isAbsolute(relative) || - relative.split(path.sep).includes('..') - ) -} - -function isInsideOwnedTempNamespaceFsAware( - target: string, - roots: string[], -): boolean { - return roots.some((root) => { - const relative = path.relative(root, target) - if (relative === '' || escapesRootFsAware(root, target)) return false - const firstSegment = relative.split(path.sep)[0] - return OWNED_TEMP_SEGMENT_PATTERNS_FS_AWARE.some((pattern) => - pattern.test(firstSegment), - ) - }) -} - -async function realpathOrLexicalForFileSystemFsAware( - fsPath: string, - fileSystem: CodebuffFileSystem, -): Promise { - try { - return String(await fileSystem.realpath(fsPath)) - } catch { - const tail: string[] = [] - let current = fsPath - while (true) { - try { - const realAncestor = String(await fileSystem.realpath(current)) - return tail.length === 0 - ? realAncestor - : path.join(realAncestor, ...tail.reverse()) - } catch { - if (current === path.dirname(current)) return fsPath - tail.push(path.basename(current)) - current = path.dirname(current) - } - } - } -} - -async function getOwnedTempComparisonRootsForFileSystemFsAware( - fileSystem: CodebuffFileSystem, -): Promise { - const roots = getOwnedTempRoots() - const realRoots = await Promise.all( - roots.map((root) => - realpathOrLexicalForFileSystemFsAware(root, fileSystem), - ), - ) - return [...new Set([...roots, ...realRoots])] -} - -async function isOwnedTempPathForFileSystem( - input: string, - fileSystem: CodebuffFileSystem, -): Promise { - if (!input || input.split(/[\\/]+/).includes('..')) return false - const fullPath = path.resolve(input) - const roots = - await getOwnedTempComparisonRootsForFileSystemFsAware(fileSystem) - if (!isInsideOwnedTempNamespaceFsAware(fullPath, roots)) return false - const realFullPath = await realpathOrLexicalForFileSystemFsAware( - fullPath, - fileSystem, - ) - if (!isInsideOwnedTempNamespaceFsAware(realFullPath, roots)) return false - return true -} - +/** + * Async twin of `getUnlinkOperationPath`, kept structurally identical so the + * pair cannot drift. + * + * WHY the FS-aware predicate: the synthesized top-level candidate must be + * re-validated through the INJECTED filesystem rather than the host sync + * predicate, so a virtual adapter cannot be spoofed by a host-named path and a + * virtual temp root is honoured. `isOwnedTempPathForFileSystem` in common does + * exactly that (adapter `realpath` plus fs-aware comparison roots), so reusing + * it preserves the RF-2 property while removing a private duplicate that would + * now have to be widened in lockstep with the shared containment rule. + */ async function getUnlinkOperationPathForFileSystem( resolved: ContainedProjectPath, parent: ContainedProjectPath | null, @@ -289,7 +218,7 @@ export async function resolveFilePathForFileSystemOperation( * READ-ONLY twin of `resolveFilePathForOperation`. * * Delegates to `resolveProjectPathForRead`, so in addition to project and - * owned-temp paths it also resolves a path strictly inside an explicitly + * temp-root paths it also resolves a path strictly inside an explicitly * allowlisted external read root (`scope: 'external-read'`, with an ABSOLUTE * `relativePath` — consumers must branch on `scope`). * diff --git a/sdk/src/tools/terminal-command-policy.ts b/sdk/src/tools/terminal-command-policy.ts index d0c36fe843..a2b2da3b21 100644 --- a/sdk/src/tools/terminal-command-policy.ts +++ b/sdk/src/tools/terminal-command-policy.ts @@ -1822,6 +1822,67 @@ function hasUnsafeTmuxGitCommand(command: string): boolean { ) } +/** + * Character ranges — start inclusive, end exclusive — covering the CONTENT of + * each single- or double-quoted region of `command`. A quote of the other kind + * inside an open region is literal text (`"` inside `'…'`, `'` inside `"…"`), + * so the scanner tracks which quote opened the region; outside single quotes a + * backslash escapes the next character, matching the other quote scanners in + * this module. + * + * Deliberately not a full shell lexer: an unterminated quote contributes NO + * range, so `ls '/` keeps counting as a real `/` operand. Anything the scanner + * cannot settle must fail toward not-quoted, because its only caller uses a + * quoted region to WAIVE a containment check. + */ +function quotedContentRanges(command: string): Array<[number, number]> { + const ranges: Array<[number, number]> = [] + let quote: "'" | '"' | null = null + let contentStart = 0 + let escaped = false + for (let index = 0; index < command.length; index += 1) { + const char = command[index] + if (escaped) { + escaped = false + continue + } + if (char === '\\' && quote !== "'") { + escaped = true + continue + } + if (quote) { + if (char === quote) { + ranges.push([contentStart, index]) + quote = null + } + continue + } + if (char === "'" || char === '"') { + quote = char + contentStart = index + 1 + } + } + return ranges +} + +/** True when `index` lands inside one of the quoted content ranges. */ +function isInsideQuotedRegion( + ranges: Array<[number, number]>, + index: number, +): boolean { + return ranges.some(([start, end]) => index >= start && index < end) +} + +/** + * Filesystem-mutation executables whose quoted root-only operand is a real + * target, optionally elevated through sudo/doas. Anchored at the command + * start so a mutator named elsewhere (a file argument, a path segment) + * cannot turn a quoted delimiter in an unrelated command into a refusal: + * `awk -F'/' … cp.log` stays allowed while `rm -rf '/'` does not. + */ +const MUTATION_EXECUTABLE_COMMAND_PATTERN = + /^\s*(?:(?:sudo|doas)\s+)?(?:rm|mv|cp|chmod|chown|chgrp|dd|shred|truncate|ln|install)\b/i + function findOutsideAbsolutePath( command: string, projectRoot: string, @@ -1839,13 +1900,51 @@ function findOutsideAbsolutePath( ) }) if (outsideShellToken) return outsideShellToken - const tokens = [ - ...command.matchAll( - /(?:^|[\s"'=(])((?:[A-Za-z]:\\|\/(?!\/))[^\s"'|;&)]*)/g, - ), - ].map((match) => match[1]) - for (const rawToken of tokens) { - const token = rawToken.replace(/[),.:]+$/, '') + // One linear quote scan per command (never per token) feeds the root-only + // check below. + const quotedRanges = quotedContentRanges(command) + for (const match of command.matchAll( + /(?:^|[\s"'=(])((?:[A-Za-z]:\\|\/(?!\/))[^\s"'|;&)]*)/g, + )) { + const token = match[1].replace(/[),.:]+$/, '') + // The pattern consumes at most one leading delimiter character before the + // capture (start-of-string consumes none), so the token begins at the tail + // of the whole match. matchAll always reports an index; NaN would keep the + // token out of every range, i.e. treated as an unquoted operand. + const tokenIndex = + (match.index ?? Number.NaN) + (match[0].length - match[1].length) + // A bare `/` inside quotes is a sed/awk expression delimiter, not a path + // operand: `sed 's/^/RESULT commit: /'` ends in `: /`, so the scan captures + // a space-preceded `/` that resolves to the filesystem root. Only that + // exact shape is ignored. Skipping quoted words wholesale would admit an + // absolute path hidden in a quoted string (`cat '/etc/passwd'`, + // `bash -c 'cat /etc/passwd'`), and an UNQUOTED bare `/` operand (`ls /`, + // `du -sh /`) must stay refused too. path.parse keeps the comparison + // platform-correct (`/` on POSIX, `C:\` on win32). A deliberately quoted + // bare root (`ls '/'`) is waived by the same shape for non-mutating + // commands: that is the accepted cost of ignoring only the root-only + // token. + if ( + token === path.parse(token).root && + isInsideQuotedRegion(quotedRanges, tokenIndex) + ) { + // The quoted-root skip exists for `sed 's/^/X /'`-style expression + // delimiters. A quoted root as an OPERAND of a filesystem-mutating + // command is the dangerous shape the skip would otherwise admit: + // `rm -rf '/'` was refused before the skip (the outside-path check + // caught the bare root), and the WORKSPACE_DENY_PATTERNS root-deletion + // regex misses it because the quote breaks its `\s+\/` anchor. Return + // the root token so the caller emits the standard outside-project + // denial. Safe at every profile that uses this helper: read-only + // profiles already refuse these executables via their mutation checks, + // so this can only turn a would-be allow into a deny, and a quoted + // word that is not ENTIRELY the root token (sed's `s/^/R: /` body) + // never reaches this branch. + if (MUTATION_EXECUTABLE_COMMAND_PATTERN.test(command)) { + return token + } + continue + } if (token.startsWith('/dev/null')) continue if (token.startsWith('/bin/') || token.startsWith('/usr/bin/')) continue const resolved = path.resolve(token)