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..9b06615 100644 --- a/src/policy.mjs +++ b/src/policy.mjs @@ -22,33 +22,55 @@ 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) { + // 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 { - 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..14f696a --- /dev/null +++ b/test/policy-override.test.mjs @@ -0,0 +1,143 @@ +/** + * 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 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", () => { + 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); + }); +});