test(git-remote): clear the insecure-HTTP override in the fetch harness

fetch_with_helper passes the test process environment through to the helper,
so an operator or CI exporting GITLAWB_ALLOW_INSECURE_HTTP decided the
transport policy for every test that runs through it. Reported by CodeRabbit
on the review of 12e68e54.

Confirmed by execution: with the variable exported,
real_git_fetch_refuses_a_remote_plaintext_node failed, because the helper
permitted the URL and the refusal never appeared on stderr. With the
env_remove in place the whole target passes with the variable still exported.

Cleared in the shared harness rather than the one test, so a later test
cannot inherit the same ambient policy. A test that wants the opt-in can pass
it through the extra_env argument.

README.md

@@ -188,7 +188,12 @@ export GITLAWB_NODE=http://localhost:7545
 git clone gitlawb://did:key:z6Mk.../my-repo
 ```
 
-For public-network use, make sure `GITLAWB_NODE` points to the node you want. The helper defaults to localhost for local development.
+For public-network use, make sure `GITLAWB_NODE` points to the node you want, over
+`https://`. The helper defaults to localhost for local development, and plaintext
+`http://` is allowed only to this machine: requests are signed but not encrypted, so
+a cleartext hop to a remote node exposes the pack contents and the `Signature`
+header. A remote `http://` node is refused, and `GITLAWB_ALLOW_INSECURE_HTTP=1`
+overrides that for a trusted private network.
 
 ### Full lifecycle against an iCaptcha-enforcing node
 
@@ -393,7 +398,7 @@ Important node settings:
 | `GITLAWB_P2P_BOOTSTRAP` | Comma-separated libp2p multiaddrs. |
 | `GITLAWB_BOOTSTRAP_DISABLE_SEEDS` | Disable embedded seed peers for isolated dev/test networks. |
 | `GITLAWB_REQUIRE_SIGNED_PEER_WRITES` | Require signed peer announce/sync writes. Defaults to `false` during the staged rollout below. |
-| `GITLAWB_ENFORCE_OWNER_PUSH` | Require the authenticated pusher to be the repo owner on `git-receive-pack`. **Defaults to `true`.** A `did:key` signature is authentication, not authorization — anyone can mint a key and sign — so with this off every signed caller may push to every repository, private ones included. Delegated and CI keys count as non-owners: a UCAN `git/push` capability is verified but not yet honored for authorization, so they cannot push while this is on. Set `false` only for a rolling upgrade; see [`docs/RUN-A-NODE.md`](docs/RUN-A-NODE.md). |
+| `GITLAWB_ENFORCE_OWNER_PUSH` | Require the authenticated pusher to be the repo owner on `git-receive-pack`. **Defaults to `true`.** A `did:key` signature is authentication, not authorization (anyone can mint a key and sign), so with this off every signed caller may push to every repository, private ones included. Delegated and CI keys count as non-owners: a UCAN `git/push` capability is verified but not yet honored for authorization, so they cannot push while this is on. Set `false` only for a rolling upgrade; see [`docs/RUN-A-NODE.md`](docs/RUN-A-NODE.md). |
 | `GITLAWB_AUTO_SYNC` | Enable automatic sync from known peers. |
 | `GITLAWB_MAX_PACK_BYTES` | Max git pack body size for smart-HTTP routes. |
 | `GITLAWB_GIT_SERVICE_TIMEOUT_SECS` | Max seconds a served git upload-pack, receive-pack, or `info/refs` advertisement may run before it is aborted (504). Default 600. Also bounds the withheld-blob classification walk (on both the upload-pack serve and receive-pack replication paths) and the push-side pin-candidate discovery (`rev-list` / `cat-file`), each reaped via process-group teardown at the deadline. On the path-scoped upload-pack path the classification walk and the pack serve share ONE deadline, so this value bounds their combined duration rather than granting each stage a full budget: a walk that consumes it leaves the serve nothing and the clone gets a 504. Serving large path-scoped repos may therefore need a higher value than they did when each stage was budgeted separately. Accepted range is 1 to 3153600000 (100 years), since the node derives deadlines from this value and a larger one cannot be represented. |

crates/git-remote-gitlawb/src/main.rs

