refactor: streamline ALLOWED_ROOT_DIRECTORY handling and remove legacy support

This commit refactors the handling of ALLOWED_ROOT_DIRECTORY by removing legacy support for ALLOWED_PROJECT_DIRS and simplifying the security logic. Key changes include:

- Removed deprecated ALLOWED_PROJECT_DIRS references from .env.example and security.ts.
- Updated initAllowedPaths() to focus solely on ALLOWED_ROOT_DIRECTORY and DATA_DIR.
- Enhanced logging for ALLOWED_ROOT_DIRECTORY configuration status.
- Adjusted route handlers to utilize the new workspace directory logic.
- Introduced a centralized storage module for localStorage operations to improve consistency and error handling.

These changes aim to enhance security and maintainability by consolidating directory management into a single variable.

Tests: All unit tests passing.

apps/server/.env.example

@@ -22,11 +22,6 @@ AUTOMAKER_API_KEY=
 # Example: ALLOWED_ROOT_DIRECTORY=/projects
 ALLOWED_ROOT_DIRECTORY=
 
-# (Legacy) Restrict file operations to these directories (comma-separated)
-# DEPRECATED: Use ALLOWED_ROOT_DIRECTORY instead for simpler configuration
-# This is kept for backward compatibility
-# ALLOWED_PROJECT_DIRS=/home/user/projects,/var/www
-
 # CORS origin - which domains can access the API
 # Use "*" for development, set specific origin for production
 CORS_ORIGIN=*

apps/server/Dockerfile

@@ -26,6 +26,14 @@ RUN npm run build --workspace=apps/server
 # Production stage
 FROM node:20-alpine
 
