refactor: centralize runtime mount allow/deny lists in a declarative config#6339
Conversation
…config The allow lists (system dirs, /etc files, $HOME tool subdirs) and deny lists (forbidden $HOME credential dirs, on-disk credential stores) were hand-maintained in five separate modules — system-mounts.ts, etc-mounts.ts, home-whitelist.ts, credential-hiding.ts and sbx-manager.ts — one representation per runtime, which let them drift (e.g. compose masked .cargo/credentials but not .copilot/config.json, while sbx did the opposite). Introduce src/config/sandbox-mount-policy.json as the single declarative source of truth, loaded and validated by src/config/mount-policy.ts. All runtimes now derive their mounts from it: - system-mounts.ts / etc-mounts.ts read the compose allow lists from the policy - home-whitelist.ts becomes a thin re-export shim - credential-hiding.ts (compose /dev/null overlays) and sbx-manager.ts (microVM move-aside/restore) both derive from the one credentials list This also closes the compose gap: .claude/.credentials.json, .copilot/config.json and .gemini/* token files are now masked in compose mode too. The JSON is imported statically (resolveJsonModule) so it ships via tsc to dist/ and is inlined by the esbuild release bundle — no extra packaging step. Docs in docs/mount-policy.md. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 23717692-af7a-4e03-a156-5b696c3f01bd
|
| Metric | Base | PR | Delta |
|---|---|---|---|
| Lines | 98.94% | 98.75% | 📉 -0.19% |
| Statements | 98.86% | 98.69% | 📉 -0.17% |
| Functions | 99.35% | 99.24% | 📉 -0.11% |
| Branches | 95.16% | 94.86% | 📉 -0.30% |
📁 Per-file Coverage Changes (3 files)
| File | Lines (Before → After) | Statements (Before → After) |
|---|---|---|
src/sbx-manager.ts |
96.6% → 96.6% (+0.00%) | 96.2% → 96.2% (-0.02%) |
src/services/agent-volumes/etc-mounts.ts |
98.3% → 98.4% (+0.03%) | 98.3% → 98.4% (+0.05%) |
src/log-directory-setup.ts |
96.2% → 100.0% (+3.78%) | 96.3% → 100.0% (+3.71%) |
✨ New Files (1 files)
src/config/mount-policy.ts: 78.1% lines
Coverage comparison generated by scripts/ci/compare-coverage.ts
There was a problem hiding this comment.
Pull request overview
Centralizes sandbox filesystem mount policy across Compose, gVisor, and sbx runtimes.
Changes:
- Adds a declarative mount-policy JSON with typed accessors and validation.
- Migrates system, home,
/etc, and credential mounts to the shared policy. - Adds policy tests and documentation.
Show a summary per file
| File | Description |
|---|---|
src/config/sandbox-mount-policy.json |
Defines centralized mount allow/deny lists. |
src/config/mount-policy.ts |
Loads and exposes the policy. |
src/config/mount-policy.test.ts |
Tests policy accessors and invariants. |
src/services/agent-volumes/system-mounts.ts |
Derives system mounts from policy. |
src/services/agent-volumes/etc-mounts.ts |
Derives /etc mounts from policy. |
src/services/agent-volumes/home-whitelist.ts |
Re-exports centralized home lists. |
src/services/agent-volumes/home-whitelist.test.ts |
Tests compatibility exports. |
src/services/agent-volumes/credential-hiding.ts |
Derives credential overlays from policy. |
src/services/agent-volumes/credential-hiding.test.ts |
Tests policy-derived overlays. |
src/sbx-manager.ts |
Derives sbx credential scrubbing from policy. |
docs/mount-policy.md |
Documents policy structure and usage. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
src/config/mount-policy.ts:175
- Directory entries without
filesare silently omitted from Compose masking. Since Compose mounts.configwholesale (home-strategy.ts:46-47), the policy's.config/heroku,.config/hub, and.config/opcredential directories remain readable there even though they are declared deny entries; only sbx enforces them. Add a Compose directory-overlay mechanism (for example, an empty directory/volume) or reject directory entries that Compose cannot enforce.
} else if (entry.files) {
for (const f of entry.files) {
files.push(`${entry.path}/${f}`);
}
}
- Files reviewed: 11/11 changed files
- Comments generated: 2
- Review effort level: Medium
| function assertStringArray(value: unknown, label: string): readonly string[] { | ||
| if (!Array.isArray(value) || !value.every((v) => typeof v === 'string')) { | ||
| fail(`${label} must be an array of strings`); | ||
| } | ||
| return value as readonly string[]; |
| * Whitelisted `$HOME` subdirs the sbx backend mounts wholesale into the microVM: | ||
| * the shared tool-dir allow list plus the agent-state dirs `.copilot`/`.gemini`. | ||
| */ | ||
| const SBX_MOUNTED_HOME_SUBDIRS: readonly string[] = ['.copilot', ...HOME_TOOL_SUBDIRS, '.gemini']; |
|
@copilot Fix the code for all comments in this review thread. When a review comment includes a suggested change, apply the suggestion exactly. Do not make changes beyond what is described in the linked review thread. |
Done in the latest commit. Both review comments addressed: Path invariant validation (
Centralize
|
|
✅ Copilot review passed with no inline comments. @lpcox Add the |
|
🌑 The shadows whisper... Smoke Codex failed. The oracle requires further meditation... |
|
📰 VERDICT: Smoke Docker Sbx has concluded. All systems operational. This is a developing story. 🎤 |
|
🔑 Smoke Copilot PAT reports failed. PAT auth path may have issues... |
|
📰 DEVELOPING STORY: Smoke Copilot reports failed. Our correspondents are investigating the incident... |
|
📡 Smoke OTel Tracing reports failed. OTel tracing regression detected. |
|
❌ Smoke Copilot BYOK AOAI (Entra) reports failed. AOAI BYOK (Entra) mode investigation needed... |
|
🔌 Smoke Services — Service connectivity failed |
|
❌ Contribution Check failed. Please review the logs for details. |
|
🛡️ Smoke Copilot Network Isolation reports failed while checking network isolation. Investigate the egress model. |
|
🚀 Security Guard has started processing this pull request |
|
Build Test Failed Build Test Suite - See logs for details |
|
❌ Smoke Copilot BYOK AOAI (api-key) reports failed. AOAI BYOK (api-key) mode investigation needed... |
|
Chroot tests failed Smoke Chroot failed - See logs for details. |
|
✅ Smoke Gemini completed. All facets verified. 💎 |
|
✅ Smoke Claude passed |
|
❌ Smoke Copilot BYOK reports failed. BYOK mode investigation needed... |
|
📡 Smoke OTel Tracing reports failed. OTel tracing regression detected. |
|
Build Test Failed Build Test Suite - See logs for details |
|
🔌 Smoke Services — Service connectivity failed |
|
❌ Smoke Copilot BYOK AOAI (Entra) reports failed. AOAI BYOK (Entra) mode investigation needed... |
|
Chroot tests failed Smoke Chroot failed - See logs for details. |
|
📰 VERDICT: Smoke Docker Sbx has concluded. All systems operational. This is a developing story. 🎤 |
|
❌ Contribution Check failed. Please review the logs for details. |
|
❌ Smoke Copilot BYOK reports failed. BYOK mode investigation needed... |
|
🔑 Smoke Copilot PAT reports failed. PAT auth path may have issues... |
|
🚀 Security Guard has started processing this pull request |
|
🌑 The shadows whisper... Smoke Codex failed. The oracle requires further meditation... |
|
✅ Smoke Gemini completed. All facets verified. 💎 Smoke test completed with partial failures. Comment added to PR #6339. |
|
❌ Smoke Copilot BYOK AOAI (api-key) reports failed. AOAI BYOK (api-key) mode investigation needed... |
|
🛡️ Smoke Copilot Network Isolation reports failed while checking network isolation. Investigate the egress model. |
|
✅ Smoke Claude passed |
|
| Metric | Base | PR | Delta |
|---|---|---|---|
| Lines | 98.94% | 98.68% | 📉 -0.26% |
| Statements | 98.86% | 98.49% | 📉 -0.37% |
| Functions | 99.35% | 99.24% | 📉 -0.11% |
| Branches | 95.16% | 94.55% | 📉 -0.61% |
📁 Per-file Coverage Changes (4 files)
| File | Lines (Before → After) | Statements (Before → After) |
|---|---|---|
src/sbx-manager.ts |
96.6% → 96.6% (-0.05%) | 96.2% → 96.1% (-0.07%) |
src/services/agent-volumes/etc-mounts.ts |
98.3% → 98.4% (+0.03%) | 98.3% → 98.4% (+0.05%) |
src/artifact-permissions.ts |
97.3% → 97.4% (+0.14%) | 97.3% → 97.4% (+0.14%) |
src/log-directory-setup.ts |
96.2% → 100.0% (+3.78%) | 96.3% → 100.0% (+3.71%) |
✨ New Files (1 files)
src/config/mount-policy.ts: 80.2% lines
Coverage comparison generated by scripts/ci/compare-coverage.ts
Smoke Test: Claude Engine Validation
Overall Result: PASS Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
Smoke Test: Gemini Engine Validation
Overall status: FAIL Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "localhost"See Network Configuration for more information.
|
🔍 Smoke Test: Docker Sbx Validation
Overall: PARTIAL — MCP test passed; pre-step output variables ( PR author: @lpcox
|
The centralize-mount-policy change (#6339) added `.copilot/config.json` to the credential deny list with the reason "Copilot CLI persisted auth token". That is incorrect: config.json holds only experiment-assignment cache, first-launch timestamps, tips-shown flags and installed-plugin metadata — no token. The Copilot CLI's actual credentials live elsewhere (e.g. its sqlite stores), and `~/.copilot` is already bind-mounted rw as a tool subdir, so masking config.json protected nothing. Worse, it broke the Copilot CLI: compose masks credential files with a read-only /dev/null overlay, and the CLI reads AND rewrites config.json at startup (via temp-file + atomic rename), so it crashed silently at launch (exit 1, zero output) — failing smoke-copilot consistently. A rename can't target a bind-mounted file anyway, so masking that path is fundamentally incompatible with how the CLI writes its config. Fix: remove `.copilot/config.json` from the credentials deny list. The CLI now reads/writes its config normally, matching pre-#6339 behavior. Updated the sbx/compose tests, sbx-manager comment, and sbx-integration docs that used config.json as their example credential path. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4bdbe67a-cb99-41a2-944b-49cfd949360e
The centralize-mount-policy change (#6339) added `.copilot/config.json` to the credential deny list with the reason "Copilot CLI persisted auth token". That is incorrect: config.json holds only experiment-assignment cache, first-launch timestamps, tips-shown flags and installed-plugin metadata — no token. The Copilot CLI's actual credentials live elsewhere (e.g. its sqlite stores), and `~/.copilot` is already bind-mounted rw as a tool subdir, so masking config.json protected nothing. Worse, it broke the Copilot CLI: compose masks credential files with a read-only /dev/null overlay, and the CLI reads AND rewrites config.json at startup (via temp-file + atomic rename), so it crashed silently at launch (exit 1, zero output) — failing smoke-copilot consistently. A rename can't target a bind-mounted file anyway, so masking that path is fundamentally incompatible with how the CLI writes its config. Fix: remove `.copilot/config.json` from the credentials deny list. The CLI now reads/writes its config normally, matching pre-#6339 behavior. Updated the sbx/compose tests, sbx-manager comment, and sbx-integration docs that used config.json as their example credential path. Copilot-Session: 4bdbe67a-cb99-41a2-944b-49cfd949360e Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Problem
There was no centralized allow/deny list of files and directories for all runtimes. What the host filesystem exposes to an agent was hand-maintained across five separate modules, with a different representation per runtime:
system-mounts.ts— system dir allow list (compose)etc-mounts.ts—/etcfile allow list (compose)home-whitelist.ts—$HOMEtool-subdir allow list + forbidden-dir guard (shared)credential-hiding.ts— credential-file deny list as/dev/nulloverlays (compose)sbx-manager.ts— credential deny list as a per-parent map (sbx microVM)Because the two credential lists were maintained independently they had drifted: compose masked
.cargo/credentialsbut not.copilot/config.json/.claude/.credentials.json/.gemini/*, while sbx did the opposite. This is exactly the centralization deferred in #6336.Fix
Introduce a single declarative source of truth:
src/config/sandbox-mount-policy.json— the allow lists (system dirs,/etcfiles,$HOMEtool subdirs) and deny lists (forbidden$HOMEcredential dirs, on-disk credential stores), each entry annotated with a reason.src/config/mount-policy.ts— loads, validates (relative paths, no.., unique paths,filesonly ondirentries), and exposes typed accessors.Every runtime now derives its mounts from the policy:
system-mounts.tssystem.directories.{default,sysroot}etc-mounts.tssystem.etc(DinD staging / identity synthesis logic unchanged)home-whitelist.tsHOME_TOOL_SUBDIRS,HOME_FORBIDDEN_SUBDIRS)credential-hiding.ts/dev/nulloverlays derived fromcredentialFilesToHide()sbx-manager.tscredentialEntriesUnderMountedParents()The two backends still use their native mechanisms (compose
/dev/nullfile overlays; sbx directory move-aside), but from the same list, so they can't drift again.Security improvement (closes the compose gap)
Centralizing means compose now also masks
.claude/.credentials.json,.copilot/config.json,.cargo/credentials.tomland.gemini/*token files — the gap explicitly flagged in #6336. Masking a non-existent path via/dev/nullis a no-op, and agents receive credentials via the api-proxy/environment, not these on-disk stores, so this is safe.Packaging
The JSON is imported statically (
resolveJsonModule), sotscemits it todist/config/and the esbuild release bundle inlines it — no runtime file read or extra copy step. Verifiednode scripts/build-bundle.mjsinlines the policy and the bundle runs.Verification
npm run buildclean ·npm run type-checkcleannpm test: 243 suites / 3836 tests pass (addedsrc/config/mount-policy.test.tswith loader + invariant coverage; updated credential-hiding / home-whitelist / sbx tests)docs/mount-policy.mdNotes
system.*is compose-only (Docker + gVisor); the sbx microVM gets system libraries from its guest image, so it doesn't consume that section./etc/passwd/groupsynthesis, sysroot) stays in code — only the static path lists moved to the config.