mirror of
https://github.com/moltbot/moltbot.git
synced 2026-04-25 23:47:20 +00:00
fix(node-host): extend script-runner set and add fail-closed guard for mutable-file approval
tsx, jiti, ts-node, ts-node-esm, vite-node, and esno were not recognized as interpreter-style script runners in invoke-system-run-plan.ts. These runners produced mutableFileOperand: null, causing invoke-system-run.ts to skip revalidation entirely. A mutated script payload would execute without the approval binding check that node ./run.js already enforced. Two-part fix: - Add tsx, jiti, and related TypeScript/ESM loaders to the known script runner set so they produce a valid mutableFileOperand from the planner - Add a fail-closed runtime guard in invoke-system-run.ts that denies execution when a script run should have a mutable-file binding but the approval plan is missing it, preventing unknown future runners from silently bypassing revalidation Fixes GHSA-qc36-x95h-7j53
This commit is contained in:
@@ -246,6 +246,38 @@ describe("hardenApprovedExecutionPaths", () => {
|
||||
initialBody: 'console.log("SAFE");\n',
|
||||
expectedArgvIndex: 1,
|
||||
},
|
||||
{
|
||||
name: "tsx direct file",
|
||||
binName: "tsx",
|
||||
argv: ["tsx", "./run.ts"],
|
||||
scriptName: "run.ts",
|
||||
initialBody: 'console.log("SAFE");\n',
|
||||
expectedArgvIndex: 1,
|
||||
},
|
||||
{
|
||||
name: "jiti direct file",
|
||||
binName: "jiti",
|
||||
argv: ["jiti", "./run.ts"],
|
||||
scriptName: "run.ts",
|
||||
initialBody: 'console.log("SAFE");\n',
|
||||
expectedArgvIndex: 1,
|
||||
},
|
||||
{
|
||||
name: "ts-node direct file",
|
||||
binName: "ts-node",
|
||||
argv: ["ts-node", "./run.ts"],
|
||||
scriptName: "run.ts",
|
||||
initialBody: 'console.log("SAFE");\n',
|
||||
expectedArgvIndex: 1,
|
||||
},
|
||||
{
|
||||
name: "vite-node direct file",
|
||||
binName: "vite-node",
|
||||
argv: ["vite-node", "./run.ts"],
|
||||
scriptName: "run.ts",
|
||||
initialBody: 'console.log("SAFE");\n',
|
||||
expectedArgvIndex: 1,
|
||||
},
|
||||
{
|
||||
name: "bun direct file",
|
||||
binName: "bun",
|
||||
@@ -387,4 +419,26 @@ describe("hardenApprovedExecutionPaths", () => {
|
||||
},
|
||||
});
|
||||
});
|
||||
|
||||
it("rejects tsx eval invocations that do not bind a concrete file", () => {
|
||||
withFakeRuntimeBin({
|
||||
binName: "tsx",
|
||||
run: () => {
|
||||
const tmp = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-tsx-eval-"));
|
||||
try {
|
||||
const prepared = buildSystemRunApprovalPlan({
|
||||
command: ["tsx", "--eval", "console.log('SAFE')"],
|
||||
cwd: tmp,
|
||||
});
|
||||
expect(prepared).toEqual({
|
||||
ok: false,
|
||||
message:
|
||||
"SYSTEM_RUN_DENIED: approval cannot safely bind this interpreter/runtime command",
|
||||
});
|
||||
} finally {
|
||||
fs.rmSync(tmp, { recursive: true, force: true });
|
||||
}
|
||||
},
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -33,6 +33,15 @@ const MUTABLE_ARGV1_INTERPRETER_PATTERNS = [
|
||||
/^ruby$/,
|
||||
] as const;
|
||||
|
||||
const GENERIC_MUTABLE_SCRIPT_RUNNERS = new Set([
|
||||
"esno",
|
||||
"jiti",
|
||||
"ts-node",
|
||||
"ts-node-esm",
|
||||
"tsx",
|
||||
"vite-node",
|
||||
]);
|
||||
|
||||
const BUN_SUBCOMMANDS = new Set([
|
||||
"add",
|
||||
"audit",
|
||||
@@ -409,6 +418,10 @@ function resolveDenoRunScriptOperandIndex(params: {
|
||||
});
|
||||
}
|
||||
|
||||
function isMutableScriptRunner(executable: string): boolean {
|
||||
return GENERIC_MUTABLE_SCRIPT_RUNNERS.has(executable) || isInterpreterLikeSafeBin(executable);
|
||||
}
|
||||
|
||||
function resolveMutableFileOperandIndex(argv: string[], cwd: string | undefined): number | null {
|
||||
const unwrapped = unwrapArgvForMutableOperand(argv);
|
||||
const executable = normalizeExecutableToken(unwrapped.argv[0] ?? "");
|
||||
@@ -443,7 +456,7 @@ function resolveMutableFileOperandIndex(argv: string[], cwd: string | undefined)
|
||||
return unwrapped.baseIndex + denoIndex;
|
||||
}
|
||||
}
|
||||
if (!isInterpreterLikeSafeBin(executable)) {
|
||||
if (!isMutableScriptRunner(executable)) {
|
||||
return null;
|
||||
}
|
||||
const genericIndex = resolveGenericInterpreterScriptOperandIndex({
|
||||
@@ -468,10 +481,10 @@ function requiresStableInterpreterApprovalBindingWithShellCommand(params: {
|
||||
if ((POSIX_SHELL_WRAPPERS as ReadonlySet<string>).has(executable)) {
|
||||
return false;
|
||||
}
|
||||
return isInterpreterLikeSafeBin(executable);
|
||||
return isMutableScriptRunner(executable);
|
||||
}
|
||||
|
||||
function resolveMutableFileOperandSnapshotSync(params: {
|
||||
export function resolveMutableFileOperandSnapshotSync(params: {
|
||||
argv: string[];
|
||||
cwd: string | undefined;
|
||||
shellCommand: string | null;
|
||||
|
||||
@@ -109,27 +109,50 @@ describe("handleSystemRunInvoke mac app exec host routing", () => {
|
||||
};
|
||||
}
|
||||
|
||||
function createRuntimeScriptOperandFixture(params: { tmp: string; runtime: "bun" | "deno" }): {
|
||||
function createRuntimeScriptOperandFixture(params: {
|
||||
tmp: string;
|
||||
runtime: "bun" | "deno" | "jiti" | "tsx";
|
||||
}): {
|
||||
command: string[];
|
||||
scriptPath: string;
|
||||
initialBody: string;
|
||||
changedBody: string;
|
||||
} {
|
||||
const scriptPath = path.join(params.tmp, "run.ts");
|
||||
if (params.runtime === "bun") {
|
||||
return {
|
||||
command: ["bun", "run", "./run.ts"],
|
||||
scriptPath,
|
||||
initialBody: 'console.log("SAFE");\n',
|
||||
changedBody: 'console.log("PWNED");\n',
|
||||
};
|
||||
const initialBody = 'console.log("SAFE");\n';
|
||||
const changedBody = 'console.log("PWNED");\n';
|
||||
switch (params.runtime) {
|
||||
case "bun":
|
||||
return {
|
||||
command: ["bun", "run", "./run.ts"],
|
||||
scriptPath,
|
||||
initialBody,
|
||||
changedBody,
|
||||
};
|
||||
case "deno":
|
||||
return {
|
||||
command: ["deno", "run", "-A", "--allow-read", "--", "./run.ts"],
|
||||
scriptPath,
|
||||
initialBody,
|
||||
changedBody,
|
||||
};
|
||||
case "jiti":
|
||||
return {
|
||||
command: ["jiti", "./run.ts"],
|
||||
scriptPath,
|
||||
initialBody,
|
||||
changedBody,
|
||||
};
|
||||
case "tsx":
|
||||
return {
|
||||
command: ["tsx", "./run.ts"],
|
||||
scriptPath,
|
||||
initialBody,
|
||||
changedBody,
|
||||
};
|
||||
}
|
||||
return {
|
||||
command: ["deno", "run", "-A", "--allow-read", "--", "./run.ts"],
|
||||
scriptPath,
|
||||
initialBody: 'console.log("SAFE");\n',
|
||||
changedBody: 'console.log("PWNED");\n',
|
||||
};
|
||||
const unsupportedRuntime: never = params.runtime;
|
||||
throw new Error(`unsupported runtime fixture: ${String(unsupportedRuntime)}`);
|
||||
}
|
||||
|
||||
function buildNestedEnvShellCommand(params: { depth: number; payload: string }): string[] {
|
||||
@@ -223,7 +246,7 @@ describe("handleSystemRunInvoke mac app exec host routing", () => {
|
||||
}
|
||||
|
||||
async function withFakeRuntimeOnPath<T>(params: {
|
||||
runtime: "bun" | "deno";
|
||||
runtime: "bun" | "deno" | "jiti" | "tsx";
|
||||
run: () => Promise<T>;
|
||||
}): Promise<T> {
|
||||
const tmp = fs.mkdtempSync(path.join(os.tmpdir(), `openclaw-${params.runtime}-path-`));
|
||||
@@ -842,7 +865,7 @@ describe("handleSystemRunInvoke mac app exec host routing", () => {
|
||||
}
|
||||
});
|
||||
|
||||
for (const runtime of ["bun", "deno"] as const) {
|
||||
for (const runtime of ["bun", "deno", "tsx", "jiti"] as const) {
|
||||
it(`denies approval-based execution when a ${runtime} script operand changes after approval`, async () => {
|
||||
await withFakeRuntimeOnPath({
|
||||
runtime,
|
||||
@@ -926,6 +949,50 @@ describe("handleSystemRunInvoke mac app exec host routing", () => {
|
||||
});
|
||||
}
|
||||
|
||||
it("denies approval-based execution when tsx is missing a required mutable script binding", async () => {
|
||||
await withFakeRuntimeOnPath({
|
||||
runtime: "tsx",
|
||||
run: async () => {
|
||||
const tmp = fs.mkdtempSync(
|
||||
path.join(os.tmpdir(), "openclaw-approval-tsx-missing-binding-"),
|
||||
);
|
||||
const fixture = createRuntimeScriptOperandFixture({ tmp, runtime: "tsx" });
|
||||
fs.writeFileSync(fixture.scriptPath, fixture.initialBody);
|
||||
try {
|
||||
const prepared = buildSystemRunApprovalPlan({
|
||||
command: fixture.command,
|
||||
cwd: tmp,
|
||||
});
|
||||
expect(prepared.ok).toBe(true);
|
||||
if (!prepared.ok) {
|
||||
throw new Error("unreachable");
|
||||
}
|
||||
|
||||
const planWithoutBinding = { ...prepared.plan };
|
||||
delete planWithoutBinding.mutableFileOperand;
|
||||
const { runCommand, sendInvokeResult } = await runSystemInvoke({
|
||||
preferMacAppExecHost: false,
|
||||
command: prepared.plan.argv,
|
||||
rawCommand: prepared.plan.commandText,
|
||||
systemRunPlan: planWithoutBinding,
|
||||
cwd: prepared.plan.cwd ?? tmp,
|
||||
approved: true,
|
||||
security: "full",
|
||||
ask: "off",
|
||||
});
|
||||
|
||||
expect(runCommand).not.toHaveBeenCalled();
|
||||
expectInvokeErrorMessage(sendInvokeResult, {
|
||||
message: "SYSTEM_RUN_DENIED: approval missing script operand binding",
|
||||
exact: true,
|
||||
});
|
||||
} finally {
|
||||
fs.rmSync(tmp, { recursive: true, force: true });
|
||||
}
|
||||
},
|
||||
});
|
||||
});
|
||||
|
||||
it("denies ./sh wrapper spoof in allowlist on-miss mode before execution", async () => {
|
||||
const marker = path.join(os.tmpdir(), `openclaw-wrapper-spoof-${process.pid}-${Date.now()}`);
|
||||
const runCommand = vi.fn(async () => {
|
||||
|
||||
@@ -29,6 +29,7 @@ import {
|
||||
hardenApprovedExecutionPaths,
|
||||
revalidateApprovedCwdSnapshot,
|
||||
revalidateApprovedMutableFileOperand,
|
||||
resolveMutableFileOperandSnapshotSync,
|
||||
type ApprovedCwdSnapshot,
|
||||
} from "./invoke-system-run-plan.js";
|
||||
import type {
|
||||
@@ -98,6 +99,8 @@ type SystemRunPolicyPhase = SystemRunParsePhase & {
|
||||
const safeBinTrustedDirWarningCache = new Set<string>();
|
||||
const APPROVAL_CWD_DRIFT_DENIED_MESSAGE =
|
||||
"SYSTEM_RUN_DENIED: approval cwd changed before execution";
|
||||
const APPROVAL_SCRIPT_OPERAND_BINDING_DENIED_MESSAGE =
|
||||
"SYSTEM_RUN_DENIED: approval missing script operand binding";
|
||||
const APPROVAL_SCRIPT_OPERAND_DRIFT_DENIED_MESSAGE =
|
||||
"SYSTEM_RUN_DENIED: approval script operand changed before execution";
|
||||
|
||||
@@ -385,6 +388,29 @@ async function executeSystemRunPhase(
|
||||
});
|
||||
return;
|
||||
}
|
||||
const expectedMutableFileOperand = phase.approvalPlan
|
||||
? resolveMutableFileOperandSnapshotSync({
|
||||
argv: phase.argv,
|
||||
cwd: phase.cwd,
|
||||
shellCommand: phase.shellPayload,
|
||||
})
|
||||
: null;
|
||||
if (expectedMutableFileOperand && !expectedMutableFileOperand.ok) {
|
||||
logWarn(`security: system.run approval script binding blocked (runId=${phase.runId})`);
|
||||
await sendSystemRunDenied(opts, phase.execution, {
|
||||
reason: "approval-required",
|
||||
message: expectedMutableFileOperand.message,
|
||||
});
|
||||
return;
|
||||
}
|
||||
if (expectedMutableFileOperand?.snapshot && !phase.approvalPlan?.mutableFileOperand) {
|
||||
logWarn(`security: system.run approval script binding missing (runId=${phase.runId})`);
|
||||
await sendSystemRunDenied(opts, phase.execution, {
|
||||
reason: "approval-required",
|
||||
message: APPROVAL_SCRIPT_OPERAND_BINDING_DENIED_MESSAGE,
|
||||
});
|
||||
return;
|
||||
}
|
||||
if (
|
||||
phase.approvalPlan?.mutableFileOperand &&
|
||||
!revalidateApprovedMutableFileOperand({
|
||||
|
||||
Reference in New Issue
Block a user