Merge pull request #155 from monzo/sanitize-user-input

core models: sanitize user input

response/core/models/action.py

@@ -2,6 +2,7 @@
 
 from response.core.models.incident import Incident
 from response.core.models.user_external import ExternalUser
+from response.core.util import sanitize
 
 
 class Action(models.Model):
@@ -17,3 +18,7 @@ def icon(self):
 
     def __str__(self):
         return f"{self.details}"
+
+    def save(self, *args, **kwargs):
+        self.details = sanitize(self.details)
+        super(Action, self).save(*args, **kwargs)

response/core/models/incident.py

@@ -4,6 +4,7 @@
 
 from response import core, slack
 from response.core.models.user_external import ExternalUser
+from response.core.util import sanitize
 
 
 class IncidentManager(models.Manager):
@@ -132,3 +133,9 @@ def action_items(self):
 
     def timeline_events(self):
         return core.models.TimelineEvent.objects.filter(incident=self)
+
+    def save(self, *args, **kwargs):
+        self.impact = sanitize(self.impact)
+        self.report = sanitize(self.report)
+        self.summary = sanitize(self.summary)
+        super(Incident, self).save(*args, **kwargs)

response/core/models/timeline.py

@@ -4,6 +4,7 @@
 from jsonfield import JSONField
 
 from response.core.models.incident import Incident
+from response.core.util import sanitize
 
 EVENT_TYPES = (
     ("text", "Freeform text field"),
@@ -23,6 +24,10 @@ class TimelineEvent(models.Model):
         help_text="Additional fields that can be added by other event types", null=True
     )
 
