diff --git a/Cargo.lock b/Cargo.lock index e172318e..fe6226ef 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -8211,6 +8211,7 @@ dependencies = [ "anyhow", "arithmetic", "axum", + "axum-extra", "bls", "builder_api", "cached", diff --git a/grandine-snapshot-tests b/grandine-snapshot-tests index c09e1cfa..1ded5ee9 160000 --- a/grandine-snapshot-tests +++ b/grandine-snapshot-tests @@ -1 +1 @@ -Subproject commit c09e1cfa56a3c3cfc2502f8650a92db8f66a9e7d +Subproject commit 1ded5ee9eed683ee965c190f44d5ed2ee660b93d diff --git a/http_api/src/error.rs b/http_api/src/error.rs index 90537f88..ba002e46 100644 --- a/http_api/src/error.rs +++ b/http_api/src/error.rs @@ -3,10 +3,12 @@ use std::error::Error as StdError; use anyhow::Error as AnyhowError; use axum::{ + extract::rejection::JsonRejection, http::StatusCode, response::{IntoResponse, Response}, Extension, Json, }; +use axum_extra::extract::QueryRejection; use bls::SignatureBytes; use futures::channel::oneshot::Canceled; use itertools::Itertools as _; @@ -69,7 +71,7 @@ pub enum Error { #[error("invalid epoch")] InvalidEpoch(#[source] AnyhowError), #[error("invalid JSON body")] - InvalidJsonBody(#[source] AnyhowError), + InvalidJsonBody(#[source] JsonRejection), #[error("invalid peer ID")] InvalidPeerId(#[source] AnyhowError), #[error( @@ -84,13 +86,13 @@ pub enum Error { #[error("invalid proposer slashing, it will never pass validation so it's rejected")] InvalidProposerSlashing(#[source] AnyhowError), #[error("invalid query string")] - InvalidQuery(#[source] AnyhowError), + InvalidQuery(#[source] QueryRejection), #[error("invalid voluntary exit, it will never pass validation so it's rejected")] InvalidSignedVoluntaryExit(#[source] AnyhowError), #[error("invalid sync committee messages")] InvalidSyncCommitteeMessages(Vec), - #[error("invalid validator index")] - InvalidValidatorIndex(#[source] AnyhowError), + #[error("invalid validator indices")] + InvalidValidatorIndices(#[source] JsonRejection), #[error("invalid validator signatures")] InvalidValidatorSignatures(Vec), #[error("invalid BLS to execution changes")] @@ -160,16 +162,26 @@ impl Error { // `StdError::sources` is not stable as of Rust 1.78.0. fn sources(&self) -> impl Iterator { - let mut error: Option<&dyn StdError> = Some(self); + let first: &dyn StdError = self; - core::iter::from_fn(move || { - let source = error?.source(); - core::mem::replace(&mut error, source) - }) + let skip_duplicates = || match self { + Self::InvalidJsonBody(_) | Self::InvalidValidatorIndices(_) => first.source()?.source(), + Self::InvalidQuery(_) => first.source()?.source()?.source(), + _ => first.source(), + }; + + let mut next = skip_duplicates(); + + core::iter::once(first).chain(core::iter::from_fn(move || { + let source = next?.source(); + core::mem::replace(&mut next, source) + })) } - const fn status_code(&self) -> StatusCode { + fn status_code(&self) -> StatusCode { match self { + Self::InvalidJsonBody(json_rejection) + | Self::InvalidValidatorIndices(json_rejection) => json_rejection.status(), Self::AttestationNotFound | Self::BlockNotFound | Self::MatchingAttestationHeadBlockNotFound @@ -191,7 +203,6 @@ impl Error { | Self::InvalidBlockId(_) | Self::InvalidContributionAndProofs(_) | Self::InvalidEpoch(_) - | Self::InvalidJsonBody(_) | Self::InvalidQuery(_) | Self::InvalidPeerId(_) | Self::InvalidProposerSlashing(_) @@ -201,7 +212,6 @@ impl Error { | Self::InvalidSyncCommitteeMessages(_) | Self::InvalidRandaoReveal | Self::InvalidValidatorId(_) - | Self::InvalidValidatorIndex(_) | Self::InvalidValidatorSignatures(_) | Self::ProposalSlotNotLaterThanStateSlot | Self::SlotNotInEpoch @@ -266,6 +276,7 @@ struct EthErrorResponse<'error> { #[allow(clippy::needless_pass_by_value)] #[cfg(test)] mod tests { + use axum::{extract::rejection::MissingJsonContentType, Error as AxumError}; use serde_json::{json, Result, Value}; use test_case::test_case; @@ -299,4 +310,34 @@ mod tests { assert_eq!(actual_json, expected_json); Ok(()) } + + // `axum::extract::rejection::JsonRejection` duplicates error sources. + // The underlying bug in `axum-core` was fixed in . + // The fix was backported in but not released. + #[test] + fn error_sources_does_not_yield_duplicates_from_json_rejection() { + let error = Error::InvalidJsonBody(JsonRejection::from(MissingJsonContentType::default())); + + assert_eq!( + error.sources().map(ToString::to_string).collect_vec(), + [ + "invalid JSON body", + "Expected request with `Content-Type: application/json`", + ], + ); + } + + // `axum_extra::extract::QueryRejection` and `axum::Error` duplicate error sources. + // `axum::Error` has not been fixed in any version yet. + // `axum::extract::rejection::QueryRejection` is even worse and harder to work around. + #[test] + fn error_sources_does_not_yield_triplicates_from_query_rejection() { + let axum_error = AxumError::new("error"); + let error = Error::InvalidQuery(QueryRejection::FailedToDeserializeQueryString(axum_error)); + + assert_eq!( + error.sources().map(ToString::to_string).collect_vec(), + ["invalid query string", "error"], + ); + } } diff --git a/http_api/src/extractors.rs b/http_api/src/extractors.rs index 6da3ec69..78a7db48 100644 --- a/http_api/src/extractors.rs +++ b/http_api/src/extractors.rs @@ -160,7 +160,6 @@ impl FromRequestParts for EthQuery { .extract() .await .map(|Query(query)| Self(query)) - .map_err(AnyhowError::msg) .map_err(Error::InvalidQuery) } } @@ -177,8 +176,7 @@ impl FromRequest for EthJson> { .extract() .await .map(|Json(slashing)| Self(slashing)) - .map_err(AnyhowError::new) - .map_err(Error::InvalidProposerSlashing) + .map_err(Error::InvalidJsonBody) } } @@ -191,8 +189,7 @@ impl FromRequest for EthJson> { .extract() .await .map(|Json(slashing)| Self(slashing)) - .map_err(AnyhowError::new) - .map_err(Error::InvalidSignedVoluntaryExit) + .map_err(Error::InvalidJsonBody) } } @@ -205,8 +202,7 @@ impl FromRequest for EthJson>> { .extract() .await .map(|Json(slashing)| Self(slashing)) - .map_err(AnyhowError::new) - .map_err(Error::InvalidAttesterSlashing) + .map_err(Error::InvalidJsonBody) } } @@ -219,7 +215,6 @@ impl FromRequest for EthJson>>> { .extract() .await .map(|Json(attestation)| Self(attestation)) - .map_err(AnyhowError::new) .map_err(Error::InvalidJsonBody) } } @@ -233,7 +228,6 @@ impl FromRequest for EthJson> { .extract() .await .map(|Json(values)| Self(values)) - .map_err(AnyhowError::new) .map_err(Error::InvalidJsonBody) } } @@ -250,8 +244,7 @@ impl FromRequest for EthJson> { .extract() .await .map(|Json(Wrapper(indices))| Self(indices)) - .map_err(AnyhowError::new) - .map_err(Error::InvalidValidatorIndex) + .map_err(Error::InvalidValidatorIndices) } } @@ -264,8 +257,7 @@ impl FromRequest for EthJson> { .extract() .await .map(|Json(indices)| Self(indices)) - .map_err(AnyhowError::new) - .map_err(Error::InvalidValidatorId) + .map_err(Error::InvalidJsonBody) } } @@ -278,7 +270,6 @@ impl FromRequest for EthJson> { .extract() .await .map(|Json(subscription)| Self(subscription)) - .map_err(AnyhowError::new) .map_err(Error::InvalidJsonBody) } } @@ -292,7 +283,6 @@ impl FromRequest for EthJson> { .extract() .await .map(|Json(subscription)| Self(subscription)) - .map_err(AnyhowError::new) .map_err(Error::InvalidJsonBody) } } @@ -306,7 +296,6 @@ impl FromRequest for EthJson FromRequest for EthJson FromRequest for EthJson> { .extract() .await .map(|Json(proposer_data)| Self(proposer_data)) - .map_err(AnyhowError::new) .map_err(Error::InvalidJsonBody) } } @@ -348,7 +335,6 @@ impl FromRequest for EthJson> { .extract() .await .map(|Json(registrations)| Self(registrations)) - .map_err(AnyhowError::new) .map_err(Error::InvalidJsonBody) } } diff --git a/validator/Cargo.toml b/validator/Cargo.toml index cd4ab32f..db713f5d 100644 --- a/validator/Cargo.toml +++ b/validator/Cargo.toml @@ -7,9 +7,10 @@ authors = ["Grandine "] workspace = true [dependencies] -axum = { workspace = true } anyhow = { workspace = true } arithmetic = { workspace = true } +axum = { workspace = true } +axum-extra = { workspace = true } bls = { workspace = true } builder_api = { workspace = true } cached = { workspace = true } diff --git a/validator/src/api.rs b/validator/src/api.rs index abcc7aed..11e9dd29 100644 --- a/validator/src/api.rs +++ b/validator/src/api.rs @@ -13,7 +13,10 @@ use anyhow::{ensure, Error as AnyhowError, Result}; use axum::{ async_trait, body::Body, - extract::{FromRef, FromRequest, FromRequestParts, Path as RequestPath, Query, State}, + extract::{ + rejection::JsonRejection, FromRef, FromRequest, FromRequestParts, Path as RequestPath, + State, + }, headers::{authorization::Bearer, Authorization}, http::{request::Parts, Request, StatusCode}, middleware::Next, @@ -21,6 +24,7 @@ use axum::{ routing::{delete, get, post}, Json, RequestExt as _, RequestPartsExt as _, Router, Server, TypedHeader, }; +use axum_extra::extract::{Query, QueryRejection}; use bls::PublicKeyBytes; use constant_time_eq::constant_time_eq; use directories::Directories; @@ -86,11 +90,11 @@ enum Error { #[error("internal error")] Internal(#[from] AnyhowError), #[error("invalid JSON body")] - InvalidJsonBody(#[source] AnyhowError), + InvalidJsonBody(#[source] JsonRejection), #[error("invalid public key")] InvalidPublicKey(#[source] AnyhowError), #[error("invalid query string")] - InvalidQuery(#[source] AnyhowError), + InvalidQuery(#[source] QueryRejection), #[error("authentication error")] Unauthorized, #[error("validator {pubkey:?} not found")] @@ -115,11 +119,10 @@ impl Error { ErrorResponse { message: self } } - const fn status_code(&self) -> StatusCode { + fn status_code(&self) -> StatusCode { match self { - Self::InvalidJsonBody(_) | Self::InvalidPublicKey(_) | Self::InvalidQuery(_) => { - StatusCode::BAD_REQUEST - } + Self::InvalidJsonBody(json_rejection) => json_rejection.status(), + Self::InvalidPublicKey(_) | Self::InvalidQuery(_) => StatusCode::BAD_REQUEST, Self::ValidatorNotFound { pubkey: _ } | Self::ValidatorNotOwned { pubkey: _ } => { StatusCode::NOT_FOUND } @@ -139,12 +142,20 @@ impl Error { // `StdError::sources` is not stable as of Rust 1.78.0. fn sources(&self) -> impl Iterator { - let mut error: Option<&dyn StdError> = Some(self); + let first: &dyn StdError = self; - core::iter::from_fn(move || { - let source = error?.source(); - core::mem::replace(&mut error, source) - }) + let skip_duplicates = || match self { + Self::InvalidJsonBody(_) => first.source()?.source(), + Self::InvalidQuery(_) => first.source()?.source()?.source(), + _ => first.source(), + }; + + let mut next = skip_duplicates(); + + core::iter::once(first).chain(core::iter::from_fn(move || { + let source = next?.source(); + core::mem::replace(&mut next, source) + })) } } @@ -214,7 +225,6 @@ impl FromRequest for EthJson { .extract() .await .map(|Json(query)| Self(query)) - .map_err(AnyhowError::new) .map_err(Error::InvalidJsonBody) } } @@ -228,7 +238,6 @@ impl FromRequest for EthJson { .extract() .await .map(|Json(query)| Self(query)) - .map_err(AnyhowError::new) .map_err(Error::InvalidJsonBody) } } @@ -242,7 +251,6 @@ impl FromRequest for EthJson { .extract() .await .map(|Json(query)| Self(query)) - .map_err(AnyhowError::new) .map_err(Error::InvalidJsonBody) } } @@ -256,7 +264,6 @@ impl FromRequest for EthJson { .extract() .await .map(|Json(query)| Self(query)) - .map_err(AnyhowError::new) .map_err(Error::InvalidJsonBody) } } @@ -270,7 +277,6 @@ impl FromRequest for EthJson { .extract() .await .map(|Json(query)| Self(query)) - .map_err(AnyhowError::new) .map_err(Error::InvalidJsonBody) } } @@ -284,7 +290,6 @@ impl FromRequest for EthJson { .extract() .await .map(|Json(query)| Self(query)) - .map_err(AnyhowError::new) .map_err(Error::InvalidJsonBody) } } @@ -298,7 +303,6 @@ impl FromRequest for EthJson { .extract() .await .map(|Json(query)| Self(query)) - .map_err(AnyhowError::new) .map_err(Error::InvalidJsonBody) } } @@ -333,7 +337,6 @@ impl FromRequestParts for EthQuery { .extract() .await .map(|Query(query)| Self(query)) - .map_err(AnyhowError::msg) .map_err(Error::InvalidQuery) } } @@ -878,6 +881,7 @@ impl ApiToken { #[cfg(test)] mod tests { use anyhow::Result as AnyhowResult; + use axum::{extract::rejection::MissingJsonContentType, Error as AxumError}; use serde_json::{json, Result, Value}; use tempfile::{Builder, NamedTempFile}; use test_case::test_case; @@ -953,6 +957,36 @@ mod tests { Ok(()) } + // `axum::extract::rejection::JsonRejection` duplicates error sources. + // The underlying bug in `axum-core` was fixed in . + // The fix was backported in but not released. + #[test] + fn error_sources_does_not_yield_duplicates_from_json_rejection() { + let error = Error::InvalidJsonBody(JsonRejection::from(MissingJsonContentType::default())); + + assert_eq!( + error.sources().map(ToString::to_string).collect_vec(), + [ + "invalid JSON body", + "Expected request with `Content-Type: application/json`", + ], + ); + } + + // `axum_extra::extract::QueryRejection` and `axum::Error` duplicate error sources. + // `axum::Error` has not been fixed in any version yet. + // `axum::extract::rejection::QueryRejection` is even worse and harder to work around. + #[test] + fn error_sources_does_not_yield_triplicates_from_query_rejection() { + let axum_error = AxumError::new("error"); + let error = Error::InvalidQuery(QueryRejection::FailedToDeserializeQueryString(axum_error)); + + assert_eq!( + error.sources().map(ToString::to_string).collect_vec(), + ["invalid query string", "error"], + ); + } + fn token_file() -> AnyhowResult { Ok(Builder::new().tempfile()?) }