docs(F001): comprehensive implementation plan for security fixes

mcp-server/src/index.ts

@@ -234,7 +234,7 @@ server.setRequestHandler(CallToolRequestSchema, async request => {
         return await implementToolHandler(args || {});
 
       case 'stackshift_cruise_control':
-        return await cruiseControlToolHandler((args as any) || {});
+        return await cruiseControlToolHandler(args || {} as any);
 
       default:
         throw new Error(`Unknown tool: ${name}`);

mcp-server/src/resources/__tests__/security.test.ts

@@ -0,0 +1,148 @@
+import { describe, test, expect, beforeEach, afterEach } from 'vitest';
+import { getStateResource, getProgressResource, getRouteResource } from '../index.js';
+import { mkdtemp, rm, writeFile } from 'fs/promises';
+import { join } from 'path';
+import { tmpdir } from 'os';
+
+describe('Resource Security Tests', () => {
+  let testDir: string;
+  let originalCwd: () => string;
+
+  beforeEach(async () => {
+    // Create temp directory for testing
+    testDir = await mkdtemp(join(tmpdir(), 'stackshift-test-'));
+
+    // Mock process.cwd() to return test directory
+    originalCwd = process.cwd;
+    process.cwd = () => testDir;
+  });
+
+  afterEach(async () => {
+    // Restore process.cwd()
+    process.cwd = originalCwd;
+
+    // Cleanup test directory
+    await rm(testDir, { recursive: true, force: true });
+  });
+
+  describe('getStateResource', () => {
+    test('reads valid state file', async () => {
+      // Create valid state file
+      const stateFile = join(testDir, '.stackshift-state.json');
+      await writeFile(
+        stateFile,
+        JSON.stringify({
+          path: 'brownfield',
+          currentStep: 'analyze',
+          completedSteps: [],
+          stepDetails: {},
+        })
+      );
+
+      // Call resource handler
+      const result = await getStateResource();
+
+      // Verify response
+      expect(result.contents).toHaveLength(1);
+      expect(result.contents[0].uri).toBe('stackshift://state');
+      expect(result.contents[0].mimeType).toBe('application/json');
+
+      const state = JSON.parse(result.contents[0].text);
+      expect(state.path).toBe('brownfield');
+    });
+
+    test('rejects file larger than 10MB', async () => {
+      // Create large file (11MB)
+      const stateFile = join(testDir, '.stackshift-state.json');
+      const largeData = 'A'.repeat(11 * 1024 * 1024);
+      await writeFile(stateFile, `{"data": "${largeData}"}`);
+
+      // Should handle error gracefully
+      const result = await getStateResource();
+
+      // Should return error response (not throw)
+      expect(result.contents[0].text).toContain('error');
+    });
+
+    test('strips dangerous JSON properties', async () => {
+      // Create state file with prototype pollution attempt
+      const stateFile = join(testDir, '.stackshift-state.json');
+      await writeFile(
+        stateFile,
+        JSON.stringify({
+          path: 'greenfield',
+          __proto__: { admin: true },
+          constructor: { malicious: true },
+          prototype: { hacked: true },
+        })
+      );
+
+      // Call resource handler
+      const result = await getStateResource();
+
+      // Verify dangerous properties are removed (check as own properties, not inherited)
+      const state = JSON.parse(result.contents[0].text);
+      expect(Object.prototype.hasOwnProperty.call(state, '__proto__')).toBe(false);
+      expect(Object.prototype.hasOwnProperty.call(state, 'constructor')).toBe(false);
+      expect(Object.prototype.hasOwnProperty.call(state, 'prototype')).toBe(false);
+    });
+
+    test('returns error when state file not found', async () => {
+      // Don't create state file
+
+      // Call resource handler
+      const result = await getStateResource();
+
+      // Should return error response
+      const response = JSON.parse(result.contents[0].text);
+      expect(response.error).toBe('No state file found');
+    });
+  });
+
+  describe('getProgressResource', () => {
+    test('reads valid state file', async () => {
+      // Create valid state file
+      const stateFile = join(testDir, '.stackshift-state.json');
+      await writeFile(
+        stateFile,
+        JSON.stringify({
+          path: 'brownfield',
+          currentStep: 'analyze',
+          completedSteps: ['analyze'],
+          stepDetails: {},
+        })
+      );
+
+      // Call resource handler
+      const result = await getProgressResource();
+
+      // Verify response
+      expect(result.contents).toHaveLength(1);
+      expect(result.contents[0].text).toContain('StackShift Progress');
+      expect(result.contents[0].text).toContain('Complete');
+    });
+  });
+
+  describe('getRouteResource', () => {
+    test('reads valid state file', async () => {
+      // Create valid state file
+      const stateFile = join(testDir, '.stackshift-state.json');
+      await writeFile(
+        stateFile,
+        JSON.stringify({
+          path: 'greenfield',
+          currentStep: 'analyze',
+          completedSteps: [],
+          stepDetails: {},
+        })
+      );
+
+      // Call resource handler
+      const result = await getRouteResource();
+
+      // Verify response
+      expect(result.contents).toHaveLength(1);
+      expect(result.contents[0].text).toContain('Greenfield');
+    });
+  });
+});

mcp-server/src/resources/index.ts

@@ -6,6 +6,8 @@
 
 import * as fs from 'fs/promises';
 import * as path from 'path';
+import { createDefaultValidator } from '../utils/security.js';
+import { readJsonSafe } from '../utils/file-utils.js';
 
 const GEARS = [
   { id: 'analyze', name: 'Gear 1: Initial Analysis', emoji: '🔍' },
@@ -20,17 +22,23 @@ const GEARS = [
  * Get current state
  */
 export async function getStateResource() {
-  const directory = process.cwd();
-  const stateFile = path.join(directory, '.stackshift-state.json');
+  // Create security validator
+  const validator = createDefaultValidator();
 
   try {
-    const state = await fs.readFile(stateFile, 'utf-8');
+    // Validate directory before using it
+    const directory = validator.validateDirectory(process.cwd());
+    const stateFile = path.join(directory, '.stackshift-state.json');
+
+    // Use safe JSON read (with size limit and sanitization)
+    const state = await readJsonSafe(stateFile);
+
     return {
       contents: [
         {
           uri: 'stackshift://state',
           mimeType: 'application/json',
-          text: state,
+          text: JSON.stringify(state, null, 2),
         },
       ],
     };
@@ -58,12 +66,16 @@ export async function getStateResource() {
  * Get progress through gears
  */
 export async function getProgressResource() {
-  const directory = process.cwd();
-  const stateFile = path.join(directory, '.stackshift-state.json');
+  // Create security validator
+  const validator = createDefaultValidator();
 
   try {
-    const stateData = await fs.readFile(stateFile, 'utf-8');
-    const state = JSON.parse(stateData);
+    // Validate directory before using it
+    const directory = validator.validateDirectory(process.cwd());
+    const stateFile = path.join(directory, '.stackshift-state.json');
+
+    // Use safe JSON read (replaces readFile + JSON.parse)
+    const state = await readJsonSafe(stateFile);
 
     const progress = GEARS.map(gear => {
       const isCompleted = state.completedSteps.includes(gear.id);
@@ -120,12 +132,16 @@ ${completedCount === totalGears ? '\n🏁 **All gears complete! Cruise into prod
  * Get selected route
  */
 export async function getRouteResource() {
-  const directory = process.cwd();
-  const stateFile = path.join(directory, '.stackshift-state.json');
+  // Create security validator
+  const validator = createDefaultValidator();
 
   try {
-    const stateData = await fs.readFile(stateFile, 'utf-8');
-    const state = JSON.parse(stateData);
+    // Validate directory before using it
+    const directory = validator.validateDirectory(process.cwd());
+    const stateFile = path.join(directory, '.stackshift-state.json');
+
+    // Use safe JSON read
+    const state = await readJsonSafe(stateFile);
 
     const routeInfo =
       state.path === 'greenfield'

mcp-server/src/utils/__tests__/skill-loader.security.test.ts

@@ -0,0 +1,71 @@
+import { describe, test, expect, beforeEach, afterEach } from 'vitest';
+import { loadSkillFile } from '../skill-loader.js';
+import { mkdtemp, rm } from 'fs/promises';
+import { join } from 'path';
+import { tmpdir } from 'os';
+
+describe('Skill Loader Security Tests', () => {
+  let testDir: string;
+  let originalHome: string | undefined;
+
+  beforeEach(async () => {
+    // Save original HOME
+    originalHome = process.env.HOME;
+
+    // Create temp directory and set as HOME
+    testDir = await mkdtemp(join(tmpdir(), 'skill-security-test-'));
+    process.env.HOME = testDir;
+  });
+
+  afterEach(async () => {
+    // Restore HOME
+    if (originalHome !== undefined) {
+      process.env.HOME = originalHome;
+    }
+
+    // Cleanup
+    await rm(testDir, { recursive: true, force: true });
+  });
+
+  test('rejects skill name with forward slash', async () => {
+    await expect(loadSkillFile('../etc/passwd')).rejects.toThrow(
+      'Invalid skill name: cannot contain path separators'
+    );
+  });
+
+  test('rejects skill name with backslash', async () => {
+    await expect(loadSkillFile('..\\windows\\system32')).rejects.toThrow(
+      'Invalid skill name: cannot contain path separators'
+    );
+  });
+
+  test('rejects skill name with parent directory reference', async () => {
+    await expect(loadSkillFile('..')).rejects.toThrow(
+      'Invalid skill name: cannot contain path separators'
+    );
+  });
+
+  test('rejects skill name with special characters', async () => {
+    await expect(loadSkillFile('hack;rm -rf')).rejects.toThrow(
+      'Invalid skill name: must be alphanumeric with hyphens/underscores only'
+    );
+  });
+
+  test('rejects skill name with spaces', async () => {
+    await expect(loadSkillFile('skill name')).rejects.toThrow(
+      'Invalid skill name: must be alphanumeric with hyphens/underscores only'
+    );
+  });
+
+  test('accepts valid skill name with hyphens', async () => {
+    // This will return null (skill not found) but should not throw validation error
+    const result = await loadSkillFile('reverse-engineer');
+    // Either returns content or null, but doesn't throw
+    expect(typeof result === 'string' || result === null).toBe(true);
+  });
+
+  test('accepts valid skill name with underscores', async () => {
+    const result = await loadSkillFile('create_specs');
+    expect(typeof result === 'string' || result === null).toBe(true);
+  });
+});

mcp-server/src/utils/__tests__/skill-loader.test.ts

@@ -29,6 +29,9 @@ describe('skill-loader', () => {
     );
     await fs.mkdir(testDir, { recursive: true });
 
+    // Set HOME to test directory for security validation
+    process.env.HOME = testDir;
+
     // Spy on console.error to suppress output during tests
     vi.spyOn(console, 'error').mockImplementation(() => {});
   });

mcp-server/src/utils/skill-loader.ts

@@ -7,15 +7,37 @@
 
 import * as fs from 'fs/promises';
 import * as path from 'path';
+import { createDefaultValidator } from './security.js';
+import { readFileSafe } from './file-utils.js';
 
 /**
  * Try to load SKILL.md from multiple possible locations
  */
 export async function loadSkillFile(skillName: string): Promise<string | null> {
+  // Validate skill name: no path separators
+  if (skillName.includes('/') || skillName.includes('\\') || skillName.includes('..')) {
+    throw new Error(`Invalid skill name: cannot contain path separators`);
+  }
+
+  // Validate skill name: whitelist regex
+  if (!/^[a-zA-Z0-9_-]+$/.test(skillName)) {
+    throw new Error(`Invalid skill name: must be alphanumeric with hyphens/underscores only`);
+  }
+
+  // Validate HOME environment variable
+  const homePath = process.env.HOME;
+  if (!homePath || homePath.includes('\0')) {
+    throw new Error('Invalid HOME environment variable');
+  }
+
+  // Create validator for HOME directory
+  const validator = createDefaultValidator();
+  const validatedHome = validator.validateDirectory(homePath);
+
   const possiblePaths = [
-    // If StackShift is installed locally
+    // If StackShift is installed locally (use validated HOME)
     path.join(
-      process.env.HOME || '~',
+      validatedHome,
       '.claude/plugins/marketplaces/jschulte/stackshift/skills',
       skillName,
       'SKILL.md'
@@ -31,7 +53,7 @@ export async function loadSkillFile(skillName: string): Promise<string | null> {
 
   for (const filePath of possiblePaths) {
     try {
-      const content = await fs.readFile(filePath, 'utf-8');
+      const content = await readFileSafe(filePath);
       if (content && content.length > 100) {
         console.error(`✅ Loaded SKILL.md from: ${filePath}`);
         return content;