From 107450aedcf471b39796de1021251848b876f937 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Fri, 4 Sep 2026 12:24:25 -0400 Subject: [PATCH 1/2] fix(code): fail closed when worker credentials cannot be owner-only `chmod` reports success without effect on mounts that do not implement POSIX permissions. On WSL2 DrvFs (`/mnt/`) the paired identity was written and reported as saved while remaining world-accessible: POSIX mode 777, and a Windows ACL granting `Authenticated Users: Modify`. Any local authenticated user could read the worker's Ed25519 private key, contradicting the owner-only guarantee the README documents. Tighten explicitly, then verify, and fail closed when group or other access survives - on the write path and the read path both, since hardening only writes would leave already-paired workers booting on an exposed key. The identity verdict is taken against the still-empty temporary file, so no private key is written to a world-readable path, and the load paths validate and read through a single descriptor so a retargeted symlink cannot make the file that was judged differ from the file that is read. Symlinks resolve for the verdict: a link's own mode is always 0777 and ignored by the kernel, so the file the bytes live in is what counts. Skipped on win32, where POSIX mode bits are not meaningful and NTFS ACLs govern access. A quarantine marker that cannot be protected is removed rather than left unparseable for every later load, and that removal is synced as durably as the write it undoes. --- packages/code/src/storage.test.ts | 164 +++++++++++++++++++++++++++++- packages/code/src/storage.ts | 108 +++++++++++++++++--- 2 files changed, 258 insertions(+), 14 deletions(-) diff --git a/packages/code/src/storage.test.ts b/packages/code/src/storage.test.ts index 3a17705a..b45d351d 100644 --- a/packages/code/src/storage.test.ts +++ b/packages/code/src/storage.test.ts @@ -1,5 +1,15 @@ import assert from 'node:assert/strict'; -import { mkdtemp, open, rm, stat, writeFile } from 'node:fs/promises'; +import { + chmod, + mkdir, + mkdtemp, + open, + readdir, + rm, + stat, + symlink, + writeFile, +} from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import test from 'node:test'; @@ -170,7 +180,8 @@ test('workspace mutation quarantine persists until explicitly cleared', async (t await clearWorkspaceMutationQuarantine(path); assert.equal(syncCalls, process.platform === 'win32' ? 1 : 4); assert.equal(await loadWorkspaceMutationQuarantine(path), undefined); - await writeFile(path, '{bad json', 'utf8'); + /* Owner-only, so this exercises the parse failure and not the mode check. */ + await writeFile(path, '{bad json', { encoding: 'utf8', mode: 0o600 }); await assert.rejects( loadWorkspaceMutationQuarantine(path), /invalid workspace quarantine file/i, @@ -213,3 +224,152 @@ test('workspace mutation quarantine cannot be replaced or cleared by another own await rm(directory, { recursive: true, force: true }); } }); + +const SAMPLE_IDENTITY = { + protocolVersion: 1 as const, + workerId: 'vm-1', + codeApiUrl: 'https://code.example/v1', + credential: 'credential', + expiresAt: new Date(Date.now() + 60_000).toISOString(), + publicKey: 'public', + privateKey: 'private', +}; + +/** + * Locate a mount that ignores POSIX permissions (WSL2 DrvFs under `/mnt/`). + * Returns undefined on hosts where every writable filesystem honours chmod. + */ +async function findChmodIgnoringDirectory(): Promise { + const roots: string[] = []; + const configured = process.env.LIBRECHAT_CODE_TEST_NONPOSIX_DIR?.trim(); + if (configured) roots.push(configured); + try { + for (const entry of await readdir('/mnt')) roots.push(join('/mnt', entry)); + } catch { + /* No /mnt on this host. */ + } + for (const root of roots) { + let directory: string | undefined; + try { + directory = await mkdtemp(join(root, 'librechat-code-mode-')); + const probe = join(directory, 'probe'); + await writeFile(probe, '', { mode: 0o600 }); + await chmod(probe, 0o600); + if (((await stat(probe)).mode & 0o077) !== 0) return directory; + } catch { + /* Root is absent or not writable. */ + } + if (directory) await rm(directory, { recursive: true, force: true }); + } + return undefined; +} + +test('credential storage fails closed on filesystems that ignore chmod', async (t) => { + const directory = await findChmodIgnoringDirectory(); + if (!directory) { + t.skip('no chmod-ignoring filesystem available on this host'); + return; + } + try { + const identityPath = join(directory, 'worker.json'); + await assert.rejects( + saveBridgeIdentity(identityPath, SAMPLE_IDENTITY), + /owner-only access/, + ); + /* The private key must not be left behind on a world-readable path. */ + await assert.rejects(stat(identityPath), { code: 'ENOENT' }); + + await assert.rejects( + ensurePrivateWorkspaceDirectory(join(directory, 'workspace')), + /owner-only access/, + ); + + const quarantinePath = join(directory, 'quarantine.json'); + await assert.rejects( + saveWorkspaceMutationQuarantine(quarantinePath, { + version: 1, + workerId: 'vm-1', + workspaceId: 'primary', + quarantinedAt: new Date().toISOString(), + reason: 'test', + }), + /owner-only access/, + ); + /* An unreadable half-written marker would wedge every later load. */ + await assert.rejects(stat(quarantinePath), { code: 'ENOENT' }); + } finally { + await rm(directory, { recursive: true, force: true }); + } +}); + +test('an already-exposed identity is refused on load', async (t) => { + const directory = await findChmodIgnoringDirectory(); + if (!directory) { + t.skip('no chmod-ignoring filesystem available on this host'); + return; + } + try { + /* Written the way a release without the save-time check would have. */ + const path = join(directory, 'legacy-worker.json'); + await writeFile(path, JSON.stringify(SAMPLE_IDENTITY), { mode: 0o600 }); + await assert.rejects(loadBridgeIdentity(path), /accessible beyond its owner/); + } finally { + await rm(directory, { recursive: true, force: true }); + } +}); + +test('an already-exposed quarantine marker is refused on load', async (t) => { + const directory = await findChmodIgnoringDirectory(); + if (!directory) { + t.skip('no chmod-ignoring filesystem available on this host'); + return; + } + try { + const path = join(directory, 'quarantine.json'); + await writeFile( + path, + JSON.stringify({ + version: 1, + workerId: 'vm-1', + workspaceId: 'primary', + ownerId: 'incarnation-1', + quarantinedAt: new Date().toISOString(), + reason: 'test', + }), + { mode: 0o600 }, + ); + await assert.rejects( + loadWorkspaceMutationQuarantine(path), + /accessible beyond its owner/, + ); + } finally { + await rm(directory, { recursive: true, force: true }); + } +}); + +test('a symlinked owner-only identity is accepted', async () => { + const base = await mkdtemp(join(tmpdir(), 'librechat-code-storage-')); + try { + const target = join(base, 'real.json'); + await saveBridgeIdentity(target, SAMPLE_IDENTITY); + /* A link's own mode is always 0777; the credential's mode is the target's. */ + const link = join(base, 'link.json'); + await symlink(target, link); + assert.deepEqual(await loadBridgeIdentity(link), SAMPLE_IDENTITY); + } finally { + await rm(base, { recursive: true, force: true }); + } +}); + +test('default workspace directories are tightened when they already exist', async () => { + const base = await mkdtemp(join(tmpdir(), 'librechat-code-storage-')); + try { + const workspace = join(base, 'workspace'); + await mkdir(workspace, { mode: 0o777 }); + await chmod(workspace, 0o777); + await ensurePrivateWorkspaceDirectory(workspace); + assert.equal((await stat(workspace)).mode & 0o777, 0o700); + } finally { + await rm(base, { recursive: true, force: true }); + } +}); diff --git a/packages/code/src/storage.ts b/packages/code/src/storage.ts index 2a7eef5c..ce76dadf 100644 --- a/packages/code/src/storage.ts +++ b/packages/code/src/storage.ts @@ -1,5 +1,5 @@ import { createHash, randomBytes } from 'node:crypto'; -import { chmod, lstat, mkdir, open, readFile, rename, rm } from 'node:fs/promises'; +import { chmod, lstat, mkdir, open, readFile, rename, rm, stat } from 'node:fs/promises'; import { homedir } from 'node:os'; import { dirname, join, resolve } from 'node:path'; @@ -149,6 +149,60 @@ 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. + */ +async function groupOrOtherAccessMode( + path: string, +): Promise { + if (process.platform === 'win32') return undefined; + const mode = (await stat(path)).mode & 0o777; + return (mode & 0o077) === 0 ? undefined : mode; +} + +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.', + ); +} + +/** + * Validate and read through one descriptor. Checking a path and then reading it + * resolves the name twice, so a symlink retargeted in between would let the file + * that was judged differ from the file that is read. + */ +async function readGuardedFile( + path: string, + exposed: (mode: string) => string, +): Promise { + const handle = await open(path, 'r'); + try { + if (process.platform !== 'win32') { + const mode = (await handle.stat()).mode & 0o777; + if ((mode & 0o077) !== 0) { + throw new BridgeProtocolError(exposed(mode.toString(8))); + } + } + return await handle.readFile('utf8'); + } finally { + await handle.close(); + } +} + export async function ensurePrivateWorkspaceDirectory( path: string, ): Promise { @@ -158,6 +212,7 @@ export async function ensurePrivateWorkspaceDirectory( throw new BridgeProtocolError('Default workspace path must be a directory'); } await chmod(path, 0o700); + await assertOwnerOnlyPath(path); } export async function saveBridgeIdentity( @@ -169,6 +224,10 @@ export async function saveBridgeIdentity( try { const file = await open(temporaryPath, 'wx', 0o600); try { + /* Tighten every way available before judging, and judge before the key + * is written, so no private key reaches a world-readable path. */ + await file.chmod(0o600); + await assertOwnerOnlyPath(temporaryPath, path); await file.writeFile(`${JSON.stringify(identity, null, 2)}\n`, 'utf8'); await file.sync(); } finally { @@ -205,10 +264,24 @@ export async function saveWorkspaceMutationQuarantine( await ensureDurableDirectory(dirname(path)); const file = await open(path, 'wx', 0o600); try { - await file.writeFile(`${JSON.stringify(record, null, 2)}\n`, 'utf8'); - await file.sync(); - } finally { - await file.close(); + try { + await file.chmod(0o600); + await assertOwnerOnlyPath(path); + await file.writeFile(`${JSON.stringify(record, null, 2)}\n`, 'utf8'); + await file.sync(); + } finally { + await file.close(); + } + } catch (error) { + /* A partial marker fails every later load, so undoing it has to reach the + * disk as durably as the write it is undoing. */ + await rm(path, { force: true }); + try { + await syncParentDirectory(path); + } catch { + /* Surface the original failure, not a cleanup-durability one. */ + } + throw error; } await syncParentDirectory(path); } @@ -218,13 +291,15 @@ export async function loadWorkspaceMutationQuarantine( ): Promise { let content: string; try { - content = await readFile(path, 'utf8'); + content = await readGuardedFile( + path, + (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.', + ); } catch (error) { - if ( - isRecord(error) && - 'code' in error && - error.code === 'ENOENT' - ) { + if (isMissingPathError(error)) { return undefined; } throw error; @@ -278,7 +353,16 @@ export async function assertWorkspaceMutationQuarantineOwner( export async function loadBridgeIdentity( path: string, ): Promise { - const identity = JSON.parse(await readFile(path, 'utf8')) as unknown; + /* 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( + path, + (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.', + ); + const identity = JSON.parse(content) as unknown; if (!isPairedIdentity(identity)) { throw new BridgeProtocolError(`Invalid bridge identity file: ${path}`); } From 9b3c0281eedc7dac611f162d88babc299d74b362 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Fri, 4 Sep 2026 13:14:31 -0400 Subject: [PATCH 2/2] docs(code): state what the mode check does not cover The verdict reads POSIX mode bits. A Linux POSIX ACL surfaces its mask in the group bits and is caught, but a macOS extended ACL inherited from the parent directory is invisible to `stat` and survives `chmod`, and Windows is exempt outright. Say so at the check rather than let the name imply more than it verifies. --- packages/code/src/storage.ts | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/packages/code/src/storage.ts b/packages/code/src/storage.ts index ce76dadf..8c0d1cbf 100644 --- a/packages/code/src/storage.ts +++ b/packages/code/src/storage.ts @@ -157,6 +157,13 @@ 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. */ async function groupOrOtherAccessMode( path: string,