scan: cut false positives without losing coverage (0.4.0)

apps/cli/package.json

@@ -1,7 +1,7 @@
 {
   "name": "@profullstack/threatcrush",
-  "version": "0.3.0",
-  "description": "All-in-one security agent daemon — monitor, detect, scan, and protect servers in real-time",
+  "version": "0.4.0",
+  "description": "All-in-one security agent daemon \u2014 monitor, detect, scan, and protect servers in real-time",
   "bin": {
     "threatcrush": "./dist/index.js"
   },

apps/cli/src/scan/__tests__/code-rules.test.ts

@@ -184,3 +184,131 @@
     expect(contextual[0]?.severity).toBe('high');
   });
 });
+
+/**
+ * Accuracy fixes from the 0.3.1 triage. Every case below was a real finding
+ * reported against a real repository where the code was correct; each keeps a
+ * genuinely vulnerable counterpart beside it, because a rule that stops
+ * reporting the safe shape by also missing the dangerous one is worse than the
+ * noise it replaced.
+ */
+describe('unescaped HTML rendering: static assignments', () => {
+  it('stays silent on an assignment with no interpolation', () => {
+    // A UI built with innerHTML reports every static heading and spinner. That
+    // was the largest single source of noise in the corpus.
+    expect(ruleIds('a.js', `el.innerHTML = '<div class="spinner"></div>';`)).toHaveLength(0);
+    expect(ruleIds('a.js', 'el.innerHTML = `<h2>Verifying your email…</h2>`;')).toHaveLength(0);
+    expect(ruleIds('a.js', 'body.innerHTML = "<p>done</p>"')).toHaveLength(0);
+  });
+
+  it('still flags interpolation and concatenation', () => {
+    expect(ruleIds('a.js', 'el.innerHTML = `<b>Results for ${q}</b>`;')).toContain(
+      'js-unescaped-html-sink',
+    );
+    expect(ruleIds('a.js', 'el.innerHTML = "<b>Results for " + q + "</b>";')).toContain(
+      'js-unescaped-html-sink',
+    );
+  });
+
+  it('does not let a static line silence a dynamic one beside it', () => {
+    // The reason this is a line guard and not a context guard.
+    const source = ['el.innerHTML = "<hr>";', 'out.innerHTML = `<b>${req.query.q}</b>`;'].join('\n');
+    expect(ruleIds('a.js', source)).toContain('js-unescaped-html-sink');
+  });
+});
+
+describe('unescaped HTML rendering: escaper aliases', () => {
+  it('recognises a short escaper alias', () => {
+    // Real code aliases the escaper because it is called on every value;
+    // matching only `escapeHtml(` reported the codebases that escape most.
+    expect(ruleIds('a.js', 'el.innerHTML = `<b>${esc(name)}</b>`;')).toHaveLength(0);
+    expect(ruleIds('a.js', 'el.innerHTML = `<b>${aEsc(name)}</b>`;')).toHaveLength(0);
+    expect(ruleIds('a.js', 'el.innerHTML = `<b>${htmlEscape(name)}</b>`;')).toHaveLength(0);
+  });
+
+  it('still flags an unescaped interpolation', () => {
+    expect(ruleIds('a.js', 'el.innerHTML = `<b>${name}</b>`;')).toContain('js-unescaped-html-sink');
+  });
+});
+
+describe('SSRF: building a URL is not reading one', () => {
+  it('stays silent when the host is constant and only query values are set', () => {
+    const source = [
+      "const url = new URL('https://api.example.com/v1/bars');",
+      "url.searchParams.set('symbols', symbols.join(','));",
+      'const res = await fetch(url, { headers });',
+    ].join('\n');
+    expect(ruleIds('a.ts', source)).not.toContain('js-ssrf-outbound-request');
+  });
+
+  it('still flags a request whose URL comes from the caller', () => {
+    const source = [
+      'const target = req.query.url;',
+      'const res = await fetch(target);',
+    ].join('\n');
+    expect(ruleIds('a.ts', source)).toContain('js-ssrf-outbound-request');
+  });
+
+  it('treats reading searchParams as untrusted input', () => {
+    const source = [
+      'const target = new URL(req.url).searchParams.get("next");',
+      'const res = await fetch(target);',
+    ].join('\n');
+    expect(ruleIds('a.ts', source)).toContain('js-ssrf-outbound-request');
+  });
+});
+
+describe('credentials in tests', () => {
+  // Deliberately vendor-less. An earlier version of this fixture used a
+  // well-formed Stripe `sk_live_` string and GitHub push protection rejected
+  // the commit — correctly, which is a decent argument for the rule this file
+  // is testing.
+  const secret =
+    'const client = new Client({ apiKey: "' + 'a1b2c3d4' + 'e5f6a7b8c9d0e1f2a3b4c5d6" });';
+
+  it('reports a key in application code at full severity', () => {
+    const [finding] = scanText('src/client.ts', secret);
+    expect(finding).toBeDefined();
+    expect(finding?.severity).not.toBe('low');
+  });
+
+  it('reports the same key in a test, but not at a blocking severity', () => {
+    // Fixtures are the overwhelming majority, and a deliberately real-looking
+    // one is sometimes the point of the test. Still reported: a genuine key
+    // does get pasted into a test, and dropping it would hide that entirely.
+    const [finding] = scanText('test/client.test.ts', secret);
+    expect(finding).toBeDefined();
+    expect(finding?.severity).toBe('low');
+    expect(finding?.message).toContain('test file');
+  });
+
+  it('recognises the usual test layouts', () => {
+    for (const path of [
+      'test/a.test.ts',
+      'tests/a.spec.js',
+      'src/__tests__/a.ts',
+      'spec/models/a_spec.rb',
+      'pkg/thing_test.go',
+      'tests/fixtures/seed.ts',
+      'app/test_views.py',
+    ]) {
+      expect(scanText(path, secret)[0]?.severity, path).toBe('low');
+    }
+    expect(scanText('src/attestation.ts', secret)[0]?.severity).not.toBe('low');
+  });
+});
+
+describe('escaper matching does not over-reach', () => {
+  it('does not treat describe() as an escaper', () => {
+    // A looser form of the alias pattern matched `describe(`, which would have
+    // silenced every finding inside every test file in every repository.
+    const source = ['describe("thing", () => {', '  el.innerHTML = `<b>${name}</b>`;'].join('\n');
+    expect(ruleIds('a.js', source)).toContain('js-unescaped-html-sink');
+  });
+
+  it('does not treat an arbitrary identifier ending in -esc- as one', () => {
+    expect(ruleIds('a.js', 'el.innerHTML = `<b>${rescale(name)}</b>`;')).toContain(
+      'js-unescaped-html-sink',
+    );
+  });
+});

