fix(gl): resolve webhook owner from caller identity, not node DID#159
fix(gl): resolve webhook owner from caller identity, not node DID#159carlitos973 wants to merge 3 commits into
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).
github-actions
Bot
added
needs-issue
|
Thanks for the contribution. A couple of things will help us review this faster:
See CONTRIBUTING.md. Update the PR and these notes will clear automatically. |
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesWebhook Owner Resolution
Sequence Diagram(s)sequenceDiagram
participant WebhookCmd
participant resolve_owner
participant KeypairDir
participant Client
participant Node
WebhookCmd->>resolve_owner: resolve_owner(repo, dir, client)
resolve_owner->>resolve_owner: parse repo as owner/repo or bare repo
alt dir provided and keypair loads
resolve_owner->>KeypairDir: load caller keypair
KeypairDir-->>resolve_owner: DID short
else local identity unavailable
resolve_owner->>Client: GET /
Client->>Node: request root DID
Node-->>Client: root DID
Client-->>resolve_owner: root DID
end
resolve_owner-->>WebhookCmd: (owner, repo)
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR fixes gl webhook owner resolution so webhook operations target the caller’s identity (or an explicit owner/repo), rather than always defaulting to the node operator’s DID—making webhooks usable for repos not owned by the node itself.
Changes:
- Reworked webhook owner/repo resolution to honor
owner/repoinputs and prefer the caller’s local identity, falling back to the node DID only when needed. - Updated webhook create/list/delete to use the new resolution logic.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let keypair = load_keypair_from_dir(dir.as_deref())?; | ||
| let client = NodeClient::new(&node, Some(keypair)); | ||
| let owner = resolve_owner(&client).await?; | ||
| let (owner, repo) = resolve_owner(&repo, dir.as_deref(), &client).await?; | ||
|
|
| async fn cmd_list(repo: String, node: String) -> Result<()> { | ||
| let client = NodeClient::new(&node, None); | ||
| let owner = resolve_owner(&client).await?; | ||
| let (owner, repo) = resolve_owner(&repo, None, &client).await?; | ||
|
|
||
| let resp: Value = client | ||
| .get(&format!("/api/v1/repos/{owner}/{repo}/hooks")) |
| /// Resolve "repo" into (owner, name). If repo already contains a slash | ||
| /// (owner/repo form), use it directly. Otherwise, prefer the caller's own | ||
| /// identity (matching the pattern used elsewhere, e.g. cert.rs's | ||
| /// resolve_repo) so webhooks are created/listed against the DID that's | ||
| /// actually calling, not the node's own operator DID -- falls back to the | ||
| /// node's root DID only if no local keypair is available at all. |
![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
🤖 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 117: The `resolve_owner` destructuring in `webhook.rs` is shadowing
`repo`, so the later delete hint loses the original owner and prints an
incorrect command. Update the `resolve_owner` result binding in the webhook flow
to keep the resolved repository name in a different variable from the input
`repo`, and use that value consistently when constructing the delete hint.
Ensure the hint in the webhook command path prints the fully qualified
owner/repo form rather than only the name.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| let keypair = load_keypair_from_dir(dir.as_deref())?; | ||
| let client = NodeClient::new(&node, Some(keypair)); | ||
| let owner = resolve_owner(&client).await?; | ||
| let (owner, repo) = resolve_owner(&repo, dir.as_deref(), &client).await?; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Shadowing repo corrupts the delete-command hint printed later.
Destructuring into (owner, repo) reassigns repo to the name-only portion. The delete hint at Line 161 (gl webhook delete {repo} {id}) then drops the owner whenever the caller passed owner/repo, so the suggested command re-resolves to the local/node DID instead of the intended owner — reproducing the very mismatch this PR fixes.
Preserve the fully-qualified form for the hint (e.g. bind the resolved name to a new variable, or print {owner}/{repo}).
🐛 Proposed fix
- let (owner, repo) = resolve_owner(&repo, dir.as_deref(), &client).await?;
+ let (owner, name) = resolve_owner(&repo, dir.as_deref(), &client).await?;Then update the path and hint to use name and print the qualified form:
- println!("\n Delete: gl webhook delete {repo} {id}");
+ println!("\n Delete: gl webhook delete {owner}/{name} {id}");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let (owner, repo) = resolve_owner(&repo, dir.as_deref(), &client).await?; | |
| let (owner, name) = resolve_owner(&repo, dir.as_deref(), &client).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/webhook.rs` at line 117, The `resolve_owner` destructuring in
`webhook.rs` is shadowing `repo`, so the later delete hint loses the original
owner and prints an incorrect command. Update the `resolve_owner` result binding
in the webhook flow to keep the resolved repository name in a different variable
from the input `repo`, and use that value consistently when constructing the
delete hint. Ensure the hint in the webhook command path prints the fully
qualified owner/repo form rather than only the name.
beardthelion
added
crate:gl
|
hello @carlitos973 thank you so much for your contribution, please address comments from coderabbit and github bot |
Per CodeRabbit review on Gitlawb#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.
|
Good catch, fixed in d42a809 -- the delete hint now prints the fully-qualified owner/repo instead of relying on the caller's default identity matching. Tests still 8/8. |
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] Make
gl webhook list <repo>resolve against the caller identity
crates/gl/src/webhook.rs:165
TheListsubcommand still has onlyrepoandnode, andcmd_listcallsresolve_owner(&repo, None, &client), so a bare repo name can never load the caller's keypair. In that path the new helper always falls back toGET /and uses the node DID, which is the same failure mode this PR says it fixes forcreate/list/delete; onlycreateanddeletepass an identity directory. Please thread the identity selection throughgl webhook listas well, and add a test that asserts the requested/api/v1/repos/{owner}/{repo}/hookspath uses the local identity for a bare repo name. -
[P2] Do not turn list failures into an empty webhook list
crates/gl/src/webhook.rs:169
cmd_listnever checks the HTTP status before readingresp["webhooks"], so a node error body like{"message":"repo not found"}or{"message":"only the repo owner..."}has nowebhooksarray and is treated as[]. That means the wrong-owner 404 class this PR is trying to fix can still exit successfully asNo webhooks for ..., hiding a broken resolution or authorization failure from the caller. Please mirror the create/delete paths by checkingstatus.is_success()before rendering, and error with the node's message on non-2xx responses. -
[P3] Complete Copilot's request to preserve the qualified repo in list output
crates/gl/src/webhook.rs:167
Copilot's list-output thread is still valid: afterlet (owner, repo) = ..., the original argument is shadowed, sogl webhook list alice/projectprintsNo webhooks for projectorWebhooks for projectinstead of the repo the user supplied. Please keep the resolved name in a separate variable, or print{owner}/{name}, so the command output stays unambiguous for explicitowner/repoinput.
…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.
|
Thanks for the review — all three points addressed in e5b1b0c:
Also tightened the Full suite: 10/10 passing (8 previous + 2 new). Also manually verified against the live node: |
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] Run rustfmt on the changed webhook tests
crates/gl/src/webhook.rs:377
cargo fmt --all -- --checkcurrently fails on this file, including thecmd_list(...).await.unwrap()calls at lines 377 and 393. The repository's PR workflow runs that exact fmt check before clippy, so this branch will fail the required formatting gate even though the webhook tests themselves pass. Please runcargo fmt --alland push the formatting-only result. -
[P2] Apply the owner-resolution fix to the MCP webhook tools too
crates/gl/src/mcp.rs:919
The CLIgl webhookpath now resolves bare repo names through the caller identity and acceptsowner/repo, butgl mcp servestill leaveswebhook_create,webhook_list, andwebhook_deleteon the old behavior. Those handlers readrepounchanged, call the MCPresolve_owner()helper, and that helper only uses an explicitownerargument or falls back toGET /, so a bare repo name still targets the node operator DID and anowner/repostring is sent as an extra path segment under the wrong owner. Please thread the same(owner, repo)resolution through the MCP webhook handlers so agents using the MCP tools get the same fix as the CLI commands.
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Verified this against the current head, each check by execution.
cmd_listnow reads the response status and bails with the node's message on a non-2xx, so a wrong-owner 404 no longer renders as an empty list. Removing that check makestest_list_webhooks_error_status_propagatesfail, so it is load-bearing.gl webhook listtakes--dirand resolves against the caller identity like create and delete; forcing the node-DID branch makestest_list_webhooks_uses_caller_identity_for_bare_repo_namefail, so it is load-bearing too.- List output prints the fully-qualified
owner/repo, and a corrupt keypair falls back to the node DID as the doc now states.
One optional hardening, not a blocker:
Findings
- [P3] Validate the resolved owner and repo as single path segments
crates/gl/src/webhook.rs:93
split_once('/')lets extra path content through, and the segments are interpolated unescaped into/api/v1/repos/{owner}/{repo}/hooks.gl webhook list 'victim/repo/../../other/target'normalizes on the wire to/api/v1/repos/other/target/hooks, so the command reads a different repo than the one it prints. The server still gates by owner, so this is not an escalation, but rejecting/and..in the resolved segments (or percent-encoding them) keeps the request and the displayed target in sync.
Core change is correct and complete. @jatmn your three items all landed on this head.
sigh my pr's take too long to run lol |
beardthelion
dismissed
their stale review
Pulling my earlier approval. cargo fmt --all --check still fails on this head (e5b1b0c) at crates/gl/src/webhook.rs:374 and :390, where the two new cmd_list tests keep .await.unwrap() on one line. That's the rustfmt P1 jatmn already flagged, so this head isn't mergeable and my approve shouldn't stand on it. The functional change is correct (status-checked cmd_list/cmd_create, owner-qualified delete hint and list output), so re-request once rustfmt is applied. The open Copilot/CodeRabbit shadowing threads were filed on the first head (4335535) and are already addressed here.
Summary
resolve_owner()incrates/gl/src/webhook.rsunconditionally fetched the node's own root DID (viaGET /) and used it as the repo owner forgl webhook create/list/delete, ignoring both an explicitowner/repoargument and the caller's own loaded identity.crates/gl/src/cert.rs'sresolve_repoalready implements the correct pattern (check for an explicit slash first, prefer the caller's own keypair, fall back to the node DID only if no local identity exists). This PR applies that same proven pattern to webhook create/list/delete.Test plan
cargo test -p gl webhook)🤖 Generated with Claude Code
Summary by CodeRabbit
owner/repoand plainrepoinputs.gl webhook createnow prints the suggested delete command asgl webhook delete owner/repo <id>for clarity.