From bc66018f176545363d89ad711506e1bf3e82797a Mon Sep 17 00:00:00 2001
From: Mathieu Leplatre <mathieu@mozilla.com>
Date: Wed, 11 Dec 2024 15:39:32 +0100
Subject: [PATCH]  Fix #3474: encode unsafe characters in request summary
 metric  (#3476)

* Fix #3474: encode unsafe characters in request summary metric

* Prevent ':' to reach metric unique name in StatsD
---
 kinto/core/initialization.py      |  6 ++++--
 kinto/plugins/statsd.py           | 12 ++++++++++--
 tests/core/test_initialization.py | 19 +++++++++++++++++++
 tests/plugins/test_statsd.py      |  5 +++++
 4 files changed, 38 insertions(+), 4 deletions(-)

diff --git a/kinto/core/initialization.py b/kinto/core/initialization.py
index 397dc7ce9..0b8fa7adc 100644
--- a/kinto/core/initialization.py
+++ b/kinto/core/initialization.py
@@ -1,6 +1,7 @@
 import logging
 import random
 import re
+import urllib.parse
 import warnings
 from datetime import datetime
 from secrets import token_hex
@@ -474,6 +475,7 @@ def on_new_response(event):
 
         try:
             endpoint = utils.strip_uri_prefix(request.path)
+            endpoint = urllib.parse.quote_plus(endpoint, safe="/?&=-_")
         except UnicodeDecodeError as e:
             # This `on_new_response` callback is also called when a HTTP 400
             # is returned because of an invalid UTF-8 path. We still want metrics.
@@ -507,7 +509,7 @@ def on_new_response(event):
             unique=[
                 ("method", request.method.lower()),
                 ("endpoint", endpoint),
-                ("status", str(request.response.status_code)),
+                ("status", str(event.response.status_code)),
             ]
             + metrics_matchdict_labels,
         )
@@ -527,7 +529,7 @@ def on_new_response(event):
         # Observe response size.
         metrics_service.observe(
             "request_size",
-            len(request.response.body or b""),
+            len(event.response.body or b""),
             labels=[("endpoint", endpoint)] + metrics_matchdict_labels,
         )
 
diff --git a/kinto/plugins/statsd.py b/kinto/plugins/statsd.py
index b6a8830fb..5ae894011 100644
--- a/kinto/plugins/statsd.py
+++ b/kinto/plugins/statsd.py
@@ -13,6 +13,14 @@
     statsd_module = None
 
 
+def sanitize(value):
+    """
+    Telegraf does not support ':' in values.
+    See https://github.com/influxdata/telegraf/issues/4495
+    """
+    return value.replace(":", "") if isinstance(value, str) else value
+
+
 @implementer(metrics.IMetricsService)
 class StatsDService:
     def __init__(self, host, port, prefix):
@@ -22,7 +30,7 @@ def timer(self, key):
         return self._client.timer(key)
 
     def observe(self, key, value, labels=[]):
-        return self._client.gauge(key, value)
+        return self._client.gauge(key, sanitize(value))
 
     def count(self, key, count=1, unique=None):
         if unique is None:
@@ -30,7 +38,7 @@ def count(self, key, count=1, unique=None):
         if isinstance(unique, list):
             # [("method", "get")] -> "method.get"
             # [("endpoint", "/"), ("method", "get")] -> "endpoint./.method.get"
-            unique = ".".join(f"{label[0]}.{label[1]}" for label in unique)
+            unique = ".".join(f"{label[0]}.{sanitize(label[1])}" for label in unique)
         else:
             warnings.warn(
                 "`unique` parameter should be of type ``list[tuple[str, str]]``",
diff --git a/tests/core/test_initialization.py b/tests/core/test_initialization.py
index bac6dd423..fd57620b8 100644
--- a/tests/core/test_initialization.py
+++ b/tests/core/test_initialization.py
@@ -420,6 +420,25 @@ def test_statsd_counts_endpoints(self):
             unique=[("method", "get"), ("endpoint", "/__heartbeat__"), ("status", "200")],
         )
 
+    def test_statsd_sanitizes_url_in_metrics(self):
+        kinto.core.initialize(self.config, "0.0.1", "settings_prefix")
+        app = webtest.TestApp(self.config.make_wsgi_app())
+        app.get(
+            "/v0/changeset'%7C%22%3F%3E%3C!DOCTYPE%22http://xh3E'),'/l')%20from%20dual)%7C'",
+            status=404,
+        )
+        self.mocked().count.assert_any_call(
+            "request_summary",
+            unique=[
+                ("method", "get"),
+                (
+                    "endpoint",
+                    "/changeset%27%257C%2522%253F%253E%253C%21DOCTYPE%2522http%3A//xh3E%27%29%2C%27/l%27%29%2520from%2520dual%29%257C%27",
+                ),
+                ("status", "404"),
+            ],
+        )
+
     def test_statsd_observe_request_size(self):
         kinto.core.initialize(self.config, "0.0.1", "settings_prefix")
         app = webtest.TestApp(self.config.make_wsgi_app())
diff --git a/tests/plugins/test_statsd.py b/tests/plugins/test_statsd.py
index e40af66b3..d801eea86 100644
--- a/tests/plugins/test_statsd.py
+++ b/tests/plugins/test_statsd.py
@@ -61,6 +61,11 @@ def test_count_turns_multiple_tuples_into_one_set_key(self):
             self.client.count("click", unique=[("component", "menu"), ("sound", "off")])
             mocked_client.set.assert_called_with("click", "component.menu.sound.off")
 
+    def test_values_are_sanitized(self):
+        with mock.patch.object(self.client, "_client") as mocked_client:
+            self.client.count("click", unique=[("user", "account:boss")])
+            mocked_client.set.assert_called_with("click", "user.accountboss")
+
     @mock.patch("kinto.plugins.statsd.statsd_module")
     def test_load_from_config(self, module_mock):
         config = testing.setUp()