diff --git a/console/tracker-client/src/console/clients/http/app.rs b/console/tracker-client/src/console/clients/http/app.rs index f146c5001..4862832bb 100644 --- a/console/tracker-client/src/console/clients/http/app.rs +++ b/console/tracker-client/src/console/clients/http/app.rs @@ -9,6 +9,17 @@ //! cargo run --bin http_tracker_client announce http://127.0.0.1:7070 9c38422213e30bff212b30c360d26f9a02136422 //! ``` //! +//! Accepted tracker URL forms for `announce` and `scrape`: +//! +//! - `https://tracker.example.com` +//! - `https://tracker.example.com/` +//! - `https://tracker.example.com/announce` +//! - `https://tracker.example.com/scrape` +//! - `https://tracker.example.com/custom-tracker-endpoint` +//! +//! The tracker URL input must not include query (`?...`) or fragment (`#...`). +//! Use dedicated CLI arguments instead of URL query params. +//! //! `Announce` request (pretty JSON output): //! //! ```text @@ -217,7 +228,7 @@ pub async fn run() -> anyhow::Result<()> { } async fn announce_command(options: AnnounceOptions, timeout: Duration) -> anyhow::Result<()> { - let base_url = Url::parse(&options.tracker_url).context("failed to parse HTTP tracker base URL")?; + let base_url = parse_and_validate_tracker_url(&options.tracker_url)?; let info_hash = InfoHash::from_str(&options.info_hash).map_err(|_| { anyhow::anyhow!( "invalid infohash `{}`. Example infohash: `9c38422213e30bff212b30c360d26f9a02136422`", @@ -300,13 +311,31 @@ fn parse_non_zero_port(port_str: &str) -> anyhow::Result { Ok(port) } +fn parse_and_validate_tracker_url(tracker_url: &str) -> anyhow::Result { + let url = Url::parse(tracker_url).context("failed to parse HTTP tracker base URL")?; + + validate_tracker_url_parts(&url)?; + + Ok(url) +} + +fn validate_tracker_url_parts(url: &Url) -> anyhow::Result<()> { + if url.query().is_some() || url.fragment().is_some() { + bail!( + "invalid tracker URL input: include only scheme, host, optional port, and optional path. Do not include query or fragment. Pass tracker request params using dedicated CLI arguments" + ); + } + + Ok(()) +} + async fn scrape_command( tracker_url: &str, info_hashes: &[String], output_format: OutputFormat, timeout: Duration, ) -> anyhow::Result<()> { - let base_url = Url::parse(tracker_url).context("failed to parse HTTP tracker base URL")?; + let base_url = parse_and_validate_tracker_url(tracker_url)?; let query = requests::scrape::Query::try_from(info_hashes).context("failed to parse infohashes")?; @@ -355,9 +384,10 @@ fn serialize_json(value: &T, output_format: OutputFormat) - #[cfg(test)] mod tests { + use reqwest::Url; use serde::Serialize; - use super::{serialize_json, OutputFormat}; + use super::{parse_and_validate_tracker_url, serialize_json, validate_tracker_url_parts, OutputFormat}; #[derive(Serialize)] struct Sample { @@ -384,4 +414,34 @@ mod tests { assert!(json.contains(" \"seeders\": 1")); assert!(json.contains(" \"leechers\": 2")); } + + #[test] + fn it_accepts_tracker_url_with_path_and_without_query_or_fragment() { + let parsed = parse_and_validate_tracker_url("https://tracker.example.com/announce"); + + assert!(parsed.is_ok()); + } + + #[test] + fn it_rejects_tracker_url_with_query() { + let parsed = parse_and_validate_tracker_url("https://tracker.example.com/announce?info_hash=abc"); + + assert!(parsed.is_err()); + } + + #[test] + fn it_rejects_tracker_url_with_fragment() { + let parsed = parse_and_validate_tracker_url("https://tracker.example.com/announce#details"); + + assert!(parsed.is_err()); + } + + #[test] + fn it_accepts_direct_validation_for_plain_base_url() { + let url = Url::parse("https://tracker.example.com/").expect("url should parse"); + + let result = validate_tracker_url_parts(&url); + + assert!(result.is_ok()); + } } diff --git a/docs/issues/open/1561-http-tracker-client-avoid-duplicating-announce-suffix.md b/docs/issues/open/1561-http-tracker-client-avoid-duplicating-announce-suffix.md index 6a3f97225..2b5d81d17 100644 --- a/docs/issues/open/1561-http-tracker-client-avoid-duplicating-announce-suffix.md +++ b/docs/issues/open/1561-http-tracker-client-avoid-duplicating-announce-suffix.md @@ -91,6 +91,18 @@ Path resolution rule for `announce`: The client should not rely on callers pre-trimming or pre-normalizing the URL. +Path resolution rule for `scrape` (same strategy as `announce`): + +- Input path empty or `/` -> resolve to `/scrape` +- Input path non-empty (for example `/scrape`, `/foo`, `/foo/bar`) -> keep it + unchanged + +CLI URL input validation rule: + +- The tracker URL input must not contain query (`?...`) or fragment (`#...`) +- If query or fragment is present, fail with a friendly error message +- Tracker protocol parameters must be provided through dedicated CLI arguments + Scope note: this issue is about tracker protocol endpoints (`announce` and `scrape`). The `health_check` endpoint is out of scope. @@ -106,6 +118,9 @@ Scope note: this issue is about tracker protocol endpoints (`announce` and - [ ] Preserve existing behaviour for valid base URLs - [ ] Add tests covering the supported input forms - [ ] Keep `health_check` behaviour unchanged in this issue +- [ ] Apply the same path-resolution strategy to `scrape` +- [ ] Reject tracker URL inputs containing query or fragment with a friendly + CLI error - [ ] `linter all` exits with code `0` - [ ] `cargo machete` reports no unused dependencies - [ ] Existing tests pass @@ -136,6 +151,15 @@ For announce requests: Do not append `announce` when any path segment already exists. +### Task 2b: Apply base-URL detection for scrape + +For scrape requests: + +- If the input URL path is empty or `/`, append `scrape` +- Otherwise, keep the original path unchanged + +Do not append `scrape` when any path segment already exists. + ### Task 3: Preserve authenticated endpoint support `build_path()` currently appends the optional authentication key as: @@ -180,6 +204,20 @@ Do not change `health_check` behavior as part of this bug fix. If endpoint normalization is later generalized to all methods, that should be handled in a separate issue with dedicated tests. +### Task 7: Reject query/fragment in CLI tracker URL input + +In the HTTP tracker client console command input parsing: + +- Reject tracker URLs that include query or fragment +- Return a friendly error explaining accepted URL parts +- Instruct users to pass tracker request params through dedicated CLI arguments + +### Task 8: Validation sequence + +- Run targeted tests first for the affected packages +- Run full checks before committing, including `linter all` and + `cargo machete` + ## Acceptance Criteria - [ ] Passing `https://tracker.torrust-demo.com` to the announce command sends @@ -188,12 +226,49 @@ separate issue with dedicated tests. command also sends the request to `/announce` - [ ] Passing a URL with a non-empty path (for example `/foo`) keeps `/foo` unchanged and does not append `announce` +- [ ] Passing `https://tracker.torrust-demo.com` to the scrape command sends + the request to `/scrape` +- [ ] Passing `https://tracker.torrust-demo.com/scrape` to the scrape command + also sends the request to `/scrape` +- [ ] Passing a URL with a non-empty path (for example `/foo`) keeps `/foo` + unchanged and does not append `scrape` +- [ ] Passing a tracker URL containing query or fragment fails fast with a + friendly CLI error and guidance to use dedicated CLI arguments - [ ] Authenticated requests still generate correct URLs - [ ] No duplicated endpoint suffix appears in final request URLs - [ ] `linter all` exits with code `0` - [ ] `cargo machete` reports no unused dependencies - [ ] Existing tests pass +## Clarifications (2026-05-11) + +- Apply the same endpoint-resolution behavior to `scrape` as `announce`. +- Reject tracker URL input containing query or fragment. +- Show a friendly error message indicating URL input must only include + scheme/host/optional port/optional path. +- Require tracker request parameters to be passed through CLI arguments, + not URL query. +- Preferred validation flow: run targeted package tests first; always run full + repository checks before committing. + +Manual smoke-check examples for query/fragment rejection: + +```text +cargo run -p torrust-tracker-client --bin http_tracker_client announce \ + 'https://tracker.torrust-demo.com/announce?foo=1' \ + 000620bbc6c52d5a96d98f6c0f1dfa523a40df82 + +Error: invalid tracker URL input: include only scheme, host, optional port, and optional path. Do not include query or fragment. Pass tracker request params using dedicated CLI arguments +``` + +```text +cargo run -p torrust-tracker-client --bin http_tracker_client scrape \ + 'https://tracker.torrust-demo.com/scrape#frag' \ + 000620bbc6c52d5a96d98f6c0f1dfa523a40df82 + +Error: invalid tracker URL input: include only scheme, host, optional port, and optional path. Do not include query or fragment. Pass tracker request params using dedicated CLI arguments +``` + ## Key Files | File | Role | diff --git a/packages/tracker-client/src/http/client/mod.rs b/packages/tracker-client/src/http/client/mod.rs index 50e979c79..edd552221 100644 --- a/packages/tracker-client/src/http/client/mod.rs +++ b/packages/tracker-client/src/http/client/mod.rs @@ -94,7 +94,7 @@ impl Client { /// /// This method fails if the returned response was not successful pub async fn announce(&self, query: &announce::Query) -> Result { - let response = self.get(&self.build_announce_path_and_query(query)).await?; + let response = self.get_url(self.build_announce_url(query)).await?; if response.status().is_success() { Ok(response) @@ -110,7 +110,7 @@ impl Client { /// /// This method fails if the returned response was not successful pub async fn scrape(&self, query: &scrape::Query) -> Result { - let response = self.get(&self.build_scrape_path_and_query(query)).await?; + let response = self.get_url(self.build_scrape_url(query)).await?; if response.status().is_success() { Ok(response) @@ -126,9 +126,7 @@ impl Client { /// /// This method fails if the returned response was not successful pub async fn announce_with_header(&self, query: &announce::Query, key: &str, value: &str) -> Result { - let response = self - .get_with_header(&self.build_announce_path_and_query(query), key, value) - .await?; + let response = self.get_url_with_header(self.build_announce_url(query), key, value).await?; if response.status().is_success() { Ok(response) @@ -179,12 +177,65 @@ impl Client { .map_err(|e| Error::ResponseError { err: e.into() }) } - fn build_announce_path_and_query(&self, query: &announce::Query) -> String { - format!("{}?{query}", self.build_path("announce")) + async fn get_url(&self, url: Url) -> Result { + self.http_client + .get(url) + .send() + .await + .map_err(|e| Error::ResponseError { err: e.into() }) } - fn build_scrape_path_and_query(&self, query: &scrape::Query) -> String { - format!("{}?{query}", self.build_path("scrape")) + async fn get_url_with_header(&self, url: Url, key: &str, value: &str) -> Result { + self.http_client + .get(url) + .header(key, value) + .send() + .await + .map_err(|e| Error::ResponseError { err: e.into() }) + } + + fn build_announce_url(&self, query: &announce::Query) -> Url { + let mut url = self.build_endpoint_url("announce"); + url.set_query(Some(&query.to_string())); + url + } + + fn build_scrape_url(&self, query: &scrape::Query) -> Url { + let mut url = self.build_endpoint_url("scrape"); + url.set_query(Some(&query.to_string())); + url + } + + fn build_endpoint_url(&self, default_endpoint: &str) -> Url { + let mut url = self.base_url.clone(); + + let current_path = url.path(); + let normalized_path = if current_path.is_empty() || current_path == "/" { + format!("/{default_endpoint}") + } else { + current_path.to_owned() + }; + + let final_path = match &self.key { + Some(key) => { + let path_without_trailing_slash = normalized_path.trim_end_matches('/'); + let key_segment = key.value(); + let already_has_key = path_without_trailing_slash + .rsplit('/') + .next() + .is_some_and(|segment| segment == key_segment); + + if already_has_key { + path_without_trailing_slash.to_string() + } else { + format!("{path_without_trailing_slash}/{key}") + } + } + None => normalized_path, + }; + + url.set_path(&final_path); + url } fn build_path(&self, path: &str) -> String { @@ -219,3 +270,102 @@ impl Key { &self.0 } } + +#[cfg(test)] +mod tests { + use std::time::Duration; + + use reqwest::Url; + + use super::{Client, Key}; + + fn test_timeout() -> Duration { + Duration::from_secs(1) + } + + #[test] + fn it_uses_announce_for_base_url_without_trailing_slash() { + let client = Client::new(Url::parse("https://tracker.example.com").unwrap(), test_timeout()).unwrap(); + + let url = client.build_endpoint_url("announce"); + + assert_eq!(url.to_string(), "https://tracker.example.com/announce"); + } + + #[test] + fn it_uses_announce_for_base_url_with_trailing_slash() { + let client = Client::new(Url::parse("https://tracker.example.com/").unwrap(), test_timeout()).unwrap(); + + let url = client.build_endpoint_url("announce"); + + assert_eq!(url.to_string(), "https://tracker.example.com/announce"); + } + + #[test] + fn it_keeps_existing_announce_path_unchanged() { + let client = Client::new(Url::parse("https://tracker.example.com/announce").unwrap(), test_timeout()).unwrap(); + + let url = client.build_endpoint_url("announce"); + + assert_eq!(url.to_string(), "https://tracker.example.com/announce"); + } + + #[test] + fn it_keeps_custom_path_unchanged_for_announce() { + let client = Client::new( + Url::parse("https://tracker.example.com/custom-tracker-endpoint").unwrap(), + test_timeout(), + ) + .unwrap(); + + let url = client.build_endpoint_url("announce"); + + assert_eq!(url.to_string(), "https://tracker.example.com/custom-tracker-endpoint"); + } + + #[test] + fn it_appends_auth_key_to_existing_announce_path() { + let client = Client::authenticated( + Url::parse("https://tracker.example.com/announce").unwrap(), + test_timeout(), + Key::new("secret-key"), + ) + .unwrap(); + + let url = client.build_endpoint_url("announce"); + + assert_eq!(url.to_string(), "https://tracker.example.com/announce/secret-key"); + } + + #[test] + fn it_does_not_append_auth_key_when_path_already_ends_with_same_key() { + let client = Client::authenticated( + Url::parse("https://tracker.example.com/announce/secret-key").unwrap(), + test_timeout(), + Key::new("secret-key"), + ) + .unwrap(); + + let url = client.build_endpoint_url("announce"); + + assert_eq!(url.to_string(), "https://tracker.example.com/announce/secret-key"); + } + + #[test] + fn it_uses_scrape_for_base_url_without_trailing_slash() { + let client = Client::new(Url::parse("https://tracker.example.com").unwrap(), test_timeout()).unwrap(); + + let url = client.build_endpoint_url("scrape"); + + assert_eq!(url.to_string(), "https://tracker.example.com/scrape"); + } + + #[test] + fn it_keeps_existing_scrape_path_unchanged() { + let client = Client::new(Url::parse("https://tracker.example.com/scrape").unwrap(), test_timeout()).unwrap(); + + let url = client.build_endpoint_url("scrape"); + + assert_eq!(url.to_string(), "https://tracker.example.com/scrape"); + } +}