diff --git a/crates/git-remote-gitlawb/src/main.rs b/crates/git-remote-gitlawb/src/main.rs index 738839c11..657d6d43d 100644 --- a/crates/git-remote-gitlawb/src/main.rs +++ b/crates/git-remote-gitlawb/src/main.rs @@ -25,6 +25,7 @@ use anyhow::{bail, Context, Result}; use gitlawb_core::http_sig::sign_request; use gitlawb_core::identity::Keypair; +use gitlawb_core::{resolve_transport_node, DEFAULT_LOCAL_NODE}; use std::io::{self, BufRead, Read, Write}; fn main() -> Result<()> { @@ -63,9 +64,9 @@ fn main() -> Result<()> { let (_, short_owner, repo_name) = parse_gitlawb_url(url)?; - // 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()); + // v0.1: default to localhost. Override with GITLAWB_NODE env var. Shared with + // `gl doctor` so the diagnostic reports the URL this actually resolves. + let node_base = node_base_from_env(); let repo_base = format!("{}/{}/{}", node_base, short_owner, repo_name); tracing::debug!("repo_base: {repo_base}"); @@ -78,6 +79,12 @@ fn main() -> Result<()> { run_helper(&repo_base, keypair.as_ref()) } +/// The node base URL this helper will contact, resolved through the same function +/// `gl doctor` reports on so the diagnostic cannot describe a different node. +fn node_base_from_env() -> String { + resolve_transport_node(std::env::var("GITLAWB_NODE").ok().as_deref()) +} + // ── CLI argument handling ────────────────────────────────────────────────────── /// How the binary was invoked, derived from its CLI arguments. @@ -123,7 +130,7 @@ fn help_text() -> String { \x20 git clone gitlawb://did:key:z6Mk.../\n\ \n\ ENVIRONMENT:\n\ - \x20 GITLAWB_NODE Node base URL (default: http://127.0.0.1:7545)\n\ + \x20 GITLAWB_NODE Node base URL (default: {DEFAULT_LOCAL_NODE})\n\ \x20 GITLAWB_KEY Identity PEM path for signed fetch/push (default: ~/.gitlawb/identity.pem)\n\ \x20 GITLAWB_LOG Log filter (default: warn)\n\ \n\ @@ -2014,9 +2021,21 @@ mod tests { assert!(help.contains("--version")); assert!(help.contains("--help")); assert!(help.contains("GITLAWB_NODE")); + // The advertised default must be the value the helper actually resolves. + assert!(help.contains(DEFAULT_LOCAL_NODE)); assert!(help.ends_with('\n')); } + /// The helper's own resolution must stay the shared one; a site walking back to + /// a private literal is exactly the drift the shared function exists to stop. + #[test] + fn node_base_matches_the_shared_resolver() { + // No env manipulation: assert the call site delegates, for the default case. + let expected = resolve_transport_node(std::env::var("GITLAWB_NODE").ok().as_deref()); + assert_eq!(node_base_from_env(), expected); + assert_eq!(resolve_transport_node(None), DEFAULT_LOCAL_NODE); + } + // ── #117 multi-round fetch negotiation ─────────────────────────────────── /// Encode a git pkt-line: 4-byte hex length (incl. the 4 bytes) + data. diff --git a/crates/gitlawb-core/src/lib.rs b/crates/gitlawb-core/src/lib.rs index d0edec0ae..997197392 100644 --- a/crates/gitlawb-core/src/lib.rs +++ b/crates/gitlawb-core/src/lib.rs @@ -17,5 +17,50 @@ pub mod sanitize; pub mod scan_token; pub mod ucan; +/// Node URL the git transport falls back to when `GITLAWB_NODE` is unset. +/// +/// `gl` defaults to the public node instead, so the two disagree on an install +/// that never sets the variable. +pub const DEFAULT_LOCAL_NODE: &str = "http://127.0.0.1:7545"; + +/// The node `git clone` and `git push` will contact, given the raw `GITLAWB_NODE` +/// value (`None` when the variable is absent). +/// +/// `git-remote-gitlawb` calls this to pick its base URL and `gl doctor` calls it +/// to report that URL, so a diagnostic cannot describe a node the transport will +/// not use. A blank or whitespace-only value is not a configured node: treating +/// it as one gave the helper an empty base and every clone URL a missing scheme +/// and host. +pub fn resolve_transport_node(env_value: Option<&str>) -> String { + match env_value.map(str::trim) { + Some(v) if !v.is_empty() => v.to_string(), + _ => DEFAULT_LOCAL_NODE.to_string(), + } +} + pub use error::Error; pub type Result = std::result::Result; + +#[cfg(test)] +mod transport_node_tests { + use super::*; + + #[test] + fn absent_or_blank_resolves_to_the_local_default() { + for raw in [None, Some(""), Some(" "), Some("\t\n")] { + assert_eq!(resolve_transport_node(raw), DEFAULT_LOCAL_NODE, "{raw:?}"); + } + } + + #[test] + fn a_configured_value_wins_and_is_trimmed() { + assert_eq!( + resolve_transport_node(Some("https://n.example")), + "https://n.example" + ); + assert_eq!( + resolve_transport_node(Some(" https://n.example ")), + "https://n.example" + ); + } +} diff --git a/crates/gl/src/doctor.rs b/crates/gl/src/doctor.rs index 86f503344..80a34ce9b 100644 --- a/crates/gl/src/doctor.rs +++ b/crates/gl/src/doctor.rs @@ -13,7 +13,9 @@ use anyhow::Result; use clap::Args; use std::path::PathBuf; -use crate::http::NodeClient; +use gitlawb_core::resolve_transport_node; + +use crate::http::{sanitize_node_msg, NodeClient}; const PUBLIC_NODE: &str = "https://node.gitlawb.com"; const GITHUB_API_BASE: &str = "https://api.github.com"; @@ -141,27 +143,51 @@ pub async fn run(args: DoctorArgs) -> Result<()> { } // ── 3. GITLAWB_NODE env var ─────────────────────────────────────────── - match std::env::var("GITLAWB_NODE") { - // A loopback host is a legitimate setup (self-hosted node, dev - // harness) — the connectivity check below fails loudly if it is not - // actually reachable, so don't red-flag the configuration itself. - Ok(v) if is_loopback_url(&v) => { - checks.push(Check::pass( - "GITLAWB_NODE", - format!( - "{v} (local node — intentional for self-hosting/dev; unset to target the public network)" - ), - )); - } - Ok(v) if !v.is_empty() => { - checks.push(Check::pass("GITLAWB_NODE", v.to_string())); - } - _ => { - checks.push(Check::fail( - "GITLAWB_NODE", - "not set — git-remote-gitlawb will fall back to http://127.0.0.1:7545", - "export GITLAWB_NODE=https://node.gitlawb.com", - )); + // The node `git clone` / `git push` will contact is not always the node `gl` + // contacts: `--node` defaults to the public node and an explicit flag outranks + // the environment, so the two diverge whenever the variable is unset, blank, or + // overridden. Probing only `gl`'s node is what let doctor greenlight an install + // whose transport was dead, so resolve the transport's node through the same + // function the helper uses and report on that one. + let env_node = std::env::var("GITLAWB_NODE").ok(); + let transport_node = resolve_transport_node(env_node.as_deref()); + let gl_node = sanitize_node_msg(&args.node); + + if transport_node == args.node { + // gl and the transport agree, so one row covers both. A loopback host is a + // legitimate setup (self-hosted node, dev harness) and the connectivity + // check below fails loudly if it is not actually reachable, so don't + // red-flag the configuration itself. + let detail = if is_loopback_url(&transport_node) { + format!( + "{gl_node} (local node, intentional for self-hosting/dev; unset to target the public network)" + ) + } else { + gl_node.clone() + }; + checks.push(Check::pass("GITLAWB_NODE", detail)); + } else { + let verdict = probe_transport(&transport_node).await; + let detail = format!( + "git push/clone will use {} ({}); gl targets {gl_node}", + sanitize_node_msg(&transport_node), + verdict.detail + ); + // Single-quote the value: this line is printed under "Suggested fixes" for + // the user to paste into a shell, and the URL is caller-supplied. + let fix = format!("export GITLAWB_NODE='{gl_node}'"); + // Tiering follows who chose the broken value. An unset variable on a stock + // install is advisory: `gl` works, only `git push`/`git clone` do not, and + // #357 requires that install to keep exiting 0. A variable the user set to + // something unusable is a real misconfiguration. + let env_was_set = env_node + .as_deref() + .map(str::trim) + .is_some_and(|v| !v.is_empty()); + if verdict.usable || !env_was_set { + checks.push(Check::warn("GITLAWB_NODE", detail, fix)); + } else { + checks.push(Check::fail("GITLAWB_NODE", detail, fix)); } } @@ -319,6 +345,63 @@ pub async fn run(args: DoctorArgs) -> Result<()> { Ok(()) } +/// Outcome of probing the node the git transport will use. +struct TransportVerdict { + /// True only when the endpoint identified itself as a gitlawb node. A bare 200 + /// is not enough: any local service answering on that port would otherwise be + /// reported as a working transport, which is the false green this check exists + /// to catch. + usable: bool, + detail: String, +} + +/// Probe `url` and describe what is actually there. +/// +/// Deliberately not `NodeClient`: this call must not follow the user's proxy +/// (a proxy swallows a loopback probe and reports a running local node as dead) +/// and must not inherit the 30s request timeout, which stalls every stock +/// `gl doctor` run behind a hung listener. +async fn probe_transport(url: &str) -> TransportVerdict { + let client = match reqwest::Client::builder() + .no_proxy() + .timeout(std::time::Duration::from_secs(3)) + .build() + { + Ok(c) => c, + Err(e) => { + return TransportVerdict { + usable: false, + detail: format!("probe failed: {}", sanitize_node_msg(&e.to_string())), + } + } + }; + let target = format!("{}/", url.trim_end_matches('/')); + match client.get(&target).send().await { + Ok(resp) if resp.status().is_success() => { + let info = resp.json::().await.unwrap_or_default(); + if info["did"].as_str().is_some() { + TransportVerdict { + usable: true, + detail: "reachable".to_string(), + } + } else { + TransportVerdict { + usable: false, + detail: "something is listening but it is not a gitlawb node".to_string(), + } + } + } + Ok(resp) => TransportVerdict { + usable: false, + detail: format!("returned HTTP {}", resp.status()), + }, + Err(e) => TransportVerdict { + usable: false, + detail: format!("unreachable: {}", sanitize_node_msg(&e.to_string())), + }, + } +} + /// Check if a binary name exists anywhere on PATH. /// True when the rc file contains a real `unalias` command naming `gl` — /// not a comment, and not a longer word like `unalias global`. Ordering diff --git a/crates/gl/tests/doctor_transport_node.rs b/crates/gl/tests/doctor_transport_node.rs new file mode 100644 index 000000000..e302e4afe --- /dev/null +++ b/crates/gl/tests/doctor_transport_node.rs @@ -0,0 +1,204 @@ +//! `gl doctor` must diagnose the node the git transport will actually use. +//! +//! `gl`'s `--node` defaults to the public node while `git-remote-gitlawb` falls +//! back to a local one, and an explicit `--node` outranks the environment. So the +//! two disagree whenever GITLAWB_NODE is unset, blank, or overridden, and doctor's +//! own `node` row describes a URL `git clone` and `git push` never contact. + +use std::io::{Read, Write}; +use std::net::TcpListener; +use std::process::Command; + +/// Substring that marks the divergence clause. Present only when gl and the git +/// transport resolve different nodes. +const DIVERGENCE: &str = "git push/clone will use"; + +struct Run { + stdout: String, + code: Option, +} + +impl Run { + /// The single output line carrying the divergence clause, so an assertion + /// cannot be satisfied by an unrelated row that happens to print the same URL. + fn divergence_line(&self) -> Option<&str> { + self.stdout.lines().find(|l| l.contains(DIVERGENCE)) + } +} + +fn doctor(env_node: Option<&str>, gl_node: &str) -> Run { + let dir = tempfile::tempdir().unwrap(); + let mut cmd = Command::new(env!("CARGO_BIN_EXE_gl")); + cmd.args(["doctor", "--node", gl_node]) + .arg("--dir") + .arg(dir.path().join("gitlawb")) + // Keep the run off the network: doctor also probes iCaptcha and the + // GitHub release API, neither of which this test is about. + .env("GITLAWB_ICAPTCHA_URL", "http://127.0.0.1:1"); + match env_node { + Some(v) => cmd.env("GITLAWB_NODE", v), + None => cmd.env_remove("GITLAWB_NODE"), + }; + let out = cmd.output().unwrap(); + Run { + stdout: String::from_utf8_lossy(&out.stdout).into_owned(), + code: out.status.code(), + } +} + +/// GITLAWB_NODE unset: the helper falls back to the local node, gl targets the +/// public one, so the row must name the local node as the transport target. +#[test] +fn unset_env_reports_the_transport_node() { + let r = doctor(None, "http://127.0.0.1:1"); + let line = r + .divergence_line() + .unwrap_or_else(|| panic!("no divergence row, got:\n{}", r.stdout)); + assert!( + line.contains(gitlawb_core::DEFAULT_LOCAL_NODE), + "divergence row must name the helper's node, got: {line}" + ); +} + +/// A blank value is not a configured node. The helper must fall back exactly as +/// it does when the variable is absent, and doctor must say so. +#[test] +fn blank_env_is_treated_as_unset() { + for blank in ["", " ", "\t"] { + let r = doctor(Some(blank), "http://127.0.0.1:1"); + let line = r + .divergence_line() + .unwrap_or_else(|| panic!("no divergence row for {blank:?}, got:\n{}", r.stdout)); + assert!( + line.contains(gitlawb_core::DEFAULT_LOCAL_NODE), + "blank env must resolve to the local default, got: {line}" + ); + } +} + +/// An explicit `--node` outranks the environment, so the transport still goes +/// somewhere gl never probes. This state produced no row at all before. +#[test] +fn explicit_node_flag_overriding_env_is_still_reported() { + let r = doctor(Some("http://127.0.0.1:2"), "http://127.0.0.1:1"); + let line = r + .divergence_line() + .unwrap_or_else(|| panic!("no divergence row, got:\n{}", r.stdout)); + assert!( + line.contains("127.0.0.1:2"), + "row must name the env-configured transport node, got: {line}" + ); +} + +/// No divergence when both resolve the same node: one row, no clause. +#[test] +fn agreeing_env_and_flag_produce_no_divergence_row() { + let r = doctor(Some("http://127.0.0.1:1"), "http://127.0.0.1:1"); + assert!( + r.divergence_line().is_none(), + "gl and the transport agree; expected no divergence row, got:\n{}", + r.stdout + ); +} + +/// The bug this check exists to prevent, one level down: something answering 200 +/// on the transport port is not proof a gitlawb node is there. Reporting it as a +/// healthy transport is the same false green doctor already shipped once. +#[test] +fn a_non_gitlawb_200_is_not_a_healthy_transport() { + let Ok(listener) = TcpListener::bind("127.0.0.1:0") else { + return; + }; + let port = listener.local_addr().unwrap().port(); + std::thread::spawn(move || { + for stream in listener.incoming().take(1) { + let Ok(mut s) = stream else { continue }; + // Read the request first. Writing the response without draining the + // request leaves the client seeing a reset rather than a 200, which + // silently turns this test into a probe of the unreachable path. + let mut buf = [0u8; 1024]; + let _ = s.read(&mut buf); + let body = b"not a node"; + let _ = write!( + s, + "HTTP/1.1 200 OK\r\nContent-Type: text/html\r\nContent-Length: {}\r\nConnection: close\r\n\r\n", + body.len() + ); + let _ = s.write_all(body); + let _ = s.flush(); + } + }); + let r = doctor( + Some(&format!("http://127.0.0.1:{port}")), + "http://127.0.0.1:1", + ); + let line = r + .divergence_line() + .unwrap_or_else(|| panic!("no divergence row, got:\n{}", r.stdout)); + // Positive assertion: only the identity check can produce this text, so the + // test cannot pass by the probe quietly failing instead. + assert!( + line.contains("not a gitlawb node"), + "a non-gitlawb 200 must be called out, not reported reachable, got: {line}" + ); +} + +/// Control characters from an attacker-influenceable node URL must not reach the +/// terminal raw, including inside the paste-ready remedy line. +/// +/// Scoped to the rows this check owns. The unchanged `node` row at doctor.rs:180, +/// :199 and :206 still interpolates the raw URL and is a separate pre-existing +/// leak, so asserting over the whole of stdout would fail for a reason this +/// change did not introduce and cannot fix without touching adjacent code. +#[test] +fn control_characters_in_the_node_url_are_stripped() { + let r = doctor(None, "http://127.0.0.1:1/\u{1b}[31m\u{7}\u{202e}"); + let owned: String = r + .stdout + .lines() + .filter(|l| l.contains(DIVERGENCE) || l.trim_start().starts_with("GITLAWB_NODE:")) + .collect::>() + .join("\n"); + assert!( + owned.contains(DIVERGENCE), + "expected the divergence row and its remedy, got:\n{}", + r.stdout + ); + assert!( + !owned.contains('\u{1b}') && !owned.contains('\u{7}') && !owned.contains('\u{202e}'), + "raw control characters reached stdout:\n{owned:?}" + ); +} + +/// #357's tiering: an unset variable is advisory (gl still works), a variable the +/// user set to something unusable is a real failure. Nothing pinned this, and the +/// difference decides whether a stock install exits non-zero once #391 lands. +#[test] +fn unset_env_warns_while_a_broken_configured_node_fails() { + let stock = doctor(None, "http://127.0.0.1:1"); + let line = stock.divergence_line().unwrap(); + assert!( + line.trim_start().starts_with('\u{26a0}'), + "an unset variable must stay advisory on a stock install, got: {line}" + ); + + let misconfigured = doctor(Some("http://127.0.0.1:2"), "http://127.0.0.1:1"); + let line = misconfigured.divergence_line().unwrap(); + assert!( + line.trim_start().starts_with('\u{2717}'), + "a configured but unusable node must fail, got: {line}" + ); +} + +/// #357's constraint: doctor must not start failing a stock install just because +/// it now says more. Nothing pinned this before. +#[test] +fn doctor_still_exits_zero() { + let r = doctor(None, "http://127.0.0.1:1"); + assert_eq!( + r.code, + Some(0), + "doctor must keep exiting 0, got:\n{}", + r.stdout + ); +}