diff --git a/src/application/ClientConfigPath.ts b/src/application/ClientConfigPath.ts new file mode 100644 index 000000000..34b2cf349 --- /dev/null +++ b/src/application/ClientConfigPath.ts @@ -0,0 +1,53 @@ +import { lstat, realpath } from "node:fs/promises"; + +interface ClientConfigPathStats { + readonly uid?: number; + isFile?(): boolean; + isSymbolicLink(): boolean; +} + +interface ClientConfigPathFileSystem { + lstat(path: string): Promise; + realpath?(path: string): Promise; +} + +const systemFileSystem: ClientConfigPathFileSystem = { lstat, realpath }; + +/** + * Resolve an existing, user-owned config symlink to one stable transaction + * path. Missing regular paths remain eligible for first-run creation. + */ +export const resolveClientConfigTransactionPath = async ( + requestedPath: string, + fileSystem: ClientConfigPathFileSystem = systemFileSystem, +): Promise => { + let requestedStats: ClientConfigPathStats; + try { + requestedStats = await fileSystem.lstat(requestedPath); + } catch (cause: unknown) { + return isMissing(cause) ? requestedPath : undefined; + } + if (!requestedStats.isSymbolicLink()) return requestedPath; + + const currentUid = + typeof process.getuid === "function" ? process.getuid() : undefined; + if ( + currentUid === undefined || + requestedStats.uid !== currentUid || + fileSystem.realpath === undefined + ) + return undefined; + + try { + const canonicalPath = await fileSystem.realpath(requestedPath); + const targetStats = await fileSystem.lstat(canonicalPath); + return targetStats.isFile?.() === true && targetStats.uid === currentUid + ? canonicalPath + : undefined; + } catch { + return undefined; + } +}; + +const isMissing = (cause: unknown): boolean => + cause instanceof Error && "code" in cause && cause.code === "ENOENT"; diff --git a/src/application/Setup.ts b/src/application/Setup.ts index 639f71d72..523f6a682 100644 --- a/src/application/Setup.ts +++ b/src/application/Setup.ts @@ -3,7 +3,6 @@ import { copyFile, mkdir, readFile, - rename, rm, writeFile, } from "node:fs/promises"; @@ -15,6 +14,7 @@ import { parse as parseToml, stringify as stringifyToml } from "smol-toml"; import { PRODUCT_IDENTITY } from "../identity.js"; import { supportsNodeVersion } from "../domain/runtimeVersion.js"; +import { resolveClientConfigTransactionPath } from "./ClientConfigPath.js"; import { runDoctor, systemDoctorHost } from "./Doctor.js"; import { installLinuxHopper, @@ -49,7 +49,7 @@ export type ClientConfigurationResult = } | { readonly status: "failed"; - readonly reason: "backup" | "write" | "readback"; + readonly reason: "path" | "backup" | "write" | "readback"; }; /** * Effects required by the idempotent setup workflow. @@ -288,10 +288,15 @@ export const configureJsonClient = async ( "mcp", ], ): Promise => { + const transactionPath = await resolveClientConfigTransactionPath( + client.configPath, + ); + if (transactionPath === undefined) + return { status: "failed", reason: "path" }; let document: Record = {}; let original: string | undefined; try { - original = await readFile(client.configPath, "utf8"); + original = await readFile(transactionPath, "utf8"); document = parseObject(original); } catch (cause: unknown) { if (!isMissing(cause)) return { status: "failed", reason: "readback" }; @@ -318,7 +323,7 @@ export const configureJsonClient = async ( if (original !== undefined) { backupPath = `${client.configPath}.rea.backup`; try { - await copyFile(client.configPath, backupPath); + await copyFile(transactionPath, backupPath); } catch { return { status: "failed", reason: "backup" }; } @@ -328,26 +333,26 @@ export const configureJsonClient = async ( [PRODUCT_IDENTITY.mcpServerKey]: desired, }; const encoded = `${JSON.stringify(document, null, 2)}\n`; - const temporary = `${client.configPath}.rea.tmp`; try { await mkdir(dirname(client.configPath), { recursive: true }); - await writeFile(temporary, encoded, { encoding: "utf8", mode: 0o600 }); - await rename(temporary, client.configPath); + await writeFileAtomic(transactionPath, encoded, { + encoding: "utf8", + mode: 0o600, + }); } catch { - await rm(temporary, { force: true }).catch(() => undefined); return { status: "failed", reason: "write" }; } try { - const readback = parseObject(await readFile(client.configPath, "utf8")); + const readback = parseObject(await readFile(transactionPath, "utf8")); const value = parseOptionalObject(readback.mcpServers)[ PRODUCT_IDENTITY.mcpServerKey ]; if (JSON.stringify(value) !== JSON.stringify(desired)) { - await restoreConfig(client.configPath, original); + await restoreConfig(transactionPath, original); return { status: "failed", reason: "readback" }; } } catch { - await restoreConfig(client.configPath, original); + await restoreConfig(transactionPath, original); return { status: "failed", reason: "readback" }; } return { @@ -367,10 +372,15 @@ export const configureTomlClient = async ( "mcp", ], ): Promise => { + const transactionPath = await resolveClientConfigTransactionPath( + client.configPath, + ); + if (transactionPath === undefined) + return { status: "failed", reason: "path" }; let document: Record = {}; let original: string | undefined; try { - original = await readFile(client.configPath, "utf8"); + original = await readFile(transactionPath, "utf8"); document = objectSchema.parse(parseToml(original)); } catch (cause: unknown) { if (!isMissing(cause)) return { status: "failed", reason: "readback" }; @@ -396,7 +406,7 @@ export const configureTomlClient = async ( const backupPath = original === undefined ? undefined : `${client.configPath}.rea.backup`; try { - if (backupPath !== undefined) await copyFile(client.configPath, backupPath); + if (backupPath !== undefined) await copyFile(transactionPath, backupPath); } catch { return { status: "failed", reason: "backup" }; } @@ -406,12 +416,12 @@ export const configureTomlClient = async ( }; try { await mkdir(dirname(client.configPath), { recursive: true }); - await writeFileAtomic(client.configPath, stringifyToml(document), { + await writeFileAtomic(transactionPath, stringifyToml(document), { encoding: "utf8", mode: 0o600, }); const readback = objectSchema.parse( - parseToml(await readFile(client.configPath, "utf8")), + parseToml(await readFile(transactionPath, "utf8")), ); if ( JSON.stringify( @@ -422,7 +432,7 @@ export const configureTomlClient = async ( ) throw new Error("TOML readback mismatch"); } catch { - await restoreConfig(client.configPath, original); + await restoreConfig(transactionPath, original); return { status: "failed", reason: "readback" }; } return { diff --git a/src/application/SetupClients.ts b/src/application/SetupClients.ts index bcaca48e0..d8f1eab62 100644 --- a/src/application/SetupClients.ts +++ b/src/application/SetupClients.ts @@ -5,8 +5,10 @@ import type { } from "./Setup.js"; const failedConfigurationMessage = ( - reason: "backup" | "write" | "readback", + reason: "path" | "backup" | "write" | "readback", ): string => { + if (reason === "path") + return "Agent configuration path could not be safely verified. Check its permissions and, if it is a symbolic link, verify that the link resolves to a regular file owned by the current user, then rerun setup."; if (reason === "backup") return "Agent configuration could not be backed up, so no change was made. Check file permissions, then rerun setup."; if (reason === "write") diff --git a/src/application/Uninstall.ts b/src/application/Uninstall.ts index d9a15f7b9..efe3c9329 100644 --- a/src/application/Uninstall.ts +++ b/src/application/Uninstall.ts @@ -1,4 +1,4 @@ -import { copyFile, lstat, readFile, rm } from "node:fs/promises"; +import { copyFile, lstat, readFile, realpath, rm } from "node:fs/promises"; import { homedir } from "node:os"; import { basename, isAbsolute, join } from "node:path"; @@ -7,9 +7,12 @@ import writeFileAtomic from "write-file-atomic"; import { z } from "zod"; import { PRODUCT_IDENTITY } from "../identity.js"; +import { resolveClientConfigTransactionPath } from "./ClientConfigPath.js"; import { supportedClients, type SetupClient } from "./SupportedClients.js"; interface ManagedPathStats { + readonly uid?: number; + isFile?(): boolean; isSymbolicLink(): boolean; } @@ -19,6 +22,7 @@ export interface UninstallFileSystem { copy(source: string, destination: string): Promise; writeText(path: string, contents: string): Promise; stat(path: string): Promise; + realpath?(path: string): Promise; remove(path: string): Promise; } @@ -28,6 +32,7 @@ const systemFileSystem: UninstallFileSystem = { writeText: (path, contents) => writeFileAtomic(path, contents, { encoding: "utf8" }), stat: (path) => lstat(path), + realpath: (path) => realpath(path), remove: (path) => rm(path, { recursive: true }), }; @@ -104,9 +109,12 @@ const removeClient = async ( "skipped", "This client has no documented local MCP configuration boundary.", ); + const resolved = await resolveUninstallConfigPath(client, fileSystem); + if (typeof resolved !== "string") return resolved; + const transactionPath = resolved; let original: string; try { - original = await fileSystem.readText(client.configPath); + original = await fileSystem.readText(transactionPath); } catch (cause: unknown) { return isMissing(cause) ? item(client.name, "skipped", "Configuration does not exist.") @@ -146,7 +154,7 @@ const removeClient = async ( document[key] = remaining; const backupPath = `${client.configPath}.rea.backup`; try { - await fileSystem.copy(client.configPath, backupPath); + await fileSystem.copy(transactionPath, backupPath); } catch { return item( client.name, @@ -159,11 +167,11 @@ const removeClient = async ( client.format === "toml" ? stringifyToml(document) : `${JSON.stringify(document, null, 2)}\n`; - await fileSystem.writeText(client.configPath, encoded); + await fileSystem.writeText(transactionPath, encoded); const readback = objectSchema.parse( client.format === "toml" - ? parseToml(await fileSystem.readText(client.configPath)) - : JSON.parse(await fileSystem.readText(client.configPath)), + ? parseToml(await fileSystem.readText(transactionPath)) + : JSON.parse(await fileSystem.readText(transactionPath)), ); if (PRODUCT_IDENTITY.mcpServerKey in optionalObject(readback[key])) throw new Error("registration readback mismatch"); @@ -174,7 +182,7 @@ const removeClient = async ( ); } catch { try { - await fileSystem.writeText(client.configPath, original); + await fileSystem.writeText(transactionPath, original); } catch { return item( client.name, @@ -190,6 +198,26 @@ const removeClient = async ( } }; +const resolveUninstallConfigPath = async ( + client: SetupClient, + fileSystem: UninstallFileSystem, +): Promise => { + const path = await resolveClientConfigTransactionPath( + client.configPath, + fileSystem.realpath === undefined + ? { lstat: fileSystem.stat } + : { lstat: fileSystem.stat, realpath: fileSystem.realpath }, + ); + return ( + path ?? + item( + client.name, + "failed", + "Configuration path could not be safely verified. Check its permissions and, if it is a symbolic link, verify that the link resolves to a regular file owned by the current user, then rerun uninstall.", + ) + ); +}; + /** Ownership rule for persistent MCP registrations created by REA setup. */ const isOwnedRegistration = (value: unknown): boolean => { const parsed = registrationSchema.safeParse(value); diff --git a/tests/setup.test.ts b/tests/setup.test.ts index 11c79dfb1..2635be5ed 100644 --- a/tests/setup.test.ts +++ b/tests/setup.test.ts @@ -274,6 +274,10 @@ describe("setup workflow", () => { }); it.each([ + [ + "path", + "Agent configuration path could not be safely verified. Check its permissions and, if it is a symbolic link, verify that the link resolves to a regular file owned by the current user, then rerun setup.", + ], [ "backup", "Agent configuration could not be backed up, so no change was made. Check file permissions, then rerun setup.", diff --git a/tests/setupConfig.test.ts b/tests/setupConfig.test.ts index d5467df7a..9dd9516ed 100644 --- a/tests/setupConfig.test.ts +++ b/tests/setupConfig.test.ts @@ -1,8 +1,16 @@ -import { mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; +import { + lstat, + mkdtemp, + readFile, + rm, + symlink, + writeFile, +} from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { afterEach, describe, expect, it } from "vitest"; +import { resolveClientConfigTransactionPath } from "../src/application/ClientConfigPath.js"; import { configureJsonClient } from "../src/application/Setup.js"; let directory: string | undefined; @@ -13,6 +21,25 @@ afterEach(async () => { }); describe("JSON client configuration transaction", () => { + it("rejects a symlink target not owned by the current user", async () => { + if (typeof process.getuid !== "function") return; + const currentUid = process.getuid(); + let statCalls = 0; + expect( + await resolveClientConfigTransactionPath("/config", { + lstat: () => { + statCalls += 1; + return Promise.resolve({ + uid: statCalls === 1 ? currentUid : currentUid + 1, + isFile: () => statCalls > 1, + isSymbolicLink: () => statCalls === 1, + }); + }, + realpath: () => Promise.resolve("/target"), + }), + ).toBeUndefined(); + }); + it("preserves existing keys, creates a backup, and reads back the MCP entry", async () => { directory = await mkdtemp(join(tmpdir(), "rea-setup-")); const configPath = join(directory, "mcp.json"); @@ -34,6 +61,48 @@ describe("JSON client configuration transaction", () => { }); }); + it("updates a symlink target without replacing the JSON config symlink", async () => { + directory = await mkdtemp(join(tmpdir(), "rea-setup-")); + const targetPath = join(directory, "managed.json"); + const configPath = join(directory, "mcp.json"); + const original = '{"theme":"dark"}\n'; + await writeFile(targetPath, original); + await symlink(targetPath, configPath); + + expect( + await configureJsonClient({ name: "cursor", configPath }), + ).toMatchObject({ status: "configured" }); + expect((await lstat(configPath)).isSymbolicLink()).toBe(true); + expect(await readFile(`${configPath}.rea.backup`, "utf8")).toBe(original); + expect(JSON.parse(await readFile(targetPath, "utf8"))).toMatchObject({ + theme: "dark", + mcpServers: { + rea: { + command: "npx", + args: ["-y", "rea-agents", "mcp"], + }, + }, + }); + expect(await configureJsonClient({ name: "cursor", configPath })).toEqual({ + status: "unchanged", + }); + }); + + it("fails before mutation when a JSON config symlink is dangling", async () => { + directory = await mkdtemp(join(tmpdir(), "rea-setup-")); + const configPath = join(directory, "mcp.json"); + await symlink(join(directory, "missing.json"), configPath); + + expect(await configureJsonClient({ name: "cursor", configPath })).toEqual({ + status: "failed", + reason: "path", + }); + expect((await lstat(configPath)).isSymbolicLink()).toBe(true); + await expect( + readFile(`${configPath}.rea.backup`, "utf8"), + ).rejects.toThrow(); + }); + it("performs no write or second backup when configuration already matches", async () => { directory = await mkdtemp(join(tmpdir(), "rea-setup-")); const configPath = join(directory, "mcp.json"); diff --git a/tests/setupLifecycle.test.ts b/tests/setupLifecycle.test.ts index 65a8c625d..06ba37dc2 100644 --- a/tests/setupLifecycle.test.ts +++ b/tests/setupLifecycle.test.ts @@ -4,7 +4,9 @@ import { mkdtemp, mkdir, readFile, + realpath, rm, + symlink, writeFile, } from "node:fs/promises"; import { tmpdir } from "node:os"; @@ -92,6 +94,44 @@ describe("agent lifecycle", () => { ).toEqual({ status: "unchanged" }); }); + it("updates a symlink target without replacing the TOML config symlink", async () => { + const home = await mkdtemp(join(tmpdir(), "rea-toml-symlink-")); + roots.push(home); + const configPath = join(home, ".codex/config.toml"); + const targetPath = join(home, "managed-config.toml"); + const original = 'model = "gpt-5"\n'; + await mkdir(dirname(configPath), { recursive: true }); + await writeFile(targetPath, original); + await symlink(targetPath, configPath); + + expect( + await configureTomlClient( + { name: "codex", configPath, format: "toml" }, + undefined, + ["rea", "mcp"], + ), + ).toMatchObject({ status: "configured" }); + expect((await lstat(configPath)).isSymbolicLink()).toBe(true); + expect(await readFile(`${configPath}.rea.backup`, "utf8")).toBe(original); + expect(await readFile(targetPath, "utf8")).toContain("[mcp_servers.rea]"); + }); + + it("fails before mutation when a TOML config symlink is dangling", async () => { + const home = await mkdtemp(join(tmpdir(), "rea-toml-symlink-")); + roots.push(home); + const configPath = join(home, ".codex/config.toml"); + await mkdir(dirname(configPath), { recursive: true }); + await symlink(join(home, "missing-config.toml"), configPath); + + expect( + await configureTomlClient({ name: "codex", configPath, format: "toml" }), + ).toEqual({ status: "failed", reason: "path" }); + expect((await lstat(configPath)).isSymbolicLink()).toBe(true); + await expect( + readFile(`${configPath}.rea.backup`, "utf8"), + ).rejects.toThrow(); + }); + it("uninstalls only owned entries and refuses purge symlinks", async () => { const home = await mkdtemp(join(tmpdir(), "rea-uninstall-")); roots.push(home); @@ -131,6 +171,52 @@ describe("agent lifecycle", () => { ); }); + it("removes an owned entry through a symlink without replacing it", async () => { + const home = await mkdtemp(join(tmpdir(), "rea-uninstall-symlink-")); + roots.push(home); + const configPath = join(home, ".cursor/mcp.json"); + const targetPath = join(home, "managed-mcp.json"); + const original = JSON.stringify({ + mcpServers: { + rea: { command: "rea", args: ["mcp"] }, + other: { command: "other" }, + }, + }); + await mkdir(dirname(configPath), { recursive: true }); + await writeFile(targetPath, original); + await symlink(targetPath, configPath); + + expect((await runUninstall(false, systemUninstallHost(home))).status).toBe( + "complete", + ); + expect((await lstat(configPath)).isSymbolicLink()).toBe(true); + expect(await readFile(`${configPath}.rea.backup`, "utf8")).toBe(original); + expect(JSON.parse(await readFile(targetPath, "utf8"))).toEqual({ + mcpServers: { other: { command: "other" } }, + }); + }); + + it("fails before mutation when an uninstall config symlink is dangling", async () => { + const home = await mkdtemp(join(tmpdir(), "rea-uninstall-symlink-")); + roots.push(home); + const configPath = join(home, ".cursor/mcp.json"); + await mkdir(dirname(configPath), { recursive: true }); + await symlink(join(home, "missing-mcp.json"), configPath); + + const result = await runUninstall(false, systemUninstallHost(home)); + expect(result.status).toBe("failed"); + expect(result.items).toContainEqual({ + name: "cursor", + status: "failed", + detail: + "Configuration path could not be safely verified. Check its permissions and, if it is a symbolic link, verify that the link resolves to a regular file owned by the current user, then rerun uninstall.", + }); + expect((await lstat(configPath)).isSymbolicLink()).toBe(true); + await expect( + readFile(`${configPath}.rea.backup`, "utf8"), + ).rejects.toThrow(); + }); + it("fails closed on malformed client configuration", async () => { const home = await mkdtemp(join(tmpdir(), "rea-uninstall-bad-")); roots.push(home); @@ -228,6 +314,7 @@ const testFileSystem: UninstallFileSystem = { copy: (source, destination) => copyFile(source, destination), writeText: (path, contents) => writeFile(path, contents), stat: (path) => lstat(path), + realpath: (path) => realpath(path), remove: (path) => rm(path, { recursive: true }), };