hive-c0re: pin the two security boundaries that had no test
`is_forbidden` and the webhook-secret load/regenerate path were the last two entries on the shortlist in hyperhive/hyperhive#3950; the other three landed in hyperhive/hyperhive#4650. Both are classification logic whose failure mode is silence, which is why they are worth a test rather than a coverage line. `is_forbidden` gates the arms that tell an operator a Forgejo admin PATCH was refused for want of a scope, and which credential to delete and re-mint to fix it. One of those PATCHes is `ensure_repo_creation_disabled` — the lockdown that stops an agent creating a repo it owns and self-merging in it. Two tests: a 403 is recognised in both shapes the typed client produces (the spec-listed `Forbidden` kind and the bare `UnexpectedStatusCode`), and nothing else is — not a 401, whose remedy is the automatic re-mint one function down, and not a transport error that never reached the forge at all. `load_or_generate` grows the path-taking half `load_or_generate_at`, the same seam `swarm-controller`'s `webhook::load_or_generate_at` already has and for the same stated reason. Three tests over it: a valid stored secret is returned verbatim and never rotated (the newline this module writes itself makes the trim load-bearing, not defensive); a malformed one is replaced by a secret that reaches *disk*, not just the caller, and is then stable; and each near miss — empty, whitespace, 63 chars, 65 chars, right length with a non-hex char — is refused. That last one is the security case: `Hmac::new_from_slice` accepts a key of any length, empty included, so a relaxed check fails nowhere and just keys every signature off a guessable value. Every test was confirmed able to fail: six mutations of the code under test, each watched red, then reverted. The two halves of the validity check and the two arms of `is_forbidden` were broken separately, so neither test passes on one arm alone.
This commit is contained in:
parent
e0673b6192
commit
2c066cc871
2 changed files with 149 additions and 6 deletions
|
|
@ -596,7 +596,7 @@ pub fn core_token() -> Option<String> {
|
|||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::{CoreTokenCheck, classify_core_token_error};
|
||||
use super::{CoreTokenCheck, classify_core_token_error, is_forbidden};
|
||||
use forgejo_api::{ApiError, ApiErrorKind, ForgejoError};
|
||||
use reqwest::StatusCode;
|
||||
|
||||
|
|
@ -656,4 +656,58 @@ mod tests {
|
|||
CoreTokenCheck::Indeterminate
|
||||
);
|
||||
}
|
||||
|
||||
/// The typed client only produces `ApiErrorKind::Forbidden` when the
|
||||
/// endpoint's spec lists 403; every other endpoint surfaces the same
|
||||
/// response as a bare `UnexpectedStatusCode`. Both shapes must be
|
||||
/// recognised, because the arms guarded by this predicate are what tell
|
||||
/// the operator that `ensure_repo_creation_disabled` — the lockdown
|
||||
/// stopping an agent from creating and self-merging in its own repo —
|
||||
/// did not apply, and which credential to re-mint to make it apply.
|
||||
/// Drop the second arm as "redundant" and that lockdown fails with a
|
||||
/// generic warning that names no remedy.
|
||||
#[test]
|
||||
fn a_403_is_recognised_in_both_shapes_the_client_can_produce() {
|
||||
assert!(is_forbidden(&api_err(ApiErrorKind::Forbidden)));
|
||||
assert!(is_forbidden(&ForgejoError::UnexpectedStatusCode(
|
||||
StatusCode::FORBIDDEN
|
||||
)));
|
||||
}
|
||||
|
||||
/// The other direction, and the more expensive one to get wrong: this
|
||||
/// predicate gates advice to **delete the core token and restart
|
||||
/// hive-c0re**. A widened match would hand that advice out for a 502
|
||||
/// from a restarting forge or a client-side error that never reached
|
||||
/// the network — destroying a working credential in response to a blip.
|
||||
/// 401 is called out separately because it is the near miss: it *is* a
|
||||
/// credential problem, but its remedy is [`classify_core_token_error`]'s
|
||||
/// automatic re-mint, so classifying it as forbidden buries the cause
|
||||
/// under a scope warning that does not apply.
|
||||
#[test]
|
||||
fn a_non_403_failure_is_not_reported_as_a_missing_admin_scope() {
|
||||
for kind in [
|
||||
ApiErrorKind::Unauthorized,
|
||||
ApiErrorKind::NotFound { errors: None },
|
||||
ApiErrorKind::ValidationFailed,
|
||||
ApiErrorKind::Generic,
|
||||
] {
|
||||
let e = api_err(kind);
|
||||
assert!(!is_forbidden(&e), "{e} must not read as forbidden");
|
||||
}
|
||||
for s in [
|
||||
StatusCode::UNAUTHORIZED,
|
||||
StatusCode::NOT_FOUND,
|
||||
StatusCode::BAD_GATEWAY,
|
||||
StatusCode::INTERNAL_SERVER_ERROR,
|
||||
] {
|
||||
assert!(
|
||||
!is_forbidden(&ForgejoError::UnexpectedStatusCode(s)),
|
||||
"status {s} must not read as forbidden"
|
||||
);
|
||||
}
|
||||
// A failure with no HTTP status at all — the request never got an
|
||||
// answer, so nothing has been said about the token's scopes.
|
||||
assert!(!is_forbidden(&ForgejoError::KeyNotAscii));
|
||||
assert!(!is_forbidden(&ForgejoError::HostRequired));
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue