diff --git a/README.md b/README.md index 457f288..6e9afd2 100644 --- a/README.md +++ b/README.md @@ -50,6 +50,8 @@ Automatic detection of: - Dangerous `eval()` usage - Debug statements in production code +Test files, fixtures, `docs/`, and common tooling directories (`.github/`, `.claude/`, `scripts/`) are excluded from the XSS/shell-execution checks, since findings there don't reflect the shipped product's attack surface. The hardcoded-secret check exempts only tests, fixtures, and docs: CI workflows, hooks, and deploy scripts are executable code, so a real credential there is still a real leak. + ### Pattern Detection Automatically identifies: - Singleton patterns diff --git a/docs/superpowers/plans/2026-07-11-security-scanner-precision.md b/docs/superpowers/plans/2026-07-11-security-scanner-precision.md new file mode 100644 index 0000000..a47c041 --- /dev/null +++ b/docs/superpowers/plans/2026-07-11-security-scanner-precision.md @@ -0,0 +1,1062 @@ +# Security & Duplicate-Detector Precision Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Eliminate the false-positive classes in CodeFlow's `detectSecurity`/`detectDuplicates` analyzer (embedded in `index.html`) that were documented by running the tool against an external project, without adding dependencies or building full taint analysis. + +**Architecture:** All changes live inside the single analyzer block in `index.html` (`CODEFLOW_ANALYZER_START`…`END`). A new shared classifier `isNonProductionPath()` generalizes two classifiers that already exist for a different feature (`isArchitectureTestFile`, `isArchitectureFixtureFile`) and wires them into `detectSecurity`/`detectDuplicates`. Six further precision fixes are layered on top, each independently testable via the existing golden-fixture test pattern. + +**Tech Stack:** Vanilla JS (no build step, no framework, no new dependencies). Tests run with Node's built-in `node:test` runner via `node --test`. + +## Global Constraints + +- No new npm dependencies. No build step exists for `index.html` and none is being introduced. +- Every change lives inside the existing `CODEFLOW_ANALYZER_START`/`END` block in `index.html`, matching the file's existing code style (`var`, no semicolonless ASI reliance, single-line `if` bodies as already used throughout this file). +- `tests/codeflow-golden.test.mjs` must stay green, unmodified, after every task (non-regression gate). +- Each task's own new/updated test(s) in `tests/security-precision.test.mjs` must pass before moving to the next task. +- Every regex/logic change in this plan has already been verified against real reproductions of the false positives (see each task's "Verified behavior" note) — implementers should not need to re-derive the regexes, only transcribe them. +- Fixture credential-like values must be obviously synthetic so they don't trip GitHub's own scanning once pushed. +- Out of scope (do not implement): full data-flow/taint analysis, `commonNames` entries for frameworks other than Next.js App Router, any change to `Debug Statements` beyond the backend/frontend severity split, any UI/packaging/documentation work beyond the README's "Security Scanner" section. + +--- + +## File Structure + +- `index.html` — modify in place (analyzer block only): new `isNonProductionPath()` function; `detectSecurity` rule guards/regex tightening; `detectDuplicates` (`Parser.detectDuplicates`) allowlist + path guard. +- `tests/fixtures/security-precision-world/` — new fixture tree, one false-positive/true-positive file pair added per task. +- `tests/security-precision.test.mjs` — new test file, created in Task 1, one `test()` appended per task. +- `README.md` — one paragraph added to the "Security Scanner" section (Task 8). + +--- + +### Task 1: Shared file classifier + Hardcoded Secret fix + +**Files:** + +- Modify: `index.html:3288-3292` (insert `isNonProductionPath` after `isArchitectureFixtureFile`) +- Modify: `index.html:1484-1486` (gate the Hardcoded Secret rule) +- Create: `tests/fixtures/security-precision-world/test/client-ip.test.ts` +- Create: `tests/fixtures/security-precision-world/lib/auth.ts` +- Create: `tests/security-precision.test.mjs` + +**Interfaces:** + +- Produces: global function `isNonProductionPath(path: string): boolean` — returns `true` for test/fixture/tooling/docs/markdown paths. Declared as a top-level `function` statement (hoisted, same pattern as the existing `getSecurityScanContent`/`isSanitizedPreviewRenderer` globals that `Parser.detectSecurity` already calls), so it is callable from inside the `Parser` object literal without import. +- Produces: `tests/security-precision.test.mjs` exposes no exports — it is a standalone `node:test` file following the same in-file harness pattern as `tests/codeflow-golden.test.mjs`. Later tasks append `test(...)` blocks to this same file and add fixtures under `tests/fixtures/security-precision-world/`. + +- [ ] **Step 1: Add the fixture pair** + +Create `tests/fixtures/security-precision-world/test/client-ip.test.ts`: + +```ts +const AUTH_SECRET = "stub-auth-secret-for-tests"; + +export function resolveClientIp(headers: Record) { + if (headers["x-forwarded-for"]) return headers["x-forwarded-for"]; + return AUTH_SECRET ? "test-mode" : "unknown"; +} +``` + +Create `tests/fixtures/security-precision-world/lib/auth.ts`: + +```ts +export const AUTH_SECRET = "hardcoded-example-not-a-real-credential-9f8e7d6c"; + +export function getAuthSecret() { + return AUTH_SECRET; +} +``` + +- [ ] **Step 2: Write the test harness and the failing test** + +Create `tests/security-precision.test.mjs`: + +```js +import assert from 'node:assert/strict'; +import { readdir, readFile } from 'node:fs/promises'; +import { basename, join, relative } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import test from 'node:test'; +import vm from 'node:vm'; + +const __dirname = fileURLToPath(new URL('.', import.meta.url)); +const repoRoot = join(__dirname, '..'); +const htmlSource = await readFile(join(repoRoot, 'index.html'), 'utf8'); +const startMarker = '// ===== CODEFLOW_ANALYZER_START ====='; +const endMarker = '// ===== CODEFLOW_ANALYZER_END ====='; +const parserStart = htmlSource.indexOf(startMarker); +const parserEnd = htmlSource.indexOf(endMarker, parserStart); + +if (parserStart < 0 || parserEnd < 0) { + throw new Error('Could not locate analyzer source in index.html'); +} + +const context = { + console, + TreeSitter: undefined, + Babel: undefined, + acorn: undefined, + getSecurityScanContent(file) { + return file && file.content ? file.content : ''; + }, + isSanitizedPreviewRenderer() { + return false; + }, +}; + +vm.createContext(context); +vm.runInContext( + `${htmlSource.slice(parserStart, parserEnd)}\nthis.Parser = Parser; this.buildAnalysisData = buildAnalysisData;`, + context +); + +const { Parser, buildAnalysisData } = context; + +async function collectFixtureFiles(root) { + const files = []; + + async function walk(dir) { + const entries = await readdir(dir, { withFileTypes: true }); + for (const entry of entries) { + const fullPath = join(dir, entry.name); + if (entry.isDirectory()) { + await walk(fullPath); + continue; + } + if (!entry.isFile() || !Parser.isIncluded(entry.name)) continue; + const repoPath = relative(root, fullPath).replace(/\\/g, '/'); + files.push({ + fullPath, + path: repoPath, + name: basename(repoPath), + folder: repoPath.includes('/') ? repoPath.slice(0, repoPath.lastIndexOf('/')) : 'root', + isCode: Parser.isCode(entry.name), + }); + } + } + + await walk(root); + return files.sort((a, b) => a.path.localeCompare(b.path)); +} + +async function analyzeFixture(name) { + const root = join(__dirname, 'fixtures', name); + const files = await collectFixtureFiles(root); + const analyzed = []; + const allFns = []; + + for (const file of files) { + const content = await readFile(file.fullPath, 'utf8'); + const layer = Parser.detectLayer(file.path); + const actualIsCode = file.isCode !== false && (!Parser.isScriptContainer(file.path) || Parser.hasEmbeddedCode(content, file.path)); + const functions = actualIsCode ? Parser.extract(content, file.path) : []; + analyzed.push({ + path: file.path, + name: file.name, + folder: file.folder, + content, + functions, + lines: content ? content.split('\n').length : 0, + layer, + churn: 0, + isCode: actualIsCode, + }); + if (actualIsCode) { + functions.forEach((fn) => allFns.push(Object.assign({}, fn, { folder: file.folder, layer }))); + } + } + + return buildAnalysisData({ + analyzed, + allFns, + excludePatterns: [], + progress() {}, + yieldFn: async () => {}, + }); +} + +test('Hardcoded Secret rule excludes test stubs, keeps real hits', async () => { + const data = await analyzeFixture('security-precision-world'); + const flaggedPaths = data.securityIssues + .filter((i) => i.title === 'Hardcoded Secret') + .map((i) => i.path); + + assert.equal(flaggedPaths.includes('test/client-ip.test.ts'), false); + assert.equal(flaggedPaths.includes('lib/auth.ts'), true); +}); +``` + +- [ ] **Step 3: Run the test to verify it fails** + +Run: `node --test tests/security-precision.test.mjs` +Expected: FAIL — `flaggedPaths.includes('test/client-ip.test.ts')` is `true` (both files are currently flagged). + +- [ ] **Step 4: Add the shared classifier** + +In `index.html`, find: + +```js +function isArchitectureFixtureFile(path){ + var p=String(path||'').toLowerCase().replace(/\\/g,'/'); + return /(^|\/)fixtures(\/|$)/.test(p)||/(^|\/)__fixtures__(\/|$)/.test(p); +} + +function isArchitectureBarrelIndex(path){ +``` + +Replace with: + +```js +function isArchitectureFixtureFile(path){ + var p=String(path||'').toLowerCase().replace(/\\/g,'/'); + return /(^|\/)fixtures(\/|$)/.test(p)||/(^|\/)__fixtures__(\/|$)/.test(p); +} + +function isNonProductionPath(path){ + var p=String(path||'').toLowerCase().replace(/\\/g,'/'); + if(isArchitectureTestFile(p))return true; + if(isArchitectureFixtureFile(p))return true; + if(/(^|\/)\.github(\/|$)/.test(p))return true; + if(/(^|\/)\.claude(\/|$)/.test(p))return true; + if(/(^|\/)(scripts|tools|tooling)(\/|$)/.test(p))return true; + if(/(^|\/)docs?(\/|$)/.test(p))return true; + if(/\.(md|markdown|mdx)$/.test(p))return true; + return false; +} + +function isArchitectureBarrelIndex(path){ +``` + +**Verified behavior:** `isArchitectureTestFile`/`isArchitectureFixtureFile` are plain top-level `function` declarations defined later in the same script scope (~line 3283); JS function-declaration hoisting makes them callable from `isNonProductionPath` regardless of source order, exactly like `Parser.detectSecurity` already calls the top-level `getSecurityScanContent`/`isSanitizedPreviewRenderer` globals declared elsewhere in the file. + +- [ ] **Step 5: Gate the Hardcoded Secret rule** + +In `index.html`, inside `detectSecurity:function(files){`, find: + +```js + lines.forEach(function(line,idx){ + if(line.match(/(?:password|passwd|pwd|secret|api_key|apikey|token|auth)\s*[=:]\s*['"][^'"]{4,}['"]/i)&&!line.includes('process.env')&&!line.includes('config.')){ + issues.push({severity:'high',title:'Hardcoded Secret',file:f.name,path:f.path,line:idx+1,desc:'Credentials should never be hardcoded. Use environment variables or a secrets manager.',code:line.trim().substring(0,80)}); + } + }); +``` + +Replace with: + +```js + lines.forEach(function(line,idx){ + if(!isNonProductionPath(f.path)&&line.match(/(?:password|passwd|pwd|secret|api_key|apikey|token|auth)\s*[=:]\s*['"][^'"]{4,}['"]/i)&&!line.includes('process.env')&&!line.includes('config.')){ + issues.push({severity:'high',title:'Hardcoded Secret',file:f.name,path:f.path,line:idx+1,desc:'Credentials should never be hardcoded. Use environment variables or a secrets manager.',code:line.trim().substring(0,80)}); + } + }); +``` + +- [ ] **Step 6: Run the test to verify it passes** + +Run: `node --test tests/security-precision.test.mjs` +Expected: PASS + +- [ ] **Step 7: Run the non-regression suite** + +Run: `node --test tests/codeflow-golden.test.mjs` +Expected: PASS, unchanged (1 test, same assertions as before this task). + +- [ ] **Step 8: Commit** + +```bash +git add index.html tests/security-precision.test.mjs tests/fixtures/security-precision-world/test/client-ip.test.ts tests/fixtures/security-precision-world/lib/auth.ts +git commit -m "fix(security): exclude test/tooling paths from Hardcoded Secret rule" +``` + +--- + +### Task 2: Command/Shell Execution — path guard + `.exec(` word-boundary fix + +**Files:** + +- Modify: `index.html:1505-1506` (JS Command Execution rule) +- Modify: `index.html:1518-1519` (VBA Shell Command Execution rule) +- Modify: `index.html:1554-1555` (Python `subprocess shell=True` rule) +- Create: `tests/fixtures/security-precision-world/.claude/hooks/pre-commit.py` +- Create: `tests/fixtures/security-precision-world/api/import.py` +- Create: `tests/fixtures/security-precision-world/lib/search.ts` +- Create: `tests/fixtures/security-precision-world/lib/runner.ts` +- Modify: `tests/security-precision.test.mjs` (append two `test()` blocks) + +**Interfaces:** + +- Consumes: `isNonProductionPath(path)` from Task 1. + +**Verified behavior (checked against the exact regexes below, run through the real analyzer):** + +- `api/import.py` and `lib/runner.ts` (production code) stay flagged. +- `.claude/hooks/pre-commit.py` (dev tooling) and `lib/search.ts` (uses `regex.exec()`, unrelated to shell) are no longer flagged. + +- [ ] **Step 1: Add the fixture files** + +Create `tests/fixtures/security-precision-world/.claude/hooks/pre-commit.py`: + +```py +import subprocess + + +def run(cmd): + subprocess.run(cmd, shell=True) +``` + +Create `tests/fixtures/security-precision-world/api/import.py`: + +```py +import subprocess + + +def run(cmd): + subprocess.run(cmd, shell=True) +``` + +Create `tests/fixtures/security-precision-world/lib/search.ts`: + +```ts +export function find(items: string[], re: RegExp) { + return items.filter((i) => re.exec(i)); +} +``` + +Create `tests/fixtures/security-precision-world/lib/runner.ts`: + +```ts +import child_process from 'child_process'; + +export function run(userInput: string) { + return child_process.exec(userInput); +} +``` + +- [ ] **Step 2: Append the failing tests** + +Append to `tests/security-precision.test.mjs`: + +```js +test('Shell Injection Risk rule excludes dev tooling, keeps real hits', async () => { + const data = await analyzeFixture('security-precision-world'); + const flaggedPaths = data.securityIssues + .filter((i) => i.title === 'Shell Injection Risk') + .map((i) => i.path); + + assert.equal(flaggedPaths.includes('.claude/hooks/pre-commit.py'), false); + assert.equal(flaggedPaths.includes('api/import.py'), true); +}); + +test('Command Execution rule excludes regex.exec(), keeps child_process.exec()', async () => { + const data = await analyzeFixture('security-precision-world'); + const flaggedPaths = data.securityIssues + .filter((i) => i.title === 'Command Execution') + .map((i) => i.path); + + assert.equal(flaggedPaths.includes('lib/search.ts'), false); + assert.equal(flaggedPaths.includes('lib/runner.ts'), true); +}); +``` + +- [ ] **Step 3: Run the tests to verify they fail** + +Run: `node --test tests/security-precision.test.mjs` +Expected: both new tests FAIL — `.claude/hooks/pre-commit.py` is currently flagged, and `lib/search.ts` (`regex.exec(...)`) is currently flagged as Command Execution. + +- [ ] **Step 4: Fix the JS Command Execution rule** + +In `index.html`, inside `detectSecurity:function(files){`, find: + +```js + if(scanContent.match(/\.exec\s*\(/)||scanContent.match(/child_process/)){ + issues.push({severity:'medium',title:'Command Execution',file:f.name,path:f.path,desc:'Shell command execution detected. Ensure input is sanitized to prevent injection.',code:''}); + } +``` + +Replace with: + +```js + var execMatch=scanContent.match(/(?:child_process|cp)\.\w*[Ee]xec\w*\s*\(/)||scanContent.match(/require\(\s*['"]child_process['"]\s*\)/)||scanContent.match(/from\s+['"]child_process['"]/); + if(!isNonProductionPath(f.path)&&execMatch){ + issues.push({severity:'medium',title:'Command Execution',file:f.name,path:f.path,desc:'Shell command execution detected. Ensure input is sanitized to prevent injection.',code:''}); + } +``` + +- [ ] **Step 5: Fix the VBA Shell Command Execution rule** + +In `index.html`, find: + +```js + if(scanContent.match(/Shell\s*\(/i)){ + issues.push({severity:'high',title:'Shell Command Execution',file:f.name,path:f.path,desc:'Shell() executes system commands. Ensure input is validated.',code:''}); + } +``` + +Replace with: + +```js + if(!isNonProductionPath(f.path)&&scanContent.match(/Shell\s*\(/i)){ + issues.push({severity:'high',title:'Shell Command Execution',file:f.name,path:f.path,desc:'Shell() executes system commands. Ensure input is validated.',code:''}); + } +``` + +- [ ] **Step 6: Fix the Python `subprocess shell=True` rule** + +In `index.html`, find: + +```js + // subprocess with shell=True + if(scanContent.match(/subprocess\.\w+\([^)]*shell\s*=\s*True/)){ + issues.push({severity:'high',title:'Shell Injection Risk',file:f.name,path:f.path,desc:'subprocess with shell=True is vulnerable to command injection. Use shell=False with a list of args.',code:''}); + } +``` + +Replace with: + +```js + // subprocess with shell=True + if(!isNonProductionPath(f.path)&&scanContent.match(/subprocess\.\w+\([^)]*shell\s*=\s*True/)){ + issues.push({severity:'high',title:'Shell Injection Risk',file:f.name,path:f.path,desc:'subprocess with shell=True is vulnerable to command injection. Use shell=False with a list of args.',code:''}); + } +``` + +- [ ] **Step 7: Run the tests to verify they pass** + +Run: `node --test tests/security-precision.test.mjs` +Expected: PASS (3 tests total: Task 1's + these 2) + +- [ ] **Step 8: Run the non-regression suite** + +Run: `node --test tests/codeflow-golden.test.mjs` +Expected: PASS, unchanged. + +- [ ] **Step 9: Commit** + +```bash +git add index.html tests/security-precision.test.mjs tests/fixtures/security-precision-world/.claude tests/fixtures/security-precision-world/api tests/fixtures/security-precision-world/lib/search.ts tests/fixtures/security-precision-world/lib/runner.ts +git commit -m "fix(security): scope Command/Shell Execution rules to product paths and real exec calls" +``` + +--- + +### Task 3: SQL Injection Risk — scope to code files, anchor to a DB-call receiver + +**Files:** + +- Modify: `index.html:1488-1490` +- Create: `tests/fixtures/security-precision-world/docs/decisions.md` +- Create: `tests/fixtures/security-precision-world/lib/db.ts` +- Modify: `tests/security-precision.test.mjs` (append one `test()` block) + +**Verified behavior:** the current rule's third alternative (`/\$\{.*\}.*(?:SELECT|INSERT|UPDATE|DELETE)/i`) requires no DB-call receiver at all, so it also matches prose containing both a `${...}`-shaped token and an English SQL-ish word on the same line (confirmed against `docs/decisions.md` below). It is also **order-dependent** — `SELECT ... ${id}` (keyword before the interpolation, the realistic case) does not match it at all, so `lib/db.ts` below is a real SQL-injection pattern the current rule silently misses. The replacement fixes both: it requires a `query`/`execute`/`raw` call whose argument list contains both a SQL keyword and a `${` interpolation, in either order. + +- [ ] **Step 1: Add the fixture pair** + +Create `tests/fixtures/security-precision-world/docs/decisions.md`: + +```md +# Decisions actees 2026-07-10 + +Voir ${CHANGELOG} pour la mise a jour complete (update log). +``` + +Create `tests/fixtures/security-precision-world/lib/db.ts`: + +```ts +export function findUser(id: string) { + return db.query(`SELECT * FROM users WHERE id = ${id}`); +} +``` + +- [ ] **Step 2: Append the failing test** + +Append to `tests/security-precision.test.mjs`: + +```js +test('SQL Injection Risk rule excludes markdown prose, keeps real template injection', async () => { + const data = await analyzeFixture('security-precision-world'); + const flaggedPaths = data.securityIssues + .filter((i) => i.title === 'SQL Injection Risk') + .map((i) => i.path); + + assert.equal(flaggedPaths.includes('docs/decisions.md'), false); + assert.equal(flaggedPaths.includes('lib/db.ts'), true); +}); +``` + +- [ ] **Step 3: Run the test to verify it fails** + +Run: `node --test tests/security-precision.test.mjs` +Expected: FAIL — `docs/decisions.md` is currently flagged and `lib/db.ts` currently is not (both wrong, in opposite directions). + +- [ ] **Step 4: Fix the rule** + +In `index.html`, inside `detectSecurity:function(files){`, find: + +```js + if(scanContent.match(/query\s*\(\s*['"`][^'"`]*\s*\+/)||scanContent.match(/execute\s*\(\s*['"`][^'"`]*\$\{/)||scanContent.match(/\$\{.*\}.*(?:SELECT|INSERT|UPDATE|DELETE)/i)){ + var m=scanContent.match(/.*(query|execute|SELECT|INSERT|UPDATE|DELETE).*(\+|\$\{).*/i); + issues.push({severity:'high',title:'SQL Injection Risk',file:f.name,path:f.path,desc:'String concatenation in SQL queries. Use parameterized queries instead.',code:m?m[0].trim().substring(0,80):''}); + } +``` + +Replace with: + +```js + var dbCallMatch=scanContent.match(/\b(?:query|execute|raw)\s*\(([^)]*)\)/i); + var hasSqlConcat=scanContent.match(/query\s*\(\s*['"`][^'"`]*\s*\+/)||scanContent.match(/execute\s*\(\s*['"`][^'"`]*\$\{/); + var hasSqlTemplateInjection=dbCallMatch&&/\$\{/.test(dbCallMatch[1])&&/(?:SELECT|INSERT|UPDATE|DELETE)/i.test(dbCallMatch[1]); + if(f.isCode&&(hasSqlConcat||hasSqlTemplateInjection)){ + var m=scanContent.match(/.*(query|execute|SELECT|INSERT|UPDATE|DELETE).*(\+|\$\{).*/i); + issues.push({severity:'high',title:'SQL Injection Risk',file:f.name,path:f.path,desc:'String concatenation in SQL queries. Use parameterized queries instead.',code:m?m[0].trim().substring(0,80):''}); + } +``` + +- [ ] **Step 5: Run the test to verify it passes** + +Run: `node --test tests/security-precision.test.mjs` +Expected: PASS (4 tests total) + +- [ ] **Step 6: Run the non-regression suite** + +Run: `node --test tests/codeflow-golden.test.mjs` +Expected: PASS, unchanged. + +- [ ] **Step 7: Commit** + +```bash +git add index.html tests/security-precision.test.mjs tests/fixtures/security-precision-world/docs tests/fixtures/security-precision-world/lib/db.ts +git commit -m "fix(security): anchor SQL Injection Risk to a real DB-call receiver" +``` + +--- + +### Task 4: XSS Vulnerability — skip literal-only `dangerouslySetInnerHTML` + +**Files:** + +- Modify: `index.html:1492-1496` +- Create: `tests/fixtures/security-precision-world/app/page.tsx` +- Create: `tests/fixtures/security-precision-world/app/profile-card.tsx` +- Modify: `tests/security-precision.test.mjs` (append one `test()` block) + +**Verified behavior:** the extraction regex `dangerouslySetInnerHTML\s*[:=]\s*\{\{?\s*__html\s*:\s*([^}]+)\}` matches both the JSX-attribute form (`dangerouslySetInnerHTML={{__html: ...}}`) and the object-literal form (`dangerouslySetInnerHTML:{__html:...}`, used by CodeFlow's own preview renderer). Confirmed against real fixture content that a pure quoted-literal capture is correctly classified as safe, and an identifier capture (`rawUserBio`) is not. + +- [ ] **Step 1: Add the fixture pair** + +Create `tests/fixtures/security-precision-world/app/page.tsx`: + +```tsx +export function LandingCopy() { + return ( +

