Don’t expand globs via symbolic links (prettier#14627)

Co-authored-by: fisker <lionkay@gmail.com>
Harry-Hopkinson · May 4, 2023 · dcc081d · dcc081d
1 parent 42589a1
commit dcc081d
Show file tree

Hide file tree

Showing 6 changed files with 200 additions and 3 deletions.
diff --git a/changelog_unreleased/cli/14627.md b/changelog_unreleased/cli/14627.md
@@ -0,0 +1,6 @@
+#### Don’t expand globs via symbolic links (#14627 by @andersk)
+
+Prettier no longer follows symbolic links while expanding command line
+arguments. This avoids problems in many scenarios such as symlinks
+outside the source tree, symlinks to ignored files, and cycles of
+symlinks.
diff --git a/docs/cli.md b/docs/cli.md
@@ -45,6 +45,8 @@ Given a list of paths/patterns, the Prettier CLI first treats every entry in it
 
 Prettier CLI will ignore files located in `node_modules` directory. To opt out from this behavior, use `--with-node-modules` flag.
 
+Prettier CLI will not follow symbolic links when expanding arguments.
+
 To escape special characters in globs, one of the two escaping syntaxes can be used: `prettier "\[my-dir]/*.js"` or `prettier "[[]my-dir]/*.js"`. Both match all JS files in a directory named `[my-dir]`, however the latter syntax is preferable as the former doesn’t work on Windows, where backslashes are treated as path separators.
 
 ## `--check`

diff --git a/src/cli/expand-patterns.js b/src/cli/expand-patterns.js
@@ -1,5 +1,5 @@
 import path from "node:path";
-import { statSafe, normalizeToPosix } from "./utils.js";
+import { lstatSafe, normalizeToPosix } from "./utils.js";
 import { fastGlob } from "./prettier-internal.js";
 
 /** @typedef {import('./context').Context} Context */
@@ -49,6 +49,7 @@ async function* expandPatternsInternal(context) {
   const globOptions = {
     dot: true,
     ignore: silentlyIgnoredDirs.map((dir) => "**/" + dir),
+    followSymbolicLinks: false,
   };
 
   let supportedFilesGlob;
@@ -64,9 +65,13 @@ async function* expandPatternsInternal(context) {
       continue;
     }
 
-    const stat = await statSafe(absolutePath);
+    const stat = await lstatSafe(absolutePath);
     if (stat) {
-      if (stat.isFile()) {
+      if (stat.isSymbolicLink()) {
+        yield {
+          error: `Explicitly specified pattern "${pattern}" is a symbolic link.`,
+        };
+      } else if (stat.isFile()) {
         entries.push({
           type: "file",
           glob: escapePathForGlob(fixWindowsSlashes(pattern)),

diff --git a/src/cli/utils.js b/src/cli/utils.js
@@ -66,6 +66,22 @@ async function statSafe(filePath) {
   }
 }
 
+/**
+ * Get stats of a given path without following symbolic links.
+ * @param {string} filePath The path to target file.
+ * @returns {Promise<import('fs').Stats | undefined>} The stats.
+ */
+async function lstatSafe(filePath) {
+  try {
+    return await fs.lstat(filePath);
+  } catch (error) {
+    /* c8 ignore next 3 */
+    if (error.code !== "ENOENT") {
+      throw error;
+    }
+  }
+}
+
 /**
  * @param {string} value
  * @returns {boolean}
@@ -96,6 +112,7 @@ export {
   pick,
   createHash,
   statSafe,
+  lstatSafe,
   isJson,
   normalizeToPosix,
 };
diff --git a/tests/integration/__tests__/patterns-dirs.js b/tests/integration/__tests__/patterns-dirs.js
@@ -152,6 +152,171 @@ if (path.sep === "/") {
   });
 }
 
+function isSymlinkSupported() {
+  if (process.platform !== "win32") {
+    return true;
+  }
+
+  const target = path.join(
+    __dirname,
+    "../cli/patterns-symlinks/test-symlink-feature-detect"
+  );
+  fs.rmSync(target, { force: true, recursive: true });
+  fs.mkdirSync(target);
+  const symlink = path.join(target, "symlink");
+  try {
+    fs.symlinkSync(target, symlink);
+  } catch {
+    return false;
+  }
+  return fs.lstatSync(symlink).isSymbolicLink();
+}
+
+(isSymlinkSupported() ? describe : describe.skip)("Ignore symlinks", () => {
+  const base = path.join(__dirname, "../cli/patterns-symlinks");
+  const directoryA = path.join(base, "test-a");
+  const directoryB = path.join(base, "test-b");
+  const clean = () => {
+    fs.rmSync(directoryA, { force: true, recursive: true });
+    fs.rmSync(directoryB, { force: true, recursive: true });
+  };
+  beforeAll(() => {
+    clean();
+    fs.mkdirSync(directoryA);
+    fs.mkdirSync(directoryB);
+    fs.writeFileSync(path.join(directoryA, "a.js"), "x");
+    fs.writeFileSync(path.join(directoryB, "b.js"), "x");
+    fs.symlinkSync(directoryA, path.join(directoryA, "symlink-to-directory-a"));
+    fs.symlinkSync(directoryB, path.join(directoryA, "symlink-to-directory-b"));
+    fs.symlinkSync(
+      path.join(directoryA, "a.js"),
+      path.join(directoryA, "symlink-to-file-a")
+    );
+    fs.symlinkSync(
+      path.join(directoryB, "b.js"),
+      path.join(directoryA, "symlink-to-file-b")
+    );
+  });
+  afterAll(clean);
+
+  test("file struct", async () => {
+    const getFileStruct = async (directory) =>
+      (await fs.promises.readdir(directory, { withFileTypes: true }))
+        .map((dirent) => ({
+          name: dirent.name,
+          isSymbolicLink: dirent.isSymbolicLink(),
+        }))
+        .sort((a, b) => a.name.localeCompare(b.name));
+
+    expect(await getFileStruct(directoryA)).toMatchInlineSnapshot(`
+      [
+        {
+          "isSymbolicLink": false,
+          "name": "a.js",
+        },
+        {
+          "isSymbolicLink": true,
+          "name": "symlink-to-directory-a",
+        },
+        {
+          "isSymbolicLink": true,
+          "name": "symlink-to-directory-b",
+        },
+        {
+          "isSymbolicLink": true,
+          "name": "symlink-to-file-a",
+        },
+        {
+          "isSymbolicLink": true,
+          "name": "symlink-to-file-b",
+        },
+      ]
+    `);
+    expect(await getFileStruct(directoryB)).toMatchInlineSnapshot(`
+      [
+        {
+          "isSymbolicLink": false,
+          "name": "b.js",
+        },
+      ]
+    `);
+  });
+
+  testPatterns("", ["test-a/*"], { stdout: "test-a/a.js" }, base);
+  testPatterns(
+    "",
+    ["test-a/symlink-to-directory-a"],
+    {
+      status: 2,
+      stdout: "",
+      stderr:
+        '[error] Explicitly specified pattern "test-a/symlink-to-directory-a" is a symbolic link.',
+    },
+    base
+  );
+  testPatterns(
+    "",
+    ["test-a/symlink-to-directory-b"],
+    {
+      status: 2,
+      stdout: "",
+      stderr:
+        '[error] Explicitly specified pattern "test-a/symlink-to-directory-b" is a symbolic link.',
+    },
+    base
+  );
+  testPatterns(
+    "",
+    ["test-a/symlink-to-file-a"],
+    {
+      status: 2,
+      stdout: "",
+      stderr:
+        '[error] Explicitly specified pattern "test-a/symlink-to-file-a" is a symbolic link.',
+    },
+    base
+  );
+  testPatterns(
+    "",
+    ["test-a/symlink-to-file-b"],
+    {
+      status: 2,
+      stdout: "",
+      stderr:
+        '[error] Explicitly specified pattern "test-a/symlink-to-file-b" is a symbolic link.',
+    },
+    base
+  );
+  testPatterns(
+    "",
+    ["test-a/symlink-*"],
+    {
+      status: 2,
+      stdout: "",
+      stderr:
+        '[error] No files matching the pattern were found: "test-a/symlink-*".',
+    },
+    base
+  );
+  testPatterns(
+    "",
+    ["test-a/*", "test-a/symlink-to-file-b"],
+    {
+      status: 2,
+      stdout: "test-a/a.js",
+      stderr:
+        '[error] Explicitly specified pattern "test-a/symlink-to-file-b" is a symbolic link.',
+    },
+    base
+  );
+  testPatterns(
+    "",
+    ["test-a/symlink-to-directory-b/*"],
+    { stdout: "test-a/symlink-to-directory-b/b.js" },
+    base
+  );
+});
+
 function testPatterns(
   namePrefix,
   cliArgs,

diff --git a/tests/integration/cli/patterns-symlinks/.gitignore b/tests/integration/cli/patterns-symlinks/.gitignore
@@ -0,0 +1,2 @@
+# dynamically created by tests
+test-*