From 44223389d88ccff485689f69b817cf3cb5bc174a Mon Sep 17 00:00:00 2001 From: Jarek Krochmalski Date: Sun, 6 Sep 2026 17:03:47 +0530 Subject: [PATCH] warn before saving a stack whose compose path won't survive a recreate --- src/lib/server/host-path.ts | 73 ++++++++- .../[name]/check-path-change/+server.ts | 15 +- src/routes/stacks/StackModal.svelte | 140 ++++++++++++++++-- tests/unpersisted-compose-path.test.ts | 113 ++++++++++++++ 4 files changed, 325 insertions(+), 16 deletions(-) create mode 100644 tests/unpersisted-compose-path.test.ts diff --git a/src/lib/server/host-path.ts b/src/lib/server/host-path.ts index 5bfe70d..ddbfcec 100644 --- a/src/lib/server/host-path.ts +++ b/src/lib/server/host-path.ts @@ -216,6 +216,14 @@ export function resetHostDetection(): void { cachedOwnExtraHosts = null; } +/** Test-only: seed the container-mount table so mount-dependent path translation + * (translateContainerPathViaMount / rewriteComposeVolumePaths) can be unit-tested. */ +export function __setCachedMountsForTest( + mounts: Array<{ source: string; destination: string }> | null +): void { + cachedMounts = mounts; +} + /** * Get the host path for our DATA_DIR mount by inspecting our own container */ @@ -426,6 +434,58 @@ export function getCachedContainerMounts(): Array<{ source: string; destination: return cachedMounts ? [...cachedMounts] : []; } +/** + * True when a container mount destination STRICTLY DEEPER than DATA_DIR covers + * `containerPath` (e.g. DATA_DIR=/app/data with a separate bind at /app/data/stacks). + * In that topology translateToHostPath() is WRONG - it resolves the path against + * DATA_DIR's host root (the outer volume), but the files physically live under the + * more-specific bind. translateContainerPathViaMount() has the right answer, so the + * caller must prefer it. Pure over the passed-in mount list for unit testing (#1533). + */ +export function pathOverriddenBySubMount( + containerPath: string, + dataDir: string, + mounts: Array<{ source: string; destination: string }> +): boolean { + const norm = (p: string) => p.replace(/\/+$/, ''); + const dd = norm(dataDir); + const cp = norm(containerPath); + return mounts.some((m) => { + const dest = norm(m.destination); + // Deeper than DATA_DIR... + if (!(dest.startsWith(dd + '/') && dest.length > dd.length)) return false; + // ...and it covers the path. + return cp === dest || cp.startsWith(dest + '/'); + }); +} + +/** + * Warn when a custom stack compose path won't survive a Dockhand recreate (#1524). + * A path Dockhand writes to that is NOT under any of its container mounts lands on the + * container's throwaway layer - it looks saved, then vanishes when the container is + * recreated (an update/restart), leaving the stack's compose "blank". Returns a + * human-readable warning, or null when the path is fine. + * + * Pure over the passed-in mount list so it unit-tests without a live container. Only + * fires when Dockhand is containerized (mounts non-empty) and the path is absolute and + * not under any mount destination; bare-metal (no mounts) and default in-DATA_DIR paths + * return null. + */ +export function unpersistedComposePathWarning( + composePath: string | null | undefined, + mounts: Array<{ source: string; destination: string }> +): string | null { + if (!composePath || mounts.length === 0) return null; + // Only a path Dockhand itself resolves inside its container can be orphaned; a + // relative path is resolved against DATA_DIR elsewhere, so only guard absolute paths. + if (!composePath.startsWith('/')) return null; + const underMount = [...mounts] + .sort((a, b) => b.destination.length - a.destination.length) + .some((m) => composePath === m.destination || composePath.startsWith(m.destination + '/')); + if (underMount) return null; + return `The compose file path "${composePath}" is not inside any of Dockhand's mounted volumes, so it is written inside the Dockhand container and will be LOST when the container is recreated (an update or restart). Use a path under a mounted volume (e.g. the default location, or add a bind mount for this path to Dockhand's own compose), then re-save.`; +} + /** * Get the host path for the Docker socket mount. * This is needed for sibling containers (e.g., scanners) that need socket access. @@ -529,14 +589,19 @@ export function rewriteComposeVolumePaths(composeContent: string, workingDir: st const absoluteContainerPath = resolve(workingDir, relativeSrc); const absoluteHostPath = translateContainerPathViaMount(absoluteContainerPath); - if (absoluteHostPath) { + if (absoluteHostPath && absoluteHostPath !== absoluteContainerPath) { const newLine = `${prefix}${absoluteHostPath}${destPart}`; modifiedLines.push(newLine); changes.push(` ${relativeSrc} -> ${absoluteHostPath}`); } else { - // Can't translate — leave line unchanged. Compose will resolve - // it relative to its cwd; if that's wrong the deploy fails - // loudly, which is better than producing a misleading host path. + // Either untranslatable, OR a 1:1 mount where the host path equals the + // container path (#1514): rewriting `./x` to the identical absolute path + // is pointless and forces stdin deploy (`-f -`), which loses the real + // com.docker.compose.project.config_files label. Leave the relative bind + // so the deploy can pass the compose file by path; compose resolves `./x` + // against the file's dir to the same host location. If it was simply + // untranslatable, compose still resolves it against its cwd - a wrong path + // there fails the deploy loudly, better than a misleading host path. modifiedLines.push(line); } } else { diff --git a/src/routes/api/stacks/[name]/check-path-change/+server.ts b/src/routes/api/stacks/[name]/check-path-change/+server.ts index 09cf1eb..1c37b63 100644 --- a/src/routes/api/stacks/[name]/check-path-change/+server.ts +++ b/src/routes/api/stacks/[name]/check-path-change/+server.ts @@ -3,6 +3,7 @@ import type { RequestHandler } from './$types'; import { authorize } from '$lib/server/authorize'; import { getStackSource } from '$lib/server/db'; import { findStackDir } from '$lib/server/stacks'; +import { getCachedContainerMounts, unpersistedComposePathWarning } from '$lib/server/host-path'; import { existsSync, readdirSync } from 'node:fs'; import { join, dirname } from 'node:path'; @@ -15,7 +16,7 @@ import { join, dirname } from 'node:path'; * query: env:integer Environment ID the stack belongs to (from GET /api/environments) * body: {newComposePath:string!} * body-example: {"newComposePath":"/opt/stacks/web/compose.yaml"} - * resp-200: {hasChanges:boolean!, oldDir:string, newDir:string, fileCount:integer!, currentComposePath:string} + * resp-200: {hasChanges:boolean!, oldDir:string, newDir:string, fileCount:integer!, currentComposePath:string, persistenceWarning:string} * resp-200-example: {"hasChanges":true,"oldDir":"/opt/stacks/old","newDir":"/opt/stacks/web","fileCount":3,"currentComposePath":"/opt/stacks/old/compose.yaml"} * resp-403: Permission denied (requires stacks:edit) * resp-500: Failed to check path changes @@ -25,7 +26,10 @@ export const POST: RequestHandler = async ({ params, request, url, cookies }) => const { name } = params; const envId = url.searchParams.get('env'); const envIdNum = envId ? parseInt(envId) : undefined; - if (auth.authEnabled && !(await auth.can('stacks', 'edit', envIdNum))) { + // edit OR create: this pre-flight (files-to-move + persistence warning) is used by both + // the relocate/save flows (edit) and the create flow (a not-yet-existing stack), and it + // reveals nothing beyond the caller's own proposed path + Dockhand's mount layout. + if (auth.authEnabled && !(await auth.can('stacks', 'edit', envIdNum)) && !(await auth.can('stacks', 'create', envIdNum))) { return json({ error: 'Permission denied' }, { status: 403 }); } const envAccessDenied = await auth.requireEnvAccess(envIdNum ?? null); @@ -78,12 +82,17 @@ export const POST: RequestHandler = async ({ params, request, url, cookies }) => } } + // Warn if the chosen path won't survive a Dockhand recreate because it is not + // under any of Dockhand's mounted volumes (#1524). + const persistenceWarning = unpersistedComposePathWarning(newComposePath, getCachedContainerMounts()); + return json({ hasChanges, oldDir: currentDir, newDir, fileCount, - currentComposePath + currentComposePath, + persistenceWarning }); } catch (error: any) { console.error(`Error checking path change for stack ${name}:`, error); diff --git a/src/routes/stacks/StackModal.svelte b/src/routes/stacks/StackModal.svelte index 85ba2e3..50cc1ec 100644 --- a/src/routes/stacks/StackModal.svelte +++ b/src/routes/stacks/StackModal.svelte @@ -136,6 +136,11 @@ const selectedProviderName = $derived( secretProviders.find((p) => p.id === formSecretProviderId)?.name ?? null ); + // Whether a provider is currently bound AND still exists (a deleted provider leaves + // formSecretProviderId pointing at a gone id) - drives the historical banner (#1522). + const selectedProviderBound = $derived( + formSecretProviderId != null && secretProviders.some((p) => p.id === formSecretProviderId) + ); // Live probe of the bound provider: key NAMES currently present (bulk + resolved // inline refs). Drives the editor's green IN VAULT marker. Empty when no provider // is bound or the probe failed; probeError holds the reason on failure. @@ -454,6 +459,43 @@ let changeLocationFileCount = $state(0); let changeLocationOldDir = $state(null); let movingLocation = $state(false); + // Persistence warning: the chosen compose path is not under a Dockhand mount, so it + // would vanish on a container recreate (#1524). Shown as a confirm before committing. + let showPersistenceWarn = $state(false); + let persistenceWarnText = $state(''); + let pendingHasFilesToMove = $state(false); + // Continuation run when the user accepts the persistence warning ("Use it anyway"). + // Lets create / save / relocate share one dialog: each stashes what to do next. + let persistenceWarnProceed: (() => void) | null = null; + + /** + * Pre-flight a compose path: if it is not under a Dockhand mount (would be lost on + * recreate, #1524), show the warning dialog and defer `proceed` until the user accepts; + * otherwise run `proceed` immediately. On any check failure, proceed (never block on the + * guard itself). Only meaningful for an absolute custom path. + */ + async function guardComposePersistence(composePath: string, proceed: () => void) { + const envId = $currentEnvironment?.id ?? null; + const probeName = mode === 'edit' ? stackName : (newStackName.trim() || 'new-stack'); + try { + const res = await fetch( + appendEnvParam(`/api/stacks/${encodeURIComponent(probeName)}/check-path-change`, envId), + { method: 'POST', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ newComposePath: composePath }) } + ); + if (res.ok) { + const data = await res.json(); + if (data.persistenceWarning) { + persistenceWarnText = data.persistenceWarning; + persistenceWarnProceed = proceed; + showPersistenceWarn = true; + return; + } + } + } catch (e) { + console.warn('Persistence pre-flight failed:', e); + } + proceed(); + } async function handleChangeLocation(selectedDir: string, _name: string) { showFileBrowser = false; @@ -482,13 +524,22 @@ if (response.ok) { const data = await response.json(); - if (data.hasChanges && data.oldDir && data.fileCount > 0) { - // Show confirmation dialog - pendingNewLocation = newDir; - pendingNewComposePath = newComposePath; - pendingNewEnvPath = newEnvPath; - changeLocationOldDir = data.oldDir; - changeLocationFileCount = data.fileCount; + // Stash the target so a follow-up confirm (persistence and/or move) can act on it. + pendingNewLocation = newDir; + pendingNewComposePath = newComposePath; + pendingNewEnvPath = newEnvPath; + changeLocationOldDir = data.oldDir ?? null; + changeLocationFileCount = data.fileCount ?? 0; + pendingHasFilesToMove = !!(data.hasChanges && data.oldDir && data.fileCount > 0); + + // A non-persisted path is the more serious warning - confirm it FIRST; on + // confirm we fall through to the move dialog (if any) or commit (#1524). + if (data.persistenceWarning) { + persistenceWarnText = data.persistenceWarning; + showPersistenceWarn = true; + return; + } + if (pendingHasFilesToMove) { showChangeLocationConfirm = true; return; } @@ -503,6 +554,41 @@ isDirty = true; } + // "Use it anyway". A continuation (create/save) runs first; otherwise this is the + // change-location flow, so continue to the move dialog or commit the new path. + function confirmPersistenceWarn() { + showPersistenceWarn = false; + if (persistenceWarnProceed) { + const go = persistenceWarnProceed; + persistenceWarnProceed = null; + go(); + return; + } + if (pendingHasFilesToMove) { + showChangeLocationConfirm = true; + return; + } + if (pendingNewComposePath) workingComposePath = pendingNewComposePath; + if (pendingNewEnvPath !== null) workingEnvPath = pendingNewEnvPath; + isDirty = true; + clearPendingLocation(); + } + + function cancelPersistenceWarn() { + showPersistenceWarn = false; + persistenceWarnProceed = null; + clearPendingLocation(); + } + + function clearPendingLocation() { + pendingNewLocation = null; + pendingNewComposePath = null; + pendingNewEnvPath = null; + changeLocationOldDir = null; + changeLocationFileCount = 0; + pendingHasFilesToMove = false; + } + function cancelChangeLocation() { showChangeLocationConfirm = false; pendingNewLocation = null; @@ -1247,7 +1333,7 @@ composeContent = newContent; } - async function handleCreate(start: boolean = false) { + async function handleCreate(start: boolean = false, persistenceAcked = false) { errors = {}; let hasErrors = false; @@ -1267,6 +1353,11 @@ if (hasErrors) return; + // Warn if the chosen compose path won't survive a recreate (#1524) - before creating. + if (!persistenceAcked && workingComposePath.trim()) { + return guardComposePersistence(workingComposePath.trim(), () => handleCreate(start, true)); + } + const envId = $currentEnvironment?.id ?? null; // Check if stack already exists @@ -1364,7 +1455,7 @@ } } - async function handleSave(restart = false, moveFromDir: string | null | undefined = undefined) { + async function handleSave(restart = false, moveFromDir: string | null | undefined = undefined, persistenceAcked = false) { errors = {}; // Validate compose content (unless file location is needed and we have a path) @@ -1399,6 +1490,13 @@ ); if (checkResponse.ok) { const checkData = await checkResponse.json(); + // Non-persisted path is the more serious warning - surface it first (#1524). + if (checkData.persistenceWarning && !persistenceAcked) { + persistenceWarnText = checkData.persistenceWarning; + persistenceWarnProceed = () => handleSave(restart, moveFromDir, true); + showPersistenceWarn = true; + return; + } if (checkData.hasChanges && checkData.oldDir && checkData.fileCount > 0) { // Show confirmation dialog pathChangeOldDir = checkData.oldDir; @@ -2160,6 +2258,7 @@ injectedSecretKeys={mode === 'edit' ? injectedSecretKeys : []} providerType={selectedProviderType} providerName={selectedProviderName} + providerBound={selectedProviderBound} {probeError} {providerKeySet} {readonly} @@ -2353,6 +2452,29 @@ + + + + + + + This location isn't persisted + + +

+ {persistenceWarnText} +

+
+ + +
+ + + diff --git a/tests/unpersisted-compose-path.test.ts b/tests/unpersisted-compose-path.test.ts new file mode 100644 index 0000000..71f07e4 --- /dev/null +++ b/tests/unpersisted-compose-path.test.ts @@ -0,0 +1,113 @@ +/** + * #1524: warn when a custom stack compose path is not under any Dockhand container + * mount, so it would be written to the container's throwaway layer and lost on recreate. + */ + +import { describe, test, expect } from 'bun:test'; +import { unpersistedComposePathWarning, pathOverriddenBySubMount, translateContainerPathViaMount, __setCachedMountsForTest } from '../src/lib/server/host-path'; + +const MOUNTS = [ + { source: 'dockhand_data', destination: '/app/data' }, + { source: '/opt/stacks', destination: '/opt/stacks' }, +]; + +describe('unpersistedComposePathWarning', () => { + test('warns for a path outside every mount', () => { + const w = unpersistedComposePathWarning('/home/docker/minecraft/compose.yaml', MOUNTS); + expect(w).toContain('/home/docker/minecraft/compose.yaml'); + expect(w).toContain('recreated'); + }); + + test('no warning for a path under the data volume (default location)', () => { + expect(unpersistedComposePathWarning('/app/data/stacks/Production/web/compose.yaml', MOUNTS)).toBeNull(); + }); + + test('no warning for a path under a user-added bind mount', () => { + expect(unpersistedComposePathWarning('/opt/stacks/web/compose.yaml', MOUNTS)).toBeNull(); + }); + + test('a path exactly equal to a mount destination is under it', () => { + expect(unpersistedComposePathWarning('/opt/stacks', MOUNTS)).toBeNull(); + }); + + test('a prefix that is not a path boundary does NOT count as under the mount', () => { + // /app/database is not under /app/data even though the string starts with it. + expect(unpersistedComposePathWarning('/app/database/compose.yaml', MOUNTS)).not.toBeNull(); + }); + + test('bare metal (no mounts) never warns', () => { + expect(unpersistedComposePathWarning('/home/docker/x/compose.yaml', [])).toBeNull(); + }); + + test('a relative path is not guarded (resolved against DATA_DIR elsewhere)', () => { + expect(unpersistedComposePathWarning('compose.yaml', MOUNTS)).toBeNull(); + expect(unpersistedComposePathWarning('./sub/compose.yaml', MOUNTS)).toBeNull(); + }); + + test('null / empty path never warns', () => { + expect(unpersistedComposePathWarning(null, MOUNTS)).toBeNull(); + expect(unpersistedComposePathWarning('', MOUNTS)).toBeNull(); + expect(unpersistedComposePathWarning(undefined, MOUNTS)).toBeNull(); + }); +}); + +describe('pathOverriddenBySubMount (#1533)', () => { + // The exact #1533 topology: /app/data on a named volume, /app/data/stacks on a + // separate bind. A stack under /app/data/stacks lives on the bind, not the volume, + // so the DATA_DIR translation is wrong and must yield to the mount translation. + const SUBMOUNT = [ + { source: '/home/docker/docker/volumes/dockhand_data/_data', destination: '/app/data' }, + { source: '/home/docker/compose', destination: '/app/data/stacks' }, + ]; + + test('true when a deeper bind overrides the stack subpath', () => { + expect(pathOverriddenBySubMount('/app/data/stacks/adguard', '/app/data', SUBMOUNT)).toBe(true); + }); + + test('true when the path equals the sub-mount destination', () => { + expect(pathOverriddenBySubMount('/app/data/stacks', '/app/data', SUBMOUNT)).toBe(true); + }); + + test('false for a DATA_DIR path NOT covered by the sub-mount', () => { + // /app/data/db is under DATA_DIR but not under the /app/data/stacks bind. + expect(pathOverriddenBySubMount('/app/data/db', '/app/data', SUBMOUNT)).toBe(false); + }); + + test('false when the only mount IS DATA_DIR itself (normal single-volume setup)', () => { + const single = [{ source: 'dockhand_data', destination: '/app/data' }]; + expect(pathOverriddenBySubMount('/app/data/stacks/adguard', '/app/data', single)).toBe(false); + }); + + test('false with no mounts (bare metal)', () => { + expect(pathOverriddenBySubMount('/app/data/stacks/adguard', '/app/data', [])).toBe(false); + }); + + test('path-boundary safe: /app/database is not under /app/data', () => { + const m = [{ source: '/x', destination: '/app/database' }]; + expect(pathOverriddenBySubMount('/app/data/stacks/adguard', '/app/data', m)).toBe(false); + }); + + test('trailing slashes on DATA_DIR / destinations are normalized', () => { + const m = [{ source: '/home/docker/compose', destination: '/app/data/stacks/' }]; + expect(pathOverriddenBySubMount('/app/data/stacks/adguard', '/app/data/', m)).toBe(true); + }); + + // The load-bearing invariant of the #1533 fix: when we NULL dataDirHostPath because a + // sub-mount overrides, mountHostPath (translateContainerPathViaMount) MUST resolve non-null + // - otherwise we'd regress a wrong-but-found path into a hard-fail (unknown). + test('overridden => translateContainerPathViaMount resolves non-null (no hard-fail regression)', () => { + const mounts = [ + { source: '/home/docker/docker/volumes/dockhand_data/_data', destination: '/app/data' }, + { source: '/home/docker/compose', destination: '/app/data/stacks' }, + ]; + const path = '/app/data/stacks/adguard'; + try { + __setCachedMountsForTest(mounts); + expect(pathOverriddenBySubMount(path, '/app/data', mounts)).toBe(true); + // the very folder the backup helper will bind-mount: + expect(translateContainerPathViaMount(path)).toBe('/home/docker/compose/adguard'); + } finally { + __setCachedMountsForTest(null); + } + }); +});