From 07f29f5afc6740670ab659ffdba8a2034e5c0f2a Mon Sep 17 00:00:00 2001 From: Jarek Krochmalski Date: Sun, 6 Sep 2026 17:03:47 +0530 Subject: [PATCH] deleting a secret provider warns which stacks it unbinds --- src/lib/components/StackEnvVarsPanel.svelte | 28 +++++++++++- src/lib/server/db.ts | 14 ++++++ src/lib/server/git-deploy-policy.ts | 33 ++++++++++++++ src/routes/api/git/stacks/+server.ts | 10 ++++- src/routes/api/git/stacks/[id]/+server.ts | 8 +++- .../api/secret-providers/[id]/+server.ts | 17 +++++-- src/routes/settings/secrets/SecretsTab.svelte | 44 +++++++++++++++++-- src/routes/stacks/GitStackModal.svelte | 1 + tests/git-deploy-policy.test.ts | 41 +++++++++++++++++ 9 files changed, 186 insertions(+), 10 deletions(-) create mode 100644 src/lib/server/git-deploy-policy.ts create mode 100644 tests/git-deploy-policy.test.ts diff --git a/src/lib/components/StackEnvVarsPanel.svelte b/src/lib/components/StackEnvVarsPanel.svelte index b51a734..b051e08 100644 --- a/src/lib/components/StackEnvVarsPanel.svelte +++ b/src/lib/components/StackEnvVarsPanel.svelte @@ -26,6 +26,13 @@ /** Bound provider type/name, for the injected banner + pills. */ providerType?: string | null; providerName?: string | null; + /** + * Whether a secret provider is CURRENTLY bound to this stack. When false but + * injectedSecretKeys is non-empty, the keys are historical (from the last deploy) + * and the next deploy will drop them - the banner says so instead of implying they + * are still active (#1522). + */ + providerBound?: boolean; /** Set when the live provider probe failed - shown as an amber line. */ probeError?: string | null; /** Key NAMES the live probe found in the bound provider (present RIGHT NOW). @@ -52,6 +59,7 @@ injectedSecretKeys = [], providerType = null, providerName = null, + providerBound = true, probeError = null, providerKeySet = new Set(), showInterpolationHint = false, @@ -524,7 +532,7 @@ {/if} - {#if injectedSecretKeys.length > 0} + {#if injectedSecretKeys.length > 0 && providerBound}
@@ -552,6 +560,24 @@
+ {:else if injectedSecretKeys.length > 0 && !providerBound} + +
+ +
+
No secret provider is bound
+

+ These {injectedSecretKeys.length} secret{injectedSecretKeys.length === 1 ? ' was' : 's were'} injected on the last deploy but the stack is no longer bound to a provider. The next deploy will drop {injectedSecretKeys.length === 1 ? 'it' : 'them'}. Reselect a provider to keep them. +

+
+ {#each injectedSecretKeys as key} + + {key} + + {/each} +
+
+
{/if} {#if probeError} diff --git a/src/lib/server/db.ts b/src/lib/server/db.ts index 35aafbd..8dc2f92 100644 --- a/src/lib/server/db.ts +++ b/src/lib/server/db.ts @@ -354,6 +354,20 @@ export async function getSecretProviderById(id: number): Promise { + const rows = await db.select({ id: secretProviders.id }).from(secretProviders).where(eq(secretProviders.id, id)); + return rows.length > 0; +} + +/** Stacks currently bound to a secret provider, so a delete can warn which ones lose it. */ +export async function getStacksUsingSecretProvider(id: number): Promise> { + return db + .select({ stackName: stackSources.stackName, environmentId: stackSources.environmentId }) + .from(stackSources) + .where(eq(stackSources.secretProviderId, id)); +} + export async function createSecretProvider(data: { type: SecretProviderType; name: string; diff --git a/src/lib/server/git-deploy-policy.ts b/src/lib/server/git-deploy-policy.ts new file mode 100644 index 0000000..af3cf09 --- /dev/null +++ b/src/lib/server/git-deploy-policy.ts @@ -0,0 +1,33 @@ +/** + * Pure git-stack deploy decisions, extracted so they are unit-testable without the + * I/O-heavy deployGitStack. A manual/one-off deploy (`force`) deploys but does NOT + * force-recreate, so only shouldDeployGitStack takes it. + */ + +/** + * Whether to deploy at all. A manual/one-off deploy (`force`) or a real git change + * always deploys; the "always redeploy on webhook/scheduled sync" setting forces a + * deploy even when git reported no changes. + */ +export function shouldDeployGitStack(input: { + force: boolean; + forceRedeploy: boolean; + gitUpdated: boolean; +}): boolean { + return input.force || input.forceRedeploy || input.gitUpdated; +} + +/** + * Whether to pass `--force-recreate`. Recreate on a real git change OR when the user + * enabled "always redeploy": that setting exists to force the container back in step + * with its configuration (a changed env var, secret, or rebound provider leaves no git + * diff), and a plain `up` without --force-recreate would no-op and never re-inject the + * shell-env secrets (#1523). Default (setting off, no git change) does NOT recreate, so + * an ordinary sync never causes a surprise recreate. + */ +export function shouldForceRecreateGitStack(input: { + forceRedeploy: boolean; + gitUpdated: boolean; +}): boolean { + return input.gitUpdated || input.forceRedeploy; +} diff --git a/src/routes/api/git/stacks/+server.ts b/src/routes/api/git/stacks/+server.ts index d19fc9d..3989beb 100644 --- a/src/routes/api/git/stacks/+server.ts +++ b/src/routes/api/git/stacks/+server.ts @@ -8,7 +8,8 @@ import { createGitRepository, upsertStackSource, setStackEnvVars, - getStackSource + getStackSource, + secretProviderExists } from '$lib/server/db'; import { deployGitStack } from '$lib/server/git'; import { authorize } from '$lib/server/authorize'; @@ -98,6 +99,13 @@ export const POST: RequestHandler = async (event) => { return json({ error: 'Permission denied: binding a secret provider requires the secrets permission' }, { status: 403 }); } + // A stale provider id (e.g. the provider was deleted/recreated while the editor + // held the old list) would otherwise hit a raw foreign-key error on save. Reject + // it cleanly so the user knows to reselect a provider (#1522). + if (typeof data.secretProviderId === 'number' && !(await secretProviderExists(data.secretProviderId))) { + return json({ error: 'The selected secret provider no longer exists. Reopen the stack and pick a current provider.' }, { status: 400 }); + } + // Check for name conflicts with existing stacks (regular/external/git) const existing = await getStackSource(trimmedStackName, data.environmentId || null); if (existing) { diff --git a/src/routes/api/git/stacks/[id]/+server.ts b/src/routes/api/git/stacks/[id]/+server.ts index 1e65534..eaa211e 100644 --- a/src/routes/api/git/stacks/[id]/+server.ts +++ b/src/routes/api/git/stacks/[id]/+server.ts @@ -1,6 +1,6 @@ import { json } from '@sveltejs/kit'; import type { RequestHandler } from './$types'; -import { getGitStack, updateGitStack, deleteGitStack, deleteStackSource, updateStackSourceName, updateStackEnvVarsName, setStackEnvVars, getStackEnvVars, deleteStackEnvVars, updateStackSource } from '$lib/server/db'; +import { getGitStack, updateGitStack, deleteGitStack, deleteStackSource, updateStackSourceName, updateStackEnvVarsName, setStackEnvVars, getStackEnvVars, deleteStackEnvVars, updateStackSource, secretProviderExists } from '$lib/server/db'; import { deleteGitStackFiles, deployGitStack } from '$lib/server/git'; import { normalizeStackBranchUpdate } from '$lib/git-stack-branch'; import { authorize } from '$lib/server/authorize'; @@ -90,6 +90,12 @@ export const PUT: RequestHandler = async (event) => { return json({ error: 'Permission denied: binding a secret provider requires the secrets permission' }, { status: 403 }); } + // A stale provider id (provider deleted/recreated while the editor held the old + // list) would otherwise hit a raw foreign-key error on save (#1522). + if (typeof data.secretProviderId === 'number' && !(await secretProviderExists(data.secretProviderId))) { + return json({ error: 'The selected secret provider no longer exists. Reopen the stack and pick a current provider.' }, { status: 400 }); + } + // Validate stack name if it's being changed if (data.stackName !== undefined) { const trimmedStackName = data.stackName.trim(); diff --git a/src/routes/api/secret-providers/[id]/+server.ts b/src/routes/api/secret-providers/[id]/+server.ts index 7776c5a..ca52963 100644 --- a/src/routes/api/secret-providers/[id]/+server.ts +++ b/src/routes/api/secret-providers/[id]/+server.ts @@ -4,7 +4,8 @@ import { getSecretProviders, getSecretProviderById, updateSecretProvider, - deleteSecretProvider + deleteSecretProvider, + getStacksUsingSecretProvider } from '$lib/server/db'; import { hasProvider } from '$lib/server/secretproviders'; import { redactProviderConfig } from '$lib/server/secretproviders/shared'; @@ -15,7 +16,7 @@ import { auditSecretProvider } from '$lib/server/audit'; * @openapi * summary: Get one secret provider with its non-secret config coordinates (the token is redacted out) * path: id:integer The secret provider id - * resp-200: {id:integer!, name:string!, type:string!, config:object!} + * resp-200: {id:integer!, name:string!, type:string!, config:object!, stacksUsing:array<{stackName:string!, environmentId:integer}>!} * resp-400: Invalid secret provider ID * resp-403: Permission denied (needs secrets:view) * resp-404: Secret provider not found @@ -39,7 +40,17 @@ export const GET: RequestHandler = async ({ params, cookies }) => { return json({ error: 'Secret provider not found' }, { status: 404 }); } const { config, ...summary } = full; - return json({ ...summary, config: redactProviderConfig(config) }); + // Name the stacks currently bound to this provider so the delete confirmation can + // warn the user that deleting it unbinds them (#1522). A provider is global, but its + // bound stacks live in environments - so env-scope the list to what the caller may + // see (Enterprise RBAC), or a restricted user would learn stack names in envs they + // cannot reach. null = all-environments access (admin/free/disabled) -> no filter. + const allStacksUsing = await getStacksUsingSecretProvider(id); + const accessibleEnvIds = await auth.getAccessibleEnvironmentIds(); + const stacksUsing = accessibleEnvIds === null + ? allStacksUsing + : allStacksUsing.filter((s) => s.environmentId === null || accessibleEnvIds.includes(s.environmentId)); + return json({ ...summary, config: redactProviderConfig(config), stacksUsing }); }; /** diff --git a/src/routes/settings/secrets/SecretsTab.svelte b/src/routes/settings/secrets/SecretsTab.svelte index e8aa0b4..94fc223 100644 --- a/src/routes/settings/secrets/SecretsTab.svelte +++ b/src/routes/settings/secrets/SecretsTab.svelte @@ -30,6 +30,11 @@ let showModal = $state(false); let editing = $state(null); let confirmDeleteId = $state(null); + // Stacks bound to the provider whose delete popover is open, so we can warn the + // user that deleting it unbinds them (#1522). Keyed by nothing - only one popover is + // open at a time. + let deleteAffectedStacks = $state>([]); + let loadingAffected = $state(false); let testingId = $state(null); // Brief green tick on the tile's Test button right after a successful test. let testOkId = $state(null); @@ -53,6 +58,22 @@ showModal = true; } + async function loadAffectedStacks(id: number) { + loadingAffected = true; + deleteAffectedStacks = []; + try { + const res = await fetch(`/api/secret-providers/${id}`); + if (res.ok) { + const data = await res.json(); + deleteAffectedStacks = data.stacksUsing ?? []; + } + } catch { + // Non-fatal: the confirmation still works, just without the stack list. + } finally { + loadingAffected = false; + } + } + async function deleteProvider(id: number) { try { const response = await fetch(`/api/secret-providers/${id}`, { @@ -195,10 +216,11 @@ position="left" onConfirm={() => deleteProvider(provider.id)} - onOpenChange={(open) => - (confirmDeleteId = open - ? provider.id - : null)} + onOpenChange={(open) => { + confirmDeleteId = open ? provider.id : null; + if (open) loadAffectedStacks(provider.id); + else deleteAffectedStacks = []; + }} > {#snippet children({ open })} {/snippet} + {#snippet extraContent()} + {#if loadingAffected} +

Checking which stacks use this provider...

+ {:else if deleteAffectedStacks.length > 0} +

+ This unbinds {deleteAffectedStacks.length} stack{deleteAffectedStacks.length === 1 ? '' : 's'}; their next deploy drops the injected secrets: +

+
    + {#each deleteAffectedStacks as s} +
  • {s.stackName}
    • + {/each} +
+ {/if} + {/snippet} {/if} diff --git a/src/routes/stacks/GitStackModal.svelte b/src/routes/stacks/GitStackModal.svelte index 41416db..16108f6 100644 --- a/src/routes/stacks/GitStackModal.svelte +++ b/src/routes/stacks/GitStackModal.svelte @@ -1365,6 +1365,7 @@ injectedSecretKeys={gitStack !== null ? injectedSecretKeys : []} providerType={secretProviders.find((p) => p.id === formSecretProviderId)?.type ?? null} providerName={secretProviders.find((p) => p.id === formSecretProviderId)?.name ?? null} + providerBound={formSecretProviderId != null && secretProviders.some((p) => p.id === formSecretProviderId)} placeholder={{ key: 'MY_VAR', value: 'value' }} infoText="Override variables from your repository env files. Non-secrets are saved to .env.dockhand in the stack directory. Secrets are stored in the database and injected via shell environment at deploy time.

Variables are available for compose file interpolation using ${'{VAR_NAME}'} syntax. They are not automatically injected into containers — use environment: or reference .env.dockhand in env_file: to pass them through." existingSecretKeys={gitStack !== null ? existingSecretKeys : new Set()} diff --git a/tests/git-deploy-policy.test.ts b/tests/git-deploy-policy.test.ts new file mode 100644 index 0000000..3761a47 --- /dev/null +++ b/tests/git-deploy-policy.test.ts @@ -0,0 +1,41 @@ +/** + * #1523: the "always redeploy" (forceRedeploy) setting must drive --force-recreate, + * not just whether we deploy. Previously forceRecreate was computed from the git-diff + * flag alone, so the setting was read and discarded and the container never re-received + * its shell-env secrets on a config-only change. + */ + +import { describe, test, expect } from 'bun:test'; +import { shouldDeployGitStack, shouldForceRecreateGitStack } from '../src/lib/server/git-deploy-policy'; + +describe('shouldDeployGitStack', () => { + test('deploys on a real git change', () => { + expect(shouldDeployGitStack({ force: false, forceRedeploy: false, gitUpdated: true })).toBe(true); + }); + test('deploys on a manual/forced deploy', () => { + expect(shouldDeployGitStack({ force: true, forceRedeploy: false, gitUpdated: false })).toBe(true); + }); + test('deploys when forceRedeploy is on even with no git change', () => { + expect(shouldDeployGitStack({ force: false, forceRedeploy: true, gitUpdated: false })).toBe(true); + }); + test('skips when nothing changed and nothing forced', () => { + expect(shouldDeployGitStack({ force: false, forceRedeploy: false, gitUpdated: false })).toBe(false); + }); +}); + +describe('shouldForceRecreateGitStack', () => { + test('recreates on a real git change', () => { + expect(shouldForceRecreateGitStack({ forceRedeploy: false, gitUpdated: true })).toBe(true); + }); + test('#1523: recreates when forceRedeploy is on even with NO git change', () => { + // The core bug: this was false before the fix, so the container never recreated + // and the shell-env secrets were never re-injected. + expect(shouldForceRecreateGitStack({ forceRedeploy: true, gitUpdated: false })).toBe(true); + }); + test('recreates when both are set', () => { + expect(shouldForceRecreateGitStack({ forceRedeploy: true, gitUpdated: true })).toBe(true); + }); + test('default (setting off, no git change) does NOT recreate - no surprise recreates', () => { + expect(shouldForceRecreateGitStack({ forceRedeploy: false, gitUpdated: false })).toBe(false); + }); +});