diff --git a/src/cli/commands/review.test.ts b/src/cli/commands/review.test.ts new file mode 100644 index 0000000..022f65d --- /dev/null +++ b/src/cli/commands/review.test.ts @@ -0,0 +1,115 @@ +import { afterEach, describe, expect, test } from "bun:test"; +import { execFileSync, spawnSync } from "node:child_process"; +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +const cliEntry = join(import.meta.dir, "..", "index.tsx"); +const syntheticGitHubToken = ["ghp", "A".repeat(36)].join("_"); + +describe("review staged-diff boundary", () => { + let fixtureRoot = ""; + + afterEach(() => { + if (fixtureRoot) rmSync(fixtureRoot, { recursive: true, force: true }); + fixtureRoot = ""; + }); + + function createRepo(name: string): string { + if (!fixtureRoot) fixtureRoot = mkdtempSync(join(tmpdir(), "shield-review-")); + const repo = join(fixtureRoot, name); + mkdirSync(repo); + execFileSync("git", ["init", "-q"], { cwd: repo }); + execFileSync("git", ["config", "user.email", "fixture@example.test"], { cwd: repo }); + execFileSync("git", ["config", "user.name", "Fixture"], { cwd: repo }); + return repo; + } + + function commitAll(repo: string, message: string): void { + execFileSync("git", ["add", "-A"], { cwd: repo }); + execFileSync("git", ["commit", "-qm", message], { cwd: repo }); + } + + function runReview(repo: string) { + return spawnSync("bun", ["run", cliEntry, "review"], { + cwd: repo, + encoding: "utf-8", + env: { + ...process.env, + HOME: join(fixtureRoot, "home"), + USERPROFILE: join(fixtureRoot, "home"), + SECURITY_DB: join(fixtureRoot, "shield.db"), + CEREBRAS_API_KEY: "", + NO_COLOR: "1", + }, + }); + } + + test("reports added vulnerable lines and excludes unchanged vulnerable lines", () => { + const unchangedRepo = createRepo("unchanged"); + writeFileSync(join(unchangedRepo, "sample.ts"), "target.innerHTML = input;\n"); + commitAll(unchangedRepo, "baseline vulnerable line"); + writeFileSync( + join(unchangedRepo, "sample.ts"), + "target.innerHTML = input;\nconst safe = true;\n", + ); + execFileSync("git", ["add", "sample.ts"], { cwd: unchangedRepo }); + + const unchanged = runReview(unchangedRepo); + expect(unchanged.status).toBe(0); + expect(unchanged.stderr).toBe(""); + expect(unchanged.stdout).toContain("No security issues found in staged changes."); + expect(unchanged.stdout).not.toContain("sample.ts:1"); + + const addedRepo = createRepo("added"); + writeFileSync(join(addedRepo, "sample.ts"), "const safe = true;\n"); + commitAll(addedRepo, "safe baseline"); + writeFileSync( + join(addedRepo, "sample.ts"), + "const safe = true;\ntarget.innerHTML = input;\n", + ); + execFileSync("git", ["add", "sample.ts"], { cwd: addedRepo }); + + const added = runReview(addedRepo); + expect(added.status).toBe(0); + expect(added.stderr).toBe(""); + expect(added.stdout).toContain("HIGH sample.ts:2"); + expect(added.stdout).not.toContain("No security issues found in staged changes."); + }); + + test("scans staged test files with the same coverage as non-test files", () => { + const testRepo = createRepo("test-file"); + writeFileSync( + join(testRepo, "fixture.test.ts"), + `const token = "${syntheticGitHubToken}";\n`, + ); + execFileSync("git", ["add", "fixture.test.ts"], { cwd: testRepo }); + + const testFile = runReview(testRepo); + expect(testFile.status).toBe(0); + expect(testFile.stderr).toBe(""); + expect(testFile.stdout).toContain("CRITICAL fixture.test.ts:1"); + + const sourceRepo = createRepo("source-file"); + writeFileSync(join(sourceRepo, "fixture.ts"), `const token = "${syntheticGitHubToken}";\n`); + execFileSync("git", ["add", "fixture.ts"], { cwd: sourceRepo }); + + const sourceFile = runReview(sourceRepo); + expect(sourceFile.status).toBe(0); + expect(sourceFile.stderr).toBe(""); + expect(sourceFile.stdout).toContain("CRITICAL fixture.ts:1"); + }); + + test("scans the staged index content rather than later unstaged edits", () => { + const repo = createRepo("index-content"); + writeFileSync(join(repo, "fixture.ts"), "const safe = true;\n"); + execFileSync("git", ["add", "fixture.ts"], { cwd: repo }); + writeFileSync(join(repo, "fixture.ts"), `const token = "${syntheticGitHubToken}";\n`); + + const result = runReview(repo); + expect(result.status).toBe(0); + expect(result.stderr).toBe(""); + expect(result.stdout).toContain("No security issues found in staged changes."); + expect(result.stdout).not.toContain("CRITICAL fixture.ts:1"); + }); +}); diff --git a/src/cli/commands/review.ts b/src/cli/commands/review.ts index 06537a5..6cad9f5 100644 --- a/src/cli/commands/review.ts +++ b/src/cli/commands/review.ts @@ -1,58 +1,148 @@ import type { Command } from "commander"; -import { execSync } from "child_process"; +import { execFileSync } from "node:child_process"; +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { dirname, isAbsolute, join, normalize, sep } from "node:path"; import chalk from "chalk"; -import { ScannerType, ReportFormat, type Finding } from "../../types/index.js"; +import { + ScannerType, + ReportFormat, + type Finding, + type FindingInput, +} from "../../types/index.js"; import { runScanner } from "../../scanners/index.js"; import { getReporter } from "../../reporters/index.js"; -import { loadConfig } from "../../lib/index.js"; + +interface StagedFile { + path: string; + addedLines: Set; +} + +function readGitPaths(cwd: string, diffFilter?: string): string[] { + const args = ["diff", "--cached", "--name-only", "-z"]; + if (diffFilter) args.push(`--diff-filter=${diffFilter}`); + const output = execFileSync("git", args, { cwd, encoding: "buffer" }); + return output.toString("utf-8").split("\0").filter(Boolean); +} + +function safeStagedPath(filePath: string): string { + const normalized = normalize(filePath); + if ( + !filePath || + isAbsolute(filePath) || + normalized === ".." || + normalized.startsWith(`..${sep}`) + ) { + throw new Error("Staged path escapes the repository boundary"); + } + return normalized; +} + +function addedLinesForFile(cwd: string, filePath: string): Set { + const diff = execFileSync( + "git", + ["diff", "--cached", "--unified=0", "--no-color", "--no-ext-diff", "--", filePath], + { cwd, encoding: "utf-8" }, + ); + const addedLines = new Set(); + + for (const line of diff.split("\n")) { + const hunk = /^@@ -\d+(?:,\d+)? \+(\d+)(?:,(\d+))? @@/.exec(line); + if (!hunk) continue; + const start = Number.parseInt(hunk[1], 10); + const count = hunk[2] === undefined ? 1 : Number.parseInt(hunk[2], 10); + for (let offset = 0; offset < count; offset++) addedLines.add(start + offset); + } + + return addedLines; +} + +function findingTouchesAddedLine(finding: FindingInput, addedLines: Set): boolean { + const endLine = finding.end_line ?? finding.line; + for (let line = finding.line; line <= endLine; line++) { + if (addedLines.has(line)) return true; + } + return false; +} + +async function scanStagedFiles(cwd: string, stagedFiles: StagedFile[]): Promise { + const snapshotRoot = mkdtempSync(join(tmpdir(), "shield-staged-review-")); + try { + const addedLinesByPath = new Map>(); + for (const stagedFile of stagedFiles) { + const filePath = safeStagedPath(stagedFile.path); + const snapshotPath = join(snapshotRoot, filePath); + mkdirSync(dirname(snapshotPath), { recursive: true }); + const content = execFileSync("git", ["show", `:${stagedFile.path}`], { + cwd, + encoding: "buffer", + }); + writeFileSync(snapshotPath, content); + addedLinesByPath.set(filePath.split(sep).join("/"), stagedFile.addedLines); + } + + const findings: FindingInput[] = []; + for (const scannerType of [ScannerType.Secrets, ScannerType.Code]) { + const results = await runScanner(scannerType, snapshotRoot, { + // A staged review promises coverage of every staged hunk. Repository-wide + // ignore patterns are appropriate for broad scans, not an explicit diff. + ignore_patterns: [], + }); + findings.push( + ...results.filter((finding) => { + const addedLines = addedLinesByPath.get(finding.file.split(sep).join("/")); + return addedLines !== undefined && findingTouchesAddedLine(finding, addedLines); + }), + ); + } + return findings; + } finally { + rmSync(snapshotRoot, { recursive: true, force: true }); + } +} export function registerReviewCommand(program: Command): void { program .command("review") .description("Security review staged git changes") .action(async () => { - let diff: string; + let allChangedFiles: string[]; + let stagedFiles: StagedFile[]; try { - diff = execSync("git diff --staged", { encoding: "utf-8" }); + const cwd = process.cwd(); + allChangedFiles = readGitPaths(cwd); + stagedFiles = readGitPaths(cwd, "ACMR").map((path) => ({ + path, + addedLines: addedLinesForFile(cwd, path), + })); } catch { console.error(chalk.red("\n Failed to get staged diff. Are you in a git repo?\n")); process.exit(1); return; } - if (!diff.trim()) { + if (allChangedFiles.length === 0) { console.log(chalk.yellow("\n No staged changes to review.\n")); return; } console.log(chalk.bold("\n Reviewing staged changes...\n")); - const changedFiles = diff - .split("\n") - .filter((line) => line.startsWith("+++ b/")) - .map((line) => line.replace("+++ b/", "")); - - if (changedFiles.length === 0) { + if (stagedFiles.length === 0) { console.log(chalk.green(" No files in staged diff to review.\n")); return; } - console.log(chalk.gray(` Checking ${changedFiles.length} changed file(s)...`)); + console.log(chalk.gray(` Checking ${stagedFiles.length} changed file(s)...`)); const cwd = process.cwd(); - const config = loadConfig(cwd); - const findingInputs: any[] = []; - - for (const scannerType of [ScannerType.Secrets, ScannerType.Code]) { - try { - const results = await runScanner(scannerType, cwd, { - ignore_patterns: config.ignore_patterns, - }); - const filtered = results.filter((f) => - changedFiles.some((cf) => f.file.endsWith(cf) || f.file === cf), - ); - findingInputs.push(...filtered); - } catch {} + let findingInputs: FindingInput[]; + try { + findingInputs = await scanStagedFiles(cwd, stagedFiles); + } catch { + console.error(chalk.red("\n Failed to scan the staged diff.\n")); + process.exit(1); + return; } if (findingInputs.length === 0) {