From 66207d24b652f784a291ba4039d42c7b8e4a8cc1 Mon Sep 17 00:00:00 2001 From: hermes Date: Sat, 17 Oct 2026 04:28:30 +0000 Subject: [PATCH] fix(net): bounded request time, curl size and time caps, lock waits, sane prices From the full-repo Codex audit: - MSN and Yahoo clients set connect and body timeouts but none for the response headers, so a server that stalled before answering hung the call; four such calls take every MCP call slot. Add a 30 s overall deadline. Every curl-impersonate run gets --max-time 600 and a 256 MiB --max-filesize, and output above that is rejected (curl cannot cap a response without a length); the Yahoo cookie fetch gets --max-time. - Ownership commands blocked on the database lock without limit, so a long sync or import, or a stuck process, held MCP call slots indefinitely. Wait up to 30 s, then fail with a "busy, try again" error. - A price of 1e20 saturated `as i64` and the change subtraction then overflowed. Prices that are not finite, negative, or above 1e12 IDR are treated as missing, and a quote's own price as an error. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01V3syn2fN2LNo5pvTSqc6eh --- crates/idx-core/src/api/msn/client.rs | 3 ++ crates/idx-core/src/api/msn/map.rs | 55 +++++++++++++++------- crates/idx-core/src/api/yahoo/client.rs | 12 ++++- crates/idx-core/src/api/yahoo/map.rs | 28 ++++++++---- crates/idx-core/src/curl_impersonate.rs | 19 +++++++- crates/idx-core/src/investor.rs | 61 ++++++++++++++++++++++--- 6 files changed, 144 insertions(+), 34 deletions(-) diff --git a/crates/idx-core/src/api/msn/client.rs b/crates/idx-core/src/api/msn/client.rs index f61ef71..5c1fbaf 100644 --- a/crates/idx-core/src/api/msn/client.rs +++ b/crates/idx-core/src/api/msn/client.rs @@ -30,6 +30,9 @@ impl MsnClient { let agent: ureq::Agent = ureq::Agent::config_builder() .timeout_connect(Some(Duration::from_secs(5))) .timeout_recv_body(Some(Duration::from_secs(10))) + // A server that stalls before sending headers must not hold a + // call (and an MCP call slot) forever. + .timeout_global(Some(Duration::from_secs(30))) .build() .into(); diff --git a/crates/idx-core/src/api/msn/map.rs b/crates/idx-core/src/api/msn/map.rs index b47f1a9..d2a51f9 100644 --- a/crates/idx-core/src/api/msn/map.rs +++ b/crates/idx-core/src/api/msn/map.rs @@ -25,11 +25,15 @@ pub(super) fn parse_quote(symbol: &str, quotes: &[MsnQuote]) -> Result Result) -> Option { value.filter(|number| number.is_finite() && *number >= 0.01) } -fn round_price(value: f64) -> i64 { - value.round() as i64 +/// Prices are IDR; anything non-finite, negative, or above this is not a +/// real quote, and treating it as missing keeps arithmetic from overflowing. +const MAX_PRICE: f64 = 1e12; + +fn round_price(value: f64) -> Option { + (value.is_finite() && (0.0..=MAX_PRICE).contains(&value)).then(|| value.round() as i64) } fn round_u64(value: Option) -> Option { @@ -667,11 +675,11 @@ pub(super) fn parse_screener_results(raw: &RawScreenerResponse) -> Result 0.0)?; - let price = round_price(raw_price); - let prev_close = q.price_previous_close.map(round_price); + let price = round_price(raw_price)?; + let prev_close = q.price_previous_close.and_then(round_price); let change = prev_close .map(|pc| price - pc) - .or_else(|| q.price_change.map(round_price)); + .or_else(|| q.price_change.and_then(round_price)); let ticker = q .symbol .as_deref() @@ -698,8 +706,8 @@ pub(super) fn parse_screener_results(raw: &RawScreenerResponse) -> Result], idx: usize) -> Option { .copied() .flatten() .filter(|value| value.is_finite()) - .map(round_price) + .and_then(round_price) } fn series_volume_at(series: &RawChartSeries, idx: usize) -> u64 { @@ -889,6 +896,22 @@ mod tests { assert_eq!(sentiment.statistics[0].neutral, 3); } + #[test] + fn parse_quote_rejects_implausible_prices_instead_of_overflowing() { + let raw: Vec = + serde_json::from_str(r#"[{"symbol":"BBCA","price":1e20,"pricePreviousClose":-1e20}]"#) + .unwrap(); + assert!(parse_quote("BBCA.JK", &raw).is_err()); + + let raw: Vec = serde_json::from_str( + r#"[{"symbol":"BBCA","price":6075.0,"pricePreviousClose":-1e20}]"#, + ) + .unwrap(); + let quote = parse_quote("BBCA.JK", &raw).unwrap(); + assert_eq!(quote.price, 6075); + assert_eq!(quote.prev_close, None); + } + #[test] fn parses_news_fixture_items() { let raw: RawNewsFeed = diff --git a/crates/idx-core/src/api/yahoo/client.rs b/crates/idx-core/src/api/yahoo/client.rs index 6dc3154..2029a84 100644 --- a/crates/idx-core/src/api/yahoo/client.rs +++ b/crates/idx-core/src/api/yahoo/client.rs @@ -30,6 +30,9 @@ impl YahooClient { let agent: ureq::Agent = ureq::Agent::config_builder() .timeout_connect(Some(Duration::from_secs(5))) .timeout_recv_body(Some(Duration::from_secs(10))) + // A server that stalls before sending headers must not hold a + // call (and an MCP call slot) forever. + .timeout_global(Some(Duration::from_secs(30))) .build() .into(); @@ -141,7 +144,14 @@ impl YahooClient { // Step 1: fetch fc.yahoo.com to set A3 cookie (returns 404 but writes cookie jar). // We allow non-zero exit here since 404 still writes the cookie. let _ = Command::new(binary) - .args(["--silent", "--cookie-jar", cookie_jar_str, COOKIE_FETCH_URL]) + .args([ + "--silent", + "--max-time", + "30", + "--cookie-jar", + cookie_jar_str, + COOKIE_FETCH_URL, + ]) .stdout(Stdio::null()) .stderr(Stdio::null()) .output(); diff --git a/crates/idx-core/src/api/yahoo/map.rs b/crates/idx-core/src/api/yahoo/map.rs index 20d8212..75c415f 100644 --- a/crates/idx-core/src/api/yahoo/map.rs +++ b/crates/idx-core/src/api/yahoo/map.rs @@ -20,8 +20,12 @@ pub(super) fn parse_quote(symbol: &str, chart: &ChartResponse) -> Result Result i64 { - value.round() as i64 +/// Prices are IDR; anything non-finite, negative, or above this is not a +/// real quote, and treating it as missing keeps arithmetic from overflowing. +const MAX_PRICE: f64 = 1e12; + +fn round_price(value: f64) -> Option { + (value.is_finite() && (0.0..=MAX_PRICE).contains(&value)).then(|| value.round() as i64) } pub(super) fn map_yahoo_error(symbol: &str, endpoint: &str, err: &ChartError) -> IdxError { diff --git a/crates/idx-core/src/curl_impersonate.rs b/crates/idx-core/src/curl_impersonate.rs index dbabb8c..0b1d69b 100644 --- a/crates/idx-core/src/curl_impersonate.rs +++ b/crates/idx-core/src/curl_impersonate.rs @@ -97,8 +97,19 @@ pub fn run_owned_with_fallback( Err(last_error.expect("at least one profile was tried")) } +/// Every curl run gives up after this long, so a stalled server cannot hang +/// a command or an MCP call. +const MAX_TIME_SECS: &str = "600"; +/// Largest response kept in memory (IDX report files are a few MB). +const MAX_RESPONSE_BYTES: usize = 256 * 1024 * 1024; + fn run_binary(stage: &str, binary: &str, args: &[String]) -> Result { - let output = Command::new(binary).args(args).output().map_err(|e| { + let max_filesize = MAX_RESPONSE_BYTES.to_string(); + let output = Command::new(binary) + .args(["--max-time", MAX_TIME_SECS, "--max-filesize", max_filesize.as_str()]) + .args(args) + .output() + .map_err(|e| { if e.kind() == std::io::ErrorKind::NotFound { return IdxError::Http(format!( "curl-impersonate binary '{binary}' not found; set {OVERRIDE_ENV} or install nixpkgs#curl-impersonate-chrome" @@ -107,6 +118,12 @@ fn run_binary(stage: &str, binary: &str, args: &[String]) -> Result MAX_RESPONSE_BYTES { + return Err(IdxError::Http(format!( + "{stage} response exceeds {MAX_RESPONSE_BYTES} bytes" + ))); + } if output.status.success() { return Ok(output); } diff --git a/crates/idx-core/src/investor.rs b/crates/idx-core/src/investor.rs index 001fdaa..b5012a2 100644 --- a/crates/idx-core/src/investor.rs +++ b/crates/idx-core/src/investor.rs @@ -484,6 +484,14 @@ fn lock_path(db_path: &Path) -> PathBuf { /// not need write access, so readers fall back to opening an existing lock /// file read-only (e.g. one created by another user). fn lock_at(db_path: &Path, exclusive: bool) -> Result { + lock_at_within(db_path, exclusive, LOCK_WAIT) +} + +fn lock_at_within( + db_path: &Path, + exclusive: bool, + wait: std::time::Duration, +) -> Result { let path = lock_path(db_path); let opened = fs::OpenOptions::new() .create(true) @@ -496,15 +504,38 @@ fn lock_at(db_path: &Path, exclusive: bool) -> Result { .map_err(|e| IdxError::Io(format!("failed to open ownership lock: {e}")))?, Err(e) => return Err(IdxError::Io(format!("failed to open ownership lock: {e}"))), }; - if exclusive { - file.lock() - } else { - file.lock_shared() + // Wait a bounded time: a blocking lock behind a long sync or import (or + // a stuck process) would hold every MCP call slot indefinitely. + let deadline = std::time::Instant::now() + wait; + loop { + let attempt = if exclusive { + file.try_lock() + } else { + file.try_lock_shared() + }; + match attempt { + Ok(()) => return Ok(file), + Err(fs::TryLockError::WouldBlock) if std::time::Instant::now() < deadline => { + std::thread::sleep(std::time::Duration::from_millis(100)); + } + Err(fs::TryLockError::WouldBlock) => { + return Err(IdxError::Io(format!( + "ownership database is busy (another idx is syncing or importing); try again in a moment ({})", + path.display() + ))); + } + Err(fs::TryLockError::Error(e)) => { + return Err(IdxError::Io(format!( + "failed to lock ownership database: {e}" + ))); + } + } } - .map_err(|e| IdxError::Io(format!("failed to lock ownership database: {e}")))?; - Ok(file) } +/// How long ownership commands wait for another process's lock. +const LOCK_WAIT: std::time::Duration = std::time::Duration::from_secs(30); + /// True when the lock file is absent and cannot be created. fn unlockable(db_path: &Path) -> bool { let path = lock_path(db_path); @@ -780,3 +811,21 @@ mod tests { assert!(parse_report_family("above10").is_err()); } } + +#[cfg(test)] +mod lock_tests { + use std::time::Duration; + + use super::lock_at_within; + + #[test] + fn a_held_lock_times_out_with_a_busy_error() { + let db = std::env::temp_dir().join(format!("idx-lock-{}.db", fastrand::u64(..))); + let _held = lock_at_within(&db, true, Duration::ZERO).unwrap(); + + let err = lock_at_within(&db, false, Duration::from_millis(200)).unwrap_err(); + + assert!(err.to_string().contains("busy"), "{err}"); + let _ = std::fs::remove_file(super::lock_path(&db)); + } +}