From a55c356a217e474caf4031d3e5557d82b2fcc7ce Mon Sep 17 00:00:00 2001 From: Cameron Garnham Date: Wed, 3 Jan 2024 21:47:48 +1100 Subject: [PATCH] dev: improve announce ip logic test --- src/servers/http/v1/handlers/announce.rs | 5 ++ tests/servers/http/asserts.rs | 4 +- tests/servers/http/v1/contract.rs | 71 +++++++++++++++++++----- 3 files changed, 66 insertions(+), 14 deletions(-) diff --git a/src/servers/http/v1/handlers/announce.rs b/src/servers/http/v1/handlers/announce.rs index e9198f20c..7b74702a6 100644 --- a/src/servers/http/v1/handlers/announce.rs +++ b/src/servers/http/v1/handlers/announce.rs @@ -78,6 +78,11 @@ async fn handle( See https://github.com/torrust/torrust-tracker/discussions/240. */ +/* code-review (da2ce7): The IP that is supplied with the announce request is completely dropped + in favour of the IP supplied by the `client_ip_sources`. + Is this intended behavior, is the IP in the client announce request completely untrusted? +*/ + async fn handle_announce( tracker: &Arc, announce_request: &Announce, diff --git a/tests/servers/http/asserts.rs b/tests/servers/http/asserts.rs index 3a2e67bf0..721861f33 100644 --- a/tests/servers/http/asserts.rs +++ b/tests/servers/http/asserts.rs @@ -4,6 +4,7 @@ use reqwest::Response; use super::responses::announce::{Announce, Compact, DeserializedCompact}; use super::responses::scrape; +use crate::servers::http::responses::announce::DictionaryPeer; use crate::servers::http::responses::error::Error; pub fn assert_bencoded_error(response_text: &String, expected_failure_reason: &str, location: &'static Location<'static>) { @@ -22,10 +23,11 @@ pub fn assert_bencoded_error(response_text: &String, expected_failure_reason: &s ); } +#[allow(dead_code)] pub async fn assert_empty_announce_response(response: Response) { assert_eq!(response.status(), 200); let announce_response: Announce = serde_bencode::from_str(&response.text().await.unwrap()).unwrap(); - assert!(announce_response.peers.is_empty()); + assert_eq!(announce_response.peers, Vec::::new()); } pub async fn assert_announce_response(response: Response, expected_announce_response: &Announce) { diff --git a/tests/servers/http/v1/contract.rs b/tests/servers/http/v1/contract.rs index a7962db0f..a149ba57d 100644 --- a/tests/servers/http/v1/contract.rs +++ b/tests/servers/http/v1/contract.rs @@ -97,8 +97,8 @@ mod for_all_config_modes { use crate::common::fixtures::invalid_info_hashes; use crate::servers::http::asserts::{ 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_is_announce_response, assert_missing_query_params_for_announce_request_error_response, + assert_cannot_parse_query_params_error_response, assert_compact_announce_response, assert_is_announce_response, + assert_missing_query_params_for_announce_request_error_response, }; use crate::servers::http::client::Client; use crate::servers::http::requests::announce::{Compact, QueryBuilder}; @@ -497,26 +497,71 @@ mod for_all_config_modes { env.stop().await; } + /// code-review (da2ce7): is this really intended behavior? #[tokio::test] async fn should_consider_two_peers_to_be_the_same_when_they_have_the_same_peer_id_even_if_the_ip_is_different() { let env = Started::new(&configuration::ephemeral_mode_public().into()).await; - let info_hash = InfoHash::from_str("9c38422213e30bff212b30c360d26f9a02136422").unwrap(); - let peer = PeerBuilder::default().build(); + let announce_policy = env.tracker.get_announce_policy(); - // Add a peer - env.add_torrent_peer(&info_hash, &peer).await; + let info_hash = InfoHash::from([0; 20]); + let id_a = peer::Id(*b"-qB00000000000000000"); + let id_b = peer::Id(*b"-qB00000000000000001"); - let announce_query = QueryBuilder::default() - .with_info_hash(&info_hash) - .with_peer_id(&peer.peer_id) - .query(); + let addr_a = SocketAddr::new(IpAddr::V4(Ipv4Addr::new(126, 0, 0, 1)), 8080); + let addr_b = SocketAddr::new(IpAddr::V4(Ipv4Addr::new(126, 0, 0, 2)), 8080); + let addr_c = SocketAddr::new(IpAddr::V4(Ipv4Addr::new(126, 0, 0, 3)), 8080); + + let peer_a = PeerBuilder::default().with_peer_id(&id_a).with_peer_addr(&addr_a).build(); + let peer_b = PeerBuilder::default().with_peer_id(&id_b).with_peer_addr(&addr_b).build(); + + // Add both Peers into DB. + env.add_torrent_peer(&info_hash, &peer_a).await; + env.add_torrent_peer(&info_hash, &peer_b).await; + + // The first query will overwrite the IP of the peer, no matter what ip we use. + { + let announce = QueryBuilder::default() + .with_info_hash(&info_hash) + .with_peer_id(&id_a) + .with_peer_addr(&addr_c.ip()) // different ip, but this is erased. + .query(); + + let response = Client::new(*env.bind_address()).announce(&announce).await; + + let response_assert = Announce { + complete: 2, + incomplete: 0, + interval: announce_policy.interval, + min_interval: announce_policy.interval_min, + peers: vec![peer_b.into()], // peer_b still has it's original ip. + }; + + println!("Query 1"); + assert_announce_response(response, &response_assert).await; + } + + // The Seconds Query will result with no listed peers, as both peer id's are now + // associated with the same client ip. i.e (0.0.0.0). + { + let announce = QueryBuilder::default() + .with_info_hash(&info_hash) + .with_peer_id(&id_b) // now we use peer b + .query(); - assert_ne!(peer.peer_addr.ip(), announce_query.peer_addr); + let response = Client::new(*env.bind_address()).announce(&announce).await; - let response = Client::new(*env.bind_address()).announce(&announce_query).await; + let response_assert = Announce { + complete: 2, + incomplete: 0, + interval: announce_policy.interval, + min_interval: announce_policy.interval_min, + peers: vec![], // peer_a now has host ip. + }; - assert_empty_announce_response(response).await; + println!("Query 2"); + assert_announce_response(response, &response_assert).await; + } env.stop().await; }