diff --git a/crates/cli/src/app_state.rs b/crates/cli/src/app_state.rs index 30b004ea2..25a295d0c 100644 --- a/crates/cli/src/app_state.rs +++ b/crates/cli/src/app_state.rs @@ -7,13 +7,13 @@ use std::{convert::Infallible, net::IpAddr, sync::Arc}; -use axum::extract::{FromRef, FromRequestParts}; +use axum::extract::{FromRef, FromRequestParts, State}; use ipnetwork::IpNetwork; use mas_context::LogContext; use mas_data_model::{AppVersion, BoxClock, BoxRng, SiteConfig, SystemClock}; use mas_handlers::{ - ActivityTracker, BoundActivityTracker, CookieManager, ErrorWrapper, GraphQLSchema, Limiter, - MetadataCache, RequesterFingerprint, passwords::PasswordManager, + ActivityTracker, ClientIp, CookieManager, ErrorWrapper, GraphQLSchema, Limiter, MetadataCache, + passwords::PasswordManager, }; use mas_i18n::Translator; use mas_keystore::{Encrypter, Keystore}; @@ -273,10 +273,11 @@ impl FromRequestParts for ActivityTracker { } fn infer_client_ip( - parts: &axum::http::request::Parts, + extensions: &axum::http::Extensions, + headers: &axum::http::HeaderMap, trusted_proxies: &[IpNetwork], ) -> Option { - let connection_info = parts.extensions.get::(); + let connection_info = extensions.get::(); let peer = if let Some(info) = connection_info { // We can always trust the proxy protocol to give us the correct IP address @@ -292,8 +293,7 @@ fn infer_client_ip( }; // Get the list of IPs from the X-Forwarded-For header - let peers_from_header = parts - .headers + let peers_from_header = headers .get("x-forwarded-for") .and_then(|value| value.to_str().ok()) .map(|value| value.split(',').filter_map(|v| v.parse().ok())) @@ -325,41 +325,23 @@ fn infer_client_ip( client_ip.or(fallback) } -impl FromRequestParts for BoundActivityTracker { - type Rejection = Infallible; - - async fn from_request_parts( - parts: &mut axum::http::request::Parts, - state: &AppState, - ) -> Result { - // TODO: we may infer the IP twice, for the activity tracker and the limiter - let ip = infer_client_ip(parts, &state.trusted_proxies); - tracing::debug!(ip = ?ip, "Inferred client IP address"); - Ok(state.activity_tracker.clone().bind(ip)) - } -} - -impl FromRequestParts for RequesterFingerprint { - type Rejection = Infallible; - - async fn from_request_parts( - parts: &mut axum::http::request::Parts, - state: &AppState, - ) -> Result { - // TODO: we may infer the IP twice, for the activity tracker and the limiter - let ip = infer_client_ip(parts, &state.trusted_proxies); - - if let Some(ip) = ip { - Ok(RequesterFingerprint::new(ip)) - } else { - // If we can't infer the IP address, we'll just use an empty fingerprint and - // warn about it - tracing::warn!( - "Could not infer client IP address for an operation which rate-limits based on IP addresses" - ); - Ok(RequesterFingerprint::EMPTY) - } - } +/// Middleware which infers the client IP address once, ahead of the rest of the +/// request handling, and stores it in the request extensions as a [`ClientIp`]. +/// +/// This lets the logging middleware and the `BoundActivityTracker` / +/// `RequesterFingerprint` extractors read the same IP without re-inferring it. +pub(crate) async fn client_ip_middleware( + State(state): State, + mut request: axum::extract::Request, + next: axum::middleware::Next, +) -> axum::response::Response { + let ip = infer_client_ip( + request.extensions(), + request.headers(), + &state.trusted_proxies, + ); + request.extensions_mut().insert(ClientIp(ip)); + next.run(request).await } impl FromRequestParts for BoxRepository { diff --git a/crates/cli/src/server.rs b/crates/cli/src/server.rs index 15114a5d9..b4f15bda9 100644 --- a/crates/cli/src/server.rs +++ b/crates/cli/src/server.rs @@ -21,6 +21,7 @@ use hyper::{Method, Request, Response, StatusCode, Version, header::USER_AGENT}; use listenfd::ListenFd; use mas_config::{HttpBindConfig, HttpResource, HttpTlsConfig, UnixOrTcp}; use mas_context::LogContext; +use mas_handlers::ClientIp; use mas_listener::{ConnectionInfo, unix_or_tcp::UnixOrTcpListener}; use mas_router::Route; use mas_templates::Templates; @@ -31,8 +32,9 @@ use mas_tower::{ use opentelemetry::{Key, KeyValue}; use opentelemetry_http::HeaderExtractor; use opentelemetry_semantic_conventions::trace::{ - HTTP_REQUEST_METHOD, HTTP_RESPONSE_STATUS_CODE, HTTP_ROUTE, NETWORK_PROTOCOL_NAME, - NETWORK_PROTOCOL_VERSION, URL_PATH, URL_QUERY, URL_SCHEME, USER_AGENT_ORIGINAL, + CLIENT_ADDRESS, HTTP_REQUEST_METHOD, HTTP_RESPONSE_STATUS_CODE, HTTP_ROUTE, + NETWORK_PROTOCOL_NAME, NETWORK_PROTOCOL_VERSION, URL_PATH, URL_QUERY, URL_SCHEME, + USER_AGENT_ORIGINAL, }; use rustls::ServerConfig; use sentry_tower::{NewSentryLayer, SentryHttpLayer}; @@ -41,7 +43,7 @@ use tower_http::services::{ServeDir, fs::ServeFileSystemResponseBody}; use tracing::Span; use tracing_opentelemetry::OpenTelemetrySpanExt; -use crate::app_state::AppState; +use crate::app_state::{AppState, client_ip_middleware}; const MAS_LISTENER_NAME: Key = Key::from_static_str("mas.listener.name"); @@ -98,6 +100,14 @@ fn otel_url_scheme(request: &Request) -> &'static str { fn make_http_span(req: &Request) -> Span { let method = otel_http_method(req); let route = otel_http_route(req); + // The client IP was inferred by `client_ip_middleware`, which wraps this + // layer, so the `ClientIp` extension is already set by the time the span is + // created. + let client_ip = req + .extensions() + .get::() + .and_then(|ip| ip.0) + .map(tracing::field::display); let span_name = if let Some(route) = route.as_ref() { format!("{method} {route}") @@ -119,6 +129,7 @@ fn make_http_span(req: &Request) -> Span { { URL_QUERY } = tracing::field::Empty, { URL_SCHEME } = otel_url_scheme(req), { USER_AGENT_ORIGINAL } = tracing::field::Empty, + { CLIENT_ADDRESS } = client_ip, ); if let Some(route) = route.as_ref() { @@ -188,6 +199,11 @@ async fn log_response_middleware( let method = otel_http_method(&request); let path = request.uri().path().to_owned(); let version = otel_net_protocol_version(&request); + let client_ip = request + .extensions() + .get::() + .and_then(|ip| ip.0) + .map(tracing::field::display); let response = next.run(request).await; @@ -200,14 +216,17 @@ async fn log_response_middleware( match status_code.as_u16() { 100..=399 => tracing::info!( name: "http.server.response", + { client.address = client_ip }, "\"{method} {path} HTTP/{version}\" {status_code} {user_agent:?} [{stats}]", ), 400..=499 => tracing::warn!( name: "http.server.response", + { client.address = client_ip }, "\"{method} {path} HTTP/{version}\" {status_code} {user_agent:?} [{stats}]", ), 500..=599 => tracing::error!( name: "http.server.response", + { client.address = client_ip }, "\"{method} {path} HTTP/{version}\" {status_code} {user_agent:?} [{stats}]", ), _ => { /* This shouldn't happen */ } @@ -335,6 +354,16 @@ pub fn build_router( span.record("otel.status_code", "OK"); }), ) + // Infer the client IP once, ahead of the rest of the request handling, + // and store it in the request extensions. Placed *after* the `TraceLayer` + // in this list so it wraps it (the first layer is the innermost): the + // `ClientIp` extension is therefore set before `make_http_span` reads it + // to record `client.address` on the span, and before the logging + // middleware and the extractors further in read it too. + .layer(axum::middleware::from_fn_with_state( + state.clone(), + client_ip_middleware, + )) .layer(mas_context::LogContextLayer::new(|req| { otel_http_method(req).into() })) diff --git a/crates/handlers/src/activity_tracker/bound.rs b/crates/handlers/src/activity_tracker/bound.rs index 8f7acbdde..329ce2445 100644 --- a/crates/handlers/src/activity_tracker/bound.rs +++ b/crates/handlers/src/activity_tracker/bound.rs @@ -1,16 +1,18 @@ +// Copyright 2026 Element Creations Ltd. // Copyright 2024, 2025 New Vector Ltd. // Copyright 2023, 2024 The Matrix.org Foundation C.I.C. // // SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-Element-Commercial // Please see LICENSE files in the repository root for full details. -use std::net::IpAddr; +use std::{convert::Infallible, net::IpAddr}; +use axum::extract::FromRequestParts; use mas_data_model::{ BrowserSession, Clock, CompatSession, Session, personal::session::PersonalSession, }; -use crate::activity_tracker::ActivityTracker; +use crate::{ClientIp, activity_tracker::ActivityTracker}; /// An activity tracker with an IP address bound to it. #[derive(Clone)] @@ -60,3 +62,27 @@ impl Bound { .await; } } + +/// Extracts a [`Bound`] activity tracker for any state that can provide an +/// [`ActivityTracker`]. The client IP is read from the [`ClientIp`] request +/// extension (set by the IP-detection middleware), defaulting to `None` when it +/// is absent. +impl FromRequestParts for Bound +where + S: Send + Sync, + ActivityTracker: FromRequestParts, +{ + type Rejection = Infallible; + + async fn from_request_parts( + parts: &mut axum::http::request::Parts, + state: &S, + ) -> Result { + let ip = parts.extensions.get::().and_then(|ip| ip.0); + let tracker = match ActivityTracker::from_request_parts(parts, state).await { + Ok(tracker) => tracker, + Err(infallible) => match infallible {}, + }; + Ok(tracker.bind(ip)) + } +} diff --git a/crates/handlers/src/client_ip.rs b/crates/handlers/src/client_ip.rs new file mode 100644 index 000000000..be38ee4cd --- /dev/null +++ b/crates/handlers/src/client_ip.rs @@ -0,0 +1,19 @@ +// Copyright 2026 Element Creations Ltd. +// +// SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-Element-Commercial +// Please see LICENSE files in the repository root for full details. + +use std::net::IpAddr; + +/// The client IP address of the request. +/// +/// This is set in the request extensions by the IP-detection middleware, ahead +/// of the rest of the request handling, so that the logging middleware and the +/// [`BoundActivityTracker`]/[`RequesterFingerprint`] extractors all observe the +/// same value without re-inferring it. It is `None` when the client IP could +/// not be determined. +/// +/// [`BoundActivityTracker`]: crate::BoundActivityTracker +/// [`RequesterFingerprint`]: crate::RequesterFingerprint +#[derive(Clone, Copy, Debug)] +pub struct ClientIp(pub Option); diff --git a/crates/handlers/src/compat/login.rs b/crates/handlers/src/compat/login.rs index 76b4074a5..ee7528c33 100644 --- a/crates/handlers/src/compat/login.rs +++ b/crates/handlers/src/compat/login.rs @@ -1202,6 +1202,53 @@ mod tests { user } + /// Test that the client IP injected on the request (as the IP-detection + /// middleware would in production) is recorded against the session by the + /// activity tracker. + #[sqlx::test(migrator = "mas_storage_pg::MIGRATOR")] + async fn test_login_records_client_ip(pool: PgPool) { + setup(); + let state = TestState::from_pool(pool).await.unwrap(); + + let user = user_with_password(&state, "alice", "password", false).await; + + let client_ip = std::net::IpAddr::from([1, 2, 3, 4]); + let request = Request::post("/_matrix/client/v3/login") + .client_ip(client_ip) + .json(serde_json::json!({ + "type": "m.login.password", + "identifier": { + "type": "m.id.user", + "user": "alice", + }, + "password": "password", + })); + + let response = state.request(request).await; + response.assert_status(StatusCode::OK); + + // Flush the activity tracker so the recorded IP is persisted + state.activity_tracker.flush().await; + + let mut repo = state.repository().await.unwrap(); + let compat_session_page = repo + .compat_session() + .list( + CompatSessionFilter::new().for_user(&user).active_only(), + Pagination::first(1), + ) + .await + .unwrap(); + let (compat_session, _) = compat_session_page + .edges + .into_iter() + .next() + .expect("a compat session should have been created") + .node; + + assert_eq!(compat_session.last_active_ip, Some(client_ip)); + } + /// Test that a user can login with a password using the Matrix /// compatibility API. #[sqlx::test(migrator = "mas_storage_pg::MIGRATOR")] diff --git a/crates/handlers/src/lib.rs b/crates/handlers/src/lib.rs index 5063c7697..2f075f4c0 100644 --- a/crates/handlers/src/lib.rs +++ b/crates/handlers/src/lib.rs @@ -69,6 +69,7 @@ mod activity_tracker; mod captcha; #[cfg(test)] mod cleanup_tests; +mod client_ip; mod preferred_language; mod rate_limit; mod session; @@ -106,6 +107,7 @@ use mas_data_model::{BoxClock, BoxRng}; pub use self::{ activity_tracker::{ActivityTracker, Bound as BoundActivityTracker}, admin::router as admin_api_router, + client_ip::ClientIp, graphql::{ Schema as GraphQLSchema, schema as graphql_schema, schema_builder as graphql_schema_builder, }, diff --git a/crates/handlers/src/rate_limit.rs b/crates/handlers/src/rate_limit.rs index 8f20f3e63..4c96fd974 100644 --- a/crates/handlers/src/rate_limit.rs +++ b/crates/handlers/src/rate_limit.rs @@ -5,13 +5,16 @@ // SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-Element-Commercial // Please see LICENSE files in the repository root for full details. -use std::{net::IpAddr, sync::Arc, time::Duration}; +use std::{convert::Infallible, net::IpAddr, sync::Arc, time::Duration}; +use axum::extract::FromRequestParts; use governor::{RateLimiter, clock::QuantaClock, state::keyed::DashMapStateStore}; use mas_config::RateLimitingConfig; use mas_data_model::{User, UserEmailAuthentication}; use ulid::Ulid; +use crate::ClientIp; + #[derive(Debug, Clone, thiserror::Error)] pub enum AccountRecoveryLimitedError { #[error("Too many account recovery requests for requester {0}")] @@ -76,6 +79,28 @@ impl RequesterFingerprint { } } +impl FromRequestParts for RequesterFingerprint { + type Rejection = Infallible; + + async fn from_request_parts( + parts: &mut axum::http::request::Parts, + _state: &S, + ) -> Result { + let ip = parts.extensions.get::().and_then(|ip| ip.0); + + if let Some(ip) = ip { + Ok(Self::new(ip)) + } else { + // If we can't infer the IP address, we'll just use an empty fingerprint and + // warn about it + tracing::warn!( + "Could not infer client IP address for an operation which rate-limits based on IP addresses" + ); + Ok(Self::EMPTY) + } + } +} + /// Rate limiters for the different operations #[derive(Debug, Clone)] pub struct Limiter { diff --git a/crates/handlers/src/test_utils.rs b/crates/handlers/src/test_utils.rs index 3b8894991..c6a47a68b 100644 --- a/crates/handlers/src/test_utils.rs +++ b/crates/handlers/src/test_utils.rs @@ -7,6 +7,7 @@ use std::{ convert::Infallible, + net::IpAddr, sync::{Arc, Mutex, RwLock}, task::{Context, Poll}, }; @@ -53,7 +54,7 @@ use tower::{Layer, Service, ServiceExt}; use url::Url; use crate::{ - ActivityTracker, BoundActivityTracker, Limiter, RequesterFingerprint, graphql, + ActivityTracker, ClientIp, Limiter, graphql, passwords::{Hasher, PasswordManager}, upstream_oauth2::cache::MetadataCache, }; @@ -619,29 +620,6 @@ impl FromRequestParts for ActivityTracker { } } -impl FromRequestParts for BoundActivityTracker { - type Rejection = Infallible; - - async fn from_request_parts( - _parts: &mut axum::http::request::Parts, - state: &TestState, - ) -> Result { - let ip = None; - Ok(state.activity_tracker.clone().bind(ip)) - } -} - -impl FromRequestParts for RequesterFingerprint { - type Rejection = Infallible; - - async fn from_request_parts( - _parts: &mut axum::http::request::Parts, - _state: &TestState, - ) -> Result { - Ok(RequesterFingerprint::EMPTY) - } -} - impl FromRequestParts for BoxClock { type Rejection = Infallible; @@ -704,6 +682,14 @@ pub(crate) trait RequestBuilderExt { /// credentials. fn basic_auth(self, username: &str, password: &str) -> Self; + /// Sets the client IP address that the extractors will see for this + /// request. + /// + /// This mirrors what the IP-detection middleware does in production: it + /// stores a [`ClientIp`] in the request extensions, which is then read by + /// the `BoundActivityTracker` and `RequesterFingerprint` extractors. + fn client_ip(self, ip: IpAddr) -> Self; + /// Builds the request with an empty body. fn empty(self) -> hyper::Request; } @@ -740,6 +726,10 @@ impl RequestBuilderExt for hyper::http::request::Builder { self } + fn client_ip(self, ip: IpAddr) -> Self { + self.extension(ClientIp(Some(ip))) + } + fn empty(self) -> hyper::Request { self.body(String::new()).unwrap() }