mirror of
https://github.com/0xrsydn/idx-cli.git
synced 2026-11-07 09:45:36 +00:00
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 <noreply@anthropic.com>
This commit is contained in:
parent
5a3ce462cc
commit
60634d3aef
16 changed files with 310 additions and 59 deletions
|
|
@ -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<String, IdxError> {
|
|||
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<NaiveDate, IdxError> {
|
||||
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()
|
||||
);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -81,7 +81,10 @@ pub fn parse_id_percentage(s: &str) -> Result<i64, IdxError> {
|
|||
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);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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<String, IdxError> {
|
||||
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}")))
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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<Option<LocalSnapshotState>, 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
|
||||
|
|
|
|||
|
|
@ -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<Cursor<&[u8]>>) -> Result<String, IdxError> {
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue