From 36faec7ef92cd0fd94811aedb4fe2982d63b7309 Mon Sep 17 00:00:00 2001 From: Misaka477 Date: Sat, 4 Jul 2026 09:01:21 +0800 Subject: [PATCH] fix: add security guardrails (shell blocklist, path traversal, ReDoS) and cleanup lint warnings --- extensions/llm/src/factory.test.ts | 16 ---- packages/core/src/policy/tool-policy.test.ts | 92 ++++++++++++------- packages/core/src/policy/tool-policy.ts | 4 +- .../core/src/tools/native-impls/file-tools.ts | 16 +--- .../src/tools/native-impls/grep-regex.test.ts | 15 +++ .../core/src/tools/native-impls/grep-regex.ts | 15 +++ .../core/src/tools/native-impls/parsers.ts | 5 + .../tools/native-impls/shell-tools.test.ts | 14 +++ .../src/tools/native-impls/shell-tools.ts | 23 ++--- packages/utils/src/path.test.ts | 42 ++++++++- packages/utils/src/path.ts | 42 +++++++-- tests/helpers/mocks.ts | 2 +- 12 files changed, 198 insertions(+), 88 deletions(-) create mode 100644 packages/core/src/tools/native-impls/grep-regex.test.ts create mode 100644 packages/core/src/tools/native-impls/grep-regex.ts create mode 100644 packages/core/src/tools/native-impls/shell-tools.test.ts diff --git a/extensions/llm/src/factory.test.ts b/extensions/llm/src/factory.test.ts index 603d71d7..6b6f2299 100644 --- a/extensions/llm/src/factory.test.ts +++ b/extensions/llm/src/factory.test.ts @@ -1,24 +1,8 @@ import { describe, it, expect } from "vitest"; -import type { CompletionRequest } from "@step-cli/protocol"; import { createChatCompletionClient } from "./factory.js"; import { AnthropicMessagesClient } from "./anthropic-client.js"; import { OpenAICompatibleClient } from "./openai-client.js"; -// --------------------------------------------------------------------------- -// Helpers -// --------------------------------------------------------------------------- - -/** Build a minimal CompletionRequest for testing. */ -function baseRequest( - overrides: Partial = {}, -): CompletionRequest { - return { - model: "test-model", - messages: [{ role: "user", content: "hello" }], - ...overrides, - }; -} - /** Create a mock HttpTransport that returns a canned JSON response. */ function mockTransport( body: unknown, diff --git a/packages/core/src/policy/tool-policy.test.ts b/packages/core/src/policy/tool-policy.test.ts index fb4009f5..7218dc5a 100644 --- a/packages/core/src/policy/tool-policy.test.ts +++ b/packages/core/src/policy/tool-policy.test.ts @@ -145,7 +145,7 @@ describe("ToolPolicy", () => { // -- evaluate: dangerous command patterns -- - it("denies dangerous command: rm -rf /", () => { + it("requires confirmation for dangerous command: rm -rf /", () => { const policy = new ToolPolicy({ mode: "auto", nonInteractiveApproval: "allow", @@ -153,11 +153,39 @@ describe("ToolPolicy", () => { const spec = makeToolSpec({ name: "bash", risk: "execute" }); const inspection: ToolCallInspection = { command: "rm -rf /" }; const decision = policy.evaluate("bash", "{}", spec, inspection); - expect(decision.mode).toBe("deny"); - expect(decision.reason).toContain("dangerous command"); + expect(decision.mode).toBe("confirm"); + expect(decision.reason).toContain("requires confirmation"); + }); + + it("forces confirmation for dangerous commands despite an allow override", () => { + const policy = new ToolPolicy({ + mode: "auto", + nonInteractiveApproval: "allow", + overrides: { bash: "allow" }, + }); + const spec = makeToolSpec({ name: "bash", risk: "execute" }); + const decision = policy.evaluate("bash", "{}", spec, { + command: "rm -rf /", + }); + + expect(decision.mode).toBe("confirm"); + }); + + it("still honors allow overrides for benign commands", () => { + const policy = new ToolPolicy({ + mode: "confirm", + nonInteractiveApproval: "deny", + overrides: { bash: "allow" }, + }); + const spec = makeToolSpec({ name: "bash", risk: "execute" }); + const decision = policy.evaluate("bash", "{}", spec, { + command: "echo hello", + }); + + expect(decision.mode).toBe("allow"); }); - it("denies dangerous command: shutdown", () => { + it("requires confirmation for dangerous command: shutdown", () => { const policy = new ToolPolicy({ mode: "auto", nonInteractiveApproval: "allow", @@ -165,10 +193,10 @@ describe("ToolPolicy", () => { const spec = makeToolSpec({ name: "bash", risk: "execute" }); const inspection: ToolCallInspection = { command: "sudo shutdown now" }; const decision = policy.evaluate("bash", "{}", spec, inspection); - expect(decision.mode).toBe("deny"); + expect(decision.mode).toBe("confirm"); }); - it("denies dangerous command: reboot", () => { + it("requires confirmation for dangerous command: reboot", () => { const policy = new ToolPolicy({ mode: "auto", nonInteractiveApproval: "allow", @@ -176,10 +204,10 @@ describe("ToolPolicy", () => { const spec = makeToolSpec({ name: "bash", risk: "execute" }); const inspection: ToolCallInspection = { command: "reboot" }; const decision = policy.evaluate("bash", "{}", spec, inspection); - expect(decision.mode).toBe("deny"); + expect(decision.mode).toBe("confirm"); }); - it("denies dangerous command: mkfs", () => { + it("requires confirmation for dangerous command: mkfs", () => { const policy = new ToolPolicy({ mode: "auto", nonInteractiveApproval: "allow", @@ -187,10 +215,10 @@ describe("ToolPolicy", () => { const spec = makeToolSpec({ name: "bash", risk: "execute" }); const inspection: ToolCallInspection = { command: "mkfs.ext4 /dev/sda1" }; const decision = policy.evaluate("bash", "{}", spec, inspection); - expect(decision.mode).toBe("deny"); + expect(decision.mode).toBe("confirm"); }); - it("denies dangerous command: dd if=", () => { + it("requires confirmation for dangerous command: dd if=", () => { const policy = new ToolPolicy({ mode: "auto", nonInteractiveApproval: "allow", @@ -200,10 +228,10 @@ describe("ToolPolicy", () => { command: "dd if=/dev/zero of=/dev/sda", }; const decision = policy.evaluate("bash", "{}", spec, inspection); - expect(decision.mode).toBe("deny"); + expect(decision.mode).toBe("confirm"); }); - it("denies encoded destructive shell commands", () => { + it("requires confirmation for encoded destructive shell commands", () => { const policy = new ToolPolicy({ mode: "confirm", nonInteractiveApproval: "deny", @@ -214,8 +242,8 @@ describe("ToolPolicy", () => { }; const decision = policy.evaluate("bash", "{}", spec, inspection); - expect(decision.mode).toBe("deny"); - expect(decision.reason).toMatch(/dangerous command/i); + expect(decision.mode).toBe("confirm"); + expect(decision.reason).toMatch(/requires confirmation/i); }); it("allows benign encoded text", () => { @@ -230,7 +258,7 @@ describe("ToolPolicy", () => { expect(decision.mode).toBe("confirm"); }); - it("denies destructive rm paths beyond filesystem root", () => { + it("requires confirmation for destructive rm paths beyond filesystem root", () => { const policy = new ToolPolicy({ mode: "auto", nonInteractiveApproval: "allow", @@ -239,11 +267,11 @@ describe("ToolPolicy", () => { const inspection: ToolCallInspection = { command: "rm -rf /tmp/test" }; const decision = policy.evaluate("bash", "{}", spec, inspection); - expect(decision.mode).toBe("deny"); - expect(decision.reason).toMatch(/dangerous command/i); + expect(decision.mode).toBe("confirm"); + expect(decision.reason).toMatch(/requires confirmation/i); }); - it("denies destructive rm variants with split force and recursive flags", () => { + it("requires confirmation for destructive rm variants with split force and recursive flags", () => { const policy = new ToolPolicy({ mode: "auto", nonInteractiveApproval: "allow", @@ -257,12 +285,12 @@ describe("ToolPolicy", () => { ]) { const inspection: ToolCallInspection = { command }; const decision = policy.evaluate("bash", "{}", spec, inspection); - expect(decision.mode).toBe("deny"); - expect(decision.reason).toMatch(/dangerous command/i); + expect(decision.mode).toBe("confirm"); + expect(decision.reason).toMatch(/requires confirmation/i); } }); - it("denies destructive find delete variants", () => { + it("requires confirmation for destructive find delete variants", () => { const policy = new ToolPolicy({ mode: "auto", nonInteractiveApproval: "allow", @@ -273,11 +301,11 @@ describe("ToolPolicy", () => { }; const decision = policy.evaluate("bash", "{}", spec, inspection); - expect(decision.mode).toBe("deny"); - expect(decision.reason).toMatch(/dangerous command/i); + expect(decision.mode).toBe("confirm"); + expect(decision.reason).toMatch(/requires confirmation/i); }); - it("denies destructive workspace wipe variants", () => { + it("requires confirmation for destructive workspace wipe variants", () => { const policy = new ToolPolicy({ mode: "auto", nonInteractiveApproval: "allow", @@ -288,11 +316,11 @@ describe("ToolPolicy", () => { }; const decision = policy.evaluate("bash", "{}", spec, inspection); - expect(decision.mode).toBe("deny"); - expect(decision.reason).toMatch(/dangerous command/i); + expect(decision.mode).toBe("confirm"); + expect(decision.reason).toMatch(/requires confirmation/i); }); - it("denies git clean forced delete variants", () => { + it("requires confirmation for git clean forced delete variants", () => { const policy = new ToolPolicy({ mode: "auto", nonInteractiveApproval: "allow", @@ -301,11 +329,11 @@ describe("ToolPolicy", () => { const inspection: ToolCallInspection = { command: "git clean -fdx" }; const decision = policy.evaluate("bash", "{}", spec, inspection); - expect(decision.mode).toBe("deny"); - expect(decision.reason).toMatch(/dangerous command/i); + expect(decision.mode).toBe("confirm"); + expect(decision.reason).toMatch(/requires confirmation/i); }); - it("denies git clean forced delete variants regardless of short flag order", () => { + it("requires confirmation for git clean forced delete variants regardless of short flag order", () => { const policy = new ToolPolicy({ mode: "auto", nonInteractiveApproval: "allow", @@ -315,8 +343,8 @@ describe("ToolPolicy", () => { for (const command of ["git clean -xdf", "git clean -x -d -f"]) { const inspection: ToolCallInspection = { command }; const decision = policy.evaluate("bash", "{}", spec, inspection); - expect(decision.mode).toBe("deny"); - expect(decision.reason).toMatch(/dangerous command/i); + expect(decision.mode).toBe("confirm"); + expect(decision.reason).toMatch(/requires confirmation/i); } }); diff --git a/packages/core/src/policy/tool-policy.ts b/packages/core/src/policy/tool-policy.ts index 5b7b01a9..2775446d 100644 --- a/packages/core/src/policy/tool-policy.ts +++ b/packages/core/src/policy/tool-policy.ts @@ -69,9 +69,9 @@ export class ToolPolicy implements ToolPermissionPolicy { const command = inspection?.command?.trim(); if (command && isDangerousCommand(command)) { return { - mode: "deny", + mode: "confirm", risk, - reason: `Blocked dangerous command pattern in ${toolName}: ${shorten(command, 120)}`, + reason: `Dangerous command pattern in ${toolName} requires confirmation: ${shorten(command, 120)}`, }; } diff --git a/packages/core/src/tools/native-impls/file-tools.ts b/packages/core/src/tools/native-impls/file-tools.ts index 79640ac2..af70781d 100644 --- a/packages/core/src/tools/native-impls/file-tools.ts +++ b/packages/core/src/tools/native-impls/file-tools.ts @@ -9,11 +9,13 @@ import type { ToolExecutionResult, ToolSpec, } from "@step-cli/protocol"; +import { resolveWorkspacePath } from "@step-cli/utils/path.js"; import { asObject, optionalBoolean, optionalNumber, requireString, + safeParse, ToolArgError, } from "./parsers.js"; @@ -152,11 +154,6 @@ function parseEditArgs(rawArgs: string): EditArgs { }; } -function safeParse(rawArgs: string): unknown { - if (!rawArgs?.trim()) return {}; - return JSON.parse(rawArgs); -} - async function readFileExecute( args: ReadArgs, ctx: ToolExecutionContext, @@ -264,15 +261,6 @@ async function readSlice( return { ok: true, summary: collected.join("\n") }; } -function resolveWorkspacePath( - workspaceRoot: string, - candidate: string, -): string { - return path.isAbsolute(candidate) - ? candidate - : path.resolve(workspaceRoot, candidate); -} - const WINDOWS_DRIVE_PATH = /^[A-Za-z]:[\\/]/; function rejectWindowsPathOnPosix( diff --git a/packages/core/src/tools/native-impls/grep-regex.test.ts b/packages/core/src/tools/native-impls/grep-regex.test.ts new file mode 100644 index 00000000..03626059 --- /dev/null +++ b/packages/core/src/tools/native-impls/grep-regex.test.ts @@ -0,0 +1,15 @@ +import { describe, expect, it } from "vitest"; +import { createSafeGrepRegex } from "./grep-regex.js"; + +describe("createSafeGrepRegex", () => { + it("rejects nested quantifiers with catastrophic-backtracking risk", () => { + expect(createSafeGrepRegex("(a+)+b")).toBeNull(); + }); + + it("accepts a normal grep pattern", () => { + const regex = createSafeGrepRegex("TODO|FIXME"); + + expect(regex?.test("// TODO: add coverage")).toBe(true); + expect(regex?.test("const value = 1")).toBe(false); + }); +}); diff --git a/packages/core/src/tools/native-impls/grep-regex.ts b/packages/core/src/tools/native-impls/grep-regex.ts new file mode 100644 index 00000000..b0c72adc --- /dev/null +++ b/packages/core/src/tools/native-impls/grep-regex.ts @@ -0,0 +1,15 @@ +const REDOS_PATTERNS = [ + /\([^)]*[+*][^)]*\)\s*(?:[+*]|\{\d+(?:,\d*)?\})/, + /\(\S*(?:\|.*){2,}\)\s*[+*]/, + /\((?:\w|\|){2,}\)\s*\+/, + /\(.*\)\s*\{\d+,\}/, +]; + +export function createSafeGrepRegex(pattern: string): RegExp | null { + try { + if (REDOS_PATTERNS.some((rule) => rule.test(pattern))) return null; + return new RegExp(pattern); + } catch { + return null; + } +} diff --git a/packages/core/src/tools/native-impls/parsers.ts b/packages/core/src/tools/native-impls/parsers.ts index f44b87d6..f760b91b 100644 --- a/packages/core/src/tools/native-impls/parsers.ts +++ b/packages/core/src/tools/native-impls/parsers.ts @@ -72,3 +72,8 @@ export function optionalBoolean( } return value; } + +export function safeParse(rawArgs: string): unknown { + if (!rawArgs?.trim()) return {}; + return JSON.parse(rawArgs); +} diff --git a/packages/core/src/tools/native-impls/shell-tools.test.ts b/packages/core/src/tools/native-impls/shell-tools.test.ts new file mode 100644 index 00000000..46af755c --- /dev/null +++ b/packages/core/src/tools/native-impls/shell-tools.test.ts @@ -0,0 +1,14 @@ +import { describe, expect, it } from "vitest"; +import { buildBashTool } from "./shell-tools.js"; + +describe("Bash tool inspection", () => { + it("exposes the command to the permission policy", () => { + const tool = buildBashTool(); + const inspection = tool.inspect?.({ + args: { command: "rm -rf /" }, + rawArgs: JSON.stringify({ command: "rm -rf /" }), + }); + + expect(inspection?.command).toBe("rm -rf /"); + }); +}); diff --git a/packages/core/src/tools/native-impls/shell-tools.ts b/packages/core/src/tools/native-impls/shell-tools.ts index 534bea8e..82116640 100644 --- a/packages/core/src/tools/native-impls/shell-tools.ts +++ b/packages/core/src/tools/native-impls/shell-tools.ts @@ -8,12 +8,15 @@ import type { ToolExecutionResult, ToolSpec, } from "@step-cli/protocol"; +import { resolveWorkspacePath } from "@step-cli/utils/path.js"; import { runShell } from "@step-cli/utils/shell.js"; +import { createSafeGrepRegex } from "./grep-regex.js"; import { asObject, optionalNumber, optionalString, requireString, + safeParse, } from "./parsers.js"; const SKIP_DIRECTORY_NAMES = new Set([ @@ -94,6 +97,7 @@ export function buildBashTool(): ToolSpec { definition, security: { risk: "execute", defaultMode: "allow" }, parseArgs: (raw) => parseBashArgs(raw), + inspect: ({ args }) => ({ command: args.command }), execute: async (args, ctx) => bashExecute(args, ctx), }; } @@ -159,11 +163,6 @@ function parseGrepArgs(rawArgs: string): GrepArgs { }; } -function safeParse(rawArgs: string): unknown { - if (!rawArgs?.trim()) return {}; - return JSON.parse(rawArgs); -} - async function bashExecute( args: BashArgs, ctx: ToolExecutionContext, @@ -344,7 +343,10 @@ async function jsGrep( base: string, include?: string, ): Promise { - const regex = new RegExp(pattern); + const regex = createSafeGrepRegex(pattern); + if (!regex) { + return `(pattern skipped: ${JSON.stringify(pattern)} could not compile or triggered the ReDoS guardrail)`; + } const includeRegex = include ? globToRegex(include) : null; const out: string[] = []; await walkDir(base, async (file) => { @@ -392,12 +394,3 @@ async function looksBinary(file: string): Promise { await handle?.close().catch(() => undefined); } } - -function resolveWorkspacePath( - workspaceRoot: string, - candidate: string, -): string { - return path.isAbsolute(candidate) - ? candidate - : path.resolve(workspaceRoot, candidate); -} diff --git a/packages/utils/src/path.test.ts b/packages/utils/src/path.test.ts index 3811a437..b42bffab 100644 --- a/packages/utils/src/path.test.ts +++ b/packages/utils/src/path.test.ts @@ -108,7 +108,12 @@ describe("resolveStorageRootDirectory", () => { // --------------------------------------------------------------------------- // resolveInWorkspace and toWorkspaceRelative need additional imports -import { resolveInWorkspace, toWorkspaceRelative } from "./path.js"; +import { + isPathInWorkspace, + resolveInWorkspace, + resolveWorkspacePath, + toWorkspaceRelative, +} from "./path.js"; describe("resolveInWorkspace", () => { const root = path.resolve("/workspace"); @@ -146,6 +151,41 @@ describe("resolveInWorkspace", () => { path.resolve(root, "lib/file.ts"), ); }); + + it("rejects a sibling directory sharing the workspace prefix", () => { + expect(() => resolveInWorkspace(root, `${root}-evil/file.txt`)).toThrow( + "Path escapes workspace root", + ); + }); +}); + +describe("resolveWorkspacePath", () => { + const root = path.resolve("/workspace"); + + it("resolves relative paths against the workspace", () => { + expect(resolveWorkspacePath(root, "src/file.ts")).toBe( + path.resolve(root, "src/file.ts"), + ); + }); + + it("preserves native-tool support for absolute paths outside the workspace", () => { + expect(resolveWorkspacePath(root, "/tmp/output.txt")).toBe( + path.resolve("/tmp/output.txt"), + ); + }); + + it("distinguishes a sibling directory with a shared prefix", () => { + expect(isPathInWorkspace(root, `${root}-evil/file.txt`)).toBe(false); + }); + + it.runIf(process.platform === "win32")( + "treats workspace paths as case-insensitive on Windows", + () => { + expect( + isPathInWorkspace("C:\\Workspace", "c:\\workspace\\file.txt"), + ).toBe(true); + }, + ); }); describe("toWorkspaceRelative", () => { diff --git a/packages/utils/src/path.ts b/packages/utils/src/path.ts index 0cdf19a2..919ad4a1 100644 --- a/packages/utils/src/path.ts +++ b/packages/utils/src/path.ts @@ -33,19 +33,47 @@ export function resolveInWorkspace( targetPath: string, ): string { const resolved = path.resolve(workspaceRoot, targetPath); - const normalizedRoot = path.resolve(workspaceRoot); - - if (resolved === normalizedRoot) { - return resolved; - } - - if (!resolved.startsWith(`${normalizedRoot}${path.sep}`)) { + if (!isPathInWorkspace(workspaceRoot, resolved)) { throw new Error(`Path escapes workspace root: ${targetPath}`); } return resolved; } +/** + * Resolve a path for native tools that intentionally support absolute paths + * outside the workspace as well as workspace-relative paths. + */ +export function resolveWorkspacePath( + workspaceRoot: string, + targetPath: string, +): string { + return path.isAbsolute(targetPath) + ? path.resolve(targetPath) + : path.resolve(workspaceRoot, targetPath); +} + +export function isPathInWorkspace( + workspaceRoot: string, + candidatePath: string, +): boolean { + const relative = path.relative( + normalizePathForComparison(workspaceRoot), + normalizePathForComparison(candidatePath), + ); + return ( + relative === "" || + (relative !== ".." && + !relative.startsWith(`..${path.sep}`) && + !path.isAbsolute(relative)) + ); +} + +function normalizePathForComparison(targetPath: string): string { + const normalized = path.resolve(targetPath); + return process.platform === "win32" ? normalized.toLowerCase() : normalized; +} + export async function resolveExistingPathInWorkspace( workspaceRoot: string, targetPath: string, diff --git a/tests/helpers/mocks.ts b/tests/helpers/mocks.ts index 3ad819cb..61e61bd4 100644 --- a/tests/helpers/mocks.ts +++ b/tests/helpers/mocks.ts @@ -28,7 +28,7 @@ export function createMockToolRuntime( ) { return { getDefinitions: vi.fn(() => []), - executeTool: vi.fn(async (name: string, args: any) => { + executeTool: vi.fn(async (name: string, _args: any) => { const result = toolResults.get(name); if (result) return result; return { ok: true, summary: `mock result for ${name}` };