New lint: HIGHER_DATA_SENSITIVITY_REQUIRED for when an event extra key could potentially contain sensitive data

Author: badboy

CHANGELOG.md

@@ -2,6 +2,8 @@
 
 ## Unreleased
 
+- New lint: `HIGHER_DATA_SENSITIVITY_REQUIRED` for when an event extra key could potentially contain sensitive data ([bug 1973017](https://bugzilla.mozilla.org/show_bug.cgi?id=1973017))
+
 ## 17.2.0
 
 - Forbid redefinition of metrics, categories, or pings within the same YAML document ([bug 1921089](https://bugzilla.mozilla.org/show_bug.cgi?id=1921089))

glean_parser/lint.py

@@ -320,6 +320,36 @@ def check_empty_datareview(
         yield "List of data reviews should not contain empty strings or TODO markers."
 
 
+def check_event_extras_potential_data_sensitivity_required(
+    metric: metrics.Metric, parser_config: Dict[str, Any]
+) -> LintGenerator:
+    # Only looking at event metrics
+    if not isinstance(metric, metrics.Event):
+        return
+
+    # TODO(bug 1890648): Not all metrics have `data_sensitivity` defined.
+    has_data_sensitivity = hasattr(metric, "data_sensitivity")
+    # If already marked as "highly sensitive" no need for further checks
+    if has_data_sensitivity and any(
+        [
+            sensitivity == metrics.DataSensitivity.highly_sensitive
+            for sensitivity in metric.data_sensitivity
+        ]
+    ):
+        return
+
+    # List of potentially sensitive extra key names we want to flag.
+    potential_sensitive_names = ["url", "uri"]
+    name_list = ", ".join(potential_sensitive_names)
+
+    for extra_key in metric.extra_keys.keys():
+        if extra_key in potential_sensitive_names:
+            yield (
+                f"`{extra_key}` could potentially be used to collect sensitive data. Increase the metric's data sensitivity or disable the lint."
+                + f" (This lint applies for the following extra key names: {name_list})"
+            )
+
+
 def check_redundant_ping(
     pings: pings.Ping, parser_config: Dict[str, Any]
 ) -> LintGenerator:
@@ -432,6 +462,10 @@ def check_name_too_similar(
     "METRIC_ON_EVENTS_LIFETIME": (check_metric_on_events_lifetime, CheckType.error),
     "UNEXPECTED_UNIT": (check_unexpected_unit, CheckType.warning),
     "EMPTY_DATAREVIEW": (check_empty_datareview, CheckType.warning),
+    "HIGHER_DATA_SENSITIVITY_REQUIRED": (
+        check_event_extras_potential_data_sensitivity_required,
+        CheckType.warning,
+    ),
 }
 
 

tests/data/events_data_sensitivity.yaml

@@ -0,0 +1,78 @@
+# Any copyright is dedicated to the Public Domain.
+# https://creativecommons.org/publicdomain/zero/1.0/
+
+---
+$schema: moz://mozilla.org/schemas/glean/metrics/2-0-0
+
+event:
+  low_sensitivity:
+    type: event
+    description: |
+      Just testing events
+    bugs:
+      - https://bugzilla.mozilla.org/show_bug.cgi?id=1973017
+    data_reviews:
+      - http://example.com/reviews
+    notification_emails:
+      - [email protected]
+    extra_keys:
+      url:
+        type: string
+        description: "This is key one"
+    expires: never
+    data_sensitivity:
+      - technical
+
+  no_data_sensitivity:
+    type: event
+    description: |
+      Just testing events
+    bugs:
+      - https://bugzilla.mozilla.org/show_bug.cgi?id=1973017
+    data_reviews:
+      - http://example.com/reviews
+    notification_emails:
+      - [email protected]
+    extra_keys:
+      url:
+        type: string
+        description: "This is key one"
+    expires: never
+
+  correct_sensitivity:
+    type: event
+    description: |
+      Just testing events
+    bugs:
+      - https://bugzilla.mozilla.org/show_bug.cgi?id=1973017
+    data_reviews:
+      - http://example.com/reviews
+    notification_emails:
+      - [email protected]
+    extra_keys:
+      url:
+        type: string
+        description: "This is key one"
+    expires: never
+    data_sensitivity:
+      - highly_sensitive
+
+  exempt:
+    type: event
+    description: |
+      Just testing events
+    bugs:
+      - https://bugzilla.mozilla.org/show_bug.cgi?id=1973017
+    data_reviews:
+      - http://example.com/reviews
+    notification_emails:
+      - [email protected]
+    extra_keys:
+      url:
+        type: string
+        description: "This is key one"
+    expires: never
+    data_sensitivity:
+      - technical
+    no_lint:
+      - HIGHER_DATA_SENSITIVITY_REQUIRED

tests/test_lint.py

@@ -644,3 +644,112 @@ def test_unit_on_metrics(metric, num_nits):
     assert len(nits) == num_nits
     if num_nits > 0:
         assert set(["UNEXPECTED_UNIT"]) == set(v.check_name for v in nits)
+
+
+@pytest.mark.parametrize(
+    "metric, num_nits",
+    [
+        (
+            {
+                "metric": {
+                    "type": "event",
+                    "extra_keys": {
+                        "id": {"type": "string", "description": "description"}
+                    },
+                }
+            },
+            0,
+        ),
+        (
+            {
+                "metric": {
+                    "type": "event",
+                    "extra_keys": {
+                        "url": {"type": "string", "description": "description"}
+                    },
+                }
+            },
+            1,
+        ),
+        (
+            {
+                "metric": {
+                    "type": "event",
+                    "extra_keys": {
+                        "url": {"type": "string", "description": "description"}
+                    },
+                    "no_lint": ["HIGHER_DATA_SENSITIVITY_REQUIRED"],
+                }
+            },
+            0,
+        ),
+        (
+            {
+                "metric": {
+                    "type": "event",
+                    "extra_keys": {
+                        "url": {"type": "string", "description": "description"}
+                    },
+                    "data_sensitivity": ["technical"],
+                }
+            },
+            1,
+        ),
+        (
+            {
+                "metric": {
+                    "type": "event",
+                    "extra_keys": {
+                        "url": {"type": "string", "description": "description"}
+                    },
+                    "data_sensitivity": ["highly_sensitive"],
+                }
+            },
+            0,
+        ),
+        (
+            {
+                "metric": {
+                    "type": "event",
+                    "extra_keys": {
+                        "url": {"type": "string", "description": "description"}
+                    },
+                    "data_sensitivity": ["technical", "highly_sensitive"],
+                }
+            },
+            0,
+        ),
+    ],
+)
+def test_events_data_sensitivity(metric, num_nits):
+    content = {"category": metric}
+    content = util.add_required(content)
+    all_metrics = parser.parse_objects(content)
+
+    errs = list(all_metrics)
+    assert len(errs) == 0
+
+    nits = lint.lint_metrics(all_metrics.value)
+
+    assert len(nits) == num_nits
+    if num_nits > 0:
+        assert set(["HIGHER_DATA_SENSITIVITY_REQUIRED"]) == set(
+            v.check_name for v in nits
+        )
+
+
+def test_events_data_sensitivity_from_file():
+    """Test that the 'glinter' reports issues with the old event API."""
+    all_metrics = parser.parse_objects([ROOT / "data" / "events_data_sensitivity.yaml"])
+    errs = list(all_metrics)
+    assert len(errs) == 0
+
+    nits = lint.lint_metrics(all_metrics.value, parser_config={})
+    assert len(nits) == 2
+    assert nits[0].check_name == "HIGHER_DATA_SENSITIVITY_REQUIRED"
+    assert nits[0].name == "event.low_sensitivity"
+    assert "data sensitivity" in nits[0].msg
+
+    assert nits[1].check_name == "HIGHER_DATA_SENSITIVITY_REQUIRED"
+    assert nits[1].name == "event.no_data_sensitivity"
+    assert "data sensitivity" in nits[1].msg