From a3467939485a55c37128b98208c554324a80434b Mon Sep 17 00:00:00 2001 From: Carlos Scheidegger Date: Tue, 20 May 2025 10:42:26 -0400 Subject: [PATCH 1/4] fix bad directory relocation fallback code --- src/command/render/project.ts | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/command/render/project.ts b/src/command/render/project.ts index 65e66820058..1674883aeec 100644 --- a/src/command/render/project.ts +++ b/src/command/render/project.ts @@ -513,7 +513,7 @@ export async function renderProject( // because src and target are in different file systems. // In that case, try to recursively copy from src copyTo(srcDir, targetDir); - safeRemoveDirSync(targetDir, context.dir); + safeRemoveDirSync(srcDir, context.dir); } } }; @@ -632,13 +632,11 @@ export async function renderProject( const sortedOperations = uniqOps.sort((a, b) => { if (a.src === b.src) { return 0; - } else { - if (isSubdir(a.src, b.src)) { - return -1; - } else { - return a.src.localeCompare(b.src); - } } + if (isSubdir(a.src, b.src)) { + return -1; + } + return a.src.localeCompare(b.src); }); // Before file move From 89c87decd748a60082b7b66536ab741ab20304ed Mon Sep 17 00:00:00 2001 From: Carlos Scheidegger Date: Tue, 20 May 2025 10:58:50 -0400 Subject: [PATCH 2/4] fix safeRemoveDir boundary check --- src/deno_ral/fs.ts | 40 ++++++++++++++++---------- tests/unit/ral/safe-remove-dir.test.ts | 32 +++++++++++++++++++++ 2 files changed, 57 insertions(+), 15 deletions(-) create mode 100644 tests/unit/ral/safe-remove-dir.test.ts diff --git a/src/deno_ral/fs.ts b/src/deno_ral/fs.ts index 534a418f1f5..a16f41cbf24 100644 --- a/src/deno_ral/fs.ts +++ b/src/deno_ral/fs.ts @@ -36,33 +36,44 @@ export function getFileInfoType(fileInfo: Deno.FileInfo): PathType | undefined { } // from https://jsr.io/@std/fs/1.0.3/_is_subdir.ts -// 2024-15-11: isSubDir("foo", "foo/bar") returns true, which gets src and dest exactly backwards?! /** - * Checks whether `src` is a sub-directory of `dest`. + * Checks whether `path2` is a sub-directory of `path1`. * - * @param src Source file path as a string or URL. - * @param dest Destination file path as a string or URL. + * The original function uses bad parameter names which are misleading. + * + * This function is such that, for all paths p: + * + * isSubdir(p, join(p, "foo")) === true + * isSubdir(p, p) === false + * isSubdir(join(p, "foo"), p) === false + * + * @param path1 First path, as a string or URL. + * @param path2 Second path, as a string or URL. * @param sep Path separator. Defaults to `\\` for Windows and `/` for other * platforms. * - * @returns `true` if `src` is a sub-directory of `dest`, `false` otherwise. + * @returns `true` if `path2` is a proper sub-directory of `path1`, `false` otherwise. */ export function isSubdir( - src: string | URL, - dest: string | URL, + path1: string | URL, + path2: string | URL, sep = SEPARATOR, ): boolean { - src = toPathString(src); - dest = toPathString(dest); + path1 = toPathString(path1); + path2 = toPathString(path2); - if (resolve(src) === resolve(dest)) { + if (resolve(path1) === resolve(path2)) { return false; } - const srcArray = src.split(sep); - const destArray = dest.split(sep); + const path1Array = path1.split(sep); + const path2Array = path2.split(sep); + + // if path1Array is longer than path2Array, then at least one of the + // comparisons will return false, because it will compare a string to + // undefined - return srcArray.every((current, i) => destArray[i] === current); + return path1Array.every((current, i) => path2Array[i] === current); } /** @@ -118,8 +129,7 @@ export function safeRemoveDirSync( path: string, boundary: string, ) { - // note the comment above about isSubdir getting src and dest backwards - if (path === boundary || isSubdir(path, boundary)) { + if (path === boundary || !isSubdir(boundary, path)) { throw new UnsafeRemovalError( `Refusing to remove directory ${path} that isn't a subdirectory of ${boundary}`, ); diff --git a/tests/unit/ral/safe-remove-dir.test.ts b/tests/unit/ral/safe-remove-dir.test.ts new file mode 100644 index 00000000000..984a7945825 --- /dev/null +++ b/tests/unit/ral/safe-remove-dir.test.ts @@ -0,0 +1,32 @@ +/* +* safe-remove-dir.test.ts +* +* Copyright (C) 2025 Posit Software, PBC +* +*/ + +import { unitTest } from "../../test.ts"; +import { assert, assertThrows } from "testing/asserts"; + +import { createTempContext } from "../../../src/core/temp.ts"; +import { ensureDirSync, safeRemoveDirSync } from "../../../src/deno_ral/fs.ts"; +import { join } from "../../../src/deno_ral/path.ts"; + +unitTest("safeRemoveDirSync", async () => { + + const temp = createTempContext(); + + const d1 = temp.createDir(); + const path = join(d1, "do-not-touch"); + const boundary = join(d1, "project-root"); + ensureDirSync(path); + ensureDirSync(boundary); + + assertThrows(() => { + safeRemoveDirSync(path, boundary); + }); + + assert(Deno.statSync(path).isDirectory); + + temp.cleanup(); +}); From 299cddd751cef93196db70eaa4322ec7859cc3bb Mon Sep 17 00:00:00 2001 From: Carlos Scheidegger Date: Tue, 20 May 2025 11:15:19 -0400 Subject: [PATCH 3/4] fix another bug in isSubdir --- src/deno_ral/fs.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/deno_ral/fs.ts b/src/deno_ral/fs.ts index a16f41cbf24..e48e1c68ca9 100644 --- a/src/deno_ral/fs.ts +++ b/src/deno_ral/fs.ts @@ -62,7 +62,10 @@ export function isSubdir( path1 = toPathString(path1); path2 = toPathString(path2); - if (resolve(path1) === resolve(path2)) { + path1 = resolve(path1); + path2 = resolve(path2); + + if (path1 === path2) { return false; } From 92c266cea668b972e672fa115a2945d6a49b65d8 Mon Sep 17 00:00:00 2001 From: Carlos Scheidegger Date: Tue, 20 May 2025 12:34:02 -0400 Subject: [PATCH 4/4] changelog --- news/changelog-1.8.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/news/changelog-1.8.md b/news/changelog-1.8.md index 0830f827b49..b0b5884c5f1 100644 --- a/news/changelog-1.8.md +++ b/news/changelog-1.8.md @@ -56,3 +56,7 @@ All changes included in 1.8: ### `inspect` - ([#12733](https://github.com/quarto-dev/quarto-cli/issues/12733)): Add installed extensions to `quarto inspect` project report. + +## Other fixes and improvements + +- ([#12782](https://github.com/quarto-dev/quarto-cli/pull/12782)): fix bug on `safeRemoveDirSync`'s detection of safe directory boundaries. \ No newline at end of file