From 17ace9e6724431c5895b1d97dbcb4dea417598bc Mon Sep 17 00:00:00 2001 From: latenighthackathon Date: Sun, 31 May 2026 23:23:57 +0000 Subject: [PATCH] fix(policy): refuse to apply a preset when the live policy could not be read applyPresetContent and applyPresets fetch the live policy with runCapture(..., { ignoreError: true }) and pass it through parseCurrentPolicy, which returns "" both for a genuinely empty policy and for exit-0-but-degraded output (an error/warning/status body, a non-object top level, or unparseable YAML). On a degraded read mergePresetIntoPolicy rebuilt the policy from only the new preset and policy set overwrote the live policy, dropping every other preset and every non-network section. The remove path already guards this; the apply paths did not. Abort the apply when rawPolicy is non-empty but parseCurrentPolicy returns empty (a degraded read), while still letting a genuinely empty read build from the scaffold (fresh sandbox). This mirrors the existing remove-path guard. Fixes #4586 Signed-off-by: latenighthackathon --- src/lib/policy/index.ts | 12 +++++ test/policies.test.ts | 99 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 111 insertions(+) diff --git a/src/lib/policy/index.ts b/src/lib/policy/index.ts index e7218398de..20e4b79a94 100644 --- a/src/lib/policy/index.ts +++ b/src/lib/policy/index.ts @@ -784,6 +784,12 @@ function applyPresetContent( } const currentPolicy = parseCurrentPolicy(rawPolicy); + if (rawPolicy.trim() && !currentPolicy) { + console.error( + ` Could not read the current policy for sandbox '${sandboxName}'; refusing to apply '${presetName}' to avoid overwriting it.`, + ); + return false; + } const merged = mergePresetIntoPolicy(currentPolicy, presetEntries); const endpoints = getPresetEndpoints(presetContent); @@ -882,6 +888,12 @@ function applyPresets(sandboxName: string, presetNames: string[]): boolean { } let merged = parseCurrentPolicy(rawPolicy); + if (rawPolicy.trim() && !merged) { + console.error( + ` Could not read the current policy for sandbox '${sandboxName}'; refusing to apply presets to avoid overwriting it.`, + ); + return false; + } const endpointLogs: string[][] = []; for (const presetName of uniquePresetNames) { diff --git a/test/policies.test.ts b/test/policies.test.ts index efafe5b48b..cd8599deb9 100644 --- a/test/policies.test.ts +++ b/test/policies.test.ts @@ -1033,6 +1033,105 @@ exit 1 }); }); + describe("issue 4586: preset apply must not overwrite a live policy that could not be read", () => { + const registryModule = requireForTest( + path.join(REPO_ROOT, "dist", "lib", "state", "registry.js"), + ) as Record; + const CUSTOM = "network_policies:\n example:\n host: example.com\n"; + const DEGRADED = '#!/bin/sh\nif [ "$1" = "policy" ] && [ "$2" = "get" ]; then echo "error: gateway is restarting"; fi\nexit 0\n'; + const EMPTY_OK = "#!/bin/sh\nexit 0\n"; + let tmpHome: string; + let fakeOpenshell: string; + let origHome: string | undefined; + let resolveSpy: ReturnType; + let savedGetSandbox: any; + let savedAddCustomPolicy: any; + + beforeEach(() => { + tmpHome = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-issue4586-")); + const localBin = path.join(tmpHome, ".local", "bin"); + fs.mkdirSync(localBin, { recursive: true }); + fakeOpenshell = path.join(localBin, "openshell"); + origHome = process.env.HOME; + process.env.HOME = tmpHome; + resolveSpy = vi + .spyOn(resolveOpenshellModule, "resolveOpenshell") + .mockReturnValue(fakeOpenshell); + savedGetSandbox = registryModule.getSandbox; + savedAddCustomPolicy = registryModule.addCustomPolicy; + registryModule.getSandbox = (name: string) => ({ name }); + registryModule.addCustomPolicy = () => true; + }); + + afterEach(() => { + if (origHome === undefined) delete process.env.HOME; + else process.env.HOME = origHome; + resolveSpy.mockRestore(); + registryModule.getSandbox = savedGetSandbox; + registryModule.addCustomPolicy = savedAddCustomPolicy; + fs.rmSync(tmpHome, { recursive: true, force: true }); + }); + + it("aborts applyPresetContent (returns false) when policy get exits 0 with degraded output", () => { + fs.writeFileSync(fakeOpenshell, DEGRADED, { mode: 0o755 }); + const errs: string[] = []; + const errSpy = vi.spyOn(console, "error").mockImplementation((...a: unknown[]) => { + errs.push(a.map((x) => String(x)).join(" ")); + }); + const logs: string[] = []; + const logSpy = vi.spyOn(console, "log").mockImplementation((...a: unknown[]) => { + logs.push(a.map((x) => String(x)).join(" ")); + }); + try { + const result = policies.applyPresetContent("alpha", "my-custom", CUSTOM, { + custom: { sourcePath: "/tmp/x.yaml" }, + }); + expect(result).toBe(false); + expect(errs.join("\n")).toMatch(/[Cc]ould not read the current policy/); + expect(logs.join("\n")).not.toContain("Applied preset:"); + } finally { + errSpy.mockRestore(); + logSpy.mockRestore(); + } + }); + + it("still applies applyPresetContent when policy get returns an empty policy (fresh sandbox)", () => { + fs.writeFileSync(fakeOpenshell, EMPTY_OK, { mode: 0o755 }); + const logs: string[] = []; + const logSpy = vi.spyOn(console, "log").mockImplementation((...a: unknown[]) => { + logs.push(a.map((x) => String(x)).join(" ")); + }); + const errSpy = vi.spyOn(console, "error").mockImplementation(() => undefined); + try { + const result = policies.applyPresetContent("alpha", "my-custom", CUSTOM, { + custom: { sourcePath: "/tmp/x.yaml" }, + }); + expect(result).toBe(true); + expect(logs.join("\n")).toContain("Applied preset:"); + } finally { + logSpy.mockRestore(); + errSpy.mockRestore(); + } + }); + + it("aborts applyPresets (returns false) when policy get exits 0 with degraded output", () => { + fs.writeFileSync(fakeOpenshell, DEGRADED, { mode: 0o755 }); + const errs: string[] = []; + const errSpy = vi.spyOn(console, "error").mockImplementation((...a: unknown[]) => { + errs.push(a.map((x) => String(x)).join(" ")); + }); + const logSpy = vi.spyOn(console, "log").mockImplementation(() => undefined); + try { + const result = policies.applyPresets("alpha", ["npm"]); + expect(result).toBe(false); + expect(errs.join("\n")).toMatch(/[Cc]ould not read the current policy/); + } finally { + errSpy.mockRestore(); + logSpy.mockRestore(); + } + }); + }); + describe("extractPresetEntries", () => { it("returns null for null input", () => { expect(policies.extractPresetEntries(null)).toBe(null);