From 221160ae4cc452d5616d2f12aad4990d4b259789 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Mon, 10 Aug 2026 21:47:25 -0500 Subject: [PATCH 1/8] 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. Rebased again onto current main (2026-08-11), which had moved 238 commits. main independently landed read_body_capped and sanitize_node_msg into http.rs, so this branch's duplicate definitions in sync.rs are dropped and read_json is layered on main's helpers instead. main's peer.rs also grew its own announce handling (remote_announce_failure, local_add_report/local_add_refusal, capped read, sanitized message) which supersedes this branch's read_json conversion there; main's side is kept whole and only peer::cmd_list, which main had not gated, still takes read_json. 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. 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/agent.rs | 41 +- crates/gl/src/bounty.rs | 146 ++-- crates/gl/src/cert.rs | 661 +++++++++++++-- crates/gl/src/changelog.rs | 46 +- crates/gl/src/http.rs | 268 ++++++ crates/gl/src/issue.rs | 121 +-- crates/gl/src/mcp.rs | 944 ++++++++++++++++++---- crates/gl/src/peer.rs | 45 +- 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 | 84 +- 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 | 257 ++++++ 19 files changed, 3070 insertions(+), 705 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 d72c230a6..f7042f577 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 ccc658a15..7f5a0a5cb 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 87ad5aece..8e3a82427 100644 --- a/crates/gl/src/cert.rs +++ b/crates/gl/src/cert.rs @@ -4,7 +4,6 @@ use anyhow::{Context, Result}; use clap::{Args, Subcommand}; -use serde_json::Value; use std::path::PathBuf; use crate::http::NodeClient; @@ -82,12 +81,7 @@ async fn resolve_repo( did.split(':').next_back().unwrap_or(&did).to_string() } else { let client = signed_client(node, dir); - let info: Value = client - .get_authed("/") - .await? - .json() - .await - .context("failed to fetch node info")?; + let info = crate::http::read_json(client.get_authed("/").await?, "node info").await?; let did = info["did"].as_str().context("node info missing 'did'")?; did.split(':').next_back().unwrap_or(did).to_string() }; @@ -100,12 +94,7 @@ async fn cmd_list(repo: String, node: String, dir: Option) -> Result<() let client = signed_client(&node, dir.as_deref()); let path = format!("/api/v1/repos/{owner}/{name}/certs"); - let resp: Value = client - .get_authed(&path) - .await? - .json() - .await - .context("failed to list certificates")?; + let resp = crate::http::read_json(client.get_authed(&path).await?, "certificates").await?; let certs = resp["certificates"].as_array().cloned().unwrap_or_default(); @@ -139,14 +128,10 @@ async fn cmd_show( let client = signed_client(&node, dir.as_deref()); let id = resolve_cert_id(&client, &owner, &name, &id).await?; - // Fetch the certificate + // Fetch the certificate. read_json checks status first and surfaces the node's + // capped+sanitized message on a non-2xx (a bounded error read, not the whole body). let path = format!("/api/v1/repos/{owner}/{name}/certs/{id}"); - let resp = client - .get_authed(&path) - .await? - .error_for_status() - .context("certificate not found")?; - let cert: Value = resp.json().await.context("certificate not found")?; + let cert = crate::http::read_json(client.get_authed(&path).await?, "certificate").await?; let cert_id = cert["id"].as_str().unwrap_or("?"); let ref_name = cert["ref_name"].as_str().unwrap_or("?"); @@ -191,27 +176,20 @@ async fn cmd_show( // Contextual only — the verdict above stands on its own, so a node-info // hiccup here must not turn a successfully displayed certificate into an - // error exit. - let current_node_did = match client.get("/").await { - Ok(resp) => resp - .json::() - .await - .ok() - .and_then(|info| info["did"].as_str().map(str::to_string)), - Err(_) => None, + // error exit. Route the lookup through read_json so a denial or a capped + // error body yields a reportable reason instead of an opaque None, and keep + // "carried no DID" distinct from "the lookup failed": an empty DID must not + // reach the comparison and fabricate a mismatch warning. + let current: std::result::Result = match client.get("/").await { + Ok(resp) => match crate::http::read_json(resp, "node info").await { + Ok(info) => did_from_node_info(&info), + Err(e) => Err(e.to_string()), + }, + Err(e) => Err(e.to_string()), }; - match current_node_did.as_deref() { - Some(current) if current == node_did => { - println!(" Issuing node DID matches the node being queried."); - } - Some(current) => { - println!(" WARNING: Certificate node DID ({node_did}) does not match"); - println!(" current node DID ({current})."); - println!(" This certificate was issued by a different node."); - } - None => { - println!(" NOTE: could not fetch current node info — skipping node-DID comparison."); - } + let current_node_did = current.as_ref().ok().cloned(); + for line in did_check_report(¤t, node_did) { + println!("{line}"); } if require_valid { @@ -239,6 +217,46 @@ async fn cmd_show( Ok(()) } +/// Pull the node's DID out of a `GET /` body for the comparison in `cmd_show`. +/// +/// A missing OR empty `did` is a failure, not a value: letting `""` through +/// would reach the comparison and print a mismatch WARNING against a DID the +/// node never claimed, which reads as "issued by a different node" when the +/// truth is that the lookup told us nothing. +fn did_from_node_info(info: &serde_json::Value) -> std::result::Result { + match info["did"].as_str() { + Some(did) if !did.is_empty() => Ok(did.to_string()), + _ => Err("node info response carried no DID".to_string()), + } +} + +/// Select the report lines for the node-DID comparison in `cmd_show`. +/// +/// `current` is the current node's DID, or the reason it could not be +/// determined. A comparison verdict (match or WARNING) is only produced when a +/// real DID was obtained; otherwise the report degrades to a could-not-compare +/// hint naming the reason, plus the offline-verification guidance. The signature +/// verdict printed above stands on its own either way — this block only answers +/// *which* node issued the certificate. +fn did_check_report(current: &std::result::Result, node_did: &str) -> Vec { + match current { + Ok(current) if current == node_did => { + vec![" Issuing node DID matches the node being queried.".to_string()] + } + Ok(current) => vec![ + format!(" WARNING: Certificate node DID ({node_did}) does not match"), + format!(" current node DID ({current})."), + " This certificate was issued by a different node.".to_string(), + ], + Err(reason) => vec![ + format!(" Could not fetch the current node's DID ({reason}), so the comparison"), + " with the certificate's node DID is unavailable.".to_string(), + " To verify offline, use the node's Ed25519 public key derived from:".to_string(), + format!(" did:key → {node_did}"), + ], + } +} + /// Rebuild the node's canonical signing payload (field order must match /// gitlawb-node/src/cert.rs::issue_ref_certificate exactly) and verify the /// certificate's Ed25519 signature against the key embedded in `node_did`. @@ -291,14 +309,7 @@ async fn resolve_cert_id(client: &NodeClient, owner: &str, name: &str, id: &str) } let path = format!("/api/v1/repos/{owner}/{name}/certs?prefix={id}"); - let resp: Value = client - .get_authed(&path) - .await? - .error_for_status() - .context("failed to list certificates")? - .json() - .await - .context("failed to list certificates")?; + let resp = crate::http::read_json(client.get_authed(&path).await?, "certificates").await?; let certs = resp["certificates"].as_array().cloned().unwrap_or_default(); let matches: Vec = certs @@ -397,4 +408,558 @@ mod tests { ); assert!(garbage.is_err(), "malformed signature must not verify"); } + + #[tokio::test] + async fn cmd_list_surfaces_denial_not_empty() { + // A gated 404 on the repo-scoped certs read must Err, not print "No certificates". + let mut server = mockito::Server::new_async().await; + let _m = server + .mock( + "GET", + mockito::Matcher::Regex(r"^/api/v1/repos/alice/secret/certs$".to_string()), + ) + .with_status(404) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"repository 'alice/secret' not found"}"#) + .expect(1) + .create_async() + .await; + let result = cmd_list("alice/secret".to_string(), server.url(), None).await; + assert!(result.is_err(), "cert list must Err on a gated 404"); + // Prove the gated certs path was actually requested: without this, an + // unmatched route (mockito's 501, also non-2xx) would satisfy is_err(). + _m.assert_async().await; + } + + #[tokio::test] + async fn resolve_repo_surfaces_denial() { + // A slash-free repo with an empty identity dir forces the GET / node-info + // fetch. A gated 404 there must Err (surfacing the status), proving the + // read_json conversion is load-bearing rather than silently ignored. + let mut server = mockito::Server::new_async().await; + let dir = tempfile::TempDir::new().unwrap(); // empty, no identity.pem, forces the GET / branch + let _m = server + .mock("GET", "/") + .with_status(404) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"denied"}"#) + .expect(1) + .create_async() + .await; + let err = resolve_repo("noslash", &server.url(), Some(dir.path())) + .await + .unwrap_err(); + assert!(err.to_string().contains("404"), "got: {err}"); + _m.assert_async().await; + } + + // The `GET /` node-info lookup after the cert loads is a fail-soft diagnostic: + // a response-level failure degrades to a could-not-compare hint and the command + // completes Ok, never a fabricated mismatch warning and never a fatal Err. The + // cert fetch itself stays fail-closed. A >=36-char id skips resolve_cert_id so + // only two mocks are needed. + #[tokio::test] + async fn cmd_show_completes_with_degraded_hint_when_node_info_denied() { + let mut server = mockito::Server::new_async().await; + let long_id = "a".repeat(36); + let _cert = server + .mock( + "GET", + format!("/api/v1/repos/alice/secret/certs/{long_id}").as_str(), + ) + .with_status(200) + .with_header("content-type", "application/json") + .with_body( + r#"{"id":"c1","ref_name":"refs/heads/main","old_sha":"0","new_sha":"1","pusher_did":"p","node_did":"n","signature":"s","issued_at":"2026-01-01T00:00:00Z"}"#, + ) + .create_async() + .await; + let _root = server + .mock("GET", "/") + .with_status(403) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"denied"}"#) + .expect(1) + .create_async() + .await; + + let result = cmd_show( + "alice/secret".to_string(), + long_id, + server.url(), + None, + false, + None, + ) + .await; + assert!( + result.is_ok(), + "cert show must complete despite a denied node-info lookup: {result:?}" + ); + _cert.assert_async().await; + _root.assert_async().await; + } + + #[tokio::test] + async fn cmd_show_degrades_on_malformed_node_info() { + // A 2xx node-info body that fails to parse degrades the same way a denial + // does: hint printed, command completes Ok. + let mut server = mockito::Server::new_async().await; + let long_id = "a".repeat(36); + let _cert = server + .mock( + "GET", + format!("/api/v1/repos/alice/secret/certs/{long_id}").as_str(), + ) + .with_status(200) + .with_header("content-type", "application/json") + .with_body( + r#"{"id":"c1","ref_name":"refs/heads/main","old_sha":"0","new_sha":"1","pusher_did":"p","node_did":"n","signature":"s","issued_at":"2026-01-01T00:00:00Z"}"#, + ) + .create_async() + .await; + let _root = server + .mock("GET", "/") + .with_status(200) + .with_body("not json") + .expect(1) + .create_async() + .await; + + let result = cmd_show( + "alice/secret".to_string(), + long_id, + server.url(), + None, + false, + None, + ) + .await; + assert!( + result.is_ok(), + "cert show must complete despite malformed node info: {result:?}" + ); + _cert.assert_async().await; + _root.assert_async().await; + } + + #[tokio::test] + async fn cmd_show_degrades_when_node_info_lacks_did() { + // A 2xx node-info body with no `did` routes to the degraded hint, not a + // fabricated empty-DID mismatch warning; the command completes Ok. + let mut server = mockito::Server::new_async().await; + let long_id = "a".repeat(36); + let _cert = server + .mock( + "GET", + format!("/api/v1/repos/alice/secret/certs/{long_id}").as_str(), + ) + .with_status(200) + .with_header("content-type", "application/json") + .with_body( + r#"{"id":"c1","ref_name":"refs/heads/main","old_sha":"0","new_sha":"1","pusher_did":"p","node_did":"n","signature":"s","issued_at":"2026-01-01T00:00:00Z"}"#, + ) + .create_async() + .await; + let _root = server + .mock("GET", "/") + .with_status(200) + .with_header("content-type", "application/json") + .with_body("{}") + .expect(1) + .create_async() + .await; + + let result = cmd_show( + "alice/secret".to_string(), + long_id, + server.url(), + None, + false, + None, + ) + .await; + assert!( + result.is_ok(), + "cert show must complete when node info lacks a DID: {result:?}" + ); + _cert.assert_async().await; + _root.assert_async().await; + } + + // Must-not case: the certificate fetch itself stays fail-closed. A gated 404 + // aborts the command with the status surfaced, and the node-info lookup is + // never reached (the expect(0) assert proves it never ran). + #[tokio::test] + async fn cmd_show_surfaces_denied_certificate() { + let mut server = mockito::Server::new_async().await; + let long_id = "a".repeat(36); + let _cert = server + .mock( + "GET", + format!("/api/v1/repos/alice/secret/certs/{long_id}").as_str(), + ) + .with_status(404) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"repository not found"}"#) + .expect(1) + .create_async() + .await; + let _root = server + .mock("GET", "/") + .with_status(200) + .with_header("content-type", "application/json") + .with_body(r#"{"did":"n"}"#) + .expect(0) + .create_async() + .await; + + let err = cmd_show( + "alice/secret".to_string(), + long_id, + server.url(), + None, + false, + None, + ) + .await + .unwrap_err(); + assert!(err.to_string().contains("404"), "got: {err}"); + _cert.assert_async().await; + _root.assert_async().await; + } + + #[tokio::test] + async fn cmd_show_reports_matching_node_did() { + // Pins the unchanged success path: node info fetched, DIDs compared, Ok. + let mut server = mockito::Server::new_async().await; + let long_id = "a".repeat(36); + let _cert = server + .mock( + "GET", + format!("/api/v1/repos/alice/secret/certs/{long_id}").as_str(), + ) + .with_status(200) + .with_header("content-type", "application/json") + .with_body( + r#"{"id":"c1","ref_name":"refs/heads/main","old_sha":"0","new_sha":"1","pusher_did":"p","node_did":"n","signature":"s","issued_at":"2026-01-01T00:00:00Z"}"#, + ) + .create_async() + .await; + let _root = server + .mock("GET", "/") + .with_status(200) + .with_header("content-type", "application/json") + .with_body(r#"{"did":"n"}"#) + .expect(1) + .create_async() + .await; + + let result = cmd_show( + "alice/secret".to_string(), + long_id, + server.url(), + None, + false, + None, + ) + .await; + assert!(result.is_ok(), "got: {result:?}"); + _cert.assert_async().await; + _root.assert_async().await; + } + + #[tokio::test] + async fn cmd_show_warns_on_mismatching_node_did() { + // A real, differing node DID drives the WARNING branch end to end; the + // command still completes Ok. + let mut server = mockito::Server::new_async().await; + let long_id = "a".repeat(36); + let _cert = server + .mock( + "GET", + format!("/api/v1/repos/alice/secret/certs/{long_id}").as_str(), + ) + .with_status(200) + .with_header("content-type", "application/json") + .with_body( + r#"{"id":"c1","ref_name":"refs/heads/main","old_sha":"0","new_sha":"1","pusher_did":"p","node_did":"n","signature":"s","issued_at":"2026-01-01T00:00:00Z"}"#, + ) + .create_async() + .await; + let _root = server + .mock("GET", "/") + .with_status(200) + .with_header("content-type", "application/json") + .with_body(r#"{"did":"did:key:other"}"#) + .expect(1) + .create_async() + .await; + + let result = cmd_show( + "alice/secret".to_string(), + long_id, + server.url(), + None, + false, + None, + ) + .await; + assert!(result.is_ok(), "got: {result:?}"); + _cert.assert_async().await; + _root.assert_async().await; + } + + // ── --verify issuer anchoring ──────────────────────────────────────────── + // + // Every other cmd_show test passes require_valid=false, so the whole + // `if require_valid` block went unexecuted. It is the security-bearing half of + // the command: a valid signature only proves the certificate is internally + // consistent (a hostile node can mint a keypair, name it in node_did, and + // self-sign), so --verify must additionally anchor the issuer to a DID the + // caller trusts. These drive all four outcomes plus the must-not case. + // + // The certificate must carry a REAL signature over the canonical payload: + // with a bogus one, --verify bails on the signature and never reaches the + // anchoring, which would make every assertion below vacuous. + fn signed_cert(node_kp: &gitlawb_core::identity::Keypair) -> (String, String) { + let node_did = node_kp.did().as_str().to_string(); + let id = "a".repeat(36); // >= 36 chars skips resolve_cert_id's prefix lookup + let payload = serde_json::json!({ + "repo_id": "repo-1", + "ref": "refs/heads/main", + "old": "0".repeat(40), + "new": "b".repeat(40), + "pusher": "did:key:z6MkPusher", + "node": node_did, + "ts": "2026-07-22T00:00:00+00:00", + }); + let sig = node_kp.sign_b64(&serde_json::to_vec(&payload).unwrap()); + let body = serde_json::json!({ + "id": id, + "repo_id": "repo-1", + "ref_name": "refs/heads/main", + "old_sha": "0".repeat(40), + "new_sha": "b".repeat(40), + "pusher_did": "did:key:z6MkPusher", + "node_did": node_did, + "signature": sig, + "issued_at": "2026-07-22T00:00:00+00:00", + }) + .to_string(); + (id, body) + } + + /// Mount the cert fetch, plus a `GET /` answering with `node_info` (a JSON + /// body) or a denial when `None`. + async fn cert_server( + server: &mut mockito::Server, + id: &str, + cert_body: &str, + node_info: Option<&str>, + ) -> (mockito::Mock, mockito::Mock) { + let cert = server + .mock("GET", format!("/api/v1/repos/alice/r/certs/{id}").as_str()) + .with_status(200) + .with_header("content-type", "application/json") + .with_body(cert_body) + .expect(1) + .create_async() + .await; + let root = match node_info { + Some(b) => { + server + .mock("GET", "/") + .with_status(200) + .with_header("content-type", "application/json") + .with_body(b) + .create_async() + .await + } + None => { + server + .mock("GET", "/") + .with_status(403) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"denied"}"#) + .create_async() + .await + } + }; + (cert, root) + } + + #[tokio::test] + async fn verify_ok_when_issuing_node_is_the_queried_node() { + let kp = gitlawb_core::identity::Keypair::generate(); + let (id, body) = signed_cert(&kp); + let info = format!(r#"{{"did":"{}"}}"#, kp.did().as_str()); + let mut server = mockito::Server::new_async().await; + let (cert, _root) = cert_server(&mut server, &id, &body, Some(&info)).await; + + let got = cmd_show("alice/r".to_string(), id, server.url(), None, true, None).await; + assert!( + got.is_ok(), + "valid sig + matching issuer must pass: {got:?}" + ); + cert.assert_async().await; + } + + #[tokio::test] + async fn verify_bails_when_issuer_is_a_different_node() { + // The self-signing-hostile-node case: the signature verifies against the + // key the cert names, but that key is not the node we queried. + let kp = gitlawb_core::identity::Keypair::generate(); + let (id, body) = signed_cert(&kp); + let other = gitlawb_core::identity::Keypair::generate(); + let info = format!(r#"{{"did":"{}"}}"#, other.did().as_str()); + let mut server = mockito::Server::new_async().await; + let (cert, _root) = cert_server(&mut server, &id, &body, Some(&info)).await; + + let err = cmd_show("alice/r".to_string(), id, server.url(), None, true, None) + .await + .unwrap_err() + .to_string(); + assert!(err.contains("expected issuer"), "got: {err}"); + cert.assert_async().await; + } + + #[tokio::test] + async fn verify_bails_when_node_info_denied_and_no_expect_node() { + // Fail closed: with no trusted DID to anchor against, --verify must NOT + // fall through to a pass just because the signature checked out. + let kp = gitlawb_core::identity::Keypair::generate(); + let (id, body) = signed_cert(&kp); + let mut server = mockito::Server::new_async().await; + let (cert, _root) = cert_server(&mut server, &id, &body, None).await; + + let err = cmd_show("alice/r".to_string(), id, server.url(), None, true, None) + .await + .unwrap_err() + .to_string(); + assert!(err.contains("cannot anchor the issuer"), "got: {err}"); + cert.assert_async().await; + } + + #[tokio::test] + async fn verify_ok_when_expect_node_anchors_a_denied_lookup() { + // --expect-node supplies the trust anchor the denied lookup could not, so + // the same denial that fails the case above now passes. + let kp = gitlawb_core::identity::Keypair::generate(); + let (id, body) = signed_cert(&kp); + let mut server = mockito::Server::new_async().await; + let (cert, _root) = cert_server(&mut server, &id, &body, None).await; + + let got = cmd_show( + "alice/r".to_string(), + id, + server.url(), + None, + true, + Some(kp.did().as_str().to_string()), + ) + .await; + assert!(got.is_ok(), "explicit anchor must pass: {got:?}"); + cert.assert_async().await; + } + + #[tokio::test] + async fn verify_bails_on_a_bad_signature_before_anchoring() { + // The must-not: a forged certificate naming the queried node must fail on + // the signature, even though its issuer would otherwise anchor cleanly. + let kp = gitlawb_core::identity::Keypair::generate(); + let (id, body) = signed_cert(&kp); + let forged = body.replace(&"b".repeat(40), &"c".repeat(40)); + let info = format!(r#"{{"did":"{}"}}"#, kp.did().as_str()); + let mut server = mockito::Server::new_async().await; + let (cert, _root) = cert_server(&mut server, &id, &forged, Some(&info)).await; + + let err = cmd_show("alice/r".to_string(), id, server.url(), None, true, None) + .await + .unwrap_err() + .to_string(); + assert!(err.contains("signature did not verify"), "got: {err}"); + cert.assert_async().await; + } + + // The must-not case for the DID extraction: an empty or missing `did` has to + // become a could-not-compare reason. If it leaks through as a value, the + // comparison below fabricates a mismatch WARNING against a DID the node + // never claimed. + #[test] + fn did_from_node_info_rejects_empty_and_missing() { + assert!(did_from_node_info(&serde_json::json!({"did": "n"})).is_ok()); + for body in [ + serde_json::json!({"did": ""}), + serde_json::json!({}), + serde_json::json!({"did": 7}), + ] { + let got = did_from_node_info(&body); + assert!( + got.is_err(), + "must not yield a comparable DID: {body} -> {got:?}" + ); + } + // And the reason it produces must route to the degraded hint, never a verdict. + let report = + did_check_report(&did_from_node_info(&serde_json::json!({"did": ""})), "n").join("\n"); + assert!( + !report.contains("WARNING"), + "fabricated a mismatch: {report}" + ); + assert!(report.contains("Could not fetch"), "got: {report}"); + } + + // did_check_report is the three-way selector between the match text, the + // mismatch WARNING, and the degraded could-not-compare hint. Substring + // asserts (not full-line equality) so cosmetic wording edits don't break them. + + #[test] + fn did_check_report_match() { + let report = did_check_report(&Ok("n".to_string()), "n").join("\n"); + assert!( + report.contains("matches the node being queried"), + "got: {report}" + ); + assert!(!report.contains("WARNING"), "got: {report}"); + assert!(!report.contains("Could not fetch"), "got: {report}"); + } + + #[test] + fn did_check_report_mismatch() { + let report = did_check_report(&Ok("did:key:other".to_string()), "n").join("\n"); + assert!(report.contains("WARNING"), "got: {report}"); + assert!(report.contains("does not match"), "got: {report}"); + assert!(report.contains("did:key:other"), "got: {report}"); + assert!(!report.contains("Could not fetch"), "got: {report}"); + } + + #[test] + fn did_check_report_missing_did_reason() { + let report = + did_check_report(&Err("node info response carried no DID".to_string()), "n").join("\n"); + assert!(report.contains("Could not fetch"), "got: {report}"); + assert!( + report.contains("node info response carried no DID"), + "got: {report}" + ); + assert!(report.contains("verify offline"), "got: {report}"); + // The must-not case: no real DID was obtained, so no comparison verdict + // may be claimed in either direction. + assert!(!report.contains("WARNING"), "got: {report}"); + assert!( + !report.contains("matches the node being queried"), + "got: {report}" + ); + } + + #[test] + fn did_check_report_lookup_error_reason() { + let report = + did_check_report(&Err("node info failed (403): denied".to_string()), "n").join("\n"); + assert!(report.contains("Could not fetch"), "got: {report}"); + assert!(report.contains("403"), "got: {report}"); + assert!(report.contains("verify offline"), "got: {report}"); + assert!(!report.contains("WARNING"), "got: {report}"); + } } diff --git a/crates/gl/src/changelog.rs b/crates/gl/src/changelog.rs index 873689445..a65a9e6f0 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 4a51dc45d..fa1851e51 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). @@ -236,6 +237,60 @@ pub(crate) fn sanitize_node_msg(s: &str) -> String { .collect() } +/// 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 = read_body_capped(resp, ERROR_BODY_CAP).await; + // Preserve the prior contract: surface the JSON `message` when present in + // the capped body, else the status alone ("request failed") — a non-JSON + // or message-less body (503 degraded, 413 body-limit) never surfaces raw + // node bytes. A body whose `message` sits past the cap parses as truncated + // garbage here and correctly falls back rather than being buffered whole. + // + // Fall back to `error` before giving up: the node's shared AppError + // envelope carries both keys, but the task API answers with `error` alone + // (`{"error":"task not found"}` in gitlawb-node/src/api/tasks.rs), so + // reading only `message` blanked every `gl task` denial to the generic + // "request failed". `sync.rs`'s hand-rolled reader already reads both; this + // matches it. + let msg = serde_json::from_str::(&raw) + .ok() + .and_then(|b| { + b.get("message") + .or_else(|| b.get("error")) + .and_then(|m| m.as_str()) + .map(str::to_owned) + }) + .unwrap_or_else(|| "request failed".to_owned()); + anyhow::bail!("{what} failed ({status}): {}", 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::*; @@ -651,4 +706,217 @@ mod tests { let out = sanitize_node_msg("ok \u{0627}\u{200D}b"); assert_eq!(out, "ok \u{0627}\u{200D}b"); } + + // ── read_json (status-checked read; #123 / INV-8 / INV-6) ──────────── + + /// Drive a real `reqwest::Response` off a mockito mock so `read_json` sees an + /// actual HTTP status + body, the same shape the gated read arms produce. + async fn response_for( + server: &mut Server, + status: usize, + body: &str, + json: bool, + ) -> reqwest::Response { + let mut m = server.mock("GET", "/x").with_status(status).with_body(body); + if json { + m = m.with_header("content-type", "application/json"); + } + let _m = m.create_async().await; + NodeClient::new(server.url(), None).get("/x").await.unwrap() + } + + #[tokio::test] + async fn read_json_returns_body_on_2xx() { + let mut server = Server::new_async().await; + let resp = response_for( + &mut server, + 200, + r#"{"name":"r","owner_did":"did:gitlawb:z"}"#, + true, + ) + .await; + let v = read_json(resp, "repo").await.unwrap(); + assert_eq!(v["name"], "r"); + } + + #[tokio::test] + async fn read_json_errs_on_404_surfacing_message_and_status() { + let mut server = Server::new_async().await; + let resp = response_for( + &mut server, + 404, + r#"{"message":"repository 'o/r' not found"}"#, + true, + ) + .await; + let err = read_json(resp, "repo").await.unwrap_err().to_string(); + assert!(err.contains("404"), "err={err}"); + assert!(err.contains("not found"), "err={err}"); + } + + #[tokio::test] + async fn read_json_errs_on_500_surfacing_message() { + let mut server = Server::new_async().await; + let resp = response_for(&mut server, 500, r#"{"message":"internal boom"}"#, true).await; + let err = read_json(resp, "commits").await.unwrap_err().to_string(); + assert!(err.contains("500"), "err={err}"); + assert!(err.contains("internal boom"), "err={err}"); + } + + #[tokio::test] + async fn read_json_errs_on_non_json_error_body_with_fallback() { + // 503 with a plain-text (middleware) body: no `message` field to surface. + let mut server = Server::new_async().await; + let resp = response_for(&mut server, 503, "service unavailable", false).await; + let err = read_json(resp, "repo").await.unwrap_err().to_string(); + assert!(err.contains("503"), "err={err}"); + assert!(err.contains("request failed"), "err={err}"); + } + + #[tokio::test] + async fn read_json_sanitizes_control_and_bidi_in_message() { + // INV-6: a hostile node embeds ESC, BEL, and a right-to-left override in + // the error message; none may reach the terminal verbatim. + let mut server = Server::new_async().await; + let resp = response_for( + &mut server, + 404, + r#"{"message":"a\u001b[31mb\u0007c\u202ed"}"#, + true, + ) + .await; + let err = read_json(resp, "repo").await.unwrap_err().to_string(); + assert!(!err.contains('\u{1b}'), "ESC leaked: {err:?}"); + assert!(!err.contains('\u{7}'), "BEL leaked: {err:?}"); + assert!(!err.contains('\u{202e}'), "bidi override leaked: {err:?}"); + } + + #[tokio::test] + async fn read_json_errs_on_garbage_2xx_body() { + // The #123 correctness point: a 200 with a non-JSON/truncated body must be + // an `Err`, NOT `Ok(Null)` that a caller renders as an empty success. + let mut server = Server::new_async().await; + let resp = response_for(&mut server, 200, "this is not json", false).await; + assert!(read_json(resp, "repo").await.is_err()); + } + + #[tokio::test] + async fn read_json_errs_on_empty_2xx_body() { + // A zero-byte 200 body must be an `Err`, not a silent `Ok(Null)`. + let mut server = Server::new_async().await; + let resp = response_for(&mut server, 200, "", false).await; + assert!(read_json(resp, "repo").await.is_err()); + } + + #[tokio::test] + async fn read_json_errs_on_non_2xx_json_without_message_or_error() { + // A non-2xx JSON body carrying NEITHER key is the real fallback case. + let mut server = Server::new_async().await; + let resp = response_for(&mut server, 403, r#"{"detail":"forbidden"}"#, true).await; + let err = read_json(resp, "repo").await.unwrap_err().to_string(); + assert!(err.contains("403"), "err={err}"); + assert!(err.contains("request failed"), "err={err}"); + // The body's own text must not leak when no recognised key carries it. + assert!(!err.contains("forbidden"), "raw body leaked: {err}"); + } + + #[tokio::test] + async fn read_json_surfaces_error_key_when_message_absent() { + // The task API answers with `error` alone — see + // gitlawb-node/src/api/tasks.rs `{"error":"task not found"}`. Reading only + // `message` blanked those denials to the generic "request failed", so the + // user was told nothing about why the call failed. + let mut server = Server::new_async().await; + let resp = response_for(&mut server, 404, r#"{"error":"task not found"}"#, true).await; + let err = read_json(resp, "task show").await.unwrap_err().to_string(); + assert!(err.contains("404"), "err={err}"); + assert!(err.contains("task not found"), "reason not surfaced: {err}"); + } + + #[tokio::test] + async fn read_json_prefers_message_over_error_key() { + // The shared AppError envelope carries both; `message` is the human text + // and must win over the machine code in `error`. + let mut server = Server::new_async().await; + let resp = response_for( + &mut server, + 500, + r#"{"error":"db_error","message":"database unavailable"}"#, + true, + ) + .await; + let err = read_json(resp, "repo").await.unwrap_err().to_string(); + assert!(err.contains("database unavailable"), "err={err}"); + assert!( + !err.contains("db_error"), + "machine code won over message: {err}" + ); + } + + #[tokio::test] + async fn read_json_sanitizes_error_key_text() { + // The `error` fallback goes through the same sanitizer as `message`; a + // hostile node must not reach the terminal through the new path. The + // controls are JSON \u escapes so the body stays VALID JSON — embedding + // them raw would fail the parse and pass this test vacuously. + let mut server = Server::new_async().await; + let resp = response_for( + &mut server, + 404, + r#"{"error":"a\u001b[31mb\u0007c\u202ed"}"#, + true, + ) + .await; + let err = read_json(resp, "task show").await.unwrap_err().to_string(); + // Proves the parse succeeded and the `error` key was read, not the + // "request failed" fallback (which would make the asserts below vacuous). + assert!(err.contains("task show failed (404"), "err={err}"); + assert!( + !err.contains("request failed"), + "fell back, sanitizer untested: {err}" + ); + assert!( + err.contains('a') && err.contains('d'), + "text dropped: {err}" + ); + assert!(!err.contains('\u{1b}'), "ESC leaked: {err:?}"); + assert!(!err.contains('\u{7}'), "BEL leaked: {err:?}"); + assert!(!err.contains('\u{202e}'), "bidi override leaked: {err:?}"); + } + + /// #186 (F2): the non-2xx error path must read a CAPPED body, not buffer and + /// parse the whole thing — a hostile node can stream an arbitrarily large valid + /// JSON error. Place the `message` field AFTER more than the cap of padding: the + /// bounded read stops before it, so it must be ABSENT from the surfaced error. + /// RED before the fix (`resp.json()` parses the full body → the far message + /// appears), GREEN after (capped read truncates → fallback → far message gone). + /// The status is still surfaced, so a legitimate small error is unaffected. + #[tokio::test] + async fn read_json_bounds_the_error_body_read() { + let mut server = Server::new_async().await; + // >8 KiB of padding, then the marker message at the very end of the body. + let big = format!( + r#"{{"padding":"{}","message":"FARMARKER_BEYOND_CAP"}}"#, + "A".repeat(16 * 1024) + ); + let resp = response_for(&mut server, 500, &big, true).await; + let err = read_json(resp, "repo").await.unwrap_err().to_string(); + assert!(err.contains("500"), "bounded read still errors: {err}"); + assert!( + !err.contains("FARMARKER_BEYOND_CAP"), + "the error body read must be capped: a message beyond the cap must not be surfaced, err={err}" + ); + } + + /// #186 (F2): an EMPTY non-2xx body is its own input class — the capped read + /// yields "", the JSON parse fails, and it falls back to the status alone + /// ("request failed"), never a panic or a spurious success. + #[tokio::test] + async fn read_json_errs_on_empty_non_2xx_body() { + let mut server = Server::new_async().await; + let resp = response_for(&mut server, 502, "", false).await; + let err = read_json(resp, "repo").await.unwrap_err().to_string(); + assert!(err.contains("502"), "err={err}"); + assert!(err.contains("request failed"), "err={err}"); + } } diff --git a/crates/gl/src/issue.rs b/crates/gl/src/issue.rs index 57bd6944b..247497e22 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 ae319c73a..c406a4718 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 6b55b8829..60942df2b 100644 --- a/crates/gl/src/peer.rs +++ b/crates/gl/src/peer.rs @@ -63,12 +63,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); @@ -169,18 +164,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!({ @@ -256,12 +253,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); @@ -281,12 +273,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)"); diff --git a/crates/gl/src/pr.rs b/crates/gl/src/pr.rs index 8e0c4b72d..f10e68058 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 8a17a77ef..b3c200bab 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 c75a7667c..196a95159 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 120f824a8..62e2f2cab 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 cf477cc6b..ecc6d42e6 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 c3ff79726..a00bbc137 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)); @@ -158,6 +159,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()); @@ -166,6 +168,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] @@ -176,6 +180,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()); @@ -184,6 +189,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] @@ -200,6 +207,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()); @@ -211,6 +219,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] @@ -236,6 +246,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()); @@ -247,6 +258,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] @@ -277,4 +290,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 c26cb35f1..5b0c3cf41 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 eef9ecb4b..ed70f7e88 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 a7311a84b..e7876b7a0 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 000000000..7e53acd05 --- /dev/null +++ b/crates/gl/tests/no_parse_before_status.rs @@ -0,0 +1,257 @@ +//! INV-21 completeness gate for #186. +//! +//! The client handlers converted in #186 must read node responses through +//! `crate::http::read_json` — status-first, capped error read, sanitized message. +//! The bypass this fix removed is *parse-before-status*: a `resp.json().await` +//! whose result is only checked against the status AFTER parsing, which lets a +//! hostile node stream an unbounded JSON error and print its `message` unsanitized. +//! +//! This test scans the converted source files and fails if that idiom returns — +//! a `.json().await` call (not `read_json`) with an `.is_success()` check within a +//! few lines AFTER it. A status check BEFORE the parse (the `KEEP` probes in +//! repo.rs) reads as status-first and is not flagged. If a converted site is +//! reverted to the bypass, this goes RED. +//! +//! The gate does not hand-list the files to scan; it DERIVES them: every +//! `src/*.rs` that references `read_json` (except `http.rs`, where it is defined) +//! is a converted handler and is scanned for the bypass idiom. +//! +//! Derivation alone has a blind spot: it keys on the very `read_json` marker a +//! full revert deletes. A converted handler with a single node call (register.rs) +//! reverted to `resp.json().await` loses `read_json` entirely, drops out of the +//! derived set, and its bypass goes unscanned. So `CONVERTED_IN_186` is the +//! authoritative required set and the gate asserts the derived surface EQUALS it, +//! failing closed in both directions: +//! +//! - a pinned file missing from the derived set was reverted off `read_json` +//! (the blind-spot escape), so the gate goes RED; +//! - a file that uses `read_json` but is not pinned is a conversion that was +//! never enrolled, so its own later revert would escape unseen; RED until it +//! is added to the pin. +//! +//! Equality is what extends the protection to handlers converted after #186, not +//! just the original sixteen: a conversion cannot ship unpinned, and a pinned +//! surface cannot be deconverted silently. +//! +//! Pre-existing bypasses in files this PR did not convert (e.g. init.rs, +//! mirror.rs, profile.rs) are known debt, tracked separately and out of scope. +//! They do not use `read_json`, so the derivation excludes them and they are not +//! pinned. + +use std::path::Path; + +/// Files that define/host `read_json` rather than consume it as a converted +/// handler. Excluded from the scanned set even though they reference the symbol. +const NOT_A_HANDLER: &[&str] = &["http.rs"]; + +/// The authoritative converted-handler surface: every non-`http.rs` file whose +/// node-response reads route through `read_json`. The gate asserts the derived +/// `read_json` set EQUALS this exactly, so a new conversion must be added here +/// (the gate is RED until it is) and a pinned surface cannot be reverted off +/// `read_json` without tripping the gate. Both directions fail closed, which is +/// what protects post-#186 conversions, not only the original sixteen. +const CONVERTED_IN_186: &[&str] = &[ + "agent.rs", + "bounty.rs", + "cert.rs", + "changelog.rs", + "issue.rs", + "mcp.rs", + "peer.rs", + "pr.rs", + "protect.rs", + "register.rs", + "repo.rs", + "star.rs", + "sync.rs", + "task.rs", + "visibility.rs", + "webhook.rs", +]; + +/// Derive the converted-handler surface: every `src/*.rs` that references +/// `read_json`, minus the definition site(s). This is the set the equality check +/// compares against `CONVERTED_IN_186`; a newly-converted handler surfaces here +/// and must then be pinned (the gate is RED until it is). +fn scanned_handlers(src: &Path) -> Vec<(String, String)> { + let mut handlers = Vec::new(); + for entry in std::fs::read_dir(src).expect("read src dir") { + let path = entry.expect("dir entry").path(); + if path.extension().and_then(|e| e.to_str()) != Some("rs") { + continue; + } + let file_name = path + .file_name() + .and_then(|n| n.to_str()) + .expect("file name") + .to_string(); + if NOT_A_HANDLER.contains(&file_name.as_str()) { + continue; + } + let text = std::fs::read_to_string(&path) + .unwrap_or_else(|e| panic!("read {}: {e}", path.display())); + if has_read_json_call_site(&text) { + handlers.push((file_name, text)); + } + } + handlers.sort(); + handlers +} + +/// Does this file actually CALL `read_json`, as opposed to merely mentioning it? +/// +/// Membership in the derived set must key on a call site, never on the text of +/// the file. A textual `contains("read_json")` matched prose too, so a handler +/// fully reverted off `read_json` kept its membership on the strength of a +/// leftover comment ("previously routed through read_json"). Still-derived means +/// the pinned-vs-derived equality below never fired, and the reverted file went +/// on being scanned as though it were converted, so the deconversion — which +/// drops the cap and the sanitizer — shipped green. Requiring the trailing `(` +/// on a non-comment line also drops test names and doc references. +fn has_read_json_call_site(text: &str) -> bool { + text.lines().any(|l| { + let t = l.trim_start(); + !t.starts_with("//") && t.contains("read_json(") + }) +} + +/// How far above a parse site to look for its status check. +/// +/// The widest legitimate gap in the tree is `sync.rs`'s hand-rolled status-first +/// `Trigger` arm, whose `.status()` sits 19 lines above its parse; 24 leaves a +/// little headroom. Wider is weaker, so this should shrink if that arm is ever +/// routed through `read_json` like its siblings. +const STATUS_LOOKBACK: usize = 24; + +/// Does `line` open a JSON parse — `.json().await`, or a turbofish +/// `.json::().await`, or the head of a split-line chain (`.json(` / +/// `.json::(` whose `()` / `.await` continue on following lines)? +/// +/// Lines routed through `read_json` are never parse sites (that IS the fix). +fn opens_json_parse(line: &str) -> bool { + if line.contains("read_json") { + return false; + } + // A `.json(` token starts every reqwest JSON parse, bare or turbofished, + // single- or split-line. We anchor on it and let the window below confirm + // the `.await` / status check; anchoring on `.json(` alone is what catches + // `resp.json::().await` and `resp\n .json()\n .await` chains + // that the old bare-`.json().await` substring missed. + line.contains(".json()") || line.contains(".json::<") +} + +/// Within `window` (the parse-site line joined with the few lines after it), is +/// the parse actually completed with `.await` and then checked against the +/// status? A completed parse whose `.is_success()` lands AFTER it is the +/// parse-before-status bypass. A status check BEFORE the parse (KEEP probes) +/// never appears in this after-the-parse window, so it is not flagged. +fn window_is_bypass(window: &str) -> bool { + // The parse must actually resolve — guards against matching a `.json`-shaped + // token that never awaits. Split-line chains put `.await` a line or two down, + // which the joined window still contains. + let completes = window.contains(".await"); + completes && window.contains(".is_success()") +} + +/// Is there a status check ABOVE this parse site — i.e. is the parse guarded at +/// all? +/// +/// `window_is_bypass` only recognises a status check that lands AFTER the parse, +/// so it describes one specific bypass and says nothing about a parse with no +/// status check anywhere. That shape is the more dangerous revert (it renders a +/// denial body as a successful result with no check at all) and it went green: +/// with nothing matching `.is_success()` after it, there was nothing to flag. +/// Requiring positive evidence of a status check first turns the rule from +/// "detect one bad ordering" into "require the good ordering", which also covers +/// a check spelled `as_u16() >= 400` since that still reads `.status()`. +fn has_status_check_above(lookback: &str) -> bool { + lookback.contains(".status()") || lookback.contains(".is_success()") +} + +#[test] +fn converted_handlers_never_parse_before_status() { + let src = Path::new(env!("CARGO_MANIFEST_DIR")).join("src"); + let handlers = scanned_handlers(&src); + + // Deriving the set is the whole point of the fix; if the tree ever stops + // using `read_json` the gate would silently pass on an empty set. + assert!( + !handlers.is_empty(), + "derived no converted handlers from src/*.rs read_json usage — the gate \ + would vacuously pass; check the derivation" + ); + + // The pin is the authoritative required set: the derived `read_json` surface + // must EQUAL it, failing closed in both directions. A pinned file that drops + // out was reverted off `read_json` (register.rs, its single call, is the + // motivating case) and its bypass would escape the derived scan. A derived + // file that is not pinned is a conversion nobody enrolled, so ITS later revert + // would escape the same way; force it into the pin now. Equality is what + // extends the protection to handlers converted after #186. + let derived: Vec<&str> = handlers.iter().map(|(n, _)| n.as_str()).collect(); + let deconverted: Vec<&str> = CONVERTED_IN_186 + .iter() + .copied() + .filter(|f| !derived.contains(f)) + .collect(); + assert!( + deconverted.is_empty(), + "pinned handler(s) no longer route node responses through read_json: a \ + parse-before-status revert would otherwise escape the derived scan. \ + Re-route through crate::http::read_json (or drop from CONVERTED_IN_186 if \ + the surface was intentionally deconverted): {deconverted:?}" + ); + let unpinned: Vec<&str> = derived + .iter() + .copied() + .filter(|f| !CONVERTED_IN_186.contains(f)) + .collect(); + assert!( + unpinned.is_empty(), + "handler(s) use read_json but are absent from CONVERTED_IN_186: add them so \ + the gate protects them against a later parse-before-status revert (an \ + unpinned conversion drops out of both the scan and this check when \ + reverted): {unpinned:?}" + ); + + let mut offenders = Vec::new(); + let mut unguarded = Vec::new(); + for (name, text) in &handlers { + let lines: Vec<&str> = text.lines().collect(); + for (i, line) in lines.iter().enumerate() { + if !opens_json_parse(line) { + continue; + } + // Join the parse-site line with the following lines so a split-line + // chain's `.await` and an after-the-parse `.is_success()` are both in + // view. Status-first probes put `.is_success()` on an EARLIER line, so + // it is outside this window and stays green. + let window = lines[i..(i + 6).min(lines.len())].join("\n"); + if window_is_bypass(&window) { + offenders.push(format!("{name}:{}", i + 1)); + continue; + } + // A parse that never resolves is not a read; only a completed parse + // can render a denial body as a result. + if !window.contains(".await") { + continue; + } + let lookback = lines[i.saturating_sub(STATUS_LOOKBACK)..i].join("\n"); + if !has_status_check_above(&lookback) { + unguarded.push(format!("{name}:{}", i + 1)); + } + } + } + + assert!( + offenders.is_empty(), + "parse-before-status bypass present in converted handler(s) — route the read \ + through crate::http::read_json (status-first, capped, sanitized): {offenders:?}" + ); + assert!( + unguarded.is_empty(), + "node response parsed with NO status check above it in a converted handler — \ + the denial body is being rendered as a result. Route the read through \ + crate::http::read_json (status-first, capped, sanitized): {unguarded:?}" + ); +} From d48ae1ea43492f075eddc597fd62922d549a94d0 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Mon, 10 Aug 2026 21:54:59 -0500 Subject: [PATCH 2/8] test(gl): pin the stats mock hits and the unsigned MCP webhook_list denial Closes the two items left open on the last review round. The bounty-stats denial tests were already load-bearing: breaking either mock's route regex turns both red, because mockito answers an unmatched route with 501 and neither test's asserted status is 501. That proof is incidental to mockito's fallback status, though, so a reworded assertion would silently drop it. Assert the hit directly with expect(1) + assert_async. MCP webhook_list keeps get_maybe_signed rather than aligning with the CLI's get_signed. An agent driving the MCP tool may legitimately have no local identity, and issuing the request so the node's own denial reaches the caller is more useful than failing client-side before the node is asked. That is only defensible if the denial actually surfaces, so pin it: no identity plus an explicit owner must issue an UNSIGNED request and turn the node's 401 into an error carrying the status and the node's reason, not an empty webhook list. The new test points at an empty temp dir instead of passing None. None falls back to the ambient ~/.gitlawb/identity.pem, which exists on a dev box and not on CI, so the request would have been signed here and unsigned there; the first run of this test failed exactly that way before the dir was pinned. Verified by execution: swapping get_maybe_signed for get_signed reds the new test on "must name the status", and breaking either stats mock route reds its test. gl: 412 unit tests + the no_parse_before_status gate green; fmt and clippy clean. --- crates/gl/src/bounty.rs | 12 +++++++++-- crates/gl/src/mcp.rs | 46 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 2 deletions(-) diff --git a/crates/gl/src/bounty.rs b/crates/gl/src/bounty.rs index 7f5a0a5cb..9fd3bab98 100644 --- a/crates/gl/src/bounty.rs +++ b/crates/gl/src/bounty.rs @@ -524,11 +524,12 @@ mod tests { #[tokio::test] async fn cmd_stats_surfaces_denial_not_fake_result() { let mut server = mockito::Server::new_async().await; - let _m = server + 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"}"#) + .expect(1) .create_async() .await; @@ -536,6 +537,12 @@ mod tests { .await .expect_err("a 403 must be an error, not a zeroed fake result"); assert!(err.to_string().contains("403"), "err={err}"); + // Pin the mock hit explicitly. The status assertion above already fails + // when the route does not match (mockito answers an unmatched route with + // 501, so "403" is absent), but that proof is incidental to mockito's + // fallback status; assert the hit directly so it survives a reworded + // assertion. + m.assert_async().await; } /// #186 (F1): the same denial-as-error behavior across the non-2xx response @@ -555,13 +562,14 @@ mod tests { if json { m = m.with_header("content-type", "application/json"); } - let _m = m.create_async().await; + let m = m.expect(1).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}" ); + m.assert_async().await; } } diff --git a/crates/gl/src/mcp.rs b/crates/gl/src/mcp.rs index c406a4718..2f3b964e3 100644 --- a/crates/gl/src/mcp.rs +++ b/crates/gl/src/mcp.rs @@ -1940,6 +1940,52 @@ mod tests { assert!(result.contains("webhooks"), "got: {result}"); } + /// The MCP tool deliberately diverges from the CLI here: `gl webhook list` + /// requires a local identity and fails client-side without one, while the MCP + /// tool issues the request unsigned and lets the node answer. Pin that path, + /// since the divergence is only defensible if the denial actually reaches the + /// caller as an error rather than as an empty webhook list. + #[tokio::test] + async fn test_webhook_list_unsigned_without_identity_surfaces_the_node_denial() { + let mut server = mockito::Server::new_async().await; + + // No identity dir, and an explicit `owner`, so the tool cannot sign and + // does not fall back to the keypair for the owner segment. The node + // owner-gates /hooks and answers a headerless caller with 401. + let m = server + .mock("GET", mockito::Matcher::Regex(r"/hooks$".to_string())) + .match_header("signature", mockito::Matcher::Missing) + .with_status(401) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"signature required"}"#) + .expect(1) + .create_async() + .await; + + // Point at an EMPTY dir rather than passing `None`: `None` falls back to + // the ambient ~/.gitlawb/identity.pem, which exists on a dev box and not + // on CI, so the request would be signed here and unsigned there. + let empty = tempfile::TempDir::new().unwrap(); + let err = call_tool( + "webhook_list", + json!({"owner": "alice", "repo": "myrepo"}), + &server.url(), + Some(empty.path()), + ) + .await + .expect_err("a 401 must surface as an error, not an empty webhook list"); + + let err = err.to_string(); + assert!(err.contains("401"), "must name the status: {err}"); + assert!( + err.contains("signature required"), + "must carry the node's own reason: {err}" + ); + // The request must actually have been issued unsigned; failing + // client-side before contacting the node would leave this unmatched. + m.assert_async().await; + } + #[tokio::test] async fn test_webhook_list_default_owner_is_keypair_not_node_did() { // When `owner` is omitted, the owner segment must come from the signing From 21ba974c54f768b840f258900fe33494a3eddba1 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Mon, 10 Aug 2026 22:00:53 -0500 Subject: [PATCH 3/8] docs(gl): describe the client-contract rules in plain English The comments carried internal shorthand identifiers that resolve to nothing for a reader outside this repo. Replace each with the property it names: a denial must surface as an error rather than render as an empty or fabricated result, and an error body must be read under a cap and sanitized before it reaches the terminal. Comment-only; no behavior change. Verified after the edit because the completeness gate reads source text to decide which handlers it scans: reverting a converted handler to a bare .json().await still turns it red. gl: 412 unit tests green, fmt and clippy clean, cargo check --workspace --locked clean. --- crates/gl/src/bounty.rs | 2 +- crates/gl/src/http.rs | 12 ++++++------ crates/gl/src/mcp.rs | 4 ++-- crates/gl/src/pr.rs | 2 +- crates/gl/src/repo.rs | 8 ++++---- crates/gl/src/status.rs | 10 +++++----- crates/gl/src/sync.rs | 2 +- crates/gl/src/task.rs | 2 +- crates/gl/tests/no_parse_before_status.rs | 3 ++- 9 files changed, 23 insertions(+), 22 deletions(-) diff --git a/crates/gl/src/bounty.rs b/crates/gl/src/bounty.rs index 9fd3bab98..a059963a3 100644 --- a/crates/gl/src/bounty.rs +++ b/crates/gl/src/bounty.rs @@ -576,7 +576,7 @@ mod tests { #[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). + // a 404 must Err, not silently print nothing. let mut server = mockito::Server::new_async().await; let _m = server .mock( diff --git a/crates/gl/src/http.rs b/crates/gl/src/http.rs index fa1851e51..adf5dc9ab 100644 --- a/crates/gl/src/http.rs +++ b/crates/gl/src/http.rs @@ -206,7 +206,7 @@ async fn obtain_proof(cfg: IcaptchaCfg) -> Result { /// 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). +/// separately, but the read itself must not be unbounded either. pub(crate) async fn read_body_capped(mut resp: reqwest::Response, cap: usize) -> String { let mut buf: Vec = Vec::new(); while buf.len() < cap { @@ -227,7 +227,7 @@ pub(crate) async fn read_body_capped(mut resp: reqwest::Response, cap: usize) -> /// Strip terminal-dangerous characters from (and cap the length of) a /// node-supplied error string before surfacing it. The node a caller talks to /// could be hostile and embed escape sequences in its error body; those must not -/// reach the terminal verbatim (INV-6). We drop the C0/C1 control bytes (which +/// reach the terminal verbatim. 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). pub(crate) fn sanitize_node_msg(s: &str) -> String { @@ -239,13 +239,13 @@ pub(crate) fn sanitize_node_msg(s: &str) -> String { /// 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 +/// node's sanitized `message` 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). +/// the bare parse renders a gated 404/5xx body back as the resource. /// 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. @@ -707,7 +707,7 @@ mod tests { assert_eq!(out, "ok \u{0627}\u{200D}b"); } - // ── read_json (status-checked read; #123 / INV-8 / INV-6) ──────────── + // ── read_json (status-checked, bounded, sanitized read; #123) ──────── /// 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. @@ -775,7 +775,7 @@ mod tests { #[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 + // 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( diff --git a/crates/gl/src/mcp.rs b/crates/gl/src/mcp.rs index 2f3b964e3..e9849ce7e 100644 --- a/crates/gl/src/mcp.rs +++ b/crates/gl/src/mcp.rs @@ -2139,7 +2139,7 @@ mod tests { assert_eq!(count, 40, "expected 40 tools, got {count}"); } - // ── Gated read arms surface node denials, not fabricated results (#123 / INV-8) ── + // ── Gated read arms surface node denials, not fabricated results (#123) ── #[tokio::test] async fn repo_get_surfaces_denial_not_fabricated_repo() { @@ -2359,7 +2359,7 @@ mod tests { _m.assert_async().await; } - // ── INV-8 across the MCP twin: every tool that renders a node response must + // ── 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 diff --git a/crates/gl/src/pr.rs b/crates/gl/src/pr.rs index f10e68058..2d3e88ca9 100644 --- a/crates/gl/src/pr.rs +++ b/crates/gl/src/pr.rs @@ -853,7 +853,7 @@ mod tests { .unwrap(); } - // ── Gated PR reads surface node denials, not empty/stub renders (#123 / INV-8) ── + // ── Gated PR reads surface node denials, not empty/stub renders (#123) ── #[tokio::test] async fn cmd_list_surfaces_denial_not_empty() { diff --git a/crates/gl/src/repo.rs b/crates/gl/src/repo.rs index 196a95159..936effc88 100644 --- a/crates/gl/src/repo.rs +++ b/crates/gl/src/repo.rs @@ -344,7 +344,7 @@ async fn cmd_info(repo: String, node: String, dir: Option) -> Result<() anyhow::bail!("repository '{owner}/{name}' not found"); } // 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 + // any other non-2xx yields the node's capped + sanitized message, and // the error read is bounded rather than buffering the whole hostile body. let r = crate::http::read_json(resp, "repo info").await?; @@ -893,7 +893,7 @@ mod tests { #[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). + // array" fallback that hides the node's actual response. let dir = TempDir::new().unwrap(); write_identity(&dir); @@ -1423,7 +1423,7 @@ mod tests { _repo.assert_async().await; } - // ── Gated CLI reads surface node denials, not empty renders (#123 / INV-8) ── + // ── Gated CLI reads surface node denials, not empty renders (#123) ── #[tokio::test] async fn cmd_commits_surfaces_denial_not_empty() { @@ -1717,7 +1717,7 @@ mod vet_merge_deny_arms { } // An authenticated caller DENIED with a 403 must surface an error, never - // render a repo card (deny-as-success is the INV-8 client-contract break). + // render a repo card (deny-as-success is the client-contract break). #[tokio::test] async fn cmd_info_403_denied_surfaces_error_not_card() { let dir = TempDir::new().unwrap(); diff --git a/crates/gl/src/status.rs b/crates/gl/src/status.rs index ecc6d42e6..4e57c4854 100644 --- a/crates/gl/src/status.rs +++ b/crates/gl/src/status.rs @@ -133,7 +133,7 @@ pub async fn run(args: StatusArgs) -> Result<()> { } /// 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). +/// non-2xx: the denial must surface, never render as "no open ...". /// 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). @@ -149,7 +149,7 @@ fn section_unavailable_line(label: &str, resp: &Result) -> Op /// 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 +/// 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 { @@ -407,7 +407,7 @@ mod tests { assert_eq!(trust_bar(0.25), "█░░░"); } - // ── gl status section denial surfacing (#123 / INV-8, R5) ──────────── + // ── gl status section denial surfacing (#123, R5) ──────────────────── async fn get_response( server: &mut mockito::Server, @@ -426,7 +426,7 @@ mod tests { #[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). + // treated as "no open PRs". let mut server = mockito::Server::new_async().await; let resp = get_response(&mut server, 404).await; assert_eq!( @@ -452,7 +452,7 @@ mod tests { } // ── trust_line: a 404 means unregistered; any OTHER failure must surface as - // unavailable, never fabricate an unregistered verdict (INV-8). ────────── + // unavailable, never fabricate an unregistered verdict. ─────────────── async fn agents_resp( server: &mut mockito::Server, status: usize, diff --git a/crates/gl/src/sync.rs b/crates/gl/src/sync.rs index a00bbc137..da8d2c97f 100644 --- a/crates/gl/src/sync.rs +++ b/crates/gl/src/sync.rs @@ -45,7 +45,7 @@ pub async fn run(args: SyncArgs) -> Result<()> { let status = resp.status(); if !status.is_success() { // Bound the read: a hostile or broken node must not force an - // unbounded allocation just to surface a denial (INV-6, read half). + // unbounded allocation just to surface a denial. let raw = read_body_capped(resp, 8 * 1024).await; let msg = serde_json::from_str::(&raw) .ok() diff --git a/crates/gl/src/task.rs b/crates/gl/src/task.rs index 5b0c3cf41..57047009a 100644 --- a/crates/gl/src/task.rs +++ b/crates/gl/src/task.rs @@ -613,7 +613,7 @@ mod tests { .unwrap(); } - // ── denial surfaces (INV-8): a node 4xx/5xx must Err, not render as success ── + // ── denial surfaces: a node 4xx/5xx must Err, not render as success ── #[tokio::test] async fn test_list_tasks_surfaces_denial() { diff --git a/crates/gl/tests/no_parse_before_status.rs b/crates/gl/tests/no_parse_before_status.rs index 7e53acd05..60dd37159 100644 --- a/crates/gl/tests/no_parse_before_status.rs +++ b/crates/gl/tests/no_parse_before_status.rs @@ -1,4 +1,5 @@ -//! INV-21 completeness gate for #186. +//! Completeness gate for #186, itself proven load-bearing by reverting a +//! converted handler and watching this go red. //! //! The client handlers converted in #186 must read node responses through //! `crate::http::read_json` — status-first, capped error read, sanitized message. From fba90038d5a258285aa14c2b671f0e041182b3a1 Mon Sep 17 00:00:00 2001 From: Kevin Codex Date: Mon, 31 Aug 2026 14:12:55 +0800 Subject: [PATCH 4/8] test(gl): accept only a full non-success guard above a raw parse A 404-only equality upstream counted as status evidence, so reverting a read_json call behind agent.rs's special-404 hint to resp.json().await? kept the fence green while a 500's denial body rendered as a result. Require .is_success() or an as_u16() range comparison in the lookback, and pin the distinction with a mutation regression that performs that exact revert on the real agent.rs source. --- crates/gl/tests/no_parse_before_status.rs | 169 +++++++++++++++++----- 1 file changed, 133 insertions(+), 36 deletions(-) diff --git a/crates/gl/tests/no_parse_before_status.rs b/crates/gl/tests/no_parse_before_status.rs index 60dd37159..ca0fb2d64 100644 --- a/crates/gl/tests/no_parse_before_status.rs +++ b/crates/gl/tests/no_parse_before_status.rs @@ -154,19 +154,76 @@ 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? +/// Is there a FULL non-success guard ABOVE this parse site — one that exits for +/// every non-2xx status, not merely a bespoke branch for one code? /// /// `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()") +/// denial body as a successful result with no check at all). Requiring positive +/// evidence of a guard first turns the rule from "detect one bad ordering" into +/// "require the good ordering". +/// +/// The evidence must cover the whole non-success range. "Any `.status()` +/// nearby" is not enough: agent.rs legitimately keeps a 404-only hint +/// (`resp.status() == StatusCode::NOT_FOUND`) ahead of its `read_json` call, +/// and a revert that swaps that call for `resp.json().await?` would still see +/// the 404 check in the lookback — while a 500's JSON denial body renders as an +/// empty agent list again. A single-status equality (or a `matches!(…, 404 | +/// 501)` enumeration) guards its own arm and nothing else, so it does not +/// count. What counts: +/// +/// - `.is_success()` — the canonical spelling; sync.rs binds the status to a +/// variable first, so this token is matched on its own rather than as a +/// `.status()` suffix; +/// - a RANGE comparison over `as_u16()` (`>= 400` and friends), which reads the +/// whole class rather than enumerating codes. +/// +/// The `special_404_hint_is_not_a_guard` mutation test below keeps this +/// distinction honest against the real agent.rs source. +fn has_full_status_guard_above(lookback: &str) -> bool { + if lookback.contains(".is_success()") { + return true; + } + lookback.contains("as_u16() >=") + || lookback.contains("as_u16() >") + || lookback.contains("as_u16() <=") + || lookback.contains("as_u16() <") +} + +/// The scan shared by the gate and the mutation regression: every raw parse +/// site in `text` is classified as an after-the-parse bypass (`offenders`) or a +/// parse with no full status guard above it (`unguarded`). Factored out so the +/// mutation test exercises EXACTLY the logic the gate runs, not a paraphrase of +/// it. +fn scan_handler(name: &str, text: &str) -> (Vec, Vec) { + let mut offenders = Vec::new(); + let mut unguarded = Vec::new(); + let lines: Vec<&str> = text.lines().collect(); + for (i, line) in lines.iter().enumerate() { + if !opens_json_parse(line) { + continue; + } + // Join the parse-site line with the following lines so a split-line + // chain's `.await` and an after-the-parse `.is_success()` are both in + // view. Status-first probes put `.is_success()` on an EARLIER line, so + // it is outside this window and stays green. + let window = lines[i..(i + 6).min(lines.len())].join("\n"); + if window_is_bypass(&window) { + offenders.push(format!("{name}:{}", i + 1)); + continue; + } + // A parse that never resolves is not a read; only a completed parse + // can render a denial body as a result. + if !window.contains(".await") { + continue; + } + let lookback = lines[i.saturating_sub(STATUS_LOOKBACK)..i].join("\n"); + if !has_full_status_guard_above(&lookback) { + unguarded.push(format!("{name}:{}", i + 1)); + } + } + (offenders, unguarded) } #[test] @@ -218,30 +275,9 @@ 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() { - if !opens_json_parse(line) { - continue; - } - // Join the parse-site line with the following lines so a split-line - // chain's `.await` and an after-the-parse `.is_success()` are both in - // view. Status-first probes put `.is_success()` on an EARLIER line, so - // it is outside this window and stays green. - let window = lines[i..(i + 6).min(lines.len())].join("\n"); - if window_is_bypass(&window) { - offenders.push(format!("{name}:{}", i + 1)); - continue; - } - // A parse that never resolves is not a read; only a completed parse - // can render a denial body as a result. - if !window.contains(".await") { - continue; - } - let lookback = lines[i.saturating_sub(STATUS_LOOKBACK)..i].join("\n"); - if !has_status_check_above(&lookback) { - unguarded.push(format!("{name}:{}", i + 1)); - } - } + let (o, u) = scan_handler(name, text); + offenders.extend(o); + unguarded.extend(u); } assert!( @@ -251,8 +287,69 @@ fn converted_handlers_never_parse_before_status() { ); 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:?}" + "node response parsed with NO full status guard 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:?}" + ); +} + +/// Mutation regression for the special-404 escape: agent.rs (and the agent-show +/// / repo-info paths shaped like it) keeps a legitimate 404-only hint ahead of +/// its `read_json` call. Revert that call to a raw `resp.json().await?` and the +/// 404 equality is the only status evidence in the lookback — under the old +/// "any `.status()` nearby" rule the fence stayed green while a 500's denial +/// body rendered as an empty agent list. This test performs that exact revert +/// on the REAL agent.rs source and requires the scan to flag the mutant, so the +/// full-guard distinction in `has_full_status_guard_above` cannot regress to a +/// mere proximity check without going red here. +#[test] +fn special_404_hint_is_not_a_guard() { + let src = Path::new(env!("CARGO_MANIFEST_DIR")).join("src"); + let text = std::fs::read_to_string(src.join("agent.rs")).expect("read agent.rs"); + + // The mutation must stay real: it requires the idiom it reverts. If + // agent.rs drops the 404 hint or the read_json call, this needs re-basing + // on whichever handler carries the special-404 idiom then. + assert!( + text.contains("StatusCode::NOT_FOUND"), + "agent.rs no longer carries the special-404 hint this mutation reverts; \ + re-anchor the mutation on the handler that does" + ); + let lines: Vec<&str> = text.lines().collect(); + let not_found_line = lines + .iter() + .position(|l| l.contains("StatusCode::NOT_FOUND")) + .unwrap(); + let read_json_line = lines[not_found_line..] + .iter() + .position(|l| l.contains("read_json")) + .map(|off| not_found_line + off) + .expect( + "agent.rs has no read_json call after its 404 hint; re-anchor the \ + mutation on the handler that carries the idiom", + ); + + // The revert under test: the guarded helper call becomes a raw parse. + let mut mutant: Vec = lines.iter().map(|l| l.to_string()).collect(); + mutant[read_json_line] = " let body: serde_json::Value = resp.json().await?;".to_string(); + let mutant = mutant.join("\n"); + + let (_, unguarded) = scan_handler("agent.rs[mutant]", &mutant); + assert!( + unguarded + .iter() + .any(|hit| hit.starts_with("agent.rs[mutant]:")), + "the special-404 revert was not flagged: a 404-only equality upstream is \ + being accepted as a full status guard, which reopens the escape this \ + mutation pins (expected an unguarded hit, got {unguarded:?})" + ); + + // Positive control: the unmutated file must stay green, or the fence is + // flagging the legitimate hint rather than the revert. + let (offenders, unguarded) = scan_handler("agent.rs", &text); + assert!( + offenders.is_empty() && unguarded.is_empty(), + "unmutated agent.rs trips the fence; the guard classifier is wrong, not \ + the handler: offenders={offenders:?} unguarded={unguarded:?}" ); } From 8adcbc39aca7f15d99c2da94bad2868d7ea6dd6b Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Tue, 1 Sep 2026 09:17:55 -0500 Subject: [PATCH 5/8] test(gl): drop the traced result from the cert assertion diagnostics (#186) CodeQL reported seven high-severity "Cleartext logging of sensitive information" alerts at cert.rs:497, 540, 584, 667, 708, 803 and 862, and the aggregate check has been red on the branch since. All seven are #[cfg(test)] assertions that format a whole Result<()> through {result:?} or {got:?}. CodeQL traces a value associated with resolve_cert_id into the command error and treats the assertion's panic diagnostic as a logging sink. The alerts are test-only. The single mod tests opens at cert.rs:328 and runs to EOF, so all seven sit inside it, and the traced certificate id is already printed unconditionally in production at cert.rs:145 because it is a public identifier. That makes these repeated instances of one test-diagnostic pattern rather than seven distinct vulnerabilities. Each message now carries static context naming the scenario instead. The failing test name and source line still identify what broke, and every assertion predicate is unchanged, so nothing is weakened into vacuity. The production cleartext-logging query stays enabled: nothing is suppressed or reconfigured. One {got:?} at cert.rs:900 is deliberately left alone. CodeQL did not flag it, its value comes from did_from_node_info over hardcoded literal JSON with no resolve_cert_id taint, and the interpolation is load-bearing there because it names which of three loop cases failed. Verified: cargo test -p gl --bin gl cert, 20 passed. fmt clean. --- crates/gl/src/cert.rs | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/crates/gl/src/cert.rs b/crates/gl/src/cert.rs index 8e3a82427..43089391a 100644 --- a/crates/gl/src/cert.rs +++ b/crates/gl/src/cert.rs @@ -494,7 +494,7 @@ mod tests { .await; assert!( result.is_ok(), - "cert show must complete despite a denied node-info lookup: {result:?}" + "cert show must complete despite a denied node-info lookup" ); _cert.assert_async().await; _root.assert_async().await; @@ -537,7 +537,7 @@ mod tests { .await; assert!( result.is_ok(), - "cert show must complete despite malformed node info: {result:?}" + "cert show must complete despite malformed node info" ); _cert.assert_async().await; _root.assert_async().await; @@ -581,7 +581,7 @@ mod tests { .await; assert!( result.is_ok(), - "cert show must complete when node info lacks a DID: {result:?}" + "cert show must complete when node info lacks a DID" ); _cert.assert_async().await; _root.assert_async().await; @@ -664,7 +664,7 @@ mod tests { None, ) .await; - assert!(result.is_ok(), "got: {result:?}"); + assert!(result.is_ok(), "cert show must report a matching node DID"); _cert.assert_async().await; _root.assert_async().await; } @@ -705,7 +705,10 @@ mod tests { None, ) .await; - assert!(result.is_ok(), "got: {result:?}"); + assert!( + result.is_ok(), + "cert show must warn, not fail, on a mismatching node DID" + ); _cert.assert_async().await; _root.assert_async().await; } @@ -798,10 +801,7 @@ mod tests { 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:?}" - ); + assert!(got.is_ok(), "valid sig + matching issuer must pass"); cert.assert_async().await; } @@ -859,7 +859,7 @@ mod tests { Some(kp.did().as_str().to_string()), ) .await; - assert!(got.is_ok(), "explicit anchor must pass: {got:?}"); + assert!(got.is_ok(), "explicit anchor must pass"); cert.assert_async().await; } From 634d39ed3a7f79c3463eebd53d2d462125c598c9 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Tue, 1 Sep 2026 09:20:02 -0500 Subject: [PATCH 6/8] test(gl): restore the nine peer denial tests dropped in the rebase (#186) The 2026-08-11 rebase squashed the branch and lost nine tests. Counting `async fn cmd_add_` in peer.rs per head: 6 at 06863da0, 3791c40c, 9ed2947d and ad0e35ad, then 0 at 21ba974c and 0 at fba90038. The production fail-soft at peer.rs:173-180 survived; only the tests pinning it went. That matters because the rule this PR's own round 3 established is that a carve-out with no test that goes red when someone makes it fatal is an incomplete sweep, not a documented decision. The peer-add public-URL lookup has been in exactly that state for three weeks. Recovered from e179cd22, not 5eff6060: the earlier commit carries only four of the seven items, while e179cd22 has all of them plus three same-class denial tests for cmd_list, cmd_ping and cmd_resolve that were lost the same way. Neither commit is an ancestor of HEAD. Nothing needed adapting. cmd_add's signature and its node-info block are byte-identical between the two heads. The announce arm has since moved to remote_announce_failure, which still formats the status the denied-announce test asserts on, and the local-add arm has moved to local_add_report, which still returns Ok either way, so both local-add tests hold. The named import list gains cmd_list, cmd_ping and cmd_resolve. Proven load-bearing rather than assumed: rewriting the fail-soft match into a fatal `?` turns cmd_add_announces_fallback_url_when_node_info_denied and cmd_add_falls_back_on_malformed_node_info red, and leaves the pre-existing a_peer_failure_past_the_read_cap_still_reports_its_status green. That last point is the reason these are not redundant with what was already there: the surviving test drives a 503 whose body is past the read cap, so it pins the cap and ordering interaction, not the fallback. Verified: cargo test -p gl --bin gl peer, 25 passed. fmt clean. --- crates/gl/src/peer.rs | 282 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 280 insertions(+), 2 deletions(-) diff --git a/crates/gl/src/peer.rs b/crates/gl/src/peer.rs index d2ccffd67..bbf118fc4 100644 --- a/crates/gl/src/peer.rs +++ b/crates/gl/src/peer.rs @@ -296,8 +296,8 @@ async fn cmd_resolve(did: String, node: String) -> Result<()> { #[cfg(test)] mod tests { use super::{ - announced_peer_summary, cmd_add, local_add_refusal, local_add_report, - remote_announce_failure, + announced_peer_summary, cmd_add, cmd_list, cmd_ping, cmd_resolve, local_add_refusal, + local_add_report, remote_announce_failure, }; use reqwest::StatusCode; use serde_json::json; @@ -590,4 +590,282 @@ mod tests { "legitimate text mangled: {warning:?}" ); } + + #[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; + } } From 73a75f623d82f369efa86bd5d2085a98400c0a8e Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Tue, 1 Sep 2026 09:22:45 -0500 Subject: [PATCH 7/8] fix(gl): route the status repo sections through read_json and pin the file (#186) `gl status` was hardened in this PR with section_unavailable_line and trust_line, so a denied section prints "unavailable ({status})" instead of rendering as empty. But it was hardened by hand rather than through read_json, so status.rs carried no marker, never entered the completeness gate's derived set, and was never scanned. Deleting the helper call would have restored the denial-as-empty bug with the gate still green: the one file written to fix this bug was the one converted file the gate did not cover. The two repo-section parses now route through read_json and status.rs is pinned in CONVERTED_IN_186. Exactly two source lines change. section_unavailable_line takes the response by reference and reads only its status, so read_json sits underneath it with no restructuring, and the outer if-let shape still stops one denied section from aborting the others. All 35 status tests pass unchanged, including the eight denial tests and their exact rendered strings. trust_line stays hand-rolled on purpose. It needs a three-way split (2xx, exactly 404, other non-2xx) that read_json collapses into a single Err, and it already carries a literal status check the scan can see. A second pinned const naming hand-rolled guards was considered and rejected. It would enforce only that a symbol is spelled somewhere in the file: deleting the println while keeping the call would stay green. A structural anchor already exists, which is read_json, and the repo's own guard-design note is explicit that a name-keyed filter must never be the membership predicate when one does. Proven, not assumed. Pinning status.rs before converting it turned the gate red naming the file. Reverting even one of the two read_json calls afterwards turns it red again, on the unguarded check rather than the pin, because the issues parse has no full status guard in its lookback window. The one gap worth naming: deleting both section_unavailable_line call sites while keeping read_json still scans green, and the denied section would then print nothing rather than a false "no open pull requests". Probed it, and clippy catches that case as a dead function under -D warnings, so CI would fail. That backstop holds only while the helper has no other caller. Verified: gate 2 passed, cargo test -p gl --bin gl status 35 passed. fmt clean. --- crates/gl/src/status.rs | 4 ++-- crates/gl/tests/no_parse_before_status.rs | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/crates/gl/src/status.rs b/crates/gl/src/status.rs index 4e57c4854..a9586ad50 100644 --- a/crates/gl/src/status.rs +++ b/crates/gl/src/status.rs @@ -76,7 +76,7 @@ pub async fn run(args: StatusArgs) -> Result<()> { 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 { + if let Ok(body) = crate::http::read_json(r, "pull requests").await { let prs = body["pulls"].as_array().cloned().unwrap_or_default(); let open: Vec<_> = prs .iter() @@ -105,7 +105,7 @@ pub async fn run(args: StatusArgs) -> Result<()> { 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 { + if let Ok(body) = crate::http::read_json(r, "issues").await { let issues = body["issues"].as_array().cloned().unwrap_or_default(); let open: Vec<_> = issues .iter() diff --git a/crates/gl/tests/no_parse_before_status.rs b/crates/gl/tests/no_parse_before_status.rs index ca0fb2d64..09b374047 100644 --- a/crates/gl/tests/no_parse_before_status.rs +++ b/crates/gl/tests/no_parse_before_status.rs @@ -64,6 +64,7 @@ const CONVERTED_IN_186: &[&str] = &[ "register.rs", "repo.rs", "star.rs", + "status.rs", "sync.rs", "task.rs", "visibility.rs", From 31e78f60ed67b71171869190328dbc23160046b7 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Tue, 1 Sep 2026 09:25:07 -0500 Subject: [PATCH 8/8] fix(gl): let gl pr view finish rendering when a sub-fetch is denied (#186) Converting cmd_view's sub-fetches to read_json turned a tolerated 404 into a mid-render abort. The header prints first, then the reviews fetch is bare `?`, so a denial leaves a half-rendered PR, exit 1, and the comments section never runs. Before the conversion the error body parsed, the section was skipped and the rest of the command still rendered, so this is a regression this PR introduced rather than pre-existing behavior. Both sub-fetches now degrade: a denial prints "Reviews unavailable ({status})" or "Comments unavailable ({status})" and rendering continues. That is the shape this PR already invented one file over for gl status's denied sections, so the two commands now behave the same way. read_json stays in use, so pr.rs stays in the gate's derived set and the pinned list is untouched. The header fetch stays fatal. With no header there is nothing to render partially, and softening it would resurrect denial-as-success for the whole command. cmd_view_header_denial_stays_fatal pins that, and it passed before this change as well as after, which is the point: it is the must-not case. mcp.rs's pr_view arm has the same fetch coupling and is deliberately left alone. It emits a single JSON result, so there is no partial-output problem, a whole-call error is the correct fail-closed answer for a machine consumer, and inventing a partial-result shape is a design change nobody asked for. Test-first and observed: cmd_view_continues_when_reviews_are_denied and cmd_view_continues_when_comments_are_denied both failed before the change and pass after. Each asserts the comments mock was hit with expect(1), which is the load-bearing part: the mock can only be hit if rendering continued past the denied fetch. Verified: cargo test -p gl --bin gl, 473 passed. Gate 2 passed. fmt and clippy clean. --- crates/gl/src/pr.rs | 156 ++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 150 insertions(+), 6 deletions(-) diff --git a/crates/gl/src/pr.rs b/crates/gl/src/pr.rs index 2d3e88ca9..07a464c9b 100644 --- a/crates/gl/src/pr.rs +++ b/crates/gl/src/pr.rs @@ -316,7 +316,10 @@ async fn cmd_view(repo: String, number: u64, node: String, dir: Option) } // Show reviews - let reviews = crate::http::read_json( + // A denied sub-fetch degrades this section rather than aborting: the header + // above has already printed, so a bare `?` here leaves a half-rendered PR and + // skips the comments below it. Mirrors `gl status`'s unavailable-section line. + let reviews = match crate::http::read_json( client .get(&format!( "/api/v1/repos/{owner}/{repo}/pulls/{number}/reviews" @@ -324,8 +327,14 @@ async fn cmd_view(repo: String, number: u64, node: String, dir: Option) .await?, "reviews", ) - .await?; - let reviews = reviews["reviews"].as_array().cloned().unwrap_or_default(); + .await + { + Ok(v) => v["reviews"].as_array().cloned().unwrap_or_default(), + Err(e) => { + println!("\nReviews unavailable ({e})"); + Vec::new() + } + }; if !reviews.is_empty() { println!("\nReviews ({}):", reviews.len()); for r in &reviews { @@ -350,7 +359,7 @@ async fn cmd_view(repo: String, number: u64, node: String, dir: Option) } // Show comments - let comments = crate::http::read_json( + let comments = match crate::http::read_json( client .get(&format!( "/api/v1/repos/{owner}/{repo}/pulls/{number}/comments" @@ -358,8 +367,14 @@ async fn cmd_view(repo: String, number: u64, node: String, dir: Option) .await?, "comments", ) - .await?; - let comments = comments["comments"].as_array().cloned().unwrap_or_default(); + .await + { + Ok(v) => v["comments"].as_array().cloned().unwrap_or_default(), + Err(e) => { + println!("\nComments unavailable ({e})"); + Vec::new() + } + }; if !comments.is_empty() { println!("\nComments ({}):", comments.len()); for c in &comments { @@ -969,4 +984,133 @@ mod tests { // 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; } + + /// A denied sub-fetch must not abort `gl pr view` after the header has + /// already printed. The header is fatal; reviews and comments degrade. + #[tokio::test] + async fn cmd_view_continues_when_reviews_are_denied() { + let dir = TempDir::new().unwrap(); + write_identity(&dir); + let mut server = mockito::Server::new_async().await; + let _pr = server + .mock("GET", mockito::Matcher::Regex(r"/pulls/1$".to_string())) + .with_status(200) + .with_header("content-type", "application/json") + .with_body(r#"{"number":1,"title":"T","status":"open","source_branch":"a","target_branch":"main","author_did":"did:key:z6MkTest"}"#) + .expect(1) + .create_async() + .await; + let _reviews = server + .mock( + "GET", + mockito::Matcher::Regex(r"/pulls/1/reviews$".to_string()), + ) + .with_status(403) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"forbidden"}"#) + .expect(1) + .create_async() + .await; + // The load-bearing assertion: this mock is only hit if rendering + // continued past the denied reviews fetch. + let _comments = server + .mock( + "GET", + mockito::Matcher::Regex(r"/pulls/1/comments$".to_string()), + ) + .with_status(200) + .with_header("content-type", "application/json") + .with_body(r#"{"comments":[]}"#) + .expect(1) + .create_async() + .await; + let r = cmd_view( + "myrepo".to_string(), + 1, + server.url(), + Some(dir.path().to_path_buf()), + ) + .await; + assert!( + r.is_ok(), + "cmd_view must finish rendering when reviews are denied" + ); + _comments.assert_async().await; + } + + #[tokio::test] + async fn cmd_view_continues_when_comments_are_denied() { + let dir = TempDir::new().unwrap(); + write_identity(&dir); + let mut server = mockito::Server::new_async().await; + let _pr = server + .mock("GET", mockito::Matcher::Regex(r"/pulls/1$".to_string())) + .with_status(200) + .with_header("content-type", "application/json") + .with_body(r#"{"number":1,"title":"T","status":"open","source_branch":"a","target_branch":"main","author_did":"did:key:z6MkTest"}"#) + .expect(1) + .create_async() + .await; + let _reviews = server + .mock( + "GET", + mockito::Matcher::Regex(r"/pulls/1/reviews$".to_string()), + ) + .with_status(200) + .with_header("content-type", "application/json") + .with_body(r#"{"reviews":[]}"#) + .expect(1) + .create_async() + .await; + let _comments = server + .mock( + "GET", + mockito::Matcher::Regex(r"/pulls/1/comments$".to_string()), + ) + .with_status(403) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"forbidden"}"#) + .expect(1) + .create_async() + .await; + let r = cmd_view( + "myrepo".to_string(), + 1, + server.url(), + Some(dir.path().to_path_buf()), + ) + .await; + assert!( + r.is_ok(), + "cmd_view must finish rendering when comments are denied" + ); + _comments.assert_async().await; + } + + /// The must-not case: with no header there is nothing to render partially, + /// so a denied header stays fatal. Softening it would resurrect + /// denial-as-success for the whole command. + #[tokio::test] + async fn cmd_view_header_denial_stays_fatal() { + let dir = TempDir::new().unwrap(); + write_identity(&dir); + let mut server = mockito::Server::new_async().await; + let _pr = server + .mock("GET", mockito::Matcher::Regex(r"/pulls/1$".to_string())) + .with_status(404) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"not found"}"#) + .expect(1) + .create_async() + .await; + let r = cmd_view( + "myrepo".to_string(), + 1, + server.url(), + Some(dir.path().to_path_buf()), + ) + .await; + assert!(r.is_err(), "a denied PR header must stay fatal"); + _pr.assert_async().await; + } }