@@ -66,6 +66,10 @@ fn main() -> Result<()> {
     // v0.1: default to localhost. Override with GITLAWB_NODE env var.
     let node_base =
         std::env::var("GITLAWB_NODE").unwrap_or_else(|_| "http://127.0.0.1:7545".to_string());
+    check_transport_security(
+        &node_base,
+        std::env::var_os("GITLAWB_ALLOW_INSECURE_HTTP").is_some(),
+    )?;
     let repo_base = format!("{}/{}/{}", node_base, short_owner, repo_name);
     tracing::debug!("repo_base: {repo_base}");
 
@@ -78,6 +82,76 @@ fn main() -> Result<()> {
     run_helper(&repo_base, keypair.as_ref())
 }
 
+/// Whether `url` is plaintext http, regardless of where it points.
+fn is_http(url: &str) -> bool {
+    reqwest::Url::parse(url.trim())
+        .map(|u| u.scheme() == "http")
+        .unwrap_or(false)
+}
+
+/// Whether `url` would send git data off this machine in cleartext.
+///
+/// True only for `http://` to a non-loopback host. RFC 9421 signs the request
+/// but does not encrypt it, so on a plaintext hop the pack contents and the
+/// `Signature` header are both readable, and a captured signature is replayable
+/// for its freshness window against any host.
+///
+/// Loopback is decided from the parsed address rather than a string match, so
+/// `127.0.0.2`, `[::1]` and an IPv4-mapped `[::ffff:127.0.0.1]` are all
+/// recognised as this machine. A value that does not parse, or that is not
+/// http(s), is not this guard's business and returns false: it fails later with
+/// its own error, and naming it a TLS problem would misdirect the reader.
+fn is_insecure_remote(url: &str) -> bool {
+    let Ok(parsed) = reqwest::Url::parse(url.trim()) else {
+        return false;
+    };
+    if parsed.scheme() != "http" {
+        return false;
+    }
+    let Some(host) = parsed.host_str() else {
+        return false;
+    };
+    let host = host.trim_end_matches('.').to_ascii_lowercase();
+    if host == "localhost" {
+        return false;
+    }
+    // host_str() keeps the brackets on an IPv6 literal.
+    let bare = host.trim_start_matches('[').trim_end_matches(']');
+    if let Ok(ip) = bare.parse::<std::net::IpAddr>() {
+        if ip.is_loopback() {
+            return false;
+        }
+        // An IPv4-mapped IPv6 literal hides a v4 loopback from is_loopback().
+        if let std::net::IpAddr::V6(v6) = ip {
+            if let Some(v4) = v6.to_ipv4_mapped() {
+                if v4.is_loopback() {
+                    return false;
+                }
+            }
+        }
+    }
+    true
+}
+
+/// Refuse a cleartext hop off this machine unless the operator has opted in.
+///
+/// Fail closed: the alternative is signing a request and then handing it, and
+/// the pack it carries, to anyone on the path. `GITLAWB_ALLOW_INSECURE_HTTP`
+/// exists for a private LAN where the operator has decided that is acceptable.
+fn check_transport_security(node_base: &str, allow_insecure: bool) -> Result<()> {
+    if allow_insecure || !is_insecure_remote(node_base) {
+        return Ok(());
+    }
+    bail!(
+        "refusing to send git data to {node_base} over plaintext http.\n\
+         Requests are signed but not encrypted, so the pack contents and the \
+         Signature header are readable by anyone on the path, and a captured \
+         signature can be replayed.\n\
+         Use https://, or set GITLAWB_ALLOW_INSECURE_HTTP=1 to accept the risk \
+         (for a trusted private network only)."
+    )
+}
+
 // ── CLI argument handling ──────────────────────────────────────────────────────
 
 /// How the binary was invoked, derived from its CLI arguments.
@@ -125,6 +199,7 @@ fn help_text() -> String {
          ENVIRONMENT:\n\
          \x20   GITLAWB_NODE   Node base URL (default: http://127.0.0.1:7545)\n\
          \x20   GITLAWB_KEY    Identity PEM path for signed fetch/push (default: ~/.gitlawb/identity.pem)\n\
+         \x20   GITLAWB_ALLOW_INSECURE_HTTP  Permit plaintext http:// to a non-loopback node (unset by default)\n\
          \x20   GITLAWB_LOG    Log filter (default: warn)\n\
          \n\
          FLAGS:\n\
@@ -199,7 +274,8 @@ fn handle_connect<R: Read>(
         other => bail!("unsupported git service: {other}"),
     }
 
-    let client = build_http_client()?;
+    // A loopback node must never be proxied; see build_http_client.
+    let client = build_http_client(!is_insecure_remote(repo_base) && is_http(repo_base))?;
 
     // ── Phase 1: ref advertisement (GET /info/refs?service=<service>) ─────────
     //
@@ -335,8 +411,22 @@ const HTTP_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(300);
 /// node named, and on a 307/308 the pack body went with them. Scope the follow to the
 /// origin that issued the redirect AND to an identical request-target, which is the
 /// same predicate `gl` uses.
-fn build_http_client() -> Result<reqwest::blocking::Client> {
-    Ok(reqwest::blocking::Client::builder()
+/// `bypass_proxy` must be true whenever the node is on this machine. reqwest's
+/// default client honours `HTTP_PROXY` / `ALL_PROXY` for a loopback URL too, so a
+/// proxy variable pointing off-machine turns an allowed local request into a
+/// cleartext hop carrying the `Signature` header, straight past
+/// `check_transport_security`. Verified: with `HTTP_PROXY` set, reqwest logged
+/// `proxy(...) intercepts 'http://127.0.0.1:7545/'` and dialled the proxy. A
+/// local node is never legitimately reached through a proxy, so this refuses one
+/// rather than trying to reimplement `NO_PROXY` parsing.
+fn build_http_client(bypass_proxy: bool) -> Result<reqwest::blocking::Client> {
+    let builder = reqwest::blocking::Client::builder();
+    let builder = if bypass_proxy {
+        builder.no_proxy()
+    } else {
+        builder
+    };
+    Ok(builder
         .timeout(HTTP_TIMEOUT)
         .redirect(reqwest::redirect::Policy::custom(same_origin_redirect))
         .build()?)
@@ -829,6 +919,148 @@ fn resolve_key_path() -> std::path::PathBuf {
 
 // ── Tests ─────────────────────────────────────────────────────────────────────
 
+#[cfg(test)]
+mod insecure_transport_tests {
+    use super::*;
+
+    /// Loopback in every form the transport can legitimately be pointed at.
+    /// Plaintext to this machine is the documented local-alpha default and must
+    /// keep working, so a false positive here breaks every stock install.
+    #[test]
+    fn loopback_http_is_allowed() {
+        for url in [
+            "http://127.0.0.1:7545",
+            "http://localhost:7545",
+            "http://[::1]:7545",
+            "http://127.0.0.2:7545",
+            "http://[::ffff:127.0.0.1]:7545",
+            "http://LocalHost:7545",
+        ] {
+            assert!(
+                !is_insecure_remote(url),
+                "{url} is this machine; plaintext to it must stay allowed"
+            );
+        }
+    }
+
+    /// The case the guard exists for: cleartext git data leaving the machine.
+    #[test]
+    fn remote_http_is_refused() {
+        for url in [
+            "http://node.example.com:7545",
+            "http://10.0.0.36:7777",
+            "http://192.168.1.10",
+            "http://[2001:db8::1]:7545",
+            "http://8.8.8.8",
+        ] {
+            assert!(
+                is_insecure_remote(url),
+                "{url} sends git data off-machine in cleartext and must be refused"
+            );
+        }
+    }
+
+    /// TLS is always fine, loopback or not, so the guard keys on the scheme and
+    /// not merely on the host being remote.
+    #[test]
+    fn https_is_always_allowed() {
+        for url in [
+            "https://node.gitlawb.com",
+            "https://10.0.0.36:7777",
+            "https://127.0.0.1:7545",
+        ] {
+            assert!(!is_insecure_remote(url), "{url} is TLS and must be allowed");
+        }
+    }
+
+    /// Spellings that defeat a naive loopback check. `Url` normalizes the scheme
+    /// and the host at parse time (lowercasing, and the WHATWG numeric forms), so
+    /// these must all still read as this machine.
+    #[test]
+    fn loopback_spellings_are_normalized_not_string_matched() {
+        for url in [
+            "HTTP://127.0.0.1:7545",
+            "Http://LOCALHOST:7545",
+            "http://localhost.:7545",
+            "http://user:[email protected]:7545",
+            "http://2130706433:7545",
+            "http://0x7f000001:7545",
+            "http://[::ffff:7f00:1]:7545",
+        ] {
+            assert!(
+                !is_insecure_remote(url),
+                "{url} resolves to this machine; plaintext to it must stay allowed"
+            );
+        }
+    }
+
+    /// The mirror of the above: an uppercase scheme must not become an escape
+    /// from the guard, and a remote host in any spelling is still remote.
+    #[test]
+    fn uppercase_scheme_does_not_escape_the_guard() {
+        for url in [
+            "HTTP://node.example.com:7545",
+            "Http://8.8.8.8",
+            "http://NODE.EXAMPLE.COM",
+            "http://user:[email protected]",
+        ] {
+            assert!(
+                is_insecure_remote(url),
+                "{url} is a remote cleartext hop and must be refused"
+            );
+        }
+    }
+
+    /// The gate itself, both directions, including the opt-in escape hatch.
+    #[test]
+    fn gate_refuses_remote_plaintext_unless_opted_in() {
+        let err = check_transport_security("http://node.example.com:7545", false)
+            .expect_err("remote plaintext must be refused by default");
+        let msg = err.to_string();
+        assert!(
+            msg.contains("plaintext http"),
+            "the refusal must say why: {msg}"
+        );
+        assert!(
+            msg.contains("GITLAWB_ALLOW_INSECURE_HTTP"),
+            "the refusal must name the escape hatch: {msg}"
+        );
+
+        check_transport_security("http://node.example.com:7545", true)
+            .expect("the opt-in must permit the same URL");
+        check_transport_security("http://127.0.0.1:7545", false)
+            .expect("the local default must keep working with no opt-in");
+        check_transport_security("https://node.gitlawb.com", false)
+            .expect("TLS must need no opt-in");
+    }
+
+    /// The documented knob must appear in --help. A gate the operator cannot
+    /// discover reads as a broken transport rather than a deliberate refusal.
+    #[test]
+    fn help_documents_the_opt_in() {
+        assert!(help_text().contains("GITLAWB_ALLOW_INSECURE_HTTP"));
+    }
+
+    /// An unparseable or non-http value is not classified as insecure here: it
+    /// fails later with its own error, and reporting it as a TLS problem would
+    /// send the reader after the wrong thing.
+    #[test]
+    fn unparseable_or_other_scheme_is_not_this_guards_problem() {
+        for url in [
+            "",
+            "   ",
+            "not a url",
+            "ftp://node.example.com",
+            "file:///tmp/x",
+        ] {
+            assert!(
+                !is_insecure_remote(url),
+                "{url:?} is not a cleartext-http-to-remote case"
+            );
+        }
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -929,7 +1161,7 @@ mod tests {
     #[test]
     fn signed_requests_do_not_follow_a_redirect_off_the_node_origin() {
         let kp = Keypair::generate();
-        let client = build_http_client().unwrap();
+        let client = build_http_client(false).unwrap();
 
         let mut elsewhere = mockito::Server::new();
         let never = elsewhere
@@ -1031,7 +1263,7 @@ mod tests {
     #[test]
     fn a_same_origin_path_changing_redirect_is_refused() {
         let kp = Keypair::generate();
-        let client = build_http_client().unwrap();
+        let client = build_http_client(false).unwrap();
 
         let mut node = mockito::Server::new();
         let bounce = node
@@ -1082,7 +1314,7 @@ mod tests {
     #[test]
     fn an_identical_target_redirect_is_still_followed_up_to_the_chain_bound() {
         let kp = Keypair::generate();
-        let client = build_http_client().unwrap();
+        let client = build_http_client(false).unwrap();
 
         let mut node = mockito::Server::new();
         let loop_route = node
@@ -1243,7 +1475,7 @@ mod tests {
     fn a_rewritten_target_never_receives_the_signature() {
         let kp = Keypair::generate();
         let expected_did = kp.did().to_string();
-        let client = build_http_client().unwrap();
+        let client = build_http_client(false).unwrap();
         let slot = std::sync::Arc::new(std::sync::Mutex::new(None::<Verdict>));
 
         let mut node = mockito::Server::new();
@@ -1305,7 +1537,7 @@ mod tests {
     fn a_direct_signed_advertisement_verifies_under_the_node_verifier() {
         let kp = Keypair::generate();
         let expected_did = kp.did().to_string();
-        let client = build_http_client().unwrap();
+        let client = build_http_client(false).unwrap();
         let slot = std::sync::Arc::new(std::sync::Mutex::new(None::<Verdict>));
 
         let mut node = mockito::Server::new();

crates/git-remote-gitlawb/tests/real_git_fetch.rs

@@ -267,6 +267,15 @@ fn write_response(mut stream: TcpStream, status: &str, content_type: &str, body:
 /// Run `git fetch` in `clone` through the helper, with a hard timeout so a
 /// regression to the deadlock fails fast instead of hanging the suite.
 fn fetch_with_helper(clone: &Path, node_url: &str) -> (bool, std::process::Output) {
+    fetch_with_helper_env(clone, node_url, &[])
+}
+
+/// [`fetch_with_helper`] with extra environment for the helper process.
+fn fetch_with_helper_env(
+    clone: &Path,
+    node_url: &str,
+    extra_env: &[(&str, &str)],
+) -> (bool, std::process::Output) {
     let helper_bin = PathBuf::from(env!("CARGO_BIN_EXE_git-remote-gitlawb"));
     let helper_dir = helper_bin.parent().unwrap().to_path_buf();
     let path_env = match std::env::var_os("PATH") {
@@ -289,7 +298,14 @@ fn fetch_with_helper(clone: &Path, node_url: &str) -> (bool, std::process::Outpu
         // machine/CI with git-l10n installed and LANG set to a translated locale.
         .env("LC_ALL", "C")
         .env("GITLAWB_NODE", node_url)
-        .env("GITLAWB_KEY", "/nonexistent-key-for-anon-fetch");
+        .env("GITLAWB_KEY", "/nonexistent-key-for-anon-fetch")
+        // The helper inherits this process's environment, so an operator or CI
+        // that exports the insecure-HTTP override would decide the transport
+        // policy for every test below. Clear it and let each test opt in.
+        .env_remove("GITLAWB_ALLOW_INSECURE_HTTP");
+    for (k, v) in extra_env {
+        cmd.env(k, v);
+    }
     run_bounded(cmd, Duration::from_secs(30))
 }
 
@@ -1242,3 +1258,71 @@ fn run_bounded_bounds_join_when_leader_exits_leaving_a_pipe_holder() {
          held pipes do not stall the joins)"
     );
 }
+
+/// The plaintext-transport gate, proven at its CALL SITE rather than in
+/// isolation. `main.rs`'s unit tests cover `is_insecure_remote` and
+/// `check_transport_security` as functions; neither notices if the call in
+/// `main()` is deleted, which is the regression that would silently restore the
+/// cleartext hop. Only driving the built binary binds the wiring.
+///
+/// `192.0.2.1` is TEST-NET-1 (RFC 5737): non-loopback, reserved for
+/// documentation, and never routable, so the gate is what stops this and no
+/// connection is attempted even if it regressed.
+#[test]
+fn real_git_fetch_refuses_a_remote_plaintext_node() {
+    // A shallow fixture is enough: the gate fires before any negotiation.
+    let repos = build_divergent_repos(1);
+    let (ok, out) = fetch_with_helper(&repos.clone, "http://192.0.2.1:7545");
+    let stderr = String::from_utf8_lossy(&out.stderr);
+
+    assert!(
+        !ok || !out.status.success(),
+        "the fetch must not succeed: {stderr}"
+    );
+    assert!(
+        stderr.contains("plaintext http"),
+        "the helper must refuse the cleartext hop and say so; stderr was: {stderr}"
+    );
+    assert!(
+        stderr.contains("GITLAWB_ALLOW_INSECURE_HTTP"),
+        "the refusal must name the escape hatch; stderr was: {stderr}"
+    );
+}
+
+/// A proxy variable must not divert a LOOPBACK node off this machine.
+///
+/// reqwest's default client honours `HTTP_PROXY` for a loopback URL, so before
+/// the fix a proxy pointing off-machine carried the signed plaintext request to
+/// it, past the transport guard, which only ever inspected the URL. Observed
+/// directly: `proxy(http://...:9999/) intercepts 'http://127.0.0.1:7545/'`.
+///
+/// The proxy here is a black hole on TEST-NET-1, so if the bypass regresses this
+/// fetch cannot succeed: it either hangs to the harness timeout or fails to
+/// connect. Success is only possible when the request went straight to the shim.
+#[test]
+fn real_git_fetch_ignores_a_proxy_for_a_loopback_node() {
+    let repos = build_divergent_repos(1);
+    let (server, clone) = (repos.server.clone(), repos.clone.clone());
+    let shim = start_shim(server.clone(), ShimMode::Normal);
+
+    let (completed, out) = fetch_with_helper_env(
+        &clone,
+        &shim.base_url,
+        &[
+            ("HTTP_PROXY", "http://192.0.2.1:9999"),
+            ("http_proxy", "http://192.0.2.1:9999"),
+            ("ALL_PROXY", "http://192.0.2.1:9999"),
+        ],
+    );
+
+    assert!(
+        completed,
+        "the fetch did not finish; a proxied loopback request would stall here. stderr:\n{}",
+        String::from_utf8_lossy(&out.stderr)
+    );
+    assert!(
+        out.status.success(),
+        "a loopback fetch must ignore the proxy variables. stderr:\n{}",
+        String::from_utf8_lossy(&out.stderr)
+    );
+}

crates/gitlawb-node/src/api/issues.rs

@@ -449,198 +449,3 @@ mod lock_pool_shed_tests {
         );
     }
 }
-
-/// #426: end-to-end at the handler seam. A corrupt object store behind a
-/// resolving issue ref used to look identical to an absent issue: get returned
-/// a fake 404, close returned a fake 404 after releasing the write guard, and
-/// list silently under-reported. All three must now surface a git error.
-#[cfg(test)]
-mod corrupt_store_tests {
-    use super::*;
-    use axum::response::IntoResponse;
-    use sqlx::PgPool;
-    use tempfile::TempDir;
-
-    fn seed_repo(owner_did: &str, name: &str) -> crate::db::RepoRecord {
-        let now = Utc::now();
-        crate::db::RepoRecord {
-            id: Uuid::new_v4().to_string(),
-            name: name.to_string(),
-            owner_did: owner_did.to_string(),
-            description: None,
-            is_public: true,
-            default_branch: "main".to_string(),
-            created_at: now,
-            updated_at: now,
-            disk_path: format!("/tmp/{name}"),
-            forked_from: None,
-            machine_id: None,
-        }
-    }
-
-    /// A state whose repo store roots in `repos_dir`, so the test can lay down
-    /// a real repo and then corrupt it without touching /tmp.
-    async fn state_with_repos_dir(pool: &PgPool, repos_dir: &std::path::Path) -> AppState {
-        let mut state = crate::test_support::test_state(pool.clone()).await;
-        state.repo_store =
-            crate::git::repo_store::RepoStore::for_testing(repos_dir.to_path_buf(), pool.clone());
-        state
-    }
-
-    /// Bare repo at the store's expected path, holding one issue ref.
-    fn bare_repo_with_issue(
-        repos_dir: &std::path::Path,
-        owner: &str,
-        name: &str,
-    ) -> (std::path::PathBuf, String) {
-        let repo_path = repos_dir
-            .join(owner.replace([':', '/'], "_"))
-            .join(format!("{name}.git"));
-        std::fs::create_dir_all(&repo_path).unwrap();
-        let out = std::process::Command::new("git")
-            .args(["init", "--bare"])
-            .arg(&repo_path)
-            .output()
-            .unwrap();
-        assert!(out.status.success());
-        let issue_id = "aaaabbbb-0000-0000-0000-000000000000".to_string();
-        git_issues::create_issue(
-            &repo_path,
-            &issue_id,
-            &serde_json::json!({
-                "id": issue_id,
-                "title": "t",
-                "status": "open",
-            })
-            .to_string(),
-        )
-        .unwrap();
-        (repo_path, issue_id)
-    }
-
-    /// Delete the blob object behind the issue ref and put garbage in its
-    /// place: `cat-file` fails while `rev-parse` still resolves the ref.
-    fn corrupt_issue_object(repo_path: &std::path::Path, issue_id: &str) {
-        let out = std::process::Command::new("git")
-            .args(["rev-parse", &format!("refs/gitlawb/issues/{issue_id}")])
-            .current_dir(repo_path)
-            .output()
-            .unwrap();
-        let oid = String::from_utf8(out.stdout).unwrap().trim().to_string();
-        let obj = repo_path.join("objects").join(&oid[..2]).join(&oid[2..]);
-        std::fs::remove_file(&obj).unwrap();
-        std::fs::write(&obj, b"garbage").unwrap();
-    }
-
-    /// The regression: closing over a corrupt store used to return a fake 404
-    /// (or panic deeper in). It must reach the caller as a git 500, and the
-    /// handler must return rather than unwind.
-    #[sqlx::test]
-    async fn close_issue_on_corrupt_store_returns_git_error_not_panic(pool: PgPool) {
-        let owner = "did:key:zISSUECORRUPTCCCCCCCCCCCCCCCCCCCCCCCCC";
-        let dir = TempDir::new().unwrap();
-        let state = state_with_repos_dir(&pool, dir.path()).await;
-        state
-            .db
-            .create_repo(&seed_repo(owner, "corrupt-close"))
-            .await
-            .expect("seed repo");
-        let (repo_path, issue_id) = bare_repo_with_issue(dir.path(), owner, "corrupt-close");
-        corrupt_issue_object(&repo_path, &issue_id);
-
-        let err = close_issue(
-            State(state),
-            Extension(AuthenticatedDid(owner.to_string())),
-            Path((owner.to_string(), "corrupt-close".to_string(), issue_id)),
-        )
-        .await
-        .expect_err("a corrupt store must error, not panic or 404");
-        assert_eq!(
-            err.into_response().status(),
-            StatusCode::INTERNAL_SERVER_ERROR,
-            "a read failure is a git 500, not a fake not-found"
-        );
-    }
-
-    /// get on a corrupt store: 500, not the 404 the fold used to produce.
-    #[sqlx::test]
-    async fn get_issue_on_corrupt_store_returns_500_not_404(pool: PgPool) {
-        let owner = "did:key:zISSUECORRUPTGETDDDDDDDDDDDDDDDDDDDDDDD";
-        let dir = TempDir::new().unwrap();
-        let state = state_with_repos_dir(&pool, dir.path()).await;
-        state
-            .db
-            .create_repo(&seed_repo(owner, "corrupt-get"))
-            .await
-            .expect("seed repo");
-        let (repo_path, issue_id) = bare_repo_with_issue(dir.path(), owner, "corrupt-get");
-        corrupt_issue_object(&repo_path, &issue_id);
-
-        let err = get_issue(
-            State(state),
-            Path((owner.to_string(), "corrupt-get".to_string(), issue_id)),
-            None,
-        )
-        .await
-        .expect_err("a corrupt store must error, not 404");
-        assert_eq!(
-            err.into_response().status(),
-            StatusCode::INTERNAL_SERVER_ERROR,
-        );
-    }
-
-    /// list on a corrupt store must fail outright; a 200 that silently drops
-    /// the unreadable issue is the other half of the fold.
-    #[sqlx::test]
-    async fn list_issues_on_corrupt_store_returns_git_error(pool: PgPool) {
-        let owner = "did:key:zISSUECORRUPTLISTEEEEEEEEEEEEEEEEEEEEEE";
-        let dir = TempDir::new().unwrap();
-        let state = state_with_repos_dir(&pool, dir.path()).await;
-        state
-            .db
-            .create_repo(&seed_repo(owner, "corrupt-list"))
-            .await
-            .expect("seed repo");
-        let (repo_path, issue_id) = bare_repo_with_issue(dir.path(), owner, "corrupt-list");
-        corrupt_issue_object(&repo_path, &issue_id);
-
-        let err = list_issues(
-            State(state),
-            Path((owner.to_string(), "corrupt-list".to_string())),
-            None,
-        )
-        .await
-        .expect_err("a corrupt store must error, not under-report");
-        assert_eq!(
-            err.into_response().status(),
-            StatusCode::INTERNAL_SERVER_ERROR,
-        );
-    }
-
-    /// Must-not direction: a genuinely absent issue still gets the 404.
-    #[sqlx::test]
-    async fn get_issue_on_absent_ref_still_returns_404(pool: PgPool) {
-        let owner = "did:key:zISSUEABSENTFFFFFFFFFFFFFFFFFFFFFFFFFFF";
-        let dir = TempDir::new().unwrap();
-        let state = state_with_repos_dir(&pool, dir.path()).await;
-        state
-            .db
-            .create_repo(&seed_repo(owner, "absent-get"))
-            .await
-            .expect("seed repo");
-        bare_repo_with_issue(dir.path(), owner, "absent-get");
-
-        let err = get_issue(
-            State(state),
-            Path((
-                owner.to_string(),
-                "absent-get".to_string(),
-                "ffffffff-0000-0000-0000-000000000000".to_string(),
-            )),
-            None,
-        )
-        .await
-        .expect_err("a missing issue is still not-found");
-        assert_eq!(err.into_response().status(), StatusCode::NOT_FOUND);
-    }
-}

crates/gitlawb-node/src/git/issues.rs

@@ -61,119 +61,6 @@ pub fn create_issue(repo_path: &Path, issue_id: &str, json: &str) -> Result<()>
     Ok(())
 }
 
-/// Issue ids are UUIDs; refs under `refs/gitlawb/issues/` only ever carry
-/// `[0-9a-zA-Z_-]` names. Anything else is not a value we pass to git.
-fn issue_ref_name_is_safe(ref_name: &str) -> bool {
-    let Some(id) = ref_name.strip_prefix("refs/gitlawb/issues/") else {
-        return false;
-    };
-    !id.is_empty()
-        && id
-            .chars()
-            .all(|c| c.is_ascii_alphanumeric() || c == '-' || c == '_')
-}
-
-/// Resolve a ref name without putting it on a command line: enumerate refs
-/// (constant argv) and match in memory. A nonzero exit propagates, so "cannot
-/// determine" is an error rather than a quiet `false`.
-fn ref_name_resolves(repo_path: &Path, ref_name: &str) -> Result<bool> {
-    // allow-unbounded-git: failure-path existence recheck, module convention.
-    let out = Command::new("git")
-        .args(["for-each-ref", "--format=%(refname)"])
-        .current_dir(repo_path)
-        .output()
-        .context("failed to run git for-each-ref")?;
-    if !out.status.success() {
-        let stderr = String::from_utf8_lossy(&out.stderr);
-        anyhow::bail!("git for-each-ref failed: {}", stderr.trim());
-    }
-    let stdout = String::from_utf8_lossy(&out.stdout);
-    Ok(stdout.lines().any(|line| line.trim() == ref_name))
-}
-
-/// Read the blob behind an issue ref.
-///
-/// `Ok(None)` means the ref itself is absent. A ref that resolves but whose
-/// object cannot be read (corrupt object, unreadable store, a mid-read gc) is
-/// an error, not an absence (#426).
-///
-/// The ref name reaches git on stdin, never argv: `cat-file --batch` answers
-/// `<oid> <type> <size>\n<body>\n` for a readable object and
-/// `<spec> missing\n` for an absent ref or an unreadable object alike, so the
-/// `missing` case is re-resolved by name enumeration to tell them apart.
-fn read_issue_blob(repo_path: &Path, ref_name: &str) -> Result<Option<String>> {
-    if !issue_ref_name_is_safe(ref_name) {
-        anyhow::bail!("refusing issue ref outside the issues namespace: {ref_name}");
-    }
-
-    // allow-unbounded-git: this module's spawns predate the bounded runner and
-    // its callers run inside issue handlers that already hold the repo guard.
-    let mut child = Command::new("git")
-        .args(["cat-file", "--batch"])
-        .current_dir(repo_path)
-        .stdin(std::process::Stdio::piped())
-        .stdout(std::process::Stdio::piped())
-        .stderr(std::process::Stdio::piped())
-        .spawn()
-        .context("failed to spawn git cat-file --batch")?;
-
-    use std::io::Write;
-    // Reap the child even when the write fails so a stdin error cannot drop an
-    // unwaited Child and leak a zombie (#53).
-    let write_result = match child.stdin.take() {
-        Some(mut stdin) => stdin
-            .write_all(ref_name.as_bytes())
-            .and_then(|()| stdin.write_all(b"\n")),
-        None => Err(std::io::Error::new(
-            std::io::ErrorKind::BrokenPipe,
-            "git cat-file --batch stdin unavailable",
-        )),
-    };
-    let output = child
-        .wait_with_output()
-        .context("git cat-file --batch failed")?;
-    write_result.context("failed to write ref name to git cat-file stdin")?;
-
-    if !output.status.success() {
-        let stderr = String::from_utf8_lossy(&output.stderr);
-        anyhow::bail!("git cat-file --batch failed: {}", stderr.trim());
-    }
-
-    // "<spec> missing\n" covers an absent ref and an unreadable object; only a
-    // ref that still resolves is the error case.
-    let missing = format!("{ref_name} missing\n");
-    if output.stdout.as_slice() == missing.as_bytes() {
-        return match ref_name_resolves(repo_path, ref_name)? {
-            true => anyhow::bail!("issue ref resolves but its object is unreadable: {ref_name}"),
-            false => Ok(None),
-        };
-    }
-
-    // Header: "<oid> <type> <size>\n" then exactly <size> bytes plus a newline.
-    let header_end = output
-        .stdout
-        .iter()
-        .position(|b| *b == b'\n')
-        .context("unexpected git cat-file --batch output")?;
-    let header = String::from_utf8_lossy(&output.stdout[..header_end]);
-    let mut parts = header.split(' ');
-    parts.next();
-    let obj_type = parts.next().unwrap_or("");
-    if obj_type != "blob" {
-        anyhow::bail!("issue ref {ref_name} resolves to a non-blob object");
-    }
-    let size: usize = parts
-        .next()
-        .and_then(|s| s.parse().ok())
-        .context("unexpected git cat-file --batch header")?;
-    let body = &output.stdout[header_end + 1..];
-    anyhow::ensure!(
-        body.len() >= size,
-        "truncated git cat-file --batch body for {ref_name}"
-    );
-    Ok(Some(String::from_utf8_lossy(&body[..size]).to_string()))
-}
-
 /// List all issue refs and return their JSON content.
 pub fn list_issues(repo_path: &Path) -> Result<Vec<String>> {
     // List all refs under refs/gitlawb/issues/
@@ -187,11 +74,9 @@ pub fn list_issues(repo_path: &Path) -> Result<Vec<String>> {
         .output()
         .context("failed to run git for-each-ref")?;
 
-    // for-each-ref exits 0 with empty output when no refs match, so a nonzero
-    // exit is a real enumeration failure, not "no issues yet".
     if !list_output.status.success() {
-        let stderr = String::from_utf8_lossy(&list_output.stderr);
-        anyhow::bail!("git for-each-ref failed: {}", stderr.trim());
+        // No issues yet
+        return Ok(vec![]);
     }
 
     let refs_str = String::from_utf8_lossy(&list_output.stdout);
@@ -203,12 +88,16 @@ pub fn list_issues(repo_path: &Path) -> Result<Vec<String>> {
             continue;
         }
 
-        // Read the blob content. A ref that vanished between for-each-ref and
-        // here is skipped; a ref that resolves but fails to read propagates so
-        // a degraded object store errors the listing instead of under-reporting.
-        match read_issue_blob(repo_path, ref_name)? {
-            Some(content) => issues.push(content),
-            None => continue,
+        // Read the blob content
+        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() {
+            let content = String::from_utf8_lossy(&cat_output.stdout).to_string();
+            issues.push(content);
         }
     }
 
@@ -238,12 +127,8 @@ pub fn resolve_issue_id(repo_path: &Path, id_or_prefix: &str) -> Result<Option<S
         .output()
         .context("failed to run git for-each-ref")?;
 
-    // for-each-ref exits 0 with empty output when nothing matches, so a
-    // nonzero exit is a real enumeration failure (e.g. not a repo), not
-    // "issue not found".
     if !list.status.success() {
-        let stderr = String::from_utf8_lossy(&list.stderr);
-        anyhow::bail!("git for-each-ref failed: {}", stderr.trim());
+        return Ok(None);
     }
 
     let output = String::from_utf8_lossy(&list.stdout);
@@ -276,12 +161,8 @@ pub fn close_issue(repo_path: &Path, issue_id: &str) -> Result<Option<String>> {
         None => return Ok(None),
     };
 
-    // A ref that resolved and is now absent was deleted between the two calls;
-    // any other read failure propagates as an error rather than panic (#426).
-    let raw = match get_issue(repo_path, &full_id)? {
-        Some(raw) => raw,
-        None => return Ok(None),
-    };
+    let raw = get_issue(repo_path, &full_id)?
+        .expect("ref existed in resolve but not in get — should be impossible");
 
     let mut issue: serde_json::Value =
         serde_json::from_str(&raw).context("invalid issue JSON in git ref")?;
@@ -300,7 +181,18 @@ pub fn get_issue(repo_path: &Path, issue_id: &str) -> Result<Option<String>> {
     };
 
     let ref_name = format!("refs/gitlawb/issues/{full_id}");
-    read_issue_blob(repo_path, &ref_name)
+    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);
+    }
+
+    let content = String::from_utf8_lossy(&cat_output.stdout).to_string();
+    Ok(Some(content))
 }
 
 #[cfg(test)]
@@ -402,109 +294,4 @@ mod tests {
         let v: serde_json::Value = serde_json::from_str(&updated).unwrap();
         assert_eq!(v["status"], "closed");
     }
-
-    /// Overwrite the loose object behind an issue ref with garbage, so
-    /// `cat-file` fails while `rev-parse` still resolves the ref (#426).
-    fn corrupt_issue_object(dir: &TempDir, full_id: &str) {
-        let oid_out = Command::new("git")
-            .args(["rev-parse", &format!("refs/gitlawb/issues/{full_id}")])
-            .current_dir(dir.path())
-            .output()
-            .unwrap();
-        let oid = String::from_utf8(oid_out.stdout)
-            .unwrap()
-            .trim()
-            .to_string();
-        let obj = dir
-            .path()
-            .join(".git/objects")
-            .join(&oid[..2])
-            .join(&oid[2..]);
-        std::fs::remove_file(&obj).unwrap();
-        std::fs::write(&obj, b"garbage").unwrap();
-    }
-
-    #[test]
-    fn test_get_and_close_missing_issue_return_none() {
-        let dir = TempDir::new().unwrap();
-        init_repo(&dir);
-        assert_eq!(get_issue(dir.path(), "nosuch00").unwrap(), None);
-        assert_eq!(close_issue(dir.path(), "nosuch00").unwrap(), None);
-    }
-
-    // #426: a directory that is not a repo makes `cat-file --batch` exit
-    // nonzero; that failure is an error, not a quiet None.
-    #[test]
-    fn test_issue_read_on_non_repo_errors_instead_of_none() {
-        let dir = TempDir::new().unwrap();
-        let not_a_repo = dir.path().join("not-a-repo");
-        std::fs::create_dir_all(&not_a_repo).unwrap();
-        assert!(read_issue_blob(&not_a_repo, "refs/gitlawb/issues/deadbeef").is_err());
-    }
-
-    // #426: the ref name is constrained to the issues namespace and a safe
-    // charset before it is fed to git, even on stdin (a newline would smuggle
-    // a second `--batch` spec).
-    #[test]
-    fn test_issue_read_rejects_ref_outside_issues_namespace() {
-        let dir = TempDir::new().unwrap();
-        init_repo(&dir);
-        assert!(read_issue_blob(dir.path(), "refs/heads/main").is_err());
-        assert!(read_issue_blob(dir.path(), "refs/gitlawb/issues/../x").is_err());
-    }
-
-    // #426: a failed for-each-ref enumeration is an error, not an empty list.
-    #[test]
-    fn test_list_issues_on_non_repo_errors_instead_of_empty() {
-        let dir = TempDir::new().unwrap();
-        let not_a_repo = dir.path().join("not-a-repo");
-        std::fs::create_dir_all(&not_a_repo).unwrap();
-        assert!(list_issues(&not_a_repo).is_err());
-    }
-
-    // #426: the same enumeration failure inside resolve_issue_id must reach
-    // get_issue/close_issue as an error, not a "not found" None.
-    #[test]
-    fn test_get_and_close_on_non_repo_error_instead_of_none() {
-        let dir = TempDir::new().unwrap();
-        let not_a_repo = dir.path().join("not-a-repo");
-        std::fs::create_dir_all(&not_a_repo).unwrap();
-        assert!(get_issue(&not_a_repo, "nosuch00").is_err());
-        assert!(close_issue(&not_a_repo, "nosuch00").is_err());
-    }
-
-    // #426: a ref that resolves but whose object is corrupt is a read error,
-    // not an absent issue. `close_issue` used to `expect()` on that None and
-    // panic inside a request handler holding the write guard.
-    #[test]
-    fn test_corrupt_issue_object_errors_instead_of_panicking() {
-        let dir = TempDir::new().unwrap();
-        init_repo(&dir);
-        let full_id = "bad00000-0000-0000-0000-000000000000";
-        create_issue(dir.path(), full_id, r#"{"status":"open"}"#).unwrap();
-        corrupt_issue_object(&dir, full_id);
-
-        assert!(get_issue(dir.path(), full_id).is_err());
-        assert!(close_issue(dir.path(), full_id).is_err());
-        assert!(list_issues(dir.path()).is_err());
-    }
-
-    #[test]
-    fn test_list_issues_returns_all() {
-        let dir = TempDir::new().unwrap();
-        init_repo(&dir);
-        create_issue(
-            dir.path(),
-            "aaa00000-0000-0000-0000-000000000000",
-            r#"{"status":"open"}"#,
-        )
-        .unwrap();
-        create_issue(
-            dir.path(),
-            "bbb00000-0000-0000-0000-000000000000",
-            r#"{"status":"open"}"#,
-        )
-        .unwrap();
-        assert_eq!(list_issues(dir.path()).unwrap().len(), 2);
-    }
 }