From 056a2a7ac52155c19cb29556a55172cd17d5c2ad Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Mon, 13 Feb 2023 19:45:17 +0000 Subject: [PATCH] fix(http): [#184] bencoded error responses for announce request HTTP tracker error responser must be bencoded. Fixed in the new Axum implementation. --- src/http/axum_implementation/query.rs | 2 +- .../axum_implementation/requests/announce.rs | 55 +++++++++++----- .../axum_implementation/responses/error.rs | 40 ++++++++++++ src/http/axum_implementation/responses/mod.rs | 1 + tests/http/asserts.rs | 62 ++++++++++++++++--- tests/http_tracker.rs | 53 +++++++++------- 6 files changed, 163 insertions(+), 50 deletions(-) create mode 100644 src/http/axum_implementation/responses/error.rs diff --git a/src/http/axum_implementation/query.rs b/src/http/axum_implementation/query.rs index c7c20b22d..3c9c676f1 100644 --- a/src/http/axum_implementation/query.rs +++ b/src/http/axum_implementation/query.rs @@ -45,7 +45,7 @@ impl FromStr for Param { fn from_str(raw_param: &str) -> Result { let pair = raw_param.split('=').collect::>(); - if pair.len() > 2 { + if pair.len() != 2 { return Err(ParseQueryError::InvalidParam { location: Location::caller(), raw_param: raw_param.to_owned(), diff --git a/src/http/axum_implementation/requests/announce.rs b/src/http/axum_implementation/requests/announce.rs index 004301744..34a9ad98a 100644 --- a/src/http/axum_implementation/requests/announce.rs +++ b/src/http/axum_implementation/requests/announce.rs @@ -4,10 +4,11 @@ use std::str::FromStr; use axum::async_trait; use axum::extract::FromRequestParts; use axum::http::request::Parts; -use axum::http::StatusCode; +use axum::response::{IntoResponse, Response}; use thiserror::Error; -use crate::http::axum_implementation::query::Query; +use crate::http::axum_implementation::query::{ParseQueryError, Query}; +use crate::http::axum_implementation::responses; use crate::http::percent_encoding::{percent_decode_info_hash, percent_decode_peer_id}; use crate::protocol::info_hash::{ConversionError, InfoHash}; use crate::tracker::peer::{self, IdConversionError}; @@ -23,17 +24,17 @@ pub struct Announce { #[derive(Error, Debug)] pub enum ParseAnnounceQueryError { - #[error("missing infohash {location}")] + #[error("missing info_hash param: {location}")] MissingInfoHash { location: &'static Location<'static> }, - #[error("invalid infohash {location}")] + #[error("invalid info_hash param: {location}")] InvalidInfoHash { location: &'static Location<'static> }, - #[error("missing peer id {location}")] + #[error("missing peer_id param: {location}")] MissingPeerId { location: &'static Location<'static> }, - #[error("invalid peer id {location}")] + #[error("invalid peer_id param: {location}")] InvalidPeerId { location: &'static Location<'static> }, - #[error("missing port {location}")] + #[error("missing port param: {location}")] MissingPort { location: &'static Location<'static> }, - #[error("invalid port {location}")] + #[error("invalid port param: {location}")] InvalidPort { location: &'static Location<'static> }, } @@ -49,12 +50,31 @@ impl From for ParseAnnounceQueryError { impl From for ParseAnnounceQueryError { #[track_caller] fn from(_err: ConversionError) -> Self { - Self::InvalidPeerId { + Self::InvalidInfoHash { location: Location::caller(), } } } +impl From for responses::error::Error { + fn from(err: ParseQueryError) -> Self { + responses::error::Error { + // code-review: should we expose error location in public HTTP tracker API? + // Error message example: "Cannot parse query params: invalid param a=b=c in src/http/axum_implementation/query.rs:50:27" + failure_reason: format!("Cannot parse query params: {err}"), + } + } +} + +impl From for responses::error::Error { + fn from(err: ParseAnnounceQueryError) -> Self { + responses::error::Error { + // code-review: should we expose error location in public HTTP tracker API? + failure_reason: format!("Cannot parse query params for announce request: {err}"), + } + } +} + impl TryFrom for Announce { type Error = ParseAnnounceQueryError; @@ -107,27 +127,28 @@ impl FromRequestParts for ExtractAnnounceRequest where S: Send + Sync, { - type Rejection = (StatusCode, &'static str); + type Rejection = Response; async fn from_request_parts(parts: &mut Parts, _state: &S) -> Result { - // todo: error responses body should be bencoded - let raw_query = parts.uri.query(); if raw_query.is_none() { - return Err((StatusCode::BAD_REQUEST, "missing query params")); + return Err(responses::error::Error { + failure_reason: "missing query params for announce request".to_string(), + } + .into_response()); } let query = raw_query.unwrap().parse::(); - if query.is_err() { - return Err((StatusCode::BAD_REQUEST, "can't parse query params")); + if let Err(error) = query { + return Err(responses::error::Error::from(error).into_response()); } let announce_request = Announce::try_from(query.unwrap()); - if announce_request.is_err() { - return Err((StatusCode::BAD_REQUEST, "can't parse query params for announce request")); + if let Err(error) = announce_request { + return Err(responses::error::Error::from(error).into_response()); } Ok(ExtractAnnounceRequest(announce_request.unwrap())) diff --git a/src/http/axum_implementation/responses/error.rs b/src/http/axum_implementation/responses/error.rs new file mode 100644 index 000000000..bcf2aaa57 --- /dev/null +++ b/src/http/axum_implementation/responses/error.rs @@ -0,0 +1,40 @@ +use axum::http::StatusCode; +use axum::response::{IntoResponse, Response}; +use serde::{self, Serialize}; + +#[derive(Serialize)] +pub struct Error { + #[serde(rename = "failure reason")] + pub failure_reason: String, +} + +impl Error { + /// # Panics + /// + /// It would panic if the `Error` struct contained an inappropriate type. + #[must_use] + pub fn write(&self) -> String { + serde_bencode::to_string(&self).unwrap() + } +} + +impl IntoResponse for Error { + fn into_response(self) -> Response { + (StatusCode::OK, self.write()).into_response() + } +} + +#[cfg(test)] +mod tests { + + use super::Error; + + #[test] + fn http_tracker_errors_can_be_bencoded() { + let err = Error { + failure_reason: "error message".to_owned(), + }; + + assert_eq!(err.write(), "d14:failure reason13:error messagee"); // cspell:disable-line + } +} diff --git a/src/http/axum_implementation/responses/mod.rs b/src/http/axum_implementation/responses/mod.rs index a493c2ac2..c566f14f3 100644 --- a/src/http/axum_implementation/responses/mod.rs +++ b/src/http/axum_implementation/responses/mod.rs @@ -1 +1,2 @@ pub mod ok; +pub mod error; diff --git a/tests/http/asserts.rs b/tests/http/asserts.rs index 8a1e2b554..146570f26 100644 --- a/tests/http/asserts.rs +++ b/tests/http/asserts.rs @@ -6,7 +6,7 @@ use super::responses::announce::{Announce, Compact, DeserializedCompact}; use super::responses::scrape; use crate::http::responses::error::Error; -pub fn assert_error_bencoded(response_text: &String, expected_failure_reason: &str, location: &'static Location<'static>) { +pub fn assert_bencoded_error(response_text: &String, expected_failure_reason: &str, location: &'static Location<'static>) { let error_failure_reason = serde_bencode::from_str::(response_text) .unwrap_or_else(|_| panic!( "response body should be a valid bencoded string for the '{expected_failure_reason}' error, got \"{response_text}\"" @@ -18,7 +18,7 @@ pub fn assert_error_bencoded(response_text: &String, expected_failure_reason: &s error_failure_reason.contains(expected_failure_reason), r#": response: `"{error_failure_reason}"` - dose not contain: `"{expected_failure_reason}"`, {location}"# + does not contain: `"{expected_failure_reason}"`, {location}"# ); } @@ -83,13 +83,13 @@ pub async fn assert_is_announce_response(response: Response) { pub async fn assert_internal_server_error_response(response: Response) { assert_eq!(response.status(), 200); - assert_error_bencoded(&response.text().await.unwrap(), "internal server", Location::caller()); + assert_bencoded_error(&response.text().await.unwrap(), "internal server", Location::caller()); } pub async fn assert_invalid_info_hash_error_response(response: Response) { assert_eq!(response.status(), 200); - assert_error_bencoded( + assert_bencoded_error( &response.text().await.unwrap(), "no valid infohashes found", Location::caller(), @@ -99,7 +99,7 @@ pub async fn assert_invalid_info_hash_error_response(response: Response) { pub async fn assert_invalid_peer_id_error_response(response: Response) { assert_eq!(response.status(), 200); - assert_error_bencoded( + assert_bencoded_error( &response.text().await.unwrap(), "peer_id is either missing or invalid", Location::caller(), @@ -109,13 +109,13 @@ pub async fn assert_invalid_peer_id_error_response(response: Response) { pub async fn assert_torrent_not_in_whitelist_error_response(response: Response) { assert_eq!(response.status(), 200); - assert_error_bencoded(&response.text().await.unwrap(), "is not whitelisted", Location::caller()); + assert_bencoded_error(&response.text().await.unwrap(), "is not whitelisted", Location::caller()); } pub async fn assert_peer_not_authenticated_error_response(response: Response) { assert_eq!(response.status(), 200); - assert_error_bencoded( + assert_bencoded_error( &response.text().await.unwrap(), "The peer is not authenticated", Location::caller(), @@ -125,13 +125,13 @@ pub async fn assert_peer_not_authenticated_error_response(response: Response) { pub async fn assert_invalid_authentication_key_error_response(response: Response) { assert_eq!(response.status(), 200); - assert_error_bencoded(&response.text().await.unwrap(), "is not valid", Location::caller()); + assert_bencoded_error(&response.text().await.unwrap(), "is not valid", Location::caller()); } pub async fn assert_could_not_find_remote_address_on_xff_header_error_response(response: Response) { assert_eq!(response.status(), 200); - assert_error_bencoded( + assert_bencoded_error( &response.text().await.unwrap(), "could not find remote address: must have a x-forwarded-for when using a reverse proxy", Location::caller(), @@ -141,9 +141,51 @@ pub async fn assert_could_not_find_remote_address_on_xff_header_error_response(r pub async fn assert_invalid_remote_address_on_xff_header_error_response(response: Response) { assert_eq!(response.status(), 200); - assert_error_bencoded( + assert_bencoded_error( &response.text().await.unwrap(), "could not find remote address: on remote proxy and unable to parse the last x-forwarded-ip", Location::caller(), ); } + +// Specific errors for announce requests + +pub async fn assert_missing_query_params_for_announce_request_error_response(response: Response) { + assert_eq!(response.status(), 200); + + assert_bencoded_error( + &response.text().await.unwrap(), + "missing query params for announce request", + Location::caller(), + ); +} + +pub async fn assert_cannot_parse_query_param_error_response(response: Response, failure: &str) { + assert_eq!(response.status(), 200); + + assert_bencoded_error( + &response.text().await.unwrap(), + &format!("Cannot parse query params: {failure}"), + Location::caller(), + ); +} + +pub async fn assert_cannot_parse_query_params_error_response(response: Response) { + assert_eq!(response.status(), 200); + + assert_bencoded_error( + &response.text().await.unwrap(), + "Cannot parse query params", + Location::caller(), + ); +} + +pub async fn assert_bad_announce_request_error_response(response: Response, failure: &str) { + assert_eq!(response.status(), 200); + + assert_bencoded_error( + &response.text().await.unwrap(), + &format!("Cannot parse query params for announce request: {failure}"), + Location::caller(), + ); +} diff --git a/tests/http_tracker.rs b/tests/http_tracker.rs index 409c5d343..84bac6e76 100644 --- a/tests/http_tracker.rs +++ b/tests/http_tracker.rs @@ -110,7 +110,7 @@ mod warp_http_tracker_server { } #[tokio::test] - async fn should_fail_when_the_request_is_empty() { + async fn should_fail_when_the_url_query_component_is_empty() { let http_tracker_server = start_default_http_tracker(Version::Warp).await; let response = Client::new(http_tracker_server.get_connection_info()).get("announce").await; @@ -1351,9 +1351,10 @@ mod axum_http_tracker_server { use crate::common::fixtures::{invalid_info_hashes, PeerBuilder}; use crate::http::asserts::{ - assert_announce_response, assert_compact_announce_response, assert_empty_announce_response, - assert_internal_server_error_response, assert_invalid_info_hash_error_response, - assert_invalid_peer_id_error_response, assert_is_announce_response, + assert_announce_response, assert_bad_announce_request_error_response, + assert_cannot_parse_query_param_error_response, assert_cannot_parse_query_params_error_response, + assert_compact_announce_response, assert_empty_announce_response, assert_internal_server_error_response, + assert_is_announce_response, assert_missing_query_params_for_announce_request_error_response, }; use crate::http::client::Client; use crate::http::requests::announce::{Compact, QueryBuilder}; @@ -1380,18 +1381,29 @@ mod axum_http_tracker_server { assert_is_announce_response(response).await; } - //#[tokio::test] - #[allow(dead_code)] - async fn should_fail_when_the_request_is_empty() { + #[tokio::test] + async fn should_fail_when_the_url_query_component_is_empty() { let http_tracker_server = start_default_http_tracker(Version::Axum).await; let response = Client::new(http_tracker_server.get_connection_info()).get("announce").await; - assert_internal_server_error_response(response).await; + assert_missing_query_params_for_announce_request_error_response(response).await; } - //#[tokio::test] - #[allow(dead_code)] + #[tokio::test] + async fn should_fail_when_url_query_parameters_are_invalid() { + let http_tracker_server = start_default_http_tracker(Version::Axum).await; + + let invalid_query_param = "a=b=c"; + + let response = Client::new(http_tracker_server.get_connection_info()) + .get(&format!("announce?{invalid_query_param}")) + .await; + + assert_cannot_parse_query_param_error_response(response, "invalid param a=b=c").await; + } + + #[tokio::test] async fn should_fail_when_a_mandatory_field_is_missing() { let http_tracker_server = start_default_http_tracker(Version::Axum).await; @@ -1405,7 +1417,7 @@ mod axum_http_tracker_server { .get(&format!("announce?{params}")) .await; - assert_invalid_info_hash_error_response(response).await; + assert_bad_announce_request_error_response(response, "missing info_hash param").await; // Without `peer_id` param @@ -1417,7 +1429,7 @@ mod axum_http_tracker_server { .get(&format!("announce?{params}")) .await; - assert_invalid_peer_id_error_response(response).await; + assert_bad_announce_request_error_response(response, "missing peer_id param").await; // Without `port` param @@ -1429,11 +1441,10 @@ mod axum_http_tracker_server { .get(&format!("announce?{params}")) .await; - assert_internal_server_error_response(response).await; + assert_bad_announce_request_error_response(response, "missing port param").await; } - //#[tokio::test] - #[allow(dead_code)] + #[tokio::test] async fn should_fail_when_the_info_hash_param_is_invalid() { let http_tracker_server = start_default_http_tracker(Version::Axum).await; @@ -1446,7 +1457,7 @@ mod axum_http_tracker_server { .get(&format!("announce?{params}")) .await; - assert_invalid_info_hash_error_response(response).await; + assert_cannot_parse_query_params_error_response(response).await; } } @@ -1511,8 +1522,7 @@ mod axum_http_tracker_server { } } - //#[tokio::test] - #[allow(dead_code)] + #[tokio::test] async fn should_fail_when_the_peer_id_param_is_invalid() { let http_tracker_server = start_default_http_tracker(Version::Axum).await; @@ -1534,12 +1544,11 @@ mod axum_http_tracker_server { .get(&format!("announce?{params}")) .await; - assert_invalid_peer_id_error_response(response).await; + assert_cannot_parse_query_params_error_response(response).await; } } - //#[tokio::test] - #[allow(dead_code)] + #[tokio::test] async fn should_fail_when_the_port_param_is_invalid() { let http_tracker_server = start_default_http_tracker(Version::Axum).await; @@ -1554,7 +1563,7 @@ mod axum_http_tracker_server { .get(&format!("announce?{params}")) .await; - assert_internal_server_error_response(response).await; + assert_cannot_parse_query_params_error_response(response).await; } }