fix switching an Infisical provider back to a static token

src/lib/utils/provider-form-config.ts

@@ -0,0 +1,27 @@
+/**
+ * Build the config payload the secret-provider edit form POSTs from its field values.
+ *
+ * - A filled field is sent as-is.
+ * - A blank SECRET field is omitted (blank = keep the stored secret).
+ * - A blank NON-SECRET field that WAS in the loaded config is sent as an explicit '' so
+ *   the server can tell "user cleared it" from "unchanged/absent". Clearing a non-secret
+ *   partner (e.g. Infisical clientId) is what drops its orphaned paired secret
+ *   (clientSecret) server-side; without the explicit '', the stale secret is merged back
+ *   and validation wedges on "Client ID is required when a client secret is set" (#1448).
+ */
+export function collectProviderFormConfig(
+	fieldDefs: Array<{ key: string; isSecret: boolean }>,
+	formValues: Record<string, string>,
+	loadedKeys: ReadonlySet<string>
+): Record<string, string> {
+	const config: Record<string, string> = {};
+	for (const field of fieldDefs) {
+		const value = (formValues[field.key] ?? '').trim();
+		if (value) {
+			config[field.key] = value;
+		} else if (!field.isSecret && loadedKeys.has(field.key)) {
+			config[field.key] = '';
+		}
+	}
+	return config;
+}

src/routes/settings/secrets/ProviderModal.svelte

@@ -133,6 +133,7 @@
 	import { scale } from 'svelte/transition';
 	import { backOut, cubicIn } from 'svelte/easing';
 	import { getProviderIcon } from '$lib/components/provider-icons';
+	import { collectProviderFormConfig } from '$lib/utils/provider-form-config';
 	import { toast } from 'svelte-sonner';
 	import { focusFirstInput } from '$lib/utils';
 
@@ -156,6 +157,11 @@
 	let formType = $state('op-service-account');
 	// One value per config field; blank means 'unset' (on edit: keep existing).
 	let formConfig = $state<Record<string, string>>({});
+	// Non-secret keys that were present in the stored config when the edit form loaded. If
+	// the user clears one, we must send an explicit '' so the server knows it was cleared
+	// (a merely absent key means "unchanged"). This is what lets clearing Infisical's
+	// clientId drop its orphaned clientSecret (#1448).
+	let loadedConfigKeys = $state<Set<string>>(new Set());
 	let formError = $state('');
 	let formSaving = $state(false);
 	let formTesting = $state(false);
@@ -172,6 +178,7 @@
 
 	function resetConfig() {
 		formConfig = {};
+		loadedConfigKeys = new Set();
 	}
 
 	function resetForm() {
@@ -211,20 +218,21 @@
 				if (value != null) next[key] = String(value);
 			}
 			formConfig = next; // secret fields (token) are absent -> stay blank
+			loadedConfigKeys = new Set(Object.keys(next));
 		} catch {
 			// leave fields blank on failure - the user can re-enter them
 		}
 	}
 
