diff --git a/core/src/build-staging/helpers.ts b/core/src/build-staging/helpers.ts index 6e5ce24133..40f7ef86e7 100644 --- a/core/src/build-staging/helpers.ts +++ b/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, }) } diff --git a/core/src/vcs/git-sub-tree.ts b/core/src/vcs/git-sub-tree.ts index 5d73dae21e..4697e8c1bd 100644 --- a/core/src/vcs/git-sub-tree.ts +++ b/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 { try { - const pathStats = await stat(path) + const pathStats = await lstat(path) if (!pathStats.isDirectory()) { gitLog.warn(`Expected directory at ${path}, but found ${getStatsType(pathStats)}.`) diff --git a/core/src/vcs/git.ts b/core/src/vcs/git.ts index 2eb7068752..0f6b5cee71 100644 --- a/core/src/vcs/git.ts +++ b/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 diff --git a/core/test/unit/src/build-staging/build-staging.ts b/core/test/unit/src/build-staging/build-staging.ts index b18b69a45c..c82ee2a94a 100644 --- a/core/test/unit/src/build-staging/build-staging.ts +++ b/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 }), diff --git a/core/test/unit/src/vcs/git.ts b/core/test/unit/src/vcs/git.ts index ba71b4e8b4..66ef44b90e 100644 --- a/core/test/unit/src/vcs/git.ts +++ b/core/test/unit/src/vcs/git.ts @@ -831,6 +831,24 @@ const commonGitHandlerTests = (gitScanMode: GitScanMode) => { expect(files.sort()).to.eql([filePath, symlinkPath]) }) + it("should include a relative symlink within the path even when target does not exist", async () => { + const target = "does-not-exist" + const symlinkPath = resolve(tmpPath, "symlink") + + await symlink(target, symlinkPath) + + const files = ( + await handler.getFiles({ + path: tmpPath, + scanRoot: undefined, + exclude: [], + log, + }) + ).map((f) => f.path) + + expect(files.sort()).to.eql([symlinkPath]) + }) + it("should exclude a relative symlink that points outside repo root", async () => { const subPath = resolve(tmpPath, "subdir") await mkdirp(subPath) @@ -856,6 +874,31 @@ const commonGitHandlerTests = (gitScanMode: GitScanMode) => { expect(files).to.eql([]) }) + it("should exclude a relative symlink that points outside repo root even if it does not start with ..", async () => { + const subPath = resolve(tmpPath, "subdir") + await mkdirp(subPath) + + const _git = new GitCli({ log, cwd: subPath }) + await _git.exec("init") + + const fileName = "foo" + const filePath = resolve(tmpPath, fileName) + const symlinkPath = resolve(subPath, "symlink") + + await createFile(filePath) + await ensureSymlink(join("hello", "..", "..", fileName), symlinkPath) + + const files = ( + await handler.getFiles({ + path: subPath, + scanRoot: undefined, + exclude: [], + log, + }) + ).map((f) => f.path) + expect(files).to.eql([]) + }) + it("should exclude an absolute symlink that points inside the path", async () => { const fileName = "foo" const filePath = resolve(tmpPath, fileName)