From b5879fb2d6197a6dbef0c3e5a64ab487618f9fda Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Wed, 15 Jul 2026 17:02:38 +0100 Subject: [PATCH 1/5] feat(agents): add Researcher custom agent for external evidence gathering Add a new custom agent that clones external tracker repositories, searches their source code and issue trackers, and returns structured findings. Used before writing issue specs or during implementation when claims about other trackers need verification. The agent uses /tmp/tracker-research/ or the workspace .tmp/ directory for temporary artifacts. --- .github/agents/researcher.agent.md | 82 ++++++++++++++++++++++++++++++ 1 file changed, 82 insertions(+) create mode 100644 .github/agents/researcher.agent.md diff --git a/.github/agents/researcher.agent.md b/.github/agents/researcher.agent.md new file mode 100644 index 000000000..397913ef4 --- /dev/null +++ b/.github/agents/researcher.agent.md @@ -0,0 +1,82 @@ +--- +name: Researcher +description: Evidence-gathering specialist for the torrust-tracker project. Clones external repositories, searches their source code and issue trackers, reads documentation, and returns structured findings. Use before writing issue specs, during implementation when a decision needs external evidence, or any time a claim about "what other trackers do" needs verification. Not for codebase-internal exploration — use the Explore subagent for that. +argument-hint: Describe the research question, what external projects or sources to investigate, and what specific evidence is needed. Include whether to clone repos, search GitHub issues, or both. +tools: [execute, read, search, todo] +user-invocable: true +disable-model-invocation: false +--- + +You are the repository's evidence-gathering specialist. Your job is to research external projects, +source code, issue trackers, and documentation to answer specific questions with concrete evidence. + +You gather facts. You do not make implementation decisions or write production code. + +## Repository Rules + +- Follow `AGENTS.md` for repository-wide conventions. +- When research findings affect an issue spec, report them in a format the **Planner** or + **Implementer** can directly incorporate. +- Prefer cloning external repos into a temporary directory outside the workspace (e.g. `/tmp/tracker-research/`) + to avoid polluting the working tree. When `/tmp` is not available or the caller prefers workspace-local + artifacts, use the workspace `.tmp/` directory instead — it is git-ignored and safe for temporary files. + +## Primary Responsibilities + +1. Clone external tracker implementations (opentracker, chihaya, etc.) and search their source + code for specific patterns, behaviors, or configuration options. +2. Search external GitHub repositories for relevant issues, PRs, and discussions using the + `github_text_search` and `github_repo` tools. +3. Read external documentation (BEPs, wiki pages, READMEs) to verify claims. +4. Compare implementations across multiple trackers and identify the de-facto standard behavior. +5. Return structured, evidence-backed findings with source references (file paths, line numbers, + issue URLs, commit hashes). + +## Research Domains + +Typical research questions include: + +- How do other BitTorrent trackers handle a specific BEP requirement? +- What is the de-facto standard for a given protocol behavior? +- Does a specific tracker feature exist in opentracker, chihaya, or other implementations? +- What configuration options do other trackers expose for a given feature? +- Are there known issues or discussions about a specific design decision in other trackers? + +## Required Workflow + +1. **Clarify the research question**: Identify exactly what evidence is needed and from which + external sources. +2. **Plan the investigation**: Decide which repos to clone, which search queries to run, and + which documentation to consult. +3. **Gather evidence**: + - For source code research: clone the repo (shallow clone with `--depth 1`), then use `grep`, + `find`, and `git log` to locate relevant code. + - For issue research: use `github_text_search` with the target org/repo and relevant keywords. + - For documentation: fetch and read relevant web pages or local docs. +4. **Cross-reference findings**: Compare evidence across multiple sources. Note agreements and + disagreements. +5. **Report findings** in a structured format (see Output Format below). + +## Output Format + +When finishing research, respond in this order: + +1. **Research question** (restated) +2. **Sources consulted** (repos cloned, queries run, docs read) +3. **Findings** — for each source: + - What was found (with file paths, line numbers, URLs) + - Direct quotes or code snippets where relevant +4. **Cross-project comparison** — table or summary showing how each project handles the behavior +5. **Conclusion** — what the evidence supports, with confidence level +6. **Open questions** — anything the evidence didn't resolve + +## Constraints + +- Do not modify any files in the workspace. This is a read-only research role. +- Do not make implementation recommendations. Report facts, not decisions. +- Do not clone repos inside the workspace. Use `/tmp/tracker-research/` or the workspace `.tmp/` directory (git-ignored). +- Do not guess or assume behavior. Every claim must be backed by evidence found during the session. +- Do not spend time on irrelevant tangents. Stay focused on the research question. +- Clean up cloned repos after reporting if the caller doesn't need them persisted. +- When source code is ambiguous, say so rather than over-interpreting. +- Prefer shallow clones (`--depth 1`) to minimize time and disk usage. From 334fba66c09fd2a43935de454b0d50561a7e7318 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Wed, 15 Jul 2026 18:13:11 +0100 Subject: [PATCH 2/5] docs(issue): add spec for renaming peer_addr to ip in HTTP announce (ref #1985) Add issue spec for renaming the non-standard peer_addr GET parameter to the BEP 3-specified ip parameter. Includes an embedded ADR deciding to accept only IP addresses (not DNS names) in the ip parameter. Also add 'hostnames' to project-words.txt for the ADR text. --- .../ISSUE.md | 227 ++++++++++++++++++ project-words.txt | 1 + 2 files changed, 228 insertions(+) create mode 100644 docs/issues/open/1985-rename-peer-addr-to-ip-in-http-announce-request/ISSUE.md diff --git a/docs/issues/open/1985-rename-peer-addr-to-ip-in-http-announce-request/ISSUE.md b/docs/issues/open/1985-rename-peer-addr-to-ip-in-http-announce-request/ISSUE.md new file mode 100644 index 000000000..44fb1aa16 --- /dev/null +++ b/docs/issues/open/1985-rename-peer-addr-to-ip-in-http-announce-request/ISSUE.md @@ -0,0 +1,227 @@ +--- +doc-type: issue +issue-type: bug +status: open +priority: p2 +github-issue: 1985 +spec-path: docs/issues/open/1985-rename-peer-addr-to-ip-in-http-announce-request/ISSUE.md +branch: "1985-rename-peer-addr-to-ip-in-http-announce-request" +related-pr: null +depends-on: null +blocks: + - docs/issues/open/1987-add-config-option-to-use-ip-from-announce-query-string/ISSUE.md +last-updated-utc: 2026-07-15 00:00 +semantic-links: + skill-links: + - create-issue + related-artifacts: + - packages/http-protocol/src/v1/requests/announce.rs + - packages/axum-http-server/src/lib.rs + - packages/axum-http-server/src/v1/extractors/announce_request.rs + - packages/http-core/src/services/announce.rs + - packages/tracker-core/src/torrent/mod.rs + - docs/adrs/ +--- + + + +# Issue #1985 - Rename `peer_addr` GET param to `ip` in HTTP announce request (BEP 3) + +## Goal + +Rename the HTTP announce GET parameter from the non-standard `peer_addr` to the BEP 3-specified `ip`, aligning the wire protocol with the specification. Rename the corresponding Rust field and constant to match, so the wire name and the code name are consistent. Additionally, make an explicit architectural decision about DNS name support in the `ip` parameter. + +## Background + +[BEP 3 — The BitTorrent Protocol Specification](https://www.bittorrent.org/beps/bep_0003.html) defines the `ip` parameter as: + +> An optional parameter giving the IP (or dns name) which this peer is at. Generally used for the origin if it's on the same machine as the tracker. + +The Torrust Tracker HTTP announce handler currently uses `peer_addr` as the GET parameter name, which is a non-standard name not defined in any BEP. The correct BEP 3 wire name is `ip`. + +### Current state + +- The wire GET parameter name is `peer_addr` (constant `PEER_ADDR = "peer_addr"` in `packages/http-protocol/src/v1/requests/announce.rs`). +- The Rust struct field is also named `peer_addr`. +- The existing module documentation in `packages/axum-http-server/src/lib.rs` contains a factually incorrect `NOTICE` (lines 65–70) claiming `peer_addr` comes from the UDP tracker protocol (BEP 15). This is wrong: `ip` is defined in BEP 3 (HTTP) and has been there from the start. The BEP 15 angle is irrelevant to this parameter. +- The field type is `Option`. DNS names provided by a client are silently dropped by `IpAddr::from_str` in `extract_peer_addr`, with no error returned to the client. +- The parameter is always ignored at the announce service level: `peer_from_request` in `packages/http-core/src/services/announce.rs` builds the peer using the connection-derived IP, never from `announce_request.peer_addr`. Whether to honour the `ip` param in future is addressed separately (see "The 'honour the `ip` param' question" below and Issue 3). + +### The DNS name question + +BEP 3 specifies the `ip` parameter as accepting "IP (or dns name)". In practice: + +- No major tracker implementation supports DNS names in this field (opentracker, chihaya, and others accept IPs only). +- The tracker's peer list stores `IpAddr` values, not hostnames. Supporting DNS would require either resolving names at announce time (latency, DoS vector) or storing hostnames (incompatible with the peer list model). +- The current behaviour (silently drop non-IP values) is confusing and undocumented. + +An explicit decision is needed. The decision is captured in the ADR drafted as part of this issue: [`docs/adrs/YYYYMMDD_accept_only_ip_addresses_in_http_announce_ip_param.md`](../adrs/). + +### The "honour the `ip` param" question + +This issue deliberately does **not** address whether the tracker should honour the `ip` GET parameter value instead of always using the connection IP. That is a separate feature request tracked as a sub-issue of the configuration overhaul epic (#1978). See related issues below. + +## Scope + +### In Scope + +- Rename the wire GET parameter from `peer_addr` to `ip` throughout the HTTP protocol layer: + - Rename the constant `PEER_ADDR` → `IP` and its value `"peer_addr"` → `"ip"` in `packages/http-protocol/src/v1/requests/announce.rs`. Also fix the hardcoded `"peer_addr"` literal in the `Display` impl (line 307) to use the renamed `IP` constant. + - Rename the struct field `peer_addr` → `ip` on `Announce` in the same file. Also fix the doc comment on the `Announce` struct (line 83) which incorrectly claims `peer_addr` is "as per BEP 3" — BEP 3 uses `ip`. + - Rename the builder method `with_peer_addr` → `with_ip` and update `AnnounceBuilder::with_default_values` accordingly. + - Update `extract_peer_addr` → `extract_ip` and update all call sites. +- Fix the factually incorrect `NOTICE` in `packages/axum-http-server/src/lib.rs` (lines 65–70): replace the claim that `peer_addr` comes from BEP 15 with an accurate description referencing BEP 3 `ip`. +- Update the parameter table in `packages/axum-http-server/src/lib.rs` from `peer_addr` to `ip`. +- Update sample URLs in documentation and doc-comments that contain `peer_addr=` to use `ip=`. +- Update any tests, fixtures, and the tracker client that construct or parse announce URLs with `peer_addr=`. +- Draft and commit the ADR for the decision to accept only IP addresses (not DNS names) in the `ip` parameter. + +### Out of Scope + +- Honouring the `ip` parameter value instead of the connection IP (separate issue, sub-issue of #1978). +- Returning a parse error to the client when a DNS name is provided instead of an IP (could be a follow-up; for now silently ignoring remains acceptable once the ADR is in place). +- Any changes to the UDP tracker protocol. +- Any changes to the scrape endpoint. + +## ADR: Accept only IP addresses in the HTTP announce `ip` parameter + +The following decision record will be committed to `docs/adrs/` as part of this issue. + +--- + +### Title + +Accept only IP addresses (not DNS names) in the HTTP announce `ip` GET parameter + +### Description + +BEP 3 defines the `ip` announce parameter as accepting "IP (or dns name)". The current implementation silently drops any value that cannot be parsed as an `IpAddr`. A decision is needed on whether to support DNS names, resolve them, or explicitly restrict the parameter to IP addresses only. + +### Context + +The `ip` GET parameter is optional and currently always ignored by the tracker at the service level. Its value is parsed and stored on the `Announce` struct but never forwarded to `peer_from_request`. Even so, a clear policy is needed for what values the tracker accepts in this field. + +Three approaches were considered: + +| Approach | What | Pros | Cons | +| ---------------------------------- | -------------------------------------------------------------------------------------------------------------------------- | -------------------------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------- | +| **A — IP only (explicit)** | Accept only valid `IpAddr` values; return a parse error or silently ignore non-IP values; document the restriction clearly | Simple, predictable, no latency, no DoS risk, consistent with all major trackers | Deviates from the literal BEP 3 spec text | +| **B — Resolve DNS names** | Accept DNS names and resolve them to IPs at announce time | Closer to BEP 3 literal wording | Latency per announce, DoS amplification risk (attacker-controlled DNS lookups), complexity, and no known client actually sends hostnames | +| **C — Accept and store hostnames** | Parse and store hostnames as strings alongside IPs | Closest to BEP 3 literal wording | Incompatible with the `IpAddr`-based peer list model; no client or tracker implements this; no BEP defines how hostnames are returned in responses | + +### Evidence from major trackers + +- **opentracker**: accepts only IP addresses in `ip`. Has a separate compile-time feature flag (`WANT_IP_FROM_QUERY_STRING`) to optionally use the `ip` value for the peer's address; the type accepted is always an IP. +- **chihaya**: accepts only IP addresses in `ip`. +- **No known tracker** supports DNS name resolution in the announce `ip` parameter. + +### Agreement + +**Approach A** — accept only IP addresses in the HTTP announce `ip` parameter. Non-IP values (including DNS names) are silently ignored; the tracker falls back to the connection IP. The restriction is documented clearly in the module doc-comment. + +This deviates from the literal BEP 3 wording ("or dns name") but matches the de-facto standard across all known tracker implementations. Clients MUST NOT send hostnames in this field when communicating with Torrust Tracker. A future issue may choose to return an explicit parse error for non-IP values instead of silently ignoring them. + +### Consequences + +- **Positive**: No latency impact on announce handling. +- **Positive**: No DNS-based DoS attack surface. +- **Positive**: Consistent with opentracker, chihaya, and all other known tracker implementations. +- **Positive**: The `IpAddr`-based peer list model is preserved without changes. +- **Negative**: Deviates from the literal BEP 3 spec text ("or dns name"). Mitigated by clear documentation and the fact that no known client sends a hostname. + +--- + +## Implementation Plan + +Status values: `TODO`, `IN_PROGRESS`, `BLOCKED`, `DONE`. + +| ID | Status | Task | Notes / Expected Output | +| --- | ------ | -------------------------------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| T1 | TODO | Rename `PEER_ADDR` constant and `"peer_addr"` wire string to `IP` / `"ip"` | `packages/http-protocol/src/v1/requests/announce.rs`. Also fix the hardcoded `"peer_addr"` literal in the `Display` impl (line 307) to use the renamed `IP` constant instead of a string literal. | +| T2 | TODO | Rename struct field `peer_addr` → `ip` on `Announce` | Same file; update all construction and match sites. Also fix the doc comment on the `Announce` struct (line 83) which incorrectly claims `peer_addr` is "as per BEP 3" — BEP 3 uses `ip`. | +| T3 | TODO | Rename `with_peer_addr` → `with_ip` on `AnnounceBuilder`; update `with_default_values` | Same file | +| T4 | TODO | Rename `extract_peer_addr` → `extract_ip`; update call sites | Same file | +| T5 | TODO | Update the `NOTICE` and parameter table in `packages/axum-http-server/src/lib.rs` | Replace incorrect BEP 15 reference with correct BEP 3 `ip` description | +| T6 | TODO | Update sample URLs in doc-comments from `peer_addr=` to `ip=` | `packages/axum-http-server/src/lib.rs`, `extractors/announce_request.rs`, `packages/tracker-core/src/torrent/mod.rs` | +| T7 | TODO | Update test fixtures and inline URL strings that use `peer_addr=` | `packages/axum-http-server/tests/server/v1/contract/for_all_config_modes/receiving_an_announce_request.rs`, `packages/axum-http-server/tests/server/v1/contract/configured_as_private.rs`, `packages/axum-http-server/src/v1/extractors/announce_request.rs` (inline test query string) | +| T8 | TODO | Commit the ADR to `docs/adrs/` | File: `docs/adrs/YYYYMMDD_accept_only_ip_addresses_in_http_announce_ip_param.md` | +| T9 | TODO | Run `cargo test --workspace` — no regressions | All tests pass | +| T10 | TODO | Run `linter all` | Must exit `0` | +| T11 | TODO | Rename test function `should_not_fail_when_the_peer_address_param_is_invalid` | Rename to `should_not_fail_when_the_ip_param_is_invalid` in `packages/axum-http-server/tests/server/v1/contract/for_all_config_modes/receiving_an_announce_request.rs` | + +## Progress Tracking + +### Workflow Checkpoints + +- [ ] Spec drafted in `docs/issues/drafts/` +- [ ] Spec reviewed and approved by user/maintainer +- [ ] GitHub issue created and issue number added to this spec +- [ ] (Optional, recommended for complex issues) Spec-only PR merged into `develop` before implementation +- [ ] Implementation completed +- [ ] Automatic verification completed (`linter all`, relevant tests, and any pre-push checks) +- [ ] Manual verification scenarios executed and recorded (status + evidence) +- [ ] Acceptance criteria reviewed after implementation and updated with evidence +- [ ] Reviewer validated acceptance criteria and updated checkboxes +- [ ] Committer verified spec progress is up to date before commit +- [ ] Issue closed and spec moved from `docs/issues/open/` to `docs/issues/closed/` + +### Progress Log + +- 2026-07-15 00:00 UTC - Copilot/User - Spec drafted; ADR embedded as a section pending extraction to `docs/adrs/` during implementation. + +## Acceptance Criteria + +- [ ] AC1: An HTTP announce request using `ip=
` is correctly parsed — the `ip` field on the `Announce` struct is populated. +- [ ] AC2: An HTTP announce request using the old `peer_addr=
` parameter no longer populates the field (the old name is not recognised). +- [ ] AC3: The Rust struct field, builder method, extractor function, and constant all use the name `ip` (no remaining `peer_addr` references for the wire parameter). The `Display` impl uses the `IP` constant rather than a hardcoded string literal. +- [ ] AC4: The `NOTICE` in `packages/axum-http-server/src/lib.rs` accurately describes the `ip` parameter with a correct BEP 3 reference (no BEP 15 mention for this parameter). +- [ ] AC5: All sample URLs in documentation use `ip=` instead of `peer_addr=`. +- [ ] AC6: The ADR `docs/adrs/YYYYMMDD_accept_only_ip_addresses_in_http_announce_ip_param.md` is committed. +- [ ] AC7: `linter all` exits with code `0`. +- [ ] AC8: Relevant tests pass with no regressions. +- [ ] Manual verification scenarios are executed and documented (status + evidence). +- [ ] Acceptance criteria are re-reviewed after implementation and reflect actual behaviour. +- [ ] Documentation is updated when behaviour/workflow changes. + +## Verification Plan + +### Automatic Checks + +- `linter all` +- `cargo test --workspace` +- Pre-push checks (when applicable) + +### Manual Verification Scenarios + +Status values: `TODO`, `IN_PROGRESS`, `DONE`, `FAILED`, `BLOCKED`. + +| ID | Scenario | Command/Steps | Expected Result | Status | Evidence | +| --- | -------------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------- | --------------------------------------------------------------- | ------ | -------- | +| M1 | Announce with `ip=
` — field is parsed | `curl -s "http://localhost:7070/announce?info_hash=...&peer_id=...&port=6881&ip=2.137.87.41"` and check tracker logs | Tracker logs show `ip` was parsed | TODO | | +| M2 | Announce with old `peer_addr=
` — field is ignored | Replace `ip=` with `peer_addr=` in M1 URL | Tracker ignores the parameter (no parse error, field is `None`) | TODO | | +| M3 | Announce with `ip=hostname.example.com` — non-IP is silently ignored | Use a DNS name as the `ip` value | Field is `None`; no error returned | TODO | | + +### Acceptance Verification + +| AC ID | Status (`TODO`/`DONE`) | Evidence | +| ----- | ---------------------- | -------- | +| AC1 | TODO | | +| AC2 | TODO | | +| AC3 | TODO | | +| AC4 | TODO | | +| AC5 | TODO | | +| AC6 | TODO | | +| AC7 | TODO | | +| AC8 | TODO | | + +## Risks and Trade-offs + +- **Breaking wire change**: Clients currently sending `peer_addr=` will have the field silently ignored after this rename. Since BEP 3 specifies `ip=` and no spec-compliant client should be sending `peer_addr=`, this is acceptable. Our own test helpers and tracker client use `peer_addr=` and are updated in scope. However, any downstream users who copied the `peer_addr=` pattern from the tracker's own documentation (which currently shows `peer_addr=` in sample URLs) will experience a silent break. Consider adding a deprecation period where both `peer_addr` and `ip` are accepted, with `peer_addr` emitting a warning, before removing it in a follow-up issue. +- **ADR timing**: The ADR decision (IP-only) reflects current tracker behaviour. No behaviour change is introduced by this issue; the ADR simply makes the policy explicit. + +## References + +- BEP 3 — The BitTorrent Protocol Specification: +- Related issue (honour `ip` param — sub-issue of #1978): to be created +- Related epic: [#1978 — Configuration Overhaul](../open/1978-configuration-overhaul-epic.md) +- opentracker `WANT_IP_FROM_QUERY_STRING`: diff --git a/project-words.txt b/project-words.txt index b8e1714df..9966f4645 100644 --- a/project-words.txt +++ b/project-words.txt @@ -159,6 +159,7 @@ hexdigit hexlify hlocalhost hmac +hostnames hotfixes hotspot hotspots From 5f87b10eba4bc73da61287c808327044b5d54656 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Wed, 15 Jul 2026 18:13:28 +0100 Subject: [PATCH 3/5] docs(issue): add spec for compact peer list default per BEP 23 (ref #1986) Add issue spec for returning compact peer list by default when the compact GET parameter is absent, aligning with the BEP 23 SUGGESTION. Includes manual verification steps using the tracker client. --- .../ISSUE.md | 187 ++++++++++++++++++ 1 file changed, 187 insertions(+) create mode 100644 docs/issues/open/1986-align-http-tracker-compact-default-with-bep-23/ISSUE.md diff --git a/docs/issues/open/1986-align-http-tracker-compact-default-with-bep-23/ISSUE.md b/docs/issues/open/1986-align-http-tracker-compact-default-with-bep-23/ISSUE.md new file mode 100644 index 000000000..2f8719f2b --- /dev/null +++ b/docs/issues/open/1986-align-http-tracker-compact-default-with-bep-23/ISSUE.md @@ -0,0 +1,187 @@ +--- +doc-type: issue +issue-type: bug +status: open +priority: p2 +github-issue: 1986 +spec-path: docs/issues/open/1986-align-http-tracker-compact-default-with-bep-23/ISSUE.md +branch: "1986-align-http-tracker-compact-default-with-bep-23" +related-pr: null +last-updated-utc: 2026-07-15 00:00 +semantic-links: + skill-links: + - create-issue + - run-tracker-locally + - use-tracker-client + related-artifacts: + - packages/axum-http-server/src/v1/handlers/announce.rs + - packages/axum-http-server/src/lib.rs + - packages/http-protocol/src/v1/requests/announce.rs + - packages/axum-http-server/tests/server/v1/contract/for_all_config_modes/receiving_an_announce_request.rs +--- + + + +# Issue #1986 - Return compact peer list by default when `compact` param is absent (BEP 23) + +## Goal + +Fix the HTTP tracker announce handler to return the compact peer list by default when the client omits the `compact` GET parameter, aligning the tracker with the SUGGESTION in [BEP 23](https://www.bittorrent.org/beps/bep_0023.html). + +## Background + +[BEP 23 — Tracker Returns Compact Peer Lists](https://www.bittorrent.org/beps/bep_0023.html) states: + +> It is SUGGESTED that trackers return compact format by default. By including `compact=0` in the announce URL, the client advises the tracker that it prefers the original format described in BEP 3, and analogously `compact=1` advises the tracker that the client prefers compact format. However the `compact` key-value pair is only advisory: the tracker MAY return using either format. `compact` is advisory so that trackers may support only the compact format. However, clients MUST continue to support both. + +The current implementation in `packages/axum-http-server/src/v1/handlers/announce.rs` only selects the compact response format when the client explicitly sends `compact=1`. When the `compact` parameter is absent (`None`), the tracker falls through to the non-compact (dictionary) branch: + +```rust +// packages/axum-http-server/src/v1/handlers/announce.rs +fn build_response(announce_request: &Announce, announce_data: DomainAnnounceData) -> Response { + // ... + if announce_request.compact.as_ref().is_some_and(|f| *f == Compact::Accepted) { + // compact path — only reached when compact=1 is explicit + } else { + // non-compact path — reached when compact=0 OR when compact is absent + } +} +``` + +This violates the BEP 23 SUGGESTION. The tracker should default to compact when no preference is expressed. + +The bug is also acknowledged in the existing module documentation and in a `code-review` comment in the contract tests: + +- `packages/axum-http-server/src/lib.rs` lines 91–95 contains a `NOTICE` that explicitly calls out this deviation. +- `packages/axum-http-server/tests/server/v1/contract/for_all_config_modes/receiving_an_announce_request.rs` contains: + +```rust +// code-review: the HTTP tracker does not return the compact response by default if the "compact" +// param is not provided in the announce URL. The BEP 23 suggest to do so. +``` + +### Why use option (a): compact by default, honour `compact=0` + +Three implementation strategies were considered: + +**(a) Compact by default; honour `compact=0` to switch to dictionary format** ← chosen +The tracker returns compact unless the client explicitly requests dictionary format via `compact=0`. This fully satisfies the BEP 23 SUGGESTION while respecting the client's explicit preference. It is the most compatible option and is the behaviour implemented by other major trackers (opentracker, chihaya). + +**(b) Always compact, ignore `compact=0`** +BEP 23 permits this — `compact` is advisory, so the tracker MAY always return compact. However, silently ignoring an explicit client preference (`compact=0`) is hostile to interoperability. Some older clients, scrapers, and Azureus/Vuze configurations rely on the dictionary format. Ignoring their request is surprising and harder to document. + +**(c) Make this a per-tracker configuration option** +Configuration is the right tool when operators have legitimate different trade-offs. Here the BEP already defines the intended behaviour unambiguously. Adding a knob pushes a spec-compliance decision onto operators who should not need to think about it. Option (a) already leaves the door open for a future simplification towards (b) if dictionary format support is ever dropped. + +## Scope + +### In Scope + +- Change `build_response` in `packages/axum-http-server/src/v1/handlers/announce.rs` so that `compact == None` (absent) is treated as compact by default, i.e. only non-compact is returned when the client explicitly sends `compact=0`. +- Update the doc comment in `packages/axum-http-server/src/lib.rs` (the `NOTICE` and the query-parameter table's `Default` column for `compact`) to reflect the new behaviour. +- Rename and invert the contract test `should_not_return_the_compact_response_by_default` → `should_return_the_compact_response_by_default` and update its assertion. +- Remove the `code-review` comment that flagged this deviation once the fix is in place. + +### Out of Scope + +- Changing the `AnnounceBuilder::default()` in `packages/http-protocol/src/v1/requests/announce.rs`, which defaults `compact` to `Some(Compact::NotAccepted)`. That builder is a test helper; its default can be revisited in a follow-up if needed. +- Always returning compact regardless of `compact=0` (option b). +- Adding a configuration option to toggle this behaviour (option c). +- Any changes to the UDP tracker protocol handling. +- Any changes to the scrape endpoint. + +## Implementation Plan + +Status values: `TODO`, `IN_PROGRESS`, `BLOCKED`, `DONE`. + +| ID | Status | Task | Notes / Expected Output | +| --- | ------ | ------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| T1 | TODO | Invert the compact-default logic in `build_response` | Change `is_some_and(Compact::Accepted)` condition so that `None` maps to compact. Only `Some(Compact::NotAccepted)` (`compact=0`) returns dictionary format. | +| T2 | TODO | Update the `NOTICE` doc comment in `packages/axum-http-server/src/lib.rs` | Remove the existing deviation notice (lines 91–95) since the behaviour will no longer deviate from BEP 23. Update the `Default` column for `compact` in the query-parameter table from `None` to `compact` (compact format). Update the `Description` column to note "compact by default per BEP 23". | +| T3 | TODO | Rename and invert the contract test | Rename `should_not_return_the_compact_response_by_default` to `should_return_the_compact_response_by_default`. Flip its assertion to confirm a compact response is returned when `compact` param is absent. Remove the `code-review` comment. | +| T4 | TODO | Verify all existing tests pass | `cargo test --workspace` — no regressions. | +| T5 | TODO | Run `linter all` | Must exit `0`. | +| T6 | TODO | Manual verification: run tracker locally and test with tracker client | Start the tracker with `cargo run` (see skill `run-tracker-locally`). Use the tracker client (see skill `use-tracker-client`) to make HTTP announce requests without `--compact`, with `--compact 1`, and with `--compact 0`. Verify the response format matches expectations for each case. Document results in the manual verification table below. | + +## Progress Tracking + +### Workflow Checkpoints + +- [ ] Spec drafted in `docs/issues/drafts/` +- [ ] Spec reviewed and approved by user/maintainer +- [ ] GitHub issue created and issue number added to this spec +- [ ] (Optional, recommended for complex issues) Spec-only PR merged into `develop` before implementation +- [ ] Implementation completed +- [ ] Automatic verification completed (`linter all`, relevant tests, and any pre-push checks) +- [ ] Manual verification scenarios executed and recorded (status + evidence) +- [ ] Acceptance criteria reviewed after implementation and updated with evidence +- [ ] Reviewer validated acceptance criteria and updated checkboxes +- [ ] Committer verified spec progress is up to date before commit +- [ ] Issue closed and spec moved from `docs/issues/open/` to `docs/issues/closed/` + +### Progress Log + +- 2026-07-15 00:00 UTC - Copilot/User - Spec drafted based on code review of `build_response`, `lib.rs` NOTICE, and the existing `code-review` comment in the contract tests. + +## Acceptance Criteria + +- [ ] AC1: When a client sends an announce request without the `compact` parameter, the tracker responds with a compact peer list. +- [ ] AC2: When a client sends `compact=1`, the tracker responds with a compact peer list. +- [ ] AC3: When a client sends `compact=0`, the tracker responds with a non-compact (dictionary) peer list. +- [ ] AC4: The contract test `should_return_the_compact_response_by_default` passes and asserts compact format when `compact` is absent. +- [ ] AC5: The contract test for `compact=0` still passes and asserts dictionary format. +- [ ] AC6: The `NOTICE` in `packages/axum-http-server/src/lib.rs` (lines 91–95) is removed since the behaviour no longer deviates from BEP 23. The query-parameter table `Default` column for `compact` accurately describes the new default (compact). +- [ ] AC7: `linter all` exits with code `0`. +- [ ] AC8: Relevant tests pass with no regressions. +- [ ] Manual verification scenarios are executed and documented (status + evidence). +- [ ] Acceptance criteria are re-reviewed after implementation and reflect actual behaviour. +- [ ] Documentation is updated when behaviour/workflow changes. + +## Verification Plan + +### Automatic Checks + +- `linter all` +- `cargo test --workspace` +- Pre-push checks (when applicable) + +### Manual Verification Scenarios + +Status values: `TODO`, `IN_PROGRESS`, `DONE`, `FAILED`, `BLOCKED`. + +| ID | Scenario | Command/Steps | Expected Result | Status | Evidence | +| --- | --------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------- | ------ | -------- | +| M1 | Announce without `compact` param — expect compact response | `curl -s "http://localhost:7070/announce?info_hash=...&peer_id=...&port=6881"` and inspect raw bencoded response | Response uses compact format (`peers` value is a bencoded string, not a list) | TODO | | +| M2 | Announce with `compact=1` — expect compact response | Add `&compact=1` to M1 URL | Response uses compact format | TODO | | +| M3 | Announce with `compact=0` — expect dictionary response | Add `&compact=0` to M1 URL | Response uses non-compact (dictionary) format (`peers` value is a bencoded list of dicts) | TODO | | +| M4 | Tracker client: announce without `--compact` — expect compact | `cargo run` (start tracker); `cargo run -p torrust-tracker-client --bin tracker_client -- http announce http://127.0.0.1:7070 9c38422213e30bff212b30c360d26f9a02136422 --port 6881` | Response uses compact format (peers encoded as a compact string) | TODO | | +| M5 | Tracker client: announce with `--compact 0` — expect dictionary | Same as M4 but add `--compact 0` | Response uses non-compact (dictionary) format | TODO | | + +### Acceptance Verification + +| AC ID | Status (`TODO`/`DONE`) | Evidence | +| ----- | ---------------------- | -------- | +| AC1 | TODO | | +| AC2 | TODO | | +| AC3 | TODO | | +| AC4 | TODO | | +| AC5 | TODO | | +| AC6 | TODO | | +| AC7 | TODO | | +| AC8 | TODO | | + +## Risks and Trade-offs + +- **Client compatibility**: Clients that previously relied on getting a dictionary response by default (no `compact` param) will now receive a compact response. Per BEP 23, all clients MUST support both formats, so this should not break any spec-compliant client. Non-compliant clients would have needed `compact=0` anyway. +- **Tracker client binary**: The project's own `tracker_client` binary (under `console/tracker-client/`) should be verified to handle compact responses correctly when it does not send `compact=0`. If the client currently relies on getting dictionary format by default, it will break after this fix. +- **Test helper `AnnounceBuilder` default**: The builder defaults to `compact=0`, which means tests using it without overriding the `compact` field continue to exercise the non-compact path. This is intentional and is not changed in this issue. It avoids accidentally masking regressions in the non-compact code path. + +## References + +- BEP 23 — Tracker Returns Compact Peer Lists: +- BEP 3 — The BitTorrent Protocol Specification: +- Related code: `packages/axum-http-server/src/v1/handlers/announce.rs` `build_response` +- Related code: `packages/axum-http-server/src/lib.rs` lines 91–95 +- Related test (renamed by this issue): `packages/axum-http-server/tests/server/v1/contract/for_all_config_modes/receiving_an_announce_request.rs` — currently `should_not_return_the_compact_response_by_default`, renamed to `should_return_the_compact_response_by_default` +- Skill: `run-tracker-locally` — `.github/skills/dev/environment-setup/run-tracker-locally/SKILL.md` +- Skill: `use-tracker-client` — `.github/skills/usage/use-tracker-client/SKILL.md` From 7c1fc8bb9fca99d9d763451447c201c83b7c4cfa Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Wed, 15 Jul 2026 18:32:27 +0100 Subject: [PATCH 4/5] docs(issue): add spec for use_ip_from_query_string config option (ref #1987) Add issue spec for an opt-in per-HTTP-tracker configuration option to honour the ip GET parameter value as the peer address instead of the TCP connection IP. Sub-issue of #1978 (configuration overhaul). Includes evidence from opentracker and chihaya confirming that neither tracker supports DNS names in the ip parameter. --- .../ISSUE.md | 180 ++++++++++++++++++ .../evidence-chihaya-no-dns-support.md | 126 ++++++++++++ .../evidence-opentracker-no-dns-support.md | 109 +++++++++++ 3 files changed, 415 insertions(+) create mode 100644 docs/issues/open/1987-add-config-option-to-use-ip-from-announce-query-string/ISSUE.md create mode 100644 docs/issues/open/1987-add-config-option-to-use-ip-from-announce-query-string/evidence-chihaya-no-dns-support.md create mode 100644 docs/issues/open/1987-add-config-option-to-use-ip-from-announce-query-string/evidence-opentracker-no-dns-support.md diff --git a/docs/issues/open/1987-add-config-option-to-use-ip-from-announce-query-string/ISSUE.md b/docs/issues/open/1987-add-config-option-to-use-ip-from-announce-query-string/ISSUE.md new file mode 100644 index 000000000..ad03756ed --- /dev/null +++ b/docs/issues/open/1987-add-config-option-to-use-ip-from-announce-query-string/ISSUE.md @@ -0,0 +1,180 @@ +--- +doc-type: issue +issue-type: feature +status: open +priority: p2 +github-issue: 1987 +spec-path: docs/issues/open/1987-add-config-option-to-use-ip-from-announce-query-string/ISSUE.md +branch: "1987-add-config-option-to-use-ip-from-announce-query-string" +related-pr: null +depends-on: + - docs/issues/open/1985-rename-peer-addr-to-ip-in-http-announce-request/ISSUE.md +blocks: null +last-updated-utc: 2026-07-15 00:00 +semantic-links: + skill-links: + - create-issue + related-artifacts: + - packages/http-protocol/src/v1/requests/announce.rs + - packages/http-core/src/services/announce.rs + - packages/configuration/src/v2_0_0/ + - docs/issues/open/1978-configuration-overhaul-epic.md + - docs/issues/open/1640-1978-per-http-tracker-on-reverse-proxy-setting.md + - evidence-opentracker-no-dns-support.md + - evidence-chihaya-no-dns-support.md +--- + + + +# Issue #1987 - Add per-HTTP-tracker config option to use peer IP from `ip` GET parameter (sub-issue of #1978) + +## Goal + +Add an optional per-HTTP-tracker configuration setting that allows the tracker to use the IP address provided in the `ip` GET parameter of the announce request instead of always deriving the peer IP from the TCP connection. This feature is analogous to opentracker's `WANT_IP_FROM_QUERY_STRING` compile-time option. + +## Background + +### Current behaviour + +The Torrust Tracker HTTP announce handler always derives the peer IP from the TCP connection (or from the `X-Forwarded-For` header when running behind a reverse proxy). The `ip` GET parameter — defined as optional in [BEP 3](https://www.bittorrent.org/beps/bep_0003.html) — is parsed but then **silently ignored**. + +BEP 3 states: + +> An optional parameter giving the IP (or dns name) which this peer is at. Generally used for the origin if it's on the same machine as the tracker. + +The BEP's "generally used for the origin" note explains the primary use case: a peer that is on the same host as the tracker announces itself and wants the tracker to register a specific routable IP (rather than `127.0.0.1` from the loopback connection). + +### Feature request + +A user request was filed (see [torrust/torrust-tracker #163 comment](https://github.com/torrust/torrust-tracker/issues/163#issuecomment-1836642956)) asking for the ability to use the IP from the query string. This mirrors opentracker's `WANT_IP_FROM_QUERY_STRING` feature, which is enabled via a compile-time flag. + +### Why it belongs to the configuration overhaul epic (#1978) + +This feature requires adding a new per-HTTP-tracker configuration field. The configuration overhaul (schema v3.0.0) is the right time to introduce new per-tracker settings cleanly, rather than adding them to the existing `v2.0.0` schema that is already being overhauled. The related per-tracker `on_reverse_proxy` setting (#1640) is being introduced in the same epic. + +### Prerequisites + +This issue depends on the `ip` GET parameter rename (from `peer_addr` to `ip`) being completed first. The rename issue must be resolved before this feature is implemented. + +### Interaction with `on_reverse_proxy` + +When both `use_ip_from_query_string` and `on_reverse_proxy` are enabled, the query string `ip` takes precedence over the `X-Forwarded-For` header. This is because the operator explicitly opted into trusting the query string value. When `use_ip_from_query_string` is disabled (default), the existing `on_reverse_proxy` logic applies unchanged. The two settings are not mutually exclusive; the query string IP wins when both are active and a valid IP is provided. + +### Security consideration + +Enabling this feature allows a remote client to claim any IP address in its announce request. The tracker would accept that address and include it in the peer list. This is a potential source of IP spoofing in the peer list. The feature must therefore be **opt-in**, disabled by default, and clearly documented as a trust-based setting — suitable only for private/controlled deployments, or as a workaround for peers behind symmetric NAT that cannot be reached via their connection IP. + +## Scope + +### In Scope + +- Add a new optional boolean configuration field to the per-HTTP-tracker configuration (name TBD during schema design, e.g. `use_ip_from_query_string`), disabled by default. +- When the option is enabled, and the `ip` GET parameter contains a valid IP address, use that IP as the peer's address instead of the connection IP. +- Document the security implications of enabling this option in the configuration schema and in the module documentation. +- Add contract tests covering both the enabled and disabled behaviour. + +### Out of Scope + +- DNS name resolution in the `ip` parameter (decided against in a separate ADR — see the rename issue). +- Changing the default behaviour (the tracker still uses the connection IP by default). +- Any changes to the UDP tracker protocol. +- Any changes to the scrape endpoint. + +## Implementation Plan + +Status values: `TODO`, `IN_PROGRESS`, `BLOCKED`, `DONE`. + +| ID | Status | Task | Notes / Expected Output | +| --- | ------ | ------------------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| T1 | TODO | Design the configuration field name and schema placement | Align with #1978 schema v3.0.0 design; propose name (e.g. `use_ip_from_query_string`) | +| T2 | TODO | Add the field to the per-HTTP-tracker configuration struct | Target the v3.0.0 schema under `packages/configuration/` as part of the #1978 overhaul | +| T3 | TODO | Thread the config value through to the announce service | `packages/http-core/src/services/announce.rs` `peer_from_request` | +| T4 | TODO | Implement the conditional IP selection in `peer_from_request` | Use `announce_request.ip` if `use_ip_from_query_string` is `true` and the field is `Some`; otherwise use the connection IP. When both `use_ip_from_query_string` and `on_reverse_proxy` are enabled, the query string IP takes precedence. Requires prerequisite issue (rename `peer_addr` → `ip`) to be merged first. | +| T5 | TODO | Add contract tests for enabled and disabled behaviour | New tests in `packages/axum-http-server/tests/` | +| T6 | TODO | Update configuration documentation | `packages/configuration/` docs and `share/default/` config file | +| T7 | TODO | Run `cargo test --workspace` — no regressions | All tests pass | +| T8 | TODO | Run `linter all` | Must exit `0` | + +## Progress Tracking + +### Workflow Checkpoints + +- [ ] Spec drafted in `docs/issues/drafts/` +- [ ] Spec reviewed and approved by user/maintainer +- [ ] GitHub issue created and issue number added to this spec +- [ ] Prerequisites completed (rename `peer_addr` → `ip` issue resolved) +- [ ] (Optional, recommended for complex issues) Spec-only PR merged into `develop` before implementation +- [ ] Implementation completed +- [ ] Automatic verification completed (`linter all`, relevant tests, and any pre-push checks) +- [ ] Manual verification scenarios executed and recorded (status + evidence) +- [ ] Acceptance criteria reviewed after implementation and updated with evidence +- [ ] Reviewer validated acceptance criteria and updated checkboxes +- [ ] Committer verified spec progress is up to date before commit +- [ ] Issue closed and spec moved from `docs/issues/open/` to `docs/issues/closed/` + +### Progress Log + +- 2026-07-15 00:00 UTC - Copilot/User - Spec drafted as a sub-issue of #1978; feature deferred to the configuration overhaul epic. + +## Acceptance Criteria + +- [ ] AC1: When `use_ip_from_query_string` is `false` (default), the tracker always uses the connection IP regardless of the `ip` GET parameter. +- [ ] AC2: When `use_ip_from_query_string` is `true` and a valid IP is provided in the `ip` GET parameter, the tracker uses that IP as the peer's address. +- [ ] AC3: When `use_ip_from_query_string` is `true` but the `ip` GET parameter is absent or contains a non-IP value, the tracker falls back to the connection IP. +- [ ] AC4: The default configuration file (`share/default/`) has `use_ip_from_query_string` set to `false` (or omitted, defaulting to `false`). +- [ ] AC5: The configuration schema documentation clearly states the security implications of enabling this option. +- [ ] AC6: Contract tests cover both enabled and disabled cases. +- [ ] AC7: `linter all` exits with code `0`. +- [ ] AC8: Relevant tests pass with no regressions. +- [ ] Manual verification scenarios are executed and documented (status + evidence). +- [ ] Acceptance criteria are re-reviewed after implementation and reflect actual behaviour. +- [ ] Documentation is updated when behaviour/workflow changes. + +## Verification Plan + +### Automatic Checks + +- `linter all` +- `cargo test --workspace` +- Pre-push checks (when applicable) + +### Manual Verification Scenarios + +Status values: `TODO`, `IN_PROGRESS`, `DONE`, `FAILED`, `BLOCKED`. + +| ID | Scenario | Command/Steps | Expected Result | Status | Evidence | +| --- | --------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------ | -------------------------------------------------------- | ------ | -------- | +| M1 | Default config: `ip` GET param is ignored | Start tracker with default config; announce with `ip=1.2.3.4` from a different source IP; check the peer list | Peer is registered with the connection IP, not `1.2.3.4` | TODO | | +| M2 | Opt-in config: `ip` GET param is used | Enable `use_ip_from_query_string`; announce with `ip=1.2.3.4`; check the peer list | Peer is registered with `1.2.3.4` | TODO | | +| M3 | Opt-in config: no `ip` param — fallback | Enable `use_ip_from_query_string`; announce without `ip` param | Peer is registered with the connection IP | TODO | | +| M4 | Opt-in + reverse proxy: `ip` param takes precedence | Enable both `use_ip_from_query_string` and `on_reverse_proxy`; announce with `ip=1.2.3.4` and `X-Forwarded-For: 5.6.7.8` | Peer is registered with `1.2.3.4` (query string wins) | TODO | | + +### Acceptance Verification + +| AC ID | Status (`TODO`/`DONE`) | Evidence | +| ----- | ---------------------- | -------- | +| AC1 | TODO | | +| AC2 | TODO | | +| AC3 | TODO | | +| AC4 | TODO | | +| AC5 | TODO | | +| AC6 | TODO | | +| AC7 | TODO | | +| AC8 | TODO | | + +## Risks and Trade-offs + +- **IP spoofing**: When enabled, a client can register any IP address in the peer list. This is inherent to the feature and must be clearly documented. The opt-in default mitigates the risk for deployments that do not need this. +- **Interaction with reverse proxy mode**: Resolved — when both `use_ip_from_query_string` and `on_reverse_proxy` are enabled, the query string `ip` takes precedence. See "Interaction with `on_reverse_proxy`" above for rationale. +- **IPv4/IPv6**: The `ip` parameter accepts both IPv4 and IPv6 addresses (via `IpAddr::from_str`). If the tracker is bound to an IPv6-only socket and a client sends an IPv4 `ip`, the address is accepted as-is — the tracker does not validate address family compatibility with the listener binding. + +## References + +- BEP 3 — The BitTorrent Protocol Specification: +- Feature request: +- Parent epic: [#1978 — Configuration Overhaul](../open/1978-configuration-overhaul-epic.md) +- Prerequisite issue: rename `peer_addr` → `ip` (to be linked once created) +- Related issue: [#1640 — Per-HTTP-tracker `on_reverse_proxy` setting](../open/1640-1978-per-http-tracker-on-reverse-proxy-setting.md) +- opentracker `WANT_IP_FROM_QUERY_STRING`: +- Research evidence — opentracker DNS name support: [evidence-opentracker-no-dns-support.md](evidence-opentracker-no-dns-support.md) +- Research evidence — chihaya DNS name support: [evidence-chihaya-no-dns-support.md](evidence-chihaya-no-dns-support.md) diff --git a/docs/issues/open/1987-add-config-option-to-use-ip-from-announce-query-string/evidence-chihaya-no-dns-support.md b/docs/issues/open/1987-add-config-option-to-use-ip-from-announce-query-string/evidence-chihaya-no-dns-support.md new file mode 100644 index 000000000..c6f362879 --- /dev/null +++ b/docs/issues/open/1987-add-config-option-to-use-ip-from-announce-query-string/evidence-chihaya-no-dns-support.md @@ -0,0 +1,126 @@ + + +# BEP 3 DNS Name Support in the `ip` Parameter + +**Date:** 2026-07-15 +**Repository:** [chihaya/chihaya](https://github.com/chihaya/chihaya) +**Branch:** `main` + +## The BEP 3 Requirement + +[BEP 3](https://www.bittorrent.org/beps/bep_0003.html) defines the optional `ip` parameter in the HTTP tracker announce request as: + +> _"An optional parameter giving the IP (or dns name) which this peer is at. Generally used for the origin if it's on the same machine as the tracker."_ + +This means the `ip` parameter should accept **both** IP addresses and DNS names (hostnames). + +## Finding: Chihaya Does NOT Support DNS Names + +Chihaya treats the `ip` parameter strictly as an IP address. DNS names are **not** supported. The value is always parsed with `net.ParseIP()`, which returns `nil` for any hostname. + +## Evidence + +### 1. Parsing — `frontend/http/parser.go` + +The `requestedIP()` function resolves the peer's IP address. All paths call `net.ParseIP()`: + +- **Line 152** — `"ip"` query param: [`net.ParseIP(ipstr)`](https://github.com/chihaya/chihaya/blob/main/frontend/http/parser.go#L152) +- **Line 155** — `"ipv4"` query param: [`net.ParseIP(ipstr)`](https://github.com/chihaya/chihaya/blob/main/frontend/http/parser.go#L155) +- **Line 158** — `"ipv6"` query param: [`net.ParseIP(ipstr)`](https://github.com/chihaya/chihaya/blob/main/frontend/http/parser.go#L158) +- **Line 163** — `RealIPHeader` (e.g. `X-Forwarded-For`): [`net.ParseIP(ip)`](https://github.com/chihaya/chihaya/blob/main/frontend/http/parser.go#L163) +- **Line 166** — `r.RemoteAddr` (TCP connection fallback): [`net.ParseIP(host)`](https://github.com/chihaya/chihaya/blob/main/frontend/http/parser.go#L166) + +```go +// frontend/http/parser.go lines 148-167 +func requestedIP(r *http.Request, p bittorrent.Params, opts ParseOptions) (ip net.IP, provided bool) { + if opts.AllowIPSpoofing { + if ipstr, ok := p.String("ip"); ok { + return net.ParseIP(ipstr), true + } + + if ipstr, ok := p.String("ipv4"); ok { + return net.ParseIP(ipstr), true + } + + if ipstr, ok := p.String("ipv6"); ok { + return net.ParseIP(ipstr), true + } + } + + if opts.RealIPHeader != "" { + if ip := r.Header.Get(opts.RealIPHeader); ip != "" { + return net.ParseIP(ip), false + } + } + + host, _, _ := net.SplitHostPort(r.RemoteAddr) + return net.ParseIP(host), false +} +``` + +If `net.ParseIP` returns `nil` (as it would for any DNS name), the request is rejected at **[line 112](https://github.com/chihaya/chihaya/blob/main/frontend/http/parser.go#L112)**: + +```go +if request.IP.IP == nil { + return nil, bittorrent.ClientError("failed to parse peer IP address") +} +``` + +### 2. Validation — `bittorrent/sanitize.go` + +The `SanitizeAnnounce()` function performs a second validation in **[lines 28–37](https://github.com/chihaya/chihaya/blob/main/bittorrent/sanitize.go#L28-L37)**. The IP must be a valid IPv4 or IPv6 address; otherwise `ErrInvalidIP` is returned: + +```go +if ip := r.IP.To4(); ip != nil { + r.IP.IP = ip + r.IP.AddressFamily = IPv4 +} else if len(r.IP.IP) == net.IPv6len { // implies r.IP.To4() == nil + r.IP.AddressFamily = IPv6 +} else { + return ErrInvalidIP +} +``` + +### 3. Data Structures — `bittorrent/bittorrent.go` + +The `IP` type at **[line 210](https://github.com/chihaya/chihaya/blob/main/bittorrent/bittorrent.go#L210)** wraps `net.IP` — a raw byte representation of an IP address. It has no field to store a DNS name: + +```go +type IP struct { + net.IP + AddressFamily +} +``` + +The `Peer` struct at **[line 230](https://github.com/chihaya/chihaya/blob/main/bittorrent/bittorrent.go#L230)** embeds this `IP` type: + +```go +type Peer struct { + ID PeerID + IP IP + Port uint16 +} +``` + +### 4. No DNS Resolution in the Codebase + +A search for `net.LookupHost`, `net.LookupIP`, or any DNS resolution function across the entire codebase returns **zero results**. There is no mechanism to resolve a hostname to an IP address. + +## Impact + +| Aspect | Current Behavior | +| ------------------------------- | ------------------------------------------------ | +| `ip` param accepting DNS names | ❌ No | +| `net.ParseIP` on `ip` value | ✅ Yes | +| DNS resolution (`net.LookupIP`) | ❌ No | +| Error returned for DNS names | `ClientError("failed to parse peer IP address")` | + +A DNS name like `"tracker.example.com"` would fail `net.ParseIP()` and be rejected with a client error before any further processing occurs. + +## What Would Need to Change + +To support DNS names as per BEP 3, the following areas would need modification: + +1. **[`frontend/http/parser.go`](https://github.com/chihaya/chihaya/blob/main/frontend/http/parser.go)** — `requestedIP()`: detect when the value is a hostname (fails `net.ParseIP()` but is a non-empty string), then call `net.LookupIP()` to resolve it. +2. **[`bittorrent/bittorrent.go`](https://github.com/chihaya/chihaya/blob/main/bittorrent/bittorrent.go)** — `IP` struct: potentially store the original DNS name alongside the resolved IP. +3. **[`bittorrent/sanitize.go`](https://github.com/chihaya/chihaya/blob/main/bittorrent/sanitize.go)** — `SanitizeAnnounce()`: handle the case where the IP was resolved from a DNS name (the `AddressFamily` would be known after resolution). diff --git a/docs/issues/open/1987-add-config-option-to-use-ip-from-announce-query-string/evidence-opentracker-no-dns-support.md b/docs/issues/open/1987-add-config-option-to-use-ip-from-announce-query-string/evidence-opentracker-no-dns-support.md new file mode 100644 index 000000000..60bdd2ee4 --- /dev/null +++ b/docs/issues/open/1987-add-config-option-to-use-ip-from-announce-query-string/evidence-opentracker-no-dns-support.md @@ -0,0 +1,109 @@ + + +# DNS Name Support in the `ip` Announce Parameter + +## BEP 3 Specification + +[BEP 3](https://www.bittorrent.org/beps/bep_0003.html) states about the `ip` GET parameter in the HTTP tracker announce request: + +> **ip**: An optional parameter giving the IP (or dns name) which this peer is at. Generally used for the origin if it's on the same machine as the tracker. + +## Finding: This Tracker Does NOT Support DNS Names in `ip` + +The opentracker implementation does **not** support DNS names in the `ip` parameter. Only literal IPv4/IPv6 addresses are accepted, and even that only when explicitly enabled at compile time. + +--- + +## Evidence + +### 1. The `ip` parameter is gated behind a compile-time feature flag + +**File:** `Makefile`, lines 24-25 + +```makefile +#FEATURES+=-DWANT_IP_FROM_QUERY_STRING +``` + +The feature is **commented out by default**. Without `-DWANT_IP_FROM_QUERY_STRING`, the `ip` parameter is not even recognized as a valid keyword. + +**File:** `ot_http.c`, lines 497-503 + +```c +static ot_keywords keywords_announce[] = { + {"port", 1}, {"left", 2}, {"event", 3}, {"numwant", 4}, + {"compact", 5}, {"compact6", 5}, {"info_hash", 6}, +#ifdef WANT_IP_FROM_QUERY_STRING + {"ip", 7}, +#endif +#ifdef WANT_FULLLOG_NETWORKS + {"lognet", 8}, +#endif + {"peer_id", 9}, {NULL, -3}}; +``` + +The `{"ip", 7}` entry only exists in the keyword table when `WANT_IP_FROM_QUERY_STRING` is defined. + +### 2. When enabled, the `ip` value is parsed with `scan_ip6()` — a literal IP parser only + +**File:** `ot_http.c`, lines 607-614 + +```c +#ifdef WANT_IP_FROM_QUERY_STRING + case 7: /* matched "ip" */ + { + char *tmp_buf1 = ws->reply, *tmp_buf2 = ws->reply + 16; + len = scan_urlencoded_query(&read_ptr, tmp_buf2, SCAN_SEARCHPATH_VALUE); + tmp_buf2[len] = 0; + if ((len <= 0) || !scan_ip6(tmp_buf2, tmp_buf1)) + HTTPERROR_400_PARAM; + OT_SETIP(&ws->peer, tmp_buf1); + } break; +#endif +``` + +The value from the `ip` parameter is passed directly to `scan_ip6()`. This function comes from the [libowfat](http://www.fefe.de/libowfat/) library and is a pure string parser that only handles literal IPv6 address notation (including IPv4-mapped IPv6 addresses like `::ffff:192.0.2.1`). It does **not** perform DNS resolution. + +### 3. No DNS resolution code exists anywhere in the codebase + +A search across the entire repository for DNS-related functions returned zero results: + +| Search Term | Matches | +| --------------- | ------------------------------------------ | +| `gethostbyname` | 0 | +| `getaddrinfo` | 0 | +| `inet_pton` | 0 | +| `inet_aton` | 0 | +| `dns` | 0 (only a false positive in `.git/hooks/`) | +| `resolve` | 0 | + +There is simply no code in this project that resolves hostnames to IP addresses. + +### 4. The same pattern applies to the proxy/X-Forwarded-For path + +**File:** `ot_http.c`, lines 521-528 + +```c +#ifdef WANT_IP_FROM_PROXY + if (accesslist_is_blessed(cookie->ip, OT_PERMISSION_MAY_PROXY)) { + ot_ip6 proxied_ip; + char *fwd = http_header(ws->request, ws->header_size, "x-forwarded-for"); + if (fwd && scan_ip6(fwd, proxied_ip)) { + OT_SETIP(ws->peer, proxied_ip); +``` + +Even the alternative `WANT_IP_FROM_PROXY` path (which reads the peer IP from the `X-Forwarded-For` header) uses `scan_ip6()` and therefore also only accepts literal IP addresses, not DNS names. + +--- + +## Summary + +| Aspect | Status | +| --------------------------------- | ----------------------------------------------------------------------------- | +| `ip` param recognized by default? | ❌ No — requires `-DWANT_IP_FROM_QUERY_STRING` | +| DNS names supported in `ip`? | ❌ No — only literal IPv6/IPv4 addresses via `scan_ip6()` | +| Any DNS resolution in codebase? | ❌ No — zero occurrences of `gethostbyname`, `getaddrinfo`, `inet_pton`, etc. | + +The BEP 3 specification allows DNS names in the `ip` parameter, but this tracker implementation does not support them. To add DNS name support, one would need to: + +1. Enable `WANT_IP_FROM_QUERY_STRING` at compile time. +2. Modify the `case 7` handler in `http_handle_announce()` to detect non-IP values and resolve them via `getaddrinfo()` before falling back to `scan_ip6()`. From ac9ba0e958b53e2fe9e3c97e989ebf5e34cad0f5 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Wed, 15 Jul 2026 19:04:32 +0100 Subject: [PATCH 5/5] fix(docs): address Copilot review comments on PR #1988 - Fix broken relative link to docs/adrs/ in 1985 ISSUE.md (../adrs/ -> ../../adrs/ from the nested issue folder) - Fix broken relative links to sibling issues in 1985 ISSUE.md (../open/1978-... -> ../1978-...) - Fix broken relative links to sibling issues in 1987 ISSUE.md (../open/1978-... and ../open/1640-... -> ../1978-... and ../1640-...) - Clarify tool naming in researcher.agent.md: github_text_search / github_repo tools are optional; fall back to gh CLI when unavailable --- .github/agents/researcher.agent.md | 7 +++++-- .../ISSUE.md | 4 ++-- .../ISSUE.md | 4 ++-- 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/.github/agents/researcher.agent.md b/.github/agents/researcher.agent.md index 397913ef4..a4615e1db 100644 --- a/.github/agents/researcher.agent.md +++ b/.github/agents/researcher.agent.md @@ -26,7 +26,8 @@ You gather facts. You do not make implementation decisions or write production c 1. Clone external tracker implementations (opentracker, chihaya, etc.) and search their source code for specific patterns, behaviors, or configuration options. 2. Search external GitHub repositories for relevant issues, PRs, and discussions using the - `github_text_search` and `github_repo` tools. + `github_repo` and `github_text_search` search tools where available, or the `gh` CLI + (`gh issue list`, `gh search issues`) when MCP tools are not accessible. 3. Read external documentation (BEPs, wiki pages, READMEs) to verify claims. 4. Compare implementations across multiple trackers and identify the de-facto standard behavior. 5. Return structured, evidence-backed findings with source references (file paths, line numbers, @@ -51,7 +52,9 @@ Typical research questions include: 3. **Gather evidence**: - For source code research: clone the repo (shallow clone with `--depth 1`), then use `grep`, `find`, and `git log` to locate relevant code. - - For issue research: use `github_text_search` with the target org/repo and relevant keywords. + - For issue research: use the `github_repo` or `github_text_search` search tools where + available, or fall back to `gh search issues --repo ` via the + terminal. - For documentation: fetch and read relevant web pages or local docs. 4. **Cross-reference findings**: Compare evidence across multiple sources. Note agreements and disagreements. diff --git a/docs/issues/open/1985-rename-peer-addr-to-ip-in-http-announce-request/ISSUE.md b/docs/issues/open/1985-rename-peer-addr-to-ip-in-http-announce-request/ISSUE.md index 44fb1aa16..3957fd81d 100644 --- a/docs/issues/open/1985-rename-peer-addr-to-ip-in-http-announce-request/ISSUE.md +++ b/docs/issues/open/1985-rename-peer-addr-to-ip-in-http-announce-request/ISSUE.md @@ -55,7 +55,7 @@ BEP 3 specifies the `ip` parameter as accepting "IP (or dns name)". In practice: - The tracker's peer list stores `IpAddr` values, not hostnames. Supporting DNS would require either resolving names at announce time (latency, DoS vector) or storing hostnames (incompatible with the peer list model). - The current behaviour (silently drop non-IP values) is confusing and undocumented. -An explicit decision is needed. The decision is captured in the ADR drafted as part of this issue: [`docs/adrs/YYYYMMDD_accept_only_ip_addresses_in_http_announce_ip_param.md`](../adrs/). +An explicit decision is needed. The decision is captured in the ADR drafted as part of this issue: [`docs/adrs/YYYYMMDD_accept_only_ip_addresses_in_http_announce_ip_param.md`](../../adrs/). ### The "honour the `ip` param" question @@ -223,5 +223,5 @@ Status values: `TODO`, `IN_PROGRESS`, `DONE`, `FAILED`, `BLOCKED`. - BEP 3 — The BitTorrent Protocol Specification: - Related issue (honour `ip` param — sub-issue of #1978): to be created -- Related epic: [#1978 — Configuration Overhaul](../open/1978-configuration-overhaul-epic.md) +- Related epic: [#1978 — Configuration Overhaul](../1978-configuration-overhaul-epic.md) - opentracker `WANT_IP_FROM_QUERY_STRING`: diff --git a/docs/issues/open/1987-add-config-option-to-use-ip-from-announce-query-string/ISSUE.md b/docs/issues/open/1987-add-config-option-to-use-ip-from-announce-query-string/ISSUE.md index ad03756ed..0959dafa0 100644 --- a/docs/issues/open/1987-add-config-option-to-use-ip-from-announce-query-string/ISSUE.md +++ b/docs/issues/open/1987-add-config-option-to-use-ip-from-announce-query-string/ISSUE.md @@ -172,9 +172,9 @@ Status values: `TODO`, `IN_PROGRESS`, `DONE`, `FAILED`, `BLOCKED`. - BEP 3 — The BitTorrent Protocol Specification: - Feature request: -- Parent epic: [#1978 — Configuration Overhaul](../open/1978-configuration-overhaul-epic.md) +- Parent epic: [#1978 — Configuration Overhaul](../1978-configuration-overhaul-epic.md) - Prerequisite issue: rename `peer_addr` → `ip` (to be linked once created) -- Related issue: [#1640 — Per-HTTP-tracker `on_reverse_proxy` setting](../open/1640-1978-per-http-tracker-on-reverse-proxy-setting.md) +- Related issue: [#1640 — Per-HTTP-tracker `on_reverse_proxy` setting](../1640-1978-per-http-tracker-on-reverse-proxy-setting.md) - opentracker `WANT_IP_FROM_QUERY_STRING`: - Research evidence — opentracker DNS name support: [evidence-opentracker-no-dns-support.md](evidence-opentracker-no-dns-support.md) - Research evidence — chihaya DNS name support: [evidence-chihaya-no-dns-support.md](evidence-chihaya-no-dns-support.md)