fix(gl): status-check the client read/write surfaces so a node denial surfaces as an error, not a fake result (#123)#186
Conversation
gl read tools parsed node responses with `.json().await?` and never checked the HTTP status, so a visibility-gated 404/5xx body was handed back to the agent as the requested resource with Ok (INV-8 break). Add a shared `read_json` helper (http.rs): surfaces a non-2xx as an Err carrying the sanitized node message (INV-6, reusing sanitize_node_msg, now pub(crate)), and errors on a garbage 2xx body instead of a silent empty success. Route repo_get / repo_commits / repo_tree through it. Denial-path tests drive a gated 404 through call_tool and assert Err (not a fabricated repo / empty commit list); verified load-bearing by reverting the wiring to red.
Route `gl repo commits` / `gl repo labels` (repo.rs) and `gl pr`
list/view/diff/comments (pr.rs) through read_json, so a gated 404/5xx
surfaces as an error instead of an empty render ("No commits", "No pull
requests", "No diff"). The pr.rs mutation commands already status-check
and are left as-is. Denial-path test per read arm.
MCP pr_list / pr_view / pr_diff, webhook_list (the client half of #94), and issue_list; CLI `gl issue list`/`comments` and `gl webhook list`. All route through read_json so a gated denial surfaces as an error, not a fabricated or empty result. Denial-path tests for the MCP arms.
… status (#123) cmd_info: sanitize the node-advertised message before it reaches the terminal (INV-6), preserving the existing bare-404 wording. gl status: a gated 404 on the PR/issue dashboard sections now renders "unavailable (<status>)" instead of silently "no open PRs" (INV-8), without hard-failing the multi-section command. Classification for the rest of the U6 sweep (recorded, none converted): whoami repo-count lists the caller's OWN repos (best-effort, not a withheld-resource read); task/cert/bounty/peer reads are not repo-confidentiality surfaces. gl mutation commands already status-check.
Completeness-sweep catch: /api/v1/repos/{owner}/{name}/certs is a
repo-scoped gated read that rendered a denial as an empty cert list.
Route it through read_json.
…l tests (#123) Code review caught a missed gated read arm: `gl bounty list --repo o/n` and MCP bounty_list hit the authorize_repo_read-gated /api/v1/repos/{o}/{n}/bounties and rendered a denial as a silent-empty success (INV-8). Route both through read_json. Add denial-path tests for the converted-but-untested arms the review flagged: bounty (repo-scoped), cert (was zero coverage), MCP pr_view / pr_diff, CLI issue list, CLI webhook list.
…123) The gl status PR/issue sections rendered a gated 404 via inline println! that no test could reach (the sections only run with a gitlawb remote, and output isn't capturable). Extract the decision into a pure section_unavailable_line helper: non-2xx -> 'unavailable (<status>)' (INV-8), 2xx -> None (caller renders), transport error -> None (degrade silently, R5). Tested all three branches; the 404 case is load-bearing (proven RED when the guard is neutered).
Add read_json edge tests (empty 2xx body -> Err; non-2xx JSON without a message key -> 'request failed' fallback) and a denial test for the standalone `gl pr comments` arm. Every read_json-routed denial test is now load-bearing: mutating read_json to the pre-fix bare-parse turns all 18 arm denial tests + 7 helper error tests RED while the 271 happy-path tests stay green.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR adds centralized status-aware JSON handling, routes CLI and MCP responses through it, sanitizes node error messages, and adds tests ensuring denied responses become errors or unavailable status sections instead of fabricated or empty results. ChangesHTTP denial handling
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CLI_or_MCP
participant NodeClient
participant read_json
CLI_or_MCP->>NodeClient: request resource
NodeClient-->>read_json: HTTP response
read_json->>read_json: validate status and sanitize message
read_json-->>CLI_or_MCP: parsed JSON or error
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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
…ching route can't pass vacuously (#123)
…er-error tests so a non-matching route can't pass vacuously (#123)
…nc, star, agent, protect, changelog) so a non-matching route can't pass vacuously (#123)
…ty show, issue show/close/comment, webhook create/delete) so a non-matching route can't pass vacuously (#123)
…l isn't rendered as success (#123)
…ndered as a successful tool result (#123)
…e denial isn't rendered as success (#123)
…son so a node error surfaces its status not a vague fallback (#123)
… per-helper denial tests that go RED without the conversion (#123)
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.
2 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.INV-8: 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 / INV-8), 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 marks non-success PRs/issues sections as “unavailable” and includes the HTTP status.Tests