fix(core): avoid crash when using copyFrom together with symlinks (#…

…6485)

* fix(core): avoid crash when using `copyFrom` together with symlinks

This commit resolves an error "Failed copying a file because a directory
exists at the target path" which was a regression introduced in #6430.

Fixes #6456

* fix: also reproduce symlinks when using patterns

* fix: allow type changes (e.g. replacing a file with a symlink)

* fix(vcs): fix several issues with symlinks

- Do not ignore symlinks whose target does not exist. These symlinks are
  perfectly valid.
- Ignore relative symlinks that point outside of the repository, even if
  they do not start with ..

core/src/build-staging/helpers.ts

@@ -112,8 +112,8 @@ export function cloneFile(
       }
 
       if (toStats) {
-        // If target is a directory or source is a symbolic link and deletes are allowed, delete the target before copying, otherwise throw
-        if (toStats.isDirectory() || fromStats.isSymbolicLink()) {
+        // If target is a directory and deletes are allowed, delete the target before copying, otherwise throw
+        if (toStats.isDirectory()) {
           if (allowDelete) {
             return remove(to, (removeErr) => {
               if (removeErr) {
@@ -131,12 +131,28 @@ export function cloneFile(
           }
         }
 
-        // Skip if both files exist, size and mtime is the same (to a precision of 2 decimal points)
-        if (fromStats.size === toStats.size && round(fromStats.mtimeMs, 2) === round(toStats.mtimeMs, 2)) {
+        if (fromStats.isSymbolicLink() && toStats.isSymbolicLink() && fromStats.targetPath === toStats.targetPath) {
+          // skip if both symlinks are equal
+          return done(null, { skipped: true })
+        } else if (fromStats.size === toStats.size && round(fromStats.mtimeMs, 2) === round(toStats.mtimeMs, 2)) {
+          // Skip if both files exist, size and mtime is the same (to a precision of 2 decimal points)
           return done(null, { skipped: true })
         }
       }
 
+      // if we are about to copy a symlink, and the target path exists, we must remove it first
+      // this allows for type changes (e.g. replacing a file with a symlink, then running garden build)
+      // at this point we know the target is a file or a symlink, so we can do this even if allowDelete=false (copy also overwrites the target)
+      if (fromStats.isSymbolicLink()) {
+        return remove(to, (removeErr) => {
+          if (removeErr) {
+            return done(removeErr)
+          } else {
+            return doClone({ from, to, fromStats: fromStats!, done, statsHelper })
+          }
+        })
+      }
+
       return doClone({ from, to, fromStats, done, statsHelper })
     }
   )
@@ -175,29 +191,29 @@ function doClone(params: CopyParams) {
     }
 
     if (fromStats.isSymbolicLink()) {
-      // reproduce the symbolic link. Validation happens before.
-      if (fromStats.targetPath) {
-        symlink(
-          fromStats.targetPath,
-          to,
-          // relevant on windows
-          // nodejs will auto-detect this on windows, but if the symlink is copied before the target then the auto-detection will get it wrong.
-          fromStats.target?.isDirectory() ? "dir" : "file",
-          (symlinkErr) => {
-            if (symlinkErr) {
-              return done(symlinkErr)
-            }
-
-            setUtimes()
-          }
-        )
-      } else {
+      if (!fromStats.targetPath) {
         return done(
           new InternalError({
             message: "Source is a symbolic link, but targetPath was null or undefined.",
           })
         )
       }
+
+      // reproduce the symbolic link
+      symlink(
+        fromStats.targetPath,
+        to,
+        // relevant on windows
+        // nodejs will auto-detect this on windows, but if the symlink is copied before the target then the auto-detection will get it wrong.
+        fromStats.target?.isDirectory() ? "dir" : "file",
+        (symlinkErr) => {
+          if (symlinkErr) {
+            return done(symlinkErr)
+          }
+
+          setUtimes()
+        }
+      )
     } else if (fromStats.isFile()) {
       // COPYFILE_FICLONE instructs the function to use a copy-on-write reflink on platforms/filesystems where available
       copyFile(from, to, constants.COPYFILE_FICLONE, (copyErr) => {
@@ -234,7 +250,9 @@ export async function scanDirectoryForClone(root: string, pattern?: string): Pro
 
   // No wildcards, so we just read and return the entire set of files from the source directory.
   if (!pattern) {
-    return (await readdir.readdirAsync(root, { deep: true, filter: (stats) => stats.isFile() })).map((f) => [f, f])
+    return (
+      await readdir.readdirAsync(root, { deep: true, filter: (stats) => stats.isFile() || stats.isSymbolicLink() })
+    ).map((f) => [f, f])
   }
 
   // We have a pattern to match, so we go into the more complex routine.
@@ -256,7 +274,7 @@ export async function scanDirectoryForClone(root: string, pattern?: string): Pro
     filter: (stats) => {
       // See if this is a file within a previously matched directory
       // Note: `stats.path` is always POSIX formatted, relative to `sourceRoot`
-      if (stats.isFile()) {
+      if (stats.isFile() || stats.isSymbolicLink()) {
         for (const dirPath of matchedDirectories) {
           if (stats.path.startsWith(dirPath + "/")) {
             // The file is in a matched directory. We need to map the target path, such that everything ahead of
@@ -270,7 +288,7 @@ export async function scanDirectoryForClone(root: string, pattern?: string): Pro
       if (mm.match(stats.path)) {
         if (stats.isDirectory()) {
           matchedDirectories.push(stats.path)
-        } else if (stats.isFile()) {
+        } else if (stats.isFile() || stats.isSymbolicLink()) {
           // When we match a file to the glob, we map it from the source path to just its basename under the target
           // directory. Again, this matches rsync behavior.
           mappedPaths.push([stats.path, basename(stats.path)])
@@ -482,10 +500,10 @@ export class FileStatsHelper {
       }
 
       // Resolve the symlink path
-      const targetPath = resolve(parse(path).dir, target)
+      const absoluteTarget = resolve(parse(path).dir, target)
 
       // Stat the final path and return
-      this.lstat(targetPath, (statErr, toStats) => {
+      this.lstat(absoluteTarget, (statErr, toStats) => {
         if (statErr?.code === "ENOENT") {
           // The symlink target does not exist. That's not an error.
           cb({ err: null, target: null, targetPath: target })
@@ -494,12 +512,12 @@ export class FileStatsHelper {
           cb({ err: statErr, target: null, targetPath: null })
         } else if (toStats.isSymbolicLink()) {
           // Keep resolving until we get to a real path
-          if (_resolvedPaths.includes(targetPath)) {
+          if (_resolvedPaths.includes(absoluteTarget)) {
             // We've gone into a symlink loop, so we ignore it
             cb({ err: null, target: null, targetPath: target })
           } else {
             this.resolveSymlink(
-              { path: targetPath, allowAbsolute, _resolvedPaths: [..._resolvedPaths, targetPath] },
+              { path: absoluteTarget, allowAbsolute, _resolvedPaths: [..._resolvedPaths, absoluteTarget] },
               ({ err: innerResolveErr, target: innerStats, targetPath: _innerTarget }) => {
                 if (innerResolveErr) {
                   cb({ err: innerResolveErr, target: null, targetPath: null })
@@ -515,7 +533,7 @@ export class FileStatsHelper {
           // Return with path and stats for final symlink target
           cb({
             err: null,
-            target: ExtendedStats.fromStats({ stats: toStats, path: targetPath }),
+            target: ExtendedStats.fromStats({ stats: toStats, path: absoluteTarget }),
             targetPath: target,
           })
         }

core/src/vcs/git-sub-tree.ts

@@ -9,7 +9,7 @@
 import { Profile } from "../util/profiling.js"
 import { getStatsType, matchPath } from "../util/fs.js"
 import { isErrnoException } from "../exceptions.js"
-import { isAbsolute, join, normalize, relative, resolve } from "path"
+import { dirname, isAbsolute, join, normalize, relative, resolve } from "path"
 import fsExtra from "fs-extra"
 import PQueue from "p-queue"
 import { defer } from "../util/util.js"
@@ -31,7 +31,7 @@ import type { Log } from "../logger/log-entry.js"
 import dedent from "dedent"
 import { gardenEnv } from "../constants.js"
 
-const { lstat, pathExists, readlink, realpath, stat } = fsExtra
+const { pathExists, readlink, lstat } = fsExtra
 
 const submoduleErrorSuggestion = `Perhaps you need to run ${styles.underline(`git submodule update --recursive`)}?`
 
@@ -221,7 +221,7 @@ export class GitSubTreeHandler extends AbstractGitHandler {
 
         // Catch and show helpful message in case the submodule path isn't a valid directory
         try {
-          const pathStats = await stat(path)
+          const pathStats = await lstat(path)
 
           if (!pathStats.isDirectory()) {
             const pathType = getStatsType(pathStats)
@@ -315,30 +315,14 @@ export class GitSubTreeHandler extends AbstractGitHandler {
           if (isAbsolute(target)) {
             gitLog.verbose(`Ignoring symlink with absolute target at ${resolvedPath}`)
             return
-          } else if (target.startsWith("..")) {
-            try {
-              const realTarget = await realpath(resolvedPath)
-              const relPath = relative(path, realTarget)
-
-              if (relPath.startsWith("..")) {
-                gitLog.verbose(`Ignoring symlink pointing outside of ${pathDescription} at ${resolvedPath}`)
-                return
-              }
-              return ensureHash({
-                file: output,
-                stats,
-                modifiedFiles,
-                hashUntrackedFiles,
-                untrackedHashedFilesCollector,
-              })
-            } catch (err) {
-              if (isErrnoException(err) && err.code === "ENOENT") {
-                gitLog.verbose(`Ignoring dead symlink at ${resolvedPath}`)
-                return
-              }
-              throw err
-            }
           } else {
+            const realTarget = resolve(dirname(resolvedPath), target)
+            const relPath = relative(path, realTarget)
+
+            if (relPath.startsWith("..")) {
+              gitLog.verbose(`Ignoring symlink pointing outside of ${pathDescription} at ${resolvedPath}`)
+              return
+            }
             return ensureHash({
               file: output,
               stats,
@@ -458,7 +442,7 @@ export class GitSubTreeHandler extends AbstractGitHandler {
 
 async function isDirectory(path: string, gitLog: Log): Promise<boolean> {
   try {
-    const pathStats = await stat(path)
+    const pathStats = await lstat(path)
 
     if (!pathStats.isDirectory()) {
       gitLog.warn(`Expected directory at ${path}, but found ${getStatsType(pathStats)}.`)

core/src/vcs/git.ts

@@ -23,7 +23,7 @@ import AsyncLock from "async-lock"
 import { isSha1 } from "../util/hashing.js"
 import { hashingStream } from "hasha"
 
-const { createReadStream, ensureDir, pathExists, readlink, stat } = fsExtra
+const { createReadStream, ensureDir, pathExists, readlink, lstat } = fsExtra
 
 export function getCommitIdFromRefList(refList: string[]): string {
   try {
@@ -393,7 +393,7 @@ export async function augmentGlobs(basePath: string, globs?: string[]): Promise<
 
       try {
         const path = joinWithPosix(basePath, pattern)
-        const stats = await stat(path)
+        const stats = await lstat(path)
         return stats.isDirectory() ? posix.join(pattern, "**", "*") : pattern
       } catch {
         return pattern

core/test/unit/src/build-staging/build-staging.ts

@@ -24,6 +24,7 @@ import { BuildTask } from "../../../../src/tasks/build.js"
 import type { ConfigGraph } from "../../../../src/graph/config-graph.js"
 import type { BuildAction } from "../../../../src/actions/build.js"
 import { DOCS_BASE_URL } from "../../../../src/constants.js"
+import { lstat, readlink, rm, symlink } from "fs/promises"
 
 // TODO-G2: rename test cases to match the new graph model semantics
 
@@ -66,9 +67,26 @@ async function assertIdentical(sourceRoot: string, targetRoot: string, posixPath
     posixPaths.map(async (path) => {
       const sourcePath = joinWithPosix(sourceRoot, path)
       const targetPath = joinWithPosix(targetRoot, path)
-      const sourceData = (await readFile(sourcePath)).toString()
-      const targetData = (await readFile(targetPath)).toString()
-      expect(sourceData).to.equal(targetData)
+
+      const sourceStat = await lstat(sourcePath)
+      const targetStat = await lstat(targetPath)
+
+      expect(sourceStat.isFile()).to.equal(targetStat.isFile(), `${targetStat} unexpected isFile()`)
+      expect(sourceStat.isDirectory()).to.equal(targetStat.isDirectory(), `${targetStat} unexpected isDirectory()`)
+      expect(sourceStat.isSymbolicLink()).to.equal(
+        targetStat.isSymbolicLink(),
+        `${targetStat} unexpected isSymbolicLink()`
+      )
+
+      if (sourceStat.isSymbolicLink() && targetStat.isSymbolicLink()) {
+        expect(await readlink(sourcePath)).to.equal(await readlink(targetPath), `${targetStat} unexpected readlink()`)
+      }
+
+      if (sourceStat.isFile() && targetStat.isFile()) {
+        const sourceData = (await readFile(sourcePath)).toString()
+        const targetData = (await readFile(targetPath)).toString()
+        expect(sourceData).to.equal(targetData, `${targetStat} unexpected readFile()`)
+      }
     })
   )
 }
@@ -122,6 +140,77 @@ describe("BuildStaging", () => {
       expect(await listFiles(targetRoot)).to.eql(["a", "subdir/c"])
     })
 
+    it("reproduces relative symlinks", async () => {
+      const sourceRoot = join(tmpPath, "source")
+
+      // prepare source
+      const files = ["node_modules/foo/bar", "node_modules/bar/baz"]
+      await ensureDir(sourceRoot)
+      await populateDirectory(sourceRoot, files)
+      await ensureDir(join(sourceRoot, "node_modules/.bin"))
+      const barLinkPath = "node_modules/.bin/bar"
+      const bazLinkPath = "node_modules/.bin/baz"
+      const nonExistentLinkPath = "node_modules/.bin/doesnotexist"
+      await symlink("../foo/bar", join(sourceRoot, barLinkPath))
+      await symlink("../bar/baz", join(sourceRoot, bazLinkPath))
+      await symlink("../this/does/not/exist", join(sourceRoot, nonExistentLinkPath))
+
+      // all the files in source
+      const expectedFiles = [...files, barLinkPath, bazLinkPath, nonExistentLinkPath]
+
+      // sync withDelete = true
+      let targetRoot = join(tmpPath, "target1")
+      await ensureDir(targetRoot)
+      // first time
+      await sync({ log, sourceRoot, targetRoot, withDelete: true })
+      await assertIdentical(sourceRoot, targetRoot, expectedFiles)
+      // second time
+      await sync({ log, sourceRoot, targetRoot, withDelete: true })
+      await assertIdentical(sourceRoot, targetRoot, expectedFiles)
+
+      // sync withDelete = false
+      targetRoot = join(tmpPath, "target2")
+      await ensureDir(targetRoot)
+      // first time
+      await sync({ log, sourceRoot, targetRoot, withDelete: false })
+      await assertIdentical(sourceRoot, targetRoot, expectedFiles)
+      // second time
+      await sync({ log, sourceRoot, targetRoot, withDelete: false })
+      await assertIdentical(sourceRoot, targetRoot, expectedFiles)
+
+      // sync with pattern
+      targetRoot = join(tmpPath, "target3")
+      await ensureDir(targetRoot)
+      // first time
+      await sync({ log, sourceRoot, sourceRelPath: "*", targetRoot, withDelete: false })
+      await assertIdentical(sourceRoot, targetRoot, expectedFiles)
+      // second time
+      await sync({ log, sourceRoot, sourceRelPath: "*", targetRoot, withDelete: false })
+      await assertIdentical(sourceRoot, targetRoot, expectedFiles)
+    })
+
+    it("should allow type changes between symlink and file", async () => {
+      const sourceRoot = join(tmpPath, "source")
+      const targetRoot = join(tmpPath, "target")
+      const file = "foo"
+
+      // scenario 1: foo is a file
+      await ensureDir(sourceRoot)
+      await populateDirectory(sourceRoot, [file])
+
+      // sync first time
+      await sync({ log, sourceRoot, targetRoot, withDelete: false })
+      await assertIdentical(sourceRoot, targetRoot, [file])
+
+      // scenario 2: foo is a symlink (target does not matter)
+      await rm(join(sourceRoot, file))
+      await symlink("targetDoesNotMatter", join(sourceRoot, file))
+
+      // the target now must be replaced with a symlink
+      await sync({ log, sourceRoot, targetRoot, withDelete: false })
+      await assertIdentical(sourceRoot, targetRoot, [file])
+    })
+
     it("throws if source relative path is absolute", async () => {
       await expectError(
         () => sync({ log, sourceRoot: tmpPath, targetRoot: tmpPath, sourceRelPath: "/foo", withDelete: false }),