Fix security-scanner and duplicate-detector false positives (revised: addresses #63 review)

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

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.

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:"<em>fixed</em>"`, 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||'')`).