Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,30 @@ jobs:
with:
node-version: 22

# Same step as the `test` job above (it moved there with the 3-way unit
# shard, #1288), repeated because jobs share no filesystem. WITHOUT IT this
# leg is a runner lottery: `prepareDirectSandbox()`
# returns null when bwrap is missing, so `overlayTargets()` in
# test/sandbox-shim-compiled-form.test.ts yields [] and 4 cases fail on an
# image that happens not to ship bubblewrap — MEASURED on two runs of the
# same commit range, one printing `bwrap missing` 5 times and failing, the
# other printing it 0 times and passing. The failures look like a code
# regression and are not one, which is the expensive part.
#
# ⚠️ Unlike vitest, `bun test` has no host-level skip gate here: the suite
# under `describe.skipIf(process.platform !== 'linux')` DOES run on this
# linux runner and can only fail once bwrap is absent. Keep this step in
# sync with the `test` job's copy.
- name: Enable bwrap sandbox for integration tests (best-effort)
run: |
sudo sysctl -w kernel.apparmor_restrict_unprivileged_userns=0 || true
command -v bwrap >/dev/null || sudo apt-get install -y bubblewrap || true
if bwrap --bind / / --unshare-user -- /bin/true 2>/dev/null; then
echo "bwrap userns OK — sandbox integration tests will run for real"
else
echo "bwrap still unavailable — sandbox integration tests will skip (see test-side gate)"
fi

- uses: oven-sh/setup-bun@v2
with:
bun-version: 1.4.1
Expand Down
8 changes: 7 additions & 1 deletion test/fs-policy-bwrap.e2e.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import { describe, it, expect, vi, beforeAll, afterAll } from 'vitest';
import { spawnSync } from 'node:child_process';
import { mkdtempSync, rmSync, rmdirSync, writeFileSync, mkdirSync, chmodSync, realpathSync, existsSync, statSync, lstatSync, readlinkSync, readFileSync, symlinkSync } from 'node:fs';
import { rmSandboxScratch } from './helpers/rm-sandbox-scratch.js';
import { tmpdir, homedir } from 'node:os';
import { join, dirname } from 'node:path';
import { buildFsPolicy, compileToBwrap } from '../src/adapters/cli/fs-policy.js';
Expand Down Expand Up @@ -109,7 +110,12 @@ d('bwrap three-tier enforcement (real bubblewrap)', () => {
writeFileSync(join(S, 'proj/.env'), 'API_KEY=zzz');
writeFileSync(join(S, 'ref/doc.md'), 'ref');
});
afterAll(() => { if (S) rmSync(S, { recursive: true, force: true }); });
// `build()` chmods the deny-mask sources to 000 (mirroring the worker), and a
// 000 DIRECTORY cannot be traversed — so a plain recursive delete throws
// `EACCES: permission denied, rm` for any NON-root uid, failing this file in an
// unnamed afterAll while every real case is already green. See the helper for
// the measurements (root hides it; Node and Bun fail alike).
afterAll(() => rmSandboxScratch(S));

