From 5fef45e003fb790020dea8f74458b1441aa03f0c Mon Sep 17 00:00:00 2001 From: jarek Date: Fri, 14 Aug 2026 14:01:36 +0200 Subject: [PATCH] validate secret provider config before save --- src/lib/server/db.ts | 13 +++---- src/lib/server/secretproviders/connect.ts | 5 ++- src/lib/server/secretproviders/doppler.ts | 5 ++- src/lib/server/secretproviders/infisical.ts | 5 ++- src/lib/server/secretproviders/shared.ts | 38 +++++++++++++++++++ src/lib/server/secretproviders/vault.ts | 21 +++++++--- .../api/secret-providers/[id]/test/+server.ts | 27 +++++++++++-- .../settings/secrets/ProviderModal.svelte | 23 +++++------ 8 files changed, 106 insertions(+), 31 deletions(-) diff --git a/src/lib/server/db.ts b/src/lib/server/db.ts index 49367fe..4a1eefa 100644 --- a/src/lib/server/db.ts +++ b/src/lib/server/db.ts @@ -6,7 +6,7 @@ */ import type { SecretProviderConfig, SecretProviderType } from './secretproviders/shared'; -import { SECRET_CONFIG_KEYS } from './secretproviders/shared'; +import { mergeProviderConfigForWrite } from './secretproviders/shared'; import { db, isPostgres, @@ -386,13 +386,10 @@ export async function updateSecretProvider( if (typeUnchanged) { const existing = await getSecretProviderById(id); if (existing) { - for (const key of SECRET_CONFIG_KEYS) { - const incoming = (data.config as Record)[key]; - if (incoming === undefined || incoming === '') { - const stored = (existing.config as Record)[key]; - if (stored !== undefined) merged[key] = stored; - } - } + merged = mergeProviderConfigForWrite( + data.config as Record, + existing.config as Record + ); } } const encrypted = encrypt(JSON.stringify(merged)); diff --git a/src/lib/server/secretproviders/connect.ts b/src/lib/server/secretproviders/connect.ts index 9410b14..902c152 100644 --- a/src/lib/server/secretproviders/connect.ts +++ b/src/lib/server/secretproviders/connect.ts @@ -15,7 +15,7 @@ import { request } from 'undici'; import type { ConnectConfig, SecretProvider, TestConnectionResult } from './shared'; -import { assertSafeProviderHost } from './shared'; +import { assertSafeProviderHost, parseProviderError, isJsonResponse } from './shared'; import { UnsupportedOperationError } from './shared'; /** A vault as returned by GET /v1/vaults. */ @@ -251,6 +251,9 @@ export const connectProvider: SecretProvider = { }); const text = await body.text(); if (statusCode >= 200 && statusCode < 300) { + if (!isJsonResponse(text)) { + return { ok: false, error: '1Password Connect did not return a JSON response - the host may not be a Connect server' }; + } return { ok: true }; } if (statusCode === 401 || statusCode === 403) { diff --git a/src/lib/server/secretproviders/doppler.ts b/src/lib/server/secretproviders/doppler.ts index 9d4897b..018c54f 100644 --- a/src/lib/server/secretproviders/doppler.ts +++ b/src/lib/server/secretproviders/doppler.ts @@ -20,7 +20,7 @@ import { request } from 'undici'; import type { DopplerConfig, SecretProvider, TestConnectionResult } from './shared'; -import { assertSafeProviderHost, parseProviderError } from './shared'; +import { assertSafeProviderHost, parseProviderError, isJsonResponse } from './shared'; import { UnsupportedOperationError } from './shared'; const DEFAULT_HOST = 'https://api.doppler.com'; @@ -77,6 +77,9 @@ export const dopplerProvider: SecretProvider = { // message parsed from Doppler's own {messages:[...]} shape. const rawBody = await body.text().catch(() => ''); if (statusCode >= 200 && statusCode < 300) { + if (!isJsonResponse(rawBody)) { + return { ok: false, error: 'Doppler did not return a JSON response - the host may not be a Doppler server' }; + } return { ok: true }; } if (rawBody) console.warn(`[Doppler] testConnection ${statusCode}: ${rawBody}`); diff --git a/src/lib/server/secretproviders/infisical.ts b/src/lib/server/secretproviders/infisical.ts index c8a7952..afd4fb5 100644 --- a/src/lib/server/secretproviders/infisical.ts +++ b/src/lib/server/secretproviders/infisical.ts @@ -18,7 +18,7 @@ import { request } from 'undici'; import type { InfisicalConfig, SecretProvider, TestConnectionResult } from './shared'; -import { assertSafeProviderHost, parseProviderError } from './shared'; +import { assertSafeProviderHost, parseProviderError, isJsonResponse } from './shared'; import { UnsupportedOperationError } from './shared'; /** Shape of a single secret in the /api/v3/secrets/raw response. */ @@ -97,6 +97,9 @@ export const infisicalProvider: SecretProvider = { // message parsed from Infisical's own {message}/{messages} shape. const rawBody = await body.text().catch(() => ''); if (statusCode >= 200 && statusCode < 300) { + if (!isJsonResponse(rawBody)) { + return { ok: false, error: 'Infisical did not return a JSON response - the host may not be an Infisical server' }; + } return { ok: true }; } if (rawBody) console.warn(`[Infisical] testConnection ${statusCode}: ${rawBody}`); diff --git a/src/lib/server/secretproviders/shared.ts b/src/lib/server/secretproviders/shared.ts index 5da35b8..ec05eb3 100644 --- a/src/lib/server/secretproviders/shared.ts +++ b/src/lib/server/secretproviders/shared.ts @@ -102,6 +102,22 @@ export function parseProviderError(rawBody: string | undefined | null): string | return pick(obj.errors) ?? pick(obj.messages) ?? pick(obj.message) ?? null; } +/** + * True when `body` parses as a JSON object or array. A 2xx status alone does not prove a + * host is the expected backend - a parked domain / captive portal / reverse proxy answers + * 200 with an HTML page. Every provider backend answers JSON, so testConnection requires + * this before reporting success. Rejects empty bodies and bare scalars (`5`, `true`). + */ +export function isJsonResponse(body: string | undefined | null): boolean { + if (!body || !body.trim()) return false; + try { + const v = JSON.parse(body); + return typeof v === 'object' && v !== null; + } catch { + return false; + } +} + /** 1Password service account: a single bearer token. */ export interface ServiceAccountConfig { token: string; @@ -170,6 +186,28 @@ export type SecretProviderConfig = */ export const SECRET_CONFIG_KEYS = new Set(['token']); +/** + * Merges an incoming (edit-form) config OVER the stored one for a write/test: the incoming + * non-secret coordinates win, but any SECRET_CONFIG_KEY (the token) that is blank/absent in + * the incoming falls back to the STORED value - because the edit form leaves the token blank + * to mean "keep the stored secret". Used by BOTH the update (persist) and the edit-mode Test + * so a Test validates exactly what a Save would persist. Keep them on this one helper so they + * can never diverge. + */ +export function mergeProviderConfigForWrite( + incoming: Record, + stored: Record +): Record { + const merged: Record = { ...incoming }; + for (const key of SECRET_CONFIG_KEYS) { + const v = incoming[key]; + if (v === undefined || v === '') { + if (stored[key] !== undefined) merged[key] = stored[key]; + } + } + return merged; +} + /** * Returns a copy of a provider config with secret values removed, so the edit form * can pre-fill the non-secret coordinates without the token ever leaving the server. diff --git a/src/lib/server/secretproviders/vault.ts b/src/lib/server/secretproviders/vault.ts index 3a3228d..4a93a99 100644 --- a/src/lib/server/secretproviders/vault.ts +++ b/src/lib/server/secretproviders/vault.ts @@ -18,7 +18,7 @@ */ import { request } from 'undici'; -import { UnsupportedOperationError, assertSafeProviderHost, sanitizeSelectorPath, parseProviderError } from './shared'; +import { UnsupportedOperationError, assertSafeProviderHost, sanitizeSelectorPath, parseProviderError, isJsonResponse } from './shared'; import type { SecretProvider, TestConnectionResult, VaultConfig } from './shared'; /** Undici response body handle, derived so we don't import undici's types. */ @@ -93,19 +93,26 @@ export const vaultProvider: SecretProvider = { return { ok: false, error: 'Token is empty' }; } try { - const { statusCode, body } = await request(`${apiBase(config)}/auth/token/lookup-self`, { + // Read the KV v2 mount config: validates host + token + the configured mount + // in one authenticated call - the same properties deploy (resolveBulk) uses. A + // wrong mount 404s and a bad token 403s, so both fail Test instead of passing. + const mount = encodeURIComponent(config.mount || 'secret'); + const { statusCode, body } = await request(`${apiBase(config)}/${mount}/config`, { method: 'GET', headers: authHeaders(config) }); + const rawBody = await body.text().catch(() => ''); if (statusCode >= 200 && statusCode < 300) { - await body.text().catch(() => ''); + // A 2xx alone isn't proof: a parked domain / proxy can answer 200 with HTML. + if (!isJsonResponse(rawBody)) { + return { ok: false, error: 'Vault did not return a JSON response - the host may not be a Vault server' }; + } return { ok: true }; } - const rawBody = await body.text().catch(() => ''); // Log the full upstream body server-side; show the client only a message // parsed from Vault's own {errors:[...]} shape (a non-Vault host probed via // SSRF returns other text that won't parse, so nothing leaks). - if (rawBody) console.warn(`[HashiCorp Vault] token lookup ${statusCode}: ${rawBody}`); + if (rawBody) console.warn(`[HashiCorp Vault] mount config ${mount} ${statusCode}: ${rawBody}`); const safe = parseProviderError(rawBody); return { ok: false, @@ -143,7 +150,9 @@ export const vaultProvider: SecretProvider = { if (statusCode < 200 || statusCode >= 300) { const detail = await readErrorDetail(body); if (detail) console.warn(`[HashiCorp Vault] read ${mount}/${path} ${statusCode}: ${detail}`); - throw new Error(`HashiCorp Vault: read failed for ${mount}/${path} (status ${statusCode})`); + throw new Error( + `HashiCorp Vault: read failed for ${mount}/${path} (status ${statusCode})${detail ? `: ${detail}` : ''}` + ); } const payload = (await body.json()) as KvV2ReadResponse; diff --git a/src/routes/api/secret-providers/[id]/test/+server.ts b/src/routes/api/secret-providers/[id]/test/+server.ts index c39b5aa..c56a174 100644 --- a/src/routes/api/secret-providers/[id]/test/+server.ts +++ b/src/routes/api/secret-providers/[id]/test/+server.ts @@ -3,9 +3,17 @@ import type { RequestHandler } from './$types'; import { getSecretProviderById } from '$lib/server/db'; import { authorize } from '$lib/server/authorize'; import { testProviderConnection } from '$lib/server/secretproviders'; +import { mergeProviderConfigForWrite } from '$lib/server/secretproviders/shared'; +import type { SecretProviderConfig } from '$lib/server/secretproviders/shared'; -/** Test a stored provider using its persisted (decrypted) config. */ -export const POST: RequestHandler = async ({ params, cookies }) => { +/** + * Test a stored provider. With no body it tests the persisted config. With an optional + * `{ config }` override (the edit form's typed non-secret fields) it tests exactly what a + * Save would persist: the incoming config merged over the stored one, with a blank token + * falling back to the stored secret. The secret is NEVER supplied by the client - it comes + * from storage server-side - so this stays a `secrets:view` operation. + */ +export const POST: RequestHandler = async ({ params, cookies, request }) => { const auth = await authorize(cookies); if (auth.authEnabled && !(await auth.can('secrets', 'view'))) { return json({ error: 'Permission denied' }, { status: 403 }); @@ -21,7 +29,20 @@ export const POST: RequestHandler = async ({ params, cookies }) => { return json({ error: 'Secret provider not found' }, { status: 404 }); } - const result = await testProviderConnection(provider.type, provider.config); + let configToTest: SecretProviderConfig = provider.config; + try { + const body = await request.json(); + if (body && typeof body.config === 'object' && body.config !== null) { + configToTest = mergeProviderConfigForWrite( + body.config as Record, + provider.config as unknown as Record + ) as unknown as SecretProviderConfig; + } + } catch { + // No/invalid body: test the stored config as-is. + } + + const result = await testProviderConnection(provider.type, configToTest); if (!result.ok) { return json({ ok: false, error: result.error }, { status: 200 }); } diff --git a/src/routes/settings/secrets/ProviderModal.svelte b/src/routes/settings/secrets/ProviderModal.svelte index 716db79..f504e12 100644 --- a/src/routes/settings/secrets/ProviderModal.svelte +++ b/src/routes/settings/secrets/ProviderModal.svelte @@ -212,23 +212,24 @@ formError = ''; try { const config = collectConfig(); - // On edit the token is server-side (blank field = keep stored), so if the - // user hasn't typed a new one, test the STORED config; the pre-filled - // non-secret fields aren't enough to re-test from the client. - const hasNewSecret = fields.some((f) => f.type === 'password' && (config[f.key] ?? '') !== ''); + const missing = missingRequired(config, isEditing); + if (missing) { + formError = missing; + return; + } let response: Response; - if (isEditing && !hasNewSecret) { - // Test the stored (server-side) config. + if (isEditing) { + // Test EXACTLY what a Save would persist: the typed non-secret fields, merged + // server-side over the stored config (a blank token keeps the stored one). This + // makes an edited address/mount/namespace actually get tested - not the old + // stored config. response = await fetch(`/api/secret-providers/${provider!.id}/test`, { method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ config }), }); } else { - const missing = missingRequired(config, isEditing); - if (missing) { - formError = missing; - return; - } response = await fetch('/api/secret-providers/test', { method: 'POST', headers: { 'Content-Type': 'application/json' },