fix(node): gate /hooks and sibling read surfaces leaking private repo metadata (#94)#113
fix(node): gate /hooks and sibling read surfaces leaking private repo metadata (#94)#113beardthelion wants to merge 20 commits into
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:
📝 WalkthroughWalkthroughNode read endpoints now enforce visibility or ownership checks for webhooks, replicas, protected branches, labels, and repo events. CLI read paths for webhooks, repo info, replicas, protected branches, labels, and MCP webhook listing now conditionally sign GET requests when an identity keypair is available. ChangesNode-side endpoint authorization
CLI signed request support
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/gl/src/webhook.rs (1)
157-164: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCheck the webhook-list HTTP status before reading
webhooks.A 401/403/404 JSON error body has no
webhooksfield, so this currently prints “No webhooks” and exits successfully instead of surfacing the authorization failure.Proposed fix
- let resp: Value = client + let resp = client .get_signed(&format!("/api/v1/repos/{owner}/{repo}/hooks")) - .await? - .json() - .await - .context("invalid JSON")?; + .await?; + let status = resp.status(); + let resp: Value = resp.json().await.unwrap_or_default(); + if !status.is_success() { + let msg = resp["message"].as_str().unwrap_or("unknown error"); + anyhow::bail!("webhook list failed ({status}): {msg}"); + } let hooks = resp["webhooks"].as_array().cloned().unwrap_or_default();🤖 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/webhook.rs` around lines 157 - 164, The webhook listing flow in `list_webhooks` is treating error responses as empty results because it parses JSON and reads `resp["webhooks"]` without checking the HTTP status first. Update the `client.get_signed(...).await?` handling to inspect the response status before deserializing or accessing `webhooks`, and surface 401/403/404 as errors instead of falling back to `No webhooks`. Keep the fix localized around the `list_webhooks` response handling and the `webhooks` extraction logic.
🧹 Nitpick comments (1)
crates/gitlawb-node/src/api/mod.rs (1)
177-181: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winGuard both halves of
list_webhooksauthorization.This row only enforces the owner gate. Add a second marker for
authorize_repo_read(so the drift test also catches regressions that reintroduce private-repo existence leaks beforerequire_repo_owner.🧪 Proposed guard hardening
// Bucket A/B hybrid — list_webhooks is read-visibility THEN owner: // authorize_repo_read 404s a non-reader of a private repo, then // require_repo_owner 403s a non-owner of a public one. The // require_repo_owner marker guards the owner half. + (webhooks, "list_webhooks", "authorize_repo_read("), (webhooks, "list_webhooks", "require_repo_owner("),🤖 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/gitlawb-node/src/api/mod.rs` around lines 177 - 181, The `list_webhooks` drift-test row in `api::mod` only checks the owner gate, so it can miss regressions in the read-visibility half. Update the marker set for `list_webhooks` to include both `authorize_repo_read(` and `require_repo_owner(` so the test covers the private-repo existence check as well as the owner check.
🤖 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/gitlawb-node/src/api/events.rs`:
- Around line 66-80: The repo visibility gate in events handling is failing open
because `repo_record` is built with `.ok().flatten()`, so a database lookup
error is indistinguishable from a missing local repo and skips the
`visibility_check` path. Update the `events` flow to preserve and propagate the
lookup error from the local repo fetch, and only allow the ungated gossip-only
behavior when the lookup successfully returns `Ok(None)`. Keep the existing
`visibility_check`/`RepoNotFound` logic in the `repo_record`-based branch so
`api::events` fails closed for local private repos.
---
Outside diff comments:
In `@crates/gl/src/webhook.rs`:
- Around line 157-164: The webhook listing flow in `list_webhooks` is treating
error responses as empty results because it parses JSON and reads
`resp["webhooks"]` without checking the HTTP status first. Update the
`client.get_signed(...).await?` handling to inspect the response status before
deserializing or accessing `webhooks`, and surface 401/403/404 as errors instead
of falling back to `No webhooks`. Keep the fix localized around the
`list_webhooks` response handling and the `webhooks` extraction logic.
---
Nitpick comments:
In `@crates/gitlawb-node/src/api/mod.rs`:
- Around line 177-181: The `list_webhooks` drift-test row in `api::mod` only
checks the owner gate, so it can miss regressions in the read-visibility half.
Update the marker set for `list_webhooks` to include both `authorize_repo_read(`
and `require_repo_owner(` so the test covers the private-repo existence check as
well as the owner check.
🪄 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: ee75b2eb-a906-4a01-b6ce-0fd597b7eec6
📒 Files selected for processing (11)
crates/gitlawb-node/src/api/events.rscrates/gitlawb-node/src/api/mod.rscrates/gitlawb-node/src/api/protect.rscrates/gitlawb-node/src/api/replicas.rscrates/gitlawb-node/src/api/webhooks.rscrates/gitlawb-node/src/test_support.rscrates/gl/src/http.rscrates/gl/src/mcp.rscrates/gl/src/protect.rscrates/gl/src/repo.rscrates/gl/src/webhook.rs
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
[P2] Fail closed when the local repo lookup errors
crates/gitlawb-node/src/api/events.rs:64
CodeRabbit's inline review item is still valid: get_repo errors are currently collapsed with .ok().flatten(), so a database error is indistinguishable from Ok(None) and the handler takes the ungated gossip-only path. That means the new private-repo visibility gate only runs when the lookup succeeds; on a lookup error, the handler can continue into the public event feed path instead of failing closed. Please complete that review request by propagating the lookup error and only using the gossip-only behavior for an actual Ok(None).
[P2] Surface webhook-list authorization failures instead of printing an empty list
crates/gl/src/webhook.rs:157
CodeRabbit's outside-diff review item is still valid: gl webhook list signs the request now, but it immediately deserializes the response and reads resp["webhooks"] without checking the HTTP status. A 401/403/404 JSON error body has no webhooks field, so the command prints No webhooks for and exits successfully instead of telling the user that the node rejected the request. Please complete that review request by checking resp.status() before extracting webhooks, matching the status handling already used by the neighboring list commands.
[P3] Pin both halves of the list_webhooks authorization gate
crates/gitlawb-node/src/api/mod.rs:181
CodeRabbit's drift-guard comment is still valid: the authz_guard row for list_webhooks only checks for require_repo_owner(, even though this route depends on authorize_repo_read( first to hide private-repo existence before the owner-only check runs. The behavior tests cover the current implementation, but the static guard can still pass if the read-visibility half is later removed. Please add the authorize_repo_read( marker for list_webhooks too, so the regression guard matches the layered security boundary this PR is adding.
beardthelion
added
kind:security
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/gitlawb-node/src/api/events.rs (1)
55-87: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse
did_matches(...)for the owner fast-path behind this new gate.This handler now relies on
visibility_checkto admit the repo owner, but that helper still treats ownership ascaller == owner_did || caller == owner_short. If a local repo row is stored in bare form, a signeddid:key:...owner is denied here and gets the opaque404reserved for non-readers.Based on learnings, owner DID comparisons must normalize bare and full forms with
did_matches(...).🤖 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/gitlawb-node/src/api/events.rs` around lines 55 - 87, The new visibility gate in the events handler is still relying on raw owner DID equality, which can wrongly deny the repo owner when the stored owner is in bare form and the caller is a full DID. Update the owner fast-path in the events flow by using did_matches(...) for the ownership check, and apply it consistently in the visibility_check path around the repo_record handling so bare and full DID forms both match correctly.Source: Learnings
🤖 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.
Outside diff comments:
In `@crates/gitlawb-node/src/api/events.rs`:
- Around line 55-87: The new visibility gate in the events handler is still
relying on raw owner DID equality, which can wrongly deny the repo owner when
the stored owner is in bare form and the caller is a full DID. Update the owner
fast-path in the events flow by using did_matches(...) for the ownership check,
and apply it consistently in the visibility_check path around the repo_record
handling so bare and full DID forms both match correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dc377dcf-7169-4355-96cd-8ce371715b57
📒 Files selected for processing (2)
crates/gitlawb-node/src/api/events.rscrates/gitlawb-node/src/test_support.rs
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the update. I rechecked the changed paths and found a few issues that still need to be addressed.
Findings
-
[P2] Resolve webhook-list repos from the signing identity
crates/gl/src/webhook.rs:153
The new--dirsupport loads and signs with the user's keypair, butcmd_liststill callsresolve_owner(&client), which gets the owner segment from the node root DID rather than from the keypair or anowner/repoargument. For a normal repo created by the local identity on a public node,gl webhook list my-repo --dir ...therefore signs as the repo owner but requests/api/v1/repos/<node-did>/my-repo/hooks, so the newly owner-gated route returns not found/forbidden instead of listing the user's hooks. Please resolve the owner the same way the other repo-scoped commands do: acceptowner/repowhen provided, and otherwise derive the owner from--dirbefore falling back to the node DID. -
[P2] Complete the webhook-list status handling request
crates/gl/src/webhook.rs:157
CodeRabbit's outside-diff request, and the laterjatmnchange request based on it, is still valid in the current patch.gl webhook listsigns the request now, but it immediately deserializes the response and readsresp["webhooks"]without checking the HTTP status. A 401/403/404 JSON error from the owner-gated endpoint has nowebhooksfield, so the command printsNo webhooks for <repo>and exits successfully instead of surfacing the authorization failure. Please complete that request by keeping theResponse, checkingstatus.is_success()before extractingwebhooks, and returning an error on rejected responses, like the neighboring list commands already do. -
[P2] Use DID-normalized owner matching in the visibility gate
crates/gitlawb-node/src/visibility.rs:17
CodeRabbit's current owner-normalization request is still valid. The newly added event gate, and the newauthorize_repo_readcallers for replicas/protected branches/webhooks, rely onvisibility_checkto admit the repo owner, butvisibility_check'sis_ownerhelper only checkscaller == owner_did || caller == owner_short. If a repo row stores the owner in bare form, as mirror rows do, a signed fulldid:key:...owner is denied by the new gate and gets the opaque 404 path reserved for non-readers. Please switch this helper to the samedid_matchessemantics used byrequire_repo_owner, and add coverage for the bare-owner/full-caller case on the newly gated read path. -
[P3] Complete the list_webhooks authz-guard request
crates/gitlawb-node/src/api/mod.rs:181
The previous CodeRabbit/jatmndrift-guard request is still only half addressed. The implementation oflist_webhooksintentionally depends onauthorize_repo_readfirst andrequire_repo_ownersecond, but theauthz_guardrow only pins the owner half. That means the static guard can still pass if the private-repo existence-hiding gate is later removed. Please add theauthorize_repo_read(marker forlist_webhookstoo, so the guard matches the layered authorization boundary this PR is adding.
Address jatmn's review feedback on the read-surface gate: - gl webhook: resolve owner from the signing identity, not the node DID. cmd_create/list/delete all routed through the local resolve_owner helper, which only ever returned the node root DID, so for a repo owned by the local identity the owner-gated /hooks route returned 404/403. Drop that helper and reuse repo::resolve_owner_repo_pair (accepts owner/repo, else derives from --dir/keypair). Fixed across all three subcommands so create-writes and list/delete-reads stay on the same owner. - gl webhook list: check the HTTP status before reading `webhooks`. A 401/403/404 body has no webhooks field and previously printed "No webhooks" and exited 0, hiding the rejection; now bails with the server message like cmd_create. - visibility::is_owner: delegate to api::did_matches so the read gate and require_repo_owner share one normalization. Fixes the under-match (bare owner stored, full did:key caller, as mirror rows) that denied the owner with the opaque 404, and the cross-method over-match. - authz_guard: pin both halves of list_webhooks (authorize_repo_read and require_repo_owner) so removing the existence-hiding gate fails the drift test. cargo fmt/clippy clean; 644 workspace tests pass.
|
All four addressed, pushed to 60ef427.
Two follow-ups in the later commits while I was in here: a fmt/clippy clean, 646 tests pass. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/webhook.rs`:
- Line 159: The webhook success-path parsing in `webhook.rs` is too permissive
because `hooks` defaults to an empty array when `body["webhooks"]` is missing or
malformed. Update the response handling around the `webhooks` extraction so that
2xx responses without a valid `webhooks` array are treated as an error instead
of “No webhooks”, and keep the existing success flow only when the `webhooks`
field is present and is an array. Use the `webhooks` parsing logic in the
webhook response code to locate and tighten this validation.
🪄 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: 761abeb7-1bd7-4e17-b0d4-d241f941a155
📒 Files selected for processing (5)
crates/gitlawb-node/src/api/mod.rscrates/gitlawb-node/src/test_support.rscrates/gitlawb-node/src/visibility.rscrates/gl/src/repo.rscrates/gl/src/webhook.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/gitlawb-node/src/api/mod.rs
- crates/gl/src/repo.rs
- crates/gitlawb-node/src/test_support.rs
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
This PR gates the list_webhooks, list_replicas, list_protected_branches, and list_repo_events read surfaces on the node, adds NodeClient::get_maybe_signed so CLI read commands can sign when an identity is present, and updates the gl/MCP callers that hit those endpoints. I found two issues that still need to be addressed before this is ready.
Findings
-
[P2] Fix MCP
webhook_listdefault owner resolution
crates/gl/src/mcp.rs:935
Thewebhook_listtool now usesget_signed, but when theownerargument is omitted it still callsresolve_owner, which falls back to the node DID. In a normal MCP server started with a public node and the user's--dir, the request is signed by the user's keypair but the path uses the node owner's DID, so the owner-gatedlist_webhooksendpoint returns 403 (or 404 for a private repo). Default the owner to the signing keypair's short DID, keeping the explicitownerargument as an override, so the MCP tool matches the behavior you already fixed forgl webhook list. -
[P3] Decide whether
list_labelsis read-visibility-gated
crates/gl/src/repo.rs:655andcrates/gitlawb-node/src/api/labels.rs:74
cmd_label_listwas switched toget_maybe_signedwith a comment saying labels are "Read-visibility-gated like the sibling read surfaces", but the node'slabels::list_labelshandler still serves labels without callingauthorize_repo_reador checking the caller. That makes the signature unnecessary and leaves private-repo labels public. Either gate the server endpoint withauthorize_repo_read(and add the marker toauthz_guard), or update the client comment to explain that the signing is only future-proofing and remove the claim that labels are already gated.
…ls on read visibility (#113) - MCP webhook_list: default owner to the signing keypair's short DID (explicit owner arg still overrides) instead of the node root DID, so the signed, owner-gated /hooks request resolves to the caller's own repo. Mirrors the gl webhook list fix. resolve_owner and webhook_create/delete left untouched. - list_labels: gate on authorize_repo_read so a private repo's label names 404 for non-readers (public repos stay anonymously listable), matching the sibling read surfaces and making the existing client comment true. Added the authz_guard marker and a read-visibility integration test. - gl label list: check HTTP status before reading the body so the now-gated 404 surfaces as an error instead of printing "No labels" and exiting 0, matching the sibling list clients (replicas, protected branches).
|
Both addressed in 80cfb13. MCP On |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/gitlawb-node/src/test_support.rs (1)
1252-1267: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAssert the anonymous 404 body does not leak labels.
This path only checks the status. Since anonymous callers are part of the private-label leak threat model, read the body here and assert
!leaks(&bytes)like the signed non-reader case.Proposed test hardening
assert_eq!( resp.status(), StatusCode::NOT_FOUND, "anon is denied the private labels" ); + let bytes = axum::body::to_bytes(resp.into_body(), usize::MAX) + .await + .unwrap(); + assert!(!leaks(&bytes), "anonymous 404 must not leak the label name");🤖 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/gitlawb-node/src/test_support.rs` around lines 1252 - 1267, The anonymous private-repo 404 test only verifies StatusCode in the router().oneshot flow and does not check for label leakage. Update this test in test_support.rs to read the response body bytes after the NOT_FOUND assertion and verify they do not leak labels using the existing leaks(...) helper, matching the signed non-reader coverage.
🤖 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/gitlawb-node/src/test_support.rs`:
- Around line 1252-1267: The anonymous private-repo 404 test only verifies
StatusCode in the router().oneshot flow and does not check for label leakage.
Update this test in test_support.rs to read the response body bytes after the
NOT_FOUND assertion and verify they do not leak labels using the existing
leaks(...) helper, matching the signed non-reader coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 177b8fc7-25a3-4bed-9ac8-e2954a160e93
📒 Files selected for processing (5)
crates/gitlawb-node/src/api/labels.rscrates/gitlawb-node/src/api/mod.rscrates/gitlawb-node/src/test_support.rscrates/gl/src/mcp.rscrates/gl/src/repo.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/gitlawb-node/src/api/mod.rs
- crates/gl/src/repo.rs
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the update. I rechecked the changed paths and found a couple of issues that still need to be addressed.
Findings
-
[P2] Surface denied repo-info reads instead of printing fake repo details
crates/gl/src/repo.rs:348
This PR changesgl repo infoto useget_maybe_signedfor the visibility-gatedGET /api/v1/repos/{owner}/{name}path, but the command still parses the response body without checkingresp.status(). When the node returns the expected JSON 404 for a private repo the caller cannot read, the command treats that error JSON as a repo, falls back toowner,?,true, andmain, prints a plausible-looking repository summary, and exits successfully. Please keep theResponse, checkstatus.is_success()before reading repo fields, and return the node's error message on 401/403/404, like the other newly gated list commands do. -
[P3] Complete CodeRabbit's anonymous label-leak assertion request
crates/gitlawb-node/src/test_support.rs:1252
CodeRabbit's latest test-hardening request is still valid in the current patch. The signed non-reader label path reads the 404 body and asserts that it does not contain the private label name, and the sibling replica/protected-branch anonymous-denial cases make the same no-leak assertion. The anonymous private-label branch, though, only checks the 404 status and never reads the body, so this PR's regression coverage can still miss the exact anonymous label-name leak it is trying to close if an error body regresses later. Please complete that request by reading the anonymous 404 body here and asserting!leaks(&bytes).
…#113) cmd_info parsed the GET /api/v1/repos/{owner}/{name} body without checking the HTTP status, so a private-repo JSON 404 was rendered as a fake repo summary (owner / ? / public / main) and exited 0. Capture the status and bail on non-2xx, matching the sibling list commands. Also assert the anonymous 404 label-list body does not leak the label name, closing the last unread-body branch in the read-visibility gate test.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the update. I rechecked the changed paths and found issues that still need to be addressed.
Findings
-
[P1] Rebase on main and resolve the merge conflicts first
GitHub mergeability
GitHub currently reports this branch asCONFLICTINGwith merge stateDIRTY, so the PR cannot be merged in its current state. Please rebase on the latestmainbranch and resolve those conflicts before addressing the review finding below, so any follow-up fix applies to the code that will actually land. -
[P2] Surface denied MCP webhook-list responses as tool errors
crates/gl/src/mcp.rs:949
The MCPwebhook_listpath now signs the owner-gated/hooksrequest, but it still immediately deserializes the response body and returns it as a successful tool result without checkingresp.status(). If the node returns the expected 401/403/404 JSON error for a missing identity, wrong owner, private repo, or deleted repo, the MCP tool reports success with that error body instead of failing the tool call, unlike the fixedgl webhook listpath which checksstatus.is_success()before readingwebhooks. Please keep theResponse, check the HTTP status, and return an error with the node message for non-2xx responses before serializing the successful webhook list.
…122) * test(node): guard that every repo-scoped handler is visibility-gated Mirrors the existing every_in_scope_mutation_has_its_gate source-string CI test on the egress side: scrapes every repo-scoped handler (any pub/pub(crate) async fn taking Path<(String,String..)>) and asserts it carries an authz marker, either a read gate (authorize_repo_read / visibility_check) or a write/owner/self gate (require_repo_owner / require_owner / did_matches / &auth.0), or is listed in KNOWN_UNGATED with its tracking issue. Verb-agnostic on purpose: a repo-scoped read named outside list_/get_ (git_info_refs, replicate_encrypted_blobs, withheld_paths) is still checked, so a new ungated egress of any name fails CI instead of shipping as the next leak. A completeness scan over src/api/ asserts every module declaring a repo-scoped handler is wired into the guard, so a whole new file cannot add an ungated handler the scrape never looks at. KNOWN_UNGATED is the live gap list (certs/issues/labels/ bounties/stars per #120, plus events/webhooks/replicas/protected pending PR #113); the staleness assert forces each entry out the moment its gate lands. A count floor trips if the scrape silently breaks, and unit tests pin handler_names and is_repo_scoped. Verified by execution: removing an allowlist entry trips the missing-gate assert; listing a gated handler trips the staleness assert; dropping a source file trips the completeness scan; breaking the parser trips the count floor; and a non-list/get handler (git_info_refs) is confirmed scraped and gate-checked. * test(node): don't let a service-conditional gate satisfy the egress guard (#122) The guard counted a handler as gated on bare substring presence of a gate marker, so git_info_refs false-passed on visibility_check( even though that check only runs under `if service == "git-upload-pack"` and leaves the git-receive-pack advertisement ungated. Add gate_runs_unconditionally, which ignores markers that sit only inside an `if service ==` block, and track git_info_refs in KNOWN_UNGATED until #119 makes the gate unconditional. When that lands, the staleness assert forces the entry's removal. * fix(review): harden gate_runs_unconditionally against unclosed blocks (#122) Code review of the egress-guard hardening: - Clamp the span-advance index so an unclosed `if service ==` block can't slice past the body end and panic (latent: real source is balanced, but the scraper should not crash on a pathological future body). Keep span-to-EOF as the fail-safe direction. - Document that only `if service ==` is detected, not `match service`, so a future match-arm gate isn't silently treated as full. - Add unit cases: gate inside both service blocks (conditional), gate inside and outside (full), and the unclosed-block no-panic regression.
list_webhooks sat on the optional_signature read group with no auth or ownership check, so any unauthenticated caller who knew a repo's owner/name could enumerate its webhooks (target URL, created_by_did, events; only the secret was redacted). Gate it read-visibility-then-owner, mirroring create_webhook/delete_webhook: headerless -> 401, non-reader of a private repo -> 404 (existence hidden, uniform with the sibling read surfaces), non-owner of a public repo -> 403. Add a list_webhooks row to the authz_guard drift table so the gate can't silently regress.
list_replicas sat on optional_signature with no visibility check, so a private repo's replica URLs were enumerable by anyone who knew owner/name. Replica lists are a documented public mirror-discovery surface, so gate on read visibility (authorize_repo_read) rather than owner-only: a public repo stays anonymously listable, a private repo returns 404 to anyone who cannot read it. register_replica registers non-owner DIDs, and a replica operator is not a visibility reader, so a non-owner replica operator of a private repo gets 404 — the intended contract, pinned by the new test. Add the authz_guard row.
list_protected_branches sat on optional_signature with no visibility check, leaking a private repo's branch names to any unauthenticated caller. Gate on read visibility (authorize_repo_read at the root, INV-2 root-listing exception): public repos stay anonymously listable, private repos 404 to a non-reader. Add the authz_guard row.
…-only (#94) list_repo_events served a private repo's ref certificates and gossip ref metadata (ref names, SHAs) to any unauthenticated caller. The handler also tolerates a repo it knows only via gossip (no local row) and legitimately serves its events, so a blanket get_repo->authorize_repo_read swap would have 404'd that federation path. Gate only the locally-hosted branch: when get_repo returns a record, run visibility_check on it at the root and 404 on deny before fetching either the cert or gossip events. A gossip-only repo (no local row) keeps its existing public federation-feed behavior; the global /api/v1/events/ref-updates feed remains a separately tracked deferral. Add the authz_guard row (visibility_check marker, since the gate is direct to avoid a second get_repo).
…ated (#94) `gl webhook list` built its NodeClient with no keypair, so the GET went out anonymously. With the node now owner-gating GET /hooks, that request returns 401. Thread a --dir through the List subcommand and load the keypair, matching `gl webhook create`/`delete`, so the listing is signed as the owner. (Replica and protected-branch listings stay anonymous for public repos under read-visibility gating, so their gl clients need no change.)
Code review caught that NodeClient::get() never attaches the RFC 9421 signature even when a keypair is loaded — only get_signed/post/put/delete sign. The earlier gl webhook-list fix loaded the keypair but still called get(), so `gl webhook list` (and the MCP webhook_list tool) 401'd against the now-owner-gated endpoint. Switch both to get_signed. Harden the two cmd_list tests to require a Signature/Signature-Input header on the mock (verified: they fail against plain get(), pass with get_signed) — the missing assertion that let the bug through. Add the untested owner-of-private- repo webhooks path (200 + redaction). Fix the stale module doc and the authz_guard bucket comment for list_webhooks.
…eir own repos (#94) The #94 server change read-visibility-gates GET /replicas and /branches/protected: public repos stay anonymous, private repos require the owner/reader to authenticate. Four gl read commands sent unsigned GETs and so broke (404) or silently degraded (empty output) for a private repo's owner: gl protect list, gl repo owner (silent empty protected list), gl repo info (silent missing replica count), gl repo replicas (and it had no --dir flag). Add NodeClient::get_maybe_signed — signs when an identity is present, stays anonymous otherwise (mirrors the conditional signing of post/put/delete) — and route the four call sites through it. Add --dir to `gl repo replicas`. Harden the protect-list tests to assert the request is signed (verified: they fail on plain get()). Public-repo anonymous reads are unchanged.
Round-2 review caught that cmd_info and cmd_owner loaded a keypair and signed
their replica/protected-branch sub-fetch but left the PRIMARY GET
/api/v1/repos/{owner}/{name} on plain get(). get_repo is read-visibility-gated,
so for a private repo that primary fetch 404'd first and the command bailed
before reaching the signed sub-fetch — the half-fix never actually let an owner
inspect their own private repo. Route both primary fetches through
get_maybe_signed.
Add the missing signing guards: harden test_cmd_owner_is_owner to require a
signature on both mocks, and add tests for cmd_replicas and cmd_info (both were
untested) asserting the request is signed when an identity is present.
Out of scope (pre-existing, gated before #94, untouched here): gl repo commits,
gl pr list/view/diff, and the MCP repo_get/repo_commits/pr_* tools call other
already-gated read endpoints unsigned and are broken for private repos
independently of this change.
Close the vetting gaps from the review: - node: a listed reader (non-owner) passes the read gate but is still refused the webhook list (403), and headerless on an existing-private vs an absent repo both 401 — proving the 401 fires before any lookup, so it is not an existence oracle. - node: the read-visibility surfaces (replicas, protected branches) admit a listed reader who is not the owner (the allow-list branch of visibility_check), while a non-reader stranger still 404s. - gl: get_maybe_signed's anonymous fallback (no identity -> unsigned GET) is now exercised, RED-proven by flipping it to get_signed (which signs and fails the no-signature-header assertion).
) Close the last two reasoned-only paths by execution: - MCP webhook_list: a call_tool("webhook_list", ...) dispatch test against a mock node asserts the /hooks request carries a signature (get_signed). RED-proven: flipping the arm back to get() fails the header assertion. - gl->node end-to-end: a real RFC-9421 signature produced exactly as the gl client's get_signed does (gitlawb_core::http_sig::sign_request over GET + empty body) is run through the node's actual optional_signature middleware, which verifies it and authorizes the owner (200 + webhook body). RED-proven: signing over POST while sending GET makes the node reject it. This stitches the gl signing side and the node verifying side in one test rather than mocking one end.
…nts (#94) `get_repo(...).await.ok().flatten()` collapsed a lookup Err into None, so a transient DB failure skipped the read-visibility gate and the handler served a private repo's gossip ref-update events. Propagate with `?` instead: an error now maps to AppError::Internal (500) and fails closed, while a genuine Ok(None) (repo not hosted locally) stays the intentional ungated pass-through. Add a regression test that forces a get_repo error (drop the column its SELECT reads) and asserts 500 with no secret in the body. The injection is self-verifying: it asserts the lookup succeeds pre-drop and errors post-drop, so the test can't pass vacuously if get_repo's projection changes later.
Address jatmn's review feedback on the read-surface gate: - gl webhook: resolve owner from the signing identity, not the node DID. cmd_create/list/delete all routed through the local resolve_owner helper, which only ever returned the node root DID, so for a repo owned by the local identity the owner-gated /hooks route returned 404/403. Drop that helper and reuse repo::resolve_owner_repo_pair (accepts owner/repo, else derives from --dir/keypair). Fixed across all three subcommands so create-writes and list/delete-reads stay on the same owner. - gl webhook list: check the HTTP status before reading `webhooks`. A 401/403/404 body has no webhooks field and previously printed "No webhooks" and exited 0, hiding the rejection; now bails with the server message like cmd_create. - visibility::is_owner: delegate to api::did_matches so the read gate and require_repo_owner share one normalization. Fixes the under-match (bare owner stored, full did:key caller, as mirror rows) that denied the owner with the opaque 404, and the cross-method over-match. - authz_guard: pin both halves of list_webhooks (authorize_repo_read and require_repo_owner) so removing the existence-hiding gate fails the drift test. cargo fmt/clippy clean; 644 workspace tests pass.
…t signing Self-review follow-ups on the read-gate work: - list_repo_events: add list_repo_events_admits_listed_reader, pinning the allow-list branch of the events gate (a listed non-owner reader gets 200 with the cert; a non-reader stranger 404s with no leak). Mirrors the read_visibility_admits_listed_reader coverage the sibling replica and protected-branch surfaces already had; the events handler was the only gated read surface missing it. - gl repo: cmd_label_list now loads an identity and uses get_maybe_signed instead of a bare anonymous get(), matching cmd_replicas/cmd_info. Public repos stay anonymously listable; a private-repo owner can read their own labels once /labels is read-visibility-gated, with no silent client drift. fmt/clippy clean; 645 workspace tests pass.
Final-review hardening of the prior two follow-ups: - gl repo label-list tests: require the signature/signature-input headers on the signed-branch mocks, and add an anonymous (no-identity) test asserting the header is absent. Without the header assertion the tests passed even with a bare get(), so the signing change was not actually pinned (RED-verified: reverting to get() now fails both signed-branch tests). - list_repo_events_admits_listed_reader: also assert the cert sha (not just the ref name) is absent from the stranger's 404 body, matching the list_repo_events_is_read_visibility_gated leak-check convention. fmt/clippy clean; 646 workspace tests pass.
…ls on read visibility (#113) - MCP webhook_list: default owner to the signing keypair's short DID (explicit owner arg still overrides) instead of the node root DID, so the signed, owner-gated /hooks request resolves to the caller's own repo. Mirrors the gl webhook list fix. resolve_owner and webhook_create/delete left untouched. - list_labels: gate on authorize_repo_read so a private repo's label names 404 for non-readers (public repos stay anonymously listable), matching the sibling read surfaces and making the existing client comment true. Added the authz_guard marker and a read-visibility integration test. - gl label list: check HTTP status before reading the body so the now-gated 404 surfaces as an error instead of printing "No labels" and exiting 0, matching the sibling list clients (replicas, protected branches).
…#113) cmd_info parsed the GET /api/v1/repos/{owner}/{name} body without checking the HTTP status, so a private-repo JSON 404 was rendered as a fake repo summary (owner / ? / public / main) and exited 0. Capture the status and bail on non-2xx, matching the sibling list commands. Also assert the anonymous 404 label-list body does not leak the label name, closing the last unread-body branch in the read-visibility gate test.
…e to 0.4.0 The repo-scoped egress guard's allowlist (added in #122, merged after this branch was opened) still listed the read surfaces this branch gates. Now that list_labels, list_repo_events, list_webhooks, list_replicas, and list_protected_branches are read-visibility gated, the staleness assert requires removing them from the allowlist. The #120 surfaces (certs, issues, bounties, stars) stay listed; git_info_refs stays until #119 lands. Lockfile versions follow the 0.4.0 release the rebase landed on.
The MCP webhook_list path deserialized the response body and returned it as a successful tool result without checking the HTTP status, so a 401/403/404 JSON error (missing identity, wrong owner, private or deleted repo) was reported as success with the error body as the result. Check status().is_success() before deserializing and surface the node's message as a tool error, matching the `gl webhook list` path. Covered by a non-2xx test (RED without the check).
beardthelion
force-pushed
the
fix/gate-repo-read-surfaces
branch
from
da8be46 to
dbf3bdf
Compare
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the update. I rechecked the changed paths and found one issue that still needs to be addressed.
Findings
- [P2] Parse the protected-branch response in
gl repo owner
crates/gl/src/repo.rs:725
This PR switches thegl repo ownerprotected-branch sub-fetch toget_maybe_signed, so a private-repo owner can now successfully reach/api/v1/repos/{owner}/{name}/branches/protected. However, that command still only reads a bare array orbody["branches"], while the node endpoint returns{"protected_branches":[...],"count":...}andgl protect listalready parses that key. As a result,gl repo owner --dir ...reportsProtected: (none)even when the signed request returns protected branches, and the current tests miss it because they mock a bare array instead of the real server response shape. Please parseprotected_brancheshere and update the owner tests to use the endpoint's actual response schema.
gl repo owner parsed the protected-branch response as a bare array or a
"branches" key, but the node returns {"protected_branches":[...],"count":N},
so it always printed "Protected: (none)" even when branches were protected.
Extract the parse into parse_protected_branches matching the gl protect list
shape, and update the owner tests to mock the real schema instead of bare
arrays (which is why the bug went uncaught).
|
@jatmn fixed in 8592621. |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the update. I rechecked the previously discussed paths and do not see any remaining actionable issues from my side.
@kevincodex1 LGTM
3 participants
Closes #94, and gates the three sibling read surfaces that share its pattern.
Problem
Four GET handlers on the node sat on the
optional_signatureread group with no ownership or visibility check, so an unauthenticated caller who knew a repo'sowner/namecould read data that should be private:list_webhooks(Unauthenticated GET /repos/:owner/:repo/hooks leaks webhook target URLs for any repo #94) leaked every webhook's target URL,created_by_did, and events (only the secret was redacted).list_replicas,list_protected_branches, andlist_repo_eventsleaked replica URLs, branch names, and ref metadata (names + SHAs) for private repos.get_repoapplies nois_publicpredicate, so a private repo resolved the same as a public one.Fix
Gating model follows data sensitivity, not a blanket rule:
list_webhooksis owner-restricted but layered read-visibility-then-owner: headerless -> 401, non-reader of a private repo -> 404 (existence hidden, uniform with the siblings), non-owner of a public repo -> 403. Webhook callback URLs are owner-secret config.list_replicas,list_protected_branches,list_repo_eventsgate on read visibility (authorize_repo_readat/, the INV-2 root-listing exception). Public repos stay anonymously readable; private repos return an opaque 404.replicasis documented as a public mirror-discovery surface, so read-visibility (not owner-only) preserves that for public repos.list_repo_eventsonly gates the locally-hosted branch: when the node holds the repo it runsvisibility_checkon the loaded record before serving either the cert or gossip events; a repo known only via gossip (no local row) keeps its existing public federation-feed behavior.Each new gate is pinned into the existing
authz_guarddrift table so it cannot silently regress.CLI/MCP
Tightening the node broke
gl/MCP read commands that sent unsigned requests. Fixed the callers of the newly gated endpoints:gl webhook listand the MCPwebhook_listtool now sign (get_signed);gl protect list,gl repo replicas, and the replica/branch sub-fetches ingl repo info/gl repo ownersign when an identity is present via a newNodeClient::get_maybe_signed(anonymous otherwise, so public repos still work without auth). Added--dirtogl repo replicas.Tests
Test-first throughout. Coverage includes the full per-surface matrix (owner / non-owner / reader / stranger / headerless / anonymous / absent), the no-existence-oracle property (headerless on an existing-private vs an absent repo both 401), the gossip-only vs local-private split for events, both branches of
get_maybe_signed, an end-to-end test that runs a realsign_requestoutput through the node'soptional_signaturemiddleware, and signature-header assertions on the CLI/MCP signing paths.Deferred (separate issues)
GET /api/v1/events/ref-updatesfeed is ungated for private-repo ref metadata (the REST analog of GraphQL refUpdates serves private-repo ref metadata to anonymous callers (visibility bypass) #112). Out of scope here; kept to the per-repo surfaces.gl/MCP read commands (gl repo commits,gl pr *, MCPrepo_get/pr_*) hit endpoints that were already visibility-gated before this change and send unsigned requests, so they are broken for private repos independently of this PR. Worth a follow-up to sign allgl/MCP read commands.Summary by CodeRabbit
--dirsupport forgl repo replicasandgl webhook listto enable signed, owner-gated listing.