diff --git a/crates/gl/src/agent.rs b/crates/gl/src/agent.rs index d72c230a..f7042f57 100644 --- a/crates/gl/src/agent.rs +++ b/crates/gl/src/agent.rs @@ -2,7 +2,6 @@ use anyhow::{Context, Result}; use clap::{Args, Subcommand}; -use serde_json::Value; use std::path::PathBuf; use crate::http::NodeClient; @@ -57,24 +56,16 @@ async fn cmd_list(node: String, capability: Option) -> Result<()> { .get(&path) .await .context("failed to connect to node")?; - let status = resp.status(); - - if status == reqwest::StatusCode::NOT_FOUND { + // Keep the bespoke 404 hint (older node without the agents API) ahead of + // read_json; every other status routes through it (capped + sanitized error). + if resp.status() == reqwest::StatusCode::NOT_FOUND { anyhow::bail!( "this node does not yet support the agents API (v0.3+)\n\ upgrade the node or check GITLAWB_NODE is pointing to the right server" ); } - let body: Value = resp - .json() - .await - .context("invalid JSON from node — is GITLAWB_NODE correct?")?; - - if !status.is_success() { - let msg = body["message"].as_str().unwrap_or("unknown error"); - anyhow::bail!("list agents failed ({status}): {msg}"); - } + let body = crate::http::read_json(resp, "list agents").await?; let agents = body["agents"].as_array().cloned().unwrap_or_default(); @@ -117,24 +108,15 @@ async fn cmd_show(did: String, node: String) -> Result<()> { .get(&format!("/api/v1/agents/{did}")) .await .context("failed to connect to node")?; - let status = resp.status(); - - if status == reqwest::StatusCode::NOT_FOUND { + // Keep the bespoke 404 hint ahead of read_json; other statuses route through it. + if resp.status() == reqwest::StatusCode::NOT_FOUND { anyhow::bail!( "agent not found, or this node does not yet support the agents API (v0.3+)\n\ upgrade the node or check GITLAWB_NODE is pointing to the right server" ); } - let agent: Value = resp - .json() - .await - .context("invalid JSON from node — is GITLAWB_NODE correct?")?; - - if !status.is_success() { - let msg = agent["message"].as_str().unwrap_or("unknown error"); - anyhow::bail!("agent not found ({status}): {msg}"); - } + let agent = crate::http::read_json(resp, "agent").await?; let full_did = agent["did"].as_str().unwrap_or("?"); let trust = agent["trust_score"].as_f64().unwrap_or(0.0); @@ -215,11 +197,14 @@ mod tests { .with_status(404) .with_header("content-type", "application/json") .with_body(r#"{"message":"not found"}"#) + .expect(1) .create_async() .await; let err = cmd_list(server.url(), None).await.unwrap_err(); assert!(err.to_string().contains("agents API")); + // Prove the mocked route was actually requested; a non-matching request (mockito's 501, also non-2xx) would otherwise satisfy the error assertion vacuously. + _m.assert_async().await; } #[tokio::test] @@ -230,11 +215,14 @@ mod tests { .with_status(500) .with_header("content-type", "application/json") .with_body(r#"{"message":"internal error"}"#) + .expect(1) .create_async() .await; let err = cmd_list(server.url(), None).await.unwrap_err(); assert!(err.to_string().contains("list agents failed")); + // Prove the mocked route was actually requested; a non-matching request (mockito's 501, also non-2xx) would otherwise satisfy the error assertion vacuously. + _m.assert_async().await; } #[tokio::test] @@ -261,6 +249,7 @@ mod tests { .with_status(404) .with_header("content-type", "application/json") .with_body(r#"{"message":"agent not found"}"#) + .expect(1) .create_async() .await; @@ -268,6 +257,8 @@ mod tests { .await .unwrap_err(); assert!(err.to_string().contains("agents API") || err.to_string().contains("not found")); + // Prove the mocked route was actually requested; a non-matching request (mockito's 501, also non-2xx) would otherwise satisfy the error assertion vacuously. + _m.assert_async().await; } #[tokio::test] diff --git a/crates/gl/src/bounty.rs b/crates/gl/src/bounty.rs index ccc658a1..7f5a0a5c 100644 --- a/crates/gl/src/bounty.rs +++ b/crates/gl/src/bounty.rs @@ -2,7 +2,6 @@ use anyhow::{Context, Result}; use clap::{Args, Subcommand}; -use serde_json::Value; use std::path::PathBuf; use crate::http::NodeClient; @@ -189,13 +188,7 @@ async fn cmd_create( .await .context("failed to connect to node")?; - let status = resp.status(); - let body: Value = resp.json().await.unwrap_or_default(); - - if !status.is_success() { - let msg = body["message"].as_str().unwrap_or("unknown error"); - anyhow::bail!("create bounty failed ({status}): {msg}"); - } + let body = crate::http::read_json(resp, "create bounty").await?; let id = body["id"].as_str().unwrap_or("?"); println!("Bounty created: {id}"); @@ -231,11 +224,14 @@ async fn cmd_list( u }; - let resp = client - .get_authed(&url) - .await - .context("failed to connect to node")?; - let body: Value = resp.json().await.unwrap_or_default(); + let body = crate::http::read_json( + client + .get_authed(&url) + .await + .context("failed to connect to node")?, + "bounties", + ) + .await?; let bounties = body["bounties"].as_array(); if let Some(arr) = bounties { @@ -259,13 +255,7 @@ async fn cmd_show(id: String, node: String, dir: Option) -> Result<()> let client = signed_client(&node, dir.as_deref()); let resp = client.get_authed(&format!("/api/v1/bounties/{id}")).await?; - let status = resp.status(); - let body: Value = resp.json().await.unwrap_or_default(); - - if !status.is_success() { - let msg = body["message"].as_str().unwrap_or("not found"); - anyhow::bail!("bounty not found: {msg}"); - } + let body = crate::http::read_json(resp, "show bounty").await?; println!("{}", serde_json::to_string_pretty(&body)?); Ok(()) @@ -289,13 +279,7 @@ async fn cmd_claim( ) .await?; - let status = resp.status(); - let body: Value = resp.json().await.unwrap_or_default(); - - if !status.is_success() { - let msg = body["message"].as_str().unwrap_or("unknown error"); - anyhow::bail!("claim failed ({status}): {msg}"); - } + let _ = crate::http::read_json(resp, "claim").await?; println!("Bounty {id} claimed. Deadline starts now."); Ok(()) @@ -314,13 +298,7 @@ async fn cmd_submit(id: String, pr: String, node: String, dir: Option) ) .await?; - let status = resp.status(); - let body: Value = resp.json().await.unwrap_or_default(); - - if !status.is_success() { - let msg = body["message"].as_str().unwrap_or("unknown error"); - anyhow::bail!("submit failed ({status}): {msg}"); - } + let _ = crate::http::read_json(resp, "submit").await?; println!("Bounty {id} submitted with PR {pr}. Awaiting creator approval."); Ok(()) @@ -344,13 +322,7 @@ async fn cmd_approve( ) .await?; - let status = resp.status(); - let body: Value = resp.json().await.unwrap_or_default(); - - if !status.is_success() { - let msg = body["message"].as_str().unwrap_or("unknown error"); - anyhow::bail!("approve failed ({status}): {msg}"); - } + let _ = crate::http::read_json(resp, "approve").await?; println!("Bounty {id} approved! Payout released to agent."); Ok(()) @@ -369,13 +341,7 @@ async fn cmd_cancel(id: String, node: String, dir: Option) -> Result<() ) .await?; - let status = resp.status(); - let body: Value = resp.json().await.unwrap_or_default(); - - if !status.is_success() { - let msg = body["message"].as_str().unwrap_or("unknown error"); - anyhow::bail!("cancel failed ({status}): {msg}"); - } + let _ = crate::http::read_json(resp, "cancel").await?; println!("Bounty {id} cancelled. Tokens refunded."); Ok(()) @@ -383,8 +349,11 @@ async fn cmd_cancel(id: String, node: String, dir: Option) -> Result<() async fn cmd_stats(node: String) -> Result<()> { let client = NodeClient::new(&node, None); - let resp = client.get("/api/v1/bounties/stats").await?; - let body: Value = resp.json().await.unwrap_or_default(); + // Route through read_json so a node denial (403/404/5xx, incl. a non-JSON + // body) surfaces as an error instead of a fake `Bounty Stats` with every + // counter zeroed (#186, the denial-as-success class #123 fixes elsewhere). + let body = + crate::http::read_json(client.get("/api/v1/bounties/stats").await?, "bounty stats").await?; let open = body["open"].as_i64().unwrap_or(0); let claimed = body["claimed"].as_i64().unwrap_or(0); @@ -521,6 +490,7 @@ mod tests { .with_status(404) .with_header("content-type", "application/json") .with_body(r#"{"message":"not found"}"#) + .expect(1) .create_async() .await; @@ -528,6 +498,8 @@ mod tests { .await .unwrap_err(); assert!(err.to_string().contains("not found")); + // Prove the mocked route was actually requested; a non-matching request (mockito's 501, also non-2xx) would otherwise satisfy the error assertion vacuously. + _m.assert_async().await; } #[tokio::test] @@ -544,4 +516,78 @@ mod tests { cmd_stats(server.url()).await.unwrap(); } + + /// #186 (F1): a node denial on the stats endpoint must surface as an error, + /// not print a fake `Bounty Stats` with every counter zeroed. RED before the + /// fix (unwrap_or_default + no status check -> Ok with zeros), GREEN after + /// (read_json bails on the non-2xx). + #[tokio::test] + async fn cmd_stats_surfaces_denial_not_fake_result() { + let mut server = mockito::Server::new_async().await; + let _m = server + .mock("GET", mockito::Matcher::Regex(r"/stats$".to_string())) + .with_status(403) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"forbidden"}"#) + .create_async() + .await; + + let err = cmd_stats(server.url()) + .await + .expect_err("a 403 must be an error, not a zeroed fake result"); + assert!(err.to_string().contains("403"), "err={err}"); + } + + /// #186 (F1): the same denial-as-error behavior across the non-2xx response + /// classes, not just 403 — a 404 and a 500 (incl. a non-JSON body) must each + /// surface as an error with the status, never a zeroed fake result. + #[tokio::test] + async fn cmd_stats_surfaces_denial_across_status_classes() { + for (status, body, json) in [ + (404, r#"{"message":"not found"}"#, true), + (500, "internal boom", false), + ] { + let mut server = mockito::Server::new_async().await; + let mut m = server + .mock("GET", mockito::Matcher::Regex(r"/stats$".to_string())) + .with_status(status) + .with_body(body); + if json { + m = m.with_header("content-type", "application/json"); + } + let _m = m.create_async().await; + + let err = cmd_stats(server.url()).await.unwrap_err().to_string(); + assert!( + err.contains(&status.to_string()), + "status {status} must surface as an error, got: {err}" + ); + } + } + + #[tokio::test] + async fn cmd_list_repo_scoped_surfaces_denial_not_empty() { + // `gl bounty list --repo owner/name` hits the gated /repos/{o}/{n}/bounties; + // a 404 must Err, not silently print nothing (INV-8). + let mut server = mockito::Server::new_async().await; + let _m = server + .mock( + "GET", + mockito::Matcher::Regex(r"/repos/alice/secret/bounties".to_string()), + ) + .with_status(404) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"repository 'alice/secret' not found"}"#) + .expect(1) + .create_async() + .await; + let result = cmd_list(Some("alice/secret".to_string()), None, server.url(), None).await; + assert!( + result.is_err(), + "bounty list --repo must Err on a gated 404" + ); + // Prove the gated repo-scoped path was actually requested: without this, + // an unmatched route (mockito's 501, also non-2xx) would satisfy is_err(). + _m.assert_async().await; + } } diff --git a/crates/gl/src/cert.rs b/crates/gl/src/cert.rs index 87ad5aec..8e3a8242 100644 --- a/crates/gl/src/cert.rs +++ b/crates/gl/src/cert.rs @@ -4,7 +4,6 @@ use anyhow::{Context, Result}; use clap::{Args, Subcommand}; -use serde_json::Value; use std::path::PathBuf; use crate::http::NodeClient; @@ -82,12 +81,7 @@ async fn resolve_repo( did.split(':').next_back().unwrap_or(&did).to_string() } else { let client = signed_client(node, dir); - let info: Value = client - .get_authed("/") - .await? - .json() - .await - .context("failed to fetch node info")?; + let info = crate::http::read_json(client.get_authed("/").await?, "node info").await?; let did = info["did"].as_str().context("node info missing 'did'")?; did.split(':').next_back().unwrap_or(did).to_string() }; @@ -100,12 +94,7 @@ async fn cmd_list(repo: String, node: String, dir: Option) -> Result<() let client = signed_client(&node, dir.as_deref()); let path = format!("/api/v1/repos/{owner}/{name}/certs"); - let resp: Value = client - .get_authed(&path) - .await? - .json() - .await - .context("failed to list certificates")?; + let resp = crate::http::read_json(client.get_authed(&path).await?, "certificates").await?; let certs = resp["certificates"].as_array().cloned().unwrap_or_default(); @@ -139,14 +128,10 @@ async fn cmd_show( let client = signed_client(&node, dir.as_deref()); let id = resolve_cert_id(&client, &owner, &name, &id).await?; - // Fetch the certificate + // Fetch the certificate. read_json checks status first and surfaces the node's + // capped+sanitized message on a non-2xx (a bounded error read, not the whole body). let path = format!("/api/v1/repos/{owner}/{name}/certs/{id}"); - let resp = client - .get_authed(&path) - .await? - .error_for_status() - .context("certificate not found")?; - let cert: Value = resp.json().await.context("certificate not found")?; + let cert = crate::http::read_json(client.get_authed(&path).await?, "certificate").await?; let cert_id = cert["id"].as_str().unwrap_or("?"); let ref_name = cert["ref_name"].as_str().unwrap_or("?"); @@ -191,27 +176,20 @@ async fn cmd_show( // Contextual only — the verdict above stands on its own, so a node-info // hiccup here must not turn a successfully displayed certificate into an - // error exit. - let current_node_did = match client.get("/").await { - Ok(resp) => resp - .json::() - .await - .ok() - .and_then(|info| info["did"].as_str().map(str::to_string)), - Err(_) => None, + // error exit. Route the lookup through read_json so a denial or a capped + // error body yields a reportable reason instead of an opaque None, and keep + // "carried no DID" distinct from "the lookup failed": an empty DID must not + // reach the comparison and fabricate a mismatch warning. + let current: std::result::Result = match client.get("/").await { + Ok(resp) => match crate::http::read_json(resp, "node info").await { + Ok(info) => did_from_node_info(&info), + Err(e) => Err(e.to_string()), + }, + Err(e) => Err(e.to_string()), }; - match current_node_did.as_deref() { - Some(current) if current == node_did => { - println!(" Issuing node DID matches the node being queried."); - } - Some(current) => { - println!(" WARNING: Certificate node DID ({node_did}) does not match"); - println!(" current node DID ({current})."); - println!(" This certificate was issued by a different node."); - } - None => { - println!(" NOTE: could not fetch current node info — skipping node-DID comparison."); - } + let current_node_did = current.as_ref().ok().cloned(); + for line in did_check_report(¤t, node_did) { + println!("{line}"); } if require_valid { @@ -239,6 +217,46 @@ async fn cmd_show( Ok(()) } +/// Pull the node's DID out of a `GET /` body for the comparison in `cmd_show`. +/// +/// A missing OR empty `did` is a failure, not a value: letting `""` through +/// would reach the comparison and print a mismatch WARNING against a DID the +/// node never claimed, which reads as "issued by a different node" when the +/// truth is that the lookup told us nothing. +fn did_from_node_info(info: &serde_json::Value) -> std::result::Result { + match info["did"].as_str() { + Some(did) if !did.is_empty() => Ok(did.to_string()), + _ => Err("node info response carried no DID".to_string()), + } +} + +/// Select the report lines for the node-DID comparison in `cmd_show`. +/// +/// `current` is the current node's DID, or the reason it could not be +/// determined. A comparison verdict (match or WARNING) is only produced when a +/// real DID was obtained; otherwise the report degrades to a could-not-compare +/// hint naming the reason, plus the offline-verification guidance. The signature +/// verdict printed above stands on its own either way — this block only answers +/// *which* node issued the certificate. +fn did_check_report(current: &std::result::Result, node_did: &str) -> Vec { + match current { + Ok(current) if current == node_did => { + vec![" Issuing node DID matches the node being queried.".to_string()] + } + Ok(current) => vec![ + format!(" WARNING: Certificate node DID ({node_did}) does not match"), + format!(" current node DID ({current})."), + " This certificate was issued by a different node.".to_string(), + ], + Err(reason) => vec![ + format!(" Could not fetch the current node's DID ({reason}), so the comparison"), + " with the certificate's node DID is unavailable.".to_string(), + " To verify offline, use the node's Ed25519 public key derived from:".to_string(), + format!(" did:key → {node_did}"), + ], + } +} + /// Rebuild the node's canonical signing payload (field order must match /// gitlawb-node/src/cert.rs::issue_ref_certificate exactly) and verify the /// certificate's Ed25519 signature against the key embedded in `node_did`. @@ -291,14 +309,7 @@ async fn resolve_cert_id(client: &NodeClient, owner: &str, name: &str, id: &str) } let path = format!("/api/v1/repos/{owner}/{name}/certs?prefix={id}"); - let resp: Value = client - .get_authed(&path) - .await? - .error_for_status() - .context("failed to list certificates")? - .json() - .await - .context("failed to list certificates")?; + let resp = crate::http::read_json(client.get_authed(&path).await?, "certificates").await?; let certs = resp["certificates"].as_array().cloned().unwrap_or_default(); let matches: Vec = certs @@ -397,4 +408,558 @@ mod tests { ); assert!(garbage.is_err(), "malformed signature must not verify"); } + + #[tokio::test] + async fn cmd_list_surfaces_denial_not_empty() { + // A gated 404 on the repo-scoped certs read must Err, not print "No certificates". + let mut server = mockito::Server::new_async().await; + let _m = server + .mock( + "GET", + mockito::Matcher::Regex(r"^/api/v1/repos/alice/secret/certs$".to_string()), + ) + .with_status(404) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"repository 'alice/secret' not found"}"#) + .expect(1) + .create_async() + .await; + let result = cmd_list("alice/secret".to_string(), server.url(), None).await; + assert!(result.is_err(), "cert list must Err on a gated 404"); + // Prove the gated certs path was actually requested: without this, an + // unmatched route (mockito's 501, also non-2xx) would satisfy is_err(). + _m.assert_async().await; + } + + #[tokio::test] + async fn resolve_repo_surfaces_denial() { + // A slash-free repo with an empty identity dir forces the GET / node-info + // fetch. A gated 404 there must Err (surfacing the status), proving the + // read_json conversion is load-bearing rather than silently ignored. + let mut server = mockito::Server::new_async().await; + let dir = tempfile::TempDir::new().unwrap(); // empty, no identity.pem, forces the GET / branch + let _m = server + .mock("GET", "/") + .with_status(404) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"denied"}"#) + .expect(1) + .create_async() + .await; + let err = resolve_repo("noslash", &server.url(), Some(dir.path())) + .await + .unwrap_err(); + assert!(err.to_string().contains("404"), "got: {err}"); + _m.assert_async().await; + } + + // The `GET /` node-info lookup after the cert loads is a fail-soft diagnostic: + // a response-level failure degrades to a could-not-compare hint and the command + // completes Ok, never a fabricated mismatch warning and never a fatal Err. The + // cert fetch itself stays fail-closed. A >=36-char id skips resolve_cert_id so + // only two mocks are needed. + #[tokio::test] + async fn cmd_show_completes_with_degraded_hint_when_node_info_denied() { + let mut server = mockito::Server::new_async().await; + let long_id = "a".repeat(36); + let _cert = server + .mock( + "GET", + format!("/api/v1/repos/alice/secret/certs/{long_id}").as_str(), + ) + .with_status(200) + .with_header("content-type", "application/json") + .with_body( + r#"{"id":"c1","ref_name":"refs/heads/main","old_sha":"0","new_sha":"1","pusher_did":"p","node_did":"n","signature":"s","issued_at":"2026-01-01T00:00:00Z"}"#, + ) + .create_async() + .await; + let _root = server + .mock("GET", "/") + .with_status(403) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"denied"}"#) + .expect(1) + .create_async() + .await; + + let result = cmd_show( + "alice/secret".to_string(), + long_id, + server.url(), + None, + false, + None, + ) + .await; + assert!( + result.is_ok(), + "cert show must complete despite a denied node-info lookup: {result:?}" + ); + _cert.assert_async().await; + _root.assert_async().await; + } + + #[tokio::test] + async fn cmd_show_degrades_on_malformed_node_info() { + // A 2xx node-info body that fails to parse degrades the same way a denial + // does: hint printed, command completes Ok. + let mut server = mockito::Server::new_async().await; + let long_id = "a".repeat(36); + let _cert = server + .mock( + "GET", + format!("/api/v1/repos/alice/secret/certs/{long_id}").as_str(), + ) + .with_status(200) + .with_header("content-type", "application/json") + .with_body( + r#"{"id":"c1","ref_name":"refs/heads/main","old_sha":"0","new_sha":"1","pusher_did":"p","node_did":"n","signature":"s","issued_at":"2026-01-01T00:00:00Z"}"#, + ) + .create_async() + .await; + let _root = server + .mock("GET", "/") + .with_status(200) + .with_body("not json") + .expect(1) + .create_async() + .await; + + let result = cmd_show( + "alice/secret".to_string(), + long_id, + server.url(), + None, + false, + None, + ) + .await; + assert!( + result.is_ok(), + "cert show must complete despite malformed node info: {result:?}" + ); + _cert.assert_async().await; + _root.assert_async().await; + } + + #[tokio::test] + async fn cmd_show_degrades_when_node_info_lacks_did() { + // A 2xx node-info body with no `did` routes to the degraded hint, not a + // fabricated empty-DID mismatch warning; the command completes Ok. + let mut server = mockito::Server::new_async().await; + let long_id = "a".repeat(36); + let _cert = server + .mock( + "GET", + format!("/api/v1/repos/alice/secret/certs/{long_id}").as_str(), + ) + .with_status(200) + .with_header("content-type", "application/json") + .with_body( + r#"{"id":"c1","ref_name":"refs/heads/main","old_sha":"0","new_sha":"1","pusher_did":"p","node_did":"n","signature":"s","issued_at":"2026-01-01T00:00:00Z"}"#, + ) + .create_async() + .await; + let _root = server + .mock("GET", "/") + .with_status(200) + .with_header("content-type", "application/json") + .with_body("{}") + .expect(1) + .create_async() + .await; + + let result = cmd_show( + "alice/secret".to_string(), + long_id, + server.url(), + None, + false, + None, + ) + .await; + assert!( + result.is_ok(), + "cert show must complete when node info lacks a DID: {result:?}" + ); + _cert.assert_async().await; + _root.assert_async().await; + } + + // Must-not case: the certificate fetch itself stays fail-closed. A gated 404 + // aborts the command with the status surfaced, and the node-info lookup is + // never reached (the expect(0) assert proves it never ran). + #[tokio::test] + async fn cmd_show_surfaces_denied_certificate() { + let mut server = mockito::Server::new_async().await; + let long_id = "a".repeat(36); + let _cert = server + .mock( + "GET", + format!("/api/v1/repos/alice/secret/certs/{long_id}").as_str(), + ) + .with_status(404) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"repository not found"}"#) + .expect(1) + .create_async() + .await; + let _root = server + .mock("GET", "/") + .with_status(200) + .with_header("content-type", "application/json") + .with_body(r#"{"did":"n"}"#) + .expect(0) + .create_async() + .await; + + let err = cmd_show( + "alice/secret".to_string(), + long_id, + server.url(), + None, + false, + None, + ) + .await + .unwrap_err(); + assert!(err.to_string().contains("404"), "got: {err}"); + _cert.assert_async().await; + _root.assert_async().await; + } + + #[tokio::test] + async fn cmd_show_reports_matching_node_did() { + // Pins the unchanged success path: node info fetched, DIDs compared, Ok. + let mut server = mockito::Server::new_async().await; + let long_id = "a".repeat(36); + let _cert = server + .mock( + "GET", + format!("/api/v1/repos/alice/secret/certs/{long_id}").as_str(), + ) + .with_status(200) + .with_header("content-type", "application/json") + .with_body( + r#"{"id":"c1","ref_name":"refs/heads/main","old_sha":"0","new_sha":"1","pusher_did":"p","node_did":"n","signature":"s","issued_at":"2026-01-01T00:00:00Z"}"#, + ) + .create_async() + .await; + let _root = server + .mock("GET", "/") + .with_status(200) + .with_header("content-type", "application/json") + .with_body(r#"{"did":"n"}"#) + .expect(1) + .create_async() + .await; + + let result = cmd_show( + "alice/secret".to_string(), + long_id, + server.url(), + None, + false, + None, + ) + .await; + assert!(result.is_ok(), "got: {result:?}"); + _cert.assert_async().await; + _root.assert_async().await; + } + + #[tokio::test] + async fn cmd_show_warns_on_mismatching_node_did() { + // A real, differing node DID drives the WARNING branch end to end; the + // command still completes Ok. + let mut server = mockito::Server::new_async().await; + let long_id = "a".repeat(36); + let _cert = server + .mock( + "GET", + format!("/api/v1/repos/alice/secret/certs/{long_id}").as_str(), + ) + .with_status(200) + .with_header("content-type", "application/json") + .with_body( + r#"{"id":"c1","ref_name":"refs/heads/main","old_sha":"0","new_sha":"1","pusher_did":"p","node_did":"n","signature":"s","issued_at":"2026-01-01T00:00:00Z"}"#, + ) + .create_async() + .await; + let _root = server + .mock("GET", "/") + .with_status(200) + .with_header("content-type", "application/json") + .with_body(r#"{"did":"did:key:other"}"#) + .expect(1) + .create_async() + .await; + + let result = cmd_show( + "alice/secret".to_string(), + long_id, + server.url(), + None, + false, + None, + ) + .await; + assert!(result.is_ok(), "got: {result:?}"); + _cert.assert_async().await; + _root.assert_async().await; + } + + // ── --verify issuer anchoring ──────────────────────────────────────────── + // + // Every other cmd_show test passes require_valid=false, so the whole + // `if require_valid` block went unexecuted. It is the security-bearing half of + // the command: a valid signature only proves the certificate is internally + // consistent (a hostile node can mint a keypair, name it in node_did, and + // self-sign), so --verify must additionally anchor the issuer to a DID the + // caller trusts. These drive all four outcomes plus the must-not case. + // + // The certificate must carry a REAL signature over the canonical payload: + // with a bogus one, --verify bails on the signature and never reaches the + // anchoring, which would make every assertion below vacuous. + fn signed_cert(node_kp: &gitlawb_core::identity::Keypair) -> (String, String) { + let node_did = node_kp.did().as_str().to_string(); + let id = "a".repeat(36); // >= 36 chars skips resolve_cert_id's prefix lookup + let payload = serde_json::json!({ + "repo_id": "repo-1", + "ref": "refs/heads/main", + "old": "0".repeat(40), + "new": "b".repeat(40), + "pusher": "did:key:z6MkPusher", + "node": node_did, + "ts": "2026-07-22T00:00:00+00:00", + }); + let sig = node_kp.sign_b64(&serde_json::to_vec(&payload).unwrap()); + let body = serde_json::json!({ + "id": id, + "repo_id": "repo-1", + "ref_name": "refs/heads/main", + "old_sha": "0".repeat(40), + "new_sha": "b".repeat(40), + "pusher_did": "did:key:z6MkPusher", + "node_did": node_did, + "signature": sig, + "issued_at": "2026-07-22T00:00:00+00:00", + }) + .to_string(); + (id, body) + } + + /// Mount the cert fetch, plus a `GET /` answering with `node_info` (a JSON + /// body) or a denial when `None`. + async fn cert_server( + server: &mut mockito::Server, + id: &str, + cert_body: &str, + node_info: Option<&str>, + ) -> (mockito::Mock, mockito::Mock) { + let cert = server + .mock("GET", format!("/api/v1/repos/alice/r/certs/{id}").as_str()) + .with_status(200) + .with_header("content-type", "application/json") + .with_body(cert_body) + .expect(1) + .create_async() + .await; + let root = match node_info { + Some(b) => { + server + .mock("GET", "/") + .with_status(200) + .with_header("content-type", "application/json") + .with_body(b) + .create_async() + .await + } + None => { + server + .mock("GET", "/") + .with_status(403) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"denied"}"#) + .create_async() + .await + } + }; + (cert, root) + } + + #[tokio::test] + async fn verify_ok_when_issuing_node_is_the_queried_node() { + let kp = gitlawb_core::identity::Keypair::generate(); + let (id, body) = signed_cert(&kp); + let info = format!(r#"{{"did":"{}"}}"#, kp.did().as_str()); + let mut server = mockito::Server::new_async().await; + let (cert, _root) = cert_server(&mut server, &id, &body, Some(&info)).await; + + let got = cmd_show("alice/r".to_string(), id, server.url(), None, true, None).await; + assert!( + got.is_ok(), + "valid sig + matching issuer must pass: {got:?}" + ); + cert.assert_async().await; + } + + #[tokio::test] + async fn verify_bails_when_issuer_is_a_different_node() { + // The self-signing-hostile-node case: the signature verifies against the + // key the cert names, but that key is not the node we queried. + let kp = gitlawb_core::identity::Keypair::generate(); + let (id, body) = signed_cert(&kp); + let other = gitlawb_core::identity::Keypair::generate(); + let info = format!(r#"{{"did":"{}"}}"#, other.did().as_str()); + let mut server = mockito::Server::new_async().await; + let (cert, _root) = cert_server(&mut server, &id, &body, Some(&info)).await; + + let err = cmd_show("alice/r".to_string(), id, server.url(), None, true, None) + .await + .unwrap_err() + .to_string(); + assert!(err.contains("expected issuer"), "got: {err}"); + cert.assert_async().await; + } + + #[tokio::test] + async fn verify_bails_when_node_info_denied_and_no_expect_node() { + // Fail closed: with no trusted DID to anchor against, --verify must NOT + // fall through to a pass just because the signature checked out. + let kp = gitlawb_core::identity::Keypair::generate(); + let (id, body) = signed_cert(&kp); + let mut server = mockito::Server::new_async().await; + let (cert, _root) = cert_server(&mut server, &id, &body, None).await; + + let err = cmd_show("alice/r".to_string(), id, server.url(), None, true, None) + .await + .unwrap_err() + .to_string(); + assert!(err.contains("cannot anchor the issuer"), "got: {err}"); + cert.assert_async().await; + } + + #[tokio::test] + async fn verify_ok_when_expect_node_anchors_a_denied_lookup() { + // --expect-node supplies the trust anchor the denied lookup could not, so + // the same denial that fails the case above now passes. + let kp = gitlawb_core::identity::Keypair::generate(); + let (id, body) = signed_cert(&kp); + let mut server = mockito::Server::new_async().await; + let (cert, _root) = cert_server(&mut server, &id, &body, None).await; + + let got = cmd_show( + "alice/r".to_string(), + id, + server.url(), + None, + true, + Some(kp.did().as_str().to_string()), + ) + .await; + assert!(got.is_ok(), "explicit anchor must pass: {got:?}"); + cert.assert_async().await; + } + + #[tokio::test] + async fn verify_bails_on_a_bad_signature_before_anchoring() { + // The must-not: a forged certificate naming the queried node must fail on + // the signature, even though its issuer would otherwise anchor cleanly. + let kp = gitlawb_core::identity::Keypair::generate(); + let (id, body) = signed_cert(&kp); + let forged = body.replace(&"b".repeat(40), &"c".repeat(40)); + let info = format!(r#"{{"did":"{}"}}"#, kp.did().as_str()); + let mut server = mockito::Server::new_async().await; + let (cert, _root) = cert_server(&mut server, &id, &forged, Some(&info)).await; + + let err = cmd_show("alice/r".to_string(), id, server.url(), None, true, None) + .await + .unwrap_err() + .to_string(); + assert!(err.contains("signature did not verify"), "got: {err}"); + cert.assert_async().await; + } + + // The must-not case for the DID extraction: an empty or missing `did` has to + // become a could-not-compare reason. If it leaks through as a value, the + // comparison below fabricates a mismatch WARNING against a DID the node + // never claimed. + #[test] + fn did_from_node_info_rejects_empty_and_missing() { + assert!(did_from_node_info(&serde_json::json!({"did": "n"})).is_ok()); + for body in [ + serde_json::json!({"did": ""}), + serde_json::json!({}), + serde_json::json!({"did": 7}), + ] { + let got = did_from_node_info(&body); + assert!( + got.is_err(), + "must not yield a comparable DID: {body} -> {got:?}" + ); + } + // And the reason it produces must route to the degraded hint, never a verdict. + let report = + did_check_report(&did_from_node_info(&serde_json::json!({"did": ""})), "n").join("\n"); + assert!( + !report.contains("WARNING"), + "fabricated a mismatch: {report}" + ); + assert!(report.contains("Could not fetch"), "got: {report}"); + } + + // did_check_report is the three-way selector between the match text, the + // mismatch WARNING, and the degraded could-not-compare hint. Substring + // asserts (not full-line equality) so cosmetic wording edits don't break them. + + #[test] + fn did_check_report_match() { + let report = did_check_report(&Ok("n".to_string()), "n").join("\n"); + assert!( + report.contains("matches the node being queried"), + "got: {report}" + ); + assert!(!report.contains("WARNING"), "got: {report}"); + assert!(!report.contains("Could not fetch"), "got: {report}"); + } + + #[test] + fn did_check_report_mismatch() { + let report = did_check_report(&Ok("did:key:other".to_string()), "n").join("\n"); + assert!(report.contains("WARNING"), "got: {report}"); + assert!(report.contains("does not match"), "got: {report}"); + assert!(report.contains("did:key:other"), "got: {report}"); + assert!(!report.contains("Could not fetch"), "got: {report}"); + } + + #[test] + fn did_check_report_missing_did_reason() { + let report = + did_check_report(&Err("node info response carried no DID".to_string()), "n").join("\n"); + assert!(report.contains("Could not fetch"), "got: {report}"); + assert!( + report.contains("node info response carried no DID"), + "got: {report}" + ); + assert!(report.contains("verify offline"), "got: {report}"); + // The must-not case: no real DID was obtained, so no comparison verdict + // may be claimed in either direction. + assert!(!report.contains("WARNING"), "got: {report}"); + assert!( + !report.contains("matches the node being queried"), + "got: {report}" + ); + } + + #[test] + fn did_check_report_lookup_error_reason() { + let report = + did_check_report(&Err("node info failed (403): denied".to_string()), "n").join("\n"); + assert!(report.contains("Could not fetch"), "got: {report}"); + assert!(report.contains("403"), "got: {report}"); + assert!(report.contains("verify offline"), "got: {report}"); + assert!(!report.contains("WARNING"), "got: {report}"); + } } diff --git a/crates/gl/src/changelog.rs b/crates/gl/src/changelog.rs index 87368944..a65a9e6f 100644 --- a/crates/gl/src/changelog.rs +++ b/crates/gl/src/changelog.rs @@ -2,7 +2,6 @@ use anyhow::{Context, Result}; use clap::Args; -use serde_json::Value; use std::path::PathBuf; use crate::http::NodeClient; @@ -42,12 +41,7 @@ pub async fn run(args: ChangelogArgs) -> Result<()> { did.split(':').next_back().unwrap_or(&did).to_string() } else { let client = NodeClient::new(&args.node, None); - let info: Value = client - .get("/") - .await? - .json() - .await - .context("failed to fetch node info")?; + let info = crate::http::read_json(client.get("/").await?, "node info").await?; let did = info["did"].as_str().context("node missing DID")?; did.split(':').next_back().unwrap_or(did).to_string() }; @@ -64,13 +58,7 @@ pub async fn run(args: ChangelogArgs) -> Result<()> { .await .context("failed to connect to node")?; - let status = resp.status(); - let body: Value = resp.json().await.unwrap_or_default(); - - if !status.is_success() { - let msg = body["message"].as_str().unwrap_or("unknown error"); - anyhow::bail!("changelog failed ({status}): {msg}"); - } + let body = crate::http::read_json(resp, "changelog").await?; let events = body["events"].as_array().cloned().unwrap_or_default(); @@ -212,6 +200,7 @@ mod tests { .with_status(404) .with_header("content-type", "application/json") .with_body(r#"{"message":"repo not found"}"#) + .expect(1) .create_async() .await; @@ -223,6 +212,8 @@ mod tests { }; let err = run(args).await.unwrap_err(); assert!(err.to_string().contains("changelog failed")); + // Prove the mocked route was actually requested; a non-matching request (mockito's 501, also non-2xx) would otherwise satisfy the error assertion vacuously. + _m.assert_async().await; } #[tokio::test] @@ -270,4 +261,31 @@ mod tests { let err = rt.block_on(run(args)).unwrap_err(); assert!(err.to_string().contains("no repo specified")); } + + #[tokio::test] + async fn resolve_via_run_surfaces_denial() { + // A slash-free repo with an empty identity dir forces the inline GET / + // node-info fetch during resolution. A gated 404 there must Err (surfacing + // the status), proving the read_json conversion is load-bearing. + let mut server = mockito::Server::new_async().await; + let dir = tempfile::TempDir::new().unwrap(); // empty, no identity.pem, forces the GET / branch + let _m = server + .mock("GET", "/") + .with_status(404) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"denied"}"#) + .expect(1) + .create_async() + .await; + let err = run(ChangelogArgs { + repo: Some("noslash".to_string()), + limit: 20, + node: server.url(), + dir: Some(dir.path().to_path_buf()), + }) + .await + .unwrap_err(); + assert!(err.to_string().contains("404"), "got: {err}"); + _m.assert_async().await; + } } diff --git a/crates/gl/src/http.rs b/crates/gl/src/http.rs index 4c30f512..83984553 100644 --- a/crates/gl/src/http.rs +++ b/crates/gl/src/http.rs @@ -10,6 +10,7 @@ use anyhow::{Context, Result}; use gitlawb_core::http_sig::sign_request; use gitlawb_core::identity::Keypair; use icaptcha_client::IcaptchaCfg; +use serde_json::Value; /// Max times we'll fetch a fresh proof and retry a 403-iCaptcha response /// (absorbs proof expiry / first-seen replay). @@ -203,6 +204,63 @@ async fn obtain_proof(cfg: IcaptchaCfg) -> Result { .context("iCaptcha solver task panicked")? } +/// Read a JSON response, surfacing a node denial/error instead of parsing it as +/// the requested resource. On a non-2xx status it returns an `Err` carrying the +/// node's sanitized `message` (INV-6) plus the status; on success it parses the +/// body and propagates a parse error, so a truncated/garbage 2xx body is an error +/// rather than a silently-empty success (the denial-as-success bug #123 fixes). +/// `what` names the resource for the error text (e.g. "repo", "commits"). +/// +/// Callers must route gated reads through this rather than `resp.json().await?`: +/// the bare parse renders a gated 404/5xx body back as the resource (INV-8). +/// Cap on the error body `read_json` reads before parsing a node's `message`. +/// Matches the capped-read bound used on the sync error path; large enough for any +/// legitimate error, small enough that a hostile node cannot exhaust memory. +const ERROR_BODY_CAP: usize = 8 * 1024; + +pub(crate) async fn read_json(resp: reqwest::Response, what: &str) -> Result { + let status = resp.status(); + if !status.is_success() { + // Bound the error read: `resp.json()` would buffer and parse the WHOLE + // body, so a hostile node could stream an arbitrarily large valid JSON + // error and exhaust this process's memory (#186). Read a small capped body + // first, then best-effort extract `message`. The error body may also be + // non-JSON (503 degraded, 413 body-limit from middleware), so a parse miss + // falls back to the capped raw text; an empty body to the status alone. + let raw = crate::sync::read_body_capped(resp, ERROR_BODY_CAP).await; + // Preserve the prior contract: surface the JSON `message` when present in + // the capped body, else the status alone ("request failed") — a non-JSON + // or message-less body (503 degraded, 413 body-limit) never surfaces raw + // node bytes. A body whose `message` sits past the cap parses as truncated + // garbage here and correctly falls back rather than being buffered whole. + // + // Fall back to `error` before giving up: the node's shared AppError + // envelope carries both keys, but the task API answers with `error` alone + // (`{"error":"task not found"}` in gitlawb-node/src/api/tasks.rs), so + // reading only `message` blanked every `gl task` denial to the generic + // "request failed". `sync.rs`'s hand-rolled reader already reads both; this + // matches it. + let msg = serde_json::from_str::(&raw) + .ok() + .and_then(|b| { + b.get("message") + .or_else(|| b.get("error")) + .and_then(|m| m.as_str()) + .map(str::to_owned) + }) + .unwrap_or_else(|| "request failed".to_owned()); + anyhow::bail!( + "{what} failed ({status}): {}", + crate::sync::sanitize_node_msg(&msg) + ); + } + // Success: a truncated/garbage 2xx body must be an `Err`, not `Ok(Null)` that + // a caller then renders as an empty success. + resp.json() + .await + .with_context(|| format!("invalid JSON in {what} response")) +} + #[cfg(test)] mod tests { use super::*; @@ -587,4 +645,217 @@ mod tests { ic.challenge.assert(); ic.answer.assert(); } + + // ── read_json (status-checked read; #123 / INV-8 / INV-6) ──────────── + + /// Drive a real `reqwest::Response` off a mockito mock so `read_json` sees an + /// actual HTTP status + body, the same shape the gated read arms produce. + async fn response_for( + server: &mut Server, + status: usize, + body: &str, + json: bool, + ) -> reqwest::Response { + let mut m = server.mock("GET", "/x").with_status(status).with_body(body); + if json { + m = m.with_header("content-type", "application/json"); + } + let _m = m.create_async().await; + NodeClient::new(server.url(), None).get("/x").await.unwrap() + } + + #[tokio::test] + async fn read_json_returns_body_on_2xx() { + let mut server = Server::new_async().await; + let resp = response_for( + &mut server, + 200, + r#"{"name":"r","owner_did":"did:gitlawb:z"}"#, + true, + ) + .await; + let v = read_json(resp, "repo").await.unwrap(); + assert_eq!(v["name"], "r"); + } + + #[tokio::test] + async fn read_json_errs_on_404_surfacing_message_and_status() { + let mut server = Server::new_async().await; + let resp = response_for( + &mut server, + 404, + r#"{"message":"repository 'o/r' not found"}"#, + true, + ) + .await; + let err = read_json(resp, "repo").await.unwrap_err().to_string(); + assert!(err.contains("404"), "err={err}"); + assert!(err.contains("not found"), "err={err}"); + } + + #[tokio::test] + async fn read_json_errs_on_500_surfacing_message() { + let mut server = Server::new_async().await; + let resp = response_for(&mut server, 500, r#"{"message":"internal boom"}"#, true).await; + let err = read_json(resp, "commits").await.unwrap_err().to_string(); + assert!(err.contains("500"), "err={err}"); + assert!(err.contains("internal boom"), "err={err}"); + } + + #[tokio::test] + async fn read_json_errs_on_non_json_error_body_with_fallback() { + // 503 with a plain-text (middleware) body: no `message` field to surface. + let mut server = Server::new_async().await; + let resp = response_for(&mut server, 503, "service unavailable", false).await; + let err = read_json(resp, "repo").await.unwrap_err().to_string(); + assert!(err.contains("503"), "err={err}"); + assert!(err.contains("request failed"), "err={err}"); + } + + #[tokio::test] + async fn read_json_sanitizes_control_and_bidi_in_message() { + // INV-6: a hostile node embeds ESC, BEL, and a right-to-left override in + // the error message; none may reach the terminal verbatim. + let mut server = Server::new_async().await; + let resp = response_for( + &mut server, + 404, + r#"{"message":"a\u001b[31mb\u0007c\u202ed"}"#, + true, + ) + .await; + let err = read_json(resp, "repo").await.unwrap_err().to_string(); + assert!(!err.contains('\u{1b}'), "ESC leaked: {err:?}"); + assert!(!err.contains('\u{7}'), "BEL leaked: {err:?}"); + assert!(!err.contains('\u{202e}'), "bidi override leaked: {err:?}"); + } + + #[tokio::test] + async fn read_json_errs_on_garbage_2xx_body() { + // The #123 correctness point: a 200 with a non-JSON/truncated body must be + // an `Err`, NOT `Ok(Null)` that a caller renders as an empty success. + let mut server = Server::new_async().await; + let resp = response_for(&mut server, 200, "this is not json", false).await; + assert!(read_json(resp, "repo").await.is_err()); + } + + #[tokio::test] + async fn read_json_errs_on_empty_2xx_body() { + // A zero-byte 200 body must be an `Err`, not a silent `Ok(Null)`. + let mut server = Server::new_async().await; + let resp = response_for(&mut server, 200, "", false).await; + assert!(read_json(resp, "repo").await.is_err()); + } + + #[tokio::test] + async fn read_json_errs_on_non_2xx_json_without_message_or_error() { + // A non-2xx JSON body carrying NEITHER key is the real fallback case. + let mut server = Server::new_async().await; + let resp = response_for(&mut server, 403, r#"{"detail":"forbidden"}"#, true).await; + let err = read_json(resp, "repo").await.unwrap_err().to_string(); + assert!(err.contains("403"), "err={err}"); + assert!(err.contains("request failed"), "err={err}"); + // The body's own text must not leak when no recognised key carries it. + assert!(!err.contains("forbidden"), "raw body leaked: {err}"); + } + + #[tokio::test] + async fn read_json_surfaces_error_key_when_message_absent() { + // The task API answers with `error` alone — see + // gitlawb-node/src/api/tasks.rs `{"error":"task not found"}`. Reading only + // `message` blanked those denials to the generic "request failed", so the + // user was told nothing about why the call failed. + let mut server = Server::new_async().await; + let resp = response_for(&mut server, 404, r#"{"error":"task not found"}"#, true).await; + let err = read_json(resp, "task show").await.unwrap_err().to_string(); + assert!(err.contains("404"), "err={err}"); + assert!(err.contains("task not found"), "reason not surfaced: {err}"); + } + + #[tokio::test] + async fn read_json_prefers_message_over_error_key() { + // The shared AppError envelope carries both; `message` is the human text + // and must win over the machine code in `error`. + let mut server = Server::new_async().await; + let resp = response_for( + &mut server, + 500, + r#"{"error":"db_error","message":"database unavailable"}"#, + true, + ) + .await; + let err = read_json(resp, "repo").await.unwrap_err().to_string(); + assert!(err.contains("database unavailable"), "err={err}"); + assert!( + !err.contains("db_error"), + "machine code won over message: {err}" + ); + } + + #[tokio::test] + async fn read_json_sanitizes_error_key_text() { + // The `error` fallback goes through the same sanitizer as `message`; a + // hostile node must not reach the terminal through the new path. The + // controls are JSON \u escapes so the body stays VALID JSON — embedding + // them raw would fail the parse and pass this test vacuously. + let mut server = Server::new_async().await; + let resp = response_for( + &mut server, + 404, + r#"{"error":"a\u001b[31mb\u0007c\u202ed"}"#, + true, + ) + .await; + let err = read_json(resp, "task show").await.unwrap_err().to_string(); + // Proves the parse succeeded and the `error` key was read, not the + // "request failed" fallback (which would make the asserts below vacuous). + assert!(err.contains("task show failed (404"), "err={err}"); + assert!( + !err.contains("request failed"), + "fell back, sanitizer untested: {err}" + ); + assert!( + err.contains('a') && err.contains('d'), + "text dropped: {err}" + ); + assert!(!err.contains('\u{1b}'), "ESC leaked: {err:?}"); + assert!(!err.contains('\u{7}'), "BEL leaked: {err:?}"); + assert!(!err.contains('\u{202e}'), "bidi override leaked: {err:?}"); + } + + /// #186 (F2): the non-2xx error path must read a CAPPED body, not buffer and + /// parse the whole thing — a hostile node can stream an arbitrarily large valid + /// JSON error. Place the `message` field AFTER more than the cap of padding: the + /// bounded read stops before it, so it must be ABSENT from the surfaced error. + /// RED before the fix (`resp.json()` parses the full body → the far message + /// appears), GREEN after (capped read truncates → fallback → far message gone). + /// The status is still surfaced, so a legitimate small error is unaffected. + #[tokio::test] + async fn read_json_bounds_the_error_body_read() { + let mut server = Server::new_async().await; + // >8 KiB of padding, then the marker message at the very end of the body. + let big = format!( + r#"{{"padding":"{}","message":"FARMARKER_BEYOND_CAP"}}"#, + "A".repeat(16 * 1024) + ); + let resp = response_for(&mut server, 500, &big, true).await; + let err = read_json(resp, "repo").await.unwrap_err().to_string(); + assert!(err.contains("500"), "bounded read still errors: {err}"); + assert!( + !err.contains("FARMARKER_BEYOND_CAP"), + "the error body read must be capped: a message beyond the cap must not be surfaced, err={err}" + ); + } + + /// #186 (F2): an EMPTY non-2xx body is its own input class — the capped read + /// yields "", the JSON parse fails, and it falls back to the status alone + /// ("request failed"), never a panic or a spurious success. + #[tokio::test] + async fn read_json_errs_on_empty_non_2xx_body() { + let mut server = Server::new_async().await; + let resp = response_for(&mut server, 502, "", false).await; + let err = read_json(resp, "repo").await.unwrap_err().to_string(); + assert!(err.contains("502"), "err={err}"); + assert!(err.contains("request failed"), "err={err}"); + } } diff --git a/crates/gl/src/issue.rs b/crates/gl/src/issue.rs index 57bd6944..247497e2 100644 --- a/crates/gl/src/issue.rs +++ b/crates/gl/src/issue.rs @@ -3,7 +3,7 @@ use anyhow::{Context, Result}; use chrono::Utc; use clap::{Args, Subcommand}; -use serde_json::{json, Value}; +use serde_json::json; use std::path::PathBuf; use uuid::Uuid; @@ -142,12 +142,7 @@ async fn resolve_repo( did.split(':').next_back().unwrap_or(&did).to_string() } else { let client = NodeClient::new(node, None); - let info: Value = client - .get("/") - .await? - .json() - .await - .context("failed to fetch node info")?; + let info = crate::http::read_json(client.get("/").await?, "node info").await?; let did = info["did"].as_str().context("node info missing 'did'")?; did.split(':').next_back().unwrap_or(did).to_string() }; @@ -198,13 +193,7 @@ async fn cmd_create( .post(&path, &request_body) .await .context("failed to connect to node")?; - let status = resp.status(); - let result: Value = resp.json().await.context("invalid JSON response")?; - - if !status.is_success() { - let msg = result["message"].as_str().unwrap_or("unknown error"); - anyhow::bail!("create issue failed ({status}): {msg}"); - } + let result = crate::http::read_json(resp, "create issue").await?; let id = result["id"].as_str().unwrap_or("?"); println!("✓ Created issue #{id}"); @@ -221,12 +210,7 @@ async fn cmd_list(repo: String, node: String, dir: Option) -> Result<() let client = signed_client(&node, dir.as_deref()); let path = format!("/api/v1/repos/{owner}/{name}/issues"); - let resp: Value = client - .get_authed(&path) - .await? - .json() - .await - .context("failed to list issues")?; + let resp = crate::http::read_json(client.get_authed(&path).await?, "issues").await?; let issues = resp["issues"].as_array().cloned().unwrap_or_default(); @@ -264,13 +248,7 @@ async fn cmd_show(repo: String, id: String, node: String, dir: Option) .get_authed(&path) .await .context("failed to connect to node")?; - let status = resp.status(); - let issue: Value = resp.json().await.context("invalid JSON response")?; - - if !status.is_success() { - let msg = issue["message"].as_str().unwrap_or("issue not found"); - anyhow::bail!("show failed ({status}): {msg}"); - } + let issue = crate::http::read_json(resp, "show").await?; let title = issue["title"].as_str().unwrap_or("(no title)"); let status = issue["status"].as_str().unwrap_or("?"); @@ -301,13 +279,7 @@ async fn cmd_close(repo: String, id: String, node: String, dir: Option) .post(&format!("{path}/close"), &body) .await .context("failed to connect to node")?; - let status = resp.status(); - let result: Value = resp.json().await.context("invalid JSON response")?; - - if !status.is_success() { - let msg = result["message"].as_str().unwrap_or("unknown error"); - anyhow::bail!("close issue failed ({status}): {msg}"); - } + let _ = crate::http::read_json(resp, "close issue").await?; println!("✗ Closed issue {id}"); Ok(()) @@ -332,13 +304,7 @@ async fn cmd_issue_comment( ) .await .context("failed to connect to node")?; - let code = resp.status(); - let result: Value = resp.json().await.context("invalid JSON")?; - - if !code.is_success() { - let msg = result["message"].as_str().unwrap_or("unknown error"); - anyhow::bail!("comment failed ({code}): {msg}"); - } + let _ = crate::http::read_json(resp, "comment").await?; println!("· Comment posted on issue {id}"); Ok(()) @@ -353,14 +319,15 @@ async fn cmd_issue_comments( let (owner, name) = resolve_repo(&repo, &node, dir.as_deref()).await?; let client = signed_client(&node, dir.as_deref()); - let resp: Value = client - .get_authed(&format!( - "/api/v1/repos/{owner}/{name}/issues/{id}/comments" - )) - .await? - .json() - .await - .context("invalid JSON")?; + let resp = crate::http::read_json( + client + .get_authed(&format!( + "/api/v1/repos/{owner}/{name}/issues/{id}/comments" + )) + .await?, + "issue comments", + ) + .await?; let comments = resp["comments"].as_array().cloned().unwrap_or_default(); if comments.is_empty() { @@ -490,6 +457,7 @@ mod tests { .with_status(404) .with_header("content-type", "application/json") .with_body(r#"{"message":"repo not found"}"#) + .expect(1) .create_async() .await; @@ -504,6 +472,8 @@ mod tests { assert!(result.is_err()); assert!(result.unwrap_err().to_string().contains("repo not found")); + // Prove the mocked route was actually requested; a non-matching request (mockito's 501, also non-2xx) would otherwise satisfy is_err() vacuously. + _m.assert_async().await; } #[tokio::test] @@ -551,6 +521,7 @@ mod tests { .with_status(404) .with_header("content-type", "application/json") .with_body(r#"{"message":"issue not found"}"#) + .expect(1) .create_async() .await; @@ -564,6 +535,8 @@ mod tests { .unwrap_err(); assert!(err.to_string().contains("show failed"), "got: {err}"); assert!(err.to_string().contains("issue not found"), "got: {err}"); + // Prove the mocked route was actually requested; a non-matching request (mockito's 501, also non-2xx) would otherwise satisfy the error assertion vacuously. + _m.assert_async().await; } #[tokio::test] @@ -612,6 +585,7 @@ mod tests { .with_status(404) .with_header("content-type", "application/json") .with_body(r#"{"message":"issue not found"}"#) + .expect(1) .create_async() .await; @@ -624,6 +598,8 @@ mod tests { .await .unwrap_err(); assert!(err.to_string().contains("close issue failed"), "got: {err}"); + // Prove the mocked route was actually requested; a non-matching request (mockito's 501, also non-2xx) would otherwise satisfy the error assertion vacuously. + _m.assert_async().await; } #[tokio::test] @@ -670,6 +646,7 @@ mod tests { .with_status(404) .with_header("content-type", "application/json") .with_body(r#"{"message":"issue not found"}"#) + .expect(1) .create_async() .await; @@ -683,6 +660,8 @@ mod tests { .await .unwrap_err(); assert!(err.to_string().contains("issue not found"), "got: {err}"); + // Prove the mocked route was actually requested; a non-matching request (mockito's 501, also non-2xx) would otherwise satisfy the error assertion vacuously. + _m.assert_async().await; } #[tokio::test] @@ -740,4 +719,48 @@ mod tests { .await .unwrap(); } + + #[tokio::test] + async fn cmd_list_surfaces_denial_not_empty() { + // A gated 404 on the issue-list read must Err, not print "No issues". + let mut server = mockito::Server::new_async().await; + let _m = server + .mock( + "GET", + mockito::Matcher::Regex(r"^/api/v1/repos/alice/secret/issues$".to_string()), + ) + .with_status(404) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"repository 'alice/secret' not found"}"#) + .expect(1) + .create_async() + .await; + let result = cmd_list("alice/secret".to_string(), server.url(), None).await; + assert!(result.is_err(), "issue list must Err on a gated 404"); + // Prove the gated issues path was actually requested: without this, an + // unmatched route (mockito's 501, also non-2xx) would satisfy is_err(). + _m.assert_async().await; + } + + #[tokio::test] + async fn resolve_repo_surfaces_denial() { + // A slash-free repo with an empty identity dir forces the GET / node-info + // fetch. A gated 404 there must Err (surfacing the status), proving the + // read_json conversion is load-bearing rather than silently ignored. + let mut server = mockito::Server::new_async().await; + let dir = tempfile::TempDir::new().unwrap(); // empty, no identity.pem, forces the GET / branch + let _m = server + .mock("GET", "/") + .with_status(404) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"denied"}"#) + .expect(1) + .create_async() + .await; + let err = resolve_repo("noslash", &server.url(), Some(dir.path())) + .await + .unwrap_err(); + assert!(err.to_string().contains("404"), "got: {err}"); + _m.assert_async().await; + } } diff --git a/crates/gl/src/mcp.rs b/crates/gl/src/mcp.rs index ae319c73..c406a471 100644 --- a/crates/gl/src/mcp.rs +++ b/crates/gl/src/mcp.rs @@ -654,12 +654,13 @@ async fn call_tool( } "node_info" => { - let info: Value = client.get("/").await?.json().await?; + let info = crate::http::read_json(client.get("/").await?, "node info").await?; Ok(serde_json::to_string_pretty(&info)?) } "node_health" => { - let health: Value = client.get("/health").await?.json().await?; + let health = + crate::http::read_json(client.get("/health").await?, "node health").await?; Ok(serde_json::to_string_pretty(&health)?) } @@ -670,39 +671,48 @@ async fn call_tool( "description": args["description"], "is_public": args["is_public"].as_bool().unwrap_or(true), }))?; - let resp: Value = client.post("/api/v1/repos", &body).await?.json().await?; + let resp = + crate::http::read_json(client.post("/api/v1/repos", &body).await?, "create repo") + .await?; Ok(serde_json::to_string_pretty(&resp)?) } "repo_list" => { - let repos: Value = client.get("/api/v1/repos").await?.json().await?; + let repos = + crate::http::read_json(client.get("/api/v1/repos").await?, "list repos").await?; Ok(serde_json::to_string_pretty(&repos)?) } "repo_list_federated" => { - let result: Value = client.get("/api/v1/repos/federated").await?.json().await?; + let result = crate::http::read_json( + client.get("/api/v1/repos/federated").await?, + "list federated repos", + ) + .await?; Ok(serde_json::to_string_pretty(&result)?) } "repo_get" => { let name = args["name"].as_str().context("missing 'name'")?; let owner = resolve_owner(&args, &client).await?; - let repo: Value = client - .get(&format!("/api/v1/repos/{owner}/{name}")) - .await? - .json() - .await?; + let repo = crate::http::read_json( + client.get(&format!("/api/v1/repos/{owner}/{name}")).await?, + "repo", + ) + .await?; Ok(serde_json::to_string_pretty(&repo)?) } "repo_commits" => { let name = args["name"].as_str().context("missing 'name'")?; let owner = resolve_owner(&args, &client).await?; - let commits: Value = client - .get(&format!("/api/v1/repos/{owner}/{name}/commits")) - .await? - .json() - .await?; + let commits = crate::http::read_json( + client + .get(&format!("/api/v1/repos/{owner}/{name}/commits")) + .await?, + "commits", + ) + .await?; Ok(serde_json::to_string_pretty(&commits)?) } @@ -710,17 +720,19 @@ async fn call_tool( let name = args["name"].as_str().context("missing 'name'")?; let path = args["path"].as_str().unwrap_or(""); let owner = resolve_owner(&args, &client).await?; - let tree: Value = client - .get(&format!("/api/v1/repos/{owner}/{name}/tree/{path}")) - .await? - .json() - .await?; + let tree = crate::http::read_json( + client + .get(&format!("/api/v1/repos/{owner}/{name}/tree/{path}")) + .await?, + "tree", + ) + .await?; Ok(serde_json::to_string_pretty(&tree)?) } "repo_clone_url" => { let name = args["name"].as_str().context("missing 'name'")?; - let info: Value = client.get("/").await?.json().await?; + let info = crate::http::read_json(client.get("/").await?, "node info").await?; let did = info["did"].as_str().context("node info missing DID")?; Ok(format!("gitlawb://{}/{}", did, name)) } @@ -736,7 +748,11 @@ async fn call_tool( "capabilities": caps, "model": args["model"], }))?; - let resp: Value = client.post("/api/register", &body).await?.json().await?; + let resp = crate::http::read_json( + client.post("/api/register", &body).await?, + "register agent", + ) + .await?; Ok(serde_json::to_string_pretty(&resp)?) } @@ -793,6 +809,10 @@ async fn call_tool( "/{owner}/{name}/info/refs?service=git-upload-pack" )) .await?; + let status = resp.status(); + if !status.is_success() { + anyhow::bail!("git refs failed ({status})"); + } let bytes = resp.bytes().await?; // Parse pkt-line refs let refs = parse_info_refs(&bytes); @@ -814,22 +834,26 @@ async fn call_tool( "source_branch": head, "target_branch": base, }))?; - let resp: Value = client - .post(&format!("/api/v1/repos/{owner}/{repo}/pulls"), &body) - .await? - .json() - .await?; + let resp = crate::http::read_json( + client + .post(&format!("/api/v1/repos/{owner}/{repo}/pulls"), &body) + .await?, + "create pull request", + ) + .await?; Ok(serde_json::to_string_pretty(&resp)?) } "pr_list" => { let repo = args["repo"].as_str().context("missing 'repo'")?; let owner = resolve_owner(&args, &client).await?; - let resp: Value = client - .get(&format!("/api/v1/repos/{owner}/{repo}/pulls")) - .await? - .json() - .await?; + let resp = crate::http::read_json( + client + .get(&format!("/api/v1/repos/{owner}/{repo}/pulls")) + .await?, + "pull requests", + ) + .await?; Ok(serde_json::to_string_pretty(&resp)?) } @@ -837,18 +861,22 @@ async fn call_tool( let repo = args["repo"].as_str().context("missing 'repo'")?; let number = args["number"].as_i64().context("missing 'number'")?; let owner = resolve_owner(&args, &client).await?; - let pr: Value = client - .get(&format!("/api/v1/repos/{owner}/{repo}/pulls/{number}")) - .await? - .json() - .await?; - let reviews: Value = client - .get(&format!( - "/api/v1/repos/{owner}/{repo}/pulls/{number}/reviews" - )) - .await? - .json() - .await?; + let pr = crate::http::read_json( + client + .get(&format!("/api/v1/repos/{owner}/{repo}/pulls/{number}")) + .await?, + "pull request", + ) + .await?; + let reviews = crate::http::read_json( + client + .get(&format!( + "/api/v1/repos/{owner}/{repo}/pulls/{number}/reviews" + )) + .await?, + "reviews", + ) + .await?; Ok(serde_json::to_string_pretty( &json!({ "pr": pr, "reviews": reviews["reviews"] }), )?) @@ -858,11 +886,13 @@ async fn call_tool( let repo = args["repo"].as_str().context("missing 'repo'")?; let number = args["number"].as_i64().context("missing 'number'")?; let owner = resolve_owner(&args, &client).await?; - let resp: Value = client - .get(&format!("/api/v1/repos/{owner}/{repo}/pulls/{number}/diff")) - .await? - .json() - .await?; + let resp = crate::http::read_json( + client + .get(&format!("/api/v1/repos/{owner}/{repo}/pulls/{number}/diff")) + .await?, + "diff", + ) + .await?; let diff = resp["diff"].as_str().unwrap_or("(empty diff)"); Ok(diff.to_string()) } @@ -879,14 +909,16 @@ async fn call_tool( "status": status, "body": args["body"], }))?; - let resp: Value = client - .post( - &format!("/api/v1/repos/{owner}/{repo}/pulls/{number}/reviews"), - &body, - ) - .await? - .json() - .await?; + let resp = crate::http::read_json( + client + .post( + &format!("/api/v1/repos/{owner}/{repo}/pulls/{number}/reviews"), + &body, + ) + .await?, + "submit review", + ) + .await?; Ok(serde_json::to_string_pretty(&resp)?) } @@ -895,14 +927,16 @@ async fn call_tool( let number = args["number"].as_i64().context("missing 'number'")?; let owner = resolve_owner(&args, &client).await?; let body = serde_json::to_vec(&json!({}))?; - let resp: Value = client - .post( - &format!("/api/v1/repos/{owner}/{repo}/pulls/{number}/merge"), - &body, - ) - .await? - .json() - .await?; + let resp = crate::http::read_json( + client + .post( + &format!("/api/v1/repos/{owner}/{repo}/pulls/{number}/merge"), + &body, + ) + .await?, + "merge pull request", + ) + .await?; Ok(serde_json::to_string_pretty(&resp)?) } @@ -922,20 +956,22 @@ async fn call_tool( "secret": args["secret"], "events": events, }))?; - let resp: Value = client - .post(&format!("/api/v1/repos/{owner}/{repo}/hooks"), &body) - .await? - .json() - .await?; + let resp = crate::http::read_json( + client + .post(&format!("/api/v1/repos/{owner}/{repo}/hooks"), &body) + .await?, + "create webhook", + ) + .await?; Ok(serde_json::to_string_pretty(&resp)?) } "webhook_list" => { let repo = args["repo"].as_str().context("missing 'repo'")?; // Owner-gated route: an explicit `owner` arg wins, otherwise default to - // the signing keypair's short DID (NOT the node DID). The request is - // signed by this keypair, so the path owner must match it or the node - // returns 403/404. + // the signing keypair's short DID (NOT the node DID that resolve_owner + // returns). The request is signed by this keypair, so the path owner + // must match it or the node returns 403/404. let owner = if let Some(o) = args.get("owner").and_then(|v| v.as_str()) { o.to_string() } else { @@ -945,20 +981,22 @@ async fn call_tool( let did = kp.did().to_string(); did.split(':').next_back().unwrap_or(&did).to_string() }; - // Owner-gated route: must be signed (get_signed), not a plain get(). - let resp = client - .get_signed(&format!("/api/v1/repos/{owner}/{repo}/hooks")) - .await?; - // Check the HTTP status before deserializing: a 401/403/404 JSON error - // body (missing identity, wrong owner, private/deleted repo) must fail - // the tool call, not be returned as a successful result. - let status = resp.status(); - let body: Value = resp.json().await?; - if !status.is_success() { - let msg = body["message"].as_str().unwrap_or("unknown error"); - anyhow::bail!("webhook_list failed ({status}): {msg}"); - } - Ok(serde_json::to_string_pretty(&body)?) + // Owner-gated AND auth-required route (list_webhooks 401s a headerless + // caller, then 403s a non-owner). get_maybe_signed signs when an + // identity is present (so the owner is authorized) and otherwise sends + // unsigned — the node then returns the 401/403/404 denial, which + // read_json (#186) surfaces as an error instead of a fake result, with + // a bounded error read. Unlike get_signed, this still ISSUES the request + // without a local identity (surfacing the node's denial) rather than + // failing client-side before the node is contacted. + let resp = crate::http::read_json( + client + .get_maybe_signed(&format!("/api/v1/repos/{owner}/{repo}/hooks")) + .await?, + "webhook_list", + ) + .await?; + Ok(serde_json::to_string_pretty(&resp)?) } "webhook_delete" => { @@ -966,11 +1004,13 @@ async fn call_tool( let id = args["id"].as_str().context("missing 'id'")?; let owner = resolve_owner(&args, &client).await?; let body = serde_json::to_vec(&json!({}))?; - let resp: Value = client - .delete(&format!("/api/v1/repos/{owner}/{repo}/hooks/{id}"), &body) - .await? - .json() - .await?; + let resp = crate::http::read_json( + client + .delete(&format!("/api/v1/repos/{owner}/{repo}/hooks/{id}"), &body) + .await?, + "delete webhook", + ) + .await?; Ok(serde_json::to_string_pretty(&resp)?) } @@ -990,17 +1030,17 @@ async fn call_tool( } u }; - let resp: Value = client.get_authed(&url).await?.json().await?; + let resp = crate::http::read_json(client.get_authed(&url).await?, "bounties").await?; Ok(serde_json::to_string_pretty(&resp)?) } "bounty_show" => { let id = args["id"].as_str().context("missing 'id'")?; - let resp: Value = client - .get_authed(&format!("/api/v1/bounties/{id}")) - .await? - .json() - .await?; + let resp = crate::http::read_json( + client.get_authed(&format!("/api/v1/bounties/{id}")).await?, + "bounty", + ) + .await?; Ok(serde_json::to_string_pretty(&resp)?) } @@ -1013,28 +1053,32 @@ async fn call_tool( "issue_id": args.get("issue_id").and_then(|v| v.as_str()), "tx_hash": args.get("tx_hash").and_then(|v| v.as_str()), }); - let resp: Value = client - .post( - &format!("/api/v1/repos/{owner}/{name}/bounties"), - &serde_json::to_vec(&body)?, - ) - .await? - .json() - .await?; + let resp = crate::http::read_json( + client + .post( + &format!("/api/v1/repos/{owner}/{name}/bounties"), + &serde_json::to_vec(&body)?, + ) + .await?, + "create bounty", + ) + .await?; Ok(serde_json::to_string_pretty(&resp)?) } "bounty_claim" => { let id = args["id"].as_str().context("missing 'id'")?; let body = json!({ "wallet": args.get("wallet").and_then(|v| v.as_str()) }); - let resp: Value = client - .post( - &format!("/api/v1/bounties/{id}/claim"), - &serde_json::to_vec(&body)?, - ) - .await? - .json() - .await?; + let resp = crate::http::read_json( + client + .post( + &format!("/api/v1/bounties/{id}/claim"), + &serde_json::to_vec(&body)?, + ) + .await?, + "claim bounty", + ) + .await?; Ok(serde_json::to_string_pretty(&resp)?) } @@ -1042,19 +1086,23 @@ async fn call_tool( let id = args["id"].as_str().context("missing 'id'")?; let pr_id = args["pr_id"].as_str().context("missing 'pr_id'")?; let body = json!({ "pr_id": pr_id }); - let resp: Value = client - .post( - &format!("/api/v1/bounties/{id}/submit"), - &serde_json::to_vec(&body)?, - ) - .await? - .json() - .await?; + let resp = crate::http::read_json( + client + .post( + &format!("/api/v1/bounties/{id}/submit"), + &serde_json::to_vec(&body)?, + ) + .await?, + "submit bounty", + ) + .await?; Ok(serde_json::to_string_pretty(&resp)?) } "bounty_stats" => { - let resp: Value = client.get("/api/v1/bounties/stats").await?.json().await?; + let resp = + crate::http::read_json(client.get("/api/v1/bounties/stats").await?, "bounty stats") + .await?; Ok(serde_json::to_string_pretty(&resp)?) } @@ -1068,7 +1116,7 @@ async fn call_tool( if let Some(a) = args.get("assignee_did").and_then(|v| v.as_str()) { path.push_str(&format!("&assignee_did={}", urlencoding::encode(a))); } - let resp: Value = client.get(&path).await?.json().await?; + let resp = crate::http::read_json(client.get(&path).await?, "list tasks").await?; Ok(serde_json::to_string_pretty(&resp)?) } @@ -1085,7 +1133,9 @@ async fn call_tool( "deadline": args.get("deadline").and_then(|v| v.as_str()), "delegator_did": delegator_did, }))?; - let resp: Value = client.post("/api/v1/tasks", &body).await?.json().await?; + let resp = + crate::http::read_json(client.post("/api/v1/tasks", &body).await?, "create task") + .await?; Ok(serde_json::to_string_pretty(&resp)?) } @@ -1094,11 +1144,13 @@ async fn call_tool( let assignee_did = kp.did().to_string(); let id = args["id"].as_str().context("missing 'id'")?; let body = serde_json::to_vec(&json!({ "assignee_did": assignee_did }))?; - let resp: Value = client - .post(&format!("/api/v1/tasks/{id}/claim"), &body) - .await? - .json() - .await?; + let resp = crate::http::read_json( + client + .post(&format!("/api/v1/tasks/{id}/claim"), &body) + .await?, + "claim task", + ) + .await?; Ok(serde_json::to_string_pretty(&resp)?) } @@ -1110,11 +1162,13 @@ async fn call_tool( "result": args.get("result").and_then(|v| v.as_str()), "by_did": by_did, }))?; - let resp: Value = client - .post(&format!("/api/v1/tasks/{id}/complete"), &body) - .await? - .json() - .await?; + let resp = crate::http::read_json( + client + .post(&format!("/api/v1/tasks/{id}/complete"), &body) + .await?, + "complete task", + ) + .await?; Ok(serde_json::to_string_pretty(&resp)?) } @@ -1184,11 +1238,13 @@ async fn call_tool( let owner = resolve_owner(&args, &client).await?; (owner, repo.to_string()) }; - let resp: Value = client - .get_authed(&format!("/api/v1/repos/{owner}/{name}/issues")) - .await? - .json() - .await?; + let resp = crate::http::read_json( + client + .get_authed(&format!("/api/v1/repos/{owner}/{name}/issues")) + .await?, + "issues", + ) + .await?; Ok(serde_json::to_string_pretty(&resp)?) } @@ -1208,11 +1264,13 @@ async fn call_tool( "title": title, "body": args.get("body").and_then(|v| v.as_str()), }))?; - let resp: Value = client - .post(&format!("/api/v1/repos/{owner}/{name}/issues"), &body) - .await? - .json() - .await?; + let resp = crate::http::read_json( + client + .post(&format!("/api/v1/repos/{owner}/{name}/issues"), &body) + .await?, + "create issue", + ) + .await?; Ok(serde_json::to_string_pretty(&resp)?) } @@ -1230,14 +1288,16 @@ async fn call_tool( (owner, repo.to_string()) }; let body = serde_json::to_vec(&json!({ "body": comment_body }))?; - let resp: Value = client - .post( - &format!("/api/v1/repos/{owner}/{name}/issues/{issue_id}/comments"), - &body, - ) - .await? - .json() - .await?; + let resp = crate::http::read_json( + client + .post( + &format!("/api/v1/repos/{owner}/{name}/issues/{issue_id}/comments"), + &body, + ) + .await?, + "comment on issue", + ) + .await?; Ok(serde_json::to_string_pretty(&resp)?) } @@ -1252,7 +1312,7 @@ async fn resolve_owner(args: &Value, client: &NodeClient) -> Result { if let Some(o) = args.get("owner").and_then(|v| v.as_str()) { return Ok(o.to_string()); } - let info: Value = client.get("/").await?.json().await?; + let info = crate::http::read_json(client.get("/").await?, "node info").await?; let did = info["did"].as_str().context("node info missing DID")?; Ok(did.split(':').next_back().unwrap_or(did).to_string()) } @@ -2032,4 +2092,580 @@ mod tests { let count = tools.as_array().unwrap().len(); assert_eq!(count, 40, "expected 40 tools, got {count}"); } + + // ── Gated read arms surface node denials, not fabricated results (#123 / INV-8) ── + + #[tokio::test] + async fn repo_get_surfaces_denial_not_fabricated_repo() { + // The node returns the opaque 404 for a private repo the caller cannot read. + // The tool must Err surfacing it, NOT Ok with the error body serialized as the repo. + let mut server = mockito::Server::new_async().await; + let _m = server + .mock("GET", "/api/v1/repos/alice/secret") + .with_status(404) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"repository 'alice/secret' not found"}"#) + .expect(1) + .create_async() + .await; + + let result = call_tool( + "repo_get", + json!({"owner": "alice", "name": "secret"}), + &server.url(), + None, + ) + .await; + + let err = result + .expect_err("repo_get must Err on a 404, not fabricate a repo") + .to_string(); + assert!(err.contains("404"), "err={err}"); + assert!(err.contains("not found"), "err={err}"); + // Prove the gated route was actually requested; a non-matching request (mockito's 501, also non-2xx) would otherwise satisfy is_err() vacuously. + _m.assert_async().await; + } + + #[tokio::test] + async fn repo_get_returns_repo_on_200() { + let mut server = mockito::Server::new_async().await; + let _m = server + .mock("GET", "/api/v1/repos/alice/myrepo") + .with_status(200) + .with_header("content-type", "application/json") + .with_body(r#"{"name":"myrepo","owner_did":"did:gitlawb:alice"}"#) + .create_async() + .await; + let result = call_tool( + "repo_get", + json!({"owner": "alice", "name": "myrepo"}), + &server.url(), + None, + ) + .await + .unwrap(); + let parsed: Value = serde_json::from_str(&result).unwrap(); + assert_eq!(parsed["name"], "myrepo"); + } + + #[tokio::test] + async fn repo_commits_surfaces_denial_not_empty_list() { + // Before the fix a 404 rendered as "no commits" (empty). Now it must Err. + let mut server = mockito::Server::new_async().await; + let _m = server + .mock("GET", "/api/v1/repos/alice/secret/commits") + .with_status(404) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"repository 'alice/secret' not found"}"#) + .expect(1) + .create_async() + .await; + let result = call_tool( + "repo_commits", + json!({"owner": "alice", "name": "secret"}), + &server.url(), + None, + ) + .await; + assert!(result.is_err(), "repo_commits must Err on 404"); + assert!(result.unwrap_err().to_string().contains("not found")); + // Prove the gated route was actually requested; a non-matching request (mockito's 501, also non-2xx) would otherwise satisfy is_err() vacuously. + _m.assert_async().await; + } + + #[tokio::test] + async fn repo_tree_surfaces_denial_not_fabricated_tree() { + let mut server = mockito::Server::new_async().await; + let _m = server + .mock("GET", "/api/v1/repos/alice/secret/tree/") + .with_status(404) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"repository 'alice/secret' not found"}"#) + .expect(1) + .create_async() + .await; + let result = call_tool( + "repo_tree", + json!({"owner": "alice", "name": "secret"}), + &server.url(), + None, + ) + .await; + assert!(result.is_err(), "repo_tree must Err on 404"); + // Prove the gated route was actually requested; a non-matching request (mockito's 501, also non-2xx) would otherwise satisfy is_err() vacuously. + _m.assert_async().await; + } + + #[tokio::test] + async fn pr_list_surfaces_denial_not_empty() { + let mut server = mockito::Server::new_async().await; + let _m = server + .mock("GET", "/api/v1/repos/alice/secret/pulls") + .with_status(404) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"repository 'alice/secret' not found"}"#) + .expect(1) + .create_async() + .await; + let result = call_tool( + "pr_list", + json!({"owner": "alice", "repo": "secret"}), + &server.url(), + None, + ) + .await; + assert!(result.is_err(), "pr_list must Err on 404"); + // Prove the gated route was actually requested; a non-matching request (mockito's 501, also non-2xx) would otherwise satisfy is_err() vacuously. + _m.assert_async().await; + } + + #[tokio::test] + async fn webhook_list_surfaces_denial_not_hook_targets() { + // Client half of #94: a non-owner hooks read must surface the denial, + // never render the webhook target URLs as a result. + let mut server = mockito::Server::new_async().await; + let _m = server + .mock("GET", "/api/v1/repos/alice/secret/hooks") + .with_status(404) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"repository 'alice/secret' not found"}"#) + .expect(1) + .create_async() + .await; + let result = call_tool( + "webhook_list", + json!({"owner": "alice", "repo": "secret"}), + &server.url(), + None, + ) + .await; + assert!(result.is_err(), "webhook_list must Err on 404"); + // Prove the gated route was actually requested; a non-matching request (mockito's 501, also non-2xx) would otherwise satisfy is_err() vacuously. + _m.assert_async().await; + } + + #[tokio::test] + async fn issue_list_surfaces_denial_not_empty() { + let mut server = mockito::Server::new_async().await; + let _m = server + .mock("GET", "/api/v1/repos/alice/secret/issues") + .with_status(404) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"repository 'alice/secret' not found"}"#) + .expect(1) + .create_async() + .await; + let result = call_tool( + "issue_list", + json!({"repo": "alice/secret"}), + &server.url(), + None, + ) + .await; + assert!(result.is_err(), "issue_list must Err on 404"); + // Prove the gated route was actually requested; a non-matching request (mockito's 501, also non-2xx) would otherwise satisfy is_err() vacuously. + _m.assert_async().await; + } + + #[tokio::test] + async fn pr_view_surfaces_denial_not_stub() { + let mut server = mockito::Server::new_async().await; + let _m = server + .mock("GET", "/api/v1/repos/alice/secret/pulls/1") + .with_status(404) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"repository 'alice/secret' not found"}"#) + .expect(1) + .create_async() + .await; + let result = call_tool( + "pr_view", + json!({"owner": "alice", "repo": "secret", "number": 1}), + &server.url(), + None, + ) + .await; + assert!(result.is_err(), "pr_view must Err on 404"); + // Prove the gated route was actually requested; a non-matching request (mockito's 501, also non-2xx) would otherwise satisfy is_err() vacuously. + _m.assert_async().await; + } + + #[tokio::test] + async fn pr_diff_surfaces_denial_not_empty() { + let mut server = mockito::Server::new_async().await; + let _m = server + .mock("GET", "/api/v1/repos/alice/secret/pulls/1/diff") + .with_status(404) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"repository 'alice/secret' not found"}"#) + .expect(1) + .create_async() + .await; + let result = call_tool( + "pr_diff", + json!({"owner": "alice", "repo": "secret", "number": 1}), + &server.url(), + None, + ) + .await; + assert!(result.is_err(), "pr_diff must Err on 404"); + // Prove the gated route was actually requested; a non-matching request (mockito's 501, also non-2xx) would otherwise satisfy is_err() vacuously. + _m.assert_async().await; + } + + // ── INV-8 across the MCP twin: every tool that renders a node response must + // surface a denial as an Err, not print the 4xx/5xx body as a success. ── + + /// Drive one MCP tool against a node returning a 404 and assert it Errs + /// (surfacing the denial) rather than returning the error body as a result. + /// `.expect(1)` proves the tool hit the intended endpoint, so a wrong-path + /// request (mockito's 501) can't satisfy the Err assertion vacuously. + async fn assert_tool_surfaces_denial( + tool: &str, + method: &str, + path: mockito::Matcher, + args: Value, + with_identity: bool, + ) { + let mut server = mockito::Server::new_async().await; + let dir = tempfile::TempDir::new().unwrap(); + if with_identity { + let kp = gitlawb_core::identity::Keypair::generate(); + std::fs::write( + dir.path().join("identity.pem"), + kp.to_pem().unwrap().as_bytes(), + ) + .unwrap(); + } + let _m = server + .mock(method, path) + .with_status(404) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"denied"}"#) + .expect(1) + .create_async() + .await; + let dir_opt = with_identity.then(|| dir.path()); + let err = call_tool(tool, args, &server.url(), dir_opt) + .await + .unwrap_err(); + assert!( + err.to_string().contains("404"), + "{tool} must surface the 404 denial as an Err, got: {err}" + ); + _m.assert_async().await; + } + + #[tokio::test] + async fn node_info_via_mcp_surfaces_denial() { + assert_tool_surfaces_denial( + "node_info", + "GET", + mockito::Matcher::Regex(r"^/$".to_string()), + json!({}), + false, + ) + .await; + } + + #[tokio::test] + async fn node_health_via_mcp_surfaces_denial() { + assert_tool_surfaces_denial( + "node_health", + "GET", + mockito::Matcher::Regex(r"^/health$".to_string()), + json!({}), + false, + ) + .await; + } + + #[tokio::test] + async fn repo_create_via_mcp_surfaces_denial() { + assert_tool_surfaces_denial( + "repo_create", + "POST", + mockito::Matcher::Regex(r"^/api/v1/repos$".to_string()), + json!({"name": "r"}), + false, + ) + .await; + } + + #[tokio::test] + async fn repo_list_via_mcp_surfaces_denial() { + assert_tool_surfaces_denial( + "repo_list", + "GET", + mockito::Matcher::Regex(r"^/api/v1/repos$".to_string()), + json!({}), + false, + ) + .await; + } + + #[tokio::test] + async fn repo_list_federated_via_mcp_surfaces_denial() { + assert_tool_surfaces_denial( + "repo_list_federated", + "GET", + mockito::Matcher::Regex(r"^/api/v1/repos/federated$".to_string()), + json!({}), + false, + ) + .await; + } + + #[tokio::test] + async fn agent_register_via_mcp_surfaces_denial() { + assert_tool_surfaces_denial( + "agent_register", + "POST", + mockito::Matcher::Regex(r"^/api/register$".to_string()), + json!({}), + true, + ) + .await; + } + + #[tokio::test] + async fn pr_create_via_mcp_surfaces_denial() { + assert_tool_surfaces_denial( + "pr_create", + "POST", + mockito::Matcher::Regex(r"/pulls$".to_string()), + json!({"owner": "alice", "repo": "r", "head": "h", "title": "t"}), + true, + ) + .await; + } + + #[tokio::test] + async fn pr_review_via_mcp_surfaces_denial() { + assert_tool_surfaces_denial( + "pr_review", + "POST", + mockito::Matcher::Regex(r"/pulls/1/reviews$".to_string()), + json!({"owner": "alice", "repo": "r", "number": 1, "status": "approve"}), + true, + ) + .await; + } + + #[tokio::test] + async fn pr_merge_via_mcp_surfaces_denial() { + assert_tool_surfaces_denial( + "pr_merge", + "POST", + mockito::Matcher::Regex(r"/pulls/1/merge$".to_string()), + json!({"owner": "alice", "repo": "r", "number": 1}), + false, + ) + .await; + } + + #[tokio::test] + async fn webhook_create_via_mcp_surfaces_denial() { + assert_tool_surfaces_denial( + "webhook_create", + "POST", + mockito::Matcher::Regex(r"/hooks$".to_string()), + json!({"owner": "alice", "repo": "r", "url": "http://example.com"}), + true, + ) + .await; + } + + #[tokio::test] + async fn webhook_delete_via_mcp_surfaces_denial() { + assert_tool_surfaces_denial( + "webhook_delete", + "DELETE", + mockito::Matcher::Regex(r"/hooks/h1$".to_string()), + json!({"owner": "alice", "repo": "r", "id": "h1"}), + false, + ) + .await; + } + + #[tokio::test] + async fn bounty_show_via_mcp_surfaces_denial() { + assert_tool_surfaces_denial( + "bounty_show", + "GET", + mockito::Matcher::Regex(r"/api/v1/bounties/b1$".to_string()), + json!({"id": "b1"}), + false, + ) + .await; + } + + #[tokio::test] + async fn bounty_create_via_mcp_surfaces_denial() { + assert_tool_surfaces_denial( + "bounty_create", + "POST", + mockito::Matcher::Regex(r"/bounties$".to_string()), + json!({"repo": "a/b", "title": "t", "amount": 100}), + false, + ) + .await; + } + + #[tokio::test] + async fn bounty_claim_via_mcp_surfaces_denial() { + assert_tool_surfaces_denial( + "bounty_claim", + "POST", + mockito::Matcher::Regex(r"/api/v1/bounties/b1/claim$".to_string()), + json!({"id": "b1"}), + false, + ) + .await; + } + + #[tokio::test] + async fn bounty_submit_via_mcp_surfaces_denial() { + assert_tool_surfaces_denial( + "bounty_submit", + "POST", + mockito::Matcher::Regex(r"/api/v1/bounties/b1/submit$".to_string()), + json!({"id": "b1", "pr_id": "p1"}), + false, + ) + .await; + } + + #[tokio::test] + async fn bounty_stats_via_mcp_surfaces_denial() { + assert_tool_surfaces_denial( + "bounty_stats", + "GET", + mockito::Matcher::Regex(r"^/api/v1/bounties/stats$".to_string()), + json!({}), + false, + ) + .await; + } + + #[tokio::test] + async fn task_list_via_mcp_surfaces_denial() { + assert_tool_surfaces_denial( + "task_list", + "GET", + mockito::Matcher::Regex(r"/api/v1/tasks\?".to_string()), + json!({}), + false, + ) + .await; + } + + #[tokio::test] + async fn task_create_via_mcp_surfaces_denial() { + assert_tool_surfaces_denial( + "task_create", + "POST", + mockito::Matcher::Regex(r"^/api/v1/tasks$".to_string()), + json!({"kind": "code-review"}), + true, + ) + .await; + } + + #[tokio::test] + async fn task_claim_via_mcp_surfaces_denial() { + assert_tool_surfaces_denial( + "task_claim", + "POST", + mockito::Matcher::Regex(r"/api/v1/tasks/t1/claim$".to_string()), + json!({"id": "t1"}), + true, + ) + .await; + } + + #[tokio::test] + async fn task_complete_via_mcp_surfaces_denial() { + assert_tool_surfaces_denial( + "task_complete", + "POST", + mockito::Matcher::Regex(r"/api/v1/tasks/t1/complete$".to_string()), + json!({"id": "t1"}), + true, + ) + .await; + } + + #[tokio::test] + async fn issue_create_via_mcp_surfaces_denial() { + assert_tool_surfaces_denial( + "issue_create", + "POST", + mockito::Matcher::Regex(r"/issues$".to_string()), + json!({"owner": "alice", "repo": "r", "title": "t"}), + true, + ) + .await; + } + + #[tokio::test] + async fn issue_comment_via_mcp_surfaces_denial() { + assert_tool_surfaces_denial( + "issue_comment", + "POST", + mockito::Matcher::Regex(r"/issues/i1/comments$".to_string()), + json!({"owner": "alice", "repo": "r", "issue_id": "i1", "body": "b"}), + true, + ) + .await; + } + + #[tokio::test] + async fn git_refs_via_mcp_surfaces_denial() { + // git_refs reads pkt-line bytes, not JSON, so a 404 body would otherwise + // parse to an empty ref list and render as a successful (empty) result. + assert_tool_surfaces_denial( + "git_refs", + "GET", + mockito::Matcher::Regex(r"/info/refs".to_string()), + json!({"owner": "alice", "name": "r"}), + false, + ) + .await; + } + + #[tokio::test] + async fn repo_clone_url_via_mcp_surfaces_denial() { + // repo_clone_url resolves the node DID via GET / (read_json). A gated 404 + // there must Err (surfacing the status), not fabricate a clone URL. + assert_tool_surfaces_denial( + "repo_clone_url", + "GET", + mockito::Matcher::Regex(r"^/$".to_string()), + json!({"name": "r"}), + false, + ) + .await; + } + + #[tokio::test] + async fn mcp_resolve_owner_surfaces_denial() { + // With no "owner" arg, resolve_owner GETs / for the node DID via read_json. + // A gated 404 there must Err (surfacing the status), proving the conversion + // is load-bearing rather than silently ignored. + let mut server = mockito::Server::new_async().await; + let _m = server + .mock("GET", "/") + .with_status(404) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"denied"}"#) + .expect(1) + .create_async() + .await; + let err = resolve_owner(&json!({}), &NodeClient::new(server.url(), None)) + .await + .unwrap_err(); + assert!(err.to_string().contains("404"), "got: {err}"); + _m.assert_async().await; + } } diff --git a/crates/gl/src/peer.rs b/crates/gl/src/peer.rs index e9663640..4be3ab09 100644 --- a/crates/gl/src/peer.rs +++ b/crates/gl/src/peer.rs @@ -4,7 +4,6 @@ use anyhow::{Context, Result}; use clap::{Args, Subcommand}; -use serde_json::Value; use std::path::PathBuf; use crate::http::NodeClient; @@ -63,12 +62,7 @@ pub async fn run(args: PeerArgs) -> Result<()> { async fn cmd_list(node: String) -> Result<()> { let client = NodeClient::new(&node, None); - let resp: Value = client - .get("/api/v1/peers") - .await? - .json() - .await - .context("failed to list peers")?; + let resp = crate::http::read_json(client.get("/api/v1/peers").await?, "peers").await?; let peers = resp["peers"].as_array().cloned().unwrap_or_default(); let count = resp["count"].as_u64().unwrap_or(peers.len() as u64); @@ -101,18 +95,20 @@ async fn cmd_add(peer_url: String, node: String, dir: Option) -> Result let keypair = load_keypair_from_dir(dir.as_deref())?; let my_did = keypair.did().to_string(); - // Fetch our node's public URL so we can announce it to the peer + // Fetch our node's public URL so we can announce it to the peer. This lookup + // is a deliberate fail-soft diagnostic: it only improves the URL we advertise. + // A response-level failure (denial, error, garbage body) falls back to the + // --node value with a visible note rather than aborting; only a transport + // failure on the GET itself is fatal. The announce below keeps the + // fail-closed read_json check. let local_client = NodeClient::new(&node, None); - let node_info: Value = local_client - .get("/") - .await? - .json() - .await - .context("failed to fetch local node info")?; - let my_url = node_info["public_url"] - .as_str() - .unwrap_or(&node) - .to_string(); + let my_url = match crate::http::read_json(local_client.get("/").await?, "node info").await { + Ok(info) => info["public_url"].as_str().unwrap_or(&node).to_string(), + Err(e) => { + eprintln!("note: local node info unavailable ({e}); announcing {node}"); + node.clone() + } + }; // Announce our local node to the remote peer let body = serde_json::to_vec(&serde_json::json!({ @@ -126,13 +122,7 @@ async fn cmd_add(peer_url: String, node: String, dir: Option) -> Result .post(announce_path, &body) .await .context("failed to connect to peer")?; - let status = resp.status(); - let result: Value = resp.json().await.context("invalid JSON response")?; - - if !status.is_success() { - let msg = result["message"].as_str().unwrap_or("unknown error"); - anyhow::bail!("announce failed ({status}): {msg}"); - } + let result = crate::http::read_json(resp, "announce").await?; let their_did = result["node_did"].as_str().unwrap_or("?"); let their_url = result["node_url"].as_str().unwrap_or("?"); @@ -150,9 +140,17 @@ async fn cmd_add(peer_url: String, node: String, dir: Option) -> Result "did": their_did, "http_url": their_url, }))?; - // This requires the local node to be running; ignore errors here - let _ = local_client.post("/api/v1/peers/announce", &add_body).await; - println!(" Added to local peer list."); + // Best-effort: the local node may be down, and cmd_add still succeeds + // either way. But report the outcome honestly instead of printing a + // success line after a failed local add. + match local_client.post("/api/v1/peers/announce", &add_body).await { + Ok(resp) if resp.status().is_success() => { + println!(" Added to local peer list."); + } + _ => { + eprintln!("note: could not add the peer to the local node's peer list"); + } + } } Ok(()) @@ -161,12 +159,7 @@ async fn cmd_add(peer_url: String, node: String, dir: Option) -> Result async fn cmd_ping(did: String, node: String) -> Result<()> { let client = NodeClient::new(&node, None); let path = format!("/api/v1/peers/{did}/ping"); - let resp: Value = client - .get(&path) - .await? - .json() - .await - .context("failed to ping peer")?; + let resp = crate::http::read_json(client.get(&path).await?, "ping peer").await?; let url = resp["http_url"].as_str().unwrap_or("?"); let reachable = resp["reachable"].as_bool().unwrap_or(false); @@ -186,12 +179,7 @@ async fn cmd_resolve(did: String, node: String) -> Result<()> { let client = NodeClient::new(&node, None); let encoded = urlencoding::encode(&did); let path = format!("/api/v1/resolve/{encoded}"); - let resp: Value = client - .get(&path) - .await? - .json() - .await - .context("failed to resolve DID")?; + let resp = crate::http::read_json(client.get(&path).await?, "resolve DID").await?; let source = resp["source"].as_str().unwrap_or("not found"); let http_url = resp["http_url"].as_str().unwrap_or("(none)"); @@ -210,3 +198,286 @@ async fn cmd_resolve(did: String, node: String) -> Result<()> { } Ok(()) } + +#[cfg(test)] +mod tests { + use super::*; + + #[tokio::test] + async fn cmd_list_surfaces_denial_not_empty() { + // A node error must Err, not print "No known peers" as if the list were empty. + let mut server = mockito::Server::new_async().await; + let _m = server + .mock("GET", "/api/v1/peers") + .with_status(500) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"boom"}"#) + .expect(1) + .create_async() + .await; + let err = cmd_list(server.url()).await.unwrap_err(); + assert!(err.to_string().contains("500"), "got: {err}"); + _m.assert_async().await; + } + + #[tokio::test] + async fn cmd_ping_surfaces_denial_not_unreachable() { + // A node error must Err, not print a fabricated "unreachable" peer. + let mut server = mockito::Server::new_async().await; + let _m = server + .mock("GET", "/api/v1/peers/peer1/ping") + .with_status(404) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"not found"}"#) + .expect(1) + .create_async() + .await; + let err = cmd_ping("peer1".to_string(), server.url()) + .await + .unwrap_err(); + assert!(err.to_string().contains("404"), "got: {err}"); + _m.assert_async().await; + } + + #[tokio::test] + async fn cmd_resolve_surfaces_denial_not_notfound() { + // A node error must Err, not print "Source: not found" as if resolved-absent. + let mut server = mockito::Server::new_async().await; + let _m = server + .mock( + "GET", + mockito::Matcher::Regex(r"/api/v1/resolve/".to_string()), + ) + .with_status(500) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"boom"}"#) + .expect(1) + .create_async() + .await; + let err = cmd_resolve("peer1".to_string(), server.url()) + .await + .unwrap_err(); + assert!(err.to_string().contains("500"), "got: {err}"); + _m.assert_async().await; + } + + // cmd_add needs a local identity to build my_did before the GET /. + fn identity_dir() -> tempfile::TempDir { + let dir = tempfile::TempDir::new().unwrap(); + let kp = gitlawb_core::identity::Keypair::generate(); + std::fs::write( + dir.path().join("identity.pem"), + kp.to_pem().unwrap().as_bytes(), + ) + .unwrap(); + dir + } + + /// Mock a successful announce that requires `http_url` in the POSTed body. + /// The `{}` response body matters: it leaves `node_url` absent, so cmd_add + /// skips the follow-up local peer-list POST. + async fn announce_ok_mock(server: &mut mockito::ServerGuard, http_url: &str) -> mockito::Mock { + announce_mock_with_body(server, http_url, "{}").await + } + + /// Like [`announce_ok_mock`] but with a custom response body, for tests + /// that need `node_url` populated so the follow-up local add runs. + async fn announce_mock_with_body( + server: &mut mockito::ServerGuard, + http_url: &str, + body: &str, + ) -> mockito::Mock { + server + .mock("POST", "/api/v1/peers/announce") + .match_body(mockito::Matcher::PartialJson(serde_json::json!({ + "http_url": http_url, + }))) + .with_status(200) + .with_header("content-type", "application/json") + .with_body(body) + .expect(1) + .create_async() + .await + } + + // The local `GET /` lookup in cmd_add is a fail-soft diagnostic: a denied + // or error node-info response must fall back to announcing the --node URL + // (with a stderr note), not abort the command before the announce POST. + #[tokio::test] + async fn cmd_add_announces_fallback_url_when_node_info_denied() { + let mut local = mockito::Server::new_async().await; + let _info = local + .mock("GET", "/") + .with_status(500) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"internal"}"#) + .expect(1) + .create_async() + .await; + let mut remote = mockito::Server::new_async().await; + let _announce = announce_ok_mock(&mut remote, &local.url()).await; + + let dir = identity_dir(); + cmd_add(remote.url(), local.url(), Some(dir.path().to_path_buf())) + .await + .unwrap(); + _info.assert_async().await; + _announce.assert_async().await; + } + + // Same fallback for a 200 node-info response whose body is not JSON. + #[tokio::test] + async fn cmd_add_falls_back_on_malformed_node_info() { + let mut local = mockito::Server::new_async().await; + let _info = local + .mock("GET", "/") + .with_status(200) + .with_body("not json") + .expect(1) + .create_async() + .await; + let mut remote = mockito::Server::new_async().await; + let _announce = announce_ok_mock(&mut remote, &local.url()).await; + + let dir = identity_dir(); + cmd_add(remote.url(), local.url(), Some(dir.path().to_path_buf())) + .await + .unwrap(); + _info.assert_async().await; + _announce.assert_async().await; + } + + // Must-not case: the announce POST itself stays fail-closed. A denied + // announce surfaces as an Err naming the status, never a printed success. + #[tokio::test] + async fn cmd_add_surfaces_denied_announce() { + let mut local = mockito::Server::new_async().await; + let _info = local + .mock("GET", "/") + .with_status(200) + .with_header("content-type", "application/json") + .with_body(r#"{"public_url":"https://pub.example"}"#) + .expect(1) + .create_async() + .await; + let mut remote = mockito::Server::new_async().await; + let _announce = remote + .mock("POST", "/api/v1/peers/announce") + .with_status(403) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"denied"}"#) + .expect(1) + .create_async() + .await; + + let dir = identity_dir(); + let err = cmd_add(remote.url(), local.url(), Some(dir.path().to_path_buf())) + .await + .unwrap_err(); + assert!(err.to_string().contains("403"), "got: {err}"); + _info.assert_async().await; + _announce.assert_async().await; + } + + // Success path unchanged: a usable node-info response announces its + // public_url; the --node fallback must not leak in. + #[tokio::test] + async fn cmd_add_uses_public_url_when_lookup_succeeds() { + let mut local = mockito::Server::new_async().await; + let _info = local + .mock("GET", "/") + .with_status(200) + .with_header("content-type", "application/json") + .with_body(r#"{"public_url":"https://pub.example"}"#) + .expect(1) + .create_async() + .await; + let mut remote = mockito::Server::new_async().await; + let _announce = announce_ok_mock(&mut remote, "https://pub.example").await; + + let dir = identity_dir(); + cmd_add(remote.url(), local.url(), Some(dir.path().to_path_buf())) + .await + .unwrap(); + _info.assert_async().await; + _announce.assert_async().await; + } + + // When the peer's announce response carries a node_url, cmd_add posts the + // peer back to the local node's peer list; a successful local POST is the + // one case that prints the added line. + #[tokio::test] + async fn cmd_add_adds_peer_to_local_list_on_success() { + let mut local = mockito::Server::new_async().await; + let _info = local + .mock("GET", "/") + .with_status(200) + .with_header("content-type", "application/json") + .with_body(r#"{"public_url":"https://pub.example"}"#) + .expect(1) + .create_async() + .await; + let _local_add = local + .mock("POST", "/api/v1/peers/announce") + .with_status(200) + .with_header("content-type", "application/json") + .with_body(r#"{"status":"added"}"#) + .expect(1) + .create_async() + .await; + let mut remote = mockito::Server::new_async().await; + let _announce = announce_mock_with_body( + &mut remote, + "https://pub.example", + r#"{"node_did":"their-did","node_url":"http://their.example"}"#, + ) + .await; + + let dir = identity_dir(); + cmd_add(remote.url(), local.url(), Some(dir.path().to_path_buf())) + .await + .unwrap(); + _info.assert_async().await; + _announce.assert_async().await; + _local_add.assert_async().await; + } + + // The local add stays best-effort: a failing local POST must not fail the + // command (the announce to the remote peer already succeeded), it only + // changes the printed outcome. + #[tokio::test] + async fn cmd_add_stays_ok_when_local_add_fails() { + let mut local = mockito::Server::new_async().await; + let _info = local + .mock("GET", "/") + .with_status(200) + .with_header("content-type", "application/json") + .with_body(r#"{"public_url":"https://pub.example"}"#) + .expect(1) + .create_async() + .await; + let _local_add = local + .mock("POST", "/api/v1/peers/announce") + .with_status(500) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"boom"}"#) + .expect(1) + .create_async() + .await; + let mut remote = mockito::Server::new_async().await; + let _announce = announce_mock_with_body( + &mut remote, + "https://pub.example", + r#"{"node_did":"their-did","node_url":"http://their.example"}"#, + ) + .await; + + let dir = identity_dir(); + cmd_add(remote.url(), local.url(), Some(dir.path().to_path_buf())) + .await + .unwrap(); + _info.assert_async().await; + _announce.assert_async().await; + _local_add.assert_async().await; + } +} diff --git a/crates/gl/src/pr.rs b/crates/gl/src/pr.rs index 8e0c4b72..f10e6805 100644 --- a/crates/gl/src/pr.rs +++ b/crates/gl/src/pr.rs @@ -2,7 +2,6 @@ use anyhow::{Context, Result}; use clap::{Args, Subcommand}; -use serde_json::Value; use std::path::PathBuf; use crate::http::NodeClient; @@ -233,13 +232,7 @@ async fn cmd_create( .post(&format!("/api/v1/repos/{owner}/{repo}/pulls"), &payload) .await .context("failed to connect to node")?; - let status = resp.status(); - let pr: Value = resp.json().await.context("invalid JSON")?; - - if !status.is_success() { - let msg = pr["message"].as_str().unwrap_or("unknown error"); - anyhow::bail!("create PR failed ({status}): {msg}"); - } + let pr = crate::http::read_json(resp, "create PR").await?; let number = pr["number"].as_i64().unwrap_or(0); println!("✓ Opened PR #{number}: {title}"); @@ -254,12 +247,13 @@ async fn cmd_list(repo: String, node: String, dir: Option) -> Result<() let owner = resolve_owner(&keypair); let client = NodeClient::new(&node, None); - let resp: Value = client - .get(&format!("/api/v1/repos/{owner}/{repo}/pulls")) - .await? - .json() - .await - .context("invalid JSON")?; + let resp = crate::http::read_json( + client + .get(&format!("/api/v1/repos/{owner}/{repo}/pulls")) + .await?, + "pull requests", + ) + .await?; let prs = resp["pulls"].as_array().cloned().unwrap_or_default(); if prs.is_empty() { @@ -298,12 +292,13 @@ async fn cmd_view(repo: String, number: u64, node: String, dir: Option) let owner = resolve_owner(&keypair); let client = NodeClient::new(&node, None); - let pr: Value = client - .get(&format!("/api/v1/repos/{owner}/{repo}/pulls/{number}")) - .await? - .json() - .await - .context("invalid JSON")?; + let pr = crate::http::read_json( + client + .get(&format!("/api/v1/repos/{owner}/{repo}/pulls/{number}")) + .await?, + "pull request", + ) + .await?; let title = pr["title"].as_str().unwrap_or("?"); let status = pr["status"].as_str().unwrap_or("?"); @@ -321,14 +316,15 @@ async fn cmd_view(repo: String, number: u64, node: String, dir: Option) } // Show reviews - let reviews: Value = client - .get(&format!( - "/api/v1/repos/{owner}/{repo}/pulls/{number}/reviews" - )) - .await? - .json() - .await - .context("invalid JSON")?; + let reviews = crate::http::read_json( + client + .get(&format!( + "/api/v1/repos/{owner}/{repo}/pulls/{number}/reviews" + )) + .await?, + "reviews", + ) + .await?; let reviews = reviews["reviews"].as_array().cloned().unwrap_or_default(); if !reviews.is_empty() { println!("\nReviews ({}):", reviews.len()); @@ -354,14 +350,15 @@ async fn cmd_view(repo: String, number: u64, node: String, dir: Option) } // Show comments - let comments: Value = client - .get(&format!( - "/api/v1/repos/{owner}/{repo}/pulls/{number}/comments" - )) - .await? - .json() - .await - .context("invalid JSON")?; + let comments = crate::http::read_json( + client + .get(&format!( + "/api/v1/repos/{owner}/{repo}/pulls/{number}/comments" + )) + .await?, + "comments", + ) + .await?; let comments = comments["comments"].as_array().cloned().unwrap_or_default(); if !comments.is_empty() { println!("\nComments ({}):", comments.len()); @@ -386,12 +383,13 @@ async fn cmd_diff(repo: String, number: u64, node: String, dir: Option) let owner = resolve_owner(&keypair); let client = NodeClient::new(&node, None); - let resp: Value = client - .get(&format!("/api/v1/repos/{owner}/{repo}/pulls/{number}/diff")) - .await? - .json() - .await - .context("invalid JSON")?; + let resp = crate::http::read_json( + client + .get(&format!("/api/v1/repos/{owner}/{repo}/pulls/{number}/diff")) + .await?, + "diff", + ) + .await?; let diff = resp["diff"].as_str().unwrap_or(""); if diff.is_empty() { @@ -415,13 +413,7 @@ async fn cmd_merge(repo: String, number: u64, node: String, dir: Option ) .await .context("failed to connect to node")?; - let status = resp.status(); - let result: Value = resp.json().await.context("invalid JSON")?; - - if !status.is_success() { - let msg = result["message"].as_str().unwrap_or("unknown error"); - anyhow::bail!("merge failed ({status}): {msg}"); - } + let result = crate::http::read_json(resp, "merge").await?; let sha = result["merge_sha"].as_str().unwrap_or("?"); println!("✓ Merged PR #{number}"); @@ -453,13 +445,7 @@ async fn cmd_review( ) .await .context("failed to connect to node")?; - let code = resp.status(); - let result: Value = resp.json().await.context("invalid JSON")?; - - if !code.is_success() { - let msg = result["message"].as_str().unwrap_or("unknown error"); - anyhow::bail!("review failed ({code}): {msg}"); - } + let _ = crate::http::read_json(resp, "review").await?; let icon = match status.as_str() { "approved" => "✓", @@ -489,13 +475,7 @@ async fn cmd_comment( ) .await .context("failed to connect to node")?; - let code = resp.status(); - let result: Value = resp.json().await.context("invalid JSON")?; - - if !code.is_success() { - let msg = result["message"].as_str().unwrap_or("unknown error"); - anyhow::bail!("comment failed ({code}): {msg}"); - } + let _ = crate::http::read_json(resp, "comment").await?; println!("· Comment posted on PR #{number}"); Ok(()) @@ -506,14 +486,15 @@ async fn cmd_comments(repo: String, number: u64, node: String, dir: Option) -> Result<() .await .context("failed to connect to node")?; - let status = resp.status(); - let body: Value = resp.json().await.unwrap_or_default(); - - if !status.is_success() { - let msg = body["message"].as_str().unwrap_or("unknown error"); - anyhow::bail!("list protected branches failed ({status}): {msg}"); - } + let body = crate::http::read_json(resp, "list protected branches").await?; let branches = body["protected_branches"] .as_array() @@ -247,6 +223,7 @@ mod tests { .with_status(400) .with_header("content-type", "application/json") .with_body(r#"{"message":"only the repo owner can protect branches"}"#) + .expect(1) .create_async() .await; @@ -259,6 +236,8 @@ mod tests { .await .unwrap_err(); assert!(err.to_string().contains("protect failed")); + // Prove the mocked route was actually requested; a non-matching request (mockito's 501, also non-2xx) would otherwise satisfy the error assertion vacuously. + _m.assert_async().await; } #[tokio::test] @@ -370,4 +349,26 @@ mod tests { assert_eq!(owner, "alice"); assert_eq!(name, "myrepo"); } + + #[tokio::test] + async fn resolve_owner_repo_surfaces_denial() { + // A slash-free repo with an empty identity dir forces the GET / node-info + // fetch. A gated 404 there must Err (surfacing the status), proving the + // read_json conversion is load-bearing rather than silently ignored. + let mut server = mockito::Server::new_async().await; + let dir = tempfile::TempDir::new().unwrap(); // empty, no identity.pem, forces the GET / branch + let _m = server + .mock("GET", "/") + .with_status(404) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"denied"}"#) + .expect(1) + .create_async() + .await; + let err = resolve_owner_repo("noslash", &server.url(), Some(dir.path())) + .await + .unwrap_err(); + assert!(err.to_string().contains("404"), "got: {err}"); + _m.assert_async().await; + } } diff --git a/crates/gl/src/register.rs b/crates/gl/src/register.rs index 8a17a77e..b3c200ba 100644 --- a/crates/gl/src/register.rs +++ b/crates/gl/src/register.rs @@ -5,7 +5,7 @@ use anyhow::{Context, Result}; use clap::Args; -use serde_json::{json, Value}; +use serde_json::json; use std::path::PathBuf; use crate::http::NodeClient; @@ -55,16 +55,7 @@ pub async fn run(args: RegisterArgs) -> Result<()> { .await .context("failed to connect to node")?; - let status = resp.status(); - let payload: Value = resp.json().await.context("invalid JSON response")?; - - if !status.is_success() { - let msg = payload - .get("message") - .and_then(|v| v.as_str()) - .unwrap_or("unknown error"); - anyhow::bail!("registration failed ({status}): {msg}"); - } + let payload = crate::http::read_json(resp, "registration").await?; // Save bootstrap UCAN let ucan = payload.get("ucan").and_then(|v| v.as_str()).unwrap_or(""); @@ -171,6 +162,7 @@ mod tests { .with_status(401) .with_header("content-type", "application/json") .with_body(r#"{"message":"invalid signature"}"#) + .expect(1) .create_async() .await; @@ -187,6 +179,8 @@ mod tests { .unwrap_err() .to_string() .contains("invalid signature")); + // Prove the mocked route was actually requested; a non-matching request (mockito's 501, also non-2xx) would otherwise satisfy is_err() vacuously. + _m.assert_async().await; } #[tokio::test] diff --git a/crates/gl/src/repo.rs b/crates/gl/src/repo.rs index c75a7667..196a9515 100644 --- a/crates/gl/src/repo.rs +++ b/crates/gl/src/repo.rs @@ -251,13 +251,7 @@ async fn cmd_create( .post("/api/v1/repos", &body) .await .context("failed to connect to node")?; - let status = resp.status(); - let payload: Value = resp.json().await.context("invalid JSON response")?; - - if !status.is_success() { - let msg = payload["message"].as_str().unwrap_or("unknown error"); - anyhow::bail!("create failed ({status}): {msg}"); - } + let payload = crate::http::read_json(resp, "create").await?; let clone_url = payload["clone_url"].as_str().unwrap_or(""); let gitlawb_url = format!("gitlawb://{owner_did}/{name}"); @@ -277,12 +271,7 @@ async fn cmd_list(node: String, dir: Option) -> Result<()> { let client = NodeClient::new(&node, None); let url = format!("/api/v1/repos?owner={owner}"); - let repos: Value = client - .get(&url) - .await? - .json() - .await - .context("failed to list repos")?; + let repos = crate::http::read_json(client.get(&url).await?, "list repos").await?; let repos = repos.as_array().context("expected array")?; if repos.is_empty() { @@ -351,20 +340,13 @@ async fn cmd_info(repo: String, node: String, dir: Option) -> Result<() // A non-existent (or unreadable/quarantined) repo is a real 404 from the // node — surface it plainly instead of printing a stub card with `?` fields // and a placeholder owner DID. - if !resp.status().is_success() { - if resp.status().as_u16() == 404 { - anyhow::bail!("repository '{owner}/{name}' not found"); - } - let status = resp.status(); - let msg = resp - .json::() - .await - .ok() - .and_then(|v| v["message"].as_str().map(String::from)) - .unwrap_or_else(|| "request failed".to_string()); - anyhow::bail!("repo info failed ({status}): {msg}"); + if resp.status().as_u16() == 404 { + anyhow::bail!("repository '{owner}/{name}' not found"); } - let r: Value = resp.json().await.context("parse repo info")?; + // Every other status routes through read_json: a 2xx yields the parsed body, + // any other non-2xx yields the node's capped + sanitized message (INV-6), and + // the error read is bounded rather than buffering the whole hostile body. + let r = crate::http::read_json(resp, "repo info").await?; let owner_did = r["owner_did"].as_str().unwrap_or(&owner); let gitlawb_url = format!("gitlawb://{owner_did}/{name}"); @@ -423,12 +405,7 @@ async fn cmd_replica_register( .await .context("failed to connect to origin node")?; - let status = resp.status(); - let body: Value = resp.json().await.unwrap_or_default(); - if !status.is_success() { - let msg = body["message"].as_str().unwrap_or("unknown error"); - anyhow::bail!("replica register failed ({status}): {msg}"); - } + let body = crate::http::read_json(resp, "replica register").await?; let count = body["replica_count"].as_i64().unwrap_or(0); println!("Registered as replica of {owner}/{name}"); @@ -455,12 +432,7 @@ async fn cmd_replica_unregister(repo: String, node: String, dir: Option .await .context("failed to connect to origin node")?; - let status = resp.status(); - let body: Value = resp.json().await.unwrap_or_default(); - if !status.is_success() { - let msg = body["message"].as_str().unwrap_or("unknown error"); - anyhow::bail!("replica unregister failed ({status}): {msg}"); - } + let body = crate::http::read_json(resp, "replica unregister").await?; let count = body["replica_count"].as_i64().unwrap_or(0); println!("Unregistered as replica of {owner}/{name} ({count} replicas remaining)"); @@ -480,12 +452,7 @@ async fn cmd_replicas(repo: String, node: String, dir: Option) -> Resul .get_maybe_signed(&format!("/api/v1/repos/{owner}/{name}/replicas")) .await .context("failed to connect to node")?; - let status = resp.status(); - let body: Value = resp.json().await.unwrap_or_default(); - if !status.is_success() { - let msg = body["message"].as_str().unwrap_or("unknown error"); - anyhow::bail!("replicas list failed ({status}): {msg}"); - } + let body = crate::http::read_json(resp, "replicas list").await?; let count = body["replica_count"].as_i64().unwrap_or(0); println!("{owner}/{name}: {count} replicas"); @@ -521,12 +488,7 @@ pub(crate) async fn cmd_commits( }; let url = format!("/api/v1/repos/{owner}/{name}/commits?branch={branch}&limit={limit}"); - let resp: Value = client - .get(&url) - .await? - .json() - .await - .context("failed to fetch commits")?; + let resp = crate::http::read_json(client.get(&url).await?, "commits").await?; let commits = resp["commits"].as_array().cloned().unwrap_or_default(); if commits.is_empty() { @@ -579,13 +541,7 @@ async fn cmd_fork( .post(&format!("/api/v1/repos/{owner}/{repo_name}/fork"), &body) .await .context("failed to connect to node")?; - let status = resp.status(); - let result: Value = resp.json().await.context("invalid JSON response")?; - - if !status.is_success() { - let msg = result["message"].as_str().unwrap_or("unknown error"); - anyhow::bail!("fork failed ({status}): {msg}"); - } + let result = crate::http::read_json(resp, "fork").await?; let fork_name = result["name"].as_str().unwrap_or(&repo_name); let owner_did = result["owner_did"].as_str().unwrap_or("?"); @@ -623,13 +579,7 @@ async fn cmd_label_add( .post(&format!("/api/v1/repos/{owner}/{name}/labels"), &body) .await .context("failed to connect to node")?; - let status = resp.status(); - let result: Value = resp.json().await.context("invalid JSON")?; - - if !status.is_success() { - let msg = result["message"].as_str().unwrap_or("unknown error"); - anyhow::bail!("add label failed ({status}): {msg}"); - } + let result = crate::http::read_json(resp, "add label").await?; let added = result["added"].as_bool().unwrap_or(true); if added { @@ -654,13 +604,7 @@ async fn cmd_label_remove( .delete(&format!("/api/v1/repos/{owner}/{name}/labels/{label}"), &[]) .await .context("failed to connect to node")?; - let status = resp.status(); - let result: Value = resp.json().await.context("invalid JSON")?; - - if !status.is_success() { - let msg = result["message"].as_str().unwrap_or("unknown error"); - anyhow::bail!("remove label failed ({status}): {msg}"); - } + let _ = crate::http::read_json(resp, "remove label").await?; println!("- Label removed: {label} from {owner}/{name}"); Ok(()) @@ -673,18 +617,21 @@ async fn cmd_label_list(repo: String, node: String, dir: Option) -> Res // read their own labels, while public repos stay anonymously listable. let client = NodeClient::new(&node, load_keypair_from_dir(dir.as_deref()).ok()); - let resp = client - .get_maybe_signed(&format!("/api/v1/repos/{owner}/{name}/labels")) - .await - .context("failed to connect to node")?; - let status = resp.status(); - let body: Value = resp.json().await.unwrap_or_default(); - if !status.is_success() { - let msg = body["message"].as_str().unwrap_or("unknown error"); - anyhow::bail!("list labels failed ({status}): {msg}"); - } + // Marry both refactors: main's get_maybe_signed (read-visibility gating — + // public repos stay anon-listable, private-repo owner signs) carrying the + // PR's read_json status-check-before-parse (#186: bounded error read, no + // parse-before-status). get_authed from the PR is dropped in favor of + // get_maybe_signed so the visibility gate is preserved. + let resp = crate::http::read_json( + client + .get_maybe_signed(&format!("/api/v1/repos/{owner}/{name}/labels")) + .await + .context("failed to connect to node")?, + "list labels", + ) + .await?; - let labels = body["labels"].as_array().cloned().unwrap_or_default(); + let labels = resp["labels"].as_array().cloned().unwrap_or_default(); if labels.is_empty() { println!("No labels on {owner}/{name}"); } else { @@ -857,6 +804,7 @@ mod tests { .with_status(409) .with_header("content-type", "application/json") .with_body(r#"{"message":"repository already exists"}"#) + .expect(1) .create_async() .await; @@ -872,6 +820,8 @@ mod tests { assert!(result.is_err()); assert!(result.unwrap_err().to_string().contains("already exists")); + // Prove the mocked route was actually requested; a non-matching request (mockito's 501, also non-2xx) would otherwise satisfy is_err() vacuously. + _m.assert_async().await; } #[tokio::test] @@ -940,6 +890,33 @@ mod tests { _m.assert_async().await; } + #[tokio::test] + async fn cmd_list_repos_surfaces_denial() { + // A node error must Err with the status, not trip the vague "expected + // array" fallback that hides the node's actual response (INV-8). + let dir = TempDir::new().unwrap(); + write_identity(&dir); + + let mut server = mockito::Server::new_async().await; + let _m = server + .mock( + "GET", + mockito::Matcher::Regex(r"^/api/v1/repos\?owner=".to_string()), + ) + .with_status(500) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"boom"}"#) + .expect(1) + .create_async() + .await; + + let err = cmd_list(server.url(), Some(dir.path().to_path_buf())) + .await + .unwrap_err(); + assert!(err.to_string().contains("500"), "got: {err}"); + _m.assert_async().await; + } + #[tokio::test] async fn test_cmd_commits_empty() { let dir = TempDir::new().unwrap(); @@ -1039,6 +1016,7 @@ mod tests { .with_status(400) .with_header("content-type", "application/json") .with_body(r#"{"message":"you already have a repo named myrepo"}"#) + .expect(1) .create_async() .await; @@ -1054,6 +1032,8 @@ mod tests { err.to_string().contains("already have a repo"), "got: {err}" ); + // Prove the mocked route was actually requested; a non-matching request (mockito's 501, also non-2xx) would otherwise satisfy is_err() vacuously. + _m.assert_async().await; } #[tokio::test] @@ -1426,6 +1406,7 @@ mod tests { .with_status(404) .with_header("content-type", "application/json") .with_body(r#"{"message":"not found"}"#) + .expect(1) .create_async() .await; @@ -1438,6 +1419,106 @@ mod tests { .await .unwrap_err(); assert!(err.to_string().contains("not found"), "got: {err}"); + // Prove the mocked route was actually requested; a non-matching request (mockito's 501, also non-2xx) would otherwise satisfy is_err() vacuously. + _repo.assert_async().await; + } + + // ── Gated CLI reads surface node denials, not empty renders (#123 / INV-8) ── + + #[tokio::test] + async fn cmd_commits_surfaces_denial_not_empty() { + // A gated 404 must Err, not print "No commits" as if the repo were empty. + let mut server = mockito::Server::new_async().await; + let _m = server + .mock( + "GET", + mockito::Matcher::Regex(r"^/api/v1/repos/alice/secret/commits".to_string()), + ) + .with_status(404) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"repository 'alice/secret' not found"}"#) + .expect(1) + .create_async() + .await; + let result = cmd_commits( + "alice/secret".to_string(), + "main".to_string(), + 20, + server.url(), + None, + ) + .await; + assert!(result.is_err(), "cmd_commits must Err on 404"); + assert!(result.unwrap_err().to_string().contains("not found")); + // Prove the mocked route was actually requested; a non-matching request (mockito's 501, also non-2xx) would otherwise satisfy is_err() vacuously. + _m.assert_async().await; + } + + #[tokio::test] + async fn cmd_label_list_surfaces_denial_not_empty() { + let mut server = mockito::Server::new_async().await; + let _m = server + .mock( + "GET", + mockito::Matcher::Regex(r"^/api/v1/repos/alice/secret/labels".to_string()), + ) + .with_status(404) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"repository 'alice/secret' not found"}"#) + .expect(1) + .create_async() + .await; + let result = cmd_label_list("alice/secret".to_string(), server.url(), None).await; + assert!(result.is_err(), "cmd_label_list must Err on 404"); + // Prove the mocked route was actually requested; a non-matching request (mockito's 501, also non-2xx) would otherwise satisfy is_err() vacuously. + _m.assert_async().await; + } + + // cmd_info keeps its bespoke 404 message ahead of read_json... + #[tokio::test] + async fn cmd_info_404_keeps_bespoke_message() { + let mut server = mockito::Server::new_async().await; + let _m = server + .mock("GET", "/api/v1/repos/alice/secret") + .with_status(404) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"gone"}"#) + .expect(1) + .create_async() + .await; + let err = cmd_info("alice/secret".to_string(), server.url(), None) + .await + .unwrap_err() + .to_string(); + assert!( + err.contains("repository 'alice/secret' not found"), + "bespoke 404 text lost: {err}" + ); + _m.assert_async().await; + } + + // ...and every other non-2xx routes through read_json (capped + sanitized). + #[tokio::test] + async fn cmd_info_500_routes_through_read_json() { + let mut server = mockito::Server::new_async().await; + let _m = server + .mock("GET", "/api/v1/repos/alice/secret") + .with_status(500) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"boom"}"#) + .expect(1) + .create_async() + .await; + let err = cmd_info("alice/secret".to_string(), server.url(), None) + .await + .unwrap_err() + .to_string(); + assert!( + err.contains("repo info failed"), + "read_json label lost: {err}" + ); + assert!(err.contains("500"), "status not surfaced: {err}"); + _m.assert_async().await; } #[tokio::test] diff --git a/crates/gl/src/star.rs b/crates/gl/src/star.rs index 120f824a..62e2f2ca 100644 --- a/crates/gl/src/star.rs +++ b/crates/gl/src/star.rs @@ -2,7 +2,6 @@ use anyhow::{Context, Result}; use clap::{Args, Subcommand}; -use serde_json::Value; use std::path::PathBuf; use crate::http::NodeClient; @@ -75,13 +74,7 @@ async fn cmd_add(repo: String, node: String, dir: Option) -> Result<()> .await .context("failed to connect to node")?; - let status = resp.status(); - let body: Value = resp.json().await.unwrap_or_default(); - - if !status.is_success() { - let msg = body["message"].as_str().unwrap_or("unknown error"); - anyhow::bail!("star failed ({status}): {msg}"); - } + let body = crate::http::read_json(resp, "star").await?; let count = body["star_count"].as_i64().unwrap_or(0); println!("Starred {owner}/{name} ({count} stars total)"); @@ -99,13 +92,7 @@ async fn cmd_remove(repo: String, node: String, dir: Option) -> Result< .await .context("failed to connect to node")?; - let status = resp.status(); - let body: Value = resp.json().await.unwrap_or_default(); - - if !status.is_success() { - let msg = body["message"].as_str().unwrap_or("unknown error"); - anyhow::bail!("unstar failed ({status}): {msg}"); - } + let body = crate::http::read_json(resp, "unstar").await?; let count = body["star_count"].as_i64().unwrap_or(0); println!("Unstarred {owner}/{name} ({count} stars remaining)"); @@ -124,13 +111,7 @@ async fn cmd_count(repo: String, node: String, dir: Option) -> Result<( .await .context("failed to connect to node")?; - let status = resp.status(); - let body: Value = resp.json().await.unwrap_or_default(); - - if !status.is_success() { - let msg = body["message"].as_str().unwrap_or("unknown error"); - anyhow::bail!("star count failed ({status}): {msg}"); - } + let body = crate::http::read_json(resp, "star count").await?; let count = body["star_count"].as_i64().unwrap_or(0); println!("{owner}/{name}: {count} stars"); @@ -230,6 +211,7 @@ mod tests { .with_status(404) .with_header("content-type", "application/json") .with_body(r#"{"message":"repo not found"}"#) + .expect(1) .create_async() .await; @@ -241,6 +223,8 @@ mod tests { .await .unwrap_err(); assert!(err.to_string().contains("star failed")); + // Prove the mocked route was actually requested; a non-matching request (mockito's 501, also non-2xx) would otherwise satisfy the error assertion vacuously. + _m.assert_async().await; } #[tokio::test] @@ -287,6 +271,7 @@ mod tests { .with_status(404) .with_header("content-type", "application/json") .with_body(r#"{"message":"repo not found"}"#) + .expect(1) .create_async() .await; @@ -298,6 +283,8 @@ mod tests { .await .unwrap_err(); assert!(err.to_string().contains("unstar failed")); + // Prove the mocked route was actually requested; a non-matching request (mockito's 501, also non-2xx) would otherwise satisfy the error assertion vacuously. + _m.assert_async().await; } #[tokio::test] @@ -335,4 +322,51 @@ mod tests { assert_eq!(owner, "alice"); assert_eq!(name, "myrepo"); } + + // U15 (#186): a converted handler must inherit read_json's capped + sanitized + // error path end-to-end. A hostile 500 whose `message` carries terminal-control + // + bidi bytes and is long must reach the terminal neither verbatim nor unbounded. + #[tokio::test] + async fn cmd_add_hostile_error_is_sanitized_and_bounded() { + let mut server = mockito::Server::new_async().await; + let dir = tempfile::TempDir::new().unwrap(); + let kp = gitlawb_core::identity::Keypair::generate(); + std::fs::write( + dir.path().join("identity.pem"), + kp.to_pem().unwrap().as_bytes(), + ) + .unwrap(); + + // ESC + a long run + a right-to-left override, all inside the node message + // (JSON \u escapes so the source has no raw control bytes). + let hostile = format!(r#"{{"message":"a\u001b[31m{}b\u202ec"}}"#, "Z".repeat(500)); + let _m = server + .mock("PUT", mockito::Matcher::Regex(r"/star$".to_string())) + .with_status(500) + .with_header("content-type", "application/json") + .with_body(hostile) + .expect(1) + .create_async() + .await; + + let err = cmd_add( + "myrepo".to_string(), + server.url(), + Some(dir.path().to_path_buf()), + ) + .await + .unwrap_err() + .to_string(); + + assert!( + !err.contains('\u{1b}'), + "ESC leaked to the terminal: {err:?}" + ); + assert!(!err.contains('\u{202e}'), "bidi override leaked: {err:?}"); + // Bounded: sanitize_node_msg caps the surfaced message, so the 500-char run + // cannot flood the error string. + assert!(err.len() < 300, "error not bounded ({} bytes)", err.len()); + assert!(err.contains("star failed"), "handler label lost: {err}"); + _m.assert_async().await; + } } diff --git a/crates/gl/src/status.rs b/crates/gl/src/status.rs index cf477cc6..ecc6d42e 100644 --- a/crates/gl/src/status.rs +++ b/crates/gl/src/status.rs @@ -37,17 +37,9 @@ pub async fn run(args: StatusArgs) -> Result<()> { let client = NodeClient::new(&args.node, None); if let Some(ref did) = maybe_did { let short_key = did.split(':').next_back().unwrap_or(did); - match client.get(&format!("/api/v1/agents/{short_key}")).await { - Ok(resp) if resp.status().is_success() => { - if let Ok(body) = resp.json::().await { - let score = body["trust_score"].as_f64().unwrap_or(0.0); - let bar = trust_bar(score); - println!(" trust {score:.2} {bar}"); - } - } - _ => { - println!(" trust — not registered (run `gl register`)"); - } + let resp = client.get(&format!("/api/v1/agents/{short_key}")).await; + if let Some(line) = trust_line(resp).await { + println!("{line}"); } } @@ -81,7 +73,9 @@ pub async fn run(args: StatusArgs) -> Result<()> { let pr_resp = client .get(&format!("/api/v1/repos/{short_owner}/{repo_name}/pulls")) .await; - if let Ok(r) = pr_resp { + if let Some(line) = section_unavailable_line("PRs", &pr_resp) { + println!("{line}"); + } else if let Ok(r) = pr_resp { if let Ok(body) = r.json::().await { let prs = body["pulls"].as_array().cloned().unwrap_or_default(); let open: Vec<_> = prs @@ -108,7 +102,9 @@ pub async fn run(args: StatusArgs) -> Result<()> { let issue_resp = client .get(&format!("/api/v1/repos/{short_owner}/{repo_name}/issues")) .await; - if let Ok(r) = issue_resp { + if let Some(line) = section_unavailable_line("issues", &issue_resp) { + println!("{line}"); + } else if let Ok(r) = issue_resp { if let Ok(body) = r.json::().await { let issues = body["issues"].as_array().cloned().unwrap_or_default(); let open: Vec<_> = issues @@ -136,6 +132,40 @@ pub async fn run(args: StatusArgs) -> Result<()> { Ok(()) } +/// The status line for a repo section (PRs / issues) whose gated read returned a +/// non-2xx: the denial must surface, never render as "no open ..." (INV-8). +/// Returns `None` for a success (the caller renders the body) or a transport error +/// (the section degrades silently — R5 — so the multi-section `gl status` never +/// hard-fails on one denied read). +fn section_unavailable_line(label: &str, resp: &Result) -> Option { + match resp { + Ok(r) if !r.status().is_success() => { + Some(format!(" {label:<10}unavailable ({})", r.status())) + } + _ => None, + } +} + +/// The `trust` status line for the caller's own identity. A genuine 404 means the +/// identity is not registered; any OTHER non-2xx (403/429/5xx) is a failed lookup +/// and must surface as unavailable rather than fabricating an unregistered state +/// (INV-8). A transport error degrades silently — the node-unreachable line below +/// already surfaces it — matching `section_unavailable_line`. +async fn trust_line(resp: Result) -> Option { + match resp { + Ok(r) if r.status().is_success() => { + let body = r.json::().await.ok()?; + let score = body["trust_score"].as_f64().unwrap_or(0.0); + Some(format!(" trust {score:.2} {}", trust_bar(score))) + } + Ok(r) if r.status() == reqwest::StatusCode::NOT_FOUND => { + Some(" trust — not registered (run `gl register`)".to_string()) + } + Ok(r) => Some(format!(" trust unavailable ({})", r.status())), + Err(_) => None, + } +} + /// Render a simple ASCII trust bar: 0.75 → "███░" fn trust_bar(score: f64) -> String { let filled = (score * 4.0).round() as usize; @@ -376,4 +406,125 @@ mod tests { fn trust_bar_quarter() { assert_eq!(trust_bar(0.25), "█░░░"); } + + // ── gl status section denial surfacing (#123 / INV-8, R5) ──────────── + + async fn get_response( + server: &mut mockito::Server, + status: usize, + ) -> Result { + let _m = server + .mock("GET", "/x") + .with_status(status) + .with_header("content-type", "application/json") + .with_body(r#"{"pulls":[]}"#) + .create_async() + .await; + NodeClient::new(server.url(), None).get("/x").await + } + + #[tokio::test] + async fn gated_section_surfaces_unavailable_not_empty() { + // A gated 404 on a status section must surface "unavailable", never be + // treated as "no open PRs" (INV-8). + let mut server = mockito::Server::new_async().await; + let resp = get_response(&mut server, 404).await; + assert_eq!( + section_unavailable_line("PRs", &resp), + Some(format!(" {:<10}unavailable (404 Not Found)", "PRs")) + ); + } + + #[tokio::test] + async fn success_section_returns_none_for_body_render() { + // A 2xx returns None so the caller renders the body as before. + let mut server = mockito::Server::new_async().await; + let resp = get_response(&mut server, 200).await; + assert!(section_unavailable_line("issues", &resp).is_none()); + } + + #[test] + fn transport_error_degrades_silently() { + // A transport error (not a status) must NOT surface — the section + // degrades silently so gl status never hard-fails on one bad read (R5). + let err: Result = Err(anyhow::anyhow!("connection refused")); + assert!(section_unavailable_line("PRs", &err).is_none()); + } + + // ── trust_line: a 404 means unregistered; any OTHER failure must surface as + // unavailable, never fabricate an unregistered verdict (INV-8). ────────── + async fn agents_resp( + server: &mut mockito::Server, + status: usize, + body: &str, + ) -> Result { + let _m = server + .mock( + "GET", + mockito::Matcher::Regex(r"^/api/v1/agents/".to_string()), + ) + .with_status(status) + .with_header("content-type", "application/json") + .with_body(body) + .create_async() + .await; + NodeClient::new(server.url(), None) + .get("/api/v1/agents/zTest") + .await + } + + #[tokio::test] + async fn trust_line_registered_shows_score_only() { + let mut s = mockito::Server::new_async().await; + let line = trust_line(agents_resp(&mut s, 200, r#"{"trust_score":0.5}"#).await) + .await + .unwrap(); + assert!(line.contains("0.50"), "line={line}"); + assert!(!line.contains("not registered"), "line={line}"); + assert!(!line.contains("unavailable"), "line={line}"); + } + + #[tokio::test] + async fn trust_line_404_is_not_registered() { + let mut s = mockito::Server::new_async().await; + let line = trust_line(agents_resp(&mut s, 404, r#"{"message":"agent not found"}"#).await) + .await + .unwrap(); + assert!(line.contains("not registered"), "line={line}"); + assert!(!line.contains("unavailable"), "line={line}"); + } + + // Load-bearing: a 403 for an existing identity is a denied lookup, not proof it is unregistered. + #[tokio::test] + async fn trust_line_403_is_unavailable_not_registered() { + let mut s = mockito::Server::new_async().await; + let line = trust_line(agents_resp(&mut s, 403, r#"{"message":"forbidden"}"#).await) + .await + .unwrap(); + assert!( + line.contains("unavailable") && line.contains("403"), + "line={line}" + ); + assert!(!line.contains("not registered"), "line={line}"); + } + + // Load-bearing: a 5xx is a failed lookup, not an unregistered verdict. + #[tokio::test] + async fn trust_line_500_is_unavailable_not_registered() { + let mut s = mockito::Server::new_async().await; + let line = trust_line(agents_resp(&mut s, 500, r#"{"message":"boom"}"#).await) + .await + .unwrap(); + assert!(line.contains("unavailable"), "line={line}"); + assert!(!line.contains("not registered"), "line={line}"); + } + + // Load-bearing: a transport error degrades silently, never fabricates unregistered. + #[tokio::test] + async fn trust_line_transport_error_is_silent() { + let resp = NodeClient::new("http://127.0.0.1:1", None) + .get("/api/v1/agents/zTest") + .await; + assert!(trust_line(resp).await.is_none()); + } } diff --git a/crates/gl/src/sync.rs b/crates/gl/src/sync.rs index 634b1c0d..cb663c0a 100644 --- a/crates/gl/src/sync.rs +++ b/crates/gl/src/sync.rs @@ -71,8 +71,9 @@ pub async fn run(args: SyncArgs) -> Result<()> { SyncCmd::Status => { let client = NodeClient::new(&args.node, None); // Just show peer list and node stats for now - let stats: serde_json::Value = client.get("/api/v1/stats").await?.json().await?; - let peers: serde_json::Value = client.get("/api/v1/peers").await?.json().await?; + let stats = + crate::http::read_json(client.get("/api/v1/stats").await?, "node stats").await?; + let peers = crate::http::read_json(client.get("/api/v1/peers").await?, "peers").await?; println!("Node stats:"); println!(" repos: {}", stats["repos"].as_i64().unwrap_or(0)); println!(" agents: {}", stats["agents"].as_i64().unwrap_or(0)); @@ -107,7 +108,7 @@ fn trigger_counts(resp: &serde_json::Value) -> (u64, u64) { /// Read at most `cap` bytes of a response body. Bounds the allocation from a /// hostile or broken node returning a huge error body — the display is capped /// separately, but the read itself must not be unbounded (INV-6, read half). -async fn read_body_capped(mut resp: reqwest::Response, cap: usize) -> String { +pub(crate) async fn read_body_capped(mut resp: reqwest::Response, cap: usize) -> String { let mut buf: Vec = Vec::new(); while buf.len() < cap { match resp.chunk().await { @@ -130,7 +131,7 @@ async fn read_body_capped(mut resp: reqwest::Response, cap: usize) -> String { /// reach the terminal verbatim (INV-6). We drop the C0/C1 control bytes (which /// defangs ANSI/OSC escapes) AND the Unicode bidi/format controls (which /// `char::is_control` does not cover — they can reorder the displayed line). -fn sanitize_node_msg(s: &str) -> String { +pub(crate) fn sanitize_node_msg(s: &str) -> String { s.chars() .filter(|c| !c.is_control() && !gitlawb_core::sanitize::is_bidi_format(*c)) .take(200) @@ -191,6 +192,7 @@ mod tests { // Valid JSON: the parse-without-status-check bug deserializes this // into a zero-count success struct and prints "✓ sync triggered". .with_body(r#"{"message":"unauthorized"}"#) + .expect(1) .create_async() .await; let (args, _dir) = trigger_args(server.url()); @@ -199,6 +201,8 @@ mod tests { err.to_string().contains("401"), "expected 401 surfaced, got: {err}" ); + // Prove the mocked route was actually requested; a non-matching request (mockito's 501, also non-2xx) would otherwise satisfy the error assertion vacuously. + _m.assert_async().await; } #[tokio::test] @@ -209,6 +213,7 @@ mod tests { .with_status(429) .with_header("content-type", "application/json") .with_body(r#"{"message":"slow down"}"#) + .expect(1) .create_async() .await; let (args, _dir) = trigger_args(server.url()); @@ -217,6 +222,8 @@ mod tests { err.to_string().contains("429"), "expected 429 surfaced, got: {err}" ); + // Prove the mocked route was actually requested; a non-matching request (mockito's 501, also non-2xx) would otherwise satisfy the error assertion vacuously. + _m.assert_async().await; } #[tokio::test] @@ -233,6 +240,7 @@ mod tests { // BEL (\u0007); serde decodes them to real control bytes a naive // client would print. (The status-check bug fake-successes here.) .with_body("{\"message\":\"pwned\\u001b[31m\\u0007bad\"}") + .expect(1) .create_async() .await; let (args, _dir) = trigger_args(server.url()); @@ -244,6 +252,8 @@ mod tests { s.contains("pwned") && s.contains("bad"), "message text dropped: {s:?}" ); + // Prove the mocked route was actually requested; a non-matching request (mockito's 501, also non-2xx) would otherwise satisfy the error assertion vacuously. + _m.assert_async().await; } #[tokio::test] @@ -300,6 +310,7 @@ mod tests { .mock("POST", "/api/v1/sync/trigger") .with_status(401) .with_body("A".repeat(2_000_000)) + .expect(1) .create_async() .await; let (args, _dir) = trigger_args(server.url()); @@ -311,6 +322,8 @@ mod tests { "error message not bounded: {} chars", s.len() ); + // Prove the mocked route was actually requested; a non-matching request (mockito's 501, also non-2xx) would otherwise satisfy the error assertion vacuously. + _m.assert_async().await; } #[tokio::test] @@ -341,4 +354,71 @@ mod tests { (0, 0) ); } + + #[tokio::test] + async fn status_surfaces_stats_denial_not_fake_zeros() { + // A node error on /stats must Err, not print "0 repos / 0 agents / 0 pushes". + let mut server = mockito::Server::new_async().await; + let stats = server + .mock("GET", "/api/v1/stats") + .with_status(500) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"boom"}"#) + .expect(1) + .create_async() + .await; + // A peers mock so the pre-fix path reaches a clean success rather than a + // 501 on an unmocked route; after the fix the /stats denial bails first. + let _peers = server + .mock("GET", "/api/v1/peers") + .with_status(200) + .with_header("content-type", "application/json") + .with_body(r#"{"count":0,"peers":[]}"#) + .create_async() + .await; + let args = SyncArgs { + cmd: SyncCmd::Status, + node: server.url(), + dir: None, + }; + let err = run(args).await.unwrap_err(); + assert!( + err.to_string().contains("500"), + "expected 500 surfaced, got: {err}" + ); + stats.assert_async().await; + } + + #[tokio::test] + async fn status_surfaces_peers_denial() { + // /stats succeeds but /peers denies — the peers read must Err too, not + // print "Known peers: 0". + let mut server = mockito::Server::new_async().await; + let _stats = server + .mock("GET", "/api/v1/stats") + .with_status(200) + .with_header("content-type", "application/json") + .with_body(r#"{"repos":1,"agents":2,"pushes":3}"#) + .create_async() + .await; + let peers = server + .mock("GET", "/api/v1/peers") + .with_status(500) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"boom"}"#) + .expect(1) + .create_async() + .await; + let args = SyncArgs { + cmd: SyncCmd::Status, + node: server.url(), + dir: None, + }; + let err = run(args).await.unwrap_err(); + assert!( + err.to_string().contains("500"), + "expected 500 surfaced, got: {err}" + ); + peers.assert_async().await; + } } diff --git a/crates/gl/src/task.rs b/crates/gl/src/task.rs index c26cb35f..5b0c3cf4 100644 --- a/crates/gl/src/task.rs +++ b/crates/gl/src/task.rs @@ -166,13 +166,14 @@ async fn cmd_create( "delegator_did": delegator_did, }))?; - let resp: Value = client - .post("/api/v1/tasks", &body) - .await - .context("failed to create task")? - .json() - .await - .context("invalid JSON response")?; + let resp = crate::http::read_json( + client + .post("/api/v1/tasks", &body) + .await + .context("failed to create task")?, + "create task", + ) + .await?; print_json(&resp); Ok(()) } @@ -191,26 +192,25 @@ async fn cmd_list( if let Some(a) = &assignee_did { path.push_str(&format!("&assignee_did={}", urlencoding::encode(a))); } - let resp: Value = client - .get(&path) - .await - .context("failed to list tasks")? - .json() - .await - .context("invalid JSON response")?; + let resp = crate::http::read_json( + client.get(&path).await.context("failed to list tasks")?, + "list tasks", + ) + .await?; print_json(&resp); Ok(()) } async fn cmd_view(id: String, node: String) -> Result<()> { let client = NodeClient::new(&node, None); - let resp: Value = client - .get(&format!("/api/v1/tasks/{}", id)) - .await - .context("failed to get task")? - .json() - .await - .context("invalid JSON response")?; + let resp = crate::http::read_json( + client + .get(&format!("/api/v1/tasks/{}", id)) + .await + .context("failed to get task")?, + "view task", + ) + .await?; print_json(&resp); Ok(()) } @@ -221,13 +221,14 @@ async fn cmd_claim(id: String, node: String, dir: Option) -> Result<()> let client = NodeClient::new(&node, Some(keypair)); let body = serde_json::to_vec(&json!({ "assignee_did": assignee_did }))?; - let resp: Value = client - .post(&format!("/api/v1/tasks/{}/claim", id), &body) - .await - .context("failed to claim task")? - .json() - .await - .context("invalid JSON response")?; + let resp = crate::http::read_json( + client + .post(&format!("/api/v1/tasks/{}/claim", id), &body) + .await + .context("failed to claim task")?, + "claim task", + ) + .await?; print_json(&resp); Ok(()) } @@ -243,13 +244,14 @@ async fn cmd_complete( let client = NodeClient::new(&node, Some(keypair)); let body = serde_json::to_vec(&json!({ "result": result, "by_did": by_did }))?; - let resp: Value = client - .post(&format!("/api/v1/tasks/{}/complete", id), &body) - .await - .context("failed to complete task")? - .json() - .await - .context("invalid JSON response")?; + let resp = crate::http::read_json( + client + .post(&format!("/api/v1/tasks/{}/complete", id), &body) + .await + .context("failed to complete task")?, + "complete task", + ) + .await?; print_json(&resp); Ok(()) } @@ -265,13 +267,14 @@ async fn cmd_fail( let client = NodeClient::new(&node, Some(keypair)); let body = serde_json::to_vec(&json!({ "reason": reason, "by_did": by_did }))?; - let resp: Value = client - .post(&format!("/api/v1/tasks/{}/fail", id), &body) - .await - .context("failed to fail task")? - .json() - .await - .context("invalid JSON response")?; + let resp = crate::http::read_json( + client + .post(&format!("/api/v1/tasks/{}/fail", id), &body) + .await + .context("failed to fail task")?, + "fail task", + ) + .await?; print_json(&resp); Ok(()) } @@ -355,11 +358,12 @@ mod tests { .with_status(500) .with_header("content-type", "application/json") .with_body(r#"{"message":"internal error"}"#) + .expect(1) .create_async() .await; - // Should still succeed (prints JSON, doesn't check status code) - cmd_create( + // A node 5xx must surface as an Err, not be printed as if it were a task. + let err = cmd_create( "deploy".to_string(), "agent:task".to_string(), None, @@ -371,7 +375,9 @@ mod tests { Some(dir.path().to_path_buf()), ) .await - .unwrap(); + .unwrap_err(); + assert!(err.to_string().contains("500"), "got: {err}"); + _m.assert_async().await; } // ── list ───────────────────────────────────────────────────────── @@ -445,11 +451,16 @@ mod tests { .with_status(404) .with_header("content-type", "application/json") .with_body(r#"{"message":"not found"}"#) + .expect(1) .create_async() .await; - // cmd_view doesn't check status — it prints the JSON - cmd_view("nope".to_string(), server.url()).await.unwrap(); + // A 404 must surface as an Err, not be printed as if it were a task. + let err = cmd_view("nope".to_string(), server.url()) + .await + .unwrap_err(); + assert!(err.to_string().contains("404"), "got: {err}"); + _m.assert_async().await; } // ── claim ──────────────────────────────────────────────────────── @@ -601,4 +612,122 @@ mod tests { .await .unwrap(); } + + // ── denial surfaces (INV-8): a node 4xx/5xx must Err, not render as success ── + + #[tokio::test] + async fn test_list_tasks_surfaces_denial() { + let mut server = mockito::Server::new_async().await; + + let _m = server + .mock( + "GET", + mockito::Matcher::Regex(r"/api/v1/tasks\?".to_string()), + ) + .with_status(500) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"boom"}"#) + .expect(1) + .create_async() + .await; + + let err = cmd_list(None, None, 50, server.url()).await.unwrap_err(); + assert!(err.to_string().contains("500"), "got: {err}"); + _m.assert_async().await; + } + + #[tokio::test] + async fn test_claim_task_surfaces_denial() { + let mut server = mockito::Server::new_async().await; + let dir = tempfile::TempDir::new().unwrap(); + let kp = gitlawb_core::identity::Keypair::generate(); + std::fs::write( + dir.path().join("identity.pem"), + kp.to_pem().unwrap().as_bytes(), + ) + .unwrap(); + + let _m = server + .mock("POST", "/api/v1/tasks/task-x/claim") + .with_status(403) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"forbidden"}"#) + .expect(1) + .create_async() + .await; + + let err = cmd_claim( + "task-x".to_string(), + server.url(), + Some(dir.path().to_path_buf()), + ) + .await + .unwrap_err(); + assert!(err.to_string().contains("403"), "got: {err}"); + _m.assert_async().await; + } + + #[tokio::test] + async fn test_complete_task_surfaces_denial() { + let mut server = mockito::Server::new_async().await; + let dir = tempfile::TempDir::new().unwrap(); + let kp = gitlawb_core::identity::Keypair::generate(); + std::fs::write( + dir.path().join("identity.pem"), + kp.to_pem().unwrap().as_bytes(), + ) + .unwrap(); + + let _m = server + .mock("POST", "/api/v1/tasks/task-x/complete") + .with_status(404) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"not found"}"#) + .expect(1) + .create_async() + .await; + + let err = cmd_complete( + "task-x".to_string(), + None, + server.url(), + Some(dir.path().to_path_buf()), + ) + .await + .unwrap_err(); + assert!(err.to_string().contains("404"), "got: {err}"); + _m.assert_async().await; + } + + #[tokio::test] + async fn test_fail_task_surfaces_denial() { + let mut server = mockito::Server::new_async().await; + let dir = tempfile::TempDir::new().unwrap(); + let kp = gitlawb_core::identity::Keypair::generate(); + std::fs::write( + dir.path().join("identity.pem"), + kp.to_pem().unwrap().as_bytes(), + ) + .unwrap(); + + let _m = server + .mock("POST", "/api/v1/tasks/task-x/fail") + .with_status(409) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"already terminal"}"#) + .expect(1) + .create_async() + .await; + + let err = cmd_fail( + "task-x".to_string(), + None, + server.url(), + Some(dir.path().to_path_buf()), + ) + .await + .unwrap_err(); + assert!(err.to_string().contains("409"), "got: {err}"); + _m.assert_async().await; + } } diff --git a/crates/gl/src/visibility.rs b/crates/gl/src/visibility.rs index eef9ecb4..ed70f7e8 100644 --- a/crates/gl/src/visibility.rs +++ b/crates/gl/src/visibility.rs @@ -2,7 +2,6 @@ use anyhow::{Context, Result}; use clap::{Args, Subcommand}; -use serde_json::Value; use std::path::{Path, PathBuf}; use crate::http::NodeClient; @@ -87,12 +86,7 @@ async fn resolve_owner_repo( did.split(':').next_back().unwrap_or(&did).to_string() } else { let client = NodeClient::new(node, None); - let info: Value = client - .get("/") - .await? - .json() - .await - .context("failed to fetch node info")?; + let info = crate::http::read_json(client.get("/").await?, "node info").await?; let did = info["did"].as_str().context("node missing DID")?; did.split(':').next_back().unwrap_or(did).to_string() }; @@ -123,12 +117,7 @@ async fn cmd_set( .await .context("failed to connect to node")?; - let status = resp.status(); - let body: Value = resp.json().await.unwrap_or_default(); - if !status.is_success() { - let msg = body["message"].as_str().unwrap_or("unknown error"); - anyhow::bail!("visibility set failed ({status}): {msg}"); - } + let _ = crate::http::read_json(resp, "visibility set").await?; println!("✓ Visibility rule set on {owner}/{name}: {path_glob} (mode {mode})"); if path_glob != "/" { @@ -156,12 +145,7 @@ async fn cmd_remove( .await .context("failed to connect to node")?; - let status = resp.status(); - let body: Value = resp.json().await.unwrap_or_default(); - if !status.is_success() { - let msg = body["message"].as_str().unwrap_or("unknown error"); - anyhow::bail!("visibility remove failed ({status}): {msg}"); - } + let _ = crate::http::read_json(resp, "visibility remove").await?; println!("✓ Visibility rule removed from {owner}/{name}: {path_glob}"); Ok(()) @@ -179,12 +163,7 @@ async fn cmd_list(repo: String, node: String, dir: Option) -> Result<() .await .context("failed to connect to node")?; - let status = resp.status(); - let body: Value = resp.json().await.unwrap_or_default(); - if !status.is_success() { - let msg = body["message"].as_str().unwrap_or("unknown error"); - anyhow::bail!("visibility list failed ({status}): {msg}"); - } + let body = crate::http::read_json(resp, "visibility list").await?; let rules = body["rules"].as_array().cloned().unwrap_or_default(); if rules.is_empty() { @@ -307,4 +286,26 @@ mod tests { .await .unwrap(); } + + #[tokio::test] + async fn resolve_owner_repo_surfaces_denial() { + // A slash-free repo with an empty identity dir forces the GET / node-info + // fetch. A gated 404 there must Err (surfacing the status), proving the + // read_json conversion is load-bearing rather than silently ignored. + let mut server = mockito::Server::new_async().await; + let dir = tempfile::TempDir::new().unwrap(); // empty, no identity.pem, forces the GET / branch + let _m = server + .mock("GET", "/") + .with_status(404) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"denied"}"#) + .expect(1) + .create_async() + .await; + let err = resolve_owner_repo("noslash", &server.url(), Some(dir.path())) + .await + .unwrap_err(); + assert!(err.to_string().contains("404"), "got: {err}"); + _m.assert_async().await; + } } diff --git a/crates/gl/src/webhook.rs b/crates/gl/src/webhook.rs index a7311a84..e7876b7a 100644 --- a/crates/gl/src/webhook.rs +++ b/crates/gl/src/webhook.rs @@ -98,17 +98,17 @@ async fn cmd_create( "events": event_list, }))?; - let resp = client - .post(&format!("/api/v1/repos/{owner}/{name}/hooks"), &payload) - .await - .context("failed to connect to node")?; - let status = resp.status(); - let hook: Value = resp.json().await.context("invalid JSON")?; - - if !status.is_success() { - let msg = hook["message"].as_str().unwrap_or("unknown error"); - anyhow::bail!("create webhook failed ({status}): {msg}"); - } + // #186: read_json status-checks before parsing and bounds the error body, so + // a denial (or a hostile oversized error) fails here instead of being parsed + // whole. `what` preserves main's "create webhook failed (...)" message prefix. + let hook = crate::http::read_json( + client + .post(&format!("/api/v1/repos/{owner}/{name}/hooks"), &payload) + .await + .context("failed to connect to node")?, + "create webhook", + ) + .await?; let id = hook["id"].as_str().unwrap_or("?"); let hook_events = hook["events"] @@ -145,16 +145,15 @@ async fn cmd_list(repo: String, node: String, dir: Option) -> Result<() // get_signed (not get) attaches the RFC 9421 signature — plain get() never // signs, and the node owner-gates this route, so an unsigned GET 401s. - let resp = client - .get_signed(&format!("/api/v1/repos/{owner}/{name}/hooks")) - .await?; - let status = resp.status(); - let body: Value = resp.json().await.context("invalid JSON")?; - - if !status.is_success() { - let msg = body["message"].as_str().unwrap_or("unknown error"); - anyhow::bail!("list webhooks failed ({status}): {msg}"); - } + // #186: read_json status-checks + bounds the error body; `what` preserves + // main's "list webhooks failed (...)" message prefix. + let body = crate::http::read_json( + client + .get_signed(&format!("/api/v1/repos/{owner}/{name}/hooks")) + .await?, + "list webhooks", + ) + .await?; let hooks = body .get("webhooks") @@ -195,20 +194,20 @@ async fn cmd_delete(repo: String, id: String, node: String, dir: Option let client = NodeClient::new(&node, Some(keypair)); let payload = serde_json::to_vec(&serde_json::json!({}))?; - let resp = client - .delete( - &format!("/api/v1/repos/{owner}/{name}/hooks/{id}"), - &payload, - ) - .await - .context("failed to connect to node")?; - let status = resp.status(); - let result: Value = resp.json().await.context("invalid JSON")?; - - if !status.is_success() { - let msg = result["message"].as_str().unwrap_or("unknown error"); - anyhow::bail!("delete webhook failed ({status}): {msg}"); - } + // #186: read_json status-checks + bounds the error body; `what` preserves + // main's "delete webhook failed (...)" message prefix. The success body is + // unused, so discard it. + let _ = crate::http::read_json( + client + .delete( + &format!("/api/v1/repos/{owner}/{name}/hooks/{id}"), + &payload, + ) + .await + .context("failed to connect to node")?, + "delete webhook", + ) + .await?; println!("✓ Webhook {id} deleted"); Ok(()) diff --git a/crates/gl/tests/no_parse_before_status.rs b/crates/gl/tests/no_parse_before_status.rs new file mode 100644 index 00000000..7e53acd0 --- /dev/null +++ b/crates/gl/tests/no_parse_before_status.rs @@ -0,0 +1,257 @@ +//! INV-21 completeness gate for #186. +//! +//! The client handlers converted in #186 must read node responses through +//! `crate::http::read_json` — status-first, capped error read, sanitized message. +//! The bypass this fix removed is *parse-before-status*: a `resp.json().await` +//! whose result is only checked against the status AFTER parsing, which lets a +//! hostile node stream an unbounded JSON error and print its `message` unsanitized. +//! +//! This test scans the converted source files and fails if that idiom returns — +//! a `.json().await` call (not `read_json`) with an `.is_success()` check within a +//! few lines AFTER it. A status check BEFORE the parse (the `KEEP` probes in +//! repo.rs) reads as status-first and is not flagged. If a converted site is +//! reverted to the bypass, this goes RED. +//! +//! The gate does not hand-list the files to scan; it DERIVES them: every +//! `src/*.rs` that references `read_json` (except `http.rs`, where it is defined) +//! is a converted handler and is scanned for the bypass idiom. +//! +//! Derivation alone has a blind spot: it keys on the very `read_json` marker a +//! full revert deletes. A converted handler with a single node call (register.rs) +//! reverted to `resp.json().await` loses `read_json` entirely, drops out of the +//! derived set, and its bypass goes unscanned. So `CONVERTED_IN_186` is the +//! authoritative required set and the gate asserts the derived surface EQUALS it, +//! failing closed in both directions: +//! +//! - a pinned file missing from the derived set was reverted off `read_json` +//! (the blind-spot escape), so the gate goes RED; +//! - a file that uses `read_json` but is not pinned is a conversion that was +//! never enrolled, so its own later revert would escape unseen; RED until it +//! is added to the pin. +//! +//! Equality is what extends the protection to handlers converted after #186, not +//! just the original sixteen: a conversion cannot ship unpinned, and a pinned +//! surface cannot be deconverted silently. +//! +//! Pre-existing bypasses in files this PR did not convert (e.g. init.rs, +//! mirror.rs, profile.rs) are known debt, tracked separately and out of scope. +//! They do not use `read_json`, so the derivation excludes them and they are not +//! pinned. + +use std::path::Path; + +/// Files that define/host `read_json` rather than consume it as a converted +/// handler. Excluded from the scanned set even though they reference the symbol. +const NOT_A_HANDLER: &[&str] = &["http.rs"]; + +/// The authoritative converted-handler surface: every non-`http.rs` file whose +/// node-response reads route through `read_json`. The gate asserts the derived +/// `read_json` set EQUALS this exactly, so a new conversion must be added here +/// (the gate is RED until it is) and a pinned surface cannot be reverted off +/// `read_json` without tripping the gate. Both directions fail closed, which is +/// what protects post-#186 conversions, not only the original sixteen. +const CONVERTED_IN_186: &[&str] = &[ + "agent.rs", + "bounty.rs", + "cert.rs", + "changelog.rs", + "issue.rs", + "mcp.rs", + "peer.rs", + "pr.rs", + "protect.rs", + "register.rs", + "repo.rs", + "star.rs", + "sync.rs", + "task.rs", + "visibility.rs", + "webhook.rs", +]; + +/// Derive the converted-handler surface: every `src/*.rs` that references +/// `read_json`, minus the definition site(s). This is the set the equality check +/// compares against `CONVERTED_IN_186`; a newly-converted handler surfaces here +/// and must then be pinned (the gate is RED until it is). +fn scanned_handlers(src: &Path) -> Vec<(String, String)> { + let mut handlers = Vec::new(); + for entry in std::fs::read_dir(src).expect("read src dir") { + let path = entry.expect("dir entry").path(); + if path.extension().and_then(|e| e.to_str()) != Some("rs") { + continue; + } + let file_name = path + .file_name() + .and_then(|n| n.to_str()) + .expect("file name") + .to_string(); + if NOT_A_HANDLER.contains(&file_name.as_str()) { + continue; + } + let text = std::fs::read_to_string(&path) + .unwrap_or_else(|e| panic!("read {}: {e}", path.display())); + if has_read_json_call_site(&text) { + handlers.push((file_name, text)); + } + } + handlers.sort(); + handlers +} + +/// Does this file actually CALL `read_json`, as opposed to merely mentioning it? +/// +/// Membership in the derived set must key on a call site, never on the text of +/// the file. A textual `contains("read_json")` matched prose too, so a handler +/// fully reverted off `read_json` kept its membership on the strength of a +/// leftover comment ("previously routed through read_json"). Still-derived means +/// the pinned-vs-derived equality below never fired, and the reverted file went +/// on being scanned as though it were converted, so the deconversion — which +/// drops the cap and the sanitizer — shipped green. Requiring the trailing `(` +/// on a non-comment line also drops test names and doc references. +fn has_read_json_call_site(text: &str) -> bool { + text.lines().any(|l| { + let t = l.trim_start(); + !t.starts_with("//") && t.contains("read_json(") + }) +} + +/// How far above a parse site to look for its status check. +/// +/// The widest legitimate gap in the tree is `sync.rs`'s hand-rolled status-first +/// `Trigger` arm, whose `.status()` sits 19 lines above its parse; 24 leaves a +/// little headroom. Wider is weaker, so this should shrink if that arm is ever +/// routed through `read_json` like its siblings. +const STATUS_LOOKBACK: usize = 24; + +/// Does `line` open a JSON parse — `.json().await`, or a turbofish +/// `.json::().await`, or the head of a split-line chain (`.json(` / +/// `.json::(` whose `()` / `.await` continue on following lines)? +/// +/// Lines routed through `read_json` are never parse sites (that IS the fix). +fn opens_json_parse(line: &str) -> bool { + if line.contains("read_json") { + return false; + } + // A `.json(` token starts every reqwest JSON parse, bare or turbofished, + // single- or split-line. We anchor on it and let the window below confirm + // the `.await` / status check; anchoring on `.json(` alone is what catches + // `resp.json::().await` and `resp\n .json()\n .await` chains + // that the old bare-`.json().await` substring missed. + line.contains(".json()") || line.contains(".json::<") +} + +/// Within `window` (the parse-site line joined with the few lines after it), is +/// the parse actually completed with `.await` and then checked against the +/// status? A completed parse whose `.is_success()` lands AFTER it is the +/// parse-before-status bypass. A status check BEFORE the parse (KEEP probes) +/// never appears in this after-the-parse window, so it is not flagged. +fn window_is_bypass(window: &str) -> bool { + // The parse must actually resolve — guards against matching a `.json`-shaped + // token that never awaits. Split-line chains put `.await` a line or two down, + // which the joined window still contains. + let completes = window.contains(".await"); + completes && window.contains(".is_success()") +} + +/// Is there a status check ABOVE this parse site — i.e. is the parse guarded at +/// all? +/// +/// `window_is_bypass` only recognises a status check that lands AFTER the parse, +/// so it describes one specific bypass and says nothing about a parse with no +/// status check anywhere. That shape is the more dangerous revert (it renders a +/// denial body as a successful result with no check at all) and it went green: +/// with nothing matching `.is_success()` after it, there was nothing to flag. +/// Requiring positive evidence of a status check first turns the rule from +/// "detect one bad ordering" into "require the good ordering", which also covers +/// a check spelled `as_u16() >= 400` since that still reads `.status()`. +fn has_status_check_above(lookback: &str) -> bool { + lookback.contains(".status()") || lookback.contains(".is_success()") +} + +#[test] +fn converted_handlers_never_parse_before_status() { + let src = Path::new(env!("CARGO_MANIFEST_DIR")).join("src"); + let handlers = scanned_handlers(&src); + + // Deriving the set is the whole point of the fix; if the tree ever stops + // using `read_json` the gate would silently pass on an empty set. + assert!( + !handlers.is_empty(), + "derived no converted handlers from src/*.rs read_json usage — the gate \ + would vacuously pass; check the derivation" + ); + + // The pin is the authoritative required set: the derived `read_json` surface + // must EQUAL it, failing closed in both directions. A pinned file that drops + // out was reverted off `read_json` (register.rs, its single call, is the + // motivating case) and its bypass would escape the derived scan. A derived + // file that is not pinned is a conversion nobody enrolled, so ITS later revert + // would escape the same way; force it into the pin now. Equality is what + // extends the protection to handlers converted after #186. + let derived: Vec<&str> = handlers.iter().map(|(n, _)| n.as_str()).collect(); + let deconverted: Vec<&str> = CONVERTED_IN_186 + .iter() + .copied() + .filter(|f| !derived.contains(f)) + .collect(); + assert!( + deconverted.is_empty(), + "pinned handler(s) no longer route node responses through read_json: a \ + parse-before-status revert would otherwise escape the derived scan. \ + Re-route through crate::http::read_json (or drop from CONVERTED_IN_186 if \ + the surface was intentionally deconverted): {deconverted:?}" + ); + let unpinned: Vec<&str> = derived + .iter() + .copied() + .filter(|f| !CONVERTED_IN_186.contains(f)) + .collect(); + assert!( + unpinned.is_empty(), + "handler(s) use read_json but are absent from CONVERTED_IN_186: add them so \ + the gate protects them against a later parse-before-status revert (an \ + unpinned conversion drops out of both the scan and this check when \ + reverted): {unpinned:?}" + ); + + let mut offenders = Vec::new(); + let mut unguarded = Vec::new(); + for (name, text) in &handlers { + let lines: Vec<&str> = text.lines().collect(); + for (i, line) in lines.iter().enumerate() { + if !opens_json_parse(line) { + continue; + } + // Join the parse-site line with the following lines so a split-line + // chain's `.await` and an after-the-parse `.is_success()` are both in + // view. Status-first probes put `.is_success()` on an EARLIER line, so + // it is outside this window and stays green. + let window = lines[i..(i + 6).min(lines.len())].join("\n"); + if window_is_bypass(&window) { + offenders.push(format!("{name}:{}", i + 1)); + continue; + } + // A parse that never resolves is not a read; only a completed parse + // can render a denial body as a result. + if !window.contains(".await") { + continue; + } + let lookback = lines[i.saturating_sub(STATUS_LOOKBACK)..i].join("\n"); + if !has_status_check_above(&lookback) { + unguarded.push(format!("{name}:{}", i + 1)); + } + } + } + + assert!( + offenders.is_empty(), + "parse-before-status bypass present in converted handler(s) — route the read \ + through crate::http::read_json (status-first, capped, sanitized): {offenders:?}" + ); + assert!( + unguarded.is_empty(), + "node response parsed with NO status check above it in a converted handler — \ + the denial body is being rendered as a result. Route the read through \ + crate::http::read_json (status-first, capped, sanitized): {unguarded:?}" + ); +}