Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
115 changes: 115 additions & 0 deletions src/cli/commands/review.test.ts
Original file line number Diff line number Diff line change
@@ -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");
});
});
142 changes: 116 additions & 26 deletions src/cli/commands/review.ts
Original file line number Diff line number Diff line change
@@ -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<number>;
}

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<number> {
const diff = execFileSync(
"git",
["diff", "--cached", "--unified=0", "--no-color", "--no-ext-diff", "--", filePath],
{ cwd, encoding: "utf-8" },
);
const addedLines = new Set<number>();

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<number>): 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<FindingInput[]> {
const snapshotRoot = mkdtempSync(join(tmpdir(), "shield-staged-review-"));
try {
const addedLinesByPath = new Map<string, Set<number>>();
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) {
Expand Down
Loading