fix(security): harden regex compilation for filters and redaction
This commit is contained in:
@@ -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": {
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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 }),
|
||||
|
||||
@@ -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;
|
||||
});
|
||||
}
|
||||
|
||||
|
||||
@@ -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, {
|
||||
|
||||
@@ -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[] {
|
||||
|
||||
26
src/security/safe-regex.test.ts
Normal file
26
src/security/safe-regex.test.ts
Normal file
@@ -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("***");
|
||||
});
|
||||
});
|
||||
151
src/security/safe-regex.ts
Normal file
151
src/security/safe-regex.ts
Normal file
@@ -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<string, RegExp | null>();
|
||||
|
||||
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;
|
||||
}
|
||||
Reference in New Issue
Block a user