|
| 1 | +# Issue #1561 — HTTP Tracker Client: Avoid Duplicating the `announce` Suffix |
| 2 | + |
| 3 | +## Overview |
| 4 | + |
| 5 | +The HTTP tracker client currently assumes the user passes a tracker base URL |
| 6 | +without the request path suffix. When the user provides a full tracker URL that |
| 7 | +already ends in `/announce`, the client appends another `announce` segment and |
| 8 | +sends the request to an invalid endpoint. |
| 9 | + |
| 10 | +This is a bug in the HTTP client URL construction logic. The client should |
| 11 | +accept both forms: |
| 12 | + |
| 13 | +- base URL, for example `https://tracker.torrust-demo.com/` |
| 14 | +- full announce URL, for example `https://tracker.torrust-demo.com/announce` |
| 15 | + |
| 16 | +- GitHub issue: <https://github.com/torrust/torrust-tracker/issues/1561> |
| 17 | +- Parent EPIC: <https://github.com/torrust/torrust-tracker/issues/669> |
| 18 | + |
| 19 | +## Motivation |
| 20 | + |
| 21 | +A user naturally expects the HTTP client to accept the same long-form tracker |
| 22 | +URL that appears in torrent metadata and public tracker lists. |
| 23 | + |
| 24 | +Today this command fails: |
| 25 | + |
| 26 | +```text |
| 27 | +cargo run -p torrust-tracker-client --bin http_tracker_client announce \ |
| 28 | + https://tracker.torrust-demo.com/announce \ |
| 29 | + 000620bbc6c52d5a96d98f6c0f1dfa523a40df82 |
| 30 | +``` |
| 31 | + |
| 32 | +Because the final request URL becomes: |
| 33 | + |
| 34 | +```text |
| 35 | +https://tracker.torrust-demo.com/announceannounce?...query... |
| 36 | +``` |
| 37 | + |
| 38 | +That produces a `404 Not Found` even though the provided tracker URL is valid. |
| 39 | + |
| 40 | +## Current Behaviour |
| 41 | + |
| 42 | +The console binary parses the user input URL and passes it unchanged into the |
| 43 | +package client in `console/tracker-client/src/console/clients/http/app.rs`. |
| 44 | + |
| 45 | +The actual bug is in |
| 46 | +`packages/tracker-client/src/http/client/mod.rs`, where request URLs are built |
| 47 | +by plain string concatenation: |
| 48 | + |
| 49 | +```rust |
| 50 | +fn build_announce_path_and_query(&self, query: &announce::Query) -> String { |
| 51 | + format!("{}?{query}", self.build_path("announce")) |
| 52 | +} |
| 53 | + |
| 54 | +fn build_url(&self, path: &str) -> String { |
| 55 | + let base_url = self.base_url(); |
| 56 | + format!("{base_url}{path}") |
| 57 | +} |
| 58 | +``` |
| 59 | + |
| 60 | +If `base_url` already ends in `announce`, the client still appends `announce` |
| 61 | +again. The same risk exists for `scrape` if a full scrape URL is passed. |
| 62 | + |
| 63 | +## Proposed Behaviour |
| 64 | + |
| 65 | +The HTTP client should normalize the request URL before sending requests. |
| 66 | + |
| 67 | +Expected accepted inputs for announce: |
| 68 | + |
| 69 | +- `https://tracker.torrust-demo.com` |
| 70 | +- `https://tracker.torrust-demo.com/` |
| 71 | +- `https://tracker.torrust-demo.com/announce` |
| 72 | + |
| 73 | +Expected final request path for announce: |
| 74 | + |
| 75 | +- exactly one `/announce` |
| 76 | + |
| 77 | +Expected accepted inputs for scrape: |
| 78 | + |
| 79 | +- `https://tracker.torrust-demo.com` |
| 80 | +- `https://tracker.torrust-demo.com/` |
| 81 | +- `https://tracker.torrust-demo.com/scrape` |
| 82 | + |
| 83 | +Expected final request path for scrape: |
| 84 | + |
| 85 | +- exactly one `/scrape` |
| 86 | + |
| 87 | +The client should not rely on callers pre-trimming or pre-normalizing the URL. |
| 88 | + |
| 89 | +## Goals |
| 90 | + |
| 91 | +- [ ] Accept both bare tracker base URLs and full announce URLs in the HTTP |
| 92 | + client |
| 93 | +- [ ] Avoid duplicating the `announce` path suffix in the final request URL |
| 94 | +- [ ] Avoid duplicating the `scrape` path suffix in the final request URL |
| 95 | +- [ ] Keep authenticated path handling working, including URLs that append the |
| 96 | + authentication key after the endpoint path |
| 97 | +- [ ] Preserve existing behaviour for valid base URLs |
| 98 | +- [ ] Add tests covering the supported input forms |
| 99 | +- [ ] `linter all` exits with code `0` |
| 100 | +- [ ] `cargo machete` reports no unused dependencies |
| 101 | +- [ ] Existing tests pass |
| 102 | + |
| 103 | +## Implementation Plan |
| 104 | + |
| 105 | +### Task 1: Replace string concatenation with URL-aware path building |
| 106 | + |
| 107 | +In `packages/tracker-client/src/http/client/mod.rs`, stop constructing request |
| 108 | +URLs through `format!("{base_url}{path}")`. |
| 109 | + |
| 110 | +Instead, add a helper that derives a normalized endpoint URL from the parsed |
| 111 | +`reqwest::Url`, for example by: |
| 112 | + |
| 113 | +- inspecting the current path segments |
| 114 | +- detecting whether the last segment is already `announce` or `scrape` |
| 115 | +- replacing or appending path segments as needed |
| 116 | +- preserving scheme, host, port, and query construction |
| 117 | + |
| 118 | +The key rule is: the final URL must contain the endpoint suffix exactly once. |
| 119 | + |
| 120 | +### Task 2: Normalize announce and scrape independently |
| 121 | + |
| 122 | +Ensure announce requests always resolve to exactly one `announce` segment and |
| 123 | +scrape requests always resolve to exactly one `scrape` segment. |
| 124 | + |
| 125 | +Do not implement a narrow fix that only handles `announce`. |
| 126 | + |
| 127 | +### Task 3: Preserve authenticated endpoint support |
| 128 | + |
| 129 | +`build_path()` currently appends the optional authentication key as: |
| 130 | + |
| 131 | +```rust |
| 132 | +announce/<key> |
| 133 | +``` |
| 134 | + |
| 135 | +or |
| 136 | + |
| 137 | +```rust |
| 138 | +scrape/<key> |
| 139 | +``` |
| 140 | + |
| 141 | +The normalization logic must preserve this behaviour without producing broken |
| 142 | +paths like: |
| 143 | + |
| 144 | +- `/announce/announce/<key>` |
| 145 | +- `/announce/<key>/<key>` |
| 146 | + |
| 147 | +### Task 4: Add focused unit tests for URL building |
| 148 | + |
| 149 | +Add tests in `packages/tracker-client/src/http/client/mod.rs` covering at least: |
| 150 | + |
| 151 | +- base URL without trailing slash + announce |
| 152 | +- base URL with trailing slash + announce |
| 153 | +- full `/announce` URL + announce |
| 154 | +- base URL without trailing slash + scrape |
| 155 | +- full `/scrape` URL + scrape |
| 156 | +- authenticated announce path with a full `/announce` base URL |
| 157 | + |
| 158 | +The tests should assert the exact final URL string. |
| 159 | + |
| 160 | +### Task 5: Update HTTP client docs/examples |
| 161 | + |
| 162 | +Update the module docs in |
| 163 | +`console/tracker-client/src/console/clients/http/app.rs` or package docs so the |
| 164 | +accepted URL forms are explicit. |
| 165 | + |
| 166 | +## Acceptance Criteria |
| 167 | + |
| 168 | +- [ ] Passing `https://tracker.torrust-demo.com` to the announce command sends |
| 169 | + the request to `/announce` |
| 170 | +- [ ] Passing `https://tracker.torrust-demo.com/announce` to the announce |
| 171 | + command also sends the request to `/announce` |
| 172 | +- [ ] Passing `https://tracker.torrust-demo.com` to the scrape command sends |
| 173 | + the request to `/scrape` |
| 174 | +- [ ] Passing `https://tracker.torrust-demo.com/scrape` to the scrape command |
| 175 | + also sends the request to `/scrape` |
| 176 | +- [ ] Authenticated requests still generate correct URLs |
| 177 | +- [ ] No duplicated endpoint suffix appears in final request URLs |
| 178 | +- [ ] `linter all` exits with code `0` |
| 179 | +- [ ] `cargo machete` reports no unused dependencies |
| 180 | +- [ ] Existing tests pass |
| 181 | + |
| 182 | +## Key Files |
| 183 | + |
| 184 | +| File | Role | |
| 185 | +| -------------------------------------------------------- | ----------------------------------------- | |
| 186 | +| `packages/tracker-client/src/http/client/mod.rs` | Main bug location and URL normalization | |
| 187 | +| `console/tracker-client/src/console/clients/http/app.rs` | Console entry point that accepts user URL | |
| 188 | + |
| 189 | +## References |
| 190 | + |
| 191 | +- Parent EPIC: <https://github.com/torrust/torrust-tracker/issues/669> |
| 192 | +- GitHub issue: <https://github.com/torrust/torrust-tracker/issues/1561> |
| 193 | +- HTTP client package: `packages/tracker-client/src/http/client/` |
| 194 | +- HTTP client console app: `console/tracker-client/src/console/clients/http/app.rs` |
0 commit comments