Skip to content

feat: add remote address extractor - #673

Closed
Wervice wants to merge 9 commits into
cot-rs:masterfrom
Wervice:ip-extractor
Closed

Wervice wants to merge 9 commits into
cot-rs:masterfrom
Wervice:ip-extractor

Conversation

@Wervice

@Wervice Wervice commented Sep 15, 2026 •

Copy link
Copy Markdown
Contributor

I added an extractor RemoteAddr in the extractors.rs. This extractor works by getting an extension from the HTTP headers which contains the remote address.
For this extension to be set, axum has to be started with handler.into_make_service_with_connect_info.
The extractor then provides the IP address. Furthermore, when a proxied connection was declared using one of the following headers, the proxied IP will be used:

  • CF-Connecting-IP
  • CloudFront-Viewer-Address
  • Fly-Client-IP
  • Forwarded
  • X-Forwarded-For
  • True-Client-IP
  • X-Envoy-External-Address
  • X-Real-Ip

In order to implement proxied HTTP header detection the client_ip crate was used.
In order to comply with the orphan rules, cot-core was given access to tokio via tokio.workspace = true.

Related issue or discussion

Description

Type of change

  • Bug fix
  • New feature
  • Documentation
  • Refactor / cleanup
  • Performance improvement
  • Other (describe above)

Checklist

  • I've read the contributing guide
  • Tests pass locally (just test-all)
  • Code passes clippy (just clippy)
  • Code is properly formatted (cargo fmt)
  • New tests added (regression test for bugs, coverage for new features)
  • Documentation (both code and site) updated (if applicable)

I added an extractor `RemoteAddr` in the `extractors.rs`.
This extractor works by getting an extension from the HTTP headers which
contains the remote address.
For this extension to be set, axum has to be started with
`handler.into_make_service_with_connect_info`.
The extractor then provides the IP address. Furthermore, when a proxied
connection was declared using one of the following headers, the proxied
IP will be used:
- CF-Connecting-IP
- CloudFront-Viewer-Address
- Fly-Client-IP
- Forwarded
- X-Forwarded-For
- True-Client-IP
- X-Envoy-External-Address
- X-Real-Ip

In order to implement proxied HTTP header detection the `client_ip`
crate was used.
In order to comply with the orphan rules, `cot-core` was given access to
`tokio` via `tokio.workspace = true`.
@github-actions github-actions Bot added C-lib Crate: cot (main library crate) C-core labels Sep 15, 2026
@Wervice Wervice changed the title Added IP address extractor feat: add remote address extractor Sep 15, 2026
@Wervice
Wervice marked this pull request as draft September 16, 2026 12:08
I added support for configuring a proxy via a new trusted_proxy field in
the configuration file.
The field supports CIDR notated subnets and several headers.
I cut the `client-ip` dependency.
Currently supported headers are:
- Forwarded
- X-Forwarded-For
- CF-Connecting-IP
- X-Real-IP
@Wervice
Wervice marked this pull request as ready for review September 20, 2026 16:22

@ElijahAhianyo ElijahAhianyo left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey! Thanks for your contribution. Please address the comments and this should be in a good shape


