mirror of
https://github.com/earendil-works/pi.git
synced 2026-06-18 15:54:04 +08:00
fix(coding-agent): harden project trust persistence
This commit is contained in:
@@ -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");
|
||||
|
||||
@@ -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<T>(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?.();
|
||||
}
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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", () => {
|
||||
|
||||
@@ -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);
|
||||
|
||||
|
||||
Reference in New Issue
Block a user