feat(http): validate announce query IP overrides - #2063
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #2063 +/- ##
===========================================
+ Coverage 81.97% 82.24% +0.26%
===========================================
Files 349 349
Lines 24849 25142 +293
Branches 24849 25142 +293
===========================================
+ Hits 20369 20677 +308
+ Misses 4173 4154 -19
- Partials 307 311 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Implements the staged HTTP announce ip query-parameter behavior for issue #1987 by preserving the raw ip parameter state at the HTTP-protocol boundary, enforcing a disabled-by-default override policy in HTTP core, and adding bounded observability (metrics + debug events) for rejected non-empty overrides. This fits into the tracker’s HTTP announce request handling path (protocol parsing → service-level policy → event/metrics) while preparing for later runtime wiring once configuration schema v3 is activated by #1980.
Changes:
- Introduces a raw-state
PeerIpmodel in the HTTP protocol layer and updates announce parsing/serialization and related tests. - Adds HTTP-core policy enforcement for non-empty
ipoverrides (disabled by default), plus bounded rejection events and metrics. - Adds v3 per-HTTP-tracker config field
use_ip_from_query_string(defaultfalse) and updates contract tests and issue documentation/evidence.
Reviewed changes
Copilot reviewed 19 out of 20 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/scaffold.rs | Removes ip=... from a multi-tracker announce fixture to align with disabled-policy behavior. |
| packages/test-helpers/src/http.rs | Updates helper announce construction to use PeerIp::Absent. |
| packages/http-protocol/src/v1/requests/announce.rs | Adds PeerIp raw-state enum, strict parsing, updates Announce to use it, and expands unit tests. |
| packages/http-core/src/statistics/mod.rs | Adds a new counter metric definition for rejected announce peer IP parameters. |
| packages/http-core/src/statistics/metrics.rs | Exposes a typed accessor for the new rejection counter (labelled by bounded reason). |
| packages/http-core/src/statistics/event/handler.rs | Records a new rejection event into metrics with a bounded reason label. |
| packages/http-core/src/services/announce.rs | Enforces peer-IP selection policy and maps rejections into protocol failure responses + emits rejection events. |
| packages/http-core/src/event.rs | Adds a new HTTP-core event and bounded PeerIpRejectionReason enum. |
| packages/http-core/src/container.rs | Wires the announce service with an explicitly disabled peer-IP override policy (pending #1980). |
| packages/http-core/benches/helpers/util.rs | Updates benchmark announce construction to use PeerIp::Absent. |
| packages/configuration/src/v3_0_0/http_tracker.rs | Adds use_ip_from_query_string config field (default false) with tests + security docs. |
| packages/axum-http-server/tests/server/v1/contract/for_all_config_modes/receiving_an_announce_request.rs | Updates/expands contract coverage for disabled-policy failure reasons and invalid/DNS/malformed ip cases. |
| packages/axum-http-server/src/v1/handlers/announce.rs | Reduces debug logging to avoid dumping the full announce request struct. |
| packages/axum-http-server/src/v1/extractors/announce_request.rs | Updates extractor test expectations for PeerIp::Literal. |
| packages/axum-http-server/Cargo.toml | Removes local-ip-address dev-dependency (no longer needed by tests). |
| docs/issues/open/1987-add-config-option-to-use-ip-from-announce-query-string/manual-verification.md | Adds reproducible manual verification evidence for baseline + disabled-policy behavior. |
| docs/issues/open/1987-add-config-option-to-use-ip-from-announce-query-string/ISSUE.md | Updates spec, acceptance criteria, progress log, and staged activation notes. |
| docs/issues/open/1980-1978-configuration-overhaul-final-cleanup.md | Updates last-updated timestamp and adds a task to run #1987 enabled-mode verification post-#1980. |
| docs/issues/open/1978-configuration-overhaul-epic/configuration-v2-to-v3-migration.md | Notes #1987 staged delivery pending #1980 runtime v3 activation. |
| Cargo.lock | Removes now-unused dependency graph entries related to local-ip-address. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
josecelano
left a comment
There was a problem hiding this comment.
Addressing Copilot review suggestions in a follow-up commit.
josecelano
left a comment
There was a problem hiding this comment.
Both Copilot suggestions have been addressed in 6928864: explicit empty ip values now round-trip as ip=, and the malformed-IP decoding error now accurately covers both malformed percent encoding and invalid UTF-8 with regression tests for each. The associated inline threads are resolved.
|
ACK d00bd1b |
Summary
Implements issue #1987's staged HTTP announce
ipquery-parameter behavior.ipstates at the protocol boundary.use_ip_from_query_stringfield, defaulting tofalse.Staged activation
Production remains intentionally configured with a disabled policy until #1980 activates schema v3.0.0 at runtime. Enabled-policy behavior, including query-IP precedence over
X-Forwarded-For, is covered by focused tests. The remaining enabled-mode local verification is documented as blocked on #1980 in the issue evidence.Verification
linter all, and pre-commit.docs/issues/open/1987-add-config-option-to-use-ip-from-announce-query-string/manual-verification.md.Closes #1987.