fix: address security and reliability issues in cp operations

- Add sanitizePath to prevent path traversal attacks
- Implement recursive directory upload
- Handle WebSocket close properly to prevent promise hang
- Fix race condition between file close and utimesSync
- Append to existing pathname in buildWsURL
- Separate JSON parsing try-catch from FS operations

Author: rgarcia

src/lib/cp.ts

@@ -85,12 +85,44 @@ interface CpError {
 
 type CpMessage = CpFileHeader | CpEndMarker | CpResult | CpError;
 
+/**
+ * Sanitizes a path from the server to prevent path traversal attacks.
+ * Ensures the path stays within the destination directory.
+ */
+function sanitizePath(base: string, relativePath: string): string {
+  // Normalize the path to resolve . and .. components
+  const normalized = path.normalize(relativePath);
+
+  // Reject absolute paths
+  if (path.isAbsolute(normalized)) {
+    throw new Error(`Invalid path: absolute paths not allowed: ${relativePath}`);
+  }
+
+  // Reject paths that escape the destination
+  if (normalized.startsWith('..')) {
+    throw new Error(`Invalid path: path escapes destination: ${relativePath}`);
+  }
+
+  // Join with base and verify the result is under base
+  const result = path.join(base, normalized);
+  const absBase = path.resolve(base);
+  const absResult = path.resolve(result);
+
+  if (!absResult.startsWith(absBase + path.sep) && absResult !== absBase) {
+    throw new Error(`Invalid path: path escapes destination: ${relativePath}`);
+  }
+
+  return result;
+}
+
 /**
  * Builds the WebSocket URL for the cp endpoint.
  */
 function buildWsURL(baseURL: string, instanceId: string): string {
   const url = new URL(baseURL);
-  url.pathname = `/instances/${instanceId}/cp`;
+  // Append to existing pathname instead of overwriting
+  const existingPath = url.pathname.endsWith('/') ? url.pathname.slice(0, -1) : url.pathname;
+  url.pathname = `${existingPath}/instances/${instanceId}/cp`;
 
   if (url.protocol === 'https:') {
     url.protocol = 'wss:';
@@ -119,14 +151,54 @@ function buildWsURL(baseURL: string, instanceId: string): string {
  * ```
  */
 export async function cpToInstance(cfg: CpConfig, opts: CpToInstanceOptions): Promise<void> {
+  // Get file stats
+  const stats = fs.statSync(opts.srcPath);
+  const isDir = stats.isDirectory();
+
+  if (isDir) {
+    // For directories, first create the directory, then recursively copy contents
+    await cpSingleFileToInstance(cfg, {
+      ...opts,
+      isDir: true,
+    });
+
+    // Now recursively copy all contents
+    const entries = fs.readdirSync(opts.srcPath, { withFileTypes: true });
+    for (const entry of entries) {
+      const srcEntryPath = path.join(opts.srcPath, entry.name);
+      // Use path.posix.join for guest paths to ensure forward slashes
+      const dstEntryPath = path.posix.join(opts.dstPath, entry.name);
+
+      await cpToInstance(cfg, {
+        instanceId: opts.instanceId,
+        srcPath: srcEntryPath,
+        dstPath: dstEntryPath,
+        ...(opts.mode !== undefined && { mode: opts.mode }),
+      });
+    }
+  } else {
+    // For files, copy directly
+    await cpSingleFileToInstance(cfg, {
+      ...opts,
+      isDir: false,
+    });
+  }
+}
+
+/**
+ * Internal function to copy a single file or create a directory.
+ */
+async function cpSingleFileToInstance(
+  cfg: CpConfig,
+  opts: CpToInstanceOptions & { isDir: boolean },
+): Promise<void> {
   // Check if WebSocket is available (Node.js vs browser)
   const WebSocket = getWebSocket();
 
   const wsURL = buildWsURL(cfg.baseURL, opts.instanceId);
 
   // Get file stats
   const stats = fs.statSync(opts.srcPath);
-  const isDir = stats.isDirectory();
   const mode = opts.mode ?? stats.mode & 0o777;
 
   // Connect to WebSocket
@@ -137,26 +209,42 @@ export async function cpToInstance(cfg: CpConfig, opts: CpToInstanceOptions): Pr
   });
 
   return new Promise((resolve, reject) => {
+    let settled = false;
+
+    const doResolve = () => {
+      if (!settled) {
+        settled = true;
+        resolve();
+      }
+    };
+
+    const doReject = (err: Error) => {
+      if (!settled) {
+        settled = true;
+        reject(err);
+      }
+    };
+
     ws.on('open', () => {
       // Send initial request
       const req: CpRequest = {
         direction: 'to',
         guest_path: opts.dstPath,
-        is_dir: isDir,
+        is_dir: opts.isDir,
         mode: mode,
       };
       ws.send(JSON.stringify(req));
 
-      if (isDir) {
-        // For directories, just send end marker
+      if (opts.isDir) {
+        // For directories, just send end marker - the directory will be created
         ws.send(JSON.stringify({ type: 'end' }));
       } else {
         // Stream file content
         const fileStream = fs.createReadStream(opts.srcPath, {
           highWaterMark: 32 * 1024,
         });
 
-        fileStream.on('data', (chunk) => {
+        fileStream.on('data', (chunk: any) => {
           ws.send(chunk as Buffer);
         });
 
@@ -166,7 +254,7 @@ export async function cpToInstance(cfg: CpConfig, opts: CpToInstanceOptions): Pr
 
         fileStream.on('error', (err) => {
           ws.close();
-          reject(err);
+          doReject(err);
         });
       }
     });
@@ -178,27 +266,31 @@ export async function cpToInstance(cfg: CpConfig, opts: CpToInstanceOptions): Pr
         if (msg.type === 'result') {
           if (msg.success) {
             ws.close();
-            resolve();
+            doResolve();
           } else {
             ws.close();
-            reject(new Error(`Copy failed: ${msg.error}`));
+            doReject(new Error(`Copy failed: ${msg.error}`));
           }
         } else if (msg.type === 'error') {
           ws.close();
-          reject(new Error(`Copy error at ${msg.path}: ${msg.message}`));
+          doReject(new Error(`Copy error at ${msg.path}: ${msg.message}`));
         }
-      } catch (e) {
+      } catch {
         // Ignore non-JSON messages
       }
     });
 
     ws.on('error', (err) => {
-      reject(err);
+      doReject(err);
     });
 
     ws.on('close', (code, reason) => {
-      if (code !== 1000) {
-        reject(new Error(`WebSocket closed: ${code} ${reason}`));
+      if (!settled) {
+        if (code === 1000) {
+          doReject(new Error('WebSocket closed before receiving result'));
+        } else {
+          doReject(new Error(`WebSocket closed: ${code} ${reason}`));
+        }
       }
     });
   });
@@ -234,6 +326,32 @@ export async function cpFromInstance(cfg: CpConfig, opts: CpFromInstanceOptions)
   return new Promise((resolve, reject) => {
     let currentFile: fs.WriteStream | null = null;
     let currentHeader: CpFileHeader | null = null;
+    let settled = false;
+
+    const doResolve = () => {
+      if (!settled) {
+        settled = true;
+        resolve();
+      }
+    };
+
+    const doReject = (err: Error) => {
+      if (!settled) {
+        settled = true;
+        reject(err);
+      }
+    };
+
+    // Helper to close file and wait for it to complete
+    const closeCurrentFile = (): Promise<void> => {
+      return new Promise((res) => {
+        if (currentFile) {
+          currentFile.end(() => res());
+        } else {
+          res();
+        }
+      });
+    };
 
     ws.on('open', () => {
       // Send initial request
@@ -247,30 +365,46 @@ export async function cpFromInstance(cfg: CpConfig, opts: CpFromInstanceOptions)
 
     ws.on('message', (data: Buffer | string) => {
       // Try to parse as JSON first
+      let msg: CpMessage | null = null;
       if (typeof data === 'string' || (Buffer.isBuffer(data) && !isBinaryData(data))) {
         try {
-          const msg = JSON.parse(data.toString()) as CpMessage;
+          msg = JSON.parse(data.toString()) as CpMessage;
+        } catch {
+          // Not JSON, treat as binary data below
+        }
+      }
 
+      // Handle parsed message (outside try-catch so FS errors propagate)
+      if (msg) {
+        try {
           switch (msg.type) {
             case 'header': {
-              // Close previous file if any
+              // Close previous file if any (synchronously for simplicity in message handler)
               if (currentFile) {
                 currentFile.close();
                 currentFile = null;
               }
 
               currentHeader = msg as CpFileHeader;
-              const targetPath = path.join(opts.dstPath, currentHeader.path);
+              // Sanitize server-provided path to prevent path traversal attacks
+              const targetPath = sanitizePath(opts.dstPath, currentHeader.path);
 
               if (currentHeader.is_dir) {
                 fs.mkdirSync(targetPath, { recursive: true, mode: currentHeader.mode });
               } else if (currentHeader.is_symlink && currentHeader.link_target) {
+                // Validate symlink target
+                const linkTarget = currentHeader.link_target;
+                if (path.isAbsolute(linkTarget) || path.normalize(linkTarget).startsWith('..')) {
+                  doReject(new Error(`Invalid symlink target: ${linkTarget}`));
+                  ws.close();
+                  return;
+                }
                 try {
                   fs.unlinkSync(targetPath);
                 } catch {
                   // Ignore if doesn't exist
                 }
-                fs.symlinkSync(currentHeader.link_target, targetPath);
+                fs.symlinkSync(linkTarget, targetPath);
               } else {
                 fs.mkdirSync(path.dirname(targetPath), { recursive: true });
                 currentFile = fs.createWriteStream(targetPath, { mode: currentHeader.mode });
@@ -281,36 +415,50 @@ export async function cpFromInstance(cfg: CpConfig, opts: CpFromInstanceOptions)
             case 'end': {
               const endMsg = msg as CpEndMarker;
               if (currentFile) {
-                currentFile.close();
-                // Set modification time
-                if (currentHeader && currentHeader.mtime > 0) {
-                  const targetPath = path.join(opts.dstPath, currentHeader.path);
-                  const mtime = new Date(currentHeader.mtime * 1000);
-                  fs.utimesSync(targetPath, mtime, mtime);
-                }
+                const fileToClose = currentFile;
+                const headerToUse = currentHeader;
                 currentFile = null;
                 currentHeader = null;
+
+                // Wait for file to close before setting mtime
+                fileToClose.end(() => {
+                  if (headerToUse && headerToUse.mtime > 0) {
+                    try {
+                      const targetPath = sanitizePath(opts.dstPath, headerToUse.path);
+                      const mtime = new Date(headerToUse.mtime * 1000);
+                      fs.utimesSync(targetPath, mtime, mtime);
+                    } catch (err) {
+                      // Log but don't fail for mtime errors
+                    }
+                  }
+                  if (endMsg.final) {
+                    ws.close();
+                    doResolve();
+                  }
+                });
+                return; // Exit early since we handle resolution in callback
               }
 
               if (endMsg.final) {
                 ws.close();
-                resolve();
+                doResolve();
               }
               break;
             }
 
             case 'error': {
               const errMsg = msg as CpError;
               ws.close();
-              reject(new Error(`Copy error at ${errMsg.path}: ${errMsg.message}`));
+              doReject(new Error(`Copy error at ${errMsg.path}: ${errMsg.message}`));
               break;
             }
           }
-
-          return;
-        } catch {
-          // Not JSON, treat as binary data
+        } catch (err) {
+          // FS operation failed - propagate the error
+          ws.close();
+          doReject(err as Error);
         }
+        return;
       }
 
       // Binary data - write to current file
@@ -323,15 +471,19 @@ export async function cpFromInstance(cfg: CpConfig, opts: CpFromInstanceOptions)
       if (currentFile) {
         currentFile.close();
       }
-      reject(err);
+      doReject(err);
     });
 
     ws.on('close', (code, reason) => {
       if (currentFile) {
         currentFile.close();
       }
-      if (code !== 1000) {
-        reject(new Error(`WebSocket closed: ${code} ${reason}`));
+      if (!settled) {
+        if (code === 1000) {
+          doReject(new Error('WebSocket closed before receiving final marker'));
+        } else {
+          doReject(new Error(`WebSocket closed: ${code} ${reason}`));
+        }
       }
     });
   });