Fix critical skills path-remap root escape (including symlink traversal) (#367)

* Block skills path-remap escapes outside project root

* Harden path remap against symlink-based root escape

* test: isolate update tests from real git index

skills-engine/__tests__/apply.test.ts

@@ -11,6 +11,7 @@ import {
   initGitRepo,
   setupNanoclawDir,
 } from './test-helpers.js';
+import { readState, writeState } from '../state.js';
 
 describe('apply', () => {
   let tmpDir: string;
@@ -89,4 +90,68 @@ describe('apply', () => {
     // Added file should be cleaned up
     expect(fs.existsSync(path.join(tmpDir, 'src/added.ts'))).toBe(false);
   });
+
+  it('does not allow path_remap to write files outside project root', async () => {
+    const state = readState();
+    state.path_remap = { 'src/newfile.ts': '../../outside.txt' };
+    writeState(state);
+
+    const skillDir = createSkillPackage(tmpDir, {
+      skill: 'remap-escape',
+      version: '1.0.0',
+      core_version: '1.0.0',
+      adds: ['src/newfile.ts'],
+      modifies: [],
+      addFiles: { 'src/newfile.ts': 'safe content' },
+    });
+
+    const result = await applySkill(skillDir);
+    expect(result.success).toBe(true);
+
+    // Remap escape is ignored; file remains constrained inside project root.
+    expect(fs.existsSync(path.join(tmpDir, 'src/newfile.ts'))).toBe(true);
+    expect(fs.existsSync(path.join(tmpDir, '..', 'outside.txt'))).toBe(false);
+  });
+
+  it('does not allow path_remap symlink targets to write outside project root', async () => {
+    const outsideDir = fs.mkdtempSync(
+      path.join(path.dirname(tmpDir), 'nanoclaw-remap-outside-'),
+    );
+    const linkPath = path.join(tmpDir, 'link-out');
+
+    try {
+      fs.symlinkSync(outsideDir, linkPath);
+    } catch (err) {
+      const code = (err as NodeJS.ErrnoException).code;
+      if (code === 'EPERM' || code === 'EACCES' || code === 'ENOSYS') {
+        fs.rmSync(outsideDir, { recursive: true, force: true });
+        return;
+      }
+      fs.rmSync(outsideDir, { recursive: true, force: true });
+      throw err;
+    }
+
+    try {
+      const state = readState();
+      state.path_remap = { 'src/newfile.ts': 'link-out/pwned.txt' };
+      writeState(state);
+
+      const skillDir = createSkillPackage(tmpDir, {
+        skill: 'remap-symlink-escape',
+        version: '1.0.0',
+        core_version: '1.0.0',
+        adds: ['src/newfile.ts'],
+        modifies: [],
+        addFiles: { 'src/newfile.ts': 'safe content' },
+      });
+
+      const result = await applySkill(skillDir);
+      expect(result.success).toBe(true);
+
+      expect(fs.existsSync(path.join(tmpDir, 'src/newfile.ts'))).toBe(true);
+      expect(fs.existsSync(path.join(outsideDir, 'pwned.txt'))).toBe(false);
+    } finally {
+      fs.rmSync(outsideDir, { recursive: true, force: true });
+    }
+  });
 });

skills-engine/__tests__/path-remap.test.ts

@@ -1,6 +1,9 @@
+import fs from 'fs';
+import path from 'path';
 import { afterEach, beforeEach, describe, expect, it } from 'vitest';
 
 import { loadPathRemap, recordPathRemap, resolvePathRemap } from '../path-remap.js';
+import { readState, writeState } from '../state.js';
 import {
   cleanup,
   createMinimalState,
@@ -38,6 +41,43 @@ describe('path-remap', () => {
     it('returns original path when remap is empty', () => {
       expect(resolvePathRemap('src/file.ts', {})).toBe('src/file.ts');
     });
+
+    it('ignores remap entries that escape project root', () => {
+      const remap = { 'src/file.ts': '../../outside.txt' };
+      expect(resolvePathRemap('src/file.ts', remap)).toBe('src/file.ts');
+    });
+
+    it('ignores remap target that resolves through symlink outside project root', () => {
+      const outsideDir = fs.mkdtempSync(
+        path.join(path.dirname(tmpDir), 'nanoclaw-remap-outside-'),
+      );
+      const linkPath = path.join(tmpDir, 'link-out');
+
+      try {
+        fs.symlinkSync(outsideDir, linkPath);
+      } catch (err) {
+        const code = (err as NodeJS.ErrnoException).code;
+        if (code === 'EPERM' || code === 'EACCES' || code === 'ENOSYS') {
+          fs.rmSync(outsideDir, { recursive: true, force: true });
+          return;
+        }
+        fs.rmSync(outsideDir, { recursive: true, force: true });
+        throw err;
+      }
+
+      try {
+        const remap = { 'src/file.ts': 'link-out/pwned.txt' };
+        expect(resolvePathRemap('src/file.ts', remap)).toBe('src/file.ts');
+      } finally {
+        fs.rmSync(outsideDir, { recursive: true, force: true });
+      }
+    });
+
+    it('throws when requested path itself escapes project root', () => {
+      expect(() => resolvePathRemap('../../outside.txt', {})).toThrow(
+        /escapes project root/i,
+      );
+    });
   });
 
   describe('loadPathRemap', () => {
@@ -51,6 +91,51 @@ describe('path-remap', () => {
       const remap = loadPathRemap();
       expect(remap).toEqual({ 'src/a.ts': 'src/b.ts' });
     });
+
+    it('drops unsafe remap entries stored in state', () => {
+      const state = readState();
+      state.path_remap = {
+        'src/a.ts': 'src/b.ts',
+        'src/evil.ts': '../../outside.txt',
+      };
+      writeState(state);
+
+      const remap = loadPathRemap();
+      expect(remap).toEqual({ 'src/a.ts': 'src/b.ts' });
+    });
+
+    it('drops symlink-based escape entries stored in state', () => {
+      const outsideDir = fs.mkdtempSync(
+        path.join(path.dirname(tmpDir), 'nanoclaw-remap-outside-'),
+      );
+      const linkPath = path.join(tmpDir, 'link-out');
+
+      try {
+        fs.symlinkSync(outsideDir, linkPath);
+      } catch (err) {
+        const code = (err as NodeJS.ErrnoException).code;
+        if (code === 'EPERM' || code === 'EACCES' || code === 'ENOSYS') {
+          fs.rmSync(outsideDir, { recursive: true, force: true });
+          return;
+        }
+        fs.rmSync(outsideDir, { recursive: true, force: true });
+        throw err;
+      }
+
+      try {
+        const state = readState();
+        state.path_remap = {
+          'src/a.ts': 'src/b.ts',
+          'src/evil.ts': 'link-out/pwned.txt',
+        };
+        writeState(state);
+
+        const remap = loadPathRemap();
+        expect(remap).toEqual({ 'src/a.ts': 'src/b.ts' });
+      } finally {
+        fs.rmSync(outsideDir, { recursive: true, force: true });
+      }
+    });
   });
 
   describe('recordPathRemap', () => {
@@ -73,5 +158,11 @@ describe('path-remap', () => {
       recordPathRemap({ 'src/a.ts': 'src/c.ts' });
       expect(loadPathRemap()).toEqual({ 'src/a.ts': 'src/c.ts' });
     });
+
+    it('rejects unsafe remap entries', () => {
+      expect(() =>
+        recordPathRemap({ 'src/a.ts': '../../outside.txt' }),
+      ).toThrow(/escapes project root/i);
+    });
   });
 });

