mirror of
https://github.com/openclaw/openclaw.git
synced 2026-05-09 06:37:40 +00:00
Agents/Tools: preflight exec script files for shell var injection (#18457)
* fix(agents): don't force store=true for codex responses * test: stabilize respawn + subagent usage assertions * Agents/Tools: preflight exec to detect shell variable injection in scripts * Changelog: fix merge marker formatting
This commit is contained in:
@@ -14,6 +14,7 @@ Docs: https://docs.openclaw.ai
|
|||||||
|
|
||||||
### Fixes
|
### Fixes
|
||||||
|
|
||||||
|
- Agents/Tools/exec: add a preflight guard that detects likely shell env var injection (e.g. `$DM_JSON`, `$TMPDIR`) in Python/Node scripts before execution, preventing recurring cron failures and wasted tokens when models emit mixed shell+language source. (#12836)
|
||||||
- Security/Sessions: create new session transcript JSONL files with user-only (`0o600`) permissions and extend `openclaw security audit --fix` to remediate existing transcript file permissions.
|
- Security/Sessions: create new session transcript JSONL files with user-only (`0o600`) permissions and extend `openclaw security audit --fix` to remediate existing transcript file permissions.
|
||||||
- Infra/Fetch: ensure foreign abort-signal listener cleanup never masks original fetch successes/failures, while still preventing detached-finally unhandled rejection noise in `wrapFetchWithAbortSignal`. Thanks @Jackten.
|
- Infra/Fetch: ensure foreign abort-signal listener cleanup never masks original fetch successes/failures, while still preventing detached-finally unhandled rejection noise in `wrapFetchWithAbortSignal`. Thanks @Jackten.
|
||||||
- Gateway/Config: prevent `config.patch` object-array merges from falling back to full-array replacement when some patch entries lack `id`, so partial `agents.list` updates no longer drop unrelated agents. (#17989) Thanks @stakeswky.
|
- Gateway/Config: prevent `config.patch` object-array merges from falling back to full-array replacement when some patch entries lack `id`, so partial `agents.list` updates no longer drop unrelated agents. (#17989) Thanks @stakeswky.
|
||||||
|
|||||||
65
src/agents/bash-tools.exec.script-preflight.test.ts
Normal file
65
src/agents/bash-tools.exec.script-preflight.test.ts
Normal file
@@ -0,0 +1,65 @@
|
|||||||
|
import fs from "node:fs/promises";
|
||||||
|
import os from "node:os";
|
||||||
|
import path from "node:path";
|
||||||
|
import { describe, expect, it } from "vitest";
|
||||||
|
|
||||||
|
const isWin = process.platform === "win32";
|
||||||
|
|
||||||
|
describe("exec script preflight", () => {
|
||||||
|
it("blocks shell env var injection tokens in python scripts before execution", async () => {
|
||||||
|
if (isWin) {
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
|
const tmp = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-exec-preflight-"));
|
||||||
|
const pyPath = path.join(tmp, "bad.py");
|
||||||
|
|
||||||
|
await fs.writeFile(
|
||||||
|
pyPath,
|
||||||
|
[
|
||||||
|
"import json",
|
||||||
|
"# model accidentally wrote shell syntax:",
|
||||||
|
"payload = $DM_JSON",
|
||||||
|
"print(payload)",
|
||||||
|
].join("\n"),
|
||||||
|
"utf-8",
|
||||||
|
);
|
||||||
|
|
||||||
|
const { createExecTool } = await import("./bash-tools.exec.js");
|
||||||
|
const tool = createExecTool({ host: "gateway", security: "full", ask: "off" });
|
||||||
|
|
||||||
|
await expect(
|
||||||
|
tool.execute("call1", {
|
||||||
|
command: "python bad.py",
|
||||||
|
workdir: tmp,
|
||||||
|
}),
|
||||||
|
).rejects.toThrow(/exec preflight: detected likely shell variable injection \(\$DM_JSON\)/);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("blocks obvious shell-as-js output before node execution", async () => {
|
||||||
|
if (isWin) {
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
|
const tmp = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-exec-preflight-"));
|
||||||
|
const jsPath = path.join(tmp, "bad.js");
|
||||||
|
|
||||||
|
await fs.writeFile(
|
||||||
|
jsPath,
|
||||||
|
['NODE "$TMPDIR/hot.json"', "console.log('hi')"].join("\n"),
|
||||||
|
"utf-8",
|
||||||
|
);
|
||||||
|
|
||||||
|
const { createExecTool } = await import("./bash-tools.exec.js");
|
||||||
|
const tool = createExecTool({ host: "gateway", security: "full", ask: "off" });
|
||||||
|
|
||||||
|
await expect(
|
||||||
|
tool.execute("call1", {
|
||||||
|
command: "node bad.js",
|
||||||
|
workdir: tmp,
|
||||||
|
}),
|
||||||
|
).rejects.toThrow(
|
||||||
|
/exec preflight: (detected likely shell variable injection|JS file starts with shell syntax)/,
|
||||||
|
);
|
||||||
|
});
|
||||||
|
});
|
||||||
@@ -1,5 +1,7 @@
|
|||||||
import type { AgentTool, AgentToolResult } from "@mariozechner/pi-agent-core";
|
import type { AgentTool, AgentToolResult } from "@mariozechner/pi-agent-core";
|
||||||
import crypto from "node:crypto";
|
import crypto from "node:crypto";
|
||||||
|
import fs from "node:fs/promises";
|
||||||
|
import path from "node:path";
|
||||||
import type { BashSandboxConfig } from "./bash-tools.shared.js";
|
import type { BashSandboxConfig } from "./bash-tools.shared.js";
|
||||||
import {
|
import {
|
||||||
type ExecAsk,
|
type ExecAsk,
|
||||||
@@ -118,6 +120,97 @@ export type ExecToolDetails =
|
|||||||
nodeId?: string;
|
nodeId?: string;
|
||||||
};
|
};
|
||||||
|
|
||||||
|
function extractScriptTargetFromCommand(
|
||||||
|
command: string,
|
||||||
|
): { kind: "python"; relOrAbsPath: string } | { kind: "node"; relOrAbsPath: string } | null {
|
||||||
|
const raw = command.trim();
|
||||||
|
if (!raw) {
|
||||||
|
return null;
|
||||||
|
}
|
||||||
|
|
||||||
|
// Intentionally simple parsing: we only support common forms like
|
||||||
|
// python file.py
|
||||||
|
// python3 -u file.py
|
||||||
|
// node --experimental-something file.js
|
||||||
|
// If the command is more complex (pipes, heredocs, quoted paths with spaces), skip preflight.
|
||||||
|
const pythonMatch = raw.match(/^\s*(python3?|python)\s+(?:-[^\s]+\s+)*([^\s]+\.py)\b/i);
|
||||||
|
if (pythonMatch?.[2]) {
|
||||||
|
return { kind: "python", relOrAbsPath: pythonMatch[2] };
|
||||||
|
}
|
||||||
|
const nodeMatch = raw.match(/^\s*(node)\s+(?:--[^\s]+\s+)*([^\s]+\.js)\b/i);
|
||||||
|
if (nodeMatch?.[2]) {
|
||||||
|
return { kind: "node", relOrAbsPath: nodeMatch[2] };
|
||||||
|
}
|
||||||
|
|
||||||
|
return null;
|
||||||
|
}
|
||||||
|
|
||||||
|
async function validateScriptFileForShellBleed(params: {
|
||||||
|
command: string;
|
||||||
|
workdir: string;
|
||||||
|
}): Promise<void> {
|
||||||
|
const target = extractScriptTargetFromCommand(params.command);
|
||||||
|
if (!target) {
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
|
const absPath = path.isAbsolute(target.relOrAbsPath)
|
||||||
|
? path.resolve(target.relOrAbsPath)
|
||||||
|
: path.resolve(params.workdir, target.relOrAbsPath);
|
||||||
|
|
||||||
|
// Best-effort: only validate if file exists and is reasonably small.
|
||||||
|
let stat: { isFile(): boolean; size: number };
|
||||||
|
try {
|
||||||
|
stat = await fs.stat(absPath);
|
||||||
|
} catch {
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
if (!stat.isFile()) {
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
if (stat.size > 512 * 1024) {
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
|
const content = await fs.readFile(absPath, "utf-8");
|
||||||
|
|
||||||
|
// Common failure mode: shell env var syntax leaking into Python/JS.
|
||||||
|
// We deliberately match all-caps/underscore vars to avoid false positives with `$` as a JS identifier.
|
||||||
|
const envVarRegex = /\$[A-Z_][A-Z0-9_]{1,}/g;
|
||||||
|
const first = envVarRegex.exec(content);
|
||||||
|
if (first) {
|
||||||
|
const idx = first.index;
|
||||||
|
const before = content.slice(0, idx);
|
||||||
|
const line = before.split("\n").length;
|
||||||
|
const token = first[0];
|
||||||
|
throw new Error(
|
||||||
|
[
|
||||||
|
`exec preflight: detected likely shell variable injection (${token}) in ${target.kind} script: ${path.basename(
|
||||||
|
absPath,
|
||||||
|
)}:${line}.`,
|
||||||
|
target.kind === "python"
|
||||||
|
? `In Python, use os.environ.get(${JSON.stringify(token.slice(1))}) instead of raw ${token}.`
|
||||||
|
: `In Node.js, use process.env[${JSON.stringify(token.slice(1))}] instead of raw ${token}.`,
|
||||||
|
"(If this is inside a string literal on purpose, escape it or restructure the code.)",
|
||||||
|
].join("\n"),
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
// Another recurring pattern from the issue: shell commands accidentally emitted as JS.
|
||||||
|
if (target.kind === "node") {
|
||||||
|
const firstNonEmpty = content
|
||||||
|
.split(/\r?\n/)
|
||||||
|
.map((l) => l.trim())
|
||||||
|
.find((l) => l.length > 0);
|
||||||
|
if (firstNonEmpty && /^NODE\b/.test(firstNonEmpty)) {
|
||||||
|
throw new Error(
|
||||||
|
`exec preflight: JS file starts with shell syntax (${firstNonEmpty}). ` +
|
||||||
|
`This looks like a shell command, not JavaScript.`,
|
||||||
|
);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
export function createExecTool(
|
export function createExecTool(
|
||||||
defaults?: ExecToolDefaults,
|
defaults?: ExecToolDefaults,
|
||||||
// oxlint-disable-next-line typescript/no-explicit-any
|
// oxlint-disable-next-line typescript/no-explicit-any
|
||||||
@@ -874,6 +967,11 @@ export function createExecTool(
|
|||||||
typeof params.timeout === "number" ? params.timeout : defaultTimeoutSec;
|
typeof params.timeout === "number" ? params.timeout : defaultTimeoutSec;
|
||||||
const getWarningText = () => (warnings.length ? `${warnings.join("\n")}\n\n` : "");
|
const getWarningText = () => (warnings.length ? `${warnings.join("\n")}\n\n` : "");
|
||||||
const usePty = params.pty === true && !sandbox;
|
const usePty = params.pty === true && !sandbox;
|
||||||
|
|
||||||
|
// Preflight: catch a common model failure mode (shell syntax leaking into Python/JS sources)
|
||||||
|
// before we execute and burn tokens in cron loops.
|
||||||
|
await validateScriptFileForShellBleed({ command: params.command, workdir });
|
||||||
|
|
||||||
const run = await runExecProcess({
|
const run = await runExecProcess({
|
||||||
command: params.command,
|
command: params.command,
|
||||||
execCommand: execCommandOverride,
|
execCommand: execCommandOverride,
|
||||||
|
|||||||
@@ -154,4 +154,27 @@ describe("applyExtraParamsToAgent", () => {
|
|||||||
});
|
});
|
||||||
expect(payload.store).toBe(false);
|
expect(payload.store).toBe(false);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("does not force store=true for Codex responses (Codex requires store=false)", () => {
|
||||||
|
const payload = { store: false };
|
||||||
|
const baseStreamFn: StreamFn = (_model, _context, options) => {
|
||||||
|
options?.onPayload?.(payload);
|
||||||
|
return new AssistantMessageEventStream();
|
||||||
|
};
|
||||||
|
const agent = { streamFn: baseStreamFn };
|
||||||
|
|
||||||
|
applyExtraParamsToAgent(agent, undefined, "openai-codex", "codex-mini-latest");
|
||||||
|
|
||||||
|
const model = {
|
||||||
|
api: "openai-codex-responses",
|
||||||
|
provider: "openai-codex",
|
||||||
|
id: "codex-mini-latest",
|
||||||
|
baseUrl: "https://chatgpt.com/backend-api/codex/responses",
|
||||||
|
} as Model<"openai-codex-responses">;
|
||||||
|
const context: Context = { messages: [] };
|
||||||
|
|
||||||
|
void agent.streamFn?.(model, context, {});
|
||||||
|
|
||||||
|
expect(payload.store).toBe(false);
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
Reference in New Issue
Block a user