From 86bb88cdca9787e5109bd20f14819bb9569bd644 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Mon, 20 Apr 2026 08:27:50 -0400 Subject: [PATCH] refactor(server): tidy webhook wiring and test-helper context - Distinguish the two GitHub webhook auth-failure warn messages (missing signature header vs. HMAC mismatch) so logs can tell them apart. - Route update_github_app_webhook through fabro_github::github_api_base_url() so GITHUB_BASE_URL overrides the webhook config endpoint too. - Drop a narrative shutdown comment that restated the next two lines. - Replace concat!(file!(), ":", line!()) inside local test-helper wrappers; those macros expand at the wrapper definition site, so every panic reported the same phantom location. Pass the wrapper name instead. Co-Authored-By: Claude Opus 4.7 (1M context) --- lib/crates/fabro-server/src/github_webhooks.rs | 3 ++- lib/crates/fabro-server/src/ip_allowlist.rs | 2 +- lib/crates/fabro-server/src/jwt_auth.rs | 4 ++-- lib/crates/fabro-server/src/serve.rs | 1 - lib/crates/fabro-server/src/server.rs | 12 ++++++------ lib/crates/fabro-server/src/web_auth.rs | 6 +++--- 6 files changed, 14 insertions(+), 14 deletions(-) diff --git a/lib/crates/fabro-server/src/github_webhooks.rs b/lib/crates/fabro-server/src/github_webhooks.rs index eeab3dd5a..99e326597 100644 --- a/lib/crates/fabro-server/src/github_webhooks.rs +++ b/lib/crates/fabro-server/src/github_webhooks.rs @@ -151,8 +151,9 @@ pub(crate) async fn update_github_app_webhook( "content_type": "json", }); + let url = format!("{}/app/hook/config", fabro_github::github_api_base_url()); let resp = client - .patch("https://api.github.com/app/hook/config") + .patch(&url) .header("Authorization", format!("Bearer {jwt}")) .header("Accept", "application/vnd.github+json") .header("User-Agent", "fabro") diff --git a/lib/crates/fabro-server/src/ip_allowlist.rs b/lib/crates/fabro-server/src/ip_allowlist.rs index ef32b90ed..fc4500a33 100644 --- a/lib/crates/fabro-server/src/ip_allowlist.rs +++ b/lib/crates/fabro-server/src/ip_allowlist.rs @@ -345,7 +345,7 @@ mod tests { } async fn assert_status(response: axum::response::Response, expected: StatusCode) { - assert_axum_status(response, expected, concat!(file!(), ":", line!())).await; + assert_axum_status(response, expected, "assert_status").await; } #[test] diff --git a/lib/crates/fabro-server/src/jwt_auth.rs b/lib/crates/fabro-server/src/jwt_auth.rs index 1ae755dd7..67e819e90 100644 --- a/lib/crates/fabro-server/src/jwt_auth.rs +++ b/lib/crates/fabro-server/src/jwt_auth.rs @@ -299,11 +299,11 @@ mod tests { } async fn response_json(response: axum::response::Response) -> serde_json::Value { - expect_axum_json(response, StatusCode::OK, concat!(file!(), ":", line!())).await + expect_axum_json(response, StatusCode::OK, "response_json").await } async fn assert_status(response: axum::response::Response, expected: StatusCode) { - assert_axum_status(response, expected, concat!(file!(), ":", line!())).await; + assert_axum_status(response, expected, "assert_status").await; } fn dev_token_mode() -> AuthMode { diff --git a/lib/crates/fabro-server/src/serve.rs b/lib/crates/fabro-server/src/serve.rs index 605c2b30f..873066419 100644 --- a/lib/crates/fabro-server/src/serve.rs +++ b/lib/crates/fabro-server/src/serve.rs @@ -649,7 +649,6 @@ where let _ = child.wait(); } - // Clean up webhook listener on shutdown if let Some(manager) = webhook_manager { manager.shutdown().await; } diff --git a/lib/crates/fabro-server/src/server.rs b/lib/crates/fabro-server/src/server.rs index 33702a8e8..4aa047255 100644 --- a/lib/crates/fabro-server/src/server.rs +++ b/lib/crates/fabro-server/src/server.rs @@ -1250,12 +1250,12 @@ async fn github_webhook( .get("x-hub-signature-256") .and_then(|value| value.to_str().ok()) else { - warn!(delivery = %delivery_id, "Webhook signature verification failed"); + warn!(delivery = %delivery_id, "Webhook missing X-Hub-Signature-256 header"); return StatusCode::UNAUTHORIZED; }; if !verify_signature(&secret, &body, signature) { - warn!(delivery = %delivery_id, "Webhook signature verification failed"); + warn!(delivery = %delivery_id, "Webhook HMAC signature mismatch"); return StatusCode::UNAUTHORIZED; } @@ -7416,25 +7416,25 @@ mod tests { } async fn assert_status(response: axum::response::Response, expected: StatusCode) { - assert_axum_status(response, expected, concat!(file!(), ":", line!())).await; + assert_axum_status(response, expected, "assert_status").await; } async fn checked_response( response: axum::response::Response, expected: StatusCode, ) -> axum::response::Response { - expect_axum_status(response, expected, concat!(file!(), ":", line!())).await + expect_axum_status(response, expected, "checked_response").await } async fn response_json( response: axum::response::Response, expected: StatusCode, ) -> serde_json::Value { - expect_axum_json(response, expected, concat!(file!(), ":", line!())).await + expect_axum_json(response, expected, "response_json").await } async fn response_bytes(response: axum::response::Response, expected: StatusCode) -> Vec { - expect_axum_bytes(response, expected, concat!(file!(), ":", line!())).await + expect_axum_bytes(response, expected, "response_bytes").await } fn api(path: &str) -> String { diff --git a/lib/crates/fabro-server/src/web_auth.rs b/lib/crates/fabro-server/src/web_auth.rs index ed348aff5..20ba8a2b1 100644 --- a/lib/crates/fabro-server/src/web_auth.rs +++ b/lib/crates/fabro-server/src/web_auth.rs @@ -740,18 +740,18 @@ mod tests { } async fn response_json(response: axum::response::Response) -> Value { - expect_axum_json(response, StatusCode::OK, concat!(file!(), ":", line!())).await + expect_axum_json(response, StatusCode::OK, "response_json").await } async fn assert_status(response: axum::response::Response, expected: StatusCode) { - assert_axum_status(response, expected, concat!(file!(), ":", line!())).await; + assert_axum_status(response, expected, "assert_status").await; } async fn checked_response( response: axum::response::Response, expected: StatusCode, ) -> axum::response::Response { - fabro_test::expect_axum_status(response, expected, concat!(file!(), ":", line!())).await + fabro_test::expect_axum_status(response, expected, "checked_response").await } #[tokio::test]