From 8876eec80ecf3b090189a1b8bf503993c1860374 Mon Sep 17 00:00:00 2001 From: Lum1104 Date: Sat, 14 Mar 2026 18:47:33 +0800 Subject: [PATCH] fix(core): use execFileSync to prevent shell injection and remove dangling edges in merge Co-Authored-By: Claude Opus 4.6 --- packages/core/src/__tests__/staleness.test.ts | 25 ++++++++++--------- packages/core/src/staleness.ts | 10 ++++---- 2 files changed, 18 insertions(+), 17 deletions(-) diff --git a/packages/core/src/__tests__/staleness.test.ts b/packages/core/src/__tests__/staleness.test.ts index 4a63d72..23b9636 100644 --- a/packages/core/src/__tests__/staleness.test.ts +++ b/packages/core/src/__tests__/staleness.test.ts @@ -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 & { 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( diff --git a/packages/core/src/staleness.ts b/packages/core/src/staleness.ts index 780f42c..0750ba3 100644 --- a/packages/core/src/staleness.ts +++ b/packages/core/src/staleness.ts @@ -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 {