diff --git a/src/lib/utils/provider-form-config.ts b/src/lib/utils/provider-form-config.ts new file mode 100644 index 0000000..4df0d26 --- /dev/null +++ b/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, + loadedKeys: ReadonlySet +): Record { + const config: Record = {}; + 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; +} diff --git a/src/routes/settings/secrets/ProviderModal.svelte b/src/routes/settings/secrets/ProviderModal.svelte index e7d41b9..498e862 100644 --- a/src/routes/settings/secrets/ProviderModal.svelte +++ b/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>({}); + // 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>(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 { - const config: Record = {}; - 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): boolean { diff --git a/tests/provider-form-config.test.ts b/tests/provider-form-config.test.ts new file mode 100644 index 0000000..930d459 --- /dev/null +++ b/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> = { + '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 = {}; + 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 = {}; + 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 + }); +});