From 777d538059a914c21bdd4e0b1bd7c322a0a9c59f Mon Sep 17 00:00:00 2001 From: JATMN Date: Thu, 5 Nov 2026 17:22:15 -0800 Subject: [PATCH 1/2] fix(node): resolve issue ids from git refnames, not request input resolve_issue_id answered an exact match by echoing the request-path id back to callers, so get_issue and close_issue spliced caller input into `git cat-file blob refs/gitlawb/issues/`. #521 restricted the input to [A-Za-z0-9-], but the value handed to git was still the request string (CodeQL rust/command-line-injection #83). Resolve exact and prefix lookups through one for-each-ref listing and return the refname git reports. An exact refname still wins over longer ids that share it as a prefix; unique-prefix, ambiguous, not-found, and revision-syntax behavior is unchanged. Co-Authored-By: Claude Opus 5.5 --- crates/gitlawb-node/src/git/issues.rs | 69 +++++++++++++++------------ 1 file changed, 38 insertions(+), 31 deletions(-) diff --git a/crates/gitlawb-node/src/git/issues.rs b/crates/gitlawb-node/src/git/issues.rs index 664e441f..0f51d85d 100644 --- a/crates/gitlawb-node/src/git/issues.rs +++ b/crates/gitlawb-node/src/git/issues.rs @@ -141,19 +141,10 @@ pub fn resolve_issue_id(repo_path: &Path, id_or_prefix: &str) -> Result Result = output.lines().filter(|l| !l.trim().is_empty()).collect(); - - match matches.len() { - 0 => Ok(None), - 1 => { - // Strip the "refs/gitlawb/issues/" prefix to get the bare ID. - let full_id = matches[0] - .trim() - .strip_prefix("refs/gitlawb/issues/") - .unwrap_or(matches[0].trim()) - .to_string(); - Ok(Some(full_id)) - } - _ => anyhow::bail!( - "ambiguous issue prefix '{}': matches {} issues", - id_or_prefix, - matches.len() - ), - } + let matches: Vec<&str> = output + .lines() + .map(str::trim) + .filter(|l| !l.is_empty()) + .collect(); + + // An exact id wins over longer ids that share it as a prefix. + let found = match matches.iter().find(|m| **m == exact_ref) { + Some(m) => *m, + None => match matches.as_slice() { + [] => return Ok(None), + [m] => *m, + _ => anyhow::bail!( + "ambiguous issue prefix '{}': matches {} issues", + id_or_prefix, + matches.len() + ), + }, + }; + // Strip the "refs/gitlawb/issues/" prefix to get the bare ID. + let full_id = found + .strip_prefix("refs/gitlawb/issues/") + .unwrap_or(found) + .to_string(); + Ok(Some(full_id)) } /// Close an issue by updating its status to "closed" in the git ref. @@ -267,6 +264,16 @@ mod tests { assert_eq!(resolved, Some(full_id.to_string())); } + #[test] + fn test_resolve_exact_id_wins_over_longer_id() { + let dir = TempDir::new().unwrap(); + init_repo(&dir); + create_issue(dir.path(), "abc12345", r#"{"status":"open"}"#).unwrap(); + create_issue(dir.path(), "abc12345-0000", r#"{"status":"open"}"#).unwrap(); + let resolved = resolve_issue_id(dir.path(), "abc12345").unwrap(); + assert_eq!(resolved, Some("abc12345".to_string())); + } + #[test] fn test_resolve_prefix_matches_unique() { let dir = TempDir::new().unwrap(); From c3e3a5625f71a483c5a3c74c68e0ca4acb664601 Mon Sep 17 00:00:00 2001 From: jatmn Date: Thu, 5 Nov 2026 18:01:32 -0800 Subject: [PATCH 2/2] fix(node): keep a dangling exact issue ref from winning over longer ids for-each-ref lists a ref even when its blob is missing, so the exact-id precedence added in the previous commit picked a dangling exact ref over a readable longer sibling: resolve_issue_id returned the dangling id where main reported an ambiguous prefix, get_issue then returned None, and close_issue hit its expect(). Let the exact id win only when `git cat-file -e` finds its object, run on git's own refname so no request input returns to the argv. Otherwise fall through to the unchanged prefix rules, matching main for every layout. Co-Authored-By: Claude Opus 5.5 --- crates/gitlawb-node/src/git/issues.rs | 46 +++++++++++++++++++++++++-- 1 file changed, 43 insertions(+), 3 deletions(-) diff --git a/crates/gitlawb-node/src/git/issues.rs b/crates/gitlawb-node/src/git/issues.rs index 0f51d85d..7fdb8a47 100644 --- a/crates/gitlawb-node/src/git/issues.rs +++ b/crates/gitlawb-node/src/git/issues.rs @@ -162,9 +162,22 @@ pub fn resolve_issue_id(repo_path: &Path, id_or_prefix: &str) -> Result *m, + // An exact id wins over longer ids that share it as a prefix, but only if its + // object exists: for-each-ref also lists a ref whose blob is missing, so a + // dangling exact ref falls through to the prefix rules below. + let exact = match matches.iter().find(|m| **m == exact_ref) { + Some(m) => { + let check = Command::new("git") + .args(["cat-file", "-e", m]) + .current_dir(repo_path) + .output() + .context("failed to run git cat-file -e")?; + check.status.success().then_some(*m) + } + None => None, + }; + let found = match exact { + Some(m) => m, None => match matches.as_slice() { [] => return Ok(None), [m] => *m, @@ -274,6 +287,33 @@ mod tests { assert_eq!(resolved, Some("abc12345".to_string())); } + #[test] + fn test_resolve_dangling_exact_id_does_not_win_over_longer_id() { + let dir = TempDir::new().unwrap(); + init_repo(&dir); + create_issue(dir.path(), "abc12345", r#"{"status":"open","n":1}"#).unwrap(); + create_issue(dir.path(), "abc12345-0000", r#"{"status":"open","n":2}"#).unwrap(); + // Dangle the exact ref: the ref stays listed by for-each-ref, but its blob is gone. + let out = Command::new("git") + .args(["rev-parse", "refs/gitlawb/issues/abc12345"]) + .current_dir(dir.path()) + .output() + .unwrap(); + let hash = String::from_utf8(out.stdout).unwrap(); + let hash = hash.trim(); + std::fs::remove_file( + dir.path() + .join(".git/objects") + .join(&hash[..2]) + .join(&hash[2..]), + ) + .unwrap(); + + let result = resolve_issue_id(dir.path(), "abc12345"); + assert!(result.unwrap_err().to_string().contains("ambiguous")); + assert!(close_issue(dir.path(), "abc12345").is_err()); + } + #[test] fn test_resolve_prefix_matches_unique() { let dir = TempDir::new().unwrap();