From 763fd8ea782296bce54990e9d6a26063a964da1b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C5=81ukasz=20Szcz=C4=99sny?= <luk@wybcz.pl>
Date: Mon, 15 Jan 2024 17:47:39 +0100
Subject: [PATCH] stats: sanitize metric names (#481)

* stats: sanitize metric names

If the configuration contains a key value with a `:` or `|`, e.g.

```
- key: path
  value: "/foo:*"
  rate_limit:
    unit: minute
    requests_per_unit: 20
```

the reported statsd metrics are malformed.

Signed-off-by: Lukasz Szczesny <luk@wybcz.pl>

* Add tests

Signed-off-by: Lukasz Szczesny <luk@wybcz.pl>

---------

Signed-off-by: Lukasz Szczesny <luk@wybcz.pl>
---
 src/stats/manager_impl.go       |  2 ++
 src/utils/utilities.go          |  6 ++++
 test/mocks/stats/manager.go     |  2 ++
 test/stats/manager_impl_test.go | 56 +++++++++++++++++++++++++++++++++
 4 files changed, 66 insertions(+)
 create mode 100644 test/stats/manager_impl_test.go

diff --git a/src/stats/manager_impl.go b/src/stats/manager_impl.go
index effad309..efe8aa07 100644
--- a/src/stats/manager_impl.go
+++ b/src/stats/manager_impl.go
@@ -5,6 +5,7 @@ import (
 	logger "github.com/sirupsen/logrus"
 
 	"github.com/envoyproxy/ratelimit/src/settings"
+	"github.com/envoyproxy/ratelimit/src/utils"
 )
 
 func NewStatManager(store gostats.Store, settings settings.Settings) *ManagerImpl {
@@ -28,6 +29,7 @@ func (this *ManagerImpl) NewStats(key string) RateLimitStats {
 	ret := RateLimitStats{}
 	logger.Debugf("Creating stats for key: '%s'", key)
 	ret.Key = key
+	key = utils.SanitizeStatName(key)
 	ret.TotalHits = this.rlStatsScope.NewCounter(key + ".total_hits")
 	ret.OverLimit = this.rlStatsScope.NewCounter(key + ".over_limit")
 	ret.NearLimit = this.rlStatsScope.NewCounter(key + ".near_limit")
diff --git a/src/utils/utilities.go b/src/utils/utilities.go
index f9ecf856..48f7f7ca 100644
--- a/src/utils/utilities.go
+++ b/src/utils/utilities.go
@@ -61,3 +61,9 @@ func MaskCredentialsInUrl(url string) string {
 
 	return strings.Join(urls, ",")
 }
+
+// Remove invalid characters from the stat name.
+func SanitizeStatName(s string) string {
+	r := strings.NewReplacer(":", "_", "|", "_")
+	return r.Replace(s)
+}
diff --git a/test/mocks/stats/manager.go b/test/mocks/stats/manager.go
index 14850ac6..dd9db246 100644
--- a/test/mocks/stats/manager.go
+++ b/test/mocks/stats/manager.go
@@ -5,6 +5,7 @@ import (
 	logger "github.com/sirupsen/logrus"
 
 	"github.com/envoyproxy/ratelimit/src/stats"
+	"github.com/envoyproxy/ratelimit/src/utils"
 )
 
 type MockStatManager struct {
@@ -36,6 +37,7 @@ func (m *MockStatManager) NewStats(key string) stats.RateLimitStats {
 	ret := stats.RateLimitStats{}
 	logger.Debugf("outputing test gostats %s", key)
 	ret.Key = key
+	key = utils.SanitizeStatName(key)
 	ret.TotalHits = m.store.NewCounter(key + ".total_hits")
 	ret.OverLimit = m.store.NewCounter(key + ".over_limit")
 	ret.NearLimit = m.store.NewCounter(key + ".near_limit")
diff --git a/test/stats/manager_impl_test.go b/test/stats/manager_impl_test.go
new file mode 100644
index 00000000..793ce835
--- /dev/null
+++ b/test/stats/manager_impl_test.go
@@ -0,0 +1,56 @@
+package test_stats
+
+import (
+	"fmt"
+	"testing"
+
+	gostats "github.com/lyft/gostats"
+	gostatsMock "github.com/lyft/gostats/mock"
+	"github.com/stretchr/testify/assert"
+
+	"github.com/envoyproxy/ratelimit/src/settings"
+	"github.com/envoyproxy/ratelimit/src/stats"
+)
+
+func TestEscapingInvalidChartersInMetricName(t *testing.T) {
+	mockSink := gostatsMock.NewSink()
+	statsStore := gostats.NewStore(mockSink, false)
+	statsManager := stats.NewStatManager(statsStore, settings.Settings{})
+
+	tests := []struct {
+		name string
+		key  string
+		want string
+	}{
+		{
+			name: "use not modified key if it does not contain special characters",
+			key:  "path_/foo/bar",
+			want: "path_/foo/bar",
+		},
+		{
+			name: "escape colon",
+			key:  "path_/foo:*:bar",
+			want: "path_/foo_*_bar",
+		},
+		{
+			name: "escape pipe",
+			key:  "path_/foo|bar|baz",
+			want: "path_/foo_bar_baz",
+		},
+		{
+			name: "escape all special characters",
+			key:  "path_/foo:bar|baz",
+			want: "path_/foo_bar_baz",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			stats := statsManager.NewStats(tt.key)
+			assert.Equal(t, tt.key, stats.Key)
+
+			stats.TotalHits.Inc()
+			statsManager.GetStatsStore().Flush()
+			mockSink.AssertCounterExists(t, fmt.Sprintf("ratelimit.service.rate_limit.%s.total_hits", tt.want))
+		})
+	}
+}