From 2b2de298001c7718952a7e8647f6b1a2736c7259 Mon Sep 17 00:00:00 2001 From: Rob Ede Date: Tue, 28 Dec 2021 14:52:43 +0000 Subject: [PATCH] never return port in `realip_remote_addr` (#2554) --- CHANGES.md | 6 ++ src/config.rs | 4 +- src/info.rs | 125 ++++++++++++++++++++++++--------------- src/middleware/logger.rs | 2 +- src/request.rs | 19 +++--- 5 files changed, 97 insertions(+), 59 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index f925f3b94..b6d3b103d 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -11,7 +11,13 @@ - Some guards now return `impl Guard` and their concrete types are made private: `guard::{Header}` and all the method guards. [#2552] - The `Not` guard is now generic over the type of guard it wraps. [#2552] +### Fixed +- Rename `ConnectionInfo::{remote_addr => peer_addr}`, deprecating the old name. [#2554] +- `ConnectionInfo::peer_addr` will not return the port number. [#2554] +- `ConnectionInfo::realip_remote_addr` will not return the port number if sourcing the IP from the peer's socket address. [#2554] + [#2552]: https://github.com/actix/actix-web/pull/2552 +[#2554]: https://github.com/actix/actix-web/pull/2554 ## 4.0.0-beta.16 - 2021-12-27 diff --git a/src/config.rs b/src/config.rs index cfa9a4ca3..d68374387 100644 --- a/src/config.rs +++ b/src/config.rs @@ -128,7 +128,7 @@ impl AppConfig { /// Server host name. /// - /// Host name is used by application router as a hostname for url generation. + /// Host name is used by application router as a hostname for URL generation. /// Check [ConnectionInfo](super::dev::ConnectionInfo::host()) /// documentation for more information. /// @@ -137,7 +137,7 @@ impl AppConfig { &self.host } - /// Returns true if connection is secure(https) + /// Returns true if connection is secure (i.e., running over `https:`). pub fn secure(&self) -> bool { self.secure } diff --git a/src/info.rs b/src/info.rs index 71194b24d..ce1ef97c6 100644 --- a/src/info.rs +++ b/src/info.rs @@ -67,7 +67,7 @@ fn first_header_value<'a>(req: &'a RequestHead, name: &'_ HeaderName) -> Option< pub struct ConnectionInfo { host: String, scheme: String, - remote_addr: Option, + peer_addr: Option, realip_remote_addr: Option, } @@ -134,67 +134,70 @@ impl ConnectionInfo { .or_else(|| first_header_value(req, &*X_FORWARDED_FOR)) .map(str::to_owned); - let remote_addr = req.peer_addr.map(|addr| addr.to_string()); + let peer_addr = req.peer_addr.map(|addr| addr.ip().to_string()); ConnectionInfo { host, scheme, - remote_addr, + peer_addr, realip_remote_addr, } } + /// Real IP (remote address) of client that initiated request. + /// + /// The address is resolved through the following, in order: + /// - `Forwarded` header + /// - `X-Forwarded-For` header + /// - peer address of opened socket (same as [`remote_addr`](Self::remote_addr)) + /// + /// # Security + /// Do not use this function for security purposes unless you can be sure that the `Forwarded` + /// and `X-Forwarded-For` headers cannot be spoofed by the client. If you are running without a + /// proxy then [obtaining the peer address](Self::peer_addr) would be more appropriate. + #[inline] + pub fn realip_remote_addr(&self) -> Option<&str> { + self.realip_remote_addr + .as_deref() + .or_else(|| self.peer_addr.as_deref()) + } + + /// Returns serialized IP address of the peer connection. + /// + /// See [`HttpRequest::peer_addr`] for more details. + #[inline] + pub fn peer_addr(&self) -> Option<&str> { + self.peer_addr.as_deref() + } + + /// Hostname of the request. + /// + /// Hostname is resolved through the following, in order: + /// - `Forwarded` header + /// - `X-Forwarded-Host` header + /// - `Host` header + /// - request target / URI + /// - configured server hostname + #[inline] + pub fn host(&self) -> &str { + &self.host + } + /// Scheme of the request. /// - /// Scheme is resolved through the following headers, in this order: - /// - /// - Forwarded - /// - X-Forwarded-Proto - /// - Uri + /// Scheme is resolved through the following, in order: + /// - `Forwarded` header + /// - `X-Forwarded-Proto` header + /// - request target / URI #[inline] pub fn scheme(&self) -> &str { &self.scheme } - /// Hostname of the request. - /// - /// Hostname is resolved through the following headers, in this order: - /// - /// - Forwarded - /// - X-Forwarded-Host - /// - Host - /// - Uri - /// - Server hostname - pub fn host(&self) -> &str { - &self.host - } - - /// Remote address of the connection. - /// - /// Get remote_addr address from socket address. + #[doc(hidden)] + #[deprecated(since = "4.0.0", note = "Renamed to `peer_addr`.")] pub fn remote_addr(&self) -> Option<&str> { - self.remote_addr.as_deref() - } - - /// Real IP (remote address) of client that initiated request. - /// - /// The address is resolved through the following headers, in this order: - /// - /// - Forwarded - /// - X-Forwarded-For - /// - remote_addr name of opened socket - /// - /// # Security - /// Do not use this function for security purposes, unless you can ensure the Forwarded and - /// X-Forwarded-For headers cannot be spoofed by the client. If you want the client's socket - /// address explicitly, use [`HttpRequest::peer_addr()`][peer_addr] instead. - /// - /// [peer_addr]: crate::web::HttpRequest::peer_addr() - #[inline] - pub fn realip_remote_addr(&self) -> Option<&str> { - self.realip_remote_addr - .as_deref() - .or_else(|| self.remote_addr.as_deref()) + self.peer_addr() } } @@ -209,7 +212,7 @@ impl FromRequest for ConnectionInfo { /// Extractor for peer's socket address. /// -/// Also see [`HttpRequest::peer_addr`]. +/// Also see [`HttpRequest::peer_addr`] and [`ConnectionInfo::peer_addr`]. /// /// # Examples /// ``` @@ -432,13 +435,37 @@ mod tests { #[actix_rt::test] async fn peer_addr_extract() { + let req = TestRequest::default().to_http_request(); + let res = PeerAddr::extract(&req).await; + assert!(res.is_err()); + let addr = "127.0.0.1:8080".parse().unwrap(); let req = TestRequest::default().peer_addr(addr).to_http_request(); let peer_addr = PeerAddr::extract(&req).await.unwrap(); assert_eq!(peer_addr, PeerAddr(addr)); + } + #[actix_rt::test] + async fn remote_address() { let req = TestRequest::default().to_http_request(); - let res = PeerAddr::extract(&req).await; - assert!(res.is_err()); + let res = ConnectionInfo::extract(&req).await.unwrap(); + assert!(res.peer_addr().is_none()); + + let addr = "127.0.0.1:8080".parse().unwrap(); + let req = TestRequest::default().peer_addr(addr).to_http_request(); + let conn_info = ConnectionInfo::extract(&req).await.unwrap(); + assert_eq!(conn_info.peer_addr().unwrap(), "127.0.0.1"); + } + + #[actix_rt::test] + async fn real_ip_from_socket_addr() { + let req = TestRequest::default().to_http_request(); + let res = ConnectionInfo::extract(&req).await.unwrap(); + assert!(res.realip_remote_addr().is_none()); + + let addr = "127.0.0.1:8080".parse().unwrap(); + let req = TestRequest::default().peer_addr(addr).to_http_request(); + let conn_info = ConnectionInfo::extract(&req).await.unwrap(); + assert_eq!(conn_info.realip_remote_addr().unwrap(), "127.0.0.1"); } } diff --git a/src/middleware/logger.rs b/src/middleware/logger.rs index d7fdb234f..969cb0c10 100644 --- a/src/middleware/logger.rs +++ b/src/middleware/logger.rs @@ -547,7 +547,7 @@ impl FormatText { *self = FormatText::Str(s.to_string()); } FormatText::RemoteAddr => { - let s = if let Some(peer) = req.connection_info().remote_addr() { + let s = if let Some(peer) = req.connection_info().peer_addr() { FormatText::Str((*peer).to_string()) } else { FormatText::Str("-".to_string()) diff --git a/src/request.rs b/src/request.rs index 07fb4eb2d..b59369317 100644 --- a/src/request.rs +++ b/src/request.rs @@ -228,23 +228,28 @@ impl HttpRequest { self.app_state().rmap() } - /// Peer socket address. + /// Returns peer socket address. /// /// Peer address is the directly connected peer's socket address. If a proxy is used in front of /// the Actix Web server, then it would be address of this proxy. /// - /// To get client connection information `.connection_info()` should be used. + /// For expanded client connection information, use [`connection_info`] instead. /// - /// Will only return None when called in unit tests. + /// Will only return None when called in unit tests unless [`TestRequest::peer_addr`] is used. + /// + /// [`TestRequest::peer_addr`]: crate::test::TestRequest::peer_addr + /// [`connection_info`]: Self::connection_info #[inline] pub fn peer_addr(&self) -> Option { self.head().peer_addr } - /// Get *ConnectionInfo* for the current request. + /// Returns connection info for the current request. /// - /// This method panics if request's extensions container is already - /// borrowed. + /// The return type, [`ConnectionInfo`], can also be used as an extractor. + /// + /// # Panics + /// Panics if request's extensions container is already borrowed. #[inline] pub fn connection_info(&self) -> Ref<'_, ConnectionInfo> { if !self.extensions().contains::() { @@ -252,7 +257,7 @@ impl HttpRequest { self.extensions_mut().insert(info); } - Ref::map(self.extensions(), |e| e.get().unwrap()) + Ref::map(self.extensions(), |data| data.get().unwrap()) } /// App config