+    def save(self, *args, **kwargs):
+        self.text = sanitize(self.text)
+        super(TimelineEvent, self).save(*args, **kwargs)
+
 
 def add_incident_update_event(
     incident, update_type, old_value, new_value, text, timestamp=None

response/core/util.py

@@ -0,0 +1,11 @@
+import bleach
+import bleach_whitelist
+
+
+def sanitize(string):
+    return bleach.clean(
+        string,
+        tags=bleach_whitelist.markdown_tags,
+        attributes=bleach_whitelist.markdown_attrs,
+        styles=bleach_whitelist.all_styles,
+    )

response/slack/signals.py

@@ -122,9 +122,7 @@ def update_incident_report_event(prev_state, instance):
 
 def update_incident_summary_event(prev_state, instance):
     if prev_state.summary:
-        text = (
-            f'Incident summary updated from "{prev_state.summary}" to "{instance.summary}"',
-        )
+        text = f'Incident summary updated from "{prev_state.summary}" to "{instance.summary}"'
     else:
         text = f'Incident summary added: "{instance.summary}"'
 
@@ -140,7 +138,7 @@ def update_incident_summary_event(prev_state, instance):
 def update_incident_impact_event(prev_state, instance):
     if prev_state.impact:
         text = (
-            f'Incident impact updated from "{prev_state.impact}" to "{instance.impact}"',
+            f'Incident impact updated from "{prev_state.impact}" to "{instance.impact}"'
         )
     else:
         text = f'Incident impact added: "{instance.impact}"'

setup.py

@@ -8,6 +8,7 @@
 INSTALL_REQUIRES = [
     "Django>=2.2",
     "bleach==3.1.0",
+    "bleach-whitelist>=0.0.10",
     "cryptography>=2.7",
     "django-after-response>=0.2.2",
     "django-bootstrap4>=0.0.7",

tests/api/test_actions.py

@@ -99,6 +99,21 @@ def test_update_action(arf, api_user):
     assert updated_action.done == action_data["done"]
 
 
+def test_update_action_sanitized(arf, api_user):
+    incident = IncidentFactory.create()
+    action = incident.action_items()[0]
+    action_data = serializers.ActionSerializer(action).data
+
+    action_data["details"] = "<iframe>this should be escaped</iframe>"
+    response = update_action(arf, api_user, incident.pk, action_data)
+    assert response.status_code == 200, "Got non-201 response from API"
+
+    updated_action = Action.objects.get(pk=action.pk)
+    assert (
+        updated_action.details == "&lt;iframe&gt;this should be escaped&lt;/iframe&gt;"
+    )
+
+
 def update_action(arf, api_user, incident_id, action_data):
     req = arf.put(
         reverse("incident-action-list", kwargs={"incident_pk": incident_id}),

tests/api/test_incidents.py

@@ -158,6 +158,61 @@ def test_update_incident(arf, api_user, update_key, update_value):
         ), "Updated value wasn't persisted to the DB"
 
 
+@pytest.mark.parametrize(
+    "update_key,update_value,expected_value",
+    (
+        (
+            "impact",
+            "<iframe>this should be escaped</iframe>",
+            "&lt;iframe&gt;this should be escaped&lt;/iframe&gt;",
+        ),
+        (
+            "report",
+            "<script>alert('certainly shouldnt let this happen')</script>",
+            "&lt;script&gt;alert('certainly shouldnt let this happen')&lt;/script&gt;",
+        ),
+        (
+            "summary",
+            '<meta http-equiv="refresh content=0;">',
+            '&lt;meta http-equiv="refresh content=0;"&gt;',
+        ),
+    ),
+)
+def test_update_incident_sanitize_fields(
+    arf, api_user, update_key, update_value, expected_value
+):
+    """
+    Tests that we can't inject potentially dangerous HTML/JS into incident
+    fields
+    """
+    persisted_incidents = IncidentFactory.create_batch(5)
+
+    incident = persisted_incidents[0]
+    serializer = serializers.IncidentSerializer(incident)
+    serialized = serializer.data
+
+    updated = serialized
+    del updated["reporter"]  # can't update reporter
+    if update_key:
+        updated[update_key] = update_value
+
+    req = arf.put(
+        reverse("incident-detail", kwargs={"pk": incident.pk}), updated, format="json"
+    )
+    force_authenticate(req, user=api_user)
+
+    response = IncidentViewSet.as_view({"put": "update"})(req, pk=incident.pk)
+    print(response.rendered_content)
+    assert response.status_code == 200, "Got non-200 response from API"
+
+    if update_key:
+        new_incident = Incident.objects.get(pk=incident.pk)
+        print(getattr(new_incident, update_key))
+        assert (
+            getattr(new_incident, update_key) == expected_value
+        ), "Updated value wasn't persisted to the DB"
+
+
 def test_update_incident_lead(arf, api_user):
     """
     Tests that we can update the incident lead by name

tests/api/test_timeline.py

@@ -77,17 +77,23 @@ def test_list_timeline_events_by_incident(arf, api_user):
 
 
 @pytest.mark.parametrize(
-    "update_key,update_value",
+    "update_key,update_value,expected_value",
     (
-        ("", ""),  # no update
+        ("", "", None),  # no update
         (
             "timestamp",
             faker.date_time_between(start_date="-3d", end_date="now", tzinfo=None),
+            None,
+        ),
+        ("text", faker.paragraph(nb_sentences=5, variable_nb_sentences=True), None),
+        (
+            "text",
+            "<script>alert('certainly shouldnt let this happen')</script>",
+            "&lt;script&gt;alert('certainly shouldnt let this happen')&lt;/script&gt;",
         ),
-        ("text", faker.paragraph(nb_sentences=5, variable_nb_sentences=True)),
     ),
 )
-def test_update_timeline_event(arf, api_user, update_key, update_value):
+def test_update_timeline_event(arf, api_user, update_key, update_value, expected_value):
     incident = IncidentFactory.create()
 
     event_model = incident.timeline_events()[0]
@@ -109,8 +115,10 @@ def test_update_timeline_event(arf, api_user, update_key, update_value):
     assert response.status_code == 200, "Got non-200 response from API"
     if update_key:
         new_event = TimelineEvent.objects.get(pk=event_model.pk)
+
+        expected = expected_value or update_value
         assert (
-            getattr(new_event, update_key) == update_value
+            getattr(new_event, update_key) == expected
         ), "Updated value wasn't persisted to the DB"