From 4335535639a9b6f36ba97dd735783dcaa36c8f89 Mon Sep 17 00:00:00 2001 From: carlitos973 Date: Mon, 6 Jul 2026 06:48:13 -0700 Subject: [PATCH 1/5] 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 45b80ecd2..bc0eb3b51 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/5] 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 bc0eb3b51..f6565e700 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/5] 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 f6565e700..167ca8f68 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 ─────────────────────────────────────────────────────── From a23c784742cabeeebce426f15cc5bcb7c0e95197 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Tue, 10 Nov 2026 08:42:58 -0600 Subject: [PATCH 4/5] fix(gl): thread caller-identity repo resolution through MCP webhook tools (#159) --- crates/gl/src/mcp.rs | 120 ++++++++++++++++++++++++++++++++++++------- 1 file changed, 102 insertions(+), 18 deletions(-) diff --git a/crates/gl/src/mcp.rs b/crates/gl/src/mcp.rs index 390abbe28..0fa3cb824 100644 --- a/crates/gl/src/mcp.rs +++ b/crates/gl/src/mcp.rs @@ -963,7 +963,7 @@ async fn call_tool( .as_array() .map(|a| a.iter().filter_map(|v| v.as_str()).collect::>()) .unwrap_or_else(|| vec!["*"]); - let owner = resolve_owner(&args, &client).await?; + let (owner, name) = resolve_owner_repo(&args, repo, &keypair, &client).await?; let body = serde_json::to_vec(&json!({ "url": url, "secret": args["secret"], @@ -971,7 +971,7 @@ async fn call_tool( }))?; let resp = crate::http::read_json( client - .post(&format!("/api/v1/repos/{owner}/{repo}/hooks"), &body) + .post(&format!("/api/v1/repos/{owner}/{name}/hooks"), &body) .await?, "create webhook", ) @@ -981,19 +981,12 @@ async fn call_tool( "webhook_list" => { let repo = args["repo"].as_str().context("missing 'repo'")?; - // Owner-gated route: an explicit `owner` arg wins, otherwise default to - // the signing keypair's short DID (NOT the node DID that resolve_owner - // returns). The request is signed by this keypair, so the path owner - // must match it or the node returns 403/404. - let owner = if let Some(o) = args.get("owner").and_then(|v| v.as_str()) { - o.to_string() - } else { - let kp = keypair - .as_ref() - .context("no identity found — run `gl identity new` first")?; - let did = kp.did().to_string(); - did.split(':').next_back().unwrap_or(&did).to_string() - }; + // Owner-gated route: `owner/repo` splits first, an explicit `owner` + // arg wins next, otherwise the signing keypair's short DID (NOT the + // node DID that resolve_owner returns). The request is signed by + // this keypair, so the path owner must match it or the node + // returns 403/404. + let (owner, name) = resolve_owner_repo(&args, repo, &keypair, &client).await?; // Owner-gated AND auth-required route (list_webhooks 401s a headerless // caller, then 403s a non-owner). get_maybe_signed signs when an // identity is present (so the owner is authorized) and otherwise sends @@ -1004,7 +997,7 @@ async fn call_tool( // failing client-side before the node is contacted. let resp = crate::http::read_json( client - .get_maybe_signed(&format!("/api/v1/repos/{owner}/{repo}/hooks")) + .get_maybe_signed(&format!("/api/v1/repos/{owner}/{name}/hooks")) .await?, "webhook_list", ) @@ -1015,11 +1008,11 @@ async fn call_tool( "webhook_delete" => { let repo = args["repo"].as_str().context("missing 'repo'")?; let id = args["id"].as_str().context("missing 'id'")?; - let owner = resolve_owner(&args, &client).await?; + let (owner, name) = resolve_owner_repo(&args, repo, &keypair, &client).await?; let body = serde_json::to_vec(&json!({}))?; let resp = crate::http::read_json( client - .delete(&format!("/api/v1/repos/{owner}/{repo}/hooks/{id}"), &body) + .delete(&format!("/api/v1/repos/{owner}/{name}/hooks/{id}"), &body) .await?, "delete webhook", ) @@ -1345,6 +1338,36 @@ async fn resolve_owner(args: &Value, client: &NodeClient) -> Result { Ok(did.split(':').next_back().unwrap_or(did).to_string()) } +/// Resolve (owner, repo) for the webhook tools, matching the CLI's +/// `resolve_owner_repo_pair` contract: an `owner/repo` string splits first, an +/// explicit `owner` arg wins next, then the signing keypair's short DID. The +/// last resort is the node root DID via `resolve_owner`, reachable only by +/// callers that have no keypair (the unsigned webhook_list arm) — for a bare +/// repo name under a signed caller the path owner must be the caller's DID or +/// the node 403/404s the request under the wrong namespace. +async fn resolve_owner_repo( + args: &Value, + repo: &str, + keypair: &Option, + client: &NodeClient, +) -> Result<(String, String)> { + if let Some((o, r)) = repo.split_once('/') { + return Ok((o.to_string(), r.to_string())); + } + if let Some(o) = args.get("owner").and_then(|v| v.as_str()) { + return Ok((o.to_string(), repo.to_string())); + } + if let Some(kp) = keypair { + let did = kp.did().to_string(); + return Ok(( + did.split(':').next_back().unwrap_or(&did).to_string(), + repo.to_string(), + )); + } + let owner = resolve_owner(args, client).await?; + Ok((owner, repo.to_string())) +} + /// Parse git pkt-line info/refs response into a list of {ref, sha} objects. fn parse_info_refs(bytes: &[u8]) -> Value { let mut refs = Vec::new(); @@ -3161,4 +3184,65 @@ mod tests { assert!(err.to_string().contains("404"), "got: {err}"); _m.assert_async().await; } + + #[tokio::test] + async fn resolve_owner_repo_splits_slash_form() { + let server = mockito::Server::new_async().await; + let (owner, name) = resolve_owner_repo( + &json!({}), + "alice/proj", + &None, + &NodeClient::new(server.url(), None), + ) + .await + .unwrap(); + assert_eq!((owner.as_str(), name.as_str()), ("alice", "proj")); + } + + #[tokio::test] + async fn resolve_owner_repo_owner_arg_beats_keypair() { + let server = mockito::Server::new_async().await; + let kp = gitlawb_core::identity::Keypair::generate(); + let (owner, name) = resolve_owner_repo( + &json!({"owner": "bob"}), + "proj", + &Some(kp.clone()), + &NodeClient::new(server.url(), None), + ) + .await + .unwrap(); + assert_eq!((owner.as_str(), name.as_str()), ("bob", "proj")); + } + + #[tokio::test] + async fn resolve_owner_repo_bare_repo_uses_keypair_did() { + let mut server = mockito::Server::new_async().await; + // A root mock answering with a different DID must never be consulted: + // under a signed caller the path owner is the keypair's short DID. + let _m = server + .mock("GET", "/") + .with_status(200) + .with_body(r#"{"did":"did:key:z6MkNodeOperator"}"#) + .expect(0) + .create_async() + .await; + let kp = gitlawb_core::identity::Keypair::generate(); + let short = kp + .did() + .to_string() + .split(':') + .next_back() + .unwrap() + .to_string(); + let (owner, name) = resolve_owner_repo( + &json!({}), + "proj", + &Some(kp), + &NodeClient::new(server.url(), None), + ) + .await + .unwrap(); + assert_eq!((owner.as_str(), name.as_str()), (short.as_str(), "proj")); + _m.assert_async().await; + } } From 5baa17d26bfaa3312eac8d1c3a2bc4dc82060cd4 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Tue, 10 Nov 2026 09:28:21 -0600 Subject: [PATCH 5/5] test(gl): pin unsigned webhook_list node-denial fallback on MCP (#568) --- crates/gl/src/mcp.rs | 37 ++++++++++++++++++++++++++++++------- 1 file changed, 30 insertions(+), 7 deletions(-) diff --git a/crates/gl/src/mcp.rs b/crates/gl/src/mcp.rs index 0fa3cb824..f7aae0c01 100644 --- a/crates/gl/src/mcp.rs +++ b/crates/gl/src/mcp.rs @@ -1342,7 +1342,9 @@ async fn resolve_owner(args: &Value, client: &NodeClient) -> Result { /// `resolve_owner_repo_pair` contract: an `owner/repo` string splits first, an /// explicit `owner` arg wins next, then the signing keypair's short DID. The /// last resort is the node root DID via `resolve_owner`, reachable only by -/// callers that have no keypair (the unsigned webhook_list arm) — for a bare +/// callers that have no keypair — the unsigned `webhook_list` arm, and an +/// unsigned `webhook_delete` the same way `resolve_owner` always served it — +/// for a bare /// repo name under a signed caller the path owner must be the caller's DID or /// the node 403/404s the request under the wrong namespace. async fn resolve_owner_repo( @@ -2505,11 +2507,27 @@ mod tests { } #[tokio::test] - async fn test_webhook_list_without_keypair_errors() { - // No identity loaded and no explicit owner → the tool must error rather - // than fall back to the node DID and issue an unsigned, mis-scoped GET. - let server = mockito::Server::new_async().await; + async fn test_webhook_list_without_keypair_surfaces_node_denial() { + // No identity loaded and no explicit owner → the tool falls back to the + // node DID for the path and still issues the GET unsigned, so the node + // itself answers the denial (list_webhooks 401s a headerless caller) + // rather than the client inventing a failure before contact. + let mut server = mockito::Server::new_async().await; let dir = tempfile::TempDir::new().unwrap(); // empty: no identity.pem + let _root = server + .mock("GET", "/") + .with_status(200) + .with_header("content-type", "application/json") + .with_body(r#"{"did":"did:key:z6MkNodeOperator"}"#) + .create_async() + .await; + let _hooks = server + .mock("GET", "/api/v1/repos/z6MkNodeOperator/myrepo/hooks") + .with_status(401) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"signature required"}"#) + .create_async() + .await; let err = call_tool( "webhook_list", @@ -2518,8 +2536,13 @@ mod tests { Some(dir.path()), ) .await - .expect_err("must error without an identity"); - assert!(err.to_string().contains("no identity found"), "got: {err}"); + .expect_err("unsigned list must surface the node denial"); + let msg = err.to_string(); + assert!( + msg.contains("401") || msg.contains("signature required"), + "got: {msg}" + ); + _hooks.assert_async().await; } #[tokio::test]