security: ignore symlinks by default in fsAsync.readdirRecursive

src/fsAsync.spec.ts

@@ -128,28 +128,46 @@ describe("fsAsync", () => {
       return expect(gotFileNames).to.deep.equal(expectFiles);
     });
 
-    it("should ignore symlinks when option is set", async () => {
-      const symlinkPath = path.join(baseDir, "symlink");
+    it("ignores symlinks by default (security: prevents symlink-following during deploy)", async () => {
+      const symlinkPath = path.join(baseDir, "default-symlink");
+      fs.symlinkSync(path.join(baseDir, "visible"), symlinkPath);
+
+      try {
+        const results = await fsAsync.readdirRecursive({ path: baseDir });
+        const gotFileNames = results.map((r) => r.name).sort();
+        const expectFiles = files.map((file) => path.join(baseDir, file)).sort();
+        // Symlink MUST NOT appear in the result by default. Following symlinks
+        // during deploy/upload is a security boundary issue (see JSDoc on
+        // ReaddirRecursiveOpts.ignoreSymlinks).
+        expect(gotFileNames).to.deep.equal(expectFiles);
+        expect(gotFileNames).to.not.include(symlinkPath);
+      } finally {
+        fs.unlinkSync(symlinkPath);
+      }
+    });
+
+    it("includes symlinks only when ignoreSymlinks is explicitly set to false", async () => {
+      const symlinkPath = path.join(baseDir, "explicit-symlink");
       fs.symlinkSync(path.join(baseDir, "visible"), symlinkPath);
 
       try {
         const resultsWithSymlinks = await fsAsync.readdirRecursive({
           path: baseDir,
-          ignoreSymlinks: true,
+          ignoreSymlinks: false,
         });
         const gotFileNamesWithSymlinks = resultsWithSymlinks.map((r) => r.name).sort();
-        const expectFiles = files.map((file) => path.join(baseDir, file)).sort();
-        expect(gotFileNamesWithSymlinks).to.deep.equal(expectFiles);
+        const expectFilesWithSymlinks = [...files, "explicit-symlink"]
+          .map((file) => path.join(baseDir, file))
+          .sort();
+        expect(gotFileNamesWithSymlinks).to.deep.equal(expectFilesWithSymlinks);
 
         const resultsWithoutSymlinks = await fsAsync.readdirRecursive({
           path: baseDir,
-          ignoreSymlinks: false,
+          ignoreSymlinks: true,
         });
         const gotFileNamesWithoutSymlinks = resultsWithoutSymlinks.map((r) => r.name).sort();
-        const expectFilesWithSymlinks = [...files, "symlink"]
-          .map((file) => path.join(baseDir, file))
-          .sort();
-        expect(gotFileNamesWithoutSymlinks).to.deep.equal(expectFilesWithSymlinks);
+        const expectFiles = files.map((file) => path.join(baseDir, file)).sort();
+        expect(gotFileNamesWithoutSymlinks).to.deep.equal(expectFiles);
       } finally {
         fs.unlinkSync(symlinkPath);
       }

src/fsAsync.ts

@@ -3,6 +3,7 @@ import ignorePkg from "ignore";
 import * as _ from "lodash";
 import * as minimatch from "minimatch";
 import { join, relative } from "path";
+import { logger } from "./logger";
 
 export interface ReaddirRecursiveOpts {
   // The directory to recurse.
@@ -14,7 +15,19 @@ export interface ReaddirRecursiveOpts {
   include?: string[];
   // Maximum depth to recurse.
   maxDepth?: number;
-  // Ignore symlinked files or directories when traversing.
+  /**
+   * Ignore symlinked files or directories when traversing.
+   *
+   * **Defaults to `true`** for security: following symlinks during deploy/upload
+   * lets a path inside the source tree leak the contents of an arbitrary path
+   * outside it (including secrets like `/proc/self/environ`, SSH keys, or other
+   * credentials reachable from the deploy process). Pass `false` only if you
+   * have an audited reason to follow symlinks.
+   *
+   * If a symlink is encountered while this is `true`, the entry is silently
+   * dropped from the recursion (matching the prior `ignoreSymlinks: true`
+   * behavior). A `debug`-level log entry is emitted to aid debugging.
+   */
   ignoreSymlinks?: boolean;
 }
 
@@ -31,7 +44,16 @@ async function readdirRecursiveHelper(options: {
 }): Promise<ReaddirRecursiveFile[]> {
   const dirContents = readdirSync(options.path, { withFileTypes: true });
   const fullPaths = dirContents
-    .filter((n) => !options.ignoreSymlinks || !n.isSymbolicLink())
+    .filter((n) => {
+      if (options.ignoreSymlinks && n.isSymbolicLink()) {
+        logger.debug(
+          `[fsAsync] dropping symlink ${join(options.path, n.name)} ` +
+            `(set ignoreSymlinks:false to include it)`,
+        );
+        return false;
+      }
+      return true;
+    })
     .map((n) => join(options.path, n.name));
   const filteredPaths = fullPaths.filter((p) => !options.filter(p));
   const filePromises: Array<Promise<ReaddirRecursiveFile | ReaddirRecursiveFile[]>> = [];
@@ -63,6 +85,15 @@ async function readdirRecursiveHelper(options: {
 
 /**
  * Recursively read a directory.
+ *
+ * **Security note:** `ignoreSymlinks` defaults to `true`. Callers that
+ * upload, archive, or otherwise transport the resulting file list across
+ * a trust boundary (e.g. Cloud Functions deploy, App Hosting deploy,
+ * Hosting upload) MUST keep this default. Passing `false` causes the
+ * recursion to follow symlinks and may include arbitrary attacker-controlled
+ * targets in the produced file list (e.g. `/proc/self/environ`,
+ * `~/.ssh/id_rsa`, GCP application-default-credentials JSON).
+ *
  * @param options options object.
  * @return array of files that match.
  */
@@ -88,10 +119,12 @@ export async function readdirRecursive(
     });
   };
   const maxDepth = options.maxDepth && options.maxDepth > 0 ? options.maxDepth : Infinity;
+  // Default `ignoreSymlinks` to `true`. See JSDoc above for security rationale.
+  const ignoreSymlinks = options.ignoreSymlinks !== false;
   return await readdirRecursiveHelper({
     path: options.path,
     filter: filter,
     maxDepth,
-    ignoreSymlinks: !!options.ignoreSymlinks,
+    ignoreSymlinks,
   });
 }