apps/cli/src/scan/code-rules.ts

@@ -68,6 +68,16 @@ export interface CodeRule {
    * `process.env`, so the generic guard would veto every true positive.
    */
   guard?: RegExp | false;
+  /**
+   * Evidence — on the matched line ONLY — that this occurrence is safe.
+   *
+   * Distinct from `guard`, which also searches the surrounding window. That
+   * breadth is right for "the value was sanitised three lines up" but wrong
+   * for properties of the line itself: a static `innerHTML` assignment says
+   * nothing about a dynamic one two lines below it, and a context-scoped
+   * guard would silently veto the dynamic one too.
+   */
+  lineGuard?: RegExp;
   /** Lines of context searched backwards for guards and required evidence. */
   guardBack?: number;
   /**
@@ -86,8 +96,15 @@ export interface CodeRule {
  * model. A real source list would be framework-aware and interprocedural,
  * which is exactly what this subsystem promises not to pretend to be.
  */
+/**
+ * `searchParams` is a read *and* a write API. `params.get('q')` is inbound
+ * data; `url.searchParams.set('limit', 50)` is an outbound URL being built,
+ * and treating the two alike marked every client of every third-party API as
+ * taking untrusted input — which is what made the SSRF rule fire on requests
+ * whose host is a compile-time constant. Only the reading half is evidence.
+ */
 const UNTRUSTED_JS =
-  /\b(?:req|request|ctx|context)\s*\.\s*(?:body|query|params|param|headers|cookies|url|files)\b|\bprocess\.argv\b|\bwindow\.location\b|\bdocument\.location\b|\blocation\.(?:search|hash|href)\b|\bsearchParams\b|\bgetParameter\s*\(|\bgetQueryString\s*\(|\bgetInputStream\s*\(/;
+  /\b(?:req|request|ctx|context)\s*\.\s*(?:body|query|params|param|headers|cookies|url|files)\b|\bprocess\.argv\b|\bwindow\.location\b|\bdocument\.location\b|\blocation\.(?:search|hash|href)\b|\bsearchParams\s*\.\s*(?:get|getAll|has|entries|keys|values|forEach)\b|\bgetParameter\s*\(|\bgetQueryString\s*\(|\bgetInputStream\s*\(/;
 
 const UNTRUSTED_PY = /\brequest\b|\bparams\b|\bflask\b|\bsys\.argv\b|\bos\.environ\b\s*\[/;
 
@@ -132,7 +149,15 @@ export function untrustedPatternFor(language: ScanLanguage): RegExp {
  * operators stop reading scanner output.
  */
 export const GENERIC_GUARD =
-  /\ballow(?:ed|list|_list|ed_hosts)?\b|\bwhitelist\b|\bescape(?:Html|Html4|Xml|Sql)?\s*\(|\bhtml_escape\b|\bhtmlspecialchars\s*\(|\bsanitiz\w*\b|\bencoded\b|\brealpath\b|\bcommonpath\b|\bresolve\(\)\.startsWith\b|\bprocess\.env\b|\bos\.environ\b|\bgetenv\b|\bENV\s*\[|setObjectInputFilter|ObjectInputFilter/i;
+  // `esc(`, `aEsc(`, `htmlEscape(`, `escapeHtml(` — the escaper is almost
+  // never *named* `escapeHtml` in real code. It gets aliased to something
+  // short because it is called on nearly every interpolation, so matching only
+  // the long spellings reported the codebases that escape most rigorously.
+  //
+  // The identifier must END at the escaper (with at most a known output-context
+  // suffix). An earlier, looser form also matched `describe(`, which would have
+  // silenced findings across every test file in every repository.
+  /\ballow(?:ed|list|_list|ed_hosts)?\b|\bwhitelist\b|\b\w{0,6}[Ee]sc(?:ape)?(?:[Hh]tml|HTML|[Xx]ml|XML|[Ss]ql|[Aa]ttr|[Jj]s|[Uu]ri|[Uu]rl)?\s*\(|\bhtml_escape\b|\bhtmlspecialchars\s*\(|\bsanitiz\w*\b|\bencoded\b|\brealpath\b|\bcommonpath\b|\bresolve\(\)\.startsWith\b|\bprocess\.env\b|\bos\.environ\b|\bgetenv\b|\bENV\s*\[|setObjectInputFilter|ObjectInputFilter/i;
 
 /** Evidence that an XML parser factory has been hardened against XXE. */
 const XXE_GUARD =
@@ -340,6 +365,17 @@ export const CODE_RULES: readonly CodeRule[] = [
     languages: ['javascript', 'typescript'],
     pattern:
       /\bdangerouslySetInnerHTML\s*=|\.\s*(?:innerHTML|outerHTML)\s*=\s*(?!\s*['"`]\s*['"`]\s*;?\s*$)|\bdocument\s*\.\s*write(?:ln)?\s*\(|\.\s*insertAdjacentHTML\s*\(/,
+    /**
+     * A whole-statement assignment of a string with no interpolation and no
+     * concatenation carries no data, so it cannot carry attacker data. This
+     * was the single largest source of noise: a codebase that builds its UI
+     * with innerHTML reports every static heading and spinner as XSS, and a
+     * rule that flags 40 safe lines to catch one real one gets switched off.
+     *
+     * Line-scoped on purpose — see `lineGuard`.
+     */
+    lineGuard:
+      /(?:innerHTML|outerHTML)\s*=\s*(?:'[^'\\]*'|"[^"\\]*"|`[^`$\\]*`)\s*;?\s*$/,
   },
   {
     id: 'java-html-writer-concatenation',
@@ -791,6 +827,8 @@ export function evaluateRule(rule: CodeRule, ctx: MatchContext): RuleMatch | nul
 
   if (rule.requires && !rule.requires.test(context)) return null;
 
+  if (rule.lineGuard?.test(line)) return null;
+
   const guard = rule.guard === undefined ? GENERIC_GUARD : rule.guard;
   if (guard && (guard.test(line) || guard.test(context))) return null;
 

apps/cli/src/scan/engine.ts

@@ -135,6 +135,26 @@ function isSuppressed(suppressions: Suppressions, index: number, ruleId: string)
   return rules.has('*') || rules.has(ruleId);
 }
 
+/**
+ * Does this path hold tests or fixtures?
+ *
+ * Used to soften credential findings, never to hide them. A secret in a test
+ * is nearly always a fixture — often a deliberately real-looking one, because
+ * the test exists to prove the real path is guarded — but "nearly always" is
+ * not "always", and a genuine key does get pasted into a test. So these are
+ * still reported, at a severity that does not block a merge, rather than
+ * dropped where nobody would ever see them.
+ */
+export function isTestPath(relativePath: string): boolean {
+  const p = relativePath.replace(/\\/g, '/');
+  return (
+    /(?:^|\/)(?:tests?|__tests__|__mocks__|spec|specs|fixtures?|mocks?|e2e|testdata)\//i.test(p) ||
+    /(?:^|\/)(?:test|conftest)_[^/]+$/i.test(p) ||
+    /[._-](?:test|spec)\.[a-z]+$/i.test(p) ||
+    /_test\.[a-z]+$/i.test(p)
+  );
+}
+
 /** Scan a single file's text. Exposed for tests and for single-file callers. */
 export function scanText(
   relativePath: string,
@@ -144,6 +164,7 @@ export function scanText(
   const findings: ScanFinding[] = [];
   const lines = text.split('\n');
   const suppressions = collectSuppressions(lines);
+  const inTests = isTestPath(relativePath);
 
   // ── Credentials ────────────────────────────────────────────────────────
   lines.forEach((line, index) => {
@@ -158,10 +179,13 @@ export function scanText(
         title: rule.name,
         file: relativePath,
         line: index + 1,
-        severity: rule.severity,
+        // Reported but not blocking in tests — see isTestPath.
+        severity: inTests ? 'low' : rule.severity,
         // A matched credential format is the finding, not a proxy for one.
         confidence: 'evidence',
-        message: `Possible ${rule.name} detected`,
+        message: inTests
+          ? `Possible ${rule.name} detected in a test file — usually a fixture, still worth confirming it is not a live credential`
+          : `Possible ${rule.name} detected`,
         consequence: rule.consequence,
         cwe: rule.cwe,
         excerpt: redactSecret(line.trim()).slice(0, 200),