fix(gl): status-check the client read/write surfaces so a node denial surfaces as an error, not a fake result (#123) - #186
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR introduces centralized, status-aware JSON response handling through ChangesHTTP denial handling
Estimated code review effort: 4 (Complex) | ~70 minutes Sequence Diagram(s)sequenceDiagram
participant Handler as CLI or MCP handler
participant NodeClient
participant read_json
Handler->>NodeClient: Request resource
NodeClient-->>read_json: HTTP response
alt 2xx
read_json->>read_json: Parse JSON
read_json-->>Handler: Parsed value
else non-2xx
read_json->>read_json: Read capped and sanitize message
read_json-->>Handler: Error with status and message
end
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
crates/gl/src/cert.rs (1)
207-229: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSame missing mock-hit verification as the other new denial tests.
No
.expect(1)/m.assert()on the mock, so an unmatched request (mockito's default 501 for non-matches) would also makeread_jsonreturnErr, letting the test pass even if the anchored regex never actually matched the pathcmd_listbuilt.♻️ Proposed fix
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"); + _m.assert_async().await;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/gl/src/cert.rs` around lines 207 - 229, Ensure the cmd_list_surfaces_denial_not_empty test verifies that its mock was hit exactly once by calling the mockito mock’s assert/expect(1) method after cmd_list completes, so unmatched requests cannot satisfy the test.crates/gl/src/issue.rs (1)
739-756: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSame missing mock-hit verification as the analogous bounty/cert denial tests.
Since mockito returns 501 for any unmatched request, and 501 is also non-2xx,
read_jsonstill errors regardless of whether the regex actually matched the pathcmd_listproduced — add.expect(1)/m.assert()so the test genuinely proves the gated issues-list path is exercised.♻️ Proposed fix
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"); + _m.assert_async().await;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/gl/src/issue.rs` around lines 739 - 756, Add mock-hit verification to cmd_list_surfaces_denial_not_empty by retaining the mock handle and calling its assert method after cmd_list completes (or configuring it with expect(1)), ensuring the intended issues-list request matched the configured route.crates/gl/src/bounty.rs (1)
550-571: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDenial test doesn't verify the mocked endpoint was actually hit.
The mock has no
.expect(1)/m.assert(). Per mockito's own docs, Any calls to the Mockito server that are not matched will return 501 Not Implemented. Since 501 is also non-2xx,read_jsonwould still returnErreven if the URL built bycmd_listdidn't match the regex at all — so the test can pass without actually proving the repo-scoped bounties path is constructed/gated correctly.♻️ Proposed fix
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" ); + _m.assert_async().await;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/gl/src/bounty.rs` around lines 550 - 571, Strengthen cmd_list_repo_scoped_surfaces_denial_not_empty by configuring the Mockito expectation to require exactly one request and explicitly asserting the mock was matched after cmd_list completes. Use the existing _m mock handle’s expectation/assertion API so the test fails if the repo-scoped /repos/alice/secret/bounties endpoint is not requested.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/gl/src/bounty.rs`:
- Around line 550-571: Strengthen cmd_list_repo_scoped_surfaces_denial_not_empty
by configuring the Mockito expectation to require exactly one request and
explicitly asserting the mock was matched after cmd_list completes. Use the
existing _m mock handle’s expectation/assertion API so the test fails if the
repo-scoped /repos/alice/secret/bounties endpoint is not requested.
In `@crates/gl/src/cert.rs`:
- Around line 207-229: Ensure the cmd_list_surfaces_denial_not_empty test
verifies that its mock was hit exactly once by calling the mockito mock’s
assert/expect(1) method after cmd_list completes, so unmatched requests cannot
satisfy the test.
In `@crates/gl/src/issue.rs`:
- Around line 739-756: Add mock-hit verification to
cmd_list_surfaces_denial_not_empty by retaining the mock handle and calling its
assert method after cmd_list completes (or configuring it with expect(1)),
ensuring the intended issues-list request matched the configured route.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7f7b1bd9-cb1b-46ef-a606-2bd697fa0fea
📒 Files selected for processing (10)
crates/gl/src/bounty.rscrates/gl/src/cert.rscrates/gl/src/http.rscrates/gl/src/issue.rscrates/gl/src/mcp.rscrates/gl/src/pr.rscrates/gl/src/repo.rscrates/gl/src/status.rscrates/gl/src/sync.rscrates/gl/src/webhook.rs
beardthelion
added
crate:gl
beardthelion
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/gl/src/changelog.rs (1)
57-68: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winMain changelog fetch still bypasses
read_json/sanitize_node_msg.The status check here is correct (no fabricated-success risk), but the error message is taken raw from
body["message"]without the sanitizationread_jsonnow applies elsewhere. Converging this call ontocrate::http::read_jsonwould close the same terminal-injection gap already fixed forsync triggerand the line-45 fetch in this same function.♻️ Suggested consolidation
- let resp = client - .get(&url) - .await - .context("failed to connect to node")?; - - let status = resp.status(); - let body: Value = resp.json().await.unwrap_or_default(); - - if !status.is_success() { - let msg = body["message"].as_str().unwrap_or("unknown error"); - anyhow::bail!("changelog failed ({status}): {msg}"); - } + let body = crate::http::read_json( + client.get(&url).await.context("failed to connect to node")?, + "changelog", + ) + .await?;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/gl/src/changelog.rs` around lines 57 - 68, Update the main changelog fetch in the surrounding function to use crate::http::read_json, reusing its sanitize_node_msg handling for the response error message while preserving the existing non-success status check and changelog failure behavior. Remove the direct resp.json parsing path so this fetch follows the same sanitized handling as the other calls.crates/gl/src/register.rs (1)
58-67: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
gl register's error path bypasses the newread_json/sanitize_node_msgsanitization.This function still hand-rolls status/message extraction instead of using
crate::http::read_json, so a hostile node'smessagefield is not passed throughsanitize_node_msgbefore being embedded in the error and printed to the terminal — unlike the now-convertedagent_registerMCP tool that hits the same/api/registerendpoint. Worth converging for consistency and to close the control-char/ANSI-injection gap already fixed elsewhere in this PR.♻️ Suggested consolidation
- let status = resp.status(); - let payload: Value = resp.json().await.context("invalid JSON response")?; - - if !status.is_success() { - let msg = payload - .get("message") - .and_then(|v| v.as_str()) - .unwrap_or("unknown error"); - anyhow::bail!("registration failed ({status}): {msg}"); - } + let payload = crate::http::read_json(resp, "registration").await?;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/gl/src/register.rs` around lines 58 - 67, Update the registration flow in crates/gl/src/register.rs to use crate::http::read_json for the /api/register response instead of manually parsing the status and message from resp. Preserve the existing registration failure behavior while ensuring error messages pass through sanitize_node_msg via the shared helper, matching agent_register.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/gl/src/changelog.rs`:
- Around line 57-68: Update the main changelog fetch in the surrounding function
to use crate::http::read_json, reusing its sanitize_node_msg handling for the
response error message while preserving the existing non-success status check
and changelog failure behavior. Remove the direct resp.json parsing path so this
fetch follows the same sanitized handling as the other calls.
In `@crates/gl/src/register.rs`:
- Around line 58-67: Update the registration flow in crates/gl/src/register.rs
to use crate::http::read_json for the /api/register response instead of manually
parsing the status and message from resp. Preserve the existing registration
failure behavior while ensuring error messages pass through sanitize_node_msg
via the shared helper, matching agent_register.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3f429da4-22e0-47c0-956d-ea60ced0434a
📒 Files selected for processing (16)
crates/gl/src/agent.rscrates/gl/src/bounty.rscrates/gl/src/cert.rscrates/gl/src/changelog.rscrates/gl/src/issue.rscrates/gl/src/mcp.rscrates/gl/src/peer.rscrates/gl/src/pr.rscrates/gl/src/protect.rscrates/gl/src/register.rscrates/gl/src/repo.rscrates/gl/src/star.rscrates/gl/src/sync.rscrates/gl/src/task.rscrates/gl/src/visibility.rscrates/gl/src/webhook.rs
✅ Files skipped from review due to trivial changes (1)
- crates/gl/src/star.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/gl/src/bounty.rs
- crates/gl/src/cert.rs
- crates/gl/src/repo.rs
- crates/gl/src/pr.rs
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P2] Status-check the CLI bounty-stats response
crates/gl/src/bounty.rs:387
cmd_statsstill deserializes/api/v1/bounties/statswithunwrap_or_default()and never checks the HTTP status. Consequently, a 403/404/500 (including a non-JSON body) is printed as a successfulBounty Statsresult with every counter set to zero. This is the same denial-as-fake-result behavior the PR fixes elsewhere, and the MCP twin already routes this endpoint throughread_json. Route this CLI request throughread_jsonand add a non-2xx regression test. -
[P2] Bound error bodies in the shared response helper
crates/gl/src/http.rs:204
The newread_jsonhelper callsresp.json()on every non-2xx response before it extracts and truncatesmessage. That buffers and parses the complete response, so a configured or malicious node can stream an arbitrarily large valid JSON error body and exhaust the CLI/MCP process's memory across the newly migrated calls. The 200-character display cap does not bound allocation, andsync::read_body_cappedalready implements the required capped-read pattern for the same hostile-node error path. Read a small capped body before parsing its message, then add a large-error-body regression test.
F1: cmd_stats deserialized /api/v1/bounties/stats with unwrap_or_default() and never checked the HTTP status, so a 403/404/5xx (or non-JSON body) printed a successful Bounty Stats with every counter zeroed — the denial-as-fake-result class this PR fixes elsewhere. Route it through read_json (like the sibling bounty commands and the MCP twin). F2: read_json called resp.json() on every non-2xx response, buffering and parsing the whole body before the 200-char DISPLAY cap applied, so a hostile node could stream an arbitrarily large valid JSON error and exhaust the process's memory. Read a capped body (read_body_capped, 8 KiB — the same bound the sync error path uses) and best-effort extract message from it; a message-less or non-JSON body still falls back to the status alone. read_body_capped is now pub(crate). RED->GREEN: cmd_stats_surfaces_denial_not_fake_result (403 -> Err, was Ok+zeros). read_json_bounds_the_error_body_read (a message placed past the cap is ABSENT from the error; was surfaced by the full parse). All existing read_json + stats tests stay green; gl suite 339, fmt + clippy clean.
|
Both addressed on Stats status check (F1). Bounded error body (F2). Each fix is RED->GREEN with the guard reverted to confirm it is load-bearing. |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
- [P2] Apply the capped/sanitized error path to the remaining manual status handlers
crates/gl/src/bounty.rs:193
This command (and other changed manual handlers such asagent,issue,pr,repo,register,star,protect,visibility, andwebhook) callsresp.json()before inspecting a non-2xx status. A configured or malicious node can therefore return an arbitrarily large valid JSON error and makeglbuffer it in full, and several of these paths print itsmessagewithout the new terminal-control sanitization. That leaves the exact hostile-error-body failure mode thathttp::read_jsonnow documents and caps. Check the status before a bounded error read (or route these arms through the shared helper) and retain the command-specific success handling.
…status gate Adversarial coverage for the #186 read_json conversions: - peer cmd_add / cert cmd_show: a denied `GET /` node-info now surfaces as an Err instead of being parsed as success (INV-8). RED without the read_json conversion. - star cmd_add: a hostile 500 carrying terminal-control + bidi bytes and a long message reaches the terminal neither verbatim (sanitized) nor unbounded. - new tests/no_parse_before_status.rs: a source-level completeness gate that fails if any converted handler reintroduces a resp.json()-before-status bypass. Proven load-bearing (reverting one converted site flags it RED). Pre-existing bypasses in files this PR did not touch (init/mirror/profile/quickstart/whoami) are known debt, out of this gate's scope.
The #186 restructure of cmd_info kept the bespoke 404 'repository not found' message ahead of read_json and routed every other status through it. Pin both: a 404 still yields the bespoke text; a 500 yields read_json's 'repo info failed (500)'.
|
Done at Routed every remaining manual-status handler through While scoping it I found two sites the finding didn't name that are worse than the P2: Bespoke branches preserved: the To stop this class from creeping back I added
One scope note: the gate surfaced the same bypass idiom in @jatmn ready for another look. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/gl/src/star.rs`:
- Around line 343-369: Update the test around cmd_add to retain the created mock
handle and assert it was hit after the command completes. Ensure the assertion
verifies the /star mock received the expected PUT request before evaluating the
sanitized error contents, so a route mismatch cannot satisfy the test.
In `@crates/gl/tests/no_parse_before_status.rs`:
- Around line 23-37: Add "task" to the CONVERTED list in
no_parse_before_status.rs so the gate covers the converted task handler and
detects parse-before-status regressions in task.rs.
- Around line 50-56: Update the parse detection in the test around the
`.json().await` check to structurally identify direct Response::json calls,
including turbofish arguments and split-line method chains, while excluding
calls routed through read_json. Preserve the existing status-window validation
and any explicitly intended exceptions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a5404c35-a02e-4774-a405-afc3816b310c
📒 Files selected for processing (14)
crates/gl/src/agent.rscrates/gl/src/bounty.rscrates/gl/src/cert.rscrates/gl/src/changelog.rscrates/gl/src/issue.rscrates/gl/src/peer.rscrates/gl/src/pr.rscrates/gl/src/protect.rscrates/gl/src/register.rscrates/gl/src/repo.rscrates/gl/src/star.rscrates/gl/src/visibility.rscrates/gl/src/webhook.rscrates/gl/tests/no_parse_before_status.rs
…ile mock hit (#186) Address the three CodeRabbit threads on the #186 client-read-status work, all INV-21 (load-bearing / completeness) findings. no_parse_before_status.rs: the gate hand-listed the converted handlers, and the list had already drifted (task, mcp, and sync all call read_json but were absent, so a parse-before-status regression in them would not fail the gate). Derive the scanned set from the source tree instead: every src/*.rs that references read_json (minus http.rs, the definition site), so a converted handler auto-enrolls. The known out-of-scope debt (init/mirror/profile/ quickstart/whoami) uses raw resp.json().await and no read_json, so the derivation excludes it without a skip list. Detection now anchors on .json( and .json::<, catching turbofish resp.json::<Value>().await and split-line chains the old bare .json().await substring missed, still keyed on an is_success() that lands after the parse so status-first probes stay green. star.rs: the cmd_add hostile-response test never asserted the mock was hit, so a route mismatch would serve mockito's fallback and satisfy every assertion without exercising the sanitization. Add .expect(1) and _m.assert_async().
|
@jatmn the manual-handler status finding was already addressed on the current head (e3e21e0); your review was against 85dfbd2, before that conversion pass landed. On the current head every handler you named routes its error read through The only raw parses left are three in The three CodeRabbit threads on this head are also resolved: the completeness gate now derives its scanned set from |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P2] Preserve the fail-soft peer URL fallback
crates/gl/src/peer.rs:96
gl peer addonly needsGET /to improve the URL it advertises; the existingpublic_urlfallback intentionally uses--nodewhen that optional lookup has no usable value. Routing the lookup throughread_jsonnow returns before the remote/api/v1/peers/announcerequest whenever the local root endpoint returns a JSON 4xx/5xx, even though that remote announcement can still succeed with the fallback URL. This regresses the command's degraded-mode behavior and contradicts the PR's stated decision to leave the peer-add public-URL lookup fail-soft. Keep the remote announcement reachable on an unavailable local node-info response, while retaining status checking for the actual announce request. -
[P2] Do not make the post-fetch certificate hint fatal
crates/gl/src/cert.rs:140
The certificate has already been fetched and printed when this ancillaryGET /runs; it is used only to compare the certificate's node DID with the current node and print offline-verification guidance. A JSON 4xx/5xx response previously left the DID empty and emitted the mismatch/degraded hint, butread_json(...)?now abortsgl cert showafter its partial certificate output and omits that guidance. The PR explicitly identifies this node-DID hint as a diagnostic/fail-soft path that should stay unchanged. Handle an unavailable node-info lookup as a degraded comparison instead of failing the completed certificate-show operation.
The local GET / in cmd_add only improves the URL we advertise; a response-level failure now falls back to --node with a stderr note instead of aborting before the announce, which keeps its fail-closed read_json check. Guards pin the fallback (500 and non-JSON body), the announce denial, and the public_url success path.
The post-print GET / only feeds the node-DID comparison hint; a response-level failure now prints an explicit could-not-compare note with the offline-verification guidance and the command completes. The certificate fetch keeps its fail-closed read_json check, pinned by a denied-cert guard whose GET / mock asserts expect(0).
…ricate a DID mismatch (#186) Review findings on the fail-soft restoration: the follow-up local peer-list add printed an unconditional success line over an ignored POST result (now status-gated, with a stderr note on failure, still fail-soft); a 200 node-info body carrying no usable DID fell into the empty-string comparison and printed a fabricated 'does not match ()' warning (now routed to the could-not-compare hint via the extracted did_check_report helper, whose unit tests pin all four outcomes); the restructured mismatch branch gained an end-to-end test; the denied- announce test's info mock expectation is now asserted.
|
Both addressed on Fail-soft peer URL fallback. The local Non-fatal cert hint. The boundary, named: response-level failures degrade (any non-2xx, a 2xx with a malformed body, and for cert show a 2xx carrying no usable Hardening in the same push: the follow-up local peer-list add no longer prints "Added to local peer list." over an ignored POST result (status-gated, still best-effort), and the cert DID verdict selection moved into a small |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
- [P2] Keep the regression gate's converted-handler set independent of
read_jsonusage
crates/gl/tests/no_parse_before_status.rs:33
The test chooses files to inspect only when their current text containsread_json. A full regression of a converted single-call handler therefore removes that marker and drops the file from the scan: for example, restoringregister.rsto its previousresp.json().awaitfollowed byis_success()leaves noread_jsonreference in that file, while the test still has other handlers and passes. This defeats the stated guard against exactly the parse-before-status regression the PR is adding it to catch. Keep an expected set of converted handler files (and assert it is scanned), or derive that set from an independent source, so removal of the final helper call fails the test.
…ail closed both ways The gate derived its scanned set from read_json usage, but a converted handler with a single node call (register.rs) reverted to resp.json().await loses the marker, drops out of the derived set, and its bypass goes unscanned. Pin the converted surface as CONVERTED_IN_186 and assert the derived read_json set EQUALS it, failing closed in both directions: a pinned file that drops out was reverted off read_json (RED), and a file using read_json that is not pinned is an unenrolled conversion whose own later revert would escape the same way (RED until it is added). Equality extends the protection to handlers converted after #186, not just the original sixteen. Verified by execution: reverting register.rs's one read_json call trips the deconverted assert; a new read_json handler left unpinned trips the unpinned assert; a partial revert in a file that keeps read_json is still caught by the offender scan; and the pre-change gate passed on the register.rs revert.
Fixed on Verified by execution: reverting register.rs's one call trips the deconverted assert, an unpinned |
ad0e35a to
86e5b4a
Compare
… surfaces as an error, not a fake result (#123) Convert the gl CLI + MCP read/write surfaces to status-check the node's response before parsing, so a denial (401/403/404) or a degraded 5xx surfaces as an error instead of being rendered as an empty list or a fake success. The shared http::read_json helper checks the status first and bounds the error-body read (a hostile node can't stream an arbitrarily large error body to exhaust the client), replacing the prior parse-before-status pattern across agent, bounty, cert, changelog, issue, mcp, peer, pr, protect, repo, star, status, sync, task, visibility, and webhook. A no_parse_before_status regression gate locks the converted set in. Rebased onto current main (squash of the prior branch). main had independently gated several of these same surfaces, so the overlapping arms are reconciled to keep BOTH fixes rather than let either regress: - repo::cmd_label_list keeps main's get_maybe_signed (read-visibility gating: public repos stay anon-listable, a private-repo owner signs) carrying read_json on top; the PR's get_authed is dropped so the visibility gate holds. - mcp webhook_list keeps main's get_signed and its signing-keypair-DID owner resolution (NOT resolve_owner's node DID) — list_webhooks is owner-gated AND auth-required, so the PR's plain get()+resolve_owner would have 401'd then 403'd — with read_json layered for the bounded error read. - webhook create/list/delete keep main's get_signed + resolve_owner_repo_pair structure and its tests, with read_json swapped in for the manual resp.json() so the error body is bounded. main's superseded resolve_owner helper and its duplicate tests are dropped. gl: 388 unit tests + the no_parse_before_status gate green; fmt + clippy clean. Rebased again onto current main (2026-08-11), which had moved 238 commits. main independently landed read_body_capped and sanitize_node_msg into http.rs, so this branch's duplicate definitions in sync.rs are dropped and read_json is layered on main's helpers instead. main's peer.rs also grew its own announce handling (remote_announce_failure, local_add_report/local_add_refusal, capped read, sanitized message) which supersedes this branch's read_json conversion there; main's side is kept whole and only peer::cmd_list, which main had not gated, still takes read_json. fix(gl): report why the node-DID comparison was skipped in cert show The rebase onto main dropped did_check_report, since main's cmd_show had been rewritten to do real Ed25519 verification and collapsed every node-info failure into one "could not fetch current node info" NOTE. Put the tri-state reporting back on top of main's structure: the signature verdict and the --verify issuer anchoring are untouched, but the node-DID comparison now names the reason it could not run and points at the offline verification path, and "carried no DID" stays distinct from "the lookup failed". The lookup goes through read_json so a denial yields a reason rather than an opaque None, and it stays fail-soft: a node-info hiccup must not turn a successfully displayed certificate into an error exit. The DID extraction is split out as did_from_node_info so its guard is testable. An empty did rejects rather than flowing into the comparison, where it would print a mismatch WARNING against a DID the node never claimed. That guard had no coverage: removing it leaves every pre-existing test green and only the new one goes red. fix(gl): close two completeness-gate escapes; surface the node's error key Review of the rebased branch found the #186 completeness gate could not fail on the two shapes that matter most. Both were reproduced by seeding the regression and watching the gate stay green. An unguarded parse escaped entirely. The rule flagged a parse followed by .is_success(), which describes one bad ordering and says nothing about a read with no status check at all -- the more dangerous revert, since it renders a denial body as a result. Inverted it to require a status check above the parse, which also covers a check spelled as_u16() >= 400. The lookback is 24 lines because sync.rs's hand-rolled Trigger arm puts its .status() 19 lines up; it should shrink if that arm is ever routed through read_json. Membership in the scanned set keyed on the text "read_json" rather than a call site, so a handler fully reverted off read_json kept its membership on a leftover comment. Still-derived meant the pinned-vs-derived equality never fired and the deconversion shipped green, dropping the cap and the sanitizer. Now counts call sites on non-comment lines. read_json read only `message`, but the task API answers with `error` alone ({"error":"task not found"} in gitlawb-node/src/api/tasks.rs), so every gl task denial rendered as the generic "request failed". Falls back to `error` through the same sanitizer and cap; `message` still wins when both are present, which is the shared AppError envelope. sync.rs already read both. Also covers cmd_show --verify, which had no test at all: the issuer anchoring is the security-bearing half of the command, since a valid signature only proves the certificate is self-consistent and a hostile node can self-sign one. The certificates carry real signatures, otherwise --verify would bail before reaching the anchoring and the assertions would be vacuous. Each fix verified by reverting it: both gate escapes go red, dropping the error fallback fails the two new read_json tests, and removing the anchoring block fails three of the five --verify tests.
…enial Closes the two items left open on the last review round. The bounty-stats denial tests were already load-bearing: breaking either mock's route regex turns both red, because mockito answers an unmatched route with 501 and neither test's asserted status is 501. That proof is incidental to mockito's fallback status, though, so a reworded assertion would silently drop it. Assert the hit directly with expect(1) + assert_async. MCP webhook_list keeps get_maybe_signed rather than aligning with the CLI's get_signed. An agent driving the MCP tool may legitimately have no local identity, and issuing the request so the node's own denial reaches the caller is more useful than failing client-side before the node is asked. That is only defensible if the denial actually surfaces, so pin it: no identity plus an explicit owner must issue an UNSIGNED request and turn the node's 401 into an error carrying the status and the node's reason, not an empty webhook list. The new test points at an empty temp dir instead of passing None. None falls back to the ambient ~/.gitlawb/identity.pem, which exists on a dev box and not on CI, so the request would have been signed here and unsigned there; the first run of this test failed exactly that way before the dir was pinned. Verified by execution: swapping get_maybe_signed for get_signed reds the new test on "must name the status", and breaking either stats mock route reds its test. gl: 412 unit tests + the no_parse_before_status gate green; fmt and clippy clean.
The comments carried internal shorthand identifiers that resolve to nothing for a reader outside this repo. Replace each with the property it names: a denial must surface as an error rather than render as an empty or fabricated result, and an error body must be read under a cap and sanitized before it reaches the terminal. Comment-only; no behavior change. Verified after the edit because the completeness gate reads source text to decide which handlers it scans: reverting a converted handler to a bare .json().await still turns it red. gl: 412 unit tests green, fmt and clippy clean, cargo check --workspace --locked clean.
86e5b4a to
21ba974
Compare
|
@jatmn rebased onto current main and closed both items from your last round. The rebase. main had moved 238 commits and had independently landed The risk in a rebase that size is the completeness gate quietly going vacuous, so I checked rather than trusted the green: reverting The bounty-stats mock-hit finding does not hold, and here is the check. Both tests already pin the hit. I broke each mock's route regex in turn and both went red. Mockito answers an unmatched route with a Not Implemented status, which is not the status either test asserts, so On One note on that test, since it bit me first: it points at an empty temp dir rather than passing Also stripped some internal shorthand from the comments in |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
- [P3] Make the completeness gate require a full non-success guard
crates/gl/tests/no_parse_before_status.rs:168
has_status_check_abovetreats any nearby.status()as proof that the later JSON parse is guarded, even if that check only handles one status. For example,agent.rskeeps a 404-onlyresp.status()branch beforeread_json; replacing that helper call withresp.json().await?would make a 500 JSON response render as an empty agent list again, but this test would pass because it sees the 404 check in its 24-line lookback. The same pattern exists in the agent-show and repo-info special-404 paths. Require an all-non-2xx exit for the parsed response, or add a mutation regression covering this special-404 escape, so the advertised status-before-parse fence cannot be bypassed.
|
let payload: Value = resp.json().await.unwrap_or_default();A non-JSON body becomes Worth deciding deliberately whether it rides along here or lands after. Two arguments for folding it in: it is the first-run wizard, so a false success there is the worst placement, and the new Separately, and not a request to change this PR: the |
|
@coderabbitai full review |
|
A 404-only equality upstream counted as status evidence, so reverting a read_json call behind agent.rs's special-404 hint to resp.json().await? kept the fence green while a 500's denial body rendered as a result. Require .is_success() or an as_u16() range comparison in the lookback, and pin the distinction with a mutation regression that performs that exact revert on the real agent.rs source.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
The previous completeness-gate issue is addressed on the current head. I found one merge-readiness issue that remains before this is ready.
Merge readiness
-
[P2] Remove the test diagnostics that keep the aggregate CodeQL check red
crates/gl/src/cert.rs:497
The aggregateCodeQLcheck reports seven new high-severity “Cleartext logging of sensitive information” alerts at this assertion and lines 540, 584, 667, 708, 803, and 862. They all have the same root cause: these#[cfg(test)]assertions format the completeResult<()>through{result:?}or{got:?}. CodeQL traces a value associated withresolve_cert_idinto the command error and then treats the assertion's panic diagnostic as a cleartext logging sink. The tests use controlled mock data and certificate IDs are public identifiers, so this does not look like production secret exposure; the seven alerts are repeated instances of one test-diagnostic pattern rather than seven distinct vulnerabilities. CI is nevertheless red on the current head.The most direct code-side fix is to replace the raw-result interpolation at all seven locations with static assertion context, for example
assert!(result.is_ok(), "cert show must complete despite denied node info"). The failing test name, source line, and message will still identify the scenario without rendering the traced result value. Then rerun CodeQL and confirm that all seven alerts and the aggregate failure clear. If retaining the raw diagnostics is intentional, classify these alerts as false positives or narrowly configure CodeQL for this test-only sink instead; please do not disable the production cleartext-logging query globally.
…186) CodeQL reported seven high-severity "Cleartext logging of sensitive information" alerts at cert.rs:497, 540, 584, 667, 708, 803 and 862, and the aggregate check has been red on the branch since. All seven are #[cfg(test)] assertions that format a whole Result<()> through {result:?} or {got:?}. CodeQL traces a value associated with resolve_cert_id into the command error and treats the assertion's panic diagnostic as a logging sink. The alerts are test-only. The single mod tests opens at cert.rs:328 and runs to EOF, so all seven sit inside it, and the traced certificate id is already printed unconditionally in production at cert.rs:145 because it is a public identifier. That makes these repeated instances of one test-diagnostic pattern rather than seven distinct vulnerabilities. Each message now carries static context naming the scenario instead. The failing test name and source line still identify what broke, and every assertion predicate is unchanged, so nothing is weakened into vacuity. The production cleartext-logging query stays enabled: nothing is suppressed or reconfigured. One {got:?} at cert.rs:900 is deliberately left alone. CodeQL did not flag it, its value comes from did_from_node_info over hardcoded literal JSON with no resolve_cert_id taint, and the interpolation is load-bearing there because it names which of three loop cases failed. Verified: cargo test -p gl --bin gl cert, 20 passed. fmt clean.
) The 2026-08-11 rebase squashed the branch and lost nine tests. Counting `async fn cmd_add_` in peer.rs per head: 6 at 06863da, 3791c40, 9ed2947 and ad0e35a, then 0 at 21ba974 and 0 at fba9003. The production fail-soft at peer.rs:173-180 survived; only the tests pinning it went. That matters because the rule this PR's own round 3 established is that a carve-out with no test that goes red when someone makes it fatal is an incomplete sweep, not a documented decision. The peer-add public-URL lookup has been in exactly that state for three weeks. Recovered from e179cd2, not 5eff606: the earlier commit carries only four of the seven items, while e179cd2 has all of them plus three same-class denial tests for cmd_list, cmd_ping and cmd_resolve that were lost the same way. Neither commit is an ancestor of HEAD. Nothing needed adapting. cmd_add's signature and its node-info block are byte-identical between the two heads. The announce arm has since moved to remote_announce_failure, which still formats the status the denied-announce test asserts on, and the local-add arm has moved to local_add_report, which still returns Ok either way, so both local-add tests hold. The named import list gains cmd_list, cmd_ping and cmd_resolve. Proven load-bearing rather than assumed: rewriting the fail-soft match into a fatal `?` turns cmd_add_announces_fallback_url_when_node_info_denied and cmd_add_falls_back_on_malformed_node_info red, and leaves the pre-existing a_peer_failure_past_the_read_cap_still_reports_its_status green. That last point is the reason these are not redundant with what was already there: the surviving test drives a 503 whose body is past the read cap, so it pins the cap and ordering interaction, not the fallback. Verified: cargo test -p gl --bin gl peer, 25 passed. fmt clean.
… file (#186) `gl status` was hardened in this PR with section_unavailable_line and trust_line, so a denied section prints "unavailable ({status})" instead of rendering as empty. But it was hardened by hand rather than through read_json, so status.rs carried no marker, never entered the completeness gate's derived set, and was never scanned. Deleting the helper call would have restored the denial-as-empty bug with the gate still green: the one file written to fix this bug was the one converted file the gate did not cover. The two repo-section parses now route through read_json and status.rs is pinned in CONVERTED_IN_186. Exactly two source lines change. section_unavailable_line takes the response by reference and reads only its status, so read_json sits underneath it with no restructuring, and the outer if-let shape still stops one denied section from aborting the others. All 35 status tests pass unchanged, including the eight denial tests and their exact rendered strings. trust_line stays hand-rolled on purpose. It needs a three-way split (2xx, exactly 404, other non-2xx) that read_json collapses into a single Err, and it already carries a literal status check the scan can see. A second pinned const naming hand-rolled guards was considered and rejected. It would enforce only that a symbol is spelled somewhere in the file: deleting the println while keeping the call would stay green. A structural anchor already exists, which is read_json, and the repo's own guard-design note is explicit that a name-keyed filter must never be the membership predicate when one does. Proven, not assumed. Pinning status.rs before converting it turned the gate red naming the file. Reverting even one of the two read_json calls afterwards turns it red again, on the unguarded check rather than the pin, because the issues parse has no full status guard in its lookback window. The one gap worth naming: deleting both section_unavailable_line call sites while keeping read_json still scans green, and the denied section would then print nothing rather than a false "no open pull requests". Probed it, and clippy catches that case as a dead function under -D warnings, so CI would fail. That backstop holds only while the helper has no other caller. Verified: gate 2 passed, cargo test -p gl --bin gl status 35 passed. fmt clean.
…186) Converting cmd_view's sub-fetches to read_json turned a tolerated 404 into a mid-render abort. The header prints first, then the reviews fetch is bare `?`, so a denial leaves a half-rendered PR, exit 1, and the comments section never runs. Before the conversion the error body parsed, the section was skipped and the rest of the command still rendered, so this is a regression this PR introduced rather than pre-existing behavior. Both sub-fetches now degrade: a denial prints "Reviews unavailable ({status})" or "Comments unavailable ({status})" and rendering continues. That is the shape this PR already invented one file over for gl status's denied sections, so the two commands now behave the same way. read_json stays in use, so pr.rs stays in the gate's derived set and the pinned list is untouched. The header fetch stays fatal. With no header there is nothing to render partially, and softening it would resurrect denial-as-success for the whole command. cmd_view_header_denial_stays_fatal pins that, and it passed before this change as well as after, which is the point: it is the must-not case. mcp.rs's pr_view arm has the same fetch coupling and is deliberately left alone. It emits a single JSON result, so there is no partial-output problem, a whole-call error is the correct fail-closed answer for a machine consumer, and inventing a partial-result shape is a design change nobody asked for. Test-first and observed: cmd_view_continues_when_reviews_are_denied and cmd_view_continues_when_comments_are_denied both failed before the change and pass after. Each asserts the comments mock was hit with expect(1), which is the load-bearing part: the mock can only be hit if rendering continued past the denied fetch. Verified: cargo test -p gl --bin gl, 473 passed. Gate 2 passed. fmt and clippy clean.
|
@jatmn the CodeQL item is fixed on The seven alerts. Your read was right and I checked it independently: the single Nine denial tests were lost in the 08-11 rebase. Counting Proven rather than assumed: rewriting the fail-soft into a fatal
I considered a second pinned const for hand-rolled guards and rejected it. It would enforce only that a symbol appears somewhere in the file, so deleting the Pinning the file before converting it turned the gate red naming it. Afterwards, reverting even one of the two calls turns it red again. The gap worth naming: deleting both helper call sites while keeping
Two things I did not fix here, so they are not silently dropped. The full gl suite, the completeness gate, fmt and clippy are all green locally; CI has the run. Four commits added on top of |
5 participants
What
glclient commands and the MCP tools parsed HTTP responses without checking status, so a node 4xx/5xx (a gated denial, an auth failure, a server error) was deserialized and rendered as success: an empty list, a fabricated "0 stats", the error body printed as if it were data, or a silently swallowed denial. This routes every such surface throughcrate::http::read_json(or an explicit status check where the payload is not JSON) so a denial surfaces as anErrcarrying the node's status and message.A gated denial must not be rendered as a result.
Surfaces
GET /helpers, which reported a vague "node missing DID" on a node error instead of the node's actual status.Left as-is on purpose: the diagnostic/fail-soft paths that intentionally degrade rather than abort (
gl node status/resolvedashboards,gl doctor, thepeer addpublic-URL lookup, the cert-show node-DID hint). Those show a clear degraded state, not fabricated data.Verification
Each surface has a denial test that drives a 4xx/5xx through the real client path and asserts the error carries the node's status. The production conversions were run RED against the pre-fix code (the arm rendered success) and GREEN after. Denial tests carry
.expect(1)mock-hit assertions so a non-matching route can't satisfy them vacuously, and those assertions were confirmed load-bearing (a wrong route drives them RED). Full gl suite green; fmt and clippy clean.Scope
This began as the read-tools fix and grew into the whole-client sweep once it was clear the same denial-as-success bug spanned the CLI, the MCP twin, and the resolve helpers. It is one coherent theme (#123), but it is a lot bigger than the original diff. Happy to split it into pieces if that reviews better.
Summary by CodeRabbit
Bug Fixes
gl statusnow reports “unavailable ()” for non-success PRs/issues responses.Tests