From 3db904a6a793913a2d241df136bed0f07a9c5f1e Mon Sep 17 00:00:00 2001 From: Max Loffgren Date: Mon, 5 Oct 2026 22:24:59 -0500 Subject: [PATCH 1/4] fix(core): keep API keys off other hosts and plain http, and time out silent endpoints A named provider without a key of its own inherited settings.api_key and OPENMAX_API_KEY, so a key configured for one server went to whatever host a providers.json entry named, and any key went out as a bearer header over plain http to any host. Separately, only connecting had a deadline: an endpoint that accepted a request and then sent nothing held the turn forever, so a headless or stdio run never ended. A provider now inherits the settings key only when its base_url has the same scheme, host, and port as settings.base_url. Any other server gets no Authorization header, and a 401 from it names how to give the provider its own key. A request that would carry a key or an Authorization header over http to a non-loopback host is refused before it is sent, naming https or a loopback address as the fix. An idle timeout (10 minutes by default, idle_timeout_secs per provider) covers the wait for response headers and every gap in the body, and SSE keepalive comments count as activity. Silence ends the attempt as a transport fault: resent before any reply text, a truncation after it, and a finished reply stands. The configuration docs and the providers, settings, and stdio specs describe both rules. --- crates/core/src/client.rs | 438 ++++++++++++++++++++++++++++++++--- crates/core/src/providers.rs | 153 ++++++++++-- crates/core/src/spec.rs | 22 +- crates/core/src/types.rs | 9 +- docs/configuration.md | 20 +- docs/stdio-protocol.md | 2 +- 6 files changed, 588 insertions(+), 56 deletions(-) diff --git a/crates/core/src/client.rs b/crates/core/src/client.rs index 95658a83..4fd76578 100644 --- a/crates/core/src/client.rs +++ b/crates/core/src/client.rs @@ -7,13 +7,13 @@ //! //! Retries cover what can end one request before the reply exists: a //! transport failure on send, a rate limit or transient server error (429, -//! 500, 502, 503, 504, 529), and a stream that died, or a stream or reply -//! the server failed with a rate limit, an overload, or a server fault, -//! before any reply text arrived. Each attempt resends the same bytes after -//! an exponential backoff, stretched to a server's Retry-After, and tells -//! the caller through [`StreamDelta::Retry`]. A 429 for an exhausted quota, -//! and a Retry-After longer than a minute, are reported at once: no wait the -//! turn would take lets the request succeed. +//! 500, 502, 503, 504, 529), and a stream that died or went silent, or a +//! stream or reply the server failed with a rate limit, an overload, or a +//! server fault, before any reply text arrived. Each attempt resends the +//! same bytes after an exponential backoff, stretched to a server's +//! Retry-After, and tells the caller through [`StreamDelta::Retry`]. A 429 +//! for an exhausted quota, and a Retry-After longer than a minute, are +//! reported at once: no wait the turn would take lets the request succeed. //! Once reply text has streamed, a retry would duplicate what the caller //! already showed, so a cut after that point is reported as a truncation //! instead. A failure the server reports inside a 200 response (an `error` @@ -29,11 +29,20 @@ //! signal costs the whole budget on such a reply before its truncation is //! reported. //! -//! There is no overall request timeout, only a connect timeout. A local or -//! slow endpoint can legitimately take minutes to generate, and a deadline -//! here would look like a bug in the model rather than a policy in the client. +//! There is no overall request timeout. A local or slow endpoint can +//! legitimately take minutes to generate, and a deadline here would look like +//! a bug in the model rather than a policy in the client. Silence ends an +//! attempt instead: an endpoint that sends nothing at all (no response +//! headers, no reply bytes, not even an SSE keepalive comment) for +//! [`IDLE_TIMEOUT`], or its provider's `idle_timeout_secs`, is a connection +//! that failed, under the rules above. A one-shot JSON reply or a refusal +//! whose body goes silent is an error, as a cut there is. //! A stream that ends without `[DONE]` and without a `finish_reason` is //! reported as truncated rather than treated as a complete reply. +//! +//! A credential never crosses plain http to another machine: a request that +//! would carry the key or an Authorization header that way is refused +//! before it is sent. use std::collections::HashMap; use std::sync::Arc; @@ -237,6 +246,13 @@ pub const TRUNCATED: &str = "truncated"; // Bound wire input, including reasoning, unterminated lines, and tool arguments. const MAX_RESPONSE_BYTES: usize = 16 * 1024 * 1024; const MAX_TOOL_CALLS: usize = 128; +/// How long an endpoint may send nothing at all (no response headers, no +/// reply bytes, no keepalive comment) before the attempt ends as a transport +/// fault. Generous, because a local server can spend minutes on a long +/// prompt before its first byte; a provider's `idle_timeout_secs` sets +/// another. Without one, an endpoint that took the request and went quiet +/// held the turn forever. +const IDLE_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(600); pub struct CompletionResult { pub content: String, @@ -283,6 +299,11 @@ pub struct ChatClient { pub headers: Vec<(String, String)>, pub use_max_completion_tokens: bool, pub send_stream_options: bool, + /// How long the endpoint may send nothing before an attempt ends. + idle_timeout: std::time::Duration, + /// Why no key was sent though settings configure one, for a 401 to + /// report (see `ActiveEndpoint::key_hint`). + key_hint: Option, http: reqwest::Client, } @@ -388,16 +409,20 @@ impl ChatClient { /// Build a client from a resolved multi-provider endpoint. pub fn from_endpoint(ep: &crate::providers::ActiveEndpoint) -> Self { - Self::with_options( - ep.base_url.clone(), - ep.api_key.clone(), - ep.model.clone(), - ep.temperature, - ep.max_tokens, - ep.headers.clone(), - ep.compat.use_max_completion_tokens, - ep.compat.send_stream_options, - ) + Self { + idle_timeout: ep.idle_timeout_secs.map_or(IDLE_TIMEOUT, |secs| std::time::Duration::from_secs(secs.max(1))), + key_hint: ep.key_hint.clone(), + ..Self::with_options( + ep.base_url.clone(), + ep.api_key.clone(), + ep.model.clone(), + ep.temperature, + ep.max_tokens, + ep.headers.clone(), + ep.compat.use_max_completion_tokens, + ep.compat.send_stream_options, + ) + } } // One argument per endpoint option; the call sites build it straight from @@ -421,7 +446,8 @@ impl ChatClient { .get_or_init(|| { reqwest::Client::builder() .connect_timeout(std::time::Duration::from_secs(10)) - // No overall timeout: local generations can legitimately take minutes. + // No overall timeout: local generations can legitimately + // take minutes. `stream_chat` ends an attempt on silence. .build() .expect("failed to build http client") }) @@ -435,6 +461,8 @@ impl ChatClient { headers, use_max_completion_tokens, send_stream_options, + idle_timeout: IDLE_TIMEOUT, + key_hint: None, http, } } @@ -443,6 +471,13 @@ impl ChatClient { format!("{}/chat/completions", self.base_url.trim_end_matches('/')) } + /// Whether a request carries a credential: the key, or an Authorization + /// header among the provider's headers. + fn sends_credential(&self) -> bool { + self.api_key.as_deref().is_some_and(|key| !key.is_empty()) + || self.headers.iter().any(|(name, _)| name.eq_ignore_ascii_case("authorization")) + } + /// The stamp for reasoning this endpoint produces: the first 16 hex chars /// of sha256 over the whole route, base_url, model, credential, and /// headers. The model is part of it because a server can reject reasoning @@ -477,6 +512,11 @@ impl ChatClient { cancelled: Arc, mut on_delta: impl FnMut(StreamDelta), ) -> Result { + if self.sends_credential() { + if let Some(refusal) = plain_http_refusal(&self.base_url) { + return Err(refusal); + } + } // Serialize once before retries: cloning bytes is cheap; re-walking a // long transcript into Value (and re-serializing) on every attempt is not. // Tools ride as RawValue from the frozen registry wire form. @@ -513,14 +553,31 @@ impl ChatClient { } // An endpoint can spend a long time in prompt processing before the // first byte arrives, and a large prompt makes that worse wherever - // it runs; keep cancellation responsive throughout. + // it runs; keep cancellation responsive throughout. Only silence + // for the whole idle interval ends the wait. let send_result = tokio::select! { - r = req.send() => r, + r = tokio::time::timeout(self.idle_timeout, req.send()) => r, _ = cancelled.cancelled() => return Ok(cancelled_response()), }; let resp = match send_result { - Ok(r) => r, - Err(e) => { + Ok(Ok(r)) => r, + // The endpoint took the request and sent nothing back, not + // even a status line (a wedged server, a proxy holding a dead + // upstream). Waiting on would hold the turn for good. Nothing + // has reached the caller, so it is resent like any transport + // fault after the connection existed. + Err(_) => { + unreached = 0; + let msg = format!("request failed: {}", silence(self.idle_timeout)); + if attempt < MAX_ATTEMPTS { + if !retry_after(attempt, &msg, &cancelled, &mut on_delta).await { + return Ok(cancelled_response()); + } + continue; + } + return Err(msg); + } + Ok(Err(e)) => { let msg = format!("request failed: {}", describe_transport(&e)); unreached = if e.is_connect() || e.is_timeout() { unreached + 1 } else { 0 }; if attempt < MAX_ATTEMPTS && unreached < UNREACHED_ATTEMPTS && is_transient_transport(&e) { @@ -542,7 +599,7 @@ impl ChatClient { let beyond_cap = asked.filter(|&secs| secs > RETRY_AFTER_CAP_SECS); if !status.is_success() { let code = status.as_u16(); - let body = read_body(resp, &cancelled).await + let body = read_body(resp, &cancelled, self.idle_timeout).await .map_err(|e| format!("backend returned {status}: {e}"))?; let Some(body) = body else { return Ok(cancelled_response()); }; let text = String::from_utf8_lossy(&body); @@ -551,6 +608,11 @@ impl ChatClient { if let Some(hint) = temperature_hint(self.temperature, &message) { err.push_str(&hint); } + if code == 401 { + if let Some(hint) = &self.key_hint { + err.push_str(hint); + } + } if attempt < MAX_ATTEMPTS && is_retryable_status(code) && !quota_exhausted(&text) && beyond_cap.is_none() { if !resend_after(attempt, &err, backoff(attempt, asked), &cancelled, &mut on_delta).await { return Ok(cancelled_response()); @@ -567,7 +629,7 @@ impl ChatClient { // Some servers ignore `stream` and return a complete JSON body. if is_json { - let Some(body) = read_body(resp, &cancelled).await? else { return Ok(cancelled_response()); }; + let Some(body) = read_body(resp, &cancelled, self.idle_timeout).await? else { return Ok(cancelled_response()); }; let v: Value = serde_json::from_slice(&body).map_err(|e| format!("bad JSON response: {e}"))?; // A 200 can still carry a failure in place of the reply: an // error at the top level, or one on the choice beside @@ -589,7 +651,7 @@ impl ChatClient { return parse_complete_response(&v, &mut on_delta); } - let (reply, unfinished) = read_sse(resp, &cancelled, &mut on_delta).await; + let (reply, unfinished) = read_sse(resp, &cancelled, self.idle_timeout, &mut on_delta).await; // The restart rules: nothing the caller keeps has streamed yet, // and the budget has an attempt left. let restartable = reply.content.is_empty() && attempt < MAX_ATTEMPTS; @@ -667,6 +729,7 @@ fn server_failure(error: &Value, raw: &str) -> ServerFailure { async fn read_sse( resp: reqwest::Response, cancelled: &crate::state::CancelToken, + idle: std::time::Duration, on_delta: &mut impl FnMut(StreamDelta), ) -> (CompletionResult, Option) { let mut content = String::new(); @@ -697,12 +760,22 @@ async fn read_sse( 'outer: loop { let next = tokio::select! { - c = stream.next() => c, + c = tokio::time::timeout(idle, stream.next()) => c, _ = cancelled.cancelled() => { finish_reason = "cancelled".into(); break; } }; + // Not a byte, not even a keepalive comment, for the whole idle + // interval: the connection failed as surely as a cut, and ends the + // same way. After the server finished, the reply stands. + let Ok(next) = next else { + if !saw_terminator { + finish_reason = TRUNCATED.into(); + unfinished = Some(Unfinished::Interrupted(silence(idle))); + } + break; + }; let Some(chunk) = next else { if !saw_terminator { unfinished = Some(Unfinished::Interrupted("the stream ended before the reply finished".into())); @@ -939,14 +1012,16 @@ fn cancelled_response() -> CompletionResult { async fn read_body( resp: reqwest::Response, cancelled: &crate::state::CancelToken, + idle: std::time::Duration, ) -> Result>, String> { let mut body = Vec::new(); let mut stream = resp.bytes_stream(); loop { let next = tokio::select! { _ = cancelled.cancelled() => return Ok(None), - next = stream.next() => next, + next = tokio::time::timeout(idle, stream.next()) => next, }; + let Ok(next) = next else { return Err(format!("response body failed: {}", silence(idle))) }; let Some(chunk) = next else { return Ok(Some(body)); }; let chunk = chunk.map_err(|e| format!("response body failed: {e}"))?; if chunk.len() > MAX_RESPONSE_BYTES - body.len() { @@ -1204,6 +1279,36 @@ fn backoff(attempt: u32, asked: Option) -> std::time::Duration { floor.max(asked) } +/// Why an attempt ended when the endpoint sent nothing for `idle`. +fn silence(idle: std::time::Duration) -> String { + format!("the endpoint sent nothing for {}s", idle.as_secs_f64()) +} + +/// Why a request to `base_url` must not carry a credential, or None when it +/// may. Plain http to another machine puts the key on the network +/// unencrypted, readable anywhere along the path. Loopback never leaves this +/// machine, and neither does the unspecified address (0.0.0.0 or ::), which +/// a local server often prints as its own and which connects here. A +/// base_url that does not parse is left to the request to fail. +fn plain_http_refusal(base_url: &str) -> Option { + let url = reqwest::Url::parse(base_url.trim()).ok()?; + if url.scheme() != "http" { + return None; + } + let host = url.host_str()?; + let bare = host.trim_start_matches('[').trim_end_matches(']'); + let this_machine = bare.eq_ignore_ascii_case("localhost") + || bare.parse::().is_ok_and(|ip| { + let ip = ip.to_canonical(); + ip.is_loopback() || ip.is_unspecified() + }); + (!this_machine).then(|| { + format!( + "refusing to send credentials over plain http to {host}, where anyone on the network path can read them: use an https base_url, or a loopback address (127.0.0.1, ::1, or localhost) for a server on this machine. A server that needs no key works over http once none is configured for it (api_key, api_key_env, OPENMAX_API_KEY, or an Authorization header)" + ) + }) +} + fn is_transient_transport(err: &reqwest::Error) -> bool { err.is_connect() || err.is_timeout() || err.is_request() } @@ -1360,6 +1465,21 @@ mod tests { } fn spawn_response_once(response: String, hold: Option>) -> String { + serve_once(response, hold, None) + } + + /// [`spawn_response_once`] that also hands back the request head it read + /// (request line and headers), to show what the client sent. + fn spawn_recording_once(response: String) -> (String, std::sync::mpsc::Receiver) { + let (head, heads) = std::sync::mpsc::channel(); + (serve_once(response, None, Some(head)), heads) + } + + fn serve_once( + response: String, + hold: Option>, + head: Option>, + ) -> String { use std::io::{Read as _, Write as _}; let listener = std::net::TcpListener::bind("127.0.0.1:0").unwrap(); let addr = listener.local_addr().unwrap(); @@ -1376,6 +1496,9 @@ mod tests { } } let headers = String::from_utf8_lossy(&buf).to_string(); + if let Some(head) = head { + let _ = head.send(headers.clone()); + } let content_length: usize = headers .lines() .find_map(|l| { @@ -1640,6 +1763,9 @@ mod tests { let served = Arc::new(std::sync::Mutex::new(0usize)); let count = served.clone(); std::thread::spawn(move || { + // Connections a SILENT or STALL: body holds open, until the + // sequence ends. + let mut held = Vec::new(); for sse in bodies { let Ok((mut stream, _)) = listener.accept() else { return }; let mut buf = Vec::new(); @@ -1666,6 +1792,38 @@ mod tests { if sse.is_empty() { continue; } + // SILENT takes the request and answers nothing, not even a + // status line, while keeping the connection open. + if sse == "SILENT" { + held.push(stream); + continue; + } + // A body prefixed with STALL: is sent as one chunk of a + // chunked stream, which then neither continues nor ends. + if let Some(payload) = sse.strip_prefix("STALL:") { + let _ = stream.write_all( + format!( + "HTTP/1.1 200 OK\r\nContent-Type: text/event-stream\r\nTransfer-Encoding: chunked\r\n\r\n{:x}\r\n{payload}\r\n", + payload.len() + ) + .as_bytes(), + ); + held.push(stream); + continue; + } + // A body prefixed with KEEPALIVE: is preceded by SSE comments, + // each sent well inside [`SILENT_FOR`] and all of them well + // past it: a server holding its stream open through a long + // prompt. + if let Some(payload) = sse.strip_prefix("KEEPALIVE:") { + let _ = stream.write_all(b"HTTP/1.1 200 OK\r\nContent-Type: text/event-stream\r\nConnection: close\r\n\r\n"); + for _ in 0..12 { + std::thread::sleep(SILENT_FOR / 5); + let _ = stream.write_all(b": keepalive\n\n"); + } + let _ = stream.write_all(payload.as_bytes()); + continue; + } // A body prefixed with RAW: is the whole response, status // line and headers included (see `status_response`). if let Some(response) = sse.strip_prefix("RAW:") { @@ -1712,9 +1870,20 @@ mod tests { } async fn try_stream_sequence(bodies: Vec) -> (Result, Vec, usize) { + try_stream_sequence_idle(bodies, IDLE_TIMEOUT).await + } + + /// [`try_stream_sequence`] with the client giving up on an endpoint + /// after `idle` of silence. + async fn try_stream_sequence_idle( + bodies: Vec, + idle: std::time::Duration, + ) -> (Result, Vec, usize) { let (url, served) = spawn_sse_sequence(bodies); let mut deltas: Vec = Vec::new(); - let result = ChatClient::new(url, None, "m".into(), None, 64) + let mut client = ChatClient::new(url, None, "m".into(), None, 64); + client.idle_timeout = idle; + let result = client .stream_chat(&[ChatMessage::user("hi")], "[]", Arc::new(crate::state::CancelToken::default()), |d| { deltas.push(match d { StreamDelta::Content(t) => format!("content:{t}"), @@ -2316,6 +2485,213 @@ mod tests { assert_eq!(result.tool_calls.len(), 1); } + /// How long a test endpoint stays quiet before the client gives up on + /// it: the idle interval, injected in place of the minutes a real + /// endpoint gets. + const SILENT_FOR: std::time::Duration = std::time::Duration::from_millis(250); + + /// [`try_stream_sequence_idle`] at [`SILENT_FOR`], under a deadline that + /// turns a hang into a failure. + async fn stream_with_silence(bodies: Vec) -> (Result, Vec, usize) { + tokio::time::timeout(std::time::Duration::from_secs(10), try_stream_sequence_idle(bodies, SILENT_FOR)) + .await + .expect("a silent endpoint must not hold the request forever") + } + + /// The bug this guards: only connecting had a deadline, so an endpoint + /// that took the request and then sent nothing (a wedged server, a proxy + /// holding a dead upstream) held the turn forever, and a headless run + /// never exited. Silence for the whole idle interval ends the attempt as + /// a transport fault: before any reply text it is resent, whether the + /// endpoint never answered or its stream stopped part way. + #[tokio::test] + async fn a_silent_endpoint_is_resent_before_reply_text() { + let silent = format!("the endpoint sent nothing for {}s", SILENT_FOR.as_secs_f64()); + let (result, deltas, served) = stream_with_silence(vec!["SILENT".into(), FINISHED.into()]).await; + assert_eq!(result.expect("the second attempt finished").content, "all of it"); + assert_eq!(served, 2); + assert_eq!(deltas, vec![format!("retry:2/{MAX_ATTEMPTS}:request failed: {silent}"), "content:all of it".to_string()]); + + let thinking = "data: {\"choices\":[{\"delta\":{\"reasoning_content\":\"let me\"},\"finish_reason\":null}]}\n\n"; + let (result, deltas, served) = stream_with_silence(vec![format!("STALL:{thinking}"), FINISHED.into()]).await; + assert_eq!(result.expect("the second attempt finished").content, "all of it"); + assert_eq!(served, 2); + assert_eq!( + deltas, + vec!["reasoning:let me".to_string(), format!("retry:2/{MAX_ATTEMPTS}:{silent}"), "content:all of it".to_string()] + ); + } + + /// Silence after reply text is a truncation, as a cut there is: the + /// caller has already shown that text. Silence after the server finished + /// leaves the reply finished. + #[tokio::test] + async fn a_stream_silent_after_reply_text_or_its_finish_is_not_resent() { + let half = "data: {\"choices\":[{\"delta\":{\"content\":\"half an ans\"},\"finish_reason\":null}]}\n\n"; + let (result, deltas, served) = stream_with_silence(vec![format!("STALL:{half}"), FINISHED.into()]).await; + let result = result.expect("a truncated reply is still a reply"); + assert_eq!((result.content.as_str(), result.finish_reason.as_str()), ("half an ans", TRUNCATED)); + assert_eq!(served, 1); + assert_eq!(deltas, vec!["content:half an ans".to_string()]); + + let done = "data: {\"choices\":[{\"delta\":{\"content\":\"all of it\"},\"finish_reason\":\"stop\"}]}\n\n"; + let (result, _, served) = stream_with_silence(vec![format!("STALL:{done}"), FINISHED.into()]).await; + let result = result.expect("a finished reply"); + assert_eq!((result.content.as_str(), result.finish_reason.as_str()), ("all of it", "stop")); + assert_eq!(served, 1); + } + + /// Any byte is activity, an SSE comment too: a server that keeps its + /// stream alive with comments through a long prompt is waited for, however + /// long that takes in all. + #[tokio::test] + async fn keepalive_comments_hold_off_the_idle_timeout() { + let started = std::time::Instant::now(); + let (result, deltas, served) = stream_with_silence(vec![format!("KEEPALIVE:{FINISHED}")]).await; + assert_eq!(result.expect("a kept-alive stream finishes").content, "all of it"); + assert_eq!((served, deltas), (1, vec!["content:all of it".to_string()])); + assert!(started.elapsed() > SILENT_FOR * 2, "the stream outlasted the idle interval"); + } + + /// A reply for [`spawn_recording_once`]: the finished stream. + fn finished_response() -> String { + format!("HTTP/1.1 200 OK\r\nContent-Type: text/event-stream\r\nConnection: close\r\n\r\n{FINISHED}") + } + + /// The bug this guards: a provider without a key of its own was sent the + /// key settings.json holds for settings.base_url, whatever host the + /// provider named. It inherits that key only when it is the same server + /// (scheme, host, and port); any other gets no key, and the 401 that + /// follows says how to give it its own. + #[tokio::test] + async fn a_settings_key_reaches_only_its_own_host() { + let dir = std::env::temp_dir().join(format!("openmax-keys-{}", uuid::Uuid::new_v4())); + std::fs::create_dir_all(&dir).unwrap(); + let provider_at = |url: &str| { + let catalog = json!({"providers": {"local": {"base_url": url, "models": [{"id": "m", "context_tokens": 8192}]}}}); + std::fs::write(crate::providers::providers_path(&dir), catalog.to_string()).unwrap(); + }; + let mut settings = crate::config::Settings { + provider: Some("local".into()), + model: "m".into(), + base_url: "https://api.example.com/v1".into(), + api_key: Some("sk-settings".into()), + ..Default::default() + }; + let request = |endpoint: &crate::providers::ActiveEndpoint| { + let client = ChatClient::from_endpoint(endpoint); + async move { + client + .stream_chat(&[ChatMessage::user("hi")], "[]", Arc::new(crate::state::CancelToken::default()), |_| {}) + .await + } + }; + + let refusal = r#"{"error":{"message":"missing credentials"}}"#; + let (url, heads) = spawn_recording_once(format!( + "HTTP/1.1 401 Unauthorized\r\nContent-Type: application/json\r\nContent-Length: {}\r\n\r\n{refusal}", + refusal.len() + )); + provider_at(&url); + let endpoint = crate::providers::resolve(&settings, &dir).unwrap(); + let err = request(&endpoint).await.err().expect("a 401 is an error"); + let head = heads.recv().unwrap().to_ascii_lowercase(); + assert!(!head.contains("authorization") && !head.contains("sk-settings"), "{head}"); + assert!(err.starts_with("backend returned 401 Unauthorized: missing credentials"), "{err}"); + assert!(err.contains("provider 'local'") && err.contains("api_key_env"), "{err}"); + + // The same scheme, host, and port is the same server, whatever the path. + let (url, heads) = spawn_recording_once(finished_response()); + provider_at(&url); + settings.base_url = url.trim_end_matches("/v1").into(); + let endpoint = crate::providers::resolve(&settings, &dir).unwrap(); + assert_eq!(request(&endpoint).await.unwrap().content, "all of it"); + let head = heads.recv().unwrap().to_ascii_lowercase(); + assert!(head.contains("authorization: bearer sk-settings"), "{head}"); + let _ = std::fs::remove_dir_all(dir); + } + + /// The bug this guards: the key went out as a bearer header over plain + /// http to any host, readable anywhere on the path. A request that would + /// carry a credential (the key, or an Authorization header) over http to + /// another machine is refused before anything is sent, naming the fix; + /// over loopback, to a server on this machine, it goes out. + #[tokio::test] + async fn a_key_never_crosses_plain_http_to_another_machine() { + for (key, headers) in [ + (Some("sk-test".to_string()), Vec::new()), + (None, vec![("Authorization".to_string(), "Bearer sk-test".to_string())]), + ] { + let mut client = ChatClient::new("http://models.example.invalid/v1".into(), key, "m".into(), None, 64); + client.headers = headers; + let mut retries = 0; + let err = client + .stream_chat(&[ChatMessage::user("hi")], "[]", Arc::new(crate::state::CancelToken::default()), |_| retries += 1) + .await + .err() + .expect("a key over plain http to another machine is refused"); + assert!(err.starts_with("refusing to send"), "{err}"); + assert!(err.contains("models.example.invalid") && err.contains("https") && err.contains("127.0.0.1"), "{err}"); + assert_eq!(retries, 0, "nothing was sent, so nothing was resent"); + } + + let (url, heads) = spawn_recording_once(finished_response()); + let result = ChatClient::new(url, Some("sk-local".into()), "m".into(), None, 64) + .stream_chat(&[ChatMessage::user("hi")], "[]", Arc::new(crate::state::CancelToken::default()), |_| {}) + .await + .expect("loopback http carries a key"); + assert_eq!(result.content, "all of it"); + let head = heads.recv().unwrap().to_ascii_lowercase(); + assert!(head.contains("authorization: bearer sk-local"), "{head}"); + } + + /// https goes anywhere; plain http only to this machine, however its + /// address is spelled. + #[test] + fn only_https_or_this_machine_may_carry_a_key() { + for url in [ + "https://api.example.com/v1", + "http://127.0.0.1:11434/v1", + "http://127.8.9.10/v1", + "http://localhost:1234/v1", + "http://LOCALHOST/v1", + "http://[::1]:8080/v1", + "http://[::ffff:127.0.0.1]/v1", + "http://0.0.0.0:8000/v1", + ] { + assert_eq!(plain_http_refusal(url), None, "{url}"); + } + for url in [ + "http://api.example.com/v1", + "http://192.168.1.5:8000/v1", + "http://10.0.0.2/v1", + "http://[2001:db8::1]/v1", + "http://localhost.example.com/v1", + ] { + assert!(plain_http_refusal(url).is_some(), "{url}"); + } + } + + /// A provider's `idle_timeout_secs` replaces the default interval for + /// its requests; a provider without one keeps the default. + #[test] + fn a_providers_idle_timeout_secs_sets_its_interval() { + let dir = std::env::temp_dir().join(format!("openmax-idle-{}", uuid::Uuid::new_v4())); + std::fs::create_dir_all(&dir).unwrap(); + let catalog = json!({"providers": { + "slow": {"base_url": "http://127.0.0.1:8080/v1", "idle_timeout_secs": 1800, "models": [{"id": "m", "context_tokens": 8192}]}, + "plain": {"base_url": "http://127.0.0.1:8081/v1", "models": [{"id": "m", "context_tokens": 8192}]}, + }}); + std::fs::write(crate::providers::providers_path(&dir), catalog.to_string()).unwrap(); + let idle = |provider: &str| { + let settings = crate::config::Settings { provider: Some(provider.into()), model: "m".into(), ..Default::default() }; + ChatClient::from_endpoint(&crate::providers::resolve(&settings, &dir).unwrap()).idle_timeout + }; + assert_eq!(idle("slow"), std::time::Duration::from_secs(1800)); + assert_eq!(idle("plain"), IDLE_TIMEOUT); + let _ = std::fs::remove_dir_all(dir); + } + #[test] fn retryable_status_codes() { // A rate limit, or an upstream that cannot serve the request right now. diff --git a/crates/core/src/providers.rs b/crates/core/src/providers.rs index 7c2755c9..7283399a 100644 --- a/crates/core/src/providers.rs +++ b/crates/core/src/providers.rs @@ -71,6 +71,8 @@ struct ProviderConfigFile { models: Vec, #[serde(default)] compat: CompatFlagsFile, + #[serde(default)] + idle_timeout_secs: Option, } #[derive(Clone, Debug, Deserialize)] @@ -88,6 +90,9 @@ pub struct ProviderConfig { pub headers: BTreeMap, pub models: Vec, pub compat: CompatFlags, + /// Seconds the endpoint may send nothing before an attempt is given up; + /// unset means the client's default. + pub idle_timeout_secs: Option, } #[derive(Clone, Debug, Default, Deserialize)] @@ -108,6 +113,12 @@ pub struct ActiveEndpoint { pub max_tokens: usize, pub temperature: Option, pub compat: CompatFlags, + /// The provider's `idle_timeout_secs`, when it sets one. + pub idle_timeout_secs: Option, + /// Set when a key is configured in settings but withheld from this + /// endpoint because it is another server: why no key was sent and how to + /// give the provider its own, for the error a 401 from it reports. + pub key_hint: Option, } pub fn providers_path(data_dir: &Path) -> PathBuf { @@ -196,6 +207,7 @@ fn parse_providers_file(text: &str) -> Result, headers: raw.headers, models: raw.models, compat: raw.compat.into(), + idle_timeout_secs: raw.idle_timeout_secs, }, ) }) @@ -245,6 +257,9 @@ pub(crate) fn check_file(path: &Path) -> Option), Str ))) } } + if provider.idle_timeout_secs == Some(0) { + return Some(Err(format!("provider '{name}' has zero idle_timeout_secs"))); + } for (header, value) in &provider.headers { if reqwest::header::HeaderName::from_bytes(header.as_bytes()).is_err() { return Some(Err(format!( @@ -303,7 +318,7 @@ fn unknown_key_warnings(text: &str) -> Vec { let Some(provider) = provider.as_object() else { continue }; unknown_keys( provider, - &["base_url", "api_key", "api_key_env", "headers", "models", "compat"], + &["base_url", "api_key", "api_key_env", "headers", "models", "compat", "idle_timeout_secs"], &format!("in provider '{name}'"), &mut out, ); @@ -516,11 +531,17 @@ pub fn resolve(settings: &Settings, data_dir: &Path) -> Result (Some(key), None), + None => match settings_key(settings.api_key.as_deref()) { + Some(key) if same_server(base_url, &settings.base_url) => (Some(key), None), + Some(_) => (None, Some(withheld_key_hint(name, p))), + None => (None, None), + }, + }; let headers = expand_headers(&p.headers); return Ok(ActiveEndpoint { provider: Some(name.clone()), @@ -532,6 +553,8 @@ pub fn resolve(settings: &Settings, data_dir: &Path) -> Result Result, - provider_env: &[String], - settings_key: Option<&str>, -) -> Option { - if let Some(k) = provider_key { +/// A provider's own key: its `api_key`, else the first non-empty variable +/// its `api_key_env` names. +fn provider_key(key: Option<&str>, env: &[String]) -> Option { + if let Some(k) = key { if let Some(v) = expand_secret(k) { if !v.is_empty() { return Some(v); } } } - for name in provider_env { + for name in env { if let Ok(v) = std::env::var(name) { let v = v.trim().to_string(); if !v.is_empty() { @@ -584,7 +607,13 @@ fn resolve_api_key( } } } - if let Some(k) = settings_key { + None +} + +/// The key configured for settings.base_url: settings.api_key, else +/// `OPENMAX_API_KEY`. +fn settings_key(key: Option<&str>) -> Option { + if let Some(k) = key { if let Some(v) = expand_secret(k) { if !v.is_empty() { return Some(v); @@ -600,6 +629,29 @@ fn resolve_api_key( None } +/// Whether two base URLs name the same server: scheme, host, and port +/// (explicit or the scheme's default) all match. The path does not count. +/// A URL that does not parse matches nothing. +fn same_server(a: &str, b: &str) -> bool { + let server = |url: &str| { + let url = reqwest::Url::parse(url.trim()).ok()?; + Some((url.scheme().to_string(), url.host_str()?.to_string(), url.port_or_known_default()?)) + }; + server(a).is_some_and(|a| server(b) == Some(a)) +} + +/// What a 401 from provider `name` adds when the settings key was withheld +/// from it: why no key went out, and how to give the provider its own. +fn withheld_key_hint(name: &str, p: &ProviderConfig) -> String { + let fix = match p.api_key_env.first() { + Some(var) => format!("export {var}, which provider '{name}' reads its key from"), + None => format!("give provider '{name}' its own key with api_key_env in ~/.openmax/providers.json"), + }; + format!( + " (no key was sent: the key in settings.json or OPENMAX_API_KEY goes only to the server settings.base_url names, by scheme, host, and port; {fix})" + ) +} + /// Expand secrets: /// - `$$...` → literal starting with `$` (escape) /// - `$ENV_VAR` → environment value @@ -714,6 +766,23 @@ mod tests { .unwrap() .unwrap_err() .contains("invalid JSON")); + + // An idle interval is a known key; zero would end every attempt at once. + std::fs::write( + &path, + r#"{"providers":{"slow":{"base_url":"http://localhost/v1","idle_timeout_secs":1800}}}"#, + ) + .unwrap(); + assert_eq!(check_file(&path).unwrap().unwrap(), (1, Vec::new())); + std::fs::write( + &path, + r#"{"providers":{"slow":{"base_url":"http://localhost/v1","idle_timeout_secs":0}}}"#, + ) + .unwrap(); + assert!(check_file(&path) + .unwrap() + .unwrap_err() + .contains("zero idle_timeout_secs")); let _ = std::fs::remove_dir_all(dir); } @@ -889,6 +958,60 @@ mod tests { let _ = std::fs::remove_dir_all(dir); } + /// The bug this guards: a provider with no key of its own was handed the + /// key settings.json holds for settings.base_url, whatever host the + /// provider names, so one server's credential went to another. Only the + /// same scheme, host, and port inherits it (the path does not matter); + /// any other provider resolves with no key and a hint for the 401 it + /// will get, naming how to give it its own. + #[test] + fn a_settings_key_is_inherited_only_by_its_own_host() { + let dir = std::env::temp_dir().join(format!("openmax-prov-{}", uuid::Uuid::new_v4())); + let unset = format!("OPENMAX_TEST_UNSET_{}", uuid::Uuid::new_v4().simple()); + let sized = r#""models": [{"id": "m", "context_tokens": 8192}]"#; + write_providers( + &dir, + &format!( + r#"{{"providers": {{ + "same": {{"base_url": "https://API.example.com:443/v2", {sized}}}, + "other_host": {{"base_url": "https://other.example.net/v1", {sized}}}, + "other_port": {{"base_url": "https://api.example.com:8443/v1", {sized}}}, + "plain": {{"base_url": "http://api.example.com/v1", {sized}}}, + "unset_env": {{"base_url": "https://other.example.net/v1", "api_key_env": "{unset}", {sized}}} + }}}}"# + ), + ); + let mut s = Settings { + base_url: "https://api.example.com/v1".into(), + api_key: Some("sk-settings".into()), + model: "m".into(), + provider: Some("same".into()), + ..Default::default() + }; + let ep = resolve(&s, &dir).unwrap(); + assert_eq!(ep.api_key.as_deref(), Some("sk-settings")); + assert_eq!(ep.key_hint, None); + + for name in ["other_host", "other_port", "plain"] { + s.provider = Some(name.into()); + let ep = resolve(&s, &dir).unwrap(); + assert_eq!(ep.api_key, None, "{name} is another server"); + let hint = ep.key_hint.expect("a withheld key explains the 401 it causes"); + assert!(hint.contains(&format!("provider '{name}'")) && hint.contains("api_key_env"), "{hint}"); + } + // A provider whose own variable is unset is told to set that one. + s.provider = Some("unset_env".into()); + let ep = resolve(&s, &dir).unwrap(); + assert_eq!(ep.api_key, None); + assert!(ep.key_hint.is_some_and(|hint| hint.contains(&unset))); + + // With no settings.base_url, the key belongs to no provider's host. + s.provider = Some("same".into()); + s.base_url.clear(); + assert_eq!(resolve(&s, &dir).unwrap().api_key, None); + let _ = std::fs::remove_dir_all(dir); + } + #[test] fn unknown_provider_errors() { let dir = std::env::temp_dir().join(format!("openmax-prov-{}", uuid::Uuid::new_v4())); diff --git a/crates/core/src/spec.rs b/crates/core/src/spec.rs index a50b759c..299aae08 100644 --- a/crates/core/src/spec.rs +++ b/crates/core/src/spec.rs @@ -639,13 +639,24 @@ Shape: `{"providers": {"": { ... }}}`. Per provider: `chat/completions` on it. - `api_key` (optional): literal secret. Prefer `api_key_env`. - `api_key_env` (optional): env var name, or list of names (first non-empty - wins). + wins). With neither (or both resolving empty), the provider uses + settings.json's `api_key` / `OPENMAX_API_KEY` only when its `base_url` has + the same scheme, host, and port as settings `base_url`; any other server + gets no key, and a 401 from it names how to give the provider its own. - `headers` (optional): extra HTTP headers as a string map. - `models` (optional): list of `{"id", "name"?, "context_tokens"?, "max_tokens"?}`. Ids are sent unchanged; order is preserved in `/model`. - `compat` (optional): wire quirks for picky servers: `{"use_max_completion_tokens": false, "send_stream_options": true}` are the defaults. +- `idle_timeout_secs` (optional, default 600): how long the endpoint may send + nothing at all (no headers, no bytes, no SSE keepalive comment) before the + attempt ends; before any reply text it is resent. Raise it for a local + server that works through a long prompt silently for longer. + +A key or Authorization header never goes over plain http to another machine: +such a request fails with an error before anything is sent. Use https, or a +loopback address (127.0.0.1, ::1, localhost) for a server on this machine. Select a provider with `"provider"` in settings.json, the `--provider` CLI option, or `/provider`; `/model` picks provider and model as one pair. This @@ -703,6 +714,9 @@ Fields (all optional in JSON; an empty `base_url`/`model` or a missing - `base_url`: OpenAI-compatible endpoint root (the harness calls `chat/completions` on it). No default, never a localhost fallback. - `api_key`: literal, or `$ENV_VAR` indirection; `OPENMAX_API_KEY` also works. + Sent only to the server `base_url` names (a named provider without a key of + its own inherits it only on the same scheme, host, and port), and never over + plain http except to a loopback address. - `model`: model id sent with every request. - `approval_mode`: `auto` | `ask` | `readonly`; default ask. Used only when neither this project nor an enclosing one has a saved choice, such as a @@ -922,9 +936,9 @@ here so a frontend can render what the model sees; `call_id` links it to the tool result it rode, or is empty for a note inserted before the next prompt like a turn-start receipt), `retry` (attempt, max_attempts, reason: the model request is being resent after a transport failure, a 429 or transient -5xx, or, before any reply text, a stream that died or a stream or reply the -provider failed with a rate limit, an overload, or a server fault; thinking -already streamed for that attempt is void), `diff` (call_id, +5xx, or, before any reply text, a stream that died or went silent, or a +stream or reply the provider failed with a rate limit, an overload, or a +server fault; thinking already streamed for that attempt is void), `diff` (call_id, path, diff, added, removed), `approval_request` (approval_id, name, summary, detail, reason, source_path, source_sha, and an optional `env`), `approval_settled` (approval_id, outcome), `refrozen` (tools, skills, changes: the refreeze receipt naming diff --git a/crates/core/src/types.rs b/crates/core/src/types.rs index 88a63b22..fa9267cd 100644 --- a/crates/core/src/types.rs +++ b/crates/core/src/types.rs @@ -164,10 +164,11 @@ pub enum AgentEvent { /// The model request is being resent: `attempt` of `max_attempts` goes /// out after a backoff, `reason` having ended the previous one (a /// transport failure, a 429 or transient 5xx, or, before any reply text, - /// a stream that died or a stream or reply the provider failed with a - /// rate limit, an overload, or a server fault). Emitted before the wait; - /// a cancel during it ends the turn and the attempt never goes out. - /// Thinking streamed for the failed attempt is void; no Token preceded it. + /// a stream that died or went silent, or a stream or reply the provider + /// failed with a rate limit, an overload, or a server fault). Emitted + /// before the wait; a cancel during it ends the turn and the attempt + /// never goes out. Thinking streamed for the failed attempt is void; no + /// Token preceded it. Retry { attempt: u32, max_attempts: u32, reason: String }, Diff { call_id: String, path: String, diff: String, added: usize, removed: usize }, /// Mutating tool waiting on the user. `detail` is a short args preview diff --git a/docs/configuration.md b/docs/configuration.md index c27a512b..f4086ae2 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -18,7 +18,12 @@ optional `~/.openmax/providers.json` for a catalog of named endpoints. `base_url` is the root of your model's HTTP API (the harness calls `chat/completions` on it). Set `model` to the id that server expects. Set -`api_key` to a literal or `$ENV_VAR`, or export `OPENMAX_API_KEY`. +`api_key` to a literal or `$ENV_VAR`, or export `OPENMAX_API_KEY`. A key never +crosses plain http to another machine: with a key configured and an `http://` +`base_url` that is not a loopback address (`127.0.0.1`, `::1`, or `localhost`), +every request fails with an error instead of sending it. Use https, or a +loopback address for a server on this machine; a server that needs no key works +over http once none is configured. `max_parallel_tools` bounds concurrent read-only tool calls, defaults to 4, and is clamped to 1 through 32 at runtime. Mutating, approval-gated, and non-batchable calls remain serial. @@ -146,6 +151,19 @@ Set `"provider"` in settings, use the provider CLI option, or use `/provider` when you only want to change the endpoint. Optional `compat` flags cover picky gateways (for example `max_completion_tokens` versus `max_tokens`). +A provider takes its key from its own `api_key` or `api_key_env`. Without one, +it uses the settings `api_key` (or `OPENMAX_API_KEY`) only when its `base_url` +has the same scheme, host, and port as the settings `base_url`: that key was +configured for that server. Any other provider gets no key, and a 401 from it +says how to give that provider its own. + +A request has no overall deadline, since a local server can spend minutes on a +long prompt, but an endpoint that sends nothing at all (no response headers, no +reply bytes, not even an SSE keepalive comment) for 10 minutes ends the +attempt. Before any reply text has arrived it is resent like a dropped +connection; after, the reply is reported truncated. A provider's +`idle_timeout_secs` sets a different interval for that provider. + Open Max works with local servers (Ollama, LM Studio, vLLM, llama.cpp), cloud gateways (OpenRouter and similar), and private proxies. diff --git a/docs/stdio-protocol.md b/docs/stdio-protocol.md index c0f0b91a..57b886b3 100644 --- a/docs/stdio-protocol.md +++ b/docs/stdio-protocol.md @@ -73,7 +73,7 @@ one). | `tool_start` | `call_id`, `name`, `args` (object) | | `tool_end` | `call_id`, `ok` (bool), `output` | | `harness_note` | `call_id`, `text` (a note the harness wrote into the model's transcript: a refreeze receipt, or a policy, providers, settings, or approval notice. `call_id` links it to the tool result it rode; it is empty for a note inserted before the next prompt, such as a turn-start receipt) | -| `retry` | `attempt`, `max_attempts`, `reason` (the model request is being resent: `attempt` is the one that goes out of the `max_attempts` budget after a backoff wait, `reason` having ended the previous one with a transport failure, a 429 or transient server error (500, 502, 503, 504, 529), or, before any reply text arrived, a stream that died or a stream or reply the provider failed with a rate limit, an overload, or a server fault. The wait is the backoff, or the response's `Retry-After` in seconds when that is longer. A `Retry-After` of more than a minute is not retried: the request fails at once and the error names the server's wait. Neither is a 429 reporting an exhausted quota (`insufficient_quota`). Emitted before the wait; a `cancel` during it ends the turn and the attempt never goes out. `thinking` lines already emitted for that attempt are void and the reply starts over; no `token` line precedes a retried stream. A request gives up before the budget when three attempts in a row never reached the endpoint) | +| `retry` | `attempt`, `max_attempts`, `reason` (the model request is being resent: `attempt` is the one that goes out of the `max_attempts` budget after a backoff wait, `reason` having ended the previous one with a transport failure, a 429 or transient server error (500, 502, 503, 504, 529), or, before any reply text arrived, a stream that died or went silent, or a stream or reply the provider failed with a rate limit, an overload, or a server fault. The wait is the backoff, or the response's `Retry-After` in seconds when that is longer. A `Retry-After` of more than a minute is not retried: the request fails at once and the error names the server's wait. Neither is a 429 reporting an exhausted quota (`insufficient_quota`). Emitted before the wait; a `cancel` during it ends the turn and the attempt never goes out. `thinking` lines already emitted for that attempt are void and the reply starts over; no `token` line precedes a retried stream. A request gives up before the budget when three attempts in a row never reached the endpoint) | | `diff` | `call_id`, `path`, `diff`, `added`, `removed` | | `approval_request` | `approval_id`, `name`, `summary`, `detail`, `reason` (`gate`, or `unapproved_source` which unattended clients must never auto-approve), `source_path`, `source_sha`, and optional `env` (see below) | | `approval_settled` | `approval_id`, `outcome` (`approved`, `declined`, `timed_out`, or `cancelled`) | From 68031901f077633563c546429c0806d70d6235a7 Mon Sep 17 00:00:00 2001 From: Max Loffgren Date: Mon, 5 Oct 2026 22:36:42 -0500 Subject: [PATCH 2/4] fix(core): word the withheld-key hint and silence docs to match behavior The 401 hint for a provider denied the settings key began "no key was sent", which is false for a provider that authenticates through its own headers entry, and sent its user toward the wrong fix. It now says the settings key was not sent, which holds however the provider authenticates. The configuration docs and the providers spec said a silent endpoint is resent whenever no reply text has arrived, but a one-shot JSON reply or an error body that goes silent after its headers fails at once. Both now say only a missing response or a stream silent before reply text is resent. --- crates/core/src/client.rs | 6 +++--- crates/core/src/providers.rs | 11 +++++++---- crates/core/src/spec.rs | 5 +++-- docs/configuration.md | 5 +++-- 4 files changed, 16 insertions(+), 11 deletions(-) diff --git a/crates/core/src/client.rs b/crates/core/src/client.rs index 4fd76578..796c2385 100644 --- a/crates/core/src/client.rs +++ b/crates/core/src/client.rs @@ -301,7 +301,7 @@ pub struct ChatClient { pub send_stream_options: bool, /// How long the endpoint may send nothing before an attempt ends. idle_timeout: std::time::Duration, - /// Why no key was sent though settings configure one, for a 401 to + /// Why the settings key was withheld from this endpoint, for a 401 to /// report (see `ActiveEndpoint::key_hint`). key_hint: Option, http: reqwest::Client, @@ -1763,8 +1763,8 @@ mod tests { let served = Arc::new(std::sync::Mutex::new(0usize)); let count = served.clone(); std::thread::spawn(move || { - // Connections a SILENT or STALL: body holds open, until the - // sequence ends. + // Connections that a SILENT or STALL: body leaves open, held + // until the sequence ends. let mut held = Vec::new(); for sse in bodies { let Ok((mut stream, _)) = listener.accept() else { return }; diff --git a/crates/core/src/providers.rs b/crates/core/src/providers.rs index 7283399a..28c9794f 100644 --- a/crates/core/src/providers.rs +++ b/crates/core/src/providers.rs @@ -116,8 +116,9 @@ pub struct ActiveEndpoint { /// The provider's `idle_timeout_secs`, when it sets one. pub idle_timeout_secs: Option, /// Set when a key is configured in settings but withheld from this - /// endpoint because it is another server: why no key was sent and how to - /// give the provider its own, for the error a 401 from it reports. + /// endpoint because it is another server: that the settings key was not + /// sent and how to give the provider its own, for the error a 401 from + /// it reports. pub key_hint: Option, } @@ -641,14 +642,16 @@ fn same_server(a: &str, b: &str) -> bool { } /// What a 401 from provider `name` adds when the settings key was withheld -/// from it: why no key went out, and how to give the provider its own. +/// from it: that the settings key was not sent, and how to give the provider +/// its own. It must hold however the provider authenticates, since one that +/// sends a credential through its own `headers` gets this hint too. fn withheld_key_hint(name: &str, p: &ProviderConfig) -> String { let fix = match p.api_key_env.first() { Some(var) => format!("export {var}, which provider '{name}' reads its key from"), None => format!("give provider '{name}' its own key with api_key_env in ~/.openmax/providers.json"), }; format!( - " (no key was sent: the key in settings.json or OPENMAX_API_KEY goes only to the server settings.base_url names, by scheme, host, and port; {fix})" + " (the key in settings.json or OPENMAX_API_KEY was not sent: it goes only to the server settings.base_url names, by scheme, host, and port; {fix})" ) } diff --git a/crates/core/src/spec.rs b/crates/core/src/spec.rs index 299aae08..1205735a 100644 --- a/crates/core/src/spec.rs +++ b/crates/core/src/spec.rs @@ -651,8 +651,9 @@ Shape: `{"providers": {"": { ... }}}`. Per provider: defaults. - `idle_timeout_secs` (optional, default 600): how long the endpoint may send nothing at all (no headers, no bytes, no SSE keepalive comment) before the - attempt ends; before any reply text it is resent. Raise it for a local - server that works through a long prompt silently for longer. + attempt ends; if no response or no reply text has arrived yet, it is + resent. Raise it for a local server that works through a long prompt + silently for longer. A key or Authorization header never goes over plain http to another machine: such a request fails with an error before anything is sent. Use https, or a diff --git a/docs/configuration.md b/docs/configuration.md index f4086ae2..78476b74 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -160,8 +160,9 @@ says how to give that provider its own. A request has no overall deadline, since a local server can spend minutes on a long prompt, but an endpoint that sends nothing at all (no response headers, no reply bytes, not even an SSE keepalive comment) for 10 minutes ends the -attempt. Before any reply text has arrived it is resent like a dropped -connection; after, the reply is reported truncated. A provider's +attempt. If no response arrives, or a stream goes silent before any reply +text, the attempt is resent like a dropped connection; a stream that goes +silent after reply text is reported truncated. A provider's `idle_timeout_secs` sets a different interval for that provider. Open Max works with local servers (Ollama, LM Studio, vLLM, llama.cpp), cloud From 65883c72ca63a5b71f569299d16d650159aad70b Mon Sep 17 00:00:00 2001 From: Max Loffgren Date: Mon, 5 Oct 2026 22:48:42 -0500 Subject: [PATCH 3/4] fix(core): refuse base_url credentials over plain http A base_url with userinfo, such as http://user:secret@192.168.1.5:8000/v1, still went out over plain http to another machine: the HTTP client turns URL userinfo into a Basic Authorization header, and the plain http check only looked at the key and the provider's headers. The check now counts a username or password in the base_url as a credential, and the refusal names it among the sources to remove. The --spec providers entry for idle_timeout_secs now matches the client: a silent one-shot reply is not resent, and 0 never disables the timeout. --- crates/core/src/client.rs | 32 ++++++++++++++++++++------------ crates/core/src/spec.rs | 15 +++++++++------ docs/configuration.md | 10 ++++++---- 3 files changed, 35 insertions(+), 22 deletions(-) diff --git a/crates/core/src/client.rs b/crates/core/src/client.rs index 796c2385..fbb373ba 100644 --- a/crates/core/src/client.rs +++ b/crates/core/src/client.rs @@ -41,8 +41,8 @@ //! reported as truncated rather than treated as a complete reply. //! //! A credential never crosses plain http to another machine: a request that -//! would carry the key or an Authorization header that way is refused -//! before it is sent. +//! would carry the key, an Authorization header, or user:password from the +//! base_url that way is refused before it is sent. use std::collections::HashMap; use std::sync::Arc; @@ -471,11 +471,15 @@ impl ChatClient { format!("{}/chat/completions", self.base_url.trim_end_matches('/')) } - /// Whether a request carries a credential: the key, or an Authorization - /// header among the provider's headers. + /// Whether a request carries a credential: the key, an Authorization + /// header among the provider's headers, or user:password in the + /// base_url, which the HTTP client turns into a Basic Authorization + /// header. fn sends_credential(&self) -> bool { self.api_key.as_deref().is_some_and(|key| !key.is_empty()) || self.headers.iter().any(|(name, _)| name.eq_ignore_ascii_case("authorization")) + || reqwest::Url::parse(self.base_url.trim()) + .is_ok_and(|url| !url.username().is_empty() || url.password().is_some()) } /// The stamp for reasoning this endpoint produces: the first 16 hex chars @@ -1304,7 +1308,7 @@ fn plain_http_refusal(base_url: &str) -> Option { }); (!this_machine).then(|| { format!( - "refusing to send credentials over plain http to {host}, where anyone on the network path can read them: use an https base_url, or a loopback address (127.0.0.1, ::1, or localhost) for a server on this machine. A server that needs no key works over http once none is configured for it (api_key, api_key_env, OPENMAX_API_KEY, or an Authorization header)" + "refusing to send credentials over plain http to {host}, where anyone on the network path can read them: use an https base_url, or a loopback address (127.0.0.1, ::1, or localhost) for a server on this machine. A server that needs no key works over http once none is configured for it (api_key, api_key_env, OPENMAX_API_KEY, an Authorization header, or user:password in the base_url)" ) }) } @@ -2613,16 +2617,19 @@ mod tests { /// The bug this guards: the key went out as a bearer header over plain /// http to any host, readable anywhere on the path. A request that would - /// carry a credential (the key, or an Authorization header) over http to - /// another machine is refused before anything is sent, naming the fix; - /// over loopback, to a server on this machine, it goes out. + /// carry a credential (the key, an Authorization header, or user:password + /// in the base_url, which the HTTP client sends as Basic auth) over http + /// to another machine is refused before anything is sent, naming the + /// fix; over loopback, to a server on this machine, it goes out. #[tokio::test] async fn a_key_never_crosses_plain_http_to_another_machine() { - for (key, headers) in [ - (Some("sk-test".to_string()), Vec::new()), - (None, vec![("Authorization".to_string(), "Bearer sk-test".to_string())]), + for (base_url, key, headers) in [ + ("http://models.example.invalid/v1", Some("sk-test".to_string()), Vec::new()), + ("http://models.example.invalid/v1", None, vec![("Authorization".to_string(), "Bearer sk-test".to_string())]), + ("http://user:secret@models.example.invalid/v1", None, Vec::new()), + ("http://:secret@models.example.invalid/v1", None, Vec::new()), ] { - let mut client = ChatClient::new("http://models.example.invalid/v1".into(), key, "m".into(), None, 64); + let mut client = ChatClient::new(base_url.into(), key, "m".into(), None, 64); client.headers = headers; let mut retries = 0; let err = client @@ -2632,6 +2639,7 @@ mod tests { .expect("a key over plain http to another machine is refused"); assert!(err.starts_with("refusing to send"), "{err}"); assert!(err.contains("models.example.invalid") && err.contains("https") && err.contains("127.0.0.1"), "{err}"); + assert!(err.contains("user:password in the base_url") && !err.contains("secret"), "{err}"); assert_eq!(retries, 0, "nothing was sent, so nothing was resent"); } diff --git a/crates/core/src/spec.rs b/crates/core/src/spec.rs index 1205735a..b5a376a0 100644 --- a/crates/core/src/spec.rs +++ b/crates/core/src/spec.rs @@ -649,15 +649,18 @@ Shape: `{"providers": {"": { ... }}}`. Per provider: - `compat` (optional): wire quirks for picky servers: `{"use_max_completion_tokens": false, "send_stream_options": true}` are the defaults. -- `idle_timeout_secs` (optional, default 600): how long the endpoint may send +- `idle_timeout_secs` (optional, default 600): a positive number of seconds + (`--check` rejects 0; it never disables the timeout) the endpoint may send nothing at all (no headers, no bytes, no SSE keepalive comment) before the - attempt ends; if no response or no reply text has arrived yet, it is - resent. Raise it for a local server that works through a long prompt + attempt ends. If no response has arrived, or a stream goes silent before + any reply text, it is resent; a stream silent after reply text is reported + truncated. Raise it for a local server that works through a long prompt silently for longer. -A key or Authorization header never goes over plain http to another machine: -such a request fails with an error before anything is sent. Use https, or a -loopback address (127.0.0.1, ::1, localhost) for a server on this machine. +A key, Authorization header, or user:password in `base_url` never goes over +plain http to another machine: such a request fails with an error before +anything is sent. Use https, or a loopback address (127.0.0.1, ::1, +localhost) for a server on this machine. Select a provider with `"provider"` in settings.json, the `--provider` CLI option, or `/provider`; `/model` picks provider and model as one pair. This diff --git a/docs/configuration.md b/docs/configuration.md index 78476b74..5882ce41 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -19,9 +19,10 @@ optional `~/.openmax/providers.json` for a catalog of named endpoints. `base_url` is the root of your model's HTTP API (the harness calls `chat/completions` on it). Set `model` to the id that server expects. Set `api_key` to a literal or `$ENV_VAR`, or export `OPENMAX_API_KEY`. A key never -crosses plain http to another machine: with a key configured and an `http://` -`base_url` that is not a loopback address (`127.0.0.1`, `::1`, or `localhost`), -every request fails with an error instead of sending it. Use https, or a +crosses plain http to another machine: with a key (or `user:password` in the +`base_url`) configured and an `http://` `base_url` that is not a loopback +address (`127.0.0.1`, `::1`, or `localhost`), every request fails with an +error instead of sending it. Use https, or a loopback address for a server on this machine; a server that needs no key works over http once none is configured. `max_parallel_tools` bounds concurrent read-only tool calls, defaults to 4, and @@ -163,7 +164,8 @@ reply bytes, not even an SSE keepalive comment) for 10 minutes ends the attempt. If no response arrives, or a stream goes silent before any reply text, the attempt is resent like a dropped connection; a stream that goes silent after reply text is reported truncated. A provider's -`idle_timeout_secs` sets a different interval for that provider. +`idle_timeout_secs` sets a different interval for that provider, in seconds +(0 does not disable it, and `--check` rejects it). Open Max works with local servers (Ollama, LM Studio, vLLM, llama.cpp), cloud gateways (OpenRouter and similar), and private proxies. From 49390a63ced71cadefcc9a5c3f056ea462c24fc2 Mon Sep 17 00:00:00 2001 From: Max Loffgren Date: Mon, 5 Oct 2026 23:08:45 -0500 Subject: [PATCH 4/4] fix(core): guard credential headers and keep redirects on the configured server Only a header named Authorization counted as a credential, so a key a provider sends in X-API-Key, api-key, or a token header still went out over plain http to another machine. And the HTTP client followed any redirect: on a move to another host it drops Authorization and cookies but keeps every other header and, on a 307 or 308, the body, so an endpoint that redirected handed such a key and the transcript to a server nobody configured. A header now counts as a credential when its name contains auth, key, token, secret, password, credential, or cookie, so the plain-http refusal covers it. The shared HTTP client follows a redirect only on the scheme, host, and port the request was sent to; one to another server fails the request with an error naming that server, and nothing reaches it. --- crates/core/src/client.rs | 129 ++++++++++++++++++++++++++++++++++---- crates/core/src/spec.rs | 11 ++-- docs/configuration.md | 10 ++- 3 files changed, 132 insertions(+), 18 deletions(-) diff --git a/crates/core/src/client.rs b/crates/core/src/client.rs index fbb373ba..731ea514 100644 --- a/crates/core/src/client.rs +++ b/crates/core/src/client.rs @@ -41,8 +41,11 @@ //! reported as truncated rather than treated as a complete reply. //! //! A credential never crosses plain http to another machine: a request that -//! would carry the key, an Authorization header, or user:password from the -//! base_url that way is refused before it is sent. +//! would carry the key, a credential header (Authorization, X-API-Key, or any +//! header named as one, see `is_credential_header`), or user:password from +//! the base_url that way is refused before it is sent. Nor does a request +//! leave the server the base_url names: a redirect is followed only on the +//! same scheme, host, and port, and one anywhere else fails the request. use std::collections::HashMap; use std::sync::Arc; @@ -448,6 +451,7 @@ impl ChatClient { .connect_timeout(std::time::Duration::from_secs(10)) // No overall timeout: local generations can legitimately // take minutes. `stream_chat` ends an attempt on silence. + .redirect(reqwest::redirect::Policy::custom(same_server_redirect)) .build() .expect("failed to build http client") }) @@ -471,13 +475,13 @@ impl ChatClient { format!("{}/chat/completions", self.base_url.trim_end_matches('/')) } - /// Whether a request carries a credential: the key, an Authorization - /// header among the provider's headers, or user:password in the - /// base_url, which the HTTP client turns into a Basic Authorization - /// header. + /// Whether a request carries a credential: the key, a credential header + /// among the provider's headers (see [`is_credential_header`]), or + /// user:password in the base_url, which the HTTP client turns into a + /// Basic Authorization header. fn sends_credential(&self) -> bool { self.api_key.as_deref().is_some_and(|key| !key.is_empty()) - || self.headers.iter().any(|(name, _)| name.eq_ignore_ascii_case("authorization")) + || self.headers.iter().any(|(name, _)| is_credential_header(name)) || reqwest::Url::parse(self.base_url.trim()) .is_ok_and(|url| !url.username().is_empty() || url.password().is_some()) } @@ -1308,11 +1312,47 @@ fn plain_http_refusal(base_url: &str) -> Option { }); (!this_machine).then(|| { format!( - "refusing to send credentials over plain http to {host}, where anyone on the network path can read them: use an https base_url, or a loopback address (127.0.0.1, ::1, or localhost) for a server on this machine. A server that needs no key works over http once none is configured for it (api_key, api_key_env, OPENMAX_API_KEY, an Authorization header, or user:password in the base_url)" + "refusing to send credentials over plain http to {host}, where anyone on the network path can read them: use an https base_url, or a loopback address (127.0.0.1, ::1, or localhost) for a server on this machine. A server that needs no key works over http once none is configured for it (api_key, api_key_env, OPENMAX_API_KEY, a credential header such as Authorization or X-API-Key, or user:password in the base_url)" ) }) } +/// Whether a configured header carries a credential, judged by its name: +/// Authorization and Proxy-Authorization, a cookie, or a key, token, secret, +/// password, or credential under whatever name the server picked (X-API-Key, +/// api-key, X-Auth-Token). Such a header is read off plain http as easily as +/// a bearer key, so a check for Authorization alone would let a key in +/// X-API-Key cross it. +fn is_credential_header(name: &str) -> bool { + let name = name.to_ascii_lowercase(); + ["auth", "key", "token", "secret", "password", "credential", "cookie"] + .iter() + .any(|word| name.contains(word)) +} + +/// The redirect policy of the shared HTTP client: follow a redirect only +/// within the server the request was sent to (scheme, host, and port). The +/// HTTP client drops Authorization and cookies on a move to another host but +/// keeps every other header and, on a 307 or 308, the body, so following one +/// would hand a key in X-API-Key, and the transcript, to a server nobody +/// configured, or move them onto plain http. +fn same_server_redirect(attempt: reqwest::redirect::Attempt) -> reqwest::redirect::Action { + let server = |url: &reqwest::Url| (url.scheme().to_string(), url.host_str().map(str::to_string), url.port_or_known_default()); + let next = server(attempt.url()); + let to = attempt.url().origin().ascii_serialization(); + if attempt.previous().len() > 10 { + // The default policy's limit, for a server that redirects to itself. + attempt.error("too many redirects") + } else if attempt.previous().first().map(server) == Some(next) { + // The first previous URL is the one the request was sent to. + attempt.follow() + } else { + attempt.error(format!( + "refusing to follow a redirect to {to}, a server other than the one base_url names: a request and its credentials go only to that server, by scheme, host, and port. If {to} is the endpoint, point base_url at it" + )) + } +} + fn is_transient_transport(err: &reqwest::Error) -> bool { err.is_connect() || err.is_timeout() || err.is_request() } @@ -2617,15 +2657,17 @@ mod tests { /// The bug this guards: the key went out as a bearer header over plain /// http to any host, readable anywhere on the path. A request that would - /// carry a credential (the key, an Authorization header, or user:password - /// in the base_url, which the HTTP client sends as Basic auth) over http - /// to another machine is refused before anything is sent, naming the - /// fix; over loopback, to a server on this machine, it goes out. + /// carry a credential (the key, a credential header such as Authorization + /// or X-API-Key, or user:password in the base_url, which the HTTP client + /// sends as Basic auth) over http to another machine is refused before + /// anything is sent, naming the fix; over loopback, to a server on this + /// machine, it goes out. #[tokio::test] async fn a_key_never_crosses_plain_http_to_another_machine() { for (base_url, key, headers) in [ ("http://models.example.invalid/v1", Some("sk-test".to_string()), Vec::new()), ("http://models.example.invalid/v1", None, vec![("Authorization".to_string(), "Bearer sk-test".to_string())]), + ("http://models.example.invalid/v1", None, vec![("X-API-Key".to_string(), "sk-test".to_string())]), ("http://user:secret@models.example.invalid/v1", None, Vec::new()), ("http://:secret@models.example.invalid/v1", None, Vec::new()), ] { @@ -2680,6 +2722,69 @@ mod tests { } } + /// A header is a credential by its name, whatever name the server picked + /// for its key or token; routing and attribution headers are not, so a + /// keyless server that wants one still works over http. + #[test] + fn a_credential_header_is_known_by_its_name() { + let sends = |name: &str| { + let mut client = ChatClient::new("http://models.example.invalid/v1".into(), None, "m".into(), None, 64); + client.headers = vec![(name.to_string(), "value".to_string())]; + client.sends_credential() + }; + for name in [ + "Authorization", + "Proxy-Authorization", + "X-API-Key", + "api-key", + "X-Auth-Token", + "X-Access-Token", + "Cookie", + "X-Client-Secret", + "X-Upstream-Password", + ] { + assert!(sends(name), "{name}"); + } + for name in ["X-Route", "HTTP-Referer", "X-Title", "User-Agent", "Accept"] { + assert!(!sends(name), "{name}"); + } + } + + /// The bug this guards: the HTTP client followed any redirect, and on a + /// move to another host it drops only Authorization and cookies, so an + /// endpoint that redirected sent the request on, with a key in X-API-Key + /// or any other header, to a server nobody configured. A redirect is + /// followed only within the server the base_url names (scheme, host, and + /// port); one anywhere else fails the request, and nothing reaches that + /// server. + #[tokio::test] + async fn a_redirect_never_carries_a_request_to_another_server() { + let (elsewhere, heads) = spawn_recording_once(finished_response()); + let elsewhere = elsewhere.trim_end_matches("/v1").to_string(); + let moved = |location: &str| status_response("307 Temporary Redirect", &format!("Location: {location}\r\n"), ""); + let (url, served) = spawn_sse_sequence(vec![moved(&format!("{elsewhere}/v1/chat/completions"))]); + let mut client = ChatClient::new(url, None, "m".into(), None, 64); + client.headers = vec![("X-API-Key".into(), "sk-test".into())]; + let mut deltas = 0; + let err = client + .stream_chat(&[ChatMessage::user("hi")], "[]", Arc::new(crate::state::CancelToken::default()), |_| deltas += 1) + .await + .err() + .expect("a redirect to another server fails the request"); + assert!(err.contains("redirect") && err.contains(&elsewhere) && !err.contains("sk-test"), "{err}"); + assert!(heads.try_recv().is_err(), "the other server was sent nothing"); + assert_eq!((*served.lock().unwrap(), deltas), (1, 0), "the refusal is not resent"); + + // Within the same server, a moved path is followed. + let (url, served) = spawn_sse_sequence(vec![moved("/v2/chat/completions"), FINISHED.into()]); + let result = ChatClient::new(url, None, "m".into(), None, 64) + .stream_chat(&[ChatMessage::user("hi")], "[]", Arc::new(crate::state::CancelToken::default()), |_| {}) + .await + .expect("a redirect within the server is followed"); + assert_eq!(result.content, "all of it"); + assert_eq!(*served.lock().unwrap(), 2); + } + /// A provider's `idle_timeout_secs` replaces the default interval for /// its requests; a provider without one keeps the default. #[test] diff --git a/crates/core/src/spec.rs b/crates/core/src/spec.rs index b5a376a0..45fa1ccb 100644 --- a/crates/core/src/spec.rs +++ b/crates/core/src/spec.rs @@ -657,10 +657,13 @@ Shape: `{"providers": {"": { ... }}}`. Per provider: truncated. Raise it for a local server that works through a long prompt silently for longer. -A key, Authorization header, or user:password in `base_url` never goes over -plain http to another machine: such a request fails with an error before -anything is sent. Use https, or a loopback address (127.0.0.1, ::1, -localhost) for a server on this machine. +A key, a credential header (one whose name contains auth, key, token, +secret, password, credential, or cookie, such as Authorization or +X-API-Key), or user:password in `base_url` never goes over plain http to +another machine: such a request fails with an error before anything is +sent. Use https, or a loopback address (127.0.0.1, ::1, localhost) for a +server on this machine. A redirect is followed only on the same scheme, +host, and port as `base_url`; one to another server fails the request. Select a provider with `"provider"` in settings.json, the `--provider` CLI option, or `/provider`; `/model` picks provider and model as one pair. This diff --git a/docs/configuration.md b/docs/configuration.md index 5882ce41..80281a44 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -24,7 +24,10 @@ crosses plain http to another machine: with a key (or `user:password` in the address (`127.0.0.1`, `::1`, or `localhost`), every request fails with an error instead of sending it. Use https, or a loopback address for a server on this machine; a server that needs no key works -over http once none is configured. +over http once none is configured. A redirect is followed only on the same +scheme, host, and port as `base_url`: one to another server fails the request +with an error naming that server, since following it would hand the request +and its headers to a server you did not configure. `max_parallel_tools` bounds concurrent read-only tool calls, defaults to 4, and is clamped to 1 through 32 at runtime. Mutating, approval-gated, and non-batchable calls remain serial. @@ -156,7 +159,10 @@ A provider takes its key from its own `api_key` or `api_key_env`. Without one, it uses the settings `api_key` (or `OPENMAX_API_KEY`) only when its `base_url` has the same scheme, host, and port as the settings `base_url`: that key was configured for that server. Any other provider gets no key, and a 401 from it -says how to give that provider its own. +says how to give that provider its own. A credential in a provider's `headers` +(a header whose name contains `auth`, `key`, `token`, `secret`, `password`, +`credential`, or `cookie`, such as `Authorization` or `X-API-Key`) follows the +same plain-http rule as the key. A request has no overall deadline, since a local server can spend minutes on a long prompt, but an endpoint that sends nothing at all (no response headers, no