mirror of
https://github.com/moltbot/moltbot.git
synced 2026-03-08 06:54:24 +00:00
fix(security): harden hooks module loading
This commit is contained in:
@@ -11,7 +11,6 @@ Docs: https://docs.openclaw.ai
|
||||
- Security/Hooks: restrict hook transform modules to `~/.openclaw/hooks/transforms` (prevents path traversal/escape module loads via config). Config note: `hooks.transformsDir` must now be within that directory. Thanks @akhmittra.
|
||||
- Security/Hooks: ignore hook package manifest entries that point outside the package directory (prevents out-of-tree handler loads during hook discovery).
|
||||
- Ollama/Agents: avoid forcing `<final>` tag enforcement for Ollama models, which could suppress all output as `(no output)`. (#16191) Thanks @Glucksberg.
|
||||
|
||||
## 2026.2.13
|
||||
|
||||
### Changes
|
||||
|
||||
@@ -400,6 +400,8 @@ The old config format still works for backwards compatibility:
|
||||
}
|
||||
```
|
||||
|
||||
Note: `module` must be a workspace-relative path. Absolute paths and traversal outside the workspace are rejected.
|
||||
|
||||
**Migration**: Use the new discovery-based system for new hooks. Legacy handlers are loaded after directory-based hooks.
|
||||
|
||||
## CLI Commands
|
||||
|
||||
@@ -139,7 +139,9 @@ Mapping options (summary):
|
||||
|
||||
- `hooks.presets: ["gmail"]` enables the built-in Gmail mapping.
|
||||
- `hooks.mappings` lets you define `match`, `action`, and templates in config.
|
||||
- `hooks.transformsDir` + `transform.module` loads a JS/TS module for custom logic (restricted to `~/.openclaw/hooks/transforms`).
|
||||
- `hooks.transformsDir` + `transform.module` loads a JS/TS module for custom logic.
|
||||
- `hooks.transformsDir` (if set) must stay within the transforms root under your OpenClaw config directory (typically `~/.openclaw/hooks/transforms`).
|
||||
- `transform.module` must resolve within the effective transforms directory (traversal/escape paths are rejected).
|
||||
- Use `match.source` to keep a generic ingest endpoint (payload-driven routing).
|
||||
- TS transforms require a TS loader (e.g. `bun` or `tsx`) or precompiled `.js` at runtime.
|
||||
- Set `deliver: true` + `channel`/`to` on mappings to route replies to a chat surface
|
||||
|
||||
58
src/config/config.hooks-module-paths.test.ts
Normal file
58
src/config/config.hooks-module-paths.test.ts
Normal file
@@ -0,0 +1,58 @@
|
||||
import { describe, expect, it } from "vitest";
|
||||
import { validateConfigObjectWithPlugins } from "./config.js";
|
||||
|
||||
describe("config hooks module paths", () => {
|
||||
it("rejects absolute hooks.mappings[].transform.module", () => {
|
||||
const res = validateConfigObjectWithPlugins({
|
||||
agents: { list: [{ id: "pi" }] },
|
||||
hooks: {
|
||||
mappings: [
|
||||
{
|
||||
match: { path: "custom" },
|
||||
action: "agent",
|
||||
transform: { module: "/tmp/transform.mjs" },
|
||||
},
|
||||
],
|
||||
},
|
||||
});
|
||||
expect(res.ok).toBe(false);
|
||||
if (!res.ok) {
|
||||
expect(res.issues.some((iss) => iss.path === "hooks.mappings.0.transform.module")).toBe(true);
|
||||
}
|
||||
});
|
||||
|
||||
it("rejects escaping hooks.mappings[].transform.module", () => {
|
||||
const res = validateConfigObjectWithPlugins({
|
||||
agents: { list: [{ id: "pi" }] },
|
||||
hooks: {
|
||||
mappings: [
|
||||
{
|
||||
match: { path: "custom" },
|
||||
action: "agent",
|
||||
transform: { module: "../escape.mjs" },
|
||||
},
|
||||
],
|
||||
},
|
||||
});
|
||||
expect(res.ok).toBe(false);
|
||||
if (!res.ok) {
|
||||
expect(res.issues.some((iss) => iss.path === "hooks.mappings.0.transform.module")).toBe(true);
|
||||
}
|
||||
});
|
||||
|
||||
it("rejects absolute hooks.internal.handlers[].module", () => {
|
||||
const res = validateConfigObjectWithPlugins({
|
||||
agents: { list: [{ id: "pi" }] },
|
||||
hooks: {
|
||||
internal: {
|
||||
enabled: true,
|
||||
handlers: [{ event: "command:new", module: "/tmp/handler.mjs" }],
|
||||
},
|
||||
},
|
||||
});
|
||||
expect(res.ok).toBe(false);
|
||||
if (!res.ok) {
|
||||
expect(res.issues.some((iss) => iss.path === "hooks.internal.handlers.0.module")).toBe(true);
|
||||
}
|
||||
});
|
||||
});
|
||||
@@ -75,7 +75,7 @@ export type HooksGmailConfig = {
|
||||
export type InternalHookHandlerConfig = {
|
||||
/** Event key to listen for (e.g., 'command:new', 'session:start') */
|
||||
event: string;
|
||||
/** Path to handler module (absolute or relative to cwd) */
|
||||
/** Path to handler module (workspace-relative) */
|
||||
module: string;
|
||||
/** Export name from module (default: 'default') */
|
||||
export?: string;
|
||||
|
||||
@@ -1,6 +1,35 @@
|
||||
import path from "node:path";
|
||||
import { z } from "zod";
|
||||
import { sensitive } from "./zod-schema.sensitive.js";
|
||||
|
||||
function isSafeRelativeModulePath(raw: string): boolean {
|
||||
const value = raw.trim();
|
||||
if (!value) {
|
||||
return false;
|
||||
}
|
||||
// Hook modules are loaded via file-path resolution + dynamic import().
|
||||
// Keep this strictly relative to a configured base dir to avoid path traversal and surprises.
|
||||
if (path.isAbsolute(value)) {
|
||||
return false;
|
||||
}
|
||||
if (value.startsWith("~")) {
|
||||
return false;
|
||||
}
|
||||
// Disallow URL-ish and drive-relative forms (e.g. "file:...", "C:foo").
|
||||
if (value.includes(":")) {
|
||||
return false;
|
||||
}
|
||||
const parts = value.split(/[\\/]+/g);
|
||||
if (parts.some((part) => part === "..")) {
|
||||
return false;
|
||||
}
|
||||
return true;
|
||||
}
|
||||
|
||||
const SafeRelativeModulePathSchema = z
|
||||
.string()
|
||||
.refine(isSafeRelativeModulePath, "module must be a safe relative path (no absolute paths)");
|
||||
|
||||
export const HookMappingSchema = z
|
||||
.object({
|
||||
id: z.string().optional(),
|
||||
@@ -38,7 +67,7 @@ export const HookMappingSchema = z
|
||||
timeoutSeconds: z.number().int().positive().optional(),
|
||||
transform: z
|
||||
.object({
|
||||
module: z.string(),
|
||||
module: SafeRelativeModulePathSchema,
|
||||
export: z.string().optional(),
|
||||
})
|
||||
.strict()
|
||||
@@ -50,7 +79,7 @@ export const HookMappingSchema = z
|
||||
export const InternalHookHandlerSchema = z
|
||||
.object({
|
||||
event: z.string(),
|
||||
module: z.string(),
|
||||
module: SafeRelativeModulePathSchema,
|
||||
export: z.string().optional(),
|
||||
})
|
||||
.strict();
|
||||
|
||||
@@ -217,7 +217,6 @@ describe("hooks mapping", () => {
|
||||
expect("skipped" in result).toBe(true);
|
||||
}
|
||||
});
|
||||
|
||||
it("treats null transform as a handled skip", async () => {
|
||||
const configDir = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-config-skip-"));
|
||||
const transformsRoot = path.join(configDir, "hooks", "transforms");
|
||||
|
||||
@@ -144,6 +144,18 @@ describe("gateway server auth/connect", () => {
|
||||
signedAtMs,
|
||||
token: token ?? null,
|
||||
});
|
||||
|
||||
test("ignores requested scopes when device identity is omitted", async () => {
|
||||
const ws = await openWs(port);
|
||||
const res = await connectReq(ws, { device: null });
|
||||
expect(res.ok).toBe(true);
|
||||
|
||||
const health = await rpcReq(ws, "health");
|
||||
expect(health.ok).toBe(false);
|
||||
expect(health.error?.message).toContain("missing scope");
|
||||
|
||||
ws.close();
|
||||
});
|
||||
const device = {
|
||||
id: identity.deviceId,
|
||||
publicKey: publicKeyRawBase64UrlFromPem(identity.publicKeyPem),
|
||||
@@ -493,6 +505,9 @@ describe("gateway server auth/connect", () => {
|
||||
const ws = await openTailscaleWs(port);
|
||||
const res = await connectReq(ws, { token: "secret", device: null });
|
||||
expect(res.ok).toBe(true);
|
||||
const health = await rpcReq(ws, "health");
|
||||
expect(health.ok).toBe(false);
|
||||
expect(health.error?.message).toContain("missing scope");
|
||||
ws.close();
|
||||
});
|
||||
});
|
||||
|
||||
@@ -298,7 +298,9 @@ export function attachGatewayWsMessageHandler(params: {
|
||||
return;
|
||||
}
|
||||
// Default-deny: scopes must be explicit. Empty/missing scopes means no permissions.
|
||||
const scopes = Array.isArray(connectParams.scopes) ? connectParams.scopes : [];
|
||||
// Note: If the client does not present a device identity, we can't bind scopes to a paired
|
||||
// device/token, so we will clear scopes after auth to avoid self-declared permissions.
|
||||
let scopes = Array.isArray(connectParams.scopes) ? connectParams.scopes : [];
|
||||
connectParams.role = role;
|
||||
connectParams.scopes = scopes;
|
||||
|
||||
@@ -428,6 +430,10 @@ export function attachGatewayWsMessageHandler(params: {
|
||||
close(1008, truncateCloseReason(authMessage));
|
||||
};
|
||||
if (!device) {
|
||||
if (scopes.length > 0) {
|
||||
scopes = [];
|
||||
connectParams.scopes = scopes;
|
||||
}
|
||||
const canSkipDevice = sharedAuthOk;
|
||||
|
||||
if (isControlUi && !allowControlUiBypass) {
|
||||
|
||||
@@ -79,7 +79,7 @@ describe("loader", () => {
|
||||
handlers: [
|
||||
{
|
||||
event: "command:new",
|
||||
module: handlerPath,
|
||||
module: path.basename(handlerPath),
|
||||
},
|
||||
],
|
||||
},
|
||||
@@ -106,8 +106,8 @@ describe("loader", () => {
|
||||
internal: {
|
||||
enabled: true,
|
||||
handlers: [
|
||||
{ event: "command:new", module: handler1Path },
|
||||
{ event: "command:stop", module: handler2Path },
|
||||
{ event: "command:new", module: path.basename(handler1Path) },
|
||||
{ event: "command:stop", module: path.basename(handler2Path) },
|
||||
],
|
||||
},
|
||||
},
|
||||
@@ -138,7 +138,7 @@ describe("loader", () => {
|
||||
handlers: [
|
||||
{
|
||||
event: "command:new",
|
||||
module: handlerPath,
|
||||
module: path.basename(handlerPath),
|
||||
export: "myHandler",
|
||||
},
|
||||
],
|
||||
@@ -158,7 +158,7 @@ describe("loader", () => {
|
||||
handlers: [
|
||||
{
|
||||
event: "command:new",
|
||||
module: "/nonexistent/path/handler.js",
|
||||
module: "missing-handler.js",
|
||||
},
|
||||
],
|
||||
},
|
||||
@@ -182,7 +182,7 @@ describe("loader", () => {
|
||||
handlers: [
|
||||
{
|
||||
event: "command:new",
|
||||
module: handlerPath,
|
||||
module: path.basename(handlerPath),
|
||||
},
|
||||
],
|
||||
},
|
||||
@@ -199,8 +199,8 @@ describe("loader", () => {
|
||||
const handlerPath = path.join(tmpDir, "relative-handler.js");
|
||||
await fs.writeFile(handlerPath, "export default async function() {}", "utf-8");
|
||||
|
||||
// Get relative path from cwd
|
||||
const relativePath = path.relative(process.cwd(), handlerPath);
|
||||
// Relative to workspaceDir (tmpDir)
|
||||
const relativePath = path.relative(tmpDir, handlerPath);
|
||||
|
||||
const cfg: OpenClawConfig = {
|
||||
hooks: {
|
||||
@@ -241,7 +241,7 @@ describe("loader", () => {
|
||||
handlers: [
|
||||
{
|
||||
event: "command:new",
|
||||
module: handlerPath,
|
||||
module: path.basename(handlerPath),
|
||||
},
|
||||
],
|
||||
},
|
||||
|
||||
@@ -116,10 +116,25 @@ export async function loadInternalHooks(
|
||||
const handlers = cfg.hooks.internal.handlers ?? [];
|
||||
for (const handlerConfig of handlers) {
|
||||
try {
|
||||
// Resolve module path (absolute or relative to cwd)
|
||||
const modulePath = path.isAbsolute(handlerConfig.module)
|
||||
? handlerConfig.module
|
||||
: path.join(process.cwd(), handlerConfig.module);
|
||||
// Legacy handler paths: keep them workspace-relative.
|
||||
const rawModule = handlerConfig.module.trim();
|
||||
if (!rawModule) {
|
||||
log.error("Handler module path is empty");
|
||||
continue;
|
||||
}
|
||||
if (path.isAbsolute(rawModule)) {
|
||||
log.error(
|
||||
`Handler module path must be workspace-relative (got absolute path): ${rawModule}`,
|
||||
);
|
||||
continue;
|
||||
}
|
||||
const baseDir = path.resolve(workspaceDir);
|
||||
const modulePath = path.resolve(baseDir, rawModule);
|
||||
const rel = path.relative(baseDir, modulePath);
|
||||
if (!rel || rel.startsWith("..") || path.isAbsolute(rel)) {
|
||||
log.error(`Handler module path must stay within workspaceDir: ${rawModule}`);
|
||||
continue;
|
||||
}
|
||||
|
||||
// Import the module with cache-busting to ensure fresh reload
|
||||
const url = pathToFileURL(modulePath).href;
|
||||
|
||||
Reference in New Issue
Block a user