From 28642756b57e763e49f2c30bdcd4f394392c58ab Mon Sep 17 00:00:00 2001 From: dtzp555 Date: Thu, 28 May 2026 17:34:36 +1000 Subject: [PATCH] =?UTF-8?q?fix(sandbox):=20PR-B=20fold-ins=20=E2=80=94=20a?= =?UTF-8?q?llow=20read=20~/.claude=20+=20skip=20wrap=20under=20test=20mock?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Live PI231 verification of d0dcd28 surfaced two real-runtime issues: 1. ~/.claude was denyRead, but claude CLI MUST read its own OAuth credentials file to authenticate. Result: 100% of anthropic requests on sandboxed PI231 failed with "Not logged in · Please run /login". Live transcript: {"event":"fallback_hop_error", "provider":"anthropic", "error":"Not logged in · Please run /login"} The cross-tenant risk for ~/.claude was a false security trade-off: (a) ~/.claude is the spawn's OWN auth, not cross-tenant material (all OLP clients share the same Anthropic OAuth — the file is not multi-tenant secret) (b) Real protection for the model accidentally exfiltrating creds via tool_use is Phase 6c's --system-prompt suppressing tool descriptions; sandbox-runtime can only blanket-deny a path, not distinguish "claude reads creds" from "model emits Read tool" Fix: remove ~/.claude from denyRead. Keep ~/.olp, ~/.ssh, ~/.config, ~/.codex (all genuinely cross-tenant). Suite 44a (cat ~/.olp/keys.json MUST fail) unchanged — still proves the core acceptance. Suite 44c added (regression guard): in-sandbox cat ~/.claude/.credentials.json MUST succeed when the file exists. 2. anthropic.mjs::_spawnAndStream called wrapSpawn() unconditionally, including when a test had set __setSpawnImpl(mockFn). The wrap rewrites bin to '/bin/sh -c ' which breaks every HTTP integration test that asserts on spawn args. Result on PI231 (sandbox active): 16 production-shape tests failed because their mocks were never called with the expected bin/args. Fix: skip wrapSpawn when spawnImpl !== defaultSpawn (mock active). Mocks don't exec, so isolation is meaningless there anyway. Real-CLI spawns (production + Suite 44) still get wrapped. Tests: 813 → 813 (44c is also PI231-gated, skips on Mac with file absent; the fold-in does not change Mac test count). Authority: - @anthropic-ai/sandbox-runtime v0.0.52 — wrapWithSandbox() shell-string return shape is the proximate cause of (2) - Live PI231 2026-05-28 transcript: /v1/chat/completions returning content=null + "Not logged in" from anthropic; OLP_E2E_SANDBOX=1 npm test showing 16 prior pass tests now fail - ADR 0014 § 4.1 PR-B acceptance criteria — the negative test (44a) continues to pass; the false-positive denyRead (~/.claude) is corrected Co-Authored-By: Claude Opus 4.7 --- lib/providers/anthropic.mjs | 25 +++++++++++++------ lib/sandbox/manager.mjs | 26 +++++++++++++++++--- test-features.mjs | 49 +++++++++++++++++++++++++++++++++++++ 3 files changed, 89 insertions(+), 11 deletions(-) diff --git a/lib/providers/anthropic.mjs b/lib/providers/anthropic.mjs index 6582f9e..715e3a6 100644 --- a/lib/providers/anthropic.mjs +++ b/lib/providers/anthropic.mjs @@ -928,15 +928,26 @@ async function* _spawnAndStream(irRequest, authContext, spawnImpl) { // ADR 0009 Amendment 1 § unchanged spawn args: only the spawn execution is // wrapped — bin/args/env/NDJSON parsing are all unchanged from pre-PR-B. // + // 2026-05-28 PR-B fold-in: skip sandbox wrap when a custom spawnImpl is in + // use (test mode — __setSpawnImpl was called). Test mocks do not actually + // exec a binary, so sandbox isolation provides no protection there; the + // wrap only obscures the original bin/args from the mock's assertions and + // breaks every HTTP integration test that uses __setSpawnImpl + asserts + // on spawn args. wrapSpawn is for real-CLI spawns; Suite 44 exercises that + // path directly without going through this provider. + // // Authority: @anthropic-ai/sandbox-runtime v0.0.52 wrapWithSandbox() API, // ADR 0014 § PR-B, spike-anthropic.mjs (PI231 2026-05-28). - const wrapped = await wrapSpawn({ - bin, - args, - env, - cwd: undefined, // let manager assign ephemeral cwd - allowedDomains: ['api.anthropic.com', 'statsig.anthropic.com'], - }); + const usingMockSpawn = spawnImpl !== defaultSpawn; + const wrapped = usingMockSpawn + ? { bin, args, env, cwd: undefined, sandboxed: false } + : await wrapSpawn({ + bin, + args, + env, + cwd: undefined, // let manager assign ephemeral cwd + allowedDomains: ['api.anthropic.com', 'statsig.anthropic.com'], + }); // OCP server.mjs:542: spawn(CLAUDE, cliArgs, { env, stdio: ["pipe", "pipe", "pipe"] }) const proc = spawnImpl(wrapped.bin, wrapped.args, { diff --git a/lib/sandbox/manager.mjs b/lib/sandbox/manager.mjs index a02f2d4..b1c8492 100644 --- a/lib/sandbox/manager.mjs +++ b/lib/sandbox/manager.mjs @@ -167,12 +167,30 @@ export async function bootstrapSandbox(opts = {}) { // denyRead: Absolute literal Linux paths per ADR 0014 § Pitfalls #2. // No ~ or glob — ripgrep glob expansion is not used here to stay safe on // both Linux (bwrap) and macOS (sandbox-exec profile). + // + // 2026-05-28 PR-B fold-in: ~/.claude is NOT in denyRead. It contains the + // spawn's own OAuth credentials — claude CLI must read its own auth file + // to function. Denying read here causes "Not logged in" failures even + // though the operator has valid credentials present. + // + // The cross-tenant risk for ~/.claude is mitigated by Phase 6c's + // --system-prompt flag (ADR 0009 Amendment 1): the system prompt is + // fully replaced, suppressing the default tool descriptions that would + // otherwise tell the model it has Read/Bash. Without tool descriptions, + // the model is highly unlikely to emit tool_use even under prompt + // injection. Sandbox's contribution here is protecting OTHER auth + // material (other clients' OLP keys, SSH identity, other providers' + // tokens) — files claude CLI does NOT legitimately need. + // + // If we ever switch to a CLI that requires reading credentials.json + // AND also legitimately offers tool execution that surfaces those files + // (no known case today), this trade-off needs revisiting. const denyRead = [ - join(home, '.olp'), // OLP API keys + config - join(home, '.claude'), // Claude OAuth credentials - join(home, '.ssh'), // SSH identity material + join(home, '.olp'), // OLP API keys + config — cross-tenant + join(home, '.ssh'), // SSH identity material — lateral movement join(home, '.config'), // Generic config dir (may contain tokens) - join(home, '.codex'), // Codex config (PR-C will wrap codex) + join(home, '.codex'), // Codex config — other-provider auth (PR-C will wrap codex) + // NOT denied: ~/.claude — this spawn's own auth, breaks claude CLI if denied ]; // allowWrite: ephemeral spawn workspace only. mkdirSync at bootstrap. diff --git a/test-features.mjs b/test-features.mjs index 56edff3..7991da5 100644 --- a/test-features.mjs +++ b/test-features.mjs @@ -18222,6 +18222,11 @@ describe('Suite 43 — Phase 7 PR-B: lib/sandbox/manager.mjs', () => { const _RUN_SANDBOX_E2E = Boolean(process.env.OLP_E2E_SANDBOX); +// Suite 44c (fold-in 2026-05-28) needs `join`. statSync already imported at +// the Suite 17 boundary above. We just need a local `join` alias here since +// the file-top `join` was bound as `_pathJoinForSetup`. +const join = _pathJoinForSetup; + describe('Suite 44 — sandbox negative security test (PI231 only)', { skip: !_RUN_SANDBOX_E2E }, () => { before(async () => { @@ -18325,4 +18330,48 @@ describe('Suite 44 — sandbox negative security test (PI231 only)', { skip: !_R assert.ok(stdout.includes('SANDBOX_PROOF'), `stdout must contain SANDBOX_PROOF; got: ${stdout.slice(0, 100)}`); }); + + it('44c: in-sandbox spawn CAN read ~/.claude/.credentials.json (regression guard for fold-in 2026-05-28)', async () => { + // Phase 7 PR-B fold-in (commit pending): ~/.claude removed from denyRead. + // The spawn's own OAuth file MUST be readable, otherwise claude CLI fails + // with "Not logged in" — and live PI231 verification produces empty + // response bodies via the anthropic fallback path. + // + // The cross-tenant protection for ~/.claude relies on Phase 6c + // --system-prompt suppressing tool descriptions, not on sandbox denyRead. + // See manager.mjs comment block above denyRead for full rationale. + const { spawn: realSpawn } = await import('node:child_process'); + const credPath = join(homedir(), '.claude', '.credentials.json'); + + // If the credentials file isn't present (e.g. dev machine without OAuth), + // this test is meaningless — skip the assertion but log. + let credStat; + try { credStat = statSync(credPath); } catch { credStat = null; } + if (!credStat) { + // No OAuth file present; cannot test read. Pass with note. + assert.ok(true, `No ${credPath} on this host — skipping read-allowed verification`); + return; + } + + const wrapped = await wrapSpawn({ + bin: 'cat', + args: [credPath], + env: { ...process.env }, + cwd: undefined, + allowedDomains: [], + }); + + const { exitCode } = await new Promise((resolve) => { + const child = realSpawn(wrapped.bin, wrapped.args, { + env: wrapped.env, + cwd: wrapped.cwd, + stdio: ['ignore', 'pipe', 'pipe'], + }); + child.on('exit', code => resolve({ exitCode: code })); + }); + + assert.equal(exitCode, 0, + `cat ${credPath} inside sandbox exited with code ${exitCode} — sandbox is denying read on a path the spawn legitimately needs. ` + + `~/.claude must NOT be in denyRead per the 2026-05-28 fold-in.`); + }); });