imbolc / axum-client-ip

A client IP address extractor for Axum
MIT License
41 stars 13 forks source link

SecureClientIp causes 500 when x-forwarded-for is missing #11

Closed 1oglop1 closed 1 year ago

1oglop1 commented 1 year ago

I configured the server to expect x-forwarded-for SecureClientIpSource::RightmostXForwardedFor and the handler with secure_ip: SecureClientIp.

However, when the client does not send the header, the server returns 500. I'd expect 400 or another 4XX error.

Am I missing anything? Is this an expected behaviour?

imbolc commented 1 year ago

Secure extractors are meant to be used when:

1oglop1 commented 1 year ago

I see, how would I go about this in development/exploration? If I just want to run the dev server and click in the browser to test something?

PS. I'm struggling to find how to build the String or &str from In/SecureIp

error[E0599]: `InsecureClientIp` doesn't implement `std::fmt::Display`
  --> crates/...
   |
62 |     let ip = ip.to_string();
   |                 ^^^^^^^^^ `InsecureClientIp` cannot be formatted with the default formatter
   |
  ::: /.../axum-client-ip-0.4.1/src/insecure.rs:32:1
   |
32 | pub struct InsecureClientIp(pub IpAddr);
   | ---------------------------
   | |
   | doesn't satisfy `InsecureClientIp: ToString`
   | doesn't satisfy `InsecureClientIp: std::fmt::Display`
   |
   = note: the following trait bounds were not satisfied:
           `InsecureClientIp: std::fmt::Display`
           which is required by `InsecureClientIp: ToString`
imbolc commented 1 year ago

Here's an example using env variables to keep the IP source. You can just put dotenv::dotenv() on top of the main, and then using .env file. So locally it's content would be IP_SOURCE=ConnectInfo and on the server IP_SOURCE=RightmostXForwardedFor.

PS. I'm struggling to find how to build the String or &str from In/SecureIp

ip.0.to_string(), but with Axum you usually extract the inner ip in the function signature: https://github.com/imbolc/axum-client-ip/blob/main/examples/secure.rs#L12

imbolc commented 1 year ago

pub struct InsecureClientIp(pub IpAddr) is a tuple containing IpAddr as a single member, so you can use .0 or tuple destruction to extract it.

1oglop1 commented 1 year ago

IP_SOURCE=ConnectInfo and on the server IP_SOURCE=RightmostXForwardedFor

Thanks a lot! I still cannot get used to this SecureClientIp(ip): SecureClientIp which unpacked the tuple for me.

Alright, I did read the example, however, the codebase I'm trying to contribute to doesn't seem to be compatible with envy because we are using clap, so I'll need to wire it somehow together. But that's the problem for another day.

imbolc commented 1 year ago

Clap can parse env vars, just enable env feature:

#[arg(env)]
ip_source: SecureClientIpSource
1oglop1 commented 1 year ago

Clap can parse env vars, just enable env feature:

#[arg(env)]
ip_source: SecureClientIpSource

I really appreciate your help, I've been writing Rust for 2 weeks now.

If I remember correctly the problem for me was setting the default value. Or converting the string RightmostXForwardedFor to an enum member. There isn't anything built-in clap I could find to simply point the enum SecureClientIpSource and pass the value via CLI.

My rust know very limited and anything that takes more than 10 minutes to figure out is out of the scope of my PoC. But I will take a look later again.

This is my Rust tutor atm: https://www.phind.com/search?cache=4fd32b39-c6b1-4d4f-8eb8-196148fffbab

imbolc commented 1 year ago

Yep, I think FromStr is a good enough solution. Clap supports default values: https://docs.rs/clap/latest/clap/_derive/_tutorial/index.html#defaults

1oglop1 commented 1 year ago

Yep, I think FromStr is a good enough solution. Clap supports default values: https://docs.rs/clap/latest/clap/_derive/_tutorial/index.html#defaults

I went back and checked what problem I had with SecureClientIpSource and found out that it seems a limitation of the language itself. Because SecureClientIpSource does not implement FromString and I cannot simply implement it for it myself without a wrapper.

only traits defined in the current crate can be implemented for types defined outside of the crate
define and implement a trait or new type instead

So it looks like that any change required must be done here axum-client-ip eg. with the use of https://docs.rs/strum/latest/strum/ and its derive macros.

EnumString seems to be the thing. https://docs.rs/strum_macros/0.24.3/strum_macros/derive.EnumString.html

imbolc commented 1 year ago

Yep, you often encounter this issue with Rust, the natural solution is the newtype pattern.

Though it seems to be a common issue, so I'd accept a PR with a FromStr implementation. No additional dependencies though, it's already implements Deserialize, you an use it in the implementation.