diff --git a/src-tauri/src/cli/agent_review_handlers.rs b/src-tauri/src/cli/agent_review_handlers.rs index 49d29f80..0367a1e2 100644 --- a/src-tauri/src/cli/agent_review_handlers.rs +++ b/src-tauri/src/cli/agent_review_handlers.rs @@ -1,3 +1,5 @@ +use std::path::{Component, Path, PathBuf}; + use tauri_plugin_cli::Matches; use crate::core; @@ -91,15 +93,28 @@ pub(super) fn handle_agent_review_command( )); } let suggestion = get_arg_value(matches, "suggestion").map(|s| strip_suggestion_fence(&s)); + let target_type = + get_arg_value(matches, "target-type").unwrap_or_else(|| DEFAULT_TARGET_TYPE.to_string()); + let target_id = require_value(matches, "target-id")?; + let file = require_value(matches, "file")?; + let side = parse_side(get_arg_value(matches, "side").as_deref())?; + validate_add_target( + &repo_path, + &target_type, + &target_id, + &file, + end_line, + side.as_deref(), + )?; let comment = local_db::create_agent_review_comment( &repo_path, - &get_arg_value(matches, "target-type").unwrap_or_else(|| DEFAULT_TARGET_TYPE.to_string()), - &require_value(matches, "target-id")?, - &require_value(matches, "file")?, + &target_type, + &target_id, + &file, None, start_line, end_line, - parse_side(get_arg_value(matches, "side").as_deref())?.as_deref(), + side.as_deref(), &require_value(matches, "comment")?, suggestion.as_deref(), LOCAL_AGENT_SOURCE, @@ -169,6 +184,77 @@ pub(super) fn handle_agent_review_command( } } +/// Target types the app renders review comments for. +const KNOWN_TARGET_TYPES: &[&str] = &[DEFAULT_TARGET_TYPE, "file_browser_file"]; + +/// Rejects an `add` that nothing could display: an unknown target type, a +/// target that does not exist, a `--file` that is not relative, or (on the +/// new side, which is the file on disk) lines past the end of the file. +fn validate_add_target( + repo_path: &str, + target_type: &str, + target_id: &str, + file: &str, + end_line: i64, + side: Option<&str>, +) -> Result<(), String> { + // The directory `--file` is relative to. + let root = match target_type { + DEFAULT_TARGET_TYPE => { + let id = target_id.trim().parse::().ok().filter(|id| *id > 0); + let id = id.ok_or_else(|| { + format!("invalid_arguments: --target-id must be a workspace id, got '{target_id}'") + })?; + let workspace_dir = core::changes::resolve_workspace_dir(repo_path, Some(id)) + .map_err(|_| format!("workspace_not_found: workspace {id} not found"))?; + PathBuf::from(workspace_dir) + } + "file_browser_file" => PathBuf::from(repo_path), + other => { + return Err(format!( + "invalid_arguments: unknown --target-type '{other}'. Expected one of: {}", + KNOWN_TARGET_TYPES.join(", ") + )) + } + }; + let relative = Path::new(file); + if !relative + .components() + .all(|c| matches!(c, Component::Normal(_) | Component::CurDir)) + { + return Err(format!( + "invalid_arguments: --file must be a relative path inside the repository, got '{file}'" + )); + } + // Canonicalize so a symlink cannot point the line check outside the root. + let resolved = match (root.join(relative).canonicalize(), root.canonicalize()) { + (Ok(path), Ok(root)) if path.starts_with(&root) && path.is_file() => Some(path), + _ => None, + }; + if target_type == "file_browser_file" + && (resolved.is_none() || resolved != Path::new(target_id).canonicalize().ok()) + { + return Err(format!( + "invalid_arguments: --target-id '{target_id}' must be the repository file --file '{file}' names" + )); + } + if side == Some("old") { + return Ok(()); + } + let content = resolved + .and_then(|path| std::fs::read(path).ok()) + .ok_or_else(|| { + format!("invalid_arguments: --file '{file}' does not exist in the repository") + })?; + let line_count = String::from_utf8_lossy(&content).lines().count(); + if end_line as usize > line_count { + return Err(format!( + "invalid_arguments: line {end_line} is past the end of {file} ({line_count} lines)" + )); + } + Ok(()) +} + fn require_comment(repo_path: &str, comment_id: &str) -> Result<(), String> { match local_db::get_agent_review_comment(repo_path, comment_id)? { Some(_) => Ok(()), @@ -240,6 +326,65 @@ mod tests { assert_eq!(strip_suggestion_fence("```suggestion\n```"), ""); } + #[test] + fn add_target_must_be_real_and_inside_the_repo() { + let dir = tempfile::tempdir().unwrap(); + let repo = dir.path().to_str().unwrap(); + let id = local_db::add_workspace( + repo, + "feat".into(), + "feat".into(), + "feat".into(), + None, + None, + None, + ) + .unwrap() + .to_string(); + let workspace = dir.path().join(".treq/workspaces/feat"); + std::fs::create_dir_all(&workspace).unwrap(); + std::fs::write(workspace.join("a.rs"), "1\n2\n3\n").unwrap(); + std::fs::write(dir.path().join("b.rs"), "1\n").unwrap(); + let b = dir.path().join("b.rs"); + let b = b.to_str().unwrap(); + std::fs::write(dir.path().join("big.rs"), "1\n2\n3\n").unwrap(); + let big = dir.path().join("big.rs"); + let big = big.to_str().unwrap(); + let outside = tempfile::tempdir().unwrap(); + std::fs::write(outside.path().join("o.rs"), "1\n2\n3\n4\n").unwrap(); + #[cfg(unix)] + std::os::unix::fs::symlink(outside.path().join("o.rs"), workspace.join("link.rs")).unwrap(); + #[cfg(unix)] + std::os::unix::fs::symlink(outside.path(), workspace.join("dir")).unwrap(); + let check = |target_type: &str, target_id: &str, file: &str, end: i64, side: Option<&str>| { + validate_add_target(repo, target_type, target_id, file, end, side) + }; + + check(DEFAULT_TARGET_TYPE, &id, "a.rs", 3, None).unwrap(); + check(DEFAULT_TARGET_TYPE, &id, "a.rs", 99, Some("old")).unwrap(); + check("file_browser_file", b, "b.rs", 1, None).unwrap(); + for (target_type, target_id, file, end) in [ + ("bogus", id.as_str(), "a.rs", 1), + (DEFAULT_TARGET_TYPE, "999", "a.rs", 1), + (DEFAULT_TARGET_TYPE, "feat", "a.rs", 1), + (DEFAULT_TARGET_TYPE, id.as_str(), "/etc/passwd", 1), + (DEFAULT_TARGET_TYPE, id.as_str(), "../../../b.rs", 1), + (DEFAULT_TARGET_TYPE, id.as_str(), "missing.rs", 1), + (DEFAULT_TARGET_TYPE, id.as_str(), "a.rs", 4), + ("file_browser_file", "/etc/passwd", "b.rs", 1), + ("file_browser_file", b, "b.rs", 2), + ("file_browser_file", big, "b.rs", 3), + ("file_browser_file", b, "missing.rs", 1), + (DEFAULT_TARGET_TYPE, id.as_str(), "link.rs", 1), + (DEFAULT_TARGET_TYPE, id.as_str(), "dir/o.rs", 1), + ] { + assert!( + check(target_type, target_id, file, end, None).is_err(), + "{target_type} {target_id} {file}:{end} was accepted" + ); + } + } + #[test] fn line_numbers_must_be_positive() { assert_eq!(parse_line_number("7", "start-line"), Ok(7));