mirror of
https://github.com/openclaw/openclaw.git
synced 2026-08-02 04:21:34 +00:00
fix(agents): harden code mode module isolation (#114742)
This commit is contained in:
committed by
GitHub
parent
2f7f543d9a
commit
3a39dbcdfd
@@ -54,7 +54,7 @@ export function validateMcpCodeModeResult(
|
||||
assert(mentions.apiFileList > 0, "session log lacks API.list usage");
|
||||
assert(mentions.apiFileRead > 0, "session log lacks API.read usage");
|
||||
assert(mentions.mcpNamespace > 0, "session log lacks MCP.fixture usage");
|
||||
assert(mentions.mcpTool > 0, "session log lacks fixture__lookup_note call");
|
||||
assert(mentions.mcpTool > 0, "session log lacks MCP.fixture.lookupNote call");
|
||||
assert(mentions.apiCall === 0, "agent should not call MCP.$api when API files are available");
|
||||
assert(mentions.toolSearchPollution === 0, "agent should not use tools.search for MCP lookup");
|
||||
return finalText;
|
||||
|
||||
@@ -106,7 +106,7 @@ async function readSessionLogMentions(stateDir: string): Promise<Record<string,
|
||||
apiFileList: "API.list",
|
||||
apiFileRead: "API.read",
|
||||
mcpNamespace: "MCP.fixture",
|
||||
mcpTool: "fixture__lookup_note",
|
||||
mcpTool: "MCP.fixture.lookupNote",
|
||||
toolSearchPollution: 'tools.search("lookup note"',
|
||||
},
|
||||
});
|
||||
|
||||
@@ -40,11 +40,6 @@ async function main() {
|
||||
memory: {
|
||||
search: {
|
||||
enabled: false,
|
||||
sync: {
|
||||
onSearch: false,
|
||||
onSessionStart: false,
|
||||
watch: false,
|
||||
},
|
||||
},
|
||||
},
|
||||
plugins: {
|
||||
|
||||
@@ -126,6 +126,21 @@ describe("headless Code Mode", () => {
|
||||
code: 'return /import.meta/.test("import.meta");',
|
||||
value: true,
|
||||
},
|
||||
{
|
||||
name: "ordinary import method",
|
||||
code: "const api = { import(value) { return value; } }; return api.import(42);",
|
||||
value: 42,
|
||||
},
|
||||
{
|
||||
name: "ordinary require method",
|
||||
code: "const api = { require(value) { return value; } }; return api.require(42);",
|
||||
value: 42,
|
||||
},
|
||||
{
|
||||
name: "ordinary import metadata property",
|
||||
code: "const api = { import: { meta: 42 } }; return api.import.meta;",
|
||||
value: 42,
|
||||
},
|
||||
])("executes harmless $name in a headless guest worker", async ({ code, value }) => {
|
||||
const result = expectCompleted(
|
||||
await runCodeModeScriptHeadless({
|
||||
@@ -152,6 +167,13 @@ describe("headless Code Mode", () => {
|
||||
});
|
||||
|
||||
it.each([
|
||||
String.raw`return r\u0065quire('node:fs');`,
|
||||
"return require?.('node:fs');",
|
||||
"return (require)('node:fs');",
|
||||
"return (0, require)('node:fs');",
|
||||
"const load = require; return load('node:fs');",
|
||||
"return module.require('node:fs');",
|
||||
"return process.getBuiltinModule('node:fs');",
|
||||
"return `${import('node:fs')}`;",
|
||||
"return `${require('node:fs')}`;",
|
||||
"return `${`nested ${import('node:fs')}`}`;",
|
||||
|
||||
@@ -109,6 +109,26 @@ describe("Code Mode guest source validation", () => {
|
||||
name: "regular expression after a keyword-shaped private member",
|
||||
code: "class Guest { #return = 10; run() { return this.#return / /import.meta/.source.length; } } return new Guest().run();",
|
||||
},
|
||||
{
|
||||
name: "ordinary import method",
|
||||
code: "const api = { import(value) { return value; } }; return api.import(42);",
|
||||
},
|
||||
{
|
||||
name: "ordinary require method",
|
||||
code: "const api = { require(value) { return value; } }; return api.require(42);",
|
||||
},
|
||||
{
|
||||
name: "optional ordinary import method",
|
||||
code: "const api = { import(value) { return value; } }; return api?.import?.(42);",
|
||||
},
|
||||
{
|
||||
name: "computed ordinary require method",
|
||||
code: 'const api = { require(value) { return value; } }; return api["require"](42);',
|
||||
},
|
||||
{
|
||||
name: "ordinary import metadata property",
|
||||
code: "const api = { import: { meta: 42 } }; return api.import.meta;",
|
||||
},
|
||||
{
|
||||
name: "ordinary malformed JavaScript for guest syntax diagnostics",
|
||||
code: "const answer = ;",
|
||||
@@ -134,6 +154,22 @@ describe("Code Mode guest source validation", () => {
|
||||
name: "comment-separated require",
|
||||
code: "return require /* hidden */ ('node:fs');",
|
||||
},
|
||||
{
|
||||
name: "Unicode-escaped direct require",
|
||||
code: String.raw`return r\u0065quire('node:fs');`,
|
||||
},
|
||||
{
|
||||
name: "optional direct require",
|
||||
code: "return require?.('node:fs');",
|
||||
},
|
||||
{
|
||||
name: "parenthesized direct require",
|
||||
code: "return (require)('node:fs');",
|
||||
},
|
||||
{
|
||||
name: "sequence-wrapped direct require",
|
||||
code: "return (0, require)('node:fs');",
|
||||
},
|
||||
{
|
||||
name: "comment-separated dynamic import",
|
||||
code: "return import /* hidden */ ('node:fs');",
|
||||
@@ -265,6 +301,14 @@ describe("Code Mode guest source validation", () => {
|
||||
name: "module-shaped comment",
|
||||
code: "const value: number = 1; /* import('node:fs') */ return value;",
|
||||
},
|
||||
{
|
||||
name: "ordinary typed import method",
|
||||
code: "const api: { import(value: number): number } = { import(value) { return value; } }; return api.import(42);",
|
||||
},
|
||||
{
|
||||
name: "ordinary typed require method",
|
||||
code: "const api: { require(value: number): number } = { require(value) { return value; } }; return api.require(42);",
|
||||
},
|
||||
])("preserves TypeScript $name", async ({ code }) => {
|
||||
await expect(prepareSource({ code, language: "typescript", config })).resolves.toEqual(
|
||||
expect.any(String),
|
||||
@@ -328,6 +372,31 @@ describe("Code Mode guest source validation", () => {
|
||||
}
|
||||
}, 30_000);
|
||||
|
||||
it("separates 20,000 ordinary methods from disguised module loaders", async () => {
|
||||
const harmlessMethods = [
|
||||
"api.import(value)",
|
||||
"api.require(value)",
|
||||
"api?.import?.(value)",
|
||||
'api["require"](value)',
|
||||
];
|
||||
const moduleExpressions = [
|
||||
String.raw`r\u0065quire('node:fs')`,
|
||||
"require?.('node:fs')",
|
||||
"(require)('node:fs')",
|
||||
"(0, require)('node:fs')",
|
||||
];
|
||||
|
||||
for (let index = 0; index < 10_000; index += 1) {
|
||||
const harmless = `const value = ${index}; const api = { import(value) { return value; }, require(value) { return value; } }; return ${harmlessMethods[index % harmlessMethods.length]};`;
|
||||
await expect(prepareSource({ code: harmless, config })).resolves.toBe(harmless);
|
||||
|
||||
const executable = `return ${moduleExpressions[index % moduleExpressions.length]};`;
|
||||
await expect(prepareSource({ code: executable, config })).rejects.toThrow(
|
||||
"code mode module access is disabled",
|
||||
);
|
||||
}
|
||||
}, 30_000);
|
||||
|
||||
it("rejects 10,000 Unicode-shifted TypeScript module-access attempts", async () => {
|
||||
for (let index = 0; index < 5_000; index += 1) {
|
||||
const padding = "😀".repeat((index % 96) + 1);
|
||||
|
||||
@@ -436,10 +436,116 @@ function maskCodeLiteralsAndComments(
|
||||
}
|
||||
}
|
||||
|
||||
function isModuleLoaderCallee(callee: import("acorn").Expression | import("acorn").Super): boolean {
|
||||
if (callee.type === "ParenthesizedExpression") {
|
||||
return isModuleLoaderCallee(callee.expression);
|
||||
}
|
||||
if (callee.type === "ChainExpression") {
|
||||
return isModuleLoaderCallee(callee.expression);
|
||||
}
|
||||
if (callee.type === "SequenceExpression") {
|
||||
const expression = callee.expressions[callee.expressions.length - 1];
|
||||
return expression !== undefined && isModuleLoaderCallee(expression);
|
||||
}
|
||||
return callee.type === "Identifier" && callee.name === "require";
|
||||
}
|
||||
|
||||
function containsModuleAccess(node: import("acorn").AnyNode): boolean {
|
||||
if (
|
||||
node.type === "ImportDeclaration" ||
|
||||
node.type === "ImportExpression" ||
|
||||
(node.type === "MetaProperty" && node.meta.name === "import") ||
|
||||
(node.type === "CallExpression" && isModuleLoaderCallee(node.callee))
|
||||
) {
|
||||
return true;
|
||||
}
|
||||
|
||||
for (const value of Object.values(node)) {
|
||||
if (Array.isArray(value)) {
|
||||
for (const child of value) {
|
||||
if (
|
||||
child !== null &&
|
||||
typeof child === "object" &&
|
||||
"type" in child &&
|
||||
typeof child.type === "string" &&
|
||||
containsModuleAccess(child as import("acorn").AnyNode)
|
||||
) {
|
||||
return true;
|
||||
}
|
||||
}
|
||||
continue;
|
||||
}
|
||||
if (
|
||||
value !== null &&
|
||||
typeof value === "object" &&
|
||||
"type" in value &&
|
||||
typeof value.type === "string" &&
|
||||
containsModuleAccess(value as import("acorn").AnyNode)
|
||||
) {
|
||||
return true;
|
||||
}
|
||||
}
|
||||
return false;
|
||||
}
|
||||
|
||||
function typeScriptContainsModuleAccess(code: string, ts: typeof import("typescript")): boolean {
|
||||
const source = ts.createSourceFile(
|
||||
"code-mode.ts",
|
||||
code,
|
||||
ts.ScriptTarget.ES2022,
|
||||
true,
|
||||
ts.ScriptKind.TS,
|
||||
);
|
||||
|
||||
const isLoaderCallee = (expression: import("typescript").Expression): boolean => {
|
||||
if (ts.isParenthesizedExpression(expression)) {
|
||||
return isLoaderCallee(expression.expression);
|
||||
}
|
||||
if (
|
||||
ts.isBinaryExpression(expression) &&
|
||||
expression.operatorToken.kind === ts.SyntaxKind.CommaToken
|
||||
) {
|
||||
return isLoaderCallee(expression.right);
|
||||
}
|
||||
return ts.isIdentifier(expression) && expression.text === "require";
|
||||
};
|
||||
|
||||
const visit = (node: import("typescript").Node): boolean => {
|
||||
if (
|
||||
ts.isImportDeclaration(node) ||
|
||||
ts.isImportEqualsDeclaration(node) ||
|
||||
(ts.isMetaProperty(node) && node.keywordToken === ts.SyntaxKind.ImportKeyword) ||
|
||||
(ts.isCallExpression(node) &&
|
||||
(node.expression.kind === ts.SyntaxKind.ImportKeyword || isLoaderCallee(node.expression)))
|
||||
) {
|
||||
return true;
|
||||
}
|
||||
return ts.forEachChild(node, (child) => (visit(child) ? true : undefined)) === true;
|
||||
};
|
||||
|
||||
return visit(source);
|
||||
}
|
||||
|
||||
function rejectsModuleAccess(
|
||||
code: string,
|
||||
typescriptRuntime?: typeof import("typescript"),
|
||||
): boolean {
|
||||
try {
|
||||
const source = parse(`(async () => {\n${code}\n})`, {
|
||||
ecmaVersion: "latest",
|
||||
});
|
||||
// The WASI guest has no host module loader. Only executable module syntax
|
||||
// belongs in this early check; ordinary guest methods are not capabilities.
|
||||
return containsModuleAccess(source);
|
||||
} catch {
|
||||
if (typescriptRuntime) {
|
||||
try {
|
||||
return typeScriptContainsModuleAccess(code, typescriptRuntime);
|
||||
} catch {
|
||||
// Keep malformed input on the conservative lexical fallback.
|
||||
}
|
||||
}
|
||||
}
|
||||
const source = maskCodeLiteralsAndComments(code, typescriptRuntime);
|
||||
return /\bimport\b\s*(?:\.|\(|["'`{*]|\w)|\brequire\b\s*\(/u.test(source);
|
||||
}
|
||||
|
||||
@@ -2009,6 +2009,31 @@ describe("Code Mode", () => {
|
||||
code: 'return `${/import.meta/.test("import.meta")}`;',
|
||||
value: "true",
|
||||
},
|
||||
{
|
||||
name: "ordinary import method",
|
||||
code: "const api = { import(value) { return value; } }; return api.import(42);",
|
||||
value: 42,
|
||||
},
|
||||
{
|
||||
name: "ordinary require method",
|
||||
code: "const api = { require(value) { return value; } }; return api.require(42);",
|
||||
value: 42,
|
||||
},
|
||||
{
|
||||
name: "optional ordinary import method",
|
||||
code: "const api = { import(value) { return value; } }; return api?.import?.(42);",
|
||||
value: 42,
|
||||
},
|
||||
{
|
||||
name: "computed ordinary require method",
|
||||
code: 'const api = { require(value) { return value; } }; return api["require"](42);',
|
||||
value: 42,
|
||||
},
|
||||
{
|
||||
name: "ordinary import metadata property",
|
||||
code: "const api = { import: { meta: 42 } }; return api.import.meta;",
|
||||
value: 42,
|
||||
},
|
||||
])("executes harmless $name in the real guest worker", async ({ code, value }) => {
|
||||
const { config, catalogRef, tools: codeModeTools } = createCodeModeHarness();
|
||||
applyCodeModeCatalog({
|
||||
@@ -2030,6 +2055,30 @@ describe("Code Mode", () => {
|
||||
expect(testing.activeRuns.size).toBe(0);
|
||||
});
|
||||
|
||||
it("never exposes Node module-loader globals to the real guest worker", async () => {
|
||||
const { config, catalogRef, tools: codeModeTools } = createCodeModeHarness();
|
||||
applyCodeModeCatalog({
|
||||
tools: [...codeModeTools, pluginTool("fake_noop", "Noop")],
|
||||
config,
|
||||
sessionId: "session-code-mode",
|
||||
sessionKey: "agent:main:main",
|
||||
runId: "run-code-mode",
|
||||
catalogRef,
|
||||
});
|
||||
|
||||
const details = await runUntilCompleted({
|
||||
execTool: expectDefined(codeModeTools[0], "codeModeTools[0] test invariant"),
|
||||
waitTool: expectDefined(codeModeTools[1], "codeModeTools[1] test invariant"),
|
||||
code: "return [typeof process, typeof module, typeof require];",
|
||||
});
|
||||
|
||||
expect(details).toMatchObject({
|
||||
status: "completed",
|
||||
value: ["undefined", "undefined", "undefined"],
|
||||
});
|
||||
expect(testing.activeRuns.size).toBe(0);
|
||||
});
|
||||
|
||||
it("isolates and cleans up 12 concurrent real guest workers", async () => {
|
||||
const { config, catalogRef, tools: codeModeTools } = createCodeModeHarness();
|
||||
applyCodeModeCatalog({
|
||||
@@ -2309,6 +2358,13 @@ describe("Code Mode", () => {
|
||||
|
||||
it.each([
|
||||
"const fs = require('node:fs'); return fs;",
|
||||
String.raw`return r\u0065quire('node:fs');`,
|
||||
"return require?.('node:fs');",
|
||||
"return (require)('node:fs');",
|
||||
"return (0, require)('node:fs');",
|
||||
"const load = require; return load('node:fs');",
|
||||
"return module.require('node:fs');",
|
||||
"return process.getBuiltinModule('node:fs');",
|
||||
"return import('node:fs');",
|
||||
"return import.meta.url;",
|
||||
"return `${import('node:fs')}`;",
|
||||
|
||||
@@ -496,6 +496,16 @@ async function readCompletedResult(vm: QuickJS, resultHandle: JSValueHandle): Pr
|
||||
// format it like the synchronous path so async rejections keep their cause
|
||||
// and location instead of collapsing to the bare message.
|
||||
const dumped = vm.dump(error);
|
||||
// Node module globals are deliberately absent from the WASI guest. Keep
|
||||
// aliases fail-closed at that runtime boundary rather than guessing source
|
||||
// provenance or installing a host-backed loader.
|
||||
if (
|
||||
dumped instanceof Error &&
|
||||
dumped.name === "ReferenceError" &&
|
||||
/^(?:require|module|process) is not defined$/u.test(dumped.message)
|
||||
) {
|
||||
throw new CodeModeWorkerFailure("invalid_input", "code mode module access is disabled.");
|
||||
}
|
||||
const text =
|
||||
dumped instanceof Error
|
||||
? formatQuickJsError(dumped.name, dumped.message, dumped.stack)
|
||||
|
||||
@@ -47,9 +47,8 @@ describe("Docker E2E seed scripts", () => {
|
||||
});
|
||||
|
||||
it("keeps MCP code-mode gateway config wired to its fixture server artifacts", () => {
|
||||
const source =
|
||||
readScript("scripts/e2e/mcp-code-mode-gateway-seed.ts") +
|
||||
readScript("scripts/e2e/lib/mcp-code-mode-probe-server.ts");
|
||||
const seed = readScript("scripts/e2e/mcp-code-mode-gateway-seed.ts");
|
||||
const source = seed + readScript("scripts/e2e/lib/mcp-code-mode-probe-server.ts");
|
||||
|
||||
expect(source).toContain('const serverPath = path.join(stateDir, "mcp-code-mode-fixture"');
|
||||
expect(source).toContain('["alpha", "fixture-note-alpha"]');
|
||||
@@ -60,5 +59,6 @@ describe("Docker E2E seed scripts", () => {
|
||||
expect(source).toContain("args: [serverPath]");
|
||||
expect(source).toContain("cwd: path.dirname(serverPath)");
|
||||
expect(source).toContain("connectionTimeoutMs: 30_000");
|
||||
expect(seed).not.toContain("sync:");
|
||||
});
|
||||
});
|
||||
|
||||
@@ -148,7 +148,7 @@ describe("MCP code-mode gateway Docker client result validation", () => {
|
||||
...okMentions,
|
||||
mcpTool: 0,
|
||||
}),
|
||||
).toThrow("session log lacks fixture__lookup_note call");
|
||||
).toThrow("session log lacks MCP.fixture.lookupNote call");
|
||||
});
|
||||
|
||||
it("rejects MCP.$api and tools.search fallback pollution", () => {
|
||||
|
||||
Reference in New Issue
Block a user