Compare commits

..
Author SHA1 Message Date
beardthelion 361805b1bd chore: sync Cargo.lock with manifests (0.5.1 + icaptcha-client sha2 dep)
The committed lockfile lagged the manifests: workspace crates were bumped to
0.5.1 and icaptcha-client's Cargo.toml declares sha2, but the lock still
carried 0.5.0 and no sha2 entry. Every build regenerated it, leaving
Cargo.lock perpetually dirty. Regenerated offline (no dependency upgrades) so
the tree stays clean.
2026-08-28 23:16:43 -05:00
beardthelion 526ee87344 fix(node): make the SDL mutation-fence wiring load-bearing; admit deprecated
An adversarial review of the SDL switch found three gaps:

- The fence's completeness source (SDL) and its hidden-mutation cross-check were
  not load-bearing: because the real MutationRoot has no visible=false field,
  reverting `fields` back to introspection and dropping the cross-check left the
  whole suite green (INV-21). Extract the sourcing + cross-check into a generic
  `assert_mutation_fence<Q,M,S>` that the real fence AND a synthetic hidden schema
  both drive. A new #[should_panic] test (fence_rejects_a_hidden_mutation) feeds a
  visible=false mutation that IS registered, so only the cross-check can fire;
  reverting the SDL source or deleting the cross-check now changes/removes that
  panic and turns the test RED (verified).

- The introspection cross-check used bare `fields { name }` (GraphQL default
  includeDeprecated:false), so a `#[graphql(deprecation = "...")]` mutation --
  executable and present in SDL -- would false-red as a hidden visible=false
  mutation. Use `fields(includeDeprecated: true)`; a new test
  (fence_admits_a_deprecated_mutation) pins it, going RED without the flag.

- schema_lists_exactly_the_registered_mutations still sourced from introspection
  and its comment called introspection "the source of truth", contradicting the
  fix. Retargeted at the SDL set with a corrected comment.

Also folds the two introspection navigations into one generic helper
(introspection_mutation_fields) and points the version-safety test at it, so
there is a single introspection query. cargo test -p gitlawb-node --bins green
(497); clippy and fmt clean. Only api/mod.rs changed.
2026-08-28 23:16:38 -05:00
beardthelion 70b01aa320 fix(node): source the GraphQL mutation fence from SDL, not introspection
Resolves jatmn's #219 P1. The every_graphql_mutation_has_its_gate completeness
fence enumerated mutation fields via async-graphql introspection, which filters
out `#[graphql(visible = false)]` fields. Those fields stay executable, so a
future hidden mutation would need no registry row and no marker while both
set-equality checks stayed green -- defeating the fence's guarantee that no live
mutation goes unclassified. (Confirmed by execution: a synthetic visible=false
mutation is absent from introspection, executable, and present in schema.sdl().)

Source completeness from the engine's own SDL instead. A new pure helper
mutation_field_names_from_sdl parses schema.sdl() with async-graphql's parser
AST (async_graphql::parser::parse_schema) and reads the mutation type's fields;
SDL includes visible=false fields and the AST parse is immune to a brace in a
field description. Introspection is kept as a cross-check: any field present in
SDL but hidden from introspection now fails named as a forbidden hidden
mutation. The stale doc comments (which claimed introspection could not hide a
field) are corrected.

A self-checking version-safety test builds a synthetic schema with a
visible=false mutation and asserts the SDL-sourced set includes it while
introspection omits it -- so if a future async-graphql ever filters SDL by
visibility, that test goes RED rather than the gap reopening silently.

Load-bearing verified by execution: stubbing the SDL enumeration to empty turns
four tests RED, including the fence itself; restoring returns green. Only
api/mod.rs changed; the mutation resolvers are untouched. `cargo test -p
gitlawb-node --bins` green (495); clippy and fmt clean.
2026-08-28 23:16:38 -05:00
beardthelion a31fed4a67 docs(node): reconcile GraphQL mutation-auth claims with the shipped state
README's known limitations, the OSS-readiness live-network blockers, and the
maintainer roadmap still listed GraphQL mutation auth as an open item, which
contradicts #87: GraphQL mutations already require a verified signer bound to the
acting DID. Reconcile each to the shipped reality while keeping the genuinely-open
nuance: the GraphQL mutation surface is the agent-task queue, repo writes stay
REST-only, and no owner-gated repo-write mutation is exposed over GraphQL. The
every_graphql_mutation_has_its_gate fence forces any new mutation to be classified
in its table; owner-gate and REST parity stay the INV-1 review check.
2026-08-28 23:16:36 -05:00
beardthelion 626c545553 test(node): add a completeness fence over the GraphQL mutation surface
Gate the GraphQL mutation surface the way REST is gated, as a SEPARATE guard with
its own bucket table (a guard over a heterogeneous set must classify per member,
INV-1). every_graphql_mutation_has_its_gate asserts the mutation set is complete
(every mutation classified) and that each carries its gate marker.

Completeness is derived from async-graphql's own introspection: the field names
come from the engine as structured data, so no source-text or SDL-format shape
(a brace or async fn in a comment or string, a whitespace/generic/skip variant, a
brace in a rustdoc description) can hide a live mutation. A new unregistered
mutation, including a repo-write, fails the build.

The marker check (does each resolver body call its gate) is a best-effort static
backstop, bounded to each resolver's own comment-masked, string-aware body. It is
explicitly not the completeness guarantee and not load-bearingness: a marker inside
a string literal is a documented vacuity residual, and the adversarial sqlx tests in
graphql/mutation.rs are the real proof the gates reject.

Verified by execution: an unregistered mutation reddens (unclassified), a removed
did_matches gate reddens (missing marker), the clean tree is green; no DB, no
feature flag.
2026-08-28 23:16:03 -05:00
24 changed files with 1276 additions and 3749 deletions

No files matched your search

