From a42b9e94047f7c5350d03c077806da68c23b7966 Mon Sep 17 00:00:00 2001 From: Xiaoxue Sun <54162759+xiaoxueSunn@users.noreply.github.com> Date: Thu, 27 Aug 2026 17:15:23 +0800 Subject: [PATCH] fix(sandbox): expose session skill manifest read-only so `skill show` works under sandbox=true MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit sandbox=true + skillInjection=prompt made `botmux skill show ` return "skill manifest not found" for every user skill: the file sandbox never exposed `/skill-manifests/.json`, so the in-sandbox read EACCES'd and readSessionSkillManifest — which only distinguished existsSync — collapsed the permission error to null, surfaced as not-found. User-skill bodies were unreadable inside the sandbox; the manifest was fine outside it. Fix (matches the reporter's requested approach, minimal scope): - worker.ts: add ONLY this session's own manifest file to mandatoryReadOnlyPaths, canonicalized so a symlinked HOME (/home/u -> /data00/home/u) still matches. Scoped to .json (not the whole dir) so a task can't read other sessions' manifests, and built from config.session.dataDir (the exact path the store writes) rather than the possibly-unset SESSION_DATA_DIR. - manifest-store.ts: readSessionSkillManifest now returns null ONLY on ENOENT; a permission/read error throws SkillManifestReadError and corrupt JSON throws SkillManifestParseError, so a sandbox misconfig no longer masquerades as not-found. - cli-session-command.ts: map those errors to distinct, diagnosable messages (exit 1) while a genuinely-absent manifest stays not-found (exit 2). Adds regression tests: absent -> null, corrupt -> throws, unreadable -> throws; and CLI-level corrupt vs absent messaging. Co-Authored-By: Claude --- src/core/skills/cli-session-command.ts | 18 +++++++++-- src/core/skills/manifest-store.ts | 41 ++++++++++++++++++++++---- src/worker.ts | 14 +++++++++ test/skill-cli-commands.test.ts | 14 +++++++++ test/skill-manifest-store.test.ts | 39 ++++++++++++++++++++++-- 5 files changed, 117 insertions(+), 9 deletions(-) diff --git a/src/core/skills/cli-session-command.ts b/src/core/skills/cli-session-command.ts index f4366f9063..2c57d3bc0b 100644 --- a/src/core/skills/cli-session-command.ts +++ b/src/core/skills/cli-session-command.ts @@ -1,4 +1,4 @@ -import { readSessionSkillManifest } from './manifest-store.js'; +import { readSessionSkillManifest, SkillManifestReadError, SkillManifestParseError } from './manifest-store.js'; import { listSkillResources, readSkillEntrypoint, readSkillResource } from './resource-reader.js'; import { builtinSkillContent, builtinSkillEntries } from '../../skills/injection-mode.js'; import { whiteboardEnabled } from '../../services/whiteboard-store.js'; @@ -37,7 +37,21 @@ export function runSkillSessionCommand( } const sessionId = sessionIdFromEnv(env); if (!sessionId) return { code: 2, stdout: '', stderr: 'missing BOTMUX_SESSION_ID\n' }; - const manifest = readSessionSkillManifest(sessionId); + let manifest; + try { + manifest = readSessionSkillManifest(sessionId); + } catch (err) { + // Present-but-unreadable (sandbox/policy) or corrupt manifest — a real + // fault, NOT "not found". Distinct message + exit 1 so the sandbox misconfig + // is diagnosable instead of masquerading as an absent manifest (exit 2). + if (err instanceof SkillManifestReadError) { + return { code: 1, stdout: '', stderr: `skill manifest exists but is unreadable for session ${sessionId} (${(err.cause as any)?.code ?? 'read error'}) — check the file sandbox read-only policy: ${err.path}\n` }; + } + if (err instanceof SkillManifestParseError) { + return { code: 1, stdout: '', stderr: `skill manifest is corrupt for session ${sessionId}: ${err.path}\n` }; + } + throw err; + } if (!manifest) { // No user-skill manifest — still surface built-ins for discovery. if (sub === 'list') { diff --git a/src/core/skills/manifest-store.ts b/src/core/skills/manifest-store.ts index 730a002557..01a7fa9c48 100644 --- a/src/core/skills/manifest-store.ts +++ b/src/core/skills/manifest-store.ts @@ -1,9 +1,27 @@ -import { existsSync, mkdirSync, readFileSync, rmSync } from 'node:fs'; +import { mkdirSync, readFileSync, rmSync } from 'node:fs'; import { join } from 'node:path'; import { config } from '../../config.js'; import { atomicWriteFileSync } from '../../utils/atomic-write.js'; import type { SessionSkillManifest } from './types.js'; +/** Manifest file exists but could not be READ (permission denied, I/O error). + * Distinct from "absent" so callers can diagnose a sandbox/policy misconfig + * instead of reporting a misleading "not found". */ +export class SkillManifestReadError extends Error { + constructor(public readonly path: string, public readonly cause: any) { + super(`skill manifest unreadable (${cause?.code ?? 'read error'}): ${path}`); + this.name = 'SkillManifestReadError'; + } +} + +/** Manifest file was read but is not valid JSON. */ +export class SkillManifestParseError extends Error { + constructor(public readonly path: string, public readonly cause: any) { + super(`skill manifest is corrupt (invalid JSON): ${path}`); + this.name = 'SkillManifestParseError'; + } +} + function manifestDir(): string { return join(config.session.dataDir, 'skill-manifests'); } @@ -19,11 +37,24 @@ export function writeSessionSkillManifest(manifest: SessionSkillManifest): void export function readSessionSkillManifest(sessionId: string): SessionSkillManifest | null { const file = manifestPath(sessionId); - if (!existsSync(file)) return null; + let raw: string; + try { + raw = readFileSync(file, 'utf-8'); + } catch (err: any) { + // ENOENT is the only "genuinely absent" case → null (callers report + // not-found). A permission error (EACCES/EPERM — e.g. the file sandbox + // never exposed this session's manifest) or any other read failure is a + // real fault: surface it so `skill show` can tell "no manifest" apart from + // "manifest unreadable", instead of masking a sandbox misconfig as + // not-found (the historical failure mode). + if (err?.code === 'ENOENT') return null; + throw new SkillManifestReadError(file, err); + } try { - return JSON.parse(readFileSync(file, 'utf-8')) as SessionSkillManifest; - } catch { - return null; + return JSON.parse(raw) as SessionSkillManifest; + } catch (err: any) { + // File is present and readable but corrupt — a distinct, diagnosable fault. + throw new SkillManifestParseError(file, err); } } diff --git a/src/worker.ts b/src/worker.ts index e289e721dd..35071acde3 100644 --- a/src/worker.ts +++ b/src/worker.ts @@ -12903,6 +12903,20 @@ async function spawnCli( config.session.dataDir, ).map(canonical)); } + // The sandboxed CLI reads its OWN session's skill manifest via + // `botmux skill show ` (manifest-store.ts writes it to + // `/skill-manifests/.json`). Without an + // explicit carve-out the file sandbox never exposes it, so the read EACCES's + // and `skill show` maps that to "manifest not found" — user-skill bodies are + // unreadable under sandbox=true + skillInjection=prompt. Expose ONLY this + // session's own manifest file, read-only, canonicalized so a symlinked HOME + // (`/home/u` → `/data00/home/u`) still matches. Scoping to the single + // `.json` (not the whole skill-manifests dir) keeps a task from + // reading other sessions' manifests. Built from config.session.dataDir — the + // exact path the store writes to, not the possibly-unset SESSION_DATA_DIR. + mandatoryReadOnlyPaths.push( + canonical(join(config.session.dataDir, 'skill-manifests', `${cfg.sessionId}.json`)), + ); if (process.platform === 'darwin') { const gatewaySocketRoot = canonical( sessionMcpGatewayHost ? dirname(sessionMcpGatewayHost.socketDir) : tmpdir(), diff --git a/test/skill-cli-commands.test.ts b/test/skill-cli-commands.test.ts index 8c48181888..ada68ac8be 100644 --- a/test/skill-cli-commands.test.ts +++ b/test/skill-cli-commands.test.ts @@ -62,4 +62,18 @@ describe('botmux skill session command', () => { it('refuses to run without a session id', () => { expect(runSkillSessionCommand(['list'], {}).stderr).toContain('missing BOTMUX_SESSION_ID'); }); + + it('reports a corrupt manifest as unreadable, not not-found', () => { + write(join(dataDir, 'skill-manifests', 'broken.json'), '{ nope'); + const res = runSkillSessionCommand(['show', 'deploy'], { BOTMUX_SESSION_ID: 'broken' }); + expect(res.code).toBe(1); + expect(res.stderr).toContain('corrupt'); + expect(res.stderr).not.toContain('not found'); + }); + + it('still reports a genuinely absent manifest as not found', () => { + const res = runSkillSessionCommand(['show', 'deploy'], { BOTMUX_SESSION_ID: 'absent' }); + expect(res.code).toBe(2); + expect(res.stderr).toContain('manifest not found'); + }); }); diff --git a/test/skill-manifest-store.test.ts b/test/skill-manifest-store.test.ts index 58bbc5de91..b82e2854bf 100644 --- a/test/skill-manifest-store.test.ts +++ b/test/skill-manifest-store.test.ts @@ -1,9 +1,14 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; -import { mkdtempSync, rmSync } from 'node:fs'; +import { mkdtempSync, rmSync, writeFileSync, mkdirSync, chmodSync } from 'node:fs'; import { join } from 'node:path'; import { tmpdir } from 'node:os'; -import { readSessionSkillManifest, writeSessionSkillManifest } from '../src/core/skills/manifest-store.js'; +import { + readSessionSkillManifest, + writeSessionSkillManifest, + SkillManifestReadError, + SkillManifestParseError, +} from '../src/core/skills/manifest-store.js'; import type { SessionSkillManifest } from '../src/core/skills/types.js'; describe('session skill manifest store', () => { @@ -34,4 +39,34 @@ describe('session skill manifest store', () => { expect(readSessionSkillManifest('s1')).toEqual(manifest); }); + + it('returns null only when the manifest is genuinely absent (ENOENT)', () => { + expect(readSessionSkillManifest('never-written')).toBeNull(); + }); + + it('throws a corrupt-JSON error instead of masking it as not-found', () => { + mkdirSync(join(dataDir, 'skill-manifests'), { recursive: true }); + writeFileSync(join(dataDir, 'skill-manifests', 'bad.json'), '{ not valid json'); + expect(() => readSessionSkillManifest('bad')).toThrow(SkillManifestParseError); + }); + + it('throws a read error (not null) when the manifest is present but unreadable', () => { + // A permission-denied read (the sandbox never exposed this session's + // manifest) must NOT collapse to "not found" — regression for the + // sandbox=true skill-body-unreadable bug. + mkdirSync(join(dataDir, 'skill-manifests'), { recursive: true }); + const file = join(dataDir, 'skill-manifests', 'locked.json'); + writeFileSync(file, '{}'); + chmodSync(file, 0o000); + try { + // root ignores mode bits — skip the assertion there rather than false-fail. + let readable = true; + try { readSessionSkillManifest('locked'); } catch { readable = false; } + if (typeof process.getuid === 'function' && process.getuid() === 0) return; + expect(readable).toBe(false); + expect(() => readSessionSkillManifest('locked')).toThrow(SkillManifestReadError); + } finally { + chmodSync(file, 0o600); + } + }); });