diff --git a/apps/cli/package.json b/apps/cli/package.json index 4210f65..a8067bb 100644 --- a/apps/cli/package.json +++ b/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" }, diff --git a/apps/cli/src/scan/__tests__/code-rules.test.ts b/apps/cli/src/scan/__tests__/code-rules.test.ts index 36f7344..e604152 100644 --- a/apps/cli/src/scan/__tests__/code-rules.test.ts +++ b/apps/cli/src/scan/__tests__/code-rules.test.ts @@ -184,3 +184,131 @@ describe('confidence', () => { 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 = '
';`)).toHaveLength(0); + expect(ruleIds('a.js', 'el.innerHTML = `

Verifying your email…

`;')).toHaveLength(0); + expect(ruleIds('a.js', 'body.innerHTML = "

done

"')).toHaveLength(0); + }); + + it('still flags interpolation and concatenation', () => { + expect(ruleIds('a.js', 'el.innerHTML = `Results for ${q}`;')).toContain( + 'js-unescaped-html-sink', + ); + expect(ruleIds('a.js', 'el.innerHTML = "Results for " + q + "";')).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 = "
";', 'out.innerHTML = `${req.query.q}`;'].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 = `${esc(name)}`;')).toHaveLength(0); + expect(ruleIds('a.js', 'el.innerHTML = `${aEsc(name)}`;')).toHaveLength(0); + expect(ruleIds('a.js', 'el.innerHTML = `${htmlEscape(name)}`;')).toHaveLength(0); + }); + + it('still flags an unescaped interpolation', () => { + expect(ruleIds('a.js', 'el.innerHTML = `${name}`;')).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 = `${name}`;'].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 = `${rescale(name)}`;')).toContain( + 'js-unescaped-html-sink', + ); + }); +}); diff --git a/apps/cli/src/scan/code-rules.ts b/apps/cli/src/scan/code-rules.ts index ed95c1e..a244c62 100644 --- a/apps/cli/src/scan/code-rules.ts +++ b/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; diff --git a/apps/cli/src/scan/engine.ts b/apps/cli/src/scan/engine.ts index 0cf5737..7ff5efe 100644 --- a/apps/cli/src/scan/engine.ts +++ b/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),