From d95cf256e755e3a804da55f13ef5afb3b6530afa Mon Sep 17 00:00:00 2001 From: liquidhorizon88-bot Date: Tue, 3 Mar 2026 18:47:57 -0500 Subject: [PATCH] Security audit: suggest valid gateway.nodes.denyCommands entries (#29713) Merged via squash. Prepared head SHA: db23298f9806b8de8c4b3e816f1649c18ebc0c64 Co-authored-by: liquidhorizon88-bot <257047709+liquidhorizon88-bot@users.noreply.github.com> Co-authored-by: grp06 <1573959+grp06@users.noreply.github.com> Reviewed-by: @grp06 --- CHANGELOG.md | 1 + extensions/feishu/src/client.test.ts | 19 +++++++- src/security/audit-extra.sync.ts | 69 ++++++++++++++++++++++++++-- src/security/audit.test.ts | 39 ++++++++++++++++ 4 files changed, 123 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f595a666416..2284c1a0dc3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ Docs: https://docs.openclaw.ai ### Fixes - Security/auth labels: remove token and API-key snippets from user-facing auth status labels so `/status` and `/models` do not expose credential fragments. (#33262) thanks @cu1ch3n. +- Security/audit denyCommands guidance: suggest likely exact node command IDs for unknown `gateway.nodes.denyCommands` entries so ineffective denylist entries are easier to correct. (#29713) thanks @liquidhorizon88-bot. - Docs/security hardening guidance: document Docker `DOCKER-USER` + UFW policy and add cross-linking from Docker install docs for VPS/public-host setups. (#27613) thanks @dorukardahan. - Docs/security threat-model links: replace relative `.md` links with Mintlify-compatible root-relative routes in security docs to prevent broken internal navigation. (#27698) thanks @clawdoo. - iOS/Voice timing safety: guard system speech start/finish callbacks to the active utterance to avoid misattributed start events during rapid stop/restart cycles. (#33304) thanks @mbelinky; original implementation direction by @ngutman. diff --git a/extensions/feishu/src/client.test.ts b/extensions/feishu/src/client.test.ts index de05dcb9619..e7a9e097082 100644 --- a/extensions/feishu/src/client.test.ts +++ b/extensions/feishu/src/client.test.ts @@ -77,9 +77,12 @@ describe("createFeishuWSClient proxy handling", () => { expect(options?.agent).toBeUndefined(); }); - it("prefers HTTPS proxy vars over HTTP proxy vars across runtimes", () => { + it("uses proxy env precedence: https_proxy first, then HTTPS_PROXY, then http_proxy/HTTP_PROXY", () => { + // NOTE: On Windows, environment variables are case-insensitive, so it's not + // possible to set both https_proxy and HTTPS_PROXY to different values. + // Keep this test cross-platform by asserting precedence via mutually-exclusive + // setups. process.env.https_proxy = "http://lower-https:8001"; - process.env.HTTPS_PROXY = "http://upper-https:8002"; process.env.http_proxy = "http://lower-http:8003"; process.env.HTTP_PROXY = "http://upper-http:8004"; @@ -108,6 +111,18 @@ describe("createFeishuWSClient proxy handling", () => { expect(options.agent).toEqual({ proxyUrl: expectedHttpsProxy }); }); + it("uses HTTPS_PROXY when https_proxy is unset", () => { + process.env.HTTPS_PROXY = "http://upper-https:8002"; + process.env.http_proxy = "http://lower-http:8003"; + + createFeishuWSClient(baseAccount); + + expect(httpsProxyAgentCtorMock).toHaveBeenCalledTimes(1); + expect(httpsProxyAgentCtorMock).toHaveBeenCalledWith("http://upper-https:8002"); + const options = firstWsClientOptions(); + expect(options.agent).toEqual({ proxyUrl: "http://upper-https:8002" }); + }); + it("passes HTTP_PROXY to ws client when https vars are unset", () => { process.env.HTTP_PROXY = "http://upper-http:8999"; diff --git a/src/security/audit-extra.sync.ts b/src/security/audit-extra.sync.ts index a3f81d40870..8d14ced6fea 100644 --- a/src/security/audit-extra.sync.ts +++ b/src/security/audit-extra.sync.ts @@ -240,6 +240,61 @@ function looksLikeNodeCommandPattern(value: string): boolean { return /\s/.test(value) || value.includes("group:"); } +function editDistance(a: string, b: string): number { + if (a === b) { + return 0; + } + if (!a) { + return b.length; + } + if (!b) { + return a.length; + } + + const dp: number[] = Array.from({ length: b.length + 1 }, (_, j) => j); + + for (let i = 1; i <= a.length; i++) { + let prev = dp[0]; + dp[0] = i; + for (let j = 1; j <= b.length; j++) { + const temp = dp[j]; + const cost = a[i - 1] === b[j - 1] ? 0 : 1; + dp[j] = Math.min(dp[j] + 1, dp[j - 1] + 1, prev + cost); + prev = temp; + } + } + + return dp[b.length]; +} + +function suggestKnownNodeCommands(unknown: string, known: Set): string[] { + const needle = unknown.trim(); + if (!needle) { + return []; + } + + // Fast path: prefix-ish suggestions. + const prefix = needle.includes(".") ? needle.split(".").slice(0, 2).join(".") : needle; + const prefixHits = Array.from(known) + .filter((cmd) => cmd.startsWith(prefix)) + .slice(0, 3); + if (prefixHits.length > 0) { + return prefixHits; + } + + // Fuzzy: Levenshtein over a small-ish known set. + const ranked = Array.from(known) + .map((cmd) => ({ cmd, d: editDistance(needle, cmd) })) + .toSorted((a, b) => a.d - b.d || a.cmd.localeCompare(b.cmd)); + + const best = ranked[0]?.d ?? Infinity; + const threshold = Math.max(2, Math.min(4, best)); + return ranked + .filter((r) => r.d <= threshold) + .slice(0, 3) + .map((r) => r.cmd); +} + function resolveToolPolicies(params: { cfg: OpenClawConfig; agentTools?: AgentToolsConfig; @@ -944,9 +999,17 @@ export function collectNodeDenyCommandPatternFindings(cfg: OpenClawConfig): Secu ); } if (unknownExact.length > 0) { - detailParts.push( - `Unknown command names (not in defaults/allowCommands): ${unknownExact.join(", ")}`, - ); + const unknownDetails = unknownExact + .map((entry) => { + const suggestions = suggestKnownNodeCommands(entry, knownCommands); + if (suggestions.length === 0) { + return entry; + } + return `${entry} (did you mean: ${suggestions.join(", ")})`; + }) + .join(", "); + + detailParts.push(`Unknown command names (not in defaults/allowCommands): ${unknownDetails}`); } const examples = Array.from(knownCommands).slice(0, 8); diff --git a/src/security/audit.test.ts b/src/security/audit.test.ts index f22e9725745..8eb3ff71aba 100644 --- a/src/security/audit.test.ts +++ b/src/security/audit.test.ts @@ -1156,6 +1156,45 @@ description: test skill expect(finding?.severity).toBe("warn"); expect(finding?.detail).toContain("system.*"); expect(finding?.detail).toContain("system.runx"); + expect(finding?.detail).toContain("did you mean"); + expect(finding?.detail).toContain("system.run"); + }); + + it("suggests prefix-matching commands for unknown denyCommands entries", async () => { + const cfg: OpenClawConfig = { + gateway: { + nodes: { + denyCommands: ["system.run.prep"], + }, + }, + }; + + const res = await audit(cfg); + const finding = res.findings.find( + (f) => f.checkId === "gateway.nodes.deny_commands_ineffective", + ); + expect(finding?.severity).toBe("warn"); + expect(finding?.detail).toContain("system.run.prep"); + expect(finding?.detail).toContain("did you mean"); + expect(finding?.detail).toContain("system.run.prepare"); + }); + + it("keeps unknown denyCommands entries without suggestions when no close command exists", async () => { + const cfg: OpenClawConfig = { + gateway: { + nodes: { + denyCommands: ["zzzzzzzzzzzzzz"], + }, + }, + }; + + const res = await audit(cfg); + const finding = res.findings.find( + (f) => f.checkId === "gateway.nodes.deny_commands_ineffective", + ); + expect(finding?.severity).toBe("warn"); + expect(finding?.detail).toContain("zzzzzzzzzzzzzz"); + expect(finding?.detail).not.toContain("did you mean"); }); it("scores dangerous gateway.nodes.allowCommands by exposure", async () => {