+# Install git, curl, and GitHub CLI
+RUN apk add --no-cache git curl && \
+    GH_VERSION=$(curl -s https://api.github.com/repos/cli/cli/releases/latest | grep '"tag_name"' | cut -d '"' -f 4 | sed 's/v//') && \
+    curl -L "https://github.com/cli/cli/releases/download/v${GH_VERSION}/gh_${GH_VERSION}_linux_amd64.tar.gz" -o gh.tar.gz && \
+    tar -xzf gh.tar.gz && \
+    mv gh_*_linux_amd64/bin/gh /usr/local/bin/gh && \
+    rm -rf gh.tar.gz gh_*_linux_amd64
+
 WORKDIR /app
 
 # Create non-root user

apps/server/src/lib/security.ts

@@ -23,21 +23,27 @@ let allowedRootDirectory: string | null = null;
 // Data directory - always allowed for settings/credentials
 let dataDirectory: string | null = null;
 
-// Allowed project directories - kept for backward compatibility and API compatibility
+// Allowed paths set - stores ALLOWED_ROOT_DIRECTORY and DATA_DIR
 const allowedPaths = new Set<string>();
 
 /**
  * Initialize security settings from environment variables
  * - ALLOWED_ROOT_DIRECTORY: main security boundary
  * - DATA_DIR: appData exception, always allowed
- * - ALLOWED_PROJECT_DIRS: legacy variable, stored for compatibility
  */
 export function initAllowedPaths(): void {
-  // Load ALLOWED_ROOT_DIRECTORY (new single variable)
+  // Load ALLOWED_ROOT_DIRECTORY
   const rootDir = process.env.ALLOWED_ROOT_DIRECTORY;
   if (rootDir) {
     allowedRootDirectory = path.resolve(rootDir);
     allowedPaths.add(allowedRootDirectory);
+    console.log(
+      `[Security] ✓ ALLOWED_ROOT_DIRECTORY configured: ${allowedRootDirectory}`
+    );
+  } else {
+    console.log(
+      "[Security] ⚠️  ALLOWED_ROOT_DIRECTORY not set - allowing access to all paths"
+    );
   }
 
   // Load DATA_DIR (appData exception - always allowed)
@@ -45,17 +51,7 @@ export function initAllowedPaths(): void {
   if (dataDir) {
     dataDirectory = path.resolve(dataDir);
     allowedPaths.add(dataDirectory);
-  }
-
-  // Load legacy ALLOWED_PROJECT_DIRS for backward compatibility during transition
-  const dirs = process.env.ALLOWED_PROJECT_DIRS;
-  if (dirs) {
-    for (const dir of dirs.split(",")) {
-      const trimmed = dir.trim();
-      if (trimmed) {
-        allowedPaths.add(path.resolve(trimmed));
-      }
-    }
+    console.log(`[Security] ✓ DATA_DIR configured: ${dataDirectory}`);
   }
 }
 
@@ -68,19 +64,13 @@ export function addAllowedPath(filePath: string): void {
 }
 
 /**
- * Check if a path is allowed based on ALLOWED_ROOT_DIRECTORY and legacy ALLOWED_PROJECT_DIRS
+ * Check if a path is allowed based on ALLOWED_ROOT_DIRECTORY
  * Returns true if:
  * - Path is within ALLOWED_ROOT_DIRECTORY, OR
- * - Path is within any legacy allowed path (ALLOWED_PROJECT_DIRS), OR
  * - Path is within DATA_DIR (appData exception), OR
  * - No restrictions are configured (backward compatibility)
  */
 export function isPathAllowed(filePath: string): boolean {
-  // If no restrictions are configured, allow all paths (backward compatibility)
-  if (!allowedRootDirectory && allowedPaths.size === 0) {
-    return true;
-  }
-
   const resolvedPath = path.resolve(filePath);
 
   // Always allow appData directory (settings, credentials)
@@ -88,19 +78,21 @@ export function isPathAllowed(filePath: string): boolean {
     return true;
   }
 
-  // Allow if within ALLOWED_ROOT_DIRECTORY
-  if (allowedRootDirectory && isPathWithinDirectory(resolvedPath, allowedRootDirectory)) {
+  // If no ALLOWED_ROOT_DIRECTORY restriction is configured, allow all paths
+  // Note: DATA_DIR is checked above as an exception, but doesn't restrict other paths
+  if (!allowedRootDirectory) {
     return true;
   }
 
-  // Check legacy allowed paths (ALLOWED_PROJECT_DIRS)
-  for (const allowedPath of allowedPaths) {
-    if (isPathWithinDirectory(resolvedPath, allowedPath)) {
-      return true;
-    }
+  // Allow if within ALLOWED_ROOT_DIRECTORY
+  if (
+    allowedRootDirectory &&
+    isPathWithinDirectory(resolvedPath, allowedRootDirectory)
+  ) {
+    return true;
   }
 
-  // If any restrictions are configured but path doesn't match, deny
+  // If restrictions are configured but path doesn't match, deny
   return false;
 }
 
@@ -132,10 +124,7 @@ export function isPathWithinDirectory(
   // If relative path starts with "..", it's outside the directory
   // If relative path is absolute, it's outside the directory
   // If relative path is empty or ".", it's the directory itself
-  return (
-    !relativePath.startsWith("..") &&
-    !path.isAbsolute(relativePath)
-  );
+  return !relativePath.startsWith("..") && !path.isAbsolute(relativePath);
 }
 
 /**

apps/server/src/routes/workspace/routes/config.ts

@@ -5,18 +5,25 @@
 import type { Request, Response } from "express";
 import fs from "fs/promises";
 import path from "path";
-import { addAllowedPath, getAllowedRootDirectory } from "../../../lib/security.js";
+import {
+  addAllowedPath,
+  getAllowedRootDirectory,
+  getDataDirectory,
+} from "../../../lib/security.js";
 import { getErrorMessage, logError } from "../common.js";
 
 export function createConfigHandler() {
   return async (_req: Request, res: Response): Promise<void> => {
     try {
       const allowedRootDirectory = getAllowedRootDirectory();
+      const dataDirectory = getDataDirectory();
 
       if (!allowedRootDirectory) {
+        // When ALLOWED_ROOT_DIRECTORY is not set, return DATA_DIR as default directory
         res.json({
           success: true,
           configured: false,
+          defaultDir: dataDirectory || null,
         });
         return;
       }
@@ -41,6 +48,7 @@ export function createConfigHandler() {
           success: true,
           configured: true,
           workspaceDir: resolvedWorkspaceDir,
+          defaultDir: resolvedWorkspaceDir,
         });
       } catch {
         res.json({

apps/server/tests/setup.ts

@@ -8,7 +8,6 @@ import { vi, beforeEach } from "vitest";
 // Set test environment variables
 process.env.NODE_ENV = "test";
 process.env.DATA_DIR = "/tmp/test-data";
-process.env.ALLOWED_PROJECT_DIRS = "/tmp/test-projects";
 
 // Reset all mocks before each test
 beforeEach(() => {

apps/server/tests/unit/lib/security.test.ts

@@ -11,81 +11,49 @@ describe("security.ts", () => {
   });
 
   describe("initAllowedPaths", () => {
-    it("should parse comma-separated directories from environment", async () => {
-      process.env.ALLOWED_PROJECT_DIRS = "/path1,/path2,/path3";
+    it("should load ALLOWED_ROOT_DIRECTORY if set", async () => {
+      process.env.ALLOWED_ROOT_DIRECTORY = "/projects";
       process.env.DATA_DIR = "";
 
-      const { initAllowedPaths, getAllowedPaths } = await import(
-        "@/lib/security.js"
-      );
+      const { initAllowedPaths, getAllowedPaths, getAllowedRootDirectory } =
+        await import("@/lib/security.js");
       initAllowedPaths();
 
       const allowed = getAllowedPaths();
-      expect(allowed).toContain(path.resolve("/path1"));
-      expect(allowed).toContain(path.resolve("/path2"));
-      expect(allowed).toContain(path.resolve("/path3"));
-    });
-
-    it("should trim whitespace from paths", async () => {
-      process.env.ALLOWED_PROJECT_DIRS = " /path1 , /path2 , /path3 ";
-      process.env.DATA_DIR = "";
-
-      const { initAllowedPaths, getAllowedPaths } = await import(
-        "@/lib/security.js"
-      );
-      initAllowedPaths();
-
-      const allowed = getAllowedPaths();
-      expect(allowed).toContain(path.resolve("/path1"));
-      expect(allowed).toContain(path.resolve("/path2"));
+      expect(allowed).toContain(path.resolve("/projects"));
+      expect(getAllowedRootDirectory()).toBe(path.resolve("/projects"));
     });
 
     it("should always include DATA_DIR if set", async () => {
-      process.env.ALLOWED_PROJECT_DIRS = "";
+      delete process.env.ALLOWED_ROOT_DIRECTORY;
       process.env.DATA_DIR = "/data/dir";
 
-      const { initAllowedPaths, getAllowedPaths } = await import(
-        "@/lib/security.js"
-      );
+      const { initAllowedPaths, getAllowedPaths } =
+        await import("@/lib/security.js");
       initAllowedPaths();
 
       const allowed = getAllowedPaths();
       expect(allowed).toContain(path.resolve("/data/dir"));
     });
 
-    it("should handle empty ALLOWED_PROJECT_DIRS", async () => {
-      process.env.ALLOWED_PROJECT_DIRS = "";
+    it("should handle both ALLOWED_ROOT_DIRECTORY and DATA_DIR", async () => {
+      process.env.ALLOWED_ROOT_DIRECTORY = "/projects";
       process.env.DATA_DIR = "/data";
-      delete process.env.ALLOWED_ROOT_DIRECTORY;
 
-      const { initAllowedPaths, getAllowedPaths } = await import(
-        "@/lib/security.js"
-      );
+      const { initAllowedPaths, getAllowedPaths } =
+        await import("@/lib/security.js");
       initAllowedPaths();
 
       const allowed = getAllowedPaths();
-      expect(allowed).toHaveLength(1);
-      expect(allowed[0]).toBe(path.resolve("/data"));
-    });
-
-    it("should skip empty entries in comma list", async () => {
-      process.env.ALLOWED_PROJECT_DIRS = "/path1,,/path2,  ,/path3";
-      process.env.DATA_DIR = "";
-      delete process.env.ALLOWED_ROOT_DIRECTORY;
-
-      const { initAllowedPaths, getAllowedPaths } = await import(
-        "@/lib/security.js"
-      );
-      initAllowedPaths();
-
-      const allowed = getAllowedPaths();
-      expect(allowed).toHaveLength(3);
+      expect(allowed).toContain(path.resolve("/projects"));
+      expect(allowed).toContain(path.resolve("/data"));
+      expect(allowed).toHaveLength(2);
     });
   });
 
   describe("addAllowedPath", () => {
     it("should add path to allowed list", async () => {
-      process.env.ALLOWED_PROJECT_DIRS = "";
+      delete process.env.ALLOWED_ROOT_DIRECTORY;
       process.env.DATA_DIR = "";
 
       const { initAllowedPaths, addAllowedPath, getAllowedPaths } =
@@ -99,7 +67,7 @@ describe("security.ts", () => {
     });
 
     it("should resolve relative paths before adding", async () => {
-      process.env.ALLOWED_PROJECT_DIRS = "";
+      delete process.env.ALLOWED_ROOT_DIRECTORY;
       process.env.DATA_DIR = "";
 
       const { initAllowedPaths, addAllowedPath, getAllowedPaths } =
@@ -115,14 +83,12 @@ describe("security.ts", () => {
   });
 
   describe("isPathAllowed", () => {
-    it("should allow paths within configured allowed directories", async () => {
-      process.env.ALLOWED_PROJECT_DIRS = "/allowed/project";
+    it("should allow paths within ALLOWED_ROOT_DIRECTORY", async () => {
+      process.env.ALLOWED_ROOT_DIRECTORY = "/allowed/project";
       process.env.DATA_DIR = "";
-      delete process.env.ALLOWED_ROOT_DIRECTORY;
 
-      const { initAllowedPaths, isPathAllowed } = await import(
-        "@/lib/security.js"
-      );
+      const { initAllowedPaths, isPathAllowed } =
+        await import("@/lib/security.js");
       initAllowedPaths();
 
       // Paths within allowed directory should be allowed
@@ -136,13 +102,11 @@ describe("security.ts", () => {
     });
 
     it("should allow all paths when no restrictions are configured", async () => {
-      delete process.env.ALLOWED_PROJECT_DIRS;
       delete process.env.DATA_DIR;
       delete process.env.ALLOWED_ROOT_DIRECTORY;
 
-      const { initAllowedPaths, isPathAllowed } = await import(
-        "@/lib/security.js"
-      );
+      const { initAllowedPaths, isPathAllowed } =
+        await import("@/lib/security.js");
       initAllowedPaths();
 
       // All paths should be allowed when no restrictions are configured
@@ -152,17 +116,33 @@ describe("security.ts", () => {
       expect(isPathAllowed("/etc/passwd")).toBe(true);
       expect(isPathAllowed("/any/path")).toBe(true);
     });
+
+    it("should allow all paths when DATA_DIR is set but ALLOWED_ROOT_DIRECTORY is not", async () => {
+      process.env.DATA_DIR = "/data";
+      delete process.env.ALLOWED_ROOT_DIRECTORY;
+
+      const { initAllowedPaths, isPathAllowed } =
+        await import("@/lib/security.js");
+      initAllowedPaths();
+
+      // DATA_DIR should be allowed
+      expect(isPathAllowed("/data/settings.json")).toBe(true);
+      // But all other paths should also be allowed when ALLOWED_ROOT_DIRECTORY is not set
+      expect(isPathAllowed("/allowed/project/file.txt")).toBe(true);
+      expect(isPathAllowed("/not/allowed/file.txt")).toBe(true);
+      expect(isPathAllowed("/tmp/file.txt")).toBe(true);
+      expect(isPathAllowed("/etc/passwd")).toBe(true);
+      expect(isPathAllowed("/any/path")).toBe(true);
+    });
   });
 
   describe("validatePath", () => {
     it("should return resolved path for allowed paths", async () => {
-      process.env.ALLOWED_PROJECT_DIRS = "/allowed";
+      process.env.ALLOWED_ROOT_DIRECTORY = "/allowed";
       process.env.DATA_DIR = "";
-      delete process.env.ALLOWED_ROOT_DIRECTORY;
 
-      const { initAllowedPaths, validatePath } = await import(
-        "@/lib/security.js"
-      );
+      const { initAllowedPaths, validatePath } =
+        await import("@/lib/security.js");
       initAllowedPaths();
 
       const result = validatePath("/allowed/file.txt");
@@ -170,13 +150,11 @@ describe("security.ts", () => {
     });
 
     it("should throw error for paths outside allowed directories", async () => {
-      process.env.ALLOWED_PROJECT_DIRS = "/allowed";
+      process.env.ALLOWED_ROOT_DIRECTORY = "/allowed";
       process.env.DATA_DIR = "";
-      delete process.env.ALLOWED_ROOT_DIRECTORY;
 
-      const { initAllowedPaths, validatePath } = await import(
-        "@/lib/security.js"
-      );
+      const { initAllowedPaths, validatePath } =
+        await import("@/lib/security.js");
       initAllowedPaths();
 
       // Disallowed paths should throw PathNotAllowedError
@@ -184,13 +162,11 @@ describe("security.ts", () => {
     });
 
     it("should not throw error for any path when no restrictions are configured", async () => {
-      delete process.env.ALLOWED_PROJECT_DIRS;
       delete process.env.DATA_DIR;
       delete process.env.ALLOWED_ROOT_DIRECTORY;
 
-      const { initAllowedPaths, validatePath } = await import(
-        "@/lib/security.js"
-      );
+      const { initAllowedPaths, validatePath } =
+        await import("@/lib/security.js");
       initAllowedPaths();
 
       // All paths are allowed when no restrictions configured
@@ -202,13 +178,11 @@ describe("security.ts", () => {
 
     it("should resolve relative paths within allowed directory", async () => {
       const cwd = process.cwd();
-      process.env.ALLOWED_PROJECT_DIRS = cwd;
+      process.env.ALLOWED_ROOT_DIRECTORY = cwd;
       process.env.DATA_DIR = "";
-      delete process.env.ALLOWED_ROOT_DIRECTORY;
 
-      const { initAllowedPaths, validatePath } = await import(
-        "@/lib/security.js"
-      );
+      const { initAllowedPaths, validatePath } =
+        await import("@/lib/security.js");
       initAllowedPaths();
 
       const result = validatePath("./file.txt");
@@ -218,26 +192,26 @@ describe("security.ts", () => {
 
   describe("getAllowedPaths", () => {
     it("should return array of allowed paths", async () => {
-      process.env.ALLOWED_PROJECT_DIRS = "/path1,/path2";
+      process.env.ALLOWED_ROOT_DIRECTORY = "/projects";
       process.env.DATA_DIR = "/data";
 
-      const { initAllowedPaths, getAllowedPaths } = await import(
-        "@/lib/security.js"
-      );
+      const { initAllowedPaths, getAllowedPaths } =
+        await import("@/lib/security.js");
       initAllowedPaths();
 
       const result = getAllowedPaths();
       expect(Array.isArray(result)).toBe(true);
-      expect(result.length).toBeGreaterThan(0);
+      expect(result.length).toBe(2);
+      expect(result).toContain(path.resolve("/projects"));
+      expect(result).toContain(path.resolve("/data"));
     });
 
     it("should return resolved paths", async () => {
-      process.env.ALLOWED_PROJECT_DIRS = "/test";
+      process.env.ALLOWED_ROOT_DIRECTORY = "/test";
       process.env.DATA_DIR = "";
 
-      const { initAllowedPaths, getAllowedPaths } = await import(
-        "@/lib/security.js"
-      );
+      const { initAllowedPaths, getAllowedPaths } =
+        await import("@/lib/security.js");
       initAllowedPaths();
 
       const result = getAllowedPaths();