From 60634d3aef1294afa99ebebb9d53630c38b5deb7 Mon Sep 17 00:00:00 2001 From: hermes Date: Wed, 14 Oct 2026 21:58:37 +0000 Subject: [PATCH] fix: address final review (path traversal, sync races, MCP limits) Findings from a Codex (gpt-6.1-sol, xhigh) review of v0.2.4..HEAD, each verified in the code before fixing: - high: ticker symbols reached cache file paths unchecked, so a symbol like `../../REPORT` read (and deleted, if unparseable) .json files outside the cache, reachable through MCP get_quotes. resolve_symbol now accepts only ticker characters, and cache keys are encoded as single safe path components. - high: sync decided before downloading and replaced the database without rechecking; an import that landed meanwhile was erased. It now rechecks the local state before installing, and idx-mcp runs ownership_sync exclusively with ownership reads. - replaces_sqlite_sha256 never downgrades history: the client requires the rebuild to keep at least the local release count, and the publisher lists a replaced build only then. - cache writes are atomic (temp file + rename), so concurrent readers never see, and delete, a partial entry. - idx-mcp caps symbols (20, deduplicated) and limits, and runs at most 4 tool calls at once. - each Yahoo client gets its own cookie jar, paired with its crumb. - ZIP entries (XLSX, KSEI archives) are read with a 256 MiB bound; non-ASCII archive dates and huge percentages are parse errors instead of panics or overflow. - agent-smoke --keep deletes every copied agent config, not only auth files. CLI goldens unchanged; the MCP tool golden changes only to state the new caps. Co-Authored-By: Claude Opus 5.5 --- crates/idx-core/src/api/mod.rs | 33 +++++++ crates/idx-core/src/api/yahoo/client.rs | 25 ++++-- crates/idx-core/src/cache.rs | 51 ++++++++++- crates/idx-core/src/ownership/archive.rs | 21 +++-- crates/idx-core/src/ownership/entities.rs | 11 ++- crates/idx-core/src/ownership/mod.rs | 24 ++++++ crates/idx-core/src/ownership/snapshot.rs | 47 +++++++++- crates/idx-core/src/ownership/xlsx.rs | 8 +- crates/idx-mcp/Cargo.toml | 2 +- crates/idx-mcp/src/params.rs | 8 +- crates/idx-mcp/src/server.rs | 100 ++++++++++++++++++---- crates/idx-mcp/tests/golden/tools.json | 10 +-- crates/idx-mcp/tests/mcp.rs | 16 ++++ docs/SMOKE.md | 2 +- scripts/publish-ownership-snapshot.sh | 4 +- tests/agents/agent-smoke.sh | 7 +- 16 files changed, 310 insertions(+), 59 deletions(-) diff --git a/crates/idx-core/src/api/mod.rs b/crates/idx-core/src/api/mod.rs index 258b122..1a7bf04 100644 --- a/crates/idx-core/src/api/mod.rs +++ b/crates/idx-core/src/api/mod.rs @@ -163,6 +163,19 @@ pub fn resolve_symbol(symbol: &str, exchange: &str) -> Result "ticker symbol cannot be empty".into(), )); } + // Tickers name cache files and provider URLs: letters, digits, and the + // separators real symbols use (BBCA.JK, BRK-B, ^JKSE), nothing else. + if trimmed.len() > 32 + || trimmed.contains("..") + || !trimmed + .chars() + .all(|ch| ch.is_ascii_alphanumeric() || matches!(ch, '.' | '-' | '^' | '=')) + { + return Err(IdxError::InvalidInput(format!( + "invalid ticker symbol '{}'", + symbol.trim() + ))); + } if let Some((base, suffix)) = trimmed.rsplit_once('.') && !base.is_empty() && !suffix.is_empty() @@ -301,6 +314,26 @@ mod tests { assert_eq!(resolve_symbol("BBCA", "JK").unwrap(), "BBCA.JK"); } + #[test] + fn rejects_symbols_that_are_not_tickers() { + for bad in [ + "../../REPORT", + "a/b", + "a\\b", + "BB CA", + "BBCA..JK", + "BBCA\u{0}", + &"A".repeat(40), + ] { + assert!( + matches!(resolve_symbol(bad, "JK"), Err(IdxError::InvalidInput(_))), + "{bad:?} accepted" + ); + } + assert_eq!(resolve_symbol("^jkse", "JK").unwrap(), "^JKSE.JK"); + assert_eq!(resolve_symbol("BRK-B.US", "JK").unwrap(), "BRK-B.US"); + } + #[test] fn selected_provider_exposes_expected_capabilities_for_msn() { let provider = build_selected_provider(ProviderKind::Msn, false, false); diff --git a/crates/idx-core/src/api/yahoo/client.rs b/crates/idx-core/src/api/yahoo/client.rs index 0f96262..6dc3154 100644 --- a/crates/idx-core/src/api/yahoo/client.rs +++ b/crates/idx-core/src/api/yahoo/client.rs @@ -2,6 +2,7 @@ use std::path::{Path, PathBuf}; use std::process::Command; use std::process::Stdio; use std::sync::Mutex; +use std::sync::atomic::{AtomicU64, Ordering}; use std::time::Duration; use crate::api::types::{Interval, Period}; @@ -19,6 +20,9 @@ const CRUMB_FETCH_URL: &str = "https://query1.finance.yahoo.com/v1/test/getcrumb pub(super) struct YahooClient { agent: ureq::Agent, crumb: Mutex>, + /// One jar per client, paired with its crumb: clients in the same process + /// (concurrent MCP calls) must not overwrite each other's cookies. + cookie_jar: PathBuf, } impl YahooClient { @@ -29,9 +33,16 @@ impl YahooClient { .build() .into(); + static NEXT_JAR: AtomicU64 = AtomicU64::new(0); + let cookie_jar = std::env::temp_dir().join(format!( + "idx_yf_{}_{}.txt", + std::process::id(), + NEXT_JAR.fetch_add(1, Ordering::Relaxed) + )); Self { agent, crumb: Mutex::new(None), + cookie_jar, } } @@ -49,10 +60,6 @@ impl YahooClient { ) } - fn cookie_jar_path() -> PathBuf { - std::env::temp_dir().join(format!("idx_yf_{}.txt", std::process::id())) - } - pub(super) fn parse_crumb_body(raw: &str) -> Result { let crumb = raw.trim(); if crumb.is_empty() { @@ -123,7 +130,7 @@ impl YahooClient { fn fetch_crumb_via_curl(&self) -> Result { let binary = curl_impersonate::chrome_curl_binary()?; - let cookie_jar = Self::cookie_jar_path(); + let cookie_jar = &self.cookie_jar; let cookie_jar_str = cookie_jar.to_str().ok_or_else(|| { IdxError::Io(format!( "failed to encode Yahoo cookie jar path {}", @@ -235,7 +242,7 @@ impl YahooClient { ) -> Result { for auth_attempt in 0..2 { let crumb = self.get_or_init_crumb()?; - let cookie_header = match Self::cookie_header_from_jar(&Self::cookie_jar_path()) { + let cookie_header = match Self::cookie_header_from_jar(&self.cookie_jar) { Ok(header) => header, Err(err) => { runtime::warn(format!("failed to parse Yahoo cookie jar: {err}")); @@ -284,6 +291,12 @@ fn jitter() -> Duration { Duration::from_millis(fastrand::u64(0..100)) } +impl Drop for YahooClient { + fn drop(&mut self) { + let _ = std::fs::remove_file(&self.cookie_jar); + } +} + #[cfg(test)] mod tests { use super::YahooClient; diff --git a/crates/idx-core/src/cache.rs b/crates/idx-core/src/cache.rs index 3740eed..ad74752 100644 --- a/crates/idx-core/src/cache.rs +++ b/crates/idx-core/src/cache.rs @@ -136,7 +136,14 @@ impl Cache { }; let raw = serde_json::to_string_pretty(&entry) .map_err(|e| IdxError::ParseError(e.to_string()))?; - fs::write(path, raw).map_err(|e| IdxError::Io(e.to_string())) + // Write then rename, so a concurrent reader never sees a partial + // entry (and deletes it as corrupt). + let temp = path.with_extension(format!("json.{}.tmp", fastrand::u64(..))); + fs::write(&temp, raw).map_err(|e| IdxError::Io(e.to_string()))?; + fs::rename(&temp, &path).map_err(|e| { + let _ = fs::remove_file(&temp); + IdxError::Io(e.to_string()) + }) } pub fn info(&self) -> Result { @@ -244,10 +251,30 @@ impl Cache { } fn entry_path(&self, data_type: &str, symbol: &str) -> PathBuf { - self.root.join(data_type).join(format!("{symbol}.json")) + self.root + .join(safe_file_name(data_type)) + .join(format!("{}.json", safe_file_name(symbol))) } } +/// Keys become single path components: anything that could separate or +/// climb directories is percent-encoded. Keys made of letters, digits, and +/// `._-:^=` (every key idx produces) are unchanged. +fn safe_file_name(key: &str) -> String { + let mut out = String::with_capacity(key.len()); + for (i, byte) in key.bytes().enumerate() { + let safe = byte.is_ascii_alphanumeric() + || matches!(byte, b'_' | b'-' | b':' | b'^' | b'=') + || (byte == b'.' && i > 0); + if safe { + out.push(byte as char); + } else { + out.push_str(&format!("%{byte:02X}")); + } + } + out +} + fn is_expired(fetched_at: DateTime, ttl_secs: u64) -> bool { let ttl = chrono::Duration::from_std(Duration::from_secs(ttl_secs)).unwrap_or(chrono::Duration::MAX); @@ -275,7 +302,7 @@ mod tests { use serde::{Deserialize, Serialize}; - use super::{Cache, CacheEntry}; + use super::{Cache, CacheEntry, safe_file_name}; static TMP_COUNTER: AtomicUsize = AtomicUsize::new(0); @@ -377,4 +404,22 @@ mod tests { // The corrupted file should be deleted assert!(!path.exists(), "corrupted file should be deleted"); } + + #[test] + fn cache_keys_cannot_leave_the_cache_root() { + assert_eq!(safe_file_name("BBCA.JK-3mo-1d"), "BBCA.JK-3mo-1d"); + assert_eq!( + safe_file_name("top-performers:id:50"), + "top-performers:id:50" + ); + assert_eq!(safe_file_name("../../REPORT"), "%2E.%2F..%2FREPORT"); + assert_eq!(safe_file_name(".."), "%2E."); + + let cache = Cache::at(std::path::PathBuf::from("/cache/idx")); + let path = cache.entry_path("msn-quote", "../../REPORT"); + assert_eq!( + path.parent().unwrap(), + std::path::Path::new("/cache/idx/msn-quote") + ); + } } diff --git a/crates/idx-core/src/ownership/archive.rs b/crates/idx-core/src/ownership/archive.rs index 1c7f3a9..f883c47 100644 --- a/crates/idx-core/src/ownership/archive.rs +++ b/crates/idx-core/src/ownership/archive.rs @@ -1,5 +1,5 @@ use std::fs; -use std::io::{Cursor, Read}; +use std::io::Cursor; use std::path::Path; use chrono::NaiveDate; @@ -177,11 +177,7 @@ fn extract_txt_from_zip(path: &Path) -> Result { continue; } - let mut output = String::new(); - file.read_to_string(&mut output).map_err(|e| { - IdxError::ParseError(format!("failed to decode KSEI archive TXT entry: {e}")) - })?; - return Ok(output); + return super::read_zip_entry_bounded(&mut file, "KSEI archive TXT entry"); } Err(IdxError::ParseError( @@ -201,7 +197,8 @@ fn validate_header(header: &str) -> Result<(), IdxError> { fn parse_archive_date(raw: &str) -> Result { let trimmed = raw.trim(); - if trimmed.len() != 11 { + // Fixed byte offsets below; non-ASCII input would split a character. + if trimmed.len() != 11 || !trimmed.is_ascii() { return Err(IdxError::ParseError(format!( "invalid KSEI archive date `{trimmed}`" ))); @@ -357,4 +354,14 @@ mod tests { assert!(pdf_local_cp.total_shares <= archive_local_cp.total_shares); assert!(pdf_local_cp.percentage_bps <= archive_local_cp.percentage_bps); } + + #[test] + fn non_ascii_archive_dates_are_parse_errors_not_panics() { + // 11 bytes, but byte offsets would split the 'é'. + assert!(super::parse_archive_date("1éJan-2026").is_err()); + assert_eq!( + super::parse_archive_date("27-FEB-2026").unwrap(), + chrono::NaiveDate::from_ymd_opt(2026, 2, 27).unwrap() + ); + } } diff --git a/crates/idx-core/src/ownership/entities.rs b/crates/idx-core/src/ownership/entities.rs index 21c94a0..4293eab 100644 --- a/crates/idx-core/src/ownership/entities.rs +++ b/crates/idx-core/src/ownership/entities.rs @@ -81,7 +81,10 @@ pub fn parse_id_percentage(s: &str) -> Result { IdxError::ParseError(format!("failed to parse Indonesian percentage '{s}': {e}")) })?; - Ok((whole_i * 100) + frac_i) + whole_i + .checked_mul(100) + .and_then(|bps| bps.checked_add(frac_i)) + .ok_or_else(|| IdxError::ParseError(format!("Indonesian percentage '{s}' is out of range"))) } /// Parse KSEI date format to NaiveDate. @@ -453,4 +456,10 @@ mod tests { assert!(normalize_ksei_row(&raw).is_err()); } + + #[test] + fn huge_percentages_are_parse_errors_not_overflow() { + assert!(super::parse_id_percentage("9223372036854775807,99").is_err()); + assert_eq!(super::parse_id_percentage("54,94").unwrap(), 5494); + } } diff --git a/crates/idx-core/src/ownership/mod.rs b/crates/idx-core/src/ownership/mod.rs index 6635dae..05700eb 100644 --- a/crates/idx-core/src/ownership/mod.rs +++ b/crates/idx-core/src/ownership/mod.rs @@ -8,3 +8,27 @@ pub mod search; pub mod snapshot; pub mod types; pub mod xlsx; + +use std::io::Read; + +use crate::error::IdxError; + +/// Largest decompressed ZIP entry read into memory. Real IDX worksheets and +/// KSEI TXT archives are tens of MB; this bounds a hostile "zip bomb". +const MAX_ZIP_ENTRY_BYTES: u64 = 256 * 1024 * 1024; + +/// Reads one decompressed ZIP entry as UTF-8, failing past the size limit. +pub(crate) fn read_zip_entry_bounded(entry: impl Read, what: &str) -> Result { + let mut bytes = Vec::new(); + entry + .take(MAX_ZIP_ENTRY_BYTES + 1) + .read_to_end(&mut bytes) + .map_err(|e| IdxError::ParseError(format!("failed to read {what}: {e}")))?; + if bytes.len() as u64 > MAX_ZIP_ENTRY_BYTES { + return Err(IdxError::ParseError(format!( + "{what} expands beyond {MAX_ZIP_ENTRY_BYTES} bytes" + ))); + } + String::from_utf8(bytes) + .map_err(|e| IdxError::ParseError(format!("failed to decode {what}: {e}"))) +} diff --git a/crates/idx-core/src/ownership/snapshot.rs b/crates/idx-core/src/ownership/snapshot.rs index 3b26a18..6fe1a00 100644 --- a/crates/idx-core/src/ownership/snapshot.rs +++ b/crates/idx-core/src/ownership/snapshot.rs @@ -202,6 +202,14 @@ pub fn sync_snapshot( let install_result = (|| -> Result<(), IdxError> { validate_snapshot_db(&temp_path, &manifest.snapshot)?; + // The decision was made before the download; an import or edit that + // landed since must not be overwritten without --force. + if !force && !same_local_state(local_state.as_ref(), inspect_local_db(db_path)?.as_ref()) { + return Err(IdxError::DatabaseError( + "the local ownership database changed during sync; rerun `idx ownership sync`" + .to_string(), + )); + } install_snapshot_file(&temp_path, db_path) })(); @@ -362,6 +370,21 @@ fn inspect_local_db(db_path: &Path) -> Result, IdxErr })) } +fn same_local_state( + before: Option<&LocalSnapshotState>, + after: Option<&LocalSnapshotState>, +) -> bool { + match (before, after) { + (None, None) => true, + (Some(before), Some(after)) => { + before.file_sha256.is_some() + && before.file_sha256 == after.file_sha256 + && before.release_count == after.release_count + } + _ => false, + } +} + /// Moves committed WAL frames into the database file, so its hash covers local /// edits too. Returns false when changes remain in the WAL. fn checkpoint_wal(db_path: &Path) -> bool { @@ -446,10 +469,12 @@ fn build_sync_decision( }; } - if snapshot - .replaces_sqlite_sha256 - .iter() - .any(|sha256| file_sha256.is_some_and(|local| sha256.eq_ignore_ascii_case(local))) + // A rebuild never silently drops history the local database has. + if snapshot.release_count >= local.release_count + && snapshot + .replaces_sqlite_sha256 + .iter() + .any(|sha256| file_sha256.is_some_and(|local| sha256.eq_ignore_ascii_case(local))) { return SyncDecision { action: OwnershipSyncAction::Updated, @@ -909,6 +934,20 @@ mod tests { assert!(!decision.should_download); } + #[test] + fn build_sync_decision_never_drops_history_through_a_rebuild() { + // A forced same-date rebuild with fewer releases lists the old build; + // a client holding it keeps its history instead of downgrading. + let earlier = "e".repeat(64); + let mut snapshot = same_date_snapshot(vec![earlier.clone()]); + snapshot.release_count = 1; + let mut local = same_release_local(&snapshot, &earlier); + local.release_count = 3; + + let decision = build_sync_decision(Some(&local), &snapshot, false); + assert!(!decision.should_download, "{}", decision.reason); + } + #[test] fn build_sync_decision_keeps_a_locally_modified_database() { // Same release metadata but a file matching no published build (local diff --git a/crates/idx-core/src/ownership/xlsx.rs b/crates/idx-core/src/ownership/xlsx.rs index 90f7348..c150cfd 100644 --- a/crates/idx-core/src/ownership/xlsx.rs +++ b/crates/idx-core/src/ownership/xlsx.rs @@ -9,7 +9,7 @@ use std::collections::HashMap; use std::fs; -use std::io::{Cursor, Read}; +use std::io::Cursor; use std::path::Path; use chrono::{Duration, NaiveDate}; @@ -83,11 +83,7 @@ fn read_zip_entry( ))); } }; - let mut output = String::new(); - file.read_to_string(&mut output).map_err(|e| { - IdxError::ParseError(format!("failed to decode ownership XLSX entry {name}: {e}")) - })?; - Ok(Some(output)) + super::read_zip_entry_bounded(&mut file, &format!("ownership XLSX entry {name}")).map(Some) } fn first_worksheet_name(zip: &mut zip::ZipArchive>) -> Result { diff --git a/crates/idx-mcp/Cargo.toml b/crates/idx-mcp/Cargo.toml index c055a91..60285c3 100644 --- a/crates/idx-mcp/Cargo.toml +++ b/crates/idx-mcp/Cargo.toml @@ -24,7 +24,7 @@ rmcp = { version = "3.5", default-features = false, features = ["server", "macro schemars = "1" serde.workspace = true serde_json.workspace = true -tokio = { version = "1", features = ["macros", "rt-multi-thread", "io-std"] } +tokio = { version = "1", features = ["macros", "rt-multi-thread", "io-std", "sync"] } [dev-dependencies] # Tests drive the binary with the mock provider; release builds exclude it. diff --git a/crates/idx-mcp/src/params.rs b/crates/idx-mcp/src/params.rs index 74e3fee..9c4a1b8 100644 --- a/crates/idx-mcp/src/params.rs +++ b/crates/idx-mcp/src/params.rs @@ -8,7 +8,7 @@ use serde::Deserialize; #[derive(Debug, Deserialize, JsonSchema)] pub struct SymbolsParams { - /// IDX tickers, e.g. ["BBCA", "BBRI"]; the `.JK` suffix is optional. + /// IDX tickers, e.g. ["BBCA", "BBRI"]; the `.JK` suffix is optional. At most 20. pub symbols: Vec, } @@ -36,7 +36,7 @@ pub struct ScreenParams { #[serde(default)] #[schemars(schema_with = "region_schema")] pub region: Option, - /// Maximum results (default 50). + /// Maximum results (default 50, at most 200). #[serde(default)] pub limit: Option, } @@ -130,7 +130,7 @@ pub struct EntityParams { pub struct SearchParams { /// Full-text query over investor names. pub query: String, - /// Maximum results (default 20). + /// Maximum results (default 20, at most 100). #[serde(default)] pub limit: Option, } @@ -144,7 +144,7 @@ pub struct ChangesParams { /// Only this ticker's changes. #[serde(default)] pub ticker: Option, - /// Maximum rows returned (default 200); `total` reports the full count. + /// Maximum rows returned (default 200, at most 2000); `total` reports the full count. #[serde(default)] pub limit: Option, } diff --git a/crates/idx-mcp/src/server.rs b/crates/idx-mcp/src/server.rs index ac817b2..a6fe864 100644 --- a/crates/idx-mcp/src/server.rs +++ b/crates/idx-mcp/src/server.rs @@ -15,6 +15,7 @@ use rmcp::model::{CallToolResult, Implementation, ServerCapabilities, ServerConf use rmcp::{ErrorData, ServerHandler, tool, tool_handler, tool_router}; use serde::Serialize; use serde_json::{Map, Value, json}; +use tokio::sync::{RwLock, Semaphore}; use crate::params::{ AnalysisKind, AnalyzeParams, ChangesParams, CompanyParams, CompanySection, EntityParams, @@ -26,6 +27,18 @@ const NEWS_LIMIT: usize = 10; const SCREEN_LIMIT: usize = 50; const SEARCH_LIMIT: usize = 20; const CHANGES_LIMIT: usize = 200; +// Hard caps on what one call may request, so a misbehaving or prompt-injected +// agent cannot turn a call into unbounded upstream traffic or output. +const MAX_SYMBOLS: usize = 20; +const MAX_SCREEN_LIMIT: usize = 200; +const MAX_SEARCH_LIMIT: usize = 100; +const MAX_CHANGES_LIMIT: usize = 2000; + +/// Tool calls running at once; the rest wait their turn. +static CALL_SLOTS: Semaphore = Semaphore::const_new(4); +/// `ownership_sync` replaces the database file: it runs alone, while +/// ownership reads may run together. +static OWNERSHIP: RwLock<()> = RwLock::const_new(()); const INSTRUCTIONS: &str = "Indonesian stock market (IDX) data. Market and financial \ tools fetch live data (cached on disk) and wrap it as {data, source, fetched_at, stale}; \ @@ -79,6 +92,7 @@ impl IdxServer { } else { "run `idx ownership sync`, or restart idx-mcp with --allow-sync" }; + let _read = OWNERSHIP.read().await; blocking(move || { if !investor.status()?.populated { return Err(IdxError::NoMarketData(format!( @@ -98,10 +112,10 @@ impl IdxServer { for one or more IDX tickers." )] async fn get_quotes(&self, params: Parameters) -> ToolResult { - let SymbolsParams { symbols } = params.0; - if let Some(err) = empty_input("symbols", &symbols) { - return err; - } + let symbols = match checked_symbols(params.0.symbols) { + Ok(symbols) => symbols, + Err(err) => return err, + }; self.live(move |idx| { let quotes = idx.market().quotes(&symbols, CachePolicy::Default)?; Ok(json!({ "quotes": quotes })) @@ -143,7 +157,7 @@ for one or more IDX tickers." .unwrap_or("top-performers") .parse::()?, region: region.as_deref().unwrap_or("id").parse::()?, - limit: limit.unwrap_or(SCREEN_LIMIT), + limit: bounded_limit("limit", limit, SCREEN_LIMIT, MAX_SCREEN_LIMIT)?, }; idx.market().screen(&query, CachePolicy::Default) }) @@ -232,10 +246,10 @@ are listed under `errors`; the call fails only if every analysis fails." tickers. Tickers that fail are listed under `failures`." )] async fn compare_stocks(&self, params: Parameters) -> ToolResult { - let SymbolsParams { symbols } = params.0; - if let Some(err) = empty_input("symbols", &symbols) { - return err; - } + let symbols = match checked_symbols(params.0.symbols) { + Ok(symbols) => symbols, + Err(err) => return err, + }; self.live(move |idx| { let comparison = idx.analysis().compare(&symbols, CachePolicy::Default)?; let failures: Vec = comparison @@ -273,7 +287,8 @@ with concentration metrics (top1, top3, HHI, free float)." async fn search_investors(&self, params: Parameters) -> ToolResult { let SearchParams { query, limit } = params.0; self.local(move |investor| { - let entities = investor.search(&query, limit.unwrap_or(SEARCH_LIMIT))?; + let limit = bounded_limit("limit", limit, SEARCH_LIMIT, MAX_SEARCH_LIMIT)?; + let entities = investor.search(&query, limit)?; Ok(json!({ "entities": entities })) }) .await @@ -293,7 +308,12 @@ release dates, optionally for one ticker." self.local(move |investor| { let mut changes = investor.changes(&from, &to, ticker.as_deref())?; let total = changes.len(); - changes.truncate(limit.unwrap_or(CHANGES_LIMIT)); + changes.truncate(bounded_limit( + "limit", + limit, + CHANGES_LIMIT, + MAX_CHANGES_LIMIT, + )?); Ok(json!({ "from": from, "to": to, "total": total, "changes": changes })) }) .await @@ -305,6 +325,7 @@ KSEI release dates." )] async fn ownership_status(&self) -> ToolResult { let investor = Investor::new(&self.config); + let _read = OWNERSHIP.read().await; blocking(move || investor.status()).await } @@ -315,6 +336,7 @@ the local database." async fn ownership_sync(&self, params: Parameters) -> ToolResult { let SyncParams { force } = params.0; let investor = Investor::new(&self.config); + let _write = OWNERSHIP.write().await; blocking(move || investor.sync(None, force)).await } } @@ -380,6 +402,10 @@ where T: Serialize + Send + 'static, F: FnOnce() -> Result + Send + 'static, { + let _slot = CALL_SLOTS + .acquire() + .await + .map_err(|err| ErrorData::internal_error(err.to_string(), None))?; let result = tokio::task::spawn_blocking(move || f().and_then(to_json)) .await .map_err(|err| ErrorData::internal_error(err.to_string(), None))?; @@ -389,15 +415,55 @@ where }) } +fn input_error(message: String) -> ToolResult { + let err = IdxError::InvalidInput(message); + Ok(CallToolResult::structured_error(Value::Object(error_body( + &err, + )))) +} + /// An explicitly empty list is an input error, as in the CLI, rather than an /// empty success an agent could mistake for data. fn empty_input(field: &str, values: &[T]) -> Option { - values.is_empty().then(|| { - let err = IdxError::InvalidInput(format!("{field} must not be empty")); - Ok(CallToolResult::structured_error(Value::Object(error_body( - &err, - )))) - }) + values + .is_empty() + .then(|| input_error(format!("{field} must not be empty"))) +} + +/// Non-empty, deduplicated (case-insensitively, first wins), and capped. +fn checked_symbols(symbols: Vec) -> Result, ToolResult> { + if let Some(err) = empty_input("symbols", &symbols) { + return Err(err); + } + let mut seen = std::collections::HashSet::new(); + let symbols: Vec = symbols + .into_iter() + .filter(|symbol| seen.insert(symbol.trim().to_uppercase())) + .collect(); + if symbols.len() > MAX_SYMBOLS { + return Err(input_error(format!( + "at most {MAX_SYMBOLS} symbols per call (got {})", + symbols.len() + ))); + } + Ok(symbols) +} + +fn bounded_limit( + field: &str, + value: Option, + default: usize, + max: usize, +) -> Result { + match value.unwrap_or(default) { + 0 => Err(IdxError::InvalidInput(format!( + "{field} must be at least 1" + ))), + n if n > max => Err(IdxError::InvalidInput(format!( + "{field} must be at most {max} (got {n})" + ))), + n => Ok(n), + } } fn to_json(value: T) -> Result { diff --git a/crates/idx-mcp/tests/golden/tools.json b/crates/idx-mcp/tests/golden/tools.json index b8c7505..ffd6b54 100644 --- a/crates/idx-mcp/tests/golden/tools.json +++ b/crates/idx-mcp/tests/golden/tools.json @@ -43,7 +43,7 @@ "$schema": "https://json-schema.org/draft/2020-12/schema", "properties": { "symbols": { - "description": "IDX tickers, e.g. [\"BBCA\", \"BBRI\"]; the `.JK` suffix is optional.", + "description": "IDX tickers, e.g. [\"BBCA\", \"BBRI\"]; the `.JK` suffix is optional. At most 20.", "items": { "type": "string" }, @@ -125,7 +125,7 @@ }, "limit": { "default": null, - "description": "Maximum rows returned (default 200); `total` reports the full count.", + "description": "Maximum rows returned (default 200, at most 2000); `total` reports the full count.", "format": "uint", "minimum": 0, "type": [ @@ -202,7 +202,7 @@ "$schema": "https://json-schema.org/draft/2020-12/schema", "properties": { "symbols": { - "description": "IDX tickers, e.g. [\"BBCA\", \"BBRI\"]; the `.JK` suffix is optional.", + "description": "IDX tickers, e.g. [\"BBCA\", \"BBRI\"]; the `.JK` suffix is optional. At most 20.", "items": { "type": "string" }, @@ -278,7 +278,7 @@ }, "limit": { "default": null, - "description": "Maximum results (default 50).", + "description": "Maximum results (default 50, at most 200).", "format": "uint", "minimum": 0, "type": [ @@ -310,7 +310,7 @@ "properties": { "limit": { "default": null, - "description": "Maximum results (default 20).", + "description": "Maximum results (default 20, at most 100).", "format": "uint", "minimum": 0, "type": [ diff --git a/crates/idx-mcp/tests/mcp.rs b/crates/idx-mcp/tests/mcp.rs index 555839e..e7d07b4 100644 --- a/crates/idx-mcp/tests/mcp.rs +++ b/crates/idx-mcp/tests/mcp.rs @@ -279,6 +279,22 @@ fn errors_carry_cli_codes() { assert_eq!(body["code"], "INVALIDINPUT", "{tool}"); } + let many: Vec = (0..21).map(|i| format!("S{i}")).collect(); + for (tool, arguments) in [ + ("get_quotes", json!({"symbols": many})), + ("screen_stocks", json!({"limit": 201})), + ("screen_stocks", json!({"limit": 0})), + ("get_quotes", json!({"symbols": ["../../REPORT"]})), + ] { + let (is_error, body) = client.call(tool, arguments.clone()); + assert!(is_error, "{tool} {arguments}: {body}"); + assert_eq!(body["code"], "INVALIDINPUT", "{tool} {arguments}"); + } + // Duplicates collapse instead of counting against the cap. + let repeated = vec!["BBCA"; 50]; + let quotes = client.ok("get_quotes", json!({"symbols": repeated})); + assert_eq!(quotes["quotes"].as_array().unwrap().len(), 1); + // Empty ownership database: investor tools point at the sync path. let status = client.ok("ownership_status", json!({})); assert_eq!(status["populated"], false); diff --git a/docs/SMOKE.md b/docs/SMOKE.md index 2008abf..e14234a 100644 --- a/docs/SMOKE.md +++ b/docs/SMOKE.md @@ -92,7 +92,7 @@ tests/agents/agent-smoke.sh --live --case shareholders --keep # real da your own agent settings and MCP servers are not loaded or changed. Logins are copied from the agent's home (Codex `auth.json`, pi and Hermes auth files) into a temp dir that is deleted afterwards; `--keep` keeps the run - output but still deletes the copied credentials. + output but still deletes every copied agent config. - Tools are auto-approved for the run (Claude `dontAsk` with `mcp__idx__*`, Codex `default_tools_approval_mode="approve"`); Claude and pi also get no built-in tools, so they can only answer through MCP. diff --git a/scripts/publish-ownership-snapshot.sh b/scripts/publish-ownership-snapshot.sh index dd2c9c3..e520ed7 100755 --- a/scripts/publish-ownership-snapshot.sh +++ b/scripts/publish-ownership-snapshot.sh @@ -464,7 +464,9 @@ MANIFEST_PATH="$OUTPUT_DIR/ownership-snapshot-manifest.json" # A rebuild for the published as-of date (e.g. after a parser fix) lists the # builds it replaces, so clients still holding one update without --force. -if [[ "$BUILT_AS_OF" == "$PUBLISHED_AS_OF" && "$PUBLISHED_SHA256" =~ ^[0-9a-fA-F]{64}$ ]]; then +# Only when it keeps at least the published history: clients never downgrade. +if [[ "$BUILT_AS_OF" == "$PUBLISHED_AS_OF" && "$PUBLISHED_SHA256" =~ ^[0-9a-fA-F]{64}$ ]] \ + && [[ "$PUBLISHED_RELEASE_COUNT" =~ ^[0-9]+$ ]] && (( BUILT_RELEASE_COUNT >= PUBLISHED_RELEASE_COUNT )); then replaces_tmp="$PUBLISH_WORKDIR/manifest-replaces.json" jq --arg published "$PUBLISHED_SHA256" \ --argjson previous "$(jq -c '.snapshot.replaces_sqlite_sha256 // []' <<< "$PUBLISHED_MANIFEST" 2>/dev/null || echo '[]')" ' diff --git a/tests/agents/agent-smoke.sh b/tests/agents/agent-smoke.sh index f2a5957..54372d6 100755 --- a/tests/agents/agent-smoke.sh +++ b/tests/agents/agent-smoke.sh @@ -81,9 +81,10 @@ done work="$(mktemp -d "${TMPDIR:-/tmp}/idx-agent-smoke.XXXXXX")" cleanup() { if [[ "$keep" == 1 ]]; then - # Never leave copied agent credentials behind. - rm -f -- "$work/codex/auth.json" "$work/pi/auth.json" "$work/hermes/auth.json" "$work/hermes/.env" - echo "agent-smoke: kept $work (copied credentials removed)" + # Never leave copied agent logins or settings (which may hold API keys) + # behind; each run's raw output is already under runs/. + rm -rf -- "$work/codex" "$work/pi" "$work/hermes" "$work/opencode" + echo "agent-smoke: kept $work (copied agent configs removed)" elif [[ "$work" == */idx-agent-smoke.* ]]; then rm -rf -- "$work" fi