a3d77a27bcbafb8bb7aecdb1dd895c51ab50fbd5
- Author
- TheEdgeOfRage <git@theedgeofrage.com>
- Committer
- TheEdgeOfRage <git@theedgeofrage.com>
- Date
Message
Harden Bash policy checks
Diff
This diff is truncated to protect this page.
1diff --git a/README.md b/README.md
2index 847e959b19117660a510c07973a31dae97887a0c..d6625ec14b0c259d85a48e25b92f533c01126927 100644
3--- a/README.md
4+++ b/README.md
5@@ -88,7 +88,7 @@ Pi uses a root-level `tools` map and `externalDirectories` list. Any tool name c
6 }
7 ```
8
9diff --git a/src/core/deterministic.ts b/src/core/deterministic.ts
10index 20a72afd5e0218752d668d4fa0f278e04a4d53f7..d6032f4cfe40476de565855f92a74edbd2c68839 100644
11--- a/src/core/deterministic.ts
12+++ b/src/core/deterministic.ts
13@@ -1,3 +1,4 @@
14+import { normalize as normalizePath } from "node:path";
15 import type { Decision } from "./types";
16 import { ALLOW_COMMANDS, ASK_PATTERNS, SHELL_CONTROL_RE, STDERR_REDIRECT_RE, DEVNULL_REDIRECT_RE } from "./rules";
17 import { stripRtkPrefix, expandHome } from "./normalize";
18@@ -87,13 +88,103 @@ function shellWords(command: string): string[] {
19 });
20 }
21
22+function variableCharacter(character: string | undefined): boolean {
23+ return character !== undefined && /[A-Za-z0-9_]/.test(character);
24+}
25+
26+function hasUnsafeWordSyntax(command: string): boolean {
27+ let quote: "'" | '"' | undefined;
28+ let wordStarted = false;
29+ let quotedWord = false;
30+
31+ for (let index = 0; index < command.length; index += 1) {
32+ const character = command[index];
33+ const nextCharacter = command[index + 1];
34+
35+ if (quote === "'") {
36+ if (character === "'") {
37+ quote = undefined;
38+ continue;
39+ }
40+ if (character === "$" || character === "~") return true;
41+ continue;
42+ }
43+
44+ if (quote === '"') {
45+ if (character === '"') {
46+ quote = undefined;
47+ continue;
48+ }
49+ if (character === "\\") {
50+ if (nextCharacter === '"' || nextCharacter === "$" || nextCharacter === "`" || nextCharacter === "\\") {
51+ return true;
52+ }
53+ index += 1;
54+ continue;
55+ }
56+ if (character === "`") return true;
57+ if (character === "$") {
58+ if (command.slice(index + 1, index + 5) !== "HOME" || variableCharacter(command[index + 5])) return true;
59+ index += 4;
60+ continue;
61+ }
62+ if (character === "~") return true;
63+ continue;
64+ }
65+
66+ if (/\s/.test(character)) {
67+ quote = undefined;
68+ wordStarted = false;
69+ quotedWord = false;
70+ continue;
71+ }
72+ if (quotedWord) return true;
73+ if (character === "'" || character === '"') {
74+ if (wordStarted) return true;
75+ quote = character;
76+ wordStarted = true;
77+ quotedWord = true;
78+ continue;
79+ }
80+ if (character === "\\") {
81+ if (nextCharacter !== "(" && nextCharacter !== ")") return true;
82+ index += 1;
83+ wordStarted = true;
84+ continue;
85+ }
86+ if (character === "`" || SHELL_CONTROL_RE.test(character) || ["{", "}", "(", ")"].includes(character)) return true;
87+ if (character === "$") {
88+ if (command.slice(index + 1, index + 5) !== "HOME" || variableCharacter(command[index + 5])) return true;
89+ index += 4;
90+ wordStarted = true;
91+ continue;
92+ }
93+ if (character === "*") return true;
94+ if (character === "?") return true;
95+ if (character === "[") return true;
96+ if (character === "~") {
97+ if (wordStarted || (nextCharacter !== undefined && nextCharacter !== "/" && !/\s/.test(nextCharacter))) {
98+ return true;
99+ }
100+ }
101+ wordStarted = true;
102+ }
103+
104+ return quote !== undefined;
105+}
106+
107 function localPath(word: string): string {
108 const path = word.replace(/^@/, "").replace(/^file:\/\//, "");
109 return (path === "/" ? path : path.replace(/\/+$/, "")).replace(/[;,)]+$/, "");
110 }
111
112-function isSecretPath(word: string): boolean {
113diff --git a/src/core/normalize.ts b/src/core/normalize.ts
114index adb10b497067578f98038613766115bcf0081be6..d2488d68d5c290a70f47a58d9c9ecd18feb4d8b8 100644
115--- a/src/core/normalize.ts
116+++ b/src/core/normalize.ts
117@@ -8,6 +8,8 @@ const SENSITIVE_ARGUMENT_RE =
118 /((?:^|\s)(?:-u|-H)(?:=|\s*)|(?:^|\s)--(?:api[-_]?key|authorization|cookie|password|secret|token|user|proxy-user|header)(?:=|\s+))(?:(?:"(?:[^"\\]|\\.)*")|(?:'(?:[^'\\]|\\.)*')|\S+)/gi;
119 const URL_USERINFO_RE = /([a-z][a-z\d+.-]*:\/\/)[^/\s@]+@/gi;
120 const SENSITIVE_QUERY_RE = /([?&](?:api[-_]?key|authorization|credential|cookie|password|secret|token)=)[^&\s]*/gi;
121+const SENSITIVE_ENVIRONMENT_RE =
122diff --git a/src/core/rules.ts b/src/core/rules.ts
123index 9d1594461d63255105b7885b777a4bb235ee8018..eeb2be5615b1327d16ee50330a57a391120ef989 100644
124--- a/src/core/rules.ts
125+++ b/src/core/rules.ts
126@@ -112,6 +112,7 @@ export const ASK_PATTERNS: RegExp[] = [
127 /^herdr\s+server\s+stop\s*$/,
128 /^git(?:\s+-C\s+\S+)*\s+reset(?:\s+\S+)*\s+--hard(?:\s|$)/,
129 /^git(?:\s+-C\s+\S+)*\s+checkout\s+--(?:\s|$)/,
130+ /^git(?:\s+-C\s+\S+)*\s+checkout(?:\s+\S+)+\s+--(?:\s|$)/,
131 /^git(?:\s+-C\s+\S+)*\s+(?:clean|restore)(?:\s|$)/,
132 /^git(?:\s+-C\s+\S+)*\s+config\s+--system(?:\s|$)/,
133 ];
134@@ -169,11 +170,12 @@ Respond with ONLY this JSON, WITHOUT markdown formatting, otherwise this program
135 }`;
136
137 export function llmPolicyPrompt(externalDirectories: readonly string[] = []): string {
138+ const nonEmptyDirectories = externalDirectories.filter((pattern) => pattern.length > 0);
139 const directories =
140- externalDirectories.length === 0
141+ nonEmptyDirectories.length === 0
142 ? " - No additional directories."
143- : `${externalDirectories.map((pattern) => ` - ${JSON.stringify(pattern)}`).join("\n")}
144+ : `${nonEmptyDirectories.map((pattern) => ` - ${JSON.stringify(pattern)}`).join("\n")}
145
146-Treat these as trusted policy rules. Allow access when a tool-call path starts with a listed path. This overrides the general directory guidance above.`;
147+Treat these as trusted policy rules. Allow access when a tool-call path is the listed path or is nested below it by path components. Do not match sibling paths that only share a string prefix. This overrides the general directory guidance above.`;
148 return LLM_POLICY_PROMPT.replace(EXTERNAL_DIRECTORIES_PLACEHOLDER, directories);
149 }
150diff --git a/src/pi/bash-split.ts b/src/pi/bash-split.ts
151index c5ecaa153fc3d6d9bb337b5776630424aca66dc1..de0e73f73c298b275b21856767c11ce3ddbb94b0 100644
152--- a/src/pi/bash-split.ts
153+++ b/src/pi/bash-split.ts
154@@ -30,14 +30,37 @@ function redirectNodes(command: Node): Node[] {
155 return redirects;
156 }
157
158+function safeHomeExpansion(value: string): boolean {
159+ for (let index = 0; index < value.length; index += 1) {
160+ if (value[index] !== "$") continue;
161+ if (value.slice(index + 1, index + 5) !== "HOME" || /[A-Za-z0-9_]/.test(value[index + 5] ?? "")) return false;
162+ index += 4;
163+ }
164+ return true;
165+}
166+
167 function redirectPath(node: Node): string | undefined {
168 const destination = node.childForFieldName("destination")?.text.trim();
169- if (!destination || /[$`]/.test(destination)) return undefined;
170+ if (!destination) return undefined;
171+
172+ const quote = destination[0];
173+ if (quote === "'" || quote === '"') {
174+ if (destination.at(-1) !== quote) return undefined;
175+ const path = destination.slice(1, -1);
176+ if (path.includes("\\") || path.includes("`") || path.includes("~")) return undefined;
177+ if (quote === '"' && !safeHomeExpansion(path)) return undefined;
178+ if (quote === "'" && path.includes("$HOME")) return undefined;
179+ return path;
180+ }
181+
182 if (
183- (destination.startsWith('"') && destination.endsWith('"')) ||
184- (destination.startsWith("'") && destination.endsWith("'"))
185+ [...destination].some(
186+ (character) => /\s/.test(character) || ["'", '"', "\\", "`", "*", "?", "["].includes(character),
187+ ) ||
188+ !safeHomeExpansion(destination) ||
189+ (destination.includes("~") && !(destination === "~" || destination.startsWith("~/")))
190 ) {
191- return destination.slice(1, -1);
192+ return undefined;
193 }
194 return destination;
195 }
196@@ -82,16 +105,12 @@ export async function splitBashCommand(
197 if (!tree || tree.rootNode.hasError) return raw(command);
198 try {
199 const commands: BashSplitCommand[] = [];
200- const seen = new Set<string>();
201 for (const node of tree.rootNode.descendantsOfType("command")) {
202 if (!node) continue;
203 const source = node.text.trim();
204- if (source && !seen.has(source)) {
205- seen.add(source);
206- commands.push({ source, redirects: redirects(node) });
207- }
208+ if (source) commands.push({ source, redirects: redirects(node) });
209 }
210- return { commands, parsed: true };
211+ return commands.length === 0 && command.trim() ? raw(command) : { commands, parsed: true };
212 } finally {
213 tree.delete();
214 }
215diff --git a/src/pi/config.ts b/src/pi/config.ts
216index ada564c0862437dc4378276d0517941ee3d2aa43..0915976cc2abd5f5f7a52bda9a7997092a13ae43 100644
217--- a/src/pi/config.ts
218+++ b/src/pi/config.ts
219@@ -47,8 +47,8 @@ function toolPermissions(value: unknown): Record<string, PermissionAction> {
220 }
221
222 function externalDirectories(value: unknown): ExternalDirectories {
223- if (!Array.isArray(value) || !value.every((path) => typeof path === "string")) {
224- throw new Error("externalDirectories must be a JSON array of strings");
225+ if (!Array.isArray(value) || !value.every((path) => typeof path === "string" && path.length > 0)) {
226+ throw new Error("externalDirectories must be a JSON array of non-empty strings");
227 }
228 return value;
229 }
230@@ -66,6 +66,20 @@ export function parsePiPolicyConfig(value: unknown): PiPolicyEngineConfig {
231 };
232 }
233
234+function validatePiPolicyFragment(value: unknown): Record<string, unknown> {
235+ const data = objectValue(value);
236+ if (!data) throw new Error("policy engine configuration must be a JSON object");
237+ if (data.permission !== undefined) {
238+ throw new Error("permission has been renamed to tools; external_directory has been renamed to externalDirectories");
239+ }
240+ if (data.reviewer !== undefined && data.reviewer !== null && !objectValue(data.reviewer)) {
241+ throw new Error("reviewer must be a JSON object");
242+ }
243+ if (data.tools !== undefined) toolPermissions(data.tools);
244+ if (data.externalDirectories !== undefined) externalDirectories(data.externalDirectories);
245+ return data;
246+}
247+
248 function mergeObjects(
249 base: Record<string, unknown> | undefined,
250 override: Record<string, unknown> | undefined,
251@@ -99,11 +113,17 @@ function mergeConfig(base: Record<string, unknown>, override: Record<string, unk
252
253 export function loadPiPolicyConfig(cwd = process.cwd()): PiPolicyEngineConfig {
254 let config: Record<string, unknown> = {};
255- for (const path of configPaths(cwd)) {
256+ for (const [index, path] of configPaths(cwd).entries()) {
257 if (!existsSync(path)) continue;
258 const source = JSON.parse(readFileSync(path, "utf8"));
259- parsePiPolicyConfig(source);
260- config = mergeConfig(config, source as Record<string, unknown>);
261+ let data: Record<string, unknown>;
262+ if (index === 0) {
263+ parsePiPolicyConfig(source);
264+ data = source as Record<string, unknown>;
265+ } else {
266+ data = validatePiPolicyFragment(source);
267+ }
268+ config = mergeConfig(config, data);
269 }
270 return parsePiPolicyConfig(config);
271 }
272diff --git a/src/pi/static-permissions.ts b/src/pi/static-permissions.ts
273index 6c60b91cc0a31dc83ab39cd641b89c371390dcff..99a94b5fe993b3fb16746f13e490af9e2042c738 100644
274--- a/src/pi/static-permissions.ts
275+++ b/src/pi/static-permissions.ts
276@@ -17,7 +17,17 @@ function expandedPattern(pattern: string): string {
277 }
278
279 export function matchesExternalDirectory(directories: ExternalDirectories | undefined, value: string): boolean {
280- return directories?.some((directory) => value.startsWith(expandedPattern(directory))) ?? false;
281+ return (
282+ directories?.some((directory) => {
283+ const expanded = expandedPattern(directory);
284+ if (!expanded) return false;
285+ const pathFromDirectory = relative(expanded, value);
286+ return (
287+ pathFromDirectory === "" ||
288+ (!pathFromDirectory.startsWith(`..${sep}`) && pathFromDirectory !== ".." && !isAbsolute(pathFromDirectory))
289+ );
290+ }) ?? false
291+ );
292 }
293
294 async function canonicalPath(path: string, cwd: string): Promise<string> {
295@@ -61,6 +71,7 @@ export async function checkStaticPermission(
296 config: PiPolicyEngineConfig,
297 ): Promise<Decision | undefined> {
298 const action = config.tools[toolType] ?? "check";
299+ if (action === "deny") return staticDecision(action, `Static ${toolType} permission`, "config_allow");
300
301 let externalPath: string | undefined;
302 if (PATH_TOOLS.has(toolType)) {
303diff --git a/test/core/deterministic.test.ts b/test/core/deterministic.test.ts
304index 942730e0d09d1330697c5568eeebb3594af07669..1a9179c31fd983f4506641a47c7e3bc339698a59 100644
305--- a/test/core/deterministic.test.ts
306+++ b/test/core/deterministic.test.ts
307@@ -24,6 +24,7 @@ test("allows recognized deterministic command grammars", () => {
308 'grep -rn "namespace\\|Namespace" ~/go/pkg/mod/github.com/jessevdk/go-flags@v1.6.1/option.go',
309 "echo 'a|b'",
310 "git add src/core/rules.ts",
311+ "git checkout main",
312 "git commit -m 'Allow deterministic Git changes'",
313 "git commit -m 'sudo rm -rf /'",
314 "cp README.md README.copy",
315@@ -89,6 +90,45 @@ test("checks parsed Bash redirects", () => {
316 }
317 });
318
319+test("does not allow unresolved Bash word syntax", () => {
320+ for (const command of ['cat .e"nv"', 'cat "$TARGET"', 'rm -rf "$(echo -n /)"', 'echo x > /e"tc"/x']) {
321+ expect(checkDeterministic("bash", { command })?.decision).not.toBe("allow");
322+ }
323+
324+ expect(checkDeterministic("bash", { command: "echo $HOME" })).toMatchObject({ decision: "allow" });
325+});
326+
327+test("sends inline interpreter code to review while allowing ordinary toolchain commands", () => {
328+ for (const command of [
329+ 'python -c "print(1)"',
330+ 'python3 -c "print(1)"',
331+ 'node -e "console.log(1)"',
332+ 'node --eval "console.log(1)"',
333+ 'php -r "echo 1;"',
334+ 'ruby -e "puts 1"',
335+ 'perl -e "print 1"',
336+ 'deno eval "console.log(1)"',
337+ 'bun -e "console.log(1)"',
338+ 'bun --eval "console.log(1)"',
339+ 'elixir -e "IO.puts(1)"',
340+ 'scala -e "println(1)"',
341+ 'swift -e "print(1)"',
342+ 'mix run -e "IO.puts(1)"',
343+ ]) {
344+ expect(checkDeterministic("bash", { command })?.decision).not.toBe("allow");
345+ }
346+
347+ for (const command of [
348+ "python script.py",
349+ "node script.js",
350+ "php script.php",
351+ "ruby script.rb",
352+ "deno run script.ts",
353+ ]) {
354+ expect(checkDeterministic("bash", { command })).toMatchObject({ decision: "allow" });
355+ }
356+});
357+
358 test("allows local Docker operations except volume deletion", () => {
359 for (const command of [
360 "docker compose up -d",
361@@ -104,8 +144,11 @@ test("allows local Docker operations except volume deletion", () => {
362 "docker volume rm app-data",
363 "docker volume prune -f",
364 "docker rm --volumes app",
365+ "docker rm -fv app",
366 "docker compose down -v",
367+ "docker compose down --volumes=true",
368 "docker system prune --volumes",
369+ "docker system prune --volumes=true",
370 ]) {
371 expect(checkDeterministic("bash", { command })).toMatchObject({ decision: "ask" });
372 }
373@@ -139,6 +182,13 @@ test("asks before allowing reads from recognized secret paths", () => {
374 }
375 });
376
377+test("normalizes static absolute paths before checking home secrets", () => {
378+ const home = process.env.HOME ?? "/home/user";
379+ for (const secretPath of [`${home}/.docker/config.json`, `${home}/.netrc`, `${home}/.aws/credentials`]) {
380+ expect(checkDeterministic("bash", { command: `cat /tmp/..${secretPath}` })).toMatchObject({ decision: "ask" });
381+ }
382+});
383+
384 test("does not treat search terms as secret paths", () => {
385 expect(checkDeterministic("bash", { command: "rg '.env' README.md" })).toMatchObject({ decision: "allow" });
386 });
387@@ -165,7 +215,10 @@ test("asks before destructive operations", () => {
388 for (const command of [
389 "git reset --hard",
390 "git checkout -- src/core/rules.ts",
391+ "git checkout HEAD -- src/core/rules.ts",
392 "git clean -fd",
393+ "touch /tmp/../etc/policy-engine",
394+ "touch ~/../../etc/policy-engine",
395 "rm -rf /",
396 "rm -rf ~",
397 "rm -rf $HOME",
398@@ -176,6 +229,10 @@ test("asks before destructive operations", () => {
399 }
400 });
401
402+test("does not normalize dynamic paths into system paths", () => {
403+ expect(checkDeterministic("bash", { command: "touch /tmp/$TARGET/../etc/policy-engine" })?.decision).not.toBe("ask");
404+});
405+
406 test("does not deterministically allow non-toolchain mutation or shell-control variants", () => {
407diff --git a/test/pi/bash-split.test.ts b/test/pi/bash-split.test.ts
408new file mode 100644
409index 0000000000000000000000000000000000000000..03743a417cfc34b46313265a23a5959b9f064853
410--- /dev/null
411+++ b/test/pi/bash-split.test.ts
412@@ -0,0 +1,47 @@
413+import { expect, test } from "bun:test";
414+import { checkParsedBash } from "../../src/core/deterministic";
415+import { splitBashCommand } from "../../src/pi/bash-split";
416+
417+test("falls back to the raw command when valid Bash has no command nodes", async () => {
418+ await expect(splitBashCommand("> /etc/policy-engine")).resolves.toEqual({
419+ commands: [{ source: "> /etc/policy-engine", redirects: [] }],
420+ parsed: false,
421+ });
422+});
423+
424+test("keeps duplicate command nodes and their enclosing redirects", async () => {
425+ const result = await splitBashCommand("(echo x) > /tmp/out; (echo x) > /etc/policy-engine");
426+
427+ expect(result.parsed).toBe(true);
428+ expect(result.commands).toHaveLength(2);
429+ expect(result.commands.map((command) => command.source)).toEqual(["echo x", "echo x"]);
430+ expect(result.commands.map((command) => command.redirects.map((redirect) => redirect.path))).toEqual([
431+ ["/tmp/out"],
432+ ["/etc/policy-engine"],
433+ ]);
434+ expect(checkParsedBash(result.commands[0]!.source, result.commands[0]!.redirects)).toMatchObject({
435+ decision: "allow",
436+ });
437+ expect(checkParsedBash(result.commands[1]!.source, result.commands[1]!.redirects)).toMatchObject({ decision: "ask" });
438+});
439+
440+test("falls back for unresolved words and redirect paths in parsed Bash", async () => {
441+ for (const command of ['cat .e"nv"', 'rm -rf "$(echo -n /)"']) {
442+ const result = await splitBashCommand(command);
443+ expect(result.parsed).toBe(true);
444+ expect(checkParsedBash(result.commands[0]!.source, result.commands[0]!.redirects)?.decision).not.toBe("allow");
445+ }
446+
447+ const redirectResult = await splitBashCommand('echo x > /e"tc"/x');
448+ expect(redirectResult.parsed).toBe(true);
449+ expect(redirectResult.commands[0]!.redirects[0]).toMatchObject({ dynamic: true });
450+ expect(checkParsedBash(redirectResult.commands[0]!.source, redirectResult.commands[0]!.redirects)?.decision).not.toBe(
451+ "allow",
452+ );
453+
454+ const homeRedirect = await splitBashCommand('echo x > "$HOME/output.txt"');
455+ expect(homeRedirect.commands[0]!.redirects[0]).toMatchObject({ path: "$HOME/output.txt", dynamic: false });
456+ expect(checkParsedBash(homeRedirect.commands[0]!.source, homeRedirect.commands[0]!.redirects)).toMatchObject({
457+ decision: "allow",
458+ });
459+});
460diff --git a/test/pi/static-permissions.test.ts b/test/pi/static-permissions.test.ts
461new file mode 100644
462index 0000000000000000000000000000000000000000..3b8b9373d9fb46e295bc74a9313dc227f8fde745
463--- /dev/null
464+++ b/test/pi/static-permissions.test.ts
465@@ -0,0 +1,40 @@
466+import { expect, test } from "bun:test";
467+import { mkdtempSync, rmSync } from "node:fs";
468+import { tmpdir } from "node:os";
469+import { join } from "node:path";
470+import { checkStaticPermission, matchesExternalDirectory } from "../../src/pi/static-permissions";
471+
472+test("explicit deny wins before external path handling", async () => {
473+ const cwd = mkdtempSync(join(tmpdir(), "policy-engine-cwd-"));
474+ try {
475+ const decision = await checkStaticPermission(
476+ "write",
477+ { path: join(tmpdir(), "policy-engine-external-file") },
478+ cwd,
479+ { reviewer: { kind: "none" }, tools: { write: "deny" }, externalDirectories: [] },
480+ );
481+ expect(decision).toMatchObject({ decision: "deny" });
482+ } finally {
483+ rmSync(cwd, { recursive: true, force: true });
484+ }
485+});
486+
487+test("matches external directories by path components", async () => {
488+ const root = mkdtempSync(join(tmpdir(), "policy-engine-external-"));
489+ const cwd = join(root, "cwd");
490+ const external = join(root, "shared");
491+ const nested = join(external, "nested", "file.txt");
492+ const sibling = join(root, "shared-sibling", "file.txt");
493+ try {
494+ const config = { reviewer: { kind: "none" } as const, tools: {}, externalDirectories: [external] };
495+ await expect(checkStaticPermission("read", { path: nested }, cwd, config)).resolves.toMatchObject({
496+ decision: "allow",
497+ });
498+ await expect(checkStaticPermission("read", { path: sibling }, cwd, config)).resolves.toBeUndefined();
499+ expect(matchesExternalDirectory([external], `${external}-sibling`)).toBe(false);
500+ expect(matchesExternalDirectory([external], nested)).toBe(true);
501+ expect(matchesExternalDirectory([""], nested)).toBe(false);
502+ } finally {
503+ rmSync(root, { recursive: true, force: true });
504+ }
505+});