diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4e79d60..d7dacd3 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -81,6 +81,10 @@ jobs: env: DC_SANDBOX_NET_TEST: '1' DC_REQUIRE_RIPGREP: '1' + # Linux is the only runner with bubblewrap, and the bwrap integration + # tests are the only ones that check what the sandbox does rather than + # what arguments it builds. Make a missing binary fail there. + DC_REQUIRE_BWRAP: ${{ runner.os == 'Linux' && '1' || '0' }} run: pnpm test - name: Build + app-server release gate diff --git a/CHANGELOG.md b/CHANGELOG.md index 2ff7e2a..9209054 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -48,6 +48,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ask, and a hit is not yet a read. - The plugin capability bridge passed no contract into the tools it executed, so a plugin's `Grep` skipped the same filter. +- **`--sandbox read-only` was not read-only on Linux.** `buildLinuxBwrapArgs` + ended with an unconditional `--bind `, and bwrap applies binds in + order with the last one winning — so the read-only bind that the mode had + correctly asked for was overwritten a few arguments later, and a command could + write to the workspace. macOS never had this: its profile grants writes only + from `allowWrite`, which read-only leaves empty. #226 introduced the mode axis + and verified it on macOS; this is the half nobody looked at. Callers using the + legacy `enabled: true` shape are unaffected. ### 🐛 Fixed diff --git a/docs/THREE_WAY_REVIEW.md b/docs/THREE_WAY_REVIEW.md index 985033c..980e308 100644 --- a/docs/THREE_WAY_REVIEW.md +++ b/docs/THREE_WAY_REVIEW.md @@ -293,7 +293,13 @@ AskUserQuestion、模式/模型/effort 下拉、Inspector、FilePanel(Source/D 协议了,但 `Sidebar.tsx` 绕过 shim 直接 `listSessions()`——所以"第二个读取者"其实一直是三处 而不是两处。delete 协议侧原本没有方法,新增 `thread/delete`:app-server 是 thread 存储的 单一 owner,renderer 越过它删文件可能把正在写的 writer 的地板抽掉。 -- **Linux (bwrap) 侧只共享了 sandbox 模式解析,没有在 Linux 主机上做过 #226 那样的实测。** +- ~~**Linux (bwrap) 侧只共享了 sandbox 模式解析,没有在 Linux 主机上做过 #226 那样的实测。**~~ + 已补:bwrap 集成测试新增 mode 轴(read-only 可读不可写 / workspace-write 可读可写 / + danger-full-access 不包 bwrap),Linux CI 用 `DC_REQUIRE_BWRAP=1` 禁止静默跳过。 + 实测立刻抓到一个真 bug:`buildLinuxBwrapArgs` 末尾无条件 `--bind cwd cwd`, + bwrap 后绑定覆盖先绑定,所以 **`--sandbox read-only` 在 Linux 上根本不只读**。 + macOS 侧没有这个问题(写权限只来自 `allowWrite`)—— 正是"只在一个平台上实测过" + 的代价。 - 图片输入仍是空壳(有意保留:DeepSeek 无 vision 模型)。 ## 附:Codex 侧信息的可信度声明 diff --git a/packages/core/src/sandbox/bwrap-integration.test.ts b/packages/core/src/sandbox/bwrap-integration.test.ts index 0505773..7d0ba5f 100644 --- a/packages/core/src/sandbox/bwrap-integration.test.ts +++ b/packages/core/src/sandbox/bwrap-integration.test.ts @@ -6,11 +6,11 @@ // Spec: docs/DEVELOPMENT_PLAN.md §3.9a import { execSync, spawn } from 'node:child_process'; -import { mkdtemp, readFile, rm } from 'node:fs/promises'; +import { mkdtemp, readFile, rm, writeFile } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { afterEach, beforeEach, describe, expect, it } from 'vitest'; -import type { SandboxConfig } from '../config/types.js'; +import type { SandboxConfig, SandboxMode } from '../config/types.js'; import { wrapBashCommand } from './index.js'; function hasBwrap(): boolean { @@ -18,6 +18,13 @@ function hasBwrap(): boolean { execSync('command -v bwrap', { stdio: 'ignore' }); return true; } catch { + // These are the only tests that observe what the Linux sandbox *does* + // rather than what arguments it produces, and they self-skip. On the Linux + // runner that has to be a failure: a green suite that skipped every real + // enforcement check is how `--sandbox read-only` stayed writable. + if (process.env.DC_REQUIRE_BWRAP === '1') { + throw new Error('DC_REQUIRE_BWRAP=1 but bwrap is not on PATH'); + } return false; } } @@ -32,8 +39,9 @@ async function runSandboxed( userCommand: string, cwd: string, config: SandboxConfig, + defaultMode?: SandboxMode, ): Promise { - const wrapped = await wrapBashCommand({ userCommand, cwd, config }); + const wrapped = await wrapBashCommand({ userCommand, cwd, config, defaultMode }); return new Promise((resolve) => { const child = spawn(wrapped.command, wrapped.args, { cwd }); let stdout = ''; @@ -51,6 +59,8 @@ describe.skipIf(!RUN)('bwrap sandbox (real-kernel integration)', () => { let cwd: string; beforeEach(async () => { cwd = await mkdtemp(join(tmpdir(), 'dc-bwrap-int-')); + // Written outside the sandbox, so a read-only run has something to read. + await writeFile(join(cwd, 'seeded.txt'), 'from outside\n'); }); afterEach(async () => { await rm(cwd, { recursive: true, force: true }); @@ -87,4 +97,86 @@ describe.skipIf(!RUN)('bwrap sandbox (real-kernel integration)', () => { ); expect(r.stdout).not.toContain('exit=0'); }, 20_000); + + // The `--sandbox` axis added in #226 was live-verified on macOS only, where it + // exposed a profile that denied reads of the project directory itself. Linux + // shared the mode *resolution* and nothing more: `sandboxConfigForMode` was + // unit-tested, `buildLinuxBwrapArgs` was argument-tested, and no test ever + // spawned bwrap in a named mode to see what a command could actually do. + // + // A read-only sandbox that cannot read is the failure macOS had. A read-only + // sandbox that can write is the failure worth catching here. + describe('modes', () => { + // No `mode` and no `enabled`, so the mode comes from `defaultMode` — the + // path every host takes, since #226 made workspace-write the default rather + // than something each caller sets. + const unset: SandboxConfig = {}; + + it('read-only: the workspace is readable', async () => { + // This is the assertion macOS failed before #226: a sandbox that denies + // reads of the project directory makes every command useless. + const r = await runSandboxed(`cat ${cwd}/seeded.txt`, cwd, { ...unset, mode: 'read-only' }); + expect(r.code).toBe(0); + expect(r.stdout).toContain('from outside'); + }); + + it('read-only: the workspace is not writable', async () => { + const r = await runSandboxed(`echo nope > ${cwd}/written.txt`, cwd, { + ...unset, + mode: 'read-only', + }); + expect(r.code).not.toBe(0); + expect(r.stderr.toLowerCase()).toMatch(/read-only|permission|denied/); + }); + + it('workspace-write: the workspace is readable and writable', async () => { + const r = await runSandboxed( + `cat ${cwd}/seeded.txt && echo ok > ${cwd}/w.txt && cat ${cwd}/w.txt`, + cwd, + { ...unset, mode: 'workspace-write' }, + ); + expect(r.code).toBe(0); + expect(r.stdout).toContain('from outside'); + expect(r.stdout).toContain('ok'); + }); + + it('workspace-write: outside the workspace stays read-only', async () => { + const r = await runSandboxed('echo x > /etc/dc-should-not-exist', cwd, { + ...unset, + mode: 'workspace-write', + }); + expect(r.code).not.toBe(0); + }); + + it('danger-full-access: no bwrap at all', async () => { + const wrapped = await wrapBashCommand({ + userCommand: 'true', + cwd, + config: { ...unset, mode: 'danger-full-access' }, + }); + expect(wrapped.command).toBe('/bin/sh'); + }); + + it('defaultMode applies when the config names no mode', async () => { + // A host passing workspace-write must get a sandbox, not the historical + // "off unless configured" behaviour. + const wrapped = await wrapBashCommand({ + userCommand: 'true', + cwd, + config: unset, + defaultMode: 'workspace-write', + }); + expect(wrapped.command).toBe('bwrap'); + + const r = await runSandboxed(`echo ok > ${cwd}/d.txt`, cwd, unset, 'workspace-write'); + expect(r.code).toBe(0); + }); + + it('a library caller with no mode anywhere is still unsandboxed', async () => { + // Stated in wrapBashCommand's contract: an embedder must not become + // sandboxed by upgrading. + const wrapped = await wrapBashCommand({ userCommand: 'true', cwd, config: unset }); + expect(wrapped.command).toBe('/bin/sh'); + }); + }); }); diff --git a/packages/core/src/sandbox/profile.test.ts b/packages/core/src/sandbox/profile.test.ts index 854ac60..306afe6 100644 --- a/packages/core/src/sandbox/profile.test.ts +++ b/packages/core/src/sandbox/profile.test.ts @@ -109,6 +109,25 @@ describe('buildLinuxBwrapArgs', () => { expect(args[idx + 2]).toBe('/my/project'); }); + it('does not bind cwd read-write under read-only', () => { + // bwrap applies binds in order and the last wins, so an unconditional + // `--bind cwd` at the end silently overrode the read-only bind that + // sandboxConfigForMode had asked for. The mode resolved correctly and the + // workspace was writable anyway. + const args = buildLinuxBwrapArgs( + { enabled: true, mode: 'read-only', filesystem: { allowRead: ['/my/project'] } }, + '/my/project', + ); + expect(args).not.toContain('--bind'); + const last = args.lastIndexOf('/my/project'); + expect(args[last - 2]).toBe('--ro-bind-try'); + }); + + it('still binds cwd read-write under workspace-write', () => { + const args = buildLinuxBwrapArgs({ enabled: true, mode: 'workspace-write' }, '/my/project'); + expect(args).toContain('--bind'); + }); + it('unshares pid/ipc/uts', () => { const args = buildLinuxBwrapArgs({ enabled: true }, '/x'); expect(args).toContain('--unshare-pid'); diff --git a/packages/core/src/sandbox/profile.ts b/packages/core/src/sandbox/profile.ts index e0c665c..89d3a16 100644 --- a/packages/core/src/sandbox/profile.ts +++ b/packages/core/src/sandbox/profile.ts @@ -225,8 +225,23 @@ export function buildLinuxBwrapArgs( for (const p of fs.allowWrite ?? []) { args.push('--bind-try', p, p); } - // cwd is rw by default - args.push('--bind', cwd, cwd); + // cwd has to be visible or nothing works, but whether it is *writable* is the + // mode's decision — and this line used to make it unconditionally writable. + // + // bwrap applies binds in order and the last one wins, so under `read-only` + // the `--ro-bind-try` this loop already emitted for cwd (sandboxConfigForMode + // puts it in allowRead) was immediately overwritten by a read-write bind. The + // mode resolved correctly, the profile said the right thing, and a command + // could still write to the workspace. + // + // macOS never had this: buildMacOsProfile grants writes only from allowWrite, + // which read-only leaves empty. #226 verified the mode axis on macOS alone, + // and this is the half that was not looked at. + // + // An absent mode keeps the historical read-write bind, so a caller using the + // legacy `enabled: true` shape is unaffected. + if (config.mode === 'read-only') args.push('--ro-bind-try', cwd, cwd); + else args.push('--bind', cwd, cwd); // Network — three modes: // 1. allowedDomains: [] → no network at all