From 85a193910c1f004b536c46081db9173a8a192551 Mon Sep 17 00:00:00 2001 From: preved911 Date: Wed, 29 Jul 2026 01:54:58 +0000 Subject: [PATCH] feat: check redirect targets against edit rules and external_directory Redirects (2>&1, >/dev/null, > file, etc.) were invisible to the plugin because unbash stores them in cmd.redirects, not cmd.suffix. This meant the reconstructed command text silently dropped redirects, bypassing all security checks. - chain.ts: include redirects in ChainSegment with wellKnown flag - config.ts: add permission.edit rule parsing - enforce.ts: check non-well-known redirects against edit rules; if target is outside cwd, also check external_directory --- src/__tests__/chain.test.ts | 48 +++++++++++++ src/__tests__/enforce.test.ts | 126 ++++++++++++++++++++++++++++++++-- src/chain.ts | 48 +++++++++++-- src/config.ts | 16 ++++- src/enforce.ts | 58 +++++++++++++--- 5 files changed, 272 insertions(+), 24 deletions(-) diff --git a/src/__tests__/chain.test.ts b/src/__tests__/chain.test.ts index dc2b942..c95cf51 100644 --- a/src/__tests__/chain.test.ts +++ b/src/__tests__/chain.test.ts @@ -97,4 +97,52 @@ describe("parseChain", () => { expect(result.segments).toHaveLength(0); expect(result.parseError).toBe(false); }); + + it("captures fd redirect as well-known", () => { + const result = parseChain("ls -la 2>&1"); + expect(result.segments).toHaveLength(1); + expect(result.segments[0].redirects).toHaveLength(1); + expect(result.segments[0].redirects[0].target).toBe("1"); + expect(result.segments[0].redirects[0].wellKnown).toBe(true); + expect(result.segments[0].command).toContain("2>&1"); + }); + + it("captures /dev/null redirect as well-known", () => { + const result = parseChain("ls -la > /dev/null"); + expect(result.segments).toHaveLength(1); + expect(result.segments[0].redirects).toHaveLength(1); + expect(result.segments[0].redirects[0].target).toBe("/dev/null"); + expect(result.segments[0].redirects[0].wellKnown).toBe(true); + expect(result.segments[0].command).toContain(">/dev/null"); + }); + + it("captures file redirect as not well-known", () => { + const result = parseChain("ls -la > /tmp/out.txt"); + expect(result.segments).toHaveLength(1); + expect(result.segments[0].redirects).toHaveLength(1); + expect(result.segments[0].redirects[0].target).toBe("/tmp/out.txt"); + expect(result.segments[0].redirects[0].wellKnown).toBe(false); + expect(result.segments[0].command).toContain(">/tmp/out.txt"); + }); + + it("captures heredoc as well-known", () => { + const result = parseChain("cat << EOF"); + expect(result.segments).toHaveLength(1); + expect(result.segments[0].redirects).toHaveLength(1); + expect(result.segments[0].redirects[0].wellKnown).toBe(true); + }); + + it("captures redirect in chain", () => { + const result = parseChain("echo hello > file.txt && cat file.txt"); + expect(result.segments).toHaveLength(2); + expect(result.segments[0].redirects).toHaveLength(1); + expect(result.segments[0].redirects[0].target).toBe("file.txt"); + expect(result.segments[0].redirects[0].wellKnown).toBe(false); + expect(result.segments[1].redirects).toHaveLength(0); + }); + + it("includes redirect in command text", () => { + const result = parseChain("echo test 2>/dev/null"); + expect(result.segments[0].command).toBe("echo test 2>/dev/null"); + }); }); diff --git a/src/__tests__/enforce.test.ts b/src/__tests__/enforce.test.ts index d6774f7..61fd881 100644 --- a/src/__tests__/enforce.test.ts +++ b/src/__tests__/enforce.test.ts @@ -1,6 +1,7 @@ import { describe, it, expect, beforeEach } from "vitest"; import { resolveSegment, resolveChain, beforeExecute, handlePermissionAsk, clearStoredDecision, isComplexChain, buildReadabilityMessage } from "../enforce.js"; import type { PluginConfig } from "../config.js"; +import type { ChainSegment } from "../chain.js"; const defaultConfig: PluginConfig = { bashRules: [ @@ -8,6 +9,7 @@ const defaultConfig: PluginConfig = { { pattern: "git *", action: "allow" }, { pattern: "sudo *", action: "deny" }, ], + editRules: [], externalDirectoryRules: [ { pattern: "./**", action: "allow" }, ], @@ -26,6 +28,7 @@ describe("resolveSegment", () => { const configNoMatch: PluginConfig = { ...defaultConfig, bashRules: [{ pattern: "*", action: "ask" }], + editRules: [], }; const act = resolveSegment("cat /etc/passwd", "cat", "/project", configNoMatch); expect(act).not.toBeNull(); @@ -34,6 +37,7 @@ describe("resolveSegment", () => { it("most restrictive wins across checks", () => { const config: PluginConfig = { bashRules: [{ pattern: "*", action: "ask" }], + editRules: [], externalDirectoryRules: [{ pattern: "*", action: "deny" }], externalDirectoryDefault: null, enabled: true, @@ -45,6 +49,7 @@ describe("resolveSegment", () => { it("no check triggers returns null", () => { const config: PluginConfig = { bashRules: [], + editRules: [], externalDirectoryRules: [], externalDirectoryDefault: null, enabled: true, @@ -58,8 +63,8 @@ describe("resolveChain", () => { it("all segments allowed — chain let through", () => { const chain = resolveChain( [ - { command: "git status", commandName: "git" }, - { command: "git log", commandName: "git" }, + { command: "git status", commandName: "git", redirects: [] }, + { command: "git log", commandName: "git", redirects: [] }, ], "/project", defaultConfig, @@ -70,14 +75,15 @@ describe("resolveChain", () => { it("any segment not allowed — chain takes its action", () => { const config: PluginConfig = { bashRules: [{ pattern: "git *", action: "allow" }], + editRules: [], externalDirectoryRules: [], externalDirectoryDefault: null, enabled: true, }; const chain = resolveChain( [ - { command: "git status", commandName: "git" }, - { command: "rm -rf /", commandName: "rm" }, + { command: "git status", commandName: "git", redirects: [] }, + { command: "rm -rf /", commandName: "rm", redirects: [] }, ], "/project", config, @@ -88,8 +94,8 @@ describe("resolveChain", () => { it("deny in any segment denies whole chain", () => { const chain = resolveChain( [ - { command: "git status", commandName: "git" }, - { command: "sudo rm -rf /", commandName: "sudo" }, + { command: "git status", commandName: "git", redirects: [] }, + { command: "sudo rm -rf /", commandName: "sudo", redirects: [] }, ], "/project", defaultConfig, @@ -99,7 +105,7 @@ describe("resolveChain", () => { it("single segment with no issues", () => { const chain = resolveChain( - [{ command: "git status", commandName: "git" }], + [{ command: "git status", commandName: "git", redirects: [] }], "/project", defaultConfig, ); @@ -156,6 +162,7 @@ describe("handlePermissionAsk", () => { it("does nothing for stored ask decisions", () => { const config: PluginConfig = { bashRules: [{ pattern: "*", action: "ask" }], + editRules: [], externalDirectoryRules: [], externalDirectoryDefault: null, enabled: true, @@ -208,6 +215,7 @@ describe("readabilityRejection", () => { bashRules: [ { pattern: "*", action: "ask" }, ], + editRules: [], externalDirectoryRules: [{ pattern: "./**", action: "allow" }], externalDirectoryDefault: null, enabled: true, @@ -219,6 +227,7 @@ describe("readabilityRejection", () => { { pattern: "git *", action: "allow" }, { pattern: "npm *", action: "allow" }, ], + editRules: [], externalDirectoryRules: [{ pattern: "./**", action: "allow" }], externalDirectoryDefault: null, enabled: true, @@ -257,6 +266,7 @@ describe("readabilityRejection", () => { it("does not reject complex chain when all segments are denied anyway", () => { const denyConfig: PluginConfig = { bashRules: [{ pattern: "*", action: "deny" }], + editRules: [], externalDirectoryRules: [], externalDirectoryDefault: null, enabled: true, @@ -289,3 +299,105 @@ describe("buildReadabilityMessage", () => { expect(msg).toContain("exit 1"); }); }); + +describe("redirect enforcement", () => { + const cwd = "/project"; + + it("well-known fd redirect does not trigger edit check", () => { + const config: PluginConfig = { + bashRules: [{ pattern: "*", action: "allow" }], + editRules: [{ pattern: "*", action: "deny" }], + externalDirectoryRules: [], + externalDirectoryDefault: null, + enabled: true, + }; + const action = resolveSegment("ls", "ls", cwd, config, [ + { operator: ">&", target: "1", fileDescriptor: 2, wellKnown: true }, + ]); + expect(action).toBe("allow"); + }); + + it("/dev/null redirect does not trigger edit check", () => { + const config: PluginConfig = { + bashRules: [{ pattern: "*", action: "allow" }], + editRules: [{ pattern: "*", action: "deny" }], + externalDirectoryRules: [], + externalDirectoryDefault: null, + enabled: true, + }; + const action = resolveSegment("ls", "ls", cwd, config, [ + { operator: ">", target: "/dev/null", fileDescriptor: undefined, wellKnown: true }, + ]); + expect(action).toBe("allow"); + }); + + it("file redirect inside cwd checks only edit rules", () => { + const config: PluginConfig = { + bashRules: [{ pattern: "*", action: "allow" }], + editRules: [{ pattern: "/project/**", action: "allow" }], + externalDirectoryRules: [{ pattern: "*", action: "deny" }], + externalDirectoryDefault: null, + enabled: true, + }; + const action = resolveSegment("ls", "ls", cwd, config, [ + { operator: ">", target: "output.txt", fileDescriptor: undefined, wellKnown: false }, + ]); + expect(action).toBe("allow"); + }); + + it("file redirect outside cwd checks both edit and external_directory", () => { + const config: PluginConfig = { + bashRules: [{ pattern: "*", action: "allow" }], + editRules: [{ pattern: "/etc/**", action: "deny" }], + externalDirectoryRules: [{ pattern: "./**", action: "allow" }], + externalDirectoryDefault: "ask", + enabled: true, + }; + const action = resolveSegment("ls", "ls", cwd, config, [ + { operator: ">", target: "/etc/passwd", fileDescriptor: undefined, wellKnown: false }, + ]); + expect(action).toBe("deny"); + }); + + it("file redirect outside cwd with denied external_directory", () => { + const config: PluginConfig = { + bashRules: [{ pattern: "*", action: "allow" }], + editRules: [], + externalDirectoryRules: [], + externalDirectoryDefault: "deny", + enabled: true, + }; + const action = resolveSegment("ls", "ls", cwd, config, [ + { operator: ">", target: "/tmp/foo", fileDescriptor: undefined, wellKnown: false }, + ]); + expect(action).toBe("deny"); + }); + + it("redirect with ask edit rule produces ask", () => { + const config: PluginConfig = { + bashRules: [{ pattern: "*", action: "allow" }], + editRules: [{ pattern: "*", action: "ask" }], + externalDirectoryRules: [], + externalDirectoryDefault: null, + enabled: true, + }; + const action = resolveSegment("ls", "ls", cwd, config, [ + { operator: ">", target: "out.txt", fileDescriptor: undefined, wellKnown: false }, + ]); + expect(action).toBe("ask"); + }); + + it("redirect check combined with bash deny still denies", () => { + const config: PluginConfig = { + bashRules: [{ pattern: "*", action: "deny" }], + editRules: [{ pattern: "*", action: "allow" }], + externalDirectoryRules: [], + externalDirectoryDefault: null, + enabled: true, + }; + const action = resolveSegment("sudo rm -rf /", "sudo rm -rf /", cwd, config, [ + { operator: ">", target: "out.txt", fileDescriptor: undefined, wellKnown: false }, + ]); + expect(action).toBe("deny"); + }); +}); diff --git a/src/chain.ts b/src/chain.ts index 802c440..5418b6f 100644 --- a/src/chain.ts +++ b/src/chain.ts @@ -1,9 +1,17 @@ import { parse } from "unbash"; -import type { Script, Statement, Node, CommandExpansionPart, Command, AndOr, Pipeline } from "unbash"; +import type { Script, Statement, Node, CommandExpansionPart, Command, AndOr, Pipeline, Redirect } from "unbash"; + +export interface RedirectInfo { + operator: string; + target: string; + fileDescriptor: number | undefined; + wellKnown: boolean; +} export interface ChainSegment { command: string; commandName: string; + redirects: RedirectInfo[]; } export interface ChainResult { @@ -13,6 +21,23 @@ export interface ChainResult { errors: string[]; } +function isWellKnownRedirect(redir: Redirect): boolean { + const target = redir.target?.text ?? redir.content ?? ""; + if (target === "/dev/null") return true; + if (/^\d+$/.test(target)) return true; + if (redir.operator === "<<" || redir.operator === "<<-" || redir.operator === "<<<") return true; + return false; +} + +function redirectToInfo(redir: Redirect): RedirectInfo { + return { + operator: redir.operator, + target: redir.target?.text ?? redir.content ?? "", + fileDescriptor: redir.fileDescriptor, + wellKnown: isWellKnownRedirect(redir), + }; +} + function getCommandText(cmd: Command): string { const parts: string[] = []; if (cmd.name) { @@ -21,6 +46,12 @@ function getCommandText(cmd: Command): string { for (const word of cmd.suffix) { parts.push(word.text); } + for (const redir of cmd.redirects) { + const prefix = redir.fileDescriptor !== undefined ? String(redir.fileDescriptor) : ""; + const op = redir.operator; + const target = redir.target?.text ?? ""; + parts.push(`${prefix}${op}${target}`); + } return parts.join(" "); } @@ -51,16 +82,23 @@ function extractCommandsFromNode(node: Node): Command[] { return result; } +function buildSegment(cmd: Command, stmtRedirects: Redirect[]): ChainSegment { + const cmdRedirects = (cmd.redirects ?? []).map(redirectToInfo); + const statementRedirects = (stmtRedirects ?? []).map(redirectToInfo); + return { + command: getCommandText(cmd), + commandName: getCommandName(cmd), + redirects: [...cmdRedirects, ...statementRedirects], + }; +} + function extractCommandsFromScript(script: Script): ChainSegment[] { const segments: ChainSegment[] = []; for (const stmt of script.commands) { const cmds = extractCommandsFromNode(stmt.command); for (const cmd of cmds) { if (cmd.type === "Command") { - segments.push({ - command: getCommandText(cmd), - commandName: getCommandName(cmd), - }); + segments.push(buildSegment(cmd, stmt.redirects)); } } } diff --git a/src/config.ts b/src/config.ts index 754da72..bc8c49d 100644 --- a/src/config.ts +++ b/src/config.ts @@ -12,6 +12,7 @@ export interface ExternalDirectoryRule { export interface PluginConfig { bashRules: BashPermissionRule[]; + editRules: BashPermissionRule[]; externalDirectoryRules: ExternalDirectoryRule[]; externalDirectoryDefault: ExternalDirectoryAction | null; enabled: boolean; @@ -25,6 +26,7 @@ export function parseConfig(config: Record): PluginConfig { const permission = config.permission as Record | undefined; let bashRules: BashPermissionRule[] = []; + let editRules: BashPermissionRule[] = []; let externalDirectoryRules: ExternalDirectoryRule[] = []; let externalDirectoryDefault: ExternalDirectoryAction | null = null; let enabled = true; @@ -42,6 +44,18 @@ export function parseConfig(config: Record): PluginConfig { })); } + const edit = permission.edit; + if (typeof edit === "string" && isPermissionAction(edit)) { + editRules = [{ pattern: "*", action: edit }]; + } else if (edit && typeof edit === "object") { + editRules = Object.entries(edit) + .filter((entry): entry is [string, unknown] => true) + .map(([pattern, action]) => ({ + pattern, + action: (isPermissionAction(String(action)) ? String(action) : "ask") as "ask" | "allow" | "deny", + })); + } + const wildAction = bashRules.find((r) => r.pattern === "*")?.action; if (wildAction === "allow") { enabled = false; @@ -63,7 +77,7 @@ export function parseConfig(config: Record): PluginConfig { enabled = false; } - return { bashRules, externalDirectoryRules, externalDirectoryDefault, enabled }; + return { bashRules, editRules, externalDirectoryRules, externalDirectoryDefault, enabled }; } export function matchBashPermission(segment: string, rules: BashPermissionRule[]): "ask" | "allow" | "deny" | null { diff --git a/src/enforce.ts b/src/enforce.ts index 2d767a2..4e84944 100644 --- a/src/enforce.ts +++ b/src/enforce.ts @@ -1,7 +1,9 @@ import type { PluginConfig } from "./config.js"; import { matchBashPermission, matchExternalDirectory } from "./config.js"; import { parseChain } from "./chain.js"; +import type { ChainSegment, RedirectInfo } from "./chain.js"; import { extractPaths } from "./paths.js"; +import path from "path"; export type ChainAction = "allow" | "ask" | "deny" | null; @@ -19,7 +21,43 @@ export function clearStoredDecision(callID: string): void { decisionStore.delete(callID); } -export function resolveSegment(segment: string, segmentName: string, cwd: string, config: PluginConfig): ChainAction { +function resolveRedirectTargets(redirects: RedirectInfo[], cwd: string, config: PluginConfig): ChainAction { + const actions: ChainAction[] = []; + + for (const redir of redirects) { + if (redir.wellKnown) continue; + + const resolvedPath = path.resolve(cwd, redir.target); + const underCwd = resolvedPath.startsWith(cwd + path.sep) || resolvedPath === cwd; + + const editAction = matchBashPermission(resolvedPath, config.editRules); + if (editAction) actions.push(editAction); + + if (!underCwd) { + const edResult = matchExternalDirectory(resolvedPath, config.externalDirectoryRules, config.externalDirectoryDefault, cwd); + if (edResult.violated && edResult.action) { + actions.push(edResult.action); + } + } + } + + if (actions.length === 0) return null; + if (actions.includes("deny")) return "deny"; + if (actions.includes("ask")) return "ask"; + if (actions.includes("allow")) return "allow"; + return null; +} + +function combineActions(a: ChainAction, b: ChainAction): ChainAction { + const actions = [a, b].filter((x): x is NonNullable => x !== null); + if (actions.length === 0) return null; + if (actions.includes("deny")) return "deny"; + if (actions.includes("ask")) return "ask"; + if (actions.includes("allow")) return "allow"; + return null; +} + +export function resolveSegment(segment: string, segmentName: string, cwd: string, config: PluginConfig, redirects?: RedirectInfo[]): ChainAction { const bashAction = matchBashPermission(segment, config.bashRules); const paths = extractPaths(segment, cwd); @@ -34,23 +72,21 @@ export function resolveSegment(segment: string, segmentName: string, cwd: string } } - const actions: ChainAction[] = []; - if (bashAction) actions.push(bashAction); - if (edAction) actions.push(edAction); + let combined = combineActions(bashAction, edAction); - if (actions.length === 0) return null; + if (redirects && redirects.length > 0) { + const redirectAction = resolveRedirectTargets(redirects, cwd, config); + combined = combineActions(combined, redirectAction); + } - if (actions.includes("deny")) return "deny"; - if (actions.includes("ask")) return "ask"; - if (actions.includes("allow")) return "allow" as ChainAction; - return null; + return combined; } -export function resolveChain(segments: Array<{ command: string; commandName: string }>, cwd: string, config: PluginConfig): ChainAction { +export function resolveChain(segments: ChainSegment[], cwd: string, config: PluginConfig): ChainAction { const segmentActions: ChainAction[] = []; for (const seg of segments) { - const action = resolveSegment(seg.command, seg.commandName, cwd, config); + const action = resolveSegment(seg.command, seg.commandName, cwd, config, seg.redirects); segmentActions.push(action); }