fix(http): [#184] bencoded error responses for announce request

HTTP tracker error responser must be bencoded. Fixed in the new Axum
implementation.

Author: josecelano

src/http/axum_implementation/query.rs

@@ -45,7 +45,7 @@ impl FromStr for Param {
     fn from_str(raw_param: &str) -> Result<Self, Self::Err> {
         let pair = raw_param.split('=').collect::<Vec<&str>>();
 
-        if pair.len() > 2 {
+        if pair.len() != 2 {
             return Err(ParseQueryError::InvalidParam {
                 location: Location::caller(),
                 raw_param: raw_param.to_owned(),

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<IdConversionError> for ParseAnnounceQueryError {
 impl From<ConversionError> for ParseAnnounceQueryError {
     #[track_caller]
     fn from(_err: ConversionError) -> Self {
-        Self::InvalidPeerId {
+        Self::InvalidInfoHash {
             location: Location::caller(),
         }
     }
 }
 
+impl From<ParseQueryError> 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<ParseAnnounceQueryError> 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<Query> for Announce {
     type Error = ParseAnnounceQueryError;
 
@@ -107,27 +127,28 @@ impl<S> FromRequestParts<S> 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<Self, Self::Rejection> {
-        // 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::<Query>();
 
-        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()))

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
+    }
+}

src/http/axum_implementation/responses/mod.rs

@@ -1 +1,2 @@
+pub mod error;
 pub mod ok;

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::<Error>(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(),
+    );
+}