Merge pull request #51 from CodeAnt-AI/pranav/cod-1557-base-ref-injection-symlink-fixes

Block ref injection and out-of-repo symlink reads in --base (0.5.13)

Author: pranavcodeant

changelog.md

@@ -1,5 +1,9 @@
 # Changelog
 
+## [0.5.13] - 13/10/2026
+- Security: `--base` / `--base-commit` values (including from the MCP `codeant_review_local` tool) are resolved without a shell, and option-like values are rejected, so they can no longer run commands or pass options to git
+- Security: symlinks that point outside the repository, and files reached through them, are no longer read into review or secrets payloads. In 0.5.12, `--base` / `--base-commit` sent a committed symlink's target contents
+
 ## [0.5.12] - 13/10/2026
 - `review` / `secrets` with `--base` or `--base-commit` now include staged, unstaged, and untracked files (respecting `.gitignore`), not just commits. Previously `review --all --base main` silently skipped new files
 - With `--base` / `--base-commit`, file content is read from the working tree so it matches the reviewed diff

package-lock.json

@@ -1,6 +1,6 @@
 {
   "name": "codeant-cli",
-  "version": "0.5.12",
+  "version": "0.5.13",
   "description": "Code review CLI tool",
   "type": "module",
   "repository": {

package.json

@@ -1,9 +1,11 @@
 import fs from 'fs/promises';
+import { constants as fsConstants } from 'fs';
 import path from 'path';
 import { promisify } from 'util';
-import { exec } from 'child_process';
+import { exec, execFile } from 'child_process';
 
 const execAsync = promisify(exec);
+const execFileAsync = promisify(execFile);
 
 /**
  * Expand untracked paths from `git status --porcelain`.
@@ -468,18 +470,58 @@ class GitDiffHelper {
     return diffs;
   }
 
+  /**
+   * Read a working-tree file. Returns null if it resolves outside the repository (a symlink
+   * pointing elsewhere) or isn't a regular file, so those contents are never uploaded.
+   */
+  async _readRepoFile(relPath) {
+    const realRoot = await fs.realpath(this.gitRoot);
+    const isInside = p => {
+      const rel = path.relative(realRoot, p);
+      return !!rel && rel !== '..' && !rel.startsWith('..' + path.sep) && !path.isAbsolute(rel);
+    };
+    const realPath = await fs.realpath(path.resolve(this.gitRoot, relPath));
+    if (!isInside(realPath)) return null;
+
+    // Open first and then check what was actually opened, so a symlink swapped in after the
+    // check above can't redirect the read. O_NONBLOCK keeps a FIFO from blocking the open.
+    const { O_RDONLY, O_NOFOLLOW = 0, O_NONBLOCK = 0 } = fsConstants;
+    const handle = await fs.open(realPath, O_RDONLY | O_NOFOLLOW | O_NONBLOCK);
+    try {
+      const opened = await handle.stat();
+      if (!opened.isFile()) return null;
+      // The opened file must still be the file at this path, and the path must still resolve
+      // inside the repo (a parent directory may have been swapped for a symlink)
+      const nowPath = await fs.realpath(realPath);
+      const now = await fs.stat(nowPath);
+      if (!isInside(nowPath) || now.dev !== opened.dev || now.ino !== opened.ino) return null;
+      return await handle.readFile('utf8');
+    } finally {
+      await handle.close();
+    }
+  }
+
   /**
    * Build a synthetic "new file" diff for an untracked file, which git diff can't see.
    * Returns null if the file can't be read.
    */
   async _buildUntrackedFileDiff(file) {
     try {
-      const absolutePath = path.resolve(this.gitRoot, file);
-      const content = await fs.readFile(absolutePath, 'utf8');
+      let content = await this._readRepoFile(file).catch(() => null);
+      let mode = '100644';
+      if (content === null) {
+        // A symlink we won't follow (it points outside the repo or nowhere): show the link
+        // itself, as git would once it's committed, using its target path and never the
+        // target's contents
+        const absolutePath = path.resolve(this.gitRoot, file);
+        if (!(await fs.lstat(absolutePath)).isSymbolicLink()) return null;
+        content = await fs.readlink(absolutePath);
+        mode = '120000';
+      }
       const lines = content.split('\n');
 
       let patchStr = `diff --git a/${file} b/${file}\n`;
-      patchStr += `new file mode 100644\n`;
+      patchStr += `new file mode ${mode}\n`;
       patchStr += `index 0000000..${this._generateRandomHash()}\n`;
       patchStr += `--- /dev/null\n`;
       patchStr += `+++ b/${file}\n`;
@@ -549,8 +591,7 @@ class GitDiffHelper {
     let resolvedRef;
     for (const candidate of [branch, `origin/${branch}`]) {
       try {
-        const { stdout } = await execAsync(`git rev-parse ${candidate}`, { cwd: this.gitRoot });
-        resolvedRef = stdout.trim();
+        resolvedRef = await this._resolveCommitSha(candidate);
         break;
       } catch (err) {
         // try next
@@ -589,15 +630,29 @@ class GitDiffHelper {
     // Resolve the commit ref (supports HEAD~3, sha, tag, etc.)
     let resolvedCommit;
     try {
-      const { stdout } = await execAsync(`git rev-parse ${commit}`, { cwd: this.gitRoot });
-      resolvedCommit = stdout.trim();
+      resolvedCommit = await this._resolveCommitSha(commit);
     } catch (err) {
       throw new Error(`Could not resolve commit: ${commit}`);
     }
 
     return this._getWorkingTreeDiffAgainst(resolvedCommit);
   }
 
+  /**
+   * Resolve a user-supplied ref (from the CLI or MCP) to a commit SHA without going through a
+   * shell, so later git commands only ever see the SHA. Throws if it doesn't name a commit.
+   */
+  async _resolveCommitSha(ref) {
+    // A leading "-" would be parsed as a git option (e.g. --output=<file>)
+    if (ref.startsWith('-')) throw new Error(`Invalid ref: ${ref}`);
+    const { stdout } = await execFileAsync(
+      'git',
+      ['rev-parse', '--verify', '--quiet', `${ref}^{commit}`],
+      { cwd: this.gitRoot }
+    );
+    return stdout.trim();
+  }
+
   /**
    * Diff a base commit against the working tree: commits since base, staged and unstaged
    * changes (including staged new files), plus untracked files not ignored by .gitignore.
@@ -1011,8 +1066,8 @@ class GitDiffHelper {
     } catch (err) {
       // Handle untracked files
       try {
-        const absoluteFilePath = path.resolve(this.gitRoot, filePath);
-        const headFileStr = await fs.readFile(absoluteFilePath, 'utf8');
+        const headFileStr = await this._readRepoFile(filePath);
+        if (headFileStr === null) throw new Error('Not a regular file inside the repository');
         const lines = headFileStr.split('\n');
 
         let patchStr = `diff --git a/${filePath} b/${filePath}\n`;
@@ -1069,9 +1124,9 @@ class GitDiffHelper {
         { cwd: this.gitRoot }
       ).then(r => r.stdout).catch(() => '');
     } else {
-      headFileStr = await fs.readFile(
-        path.join(this.gitRoot, filePath), 'utf8'
-      ).catch(() => '');
+      headFileStr = await this._readRepoFile(filePath)
+        .then(content => content ?? '')
+        .catch(() => '');
     }
 
     // Get patch

src/utils/gitDiffHelper.js

@@ -1,6 +1,7 @@
-import { describe, it, expect, beforeAll, afterAll, beforeEach, afterEach } from 'vitest';
+import { describe, it, expect, vi, beforeAll, afterAll, beforeEach, afterEach } from 'vitest';
 import { execSync } from 'child_process';
 import fs from 'fs';
+import fsp from 'fs/promises';
 import path from 'path';
 import os from 'os';
 import GitDiffHelper from '../src/utils/gitDiffHelper.js';
@@ -1134,4 +1135,143 @@ describe('GitDiffHelper integration tests', () => {
       ).rejects.toThrow();
     });
   });
+
+  // ─────────────────────────────────────────────────────
+  // SECURITY: user-supplied refs and symlinks
+  // ─────────────────────────────────────────────────────
+  describe('security', () => {
+    let outsideDir;
+    let startBranch;
+
+    beforeEach(() => {
+      startBranch = git('rev-parse --abbrev-ref HEAD');
+      git('checkout -- .');
+      git('reset HEAD -- .');
+      git('clean -fd');
+      outsideDir = fs.mkdtempSync(path.join(os.tmpdir(), 'codeant-outside-'));
+      fs.writeFileSync(path.join(outsideDir, 'creds.txt'), 'OUTSIDE_SECRET\n');
+      fs.mkdirSync(path.join(outsideDir, 'dir'));
+      fs.writeFileSync(path.join(outsideDir, 'dir', 'inner.txt'), 'OUTSIDE_DIR_SECRET\n');
+    });
+
+    afterEach(() => {
+      git('reset HEAD -- .');
+      git('checkout -- .');
+      git('clean -fd');
+      if (git('rev-parse --abbrev-ref HEAD') !== startBranch) {
+        git(`checkout ${startBranch}`);
+      }
+      fs.rmSync(outsideDir, { recursive: true, force: true });
+    });
+
+    const leaked = diffs => diffs.filter(d => /OUTSIDE_(DIR_)?SECRET/.test(d.head_file_str + d.patch_str));
+
+    it('does not run shell metacharacters in a base branch or commit', async () => {
+      const marker = path.join(outsideDir, 'pwned');
+      const helper = await createHelper();
+
+      await expect(
+        helper.getDiffBasedOnReviewConfig({ type: 'base-branch', baseBranch: `HEAD; touch ${marker}` })
+      ).rejects.toThrow('Could not resolve branch');
+      await expect(
+        helper.getDiffBasedOnReviewConfig({ type: 'base-commit', baseCommit: `HEAD$(touch ${marker})` })
+      ).rejects.toThrow('Could not resolve commit');
+
+      expect(fs.existsSync(marker)).toBe(false);
+    });
+
+    it('rejects option-like refs instead of passing them to git', async () => {
+      const written = path.join(outsideDir, 'written.txt');
+      const helper = await createHelper();
+
+      await expect(
+        helper.getDiffBasedOnReviewConfig({ type: 'base-commit', baseCommit: `--output=${written}` })
+      ).rejects.toThrow('Could not resolve commit');
+
+      expect(fs.existsSync(written)).toBe(false);
+    });
+
+    it('does not read untracked symlinks that point outside the repo', async () => {
+      fs.symlinkSync(path.join(outsideDir, 'creds.txt'), path.join(testDir, 'src/link-file'));
+      fs.symlinkSync(path.join(outsideDir, 'dir'), path.join(testDir, 'src/link-dir'));
+
+      const helper = await createHelper();
+      for (const config of [
+        { type: 'base-commit', baseCommit: 'HEAD' },
+        { type: 'uncommitted' },
+        { type: 'all' },
+      ]) {
+        const diffs = await helper.getDiffBasedOnReviewConfig(config);
+        expect(leaked(diffs)).toEqual([]);
+      }
+    });
+
+    it('shows untracked symlinks that point outside the repo as links, not their contents', async () => {
+      const target = path.join(outsideDir, 'creds.txt');
+      fs.symlinkSync(target, path.join(testDir, 'src/link-file'));
+
+      const helper = await createHelper();
+      const diffs = await helper.getDiffBasedOnReviewConfig({ type: 'base-commit', baseCommit: 'HEAD' });
+
+      const diff = diffs.find(d => d.filename_str === 'src/link-file');
+      expect(diff).toBeDefined();
+      expect(diff.edit_type_str).toBe('ADDED');
+      expect(diff.head_file_str).toBe(target);
+      expect(diff.patch_str).toContain('new file mode 120000');
+      expect(leaked(diffs)).toEqual([]);
+    });
+
+    it('does not follow a symlink swapped in after the path check', async () => {
+      writeFile('src/race/data.txt', 'inside\n');
+      fs.writeFileSync(path.join(outsideDir, 'data.txt'), 'OUTSIDE_SECRET\n');
+      const raceDir = path.join(testDir, 'src/race');
+      const realOpen = fsp.open;
+      let swapped = false;
+      // Swap the parent directory for a link to outside the repo between the check and the open
+      const spy = vi.spyOn(fsp, 'open').mockImplementation(async (file, ...rest) => {
+        if (!swapped && String(file).endsWith(path.join('race', 'data.txt'))) {
+          swapped = true;
+          fs.renameSync(raceDir, raceDir + '-orig');
+          fs.symlinkSync(outsideDir, raceDir);
+        }
+        return realOpen(file, ...rest);
+      });
+
+      try {
+        const helper = await createHelper();
+        const diffs = await helper.getDiffBasedOnReviewConfig({ type: 'uncommitted' });
+        expect(swapped).toBe(true);
+        expect(leaked(diffs)).toEqual([]);
+      } finally {
+        spy.mockRestore();
+        fs.rmSync(raceDir, { recursive: true, force: true });
+        fs.rmSync(raceDir + '-orig', { recursive: true, force: true });
+      }
+    });
+
+    it('does not read committed symlinks that point outside the repo', async () => {
+      const baseHash = git('rev-parse HEAD');
+      fs.symlinkSync(path.join(outsideDir, 'creds.txt'), path.join(testDir, 'src/committed-link'));
+      git('checkout -b security-committed-link');
+      git('add src/committed-link');
+      git('commit -m "committed link"');
+
+      const helper = await createHelper();
+      const diffs = await helper.getDiffBasedOnReviewConfig({ type: 'base-commit', baseCommit: baseHash });
+
+      expect(diffs.map(d => d.filename_str)).toContain('src/committed-link');
+      expect(leaked(diffs)).toEqual([]);
+    });
+
+    it('still reads symlinks that stay inside the repo', async () => {
+      fs.symlinkSync('index.js', path.join(testDir, 'src/inside-link.js'));
+
+      const helper = await createHelper();
+      const diffs = await helper.getDiffBasedOnReviewConfig({ type: 'base-commit', baseCommit: 'HEAD' });
+
+      const diff = diffs.find(d => d.filename_str === 'src/inside-link.js');
+      expect(diff).toBeDefined();
+      expect(diff.head_file_str).toBe(readFile('src/index.js'));
+    });
+  });
 });