refactor(security): extract safe-bin policy and dedupe tests

This commit is contained in:
Peter Steinberger
2026-02-19 14:23:19 +01:00
parent 0e85380e56
commit 2d485cd47a
3 changed files with 365 additions and 406 deletions

View File

@@ -67,40 +67,74 @@ vi.mock("../infra/exec-approvals.js", async (importOriginal) => {
return { ...mod, resolveExecApprovals: () => approvals };
});
type ExecToolResult = {
content: Array<{ type: string; text?: string }>;
details?: { status?: string };
};
type ExecTool = {
execute(
callId: string,
params: {
command: string;
workdir: string;
env?: Record<string, string>;
},
): Promise<ExecToolResult>;
};
async function createSafeBinsExecTool(params: {
tmpPrefix: string;
safeBins: string[];
files?: Array<{ name: string; contents: string }>;
}): Promise<{ tmpDir: string; execTool: ExecTool }> {
const { createOpenClawCodingTools } = await import("./pi-tools.js");
const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), params.tmpPrefix));
for (const file of params.files ?? []) {
fs.writeFileSync(path.join(tmpDir, file.name), file.contents, "utf8");
}
const cfg: OpenClawConfig = {
tools: {
exec: {
host: "gateway",
security: "allowlist",
ask: "off",
safeBins: params.safeBins,
},
},
};
const tools = createOpenClawCodingTools({
config: cfg,
sessionKey: "agent:main:main",
workspaceDir: tmpDir,
agentDir: path.join(tmpDir, "agent"),
});
const execTool = tools.find((tool) => tool.name === "exec");
if (!execTool) {
throw new Error("exec tool missing from coding tools");
}
return { tmpDir, execTool: execTool as ExecTool };
}
describe("createOpenClawCodingTools safeBins", () => {
it("threads tools.exec.safeBins into exec allowlist checks", async () => {
if (process.platform === "win32") {
return;
}
const { createOpenClawCodingTools } = await import("./pi-tools.js");
const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-safe-bins-"));
const cfg: OpenClawConfig = {
tools: {
exec: {
host: "gateway",
security: "allowlist",
ask: "off",
safeBins: ["echo"],
},
},
};
const tools = createOpenClawCodingTools({
config: cfg,
sessionKey: "agent:main:main",
workspaceDir: tmpDir,
agentDir: path.join(tmpDir, "agent"),
const { tmpDir, execTool } = await createSafeBinsExecTool({
tmpPrefix: "openclaw-safe-bins-",
safeBins: ["echo"],
});
const execTool = tools.find((tool) => tool.name === "exec");
expect(execTool).toBeDefined();
const marker = `safe-bins-${Date.now()}`;
const envSnapshot = captureEnv(["OPENCLAW_SHELL_ENV_TIMEOUT_MS"]);
const result = await (async () => {
try {
process.env.OPENCLAW_SHELL_ENV_TIMEOUT_MS = "1000";
return await execTool!.execute("call1", {
return await execTool.execute("call1", {
command: `echo ${marker}`,
workdir: tmpDir,
});
@@ -120,33 +154,14 @@ describe("createOpenClawCodingTools safeBins", () => {
return;
}
const { createOpenClawCodingTools } = await import("./pi-tools.js");
const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-safe-bins-expand-"));
fs.writeFileSync(path.join(tmpDir, "secret.txt"), "TOP_SECRET\n", "utf8");
const cfg: OpenClawConfig = {
tools: {
exec: {
host: "gateway",
security: "allowlist",
ask: "off",
safeBins: ["head", "wc"],
},
},
};
const tools = createOpenClawCodingTools({
config: cfg,
sessionKey: "agent:main:main",
workspaceDir: tmpDir,
agentDir: path.join(tmpDir, "agent"),
const { tmpDir, execTool } = await createSafeBinsExecTool({
tmpPrefix: "openclaw-safe-bins-expand-",
safeBins: ["head", "wc"],
files: [{ name: "secret.txt", contents: "TOP_SECRET\n" }],
});
const execTool = tools.find((tool) => tool.name === "exec");
expect(execTool).toBeDefined();
await expect(
execTool!.execute("call1", {
execTool.execute("call1", {
command: "head $FOO ; wc -l",
workdir: tmpDir,
env: { FOO: "secret.txt" },
@@ -159,33 +174,15 @@ describe("createOpenClawCodingTools safeBins", () => {
return;
}
const { createOpenClawCodingTools } = await import("./pi-tools.js");
const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-safe-bins-oracle-"));
fs.writeFileSync(path.join(tmpDir, "existing.txt"), "x\n", "utf8");
const cfg: OpenClawConfig = {
tools: {
exec: {
host: "gateway",
security: "allowlist",
ask: "off",
safeBins: ["sort"],
},
},
};
const tools = createOpenClawCodingTools({
config: cfg,
sessionKey: "agent:main:main",
workspaceDir: tmpDir,
agentDir: path.join(tmpDir, "agent"),
const { tmpDir, execTool } = await createSafeBinsExecTool({
tmpPrefix: "openclaw-safe-bins-oracle-",
safeBins: ["sort"],
files: [{ name: "existing.txt", contents: "x\n" }],
});
const execTool = tools.find((tool) => tool.name === "exec");
expect(execTool).toBeDefined();
const run = async (command: string) => {
try {
const result = await execTool!.execute("call-oracle", { command, workdir: tmpDir });
const result = await execTool.execute("call-oracle", { command, workdir: tmpDir });
const text = result.content.find((content) => content.type === "text")?.text ?? "";
const resultDetails = result.details as { status?: string };
return { kind: "result" as const, status: resultDetails.status, text };
@@ -204,46 +201,25 @@ describe("createOpenClawCodingTools safeBins", () => {
return;
}
const { createOpenClawCodingTools } = await import("./pi-tools.js");
const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-safe-bins-sort-"));
const cfg: OpenClawConfig = {
tools: {
exec: {
host: "gateway",
security: "allowlist",
ask: "off",
safeBins: ["sort"],
},
},
};
const tools = createOpenClawCodingTools({
config: cfg,
sessionKey: "agent:main:main",
workspaceDir: tmpDir,
agentDir: path.join(tmpDir, "agent"),
const { tmpDir, execTool } = await createSafeBinsExecTool({
tmpPrefix: "openclaw-safe-bins-sort-",
safeBins: ["sort"],
});
const execTool = tools.find((tool) => tool.name === "exec");
expect(execTool).toBeDefined();
const shortTarget = path.join(tmpDir, "blocked-short.txt");
await expect(
execTool!.execute("call1", {
command: "sort -oblocked-short.txt",
workdir: tmpDir,
}),
).rejects.toThrow("exec denied: allowlist miss");
expect(fs.existsSync(shortTarget)).toBe(false);
const cases = [
{ command: "sort -oblocked-short.txt", target: "blocked-short.txt" },
{ command: "sort --output=blocked-long.txt", target: "blocked-long.txt" },
] as const;
const longTarget = path.join(tmpDir, "blocked-long.txt");
await expect(
execTool!.execute("call2", {
command: "sort --output=blocked-long.txt",
workdir: tmpDir,
}),
).rejects.toThrow("exec denied: allowlist miss");
expect(fs.existsSync(longTarget)).toBe(false);
for (const [index, testCase] of cases.entries()) {
await expect(
execTool.execute(`call${index + 1}`, {
command: testCase.command,
workdir: tmpDir,
}),
).rejects.toThrow("exec denied: allowlist miss");
expect(fs.existsSync(path.join(tmpDir, testCase.target))).toBe(false);
}
});
it("blocks grep recursive flags from reading cwd via safeBins", async () => {
@@ -251,36 +227,14 @@ describe("createOpenClawCodingTools safeBins", () => {
return;
}
const { createOpenClawCodingTools } = await import("./pi-tools.js");
const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-safe-bins-grep-"));
fs.writeFileSync(
path.join(tmpDir, "secret.txt"),
"SAFE_BINS_RECURSIVE_SHOULD_NOT_LEAK\n",
"utf8",
);
const cfg: OpenClawConfig = {
tools: {
exec: {
host: "gateway",
security: "allowlist",
ask: "off",
safeBins: ["grep"],
},
},
};
const tools = createOpenClawCodingTools({
config: cfg,
sessionKey: "agent:main:main",
workspaceDir: tmpDir,
agentDir: path.join(tmpDir, "agent"),
const { tmpDir, execTool } = await createSafeBinsExecTool({
tmpPrefix: "openclaw-safe-bins-grep-",
safeBins: ["grep"],
files: [{ name: "secret.txt", contents: "SAFE_BINS_RECURSIVE_SHOULD_NOT_LEAK\n" }],
});
const execTool = tools.find((tool) => tool.name === "exec");
expect(execTool).toBeDefined();
await expect(
execTool!.execute("call1", {
execTool.execute("call1", {
command: "grep -R SAFE_BINS_RECURSIVE_SHOULD_NOT_LEAK",
workdir: tmpDir,
}),

View File

@@ -1,4 +1,5 @@
import path from "node:path";
import type { ExecAllowlistEntry } from "./exec-approvals.js";
import {
DEFAULT_SAFE_BINS,
analyzeShellCommand,
@@ -10,26 +11,13 @@ import {
type CommandResolution,
type ExecCommandSegment,
} from "./exec-approvals-analysis.js";
import type { ExecAllowlistEntry } from "./exec-approvals.js";
import {
SAFE_BIN_GENERIC_PROFILE,
SAFE_BIN_PROFILES,
validateSafeBinArgv,
} from "./exec-safe-bin-policy.js";
import { isTrustedSafeBinPath } from "./exec-safe-bin-trust.js";
function isPathLikeToken(value: string): boolean {
const trimmed = value.trim();
if (!trimmed) {
return false;
}
if (trimmed === "-") {
return false;
}
if (trimmed.startsWith("./") || trimmed.startsWith("../") || trimmed.startsWith("~")) {
return true;
}
if (trimmed.startsWith("/")) {
return true;
}
return /^[A-Za-z]:[\\/]/.test(trimmed);
}
export function normalizeSafeBins(entries?: string[]): Set<string> {
if (!Array.isArray(entries)) {
return new Set();
@@ -47,259 +35,6 @@ export function resolveSafeBins(entries?: string[] | null): Set<string> {
return normalizeSafeBins(entries ?? []);
}
function hasGlobToken(value: string): boolean {
// Safe bins are stdin-only; globbing is both surprising and a historical bypass vector.
// Note: we still harden execution-time expansion separately.
return /[*?[\]]/.test(value);
}
type SafeBinProfile = {
minPositional?: number;
maxPositional?: number;
valueFlags?: ReadonlySet<string>;
blockedFlags?: ReadonlySet<string>;
};
const NO_FLAGS = new Set<string>();
const SAFE_BIN_GENERIC_PROFILE: SafeBinProfile = {};
const SAFE_BIN_PROFILES: Record<string, SafeBinProfile> = {
jq: {
maxPositional: 1,
valueFlags: new Set([
"--arg",
"--argjson",
"--argstr",
"--argfile",
"--rawfile",
"--slurpfile",
"--from-file",
"--library-path",
"-L",
"-f",
]),
blockedFlags: new Set([
"--argfile",
"--rawfile",
"--slurpfile",
"--from-file",
"--library-path",
"-L",
"-f",
]),
},
grep: {
maxPositional: 1,
valueFlags: new Set([
"--regexp",
"--file",
"--max-count",
"--after-context",
"--before-context",
"--context",
"--devices",
"--directories",
"--binary-files",
"--exclude",
"--exclude-from",
"--include",
"--label",
"-e",
"-f",
"-m",
"-A",
"-B",
"-C",
"-D",
"-d",
]),
blockedFlags: new Set([
"--file",
"--exclude-from",
"--dereference-recursive",
"--directories",
"--recursive",
"-f",
"-d",
"-r",
"-R",
]),
},
cut: {
maxPositional: 0,
valueFlags: new Set([
"--bytes",
"--characters",
"--fields",
"--delimiter",
"--output-delimiter",
"-b",
"-c",
"-f",
"-d",
]),
},
sort: {
maxPositional: 0,
valueFlags: new Set([
"--key",
"--field-separator",
"--buffer-size",
"--temporary-directory",
"--compress-program",
"--parallel",
"--batch-size",
"--random-source",
"--files0-from",
"--output",
"-k",
"-t",
"-S",
"-T",
"-o",
]),
blockedFlags: new Set(["--files0-from", "--output", "-o"]),
},
uniq: {
maxPositional: 0,
valueFlags: new Set([
"--skip-fields",
"--skip-chars",
"--check-chars",
"--group",
"-f",
"-s",
"-w",
]),
},
head: {
maxPositional: 0,
valueFlags: new Set(["--lines", "--bytes", "-n", "-c"]),
},
tail: {
maxPositional: 0,
valueFlags: new Set([
"--lines",
"--bytes",
"--sleep-interval",
"--max-unchanged-stats",
"--pid",
"-n",
"-c",
]),
},
tr: {
minPositional: 1,
maxPositional: 2,
},
wc: {
maxPositional: 0,
valueFlags: new Set(["--files0-from"]),
blockedFlags: new Set(["--files0-from"]),
},
};
function isSafeLiteralToken(value: string): boolean {
if (!value || value === "-") {
return true;
}
return !hasGlobToken(value) && !isPathLikeToken(value);
}
function validateSafeBinArgv(args: string[], profile: SafeBinProfile): boolean {
const valueFlags = profile.valueFlags ?? NO_FLAGS;
const blockedFlags = profile.blockedFlags ?? NO_FLAGS;
const positional: string[] = [];
for (let i = 0; i < args.length; i += 1) {
const token = args[i];
if (!token) {
continue;
}
if (token === "--") {
for (let j = i + 1; j < args.length; j += 1) {
const rest = args[j];
if (!rest || rest === "-") {
continue;
}
if (!isSafeLiteralToken(rest)) {
return false;
}
positional.push(rest);
}
break;
}
if (token === "-") {
continue;
}
if (!token.startsWith("-")) {
if (!isSafeLiteralToken(token)) {
return false;
}
positional.push(token);
continue;
}
if (token.startsWith("--")) {
const eqIndex = token.indexOf("=");
const flag = eqIndex > 0 ? token.slice(0, eqIndex) : token;
if (blockedFlags.has(flag)) {
return false;
}
if (eqIndex > 0) {
if (!isSafeLiteralToken(token.slice(eqIndex + 1))) {
return false;
}
continue;
}
if (!valueFlags.has(flag)) {
continue;
}
const value = args[i + 1];
if (!value || !isSafeLiteralToken(value)) {
return false;
}
i += 1;
continue;
}
let consumedValue = false;
for (let j = 1; j < token.length; j += 1) {
const flag = `-${token[j]}`;
if (blockedFlags.has(flag)) {
return false;
}
if (!valueFlags.has(flag)) {
continue;
}
const inlineValue = token.slice(j + 1);
if (inlineValue) {
if (!isSafeLiteralToken(inlineValue)) {
return false;
}
} else {
const value = args[i + 1];
if (!value || !isSafeLiteralToken(value)) {
return false;
}
i += 1;
}
consumedValue = true;
break;
}
if (!consumedValue && hasGlobToken(token)) {
return false;
}
}
const minPositional = profile.minPositional ?? 0;
if (positional.length < minPositional) {
return false;
}
if (typeof profile.maxPositional === "number" && positional.length > profile.maxPositional) {
return false;
}
return true;
}
export function isSafeBinUsage(params: {
argv: string[];
resolution: CommandResolution | null;
@@ -321,9 +56,7 @@ export function isSafeBinUsage(params: {
if (!execName) {
return false;
}
const matchesSafeBin =
params.safeBins.has(execName) ||
(process.platform === "win32" && params.safeBins.has(path.parse(execName).name));
const matchesSafeBin = params.safeBins.has(execName);
if (!matchesSafeBin) {
return false;
}

View File

@@ -0,0 +1,272 @@
function isPathLikeToken(value: string): boolean {
const trimmed = value.trim();
if (!trimmed) {
return false;
}
if (trimmed === "-") {
return false;
}
if (trimmed.startsWith("./") || trimmed.startsWith("../") || trimmed.startsWith("~")) {
return true;
}
if (trimmed.startsWith("/")) {
return true;
}
return /^[A-Za-z]:[\\/]/.test(trimmed);
}
function hasGlobToken(value: string): boolean {
// Safe bins are stdin-only; globbing is both surprising and a historical bypass vector.
// Note: we still harden execution-time expansion separately.
return /[*?[\]]/.test(value);
}
export type SafeBinProfile = {
minPositional?: number;
maxPositional?: number;
valueFlags?: ReadonlySet<string>;
blockedFlags?: ReadonlySet<string>;
};
const NO_FLAGS = new Set<string>();
export const SAFE_BIN_GENERIC_PROFILE: SafeBinProfile = {};
export const SAFE_BIN_PROFILES: Record<string, SafeBinProfile> = {
jq: {
maxPositional: 1,
valueFlags: new Set([
"--arg",
"--argjson",
"--argstr",
"--argfile",
"--rawfile",
"--slurpfile",
"--from-file",
"--library-path",
"-L",
"-f",
]),
blockedFlags: new Set([
"--argfile",
"--rawfile",
"--slurpfile",
"--from-file",
"--library-path",
"-L",
"-f",
]),
},
grep: {
maxPositional: 1,
valueFlags: new Set([
"--regexp",
"--file",
"--max-count",
"--after-context",
"--before-context",
"--context",
"--devices",
"--directories",
"--binary-files",
"--exclude",
"--exclude-from",
"--include",
"--label",
"-e",
"-f",
"-m",
"-A",
"-B",
"-C",
"-D",
"-d",
]),
blockedFlags: new Set([
"--file",
"--exclude-from",
"--dereference-recursive",
"--directories",
"--recursive",
"-f",
"-d",
"-r",
"-R",
]),
},
cut: {
maxPositional: 0,
valueFlags: new Set([
"--bytes",
"--characters",
"--fields",
"--delimiter",
"--output-delimiter",
"-b",
"-c",
"-f",
"-d",
]),
},
sort: {
maxPositional: 0,
valueFlags: new Set([
"--key",
"--field-separator",
"--buffer-size",
"--temporary-directory",
"--compress-program",
"--parallel",
"--batch-size",
"--random-source",
"--files0-from",
"--output",
"-k",
"-t",
"-S",
"-T",
"-o",
]),
blockedFlags: new Set(["--files0-from", "--output", "-o"]),
},
uniq: {
maxPositional: 0,
valueFlags: new Set([
"--skip-fields",
"--skip-chars",
"--check-chars",
"--group",
"-f",
"-s",
"-w",
]),
},
head: {
maxPositional: 0,
valueFlags: new Set(["--lines", "--bytes", "-n", "-c"]),
},
tail: {
maxPositional: 0,
valueFlags: new Set([
"--lines",
"--bytes",
"--sleep-interval",
"--max-unchanged-stats",
"--pid",
"-n",
"-c",
]),
},
tr: {
minPositional: 1,
maxPositional: 2,
},
wc: {
maxPositional: 0,
valueFlags: new Set(["--files0-from"]),
blockedFlags: new Set(["--files0-from"]),
},
};
function isSafeLiteralToken(value: string): boolean {
if (!value || value === "-") {
return true;
}
return !hasGlobToken(value) && !isPathLikeToken(value);
}
export function validateSafeBinArgv(args: string[], profile: SafeBinProfile): boolean {
const valueFlags = profile.valueFlags ?? NO_FLAGS;
const blockedFlags = profile.blockedFlags ?? NO_FLAGS;
const positional: string[] = [];
for (let i = 0; i < args.length; i += 1) {
const token = args[i];
if (!token) {
continue;
}
if (token === "--") {
for (let j = i + 1; j < args.length; j += 1) {
const rest = args[j];
if (!rest || rest === "-") {
continue;
}
if (!isSafeLiteralToken(rest)) {
return false;
}
positional.push(rest);
}
break;
}
if (token === "-") {
continue;
}
if (!token.startsWith("-")) {
if (!isSafeLiteralToken(token)) {
return false;
}
positional.push(token);
continue;
}
if (token.startsWith("--")) {
const eqIndex = token.indexOf("=");
const flag = eqIndex > 0 ? token.slice(0, eqIndex) : token;
if (blockedFlags.has(flag)) {
return false;
}
if (eqIndex > 0) {
if (!isSafeLiteralToken(token.slice(eqIndex + 1))) {
return false;
}
continue;
}
if (!valueFlags.has(flag)) {
continue;
}
const value = args[i + 1];
if (!value || !isSafeLiteralToken(value)) {
return false;
}
i += 1;
continue;
}
let consumedValue = false;
for (let j = 1; j < token.length; j += 1) {
const flag = `-${token[j]}`;
if (blockedFlags.has(flag)) {
return false;
}
if (!valueFlags.has(flag)) {
continue;
}
const inlineValue = token.slice(j + 1);
if (inlineValue) {
if (!isSafeLiteralToken(inlineValue)) {
return false;
}
} else {
const value = args[i + 1];
if (!value || !isSafeLiteralToken(value)) {
return false;
}
i += 1;
}
consumedValue = true;
break;
}
if (!consumedValue && hasGlobToken(token)) {
return false;
}
}
const minPositional = profile.minPositional ?? 0;
if (positional.length < minPositional) {
return false;
}
if (typeof profile.maxPositional === "number" && positional.length > profile.maxPositional) {
return false;
}
return true;
}