Repository navigation
feat(request): add client IP address extractor - #724
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The Forwarded header parser treats quoted commas and semicolons as delimiters, allowing client-controlled metadata to produce incorrect client IPs and potentially enable IP spoofing through a trusted proxy.
1 open issue
What changed in this PR
This follow-up to #673 adds client IP extraction to Cot’s request handling, with configurable trusted proxies and forwarding headers.
Changes:
- Adds
ClientIpAddr, using the connection’s peer address unless a trusted proxy supplies forwarding headers. - Introduces client IP configuration, documentation, and the
ipnetdependency. - Adds test-request support, parser tests, and an end-to-end extraction test.
| File | Description |
|---|---|
docs/configuration.md |
Documents trusted proxies and forwarding headers. |
cot/tests/extractors.rs |
Adds an end-to-end client IP test. |
cot/src/test.rs |
Supports peer addresses in test requests. |
cot/src/request/extractors/client_ip.rs |
Implements client IP extraction and parser tests. |
cot/src/request/extractors.rs |
Exports ClientIpAddr. |
cot/src/request.rs |
Defines the internal peer-address extension. |
cot/src/project.rs |
Attaches connection peer addresses to requests. |
cot/src/config.rs |
Adds proxy and header configuration with defaults and tests. |
cot/Cargo.toml |
Adds the workspace ipnet dependency. |
Cargo.toml |
Declares ipnet with serialization support. |
Cargo.lock |
Records the updated dependency relationships. |
Review effort: Balanced
|
| Project | cot |
| Branch | client-ip-extractor-fixes |
| Testbed | github-ubuntu-latest |
Click to view all benchmark results
| Benchmark | Latency | Benchmark Result microseconds (µs) (Result Δ%) | Upper Boundary microseconds (µs) (Limit %) |
|---|---|---|---|
| empty_router/empty_router | 📈 view plot 🚷 view threshold | 8,930.60 µs(-27.58%)Baseline: 12,331.92 µs | 18,304.07 µs (48.79%) |
| json_api/json_api | 📈 view plot 🚷 view threshold | 985.82 µs(+1.19%)Baseline: 974.23 µs | 1,367.30 µs (72.10%) |
| nested_routers/nested_routers | 📈 view plot 🚷 view threshold | 959.60 µs(+3.74%)Baseline: 925.04 µs | 1,280.81 µs (74.92%) |
| single_root_route/single_root_route | 📈 view plot 🚷 view threshold | 927.63 µs(+5.17%)Baseline: 882.00 µs | 1,229.57 µs (75.44%) |
| single_root_route_burst/single_root_route_burst | 📈 view plot 🚷 view threshold | 16,655.00 µs(+1.76%)Baseline: 16,366.89 µs | 22,689.57 µs (73.40%) |
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
172a0ea to
dc417b4
Compare
There was a problem hiding this comment.
🟡 Changes recommended
IPv4-mapped IPv6 proxy addresses fail to match trusted IPv4 networks, causing forwarding headers to be ignored or proxy traversal to stop prematurely; canonical address matching and regression tests are needed.
1 open issue
1 resolved since last review
Review effort: Balanced
dc417b4 to
66b7332
Compare
66b7332 to
b310d11
Compare
b310d11 to
3c1a3a9
Compare
- Return an error instead of panicking when the remote address of the request is unknown. - Use the right-most untrusted address across all header instances, not the client-controlled left-most one. - Treat invalid addresses as unknown instead of returning 500. - Add `[client_ip]` to the config reference.
- Move header parsing into a private module. - Drop `RequestExt::client_ip`; rename `direct_peer_ip` to `peer_ip`. - Use the standard builder for `ClientIpConfig`; drop getters and `new`. - Make the tests table-driven.
3c1a3a9 to
25fef2e
Compare
- Return an error instead of panicking when the remote address of the request is unknown. - Use the right-most untrusted address across all header instances, not the client-controlled left-most one. - Treat invalid addresses as unknown instead of returning 500. - Add `[client_ip]` to the config reference.
- Move header parsing into a private module. - Drop `RequestExt::client_ip`; rename `direct_peer_ip` to `peer_ip`. - Use the standard builder for `ClientIpConfig`; drop getters and `new`. - Make the tests table-driven.


Description
This is a followup to #673, which refactors the code a little bit and fixes a few shortcomings.
The details are in the commits. I've left the work done by @Wervice as the base commit (mostly unchaged - only fixed some
hypermigration-related rebase issues) to properly attribute their work, and applied my changes on top of that.We should merge this by rebasing, not by squashing as usual.
Type of change
Checklist
just test-all)just clippy)cargo fmt)