fix: security review fixes — SQL injection, panic, and race condition
database.rs: - search_by_tags: replace string-interpolated LIKE with parameterized ?N bindings - search_by_notes: replace format!() SQL with prepared statement + ?1/?2 params - search_by_set_file: same — use ?1 for pattern and ?2 for limit backtest.rs (extract_and_store / monitor_backtest_completion): - extract_and_store now returns bool — callers only delete report file if true - guard both success paths with conditional delete + warning on failure - replace .parent().unwrap() panic with explicit match + error log + graceful return Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Sonnet 4.6
parent
f8b6a11bb8
commit
804c1f8808
@@ -295,13 +295,14 @@ impl BacktestPipeline {
|
|||||||
}
|
}
|
||||||
|
|
||||||
/// Extract deals from a completed report and store them in the DB.
|
/// Extract deals from a completed report and store them in the DB.
|
||||||
|
/// Returns true if extraction and DB registration succeeded.
|
||||||
async fn extract_and_store(
|
async fn extract_and_store(
|
||||||
report_path: &Path,
|
report_path: &Path,
|
||||||
report_dir: &Path,
|
report_dir: &Path,
|
||||||
report_id: &str,
|
report_id: &str,
|
||||||
config: &Config,
|
config: &Config,
|
||||||
params: &BacktestParams,
|
params: &BacktestParams,
|
||||||
) {
|
) -> bool {
|
||||||
let extractor = ReportExtractor::new();
|
let extractor = ReportExtractor::new();
|
||||||
let start_time = chrono::Utc::now();
|
let start_time = chrono::Utc::now();
|
||||||
match extractor.extract(
|
match extractor.extract(
|
||||||
@@ -313,7 +314,7 @@ impl BacktestPipeline {
|
|||||||
let db = ReportDb::new(&Config::db_path());
|
let db = ReportDb::new(&Config::db_path());
|
||||||
if db.init().is_err() {
|
if db.init().is_err() {
|
||||||
tracing::warn!("launch_backtest: failed to init DB for {}", report_id);
|
tracing::warn!("launch_backtest: failed to init DB for {}", report_id);
|
||||||
return;
|
return false;
|
||||||
}
|
}
|
||||||
let entry = ReportEntry {
|
let entry = ReportEntry {
|
||||||
id: report_id.to_string(),
|
id: report_id.to_string(),
|
||||||
@@ -345,15 +346,17 @@ impl BacktestPipeline {
|
|||||||
};
|
};
|
||||||
if let Err(e) = db.insert(&entry) {
|
if let Err(e) = db.insert(&entry) {
|
||||||
tracing::warn!("launch_backtest: failed to register report in DB: {}", e);
|
tracing::warn!("launch_backtest: failed to register report in DB: {}", e);
|
||||||
return;
|
return false;
|
||||||
}
|
}
|
||||||
if let Err(e) = db.insert_deals(report_id, &extraction.deals) {
|
if let Err(e) = db.insert_deals(report_id, &extraction.deals) {
|
||||||
tracing::warn!("launch_backtest: failed to store deals in DB: {}", e);
|
tracing::warn!("launch_backtest: failed to store deals in DB: {}", e);
|
||||||
}
|
}
|
||||||
tracing::info!("launch_backtest: extracted {} deals for {}", extraction.deals.len(), report_id);
|
tracing::info!("launch_backtest: extracted {} deals for {}", extraction.deals.len(), report_id);
|
||||||
|
true
|
||||||
}
|
}
|
||||||
Err(e) => {
|
Err(e) => {
|
||||||
tracing::warn!("launch_backtest: extraction failed for {}: {}", report_id, e);
|
tracing::warn!("launch_backtest: extraction failed for {}: {}", report_id, e);
|
||||||
|
false
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -381,8 +384,12 @@ impl BacktestPipeline {
|
|||||||
let candidate = expected_report.with_extension(ext.trim_start_matches('.'));
|
let candidate = expected_report.with_extension(ext.trim_start_matches('.'));
|
||||||
if candidate.exists() {
|
if candidate.exists() {
|
||||||
tracing::info!("Backtest {} completed: report found at {}", report_id, candidate.display());
|
tracing::info!("Backtest {} completed: report found at {}", report_id, candidate.display());
|
||||||
Self::extract_and_store(&candidate, &report_dir, &report_id, &config, ¶ms).await;
|
let extracted = Self::extract_and_store(&candidate, &report_dir, &report_id, &config, ¶ms).await;
|
||||||
let _ = fs::remove_file(&candidate);
|
if extracted {
|
||||||
|
let _ = fs::remove_file(&candidate);
|
||||||
|
} else {
|
||||||
|
tracing::warn!("Backtest {}: extraction failed, keeping report file at {}", report_id, candidate.display());
|
||||||
|
}
|
||||||
Self::update_job_status(&report_dir, "completed", Some(candidate.to_string_lossy().to_string())).await;
|
Self::update_job_status(&report_dir, "completed", Some(candidate.to_string_lossy().to_string())).await;
|
||||||
if let Some(ref callback) = notification_callback {
|
if let Some(ref callback) = notification_callback {
|
||||||
callback("backtest_completed", json!({
|
callback("backtest_completed", json!({
|
||||||
@@ -401,10 +408,22 @@ impl BacktestPipeline {
|
|||||||
|
|
||||||
if !in_grace && !mt5_alive {
|
if !in_grace && !mt5_alive {
|
||||||
// MT5 exited without report - check for any newer report
|
// MT5 exited without report - check for any newer report
|
||||||
if let Some(path) = Self::find_newest_report(expected_report.parent().unwrap(), poll_start) {
|
let reports_parent = match expected_report.parent() {
|
||||||
|
Some(p) => p,
|
||||||
|
None => {
|
||||||
|
tracing::error!("Backtest {}: expected_report path has no parent", report_id);
|
||||||
|
Self::update_job_status(&report_dir, "failed", None).await;
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
};
|
||||||
|
if let Some(path) = Self::find_newest_report(reports_parent, poll_start) {
|
||||||
tracing::info!("Backtest {} completed: found fallback report {}", report_id, path.display());
|
tracing::info!("Backtest {} completed: found fallback report {}", report_id, path.display());
|
||||||
Self::extract_and_store(&path, &report_dir, &report_id, &config, ¶ms).await;
|
let extracted = Self::extract_and_store(&path, &report_dir, &report_id, &config, ¶ms).await;
|
||||||
let _ = fs::remove_file(&path);
|
if extracted {
|
||||||
|
let _ = fs::remove_file(&path);
|
||||||
|
} else {
|
||||||
|
tracing::warn!("Backtest {}: extraction failed, keeping fallback report at {}", report_id, path.display());
|
||||||
|
}
|
||||||
Self::update_job_status(&report_dir, "completed", Some(path.to_string_lossy().to_string())).await;
|
Self::update_job_status(&report_dir, "completed", Some(path.to_string_lossy().to_string())).await;
|
||||||
if let Some(ref callback) = notification_callback {
|
if let Some(ref callback) = notification_callback {
|
||||||
callback("backtest_completed", json!({
|
callback("backtest_completed", json!({
|
||||||
|
|||||||
+33
-22
@@ -564,26 +564,39 @@ impl ReportDb {
|
|||||||
/// Search reports by tags (at least one tag must match)
|
/// Search reports by tags (at least one tag must match)
|
||||||
pub fn search_by_tags(&self, tags: &[String], limit: usize) -> Result<Vec<ReportEntry>> {
|
pub fn search_by_tags(&self, tags: &[String], limit: usize) -> Result<Vec<ReportEntry>> {
|
||||||
let conn = self.connect()?;
|
let conn = self.connect()?;
|
||||||
let mut sql = "SELECT id, expert, symbol, timeframe, model, from_date, to_date, \
|
let base_sql = "SELECT id, expert, symbol, timeframe, model, from_date, to_date, \
|
||||||
created_at, set_file_original, set_snapshot_path, report_dir, charts_dir, \
|
created_at, set_file_original, set_snapshot_path, report_dir, charts_dir, \
|
||||||
net_profit, profit_factor, max_dd_pct, sharpe_ratio, total_trades, \
|
net_profit, profit_factor, max_dd_pct, sharpe_ratio, total_trades, \
|
||||||
win_rate_pct, recovery_factor, deposit, currency, leverage, \
|
win_rate_pct, recovery_factor, deposit, currency, leverage, \
|
||||||
duration_seconds, tags, notes, verdict \
|
duration_seconds, tags, notes, verdict \
|
||||||
FROM reports WHERE 1=1"
|
FROM reports WHERE 1=1";
|
||||||
.to_string();
|
|
||||||
|
|
||||||
// Build OR conditions for tags - use JSON1 extension for tag matching
|
let (sql, params): (String, Vec<rusqlite::types::Value>) = if tags.is_empty() {
|
||||||
if !tags.is_empty() {
|
(
|
||||||
let tag_conditions: Vec<String> = tags.iter()
|
format!("{} ORDER BY created_at DESC LIMIT ?1", base_sql),
|
||||||
.map(|tag| format!("tags LIKE '%{}%'", tag.replace("'", "''")))
|
vec![(limit as i64).into()],
|
||||||
|
)
|
||||||
|
} else {
|
||||||
|
let placeholders = tags.iter().enumerate()
|
||||||
|
.map(|(i, _)| format!("tags LIKE ?{}", i + 1))
|
||||||
|
.collect::<Vec<_>>()
|
||||||
|
.join(" OR ");
|
||||||
|
let sql = format!(
|
||||||
|
"{} AND ({}) ORDER BY created_at DESC LIMIT ?{}",
|
||||||
|
base_sql,
|
||||||
|
placeholders,
|
||||||
|
tags.len() + 1
|
||||||
|
);
|
||||||
|
let mut p: Vec<rusqlite::types::Value> = tags.iter()
|
||||||
|
.map(|t| format!("%{}%", t).into())
|
||||||
.collect();
|
.collect();
|
||||||
sql.push_str(&format!(" AND ({})", tag_conditions.join(" OR ")));
|
p.push((limit as i64).into());
|
||||||
}
|
(sql, p)
|
||||||
sql.push_str(&format!(" ORDER BY created_at DESC LIMIT {}", limit));
|
};
|
||||||
|
|
||||||
let mut stmt = conn.prepare(&sql)?;
|
let mut stmt = conn.prepare(&sql)?;
|
||||||
let entries: Vec<ReportEntry> = stmt
|
let entries: Vec<ReportEntry> = stmt
|
||||||
.query_map([], |row| {
|
.query_map(rusqlite::params_from_iter(params.iter()), |row| {
|
||||||
let tags_str: String = row.get(23).unwrap_or_else(|_| "[]".to_string());
|
let tags_str: String = row.get(23).unwrap_or_else(|_| "[]".to_string());
|
||||||
let tags: Vec<String> = serde_json::from_str(&tags_str).unwrap_or_default();
|
let tags: Vec<String> = serde_json::from_str(&tags_str).unwrap_or_default();
|
||||||
Ok(ReportEntry {
|
Ok(ReportEntry {
|
||||||
@@ -624,19 +637,18 @@ impl ReportDb {
|
|||||||
/// Search reports by notes text (case-insensitive LIKE)
|
/// Search reports by notes text (case-insensitive LIKE)
|
||||||
pub fn search_by_notes(&self, query: &str, limit: usize) -> Result<Vec<ReportEntry>> {
|
pub fn search_by_notes(&self, query: &str, limit: usize) -> Result<Vec<ReportEntry>> {
|
||||||
let conn = self.connect()?;
|
let conn = self.connect()?;
|
||||||
let pattern = format!("%{}%", query.replace("'", "''"));
|
let pattern = format!("%{}%", query);
|
||||||
let mut stmt = conn.prepare(
|
let mut stmt = conn.prepare(
|
||||||
&format!("SELECT id, expert, symbol, timeframe, model, from_date, to_date, \
|
"SELECT id, expert, symbol, timeframe, model, from_date, to_date, \
|
||||||
created_at, set_file_original, set_snapshot_path, report_dir, charts_dir, \
|
created_at, set_file_original, set_snapshot_path, report_dir, charts_dir, \
|
||||||
net_profit, profit_factor, max_dd_pct, sharpe_ratio, total_trades, \
|
net_profit, profit_factor, max_dd_pct, sharpe_ratio, total_trades, \
|
||||||
win_rate_pct, recovery_factor, deposit, currency, leverage, \
|
win_rate_pct, recovery_factor, deposit, currency, leverage, \
|
||||||
duration_seconds, tags, notes, verdict \
|
duration_seconds, tags, notes, verdict \
|
||||||
FROM reports WHERE notes LIKE '{}' ORDER BY created_at DESC LIMIT {}",
|
FROM reports WHERE notes LIKE ?1 ORDER BY created_at DESC LIMIT ?2"
|
||||||
pattern, limit)
|
|
||||||
)?;
|
)?;
|
||||||
|
|
||||||
let entries: Vec<ReportEntry> = stmt
|
let entries: Vec<ReportEntry> = stmt
|
||||||
.query_map([], |row| {
|
.query_map(rusqlite::params![pattern, limit as i64], |row| {
|
||||||
let tags_str: String = row.get(23).unwrap_or_else(|_| "[]".to_string());
|
let tags_str: String = row.get(23).unwrap_or_else(|_| "[]".to_string());
|
||||||
let tags: Vec<String> = serde_json::from_str(&tags_str).unwrap_or_default();
|
let tags: Vec<String> = serde_json::from_str(&tags_str).unwrap_or_default();
|
||||||
Ok(ReportEntry {
|
Ok(ReportEntry {
|
||||||
@@ -677,20 +689,19 @@ impl ReportDb {
|
|||||||
/// Find reports by set file (original or snapshot)
|
/// Find reports by set file (original or snapshot)
|
||||||
pub fn search_by_set_file(&self, set_file: &str, limit: usize) -> Result<Vec<ReportEntry>> {
|
pub fn search_by_set_file(&self, set_file: &str, limit: usize) -> Result<Vec<ReportEntry>> {
|
||||||
let conn = self.connect()?;
|
let conn = self.connect()?;
|
||||||
let pattern = format!("%{}%", set_file.replace("'", "''"));
|
let pattern = format!("%{}%", set_file);
|
||||||
let mut stmt = conn.prepare(
|
let mut stmt = conn.prepare(
|
||||||
&format!("SELECT id, expert, symbol, timeframe, model, from_date, to_date, \
|
"SELECT id, expert, symbol, timeframe, model, from_date, to_date, \
|
||||||
created_at, set_file_original, set_snapshot_path, report_dir, charts_dir, \
|
created_at, set_file_original, set_snapshot_path, report_dir, charts_dir, \
|
||||||
net_profit, profit_factor, max_dd_pct, sharpe_ratio, total_trades, \
|
net_profit, profit_factor, max_dd_pct, sharpe_ratio, total_trades, \
|
||||||
win_rate_pct, recovery_factor, deposit, currency, leverage, \
|
win_rate_pct, recovery_factor, deposit, currency, leverage, \
|
||||||
duration_seconds, tags, notes, verdict \
|
duration_seconds, tags, notes, verdict \
|
||||||
FROM reports WHERE (set_file_original LIKE '{}' OR set_snapshot_path LIKE '{}') \
|
FROM reports WHERE (set_file_original LIKE ?1 OR set_snapshot_path LIKE ?1) \
|
||||||
ORDER BY created_at DESC LIMIT {}",
|
ORDER BY created_at DESC LIMIT ?2"
|
||||||
pattern, pattern, limit)
|
|
||||||
)?;
|
)?;
|
||||||
|
|
||||||
let entries: Vec<ReportEntry> = stmt
|
let entries: Vec<ReportEntry> = stmt
|
||||||
.query_map([], |row| {
|
.query_map(rusqlite::params![pattern, limit as i64], |row| {
|
||||||
let tags_str: String = row.get(23).unwrap_or_else(|_| "[]".to_string());
|
let tags_str: String = row.get(23).unwrap_or_else(|_| "[]".to_string());
|
||||||
let tags: Vec<String> = serde_json::from_str(&tags_str).unwrap_or_default();
|
let tags: Vec<String> = serde_json::from_str(&tags_str).unwrap_or_default();
|
||||||
Ok(ReportEntry {
|
Ok(ReportEntry {
|
||||||
|
|||||||
Reference in New Issue
Block a user