diff --git a/packages/client/README.md b/packages/client/README.md index 065336d0..8a790c27 100644 --- a/packages/client/README.md +++ b/packages/client/README.md @@ -548,7 +548,7 @@ switch (outcome.reason) { #### Privilege separation: the agent must not be able to rewrite its own skills -**Run `writeSkills` as a different identity than the agent.** Reconcile as one user, run the agent as another. The reconcile sets modes explicitly rather than from the process umask: skill files and the manifest at `0644` (applied to the open file handle, so it cannot be redirected), per-skill `//` directories at `0755`, and never the execute bit. Those modes only protect anything if the two identities differ. +**Run `writeSkills` as a different identity than the agent.** Reconcile as one user, run the agent as another. The reconcile sets modes explicitly rather than from the process umask: skill files and the manifest at `0644` (applied to the open file handle, so it cannot be redirected), per-skill `//` directories at `0755` (keeping a setgid bit inherited from a setgid root, so a shared group still propagates), and never the execute bit. Those modes only protect anything if the two identities differ. **What to verify, as the identity that will run the agent.** The SDK cannot check this for you (see below), so make it a deployment step. The agent's identity must have no write access to: diff --git a/packages/client/src/__tests__/safe-fs.test.ts b/packages/client/src/__tests__/safe-fs.test.ts index 5614b25c..1dac83c2 100644 --- a/packages/client/src/__tests__/safe-fs.test.ts +++ b/packages/client/src/__tests__/safe-fs.test.ts @@ -7,7 +7,20 @@ */ import { constants as fsConstants } from 'node:fs'; -import { lstat, mkdir, mkdtemp, readdir, readFile, realpath, rename, rm, symlink, writeFile } from 'node:fs/promises'; +import { + chmod, + lstat, + mkdir, + mkdtemp, + readdir, + readFile, + realpath, + rename, + rm, + stat, + symlink, + writeFile, +} from 'node:fs/promises'; import { tmpdir } from 'node:os'; import path from 'node:path'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; @@ -73,6 +86,17 @@ describe('openOrCreateDirectory', () => { } }); + it.skipIf(process.platform === 'win32')('leaves the mode of an existing directory alone', async () => { + // The explicit 0755 is for directories this call creates; one that was + // already there keeps the mode its owner chose. + const dir = path.join(scratch, 'existing'); + await mkdir(dir); + await chmod(dir, 0o700); + const handle = await openOrCreateDirectory(dir); + await handle.close(); + expect((await stat(dir)).mode & 0o777).toBe(0o700); + }); + it('refuses an existing symlink-to-directory', async () => { // `mkdir(..., { recursive: true })` would treat this as "already there". The // plain mkdir plus an lstat on the EEXIST path is what refuses it. diff --git a/packages/client/src/__tests__/skills-fs.test.ts b/packages/client/src/__tests__/skills-fs.test.ts index 8dbec4eb..7c2a959c 100644 --- a/packages/client/src/__tests__/skills-fs.test.ts +++ b/packages/client/src/__tests__/skills-fs.test.ts @@ -18,6 +18,7 @@ import { readFile, realpath, rm, + rmdir, stat, symlink, writeFile, @@ -692,6 +693,47 @@ describe('writeSkills atomicity and permissions', () => { expect(mode & fsConstants.S_IXOTH).toBe(0); }); + it.skipIf(process.platform === 'win32')( + 'creates per-skill directories as 0755 even under a restrictive umask', + async () => { + // mkdir's mode is masked by the umask, so without an explicit chmod a 0077 + // umask leaves // at 0700 and a separate agent identity cannot + // read the SKILL.md inside it. + const previous = process.umask(0o077); + let report: Awaited>; + try { + report = await writeSkills([skill('a')], root); + } finally { + process.umask(previous); + } + expect(report.ok).toBe(true); + expect((await stat(path.join(root, 'a'))).mode & 0o777).toBe(0o755); + expect((await stat(path.join(root, 'a', SKILL_MD))).mode & 0o777).toBe(0o644); + }, + ); + + it.skipIf(process.platform === 'win32')('keeps a setgid bit the new skill directory inherited', async (ctx) => { + // Linux copies a setgid parent's bit onto a new directory so files inside + // inherit the shared group. The explicit 0755 must not clear it; fchmod + // sets exactly the bits it is given. + await chmod(root, 0o2755); + const probe = path.join(root, 'probe'); + await mkdir(probe); + const inherits = ((await stat(probe)).mode & 0o2000) !== 0; + await rmdir(probe); + if (!inherits) ctx.skip(); + + const previous = process.umask(0o077); + let report: Awaited>; + try { + report = await writeSkills([skill('a')], root); + } finally { + process.umask(previous); + } + expect(report.ok).toBe(true); + expect((await stat(path.join(root, 'a'))).mode & 0o7777).toBe(0o2755); + }); + it('goes through a single atomic rename with the temp file in the target directory', async () => { // Positive control for the interception hook. Without this, // the `calls === []` assertions in the failure tests below and in the diff --git a/packages/client/src/safe-fs.ts b/packages/client/src/safe-fs.ts index 8ef49a9a..79cb4eb0 100644 --- a/packages/client/src/safe-fs.ts +++ b/packages/client/src/safe-fs.ts @@ -31,6 +31,16 @@ import path from 'node:path'; /** Mode set explicitly on every written file: never from the umask, never executable. */ const FILE_MODE = 0o644; +/** + * Permission bits set explicitly on every directory this module creates, because `mkdir`'s + * mode argument is masked by the umask: under `0077` a separate agent identity + * could not traverse the directory to read what is inside. + */ +const DIRECTORY_MODE = 0o755; + +/** The setgid bit. Not in `fs.constants`, which exposes no special mode bits. */ +const S_ISGID = 0o2000; + /** Bound on the `O_EXCL` retry loop; with 64-bit random names it only keeps the loop finite. */ const TEMP_NAME_ATTEMPTS = 5; @@ -180,15 +190,33 @@ export async function openDirectoryNoFollow(directory: string): Promise { + let created = false; try { - await mkdir(directory, 0o755); + await mkdir(directory, DIRECTORY_MODE); + created = true; } catch (error) { if ((error as NodeJS.ErrnoException).code !== 'EEXIST') throw error; const info = await lstat(directory); if (info.isSymbolicLink()) throw new Error('the directory is a symlink'); if (!info.isDirectory()) throw new Error('the path is not a directory'); } - return openDirectoryNoFollow(directory); + const handle = await openDirectoryNoFollow(directory); + // Only a directory this call created: an existing one keeps the mode its + // owner gave it. Set on the handle, never chmod on the path, which a swap + // between mkdir and open could redirect. + if (created) { + try { + // Keep the setgid bit Linux copies from a setgid parent: fchmod sets + // exactly the bits given, so a bare 0755 would clear it and files + // written inside would stop inheriting the shared group. + const inherited = (await handle.stat()).mode & S_ISGID; + await handle.chmod(inherited | DIRECTORY_MODE); + } catch (error) { + await handle.close().catch(() => undefined); + throw error; + } + } + return handle; } /**