fix(#2164): address argus review — Option<String> secret + stale hook cleanup
Two issues flagged by argus in PR #2388 review: 1. Empty-key fallback: when load_or_generate() failed, webhook_secret was String::new(). An attacker knowing this could forge deliveries with a valid HMAC of the empty key. Fix: change to Option<String>; on None, skip hook registration entirely and return 503 from /webhook/* handlers (rather than 401 with a misleadingly-verifiable empty-key HMAC). 2. Stale hook cleanup: on upgrade from old code, old loopback hooks (http://127.0.0.1:.../webhook/knowledge, .../webhook/config-pr) were left alongside the new domain-URL hook. Fix: during ensure_webhook / ensure_config_pr_webhook, after listing hooks, delete any that end with our path suffix but point at a different base URL. clippy + nix fmt clean.
This commit is contained in:
parent
79a29873e3
commit
7b1b1d9db8
5 changed files with 84 additions and 12 deletions
|
|
@ -358,6 +358,30 @@ pub async fn ensure_config_pr_webhook(
|
|||
tracing::debug!(%target_url, "forge: config-pr webhook already configured");
|
||||
return Ok(());
|
||||
}
|
||||
// Delete stale hooks that point at our path but a different base
|
||||
// (e.g. old loopback hooks from before the SSRF-bypass migration).
|
||||
for h in &hooks {
|
||||
let hook_url = h
|
||||
.config
|
||||
.as_ref()
|
||||
.and_then(|c| c.get("url"))
|
||||
.map_or("", String::as_str);
|
||||
if hook_url.ends_with("/webhook/config-pr")
|
||||
&& hook_url != target_url
|
||||
&& let Some(id) = h.id
|
||||
{
|
||||
tracing::info!(
|
||||
hook_url,
|
||||
org = CONFIG_ORG,
|
||||
"forge: deleting stale config-pr webhook (wrong base)"
|
||||
);
|
||||
let _ = tokio::time::timeout(
|
||||
HTTP_TIMEOUT,
|
||||
client.org_delete_hook(CONFIG_ORG, id).send(),
|
||||
)
|
||||
.await;
|
||||
}
|
||||
}
|
||||
}
|
||||
Err(e) => {
|
||||
tracing::debug!(error = %e, "forge: listing config-pr hooks failed; attempting create");
|
||||
|
|
|
|||
Loading…
Reference in a new issue