From c0c5353782792b208961ad46382c429129f41755 Mon Sep 17 00:00:00 2001 From: Jasper Date: Sun, 23 Aug 2026 23:17:35 +0000 Subject: [PATCH] fix(security): clear custom provider transition secrets (#11467) --- .../forms/CustomProviderForm.test.tsx | 180 ++++++++++++++++++ .../forms/CustomProviderForm.tsx | 41 +++- 2 files changed, 214 insertions(+), 7 deletions(-) create mode 100644 ui/desktop/src/components/settings/providers/modal/subcomponents/forms/CustomProviderForm.test.tsx diff --git a/ui/desktop/src/components/settings/providers/modal/subcomponents/forms/CustomProviderForm.test.tsx b/ui/desktop/src/components/settings/providers/modal/subcomponents/forms/CustomProviderForm.test.tsx new file mode 100644 index 000000000..8df9ef6e6 --- /dev/null +++ b/ui/desktop/src/components/settings/providers/modal/subcomponents/forms/CustomProviderForm.test.tsx @@ -0,0 +1,180 @@ +import { act, render, screen, waitFor } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; +import { describe, expect, it, vi } from 'vitest'; +import { IntlTestWrapper } from '../../../../../../i18n/test-utils'; +import CustomProviderForm from './CustomProviderForm'; + +const templates = vi.hoisted(() => ({ + a: { + providerId: 'template-a', + name: 'Template A', + format: 'openai', + apiUrl: 'https://a.example.com', + models: [ + { + id: 'model-a', + name: 'Model A', + contextLimit: 8192, + capabilities: { + toolCall: false, + reasoning: false, + attachment: false, + temperature: true, + }, + deprecated: false, + }, + ], + supportsStreaming: true, + envVar: 'TEMPLATE_A_API_KEY', + docUrl: '', + }, + b: { + providerId: 'template-b', + name: 'Template B', + format: 'anthropic', + apiUrl: 'https://b.example.com', + models: [ + { + id: 'model-b', + name: 'Model B', + contextLimit: 8192, + capabilities: { + toolCall: false, + reasoning: false, + attachment: false, + temperature: true, + }, + deprecated: false, + }, + ], + supportsStreaming: true, + envVar: 'TEMPLATE_B_API_KEY', + docUrl: '', + }, +})); + +vi.mock('../ProviderCatalogPicker', () => ({ + default: ({ onSelect }: { onSelect: (template: typeof templates.a) => void }) => ( +
+ + +
+ ), +})); + +const renderForm = (onSubmit = vi.fn()) => { + render( + , + { wrapper: IntlTestWrapper } + ); + return onSubmit; +}; + +const openTemplateCatalog = async (user: ReturnType) => { + await user.click(screen.getByText('Start from a provider template')); +}; + +const addHeader = async (user: ReturnType, name: string, value: string) => { + const nameInputs = screen.getAllByPlaceholderText('Header name'); + const valueInputs = screen.getAllByPlaceholderText('Value'); + await user.type(nameInputs[nameInputs.length - 1], name); + await user.type(valueInputs[valueInputs.length - 1], value); + await user.click(screen.getByRole('button', { name: 'Add' })); +}; + +describe('CustomProviderForm transitions', () => { + it('does not carry credentials from a cleared template into the next template', async () => { + const user = userEvent.setup(); + const onSubmit = renderForm(); + await openTemplateCatalog(user); + await user.click(screen.getByRole('button', { name: 'Use Template A' })); + + await user.type(screen.getByLabelText(/API Key/), 'template-a-secret'); + await addHeader(user, 'Authorization', 'Bearer template-a'); + + const pendingNames = screen.getAllByPlaceholderText('Header name'); + const pendingValues = screen.getAllByPlaceholderText('Value'); + await user.type(pendingNames[pendingNames.length - 1], 'Authorization'); + await user.type(pendingValues[pendingValues.length - 1], 'Bearer pending-template-a'); + await user.click(screen.getByRole('button', { name: 'Add' })); + expect(screen.getByText('A header with this name already exists')).toBeInTheDocument(); + + await user.click(screen.getByRole('button', { name: 'Clear' })); + await openTemplateCatalog(user); + await user.click(screen.getByRole('button', { name: 'Use Template B' })); + + expect(screen.getByLabelText(/API Key/)).toHaveValue(''); + expect(screen.queryByDisplayValue('Bearer template-a')).not.toBeInTheDocument(); + expect(screen.queryByDisplayValue('Bearer pending-template-a')).not.toBeInTheDocument(); + expect(screen.queryByText('A header with this name already exists')).not.toBeInTheDocument(); + + await user.type(screen.getByLabelText(/API Key/), 'template-b-secret'); + await addHeader(user, 'X-Template-B', 'template-b-header'); + await user.click(screen.getByRole('button', { name: 'Create Provider' })); + + await waitFor(() => expect(onSubmit).toHaveBeenCalledOnce()); + expect(onSubmit).toHaveBeenCalledWith( + expect.objectContaining({ + api_key: 'template-b-secret', + catalog_provider_id: 'template-b', + headers: { 'X-Template-B': 'template-b-header' }, + }) + ); + }); + + it('clears secrets and submit state when returning to the setup choice', async () => { + vi.spyOn(console, 'error').mockImplementation(() => {}); + const user = userEvent.setup(); + let rejectSubmit: ((reason: Error) => void) | undefined; + const onSubmit = vi.fn( + () => + new Promise((_resolve, reject) => { + rejectSubmit = reject; + }) + ); + renderForm(onSubmit); + await user.click(screen.getByText('Configure manually')); + + await user.type(screen.getByLabelText(/Display Name/), 'Manual Provider'); + await user.type(screen.getByLabelText(/API URL/), 'https://manual.example.com'); + await user.type(screen.getByLabelText(/Available Models/), 'model-a'); + await user.click(screen.getByLabelText('This provider requires an API key')); + await user.type(screen.getByLabelText(/API Key/), 'manual-secret'); + await addHeader(user, 'Authorization', 'Bearer manual-secret'); + + const pendingNames = screen.getAllByPlaceholderText('Header name'); + const pendingValues = screen.getAllByPlaceholderText('Value'); + await user.type(pendingNames[pendingNames.length - 1], 'Authorization'); + await user.type(pendingValues[pendingValues.length - 1], 'Bearer pending-secret'); + await user.click(screen.getByRole('button', { name: 'Add' })); + await user.click(screen.getByRole('button', { name: 'Create Provider' })); + await waitFor(() => expect(onSubmit).toHaveBeenCalledOnce()); + + await user.click(screen.getByRole('button', { name: '← Back' })); + await user.click(screen.getByText('Configure manually')); + + await act(async () => { + rejectSubmit?.(new Error('save failed')); + await Promise.resolve(); + }); + + expect(screen.getByLabelText(/API Key/)).toHaveValue(''); + expect(screen.queryByDisplayValue('Bearer manual-secret')).not.toBeInTheDocument(); + expect(screen.queryByDisplayValue('Bearer pending-secret')).not.toBeInTheDocument(); + expect(screen.queryByText('A header with this name already exists')).not.toBeInTheDocument(); + expect(screen.queryByText(/Failed to save provider/)).not.toBeInTheDocument(); + }); + + it('clears form validation when returning to the setup choice', async () => { + const user = userEvent.setup(); + renderForm(); + await user.click(screen.getByText('Configure manually')); + await user.click(screen.getByRole('button', { name: 'Create Provider' })); + expect(screen.getByText('Display name is required')).toBeInTheDocument(); + + await user.click(screen.getByRole('button', { name: '← Back' })); + await user.click(screen.getByText('Configure manually')); + + expect(screen.queryByText('Display name is required')).not.toBeInTheDocument(); + }); +}); diff --git a/ui/desktop/src/components/settings/providers/modal/subcomponents/forms/CustomProviderForm.tsx b/ui/desktop/src/components/settings/providers/modal/subcomponents/forms/CustomProviderForm.tsx index 087ff54ed..1c0b656ab 100644 --- a/ui/desktop/src/components/settings/providers/modal/subcomponents/forms/CustomProviderForm.tsx +++ b/ui/desktop/src/components/settings/providers/modal/subcomponents/forms/CustomProviderForm.tsx @@ -1,4 +1,4 @@ -import React, { useState, useEffect } from 'react'; +import React, { useState, useEffect, useRef } from 'react'; import { Input } from '../../../../../ui/input'; import { Select } from '../../../../../ui/Select'; import { Button } from '../../../../../ui/button'; @@ -264,11 +264,24 @@ export default function CustomProviderForm({ const [validationErrors, setValidationErrors] = useState>({}); const [submitError, setSubmitError] = useState(null); const [showDeleteConfirmation, setShowDeleteConfirmation] = useState(false); + const contextVersionRef = useRef(0); // Template + step state const [selectedTemplate, setSelectedTemplate] = useState(null); const [step, setStep] = useState(initialData ? 'form' : 'choice'); + const clearSensitiveState = () => { + contextVersionRef.current += 1; + setApiKey(''); + setHeaders([]); + setNewHeaderKey(''); + setNewHeaderValue(''); + setHeaderValidationError(null); + setInvalidHeaderFields({ key: false, value: false }); + setValidationErrors({}); + setSubmitError(null); + }; + useEffect(() => { if (initialData) { const engineMap: Record = { @@ -297,6 +310,7 @@ export default function CustomProviderForm({ }, [initialData]); const handleTemplateSelect = (template: ProviderTemplateDto) => { + clearSensitiveState(); setSelectedTemplate(template); // Prefill fields from template @@ -320,6 +334,7 @@ export default function CustomProviderForm({ }; const handleClearTemplate = () => { + clearSensitiveState(); setSelectedTemplate(null); setDisplayName(''); setApiUrl(''); @@ -331,6 +346,16 @@ export default function CustomProviderForm({ setStep('choice'); }; + const handleBackToChoice = () => { + clearSensitiveState(); + setStep('choice'); + }; + + const handleCancel = () => { + clearSensitiveState(); + onCancel(); + }; + const handleRequiresAuthChange = (checked: boolean) => { setRequiresAuth(checked); if (!checked) { @@ -406,6 +431,7 @@ export default function CustomProviderForm({ const handleSubmit = async (e: React.FormEvent) => { e.preventDefault(); + const contextVersion = contextVersionRef.current; setSubmitError(null); setValidationErrors({}); @@ -464,6 +490,7 @@ export default function CustomProviderForm({ base_path: basePath || undefined, }); } catch (error) { + if (contextVersionRef.current !== contextVersion) return; console.error('Failed to save custom provider:', error); setSubmitError(intl.formatMessage(i18n.submitError)); } @@ -522,7 +549,7 @@ export default function CustomProviderForm({
-
@@ -534,12 +561,12 @@ export default function CustomProviderForm({ if (step === 'catalog') { return (
- +
- -
@@ -587,7 +614,7 @@ export default function CustomProviderForm({ {/* Back to choice (create without template only) */} {!initialData && !selectedTemplate && ( - )} @@ -952,7 +979,7 @@ export default function CustomProviderForm({ {intl.formatMessage(i18n.deleteProvider)} )} -