From c4f3300e9b445f16dff95ea97fa88896e3cb4a9a Mon Sep 17 00:00:00 2001 From: Nico <47644445+nicotsx@users.noreply.github.com> Date: Wed, 25 Feb 2026 19:09:32 +0100 Subject: [PATCH] fix(backups): correctly resolve paths when folder name matches volume name (#576) When a selected subfolder had the exact same name as its parent volume mount, the `path.relative` check falsely identified the subfolder path as an absolute path already inside the volume. This caused the entire volume root to be backed up instead of the intended subfolder. Closes #250 ## Summary by CodeRabbit * **Bug Fixes** * Improved handling of backup patterns when folder names match volume names. * Enhanced error handling for snapshot deletion operations. --- .../__tests__/backups.patterns.test.ts | 24 ++++++----- app/server/modules/backups/backup.helpers.ts | 5 +-- .../__tests__/repositories.controller.test.ts | 29 ++++++++++++- .../__tests__/repositories.service.test.ts | 41 +++++++++++++++++++ 4 files changed, 83 insertions(+), 16 deletions(-) diff --git a/app/server/modules/backups/__tests__/backups.patterns.test.ts b/app/server/modules/backups/__tests__/backups.patterns.test.ts index 2e26a990..9dee49de 100644 --- a/app/server/modules/backups/__tests__/backups.patterns.test.ts +++ b/app/server/modules/backups/__tests__/backups.patterns.test.ts @@ -53,20 +53,23 @@ describe("executeBackup - include / exclude patterns", () => { ); }); - test("should not join with volume path if pattern already starts with it", async () => { + test("should handle the case where a subfolder has the exact same name as the volume name", async () => { // arrange - const volume = await createTestVolume(); + const volumeName = "SyncFolder"; + const volume = await createTestVolume({ + name: volumeName, + type: "directory", + config: { backend: "directory", path: `/${volumeName}` }, + }); const volumePath = getVolumePath(volume); const repository = await createTestRepository(); - const alreadyJoinedInclude = path.join(volumePath, "already/joined"); - const alreadyJoinedExclude = path.join(volumePath, "already/excluded"); + const selectedPath = `/${volumeName}`; // Selection of the folder inside the volume const schedule = await createTestBackupSchedule({ volumeId: volume.id, repositoryId: repository.id, - includePatterns: [alreadyJoinedInclude], - excludePatterns: [alreadyJoinedExclude], + includePatterns: [selectedPath], }); // act @@ -77,8 +80,8 @@ describe("executeBackup - include / exclude patterns", () => { expect.anything(), volumePath, expect.objectContaining({ - include: [alreadyJoinedInclude], - exclude: [alreadyJoinedExclude], + // Should produce /SyncFolder/SyncFolder and not just /SyncFolder + include: [path.join(volumePath, volumeName)], }), ); }); @@ -89,14 +92,13 @@ describe("executeBackup - include / exclude patterns", () => { const volumePath = getVolumePath(volume); const repository = await createTestRepository(); - const alreadyJoinedInclude = path.join(volumePath, "already/joined"); const relativeInclude = "relative/include"; const anchoredInclude = "/anchored/include"; const schedule = await createTestBackupSchedule({ volumeId: volume.id, repositoryId: repository.id, - includePatterns: [alreadyJoinedInclude, relativeInclude, anchoredInclude], + includePatterns: [relativeInclude, anchoredInclude], }); // act @@ -107,7 +109,7 @@ describe("executeBackup - include / exclude patterns", () => { expect.anything(), volumePath, expect.objectContaining({ - include: [alreadyJoinedInclude, relativeInclude, path.join(volumePath, "anchored/include")], + include: [relativeInclude, path.join(volumePath, "anchored/include")], }), ); }); diff --git a/app/server/modules/backups/backup.helpers.ts b/app/server/modules/backups/backup.helpers.ts index ed3d5ca5..ab5b3d7d 100644 --- a/app/server/modules/backups/backup.helpers.ts +++ b/app/server/modules/backups/backup.helpers.ts @@ -23,10 +23,7 @@ export const processPattern = (pattern: string, volumePath: string) => { const isNegated = pattern.startsWith("!"); const p = isNegated ? pattern.slice(1) : pattern; - const relative = path.relative(volumePath, p); - const isInsideVolume = !relative.startsWith("..") && !path.isAbsolute(relative); - - if (isInsideVolume || !p.startsWith("/")) { + if (!p.startsWith("/")) { return pattern; } diff --git a/app/server/modules/repositories/__tests__/repositories.controller.test.ts b/app/server/modules/repositories/__tests__/repositories.controller.test.ts index 40c6c706..94210d52 100644 --- a/app/server/modules/repositories/__tests__/repositories.controller.test.ts +++ b/app/server/modules/repositories/__tests__/repositories.controller.test.ts @@ -1,4 +1,4 @@ -import { test, describe, expect } from "bun:test"; +import { test, describe, expect, spyOn } from "bun:test"; import crypto from "node:crypto"; import { createApp } from "~/server/app"; import { db } from "~/server/db/db"; @@ -240,4 +240,31 @@ describe("repositories updates", () => { expect(res.status).toBe(400); }); + + describe("delete snapshot", () => { + test("should return 500 when restic deleteSnapshot throws ResticError", async () => { + const { token, organizationId } = await createTestSession(); + const repository = await createRepositoryRecord(organizationId); + + const { restic } = await import("~/server/utils/restic"); + const { ResticError } = await import("~/server/utils/errors"); + + const deleteSnapshotSpy = spyOn(restic, "deleteSnapshot").mockImplementation(async () => { + throw new ResticError(1, "Fatal: unexpected HTTP response (403): 403 Forbidden"); + }); + + try { + const res = await app.request(`/api/v1/repositories/${repository.shortId}/snapshots/snap123`, { + method: "DELETE", + headers: getAuthHeaders(token), + }); + + expect(res.status).toBe(500); + const body = await res.json(); + expect(body.message).toContain("Command failed"); + } finally { + deleteSnapshotSpy.mockRestore(); + } + }); + }); }); diff --git a/app/server/modules/repositories/__tests__/repositories.service.test.ts b/app/server/modules/repositories/__tests__/repositories.service.test.ts index 730372e7..ef1e3c0e 100644 --- a/app/server/modules/repositories/__tests__/repositories.service.test.ts +++ b/app/server/modules/repositories/__tests__/repositories.service.test.ts @@ -12,8 +12,28 @@ import { restic } from "~/server/utils/restic"; import { createTestSession } from "~/test/helpers/auth"; import { createTestBackupSchedule } from "~/test/helpers/backup"; import { cache, cacheKeys } from "~/server/utils/cache"; +import { ResticError } from "~/server/utils/errors"; import { repositoriesService } from "../repositories.service"; +const createTestRepository = async (organizationId: string) => { + const id = randomUUID(); + const shortId = generateShortId(); + const [repository] = await db + .insert(repositoriesTable) + .values({ + id, + shortId, + name: `Test-${randomUUID()}`, + type: "local", + config: { backend: "local", path: "/tmp" }, + compressionMode: "auto", + status: "healthy", + organizationId, + }) + .returning(); + return repository; +}; + describe("repositoriesService.createRepository", () => { const initMock = mock(() => Promise.resolve({ success: true, error: null })); @@ -371,3 +391,24 @@ describe("repositoriesService.getRetentionCategories", () => { expect(forgetSpy).toHaveBeenCalledTimes(2); }); }); + +describe("repositoriesService.deleteSnapshot", () => { + afterEach(() => { + mock.restore(); + }); + + test("should throw original error when restic deleteSnapshot fails", async () => { + const { organizationId, user } = await createTestSession(); + const repository = await createTestRepository(organizationId); + + spyOn(restic, "deleteSnapshot").mockImplementation(async () => { + throw new ResticError(1, "Fatal: unexpected HTTP response (403): 403 Forbidden"); + }); + + await expect( + withContext({ organizationId, userId: user.id }, () => + repositoriesService.deleteSnapshot(repository.shortId, "snap123"), + ), + ).rejects.toThrow("Fatal: unexpected HTTP response"); + }); +});