From e8e0379e14dca3f30616dd388104b9c8f3fb18ed Mon Sep 17 00:00:00 2001 From: italic-jinxin <106428113+italic-jinxin@users.noreply.github.com> Date: Wed, 25 Mar 2026 19:04:03 +0800 Subject: [PATCH] fix(security): harden LLM API key handling across settings and LLM endpoints --- src/channels/web/handlers/settings.rs | 168 +++++++++++++++++++++++--- src/channels/web/server.rs | 98 +++++++++++++-- src/channels/web/static/app.js | 21 +++- 3 files changed, 261 insertions(+), 26 deletions(-) diff --git a/src/channels/web/handlers/settings.rs b/src/channels/web/handlers/settings.rs index ade5fe13..d52edf27 100644 --- a/src/channels/web/handlers/settings.rs +++ b/src/channels/web/handlers/settings.rs @@ -30,12 +30,34 @@ pub async fn settings_list_handler( StatusCode::INTERNAL_SERVER_ERROR })?; + // Build a map of sensitive keys so we can annotate and mask them. + let sensitive_keys = ["llm_builtin_overrides", "llm_custom_providers"]; + let mut sensitive_map: std::collections::HashMap = rows + .iter() + .filter(|r| sensitive_keys.contains(&r.key.as_str())) + .map(|r| (r.key.clone(), r.value.clone())) + .collect(); + if !sensitive_map.is_empty() { + annotate_secret_key_presence(&state, &user.user_id, &mut sensitive_map).await; + mask_settings_api_keys(&mut sensitive_map); + } + let settings = rows .into_iter() - .map(|r| SettingResponse { - key: r.key, - value: r.value, - updated_at: r.updated_at.to_rfc3339(), + .map(|r| { + let value = if sensitive_keys.contains(&r.key.as_str()) { + sensitive_map + .get(&r.key) + .cloned() + .unwrap_or(r.value.clone()) + } else { + r.value + }; + SettingResponse { + key: r.key, + value, + updated_at: r.updated_at.to_rfc3339(), + } }) .collect(); @@ -60,9 +82,22 @@ pub async fn settings_get_handler( })? .ok_or(StatusCode::NOT_FOUND)?; + // Mask any plaintext API keys that may exist from legacy data. + let value = if matches!( + key.as_str(), + "llm_builtin_overrides" | "llm_custom_providers" + ) { + let mut map = std::collections::HashMap::from([(key.clone(), row.value.clone())]); + annotate_secret_key_presence(&state, &user.user_id, &mut map).await; + mask_settings_api_keys(&mut map); + map.remove(&key).unwrap_or(row.value) + } else { + row.value + }; + Ok(Json(SettingResponse { key: row.key, - value: row.value, + value, updated_at: row.updated_at.to_rfc3339(), })) } @@ -241,8 +276,21 @@ pub async fn settings_import_handler( .store .as_ref() .ok_or(StatusCode::SERVICE_UNAVAILABLE)?; + + // Vault any API keys present in the imported settings, same as the + // individual SET handler does, so plaintext keys never reach the DB. + let mut sanitized = body.settings.clone(); + if let Some(v) = sanitized.get("llm_builtin_overrides").cloned() { + let clean = extract_builtin_override_keys(&state, &user.user_id, &v).await?; + sanitized.insert("llm_builtin_overrides".to_string(), clean); + } + if let Some(v) = sanitized.get("llm_custom_providers").cloned() { + let clean = extract_custom_provider_keys(&state, &user.user_id, &v).await?; + sanitized.insert("llm_custom_providers".to_string(), clean); + } + store - .set_all_settings(&user.user_id, &body.settings) + .set_all_settings(&user.user_id, &sanitized) .await .map_err(|e| { tracing::error!("Failed to import settings: {}", e); @@ -266,19 +314,45 @@ fn custom_secret_name(provider_id: &str) -> String { format!("llm_custom_{}_api_key", provider_id) } +/// Returns true if the `api_key` value is a real key (not sentinel/empty). +fn is_real_api_key(key: &str) -> bool { + !key.is_empty() && key != API_KEY_UNCHANGED +} + +/// Require the secrets store when real API keys are present. +/// Returns `Ok(None)` when no secrets store and no real keys (passthrough). +fn require_secrets_store( + state: &GatewayState, + has_real_keys: bool, +) -> Result>, StatusCode> { + match state.secrets_store.as_ref() { + Some(s) => Ok(Some(s)), + None if has_real_keys => { + tracing::error!("Cannot store API keys: secrets store is not available"); + Err(StatusCode::SERVICE_UNAVAILABLE) + } + None => Ok(None), + } +} + /// Extract API keys from builtin overrides, store in secrets, return sanitized JSON. async fn extract_builtin_override_keys( state: &GatewayState, user_id: &str, value: &serde_json::Value, ) -> Result { - let secrets = match state.secrets_store.as_ref() { - Some(s) => s, + let obj = match value.as_object() { + Some(o) => o, None => return Ok(value.clone()), }; - let obj = match value.as_object() { - Some(o) => o, + let has_real_keys = obj.values().any(|v| { + v.get("api_key") + .and_then(|k| k.as_str()) + .is_some_and(is_real_api_key) + }); + let secrets = match require_secrets_store(state, has_real_keys)? { + Some(s) => s, None => return Ok(value.clone()), }; @@ -286,7 +360,7 @@ async fn extract_builtin_override_keys( for (provider_id, override_val) in obj { if let Some(api_key) = override_val.get("api_key").and_then(|v| v.as_str()) { - if api_key == API_KEY_UNCHANGED || api_key.is_empty() { + if !is_real_api_key(api_key) { // Unchanged or empty — remove from settings, keep existing secret. if let Some(o) = sanitized .get_mut(provider_id) @@ -322,13 +396,18 @@ async fn extract_custom_provider_keys( user_id: &str, value: &serde_json::Value, ) -> Result { - let secrets = match state.secrets_store.as_ref() { - Some(s) => s, + let arr = match value.as_array() { + Some(a) => a, None => return Ok(value.clone()), }; - let arr = match value.as_array() { - Some(a) => a, + let has_real_keys = arr.iter().any(|v| { + v.get("api_key") + .and_then(|k| k.as_str()) + .is_some_and(is_real_api_key) + }); + let secrets = match require_secrets_store(state, has_real_keys)? { + Some(s) => s, None => return Ok(value.clone()), }; @@ -344,7 +423,7 @@ async fn extract_custom_provider_keys( } if let Some(api_key) = provider_val.get("api_key").and_then(|v| v.as_str()) { - if api_key == API_KEY_UNCHANGED || api_key.is_empty() { + if !is_real_api_key(api_key) { if let Some(o) = sanitized[idx].as_object_mut() { o.remove("api_key"); } @@ -709,4 +788,61 @@ mod tests { .unwrap(); assert_eq!(decrypted.expose(), "sk-original"); } + + /// When secrets store is unavailable, attempting to save a real API key + /// must fail with 503 rather than silently storing plaintext. + #[tokio::test] + async fn test_extract_builtin_keys_rejects_without_secrets_store() { + let state = GatewayState { + secrets_store: None, + ..test_gateway_state(test_secrets_store()) + }; + + let input = serde_json::json!({ + "openai": { "api_key": "sk-real-key", "model": "gpt-4" } + }); + + let err = extract_builtin_override_keys(&state, "test", &input) + .await + .unwrap_err(); + assert_eq!(err, StatusCode::SERVICE_UNAVAILABLE); + } + + /// When secrets store is unavailable but no real keys are present + /// (only sentinels or no api_key at all), the call should succeed. + #[tokio::test] + async fn test_extract_builtin_keys_allows_no_keys_without_secrets_store() { + let state = GatewayState { + secrets_store: None, + ..test_gateway_state(test_secrets_store()) + }; + + let input = serde_json::json!({ + "openai": { "api_key": "••••••••", "model": "gpt-4" }, + "anthropic": { "model": "claude-3" } + }); + + let result = extract_builtin_override_keys(&state, "test", &input) + .await + .unwrap(); + // Without secrets store, the value passes through unchanged (no vaulting needed). + assert!(result.as_object().is_some()); + } + + #[tokio::test] + async fn test_extract_custom_keys_rejects_without_secrets_store() { + let state = GatewayState { + secrets_store: None, + ..test_gateway_state(test_secrets_store()) + }; + + let input = serde_json::json!([ + { "id": "my-llm", "api_key": "gsk-real-key", "adapter": "open_ai_completions" } + ]); + + let err = extract_custom_provider_keys(&state, "test", &input) + .await + .unwrap_err(); + assert_eq!(err, StatusCode::SERVICE_UNAVAILABLE); + } } diff --git a/src/channels/web/server.rs b/src/channels/web/server.rs index 16eb14da..08b937b0 100644 --- a/src/channels/web/server.rs +++ b/src/channels/web/server.rs @@ -2610,12 +2610,23 @@ async fn routines_runs_handler( }))) } +/// Fields shared by `test_connection` and `list_models` requests. +/// +/// When `api_key` is absent the handler falls back to the encrypted secrets +/// store, using `provider_id` + `provider_type` to locate the vaulted key. #[derive(serde::Deserialize)] struct TestConnectionRequest { adapter: String, base_url: String, #[serde(default)] api_key: Option, + /// Provider identifier used to look up the vaulted API key when `api_key` + /// is not supplied by the frontend (key already stored in secrets). + #[serde(default)] + provider_id: Option, + /// `"builtin"` or `"custom"` — determines the secret name prefix. + #[serde(default)] + provider_type: Option, /// Accepted for backward compatibility with frontends that still send it, /// but no longer used since test_connection switched to `GET /models`. #[serde(default)] @@ -2630,11 +2641,51 @@ struct TestConnectionResponse { } async fn llm_test_connection_handler( - Json(body): Json, + State(state): State>, + AuthenticatedUser(user): AuthenticatedUser, + Json(mut body): Json, ) -> Json { + resolve_api_key_from_secrets( + &state, + &user.user_id, + &mut body.api_key, + &body.provider_id, + &body.provider_type, + ) + .await; Json(test_provider_connection(body).await) } +/// When the frontend doesn't supply an `api_key` (because it was already vaulted), +/// look it up from the encrypted secrets store using `provider_id` + `provider_type`. +async fn resolve_api_key_from_secrets( + state: &GatewayState, + user_id: &str, + api_key: &mut Option, + provider_id: &Option, + provider_type: &Option, +) { + // Already have a key from the request — nothing to resolve. + if api_key.as_ref().is_some_and(|k| !k.is_empty()) { + return; + } + let pid = match provider_id.as_deref().filter(|s| !s.is_empty()) { + Some(id) => id, + None => return, + }; + let secrets = match state.secrets_store.as_ref() { + Some(s) => s, + None => return, + }; + let secret_name = match provider_type.as_deref() { + Some("custom") => format!("llm_custom_{}_api_key", pid), + _ => format!("llm_builtin_{}_api_key", pid), + }; + if let Ok(decrypted) = secrets.get_decrypted(user_id, &secret_name).await { + *api_key = Some(decrypted.expose().to_string()); + } +} + /// Check if a base URL belongs to a NEAR AI private endpoint by verifying the /// hostname ends with `.near.ai` and contains "private". This prevents an /// attacker from crafting `https://evil.com/private/...` to match. @@ -2763,14 +2814,23 @@ fn interpret_chat_response( message: format!("Authentication failed ({})", status), } } else if status == reqwest::StatusCode::BAD_REQUEST - || status == reqwest::StatusCode::NOT_FOUND || status == reqwest::StatusCode::UNPROCESSABLE_ENTITY { - // 400/404/422 = server reachable, likely wrong model name or endpoint variant — still a success for connectivity + // 400/422 = server reachable, likely wrong endpoint variant — connectivity OK TestConnectionResponse { ok: true, message: format!("Server reachable ({})", status), } + } else if status == reqwest::StatusCode::NOT_FOUND { + // 404 = /models endpoint not found — server reachable but not OpenAI-compatible + TestConnectionResponse { + ok: false, + message: format!( + "Server reachable but /models endpoint not found ({}). \ + Check the base URL and adapter type.", + status + ), + } } else if status.is_client_error() { TestConnectionResponse { ok: false, @@ -2796,6 +2856,10 @@ struct ListModelsRequest { base_url: String, #[serde(default)] api_key: Option, + #[serde(default)] + provider_id: Option, + #[serde(default)] + provider_type: Option, } #[derive(serde::Serialize)] @@ -2805,7 +2869,19 @@ struct ListModelsResponse { message: String, } -async fn llm_list_models_handler(Json(body): Json) -> Json { +async fn llm_list_models_handler( + State(state): State>, + AuthenticatedUser(user): AuthenticatedUser, + Json(mut body): Json, +) -> Json { + resolve_api_key_from_secrets( + &state, + &user.user_id, + &mut body.api_key, + &body.provider_id, + &body.provider_type, + ) + .await; Json(fetch_provider_models(body).await) } @@ -2944,7 +3020,13 @@ async fn fetch_provider_models(req: ListModelsRequest) -> ListModelsResponse { /// /// The frontend uses these as fallback values when the DB has no overrides. /// API keys are never returned — only a boolean `has_api_key`. -async fn llm_env_defaults_handler() -> Json { +async fn llm_env_defaults_handler( + AuthenticatedUser(_user): AuthenticatedUser, +) -> Json { + Json(build_llm_env_defaults()) +} + +fn build_llm_env_defaults() -> serde_json::Value { use crate::config::helpers::optional_env; use crate::llm::registry::ProviderRegistry; @@ -2995,7 +3077,7 @@ async fn llm_env_defaults_handler() -> Json { defaults.insert(def.id.clone(), serde_json::Value::Object(entry)); } - Json(serde_json::Value::Object(defaults)) + serde_json::Value::Object(defaults) } // --- Gateway control plane handlers --- @@ -3254,7 +3336,7 @@ mod tests { std::env::set_var("NEARAI_BASE_URL", "https://test.near.ai/v1"); } - let Json(result) = llm_env_defaults_handler().await; + let result = build_llm_env_defaults(); let map = result.as_object().expect("should be an object"); // Check NEAR AI entry @@ -3290,7 +3372,7 @@ mod tests { #[tokio::test] async fn test_llm_env_defaults_includes_registry_providers() { - let Json(result) = llm_env_defaults_handler().await; + let result = build_llm_env_defaults(); let map = result.as_object().expect("should be an object"); // Registry providers should be present (openai, anthropic, ollama, etc.) diff --git a/src/channels/web/static/app.js b/src/channels/web/static/app.js index 9dac47bb..43013010 100644 --- a/src/channels/web/static/app.js +++ b/src/channels/web/static/app.js @@ -6483,9 +6483,18 @@ document.getElementById('test-provider-btn').addEventListener('click', () => { result.style.display = 'none'; result.className = 'test-connection-result'; + // Resolve provider_id so the backend can look up vaulted API keys. + const providerId = _configuringBuiltinId || document.getElementById('provider-id').value.trim(); + apiFetch('/api/llm/test_connection', { method: 'POST', - body: { adapter, base_url: baseUrl, api_key: apiKey || undefined, model: model || undefined }, + body: { + adapter, base_url: baseUrl, + api_key: apiKey || undefined, + model: model || undefined, + provider_id: providerId || undefined, + provider_type: _configuringBuiltinId ? 'builtin' : 'custom', + }, }) .then((data) => { result.textContent = data.message; @@ -6680,9 +6689,17 @@ document.getElementById('fetch-models-btn').addEventListener('click', () => { btn.disabled = true; btn.textContent = I18n.t('config.fetchingModels'); + // Resolve provider_id so the backend can look up vaulted API keys. + const providerId = _configuringBuiltinId || document.getElementById('provider-id').value.trim(); + apiFetch('/api/llm/list_models', { method: 'POST', - body: { adapter, base_url: baseUrl, api_key: apiKey || undefined }, + body: { + adapter, base_url: baseUrl, + api_key: apiKey || undefined, + provider_id: providerId || undefined, + provider_type: _configuringBuiltinId ? 'builtin' : 'custom', + }, }) .then((data) => { const select = document.getElementById('provider-model-select');