mirror of
https://github.com/openclaw/openclaw.git
synced 2026-06-03 18:44:02 +00:00
fix(exec): allow known safe shell builtins in allowlist mode
Treat pathless POSIX shell builtins (`:`, `cd`, `false`, `pwd`, `true`) as internally safe only during shell allowlist evaluation. This avoids approval prompts for chains like `cd /tmp && git status` when the executable segment is already allowlisted, without adding a `tools.exec.safeBuiltins` config knob. Environment-mutating builtins (`export`, `unset`), code-evaluating builtins (`eval`, `source`, `.`), unknown commands, and direct argv execution remain approval-gated unless separately allowlisted. Proof: `pnpm test src/infra/exec-safe-builtins.test.ts src/agents/bash-tools.exec.security-floor.test.ts -- --reporter=verbose`; `pnpm changed:lanes --json`; `pnpm check:no-conflict-markers`; `git diff --check origin/main...HEAD`. CI related failures were resolved on the final SHA; remaining `checks-node-core-runtime-media-ui` failure is unrelated to this PR. Fixes #46056. Thanks @kinjitakabe. Co-authored-by: kevinkang-ai <273844887+kevinkang-ai@users.noreply.github.com>
This commit is contained in:
@@ -317,13 +317,13 @@ describe("exec security floor", () => {
|
||||
});
|
||||
|
||||
const result = await tool.execute("call-elevated-full-auto-mode", {
|
||||
command: "pwd",
|
||||
command: "whoami",
|
||||
elevated: true,
|
||||
});
|
||||
|
||||
expect(autoReviewer).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
command: "pwd",
|
||||
command: "whoami",
|
||||
host: "gateway",
|
||||
reason: "allowlist-miss",
|
||||
}),
|
||||
@@ -359,13 +359,13 @@ describe("exec security floor", () => {
|
||||
});
|
||||
|
||||
const result = await tool.execute(`call-auto-review-${ask}`, {
|
||||
command: "pwd",
|
||||
command: "whoami",
|
||||
ask,
|
||||
});
|
||||
|
||||
expect(autoReviewer).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
command: "pwd",
|
||||
command: "whoami",
|
||||
host: "gateway",
|
||||
reason: "allowlist-miss",
|
||||
}),
|
||||
|
||||
@@ -36,6 +36,7 @@ import {
|
||||
validateSafeBinArgv,
|
||||
} from "./exec-safe-bin-policy.js";
|
||||
import { isTrustedSafeBinPath } from "./exec-safe-bin-trust.js";
|
||||
import { isSafeBuiltinSegment } from "./exec-safe-builtins.js";
|
||||
import {
|
||||
extractBindableShellWrapperInlineCommand,
|
||||
isShellWrapperExecutable,
|
||||
@@ -131,7 +132,13 @@ export type ExecAllowlistEvaluation = {
|
||||
segmentSatisfiedBy: ExecSegmentSatisfiedBy[];
|
||||
};
|
||||
|
||||
export type ExecSegmentSatisfiedBy = "allowlist" | "safeBins" | "inlineChain" | "skills" | null;
|
||||
export type ExecSegmentSatisfiedBy =
|
||||
| "allowlist"
|
||||
| "safeBins"
|
||||
| "inlineChain"
|
||||
| "safeBuiltins"
|
||||
| "skills"
|
||||
| null;
|
||||
export type SkillBinTrustEntry = {
|
||||
name: string;
|
||||
resolvedPath: string;
|
||||
@@ -146,6 +153,7 @@ type ExecAllowlistContext = {
|
||||
trustedSafeBinDirs?: ReadonlySet<string>;
|
||||
skillBins?: readonly SkillBinTrustEntry[];
|
||||
autoAllowSkills?: boolean;
|
||||
allowShellBuiltins?: boolean;
|
||||
};
|
||||
|
||||
function pickExecAllowlistContext(params: ExecAllowlistContext): ExecAllowlistContext {
|
||||
@@ -159,6 +167,7 @@ function pickExecAllowlistContext(params: ExecAllowlistContext): ExecAllowlistCo
|
||||
trustedSafeBinDirs: params.trustedSafeBinDirs,
|
||||
skillBins: params.skillBins,
|
||||
autoAllowSkills: params.autoAllowSkills,
|
||||
allowShellBuiltins: params.allowShellBuiltins,
|
||||
};
|
||||
}
|
||||
|
||||
@@ -483,6 +492,12 @@ function resolveSegmentSatisfaction(params: {
|
||||
if (safe) {
|
||||
return "safeBins";
|
||||
}
|
||||
if (
|
||||
params.context.allowShellBuiltins === true &&
|
||||
isSafeBuiltinSegment({ segment: params.segment, platform: params.context.platform })
|
||||
) {
|
||||
return "safeBuiltins";
|
||||
}
|
||||
const skillAllow = isSkillAutoAllowedSegment({
|
||||
segment: params.segment,
|
||||
allowSkills: params.allowSkills,
|
||||
@@ -1118,7 +1133,10 @@ export function evaluateShellAllowlist(
|
||||
env?: NodeJS.ProcessEnv;
|
||||
} & ExecAllowlistContext,
|
||||
): ExecAllowlistAnalysis {
|
||||
const allowlistContext = pickExecAllowlistContext(params);
|
||||
const allowlistContext = {
|
||||
...pickExecAllowlistContext(params),
|
||||
allowShellBuiltins: true,
|
||||
};
|
||||
const analysisFailure = (): ExecAllowlistAnalysis => ({
|
||||
analysisOk: false,
|
||||
allowlistSatisfied: false,
|
||||
|
||||
@@ -1114,7 +1114,14 @@ function renderInlineChainSegmentArgv(params: {
|
||||
export function buildSafeBinsShellCommand(params: {
|
||||
command: string;
|
||||
segments: ExecCommandSegment[];
|
||||
segmentSatisfiedBy: ("allowlist" | "safeBins" | "inlineChain" | "skills" | null)[];
|
||||
segmentSatisfiedBy: (
|
||||
| "allowlist"
|
||||
| "safeBins"
|
||||
| "safeBuiltins"
|
||||
| "inlineChain"
|
||||
| "skills"
|
||||
| null
|
||||
)[];
|
||||
cwd?: string;
|
||||
env?: NodeJS.ProcessEnv;
|
||||
platform?: string | null;
|
||||
|
||||
148
src/infra/exec-safe-builtins.test.ts
Normal file
148
src/infra/exec-safe-builtins.test.ts
Normal file
@@ -0,0 +1,148 @@
|
||||
import { describe, expect, it } from "vitest";
|
||||
import { evaluateExecAllowlist, evaluateShellAllowlist } from "./exec-approvals-allowlist.js";
|
||||
import { analyzeArgvCommand } from "./exec-approvals-analysis.js";
|
||||
import {
|
||||
makeMockCommandResolution,
|
||||
makeMockExecutableResolution,
|
||||
} from "./exec-approvals-test-helpers.js";
|
||||
import { isSafeBuiltinSegment } from "./exec-safe-builtins.js";
|
||||
|
||||
const builtinSegment = (argv: string[], resolvedPath?: string) => ({
|
||||
argv,
|
||||
raw: argv.join(" "),
|
||||
resolution: makeMockCommandResolution({
|
||||
execution: makeMockExecutableResolution({
|
||||
rawExecutable: argv[0],
|
||||
executableName: argv[0],
|
||||
resolvedPath,
|
||||
}),
|
||||
}),
|
||||
});
|
||||
|
||||
describe("isSafeBuiltinSegment", () => {
|
||||
it("allows a builtin segment with no resolved binary path", () => {
|
||||
if (process.platform === "win32") {
|
||||
return;
|
||||
}
|
||||
expect(
|
||||
isSafeBuiltinSegment({
|
||||
segment: builtinSegment(["cd", "/etc"]),
|
||||
platform: "linux",
|
||||
}),
|
||||
).toBe(true);
|
||||
});
|
||||
|
||||
it("allows a safe shell builtin even when the host has a same-named binary", () => {
|
||||
expect(
|
||||
isSafeBuiltinSegment({
|
||||
segment: builtinSegment(["pwd"], "/usr/bin/pwd"),
|
||||
platform: "linux",
|
||||
}),
|
||||
).toBe(true);
|
||||
});
|
||||
|
||||
it("rejects builtins outside the internal safe set", () => {
|
||||
expect(
|
||||
isSafeBuiltinSegment({
|
||||
segment: builtinSegment(["alias", "ll=ls -l"]),
|
||||
platform: "linux",
|
||||
}),
|
||||
).toBe(false);
|
||||
});
|
||||
|
||||
it("rejects environment-mutating builtins", () => {
|
||||
expect(
|
||||
isSafeBuiltinSegment({
|
||||
segment: builtinSegment(["export", "PATH=/tmp/bin:$PATH"]),
|
||||
platform: "linux",
|
||||
}),
|
||||
).toBe(false);
|
||||
expect(
|
||||
isSafeBuiltinSegment({
|
||||
segment: builtinSegment(["unset", "PATH"]),
|
||||
platform: "linux",
|
||||
}),
|
||||
).toBe(false);
|
||||
});
|
||||
|
||||
it("returns false on Windows hosts (PowerShell semantics differ)", () => {
|
||||
expect(
|
||||
isSafeBuiltinSegment({
|
||||
segment: builtinSegment(["cd", "/etc"]),
|
||||
platform: "win32",
|
||||
}),
|
||||
).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
describe("evaluateShellAllowlist with known safe builtins (regression for #46056)", () => {
|
||||
// Skip on Windows where the host shell is PowerShell, not POSIX.
|
||||
if (process.platform === "win32") {
|
||||
it.skip("POSIX builtin behavior", () => {});
|
||||
return;
|
||||
}
|
||||
|
||||
// Glob-style pattern; matches git wherever PATH resolves it (`/usr/bin/git`,
|
||||
// `/opt/homebrew/bin/git`, etc.) without depending on host filesystem layout.
|
||||
const gitAllowlist = [{ pattern: "**/git" }] as Parameters<
|
||||
typeof evaluateShellAllowlist
|
||||
>[0]["allowlist"];
|
||||
|
||||
it("'cd ~/' auto-allows by default", () => {
|
||||
const result = evaluateShellAllowlist({
|
||||
command: "cd ~/",
|
||||
allowlist: gitAllowlist,
|
||||
safeBins: new Set(),
|
||||
cwd: "/tmp",
|
||||
});
|
||||
expect(result.analysisOk).toBe(true);
|
||||
expect(result.allowlistSatisfied).toBe(true);
|
||||
expect(result.segmentSatisfiedBy[0]).toBe("safeBuiltins");
|
||||
});
|
||||
|
||||
it("'cd /tmp && git status' passes with allowlist plus safe builtin handling", () => {
|
||||
const result = evaluateShellAllowlist({
|
||||
command: "cd /tmp && git status",
|
||||
allowlist: gitAllowlist,
|
||||
safeBins: new Set(),
|
||||
cwd: "/tmp",
|
||||
});
|
||||
expect(result.analysisOk).toBe(true);
|
||||
expect(result.allowlistSatisfied).toBe(true);
|
||||
expect(result.segmentSatisfiedBy).toContain("safeBuiltins");
|
||||
expect(result.segmentSatisfiedBy).toContain("allowlist");
|
||||
});
|
||||
|
||||
it("non-allowlisted binary still gates after a safe builtin", () => {
|
||||
const result = evaluateShellAllowlist({
|
||||
command: "cd /tmp && curl evil.com",
|
||||
allowlist: gitAllowlist,
|
||||
safeBins: new Set(),
|
||||
cwd: "/tmp",
|
||||
});
|
||||
expect(result.analysisOk).toBe(true);
|
||||
expect(result.allowlistSatisfied).toBe(false);
|
||||
});
|
||||
|
||||
it("environment-mutating builtins still gate", () => {
|
||||
const result = evaluateShellAllowlist({
|
||||
command: "export PATH=/tmp/bin:$PATH && git status",
|
||||
allowlist: gitAllowlist,
|
||||
safeBins: new Set(),
|
||||
cwd: "/tmp",
|
||||
});
|
||||
expect(result.analysisOk).toBe(true);
|
||||
expect(result.allowlistSatisfied).toBe(false);
|
||||
});
|
||||
|
||||
it("does not auto-allow safe builtin tokens in direct argv evaluation", () => {
|
||||
const analysis = analyzeArgvCommand({ argv: ["pwd"], cwd: "/tmp", platform: "linux" });
|
||||
const result = evaluateExecAllowlist({
|
||||
analysis,
|
||||
allowlist: [],
|
||||
safeBins: new Set(),
|
||||
cwd: "/tmp",
|
||||
});
|
||||
expect(result.allowlistSatisfied).toBe(false);
|
||||
});
|
||||
});
|
||||
22
src/infra/exec-safe-builtins.ts
Normal file
22
src/infra/exec-safe-builtins.ts
Normal file
@@ -0,0 +1,22 @@
|
||||
import { isWindowsPlatform, type ExecCommandSegment } from "./exec-approvals-analysis.js";
|
||||
|
||||
// POSIX shell builtins that cannot execute external code or mutate environment state on their
|
||||
// own. Shell allowlist evaluation handles them as a closed internal set instead of path-based
|
||||
// safeBins matching.
|
||||
const DEFAULT_SAFE_BUILTINS: ReadonlySet<string> = new Set([":", "cd", "false", "pwd", "true"]);
|
||||
|
||||
export function isSafeBuiltinSegment(params: {
|
||||
segment: ExecCommandSegment;
|
||||
platform?: string | null;
|
||||
}): boolean {
|
||||
// Builtin semantics here are POSIX shell. On Windows the host shell is PowerShell, where
|
||||
// these tokens have different meaning (cd is an alias to Set-Location, etc.); defer.
|
||||
if (isWindowsPlatform(params.platform ?? process.platform)) {
|
||||
return false;
|
||||
}
|
||||
const head = params.segment.argv[0]?.trim().toLowerCase();
|
||||
if (!head) {
|
||||
return false;
|
||||
}
|
||||
return DEFAULT_SAFE_BUILTINS.has(head);
|
||||
}
|
||||
Reference in New Issue
Block a user