it('readWrite: reads AND writes the project', () => {
const { args } = build({});
Expand Down
44 changes: 44 additions & 0 deletions test/helpers/rm-sandbox-scratch.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
/**
* Delete a scratch tree that contains bwrap DENY-MASK sources.
*
* WHY THIS EXISTS: the sandbox (and the tests that mirror it) chmods every mask
* source to `0o000` — emptiness is the guarantee, but the mode is what makes a
* mask unlistable for a non-root uid. A `0o000` DIRECTORY cannot be traversed,
* and `rm -r` must traverse to unlink what is inside, so a plain
* `rmSync(root, { recursive: true, force: true })` throws
* `EACCES: permission denied, rm '<scratch>'`. `force: true` does NOT help: it
* suppresses ENOENT, not EACCES.
*
* MEASURED — why it hid for so long: as root the delete succeeds, because
* CAP_DAC_OVERRIDE bypasses the DAC check. It fails only for a normal uid, which
* is exactly what a GitHub runner is. It fails identically under Node and Bun, so
* this is a cleanup bug, NOT a runtime difference (do not "fix" it with a
* bun-only skip).
*
* It surfaces as a failing UNNAMED afterEach/afterAll hook while every real case
* is green — a shape that reads like the suite is broken when only teardown is.
*
* Restoring traversal (0o700) on the way out is safe: the tree is a mkdtemp
* scratch dir being deleted in the same breath, so no mask outlives this call.
*/
import { chmodSync, lstatSync, readdirSync, rmSync } from 'node:fs';
import { join } from 'node:path';

export function rmSandboxScratch(root: string | undefined | null): void {
if (!root) return;
// Depth-first, chmod-then-descend: a 0o000 dir is unreadable until it is
// chmod'ed, so the restore has to happen BEFORE readdir, not after.
const stack = [root];
while (stack.length) {
const dir = stack.pop()!;
try { chmodSync(dir, 0o700); } catch { /* already gone, or not ours */ }
let entries: string[] = [];
try { entries = readdirSync(dir); } catch { continue; }
for (const entry of entries) {
const full = join(dir, entry);
// lstat, never stat: a symlink must not walk us out of the scratch tree.
try { if (lstatSync(full).isDirectory()) stack.push(full); } catch { /* vanished */ }
}
}
rmSync(root, { recursive: true, force: true });
}
24 changes: 22 additions & 2 deletions test/plugin-mcp-sandbox.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ import {
rmSync,
writeFileSync,
} from 'node:fs';
import { rmSandboxScratch } from './helpers/rm-sandbox-scratch.js';
import { isBunRuntime } from './helpers/ts-runner.js';
import { spawnSync } from 'node:child_process';
import { tmpdir } from 'node:os';
import { dirname, join, resolve } from 'node:path';
Expand Down Expand Up @@ -67,7 +69,11 @@ describe.skipIf(process.platform !== 'linux' || !existsSync(builtCli) || !bwrapU

afterEach(() => {
vi.unstubAllEnvs();
rmSync(root, { recursive: true, force: true });
// prepareDirectSandbox() chmods its deny-mask sources to 000 under `root`, and
// a 000 directory cannot be traversed — so a plain recursive delete throws
// `EACCES: permission denied, rm` for any NON-root uid (measured: green as
// root, red on Node and Bun alike as a normal user, i.e. what CI runs as).
rmSandboxScratch(root);
});

it.each(['default', 'custom'] as const)(
Expand Down Expand Up @@ -240,7 +246,21 @@ describe.skipIf(process.platform !== 'linux' || !existsSync(builtCli) || !bwrapU
// root (shared drive / ~/.local/bin symlink / fnm / nvm). Before the fix the
// trusted `botmux` shim's `exec node` failed `not found` → MCP gateway exited
// → Connection closed. The fix prepends dirname(realpath(process.execPath)).
it('resolves bare `node` under a hostile symlink-form host PATH (canonical exec dir prepended)', () => {
//
// NODE-ONLY, and not merely by preference: the fix under test is
// "prepend dirname(realpath(process.execPath))", and the probe then asserts
// bare `node` runs inside the sandbox. Under `bun test` process.execPath is the
// BUN binary, so the sandbox is granted bun's directory and the probe looks for
// a `node` that was never put there — exit 127. MEASURED on CI (runner node is
// /opt/hostedtoolcache/node/..., outside the /usr/bin the fixture keeps) and
// masked on a dev box that happens to have /usr/bin/node, which is why it reads
// as green locally under both runtimes.
//
// Granting bun's dir and probing for `bun` instead would NOT preserve the
// regression: the shim this protects execs `node` literally (see
// botmuxShimExecLine), so a bun-shaped probe would assert something production
// never does. The vitest/Node leg runs this for real; keep it there.
it.skipIf(isBunRuntime())('resolves bare `node` under a hostile symlink-form host PATH (canonical exec dir prepended) — Node-only: execPath is bun under `bun test`', () => {
const botmuxHome = join(dataDir, '..');
const botHome = join(botmuxHome, 'bots', 'cli_test');
const outbox = join(dataDir, 'sandboxes', 'sid-path', 'outbox');
Expand Down
25 changes: 24 additions & 1 deletion test/session-store-bwrap.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import { mkdtempSync, mkdirSync, writeFileSync, existsSync, readFileSync, rmSync
import { dirname, join } from 'node:path';
import { tmpdir } from 'node:os';
import { DatabaseSync } from 'node:sqlite';
import { isBunRuntime } from './helpers/ts-runner.js';

function bwrapUsable(): boolean {
if (process.platform !== 'linux') return false;
Expand All @@ -44,8 +45,30 @@ async function pollFor(predicate: () => boolean, what: string, timeoutMs = 10_00
}
}

/**
* Bun-only skip — the SCENARIO, not just an assertion, is Node-shaped.
*
* This regression is about a sidecar getting a NEW INODE when the store is
* closed and reopened: SQLite deletes `-wal`/`-shm` with the last connection, so
* a single-file `--ro-bind` pins a dead inode while a directory bind follows the
* name. MEASURED, closing the last connection:
* node:sqlite (Node) → `-wal` deleted ← the premise holds
* node:sqlite (Bun 1.4.2) → `-wal` KEPT
* bun:sqlite (Bun, = PRODUCTION) → `-wal` KEPT
* `src/services/sqlite-compat.ts` deliberately uses `bun:sqlite` under Bun, so
* the third line is what actually ships there. With the sidecar never deleted
* there is no new inode, and the final `toBe('v2')` would pass VACUOUSLY — it
* would stop testing the bind shape while still looking green.
*
* So this stays Node-only coverage instead of being weakened into an assertion
* that cannot fail. The `bun-test` leg runs every OTHER case in this repo; the
* inode scenario is exercised by the vitest/Node leg, which is where the premise
* is real.
*/
const BUN_SKIP_REASON = 'SQLite under Bun keeps -wal on last close, so the reopen-inode premise never occurs';

describe.skipIf(!bwrapUsable())('bwrap persistent pane × SQLite store reopen', () => {
it('a dir-bound sandbox that outlives the writer reads commits made by the REOPENED store', async () => {
it.skipIf(isBunRuntime())(`a dir-bound sandbox that outlives the writer reads commits made by the REOPENED store (${BUN_SKIP_REASON} — Node-only)`, async () => {
const dataDir = mkdtempSync(join(tmpdir(), 'bwrap-session-store-'));
const ctlDir = mkdtempSync(join(tmpdir(), 'bwrap-session-ctl-'));
const storeDir = join(dataDir, 'session-stores', 'appA');
Expand Down