Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
164 changes: 162 additions & 2 deletions packages/code/src/storage.test.ts
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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/<drive>`).
* Returns undefined on hosts where every writable filesystem honours chmod.
*/
async function findChmodIgnoringDirectory(): Promise<string | undefined> {
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 });
}
});
115 changes: 103 additions & 12 deletions packages/code/src/storage.ts
Original file line number Diff line number Diff line change
@@ -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';

Expand Down Expand Up @@ -149,6 +149,67 @@ 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/<drive>`), 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.
*
* 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,
): Promise<number | undefined> {
if (process.platform === 'win32') return undefined;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Enforce private credential storage on native Windows

On native Windows this unconditional return disables the new protection entirely, even though Windows is a supported worker platform and its ACLs can grant other local accounts access regardless of the numeric mode passed to open or chmod. Consequently pairing, identity loading, and quarantine storage all accept files without verifying that their ACL is owner-only. Use a Windows ACL check/tightening implementation, or fail closed when owner-only storage cannot be established, rather than treating every Windows path as private.

Useful? React with πŸ‘Β / πŸ‘Ž.

const mode = (await stat(path)).mode & 0o777;
return (mode & 0o077) === 0 ? undefined : mode;
Comment on lines +172 to +173

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Verify the secured file's owner, not only its mode

On Unix, when the worker runs as root or with equivalent filesystem capabilities, a foreign-owned 0600 identity or quarantine file passes this check because only permission bits are inspected. For example, a non-root-owned identity inside a root-owned 0755 directory is accepted, yet that non-root owner can rewrite the file and control the credentials subsequently trusted by the worker. Require stat(path).uid to match the worker UID for these protected files in addition to checking their mode.

Useful? React with πŸ‘Β / πŸ‘Ž.

Comment on lines +172 to +173

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Clear inherited ACLs before accepting owner-only mode

On macOS, a supported native platform, a destination directory can have an inheritable extended ACL granting another user or everyone access. The temporary identity file inherits that ACL, while file.chmod(0o600) changes only its POSIX mode and this stat check therefore accepts it as 0600; the private key is then written despite remaining accessible through the ACL. Inspect and remove or reject extended ACL entries before accepting the file as owner-only.

Useful? React with πŸ‘Β / πŸ‘Ž.

}

async function assertOwnerOnlyPath(
path: string,
reportedPath: string = path,
): Promise<void> {
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<string> {
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<void> {
Expand All @@ -158,6 +219,7 @@ export async function ensurePrivateWorkspaceDirectory(
throw new BridgeProtocolError('Default workspace path must be a directory');
}
await chmod(path, 0o700);
await assertOwnerOnlyPath(path);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Verify the owner of existing default workspaces

When --default-workspace runs as root and the stable directory already exists under another account's ownership, such as a bind-mounted 0700 directory, chmod and this mode-only check both succeed. The foreign owner can still unlink or replace entries in the workspace, allowing it to alter inputs and results despite the directory being presented as application-owned and owner-only; reject an existing directory whose UID is neither the worker's nor a trusted owner.

Useful? React with πŸ‘Β / πŸ‘Ž.

}

export async function saveBridgeIdentity(
Expand All @@ -169,6 +231,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 {
Expand Down Expand Up @@ -205,10 +271,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);
}
Expand All @@ -218,13 +298,15 @@ export async function loadWorkspaceMutationQuarantine(
): Promise<WorkspaceMutationQuarantineRecord | undefined> {
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;
Expand Down Expand Up @@ -278,7 +360,16 @@ export async function assertWorkspaceMutationQuarantineOwner(
export async function loadBridgeIdentity(
path: string,
): Promise<PairedBridgeWorkerIdentity> {
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}`);
}
Expand Down