Generated
+12 -93
View File
@@ -770,23 +770,6 @@ dependencies = [
"zeroize",
]
[[package]]
name = "ark-ff"
version = "0.6.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "f7a806ac6c8307b929df4645776290a50ee2aac754ad09d8bdf73391309e43af"
dependencies = [
"ark-ff-asm 0.6.0",
"ark-ff-macros 0.6.0",
"ark-serialize 0.6.0",
"ark-std 0.6.0",
"digest 0.10.7",
"educe",
"num-bigint",
"num-traits",
"zeroize",
]
[[package]]
name = "ark-ff-asm"
version = "0.3.0"
@@ -817,16 +800,6 @@ dependencies = [
"syn 2.0.117",
]
[[package]]
name = "ark-ff-asm"
version = "0.6.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "1479009684adc073dff49a1025d3a7065b317a9ead25aaaca38cdc70058ba8a2"
dependencies = [
"quote",
"syn 2.0.117",
]
[[package]]
name = "ark-ff-macros"
version = "0.3.0"
@@ -865,19 +838,6 @@ dependencies = [
"syn 2.0.117",
]
[[package]]
name = "ark-ff-macros"
version = "0.6.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "4a0691ed21ef00ef89c1e9bda832eba493dda3ec2f8d892fb25b705f73f06bb8"
dependencies = [
"num-bigint",
"num-traits",
"proc-macro2",
"quote",
"syn 2.0.117",
]
[[package]]
name = "ark-serialize"
version = "0.3.0"
@@ -911,30 +871,6 @@ dependencies = [
"num-bigint",
]
[[package]]
name = "ark-serialize"
version = "0.6.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "a74dd304fd536fb95d0a328e72be759209cc496a9da094c5bc56e5fea4f9e86b"
dependencies = [
"ark-serialize-derive",
"ark-std 0.6.0",
"digest 0.10.7",
"num-bigint",
"serde_with",
]
[[package]]
name = "ark-serialize-derive"
version = "0.6.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "4f153690697a2b91e5e1251ff98411ee5371500a111a0fd317a70e588eb300f9"
dependencies = [
"proc-macro2",
"quote",
"syn 2.0.117",
]
[[package]]
name = "ark-std"
version = "0.3.0"
@@ -965,16 +901,6 @@ dependencies = [
"rand 0.8.6",
]
[[package]]
name = "ark-std"
version = "0.6.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "367c9c827ed431bff6868b7aa926e05b16eb46603cc8b6e768e4a5553fa1d155"
dependencies = [
"num-traits",
"rand 0.8.6",
]
[[package]]
name = "arrayref"
version = "0.3.9"
@@ -3374,22 +3300,19 @@ dependencies = [
[[package]]
name = "git-remote-gitlawb"
version = "0.7.1"
version = "0.5.1"
dependencies = [
"anyhow",
"gitlawb-core",
"libc",
"mockito",
"reqwest",
"tempfile",
"tracing",
"tracing-subscriber",
"windows-sys 0.61.2",
]
[[package]]
name = "gitlawb-attest"
version = "0.7.1"
version = "0.5.1"
dependencies = [
"base64",
"ed25519-dalek",
@@ -3406,7 +3329,7 @@ dependencies = [
[[package]]
name = "gitlawb-core"
version = "0.7.1"
version = "0.5.1"
dependencies = [
"anyhow",
"base64",
@@ -3427,14 +3350,13 @@ dependencies = [
"sha2",
"thiserror 2.0.18",
"tokio",
"url",
"uuid",
"zeroize",
]
[[package]]
name = "gitlawb-node"
version = "0.7.1"
version = "0.5.1"
dependencies = [
"alloy",
"anyhow",
@@ -3490,11 +3412,10 @@ dependencies = [
[[package]]
name = "gl"
version = "0.7.1"
version = "0.5.1"
dependencies = [
"alloy",
"anyhow",
"base64",
"chrono",
"clap",
"dirs",
@@ -3543,9 +3464,9 @@ dependencies = [
[[package]]
name = "h2"
version = "0.4.18"
version = "0.4.13"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "839c0e8a181239723652be9062bb56ca5bf5f64011f73b623f6f4fc59086a228"
checksum = "2f44da3a8150a6703ed5d34e164b875fd14c2cdab9af1252a9a1020bde2bdc54"
dependencies = [
"atomic-waker",
"bytes",
@@ -3864,7 +3785,7 @@ dependencies = [
"libc",
"percent-encoding",
"pin-project-lite",
"socket2 0.5.10",
"socket2 0.6.3",
"tokio",
"tower-service",
"tracing",
@@ -3900,7 +3821,6 @@ version = "0.4.0"
dependencies = [
"anyhow",
"gitlawb-core",
"mockito",
"reqwest",
"serde",
"serde_json",
@@ -5432,7 +5352,7 @@ dependencies = [
"quinn-udp",
"rustc-hash",
"rustls",
"socket2 0.5.10",
"socket2 0.6.3",
"thiserror 2.0.18",
"tokio",
"tracing",
@@ -5469,7 +5389,7 @@ dependencies = [
"cfg_aliases",
"libc",
"once_cell",
"socket2 0.5.10",
"socket2 0.6.3",
"tracing",
"windows-sys 0.52.0",
]
@@ -5819,15 +5739,14 @@ dependencies = [
[[package]]
name = "ruint"
version = "1.20.0"
version = "1.17.2"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "f5e99bff0393163bb25029a6af25d3d8d202ba5b5438a74d1bd8789f5c822970"
checksum = "c141e807189ad38a07276942c6623032d3753c8859c146104ac2e4d68865945a"
dependencies = [
"alloy-rlp",
"ark-ff 0.3.0",
"ark-ff 0.4.2",
"ark-ff 0.5.0",
"ark-ff 0.6.0",
"bytes",
"fastrlp 0.3.1",
"fastrlp 0.4.0",
+1 -1
View File
@@ -71,7 +71,7 @@ Known limitations:
- Agent lifecycle revocation is not enforced by HTTP Signature authorization; do not rely on removing or revoking an agent record to block a compromised signer.
- Read visibility is not a blanket data-classification boundary: task, IPFS-pin, and Arweave-anchor listings are not repository-gated; withheld path names can be visible to a root reader; and later visibility changes cannot retract content already announced or externally anchored.
- Peer writes are signed by upgraded nodes, but strict signed-peer enforcement is opt-in during rolling upgrades.
- Current GraphQL mutations require an authenticated signer, but there is no mutation-specific guardrail that prevents a future mutation from omitting that check.
- GraphQL mutations require a signed identity (a verified signer bound to the acting DID) but are limited to the agent-task queue; repo writes are not exposed over GraphQL and remain REST-only.
- Pull-request review comments do not yet have threaded line-level anchors, and merges do not enforce approval requirements.
See:
+548
View File
@@ -279,6 +279,554 @@ mod authz_guard {
}
}
/// Completeness fence over the GraphQL mutation surface: the mirror of
/// `every_in_scope_mutation_has_its_gate` for `graphql/mutation.rs`. Kept a
/// SEPARATE guard with its own table (never folded into the REST rows above):
/// a guard over a heterogeneous set must classify per member, and these
/// same-named task rows belong to the GraphQL surface, not `api/tasks.rs`.
///
/// COMPLETENESS comes from the engine's own SDL (`schema.sdl()`), parsed as a
/// structured AST (#219), NOT introspection and NOT a text parse of source.
/// This matters: async-graphql OMITS `#[graphql(visible = false)]` fields from
/// introspection while leaving them executable, so an introspection-sourced fence
/// could be defeated by a hidden mutation. SDL includes those fields, so no
/// visibility flag (and, via the AST parse, no brace-in-description quirk) can hide
/// a live mutation. The SDL field set must equal the registered set: a new field with
/// no row fails (a repo-write cannot slip in unlisted); a registered name absent from
/// the schema fails (renamed or removed). The set is also cross-checked against
/// introspection, so any field present in SDL but hidden from introspection fails
/// named as a forbidden hidden mutation.
///
/// The MARKER check is the only source-scraped part, and it is not the
/// completeness guarantee. Three honest limits, so a green check is not misread
/// as full authorization coverage:
/// 1. A present marker is not load-bearingness (the gate may not redden on a
/// hostile probe: the #203 present-but-vacuous case). The adversarial tests
/// in `graphql/mutation.rs` are the load-bearing layer.
/// 2. A present marker is not gate-correctness (it may be the wrong bucket for
/// the operation's resource: a repo-write registered as signer-self would
/// pass, so INV-1 review remains the defense).
/// 3. The marker check scrapes the comment-masked, brace-bounded resolver body;
/// a `{`/`}` or a `did_matches(` inside a STRING literal is not masked, so it
/// could mis-slice that body or satisfy the marker vacuously (the same
/// vacuous-marker class as limit 1). Completeness is unaffected: it comes
/// from the SDL AST, not from this scrape.
#[test]
fn every_graphql_mutation_has_its_gate() {
// (Rust snake name, schema field in camelCase, expected gate marker). The
// four are Bucket C signer-self (`did_matches(`); a repo-write would be
// Bucket A (`require_repo_owner(`). Same-named REST rows live in the guard
// above against `api/tasks.rs`.
let registered: &[(&str, &str, &str)] = &[
("create_task", "createTask", "did_matches("),
("claim_task", "claimTask", "did_matches("),
("complete_task", "completeTask", "did_matches("),
("fail_task", "failTask", "did_matches("),
];
// Completeness + hidden-mutation cross-check against the REAL schema (#219),
// delegated to the generic `assert_mutation_fence` so the SDL sourcing and the
// cross-check are ALSO exercised by a synthetic hidden schema in
// `fence_rejects_a_hidden_mutation` (INV-21). Because the real MutationRoot has no
// hidden field today, that synthetic test — not a hypothetical production canary —
// is what turns reverting the SDL source or deleting the cross-check RED.
{
use crate::graphql::mutation::MutationRoot;
use crate::graphql::query::QueryRoot;
use crate::graphql::subscription::SubscriptionRoot;
let schema =
async_graphql::Schema::build(QueryRoot, MutationRoot, SubscriptionRoot).finish();
let registered_camel: Vec<&str> = registered.iter().map(|(_, c, _)| *c).collect();
assert_mutation_fence(&schema, &registered_camel);
}
// Marker present (source scrape; the vacuity limit, backstopped by the
// adversarial tests): each resolver body carries its bucket marker.
let masked = mask_comments(include_str!("../graphql/mutation.rs"));
for (snake, _, marker) in registered {
let body = resolver_body(&masked, snake);
assert!(
body.contains(marker),
"GraphQL mutation `{snake}` is missing its gate marker `{marker}`: gate removed"
);
}
}
/// Version-safety self-check (#219, INV-21): the fence's completeness guarantee rests
/// on `schema.sdl()` INCLUDING `visible = false` fields while introspection omits
/// them. That is observed async-graphql behavior, not a documented contract, so pin
/// it here: a synthetic schema with a hidden mutation must have that field in its
/// SDL-derived set and NOT in its introspection-derived set. If a future
/// async-graphql upgrade ever filters SDL by visibility, the first assertion flips
/// RED — the assumption is load-bearing, not trusted.
#[test]
fn sdl_source_sees_hidden_mutations_that_introspection_omits() {
use async_graphql::{EmptySubscription, Object, Schema};
struct Q;
#[Object]
impl Q {
async fn ping(&self) -> i32 {
0
}
}
struct M;
#[Object]
impl M {
async fn visible_mut(&self) -> i32 {
1
}
#[graphql(visible = false)]
async fn hidden_mut(&self) -> i32 {
2
}
}
let schema = Schema::build(Q, M, EmptySubscription).finish();
// SDL source (the fix): includes the hidden field.
let sdl_fields = mutation_field_names_from_sdl(&schema.sdl());
assert!(
sdl_fields.iter().any(|f| f == "hiddenMut"),
"SDL-sourced set MUST include the visible=false mutation; if this is RED, \
async-graphql started filtering SDL by visibility and the fence's \
completeness guarantee is broken: {sdl_fields:?}"
);
// Introspection (the fence's own helper): OMITS the hidden field. This is the
// exact gap the fix closes — an introspection-sourced fence never sees `hiddenMut`.
let introspected = introspection_mutation_fields(&schema);
assert!(
!introspected.iter().any(|f| f == "hiddenMut"),
"introspection is expected to OMIT the visible=false mutation (that is the \
gap the SDL source closes); got {introspected:?}"
);
// The exact diff the fence computes: hidden ∈ (sdl \ introspection).
let hidden: Vec<&String> = sdl_fields
.iter()
.filter(|f| !introspected.iter().any(|i| i == *f))
.collect();
assert_eq!(
hidden,
vec!["hiddenMut"],
"the SDL-vs-introspection diff must surface exactly the hidden mutation"
);
}
/// INV-21 wiring guard (#219): drives the generic `assert_mutation_fence` — the SAME
/// helper the real fence calls — against a synthetic schema whose mutation is
/// `visible = false` and IS registered, so completeness passes and ONLY the
/// hidden-from-introspection cross-check can fail. This is what makes the fence's SDL
/// sourcing load-bearing without a production canary: reverting the source to
/// introspection (the hidden field then absent from the SDL set) or deleting the
/// cross-check removes this panic (or changes its message), turning the
/// `should_panic` test RED. The real MutationRoot has no hidden field, so nothing on
/// the real schema could catch that regression.
#[test]
#[should_panic(expected = "hidden from introspection")]
fn fence_rejects_a_hidden_mutation() {
use async_graphql::{EmptySubscription, Object, Schema};
struct Q;
#[Object]
impl Q {
async fn ping(&self) -> i32 {
0
}
}
struct M;
#[Object]
impl M {
async fn visible_mut(&self) -> i32 {
1
}
#[graphql(visible = false)]
async fn hidden_mut(&self) -> i32 {
2
}
}
let schema = Schema::build(Q, M, EmptySubscription).finish();
// Both fields registered, so completeness passes and only the cross-check fires.
assert_mutation_fence(&schema, &["visibleMut", "hiddenMut"]);
}
/// Deprecated-mutation guard (#219): a `#[graphql(deprecation = "...")]` mutation is
/// executable and present in SDL, but GraphQL introspection omits it by default. The
/// fence's introspection helper uses `includeDeprecated: true`, so a deprecated (but
/// visible) mutation is NOT mistaken for a hidden one. With it registered, the fence
/// must NOT panic. Remove `includeDeprecated: true` and this goes RED — the deprecated
/// field would look hidden and false-red as a forbidden visible=false mutation.
#[test]
fn fence_admits_a_deprecated_mutation() {
use async_graphql::{EmptySubscription, Object, Schema};
struct Q;
#[Object]
impl Q {
async fn ping(&self) -> i32 {
0
}
}
struct M;
#[Object]
impl M {
async fn visible_mut(&self) -> i32 {
1
}
#[graphql(deprecation = "use visibleMut")]
async fn deprecated_mut(&self) -> i32 {
2
}
}
let schema = Schema::build(Q, M, EmptySubscription).finish();
// Must not panic: the deprecated field is visible (present in introspection with
// includeDeprecated) and registered, so it is neither hidden nor unclassified.
assert_mutation_fence(&schema, &["visibleMut", "deprecatedMut"]);
}
/// Completeness + hidden-mutation cross-check for a BUILT schema (#219). Generic over
/// the schema type so the real fence AND the synthetic `fence_rejects_a_hidden_mutation`
/// / `fence_admits_a_deprecated_mutation` tests drive the SAME sourcing and cross-check.
/// Completeness is sourced from SDL (which includes `visible = false` fields); the
/// cross-check fails any SDL field hidden from introspection. This genericity is the
/// INV-21 fix: because the real MutationRoot has no hidden field, only a synthetic
/// hidden schema through THIS helper makes reverting the SDL source or removing the
/// cross-check RED, rather than leaving the class latent behind a production canary.
fn assert_mutation_fence<Q, M, S>(
schema: &async_graphql::Schema<Q, M, S>,
registered_camel: &[&str],
) where
Q: async_graphql::ObjectType + 'static,
M: async_graphql::ObjectType + 'static,
S: async_graphql::SubscriptionType + 'static,
{
let sdl_fields = mutation_field_names_from_sdl(&schema.sdl());
let introspected = introspection_mutation_fields(schema);
// No SDL field may be hidden from introspection (a visible=false mutation).
for f in &sdl_fields {
assert!(
introspected.iter().any(|i| i == f),
"GraphQL mutation `{f}` is in the schema but hidden from introspection \
(a `#[graphql(visible = false)]` field): hidden mutations are forbidden \
by this fence — make it visible so its classification is enforceable, or \
remove it"
);
}
// Completeness both directions against the registered (camelCase) set.
for f in &sdl_fields {
assert!(
registered_camel.contains(&f.as_str()),
"unclassified GraphQL mutation `{f}`: add it to \
every_graphql_mutation_has_its_gate with a deliberate gate \
(`require_repo_owner(` for a repo-write, `did_matches(` for a task op)"
);
}
for c in registered_camel {
assert!(
sdl_fields.iter().any(|f| f == c),
"registered GraphQL mutation `{c}` is not in the schema \
(renamed or removed); update the table"
);
}
}
/// The mutation field names via the engine's introspection, run against a passed
/// schema. STRUCTURED (names arrive as data). Uses `fields(includeDeprecated: true)`:
/// GraphQL's default omits `@deprecated` fields, but a `#[graphql(deprecation = "...")]`
/// mutation is still executable and present in SDL, so without this the cross-check
/// above would false-red a deprecated mutation as a hidden (visible=false) one. Only a
/// real visibility filter should trip that arm; deprecation is a lifecycle marker.
fn introspection_mutation_fields<Q, M, S>(
schema: &async_graphql::Schema<Q, M, S>,
) -> Vec<String>
where
Q: async_graphql::ObjectType + 'static,
M: async_graphql::ObjectType + 'static,
S: async_graphql::SubscriptionType + 'static,
{
use async_graphql::Value;
let resp = tokio::runtime::Runtime::new()
.expect("tokio runtime")
.block_on(schema.execute(
"{ __schema { mutationType { fields(includeDeprecated: true) { name } } } }",
));
assert!(
resp.errors.is_empty(),
"schema introspection failed: {:?}",
resp.errors
);
let mut names = Vec::new();
if let Value::Object(root) = &resp.data {
if let Some(Value::Object(sc)) = root.get("__schema") {
if let Some(Value::Object(mt)) = sc.get("mutationType") {
if let Some(Value::List(field_list)) = mt.get("fields") {
for field in field_list {
if let Value::Object(fo) = field {
if let Some(Value::String(n)) = fo.get("name") {
names.push(n.clone());
}
}
}
}
}
}
}
names
}
/// The mutation field names sourced from the built schema's SDL — the completeness
/// source (#219). Builds the same DB-free schema as the introspection helper and runs
/// its `.sdl()` (sync, no runtime needed) through [`mutation_field_names_from_sdl`].
/// SDL includes `#[graphql(visible = false)]` fields, so this source cannot be
/// defeated by hiding a mutation from introspection.
fn graphql_mutation_fields_from_sdl() -> Vec<String> {
use crate::graphql::mutation::MutationRoot;
use crate::graphql::query::QueryRoot;
use crate::graphql::subscription::SubscriptionRoot;
let schema =
async_graphql::Schema::build(QueryRoot, MutationRoot, SubscriptionRoot).finish();
mutation_field_names_from_sdl(&schema.sdl())
}
/// The mutation type's field names extracted from an async-graphql SDL string via
/// the engine's OWN parser AST (`async_graphql::parser::parse_schema`), NOT
/// introspection. This is the completeness source (#219): unlike introspection,
/// which async-graphql filters by `visible` (a `#[graphql(visible = false)]`
/// mutation is executable but ABSENT from introspection), `schema.sdl()` includes
/// every registered field, so a hidden mutation still appears here and must carry a
/// classified row. Parsing the AST (not a brace/line text scan) means a `}` inside a
/// field description cannot mis-slice the type block. camelCase, as async-graphql
/// emits. An empty result on a navigation miss is safe: the set-equality in
/// `every_graphql_mutation_has_its_gate` turns "no fields" into a loud failure for
/// every registered name, never a vacuous pass.
fn mutation_field_names_from_sdl(sdl: &str) -> Vec<String> {
use async_graphql::parser::parse_schema;
use async_graphql::parser::types::{TypeKind, TypeSystemDefinition};
let doc = parse_schema(sdl).expect("async-graphql SDL must parse");
// The mutation root type name: from the `schema { mutation: X }` block if
// present, else the GraphQL default "Mutation".
let mutation_type = doc
.definitions
.iter()
.find_map(|def| match def {
TypeSystemDefinition::Schema(s) => s
.node
.mutation
.as_ref()
.map(|m| m.node.as_str().to_string()),
_ => None,
})
.unwrap_or_else(|| "Mutation".to_string());
// The fields of that object type.
doc.definitions
.iter()
.find_map(|def| match def {
TypeSystemDefinition::Type(t) if t.node.name.node.as_str() == mutation_type => {
match &t.node.kind {
TypeKind::Object(obj) => Some(
obj.fields
.iter()
.map(|f| f.node.name.node.as_str().to_string())
.collect::<Vec<_>>(),
),
_ => None,
}
}
_ => None,
})
.unwrap_or_default()
}
#[test]
fn sdl_extractor_includes_a_hidden_visible_false_field() {
// A field that would be hidden from introspection must still be listed from SDL.
let sdl = "schema { query: QueryRoot mutation: MutationRoot }\n\
type QueryRoot { ping: Int! }\n\
type MutationRoot { createTask: Int! hiddenMut: Int! }\n";
let fields = mutation_field_names_from_sdl(sdl);
assert!(
fields.iter().any(|f| f == "createTask"),
"visible field must be listed: {fields:?}"
);
assert!(
fields.iter().any(|f| f == "hiddenMut"),
"a visible=false field (present in SDL, hidden from introspection) MUST be \
listed by the completeness source: {fields:?}"
);
}
#[test]
fn sdl_extractor_is_not_fooled_by_a_brace_in_a_field_description() {
// A `}` inside a field description must not truncate the type block (the AST
// parse is immune to the text-parse quirk the old design feared).
let sdl = "schema { query: QueryRoot mutation: MutationRoot }\n\
type QueryRoot { ping: Int! }\n\
type MutationRoot {\n\
\"\"\"a description with a } brace and { another\"\"\"\n\
createTask: Int!\n\
claimTask: Int!\n\
}\n";
let fields = mutation_field_names_from_sdl(sdl);
assert!(
fields.iter().any(|f| f == "createTask") && fields.iter().any(|f| f == "claimTask"),
"both fields must be extracted despite a brace in a description: {fields:?}"
);
}
#[test]
fn sdl_extractor_returns_empty_for_a_query_only_schema() {
let sdl = "schema { query: QueryRoot }\ntype QueryRoot { ping: Int! }\n";
assert!(
mutation_field_names_from_sdl(sdl).is_empty(),
"a schema with no mutation root yields no mutation fields (no panic)"
);
}
/// The comment-masked body of `async fn <snake>` in `graphql/mutation.rs`, bounded
/// to the resolver's OWN braces (its `{ .. }`), for the marker check. Brace-matching
/// on the comment-masked source means a `}` in a trailing helper cannot extend the
/// body and a comment marker does not count. A `{`/`}` inside a STRING literal is
/// not masked, a documented marker-check-only residual (limit 3); completeness does
/// not use this.
fn resolver_body(masked_src: &str, snake: &str) -> String {
let needle = format!("async fn {snake}(");
let start = masked_src.find(&needle).unwrap_or_else(|| {
panic!("resolver `async fn {snake}(` not found in graphql/mutation.rs")
});
let open = start
+ masked_src[start..]
.find('{')
.expect("resolver body opening brace");
let mut depth = 0i32;
let mut end = masked_src.len();
for (i, c) in masked_src[open..].char_indices() {
match c {
'{' => depth += 1,
'}' => {
depth -= 1;
if depth == 0 {
end = open + i + 1;
break;
}
}
_ => {}
}
}
masked_src[open..end].to_string()
}
/// `src` with `//` line comments and `/* */` block comments (nested) blanked to
/// spaces, WITHOUT opening a comment inside a `"..."` string (so a `"https://"` does
/// not erase the rest of its line). String CONTENTS are left intact, so a brace or
/// a gate marker inside a string is not masked; for the marker check that is a
/// documented vacuity residual (limit 3), and completeness does not use this at
/// all. Char literals and raw/byte strings are not tracked (absent in resolver
/// bodies); a `'"'` char or a raw string is the remaining narrow residual.
fn mask_comments(src: &str) -> String {
let chars: Vec<char> = src.chars().collect();
let mut out = String::with_capacity(src.len());
let mut i = 0;
let mut block_depth = 0usize;
let mut in_string = false;
while i < chars.len() {
let c = chars[i];
let next = chars.get(i + 1).copied();
if block_depth > 0 {
if c == '/' && next == Some('*') {
block_depth += 1;
out.push_str(" ");
i += 2;
} else if c == '*' && next == Some('/') {
block_depth -= 1;
out.push_str(" ");
i += 2;
} else {
out.push(if c == '\n' { '\n' } else { ' ' });
i += 1;
}
} else if in_string {
out.push(c);
if c == '\\' {
if let Some(n) = next {
out.push(n);
i += 2;
} else {
i += 1;
}
} else {
if c == '"' {
in_string = false;
}
i += 1;
}
} else if c == '"' {
in_string = true;
out.push('"');
i += 1;
} else if c == '/' && next == Some('/') {
while i < chars.len() && chars[i] != '\n' {
out.push(' ');
i += 1;
}
} else if c == '/' && next == Some('*') {
block_depth = 1;
out.push_str(" ");
i += 2;
} else {
out.push(c);
i += 1;
}
}
out
}
/// The SDL field set (the completeness source of truth, #219) is exactly the
/// registered mutations. If async-graphql adds or renames a field, this reddens
/// and forces the `every_graphql_mutation_has_its_gate` table to be updated. Uses
/// SDL, not introspection: introspection omits `visible = false` fields, so an
/// introspection-based pin here would go green while a hidden mutation existed.
#[test]
fn schema_lists_exactly_the_registered_mutations() {
let mut fields = graphql_mutation_fields_from_sdl();
fields.sort();
assert_eq!(
fields,
vec!["claimTask", "completeTask", "createTask", "failTask"],
"the SDL mutation field set is the authoritative discovered set"
);
}
/// The marker check is bounded to a resolver's OWN body and is comment- and
/// string-aware: a `//` inside a `"https://"` string does not erase a real gate on
/// that line (no false-red), and a trailing module helper's `did_matches(` does not
/// rescue a resolver whose own gate was removed (no false-green).
#[test]
fn resolver_body_is_bounded_and_comment_and_string_aware() {
let src = "\
impl MutationRoot {
async fn ok(&self) -> Result<u32> {
let _ = (\"see https://x/y\", did_matches(a, b)); // URL string then gate, one line
Ok(0)
}
async fn last(&self) -> Result<u32> {
// did_matches( only in a comment
Ok(0)
}
}
fn helper() { let _ = did_matches(z, z); }
#[cfg(test)]
mod tests { async fn t() { let _ = did_matches(x, y); } }
";
let masked = mask_comments(src);
assert!(
resolver_body(&masked, "ok").contains("did_matches("),
"a `//` inside a string must not erase the real gate on that line"
);
assert!(
!resolver_body(&masked, "last").contains("did_matches("),
"the marker check must be bounded to the resolver's own body (a trailing \
helper's marker must not rescue a resolver whose gate was removed)"
);
}
/// Proves the comment-stripping that GUARD-1 added: a marker that appears only
/// in a full-line comment (the real `replicas.rs` false-pass shape) must NOT
/// satisfy a row.
+25 -16
View File
@@ -2,6 +2,7 @@
use anyhow::{Context, Result};
use clap::{Args, Subcommand};
use serde_json::Value;
use std::path::PathBuf;
use crate::http::NodeClient;
@@ -56,16 +57,24 @@ async fn cmd_list(node: String, capability: Option<String>) -> Result<()> {
.get(&path)
.await
.context("failed to connect to node")?;
// 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 {
let status = resp.status();
if 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 = crate::http::read_json(resp, "list agents").await?;
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 agents = body["agents"].as_array().cloned().unwrap_or_default();
@@ -108,15 +117,24 @@ async fn cmd_show(did: String, node: String) -> Result<()> {
.get(&format!("/api/v1/agents/{did}"))
.await
.context("failed to connect to node")?;
// Keep the bespoke 404 hint ahead of read_json; other statuses route through it.
if resp.status() == reqwest::StatusCode::NOT_FOUND {
let status = resp.status();
if 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 = crate::http::read_json(resp, "agent").await?;
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 full_did = agent["did"].as_str().unwrap_or("?");
let trust = agent["trust_score"].as_f64().unwrap_or(0.0);
@@ -197,14 +215,11 @@ 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]
@@ -215,14 +230,11 @@ 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]
@@ -249,7 +261,6 @@ mod tests {
.with_status(404)
.with_header("content-type", "application/json")
.with_body(r#"{"message":"agent not found"}"#)
.expect(1)
.create_async()
.await;
@@ -257,8 +268,6 @@ 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]
+50 -104
View File
@@ -2,6 +2,7 @@
use anyhow::{Context, Result};
use clap::{Args, Subcommand};
use serde_json::Value;
use std::path::PathBuf;
use crate::http::NodeClient;
@@ -188,7 +189,13 @@ async fn cmd_create(
.await
.context("failed to connect to node")?;
let body = crate::http::read_json(resp, "create bounty").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!("create bounty failed ({status}): {msg}");
}
let id = body["id"].as_str().unwrap_or("?");
println!("Bounty created: {id}");
@@ -224,14 +231,11 @@ async fn cmd_list(
u
};
let body = crate::http::read_json(
client
.get_authed(&url)
.await
.context("failed to connect to node")?,
"bounties",
)
.await?;
let resp = client
.get_authed(&url)
.await
.context("failed to connect to node")?;
let body: Value = resp.json().await.unwrap_or_default();
let bounties = body["bounties"].as_array();
if let Some(arr) = bounties {
@@ -255,7 +259,13 @@ async fn cmd_show(id: String, node: String, dir: Option<PathBuf>) -> Result<()>
let client = signed_client(&node, dir.as_deref());
let resp = client.get_authed(&format!("/api/v1/bounties/{id}")).await?;
let body = crate::http::read_json(resp, "show bounty").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}");
}
println!("{}", serde_json::to_string_pretty(&body)?);
Ok(())
@@ -279,7 +289,13 @@ async fn cmd_claim(
)
.await?;
let _ = crate::http::read_json(resp, "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}");
}
println!("Bounty {id} claimed. Deadline starts now.");
Ok(())
@@ -298,7 +314,13 @@ async fn cmd_submit(id: String, pr: String, node: String, dir: Option<PathBuf>)
)
.await?;
let _ = crate::http::read_json(resp, "submit").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}");
}
println!("Bounty {id} submitted with PR {pr}. Awaiting creator approval.");
Ok(())
@@ -322,7 +344,13 @@ async fn cmd_approve(
)
.await?;
let _ = crate::http::read_json(resp, "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}");
}
println!("Bounty {id} approved! Payout released to agent.");
Ok(())
@@ -341,7 +369,13 @@ async fn cmd_cancel(id: String, node: String, dir: Option<PathBuf>) -> Result<()
)
.await?;
let _ = crate::http::read_json(resp, "cancel").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}");
}
println!("Bounty {id} cancelled. Tokens refunded.");
Ok(())
@@ -349,11 +383,8 @@ async fn cmd_cancel(id: String, node: String, dir: Option<PathBuf>) -> Result<()
async fn cmd_stats(node: String) -> Result<()> {
let client = NodeClient::new(&node, None);
// 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 resp = client.get("/api/v1/bounties/stats").await?;
let body: Value = resp.json().await.unwrap_or_default();
let open = body["open"].as_i64().unwrap_or(0);
let claimed = body["claimed"].as_i64().unwrap_or(0);
@@ -490,7 +521,6 @@ mod tests {
.with_status(404)
.with_header("content-type", "application/json")
.with_body(r#"{"message":"not found"}"#)
.expect(1)
.create_async()
.await;
@@ -498,8 +528,6 @@ 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]
@@ -516,86 +544,4 @@ 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"}"#)
.expect(1)
.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}");
// 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
/// 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.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;
}
}
#[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.
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;
}
}
+48 -613
View File
@@ -4,6 +4,7 @@
use anyhow::{Context, Result};
use clap::{Args, Subcommand};
use serde_json::Value;
use std::path::PathBuf;
use crate::http::NodeClient;
@@ -81,7 +82,12 @@ async fn resolve_repo(
did.split(':').next_back().unwrap_or(&did).to_string()
} else {
let client = signed_client(node, dir);
let info = crate::http::read_json(client.get_authed("/").await?, "node info").await?;
let info: Value = client
.get_authed("/")
.await?
.json()
.await
.context("failed to fetch node info")?;
let did = info["did"].as_str().context("node info missing 'did'")?;
did.split(':').next_back().unwrap_or(did).to_string()
};
@@ -94,7 +100,12 @@ async fn cmd_list(repo: String, node: String, dir: Option<PathBuf>) -> Result<()
let client = signed_client(&node, dir.as_deref());
let path = format!("/api/v1/repos/{owner}/{name}/certs");
let resp = crate::http::read_json(client.get_authed(&path).await?, "certificates").await?;
let resp: Value = client
.get_authed(&path)
.await?
.json()
.await
.context("failed to list certificates")?;
let certs = resp["certificates"].as_array().cloned().unwrap_or_default();
@@ -128,10 +139,14 @@ 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. 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).
// Fetch the certificate
let path = format!("/api/v1/repos/{owner}/{name}/certs/{id}");
let cert = crate::http::read_json(client.get_authed(&path).await?, "certificate").await?;
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_id = cert["id"].as_str().unwrap_or("?");
let ref_name = cert["ref_name"].as_str().unwrap_or("?");
@@ -176,20 +191,27 @@ 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. 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<String, String> = 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()),
// error exit.
let current_node_did = match client.get("/").await {
Ok(resp) => resp
.json::<Value>()
.await
.ok()
.and_then(|info| info["did"].as_str().map(str::to_string)),
Err(_) => None,
};
let current_node_did = current.as_ref().ok().cloned();
for line in did_check_report(&current, node_did) {
println!("{line}");
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.");
}
}
if require_valid {
@@ -217,46 +239,6 @@ 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<String, String> {
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<String, String>, node_did: &str) -> Vec<String> {
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`.
@@ -309,7 +291,14 @@ 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 = crate::http::read_json(client.get_authed(&path).await?, "certificates").await?;
let resp: Value = client
.get_authed(&path)
.await?
.error_for_status()
.context("failed to list certificates")?
.json()
.await
.context("failed to list certificates")?;
let certs = resp["certificates"].as_array().cloned().unwrap_or_default();
let matches: Vec<String> = certs
@@ -408,558 +397,4 @@ 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"
);
_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"
);
_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"
);
_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(), "cert show must report a matching node DID");
_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(),
"cert show must warn, not fail, on a mismatching node DID"
);
_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");
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");
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}");
}
}
+14 -32
View File
@@ -2,6 +2,7 @@
use anyhow::{Context, Result};
use clap::Args;
use serde_json::Value;
use std::path::PathBuf;
use crate::http::NodeClient;
@@ -41,7 +42,12 @@ 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 = crate::http::read_json(client.get("/").await?, "node info").await?;
let info: Value = client
.get("/")
.await?
.json()
.await
.context("failed to fetch node info")?;
let did = info["did"].as_str().context("node missing DID")?;
did.split(':').next_back().unwrap_or(did).to_string()
};
@@ -58,7 +64,13 @@ pub async fn run(args: ChangelogArgs) -> Result<()> {
.await
.context("failed to connect to node")?;
let body = crate::http::read_json(resp, "changelog").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!("changelog failed ({status}): {msg}");
}
let events = body["events"].as_array().cloned().unwrap_or_default();
@@ -200,7 +212,6 @@ mod tests {
.with_status(404)
.with_header("content-type", "application/json")
.with_body(r#"{"message":"repo not found"}"#)
.expect(1)
.create_async()
.await;
@@ -212,8 +223,6 @@ 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]
@@ -261,31 +270,4 @@ 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;
}
}
+1 -269
View File
@@ -10,7 +10,6 @@ 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).
@@ -320,7 +319,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. We drop the C0/C1 control bytes (which
/// reach the terminal verbatim (INV-6). We drop the C0/C1 control bytes (which
/// defangs ANSI/OSC escapes) AND the Unicode bidi/format controls (which
/// `char::is_control` does not cover — they can reorder the displayed line).
pub(crate) fn sanitize_node_msg(s: &str) -> String {
@@ -330,60 +329,6 @@ 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` 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.
/// 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<Value> {
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.text;
// 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::<Value>(&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::*;
@@ -1323,217 +1268,4 @@ 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, 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.
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() {
// 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}");
}
}
+49 -72
View File
@@ -3,7 +3,7 @@
use anyhow::{Context, Result};
use chrono::Utc;
use clap::{Args, Subcommand};
use serde_json::json;
use serde_json::{json, Value};
use std::path::PathBuf;
use uuid::Uuid;
@@ -142,7 +142,12 @@ async fn resolve_repo(
did.split(':').next_back().unwrap_or(&did).to_string()
} else {
let client = NodeClient::new(node, None);
let info = crate::http::read_json(client.get("/").await?, "node info").await?;
let info: Value = client
.get("/")
.await?
.json()
.await
.context("failed to fetch node info")?;
let did = info["did"].as_str().context("node info missing 'did'")?;
did.split(':').next_back().unwrap_or(did).to_string()
};
@@ -193,7 +198,13 @@ async fn cmd_create(
.post(&path, &request_body)
.await
.context("failed to connect to node")?;
let result = crate::http::read_json(resp, "create issue").await?;
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 id = result["id"].as_str().unwrap_or("?");
println!("✓ Created issue #{id}");
@@ -210,7 +221,12 @@ async fn cmd_list(repo: String, node: String, dir: Option<PathBuf>) -> Result<()
let client = signed_client(&node, dir.as_deref());
let path = format!("/api/v1/repos/{owner}/{name}/issues");
let resp = crate::http::read_json(client.get_authed(&path).await?, "issues").await?;
let resp: Value = client
.get_authed(&path)
.await?
.json()
.await
.context("failed to list issues")?;
let issues = resp["issues"].as_array().cloned().unwrap_or_default();
@@ -248,7 +264,13 @@ async fn cmd_show(repo: String, id: String, node: String, dir: Option<PathBuf>)
.get_authed(&path)
.await
.context("failed to connect to node")?;
let issue = crate::http::read_json(resp, "show").await?;
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 title = issue["title"].as_str().unwrap_or("(no title)");
let status = issue["status"].as_str().unwrap_or("?");
@@ -279,7 +301,13 @@ async fn cmd_close(repo: String, id: String, node: String, dir: Option<PathBuf>)
.post(&format!("{path}/close"), &body)
.await
.context("failed to connect to node")?;
let _ = crate::http::read_json(resp, "close issue").await?;
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}");
}
println!("✗ Closed issue {id}");
Ok(())
@@ -304,7 +332,13 @@ async fn cmd_issue_comment(
)
.await
.context("failed to connect to node")?;
let _ = crate::http::read_json(resp, "comment").await?;
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}");
}
println!("· Comment posted on issue {id}");
Ok(())
@@ -319,15 +353,14 @@ 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 = crate::http::read_json(
client
.get_authed(&format!(
"/api/v1/repos/{owner}/{name}/issues/{id}/comments"
))
.await?,
"issue comments",
)
.await?;
let resp: Value = client
.get_authed(&format!(
"/api/v1/repos/{owner}/{name}/issues/{id}/comments"
))
.await?
.json()
.await
.context("invalid JSON")?;
let comments = resp["comments"].as_array().cloned().unwrap_or_default();
if comments.is_empty() {
@@ -457,7 +490,6 @@ mod tests {
.with_status(404)
.with_header("content-type", "application/json")
.with_body(r#"{"message":"repo not found"}"#)
.expect(1)
.create_async()
.await;
@@ -472,8 +504,6 @@ 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]
@@ -521,7 +551,6 @@ mod tests {
.with_status(404)
.with_header("content-type", "application/json")
.with_body(r#"{"message":"issue not found"}"#)
.expect(1)
.create_async()
.await;
@@ -535,8 +564,6 @@ 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]
@@ -585,7 +612,6 @@ mod tests {
.with_status(404)
.with_header("content-type", "application/json")
.with_body(r#"{"message":"issue not found"}"#)
.expect(1)
.create_async()
.await;
@@ -598,8 +624,6 @@ 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]
@@ -646,7 +670,6 @@ mod tests {
.with_status(404)
.with_header("content-type", "application/json")
.with_body(r#"{"message":"issue not found"}"#)
.expect(1)
.create_async()
.await;
@@ -660,8 +683,6 @@ 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]
@@ -719,48 +740,4 @@ 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;
}
}
+154 -836
View File
File diff suppressed because it is too large. Load diff
+31 -296
View File
@@ -63,7 +63,12 @@ pub async fn run(args: PeerArgs) -> Result<()> {
async fn cmd_list(node: String) -> Result<()> {
let client = NodeClient::new(&node, None);
let resp = crate::http::read_json(client.get("/api/v1/peers").await?, "peers").await?;
let resp: Value = client
.get("/api/v1/peers")
.await?
.json()
.await
.context("failed to list peers")?;
let peers = resp["peers"].as_array().cloned().unwrap_or_default();
let count = resp["count"].as_u64().unwrap_or(peers.len() as u64);
@@ -164,20 +169,18 @@ async fn cmd_add(peer_url: String, node: String, dir: Option<PathBuf>) -> 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. 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.
// Fetch our node's public URL so we can announce it to the peer
let local_client = NodeClient::new(&node, None);
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()
}
};
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();
// Announce our local node to the remote peer
let body = serde_json::to_vec(&serde_json::json!({
@@ -253,7 +256,12 @@ async fn cmd_add(peer_url: String, node: String, dir: Option<PathBuf>) -> 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 = crate::http::read_json(client.get(&path).await?, "ping peer").await?;
let resp: Value = client
.get(&path)
.await?
.json()
.await
.context("failed to ping peer")?;
let url = resp["http_url"].as_str().unwrap_or("?");
let reachable = resp["reachable"].as_bool().unwrap_or(false);
@@ -273,7 +281,12 @@ 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 = crate::http::read_json(client.get(&path).await?, "resolve DID").await?;
let resp: Value = client
.get(&path)
.await?
.json()
.await
.context("failed to resolve DID")?;
let source = resp["source"].as_str().unwrap_or("not found");
let http_url = resp["http_url"].as_str().unwrap_or("(none)");
@@ -296,8 +309,8 @@ async fn cmd_resolve(did: String, node: String) -> Result<()> {
#[cfg(test)]
mod tests {
use super::{
announced_peer_summary, cmd_add, cmd_list, cmd_ping, cmd_resolve, local_add_refusal,
local_add_report, remote_announce_failure,
announced_peer_summary, cmd_add, local_add_refusal, local_add_report,
remote_announce_failure,
};
use reqwest::StatusCode;
use serde_json::json;
@@ -590,282 +603,4 @@ 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;
}
}
+73 -321
View File
@@ -2,6 +2,7 @@
use anyhow::{Context, Result};
use clap::{Args, Subcommand};
use serde_json::Value;
use std::path::PathBuf;
use crate::http::NodeClient;
@@ -232,7 +233,13 @@ async fn cmd_create(
.post(&format!("/api/v1/repos/{owner}/{repo}/pulls"), &payload)
.await
.context("failed to connect to node")?;
let pr = crate::http::read_json(resp, "create PR").await?;
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 number = pr["number"].as_i64().unwrap_or(0);
println!("✓ Opened PR #{number}: {title}");
@@ -247,13 +254,12 @@ async fn cmd_list(repo: String, node: String, dir: Option<PathBuf>) -> Result<()
let owner = resolve_owner(&keypair);
let client = NodeClient::new(&node, None);
let resp = crate::http::read_json(
client
.get(&format!("/api/v1/repos/{owner}/{repo}/pulls"))
.await?,
"pull requests",
)
.await?;
let resp: Value = client
.get(&format!("/api/v1/repos/{owner}/{repo}/pulls"))
.await?
.json()
.await
.context("invalid JSON")?;
let prs = resp["pulls"].as_array().cloned().unwrap_or_default();
if prs.is_empty() {
@@ -292,13 +298,12 @@ async fn cmd_view(repo: String, number: u64, node: String, dir: Option<PathBuf>)
let owner = resolve_owner(&keypair);
let client = NodeClient::new(&node, None);
let pr = crate::http::read_json(
client
.get(&format!("/api/v1/repos/{owner}/{repo}/pulls/{number}"))
.await?,
"pull request",
)
.await?;
let pr: Value = client
.get(&format!("/api/v1/repos/{owner}/{repo}/pulls/{number}"))
.await?
.json()
.await
.context("invalid JSON")?;
let title = pr["title"].as_str().unwrap_or("?");
let status = pr["status"].as_str().unwrap_or("?");
@@ -316,25 +321,15 @@ async fn cmd_view(repo: String, number: u64, node: String, dir: Option<PathBuf>)
}
// Show reviews
// 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"
))
.await?,
"reviews",
)
.await
{
Ok(v) => v["reviews"].as_array().cloned().unwrap_or_default(),
Err(e) => {
println!("\nReviews unavailable ({e})");
Vec::new()
}
};
let reviews: Value = client
.get(&format!(
"/api/v1/repos/{owner}/{repo}/pulls/{number}/reviews"
))
.await?
.json()
.await
.context("invalid JSON")?;
let reviews = reviews["reviews"].as_array().cloned().unwrap_or_default();
if !reviews.is_empty() {
println!("\nReviews ({}):", reviews.len());
for r in &reviews {
@@ -359,22 +354,15 @@ async fn cmd_view(repo: String, number: u64, node: String, dir: Option<PathBuf>)
}
// Show comments
let comments = match crate::http::read_json(
client
.get(&format!(
"/api/v1/repos/{owner}/{repo}/pulls/{number}/comments"
))
.await?,
"comments",
)
.await
{
Ok(v) => v["comments"].as_array().cloned().unwrap_or_default(),
Err(e) => {
println!("\nComments unavailable ({e})");
Vec::new()
}
};
let comments: Value = client
.get(&format!(
"/api/v1/repos/{owner}/{repo}/pulls/{number}/comments"
))
.await?
.json()
.await
.context("invalid JSON")?;
let comments = comments["comments"].as_array().cloned().unwrap_or_default();
if !comments.is_empty() {
println!("\nComments ({}):", comments.len());
for c in &comments {
@@ -398,13 +386,12 @@ async fn cmd_diff(repo: String, number: u64, node: String, dir: Option<PathBuf>)
let owner = resolve_owner(&keypair);
let client = NodeClient::new(&node, None);
let resp = crate::http::read_json(
client
.get(&format!("/api/v1/repos/{owner}/{repo}/pulls/{number}/diff"))
.await?,
"diff",
)
.await?;
let resp: Value = client
.get(&format!("/api/v1/repos/{owner}/{repo}/pulls/{number}/diff"))
.await?
.json()
.await
.context("invalid JSON")?;
let diff = resp["diff"].as_str().unwrap_or("");
if diff.is_empty() {
@@ -428,7 +415,13 @@ async fn cmd_merge(repo: String, number: u64, node: String, dir: Option<PathBuf>
)
.await
.context("failed to connect to node")?;
let result = crate::http::read_json(resp, "merge").await?;
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 sha = result["merge_sha"].as_str().unwrap_or("?");
println!("✓ Merged PR #{number}");
@@ -460,7 +453,13 @@ async fn cmd_review(
)
.await
.context("failed to connect to node")?;
let _ = crate::http::read_json(resp, "review").await?;
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 icon = match status.as_str() {
"approved" => "✓",
@@ -490,7 +489,13 @@ async fn cmd_comment(
)
.await
.context("failed to connect to node")?;
let _ = crate::http::read_json(resp, "comment").await?;
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}");
}
println!("· Comment posted on PR #{number}");
Ok(())
@@ -501,15 +506,14 @@ async fn cmd_comments(repo: String, number: u64, node: String, dir: Option<PathB
let owner = resolve_owner(&keypair);
let client = NodeClient::new(&node, None);
let resp = crate::http::read_json(
client
.get(&format!(
"/api/v1/repos/{owner}/{repo}/pulls/{number}/comments"
))
.await?,
"comments",
)
.await?;
let resp: Value = client
.get(&format!(
"/api/v1/repos/{owner}/{repo}/pulls/{number}/comments"
))
.await?
.json()
.await
.context("invalid JSON")?;
let comments = resp["comments"].as_array().cloned().unwrap_or_default();
if comments.is_empty() {
@@ -650,7 +654,6 @@ mod tests {
.with_status(422)
.with_header("content-type", "application/json")
.with_body(r#"{"message":"branch not found"}"#)
.expect(1)
.create_async()
.await;
@@ -668,8 +671,6 @@ mod tests {
assert!(result.is_err());
assert!(result.unwrap_err().to_string().contains("branch 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]
@@ -767,7 +768,6 @@ mod tests {
.with_status(404)
.with_header("content-type", "application/json")
.with_body(r#"{"message":"PR not found"}"#)
.expect(1)
.create_async()
.await;
@@ -781,8 +781,6 @@ mod tests {
.await
.unwrap_err();
assert!(err.to_string().contains("PR 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.
_m.assert_async().await;
}
#[tokio::test]
@@ -867,250 +865,4 @@ mod tests {
.await
.unwrap();
}
// ── Gated PR reads surface node denials, not empty/stub renders (#123) ──
#[tokio::test]
async fn cmd_list_surfaces_denial_not_empty() {
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"/pulls$".to_string()))
.with_status(404)
.with_header("content-type", "application/json")
.with_body(r#"{"message":"repository not found"}"#)
.expect(1)
.create_async()
.await;
let result = cmd_list(
"myrepo".to_string(),
server.url(),
Some(dir.path().to_path_buf()),
)
.await;
assert!(
result.is_err(),
"cmd_list must Err on 404, not print 'No pull requests'"
);
// 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_view_surfaces_denial_not_stub() {
let dir = TempDir::new().unwrap();
write_identity(&dir);
let mut server = mockito::Server::new_async().await;
// The PR fetch is first; a 404 there errors before the reviews/comments reads.
let _m = server
.mock("GET", mockito::Matcher::Regex(r"/pulls/1$".to_string()))
.with_status(404)
.with_header("content-type", "application/json")
.with_body(r#"{"message":"repository not found"}"#)
.expect(1)
.create_async()
.await;
let result = cmd_view(
"myrepo".to_string(),
1,
server.url(),
Some(dir.path().to_path_buf()),
)
.await;
assert!(
result.is_err(),
"cmd_view must Err on 404, not print a stub PR"
);
// 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_diff_surfaces_denial_not_empty() {
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"/pulls/1/diff$".to_string()),
)
.with_status(404)
.with_header("content-type", "application/json")
.with_body(r#"{"message":"repository not found"}"#)
.expect(1)
.create_async()
.await;
let result = cmd_diff(
"myrepo".to_string(),
1,
server.url(),
Some(dir.path().to_path_buf()),
)
.await;
assert!(
result.is_err(),
"cmd_diff must Err on 404, not print 'No diff'"
);
// 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_comments_surfaces_denial_not_empty() {
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"/pulls/1/comments$".to_string()),
)
.with_status(404)
.with_header("content-type", "application/json")
.with_body(r#"{"message":"repository not found"}"#)
.expect(1)
.create_async()
.await;
let result = cmd_comments(
"myrepo".to_string(),
1,
server.url(),
Some(dir.path().to_path_buf()),
)
.await;
assert!(result.is_err(), "cmd_comments must Err on a gated 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;
}
/// 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;
}
}
+28 -29
View File
@@ -2,6 +2,7 @@
use anyhow::{Context, Result};
use clap::{Args, Subcommand};
use serde_json::Value;
use std::path::PathBuf;
use crate::http::NodeClient;
@@ -81,7 +82,12 @@ async fn resolve_owner_repo(
did.split(':').next_back().unwrap_or(&did).to_string()
} else {
let client = NodeClient::new(node, None);
let info = crate::http::read_json(client.get("/").await?, "node info").await?;
let info: Value = client
.get("/")
.await?
.json()
.await
.context("failed to fetch node info")?;
let did = info["did"].as_str().context("node missing DID")?;
did.split(':').next_back().unwrap_or(did).to_string()
};
@@ -102,7 +108,13 @@ async fn cmd_set(branch: String, repo: String, node: String, dir: Option<PathBuf
.await
.context("failed to connect to node")?;
let _ = crate::http::read_json(resp, "protect").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!("protect failed ({status}): {msg}");
}
println!("✓ Branch '{branch}' is now protected in {owner}/{name}");
println!(" Only the repo owner can push to this branch.");
@@ -128,7 +140,13 @@ async fn cmd_remove(
.await
.context("failed to connect to node")?;
let _ = crate::http::read_json(resp, "unprotect").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!("unprotect failed ({status}): {msg}");
}
println!("✓ Branch '{branch}' is no longer protected in {owner}/{name}");
Ok(())
@@ -145,7 +163,13 @@ async fn cmd_list(repo: String, node: String, dir: Option<PathBuf>) -> Result<()
.await
.context("failed to connect to node")?;
let body = crate::http::read_json(resp, "list protected branches").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!("list protected branches failed ({status}): {msg}");
}
let branches = body["protected_branches"]
.as_array()
@@ -223,7 +247,6 @@ 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;
@@ -236,8 +259,6 @@ 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]
@@ -349,26 +370,4 @@ 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;
}
}
+11 -5
View File
@@ -5,7 +5,7 @@
use anyhow::{Context, Result};
use clap::Args;
use serde_json::json;
use serde_json::{json, Value};
use std::path::PathBuf;
use crate::http::NodeClient;
@@ -55,7 +55,16 @@ pub async fn run(args: RegisterArgs) -> Result<()> {
.await
.context("failed to connect to node")?;
let payload = crate::http::read_json(resp, "registration").await?;
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}");
}
// Save bootstrap UCAN
let ucan = payload.get("ucan").and_then(|v| v.as_str()).unwrap_or("");
@@ -162,7 +171,6 @@ mod tests {
.with_status(401)
.with_header("content-type", "application/json")
.with_body(r#"{"message":"invalid signature"}"#)
.expect(1)
.create_async()
.await;
@@ -179,8 +187,6 @@ 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]
+83 -164
View File
@@ -251,7 +251,13 @@ async fn cmd_create(
.post("/api/v1/repos", &body)
.await
.context("failed to connect to node")?;
let payload = crate::http::read_json(resp, "create").await?;
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 clone_url = payload["clone_url"].as_str().unwrap_or("");
let gitlawb_url = format!("gitlawb://{owner_did}/{name}");
@@ -271,7 +277,12 @@ async fn cmd_list(node: String, dir: Option<PathBuf>) -> Result<()> {
let client = NodeClient::new(&node, None);
let url = format!("/api/v1/repos?owner={owner}");
let repos = crate::http::read_json(client.get(&url).await?, "list repos").await?;
let repos: Value = client
.get(&url)
.await?
.json()
.await
.context("failed to list repos")?;
let repos = repos.as_array().context("expected array")?;
if repos.is_empty() {
@@ -340,13 +351,20 @@ async fn cmd_info(repo: String, node: String, dir: Option<PathBuf>) -> 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().as_u16() == 404 {
anyhow::bail!("repository '{owner}/{name}' not found");
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::<Value>()
.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}");
}
// Every other status routes through read_json: a 2xx yields the parsed body,
// 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?;
let r: Value = resp.json().await.context("parse repo info")?;
let owner_did = r["owner_did"].as_str().unwrap_or(&owner);
let gitlawb_url = format!("gitlawb://{owner_did}/{name}");
@@ -405,7 +423,12 @@ async fn cmd_replica_register(
.await
.context("failed to connect to origin node")?;
let body = crate::http::read_json(resp, "replica register").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!("replica register failed ({status}): {msg}");
}
let count = body["replica_count"].as_i64().unwrap_or(0);
println!("Registered as replica of {owner}/{name}");
@@ -432,7 +455,12 @@ async fn cmd_replica_unregister(repo: String, node: String, dir: Option<PathBuf>
.await
.context("failed to connect to origin node")?;
let body = crate::http::read_json(resp, "replica unregister").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!("replica unregister failed ({status}): {msg}");
}
let count = body["replica_count"].as_i64().unwrap_or(0);
println!("Unregistered as replica of {owner}/{name} ({count} replicas remaining)");
@@ -452,7 +480,12 @@ async fn cmd_replicas(repo: String, node: String, dir: Option<PathBuf>) -> Resul
.get_maybe_signed(&format!("/api/v1/repos/{owner}/{name}/replicas"))
.await
.context("failed to connect to node")?;
let body = crate::http::read_json(resp, "replicas list").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!("replicas list failed ({status}): {msg}");
}
let count = body["replica_count"].as_i64().unwrap_or(0);
println!("{owner}/{name}: {count} replicas");
@@ -488,7 +521,12 @@ pub(crate) async fn cmd_commits(
};
let url = format!("/api/v1/repos/{owner}/{name}/commits?branch={branch}&limit={limit}");
let resp = crate::http::read_json(client.get(&url).await?, "commits").await?;
let resp: Value = client
.get(&url)
.await?
.json()
.await
.context("failed to fetch commits")?;
let commits = resp["commits"].as_array().cloned().unwrap_or_default();
if commits.is_empty() {
@@ -541,7 +579,13 @@ async fn cmd_fork(
.post(&format!("/api/v1/repos/{owner}/{repo_name}/fork"), &body)
.await
.context("failed to connect to node")?;
let result = crate::http::read_json(resp, "fork").await?;
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 fork_name = result["name"].as_str().unwrap_or(&repo_name);
let owner_did = result["owner_did"].as_str().unwrap_or("?");
@@ -579,7 +623,13 @@ async fn cmd_label_add(
.post(&format!("/api/v1/repos/{owner}/{name}/labels"), &body)
.await
.context("failed to connect to node")?;
let result = crate::http::read_json(resp, "add label").await?;
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 added = result["added"].as_bool().unwrap_or(true);
if added {
@@ -604,7 +654,13 @@ async fn cmd_label_remove(
.delete(&format!("/api/v1/repos/{owner}/{name}/labels/{label}"), &[])
.await
.context("failed to connect to node")?;
let _ = crate::http::read_json(resp, "remove label").await?;
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}");
}
println!("- Label removed: {label} from {owner}/{name}");
Ok(())
@@ -617,21 +673,18 @@ async fn cmd_label_list(repo: String, node: String, dir: Option<PathBuf>) -> Res
// read their own labels, while public repos stay anonymously listable.
let client = NodeClient::new(&node, load_keypair_from_dir(dir.as_deref()).ok());
// 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 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}");
}
let labels = resp["labels"].as_array().cloned().unwrap_or_default();
let labels = body["labels"].as_array().cloned().unwrap_or_default();
if labels.is_empty() {
println!("No labels on {owner}/{name}");
} else {
@@ -804,7 +857,6 @@ mod tests {
.with_status(409)
.with_header("content-type", "application/json")
.with_body(r#"{"message":"repository already exists"}"#)
.expect(1)
.create_async()
.await;
@@ -820,8 +872,6 @@ 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]
@@ -890,33 +940,6 @@ 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.
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();
@@ -1016,7 +1039,6 @@ 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;
@@ -1032,8 +1054,6 @@ 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]
@@ -1406,7 +1426,6 @@ mod tests {
.with_status(404)
.with_header("content-type", "application/json")
.with_body(r#"{"message":"not found"}"#)
.expect(1)
.create_async()
.await;
@@ -1419,106 +1438,6 @@ 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) ──
#[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]
@@ -1717,7 +1636,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 client-contract break).
// render a repo card (deny-as-success is the INV-8 client-contract break).
#[tokio::test]
async fn cmd_info_403_denied_surfaces_error_not_card() {
let dir = TempDir::new().unwrap();
+22 -56
View File
@@ -2,6 +2,7 @@
use anyhow::{Context, Result};
use clap::{Args, Subcommand};
use serde_json::Value;
use std::path::PathBuf;
use crate::http::NodeClient;
@@ -74,7 +75,13 @@ async fn cmd_add(repo: String, node: String, dir: Option<PathBuf>) -> Result<()>
.await
.context("failed to connect to node")?;
let body = crate::http::read_json(resp, "star").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!("star failed ({status}): {msg}");
}
let count = body["star_count"].as_i64().unwrap_or(0);
println!("Starred {owner}/{name} ({count} stars total)");
@@ -92,7 +99,13 @@ async fn cmd_remove(repo: String, node: String, dir: Option<PathBuf>) -> Result<
.await
.context("failed to connect to node")?;
let body = crate::http::read_json(resp, "unstar").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!("unstar failed ({status}): {msg}");
}
let count = body["star_count"].as_i64().unwrap_or(0);
println!("Unstarred {owner}/{name} ({count} stars remaining)");
@@ -111,7 +124,13 @@ async fn cmd_count(repo: String, node: String, dir: Option<PathBuf>) -> Result<(
.await
.context("failed to connect to node")?;
let body = crate::http::read_json(resp, "star count").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!("star count failed ({status}): {msg}");
}
let count = body["star_count"].as_i64().unwrap_or(0);
println!("{owner}/{name}: {count} stars");
@@ -211,7 +230,6 @@ mod tests {
.with_status(404)
.with_header("content-type", "application/json")
.with_body(r#"{"message":"repo not found"}"#)
.expect(1)
.create_async()
.await;
@@ -223,8 +241,6 @@ 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]
@@ -271,7 +287,6 @@ mod tests {
.with_status(404)
.with_header("content-type", "application/json")
.with_body(r#"{"message":"repo not found"}"#)
.expect(1)
.create_async()
.await;
@@ -283,8 +298,6 @@ 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]
@@ -322,51 +335,4 @@ 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;
}
}
+15 -166
View File
@@ -37,9 +37,17 @@ 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);
let resp = client.get(&format!("/api/v1/agents/{short_key}")).await;
if let Some(line) = trust_line(resp).await {
println!("{line}");
match client.get(&format!("/api/v1/agents/{short_key}")).await {
Ok(resp) if resp.status().is_success() => {
if let Ok(body) = resp.json::<Value>().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`)");
}
}
}
@@ -73,10 +81,8 @@ pub async fn run(args: StatusArgs) -> Result<()> {
let pr_resp = client
.get(&format!("/api/v1/repos/{short_owner}/{repo_name}/pulls"))
.await;
if let Some(line) = section_unavailable_line("PRs", &pr_resp) {
println!("{line}");
} else if let Ok(r) = pr_resp {
if let Ok(body) = crate::http::read_json(r, "pull requests").await {
if let Ok(r) = pr_resp {
if let Ok(body) = r.json::<Value>().await {
let prs = body["pulls"].as_array().cloned().unwrap_or_default();
let open: Vec<_> = prs
.iter()
@@ -102,10 +108,8 @@ pub async fn run(args: StatusArgs) -> Result<()> {
let issue_resp = client
.get(&format!("/api/v1/repos/{short_owner}/{repo_name}/issues"))
.await;
if let Some(line) = section_unavailable_line("issues", &issue_resp) {
println!("{line}");
} else if let Ok(r) = issue_resp {
if let Ok(body) = crate::http::read_json(r, "issues").await {
if let Ok(r) = issue_resp {
if let Ok(body) = r.json::<Value>().await {
let issues = body["issues"].as_array().cloned().unwrap_or_default();
let open: Vec<_> = issues
.iter()
@@ -132,40 +136,6 @@ 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 ...".
/// 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<reqwest::Response>) -> Option<String> {
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
/// A transport error degrades silently — the node-unreachable line below
/// already surfaces it — matching `section_unavailable_line`.
async fn trust_line(resp: Result<reqwest::Response>) -> Option<String> {
match resp {
Ok(r) if r.status().is_success() => {
let body = r.json::<Value>().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;
@@ -406,125 +376,4 @@ mod tests {
fn trust_bar_quarter() {
assert_eq!(trust_bar(0.25), "█░░░");
}
// ── gl status section denial surfacing (#123, R5) ────────────────────
async fn get_response(
server: &mut mockito::Server,
status: usize,
) -> Result<reqwest::Response> {
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".
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<reqwest::Response> = 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. ───────────────
async fn agents_resp(
server: &mut mockito::Server,
status: usize,
body: &str,
) -> Result<reqwest::Response> {
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());
}
}
+2 -82
View File
@@ -71,9 +71,8 @@ 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 =
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?;
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?;
println!("Node stats:");
println!(" repos: {}", stats["repos"].as_i64().unwrap_or(0));
println!(" agents: {}", stats["agents"].as_i64().unwrap_or(0));
@@ -159,7 +158,6 @@ 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());
@@ -168,8 +166,6 @@ 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]
@@ -180,7 +176,6 @@ 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());
@@ -189,8 +184,6 @@ 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]
@@ -207,7 +200,6 @@ 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());
@@ -219,8 +211,6 @@ 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]
@@ -246,7 +236,6 @@ 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());
@@ -258,8 +247,6 @@ 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]
@@ -290,71 +277,4 @@ 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;
}
}
+47 -176
View File
@@ -166,14 +166,13 @@ async fn cmd_create(
"delegator_did": delegator_did,
}))?;
let resp = crate::http::read_json(
client
.post("/api/v1/tasks", &body)
.await
.context("failed to create task")?,
"create task",
)
.await?;
let resp: Value = client
.post("/api/v1/tasks", &body)
.await
.context("failed to create task")?
.json()
.await
.context("invalid JSON response")?;
print_json(&resp);
Ok(())
}
@@ -192,25 +191,26 @@ async fn cmd_list(
if let Some(a) = &assignee_did {
path.push_str(&format!("&assignee_did={}", urlencoding::encode(a)));
}
let resp = crate::http::read_json(
client.get(&path).await.context("failed to list tasks")?,
"list tasks",
)
.await?;
let resp: Value = client
.get(&path)
.await
.context("failed to list tasks")?
.json()
.await
.context("invalid JSON response")?;
print_json(&resp);
Ok(())
}
async fn cmd_view(id: String, node: String) -> Result<()> {
let client = NodeClient::new(&node, None);
let resp = crate::http::read_json(
client
.get(&format!("/api/v1/tasks/{}", id))
.await
.context("failed to get task")?,
"view task",
)
.await?;
let resp: Value = client
.get(&format!("/api/v1/tasks/{}", id))
.await
.context("failed to get task")?
.json()
.await
.context("invalid JSON response")?;
print_json(&resp);
Ok(())
}
@@ -221,14 +221,13 @@ async fn cmd_claim(id: String, node: String, dir: Option<PathBuf>) -> Result<()>
let client = NodeClient::new(&node, Some(keypair));
let body = serde_json::to_vec(&json!({ "assignee_did": assignee_did }))?;
let resp = crate::http::read_json(
client
.post(&format!("/api/v1/tasks/{}/claim", id), &body)
.await
.context("failed to claim task")?,
"claim task",
)
.await?;
let resp: Value = client
.post(&format!("/api/v1/tasks/{}/claim", id), &body)
.await
.context("failed to claim task")?
.json()
.await
.context("invalid JSON response")?;
print_json(&resp);
Ok(())
}
@@ -244,14 +243,13 @@ 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 = crate::http::read_json(
client
.post(&format!("/api/v1/tasks/{}/complete", id), &body)
.await
.context("failed to complete task")?,
"complete task",
)
.await?;
let resp: Value = client
.post(&format!("/api/v1/tasks/{}/complete", id), &body)
.await
.context("failed to complete task")?
.json()
.await
.context("invalid JSON response")?;
print_json(&resp);
Ok(())
}
@@ -267,14 +265,13 @@ 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 = crate::http::read_json(
client
.post(&format!("/api/v1/tasks/{}/fail", id), &body)
.await
.context("failed to fail task")?,
"fail task",
)
.await?;
let resp: Value = client
.post(&format!("/api/v1/tasks/{}/fail", id), &body)
.await
.context("failed to fail task")?
.json()
.await
.context("invalid JSON response")?;
print_json(&resp);
Ok(())
}
@@ -358,12 +355,11 @@ mod tests {
.with_status(500)
.with_header("content-type", "application/json")
.with_body(r#"{"message":"internal error"}"#)
.expect(1)
.create_async()
.await;
// A node 5xx must surface as an Err, not be printed as if it were a task.
let err = cmd_create(
// Should still succeed (prints JSON, doesn't check status code)
cmd_create(
"deploy".to_string(),
"agent:task".to_string(),
None,
@@ -375,9 +371,7 @@ mod tests {
Some(dir.path().to_path_buf()),
)
.await
.unwrap_err();
assert!(err.to_string().contains("500"), "got: {err}");
_m.assert_async().await;
.unwrap();
}
// ── list ─────────────────────────────────────────────────────────
@@ -451,16 +445,11 @@ mod tests {
.with_status(404)
.with_header("content-type", "application/json")
.with_body(r#"{"message":"not found"}"#)
.expect(1)
.create_async()
.await;
// 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;
// cmd_view doesn't check status — it prints the JSON
cmd_view("nope".to_string(), server.url()).await.unwrap();
}
// ── claim ────────────────────────────────────────────────────────
@@ -612,122 +601,4 @@ mod tests {
.await
.unwrap();
}
// ── denial surfaces: 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;
}
}
+25 -26
View File
@@ -2,6 +2,7 @@
use anyhow::{Context, Result};
use clap::{Args, Subcommand};
use serde_json::Value;
use std::path::{Path, PathBuf};
use crate::http::NodeClient;
@@ -86,7 +87,12 @@ async fn resolve_owner_repo(
did.split(':').next_back().unwrap_or(&did).to_string()
} else {
let client = NodeClient::new(node, None);
let info = crate::http::read_json(client.get("/").await?, "node info").await?;
let info: Value = client
.get("/")
.await?
.json()
.await
.context("failed to fetch node info")?;
let did = info["did"].as_str().context("node missing DID")?;
did.split(':').next_back().unwrap_or(did).to_string()
};
@@ -117,7 +123,12 @@ async fn cmd_set(
.await
.context("failed to connect to node")?;
let _ = crate::http::read_json(resp, "visibility set").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!("visibility set failed ({status}): {msg}");
}
println!("✓ Visibility rule set on {owner}/{name}: {path_glob} (mode {mode})");
if path_glob != "/" {
@@ -145,7 +156,12 @@ async fn cmd_remove(
.await
.context("failed to connect to node")?;
let _ = crate::http::read_json(resp, "visibility remove").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!("visibility remove failed ({status}): {msg}");
}
println!("✓ Visibility rule removed from {owner}/{name}: {path_glob}");
Ok(())
@@ -163,7 +179,12 @@ async fn cmd_list(repo: String, node: String, dir: Option<PathBuf>) -> Result<()
.await
.context("failed to connect to node")?;
let body = crate::http::read_json(resp, "visibility list").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!("visibility list failed ({status}): {msg}");
}
let rules = body["rules"].as_array().cloned().unwrap_or_default();
if rules.is_empty() {
@@ -286,26 +307,4 @@ 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;
}
}
+35 -34
View File
@@ -98,17 +98,17 @@ async fn cmd_create(
"events": event_list,
}))?;
// #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 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}");
}
let id = hook["id"].as_str().unwrap_or("?");
let hook_events = hook["events"]
@@ -145,15 +145,16 @@ async fn cmd_list(repo: String, node: String, dir: Option<PathBuf>) -> 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.
// #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 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}");
}
let hooks = body
.get("webhooks")
@@ -194,20 +195,20 @@ async fn cmd_delete(repo: String, id: String, node: String, dir: Option<PathBuf>
let client = NodeClient::new(&node, Some(keypair));
let payload = serde_json::to_vec(&serde_json::json!({}))?;
// #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?;
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}");
}
println!("✓ Webhook {id} deleted");
Ok(())
-356
View File
@@ -1,356 +0,0 @@
//! 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.
//! 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",
"status.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::<T>().await`, or the head of a split-line chain (`.json(` /
/// `.json::<T>(` 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::<Value>().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 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). 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<String>, Vec<String>) {
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]
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 (o, u) = scan_handler(name, text);
offenders.extend(o);
unguarded.extend(u);
}
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 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<String> = 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:?}"
);
}
+1 -1
View File
@@ -41,7 +41,7 @@ Owner focus: protocol/auth.
- Close default-open repo write authorization and wire trusted UCAN delegation into repository permissions.
- Close the remaining visibility gaps: restrict or document task, pin, and anchor metadata routes, and publish an irreversible-publication policy.
- Add UCAN revocation or blocklisting, with an emergency compromised-key runbook.
- Add mutation-aware GraphQL auth before GraphQL becomes a public write API surface.
- GraphQL mutations require a verified signer (shipped, #87). Keep the surface limited to the agent-task queue; do not expose an owner-gated repo-write mutation over GraphQL without its owner-gate and REST parity. The `every_graphql_mutation_has_its_gate` fence forces any new mutation to be classified in its table (discovered from the schema, so none can slip in unlisted); the owner-gate and REST parity themselves stay the INV-1 review check, not something the fence proves.
- Harden peer registration and outbound peer calls against SSRF and peer-list poisoning.
- After all live peers upgrade, enable signed peer writes by default.
+1 -1
View File
@@ -92,7 +92,7 @@ Fixed or staged in this pass:
Live-network blockers to prioritize:
- GraphQL POST is still open for compatibility; GraphQL mutations should get mutation-aware auth before it becomes a public write API surface.
- GraphQL mutations now require a verified signer bound to the acting DID (#87), and the surface stays limited to the agent-task queue, so there is no public repo-write GraphQL API to gate. Queries remain open by design. (Resolved; kept here as posture, not a live blocker.)
- Push authorization is still not capability-complete. A valid DID signature is authentication, not authorization. Owner checks are now enforced on every branch, protected or not (`GITLAWB_ENFORCE_OWNER_PUSH`, on by default); what remains is that a UCAN `git/push` capability is not yet honored, so a delegated or CI key cannot push.
- UCAN chain validation is incomplete and UCAN revocation/blocklisting is not implemented as an operator feature.
- Private repository reads are not enforced. `is_public` and `GITLAWB_PUBLIC_READ` exist, but per-repository private-read behavior is not wired.