fix(security): harden LLM API key handling across settings and LLM endpoints

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<String, serde_json::Value> = 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<Option<&Arc<dyn SecretsStore + Send + Sync>>, 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<serde_json::Value, StatusCode> {
-    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<serde_json::Value, StatusCode> {
-    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);
+    }
 }

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<String>,
+    /// 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<String>,
+    /// `"builtin"` or `"custom"` — determines the secret name prefix.
+    #[serde(default)]
+    provider_type: Option<String>,
     /// 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<TestConnectionRequest>,
+    State(state): State<Arc<GatewayState>>,
+    AuthenticatedUser(user): AuthenticatedUser,
+    Json(mut body): Json<TestConnectionRequest>,
 ) -> Json<TestConnectionResponse> {
+    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<String>,
+    provider_id: &Option<String>,
+    provider_type: &Option<String>,
+) {
+    // 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<String>,
+    #[serde(default)]
+    provider_id: Option<String>,
+    #[serde(default)]
+    provider_type: Option<String>,
 }
 
 #[derive(serde::Serialize)]
@@ -2805,7 +2869,19 @@ struct ListModelsResponse {
     message: String,
 }
 
-async fn llm_list_models_handler(Json(body): Json<ListModelsRequest>) -> Json<ListModelsResponse> {
+async fn llm_list_models_handler(
+    State(state): State<Arc<GatewayState>>,
+    AuthenticatedUser(user): AuthenticatedUser,
+    Json(mut body): Json<ListModelsRequest>,
+) -> Json<ListModelsResponse> {
+    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<serde_json::Value> {
+async fn llm_env_defaults_handler(
+    AuthenticatedUser(_user): AuthenticatedUser,
+) -> Json<serde_json::Value> {
+    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<serde_json::Value> {
         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.)

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');