diff --git a/src/discord/monitor/exec-approvals.test.ts b/src/discord/monitor/exec-approvals.test.ts index 4184b6387c4..f3adf7089c3 100644 --- a/src/discord/monitor/exec-approvals.test.ts +++ b/src/discord/monitor/exec-approvals.test.ts @@ -309,6 +309,15 @@ describe("DiscordExecApprovalHandler.shouldHandle", () => { ); }); + it("rejects unsafe nested-repetition regex in session filter", () => { + const handler = createHandler({ + enabled: true, + approvers: ["123"], + sessionFilter: ["(a+)+$"], + }); + expect(handler.shouldHandle(createRequest({ sessionKey: `${"a".repeat(28)}!` }))).toBe(false); + }); + it("filters by discord account when session store includes account", () => { writeStore({ "agent:test-agent:discord:channel:999888777": { diff --git a/src/discord/monitor/exec-approvals.ts b/src/discord/monitor/exec-approvals.ts index 66f3c85905f..68f46b5e1c2 100644 --- a/src/discord/monitor/exec-approvals.ts +++ b/src/discord/monitor/exec-approvals.ts @@ -24,6 +24,7 @@ import type { import { logDebug, logError } from "../../logger.js"; import { normalizeAccountId, resolveAgentIdFromSessionKey } from "../../routing/session-key.js"; import type { RuntimeEnv } from "../../runtime.js"; +import { compileSafeRegex } from "../../security/safe-regex.js"; import { GATEWAY_CLIENT_MODES, GATEWAY_CLIENT_NAMES, @@ -364,11 +365,11 @@ export class DiscordExecApprovalHandler { return false; } const matches = config.sessionFilter.some((p) => { - try { - return session.includes(p) || new RegExp(p).test(session); - } catch { - return session.includes(p); + if (session.includes(p)) { + return true; } + const regex = compileSafeRegex(p); + return regex ? regex.test(session) : false; }); if (!matches) { return false; diff --git a/src/infra/exec-approval-forwarder.test.ts b/src/infra/exec-approval-forwarder.test.ts index 6902d846157..298efa4789c 100644 --- a/src/infra/exec-approval-forwarder.test.ts +++ b/src/infra/exec-approval-forwarder.test.ts @@ -160,6 +160,34 @@ describe("exec approval forwarder", () => { expect(deliver).not.toHaveBeenCalled(); }); + it("rejects unsafe nested-repetition regex in sessionFilter", async () => { + const cfg = { + approvals: { + exec: { + enabled: true, + mode: "session", + sessionFilter: ["(a+)+$"], + }, + }, + } as OpenClawConfig; + + const { deliver, forwarder } = createForwarder({ + cfg, + resolveSessionTarget: () => ({ channel: "slack", to: "U1" }), + }); + + const request = { + ...baseRequest, + request: { + ...baseRequest.request, + sessionKey: `${"a".repeat(28)}!`, + }, + }; + + await expect(forwarder.handleRequested(request)).resolves.toBe(false); + expect(deliver).not.toHaveBeenCalled(); + }); + it("returns false when all targets are skipped", async () => { await expectDiscordSessionTargetRequest({ cfg: makeSessionCfg({ discordExecApprovalsEnabled: true }), diff --git a/src/infra/exec-approval-forwarder.ts b/src/infra/exec-approval-forwarder.ts index e9d3dbad3f8..46617f07d7d 100644 --- a/src/infra/exec-approval-forwarder.ts +++ b/src/infra/exec-approval-forwarder.ts @@ -7,6 +7,7 @@ import type { } from "../config/types.approvals.js"; import { createSubsystemLogger } from "../logging/subsystem.js"; import { normalizeAccountId, parseAgentSessionKey } from "../routing/session-key.js"; +import { compileSafeRegex } from "../security/safe-regex.js"; import { isDeliverableMessageChannel, normalizeMessageChannel } from "../utils/message-channel.js"; import type { ExecApprovalDecision, @@ -52,11 +53,11 @@ function normalizeMode(mode?: ExecApprovalForwardingConfig["mode"]) { function matchSessionFilter(sessionKey: string, patterns: string[]): boolean { return patterns.some((pattern) => { - try { - return sessionKey.includes(pattern) || new RegExp(pattern).test(sessionKey); - } catch { - return sessionKey.includes(pattern); + if (sessionKey.includes(pattern)) { + return true; } + const regex = compileSafeRegex(pattern); + return regex ? regex.test(sessionKey) : false; }); } diff --git a/src/logging/redact.test.ts b/src/logging/redact.test.ts index 131c41c6b22..91180619a17 100644 --- a/src/logging/redact.test.ts +++ b/src/logging/redact.test.ts @@ -93,6 +93,15 @@ describe("redactSensitiveText", () => { expect(output).toBe("token=abcdef…ghij"); }); + it("ignores unsafe nested-repetition custom patterns", () => { + const input = `${"a".repeat(28)}!`; + const output = redactSensitiveText(input, { + mode: "tools", + patterns: ["(a+)+$"], + }); + expect(output).toBe(input); + }); + it("skips redaction when mode is off", () => { const input = "OPENAI_API_KEY=sk-1234567890abcdef"; const output = redactSensitiveText(input, { diff --git a/src/logging/redact.ts b/src/logging/redact.ts index 60e9e6601a5..836e9f68405 100644 --- a/src/logging/redact.ts +++ b/src/logging/redact.ts @@ -1,4 +1,5 @@ import type { OpenClawConfig } from "../config/config.js"; +import { compileSafeRegex } from "../security/safe-regex.js"; import { resolveNodeRequireFromMeta } from "./node-require.js"; const requireConfig = resolveNodeRequireFromMeta(import.meta.url); @@ -51,15 +52,11 @@ function parsePattern(raw: string): RegExp | null { return null; } const match = raw.match(/^\/(.+)\/([gimsuy]*)$/); - try { - if (match) { - const flags = match[2].includes("g") ? match[2] : `${match[2]}g`; - return new RegExp(match[1], flags); - } - return new RegExp(raw, "gi"); - } catch { - return null; + if (match) { + const flags = match[2].includes("g") ? match[2] : `${match[2]}g`; + return compileSafeRegex(match[1], flags); } + return compileSafeRegex(raw, "gi"); } function resolvePatterns(value?: string[]): RegExp[] { diff --git a/src/security/safe-regex.test.ts b/src/security/safe-regex.test.ts new file mode 100644 index 00000000000..30fa2793649 --- /dev/null +++ b/src/security/safe-regex.test.ts @@ -0,0 +1,26 @@ +import { describe, expect, it } from "vitest"; +import { compileSafeRegex, hasNestedRepetition } from "./safe-regex.js"; + +describe("safe regex", () => { + it("flags nested repetition patterns", () => { + expect(hasNestedRepetition("(a+)+$")).toBe(true); + expect(hasNestedRepetition("^(?:foo|bar)$")).toBe(false); + }); + + it("rejects unsafe nested repetition during compile", () => { + expect(compileSafeRegex("(a+)+$")).toBeNull(); + }); + + it("compiles common safe filter regex", () => { + const re = compileSafeRegex("^agent:.*:discord:"); + expect(re).toBeInstanceOf(RegExp); + expect(re?.test("agent:main:discord:channel:123")).toBe(true); + expect(re?.test("agent:main:telegram:channel:123")).toBe(false); + }); + + it("supports explicit flags", () => { + const re = compileSafeRegex("token=([A-Za-z0-9]+)", "gi"); + expect(re).toBeInstanceOf(RegExp); + expect("TOKEN=abcd1234".replace(re as RegExp, "***")).toBe("***"); + }); +}); diff --git a/src/security/safe-regex.ts b/src/security/safe-regex.ts new file mode 100644 index 00000000000..4f4f6921ab2 --- /dev/null +++ b/src/security/safe-regex.ts @@ -0,0 +1,151 @@ +type QuantifierRead = { + consumed: number; +}; + +type TokenState = { + containsRepetition: boolean; +}; + +type ParseFrame = { + lastToken: TokenState | null; + containsRepetition: boolean; +}; + +const SAFE_REGEX_CACHE_MAX = 256; +const safeRegexCache = new Map(); + +export function hasNestedRepetition(source: string): boolean { + // Conservative parser: reject patterns where a repeated token/group is repeated again. + const frames: ParseFrame[] = [{ lastToken: null, containsRepetition: false }]; + let inCharClass = false; + + const emitToken = (token: TokenState) => { + const frame = frames[frames.length - 1]; + frame.lastToken = token; + if (token.containsRepetition) { + frame.containsRepetition = true; + } + }; + + for (let i = 0; i < source.length; i += 1) { + const ch = source[i]; + + if (ch === "\\") { + i += 1; + emitToken({ containsRepetition: false }); + continue; + } + + if (inCharClass) { + if (ch === "]") { + inCharClass = false; + } + continue; + } + + if (ch === "[") { + inCharClass = true; + emitToken({ containsRepetition: false }); + continue; + } + + if (ch === "(") { + frames.push({ lastToken: null, containsRepetition: false }); + continue; + } + + if (ch === ")") { + if (frames.length > 1) { + const frame = frames.pop() as ParseFrame; + emitToken({ containsRepetition: frame.containsRepetition }); + } + continue; + } + + if (ch === "|") { + const frame = frames[frames.length - 1]; + frame.lastToken = null; + continue; + } + + const quantifier = readQuantifier(source, i); + if (quantifier) { + const frame = frames[frames.length - 1]; + const token = frame.lastToken; + if (!token) { + continue; + } + if (token.containsRepetition) { + return true; + } + token.containsRepetition = true; + frame.containsRepetition = true; + i += quantifier.consumed - 1; + continue; + } + + emitToken({ containsRepetition: false }); + } + + return false; +} + +function readQuantifier(source: string, index: number): QuantifierRead | null { + const ch = source[index]; + if (ch === "*" || ch === "+" || ch === "?") { + return { consumed: source[index + 1] === "?" ? 2 : 1 }; + } + if (ch !== "{") { + return null; + } + let i = index + 1; + while (i < source.length && /\d/.test(source[i])) { + i += 1; + } + if (i === index + 1) { + return null; + } + if (source[i] === ",") { + i += 1; + while (i < source.length && /\d/.test(source[i])) { + i += 1; + } + } + if (source[i] !== "}") { + return null; + } + i += 1; + if (source[i] === "?") { + i += 1; + } + return { consumed: i - index }; +} + +export function compileSafeRegex(source: string, flags = ""): RegExp | null { + const trimmed = source.trim(); + if (!trimmed) { + return null; + } + const cacheKey = `${flags}::${trimmed}`; + if (safeRegexCache.has(cacheKey)) { + return safeRegexCache.get(cacheKey) ?? null; + } + + let compiled: RegExp | null = null; + if (!hasNestedRepetition(trimmed)) { + try { + compiled = new RegExp(trimmed, flags); + } catch { + compiled = null; + } + } + + safeRegexCache.set(cacheKey, compiled); + if (safeRegexCache.size > SAFE_REGEX_CACHE_MAX) { + const oldestKey = safeRegexCache.keys().next().value; + if (oldestKey) { + safeRegexCache.delete(oldestKey); + } + } + return compiled; +}