From e36757b293d9919d14621951d2c8b05068891738 Mon Sep 17 00:00:00 2001 From: Ben Sully Date: Wed, 19 Oct 2022 12:32:21 +0100 Subject: [PATCH] Allow non-JSON custom webhook templates, provided they render as valid JSON (#639) * Allow non-JSON custom webhook templates, provided they render as valid JSON Previously, both the provided template _and_ the rendered template had to be valid JSON in order for validation to pass. This was unnecessarily restrictive: really, only the rendered template needs to be valid JSON. It also disallowed using templates such as: { "labels": {{ alert_payload.labels | tojson }} } even though this would be valid JSON after rendering. This commit relaxes the validation of custom webhook templates so that they don't need to be valid JSON, provided that the rendered template _is_ valid JSON. This is checked using a dummy dictionary of render params, which use a constant string for the `alert_group_id` field and a `defaultdict(dict)` for the `alert_payload` field. This should permit templates like the one above, but still deny templates such as { "labels": {{ alert_payload.labels }} } which would otherwise fail later if `labels` is not valid JSON. This should resolve #638. * Use defaultdict(str) instead of defaultdict(lambda: "") * Add missing comment to resolution note validation code * Update validation of resolution notes in public API, too * Add extra test cases for JSON webhook templates endpoint of public API --- engine/apps/api/serializers/custom_button.py | 31 +++- engine/apps/api/tests/test_custom_button.py | 101 +++++++++++++ engine/apps/public_api/serializers/action.py | 31 +++- .../public_api/tests/test_custom_actions.py | 137 ++++++++++++++++++ 4 files changed, 288 insertions(+), 12 deletions(-) diff --git a/engine/apps/api/serializers/custom_button.py b/engine/apps/api/serializers/custom_button.py index 11e184e7..0ece3b2e 100644 --- a/engine/apps/api/serializers/custom_button.py +++ b/engine/apps/api/serializers/custom_button.py @@ -1,4 +1,5 @@ import json +from collections import defaultdict from django.core.validators import URLValidator, ValidationError from jinja2 import Template, TemplateError @@ -51,15 +52,33 @@ class CustomButtonSerializer(serializers.ModelSerializer): return None try: - json.loads(data) - except ValueError: - raise serializers.ValidationError("Data has incorrect format") - - try: - Template(data) + template = Template(data) except TemplateError: raise serializers.ValidationError("Data has incorrect template") + try: + rendered = template.render( + { + # Validate that the template can be rendered with a JSON-ish alert payload. + # We don't know what the actual payload will be, so we use a defaultdict + # so that attribute access within a template will never fail + # (provided it's only one level deep - we won't accept templates that attempt + # to do nested attribute access). + # Every attribute access should return a string to ensure that users are + # correctly using `tojson` or wrapping fields in strings. + # If we instead used a `defaultdict(dict)` or `defaultdict(lambda: 1)` we + # would accidentally accept templates such as `{"name": {{ alert_payload.name }}}` + # which would then fail at the true render time due to the + # lack of explicit quotes around the template variable; this would render + # as `{"name": some_alert_name}` which is not valid JSON. + "alert_payload": defaultdict(str), + "alert_group_id": "abcd", + } + ) + json.loads(rendered) + except ValueError: + raise serializers.ValidationError("Data has incorrect format") + return data def validate_forward_whole_payload(self, data): diff --git a/engine/apps/api/tests/test_custom_button.py b/engine/apps/api/tests/test_custom_button.py index bc91fc60..d957b9a1 100644 --- a/engine/apps/api/tests/test_custom_button.py +++ b/engine/apps/api/tests/test_custom_button.py @@ -128,6 +128,91 @@ def test_create_valid_data_button(custom_button_internal_api_setup, make_user_au assert response.json() == expected_response +@pytest.mark.django_db +def test_create_valid_nested_data_button(custom_button_internal_api_setup, make_user_auth_headers): + user, token, custom_button = custom_button_internal_api_setup + client = APIClient() + url = reverse("api-internal:custom_button-list") + + data = { + "name": "amixr_button_with_valid_data", + "webhook": TEST_URL, + # Assert that nested field access still works as long as the variable + # is quoted, making it valid JSON. + # This ensures backwards compatibility from when templates were required + # to be JSON. + "data": '{"nested_item": "{{ alert_payload.foo.bar }}"}', + "team": None, + } + + response = client.post(url, data, format="json", **make_user_auth_headers(user, token)) + # modify initial data by adding id and None for optional fields + custom_button = CustomButton.objects.get(public_primary_key=response.data["id"]) + expected_response = data | { + "id": custom_button.public_primary_key, + "user": None, + "password": None, + "authorization_header": None, + "forward_whole_payload": False, + } + assert response.status_code == status.HTTP_201_CREATED + assert response.json() == expected_response + + +@pytest.mark.django_db +def test_create_valid_data_after_render_button(custom_button_internal_api_setup, make_user_auth_headers): + user, token, custom_button = custom_button_internal_api_setup + client = APIClient() + url = reverse("api-internal:custom_button-list") + + data = { + "name": "amixr_button_with_valid_data", + "webhook": TEST_URL, + "data": '{"name": "{{ alert_payload.name }}", "labels": {{ alert_payload.labels | tojson }}}', + "team": None, + } + + response = client.post(url, data, format="json", **make_user_auth_headers(user, token)) + # modify initial data by adding id and None for optional fields + custom_button = CustomButton.objects.get(public_primary_key=response.data["id"]) + expected_response = data | { + "id": custom_button.public_primary_key, + "user": None, + "password": None, + "authorization_header": None, + "forward_whole_payload": False, + } + assert response.status_code == status.HTTP_201_CREATED + assert response.json() == expected_response + + +@pytest.mark.django_db +def test_create_valid_data_after_render_use_all_data_button(custom_button_internal_api_setup, make_user_auth_headers): + user, token, custom_button = custom_button_internal_api_setup + client = APIClient() + url = reverse("api-internal:custom_button-list") + + data = { + "name": "amixr_button_with_valid_data", + "webhook": TEST_URL, + "data": "{{ alert_payload | tojson }}", + "team": None, + } + + response = client.post(url, data, format="json", **make_user_auth_headers(user, token)) + # modify initial data by adding id and None for optional fields + custom_button = CustomButton.objects.get(public_primary_key=response.data["id"]) + expected_response = data | { + "id": custom_button.public_primary_key, + "user": None, + "password": None, + "authorization_header": None, + "forward_whole_payload": False, + } + assert response.status_code == status.HTTP_201_CREATED + assert response.json() == expected_response + + @pytest.mark.django_db def test_create_invalid_url_custom_button(custom_button_internal_api_setup, make_user_auth_headers): user, token, custom_button = custom_button_internal_api_setup @@ -157,6 +242,22 @@ def test_create_invalid_data_custom_button(custom_button_internal_api_setup, mak assert response.status_code == status.HTTP_400_BAD_REQUEST +@pytest.mark.django_db +def test_create_invalid_templated_data_custom_button(custom_button_internal_api_setup, make_user_auth_headers): + user, token, custom_button = custom_button_internal_api_setup + client = APIClient() + url = reverse("api-internal:custom_button-list") + + data = { + "name": "amixr_button_invalid_data", + "webhook": TEST_URL, + # This would need a `| tojson` or some double quotes around it to pass validation. + "data": "{{ alert_payload.name }}", + } + response = client.post(url, data, format="json", **make_user_auth_headers(user, token)) + assert response.status_code == status.HTTP_400_BAD_REQUEST + + @pytest.mark.django_db def test_update_custom_button(custom_button_internal_api_setup, make_user_auth_headers): user, token, custom_button = custom_button_internal_api_setup diff --git a/engine/apps/public_api/serializers/action.py b/engine/apps/public_api/serializers/action.py index f652bc7c..ab9e4740 100644 --- a/engine/apps/public_api/serializers/action.py +++ b/engine/apps/public_api/serializers/action.py @@ -1,4 +1,5 @@ import json +from collections import defaultdict from django.core.validators import URLValidator, ValidationError from jinja2 import Template, TemplateError @@ -55,15 +56,33 @@ class ActionCreateSerializer(serializers.ModelSerializer): return None try: - json.loads(data) - except ValueError: - raise serializers.ValidationError("Data has incorrect format") - - try: - Template(data) + template = Template(data) except TemplateError: raise serializers.ValidationError("Data has incorrect template") + try: + rendered = template.render( + { + # Validate that the template can be rendered with a JSON-ish alert payload. + # We don't know what the actual payload will be, so we use a defaultdict + # so that attribute access within a template will never fail + # (provided it's only one level deep - we won't accept templates that attempt + # to do nested attribute access). + # Every attribute access should return a string to ensure that users are + # correctly using `tojson` or wrapping fields in strings. + # If we instead used a `defaultdict(dict)` or `defaultdict(lambda: 1)` we + # would accidentally accept templates such as `{"name": {{ alert_payload.name }}}` + # which would then fail at the true render time due to the + # lack of explicit quotes around the template variable; this would render + # as `{"name": some_alert_name}` which is not valid JSON. + "alert_payload": defaultdict(str), + "alert_group_id": "abcd", + } + ) + json.loads(rendered) + except ValueError: + raise serializers.ValidationError("Data has incorrect format") + return data def validate_forward_whole_payload(self, data): diff --git a/engine/apps/public_api/tests/test_custom_actions.py b/engine/apps/public_api/tests/test_custom_actions.py index 9fb4ebb6..7c6a55e3 100644 --- a/engine/apps/public_api/tests/test_custom_actions.py +++ b/engine/apps/public_api/tests/test_custom_actions.py @@ -167,6 +167,120 @@ def test_create_custom_action(make_organization_and_user_with_token): assert response.data == expected_result +@pytest.mark.django_db +def test_create_custom_action_nested_data(make_organization_and_user_with_token): + + organization, user, token = make_organization_and_user_with_token() + client = APIClient() + + url = reverse("api-public:actions-list") + + data = { + "name": "Test outgoing webhook with nested data", + "url": "https://example.com", + # Assert that nested field access still works as long as the variable + # is quoted, making it valid JSON. + # This ensures backwards compatibility from when templates were required + # to be JSON. + "data": '{"nested_item": "{{ alert_payload.foo.bar }}"}', + } + + response = client.post(url, data=data, format="json", HTTP_AUTHORIZATION=f"{token}") + + custom_action = CustomButton.objects.get(public_primary_key=response.data["id"]) + + expected_result = { + "id": custom_action.public_primary_key, + "name": custom_action.name, + "team_id": None, + "url": custom_action.webhook, + "data": custom_action.data, + "user": custom_action.user, + "password": custom_action.password, + "authorization_header": custom_action.authorization_header, + "forward_whole_payload": custom_action.forward_whole_payload, + } + + assert response.status_code == status.HTTP_201_CREATED + assert response.json() == expected_result + + +@pytest.mark.django_db +def test_create_custom_action_valid_after_render(make_organization_and_user_with_token): + + organization, user, token = make_organization_and_user_with_token() + client = APIClient() + + url = reverse("api-public:actions-list") + + data = { + "name": "Test outgoing webhook with nested data", + "url": "https://example.com", + # Assert that nested field access still works as long as the variable + # is quoted, making it valid JSON. + # This ensures backwards compatibility from when templates were required + # to be JSON. + "data": '{"name": "{{ alert_payload.name }}", "labels": {{ alert_payload.labels | tojson }}}', + } + + response = client.post(url, data=data, format="json", HTTP_AUTHORIZATION=f"{token}") + + custom_action = CustomButton.objects.get(public_primary_key=response.data["id"]) + + expected_result = { + "id": custom_action.public_primary_key, + "name": custom_action.name, + "team_id": None, + "url": custom_action.webhook, + "data": custom_action.data, + "user": custom_action.user, + "password": custom_action.password, + "authorization_header": custom_action.authorization_header, + "forward_whole_payload": custom_action.forward_whole_payload, + } + + assert response.status_code == status.HTTP_201_CREATED + assert response.json() == expected_result + + +@pytest.mark.django_db +def test_create_custom_action_valid_after_render_use_all_data(make_organization_and_user_with_token): + + organization, user, token = make_organization_and_user_with_token() + client = APIClient() + + url = reverse("api-public:actions-list") + + data = { + "name": "Test outgoing webhook with nested data", + "url": "https://example.com", + # Assert that nested field access still works as long as the variable + # is quoted, making it valid JSON. + # This ensures backwards compatibility from when templates were required + # to be JSON. + "data": "{{ alert_payload | tojson }}", + } + + response = client.post(url, data=data, format="json", HTTP_AUTHORIZATION=f"{token}") + + custom_action = CustomButton.objects.get(public_primary_key=response.data["id"]) + + expected_result = { + "id": custom_action.public_primary_key, + "name": custom_action.name, + "team_id": None, + "url": custom_action.webhook, + "data": custom_action.data, + "user": custom_action.user, + "password": custom_action.password, + "authorization_header": custom_action.authorization_header, + "forward_whole_payload": custom_action.forward_whole_payload, + } + + assert response.status_code == status.HTTP_201_CREATED + assert response.json() == expected_result + + @pytest.mark.django_db def test_create_custom_action_invalid_data( make_organization_and_user_with_token, @@ -205,6 +319,29 @@ def test_create_custom_action_invalid_data( assert response.status_code == status.HTTP_400_BAD_REQUEST assert response.data["name"][0] == "This field is required." + data = { + "name": "Test outgoing webhook", + "url": "https://example.com", + "data": "invalid_json", + } + + response = client.post(url, data=data, format="json", HTTP_AUTHORIZATION=f"{token}") + + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert response.data["data"][0] == "Data has incorrect format" + + data = { + "name": "Test outgoing webhook", + "url": "https://example.com", + # This would need a `| tojson` or some double quotes around it to pass validation. + "data": "{{ alert_payload.name }}", + } + + response = client.post(url, data=data, format="json", HTTP_AUTHORIZATION=f"{token}") + + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert response.data["data"][0] == "Data has incorrect format" + @pytest.mark.django_db def test_update_custom_action(