Better handling of redis span/breadcrumb data (#2033)

antonpirker · web-flow · commit 3255a93c8975 · 2023-04-25T09:49:30.000+02:00
- Arguments of the redis AUTH command is never collected (because it contains username and password)
- When send_default_pii=False the arguments of all redis commands are redacted (except the first parameter, because it is always the "key" and thus important for debugging
- Span descriptions and breadcrumb message are truncated to a max size of 1024 (the max size can be configured in a new argument to RedisIntegration(max_data_size=30) (if max_data_size is set to a falsie value (0 or None) then no truncation is done)
diff --git a/sentry_sdk/integrations/redis.py b/sentry_sdk/integrations/redis.py
@@ -2,7 +2,12 @@
 
 from sentry_sdk import Hub
 from sentry_sdk.consts import OP
-from sentry_sdk.utils import capture_internal_exceptions, logger
+from sentry_sdk.hub import _should_send_default_pii
+from sentry_sdk.utils import (
+    SENSITIVE_DATA_SUBSTITUTE,
+    capture_internal_exceptions,
+    logger,
+)
 from sentry_sdk.integrations import Integration, DidNotEnable
 
 from sentry_sdk._types import TYPE_CHECKING
@@ -15,8 +20,13 @@
 )
 _MULTI_KEY_COMMANDS = frozenset(["del", "touch", "unlink"])
 
-#: Trim argument lists to this many values
-_MAX_NUM_ARGS = 10
+_COMMANDS_INCLUDING_SENSITIVE_DATA = [
+    "auth",
+]
+
+_MAX_NUM_ARGS = 10  # Trim argument lists to this many values
+
+_DEFAULT_MAX_DATA_SIZE = 1024
 
 
 def patch_redis_pipeline(pipeline_cls, is_cluster, get_command_args_fn):
@@ -96,6 +106,10 @@ def _patch_rediscluster():
 class RedisIntegration(Integration):
     identifier = "redis"
 
+    def __init__(self, max_data_size=_DEFAULT_MAX_DATA_SIZE):
+        # type: (int) -> None
+        self.max_data_size = max_data_size
+
     @staticmethod
     def setup_once():
         # type: () -> None
@@ -139,8 +153,9 @@ def patch_redis_client(cls, is_cluster):
     def sentry_patched_execute_command(self, name, *args, **kwargs):
         # type: (Any, str, *Any, **Any) -> Any
         hub = Hub.current
+        integration = hub.get_integration(RedisIntegration)
 
-        if hub.get_integration(RedisIntegration) is None:
+        if integration is None:
             return old_execute_command(self, name, *args, **kwargs)
 
         description = name
@@ -151,12 +166,33 @@ def sentry_patched_execute_command(self, name, *args, **kwargs):
                 if i > _MAX_NUM_ARGS:
                     break
 
-                description_parts.append(repr(arg))
+                name_low = name.lower()
+
+                if name_low in _COMMANDS_INCLUDING_SENSITIVE_DATA:
+                    description_parts.append(SENSITIVE_DATA_SUBSTITUTE)
+                    continue
+
+                arg_is_the_key = i == 0
+                if arg_is_the_key:
+                    description_parts.append(repr(arg))
+
+                else:
+                    if _should_send_default_pii():
+                        description_parts.append(repr(arg))
+                    else:
+                        description_parts.append(SENSITIVE_DATA_SUBSTITUTE)
 
             description = " ".join(description_parts)
 
+        data_should_be_truncated = (
+            integration.max_data_size and len(description) > integration.max_data_size
+        )
+        if data_should_be_truncated:
+            description = description[: integration.max_data_size - len("...")] + "..."
+
         with hub.start_span(op=OP.DB_REDIS, description=description) as span:
             span.set_tag("redis.is_cluster", is_cluster)
+
             if name:
                 span.set_tag("redis.command", name)
 
diff --git a/tests/integrations/redis/test_redis.py b/tests/integrations/redis/test_redis.py
@@ -1,3 +1,5 @@
+import mock
+
 from sentry_sdk import capture_message, start_transaction
 from sentry_sdk.integrations.redis import RedisIntegration
 
@@ -37,7 +39,6 @@ def test_redis_pipeline(sentry_init, capture_events, is_transaction):
 
     connection = FakeStrictRedis()
     with start_transaction():
-
         pipeline = connection.pipeline(transaction=is_transaction)
         pipeline.get("foo")
         pipeline.set("bar", 1)
@@ -58,3 +59,166 @@ def test_redis_pipeline(sentry_init, capture_events, is_transaction):
         "redis.transaction": is_transaction,
         "redis.is_cluster": False,
     }
+
+
+def test_sensitive_data(sentry_init, capture_events):
+    # fakeredis does not support the AUTH command, so we need to mock it
+    with mock.patch(
+        "sentry_sdk.integrations.redis._COMMANDS_INCLUDING_SENSITIVE_DATA", ["get"]
+    ):
+        sentry_init(
+            integrations=[RedisIntegration()],
+            traces_sample_rate=1.0,
+            send_default_pii=True,
+        )
+        events = capture_events()
+
+        connection = FakeStrictRedis()
+        with start_transaction():
+            connection.get(
+                "this is super secret"
+            )  # because fakeredis does not support AUTH we use GET instead
+
+        (event,) = events
+        spans = event["spans"]
+        assert spans[0]["op"] == "db.redis"
+        assert spans[0]["description"] == "GET [Filtered]"
+
+
+def test_pii_data_redacted(sentry_init, capture_events):
+    sentry_init(
+        integrations=[RedisIntegration()],
+        traces_sample_rate=1.0,
+    )
+    events = capture_events()
+
+    connection = FakeStrictRedis()
+    with start_transaction():
+        connection.set("somekey1", "my secret string1")
+        connection.set("somekey2", "my secret string2")
+        connection.get("somekey2")
+        connection.delete("somekey1", "somekey2")
+
+    (event,) = events
+    spans = event["spans"]
+    assert spans[0]["op"] == "db.redis"
+    assert spans[0]["description"] == "SET 'somekey1' [Filtered]"
+    assert spans[1]["description"] == "SET 'somekey2' [Filtered]"
+    assert spans[2]["description"] == "GET 'somekey2'"
+    assert spans[3]["description"] == "DEL 'somekey1' [Filtered]"
+
+
+def test_pii_data_sent(sentry_init, capture_events):
+    sentry_init(
+        integrations=[RedisIntegration()],
+        traces_sample_rate=1.0,
+        send_default_pii=True,
+    )
+    events = capture_events()
+
+    connection = FakeStrictRedis()
+    with start_transaction():
+        connection.set("somekey1", "my secret string1")
+        connection.set("somekey2", "my secret string2")
+        connection.get("somekey2")
+        connection.delete("somekey1", "somekey2")
+
+    (event,) = events
+    spans = event["spans"]
+    assert spans[0]["op"] == "db.redis"
+    assert spans[0]["description"] == "SET 'somekey1' 'my secret string1'"
+    assert spans[1]["description"] == "SET 'somekey2' 'my secret string2'"
+    assert spans[2]["description"] == "GET 'somekey2'"
+    assert spans[3]["description"] == "DEL 'somekey1' 'somekey2'"
+
+
+def test_data_truncation(sentry_init, capture_events):
+    sentry_init(
+        integrations=[RedisIntegration()],
+        traces_sample_rate=1.0,
+        send_default_pii=True,
+    )
+    events = capture_events()
+
+    connection = FakeStrictRedis()
+    with start_transaction():
+        long_string = "a" * 100000
+        connection.set("somekey1", long_string)
+        short_string = "b" * 10
+        connection.set("somekey2", short_string)
+
+    (event,) = events
+    spans = event["spans"]
+    assert spans[0]["op"] == "db.redis"
+    assert spans[0]["description"] == "SET 'somekey1' '%s..." % (
+        long_string[: 1024 - len("...") - len("SET 'somekey1' '")],
+    )
+    assert spans[1]["description"] == "SET 'somekey2' '%s'" % (short_string,)
+
+
+def test_data_truncation_custom(sentry_init, capture_events):
+    sentry_init(
+        integrations=[RedisIntegration(max_data_size=30)],
+        traces_sample_rate=1.0,
+        send_default_pii=True,
+    )
+    events = capture_events()
+
+    connection = FakeStrictRedis()
+    with start_transaction():
+        long_string = "a" * 100000
+        connection.set("somekey1", long_string)
+        short_string = "b" * 10
+        connection.set("somekey2", short_string)
+
+    (event,) = events
+    spans = event["spans"]
+    assert spans[0]["op"] == "db.redis"
+    assert spans[0]["description"] == "SET 'somekey1' '%s..." % (
+        long_string[: 30 - len("...") - len("SET 'somekey1' '")],
+    )
+    assert spans[1]["description"] == "SET 'somekey2' '%s'" % (short_string,)
+
+
+def test_breadcrumbs(sentry_init, capture_events):
+
+    sentry_init(
+        integrations=[RedisIntegration(max_data_size=30)],
+        send_default_pii=True,
+    )
+    events = capture_events()
+
+    connection = FakeStrictRedis()
+
+    long_string = "a" * 100000
+    connection.set("somekey1", long_string)
+    short_string = "b" * 10
+    connection.set("somekey2", short_string)
+
+    capture_message("hi")
+
+    (event,) = events
+    crumbs = event["breadcrumbs"]["values"]
+
+    assert crumbs[0] == {
+        "message": "SET 'somekey1' 'aaaaaaaaaaa...",
+        "type": "redis",
+        "category": "redis",
+        "data": {
+            "redis.is_cluster": False,
+            "redis.command": "SET",
+            "redis.key": "somekey1",
+        },
+        "timestamp": crumbs[0]["timestamp"],
+    }
+    assert crumbs[1] == {
+        "message": "SET 'somekey2' 'bbbbbbbbbb'",
+        "type": "redis",
+        "category": "redis",
+        "data": {
+            "redis.is_cluster": False,
+            "redis.command": "SET",
+            "redis.key": "somekey2",
+        },
+        "timestamp": crumbs[1]["timestamp"],
+    }
