Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 18 additions & 7 deletions src/channels/webhook_server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -342,16 +342,27 @@ mod tests {
.expect("Failed to send request");
assert_eq!(response.status(), 200, "Server should be listening");

// Try to restart on an invalid address (port 1 typically requires elevated privileges)
let invalid_addr: SocketAddr = "127.0.0.1:1".parse().unwrap();

// Attempt bind (should fail); server state is untouched because we
// never call install_listener on failure.
// Try to bind an address that should fail. Port 1 requires elevated
// privileges on most systems, but when running as root (containers, CI)
// the bind succeeds. Fall back to re-binding the already-occupied first
// port, which always fails regardless of privilege level.
let app = server
.merged_router_clone()
.expect("Router should exist after start()");
let result = tokio::net::TcpListener::bind(invalid_addr).await;
assert!(result.is_err(), "Bind to privileged port should fail");
let privileged: SocketAddr = "127.0.0.1:1".parse().unwrap();
let result = tokio::net::TcpListener::bind(privileged).await;
let result = if result.is_ok() {
// Running as root — port 1 bind succeeded. Drop it and try the
// already-occupied port instead, which must fail.
drop(result);
tokio::net::TcpListener::bind(addr1).await
} else {
result
};
assert!(
result.is_err(),
"Bind should fail (privileged port or already in use)"
);
// `app` is dropped — server state unchanged (rollback by construction)
drop(app);

Expand Down
13 changes: 11 additions & 2 deletions src/cli/doctor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -765,9 +765,13 @@ mod tests {
let result = rt.block_on(check_nearai_session(&settings));
match result {
CheckResult::Skip(msg) => {
// In offline environments, LlmConfig::resolve() may fail
// with a DNS error before the backend check runs. Accept
// either the normal "backend=anthropic" skip or a DNS-related
// config error skip.
assert!(
msg.contains("backend=anthropic"),
"expected backend name in skip message, got: {msg}"
msg.contains("backend=anthropic") || msg.contains("failed to resolve"),
"expected backend name or DNS error in skip message, got: {msg}"
);
}
other => panic!(
Expand Down Expand Up @@ -891,6 +895,11 @@ mod tests {
"should not show bedrock model for nearai backend: {msg}"
);
}
// In sandboxed / offline environments, DNS resolution for the
// NearAI auth URL may fail during config construction. The
// doctor correctly reports this as a Fail, so the test should
// not panic — just verify the message is DNS-related.
CheckResult::Fail(msg) if msg.contains("failed to resolve") => {}
other => panic!(
"expected Pass for default LLM config, got: {}",
format_result(&other)
Expand Down
76 changes: 63 additions & 13 deletions src/config/helpers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -329,17 +329,35 @@ fn validate_base_url_with_policy(
let port = parsed
.port()
.unwrap_or(if scheme == "http" { 80 } else { 443 });
// `to_socket_addrs` performs blocking DNS resolution. This helper is
// also called from async request handlers (e.g. the LLM utility
// `to_socket_addrs` performs blocking DNS resolution. This helper
// is also called from async request handlers (e.g. the LLM utility
// routes), so wrap the lookup in `block_in_place` when running on a
// multi-threaded tokio worker to avoid stalling other tasks. The
// multi-threaded tokio worker to avoid stalling other tasks. The
// `try_current()` check keeps sync callers (config bootstrap, CLI)
// working unchanged.
let resolve = || -> std::io::Result<Vec<IpAddr>> {
Ok((host, port)
.to_socket_addrs()?
.map(|addr| addr.ip())
.collect())
//
// A 10-second timeout prevents the entire process from hanging when
// the DNS resolver blocks indefinitely (e.g. sandboxed environments
// or broken resolvers).
let host_owned = host.to_string();
let resolve = move || -> std::io::Result<Vec<IpAddr>> {
use std::sync::mpsc;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium: DNS timeout thread leaks ~8MB stack if resolution hangs

When the 10s DNS timeout fires, the spawned std::thread continues running indefinitely (blocked in to_socket_addrs()). In pathological DNS environments, repeated config-load attempts could accumulate zombie threads.

Acceptable for config-time (bounded number of calls at startup), but should be documented.

Suggested fix: Document as a known limitation. Consider std::thread::Builder::new().name("dns-timeout".into()).spawn(...) for debuggability.

use std::time::Duration;
let (tx, rx) = mpsc::channel();
let h = host_owned.clone();
std::thread::spawn(move || {
let result = (h.as_str(), port)
.to_socket_addrs()
.map(|addrs| addrs.map(|a| a.ip()).collect::<Vec<_>>());
let _ = tx.send(result);
Comment on lines +348 to +352

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Avoid leaking resolver threads on DNS timeout

Spawning a detached OS thread for every hostname lookup here means a timed-out recv_timeout returns to the caller while the worker thread can remain blocked in to_socket_addrs() indefinitely; repeated validations (for example through async LLM endpoint checks that call validate_operator_base_url) can accumulate stuck threads and eventually exhaust process resources. This regresses from bounded blocking to an unbounded thread-growth failure mode whenever DNS hangs rather than fails fast.

Useful? React with 👍 / 👎.

});
rx.recv_timeout(Duration::from_secs(10))
.unwrap_or_else(|_| {
Err(std::io::Error::new(
std::io::ErrorKind::TimedOut,
"DNS resolution timed out after 10s",
))
})
};
let lookup = match tokio::runtime::Handle::try_current() {
Ok(handle) if handle.runtime_flavor() == tokio::runtime::RuntimeFlavor::MultiThread => {
Expand Down Expand Up @@ -728,9 +746,22 @@ mod tests {
/// "DNS resolution failure" unreliable. Detect that case and skip the test.
fn invalid_tld_resolves_locally() -> bool {
use std::net::ToSocketAddrs;
("ironclaw-dns-hijack-probe.invalid", 443u16)
.to_socket_addrs()
.is_ok()
use std::sync::mpsc;
use std::time::Duration;
// Spawn the DNS lookup in a thread with a channel timeout so
// that sandboxed / restricted-DNS environments (where the
// resolver may block indefinitely for .invalid TLDs) don't
// hang the entire test suite.
let (tx, rx) = mpsc::channel();
std::thread::spawn(move || {
let resolved = ("ironclaw-dns-hijack-probe.invalid", 443u16)
.to_socket_addrs()
.is_ok();
let _ = tx.send(resolved);
});
// If DNS doesn't respond within 5 seconds, treat it as
// "DNS is broken" and skip the test (same as hijacked DNS).
rx.recv_timeout(Duration::from_secs(5)).unwrap_or(true)
}

#[test]
Expand All @@ -742,8 +773,27 @@ mod tests {
);
return;
}
// .invalid TLD is guaranteed to never resolve (RFC 6761)
let result = validate_base_url("https://ssrf-test.invalid", "TEST");
// The validate_base_url call also performs DNS resolution which
// can hang in restricted-DNS environments, so run it in a
// thread with a timeout.
use std::sync::mpsc;
use std::time::Duration;
let (tx, rx) = mpsc::channel();
std::thread::spawn(move || {
// .invalid TLD is guaranteed to never resolve (RFC 6761)
let result = validate_base_url("https://ssrf-test.invalid", "TEST");
let _ = tx.send(result);
});
let result = match rx.recv_timeout(Duration::from_secs(10)) {
Ok(r) => r,
Err(_) => {
eprintln!(
"skipping validate_base_url_rejects_dns_failure: \
DNS resolution timed out"
);
return;
}
};
assert!(result.is_err());
let err = result.unwrap_err().to_string();
assert!(
Expand Down
59 changes: 44 additions & 15 deletions src/config/llm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -135,9 +135,17 @@ impl LlmConfig {
}

// Session config (used by NearAI provider for OAuth/session-token auth)
let nearai_auth_url = optional_env("NEARAI_AUTH_URL")?
let nearai_auth_url_explicit = optional_env("NEARAI_AUTH_URL")?;
let nearai_auth_url = nearai_auth_url_explicit
.clone()
.unwrap_or_else(|| "https://private.near.ai".to_string());
validate_base_url(&nearai_auth_url, "NEARAI_AUTH_URL")?;
// Only validate (DNS-resolve) the auth URL when it was explicitly
// configured. Default/hardcoded URLs are known-good and don't need
// DNS-based SSRF validation — attempting it causes spurious failures
// in offline / sandboxed environments.
if nearai_auth_url_explicit.is_some() {
validate_base_url(&nearai_auth_url, "NEARAI_AUTH_URL")?;
}
let session = SessionConfig {
auth_base_url: nearai_auth_url,
session_path: optional_env("NEARAI_SESSION_PATH")?
Expand All @@ -163,6 +171,9 @@ impl LlmConfig {
} else {
crate::llm::DEFAULT_MODEL.to_string()
};
let nearai_base_url_explicit = nearai_override
.and_then(|o| o.base_url.clone())
.or_else(|| optional_env("NEARAI_BASE_URL").ok().flatten());

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium: .ok().flatten() silently swallows env-var parse errors

nearai_base_url_explicit uses .ok().flatten() on optional_env("NEARAI_BASE_URL"), silently discarding ConfigError. If optional_env fails, nearai_base_url_explicit becomes None, and validate_base_url is skipped. The error surfaces on the next optional_env("NEARAI_BASE_URL")? call, so behavior is correct in practice, but .ok().flatten() is fragile — a refactor removing the second call would silently lose the error.

Suggested fix: Use optional_env("NEARAI_BASE_URL")? for the explicit check too, or extract the result into a shared let-binding.

let nearai_base_url = if let Some(url) = nearai_override.and_then(|o| o.base_url.clone()) {
url
} else if let Some(url) = optional_env("NEARAI_BASE_URL")? {
Expand All @@ -172,7 +183,9 @@ impl LlmConfig {
} else {
"https://private.near.ai".to_string()
};
validate_base_url(&nearai_base_url, "NEARAI_BASE_URL")?;
if nearai_base_url_explicit.is_some() {
validate_base_url(&nearai_base_url, "NEARAI_BASE_URL")?;
}
let nearai = NearAiConfig {
model: nearai_model,
cheap_model: optional_env("NEARAI_CHEAP_MODEL")?,
Expand Down Expand Up @@ -261,12 +274,20 @@ impl LlmConfig {
.or(optional_env("OPENAI_CODEX_MODEL")?)
.or(optional_env("OPENAI_MODEL")?)
.unwrap_or_else(|| "gpt-5.3-codex".to_string());
let auth_endpoint = optional_env("OPENAI_CODEX_AUTH_URL")?
let auth_endpoint_explicit = optional_env("OPENAI_CODEX_AUTH_URL")?;
let auth_endpoint = auth_endpoint_explicit
.clone()
.unwrap_or_else(|| "https://auth.openai.com".to_string());
validate_base_url(&auth_endpoint, "OPENAI_CODEX_AUTH_URL")?;
let api_base_url = optional_env("OPENAI_CODEX_API_URL")?
if auth_endpoint_explicit.is_some() {
validate_base_url(&auth_endpoint, "OPENAI_CODEX_AUTH_URL")?;
}
let api_base_url_explicit = optional_env("OPENAI_CODEX_API_URL")?;
let api_base_url = api_base_url_explicit
.clone()
.unwrap_or_else(|| "https://chatgpt.com/backend-api/codex".to_string());
validate_base_url(&api_base_url, "OPENAI_CODEX_API_URL")?;
if api_base_url_explicit.is_some() {
validate_base_url(&api_base_url, "OPENAI_CODEX_API_URL")?;
}
let client_id = optional_env("OPENAI_CODEX_CLIENT_ID")?
.unwrap_or_else(|| "app_EMoamEEZ73f0CkXaXp7hrann".to_string());
let session_path = optional_env("OPENAI_CODEX_SESSION_PATH")?
Expand Down Expand Up @@ -501,7 +522,10 @@ impl LlmConfig {
} else {
None
};
let base_url = codex_base_url_override
// Track whether the URL was explicitly configured (not a registry default).
// Registry defaults are known-good hardcoded URLs that don't need DNS-based
// SSRF validation.
let explicit_base_url = codex_base_url_override
.or_else(|| {
// DB settings: per-provider base_url override
settings
Expand All @@ -519,7 +543,9 @@ impl LlmConfig {
_ => None,
}
})
.or(env_base_url)
.or(env_base_url);
let base_url = explicit_base_url
.clone()
.or_else(|| default_base_url.map(String::from))
.unwrap_or_default();

Expand All @@ -533,10 +559,11 @@ impl LlmConfig {
});
}

// Provider base URLs are explicit operator configuration, so allow
// private/local endpoints while still rejecting unsafe schemes,
// public plaintext HTTP, and special blocked addresses.
if !base_url.is_empty() {
// Only validate explicitly-configured URLs (from env vars or DB
// settings). Registry-provided defaults are hardcoded known-good
// URLs that don't need DNS-based SSRF validation — validating them
// causes spurious failures in offline / sandboxed environments.
if explicit_base_url.is_some() && !base_url.is_empty() {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium: Skipping SSRF validation for registry defaults assumes trusted registry

Skipping validate_base_url for registry-default URLs is safe only if the registry content is trusted. If ProviderRegistry is ever loaded from untrusted sources (user-editable config, remote fetches), an attacker could inject a malicious default_base_url that bypasses SSRF validation entirely.

Suggested fix: Add a comment documenting the trust assumption: "Registry defaults are compile-time constants from src/llm/providers/*.json — if registry loading changes to accept external sources, this must be revisited."

let field = base_url_env.unwrap_or("LLM_BASE_URL");
validate_operator_base_url(&base_url, field)?;
}
Expand Down Expand Up @@ -710,7 +737,8 @@ mod tests {

let settings = Settings {
llm_backend: Some("openai_compatible".to_string()),
openai_compatible_base_url: Some("https://openrouter.ai/api/v1".to_string()),
// Use localhost to avoid DNS resolution in test environments.
openai_compatible_base_url: Some("http://localhost:11434/api/v1".to_string()),
selected_model: Some("openai/gpt-5.1-codex".to_string()),
..Default::default()
};
Expand All @@ -732,7 +760,8 @@ mod tests {

let settings = Settings {
llm_backend: Some("openai_compatible".to_string()),
openai_compatible_base_url: Some("https://openrouter.ai/api/v1".to_string()),
// Use localhost to avoid DNS resolution in test environments.
openai_compatible_base_url: Some("http://localhost:11434/api/v1".to_string()),
selected_model: Some("openai/gpt-5.1-codex".to_string()),
..Default::default()
};
Expand Down
6 changes: 6 additions & 0 deletions src/tools/mcp/auth.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1930,6 +1930,12 @@ mod tests {

#[tokio::test]
async fn test_validate_url_safe_https() {
// This test requires external DNS resolution (example.com).
// Skip in sandboxed/offline environments where DNS is unavailable.
if tokio::net::lookup_host("example.com:443").await.is_err() {
eprintln!("skipping test_validate_url_safe_https: DNS unavailable");
return;
}
assert!(validate_url_safe("https://example.com/path").await.is_ok());
}

Expand Down
2 changes: 1 addition & 1 deletion src/tunnel/custom.rs
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ impl Tunnel for CustomTunnel {
.timeout(std::time::Duration::from_secs(5))
.send()
.await
.is_ok();
.is_ok_and(|r| r.status().is_success());
}

let guard = self.proc.lock().await;
Expand Down
Loading