From 33e19fb5aeead4842ab5a4afd7ce28012a571bec Mon Sep 17 00:00:00 2001 From: Vincent Koc Date: Mon, 4 May 2026 02:35:43 -0700 Subject: [PATCH] fix(security): ignore scanner comment context --- CHANGELOG.md | 1 + .../npm-install-security-scan.release.test.ts | 18 ++++- src/security/skill-scanner.test.ts | 12 ++++ src/security/skill-scanner.ts | 67 +++++++++++++++++-- 4 files changed, 91 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 08098e7b425..e6671d1be2c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -66,6 +66,7 @@ Docs: https://docs.openclaw.ai - fix: harden backend message action gateway routing [AI]. (#76374) Thanks @pgondhi987. - Gate QQBot streaming command auth [AI]. (#76375) Thanks @pgondhi987. - Plugins/release: make the published npm runtime verifier reject blank `openclaw.runtimeExtensions` entries instead of treating them as absent and passing via inferred outputs. Thanks @vincentkoc. +- Plugins/security: ignore inline and block comments when matching source-rule context in plugin install scans, so comment-only `fetch`/`post` references near environment defaults do not block clean plugins. Thanks @vincentkoc. - Doctor/plugins: remove stale managed install records for bundled plugins even when the bundled plugin is not explicitly configured, so doctor cleanup cannot leave orphaned install metadata behind. Thanks @vincentkoc. - Web fetch: scope provider fallback cache entries by the selected fetch provider so config reloads cannot reuse another provider's cached fallback payload. Thanks @vincentkoc. - Web search: honor late-bound `tools.web.search.enabled: false` during tool execution so config reloads cannot leave an already-created `web_search` tool runnable. Thanks @vincentkoc. diff --git a/src/plugins/npm-install-security-scan.release.test.ts b/src/plugins/npm-install-security-scan.release.test.ts index 2d8bac8ad8c..1f4c8d3d335 100644 --- a/src/plugins/npm-install-security-scan.release.test.ts +++ b/src/plugins/npm-install-security-scan.release.test.ts @@ -21,11 +21,16 @@ type PublishablePluginPackage = { const REVIEWED_PUBLISHABLE_CRITICAL_FINDINGS = new Set([ "@openclaw/acpx:dangerous-exec:src/codex-auth-bridge.ts", "@openclaw/acpx:dangerous-exec:src/runtime-internals/mcp-proxy.mjs", + "@openclaw/acpx:dangerous-exec:dist/mcp-proxy.mjs", + "@openclaw/acpx:dangerous-exec:dist/service-.js", "@openclaw/codex:dangerous-exec:src/app-server/transport-stdio.ts", + "@openclaw/codex:dangerous-exec:dist/client-.js", "@openclaw/google-meet:dangerous-exec:src/node-host.ts", "@openclaw/google-meet:dangerous-exec:src/realtime.ts", + "@openclaw/google-meet:dangerous-exec:dist/index.js", "@openclaw/voice-call:dangerous-exec:src/tunnel.ts", "@openclaw/voice-call:dangerous-exec:src/webhook/tailscale.ts", + "@openclaw/voice-call:dangerous-exec:dist/runtime-entry-.js", ]); const tempDirs: string[] = []; @@ -72,6 +77,15 @@ function isScannerWalkedPackedPath(packedPath: string): boolean { ); } +function normalizePackedFindingPath(packedPath: string): string { + for (const prefix of ["client", "runtime-entry", "service"]) { + if (packedPath.startsWith(`dist/${prefix}-`) && packedPath.endsWith(".js")) { + return `dist/${prefix}-.js`; + } + } + return packedPath; +} + function stageScannerRelevantPackedFiles( packageDir: string, packedFiles: readonly string[], @@ -141,7 +155,9 @@ describe("publishable plugin npm package install security scan", () => { if (finding.severity !== "critical") { continue; } - const packedPath = relative(stageDir, finding.file).split(sep).join("/"); + const packedPath = normalizePackedFindingPath( + relative(stageDir, finding.file).split(sep).join("/"), + ); const key = `${plugin.packageName}:${finding.ruleId}:${packedPath}`; if (REVIEWED_PUBLISHABLE_CRITICAL_FINDINGS.has(key)) { reviewedCriticalFindings.add(key); diff --git a/src/security/skill-scanner.test.ts b/src/security/skill-scanner.test.ts index 2d3a0fee308..64c22171da2 100644 --- a/src/security/skill-scanner.test.ts +++ b/src/security/skill-scanner.test.ts @@ -274,6 +274,18 @@ const env = process.env; expect(findings.some((f) => f.ruleId === "env-harvesting")).toBe(false); }); + it("does not use inline or block comments as source-rule context", () => { + const source = ` +const env = process.env; // fetch("https://example.invalid") +/* + * rest.post("/channels/123/messages", {}); + */ +const url = "https://example.com/path//segment"; +`; + const findings = scanSource(source, "plugin.ts"); + expect(findings.some((f) => f.ruleId === "env-harvesting")).toBe(false); + }); + it("returns empty array for clean plugin code", () => { const source = ` export function greet(name: string): string { diff --git a/src/security/skill-scanner.ts b/src/security/skill-scanner.ts index 384b2f0efbc..e530183226a 100644 --- a/src/security/skill-scanner.ts +++ b/src/security/skill-scanner.ts @@ -236,11 +236,66 @@ function isBenignMemberExecMatch(line: string, match: RegExpExecArray): boolean return !/\b(?:cp|childProcess|child_process)\s*\.\s*exec\s*\(/.test(line); } -function stripFullLineCommentsForHeuristics(source: string): string { - return source - .split("\n") - .map((line) => (line.trimStart().startsWith("//") ? "" : line)) - .join("\n"); +function stripCommentsForHeuristics(source: string): string { + let stripped = ""; + let quote: "'" | '"' | "`" | null = null; + let escaped = false; + let inBlockComment = false; + + for (let i = 0; i < source.length; i++) { + const ch = source[i] ?? ""; + const next = source[i + 1] ?? ""; + + if (inBlockComment) { + if (ch === "*" && next === "/") { + inBlockComment = false; + i++; + continue; + } + if (ch === "\n") { + stripped += "\n"; + } + continue; + } + + if (quote) { + stripped += ch; + if (escaped) { + escaped = false; + } else if (ch === "\\") { + escaped = true; + } else if (ch === quote) { + quote = null; + } + continue; + } + + if (ch === "'" || ch === '"' || ch === "`") { + quote = ch; + stripped += ch; + continue; + } + + if (ch === "/" && next === "/") { + while (i < source.length && source[i] !== "\n") { + i++; + } + if (source[i] === "\n") { + stripped += "\n"; + } + continue; + } + + if (ch === "/" && next === "*") { + inBlockComment = true; + i++; + continue; + } + + stripped += ch; + } + + return stripped; } function findSourceRuleMatch(params: { @@ -282,7 +337,7 @@ function findSourceRuleMatch(params: { export function scanSource(source: string, filePath: string): SkillScanFinding[] { const findings: SkillScanFinding[] = []; const lines = source.split("\n"); - const heuristicSource = stripFullLineCommentsForHeuristics(source); + const heuristicSource = stripCommentsForHeuristics(source); const heuristicLines = heuristicSource.split("\n"); const matchedLineRules = new Set();