Fix provider API key validation trimming (#810)
This commit is contained in:
committed by
GitHub
Unverified
parent
66b2ddb2dc
commit
49518300dc
@@ -34,6 +34,7 @@ import {
|
|||||||
getProviderDocsUrl,
|
getProviderDocsUrl,
|
||||||
type ProviderType,
|
type ProviderType,
|
||||||
getProviderIconUrl,
|
getProviderIconUrl,
|
||||||
|
normalizeProviderApiKeyInput,
|
||||||
resolveProviderApiKeyForSave,
|
resolveProviderApiKeyForSave,
|
||||||
resolveProviderModelForSave,
|
resolveProviderModelForSave,
|
||||||
shouldShowProviderModelId,
|
shouldShowProviderModelId,
|
||||||
@@ -424,10 +425,11 @@ function ProviderCard({
|
|||||||
try {
|
try {
|
||||||
const payload: { newApiKey?: string; updates?: Partial<ProviderConfig> } = {};
|
const payload: { newApiKey?: string; updates?: Partial<ProviderConfig> } = {};
|
||||||
const normalizedFallbackModels = normalizeFallbackModels(fallbackModelsText.split('\n'));
|
const normalizedFallbackModels = normalizeFallbackModels(fallbackModelsText.split('\n'));
|
||||||
|
const normalizedNewKey = normalizeProviderApiKeyInput(newKey);
|
||||||
|
|
||||||
if (newKey.trim()) {
|
if (normalizedNewKey) {
|
||||||
setValidating(true);
|
setValidating(true);
|
||||||
const result = await onValidateKey(newKey, {
|
const result = await onValidateKey(normalizedNewKey, {
|
||||||
baseUrl: baseUrl.trim() || undefined,
|
baseUrl: baseUrl.trim() || undefined,
|
||||||
apiProtocol: (account.vendorId === 'custom' || account.vendorId === 'ollama') ? apiProtocol : undefined,
|
apiProtocol: (account.vendorId === 'custom' || account.vendorId === 'ollama') ? apiProtocol : undefined,
|
||||||
});
|
});
|
||||||
@@ -437,7 +439,7 @@ function ProviderCard({
|
|||||||
setSaving(false);
|
setSaving(false);
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
payload.newApiKey = newKey.trim();
|
payload.newApiKey = normalizedNewKey;
|
||||||
}
|
}
|
||||||
|
|
||||||
{
|
{
|
||||||
@@ -1164,13 +1166,14 @@ function AddProviderDialog({
|
|||||||
try {
|
try {
|
||||||
// Validate key first if the provider requires one and a key was entered
|
// Validate key first if the provider requires one and a key was entered
|
||||||
const requiresKey = typeInfo?.requiresApiKey ?? false;
|
const requiresKey = typeInfo?.requiresApiKey ?? false;
|
||||||
if (requiresKey && !apiKey.trim()) {
|
const normalizedApiKey = normalizeProviderApiKeyInput(apiKey);
|
||||||
|
if (requiresKey && !normalizedApiKey) {
|
||||||
setValidationError(t('aiProviders.toast.invalidKey')); // reusing invalid key msg or should add 'required' msg? null checks
|
setValidationError(t('aiProviders.toast.invalidKey')); // reusing invalid key msg or should add 'required' msg? null checks
|
||||||
setSaving(false);
|
setSaving(false);
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
if (requiresKey && apiKey) {
|
if (requiresKey && normalizedApiKey) {
|
||||||
const result = await onValidateKey(selectedType, apiKey, {
|
const result = await onValidateKey(selectedType, normalizedApiKey, {
|
||||||
baseUrl: baseUrl.trim() || undefined,
|
baseUrl: baseUrl.trim() || undefined,
|
||||||
apiProtocol: (selectedType === 'custom' || selectedType === 'ollama') ? apiProtocol : undefined,
|
apiProtocol: (selectedType === 'custom' || selectedType === 'ollama') ? apiProtocol : undefined,
|
||||||
});
|
});
|
||||||
@@ -1191,7 +1194,7 @@ function AddProviderDialog({
|
|||||||
await onAdd(
|
await onAdd(
|
||||||
selectedType,
|
selectedType,
|
||||||
name || (typeInfo?.id === 'custom' ? t('aiProviders.custom') : typeInfo?.name) || selectedType,
|
name || (typeInfo?.id === 'custom' ? t('aiProviders.custom') : typeInfo?.name) || selectedType,
|
||||||
apiKey.trim(),
|
normalizedApiKey,
|
||||||
{
|
{
|
||||||
baseUrl: baseUrl.trim() || undefined,
|
baseUrl: baseUrl.trim() || undefined,
|
||||||
apiProtocol: (selectedType === 'custom' || selectedType === 'ollama') ? apiProtocol : undefined,
|
apiProtocol: (selectedType === 'custom' || selectedType === 'ollama') ? apiProtocol : undefined,
|
||||||
@@ -1655,6 +1658,7 @@ function AddProviderDialog({
|
|||||||
|
|
||||||
<div className="flex justify-end gap-3">
|
<div className="flex justify-end gap-3">
|
||||||
<Button
|
<Button
|
||||||
|
data-testid="add-provider-submit-button"
|
||||||
onClick={handleAdd}
|
onClick={handleAdd}
|
||||||
className={cn("rounded-full px-8 h-[42px] text-[13px] font-semibold bg-[#0a84ff] hover:bg-[#007aff] text-white shadow-sm", useOAuthFlow && "hidden")}
|
className={cn("rounded-full px-8 h-[42px] text-[13px] font-semibold bg-[#0a84ff] hover:bg-[#007aff] text-white shadow-sm", useOAuthFlow && "hidden")}
|
||||||
disabled={!selectedType || saving || (showModelIdField && modelId.trim().length === 0)}
|
disabled={!selectedType || saving || (showModelIdField && modelId.trim().length === 0)}
|
||||||
|
|||||||
@@ -246,9 +246,13 @@ export function resolveProviderModelForSave(
|
|||||||
return trimmedModelId || provider?.defaultModelId || undefined;
|
return trimmedModelId || provider?.defaultModelId || undefined;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
export function normalizeProviderApiKeyInput(apiKey: string): string {
|
||||||
|
return apiKey.trim();
|
||||||
|
}
|
||||||
|
|
||||||
/** Normalize provider API key before saving; Ollama uses a local placeholder when blank. */
|
/** Normalize provider API key before saving; Ollama uses a local placeholder when blank. */
|
||||||
export function resolveProviderApiKeyForSave(type: ProviderType | string, apiKey: string): string | undefined {
|
export function resolveProviderApiKeyForSave(type: ProviderType | string, apiKey: string): string | undefined {
|
||||||
const trimmed = apiKey.trim();
|
const trimmed = normalizeProviderApiKeyInput(apiKey);
|
||||||
if (type === 'ollama') {
|
if (type === 'ollama') {
|
||||||
return trimmed || OLLAMA_PLACEHOLDER_API_KEY;
|
return trimmed || OLLAMA_PLACEHOLDER_API_KEY;
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -99,6 +99,7 @@ import {
|
|||||||
type ProviderTypeInfo,
|
type ProviderTypeInfo,
|
||||||
getProviderDocsUrl,
|
getProviderDocsUrl,
|
||||||
getProviderIconUrl,
|
getProviderIconUrl,
|
||||||
|
normalizeProviderApiKeyInput,
|
||||||
resolveProviderApiKeyForSave,
|
resolveProviderApiKeyForSave,
|
||||||
resolveProviderModelForSave,
|
resolveProviderModelForSave,
|
||||||
shouldInvertInDark,
|
shouldInvertInDark,
|
||||||
@@ -1009,6 +1010,7 @@ function ProviderContent({
|
|||||||
const isOAuth = selectedProviderData?.isOAuth ?? false;
|
const isOAuth = selectedProviderData?.isOAuth ?? false;
|
||||||
const supportsApiKey = selectedProviderData?.supportsApiKey ?? false;
|
const supportsApiKey = selectedProviderData?.supportsApiKey ?? false;
|
||||||
const useOAuthFlow = isOAuth && (!supportsApiKey || authMode === 'oauth');
|
const useOAuthFlow = isOAuth && (!supportsApiKey || authMode === 'oauth');
|
||||||
|
const normalizedApiKey = normalizeProviderApiKeyInput(apiKey);
|
||||||
|
|
||||||
const handleValidateAndSave = async () => {
|
const handleValidateAndSave = async () => {
|
||||||
if (!selectedProvider) return;
|
if (!selectedProvider) return;
|
||||||
@@ -1034,11 +1036,19 @@ function ProviderContent({
|
|||||||
try {
|
try {
|
||||||
// Validate key if the provider requires one and a key was entered
|
// Validate key if the provider requires one and a key was entered
|
||||||
const isApiKeyRequired = requiresKey || (supportsApiKey && authMode === 'apikey');
|
const isApiKeyRequired = requiresKey || (supportsApiKey && authMode === 'apikey');
|
||||||
if (isApiKeyRequired && apiKey) {
|
if (isApiKeyRequired && !normalizedApiKey) {
|
||||||
|
setKeyValid(false);
|
||||||
|
onConfiguredChange(false);
|
||||||
|
toast.error(t('provider.invalid'));
|
||||||
|
setValidating(false);
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
|
if (isApiKeyRequired) {
|
||||||
const result = await invokeIpc(
|
const result = await invokeIpc(
|
||||||
'provider:validateKey',
|
'provider:validateKey',
|
||||||
selectedAccountId || selectedProvider,
|
selectedAccountId || selectedProvider,
|
||||||
apiKey,
|
normalizedApiKey,
|
||||||
{
|
{
|
||||||
baseUrl: baseUrl.trim() || undefined,
|
baseUrl: baseUrl.trim() || undefined,
|
||||||
apiProtocol: (selectedProvider === 'custom' || selectedProvider === 'ollama')
|
apiProtocol: (selectedProvider === 'custom' || selectedProvider === 'ollama')
|
||||||
@@ -1146,7 +1156,7 @@ function ProviderContent({
|
|||||||
const isApiKeyRequired = requiresKey || (supportsApiKey && authMode === 'apikey');
|
const isApiKeyRequired = requiresKey || (supportsApiKey && authMode === 'apikey');
|
||||||
const canSubmit =
|
const canSubmit =
|
||||||
selectedProvider
|
selectedProvider
|
||||||
&& (isApiKeyRequired ? apiKey.length > 0 : true)
|
&& (isApiKeyRequired ? normalizedApiKey.length > 0 : true)
|
||||||
&& (showModelIdField ? modelId.trim().length > 0 : true)
|
&& (showModelIdField ? modelId.trim().length > 0 : true)
|
||||||
&& !useOAuthFlow;
|
&& !useOAuthFlow;
|
||||||
|
|
||||||
|
|||||||
@@ -9,6 +9,7 @@ import type {
|
|||||||
ProviderVendorInfo,
|
ProviderVendorInfo,
|
||||||
ProviderWithKeyInfo,
|
ProviderWithKeyInfo,
|
||||||
} from '@/lib/providers';
|
} from '@/lib/providers';
|
||||||
|
import { normalizeProviderApiKeyInput } from '@/lib/providers';
|
||||||
import { hostApiFetch } from '@/lib/host-api';
|
import { hostApiFetch } from '@/lib/host-api';
|
||||||
import {
|
import {
|
||||||
fetchProviderSnapshot,
|
fetchProviderSnapshot,
|
||||||
@@ -326,9 +327,10 @@ export const useProviderStore = create<ProviderState>((set, get) => ({
|
|||||||
|
|
||||||
validateAccountApiKey: async (providerId, apiKey, options) => {
|
validateAccountApiKey: async (providerId, apiKey, options) => {
|
||||||
try {
|
try {
|
||||||
|
const normalizedApiKey = normalizeProviderApiKeyInput(apiKey);
|
||||||
const result = await hostApiFetch<{ valid: boolean; error?: string }>('/api/providers/validate', {
|
const result = await hostApiFetch<{ valid: boolean; error?: string }>('/api/providers/validate', {
|
||||||
method: 'POST',
|
method: 'POST',
|
||||||
body: JSON.stringify({ providerId, apiKey, options }),
|
body: JSON.stringify({ providerId, apiKey: normalizedApiKey, options }),
|
||||||
});
|
});
|
||||||
return result;
|
return result;
|
||||||
} catch (error) {
|
} catch (error) {
|
||||||
|
|||||||
@@ -62,4 +62,83 @@ test.describe('ClawX provider lifecycle', () => {
|
|||||||
await relaunchedApp.close();
|
await relaunchedApp.close();
|
||||||
}
|
}
|
||||||
});
|
});
|
||||||
|
|
||||||
|
test('trims whitespace before validating and saving a custom provider key', async ({ electronApp, page }) => {
|
||||||
|
await completeSetup(page);
|
||||||
|
|
||||||
|
await electronApp.evaluate(async ({ app: _app }) => {
|
||||||
|
const { ipcMain } = process.mainModule!.require('electron') as typeof import('electron');
|
||||||
|
|
||||||
|
let accounts: Array<Record<string, unknown>> = [];
|
||||||
|
let statuses: Array<Record<string, unknown>> = [];
|
||||||
|
let defaultAccountId: string | null = null;
|
||||||
|
|
||||||
|
const respond = (json: unknown, status = 200) => ({
|
||||||
|
ok: true,
|
||||||
|
data: {
|
||||||
|
status,
|
||||||
|
ok: status >= 200 && status < 300,
|
||||||
|
json,
|
||||||
|
},
|
||||||
|
});
|
||||||
|
|
||||||
|
ipcMain.removeHandler('hostapi:fetch');
|
||||||
|
ipcMain.handle('hostapi:fetch', async (_event: unknown, request: { path?: string; method?: string; body?: string | null }) => {
|
||||||
|
const path = request?.path ?? '';
|
||||||
|
const method = request?.method ?? 'GET';
|
||||||
|
const body = request?.body ? JSON.parse(request.body) : null;
|
||||||
|
|
||||||
|
if (path === '/api/provider-accounts' && method === 'GET') return respond(accounts);
|
||||||
|
if (path === '/api/providers' && method === 'GET') return respond(statuses);
|
||||||
|
if (path === '/api/provider-vendors' && method === 'GET') return respond([]);
|
||||||
|
if (path === '/api/provider-accounts/default' && method === 'GET') return respond({ accountId: defaultAccountId });
|
||||||
|
|
||||||
|
if (path === '/api/providers/validate' && method === 'POST') {
|
||||||
|
if (body?.apiKey !== 'sk-lm-test') {
|
||||||
|
return respond({ valid: false, error: `unexpected key: ${String(body?.apiKey)}` }, 400);
|
||||||
|
}
|
||||||
|
return respond({ valid: true });
|
||||||
|
}
|
||||||
|
|
||||||
|
if (path === '/api/provider-accounts' && method === 'POST') {
|
||||||
|
accounts = [body.account];
|
||||||
|
statuses = [{
|
||||||
|
id: body.account.id,
|
||||||
|
name: body.account.label,
|
||||||
|
type: body.account.vendorId,
|
||||||
|
baseUrl: body.account.baseUrl,
|
||||||
|
model: body.account.model,
|
||||||
|
enabled: body.account.enabled,
|
||||||
|
createdAt: body.account.createdAt,
|
||||||
|
updatedAt: body.account.updatedAt,
|
||||||
|
hasKey: Boolean(body.apiKey),
|
||||||
|
keyMasked: body.apiKey ? 'sk-***' : null,
|
||||||
|
}];
|
||||||
|
return respond({ success: true });
|
||||||
|
}
|
||||||
|
|
||||||
|
if (path === '/api/provider-accounts/default' && method === 'PUT') {
|
||||||
|
defaultAccountId = body?.accountId ?? null;
|
||||||
|
return respond({ success: true });
|
||||||
|
}
|
||||||
|
|
||||||
|
return respond({});
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
await page.getByTestId('sidebar-nav-models').click();
|
||||||
|
await expect(page.getByTestId('providers-settings')).toBeVisible();
|
||||||
|
|
||||||
|
await page.getByTestId('providers-add-button').click();
|
||||||
|
await expect(page.getByTestId('add-provider-dialog')).toBeVisible();
|
||||||
|
|
||||||
|
await page.getByTestId('add-provider-type-custom').click();
|
||||||
|
await page.getByTestId('add-provider-name-input').fill('LM Studio Local');
|
||||||
|
await page.getByTestId('add-provider-api-key-input').fill(' sk-lm-test \n');
|
||||||
|
await page.getByTestId('add-provider-base-url-input').fill('http://127.0.0.1:1234/v1');
|
||||||
|
await page.getByTestId('add-provider-model-id-input').fill('local-model');
|
||||||
|
await page.getByTestId('add-provider-submit-button').click();
|
||||||
|
|
||||||
|
await expect(page.getByTestId('provider-card-custom')).toContainText('LM Studio Local');
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
42
tests/unit/provider-store-validation.test.ts
Normal file
42
tests/unit/provider-store-validation.test.ts
Normal file
@@ -0,0 +1,42 @@
|
|||||||
|
import { beforeEach, describe, expect, it, vi } from 'vitest';
|
||||||
|
|
||||||
|
const mockFetchProviderSnapshot = vi.fn();
|
||||||
|
const mockHostApiFetch = vi.fn();
|
||||||
|
|
||||||
|
vi.mock('@/lib/provider-accounts', () => ({
|
||||||
|
fetchProviderSnapshot: (...args: unknown[]) => mockFetchProviderSnapshot(...args),
|
||||||
|
}));
|
||||||
|
|
||||||
|
vi.mock('@/lib/host-api', () => ({
|
||||||
|
hostApiFetch: (...args: unknown[]) => mockHostApiFetch(...args),
|
||||||
|
}));
|
||||||
|
|
||||||
|
import { useProviderStore } from '@/stores/providers';
|
||||||
|
|
||||||
|
describe('useProviderStore – validateAccountApiKey()', () => {
|
||||||
|
beforeEach(() => {
|
||||||
|
vi.clearAllMocks();
|
||||||
|
});
|
||||||
|
|
||||||
|
it('trims API keys before sending provider validation requests', async () => {
|
||||||
|
mockHostApiFetch.mockResolvedValueOnce({ valid: true });
|
||||||
|
|
||||||
|
const result = await useProviderStore.getState().validateAccountApiKey('custom', ' sk-lm-test \n', {
|
||||||
|
baseUrl: 'http://127.0.0.1:1234/v1',
|
||||||
|
apiProtocol: 'openai-completions',
|
||||||
|
});
|
||||||
|
|
||||||
|
expect(result).toEqual({ valid: true });
|
||||||
|
expect(mockHostApiFetch).toHaveBeenCalledWith('/api/providers/validate', {
|
||||||
|
method: 'POST',
|
||||||
|
body: JSON.stringify({
|
||||||
|
providerId: 'custom',
|
||||||
|
apiKey: 'sk-lm-test',
|
||||||
|
options: {
|
||||||
|
baseUrl: 'http://127.0.0.1:1234/v1',
|
||||||
|
apiProtocol: 'openai-completions',
|
||||||
|
},
|
||||||
|
}),
|
||||||
|
});
|
||||||
|
});
|
||||||
|
});
|
||||||
@@ -1,5 +1,6 @@
|
|||||||
import { describe, expect, it } from 'vitest';
|
import { describe, expect, it } from 'vitest';
|
||||||
import {
|
import {
|
||||||
|
normalizeProviderApiKeyInput,
|
||||||
PROVIDER_TYPES,
|
PROVIDER_TYPES,
|
||||||
PROVIDER_TYPE_INFO,
|
PROVIDER_TYPE_INFO,
|
||||||
getProviderDocsUrl,
|
getProviderDocsUrl,
|
||||||
@@ -171,6 +172,7 @@ describe('provider metadata', () => {
|
|||||||
});
|
});
|
||||||
|
|
||||||
it('normalizes provider API keys for save flow', () => {
|
it('normalizes provider API keys for save flow', () => {
|
||||||
|
expect(normalizeProviderApiKeyInput(' sk-test \n')).toBe('sk-test');
|
||||||
expect(resolveProviderApiKeyForSave('ollama', '')).toBe('ollama-local');
|
expect(resolveProviderApiKeyForSave('ollama', '')).toBe('ollama-local');
|
||||||
expect(resolveProviderApiKeyForSave('ollama', ' ')).toBe('ollama-local');
|
expect(resolveProviderApiKeyForSave('ollama', ' ')).toBe('ollama-local');
|
||||||
expect(resolveProviderApiKeyForSave('ollama', 'real-key')).toBe('real-key');
|
expect(resolveProviderApiKeyForSave('ollama', 'real-key')).toBe('real-key');
|
||||||
|
|||||||
Reference in New Issue
Block a user