skills-engine/__tests__/update.test.ts

@@ -1,23 +1,23 @@
 import fs from 'fs';
 import path from 'path';
-import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
+import { afterEach, beforeEach, describe, expect, it } from 'vitest';
 import { stringify } from 'yaml';
 
 import { cleanup, createTempDir, initGitRepo, setupNanoclawDir } from './test-helpers.js';
 
-// We need to mock process.cwd() since update.ts uses it
 let tmpDir: string;
+const originalCwd = process.cwd();
 
 describe('update', () => {
   beforeEach(() => {
     tmpDir = createTempDir();
     setupNanoclawDir(tmpDir);
     initGitRepo(tmpDir);
-    vi.spyOn(process, 'cwd').mockReturnValue(tmpDir);
+    process.chdir(tmpDir);
   });
 
   afterEach(() => {
-    vi.restoreAllMocks();
+    process.chdir(originalCwd);
     cleanup(tmpDir);
   });
 

skills-engine/path-remap.ts

@@ -1,19 +1,130 @@
+import fs from 'fs';
+import path from 'path';
+
 import { readState, writeState } from './state.js';
 
+function isWithinRoot(rootPath: string, targetPath: string): boolean {
+  return targetPath === rootPath || targetPath.startsWith(rootPath + path.sep);
+}
+
+function nearestExistingPathOrSymlink(candidateAbsPath: string): string {
+  let current = candidateAbsPath;
+  while (true) {
+    try {
+      fs.lstatSync(current);
+      return current;
+    } catch {
+      const parent = path.dirname(current);
+      if (parent === current) {
+        throw new Error(`Invalid remap path: "${candidateAbsPath}"`);
+      }
+      current = parent;
+    }
+  }
+}
+
+function toSafeProjectRelativePath(
+  candidatePath: string,
+  projectRoot: string,
+): string {
+  if (typeof candidatePath !== 'string' || candidatePath.trim() === '') {
+    throw new Error(`Invalid remap path: "${candidatePath}"`);
+  }
+
+  const root = path.resolve(projectRoot);
+  const realRoot = fs.realpathSync(root);
+  const resolved = path.resolve(root, candidatePath);
+  if (
+    !resolved.startsWith(root + path.sep) &&
+    resolved !== root
+  ) {
+    throw new Error(`Path remap escapes project root: "${candidatePath}"`);
+  }
+  if (resolved === root) {
+    throw new Error(`Path remap points to project root: "${candidatePath}"`);
+  }
+
+  // Detect symlink escapes by resolving the nearest existing ancestor/symlink.
+  const anchorPath = nearestExistingPathOrSymlink(resolved);
+  const anchorStat = fs.lstatSync(anchorPath);
+  let realAnchor: string;
+
+  if (anchorStat.isSymbolicLink()) {
+    const linkTarget = fs.readlinkSync(anchorPath);
+    const linkResolved = path.resolve(path.dirname(anchorPath), linkTarget);
+    realAnchor = fs.realpathSync(linkResolved);
+  } else {
+    realAnchor = fs.realpathSync(anchorPath);
+  }
+
+  const relativeRemainder = path.relative(anchorPath, resolved);
+  const realResolved = relativeRemainder
+    ? path.resolve(realAnchor, relativeRemainder)
+    : realAnchor;
+
+  if (!isWithinRoot(realRoot, realResolved)) {
+    throw new Error(
+      `Path remap escapes project root via symlink: "${candidatePath}"`,
+    );
+  }
+
+  return path.relative(realRoot, realResolved);
+}
+
+function sanitizeRemapEntries(
+  remap: Record<string, string>,
+  mode: 'throw' | 'drop',
+): Record<string, string> {
+  const projectRoot = process.cwd();
+  const sanitized: Record<string, string> = {};
+
+  for (const [from, to] of Object.entries(remap)) {
+    try {
+      const safeFrom = toSafeProjectRelativePath(from, projectRoot);
+      const safeTo = toSafeProjectRelativePath(to, projectRoot);
+      sanitized[safeFrom] = safeTo;
+    } catch (err) {
+      if (mode === 'throw') {
+        throw err;
+      }
+    }
+  }
+
+  return sanitized;
+}
+
 export function resolvePathRemap(
   relPath: string,
   remap: Record<string, string>,
 ): string {
-  return remap[relPath] ?? relPath;
+  const projectRoot = process.cwd();
+  const safeRelPath = toSafeProjectRelativePath(relPath, projectRoot);
+  const remapped =
+    remap[safeRelPath] ??
+    remap[relPath];
+
+  if (remapped === undefined) {
+    return safeRelPath;
+  }
+
+  // Fail closed: if remap target is invalid, ignore remap and keep original path.
+  try {
+    return toSafeProjectRelativePath(remapped, projectRoot);
+  } catch {
+    return safeRelPath;
+  }
 }
 
 export function loadPathRemap(): Record<string, string> {
   const state = readState();
-  return state.path_remap ?? {};
+  const remap = state.path_remap ?? {};
+  return sanitizeRemapEntries(remap, 'drop');
 }
 
 export function recordPathRemap(remap: Record<string, string>): void {
   const state = readState();
-  state.path_remap = { ...state.path_remap, ...remap };
+  const existing = sanitizeRemapEntries(state.path_remap ?? {}, 'drop');
+  const incoming = sanitizeRemapEntries(remap, 'throw');
+  state.path_remap = { ...existing, ...incoming };
   writeState(state);
 }