close_issue expects a resolved ref to stay readable, but get_issue folds every git cat-file failure into None

[PierrunoYT]

close_issue (git/issues.rs:158) resolves an issue ref, then asserts that reading it back cannot fail:

let full_id = match resolve_issue_id(repo_path, issue_id)? {
    Some(id) => id,
    None => return Ok(None),
};

let raw = get_issue(repo_path, &full_id)?
    .expect("ref existed in resolve but not in get — should be impossible");

The expectation is not guaranteed by the code it depends on.

Why the invariant does not hold

get_issue returns Ok(None) for two different situations, and only one of them is "the ref is absent":

let cat_output = Command::new("git")
    .args(["cat-file", "blob", &ref_name])
    .current_dir(repo_path)
    .output()
    .context("failed to run git cat-file")?;

if !cat_output.status.success() {
    return Ok(None);
}

Any non-zero exit from git cat-file becomes None. A missing ref produces that, but so does an unreadable object, a corrupt or truncated object file, a permissions problem on the object store, a repository that has been repacked or gc'd between the two calls, or an ENOSPC-class failure. resolve_issue_id runs cat-file -e / for-each-ref at an earlier instant against the same store, so the two calls are not atomic with respect to any of that.

The result is that an operational Git failure is converted into a panic rather than an error, on a path that has a perfectly good Result to return it through.

Reachability and severity

Low, and worth stating plainly rather than inflating.

The trigger is a Git or filesystem fault, not attacker-controlled input — the issue_id reaching create_issue is a server-minted UUID, and resolve_issue_id interpolates into a ref name and a for-each-ref pattern that always begins refs/gitlawb/issues/, so neither argument injection nor a traversal outside that namespace is available. I did not find a route that lets a caller induce the fault on demand.

What makes it worth fixing anyway is where it sits. close_issue runs from POST /api/v1/repos/{owner}/{repo}/issues/{id}/close, inside a request handler, while a write guard is held — so the failure mode for a degraded repository is a panicking task rather than a 500, and unwinding out of that path is a worse way to discover a corrupt object store than an error is.

Fix direction

Preserve the distinction get_issue currently collapses. Separate "ref absent" from "read failed": inspect the cat-file exit status and stderr, return Ok(None) only for the absent case, and propagate everything else as an Err. Then close_issue uses ? and the expect disappears rather than being replaced by a different assertion.

list_issues has the same shape at git/issues.rs:96 — a failed cat-file silently drops the issue from the listing — so the same change is worth applying there, where the current behavior is to under-report rather than to panic.

Validation status

Verified by reading the code and tracing both callers of get_issue. Not reproduced against a corrupt repository; the claim is that the invariant is unenforced, not that a specific fault has been observed to trigger it.

Disclosure note

Originally submitted through private vulnerability reporting and filed publicly at the maintainer's direction. Public filing is appropriate given the trigger is an operational fault rather than attacker-controlled input, and no exploitation route was demonstrated.

Related but distinct: #350 covers the panic class in gl (fixed-width slicing of node-supplied strings) and #417 covers the non-UTF-8 mirror path. This is the node-side sibling.

---

Found during an external audit pass.