fix(isolation): close the two P1s in scenario-verify's isolation evidence boundary

[tnunamak]

What this fixes

scenario-verify is the tool this project (PDP-Connect, which builds connectors that pull a user's data from third-party sites and services) uses to check whether a connector's recorded run can be trusted as evidence and, if so, how strongly. Before this PR, it could award its strongest possible claim about a run — recorded_replay: PASS, meaning "this replay actually ran inside an isolated sandbox, not just on the bare host" — without the underlying operating-system isolation actually holding. A connector's replay could look sandboxed on paper while its child process still had a real path to touch the host filesystem or dial a socket outside the sandbox, and the tool would print the same "PASS" a genuinely isolated run gets.

An external reviewer (call them the first-pass reviewer — a different person from the second, verification reviewer described below) found this while doing a final pass over PR #238 (the earlier PR that introduced most of this isolation machinery) against the tree actually merged to main at commit ab415be6c. Their full report is saved as local/reviewer-238-merged-ab415be6c-0902.md — local/ is a gitignored scratch folder this project uses for review notes and other working documents that aren't meant to ship as part of the codebase, so this path won't be visible in the diff and isn't reachable from a plain GitHub checkout; it's referenced here for traceability on this machine. Two separate defects could each cause it:

1. Launcher trust gap. The isolation code spawns two external programs, unshare and bwrap (both are Linux sandboxing tools — unshare creates new OS namespaces so a process gets its own private view of the filesystem/network/process list, and bwrap, short for bubblewrap, builds a restricted filesystem view for a child process using bind mounts). Before this PR, the code resolved both by name through the inherited $PATH environment variable — the same list of directories the shell searches for any command. If an attacker could get a program named unshare or bwrap onto an earlier $PATH directory than the real ones (for example, by controlling $PATH in a CI job or a connector's own environment), that fake binary would run instead of the real sandboxing tool, and the code had no way to tell the difference. It would report success and the sandbox would simply never have existed.

2. Partial read-only mounts. The isolation code makes the connector's own source directory (REPO_ROOT, the reviewer's and this project's name for the checked-out repository root — the code the connector runs from) and a few other paths read-only inside the sandbox using a bind mount followed by remount,ro,bind (the standard two-step Linux recipe for making an existing mount read-only). The problem: when a mount point has its own nested mounts underneath it — for example Docker injects separate mounts for /etc/resolv.conf and /etc/hostname inside /etc — that remount,ro,bind command only affects the top mount, not the submounts underneath it. Linux does not apply it recursively. A submount could stay writable even though its parent directory reported as read-only, and the code's own verification check only tested the parent, never walked down into submounts to check each one.

Either gap meant recorded_replay: PASS could be printed while the isolation it claims to prove either never engaged (gap 1) or engaged incompletely (gap 2).

The fix, commit by commit

01cbef87e withholds the strong claim entirely (falls back to diagnostic_replay: PASS, a weaker claim meaning only "this replay ran and passed its own checks," not "under proven OS isolation") while the two gaps above get fixed, so nothing in between could accidentally print a claim it hadn't yet earned.

7c829403a fixes gap 1: unshare and bwrap are now resolved from a fixed, hardcoded allowlist of absolute paths (e.g. /usr/bin/unshare) instead of searching $PATH. A $PATH-prepended fake binary is never selected, whether the code is just checking up front whether isolation is even possible on this machine (a quick check the code runs first, before committing to a real sandboxed run) or actually launching the sandboxed process for real — both now resolve through the same trusted-path lookup. The only way to defeat this is to edit the allowlist itself in the source, which is a code change, not an environment-variable attack.

21dfe6d63 fixes gap 2: the remount step now walks every submount under each read-only bind and remounts each one individually, not just the top-level mount. A second, independent check runs after the sandbox is fully set up (after Linux's pivot_root, the syscall that swaps the sandbox's root filesystem in for the real one) and verifies every submount is actually read-only before letting the connector's own code run at all — if it isn't, the spawn fails closed (aborts) rather than proceeding with a partially-writable filesystem.

fff750d27 closes a third gap the first two fixes exposed rather than closed: making every submount read-only stops a connector from creating new sockets on disk during a run, but it doesn't stop it from dialing a Unix domain socket (a file-like, filesystem-backed inter-process communication channel — think a named pipe two processes both connect to) that already existed at that path before the sandbox even started. A read-only bind mount blocks writes, not socket connections, so a pre-existing socket under a read-only bind stayed reachable from inside the sandbox. This commit adds a scan, run once immediately before each spawn, that walks every read-only-bound, user-writable path (the connector's source directory, the Node.js runtime install directory, and the on-disk cache for Playwright — a browser-automation library some connectors use to drive a real browser — but not /usr or /etc, which are root-owned and outside this scan's threat model, since only a privileged root process could plant a socket there, not the sandboxed connector) looking for sockets that already exist. If it finds any, recorded_replay is withheld and the exact socket path is named as the reason. A second reviewer, brought in specifically to verify these fixes (see "Verification" below), checked this exclusion empirically, not just by reading the code: as an unprivileged user (matching the real threat model, since the sandboxed process runs as the same non-root user as its caller), it could not create a socket under /usr or /etc at all, and found none already there on a representative container image.

ced8300be is a small follow-up that same verification reviewer asked for: the recorded_replay: PASS line printed to the terminal was terse and didn't say what had been checked — a reader had to already know to go read claims.ts (the source file that decides which of the two claims, recorded_replay or diagnostic_replay, a run is eligible for, and why) to learn what "PASS" meant, even though the failure path already named its reasons explicitly. The PASS line now states its three preconditions inline (trusted launcher path used, every read-only submount verified, no pre-existing sockets found), so both outcomes are self-explanatory without cross-referencing code.

Verification

This second, verification reviewer (not the author of these fixes, and not the same person as the first-pass reviewer above) reproduced all three fixes live rather than accepting them from the diff or commit messages, in a privileged Docker container (a container started with expanded kernel permissions, needed here because creating OS namespaces normally requires elevated rights) running both bwrap and unshare for real. The bare host used for this work cannot run either tool directly — it's locked down by AppArmor, a Linux kernel feature that restricts what unprivileged processes are allowed to do, and it blocks the exact namespace-creation calls unshare/bwrap need — which is why the privileged container is necessary and matches every isolation review in this project's history. Their full report is saved the same way as the first report above, as local/isolation-r10-independent-0902.md. For each fix, the reviewer reverted only that fix (not the test), confirmed the exact escape the first-pass reviewer named actually reproduces without it, then restored the fix and confirmed it's blocked. Verdict: SHIP — no P1s (severity-1: a defect serious enough to block shipping) and no P2s (severity-2: a real defect, but not blocking) were found — plus two purely informational follow-ups (one of which, the PASS-line wording, is already addressed in ced8300be above).

This PR further rebases that reviewed work onto current main — it now includes two unrelated fixes merged to main after the review (PR #269, which fixed an Apple Health connector import bug, and PR #272, which fixed an authentication-port bug for clients using MCP, a protocol unrelated code in this project uses to expose tools to AI assistants) — and re-verifies the test numbers on the new commit, since the base moved:

• isolation-mechanism.test.ts (the isolation-specific test suite, covering both sandboxing mechanisms): 67 total, 57 pass, 0 fail, 10 skip, run standalone four separate times with identical results each time — 23 tests tagged [bwrap], 24 tagged [unshare], zero failures under either mechanism. The 10 skips are pre-existing fixture-precondition self-skips (tests that check for $HOME/.ssh/agent or /run/user/<uid> and skip themselves if those paths don't exist in the test environment) already characterized as benign in earlier reviews of this code.
• scenario-cli.test.ts (the real end-to-end CLI round trip): 51/51 pass.
• scenario-verify-strict.test.ts (unit tests for the logic that decides which claim — recorded_replay or diagnostic_replay — a run is eligible for, including the new socket-scan tests): 86/86 pass.
• The full pnpm test package suite (pnpm is this monorepo's package manager and task runner; pnpm test runs every test file under bin/, connectors/, src/ — roughly 5,300 individual tests) is not cleanly 0-failures under concurrent load in this container, and the previous commit message on fff750d27 incorrectly claimed it was — that claim has been corrected in the commit message itself rather than left standing. Running that full suite concurrently (this project's test runner runs multiple files in parallel to save time) surfaces occasional failures in isolation-mechanism.test.ts and connectors/slack/slackdump-runtime.test.ts (a stall-timeout test that is inherently timing-sensitive) alongside connectors/codex/coverage-truthful.test.ts (a test that checks the tool correctly flags files it couldn't read as "coverage unaccounted for" rather than silently treating them as covered — unrelated to isolation). Rerunning each of those three files standalone, outside the concurrent load, they all pass cleanly (0 fail) — confirming resource contention from ~5,300 tests sharing one container, not a code defect. To isolate what's actually pre-existing versus caused by this branch, the same full suite was also run on this branch's parent commit (d250afcf6, before any of these isolation fixes existed): only connectors/codex/coverage-truthful.test.ts failed there too, with the same 2 failures. That confirms this file alone is genuinely pre-existing and unrelated to this branch; the other two files are load-induced flakes specific to running the full suite concurrently, not caused by this branch's code, but also not something this reviewer's environment reproduces as a clean "0 failures" the way the isolation-specific files do.

Round 11 changes

A second external reviewer looked at this PR at commit ced8300be and found the Round 10 repair above was still incomplete in three ways. Full verbatim verdict: local/reviewer-274-278-263-0902.md (same gitignored scratch-notes convention as above). Four new commits fix all three, and — per the reviewer's explicit instruction — recorded_replay stays withheld this round too, pending a fresh independent review of these changes (the commit that writes a fix doesn't get to also certify it).

1. The trusted-launcher fix (Round 10) covered unshare/bwrap themselves, but not the shell they hand their setup script to. Both tools were told to run sh -c '<script>' — a bare name, not an absolute path — so sh still got resolved through whatever $PATH the calling process happened to have, before the script's own PATH= line (which only protects commands inside the script) could matter. Confirmed live: a fake sh planted earlier in $PATH ran instead of the real one, skipping the whole sandbox setup while still reporting success. Fixed by resolving sh through the same fixed, absolute allowlist as unshare/bwrap, at all three places a sh -c script gets built.

2. The pre-existing-socket scanner (added in Round 10 to close a socket-dial gap) silently treated "I couldn't read this directory" as "nothing here." A directory that's searchable but not listable (a real, separate Unix permission combination) hides its contents from the scan while a socket inside it stays fully connectable — confirmed live with a real socket under such a directory. The scanner now reports "couldn't verify" as its own explicit, distinct outcome, and that withholds the strong claim exactly like finding an actual socket does — never silently passes. Separately, that scanner only ran once, from outside the sandbox, before anything started; a socket could in principle appear after that single check. It's now also run twice more, from inside the sandbox itself, right up against the moment the connector code actually starts — this narrows the gap for a socket to slip in unnoticed to a few milliseconds instead of a whole run's duration.

3. The check that walks a read-only directory's nested mounts (also from Round 10) used a parsing shortcut that could miss real paths and couldn't check individual files. Linux's own mount-listing file escapes unusual characters like spaces in a path (turning a space into \040), and the old code compared against paths without decoding that escaping first, so a nested mount at a path containing a space was silently skipped — confirmed live. The same check also tried to verify each nested mount by creating a test file inside it, which only makes sense for a directory; a single file bind-mounted on top of another file (a real, common Docker pattern) always failed that particular test for an unrelated reason, so it always got reported as "already read-only" whether or not it actually was — confirmed live: a genuinely still-writable file mount passed the old check. Both are fixed now: paths are properly decoded before comparing, and a file mount gets checked by trying to open it for writing directly instead.

Round 11 verification (corrected): at reviewed head 4bf2997e6, isolation-mechanism.test.ts reported 80 total: 68 pass, 2 fail, 10 skip as root. The two failures are permission tests that require a real non-root user; the same file as UID 1000 reported 80 total: 34 pass, 0 fail, 46 skip.

One honestly-disclosed limitation found while building these tests: running all three of the files above together in one process (rather than each on its own, which is how they're actually invoked) surfaces occasional additional flakiness beyond each file's own clean run, traced to the new inside-the-sandbox socket scan racing against other, unrelated concurrent tests that are creating and deleting their own scratch files in the same shared repository checkout — not a defect in the scan logic itself (a dedicated fix already closes the specific case where the scanning tool's own "path vanished mid-check" report was being treated the same as "found something," which was the main source of this). This is the same class of test-environment resource contention Round 10's own PR description already disclosed for the full test suite; a follow-up architectural change (a private snapshot of what the sandbox is allowed to see, taken once and never re-read from the live, shared filesystem) is noted in the code as the way to close this fully, but is out of scope for this bounded repair.

What this does not cover

This PR closes exactly the two blocking (P1-severity) defects the first-pass reviewer found against the merged tree at commit ab415be6c — the launcher-trust gap and the partial-read-only-mount gap — plus the socket-dial gap the fixes themselves exposed mid-repair. It does not touch the product/runtime code the earlier PR #238 review already accepted — only the scenario-verification tool's evidence boundary.

The original review also noted that a larger, separate release effort — spanning code in other repositories this organization maintains alongside this one, unrelated to isolation and outside this PR's diff — was still waiting on two other pull requests to merge. Both have since merged, so that larger release is no longer blocked on them. A third, unrelated item in that same larger effort remains open in draft form; it doesn't touch isolation and doesn't block this PR.

Round 12 changes

Before this change, a caller could place a silent program named find in filesystemBindPath, the writable evidence directory that the Bubblewrap sandbox exposes to the child, and put that directory first in the command-search PATH. The Bubblewrap inner script then resolved find from that attacker-controlled directory during its Unix-socket scan, so a real pre-existing socket under the read-only repository bind was not reported and the child command ran. The inner script now first sets PATH=/usr/sbin:/usr/bin:/sbin:/bin, so its find, grep, and echo commands resolve only from the sandbox fixed system directories.

postPivotVerificationStatements, the function that produces the final filesystem-closure checks, documented that its returned shell statements can run without a setup prelude but did not establish that same trusted PATH before its file probe invokes sh. Its first returned statement now initializes the fixed PATH, so the documented standalone caller cannot replace that shell through its environment.

A privileged-container regression test runs three controls from the reviewer: a real socket with a normal PATH exits 92; the same socket with a writable-directory fake find first on PATH still exits 92; and, after removing the socket, the normal control exits 0 and the child writes its marker. A second test calls the post-pivot statements with a fake sh first on PATH and verifies that the fake shell never runs. I verified the regressions by temporarily removing each new PATH assignment: each removal made the corresponding test fail, and restoring the assignment made it pass.

At 4bf2997e6, run pnpm --dir packages/polyfill-connectors test measured 5,375 tests: 5,286 pass, 21 fail, 46 cancelled, and 22 skipped. The branch parent measured 5,345 tests: 5,258 pass, 19 fail, 46 cancelled, and 22 skipped; the two additional failures are root-only permission-coverage cases in isolation-mechanism.test.ts, which pass as a non-root user. The prior 68/70 wording is corrected to the measured root result: 80 total, 68 pass, 2 fail, 10 skip. This round adds two passing isolation tests, whose current standalone results are 82 total, 70 pass, 2 fail, 10 skip as root and 82 total, 36 pass, 0 fail, 46 skip as UID 1000. scenario-verify-strict.test.ts passes 140/140 and scenario-cli.test.ts passes 51/51 in the same privileged environment. A later full package invocation exceeded the previous run duration, was stopped, and is not claimed as passing.

recorded_replay remains withheld. The production flag that would allow that evidence claim remains hardcoded false; this repair proves the two reviewed PATH boundaries but does not independently certify the broader isolation evidence boundary.

Round 13 changes

A mount point whose name contained a newline could leave a directory writable inside the sandbox while the sandbox's own safety check reported it as read-only. The mount enumerator — the code that reads the kernel's mount table and lists the nested mounts underneath a directory the sandbox is supposed to make read-only, such as the separate /etc/resolv.conf mount Docker injects inside /etc — was reading paths in a way that split one real path into two fake ones.

Linux exposes the live mount table as a text file, /proc/self/mountinfo, one mount per line. Because a filename may legally contain a newline, the kernel escapes such a byte as the four characters \012 — a backslash followed by the byte's value in octal — so that a record still occupies exactly one line. The old code decoded that escaping first, turning \012 back into a real newline, and only then passed the result through a pipe that splits on newlines. A mount at <dir>/a<newline>b therefore arrived as the two entries <dir>/a and b, and neither of those is a path that exists. Confirmed live before the fix: both entries came back as non-existent. Two callers share that reading code, and the consequence differs between them. The setup step — the phase that applies the read-only remounts while the sandbox is being built — tries to remount a path that isn't there and fails loudly. The other caller is the verifier — the check that runs after the sandbox has switched to its new root filesystem (the pivot_root system call, which is why the code calls it the post-pivot check; it is one component, not two) and confirms every directory that should be read-only actually is. It probes the path for writability, and a probe against a non-existent path produces no output, which the verifier reads as "this mount is confirmed read-only." So the one shape that should have raised an alarm reported success instead.

The fix reverses which form of the path travels through the newline-splitting stage. Raw, still-escaped records cannot contain a newline, by the kernel's own guarantee, so those are what get split into lines now; the unescaping happens afterward, once per record, inside the loop; and the finished path is handed to the remount and probe commands as a direct argument, never re-serialized into text that something downstream would have to split again. Decoding runs exactly once, left to right, which is what keeps a mount point literally named \012 (the kernel escapes its backslash, so it arrives as \134012) distinct from one that really contains a newline.

The second defect was that a failure to read the mount table at all was indistinguishable from finding nothing. The old code piped the mount enumerator into a loop; when the enumerator exited nonzero, or the mount table was missing, the pipeline still reported success having looped zero times. Confirmed live: both cases exited 0. Setup then continued and the verifier passed, on the strength of an enumeration that never happened.

The mount enumerator now refuses to proceed without positive evidence that it worked. It captures its own exit status directly instead of through a pipe that discards it, rejects a file that ends mid-record, and requires a count of records that each passed a structural check against the documented mountinfo format. Counting lines would not have been enough: a file of pure garbage and a truncated file both produce a nonzero line count, so the structural check and the mid-record test are what actually catch them. The verifier also captures the exit status of its own read-only check. That check runs inside a shell command substitution, which yields a command's output rather than its exit status, so an enumeration failure there would otherwise vanish — the same swallowed-failure shape one level up.

Two details are worth recording because the obvious implementations of each are wrong here. The mount enumerator stages its intermediate result in a shell variable rather than a temporary file, because neither place this code runs is guaranteed to have a writable temporary directory. This sandbox is built two different ways depending on what the host allows — with bwrap (Bubblewrap, a userspace sandboxing tool) or with unshare (the Linux utility that creates new namespaces directly) — and an earlier revision that used mktemp, the standard utility for creating a temporary file, failed under both for reasons that had nothing to do with mounts: the Bubblewrap sandbox starts from an empty root filesystem with no /tmp, and the unshare path's new root provides none either. And truncation is detected by inspecting the file's final byte rather than with RT, a record-terminator variable that exists in GNU awk but not in mawk, the awk actually installed in the target environment — an RT-based test would have silently passed on every input.

The regressions run in a privileged container — one started with the Docker flag that grants the mount permissions these tests need — and use real bind mounts rather than hand-written fixtures, so the kernel's own escaping is what the code faces. Five mount points whose names contain a newline, a tab, a space, a backslash, and the literal text \012 are each asserted to arrive as exactly one path that exists on disk. A sixth case mounts <root>/pre-fix alongside <root>/pre and asserts the first is not treated as living inside the second — the failure a naive string-prefix test invites, since <root>/pre is a prefix of <root>/pre-fix without being a parent directory of it. Six more controls point the mount enumerator at a mount table that is unreadable, absent, empty, malformed, or truncated, or make it exit nonzero, and require each to block the sandboxed program from running and to fail the verifier.

Both fixes were checked by reverting them. Restoring the decoded-path output fails exactly the newline and literal-\012 cases, and the failure output shows the same non-existent fragments described above. Disabling the fail-closed conditions fails all six runnable controls. The unreadable-file case turned out to be caught independently by two of those conditions and fails only when both are removed.

In the privileged container, isolation-mechanism.test.ts — the suite covering both of the bwrap and unshare mechanisms named above, which contains the regressions described here alongside the rest of this sandbox's coverage — reports 96 total: 83 pass, 2 fail, 11 skip. The commit this branch builds on measures 82 total: 70 pass, 2 fail, 10 skip. The 14-test increase is exactly the new regressions listed above, 13 of which run and pass; the one added skip is the unreadable-file control, which is skipped when the suite runs as root because root bypasses file permissions entirely and would make that control pass without proving anything. The 2 failures are the same two permission checks in both runs — they need a non-root user — so this change adds no failures. scenario-verify-strict.test.ts, the unit tests for the logic that decides which evidence label a run has earned (the strongest being recorded_replay, which asserts "this replay ran under proven OS isolation"), passes 89/89. scenario-cli.test.ts, which tests the command-line entry point that runs a scenario end to end, passes 50/51; the one failure is a timing-sensitive replay-pacing test that fails identically on the parent commit.

One coverage limit is worth stating precisely. The five byte classes tested here are the four the kernel documents as escaped in a mount path (space, tab, newline, backslash) plus the literal-escape case; they are not a proof that no other byte reaches this code unescaped, and that broader claim is untested.

recorded_replay, defined above, remains withheld, and the flag that would permit it stays hardcoded false. This round closes the two reviewed defects in the mount enumerator. It does not close the broader isolation evidence boundary — the full set of properties that would have to hold before a run could honestly claim it executed under proven OS isolation, of which correct mount enumeration is one part among several. Closing that boundary would require an independent review of the whole sandbox, not just these two repairs, and nothing here should be read as certifying it.

Assisted-by: AI

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated
pdpp	Ready	Preview	Sep 4, 2026 12:17am UTC

[tnunamak]

Fixed the writable-file submount probe truncation hazard.

• probe_ro now opens file submounts in append mode (>>) without O_TRUNC and performs no write.
• The regression test bind-mounts a real host file, requires the writable-submount verdict, and asserts the source bytes are unchanged.
• New head: 4bf2997e62cea76738b5556b19b99dbb60ae6452

Validation: isolation mechanism suite (40 pass, 40 capability-gated skips), scenario-verify strict suite (89 pass), and package static verification passed. The complete package suite ran 5,323 pass / 50 skipped with two unrelated timing failures recorded separately.

[tnunamak]

Ported to PDP-Connect/data-connectors#81, which is now the single home for connector scenario tooling. Closing so the work is not open in two repositories.

Assisted-by: AI