From 6a2efae8778221e9aca2f62e9dd5421aa5b8a343 Mon Sep 17 00:00:00 2001 From: juangaitanv Date: Mon, 29 Jun 2026 17:40:52 +0200 Subject: [PATCH 01/16] fix(cli): resolve list/wait projects by repo URL, not directory name (COR-1577) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit list/wait now resolve the canonical project via GET /api/v1/projects?repo_url= (with an old-backend safety guard and CWD-name fallback when there's no git remote); dropped the brittle client-side scan.project==name filter; added mutually-exclusive --project-name/--repo override flags; replaced the silent empty table and bare "Error querying scan list" with clear miss messages. CLI-only — no backend/persisted-state changes. Residual of COR-1493. --- src/list.rs | 434 +++++++++++++++++++++------------------ src/main.rs | 45 +++- src/scan.rs | 26 +-- src/utils/api.rs | 230 +++++++++++++++++++++ src/utils/generic.rs | 93 +++++++++ src/wait.rs | 34 ++- tests/common/mod.rs | 59 ++++++ tests/list_resolution.rs | 262 +++++++++++++++++++++++ tests/wait_resolution.rs | 208 +++++++++++++++++++ 9 files changed, 1164 insertions(+), 227 deletions(-) create mode 100644 tests/list_resolution.rs create mode 100644 tests/wait_resolution.rs diff --git a/src/list.rs b/src/list.rs index 00790ba..43f66c3 100644 --- a/src/list.rs +++ b/src/list.rs @@ -4,6 +4,7 @@ use crate::utils; use serde_json::json; use std::path::Path; +#[allow(clippy::too_many_arguments)] pub fn run( config: &Config, issues: &bool, @@ -12,11 +13,19 @@ pub fn run( page: &Option, page_size: &Option, scan_id: &Option, + project_name_override: Option, + repo_override: Option, ) { - let project_name = - utils::generic::get_current_working_directory().unwrap_or("unknown".to_string()); - println!(); + // Leading blank line is cosmetic spacing for the human tables. Gate it on + // non-JSON mode so `--json` (success OR miss) keeps a clean stdout. + if !*json { + println!(); + } if *sca_issues { + // SCA has no project parameter (get_sca_issues); keep the CWD basename + // for its legacy error copy and do NOT resolve here. + let project_name = + utils::generic::get_current_working_directory().unwrap_or("unknown".to_string()); let sca_issues_response = match utils::api::get_sca_issues( &config.get_url(), Some((*page).unwrap_or(1)), @@ -106,229 +115,260 @@ pub fn run( Some(sca_issues_response.page), Some(sca_issues_response.total_pages), ); - } else if *issues { - let issues_response = match utils::api::get_scan_issues( + } else { + // Resolve once for both the --issues and scan-listing paths. + let resolved = utils::api::resolve_project( &config.get_url(), - &project_name, - Some((*page).unwrap_or(1)), - *page_size, - scan_id.clone(), - ) { - Ok(response) => response, - Err(e) => { - debug(&format!("Error Sending Request: {}", e)); - if e.to_string().contains("404") { - if scan_id.is_some() { - log::error!("Scan with ID '{}' doesn't exist. Please run 'corgea scan' to create a new scan for this project.", scan_id.as_ref().unwrap()); + project_name_override.as_deref(), + repo_override.as_deref(), + ); + let project_name = resolved.query_name.clone(); + if *issues { + let issues_response = match utils::api::get_scan_issues( + &config.get_url(), + &project_name, + Some((*page).unwrap_or(1)), + *page_size, + scan_id.clone(), + ) { + Ok(response) => response, + Err(e) => { + debug(&format!("Error Sending Request: {}", e)); + if e.to_string().contains("404") { + if scan_id.is_some() { + log::error!("Scan with ID '{}' doesn't exist. Please run 'corgea scan' to create a new scan for this project.", scan_id.as_ref().unwrap()); + } else if resolved.confirmed { + log::error!("Project '{}' has no issues yet. Run 'corgea scan' to create a scan for this project.", project_name); + } else { + log::error!( + "No Corgea project found for {}. Run 'corgea scan' to create one, or pass --project-name .", + resolved.tried_label + ); + } } else { - log::error!("Project with name '{}' doesn't exist. Please run 'corgea scan' to create a new scan for this project.", project_name); + log::error!( + "Unable to fetch scan issues. Please check your connection and ensure that:\n\ + - The server URL is reachable.\n\ + - Your authentication token is valid.\n\n\ + Check out our docs at https://docs.corgea.app/install_cli#login-with-the-cli {}", + e + ); } - } else { - log::error!( - "Unable to fetch scan issues. Please check your connection and ensure that:\n\ - - The server URL is reachable.\n\ - - Your authentication token is valid.\n\n\ - Check out our docs at https://docs.corgea.app/install_cli#login-with-the-cli {}", - e - ); + std::process::exit(1); } - std::process::exit(1); - } - }; - let mut render_blocking_rules = false; - let mut blocking_rules: std::collections::HashMap = - std::collections::HashMap::new(); + }; + let mut render_blocking_rules = false; + let mut blocking_rules: std::collections::HashMap = + std::collections::HashMap::new(); - if scan_id.is_some() { - let mut page: u32 = 1; - loop { - match utils::api::check_blocking_rules( - &config.get_url(), - scan_id.as_ref().unwrap(), - Some(page), - ) { - Ok(rules) => { - if rules.block { - render_blocking_rules = true; - for issue in rules.blocking_issues { - blocking_rules.insert(issue.id, issue.triggered_by_rules.join(",")); - } - if rules.total_pages == page { + if scan_id.is_some() { + let mut page: u32 = 1; + loop { + match utils::api::check_blocking_rules( + &config.get_url(), + scan_id.as_ref().unwrap(), + Some(page), + ) { + Ok(rules) => { + if rules.block { + render_blocking_rules = true; + for issue in rules.blocking_issues { + blocking_rules + .insert(issue.id, issue.triggered_by_rules.join(",")); + } + if rules.total_pages == page { + break; + } + page += 1; + } else { break; } - page += 1; - } else { - break; } - } - Err(e) => { - log::error!("Failed to check blocking rules: {}", e); - std::process::exit(1); + Err(e) => { + log::error!("Failed to check blocking rules: {}", e); + std::process::exit(1); + } } } } - } - if *json { - let mut json = serde_json::json!({ - "page": issues_response.page, - "total_pages": issues_response.total_pages, - "results": &issues_response.issues - }); - if render_blocking_rules { - json["results"] = serde_json::json!(issues_response - .issues - .unwrap_or_default() - .iter() - .map(|issue| { - serde_json::json!(utils::api::IssueWithBlockingRules { - id: issue.id.clone(), - scan_id: issue.scan_id.clone(), - status: issue.status.clone(), - urgency: issue.urgency.clone(), - created_at: issue.created_at.clone(), - classification: issue.classification.clone(), - location: issue.location.clone(), - details: issue.details.clone(), - auto_triage: issue.auto_triage.clone(), - auto_fix_suggestion: issue.auto_fix_suggestion.clone(), - blocked: blocking_rules.contains_key(&issue.id), - blocking_rules: if blocking_rules.contains_key(&issue.id) { - Some(vec![blocking_rules.get(&issue.id).unwrap().clone()]) - } else { - None - } + if *json { + let mut json = serde_json::json!({ + "page": issues_response.page, + "total_pages": issues_response.total_pages, + "results": &issues_response.issues + }); + if render_blocking_rules { + json["results"] = serde_json::json!(issues_response + .issues + .unwrap_or_default() + .iter() + .map(|issue| { + serde_json::json!(utils::api::IssueWithBlockingRules { + id: issue.id.clone(), + scan_id: issue.scan_id.clone(), + status: issue.status.clone(), + urgency: issue.urgency.clone(), + created_at: issue.created_at.clone(), + classification: issue.classification.clone(), + location: issue.location.clone(), + details: issue.details.clone(), + auto_triage: issue.auto_triage.clone(), + auto_fix_suggestion: issue.auto_fix_suggestion.clone(), + blocked: blocking_rules.contains_key(&issue.id), + blocking_rules: if blocking_rules.contains_key(&issue.id) { + Some(vec![blocking_rules.get(&issue.id).unwrap().clone()]) + } else { + None + } + }) }) - }) - .collect::>()); + .collect::>()); + } + let output = json!(json); + println!("{}", serde_json::to_string_pretty(&output).unwrap()); + return; } - let output = json!(json); - println!("{}", serde_json::to_string_pretty(&output).unwrap()); - return; - } - let mut table_header = vec![ - "Issue ID".to_string(), - "Category".to_string(), - "Urgency".to_string(), - "File Path".to_string(), - "Line".to_string(), - ]; - if render_blocking_rules { - table_header.push("Blocking".to_string()); - table_header.push("Rule ID".to_string()); - } - let mut table = vec![table_header]; + let mut table_header = vec![ + "Issue ID".to_string(), + "Category".to_string(), + "Urgency".to_string(), + "File Path".to_string(), + "Line".to_string(), + ]; + if render_blocking_rules { + table_header.push("Blocking".to_string()); + table_header.push("Rule ID".to_string()); + } + let mut table = vec![table_header]; - for issue in &issues_response.issues.unwrap_or_default() { - let classification_display = issue.classification.id.clone(); - let path = Path::new(&issue.location.file.path); - let path_parts: Vec<&str> = path - .components() - .filter_map(|c| c.as_os_str().to_str()) - .collect(); + for issue in &issues_response.issues.unwrap_or_default() { + let classification_display = issue.classification.id.clone(); + let path = Path::new(&issue.location.file.path); + let path_parts: Vec<&str> = path + .components() + .filter_map(|c| c.as_os_str().to_str()) + .collect(); - let shortened_path = if path_parts.len() > 2 { - let base_part = if path_parts[0].len() > 1 { - path_parts[0] + let shortened_path = if path_parts.len() > 2 { + let base_part = if path_parts[0].len() > 1 { + path_parts[0] + } else { + path_parts[1] + }; + format!("{}/../{}", base_part, path_parts[path_parts.len() - 1]).to_string() } else { - path_parts[1] + issue.location.file.path.clone() }; - format!("{}/../{}", base_part, path_parts[path_parts.len() - 1]).to_string() - } else { - issue.location.file.path.clone() - }; - let mut row = vec![ - issue.id.clone(), - classification_display, - issue.urgency.clone(), - shortened_path, - issue.location.line_number.to_string(), - ]; - if render_blocking_rules { - row.push(blocking_rules.contains_key(&issue.id).to_string()); - row.push( - blocking_rules - .get(&issue.id) - .unwrap_or(&"".to_string()) - .to_string(), - ); + let mut row = vec![ + issue.id.clone(), + classification_display, + issue.urgency.clone(), + shortened_path, + issue.location.line_number.to_string(), + ]; + if render_blocking_rules { + row.push(blocking_rules.contains_key(&issue.id).to_string()); + row.push( + blocking_rules + .get(&issue.id) + .unwrap_or(&"".to_string()) + .to_string(), + ); + } + table.push(row); } - table.push(row); - } - utils::terminal::print_table(table, issues_response.page, issues_response.total_pages); - } else { - let (scans, page, total_pages) = match utils::api::query_scan_list( - &config.get_url(), - Some(&project_name), - *page, - *page_size, - ) { - Ok(scans) => { - let page = scans.page; - let total_pages = scans.total_pages; - let filtered_scans: Vec = scans - .scans - .unwrap_or_default() - .into_iter() - .filter(|scan| scan.project == project_name) - .collect(); - (filtered_scans, page, total_pages) + utils::terminal::print_table(table, issues_response.page, issues_response.total_pages); + } else { + let (scans, page, total_pages) = match utils::api::query_scan_list( + &config.get_url(), + Some(&project_name), + *page, + *page_size, + ) { + Ok(scans) => { + let page = scans.page; + let total_pages = scans.total_pages; + // The server already filtered by the resolved project; the old + // client-side `scan.project == cwd_basename` pass is redundant + // and would discard every repo-resolved scan. (COR-1577) + (scans.scans.unwrap_or_default(), page, total_pages) + } + Err(e) => { + if e.to_string().contains("404") { + log::error!( + "No Corgea project found for {}. Run 'corgea scan' to create one, or pass --project-name .", + resolved.tried_label + ); + } else { + log::error!( + "Unable to fetch scans. Please check your connection and ensure that:\n\ + - The server URL is reachable.\n\ + - Your authentication token is valid.\n\n\ + Check out our docs at https://docs.corgea.app/install_cli#login-with-the-cli" + ); + } + std::process::exit(1); + } + }; + // JSON mode stays a valid machine envelope even when empty. + if *json { + let output = json!({ + "page": page, + "total_pages": total_pages, + "results": scans + }); + println!("{}", serde_json::to_string_pretty(&output).unwrap()); + return; } - Err(e) => { - if e.to_string().contains("404") { - log::error!("Project with name '{}' doesn't exist. Please run 'corgea scan' to create a new scan for this project.", project_name); + // Human mode: never render a silent empty table on a miss. + if scans.is_empty() { + if resolved.confirmed { + println!( + "Project '{}' has no scans yet. Run 'corgea scan' to create one.", + project_name + ); } else { - log::error!( - "Unable to fetch scans. Please check your connection and ensure that:\n\ - - The server URL is reachable.\n\ - - Your authentication token is valid.\n\n\ - Check out our docs at https://docs.corgea.app/install_cli#login-with-the-cli" + println!( + "No Corgea project found for {}. Run 'corgea scan' to create one, or pass --project-name .", + resolved.tried_label ); } - std::process::exit(1); + return; } - }; - if *json { - let output = json!({ - "page": page, - "total_pages": total_pages, - "results": scans - }); - println!("{}", serde_json::to_string_pretty(&output).unwrap()); - return; - } - let mut table = vec![vec![ - "Scan ID".to_string(), - "Project".to_string(), - "Status".to_string(), - "Repo".to_string(), - "Branch".to_string(), - ]]; + let mut table = vec![vec![ + "Scan ID".to_string(), + "Project".to_string(), + "Status".to_string(), + "Repo".to_string(), + "Branch".to_string(), + ]]; - for scan in &scans { - let formatted_repo = scan.repo.clone().unwrap_or("N/A".to_string()); - let formatted_repo = if formatted_repo != "N/A" { - if let Some(repo_name) = formatted_repo.split('/').next_back() { - let owner = formatted_repo.split('/').nth(3).unwrap_or("unknown"); - let repo_name = repo_name.strip_suffix(".git").unwrap_or(repo_name); - format!("{}/{}", owner, repo_name) + for scan in &scans { + let formatted_repo = scan.repo.clone().unwrap_or("N/A".to_string()); + let formatted_repo = if formatted_repo != "N/A" { + if let Some(repo_name) = formatted_repo.split('/').next_back() { + let owner = formatted_repo.split('/').nth(3).unwrap_or("unknown"); + let repo_name = repo_name.strip_suffix(".git").unwrap_or(repo_name); + format!("{}/{}", owner, repo_name) + } else { + formatted_repo + } } else { formatted_repo - } - } else { - formatted_repo - }; + }; - table.push(vec![ - scan.id.clone(), - scan.project.clone(), - scan.status.clone(), - formatted_repo, - scan.branch.clone().unwrap_or("N/A".to_string()), - ]); - } + table.push(vec![ + scan.id.clone(), + scan.project.clone(), + scan.status.clone(), + formatted_repo, + scan.branch.clone().unwrap_or("N/A".to_string()), + ]); + } - utils::terminal::print_table(table, page, total_pages); + utils::terminal::print_table(table, page, total_pages); + } } } diff --git a/src/main.rs b/src/main.rs index de89adb..0658bab 100644 --- a/src/main.rs +++ b/src/main.rs @@ -134,7 +134,20 @@ enum Commands { project_name: Option, }, /// Wait for the latest in progress scan - Wait { scan_id: Option }, + Wait { + scan_id: Option, + #[arg( + long, + conflicts_with = "repo", + help = "Query this exact Corgea project name directly (skips repo auto-resolution)." + )] + project_name: Option, + #[arg( + long, + help = "Resolve the project from this repo (org/repo slug or remote URL) instead of the git remote." + )] + repo: Option, + }, /// List something, by default it lists the scans #[command(alias = "ls")] List { @@ -159,6 +172,19 @@ enum Commands { #[arg(long, value_parser = clap::value_parser!(u16), help = "Number of items per page")] page_size: Option, + + #[arg( + long, + conflicts_with = "repo", + help = "Query this exact Corgea project name directly (skips repo auto-resolution)." + )] + project_name: Option, + + #[arg( + long, + help = "Resolve the project from this repo (org/repo slug or remote URL) instead of the git remote." + )] + repo: Option, }, /// Inspect something, by default it will inspect a scan Inspect { @@ -600,9 +626,18 @@ fn main() { ), } } - Some(Commands::Wait { scan_id }) => { + Some(Commands::Wait { + scan_id, + project_name, + repo, + }) => { verify_token_and_exit_when_fail(&corgea_config); - wait::run(&corgea_config, scan_id.clone(), None); + wait::run( + &corgea_config, + scan_id.clone(), + project_name.clone(), + repo.clone(), + ); } Some(Commands::List { issues, @@ -611,6 +646,8 @@ fn main() { page_size, scan_id, sca_issues, + project_name, + repo, }) => { verify_token_and_exit_when_fail(&corgea_config); if *issues && *sca_issues { @@ -629,6 +666,8 @@ fn main() { page, page_size, scan_id, + project_name.clone(), + repo.clone(), ); } Some(Commands::Inspect { diff --git a/src/scan.rs b/src/scan.rs index af1fd4a..cb6ec0b 100644 --- a/src/scan.rs +++ b/src/scan.rs @@ -48,7 +48,6 @@ pub fn run_command(base_cmd: &String, mut command: Command) -> String { pub struct ScanUploadResult { pub scan_id: String, - pub project_id: Option, } pub fn run_semgrep(config: &Config, project_name: Option) { @@ -65,8 +64,10 @@ pub fn run_semgrep(config: &Config, project_name: Option) { let output = run_command(&base_command.to_string(), command); - if let Some(result) = parse_scan(config, output, true, project_name) { - crate::wait::run(config, Some(result.scan_id), result.project_id); + if let Some(result) = parse_scan(config, output, true, project_name.clone()) { + // Preserve an explicit --project-name for the post-scan wait; when None, + // wait resolves by the git remote / CWD (COR-1577). + crate::wait::run(config, Some(result.scan_id), project_name, None); } } @@ -80,8 +81,10 @@ pub fn run_snyk(config: &Config, project_name: Option) { let output = run_command(&base_command.to_string(), command); - if let Some(result) = parse_scan(config, output, true, project_name) { - crate::wait::run(config, Some(result.scan_id), result.project_id); + if let Some(result) = parse_scan(config, output, true, project_name.clone()) { + // Preserve an explicit --project-name for the post-scan wait; when None, + // wait resolves by the git remote / CWD (COR-1577). + crate::wait::run(config, Some(result.scan_id), project_name, None); } } @@ -369,7 +372,6 @@ pub fn upload_scan( }; let mut sast_scan_id: Option = None; - let mut project_id: Option = None; let mut upload_failed = false; @@ -396,13 +398,6 @@ pub fn upload_scan( sast_scan_id = Some(id_num.to_string()); } } - if let Some(pid_val) = json.get("project_id") { - if let Some(pid_str) = pid_val.as_str() { - project_id = Some(pid_str.to_string()); - } else if let Some(pid_num) = pid_val.as_i64() { - project_id = Some(pid_num.to_string()); - } - } } Err(e) => { log::warn!("Failed to parse response JSON: {}", e); @@ -514,8 +509,5 @@ pub fn upload_scan( println!("Go to {base_url} to see results."); - sast_scan_id.map(|scan_id| ScanUploadResult { - scan_id, - project_id, - }) + sast_scan_id.map(|scan_id| ScanUploadResult { scan_id }) } diff --git a/src/utils/api.rs b/src/utils/api.rs index a504760..ce34062 100644 --- a/src/utils/api.rs +++ b/src/utils/api.rs @@ -729,6 +729,171 @@ pub fn query_scan_list( } } +#[derive(Deserialize, Debug)] +pub struct ProjectSummary { + // Project.id is an integer in the envelope; model it tolerantly so an + // unexpected string id (or a missing field) never fails deserialization. + #[serde(default)] + pub id: serde_json::Value, + pub name: String, + #[serde(default)] + pub repo_url: Option, +} + +#[derive(Deserialize, Debug)] +pub struct ProjectsResponse { + // Models the `@paginated` envelope status; deserialized for completeness but + // not consumed (the slug guard + non-2xx/parse handling already gate misses). + #[allow(dead_code)] + pub status: String, + #[serde(default)] + pub projects: Option>, +} + +/// Stringify a scalar JSON id (number -> "123", string -> as-is) for the wait +/// scan URL. Returns None for null/array/object/missing so a bogus id never +/// becomes part of a URL like `/project/null/`. +fn id_to_string(v: &serde_json::Value) -> Option { + match v { + serde_json::Value::String(s) => Some(s.clone()), + serde_json::Value::Number(n) => Some(n.to_string()), + _ => None, + } +} + +/// Resolve the canonical project for a repo slug via GET /api/v1/projects?repo_url=… +/// +/// Old-backend safety guard: a pre-COR-1426 backend ignores the unknown +/// `repo_url` param and returns ALL company projects. Keep only candidates +/// whose returned `repo_url` (case-insensitive) contains the slug; on an old +/// backend none match -> Ok(None) -> caller falls back to the CWD-name path. +pub fn resolve_project_by_repo( + url: &str, + slug: &str, +) -> Result, Box> { + let request_url = format!("{}{}/projects", url, API_BASE); + let client = http_client(); + debug(&format!( + "Resolving project via {} (repo_url={})", + request_url, slug + )); + let response = client + .get(&request_url) + .query(&[("repo_url", slug)]) + .send()?; + check_for_warnings(response.headers(), response.status()); + if !response.status().is_success() { + // Endpoint absent/erroring on a very old backend -> treat as unresolved. + return Ok(None); + } + let text = response.text()?; + let parsed: ProjectsResponse = match serde_json::from_str(&text) { + Ok(p) => p, + Err(e) => { + debug(&format!( + "Failed to parse /projects response: {} | body: {}", + e, text + )); + return Ok(None); + } + }; + let slug_lc = slug.to_lowercase(); + let matched = parsed.projects.unwrap_or_default().into_iter().find(|p| { + p.repo_url + .as_deref() + .map(|r| r.to_lowercase().contains(&slug_lc)) + .unwrap_or(false) + }); + Ok(matched) +} + +/// What `list`/`wait` need to drive the existing name-based queries. +#[derive(Debug)] +pub struct ResolvedProject { + /// Sent as `?project=` to the listing endpoints. + pub query_name: String, + /// True only when a backend project was confirmed via /projects. Drives + /// confirmed-but-empty vs unresolved-miss messaging. + pub confirmed: bool, + /// Numeric id (stringified) when confirmed and the id is a scalar — used + /// for the wait scan URL. None otherwise (falls back to query_name). + pub project_id: Option, + /// What we looked for ("project 'x'" / "repo 'org/repo'" / "directory + /// 'dir'"), pre-formatted for the miss message. + pub tried_label: String, +} + +/// Resolve which project `list`/`wait` should query. +/// 1) --project-name -> use verbatim, skip resolution. +/// 2) else slug from --repo or the git remote -> resolve_project_by_repo: +/// confirmed -> canonical {name,id}. +/// 3) unconfirmed: explicit --repo -> query the slug as a name (never the CWD); +/// auto-detected remote -> CWD basename (no-regression fallback). +/// 4) no remote -> CWD basename (today's behavior). +pub fn resolve_project( + url: &str, + project_name_override: Option<&str>, + repo_override: Option<&str>, +) -> ResolvedProject { + if let Some(name) = project_name_override { + return ResolvedProject { + query_name: name.to_string(), + confirmed: false, + project_id: None, + tried_label: format!("project '{}'", name), + }; + } + + // Explicit --repo vs auto-detected remote changes the unconfirmed fallback. + let repo_was_explicit = repo_override.is_some(); + // --repo may already be a bare `org/repo` slug (extract_repo_slug needs a + // host segment, so it returns None for a 2-segment value) -> use raw value. + let slug = match repo_override { + Some(r) => utils::generic::extract_repo_slug(r).or_else(|| Some(r.to_string())), + None => utils::generic::get_repo_info("./") + .ok() + .flatten() + .and_then(|info| info.repo_url) + .and_then(|u| utils::generic::extract_repo_slug(&u)), + }; + + // CWD basename, resolved lazily — only the fallback paths below read it. + let cwd = + || utils::generic::get_current_working_directory().unwrap_or_else(|| "unknown".to_string()); + + if let Some(slug) = slug { + if let Ok(Some(project)) = resolve_project_by_repo(url, &slug) { + return ResolvedProject { + query_name: project.name, + confirmed: true, + project_id: id_to_string(&project.id), + tried_label: format!("repo '{}'", slug), + }; + } + // Unconfirmed: explicit --repo queries the slug as a name (clean miss, + // never wrong CWD results); auto-detected remote keeps the CWD fallback. + let query_name = if repo_was_explicit { + slug.clone() + } else { + cwd() + }; + return ResolvedProject { + query_name, + confirmed: false, + project_id: None, + tried_label: format!("repo '{}'", slug), + }; + } + + let cwd = cwd(); + ResolvedProject { + tried_label: format!("directory '{}'", cwd), + query_name: cwd, + confirmed: false, + project_id: None, + } +} + pub fn exchange_code_for_token( base_url: &str, code: &str, @@ -1302,4 +1467,69 @@ mod tests { assert!(result.is_err()); assert_eq!(attempts.get(), RETRY_BACKOFF_SECS.len() + 1); } + + // Single-response-per-connection JSON stub on an ephemeral port; returns base URL. + fn spawn_projects_stub(body: &'static str) -> String { + use std::io::{Read, Write}; + let listener = TcpListener::bind("127.0.0.1:0").expect("bind stub"); + let base = format!("http://127.0.0.1:{}", listener.local_addr().unwrap().port()); + thread::spawn(move || { + for stream in listener.incoming() { + let Ok(mut stream) = stream else { continue }; + // Drain the request headers before responding. Closing the + // socket with an unread request still in the kernel buffer + // triggers a TCP RST that surfaces on the client as hyper + // `UnexpectedMessage` (flaky, timing-dependent). + let mut chunk = [0u8; 1024]; + let mut buf = Vec::new(); + while let Ok(n) = stream.read(&mut chunk) { + if n == 0 { + break; + } + buf.extend_from_slice(&chunk[..n]); + if buf.windows(4).any(|w| w == b"\r\n\r\n") { + break; + } + } + let resp = format!( + "HTTP/1.1 200 OK\r\nContent-Length: {}\r\nContent-Type: application/json\r\nConnection: close\r\n\r\n{}", + body.len(), + body + ); + let _ = stream.write_all(resp.as_bytes()); + } + }); + base + } + + #[test] + fn resolve_project_by_repo_keeps_only_repo_url_matches() { + // New backend: filter applied, one matching project returned. + let base = spawn_projects_stub( + r#"{"status":"ok","projects":[{"id":7,"name":"bohappdev/dotnet-azure-web-tsb","repo_url":"https://github.com/bohappdev/dotnet-azure-web-tsb"}]}"#, + ); + let got = resolve_project_by_repo(&base, "bohappdev/dotnet-azure-web-tsb").unwrap(); + assert_eq!( + got.map(|p| p.name).as_deref(), + Some("bohappdev/dotnet-azure-web-tsb") + ); + } + + #[test] + fn resolve_project_by_repo_guards_against_old_backend_returning_all() { + // Old backend ignores ?repo_url and returns unrelated projects -> guard -> None. + let base = spawn_projects_stub( + r#"{"status":"ok","projects":[{"id":1,"name":"other/repo","repo_url":"https://github.com/other/repo"},{"id":2,"name":"misc/thing","repo_url":"https://github.com/misc/thing"}]}"#, + ); + let got = resolve_project_by_repo(&base, "bohappdev/dotnet-azure-web-tsb").unwrap(); + assert!(got.is_none(), "non-matching projects must be discarded"); + } + + #[test] + fn resolve_project_by_repo_empty_projects_is_none() { + let base = spawn_projects_stub(r#"{"status":"ok","projects":[]}"#); + assert!(resolve_project_by_repo(&base, "org/repo") + .unwrap() + .is_none()); + } } diff --git a/src/utils/generic.rs b/src/utils/generic.rs index f07d013..6386b62 100644 --- a/src/utils/generic.rs +++ b/src/utils/generic.rs @@ -260,6 +260,48 @@ fn extract_repo_name_from_url(url: &str) -> Option { None } +/// Extract the `org/repo` slug from a git remote URL. Returns the last two +/// meaningful path segments so it is a substring of essentially every stored +/// (normalized) `repo_url` form. Distinct from `extract_repo_name_from_url`, +/// which returns only the final segment (`repo`). +/// +/// Handles: +/// https://github.com/org/repo(.git) -> org/repo +/// git@github.com:org/repo(.git) -> org/repo +/// ssh://git@github.com/org/repo -> org/repo +/// https://dev.azure.com/org/project/_git/repo -> project/_git/repo +/// +/// Azure `_git` is kept (and the preceding project segment included) because +/// doghouse `normalize_repo_url` stores Azure as `.../project/_git/repo` +/// (`heeler/models.py:208-212`) — `project/_git/repo` is a substring of that, +/// `project/repo` is NOT. Azure SSH remotes (`ssh.dev.azure.com/v3/...`, which +/// carry no `_git` segment) are a known limitation; users pass --project-name. +/// +/// Returns None when fewer than two path segments follow the host (a bare host +/// or garbage input). +pub fn extract_repo_slug(url: &str) -> Option { + let url = url.trim().trim_end_matches('/'); + let url = url.strip_suffix(".git").unwrap_or(url); + // Drop scheme (`https://`, `ssh://`, …) if present. + let url = url.rsplit("://").next().unwrap_or(url); + // Split host from path: URL forms use '/', scp-like `git@host:org/repo` + // uses ':'. After filtering empties, segments[0] is the host. + let segments: Vec<&str> = url.split(['/', ':']).filter(|s| !s.is_empty()).collect(); + if segments.len() < 3 { + return None; // need host + at least org + repo + } + let last = segments[segments.len() - 1]; + let prev = segments[segments.len() - 2]; + if prev == "_git" && segments.len() >= 4 { + // Azure DevOps: keep the project so the slug stays a substring of the + // normalized stored URL (.../project/_git/repo). + let project = segments[segments.len() - 3]; + Some(format!("{}/_git/{}", project, last)) + } else { + Some(format!("{}/{}", prev, last)) + } +} + pub fn get_env_var_if_exists(var_name: &str) -> Option { match env::var(var_name) { Ok(value) if !value.trim().is_empty() => Some(value), @@ -370,4 +412,55 @@ mod tests { added ); } + + #[test] + fn extract_repo_slug_handles_common_remote_forms() { + assert_eq!( + extract_repo_slug("https://github.com/org/repo.git").as_deref(), + Some("org/repo") + ); + assert_eq!( + extract_repo_slug("https://github.com/org/repo").as_deref(), + Some("org/repo") + ); + assert_eq!( + extract_repo_slug("git@github.com:org/repo.git").as_deref(), + Some("org/repo") + ); + assert_eq!( + extract_repo_slug("git@github.com:org/repo").as_deref(), + Some("org/repo") + ); + assert_eq!( + extract_repo_slug("ssh://git@github.com/org/repo").as_deref(), + Some("org/repo") + ); + assert_eq!( + extract_repo_slug("https://github.com/org/repo/").as_deref(), + Some("org/repo") + ); + // host:port should not leak the port into the slug + assert_eq!( + extract_repo_slug("https://git.example.com:8443/org/repo").as_deref(), + Some("org/repo") + ); + // Bank of Hope case + assert_eq!( + extract_repo_slug("git@github.com:bohappdev/dotnet-azure-web-tsb.git").as_deref(), + Some("bohappdev/dotnet-azure-web-tsb") + ); + // Azure DevOps `_git` HTTPS -> keeps project + _git so it stays a substring + // of the normalized stored URL. + assert_eq!( + extract_repo_slug("https://dev.azure.com/org/project/_git/repo").as_deref(), + Some("project/_git/repo") + ); + } + + #[test] + fn extract_repo_slug_returns_none_for_unsplittable_input() { + assert_eq!(extract_repo_slug("not a url"), None); + assert_eq!(extract_repo_slug(""), None); + assert_eq!(extract_repo_slug("github.com"), None); // host only + } } diff --git a/src/wait.rs b/src/wait.rs index fdd69bb..6039548 100644 --- a/src/wait.rs +++ b/src/wait.rs @@ -2,14 +2,18 @@ use crate::config::Config; use crate::scanners::blast; use crate::utils; -pub fn run(config: &Config, scan_id: Option, project_id: Option) { - let project_name = match utils::generic::get_current_working_directory() { - Some(name) => name, - None => { - log::error!("Unable to retrieve the current working directory. Please check your permissions and try again."); - std::process::exit(1); - } - }; +pub fn run( + config: &Config, + scan_id: Option, + project_name_override: Option, + repo_override: Option, +) { + let resolved = utils::api::resolve_project( + &config.get_url(), + project_name_override.as_deref(), + repo_override.as_deref(), + ); + let project_name = resolved.query_name.clone(); let scans_result = utils::api::query_scan_list(&config.get_url(), Some(&project_name), Some(1), None); @@ -45,13 +49,23 @@ pub fn run(config: &Config, scan_id: Option, project_id: Option) None => match scans.first() { Some(scan) => (scan.id.clone(), scan.status == "Complete"), None => { - log::error!("Error querying scan list"); + if resolved.confirmed { + log::error!( + "Project '{}' has no scans yet. Run 'corgea scan' to start one.", + project_name + ); + } else { + log::error!( + "No scan to wait for: no Corgea project found for {}. Run 'corgea scan', or pass --scan-id / --project-name.", + resolved.tried_label + ); + } std::process::exit(1); } }, }; - let scan_url = match &project_id { + let scan_url = match &resolved.project_id { Some(pid) => format!("{}/project/{}/?scan_id={}", config.get_url(), pid, scan_id), None => format!( "{}/project/{}?scan_id={}", diff --git a/tests/common/mod.rs b/tests/common/mod.rs index bc4ec03..a8fb3af 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -433,3 +433,62 @@ pub fn tree_harness( .vuln_statuses(statuses) .build() } + +// --- project-resolution e2e fixtures (shared by list_resolution.rs and +// wait_resolution.rs) ------------------------------------------------------- + +/// Canonical project name for the Bank-of-Hope resolution case: the dir +/// basename (`dotnet-azure-web-tsb`) differs from the stored project name. +#[allow(dead_code)] +pub const CANON: &str = "bohappdev/dotnet-azure-web-tsb"; +/// Git remote whose slug resolves to `CANON`. +#[allow(dead_code)] +pub const REMOTE: &str = "https://github.com/bohappdev/dotnet-azure-web-tsb.git"; + +/// `/projects` hit returning the canonical project whose `repo_url` contains +/// the slug (id 7) — the new-backend confirmed path. +#[allow(dead_code)] +pub fn projects_match() -> String { + r#"{"status":"ok","projects":[{"id":7,"name":"bohappdev/dotnet-azure-web-tsb","repo_url":"https://github.com/bohappdev/dotnet-azure-web-tsb"}]}"#.to_string() +} + +/// `/projects` miss (repo not onboarded / pre-COR-1426 backend filtered out). +#[allow(dead_code)] +pub fn projects_empty() -> String { + r#"{"status":"ok","projects":[]}"#.to_string() +} + +/// `/scans` returning one `Complete` scan under `project`. +#[allow(dead_code)] +pub fn scans_one(project: &str) -> String { + format!( + r#"{{"status":"ok","page":1,"total_pages":1,"scans":[{{"id":"scan-123","project":"{project}","repo":"https://github.com/bohappdev/dotnet-azure-web-tsb","branch":"main","status":"Complete","engine":"blast","created_at":"2026-01-01T00:00:00Z"}}]}}"# + ) +} + +/// `/scans` returning an empty page. +#[allow(dead_code)] +pub fn scans_empty() -> String { + r#"{"status":"ok","page":1,"total_pages":1,"scans":[]}"#.to_string() +} + +/// Temp git repo at `/` with `origin` set to `remote`. The dir +/// basename is the caller's to choose so it can differ from the stored name. +#[allow(dead_code)] +pub fn temp_git_repo(dirname: &str, remote: &str) -> (TempDir, std::path::PathBuf) { + let tmp = TempDir::new().expect("temp dir"); + let repo_dir = tmp.path().join(dirname); + std::fs::create_dir(&repo_dir).expect("create repo dir"); + let repo = git2::Repository::init(&repo_dir).expect("git init"); + repo.remote("origin", remote).expect("set origin"); + (tmp, repo_dir) +} + +/// Temp NON-git dir at `/` (no remote). +#[allow(dead_code)] +pub fn temp_plain_dir(dirname: &str) -> (TempDir, std::path::PathBuf) { + let tmp = TempDir::new().expect("temp dir"); + let dir = tmp.path().join(dirname); + std::fs::create_dir(&dir).expect("create dir"); + (tmp, dir) +} diff --git a/tests/list_resolution.rs b/tests/list_resolution.rs new file mode 100644 index 0000000..a33d005 --- /dev/null +++ b/tests/list_resolution.rs @@ -0,0 +1,262 @@ +//! End-to-end tests for `corgea list` repo-URL project resolution (COR-1577). +//! +//! Each test runs the real binary against a one-response-per-connection HTTP +//! stub (`common::spawn_http_stub`) and, where a git remote matters, a temp +//! git repo built with `git2` whose directory basename DIFFERS from the stored +//! canonical project name (the Bank of Hope case: dir `dotnet-azure-web-tsb` +//! vs project `bohappdev/dotnet-azure-web-tsb`). +//! +//! Routing matches on the request-target PATH PREFIX with `starts_with`: the +//! `/projects` request carries a percent-encoded query string +//! (`?repo_url=bohappdev%2Fdotnet-azure-web-tsb`), so the full target is not a +//! stable key. `verify_token_and_exit_when_fail` calls `GET /api/v1/verify` +//! first, so every stub serves it. + +mod common; + +use common::{ + projects_empty, projects_match, scans_empty, scans_one, temp_git_repo, temp_plain_dir, CANON, + REMOTE, +}; +use std::path::Path; +use std::process::Output; + +// --- stub bodies ----------------------------------------------------------- + +/// `/issues` returning one issue (status `ok`). +fn issues_one() -> String { + r#"{"status":"ok","page":1,"total_pages":1,"total_issues":1,"issues":[{"id":"issue-abc","scan_id":"scan-123","status":"open","urgency":"high","created_at":"2026-01-01T00:00:00Z","classification":{"id":"CWE-89","name":"SQL Injection","description":null},"location":{"file":{"name":"app.py","language":"python","path":"src/app.py"},"line_number":42,"project":{"name":"bohappdev/dotnet-azure-web-tsb","branch":null,"git_sha":null}},"details":null,"auto_triage":{"false_positive_detection":{"status":"none","reasoning":null}},"auto_fix_suggestion":null}]}"#.to_string() +} + +/// `/issues` exact-name miss (HTTP 200 `no_project_found`, mapped to 404). +fn issues_miss() -> String { + r#"{"status":"no_project_found"}"#.to_string() +} + +// --- harness --------------------------------------------------------------- + +/// Stub serving verify + the three listing endpoints, keyed on path prefix. +fn spawn_stub(projects: String, scans: String, issues: String) -> String { + common::spawn_http_stub(move |path| { + if path.starts_with("/api/v1/verify") { + ("200 OK", r#"{"status":"ok"}"#.to_string()) + } else if path.starts_with("/api/v1/projects?repo_url=") { + ("200 OK", projects.clone()) + } else if path.starts_with("/api/v1/scans?") { + ("200 OK", scans.clone()) + } else if path.starts_with("/api/v1/issues?") { + ("200 OK", issues.clone()) + } else { + ("404 Not Found", r#"{"message":"not found"}"#.to_string()) + } + }) +} + +/// Run `corgea list ` against `url` from `cwd`, isolated from the +/// host (temp HOME). `CORGEA_URL`/`CORGEA_TOKEN` are layered back on after +/// `corgea_isolated` strips them. +fn run_list(args: &[&str], url: &str, cwd: &Path) -> Output { + let (mut cmd, _home) = common::corgea_isolated(); + cmd.arg("list"); + cmd.args(args); + cmd.env("CORGEA_URL", url) + .env("CORGEA_TOKEN", "test-token") + .current_dir(cwd); + cmd.output().expect("spawn corgea") +} + +// --- tests ----------------------------------------------------------------- + +#[test] +fn list_uses_canonical_name_from_repo() { + let url = spawn_stub(projects_match(), scans_one(CANON), issues_one()); + let (_tmp, repo) = temp_git_repo("dotnet-azure-web-tsb", REMOTE); + let out = run_list(&[], &url, &repo); + let stdout = String::from_utf8_lossy(&out.stdout); + assert_eq!( + out.status.code(), + Some(0), + "stderr: {}", + String::from_utf8_lossy(&out.stderr) + ); + // Project column shows the canonical org/repo, proving resolution, not the + // dir basename, drove the listing. + assert!( + stdout.contains("bohappdev/dotnet-azure-web-tsb"), + "stdout: {stdout}" + ); + assert!(stdout.contains("scan-123"), "stdout: {stdout}"); +} + +#[test] +fn list_issues_shows_repo_resolved_issue() { + let url = spawn_stub(projects_match(), scans_one(CANON), issues_one()); + let (_tmp, repo) = temp_git_repo("dotnet-azure-web-tsb", REMOTE); + let out = run_list(&["--issues"], &url, &repo); + let stdout = String::from_utf8_lossy(&out.stdout); + assert_eq!( + out.status.code(), + Some(0), + "stderr: {}", + String::from_utf8_lossy(&out.stderr) + ); + assert!(stdout.contains("issue-abc"), "stdout: {stdout}"); +} + +#[test] +fn list_repo_flag_resolves_from_flag_not_remote() { + use std::sync::{Arc, Mutex}; + let hits = Arc::new(Mutex::new(Vec::::new())); + let hits_c = hits.clone(); + // Records every request target so we can prove the slug came from --repo. + let url = common::spawn_http_stub(move |path| { + hits_c.lock().unwrap().push(path.to_string()); + if path.starts_with("/api/v1/verify") { + ("200 OK", r#"{"status":"ok"}"#.to_string()) + } else if path.starts_with("/api/v1/projects?repo_url=") { + ("200 OK", projects_match()) + } else if path.starts_with("/api/v1/scans?") { + ("200 OK", scans_one(CANON)) + } else { + ("404 Not Found", r#"{"message":"not found"}"#.to_string()) + } + }); + // Non-git dir: no remote, so a resolved slug can ONLY have come from --repo. + let (_tmp, dir) = temp_plain_dir("unrelated-dir"); + let out = run_list(&["--repo", "bohappdev/dotnet-azure-web-tsb"], &url, &dir); + let stdout = String::from_utf8_lossy(&out.stdout); + assert_eq!( + out.status.code(), + Some(0), + "stderr: {}", + String::from_utf8_lossy(&out.stderr) + ); + assert!( + stdout.contains("bohappdev/dotnet-azure-web-tsb"), + "stdout: {stdout}" + ); + let hits = hits.lock().unwrap(); + assert!( + hits.iter() + .any(|h| h.starts_with("/api/v1/projects?repo_url=bohappdev%2Fdotnet-azure-web-tsb")), + "expected a /projects hit carrying the flag slug; hits: {hits:?}" + ); +} + +#[test] +fn list_miss_names_repo_no_empty_table() { + let url = spawn_stub(projects_empty(), scans_empty(), issues_miss()); + let (_tmp, repo) = temp_git_repo("dotnet-azure-web-tsb", REMOTE); + let out = run_list(&[], &url, &repo); + let stdout = String::from_utf8_lossy(&out.stdout); + assert_eq!( + out.status.code(), + Some(0), + "stderr: {}", + String::from_utf8_lossy(&out.stderr) + ); + assert!( + stdout.contains("repo 'bohappdev/dotnet-azure-web-tsb'"), + "stdout: {stdout}" + ); + assert!( + !stdout.contains("Scan ID"), + "should not render a table; stdout: {stdout}" + ); +} + +#[test] +fn list_no_remote_falls_back_to_cwd_name() { + // Regression: non-git dir whose basename matches a project the scans stub + // serves under that exact name still lists scans (CWD-name fallback). + let url = spawn_stub(projects_empty(), scans_one("myproject"), issues_one()); + let (_tmp, dir) = temp_plain_dir("myproject"); + let out = run_list(&[], &url, &dir); + let stdout = String::from_utf8_lossy(&out.stdout); + assert_eq!( + out.status.code(), + Some(0), + "stderr: {}", + String::from_utf8_lossy(&out.stderr) + ); + assert!(stdout.contains("myproject"), "stdout: {stdout}"); + assert!(stdout.contains("scan-123"), "stdout: {stdout}"); +} + +#[test] +fn list_project_name_override() { + let url = spawn_stub(projects_empty(), scans_one("some/name"), issues_one()); + let (_tmp, dir) = temp_plain_dir("whatever"); + let out = run_list(&["--project-name", "some/name"], &url, &dir); + let stdout = String::from_utf8_lossy(&out.stdout); + assert_eq!( + out.status.code(), + Some(0), + "stderr: {}", + String::from_utf8_lossy(&out.stderr) + ); + assert!(stdout.contains("some/name"), "stdout: {stdout}"); +} + +#[test] +fn list_project_name_and_repo_are_mutually_exclusive() { + let (_tmp, dir) = temp_plain_dir("whatever"); + let (mut cmd, _home) = common::corgea_isolated(); + cmd.args(["list", "--project-name", "a", "--repo", "b"]) + .env("CORGEA_URL", "http://127.0.0.1:1") + .env("CORGEA_TOKEN", "test-token") + .current_dir(&dir); + let out = cmd.output().expect("spawn corgea"); + // clap rejects conflicting args at parse time with a usage error, exit 2. + assert_eq!( + out.status.code(), + Some(2), + "stderr: {}", + String::from_utf8_lossy(&out.stderr) + ); +} + +#[test] +fn list_json_miss_is_valid_empty_envelope() { + let url = spawn_stub(projects_empty(), scans_empty(), issues_miss()); + let (_tmp, repo) = temp_git_repo("dotnet-azure-web-tsb", REMOTE); + let out = run_list(&["--json"], &url, &repo); + let stdout = String::from_utf8_lossy(&out.stdout); + assert_eq!( + out.status.code(), + Some(0), + "stderr: {}", + String::from_utf8_lossy(&out.stderr) + ); + let v: serde_json::Value = serde_json::from_str(stdout.trim()) + .unwrap_or_else(|e| panic!("stdout not JSON ({e}): {stdout}")); + assert_eq!( + v["results"].as_array().map(|a| a.len()), + Some(0), + "results should be empty; stdout: {stdout}" + ); + assert!( + !stdout.contains("No Corgea project"), + "no human prose on stdout; stdout: {stdout}" + ); +} + +#[test] +fn list_issues_json_miss_keeps_stdout_clean() { + let url = spawn_stub(projects_empty(), scans_empty(), issues_miss()); + let (_tmp, repo) = temp_git_repo("dotnet-azure-web-tsb", REMOTE); + let out = run_list(&["--issues", "--json"], &url, &repo); + let stdout = String::from_utf8_lossy(&out.stdout); + let stderr = String::from_utf8_lossy(&out.stderr); + // The --issues miss is a hard error via log::error! (stderr); stdout stays + // clean so JSON consumers never see corrupt output. + assert_eq!(out.status.code(), Some(1), "stderr: {stderr}"); + assert!( + stdout.trim().is_empty(), + "stdout must be clean; stdout: {stdout}" + ); + assert!( + stderr.contains("No Corgea project found"), + "stderr should name the miss; stderr: {stderr}" + ); +} diff --git a/tests/wait_resolution.rs b/tests/wait_resolution.rs new file mode 100644 index 0000000..28c01b3 --- /dev/null +++ b/tests/wait_resolution.rs @@ -0,0 +1,208 @@ +//! End-to-end tests for `corgea wait` repo-URL project resolution (COR-1577). +//! +//! Mirrors `tests/list_resolution.rs`: each test runs the real binary against a +//! one-response-per-connection HTTP stub (`common::spawn_http_stub`) and, where +//! a git remote matters, a temp git repo built with `git2` whose directory +//! basename DIFFERS from the stored canonical project name (the Bank of Hope +//! case: dir `dotnet-azure-web-tsb` vs project `bohappdev/dotnet-azure-web-tsb`). +//! +//! Routing matches on the request-target PATH PREFIX with `starts_with`: the +//! `/projects` request carries a percent-encoded query string +//! (`?repo_url=bohappdev%2Fdotnet-azure-web-tsb`), so the full target is not a +//! stable key. `verify_token_and_exit_when_fail` calls `GET /api/v1/verify` +//! first, so every stub serves it. The `/api/v1/scan/` arm serves BOTH +//! `GET /api/v1/scan/{id}` (so `check_scan_status` succeeds) and +//! `/api/v1/scan/{id}/issues?…` (so `report_scan_status` succeeds) — branching +//! on `contains("/issues")`. + +mod common; + +use common::{ + projects_empty, projects_match, scans_empty, scans_one, temp_git_repo, temp_plain_dir, CANON, + REMOTE, +}; +use std::path::Path; +use std::process::Output; + +// --- stub bodies ----------------------------------------------------------- + +/// `GET /api/v1/scan/{id}` returning a single completed scan (consumed by +/// `check_scan_status`/`get_scan`, which check the lowercase `complete`). +fn scan_complete() -> String { + r#"{"id":"scan-123","project":"bohappdev/dotnet-azure-web-tsb","repo":"https://github.com/bohappdev/dotnet-azure-web-tsb","branch":"main","status":"complete","engine":"blast","created_at":"2026-01-01T00:00:00Z"}"#.to_string() +} + +/// `GET /api/v1/scan/{id}/issues` returning an empty page (one round-trip: +/// `total_pages` 1). `report_scan_status` groups by urgency and succeeds with +/// no issues — enough to print the result link. +fn scan_issues_empty() -> String { + r#"{"status":"ok","page":1,"total_pages":1,"total_issues":0,"issues":[]}"#.to_string() +} + +// --- harness --------------------------------------------------------------- + +/// Stub serving verify + projects + scans + the single-scan/scan-issues +/// endpoints, keyed on path prefix. +fn spawn_stub(projects: String, scans: String) -> String { + common::spawn_http_stub(move |path| { + if path.starts_with("/api/v1/verify") { + ("200 OK", r#"{"status":"ok"}"#.to_string()) + } else if path.starts_with("/api/v1/projects?repo_url=") { + ("200 OK", projects.clone()) + } else if path.starts_with("/api/v1/scans?") { + ("200 OK", scans.clone()) + } else if path.starts_with("/api/v1/scan/") { + if path.contains("/issues") { + ("200 OK", scan_issues_empty()) + } else { + ("200 OK", scan_complete()) + } + } else { + ("404 Not Found", r#"{"message":"not found"}"#.to_string()) + } + }) +} + +/// Run `corgea wait ` against `url` from `cwd`, isolated from the host +/// (temp HOME). `CORGEA_URL`/`CORGEA_TOKEN` are layered back on after +/// `corgea_isolated` strips them. +fn run_wait(args: &[&str], url: &str, cwd: &Path) -> Output { + let (mut cmd, _home) = common::corgea_isolated(); + cmd.arg("wait"); + cmd.args(args); + cmd.env("CORGEA_URL", url) + .env("CORGEA_TOKEN", "test-token") + .current_dir(cwd); + cmd.output().expect("spawn corgea") +} + +// --- tests ----------------------------------------------------------------- + +#[test] +fn wait_uses_canonical_project_and_numeric_id_scan_url() { + let url = spawn_stub(projects_match(), scans_one(CANON)); + let (_tmp, repo) = temp_git_repo("dotnet-azure-web-tsb", REMOTE); + let out = run_wait(&[], &url, &repo); + let stdout = String::from_utf8_lossy(&out.stdout); + assert_eq!( + out.status.code(), + Some(0), + "stderr: {}", + String::from_utf8_lossy(&out.stderr) + ); + // The scan URL uses the numeric project id (7) returned by /projects, + // proving `resolved.project_id` flows through — not the dir basename. + assert!( + stdout.contains("/project/7/?scan_id=scan-123"), + "stdout: {stdout}" + ); +} + +#[test] +fn wait_repo_flag_resolves_from_flag_not_remote() { + use std::sync::{Arc, Mutex}; + let hits = Arc::new(Mutex::new(Vec::::new())); + let hits_c = hits.clone(); + // Records every request target so we can prove the slug came from --repo. + let url = common::spawn_http_stub(move |path| { + hits_c.lock().unwrap().push(path.to_string()); + if path.starts_with("/api/v1/verify") { + ("200 OK", r#"{"status":"ok"}"#.to_string()) + } else if path.starts_with("/api/v1/projects?repo_url=") { + ("200 OK", projects_match()) + } else if path.starts_with("/api/v1/scans?") { + ("200 OK", scans_one(CANON)) + } else if path.starts_with("/api/v1/scan/") { + if path.contains("/issues") { + ("200 OK", scan_issues_empty()) + } else { + ("200 OK", scan_complete()) + } + } else { + ("404 Not Found", r#"{"message":"not found"}"#.to_string()) + } + }); + // Non-git dir: no remote, so a resolved slug can ONLY have come from --repo. + let (_tmp, dir) = temp_plain_dir("unrelated-dir"); + let out = run_wait(&["--repo", "bohappdev/dotnet-azure-web-tsb"], &url, &dir); + let stdout = String::from_utf8_lossy(&out.stdout); + assert_eq!( + out.status.code(), + Some(0), + "stderr: {}", + String::from_utf8_lossy(&out.stderr) + ); + assert!( + stdout.contains("/project/7/?scan_id=scan-123"), + "stdout: {stdout}" + ); + let hits = hits.lock().unwrap(); + assert!( + hits.iter() + .any(|h| h.starts_with("/api/v1/projects?repo_url=bohappdev%2Fdotnet-azure-web-tsb")), + "expected a /projects hit carrying the flag slug; hits: {hits:?}" + ); +} + +#[test] +fn wait_miss_names_repo_no_bare_error() { + let url = spawn_stub(projects_empty(), scans_empty()); + let (_tmp, repo) = temp_git_repo("dotnet-azure-web-tsb", REMOTE); + let out = run_wait(&[], &url, &repo); + let stdout = String::from_utf8_lossy(&out.stdout); + let stderr = String::from_utf8_lossy(&out.stderr); + assert_eq!( + out.status.code(), + Some(1), + "stdout: {stdout}\nstderr: {stderr}" + ); + // Clear, actionable miss naming the repo the resolver tried. + assert!( + stderr.contains("repo 'bohappdev/dotnet-azure-web-tsb'"), + "stderr should name the repo; stderr: {stderr}" + ); + // The bare cryptic error is gone for good. + assert!( + !stdout.contains("Error querying scan list") + && !stderr.contains("Error querying scan list"), + "the bare error must be absent; stdout: {stdout}\nstderr: {stderr}" + ); +} + +#[test] +fn wait_project_name_override() { + let url = spawn_stub(projects_empty(), scans_one("some/name")); + let (_tmp, dir) = temp_plain_dir("whatever"); + let out = run_wait(&["--project-name", "some/name"], &url, &dir); + let stdout = String::from_utf8_lossy(&out.stdout); + assert_eq!( + out.status.code(), + Some(0), + "stderr: {}", + String::from_utf8_lossy(&out.stderr) + ); + // Override skips resolution: no confirmed project id, so the scan URL falls + // back to the name form keyed on the exact `--project-name` value. + assert!( + stdout.contains("/project/some/name?scan_id=scan-123"), + "stdout: {stdout}" + ); +} + +#[test] +fn wait_project_name_and_repo_are_mutually_exclusive() { + let (_tmp, dir) = temp_plain_dir("whatever"); + let (mut cmd, _home) = common::corgea_isolated(); + cmd.args(["wait", "--project-name", "a", "--repo", "b"]) + .env("CORGEA_URL", "http://127.0.0.1:1") + .env("CORGEA_TOKEN", "test-token") + .current_dir(&dir); + let out = cmd.output().expect("spawn corgea"); + // clap rejects conflicting args at parse time with a usage error, exit 2. + assert_eq!( + out.status.code(), + Some(2), + "stderr: {}", + String::from_utf8_lossy(&out.stderr) + ); +} From 916df2079a7e507060f75c7d2fdb206d7c931212 Mon Sep 17 00:00:00 2001 From: juangaitanv Date: Tue, 30 Jun 2026 10:35:50 +0200 Subject: [PATCH 02/16] fix(cli): harden list/wait project resolution (PR #122 review) Addresses four review findings on the COR-1577 repo-URL resolver: 1. Boundary-aware repo match (was a substring `contains`): the backend's `repo_url__icontains` returns siblings like `org/repo-v2` for slug `org/repo`, and the old guard could confirm the wrong project. Now matches on a path-segment boundary (equal or ends-with `/`, after stripping `.git`/trailing slash). 2. Subdirectory invocation: resolution used `Repository::open`, which only succeeds at the repo root, so `list`/`wait` from a subdir fell back to the subdir basename. New `discover_repo_url` walks up via `Repository::discover`. 3. Surface hard resolver failures: `resolve_project` now returns a Result and propagates network/auth/5xx from `/projects` instead of silently falling back to the local-directory project; a clean no-match (or 404 on an old backend) stays a soft fallback. 4. Skip redundant resolution: `list --issues --scan-id` hits `/scan/{id}/issues`, which ignores the project, so the extra `/projects` round-trip is now skipped. Adds unit tests (boundary match, sibling rejection, 404-soft vs 5xx-hard) and integration tests (no `/projects` call for `--issues --scan-id`, resolution from a subdirectory). --- src/list.rs | 45 +++++++++++--- src/utils/api.rs | 130 ++++++++++++++++++++++++++++++++------- src/utils/generic.rs | 13 ++++ src/wait.rs | 17 ++++- tests/list_resolution.rs | 87 ++++++++++++++++++++++++++ 5 files changed, 259 insertions(+), 33 deletions(-) diff --git a/src/list.rs b/src/list.rs index 43f66c3..bc70287 100644 --- a/src/list.rs +++ b/src/list.rs @@ -116,13 +116,35 @@ pub fn run( Some(sca_issues_response.total_pages), ); } else { - // Resolve once for both the --issues and scan-listing paths. - let resolved = utils::api::resolve_project( - &config.get_url(), - project_name_override.as_deref(), - repo_override.as_deref(), - ); - let project_name = resolved.query_name.clone(); + // The --scan-id issue route hits /scan/{id}/issues and ignores the + // project, so skip the extra /projects resolution in that one mode; + // every other path here queries by project and needs it resolved. + let resolved: Option = if *issues && scan_id.is_some() { + None + } else { + match utils::api::resolve_project( + &config.get_url(), + project_name_override.as_deref(), + repo_override.as_deref(), + ) { + Ok(resolved) => Some(resolved), + Err(e) => { + log::error!( + "Unable to resolve the Corgea project. Please check your connection and ensure that:\n\ + - The server URL is reachable.\n\ + - Your authentication token is valid.\n\n\ + Check out our docs at https://docs.corgea.app/install_cli#login-with-the-cli\n\n\ + Error details: {}", + e + ); + std::process::exit(1); + } + } + }; + let project_name = resolved + .as_ref() + .map(|r| r.query_name.clone()) + .unwrap_or_default(); if *issues { let issues_response = match utils::api::get_scan_issues( &config.get_url(), @@ -137,12 +159,12 @@ pub fn run( if e.to_string().contains("404") { if scan_id.is_some() { log::error!("Scan with ID '{}' doesn't exist. Please run 'corgea scan' to create a new scan for this project.", scan_id.as_ref().unwrap()); - } else if resolved.confirmed { + } else if resolved.as_ref().map(|r| r.confirmed).unwrap_or(false) { log::error!("Project '{}' has no issues yet. Run 'corgea scan' to create a scan for this project.", project_name); } else { log::error!( "No Corgea project found for {}. Run 'corgea scan' to create one, or pass --project-name .", - resolved.tried_label + resolved.as_ref().map(|r| r.tried_label.as_str()).unwrap_or_default() ); } } else { @@ -281,6 +303,11 @@ pub fn run( utils::terminal::print_table(table, issues_response.page, issues_response.total_pages); } else { + // Scan-listing always resolves (the skip only applies to + // --issues --scan-id), so `resolved` is present here. + let resolved = resolved + .as_ref() + .expect("scan listing always resolves the project"); let (scans, page, total_pages) = match utils::api::query_scan_list( &config.get_url(), Some(&project_name), diff --git a/src/utils/api.rs b/src/utils/api.rs index ce34062..080a1b7 100644 --- a/src/utils/api.rs +++ b/src/utils/api.rs @@ -761,12 +761,29 @@ fn id_to_string(v: &serde_json::Value) -> Option { } } +/// True when a stored `repo_url` refers to exactly `slug` (an `org/repo`-style +/// value, already lowercased). Matches on a path-segment boundary so a sibling +/// or prefix repo is never mistaken for the target: the backend's +/// `repo_url__icontains` filter returns `org/repo-v2` for slug `org/repo`, and a +/// plain substring check would wrongly accept it. Strips `.git`/trailing slash +/// first so `…/org/repo.git` and `…/org/repo/` both match `org/repo`. +fn repo_url_matches_slug(repo_url: &str, slug_lc: &str) -> bool { + let r = repo_url.trim().trim_end_matches('/'); + let r = r.strip_suffix(".git").unwrap_or(r).to_lowercase(); + r == slug_lc || r.ends_with(&format!("/{}", slug_lc)) +} + /// Resolve the canonical project for a repo slug via GET /api/v1/projects?repo_url=… /// /// Old-backend safety guard: a pre-COR-1426 backend ignores the unknown /// `repo_url` param and returns ALL company projects. Keep only candidates -/// whose returned `repo_url` (case-insensitive) contains the slug; on an old -/// backend none match -> Ok(None) -> caller falls back to the CWD-name path. +/// whose returned `repo_url` matches the slug on a path-segment boundary; on an +/// old backend none match -> Ok(None) -> caller falls back to the CWD-name path. +/// +/// Hard failures (network, auth, 5xx) return `Err` so the caller can surface the +/// backend problem instead of silently resolving to the local-directory project; +/// only a clean "no match" (or a 404 from a backend without the endpoint) is a +/// soft `Ok(None)`. pub fn resolve_project_by_repo( url: &str, slug: &str, @@ -782,10 +799,17 @@ pub fn resolve_project_by_repo( .query(&[("repo_url", slug)]) .send()?; check_for_warnings(response.headers(), response.status()); - if !response.status().is_success() { - // Endpoint absent/erroring on a very old backend -> treat as unresolved. + let status = response.status(); + if status == reqwest::StatusCode::NOT_FOUND { + // /projects absent on a very old backend -> soft miss; the caller falls + // back to the name-based path, so this stays a no-regression case. return Ok(None); } + if !status.is_success() { + // Auth (401/403) or server (5xx) failure: surface it rather than + // silently resolving to the local-directory project. + return Err(format!("/projects request failed: HTTP {}", status).into()); + } let text = response.text()?; let parsed: ProjectsResponse = match serde_json::from_str(&text) { Ok(p) => p, @@ -801,7 +825,7 @@ pub fn resolve_project_by_repo( let matched = parsed.projects.unwrap_or_default().into_iter().find(|p| { p.repo_url .as_deref() - .map(|r| r.to_lowercase().contains(&slug_lc)) + .map(|r| repo_url_matches_slug(r, &slug_lc)) .unwrap_or(false) }); Ok(matched) @@ -825,36 +849,38 @@ pub struct ResolvedProject { /// Resolve which project `list`/`wait` should query. /// 1) --project-name -> use verbatim, skip resolution. -/// 2) else slug from --repo or the git remote -> resolve_project_by_repo: +/// 2) else slug from --repo or the discovered git remote -> resolve_project_by_repo: /// confirmed -> canonical {name,id}. /// 3) unconfirmed: explicit --repo -> query the slug as a name (never the CWD); /// auto-detected remote -> CWD basename (no-regression fallback). /// 4) no remote -> CWD basename (today's behavior). +/// +/// Returns `Err` only for a hard resolver failure (network/auth/5xx from +/// /projects); a clean "no match" resolves to the fallback name above. pub fn resolve_project( url: &str, project_name_override: Option<&str>, repo_override: Option<&str>, -) -> ResolvedProject { +) -> Result> { if let Some(name) = project_name_override { - return ResolvedProject { + return Ok(ResolvedProject { query_name: name.to_string(), confirmed: false, project_id: None, tried_label: format!("project '{}'", name), - }; + }); } // Explicit --repo vs auto-detected remote changes the unconfirmed fallback. let repo_was_explicit = repo_override.is_some(); // --repo may already be a bare `org/repo` slug (extract_repo_slug needs a // host segment, so it returns None for a 2-segment value) -> use raw value. + // No --repo: discover the enclosing repo's remote (works from a subdir). let slug = match repo_override { Some(r) => utils::generic::extract_repo_slug(r).or_else(|| Some(r.to_string())), - None => utils::generic::get_repo_info("./") - .ok() - .flatten() - .and_then(|info| info.repo_url) - .and_then(|u| utils::generic::extract_repo_slug(&u)), + None => { + utils::generic::discover_repo_url().and_then(|u| utils::generic::extract_repo_slug(&u)) + } }; // CWD basename, resolved lazily — only the fallback paths below read it. @@ -862,13 +888,15 @@ pub fn resolve_project( || utils::generic::get_current_working_directory().unwrap_or_else(|| "unknown".to_string()); if let Some(slug) = slug { - if let Ok(Some(project)) = resolve_project_by_repo(url, &slug) { - return ResolvedProject { + // `?` propagates hard resolver failures (network/auth/5xx); only a clean + // Ok(None) "no match" falls through to the name-based fallback below. + if let Some(project) = resolve_project_by_repo(url, &slug)? { + return Ok(ResolvedProject { query_name: project.name, confirmed: true, project_id: id_to_string(&project.id), tried_label: format!("repo '{}'", slug), - }; + }); } // Unconfirmed: explicit --repo queries the slug as a name (clean miss, // never wrong CWD results); auto-detected remote keeps the CWD fallback. @@ -877,21 +905,21 @@ pub fn resolve_project( } else { cwd() }; - return ResolvedProject { + return Ok(ResolvedProject { query_name, confirmed: false, project_id: None, tried_label: format!("repo '{}'", slug), - }; + }); } let cwd = cwd(); - ResolvedProject { + Ok(ResolvedProject { tried_label: format!("directory '{}'", cwd), query_name: cwd, confirmed: false, project_id: None, - } + }) } pub fn exchange_code_for_token( @@ -1470,6 +1498,12 @@ mod tests { // Single-response-per-connection JSON stub on an ephemeral port; returns base URL. fn spawn_projects_stub(body: &'static str) -> String { + spawn_projects_stub_status("200 OK", body) + } + + // As `spawn_projects_stub` but with a caller-chosen status line, for the + // resolver error-path tests (404 soft-miss vs 5xx hard error). + fn spawn_projects_stub_status(status_line: &'static str, body: &'static str) -> String { use std::io::{Read, Write}; let listener = TcpListener::bind("127.0.0.1:0").expect("bind stub"); let base = format!("http://127.0.0.1:{}", listener.local_addr().unwrap().port()); @@ -1492,7 +1526,8 @@ mod tests { } } let resp = format!( - "HTTP/1.1 200 OK\r\nContent-Length: {}\r\nContent-Type: application/json\r\nConnection: close\r\n\r\n{}", + "HTTP/1.1 {}\r\nContent-Length: {}\r\nContent-Type: application/json\r\nConnection: close\r\n\r\n{}", + status_line, body.len(), body ); @@ -1532,4 +1567,55 @@ mod tests { .unwrap() .is_none()); } + + #[test] + fn repo_url_matches_slug_enforces_path_boundary() { + // Exact repo (with/without scheme, .git, trailing slash) matches; a + // sibling/prefix repo or a different org must not (COR-1577 review). + assert!(repo_url_matches_slug( + "https://github.com/acme/api", + "acme/api" + )); + assert!(repo_url_matches_slug( + "https://github.com/acme/api.git", + "acme/api" + )); + assert!(repo_url_matches_slug("acme/api", "acme/api")); + assert!(!repo_url_matches_slug( + "https://github.com/acme/api-v2", + "acme/api" + )); + assert!(!repo_url_matches_slug( + "https://github.com/notacme/api", + "acme/api" + )); + } + + #[test] + fn resolve_project_by_repo_rejects_sibling_prefix_repo() { + // Backend `repo_url__icontains` returns the sibling `acme/api-v2` for + // slug `acme/api`; the boundary guard must reject it rather than + // confirming the wrong project (COR-1577 review). + let base = spawn_projects_stub( + r#"{"status":"ok","projects":[{"id":9,"name":"acme/api-v2","repo_url":"https://github.com/acme/api-v2"}]}"#, + ); + let got = resolve_project_by_repo(&base, "acme/api").unwrap(); + assert!(got.is_none(), "a prefix sibling repo must not be confirmed"); + } + + #[test] + fn resolve_project_by_repo_404_is_soft_none() { + // /projects absent on a very old backend -> soft miss (Ok(None)). + let base = spawn_projects_stub_status("404 Not Found", r#"{"message":"not found"}"#); + assert!(resolve_project_by_repo(&base, "org/repo") + .unwrap() + .is_none()); + } + + #[test] + fn resolve_project_by_repo_server_error_is_hard_err() { + // 5xx must surface, not silently fall back to the local-dir project. + let base = spawn_projects_stub_status("500 Internal Server Error", r#"{"error":"boom"}"#); + assert!(resolve_project_by_repo(&base, "org/repo").is_err()); + } } diff --git a/src/utils/generic.rs b/src/utils/generic.rs index 6386b62..8cd4ecf 100644 --- a/src/utils/generic.rs +++ b/src/utils/generic.rs @@ -343,6 +343,19 @@ pub fn get_repo_info(dir: &str) -> Result, git2::Error> { })) } +/// Find the enclosing repository's `origin` remote URL, searching upward from +/// the current directory so `corgea list`/`wait` resolve correctly when run +/// from a subdirectory, not only the repo root. `get_repo_info` uses +/// `Repository::open`, which succeeds only at the root; this uses +/// `Repository::discover`. Returns None outside a git repo or when `origin` +/// carries no URL. +pub fn discover_repo_url() -> Option { + let repo = Repository::discover(Path::new(".")).ok()?; + repo.find_remote("origin") + .ok() + .and_then(|remote| remote.url().map(|url| url.to_string())) +} + pub fn get_status(status: &str) -> &str { match status.to_lowercase().as_str() { "fix available" | "fix_available" => "Fix Available", diff --git a/src/wait.rs b/src/wait.rs index 6039548..ea34aa8 100644 --- a/src/wait.rs +++ b/src/wait.rs @@ -8,11 +8,24 @@ pub fn run( project_name_override: Option, repo_override: Option, ) { - let resolved = utils::api::resolve_project( + let resolved = match utils::api::resolve_project( &config.get_url(), project_name_override.as_deref(), repo_override.as_deref(), - ); + ) { + Ok(resolved) => resolved, + Err(e) => { + log::error!( + "Unable to resolve the Corgea project. Please check your connection and ensure that:\n\ + - The server URL is reachable.\n\ + - Your authentication token is valid.\n\n\ + Check out our docs at https://docs.corgea.app/install_cli#login-with-the-cli\n\n\ + Error details: {}", + e + ); + std::process::exit(1); + } + }; let project_name = resolved.query_name.clone(); let scans_result = diff --git a/tests/list_resolution.rs b/tests/list_resolution.rs index a33d005..ae7edb5 100644 --- a/tests/list_resolution.rs +++ b/tests/list_resolution.rs @@ -241,6 +241,93 @@ fn list_json_miss_is_valid_empty_envelope() { ); } +#[test] +fn list_issues_with_scan_id_skips_project_resolution() { + use std::sync::{Arc, Mutex}; + let hits = Arc::new(Mutex::new(Vec::::new())); + let hits_c = hits.clone(); + // The scan-id issue route ignores the project, so no /projects call should + // be made even from a real git repo where a remote IS present. (COR-1577) + let url = common::spawn_http_stub(move |path| { + hits_c.lock().unwrap().push(path.to_string()); + if path.starts_with("/api/v1/verify") { + ("200 OK", r#"{"status":"ok"}"#.to_string()) + } else if path.contains("/check_blocking_rules") { + ( + "200 OK", + r#"{"block":false,"blocking_issues":[],"total_pages":1}"#.to_string(), + ) + } else if path.starts_with("/api/v1/scan/") && path.contains("/issues") { + ( + "200 OK", + r#"{"status":"ok","page":1,"total_pages":1,"total_issues":0,"issues":[]}"# + .to_string(), + ) + } else { + ("404 Not Found", r#"{"message":"not found"}"#.to_string()) + } + }); + let (_tmp, repo) = temp_git_repo("dotnet-azure-web-tsb", REMOTE); + let out = run_list(&["--issues", "--scan-id", "scan-xyz"], &url, &repo); + assert_eq!( + out.status.code(), + Some(0), + "stderr: {}", + String::from_utf8_lossy(&out.stderr) + ); + let hits = hits.lock().unwrap(); + assert!( + !hits.iter().any(|h| h.starts_with("/api/v1/projects")), + "no /projects resolution for --issues --scan-id; hits: {hits:?}" + ); + assert!( + hits.iter().any(|h| h.contains("/scan/scan-xyz/issues")), + "expected the scan-id issue route; hits: {hits:?}" + ); +} + +#[test] +fn list_resolves_from_subdirectory() { + use std::sync::{Arc, Mutex}; + let hits = Arc::new(Mutex::new(Vec::::new())); + let hits_c = hits.clone(); + let url = common::spawn_http_stub(move |path| { + hits_c.lock().unwrap().push(path.to_string()); + if path.starts_with("/api/v1/verify") { + ("200 OK", r#"{"status":"ok"}"#.to_string()) + } else if path.starts_with("/api/v1/projects?repo_url=") { + ("200 OK", projects_match()) + } else if path.starts_with("/api/v1/scans?") { + ("200 OK", scans_one(CANON)) + } else { + ("404 Not Found", r#"{"message":"not found"}"#.to_string()) + } + }); + let (_tmp, repo) = temp_git_repo("dotnet-azure-web-tsb", REMOTE); + let subdir = repo.join("src"); + std::fs::create_dir(&subdir).expect("create subdir"); + let out = run_list(&[], &url, &subdir); + let stdout = String::from_utf8_lossy(&out.stdout); + assert_eq!( + out.status.code(), + Some(0), + "stderr: {}", + String::from_utf8_lossy(&out.stderr) + ); + // Discovery walks up from src/ to the repo root, so the remote slug — not + // the `src` basename — drives resolution: a /projects hit proves it. + let hits = hits.lock().unwrap(); + assert!( + hits.iter() + .any(|h| h.starts_with("/api/v1/projects?repo_url=bohappdev%2Fdotnet-azure-web-tsb")), + "expected /projects resolution from the subdir; hits: {hits:?}" + ); + assert!( + stdout.contains("bohappdev/dotnet-azure-web-tsb"), + "stdout: {stdout}" + ); +} + #[test] fn list_issues_json_miss_keeps_stdout_clean() { let url = spawn_stub(projects_empty(), scans_empty(), issues_miss()); From e96867e00535b0bb34c97b581bf011b0d04ff8ff Mon Sep 17 00:00:00 2001 From: Test Date: Tue, 28 Jul 2026 15:19:53 +0200 Subject: [PATCH 03/16] fix(cli): address PR #122 review (COR-1577) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Compare the whole post-host repo path, not a trailing org/repo slug: extract_repo_slug -> extract_repo_path, repo_url_matches_slug -> repo_url_matches_path. A nested mirror (…/mirrors/acme/api) or a differently-rooted path (…/org/team/repo) no longer matches; Azure `_git` and GitLab subgroup paths now compare in full. - Restore ScanUploadResult.project_id and thread it into wait::run, which skips resolution entirely when it has both a scan id and an upload project id, and only lists scans when no scan id was given. The semgrep/snyk post-scan wait makes zero /projects and /scans calls and links the id-form URL again. - Validate the project name before building the name-form scan URL, so a slash-only name can no longer produce /project//?scan_id=. - The unresolved-project listing miss exits 1 in both human and --json mode (JSON still prints its envelope first); a confirmed project with no scans stays exit 0. - Trim comment density to match the surrounding files. New tests: whole-path matching boundaries, wait resolving from a subdirectory, the post-upload wait issuing no /projects or /scans, trailing slash trimming, and the miss/empty exit codes. --- src/list.rs | 38 +++---- src/main.rs | 1 + src/scan.rs | 34 +++++-- src/utils/api.rs | 145 +++++++++++++------------- src/utils/generic.rs | 103 ++++++++++--------- src/wait.rs | 98 +++++++++++------- tests/list_resolution.rs | 46 +++++---- tests/wait_resolution.rs | 212 ++++++++++++++++++++++++++++++++++----- 8 files changed, 444 insertions(+), 233 deletions(-) diff --git a/src/list.rs b/src/list.rs index c7c952c..838a479 100644 --- a/src/list.rs +++ b/src/list.rs @@ -20,8 +20,7 @@ pub fn run( println!(); } if *sca_issues { - // SCA has no project parameter (get_sca_issues); keep the CWD basename - // for its legacy error copy and do NOT resolve here. + // SCA has no project parameter; the CWD basename is only error copy. let project_name = utils::generic::get_current_working_directory().unwrap_or("unknown".to_string()); let sca_issues_response = match utils::api::get_sca_issues( @@ -117,8 +116,7 @@ pub fn run( ); } else { // The --scan-id issue route hits /scan/{id}/issues and ignores the - // project, so skip the extra /projects resolution in that one mode; - // every other path here queries by project and needs it resolved. + // project, so it needs no resolution; every other path queries by name. let resolved: Option = if *issues && scan_id.is_some() { None } else { @@ -303,8 +301,6 @@ pub fn run( utils::terminal::print_table(table, issues_response.page, issues_response.total_pages); } else { - // Scan-listing always resolves (the skip only applies to - // --issues --scan-id), so `resolved` is present here. let resolved = resolved .as_ref() .expect("scan listing always resolves the project"); @@ -318,8 +314,8 @@ pub fn run( let page = scans.page; let total_pages = scans.total_pages; // The server already filtered by the resolved project; the old - // client-side `scan.project == cwd_basename` pass is redundant - // and would discard every repo-resolved scan. (COR-1577) + // client-side `scan.project == cwd_basename` pass would discard + // every repo-resolved scan. (COR-1577) (scans.scans.unwrap_or_default(), page, total_pages) } Err(e) => { @@ -339,7 +335,9 @@ pub fn run( std::process::exit(1); } }; - // JSON mode stays a valid machine envelope even when empty. + // An unresolved project is a miss (exit 1, as --issues and `wait`); + // a confirmed project with no scans is a valid empty result. + let unresolved_miss = scans.is_empty() && !resolved.confirmed; if *json { let output = json!({ "page": page, @@ -347,21 +345,27 @@ pub fn run( "results": scans }); println!("{}", serde_json::to_string_pretty(&output).unwrap()); + if unresolved_miss { + log::error!( + "No Corgea project found for {}. Run 'corgea scan' to create one, or pass --project-name .", + resolved.tried_label + ); + std::process::exit(1); + } return; } - // Human mode: never render a silent empty table on a miss. if scans.is_empty() { - if resolved.confirmed { - println!( - "Project '{}' has no scans yet. Run 'corgea scan' to create one.", - project_name - ); - } else { - println!( + if unresolved_miss { + log::error!( "No Corgea project found for {}. Run 'corgea scan' to create one, or pass --project-name .", resolved.tried_label ); + std::process::exit(1); } + println!( + "Project '{}' has no scans yet. Run 'corgea scan' to create one.", + project_name + ); return; } let mut table = vec![vec![ diff --git a/src/main.rs b/src/main.rs index ae6ad9e..5fb65f1 100644 --- a/src/main.rs +++ b/src/main.rs @@ -700,6 +700,7 @@ fn main() { scan_id.clone(), project_name.clone(), repo.clone(), + None, ); } Some(Commands::List { diff --git a/src/scan.rs b/src/scan.rs index cb6ec0b..2f7cd73 100644 --- a/src/scan.rs +++ b/src/scan.rs @@ -48,6 +48,7 @@ pub fn run_command(base_cmd: &String, mut command: Command) -> String { pub struct ScanUploadResult { pub scan_id: String, + pub project_id: Option, } pub fn run_semgrep(config: &Config, project_name: Option) { @@ -65,9 +66,13 @@ pub fn run_semgrep(config: &Config, project_name: Option) { let output = run_command(&base_command.to_string(), command); if let Some(result) = parse_scan(config, output, true, project_name.clone()) { - // Preserve an explicit --project-name for the post-scan wait; when None, - // wait resolves by the git remote / CWD (COR-1577). - crate::wait::run(config, Some(result.scan_id), project_name, None); + crate::wait::run( + config, + Some(result.scan_id), + project_name, + None, + result.project_id, + ); } } @@ -82,9 +87,13 @@ pub fn run_snyk(config: &Config, project_name: Option) { let output = run_command(&base_command.to_string(), command); if let Some(result) = parse_scan(config, output, true, project_name.clone()) { - // Preserve an explicit --project-name for the post-scan wait; when None, - // wait resolves by the git remote / CWD (COR-1577). - crate::wait::run(config, Some(result.scan_id), project_name, None); + crate::wait::run( + config, + Some(result.scan_id), + project_name, + None, + result.project_id, + ); } } @@ -372,6 +381,7 @@ pub fn upload_scan( }; let mut sast_scan_id: Option = None; + let mut project_id: Option = None; let mut upload_failed = false; @@ -398,6 +408,13 @@ pub fn upload_scan( sast_scan_id = Some(id_num.to_string()); } } + if let Some(pid_val) = json.get("project_id") { + if let Some(pid_str) = pid_val.as_str() { + project_id = Some(pid_str.to_string()); + } else if let Some(pid_num) = pid_val.as_i64() { + project_id = Some(pid_num.to_string()); + } + } } Err(e) => { log::warn!("Failed to parse response JSON: {}", e); @@ -509,5 +526,8 @@ pub fn upload_scan( println!("Go to {base_url} to see results."); - sast_scan_id.map(|scan_id| ScanUploadResult { scan_id }) + sast_scan_id.map(|scan_id| ScanUploadResult { + scan_id, + project_id, + }) } diff --git a/src/utils/api.rs b/src/utils/api.rs index f85e173..cb5703f 100644 --- a/src/utils/api.rs +++ b/src/utils/api.rs @@ -735,8 +735,7 @@ pub fn query_scan_list( #[derive(Deserialize, Debug)] pub struct ProjectSummary { - // Project.id is an integer in the envelope; model it tolerantly so an - // unexpected string id (or a missing field) never fails deserialization. + // Tolerant: an unexpected string id (or none) must not fail the parse. #[serde(default)] pub id: serde_json::Value, pub name: String, @@ -746,17 +745,14 @@ pub struct ProjectSummary { #[derive(Deserialize, Debug)] pub struct ProjectsResponse { - // Models the `@paginated` envelope status; deserialized for completeness but - // not consumed (the slug guard + non-2xx/parse handling already gate misses). + // Part of the `@paginated` envelope; not consumed. #[allow(dead_code)] pub status: String, #[serde(default)] pub projects: Option>, } -/// Stringify a scalar JSON id (number -> "123", string -> as-is) for the wait -/// scan URL. Returns None for null/array/object/missing so a bogus id never -/// becomes part of a URL like `/project/null/`. +/// Stringify a scalar JSON id; None otherwise, so no `/project/null/` URL. fn id_to_string(v: &serde_json::Value) -> Option { match v { serde_json::Value::String(s) => Some(s.clone()), @@ -765,53 +761,49 @@ fn id_to_string(v: &serde_json::Value) -> Option { } } -/// True when a stored `repo_url` refers to exactly `slug` (an `org/repo`-style -/// value, already lowercased). Matches on a path-segment boundary so a sibling -/// or prefix repo is never mistaken for the target: the backend's -/// `repo_url__icontains` filter returns `org/repo-v2` for slug `org/repo`, and a -/// plain substring check would wrongly accept it. Strips `.git`/trailing slash -/// first so `…/org/repo.git` and `…/org/repo/` both match `org/repo`. -fn repo_url_matches_slug(repo_url: &str, slug_lc: &str) -> bool { +/// True when a stored `repo_url` points at exactly `expected_path` (a whole +/// post-host path, already lowercased). Comparing whole paths keeps the +/// backend's `repo_url__icontains` results honest: neither the sibling +/// `acme/api-v2` nor the nested `…/mirrors/acme/api` passes for `acme/api`. +/// Falls back to a normalized compare for a stored bare `acme/api`. +fn repo_url_matches_path(repo_url: &str, expected_path: &str) -> bool { + if let Some(path) = utils::generic::extract_repo_path(repo_url) { + return path == expected_path; + } let r = repo_url.trim().trim_end_matches('/'); let r = r.strip_suffix(".git").unwrap_or(r).to_lowercase(); - r == slug_lc || r.ends_with(&format!("/{}", slug_lc)) + r == expected_path } -/// Resolve the canonical project for a repo slug via GET /api/v1/projects?repo_url=… +/// Resolve the canonical project for a repo path via GET /api/v1/projects?repo_url=… /// /// Old-backend safety guard: a pre-COR-1426 backend ignores the unknown -/// `repo_url` param and returns ALL company projects. Keep only candidates -/// whose returned `repo_url` matches the slug on a path-segment boundary; on an -/// old backend none match -> Ok(None) -> caller falls back to the CWD-name path. +/// `repo_url` param and returns ALL company projects, so every candidate is +/// re-checked against the path here — on such a backend none match, and the +/// caller falls back to the CWD-name path. /// -/// Hard failures (network, auth, 5xx) return `Err` so the caller can surface the -/// backend problem instead of silently resolving to the local-directory project; -/// only a clean "no match" (or a 404 from a backend without the endpoint) is a -/// soft `Ok(None)`. +/// `Err` for hard failures (network/auth/5xx); a clean "no match" (or a 404 from +/// a backend without the endpoint) is a soft `Ok(None)`. pub fn resolve_project_by_repo( url: &str, - slug: &str, + repo_path: &str, ) -> Result, Box> { let request_url = format!("{}{}/projects", url, API_BASE); let client = http_client(); debug(&format!( "Resolving project via {} (repo_url={})", - request_url, slug + request_url, repo_path )); let response = client .get(&request_url) - .query(&[("repo_url", slug)]) + .query(&[("repo_url", repo_path)]) .send()?; check_for_warnings(response.headers(), response.status()); let status = response.status(); if status == reqwest::StatusCode::NOT_FOUND { - // /projects absent on a very old backend -> soft miss; the caller falls - // back to the name-based path, so this stays a no-regression case. return Ok(None); } if !status.is_success() { - // Auth (401/403) or server (5xx) failure: surface it rather than - // silently resolving to the local-directory project. return Err(format!("/projects request failed: HTTP {}", status).into()); } let text = response.text()?; @@ -825,11 +817,11 @@ pub fn resolve_project_by_repo( return Ok(None); } }; - let slug_lc = slug.to_lowercase(); + let expected = repo_path.to_lowercase(); let matched = parsed.projects.unwrap_or_default().into_iter().find(|p| { p.repo_url .as_deref() - .map(|r| repo_url_matches_slug(r, &slug_lc)) + .map(|r| repo_url_matches_path(r, &expected)) .unwrap_or(false) }); Ok(matched) @@ -840,27 +832,18 @@ pub fn resolve_project_by_repo( pub struct ResolvedProject { /// Sent as `?project=` to the listing endpoints. pub query_name: String, - /// True only when a backend project was confirmed via /projects. Drives - /// confirmed-but-empty vs unresolved-miss messaging. + /// True only when /projects confirmed a backend project. pub confirmed: bool, - /// Numeric id (stringified) when confirmed and the id is a scalar — used - /// for the wait scan URL. None otherwise (falls back to query_name). pub project_id: Option, - /// What we looked for ("project 'x'" / "repo 'org/repo'" / "directory - /// 'dir'"), pre-formatted for the miss message. + /// Pre-formatted subject of the miss message ("repo 'org/repo'", …). pub tried_label: String, } -/// Resolve which project `list`/`wait` should query. -/// 1) --project-name -> use verbatim, skip resolution. -/// 2) else slug from --repo or the discovered git remote -> resolve_project_by_repo: -/// confirmed -> canonical {name,id}. -/// 3) unconfirmed: explicit --repo -> query the slug as a name (never the CWD); -/// auto-detected remote -> CWD basename (no-regression fallback). -/// 4) no remote -> CWD basename (today's behavior). -/// -/// Returns `Err` only for a hard resolver failure (network/auth/5xx from -/// /projects); a clean "no match" resolves to the fallback name above. +/// Resolve which project `list`/`wait` should query: `--project-name` verbatim, +/// else the repo path from `--repo` or the discovered remote. Unconfirmed, an +/// explicit `--repo` queries that path as a name and everything else falls back +/// to the CWD basename (today's behavior). `Err` only for a hard resolver +/// failure (network/auth/5xx from /projects). pub fn resolve_project( url: &str, project_name_override: Option<&str>, @@ -877,35 +860,29 @@ pub fn resolve_project( // Explicit --repo vs auto-detected remote changes the unconfirmed fallback. let repo_was_explicit = repo_override.is_some(); - // --repo may already be a bare `org/repo` slug (extract_repo_slug needs a - // host segment, so it returns None for a 2-segment value) -> use raw value. - // No --repo: discover the enclosing repo's remote (works from a subdir). - let slug = match repo_override { - Some(r) => utils::generic::extract_repo_slug(r).or_else(|| Some(r.to_string())), + // --repo may already be a bare `org/repo` (extract_repo_path needs a host + // segment, so it returns None there) -> use the raw value. + let repo_path = match repo_override { + Some(r) => utils::generic::extract_repo_path(r).or_else(|| Some(r.to_string())), None => { - utils::generic::discover_repo_url().and_then(|u| utils::generic::extract_repo_slug(&u)) + utils::generic::discover_repo_url().and_then(|u| utils::generic::extract_repo_path(&u)) } }; - // CWD basename, resolved lazily — only the fallback paths below read it. let cwd = || utils::generic::get_current_working_directory().unwrap_or_else(|| "unknown".to_string()); - if let Some(slug) = slug { - // `?` propagates hard resolver failures (network/auth/5xx); only a clean - // Ok(None) "no match" falls through to the name-based fallback below. - if let Some(project) = resolve_project_by_repo(url, &slug)? { + if let Some(repo_path) = repo_path { + if let Some(project) = resolve_project_by_repo(url, &repo_path)? { return Ok(ResolvedProject { query_name: project.name, confirmed: true, project_id: id_to_string(&project.id), - tried_label: format!("repo '{}'", slug), + tried_label: format!("repo '{}'", repo_path), }); } - // Unconfirmed: explicit --repo queries the slug as a name (clean miss, - // never wrong CWD results); auto-detected remote keeps the CWD fallback. let query_name = if repo_was_explicit { - slug.clone() + repo_path.clone() } else { cwd() }; @@ -913,7 +890,7 @@ pub fn resolve_project( query_name, confirmed: false, project_id: None, - tried_label: format!("repo '{}'", slug), + tried_label: format!("repo '{}'", repo_path), }); } @@ -1606,33 +1583,53 @@ mod tests { } #[test] - fn repo_url_matches_slug_enforces_path_boundary() { - // Exact repo (with/without scheme, .git, trailing slash) matches; a - // sibling/prefix repo or a different org must not (COR-1577 review). - assert!(repo_url_matches_slug( + fn repo_url_matches_path_compares_the_whole_path() { + // Same repo across scheme/.git/trailing-slash/port variants. + assert!(repo_url_matches_path( "https://github.com/acme/api", "acme/api" )); - assert!(repo_url_matches_slug( + assert!(repo_url_matches_path( "https://github.com/acme/api.git", "acme/api" )); - assert!(repo_url_matches_slug("acme/api", "acme/api")); - assert!(!repo_url_matches_slug( + assert!(repo_url_matches_path("git@github.com:acme/api", "acme/api")); + assert!(repo_url_matches_path("acme/api", "acme/api")); + // Sibling / prefix repo, and a different org. + assert!(!repo_url_matches_path( "https://github.com/acme/api-v2", "acme/api" )); - assert!(!repo_url_matches_slug( + assert!(!repo_url_matches_path( "https://github.com/notacme/api", "acme/api" )); + // The owner must be top-level on the host: a nested mirror is a + // different repository, as is `org/team/repo` for `team/repo`. + assert!(!repo_url_matches_path( + "https://github.com/mirrors/acme/api", + "acme/api" + )); + assert!(!repo_url_matches_path( + "https://github.com/org/team/repo", + "team/repo" + )); + // Multi-segment paths compare in full. + assert!(repo_url_matches_path( + "https://dev.azure.com/org/project/_git/repo", + "org/project/_git/repo" + )); + assert!(repo_url_matches_path( + "git@gitlab.com:group/subgroup/repo.git", + "group/subgroup/repo" + )); } #[test] fn resolve_project_by_repo_rejects_sibling_prefix_repo() { // Backend `repo_url__icontains` returns the sibling `acme/api-v2` for - // slug `acme/api`; the boundary guard must reject it rather than - // confirming the wrong project (COR-1577 review). + // `acme/api`; the path guard must reject it rather than confirming the + // wrong project (COR-1577 review). let base = spawn_projects_stub( r#"{"status":"ok","projects":[{"id":9,"name":"acme/api-v2","repo_url":"https://github.com/acme/api-v2"}]}"#, ); diff --git a/src/utils/generic.rs b/src/utils/generic.rs index 68534ec..faf9eb2 100644 --- a/src/utils/generic.rs +++ b/src/utils/generic.rs @@ -260,46 +260,33 @@ fn extract_repo_name_from_url(url: &str) -> Option { None } -/// Extract the `org/repo` slug from a git remote URL. Returns the last two -/// meaningful path segments so it is a substring of essentially every stored -/// (normalized) `repo_url` form. Distinct from `extract_repo_name_from_url`, -/// which returns only the final segment (`repo`). -/// -/// Handles: -/// https://github.com/org/repo(.git) -> org/repo -/// git@github.com:org/repo(.git) -> org/repo -/// ssh://git@github.com/org/repo -> org/repo -/// https://dev.azure.com/org/project/_git/repo -> project/_git/repo -/// -/// Azure `_git` is kept (and the preceding project segment included) because -/// doghouse `normalize_repo_url` stores Azure as `.../project/_git/repo` -/// (`heeler/models.py:208-212`) — `project/_git/repo` is a substring of that, -/// `project/repo` is NOT. Azure SSH remotes (`ssh.dev.azure.com/v3/...`, which -/// carry no `_git` segment) are a known limitation; users pass --project-name. -/// -/// Returns None when fewer than two path segments follow the host (a bare host -/// or garbage input). -pub fn extract_repo_slug(url: &str) -> Option { +/// The whole repository path after the host, lowercased (`org/repo`, +/// `group/subgroup/repo`, Azure `org/project/_git/repo`). The host is excluded +/// so SSH/HTTPS/port variants of one remote compare equal; None when fewer than +/// two path segments follow it. The full path — not a trailing `org/repo` slug +/// — is what doghouse `normalize_repo_url` stores (`heeler/models.py:201-246`), +/// so it is what an equality compare needs. Azure SSH remotes +/// (`ssh.dev.azure.com/v3/org/…`, no `_git` segment) remain a known limitation. +pub fn extract_repo_path(url: &str) -> Option { let url = url.trim().trim_end_matches('/'); let url = url.strip_suffix(".git").unwrap_or(url); - // Drop scheme (`https://`, `ssh://`, …) if present. let url = url.rsplit("://").next().unwrap_or(url); - // Split host from path: URL forms use '/', scp-like `git@host:org/repo` - // uses ':'. After filtering empties, segments[0] is the host. - let segments: Vec<&str> = url.split(['/', ':']).filter(|s| !s.is_empty()).collect(); + // Drop userinfo (`git@`, `oauth2:token@`) so it is never read as the host. + let host_end = url.find('/').unwrap_or(url.len()); + let url = match url[..host_end].rfind('@') { + Some(at) => &url[at + 1..], + None => url, + }; + // URL forms split host from path on '/', scp-like `git@host:org/repo` on ':'. + let mut segments: Vec<&str> = url.split(['/', ':']).filter(|s| !s.is_empty()).collect(); + // segments[0] is the host; an all-digit segment right after it is a port. + if segments.len() >= 4 && segments[1].chars().all(|c| c.is_ascii_digit()) { + segments.remove(1); + } if segments.len() < 3 { return None; // need host + at least org + repo } - let last = segments[segments.len() - 1]; - let prev = segments[segments.len() - 2]; - if prev == "_git" && segments.len() >= 4 { - // Azure DevOps: keep the project so the slug stays a substring of the - // normalized stored URL (.../project/_git/repo). - let project = segments[segments.len() - 3]; - Some(format!("{}/_git/{}", project, last)) - } else { - Some(format!("{}/{}", prev, last)) - } + Some(segments[1..].join("/").to_lowercase()) } pub fn get_env_var_if_exists(var_name: &str) -> Option { @@ -505,53 +492,65 @@ mod tests { } #[test] - fn extract_repo_slug_handles_common_remote_forms() { + fn extract_repo_path_handles_common_remote_forms() { assert_eq!( - extract_repo_slug("https://github.com/org/repo.git").as_deref(), + extract_repo_path("https://github.com/org/repo.git").as_deref(), Some("org/repo") ); assert_eq!( - extract_repo_slug("https://github.com/org/repo").as_deref(), + extract_repo_path("https://github.com/org/repo").as_deref(), Some("org/repo") ); assert_eq!( - extract_repo_slug("git@github.com:org/repo.git").as_deref(), + extract_repo_path("git@github.com:org/repo.git").as_deref(), Some("org/repo") ); assert_eq!( - extract_repo_slug("git@github.com:org/repo").as_deref(), + extract_repo_path("git@github.com:org/repo").as_deref(), Some("org/repo") ); assert_eq!( - extract_repo_slug("ssh://git@github.com/org/repo").as_deref(), + extract_repo_path("ssh://git@github.com/org/repo").as_deref(), Some("org/repo") ); assert_eq!( - extract_repo_slug("https://github.com/org/repo/").as_deref(), + extract_repo_path("https://github.com/org/repo/").as_deref(), Some("org/repo") ); - // host:port should not leak the port into the slug assert_eq!( - extract_repo_slug("https://git.example.com:8443/org/repo").as_deref(), + extract_repo_path("https://github.com/Org/Repo").as_deref(), + Some("org/repo") + ); + // host:port must not leak the port into the path + assert_eq!( + extract_repo_path("https://git.example.com:8443/org/repo").as_deref(), + Some("org/repo") + ); + // token userinfo must not be read as the host + assert_eq!( + extract_repo_path("https://oauth2:tok@gitlab.com/org/repo").as_deref(), Some("org/repo") ); // Bank of Hope case assert_eq!( - extract_repo_slug("git@github.com:bohappdev/dotnet-azure-web-tsb.git").as_deref(), + extract_repo_path("git@github.com:bohappdev/dotnet-azure-web-tsb.git").as_deref(), Some("bohappdev/dotnet-azure-web-tsb") ); - // Azure DevOps `_git` HTTPS -> keeps project + _git so it stays a substring - // of the normalized stored URL. + // Whole path is kept: Azure `_git` and GitLab subgroups + assert_eq!( + extract_repo_path("https://dev.azure.com/org/project/_git/repo").as_deref(), + Some("org/project/_git/repo") + ); assert_eq!( - extract_repo_slug("https://dev.azure.com/org/project/_git/repo").as_deref(), - Some("project/_git/repo") + extract_repo_path("git@gitlab.com:group/subgroup/repo.git").as_deref(), + Some("group/subgroup/repo") ); } #[test] - fn extract_repo_slug_returns_none_for_unsplittable_input() { - assert_eq!(extract_repo_slug("not a url"), None); - assert_eq!(extract_repo_slug(""), None); - assert_eq!(extract_repo_slug("github.com"), None); // host only + fn extract_repo_path_returns_none_for_unsplittable_input() { + assert_eq!(extract_repo_path("not a url"), None); + assert_eq!(extract_repo_path(""), None); + assert_eq!(extract_repo_path("github.com"), None); // host only } } diff --git a/src/wait.rs b/src/wait.rs index ea34aa8..cb3bbef 100644 --- a/src/wait.rs +++ b/src/wait.rs @@ -7,43 +7,61 @@ pub fn run( scan_id: Option, project_name_override: Option, repo_override: Option, + project_id_override: Option, ) { - let resolved = match utils::api::resolve_project( - &config.get_url(), - project_name_override.as_deref(), - repo_override.as_deref(), - ) { - Ok(resolved) => resolved, - Err(e) => { - log::error!( - "Unable to resolve the Corgea project. Please check your connection and ensure that:\n\ - - The server URL is reachable.\n\ - - Your authentication token is valid.\n\n\ - Check out our docs at https://docs.corgea.app/install_cli#login-with-the-cli\n\n\ - Error details: {}", - e - ); - std::process::exit(1); + // A scan id plus the project id from the upload response leaves nothing to + // resolve: everything below keys off the scan, and the id-form URL is + // already known. + let resolved: Option = if scan_id.is_some() + && project_id_override.is_some() + { + None + } else { + match utils::api::resolve_project( + &config.get_url(), + project_name_override.as_deref(), + repo_override.as_deref(), + ) { + Ok(resolved) => Some(resolved), + Err(e) => { + log::error!( + "Unable to resolve the Corgea project. Please check your connection and ensure that:\n\ + - The server URL is reachable.\n\ + - Your authentication token is valid.\n\n\ + Check out our docs at https://docs.corgea.app/install_cli#login-with-the-cli\n\n\ + Error details: {}", + e + ); + std::process::exit(1); + } } }; - let project_name = resolved.query_name.clone(); + let project_name = match &resolved { + Some(r) => r.query_name.clone(), + None => project_name_override + .clone() + .unwrap_or_else(|| utils::generic::get_current_working_directory().unwrap_or_default()), + }; - let scans_result = - utils::api::query_scan_list(&config.get_url(), Some(&project_name), Some(1), None); - let scans: Vec = match scans_result { - Ok(result) => result.scans.unwrap_or_default(), - Err(e) => { - log::error!( - "Unable to query the scan list. Please check your connection and ensure that: + // Only the scan-less path reads the listing. + let scans: Vec = if scan_id.is_some() { + Vec::new() + } else { + match utils::api::query_scan_list(&config.get_url(), Some(&project_name), Some(1), None) { + Ok(result) => result.scans.unwrap_or_default(), + Err(e) => { + log::error!( + "Unable to query the scan list. Please check your connection and ensure that: - The server URL is reachable. - Your authentication token is valid. Check out our docs at https://docs.corgea.app/install_cli#login-with-the-cli Error details: {}", - e - ); - std::process::exit(1); + e + ); + std::process::exit(1); + } } }; let (scan_id, processed) = match scan_id { @@ -62,7 +80,7 @@ pub fn run( None => match scans.first() { Some(scan) => (scan.id.clone(), scan.status == "Complete"), None => { - if resolved.confirmed { + if resolved.as_ref().map(|r| r.confirmed).unwrap_or(false) { log::error!( "Project '{}' has no scans yet. Run 'corgea scan' to start one.", project_name @@ -70,7 +88,7 @@ pub fn run( } else { log::error!( "No scan to wait for: no Corgea project found for {}. Run 'corgea scan', or pass --scan-id / --project-name.", - resolved.tried_label + resolved.as_ref().map(|r| r.tried_label.as_str()).unwrap_or_default() ); } std::process::exit(1); @@ -78,14 +96,22 @@ pub fn run( }, }; - let scan_url = match &resolved.project_id { + let project_id = + project_id_override.or_else(|| resolved.as_ref().and_then(|r| r.project_id.clone())); + let scan_url = match &project_id { Some(pid) => format!("{}/project/{}/?scan_id={}", config.get_url(), pid, scan_id), - None => format!( - "{}/project/{}?scan_id={}", - config.get_url(), - project_name, - scan_id - ), + None => { + // Without an id the name goes into the path; an empty or slash-only + // name would yield `/project//?scan_id=…`, which resolves nowhere. + let name = project_name.trim().trim_matches('/'); + if name.is_empty() { + log::error!( + "Cannot build the scan URL: no Corgea project resolved. Pass --project-name ." + ); + std::process::exit(1); + } + format!("{}/project/{}?scan_id={}", config.get_url(), name, scan_id) + } }; if !processed { diff --git a/tests/list_resolution.rs b/tests/list_resolution.rs index ae7edb5..7f1cb33 100644 --- a/tests/list_resolution.rs +++ b/tests/list_resolution.rs @@ -1,16 +1,6 @@ //! End-to-end tests for `corgea list` repo-URL project resolution (COR-1577). -//! -//! Each test runs the real binary against a one-response-per-connection HTTP -//! stub (`common::spawn_http_stub`) and, where a git remote matters, a temp -//! git repo built with `git2` whose directory basename DIFFERS from the stored -//! canonical project name (the Bank of Hope case: dir `dotnet-azure-web-tsb` -//! vs project `bohappdev/dotnet-azure-web-tsb`). -//! -//! Routing matches on the request-target PATH PREFIX with `starts_with`: the -//! `/projects` request carries a percent-encoded query string -//! (`?repo_url=bohappdev%2Fdotnet-azure-web-tsb`), so the full target is not a -//! stable key. `verify_token_and_exit_when_fail` calls `GET /api/v1/verify` -//! first, so every stub serves it. +//! Stubs route on the request-target path PREFIX: `/projects` carries a +//! percent-encoded query, so the full target is not a stable key. mod common; @@ -149,15 +139,11 @@ fn list_miss_names_repo_no_empty_table() { let (_tmp, repo) = temp_git_repo("dotnet-azure-web-tsb", REMOTE); let out = run_list(&[], &url, &repo); let stdout = String::from_utf8_lossy(&out.stdout); - assert_eq!( - out.status.code(), - Some(0), - "stderr: {}", - String::from_utf8_lossy(&out.stderr) - ); + let stderr = String::from_utf8_lossy(&out.stderr); + assert_eq!(out.status.code(), Some(1), "stderr: {stderr}"); assert!( - stdout.contains("repo 'bohappdev/dotnet-azure-web-tsb'"), - "stdout: {stdout}" + stderr.contains("repo 'bohappdev/dotnet-azure-web-tsb'"), + "stderr: {stderr}" ); assert!( !stdout.contains("Scan ID"), @@ -165,6 +151,23 @@ fn list_miss_names_repo_no_empty_table() { ); } +#[test] +fn list_confirmed_project_with_no_scans_exits_zero() { + // A confirmed project that simply has no scans is a valid empty result — + // failing it would break CI polling. + let url = spawn_stub(projects_match(), scans_empty(), issues_miss()); + let (_tmp, repo) = temp_git_repo("dotnet-azure-web-tsb", REMOTE); + let out = run_list(&[], &url, &repo); + let stdout = String::from_utf8_lossy(&out.stdout); + assert_eq!( + out.status.code(), + Some(0), + "stderr: {}", + String::from_utf8_lossy(&out.stderr) + ); + assert!(stdout.contains("has no scans yet"), "stdout: {stdout}"); +} + #[test] fn list_no_remote_falls_back_to_cwd_name() { // Regression: non-git dir whose basename matches a project the scans stub @@ -222,9 +225,10 @@ fn list_json_miss_is_valid_empty_envelope() { let (_tmp, repo) = temp_git_repo("dotnet-azure-web-tsb", REMOTE); let out = run_list(&["--json"], &url, &repo); let stdout = String::from_utf8_lossy(&out.stdout); + // Envelope on stdout, miss on stderr, exit 1. assert_eq!( out.status.code(), - Some(0), + Some(1), "stderr: {}", String::from_utf8_lossy(&out.stderr) ); diff --git a/tests/wait_resolution.rs b/tests/wait_resolution.rs index 28c01b3..e78c2f3 100644 --- a/tests/wait_resolution.rs +++ b/tests/wait_resolution.rs @@ -1,19 +1,7 @@ //! End-to-end tests for `corgea wait` repo-URL project resolution (COR-1577). -//! -//! Mirrors `tests/list_resolution.rs`: each test runs the real binary against a -//! one-response-per-connection HTTP stub (`common::spawn_http_stub`) and, where -//! a git remote matters, a temp git repo built with `git2` whose directory -//! basename DIFFERS from the stored canonical project name (the Bank of Hope -//! case: dir `dotnet-azure-web-tsb` vs project `bohappdev/dotnet-azure-web-tsb`). -//! -//! Routing matches on the request-target PATH PREFIX with `starts_with`: the -//! `/projects` request carries a percent-encoded query string -//! (`?repo_url=bohappdev%2Fdotnet-azure-web-tsb`), so the full target is not a -//! stable key. `verify_token_and_exit_when_fail` calls `GET /api/v1/verify` -//! first, so every stub serves it. The `/api/v1/scan/` arm serves BOTH -//! `GET /api/v1/scan/{id}` (so `check_scan_status` succeeds) and -//! `/api/v1/scan/{id}/issues?…` (so `report_scan_status` succeeds) — branching -//! on `contains("/issues")`. +//! Stubs route on the request-target path PREFIX. The `/api/v1/scan/` arm +//! serves both `GET /scan/{id}` (`check_scan_status`) and `/scan/{id}/issues` +//! (`report_scan_status`), branching on `contains("/issues")`. mod common; @@ -26,15 +14,14 @@ use std::process::Output; // --- stub bodies ----------------------------------------------------------- -/// `GET /api/v1/scan/{id}` returning a single completed scan (consumed by -/// `check_scan_status`/`get_scan`, which check the lowercase `complete`). +/// `GET /api/v1/scan/{id}` returning a completed scan (`check_scan_status` +/// checks the lowercase `complete`). fn scan_complete() -> String { r#"{"id":"scan-123","project":"bohappdev/dotnet-azure-web-tsb","repo":"https://github.com/bohappdev/dotnet-azure-web-tsb","branch":"main","status":"complete","engine":"blast","created_at":"2026-01-01T00:00:00Z"}"#.to_string() } -/// `GET /api/v1/scan/{id}/issues` returning an empty page (one round-trip: -/// `total_pages` 1). `report_scan_status` groups by urgency and succeeds with -/// no issues — enough to print the result link. +/// `GET /api/v1/scan/{id}/issues` returning an empty page — enough for +/// `report_scan_status` to succeed and print the result link. fn scan_issues_empty() -> String { r#"{"status":"ok","page":1,"total_pages":1,"total_issues":0,"issues":[]}"#.to_string() } @@ -90,8 +77,7 @@ fn wait_uses_canonical_project_and_numeric_id_scan_url() { "stderr: {}", String::from_utf8_lossy(&out.stderr) ); - // The scan URL uses the numeric project id (7) returned by /projects, - // proving `resolved.project_id` flows through — not the dir basename. + // Numeric project id (7) from /projects, not the dir basename. assert!( stdout.contains("/project/7/?scan_id=scan-123"), "stdout: {stdout}" @@ -156,12 +142,10 @@ fn wait_miss_names_repo_no_bare_error() { Some(1), "stdout: {stdout}\nstderr: {stderr}" ); - // Clear, actionable miss naming the repo the resolver tried. assert!( stderr.contains("repo 'bohappdev/dotnet-azure-web-tsb'"), "stderr should name the repo; stderr: {stderr}" ); - // The bare cryptic error is gone for good. assert!( !stdout.contains("Error querying scan list") && !stderr.contains("Error querying scan list"), @@ -181,14 +165,190 @@ fn wait_project_name_override() { "stderr: {}", String::from_utf8_lossy(&out.stderr) ); - // Override skips resolution: no confirmed project id, so the scan URL falls - // back to the name form keyed on the exact `--project-name` value. + // No confirmed id, so the URL keeps the exact `--project-name` value. assert!( stdout.contains("/project/some/name?scan_id=scan-123"), "stdout: {stdout}" ); } +#[test] +fn wait_project_name_trailing_slash_is_trimmed() { + // A slash-only or trailing-slash name would build `/project/foo//?scan_id=`. + let url = spawn_stub(projects_empty(), scans_one("foo")); + let (_tmp, dir) = temp_plain_dir("whatever"); + let out = run_wait(&["--project-name", "foo/"], &url, &dir); + let stdout = String::from_utf8_lossy(&out.stdout); + assert_eq!( + out.status.code(), + Some(0), + "stderr: {}", + String::from_utf8_lossy(&out.stderr) + ); + assert!( + stdout.contains("/project/foo?scan_id=scan-123"), + "stdout: {stdout}" + ); +} + +#[test] +fn wait_resolves_from_subdirectory() { + use std::sync::{Arc, Mutex}; + let hits = Arc::new(Mutex::new(Vec::::new())); + let hits_c = hits.clone(); + let url = common::spawn_http_stub(move |path| { + hits_c.lock().unwrap().push(path.to_string()); + if path.starts_with("/api/v1/verify") { + ("200 OK", r#"{"status":"ok"}"#.to_string()) + } else if path.starts_with("/api/v1/projects?repo_url=") { + ("200 OK", projects_match()) + } else if path.starts_with("/api/v1/scans?") { + ("200 OK", scans_one(CANON)) + } else if path.starts_with("/api/v1/scan/") { + if path.contains("/issues") { + ("200 OK", scan_issues_empty()) + } else { + ("200 OK", scan_complete()) + } + } else { + ("404 Not Found", r#"{"message":"not found"}"#.to_string()) + } + }); + let (_tmp, repo) = temp_git_repo("dotnet-azure-web-tsb", REMOTE); + let subdir = repo.join("src"); + std::fs::create_dir(&subdir).expect("create subdir"); + let out = run_wait(&[], &url, &subdir); + let stdout = String::from_utf8_lossy(&out.stdout); + assert_eq!( + out.status.code(), + Some(0), + "stderr: {}", + String::from_utf8_lossy(&out.stderr) + ); + // Discovery walks up from src/, so the remote — not the `src` basename — + // drives resolution. + let hits = hits.lock().unwrap(); + assert!( + hits.iter() + .any(|h| h.starts_with("/api/v1/projects?repo_url=bohappdev%2Fdotnet-azure-web-tsb")), + "expected /projects resolution from the subdir; hits: {hits:?}" + ); + assert!( + stdout.contains("/project/7/?scan_id=scan-123"), + "stdout: {stdout}" + ); +} + +#[test] +fn wait_with_scan_id_does_not_list_scans() { + use std::sync::{Arc, Mutex}; + let hits = Arc::new(Mutex::new(Vec::::new())); + let hits_c = hits.clone(); + let url = common::spawn_http_stub(move |path| { + hits_c.lock().unwrap().push(path.to_string()); + if path.starts_with("/api/v1/verify") { + ("200 OK", r#"{"status":"ok"}"#.to_string()) + } else if path.starts_with("/api/v1/projects?repo_url=") { + ("200 OK", projects_match()) + } else if path.starts_with("/api/v1/scan/") { + if path.contains("/issues") { + ("200 OK", scan_issues_empty()) + } else { + ("200 OK", scan_complete()) + } + } else { + ("404 Not Found", r#"{"message":"not found"}"#.to_string()) + } + }); + let (_tmp, repo) = temp_git_repo("dotnet-azure-web-tsb", REMOTE); + let out = run_wait(&["scan-123"], &url, &repo); + assert_eq!( + out.status.code(), + Some(0), + "stderr: {}", + String::from_utf8_lossy(&out.stderr) + ); + // The listing is only read when no scan id was given. + let hits = hits.lock().unwrap(); + assert!( + !hits.iter().any(|h| h.starts_with("/api/v1/scans")), + "no scan listing with --scan-id; hits: {hits:?}" + ); +} + +/// `corgea scan semgrep` with a fake `semgrep` on PATH: the post-scan wait gets +/// the project id straight from the upload response, so it must resolve nothing +/// — no `/projects`, no `/scans` — and still link the id-form URL. +#[cfg(unix)] +#[test] +fn scan_post_wait_uses_upload_project_id_without_resolving() { + use std::sync::{Arc, Mutex}; + let hits = Arc::new(Mutex::new(Vec::::new())); + let hits_c = hits.clone(); + let url = common::spawn_http_stub(move |path| { + hits_c.lock().unwrap().push(path.to_string()); + if path.starts_with("/api/v1/verify") + || path.starts_with("/api/v1/code-upload") + || path.starts_with("/api/v1/git-config-upload") + { + ("200 OK", r#"{"status":"ok"}"#.to_string()) + } else if path.starts_with("/api/v1/scan-upload") { + ( + "200 OK", + r#"{"status":"ok","sast_scan_id":"scan-123","project_id":42}"#.to_string(), + ) + } else if path.starts_with("/api/v1/scan/") { + if path.contains("/issues") { + ("200 OK", scan_issues_empty()) + } else { + ("200 OK", scan_complete()) + } + } else { + ("404 Not Found", r#"{"message":"not found"}"#.to_string()) + } + }); + let (_tmp, repo) = temp_git_repo("dotnet-azure-web-tsb", REMOTE); + std::fs::write(repo.join("app.py"), "x = 1\n").expect("write source"); + let bin = repo.join("fakebin"); + std::fs::create_dir(&bin).expect("create fake bin dir"); + let report = r#"{"version":"1.0.0","errors":[],"results":[{"check_id":"rule","path":"app.py","start":{"line":1},"end":{"line":1},"extra":{"message":"m","severity":"ERROR","metadata":{"source":"https://semgrep.dev/r/rule"}}}]}"#; + common::write_script( + &bin, + "semgrep", + &format!("#!/bin/sh\nprintf '%s' '{report}'\n"), + ); + + let (mut cmd, _home) = common::corgea_isolated(); + let out = cmd + .args(["scan", "semgrep"]) + .env("PATH", &bin) + .env("CORGEA_URL", &url) + .env("CORGEA_TOKEN", "test-token") + .current_dir(&repo) + .output() + .expect("spawn corgea"); + let stdout = String::from_utf8_lossy(&out.stdout); + assert_eq!( + out.status.code(), + Some(0), + "stdout: {stdout}\nstderr: {}", + String::from_utf8_lossy(&out.stderr) + ); + assert!( + stdout.contains("/project/42/?scan_id=scan-123"), + "expected the id-form scan URL; stdout: {stdout}" + ); + let hits = hits.lock().unwrap(); + assert!( + !hits.iter().any(|h| h.starts_with("/api/v1/projects")), + "no /projects resolution after an upload that carried the id; hits: {hits:?}" + ); + assert!( + !hits.iter().any(|h| h.starts_with("/api/v1/scans")), + "no scan listing after an upload that carried the id; hits: {hits:?}" + ); +} + #[test] fn wait_project_name_and_repo_are_mutually_exclusive() { let (_tmp, dir) = temp_plain_dir("whatever"); From fb7774a0781c894515203ddd001f83bf27dd14dd Mon Sep 17 00:00:00 2001 From: Test Date: Tue, 28 Jul 2026 15:31:55 +0200 Subject: [PATCH 04/16] test(cli): pin the post-scan wait test to the non-CI upload path running_in_ci() is true under GitHub Actions, which sent upload_scan down the GITHUB_REPOSITORY branch and panicked. The test covers the wait phase, so remove CI/GITHUB_ACTIONS from its env and exercise one path everywhere. --- tests/wait_resolution.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/wait_resolution.rs b/tests/wait_resolution.rs index e78c2f3..243f577 100644 --- a/tests/wait_resolution.rs +++ b/tests/wait_resolution.rs @@ -324,6 +324,10 @@ fn scan_post_wait_uses_upload_project_id_without_resolving() { .env("PATH", &bin) .env("CORGEA_URL", &url) .env("CORGEA_TOKEN", "test-token") + // `running_in_ci()` is true under Actions, which sends `upload_scan` + // down the GITHUB_REPOSITORY branch. Pin the non-CI path either way. + .env_remove("CI") + .env_remove("GITHUB_ACTIONS") .current_dir(&repo) .output() .expect("spawn corgea"); From d77a3e7fd10949ddd4a267585fa2045c3e15fcde Mon Sep 17 00:00:00 2001 From: Test Date: Tue, 28 Jul 2026 16:02:43 +0200 Subject: [PATCH 05/16] refactor(cli): core cleanups over the COR-1577 diff Behavior-preserving: - api.rs test stub reuses `read_http_request`/`http_response` instead of hand-rolling the drain loop and the response `format!`. - drop the never-read `ProjectsResponse.status` (serde ignores unknown fields, so it bought no validation). - extract `resolve_or_exit` for the error handler duplicated verbatim in `list` and `wait`. - list: resolve per branch, dropping the `Option` and its `expect`; collapse the duplicated unresolved-miss block to one copy (the JSON envelope still prints before the error). - wait: construct a `ResolvedProject` on the skip path, dropping the four `.as_ref().map(...)` chains; that path's name now falls back to "unknown" like `resolve_project` instead of an empty string. - generic.rs: share the origin-URL extraction via `origin_url`. - resolve_project: drop the `repo_was_explicit` flag and the lazy `cwd` closure. - generic.rs test: run its git commands through one helper that scrubs the inherited GIT_* env, as tests/cli_deps.rs already does, so the temp repo is built even when the suite runs from a git hook. --- src/list.rs | 97 ++++++++++++++++++-------------------------- src/utils/api.rs | 57 ++++++++++++++------------ src/utils/generic.rs | 72 +++++++++++++++----------------- src/wait.rs | 46 ++++++++------------- 4 files changed, 119 insertions(+), 153 deletions(-) diff --git a/src/list.rs b/src/list.rs index 838a479..fe3fc4e 100644 --- a/src/list.rs +++ b/src/list.rs @@ -115,35 +115,20 @@ pub fn run( Some(sca_issues_response.total_pages), ); } else { - // The --scan-id issue route hits /scan/{id}/issues and ignores the - // project, so it needs no resolution; every other path queries by name. - let resolved: Option = if *issues && scan_id.is_some() { - None - } else { - match utils::api::resolve_project( - &config.get_url(), - project_name_override.as_deref(), - repo_override.as_deref(), - ) { - Ok(resolved) => Some(resolved), - Err(e) => { - log::error!( - "Unable to resolve the Corgea project. Please check your connection and ensure that:\n\ - - The server URL is reachable.\n\ - - Your authentication token is valid.\n\n\ - Check out our docs at https://docs.corgea.app/install_cli#login-with-the-cli\n\n\ - Error details: {}", - e - ); - std::process::exit(1); - } - } - }; - let project_name = resolved - .as_ref() - .map(|r| r.query_name.clone()) - .unwrap_or_default(); if *issues { + // The --scan-id issue route hits /scan/{id}/issues and ignores the + // project, so it needs no resolution. + let resolved = scan_id.is_none().then(|| { + utils::api::resolve_or_exit( + &config.get_url(), + project_name_override.as_deref(), + repo_override.as_deref(), + ) + }); + let project_name = resolved + .as_ref() + .map(|r| r.query_name.clone()) + .unwrap_or_default(); let issues_response = match utils::api::get_scan_issues( &config.get_url(), &project_name, @@ -155,15 +140,14 @@ pub fn run( Err(e) => { debug(&format!("Error Sending Request: {}", e)); if e.to_string().contains("404") { - if scan_id.is_some() { - log::error!("Scan with ID '{}' doesn't exist. Please run 'corgea scan' to create a new scan for this project.", scan_id.as_ref().unwrap()); - } else if resolved.as_ref().map(|r| r.confirmed).unwrap_or(false) { - log::error!("Project '{}' has no issues yet. Run 'corgea scan' to create a scan for this project.", project_name); - } else { - log::error!( + // `resolved` is None exactly on the --scan-id route. + match &resolved { + None => log::error!("Scan with ID '{}' doesn't exist. Please run 'corgea scan' to create a new scan for this project.", scan_id.as_ref().unwrap()), + Some(r) if r.confirmed => log::error!("Project '{}' has no issues yet. Run 'corgea scan' to create a scan for this project.", project_name), + Some(r) => log::error!( "No Corgea project found for {}. Run 'corgea scan' to create one, or pass --project-name .", - resolved.as_ref().map(|r| r.tried_label.as_str()).unwrap_or_default() - ); + r.tried_label + ), } } else { log::error!( @@ -301,12 +285,15 @@ pub fn run( utils::terminal::print_table(table, issues_response.page, issues_response.total_pages); } else { - let resolved = resolved - .as_ref() - .expect("scan listing always resolves the project"); + let resolved = utils::api::resolve_or_exit( + &config.get_url(), + project_name_override.as_deref(), + repo_override.as_deref(), + ); + let project_name = &resolved.query_name; let (scans, page, total_pages) = match utils::api::query_scan_list( &config.get_url(), - Some(&project_name), + Some(project_name), *page, *page_size, ) { @@ -335,33 +322,29 @@ pub fn run( std::process::exit(1); } }; - // An unresolved project is a miss (exit 1, as --issues and `wait`); - // a confirmed project with no scans is a valid empty result. - let unresolved_miss = scans.is_empty() && !resolved.confirmed; if *json { let output = json!({ "page": page, "total_pages": total_pages, "results": scans }); + // The envelope prints first so JSON consumers get valid stdout + // even when the miss below exits 1. println!("{}", serde_json::to_string_pretty(&output).unwrap()); - if unresolved_miss { - log::error!( - "No Corgea project found for {}. Run 'corgea scan' to create one, or pass --project-name .", - resolved.tried_label - ); - std::process::exit(1); - } + } + // An unresolved project is a miss (exit 1, as --issues and `wait`); + // a confirmed project with no scans is a valid empty result. + if scans.is_empty() && !resolved.confirmed { + log::error!( + "No Corgea project found for {}. Run 'corgea scan' to create one, or pass --project-name .", + resolved.tried_label + ); + std::process::exit(1); + } + if *json { return; } if scans.is_empty() { - if unresolved_miss { - log::error!( - "No Corgea project found for {}. Run 'corgea scan' to create one, or pass --project-name .", - resolved.tried_label - ); - std::process::exit(1); - } println!( "Project '{}' has no scans yet. Run 'corgea scan' to create one.", project_name diff --git a/src/utils/api.rs b/src/utils/api.rs index cb5703f..04a4b21 100644 --- a/src/utils/api.rs +++ b/src/utils/api.rs @@ -745,9 +745,6 @@ pub struct ProjectSummary { #[derive(Deserialize, Debug)] pub struct ProjectsResponse { - // Part of the `@paginated` envelope; not consumed. - #[allow(dead_code)] - pub status: String, #[serde(default)] pub projects: Option>, } @@ -858,8 +855,6 @@ pub fn resolve_project( }); } - // Explicit --repo vs auto-detected remote changes the unconfirmed fallback. - let repo_was_explicit = repo_override.is_some(); // --repo may already be a bare `org/repo` (extract_repo_path needs a host // segment, so it returns None there) -> use the raw value. let repo_path = match repo_override { @@ -870,7 +865,7 @@ pub fn resolve_project( }; let cwd = - || utils::generic::get_current_working_directory().unwrap_or_else(|| "unknown".to_string()); + utils::generic::get_current_working_directory().unwrap_or_else(|| "unknown".to_string()); if let Some(repo_path) = repo_path { if let Some(project) = resolve_project_by_repo(url, &repo_path)? { @@ -881,10 +876,11 @@ pub fn resolve_project( tried_label: format!("repo '{}'", repo_path), }); } - let query_name = if repo_was_explicit { + // Explicit --repo vs auto-detected remote changes the unconfirmed fallback. + let query_name = if repo_override.is_some() { repo_path.clone() } else { - cwd() + cwd }; return Ok(ResolvedProject { query_name, @@ -894,7 +890,6 @@ pub fn resolve_project( }); } - let cwd = cwd(); Ok(ResolvedProject { tried_label: format!("directory '{}'", cwd), query_name: cwd, @@ -903,6 +898,29 @@ pub fn resolve_project( }) } +/// `resolve_project`, or a hard exit with the shared failure copy. Every +/// caller treats a resolver error as fatal. +pub fn resolve_or_exit( + url: &str, + project_name_override: Option<&str>, + repo_override: Option<&str>, +) -> ResolvedProject { + match resolve_project(url, project_name_override, repo_override) { + Ok(resolved) => resolved, + Err(e) => { + log::error!( + "Unable to resolve the Corgea project. Please check your connection and ensure that:\n\ + - The server URL is reachable.\n\ + - Your authentication token is valid.\n\n\ + Check out our docs at https://docs.corgea.app/install_cli#login-with-the-cli\n\n\ + Error details: {}", + e + ); + std::process::exit(1); + } + } +} + pub fn exchange_code_for_token( base_url: &str, code: &str, @@ -1518,7 +1536,7 @@ mod tests { // As `spawn_projects_stub` but with a caller-chosen status line, for the // resolver error-path tests (404 soft-miss vs 5xx hard error). fn spawn_projects_stub_status(status_line: &'static str, body: &'static str) -> String { - use std::io::{Read, Write}; + use std::io::Write; let listener = TcpListener::bind("127.0.0.1:0").expect("bind stub"); let base = format!("http://127.0.0.1:{}", listener.local_addr().unwrap().port()); thread::spawn(move || { @@ -1528,23 +1546,8 @@ mod tests { // socket with an unread request still in the kernel buffer // triggers a TCP RST that surfaces on the client as hyper // `UnexpectedMessage` (flaky, timing-dependent). - let mut chunk = [0u8; 1024]; - let mut buf = Vec::new(); - while let Ok(n) = stream.read(&mut chunk) { - if n == 0 { - break; - } - buf.extend_from_slice(&chunk[..n]); - if buf.windows(4).any(|w| w == b"\r\n\r\n") { - break; - } - } - let resp = format!( - "HTTP/1.1 {}\r\nContent-Length: {}\r\nContent-Type: application/json\r\nConnection: close\r\n\r\n{}", - status_line, - body.len(), - body - ); + let _ = corgea::vuln_api_stub::read_http_request(&mut stream); + let resp = corgea::vuln_api_stub::http_response(status_line, "", body); let _ = stream.write_all(resp.as_bytes()); } }); diff --git a/src/utils/generic.rs b/src/utils/generic.rs index faf9eb2..573b453 100644 --- a/src/utils/generic.rs +++ b/src/utils/generic.rs @@ -326,27 +326,26 @@ pub fn get_repo_info(dir: &str) -> Result, git2::Error> { .map(|commit| commit.id().to_string()) }); - let repo_url = repo - .find_remote("origin") - .ok() - .and_then(|remote| remote.url().map(|url| url.to_string())); - Ok(Some(RepoInfo { branch, - repo_url, + repo_url: origin_url(&repo), sha, })) } +/// `origin`'s URL, or None when the remote is missing or carries no URL. +fn origin_url(repo: &Repository) -> Option { + repo.find_remote("origin") + .ok() + .and_then(|remote| remote.url().map(|url| url.to_string())) +} + /// The enclosing repository's `origin` remote URL, searched upward from the /// current directory so `corgea list`/`wait` also resolve from a subdirectory. /// `get_repo_info` deliberately returns None outside the worktree root; this /// does not. None outside a git repo or when `origin` carries no URL. pub fn discover_repo_url() -> Option { - let repo = Repository::discover(Path::new(".")).ok()?; - repo.find_remote("origin") - .ok() - .and_then(|remote| remote.url().map(|url| url.to_string())) + origin_url(&Repository::discover(Path::new(".")).ok()?) } /// True when `dir` is the repository worktree root (not a subdirectory). @@ -394,41 +393,34 @@ mod tests { use std::fs; use std::process::Command; - #[test] - fn get_repo_info_at_root_only_not_nested_cwd() { - let dir = tempfile::tempdir().unwrap(); - let root = dir.path(); - assert!(Command::new("git") - .args(["init"]) - .current_dir(root) - .status() - .unwrap() - .success()); + /// `git ` in `dir`. Scrubs the GIT_* env a parent git process + /// injects (e.g. when these tests run from a pre-commit hook): an + /// inherited GIT_DIR would point `git init` at the developer's repo + /// instead of `dir`. Same scrub as `tests/cli_deps.rs::run_git`. + fn git(dir: &Path, args: &[&str]) { assert!(Command::new("git") - .args(["config", "user.email", "test@example.com"]) - .current_dir(root) - .status() - .unwrap() - .success()); - assert!(Command::new("git") - .args(["config", "user.name", "Test"]) - .current_dir(root) + .args(args) + .current_dir(dir) + .env_remove("GIT_DIR") + .env_remove("GIT_WORK_TREE") + .env_remove("GIT_COMMON_DIR") + .env_remove("GIT_INDEX_FILE") + .env_remove("GIT_PREFIX") .status() .unwrap() .success()); + } + + #[test] + fn get_repo_info_at_root_only_not_nested_cwd() { + let dir = tempfile::tempdir().unwrap(); + let root = dir.path(); + git(root, &["init"]); + git(root, &["config", "user.email", "test@example.com"]); + git(root, &["config", "user.name", "Test"]); fs::write(root.join("README"), "hi").unwrap(); - assert!(Command::new("git") - .args(["add", "README"]) - .current_dir(root) - .status() - .unwrap() - .success()); - assert!(Command::new("git") - .args(["commit", "-m", "init"]) - .current_dir(root) - .status() - .unwrap() - .success()); + git(root, &["add", "README"]); + git(root, &["commit", "-m", "init"]); let root_s = root.to_str().unwrap(); let nested = root.join("pkg").join("inner"); diff --git a/src/wait.rs b/src/wait.rs index cb3bbef..1995e15 100644 --- a/src/wait.rs +++ b/src/wait.rs @@ -12,36 +12,25 @@ pub fn run( // A scan id plus the project id from the upload response leaves nothing to // resolve: everything below keys off the scan, and the id-form URL is // already known. - let resolved: Option = if scan_id.is_some() - && project_id_override.is_some() - { - None + let resolved = if scan_id.is_some() && project_id_override.is_some() { + let name = project_name_override.clone().unwrap_or_else(|| { + utils::generic::get_current_working_directory().unwrap_or_else(|| "unknown".to_string()) + }); + utils::api::ResolvedProject { + // `confirmed`/`tried_label` are only read on the no-scan-id path. + tried_label: format!("project '{}'", name), + query_name: name, + confirmed: false, + project_id: None, + } } else { - match utils::api::resolve_project( + utils::api::resolve_or_exit( &config.get_url(), project_name_override.as_deref(), repo_override.as_deref(), - ) { - Ok(resolved) => Some(resolved), - Err(e) => { - log::error!( - "Unable to resolve the Corgea project. Please check your connection and ensure that:\n\ - - The server URL is reachable.\n\ - - Your authentication token is valid.\n\n\ - Check out our docs at https://docs.corgea.app/install_cli#login-with-the-cli\n\n\ - Error details: {}", - e - ); - std::process::exit(1); - } - } - }; - let project_name = match &resolved { - Some(r) => r.query_name.clone(), - None => project_name_override - .clone() - .unwrap_or_else(|| utils::generic::get_current_working_directory().unwrap_or_default()), + ) }; + let project_name = resolved.query_name.clone(); // Only the scan-less path reads the listing. let scans: Vec = if scan_id.is_some() { @@ -80,7 +69,7 @@ pub fn run( None => match scans.first() { Some(scan) => (scan.id.clone(), scan.status == "Complete"), None => { - if resolved.as_ref().map(|r| r.confirmed).unwrap_or(false) { + if resolved.confirmed { log::error!( "Project '{}' has no scans yet. Run 'corgea scan' to start one.", project_name @@ -88,7 +77,7 @@ pub fn run( } else { log::error!( "No scan to wait for: no Corgea project found for {}. Run 'corgea scan', or pass --scan-id / --project-name.", - resolved.as_ref().map(|r| r.tried_label.as_str()).unwrap_or_default() + resolved.tried_label ); } std::process::exit(1); @@ -96,8 +85,7 @@ pub fn run( }, }; - let project_id = - project_id_override.or_else(|| resolved.as_ref().and_then(|r| r.project_id.clone())); + let project_id = project_id_override.or(resolved.project_id); let scan_url = match &project_id { Some(pid) => format!("{}/project/{}/?scan_id={}", config.get_url(), pid, scan_id), None => { From 175f2aff5345c3bc1f8101d1d5c645dbf4d3a6ab Mon Sep 17 00:00:00 2001 From: Test Date: Tue, 28 Jul 2026 16:06:29 +0200 Subject: [PATCH 06/16] refactor(cli): pass list/wait arguments as structs `wait::run` took five positional params, four of them Option; `repo_override` and `project_id_override` were adjacent and same-typed, so transposing them at the scan.rs call sites compiled silently and changed resolution. `list::run` took nine and carried #[allow(clippy::too_many_arguments)]. - `ProjectSelector { name, repo }` replaces the two Option<&str> resolver params, so the pair travels as one value. - `WaitArgs`/`ListArgs` name every argument at the call sites; the too_many_arguments allow is gone rather than institutionalized. - list::run now owns its arguments, so two `is_some()` + `unwrap()` pairs clippy started flagging become `if let Some(id)`. --- src/list.rs | 88 +++++++++++++++++++++++------------------------- src/main.rs | 32 +++++++++++------- src/scan.rs | 24 ++++++++----- src/utils/api.rs | 24 ++++++++----- src/wait.rs | 35 ++++++++++--------- 5 files changed, 114 insertions(+), 89 deletions(-) diff --git a/src/list.rs b/src/list.rs index fe3fc4e..f12b78d 100644 --- a/src/list.rs +++ b/src/list.rs @@ -1,40 +1,50 @@ use crate::config::Config; use crate::log::debug; use crate::utils; +use crate::utils::api::ProjectSelector; use serde_json::json; use std::path::Path; -#[allow(clippy::too_many_arguments)] -pub fn run( - config: &Config, - issues: &bool, - sca_issues: &bool, - json: &bool, - page: &Option, - page_size: &Option, - scan_id: &Option, - project_name_override: Option, - repo_override: Option, -) { - if !*json { +#[derive(Default)] +pub struct ListArgs { + pub issues: bool, + pub sca_issues: bool, + pub json: bool, + pub page: Option, + pub page_size: Option, + pub scan_id: Option, + pub selector: ProjectSelector, +} + +pub fn run(config: &Config, args: ListArgs) { + let ListArgs { + issues, + sca_issues, + json, + page, + page_size, + scan_id, + selector, + } = args; + if !json { println!(); } - if *sca_issues { + if sca_issues { // SCA has no project parameter; the CWD basename is only error copy. let project_name = utils::generic::get_current_working_directory().unwrap_or("unknown".to_string()); let sca_issues_response = match utils::api::get_sca_issues( &config.get_url(), - Some((*page).unwrap_or(1)), - *page_size, + Some(page.unwrap_or(1)), + page_size, scan_id.clone(), ) { Ok(response) => response, Err(e) => { debug(&format!("Error Sending Request: {}", e)); if e.to_string().contains("404") { - if scan_id.is_some() { - log::error!("Scan with ID '{}' doesn't exist or has no SCA issues. Please run 'corgea scan' to create a new scan for this project.", scan_id.as_ref().unwrap()); + if let Some(id) = &scan_id { + log::error!("Scan with ID '{}' doesn't exist or has no SCA issues. Please run 'corgea scan' to create a new scan for this project.", id); } else { log::error!("No SCA issues found for project '{}'. Please run 'corgea scan' to create a new scan for this project.", project_name); } @@ -51,7 +61,7 @@ pub fn run( } }; - if *json { + if json { let output = serde_json::json!({ "page": sca_issues_response.page, "total_pages": sca_issues_response.total_pages, @@ -115,16 +125,12 @@ pub fn run( Some(sca_issues_response.total_pages), ); } else { - if *issues { + if issues { // The --scan-id issue route hits /scan/{id}/issues and ignores the // project, so it needs no resolution. - let resolved = scan_id.is_none().then(|| { - utils::api::resolve_or_exit( - &config.get_url(), - project_name_override.as_deref(), - repo_override.as_deref(), - ) - }); + let resolved = scan_id + .is_none() + .then(|| utils::api::resolve_or_exit(&config.get_url(), &selector)); let project_name = resolved .as_ref() .map(|r| r.query_name.clone()) @@ -132,8 +138,8 @@ pub fn run( let issues_response = match utils::api::get_scan_issues( &config.get_url(), &project_name, - Some((*page).unwrap_or(1)), - *page_size, + Some(page.unwrap_or(1)), + page_size, scan_id.clone(), ) { Ok(response) => response, @@ -165,14 +171,10 @@ pub fn run( let mut blocking_rules: std::collections::HashMap = std::collections::HashMap::new(); - if scan_id.is_some() { + if let Some(id) = &scan_id { let mut page: u32 = 1; loop { - match utils::api::check_blocking_rules( - &config.get_url(), - scan_id.as_ref().unwrap(), - Some(page), - ) { + match utils::api::check_blocking_rules(&config.get_url(), id, Some(page)) { Ok(rules) => { if rules.block { render_blocking_rules = true; @@ -196,7 +198,7 @@ pub fn run( } } - if *json { + if json { let mut json = serde_json::json!({ "page": issues_response.page, "total_pages": issues_response.total_pages, @@ -285,17 +287,13 @@ pub fn run( utils::terminal::print_table(table, issues_response.page, issues_response.total_pages); } else { - let resolved = utils::api::resolve_or_exit( - &config.get_url(), - project_name_override.as_deref(), - repo_override.as_deref(), - ); + let resolved = utils::api::resolve_or_exit(&config.get_url(), &selector); let project_name = &resolved.query_name; let (scans, page, total_pages) = match utils::api::query_scan_list( &config.get_url(), Some(project_name), - *page, - *page_size, + page, + page_size, ) { Ok(scans) => { let page = scans.page; @@ -322,7 +320,7 @@ pub fn run( std::process::exit(1); } }; - if *json { + if json { let output = json!({ "page": page, "total_pages": total_pages, @@ -341,7 +339,7 @@ pub fn run( ); std::process::exit(1); } - if *json { + if json { return; } if scans.is_empty() { diff --git a/src/main.rs b/src/main.rs index 5fb65f1..df64d0c 100644 --- a/src/main.rs +++ b/src/main.rs @@ -697,10 +697,14 @@ fn main() { verify_token_and_exit_when_fail(&corgea_config); wait::run( &corgea_config, - scan_id.clone(), - project_name.clone(), - repo.clone(), - None, + wait::WaitArgs { + scan_id: scan_id.clone(), + selector: utils::api::ProjectSelector { + name: project_name.clone(), + repo: repo.clone(), + }, + upload_project_id: None, + }, ); } Some(Commands::List { @@ -724,14 +728,18 @@ fn main() { } list::run( &corgea_config, - issues, - sca_issues, - json, - page, - page_size, - scan_id, - project_name.clone(), - repo.clone(), + list::ListArgs { + issues: *issues, + sca_issues: *sca_issues, + json: *json, + page: *page, + page_size: *page_size, + scan_id: scan_id.clone(), + selector: utils::api::ProjectSelector { + name: project_name.clone(), + repo: repo.clone(), + }, + }, ); } Some(Commands::Inspect { diff --git a/src/scan.rs b/src/scan.rs index 2f7cd73..ec0dbcd 100644 --- a/src/scan.rs +++ b/src/scan.rs @@ -68,10 +68,14 @@ pub fn run_semgrep(config: &Config, project_name: Option) { if let Some(result) = parse_scan(config, output, true, project_name.clone()) { crate::wait::run( config, - Some(result.scan_id), - project_name, - None, - result.project_id, + crate::wait::WaitArgs { + scan_id: Some(result.scan_id), + selector: crate::utils::api::ProjectSelector { + name: project_name, + ..Default::default() + }, + upload_project_id: result.project_id, + }, ); } } @@ -89,10 +93,14 @@ pub fn run_snyk(config: &Config, project_name: Option) { if let Some(result) = parse_scan(config, output, true, project_name.clone()) { crate::wait::run( config, - Some(result.scan_id), - project_name, - None, - result.project_id, + crate::wait::WaitArgs { + scan_id: Some(result.scan_id), + selector: crate::utils::api::ProjectSelector { + name: project_name, + ..Default::default() + }, + upload_project_id: result.project_id, + }, ); } } diff --git a/src/utils/api.rs b/src/utils/api.rs index 04a4b21..1bc28d9 100644 --- a/src/utils/api.rs +++ b/src/utils/api.rs @@ -836,6 +836,16 @@ pub struct ResolvedProject { pub tried_label: String, } +/// How the caller asked for a project: `--project-name` and `--repo` are +/// mutually exclusive at the CLI, but both travel together to the resolver. +/// One struct so the two same-typed options cannot be transposed at a call +/// site. +#[derive(Default, Clone)] +pub struct ProjectSelector { + pub name: Option, + pub repo: Option, +} + /// Resolve which project `list`/`wait` should query: `--project-name` verbatim, /// else the repo path from `--repo` or the discovered remote. Unconfirmed, an /// explicit `--repo` queries that path as a name and everything else falls back @@ -843,10 +853,10 @@ pub struct ResolvedProject { /// failure (network/auth/5xx from /projects). pub fn resolve_project( url: &str, - project_name_override: Option<&str>, - repo_override: Option<&str>, + selector: &ProjectSelector, ) -> Result> { - if let Some(name) = project_name_override { + let repo_override = selector.repo.as_deref(); + if let Some(name) = selector.name.as_deref() { return Ok(ResolvedProject { query_name: name.to_string(), confirmed: false, @@ -900,12 +910,8 @@ pub fn resolve_project( /// `resolve_project`, or a hard exit with the shared failure copy. Every /// caller treats a resolver error as fatal. -pub fn resolve_or_exit( - url: &str, - project_name_override: Option<&str>, - repo_override: Option<&str>, -) -> ResolvedProject { - match resolve_project(url, project_name_override, repo_override) { +pub fn resolve_or_exit(url: &str, selector: &ProjectSelector) -> ResolvedProject { + match resolve_project(url, selector) { Ok(resolved) => resolved, Err(e) => { log::error!( diff --git a/src/wait.rs b/src/wait.rs index 1995e15..09e3611 100644 --- a/src/wait.rs +++ b/src/wait.rs @@ -1,19 +1,28 @@ use crate::config::Config; use crate::scanners::blast; use crate::utils; +use crate::utils::api::ProjectSelector; + +#[derive(Default)] +pub struct WaitArgs { + pub scan_id: Option, + pub selector: ProjectSelector, + /// Project id straight from an upload response, skipping resolution. + pub upload_project_id: Option, +} + +pub fn run(config: &Config, args: WaitArgs) { + let WaitArgs { + scan_id, + selector, + upload_project_id, + } = args; -pub fn run( - config: &Config, - scan_id: Option, - project_name_override: Option, - repo_override: Option, - project_id_override: Option, -) { // A scan id plus the project id from the upload response leaves nothing to // resolve: everything below keys off the scan, and the id-form URL is // already known. - let resolved = if scan_id.is_some() && project_id_override.is_some() { - let name = project_name_override.clone().unwrap_or_else(|| { + let resolved = if scan_id.is_some() && upload_project_id.is_some() { + let name = selector.name.clone().unwrap_or_else(|| { utils::generic::get_current_working_directory().unwrap_or_else(|| "unknown".to_string()) }); utils::api::ResolvedProject { @@ -24,11 +33,7 @@ pub fn run( project_id: None, } } else { - utils::api::resolve_or_exit( - &config.get_url(), - project_name_override.as_deref(), - repo_override.as_deref(), - ) + utils::api::resolve_or_exit(&config.get_url(), &selector) }; let project_name = resolved.query_name.clone(); @@ -85,7 +90,7 @@ pub fn run( }, }; - let project_id = project_id_override.or(resolved.project_id); + let project_id = upload_project_id.or(resolved.project_id); let scan_url = match &project_id { Some(pid) => format!("{}/project/{}/?scan_id={}", config.get_url(), pid, scan_id), None => { From 0b0abdd7d30cb4e6f645d4fd96e783f772c83435 Mon Sep 17 00:00:00 2001 From: Test Date: Tue, 28 Jul 2026 16:11:56 +0200 Subject: [PATCH 07/16] test(cli): share the resolution e2e stub harness `spawn_stub` and `run_list`/`run_wait` were near-identical across list_resolution.rs and wait_resolution.rs, and the Arc>> hit-recording closure was copy-pasted at seven sites. - `common::Routes` is one route table for every endpoint the two suites stub; an unset field 404s, so "this endpoint is never dialed" tests just leave it out. - `spawn_recording_http_stub` wraps `spawn_http_stub` with the hit log, mirroring `vuln_api_stub::spawn_capturing_vuln_api_stub`. - `run_corgea(subcommand, ...)` replaces the two copies of the runner. Scaffolding only: no assertion changed. --- tests/common/mod.rs | 100 +++++++++++++++++++++++++++ tests/list_resolution.rs | 104 +++++++++------------------- tests/wait_resolution.rs | 146 ++++++++++----------------------------- 3 files changed, 170 insertions(+), 180 deletions(-) diff --git a/tests/common/mod.rs b/tests/common/mod.rs index 08e1969..786f4d7 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -127,6 +127,27 @@ where base_url } +/// Every request target a stub was asked for, in order. +#[allow(dead_code)] +pub type Hits = std::sync::Arc>>; + +/// `spawn_http_stub` that also records every request target, so a test can +/// assert which endpoints were (not) dialed. Mirrors +/// `vuln_api_stub::spawn_capturing_vuln_api_stub`. +#[allow(dead_code)] +pub fn spawn_recording_http_stub(route: F) -> (String, Hits) +where + F: Fn(&str) -> (&'static str, String) + Send + 'static, +{ + let hits: Hits = Default::default(); + let recorder = hits.clone(); + let base_url = spawn_http_stub(move |path| { + recorder.lock().unwrap().push(path.to_string()); + route(path) + }); + (base_url, hits) +} + /// Registry stub serving `/pypi/oldpkg/json` (pypi) and `/oldpkg` (npm /// packument), both published 2020 → never recent. Everything else 404s. #[allow(dead_code)] @@ -492,3 +513,82 @@ pub fn temp_plain_dir(dirname: &str) -> (TempDir, std::path::PathBuf) { std::fs::create_dir(&dir).expect("create dir"); (tmp, dir) } + +/// The endpoints the `list`/`wait` resolution tests stub. `/verify` is always +/// answered `ok`; a field left `None` 404s, as does any other path — so a test +/// asserting an endpoint was never dialed simply leaves it unset. +/// +/// Routing is on the request-target PREFIX: `/projects` carries a +/// percent-encoded query, so the full target is not a stable key. +#[allow(dead_code)] +#[derive(Default, Clone)] +pub struct Routes { + pub projects: Option, + pub scans: Option, + pub issues: Option, + /// `GET /scan/{id}` — `check_scan_status`. + pub scan: Option, + /// `GET /scan/{id}/issues` — `report_scan_status` and the `--scan-id` + /// issue route. + pub scan_issues: Option, +} + +#[allow(dead_code)] +impl Routes { + /// The stub answer for `path`: a served body, else 404. Tests needing an + /// endpoint outside this table match it first and delegate here. + pub fn answer(&self, path: &str) -> (&'static str, String) { + let body = if path.starts_with("/api/v1/verify") { + Some(r#"{"status":"ok"}"#.to_string()) + } else if path.starts_with("/api/v1/projects?repo_url=") { + self.projects.clone() + } else if path.starts_with("/api/v1/scans?") { + self.scans.clone() + } else if path.starts_with("/api/v1/issues?") { + self.issues.clone() + } else if path.starts_with("/api/v1/scan/") { + if path.contains("/issues") { + self.scan_issues.clone() + } else { + self.scan.clone() + } + } else { + None + }; + match body { + Some(body) => ("200 OK", body), + None => ("404 Not Found", NOT_FOUND_JSON.to_string()), + } + } +} + +/// `Routes` behind a plain stub; returns the base URL. +#[allow(dead_code)] +pub fn spawn_resolution_stub(routes: Routes) -> String { + spawn_http_stub(move |path| routes.answer(path)) +} + +/// `Routes` behind a recording stub; returns the base URL and the hit log. +#[allow(dead_code)] +pub fn spawn_recording_resolution_stub(routes: Routes) -> (String, Hits) { + spawn_recording_http_stub(move |path| routes.answer(path)) +} + +/// Run `corgea ` against `url` from `cwd`, isolated from +/// the host (temp HOME). `CORGEA_URL`/`CORGEA_TOKEN` are layered back on after +/// `corgea_isolated` strips them. +#[allow(dead_code)] +pub fn run_corgea( + subcommand: &str, + args: &[&str], + url: &str, + cwd: &std::path::Path, +) -> std::process::Output { + let (mut cmd, _home) = corgea_isolated(); + cmd.arg(subcommand); + cmd.args(args); + cmd.env("CORGEA_URL", url) + .env("CORGEA_TOKEN", "test-token") + .current_dir(cwd); + cmd.output().expect("spawn corgea") +} diff --git a/tests/list_resolution.rs b/tests/list_resolution.rs index 7f1cb33..cc029ea 100644 --- a/tests/list_resolution.rs +++ b/tests/list_resolution.rs @@ -1,12 +1,11 @@ //! End-to-end tests for `corgea list` repo-URL project resolution (COR-1577). -//! Stubs route on the request-target path PREFIX: `/projects` carries a -//! percent-encoded query, so the full target is not a stable key. +//! The stub route table lives in `common::Routes`. mod common; use common::{ - projects_empty, projects_match, scans_empty, scans_one, temp_git_repo, temp_plain_dir, CANON, - REMOTE, + projects_empty, projects_match, scans_empty, scans_one, temp_git_repo, temp_plain_dir, Routes, + CANON, REMOTE, }; use std::path::Path; use std::process::Output; @@ -25,34 +24,22 @@ fn issues_miss() -> String { // --- harness --------------------------------------------------------------- -/// Stub serving verify + the three listing endpoints, keyed on path prefix. +/// The three listing endpoints `list` reads. +fn routes(projects: String, scans: String, issues: String) -> Routes { + Routes { + projects: Some(projects), + scans: Some(scans), + issues: Some(issues), + ..Default::default() + } +} + fn spawn_stub(projects: String, scans: String, issues: String) -> String { - common::spawn_http_stub(move |path| { - if path.starts_with("/api/v1/verify") { - ("200 OK", r#"{"status":"ok"}"#.to_string()) - } else if path.starts_with("/api/v1/projects?repo_url=") { - ("200 OK", projects.clone()) - } else if path.starts_with("/api/v1/scans?") { - ("200 OK", scans.clone()) - } else if path.starts_with("/api/v1/issues?") { - ("200 OK", issues.clone()) - } else { - ("404 Not Found", r#"{"message":"not found"}"#.to_string()) - } - }) + common::spawn_resolution_stub(routes(projects, scans, issues)) } -/// Run `corgea list ` against `url` from `cwd`, isolated from the -/// host (temp HOME). `CORGEA_URL`/`CORGEA_TOKEN` are layered back on after -/// `corgea_isolated` strips them. fn run_list(args: &[&str], url: &str, cwd: &Path) -> Output { - let (mut cmd, _home) = common::corgea_isolated(); - cmd.arg("list"); - cmd.args(args); - cmd.env("CORGEA_URL", url) - .env("CORGEA_TOKEN", "test-token") - .current_dir(cwd); - cmd.output().expect("spawn corgea") + common::run_corgea("list", args, url, cwd) } // --- tests ----------------------------------------------------------------- @@ -95,21 +82,11 @@ fn list_issues_shows_repo_resolved_issue() { #[test] fn list_repo_flag_resolves_from_flag_not_remote() { - use std::sync::{Arc, Mutex}; - let hits = Arc::new(Mutex::new(Vec::::new())); - let hits_c = hits.clone(); // Records every request target so we can prove the slug came from --repo. - let url = common::spawn_http_stub(move |path| { - hits_c.lock().unwrap().push(path.to_string()); - if path.starts_with("/api/v1/verify") { - ("200 OK", r#"{"status":"ok"}"#.to_string()) - } else if path.starts_with("/api/v1/projects?repo_url=") { - ("200 OK", projects_match()) - } else if path.starts_with("/api/v1/scans?") { - ("200 OK", scans_one(CANON)) - } else { - ("404 Not Found", r#"{"message":"not found"}"#.to_string()) - } + let (url, hits) = common::spawn_recording_resolution_stub(Routes { + projects: Some(projects_match()), + scans: Some(scans_one(CANON)), + ..Default::default() }); // Non-git dir: no remote, so a resolved slug can ONLY have come from --repo. let (_tmp, dir) = temp_plain_dir("unrelated-dir"); @@ -247,28 +224,23 @@ fn list_json_miss_is_valid_empty_envelope() { #[test] fn list_issues_with_scan_id_skips_project_resolution() { - use std::sync::{Arc, Mutex}; - let hits = Arc::new(Mutex::new(Vec::::new())); - let hits_c = hits.clone(); // The scan-id issue route ignores the project, so no /projects call should // be made even from a real git repo where a remote IS present. (COR-1577) - let url = common::spawn_http_stub(move |path| { - hits_c.lock().unwrap().push(path.to_string()); - if path.starts_with("/api/v1/verify") { - ("200 OK", r#"{"status":"ok"}"#.to_string()) - } else if path.contains("/check_blocking_rules") { + // `projects` stays unset: dialing it would 404 and show up in the hits. + let routes = Routes { + scan_issues: Some( + r#"{"status":"ok","page":1,"total_pages":1,"total_issues":0,"issues":[]}"#.to_string(), + ), + ..Default::default() + }; + let (url, hits) = common::spawn_recording_http_stub(move |path| { + if path.contains("/check_blocking_rules") { ( "200 OK", r#"{"block":false,"blocking_issues":[],"total_pages":1}"#.to_string(), ) - } else if path.starts_with("/api/v1/scan/") && path.contains("/issues") { - ( - "200 OK", - r#"{"status":"ok","page":1,"total_pages":1,"total_issues":0,"issues":[]}"# - .to_string(), - ) } else { - ("404 Not Found", r#"{"message":"not found"}"#.to_string()) + routes.answer(path) } }); let (_tmp, repo) = temp_git_repo("dotnet-azure-web-tsb", REMOTE); @@ -292,20 +264,10 @@ fn list_issues_with_scan_id_skips_project_resolution() { #[test] fn list_resolves_from_subdirectory() { - use std::sync::{Arc, Mutex}; - let hits = Arc::new(Mutex::new(Vec::::new())); - let hits_c = hits.clone(); - let url = common::spawn_http_stub(move |path| { - hits_c.lock().unwrap().push(path.to_string()); - if path.starts_with("/api/v1/verify") { - ("200 OK", r#"{"status":"ok"}"#.to_string()) - } else if path.starts_with("/api/v1/projects?repo_url=") { - ("200 OK", projects_match()) - } else if path.starts_with("/api/v1/scans?") { - ("200 OK", scans_one(CANON)) - } else { - ("404 Not Found", r#"{"message":"not found"}"#.to_string()) - } + let (url, hits) = common::spawn_recording_resolution_stub(Routes { + projects: Some(projects_match()), + scans: Some(scans_one(CANON)), + ..Default::default() }); let (_tmp, repo) = temp_git_repo("dotnet-azure-web-tsb", REMOTE); let subdir = repo.join("src"); diff --git a/tests/wait_resolution.rs b/tests/wait_resolution.rs index 243f577..09cd0c4 100644 --- a/tests/wait_resolution.rs +++ b/tests/wait_resolution.rs @@ -1,13 +1,12 @@ //! End-to-end tests for `corgea wait` repo-URL project resolution (COR-1577). -//! Stubs route on the request-target path PREFIX. The `/api/v1/scan/` arm -//! serves both `GET /scan/{id}` (`check_scan_status`) and `/scan/{id}/issues` -//! (`report_scan_status`), branching on `contains("/issues")`. +//! The stub route table lives in `common::Routes`, whose `scan`/`scan_issues` +//! arms serve `check_scan_status` and `report_scan_status` respectively. mod common; use common::{ - projects_empty, projects_match, scans_empty, scans_one, temp_git_repo, temp_plain_dir, CANON, - REMOTE, + projects_empty, projects_match, scans_empty, scans_one, temp_git_repo, temp_plain_dir, Routes, + CANON, REMOTE, }; use std::path::Path; use std::process::Output; @@ -28,39 +27,29 @@ fn scan_issues_empty() -> String { // --- harness --------------------------------------------------------------- -/// Stub serving verify + projects + scans + the single-scan/scan-issues -/// endpoints, keyed on path prefix. +/// The single-scan endpoints every `wait` run reads once it has a scan id. +fn scan_routes() -> Routes { + Routes { + scan: Some(scan_complete()), + scan_issues: Some(scan_issues_empty()), + ..Default::default() + } +} + +fn routes(projects: String, scans: String) -> Routes { + Routes { + projects: Some(projects), + scans: Some(scans), + ..scan_routes() + } +} + fn spawn_stub(projects: String, scans: String) -> String { - common::spawn_http_stub(move |path| { - if path.starts_with("/api/v1/verify") { - ("200 OK", r#"{"status":"ok"}"#.to_string()) - } else if path.starts_with("/api/v1/projects?repo_url=") { - ("200 OK", projects.clone()) - } else if path.starts_with("/api/v1/scans?") { - ("200 OK", scans.clone()) - } else if path.starts_with("/api/v1/scan/") { - if path.contains("/issues") { - ("200 OK", scan_issues_empty()) - } else { - ("200 OK", scan_complete()) - } - } else { - ("404 Not Found", r#"{"message":"not found"}"#.to_string()) - } - }) + common::spawn_resolution_stub(routes(projects, scans)) } -/// Run `corgea wait ` against `url` from `cwd`, isolated from the host -/// (temp HOME). `CORGEA_URL`/`CORGEA_TOKEN` are layered back on after -/// `corgea_isolated` strips them. fn run_wait(args: &[&str], url: &str, cwd: &Path) -> Output { - let (mut cmd, _home) = common::corgea_isolated(); - cmd.arg("wait"); - cmd.args(args); - cmd.env("CORGEA_URL", url) - .env("CORGEA_TOKEN", "test-token") - .current_dir(cwd); - cmd.output().expect("spawn corgea") + common::run_corgea("wait", args, url, cwd) } // --- tests ----------------------------------------------------------------- @@ -86,28 +75,9 @@ fn wait_uses_canonical_project_and_numeric_id_scan_url() { #[test] fn wait_repo_flag_resolves_from_flag_not_remote() { - use std::sync::{Arc, Mutex}; - let hits = Arc::new(Mutex::new(Vec::::new())); - let hits_c = hits.clone(); // Records every request target so we can prove the slug came from --repo. - let url = common::spawn_http_stub(move |path| { - hits_c.lock().unwrap().push(path.to_string()); - if path.starts_with("/api/v1/verify") { - ("200 OK", r#"{"status":"ok"}"#.to_string()) - } else if path.starts_with("/api/v1/projects?repo_url=") { - ("200 OK", projects_match()) - } else if path.starts_with("/api/v1/scans?") { - ("200 OK", scans_one(CANON)) - } else if path.starts_with("/api/v1/scan/") { - if path.contains("/issues") { - ("200 OK", scan_issues_empty()) - } else { - ("200 OK", scan_complete()) - } - } else { - ("404 Not Found", r#"{"message":"not found"}"#.to_string()) - } - }); + let (url, hits) = + common::spawn_recording_resolution_stub(routes(projects_match(), scans_one(CANON))); // Non-git dir: no remote, so a resolved slug can ONLY have come from --repo. let (_tmp, dir) = temp_plain_dir("unrelated-dir"); let out = run_wait(&["--repo", "bohappdev/dotnet-azure-web-tsb"], &url, &dir); @@ -193,27 +163,8 @@ fn wait_project_name_trailing_slash_is_trimmed() { #[test] fn wait_resolves_from_subdirectory() { - use std::sync::{Arc, Mutex}; - let hits = Arc::new(Mutex::new(Vec::::new())); - let hits_c = hits.clone(); - let url = common::spawn_http_stub(move |path| { - hits_c.lock().unwrap().push(path.to_string()); - if path.starts_with("/api/v1/verify") { - ("200 OK", r#"{"status":"ok"}"#.to_string()) - } else if path.starts_with("/api/v1/projects?repo_url=") { - ("200 OK", projects_match()) - } else if path.starts_with("/api/v1/scans?") { - ("200 OK", scans_one(CANON)) - } else if path.starts_with("/api/v1/scan/") { - if path.contains("/issues") { - ("200 OK", scan_issues_empty()) - } else { - ("200 OK", scan_complete()) - } - } else { - ("404 Not Found", r#"{"message":"not found"}"#.to_string()) - } - }); + let (url, hits) = + common::spawn_recording_resolution_stub(routes(projects_match(), scans_one(CANON))); let (_tmp, repo) = temp_git_repo("dotnet-azure-web-tsb", REMOTE); let subdir = repo.join("src"); std::fs::create_dir(&subdir).expect("create subdir"); @@ -241,24 +192,10 @@ fn wait_resolves_from_subdirectory() { #[test] fn wait_with_scan_id_does_not_list_scans() { - use std::sync::{Arc, Mutex}; - let hits = Arc::new(Mutex::new(Vec::::new())); - let hits_c = hits.clone(); - let url = common::spawn_http_stub(move |path| { - hits_c.lock().unwrap().push(path.to_string()); - if path.starts_with("/api/v1/verify") { - ("200 OK", r#"{"status":"ok"}"#.to_string()) - } else if path.starts_with("/api/v1/projects?repo_url=") { - ("200 OK", projects_match()) - } else if path.starts_with("/api/v1/scan/") { - if path.contains("/issues") { - ("200 OK", scan_issues_empty()) - } else { - ("200 OK", scan_complete()) - } - } else { - ("404 Not Found", r#"{"message":"not found"}"#.to_string()) - } + // `scans` stays unset — the assertion is that it is never dialed. + let (url, hits) = common::spawn_recording_resolution_stub(Routes { + projects: Some(projects_match()), + ..scan_routes() }); let (_tmp, repo) = temp_git_repo("dotnet-azure-web-tsb", REMOTE); let out = run_wait(&["scan-123"], &url, &repo); @@ -282,14 +219,11 @@ fn wait_with_scan_id_does_not_list_scans() { #[cfg(unix)] #[test] fn scan_post_wait_uses_upload_project_id_without_resolving() { - use std::sync::{Arc, Mutex}; - let hits = Arc::new(Mutex::new(Vec::::new())); - let hits_c = hits.clone(); - let url = common::spawn_http_stub(move |path| { - hits_c.lock().unwrap().push(path.to_string()); - if path.starts_with("/api/v1/verify") - || path.starts_with("/api/v1/code-upload") - || path.starts_with("/api/v1/git-config-upload") + // Neither `projects` nor `scans` is served — the assertions are that + // neither is dialed. The upload endpoints are this test's own. + let routes = scan_routes(); + let (url, hits) = common::spawn_recording_http_stub(move |path| { + if path.starts_with("/api/v1/code-upload") || path.starts_with("/api/v1/git-config-upload") { ("200 OK", r#"{"status":"ok"}"#.to_string()) } else if path.starts_with("/api/v1/scan-upload") { @@ -297,14 +231,8 @@ fn scan_post_wait_uses_upload_project_id_without_resolving() { "200 OK", r#"{"status":"ok","sast_scan_id":"scan-123","project_id":42}"#.to_string(), ) - } else if path.starts_with("/api/v1/scan/") { - if path.contains("/issues") { - ("200 OK", scan_issues_empty()) - } else { - ("200 OK", scan_complete()) - } } else { - ("404 Not Found", r#"{"message":"not found"}"#.to_string()) + routes.answer(path) } }); let (_tmp, repo) = temp_git_repo("dotnet-azure-web-tsb", REMOTE); From da0848d6b323cf39634e9c4e55737875e2936713 Mon Sep 17 00:00:00 2001 From: Test Date: Tue, 28 Jul 2026 16:15:04 +0200 Subject: [PATCH 08/16] style(cli): early-return the SCA branch in list::run The diff wrapped ~290 lines in a new `} else {`. The SCA branch ends at its print_table call, so returning there puts the issues and scan-listing arms back at their original indent level. Reviewable with `git show --ignore-all-space`: the removed `else`, the `return;`, one closing brace, and one line rustfmt rejoined now that it fits. Nothing else. --- src/list.rs | 468 ++++++++++++++++++++++++++-------------------------- 1 file changed, 234 insertions(+), 234 deletions(-) diff --git a/src/list.rs b/src/list.rs index f12b78d..48da7d8 100644 --- a/src/list.rs +++ b/src/list.rs @@ -124,266 +124,266 @@ pub fn run(config: &Config, args: ListArgs) { Some(sca_issues_response.page), Some(sca_issues_response.total_pages), ); - } else { - if issues { - // The --scan-id issue route hits /scan/{id}/issues and ignores the - // project, so it needs no resolution. - let resolved = scan_id - .is_none() - .then(|| utils::api::resolve_or_exit(&config.get_url(), &selector)); - let project_name = resolved - .as_ref() - .map(|r| r.query_name.clone()) - .unwrap_or_default(); - let issues_response = match utils::api::get_scan_issues( - &config.get_url(), - &project_name, - Some(page.unwrap_or(1)), - page_size, - scan_id.clone(), - ) { - Ok(response) => response, - Err(e) => { - debug(&format!("Error Sending Request: {}", e)); - if e.to_string().contains("404") { - // `resolved` is None exactly on the --scan-id route. - match &resolved { - None => log::error!("Scan with ID '{}' doesn't exist. Please run 'corgea scan' to create a new scan for this project.", scan_id.as_ref().unwrap()), - Some(r) if r.confirmed => log::error!("Project '{}' has no issues yet. Run 'corgea scan' to create a scan for this project.", project_name), - Some(r) => log::error!( - "No Corgea project found for {}. Run 'corgea scan' to create one, or pass --project-name .", - r.tried_label - ), - } - } else { - log::error!( - "Unable to fetch scan issues. Please check your connection and ensure that:\n\ - - The server URL is reachable.\n\ - - Your authentication token is valid.\n\n\ - Check out our docs at https://docs.corgea.app/install_cli#login-with-the-cli {}", - e - ); + return; + } + + if issues { + // The --scan-id issue route hits /scan/{id}/issues and ignores the + // project, so it needs no resolution. + let resolved = scan_id + .is_none() + .then(|| utils::api::resolve_or_exit(&config.get_url(), &selector)); + let project_name = resolved + .as_ref() + .map(|r| r.query_name.clone()) + .unwrap_or_default(); + let issues_response = match utils::api::get_scan_issues( + &config.get_url(), + &project_name, + Some(page.unwrap_or(1)), + page_size, + scan_id.clone(), + ) { + Ok(response) => response, + Err(e) => { + debug(&format!("Error Sending Request: {}", e)); + if e.to_string().contains("404") { + // `resolved` is None exactly on the --scan-id route. + match &resolved { + None => log::error!("Scan with ID '{}' doesn't exist. Please run 'corgea scan' to create a new scan for this project.", scan_id.as_ref().unwrap()), + Some(r) if r.confirmed => log::error!("Project '{}' has no issues yet. Run 'corgea scan' to create a scan for this project.", project_name), + Some(r) => log::error!( + "No Corgea project found for {}. Run 'corgea scan' to create one, or pass --project-name .", + r.tried_label + ), } - std::process::exit(1); + } else { + log::error!( + "Unable to fetch scan issues. Please check your connection and ensure that:\n\ + - The server URL is reachable.\n\ + - Your authentication token is valid.\n\n\ + Check out our docs at https://docs.corgea.app/install_cli#login-with-the-cli {}", + e + ); } - }; - let mut render_blocking_rules = false; - let mut blocking_rules: std::collections::HashMap = - std::collections::HashMap::new(); + std::process::exit(1); + } + }; + let mut render_blocking_rules = false; + let mut blocking_rules: std::collections::HashMap = + std::collections::HashMap::new(); - if let Some(id) = &scan_id { - let mut page: u32 = 1; - loop { - match utils::api::check_blocking_rules(&config.get_url(), id, Some(page)) { - Ok(rules) => { - if rules.block { - render_blocking_rules = true; - for issue in rules.blocking_issues { - blocking_rules - .insert(issue.id, issue.triggered_by_rules.join(",")); - } - if rules.total_pages == page { - break; - } - page += 1; - } else { + if let Some(id) = &scan_id { + let mut page: u32 = 1; + loop { + match utils::api::check_blocking_rules(&config.get_url(), id, Some(page)) { + Ok(rules) => { + if rules.block { + render_blocking_rules = true; + for issue in rules.blocking_issues { + blocking_rules.insert(issue.id, issue.triggered_by_rules.join(",")); + } + if rules.total_pages == page { break; } - } - Err(e) => { - log::error!("Failed to check blocking rules: {}", e); - std::process::exit(1); + page += 1; + } else { + break; } } + Err(e) => { + log::error!("Failed to check blocking rules: {}", e); + std::process::exit(1); + } } } + } - if json { - let mut json = serde_json::json!({ - "page": issues_response.page, - "total_pages": issues_response.total_pages, - "results": &issues_response.issues - }); - if render_blocking_rules { - json["results"] = serde_json::json!(issues_response - .issues - .unwrap_or_default() - .iter() - .map(|issue| { - serde_json::json!(utils::api::IssueWithBlockingRules { - id: issue.id.clone(), - scan_id: issue.scan_id.clone(), - status: issue.status.clone(), - urgency: issue.urgency.clone(), - created_at: issue.created_at.clone(), - classification: issue.classification.clone(), - location: issue.location.clone(), - details: issue.details.clone(), - auto_triage: issue.auto_triage.clone(), - auto_fix_suggestion: issue.auto_fix_suggestion.clone(), - blocked: blocking_rules.contains_key(&issue.id), - blocking_rules: if blocking_rules.contains_key(&issue.id) { - Some(vec![blocking_rules.get(&issue.id).unwrap().clone()]) - } else { - None - } - }) - }) - .collect::>()); - } - let output = json!(json); - println!("{}", serde_json::to_string_pretty(&output).unwrap()); - return; - } - let mut table_header = vec![ - "Issue ID".to_string(), - "Category".to_string(), - "Urgency".to_string(), - "File Path".to_string(), - "Line".to_string(), - ]; + if json { + let mut json = serde_json::json!({ + "page": issues_response.page, + "total_pages": issues_response.total_pages, + "results": &issues_response.issues + }); if render_blocking_rules { - table_header.push("Blocking".to_string()); - table_header.push("Rule ID".to_string()); + json["results"] = serde_json::json!(issues_response + .issues + .unwrap_or_default() + .iter() + .map(|issue| { + serde_json::json!(utils::api::IssueWithBlockingRules { + id: issue.id.clone(), + scan_id: issue.scan_id.clone(), + status: issue.status.clone(), + urgency: issue.urgency.clone(), + created_at: issue.created_at.clone(), + classification: issue.classification.clone(), + location: issue.location.clone(), + details: issue.details.clone(), + auto_triage: issue.auto_triage.clone(), + auto_fix_suggestion: issue.auto_fix_suggestion.clone(), + blocked: blocking_rules.contains_key(&issue.id), + blocking_rules: if blocking_rules.contains_key(&issue.id) { + Some(vec![blocking_rules.get(&issue.id).unwrap().clone()]) + } else { + None + } + }) + }) + .collect::>()); } - let mut table = vec![table_header]; + let output = json!(json); + println!("{}", serde_json::to_string_pretty(&output).unwrap()); + return; + } + let mut table_header = vec![ + "Issue ID".to_string(), + "Category".to_string(), + "Urgency".to_string(), + "File Path".to_string(), + "Line".to_string(), + ]; + if render_blocking_rules { + table_header.push("Blocking".to_string()); + table_header.push("Rule ID".to_string()); + } + let mut table = vec![table_header]; - for issue in &issues_response.issues.unwrap_or_default() { - let classification_display = issue.classification.id.clone(); - let path = Path::new(&issue.location.file.path); - let path_parts: Vec<&str> = path - .components() - .filter_map(|c| c.as_os_str().to_str()) - .collect(); + for issue in &issues_response.issues.unwrap_or_default() { + let classification_display = issue.classification.id.clone(); + let path = Path::new(&issue.location.file.path); + let path_parts: Vec<&str> = path + .components() + .filter_map(|c| c.as_os_str().to_str()) + .collect(); - let shortened_path = if path_parts.len() > 2 { - let base_part = if path_parts[0].len() > 1 { - path_parts[0] - } else { - path_parts[1] - }; - format!("{}/../{}", base_part, path_parts[path_parts.len() - 1]).to_string() + let shortened_path = if path_parts.len() > 2 { + let base_part = if path_parts[0].len() > 1 { + path_parts[0] } else { - issue.location.file.path.clone() + path_parts[1] }; - let mut row = vec![ - issue.id.clone(), - classification_display, - issue.urgency.clone(), - shortened_path, - issue.location.line_number.to_string(), - ]; - if render_blocking_rules { - row.push(blocking_rules.contains_key(&issue.id).to_string()); - row.push( - blocking_rules - .get(&issue.id) - .unwrap_or(&"".to_string()) - .to_string(), - ); - } - table.push(row); - } - - utils::terminal::print_table(table, issues_response.page, issues_response.total_pages); - } else { - let resolved = utils::api::resolve_or_exit(&config.get_url(), &selector); - let project_name = &resolved.query_name; - let (scans, page, total_pages) = match utils::api::query_scan_list( - &config.get_url(), - Some(project_name), - page, - page_size, - ) { - Ok(scans) => { - let page = scans.page; - let total_pages = scans.total_pages; - // The server already filtered by the resolved project; the old - // client-side `scan.project == cwd_basename` pass would discard - // every repo-resolved scan. (COR-1577) - (scans.scans.unwrap_or_default(), page, total_pages) - } - Err(e) => { - if e.to_string().contains("404") { - log::error!( - "No Corgea project found for {}. Run 'corgea scan' to create one, or pass --project-name .", - resolved.tried_label - ); - } else { - log::error!( - "Unable to fetch scans. Please check your connection and ensure that:\n\ - - The server URL is reachable.\n\ - - Your authentication token is valid.\n\n\ - Check out our docs at https://docs.corgea.app/install_cli#login-with-the-cli" - ); - } - std::process::exit(1); - } + format!("{}/../{}", base_part, path_parts[path_parts.len() - 1]).to_string() + } else { + issue.location.file.path.clone() }; - if json { - let output = json!({ - "page": page, - "total_pages": total_pages, - "results": scans - }); - // The envelope prints first so JSON consumers get valid stdout - // even when the miss below exits 1. - println!("{}", serde_json::to_string_pretty(&output).unwrap()); - } - // An unresolved project is a miss (exit 1, as --issues and `wait`); - // a confirmed project with no scans is a valid empty result. - if scans.is_empty() && !resolved.confirmed { - log::error!( - "No Corgea project found for {}. Run 'corgea scan' to create one, or pass --project-name .", - resolved.tried_label + let mut row = vec![ + issue.id.clone(), + classification_display, + issue.urgency.clone(), + shortened_path, + issue.location.line_number.to_string(), + ]; + if render_blocking_rules { + row.push(blocking_rules.contains_key(&issue.id).to_string()); + row.push( + blocking_rules + .get(&issue.id) + .unwrap_or(&"".to_string()) + .to_string(), ); - std::process::exit(1); } - if json { - return; + table.push(row); + } + + utils::terminal::print_table(table, issues_response.page, issues_response.total_pages); + } else { + let resolved = utils::api::resolve_or_exit(&config.get_url(), &selector); + let project_name = &resolved.query_name; + let (scans, page, total_pages) = match utils::api::query_scan_list( + &config.get_url(), + Some(project_name), + page, + page_size, + ) { + Ok(scans) => { + let page = scans.page; + let total_pages = scans.total_pages; + // The server already filtered by the resolved project; the old + // client-side `scan.project == cwd_basename` pass would discard + // every repo-resolved scan. (COR-1577) + (scans.scans.unwrap_or_default(), page, total_pages) } - if scans.is_empty() { - println!( - "Project '{}' has no scans yet. Run 'corgea scan' to create one.", - project_name - ); - return; + Err(e) => { + if e.to_string().contains("404") { + log::error!( + "No Corgea project found for {}. Run 'corgea scan' to create one, or pass --project-name .", + resolved.tried_label + ); + } else { + log::error!( + "Unable to fetch scans. Please check your connection and ensure that:\n\ + - The server URL is reachable.\n\ + - Your authentication token is valid.\n\n\ + Check out our docs at https://docs.corgea.app/install_cli#login-with-the-cli" + ); + } + std::process::exit(1); } - let mut table = vec![vec![ - "Scan ID".to_string(), - "Project".to_string(), - "Status".to_string(), - "Repo".to_string(), - "Branch".to_string(), - "SHA".to_string(), - ]]; + }; + if json { + let output = json!({ + "page": page, + "total_pages": total_pages, + "results": scans + }); + // The envelope prints first so JSON consumers get valid stdout + // even when the miss below exits 1. + println!("{}", serde_json::to_string_pretty(&output).unwrap()); + } + // An unresolved project is a miss (exit 1, as --issues and `wait`); + // a confirmed project with no scans is a valid empty result. + if scans.is_empty() && !resolved.confirmed { + log::error!( + "No Corgea project found for {}. Run 'corgea scan' to create one, or pass --project-name .", + resolved.tried_label + ); + std::process::exit(1); + } + if json { + return; + } + if scans.is_empty() { + println!( + "Project '{}' has no scans yet. Run 'corgea scan' to create one.", + project_name + ); + return; + } + let mut table = vec![vec![ + "Scan ID".to_string(), + "Project".to_string(), + "Status".to_string(), + "Repo".to_string(), + "Branch".to_string(), + "SHA".to_string(), + ]]; - for scan in &scans { - let formatted_repo = scan.repo.clone().unwrap_or("N/A".to_string()); - let formatted_repo = if formatted_repo != "N/A" { - if let Some(repo_name) = formatted_repo.split('/').next_back() { - let owner = formatted_repo.split('/').nth(3).unwrap_or("unknown"); - let repo_name = repo_name.strip_suffix(".git").unwrap_or(repo_name); - format!("{}/{}", owner, repo_name) - } else { - formatted_repo - } + for scan in &scans { + let formatted_repo = scan.repo.clone().unwrap_or("N/A".to_string()); + let formatted_repo = if formatted_repo != "N/A" { + if let Some(repo_name) = formatted_repo.split('/').next_back() { + let owner = formatted_repo.split('/').nth(3).unwrap_or("unknown"); + let repo_name = repo_name.strip_suffix(".git").unwrap_or(repo_name); + format!("{}/{}", owner, repo_name) } else { formatted_repo - }; - - table.push(vec![ - scan.id.clone(), - scan.project.clone(), - scan.status.clone(), - formatted_repo, - scan.branch.clone().unwrap_or("N/A".to_string()), - format_short_sha(scan.git_sha.as_deref()), - ]); - } + } + } else { + formatted_repo + }; - utils::terminal::print_table(table, page, total_pages); + table.push(vec![ + scan.id.clone(), + scan.project.clone(), + scan.status.clone(), + formatted_repo, + scan.branch.clone().unwrap_or("N/A".to_string()), + format_short_sha(scan.git_sha.as_deref()), + ]); } + + utils::terminal::print_table(table, page, total_pages); } } From 173be59058d73cef4f32775588829eaed854ad2d Mon Sep 17 00:00:00 2001 From: Test Date: Tue, 28 Jul 2026 16:43:48 +0200 Subject: [PATCH 09/16] fix(cli): page through /projects and fail loudly on a bad body (PR #122 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two resolver gaps found in review: - `/api/v1/projects` is `@paginated(default_page_size=20, max_page_size=50)` over a `repo_url__icontains` filter ordered by `-created_at` (doghouse api/views/core.py:1640-1683, api/decorators.py:158). Searching only page 1 meant enough `acme/api-*` siblings could push the exact `acme/api` onto page 2, the guard reported no match, and resolution fell back to the CWD name — the very miss COR-1577 fixes. Now requests page_size=50 and walks pages until an exact match or the last page, bounded by PROJECTS_MAX_PAGES so a bogus total_pages cannot loop forever. - A 200 carrying an unparseable body (proxy HTML, incompatible schema) was converted to a clean miss, taking the same CWD-name fallback. That contradicts the function's own contract — only a 404 or a valid envelope with no match is soft — and can silently resolve a DIFFERENT project. It now propagates, like the 5xx path. --- src/utils/api.rs | 165 +++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 137 insertions(+), 28 deletions(-) diff --git a/src/utils/api.rs b/src/utils/api.rs index 1bc28d9..83000b1 100644 --- a/src/utils/api.rs +++ b/src/utils/api.rs @@ -747,8 +747,19 @@ pub struct ProjectSummary { pub struct ProjectsResponse { #[serde(default)] pub projects: Option>, + /// Absent on a backend that does not paginate -> treated as a single page. + #[serde(default)] + pub total_pages: Option, } +/// The backend's `@paginated` `max_page_size` for /projects; a larger value is +/// clamped server-side. +const PROJECTS_PAGE_SIZE: u16 = 50; + +/// Ceiling on pages walked looking for an exact repo match, so a bogus +/// server-reported `total_pages` cannot drive an unbounded request loop. +const PROJECTS_MAX_PAGES: u32 = 20; + /// Stringify a scalar JSON id; None otherwise, so no `/project/null/` URL. fn id_to_string(v: &serde_json::Value) -> Option { match v { @@ -772,28 +783,30 @@ fn repo_url_matches_path(repo_url: &str, expected_path: &str) -> bool { r == expected_path } -/// Resolve the canonical project for a repo path via GET /api/v1/projects?repo_url=… -/// -/// Old-backend safety guard: a pre-COR-1426 backend ignores the unknown -/// `repo_url` param and returns ALL company projects, so every candidate is -/// re-checked against the path here — on such a backend none match, and the -/// caller falls back to the CWD-name path. +/// One page of GET /api/v1/projects?repo_url=… /// -/// `Err` for hard failures (network/auth/5xx); a clean "no match" (or a 404 from -/// a backend without the endpoint) is a soft `Ok(None)`. -pub fn resolve_project_by_repo( +/// `Ok(None)` only for a 404 (a backend without the endpoint). A 5xx or a body +/// that does not parse is an `Err`: both are hard failures, and treating them +/// as a clean miss would silently fall back to the CWD-name path. +fn fetch_projects_page( url: &str, repo_path: &str, -) -> Result, Box> { + page: u32, +) -> Result, Box> { let request_url = format!("{}{}/projects", url, API_BASE); let client = http_client(); debug(&format!( - "Resolving project via {} (repo_url={})", - request_url, repo_path + "Resolving project via {} (repo_url={}, page={})", + request_url, repo_path, page )); + let (page, page_size) = (page.to_string(), PROJECTS_PAGE_SIZE.to_string()); let response = client .get(&request_url) - .query(&[("repo_url", repo_path)]) + .query(&[ + ("repo_url", repo_path), + ("page", &page), + ("page_size", &page_size), + ]) .send()?; check_for_warnings(response.headers(), response.status()); let status = response.status(); @@ -804,24 +817,63 @@ pub fn resolve_project_by_repo( return Err(format!("/projects request failed: HTTP {}", status).into()); } let text = response.text()?; - let parsed: ProjectsResponse = match serde_json::from_str(&text) { - Ok(p) => p, + match serde_json::from_str(&text) { + Ok(parsed) => Ok(Some(parsed)), Err(e) => { - debug(&format!( - "Failed to parse /projects response: {} | body: {}", - e, text - )); - return Ok(None); + debug(&format!("/projects response body: {}", text)); + Err(format!("Failed to parse the /projects response: {}", e).into()) } - }; + } +} + +/// Resolve the canonical project for a repo path via GET /api/v1/projects?repo_url=… +/// +/// The backend filters `repo_url__icontains` over a paginated list, so the +/// exact repo can sit behind a page of siblings (`acme/api-v2`, …) — pages are +/// walked until it turns up or they run out. +/// +/// Old-backend safety guard: a pre-COR-1426 backend ignores the unknown +/// `repo_url` param and returns ALL company projects, so every candidate is +/// re-checked against the path here — on such a backend none match, and the +/// caller falls back to the CWD-name path. +/// +/// `Err` for hard failures (network/auth/5xx/unparseable body); a clean "no +/// match" (or a 404 from a backend without the endpoint) is a soft `Ok(None)`. +pub fn resolve_project_by_repo( + url: &str, + repo_path: &str, +) -> Result, Box> { let expected = repo_path.to_lowercase(); - let matched = parsed.projects.unwrap_or_default().into_iter().find(|p| { - p.repo_url - .as_deref() - .map(|r| repo_url_matches_path(r, &expected)) - .unwrap_or(false) - }); - Ok(matched) + let mut page = 1; + loop { + let Some(parsed) = fetch_projects_page(url, repo_path, page)? else { + return Ok(None); + }; + let total_pages = parsed.total_pages.unwrap_or(1); + let projects = parsed.projects.unwrap_or_default(); + if projects.is_empty() { + return Ok(None); + } + let matched = projects.into_iter().find(|p| { + p.repo_url + .as_deref() + .map(|r| repo_url_matches_path(r, &expected)) + .unwrap_or(false) + }); + if matched.is_some() { + return Ok(matched); + } + if page >= total_pages.min(PROJECTS_MAX_PAGES) { + if total_pages > PROJECTS_MAX_PAGES { + debug(&format!( + "Gave up after {} /projects pages ({} reported)", + PROJECTS_MAX_PAGES, total_pages + )); + } + return Ok(None); + } + page += 1; + } } /// What `list`/`wait` need to drive the existing name-based queries. @@ -1560,6 +1612,28 @@ mod tests { base } + // Serves `page_one` until a request carries `page=2`, then `page_two` — + // for the pagination walk. + fn spawn_paged_projects_stub(page_one: &'static str, page_two: &'static str) -> String { + use std::io::Write; + let listener = TcpListener::bind("127.0.0.1:0").expect("bind stub"); + let base = format!("http://127.0.0.1:{}", listener.local_addr().unwrap().port()); + thread::spawn(move || { + for stream in listener.incoming() { + let Ok(mut stream) = stream else { continue }; + let request = corgea::vuln_api_stub::read_http_request(&mut stream); + let body = if String::from_utf8_lossy(&request).contains("page=2") { + page_two + } else { + page_one + }; + let resp = corgea::vuln_api_stub::http_response("200 OK", "", body); + let _ = stream.write_all(resp.as_bytes()); + } + }); + base + } + #[test] fn resolve_project_by_repo_keeps_only_repo_url_matches() { // New backend: filter applied, one matching project returned. @@ -1661,4 +1735,39 @@ mod tests { let base = spawn_projects_stub_status("500 Internal Server Error", r#"{"error":"boom"}"#); assert!(resolve_project_by_repo(&base, "org/repo").is_err()); } + + #[test] + fn resolve_project_by_repo_unparseable_body_is_hard_err() { + // A 200 carrying HTML (proxy / captive portal) or an incompatible + // schema is a hard failure too: silently reporting "no match" would + // resolve a DIFFERENT project via the CWD-name fallback (COR-1577 + // review). + let base = spawn_projects_stub("Access denied"); + assert!(resolve_project_by_repo(&base, "org/repo").is_err()); + } + + #[test] + fn resolve_project_by_repo_walks_past_the_first_page() { + // The backend filters `repo_url__icontains` over a 20-per-page list, so + // enough `acme/api-*` siblings push the exact `acme/api` onto page 2. + // Stopping at page 1 would recreate the very miss this resolves. + let base = spawn_paged_projects_stub( + r#"{"status":"ok","total_pages":2,"projects":[{"id":1,"name":"acme/api-v2","repo_url":"https://github.com/acme/api-v2"},{"id":2,"name":"acme/api-gw","repo_url":"https://github.com/acme/api-gw"}]}"#, + r#"{"status":"ok","total_pages":2,"projects":[{"id":9,"name":"acme/api","repo_url":"https://github.com/acme/api"}]}"#, + ); + let got = resolve_project_by_repo(&base, "acme/api").unwrap(); + assert_eq!(got.map(|p| p.name).as_deref(), Some("acme/api")); + } + + #[test] + fn resolve_project_by_repo_stops_at_the_last_page() { + // `total_pages: 1` (and its absence, as in the other stubs here) must + // end the walk rather than request page 2 forever. + let base = spawn_projects_stub( + r#"{"status":"ok","total_pages":1,"projects":[{"id":1,"name":"acme/api-v2","repo_url":"https://github.com/acme/api-v2"}]}"#, + ); + assert!(resolve_project_by_repo(&base, "acme/api") + .unwrap() + .is_none()); + } } From 26cbaa6e0f89a34190e7fc1422b5e2087da8fd9f Mon Sep 17 00:00:00 2001 From: Test Date: Wed, 29 Jul 2026 09:53:24 +0200 Subject: [PATCH 10/16] feat(cli): add `corgea wait --project-id` (PR #122 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Rename `resolve_or_exit` -> `resolve_project_or_exit` so it reads consistently with its sibling `resolve_project`. - Expose the project id `wait` already accepted internally from an upload response as a `--project-id` flag, and rename the field to `project_id` now that it has two sources. Paired with a scan id it skips resolution entirely, which is what a CI job passing the id between steps wants — and it makes `wait` usable during a /projects outage. `list` gets no equivalent flag: `/api/v1/issues` and `/api/v1/scans` accept only `project` (exact name) or `repo` (icontains), never an id (doghouse api/views/core.py:707,2057), so an id cannot drive those queries until the backend takes one. --- src/list.rs | 4 ++-- src/main.rs | 8 +++++++- src/scan.rs | 4 ++-- src/utils/api.rs | 2 +- src/wait.rs | 18 +++++++++--------- tests/wait_resolution.rs | 28 ++++++++++++++++++++++++++++ 6 files changed, 49 insertions(+), 15 deletions(-) diff --git a/src/list.rs b/src/list.rs index 48da7d8..af22129 100644 --- a/src/list.rs +++ b/src/list.rs @@ -132,7 +132,7 @@ pub fn run(config: &Config, args: ListArgs) { // project, so it needs no resolution. let resolved = scan_id .is_none() - .then(|| utils::api::resolve_or_exit(&config.get_url(), &selector)); + .then(|| utils::api::resolve_project_or_exit(&config.get_url(), &selector)); let project_name = resolved .as_ref() .map(|r| r.query_name.clone()) @@ -288,7 +288,7 @@ pub fn run(config: &Config, args: ListArgs) { utils::terminal::print_table(table, issues_response.page, issues_response.total_pages); } else { - let resolved = utils::api::resolve_or_exit(&config.get_url(), &selector); + let resolved = utils::api::resolve_project_or_exit(&config.get_url(), &selector); let project_name = &resolved.query_name; let (scans, page, total_pages) = match utils::api::query_scan_list( &config.get_url(), diff --git a/src/main.rs b/src/main.rs index df64d0c..c64fb3f 100644 --- a/src/main.rs +++ b/src/main.rs @@ -154,6 +154,11 @@ enum Commands { help = "Resolve the project from this repo (org/repo slug or remote URL) instead of the git remote." )] repo: Option, + #[arg( + long, + help = "Use this known Corgea project id for the result link. Together with a scan id it skips project resolution entirely." + )] + project_id: Option, }, /// List something, by default it lists the scans #[command(alias = "ls")] @@ -693,6 +698,7 @@ fn main() { scan_id, project_name, repo, + project_id, }) => { verify_token_and_exit_when_fail(&corgea_config); wait::run( @@ -703,7 +709,7 @@ fn main() { name: project_name.clone(), repo: repo.clone(), }, - upload_project_id: None, + project_id: project_id.clone(), }, ); } diff --git a/src/scan.rs b/src/scan.rs index ec0dbcd..cd0af92 100644 --- a/src/scan.rs +++ b/src/scan.rs @@ -74,7 +74,7 @@ pub fn run_semgrep(config: &Config, project_name: Option) { name: project_name, ..Default::default() }, - upload_project_id: result.project_id, + project_id: result.project_id, }, ); } @@ -99,7 +99,7 @@ pub fn run_snyk(config: &Config, project_name: Option) { name: project_name, ..Default::default() }, - upload_project_id: result.project_id, + project_id: result.project_id, }, ); } diff --git a/src/utils/api.rs b/src/utils/api.rs index 83000b1..47eb208 100644 --- a/src/utils/api.rs +++ b/src/utils/api.rs @@ -962,7 +962,7 @@ pub fn resolve_project( /// `resolve_project`, or a hard exit with the shared failure copy. Every /// caller treats a resolver error as fatal. -pub fn resolve_or_exit(url: &str, selector: &ProjectSelector) -> ResolvedProject { +pub fn resolve_project_or_exit(url: &str, selector: &ProjectSelector) -> ResolvedProject { match resolve_project(url, selector) { Ok(resolved) => resolved, Err(e) => { diff --git a/src/wait.rs b/src/wait.rs index 09e3611..d4add4b 100644 --- a/src/wait.rs +++ b/src/wait.rs @@ -7,21 +7,21 @@ use crate::utils::api::ProjectSelector; pub struct WaitArgs { pub scan_id: Option, pub selector: ProjectSelector, - /// Project id straight from an upload response, skipping resolution. - pub upload_project_id: Option, + /// Known project id — `--project-id`, or straight from an upload response. + /// Paired with a scan id it skips resolution entirely. + pub project_id: Option, } pub fn run(config: &Config, args: WaitArgs) { let WaitArgs { scan_id, selector, - upload_project_id, + project_id: known_project_id, } = args; - // A scan id plus the project id from the upload response leaves nothing to - // resolve: everything below keys off the scan, and the id-form URL is - // already known. - let resolved = if scan_id.is_some() && upload_project_id.is_some() { + // A scan id plus a known project id leaves nothing to resolve: everything + // below keys off the scan, and the id-form URL is already known. + let resolved = if scan_id.is_some() && known_project_id.is_some() { let name = selector.name.clone().unwrap_or_else(|| { utils::generic::get_current_working_directory().unwrap_or_else(|| "unknown".to_string()) }); @@ -33,7 +33,7 @@ pub fn run(config: &Config, args: WaitArgs) { project_id: None, } } else { - utils::api::resolve_or_exit(&config.get_url(), &selector) + utils::api::resolve_project_or_exit(&config.get_url(), &selector) }; let project_name = resolved.query_name.clone(); @@ -90,7 +90,7 @@ pub fn run(config: &Config, args: WaitArgs) { }, }; - let project_id = upload_project_id.or(resolved.project_id); + let project_id = known_project_id.or(resolved.project_id); let scan_url = match &project_id { Some(pid) => format!("{}/project/{}/?scan_id={}", config.get_url(), pid, scan_id), None => { diff --git a/tests/wait_resolution.rs b/tests/wait_resolution.rs index 09cd0c4..2399c64 100644 --- a/tests/wait_resolution.rs +++ b/tests/wait_resolution.rs @@ -213,6 +213,34 @@ fn wait_with_scan_id_does_not_list_scans() { ); } +#[test] +fn wait_project_id_flag_skips_resolution() { + // A caller who already knows the id (CI passing it between steps) needs no + // lookup at all: neither `projects` nor `scans` is served, and the id-form + // URL still comes out. (PR #122 review) + let (url, hits) = common::spawn_recording_resolution_stub(scan_routes()); + let (_tmp, repo) = temp_git_repo("dotnet-azure-web-tsb", REMOTE); + let out = run_wait(&["scan-123", "--project-id", "42"], &url, &repo); + let stdout = String::from_utf8_lossy(&out.stdout); + assert_eq!( + out.status.code(), + Some(0), + "stderr: {}", + String::from_utf8_lossy(&out.stderr) + ); + assert!( + stdout.contains("/project/42/?scan_id=scan-123"), + "stdout: {stdout}" + ); + let hits = hits.lock().unwrap(); + assert!( + !hits + .iter() + .any(|h| h.starts_with("/api/v1/projects") || h.starts_with("/api/v1/scans")), + "--project-id must resolve nothing; hits: {hits:?}" + ); +} + /// `corgea scan semgrep` with a fake `semgrep` on PATH: the post-scan wait gets /// the project id straight from the upload response, so it must resolve nothing /// — no `/projects`, no `/scans` — and still link the id-form URL. From efe1ce0776a53e7fa55384ea7dd5c9f6fa82c034 Mon Sep 17 00:00:00 2001 From: Test Date: Wed, 29 Jul 2026 10:07:24 +0200 Subject: [PATCH 11/16] fix(cli): host-qualify repo matches and restore the legacy fallback name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four resolver defects from the second review pass: - `repo_url_matches_path` compared paths only, so `github.com/acme/api` and `gitlab.com/acme/api` were interchangeable — the backend's hostless `icontains` returns both, and whichever sorted first won. Candidates are now held to the origin's host when both sides carry one; a bare `--repo org/repo` or a stored bare `acme/api` still matches, having no host to contradict. `extract_repo_host` joins `extract_repo_path` over a shared `split_remote`. - The unconfirmed fallback queried the checkout directory name, but the pre-COR-1577 CLI queried `determine_project_name(None)` — the repo basename at the worktree root. Cloning `acme/api` into `build-123` thus made an old or not-yet-onboarded backend miss a project it used to find. It now calls that same helper, so the fallback is unchanged from before. - `corgea list --project-name X` exited 1 when X had no scans: an explicit name never sets `confirmed`, and /scans answers 200-empty both for "no scans" and "no such project". The caller's exact name is the better authority, so this is now the same valid empty result as a confirmed project with no scans. - `--project-id` without a scan id only relabeled the link while the scan was still picked by the resolved name, so it could print another project's URL. clap now requires the scan id and rejects an empty value. --- src/list.rs | 9 ++- src/main.rs | 4 +- src/utils/api.rs | 167 ++++++++++++++++++++++++++++++--------- src/utils/generic.rs | 19 ++++- tests/list_resolution.rs | 49 ++++++++++++ tests/wait_resolution.rs | 15 ++++ 6 files changed, 217 insertions(+), 46 deletions(-) diff --git a/src/list.rs b/src/list.rs index af22129..2d21b49 100644 --- a/src/list.rs +++ b/src/list.rs @@ -331,9 +331,12 @@ pub fn run(config: &Config, args: ListArgs) { // even when the miss below exits 1. println!("{}", serde_json::to_string_pretty(&output).unwrap()); } - // An unresolved project is a miss (exit 1, as --issues and `wait`); - // a confirmed project with no scans is a valid empty result. - if scans.is_empty() && !resolved.confirmed { + // An unresolved project is a miss (exit 1, as --issues and `wait`); a + // confirmed project with no scans is a valid empty result. So is an + // explicit --project-name: /scans answers 200-empty either way, so the + // caller's own exact name is the better authority than a guess that it + // does not exist. + if scans.is_empty() && !resolved.confirmed && selector.name.is_none() { log::error!( "No Corgea project found for {}. Run 'corgea scan' to create one, or pass --project-name .", resolved.tried_label diff --git a/src/main.rs b/src/main.rs index c64fb3f..96d2c46 100644 --- a/src/main.rs +++ b/src/main.rs @@ -156,7 +156,9 @@ enum Commands { repo: Option, #[arg( long, - help = "Use this known Corgea project id for the result link. Together with a scan id it skips project resolution entirely." + requires = "scan_id", + value_parser = clap::builder::NonEmptyStringValueParser::new(), + help = "Use this known Corgea project id for the result link, skipping project resolution. Requires a scan id, which is what the id then belongs to." )] project_id: Option, }, diff --git a/src/utils/api.rs b/src/utils/api.rs index 47eb208..2acc590 100644 --- a/src/utils/api.rs +++ b/src/utils/api.rs @@ -770,13 +770,23 @@ fn id_to_string(v: &serde_json::Value) -> Option { } /// True when a stored `repo_url` points at exactly `expected_path` (a whole -/// post-host path, already lowercased). Comparing whole paths keeps the -/// backend's `repo_url__icontains` results honest: neither the sibling -/// `acme/api-v2` nor the nested `…/mirrors/acme/api` passes for `acme/api`. -/// Falls back to a normalized compare for a stored bare `acme/api`. -fn repo_url_matches_path(repo_url: &str, expected_path: &str) -> bool { +/// post-host path, already lowercased), on `expected_host` when both carry one. +/// Comparing whole paths keeps the backend's `repo_url__icontains` results +/// honest: neither the sibling `acme/api-v2` nor the nested `…/mirrors/acme/api` +/// passes for `acme/api`. The host compare then separates two repositories that +/// share a path across forges — `github.com/acme/api` is not +/// `gitlab.com/acme/api` — and is skipped when either side is hostless (a bare +/// `--repo acme/api`, or a stored bare `acme/api`), which carries no host to +/// contradict. Falls back to a normalized compare for a stored bare `acme/api`. +fn repo_url_matches_path(repo_url: &str, expected_path: &str, expected_host: Option<&str>) -> bool { if let Some(path) = utils::generic::extract_repo_path(repo_url) { - return path == expected_path; + if path != expected_path { + return false; + } + return match (expected_host, utils::generic::extract_repo_host(repo_url)) { + (Some(expected), Some(actual)) => actual == expected, + _ => true, + }; } let r = repo_url.trim().trim_end_matches('/'); let r = r.strip_suffix(".git").unwrap_or(r).to_lowercase(); @@ -842,6 +852,7 @@ fn fetch_projects_page( pub fn resolve_project_by_repo( url: &str, repo_path: &str, + expected_host: Option<&str>, ) -> Result, Box> { let expected = repo_path.to_lowercase(); let mut page = 1; @@ -857,7 +868,7 @@ pub fn resolve_project_by_repo( let matched = projects.into_iter().find(|p| { p.repo_url .as_deref() - .map(|r| repo_url_matches_path(r, &expected)) + .map(|r| repo_url_matches_path(r, &expected, expected_host)) .unwrap_or(false) }); if matched.is_some() { @@ -918,19 +929,24 @@ pub fn resolve_project( } // --repo may already be a bare `org/repo` (extract_repo_path needs a host - // segment, so it returns None there) -> use the raw value. - let repo_path = match repo_override { - Some(r) => utils::generic::extract_repo_path(r).or_else(|| Some(r.to_string())), - None => { - utils::generic::discover_repo_url().and_then(|u| utils::generic::extract_repo_path(&u)) - } + // segment, so it returns None there) -> use the raw value, and with it no + // host to hold candidates to. + let (repo_path, repo_host) = match repo_override { + Some(r) => ( + utils::generic::extract_repo_path(r).or_else(|| Some(r.to_string())), + utils::generic::extract_repo_host(r), + ), + None => match utils::generic::discover_repo_url() { + Some(u) => ( + utils::generic::extract_repo_path(&u), + utils::generic::extract_repo_host(&u), + ), + None => (None, None), + }, }; - let cwd = - utils::generic::get_current_working_directory().unwrap_or_else(|| "unknown".to_string()); - if let Some(repo_path) = repo_path { - if let Some(project) = resolve_project_by_repo(url, &repo_path)? { + if let Some(project) = resolve_project_by_repo(url, &repo_path, repo_host.as_deref())? { return Ok(ResolvedProject { query_name: project.name, confirmed: true, @@ -938,11 +954,14 @@ pub fn resolve_project( tried_label: format!("repo '{}'", repo_path), }); } - // Explicit --repo vs auto-detected remote changes the unconfirmed fallback. + // Unconfirmed: an explicit --repo queries that path as a name, and an + // auto-detected remote queries exactly what the pre-COR-1577 CLI did + // (repo basename at the worktree root, else the CWD name) so an old or + // not-yet-onboarded backend still resolves. let query_name = if repo_override.is_some() { repo_path.clone() } else { - cwd + utils::generic::determine_project_name(None) }; return Ok(ResolvedProject { query_name, @@ -952,6 +971,8 @@ pub fn resolve_project( }); } + let cwd = + utils::generic::get_current_working_directory().unwrap_or_else(|| "unknown".to_string()); Ok(ResolvedProject { tried_label: format!("directory '{}'", cwd), query_name: cwd, @@ -1640,7 +1661,7 @@ mod tests { let base = spawn_projects_stub( r#"{"status":"ok","projects":[{"id":7,"name":"bohappdev/dotnet-azure-web-tsb","repo_url":"https://github.com/bohappdev/dotnet-azure-web-tsb"}]}"#, ); - let got = resolve_project_by_repo(&base, "bohappdev/dotnet-azure-web-tsb").unwrap(); + let got = resolve_project_by_repo(&base, "bohappdev/dotnet-azure-web-tsb", None).unwrap(); assert_eq!( got.map(|p| p.name).as_deref(), Some("bohappdev/dotnet-azure-web-tsb") @@ -1653,14 +1674,14 @@ mod tests { let base = spawn_projects_stub( r#"{"status":"ok","projects":[{"id":1,"name":"other/repo","repo_url":"https://github.com/other/repo"},{"id":2,"name":"misc/thing","repo_url":"https://github.com/misc/thing"}]}"#, ); - let got = resolve_project_by_repo(&base, "bohappdev/dotnet-azure-web-tsb").unwrap(); + let got = resolve_project_by_repo(&base, "bohappdev/dotnet-azure-web-tsb", None).unwrap(); assert!(got.is_none(), "non-matching projects must be discarded"); } #[test] fn resolve_project_by_repo_empty_projects_is_none() { let base = spawn_projects_stub(r#"{"status":"ok","projects":[]}"#); - assert!(resolve_project_by_repo(&base, "org/repo") + assert!(resolve_project_by_repo(&base, "org/repo", None) .unwrap() .is_none()); } @@ -1670,41 +1691,92 @@ mod tests { // Same repo across scheme/.git/trailing-slash/port variants. assert!(repo_url_matches_path( "https://github.com/acme/api", - "acme/api" + "acme/api", + None )); assert!(repo_url_matches_path( "https://github.com/acme/api.git", - "acme/api" + "acme/api", + None )); - assert!(repo_url_matches_path("git@github.com:acme/api", "acme/api")); - assert!(repo_url_matches_path("acme/api", "acme/api")); + assert!(repo_url_matches_path( + "git@github.com:acme/api", + "acme/api", + None + )); + assert!(repo_url_matches_path("acme/api", "acme/api", None)); // Sibling / prefix repo, and a different org. assert!(!repo_url_matches_path( "https://github.com/acme/api-v2", - "acme/api" + "acme/api", + None )); assert!(!repo_url_matches_path( "https://github.com/notacme/api", - "acme/api" + "acme/api", + None )); // The owner must be top-level on the host: a nested mirror is a // different repository, as is `org/team/repo` for `team/repo`. assert!(!repo_url_matches_path( "https://github.com/mirrors/acme/api", - "acme/api" + "acme/api", + None )); assert!(!repo_url_matches_path( "https://github.com/org/team/repo", - "team/repo" + "team/repo", + None )); // Multi-segment paths compare in full. assert!(repo_url_matches_path( "https://dev.azure.com/org/project/_git/repo", - "org/project/_git/repo" + "org/project/_git/repo", + None )); assert!(repo_url_matches_path( "git@gitlab.com:group/subgroup/repo.git", - "group/subgroup/repo" + "group/subgroup/repo", + None + )); + } + + #[test] + fn repo_url_matches_path_separates_the_same_path_on_two_forges() { + // `?repo_url=acme/api` is hostless, so `icontains` returns the GitLab + // project too; confirming it would list another repo's scans. + assert!(repo_url_matches_path( + "https://github.com/acme/api", + "acme/api", + Some("github.com") + )); + assert!(!repo_url_matches_path( + "https://gitlab.com/acme/api", + "acme/api", + Some("github.com") + )); + // Scheme/userinfo/port variants of the same host still match. + assert!(repo_url_matches_path( + "git@github.com:acme/api.git", + "acme/api", + Some("github.com") + )); + assert!(repo_url_matches_path( + "https://gitlab.acme.com:8443/acme/api", + "acme/api", + Some("gitlab.acme.com") + )); + // Hostless on either side carries no host to contradict: a bare + // `--repo acme/api` still matches, as does a stored bare `acme/api`. + assert!(repo_url_matches_path( + "https://gitlab.com/acme/api", + "acme/api", + None + )); + assert!(repo_url_matches_path( + "acme/api", + "acme/api", + Some("github.com") )); } @@ -1716,7 +1788,7 @@ mod tests { let base = spawn_projects_stub( r#"{"status":"ok","projects":[{"id":9,"name":"acme/api-v2","repo_url":"https://github.com/acme/api-v2"}]}"#, ); - let got = resolve_project_by_repo(&base, "acme/api").unwrap(); + let got = resolve_project_by_repo(&base, "acme/api", None).unwrap(); assert!(got.is_none(), "a prefix sibling repo must not be confirmed"); } @@ -1724,7 +1796,7 @@ mod tests { fn resolve_project_by_repo_404_is_soft_none() { // /projects absent on a very old backend -> soft miss (Ok(None)). let base = spawn_projects_stub_status("404 Not Found", r#"{"message":"not found"}"#); - assert!(resolve_project_by_repo(&base, "org/repo") + assert!(resolve_project_by_repo(&base, "org/repo", None) .unwrap() .is_none()); } @@ -1733,7 +1805,7 @@ mod tests { fn resolve_project_by_repo_server_error_is_hard_err() { // 5xx must surface, not silently fall back to the local-dir project. let base = spawn_projects_stub_status("500 Internal Server Error", r#"{"error":"boom"}"#); - assert!(resolve_project_by_repo(&base, "org/repo").is_err()); + assert!(resolve_project_by_repo(&base, "org/repo", None).is_err()); } #[test] @@ -1743,7 +1815,7 @@ mod tests { // resolve a DIFFERENT project via the CWD-name fallback (COR-1577 // review). let base = spawn_projects_stub("Access denied"); - assert!(resolve_project_by_repo(&base, "org/repo").is_err()); + assert!(resolve_project_by_repo(&base, "org/repo", None).is_err()); } #[test] @@ -1755,10 +1827,29 @@ mod tests { r#"{"status":"ok","total_pages":2,"projects":[{"id":1,"name":"acme/api-v2","repo_url":"https://github.com/acme/api-v2"},{"id":2,"name":"acme/api-gw","repo_url":"https://github.com/acme/api-gw"}]}"#, r#"{"status":"ok","total_pages":2,"projects":[{"id":9,"name":"acme/api","repo_url":"https://github.com/acme/api"}]}"#, ); - let got = resolve_project_by_repo(&base, "acme/api").unwrap(); + let got = resolve_project_by_repo(&base, "acme/api", None).unwrap(); assert_eq!(got.map(|p| p.name).as_deref(), Some("acme/api")); } + #[test] + fn resolve_project_by_repo_rejects_the_same_path_on_another_host() { + // The backend's hostless `icontains` returns the GitLab project for a + // GitHub origin; confirming it would resolve the wrong project. + let base = spawn_projects_stub( + r#"{"status":"ok","projects":[{"id":3,"name":"acme/api","repo_url":"https://gitlab.com/acme/api"}]}"#, + ); + assert!( + resolve_project_by_repo(&base, "acme/api", Some("gitlab.com")) + .unwrap() + .is_some() + ); + assert!( + resolve_project_by_repo(&base, "acme/api", Some("github.com")) + .unwrap() + .is_none() + ); + } + #[test] fn resolve_project_by_repo_stops_at_the_last_page() { // `total_pages: 1` (and its absence, as in the other stubs here) must @@ -1766,7 +1857,7 @@ mod tests { let base = spawn_projects_stub( r#"{"status":"ok","total_pages":1,"projects":[{"id":1,"name":"acme/api-v2","repo_url":"https://github.com/acme/api-v2"}]}"#, ); - assert!(resolve_project_by_repo(&base, "acme/api") + assert!(resolve_project_by_repo(&base, "acme/api", None) .unwrap() .is_none()); } diff --git a/src/utils/generic.rs b/src/utils/generic.rs index 573b453..1fc9299 100644 --- a/src/utils/generic.rs +++ b/src/utils/generic.rs @@ -268,6 +268,19 @@ fn extract_repo_name_from_url(url: &str) -> Option { /// so it is what an equality compare needs. Azure SSH remotes /// (`ssh.dev.azure.com/v3/org/…`, no `_git` segment) remain a known limitation. pub fn extract_repo_path(url: &str) -> Option { + Some(split_remote(url)?[1..].join("/").to_lowercase()) +} + +/// The host of a git remote (`github.com`), lowercased and without userinfo or +/// port. None for a hostless value such as a bare `org/repo` — the same inputs +/// `extract_repo_path` rejects. +pub fn extract_repo_host(url: &str) -> Option { + Some(split_remote(url)?[0].to_lowercase()) +} + +/// Split a git remote into `[host, path segments…]`, dropping scheme, userinfo +/// and port. None when fewer than two path segments follow the host. +fn split_remote(url: &str) -> Option> { let url = url.trim().trim_end_matches('/'); let url = url.strip_suffix(".git").unwrap_or(url); let url = url.rsplit("://").next().unwrap_or(url); @@ -283,10 +296,8 @@ pub fn extract_repo_path(url: &str) -> Option { if segments.len() >= 4 && segments[1].chars().all(|c| c.is_ascii_digit()) { segments.remove(1); } - if segments.len() < 3 { - return None; // need host + at least org + repo - } - Some(segments[1..].join("/").to_lowercase()) + // Need host + at least org + repo. + (segments.len() >= 3).then_some(segments) } pub fn get_env_var_if_exists(var_name: &str) -> Option { diff --git a/tests/list_resolution.rs b/tests/list_resolution.rs index cc029ea..6b5a082 100644 --- a/tests/list_resolution.rs +++ b/tests/list_resolution.rs @@ -178,6 +178,55 @@ fn list_project_name_override() { assert!(stdout.contains("some/name"), "stdout: {stdout}"); } +#[test] +fn list_unconfirmed_falls_back_to_the_repo_name_not_the_checkout_dir() { + // Cloned into `build-123`: on a /projects soft miss (old or not-yet- + // onboarded backend) the query must stay what the pre-COR-1577 CLI sent — + // the repo basename — or a working setup starts missing. (PR #122 review) + let (url, hits) = common::spawn_recording_resolution_stub(Routes { + projects: Some(projects_empty()), + scans: Some(scans_one("dotnet-azure-web-tsb")), + ..Default::default() + }); + let (_tmp, repo) = temp_git_repo("build-123", REMOTE); + let out = run_list(&[], &url, &repo); + assert_eq!( + out.status.code(), + Some(0), + "stderr: {}", + String::from_utf8_lossy(&out.stderr) + ); + let hits = hits.lock().unwrap(); + assert!( + hits.iter() + .any(|h| h.contains("project=dotnet-azure-web-tsb")), + "expected the repo basename, not the checkout dir; hits: {hits:?}" + ); + assert!( + !hits.iter().any(|h| h.contains("project=build-123")), + "the checkout dir name must not be queried; hits: {hits:?}" + ); +} + +#[test] +fn list_project_name_with_no_scans_exits_zero() { + // /scans answers 200-empty both for "project has no scans" and "no such + // project", so an explicit --project-name is the better authority: report + // the empty result, do not claim the project does not exist. (PR #122 + // review) + let url = spawn_stub(projects_empty(), scans_empty(), issues_miss()); + let (_tmp, dir) = temp_plain_dir("whatever"); + let out = run_list(&["--project-name", "some/name"], &url, &dir); + let stdout = String::from_utf8_lossy(&out.stdout); + assert_eq!( + out.status.code(), + Some(0), + "stderr: {}", + String::from_utf8_lossy(&out.stderr) + ); + assert!(stdout.contains("has no scans yet"), "stdout: {stdout}"); +} + #[test] fn list_project_name_and_repo_are_mutually_exclusive() { let (_tmp, dir) = temp_plain_dir("whatever"); diff --git a/tests/wait_resolution.rs b/tests/wait_resolution.rs index 2399c64..98970e2 100644 --- a/tests/wait_resolution.rs +++ b/tests/wait_resolution.rs @@ -241,6 +241,21 @@ fn wait_project_id_flag_skips_resolution() { ); } +#[test] +fn wait_project_id_requires_a_scan_id() { + // Without a scan id the scan is still picked by the resolved project name, + // so a lone --project-id would only relabel the link — pointing at a + // different project than the scan. clap rejects it. (PR #122 review) + let (_tmp, repo) = temp_git_repo("dotnet-azure-web-tsb", REMOTE); + let out = run_wait(&["--project-id", "42"], "http://127.0.0.1:1", &repo); + assert_eq!( + out.status.code(), + Some(2), + "stderr: {}", + String::from_utf8_lossy(&out.stderr) + ); +} + /// `corgea scan semgrep` with a fake `semgrep` on PATH: the post-scan wait gets /// the project id straight from the upload response, so it must resolve nothing /// — no `/projects`, no `/scans` — and still link the id-form URL. From 58027ec9a42aa48d7e826d1f7854df86ef430102 Mon Sep 17 00:00:00 2001 From: Test Date: Wed, 29 Jul 2026 10:22:19 +0200 Subject: [PATCH 12/16] fix(cli): require the /projects envelope and scope SCA to the selector MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - `ProjectsResponse.projects` was `#[serde(default)] Option>`, so a 200 error envelope like `{"status":"error"}` parsed clean, read as "no matches", and took the legacy-name fallback — defeating the hard-failure contract on the neighbouring parse arm. doghouse `@paginated` emits the key on every 200, empty array included (api/decorators.py:228,241,259), so the field is now required and a 200 without it fails the parse. - `--project-name` / `--repo` were accepted with `--sca-issues` and then ignored: the SCA branch returns before any resolution and `get_sca_issues` sent only pagination, so a script asking for one project silently got company-wide findings. `list_sca_issues` does take `project` (api/views/core.py:1179-1195), so the resolved name is now threaded into the request. Only an explicit selector scopes it — unflagged `--sca-issues` keeps returning the company-wide latest scan, since narrowing that default is a separate decision. --- src/list.rs | 17 ++++++++-- src/utils/api.rs | 50 +++++++++++++++++++++++----- tests/list_resolution.rs | 70 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 126 insertions(+), 11 deletions(-) diff --git a/src/list.rs b/src/list.rs index 2d21b49..ca2b671 100644 --- a/src/list.rs +++ b/src/list.rs @@ -30,14 +30,25 @@ pub fn run(config: &Config, args: ListArgs) { println!(); } if sca_issues { - // SCA has no project parameter; the CWD basename is only error copy. - let project_name = - utils::generic::get_current_working_directory().unwrap_or("unknown".to_string()); + // Only an explicit --project-name/--repo scopes the SCA listing: the + // endpoint takes `project`, but unflagged `--sca-issues` has always + // returned the company-wide latest scan and narrowing that silently is + // not this change's to make. Without a selector the CWD basename stays + // what it was — error copy only. + let resolved = (scan_id.is_none() && selector.is_set()) + .then(|| utils::api::resolve_project_or_exit(&config.get_url(), &selector)); + let project_name = resolved + .as_ref() + .map(|r| r.query_name.clone()) + .unwrap_or_else(|| { + utils::generic::get_current_working_directory().unwrap_or("unknown".to_string()) + }); let sca_issues_response = match utils::api::get_sca_issues( &config.get_url(), Some(page.unwrap_or(1)), page_size, scan_id.clone(), + resolved.as_ref().map(|r| r.query_name.as_str()), ) { Ok(response) => response, Err(e) => { diff --git a/src/utils/api.rs b/src/utils/api.rs index 2acc590..dbbc7fd 100644 --- a/src/utils/api.rs +++ b/src/utils/api.rs @@ -745,8 +745,11 @@ pub struct ProjectSummary { #[derive(Deserialize, Debug)] pub struct ProjectsResponse { - #[serde(default)] - pub projects: Option>, + /// Required: doghouse's `@paginated` emits this key on every 200, empty + /// array included (`api/decorators.py:228,241,259`). A 200 without it is + /// not this endpoint answering, so it must fail the parse rather than read + /// as "no matches" and take the legacy-name fallback. + pub projects: Vec, /// Absent on a backend that does not paginate -> treated as a single page. #[serde(default)] pub total_pages: Option, @@ -861,7 +864,7 @@ pub fn resolve_project_by_repo( return Ok(None); }; let total_pages = parsed.total_pages.unwrap_or(1); - let projects = parsed.projects.unwrap_or_default(); + let projects = parsed.projects; if projects.is_empty() { return Ok(None); } @@ -909,6 +912,14 @@ pub struct ProjectSelector { pub repo: Option, } +impl ProjectSelector { + /// True when the caller named a project or repo explicitly, rather than + /// leaving it to auto-detection. + pub fn is_set(&self) -> bool { + self.name.is_some() || self.repo.is_some() + } +} + /// Resolve which project `list`/`wait` should query: `--project-name` verbatim, /// else the repo path from `--repo` or the discovered remote. Unconfirmed, an /// explicit `--repo` queries that path as a name and everything else falls back @@ -1109,6 +1120,7 @@ pub fn get_sca_issues( page: Option, page_size: Option, scan_id: Option, + project: Option<&str>, ) -> Result> { let client = http_client(); let mut query_params = vec![]; @@ -1118,6 +1130,11 @@ pub fn get_sca_issues( if let Some(page_size) = page_size { query_params.push(("page_size", page_size.to_string())); } + // Scopes the scan-less route to one project (doghouse `list_sca_issues` + // reads `project`); the scan route already keys off the scan. + if let Some(project) = project { + query_params.push(("project", project.to_string())); + } let endpoint = if let Some(scan_id) = scan_id { format!("{}{}/scan/{}/issues/sca", url, API_BASE, scan_id) @@ -1181,11 +1198,18 @@ pub fn get_all_sca_issues( let mut current_page: u32 = 1; loop { - let response = - match get_sca_issues(url, Some(current_page as u16), Some(30), scan_id.clone()) { - Ok(response) => response, - Err(e) => return Err(format!("Failed to get SCA issues: {}", e).into()), - }; + // No project scope: every caller passes a scan id, which selects the + // scan on its own. + let response = match get_sca_issues( + url, + Some(current_page as u16), + Some(30), + scan_id.clone(), + None, + ) { + Ok(response) => response, + Err(e) => return Err(format!("Failed to get SCA issues: {}", e).into()), + }; if response.issues.is_empty() { break; @@ -1831,6 +1855,16 @@ mod tests { assert_eq!(got.map(|p| p.name).as_deref(), Some("acme/api")); } + #[test] + fn resolve_project_by_repo_missing_projects_key_is_hard_err() { + // `@paginated` always emits `projects`, empty array included, so a 200 + // without it is an error envelope or a foreign responder. Reading it as + // "no matches" would take the legacy-name fallback and quietly query a + // different project (PR #122 review). + let base = spawn_projects_stub(r#"{"status":"error","message":"boom"}"#); + assert!(resolve_project_by_repo(&base, "org/repo", None).is_err()); + } + #[test] fn resolve_project_by_repo_rejects_the_same_path_on_another_host() { // The backend's hostless `icontains` returns the GitLab project for a diff --git a/tests/list_resolution.rs b/tests/list_resolution.rs index 6b5a082..28f58b2 100644 --- a/tests/list_resolution.rs +++ b/tests/list_resolution.rs @@ -227,6 +227,76 @@ fn list_project_name_with_no_scans_exits_zero() { assert!(stdout.contains("has no scans yet"), "stdout: {stdout}"); } +#[test] +fn list_sca_issues_scopes_to_an_explicit_project_name() { + // The flags are offered on every `list` mode, so with --sca-issues they + // must actually scope the request rather than silently return every + // project's findings. `list_sca_issues` reads `project`. (PR #122 review) + let (url, hits) = common::spawn_recording_http_stub(|path| { + if path.starts_with("/api/v1/verify") { + ("200 OK", r#"{"status":"ok"}"#.to_string()) + } else if path.starts_with("/api/v1/issues/sca") { + ( + "200 OK", + r#"{"status":"ok","page":1,"total_pages":1,"total_issues":0,"issues":[]}"# + .to_string(), + ) + } else { + ("404 Not Found", common::NOT_FOUND_JSON.to_string()) + } + }); + let (_tmp, dir) = temp_plain_dir("whatever"); + let out = run_list(&["--sca-issues", "--project-name", "some/name"], &url, &dir); + assert_eq!( + out.status.code(), + Some(0), + "stderr: {}", + String::from_utf8_lossy(&out.stderr) + ); + let hits = hits.lock().unwrap(); + assert!( + hits.iter() + .any(|h| h.starts_with("/api/v1/issues/sca") && h.contains("project=some%2Fname")), + "the SCA request must carry the named project; hits: {hits:?}" + ); +} + +#[test] +fn list_sca_issues_without_a_selector_stays_unscoped() { + // Unflagged --sca-issues has always returned the company-wide latest scan; + // adding the flags must not silently narrow it. + let (url, hits) = common::spawn_recording_http_stub(|path| { + if path.starts_with("/api/v1/verify") { + ("200 OK", r#"{"status":"ok"}"#.to_string()) + } else if path.starts_with("/api/v1/issues/sca") { + ( + "200 OK", + r#"{"status":"ok","page":1,"total_pages":1,"total_issues":0,"issues":[]}"# + .to_string(), + ) + } else { + ("404 Not Found", common::NOT_FOUND_JSON.to_string()) + } + }); + let (_tmp, repo) = temp_git_repo("dotnet-azure-web-tsb", REMOTE); + let out = run_list(&["--sca-issues"], &url, &repo); + assert_eq!( + out.status.code(), + Some(0), + "stderr: {}", + String::from_utf8_lossy(&out.stderr) + ); + let hits = hits.lock().unwrap(); + assert!( + !hits.iter().any(|h| h.contains("project=")), + "no selector means no project scope; hits: {hits:?}" + ); + assert!( + !hits.iter().any(|h| h.starts_with("/api/v1/projects")), + "and no resolution round trip; hits: {hits:?}" + ); +} + #[test] fn list_project_name_and_repo_are_mutually_exclusive() { let (_tmp, dir) = temp_plain_dir("whatever"); From b1ddd18f36ec936e34183a58d835497ce85c0ec3 Mon Sep 17 00:00:00 2001 From: Test Date: Wed, 29 Jul 2026 10:33:58 +0200 Subject: [PATCH 13/16] fix(cli): keep bare multi-segment repo slugs whole, trim --project-name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - `split_remote` read the first segment of any 3+ segment value as a host, so a bare GitLab `--repo group/subgroup/repo` resolved as host `group` + path `subgroup/repo` and queried the wrong project. Without a scheme or userinfo the leading segment now counts as a host only if it looks like one, so bare paths keep every segment (and `.git`/trailing `/` are trimmed on that route too). - `--project-name foo/` reached `?project=` verbatim, which the backend matches exactly, so it missed the project `foo`; the trim in wait.rs runs only after a scan is already selected. Normalized in `resolve_project`, where every caller passes through, and a slash-only value is rejected. - The no-repository fallback sent the raw directory basename, but `determine_project_name` sanitized it before this PR — a project onboarded from `my app` is stored as `my_app`. It now uses the same helper as the repo fallback; the raw name stays as the error label. Test fixes in the same pass: - `list_uses_canonical_name_from_repo` and `list_issues_shows_repo_resolved_issue` asserted only on stdout, which the stub supplies whatever the CLI sends, from a checkout named after the repo. Both now run from `build-123` and assert the project actually sent to /scans and /issues — the PR's central claim was previously untested. - `wait_project_name_trailing_slash_is_trimmed` likewise only checked the printed URL, which is trimmed separately; it now asserts the query. --- src/utils/api.rs | 24 ++++++++---- src/utils/generic.rs | 34 ++++++++++++---- tests/list_resolution.rs | 83 +++++++++++++++++++++++++++++++++++++--- tests/wait_resolution.rs | 28 +++++++++++++- 4 files changed, 145 insertions(+), 24 deletions(-) diff --git a/src/utils/api.rs b/src/utils/api.rs index dbbc7fd..b1291d5 100644 --- a/src/utils/api.rs +++ b/src/utils/api.rs @@ -931,6 +931,12 @@ pub fn resolve_project( ) -> Result> { let repo_override = selector.repo.as_deref(); if let Some(name) = selector.name.as_deref() { + // Normalize before it reaches `?project=`, which the backend matches + // exactly: `--project-name foo/` must not miss the project `foo`. + let name = name.trim().trim_matches('/'); + if name.is_empty() { + return Err("--project-name must name a project".into()); + } return Ok(ResolvedProject { query_name: name.to_string(), confirmed: false, @@ -939,14 +945,14 @@ pub fn resolve_project( }); } - // --repo may already be a bare `org/repo` (extract_repo_path needs a host - // segment, so it returns None there) -> use the raw value, and with it no - // host to hold candidates to. + // --repo may be a bare path (`org/repo`, or a GitLab `group/subgroup/repo`) + // rather than a URL; `extract_repo_path` returns None for those, so the + // whole value is the path and there is no host to hold candidates to. let (repo_path, repo_host) = match repo_override { - Some(r) => ( - utils::generic::extract_repo_path(r).or_else(|| Some(r.to_string())), - utils::generic::extract_repo_host(r), - ), + Some(r) => match utils::generic::extract_repo_path(r) { + Some(path) => (Some(path), utils::generic::extract_repo_host(r)), + None => (Some(utils::generic::strip_git_suffix(r).to_string()), None), + }, None => match utils::generic::discover_repo_url() { Some(u) => ( utils::generic::extract_repo_path(&u), @@ -986,7 +992,9 @@ pub fn resolve_project( utils::generic::get_current_working_directory().unwrap_or_else(|| "unknown".to_string()); Ok(ResolvedProject { tried_label: format!("directory '{}'", cwd), - query_name: cwd, + // Same legacy query as above: the sanitized basename here, since a + // directory named `my app` was onboarded as `my_app`. + query_name: utils::generic::determine_project_name(None), confirmed: false, project_id: None, }) diff --git a/src/utils/generic.rs b/src/utils/generic.rs index 1fc9299..536f0ec 100644 --- a/src/utils/generic.rs +++ b/src/utils/generic.rs @@ -279,16 +279,19 @@ pub fn extract_repo_host(url: &str) -> Option { } /// Split a git remote into `[host, path segments…]`, dropping scheme, userinfo -/// and port. None when fewer than two path segments follow the host. +/// and port. None when fewer than two path segments follow the host, or when +/// the value carries no host at all. fn split_remote(url: &str) -> Option> { - let url = url.trim().trim_end_matches('/'); - let url = url.strip_suffix(".git").unwrap_or(url); - let url = url.rsplit("://").next().unwrap_or(url); + let url = strip_git_suffix(url); + let (had_scheme, url) = match url.split_once("://") { + Some((_, rest)) => (true, rest), + None => (false, url), + }; // Drop userinfo (`git@`, `oauth2:token@`) so it is never read as the host. let host_end = url.find('/').unwrap_or(url.len()); - let url = match url[..host_end].rfind('@') { - Some(at) => &url[at + 1..], - None => url, + let (had_userinfo, url) = match url[..host_end].rfind('@') { + Some(at) => (true, &url[at + 1..]), + None => (false, url), }; // URL forms split host from path on '/', scp-like `git@host:org/repo` on ':'. let mut segments: Vec<&str> = url.split(['/', ':']).filter(|s| !s.is_empty()).collect(); @@ -297,7 +300,22 @@ fn split_remote(url: &str) -> Option> { segments.remove(1); } // Need host + at least org + repo. - (segments.len() >= 3).then_some(segments) + if segments.len() < 3 { + return None; + } + // With no scheme or userinfo to mark a URL, the leading segment is a host + // only if it looks like one — otherwise this is a bare multi-segment path + // such as a GitLab `group/subgroup/repo`, whose every segment is the path. + if !had_scheme && !had_userinfo && !segments[0].contains('.') && segments[0] != "localhost" { + return None; + } + Some(segments) +} + +/// Trim surrounding space, a trailing `/`, and a `.git` suffix. +pub fn strip_git_suffix(url: &str) -> &str { + let url = url.trim().trim_end_matches('/'); + url.strip_suffix(".git").unwrap_or(url) } pub fn get_env_var_if_exists(var_name: &str) -> Option { diff --git a/tests/list_resolution.rs b/tests/list_resolution.rs index 28f58b2..b5b2cc0 100644 --- a/tests/list_resolution.rs +++ b/tests/list_resolution.rs @@ -46,8 +46,16 @@ fn run_list(args: &[&str], url: &str, cwd: &Path) -> Output { #[test] fn list_uses_canonical_name_from_repo() { - let url = spawn_stub(projects_match(), scans_one(CANON), issues_one()); - let (_tmp, repo) = temp_git_repo("dotnet-azure-web-tsb", REMOTE); + // The checkout is `build-123`, so the canonical name can only come from + // resolution — and the assertion is on the query the CLI SENT, since the + // stub answers every /scans the same way and stdout alone would stay green + // through a regression. (PR #122 review) + let (url, hits) = common::spawn_recording_resolution_stub(routes( + projects_match(), + scans_one(CANON), + issues_one(), + )); + let (_tmp, repo) = temp_git_repo("build-123", REMOTE); let out = run_list(&[], &url, &repo); let stdout = String::from_utf8_lossy(&out.stdout); assert_eq!( @@ -56,19 +64,29 @@ fn list_uses_canonical_name_from_repo() { "stderr: {}", String::from_utf8_lossy(&out.stderr) ); - // Project column shows the canonical org/repo, proving resolution, not the - // dir basename, drove the listing. assert!( stdout.contains("bohappdev/dotnet-azure-web-tsb"), "stdout: {stdout}" ); assert!(stdout.contains("scan-123"), "stdout: {stdout}"); + let hits = hits.lock().unwrap(); + assert!( + hits.iter().any(|h| h.starts_with("/api/v1/scans?") + && h.contains("project=bohappdev%2Fdotnet-azure-web-tsb")), + "the canonical project must drive /scans; hits: {hits:?}" + ); } #[test] fn list_issues_shows_repo_resolved_issue() { - let url = spawn_stub(projects_match(), scans_one(CANON), issues_one()); - let (_tmp, repo) = temp_git_repo("dotnet-azure-web-tsb", REMOTE); + // Same shape as above: `build-123` plus an assertion on the sent query, so + // a fallback to the checkout name cannot pass. + let (url, hits) = common::spawn_recording_resolution_stub(routes( + projects_match(), + scans_one(CANON), + issues_one(), + )); + let (_tmp, repo) = temp_git_repo("build-123", REMOTE); let out = run_list(&["--issues"], &url, &repo); let stdout = String::from_utf8_lossy(&out.stdout); assert_eq!( @@ -78,6 +96,14 @@ fn list_issues_shows_repo_resolved_issue() { String::from_utf8_lossy(&out.stderr) ); assert!(stdout.contains("issue-abc"), "stdout: {stdout}"); + let hits = hits.lock().unwrap(); + // `get_scan_issues` builds this target by hand, so the slash arrives + // unencoded — unlike the `.query()`-built /scans target above. + assert!( + hits.iter().any(|h| h.starts_with("/api/v1/issues?") + && h.contains("project=bohappdev/dotnet-azure-web-tsb")), + "the canonical project must drive /issues; hits: {hits:?}" + ); } #[test] @@ -208,6 +234,51 @@ fn list_unconfirmed_falls_back_to_the_repo_name_not_the_checkout_dir() { ); } +#[test] +fn list_repo_flag_keeps_every_segment_of_a_bare_slug() { + // A GitLab `group/subgroup/repo` has no host: all three segments are the + // path. Reading `group` as the host would query `subgroup/repo` and hit a + // different project. (PR #122 review) + let (url, hits) = common::spawn_recording_resolution_stub(Routes { + projects: Some(projects_empty()), + scans: Some(scans_empty()), + ..Default::default() + }); + let (_tmp, dir) = temp_plain_dir("unrelated-dir"); + let out = run_list(&["--repo", "group/subgroup/repo.git"], &url, &dir); + assert_eq!(out.status.code(), Some(1), "an empty miss still exits 1"); + let hits = hits.lock().unwrap(); + assert!( + hits.iter() + .any(|h| h.starts_with("/api/v1/projects?repo_url=group%2Fsubgroup%2Frepo&")), + "every segment must survive, .git trimmed; hits: {hits:?}" + ); +} + +#[test] +fn list_no_remote_falls_back_to_the_sanitized_dir_name() { + // `determine_project_name` sanitized the basename before this PR, so a + // project onboarded from `my app` is stored as `my_app` — query that, not + // the raw name. (PR #122 review) + let (url, hits) = common::spawn_recording_resolution_stub(Routes { + scans: Some(scans_one("my_app")), + ..Default::default() + }); + let (_tmp, dir) = temp_plain_dir("my app"); + let out = run_list(&[], &url, &dir); + assert_eq!( + out.status.code(), + Some(0), + "stderr: {}", + String::from_utf8_lossy(&out.stderr) + ); + let hits = hits.lock().unwrap(); + assert!( + hits.iter().any(|h| h.contains("project=my_app")), + "expected the sanitized dir name; hits: {hits:?}" + ); +} + #[test] fn list_project_name_with_no_scans_exits_zero() { // /scans answers 200-empty both for "project has no scans" and "no such diff --git a/tests/wait_resolution.rs b/tests/wait_resolution.rs index 98970e2..c8c5206 100644 --- a/tests/wait_resolution.rs +++ b/tests/wait_resolution.rs @@ -144,8 +144,11 @@ fn wait_project_name_override() { #[test] fn wait_project_name_trailing_slash_is_trimmed() { - // A slash-only or trailing-slash name would build `/project/foo//?scan_id=`. - let url = spawn_stub(projects_empty(), scans_one("foo")); + // The trailing slash must go before the name reaches `?project=`, which the + // backend matches exactly — asserting only the printed URL would pass even + // if `project=foo%2F` were sent. (PR #122 review) + let (url, hits) = + common::spawn_recording_resolution_stub(routes(projects_empty(), scans_one("foo"))); let (_tmp, dir) = temp_plain_dir("whatever"); let out = run_wait(&["--project-name", "foo/"], &url, &dir); let stdout = String::from_utf8_lossy(&out.stdout); @@ -159,6 +162,27 @@ fn wait_project_name_trailing_slash_is_trimmed() { stdout.contains("/project/foo?scan_id=scan-123"), "stdout: {stdout}" ); + let hits = hits.lock().unwrap(); + // `project` is the last query param, so `ends_with` also proves the value + // is not the un-trimmed `foo%2F`. + assert!( + hits.iter() + .any(|h| h.starts_with("/api/v1/scans?") && h.ends_with("project=foo")), + "the scan listing must be queried for `foo`; hits: {hits:?}" + ); +} + +#[test] +fn wait_project_name_slash_only_is_rejected() { + let url = common::spawn_resolution_stub(scan_routes()); + let (_tmp, dir) = temp_plain_dir("whatever"); + let out = run_wait(&["--project-name", "/"], &url, &dir); + let stderr = String::from_utf8_lossy(&out.stderr); + assert_eq!(out.status.code(), Some(1), "stderr: {stderr}"); + assert!( + stderr.contains("--project-name must name a project"), + "stderr: {stderr}" + ); } #[test] From fe5b050f10303707ceb433d88d66792e6603aa2f Mon Sep 17 00:00:00 2001 From: Test Date: Wed, 29 Jul 2026 10:46:25 +0200 Subject: [PATCH 14/16] fix(cli): mark network remotes by scheme/userinfo/scp colon, not a dot MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous heuristic read a dotted leading segment as a host, which truncates a GitLab namespace that legitimately contains one: `--repo my.group/sub/repo` resolved as host `my.group` + path `sub/repo` and could never find the intended project. Only a scheme, userinfo, or an scp-style colon before the first slash now marks a value as a network remote; anything else is a bare path kept whole. Stored `repo_url`s are unaffected — doghouse `normalize_repo_url` rewrites `git@host:path` to `https://host/path` and never stores a schemeless value (heeler/models.py:224-232). Adds unit coverage for `extract_repo_host` and for bare-vs-marked paths, plus an e2e assertion on the request target for `my.group/sub/repo`. --- src/utils/generic.rs | 52 ++++++++++++++++++++++++++++++++++++---- tests/list_resolution.rs | 21 ++++++++++++++++ 2 files changed, 69 insertions(+), 4 deletions(-) diff --git a/src/utils/generic.rs b/src/utils/generic.rs index 536f0ec..338d58e 100644 --- a/src/utils/generic.rs +++ b/src/utils/generic.rs @@ -293,6 +293,9 @@ fn split_remote(url: &str) -> Option> { Some(at) => (true, &url[at + 1..]), None => (false, url), }; + // A colon before the first '/' is the scp-style `host:org/repo` separator. + let first_slash = url.find('/').unwrap_or(url.len()); + let had_scp_colon = url[..first_slash].contains(':'); // URL forms split host from path on '/', scp-like `git@host:org/repo` on ':'. let mut segments: Vec<&str> = url.split(['/', ':']).filter(|s| !s.is_empty()).collect(); // segments[0] is the host; an all-digit segment right after it is a port. @@ -303,10 +306,11 @@ fn split_remote(url: &str) -> Option> { if segments.len() < 3 { return None; } - // With no scheme or userinfo to mark a URL, the leading segment is a host - // only if it looks like one — otherwise this is a bare multi-segment path - // such as a GitLab `group/subgroup/repo`, whose every segment is the path. - if !had_scheme && !had_userinfo && !segments[0].contains('.') && segments[0] != "localhost" { + // A scheme, userinfo or scp colon is what marks a network remote. Without + // one this is a bare path — a GitLab `group/subgroup/repo`, whose namespace + // may itself contain dots — so every segment belongs to the path and there + // is no host to take. + if !had_scheme && !had_userinfo && !had_scp_colon { return None; } Some(segments) @@ -574,4 +578,44 @@ mod tests { assert_eq!(extract_repo_path(""), None); assert_eq!(extract_repo_path("github.com"), None); // host only } + + #[test] + fn extract_repo_path_leaves_unmarked_bare_paths_alone() { + // Nothing marks these as a network remote, so they are paths in full — + // the caller keeps the value verbatim. A GitLab namespace may contain + // dots, so a dotted leading segment is no evidence of a host. + assert_eq!(extract_repo_path("group/subgroup/repo"), None); + assert_eq!(extract_repo_path("my.group/sub/repo"), None); + assert_eq!(extract_repo_host("my.group/sub/repo"), None); + // A scheme, userinfo or scp colon still marks one. + assert_eq!( + extract_repo_path("https://my.group/sub/repo").as_deref(), + Some("sub/repo") + ); + assert_eq!( + extract_repo_path("git@my.group:sub/repo").as_deref(), + Some("sub/repo") + ); + } + + #[test] + fn extract_repo_host_reads_the_host_from_marked_remotes() { + assert_eq!( + extract_repo_host("https://github.com/acme/api").as_deref(), + Some("github.com") + ); + assert_eq!( + extract_repo_host("git@gitlab.com:group/subgroup/repo.git").as_deref(), + Some("gitlab.com") + ); + // Port and userinfo are not part of the host. + assert_eq!( + extract_repo_host("https://git.example.com:8443/org/repo").as_deref(), + Some("git.example.com") + ); + assert_eq!( + extract_repo_host("https://oauth2:tok@gitlab.com/org/repo").as_deref(), + Some("gitlab.com") + ); + } } diff --git a/tests/list_resolution.rs b/tests/list_resolution.rs index b5b2cc0..66bc152 100644 --- a/tests/list_resolution.rs +++ b/tests/list_resolution.rs @@ -255,6 +255,27 @@ fn list_repo_flag_keeps_every_segment_of_a_bare_slug() { ); } +#[test] +fn list_repo_flag_keeps_a_dotted_bare_namespace_whole() { + // GitLab namespaces may contain dots, so a dotted leading segment is no + // evidence of a host: `my.group/sub/repo` is a path in full. (PR #122 + // review) + let (url, hits) = common::spawn_recording_resolution_stub(Routes { + projects: Some(projects_empty()), + scans: Some(scans_empty()), + ..Default::default() + }); + let (_tmp, dir) = temp_plain_dir("unrelated-dir"); + let out = run_list(&["--repo", "my.group/sub/repo"], &url, &dir); + assert_eq!(out.status.code(), Some(1), "an empty miss still exits 1"); + let hits = hits.lock().unwrap(); + assert!( + hits.iter() + .any(|h| h.starts_with("/api/v1/projects?repo_url=my.group%2Fsub%2Frepo&")), + "the dotted namespace must not be read as a host; hits: {hits:?}" + ); +} + #[test] fn list_no_remote_falls_back_to_the_sanitized_dir_name() { // `determine_project_name` sanitized the basename before this PR, so a From 5069de0908f6f77552f882d25cdc23156913098a Mon Sep 17 00:00:00 2001 From: Test Date: Wed, 29 Jul 2026 10:56:34 +0200 Subject: [PATCH 15/16] fix(cli): encode the issues query, fail closed on a truncated page walk MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - `get_scan_issues` interpolated the project name straight into the query string, so `--project-name 'foo&bar'` sent `?project=foo&bar&page=1` and the server read the project as `foo` — returning another project's issues under the name the caller asked for. Built with `query` now, like `query_scan_list` and `get_sca_issues`. - Hitting PROJECTS_MAX_PAGES returned `Ok(None)`, which the caller reads as a clean miss and answers with the legacy-name fallback — so a repo on page 21+ could silently list a different same-basename project and exit 0. A truncated search is now an error naming the reported page count. Also locks in the SSH-config host alias (`corp-github:org/repo`) that the scp-colon rule from fe5b050 already handles: no scheme, no userinfo, no dot, but the colon before the first slash still marks it a remote. --- src/utils/api.rs | 63 ++++++++++++++++++++++++++-------------- src/utils/generic.rs | 10 +++++++ tests/list_resolution.rs | 29 ++++++++++++++++-- 3 files changed, 78 insertions(+), 24 deletions(-) diff --git a/src/utils/api.rs b/src/utils/api.rs index b1291d5..03be728 100644 --- a/src/utils/api.rs +++ b/src/utils/api.rs @@ -471,27 +471,29 @@ pub fn get_scan_issues( page_size: Option, scan_id: Option, ) -> Result> { - let mut seperator = "?"; - let mut url = match scan_id { - Some(scan_id) => format!("{}{}/scan/{}/issues", url, API_BASE, scan_id), - None => { - seperator = "&"; - format!("{}{}/issues?project={}", url, API_BASE, project) - } + // Built with `query`, not `format!`: a project name is user- and + // server-supplied, so an `&`/`#`/`?` in it would otherwise split the query + // and address a different project. + let (url, mut query_params) = match scan_id { + Some(scan_id) => ( + format!("{}{}/scan/{}/issues", url, API_BASE, scan_id), + vec![], + ), + None => ( + format!("{}{}/issues", url, API_BASE), + vec![("project", project.to_string())], + ), }; if let Some(p) = page { - url.push_str(&format!("{}page={}", seperator, p)); - } - if let Some(p_size) = page_size { - url.push_str(&format!("&page_size={}", p_size)); - } else { - url.push_str("&page_size=30"); + query_params.push(("page", p.to_string())); } + query_params.push(("page_size", page_size.unwrap_or(30).to_string())); let client = http_client(); debug(&format!("Sending request to URL: {}", url)); + debug(&format!("Query params: {:?}", query_params)); - let response = match client.get(&url).send() { + let response = match client.get(&url).query(&query_params).send() { Ok(res) => { check_for_warnings(res.headers(), res.status()); res @@ -877,15 +879,19 @@ pub fn resolve_project_by_repo( if matched.is_some() { return Ok(matched); } - if page >= total_pages.min(PROJECTS_MAX_PAGES) { - if total_pages > PROJECTS_MAX_PAGES { - debug(&format!( - "Gave up after {} /projects pages ({} reported)", - PROJECTS_MAX_PAGES, total_pages - )); - } + if page >= total_pages { return Ok(None); } + // Every reported page was NOT searched, so this is not a clean miss: + // saying so would send the caller to the legacy-name fallback, which + // can list a different same-basename project and exit 0. + if page >= PROJECTS_MAX_PAGES { + return Err(format!( + "/projects reported {} pages; refusing to guess after searching {}", + total_pages, PROJECTS_MAX_PAGES + ) + .into()); + } page += 1; } } @@ -1863,6 +1869,21 @@ mod tests { assert_eq!(got.map(|p| p.name).as_deref(), Some("acme/api")); } + #[test] + fn resolve_project_by_repo_truncated_search_is_hard_err() { + // The ceiling stops the walk before every reported page was searched, + // so "no match" would be a guess — and the caller acts on it by + // querying the legacy name, which can list a different project. + let base = spawn_projects_stub( + r#"{"status":"ok","total_pages":999,"projects":[{"id":1,"name":"acme/api-v2","repo_url":"https://github.com/acme/api-v2"}]}"#, + ); + let err = resolve_project_by_repo(&base, "acme/api", None).unwrap_err(); + assert!( + err.to_string().contains("999 pages"), + "error should name the reported page count: {err}" + ); + } + #[test] fn resolve_project_by_repo_missing_projects_key_is_hard_err() { // `@paginated` always emits `projects`, empty array included, so a 200 diff --git a/src/utils/generic.rs b/src/utils/generic.rs index 338d58e..04c11a9 100644 --- a/src/utils/generic.rs +++ b/src/utils/generic.rs @@ -596,6 +596,16 @@ mod tests { extract_repo_path("git@my.group:sub/repo").as_deref(), Some("sub/repo") ); + // An SSH-config host alias carries no userinfo and no dot, but the + // colon before the first slash still marks it scp-style. + assert_eq!( + extract_repo_path("corp-github:bohappdev/dotnet-azure-web-tsb.git").as_deref(), + Some("bohappdev/dotnet-azure-web-tsb") + ); + assert_eq!( + extract_repo_host("corp-github:bohappdev/dotnet-azure-web-tsb.git").as_deref(), + Some("corp-github") + ); } #[test] diff --git a/tests/list_resolution.rs b/tests/list_resolution.rs index 66bc152..2de47bd 100644 --- a/tests/list_resolution.rs +++ b/tests/list_resolution.rs @@ -97,15 +97,38 @@ fn list_issues_shows_repo_resolved_issue() { ); assert!(stdout.contains("issue-abc"), "stdout: {stdout}"); let hits = hits.lock().unwrap(); - // `get_scan_issues` builds this target by hand, so the slash arrives - // unencoded — unlike the `.query()`-built /scans target above. assert!( hits.iter().any(|h| h.starts_with("/api/v1/issues?") - && h.contains("project=bohappdev/dotnet-azure-web-tsb")), + && h.contains("project=bohappdev%2Fdotnet-azure-web-tsb")), "the canonical project must drive /issues; hits: {hits:?}" ); } +#[test] +fn list_issues_percent_encodes_the_project_name() { + // A project name is user- and server-supplied; interpolated raw, an `&` + // would split the query and address the project `foo` instead. (PR #122 + // review) + let (url, hits) = common::spawn_recording_resolution_stub(routes( + projects_empty(), + scans_empty(), + issues_one(), + )); + let (_tmp, dir) = temp_plain_dir("whatever"); + let out = run_list(&["--issues", "--project-name", "foo&bar#baz"], &url, &dir); + assert_eq!( + out.status.code(), + Some(0), + "stderr: {}", + String::from_utf8_lossy(&out.stderr) + ); + let hits = hits.lock().unwrap(); + assert!( + hits.iter().any(|h| h.contains("project=foo%26bar%23baz")), + "the delimiters must be encoded, not split the query; hits: {hits:?}" + ); +} + #[test] fn list_repo_flag_resolves_from_flag_not_remote() { // Records every request target so we can prove the slug came from --repo. From ea1e811688ba4373b4df38251c951d1c519a6822 Mon Sep 17 00:00:00 2001 From: Test Date: Wed, 29 Jul 2026 11:10:06 +0200 Subject: [PATCH 16/16] fix(cli): make the repo host a tie-breaker, not a match gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Requiring the origin host to equal the stored project's host broke every SSH-config alias remote — including this repository's own `git@github.com-corgea:Corgea/cli.git`, whose host is `github.com-corgea` and never equals the stored `github.com`. The exact full-path match was discarded and resolution fell back to the checkout basename, leaving COR-1577 unfixed for precisely the enterprise setups it targets. The host now only settles ties, which is all it was ever for: - a candidate on our host wins immediately; - a lone path match is accepted whatever its host, since there is nothing to disambiguate; - several path matches with none on our host is genuinely ambiguous and errors, naming the competing URLs and pointing at --project-name, rather than flipping a coin or falling back to a third project. The cross-forge collision the host check was added for is still caught: with `github.com/acme/api` and `gitlab.com/acme/api` both present, the origin host picks, and an unrelated origin gets the error. --- src/utils/api.rs | 193 +++++++++++++++++++++------------------ tests/list_resolution.rs | 35 +++++++ 2 files changed, 139 insertions(+), 89 deletions(-) diff --git a/src/utils/api.rs b/src/utils/api.rs index 03be728..a6ea454 100644 --- a/src/utils/api.rs +++ b/src/utils/api.rs @@ -775,29 +775,24 @@ fn id_to_string(v: &serde_json::Value) -> Option { } /// True when a stored `repo_url` points at exactly `expected_path` (a whole -/// post-host path, already lowercased), on `expected_host` when both carry one. -/// Comparing whole paths keeps the backend's `repo_url__icontains` results -/// honest: neither the sibling `acme/api-v2` nor the nested `…/mirrors/acme/api` -/// passes for `acme/api`. The host compare then separates two repositories that -/// share a path across forges — `github.com/acme/api` is not -/// `gitlab.com/acme/api` — and is skipped when either side is hostless (a bare -/// `--repo acme/api`, or a stored bare `acme/api`), which carries no host to -/// contradict. Falls back to a normalized compare for a stored bare `acme/api`. -fn repo_url_matches_path(repo_url: &str, expected_path: &str, expected_host: Option<&str>) -> bool { +/// post-host path, already lowercased). Comparing whole paths keeps the +/// backend's `repo_url__icontains` results honest: neither the sibling +/// `acme/api-v2` nor the nested `…/mirrors/acme/api` passes for `acme/api`. +/// Falls back to a normalized compare for a stored bare `acme/api`. +fn repo_url_matches_path(repo_url: &str, expected_path: &str) -> bool { if let Some(path) = utils::generic::extract_repo_path(repo_url) { - if path != expected_path { - return false; - } - return match (expected_host, utils::generic::extract_repo_host(repo_url)) { - (Some(expected), Some(actual)) => actual == expected, - _ => true, - }; + return path == expected_path; } let r = repo_url.trim().trim_end_matches('/'); let r = r.strip_suffix(".git").unwrap_or(r).to_lowercase(); r == expected_path } +/// True when a candidate is stored on exactly `expected_host`. +fn repo_url_on_host(repo_url: &str, expected_host: &str) -> bool { + utils::generic::extract_repo_host(repo_url).as_deref() == Some(expected_host) +} + /// One page of GET /api/v1/projects?repo_url=… /// /// `Ok(None)` only for a 404 (a backend without the endpoint). A 5xx or a body @@ -852,14 +847,23 @@ fn fetch_projects_page( /// re-checked against the path here — on such a backend none match, and the /// caller falls back to the CWD-name path. /// -/// `Err` for hard failures (network/auth/5xx/unparseable body); a clean "no -/// match" (or a 404 from a backend without the endpoint) is a soft `Ok(None)`. +/// The host is a tie-breaker, not a gate: it settles which of several +/// same-path candidates is ours (`github.com/acme/api` is not +/// `gitlab.com/acme/api`), but a lone path match is accepted whatever its +/// host — an SSH-config alias origin (`corp-github:acme/api`) never matches +/// the stored `github.com` and must still resolve. Several path matches with +/// none on our host is genuinely ambiguous and errors rather than guessing. +/// +/// `Err` for hard failures (network/auth/5xx/unparseable body/ambiguity); a +/// clean "no match" (or a 404 from a backend without the endpoint) is a soft +/// `Ok(None)`. pub fn resolve_project_by_repo( url: &str, repo_path: &str, expected_host: Option<&str>, ) -> Result, Box> { let expected = repo_path.to_lowercase(); + let mut candidates: Vec = Vec::new(); let mut page = 1; loop { let Some(parsed) = fetch_projects_page(url, repo_path, page)? else { @@ -868,19 +872,23 @@ pub fn resolve_project_by_repo( let total_pages = parsed.total_pages.unwrap_or(1); let projects = parsed.projects; if projects.is_empty() { - return Ok(None); + break; } - let matched = projects.into_iter().find(|p| { - p.repo_url - .as_deref() - .map(|r| repo_url_matches_path(r, &expected, expected_host)) - .unwrap_or(false) - }); - if matched.is_some() { - return Ok(matched); + for project in projects { + let Some(repo_url) = project.repo_url.as_deref() else { + continue; + }; + if !repo_url_matches_path(repo_url, &expected) { + continue; + } + // Our own host settles it; no need to read further pages. + if expected_host.is_some_and(|h| repo_url_on_host(repo_url, h)) { + return Ok(Some(project)); + } + candidates.push(project); } if page >= total_pages { - return Ok(None); + break; } // Every reported page was NOT searched, so this is not a clean miss: // saying so would send the caller to the legacy-name fallback, which @@ -894,6 +902,21 @@ pub fn resolve_project_by_repo( } page += 1; } + + if candidates.len() > 1 { + let hosts: Vec<&str> = candidates + .iter() + .filter_map(|p| p.repo_url.as_deref()) + .collect(); + return Err(format!( + "{} Corgea projects claim repo '{}' ({}); pass --project-name to choose one", + candidates.len(), + repo_path, + hosts.join(", ") + ) + .into()); + } + Ok(candidates.pop()) } /// What `list`/`wait` need to drive the existing name-based queries. @@ -1729,93 +1752,64 @@ mod tests { // Same repo across scheme/.git/trailing-slash/port variants. assert!(repo_url_matches_path( "https://github.com/acme/api", - "acme/api", - None + "acme/api" )); assert!(repo_url_matches_path( "https://github.com/acme/api.git", - "acme/api", - None - )); - assert!(repo_url_matches_path( - "git@github.com:acme/api", - "acme/api", - None + "acme/api" )); - assert!(repo_url_matches_path("acme/api", "acme/api", None)); + assert!(repo_url_matches_path("git@github.com:acme/api", "acme/api")); + assert!(repo_url_matches_path("acme/api", "acme/api")); // Sibling / prefix repo, and a different org. assert!(!repo_url_matches_path( "https://github.com/acme/api-v2", - "acme/api", - None + "acme/api" )); assert!(!repo_url_matches_path( "https://github.com/notacme/api", - "acme/api", - None + "acme/api" )); // The owner must be top-level on the host: a nested mirror is a // different repository, as is `org/team/repo` for `team/repo`. assert!(!repo_url_matches_path( "https://github.com/mirrors/acme/api", - "acme/api", - None + "acme/api" )); assert!(!repo_url_matches_path( "https://github.com/org/team/repo", - "team/repo", - None + "team/repo" )); // Multi-segment paths compare in full. assert!(repo_url_matches_path( "https://dev.azure.com/org/project/_git/repo", - "org/project/_git/repo", - None + "org/project/_git/repo" )); assert!(repo_url_matches_path( "git@gitlab.com:group/subgroup/repo.git", - "group/subgroup/repo", - None + "group/subgroup/repo" )); } #[test] - fn repo_url_matches_path_separates_the_same_path_on_two_forges() { - // `?repo_url=acme/api` is hostless, so `icontains` returns the GitLab - // project too; confirming it would list another repo's scans. - assert!(repo_url_matches_path( + fn repo_url_on_host_reads_through_url_forms() { + assert!(repo_url_on_host( "https://github.com/acme/api", - "acme/api", - Some("github.com") + "github.com" )); - assert!(!repo_url_matches_path( - "https://gitlab.com/acme/api", - "acme/api", - Some("github.com") - )); - // Scheme/userinfo/port variants of the same host still match. - assert!(repo_url_matches_path( + assert!(repo_url_on_host( "git@github.com:acme/api.git", - "acme/api", - Some("github.com") + "github.com" )); - assert!(repo_url_matches_path( + assert!(repo_url_on_host( "https://gitlab.acme.com:8443/acme/api", - "acme/api", - Some("gitlab.acme.com") + "gitlab.acme.com" )); - // Hostless on either side carries no host to contradict: a bare - // `--repo acme/api` still matches, as does a stored bare `acme/api`. - assert!(repo_url_matches_path( + assert!(!repo_url_on_host( "https://gitlab.com/acme/api", - "acme/api", - None - )); - assert!(repo_url_matches_path( - "acme/api", - "acme/api", - Some("github.com") + "github.com" )); + // A stored bare `acme/api` is on no host at all. + assert!(!repo_url_on_host("acme/api", "github.com")); } #[test] @@ -1895,21 +1889,42 @@ mod tests { } #[test] - fn resolve_project_by_repo_rejects_the_same_path_on_another_host() { - // The backend's hostless `icontains` returns the GitLab project for a - // GitHub origin; confirming it would resolve the wrong project. + fn resolve_project_by_repo_picks_the_candidate_on_the_origin_host() { + // `?repo_url=acme/api` is hostless, so `icontains` returns both forges; + // the origin host is what says which one is ours. let base = spawn_projects_stub( - r#"{"status":"ok","projects":[{"id":3,"name":"acme/api","repo_url":"https://gitlab.com/acme/api"}]}"#, + r#"{"status":"ok","projects":[{"id":3,"name":"gl","repo_url":"https://gitlab.com/acme/api"},{"id":4,"name":"gh","repo_url":"https://github.com/acme/api"}]}"#, ); - assert!( - resolve_project_by_repo(&base, "acme/api", Some("gitlab.com")) - .unwrap() - .is_some() + let got = resolve_project_by_repo(&base, "acme/api", Some("github.com")).unwrap(); + assert_eq!(got.map(|p| p.name).as_deref(), Some("gh")); + let got = resolve_project_by_repo(&base, "acme/api", Some("gitlab.com")).unwrap(); + assert_eq!(got.map(|p| p.name).as_deref(), Some("gl")); + } + + #[test] + fn resolve_project_by_repo_accepts_a_lone_match_from_an_unknown_host() { + // An SSH-config alias origin (`corp-github:acme/api`) never equals the + // stored `github.com`, but there is nothing to disambiguate — holding + // out for a host match would leave COR-1577 unfixed for exactly the + // alias remotes this repo itself uses (PR #122 review). + let base = spawn_projects_stub( + r#"{"status":"ok","projects":[{"id":4,"name":"acme/api","repo_url":"https://github.com/acme/api"}]}"#, + ); + let got = resolve_project_by_repo(&base, "acme/api", Some("corp-github")).unwrap(); + assert_eq!(got.map(|p| p.name).as_deref(), Some("acme/api")); + } + + #[test] + fn resolve_project_by_repo_errors_when_the_path_is_claimed_twice() { + // Two forges, neither ours: picking either would be a coin flip, and + // reporting no match would quietly list a third project's scans. + let base = spawn_projects_stub( + r#"{"status":"ok","projects":[{"id":3,"name":"gl","repo_url":"https://gitlab.com/acme/api"},{"id":4,"name":"gh","repo_url":"https://github.com/acme/api"}]}"#, ); + let err = resolve_project_by_repo(&base, "acme/api", Some("corp-github")).unwrap_err(); assert!( - resolve_project_by_repo(&base, "acme/api", Some("github.com")) - .unwrap() - .is_none() + err.to_string().contains("--project-name"), + "the error should say how to disambiguate: {err}" ); } diff --git a/tests/list_resolution.rs b/tests/list_resolution.rs index 2de47bd..ca5b52b 100644 --- a/tests/list_resolution.rs +++ b/tests/list_resolution.rs @@ -129,6 +129,41 @@ fn list_issues_percent_encodes_the_project_name() { ); } +#[test] +fn list_resolves_through_an_ssh_config_host_alias() { + // `git@github.com-corgea:org/repo.git` — the shape this very repository's + // origin uses. Its host never equals the stored `github.com`, so treating + // the host as a gate rather than a tie-breaker would leave COR-1577 + // unfixed for every aliased remote. (PR #122 review) + let (url, hits) = common::spawn_recording_resolution_stub(routes( + projects_match(), + scans_one(CANON), + issues_one(), + )); + let (_tmp, repo) = temp_git_repo( + "build-123", + "git@github.com-corgea:bohappdev/dotnet-azure-web-tsb.git", + ); + let out = run_list(&[], &url, &repo); + let stdout = String::from_utf8_lossy(&out.stdout); + assert_eq!( + out.status.code(), + Some(0), + "stderr: {}", + String::from_utf8_lossy(&out.stderr) + ); + assert!( + stdout.contains("bohappdev/dotnet-azure-web-tsb"), + "stdout: {stdout}" + ); + let hits = hits.lock().unwrap(); + assert!( + hits.iter().any(|h| h.starts_with("/api/v1/scans?") + && h.contains("project=bohappdev%2Fdotnet-azure-web-tsb")), + "the alias must still resolve the canonical project; hits: {hits:?}" + ); +} + #[test] fn list_repo_flag_resolves_from_flag_not_remote() { // Records every request target so we can prove the slug came from --repo.