mirror of https://github.com/openclaw/openclaw.git
fix(exec): harden shell-side approval guardrails (#57839)
* fix(exec): harden approval handling * fix(exec): tighten approval guardrails * fix(exec): reject prefixed approval commands * fix(exec): isolate shell approval guardrails * fix(exec): recurse through wrapped approval commands * fix(exec): restore allowlist wrapper import * fix(exec): strip env wrappers before approval detection * fix(exec): inspect nested shell wrapper options
This commit is contained in:
scoootscooob
GitHub
parent
9ff57ac479
commit
dd9d0bdd8e
|
|
@ -1,69 +1,80 @@
|
|||
import { beforeEach, describe, expect, it, vi } from "vitest";
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
|
||||
|
||||
vi.mock("./tools/gateway.js", () => ({
|
||||
callGatewayTool: vi.fn(async () => ({ ok: true })),
|
||||
}));
|
||||
|
||||
let sendExecApprovalFollowup: typeof import("./bash-tools.exec-approval-followup.js").sendExecApprovalFollowup;
|
||||
let callGatewayTool: typeof import("./tools/gateway.js").callGatewayTool;
|
||||
let buildExecApprovalFollowupPrompt: typeof import("./bash-tools.exec-approval-followup.js").buildExecApprovalFollowupPrompt;
|
||||
let sendExecApprovalFollowup: typeof import("./bash-tools.exec-approval-followup.js").sendExecApprovalFollowup;
|
||||
|
||||
describe("sendExecApprovalFollowup", () => {
|
||||
beforeEach(async () => {
|
||||
vi.resetModules();
|
||||
vi.doMock("./tools/gateway.js", () => ({
|
||||
callGatewayTool: vi.fn(),
|
||||
}));
|
||||
({ sendExecApprovalFollowup } = await import("./bash-tools.exec-approval-followup.js"));
|
||||
({ callGatewayTool } = await import("./tools/gateway.js"));
|
||||
vi.mocked(callGatewayTool).mockReset();
|
||||
vi.mocked(callGatewayTool).mockResolvedValue({});
|
||||
beforeEach(async () => {
|
||||
vi.resetModules();
|
||||
({ callGatewayTool } = await import("./tools/gateway.js"));
|
||||
({ buildExecApprovalFollowupPrompt, sendExecApprovalFollowup } = await import(
|
||||
"./bash-tools.exec-approval-followup.js"
|
||||
));
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
vi.resetAllMocks();
|
||||
});
|
||||
|
||||
describe("exec approval followup", () => {
|
||||
it("uses an explicit denial prompt when the command did not run", () => {
|
||||
const prompt = buildExecApprovalFollowupPrompt(
|
||||
"Exec denied (gateway id=req-1, user-denied): uname -a",
|
||||
);
|
||||
|
||||
expect(prompt).toContain("did not run");
|
||||
expect(prompt).toContain("Do not mention, summarize, or reuse output");
|
||||
expect(prompt).not.toContain("already approved has completed");
|
||||
});
|
||||
|
||||
it("keeps followup session-only when no external delivery target exists", async () => {
|
||||
const ok = await sendExecApprovalFollowup({
|
||||
approvalId: "approval-1",
|
||||
it("keeps followups internal when no external route is available", async () => {
|
||||
await sendExecApprovalFollowup({
|
||||
approvalId: "req-1",
|
||||
sessionKey: "agent:main:main",
|
||||
resultText: "Exec finished (gateway id=approval-1, code 0)",
|
||||
resultText: "Exec completed: echo ok",
|
||||
});
|
||||
|
||||
expect(ok).toBe(true);
|
||||
expect(callGatewayTool).toHaveBeenCalledTimes(1);
|
||||
const payload = vi.mocked(callGatewayTool).mock.calls[0]?.[2] as Record<string, unknown>;
|
||||
expect(payload.deliver).toBe(false);
|
||||
expect(payload).not.toHaveProperty("bestEffortDeliver");
|
||||
expect(payload.channel).toBeUndefined();
|
||||
expect(payload.to).toBeUndefined();
|
||||
expect(callGatewayTool).toHaveBeenCalledWith(
|
||||
"agent",
|
||||
expect.any(Object),
|
||||
expect.objectContaining({
|
||||
sessionKey: "agent:main:main",
|
||||
deliver: false,
|
||||
channel: undefined,
|
||||
to: undefined,
|
||||
}),
|
||||
{ expectFinal: true },
|
||||
);
|
||||
});
|
||||
|
||||
it("keeps followup session-only when turn source is internal webchat", async () => {
|
||||
const ok = await sendExecApprovalFollowup({
|
||||
approvalId: "approval-2",
|
||||
sessionKey: "agent:main:main",
|
||||
turnSourceChannel: "webchat",
|
||||
turnSourceTo: "chat:123",
|
||||
resultText: "Exec finished (gateway id=approval-2, code 0)",
|
||||
});
|
||||
|
||||
expect(ok).toBe(true);
|
||||
const payload = vi.mocked(callGatewayTool).mock.calls[0]?.[2] as Record<string, unknown>;
|
||||
expect(payload.deliver).toBe(false);
|
||||
expect(payload).not.toHaveProperty("bestEffortDeliver");
|
||||
expect(payload.channel).toBe("webchat");
|
||||
expect(payload.to).toBe("chat:123");
|
||||
});
|
||||
|
||||
it("enables delivery for valid external turn source targets", async () => {
|
||||
const ok = await sendExecApprovalFollowup({
|
||||
approvalId: "approval-3",
|
||||
sessionKey: "agent:main:main",
|
||||
turnSourceChannel: " discord ",
|
||||
turnSourceTo: "channel:123",
|
||||
it("uses external delivery when a deliverable route is available", async () => {
|
||||
await sendExecApprovalFollowup({
|
||||
approvalId: "req-2",
|
||||
sessionKey: "agent:main:discord:channel:123",
|
||||
turnSourceChannel: "discord",
|
||||
turnSourceTo: "123",
|
||||
turnSourceAccountId: "default",
|
||||
resultText: "Exec finished (gateway id=approval-3, code 0)",
|
||||
turnSourceThreadId: "456",
|
||||
resultText: "Exec completed: echo ok",
|
||||
});
|
||||
|
||||
expect(ok).toBe(true);
|
||||
const payload = vi.mocked(callGatewayTool).mock.calls[0]?.[2] as Record<string, unknown>;
|
||||
expect(payload.deliver).toBe(true);
|
||||
expect(payload.bestEffortDeliver).toBe(true);
|
||||
expect(payload.channel).toBe("discord");
|
||||
expect(payload.to).toBe("channel:123");
|
||||
expect(payload.accountId).toBe("default");
|
||||
expect(callGatewayTool).toHaveBeenCalledWith(
|
||||
"agent",
|
||||
expect.any(Object),
|
||||
expect.objectContaining({
|
||||
sessionKey: "agent:main:discord:channel:123",
|
||||
deliver: true,
|
||||
bestEffortDeliver: true,
|
||||
channel: "discord",
|
||||
to: "123",
|
||||
accountId: "default",
|
||||
threadId: "456",
|
||||
}),
|
||||
{ expectFinal: true },
|
||||
);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -433,7 +433,7 @@ describe("exec approvals", () => {
|
|||
expect(calls).toContain("exec.approval.waitDecision");
|
||||
});
|
||||
|
||||
it("starts a direct agent follow-up after approved gateway exec completes", async () => {
|
||||
it("starts an internal agent follow-up after approved gateway exec completes without an external route", async () => {
|
||||
const agentCalls: Array<Record<string, unknown>> = [];
|
||||
|
||||
mockAcceptedApprovalFlow({
|
||||
|
|
@ -639,6 +639,55 @@ describe("exec approvals", () => {
|
|||
expect(calls).toContain("exec.approval.request");
|
||||
});
|
||||
|
||||
it("runs a skill wrapper chain without prompting when the wrapper is allowlisted", async () => {
|
||||
if (process.platform === "win32") {
|
||||
return;
|
||||
}
|
||||
const tempDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-skill-wrapper-"));
|
||||
try {
|
||||
const skillDir = path.join(tempDir, ".openclaw", "skills", "gog");
|
||||
const skillPath = path.join(skillDir, "SKILL.md");
|
||||
const binDir = path.join(tempDir, "bin");
|
||||
const wrapperPath = path.join(binDir, "gog-wrapper");
|
||||
await fs.mkdir(skillDir, { recursive: true });
|
||||
await fs.mkdir(binDir, { recursive: true });
|
||||
await fs.writeFile(skillPath, "# gog skill\n");
|
||||
await fs.writeFile(wrapperPath, "#!/bin/sh\necho '{\"events\":[]}'\n");
|
||||
await fs.chmod(wrapperPath, 0o755);
|
||||
|
||||
await writeExecApprovalsConfig({
|
||||
version: 1,
|
||||
defaults: { security: "allowlist", ask: "off", askFallback: "deny" },
|
||||
agents: {
|
||||
main: {
|
||||
allowlist: [{ pattern: wrapperPath }],
|
||||
},
|
||||
},
|
||||
});
|
||||
|
||||
const calls: string[] = [];
|
||||
mockGatewayOkCalls(calls);
|
||||
|
||||
const tool = createExecTool({
|
||||
host: "gateway",
|
||||
ask: "off",
|
||||
security: "allowlist",
|
||||
approvalRunningNoticeMs: 0,
|
||||
});
|
||||
|
||||
const result = await tool.execute("call-skill-wrapper", {
|
||||
command: `cat ${JSON.stringify(skillPath)} && printf '\\n---CMD---\\n' && ${JSON.stringify(wrapperPath)} calendar events primary --today --json`,
|
||||
workdir: tempDir,
|
||||
});
|
||||
|
||||
expect(result.details.status).toBe("completed");
|
||||
expect(getResultText(result)).toContain('{"events":[]}');
|
||||
expect(calls).not.toContain("exec.approval.request");
|
||||
} finally {
|
||||
await fs.rm(tempDir, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
it("shows full chained node commands in approval-pending message", async () => {
|
||||
const calls: string[] = [];
|
||||
vi.mocked(callGatewayTool).mockImplementation(async (method, _opts, params) => {
|
||||
|
|
|
|||
|
|
@ -269,4 +269,31 @@ describe("exec host env validation", () => {
|
|||
}),
|
||||
).rejects.toThrow(/requires a sandbox runtime/);
|
||||
});
|
||||
|
||||
it.each([
|
||||
"echo ok && /approve abc123 allow-once",
|
||||
"echo ok | /approve abc123 deny",
|
||||
"echo ok\n/approve abc123 allow-once",
|
||||
"FOO=1 /approve abc123 allow-once",
|
||||
"env -i /approve abc123 deny",
|
||||
"env --ignore-environment /approve abc123 allow-once",
|
||||
"env -i FOO=1 /approve abc123 allow-once",
|
||||
"env -S '/approve abc123 deny'",
|
||||
"command /approve abc123 deny",
|
||||
"command -p /approve abc123 deny",
|
||||
"exec -a openclaw /approve abc123 deny",
|
||||
"sudo /approve abc123 allow-once",
|
||||
"sudo -E /approve abc123 allow-once",
|
||||
"bash -lc '/approve abc123 deny'",
|
||||
"bash -c 'sudo /approve abc123 allow-once'",
|
||||
"sh -c '/approve abc123 allow-once'",
|
||||
])("rejects /approve shell commands in %s", async (command) => {
|
||||
const tool = createExecTool({ host: "gateway", security: "full", ask: "off" });
|
||||
|
||||
await expect(
|
||||
tool.execute("call-approve", {
|
||||
command,
|
||||
}),
|
||||
).rejects.toThrow(/exec cannot run \/approve commands/);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -1,6 +1,7 @@
|
|||
import fs from "node:fs/promises";
|
||||
import path from "node:path";
|
||||
import type { AgentTool, AgentToolResult } from "@mariozechner/pi-agent-core";
|
||||
import { analyzeShellCommand } from "../infra/exec-approvals-analysis.js";
|
||||
import { type ExecHost, loadExecApprovals, maxAsk, minSecurity } from "../infra/exec-approvals.js";
|
||||
import { resolveExecSafeBinRuntimePolicy } from "../infra/exec-safe-bin-runtime-policy.js";
|
||||
import { sanitizeHostExecEnvWithDiagnostics } from "../infra/host-env-security.js";
|
||||
|
|
@ -10,6 +11,7 @@ import {
|
|||
} from "../infra/shell-env.js";
|
||||
import { logInfo } from "../logger.js";
|
||||
import { parseAgentSessionKey, resolveAgentIdFromSessionKey } from "../routing/session-key.js";
|
||||
import { splitShellArgs } from "../utils/shell-argv.js";
|
||||
import { markBackgrounded } from "./bash-process-registry.js";
|
||||
import { processGatewayAllowlist } from "./bash-tools.exec-host-gateway.js";
|
||||
import { executeNodeHostCommand } from "./bash-tools.exec-host-node.js";
|
||||
|
|
@ -173,6 +175,261 @@ async function validateScriptFileForShellBleed(params: {
|
|||
}
|
||||
}
|
||||
|
||||
type ParsedExecApprovalCommand = {
|
||||
approvalId: string;
|
||||
decision: "allow-once" | "allow-always" | "deny";
|
||||
};
|
||||
|
||||
function parseExecApprovalShellCommand(raw: string): ParsedExecApprovalCommand | null {
|
||||
const normalized = raw.trimStart();
|
||||
const match = normalized.match(
|
||||
/^\/approve(?:@[^\s]+)?\s+([A-Za-z0-9][A-Za-z0-9._:-]*)\s+(allow-once|allow-always|always|deny)\b/i,
|
||||
);
|
||||
if (!match) {
|
||||
return null;
|
||||
}
|
||||
return {
|
||||
approvalId: match[1],
|
||||
decision:
|
||||
match[2].toLowerCase() === "always"
|
||||
? "allow-always"
|
||||
: (match[2].toLowerCase() as ParsedExecApprovalCommand["decision"]),
|
||||
};
|
||||
}
|
||||
|
||||
function rejectExecApprovalShellCommand(command: string): void {
|
||||
const isEnvAssignmentToken = (token: string): boolean =>
|
||||
/^[A-Za-z_][A-Za-z0-9_]*=.*$/u.test(token);
|
||||
const shellWrappers = new Set(["bash", "dash", "fish", "ksh", "sh", "zsh"]);
|
||||
const commandStandaloneOptions = new Set(["-p", "-v", "-V"]);
|
||||
const envOptionsWithValues = new Set([
|
||||
"-C",
|
||||
"-S",
|
||||
"-u",
|
||||
"--argv0",
|
||||
"--block-signal",
|
||||
"--chdir",
|
||||
"--default-signal",
|
||||
"--ignore-signal",
|
||||
"--split-string",
|
||||
"--unset",
|
||||
]);
|
||||
const execOptionsWithValues = new Set(["-a"]);
|
||||
const execStandaloneOptions = new Set(["-c", "-l"]);
|
||||
const sudoOptionsWithValues = new Set([
|
||||
"-C",
|
||||
"-D",
|
||||
"-g",
|
||||
"-p",
|
||||
"-R",
|
||||
"-T",
|
||||
"-U",
|
||||
"-u",
|
||||
"--chdir",
|
||||
"--close-from",
|
||||
"--group",
|
||||
"--host",
|
||||
"--other-user",
|
||||
"--prompt",
|
||||
"--role",
|
||||
"--type",
|
||||
"--user",
|
||||
]);
|
||||
const sudoStandaloneOptions = new Set(["-A", "-E", "--askpass", "--preserve-env"]);
|
||||
const extractEnvSplitStringPayload = (argv: string[]): string[] => {
|
||||
const remaining = [...argv];
|
||||
while (remaining[0] && isEnvAssignmentToken(remaining[0])) {
|
||||
remaining.shift();
|
||||
}
|
||||
if (remaining[0] !== "env") {
|
||||
return [];
|
||||
}
|
||||
remaining.shift();
|
||||
const payloads: string[] = [];
|
||||
while (remaining.length > 0) {
|
||||
while (remaining[0] && isEnvAssignmentToken(remaining[0])) {
|
||||
remaining.shift();
|
||||
}
|
||||
const token: string | undefined = remaining[0];
|
||||
if (!token) {
|
||||
break;
|
||||
}
|
||||
if (token === "--") {
|
||||
remaining.shift();
|
||||
continue;
|
||||
}
|
||||
if (!token.startsWith("-") || token === "-") {
|
||||
break;
|
||||
}
|
||||
const option = remaining.shift()!;
|
||||
const normalized = option.split("=", 1)[0];
|
||||
if (normalized === "-S" || normalized === "--split-string") {
|
||||
const value = option.includes("=")
|
||||
? option.slice(option.indexOf("=") + 1)
|
||||
: remaining.shift();
|
||||
if (value?.trim()) {
|
||||
payloads.push(value);
|
||||
}
|
||||
continue;
|
||||
}
|
||||
if (envOptionsWithValues.has(normalized) && !option.includes("=") && remaining[0]) {
|
||||
remaining.shift();
|
||||
}
|
||||
}
|
||||
return payloads;
|
||||
};
|
||||
const stripApprovalCommandPrefixes = (argv: string[]): string[] => {
|
||||
const remaining = [...argv];
|
||||
while (remaining.length > 0) {
|
||||
while (remaining[0] && isEnvAssignmentToken(remaining[0])) {
|
||||
remaining.shift();
|
||||
}
|
||||
|
||||
const token = remaining[0];
|
||||
if (!token) {
|
||||
break;
|
||||
}
|
||||
if (token === "--") {
|
||||
remaining.shift();
|
||||
continue;
|
||||
}
|
||||
if (token === "env") {
|
||||
remaining.shift();
|
||||
while (remaining.length > 0) {
|
||||
while (remaining[0] && isEnvAssignmentToken(remaining[0])) {
|
||||
remaining.shift();
|
||||
}
|
||||
const envToken = remaining[0];
|
||||
if (!envToken) {
|
||||
break;
|
||||
}
|
||||
if (envToken === "--") {
|
||||
remaining.shift();
|
||||
continue;
|
||||
}
|
||||
if (!envToken.startsWith("-") || envToken === "-") {
|
||||
break;
|
||||
}
|
||||
const option = remaining.shift()!;
|
||||
const normalized = option.split("=", 1)[0];
|
||||
if (envOptionsWithValues.has(normalized) && !option.includes("=") && remaining[0]) {
|
||||
remaining.shift();
|
||||
}
|
||||
}
|
||||
continue;
|
||||
}
|
||||
if (token === "command" || token === "builtin") {
|
||||
remaining.shift();
|
||||
while (remaining[0]?.startsWith("-")) {
|
||||
const option = remaining.shift()!;
|
||||
if (option === "--") {
|
||||
break;
|
||||
}
|
||||
if (!commandStandaloneOptions.has(option.split("=", 1)[0])) {
|
||||
continue;
|
||||
}
|
||||
}
|
||||
continue;
|
||||
}
|
||||
if (token === "exec") {
|
||||
remaining.shift();
|
||||
while (remaining[0]?.startsWith("-")) {
|
||||
const option = remaining.shift()!;
|
||||
if (option === "--") {
|
||||
break;
|
||||
}
|
||||
const normalized = option.split("=", 1)[0];
|
||||
if (execStandaloneOptions.has(normalized)) {
|
||||
continue;
|
||||
}
|
||||
if (execOptionsWithValues.has(normalized) && !option.includes("=") && remaining[0]) {
|
||||
remaining.shift();
|
||||
}
|
||||
}
|
||||
continue;
|
||||
}
|
||||
if (token === "sudo") {
|
||||
remaining.shift();
|
||||
while (remaining[0]?.startsWith("-")) {
|
||||
const option = remaining.shift()!;
|
||||
if (option === "--") {
|
||||
break;
|
||||
}
|
||||
const normalized = option.split("=", 1)[0];
|
||||
if (sudoStandaloneOptions.has(normalized)) {
|
||||
continue;
|
||||
}
|
||||
if (sudoOptionsWithValues.has(normalized) && !option.includes("=") && remaining[0]) {
|
||||
remaining.shift();
|
||||
}
|
||||
}
|
||||
continue;
|
||||
}
|
||||
break;
|
||||
}
|
||||
return remaining;
|
||||
};
|
||||
const extractShellWrapperPayload = (argv: string[]): string[] => {
|
||||
const [commandName, ...rest] = argv;
|
||||
if (!commandName || !shellWrappers.has(path.basename(commandName))) {
|
||||
return [];
|
||||
}
|
||||
for (let i = 0; i < rest.length; i += 1) {
|
||||
const token = rest[i];
|
||||
if (!token) {
|
||||
continue;
|
||||
}
|
||||
if (token === "-c" || token === "-lc" || token === "-ic" || token === "-xc") {
|
||||
return rest[i + 1] ? [rest[i + 1]] : [];
|
||||
}
|
||||
if (/^-[^-]*c[^-]*$/u.test(token)) {
|
||||
return rest[i + 1] ? [rest[i + 1]] : [];
|
||||
}
|
||||
}
|
||||
return [];
|
||||
};
|
||||
const buildCandidates = (argv: string[]): string[] => {
|
||||
const envSplitCandidates = extractEnvSplitStringPayload(argv).flatMap((payload) => {
|
||||
const innerArgv = splitShellArgs(payload);
|
||||
return innerArgv ? buildCandidates(innerArgv) : [payload];
|
||||
});
|
||||
const stripped = stripApprovalCommandPrefixes(argv);
|
||||
const shellWrapperCandidates = extractShellWrapperPayload(stripped).flatMap((payload) => {
|
||||
const innerArgv = splitShellArgs(payload);
|
||||
return innerArgv ? buildCandidates(innerArgv) : [payload];
|
||||
});
|
||||
return [
|
||||
...(stripped.length > 0 ? [stripped.join(" ")] : []),
|
||||
...envSplitCandidates,
|
||||
...shellWrapperCandidates,
|
||||
];
|
||||
};
|
||||
|
||||
const rawCommand = command.trim();
|
||||
const analysis = analyzeShellCommand({ command: rawCommand });
|
||||
const candidates = analysis.ok
|
||||
? analysis.segments.flatMap((segment) => buildCandidates(segment.argv))
|
||||
: rawCommand
|
||||
.split(/\r?\n/)
|
||||
.map((line) => line.trim())
|
||||
.filter(Boolean)
|
||||
.flatMap((line) => {
|
||||
const argv = splitShellArgs(line);
|
||||
return argv ? buildCandidates(argv) : [line];
|
||||
});
|
||||
for (const candidate of candidates) {
|
||||
if (!parseExecApprovalShellCommand(candidate)) {
|
||||
continue;
|
||||
}
|
||||
throw new Error(
|
||||
[
|
||||
"exec cannot run /approve commands.",
|
||||
"Show the /approve command to the user as chat text, or route it through the approval command handler instead of shell execution.",
|
||||
].join(" "),
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
export function createExecTool(
|
||||
defaults?: ExecToolDefaults,
|
||||
// oxlint-disable-next-line typescript/no-explicit-any
|
||||
|
|
@ -378,6 +635,7 @@ export function createExecTool(
|
|||
// fall back to the gateway's cwd. The node is responsible for validating its own cwd.
|
||||
workdir = resolveWorkdir(rawWorkdir, warnings);
|
||||
}
|
||||
rejectExecApprovalShellCommand(params.command);
|
||||
|
||||
const inheritedBaseEnv = coerceEnv(process.env);
|
||||
const hostEnvResult =
|
||||
|
|
|
|||
|
|
@ -149,6 +149,16 @@ describe("buildAgentSystemPrompt", () => {
|
|||
);
|
||||
});
|
||||
|
||||
it("tells the agent not to execute /approve through exec", () => {
|
||||
const prompt = buildAgentSystemPrompt({
|
||||
workspaceDir: "/tmp/openclaw",
|
||||
});
|
||||
|
||||
expect(prompt).toContain(
|
||||
"Never execute /approve through exec or any other shell/tool path; /approve is a user-facing approval command, not a shell command.",
|
||||
);
|
||||
});
|
||||
|
||||
it("omits skills in minimal prompt mode when skillsPrompt is absent", () => {
|
||||
const prompt = buildAgentSystemPrompt({
|
||||
workspaceDir: "/tmp/openclaw",
|
||||
|
|
|
|||
|
|
@ -450,7 +450,8 @@ export function buildAgentSystemPrompt(params: {
|
|||
"Keep narration brief and value-dense; avoid repeating obvious steps.",
|
||||
"Use plain human language for narration unless in a technical context.",
|
||||
"When a first-class tool exists for an action, use the tool directly instead of asking the user to run equivalent CLI or slash commands.",
|
||||
"When exec returns approval-pending, include the concrete /approve command from tool output (with allow-once|allow-always|deny) and do not ask for a different or rotated code.",
|
||||
"When exec returns approval-pending, include the concrete /approve command from tool output (with allow-once|allow-always|deny) as plain chat text for the user, and do not ask for a different or rotated code.",
|
||||
"Never execute /approve through exec or any other shell/tool path; /approve is a user-facing approval command, not a shell command.",
|
||||
"Treat allow-once as single-command only: if another elevated command needs approval, request a fresh /approve and do not claim prior approval covered it.",
|
||||
"When approvals are required, preserve and show the full command/script exactly as provided (including chained operators like &&, ||, |, ;, or multiline shells) so the user can approve what will actually run.",
|
||||
"",
|
||||
|
|
|
|||
|
|
@ -8,10 +8,11 @@ import {
|
|||
resolveCommandResolutionFromArgv,
|
||||
resolvePolicyTargetCandidatePath,
|
||||
resolvePolicyTargetResolution,
|
||||
splitCommandChain,
|
||||
splitCommandChainWithOperators,
|
||||
type ExecCommandAnalysis,
|
||||
type ExecCommandSegment,
|
||||
type ExecutableResolution,
|
||||
type ShellChainOperator,
|
||||
} from "./exec-approvals-analysis.js";
|
||||
import type { ExecAllowlistEntry } from "./exec-approvals.js";
|
||||
import { isInterpreterLikeAllowlistPattern } from "./exec-inline-eval.js";
|
||||
|
|
@ -25,6 +26,7 @@ import { isTrustedSafeBinPath } from "./exec-safe-bin-trust.js";
|
|||
import {
|
||||
extractShellWrapperInlineCommand,
|
||||
isShellWrapperExecutable,
|
||||
normalizeExecutableToken,
|
||||
} from "./exec-wrapper-resolution.js";
|
||||
import { resolveExecWrapperTrustPlan } from "./exec-wrapper-trust-plan.js";
|
||||
import { expandHomePrefix } from "./home-dir.js";
|
||||
|
|
@ -107,7 +109,7 @@ export type ExecAllowlistEvaluation = {
|
|||
segmentSatisfiedBy: ExecSegmentSatisfiedBy[];
|
||||
};
|
||||
|
||||
export type ExecSegmentSatisfiedBy = "allowlist" | "safeBins" | "skills" | null;
|
||||
export type ExecSegmentSatisfiedBy = "allowlist" | "safeBins" | "skills" | "skillPrelude" | null;
|
||||
export type SkillBinTrustEntry = {
|
||||
name: string;
|
||||
resolvedPath: string;
|
||||
|
|
@ -200,6 +202,163 @@ function isSkillAutoAllowedSegment(params: {
|
|||
return Boolean(params.skillBinTrust.get(executableName)?.has(resolvedPath));
|
||||
}
|
||||
|
||||
function resolveSkillPreludePath(rawPath: string, cwd?: string): string {
|
||||
const expanded = rawPath.startsWith("~") ? expandHomePrefix(rawPath) : rawPath;
|
||||
if (path.isAbsolute(expanded)) {
|
||||
return path.resolve(expanded);
|
||||
}
|
||||
return path.resolve(cwd?.trim() || process.cwd(), expanded);
|
||||
}
|
||||
|
||||
function isSkillMarkdownPreludePath(filePath: string): boolean {
|
||||
const normalized = filePath.replace(/\\/g, "/");
|
||||
const lowerNormalized = normalized.toLowerCase();
|
||||
if (!lowerNormalized.endsWith("/skill.md")) {
|
||||
return false;
|
||||
}
|
||||
const parts = lowerNormalized.split("/").filter(Boolean);
|
||||
if (parts.length < 2) {
|
||||
return false;
|
||||
}
|
||||
for (let index = parts.length - 2; index >= 0; index -= 1) {
|
||||
if (parts[index] !== "skills") {
|
||||
continue;
|
||||
}
|
||||
const segmentsAfterSkills = parts.length - index - 1;
|
||||
if (segmentsAfterSkills === 1 || segmentsAfterSkills === 2) {
|
||||
return true;
|
||||
}
|
||||
}
|
||||
return false;
|
||||
}
|
||||
|
||||
function resolveSkillMarkdownPreludeId(filePath: string): string | null {
|
||||
const normalized = filePath.replace(/\\/g, "/");
|
||||
const lowerNormalized = normalized.toLowerCase();
|
||||
if (!lowerNormalized.endsWith("/skill.md")) {
|
||||
return null;
|
||||
}
|
||||
const parts = lowerNormalized.split("/").filter(Boolean);
|
||||
if (parts.length < 3) {
|
||||
return null;
|
||||
}
|
||||
for (let index = parts.length - 2; index >= 0; index -= 1) {
|
||||
if (parts[index] !== "skills") {
|
||||
continue;
|
||||
}
|
||||
if (parts.length - index - 1 !== 2) {
|
||||
continue;
|
||||
}
|
||||
const skillId = parts[index + 1]?.trim();
|
||||
return skillId || null;
|
||||
}
|
||||
return null;
|
||||
}
|
||||
|
||||
function isSkillPreludeReadSegment(segment: ExecCommandSegment, cwd?: string): boolean {
|
||||
const execution = resolveExecutionTargetResolution(segment.resolution);
|
||||
if (execution?.executableName?.toLowerCase() !== "cat") {
|
||||
return false;
|
||||
}
|
||||
// Keep the display-prelude exception narrow: only a plain `cat <...>/SKILL.md`
|
||||
// qualifies, not extra argv forms or arbitrary file reads.
|
||||
if (segment.argv.length !== 2) {
|
||||
return false;
|
||||
}
|
||||
const rawPath = segment.argv[1]?.trim();
|
||||
if (!rawPath) {
|
||||
return false;
|
||||
}
|
||||
return isSkillMarkdownPreludePath(resolveSkillPreludePath(rawPath, cwd));
|
||||
}
|
||||
|
||||
function isSkillPreludeMarkerSegment(segment: ExecCommandSegment): boolean {
|
||||
const execution = resolveExecutionTargetResolution(segment.resolution);
|
||||
if (execution?.executableName?.toLowerCase() !== "printf") {
|
||||
return false;
|
||||
}
|
||||
if (segment.argv.length !== 2) {
|
||||
return false;
|
||||
}
|
||||
const marker = segment.argv[1];
|
||||
return marker === "\\n---CMD---\\n" || marker === "\n---CMD---\n";
|
||||
}
|
||||
|
||||
function isSkillPreludeSegment(segment: ExecCommandSegment, cwd?: string): boolean {
|
||||
return isSkillPreludeReadSegment(segment, cwd) || isSkillPreludeMarkerSegment(segment);
|
||||
}
|
||||
|
||||
function isSkillPreludeOnlyEvaluation(
|
||||
segments: ExecCommandSegment[],
|
||||
cwd: string | undefined,
|
||||
): boolean {
|
||||
return segments.length > 0 && segments.every((segment) => isSkillPreludeSegment(segment, cwd));
|
||||
}
|
||||
|
||||
function resolveSkillPreludeIds(
|
||||
segments: ExecCommandSegment[],
|
||||
cwd: string | undefined,
|
||||
): ReadonlySet<string> {
|
||||
const skillIds = new Set<string>();
|
||||
for (const segment of segments) {
|
||||
if (!isSkillPreludeReadSegment(segment, cwd)) {
|
||||
continue;
|
||||
}
|
||||
const rawPath = segment.argv[1]?.trim();
|
||||
if (!rawPath) {
|
||||
continue;
|
||||
}
|
||||
const skillId = resolveSkillMarkdownPreludeId(resolveSkillPreludePath(rawPath, cwd));
|
||||
if (skillId) {
|
||||
skillIds.add(skillId);
|
||||
}
|
||||
}
|
||||
return skillIds;
|
||||
}
|
||||
|
||||
function resolveAllowlistedSkillWrapperId(segment: ExecCommandSegment): string | null {
|
||||
const execution = resolveExecutionTargetResolution(segment.resolution);
|
||||
const executableName = normalizeExecutableToken(
|
||||
execution?.executableName ?? segment.argv[0] ?? "",
|
||||
);
|
||||
if (!executableName.endsWith("-wrapper")) {
|
||||
return null;
|
||||
}
|
||||
const skillId = executableName.slice(0, -"-wrapper".length).trim();
|
||||
return skillId || null;
|
||||
}
|
||||
|
||||
function resolveTrustedSkillExecutionIds(params: {
|
||||
analysis: ExecCommandAnalysis;
|
||||
evaluation: ExecAllowlistEvaluation;
|
||||
}): ReadonlySet<string> {
|
||||
const skillIds = new Set<string>();
|
||||
if (!params.evaluation.allowlistSatisfied) {
|
||||
return skillIds;
|
||||
}
|
||||
for (const [index, segment] of params.analysis.segments.entries()) {
|
||||
const satisfiedBy = params.evaluation.segmentSatisfiedBy[index];
|
||||
if (satisfiedBy === "skills") {
|
||||
const execution = resolveExecutionTargetResolution(segment.resolution);
|
||||
const executableName = normalizeExecutableToken(
|
||||
execution?.executableName ?? execution?.rawExecutable ?? segment.argv[0] ?? "",
|
||||
);
|
||||
if (executableName) {
|
||||
skillIds.add(executableName);
|
||||
}
|
||||
continue;
|
||||
}
|
||||
if (satisfiedBy !== "allowlist") {
|
||||
continue;
|
||||
}
|
||||
const wrapperSkillId = resolveAllowlistedSkillWrapperId(segment);
|
||||
if (wrapperSkillId) {
|
||||
skillIds.add(wrapperSkillId);
|
||||
}
|
||||
}
|
||||
return skillIds;
|
||||
}
|
||||
|
||||
function evaluateSegments(
|
||||
segments: ExecCommandSegment[],
|
||||
params: ExecAllowlistContext,
|
||||
|
|
@ -531,7 +690,9 @@ export function evaluateShellAllowlist(
|
|||
return analysisFailure();
|
||||
}
|
||||
|
||||
const chainParts = isWindowsPlatform(params.platform) ? null : splitCommandChain(params.command);
|
||||
const chainParts = isWindowsPlatform(params.platform)
|
||||
? null
|
||||
: splitCommandChainWithOperators(params.command);
|
||||
if (!chainParts) {
|
||||
const analysis = analyzeShellCommand({
|
||||
command: params.command,
|
||||
|
|
@ -552,11 +713,7 @@ export function evaluateShellAllowlist(
|
|||
};
|
||||
}
|
||||
|
||||
const allowlistMatches: ExecAllowlistEntry[] = [];
|
||||
const segments: ExecCommandSegment[] = [];
|
||||
const segmentSatisfiedBy: ExecSegmentSatisfiedBy[] = [];
|
||||
|
||||
for (const part of chainParts) {
|
||||
const chainEvaluations = chainParts.map(({ part, opToNext }) => {
|
||||
const analysis = analyzeShellCommand({
|
||||
command: part,
|
||||
cwd: params.cwd,
|
||||
|
|
@ -564,14 +721,70 @@ export function evaluateShellAllowlist(
|
|||
platform: params.platform,
|
||||
});
|
||||
if (!analysis.ok) {
|
||||
return analysisFailure();
|
||||
return null;
|
||||
}
|
||||
return {
|
||||
analysis,
|
||||
evaluation: evaluateExecAllowlist({ analysis, ...allowlistContext }),
|
||||
opToNext,
|
||||
};
|
||||
});
|
||||
if (chainEvaluations.some((entry) => entry === null)) {
|
||||
return analysisFailure();
|
||||
}
|
||||
|
||||
const finalizedEvaluations = chainEvaluations as Array<{
|
||||
analysis: ExecCommandAnalysis;
|
||||
evaluation: ExecAllowlistEvaluation;
|
||||
opToNext: ShellChainOperator | null;
|
||||
}>;
|
||||
const allowSkillPreludeAtIndex = new Set<number>();
|
||||
const reachableSkillIds = new Set<string>();
|
||||
// Only allow the `cat SKILL.md && printf ...` display prelude when it sits on a
|
||||
// contiguous `&&` chain that actually reaches a later trusted skill-wrapper execution.
|
||||
for (let index = finalizedEvaluations.length - 1; index >= 0; index -= 1) {
|
||||
const { analysis, evaluation, opToNext } = finalizedEvaluations[index];
|
||||
const trustedSkillIds = resolveTrustedSkillExecutionIds({
|
||||
analysis,
|
||||
evaluation,
|
||||
});
|
||||
if (trustedSkillIds.size > 0) {
|
||||
for (const skillId of trustedSkillIds) {
|
||||
reachableSkillIds.add(skillId);
|
||||
}
|
||||
continue;
|
||||
}
|
||||
|
||||
const isPreludeOnly =
|
||||
!evaluation.allowlistSatisfied && isSkillPreludeOnlyEvaluation(analysis.segments, params.cwd);
|
||||
const preludeSkillIds = isPreludeOnly
|
||||
? resolveSkillPreludeIds(analysis.segments, params.cwd)
|
||||
: new Set<string>();
|
||||
const reachesTrustedSkillExecution =
|
||||
opToNext === "&&" &&
|
||||
(preludeSkillIds.size === 0
|
||||
? reachableSkillIds.size > 0
|
||||
: [...preludeSkillIds].some((skillId) => reachableSkillIds.has(skillId)));
|
||||
if (isPreludeOnly && reachesTrustedSkillExecution) {
|
||||
allowSkillPreludeAtIndex.add(index);
|
||||
continue;
|
||||
}
|
||||
|
||||
reachableSkillIds.clear();
|
||||
}
|
||||
const allowlistMatches: ExecAllowlistEntry[] = [];
|
||||
const segments: ExecCommandSegment[] = [];
|
||||
const segmentSatisfiedBy: ExecSegmentSatisfiedBy[] = [];
|
||||
|
||||
for (const [index, { analysis, evaluation }] of finalizedEvaluations.entries()) {
|
||||
const effectiveSegmentSatisfiedBy = allowSkillPreludeAtIndex.has(index)
|
||||
? analysis.segments.map(() => "skillPrelude" as const)
|
||||
: evaluation.segmentSatisfiedBy;
|
||||
|
||||
segments.push(...analysis.segments);
|
||||
const evaluation = evaluateExecAllowlist({ analysis, ...allowlistContext });
|
||||
allowlistMatches.push(...evaluation.allowlistMatches);
|
||||
segmentSatisfiedBy.push(...evaluation.segmentSatisfiedBy);
|
||||
if (!evaluation.allowlistSatisfied) {
|
||||
segmentSatisfiedBy.push(...effectiveSegmentSatisfiedBy);
|
||||
if (!evaluation.allowlistSatisfied && !allowSkillPreludeAtIndex.has(index)) {
|
||||
return {
|
||||
analysisOk: true,
|
||||
allowlistSatisfied: false,
|
||||
|
|
|
|||
|
|
@ -293,6 +293,100 @@ describe("exec approvals shell analysis", () => {
|
|||
expect(result.allowlistSatisfied).toBe(testCase.expectedAllowlistSatisfied);
|
||||
});
|
||||
|
||||
it("allows the skill display prelude when a later skill wrapper is allowlisted", () => {
|
||||
if (process.platform === "win32") {
|
||||
return;
|
||||
}
|
||||
const skillRoot = makeTempDir();
|
||||
const skillDir = path.join(skillRoot, "skills", "gog");
|
||||
const skillPath = path.join(skillDir, "SKILL.md");
|
||||
const wrapperPath = path.join(skillRoot, "bin", "gog-wrapper");
|
||||
fs.mkdirSync(path.dirname(skillPath), { recursive: true });
|
||||
fs.mkdirSync(path.dirname(wrapperPath), { recursive: true });
|
||||
fs.writeFileSync(skillPath, "# gog\n");
|
||||
fs.writeFileSync(wrapperPath, "#!/bin/sh\n", { mode: 0o755 });
|
||||
|
||||
const result = evaluateShellAllowlist({
|
||||
command: `cat ${skillPath} && printf '\\n---CMD---\\n' && ${wrapperPath} calendar events primary --today --json`,
|
||||
allowlist: [{ pattern: wrapperPath }],
|
||||
safeBins: new Set(),
|
||||
cwd: skillRoot,
|
||||
});
|
||||
|
||||
expect(result.analysisOk).toBe(true);
|
||||
expect(result.allowlistSatisfied).toBe(true);
|
||||
expect(result.segmentSatisfiedBy).toEqual(["skillPrelude", "skillPrelude", "allowlist"]);
|
||||
});
|
||||
|
||||
it("does not treat arbitrary allowlisted binaries as trusted skill wrappers", () => {
|
||||
if (process.platform === "win32") {
|
||||
return;
|
||||
}
|
||||
const skillRoot = makeTempDir();
|
||||
const skillDir = path.join(skillRoot, "skills", "gog");
|
||||
const skillPath = path.join(skillDir, "SKILL.md");
|
||||
fs.mkdirSync(skillDir, { recursive: true });
|
||||
fs.writeFileSync(skillPath, "# gog\n");
|
||||
|
||||
const result = evaluateShellAllowlist({
|
||||
command: `cat ${skillPath} && printf '\\n---CMD---\\n' && /bin/echo calendar events primary --today --json`,
|
||||
allowlist: [{ pattern: "/bin/echo" }],
|
||||
safeBins: new Set(),
|
||||
cwd: skillRoot,
|
||||
});
|
||||
|
||||
expect(result.analysisOk).toBe(true);
|
||||
expect(result.allowlistSatisfied).toBe(false);
|
||||
expect(result.segmentSatisfiedBy).toEqual([null]);
|
||||
});
|
||||
|
||||
it("still rejects the skill display prelude when no trusted skill command follows", () => {
|
||||
if (process.platform === "win32") {
|
||||
return;
|
||||
}
|
||||
const skillRoot = makeTempDir();
|
||||
const skillDir = path.join(skillRoot, "skills", "gog");
|
||||
const skillPath = path.join(skillDir, "SKILL.md");
|
||||
fs.mkdirSync(skillDir, { recursive: true });
|
||||
fs.writeFileSync(skillPath, "# gog\n");
|
||||
|
||||
const result = evaluateShellAllowlist({
|
||||
command: `cat ${skillPath} && printf '\\n---CMD---\\n'`,
|
||||
allowlist: [],
|
||||
safeBins: new Set(),
|
||||
cwd: skillRoot,
|
||||
});
|
||||
|
||||
expect(result.analysisOk).toBe(true);
|
||||
expect(result.allowlistSatisfied).toBe(false);
|
||||
expect(result.segmentSatisfiedBy).toEqual([null]);
|
||||
});
|
||||
|
||||
it("rejects the skill display prelude when a trusted wrapper is not reachable", () => {
|
||||
if (process.platform === "win32") {
|
||||
return;
|
||||
}
|
||||
const skillRoot = makeTempDir();
|
||||
const skillDir = path.join(skillRoot, "skills", "gog");
|
||||
const skillPath = path.join(skillDir, "SKILL.md");
|
||||
const wrapperPath = path.join(skillRoot, "bin", "gog-wrapper");
|
||||
fs.mkdirSync(path.dirname(skillPath), { recursive: true });
|
||||
fs.mkdirSync(path.dirname(wrapperPath), { recursive: true });
|
||||
fs.writeFileSync(skillPath, "# gog\n");
|
||||
fs.writeFileSync(wrapperPath, "#!/bin/sh\n", { mode: 0o755 });
|
||||
|
||||
const result = evaluateShellAllowlist({
|
||||
command: `cat ${skillPath} && printf '\\n---CMD---\\n' && false && ${wrapperPath} calendar events primary --today --json`,
|
||||
allowlist: [{ pattern: wrapperPath }],
|
||||
safeBins: new Set(),
|
||||
cwd: skillRoot,
|
||||
});
|
||||
|
||||
expect(result.analysisOk).toBe(true);
|
||||
expect(result.allowlistSatisfied).toBe(false);
|
||||
expect(result.segmentSatisfiedBy).toEqual([null]);
|
||||
});
|
||||
|
||||
it.each(['/usr/bin/echo "foo && bar"', '/usr/bin/echo "foo\\" && bar"'])(
|
||||
"respects quoted chain separator for %s",
|
||||
(command) => {
|
||||
|
|
|
|||
|
|
@ -697,7 +697,7 @@ function renderSafeBinSegmentArgv(segment: ExecCommandSegment): string | null {
|
|||
export function buildSafeBinsShellCommand(params: {
|
||||
command: string;
|
||||
segments: ExecCommandSegment[];
|
||||
segmentSatisfiedBy: ("allowlist" | "safeBins" | "skills" | null)[];
|
||||
segmentSatisfiedBy: ("allowlist" | "safeBins" | "skills" | "skillPrelude" | null)[];
|
||||
platform?: string | null;
|
||||
}): { ok: boolean; command?: string; reason?: string } {
|
||||
if (params.segments.length !== params.segmentSatisfiedBy.length) {
|
||||
|
|
|
|||
Loading…
Reference in New Issue