fix(core): use execFileSync to prevent shell injection and remove dangling edges in merge

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
Lum1104
2026-03-14 18:47:33 +08:00
co-authored by Claude Opus 4.6
parent 19357d25a3
commit 8876eec80e
2 changed files with 18 additions and 17 deletions
+13 -12
View File
@@ -2,14 +2,14 @@ import { describe, it, expect, vi, beforeEach } from "vitest";
import type { KnowledgeGraph, GraphNode, GraphEdge } from "../types.js";
vi.mock("child_process", () => ({
execSync: vi.fn(),
execFileSync: vi.fn(),
}));
// Import after mocking
import { execSync } from "child_process";
import { execFileSync } from "child_process";
import { getChangedFiles, isStale, mergeGraphUpdate } from "../staleness.js";
const mockedExecSync = vi.mocked(execSync);
const mockedExecFileSync = vi.mocked(execFileSync);
const makeNode = (
overrides: Partial<GraphNode> & { id: string; name: string },
@@ -55,19 +55,20 @@ beforeEach(() => {
describe("getChangedFiles", () => {
it("returns changed file list from git diff", () => {
mockedExecSync.mockReturnValue("src/index.ts\nsrc/utils.ts\n");
mockedExecFileSync.mockReturnValue("src/index.ts\nsrc/utils.ts\n");
const result = getChangedFiles("/project", "abc123");
expect(result).toEqual(["src/index.ts", "src/utils.ts"]);
expect(mockedExecSync).toHaveBeenCalledWith(
"git diff abc123..HEAD --name-only",
expect(mockedExecFileSync).toHaveBeenCalledWith(
"git",
["diff", "abc123..HEAD", "--name-only"],
{ cwd: "/project", encoding: "utf-8" },
);
});
it("returns empty array when no changes", () => {
mockedExecSync.mockReturnValue("");
mockedExecFileSync.mockReturnValue("");
const result = getChangedFiles("/project", "abc123");
@@ -75,7 +76,7 @@ describe("getChangedFiles", () => {
});
it("returns empty array on git error", () => {
mockedExecSync.mockImplementation(() => {
mockedExecFileSync.mockImplementation(() => {
throw new Error("fatal: bad revision");
});
@@ -87,7 +88,7 @@ describe("getChangedFiles", () => {
describe("isStale", () => {
it("returns stale when files have changed", () => {
mockedExecSync.mockReturnValue("src/index.ts\n");
mockedExecFileSync.mockReturnValue("src/index.ts\n");
const result = isStale("/project", "abc123");
@@ -98,7 +99,7 @@ describe("isStale", () => {
});
it("returns not stale when no files changed", () => {
mockedExecSync.mockReturnValue("");
mockedExecFileSync.mockReturnValue("");
const result = isStale("/project", "abc123");
@@ -223,12 +224,12 @@ describe("mergeGraphUpdate", () => {
),
).toBeDefined();
// Edge to changed file from unchanged should remain
// Edge to changed file from unchanged should be removed (dangling target)
expect(
result.edges.find(
(e) => e.source === "file-c" && e.target === "file-a",
),
).toBeDefined();
).toBeUndefined();
// New edge should be added
expect(
+5 -5
View File
@@ -1,4 +1,4 @@
import { execSync } from "child_process";
import { execFileSync } from "child_process";
import type { KnowledgeGraph, GraphNode, GraphEdge } from "./types.js";
export interface StalenessResult {
@@ -15,7 +15,7 @@ export function getChangedFiles(
lastCommitHash: string,
): string[] {
try {
const output = execSync(`git diff ${lastCommitHash}..HEAD --name-only`, {
const output = execFileSync('git', ['diff', `${lastCommitHash}..HEAD`, '--name-only'], {
cwd: projectDir,
encoding: "utf-8",
});
@@ -46,7 +46,7 @@ export function isStale(
* Merge new analysis results into an existing knowledge graph.
*
* 1. Remove old nodes belonging to changed files (matched by filePath).
* 2. Remove old edges where the SOURCE node belongs to a changed file.
* 2. Remove old edges where the SOURCE or TARGET node belongs to a changed file.
* 3. Add new nodes and edges.
* 4. Update project.gitCommitHash and project.analyzedAt.
* 5. Return the merged graph.
@@ -72,9 +72,9 @@ export function mergeGraphUpdate(
(node) => !removedNodeIds.has(node.id),
);
// Keep edges whose source node is not in the removed set
// Keep edges whose source or target node is not in the removed set
const retainedEdges = existingGraph.edges.filter(
(edge) => !removedNodeIds.has(edge.source),
(edge) => !removedNodeIds.has(edge.source) && !removedNodeIds.has(edge.target),
);
return {