diff --git a/crates/buzz-agent/src/config.rs b/crates/buzz-agent/src/config.rs index 3a8b82cc4..e20a97650 100644 --- a/crates/buzz-agent/src/config.rs +++ b/crates/buzz-agent/src/config.rs @@ -905,7 +905,9 @@ impl Config { parse_openai_api(env("OPENAI_COMPAT_API").as_deref())?, ), Provider::OpenAiCompat => ( - env("OPENAI_COMPAT_API_KEY").unwrap_or_default(), + env("OPENAI_COMPAT_API_KEY") + .map(|value| value.trim().to_string()) + .unwrap_or_default(), resolve_model( buzz_agent_model.as_deref(), env("OPENAI_COMPAT_MODEL").as_deref(), @@ -1171,14 +1173,22 @@ fn resolve_provider( } fn parse_openai_compat_base_url(raw: Option<&str>) -> Result { + const INVALID_URL: &str = + "config: OPENAI_COMPAT_BASE_URL must be an HTTP(S) URL without credentials, query, or fragment"; + let value = raw .map(str::trim) .filter(|value| !value.is_empty()) .ok_or_else(|| "config: OPENAI_COMPAT_BASE_URL required for openai-compat".to_string())?; - let parsed = url::Url::parse(value) - .map_err(|_| "config: OPENAI_COMPAT_BASE_URL must be a valid HTTP(S) URL".to_string())?; - if !matches!(parsed.scheme(), "http" | "https") || parsed.host_str().is_none() { - return Err("config: OPENAI_COMPAT_BASE_URL must be a valid HTTP(S) URL".to_string()); + let parsed = url::Url::parse(value).map_err(|_| INVALID_URL.to_string())?; + if !matches!(parsed.scheme(), "http" | "https") + || parsed.host_str().is_none() + || !parsed.username().is_empty() + || parsed.password().is_some() + || parsed.query().is_some() + || parsed.fragment().is_some() + { + return Err(INVALID_URL.to_string()); } Ok(value.trim_end_matches('/').to_string()) } @@ -1493,9 +1503,19 @@ mod tests { assert!(parse_openai_compat_base_url(None) .unwrap_err() .contains("required for openai-compat")); - assert!(parse_openai_compat_base_url(Some("ftp://localhost/v1")) - .unwrap_err() - .contains("valid HTTP(S) URL")); + for invalid in [ + "ftp://localhost/v1", + "http://user:secret@localhost/v1", + "http://localhost/v1?tenant=x", + "http://localhost/v1#fragment", + ] { + assert!( + parse_openai_compat_base_url(Some(invalid)) + .unwrap_err() + .contains("without credentials, query, or fragment"), + "invalid={invalid}" + ); + } assert_eq!( parse_openai_compat_base_url(Some(" http://localhost:11434/v1/// ")).unwrap(), "http://localhost:11434/v1" diff --git a/desktop/src-tauri/src/commands/agent_models.rs b/desktop/src-tauri/src/commands/agent_models.rs index d5552fbfb..c08fb3583 100644 --- a/desktop/src-tauri/src/commands/agent_models.rs +++ b/desktop/src-tauri/src/commands/agent_models.rs @@ -12,7 +12,7 @@ use super::managed_agent_definition::apply_model_provider_prompt_update; use super::agent_models_env::env_value; use super::agent_models_env::{ effective_discovery_provider, env_or_process_override, env_or_process_value, - redaction_env_with_value, DiscoveryProvider, + redaction_env_with_value, validate_openai_compat_base_url, DiscoveryProvider, }; use super::agent_update_rollback::{rollback_failed_agent_update, AgentUpdateRollback}; @@ -367,19 +367,17 @@ fn openai_compatible_models_url_for_discovery( provider: Option<&str>, env: &BTreeMap, ) -> Result { + let is_compat = + provider.is_some_and(|value| value.trim().eq_ignore_ascii_case("openai-compat")); let base_url = env_or_process_value(env, "OPENAI_COMPAT_BASE_URL"); - let base_url = if provider.map(str::trim) == Some("openai-compat") { + let base_url = if is_compat { base_url.ok_or_else(|| { "OPENAI_COMPAT_BASE_URL required for OpenAI-compatible model discovery".to_string() })? } else { base_url.unwrap_or_else(|| "https://api.openai.com/v1".to_string()) }; - let parsed = url::Url::parse(base_url.trim()) - .map_err(|_| "OPENAI_COMPAT_BASE_URL must be a valid HTTP(S) URL".to_string())?; - if !matches!(parsed.scheme(), "http" | "https") || parsed.host_str().is_none() { - return Err("OPENAI_COMPAT_BASE_URL must be a valid HTTP(S) URL".to_string()); - } + validate_openai_compat_base_url(&base_url)?; Ok(format!("{}/models", base_url.trim().trim_end_matches('/'))) } @@ -511,7 +509,9 @@ async fn discover_openai_compatible_models( return Ok(None); } - let is_compat = provider.as_deref().map(str::trim) == Some("openai-compat"); + let is_compat = provider + .as_deref() + .is_some_and(|value| value.trim().eq_ignore_ascii_case("openai-compat")); let api_key = if relay_mesh { crate::managed_agents::RELAY_MESH_API_KEY_PLACEHOLDER.to_string() } else if is_compat { diff --git a/desktop/src-tauri/src/commands/agent_models_env.rs b/desktop/src-tauri/src/commands/agent_models_env.rs index 9ce8e7ab0..8950e7708 100644 --- a/desktop/src-tauri/src/commands/agent_models_env.rs +++ b/desktop/src-tauri/src/commands/agent_models_env.rs @@ -39,6 +39,22 @@ pub(super) fn env_or_process_override(env: &BTreeMap, key: &str) }) } +pub(super) fn validate_openai_compat_base_url(value: &str) -> Result<(), String> { + const INVALID_URL: &str = + "OPENAI_COMPAT_BASE_URL must be an HTTP(S) URL without credentials, query, or fragment"; + let parsed = url::Url::parse(value.trim()).map_err(|_| INVALID_URL.to_string())?; + if !matches!(parsed.scheme(), "http" | "https") + || parsed.host_str().is_none() + || !parsed.username().is_empty() + || parsed.password().is_some() + || parsed.query().is_some() + || parsed.fragment().is_some() + { + return Err(INVALID_URL.to_string()); + } + Ok(()) +} + /// Clone `env` with `key` set to the value a request actually used, so error /// redaction masks the inherited process value and not just the mapped one. pub(super) fn redaction_env_with_value( @@ -126,3 +142,23 @@ pub(super) fn effective_discovery_provider( inferred: true, } } + +#[cfg(test)] +mod tests { + use super::validate_openai_compat_base_url; + + #[test] + fn rejects_ambiguous_openai_compat_url_components() { + for value in [ + "http://user:secret@localhost/v1", + "http://localhost/v1?tenant=x", + "http://localhost/v1#fragment", + ] { + let error = validate_openai_compat_base_url(value).unwrap_err(); + assert!( + error.contains("without credentials, query, or fragment"), + "value={value} error={error}" + ); + } + } +} diff --git a/desktop/src-tauri/src/managed_agents/readiness.rs b/desktop/src-tauri/src/managed_agents/readiness.rs index ff73820b8..0d81485b9 100644 --- a/desktop/src-tauri/src/managed_agents/readiness.rs +++ b/desktop/src-tauri/src/managed_agents/readiness.rs @@ -460,8 +460,8 @@ fn buzz_agent_requirements(effective: &EffectiveAgentEnv) -> Vec { let provider = effective .env .get("BUZZ_AGENT_PROVIDER") - .filter(|v| !v.is_empty()) - .map(String::as_str); + .map(|value| value.trim().to_ascii_lowercase()) + .filter(|value| !value.is_empty()); if provider.is_none() { missing.push(Requirement::NormalizedField { field: "provider".to_string(), @@ -475,7 +475,7 @@ fn buzz_agent_requirements(effective: &EffectiveAgentEnv) -> Vec { // databricks/databricks_v2, ANTHROPIC_MODEL for anthropic, etc.). The // baked buzz-releases env sets DATABRICKS_MODEL but not BUZZ_AGENT_MODEL, // so without this fallback agents baked from releases appear "not ready". - let provider_model_key = match provider { + let provider_model_key = match provider.as_deref() { Some("databricks") | Some("databricks_v2") | Some("databricks-v2") => { Some("DATABRICKS_MODEL") } @@ -503,7 +503,7 @@ fn buzz_agent_requirements(effective: &EffectiveAgentEnv) -> Vec { // A key present with an empty value is treated as absent — matching the // dialog's (envVars[key] ?? "").length === 0 emptiness check. let env_key_missing = |key: &str| effective.env.get(key).is_none_or(|v| v.is_empty()); - match provider { + match provider.as_deref() { Some("anthropic") if env_key_missing("ANTHROPIC_API_KEY") => { missing.push(Requirement::EnvKey { @@ -566,16 +566,17 @@ fn goose_requirements( let provider = effective .env .get("GOOSE_PROVIDER") - .filter(|v| !v.is_empty()) - .map(String::as_str); + .map(|value| value.trim().to_ascii_lowercase()) + .filter(|value| !value.is_empty()); // Effective provider for credential checking: prefer env layer, then file. - let effective_provider = provider.or_else(|| { - file_cfg - .as_ref() - .and_then(|c| c.provider.as_deref()) - .filter(|v| !v.is_empty()) - }); + let file_provider = file_cfg + .as_ref() + .and_then(|config| config.provider.as_deref()) + .map(str::trim) + .filter(|value| !value.is_empty()) + .map(str::to_ascii_lowercase); + let effective_provider = provider.as_deref().or(file_provider.as_deref()); if provider.is_none() { // Silenced if the file config provides a provider. @@ -783,7 +784,7 @@ mod tests { let with_url = make_env( "buzz-agent", env_with(&[ - ("BUZZ_AGENT_PROVIDER", "openai-compat"), + ("BUZZ_AGENT_PROVIDER", " OpenAI-Compat "), ("BUZZ_AGENT_MODEL", "llama3"), ("OPENAI_COMPAT_BASE_URL", "http://localhost:11434/v1"), ]), diff --git a/desktop/src/features/agents/AGENTS.md b/desktop/src/features/agents/AGENTS.md index 7accd54af..c52cbec86 100644 --- a/desktop/src/features/agents/AGENTS.md +++ b/desktop/src/features/agents/AGENTS.md @@ -105,7 +105,11 @@ with a TypeScript lookup table or an id comparison in a component. `onboarding-agent-defaults.spec.ts`. 9. **The defaults modal is progressively disclosed.** An unset global config starts on the Buzz Agent-first deployment fallback and carries that visible - harness into the next saved edit. The `progressive-defaults` disclosure + harness into the next saved edit. Provider changes preserve typed API keys + in Advanced, but `OPENAI_COMPAT_BASE_URL` is provider-owned routing state and + must be cleared whenever a defaults/onboarding transition leaves + `openai-compat`; otherwise official OpenAI discovery inherits the stale + custom endpoint. The `progressive-defaults` disclosure preset therefore begins at Provider for Buzz Agent, then reveals Model, Effort, and Advanced only after a provider is configured. Harnesses whose runtime metadata has no provider field skip that gate. Reveals animate their diff --git a/desktop/src/features/agents/ui/AgentConfigFields.tsx b/desktop/src/features/agents/ui/AgentConfigFields.tsx index 198765324..232ad385e 100644 --- a/desktop/src/features/agents/ui/AgentConfigFields.tsx +++ b/desktop/src/features/agents/ui/AgentConfigFields.tsx @@ -37,7 +37,6 @@ import { CARD_MINT_KEY_ANNOTATIONS, CUSTOM_PROVIDER_DROPDOWN_VALUE, getPersonaProviderOptions, - getProviderApiKeyEnvVar, getProviderApiKeyLabel, runtimeSupportsLlmProviderSelection, } from "@/features/agents/ui/agentConfigOptions"; @@ -67,6 +66,7 @@ import { SettingsOptionGroup } from "@/features/settings/ui/SettingsOptionGroup" import { AdvancedRequiredBadge } from "./AdvancedRequiredBadge"; import { CardMintKeyCue } from "./CardMintKeyCue"; import { getGlobalAgentCredentialState } from "./globalAgentCredentialState"; +import { envVarsClearingOpenAiCompatBaseUrl } from "./providerEnvVarUpdates"; export const EMPTY_GLOBAL_CONFIG: GlobalAgentConfig = { env_vars: {}, @@ -95,7 +95,9 @@ type AgentConfigDisclosure = // - auto-select a valid model when the provider changes // - keep the model select usable during discovery // - preserve credential env vars across provider switches (the abandoned -// provider's key stays in env_vars — visible/deletable under Advanced) +// provider's key stays in env_vars — visible/deletable under Advanced), except +// provider-owned endpoint metadata is cleared when it would affect the next +// provider. // - require a provider before model/effort are editable (no saveable // invalid state — design principle #4) const autoSelectModelOnProviderChange = true; @@ -520,29 +522,23 @@ export function AgentConfigFields({ }); function handleProviderChange(value: string) { userEditedProviderRef.current = true; - const previousApiKey = getProviderApiKeyEnvVar(effectiveProvider); if (value === CUSTOM_PROVIDER_DROPDOWN_VALUE) { - const nextEnvVars = { ...config.env_vars }; - if (!preserveCredentialEnvVarsOnProviderChange && previousApiKey) { - delete nextEnvVars[previousApiKey]; - } + const nextEnvVars = envVarsClearingOpenAiCompatBaseUrl( + config.env_vars, + effectiveProvider, + "", + ); onIsCustomProviderChange(true); onConfigChange({ ...config, env_vars: nextEnvVars, provider: null }); return; } const nextProvider = value === AUTO_PROVIDER_DROPDOWN_VALUE || value === "" ? null : value; - const nextApiKey = getProviderApiKeyEnvVar( + const nextEnvVars = envVarsClearingOpenAiCompatBaseUrl( + config.env_vars, + effectiveProvider, nextProvider ?? bakedProvider ?? "", ); - const nextEnvVars = { ...config.env_vars }; - if ( - !preserveCredentialEnvVarsOnProviderChange && - previousApiKey && - previousApiKey !== nextApiKey - ) { - delete nextEnvVars[previousApiKey]; - } const providerChanged = nextProvider !== (config.provider ?? null); onIsCustomProviderChange(false); onConfigChange({ diff --git a/desktop/src/features/agents/ui/OpenAiCompatibleBaseUrlField.test.mjs b/desktop/src/features/agents/ui/OpenAiCompatibleBaseUrlField.test.mjs index cf8c54205..011425611 100644 --- a/desktop/src/features/agents/ui/OpenAiCompatibleBaseUrlField.test.mjs +++ b/desktop/src/features/agents/ui/OpenAiCompatibleBaseUrlField.test.mjs @@ -3,19 +3,23 @@ import test from "node:test"; import { openAiCompatibleBaseUrlError } from "./OpenAiCompatibleBaseUrlField.tsx"; -test("OpenAI-compatible base URL accepts only absolute HTTP(S) URLs", () => { +test("OpenAI-compatible base URL accepts only safe absolute HTTP(S) URLs", () => { + const invalidMessage = + "Enter an HTTP or HTTPS URL without credentials, query, or fragment."; + assert.equal(openAiCompatibleBaseUrlError("http://localhost:11434/v1"), null); assert.equal( openAiCompatibleBaseUrlError(" https://models.example/v1/ "), null, ); assert.equal(openAiCompatibleBaseUrlError(""), "Base URL is required."); - assert.equal( - openAiCompatibleBaseUrlError("ftp://models.example/v1"), - "Enter a valid HTTP or HTTPS URL.", - ); - assert.equal( - openAiCompatibleBaseUrlError("localhost:11434/v1"), - "Enter a valid HTTP or HTTPS URL.", - ); + for (const value of [ + "ftp://models.example/v1", + "localhost:11434/v1", + "https://user:secret@models.example/v1", + "https://models.example/v1?tenant=x", + "https://models.example/v1#fragment", + ]) { + assert.equal(openAiCompatibleBaseUrlError(value), invalidMessage, value); + } }); diff --git a/desktop/src/features/agents/ui/OpenAiCompatibleBaseUrlField.tsx b/desktop/src/features/agents/ui/OpenAiCompatibleBaseUrlField.tsx index c1a087a17..0829b7edb 100644 --- a/desktop/src/features/agents/ui/OpenAiCompatibleBaseUrlField.tsx +++ b/desktop/src/features/agents/ui/OpenAiCompatibleBaseUrlField.tsx @@ -8,18 +8,26 @@ import { export const OPENAI_COMPAT_BASE_URL = "OPENAI_COMPAT_BASE_URL"; +function isSafeHttpBaseUrl(url: URL): boolean { + return ( + (url.protocol === "http:" || url.protocol === "https:") && + url.host.length > 0 && + url.username.length === 0 && + url.password.length === 0 && + url.search.length === 0 && + url.hash.length === 0 + ); +} + export function openAiCompatibleBaseUrlError(value: string): string | null { const trimmed = value.trim(); if (trimmed.length === 0) return "Base URL is required."; try { - const url = new URL(trimmed); - if ((url.protocol === "http:" || url.protocol === "https:") && url.host) { - return null; - } + if (isSafeHttpBaseUrl(new URL(trimmed))) return null; } catch { // Fall through to the actionable validation message. } - return "Enter a valid HTTP or HTTPS URL."; + return "Enter an HTTP or HTTPS URL without credentials, query, or fragment."; } export function OpenAiCompatibleBaseUrlField({ diff --git a/desktop/src/features/agents/ui/providerEnvVarUpdates.test.mjs b/desktop/src/features/agents/ui/providerEnvVarUpdates.test.mjs index 6bce6a7a9..e2956f8a4 100644 --- a/desktop/src/features/agents/ui/providerEnvVarUpdates.test.mjs +++ b/desktop/src/features/agents/ui/providerEnvVarUpdates.test.mjs @@ -3,6 +3,7 @@ import test from "node:test"; import { envVarsClearingManagedApiKey, + envVarsClearingOpenAiCompatBaseUrl, envVarsWithoutKey, } from "./providerEnvVarUpdates.ts"; @@ -35,6 +36,45 @@ test("envVarsClearingManagedApiKey clears when leaving to a custom/empty provide assert.deepEqual(next, {}); }); +test("envVarsClearingOpenAiCompatBaseUrl clears only the compatible URL", () => { + const current = { + OPENAI_COMPAT_API_KEY: "sk-1", + OPENAI_COMPAT_BASE_URL: "http://localhost:11434/v1", + KEEP: "x", + }; + assert.deepEqual( + envVarsClearingOpenAiCompatBaseUrl(current, " OpenAI-Compat ", "openai"), + { + OPENAI_COMPAT_API_KEY: "sk-1", + KEEP: "x", + }, + ); + assert.equal( + envVarsClearingOpenAiCompatBaseUrl( + current, + "openai-compat", + "OPENAI-COMPAT", + ), + current, + ); +}); + +test("envVarsClearingManagedApiKey clears the compatible URL when leaving compat", () => { + const next = envVarsClearingManagedApiKey( + { + OPENAI_COMPAT_API_KEY: "sk-1", + OPENAI_COMPAT_BASE_URL: "http://localhost:11434/v1", + KEEP: "x", + }, + "openai-compat", + "openai", + ); + assert.deepEqual(next, { + OPENAI_COMPAT_API_KEY: "sk-1", + KEEP: "x", + }); +}); + test("envVarsClearingManagedApiKey is a no-op when the managed key is shared or absent", () => { const current = { ANTHROPIC_API_KEY: "sk-1" }; assert.equal( diff --git a/desktop/src/features/agents/ui/providerEnvVarUpdates.ts b/desktop/src/features/agents/ui/providerEnvVarUpdates.ts index df4c89fad..be2dc7e81 100644 --- a/desktop/src/features/agents/ui/providerEnvVarUpdates.ts +++ b/desktop/src/features/agents/ui/providerEnvVarUpdates.ts @@ -1,6 +1,12 @@ import type { EnvVarsValue } from "./EnvVarsEditor"; import { getProviderApiKeyEnvVar } from "./agentConfigOptions"; +const OPENAI_COMPAT_BASE_URL = "OPENAI_COMPAT_BASE_URL"; + +function normalizedProvider(provider: string): string { + return provider.trim().toLowerCase(); +} + /** * Pure env-var update helpers shared by the persona / create-agent / * edit-agent dialogs. Every function returns the SAME reference when nothing @@ -21,10 +27,24 @@ export function envVarsWithoutKey( return next; } +/** Remove the compatible endpoint when switching away from openai-compat. */ +export function envVarsClearingOpenAiCompatBaseUrl( + current: EnvVarsValue, + previousProvider: string, + nextProvider: string, +): EnvVarsValue { + if ( + normalizedProvider(previousProvider) === "openai-compat" && + normalizedProvider(nextProvider) !== "openai-compat" + ) { + return envVarsWithoutKey(current, OPENAI_COMPAT_BASE_URL); + } + return current; +} + /** - * Clear the previous provider's managed API key when switching providers. - * No-op when the previous provider has no managed key or the next provider - * uses the same one. + * Clear the previous provider's managed API key and provider-owned endpoint + * when switching providers. */ export function envVarsClearingManagedApiKey( current: EnvVarsValue, @@ -33,8 +53,14 @@ export function envVarsClearingManagedApiKey( ): EnvVarsValue { const previousEnvVar = getProviderApiKeyEnvVar(previousProvider); const nextEnvVar = getProviderApiKeyEnvVar(nextProvider); + let next = current; if (previousEnvVar && previousEnvVar !== nextEnvVar) { - return envVarsWithoutKey(current, previousEnvVar); + next = envVarsWithoutKey(next, previousEnvVar); } - return current; + next = envVarsClearingOpenAiCompatBaseUrl( + next, + previousProvider, + nextProvider, + ); + return next; } diff --git a/desktop/src/features/agents/ui/runtimeModelProviderSelection.ts b/desktop/src/features/agents/ui/runtimeModelProviderSelection.ts index e98dc540d..89fdd1808 100644 --- a/desktop/src/features/agents/ui/runtimeModelProviderSelection.ts +++ b/desktop/src/features/agents/ui/runtimeModelProviderSelection.ts @@ -8,10 +8,7 @@ import { shouldClearKnownModelForSelectionScope, } from "./agentConfigOptions"; import { shouldClearModelForRuntimeChange } from "./personaRuntimeModel"; -import { - envVarsClearingManagedApiKey, - envVarsWithoutKey, -} from "./providerEnvVarUpdates"; +import { envVarsClearingManagedApiKey } from "./providerEnvVarUpdates"; /** * Pure transition functions for the runtime -> LLM provider -> model dropdown @@ -92,10 +89,11 @@ export function selectionOnProviderDropdownChange( const next = { ...current }; if (params.nextValue === CUSTOM_PROVIDER_DROPDOWN_VALUE) { - const previousEnvVar = getProviderApiKeyEnvVar(current.provider); - if (previousEnvVar) { - next.envVars = envVarsWithoutKey(next.envVars, previousEnvVar); - } + next.envVars = envVarsClearingManagedApiKey( + next.envVars, + current.provider, + "", + ); next.isCustomProviderEditing = true; next.provider = ""; return next;