Repository navigation
fix(gl): resolve webhook owner from caller identity, not node DID - #568
Conversation
resolve_owner() in webhook.rs unconditionally fetched the node's own root DID via GET / and used it as the repo owner, ignoring both an explicit owner/repo argument and the caller's own loaded identity. This made gl webhook create/list/delete effectively unusable for any repo not owned by the node operator itself -- every call resolved to the wrong owner and 404d. cert.rs's resolve_repo already has the correct pattern: check for an explicit slash first, then prefer the caller's own keypair, falling back to the node DID only if no local identity is available. This applies that same pattern to webhook create/list/delete. Verified against a live self-hosted node: webhook create/list/delete all resolve the correct owner now. Existing webhook test suite passes unchanged (8/8).
Per CodeRabbit review on #159: the resolve_owner destructuring shadowed repo with the bare name, so the printed delete hint lost the owner qualification -- only correct by coincidence if the caller runs it under the same identity used at create time. Print owner/repo explicitly so the hint always works regardless of default identity.
…ok list Apply the same fix already applied to create/delete to the list subcommand, per review feedback on this PR: - gl webhook list now accepts --dir and resolves the caller identity the same way create/delete do, instead of always falling back to the node root DID for bare repo names - cmd_list checks the HTTP status before parsing, so a non-2xx error (e.g. repo not found, not authorized) surfaces as an error instead of silently rendering as an empty webhook list - list output now prints the fully-qualified owner/repo, matching the fix already applied to the create/delete hints Also tightens resolve_owners doc comment: it falls back to the node DID on any keypair-load failure, not only when no keypair exists.
|
Warning Review limit reachedThis review ran on the open-source allowance, not this organization's plan, because the pull request author doesn't have an assigned seat. Waiting won't change this — ask an organization admin to assign them a seat, or add seats in Billing if every seat is already assigned, then retry. Next included review available in 5 minutes. View limit details
|
|
@coderabbitai full review |
Action not completed
|
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Merge readiness
- Required check
test (stable)andtest (beta)fail on this head (same root cause as Finding 1).fmt + clippy,build --release, and MSRV pass. - Branch is current with
origin/main(merge-base309fc836equals livemain); mergeable, no rebase needed. - Supersedes closed #159; scope is MCP-only delta on
mcp.rs. - CodeRabbit review rate-limited on this run (not a code blocker).
- PR checklist claims focused gl tests pass on this head; required workspace
cargo testdisagrees — treat as PR-owned until Finding 1 is closed.
Attribution check (not findings)
These were investigated and are not actionable on this PR:
- Wrong owner via node DID on MCP create/delete for signed callers — addressed by this diff (
{owner}/{name}+ keypair path). ownerarg vsowner/reposlash — slash-first matchesresolve_owner_repo_paironmain; not newly introduced here.- MCP
webhook_deletewithout identity using node DID — unchanged pre-existing path (merge-base already usedresolve_ownerfor delete); this PR only fixes path composition. - Forcing MCP
webhook_listto client-side hard-fail without identity like the CLI — intentional divergence (documented ~L2333–2337); out of scope.
Findings
🟡 P2 — PR-owned test drift: bare-repo webhook_list without identity
📍 Where: crates/gl/src/mcp.rs — resolve_owner_repo (~1348–1371), webhook_list handler (~982–1005), test_webhook_list_without_keypair_errors (~2507–2522).
💥 What fails: Required CI cargo test fails. Inputs: bare repo, empty identity dir (Some path, no identity.pem), no owner arg.
On merge-base 309fc836, webhook_list errored client-side with no identity found before HTTP.
On head a23c784, the same inputs call resolve_owner_repo → resolve_owner → GET /. Mockito returns 501, so the test fails on node info failed (501 Not Implemented) instead of the asserted no identity found.
The test and its comment are unchanged in the three-dot diff; only the handler wiring changed.
🔎 Root cause: Implementation and documented unsigned-list intent moved to node-DID fallback for bare repo without a local identity, but the regression pin still encodes merge-base client-side failure. That is test/comment drift inside this PR, not a pre-existing main defect.
📜 Stated contract:
test_webhook_list_without_keypair_errors(L2509–2510): “must error rather than fall back to the node DID and issue an unsigned, mis-scoped GET.”
PR body /
resolve_owner_repodoc (L1344–1346): node-DID last resort for callers without a keypair on the unsigned list path.
test_webhook_list_unsigned_without_identity_surfaces_the_node_denial(L2333–2337): MCP may contact the node unsigned and surface denial (today with explicitowneronly).
The repo cannot satisfy both the stale test comment and the new helper until the test is rewritten.
🏷️ Attribution: PR-introduced incomplete fix — behavior delta from a23c784, stale oracle not updated. Not unchanged pre-existing main debt.
📌 In this PR:
resolve_owner_repofallback (~1367–1370) andwebhook_listcaller (~989)- Stale test + comment (~2507–2522)
- Helper doc (~1344–1346) and conflicting test comment (~2509–2510) — align in the same pass
🔒 Unchanged on main: test_webhook_list_unsigned_without_identity_surfaces_the_node_denial (explicit owner, unsigned denial). CLI webhook.rs / resolve_owner_repo_pair. Node server hook auth.
🔧 Required correction: Replace or rewrite test_webhook_list_without_keypair_errors to pin head behavior:
- Mock
GET /returning a node DID (short form used in path). - Mock unsigned
GETto/api/v1/repos/{nodeDid}/{name}/hookswith a 401/403 denial (or assert the error chain through node info if that is the chosen failure mode). - Assert the tool errors from the node path, not with
no identity foundbefore HTTP. - Update L2509–2510 and L1344–1345 so comments match; if delete legitimately shares the fallback, say so in the helper doc (comment-only accuracy).
Optionally remove the test if a single new case fully subsumes it, but CI must pass without weakening the unsigned bare-repo pin.
// Pin: bare repo, no keypair → GET / then unsigned /hooks → Err(denial), not Ok and not
// client-side "no identity found" before the hooks request is attempted.🛠️ Author fix: One pass on every “In this PR” row. Do not revert resolve_owner_repo or signed bare-repo keypair behavior to green the old assertion. Do not edit CLI, server, or unrelated MCP tools.
🚫 Out of scope: CLI parity on no-identity list, delete auth redesign, shared cross-crate helper extraction, slash-vs-owner precedence changes.
|
Addressed on 5baa17d: the stale pin is rewritten as test_webhook_list_without_keypair_surfaces_node_denial. It mocks GET / returning the node DID, mocks the unsigned hooks GET returning a 401, and asserts the error is the node's denial with the hooks mock actually hit, so the unsigned-list divergence is now pinned rather than contradicted. The helper doc now names unsigned callers generally since webhook_delete reaches the same fallback. 7/7 webhook_list tests green. |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
🟢 Thanks for the contribution. I do not see any actionable issues from my review.
Merge readiness
- Prior review: jatmn CHANGES_REQUESTED on
a23c784(staletest_webhook_list_without_keypair_errorsoracle) appears addressed on5baa17d26: the pin is nowtest_webhook_list_without_keypair_surfaces_node_denialwithGET /+ unsigned hooks401, and requiredtest (stable)/test (beta)pass on this head. - Live target drift:
origin/mainis one commit ahead of the merge-base (66fe646— #566 quarantine/visibility). Nomcp.rsconflict surfaced in a dry merge check; rebasing onto currentmainbefore merge is still prudent so the branch is not behind live target. - Merge gate: GitHub shows mergeable; merge state blocked pending an approving review after the fix commit (expected while
CHANGES_REQUESTEDtargets an older head). - Supersedes closed #159; scope on this head is the MCP
mcp.rsdelta only (CLI webhook path already onmain). - CodeRabbit remains rate-limited — not a code blocker.
Attribution check (not findings)
Verified on head 5baa17d26 and merge-base 309fc836:
- Wrong owner via node DID for signed bare-repo MCP webhook calls — addressed (
resolve_owner_repo+{owner}/{name}paths;test_webhook_list_default_owner_is_keypair_not_node_did,resolve_owner_repo_bare_repo_uses_keypair_did). - Unsigned MCP
webhook_listwithout identity surfacing node denial vs CLI client-side identity error — intentional divergence, pinned bytest_webhook_list_unsigned_without_identity_surfaces_the_node_denialandtest_webhook_list_without_keypair_surfaces_node_denial. - MCP
webhook_deletewithout local identity reaching node-DID fallback + unsignedDELETE— unchanged pre-existing behavior aside from correctnamesegment; out of scope for this PR’s stated fix. - Optional
ownerarg ignored whenrepois alreadyowner/name— matches slash-first resolution used elsewhere; node owner-gates return 403 on signer mismatch (not a PR regression).
Summary
Re-home of #159 (author: carlitos973), carried onto current main with the remaining MCP-side ask implemented as a new commit on top of the contributor's unchanged work.
gl webhookand the MCP webhook tools now resolve a bare repo name through the caller's identity rather than the node operator DID.Motivation & context
gl webhook create/list/delete <repo>resolved the owner segment from the node's root DID (GET /), so webhooks landed under the node operator's namespace instead of the caller's. On current main the CLI fix has already landed independently (resolve_owner_repo_pair, signed list request, pinned tests); what remained is the same defect in the MCP webhook tools, which still usedresolve_owner's node-DID fallback and sent anowner/repostring as an extra path segment under the wrong owner.Kind of change
What changed
Contributor work (unchanged): bare-repo owner resolution via caller keypair for the CLI webhook path.
Maintainer commit:
resolve_owner_repoinmcp.rs:owner/repoinput splits first, an explicitownerarg wins next, then the signing keypair's short DID, with the node-DID fallback reachable only by the unsignedwebhook_listarm (which exists to surface the node's denial).webhook_create,webhook_list,webhook_deleteall threaded through it, matching the CLI'sresolve_owner_repo_paircontract.How a reviewer can verify
cargo test -p gl --bin gl resolve_owner(the three new pins plus the existingresolve_ownerdeny test).cargo test -p gl --bin gl webhook(CLI suite unchanged on this head).resolve_ownerinmcp.rs; each webhook handler calls it instead of the node-DID fallback.Before you request review
cargo fmt --all -- --checkcleancargo clippy --locked --workspace --all-targets -D warningsclean (pre-push gate ran on this head)Protocol & signing impact
None: no wire-format change.
webhook_liststill issues unsigned when no identity is present so the node's denial surfaces rather than a client-side failure.Notes for reviewers
The CLI half of the original PR is redundant on current main; the merge took main's side and this branch's delta is the MCP threading plus its tests. Supersedes #159.