From eb3c91a02bf9539a3ee965d723cb8d26e3b4e708 Mon Sep 17 00:00:00 2001 From: ColinkaMir <72103708+ColinkaMir@users.noreply.github.com> Date: Fri, 25 Sep 2026 10:07:02 +0000 Subject: [PATCH 1/2] Fail startup when NAD_POLICY names a file that is not there A missing policy file is only optional when nobody asked for that file. NAD_POLICY is an operator saying use this one, so a typo in it started the agent with no limits and no allowlist, which is the same silently-permit-everything state a malformed file already refuses to cause. loadPolicy could not tell the two cases apart because both arrived as the same path argument: process.env.NAD_POLICY || DEFAULT_PATH was resolved in the parameter default. The env override is now read inside the function, so ENOENT is refused only when a nonempty NAD_POLICY selected the file, and the default being absent still returns null. A dangling symlink needs no arm of its own: the open follows the link and misses the target, so it arrives as the same ENOENT, which a test now shows rather than a comment claiming it. It throws, and cli.mjs already catches a throw from loadPolicy, prints it and exits 1 before the wallet is touched. src/mcp.mjs follows the same rule and gets the behaviour with it. An explicit argument keeps its older meaning, because callers that pass a path have always owned the result and the suite depends on that. One existing harness had to move with this. test/native-tools-cli-exit.test.mjs pointed NAD_POLICY at an absent file to mean no policy, which is now an error; it writes an empty policy object instead, which keeps that harness's intent of a run with no rules. Three of the eight new tests fail on main: the refusal, the dangling symlink and the startup ordering. The other five pin behaviour that must not change, and they pass before and after. --- README.md | 4 +- src/policy.mjs | 33 ++++++-- test/native-tools-cli-exit.test.mjs | 9 ++- test/policy-override.test.mjs | 116 ++++++++++++++++++++++++++++ 4 files changed, 151 insertions(+), 11 deletions(-) create mode 100644 test/policy-override.test.mjs diff --git a/README.md b/README.md index 4a6e131..52cc734 100644 --- a/README.md +++ b/README.md @@ -58,7 +58,9 @@ the wallet enforces it before every confirmation prompt: rather than silently skipping a rule the wallet cannot evaluate. Every field is optional and **no file means no policy**, so behavior is unchanged unless you opt -in. A violation is refused before the prompt, naming the rule +in. The one exception is a file you asked for: if `NAD_POLICY` names a path that does not exist, +startup stops and says so, because a typo there would otherwise run the agent with none of the +limits you configured. A violation is refused before the prompt, naming the rule (`policy maxPerSend: amount 0.9 MON is above the policy limit of 0.5 MON per send.`), and the confirmation block gains a `Policy:` line showing what applied. A malformed policy file stops the agent at startup instead of being ignored. diff --git a/src/policy.mjs b/src/policy.mjs index 1fea5b4..6222b70 100644 --- a/src/policy.mjs +++ b/src/policy.mjs @@ -22,33 +22,50 @@ const DEFAULT_PATH = resolve(HERE, "..", "policy.json"); * Load and validate `policy.json` (override with NAD_POLICY). Returns null when * no file exists. Throws on a malformed policy: a policy that cannot be parsed * must stop the agent, never silently permit everything. + * + * A missing file is only optional when nobody asked for that file. `NAD_POLICY` is an operator + * saying "use this one", so a typo in it used to start the agent with no limits and no allowlist + * at all — the same silent everything-permitted state a malformed file already refuses to cause. + * An explicit argument keeps its older meaning: the caller named the path and owns the result. */ -export function loadPolicy(path = process.env.NAD_POLICY || DEFAULT_PATH) { +export function loadPolicy(path) { + const override = (process.env.NAD_POLICY ?? "").trim(); + const selected = path ?? (override || DEFAULT_PATH); + const isOverride = path === undefined && override !== ""; let raw; try { - raw = readFileSync(path, "utf8"); + raw = readFileSync(selected, "utf8"); } catch (err) { - if (err.code === "ENOENT") return null; - throw new Error(`policy file ${path} could not be read: ${err.message}`); + if (err.code === "ENOENT") { + // A dangling symlink lands here too: the open follows the link and misses its target. + if (isOverride) { + throw new Error( + `policy file ${selected} does not exist, and NAD_POLICY points at it: ` + + `create the file or unset NAD_POLICY to run without a policy`, + ); + } + return null; + } + throw new Error(`policy file ${selected} could not be read: ${err.message}`); } let parsed; try { parsed = JSON.parse(raw); } catch (err) { - throw new Error(`policy file ${path} is not valid JSON: ${err.message}`); + throw new Error(`policy file ${selected} is not valid JSON: ${err.message}`); } if (!parsed || typeof parsed !== "object" || Array.isArray(parsed)) { - throw new Error(`policy file ${path} must contain a JSON object`); + throw new Error(`policy file ${selected} must contain a JSON object`); } const KNOWN_KEYS = new Set(["maxPerSend", "maxPerSession", "allowlist"]); const unknown = Object.keys(parsed).filter((k) => !KNOWN_KEYS.has(k)); if (unknown.length) { - throw new Error(`policy file ${path} contains unknown key${unknown.length > 1 ? "s" : ""}: ${unknown.join(", ")}`); + throw new Error(`policy file ${selected} contains unknown key${unknown.length > 1 ? "s" : ""}: ${unknown.join(", ")}`); } - const policy = { path, maxPerSend: null, maxPerSession: null, allowlist: null }; + const policy = { path: selected, maxPerSend: null, maxPerSession: null, allowlist: null }; for (const key of ["maxPerSend", "maxPerSession"]) { if (parsed[key] == null) continue; diff --git a/test/native-tools-cli-exit.test.mjs b/test/native-tools-cli-exit.test.mjs index 0c4fa16..515c1c5 100644 --- a/test/native-tools-cli-exit.test.mjs +++ b/test/native-tools-cli-exit.test.mjs @@ -16,7 +16,7 @@ import { describe, test, beforeEach } from "node:test"; import assert from "node:assert/strict"; import { execFileSync } from "node:child_process"; -import { existsSync, mkdtempSync } from "node:fs"; +import { existsSync, mkdtempSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { fileURLToPath, pathToFileURL } from "node:url"; @@ -291,6 +291,8 @@ const SHIM_REGISTER_URL = pathToFileURL(SHIM_REGISTER).href; // derives locally from it (dry-run); the startup balance read is best-effort. const TEST_SEED = "abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about"; const shimStateDir = mkdtempSync(join(tmpdir(), "nad-shim-")); +const EMPTY_POLICY = join(shimStateDir, "empty-policy.json"); +writeFileSync(EMPTY_POLICY, "{}"); function runRealCli(scenario) { assert.ok(existsSync(DIST_CLI), "dist/cli.mjs missing — run npm run build first (CI builds before testing)"); @@ -301,7 +303,10 @@ function runRealCli(scenario) { USE_NATIVE_TOOLS: "true", MONAD_NETWORK: "testnet", NAD_STATE_PATH: join(shimStateDir, `${scenario}.json`), - NAD_POLICY: join(shimStateDir, "no-policy.json"), // absent → no policy + // An empty policy object rather than an absent file: a nonempty NAD_POLICY now means the + // operator chose that file, and a missing one stops startup. `{}` keeps this harness's + // intent, which is a run with no rules rather than a run with no policy file. + NAD_POLICY: EMPTY_POLICY, NAD_MCP_CONFIG: join(shimStateDir, "no-mcp.json"), // absent → no servers NAD_SHIM_SCENARIO: scenario, NO_COLOR: "1", diff --git a/test/policy-override.test.mjs b/test/policy-override.test.mjs new file mode 100644 index 0000000..bdffa5a --- /dev/null +++ b/test/policy-override.test.mjs @@ -0,0 +1,116 @@ +/** + * Issue #108: an explicit NAD_POLICY that names a missing file used to start the agent with no + * limits and no allowlist at all. + * + * The distinction these tests pin is not "does the file exist" but "did anybody ask for it". + * A default that is absent stays optional, because that is the documented way to run without a + * policy; a nonempty NAD_POLICY is an operator choosing a file, and a typo in it must stop the + * agent rather than quietly remove every rule. Offline throughout: no wallet, no network. + */ + +import { describe, it, afterEach } from "node:test"; +import assert from "node:assert/strict"; +import { execFileSync } from "node:child_process"; +import { mkdtempSync, symlinkSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { fileURLToPath } from "node:url"; + +import { loadPolicy } from "../src/policy.mjs"; + +const DIR = mkdtempSync(join(tmpdir(), "nad-policy-override-")); +const MISSING = join(DIR, "not-here.json"); +const VALID = join(DIR, "valid.json"); +const BROKEN = join(DIR, "broken.json"); +writeFileSync(VALID, JSON.stringify({ maxPerSend: "0.5" })); +writeFileSync(BROKEN, "{ not json"); + +const previous = process.env.NAD_POLICY; +const restore = () => { + if (previous === undefined) delete process.env.NAD_POLICY; + else process.env.NAD_POLICY = previous; +}; + +describe("loadPolicy — an explicit override that is not there", () => { + afterEach(restore); + + it("refuses a missing file the operator named, and names it", () => { + process.env.NAD_POLICY = MISSING; + assert.throws(() => loadPolicy(), (err) => { + assert.match(err.message, /does not exist/); + assert.ok(err.message.includes(MISSING), `the path has to be in the message: ${err.message}`); + return true; + }); + }); + + it("still returns null when nobody named a file", () => { + // The documented way to run without limits: no override, and the default is absent. This is + // the behaviour the refusal above must not swallow. + delete process.env.NAD_POLICY; + assert.equal(loadPolicy(join(DIR, "no-default-here", "policy.json")), null); + }); + + it("treats an empty or blank override as no override", () => { + for (const value of ["", " "]) { + process.env.NAD_POLICY = value; + assert.equal(loadPolicy(join(DIR, "no-default-here", "policy.json")), null); + } + }); + + it("still loads an override that exists", () => { + process.env.NAD_POLICY = VALID; + const policy = loadPolicy(); + assert.equal(policy.path, VALID); + assert.ok(policy.maxPerSend > 0n); + }); + + it("still reports a malformed override as malformed, not as missing", () => { + process.env.NAD_POLICY = BROKEN; + assert.throws(() => loadPolicy(), /not valid JSON/); + }); + + it("treats a dangling symlink as missing", (t) => { + const link = join(DIR, "dangling.json"); + try { + symlinkSync(join(DIR, "target-that-never-existed.json"), link); + } catch { + // Windows refuses symlinks without privileges; the branch is the same ENOENT either way. + t.skip("symlinks unavailable on this platform"); + return; + } + process.env.NAD_POLICY = link; + assert.throws(() => loadPolicy(), /does not exist/); + }); + + it("keeps the older contract for an explicit argument", () => { + // Callers that pass a path have always owned the consequences, and the existing suite + // depends on it. Only the environment override changed meaning. + delete process.env.NAD_POLICY; + assert.equal(loadPolicy(MISSING), null); + }); +}); + +describe("startup — the refusal happens before the wallet", () => { + it("exits nonzero and never initialises the wallet", () => { + const cli = fileURLToPath(new URL("../dist/cli.mjs", import.meta.url)); + const env = { ...process.env, NAD_POLICY: MISSING, NO_COLOR: "1" }; + delete env.NAD_CLI_NO_RUN; + + let status = 0; + let output = ""; + try { + output = execFileSync(process.execPath, [cli], { + env, input: "", encoding: "utf8", timeout: 120000, stdio: ["pipe", "pipe", "pipe"], + }); + } catch (err) { + status = err.status; + output = `${err.stdout ?? ""}${err.stderr ?? ""}`; + } + + assert.equal(status, 1, `expected exit 1, transcript:\n${output}`); + assert.match(output, /does not exist/); + // The ordering is the point: a policy the operator meant to apply must stop the run before + // anything can be signed, so the wallet line must never appear. + assert.doesNotMatch(output, /initializing wallet/i); + }); +}); From 2cf68e298197577a3a61b4caf8156344aad1e176 Mon Sep 17 00:00:00 2001 From: ColinkaMir <72103708+ColinkaMir@users.noreply.github.com> Date: Fri, 25 Sep 2026 11:50:40 +0000 Subject: [PATCH 2/2] Open the configured path literally, and stop calling a blank override empty Review catch, and a real one: trimming NAD_POLICY before opening it can select a different file. On Linux 'policy.json ' and 'policy.json' are two paths, so a configured file carrying whitespace was swapped for its neighbour, and a policy with rules could quietly become one without any. The value is now used exactly as configured. The blank case moves with it. A whitespace-only override is not 'no override', it is a path somebody typed, so it takes the missing-file refusal like any other nonempty value. Only a completely empty or unset variable falls back to the optional default. The test that claimed to cover this passed an explicit path argument, which takes the other branch entirely and proved nothing about environment selection. It is replaced by three that drive the environment: an empty override falling back, a whitespace-only override refused, and the reviewer's own pair of files one trailing space apart, asserting that the file named is the file opened and that its rules survive. That last one skips on Win32, where the platform strips trailing spaces from path components. Both new cases fail on the previous commit. --- src/policy.mjs | 9 +++++++-- test/policy-override.test.mjs | 35 +++++++++++++++++++++++++++++++---- 2 files changed, 38 insertions(+), 6 deletions(-) diff --git a/src/policy.mjs b/src/policy.mjs index 6222b70..9b06615 100644 --- a/src/policy.mjs +++ b/src/policy.mjs @@ -29,8 +29,13 @@ const DEFAULT_PATH = resolve(HERE, "..", "policy.json"); * An explicit argument keeps its older meaning: the caller named the path and owns the result. */ export function loadPolicy(path) { - const override = (process.env.NAD_POLICY ?? "").trim(); - const selected = path ?? (override || DEFAULT_PATH); + // The value is used exactly as configured, never trimmed: a filesystem path may legitimately + // carry leading or trailing whitespace, and trimming it opens a DIFFERENT file. On Linux + // `policy.json ` and `policy.json` are two paths, so a trim could quietly swap a policy with + // rules for one without any. Only a completely empty value means "no override"; a blank one is + // a path the operator typed, and if nothing is there the startup has to say so. + const override = process.env.NAD_POLICY ?? ""; + const selected = path ?? (override !== "" ? override : DEFAULT_PATH); const isOverride = path === undefined && override !== ""; let raw; try { diff --git a/test/policy-override.test.mjs b/test/policy-override.test.mjs index bdffa5a..14f696a 100644 --- a/test/policy-override.test.mjs +++ b/test/policy-override.test.mjs @@ -50,11 +50,38 @@ describe("loadPolicy — an explicit override that is not there", () => { assert.equal(loadPolicy(join(DIR, "no-default-here", "policy.json")), null); }); - it("treats an empty or blank override as no override", () => { - for (const value of ["", " "]) { - process.env.NAD_POLICY = value; - assert.equal(loadPolicy(join(DIR, "no-default-here", "policy.json")), null); + it("treats only a completely empty override as no override", () => { + // Driven through the environment on purpose. An earlier version of this test passed an + // explicit path argument, which takes the other branch entirely and proved nothing about + // the environment selection it claimed to cover. + process.env.NAD_POLICY = ""; + assert.equal(loadPolicy(), null, "an empty override falls back to the optional default"); + }); + + it("refuses a whitespace-only override, because it is a path that is not there", () => { + // A blank string is not "no override": it is a path the operator typed. #108 asks for a + // nonempty override naming a missing file to stop startup, and " " is nonempty. + process.env.NAD_POLICY = " "; + assert.throws(() => loadPolicy(), /does not exist/); + }); + + it("opens the configured path literally, whitespace and all", (t) => { + // The review case: two files one trailing space apart. Trimming the configured value opens + // the wrong one and silently drops every rule the operator meant to apply. + if (process.platform === "win32") { + t.skip("Win32 strips trailing spaces from path components"); + return; } + const plain = join(DIR, "pair.json"); + const spaced = `${plain} `; + writeFileSync(plain, JSON.stringify({})); + writeFileSync(spaced, JSON.stringify({ maxPerSend: "0.5" })); + + process.env.NAD_POLICY = spaced; + const policy = loadPolicy(); + + assert.equal(policy.path, spaced, "the file named is the file opened"); + assert.ok(policy.maxPerSend > 0n, "the rules from the configured file have to survive"); }); it("still loads an override that exists", () => {