hive-c0re: test verify_hmac guards and webhook status mapping
`verify_hmac` is the only authentication gate on the public `/webhook/config-pr` endpoint, and neither of its guard clauses nor the handler's "unavailable" → 503 / otherwise → 401 mapping had a test. The tests build a real `AppState` over the tempdir `Coordinator` that the socket_server schedule tests already use. That helper is widened from `pub(in crate::socket_server)` to `pub(crate)` and re-exported from `socket_server` under `#[cfg(test)]`, so non-test code is unchanged. Removing the missing-secret guard (replacing it with `unwrap_or_default()`) fails verify_hmac_rejects_every_delivery_when_no_secret_is_loaded and config_pr_answers_503_when_no_secret_is_loaded. Deleting the missing-header guard fails verify_hmac_rejects_a_delivery_with_no_usable_signature_header. Breaking the "unavailable" match fails the 503 test. Without the missing-header guard, an unsigned delivery is still refused by `verify_signature` (no `sha256=` prefix). The test therefore pins the guard's own message rather than the bare rejection. Closes #4654
This commit is contained in:
parent
7488c337cc
commit
a40c0026cf
3 changed files with 141 additions and 1 deletions
|
|
@ -222,3 +222,141 @@ pub(super) async fn post_webhook_config_pr(
|
||||||
|
|
||||||
(StatusCode::OK, "ok").into_response()
|
(StatusCode::OK, "ok").into_response()
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[cfg(test)]
|
||||||
|
mod tests {
|
||||||
|
use axum::{
|
||||||
|
body::Bytes,
|
||||||
|
extract::State,
|
||||||
|
http::{HeaderMap, HeaderValue, StatusCode},
|
||||||
|
};
|
||||||
|
|
||||||
|
use super::{AppState, post_webhook_config_pr, verify_hmac};
|
||||||
|
|
||||||
|
const SECRET: &str = "s3cr3t";
|
||||||
|
|
||||||
|
/// The `TempDir` holds the coordinator's sqlite files; keep it alive as
|
||||||
|
/// long as the state.
|
||||||
|
fn state(webhook_secret: Option<&str>) -> (tempfile::TempDir, AppState) {
|
||||||
|
let (dir, coord) = crate::socket_server::coordinator();
|
||||||
|
let state = AppState {
|
||||||
|
coord,
|
||||||
|
webhook_secret: webhook_secret.map(str::to_owned),
|
||||||
|
};
|
||||||
|
(dir, state)
|
||||||
|
}
|
||||||
|
|
||||||
|
/// The `X-Hub-Signature-256` header Forgejo would send for `secret` + `body`.
|
||||||
|
fn signed(secret: &str, body: &[u8]) -> HeaderMap {
|
||||||
|
use std::fmt::Write as _;
|
||||||
|
|
||||||
|
use hmac::{Hmac, KeyInit, Mac};
|
||||||
|
use sha2::Sha256;
|
||||||
|
let mut mac = Hmac::<Sha256>::new_from_slice(secret.as_bytes()).unwrap();
|
||||||
|
mac.update(body);
|
||||||
|
let mut hex = String::new();
|
||||||
|
for b in mac.finalize().into_bytes() {
|
||||||
|
write!(hex, "{b:02x}").unwrap();
|
||||||
|
}
|
||||||
|
let mut headers = HeaderMap::new();
|
||||||
|
headers.insert(
|
||||||
|
"x-hub-signature-256",
|
||||||
|
HeaderValue::from_str(&format!("sha256={hex}")).unwrap(),
|
||||||
|
);
|
||||||
|
headers
|
||||||
|
}
|
||||||
|
|
||||||
|
/// With no secret loaded, nothing verifies — including a delivery signed
|
||||||
|
/// with the empty key. The message must say "unavailable": the handler
|
||||||
|
/// maps on that word to 503.
|
||||||
|
#[test]
|
||||||
|
fn verify_hmac_rejects_every_delivery_when_no_secret_is_loaded() {
|
||||||
|
let (_dir, state) = state(None);
|
||||||
|
let body = Bytes::from_static(b"{}");
|
||||||
|
for (label, headers) in [
|
||||||
|
("signed with the empty key", signed("", &body)),
|
||||||
|
("signed with some key", signed(SECRET, &body)),
|
||||||
|
("unsigned", HeaderMap::new()),
|
||||||
|
] {
|
||||||
|
let err = verify_hmac(&state, &headers, &body).expect_err(label);
|
||||||
|
assert!(err.contains("unavailable"), "{label}: {err}");
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
/// An absent header, an empty one and one that is not valid UTF-8 all
|
||||||
|
/// read as "no signature", and are refused before any HMAC is computed.
|
||||||
|
#[test]
|
||||||
|
fn verify_hmac_rejects_a_delivery_with_no_usable_signature_header() {
|
||||||
|
let (_dir, state) = state(Some(SECRET));
|
||||||
|
let body = Bytes::from_static(b"{}");
|
||||||
|
let mut empty = HeaderMap::new();
|
||||||
|
empty.insert("x-hub-signature-256", HeaderValue::from_static(""));
|
||||||
|
let mut not_utf8 = HeaderMap::new();
|
||||||
|
not_utf8.insert(
|
||||||
|
"x-hub-signature-256",
|
||||||
|
HeaderValue::from_bytes(b"sha256=\xff").unwrap(),
|
||||||
|
);
|
||||||
|
for (label, headers) in [
|
||||||
|
("absent", HeaderMap::new()),
|
||||||
|
("empty", empty),
|
||||||
|
("not UTF-8", not_utf8),
|
||||||
|
] {
|
||||||
|
assert_eq!(
|
||||||
|
verify_hmac(&state, &headers, &body),
|
||||||
|
Err("missing X-Hub-Signature-256 header".to_owned()),
|
||||||
|
"{label}"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
/// The other side of both guards: a loaded secret and a matching
|
||||||
|
/// signature pass.
|
||||||
|
#[test]
|
||||||
|
fn verify_hmac_accepts_a_correctly_signed_delivery() {
|
||||||
|
let (_dir, state) = state(Some(SECRET));
|
||||||
|
let body = Bytes::from_static(b"{}");
|
||||||
|
assert_eq!(verify_hmac(&state, &signed(SECRET, &body), &body), Ok(()));
|
||||||
|
}
|
||||||
|
|
||||||
|
/// 503 means "this hive cannot verify any delivery", 401 means "this
|
||||||
|
/// delivery is not authentic".
|
||||||
|
#[tokio::test]
|
||||||
|
async fn config_pr_answers_503_when_no_secret_is_loaded() {
|
||||||
|
let (_dir, state) = state(None);
|
||||||
|
let body = Bytes::from_static(b"{}");
|
||||||
|
let headers = signed(SECRET, &body);
|
||||||
|
let resp = post_webhook_config_pr(State(state), headers, body).await;
|
||||||
|
assert_eq!(resp.status(), StatusCode::SERVICE_UNAVAILABLE);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[tokio::test]
|
||||||
|
async fn config_pr_answers_401_for_a_missing_or_wrong_signature() {
|
||||||
|
let body = Bytes::from_static(b"{}");
|
||||||
|
for (label, headers) in [
|
||||||
|
("missing", HeaderMap::new()),
|
||||||
|
("wrong secret", signed("different-secret", &body)),
|
||||||
|
("other body", signed(SECRET, b"tampered")),
|
||||||
|
] {
|
||||||
|
let (_dir, state) = state(Some(SECRET));
|
||||||
|
let resp = post_webhook_config_pr(State(state), headers, body.clone()).await;
|
||||||
|
assert_eq!(resp.status(), StatusCode::UNAUTHORIZED, "{label}");
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
/// A verified delivery reaches payload handling: an action the handler
|
||||||
|
/// ignores comes back 200, and an unparseable body 400, neither of which
|
||||||
|
/// is an auth status.
|
||||||
|
#[tokio::test]
|
||||||
|
async fn config_pr_lets_a_correctly_signed_delivery_through() {
|
||||||
|
for (body, expected) in [
|
||||||
|
(&br#"{"action":"closed"}"#[..], StatusCode::OK),
|
||||||
|
(&b"not json"[..], StatusCode::BAD_REQUEST),
|
||||||
|
] {
|
||||||
|
let (_dir, state) = state(Some(SECRET));
|
||||||
|
let body = Bytes::from_static(body);
|
||||||
|
let headers = signed(SECRET, &body);
|
||||||
|
let resp = post_webhook_config_pr(State(state), headers, body).await;
|
||||||
|
assert_eq!(resp.status(), expected);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
|
||||||
|
|
@ -27,6 +27,8 @@ mod schedules;
|
||||||
pub(crate) use config_approvals::submit_merge_config_pr;
|
pub(crate) use config_approvals::submit_merge_config_pr;
|
||||||
pub(crate) use schedules::filter_ghost_schedule_targets;
|
pub(crate) use schedules::filter_ghost_schedule_targets;
|
||||||
pub use schedules::schedule_to_wire_public;
|
pub use schedules::schedule_to_wire_public;
|
||||||
|
#[cfg(test)]
|
||||||
|
pub(crate) use schedules::tests::coordinator;
|
||||||
|
|
||||||
use schedules::{
|
use schedules::{
|
||||||
EditSchedulePatch, handle_cancel_schedule, handle_edit_schedule, handle_fire_schedule_now,
|
EditSchedulePatch, handle_cancel_schedule, handle_edit_schedule, handle_fire_schedule_now,
|
||||||
|
|
|
||||||
|
|
@ -380,7 +380,7 @@ pub(super) mod tests {
|
||||||
/// A real `Coordinator` over a throwaway sqlite dir. The socket-server
|
/// A real `Coordinator` over a throwaway sqlite dir. The socket-server
|
||||||
/// handlers take `&Arc<Coordinator>`, so there is no lighter way in;
|
/// handlers take `&Arc<Coordinator>`, so there is no lighter way in;
|
||||||
/// `open` touches nothing outside the db path it is handed.
|
/// `open` touches nothing outside the db path it is handed.
|
||||||
pub(in crate::socket_server) fn coordinator() -> (tempfile::TempDir, Arc<Coordinator>) {
|
pub(crate) fn coordinator() -> (tempfile::TempDir, Arc<Coordinator>) {
|
||||||
let dir = tempfile::tempdir().expect("tempdir");
|
let dir = tempfile::tempdir().expect("tempdir");
|
||||||
let coord = Coordinator::open(
|
let coord = Coordinator::open(
|
||||||
&dir.path().join("broker.sqlite"),
|
&dir.path().join("broker.sqlite"),
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue