From 856f98023c83397d2693f13ff13c26ef9b7a5afa Mon Sep 17 00:00:00 2001 From: Lawyered Date: Sun, 22 Feb 2026 17:10:45 -0500 Subject: [PATCH] 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 | 65 ++++++++++++ skills-engine/__tests__/path-remap.test.ts | 91 ++++++++++++++++ skills-engine/__tests__/update.test.ts | 8 +- skills-engine/path-remap.ts | 117 ++++++++++++++++++++- 4 files changed, 274 insertions(+), 7 deletions(-) diff --git a/skills-engine/__tests__/apply.test.ts b/skills-engine/__tests__/apply.test.ts index cc0f893..bb41f32 100644 --- a/skills-engine/__tests__/apply.test.ts +++ b/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 }); + } + }); }); diff --git a/skills-engine/__tests__/path-remap.test.ts b/skills-engine/__tests__/path-remap.test.ts index 04a7363..8ba3d39 100644 --- a/skills-engine/__tests__/path-remap.test.ts +++ b/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); + }); }); }); diff --git a/skills-engine/__tests__/update.test.ts b/skills-engine/__tests__/update.test.ts index 1ad17f1..1e2646f 100644 --- a/skills-engine/__tests__/update.test.ts +++ b/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); }); diff --git a/skills-engine/path-remap.ts b/skills-engine/path-remap.ts index ddd1f9e..cd498a4 100644 --- a/skills-engine/path-remap.ts +++ b/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, + mode: 'throw' | 'drop', +): Record { + const projectRoot = process.cwd(); + const sanitized: Record = {}; + + 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 { - 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 { const state = readState(); - return state.path_remap ?? {}; + const remap = state.path_remap ?? {}; + return sanitizeRemapEntries(remap, 'drop'); } export function recordPathRemap(remap: Record): 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); }