diff --git a/CHANGELOG.md b/CHANGELOG.md index e72ff9d..4e3ed13 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,18 @@ release PR may merge — and a section is closed at the commit that bumps it. ## [Unreleased] +### Fixed + +- The **Providers** page said *API key missing* on every provider, including the ones + with a perfectly good key. It now reports the real state. Editing a provider no longer + shows the saved key in the form either — leave the field blank and the existing key is + kept, type a new one to replace it. + +### Security + +- An LLM provider's API key is never sent to the browser any more: the provider list and + the edit form receive only whether a key is stored, not its value. + ## [0.3.0] - 2026-08-24 ### Added diff --git a/crates/skald-core/src/llm/manager.rs b/crates/skald-core/src/llm/manager.rs index f9ff1e2..0d736ee 100644 --- a/crates/skald-core/src/llm/manager.rs +++ b/crates/skald-core/src/llm/manager.rs @@ -314,6 +314,7 @@ impl LlmManager { provider: p.provider.clone(), base_url: p.base_url.clone(), description: p.description.clone(), + has_api_key: p.api_key.as_deref().is_some_and(|k| !k.trim().is_empty()), supported_types, } }).collect() diff --git a/crates/skald-core/src/llm/mod.rs b/crates/skald-core/src/llm/mod.rs index 5041a41..d44a23d 100644 --- a/crates/skald-core/src/llm/mod.rs +++ b/crates/skald-core/src/llm/mod.rs @@ -75,7 +75,8 @@ pub fn dtl_mode_from_format(fmt: &str) -> DtlMode { // ── Provider ────────────────────────────────────────────────────────────────── -/// Public provider metadata (no api_key). +/// Public provider metadata. The api_key itself never leaves the server: the UI +/// only needs to know **whether** one is stored, so this carries a boolean. #[derive(Debug, Clone, serde::Serialize)] pub struct LlmProviderInfo { pub id: i64, @@ -84,6 +85,8 @@ pub struct LlmProviderInfo { pub provider: String, pub base_url: Option, pub description: Option, + /// True when a non-empty api_key is stored for this provider. + pub has_api_key: bool, /// Service types this provider supports (from ProviderRegistry at runtime). pub supported_types: Vec, } diff --git a/dev-docs/llm-stack.md b/dev-docs/llm-stack.md index d378a1e..d9abae5 100644 --- a/dev-docs/llm-stack.md +++ b/dev-docs/llm-stack.md @@ -10,6 +10,14 @@ LLM client abstraction (OpenAI-compat, Anthropic, Ollama…). OpenAI-compatible provider *types* are runtime data, not code: `providers/declared.rs` loads `providers.yaml` at boot (see Config in [../CLAUDE.md](../CLAUDE.md)); only non-OpenAI-compatible or bespoke providers (anthropic, ollama, openai, openrouter) stay native. **Retriability** (`Model::is_retriable`, `agent-loop`) keys on the real HTTP status carried by `ModelError { status }`, **not** a substring of the message — a model id/token count containing "404"/"401" cannot mis-classify; 401/403/404/422 don't retry, 400/429/5xx/network do. **Request logging** is the `logging.rs::LoggingModel` decorator, attached by the *caller's* `ModelSelector` (`loop_adapters/selector.rs::SkaldSelector::with_log`) — never by `LlmManager`, which builds one shared client per model and cannot know whose traffic it serves. The decorator's `RequestLogTarget` carries the owner: metadata → `llm_requests` in the registry (`user_id`, the column the UI filters on), payload bodies/headers → `llm_request_payloads` in that user's own encrypted DB, keyed by `request_id`; session + frame come from the request's own `conversation`/`frame`, so kernel rounds, sub-agent frames and compaction summaries are all attributed with no extra plumbing (`ModelRequest::log` is unused here) +## The provider API surface — the key is a boolean, never a value + +**No provider endpoint ever returns a stored `api_key`, and the trap is that omitting it silently reads as "no key".** `LlmProviderInfo` (list) and `ProviderDetail` (`src/frontend/api/llm.rs`, the detail DTO — deliberately *not* `LlmProviderRecord`, which does carry the secret) both expose **`has_api_key: bool`** instead. That is the whole contract: the UI needs to know *whether* a key is on file, never what it is, and the browser is where a leaked key would end up in a devtools tab or a screenshot. + +It shipped broken in exactly the way this shape invites: `list_providers_info` never carried `api_key` (correctly), while the card tested `Boolean(p.api_key)` — always `undefined` — so every provider was badged "API key missing" even with a working key. A missing field is falsy, not an error; nothing logs, nothing fails to build. If you add a provider surface, read `has_api_key`, and if you add a field to either DTO, keep the secret out by construction rather than by remembering to strip it. + +The consequence on the write path is load-bearing: since the edit form can no longer prefill the key, **an empty `api_key` in the `PUT` payload means "keep the stored one"** — `update_provider` re-reads the record and carries the old value over, because a blind `UPDATE … SET api_key=NULL` would wipe a working provider on any unrelated edit (a renamed description). The i18n placeholder (`providers.modal.api_key_ph`) already promised this behaviour before the backend implemented it. Side effect to know about: there is no longer a way to *clear* a key from the form — deleting the provider is the escape hatch. + ## Token streaming & reasoning display The chat streams tokens live, as a **parallel best-effort side-channel** that never alters the turn's authoritative flow: the final `Done` (or `Thinking`) event still carries the complete content and the frontend treats it as truth. diff --git a/src/frontend/api/llm.rs b/src/frontend/api/llm.rs index 37a7deb..a9dd7d4 100644 --- a/src/frontend/api/llm.rs +++ b/src/frontend/api/llm.rs @@ -98,12 +98,43 @@ pub async fn create_provider( Ok(StatusCode::CREATED) } +/// One provider, as the edit form sees it. Deliberately **not** `LlmProviderRecord`: +/// the stored api_key never travels to the browser — the form only needs to know +/// whether one exists, so it can offer "leave blank to keep it". +#[derive(Serialize)] +pub struct ProviderDetail { + pub id: i64, + pub name: String, + #[serde(rename = "type")] + pub provider: String, + pub has_api_key: bool, + pub base_url: Option, + pub description: Option, +} + +impl From for ProviderDetail { + fn from(r: LlmProviderRecord) -> Self { + ProviderDetail { + id: r.id, + name: r.name, + provider: r.provider, + has_api_key: has_key(&r.api_key), + base_url: r.base_url, + description: r.description, + } + } +} + +fn has_key(key: &Option) -> bool { + key.as_deref().is_some_and(|k| !k.trim().is_empty()) +} + pub async fn get_provider( State(skald): State>, axum::extract::Path(id): axum::extract::Path, -) -> Result, ApiError> { +) -> Result, ApiError> { skald.llm_manager().get_provider(id).await - .map(Json) + .map(|r| Json(ProviderDetail::from(r))) .ok_or_else(|| ApiError::not_found(format!("provider {id} not found"))) } @@ -113,7 +144,12 @@ pub async fn update_provider( Json(payload): Json, ) -> Result { validate_provider_type(&skald, &payload.provider)?; - let record = LlmProviderRecord::from(payload); + let mut record = LlmProviderRecord::from(payload); + // The form never receives the stored key, so it cannot send it back: an empty + // api_key means "keep the one on file", not "erase it". + if !has_key(&record.api_key) { + record.api_key = skald.llm_manager().get_provider(id).await.and_then(|r| r.api_key); + } skald.llm_manager().update_provider(id, record).await?; Ok(StatusCode::NO_CONTENT) } diff --git a/web/components/llm-providers.js b/web/components/llm-providers.js index efe9905..e14efd1 100644 --- a/web/components/llm-providers.js +++ b/web/components/llm-providers.js @@ -98,14 +98,15 @@ export class LlmProvidersPage extends LightElement { const res = await fetch(`/api/llm/providers/${provider.id}`); if (!res.ok) throw new Error(`HTTP ${res.status}`); const record = await res.json(); + // The server never sends the stored key back — an empty box means "keep it". this._form = { name: record.name, type: record.type, - api_key: record.api_key ?? '', + api_key: '', base_url: record.base_url ?? '', description: record.description ?? '', }; - this._modal = { mode: 'edit', id: record.id }; + this._modal = { mode: 'edit', id: record.id, hasKey: Boolean(record.has_api_key) }; } catch (e) { this._error = e.message; } @@ -172,7 +173,7 @@ export class LlmProvidersPage extends LightElement { const icon = meta.icon; const label = meta.display_name; const count = this._modelCounts[String(p.id)]; - const hasKey = Boolean(p.api_key); + const hasKey = Boolean(p.has_api_key); const needsUrl = meta.fields.some(f => f.key === 'base_url'); return html` @@ -268,7 +269,7 @@ export class LlmProvidersPage extends LightElement { this._setField('api_key', e.target.value)} /> ` : ''}