diff --git a/packages/coding-agent/src/core/settings-manager.ts b/packages/coding-agent/src/core/settings-manager.ts index 1aea86bab..ce88d9d96 100644 --- a/packages/coding-agent/src/core/settings-manager.ts +++ b/packages/coding-agent/src/core/settings-manager.ts @@ -224,8 +224,7 @@ export class FileSettingsStorage implements SettingsStorage { withLock(scope: SettingsScope, fn: (current: string | undefined) => string | undefined): void { if (scope === "project" && !this.projectConfigTrusted) { - fn(undefined); - return; + throw new Error("Project config is not trusted; refusing to access project settings"); } const path = scope === "global" ? this.globalSettingsPath : this.projectSettingsPath; @@ -273,8 +272,7 @@ export class InMemorySettingsStorage implements SettingsStorage { withLock(scope: SettingsScope, fn: (current: string | undefined) => string | undefined): void { if (scope === "project" && !this.projectConfigTrusted) { - fn(undefined); - return; + throw new Error("Project config is not trusted; refusing to access project settings"); } const current = scope === "global" ? this.global : this.project; @@ -366,6 +364,10 @@ export class SettingsManager { } private static loadFromStorage(storage: SettingsStorage, scope: SettingsScope): Settings { + if (scope === "project" && storage.isProjectConfigTrusted?.() === false) { + return {}; + } + let content: string | undefined; storage.withLock(scope, (current) => { content = current; @@ -531,6 +533,12 @@ export class SettingsManager { } } + private assertProjectConfigTrustedForWrite(): void { + if (!this.projectConfigTrusted) { + throw new Error("Project config is not trusted; refusing to write project settings"); + } + } + private recordError(scope: SettingsScope, error: unknown): void { const normalizedError = error instanceof Error ? error : new Error(String(error)); this.errors.push({ scope, error: normalizedError }); @@ -614,6 +622,7 @@ export class SettingsManager { } private saveProjectSettings(settings: Settings): void { + this.assertProjectConfigTrustedForWrite(); this.projectSettings = structuredClone(settings); this.settings = deepMergeSettings(this.globalSettings, this.projectSettings); @@ -899,6 +908,7 @@ export class SettingsManager { } setProjectPackages(packages: PackageSource[]): void { + this.assertProjectConfigTrustedForWrite(); const projectSettings = structuredClone(this.projectSettings); projectSettings.packages = packages; this.markProjectModified("packages"); @@ -916,6 +926,7 @@ export class SettingsManager { } setProjectExtensionPaths(paths: string[]): void { + this.assertProjectConfigTrustedForWrite(); const projectSettings = structuredClone(this.projectSettings); projectSettings.extensions = paths; this.markProjectModified("extensions"); @@ -933,6 +944,7 @@ export class SettingsManager { } setProjectSkillPaths(paths: string[]): void { + this.assertProjectConfigTrustedForWrite(); const projectSettings = structuredClone(this.projectSettings); projectSettings.skills = paths; this.markProjectModified("skills"); @@ -950,6 +962,7 @@ export class SettingsManager { } setProjectPromptTemplatePaths(paths: string[]): void { + this.assertProjectConfigTrustedForWrite(); const projectSettings = structuredClone(this.projectSettings); projectSettings.prompts = paths; this.markProjectModified("prompts"); @@ -967,6 +980,7 @@ export class SettingsManager { } setProjectThemePaths(paths: string[]): void { + this.assertProjectConfigTrustedForWrite(); const projectSettings = structuredClone(this.projectSettings); projectSettings.themes = paths; this.markProjectModified("themes"); diff --git a/packages/coding-agent/src/core/trust-manager.ts b/packages/coding-agent/src/core/trust-manager.ts index 4b641a343..fb6b9ffab 100644 --- a/packages/coding-agent/src/core/trust-manager.ts +++ b/packages/coding-agent/src/core/trust-manager.ts @@ -51,6 +51,47 @@ function writeTrustFile(path: string, data: TrustFile): void { writeFileSync(path, `${JSON.stringify(sorted, null, 2)}\n`, "utf-8"); } +function acquireTrustLockSync(path: string): () => void { + const trustDir = dirname(path); + mkdirSync(trustDir, { recursive: true }); + const maxAttempts = 10; + const delayMs = 20; + let lastError: unknown; + + for (let attempt = 1; attempt <= maxAttempts; attempt++) { + try { + return lockfile.lockSync(trustDir, { realpath: false, lockfilePath: `${path}.lock` }); + } catch (error) { + const code = + typeof error === "object" && error !== null && "code" in error + ? String((error as { code?: unknown }).code) + : undefined; + if (code !== "ELOCKED" || attempt === maxAttempts) { + throw error; + } + lastError = error; + const start = Date.now(); + while (Date.now() - start < delayMs) { + // Sleep synchronously to avoid changing trust store callers to async. + } + } + } + + if (lastError instanceof Error) { + throw lastError; + } + throw new Error("Failed to acquire trust store lock"); +} + +function withTrustFileLock(path: string, fn: () => T): T { + const release = acquireTrustLockSync(path); + try { + return fn(); + } finally { + release(); + } +} + export function hasProjectConfig(cwd: string): boolean { const resolvedCwd = resolvePath(cwd); return existsSync(join(resolvedCwd, CONFIG_DIR_NAME)); @@ -64,19 +105,15 @@ export class ProjectTrustStore { } get(cwd: string): ProjectTrustDecision { - const data = readTrustFile(this.trustPath); - const value = data[normalizeCwd(cwd)]; - return value === true || value === false ? value : null; + return withTrustFileLock(this.trustPath, () => { + const data = readTrustFile(this.trustPath); + const value = data[normalizeCwd(cwd)]; + return value === true || value === false ? value : null; + }); } set(cwd: string, decision: ProjectTrustDecision): void { - const trustDir = dirname(this.trustPath); - mkdirSync(trustDir, { recursive: true }); - let release: (() => void) | undefined; - try { - // Lock before reading or creating trust.json so malformed content cannot be - // silently replaced by a concurrent or follow-up write. - release = lockfile.lockSync(trustDir, { realpath: false, lockfilePath: `${this.trustPath}.lock` }); + withTrustFileLock(this.trustPath, () => { const data = readTrustFile(this.trustPath); const key = normalizeCwd(cwd); if (decision === null) { @@ -85,8 +122,6 @@ export class ProjectTrustStore { data[key] = decision; } writeTrustFile(this.trustPath, data); - } finally { - release?.(); - } + }); } } diff --git a/packages/coding-agent/test/settings-manager.test.ts b/packages/coding-agent/test/settings-manager.test.ts index 7fc6725d1..829665961 100644 --- a/packages/coding-agent/test/settings-manager.test.ts +++ b/packages/coding-agent/test/settings-manager.test.ts @@ -268,6 +268,20 @@ describe("SettingsManager", () => { expect(manager.getTheme()).toBe("global"); expect(manager.getProjectSettings()).toEqual({}); }); + + it("should fail project settings writes when project config is not trusted", async () => { + const projectSettingsPath = join(projectDir, ".pi", "settings.json"); + writeFileSync(projectSettingsPath, JSON.stringify({ packages: ["npm:existing"] })); + const manager = SettingsManager.create(projectDir, agentDir, { projectConfigTrusted: false }); + + expect(() => manager.setProjectPackages(["npm:new"])).toThrow( + "Project config is not trusted; refusing to write project settings", + ); + await manager.flush(); + + expect(manager.getProjectSettings()).toEqual({}); + expect(JSON.parse(readFileSync(projectSettingsPath, "utf-8"))).toEqual({ packages: ["npm:existing"] }); + }); }); describe("httpIdleTimeoutMs", () => { diff --git a/packages/coding-agent/test/trust-manager.test.ts b/packages/coding-agent/test/trust-manager.test.ts index ceed813ce..c2c7a8813 100644 --- a/packages/coding-agent/test/trust-manager.test.ts +++ b/packages/coding-agent/test/trust-manager.test.ts @@ -1,6 +1,7 @@ import { mkdirSync, readFileSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; +import lockfile from "proper-lockfile"; import { afterEach, beforeEach, describe, expect, it } from "vitest"; import { hasProjectConfig, ProjectTrustStore } from "../src/core/trust-manager.ts"; @@ -43,6 +44,29 @@ describe("ProjectTrustStore", () => { expect(readFileSync(trustPath, "utf-8")).toBe("{not json"); }); + it("does not read trust.json while another process holds the trust lock", () => { + const trustPath = join(agentDir, "trust.json"); + writeFileSync(trustPath, "{partial", "utf-8"); + const release = lockfile.lockSync(agentDir, { realpath: false, lockfilePath: `${trustPath}.lock` }); + const store = new ProjectTrustStore(agentDir); + + try { + let error: unknown; + try { + store.get(cwd); + } catch (caught) { + error = caught; + } + + expect(error).toBeInstanceOf(Error); + expect((error as { code?: unknown }).code).toBe("ELOCKED"); + } finally { + release(); + } + + expect(() => store.get(cwd)).toThrow(/Failed to read trust store/); + }); + it("detects .pi project config directories", () => { expect(hasProjectConfig(cwd)).toBe(false);