From 4335535639a9b6f36ba97dd735783dcaa36c8f89 Mon Sep 17 00:00:00 2001 From: carlitos973 Date: Mon, 6 Jul 2026 06:48:13 -0700 Subject: [PATCH 1/3] fix(gl): resolve webhook owner from caller identity, not node DID 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). --- crates/gl/src/webhook.rs | 39 +++++++++++++++++++++++++++++---------- 1 file changed, 29 insertions(+), 10 deletions(-) diff --git a/crates/gl/src/webhook.rs b/crates/gl/src/webhook.rs index 45b80ecd..bc0eb3b5 100644 --- a/crates/gl/src/webhook.rs +++ b/crates/gl/src/webhook.rs @@ -76,13 +76,32 @@ pub async fn run(args: WebhookArgs) -> Result<()> { } } -async fn resolve_owner(client: &NodeClient) -> Result { - let info: Value = client.get("/").await?.json().await?; - let did = info["did"] - .as_str() - .context("node missing DID")? - .to_string(); - Ok(did.split(':').next_back().unwrap_or(&did).to_string()) +/// 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. +async fn resolve_owner( + repo: &str, + dir: Option<&std::path::Path>, + client: &NodeClient, +) -> Result<(String, String)> { + if let Some((owner, name)) = repo.split_once('/') { + return Ok((owner.to_string(), name.to_string())); + } + let short = if let Ok(kp) = load_keypair_from_dir(dir) { + let did = kp.did().to_string(); + did.split(':').next_back().unwrap_or(&did).to_string() + } else { + let info: Value = client.get("/").await?.json().await?; + let did = info["did"] + .as_str() + .context("node missing DID")? + .to_string(); + did.split(':').next_back().unwrap_or(&did).to_string() + }; + Ok((short, repo.to_string())) } async fn cmd_create( @@ -95,7 +114,7 @@ async fn cmd_create( ) -> Result<()> { 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?; let event_list: Vec<&str> = events.split(',').map(str::trim).collect(); @@ -145,7 +164,7 @@ async fn cmd_create( 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")) @@ -186,7 +205,7 @@ async fn cmd_list(repo: String, node: String) -> Result<()> { async fn cmd_delete(repo: String, id: String, node: String, dir: Option) -> Result<()> { 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?; let payload = serde_json::to_vec(&serde_json::json!({}))?; let resp = client From d42a80934292aaeaebde0a19d2ee8e107b41db0f Mon Sep 17 00:00:00 2001 From: carlitos973 Date: Mon, 6 Jul 2026 07:07:15 -0700 Subject: [PATCH 2/3] fix(gl): print fully-qualified owner/repo in webhook create delete hint 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. --- crates/gl/src/webhook.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/gl/src/webhook.rs b/crates/gl/src/webhook.rs index bc0eb3b5..f6565e70 100644 --- a/crates/gl/src/webhook.rs +++ b/crates/gl/src/webhook.rs @@ -158,7 +158,7 @@ async fn cmd_create( if has_secret { println!(" Secret: set (HMAC-SHA256 signing enabled)"); } - println!("\n Delete: gl webhook delete {repo} {id}"); + println!("\n Delete: gl webhook delete {owner}/{repo} {id}"); Ok(()) } From e5b1b0c8ebb4be5f53895bfe70e78e799e07a1f6 Mon Sep 17 00:00:00 2001 From: carlitos973 Date: Mon, 6 Jul 2026 08:37:37 -0700 Subject: [PATCH 3/3] fix(gl): thread identity resolution and status checking through webhook 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. --- crates/gl/src/webhook.rs | 85 ++++++++++++++++++++++++++++++++++------ 1 file changed, 72 insertions(+), 13 deletions(-) diff --git a/crates/gl/src/webhook.rs b/crates/gl/src/webhook.rs index f6565e70..167ca8f6 100644 --- a/crates/gl/src/webhook.rs +++ b/crates/gl/src/webhook.rs @@ -42,6 +42,8 @@ pub enum WebhookCmd { repo: String, #[arg(long, default_value = "https://node.gitlawb.com", env = "GITLAWB_NODE")] node: String, + #[arg(long)] + dir: Option, }, /// Delete a webhook Delete { @@ -66,7 +68,7 @@ pub async fn run(args: WebhookArgs) -> Result<()> { node, dir, } => cmd_create(repo, url, events, secret, node, dir).await, - WebhookCmd::List { repo, node } => cmd_list(repo, node).await, + WebhookCmd::List { repo, node, dir } => cmd_list(repo, node, dir).await, WebhookCmd::Delete { repo, id, @@ -81,7 +83,8 @@ pub async fn run(args: WebhookArgs) -> Result<()> { /// 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. +/// node's root DID if loading a local keypair fails for any reason +/// (missing, unreadable, or unparseable). async fn resolve_owner( repo: &str, dir: Option<&std::path::Path>, @@ -162,24 +165,29 @@ async fn cmd_create( Ok(()) } -async fn cmd_list(repo: String, node: String) -> Result<()> { +async fn cmd_list(repo: String, node: String, dir: Option) -> Result<()> { let client = NodeClient::new(&node, None); - let (owner, repo) = resolve_owner(&repo, None, &client).await?; + let (owner, repo) = resolve_owner(&repo, dir.as_deref(), &client).await?; - let resp: Value = client + let resp = client .get(&format!("/api/v1/repos/{owner}/{repo}/hooks")) - .await? - .json() .await - .context("invalid JSON")?; + .context("failed to connect to node")?; + let status = resp.status(); + let body: Value = resp.json().await.context("invalid JSON")?; - let hooks = resp["webhooks"].as_array().cloned().unwrap_or_default(); + if !status.is_success() { + let msg = body["message"].as_str().unwrap_or("unknown error"); + anyhow::bail!("list webhooks failed ({status}): {msg}"); + } + + let hooks = body["webhooks"].as_array().cloned().unwrap_or_default(); if hooks.is_empty() { - println!("No webhooks for {repo}"); + println!("No webhooks for {owner}/{repo}"); return Ok(()); } - println!("Webhooks for {repo} ({} total)\n", hooks.len()); + println!("Webhooks for {owner}/{repo} ({} total)\n", hooks.len()); for hook in &hooks { let id = hook["id"].as_str().unwrap_or("?"); let url = hook["url"].as_str().unwrap_or("?"); @@ -366,7 +374,7 @@ mod tests { .create_async() .await; - cmd_list("my-repo".to_string(), server.url()).await.unwrap(); + cmd_list("my-repo".to_string(), server.url(), None).await.unwrap(); } #[tokio::test] @@ -382,7 +390,58 @@ mod tests { .create_async() .await; - cmd_list("my-repo".to_string(), server.url()).await.unwrap(); + cmd_list("my-repo".to_string(), server.url(), None).await.unwrap(); + } + + #[tokio::test] + async fn test_list_webhooks_uses_caller_identity_for_bare_repo_name() { + let mut server = mockito::Server::new_async().await; + // A root mock is present (it would resolve to "z6MkTestOwner"), but a + // local identity is also supplied -- the identity must win, matching + // create/delete. If cmd_list fell back to the root DID instead, it + // would hit a path with no matching mock and fail. + let _root = mock_root(&mut server).await; + let (dir, kp) = tmp_identity(); + let did = kp.did().to_string(); + let short = did.split(':').next_back().unwrap_or(&did).to_string(); + + let _m = server + .mock( + "GET", + mockito::Matcher::Regex(format!(r"/repos/{short}/my-repo/hooks$")), + ) + .with_status(200) + .with_header("content-type", "application/json") + .with_body(r#"{"webhooks":[]}"#) + .create_async() + .await; + + cmd_list( + "my-repo".to_string(), + server.url(), + Some(dir.path().to_path_buf()), + ) + .await + .unwrap(); + } + + #[tokio::test] + async fn test_list_webhooks_error_status_propagates() { + let mut server = mockito::Server::new_async().await; + let _root = mock_root(&mut server).await; + + let _m = server + .mock("GET", mockito::Matcher::Regex(r"/hooks$".to_string())) + .with_status(404) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"repo not found"}"#) + .create_async() + .await; + + let err = cmd_list("my-repo".to_string(), server.url(), None) + .await + .unwrap_err(); + assert!(err.to_string().contains("repo not found")); } // ── delete ───────────────────────────────────────────────────────