-	// A blank secret field on edit means "keep the stored value"; non-secret fields are
-	// pre-filled (loadProviderConfig). Collect only the fields the user actually filled.
+	// See collectProviderFormConfig: a blank secret field means "keep stored"; a cleared
+	// non-secret field that was loaded is sent as an explicit '' so the server can drop it
+	// (and any paired secret, e.g. Infisical clientSecret when clientId is cleared). #1448
 	function collectConfig(): Record<string, string> {
-		const config: Record<string, string> = {};
-		for (const field of fields) {
-			const value = (formConfig[field.key] ?? '').trim();
-			if (value) config[field.key] = value;
-		}
-		return config;
+		return collectProviderFormConfig(
+			fields.map((f) => ({ key: f.key, isSecret: f.type === 'password' })),
+			formConfig,
+			loadedConfigKeys
+		);
 	}
 
 	function fieldRequired(field: ProviderField, config: Record<string, string>): boolean {

tests/provider-form-config.test.ts

@@ -0,0 +1,183 @@
+import { describe, test, expect } from 'bun:test';
+import { collectProviderFormConfig } from '../src/lib/utils/provider-form-config';
+import { mergeProviderConfigForWrite } from '../src/lib/server/secretproviders/shared';
+
+// Field shape (key + isSecret) per provider, mirroring PROVIDER_FIELDS in
+// routes/settings/secrets/ProviderModal.svelte. Only the shape matters for what the edit
+// form emits, so this is kept as a small local table rather than importing the .svelte
+// module. If a provider's fields change there, add/adjust the row here too.
+const PROVIDER_FIELD_SHAPES: Record<string, Array<{ key: string; isSecret: boolean }>> = {
+	'op-service-account': [{ key: 'token', isSecret: true }],
+	'op-connect': [
+		{ key: 'host', isSecret: false },
+		{ key: 'token', isSecret: true }
+	],
+	infisical: [
+		{ key: 'host', isSecret: false },
+		{ key: 'token', isSecret: true },
+		{ key: 'clientId', isSecret: false },
+		{ key: 'clientSecret', isSecret: true },
+		{ key: 'projectId', isSecret: false },
+		{ key: 'environment', isSecret: false },
+		{ key: 'path', isSecret: false }
+	],
+	vault: [
+		{ key: 'address', isSecret: false },
+		{ key: 'token', isSecret: true },
+		{ key: 'namespace', isSecret: false },
+		{ key: 'mount', isSecret: false }
+	],
+	doppler: [
+		{ key: 'token', isSecret: true },
+		{ key: 'project', isSecret: false },
+		{ key: 'config', isSecret: false }
+	],
+	bitwarden: [
+		{ key: 'token', isSecret: true },
+		{ key: 'serverUrl', isSecret: false }
+	],
+	proton: [{ key: 'token', isSecret: true }],
+	'azure-kv': [
+		{ key: 'vaultUri', isSecret: false },
+		{ key: 'tenantId', isSecret: false },
+		{ key: 'clientId', isSecret: false },
+		{ key: 'clientSecret', isSecret: true }
+	]
+};
+
+const setOf = (...keys: string[]) => new Set(keys);
+
+describe('collectProviderFormConfig - core rules', () => {
+	test('a filled field is sent as-is', () => {
+		const cfg = collectProviderFormConfig(
+			PROVIDER_FIELD_SHAPES.vault,
+			{ address: 'http://vault', token: 'hvs.x', mount: 'kv' },
+			setOf('address', 'mount')
+		);
+		expect(cfg).toEqual({ address: 'http://vault', token: 'hvs.x', mount: 'kv' });
+	});
+
+	test('a blank secret is omitted (blank = keep stored)', () => {
+		const cfg = collectProviderFormConfig(
+			PROVIDER_FIELD_SHAPES.vault,
+			{ address: 'http://vault', token: '', mount: 'kv' },
+			setOf('address', 'token', 'mount')
+		);
+		expect('token' in cfg).toBe(false);
+	});
+
+	test('a cleared loaded non-secret is sent as ""', () => {
+		const cfg = collectProviderFormConfig(
+			PROVIDER_FIELD_SHAPES.vault,
+			{ address: 'http://vault', mount: '' },
+			setOf('address', 'mount')
+		);
+		expect(cfg.mount).toBe('');
+	});
+
+	test('a non-secret that was never loaded is not invented as ""', () => {
+		const cfg = collectProviderFormConfig(
+			PROVIDER_FIELD_SHAPES.vault,
+			{ address: 'http://vault' },
+			setOf('address') // mount never existed
+		);
+		expect('mount' in cfg).toBe(false);
+	});
+
+	test('whitespace-only values are treated as blank', () => {
+		const cfg = collectProviderFormConfig(
+			PROVIDER_FIELD_SHAPES.vault,
+			{ address: '  ', token: '   ', mount: '  ' },
+			setOf('address', 'mount')
+		);
+		expect(cfg.address).toBe(''); // loaded non-secret -> explicit clear
+		expect(cfg.mount).toBe('');
+		expect('token' in cfg).toBe(false); // blank secret omitted
+	});
+});
+
+// The bug (#1448) and the fix must behave correctly for EVERY provider's field set - a
+// cleared secret must never be omitted-as-keep when its partner is cleared, and every
+// provider's optional non-secret fields must survive a clear as an explicit "".
+describe('per-provider: no field is silently dropped or wrongly kept', () => {
+	for (const [type, fields] of Object.entries(PROVIDER_FIELD_SHAPES)) {
+		const secretKeys = fields.filter((f) => f.isSecret).map((f) => f.key);
+		const nonSecretKeys = fields.filter((f) => !f.isSecret).map((f) => f.key);
+
+		test(`${type}: blank secrets omitted, cleared loaded non-secrets sent as ""`, () => {
+			// Simulate: everything loaded, user clears every field.
+			const allKeys = fields.map((f) => f.key);
+			const blankForm: Record<string, string> = {};
+			for (const k of allKeys) blankForm[k] = '';
+			const cfg = collectProviderFormConfig(fields, blankForm, new Set(allKeys));
+
+			for (const k of secretKeys) {
+				expect(k in cfg).toBe(false); // secrets never sent blank
+			}
+			for (const k of nonSecretKeys) {
+				expect(cfg[k]).toBe(''); // cleared non-secrets sent explicitly
+			}
+		});
+
+		test(`${type}: a fresh full config passes through unchanged`, () => {
+			const filled: Record<string, string> = {};
+			for (const f of fields) filled[f.key] = `val-${f.key}`;
+			const cfg = collectProviderFormConfig(fields, filled, new Set());
+			expect(cfg).toEqual(filled);
+		});
+	}
+});
+
+// #1448 is Infisical-specific (clientSecret paired with clientId), but the client rule is
+// generic. Prove the full chain (form output -> server merge) for the paired case, and that
+// no OTHER provider's stored secret is dropped just because a non-secret was cleared.
+describe('#1448 full chain: form output -> server merge', () => {
+	test('infisical: clearing clientId drops the orphaned clientSecret', () => {
+		const stored = { host: 'http://infisical', clientId: 'cid', clientSecret: 'sec' };
+		const emitted = collectProviderFormConfig(
+			PROVIDER_FIELD_SHAPES.infisical,
+			{ host: 'http://infisical', clientId: '', clientSecret: '', token: 'new-tok' },
+			setOf('host', 'clientId', 'clientSecret')
+		);
+		const merged = mergeProviderConfigForWrite(emitted, stored);
+		expect(merged.token).toBe('new-tok');
+		expect(merged.clientSecret).toBeUndefined();
+	});
+
+	test('infisical: keeping clientId preserves the stored clientSecret', () => {
+		const stored = { host: 'http://infisical', clientId: 'cid', clientSecret: 'sec' };
+		const emitted = collectProviderFormConfig(
+			PROVIDER_FIELD_SHAPES.infisical,
+			{ host: 'http://new', clientId: 'cid', clientSecret: '' },
+			setOf('host', 'clientId')
+		);
+		const merged = mergeProviderConfigForWrite(emitted, stored);
+		expect(merged.clientSecret).toBe('sec');
+	});
+
+	test('azure-kv: clearing a non-secret does NOT drop the stored clientSecret (not a merge-paired secret)', () => {
+		// azure-kv's clientSecret has no PAIRED_SECRET_PARTNERS entry, so a blank clientSecret
+		// keeps the stored one regardless of other fields - clearing tenantId must not wipe it.
+		const stored = { vaultUri: 'https://v', tenantId: 'tid', clientId: 'cid', clientSecret: 'sec' };
+		const emitted = collectProviderFormConfig(
+			PROVIDER_FIELD_SHAPES['azure-kv'],
+			{ vaultUri: 'https://v', tenantId: '', clientId: 'cid', clientSecret: '' },
+			setOf('vaultUri', 'tenantId', 'clientId')
+		);
+		const merged = mergeProviderConfigForWrite(emitted, stored);
+		expect(merged.clientSecret).toBe('sec'); // kept
+		expect(merged.tenantId).toBe(''); // cleared coordinate honored
+	});
+
+	test('vault: clearing mount keeps the stored token (blank secret = keep)', () => {
+		const stored = { address: 'http://vault', token: 'hvs.stored', mount: 'kv' };
+		const emitted = collectProviderFormConfig(
+			PROVIDER_FIELD_SHAPES.vault,
+			{ address: 'http://vault', mount: '' },
+			setOf('address', 'mount')
+		);
+		const merged = mergeProviderConfigForWrite(emitted, stored);
+		expect(merged.token).toBe('hvs.stored');
+		expect(merged.mount).toBe(''); // server reads '' as the default mount
+	});
+});