fix(health): mention is not use — call-shaped rules only fire from code #5617
2 changed files+171−3
Addedsrc/__tests__/mention-is-not-use.test.ts+93−0View fileUnifiedSplit
@@ -0,0 +1,93 @@
1/**
2 * Mention is not use.
3 *
4 * After the language guard removed the Rust false positive, three `no-eval`
5 * findings survived on a real repo — and all three were MENTIONS in
6 * security-conscious code, fetched verbatim rather than imagined (inventing
7 * fixtures produced tests that pinned nonexistent behaviour three times in
8 * one day):
9 *
10 * security-headers.ts:7 a comment quoting the CSP effect of blocking eval
11 * waf-rules.ts:77 a WAF rule's own description string
12 * security.ts:11 a comment noting wasm-unsafe-eval does not enable eval
13 *
14 * The same bug as `eval` in a Rust signature file, wearing a .ts extension:
15 * the places that DISCUSS eval most are CSP comments, WAF signature lists
16 * and security middleware — exactly where a naive rule fires most and
17 * matters least.
18 *
19 * `no-security-disable` gets the prose treatment instead: all four findings
20 * the widened file set produced were .md files QUOTING a governed directive
21 * (a status doc explaining the sanctioned `strategic-override:` mechanism).
22 * In prose, describing a directive is not applying one. In a shell script
23 * that undoes hardening, it still fires — a rollback tool genuinely does
24 * disable security controls, and `low` is the honest reading of that.
25 */
26import { describe, expect, it } from "bun:test";
27import { detectSecurityIssuesInLine, patternMatchesInCode } from "../lib/intelligence";
28
29const rules = (filePath: string, line: string) =>
30 detectSecurityIssuesInLine({ filePath, line, lineNumber: 1 }).map((i) => i.rule);
31
32describe("no-eval: the three real mentions, verbatim", () => {
33 it("a comment explaining a CSP is not a code-injection risk", () => {
34 expect(rules("apps/api/src/middleware/security-headers.ts", "// keeps non-WASM eval() blocked.")).toEqual([]); // gluecron:allow-secret
35 expect(
36 rules("services/bun-gateway/src/middleware/security.ts", "// WebGPU inference). Despite the name, this does NOT enable eval() or") // gluecron:allow-secret
37 ).toEqual([]);
38 });
39
40 it("a WAF rule's description string is not a code-injection risk", () => {
41 expect(
42 rules(
43 "apps/api/src/middleware/waf-rules.ts",
44 ' "OWASP cross-site scripting signature match (<script>, javascript:, on* handlers, <iframe>, eval()).",' // gluecron:allow-secret
45 )
46 ).toEqual([]);
47 });
48
49 it("a block-comment interior line is a mention", () => {
50 expect(rules("src/a.ts", " * calls eval() on the payload")).toEqual([]); // gluecron:allow-secret
51 });
52});
53
54describe("no-eval: real calls still fire", () => {
55 it("a plain call fires", () => {
56 expect(rules("src/a.js", "const r = eval(userInput);")).toContain("no-eval"); // gluecron:allow-secret
57 });
58
59 it("one mention does not immunise a real call on the same line", () => {
60 expect(patternMatchesInCode('check("eval(") && eval(x)', /eval\s*\(/)).toBe(true); // gluecron:allow-secret
61 });
62
63 it("a // inside a URL string does not start a comment", () => {
64 expect(patternMatchesInCode('fetch("https://x.test"); eval(y);', /eval\s*\(/)).toBe(true); // gluecron:allow-secret
65 });
66
67 it("document.write gets the same treatment", () => {
68 expect(rules("src/a.js", "// never use document.write(html) here")).toEqual([]);
69 expect(rules("src/a.js", "document.write(html);")).toContain("no-document-write"); // gluecron:allow-secret
70 });
71});
72
73describe("rules that WANT comments are untouched", () => {
74 it("security-todo still fires from a comment", () => {
75 expect(rules("src/a.ts", "// TODO: fix this security hole")).toContain("security-todo");
76 });
77});
78
79describe("no-security-disable: prose describes, code applies", () => {
80 it("a status doc quoting the governed override mechanism is not a finding", () => {
81 expect(
82 rules("docs/STATUS.md", "Rule-5 allows `strategic-override:` to disable a security lint, with a DECISIONS.md reference.")
83 ).toEqual([]);
84 });
85
86 it("a rollback script that disables security controls still fires", () => {
87 // unharden-box.sh exists to undo hardening; that a security-disabling
88 // script matches a security-disabling rule is the rule working.
89 expect(rules("infra/security/unharden-box.sh", "systemctl disable security-hardening.service")).toContain(
90 "no-security-disable"
91 );
92 });
93});
Modifiedsrc/lib/intelligence.ts+78−3View fileUnifiedSplit
@@ -502,6 +502,18 @@ const SECURITY_PATTERNS: Array<{
502502 rule: string;
503503 /** Extensions this rule may fire on. Omitted = every file. */
504504 appliesTo?: readonly string[];
505 /**
506 * Fire only when a match sits in CODE — not in a comment or string literal.
507 *
508 * For call-shaped rules, mention is not use. Measured on a real repo, all
509 * three surviving `no-eval` findings were mentions: two comments explaining
510 * a CSP ("keeps non-WASM eval blocked", "does NOT enable eval") and a
511 * WAF rule's own description string listing the signatures it matches. The
512 * language guard removed the Rust case; these were the same bug wearing a
513 * .ts extension. Off by default because some rules WANT comments —
514 * `security-todo` is nothing but comments.
515 */
516 codeOnly?: boolean;
505517}> = [
506518 // Hardcoded secrets
507519 { pattern: /(?:password|passwd|pwd)\s*[:=]\s*["']([^"']{4,})/i, severity: "critical", message: "Possible hardcoded password", rule: "no-hardcoded-secrets", appliesTo: NOT_PROSE },
@@ -562,9 +574,9 @@ const SECURITY_PATTERNS: Array<{
562574 { pattern: /\bnpm_[A-Za-z0-9]{36}\b/, severity: "critical", message: "npm access token committed", rule: "no-vendor-credentials" },
563575 { pattern: /\bSG\.[A-Za-z0-9_-]{22}\.[A-Za-z0-9_-]{43}\b/, severity: "critical", message: "SendGrid API key committed", rule: "no-vendor-credentials" },
564576 // Injection vulnerabilities
565 { pattern: /eval\s*\(/, severity: "high", message: "eval call — potential code injection", rule: "no-eval", appliesTo: EVAL_LANGS },
577 { pattern: /eval\s*\(/, severity: "high", message: "eval call — potential code injection", rule: "no-eval", appliesTo: EVAL_LANGS, codeOnly: true },
566578 { pattern: /innerHTML\s*=/, severity: "medium", message: "Direct innerHTML assignment — potential XSS", rule: "no-inner-html", appliesTo: JS_TS },
567 { pattern: /document\.write\s*\(/, severity: "medium", message: "document.write usage — potential XSS", rule: "no-document-write", appliesTo: JS_TS },
579 { pattern: /document\.write\s*\(/, severity: "medium", message: "document.write usage — potential XSS", rule: "no-document-write", appliesTo: JS_TS, codeOnly: true },
568580 { pattern: /exec\s*\(\s*[`"'].*\$\{/, severity: "high", message: "Shell command with template literal — potential injection", rule: "no-shell-injection", appliesTo: JS_TS },
569581 // SQL injection
570582 { pattern: /query\s*\(\s*[`"'].*\$\{/, severity: "high", message: "SQL query with interpolation — potential SQL injection", rule: "no-sql-injection", appliesTo: JS_TS },
@@ -574,7 +586,7 @@ const SECURITY_PATTERNS: Array<{
574586 { pattern: /createHash\s*\(\s*["']sha1["']\)/, severity: "low", message: "SHA1 hash — consider SHA-256+", rule: "weak-hash", appliesTo: JS_TS },
575587 // Misc
576588 { pattern: /TODO.*(?:security|hack|fixme|unsafe|vulnerable)/i, severity: "info", message: "Security-related TODO found", rule: "security-todo" },
577 { pattern: /(?:disable|ignore).*(?:eslint|tslint|security)/i, severity: "low", message: "Security linter rule disabled", rule: "no-security-disable" },
589 { pattern: /(?:disable|ignore).*(?:eslint|tslint|security)/i, severity: "low", message: "Security linter rule disabled", rule: "no-security-disable", appliesTo: NOT_PROSE },
578590];
579591
580592// Committed env *templates* are the documented, expected way to ship an
@@ -959,6 +971,66 @@ function joinStatement(lines: string[], start: number, maxLines = 60): string {
959971 return lines.slice(start, end).join("\n");
960972}
961973
974/**
975 * Does this pattern match anywhere in the CODE of the line — as opposed to
976 * matching only inside comments or string literals?
977 *
978 * Single pass classifying each character as code, string, or comment:
979 * `//` outside a string starts a comment (a `//` inside "https://…" does
980 * not); an unescaped quote opens a string until its unescaped partner; a
981 * line whose trimmed text begins with `*` or `/*` is treated as the interior
982 * of a block comment, the same heuristic the arg-injection test settled on.
983 * Every match of the pattern is tried, so a guard string and a real call on
984 * the same line still fires — one mention does not immunise the call.
985 *
986 * Known, accepted limit: an interpolation inside a template literal is
987 * classified as string, so an eval call inside an interpolation is missed.
988 * Rare, and the cost
989 * of a miss at this severity is far below the cost of flagging every WAF
990 * signature list and CSP comment in a security-conscious codebase — which is
991 * exactly where these rules fire most and matter least.
992 *
993 * Exported and asserted directly, per this module's convention.
994 */
995export function patternMatchesInCode(line: string, pattern: RegExp): boolean {
996 const trimmed = line.trimStart();
997 if (trimmed.startsWith("*") || trimmed.startsWith("/*")) return false;
998
999 // kinds[i] = true when line[i] is code.
1000 const kinds: boolean[] = new Array(line.length).fill(true);
1001 let quote: string | null = null;
1002 for (let i = 0; i < line.length; i++) {
1003 const ch = line[i];
1004 if (quote) {
1005 kinds[i] = false;
1006 if (ch === "\\") {
1007 if (i + 1 < line.length) kinds[++i] = false;
1008 continue;
1009 }
1010 if (ch === quote) quote = null;
1011 continue;
1012 }
1013 if (ch === '"' || ch === "'" || ch === "`") {
1014 quote = ch;
1015 kinds[i] = false;
1016 continue;
1017 }
1018 if (ch === "/" && line[i + 1] === "/") {
1019 for (let j = i; j < line.length; j++) kinds[j] = false;
1020 break;
1021 }
1022 }
1023
1024 const global = new RegExp(
1025 pattern.source,
1026 pattern.flags.includes("g") ? pattern.flags : pattern.flags + "g"
1027 );
1028 for (const m of line.matchAll(global)) {
1029 if (m.index !== undefined && kinds[m.index]) return true;
1030 }
1031 return false;
1032}
1033
9621034export function detectSecurityIssuesInLine(args: {
9631035 filePath: string;
9641036 line: string;
@@ -989,6 +1061,9 @@ export function detectSecurityIssuesInLine(args: {
9891061 const match = line.match(rule.pattern);
9901062 if (!match) continue;
9911063
1064 // Mention is not use: a call-shaped rule only fires from code.
1065 if (rule.codeOnly && !patternMatchesInCode(line, rule.pattern)) continue;
1066
9921067 if (rule.rule === "no-hardcoded-secrets") {
9931068 // Prefer the rule's own capture when it has one. The env-fallback rule
9941069 // captures the literal after `??`/`||`, which is the actual credential;
9951070
c comment · e edit title · m merge · a approve · r request changes · ? shortcuts