diff --git a/package.json b/package.json index 8d2dc8e..7590709 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "kcode", - "version": "2.10.5", + "version": "2.10.13", "description": "AI-powered coding assistant for the terminal - by Astrolexis", "author": "Astrolexis", "module": "src/index.ts", diff --git a/src/core/audit-engine/audit-engine.test.ts b/src/core/audit-engine/audit-engine.test.ts index f43baa0..d8a47ce 100644 --- a/src/core/audit-engine/audit-engine.test.ts +++ b/src/core/audit-engine/audit-engine.test.ts @@ -137,6 +137,55 @@ describe("scanner", () => { const hits = candidates.filter((c) => c.pattern_id === "cpp-006-strcpy-family"); expect(hits.length).toBe(2); }); + + test("findSourceFiles does NOT follow symlinks that escape the project root", async () => { + // Create a sibling directory with a source file that is NOT part + // of the audited project. Then place a symlink inside the project + // pointing at it. A naïve walker would follow the symlink and + // report the outside file; our scanner must reject it. + const outside = mkdtempSync(join(tmpdir(), "kcode-scan-outside-")); + try { + writeFileSync(join(outside, "leaked.py"), "print('secret')"); + writeFileSync(join(tmp, "normal.py"), "print('ok')"); + const { symlinkSync } = await import("node:fs"); + try { + symlinkSync(outside, join(tmp, "linked-outside")); + } catch { + // Symlinks may not be supported (e.g., non-privileged Windows); + // skip this test gracefully on such platforms. + return; + } + const files = findSourceFiles(tmp); + // We should see the normal file but NEVER the leaked file. + expect(files.some((f) => f.endsWith("normal.py"))).toBe(true); + expect(files.some((f) => f.endsWith("leaked.py"))).toBe(false); + } finally { + rmSync(outside, { recursive: true, force: true }); + } + }); + + test("findSourceFiles breaks symlink cycles without hanging", async () => { + // Two symlinks pointing at each other → naïve walker would loop. + // The scanner uses realpath + a visited-inode set and must + // terminate in bounded time. + writeFileSync(join(tmp, "real.ts"), "export const x = 1;"); + const { symlinkSync } = await import("node:fs"); + try { + mkdirSync(join(tmp, "a")); + mkdirSync(join(tmp, "b")); + symlinkSync(join(tmp, "b"), join(tmp, "a/to-b")); + symlinkSync(join(tmp, "a"), join(tmp, "b/to-a")); + } catch { + // Symlinks unsupported; skip. + return; + } + const files = findSourceFiles(tmp); + // Must include the real file and must not explode. + expect(files.some((f) => f.endsWith("real.ts"))).toBe(true); + // Upper bound on walk size is also a loose regression check: + // an infinite loop would blow past any sensible count. + expect(files.length).toBeLessThan(100); + }); }); describe("verifier", () => { diff --git a/src/core/audit-engine/fixer.test.ts b/src/core/audit-engine/fixer.test.ts index d107fa7..8ca6f7a 100644 --- a/src/core/audit-engine/fixer.test.ts +++ b/src/core/audit-engine/fixer.test.ts @@ -129,4 +129,302 @@ describe("fixer", () => { } expect(missing).toEqual([]); }); + + test("dart-007-json-null-check rewrites non-nullable casts to nullable+default", async () => { + writeFileSync( + join(tmp, "plant.dart"), + `class Plant { + final int id; + final String name; + final double capacity; + + Plant({required this.id, required this.name, required this.capacity}); + + factory Plant.fromJson(Map json) { + return Plant( + id: json['id'] as int, + name: json['name'] as String, + capacity: json['capacity'] as double, + ); + } +} +`, + ); + + const result = await runAudit({ + projectRoot: tmp, + llmCallback: async () => "VERDICT: CONFIRMED\n", + skipVerification: true, + }); + const fixes = applyFixes(result); + const transformed = fixes.filter((f) => f.kind === "transformed"); + expect(transformed.length).toBeGreaterThanOrEqual(1); + + const content = readFileSync(join(tmp, "plant.dart"), "utf-8"); + expect(content).toContain("as int? ?? 0"); + expect(content).toContain("as String? ?? ''"); + expect(content).toContain("as double? ?? 0.0"); + // No raw non-nullable casts left. + expect(content).not.toMatch(/as\s+int\b(?!\?)/); + expect(content).not.toMatch(/as\s+String\b(?!\?)/); + expect(content).not.toMatch(/as\s+double\b(?!\?)/); + }); + + test("dart-007 is idempotent — rerunning /fix does not double-wrap", async () => { + writeFileSync( + join(tmp, "safe.dart"), + `class Safe { + factory Safe.fromJson(Map json) { + return Safe(id: json['id'] as int? ?? 0, name: json['name'] as String? ?? ''); + } + Safe({required this.id, required this.name}); + final int id; + final String name; +} +`, + ); + + const result = await runAudit({ + projectRoot: tmp, + llmCallback: async () => "VERDICT: CONFIRMED\n", + skipVerification: true, + }); + const fixes = applyFixes(result); + // Either the pattern doesn't match (no unsafe cast) or the fixer skips + // the already-safe line. Either way, zero transformed changes. + const transformed = fixes.filter((f) => f.kind === "transformed"); + expect(transformed.length).toBe(0); + + const content = readFileSync(join(tmp, "safe.dart"), "utf-8"); + // Verify the file is still well-formed — no `as int? ?? 0? ?? 0`. + expect(content).not.toMatch(/\?\s*\?\?\s*\w+\?\s*\?\?/); + }); + + test("dart-007 does NOT rewrite non-json as-casts (business logic safety)", async () => { + // This file intentionally has: + // (a) a real json[...] as int cast that SHOULD be fixed + // (b) an unrelated `users.length as int` that MUST be left alone + // (c) a generic `result as String` that MUST be left alone + writeFileSync( + join(tmp, "mixed.dart"), + `class Mixed { + final int id; + final int count; + final String label; + + Mixed({required this.id, required this.count, required this.label}); + + factory Mixed.fromJson(Map json) { + final users = [1, 2, 3]; + final count = users.length as int; + final result = someCall(); + return Mixed( + id: json['id'] as int, + count: count, + label: result as String, + ); + } + static dynamic someCall() => 'hi'; +} +`, + ); + + const result = await runAudit({ + projectRoot: tmp, + llmCallback: async () => "VERDICT: CONFIRMED\n", + skipVerification: true, + }); + const fixes = applyFixes(result); + const transformed = fixes.filter((f) => f.kind === "transformed"); + + const content = readFileSync(join(tmp, "mixed.dart"), "utf-8"); + // The json[...] cast should be fixed. + expect(content).toContain("json['id'] as int? ?? 0"); + // The non-json casts MUST remain untouched — this is the whole + // point of the hole-#1 fix. If the regex started matching them, + // it would silently change the semantics of business logic. + expect(content).toContain("users.length as int;"); + expect(content).toContain("result as String,"); + expect(content).not.toContain("users.length as int? ??"); + expect(content).not.toContain("result as String? ??"); + // Sanity: at least one real fix was applied. + expect(transformed.length).toBeGreaterThanOrEqual(1); + }); + + test("dart-005 skips insertion when setState is not inside a State subclass", async () => { + // The setState call here is on a misleading helper that isn't + // inside a State, so `mounted` wouldn't be defined. The fixer + // must skip rather than produce uncompilable code. + writeFileSync( + join(tmp, "helper.dart"), + `class NotAState { + void doWork() async { + await Future.delayed(Duration(seconds: 1)); + setState(() => print('oops')); + } + void setState(void Function() fn) => fn(); +} +`, + ); + + const result = await runAudit({ + projectRoot: tmp, + llmCallback: async () => "VERDICT: CONFIRMED\n", + skipVerification: true, + }); + const fixes = applyFixes(result); + const dart005 = fixes.filter((f) => f.pattern_id === "dart-005-setstate-after-dispose"); + // If the pattern fires, the fixer must NOT apply it to this file — + // NotAState doesn't extend State. Either no finding or all + // findings for this pattern are skipped. + for (const f of dart005) { + expect(f.kind).toBe("skipped"); + } + + // Confirm the file was not touched by this pattern. + const content = readFileSync(join(tmp, "helper.dart"), "utf-8"); + expect(content).not.toContain("if (!mounted) return;"); + }); + + test("dart-005 recognizes a mounted guard added earlier in the same block", async () => { + // The await and setState are 6+ lines apart. A valid mounted guard + // sits right after the await — the old 3-line lookback missed this + // and would insert a DUPLICATE guard. The full-span check should + // recognize it and skip the insertion. + writeFileSync( + join(tmp, "state.dart"), + `import 'package:flutter/widgets.dart'; + +class MyScreen extends StatefulWidget { + @override + State createState() => _MyScreenState(); +} + +class _MyScreenState extends State { + bool _loaded = false; + + Future _load() async { + final data = await fetchData(); + if (!mounted) return; + // Four spacer lines between the guard and setState to defeat a + // short-window lookback. + // spacer + // spacer + // spacer + setState(() { + _loaded = true; + }); + } + + Future> fetchData() async => []; + + @override + Widget build(BuildContext context) => const SizedBox(); +} +`, + ); + + const result = await runAudit({ + projectRoot: tmp, + llmCallback: async () => "VERDICT: CONFIRMED\n", + skipVerification: true, + }); + const fixes = applyFixes(result); + const dart005 = fixes.filter((f) => f.pattern_id === "dart-005-setstate-after-dispose"); + // Either the pattern doesn't fire at all (regex doesn't match + // because a guard is already there and no setState "after" an + // unchecked await remains), or the fixer sees the guard and skips. + // Both are acceptable; what's NOT acceptable is a "transformed" + // result that inserts a duplicate guard. + for (const f of dart005) { + expect(f.kind).not.toBe("transformed"); + } + + const content = readFileSync(join(tmp, "state.dart"), "utf-8"); + // Count occurrences of `if (!mounted) return;` — must be exactly 1. + const guardCount = (content.match(/if \(!mounted\) return;/g) ?? []).length; + expect(guardCount).toBe(1); + }); + + test("applyRecipe does not insert duplicate annotations across repeated /fix runs", async () => { + // dart-001-insecure-http uses the generic recipe (advisory only, + // no bespoke fixer), so repeated /fix runs go through applyRecipe + // each time. The file starts with one `http://` URL. After the + // first /fix the annotation should be present exactly once, and + // a second /fix must NOT append another identical annotation. + writeFileSync( + join(tmp, "api.dart"), + `import 'package:http/http.dart' as http; + +Future fetch() async { + final r = await http.get(Uri.parse('http://api.example.com/data')); + print(r.body); +} +`, + ); + + // First /fix run — adds one annotation. + const r1 = await runAudit({ + projectRoot: tmp, + llmCallback: async () => "VERDICT: CONFIRMED\n", + skipVerification: true, + }); + applyFixes(r1); + const afterFirst = readFileSync(join(tmp, "api.dart"), "utf-8"); + const firstCount = (afterFirst.match(/KCODE-AUDIT:dart-001-insecure-http/g) ?? []).length; + expect(firstCount).toBe(1); + + // Second /fix run against the same (stale) audit result. The + // annotation is already in the file; the guard must catch it. + applyFixes(r1); + const afterSecond = readFileSync(join(tmp, "api.dart"), "utf-8"); + const secondCount = (afterSecond.match(/KCODE-AUDIT:dart-001-insecure-http/g) ?? []).length; + expect(secondCount).toBe(1); + + // Third run with a re-scan (scanner now reports the line AFTER + // the annotation shifted everything down by 1). The guard must + // still catch it because the window check looks ±3 lines. + const r3 = await runAudit({ + projectRoot: tmp, + llmCallback: async () => "VERDICT: CONFIRMED\n", + skipVerification: true, + }); + applyFixes(r3); + const afterThird = readFileSync(join(tmp, "api.dart"), "utf-8"); + const thirdCount = (afterThird.match(/KCODE-AUDIT:dart-001-insecure-http/g) ?? []).length; + expect(thirdCount).toBe(1); + }); + + test("generic recipes are reported as 'annotated', not 'transformed'", async () => { + // Pick a pattern that uses the generic recipe fallback (no bespoke + // fixer). dart-001-insecure-http is a simple regex pattern with only + // an advisory recipe — ideal for this test. + writeFileSync( + join(tmp, "api.dart"), + `import 'package:http/http.dart' as http; + +Future fetch() async { + final r = await http.get(Uri.parse('http://api.example.com/data')); + print(r.body); +} +`, + ); + + const result = await runAudit({ + projectRoot: tmp, + llmCallback: async () => "VERDICT: CONFIRMED\n", + skipVerification: true, + }); + const fixes = applyFixes(result); + // At least one finding should be annotated (recipe-only). None of + // these recipe-based findings should be reported as 'transformed'. + const annotated = fixes.filter((f) => f.kind === "annotated"); + expect(annotated.length).toBeGreaterThanOrEqual(1); + // The key property: buggy code is UNCHANGED. The `http://` URL must + // still be in the file — an annotation did not rewrite it. + const content = readFileSync(join(tmp, "api.dart"), "utf-8"); + expect(content).toContain("http://api.example.com/data"); + // And a KCODE-AUDIT marker comment was inserted above it. + expect(content).toContain("KCODE-AUDIT:dart-001-insecure-http"); + }); }); diff --git a/src/core/audit-engine/fixer.ts b/src/core/audit-engine/fixer.ts index 2903bd6..c10bd48 100644 --- a/src/core/audit-engine/fixer.ts +++ b/src/core/audit-engine/fixer.ts @@ -5,14 +5,59 @@ // // Flow: read finding → read source file → apply fix rule → write file -import { readFileSync, writeFileSync } from "node:fs"; +import { readFileSync, renameSync, unlinkSync, writeFileSync } from "node:fs"; +import { randomBytes } from "node:crypto"; import type { AuditResult, Finding } from "./types"; +/** + * Atomic file write: write to a sibling temp file, fsync if possible, + * then rename over the target. This avoids half-written state if the + * process dies mid-write (disk full, Ctrl-C, crash). The temp file lives + * in the same directory as the target so the rename is on the same + * filesystem and therefore atomic on POSIX. + * + * If the rename fails (e.g., cross-device on some setups), we clean up + * the temp file and let the error propagate. + */ +function atomicWriteFileSync(targetPath: string, content: string): void { + // 8 random bytes → 16 hex chars — low collision risk even with concurrent + // /fix runs, and short enough to stay under path length limits. + const tmp = `${targetPath}.kcode-fix-${randomBytes(8).toString("hex")}.tmp`; + try { + writeFileSync(tmp, content); + renameSync(tmp, targetPath); + } catch (err) { + // Best-effort cleanup of the temp file if the rename failed. We + // swallow errors from unlinkSync because the temp file may not + // exist (writeFileSync itself may have thrown before creating it). + try { unlinkSync(tmp); } catch { /* ignore */ } + throw err; + } +} + +/** + * Outcome of applying a single fix. + * + * - `transformed`: a bespoke fixer rewrote real code to remove the bug. + * The file on disk is now different in a meaningful way. + * - `annotated`: the generic recipe inserted a `KCODE-AUDIT:` warning + * comment above the finding. The buggy code is UNCHANGED — the comment + * is an advisory TODO the user still has to act on. + * - `skipped`: neither a bespoke fixer nor an annotation was applied + * (line out of range, pattern no longer present, marker already there). + * + * `applied` is kept as a boolean for existing callers; it is true for both + * `transformed` and `annotated`. New UI should look at `kind` instead so + * annotations aren't reported as real fixes. + */ +export type FixKind = "transformed" | "annotated" | "skipped"; + export interface FixResult { file: string; line: number; pattern_id: string; applied: boolean; + kind: FixKind; description: string; } @@ -43,6 +88,7 @@ export function applyFixes(result: AuditResult): FixResult[] { line: f.line, pattern_id: f.pattern_id, applied: false, + kind: "skipped", description: `Cannot read file: ${file}`, }); } @@ -65,12 +111,18 @@ export function applyFixes(result: AuditResult): FixResult[] { line: finding.line, pattern_id: finding.pattern_id, applied: fixResult.applied, + kind: fixResult.kind, description: fixResult.description, }); } if (modified) { - writeFileSync(file, lines.join("\n")); + // Preserve the original file's trailing newline convention — + // lines.split("\n") produces an empty last element if the file + // ended with "\n", which lines.join("\n") will serialize back + // correctly. No extra work needed; just use atomic write to + // avoid corruption on mid-write crash. + atomicWriteFileSync(file, lines.join("\n")); } } @@ -79,6 +131,7 @@ export function applyFixes(result: AuditResult): FixResult[] { interface OneFixResult { applied: boolean; + kind: FixKind; lines: string[]; description: string; } @@ -107,6 +160,10 @@ function applyOneFix(lines: string[], finding: Finding): OneFixResult { return fixPySqlInjection(lines, finding); case "py-005-yaml-unsafe-load": return fixPyYamlLoad(lines, finding); + case "dart-005-setstate-after-dispose": + return fixDartSetStateAfterDispose(lines, finding); + case "dart-007-json-null-check": + return fixDartJsonNullCheck(lines, finding); default: { // Fall through to the generic recipe table below. Every pattern // registered in patterns.ts has an entry here — the bespoke fixers @@ -117,6 +174,7 @@ function applyOneFix(lines: string[], finding: Finding): OneFixResult { if (recipe) return applyRecipe(lines, finding, recipe); return { applied: false, + kind: "skipped", lines, description: `No auto-fix for pattern: ${finding.pattern_id}`, }; @@ -130,14 +188,14 @@ function applyOneFix(lines: string[], finding: Finding): OneFixResult { function fixPointerArithmetic(lines: string[], finding: Finding): OneFixResult { const idx = finding.line - 1; if (idx < 0 || idx >= lines.length) { - return { applied: false, lines, description: "Line out of range" }; + return { applied: false, kind: "skipped", lines, description: "Line out of range" }; } const line = lines[idx]!; const re = /\(\s*&\s*(\w+)\s*\)\s*\[\s*(\w+)\s*\]/; const m = line.match(re); if (!m) { - return { applied: false, lines, description: "Pattern not found on this line" }; + return { applied: false, kind: "skipped", lines, description: "Pattern not found on this line" }; } const varName = m[1]; @@ -147,6 +205,7 @@ function fixPointerArithmetic(lines: string[], finding: Finding): OneFixResult { result[idx] = fixed; return { applied: true, + kind: "transformed", lines: result, description: `(&${varName})[${indexVar}] → ((const char*)${varName} + ${indexVar})`, }; @@ -158,7 +217,7 @@ function fixPointerArithmetic(lines: string[], finding: Finding): OneFixResult { function fixUnreachableCode(lines: string[], finding: Finding): OneFixResult { const idx = finding.line - 1; if (idx < 0 || idx >= lines.length) { - return { applied: false, lines, description: "Line out of range" }; + return { applied: false, kind: "skipped", lines, description: "Line out of range" }; } // The pattern matches: line with return/throw/break, NEXT line with statement @@ -186,13 +245,14 @@ function fixUnreachableCode(lines: string[], finding: Finding): OneFixResult { result[i + 1] = returnLine; return { applied: true, + kind: "transformed", lines: result, description: `Moved unreachable statement before return/throw`, }; } } - return { applied: false, lines, description: "Could not locate return+unreachable pair" }; + return { applied: false, kind: "skipped", lines, description: "Could not locate return+unreachable pair" }; } /** @@ -203,7 +263,7 @@ function fixUnreachableCode(lines: string[], finding: Finding): OneFixResult { function fixUncheckedDataIndex(lines: string[], finding: Finding): OneFixResult { const idx = finding.line - 1; if (idx < 0 || idx >= lines.length) { - return { applied: false, lines, description: "Line out of range" }; + return { applied: false, kind: "skipped", lines, description: "Line out of range" }; } // Find the function DEFINITION (not a call) by walking backwards. @@ -225,7 +285,7 @@ function fixUncheckedDataIndex(lines: string[], finding: Finding): OneFixResult } } if (funcStart < 0) { - return { applied: false, lines, description: "Could not find decode() function" }; + return { applied: false, kind: "skipped", lines, description: "Could not find decode() function" }; } // Find the opening brace @@ -237,13 +297,13 @@ function fixUncheckedDataIndex(lines: string[], finding: Finding): OneFixResult } } if (braceIdx < 0) { - return { applied: false, lines, description: "Could not find opening brace" }; + return { applied: false, kind: "skipped", lines, description: "Could not find opening brace" }; } // Check if there's already a size check (don't double-fix) const lineAfterBrace = lines[braceIdx + 1]?.trim() ?? ""; if (lineAfterBrace.includes("data.size()") || lineAfterBrace.includes("size() <")) { - return { applied: false, lines, description: "Size check already exists" }; + return { applied: false, kind: "skipped", lines, description: "Size check already exists" }; } // Scan the function body for the highest data[N] index @@ -276,7 +336,7 @@ function fixUncheckedDataIndex(lines: string[], finding: Finding): OneFixResult } if (maxIndex === 0) { - return { applied: false, lines, description: "No data[N] access found in function" }; + return { applied: false, kind: "skipped", lines, description: "No data[N] access found in function" }; } // Determine indentation from the line after the brace @@ -289,6 +349,7 @@ function fixUncheckedDataIndex(lines: string[], finding: Finding): OneFixResult return { applied: true, + kind: "transformed", lines: result, description: `Added size guard: data.size() <= ${maxIndex}`, }; @@ -300,7 +361,7 @@ function fixUncheckedDataIndex(lines: string[], finding: Finding): OneFixResult function fixFdLeakThrow(lines: string[], finding: Finding): OneFixResult { const idx = finding.line - 1; if (idx < 0 || idx >= lines.length) { - return { applied: false, lines, description: "Line out of range" }; + return { applied: false, kind: "skipped", lines, description: "Line out of range" }; } // Find the socket/open assignment near finding.line @@ -327,13 +388,14 @@ function fixFdLeakThrow(lines: string[], finding: Finding): OneFixResult { result.splice(i, 0, `${indent}::close(${fdVar});`); return { applied: true, + kind: "transformed", lines: result, description: `Added ::close(${fdVar}) before throw`, }; } } - return { applied: false, lines, description: "Could not find throw without preceding close()" }; + return { applied: false, kind: "skipped", lines, description: "Could not find throw without preceding close()" }; } /** @@ -343,7 +405,7 @@ function fixFdLeakThrow(lines: string[], finding: Finding): OneFixResult { function fixStrcpyFamily(lines: string[], finding: Finding): OneFixResult { const idx = finding.line - 1; if (idx < 0 || idx >= lines.length) { - return { applied: false, lines, description: "Line out of range" }; + return { applied: false, kind: "skipped", lines, description: "Line out of range" }; } const line = lines[idx]!; @@ -358,7 +420,7 @@ function fixStrcpyFamily(lines: string[], finding: Finding): OneFixResult { // Replace the entire strcpy(...) call using the exact matched text const fullMatch = strcpyMatch[0]; result[idx] = line.replace(fullMatch, `strncpy(${dst}, ${src}, ${len})`); - return { applied: true, lines: result, description: `strcpy → strncpy (${src}, ${len} bytes)` }; + return { applied: true, kind: "transformed", lines: result, description: `strcpy → strncpy (${src}, ${len} bytes)` }; } // strcat(dst, "literal") → strncat(dst, "literal", len) @@ -371,7 +433,7 @@ function fixStrcpyFamily(lines: string[], finding: Finding): OneFixResult { /\bstrcat\s*\([^)]+\)/, `strncat(${dst}, ${src}, ${len})`, ); - return { applied: true, lines: result, description: `strcat → strncat (${src}, ${len} chars)` }; + return { applied: true, kind: "transformed", lines: result, description: `strcat → strncat (${src}, ${len} chars)` }; } // sprintf(dst, "fmt", ...) → snprintf(dst, sizeof(dst), "fmt", ...) @@ -386,10 +448,10 @@ function fixStrcpyFamily(lines: string[], finding: Finding): OneFixResult { new RegExp(`snprintf\\(${dst}, sizeof\\(${dst}\\), ${dst},`), `snprintf(${dst}, sizeof(${dst}),`, ); - return { applied: true, lines: result, description: `sprintf → snprintf(${dst}, sizeof(${dst}), ...)` }; + return { applied: true, kind: "transformed", lines: result, description: `sprintf → snprintf(${dst}, sizeof(${dst}), ...)` }; } - return { applied: false, lines, description: "Non-literal source — manual fix needed" }; + return { applied: false, kind: "skipped", lines, description: "Non-literal source — manual fix needed" }; } // ── Python auto-fixes ───────────────────────────────────────── @@ -399,7 +461,7 @@ function fixStrcpyFamily(lines: string[], finding: Finding): OneFixResult { */ function fixPyShellInjection(lines: string[], finding: Finding): OneFixResult { const idx = finding.line - 1; - if (idx < 0 || idx >= lines.length) return { applied: false, lines, description: "Line out of range" }; + if (idx < 0 || idx >= lines.length) return { applied: false, kind: "skipped", lines, description: "Line out of range" }; const line = lines[idx]!; const result = [...lines]; @@ -408,20 +470,20 @@ function fixPyShellInjection(lines: string[], finding: Finding): OneFixResult { if (osSystemMatch) { const cmd = osSystemMatch[1]!.trim(); result[idx] = line.replace(/os\.system\s*\([^)]+\)/, `subprocess.run(${cmd}, shell=False) # FIXED: was os.system`); - return { applied: true, lines: result, description: "os.system → subprocess.run(shell=False)" }; + return { applied: true, kind: "transformed", lines: result, description: "os.system → subprocess.run(shell=False)" }; } // subprocess.call(..., shell=True) → shell=False if (line.includes("shell=True") || line.includes("shell = True")) { result[idx] = line.replace(/shell\s*=\s*True/g, "shell=False # FIXED: was shell=True"); - return { applied: true, lines: result, description: "shell=True → shell=False" }; + return { applied: true, kind: "transformed", lines: result, description: "shell=True → shell=False" }; } // subprocess with f-string → add comment warning if (line.match(/subprocess\.\w+\s*\(\s*f["']/)) { const indent = line.match(/^(\s*)/)?.[1] ?? ""; result.splice(idx, 0, `${indent}# SECURITY: Use list args instead of f-string to prevent injection`); - return { applied: true, lines: result, description: "Added security warning for f-string in subprocess" }; + return { applied: true, kind: "transformed", lines: result, description: "Added security warning for f-string in subprocess" }; } // List args with f-strings/format — add input validation warning @@ -431,10 +493,10 @@ function fixPyShellInjection(lines: string[], finding: Finding): OneFixResult { `${indent}# SECURITY: Validate user-controlled args before passing to subprocess`, `${indent}# Sanitize: strip shell metacharacters, validate expected format`, ); - return { applied: true, lines: result, description: "Added input validation warning for subprocess args" }; + return { applied: true, kind: "transformed", lines: result, description: "Added input validation warning for subprocess args" }; } - return { applied: false, lines, description: "Complex shell injection — manual fix needed" }; + return { applied: false, kind: "skipped", lines, description: "Complex shell injection — manual fix needed" }; } /** @@ -442,7 +504,7 @@ function fixPyShellInjection(lines: string[], finding: Finding): OneFixResult { */ function fixPyPathTraversal(lines: string[], finding: Finding): OneFixResult { const idx = finding.line - 1; - if (idx < 0 || idx >= lines.length) return { applied: false, lines, description: "Line out of range" }; + if (idx < 0 || idx >= lines.length) return { applied: false, kind: "skipped", lines, description: "Line out of range" }; const line = lines[idx]!; const indent = line.match(/^(\s*)/)?.[1] ?? ""; const result = [...lines]; @@ -452,7 +514,7 @@ function fixPyPathTraversal(lines: string[], finding: Finding): OneFixResult { `${indent}# SECURITY: Validate path to prevent traversal`, `${indent}import os; _path = os.path.abspath(_path); assert _path.startswith(os.getcwd()), "Path traversal blocked"`, ); - return { applied: true, lines: result, description: "Added path traversal guard" }; + return { applied: true, kind: "transformed", lines: result, description: "Added path traversal guard" }; } /** @@ -460,20 +522,20 @@ function fixPyPathTraversal(lines: string[], finding: Finding): OneFixResult { */ function fixPyEval(lines: string[], finding: Finding): OneFixResult { const idx = finding.line - 1; - if (idx < 0 || idx >= lines.length) return { applied: false, lines, description: "Line out of range" }; + if (idx < 0 || idx >= lines.length) return { applied: false, kind: "skipped", lines, description: "Line out of range" }; const line = lines[idx]!; const result = [...lines]; if (line.includes("eval(")) { result[idx] = line.replace(/\beval\s*\(/, "ast.literal_eval( # FIXED: was eval("); - return { applied: true, lines: result, description: "eval() → ast.literal_eval()" }; + return { applied: true, kind: "transformed", lines: result, description: "eval() → ast.literal_eval()" }; } if (line.includes("exec(")) { const indent = line.match(/^(\s*)/)?.[1] ?? ""; result.splice(idx, 0, `${indent}# SECURITY WARNING: exec() executes arbitrary code — remove or sandbox`); - return { applied: true, lines: result, description: "Added exec() security warning" }; + return { applied: true, kind: "transformed", lines: result, description: "Added exec() security warning" }; } - return { applied: false, lines, description: "Complex eval/exec — manual fix needed" }; + return { applied: false, kind: "skipped", lines, description: "Complex eval/exec — manual fix needed" }; } /** @@ -481,13 +543,13 @@ function fixPyEval(lines: string[], finding: Finding): OneFixResult { */ function fixPySqlInjection(lines: string[], finding: Finding): OneFixResult { const idx = finding.line - 1; - if (idx < 0 || idx >= lines.length) return { applied: false, lines, description: "Line out of range" }; + if (idx < 0 || idx >= lines.length) return { applied: false, kind: "skipped", lines, description: "Line out of range" }; const indent = lines[idx]!.match(/^(\s*)/)?.[1] ?? ""; const result = [...lines]; result.splice(idx, 0, `${indent}# SECURITY: Use parameterized query: cursor.execute("... WHERE id = %s", (id,))`, ); - return { applied: true, lines: result, description: "Added SQL injection warning + fix template" }; + return { applied: true, kind: "transformed", lines: result, description: "Added SQL injection warning + fix template" }; } /** @@ -495,15 +557,15 @@ function fixPySqlInjection(lines: string[], finding: Finding): OneFixResult { */ function fixPyYamlLoad(lines: string[], finding: Finding): OneFixResult { const idx = finding.line - 1; - if (idx < 0 || idx >= lines.length) return { applied: false, lines, description: "Line out of range" }; + if (idx < 0 || idx >= lines.length) return { applied: false, kind: "skipped", lines, description: "Line out of range" }; const line = lines[idx]!; const result = [...lines]; if (line.includes("yaml.load(")) { result[idx] = line.replace(/yaml\.load\s*\(/, "yaml.safe_load( # FIXED: was yaml.load("); - return { applied: true, lines: result, description: "yaml.load() → yaml.safe_load()" }; + return { applied: true, kind: "transformed", lines: result, description: "yaml.load() → yaml.safe_load()" }; } - return { applied: false, lines, description: "Complex YAML load — manual fix needed" }; + return { applied: false, kind: "skipped", lines, description: "Complex YAML load — manual fix needed" }; } /** @@ -512,14 +574,14 @@ function fixPyYamlLoad(lines: string[], finding: Finding): OneFixResult { function fixLoopBound(lines: string[], finding: Finding): OneFixResult { const idx = finding.line - 1; if (idx < 0 || idx >= lines.length) { - return { applied: false, lines, description: "Line out of range" }; + return { applied: false, kind: "skipped", lines, description: "Line out of range" }; } const line = lines[idx]!; // Extract the bound variable: for (...; var < BOUND; ...) const m = line.match(/\w+\s*<\s*(\w+(?:\.\w+|->[\w.]+)+)/); if (!m) { - return { applied: false, lines, description: "Could not extract loop bound" }; + return { applied: false, kind: "skipped", lines, description: "Could not extract loop bound" }; } const boundExpr = m[1]!; @@ -528,18 +590,194 @@ function fixLoopBound(lines: string[], finding: Finding): OneFixResult { // Check if there's already a validation above const prev = lines[idx - 1]?.trim() ?? ""; if (prev.includes(boundExpr) && (prev.includes("if") || prev.includes("max"))) { - return { applied: false, lines, description: "Bound validation already exists" }; + return { applied: false, kind: "skipped", lines, description: "Bound validation already exists" }; } const result = [...lines]; result.splice(idx, 0, `${indent}if (${boundExpr} > 10000) { return; } // guard: cap loop bound`); return { applied: true, + kind: "transformed", lines: result, description: `Added loop bound cap: ${boundExpr} > 10000`, }; } +/** + * dart-007: Rewrite `json['key'] as Int|String|double|bool|num` casts + * that target non-nullable primitives to use the nullable variant plus a + * safe default. + * + * id: json['id'] as int, → id: json['id'] as int? ?? 0, + * name: json['name'] as String, → name: json['name'] as String? ?? '', + * + * SCOPE — only the `json[...] as Type` shape is touched: + * - `foo.length as int` is NOT rewritten (not a JSON subscript). + * - `Map.cast()` is NOT rewritten (not an `as` cast). + * - `users as List` is NOT rewritten (not on a json subscript). + * This prevents the fixer from silently changing the semantics of + * business-logic casts that have nothing to do with JSON parsing. + * + * The audit engine dedupes matches of the same pattern in the same file + * into a single Finding (for verification efficiency), so a fromJson + * with 40 unsafe casts produces ONE finding. That finding's line is + * just the first match. Because every other match has the same + * json[...] shape, sweeping the whole file with the narrow regex + * rewrites all of them in one pass — AND because the regex is scoped + * to json subscripts, it never accidentally rewrites unrelated casts. + * + * Idempotency — the regex has a `(?!\?)` lookahead so casts already + * written as `as T?` are left alone, regardless of whether they're + * followed by `?? default` or not. + */ +function fixDartJsonNullCheck(lines: string[], _finding: Finding): OneFixResult { + const DEFAULTS: Record = { + int: "0", + double: "0.0", + num: "0", + bool: "false", + String: "''", + }; + // Narrow regex: `json['anything'] as TYPE` where TYPE is one of the + // supported primitives AND is not already nullable. Captures the whole + // `json[...] as TYPE` span as group 1 so the replacement can keep the + // original json access intact. + // + // Note: the outer `json` identifier match is intentionally literal — + // the pattern library's regex in patterns.ts only fires on the + // identifier `json`, which is the conventional parameter name for + // Dart fromJson factories. Projects that use a different name (e.g. + // `data` or `m`) won't get auto-fixed, but that's the correct + // conservative behavior for a deterministic rewriter. + const rex = /(\bjson\s*\[\s*['"][^'"]+['"]\s*\]\s*as\s+(int|double|num|bool|String))\b(?!\?)/g; + let totalCount = 0; + const result = lines.map((line) => + line.replace(rex, (_full, wholeCast: string, type: string) => { + totalCount++; + return `${wholeCast}? ?? ${DEFAULTS[type]}`; + }), + ); + if (totalCount === 0) { + return { + applied: false, + kind: "skipped", + lines, + description: "No unsafe `json[...] as Type` casts found in file", + }; + } + return { + applied: true, + kind: "transformed", + lines: result, + description: `Rewrote ${totalCount} json[...] non-nullable cast${totalCount === 1 ? "" : "s"} to nullable with default`, + }; +} + +// Walk backwards from `fromIdx` looking for a class declaration that +// extends one of Flutter's State base types. Returns true only if the +// setState call is clearly inside a State / ConsumerState / +// StatefulWidgetState subclass, where `mounted` is a defined +// instance getter. Returns false if no such class is found within a +// reasonable lookback (400 lines — large enough for most Dart files, +// small enough to avoid pathological O(n²) behavior). +// +// This keeps us from inserting `if (!mounted) return;` into code where +// `mounted` doesn't exist (e.g., a standalone function, a helper class, +// a mixin that receives a callback) which would fail to compile. +function isInsideFlutterState(lines: string[], fromIdx: number): boolean { + // Any of these patterns identifies a class where `mounted` is defined. + // We accept both raw Flutter (`State`) and common Riverpod/Provider + // extensions (`ConsumerState`, `ConsumerStatefulState`), plus the + // fully-qualified form. + const classRex = /\bclass\s+\w+[^{]*\bextends\s+\w*State(?:<|\b)/; + const limit = Math.max(0, fromIdx - 400); + for (let i = fromIdx; i >= limit; i--) { + if (classRex.test(lines[i]!)) return true; + } + return false; +} + +/** + * dart-005: Insert `if (!mounted) return;` before a setState call that + * sits after an `await`. Only fires when: + * + * 1. A `setState(` call is found within 10 lines after the finding's + * await line (walked forward). + * 2. NO mounted/disposed guard exists anywhere between the await and + * the setState call (full span check, not just 3-line lookback). + * 3. The setState is inside a `class Foo extends ... State<...>` + * subclass, so `mounted` is a valid instance getter. Otherwise we + * skip rather than produce uncompilable code. + */ +function fixDartSetStateAfterDispose(lines: string[], finding: Finding): OneFixResult { + const startIdx = finding.line - 1; + if (startIdx < 0 || startIdx >= lines.length) { + return { applied: false, kind: "skipped", lines, description: "Line out of range" }; + } + let setStateIdx = -1; + for (let i = startIdx; i < Math.min(lines.length, startIdx + 10); i++) { + if (/\bsetState\s*\(/.test(lines[i]!)) { + setStateIdx = i; + break; + } + } + if (setStateIdx === -1) { + return { + applied: false, + kind: "skipped", + lines, + description: "Could not locate setState call within 10 lines after await", + }; + } + // Full-span guard detection: walk every line between the await + // (startIdx) and the setState (setStateIdx) looking for ANY guard. + // This fixes the earlier 3-line lookback bug where a valid guard at + // line -5 was missed and we inserted a duplicate. + for (let i = setStateIdx - 1; i >= startIdx; i--) { + const prev = lines[i]!; + if (prev.trim() === "") continue; + if (/\bif\s*\(\s*!?(mounted|context\.mounted)\s*\)/.test(prev)) { + return { + applied: false, + kind: "skipped", + lines, + description: "mounted guard already present between await and setState", + }; + } + if (/\bif\s*\(\s*!?_?disposed\s*\)/.test(prev)) { + return { + applied: false, + kind: "skipped", + lines, + description: "disposed guard already present between await and setState", + }; + } + } + // Verify we're inside a State subclass before assuming `mounted` + // is defined. If not, skip the bespoke fix — the generic recipe + // (advisory comment) is still available through the default branch, + // but dart-005 routes here first. We'd rather return "skipped" than + // emit uncompilable code. + if (!isInsideFlutterState(lines, setStateIdx)) { + return { + applied: false, + kind: "skipped", + lines, + description: "setState not inside a State subclass — `mounted` may be undefined here", + }; + } + const indent = lines[setStateIdx]!.match(/^(\s*)/)?.[1] ?? ""; + const guard = `${indent}if (!mounted) return;`; + const result = [...lines]; + result.splice(setStateIdx, 0, guard); + return { + applied: true, + kind: "transformed", + lines: result, + description: "Inserted `if (!mounted) return;` before setState", + }; +} + // ── Generic recipe table ────────────────────────────────────── // // Every pattern in patterns.ts must have coverage: either a bespoke @@ -885,17 +1123,38 @@ function applyRecipe( ): OneFixResult { const idx = finding.line - 1; if (idx < 0 || idx >= lines.length) { - return { applied: false, lines, description: "Line out of range" }; + return { applied: false, kind: "skipped", lines, description: "Line out of range" }; } const line = lines[idx]!; const indent = line.match(/^(\s*)/)?.[1] ?? ""; const prefix = commentPrefix(finding.file); const tag = `KCODE-AUDIT:${finding.pattern_id}`; - // Skip if a previous /fix run already tagged this line. - const prev = lines[idx - 1] ?? ""; - if (prev.includes(tag)) { - return { applied: false, lines, description: "Warning already present" }; + // Skip if a previous /fix run already tagged this location. + // + // The old check only looked at `lines[idx - 1]`, which fails when + // /fix is re-run against a stale AUDIT_REPORT.json whose line + // numbers predate a previous annotation. Example: + // + // Run 1 — scanner reports Future.delayed at line 190. Annotation + // inserted at idx 189. Future.delayed shifts to 191. + // Run 2 — stale report still says line 190. idx = 189. + // `lines[idx - 1] = lines[188]` = debugPrint — no tag. + // Guard misses → duplicate annotation inserted at 189. + // + // The reliable fix: scan a small window (±3 lines) around the + // insertion point for the tag. Any hit, skip. This absorbs line + // drift of up to 3 positions from stale reports. + const WINDOW = 3; + let existingTag = false; + for (let i = Math.max(0, idx - WINDOW); i <= Math.min(lines.length - 1, idx + WINDOW); i++) { + if (lines[i]!.includes(tag)) { + existingTag = true; + break; + } + } + if (existingTag) { + return { applied: false, kind: "skipped", lines, description: "Warning already present" }; } const warningLines: string[] = []; @@ -906,7 +1165,11 @@ function applyRecipe( const result = [...lines]; result.splice(idx, 0, ...warningLines); - return { applied: true, lines: result, description: recipe.description }; + // IMPORTANT: this is an annotation, NOT a real fix. The buggy code is + // unchanged; we only inserted an advisory `KCODE-AUDIT:` comment. + // Callers should report this distinctly from transformed fixes so users + // know the finding still needs manual attention. + return { applied: true, kind: "annotated", lines: result, description: recipe.description }; } /** @@ -926,6 +1189,8 @@ const BESPOKE_PATTERN_IDS: ReadonlySet = new Set([ "py-004-sql-injection", "py-005-yaml-unsafe-load", "py-008-path-traversal", + "dart-005-setstate-after-dispose", + "dart-007-json-null-check", ]); export function hasFixRecipe(patternId: string): boolean { diff --git a/src/core/audit-engine/patterns.ts b/src/core/audit-engine/patterns.ts index 9212cc5..c3fada5 100644 --- a/src/core/audit-engine/patterns.ts +++ b/src/core/audit-engine/patterns.ts @@ -554,10 +554,19 @@ export const PYTHON_PATTERNS: BugPattern[] = [ explanation: "The global keyword creates shared mutable state that makes code harder to test, reason about, and maintain. It can cause subtle bugs in multi-threaded code and makes dependency injection impossible.", verify_prompt: - "Is this global used for legitimate module-level state (e.g., singleton " + - "initialization, configuration cache)? If it's a well-known pattern like " + - "module-level logger or config, respond FALSE_POSITIVE. If it's used to pass " + - "state between functions, respond CONFIRMED.", + "Is this global used for a legitimate module-level state pattern? " + + "Respond FALSE_POSITIVE for any of: " + + "(1) module-level logger, (2) configuration / settings cache, " + + "(3) singleton lazy-init (e.g., `_instance`, `_client`, `_pool`), " + + "(4) circuit breaker state (`_circuit_open_until`, `_failure_count`, `_last_failure`), " + + "(5) rate limiter / token bucket state, " + + "(6) connection pool / HTTP session reuse, " + + "(7) feature flag cache or hot-reloaded config, " + + "(8) memoization / LRU cache implementation, " + + "(9) test fixtures or pytest monkeypatch setup. " + + "These are all well-known Python patterns where module-level state is idiomatic. " + + "Only respond CONFIRMED if the global is used to pass arbitrary state between " + + "unrelated functions in a way that suggests the code should have been a class.", cwe: "CWE-1054", fix_template: "Pass the value as a function parameter, use a class to encapsulate state, or use a module-level constant.", }, @@ -3577,7 +3586,7 @@ export const UNIVERSAL_PATTERNS: BugPattern[] = [ id: "sh-001-eval-injection", title: "eval with variable expansion in shell script", severity: "critical", - languages: ["c", "cpp", "python", "javascript", "typescript", "go", "rust", "java"], + languages: ["shell"], regex: /\beval\s+["']?\$[\{(]/g, explanation: "eval with variable expansion in shell enables command injection.", verify_prompt: "Is the variable from trusted internal source or user input? CONFIRMED if user-controlled." + diff --git a/src/core/audit-engine/scanner.ts b/src/core/audit-engine/scanner.ts index 0d8f9c7..935afd0 100644 --- a/src/core/audit-engine/scanner.ts +++ b/src/core/audit-engine/scanner.ts @@ -5,8 +5,8 @@ // calls yet — candidates are just regex matches in files. import { execSync } from "node:child_process"; -import { existsSync, readFileSync, readdirSync, statSync } from "node:fs"; -import { extname, join, relative } from "node:path"; +import { existsSync, readFileSync, readdirSync, realpathSync, statSync } from "node:fs"; +import { extname, join, relative, resolve, sep } from "node:path"; import { getAllPatterns } from "./patterns"; import type { BugPattern, Candidate, Language } from "./types"; @@ -94,26 +94,103 @@ const SOURCE_EXTENSIONS: Record = { ".jl": "julia", ".sql": "sql", ".m": "matlab", + ".sh": "shell", + ".bash": "shell", + ".zsh": "shell", + ".ksh": "shell", }; +// Directories matched by exact name at any depth. This is the coarse +// first-pass filter — if a folder in the walk is literally one of these, +// the entire subtree is skipped. const SKIP_DIRS = new Set([ - "node_modules", + // Universal VCS / IDE / build roots ".git", - ".next", + ".hg", + ".svn", + ".idea", + ".vscode", + ".vs", "build", "dist", - "target", "out", + "target", + // JavaScript / TypeScript ecosystem + "node_modules", + ".next", + ".nuxt", + ".svelte-kit", + ".turbo", + ".parcel-cache", + ".vuepress", + ".docusaurus", + ".cache", + "coverage", + ".nyc_output", + // Python ecosystem ".venv", "venv", + "env", "__pycache__", + ".pytest_cache", + ".mypy_cache", + ".ruff_cache", + ".tox", + "site-packages", + // Ruby ecosystem + ".bundle", + // Elixir / Erlang + "_build", + "deps", + ".elixir_ls", + // Scala / SBT + ".bloop", + ".metals", + // Haskell + ".stack-work", + "dist-newstyle", + // Rust — already has target above + // Go — already has vendor below + // C / C++ + "CMakeFiles", + ".ccls-cache", + "cmake-build-debug", + "cmake-build-release", + "cmake-build-relwithdebinfo", + // Third-party / vendored "3rdParty", "third_party", "vendor", + "Godeps", + // Project-specific noise we've seen before "hidapi", "hidtest", "testgui", "pp_data_dump", + // Flutter generated / ephemeral — plugin symlinks, dart tool cache, ephemeral + // platform projects. All of this is either vendored plugin code or generated + // by `flutter pub get`, and should never surface in a user project audit. + ".dart_tool", + ".plugin_symlinks", + "ephemeral", + ".flutter-plugins", + ".flutter-plugins-dependencies", + // iOS / macOS generated + "Pods", + "DerivedData", + "xcuserdata", + ".build", // Swift Package Manager + ".swiftpm", + // Android generated + ".gradle", + ".cxx", + // Other JVM platform junk + ".kotlin", + ".mvn", + // .NET + "bin", + "obj", + "packages", // Test directories — findings in test code are low-value noise. // Unit test stubs intentionally replicate unsafe patterns (e.g. strcat) // and test harnesses control their own inputs. @@ -135,12 +212,135 @@ const SKIP_DIRS = new Set([ "testcases", ]); +// Substrings matched against the full relative path. Used when the noisy +// directory is *not* a literal top-level name but rather a path segment +// under some user directory (e.g. "src/generated/..." or ".../build/intermediates/..."). +const SKIP_PATH_SUBSTRINGS: readonly string[] = [ + "/generated/", + "/.generated/", + "/_generated/", + "/build/intermediates/", + "/build/generated/", + "/autogen/", + "/auto-generated/", +]; + +// Regex matched against the basename of each file. Captures generated-file +// conventions that language toolchains emit even inside user-authored trees. +const SKIP_FILENAME_PATTERNS: readonly RegExp[] = [ + // Minified / bundled JS / CSS + /\.min\.(js|mjs|css)$/i, + /\.bundle\.js$/i, + /\.chunk\.js$/i, + // Source maps are non-source anyway but guard against weird extensions + /\.map$/i, + // Dart code-gen (build_runner, freezed, json_serializable) + /\.g\.dart$/, + /\.freezed\.dart$/, + /\.gr\.dart$/, + /\.config\.dart$/, + // Python generated — protobuf, grpc, swig + /_pb2?\.py$/, + /_pb2_grpc\.py$/, + // Go generated — protobuf, mocks, stringer + /\.pb\.go$/, + /\.pb\.gw\.go$/, + /_mock\.go$/, + /_string\.go$/, // stringer + // C / C++ generated — Qt moc, flex/bison, protobuf + /^moc_.*\.(cc|cpp|cxx)$/, + /^ui_.*\.h$/, + /\.pb\.(cc|cpp|h)$/, + /^lex\..*\.c$/, + /\.tab\.(c|h)$/, + // C# generated — Windows Forms / Xaml designer + /\.designer\.cs$/i, + /\.g\.cs$/i, + /\.g\.i\.cs$/i, + // Swift generated + /^Generated.*\.swift$/, + // Java / Kotlin generated + /_Factory\.java$/, + /_MembersInjector\.java$/, + /Dagger.*\.java$/, + // Rust macro expansions / build-script output usually land under target/ + // which is already in SKIP_DIRS. + // TypeScript / JavaScript declaration files for bundled libs + /\.d\.ts\.map$/, +]; + +/** + * Heuristic: is this file minified / machine-generated? + * + * Looks at the longest line. Real source code almost never has lines longer + * than ~1000 characters; minified JS/CSS routinely has single lines of 100KB+. + * Threshold of 5000 is conservative enough to avoid false positives on + * generated SQL schemas or long strings. + */ +function looksMinified(content: string): boolean { + if (content.length < 5000) return false; // small files can't be meaningfully minified + let maxLine = 0; + let lineStart = 0; + for (let i = 0; i < content.length; i++) { + if (content.charCodeAt(i) === 10 /* \n */) { + const len = i - lineStart; + if (len > maxLine) maxLine = len; + lineStart = i + 1; + } + } + const tailLen = content.length - lineStart; + if (tailLen > maxLine) maxLine = tailLen; + return maxLine > 5000; +} + +function isSkippedFilename(basename: string): boolean { + for (const rex of SKIP_FILENAME_PATTERNS) { + if (rex.test(basename)) return true; + } + return false; +} + +function isSkippedPath(fullPath: string): boolean { + // Normalize separators so the same substrings work on Windows-style paths. + const p = fullPath.replace(/\\/g, "/"); + for (const needle of SKIP_PATH_SUBSTRINGS) { + if (p.includes(needle)) return true; + } + return false; +} + /** * Walk a directory tree and return absolute paths of source files. + * + * Symlink safety: + * - Every directory is resolved via `realpath` so cyclic symlinks + * (a→b→a, link→., etc.) are detected and only traversed once. + * - Every resolved path is required to stay inside the resolved + * project root. A symlink that points outside the project + * (`my-lib -> /etc/ssh/...`) is silently skipped instead of + * leaking files outside the audit scope. + * - File symlinks are resolved the same way before being emitted, + * so the audit never double-reports the same file via two aliases. */ export function findSourceFiles(root: string, maxFiles = 500): string[] { const out: string[] = []; - const stack: string[] = [root]; + // Resolve the project root once. If realpath fails (broken link, + // missing dir) fall back to the plain absolute path. + let rootReal: string; + try { + rootReal = realpathSync(resolve(root)); + } catch { + rootReal = resolve(root); + } + // rootPrefix is what we compare every resolved descendant against. + // Appending the separator avoids `/home/foo` matching `/home/foo-evil`. + const rootPrefix = rootReal.endsWith(sep) ? rootReal : rootReal + sep; + + const visitedDirs = new Set(); + const visitedFiles = new Set(); + const stack: string[] = [rootReal]; + visitedDirs.add(rootReal); + while (stack.length > 0 && out.length < maxFiles) { const dir = stack.pop()!; let entries: string[]; @@ -159,13 +359,40 @@ export function findSourceFiles(root: string, maxFiles = 500): string[] { continue; } if (s.isDirectory()) { - stack.push(full); + if (isSkippedPath(full + "/")) continue; + // Resolve the real path before descending — this is how we + // both break symlink cycles and prevent escaping the project + // root through a symlink into /etc or $HOME. + let real: string; + try { + real = realpathSync(full); + } catch { + continue; + } + // Root-confinement: the resolved directory must equal the + // project root itself OR be strictly inside it. + if (real !== rootReal && !real.startsWith(rootPrefix)) continue; + if (visitedDirs.has(real)) continue; + visitedDirs.add(real); + stack.push(real); } else if (s.isFile()) { const ext = extname(entry).toLowerCase(); - if (SOURCE_EXTENSIONS[ext]) { - out.push(full); - if (out.length >= maxFiles) break; + if (!SOURCE_EXTENSIONS[ext]) continue; + if (isSkippedFilename(entry)) continue; + if (isSkippedPath(full)) continue; + let real: string; + try { + real = realpathSync(full); + } catch { + continue; } + // Same root confinement for file symlinks: never report a file + // whose real path is outside the audited project. + if (real !== rootReal && !real.startsWith(rootPrefix)) continue; + if (visitedFiles.has(real)) continue; + visitedFiles.add(real); + out.push(real); + if (out.length >= maxFiles) break; } } } @@ -285,6 +512,10 @@ export function scanProject( } // Skip excessively large files if (content.length > 500_000) continue; + // Skip minified / machine-generated files that slipped past filename + // and path filters. These produce massive false-positive counts because + // their single long lines match many regexes accidentally. + if (looksMinified(content)) continue; for (const pattern of patterns) { const lang = getLanguageForFile(file); diff --git a/src/core/audit-engine/types.ts b/src/core/audit-engine/types.ts index 715115b..71f5687 100644 --- a/src/core/audit-engine/types.ts +++ b/src/core/audit-engine/types.ts @@ -7,7 +7,7 @@ export type Language = | "javascript" | "typescript" | "swift" | "java" | "kotlin" | "csharp" | "php" | "ruby" | "dart" | "scala" | "elixir" | "lua" | "zig" | "haskell" - | "perl" | "r" | "julia" | "sql" | "matlab"; + | "perl" | "r" | "julia" | "sql" | "matlab" | "shell"; /** * A bug pattern is a rule that identifies a specific class of dangerous code. diff --git a/src/core/mcp-proto-pollution.test.ts b/src/core/mcp-proto-pollution.test.ts new file mode 100644 index 0000000..dcaed88 --- /dev/null +++ b/src/core/mcp-proto-pollution.test.ts @@ -0,0 +1,55 @@ +// Regression test for js-008-prototype-pollution-bracket in +// McpManager.loadFromConfigs. A malicious MCP config could list +// `__proto__`, `constructor`, or `prototype` as a "server name"; +// prior to v2.10.13 this was assigned via bracket notation into a +// plain object literal, poisoning Object.prototype for the rest of +// the process. + +import { describe, expect, test } from "bun:test"; +import { McpManager } from "./mcp"; + +describe("McpManager.loadFromConfigs — prototype pollution", () => { + test("does not pollute Object.prototype via __proto__ server name", async () => { + // Stub network side effects so the test stays hermetic. + const mgr = new McpManager(); + const anyMgr = mgr as unknown as { startServers: () => Promise; startHealthChecks: () => void }; + anyMgr.startServers = async () => { /* no-op */ }; + anyMgr.startHealthChecks = () => { /* no-op */ }; + + // Use JSON.parse so TS doesn't narrow away the __proto__ literal. + const hostile = JSON.parse(`{ + "__proto__": { "polluted": true }, + "constructor": { "polluted": true }, + "prototype": { "polluted": true }, + "legit": { "command": "echo", "args": ["hi"] } + }`); + + // The load call must complete without crashing AND without + // assigning any of the hostile entries onto Object.prototype. + await mgr.loadFromConfigs(hostile); + + const blankProbe: Record = {}; + expect((blankProbe as { polluted?: unknown }).polluted).toBeUndefined(); + expect(Object.prototype).not.toHaveProperty("polluted"); + expect(({} as { polluted?: unknown }).polluted).toBeUndefined(); + }); + + test("legitimate server names are still accepted", async () => { + const mgr = new McpManager(); + const anyMgr = mgr as unknown as { startServers: (c: unknown) => Promise; startHealthChecks: () => void }; + let startedWith: unknown = null; + anyMgr.startServers = async (configs) => { startedWith = configs; }; + anyMgr.startHealthChecks = () => { /* no-op */ }; + + const configs = JSON.parse(`{ + "good-server": { "command": "echo", "args": ["hi"] } + }`); + await mgr.loadFromConfigs(configs); + + expect(startedWith).not.toBeNull(); + const got = startedWith as Record; + expect(got["good-server"]).toBeDefined(); + // Hostile keys should be absent entirely. + expect(got["__proto__"]).toBeUndefined(); + }); +}); diff --git a/src/core/mcp.ts b/src/core/mcp.ts index d783fa3..863e201 100644 --- a/src/core/mcp.ts +++ b/src/core/mcp.ts @@ -161,10 +161,17 @@ export class McpManager { * Used by --mcp-config CLI flag. */ async loadFromConfigs(configs: McpServersConfig): Promise { - const validated: McpServersConfig = {}; + // Reject dangerous keys before assigning — a malicious MCP config + // could use "__proto__", "constructor", or "prototype" as a server + // name to pollute Object.prototype via bracket assignment. + const UNSAFE_KEYS = new Set(["__proto__", "constructor", "prototype"]); + // Use a null-prototype object so even a bypass (e.g., via Proxy + // or some exotic Object.defineProperty trick) cannot touch the + // prototype chain of a normal object. + const validated: McpServersConfig = Object.create(null) as McpServersConfig; for (const [name, config] of Object.entries(configs)) { + if (UNSAFE_KEYS.has(name)) continue; if (isValidServerConfig(config)) { - // KCODE-AUDIT:js-008-prototype-pollution-bracket — Reject __proto__, constructor and prototype keys before assigning. validated[name] = config as McpServerConfig; } } diff --git a/src/core/task-orchestrator/workflow-chain.ts b/src/core/task-orchestrator/workflow-chain.ts index 5e0a8da..03ee20f 100644 --- a/src/core/task-orchestrator/workflow-chain.ts +++ b/src/core/task-orchestrator/workflow-chain.ts @@ -91,13 +91,17 @@ async function stepFix(cwd: string, ctx: Map): Promise<{ output: const { applyFixes } = await import("../audit-engine/fixer"); const data = JSON.parse(readFileSync(jsonPath, "utf-8")); const fixes = applyFixes(data); - const applied = fixes.filter(f => f.applied).length; - const skipped = fixes.filter(f => !f.applied).length; - - ctx.set("fixes_applied", String(applied)); + const transformed = fixes.filter(f => f.kind === "transformed").length; + const annotated = fixes.filter(f => f.kind === "annotated").length; + const skipped = fixes.filter(f => f.kind === "skipped").length; + + // Only "transformed" counts as a real fix. Annotations are advisory + // TODOs that still need manual attention, so don't claim them as done. + ctx.set("fixes_applied", String(transformed)); + ctx.set("fixes_annotated", String(annotated)); return { - output: `${applied} fixes applied, ${skipped} skipped`, - success: applied > 0 || skipped === 0, + output: `${transformed} real fixes, ${annotated} advisory comments, ${skipped} skipped`, + success: transformed > 0 || (annotated === 0 && skipped === 0), }; } diff --git a/src/tools/notebook-utils.test.ts b/src/tools/notebook-utils.test.ts index 0a72096..aff748d 100644 --- a/src/tools/notebook-utils.test.ts +++ b/src/tools/notebook-utils.test.ts @@ -51,8 +51,16 @@ describe("parseNotebook", () => { expect(() => parseNotebook(nb3)).toThrow("Only nbformat 4"); }); - test("rejects invalid JSON", () => { - expect(() => parseNotebook("not json")).toThrow(); + test("rejects invalid JSON with a descriptive error", () => { + // Bare SyntaxError is unhelpful ("Unexpected token n in JSON at + // position 0"); parseNotebook must wrap it with context so the + // caller knows this was a notebook parse failure. + expect(() => parseNotebook("not json")).toThrow(/Invalid notebook JSON/); + }); + + test("rejects non-object root (e.g., array or string literal)", () => { + expect(() => parseNotebook('"just a string"')).toThrow(/root is not an object/); + expect(() => parseNotebook("[1, 2, 3]")).toThrow("Only nbformat 4"); }); }); diff --git a/src/tools/notebook-utils.ts b/src/tools/notebook-utils.ts index 515eb41..b675af0 100644 --- a/src/tools/notebook-utils.ts +++ b/src/tools/notebook-utils.ts @@ -33,10 +33,24 @@ export interface CellOutput { /** Parse .ipynb JSON content into a typed notebook structure */ export function parseNotebook(content: string): JupyterNotebook { - // KCODE-AUDIT:js-014-json-parse-no-catch — Wrap JSON.parse in try/catch and handle SyntaxError. - const nb = JSON.parse(content); - if (nb.nbformat !== 4) { - throw new Error(`Only nbformat 4 is supported, found: ${nb.nbformat}`); + // Catch malformed .ipynb JSON and re-raise with a useful diagnostic. + // Raw SyntaxError from JSON.parse dumps its position offset but no + // context about which file the caller was trying to read, so the + // user sees "Unexpected token } in JSON at position 1234" instead of + // "Invalid notebook JSON: ...". + let nb: unknown; + try { + nb = JSON.parse(content); + } catch (err) { + const msg = err instanceof Error ? err.message : String(err); + throw new Error(`Invalid notebook JSON: ${msg}`); + } + if (typeof nb !== "object" || nb === null) { + throw new Error("Invalid notebook: root is not an object"); + } + const root = nb as { nbformat?: unknown }; + if (root.nbformat !== 4) { + throw new Error(`Only nbformat 4 is supported, found: ${String(root.nbformat)}`); } return nb as JupyterNotebook; } diff --git a/src/ui/App.tsx b/src/ui/App.tsx index b35fdbd..c0de0d4 100644 --- a/src/ui/App.tsx +++ b/src/ui/App.tsx @@ -325,27 +325,35 @@ export default function App({ config, conversationManager, tools, initialSession return () => clearInterval(timer); }, [engineProgress]); - // Terminal tab title — shows activity status with emojis + // Terminal tab title — professional block-progress indicator useEffect(() => { const isWorking = mode === "responding" || (scanProgress?.active ?? false) || mode === ("escalation" as any); - // Animated title when working, static when idle if (isWorking) { - const frames = ["⚡ KCode", "🔥 KCode", "⚡ KCode", "💥 KCode"]; + const frames = [ + "▰▱▱▱ KCode", + "▰▰▱▱ KCode", + "▰▰▰▱ KCode", + "▰▰▰▰ KCode", + "▱▰▰▰ KCode", + "▱▱▰▰ KCode", + "▱▱▱▰ KCode", + "▱▱▱▱ KCode", + ]; let frame = 0; const timer = setInterval(() => { - process.stdout.write(`\x1b]0;${frames[frame % frames.length]} — working...\x07`); + process.stdout.write(`\x1b]0;${frames[frame % frames.length]}\x07`); frame++; - }, 400); + }, 250); return () => { clearInterval(timer); - process.stdout.write(`\x1b]0;✅ KCode\x07`); + process.stdout.write(`\x1b]0;▪ KCode\x07`); }; } - process.stdout.write(`\x1b]0;✅ KCode\x07`); + process.stdout.write(`\x1b]0;▪ KCode\x07`); }, [mode, scanProgress?.active]); // Ask user if they want to resume the previous session's model. diff --git a/src/ui/actions/file-actions.ts b/src/ui/actions/file-actions.ts index d4e1c98..6594392 100644 --- a/src/ui/actions/file-actions.ts +++ b/src/ui/actions/file-actions.ts @@ -258,28 +258,50 @@ export async function handleFileAction(action: string, ctx: ActionContext): Prom const { applyFixes } = await import("../../core/audit-engine/fixer.js"); const fixes = applyFixes(auditResult); - const applied = fixes.filter((f) => f.applied); - const skipped = fixes.filter((f) => !f.applied); + // Three-way split: transformed (real code change), annotated + // (KCODE-AUDIT advisory comment only — finding still needs a manual + // fix), skipped (nothing applied). The previous UI lumped + // transformed and annotated together as "Applied", which lied to + // the user: they'd see "5 applied" and then discover every + // "fix" was just a TODO comment. + const transformed = fixes.filter((f) => f.kind === "transformed"); + const annotated = fixes.filter((f) => f.kind === "annotated"); + const skipped = fixes.filter((f) => f.kind === "skipped"); const lines: string[] = [ ` KCode Auto-Fixer`, ` Project: ${projectRoot}`, "", - ` ✅ Applied: ${applied.length} fixes`, + ` ✅ Fixed: ${transformed.length} (real code transforms)`, + ` 📝 Annotated: ${annotated.length} (advisory comment only — still needs manual fix)`, ` ⏭ Skipped: ${skipped.length}`, "", ]; - if (applied.length > 0) { - lines.push(" Applied fixes:"); - for (const f of applied) { + if (transformed.length > 0) { + lines.push(" Real fixes (code rewritten):"); + for (const f of transformed) { const rel = f.file.replace(projectRoot + "/", ""); lines.push(` ✅ ${rel}:${f.line} ${f.description}`); } } + if (annotated.length > 0) { + lines.push("", " Advisory annotations (KCODE-AUDIT comments added, code unchanged):"); + for (const f of annotated.slice(0, 10)) { + const rel = f.file.replace(projectRoot + "/", ""); + lines.push(` 📝 ${rel}:${f.line} ${f.pattern_id} — ${f.description}`); + } + if (annotated.length > 10) { + lines.push(` ... and ${annotated.length - 10} more`); + } + lines.push( + " (Use `grep -rn KCODE-AUDIT` to list all advisories in the project.)", + ); + } + if (skipped.length > 0) { - lines.push("", " Skipped (manual fix needed):"); + lines.push("", " Skipped (no fix strategy available):"); for (const f of skipped.slice(0, 10)) { const rel = f.file.replace(projectRoot + "/", ""); lines.push(` ⏭ ${rel}:${f.line} ${f.description}`);