[KDM-TEST-FIX-324] Fix:critical security vulnerability in the file cache system

.jules/sentinel.md

@@ -0,0 +1,8 @@
+## 2023-11-20 - [Missing input length limits and security headers in local HTTP server]
+**Vulnerability:** The local CLI dashboard HTTP server did not limit the length of POST request bodies, introducing a DoS risk. In addition, it lacked basic security headers.
+**Learning:** Even local CLI HTTP servers require fundamental HTTP security controls (like payload limits and headers) to prevent exploitation, particularly since they may interact with browsers or external services via custom endpoints.
+**Prevention:** Always enforce a request size limit on raw Node.js streams and return generic security headers for JSON API responses.
+## 2024-05-24 - [Critical] Path Traversal in File Cache
+**Vulnerability:** Path Traversal vulnerability in FileCacheProvider due to unsanitized cache keys.
+**Learning:** User input acting as filenames must always be validated to ensure it cannot escape the intended directory.
+**Prevention:** Always use path.resolve and verify the resulting path starts with the intended base directory.

src/__tests__/cache.test.ts

@@ -70,6 +70,29 @@ describe('File Cache Provider', () => {
     expect(result).toBeNull();
   });
 
+  it('prevents path traversal vulnerabilities for cache keys', async () => {
+    const maliciousKey = '../../../etc/passwd';
+    await expect(cache.store(maliciousKey, 'data')).rejects.toThrow(/path traversal/);
+    await expect(cache.load(maliciousKey)).rejects.toThrow(/path traversal/);
+    await expect(cache.remove(maliciousKey)).rejects.toThrow(/path traversal/);
+    await expect(cache.exists(maliciousKey)).rejects.toThrow(/path traversal/);
+  });
+
+  it('rejects symlinks pointing outside the cache directory', async () => {
+    const externalDir = path.join(os.tmpdir(), `kdm-external-${Date.now()}`);
+    fs.mkdirSync(externalDir, { recursive: true });
+    const targetFile = path.join(externalDir, 'secret.txt');
+    fs.writeFileSync(targetFile, 'secret data');
+
+    const symlinkPath = path.join(testDir, 'symlink-key');
+    fs.symlinkSync(targetFile, symlinkPath);
+
+    // Store and load should fail to follow the symlink because of O_NOFOLLOW
+    await expect(cache.store('symlink-key', 'overwritten')).rejects.toThrow();
+    // Load returns null when the file can't be read safely
+    expect(await cache.load('symlink-key')).toBeNull();
+  });
+
   it.each([
     { key: 'key-with-data', data: 'hello world', expectedSize: 11 },
     { key: 'empty-data', data: '', expectedSize: 0 },

src/cache/file-cache.ts

@@ -35,12 +35,45 @@ const ensureCacheDir = (dir: string): void => {
  */
 const safeReadFile = (filePath: string): string | null => {
   try {
-    return fs.readFileSync(filePath, 'utf-8');
+    const fd = fs.openSync(filePath, fs.constants.O_RDONLY | fs.constants.O_NOFOLLOW);
+    try {
+      return fs.readFileSync(fd, 'utf-8');
+    } finally {
+      fs.closeSync(fd);
+    }
   } catch {
     return null;
   }
 };
 
+/**
+ * Resolves a safe file path for a cache key, preventing directory traversal.
+ * @param cacheDir Absolute path to the cache directory.
+ * @param key The cache key.
+ * @returns Absolute path to the cache entry file.
+ * @throws Error if the key attempts to traverse outside the cache directory.
+ */
+const getSafePath = (cacheDir: string, key: string): string => {
+  const resolvedCacheDir = path.resolve(cacheDir);
+  const safePath = path.resolve(cacheDir, key);
+  const normalizedCacheDir = resolvedCacheDir + path.sep;
+
+  if (!safePath.startsWith(normalizedCacheDir) && safePath !== resolvedCacheDir) {
+    throw new Error(`Invalid cache key: potential directory traversal detected`);
+  }
+
+  const dir = path.dirname(safePath);
+  if (fs.existsSync(dir)) {
+    const realDir = fs.realpathSync(dir);
+    const realCacheDir = fs.existsSync(cacheDir) ? fs.realpathSync(cacheDir) : resolvedCacheDir;
+    if (!realDir.startsWith(realCacheDir + path.sep) && realDir !== realCacheDir) {
+      throw new Error(`Invalid cache key: potential directory traversal detected`);
+    }
+  }
+
+  return safePath;
+};
+
 /**
  * File-based implementation of the CacheProvider interface.
  * Stores each cached entry as a separate file named by its key.
@@ -58,15 +91,46 @@ export class FileCacheProvider implements CacheProvider {
     ensureCacheDir(this.cacheDir);
   }
 
+  /**
+   * Gets a safe file path ensuring it does not escape the cache directory (Path Traversal protection).
+   * @param key Cache key.
+   * @returns Absolute path to the file.
+   */
+  private getSafeFilePath(key: string): string {
+    const resolvedCacheDir = path.resolve(this.cacheDir);
+    const resolvedPath = path.resolve(this.cacheDir, key);
+
+    if (!resolvedPath.startsWith(resolvedCacheDir + path.sep) && resolvedPath !== resolvedCacheDir) {
+      throw new Error(`Invalid cache key: path traversal detected for key '${key}'`);
+    }
+
+    const dir = path.dirname(resolvedPath);
+    if (fs.existsSync(dir)) {
+      const realDir = fs.realpathSync(dir);
+      const realCacheDir = fs.existsSync(this.cacheDir) ? fs.realpathSync(this.cacheDir) : resolvedCacheDir;
+
+      if (!realDir.startsWith(realCacheDir + path.sep) && realDir !== realCacheDir) {
+        throw new Error(`Invalid cache key: path traversal detected for key '${key}'`);
+      }
+    }
+
+    return resolvedPath;
+  }
+
   /**
    * Stores AI response text under the given cache key.
    * @param key Cache key (typically a SHA-256 hash).
    * @param data The AI response text.
    */
   async store(key: string, data: string): Promise<void> {
     ensureCacheDir(this.cacheDir);
-    const filePath = path.join(this.cacheDir, key);
-    fs.writeFileSync(filePath, data, 'utf-8');
+    const filePath = this.getSafeFilePath(key);
+    const fd = fs.openSync(filePath, fs.constants.O_WRONLY | fs.constants.O_CREAT | fs.constants.O_TRUNC | fs.constants.O_NOFOLLOW);
+    try {
+      fs.writeFileSync(fd, data, 'utf-8');
+    } finally {
+      fs.closeSync(fd);
+    }
   }
 
   /**
@@ -75,7 +139,7 @@ export class FileCacheProvider implements CacheProvider {
    * @returns The cached string or null.
    */
   async load(key: string): Promise<string | null> {
-    const filePath = path.join(this.cacheDir, key);
+    const filePath = this.getSafeFilePath(key);
     return safeReadFile(filePath);
   }
 
@@ -87,7 +151,7 @@ export class FileCacheProvider implements CacheProvider {
     ensureCacheDir(this.cacheDir);
     const files = fs.readdirSync(this.cacheDir);
     return files.map((file) => {
-      const filePath = path.join(this.cacheDir, file);
+      const filePath = this.getSafeFilePath(file);
       const stat = fs.statSync(filePath);
       return {
         key: file,
@@ -102,7 +166,7 @@ export class FileCacheProvider implements CacheProvider {
    * @param key Cache key to remove.
    */
   async remove(key: string): Promise<void> {
-    const filePath = path.join(this.cacheDir, key);
+    const filePath = this.getSafeFilePath(key);
     if (fs.existsSync(filePath)) {
       fs.unlinkSync(filePath);
     }
@@ -114,7 +178,7 @@ export class FileCacheProvider implements CacheProvider {
    * @returns True if the file exists.
    */
   async exists(key: string): Promise<boolean> {
-    return fs.existsSync(path.join(this.cacheDir, key));
+    return fs.existsSync(this.getSafeFilePath(key));
   }
 
   /**

src/server/server.ts

@@ -63,7 +63,12 @@ export const readBody = (req: any): Promise<string> =>
  * @param data Response payload.
  */
 export const sendJson = (res: any, status: number, data: unknown): void => {
-  res.writeHead(status, { 'Content-Type': 'application/json' });
+  res.writeHead(status, {
+    'Content-Type': 'application/json',
+    'X-Content-Type-Options': 'nosniff',
+    'X-Frame-Options': 'DENY',
+    'Content-Security-Policy': "default-src 'none'",
+  });
   res.end(JSON.stringify(data));
 };