#[cfg(test)]
mod tests {
use serde::Deserialize;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why was this moved?

Comment thread cot-core/src/request/extractors.rs Outdated
use crate::request::extractors::{FromRequest, Json, Path, UrlQuery};

use serde::Deserialize;
use std::net::{Ipv4Addr, Ipv6Addr};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't use this import anywhere, no?

Comment thread cot/src/config.rs Outdated
Comment on lines +1256 to +1281
pub struct IpWithSubnet {
ip: IpAddr,
mask: Option<u8>,
}

impl Serialize for IpWithSubnet {
fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
where
S: serde::Serializer,
{
if let Some(mask) = self.mask {
serializer.serialize_str(&format!("{}/{}", self.ip, mask))
} else {
serializer.serialize_str(&self.ip.to_string())
}
}
}

struct IpWithSubnetVistor;

impl Visitor<'_> for IpWithSubnetVistor {
type Value = String;

fn expecting(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
formatter.write_str("a CIDR notation")
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about this; doing this manually is a bit too finicky and error-prone. If the goal is to support Cidr, we should delegate this to a battle-tested dependency like ipnet

Comment thread cot/src/remote_addr.rs Outdated
Comment on lines +95 to +105
/// An extractor that extracts the IP address of the remote.
/// This automatically checks for proxy IP headers and contains their IP if one such is specified.
///
/// # Examples
/// ```rust
/// pub async fn example_handler(ip: RemoteAddr) -> cot::Result<Html> {
/// dbg!(ip.ip()); // Prints the IP as a debug statement
/// dbg!(ip.closest_ip()); // Prints the closest IP as a debug statement
/// ///...
/// }
/// ```

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The extractor should live in requests/extractor.rs

Comment thread cot/src/config.rs Outdated
Comment on lines +1224 to +1249
pub struct ProxyConfig {
trusted_proxies: Vec<IpWithSubnet>,
trusted_headers: Vec<String>,
}

impl ProxyConfig {
#[must_use]
/// Get the proxies that have been configured to be trusted.
pub fn get_trusted_proxies(&self) -> Vec<IpWithSubnet> {
self.trusted_proxies.clone()
}

#[must_use]
/// Get the proxy headers that have been configured to be trusted.
pub fn get_trusted_headers(&self) -> Vec<HeaderName> {
self.trusted_headers
.iter()
.filter_map(|h| HeaderName::from_str(h).ok())
.collect()
}

#[must_use]
/// Create a new [`ProxyConfig`] with trusted proxies and trusted headers.
pub fn new(proxies: Vec<IpWithSubnet>, headers: Vec<String>) -> Self {
Self {
trusted_proxies: proxies,

@ElijahAhianyo ElijahAhianyo Sep 21, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should aim for some consistency in the API here. The config struct here should be a builder(we do that by using the derive_builder::Builder macro).

We should aim for something like this:

#[derive(Debug, Clone, PartialEq, Eq, serde::Serialize, serde::Deserialize, derive_builder::Builder)]
#[builder(build_fn(name = "build_impl"))]
pub struct ClientIpConfig {
    #[serde(default)]
    #[builder(default)]
    pub trusted_proxies: TrustedProxies,

    #[serde(default = "default_client_ip_headers")]
    #[builder(default = "default_client_ip_headers()")]
    pub headers: Vec<ClientIpHeader>,
}

fn default_client_ip_headers() -> Vec<ClientIpHeader> {
    vec![ClientIpHeader::XForwardedFor]
}

impl Default for ClientIpConfig {
    fn default() -> Self {
        Self {
            trusted_proxies: TrustedProxies::default(),
            headers: default_client_ip_headers(),
        }
    }
}

While we're here, I think we need some kind of type-safety with deserializing proxy IPs and headers. For headers, we want to provide an enum(ClientIpHeader in the case of the example above), with supported headers (x-forwarded-for, true-client-ip, etc) as variant but also an escape hatch for custom headers. This should look somewhat like this:

#[derive(Debug, Clone, PartialEq, Eq)]
pub enum ClientIpHeader {
    /// `X-Forwarded-For`, the de facto standard, a comma-separated list of
    /// hops, closest-to-origin first.
    XForwardedFor,
    /// `X-Real-IP`,  commonly set by nginx, a single address.
    XRealIp,
    /// `True-Client-IP`, used by Akamai and Cloudflare Enterprise.
    TrueClientIp,
    /// `CF-Connecting-IP` set by Cloudflare.
    CfConnectingIp,
    /// Any other header name.
    Custom(HeaderName),
}

I would also expect TrustedProxies (in the example above) to look something like this:

#[derive(Debug, Clone, Default, PartialEq, Eq)]
pub struct TrustedProxies(Vec<TrustedProxyEntry>);

#[derive(Debug, Clone, Copy, PartialEq, Eq)]
enum TrustedProxyEntry {
    /// Trusts every proxy address. (we should have a security note that states the risk of using this)
    Any,
    /// Trusts every address in this network (a single address is a network
    /// with a full-length prefix). 
    Network(ipnet::IpNet), 
}

In terms of design, i would expect to abstract the header and trusted proxy structs into the cot-core crate (cot-core/src/request/client_ip.rs or equivalent) since they dont need axum or the project config

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't TrustedProxyEntry::Any be equivalent to the user having 0.0.0.0/0 and ::/0?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These belong to 2 different families (IPv4 and IPv6). 0.0.0.0/0 means allowing all IPv4 addresses and doesn't apply to IPv6, whereas ::/0 means allowing all IPv6 addresses and doesn't apply to IPv4. We should allow people to do that. In theory, TrustedProxyEntry::Any is equivalent to (or desugars to) specifying ["0.0.0.0/0", "::/0"]. So specifying 0.0.0.0/0 or ::/0 should not collapse into TrustedProxyEntry::Any.

Comment thread cot/src/remote_addr.rs Outdated
@@ -0,0 +1,586 @@
use axum::{extract::connect_info::Connected, serve::IncomingStream};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should improve the abstraction design here. Core components, code, or data structures that do not integrate with third-party dependencies in cot (like axum) or ProjectConfig should move into cot-core. In other words, the contents of this file should move into the appropriate modules

Comment thread cot/src/remote_addr.rs Outdated
Comment on lines +131 to +140
impl<'a> Connected<IncomingStream<'a, tokio::net::TcpListener>> for RemoteAddr {
fn connect_info(stream: IncomingStream<'a, tokio::net::TcpListener>) -> Self {
let closest_ip = stream.remote_addr().ip();
RemoteAddr {
direct: closest_ip,
proxied: None,
}
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Connected<IncomingStream<'a, TcpListener>> must be implemented for this to work:

axum::serve(
        listener,
        handler.into_make_service_with_connect_info::<RemoteAddr>(),
    )

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is essentially so that axum can give me the IP address in the first place.

- I moved the code into different files and some to cot-core
- I removed my own IpWithSubnet in favour of IpNet
- The code now uses an enum for handling headers
- I fixed some mistakes that made the CI/CD fail

@m4tx m4tx left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, thanks for this, and for your patience waiting for the review! I've added a few comments - when these will be addressed, I think we can merge this!

Comment thread cot/src/config.rs

#[derive(Debug, Clone, PartialEq, Eq)]
/// A header used for deriving the IP of the client.
pub enum ClientIpHeader {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if we need an enum for this. Perhaps just a regular String (or some newtype wrapping a String) would do? We could have the common values listed in the docs, and we wouldn't limit the users to a predefined set of values.

EDIT: ahh, I see the behavior is in fact different for different headers. I'm wondering if we should have a "custom" enum field, though, to specify a header name that would be used for when a single address arrives. This would allow us to remove XRealIp, TrueClientIp, and CfConnectingIp, which would be nice considering they are really focused on specific reverse proxies and are not really standard (and they seem to share the same behavior).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this is reasonable. I changed it in an upcoming commit.

Comment thread cot-core/src/lib.rs
//! the Cot framework and for building custom extensions.

mod body;
pub mod remote_addr;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think there's a value in making this public? I'm not exactly sure if we want to expose this, as the usefulness of this for end-users is probably pretty limited, and we'd need to keep the public API promises if it's pub.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly, this should only be public inside the workspace, no? So as long as it doesn't get exported from cot/src/lib.rs it shouldn't be public?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right, this is cot-core, not cot. Yeah, then it should be fine.

Comment thread cot/Cargo.toml
tower-sessions = { workspace = true, features = ["memory-store"] }
tracing.workspace = true
url = { workspace = true, features = ["serde"] }
ipnet.workspace = true

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's generally discouraged to add more dependencies to the main crates: https://github.com/cot-rs/cot/blob/master/CONTRIBUTING.md#dependencies

However, I feel like it's okay to keep this one, as its functionality is not trivial. If you feel like it, though, feel free to reimplement its functionality in this PR, or another one!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one I used because it was previously suggested to me as a replacement for doing IP subnet calculations myself.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see; sorry for confusion then.

- I formatted the code
- I removed an unused import
- I changed the enum to have a Custom field instead of several
non-standard

@m4tx m4tx left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, but seems like the clippy and tests are failing.

For clippy, seems like you'll need to use std::future::ready in your remote address extractor (or bypass the clippy lint as we do in other extractors now), and add docs to the fields inside ClientIpConfig so that the builder can pick them up.

For the tests, seems like you need to update Cargo.lock.

@ElijahAhianyo do you want to have another look?

@Wervice

Wervice commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor Author

Yeah I will try to fix the failing checks, I didn't know I may update the Cargo.lock .

@Wervice

Wervice commented Oct 1, 2026

Copy link
Copy Markdown
Contributor Author

So, I've tried to get clippy to not complain about missing documentation, but where I derive the Builder macro it notes that I did not document a function. I've tried to figure out why it complains, but I just don't know what my code does differently then other code where this doesn't lead to a warning.

@m4tx

m4tx commented Oct 1, 2026

Copy link
Copy Markdown
Member

I think the missing part is that the fields in ClientIpConfig are not documented. The Builder derive macro just copy&pastes the rustdoc from the fields into the functions it generates.

Also while we're at this: we keep the fields pub in other structs, so I think we should do the same in ClientIpConfig, too.

@Wervice

Wervice commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor Author

Thank you for the quick response, that was it! I also made the fields public.

@github-actions github-actions Bot added the A-deps Area: Dependencies label Oct 1, 2026
@Wervice
Wervice requested a review from ElijahAhianyo October 1, 2026 21:52
@Wervice

Wervice commented Oct 2, 2026

Copy link
Copy Markdown
Contributor Author

Huh, I looked at the failed check for text coverage, but I am not sure if the test failed naturally, or because of a bug in the test? It's errors from the cc compiler.

@m4tx

m4tx commented Oct 2, 2026

Copy link
Copy Markdown
Member

Ah, don't worry about this; it seems like the GH runner runs out of disk space. I'm working on a fix; we can merge without this check passing.

@ElijahAhianyo

Copy link
Copy Markdown
Contributor

Looks good to me, but seems like the clippy and tests are failing.

For clippy, seems like you'll need to use std::future::ready in your remote address extractor (or bypass the clippy lint as we do in other extractors now), and add docs to the fields inside ClientIpConfig so that the builder can pick them up.

For the tests, seems like you need to update Cargo.lock.

@ElijahAhianyo do you want to have another look?

Ah yeah, I'm taking a look at this now

Comment thread cot-core/src/remote_addr.rs Outdated

fn ip_or_socket_string_to_ip<'a, T>(s: T) -> Option<IpAddr>
where
T: Into<&'a str> + Clone,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: we always pass in a &str to this function, is there any need for the generics?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I'll remove them.

Comment thread cot-core/src/remote_addr.rs Outdated
match v
.to_lowercase()
.split(';')
.find(|seg| seg.starts_with("for="))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will this work for "by=10.0.0.1, for=203.0.113.9"?

@Wervice Wervice Oct 2, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I missed that multiple proxies may append their own header at the end. I have fixed this in the next commit.

Edit: But I am unsure if by=10.0.0.1, for=203.0.113.9 is possible/should be accepted. I understand it as follows:
The first proxy in the chain is 10.0.0.1. The second doesn't name its IP, but says that it should be sent to 203.0.113.9. Of course the code can just take this value as a target, but then it may not always use the left-most proxy, is that ok?

Comment thread cot/src/request/extractors.rs Outdated
Comment on lines +363 to +365
let config = head.project_config().clone().client_ip;
let trusted_proxies = config.get_proxies();
let trusted_headers = config.get_trusted_headers();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to clone the project config on every request?
It doesnt also look like (from the usage below) that trusted_proxies and trusted_headers need to be cloned. Perhaps we can take the references and also get rid of the getters since this is the only place we use them

Suggested change
let config = head.project_config().clone().client_ip;
let trusted_proxies = config.get_proxies();
let trusted_headers = config.get_trusted_headers();
let config = &head.project_config().client_ip;
let trusted_proxies = &config.proxies;
let trusted_headers = &config.headers;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you're right, references are fine here.

#[cot::test]
async fn forwarded_valid_v6() {
assert_eq!(
extract_forwarded(&HeaderValue::from_str("For=\"[2001:db8:cafe::17]:4711\"").unwrap())

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should also have test cases for addresses without ports:

For=\"[2001:db8:cafe::17]\"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the code failed, as it didn't expect brackets. I fixed that.


let mut proxy: Option<IpAddr> = None;

if trusted_proxies.iter().any(|proxy| proxy.contains(&closest)) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

considering this logic, I don't see a wildcard case where we allow all proxies. This is not blocking, but I think it's very important to have

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, the code doesn't particularly disallow unknown proxies, it just doesn't consider them as proxies.
Also, the user can configure ::/0 and 0.0.0.0/0 if they really want to allow any proxy.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, the code doesn't particularly disallow unknown proxies, it just doesn't consider them as proxies.

Actually, the code does fall back to the socket IP provided by the connection (we get this from axum). That address is the closest hop, i.e., the last proxy that connected to us(the server), and is not necessarily the client IP. Our current approach only tries to retrieve the actual client IP from the headers if we can verify that the connecting peer is a whitelisted proxy, so this is deny-by-default behavior.

Also, the user can configure ::/0 and 0.0.0.0/0 if they really want to allow any proxy.

Yeah, I agree, a wildcard syntax would only be syntactic sugar for specifying both ::/0 and 0.0.0.0/0. It's a nice-to-have, not a blocker for this PR.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to clarify - the code will return:

  1. (proxy, client IP) when a trusted proxy has been involved
  2. (client IP, None) when an untrusted proxy is connecting
  3. (client IP, None) when no proxies are configured and a client is connecting directly

Is this correct? If so, shouldn't we differentiate somehow between cases 2 and 3 to ensure the correct behavior for users?

Comment thread cot/src/config.rs Outdated
/// hops, closest-to-origin first.
XForwardedFor,
/// Option for any header which simply contains a single IP address.
Custom(String),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could provide first-class support for some common headers used in the industry like CF-Connecting-IP, True-Client-IP (used by cloudflare), X-real-IP. What do you think about this? @m4tx

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, @Wervice, is it possible to use Custom(http::HeaderName) here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

}
}

#[derive(Clone, Copy, Debug)]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition to the extractor and for the sake of ergonomics, I wonder if we can also include a client_ip method in cot::request::RequestExt so users can do something like this:

use cot::request::{Request, RequestExt};
use cot::response::Response;

async fn index(request: Request) -> cot::Result<Response> {
    let ip = request.client_ip();
}

@Wervice Wervice Oct 3, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but it'll have to be async, because FromRequestHead is async.

Edit: I'll try to make it sync.

Comment thread cot/src/request/extractors.rs Outdated
/// todo!()
/// }
/// ```
pub struct RemoteAddr {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we rename this to ClientIpAddr since that's more fitting than RemoteAddr?

cc: @m4tx

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since I'm using ClientIp+.... everywhere else, I renamed it.

- Using `HeaderName` instead of `String` for config
  - Replaced std::str::FromStr with FromStr for `CacheType`, to remove
new warning
- Renamed RemoteAddr to ClientIpAddr
- Added implementation in `RequestExt`
- Updated `Forwarded` header parsing
@Wervice
Wervice requested a review from ElijahAhianyo October 3, 2026 09:55
@codecov

codecov Bot commented Oct 3, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 90.98196% with 45 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
cot-core/src/remote_addr.rs 89.69% 17 Missing ⚠️
cot/src/config.rs 82.08% 11 Missing and 1 partial ⚠️
cot/src/request.rs 0.00% 10 Missing ⚠️
cot/src/request/extractors.rs 98.00% 3 Missing and 2 partials ⚠️
cot/src/project.rs 85.71% 0 Missing and 1 partial ⚠️
Flag Coverage Δ
rust 90.24% <90.98%> (+0.06%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
cot/src/project.rs 87.71% <85.71%> (+0.04%) ⬆️
cot/src/request/extractors.rs 95.93% <98.00%> (+5.50%) ⬆️
cot/src/request.rs 92.12% <0.00%> (-3.02%) ⬇️
cot/src/config.rs 94.29% <82.08%> (-0.29%) ⬇️
cot-core/src/remote_addr.rs 89.69% <89.69%> (ø)

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@m4tx m4tx left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, thanks for the patience waiting for the review - I've added two comments to the latest revision, please take a look.

Additionally, there's a merge conflict with config.rs, most likely because we've recently added support for automatically generating config TOML reference in the guide (#621). This change should support this, too - let us know if you run into any problems with that!

Comment thread cot/src/config.rs
/// - `Forwarded`
/// - `X-Forwarded-For`
/// - `CF-Connecting-IP`
/// - `X-Real-IP`

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we clarify that these are supported by the Custom variant?


let mut proxy: Option<IpAddr> = None;

if trusted_proxies.iter().any(|proxy| proxy.contains(&closest)) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to clarify - the code will return:

  1. (proxy, client IP) when a trusted proxy has been involved
  2. (client IP, None) when an untrusted proxy is connecting
  3. (client IP, None) when no proxies are configured and a client is connecting directly

Is this correct? If so, shouldn't we differentiate somehow between cases 2 and 3 to ensure the correct behavior for users?

@Wervice

Wervice commented Oct 15, 2026

Copy link
Copy Markdown
Contributor Author

Hey, I will try to answer your questions as soon as possible, but I currently don't have the capacity, I'm sorry.

@m4tx

m4tx commented Nov 1, 2026

Copy link
Copy Markdown
Member

Hey @Wervice, I took the liberty of cleaning up your PR a bit and fixing some issues I've found, as I'd really like to see this change in next release of Cot (very soon) - I'm hoping that's okay with you. I kept your work as the base commit for it, so your part will obviously be properly attributes in the history. It's in #724, feel free to have a look and review.

Thanks a lot for your contribution, this feature has been long overdue!

@Wervice

Wervice commented Nov 2, 2026

Copy link
Copy Markdown
Contributor Author

Hey @m4tx, I am glad I could contribute to the project and I'm sorry that I couldn't finish the code myself.
I have no problem with you editing the commit and I'm looking forward to using the feature as soon as I can get back to coding.

@m4tx

m4tx commented Nov 6, 2026

Copy link
Copy Markdown
Member

Closing as superseded by #724.

Thanks a lot again for the contribution!

@m4tx m4tx closed this Nov 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-deps Area: Dependencies C-core C-lib Crate: cot (main library crate)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants