Skip to content

fix(gl): resolve webhook owner from caller identity, not node DID #159

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
carlitos973 wants to merge 3 commits into
base: main
Choose a base branch
from
Open

fix(gl): resolve webhook owner from caller identity, not node DID #159

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4335535
fix(gl): resolve webhook owner from caller identity, not node DID
AlaskanScout Jul 6, 2026
d42a809
fix(gl): print fully-qualified owner/repo in webhook create delete hint
AlaskanScout Jul 6, 2026
e5b1b0c
fix(gl): thread identity resolution and status checking through webho…
AlaskanScout Jul 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
122 changes: 100 additions & 22 deletions crates/gl/src/webhook.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<PathBuf>,
},
/// Delete a webhook
Delete {
Expand All @@ -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,
Expand All @@ -76,13 +78,33 @@ pub async fn run(args: WebhookArgs) -> Result<()> {
}
}

async fn resolve_owner(client: &NodeClient) -> Result<String> {
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 if loading a local keypair fails for any reason
/// (missing, unreadable, or unparseable).
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(
Expand All @@ -95,7 +117,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?;

@coderabbitai coderabbitai Bot Jul 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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.

Suggested change
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.


Comment on lines 118 to 121
let event_list: Vec<&str> = events.split(',').map(str::trim).collect();

Expand Down Expand Up @@ -139,28 +161,33 @@ 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(())
}

async fn cmd_list(repo: String, node: String) -> Result<()> {
async fn cmd_list(repo: String, node: String, dir: Option<PathBuf>) -> Result<()> {
let client = NodeClient::new(&node, None);
let owner = resolve_owner(&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")?;

if !status.is_success() {
let msg = body["message"].as_str().unwrap_or("unknown error");
anyhow::bail!("list webhooks failed ({status}): {msg}");
}

let hooks = resp["webhooks"].as_array().cloned().unwrap_or_default();
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("?");
Expand All @@ -186,7 +213,7 @@ async fn cmd_list(repo: String, node: String) -> Result<()> {
async fn cmd_delete(repo: String, id: String, node: String, dir: Option<PathBuf>) -> 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
Expand Down Expand Up @@ -347,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]
Expand All @@ -363,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 ───────────────────────────────────────────────────────
Expand Down
Loading