From 45b54e850def57e64cbd723ba744d92ee2e5c19d Mon Sep 17 00:00:00 2001 From: Nicolas Meienberger Date: Tue, 30 Jun 2026 16:04:38 +0200 Subject: [PATCH] fix: harden volume browser path codec Volume browser paths are API paths, not native filesystem paths. They need stable slash-separated encoding so paths containing spaces, backslashes, or percent-encoded characters can round-trip between the browser and the agent without escaping the mounted volume root. This adds a small codec at the volume-host boundary, verifies symlink/root containment on the resolved native path, and keeps the local file browser from normalizing user-facing request. --- .../__tests__/local-file-browser.test.tsx | 54 +++++++++ .../__tests__/volume-file-browser.test.tsx | 101 ++++++++++++++++ .../file-browsers/local-file-browser.tsx | 5 +- .../file-browsers/volume-file-browser.tsx | 11 ++ .../src/commands/__tests__/volume.test.ts | 6 +- .../helpers/__tests__/backup.helpers.test.ts | 8 ++ .../volume-host/__tests__/operations.test.ts | 44 ++++++- apps/agent/src/volume-host/operations.ts | 112 ++++++++++++++---- 8 files changed, 311 insertions(+), 30 deletions(-) create mode 100644 app/client/components/file-browsers/__tests__/local-file-browser.test.tsx create mode 100644 app/client/components/file-browsers/__tests__/volume-file-browser.test.tsx diff --git a/app/client/components/file-browsers/__tests__/local-file-browser.test.tsx b/app/client/components/file-browsers/__tests__/local-file-browser.test.tsx new file mode 100644 index 000000000..978abad51 --- /dev/null +++ b/app/client/components/file-browsers/__tests__/local-file-browser.test.tsx @@ -0,0 +1,54 @@ +import { afterEach, describe, expect, test } from "vitest"; +import { HttpResponse, http, server } from "~/test/msw/server"; +import { cleanup, render, waitFor } from "~/test/test-utils"; +import { LocalFileBrowser } from "../local-file-browser"; + +afterEach(() => { + cleanup(); +}); + +describe("LocalFileBrowser", () => { + test("keeps Windows drive roots as host paths", async () => { + const requests: string[] = []; + + server.use( + http.get("/api/v1/volumes/filesystem/browse", ({ request }) => { + const url = new URL(request.url); + requests.push(url.searchParams.get("path") ?? ""); + + return HttpResponse.json({ + directories: [], + path: "C:\\", + }); + }), + ); + + render(); + + await waitFor(() => { + expect(requests).toEqual(["C:\\"]); + }); + }); + + test("uses the trimmed initial path for the first browse request", async () => { + const requests: string[] = []; + + server.use( + http.get("/api/v1/volumes/filesystem/browse", ({ request }) => { + const url = new URL(request.url); + requests.push(url.searchParams.get("path") ?? ""); + + return HttpResponse.json({ + directories: [], + path: "/tmp", + }); + }), + ); + + render(); + + await waitFor(() => { + expect(requests).toEqual(["/tmp"]); + }); + }); +}); diff --git a/app/client/components/file-browsers/__tests__/volume-file-browser.test.tsx b/app/client/components/file-browsers/__tests__/volume-file-browser.test.tsx new file mode 100644 index 000000000..5350b109d --- /dev/null +++ b/app/client/components/file-browsers/__tests__/volume-file-browser.test.tsx @@ -0,0 +1,101 @@ +import type { ComponentProps } from "react"; +import { afterEach, describe, expect, test } from "vitest"; +import { HttpResponse, http, server } from "~/test/msw/server"; +import { cleanup, fireEvent, render, screen, userEvent, waitFor, within } from "~/test/test-utils"; +import { VolumeFileBrowser } from "../volume-file-browser"; + +type VolumeFilesRequest = { + shortId: string; + path: string | null; + offset: string | null; +}; + +const renderVolumeFileBrowser = (props: Partial> = {}) => { + return render(); +}; + +afterEach(() => { + cleanup(); +}); + +describe("VolumeFileBrowser", () => { + test("returns literal selected paths when the volume API uses encoded navigation paths", async () => { + server.use( + http.get("/api/v1/volumes/:shortId/files", () => { + return HttpResponse.json({ + files: [{ name: "movies [1]", path: "/movies%20%5B1%5D", type: "directory" }], + path: "/", + offset: 0, + limit: 500, + total: 1, + hasMore: false, + }); + }), + ); + + let selectedPaths: Set | undefined; + renderVolumeFileBrowser({ + withCheckboxes: true, + onSelectionChange: (paths) => { + selectedPaths = paths; + }, + }); + + const row = await screen.findByRole("button", { name: "movies [1]" }); + await userEvent.click(within(row).getByRole("checkbox")); + + expect(selectedPaths ? Array.from(selectedPaths) : []).toEqual(["/movies [1]"]); + }); + + test("uses encoded paths when fetching an expanded folder's children", async () => { + const requests: VolumeFilesRequest[] = []; + + server.use( + http.get("/api/v1/volumes/:shortId/files", ({ params, request }) => { + const url = new URL(request.url); + const requestPath = url.searchParams.get("path"); + requests.push({ + shortId: String(params.shortId), + path: requestPath, + offset: url.searchParams.get("offset"), + }); + + if (requestPath === "/movies%20%5B1%5D") { + return HttpResponse.json({ + files: [{ name: "clip.txt", path: "/movies%20%5B1%5D/clip.txt", type: "file" }], + path: "/movies%20%5B1%5D", + offset: 0, + limit: 500, + total: 1, + hasMore: false, + }); + } + + return HttpResponse.json({ + files: [{ name: "movies [1]", path: "/movies%20%5B1%5D", type: "directory" }], + path: "/", + offset: 0, + limit: 500, + total: 1, + hasMore: false, + }); + }), + ); + + renderVolumeFileBrowser(); + + const row = await screen.findByRole("button", { name: "movies [1]" }); + const expandIcon = row.querySelector("svg"); + if (!expandIcon) { + throw new Error("Expected expand icon for folder row"); + } + fireEvent.click(expandIcon); + + await waitFor(() => { + expect(requests).toEqual([ + { shortId: "volume-1", path: null, offset: null }, + { shortId: "volume-1", path: "/movies%20%5B1%5D", offset: null }, + ]); + }); + }); +}); diff --git a/app/client/components/file-browsers/local-file-browser.tsx b/app/client/components/file-browsers/local-file-browser.tsx index 69e8d010c..2a9f231b4 100644 --- a/app/client/components/file-browsers/local-file-browser.tsx +++ b/app/client/components/file-browsers/local-file-browser.tsx @@ -3,7 +3,6 @@ import { browseFilesystemOptions } from "~/client/api-client/@tanstack/react-que import { FileBrowser, type FileBrowserUiProps } from "~/client/components/file-browsers/file-browser"; import { useFileBrowser } from "~/client/hooks/use-file-browser"; import { parseError } from "~/client/lib/errors"; -import { normalizeAbsolutePath } from "@zerobyte/core/utils"; import { logger } from "~/client/lib/logger"; type LocalFileBrowserProps = FileBrowserUiProps & { @@ -13,10 +12,10 @@ type LocalFileBrowserProps = FileBrowserUiProps & { export const LocalFileBrowser = ({ initialPath = "/", enabled = true, ...uiProps }: LocalFileBrowserProps) => { const queryClient = useQueryClient(); - const normalizedInitialPath = normalizeAbsolutePath(initialPath); + const browsePath = initialPath.trim() || "/"; const { data, isLoading, error } = useQuery({ - ...browseFilesystemOptions({ query: { path: normalizedInitialPath } }), + ...browseFilesystemOptions({ query: { path: browsePath } }), enabled, }); diff --git a/app/client/components/file-browsers/volume-file-browser.tsx b/app/client/components/file-browsers/volume-file-browser.tsx index 347da3efd..646843d95 100644 --- a/app/client/components/file-browsers/volume-file-browser.tsx +++ b/app/client/components/file-browsers/volume-file-browser.tsx @@ -10,6 +10,16 @@ type VolumeFileBrowserProps = FileBrowserUiProps & { enabled?: boolean; }; +const mapVolumePathSegments = (volumePath: string, transform: (segment: string) => string) => { + const segments = volumePath.split("/").filter(Boolean).map(transform); + return segments.length ? `/${segments.join("/")}` : "/"; +}; + +const volumePathTransform = { + strip: (volumePath: string) => mapVolumePathSegments(volumePath, decodeURIComponent), + add: (volumePath: string) => mapVolumePathSegments(volumePath, encodeURIComponent), +}; + export const VolumeFileBrowser = ({ volumeId, enabled = true, ...uiProps }: VolumeFileBrowserProps) => { const queryClient = useQueryClient(); @@ -39,6 +49,7 @@ export const VolumeFileBrowser = ({ volumeId, enabled = true, ...uiProps }: Volu ) .catch((e) => logger.error(e)); }, + pathTransform: volumePathTransform, }); return ( diff --git a/apps/agent/src/commands/__tests__/volume.test.ts b/apps/agent/src/commands/__tests__/volume.test.ts index 075278388..2c69b1b00 100644 --- a/apps/agent/src/commands/__tests__/volume.test.ts +++ b/apps/agent/src/commands/__tests__/volume.test.ts @@ -58,7 +58,11 @@ test("runs backend-backed volume commands on the agent host", async () => { commandId: "command-1", command: { name: "volume.mount", - volume: { id: 1, config: { backend: "directory", path: "/tmp/source" }, provisioningId: undefined }, + volume: { + id: 1, + config: { backend: "directory", path: "/tmp/source" }, + provisioningId: undefined, + }, }, }), ); diff --git a/apps/agent/src/commands/helpers/__tests__/backup.helpers.test.ts b/apps/agent/src/commands/helpers/__tests__/backup.helpers.test.ts index 7c6cce686..5db44cc7a 100644 --- a/apps/agent/src/commands/helpers/__tests__/backup.helpers.test.ts +++ b/apps/agent/src/commands/helpers/__tests__/backup.helpers.test.ts @@ -199,4 +199,12 @@ describe("backup path options", () => { expect(options.includePaths).toEqual([path.join(volumePath, "movies [1]")]); expect(options.includePatterns).toEqual([path.join(volumePath, "**/*.txt")]); }); + + test("treats selected include paths as literal path values", () => { + const volumePath = "/var/lib/zerobyte/volumes/vol123/_data"; + + const options = createOptions(createPathOptions({ includePaths: ["/movies%20%5B1%5D"] }), volumePath); + + expect(options.includePaths).toEqual([path.join(volumePath, "movies%20%5B1%5D")]); + }); }); diff --git a/apps/agent/src/volume-host/__tests__/operations.test.ts b/apps/agent/src/volume-host/__tests__/operations.test.ts index d5dd756e5..fd52ecd1c 100644 --- a/apps/agent/src/volume-host/__tests__/operations.test.ts +++ b/apps/agent/src/volume-host/__tests__/operations.test.ts @@ -53,6 +53,48 @@ test("listVolumeFiles returns sorted paginated entries inside the volume", async expect(result.files[1]).toMatchObject({ path: "/b-file.txt", type: "file", size: 5 }); }); +test("listVolumeFiles treats an empty string as the volume root", async () => { + const volume = await createDirectoryVolume(); + await fs.mkdir(path.join(tempRoot!, "logs")); + + const result = await listVolumeFiles(volume, "", 0, 10); + + expect(result.path).toBe("/"); + expect(result.files[0]).toMatchObject({ name: "logs", path: "/logs", type: "directory" }); +}); + +test("listVolumeFiles preserves literal POSIX path segment characters", async () => { + const volume = await createDirectoryVolume(); + await fs.mkdir(path.join(tempRoot!, "movies [1]")); + await fs.writeFile(path.join(tempRoot!, "movies [1]", "clip one.txt"), "hello"); + await fs.mkdir(path.join(tempRoot!, "foo\\bar")); + await fs.writeFile(path.join(tempRoot!, "foo\\bar", "nested.txt"), "hello"); + await fs.mkdir(path.join(tempRoot!, "foo%2Fbar")); + + const result = await listVolumeFiles(volume, undefined, 0, 10); + + expect(result.files).toEqual( + expect.arrayContaining([ + expect.objectContaining({ name: "movies [1]", path: "/movies%20%5B1%5D" }), + expect.objectContaining({ name: "foo\\bar", path: "/foo%5Cbar" }), + expect.objectContaining({ name: "foo%2Fbar", path: "/foo%252Fbar" }), + ]), + ); + + const spacedNested = await listVolumeFiles(volume, "/movies%20%5B1%5D", 0, 10); + + expect(spacedNested.path).toBe("/movies%20%5B1%5D"); + expect(spacedNested.files[0]).toMatchObject({ + name: "clip one.txt", + path: "/movies%20%5B1%5D/clip%20one.txt", + }); + + const nested = await listVolumeFiles(volume, "/foo%5Cbar", 0, 10); + + expect(nested.path).toBe("/foo%5Cbar"); + expect(nested.files[0]).toMatchObject({ name: "nested.txt", path: "/foo%5Cbar/nested.txt" }); +}); + test("listVolumeFiles rejects traversal outside the volume", async () => { const volume = await createDirectoryVolume(); @@ -62,5 +104,5 @@ test("listVolumeFiles rejects traversal outside the volume", async () => { test("listVolumeFiles reports missing directories consistently", async () => { const volume = await createDirectoryVolume(); - await expect(listVolumeFiles(volume, "missing", 0, 10)).rejects.toThrow("Directory not found"); + await expect(listVolumeFiles(volume, "/missing", 0, 10)).rejects.toThrow("Directory not found"); }); diff --git a/apps/agent/src/volume-host/operations.ts b/apps/agent/src/volume-host/operations.ts index 80c661894..7be4ebaf7 100644 --- a/apps/agent/src/volume-host/operations.ts +++ b/apps/agent/src/volume-host/operations.ts @@ -9,6 +9,87 @@ import { createVolumeBackend, getVolumePath, isNodeJSErrnoException } from "."; const DEFAULT_PAGE_SIZE = 500; const MAX_PAGE_SIZE = 500; +type VolumePathResolution = { + requestPath: string; + nativePath: string; + relativeRoot: string; +}; + +const encodeVolumePath = (segments: string[]) => { + return segments.length ? `/${segments.map(encodeURIComponent).join("/")}` : "/"; +}; + +const parseVolumeRequestPath = (value: string) => { + if (!value.startsWith("/")) { + throw new Error("Invalid path"); + } + + const segments: string[] = []; + for (const segment of value.split("/")) { + if (!segment || segment === ".") continue; + if (segment === "..") { + segments.pop(); + continue; + } + + try { + segments.push(decodeURIComponent(segment)); + } catch { + throw new Error("Invalid path"); + } + } + + return segments; +}; + +const assertPathIsWithinRoot = (rootPath: string, targetPath: string) => { + const relativePath = path.relative(rootPath, targetPath); + + if (relativePath === ".." || relativePath.startsWith(`..${path.sep}`) || path.isAbsolute(relativePath)) { + throw new Error("Invalid path"); + } + + return relativePath; +}; + +const toVolumePath = (nativeRelativePath: string) => { + const segments = nativeRelativePath.split(path.sep).filter(Boolean).map(encodeURIComponent); + return segments.length ? `/${segments.join("/")}` : "/"; +}; + +const assertPathHasNoSymlinkSegments = async (rootPath: string, relativePath: string) => { + let currentPath = rootPath; + for (const segment of relativePath.split(path.sep).filter(Boolean)) { + currentPath = path.join(currentPath, segment); + const stats = await fs.lstat(currentPath); + if (stats.isSymbolicLink()) { + throw new Error("Invalid path"); + } + } +}; + +const resolveVolumeRequestPath = async (volumePath: string, subPath?: string): Promise => { + const segments = parseVolumeRequestPath(subPath?.length ? subPath : "/"); + const requestPath = encodeVolumePath(segments); + const nativePath = path.normalize(path.join(volumePath, ...segments)); + const requestedRelativePath = assertPathIsWithinRoot(volumePath, nativePath); + const realVolumeRoot = await fs.realpath(volumePath); + + try { + const realRequestedPath = await fs.realpath(nativePath); + assertPathIsWithinRoot(realVolumeRoot, realRequestedPath); + return { requestPath, nativePath: realRequestedPath, relativeRoot: realVolumeRoot }; + } catch (error) { + if (!isNodeJSErrnoException(error) || error.code !== "ENOENT") { + throw error; + } + + await fs.lstat(nativePath); + await assertPathHasNoSymlinkSegments(volumePath, requestedRelativePath); + return { requestPath, nativePath, relativeRoot: volumePath }; + } +}; + export const listVolumeFiles = async ( volume: AgentVolume, subPath?: string, @@ -16,31 +97,12 @@ export const listVolumeFiles = async ( limit: number = DEFAULT_PAGE_SIZE, ) => { const volumePath = getVolumePath(volume); - const requestedPath = subPath ? path.join(volumePath, subPath) : volumePath; - const normalizedPath = path.normalize(requestedPath); - const requestedRelativePath = path.relative(volumePath, normalizedPath); - - if ( - requestedRelativePath === ".." || - requestedRelativePath.startsWith(`..${path.sep}`) || - path.isAbsolute(requestedRelativePath) - ) { - throw new Error("Invalid path"); - } - const pageSize = Math.min(Math.max(limit, 1), MAX_PAGE_SIZE); const startOffset = Math.max(offset, 0); try { - const realVolumeRoot = await fs.realpath(volumePath); - const realRequestedPath = await fs.realpath(requestedPath); - const relative = path.relative(realVolumeRoot, realRequestedPath); - - if (relative === ".." || relative.startsWith(`..${path.sep}`) || path.isAbsolute(relative)) { - throw new Error("Invalid path"); - } - - const dirents = await fs.readdir(realRequestedPath, { withFileTypes: true }); + const resolvedPath = await resolveVolumeRequestPath(volumePath, subPath); + const dirents = await fs.readdir(resolvedPath.nativePath, { withFileTypes: true }); dirents.sort((a, b) => { const aIsDir = a.isDirectory(); @@ -58,15 +120,15 @@ export const listVolumeFiles = async ( const entries = ( await Promise.all( paginatedDirents.map(async (dirent) => { - const fullPath = path.join(realRequestedPath, dirent.name); + const fullPath = path.join(resolvedPath.nativePath, dirent.name); try { const stats = await fs.stat(fullPath); - const relativePath = path.relative(realVolumeRoot, fullPath); + const relativePath = path.relative(resolvedPath.relativeRoot, fullPath); return { name: dirent.name, - path: `/${relativePath}`, + path: toVolumePath(relativePath), type: dirent.isDirectory() ? ("directory" as const) : ("file" as const), size: dirent.isFile() ? stats.size : undefined, modifiedAt: stats.mtimeMs, @@ -80,7 +142,7 @@ export const listVolumeFiles = async ( return { files: entries, - path: subPath || "/", + path: resolvedPath.requestPath, offset: startOffset, limit: pageSize, total,