From e179cd22ea8443708d0231ac56097395004266f9 Mon Sep 17 00:00:00 2001 From: t Date: Tue, 21 Jul 2026 22:30:48 -0500 Subject: [PATCH 1/3] fix(gl): status-check the client read/write surfaces so a node denial surfaces as an error, not a fake result (#123) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Convert the gl CLI + MCP read/write surfaces to status-check the node's response before parsing, so a denial (401/403/404) or a degraded 5xx surfaces as an error instead of being rendered as an empty list or a fake success. The shared http::read_json helper checks the status first and bounds the error-body read (a hostile node can't stream an arbitrarily large error body to exhaust the client), replacing the prior parse-before-status pattern across agent, bounty, cert, changelog, issue, mcp, peer, pr, protect, repo, star, status, sync, task, visibility, and webhook. A no_parse_before_status regression gate locks the converted set in. Rebased onto current main (squash of the prior branch). main had independently gated several of these same surfaces, so the overlapping arms are reconciled to keep BOTH fixes rather than let either regress: - repo::cmd_label_list keeps main's get_maybe_signed (read-visibility gating: public repos stay anon-listable, a private-repo owner signs) carrying read_json on top; the PR's get_authed is dropped so the visibility gate holds. - mcp webhook_list keeps main's get_signed and its signing-keypair-DID owner resolution (NOT resolve_owner's node DID) — list_webhooks is owner-gated AND auth-required, so the PR's plain get()+resolve_owner would have 401'd then 403'd — with read_json layered for the bounded error read. - webhook create/list/delete keep main's get_signed + resolve_owner_repo_pair structure and its tests, with read_json swapped in for the manual resp.json() so the error body is bounded. main's superseded resolve_owner helper and its duplicate tests are dropped. gl: 388 unit tests + the no_parse_before_status gate green; fmt + clippy clean. --- crates/gl/src/agent.rs | 41 +- crates/gl/src/bounty.rs | 146 ++-- crates/gl/src/cert.rs | 334 +++++++- crates/gl/src/changelog.rs | 46 +- crates/gl/src/http.rs | 193 +++++ crates/gl/src/issue.rs | 121 +-- crates/gl/src/mcp.rs | 944 ++++++++++++++++++---- crates/gl/src/peer.rs | 351 +++++++- crates/gl/src/pr.rs | 246 ++++-- crates/gl/src/protect.rs | 57 +- crates/gl/src/register.rs | 16 +- crates/gl/src/repo.rs | 245 ++++-- crates/gl/src/star.rs | 78 +- crates/gl/src/status.rs | 177 +++- crates/gl/src/sync.rs | 88 +- crates/gl/src/task.rs | 223 +++-- crates/gl/src/visibility.rs | 51 +- crates/gl/src/webhook.rs | 69 +- crates/gl/tests/no_parse_before_status.rs | 200 +++++ 19 files changed, 2929 insertions(+), 697 deletions(-) create mode 100644 crates/gl/tests/no_parse_before_status.rs 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..a466e2a2 100644 --- a/crates/gl/src/cert.rs +++ b/crates/gl/src/cert.rs @@ -82,12 +82,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 +95,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 +129,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("?"); @@ -291,14 +277,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 +376,305 @@ 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; + } } 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..06788454 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,51 @@ 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. + let msg = serde_json::from_str::(&raw) + .ok() + .and_then(|b| b.get("message").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 +633,151 @@ 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() { + // A non-2xx JSON body that lacks a `message` key falls back to "request failed". + let mut server = Server::new_async().await; + let resp = response_for(&mut server, 403, r#"{"error":"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}"); + } + + /// #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..6b263e3a --- /dev/null +++ b/crates/gl/tests/no_parse_before_status.rs @@ -0,0 +1,200 @@ +//! 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 text.contains("read_json") { + handlers.push((file_name, text)); + } + } + handlers.sort(); + handlers +} + +/// 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()") +} + +#[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(); + 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)); + } + } + } + + 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:?}" + ); +} From 02797070a2f7727fc2365561d3d72ffe94f9c3a0 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Mon, 27 Jul 2026 19:52:11 -0500 Subject: [PATCH 2/3] fix(gl): report why the node-DID comparison was skipped in cert show The rebase onto main dropped did_check_report, since main's cmd_show had been rewritten to do real Ed25519 verification and collapsed every node-info failure into one "could not fetch current node info" NOTE. Put the tri-state reporting back on top of main's structure: the signature verdict and the --verify issuer anchoring are untouched, but the node-DID comparison now names the reason it could not run and points at the offline verification path, and "carried no DID" stays distinct from "the lookup failed". The lookup goes through read_json so a denial yields a reason rather than an opaque None, and it stays fail-soft: a node-info hiccup must not turn a successfully displayed certificate into an error exit. The DID extraction is split out as did_from_node_info so its guard is testable. An empty did rejects rather than flowing into the comparison, where it would print a mismatch WARNING against a DID the node never claimed. That guard had no coverage: removing it leaves every pre-existing test green and only the new one goes red. --- crates/gl/src/cert.rs | 155 ++++++++++++++++++++++++++++++++++++------ 1 file changed, 134 insertions(+), 21 deletions(-) diff --git a/crates/gl/src/cert.rs b/crates/gl/src/cert.rs index a466e2a2..34ea9011 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; @@ -177,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 { @@ -225,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`. @@ -677,4 +709,85 @@ mod tests { _cert.assert_async().await; _root.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}"); + } } From ad0e35ad42171db3bb54794d95c6b7dc9a073833 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Mon, 27 Jul 2026 20:17:20 -0500 Subject: [PATCH 3/3] fix(gl): close two completeness-gate escapes; surface the node's error key Review of the rebased branch found the #186 completeness gate could not fail on the two shapes that matter most. Both were reproduced by seeding the regression and watching the gate stay green. An unguarded parse escaped entirely. The rule flagged a parse followed by .is_success(), which describes one bad ordering and says nothing about a read with no status check at all -- the more dangerous revert, since it renders a denial body as a result. Inverted it to require a status check above the parse, which also covers a check spelled as_u16() >= 400. The lookback is 24 lines because sync.rs's hand-rolled Trigger arm puts its .status() 19 lines up; it should shrink if that arm is ever routed through read_json. Membership in the scanned set keyed on the text "read_json" rather than a call site, so a handler fully reverted off read_json kept its membership on a leftover comment. Still-derived meant the pinned-vs-derived equality never fired and the deconversion shipped green, dropping the cap and the sanitizer. Now counts call sites on non-comment lines. read_json read only `message`, but the task API answers with `error` alone ({"error":"task not found"} in gitlawb-node/src/api/tasks.rs), so every gl task denial rendered as the generic "request failed". Falls back to `error` through the same sanitizer and cap; `message` still wins when both are present, which is the shared AppError envelope. sync.rs already read both. Also covers cmd_show --verify, which had no test at all: the issuer anchoring is the security-bearing half of the command, since a valid signature only proves the certificate is self-consistent and a hostile node can self-sign one. The certificates carry real signatures, otherwise --verify would bail before reaching the anchoring and the assertions would be vacuous. Each fix verified by reverting it: both gate escapes go red, dropping the error fallback fails the two new read_json tests, and removing the anchoring block fails three of the five --verify tests. --- crates/gl/src/cert.rs | 172 ++++++++++++++++++++++ crates/gl/src/http.rs | 86 ++++++++++- crates/gl/tests/no_parse_before_status.rs | 59 +++++++- 3 files changed, 312 insertions(+), 5 deletions(-) diff --git a/crates/gl/src/cert.rs b/crates/gl/src/cert.rs index 34ea9011..8e3a8242 100644 --- a/crates/gl/src/cert.rs +++ b/crates/gl/src/cert.rs @@ -710,6 +710,178 @@ mod tests { _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 diff --git a/crates/gl/src/http.rs b/crates/gl/src/http.rs index 06788454..83984553 100644 --- a/crates/gl/src/http.rs +++ b/crates/gl/src/http.rs @@ -233,9 +233,21 @@ pub(crate) async fn read_json(resp: reqwest::Response, what: &str) -> Result(&raw) .ok() - .and_then(|b| b.get("message").and_then(|m| m.as_str()).map(str::to_owned)) + .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}): {}", @@ -736,13 +748,79 @@ mod tests { } #[tokio::test] - async fn read_json_errs_on_non_2xx_json_without_message() { - // A non-2xx JSON body that lacks a `message` key falls back to "request failed". + 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#"{"error":"forbidden"}"#, true).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 diff --git a/crates/gl/tests/no_parse_before_status.rs b/crates/gl/tests/no_parse_before_status.rs index 6b263e3a..7e53acd0 100644 --- a/crates/gl/tests/no_parse_before_status.rs +++ b/crates/gl/tests/no_parse_before_status.rs @@ -90,7 +90,7 @@ fn scanned_handlers(src: &Path) -> Vec<(String, String)> { } let text = std::fs::read_to_string(&path) .unwrap_or_else(|e| panic!("read {}: {e}", path.display())); - if text.contains("read_json") { + if has_read_json_call_site(&text) { handlers.push((file_name, text)); } } @@ -98,6 +98,31 @@ fn scanned_handlers(src: &Path) -> Vec<(String, String)> { 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)? @@ -128,6 +153,21 @@ fn window_is_bypass(window: &str) -> bool { 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"); @@ -175,6 +215,7 @@ fn converted_handlers_never_parse_before_status() { ); 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() { @@ -188,6 +229,16 @@ fn converted_handlers_never_parse_before_status() { 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)); } } } @@ -197,4 +248,10 @@ fn converted_handlers_never_parse_before_status() { "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:?}" + ); }