Merge pull request sozu-proxy#1063 from sozu-proxy/devel/edemolis/fix…

…/access-logs-spaces Sanitize user-agent in access logs
Shine-neko · Jan 25, 2024 · 7278c8f · 7278c8f
2 parents 5e30411 + 04d3105
commit 7278c8f
Show file tree

Hide file tree

Showing 2 changed files with 44 additions and 33 deletions.
diff --git a/lib/src/logs.rs b/lib/src/logs.rs
@@ -141,11 +141,11 @@ pub struct RequestRecord<'a> {
     pub client_rtt: Option<Duration>,
     pub server_rtt: Option<Duration>,
     pub metrics: &'a SessionMetrics,
-    pub user_agent: Option<&'a str>,
+    pub user_agent: Option<String>,
 }
 
 impl RequestRecord<'_> {
-    pub fn log(&self) {
+    pub fn log(self) {
         let context = &self.context;
         let cluster_id = context.cluster_id;
         let tags = self.tags;
@@ -154,7 +154,7 @@ impl RequestRecord<'_> {
         let session_address = self.session_address;
         let backend_address = self.backend_address;
         let endpoint = &self.endpoint;
-        let user_agent = &self.user_agent;
+        let mut user_agent = self.user_agent;
 
         let metrics = self.metrics;
         // let backend_response_time = metrics.backend_response_time();
@@ -195,10 +195,23 @@ impl RequestRecord<'_> {
             }
         }
 
+        let (tags, ua_sep, user_agent) = match (tags, &mut user_agent) {
+            (None, None) => ("-", "", ""),
+            (Some(tags), None) => (tags, "", ""),
+            (None, Some(ua)) => {
+                prepare_user_agent(ua);
+                ("", "user-agent=", ua.as_str())
+            }
+            (Some(tags), Some(ua)) => {
+                prepare_user_agent(ua);
+                (tags, ", user-agent=", ua.as_str())
+            }
+        };
+
         match self.error {
             None => {
                 info_access!(
-                    "{}{} -> {} \t{}/{}/{}/{} \t{} -> {} \t {}{} {} {}",
+                    "{}{} -> {} \t{}/{}/{}/{} \t{} -> {} \t {}{}{} {} {}",
                     context,
                     session_address.as_str_or("X"),
                     backend_address.as_str_or("X"),
@@ -208,18 +221,9 @@ impl RequestRecord<'_> {
                     LogDuration(server_rtt),
                     metrics.bin,
                     metrics.bout,
-                    match user_agent {
-                        Some(_) => tags.as_str_or(""),
-                        None => tags.as_str_or("-"),
-                    },
-                    match tags {
-                        Some(tags) if !tags.is_empty() => user_agent
-                            .map(|ua| format!(", user-agent={ua}"))
-                            .unwrap_or_default(),
-                        Some(_) | None => user_agent
-                            .map(|ua| format!("user-agent={ua}"))
-                            .unwrap_or_default(),
-                    },
+                    tags,
+                    ua_sep,
+                    user_agent,
                     protocol,
                     endpoint
                 );
@@ -230,7 +234,7 @@ impl RequestRecord<'_> {
                 );
             }
             Some(message) => error_access!(
-                "{}{} -> {} \t{}/{}/{}/{} \t{} -> {} \t {}{} {} {} | {}",
+                "{}{} -> {} \t{}/{}/{}/{} \t{} -> {} \t {}{}{} {} {} | {}",
                 context,
                 session_address.as_str_or("X"),
                 backend_address.as_str_or("X"),
@@ -240,22 +244,28 @@ impl RequestRecord<'_> {
                 LogDuration(server_rtt),
                 metrics.bin,
                 metrics.bout,
-                match user_agent {
-                    Some(_) => tags.as_str_or(""),
-                    None => tags.as_str_or("-"),
-                },
-                match tags {
-                    Some(tags) if !tags.is_empty() => user_agent
-                        .map(|ua| format!(", user-agent={ua}"))
-                        .unwrap_or_default(),
-                    Some(_) | None => user_agent
-                        .map(|ua| format!("user-agent={ua}"))
-                        .unwrap_or_default(),
-                },
+                tags,
+                ua_sep,
+                user_agent,
                 protocol,
                 endpoint,
                 message
             ),
         }
     }
 }
+
+fn prepare_user_agent(ua: &mut String) {
+    let mut ua_bytes = std::mem::take(ua).into_bytes();
+    for c in &mut ua_bytes {
+        if *c == b' ' {
+            *c = b'_';
+        }
+    }
+    if let Some(last) = ua_bytes.last_mut() {
+        if *last == b',' {
+            *last = b'!'
+        }
+    }
+    *ua = unsafe { String::from_utf8_unchecked(ua_bytes) };
+}
diff --git a/lib/src/protocol/kawa_h1/mod.rs b/lib/src/protocol/kawa_h1/mod.rs
@@ -810,7 +810,7 @@ impl<Front: SocketHandler, L: ListenerHandler + L7ListenerHandler> Http<Front, L
         )
     }
 
-    pub fn log_request(&self, metrics: &SessionMetrics, message: Option<&str>) {
+    pub fn log_request(&mut self, metrics: &SessionMetrics, message: Option<&str>) {
         let listener = self.listener.borrow();
         let tags = self.context.authority.as_ref().and_then(|host| {
             let hostname = match host.split_once(':') {
@@ -824,6 +824,7 @@ impl<Front: SocketHandler, L: ListenerHandler + L7ListenerHandler> Http<Front, L
             SessionStatus::DefaultAnswer(answers, ..) => Some(answers.into()),
         };
 
+        let user_agent = self.context.user_agent.take();
         RequestRecord {
             error: message,
             context: self.log_context(),
@@ -841,15 +842,15 @@ impl<Front: SocketHandler, L: ListenerHandler + L7ListenerHandler> Http<Front, L
             client_rtt: socket_rtt(self.front_socket()),
             server_rtt: self.backend_socket.as_ref().and_then(socket_rtt),
             metrics,
-            user_agent: self.context.user_agent.as_deref(),
+            user_agent,
         }
         .log();
     }
 
-    pub fn log_request_success(&self, metrics: &SessionMetrics) {
+    pub fn log_request_success(&mut self, metrics: &SessionMetrics) {
         self.log_request(metrics, None);
     }
-    pub fn log_default_answer_success(&self, metrics: &SessionMetrics) {
+    pub fn log_default_answer_success(&mut self, metrics: &SessionMetrics) {
         self.log_request(metrics, None);
     }
     pub fn log_request_error(&mut self, metrics: &mut SessionMetrics, message: &str) {