From 4650240c1ea629fe237ed50f62a076e88155cf61 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Mon, 7 Sep 2026 16:03:53 -0400 Subject: [PATCH 1/8] fix: preserve required native sandbox environment names (#129) --- packages/code/README.md | 6 ++ packages/code/src/native-sandbox.test.ts | 95 ++++++++++++++++++++++++ packages/code/src/native-sandbox.ts | 39 +++++++++- 3 files changed, 137 insertions(+), 3 deletions(-) diff --git a/packages/code/README.md b/packages/code/README.md index 54284c56..b0bcad80 100644 --- a/packages/code/README.md +++ b/packages/code/README.md @@ -86,6 +86,12 @@ policy: an allowed destination can receive workspace data. The normalized allowlist is included in the worker policy digest. Tool approval hooks remain the user-facing allow/deny boundary for each invocation. +The native sandbox preserves standard `HTTP_PROXY`, `HTTPS_PROXY`, `ALL_PROXY`, +and `NO_PROXY` names (including lowercase forms), plus Windows process and profile +variables on Windows. SRT remains responsible for the final sandbox environment +and can replace proxy values with its filtered proxy endpoints. This does not +expand the allowed domains or expose unrelated inherited credentials. + ### GitHub authentication The native BYOM worker can provide Git HTTPS authentication without exposing a diff --git a/packages/code/src/native-sandbox.test.ts b/packages/code/src/native-sandbox.test.ts index 50f8e5c3..d344cabf 100644 --- a/packages/code/src/native-sandbox.test.ts +++ b/packages/code/src/native-sandbox.test.ts @@ -34,6 +34,7 @@ function fakeManager( beforeWrap?: () => Promise; appendGitSafeDirectory?: boolean; inheritedGitEnvironment?: Record; + wrappedEnvironment?: NodeJS.ProcessEnv; } = {}, ) { let config: SandboxRuntimeConfig | undefined; @@ -72,6 +73,7 @@ function fakeManager( env: { PATH: process.env.PATH, ...gitEnvironment, + ...options.wrappedEnvironment, ...(credentialSeenDuringWrap ? { LIBRECHAT_CODE_TEST_CREDENTIAL: @@ -148,6 +150,99 @@ test('initializes SRT with a default-deny network and scrubbed worker credential assert.equal(fake.reset, true); }); +const proxyEnvironment = { + HTTP_PROXY: 'http://upstream.invalid:8080', + HTTPS_PROXY: 'http://upstream.invalid:8080', + ALL_PROXY: 'socks5://upstream.invalid:1080', + NO_PROXY: 'upstream.internal', + http_proxy: 'http://upstream.invalid:8080', + https_proxy: 'http://upstream.invalid:8080', + all_proxy: 'socks5://upstream.invalid:1080', + no_proxy: 'upstream.internal', +}; +const windowsEnvironment = { + SYSTEMROOT: 'C:\\Windows', + SystemRoot: 'C:\\Windows', + SYSTEMDRIVE: 'C:', + windir: 'C:\\Windows', + ComSpec: 'C:\\Windows\\System32\\cmd.exe', + PATHEXT: '.COM;.EXE;.BAT;.CMD', + TEMP: 'C:\\Temp', + Temp: 'C:\\Temp', + TMP: 'C:\\Temp', + USERPROFILE: 'C:\\Users\\sandbox', + HOMEDRIVE: 'C:', + HOMEPATH: '\\Users\\sandbox', + APPDATA: 'C:\\Users\\sandbox\\AppData\\Roaming', + LOCALAPPDATA: 'C:\\Users\\sandbox\\AppData\\Local', +}; + +for (const platform of ['darwin', 'linux', 'win32'] as const) { + test(`preserves required ${platform} environment names without allowing credentials`, async (t) => { + const root = await mkdtemp(join(tmpdir(), 'librechat-code-native-')); + t.after(() => rm(root, { recursive: true, force: true })); + const fake = fakeManager(); + const credentials = { + LIBRECHAT_CODE_WORKER_TOKEN: 'worker-secret', + LIBRECHAT_CODE_HTTP_PROXY: 'worker-secret', + AWS_SECRET_ACCESS_KEY: 'aws-secret', + GITHUB_TOKEN: 'github-secret', + HTTP_PROXY_TOKEN: 'proxy-secret', + CUSTOM_PROXY: 'proxy-secret', + SYSTEMROOT_TOKEN: 'runtime-secret', + NODE_OPTIONS: '--require /host/private.js', + LD_PRELOAD: '/host/private.so', + }; + const sandbox = new NativeSrtWorkspaceCommandSandbox({ + workspaceRoot: root, platform, allowedDomains: ['github.com'], + environment: { + ...proxyEnvironment, ...windowsEnvironment, ...credentials, + HtTp_PrOxY: 'http://mixed-case.invalid:8080', + PATH: '/usr/bin', LC_ALL: 'C.UTF-8', + }, + manager: fake.manager, + }); + t.after(() => sandbox.close()); + await sandbox.prepare(); + const denied = new Set(fake.config?.credentials?.envVars + ?.filter(({ mode }) => mode === 'deny').map(({ name }) => name)); + for (const name of [...Object.keys(proxyEnvironment), 'PATH', 'LC_ALL']) { + assert.equal(denied.has(name), false, `${name} must remain available`); + } + for (const name of Object.keys(windowsEnvironment)) { + assert.equal(denied.has(name), platform !== 'win32', `${name} must be platform-specific`); + } + assert.equal(denied.has('HtTp_PrOxY'), platform !== 'win32'); + for (const name of Object.keys(credentials)) { + assert.equal(denied.has(name), true, `${name} must remain denied`); + } + assert.deepEqual(fake.config?.network.allowedDomains, ['github.com']); + assert.equal(fake.config?.network.strictAllowlist, true); + }); +} + +test('uses SRT proxy values without restoring inherited proxies or credentials', async (t) => { + const root = await mkdtemp(join(tmpdir(), 'librechat-code-native-')); + t.after(() => rm(root, { recursive: true, force: true })); + const wrappedEnvironment = { + HTTP_PROXY: 'http://localhost:3128', HTTPS_PROXY: 'http://localhost:3128', + ALL_PROXY: 'http://localhost:3128', NO_PROXY: 'localhost', + }; + const fake = fakeManager({ wrappedEnvironment }); + const sandbox = new NativeSrtWorkspaceCommandSandbox({ + workspaceRoot: root, + environment: { ...proxyEnvironment, GITHUB_TOKEN: 'host-secret' }, + manager: fake.manager, + }); + t.after(() => sandbox.close()); + const result = await sandbox.execute({ + ...request, maxOutputBytes: 256, + command: 'printf "%s|%s|%s|%s|%s" "$HTTP_PROXY" "$HTTPS_PROXY" "$ALL_PROXY" "$NO_PROXY" "${GITHUB_TOKEN-unset}"', + }); + assert.equal(result.exitCode, 0); + assert.equal(result.stdout, `${Object.values(wrappedEnvironment).join('|')}|unset`); +}); + test('masks a host credential for only its injection host and restores the parent environment', async (t) => { const root = await mkdtemp(join(tmpdir(), 'librechat-code-native-')); t.after(() => rm(root, { recursive: true, force: true })); diff --git a/packages/code/src/native-sandbox.ts b/packages/code/src/native-sandbox.ts index 1b1d96cb..716072a6 100644 --- a/packages/code/src/native-sandbox.ts +++ b/packages/code/src/native-sandbox.ts @@ -48,6 +48,36 @@ const SAFE_CHILD_ENV_NAMES = new Set([ 'USER', ]); +// Preserve the conventional proxy names, not arbitrary *_PROXY variables. +// SRT owns their final values and may replace them with its filtered proxy. +const PROXY_CHILD_ENV_NAMES = new Set([ + 'HTTP_PROXY', + 'HTTPS_PROXY', + 'ALL_PROXY', + 'NO_PROXY', + 'http_proxy', + 'https_proxy', + 'all_proxy', + 'no_proxy', +]); + +// Windows resolves these names case-insensitively. Keep the exception +// platform-specific so similarly named POSIX variables remain denied. +const WINDOWS_CHILD_ENV_NAMES = new Set([ + 'SYSTEMROOT', + 'SYSTEMDRIVE', + 'WINDIR', + 'COMSPEC', + 'PATHEXT', + 'TEMP', + 'TMP', + 'USERPROFILE', + 'HOMEDRIVE', + 'HOMEPATH', + 'APPDATA', + 'LOCALAPPDATA', +]); + let hostEnvironmentMutationQueue: Promise = Promise.resolve(); const TRUSTED_GIT_ENVIRONMENT = { @@ -148,7 +178,7 @@ function boundedUtf8(buffer: Buffer, budget: number): string { return ''; } -function safeEnvironmentNames( +function deniedEnvironmentNames( environment: NodeJS.ProcessEnv, platform: NodeJS.Platform, ): string[] { @@ -157,7 +187,10 @@ function safeEnvironmentNames( const normalized = platform === 'win32' ? name.toUpperCase() : name; return ( normalized.startsWith('LIBRECHAT_CODE_') || - (!SAFE_CHILD_ENV_NAMES.has(normalized) && !normalized.startsWith('LC_')) + (!SAFE_CHILD_ENV_NAMES.has(normalized) && + !PROXY_CHILD_ENV_NAMES.has(normalized) && + !(platform === 'win32' && WINDOWS_CHILD_ENV_NAMES.has(normalized)) && + !normalized.startsWith('LC_')) ); }) .sort(); @@ -272,7 +305,7 @@ export class NativeSrtWorkspaceCommandSandbox implements WorkspaceCommandSandbox mode: 'deny' as const, })), envVars: [ - ...safeEnvironmentNames(this.environment, this.platform) + ...deniedEnvironmentNames(this.environment, this.platform) .filter((name) => { const normalized = normalizedEnvironmentName( name, From 70e8d53253b529339815fc1ec065ed3291dace84 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Mon, 7 Sep 2026 16:04:10 -0400 Subject: [PATCH 2/8] fix: clean sandbox state on pre-spawn exits (#130) --- packages/code/src/native-sandbox.test.ts | 97 ++++++++++++++++++++++++ packages/code/src/native-sandbox.ts | 26 ++++--- 2 files changed, 112 insertions(+), 11 deletions(-) diff --git a/packages/code/src/native-sandbox.test.ts b/packages/code/src/native-sandbox.test.ts index d344cabf..5ee411fb 100644 --- a/packages/code/src/native-sandbox.test.ts +++ b/packages/code/src/native-sandbox.test.ts @@ -647,3 +647,100 @@ test('maps platform-native exit statuses into the bridge protocol range', async const result = await sandbox.execute(request); assert.equal(result.exitCode, 1); }); + +test('cleans allocated command state exactly once on every execution exit', async (t) => { + for (const outcome of [ + 'abort-before-spawn', + 'spawn-throw', + 'close', + 'error', + 'abort-after-spawn', + 'timeout', + 'wrap-throw', + ] as const) { + for (const cleanupThrows of [false, true]) { + await t.test(`${outcome}, cleanup throws: ${cleanupThrows}`, async (t) => { + const root = await mkdtemp(join(tmpdir(), 'librechat-code-native-')); + t.after(() => rm(root, { recursive: true, force: true })); + const controller = new AbortController(); + let cleanupCalls = 0; + let spawnCalls = 0; + let allocated = false; + const fake = fakeManager({ + async beforeWrap() { + if (outcome === 'wrap-throw') throw new Error('wrap failed'); + allocated = true; + if (outcome === 'abort-before-spawn') controller.abort(); + }, + }); + fake.manager.cleanupAfterCommand = () => { + cleanupCalls += 1; + assert.equal(allocated, true); + allocated = false; + if (cleanupThrows) throw new Error('cleanup failed'); + }; + const sandbox = new NativeSrtWorkspaceCommandSandbox({ + workspaceRoot: root, + manager: fake.manager, + spawnCommand() { + spawnCalls += 1; + assert.equal(allocated, true); + if (outcome === 'spawn-throw') throw new Error('spawn failed'); + const child = new EventEmitter() as ChildProcessWithoutNullStreams; + let closeQueued = false; + const close = () => { + if (!closeQueued) { + closeQueued = true; + queueMicrotask(() => child.emit('close', null, 'SIGKILL')); + } + return true; + }; + Object.assign(child, { + stdin: new PassThrough(), + stdout: new PassThrough(), + stderr: new PassThrough(), + pid: undefined, + kill: close, + }); + queueMicrotask(() => { + assert.equal(cleanupCalls, 0); + if (outcome === 'error') { + child.emit('error', new Error('spawn failed')); + } else if (outcome === 'abort-after-spawn') { + controller.abort(); + } else if (outcome === 'close') { + child.emit('close', 0, null); + } + }); + return child; + }, + }); + const execution = sandbox.execute( + { ...request, timeoutMs: 10 }, + controller.signal, + ); + if (outcome === 'close' || outcome === 'timeout') { + const result = await execution; + assert.equal(result.exitCode, outcome === 'close' ? 0 : null); + assert.equal(result.timedOut, outcome === 'timeout'); + } else { + await assert.rejects(execution, (error: unknown) => + error instanceof WorkspaceToolError && + error.code === (outcome.startsWith('abort') + ? 'EXECUTION_ABORTED' + : 'COMMAND_UNAVAILABLE') && + error.mutationMayHaveCommitted === (outcome === 'abort-after-spawn'), + ); + } + assert.equal( + spawnCalls, + outcome === 'abort-before-spawn' || outcome === 'wrap-throw' ? 0 : 1, + ); + assert.equal(cleanupCalls, outcome === 'wrap-throw' ? 0 : 1); + assert.equal(allocated, false); + await sandbox.close(); + assert.equal(fake.reset, true); + }); + } + } +}); diff --git a/packages/code/src/native-sandbox.ts b/packages/code/src/native-sandbox.ts index 716072a6..9743ad76 100644 --- a/packages/code/src/native-sandbox.ts +++ b/packages/code/src/native-sandbox.ts @@ -413,13 +413,22 @@ export class NativeSrtWorkspaceCommandSandbox implements WorkspaceCommandSandbox 'COMMAND_UNAVAILABLE', ); } - if (signal?.aborted) { - throw new WorkspaceToolError( - 'Workspace command execution aborted', - 'EXECUTION_ABORTED', - ); + try { + if (signal?.aborted) { + throw new WorkspaceToolError( + 'Workspace command execution aborted', + 'EXECUTION_ABORTED', + ); + } + return await this.runWrapped(request, wrapped, cwd, commandId, signal); + } finally { + // A successful wrap owns command state even when no child is spawned. + try { + this.manager.cleanupAfterCommand(); + } catch { + // Cleanup is retried by close(); command settlement must still finish. + } } - return await this.runWrapped(request, wrapped, cwd, commandId, signal); } private async withTemporaryHostEnvironment( @@ -521,11 +530,6 @@ export class NativeSrtWorkspaceCommandSandbox implements WorkspaceCommandSandbox const cleanup = (): void => { clearTimeout(timer); signal?.removeEventListener('abort', abort); - try { - this.manager.cleanupAfterCommand(); - } catch { - // Cleanup is retried by close(); command settlement must still finish. - } }; child.once('error', () => { if (settled) return; From 6365863b200c3be8b313a6f727fa3b1c50680a63 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Mon, 7 Sep 2026 16:04:25 -0400 Subject: [PATCH 3/8] fix: continue bounded listings after skipped candidate windows (#132) --- packages/code/src/workspace-listing.test.ts | 133 +++++++ packages/code/src/workspace.ts | 374 ++++++++++---------- 2 files changed, 327 insertions(+), 180 deletions(-) create mode 100644 packages/code/src/workspace-listing.test.ts diff --git a/packages/code/src/workspace-listing.test.ts b/packages/code/src/workspace-listing.test.ts new file mode 100644 index 00000000..90845b02 --- /dev/null +++ b/packages/code/src/workspace-listing.test.ts @@ -0,0 +1,133 @@ +import assert from 'node:assert/strict'; +import childProcess from 'node:child_process'; +import { EventEmitter } from 'node:events'; +import { mkdtemp, rm, symlink, writeFile } from 'node:fs/promises'; +import { syncBuiltinESMExports } from 'node:module'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { PassThrough } from 'node:stream'; +import test from 'node:test'; + +import type { TestContext } from 'node:test'; +import type { ChildProcess } from 'node:child_process'; +import { BRIDGE_WORKSPACE_LIST_MAX_RESULTS } from './protocol.js'; +import { isWorkspaceToolResult, LocalWorkspaceTools, WorkspaceToolError } from './workspace.js'; + +const request = { + protocolVersion: 1 as const, + operation: 'list_files' as const, + workspaceId: 'primary', + maxResults: 1, +}; +const skippedPaths = Array.from( + { length: 2 * (BRIDGE_WORKSPACE_LIST_MAX_RESULTS + request.maxResults) + 5 }, + (_, index) => `a-${String(index).padStart(4, '0')}`, +); + +// Control candidate discovery independently of filesystem verification, as +// files can vanish or be replaced by symlinks after rg has enumerated them. +function candidateSource(t: TestContext, paths: string[], onScan?: (scan: number) => void) { + let scans = 0; + let cappedScans = 0; + t.mock.method(childProcess, 'spawn', (command: string, args: string[]) => { + assert.equal(command, 'rg'); + assert.ok(args.includes('--no-follow')); + assert.ok(args.includes('--null')); + assert.ok(args.includes('--sort')); + scans += 1; + const child = new EventEmitter() as ChildProcess; + const stdout = new PassThrough(); + let closed = false; + const close = () => { + if (closed) return; + closed = true; + queueMicrotask(() => child.emit('close', 0)); + }; + Object.assign(child, { + stdout, + kill: () => { cappedScans += 1; close(); return true; }, + }); + onScan?.(scans); + queueMicrotask(() => { + if (closed) return; + stdout.end(Buffer.from(`${paths.join('\0')}\0`)); + close(); + }); + return child; + }); + syncBuiltinESMExports(); + t.after(() => { + t.mock.restoreAll(); + syncBuiltinESMExports(); + }); + return { get scans() { return scans; }, get cappedScans() { return cappedScans; } }; +} + +async function workspace(t: TestContext) { + const root = await mkdtemp(join(tmpdir(), 'librechat-code-list-windows-')); + t.after(() => rm(root, { recursive: true, force: true })); + const tools = await LocalWorkspaceTools.create({ workspaces: [{ id: 'primary', root }] }); + return { root, tools }; +} + +for (const skippedKind of ['missing', 'symlink'] as const) { + test(`lists and paginates valid files after multiple windows of ${skippedKind} candidates`, async (t) => { + const { root, tools } = await workspace(t); + await writeFile(join(root, 'z-first.txt'), 'first'); + await writeFile(join(root, 'z-last.txt'), 'last'); + if (skippedKind === 'symlink') { + await Promise.all(skippedPaths.map(path => symlink('z-first.txt', join(root, path)))); + } + const source = candidateSource(t, [...skippedPaths, 'z-first.txt', 'z-last.txt']); + const first = await tools.execute(request); + assert.equal(isWorkspaceToolResult(request, first, tools.capabilities), true); + assert.deepEqual(first, { + protocolVersion: 1, operation: 'list_files', workspaceId: 'primary', + paths: ['z-first.txt'], truncated: true, nextAfterPath: 'z-first.txt', + }); + assert.equal(source.scans, 3); + assert.equal(source.cappedScans, 2, 'each full candidate window still stops rg'); + const nextRequest = { ...request, afterPath: 'z-first.txt' }; + const last = await tools.execute(nextRequest); + assert.equal(isWorkspaceToolResult(nextRequest, last, tools.capabilities), true); + assert.deepEqual(last, { + protocolVersion: 1, operation: 'list_files', workspaceId: 'primary', + paths: ['z-last.txt'], truncated: false, + }); + assert.equal(source.scans, 4); + }); +} + +test('returns a complete empty page when successive skipped windows exhaust the listing', async (t) => { + const { tools } = await workspace(t); + const source = candidateSource(t, skippedPaths); + const result = await tools.execute(request); + assert.equal(isWorkspaceToolResult(request, result, tools.capabilities), true); + assert.deepEqual(result, { + protocolVersion: 1, operation: 'list_files', workspaceId: 'primary', + paths: [], truncated: false, + }); + assert.equal(source.scans, 3); + assert.equal(source.cappedScans, 2); +}); + +test('successive candidate windows share the original listing deadline', async (t) => { + const { tools } = await workspace(t); + let now = Date.now(); + t.mock.method(Date, 'now', () => now); + const source = candidateSource(t, skippedPaths, () => { now += 6_000; }); + await assert.rejects(tools.execute(request), (error: unknown) => + error instanceof WorkspaceToolError && error.code === 'LIST_TIMEOUT'); + assert.equal(source.scans, 2, 'a later window must not reset the ten-second budget'); +}); + +test('cancellation interrupts a later candidate window', async (t) => { + const { tools } = await workspace(t); + const controller = new AbortController(); + const source = candidateSource(t, skippedPaths, scan => { + if (scan === 2) controller.abort(); + }); + await assert.rejects(tools.execute(request, controller.signal), (error: unknown) => + error instanceof WorkspaceToolError && error.code === 'EXECUTION_ABORTED'); + assert.equal(source.scans, 2); +}); diff --git a/packages/code/src/workspace.ts b/packages/code/src/workspace.ts index 5302593a..f7770e76 100644 --- a/packages/code/src/workspace.ts +++ b/packages/code/src/workspace.ts @@ -1157,202 +1157,213 @@ async function listWorkspaceFiles( .filter((segment) => segment.length > 0 && segment !== '.') .join('/'); const requestedResultPath = normalizedRequestedResultPath || undefined; - const afterPath = request.afterPath; + let afterPath = request.afterPath; - const candidates: Array<{ filesystemPath: string; resultPath: string }> = []; - let truncated = false; - let pending: Buffer = Buffer.alloc(0); - let stoppedForLimit = false; - await new Promise((resolvePromise, reject) => { - const args = [ - '--files', - '--no-config', - '--no-follow', - '--no-messages', - '--sort', - 'path', - '--null', - ]; - if (portableCanonicalListPath !== '.') { - args.push( - '--glob', - canonicalTargetIsDirectory - ? `${portableCanonicalListPath}/**` - : portableCanonicalListPath, - ); - } - args.push('--', '.'); - const child = spawn( - 'rg', - args, - { cwd: root, stdio: ['ignore', 'pipe', 'ignore'] }, - ); - let aborted = false; - let timedOut = false; - const abort = () => { - aborted = true; - child.kill(); - }; - signal?.addEventListener('abort', abort, { once: true }); - if (signal?.aborted) abort(); - const timeout = setTimeout(() => { - timedOut = true; - child.kill(); - }, Math.max(0, deadline - Date.now())); - const cleanup = () => { - clearTimeout(timeout); - signal?.removeEventListener('abort', abort); - }; - const pathDecoder = new TextDecoder('utf-8', { - fatal: true, - ignoreBOM: true, - }); - const consumePath = (rawPath: Buffer) => { - if (rawPath.length === 0 || stoppedForLimit) return; - let path: string; - try { - path = pathDecoder.decode(rawPath); - } catch { - return; + for (;;) { + await withinListDeadline(Promise.resolve(), signal, deadline); + const candidates: Array<{ filesystemPath: string; resultPath: string }> = []; + let truncated = false; + let pending: Buffer = Buffer.alloc(0); + let stoppedForLimit = false; + await new Promise((resolvePromise, reject) => { + const args = [ + '--files', + '--no-config', + '--no-follow', + '--no-messages', + '--sort', + 'path', + '--null', + ]; + if (portableCanonicalListPath !== '.') { + args.push( + '--glob', + canonicalTargetIsDirectory + ? `${portableCanonicalListPath}/**` + : portableCanonicalListPath, + ); } - if (!Buffer.from(path).equals(rawPath)) return; - if (candidates.length === maxResults + BRIDGE_WORKSPACE_LIST_MAX_RESULTS) { - truncated = true; - stoppedForLimit = true; + args.push('--', '.'); + const child = spawn( + 'rg', + args, + { cwd: root, stdio: ['ignore', 'pipe', 'ignore'] }, + ); + let aborted = false; + let timedOut = false; + const abort = () => { + aborted = true; child.kill(); - return; - } - const portablePath = sep === '\\' ? path.split(sep).join('/') : path; - const normalizedPath = portablePath.startsWith('./') - ? portablePath.slice(2) - : portablePath; - const resultPath = - requestedResultPath == null - ? normalizedPath - : portableCanonicalListPath === '.' - ? `${requestedResultPath}/${normalizedPath}` - : normalizedPath === portableCanonicalListPath || - normalizedPath.startsWith(`${portableCanonicalListPath}/`) - ? `${requestedResultPath}${normalizedPath.slice(portableCanonicalListPath.length)}` - : normalizedPath; - if (!isSafePortableRelativePath(resultPath)) return; - if ( - afterPath !== undefined && - comparePortableRelativePaths(resultPath, afterPath) <= 0 - ) { - return; - } - candidates.push({ filesystemPath: normalizedPath, resultPath }); - }; + }; + signal?.addEventListener('abort', abort, { once: true }); + if (signal?.aborted) abort(); + const timeout = setTimeout(() => { + timedOut = true; + child.kill(); + }, Math.max(0, deadline - Date.now())); + const cleanup = () => { + clearTimeout(timeout); + signal?.removeEventListener('abort', abort); + }; + const pathDecoder = new TextDecoder('utf-8', { + fatal: true, + ignoreBOM: true, + }); + const consumePath = (rawPath: Buffer) => { + if (rawPath.length === 0 || stoppedForLimit) return; + let path: string; + try { + path = pathDecoder.decode(rawPath); + } catch { + return; + } + if (!Buffer.from(path).equals(rawPath)) return; + if (candidates.length === maxResults + BRIDGE_WORKSPACE_LIST_MAX_RESULTS) { + truncated = true; + stoppedForLimit = true; + child.kill(); + return; + } + const portablePath = sep === '\\' ? path.split(sep).join('/') : path; + const normalizedPath = portablePath.startsWith('./') + ? portablePath.slice(2) + : portablePath; + const resultPath = + requestedResultPath == null + ? normalizedPath + : portableCanonicalListPath === '.' + ? `${requestedResultPath}/${normalizedPath}` + : normalizedPath === portableCanonicalListPath || + normalizedPath.startsWith(`${portableCanonicalListPath}/`) + ? `${requestedResultPath}${normalizedPath.slice(portableCanonicalListPath.length)}` + : normalizedPath; + if (!isSafePortableRelativePath(resultPath)) return; + if ( + afterPath !== undefined && + comparePortableRelativePaths(resultPath, afterPath) <= 0 + ) { + return; + } + candidates.push({ filesystemPath: normalizedPath, resultPath }); + }; - child.stdout.on('data', (chunk: Buffer) => { - pending = pending.length === 0 ? chunk : Buffer.concat([pending, chunk]); - let delimiter = pending.indexOf(0); - while (delimiter >= 0) { - consumePath(pending.subarray(0, delimiter)); - pending = pending.subarray(delimiter + 1); - delimiter = pending.indexOf(0); - } - }); - child.once('error', () => { - cleanup(); - reject( - new WorkspaceToolError( - 'Workspace listing unavailable', - 'LIST_UNAVAILABLE', - ), - ); - }); - child.once('close', (code) => { - cleanup(); - consumePath(pending); - if (aborted) { - reject( - new WorkspaceToolError( - 'Workspace tool execution aborted', - 'EXECUTION_ABORTED', - ), - ); - } else if (timedOut) { - reject( - new WorkspaceToolError('Workspace listing timed out', 'LIST_TIMEOUT'), - ); - } else if (stoppedForLimit || code === 0 || code === 1) { - resolvePromise(); - } else { + child.stdout.on('data', (chunk: Buffer) => { + pending = pending.length === 0 ? chunk : Buffer.concat([pending, chunk]); + let delimiter = pending.indexOf(0); + while (delimiter >= 0) { + consumePath(pending.subarray(0, delimiter)); + pending = pending.subarray(delimiter + 1); + delimiter = pending.indexOf(0); + } + }); + child.once('error', () => { + cleanup(); reject( new WorkspaceToolError( 'Workspace listing unavailable', 'LIST_UNAVAILABLE', ), ); - } + }); + child.once('close', (code) => { + cleanup(); + consumePath(pending); + if (aborted) { + reject( + new WorkspaceToolError( + 'Workspace tool execution aborted', + 'EXECUTION_ABORTED', + ), + ); + } else if (timedOut) { + reject( + new WorkspaceToolError('Workspace listing timed out', 'LIST_TIMEOUT'), + ); + } else if (stoppedForLimit || code === 0 || code === 1) { + resolvePromise(); + } else { + reject( + new WorkspaceToolError( + 'Workspace listing unavailable', + 'LIST_UNAVAILABLE', + ), + ); + } + }); }); - }); - const paths: string[] = []; - const seenPaths = new Set(); - for (const candidate of candidates) { - let canonicalPath: string; - try { - canonicalPath = await withinListDeadline( - realpath(resolveWorkspacePath(root, candidate.filesystemPath)), - signal, - deadline, - ); - } catch (error) { - if (error instanceof WorkspaceToolError) throw error; - continue; - } - if (!isWithinRoot(root, canonicalPath)) { - continue; - } - const reportedPath = resolveWorkspacePath(root, candidate.resultPath); - try { - const reportedPathStat = await withinListDeadline( - lstat(reportedPath), - signal, - deadline, - ); - if (reportedPathStat.isSymbolicLink()) continue; - const canonicalReportedPath = await withinListDeadline( - realpath(reportedPath), - signal, - deadline, - ); - if (canonicalReportedPath !== canonicalPath) continue; - } catch (error) { - if (error instanceof WorkspaceToolError) throw error; - continue; + const paths: string[] = []; + const seenPaths = new Set(); + for (const candidate of candidates) { + let canonicalPath: string; + try { + canonicalPath = await withinListDeadline( + realpath(resolveWorkspacePath(root, candidate.filesystemPath)), + signal, + deadline, + ); + } catch (error) { + if (error instanceof WorkspaceToolError) throw error; + continue; + } + if (!isWithinRoot(root, canonicalPath)) { + continue; + } + const reportedPath = resolveWorkspacePath(root, candidate.resultPath); + try { + const reportedPathStat = await withinListDeadline( + lstat(reportedPath), + signal, + deadline, + ); + if (reportedPathStat.isSymbolicLink()) continue; + const canonicalReportedPath = await withinListDeadline( + realpath(reportedPath), + signal, + deadline, + ); + if (canonicalReportedPath !== canonicalPath) continue; + } catch (error) { + if (error instanceof WorkspaceToolError) throw error; + continue; + } + let regularFile = false; + try { + regularFile = ( + await withinListDeadline(stat(canonicalPath), signal, deadline) + ).isFile(); + } catch (error) { + if (error instanceof WorkspaceToolError) throw error; + continue; + } + if (!regularFile || seenPaths.has(candidate.resultPath)) continue; + if (paths.length === maxResults) { + truncated = true; + break; + } + seenPaths.add(candidate.resultPath); + paths.push(candidate.resultPath); } - let regularFile = false; - try { - regularFile = ( - await withinListDeadline(stat(canonicalPath), signal, deadline) - ).isFile(); - } catch (error) { - if (error instanceof WorkspaceToolError) throw error; + + if (truncated && paths.length === 0) { + // A scan window can consist entirely of vanished files or symlinks. + // Advance the internal scan cursor, not the public page cursor, and + // keep the original deadline and per-window candidate limit. + afterPath = candidates[candidates.length - 1].resultPath; continue; } - if (!regularFile || seenPaths.has(candidate.resultPath)) continue; - if (paths.length === maxResults) { - truncated = true; - break; - } - seenPaths.add(candidate.resultPath); - paths.push(candidate.resultPath); - } - return { - protocolVersion: BRIDGE_PROTOCOL_VERSION, - operation: 'list_files', - workspaceId: request.workspaceId, - paths, - truncated, - ...(truncated && paths.length > 0 - ? { nextAfterPath: paths[paths.length - 1] } - : {}), - }; + return { + protocolVersion: BRIDGE_PROTOCOL_VERSION, + operation: 'list_files', + workspaceId: request.workspaceId, + paths, + truncated, + ...(truncated && paths.length > 0 + ? { nextAfterPath: paths[paths.length - 1] } + : {}), + }; + } } async function withinListDeadline( @@ -1360,6 +1371,9 @@ async function withinListDeadline( signal: AbortSignal | undefined, deadline: number, ): Promise { + // The filesystem operation has already started. Observe its rejection even + // when cancellation or the deadline prevents us from waiting for it. + void operation.catch(() => undefined); if (signal?.aborted) { throw new WorkspaceToolError( 'Workspace tool execution aborted', From 2634f31ff50304c654d4a378cc13db8e2efa861c Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Mon, 7 Sep 2026 18:14:18 -0400 Subject: [PATCH 4/8] fix: fail closed when credential ACL verification is unavailable (#138) --- packages/code/README.md | 13 ++++- packages/code/src/github.ts | 2 + packages/code/src/private-storage.test.ts | 63 +++++++++++++++++++++++ packages/code/src/private-storage.ts | 16 ++++++ packages/code/src/storage.ts | 16 +++--- 5 files changed, 102 insertions(+), 8 deletions(-) create mode 100644 packages/code/src/private-storage.test.ts create mode 100644 packages/code/src/private-storage.ts diff --git a/packages/code/README.md b/packages/code/README.md index b0bcad80..faddc481 100644 --- a/packages/code/README.md +++ b/packages/code/README.md @@ -35,6 +35,14 @@ LIBRECHAT_CODE_SANDBOX_ENDPOINT=http://127.0.0.1:2000/api/v2 \ librechat-code run ``` +Credential and quarantine storage currently requires Linux (including WSL2), +where ownership and POSIX mode/ACL-mask checks can establish owner-only access. +Native Windows and macOS fail closed before pairing-code redemption, credential +loads, or storage mutations because their extended ACLs cannot yet be verified. +Use storage on a native Linux filesystem, not a Windows drive under `/mnt`. +Support for these platforms requires a native ACL verifier; `chmod` alone is +not sufficient. + Use `--identity ` while pairing and `LIBRECHAT_CODE_IDENTITY_FILE=` while running to override the identity file location. @@ -110,8 +118,9 @@ read only by the trusted worker, which mints and refreshes short-lived installation tokens. A personal access token is supported as a fallback with `LIBRECHAT_CODE_GITHUB_TOKEN`, but the GitHub App is the safer default because its repository access and permissions can be narrowly installed and revoked. -Native Windows currently requires token mode because the worker cannot -reliably validate private-key ACLs there; use WSL2 for GitHub App mode. +Native Windows and macOS credential storage are unavailable until native ACL +verification is implemented; use Linux or WSL2. This also applies to GitHub App +private keys. Git receives authentication through process-scoped `GIT_CONFIG_*` variables. The same isolated config supplies the standard Git LFS filters; hosts using LFS diff --git a/packages/code/src/github.ts b/packages/code/src/github.ts index 9d265e58..058cea83 100644 --- a/packages/code/src/github.ts +++ b/packages/code/src/github.ts @@ -2,6 +2,7 @@ import { constants } from 'node:fs'; import { createHash, createPrivateKey, sign } from 'node:crypto'; import { open, realpath, stat } from 'node:fs/promises'; import { dirname } from 'node:path'; +import { assertPrivateStorageSupported } from './private-storage.js'; export const GITHUB_CREDENTIAL_ENV_NAME = 'LIBRECHAT_CODE_GITHUB_AUTHORIZATION'; export const GITHUB_ALLOWED_DOMAINS = [ @@ -44,6 +45,7 @@ function assertPositiveIdentifier(name: string, value: string): void { } async function readPrivateKey(path: string): Promise { + assertPrivateStorageSupported(); if (process.platform !== 'win32') { const directory = await stat(await realpath(dirname(path))); const uid = process.getuid?.(); diff --git a/packages/code/src/private-storage.test.ts b/packages/code/src/private-storage.test.ts new file mode 100644 index 00000000..c43772ef --- /dev/null +++ b/packages/code/src/private-storage.test.ts @@ -0,0 +1,63 @@ +import assert from 'node:assert/strict'; +import { mkdtemp, readFile, readdir, rm, writeFile } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import test from 'node:test'; +import { assertPrivateStorageSupported } from './private-storage.js'; +import { assertIdentityPathIsPrivate, saveBridgeIdentity, loadBridgeIdentity, + saveWorkspaceMutationQuarantine, loadWorkspaceMutationQuarantine, + clearWorkspaceMutationQuarantine, ensurePrivateWorkspaceDirectory } from './storage.js'; +import { GitHubAppCredentialProvider } from './github.js'; + +const identity = { protocolVersion: 1 as const, workerId: 'worker', + codeApiUrl: 'https://example.com', credential: 'private', privateKey: 'private', + publicKey: 'public', expiresAt: '2099-01-01T00:00:00Z' }; +const quarantine = { version: 1 as const, workerId: 'worker', workspaceId: 'primary', + reason: 'uncertain mutation', quarantinedAt: '2026-01-01T00:00:00Z' }; + +for (const unsupported of ['win32', 'darwin', 'freebsd']) { + test(`${unsupported} refuses storage before creation, reads, or deletion`, async t => { + const root = await mkdtemp(join(tmpdir(), 'private-storage-')); + t.after(() => rm(root, { recursive: true, force: true })); + const existing = join(root, 'existing.json'); + await writeFile(existing, JSON.stringify(identity), { mode: 0o600 }); + const platform = Object.getOwnPropertyDescriptor(process, 'platform')!; + t.after(() => Object.defineProperty(process, 'platform', platform)); + Object.defineProperty(process, 'platform', { ...platform, value: unsupported }); + const path = join(root, 'missing', 'identity.json'); + for (const action of [ + () => assertIdentityPathIsPrivate(path), () => saveBridgeIdentity(path, identity), + () => loadBridgeIdentity(existing), () => saveWorkspaceMutationQuarantine(path, quarantine), + () => loadWorkspaceMutationQuarantine(path), () => clearWorkspaceMutationQuarantine(existing), + () => ensurePrivateWorkspaceDirectory(join(root, 'workspace')), + ]) await assert.rejects(action(), /ACL verification is unavailable/); + assert.deepEqual(await readdir(root), ['existing.json']); + assert.equal(await readFile(existing, 'utf8'), JSON.stringify(identity)); + }); +} + +test('GitHub App credentials also reject unsupported ACL verification before signing or fetching', async t => { + const platform = Object.getOwnPropertyDescriptor(process, 'platform')!; + t.after(() => Object.defineProperty(process, 'platform', platform)); + Object.defineProperty(process, 'platform', { ...platform, value: 'darwin' }); + let fetched = false; + const provider = new GitHubAppCredentialProvider({ + appId: '1', installationId: '1', privateKeyPath: '/must-not-be-read', + fetch: async () => { fetched = true; throw new Error('must not fetch'); }, + }); + await assert.rejects(provider.getCredential(), /ACL verification is unavailable/); + assert.equal(fetched, false); +}); + +test('Linux still requires an available ownership API', t => { + const platform = Object.getOwnPropertyDescriptor(process, 'platform')!; + const getuid = Object.getOwnPropertyDescriptor(process, 'getuid'); + t.after(() => { + Object.defineProperty(process, 'platform', platform); + if (getuid) Object.defineProperty(process, 'getuid', getuid); + else delete process.getuid; + }); + Object.defineProperty(process, 'platform', { ...platform, value: 'linux' }); + Object.defineProperty(process, 'getuid', { configurable: true, value: undefined }); + assert.throws(assertPrivateStorageSupported, /ACL verification is unavailable/); +}); diff --git a/packages/code/src/private-storage.ts b/packages/code/src/private-storage.ts new file mode 100644 index 00000000..90337c5c --- /dev/null +++ b/packages/code/src/private-storage.ts @@ -0,0 +1,16 @@ +import { BridgeProtocolError } from './protocol.js'; + +/** Linux POSIX ACL masks are reflected in group mode bits. macOS extended + * ACLs and Windows DACLs are not: chmod/stat alone cannot establish privacy. + * Fail before creating files, reading credentials, or redeeming pairing codes + * until a native verifier can inspect the actual opened object's ACLs. + */ +export function assertPrivateStorageSupported(): void { + if (process.platform !== 'linux' || process.getuid === undefined) { + throw new BridgeProtocolError( + 'Owner-only storage ACL verification is unavailable on this platform. ' + + 'Worker credentials, GitHub App keys, and quarantine state require Linux ' + + '(including WSL2) with storage on a native Linux filesystem, not /mnt.', + ); + } +} diff --git a/packages/code/src/storage.ts b/packages/code/src/storage.ts index af3e90a7..adf2e526 100644 --- a/packages/code/src/storage.ts +++ b/packages/code/src/storage.ts @@ -14,6 +14,7 @@ import { homedir } from 'node:os'; import { dirname, join, resolve } from 'node:path'; import { BRIDGE_PROTOCOL_VERSION, BridgeProtocolError } from './protocol.js'; +import { assertPrivateStorageSupported } from './private-storage.js'; import type { PairedBridgeWorkerIdentity } from './pairing.js'; @@ -168,12 +169,8 @@ export function defaultWorkspaceQuarantinePath( * Symlinks are resolved: a link's own mode is always `0777` and ignored by the * kernel, so the file the bytes live in is what counts. * - * This reads POSIX mode bits, which is not the whole access story everywhere. - * A Linux POSIX ACL surfaces its mask in the group bits and so is caught, but - * a macOS extended ACL inherited from the parent directory is invisible here - * and survives `chmod`, and Windows is exempt entirely. Establishing owner-only - * storage on those needs real ACL inspection; until then this verifies what the - * mode can express and nothing more. + * Linux POSIX ACL masks are reflected in group mode bits. Platforms whose + * ACLs cannot be verified are rejected at the storage entry points. */ async function groupOrOtherAccessMode( path: string, @@ -310,6 +307,7 @@ async function assertOwnerOnlyPath( export async function ensurePrivateWorkspaceDirectory( path: string, ): Promise { + assertPrivateStorageSupported(); await mkdir(path, { recursive: true, mode: 0o700 }); const metadata = await lstat(path); if (!metadata.isDirectory() || metadata.isSymbolicLink()) { @@ -390,6 +388,7 @@ async function assertSiblingPublishable(path: string): Promise { export async function assertIdentityPathIsPrivate( path: string, ): Promise { + assertPrivateStorageSupported(); await mkdir(dirname(path), { recursive: true, mode: 0o700 }); await assertIdentityDestinationIsReplaceable(path); let created = false; @@ -452,6 +451,7 @@ export async function saveBridgeIdentity( path: string, identity: PairedBridgeWorkerIdentity, ): Promise { + assertPrivateStorageSupported(); await mkdir(dirname(path), { recursive: true, mode: 0o700 }); const temporaryPath = `${path}.${randomBytes(8).toString('hex')}.tmp`; try { @@ -492,6 +492,7 @@ export async function saveWorkspaceMutationQuarantine( path: string, record: WorkspaceMutationQuarantineRecord, ): Promise { + assertPrivateStorageSupported(); await ensureDurableDirectory(dirname(path)); const file = await open(path, 'wx', 0o600); try { @@ -521,6 +522,7 @@ export async function saveWorkspaceMutationQuarantine( export async function loadWorkspaceMutationQuarantine( path: string, ): Promise { + assertPrivateStorageSupported(); /* A marker another account can rewrite is not a control: it could be cleared * to resume mutations, or forged to wedge the worker under a foreign owner. */ let content: string; @@ -553,6 +555,7 @@ export async function clearWorkspaceMutationQuarantine( path: string, ownerId?: string, ): Promise { + assertPrivateStorageSupported(); if (ownerId != null) { const record = await loadWorkspaceMutationQuarantine(path); if (record == null || record.ownerId !== ownerId) { @@ -586,6 +589,7 @@ export async function assertWorkspaceMutationQuarantineOwner( export async function loadBridgeIdentity( path: string, ): Promise { + assertPrivateStorageSupported(); /* An identity written before this check, or by an older release, is still a * private key other local accounts can read. Refuse it rather than booting. */ const content = await readGuardedFile( From 10bd0db4eebcdc46d3336a81b6d33124b45cc571 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Mon, 7 Sep 2026 18:21:20 -0400 Subject: [PATCH 5/8] fix: validate every credential storage ancestor (#139) --- packages/code/README.md | 6 + packages/code/src/github.test.ts | 2 +- packages/code/src/github.ts | 22 +--- packages/code/src/private-storage.ts | 56 +++++++++ packages/code/src/storage-ancestors.test.ts | 119 ++++++++++++++++++++ packages/code/src/storage.ts | 72 +++--------- 6 files changed, 202 insertions(+), 75 deletions(-) create mode 100644 packages/code/src/storage-ancestors.test.ts diff --git a/packages/code/README.md b/packages/code/README.md index faddc481..f9573ee5 100644 --- a/packages/code/README.md +++ b/packages/code/README.md @@ -43,6 +43,12 @@ Use storage on a native Linux filesystem, not a Windows drive under `/mnt`. Support for these platforms requires a native ACL verifier; `chmod` alone is not sufficient. +Every storage ancestor, including intermediate symlink entries and targets, must +be owned by this account or root and must not allow group/other writes unless +protected by the sticky bit. A private directory inside a shared writable parent +is insufficient: that parent can replace the directory. This also applies when +loading GitHub App keys or clearing quarantine state. + Use `--identity ` while pairing and `LIBRECHAT_CODE_IDENTITY_FILE=` while running to override the identity file location. diff --git a/packages/code/src/github.test.ts b/packages/code/src/github.test.ts index 8006f2fe..81dbb2c3 100644 --- a/packages/code/src/github.test.ts +++ b/packages/code/src/github.test.ts @@ -197,7 +197,7 @@ test('rejects a GitHub App key in a shared writable directory', async (t) => { await assert.rejects( provider.getCredential(), - /private key directory must not be writable/, + /writable by other accounts/, ); }); diff --git a/packages/code/src/github.ts b/packages/code/src/github.ts index 058cea83..7bacc297 100644 --- a/packages/code/src/github.ts +++ b/packages/code/src/github.ts @@ -1,8 +1,8 @@ import { constants } from 'node:fs'; import { createHash, createPrivateKey, sign } from 'node:crypto'; -import { open, realpath, stat } from 'node:fs/promises'; +import { open } from 'node:fs/promises'; import { dirname } from 'node:path'; -import { assertPrivateStorageSupported } from './private-storage.js'; +import { assertPrivateStorageAncestors, assertPrivateStorageSupported } from './private-storage.js'; export const GITHUB_CREDENTIAL_ENV_NAME = 'LIBRECHAT_CODE_GITHUB_AUTHORIZATION'; export const GITHUB_ALLOWED_DOMAINS = [ @@ -46,23 +46,7 @@ function assertPositiveIdentifier(name: string, value: string): void { async function readPrivateKey(path: string): Promise { assertPrivateStorageSupported(); - if (process.platform !== 'win32') { - const directory = await stat(await realpath(dirname(path))); - const uid = process.getuid?.(); - if (uid !== undefined && directory.uid !== uid && directory.uid !== 0) { - throw new Error( - 'GitHub App private key directory must be owned by this user or root', - ); - } - const mode = directory.mode & 0o7777; - const protectedByStickyBit = - (mode & 0o1000) !== 0 && (directory.uid === uid || directory.uid === 0); - if ((mode & 0o022) !== 0 && !protectedByStickyBit) { - throw new Error( - 'GitHub App private key directory must not be writable by group or other users', - ); - } - } + await assertPrivateStorageAncestors(dirname(path)); const handle = await open( path, diff --git a/packages/code/src/private-storage.ts b/packages/code/src/private-storage.ts index 90337c5c..ea9952a7 100644 --- a/packages/code/src/private-storage.ts +++ b/packages/code/src/private-storage.ts @@ -1,3 +1,6 @@ +import { lstat, readlink } from 'node:fs/promises'; +import { dirname, isAbsolute } from 'node:path'; + import { BridgeProtocolError } from './protocol.js'; /** Linux POSIX ACL masks are reflected in group mode bits. macOS extended @@ -14,3 +17,56 @@ export function assertPrivateStorageSupported(): void { ); } } + +/** + * Walk from the trust root before touching a descendant. Checking only a + * canonical parent misses replaceable ancestors and symlink entries. Resolve + * links one component at a time so even intermediate link targets are checked. + * Other local accounts cannot replace a checked entry: its parent is either + * non-writable or sticky and the entry belongs to this account or root. + */ +export async function assertPrivateStorageAncestors( + path: string, + allowMissing = false, +): Promise { + assertPrivateStorageSupported(); + const uid = process.getuid!(); + let current = '/'; + const pending = (isAbsolute(path) ? path : `${process.cwd()}/${path}`).split('/'); + let links = 0; + while (true) { + const metadata = await lstat(current).catch((error: NodeJS.ErrnoException) => { + if (allowMissing && error.code === 'ENOENT') return undefined; + throw error; + }); + if (metadata === undefined) return; + if (metadata.uid !== uid && metadata.uid !== 0) { + throw new BridgeProtocolError( + `${current} is owned by another account (uid ${metadata.uid}), ` + + `which can replace ${path}. Keep worker storage on paths this account or root owns.`, + ); + } + if (metadata.isSymbolicLink()) { + if (++links > 40) throw new BridgeProtocolError(`Too many storage symlinks: ${path}`); + const target = await readlink(current); + current = isAbsolute(target) ? '/' : dirname(current); + pending.unshift(...target.split('/')); + continue; + } + if (metadata.isDirectory()) { + const mode = metadata.mode & 0o7777; + if ((mode & 0o022) !== 0 && (mode & 0o1000) === 0) { + throw new BridgeProtocolError( + `Directory ${current} is writable by other accounts (mode ${mode.toString(8)}), ` + + `so ${path} can be replaced even while owner-only.`, + ); + } + } else if (pending.some((part) => part !== '' && part !== '.')) { + throw new BridgeProtocolError(`Storage ancestor must be a directory: ${current}`); + } + let next = pending.shift(); + while (next === '' || next === '.') next = pending.shift(); + if (next === undefined) return; + current = next === '..' ? dirname(current) : `${current === '/' ? '' : current}/${next}`; + } +} diff --git a/packages/code/src/storage-ancestors.test.ts b/packages/code/src/storage-ancestors.test.ts new file mode 100644 index 00000000..1afc7705 --- /dev/null +++ b/packages/code/src/storage-ancestors.test.ts @@ -0,0 +1,119 @@ +import assert from 'node:assert/strict'; +import { chmod, mkdir, mkdtemp, readFile, readdir, rm, symlink, writeFile } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import test from 'node:test'; + +import { GitHubAppCredentialProvider } from './github.js'; +import { assertPrivateStorageAncestors } from './private-storage.js'; +import { + assertIdentityPathIsPrivate, clearWorkspaceMutationQuarantine, + ensurePrivateWorkspaceDirectory, loadBridgeIdentity, loadWorkspaceMutationQuarantine, + saveBridgeIdentity, saveWorkspaceMutationQuarantine, +} from './storage.js'; + +const identity = { + protocolVersion: 1 as const, workerId: 'worker', codeApiUrl: 'https://code.example/v1', + credential: 'secret', expiresAt: '2099-01-01T00:00:00Z', publicKey: 'public', privateKey: 'private', +}; +const marker = { + version: 1 as const, workerId: 'worker', workspaceId: 'workspace', ownerId: 'owner', + quarantinedAt: '2026-01-01T00:00:00Z', reason: 'uncertain result', +}; + +test('a writable ancestor blocks every storage operation before changing private descendants', async (t) => { + const root = await mkdtemp(join(tmpdir(), 'storage-ancestors-')); + t.after(() => rm(root, { recursive: true, force: true })); + const parent = join(root, 'shared'); + const privateDir = join(parent, 'private'); + await mkdir(privateDir, { recursive: true, mode: 0o700 }); + const credential = join(privateDir, 'identity.json'); + const quarantine = join(privateDir, 'quarantine.json'); + await saveBridgeIdentity(credential, identity); + await saveWorkspaceMutationQuarantine(quarantine, marker); + const reservation = await assertIdentityPathIsPrivate(join(privateDir, 'reserved.json')); + await chmod(parent, 0o777); + for (const operation of [ + () => loadBridgeIdentity(credential), + () => saveBridgeIdentity(credential, identity), + () => assertIdentityPathIsPrivate(credential), + () => assertIdentityPathIsPrivate(join(privateDir, 'missing', 'identity.json')), + () => loadWorkspaceMutationQuarantine(quarantine), + () => loadWorkspaceMutationQuarantine(join(privateDir, 'absent.json')), + () => saveWorkspaceMutationQuarantine(join(privateDir, 'new.json'), marker), + () => clearWorkspaceMutationQuarantine(quarantine), + () => clearWorkspaceMutationQuarantine(quarantine, 'owner'), + () => ensurePrivateWorkspaceDirectory(join(privateDir, 'workspace')), + () => reservation.release(), + ]) await assert.rejects(operation(), /writable by other accounts/); + assert.deepEqual((await readdir(privateDir)).sort(), ['identity.json', 'quarantine.json', 'reserved.json']); + assert.deepEqual(JSON.parse(await readFile(credential, 'utf8')), identity); + assert.deepEqual(JSON.parse(await readFile(quarantine, 'utf8')), marker); + + // A trusted sticky parent protects this account's private directory entry. + await chmod(parent, 0o1777); + assert.deepEqual(await loadBridgeIdentity(credential), identity); + await clearWorkspaceMutationQuarantine(quarantine, 'owner'); + await reservation.release(); +}); + +test('intermediate symlinks cannot hide replaceable entry or target ancestors', async (t) => { + const root = await mkdtemp(join(tmpdir(), 'storage-links-')); + t.after(() => rm(root, { recursive: true, force: true })); + const shared = join(root, 'shared'); + const privateDir = join(shared, 'private'); + const safe = join(root, 'safe'); + await mkdir(privateDir, { recursive: true, mode: 0o700 }); + await mkdir(safe, { mode: 0o700 }); + const file = join(safe, 'identity.json'); + await saveBridgeIdentity(file, identity); + await symlink(safe, join(privateDir, 'alias')); + await symlink(join(privateDir, 'alias'), join(root, 'indirect')); + await symlink(privateDir, join(root, 'target')); + await chmod(shared, 0o777); + for (const path of [ + join(privateDir, 'alias', 'identity.json'), + join(root, 'indirect', 'identity.json'), + join(root, 'target', 'missing.json'), + // Do not lexically normalize away a component the kernel traverses. + `${root}/indirect/../safe/identity.json`, + ]) await assert.rejects(assertIdentityPathIsPrivate(path), /writable by other accounts/); + await assert.rejects(loadBridgeIdentity(join(root, 'indirect', 'identity.json')), /writable by other accounts/); +}); + +test('a symlink owned by another account is rejected even under a trusted sticky parent', async (t) => { + const root = await mkdtemp(join(tmpdir(), 'storage-link-owner-')); + t.after(() => rm(root, { recursive: true, force: true })); + const link = join(root, 'alias'); + await symlink(root, link); + // Use a metadata fixture: changing real ownership requires administrator access. + const fs = await import('node:fs/promises'); + const { syncBuiltinESMExports } = await import('node:module'); + const original = fs.default.lstat; + t.after(() => { fs.default.lstat = original; syncBuiltinESMExports(); }); + fs.default.lstat = (async (...args: Parameters) => { + const metadata = await original(...args); + if (String(args[0]).endsWith('/alias')) Object.defineProperty(metadata, 'uid', { value: process.getuid!() + 1000 }); + return metadata; + }) as typeof original; + syncBuiltinESMExports(); + await chmod(root, 0o1777); + await assert.rejects(assertPrivateStorageAncestors(link), /owned by another account/); +}); + +test('GitHub App keys reject writable ancestors before making a token request', async (t) => { + const root = await mkdtemp(join(tmpdir(), 'github-ancestors-')); + t.after(() => rm(root, { recursive: true, force: true })); + const privateDir = join(root, 'private'); + await mkdir(privateDir, { mode: 0o700 }); + const path = join(privateDir, 'app.pem'); + await writeFile(path, 'never read', { mode: 0o600 }); + await chmod(root, 0o777); + let requested = false; + const provider = new GitHubAppCredentialProvider({ + appId: '1', installationId: '2', privateKeyPath: path, + fetch: async () => { requested = true; throw new Error('unexpected request'); }, + }); + await assert.rejects(provider.getCredential(), /writable by other accounts/); + assert.equal(requested, false); +}); diff --git a/packages/code/src/storage.ts b/packages/code/src/storage.ts index adf2e526..c4dab0bd 100644 --- a/packages/code/src/storage.ts +++ b/packages/code/src/storage.ts @@ -4,8 +4,6 @@ import { lstat, mkdir, open, - readFile, - realpath, rename, rm, stat, @@ -14,7 +12,7 @@ import { homedir } from 'node:os'; import { dirname, join, resolve } from 'node:path'; import { BRIDGE_PROTOCOL_VERSION, BridgeProtocolError } from './protocol.js'; -import { assertPrivateStorageSupported } from './private-storage.js'; +import { assertPrivateStorageAncestors, assertPrivateStorageSupported } from './private-storage.js'; import type { PairedBridgeWorkerIdentity } from './pairing.js'; @@ -182,60 +180,13 @@ async function groupOrOtherAccessMode( return (mode & 0o077) === 0 ? undefined : mode; } -/** - * A `0600` file in a directory other accounts can write is not owner-only in - * practice: they cannot read it, but they can unlink and substitute it, so a - * swapped credential or a forged quarantine marker would be trusted. The sticky - * bit counts as protection, which keeps shared `/tmp`-style parents usable. A - * writable ancestor above a private directory could still have that directory - * renamed out from under us, which is broader hardening than this addresses. - * - * Deliberately not applied to the registered workspace, which is the user's own - * project directory and may legitimately be shared. - */ -async function assertDirectoryNotSharedWritable( - directory: string, - path: string, -): Promise { - const metadata = await stat(directory); - const mode = metadata.mode & 0o7777; - const uid = process.getuid?.(); - if (uid !== undefined && !isTrustedOwner(metadata.uid, uid)) { - throw new BridgeProtocolError( - `Directory ${directory} is owned by another account (uid ${metadata.uid}), ` + - `which can grant itself write access and replace ${path}. Keep worker ` + - 'credentials in a directory this account owns.', - ); - } - if ((mode & 0o022) === 0) return; - if ((mode & 0o1000) !== 0 && (metadata.uid === uid || metadata.uid === 0)) { - return; - } - throw new BridgeProtocolError( - `Directory ${directory} is writable by other accounts (mode ${mode.toString(8)}), ` + - `so ${path} can be replaced even while owner-only. Keep worker credentials ` + - 'in a directory only this account can write.', - ); -} - -/** Publishing goes through `rename`, which replaces the named entry itself. */ +/** Publishing replaces the entry itself; reading also follows its target. */ async function assertWriteContainerPrivate(path: string): Promise { - if (process.platform === 'win32' || process.getuid === undefined) return; - await assertDirectoryNotSharedWritable(await realpath(dirname(path)), path); + await assertPrivateStorageAncestors(dirname(path), true); } -/** - * Reading follows the link, so both the entry and the file it names are trust - * boundaries: a writable directory at either end allows a substitution. - */ async function assertReadPathPrivate(path: string): Promise { - if (process.platform === 'win32' || process.getuid === undefined) return; - const entryDirectory = await realpath(dirname(path)); - await assertDirectoryNotSharedWritable(entryDirectory, path); - const targetDirectory = dirname(await realpath(path)); - if (targetDirectory !== entryDirectory) { - await assertDirectoryNotSharedWritable(targetDirectory, path); - } + await assertPrivateStorageAncestors(path); } /** Root is the trust root; anyone else holding a credential path is not. */ @@ -270,6 +221,7 @@ async function readGuardedFile( path: string, exposed: (mode: string) => string, ): Promise { + await assertReadPathPrivate(path); const handle = await open(path, 'r'); try { const stats = await handle.stat(); @@ -308,16 +260,18 @@ export async function ensurePrivateWorkspaceDirectory( path: string, ): Promise { assertPrivateStorageSupported(); + await assertWriteContainerPrivate(path); await mkdir(path, { recursive: true, mode: 0o700 }); + await assertWriteContainerPrivate(path); const metadata = await lstat(path); if (!metadata.isDirectory() || metadata.isSymbolicLink()) { throw new BridgeProtocolError('Default workspace path must be a directory'); } - await chmod(path, 0o700); - await assertOwnerOnlyPath(path); /* This directory is application-owned by contract; a pre-existing one under * another account lets that owner alter workspace inputs and results. */ await assertOwnedByWorker(path); + await chmod(path, 0o700); + await assertOwnerOnlyPath(path); } /** @@ -389,7 +343,9 @@ export async function assertIdentityPathIsPrivate( path: string, ): Promise { assertPrivateStorageSupported(); + await assertWriteContainerPrivate(path); await mkdir(dirname(path), { recursive: true, mode: 0o700 }); + await assertWriteContainerPrivate(path); await assertIdentityDestinationIsReplaceable(path); let created = false; let reservedInode: bigint | undefined; @@ -431,6 +387,7 @@ export async function assertIdentityPathIsPrivate( return { async release(): Promise { if (!created || reservedInode === undefined) return; + await assertWriteContainerPrivate(path); /* Only ever drop the placeholder this call made. A concurrent `pair` * may have published a real identity over the name since, and removing * that would destroy a credential whose code is already spent. */ @@ -452,7 +409,9 @@ export async function saveBridgeIdentity( identity: PairedBridgeWorkerIdentity, ): Promise { assertPrivateStorageSupported(); + await assertWriteContainerPrivate(path); await mkdir(dirname(path), { recursive: true, mode: 0o700 }); + await assertWriteContainerPrivate(path); const temporaryPath = `${path}.${randomBytes(8).toString('hex')}.tmp`; try { const file = await open(temporaryPath, 'wx', 0o600); @@ -493,7 +452,9 @@ export async function saveWorkspaceMutationQuarantine( record: WorkspaceMutationQuarantineRecord, ): Promise { assertPrivateStorageSupported(); + await assertWriteContainerPrivate(path); await ensureDurableDirectory(dirname(path)); + await assertWriteContainerPrivate(path); const file = await open(path, 'wx', 0o600); try { try { @@ -556,6 +517,7 @@ export async function clearWorkspaceMutationQuarantine( ownerId?: string, ): Promise { assertPrivateStorageSupported(); + await assertWriteContainerPrivate(path); if (ownerId != null) { const record = await loadWorkspaceMutationQuarantine(path); if (record == null || record.ownerId !== ownerId) { From 3104227f3a612d9497e708cc6fcbbb585b574c0c Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Mon, 7 Sep 2026 18:24:53 -0400 Subject: [PATCH 6/8] fix: reject identity mount targets before pairing (#140) --- packages/code/README.md | 8 ++ packages/code/src/identity-mount.test.ts | 103 +++++++++++++++++++++++ packages/code/src/identity-mount.ts | 61 ++++++++++++++ packages/code/src/storage.ts | 9 +- 4 files changed, 180 insertions(+), 1 deletion(-) create mode 100644 packages/code/src/identity-mount.test.ts create mode 100644 packages/code/src/identity-mount.ts diff --git a/packages/code/README.md b/packages/code/README.md index f9573ee5..43b681a0 100644 --- a/packages/code/README.md +++ b/packages/code/README.md @@ -53,6 +53,14 @@ Use `--identity ` while pairing and `LIBRECHAT_CODE_IDENTITY_FILE=` while running to override the identity file location. +The identity file itself must not be a bind-mount target: saving a paired +credential atomically replaces that entry. Mount its containing directory +instead. Pairing preflight checks `/proc/self/mountinfo` before redeeming the +one-time code and fails closed if mount information cannot be verified (including +a mount table larger than 4 MiB). Existing identity reads remain supported. +The check describes the current mount namespace; administrators must keep mount +configuration stable during pairing. + ## Native BYOM sandbox (default) The MVP command sandbox runs directly on the user's chosen laptop or VM. It diff --git a/packages/code/src/identity-mount.test.ts b/packages/code/src/identity-mount.test.ts new file mode 100644 index 00000000..13f99904 --- /dev/null +++ b/packages/code/src/identity-mount.test.ts @@ -0,0 +1,103 @@ +import assert from 'node:assert/strict'; +import { spawnSync } from 'node:child_process'; +import fs, { mkdtemp, readFile, readdir, realpath, rm, symlink, writeFile } from 'node:fs/promises'; +import { syncBuiltinESMExports } from 'node:module'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import test from 'node:test'; + +import { identityIsMountPoint } from './identity-mount.js'; +import { assertIdentityPathIsPrivate, saveBridgeIdentity } from './storage.js'; + +const rootMount = '1 0 8:1 / / rw - ext4 /dev/root rw\n'; +const encode = (path: string) => path.replace(/[\\ \t\n]/g, (char) => + `\\${char.charCodeAt(0).toString(8).padStart(3, '0')}`, +); +const mount = (path: string) => `${rootMount}2 1 8:1 /source ${encode(path)} rw shared:1 - ext4 /dev/root rw\n`; +const identity = { + protocolVersion: 1 as const, workerId: 'worker', codeApiUrl: 'https://code.example/v1', + credential: 'secret', expiresAt: '2099-01-01T00:00:00Z', publicKey: 'public', privateKey: 'private', +}; + +test('mount parsing detects same-device bind mounts and escaped path names', () => { + for (const path of ['/home/worker/key.json', '/home/worker/key with\tline\nbreak\\040']) { + assert.equal(identityIsMountPoint(mount(path), path), true); + assert.equal(identityIsMountPoint(mount(path), `${path}.sibling`), false); + } + assert.equal(identityIsMountPoint(mount('/home/worker'), '/home/worker/key.json'), false); + for (const invalid of ['', 'malformed\n', '1 0 8:1 / /bad\\999 rw - ext4 /dev/root rw\n']) { + assert.throws(() => identityIsMountPoint(invalid, '/key'), /malformed/); + } +}); + +test('preflight and direct saves refuse mounted destinations without altering them', async (t) => { + const root = await mkdtemp(join(tmpdir(), 'identity-mount-')); + t.after(() => rm(root, { recursive: true, force: true })); + const path = join(root, 'identity.json'); + const fixture = join(root, 'mountinfo'); + await writeFile(path, 'existing credential', { mode: 0o600 }); + await writeFile(fixture, mount(await realpath(path))); + const original = fs.open; + fs.open = ((path, ...args) => original(path === '/proc/self/mountinfo' ? fixture : path, ...args)) as typeof fs.open; + syncBuiltinESMExports(); + t.after(() => { fs.open = original; syncBuiltinESMExports(); }); + await assert.rejects(assertIdentityPathIsPrivate(path), /is a mount point/); + await assert.rejects(saveBridgeIdentity(path, identity), /is a mount point/); + assert.equal(await readFile(path, 'utf8'), 'existing credential'); + assert.deepEqual((await readdir(root)).sort(), ['identity.json', 'mountinfo']); + + // Parent aliases still name the mounted entry; a leaf link can be replaced. + const parentAlias = join(root, 'alias'); + await symlink(root, parentAlias); + await assert.rejects(assertIdentityPathIsPrivate(join(parentAlias, 'identity.json')), /is a mount point/); + const leaf = join(root, 'leaf.json'); + await symlink(path, leaf); + await assertIdentityPathIsPrivate(leaf); + await saveBridgeIdentity(leaf, identity); + assert.deepEqual(JSON.parse(await readFile(leaf, 'utf8')), identity); + assert.equal(await readFile(path, 'utf8'), 'existing credential'); +}); + +test('unavailable, malformed, and oversized mount tables fail before reserving a new identity', async (t) => { + const root = await mkdtemp(join(tmpdir(), 'identity-mount-info-')); + t.after(() => rm(root, { recursive: true, force: true })); + const fixture = join(root, 'mountinfo'); + const path = join(root, 'identity.json'); + const original = fs.open; + fs.open = ((path, ...args) => original(path === '/proc/self/mountinfo' ? fixture : path, ...args)) as typeof fs.open; + syncBuiltinESMExports(); + t.after(() => { fs.open = original; syncBuiltinESMExports(); }); + await assert.rejects(assertIdentityPathIsPrivate(path), /Cannot verify identity mount status/); + for (const content of ['bad', 'x'.repeat(4 * 1024 * 1024 + 1)]) { + await writeFile(fixture, content); + await assert.rejects(assertIdentityPathIsPrivate(path), /Cannot verify identity mount status/); + } + assert.deepEqual(await readdir(root), ['mountinfo']); +}); + +test('CLI refuses a mounted identity before redeeming the one-time pairing code', async (t) => { + const root = await mkdtemp(join(tmpdir(), 'identity-mount-cli-')); + t.after(() => rm(root, { recursive: true, force: true })); + const path = join(root, 'identity.json'); + const fixture = join(root, 'mountinfo'); + const preload = join(root, 'preload.mjs'); + await writeFile(path, 'existing credential', { mode: 0o600 }); + await writeFile(fixture, mount(await realpath(path))); + await writeFile(preload, ` +import fs from 'node:fs/promises'; +import { syncBuiltinESMExports } from 'node:module'; +Object.defineProperty(process, 'platform', { value: 'linux' }); +const original = fs.open; +fs.open = (path, ...args) => original(path === '/proc/self/mountinfo' ? ${JSON.stringify(fixture)} : path, ...args); +syncBuiltinESMExports(); +globalThis.fetch = async () => { process.stderr.write('PAIRING_REQUEST_ATTEMPTED'); throw new Error('unexpected request'); }; +`); + const result = spawnSync(process.execPath, [ + '--import', preload, new URL('./cli.js', import.meta.url).pathname, + 'pair', 'https://code.example/v1', 'one-time-code', '--worker-id', 'worker', '--identity', path, + ], { encoding: 'utf8', timeout: 10_000 }); + assert.equal(result.status, 1, result.stderr); + assert.match(result.stderr, /is a mount point/); + assert.doesNotMatch(result.stderr, /PAIRING_REQUEST_ATTEMPTED/); + assert.equal(await readFile(path, 'utf8'), 'existing credential'); +}); diff --git a/packages/code/src/identity-mount.ts b/packages/code/src/identity-mount.ts new file mode 100644 index 00000000..ba87d6d6 --- /dev/null +++ b/packages/code/src/identity-mount.ts @@ -0,0 +1,61 @@ +import { open, realpath } from 'node:fs/promises'; +import { basename, dirname, join } from 'node:path'; + +import { BridgeProtocolError } from './protocol.js'; + +const MOUNTINFO_MAX_BYTES = 4 * 1024 * 1024; + +/** mountinfo describes this process's namespace, including same-device bind mounts. */ +export function identityIsMountPoint(mountinfo: string, entryPath: string): boolean { + const lines = mountinfo.trimEnd().split('\n'); + let mounted = false; + for (const line of lines) { + const fields = line.split(' '); + const separator = fields.indexOf('-', 6); + const mountpoint = fields[4]; + if ( + separator < 6 || fields.length !== separator + 4 || + !/^\d+$/.test(fields[0] ?? '') || !/^\d+$/.test(fields[1] ?? '') || + !/^\d+:\d+$/.test(fields[2] ?? '') || !mountpoint?.startsWith('/') || + /\\(?!040|011|012|134)/.test(mountpoint) + ) throw new BridgeProtocolError('Cannot verify identity mount status: malformed /proc/self/mountinfo'); + const decoded = mountpoint.replace(/\\(040|011|012|134)/g, (_, octal: string) => + String.fromCharCode(parseInt(octal, 8)), + ); + if (decoded === entryPath) mounted = true; + } + return mounted; +} + +export async function assertIdentityIsNotMountPoint(path: string): Promise { + // Resolve the parent, not the leaf: rename replaces a leaf symlink itself. + const entryPath = join(await realpath(dirname(path)), basename(path)); + let content: string; + try { + const handle = await open('/proc/self/mountinfo', 'r'); + try { + const buffer = Buffer.alloc(MOUNTINFO_MAX_BYTES + 1); + let length = 0; + while (length < buffer.length) { + const { bytesRead } = await handle.read(buffer, length, buffer.length - length, null); + if (bytesRead === 0) break; + length += bytesRead; + } + if (length > MOUNTINFO_MAX_BYTES) throw new Error('mount table exceeds limit'); + content = buffer.toString('utf8', 0, length); + } finally { + await handle.close(); + } + } catch { + throw new BridgeProtocolError( + 'Cannot verify identity mount status: /proc/self/mountinfo must be readable ' + + 'and no larger than 4 MiB. Use an environment with procfs available.', + ); + } + if (identityIsMountPoint(content, entryPath)) { + throw new BridgeProtocolError( + `Bridge identity path ${path} is a mount point and cannot be atomically replaced. ` + + 'Mount its containing directory instead, or choose an unmounted identity file.', + ); + } +} diff --git a/packages/code/src/storage.ts b/packages/code/src/storage.ts index c4dab0bd..bb0c3ba4 100644 --- a/packages/code/src/storage.ts +++ b/packages/code/src/storage.ts @@ -14,6 +14,8 @@ import { dirname, join, resolve } from 'node:path'; import { BRIDGE_PROTOCOL_VERSION, BridgeProtocolError } from './protocol.js'; import { assertPrivateStorageAncestors, assertPrivateStorageSupported } from './private-storage.js'; +import { assertIdentityIsNotMountPoint } from './identity-mount.js'; + import type { PairedBridgeWorkerIdentity } from './pairing.js'; function isRecord(value: unknown): value is Record { @@ -287,7 +289,10 @@ async function assertIdentityDestinationIsReplaceable( try { metadata = await lstat(path); } catch (error) { - if (isMissingPathError(error)) return; + if (isMissingPathError(error)) { + await assertIdentityIsNotMountPoint(path); + return; + } throw error; } if (metadata.isDirectory()) { @@ -295,6 +300,7 @@ async function assertIdentityDestinationIsReplaceable( `Bridge identity path ${path} is a directory. Point --identity at a file.`, ); } + await assertIdentityIsNotMountPoint(path); const uid = process.platform === 'win32' ? undefined : process.getuid?.(); if (uid === undefined || uid === 0 || metadata.uid === uid) return; /* Ownership only blocks `rename` under the sticky bit, and owning the @@ -412,6 +418,7 @@ export async function saveBridgeIdentity( await assertWriteContainerPrivate(path); await mkdir(dirname(path), { recursive: true, mode: 0o700 }); await assertWriteContainerPrivate(path); + await assertIdentityDestinationIsReplaceable(path); const temporaryPath = `${path}.${randomBytes(8).toString('hex')}.tmp`; try { const file = await open(temporaryPath, 'wx', 0o600); From 2163130ba968c1bf1a0ff53e40d4f8b2725d4084 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Mon, 7 Sep 2026 21:08:57 -0400 Subject: [PATCH 7/8] fix(codeapi): continue bounded listings after skipped candidate windows (#144) Source: ClickHouse/ai@472c19ab1a6e4cccb185a1716b5015f5be6358d6 Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: danny-avila <110412045+danny-avila@users.noreply.github.com> --- CONTRIBUTING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 45c85202..37ecc984 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -7,7 +7,7 @@ Thanks for your interest in Code Interpreter! This repository is published from an internal ClickHouse monorepo, which is the source of truth. Internal changes that are not already public are mirrored here as a snapshot commit on the `sync/main` branch (spot them by the -`Source: ClickHouse/ai@` trailer); a maintainer merges the resulting sync +`Source: ClickHouse/ai@` trailer); a maintainer merges the resulting sync pull request to release it to `main`. Practical consequences: From f300263a13bb69cbfe2754f1fd8e986ee5c1ab5e Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Mon, 7 Sep 2026 21:42:26 -0400 Subject: [PATCH 8/8] fix: restore macOS private storage with native ACL verification (#146) * fix: restore macOS private storage with native ACL verification * fix: reject inheritable ACL grants before storage creation --- .github/workflows/ci.yml | 16 ++ packages/code/README.md | 26 +- packages/code/package-lock.json | 321 +++++++++++++++++++++- packages/code/package.json | 3 +- packages/code/src/github.ts | 3 +- packages/code/src/identity-mount.ts | 15 +- packages/code/src/macos-storage.test.ts | 166 +++++++++++ packages/code/src/macos-storage.ts | 94 +++++++ packages/code/src/private-storage.test.ts | 4 +- packages/code/src/private-storage.ts | 51 +++- packages/code/src/storage.ts | 87 +++--- 11 files changed, 720 insertions(+), 66 deletions(-) create mode 100644 packages/code/src/macos-storage.test.ts create mode 100644 packages/code/src/macos-storage.ts diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0b25285b..a1a4b6e3 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -155,6 +155,22 @@ jobs: - name: Tests run: npm test + macos-storage-tests: + name: macOS Storage ACL Tests + runs-on: macos-14 + defaults: + run: + working-directory: packages/code + steps: + - uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6 + - uses: actions/setup-node@249970729cb0ef3589644e2896645e5dc5ba9c38 # v6 + with: + node-version: 24.16.0 + - run: npm ci + - run: npm run build + - name: Native ACL and credential lifecycle tests + run: node --test dist/macos-storage.test.js dist/private-storage.test.js dist/storage.test.js dist/github.test.js + lambda-microvm-provisioning: name: Lambda MicroVM Provisioning runs-on: ubuntu-latest diff --git a/packages/code/README.md b/packages/code/README.md index 43b681a0..84adfc91 100644 --- a/packages/code/README.md +++ b/packages/code/README.md @@ -35,13 +35,21 @@ LIBRECHAT_CODE_SANDBOX_ENDPOINT=http://127.0.0.1:2000/api/v2 \ librechat-code run ``` -Credential and quarantine storage currently requires Linux (including WSL2), -where ownership and POSIX mode/ACL-mask checks can establish owner-only access. -Native Windows and macOS fail closed before pairing-code redemption, credential -loads, or storage mutations because their extended ACLs cannot yet be verified. -Use storage on a native Linux filesystem, not a Windows drive under `/mnt`. -Support for these platforms requires a native ACL verifier; `chmod` alone is -not sufficient. +Credential and quarantine storage supports macOS and Linux (including WSL2). +On macOS, native descriptor-based ACL calls remove inherited ACLs from new +credential/state files before writing secrets and verify the result. Reads reject +ACL-exposed identities and GitHub App keys; ancestor checks reject ACL write +grants and inheritable allow entries before any child is created. Removing an +ACL after creation cannot revoke descriptors opened while the grant existed. Existing sharing ACLs on parent directories +are never silently removed. Default application-owned workspace directories have +their ACLs removed and modes restricted to `0700`. + +macOS requires the packaged Koffi native dependency (prebuilt for Apple Silicon +and Intel); no Python interpreter or local compiler is needed with those builds. +If it cannot load or ACL inspection fails, storage fails closed before pairing. +Native Windows remains explicitly unsupported until DACL removal and verification +are implemented. Use WSL2 with storage on a native Linux filesystem, not a Windows +drive under `/mnt`. Linux retains ownership and POSIX mode/ACL-mask checks. Every storage ancestor, including intermediate symlink entries and targets, must be owned by this account or root and must not allow group/other writes unless @@ -132,8 +140,8 @@ read only by the trusted worker, which mints and refreshes short-lived installation tokens. A personal access token is supported as a fallback with `LIBRECHAT_CODE_GITHUB_TOKEN`, but the GitHub App is the safer default because its repository access and permissions can be narrowly installed and revoked. -Native Windows and macOS credential storage are unavailable until native ACL -verification is implemented; use Linux or WSL2. This also applies to GitHub App +Native Windows credential storage is unavailable until native DACL removal and +verification are implemented; use macOS, Linux, or WSL2. This also applies to GitHub App private keys. Git receives authentication through process-scoped `GIT_CONFIG_*` variables. diff --git a/packages/code/package-lock.json b/packages/code/package-lock.json index a2da04a0..ac9affc8 100644 --- a/packages/code/package-lock.json +++ b/packages/code/package-lock.json @@ -9,7 +9,8 @@ "version": "0.1.0", "license": "Apache-2.0", "dependencies": { - "@anthropic-ai/sandbox-runtime": "0.0.75" + "@anthropic-ai/sandbox-runtime": "0.0.75", + "koffi": "3.2.1" }, "bin": { "librechat-code": "dist/cli.js" @@ -40,6 +41,294 @@ "node": ">=20.11.0" } }, + "node_modules/@koromix/koffi-android-arm64": { + "version": "3.2.1", + "resolved": "https://registry.npmjs.org/@koromix/koffi-android-arm64/-/koffi-android-arm64-3.2.1.tgz", + "integrity": "sha512-1pJQ4jnZlUJduK9u9DC5CGy3aOgDUPvIXpNb6syV3+Dh5Q/ugezAIGCqvY+w+1mgXsve0pd0NVvJRjdZNHQ6MA==", + "cpu": [ + "arm64" + ], + "license": "MIT", + "optional": true, + "os": [ + "android" + ], + "funding": { + "url": "https://liberapay.com/Koromix" + } + }, + "node_modules/@koromix/koffi-android-x64": { + "version": "3.2.1", + "resolved": "https://registry.npmjs.org/@koromix/koffi-android-x64/-/koffi-android-x64-3.2.1.tgz", + "integrity": "sha512-HH40xGh3gVQifjOBnhwT2tECC0lL1lYe+nxHvWNSzxDIyQNcVPXg38ta7vuONRFpD+uIrw7fqGYLzbZIagkVcg==", + "cpu": [ + "x64" + ], + "license": "MIT", + "optional": true, + "os": [ + "android" + ], + "funding": { + "url": "https://liberapay.com/Koromix" + } + }, + "node_modules/@koromix/koffi-darwin-arm64": { + "version": "3.2.1", + "resolved": "https://registry.npmjs.org/@koromix/koffi-darwin-arm64/-/koffi-darwin-arm64-3.2.1.tgz", + "integrity": "sha512-Vj4h+xcjc5+Cn0DhPHjgRX4omKAv96Kehtcd+1YgYuY2W7FvQn9vS+3SmzVwhC5Qmg9bIwUZObYQ8T/4hBqQqA==", + "cpu": [ + "arm64" + ], + "license": "MIT", + "optional": true, + "os": [ + "darwin" + ], + "funding": { + "url": "https://liberapay.com/Koromix" + } + }, + "node_modules/@koromix/koffi-darwin-x64": { + "version": "3.2.1", + "resolved": "https://registry.npmjs.org/@koromix/koffi-darwin-x64/-/koffi-darwin-x64-3.2.1.tgz", + "integrity": "sha512-gFCWxNBTZIvxo1p+PURWfsy2Ctj5FGnVVs1f03lTLhBvmxEto70pdIiFztdFLDFkAJ1pmtQmruRKapeK+E8YPA==", + "cpu": [ + "x64" + ], + "license": "MIT", + "optional": true, + "os": [ + "darwin" + ], + "funding": { + "url": "https://liberapay.com/Koromix" + } + }, + "node_modules/@koromix/koffi-freebsd-arm64": { + "version": "3.2.1", + "resolved": "https://registry.npmjs.org/@koromix/koffi-freebsd-arm64/-/koffi-freebsd-arm64-3.2.1.tgz", + "integrity": "sha512-qj+f1s2e6vULaUG1cdlTcCXmunCq2t+rjxku1+esaMIqVnHpOwj0QzPuInG0AFdXjwBNQhyVR/HpDj8daEwwsQ==", + "cpu": [ + "arm64" + ], + "license": "MIT", + "optional": true, + "os": [ + "freebsd" + ], + "funding": { + "url": "https://liberapay.com/Koromix" + } + }, + "node_modules/@koromix/koffi-freebsd-ia32": { + "version": "3.2.1", + "resolved": "https://registry.npmjs.org/@koromix/koffi-freebsd-ia32/-/koffi-freebsd-ia32-3.2.1.tgz", + "integrity": "sha512-6olHb1Qfgai0jjs6ddlDDD0ZfsCxy7SPi8rMRpuYQWH0qhgtyQu82hw5b1p7z+TJ0zZP3ZeQQ6l+U/MlM1ICHQ==", + "cpu": [ + "ia32" + ], + "license": "MIT", + "optional": true, + "os": [ + "freebsd" + ], + "funding": { + "url": "https://liberapay.com/Koromix" + } + }, + "node_modules/@koromix/koffi-freebsd-x64": { + "version": "3.2.1", + "resolved": "https://registry.npmjs.org/@koromix/koffi-freebsd-x64/-/koffi-freebsd-x64-3.2.1.tgz", + "integrity": "sha512-Dikhw1ySYNVMkmeFvFVjnU5Wdk6mffNoOjJxm9bTG96vg7OlemylxqdEven47R1YJ3yzNVJn/MlQ207ORWfi2w==", + "cpu": [ + "x64" + ], + "license": "MIT", + "optional": true, + "os": [ + "freebsd" + ], + "funding": { + "url": "https://liberapay.com/Koromix" + } + }, + "node_modules/@koromix/koffi-linux-arm": { + "version": "3.2.1", + "resolved": "https://registry.npmjs.org/@koromix/koffi-linux-arm/-/koffi-linux-arm-3.2.1.tgz", + "integrity": "sha512-OfwUwZylidq95wQKp6ClInULrfB2giu7dqM6Rhe0zAe6lES5I2SXNw15T9+GnRHk3/9hKT2XZ37OZLaKSyWNLA==", + "cpu": [ + "arm" + ], + "license": "MIT", + "optional": true, + "os": [ + "linux" + ], + "funding": { + "url": "https://liberapay.com/Koromix" + } + }, + "node_modules/@koromix/koffi-linux-arm64": { + "version": "3.2.1", + "resolved": "https://registry.npmjs.org/@koromix/koffi-linux-arm64/-/koffi-linux-arm64-3.2.1.tgz", + "integrity": "sha512-K+cGUL5iBcDqxmsocrjmlASqDf24gc7artbVW3PewG2c9AqwC63lezgwvB85Nx4lZAQjB6zIFHh9A7t1yGbwhw==", + "cpu": [ + "arm64" + ], + "license": "MIT", + "optional": true, + "os": [ + "linux" + ], + "funding": { + "url": "https://liberapay.com/Koromix" + } + }, + "node_modules/@koromix/koffi-linux-ia32": { + "version": "3.2.1", + "resolved": "https://registry.npmjs.org/@koromix/koffi-linux-ia32/-/koffi-linux-ia32-3.2.1.tgz", + "integrity": "sha512-rxj6UYjU1qd98gxNQOSCdLpc5cPRi5Giq9rNd3jnGuSNIyMkwa6Dxw4cUjmhIBCYESMJtmNt5NWnJp5u9wTfYQ==", + "cpu": [ + "ia32" + ], + "license": "MIT", + "optional": true, + "os": [ + "linux" + ], + "funding": { + "url": "https://liberapay.com/Koromix" + } + }, + "node_modules/@koromix/koffi-linux-loong64": { + "version": "3.2.1", + "resolved": "https://registry.npmjs.org/@koromix/koffi-linux-loong64/-/koffi-linux-loong64-3.2.1.tgz", + "integrity": "sha512-aHhnHzkPRmT/IHDlGvESJ/Bs32m8N6UE6Ab6kMeJzgk74IN8af2m/81/wZJtybR1M2UxCV4NmlVNUYQQvSAO3Q==", + "cpu": [ + "loong64" + ], + "license": "MIT", + "optional": true, + "os": [ + "linux" + ], + "funding": { + "url": "https://liberapay.com/Koromix" + } + }, + "node_modules/@koromix/koffi-linux-riscv64": { + "version": "3.2.1", + "resolved": "https://registry.npmjs.org/@koromix/koffi-linux-riscv64/-/koffi-linux-riscv64-3.2.1.tgz", + "integrity": "sha512-qtQBsjbm3LiirLJvajWmKkNb7ARk7fvJVXdftJ7NtAnF3Xw8EbDvrtvmvtNI1yLPlYcBmlzCCD71hwhWYk0SIA==", + "cpu": [ + "riscv64" + ], + "license": "MIT", + "optional": true, + "os": [ + "linux" + ], + "funding": { + "url": "https://liberapay.com/Koromix" + } + }, + "node_modules/@koromix/koffi-linux-x64": { + "version": "3.2.1", + "resolved": "https://registry.npmjs.org/@koromix/koffi-linux-x64/-/koffi-linux-x64-3.2.1.tgz", + "integrity": "sha512-c7hw7Qs/r5gnFRTQLcbifBwRU7wiocj+2pVuDQ5Ahb3r36SZmupmgYbTWcLvTW+hul1jd7SKRV0d14ZJq/tvSw==", + "cpu": [ + "x64" + ], + "license": "MIT", + "optional": true, + "os": [ + "linux" + ], + "funding": { + "url": "https://liberapay.com/Koromix" + } + }, + "node_modules/@koromix/koffi-openbsd-ia32": { + "version": "3.2.1", + "resolved": "https://registry.npmjs.org/@koromix/koffi-openbsd-ia32/-/koffi-openbsd-ia32-3.2.1.tgz", + "integrity": "sha512-mmY8fY8LQ/CB52+h3yrMYmVyoxzW3x08S0yI6VNfHWdfU6yJtZkKCbhjmQCYMrWbYKC4gMvwZwCywIGPAkLyeA==", + "cpu": [ + "ia32" + ], + "license": "MIT", + "optional": true, + "os": [ + "openbsd" + ], + "funding": { + "url": "https://liberapay.com/Koromix" + } + }, + "node_modules/@koromix/koffi-openbsd-x64": { + "version": "3.2.1", + "resolved": "https://registry.npmjs.org/@koromix/koffi-openbsd-x64/-/koffi-openbsd-x64-3.2.1.tgz", + "integrity": "sha512-k4ig6aAPbFSRATOIIOfdf/KtlOGH4SVls6L9fy0QnTxRJYvY2oSltTsQtBDANgEQldlq8Kl5WnpRa1VSibP4Lw==", + "cpu": [ + "x64" + ], + "license": "MIT", + "optional": true, + "os": [ + "openbsd" + ], + "funding": { + "url": "https://liberapay.com/Koromix" + } + }, + "node_modules/@koromix/koffi-win32-arm64": { + "version": "3.2.1", + "resolved": "https://registry.npmjs.org/@koromix/koffi-win32-arm64/-/koffi-win32-arm64-3.2.1.tgz", + "integrity": "sha512-cTWBJGK//pDMeKQJE/79Aq9MiOAF4H8QyLZHSQ9IWm8czOfwjG4J1AhsQ9DjI9KFOykH77hhnpmQTGVMIubGig==", + "cpu": [ + "arm64" + ], + "license": "MIT", + "optional": true, + "os": [ + "win32" + ], + "funding": { + "url": "https://liberapay.com/Koromix" + } + }, + "node_modules/@koromix/koffi-win32-ia32": { + "version": "3.2.1", + "resolved": "https://registry.npmjs.org/@koromix/koffi-win32-ia32/-/koffi-win32-ia32-3.2.1.tgz", + "integrity": "sha512-Z50EM6TAZ7CFyMmyX6thv8eNpJchqe9eenhibSIy2Eq/FQYF76gU2VK/LEoaF46L8hfC7TpTp9b10MvReHEyFA==", + "cpu": [ + "ia32" + ], + "license": "MIT", + "optional": true, + "os": [ + "win32" + ], + "funding": { + "url": "https://liberapay.com/Koromix" + } + }, + "node_modules/@koromix/koffi-win32-x64": { + "version": "3.2.1", + "resolved": "https://registry.npmjs.org/@koromix/koffi-win32-x64/-/koffi-win32-x64-3.2.1.tgz", + "integrity": "sha512-ZmZNiBO6bkOSh3QNzgfvb1cMY0yMobn6ZQrSMqbAce21qyYL8niIbyipz9N/PIRDciGhsV0wUxnZsxIO+yWsHQ==", + "cpu": [ + "x64" + ], + "license": "MIT", + "optional": true, + "os": [ + "win32" + ], + "funding": { + "url": "https://liberapay.com/Koromix" + } + }, "node_modules/@pondwader/socks5-server": { "version": "1.0.10", "resolved": "https://registry.npmjs.org/@pondwader/socks5-server/-/socks5-server-1.0.10.tgz", @@ -65,6 +354,36 @@ "node": ">=18" } }, + "node_modules/koffi": { + "version": "3.2.1", + "resolved": "https://registry.npmjs.org/koffi/-/koffi-3.2.1.tgz", + "integrity": "sha512-0qE3lZ8jllRqPN4Ob6Ajl7c2bJSJDhQWuKLGP5hIEpHLllJWv1ydHFMhHmHc5p/W9GticKVDbYzZd7TBoQ4CZg==", + "hasInstallScript": true, + "license": "MIT", + "funding": { + "url": "https://liberapay.com/Koromix" + }, + "optionalDependencies": { + "@koromix/koffi-android-arm64": "3.2.1", + "@koromix/koffi-android-x64": "3.2.1", + "@koromix/koffi-darwin-arm64": "3.2.1", + "@koromix/koffi-darwin-x64": "3.2.1", + "@koromix/koffi-freebsd-arm64": "3.2.1", + "@koromix/koffi-freebsd-ia32": "3.2.1", + "@koromix/koffi-freebsd-x64": "3.2.1", + "@koromix/koffi-linux-arm": "3.2.1", + "@koromix/koffi-linux-arm64": "3.2.1", + "@koromix/koffi-linux-ia32": "3.2.1", + "@koromix/koffi-linux-loong64": "3.2.1", + "@koromix/koffi-linux-riscv64": "3.2.1", + "@koromix/koffi-linux-x64": "3.2.1", + "@koromix/koffi-openbsd-ia32": "3.2.1", + "@koromix/koffi-openbsd-x64": "3.2.1", + "@koromix/koffi-win32-arm64": "3.2.1", + "@koromix/koffi-win32-ia32": "3.2.1", + "@koromix/koffi-win32-x64": "3.2.1" + } + }, "node_modules/node-forge": { "version": "1.4.0", "resolved": "https://registry.npmjs.org/node-forge/-/node-forge-1.4.0.tgz", diff --git a/packages/code/package.json b/packages/code/package.json index c9f810b7..a819af9d 100644 --- a/packages/code/package.json +++ b/packages/code/package.json @@ -60,6 +60,7 @@ "node": ">=20.11" }, "dependencies": { - "@anthropic-ai/sandbox-runtime": "0.0.75" + "@anthropic-ai/sandbox-runtime": "0.0.75", + "koffi": "3.2.1" } } diff --git a/packages/code/src/github.ts b/packages/code/src/github.ts index 7bacc297..e71188c8 100644 --- a/packages/code/src/github.ts +++ b/packages/code/src/github.ts @@ -2,7 +2,7 @@ import { constants } from 'node:fs'; import { createHash, createPrivateKey, sign } from 'node:crypto'; import { open } from 'node:fs/promises'; import { dirname } from 'node:path'; -import { assertPrivateStorageAncestors, assertPrivateStorageSupported } from './private-storage.js'; +import { assertPrivateStorageAcl, assertPrivateStorageAncestors, assertPrivateStorageSupported } from './private-storage.js'; export const GITHUB_CREDENTIAL_ENV_NAME = 'LIBRECHAT_CODE_GITHUB_AUTHORIZATION'; export const GITHUB_ALLOWED_DOMAINS = [ @@ -69,6 +69,7 @@ async function readPrivateKey(path: string): Promise { 'GitHub App private key must not be accessible by group or other users', ); } + await assertPrivateStorageAcl(handle, path); return await handle.readFile('utf8'); } finally { await handle.close(); diff --git a/packages/code/src/identity-mount.ts b/packages/code/src/identity-mount.ts index ba87d6d6..ea4cc7f4 100644 --- a/packages/code/src/identity-mount.ts +++ b/packages/code/src/identity-mount.ts @@ -1,4 +1,4 @@ -import { open, realpath } from 'node:fs/promises'; +import { lstat, open, realpath } from 'node:fs/promises'; import { basename, dirname, join } from 'node:path'; import { BridgeProtocolError } from './protocol.js'; @@ -30,6 +30,19 @@ export function identityIsMountPoint(mountinfo: string, entryPath: string): bool export async function assertIdentityIsNotMountPoint(path: string): Promise { // Resolve the parent, not the leaf: rename replaces a leaf symlink itself. const entryPath = join(await realpath(dirname(path)), basename(path)); + if (process.platform === 'darwin') { + const metadata = await lstat(path).catch((error: NodeJS.ErrnoException) => { + if (error.code === 'ENOENT') return undefined; + throw error; + }); + // A missing entry cannot be mounted; rename replaces a symlink itself. + if (metadata === undefined || metadata.isSymbolicLink()) return; + const { macOsMountPoint } = await import('./macos-storage.js'); + if (await realpath(macOsMountPoint(entryPath)) === entryPath) { + throw new BridgeProtocolError(`Bridge identity path ${path} is a mount point and cannot be atomically replaced.`); + } + return; + } let content: string; try { const handle = await open('/proc/self/mountinfo', 'r'); diff --git a/packages/code/src/macos-storage.test.ts b/packages/code/src/macos-storage.test.ts new file mode 100644 index 00000000..afbcdf77 --- /dev/null +++ b/packages/code/src/macos-storage.test.ts @@ -0,0 +1,166 @@ +import assert from 'node:assert/strict'; +import { execFile } from 'node:child_process'; +import fs, { chmod, mkdtemp, mkdir, open, readFile, readdir, rename, rm, writeFile } from 'node:fs/promises'; +import { syncBuiltinESMExports } from 'node:module'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { promisify } from 'node:util'; +import test from 'node:test'; +import { assertPrivateStorageAcl, assertPrivateStorageAncestors, removePrivateStorageAcl } from './private-storage.js'; +import { assertIdentityPathIsPrivate, saveBridgeIdentity, loadBridgeIdentity, + saveWorkspaceMutationQuarantine, loadWorkspaceMutationQuarantine, + clearWorkspaceMutationQuarantine, ensurePrivateWorkspaceDirectory } from './storage.js'; +import { GitHubAppCredentialProvider } from './github.js'; + +const exec = promisify(execFile); +const mac = { skip: process.platform !== 'darwin' }; +const identity = { protocolVersion: 1 as const, workerId: 'worker', + codeApiUrl: 'https://example.com', credential: 'secret', privateKey: 'private', + publicKey: 'public', expiresAt: '2099-01-01T00:00:00Z' }; +const quarantine = { version: 1 as const, workerId: 'worker', workspaceId: 'primary', + ownerId: 'owner', reason: 'uncertain', quarantinedAt: '2026-01-01T00:00:00Z' }; + +async function grant(path: string, permissions: string): Promise { + await exec('/bin/chmod', ['+a', `everyone allow ${permissions}`, path]); +} + +// Real kernel ACLs: chmod(0600) alone leaves these grants effective. +test('macOS removes inherited deny ACLs before publishing identity and quarantine state', mac, async t => { + const root = await mkdtemp(join(tmpdir(), 'macos-acl-')); + t.after(() => rm(root, { recursive: true, force: true })); + await exec('/bin/chmod', ['+a', 'everyone deny read,file_inherit,directory_inherit,only_inherit', root]); + let guardedWrites = 0; + const originalOpen = fs.open; + fs.open = (async (...args: Parameters) => { + const handle = await originalOpen(...args); + if (args[1] === 'wx') { + const write = handle.writeFile.bind(handle); + handle.writeFile = async (...values) => { + await assertPrivateStorageAcl(handle, String(args[0])); + guardedWrites += 1; + return write(...values); + }; + } + return handle; + }) as typeof fs.open; + syncBuiltinESMExports(); + t.after(() => { fs.open = originalOpen; syncBuiltinESMExports(); }); + const path = join(root, 'identity.json'); + const reservation = await assertIdentityPathIsPrivate(path); + assert.equal(await readFile(path, 'utf8'), ''); + await saveBridgeIdentity(path, identity); + await reservation.release(); + assert.deepEqual(await loadBridgeIdentity(path), identity); + const marker = join(root, 'quarantine.json'); + await saveWorkspaceMutationQuarantine(marker, quarantine); + assert.deepEqual(await loadWorkspaceMutationQuarantine(marker), quarantine); + for (const file of [path, marker]) { + const listing = await exec('/bin/ls', ['-lde', file]); + assert.doesNotMatch(listing.stdout, /\n\s*0:/); + } + // Re-pairing exercises native mount verification and sibling publication. + await (await assertIdentityPathIsPrivate(path)).release(); + await clearWorkspaceMutationQuarantine(marker, 'owner'); + assert.equal(guardedWrites, 2); + const workspace = join(root, 'workspace'); + await mkdir(workspace); + await grant(workspace, 'read,write,delete'); + await ensurePrivateWorkspaceDirectory(workspace); + assert.doesNotMatch((await exec('/bin/ls', ['-lde', workspace])).stdout, /\n\s*0:/); +}); + +test('macOS refuses exposed 0600 identities, quarantine markers, and GitHub keys', mac, async t => { + const root = await mkdtemp(join(tmpdir(), 'macos-acl-')); + t.after(() => rm(root, { recursive: true, force: true })); + const path = join(root, 'identity.json'); + const marker = join(root, 'quarantine.json'); + await saveBridgeIdentity(path, identity); + await saveWorkspaceMutationQuarantine(marker, quarantine); + for (const file of [path, marker]) { + await grant(file, 'read'); + await chmod(file, 0o600); + } + await assert.rejects(loadBridgeIdentity(path), /macOS ACL grants/); + await assert.rejects(assertIdentityPathIsPrivate(path), /macOS ACL grants/); + await assert.rejects(loadWorkspaceMutationQuarantine(marker), /macOS ACL grants/); + let fetched = false; + const provider = new GitHubAppCredentialProvider({ + appId: '1', installationId: '1', privateKeyPath: path, + fetch: async () => { fetched = true; throw new Error('unexpected fetch'); }, + }); + await assert.rejects(provider.getCredential(), /macOS ACL grants/); + assert.equal(fetched, false); + assert.equal(await readFile(path, 'utf8'), `${JSON.stringify(identity, null, 2)}\n`); +}); + +test('macOS rejects ACL-writable ancestors and accepts deny-only home-style ACLs', mac, async t => { + const root = await mkdtemp(join(tmpdir(), 'macos-acl-')); + t.after(async () => { + await exec('/bin/chmod', ['-N', root]); + await rm(root, { recursive: true, force: true }); + }); + await exec('/bin/chmod', ['+a', 'everyone deny delete', root]); + await assertPrivateStorageAncestors(root); + await grant(root, 'add_file,delete_child'); + await chmod(root, 0o700); + await assert.rejects(assertIdentityPathIsPrivate(join(root, 'identity.json')), /macOS ACL grants/); +}); + +test('macOS ACL checks and removal operate on the held inode after path replacement', mac, async t => { + const root = await mkdtemp(join(tmpdir(), 'macos-acl-')); + t.after(() => rm(root, { recursive: true, force: true })); + const path = join(root, 'file'); + await writeFile(path, '', { mode: 0o600 }); + await grant(path, 'read'); + const handle = await open(path, 'r'); + try { + await rename(path, join(root, 'old')); + await writeFile(path, '', { mode: 0o600 }); + await assert.rejects(assertPrivateStorageAcl(handle, path), /macOS ACL grants/); + await removePrivateStorageAcl(handle, path); + await assertPrivateStorageAcl(handle, path); + } finally { + await handle.close(); + } + await assert.rejects(assertPrivateStorageAcl(handle, path), /Cannot verify macOS/); + await assert.rejects(removePrivateStorageAcl(handle, path), /Cannot verify macOS/); +}); + + +test('macOS checks mount status without procfs', mac, async () => { + const { macOsMountPoint } = await import('./macos-storage.js'); + assert.equal(macOsMountPoint('/'), '/'); + assert.throws(() => macOsMountPoint('/nonexistent-macos-acl-test'), /Cannot verify macOS identity mount/); +}); + + +test('macOS rejects inheritable allow ACLs before creating any storage inode', mac, async t => { + for (const inheritance of ['file_inherit', 'directory_inherit', 'file_inherit,only_inherit']) { + await t.test(inheritance, async t => { + const root = await mkdtemp(join(tmpdir(), 'macos-inherited-acl-')); + t.after(() => rm(root, { recursive: true, force: true })); + const existing = join(root, 'existing.json'); + await saveBridgeIdentity(existing, identity); + await chmod(root, 0o755); + await grant(root, `read,search,${inheritance}`); + let creates = 0; + const originalOpen = fs.open; + fs.open = (async (...args: Parameters) => { + if (args[1] === 'wx') creates += 1; + return originalOpen(...args); + }) as typeof fs.open; + syncBuiltinESMExports(); + t.after(() => { fs.open = originalOpen; syncBuiltinESMExports(); }); + const path = join(root, 'nested', 'identity.json'); + for (const action of [ + () => assertIdentityPathIsPrivate(path), + () => assertIdentityPathIsPrivate(existing), + () => saveBridgeIdentity(path, identity), + () => saveWorkspaceMutationQuarantine(path, quarantine), + () => ensurePrivateWorkspaceDirectory(join(root, 'workspace')), + ]) await assert.rejects(action(), /macOS ACL grants/); + assert.equal(creates, 0); + assert.deepEqual(await readdir(root), ['existing.json']); + }); + } +}); diff --git a/packages/code/src/macos-storage.ts b/packages/code/src/macos-storage.ts new file mode 100644 index 00000000..e75261d9 --- /dev/null +++ b/packages/code/src/macos-storage.ts @@ -0,0 +1,94 @@ +import koffi from 'koffi'; +import { BridgeProtocolError } from './protocol.js'; + +// Darwin sys/acl.h. Use the held descriptor, never /dev/fd path metadata. +const ACL_TYPE_EXTENDED = 0x100; +const ACL_EXTENDED_ALLOW = 1; +const ACL_EXTENDED_DENY = 2; +const ACL_NEXT_ENTRY = -1; +const ACL_ENTRY_FILE_INHERIT = 1 << 5; +const ACL_ENTRY_DIRECTORY_INHERIT = 1 << 6; +const lib = koffi.load('/usr/lib/libSystem.B.dylib'); +const getAcl = lib.func('void *acl_get_fd_np(int fd, int type)'); +const setAcl = lib.func('int acl_set_fd_np(int fd, void *acl, int type)'); +const initAcl = lib.func('void *acl_init(int count)'); +const freeAcl = lib.func('int acl_free(void *acl)'); +const getEntry = lib.func('int acl_get_entry(void *acl, int index, _Out_ void **entry)'); +const getTag = lib.func('int acl_get_tag_type(void *entry, _Out_ int *tag)'); +const getMask = lib.func('int acl_get_permset_mask_np(void *entry, _Out_ uint64_t *mask)'); +const getFlags = lib.func('int acl_get_flagset_np(void *entry, _Out_ void **flags)'); +const getFlag = lib.func('int acl_get_flag_np(void *flags, int flag)'); +// Only non-inheritable read/list, search/execute, and metadata reads are safe. +// Removing an inherited grant after open cannot revoke an attacker's held fd. +const ANCESTOR_READ_PERMISSIONS = (1 << 1) | (1 << 3) | (1 << 7) | (1 << 9) | (1 << 11); + +function unavailable(): never { + throw new BridgeProtocolError('Cannot verify macOS storage ACLs on the opened object; use a local filesystem with ACL support.'); +} + +export function verifyMacOsAcl(fd: number, path: string, directory = false, empty = false): void { + const acl = getAcl(fd, ACL_TYPE_EXTENDED); + if (acl == null) { + // On a valid descriptor Darwin reports ENOENT when no extended ACL exists. + if (koffi.errno() === koffi.os.errno.ENOENT) return; + unavailable(); + } + try { + for (let index = 0; ; index = ACL_NEXT_ENTRY) { + const entry = [null]; + if (getEntry(acl, index, entry) !== 0) { + // Darwin uses EINVAL at end of the ACL (unlike Linux's zero result). + if (koffi.errno() === koffi.os.errno.EINVAL) return; + unavailable(); + } + const tag = [0]; + const mask = [0]; + const flags = [null]; + if (getTag(entry[0], tag) !== 0 || getMask(entry[0], mask) !== 0 || + getFlags(entry[0], flags) !== 0) unavailable(); + const fileInherit = getFlag(flags[0], ACL_ENTRY_FILE_INHERIT); + const directoryInherit = getFlag(flags[0], ACL_ENTRY_DIRECTORY_INHERIT); + if (fileInherit < 0 || directoryInherit < 0) unavailable(); + if (empty || (tag[0] !== ACL_EXTENDED_DENY && + (tag[0] !== ACL_EXTENDED_ALLOW || !directory || fileInherit || directoryInherit || + (BigInt(mask[0]) & ~BigInt(ANCESTOR_READ_PERMISSIONS)) !== 0n))) { + throw new BridgeProtocolError( + `macOS ACL grants access beyond owner-only storage at ${path}. ` + + 'Remove the sharing ACL before using this path. If a credential was exposed, revoke it and pair again.', + ); + } + } + } finally { + freeAcl(acl); + } +} + +export function removeMacOsAcl(fd: number, path: string): void { + const acl = initAcl(0); + if (acl == null) unavailable(); + try { + if (setAcl(fd, acl, ACL_TYPE_EXTENDED) !== 0) unavailable(); + } finally { + freeAcl(acl); + } + verifyMacOsAcl(fd, path, false, true); +} + +// Darwin's statfs64 layout is identical on arm64 and x86_64 (sys/mount.h). +const Statfs = koffi.struct({ + bsize: 'uint32_t', iosize: 'int32_t', + blocks: 'uint64_t', bfree: 'uint64_t', bavail: 'uint64_t', + files: 'uint64_t', ffree: 'uint64_t', fsid: 'int32_t[2]', + owner: 'uint32_t', type: 'uint32_t', flags: 'uint32_t', subtype: 'uint32_t', + typename: 'char[16]', mountpoint: 'char[1024]', source: 'char[1024]', + flagsExt: 'uint32_t', reserved: 'uint32_t[7]', +}); +const statfs = lib.func('statfs64', 'int', ['str', koffi.out(koffi.pointer(Statfs))]); + +export function macOsMountPoint(path: string): string { + const result: { mountpoint?: string } = {}; + if (statfs(path, result) !== 0 || !result.mountpoint?.startsWith('/')) { + throw new BridgeProtocolError('Cannot verify macOS identity mount status.'); + } + return result.mountpoint; +} diff --git a/packages/code/src/private-storage.test.ts b/packages/code/src/private-storage.test.ts index c43772ef..c4b7e760 100644 --- a/packages/code/src/private-storage.test.ts +++ b/packages/code/src/private-storage.test.ts @@ -15,7 +15,7 @@ const identity = { protocolVersion: 1 as const, workerId: 'worker', const quarantine = { version: 1 as const, workerId: 'worker', workspaceId: 'primary', reason: 'uncertain mutation', quarantinedAt: '2026-01-01T00:00:00Z' }; -for (const unsupported of ['win32', 'darwin', 'freebsd']) { +for (const unsupported of ['win32', 'freebsd']) { test(`${unsupported} refuses storage before creation, reads, or deletion`, async t => { const root = await mkdtemp(join(tmpdir(), 'private-storage-')); t.after(() => rm(root, { recursive: true, force: true })); @@ -39,7 +39,7 @@ for (const unsupported of ['win32', 'darwin', 'freebsd']) { test('GitHub App credentials also reject unsupported ACL verification before signing or fetching', async t => { const platform = Object.getOwnPropertyDescriptor(process, 'platform')!; t.after(() => Object.defineProperty(process, 'platform', platform)); - Object.defineProperty(process, 'platform', { ...platform, value: 'darwin' }); + Object.defineProperty(process, 'platform', { ...platform, value: 'freebsd' }); let fetched = false; const provider = new GitHubAppCredentialProvider({ appId: '1', installationId: '1', privateKeyPath: '/must-not-be-read', diff --git a/packages/code/src/private-storage.ts b/packages/code/src/private-storage.ts index ea9952a7..af4f185a 100644 --- a/packages/code/src/private-storage.ts +++ b/packages/code/src/private-storage.ts @@ -1,23 +1,44 @@ -import { lstat, readlink } from 'node:fs/promises'; +import { lstat, open, readlink } from 'node:fs/promises'; +import { constants } from 'node:fs'; +import type { FileHandle } from 'node:fs/promises'; import { dirname, isAbsolute } from 'node:path'; import { BridgeProtocolError } from './protocol.js'; -/** Linux POSIX ACL masks are reflected in group mode bits. macOS extended - * ACLs and Windows DACLs are not: chmod/stat alone cannot establish privacy. - * Fail before creating files, reading credentials, or redeeming pairing codes - * until a native verifier can inspect the actual opened object's ACLs. - */ +/** Linux exposes POSIX ACL masks in mode bits; macOS needs native ACL calls. */ export function assertPrivateStorageSupported(): void { - if (process.platform !== 'linux' || process.getuid === undefined) { + if (!['linux', 'darwin'].includes(process.platform) || process.getuid === undefined) { throw new BridgeProtocolError( 'Owner-only storage ACL verification is unavailable on this platform. ' + - 'Worker credentials, GitHub App keys, and quarantine state require Linux ' + - '(including WSL2) with storage on a native Linux filesystem, not /mnt.', + 'Native Windows is unsupported until DACL removal and verification are implemented. ' + + 'Use macOS or Linux (including WSL2 with a native Linux filesystem, not /mnt).', ); } } +async function macOsStorage() { + try { + return await import('./macos-storage.js'); + } catch { + throw new BridgeProtocolError('macOS ACL verification is unavailable: reinstall @librechat/code with its Koffi native dependency.'); + } +} + +export async function assertPrivateStorageAcl( + handle: FileHandle, path: string, directory = false, +): Promise { + if (process.platform === 'darwin') { + (await macOsStorage()).verifyMacOsAcl(handle.fd, path, directory); + } +} + +/** Only application-owned files/directories may have their ACLs removed. */ +export async function removePrivateStorageAcl(handle: FileHandle, path: string): Promise { + if (process.platform === 'darwin') { + (await macOsStorage()).removeMacOsAcl(handle.fd, path); + } +} + /** * Walk from the trust root before touching a descendant. Checking only a * canonical parent misses replaceable ancestors and symlink entries. Resolve @@ -54,6 +75,18 @@ export async function assertPrivateStorageAncestors( continue; } if (metadata.isDirectory()) { + if (process.platform === 'darwin') { + const handle = await open(current, constants.O_RDONLY | constants.O_NOFOLLOW); + try { + const opened = await handle.stat(); + if (opened.dev !== metadata.dev || opened.ino !== metadata.ino) { + throw new BridgeProtocolError(`Storage directory changed during ACL verification: ${current}`); + } + await assertPrivateStorageAcl(handle, current, true); + } finally { + await handle.close(); + } + } const mode = metadata.mode & 0o7777; if ((mode & 0o022) !== 0 && (mode & 0o1000) === 0) { throw new BridgeProtocolError( diff --git a/packages/code/src/storage.ts b/packages/code/src/storage.ts index bb0c3ba4..af2f1295 100644 --- a/packages/code/src/storage.ts +++ b/packages/code/src/storage.ts @@ -8,11 +8,12 @@ import { rm, stat, } from 'node:fs/promises'; +import type { FileHandle } from 'node:fs/promises'; import { homedir } from 'node:os'; import { dirname, join, resolve } from 'node:path'; import { BRIDGE_PROTOCOL_VERSION, BridgeProtocolError } from './protocol.js'; -import { assertPrivateStorageAncestors, assertPrivateStorageSupported } from './private-storage.js'; +import { assertPrivateStorageAcl, removePrivateStorageAcl, assertPrivateStorageAncestors, assertPrivateStorageSupported } from './private-storage.js'; import { assertIdentityIsNotMountPoint } from './identity-mount.js'; @@ -160,28 +161,6 @@ export function defaultWorkspaceQuarantinePath( ); } -/** - * Verify a path really is owner-only. `chmod` reports success without effect on - * mounts that do not implement POSIX permissions - notably WSL2 DrvFs - * (`/mnt/`), where the result stays world-accessible - so a credential - * that cannot be protected must fail closed rather than appear protected. - * - * Symlinks are resolved: a link's own mode is always `0777` and ignored by the - * kernel, so the file the bytes live in is what counts. - * - * Linux POSIX ACL masks are reflected in group mode bits. Platforms whose - * ACLs cannot be verified are rejected at the storage entry points. - */ -async function groupOrOtherAccessMode( - path: string, -): Promise { - if (process.platform === 'win32') return undefined; - /* Resolve symlinks: the bytes live at the target, and a link's own mode is - * always 0777 and ignored by the kernel. */ - const mode = (await stat(path)).mode & 0o777; - return (mode & 0o077) === 0 ? undefined : mode; -} - /** Publishing replaces the entry itself; reading also follows its target. */ async function assertWriteContainerPrivate(path: string): Promise { await assertPrivateStorageAncestors(dirname(path), true); @@ -238,24 +217,31 @@ async function readGuardedFile( const mode = stats.mode & 0o777; if ((mode & 0o077) !== 0) throw new BridgeProtocolError(exposed(mode.toString(8))); } + await assertPrivateStorageAcl(handle, path); return await handle.readFile('utf8'); } finally { await handle.close(); } } -async function assertOwnerOnlyPath( - path: string, - reportedPath: string = path, -): Promise { - const mode = await groupOrOtherAccessMode(path); - if (mode === undefined) return; - throw new BridgeProtocolError( - `Cannot restrict ${reportedPath} to owner-only access (mode ${mode.toString(8)}). ` + - 'Filesystems that ignore POSIX permissions, such as Windows drives mounted ' + - 'under /mnt, cannot protect worker credentials or workspaces. Use a path on a ' + - 'native Linux filesystem.', - ); +async function assertOwnerOnlyFile(handle: FileHandle, path: string): Promise { + const mode = (await handle.stat()).mode & 0o777; + if ((mode & 0o077) !== 0) { + throw new BridgeProtocolError( + `Cannot restrict ${path} to owner-only access (mode ${mode.toString(8)}). ` + + 'Use a native macOS or Linux filesystem that enforces permissions, not a Windows drive under /mnt.', + ); + } + await assertPrivateStorageAcl(handle, path); +} + +async function assertOwnerOnlyPath(path: string): Promise { + const handle = await open(path, 'r'); + try { + await assertOwnerOnlyFile(handle, path); + } finally { + await handle.close(); + } } export async function ensurePrivateWorkspaceDirectory( @@ -272,7 +258,14 @@ export async function ensurePrivateWorkspaceDirectory( /* This directory is application-owned by contract; a pre-existing one under * another account lets that owner alter workspace inputs and results. */ await assertOwnedByWorker(path); - await chmod(path, 0o700); + const directory = await open(path, 'r'); + try { + await removePrivateStorageAcl(directory, path); + await directory.chmod(0o700); + await assertPrivateStorageAcl(directory, path); + } finally { + await directory.close(); + } await assertOwnerOnlyPath(path); } @@ -333,7 +326,14 @@ export interface IdentityPathReservation { async function assertSiblingPublishable(path: string): Promise { const probePath = `${path}.${randomBytes(8).toString('hex')}.probe`; try { - await (await open(probePath, 'wx', 0o600)).close(); + const probe = await open(probePath, 'wx', 0o600); + try { + await removePrivateStorageAcl(probe, path); + await probe.chmod(0o600); + await assertOwnerOnlyFile(probe, path); + } finally { + await probe.close(); + } } catch (error) { throw new BridgeProtocolError( `Cannot create a temporary file beside ${path} (${ @@ -363,8 +363,9 @@ export async function assertIdentityPathIsPrivate( const reserved = await open(path, 'wx', 0o600); try { created = true; + await removePrivateStorageAcl(reserved, path); await reserved.chmod(0o600); - await assertOwnerOnlyPath(path); + await assertOwnerOnlyFile(reserved, path); reservedInode = (await reserved.stat({ bigint: true })).ino; } finally { await reserved.close(); @@ -423,8 +424,9 @@ export async function saveBridgeIdentity( try { const file = await open(temporaryPath, 'wx', 0o600); try { + await removePrivateStorageAcl(file, path); await file.chmod(0o600); - await assertOwnerOnlyPath(temporaryPath, path); + await assertOwnerOnlyFile(file, path); await file.writeFile(`${JSON.stringify(identity, null, 2)}\n`, 'utf8'); await file.sync(); } finally { @@ -465,8 +467,9 @@ export async function saveWorkspaceMutationQuarantine( const file = await open(path, 'wx', 0o600); try { try { + await removePrivateStorageAcl(file, path); await file.chmod(0o600); - await assertOwnerOnlyPath(path); + await assertOwnerOnlyFile(file, path); await assertWriteContainerPrivate(path); await file.writeFile(`${JSON.stringify(record, null, 2)}\n`, 'utf8'); await file.sync(); @@ -500,7 +503,7 @@ export async function loadWorkspaceMutationQuarantine( (mode) => `Workspace quarantine ${path} is accessible beyond its owner (mode ${mode}). ` + 'Another local account could clear or forge it. Keep worker state on a ' + - 'native Linux filesystem.', + 'native macOS or Linux filesystem.', ); await assertReadPathPrivate(path); } catch (error) { @@ -566,7 +569,7 @@ export async function loadBridgeIdentity( (mode) => `Bridge identity ${path} is accessible beyond its owner (mode ${mode}). ` + 'Treat its private key as compromised: revoke the worker and pair again with an ' + - 'identity path on a native Linux filesystem.', + 'identity path on a native macOS or Linux filesystem.', ); /* After the file's own verdict, so an exposed mode keeps its diagnosis. */ await assertReadPathPrivate(path);