diff --git a/response/core/models/action.py b/response/core/models/action.py index fcae5dca..2b8ddf4e 100644 --- a/response/core/models/action.py +++ b/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) diff --git a/response/core/models/incident.py b/response/core/models/incident.py index a2d7d3f1..84418945 100644 --- a/response/core/models/incident.py +++ b/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): @@ -129,3 +130,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) diff --git a/response/core/models/timeline.py b/response/core/models/timeline.py index a669679f..d1074631 100644 --- a/response/core/models/timeline.py +++ b/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 diff --git a/response/core/util.py b/response/core/util.py new file mode 100644 index 00000000..19c632a5 --- /dev/null +++ b/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, + ) diff --git a/response/slack/signals.py b/response/slack/signals.py index 3b597393..da2bf46c 100644 --- a/response/slack/signals.py +++ b/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}"' diff --git a/setup.py b/setup.py index 1401f6e8..bd697be2 100644 --- a/setup.py +++ b/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", diff --git a/tests/api/test_actions.py b/tests/api/test_actions.py index 3185722a..d3c85280 100644 --- a/tests/api/test_actions.py +++ b/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"] = "" + 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 == "<iframe>this should be escaped</iframe>" + ) + + def update_action(arf, api_user, incident_id, action_data): req = arf.put( reverse("incident-action-list", kwargs={"incident_pk": incident_id}), diff --git a/tests/api/test_incidents.py b/tests/api/test_incidents.py index b464023d..e10cc7bf 100644 --- a/tests/api/test_incidents.py +++ b/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>", + ), + ( + "report", + "", + "<script>alert('certainly shouldnt let this happen')</script>", + ), + ( + "summary", + '', + '<meta http-equiv="refresh content=0;">', + ), + ), +) +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 diff --git a/tests/api/test_timeline.py b/tests/api/test_timeline.py index 1fc8a415..515174cf 100644 --- a/tests/api/test_timeline.py +++ b/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>", ), - ("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"