fix: address 5 critical security audit findings (#88)

mcp-server/package.json

@@ -5,14 +5,17 @@
   "main": "./dist/index.js",
   "scripts": {
     "build": "tsc",
-    "clean": "rm -rf dist"
+    "clean": "rm -rf dist",
+    "test": "vitest run",
+    "test:watch": "vitest"
   },
   "dependencies": {
     "@anthropic-ai/claude-agent-sdk": "^0.1.0",
     "zod": "^3.22.4"
   },
   "devDependencies": {
     "@types/node": "^25.0.3",
-    "typescript": "^5.9.3"
+    "typescript": "^5.9.3",
+    "vitest": "^4.0.18"
   }
 }

mcp-server/src/tools/generate-totp.ts

@@ -18,16 +18,17 @@ import { z } from 'zod';
 import { createToolResult, type ToolResult, type GenerateTotpResponse } from '../types/tool-responses.js';
 import { base32Decode, validateTotpSecret } from '../validation/totp-validator.js';
 import { createCryptoError, createGenericError } from '../utils/error-formatter.js';
+import { TOTP_MIN_SECRET_LENGTH } from '../validation/totp-validator.js';
 
 /**
  * Input schema for generate_totp tool
  */
 export const GenerateTotpInputSchema = z.object({
   secret: z
     .string()
-    .min(1)
+    .min(TOTP_MIN_SECRET_LENGTH, `Must be at least ${TOTP_MIN_SECRET_LENGTH} base32 characters (RFC 4226)`)
     .regex(/^[A-Z2-7]+$/i, 'Must be base32-encoded')
-    .describe('Base32-encoded TOTP secret'),
+    .describe('Base32-encoded TOTP secret (minimum 32 characters per RFC 4226)'),
 });
 
 export type GenerateTotpInput = z.infer<typeof GenerateTotpInputSchema>;

mcp-server/src/utils/file-operations.test.ts

@@ -0,0 +1,59 @@
+import { describe, it, expect, afterAll } from 'vitest';
+import { saveDeliverableFile } from './file-operations.js';
+import { mkdirSync, rmSync, readFileSync, existsSync } from 'fs';
+import { join } from 'path';
+import { tmpdir } from 'os';
+
+const testDir = join(tmpdir(), `shannon-file-ops-test-${Date.now()}`);
+
+afterAll(() => {
+  try {
+    rmSync(testDir, { recursive: true, force: true });
+  } catch {
+    // Ignore cleanup errors
+  }
+});
+
+describe('saveDeliverableFile - path traversal protection', () => {
+  it('saves a valid file successfully', () => {
+    mkdirSync(testDir, { recursive: true });
+    const filepath = saveDeliverableFile(testDir, 'report.md', '# Test Report');
+    expect(existsSync(filepath)).toBe(true);
+    expect(readFileSync(filepath, 'utf8')).toBe('# Test Report');
+  });
+
+  it('rejects filename with ../ path traversal', () => {
+    expect(() => saveDeliverableFile(testDir, '../../../etc/passwd', 'malicious')).toThrow(
+      'must not contain path separators'
+    );
+  });
+
+  it('rejects filename with ../ in the middle', () => {
+    expect(() => saveDeliverableFile(testDir, 'foo/../../../bar', 'malicious')).toThrow(
+      'must not contain path separators'
+    );
+  });
+
+  it('rejects filename with backslash traversal (Windows-style)', () => {
+    // On Unix, path.basename uses / as separator, but we also check for ..
+    expect(() => saveDeliverableFile(testDir, '..\\..\\pwn.txt', 'malicious')).toThrow();
+  });
+
+  it('rejects filename containing null bytes', () => {
+    expect(() => saveDeliverableFile(testDir, 'file\0.txt', 'malicious')).toThrow(
+      'forbidden characters'
+    );
+  });
+
+  it('rejects absolute path as filename', () => {
+    expect(() => saveDeliverableFile(testDir, '/etc/passwd', 'malicious')).toThrow(
+      'must not contain path separators'
+    );
+  });
+
+  it('allows filenames with dots that are not traversal', () => {
+    mkdirSync(testDir, { recursive: true });
+    const filepath = saveDeliverableFile(testDir, 'report.v2.md', 'content');
+    expect(existsSync(filepath)).toBe(true);
+  });
+});

mcp-server/src/utils/file-operations.ts

@@ -12,7 +12,7 @@
  */
 
 import { writeFileSync, mkdirSync } from 'fs';
