From c6ce3e48039076112886939ecb9cf970ac0c4925 Mon Sep 17 00:00:00 2001 From: mARTin-B78 Date: Fri, 4 Sep 2026 07:50:30 +0200 Subject: [PATCH] security: keep provider credentials server-side --- CHANGELOG.md | 3 +++ TODO.md | 8 +++++--- routes/docker.py | 13 +++++++++++-- routes/settings.py | 35 ++++++++++++++++++++++++++++++++--- static/js/ai-backends.js | 22 +++++++++++----------- static/js/engines.js | 12 ++++-------- static/js/settings.js | 35 ++++++++++++++++++++--------------- static/js/utils.js | 6 ++---- tests/test_security.py | 30 ++++++++++++++++++++++++++++++ 9 files changed, 118 insertions(+), 46 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 14ea418..9571c9b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,9 @@ Follows [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) · versioned wi ### Changed - **API Boundary Hardening.** External API authentication is enabled by default, request and mutation limits are enforced, and settings responses redact provider credentials for non-browser callers. +- **Server-side Provider Credentials.** Provider keys are now masked even for same-origin Settings responses, are + preserved when masked/empty fields are saved, and model/health probes resolve configured keys inside the server. + Browser localStorage is no longer used as a credential store. - **Security Regression Coverage.** Added a Python test gate for character metadata validation, private-download URL rejection and oversized JSON request rejection. - **Navigation Reliability.** Rehearser phase requests now survive the feature-module initialization boundary. diff --git a/TODO.md b/TODO.md index 11b2e11..824e3e8 100644 --- a/TODO.md +++ b/TODO.md @@ -17,8 +17,9 @@ the first block before investing in architecture or polish. ### P0 — do first -- [ ] **S — Stop API keys from reaching frontend JavaScript or logs.** Audit responses, - browser storage, error paths and debug output. +- [ ] **S — Stop API keys from reaching frontend JavaScript or logs.** Provider keys are now masked in settings + responses and probes use server-side credentials; finish the external-client key handoff and audit remaining + transient input/error paths. - [ ] **S — Validate uploaded filenames and paths.** Reject traversal, unsafe names and writes outside the intended data directories. - [ ] **M — Add upload, request-size, temporary-file and concurrent-job limits.** Apply @@ -124,7 +125,8 @@ the first block before investing in architecture or polish. ## P0 — Security -- [ ] Never expose API keys to frontend JavaScript or logs. +- [ ] Never expose API keys to frontend JavaScript or logs. Provider keys are masked on read and absent from browser + storage; the deliberate external-client key display and transient entry path still need a final design review. - [ ] Validate uploaded filenames and paths. - [ ] Strictly validate custom URLs to prevent SSRF. - [ ] Add request-size limits, rate limits, and authentication for destructive/API endpoints. diff --git a/routes/docker.py b/routes/docker.py index 64978ab..4e59b2d 100644 --- a/routes/docker.py +++ b/routes/docker.py @@ -10,6 +10,7 @@ import requests from fastapi import APIRouter, Header, HTTPException from core.docker_client import _docker_get_json, _docker_post +from core.config import _load_settings from core.validation import _normalize_service_url router = APIRouter() @@ -127,7 +128,14 @@ async def probe_url(url: str, type: str = "", provider_api_key: str = Header("", if base.endswith(_suffix): base = base[: -len(_suffix)] break - key_to_use = provider_api_key if provider_api_key else "sk-dummy-key" + settings = _load_settings() + key_to_use = { + "anthropic": settings.get("llm_api_key"), + "llm": settings.get("llm_api_key"), + "openai": settings.get("llm_api_key"), + "stt": settings.get("groq_api_key") or settings.get("whisper_api_key"), + "tts": settings.get("tts_api_key"), + }.get(type) or provider_api_key or "sk-dummy-key" # Anthropic doesn't speak the OpenAI-style Bearer auth every other card # here uses — it needs x-api-key + an anthropic-version header, so the # generic Bearer probe below would 401 even with a perfectly valid key. @@ -192,7 +200,8 @@ async def engine_models(url: str, type: str = "", provider_api_key: str = Header last_err = "No response" seen: set[str] = set() models: list[str] = [] - key_to_use = provider_api_key if provider_api_key else "sk-dummy-key" + settings = _load_settings() + key_to_use = settings.get("llm_api_key") or provider_api_key or "sk-dummy-key" for endpoint in candidates: try: r = requests.get(endpoint, timeout=6, headers={"Authorization": f"Bearer {key_to_use}", "User-Agent": "TTS-Voice-Creator/models"}) diff --git a/routes/settings.py b/routes/settings.py index f6a90a0..7f03da8 100644 --- a/routes/settings.py +++ b/routes/settings.py @@ -15,6 +15,7 @@ _SECRET_SETTING_KEYS = { "groq_api_key", "whisper_api_key", "tts_api_key", "voice_design_api_key", "elevenlabs_api_key", "llm_api_key", "engine_api_keys", "external_api_key", } +_SECRET_MASK = "__configured__" @router.get("/api/settings") @@ -34,8 +35,17 @@ async def get_settings(request: Request): ) except Exception: same_origin = False - if not same_origin: - s = {key: value for key, value in s.items() if key not in _SECRET_SETTING_KEYS} + # Never return provider credentials to browser JavaScript. The mask lets + # the Settings UI indicate that a value exists without making it usable in + # client-side requests or browser storage. + for key in _SECRET_SETTING_KEYS: + value = s.get(key) + if key == "engine_api_keys" and isinstance(value, dict): + s[key] = {name: _SECRET_MASK for name, secret in value.items() if secret} + elif value: + s[key] = _SECRET_MASK + else: + s.pop(key, None) return s @@ -52,7 +62,26 @@ async def regenerate_api_key(): async def post_settings(request: Request): data = await request.json() s = _load_settings() - s.update({k: v for k, v in data.items() if k in _SETTINGS_KEYS}) + clear = {str(k) for k in (data.get("clear_secrets") or [])} if isinstance(data.get("clear_secrets"), list) else set() + for key, value in data.items(): + if key not in _SETTINGS_KEYS: + continue + if key in _SECRET_SETTING_KEYS: + if value == _SECRET_MASK: + continue + if key == "engine_api_keys" and isinstance(value, dict): + old = s.get(key) if isinstance(s.get(key), dict) else {} + merged = dict(old) + for name, secret in value.items(): + if secret and secret != _SECRET_MASK: + merged[name] = secret + for name in clear: + merged.pop(name, None) + s[key] = merged + continue + if value in (None, "") and key not in clear: + continue + s[key] = value s = _normalize_settings(s) _save_settings(s) return {"ok": True} diff --git a/static/js/ai-backends.js b/static/js/ai-backends.js index d616ce8..93bf07d 100644 --- a/static/js/ai-backends.js +++ b/static/js/ai-backends.js @@ -19,13 +19,11 @@ }); }); - // API key inputs — persist to localStorage, eye toggle, saved badge + // API key inputs — save only to the server, eye toggle, saved badge document.querySelectorAll('.llm-input[data-llm-key]').forEach(inp => { const key = inp.dataset.llmKey; const storageKey = 'llm-key-' + key; - const saved = typeof window.engineApiKeyValue === 'function' - ? window.engineApiKeyValue(key, storageKey) - : localStorage.getItem(storageKey); + const saved = typeof window.engineApiKeyValue === 'function' ? window.engineApiKeyValue(key) : ''; if (saved) inp.value = saved; const eye = document.createElement('button'); @@ -47,8 +45,8 @@ clearTimeout(t); t = setTimeout(() => { if (typeof window.saveEngineApiKey === 'function') window.saveEngineApiKey(key, inp.value, storageKey); - else if (inp.value) localStorage.setItem(storageKey, inp.value); - else localStorage.removeItem(storageKey); + // No browser-storage fallback: provider credentials belong on the + // server and must not survive in localStorage. badge.hidden = false; setTimeout(() => { badge.hidden = true; }, 1800); }, 600); @@ -142,8 +140,7 @@ const url = normalizeProbeUrl(rawUrl); const params = { url }; if (type) params.type = type; - const headers = apiKey ? { 'X-Provider-API-Key': apiKey } : {}; - const r = await fetch('/api/probe-url?' + new URLSearchParams(params), { headers }); + const r = await fetch('/api/probe-url?' + new URLSearchParams(params)); return r.json(); } @@ -207,7 +204,11 @@ const resp = await fetch('/api/settings').then(r => r.json()); const updated = { ...resp, ...patch }; await fetch('/api/settings', { method: 'POST', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify(patch) }); - Object.assign(_appSettings || {}, patch); + if (_appSettings) { + const visiblePatch = { ...patch }; + ['groq_api_key','whisper_api_key','tts_api_key','voice_design_api_key','elevenlabs_api_key','llm_api_key','engine_api_keys','external_api_key'].forEach(k => delete visiblePatch[k]); + Object.assign(_appSettings, visiblePatch); + } // refresh visible inputs in Settings if open for (const [id, val] of Object.entries(patch)) { const inp = $('s-' + id.replace(/_/g, '-')); @@ -310,8 +311,7 @@ if (refreshBtn) refreshBtn.disabled = true; try { const fetchUrl = '/api/conversation/llm-models?url=' + encodeURIComponent(url); - const headers = apiKey ? { 'X-Provider-API-Key': apiKey } : {}; - const d = await fetch(fetchUrl, { headers }).then(r => r.json()); + const d = await fetch(fetchUrl).then(r => r.json()); if (d.error) { let msg = 'Failed'; try { diff --git a/static/js/engines.js b/static/js/engines.js index 459ca7b..a64f71d 100644 --- a/static/js/engines.js +++ b/static/js/engines.js @@ -522,7 +522,7 @@ function renderLocalContainers(containers) { // API key row — only for LLMs const savedApiKey = isCustom ? ((typeof window.engineApiKeyValue === 'function' ? window.engineApiKeyValue(c.name, 'dc-apikey-' + c.name) : '') || c.apiKey || '') - : ((typeof window.engineApiKeyValue === 'function' ? window.engineApiKeyValue(c.name, 'dc-apikey-' + c.name) : '') || (_appSettings?.engine_api_keys?.[c.name]) || localStorage.getItem('dc-apikey-' + n) || ''); + : ((typeof window.engineApiKeyValue === 'function' ? window.engineApiKeyValue(c.name) : '') || ''); const apiKeyRowHtml = (c.role === 'llm' || c.name === 'litellm') ? `
@@ -673,21 +673,18 @@ function renderLocalContainers(containers) { } }); - // API key input — save to server-backed engine_api_keys and localStorage fallback + // API key input — save only to server-backed engine_api_keys grid.querySelectorAll('.dc-apikey-inp[data-dc-apikey-key]').forEach(inp => { const key = inp.dataset.dcApikeyKey; const storageKey = 'dc-apikey-' + key; - const saved = typeof window.engineApiKeyValue === 'function' - ? window.engineApiKeyValue(key, storageKey) - : localStorage.getItem(storageKey); + const saved = typeof window.engineApiKeyValue === 'function' ? window.engineApiKeyValue(key) : ''; if (saved && inp.value !== saved) inp.value = saved; let keyTimer; inp.addEventListener('input', () => { clearTimeout(keyTimer); keyTimer = setTimeout(() => { if (typeof window.saveEngineApiKey === 'function') window.saveEngineApiKey(key, inp.value, storageKey); - else if (inp.value) localStorage.setItem(storageKey, inp.value); - else localStorage.removeItem(storageKey); + // No browser-storage fallback for provider credentials. }, 300); }); }); @@ -727,7 +724,6 @@ function renderLocalContainers(containers) { const url = inp?.value.trim() || inp?.placeholder || ''; if (settingKey && url) { const patch = { [settingKey]: url }; - if (apiKey && settingKey === 'llm_url') patch.llm_api_key = apiKey; await window.applyAndSaveSettings(patch); toast(`✓ Connected and set as active ${useBtn.textContent.replace('Use as ', '').trim()}`, 'success'); } else { diff --git a/static/js/settings.js b/static/js/settings.js index 4068540..2356958 100644 --- a/static/js/settings.js +++ b/static/js/settings.js @@ -239,7 +239,11 @@ async function refreshTtsBackendAvailability(selected = '') { async function _patchSettings(patch) { try { await fetch('/api/settings', { method: 'POST', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify(patch) }); - if (_appSettings) Object.assign(_appSettings, patch || {}); + if (_appSettings) { + const visiblePatch = { ...(patch || {}) }; + ['groq_api_key','whisper_api_key','tts_api_key','voice_design_api_key','elevenlabs_api_key','llm_api_key','engine_api_keys','external_api_key'].forEach(k => delete visiblePatch[k]); + Object.assign(_appSettings, visiblePatch); + } if (typeof window.refreshStatusBarEngines === 'function') window.refreshStatusBarEngines({ force: true }); } catch (e) { console.warn('[settings] patch failed', e); } } @@ -282,24 +286,24 @@ const ENGINE_API_KEY_SETTING_MAP = { const _engineApiKeySaveTimers = {}; function engineApiKeyValue(key, localStorageKey = '') { const settingKey = ENGINE_API_KEY_SETTING_MAP[key]; - const fromSetting = settingKey && _appSettings ? (_appSettings[settingKey] || '') : ''; - const fromMap = _appSettings?.engine_api_keys?.[key] || ''; - const fromLocal = localStorageKey ? localStorage.getItem(localStorageKey) : ''; - return fromSetting || fromMap || fromLocal || localStorage.getItem('llm-key-' + key) || localStorage.getItem('dc-apikey-' + key) || ''; + const fromSetting = settingKey && _appSettings && _appSettings[settingKey] !== '__configured__' ? (_appSettings[settingKey] || '') : ''; + const fromMap = _appSettings?.engine_api_keys?.[key] && _appSettings.engine_api_keys[key] !== '__configured__' ? _appSettings.engine_api_keys[key] : ''; + return fromSetting || fromMap || ''; } function saveEngineApiKey(key, value, localStorageKey = '') { const val = String(value || '').trim(); - if (localStorageKey) { - if (val) localStorage.setItem(localStorageKey, val); - else localStorage.removeItem(localStorageKey); - } const apiKeys = { ...(_appSettings?.engine_api_keys || {}) }; if (val) apiKeys[key] = val; else delete apiKeys[key]; const patch = { engine_api_keys: apiKeys }; const settingKey = ENGINE_API_KEY_SETTING_MAP[key]; if (settingKey) patch[settingKey] = val; - if (_appSettings) Object.assign(_appSettings, patch); + if (_appSettings) { + if (settingKey) _appSettings[settingKey] = val ? '__configured__' : undefined; + const safeMap = { ...(_appSettings.engine_api_keys || {}) }; + if (val) safeMap[key] = '__configured__'; else delete safeMap[key]; + _appSettings.engine_api_keys = safeMap; + } clearTimeout(_engineApiKeySaveTimers[key]); _engineApiKeySaveTimers[key] = setTimeout(() => _patchSettings(patch), 600); } @@ -495,12 +499,13 @@ function _populateImageGenModelSelect(selectEl, provider, savedModel) { async function loadSettings() { const s = await fetch('/api/settings').then(r => r.json()); const sv = (id, val) => { const el = $(id); if (el) el.value = val; }; + const svSecret = (id, val) => { const el = $(id); if (el) { el.value = ''; el.placeholder = val === '__configured__' ? 'Configured — enter a new key to replace' : ''; } }; sv('s-whisper-url', s.whisper_url || ''); - sv('s-whisper-key', s.whisper_api_key || ''); + svSecret('s-whisper-key', s.whisper_api_key); sv('s-tts-url', s.tts_url || ''); sv('s-faster-whisper-url', s.faster_whisper_url || ''); sv('s-whisper-cpp-url', s.whisper_cpp_url || ''); - sv('s-groq-api-key', s.groq_api_key || ''); + svSecret('s-groq-api-key', s.groq_api_key); sv('s-kokoro-url', s.kokoro_url || ''); sv('s-vibevoice-url', s.vibevoice_url || ''); sv('s-xtts-url', s.xtts_url || ''); @@ -542,7 +547,7 @@ async function loadSettings() { sv('s-nvidia-zeroshot-url', s.nvidia_zeroshot_url || s.nvidia_clone_url || 'http://host.docker.internal:8093'); sv('s-nvidia-flow-url', s.nvidia_flow_url || 'http://host.docker.internal:8094'); sv('s-tts-stream-mode', s.tts_stream_mode || 'auto'); - sv('s-tts-key', s.tts_api_key || ''); + svSecret('s-tts-key', s.tts_api_key); sv('s-tts-backend', s.tts_backend || 'openai'); const defaultTtsParams = {temperature:0.1, top_p:0.8, seed:0}; const byBackend = s.tts_extra_params_by_backend || {}; @@ -557,12 +562,12 @@ async function loadSettings() { sv('s-tts-extra-kokoro', JSON.stringify(byBackend.kokoro || {}, null, 2)); sv('s-tts-extra-vibevoice', JSON.stringify(byBackend.vibevoice || {}, null, 2)); sv('s-voice-design-url', s.voice_design_url || 'http://host.docker.internal:8021'); - sv('s-vd-key', s.voice_design_api_key || ''); + svSecret('s-vd-key', s.voice_design_api_key); sv('s-voices-scan-dir', s.voices_scan_dir || ''); sv('s-output-dir', s.output_dir || ''); restoreEngineApiKeyInputs(); const extKeyInp = $('s-external-api-key'); - if (extKeyInp) extKeyInp.value = s.external_api_key || ''; + if (extKeyInp) { extKeyInp.value = ''; extKeyInp.placeholder = s.external_api_key === '__configured__' ? 'Configured — regenerate to replace' : ''; } const extKeyReq = $('s-external-api-key-required'); if (extKeyReq) extKeyReq.checked = !!s.external_api_key_required; if (extKeyReq && !extKeyReq.dataset.wired) { diff --git a/static/js/utils.js b/static/js/utils.js index 565b12a..2f525a7 100644 --- a/static/js/utils.js +++ b/static/js/utils.js @@ -249,8 +249,7 @@ async function _statusOpenLlmFlyup(anchor) { let models = []; try { const fetchUrl = '/api/conversation/llm-models?url=' + encodeURIComponent(url); - const headers = apiKey ? { 'X-Provider-API-Key': apiKey } : {}; - const data = await fetch(fetchUrl, { headers }).then(r => r.json()); + const data = await fetch(fetchUrl).then(r => r.json()); models = Array.isArray(data.models) ? data.models : []; } catch (_) {} if (!_statusFlyup || _statusFlyup.dataset.forKind !== 'llm') return; // closed/switched while awaiting @@ -319,8 +318,7 @@ async function refreshStatusBarEngines({ force = false } = {}) { STATUS_LLM_CACHE.pending = true; try { const fetchUrl = '/api/conversation/llm-models?url=' + encodeURIComponent(url); - const headers = apiKey ? { 'X-Provider-API-Key': apiKey } : {}; - const data = await fetch(fetchUrl, { headers }).then(r => r.json()); + const data = await fetch(fetchUrl).then(r => r.json()); const models = Array.isArray(data.models) ? data.models : []; STATUS_LLM_CACHE.url = url; STATUS_LLM_CACHE.model = model; diff --git a/tests/test_security.py b/tests/test_security.py index f90fe2d..14537ad 100644 --- a/tests/test_security.py +++ b/tests/test_security.py @@ -11,6 +11,7 @@ os.environ.setdefault("CONFIG_DIR", tempfile.mkdtemp(prefix="ttvc-test-config-") from fastapi import HTTPException from core.validation import _validate_character_record, _validate_http_url +from core.config import _load_settings, _save_settings from server import RequestSizeLimitMiddleware @@ -48,6 +49,35 @@ class SecurityValidationTests(unittest.TestCase): self.assertEqual(sent[0]["status"], 413) self.assertNotIn({"type": "downstream"}, sent) + def test_settings_never_returns_provider_credentials(self): + from fastapi.testclient import TestClient + from server import app + settings = _load_settings() + settings.update({ + "llm_api_key": "provider-secret", + "engine_api_keys": {"local": "engine-secret"}, + }) + _save_settings(settings) + with TestClient(app) as client: + response = client.get("/api/settings", headers={"origin": "http://testserver"}) + self.assertEqual(response.status_code, 200) + payload = response.json() + self.assertEqual(payload.get("llm_api_key"), "__configured__") + self.assertEqual(payload.get("engine_api_keys", {}).get("local"), "__configured__") + self.assertNotIn("provider-secret", response.text) + self.assertNotIn("engine-secret", response.text) + + def test_empty_masked_secret_does_not_overwrite_saved_value(self): + from fastapi.testclient import TestClient + from server import app + settings = _load_settings() + settings["llm_api_key"] = "keep-this-secret" + _save_settings(settings) + with TestClient(app) as client: + response = client.post("/api/settings", json={"llm_api_key": ""}, headers={"origin": "http://testserver"}) + self.assertEqual(response.status_code, 200) + self.assertEqual(_load_settings().get("llm_api_key"), "keep-this-secret") + if __name__ == "__main__": unittest.main()