fix(attachments): fixed attachment download functionality and path validation

Author: Chris Funderburg

server/src/index.ts

@@ -11,14 +11,14 @@ import searchRoutes from "./routes/search";
 import attachmentRoutes from "./routes/attachments";
 import { requireAuth } from "./middleware/auth";
 import { GitService } from "./services/gitService";
-import { FileSystemService } from "./services/fileSystem";
+import { DATA_DIR as DEFAULT_DATA_DIR, FileSystemService } from "./services/fileSystem";
 
 const app: Express = express();
 const PORT = process.env.PORT || 3001;
 
 // Initialize Git service
 const DATA_DIR =
-  process.env.TEST_DATA_DIR || path.join(__dirname, "../../data");
+  process.env.TEST_DATA_DIR || process.env.DATA_DIR || DEFAULT_DATA_DIR;
 export let gitService = new GitService(DATA_DIR);
 export let fileSystemService = new FileSystemService(DATA_DIR, gitService);
 
@@ -238,7 +238,7 @@ if (process.env.NODE_ENV === "production") {
   app.use(express.static(clientDistPath));
 
   // Handle client-side routing - serve index.html for all non-API routes
-  app.get("*", (req: Request, res: Response) => {
+  app.get(/.*/, (req: Request, res: Response) => {
     res.sendFile(path.join(clientDistPath, "index.html"));
   });
 }

server/src/routes/attachments.ts

@@ -2,11 +2,31 @@ import { Router, Request, Response } from "express";
 import multer from "multer";
 import path from "path";
 import fs from "fs/promises";
-import { DATA_DIR } from "../services/fileSystem";
+import { DATA_DIR as DEFAULT_DATA_DIR } from "../services/fileSystem";
 import { fileTransferLimiter, fileOperationLimiter } from "../middleware/rateLimiters";
 
 const router = Router();
 
+const getDataDir = (): string => {
+  return process.env.TEST_DATA_DIR || process.env.DATA_DIR || DEFAULT_DATA_DIR;
+};
+
+const getFolderQueryParam = (req: Request): string => {
+  const folderQuery = req.query.folder;
+  if (typeof folderQuery === "string") {
+    return folderQuery;
+  }
+
+  // Fallback: parse raw URL so '+' is treated as space (x-www-form-urlencoded semantics)
+  // This matches how browsers/axios tend to encode URL query parameters.
+  try {
+    const url = new URL(req.originalUrl, "http://localhost");
+    return url.searchParams.get("folder") || "";
+  } catch {
+    return "";
+  }
+};
+
 /**
  * Validates and sanitizes a path to prevent path traversal attacks.
  * Ensures the resolved path is within the allowed base directory.
@@ -33,10 +53,12 @@ const validatePath = (basePath: string, ...userPath: string[]): string => {
 const storage = multer.diskStorage({
   destination: async (req, file, cb) => {
     try {
-      const folderPath = (req.query.folder as string) || "";
+      const folderPath = getFolderQueryParam(req);
+
+      const dataDir = getDataDir();
 
       // Validate path to prevent traversal
-      const attachmentsDir = validatePath(DATA_DIR, folderPath, ".attachments");
+      const attachmentsDir = validatePath(dataDir, folderPath, ".attachments");
 
       // Create .attachments directory if it doesn't exist
       await fs.mkdir(attachmentsDir, { recursive: true });
@@ -83,10 +105,12 @@ router.post("/", fileTransferLimiter, upload.single("file"), async (req: Request
 // List attachments for a folder
 router.get("/", async (req: Request, res: Response) => {
   try {
-    const folderPath = (req.query.folder as string) || "";
+    const folderPath = getFolderQueryParam(req);
+
+    const dataDir = getDataDir();
 
     // Validate path to prevent traversal
-    const attachmentsDir = validatePath(DATA_DIR, folderPath, ".attachments");
+    const attachmentsDir = validatePath(dataDir, folderPath, ".attachments");
 
     try {
       const files = await fs.readdir(attachmentsDir);
@@ -120,7 +144,7 @@ router.get("/", async (req: Request, res: Response) => {
 // Delete attachment
 router.delete("/:filename", fileOperationLimiter, async (req: Request, res: Response) => {
   try {
-    const folderPath = (req.query.folder as string) || "";
+    const folderPath = getFolderQueryParam(req);
     const filenameParam = req.params.filename;
 
     if (!filenameParam || Array.isArray(filenameParam)) {
@@ -129,8 +153,10 @@ router.delete("/:filename", fileOperationLimiter, async (req: Request, res: Resp
 
     const filename = filenameParam;
 
+    const dataDir = getDataDir();
+
     // Validate path to prevent traversal - this validates both folderPath and filename
-    const filePath = validatePath(DATA_DIR, folderPath, ".attachments", filename);
+    const filePath = validatePath(dataDir, folderPath, ".attachments", filename);
 
     await fs.unlink(filePath);
     res.json({ success: true });
@@ -149,7 +175,7 @@ router.delete("/:filename", fileOperationLimiter, async (req: Request, res: Resp
 // Get/download attachment
 router.get("/:filename", fileTransferLimiter, async (req: Request, res: Response) => {
   try {
-    const folderPath = (req.query.folder as string) || "";
+    const folderPath = getFolderQueryParam(req);
     const filenameParam = req.params.filename;
 
     if (!filenameParam || Array.isArray(filenameParam)) {
@@ -158,12 +184,39 @@ router.get("/:filename", fileTransferLimiter, async (req: Request, res: Response
 
     const filename = filenameParam;
 
+    const dataDir = getDataDir();
+
     // Validate path to prevent traversal - this validates both folderPath and filename
-    const filePath = validatePath(DATA_DIR, folderPath, ".attachments", filename);
+    const filePath = validatePath(dataDir, folderPath, ".attachments", filename);
 
     // nosemgrep: javascript.express.security.audit.express-res-sendfile.express-res-sendfile
     // Safe: filePath has been validated by validatePath() to prevent directory traversal
-    res.sendFile(filePath);
+    res.sendFile(filePath, { dotfiles: "allow" }, (error) => {
+      if (!error) {
+        return;
+      }
+
+      // `send` defaults to ignoring dotfiles; our attachments live under `.attachments`.
+      // Explicitly allow dotfiles above; remaining errors indicate missing/forbidden files.
+      if ((error as any).code === "ENOENT" || (error as any).statusCode === 404) {
+        if (!res.headersSent) {
+          res.status(404).json({ error: "File not found" });
+        }
+        return;
+      }
+
+      if ((error as any).code === "EACCES" || (error as any).statusCode === 403) {
+        if (!res.headersSent) {
+          res.status(403).json({ error: "Forbidden" });
+        }
+        return;
+      }
+
+      console.error("Get attachment sendFile error:", error);
+      if (!res.headersSent) {
+        res.status(500).json({ error: "Failed to retrieve file" });
+      }
+    });
   } catch (error: any) {
     if (error.message === "Path traversal detected") {
       return res.status(403).json({ error: "Invalid file path" });

server/tests/api.test.ts

@@ -180,6 +180,22 @@ describe('API Tests', () => {
     });
   });
 
+  describe('Attachment Operations', () => {
+    it('should download an attachment from a .attachments folder', async () => {
+      const attachmentsDir = path.join(TEST_DATA_DIR, 'Howden', '.attachments');
+      await fs.mkdir(attachmentsDir, { recursive: true });
+
+      const filename = 'test-attachment.txt';
+      const filePath = path.join(attachmentsDir, filename);
+      await fs.writeFile(filePath, 'hello attachment');
+
+      const response = await request(app).get(`/api/attachments/${filename}?folder=Howden`);
+
+      expect(response.status).toBe(200);
+      expect(response.text).toBe('hello attachment');
+    });
+  });
+
   describe('Health Check', () => {
     it('should return ok status', async () => {
       const response = await request(app).get('/api/health');