-import { join } from 'path';
+import { join, resolve, basename } from 'path';
 
 /**
  * Save deliverable file to deliverables/ directory
@@ -22,8 +22,21 @@ import { join } from 'path';
  * @param content - File content to save
  */
 export function saveDeliverableFile(targetDir: string, filename: string, content: string): string {
-  const deliverablesDir = join(targetDir, 'deliverables');
-  const filepath = join(deliverablesDir, filename);
+  // Path traversal protection: reject filenames with path separators or traversal sequences
+  if (filename !== basename(filename)) {
+    throw new Error(`Invalid filename: must not contain path separators`);
+  }
+  if (filename.includes('..') || filename.includes('\0')) {
+    throw new Error(`Invalid filename: contains forbidden characters`);
+  }
+
+  const deliverablesDir = resolve(targetDir, 'deliverables');
+  const filepath = resolve(deliverablesDir, filename);
+
+  // Verify resolved path stays within the intended deliverables directory
+  if (!filepath.startsWith(deliverablesDir + '/') && filepath !== deliverablesDir) {
+    throw new Error(`Path traversal detected: resolved path escapes deliverables directory`);
+  }
 
   // Ensure deliverables directory exists
   try {

mcp-server/src/validation/totp-validator.test.ts

@@ -0,0 +1,35 @@
+import { describe, it, expect } from 'vitest';
+import { validateTotpSecret, TOTP_MIN_SECRET_LENGTH } from './totp-validator.js';
+
+describe('validateTotpSecret', () => {
+  // A valid 32-char base32 secret (160 bits)
+  const VALID_SECRET = 'JBSWY3DPEHPK3PXPJBSWY3DPEHPK3PXP';
+  // A short secret (only 16 chars = 80 bits, below minimum)
+  const SHORT_SECRET = 'JBSWY3DPEHPK3PXP';
+
+  it('accepts a secret with >= 32 base32 characters', () => {
+    expect(VALID_SECRET.length).toBeGreaterThanOrEqual(TOTP_MIN_SECRET_LENGTH);
+    expect(validateTotpSecret(VALID_SECRET)).toBe(true);
+  });
+
+  it('rejects a short secret (< 32 base32 chars)', () => {
+    expect(SHORT_SECRET.length).toBeLessThan(TOTP_MIN_SECRET_LENGTH);
+    expect(() => validateTotpSecret(SHORT_SECRET)).toThrow('too short');
+  });
+
+  it('rejects an 8-character secret', () => {
+    expect(() => validateTotpSecret('JBSWY3DP')).toThrow('too short');
+  });
+
+  it('rejects an empty secret', () => {
+    expect(() => validateTotpSecret('')).toThrow('empty');
+  });
+
+  it('mentions RFC 4226 in the error message for short secrets', () => {
+    expect(() => validateTotpSecret(SHORT_SECRET)).toThrow('RFC 4226');
+  });
+
+  it('exports TOTP_MIN_SECRET_LENGTH as 32', () => {
+    expect(TOTP_MIN_SECRET_LENGTH).toBe(32);
+  });
+});

mcp-server/src/validation/totp-validator.ts

@@ -45,9 +45,15 @@ export function base32Decode(encoded: string): Buffer {
   return Buffer.from(output);
 }
 
+/**
+ * Minimum base32 secret length.
+ * RFC 4226 Section 4 recommends at least 160 bits (20 bytes = 32 base32 characters).
+ */
+export const TOTP_MIN_SECRET_LENGTH = 32;
+
 /**
  * Validate TOTP secret
- * Must be base32-encoded string
+ * Must be base32-encoded string with minimum length per RFC 4226.
  *
  * @returns true if valid, throws Error if invalid
  */
@@ -56,12 +62,22 @@ export function validateTotpSecret(secret: string): boolean {
     throw new Error('TOTP secret cannot be empty');
   }
 
+  // Strip non-base32 characters (padding, whitespace) for validation
+  const cleanSecret = secret.replace(/[^A-Z2-7]/gi, '');
+
   // Check if it's valid base32 (only A-Z and 2-7, case-insensitive)
   const base32Regex = /^[A-Z2-7]+$/i;
-  if (!base32Regex.test(secret.replace(/[^A-Z2-7]/gi, ''))) {
+  if (!base32Regex.test(cleanSecret)) {
     throw new Error('TOTP secret must be base32-encoded (characters A-Z and 2-7)');
   }
 
+  // Enforce minimum length per RFC 4226 (160 bits = 32 base32 chars)
+  if (cleanSecret.length < TOTP_MIN_SECRET_LENGTH) {
+    throw new Error(
+      `TOTP secret too short: ${cleanSecret.length} base32 characters (minimum ${TOTP_MIN_SECRET_LENGTH} required per RFC 4226)`
+    );
+  }
+
   // Try to decode to ensure it's valid
   try {
     base32Decode(secret);

mcp-server/vitest.config.ts

@@ -0,0 +1,7 @@
+import { defineConfig } from 'vitest/config';
+
+export default defineConfig({
+  test: {
+    include: ['src/**/*.test.ts'],
+  },
+});