5 a 30x moins cher que vos outils actuels" }} /> + ); +} +``` + +Create `tests/fixtures/security-precision-world/app/profile-card.tsx`: + +```tsx +export function ProfileCard({ rawUserBio }: { rawUserBio: string }) { + return

; +} +``` + +- [ ] **Step 2: Append the failing test** + +Append to `tests/security-precision.test.mjs`: + +```js +test('XSS Vulnerability rule excludes static literals, keeps variable interpolation', async () => { + const data = await analyzeFixture('security-precision-world'); + const flaggedPaths = data.securityIssues + .filter((i) => i.title === 'XSS Vulnerability') + .map((i) => i.path); + + assert.equal(flaggedPaths.includes('app/page.tsx'), false); + assert.equal(flaggedPaths.includes('app/profile-card.tsx'), true); +}); +``` + +- [ ] **Step 3: Run the test to verify it fails** + +Run: `node --test tests/security-precision.test.mjs` +Expected: FAIL — both files are currently flagged. + +- [ ] **Step 4: Fix the rule** + +In `index.html`, inside `detectSecurity:function(files){`, find: + +```js + var hasInnerHtmlAssignment=scanContent.match(/innerHTML\s*=/); + var hasDangerousHtmlRender=scanContent.match(/dangerouslySetInnerHTML/); + var isSafePreviewRender=!hasInnerHtmlAssignment&&hasDangerousHtmlRender&&isSanitizedPreviewRenderer(f.content||''); + if((hasInnerHtmlAssignment||hasDangerousHtmlRender)&&!isSafePreviewRender){ + issues.push({severity:'high',title:'XSS Vulnerability',file:f.name,path:f.path,desc:'Direct HTML injection can lead to XSS attacks. Sanitize user input.',code:''}); + } +``` + +Replace with: + +```js + var hasInnerHtmlAssignment=scanContent.match(/innerHTML\s*=/); + var hasDangerousHtmlRender=scanContent.match(/dangerouslySetInnerHTML/); + var isSafePreviewRender=!hasInnerHtmlAssignment&&hasDangerousHtmlRender&&isSanitizedPreviewRenderer(f.content||''); + var htmlValueMatch=scanContent.match(/dangerouslySetInnerHTML\s*[:=]\s*\{\{?\s*__html\s*:\s*([^}]+)\}/); + var isLiteralOnlyHtml=!!(htmlValueMatch&&/^(['"`])(?:(?!\1)[\s\S])*\1$/.test(htmlValueMatch[1].trim())); + var isSafeStaticHtml=!hasInnerHtmlAssignment&&hasDangerousHtmlRender&&isLiteralOnlyHtml; + if((hasInnerHtmlAssignment||hasDangerousHtmlRender)&&!isSafePreviewRender&&!isSafeStaticHtml){ + issues.push({severity:'high',title:'XSS Vulnerability',file:f.name,path:f.path,desc:'Direct HTML injection can lead to XSS attacks. Sanitize user input.',code:''}); + } +``` + +- [ ] **Step 5: Run the test to verify it passes** + +Run: `node --test tests/security-precision.test.mjs` +Expected: PASS (5 tests total) + +- [ ] **Step 6: Run the non-regression suite** + +Run: `node --test tests/codeflow-golden.test.mjs` +Expected: PASS, unchanged. + +- [ ] **Step 7: Commit** + +```bash +git add index.html tests/security-precision.test.mjs tests/fixtures/security-precision-world/app +git commit -m "fix(security): skip XSS Vulnerability for literal-only dangerouslySetInnerHTML" +``` + +--- + +### Task 5: Function Constructor — remove unanchored substring match + +**Files:** + +- Modify: `index.html:1502-1504` +- Create: `tests/fixtures/security-precision-world/lib/csp.ts` +- Create: `tests/fixtures/security-precision-world/lib/dynamic.ts` +- Modify: `tests/security-precision.test.mjs` (append one `test()` block) + +- [ ] **Step 1: Add the fixture pair** + +Create `tests/fixtures/security-precision-world/lib/csp.ts`: + +```ts +// NOTE: getFunction(x) helper lives in utils/reflection.ts - unrelated to eval. +export const CSP_HEADER = "script-src 'self'"; +``` + +Create `tests/fixtures/security-precision-world/lib/dynamic.ts`: + +```ts +export function buildHandler(userCode: string) { + return new Function(userCode); +} +``` + +- [ ] **Step 2: Append the failing test** + +Append to `tests/security-precision.test.mjs`: + +```js +test('Function Constructor rule excludes substring mentions, keeps real constructor calls', async () => { + const data = await analyzeFixture('security-precision-world'); + const flaggedPaths = data.securityIssues + .filter((i) => i.title === 'Function Constructor') + .map((i) => i.path); + + assert.equal(flaggedPaths.includes('lib/csp.ts'), false); + assert.equal(flaggedPaths.includes('lib/dynamic.ts'), true); +}); +``` + +- [ ] **Step 3: Run the test to verify it fails** + +Run: `node --test tests/security-precision.test.mjs` +Expected: FAIL — `lib/csp.ts` is currently flagged (the comment contains the substring `Function(`). + +- [ ] **Step 4: Fix the rule** + +In `index.html`, inside `detectSecurity:function(files){`, find: + +```js + if(scanContent.includes('Function(')||scanContent.match(/new\s+Function\s*\(/)){ + issues.push({severity:'medium',title:'Function Constructor',file:f.name,path:f.path,desc:'Function constructor is similar to eval(). Consider alternatives.',code:''}); + } +``` + +Replace with: + +```js + if(scanContent.match(/\bnew\s+Function\s*\(/)){ + issues.push({severity:'medium',title:'Function Constructor',file:f.name,path:f.path,desc:'Function constructor is similar to eval(). Consider alternatives.',code:''}); + } +``` + +- [ ] **Step 5: Run the test to verify it passes** + +Run: `node --test tests/security-precision.test.mjs` +Expected: PASS (6 tests total) + +- [ ] **Step 6: Run the non-regression suite** + +Run: `node --test tests/codeflow-golden.test.mjs` +Expected: PASS, unchanged. + +- [ ] **Step 7: Commit** + +```bash +git add index.html tests/security-precision.test.mjs tests/fixtures/security-precision-world/lib/csp.ts tests/fixtures/security-precision-world/lib/dynamic.ts +git commit -m "fix(security): anchor Function Constructor rule to an actual constructor call" +``` + +--- + +### Task 6: Debug Statements — downgrade severity for server-only code + +**Files:** + +- Modify: `index.html:1508-1512` +- Create: `tests/fixtures/security-precision-world/server/logger.ts` +- Create: `tests/fixtures/security-precision-world/components/dashboard.tsx` +- Modify: `tests/security-precision.test.mjs` (append one `test()` block) + +**Interfaces:** + +- Consumes: `isArchitectureBackendPath(path)`, an existing classifier at `index.html:3302` (not modified by this task — only newly consumed by `detectSecurity`). + +- [ ] **Step 1: Add the fixture pair** + +Create `tests/fixtures/security-precision-world/server/logger.ts`: + +```ts +export function bootServer() { + console.log('booting server'); + console.log('loading config'); + console.info('config loaded'); + console.debug('ready to accept connections'); +} +``` + +Create `tests/fixtures/security-precision-world/components/dashboard.tsx`: + +```tsx +export function Dashboard() { + console.log('render start'); + console.log('data', 1); + console.info('mounted'); + console.debug('layout computed'); + return null; +} +``` + +- [ ] **Step 2: Append the failing test** + +Append to `tests/security-precision.test.mjs`: + +```js +test('Debug Statements rule downgrades server-only code, keeps client code at low', async () => { + const data = await analyzeFixture('security-precision-world'); + const serverIssue = data.securityIssues.find((i) => i.title === 'Debug Statements' && i.path === 'server/logger.ts'); + const clientIssue = data.securityIssues.find((i) => i.title === 'Debug Statements' && i.path === 'components/dashboard.tsx'); + + assert.equal(serverIssue.severity, 'info'); + assert.equal(clientIssue.severity, 'low'); +}); +``` + +- [ ] **Step 3: Run the test to verify it fails** + +Run: `node --test tests/security-precision.test.mjs` +Expected: FAIL — `serverIssue.severity` is currently `'low'`, not `'info'`. + +- [ ] **Step 4: Fix the rule** + +In `index.html`, inside `detectSecurity:function(files){`, find: + +```js + if(scanContent.match(/console\.(log|debug|info)\(/)){ + var consoleCount=(scanContent.match(/console\.(log|debug|info)\(/g)||[]).length; + if(consoleCount>3){ + issues.push({severity:'low',title:'Debug Statements',file:f.name,path:f.path,desc:consoleCount+' console statements found. Remove before production.',code:''}); + } + } +``` + +Replace with: + +```js + if(scanContent.match(/console\.(log|debug|info)\(/)){ + var consoleCount=(scanContent.match(/console\.(log|debug|info)\(/g)||[]).length; + if(consoleCount>3){ + var debugSeverity=isArchitectureBackendPath(f.path)?'info':'low'; + issues.push({severity:debugSeverity,title:'Debug Statements',file:f.name,path:f.path,desc:consoleCount+' console statements found. Remove before production.',code:''}); + } + } +``` + +- [ ] **Step 5: Run the test to verify it passes** + +Run: `node --test tests/security-precision.test.mjs` +Expected: PASS (7 tests total) + +- [ ] **Step 6: Run the non-regression suite** + +Run: `node --test tests/codeflow-golden.test.mjs` +Expected: PASS, unchanged. + +- [ ] **Step 7: Commit** + +```bash +git add index.html tests/security-precision.test.mjs tests/fixtures/security-precision-world/server tests/fixtures/security-precision-world/components +git commit -m "fix(security): downgrade Debug Statements severity for server-only code" +``` + +--- + +### Task 7: Duplicate function names — Next.js route-handler allowlist + path guard + +**Files:** + +- Modify: `index.html:1178-1211` (add Next.js names to `commonNames`) +- Modify: `index.html:1213-1231` (skip non-production paths in the name-duplicate grouping) +- Modify: `index.html:1259-1271` (skip non-production paths in the code-duplicate grouping) +- Create: `tests/fixtures/security-precision-world/app/api/users/route.ts` +- Create: `tests/fixtures/security-precision-world/app/api/orders/route.ts` +- Create: `tests/fixtures/security-precision-world/app/api/invoices/route.ts` +- Modify: `tests/security-precision.test.mjs` (append one `test()` block) + +**Interfaces:** + +- Consumes: `isNonProductionPath(path)` from Task 1. + +**Verified behavior:** confirmed directly against `Parser.detectDuplicates` that three files each exporting a byte-identical `POST` function currently produce both a `type:'name'` duplicate (message: "Function \"POST\" appears in 3 files... consider consolidating") and a `type:'code'` duplicate (message: "Similar code blocks... consider extracting to a shared utility"). The `'name'` type is the false positive under investigation — it treats a framework-mandated export name as a naming collision. The `'code'` type is a separate, still-legitimate signal (genuinely identical bodies are worth extracting regardless of what they're named) and is **intentionally left untouched** by the `commonNames` addition — only the path-guard change in this task affects it, and only for paths under test/fixture/tooling/docs directories. This task's test asserts only on the `'name'` type for that reason. + +- [ ] **Step 1: Add the fixture files** + +Create `tests/fixtures/security-precision-world/app/api/users/route.ts`: + +```ts +import { NextResponse } from 'next/server'; + +export async function POST(request: Request) { + const body = await request.json(); + if (!body.email) { + return NextResponse.json({ error: 'email required' }, { status: 400 }); + } + return NextResponse.json({ ok: true }); +} +``` + +Create `tests/fixtures/security-precision-world/app/api/orders/route.ts`: + +```ts +import { NextResponse } from 'next/server'; + +export async function POST(request: Request) { + const body = await request.json(); + if (!body.email) { + return NextResponse.json({ error: 'email required' }, { status: 400 }); + } + return NextResponse.json({ ok: true }); +} +``` + +Create `tests/fixtures/security-precision-world/app/api/invoices/route.ts`: + +```ts +import { NextResponse } from 'next/server'; + +export async function POST(request: Request) { + const body = await request.json(); + if (!body.email) { + return NextResponse.json({ error: 'email required' }, { status: 400 }); + } + return NextResponse.json({ ok: true }); +} +``` + +- [ ] **Step 2: Append the failing test** + +Append to `tests/security-precision.test.mjs`: + +```js +test('Duplicate names: Next.js POST route handlers are not flagged as a naming conflict', async () => { + const data = await analyzeFixture('security-precision-world'); + const postNameDup = data.duplicates.find((d) => d.type === 'name' && d.name === 'POST'); + + assert.equal(postNameDup, undefined); +}); +``` + +- [ ] **Step 3: Run the test to verify it fails** + +Run: `node --test tests/security-precision.test.mjs` +Expected: FAIL — a `type:'name'` duplicate for `POST` across the 3 route files is currently produced. + +- [ ] **Step 4: Add the Next.js route-handler names to `commonNames`** + +In `index.html`, inside `Parser.detectDuplicates:function(files,allFns){`, find: + +```js + 'onMount','onDestroy' + ]); +``` + +Replace with: + +```js + 'onMount','onDestroy', + // Next.js App Router route-handler & metadata exports (framework-mandated names) + 'GET','POST','PUT','DELETE','PATCH','HEAD','OPTIONS', + 'generateMetadata','generateStaticParams','generateImageMetadata','generateSitemaps','middleware' + ]); +``` + +- [ ] **Step 5: Skip non-production paths in the name-duplicate grouping** + +In `index.html`, find: + +```js + var fnByName=Object.create(null); + allFns.forEach(function(fn){ + // Skip non-string names (e.g. numeric object-literal keys from the JS AST walker) + if(typeof fn.name!=='string')return; +``` + +Replace with: + +```js + var fnByName=Object.create(null); + allFns.forEach(function(fn){ + // Skip functions outside shipped product code (tests, fixtures, tooling, docs) + if(isNonProductionPath(fn.file))return; + // Skip non-string names (e.g. numeric object-literal keys from the JS AST walker) + if(typeof fn.name!=='string')return; +``` + +- [ ] **Step 6: Skip non-production paths in the code-duplicate grouping** + +In `index.html`, find: + +```js + var codeGroups=Object.create(null); + allFns.forEach(function(fn){ + if(!fn.code||fn.code.length<80)return; // Skip very short functions +``` + +Replace with: + +```js + var codeGroups=Object.create(null); + allFns.forEach(function(fn){ + if(isNonProductionPath(fn.file))return; + if(!fn.code||fn.code.length<80)return; // Skip very short functions +``` + +- [ ] **Step 7: Run the test to verify it passes** + +Run: `node --test tests/security-precision.test.mjs` +Expected: PASS (8 tests total) + +- [ ] **Step 8: Run the non-regression suite** + +Run: `node --test tests/codeflow-golden.test.mjs` +Expected: PASS, unchanged. + +- [ ] **Step 9: Commit** + +```bash +git add index.html tests/security-precision.test.mjs tests/fixtures/security-precision-world/app/api +git commit -m "fix(duplicates): recognize Next.js route-handler exports as idiomatic, not duplicated" +``` + +--- + +### Task 8: Document the exclusions in the README + +**Files:** + +- Modify: `README.md:46-51` + +- [ ] **Step 1: Update the Security Scanner section** + +In `README.md`, find: + +```md +### Security Scanner +Automatic detection of: +- Hardcoded secrets & API keys +- SQL injection vulnerabilities +- Dangerous `eval()` usage +- Debug statements in production code +``` + +Replace with: + +```md +### Security Scanner +Automatic detection of: +- Hardcoded secrets & API keys +- SQL injection vulnerabilities +- Dangerous `eval()` usage +- Debug statements in production code + +Test files, fixtures, `docs/`, and common tooling directories (`.github/`, `.claude/`, `scripts/`) are excluded from the secret/XSS/shell-execution checks, since findings there don't reflect the shipped product's attack surface. +``` + +- [ ] **Step 2: Commit** + +```bash +git add README.md +git commit -m "docs: document security scanner path exclusions" +``` + +--- + +### Task 9: Full regression pass + +**Files:** none (verification only) + +- [ ] **Step 1: Run every test file in the repo** + +Run: `node --test tests/` +Expected: all suites PASS, including `tests/codeflow-golden.test.mjs` (unchanged) and `tests/security-precision.test.mjs` (8 tests, all passing). + +- [ ] **Step 2: Confirm the fixture tree matches the plan** + +Run: `find tests/fixtures/security-precision-world -type f | sort` +Expected output (order may vary by platform, content must match): + +``` +tests/fixtures/security-precision-world/.claude/hooks/pre-commit.py +tests/fixtures/security-precision-world/api/import.py +tests/fixtures/security-precision-world/app/api/invoices/route.ts +tests/fixtures/security-precision-world/app/api/orders/route.ts +tests/fixtures/security-precision-world/app/api/users/route.ts +tests/fixtures/security-precision-world/app/page.tsx +tests/fixtures/security-precision-world/app/profile-card.tsx +tests/fixtures/security-precision-world/components/dashboard.tsx +tests/fixtures/security-precision-world/docs/decisions.md +tests/fixtures/security-precision-world/lib/auth.ts +tests/fixtures/security-precision-world/lib/csp.ts +tests/fixtures/security-precision-world/lib/db.ts +tests/fixtures/security-precision-world/lib/dynamic.ts +tests/fixtures/security-precision-world/lib/runner.ts +tests/fixtures/security-precision-world/lib/search.ts +tests/fixtures/security-precision-world/server/logger.ts +tests/fixtures/security-precision-world/test/client-ip.test.ts +``` + +- [ ] **Step 3: Report status** + +No commit in this task — it is a verification checkpoint. If both steps pass, the branch is ready for the user to review and decide whether to push and open the PR against `braedonsaunders/codeflow` (not automated by this plan). + +--- + +## Self-Review Notes + +- **Spec coverage:** all 7 detector categories from the design doc have a task (1: secret, 2: shell/exec, 3: SQL, 4: XSS, 5: Function Constructor, 6: debug statements, 7: duplicates), plus the README update (8) and final regression (9). The shared classifier from design Section 1 is introduced in Task 1 and reused by Tasks 2 and 7 — no duplicate classifier logic. +- **Placeholder scan:** no TBD/TODO; every step has complete, verified code. +- **Type/name consistency:** `isNonProductionPath` is defined once (Task 1) and referenced identically (same name, same signature) in Tasks 2 and 7. `isArchitectureBackendPath` is consumed, not redefined, in Task 6. Test title strings match the exact `title:` values used in `detectSecurity`'s `issues.push(...)` calls. diff --git a/docs/superpowers/plans/2026-07-13-security-scanner-pr63-review-fixes.md b/docs/superpowers/plans/2026-07-13-security-scanner-pr63-review-fixes.md new file mode 100644 index 0000000..4050cfa --- /dev/null +++ b/docs/superpowers/plans/2026-07-13-security-scanner-pr63-review-fixes.md @@ -0,0 +1,102 @@ +# PR 63 review fixes — security scanner regressions + +Branch: `security-scanner-precision` (PR 63 on braedonsaunders/codeflow, currently CLOSED). +Base for this wave: commit `37cdcdd` (branch head). Baseline: full suite 47/47 pass via `node --test` from repo root. + +Reviewer reported four merge-blocking regressions. All four were verified against the code before planning. Each task fixes one and adds regression coverage. + +## Global constraints + +- All scanner logic lives in `index.html` between `// ===== CODEFLOW_ANALYZER_START =====` and `// ===== CODEFLOW_ANALYZER_END =====` markers. Tests (`tests/security-precision.test.mjs`) extract exactly that region and run it in a `node:vm` context — any code you add there must not depend on browser globals. +- Code style in `index.html` is dense ES5-ish: `var`, no spaces around `=` or after `,` in the analyzer section. Match it exactly. +- Fixtures live under `tests/fixtures/security-precision-world/`. Existing tests analyze that fixture tree via `analyzeFixture('security-precision-world')` and assert on `data.securityIssues` / duplicate findings. Follow the existing test patterns in `tests/security-precision.test.mjs`. +- Every task: write the failing regression test FIRST (verify it fails against current code), then apply the fix, then confirm the new test passes and the full suite (`node --test` from repo root) stays green (47 baseline + your new tests). +- Do not renumber or restructure unrelated code. Surgical diffs only. +- Commit per task with a conventional message (`fix(security): …` / `fix(duplicates): …`). + +## Task R1 — SQL template injection: scan ALL db calls, not just the first + +**Defect** (`index.html:1494-1496`): + +```js +var dbCallMatch=scanContent.match(/\b(?:query|execute|raw)\s*\(([^)]*)\)/i); +var hasSqlConcat=scanContent.match(/query\s*\(\s*['"`][^'"`]*\s*\+/)||scanContent.match(/execute\s*\(\s*['"`][^'"`]*\$\{/); +var hasSqlTemplateInjection=dbCallMatch&&/\$\{/.test(dbCallMatch[1])&&/(?:SELECT|INSERT|UPDATE|DELETE)/i.test(dbCallMatch[1]); +``` + +`.match()` without `/g` captures only the FIRST `query/execute/raw` call. A file whose first call is safe hides a vulnerable second call. Reviewer repro: ``db.query("SELECT 1"); db.query(`SELECT * FROM users WHERE id = ${id}`);`` → no finding. + +**Fix**: iterate every db call with a global regex, flag if ANY argument list contains `${` plus a SQL keyword: + +```js +var dbCallRegex=/\b(?:query|execute|raw)\s*\(([^)]*)\)/gi; +var hasSqlTemplateInjection=false; +var dbCall; +while((dbCall=dbCallRegex.exec(scanContent))!==null){ + if(/\$\{/.test(dbCall[1])&&/(?:SELECT|INSERT|UPDATE|DELETE)/i.test(dbCall[1])){hasSqlTemplateInjection=true;break;} +} +``` + +Keep `hasSqlConcat` and the finding-emission block (`f.isCode&&(hasSqlConcat||hasSqlTemplateInjection)`) unchanged. + +**Regression coverage**: add a fixture file (e.g. `tests/fixtures/security-precision-world/lib/reporting.ts`) whose FIRST db call is safe (`db.query("SELECT 1")` or similar, no `${`, no string concat) and whose SECOND call is a template-literal SQL query interpolating a variable. Test asserts a `SQL Injection Risk` finding is produced for that file. Verify test fails before the fix. Check the new fixture does not trip unrelated rules (run full suite) — if it does, adjust the fixture content, not the rules. + +## Task R2 — detect `node:child_process` + +**Defect** (`index.html:1525`): + +```js +var execMatch=scanContent.match(/(?:child_process|cp)\.\w*[Ee]xec\w*\s*\(/)||scanContent.match(/require\(\s*['"]child_process['"]\s*\)/)||scanContent.match(/from\s+['"]child_process['"]/); +``` + +Neither the `require` nor the `from` alternative allows the `node:` specifier prefix. `import { exec } from "node:child_process"; exec(userInput);` produces no Command Execution finding, while a bare `require('child_process')` with no call still does. + +**Fix**: add `(?:node:)?` to both import alternatives: + +```js +var execMatch=scanContent.match(/(?:child_process|cp)\.\w*[Ee]xec\w*\s*\(/)||scanContent.match(/require\(\s*['"](?:node:)?child_process['"]\s*\)/)||scanContent.match(/from\s+['"](?:node:)?child_process['"]/); +``` + +Do NOT change the pre-existing behavior of flagging an import without a call — the reviewer noted the asymmetry but the blocking defect is the missed `node:` form. + +**Regression coverage**: add a fixture file in a production path (NOT under `test/`, `docs/`, `.claude/` etc. — the rule is gated by `!isNonProductionPath`) containing `import { exec } from "node:child_process";` and a call `exec(someVar)`. Test asserts a `Command Execution` finding for that file. Verify it fails pre-fix. Also keep an eye on `require("node:child_process")` — cover it in the same test if cheap (a second assertion via a second snippet in the same fixture is fine). + +## Task R3 — restore structural (type "code") duplicate detection for non-production paths + +**Defect** (`index.html:1267-1268`): commit `0e9a675` added `isNonProductionPath(fn.file)` to BOTH the name-based `fnByName` loop (line ~1220, documented and intended) AND the structural `codeGroups` loop: + +```js +allFns.forEach(function(fn){ + if(isNonProductionPath(fn.file))return; // ← REMOVE this line (codeGroups loop only) + if(!fn.code||fn.code.length<80)return; +``` + +The PR body states structural `type: "code"` duplicate detection is intentionally untouched; this guard silently disabled it for tests/fixtures/tooling/docs. + +**Fix**: remove the guard from the `codeGroups` loop ONLY (the one right after `var codeGroups=Object.create(null);`). Leave the `fnByName` loop guard (with its comment) in place. + +**Regression coverage**: test that two structurally-identical functions (≥80 chars of code, different files, at least one under a non-production path such as `tests/` or a fixture `docs/`/tooling path) still yield a structural duplicate finding (`type:'code'` or however `buildAnalysisData` surfaces it — inspect how existing duplicate tests in `tests/security-precision.test.mjs` assert, e.g. the Next.js route-handler test, and mirror that mechanism). Verify the test fails against current branch code and passes after removing the guard. Prefer synthetic in-memory input over new fixture files if the existing test harness allows (it constructs `allFns` from fixtures — if fixture files are needed, place them so they don't disturb other assertions). + +## Task R4 — wire the new `info` severity into sort map, UI totals, and styles + +**Defect**: commit `9254993` introduced `severity:'info'` (Debug Statements on backend paths) but: +- `index.html:1606` sort map lacks it: `var sev={high:0,medium:1,low:2};` → `sev['info']` is `undefined`, comparator returns `NaN`, ordering becomes unstable/inconsistent. +- UI totals row (`index.html:~8755-8759`) renders only High/Medium/Low badges — an info finding is listed below but absent from the totals. +- CSS has `.security-item.high/.medium/.low` (lines 170-172) but no `.security-item.info`. +- `getSeverityColor` (`index.html:5377-5379`) falls through to blue for info, identical to low. + +**Fix**: +1. Sort map → `var sev={high:0,medium:1,low:2,info:3};` +2. Totals row: add a fourth badge after Low: `React.createElement('div',{className:'badge badge-default'},data.securityIssues.filter(function(i){return i.severity==='info';}).length,' Info')` — `badge-default` already exists (used at line ~8212). Match surrounding formatting. +3. CSS: add `.security-item.info` after line 172, following the pattern of the others but with a neutral/muted tint (inspect the CSS variables near the top of `index.html` and pick an existing neutral color var, e.g. the muted-text/border gray family — do not invent new hex without checking the palette). +4. `getSeverityColor`: return the same neutral/muted color for `level==='info'` so the StatusDot distinguishes info from low. Keep critical/high/medium/low behavior identical. + +**Regression coverage**: the vm-based harness only extracts the ANALYZER region, so UI/CSS aren't testable there. Cover what is testable: assert `detectSecurity` (or the sorted `securityIssues` from `analyzeFixture`) places an `info` finding strictly after `low` findings and that the comparator never yields NaN for any pair of severities present (e.g. assert the array of severities is ordered per `{high,medium,low,info}`). An existing fixture already produces an info finding (backend Debug Statements — check `server/logger.ts`); if none exists, extend a backend fixture with >3 console.log calls. + +## Task R5 — full-suite regression + ledger + +Run `node --test` from repo root in the working tree. Expect baseline 47 + all new tests, 0 fail. Record results. + +## After all tasks + +Final whole-branch review (this review wave only: diff `37cdcdd..HEAD`), then push to `fork` and reopen PR 63 (or open a revised PR if reopen is rejected) with a comment mapping each reviewer finding to its fix commit + regression test. diff --git a/docs/superpowers/specs/2026-07-11-security-scanner-precision-design.md b/docs/superpowers/specs/2026-07-11-security-scanner-precision-design.md new file mode 100644 index 0000000..ed5d9b8 --- /dev/null +++ b/docs/superpowers/specs/2026-07-11-security-scanner-precision-design.md @@ -0,0 +1,179 @@ +# Security & duplicate-detector precision — design + +Date: 2026-07-11 +Status: Approved for planning + +## Context + +`detectSecurity()` and `findDuplicates()` in the analyzer (embedded in `index.html` +between `CODEFLOW_ANALYZER_START`/`END`, ~lines 393-790) are plain regex/substring +scanners over raw file text, with no awareness of file role (test/fixture/tooling +vs. shipped product code) applied to security rules, no word-boundary precision on +several patterns, and a hand-curated function-name allowlist that omits Next.js App +Router route-handler conventions. + +This was diagnosed by running CodeFlow against an external project (AIROI): all 10 +"critical" security issues and the "duplicate POST/GET" quality issue it reported +were false positives. Each was traced to a specific line in the analyzer: + +| Alert | Root cause | Analyzer location | +|---|---|---| +| Hardcoded Secret in a test stub | No test-path exclusion in `detectSecurity`, even though `isArchitectureTestFile()` already exists in the same file for a different feature | ~line 703 | +| SQL Injection on a markdown bullet | Regex treats the English words select/insert/update/delete as SQL keywords, and scans all text file types including `.md` | ~line 707 | +| XSS on `dangerouslySetInnerHTML` with a static i18n string | Pure substring match on the API name, no check of whether the injected value is a literal or an external variable | ~line 711 | +| Shell Injection Risk on a dev git hook script | Scans the entire repo including tooling directories, no path exclusion | ~line 773 | +| Function Constructor false hit | `scanContent.includes('Function(')` is an unanchored substring match | ~line 721 | +| Debug Statements flagged on legitimate server logs | Pure count heuristic (`>3`), no client/server distinction | ~line 727 | +| "Duplicate POST/GET/generateMetadata" | `commonNames` allowlist covers React/Vue/Angular/Express/Python conventions but omits Next.js App Router route-handler exports | ~line 397-430 | + +The tool's own authors are aware detection noise is a real problem: `getSecurityScanContent()` +(index.html ~line 743) special-cases CodeFlow's own `index.html` to strip out +`detectSecurity` entirely when CodeFlow scans itself, so its own regex-pattern +strings (which contain `eval`, `Function(`, etc.) don't self-flag. That fix was +never generalized to consumer repositories. + +## Goal + +Reduce false-positive rate on the security scanner and duplicate-name detector to +the point where the "Security Scanner" feature advertised in the README is +trustworthy on real-world repositories, without turning the analyzer into a full +static-analysis engine (no AST-based taint tracking, no dependency additions). + +## Scope + +In scope: precision fixes to the 7 detectors above, upstreamable as a single PR to +`braedonsaunders/codeflow`. + +Out of scope (explicitly, to avoid scope creep): +- Full data-flow/taint analysis for XSS or SQL injection. +- Extending `commonNames` to frameworks other than Next.js (no concrete evidence + for other frameworks yet). +- Any change to `Debug Statements` beyond the backend/frontend path distinction. +- Any UI/UX/packaging/documentation work beyond the README's "Security Scanner" + section, which must stay accurate to the new behavior. + +## Design + +### 1. Shared file classifier + +New function, adjacent to the existing `isArchitectureTestFile`/ +`isArchitectureFixtureFile` (~line 2502): + +```js +function isDocumentationPath(path){ /* docs/ dirs + .md/.markdown/.mdx */ } +function isDevToolingPath(path){ /* .github/, .claude/, scripts|tools|tooling */ } +function isSecretScanExemptPath(path){ + // tests + fixtures + docs ONLY — dev tooling is executable code, so the + // Hardcoded Secret rule stays active there (PR 65 review, 2026-07-13). + return isArchitectureTestFile(path)||isArchitectureFixtureFile(path)||isDocumentationPath(path); +} +function isNonProductionPath(path){ + return isSecretScanExemptPath(path)||isDevToolingPath(path); +} +``` + +This reuses the two classifiers that already exist but were only wired into the +architecture/dead-code path, never into security or duplicate detection. +`isNonProductionPath` remains the single source of truth for "does this path count +as shipped product code?", now composed from per-concern predicates so individual +rules can opt into a narrower exemption. + +`detectSecurity(files)` and `findDuplicates(...)` both consult these. XSS and +shell/command-execution rules skip when `isNonProductionPath(f.path)` is true. +The Hardcoded Secret rule uses the narrower `isSecretScanExemptPath` — a credential +in a CI workflow, hook, or deploy script is a real leak even though the file is not +shipped product code. Python-specific rules (eval/exec/pickle/ +subprocess) stay active on all `.py` files regardless of path — a path-only signal +can't distinguish a production Python script from a tooling one — but this +limitation is documented in the PR body rather than left implicit. + +### 2. Per-category fixes + +1. **Hardcoded Secret** (~line 703-705): add the `isSecretScanExemptPath` guard + (tests/fixtures/docs only — see above) before pushing the issue. Regex itself + unchanged. + +2. **SQL Injection Risk** (~line 707-709): restrict to `f.isCode` files only (the + parser already computes this); tighten the third alternative to require a + plausible DB-call receiver on the same line + (`/\.(query|execute|raw)\s*\(/`) immediately preceding the SELECT/INSERT/ + UPDATE/DELETE match, instead of matching the bare English word anywhere in the + file. + +3. **XSS Vulnerability** (~line 711-716): extract the `__html:` value and skip the + issue when it is a literal/template string with no external identifier + interpolation: + ```js + var htmlMatch=scanContent.match(/dangerouslySetInnerHTML\s*:\s*\{\s*__html\s*:\s*([^}]+)\}/); + var isLiteralOnly=htmlMatch&&/^\s*(['"`])(?:(?!\1).)*\1\s*$/.test(htmlMatch[1].trim()); + ``` + Any interpolation of a variable/expression still triggers the issue — this stays + heuristic by design (per approved scope), not full taint analysis. + +4. **Command/Shell Execution** (~line 724-726, 737-738, 773-774): add the + `isNonProductionPath` guard; tighten `.exec(` to + `/(?:child_process|cp)\.\w*[Ee]xec\w*\s*\(/` so it no longer matches + `regex.exec(...)`. + +5. **Function Constructor** (~line 721-722): remove the bare + `scanContent.includes('Function(')`; keep only `/\bnew\s+Function\s*\(/`. + +6. **Debug Statements** (~line 727-731): reuse the existing + `isArchitectureBackendPath` classifier (~line 2521) to downgrade + `severity` from `'low'` to `'info'` when the file is server-only code, since a + server-side `console.log` never reaches the browser and isn't the same class of + risk as a client-side one. The issue is still surfaced, not deleted. + +7. **Duplicate function names** (~line 397-430): add to `commonNames`: + `'GET','POST','PUT','DELETE','PATCH','HEAD','OPTIONS','generateMetadata', + 'generateStaticParams','generateImageMetadata','generateSitemaps','middleware'`. + +### 3. Testing strategy + +Follow the existing golden-fixture pattern (`tests/codeflow-golden.test.mjs`): +extract the analyzer from `index.html` via `vm`, run it against fixtures on disk, +assert on `buildAnalysisData` output. + +New fixture: `tests/fixtures/security-precision-world/`, containing one +false-positive case and one true-positive case per detector (8 pairs total, +covering all 7 categories plus the shell/`.exec` word-boundary case): + +- `test/client-ip.test.ts` (stub secret, must NOT be flagged) vs. `lib/auth.ts` + (real hardcoded literal outside test dir, must stay flagged). +- `docs/decisions.md` ("update" in prose, must NOT be flagged) vs. `lib/db.ts` + (`query("SELECT * FROM x + " + id)`, must stay flagged). +- `app/page.tsx` (`__html:"fixed"`, must NOT be flagged) vs. a component + with `__html: rawUserBio`, must stay flagged. +- `.claude/hooks/pre-commit.py` (`subprocess.run(cmd, shell=True)`, must NOT be + flagged) vs. `api/import.py` (same call, outside tooling dir, must stay flagged). +- A file using `regex.exec(str)` (must NOT be flagged) vs. one using + `child_process.exec(userInput)` (must stay flagged). +- A comment containing the string `"getFunction(x)"` (must NOT be flagged) vs. + `new Function(userCode)` (must stay flagged). +- Three `route.ts` files each exporting `POST`/`GET` with a similar auth wrapper + (must NOT produce a duplicate-name issue). + +New test file `tests/security-precision.test.mjs`, one `test()` per pair, same +`assert.equal`/`assert.deepEqual` style as the existing golden test. + +Non-regression: `tests/codeflow-golden.test.mjs` must stay green unmodified. + +### 4. Delivery + +Single PR to `braedonsaunders/codeflow`, one commit per category for reviewability: + +1. `isNonProductionPath` + wiring into `detectSecurity`/`findDuplicates`. +2. One commit per of the 7 per-category fixes. +3. The new fixture + `tests/security-precision.test.mjs`. +4. README "Security Scanner" section update if the documented behavior changes + (e.g., noting that test/tooling/doc paths are excluded from the security scan). + +PR body documents the explicitly out-of-scope items above as known limitations. + +## Error handling + +No new failure modes: all changes are additional guards/regex tightening on +existing pure functions operating on already-validated file objects. No new I/O, +no new dependencies. If `f.path` is ever undefined, `isNonProductionPath` treats it +as an empty string and returns `false` (matches current defensive style elsewhere +in the file, e.g. `String(path||'')`). diff --git a/index.html b/index.html index 0ac531d..fd51cb3 100644 --- a/index.html +++ b/index.html @@ -170,6 +170,7 @@ .security-item.high{background:linear-gradient(180deg,rgba(255,95,95,0.08) 0%,var(--bg0) 55%);border-color:rgba(255,95,95,0.36)} .security-item.medium{background:linear-gradient(180deg,rgba(255,159,67,0.08) 0%,var(--bg0) 55%);border-color:rgba(255,159,67,0.34)} .security-item.low{background:linear-gradient(180deg,rgba(77,159,255,0.08) 0%,var(--bg0) 55%);border-color:rgba(77,159,255,0.34)} +.security-item.info{background:linear-gradient(180deg,rgba(92,92,102,0.08) 0%,var(--bg0) 55%);border-color:rgba(92,92,102,0.34)} .security-header{display:flex;align-items:center;gap:8px;margin-bottom:6px} .security-title{font-size:11px;color:var(--t0);font-weight:500} .security-desc{font-size:10px;color:var(--t2);margin-bottom:6px;line-height:1.4} @@ -1207,12 +1208,17 @@ // Angular lifecycle 'ngOnInit','ngOnDestroy','ngOnChanges','ngAfterViewInit', // Svelte - 'onMount','onDestroy' + 'onMount','onDestroy', + // Next.js App Router route-handler & metadata exports (framework-mandated names) + 'GET','POST','PUT','DELETE','PATCH','HEAD','OPTIONS', + 'generateMetadata','generateStaticParams','generateImageMetadata','generateSitemaps','middleware' ]); // Group functions by name (excluding common names) var fnByName=Object.create(null); allFns.forEach(function(fn){ + // Skip functions outside shipped product code (tests, fixtures, tooling, docs) + if(isNonProductionPath(fn.file))return; // Skip non-string names (e.g. numeric object-literal keys from the JS AST walker) if(typeof fn.name!=='string')return; // Skip common/idiomatic names @@ -1481,41 +1487,61 @@ if(!scanContent)return; var lines=scanContent.split('\n'); lines.forEach(function(line,idx){ - if(line.match(/(?:password|passwd|pwd|secret|api_key|apikey|token|auth)\s*[=:]\s*['"][^'"]{4,}['"]/i)&&!line.includes('process.env')&&!line.includes('config.')){ + if(!isSecretScanExemptPath(f.path)&&line.match(/(?:password|passwd|pwd|secret|api_key|apikey|token|auth)\s*[=:]\s*['"][^'"]{4,}['"]/i)&&!line.includes('process.env')&&!line.includes('config.')){ issues.push({severity:'high',title:'Hardcoded Secret',file:f.name,path:f.path,line:idx+1,desc:'Credentials should never be hardcoded. Use environment variables or a secrets manager.',code:line.trim().substring(0,80)}); } }); - if(scanContent.match(/query\s*\(\s*['"`][^'"`]*\s*\+/)||scanContent.match(/execute\s*\(\s*['"`][^'"`]*\$\{/)||scanContent.match(/\$\{.*\}.*(?:SELECT|INSERT|UPDATE|DELETE)/i)){ + var hasSqlConcat=scanContent.match(/query\s*\(\s*['"`][^'"`]*\s*\+/)||scanContent.match(/execute\s*\(\s*['"`][^'"`]*\$\{/); + var dbCallRegex=/\b(?:query|execute|raw)\s*\(([^)]*)\)/gi; + var hasSqlTemplateInjection=false; + var dbCall; + while((dbCall=dbCallRegex.exec(scanContent))!==null){ + if(/\$\{/.test(dbCall[1])&&/(?:SELECT|INSERT|UPDATE|DELETE)/i.test(dbCall[1])){hasSqlTemplateInjection=true;break;} + } + if(f.isCode&&(hasSqlConcat||hasSqlTemplateInjection)){ var m=scanContent.match(/.*(query|execute|SELECT|INSERT|UPDATE|DELETE).*(\+|\$\{).*/i); issues.push({severity:'high',title:'SQL Injection Risk',file:f.name,path:f.path,desc:'String concatenation in SQL queries. Use parameterized queries instead.',code:m?m[0].trim().substring(0,80):''}); } var hasInnerHtmlAssignment=scanContent.match(/innerHTML\s*=/); var hasDangerousHtmlRender=scanContent.match(/dangerouslySetInnerHTML/); var isSafePreviewRender=!hasInnerHtmlAssignment&&hasDangerousHtmlRender&&isSanitizedPreviewRenderer(f.content||''); - if((hasInnerHtmlAssignment||hasDangerousHtmlRender)&&!isSafePreviewRender){ + var htmlValueRegex=/dangerouslySetInnerHTML\s*[:=]\s*\{\{?\s*__html\s*:\s*([^}]+)\}/g; + var allHtmlValuesLiteral=true; + var foundHtmlValue=false; + var htmlMatch; + while((htmlMatch=htmlValueRegex.exec(scanContent))!==null){ + foundHtmlValue=true; + if(!/^(['"`])(?:(?!\1)[\s\S])*\1$/.test(htmlMatch[1].trim())){ + allHtmlValuesLiteral=false; + } + } + var isSafeStaticHtml=!hasInnerHtmlAssignment&&hasDangerousHtmlRender&&foundHtmlValue&&allHtmlValuesLiteral; + if(!isNonProductionPath(f.path)&&(hasInnerHtmlAssignment||hasDangerousHtmlRender)&&!isSafePreviewRender&&!isSafeStaticHtml){ issues.push({severity:'high',title:'XSS Vulnerability',file:f.name,path:f.path,desc:'Direct HTML injection can lead to XSS attacks. Sanitize user input.',code:''}); } if(scanContent.includes('eval(')){ var evalLine=lines.findIndex(function(l){return l.includes('eval(');}); issues.push({severity:'medium',title:'Dynamic Code Execution',file:f.name,path:f.path,line:evalLine+1,desc:'eval() executes arbitrary code. Avoid if possible or validate input strictly.',code:evalLine>=0?lines[evalLine].trim().substring(0,80):''}); } - if(scanContent.includes('Function(')||scanContent.match(/new\s+Function\s*\(/)){ + if(scanContent.match(/\b(?:new\s+)?Function\s*\(/)){ issues.push({severity:'medium',title:'Function Constructor',file:f.name,path:f.path,desc:'Function constructor is similar to eval(). Consider alternatives.',code:''}); } - if(scanContent.match(/\.exec\s*\(/)||scanContent.match(/child_process/)){ + var execMatch=scanContent.match(/(?:child_process|cp)\.\w*[Ee]xec\w*\s*\(/)||scanContent.match(/require\(\s*['"](?:node:)?child_process['"]\s*\)/)||scanContent.match(/from\s+['"](?:node:)?child_process['"]/); + if(!isNonProductionPath(f.path)&&execMatch){ issues.push({severity:'medium',title:'Command Execution',file:f.name,path:f.path,desc:'Shell command execution detected. Ensure input is sanitized to prevent injection.',code:''}); } if(scanContent.match(/console\.(log|debug|info)\(/)){ var consoleCount=(scanContent.match(/console\.(log|debug|info)\(/g)||[]).length; if(consoleCount>3){ - issues.push({severity:'low',title:'Debug Statements',file:f.name,path:f.path,desc:consoleCount+' console statements found. Remove before production.',code:''}); + var debugSeverity=isArchitectureBackendPath(f.path)?'info':'low'; + issues.push({severity:debugSeverity,title:'Debug Statements',file:f.name,path:f.path,desc:consoleCount+' console statements found. Remove before production.',code:''}); } } // VBA-specific security checks if(scanContent.match(/SendKeys\s*\(/i)){ issues.push({severity:'high',title:'SendKeys Usage',file:f.name,path:f.path,desc:'SendKeys can be exploited for code injection. Avoid using SendKeys.',code:''}); } - if(scanContent.match(/Shell\s*\(/i)){ + if(!isNonProductionPath(f.path)&&scanContent.match(/Shell\s*\(/i)){ issues.push({severity:'high',title:'Shell Command Execution',file:f.name,path:f.path,desc:'Shell() executes system commands. Ensure input is validated.',code:''}); } if(scanContent.match(/CreateObject\s*\(\s*["']WScript\.Shell["']/i)){ @@ -1551,7 +1577,7 @@ issues.push({severity:'high',title:'Pickle Deserialization',file:f.name,path:f.path,desc:'pickle.load() can execute arbitrary code from untrusted data. Use JSON or safe alternatives.',code:''}); } // subprocess with shell=True - if(scanContent.match(/subprocess\.\w+\([^)]*shell\s*=\s*True/)){ + if(!isNonProductionPath(f.path)&&scanContent.match(/subprocess\.\w+\([^)]*shell\s*=\s*True/)){ issues.push({severity:'high',title:'Shell Injection Risk',file:f.name,path:f.path,desc:'subprocess with shell=True is vulnerable to command injection. Use shell=False with a list of args.',code:''}); } // os.system / os.popen - command injection @@ -1581,7 +1607,7 @@ } } }); - return issues.sort(function(a,b){var sev={high:0,medium:1,low:2};return sev[a.severity]-sev[b.severity];}); + return issues.sort(function(a,b){var sev={high:0,medium:1,low:2,info:3};return sev[a.severity]-sev[b.severity];}); }, // AST-based function extraction - accurate detection without false positives extract:function(content,filename){ @@ -3290,6 +3316,39 @@ return /(^|\/)fixtures(\/|$)/.test(p)||/(^|\/)__fixtures__(\/|$)/.test(p); } +function isDocumentationPath(path){ + var p=String(path||'').toLowerCase().replace(/\\/g,'/'); + if(/(^|\/)docs?(\/|$)/.test(p))return true; + if(/\.(md|markdown|mdx)$/.test(p))return true; + return false; +} + +function isDevToolingPath(path){ + var p=String(path||'').toLowerCase().replace(/\\/g,'/'); + if(/(^|\/)\.github(\/|$)/.test(p))return true; + if(/(^|\/)\.claude(\/|$)/.test(p))return true; + if(/(^|\/)(scripts|tools|tooling)(\/|$)/.test(p))return true; + return false; +} + +// Secrets are dangerous wherever the code executes — CI workflows, hooks and +// deploy scripts included — so the Hardcoded Secret rule exempts only +// tests/fixtures (stub credentials) and docs (examples), NOT dev tooling. +function isSecretScanExemptPath(path){ + var p=String(path||'').toLowerCase().replace(/\\/g,'/'); + if(isArchitectureTestFile(p))return true; + if(isArchitectureFixtureFile(p))return true; + if(isDocumentationPath(p))return true; + return false; +} + +function isNonProductionPath(path){ + var p=String(path||'').toLowerCase().replace(/\\/g,'/'); + if(isSecretScanExemptPath(p))return true; + if(isDevToolingPath(p))return true; + return false; +} + function isArchitectureBarrelIndex(path){ var p=normalizeArchitecturePath(path).toLowerCase(); return /\/index\.(js|mjs|cjs|ts)$/i.test(p)&&!/\/index\.(tsx|jsx)$/i.test(p); @@ -5341,7 +5400,7 @@ } function getSeverityColor(level){ - return level==='critical'||level==='high'?'var(--red)':level==='medium'?'var(--orange)':'var(--blue)'; + return level==='critical'||level==='high'?'var(--red)':level==='medium'?'var(--orange)':level==='info'?'var(--t3)':'var(--blue)'; } function getFilePreviewIconName(filename){ @@ -8721,7 +8780,8 @@ React.createElement('div',{style:{display:'flex',gap:8,marginBottom:12}}, React.createElement('div',{className:'badge badge-danger'},data.securityIssues.filter(function(i){return i.severity==='high';}).length,' High'), React.createElement('div',{className:'badge badge-warning'},data.securityIssues.filter(function(i){return i.severity==='medium';}).length,' Medium'), - React.createElement('div',{className:'badge badge-info'},data.securityIssues.filter(function(i){return i.severity==='low';}).length,' Low') + React.createElement('div',{className:'badge badge-info'},data.securityIssues.filter(function(i){return i.severity==='low';}).length,' Low'), + React.createElement('div',{className:'badge badge-default'},data.securityIssues.filter(function(i){return i.severity==='info';}).length,' Info') ), data.securityIssues.map(function(issue,i){return React.createElement('div',{key:i,className:'security-item '+issue.severity,style:{cursor:'pointer'},onClick:function(){setDrillDown({type:'security',data:issue});}}, React.createElement('div',{className:'security-header'}, @@ -9052,7 +9112,9 @@ ? getAccentBlockStyle('rgba(255,95,95,0.36)','rgba(255,95,95,0.1)',{padding:12,marginBottom:16}) : drillDown.data.severity==='medium' ? getAccentBlockStyle('rgba(255,159,67,0.34)','rgba(255,180,100,0.1)',{padding:12,marginBottom:16}) - : getAccentBlockStyle('rgba(77,159,255,0.34)','rgba(100,180,255,0.1)',{padding:12,marginBottom:16})}, + : drillDown.data.severity==='info' + ? getAccentBlockStyle('rgba(92,92,102,0.34)','rgba(92,92,102,0.1)',{padding:12,marginBottom:16}) + : getAccentBlockStyle('rgba(77,159,255,0.34)','rgba(100,180,255,0.1)',{padding:12,marginBottom:16})}, React.createElement('div',{style:{fontSize:11,fontWeight:600,marginBottom:4}},drillDown.data.severity.toUpperCase()+' Severity'), React.createElement('div',{style:{fontSize:11,color:'var(--t2)'}},drillDown.data.desc) ), diff --git a/tests/fixtures/security-precision-world/.claude/hooks/pre-commit.py b/tests/fixtures/security-precision-world/.claude/hooks/pre-commit.py new file mode 100644 index 0000000..92a66e4 --- /dev/null +++ b/tests/fixtures/security-precision-world/.claude/hooks/pre-commit.py @@ -0,0 +1,5 @@ +import subprocess + + +def run(cmd): + subprocess.run(cmd, shell=True) diff --git a/tests/fixtures/security-precision-world/api/import.py b/tests/fixtures/security-precision-world/api/import.py new file mode 100644 index 0000000..92a66e4 --- /dev/null +++ b/tests/fixtures/security-precision-world/api/import.py @@ -0,0 +1,5 @@ +import subprocess + + +def run(cmd): + subprocess.run(cmd, shell=True) diff --git a/tests/fixtures/security-precision-world/app/api/invoices/route.ts b/tests/fixtures/security-precision-world/app/api/invoices/route.ts new file mode 100644 index 0000000..27f3bab --- /dev/null +++ b/tests/fixtures/security-precision-world/app/api/invoices/route.ts @@ -0,0 +1,9 @@ +import { NextResponse } from 'next/server'; + +export async function POST(request: Request) { + const body = await request.json(); + if (!body.email) { + return NextResponse.json({ error: 'email required' }, { status: 400 }); + } + return NextResponse.json({ ok: true }); +} diff --git a/tests/fixtures/security-precision-world/app/api/orders/route.ts b/tests/fixtures/security-precision-world/app/api/orders/route.ts new file mode 100644 index 0000000..27f3bab --- /dev/null +++ b/tests/fixtures/security-precision-world/app/api/orders/route.ts @@ -0,0 +1,9 @@ +import { NextResponse } from 'next/server'; + +export async function POST(request: Request) { + const body = await request.json(); + if (!body.email) { + return NextResponse.json({ error: 'email required' }, { status: 400 }); + } + return NextResponse.json({ ok: true }); +} diff --git a/tests/fixtures/security-precision-world/app/api/users/route.ts b/tests/fixtures/security-precision-world/app/api/users/route.ts new file mode 100644 index 0000000..27f3bab --- /dev/null +++ b/tests/fixtures/security-precision-world/app/api/users/route.ts @@ -0,0 +1,9 @@ +import { NextResponse } from 'next/server'; + +export async function POST(request: Request) { + const body = await request.json(); + if (!body.email) { + return NextResponse.json({ error: 'email required' }, { status: 400 }); + } + return NextResponse.json({ ok: true }); +} diff --git a/tests/fixtures/security-precision-world/app/mixed-render.tsx b/tests/fixtures/security-precision-world/app/mixed-render.tsx new file mode 100644 index 0000000..3260b57 --- /dev/null +++ b/tests/fixtures/security-precision-world/app/mixed-render.tsx @@ -0,0 +1,8 @@ +export function MixedRender({ rawUserBio }: { rawUserBio: string }) { + return ( +
+

safe static copy" }} /> +

+
+ ); +} diff --git a/tests/fixtures/security-precision-world/app/page.tsx b/tests/fixtures/security-precision-world/app/page.tsx new file mode 100644 index 0000000..ba5cd5d --- /dev/null +++ b/tests/fixtures/security-precision-world/app/page.tsx @@ -0,0 +1,5 @@ +export function LandingCopy() { + return ( +

5 a 30x moins cher que vos outils actuels" }} /> + ); +} diff --git a/tests/fixtures/security-precision-world/app/profile-card.tsx b/tests/fixtures/security-precision-world/app/profile-card.tsx new file mode 100644 index 0000000..19f897f --- /dev/null +++ b/tests/fixtures/security-precision-world/app/profile-card.tsx @@ -0,0 +1,3 @@ +export function ProfileCard({ rawUserBio }: { rawUserBio: string }) { + return

; +} diff --git a/tests/fixtures/security-precision-world/components/dashboard.tsx b/tests/fixtures/security-precision-world/components/dashboard.tsx new file mode 100644 index 0000000..37a070a --- /dev/null +++ b/tests/fixtures/security-precision-world/components/dashboard.tsx @@ -0,0 +1,7 @@ +export function Dashboard() { + console.log('render start'); + console.log('data', 1); + console.info('mounted'); + console.debug('layout computed'); + return null; +} diff --git a/tests/fixtures/security-precision-world/docs/decisions.md b/tests/fixtures/security-precision-world/docs/decisions.md new file mode 100644 index 0000000..7620fb9 --- /dev/null +++ b/tests/fixtures/security-precision-world/docs/decisions.md @@ -0,0 +1,3 @@ +# Decisions actees 2026-07-10 + +Voir ${CHANGELOG} pour la mise a jour complete (update log). diff --git a/tests/fixtures/security-precision-world/lib/auth.ts b/tests/fixtures/security-precision-world/lib/auth.ts new file mode 100644 index 0000000..c8f5df4 --- /dev/null +++ b/tests/fixtures/security-precision-world/lib/auth.ts @@ -0,0 +1,5 @@ +export const AUTH_SECRET = "hardcoded-example-not-a-real-credential-9f8e7d6c"; + +export function getAuthSecret() { + return AUTH_SECRET; +} diff --git a/tests/fixtures/security-precision-world/lib/csp.ts b/tests/fixtures/security-precision-world/lib/csp.ts new file mode 100644 index 0000000..725fab3 --- /dev/null +++ b/tests/fixtures/security-precision-world/lib/csp.ts @@ -0,0 +1,2 @@ +// NOTE: getFunction(x) helper lives in utils/reflection.ts - unrelated to eval. +export const CSP_HEADER = "script-src 'self'"; diff --git a/tests/fixtures/security-precision-world/lib/db.ts b/tests/fixtures/security-precision-world/lib/db.ts new file mode 100644 index 0000000..3354e90 --- /dev/null +++ b/tests/fixtures/security-precision-world/lib/db.ts @@ -0,0 +1,7 @@ +export function countUsers() { + return db.query("SELECT 1"); +} + +export function findUser(id: string) { + return db.query(`SELECT * FROM users WHERE id = ${id}`); +} diff --git a/tests/fixtures/security-precision-world/lib/dynamic.ts b/tests/fixtures/security-precision-world/lib/dynamic.ts new file mode 100644 index 0000000..690fae9 --- /dev/null +++ b/tests/fixtures/security-precision-world/lib/dynamic.ts @@ -0,0 +1,3 @@ +export function buildHandler(userCode: string) { + return new Function(userCode); +} diff --git a/tests/fixtures/security-precision-world/lib/runner.ts b/tests/fixtures/security-precision-world/lib/runner.ts new file mode 100644 index 0000000..6ec457f --- /dev/null +++ b/tests/fixtures/security-precision-world/lib/runner.ts @@ -0,0 +1,5 @@ +import child_process from 'child_process'; + +export function run(userInput: string) { + return child_process.exec(userInput); +} diff --git a/tests/fixtures/security-precision-world/lib/search.ts b/tests/fixtures/security-precision-world/lib/search.ts new file mode 100644 index 0000000..406c1e1 --- /dev/null +++ b/tests/fixtures/security-precision-world/lib/search.ts @@ -0,0 +1,3 @@ +export function find(items: string[], re: RegExp) { + return items.filter((i) => re.exec(i)); +} diff --git a/tests/fixtures/security-precision-world/preview-guard.md b/tests/fixtures/security-precision-world/preview-guard.md new file mode 100644 index 0000000..7e32922 --- /dev/null +++ b/tests/fixtures/security-precision-world/preview-guard.md @@ -0,0 +1,9 @@ +# Preview guard fixture + +This file exists purely to prove the XSS Vulnerability rule now skips +non-production paths: markdown files are excluded via `isNonProductionPath` +regardless of what code-like text they contain. + +```tsx +dangerouslySetInnerHTML={{ __html: rawUserBio }} +``` diff --git a/tests/fixtures/security-precision-world/server/logger.ts b/tests/fixtures/security-precision-world/server/logger.ts new file mode 100644 index 0000000..fa5b0ba --- /dev/null +++ b/tests/fixtures/security-precision-world/server/logger.ts @@ -0,0 +1,17 @@ +import { exec } from "node:child_process"; + +export function bootServer() { + console.log('booting server'); + console.log('loading config'); + console.info('config loaded'); + console.debug('ready to accept connections'); +} + +export function restartWorker(cmd: string) { + exec(cmd); +} + +export function spawnJob(cmd: string) { + const { exec: runCmd } = require("node:child_process"); + runCmd(cmd); +} diff --git a/tests/fixtures/security-precision-world/test/client-ip.test.ts b/tests/fixtures/security-precision-world/test/client-ip.test.ts new file mode 100644 index 0000000..c18da7b --- /dev/null +++ b/tests/fixtures/security-precision-world/test/client-ip.test.ts @@ -0,0 +1,6 @@ +const AUTH_SECRET = "stub-auth-secret-for-tests"; + +export function resolveClientIp(headers: Record) { + if (headers["x-forwarded-for"]) return headers["x-forwarded-for"]; + return AUTH_SECRET ? "test-mode" : "unknown"; +} diff --git a/tests/security-precision.test.mjs b/tests/security-precision.test.mjs new file mode 100644 index 0000000..5079d6c --- /dev/null +++ b/tests/security-precision.test.mjs @@ -0,0 +1,344 @@ +import assert from 'node:assert/strict'; +import { readdir, readFile } from 'node:fs/promises'; +import { basename, join, relative } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import test from 'node:test'; +import vm from 'node:vm'; + +const __dirname = fileURLToPath(new URL('.', import.meta.url)); +const repoRoot = join(__dirname, '..'); +const htmlSource = await readFile(join(repoRoot, 'index.html'), 'utf8'); +const startMarker = '// ===== CODEFLOW_ANALYZER_START ====='; +const endMarker = '// ===== CODEFLOW_ANALYZER_END ====='; +const parserStart = htmlSource.indexOf(startMarker); +const parserEnd = htmlSource.indexOf(endMarker, parserStart); + +if (parserStart < 0 || parserEnd < 0) { + throw new Error('Could not locate analyzer source in index.html'); +} + +const context = { + console, + TreeSitter: undefined, + Babel: undefined, + acorn: undefined, + getSecurityScanContent(file) { + return file && file.content ? file.content : ''; + }, + isSanitizedPreviewRenderer() { + return false; + }, +}; + +vm.createContext(context); +vm.runInContext( + `${htmlSource.slice(parserStart, parserEnd)}\nthis.Parser = Parser; this.buildAnalysisData = buildAnalysisData;`, + context +); + +const { Parser, buildAnalysisData } = context; + +async function collectFixtureFiles(root) { + const files = []; + + async function walk(dir) { + const entries = await readdir(dir, { withFileTypes: true }); + for (const entry of entries) { + const fullPath = join(dir, entry.name); + if (entry.isDirectory()) { + await walk(fullPath); + continue; + } + if (!entry.isFile() || !Parser.isIncluded(entry.name)) continue; + const repoPath = relative(root, fullPath).replace(/\\/g, '/'); + files.push({ + fullPath, + path: repoPath, + name: basename(repoPath), + folder: repoPath.includes('/') ? repoPath.slice(0, repoPath.lastIndexOf('/')) : 'root', + isCode: Parser.isCode(entry.name), + }); + } + } + + await walk(root); + return files.sort((a, b) => a.path.localeCompare(b.path)); +} + +async function analyzeFixture(name) { + const root = join(__dirname, 'fixtures', name); + const files = await collectFixtureFiles(root); + const analyzed = []; + const allFns = []; + + for (const file of files) { + const content = await readFile(file.fullPath, 'utf8'); + const layer = Parser.detectLayer(file.path); + const actualIsCode = file.isCode !== false && (!Parser.isScriptContainer(file.path) || Parser.hasEmbeddedCode(content, file.path)); + const functions = actualIsCode ? Parser.extract(content, file.path) : []; + analyzed.push({ + path: file.path, + name: file.name, + folder: file.folder, + content, + functions, + lines: content ? content.split('\n').length : 0, + layer, + churn: 0, + isCode: actualIsCode, + }); + if (actualIsCode) { + functions.forEach((fn) => allFns.push(Object.assign({}, fn, { folder: file.folder, layer }))); + } + } + + return buildAnalysisData({ + analyzed, + allFns, + excludePatterns: [], + progress() {}, + yieldFn: async () => {}, + }); +} + +async function analyzeSyntheticFiles(fileDescs) { + const analyzed = []; + const allFns = []; + + for (const file of fileDescs) { + const path = file.path; + const name = basename(path); + const folder = path.includes('/') ? path.slice(0, path.lastIndexOf('/')) : 'root'; + const content = file.content; + const layer = Parser.detectLayer(path); + const actualIsCode = Parser.isCode(name) !== false && (!Parser.isScriptContainer(path) || Parser.hasEmbeddedCode(content, path)); + const functions = actualIsCode ? Parser.extract(content, path) : []; + analyzed.push({ + path, + name, + folder, + content, + functions, + lines: content ? content.split('\n').length : 0, + layer, + churn: 0, + isCode: actualIsCode, + }); + if (actualIsCode) { + functions.forEach((fn) => allFns.push(Object.assign({}, fn, { folder, layer }))); + } + } + + return buildAnalysisData({ + analyzed, + allFns, + excludePatterns: [], + progress() {}, + yieldFn: async () => {}, + }); +} + +test('Hardcoded Secret rule excludes test stubs, keeps real hits', async () => { + const data = await analyzeFixture('security-precision-world'); + const flaggedPaths = data.securityIssues + .filter((i) => i.title === 'Hardcoded Secret') + .map((i) => i.path); + + assert.equal(flaggedPaths.includes('test/client-ip.test.ts'), false); + assert.equal(flaggedPaths.includes('lib/auth.ts'), true); +}); + +test('Hardcoded Secret rule still applies to executable infrastructure paths', async () => { + // Fixture lines are assembled at runtime so this test file never contains a + // literal credential-shaped string (fake values only, for the analyzer regex). + const credentialLine = (keyword, fakeValue) => `${keyword} = "${fakeValue}"\n`; + const data = await analyzeSyntheticFiles([ + { path: '.github/workflows/notify.js', content: `const ${credentialLine('api_key', 'fake-ci-value-1234')}` }, + { path: '.claude/hooks/session-start.py', content: credentialLine('password', 'fake-hook-value-1234') }, + { path: 'scripts/provision.py', content: credentialLine('token', 'fake-script-value-1234') }, + { path: 'docs/setup.md', content: credentialLine('password', 'fake-doc-value-1234') }, + ]); + const flaggedPaths = data.securityIssues + .filter((i) => i.title === 'Hardcoded Secret') + .map((i) => i.path); + + assert.equal(flaggedPaths.includes('.github/workflows/notify.js'), true); + assert.equal(flaggedPaths.includes('.claude/hooks/session-start.py'), true); + assert.equal(flaggedPaths.includes('scripts/provision.py'), true); + assert.equal(flaggedPaths.includes('docs/setup.md'), false); +}); + +test('Shell Injection Risk rule excludes dev tooling, keeps real hits', async () => { + const data = await analyzeFixture('security-precision-world'); + const flaggedPaths = data.securityIssues + .filter((i) => i.title === 'Shell Injection Risk') + .map((i) => i.path); + + assert.equal(flaggedPaths.includes('.claude/hooks/pre-commit.py'), false); + assert.equal(flaggedPaths.includes('api/import.py'), true); +}); + +test('Command Execution rule excludes regex.exec(), keeps child_process.exec()', async () => { + const data = await analyzeFixture('security-precision-world'); + const flaggedPaths = data.securityIssues + .filter((i) => i.title === 'Command Execution') + .map((i) => i.path); + + assert.equal(flaggedPaths.includes('lib/search.ts'), false); + assert.equal(flaggedPaths.includes('lib/runner.ts'), true); +}); + +test('Command Execution rule detects node: specifier on child_process import and require', async () => { + const data = await analyzeFixture('security-precision-world'); + const flaggedPaths = data.securityIssues + .filter((i) => i.title === 'Command Execution') + .map((i) => i.path); + + assert.equal(flaggedPaths.includes('server/logger.ts'), true); +}); + +test('Command Execution rule detects require("node:child_process") in isolation', async () => { + const data = await analyzeSyntheticFiles([ + { + path: 'server/spawner.ts', + content: 'const cp=require("node:child_process");\n', + }, + ]); + const flaggedPaths = data.securityIssues + .filter((i) => i.title === 'Command Execution') + .map((i) => i.path); + + assert.equal(flaggedPaths.includes('server/spawner.ts'), true); +}); + +test('SQL Injection Risk rule excludes markdown prose, keeps real template injection', async () => { + const data = await analyzeFixture('security-precision-world'); + const flaggedPaths = data.securityIssues + .filter((i) => i.title === 'SQL Injection Risk') + .map((i) => i.path); + + assert.equal(flaggedPaths.includes('docs/decisions.md'), false); + assert.equal(flaggedPaths.includes('lib/db.ts'), true); +}); + +test('SQL Injection Risk rule catches a vulnerable second db call after a safe first call', async () => { + const data = await analyzeFixture('security-precision-world'); + const flaggedPaths = data.securityIssues + .filter((i) => i.title === 'SQL Injection Risk') + .map((i) => i.path); + + assert.equal(flaggedPaths.includes('lib/db.ts'), true); +}); + +test('XSS Vulnerability rule excludes static literals, keeps variable interpolation', async () => { + const data = await analyzeFixture('security-precision-world'); + const flaggedPaths = data.securityIssues + .filter((i) => i.title === 'XSS Vulnerability') + .map((i) => i.path); + + assert.equal(flaggedPaths.includes('app/page.tsx'), false); + assert.equal(flaggedPaths.includes('app/profile-card.tsx'), true); +}); + +test('XSS Vulnerability rule still catches a dangerous occurrence after a safe literal in the same file', async () => { + const data = await analyzeFixture('security-precision-world'); + const flaggedPaths = data.securityIssues + .filter((i) => i.title === 'XSS Vulnerability') + .map((i) => i.path); + + assert.equal(flaggedPaths.includes('app/mixed-render.tsx'), true); +}); + +test('Function Constructor rule excludes substring mentions, keeps real constructor calls', async () => { + const data = await analyzeFixture('security-precision-world'); + const flaggedPaths = data.securityIssues + .filter((i) => i.title === 'Function Constructor') + .map((i) => i.path); + + assert.equal(flaggedPaths.includes('lib/csp.ts'), false); + assert.equal(flaggedPaths.includes('lib/dynamic.ts'), true); +}); + +test('Function Constructor rule flags bare Function() and new Function(), not identifiers like getFunction()', async () => { + const data = await analyzeSyntheticFiles([ + { path: 'lib/factory.ts', content: 'export function make(src: string) {\n return Function(src);\n}\n' }, + { path: 'lib/builder.ts', content: 'export function build(src: string) {\n return new Function(src);\n}\n' }, + { path: 'lib/reflection.ts', content: 'export function lookup(name: string) {\n return getFunction(name);\n}\n' }, + ]); + const flaggedPaths = data.securityIssues + .filter((i) => i.title === 'Function Constructor') + .map((i) => i.path); + + assert.equal(flaggedPaths.includes('lib/factory.ts'), true); + assert.equal(flaggedPaths.includes('lib/builder.ts'), true); + assert.equal(flaggedPaths.includes('lib/reflection.ts'), false); +}); + +test('Debug Statements rule downgrades server-only code, keeps client code at low', async () => { + const data = await analyzeFixture('security-precision-world'); + const serverIssue = data.securityIssues.find((i) => i.title === 'Debug Statements' && i.path === 'server/logger.ts'); + const clientIssue = data.securityIssues.find((i) => i.title === 'Debug Statements' && i.path === 'components/dashboard.tsx'); + + assert.equal(serverIssue.severity, 'info'); + assert.equal(clientIssue.severity, 'low'); +}); + +test('security issue sort places info strictly after high/medium/low with no NaN corruption', async () => { + const data = await analyzeSyntheticFiles([ + { path: 'server/telemetry.ts', content: 'console.log(1);console.log(2);console.log(3);console.log(4);\n' }, + { path: 'app/widget.ts', content: 'var f=new Function("return 1;");\n' }, + { path: 'lib/db-query.ts', content: 'function findUser(id){ return db.query(`SELECT * FROM users WHERE id = ${id}`); }\n' }, + ]); + const rank = { high: 0, medium: 1, low: 2, info: 3 }; + const severities = Array.from(data.securityIssues, (i) => i.severity); + + assert.deepEqual(severities, ['high', 'medium', 'info']); + for (let i = 1; i < severities.length; i++) { + assert.ok( + rank[severities[i - 1]] <= rank[severities[i]], + 'security issues must be sorted by non-decreasing severity rank (high, medium, low, info)' + ); + } +}); + +test('Duplicate names: Next.js POST route handlers are not flagged as a naming conflict', async () => { + const data = await analyzeFixture('security-precision-world'); + const postNameDup = data.duplicates.find((d) => d.type === 'name' && d.name === 'POST'); + + assert.equal(postNameDup, undefined); +}); + +test('XSS Vulnerability rule excludes dangerouslySetInnerHTML in non-production paths, even with variable interpolation', async () => { + const data = await analyzeFixture('security-precision-world'); + const flaggedPaths = data.securityIssues + .filter((i) => i.title === 'XSS Vulnerability') + .map((i) => i.path); + + assert.equal(flaggedPaths.includes('preview-guard.md'), false); + assert.equal(flaggedPaths.includes('app/profile-card.tsx'), true); +}); + +test('Duplicate code: structural duplicates are still detected across non-production paths', async () => { + const duplicateFn = [ + 'function processDataAlpha(items){', + ' var result=[];', + ' for(var i=0;i0){', + ' result.push(items[i]*2);', + ' }', + ' }', + ' return result;', + '}', + '', + ].join('\n'); + const data = await analyzeSyntheticFiles([ + { path: 'tests/helpers/a.ts', content: duplicateFn }, + { path: 'tools/b.ts', content: duplicateFn.replace('processDataAlpha', 'processDataBeta') }, + ]); + + const codeDup = data.duplicates.find( + (d) => d.type === 'code' && d.files.some((f) => f.file === 'tests/helpers/a.ts') && d.files.some((f) => f.file === 'tools/b.ts') + ); + + assert.notEqual(codeDup, undefined); +});