From 2fff3992e89ca4dad1d6f09c8ce89cefea1c5dc9 Mon Sep 17 00:00:00 2001 From: weekday Date: Tue, 18 Jun 2024 21:43:24 +0000 Subject: [PATCH] Return HTTP status code 415 from endpoints that expect JSON (#26) axum::Json already does that, but our EthJson and Error were replacing its status codes with 400. The solution was to store JsonRejection directly in Error and delegate to it in Error::status_code. Doing so also makes endpoints return status codes 413 and 422 when appropriate. The duplicated rejection messages came up again. We were able to determine the cause and come up with a better workaround this time. The issue should be fixed for query strings too. --- Cargo.lock | 1 + grandine-snapshot-tests | 2 +- http_api/src/error.rs | 65 ++++++++++++++++++++++++++------- http_api/src/extractors.rs | 24 +++---------- validator/Cargo.toml | 3 +- validator/src/api.rs | 74 +++++++++++++++++++++++++++----------- 6 files changed, 116 insertions(+), 53 deletions(-) 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()?) }