fix(node): gate /hooks and sibling read surfaces leaking private repo metadata (#94)#113
Open
beardthelion wants to merge 10 commits into
Open
fix(node): gate /hooks and sibling read surfaces leaking private repo metadata (#94)#113beardthelion wants to merge 10 commits into
beardthelion wants to merge 10 commits into
Conversation
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.
📝 WalkthroughWalkthroughFour previously ungated read endpoints ( ChangesNode-side endpoint authorization
CLI signed request support
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 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.
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
Comment on lines
+66
to
+80
| // #94: if this node hosts the repo locally, gate on read visibility BEFORE | ||
| // serving any events (cert OR gossip). A non-reader of a local private repo | ||
| // gets 404, hiding both its existence and its ref metadata. A repo the node | ||
| // knows only via gossip (no local row) has no local visibility rules to | ||
| // consult and keeps its existing public federation-feed behavior — the | ||
| // None branch is intentionally left ungated (tracked with the global | ||
| // /api/v1/events/ref-updates deferral). visibility_check on the loaded record | ||
| // avoids a second get_repo (mirrors api/encrypted.rs). | ||
| if let Some(ref record) = repo_record { | ||
| let caller = auth.as_ref().map(|e| e.0 .0.as_str()); | ||
| let rules = state.db.list_visibility_rules(&record.id).await?; | ||
| if visibility_check(&rules, record.is_public, &record.owner_did, caller, "/") | ||
| == Decision::Deny | ||
| { | ||
| return Err(AppError::RepoNotFound(format!("{owner}/{repo_name}"))); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Fail closed when the local repo lookup errors.
This new gate only runs for Some(record), but repo_record is produced with .ok().flatten(), so a DB lookup error is treated as gossip-only and skips visibility checks. Propagate the lookup error; only an actual Ok(None) should take the ungated gossip-only path.
Based on learnings, guarded private data should not create existence-oracle or unauthorized-access distinctions by failing open.
🛡️ Proposed fix
- let repo_record = state.db.get_repo(&owner, &repo_name).await.ok().flatten();
+ let repo_record = state.db.get_repo(&owner, &repo_name).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/gitlawb-node/src/api/events.rs` around lines 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.
Source: Learnings
jatmn
requested changes
Jun 28, 2026
jatmn
left a comment
jatmn
left a comment
Collaborator
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
New Features
Bug Fixes