Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 63 additions & 3 deletions console/tracker-client/src/console/clients/http/app.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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`
//!
Comment thread
josecelano marked this conversation as resolved.
//! 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
Expand Down Expand Up @@ -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`",
Expand Down Expand Up @@ -300,13 +311,31 @@ fn parse_non_zero_port(port_str: &str) -> anyhow::Result<u16> {
Ok(port)
}

fn parse_and_validate_tracker_url(tracker_url: &str) -> anyhow::Result<Url> {
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")?;

Expand Down Expand Up @@ -355,9 +384,10 @@ fn serialize_json<T: serde::Serialize>(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 {
Expand All @@ -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());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand All @@ -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
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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
Expand All @@ -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 |
Expand Down
Loading
Loading