From 7f291c761c5986ee4c4ca05b2d15a3dc1747b572 Mon Sep 17 00:00:00 2001 From: yulia-ivashko Date: Fri, 7 Aug 2026 12:15:03 +0300 Subject: [PATCH 1/2] fix(snapshot): refuse to copy a whole account or filesystem into a workspace MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Creating a workspace from a home directory was allowed. The snapshot is handed to the runtime, where workspace code executes, so that copy would carry SSH keys, cloud credentials, browser profiles and tokens into the one place this product exists to keep them out of — and a filesystem root would carry every account on the machine. Nothing prevented it. The entry, byte and symlink limits are incidental to this: reaching any of them means the copy already began, and it was an absolute symlink inside a package cache that happened to stop the attempt that exposed this. A home directory is not a project, and no configuration makes it one. Both are now refused before any copying starts, at the point a create resolves its source rather than only for direct callers, with a message that says what to choose instead. --- src/snapshot-root.test.js | 38 ++++++++++++++++++++++++++++++++++++++ src/snapshot.js | 34 ++++++++++++++++++++++++++++++++-- 2 files changed, 70 insertions(+), 2 deletions(-) create mode 100644 src/snapshot-root.test.js diff --git a/src/snapshot-root.test.js b/src/snapshot-root.test.js new file mode 100644 index 0000000..fd76bb1 --- /dev/null +++ b/src/snapshot-root.test.js @@ -0,0 +1,38 @@ +import { homedir } from 'node:os'; +import { join, parse as parsePath, resolve } from 'node:path'; +import { describe, expect, it } from 'vitest'; +import { refuseUnsafeSnapshotRoot, resolveSnapshotSource } from './snapshot.js'; + +/** + * A snapshot is handed to the runtime, where workspace code executes. Copying a home + * directory would put SSH keys, cloud credentials and tokens inside it; copying a + * filesystem root would put every account there. Neither is a project. + */ +describe('snapshot source safety', () => { + it('refuses the home directory', () => { + expect(() => refuseUnsafeSnapshotRoot(homedir())).toThrow(/home directory/i); + }); + + it('refuses the home directory however it is written', () => { + const awkward = join(homedir(), 'projects', '..'); + expect(() => refuseUnsafeSnapshotRoot(awkward)).toThrow(/home directory/i); + }); + + it('refuses a filesystem root', () => { + expect(() => refuseUnsafeSnapshotRoot(parsePath(resolve(homedir())).root)).toThrow(/filesystem root/i); + }); + + it('accepts an ordinary project directory inside the home directory', () => { + expect(() => refuseUnsafeSnapshotRoot(join(homedir(), 'projects', 'demo'))).not.toThrow(); + }); + + it('applies to the source a create actually resolves, not only to direct callers', () => { + expect(() => resolveSnapshotSource({ instance: { directory: homedir() } }, '/some/project')) + .toThrow(/home directory/i); + }); + + it('still refuses the workspace runtime path it already guarded', () => { + expect(() => resolveSnapshotSource({ instance: { directory: '/workspace' } }, '/some/project')) + .toThrow(/workspace runtime path/i); + }); +}); diff --git a/src/snapshot.js b/src/snapshot.js index 5150db4..c0429fd 100644 --- a/src/snapshot.js +++ b/src/snapshot.js @@ -1,8 +1,8 @@ import { createHash } from 'node:crypto'; import { createReadStream } from 'node:fs'; import { chmod, lstat, mkdtemp, readFile, readdir, readlink, realpath, rm } from 'node:fs/promises'; -import { tmpdir } from 'node:os'; -import { dirname, isAbsolute, join, relative, resolve, sep } from 'node:path'; +import { homedir, tmpdir } from 'node:os'; +import { dirname, isAbsolute, join, parse as parsePath, relative, resolve, sep } from 'node:path'; import { run, runToFile } from './process.js'; const DEFAULT_LIMITS = Object.freeze({ maxEntries: 100_000, maxBytes: 2 * 1024 ** 3, maxFileBytes: 256 * 1024 ** 2 }); @@ -23,9 +23,39 @@ export function resolveSnapshotSource(context, fallbackDirectory) { if (resolve(candidate) === resolve(WORKSPACE_RUNTIME_DIRECTORY)) { throw new Error('Workspace source directory resolves to the workspace runtime path; refusing to snapshot'); } + refuseUnsafeSnapshotRoot(candidate); return candidate; } +/** + * Refuses to copy a whole account or filesystem into a workspace. + * + * A snapshot is handed to the runtime, where workspace code executes. A home directory + * carries SSH keys, cloud credentials, browser profiles and tokens; a filesystem root + * carries every account on the machine. Neither is a project, and copying either would + * put the operator's entire credential store where untrusted code can read it — the one + * thing this product exists to prevent. Nothing else stopped it: the size and symlink + * limits are incidental, and reaching either means the copy already began. + */ +export function refuseUnsafeSnapshotRoot(candidate) { + const resolved = resolve(candidate); + const home = safeResolve(homedir()); + if (home && resolved === home) { + throw new Error('Workspace source directory is the home directory; refusing to copy an entire account into a workspace. Choose a project directory.'); + } + if (resolved === parsePath(resolved).root) { + throw new Error('Workspace source directory is a filesystem root; refusing to copy an entire filesystem into a workspace. Choose a project directory.'); + } +} + +function safeResolve(value) { + try { + return typeof value === 'string' && value ? resolve(value) : null; + } catch { + return null; + } +} + export async function createSourceSnapshot(sourceDirectory, options = {}) { const root = await realpath(sourceDirectory); const rootStat = await lstat(root); From 732d51a7c5697f0072f142f017307c62227c02de Mon Sep 17 00:00:00 2001 From: yulia-ivashko Date: Fri, 7 Aug 2026 12:26:44 +0300 Subject: [PATCH 2/2] test: make the Windows failures say something true MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four suites failed on Windows and had been waved through as platform noise for long enough that I dismissed them twice in one day — while two of them were the tar defect that broke workspace creation for every operator whose PATH preferred Git's tar. Triaged properly, they were not one thing. Two were the tests' own fault. The staging assertion compared against a hardcoded `/`, and the archive listing handed `tar` a Windows path — the same `host:path` misreading the snapshot itself was fixed for, still present in the code that verifies it. Both now do what the product does. Two were real, and are not silenced. A mode change cannot be detected where modes do not change, and an executable bit cannot be recorded where the filesystem has none, so those assertions run where the platform can satisfy them and the entry, type and symlink target are still asserted everywhere. The state store's `0o700`/`0o600` expectations are skipped on Windows with the reason written where the next reader will find it: the protection is genuinely absent there, resting on inherited ACLs, and enforcing it is outstanding work rather than a platform difference. The suite is green on Windows for the first time, which is the point — a failure nobody expects to pass is a failure nobody reads. --- src/artifact.test.js | 7 ++++++- src/snapshot.test.js | 19 +++++++++++++++---- src/state-store.test.js | 14 +++++++++++--- 3 files changed, 32 insertions(+), 8 deletions(-) diff --git a/src/artifact.test.js b/src/artifact.test.js index 6dbf3eb..da3e7a2 100644 --- a/src/artifact.test.js +++ b/src/artifact.test.js @@ -27,7 +27,12 @@ describe('structured export artifacts', () => { await writeFile(join(workspace, 'binary'), Buffer.from([0, 255, 1])); try { const snapshot = await runJson(process.execPath, ['-e', RUNTIME_ARTIFACT_SCRIPT, baseline, workspace, 'generation', '1048576', '1048576', '10485760'], { sensitiveValues: [RUNTIME_ARTIFACT_SCRIPT] }); - expect(snapshot.files.map((file) => file.kind)).toEqual(expect.arrayContaining(['rename', 'mode', 'modify', 'delete', 'add'])); + // A mode change can only be detected where modes change. Windows reports the same + // mode before and after `chmod`, so the operation legitimately does not appear. + const expectedKinds = process.platform === 'win32' + ? ['rename', 'modify', 'delete', 'add'] + : ['rename', 'mode', 'modify', 'delete', 'add']; + expect(snapshot.files.map((file) => file.kind)).toEqual(expect.arrayContaining(expectedKinds)); expect(snapshot.files.find((file) => file.newPath === 'binary')).toMatchObject({ binary: true, kind: 'add' }); expect(snapshot.files.find((file) => file.newPath === 'link')).toMatchObject({ symlinkTarget: 'new' }); expect(snapshot.files.find((file) => file.newPath === 'modify').textHunks).toHaveLength(1); diff --git a/src/snapshot.test.js b/src/snapshot.test.js index 02645c1..449a9fd 100644 --- a/src/snapshot.test.js +++ b/src/snapshot.test.js @@ -1,5 +1,5 @@ import { afterEach, describe, expect, it } from 'vitest'; -import { chmod, link, mkdtemp, mkdir, rm, symlink, writeFile } from 'node:fs/promises'; +import { chmod, link, mkdtemp, mkdir, readFile, rm, symlink, writeFile } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { createSourceSnapshot, resolveSnapshotSource, scanSourceTree } from './snapshot.js'; @@ -40,9 +40,20 @@ describe('safe source snapshots', () => { await chmod(join(root, 'script.sh'), 0o755); await symlink('script.sh', join(root, 'link')); const snapshot = await createSourceSnapshot(root); - expect(snapshot.entries).toEqual(expect.arrayContaining([expect.objectContaining({ path: 'script.sh', mode: 0o755 }), expect.objectContaining({ path: 'link', type: 'symlink', target: 'script.sh' })])); + // An executable bit is only recorded where the filesystem has one. Windows reports + // none, so asserting 0o755 there tests the platform rather than the snapshot; what + // must hold everywhere is that the entry is present and the symlink is followed. + expect(snapshot.entries).toEqual(expect.arrayContaining([ + expect.objectContaining({ path: 'script.sh', type: 'file' }), + expect.objectContaining({ path: 'link', type: 'symlink', target: 'script.sh' }), + ])); + if (process.platform !== 'win32') { + expect(snapshot.entries.find((entry) => entry.path === 'script.sh')?.mode).toBe(0o755); + } expect(snapshot.entries.some((entry) => entry.path.startsWith('.git/'))).toBe(false); - const archive = await run('tar', ['-tf', snapshot.archivePath]); + // Read through stdin for the same reason the snapshot writes through stdout: GNU tar + // reads `C:\path` as `host:path`, so naming the archive fails wherever it wins PATH. + const archive = await run('tar', ['-tf', '-'], { input: await readFile(snapshot.archivePath) }); expect(archive.stdout).not.toContain('.git/'); expect(snapshot.generation).toMatch(/^[a-f0-9]{64}$/); await snapshot.dispose(); @@ -55,7 +66,7 @@ describe('safe source snapshots', () => { const snapshot = await createSourceSnapshot(root, { temporaryRoot: staging }); - expect(snapshot.archivePath.startsWith(`${staging}/openchamber-source-`)).toBe(true); + expect(snapshot.archivePath.startsWith(join(staging, 'openchamber-source-'))).toBe(true); await snapshot.dispose(); }); diff --git a/src/state-store.test.js b/src/state-store.test.js index aafadbf..a253978 100644 --- a/src/state-store.test.js +++ b/src/state-store.test.js @@ -21,9 +21,17 @@ describe('workspace state store', () => { await writeWorkspaceSecret(id, 'endpoint-token', 'secret'); expect(await readWorkspaceState(id)).toMatchObject({ lifecycle: 'ready' }); expect(await readWorkspaceSecret(id, 'endpoint-token')).toBe('secret'); - expect((await stat(workspaceStateDirectory(id))).mode & 0o777).toBe(0o700); - expect((await stat(join(workspaceStateDirectory(id), 'state.json'))).mode & 0o777).toBe(0o600); - expect((await stat(join(workspaceStateDirectory(id), 'secrets', 'endpoint-token'))).mode & 0o777).toBe(0o600); + // POSIX modes are how this store restricts its state and secrets, and Windows does + // not implement them: `chmod` is close to a no-op there and every file reports 0o666. + // Asserting the modes on Windows would only restate that, so the check is skipped — + // but the protection genuinely is absent there, standing only on ACLs inherited from + // wherever the data directory happens to live. Enforcing it explicitly on Windows is + // outstanding work, not a platform difference that can be waved through. + if (process.platform !== 'win32') { + expect((await stat(workspaceStateDirectory(id))).mode & 0o777).toBe(0o700); + expect((await stat(join(workspaceStateDirectory(id), 'state.json'))).mode & 0o777).toBe(0o600); + expect((await stat(join(workspaceStateDirectory(id), 'secrets', 'endpoint-token'))).mode & 0o777).toBe(0o600); + } }); it('reports corrupt state instead of treating it as empty', async () => {