Improve Jinja Template feedback and error handling (#884)

* Improve feedback so template errors are given to user

* Add security error logging

* Add limits for templates, payloads, results

* Show popup error notification for webhook errors and template errors that don't have a result

* Update tests

* Split exceptions into warnings/errors to give more control when previewing, rendering, saving templates

* Limit title lengths

* Make TypeError a warning

* Adjust title length limit

* Remove length limiting on urlize since it is being done on template render

* Fix tests

* Add KeyError and ValueError to warnings

* No longer enforcing json result when saving webhook in case it is dependent on payload

* Add tests for expected exceptions coming from apply_jinja_template

* Update changelog

* Send raw post if template result is not JSON

CHANGELOG.md

@@ -5,6 +5,14 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
+## v1.1.6 (TBD)
+
+### Fixed
+- Got 500 error when saving Outgoing Webhook ([#890](https://github.com/grafana/oncall/issues/890))
+
+### Changed
+- When editing templates for alert group presentation or outgoing webhooks, errors and warnings are now displayed in the UI as notification popups or displayed in the preview.
+- Errors and warnings that occur when rendering templates during notification or webhooks will now render and display the error/warning as the result.
 ## v1.1.5 (2022-11-24)
 
 ### Fixed

engine/apps/alerts/incident_appearance/templaters/alert_templater.py

@@ -1,9 +1,12 @@
 from abc import ABC, abstractmethod
 from dataclasses import dataclass
 
+from django.conf import settings
+
 from apps.base.messaging import get_messaging_backend_from_id
 from apps.slack.slack_formatter import SlackFormatter
 from common.jinja_templater import apply_jinja_template
+from common.jinja_templater.apply_jinja_template import JinjaTemplateError, JinjaTemplateWarning
 
 
 class TemplateLoader:
@@ -172,9 +175,15 @@ def _render_attribute_with_template(self, attr, data, channel, templated_alert):
                 "amixr_incident_id": self.incident_id,  # TODO: decide on variable names
                 "amixr_link": self.link,  # TODO: decide on variable names
             }
-            templated_attr, success = apply_jinja_template(attr_template, data, **context)
-            if success:
-                return templated_attr
+            try:
+                if attr == "title":
+                    return apply_jinja_template(
+                        attr_template, data, result_length_limit=settings.JINJA_RESULT_TITLE_MAX_LENGTH, **context
+                    )
+                else:
+                    return apply_jinja_template(attr_template, data, **context)
+            except (JinjaTemplateError, JinjaTemplateWarning) as e:
+                return e.fallback_message
 
         return None
 

engine/apps/alerts/models/alert.py

@@ -189,12 +189,12 @@ def render_group_data(cls, alert_receive_channel, raw_request_data, is_demo=Fals
         # set web_title_cache to web title to allow alert group searching based on web_title_cache
         web_title_template = template_manager.get_attr_template("title", alert_receive_channel, render_for="web")
         if web_title_template:
-            web_title_cache = apply_jinja_template(web_title_template, raw_request_data)[0] or None
+            web_title_cache = apply_jinja_template(web_title_template, raw_request_data)
         else:
             web_title_cache = None
 
         if grouping_id_template is not None:
-            group_distinction, _ = apply_jinja_template(grouping_id_template, raw_request_data)
+            group_distinction = apply_jinja_template(grouping_id_template, raw_request_data)
 
         # Insert random uuid to prevent grouping of demo alerts or alerts with group_distinction=None
         if is_demo or not group_distinction:
@@ -204,13 +204,13 @@ def render_group_data(cls, alert_receive_channel, raw_request_data, is_demo=Fals
             group_distinction = hashlib.md5(str(group_distinction).encode()).hexdigest()
 
         if resolve_condition_template is not None:
-            is_resolve_signal, _ = apply_jinja_template(resolve_condition_template, payload=raw_request_data)
+            is_resolve_signal = apply_jinja_template(resolve_condition_template, payload=raw_request_data)
             if isinstance(is_resolve_signal, str):
                 is_resolve_signal = is_resolve_signal.strip().lower() in ["1", "true", "ok"]
             else:
                 is_resolve_signal = False
         if acknowledge_condition_template is not None:
-            is_acknowledge_signal, _ = apply_jinja_template(acknowledge_condition_template, payload=raw_request_data)
+            is_acknowledge_signal = apply_jinja_template(acknowledge_condition_template, payload=raw_request_data)
             if isinstance(is_acknowledge_signal, str):
                 is_acknowledge_signal = is_acknowledge_signal.strip().lower() in ["1", "true", "ok"]
             else:

engine/apps/alerts/models/custom_button.py

@@ -1,6 +1,7 @@
 import json
 import logging
 import re
+from json import JSONDecodeError
 
 from django.conf import settings
 from django.core.validators import MinLengthValidator
@@ -9,7 +10,8 @@
 from django.utils import timezone
 from requests.auth import HTTPBasicAuth
 
-from common.jinja_templater import jinja_template_env
+from common.jinja_templater import apply_jinja_template
+from common.jinja_templater.apply_jinja_template import JinjaTemplateError, JinjaTemplateWarning
 from common.public_primary_keys import generate_public_primary_key, increase_public_primary_key_length
 
 logger = logging.getLogger(__name__)
@@ -103,13 +105,19 @@ def build_post_kwargs(self, alert):
         if self.forward_whole_payload:
             post_kwargs["json"] = alert.raw_request_data
         elif self.data:
-            rendered_data = jinja_template_env.from_string(self.data).render(
-                {
-                    "alert_payload": self._escape_alert_payload(alert.raw_request_data),
-                    "alert_group_id": alert.group.public_primary_key,
-                }
-            )
-            post_kwargs["json"] = json.loads(rendered_data)
+            try:
+                rendered_data = apply_jinja_template(
+                    self.data,
+                    alert_payload=self._escape_alert_payload(alert.raw_request_data),
+                    alert_group_id=alert.group.public_primary_key,
+                )
+            except (JinjaTemplateError, JinjaTemplateWarning) as e:
+                post_kwargs["json"] = {"error": e.fallback_message}
+
+            try:
+                post_kwargs["json"] = json.loads(rendered_data)
+            except JSONDecodeError:
+                post_kwargs["data"] = rendered_data
         return post_kwargs
 
     def _escape_alert_payload(self, payload: dict):

engine/apps/alerts/tasks/alert_group_web_title_cache.py

@@ -76,7 +76,7 @@ def update_web_title_cache(alert_receive_channel_pk, alert_group_pks):
         if web_title_template:
             if alert_group.pk in first_alert_map:
                 raw_request_data = first_alert_map[alert_group.pk]["raw_request_data"]
-                web_title_cache = apply_jinja_template(web_title_template, raw_request_data)[0] or None
+                web_title_cache = apply_jinja_template(web_title_template, raw_request_data)
             else:
                 web_title_cache = None
         else:

engine/apps/api/serializers/alert_receive_channel.py

@@ -20,17 +20,19 @@
 from common.api_helpers.exceptions import BadRequest
 from common.api_helpers.mixins import IMAGE_URL, TEMPLATE_NAMES_ONLY_WITH_NOTIFICATION_CHANNEL, EagerLoadingMixin
 from common.api_helpers.utils import CurrentTeamDefault
-from common.jinja_templater import jinja_template_env
+from common.jinja_templater import apply_jinja_template, jinja_template_env
+from common.jinja_templater.apply_jinja_template import JinjaTemplateWarning
 
 from .integration_heartbeat import IntegrationHeartBeatSerializer
 
 
 def valid_jinja_template_for_serializer_method_field(template):
     for _, val in template.items():
         try:
-            jinja_template_env.from_string(val)
-        except TemplateSyntaxError:
-            raise serializers.ValidationError("invalid template")
+            apply_jinja_template(val, payload={})
+        except JinjaTemplateWarning:
+            # Suppress warnings, template may be valid with payload
+            pass
 
 
 class AlertReceiveChannelSerializer(EagerLoadingMixin, serializers.ModelSerializer):

engine/apps/api/serializers/custom_button.py

@@ -1,15 +1,14 @@
-import json
 from collections import defaultdict
 
 from django.core.validators import URLValidator, ValidationError
-from jinja2 import TemplateError
 from rest_framework import serializers
 from rest_framework.validators import UniqueTogetherValidator
 
 from apps.alerts.models import CustomButton
 from common.api_helpers.custom_fields import TeamPrimaryKeyRelatedField
 from common.api_helpers.utils import CurrentOrganizationDefault, CurrentTeamDefault
-from common.jinja_templater import jinja_template_env
+from common.jinja_templater import apply_jinja_template
+from common.jinja_templater.apply_jinja_template import JinjaTemplateError, JinjaTemplateWarning
 
 
 class CustomButtonSerializer(serializers.ModelSerializer):
@@ -53,32 +52,12 @@ def validate_data(self, data):
             return None
 
         try:
-            template = jinja_template_env.from_string(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")
+            apply_jinja_template(data, alert_payload=defaultdict(str), alert_group_id="abcd")
+        except JinjaTemplateError as e:
+            raise serializers.ValidationError(e.fallback_message)
+        except JinjaTemplateWarning:
+            # Suppress render exceptions since we do not have a representative payload to test with
+            pass
 
         return data
 

engine/apps/api/tests/test_alert_group.py

@@ -1446,8 +1446,9 @@ def test_alert_group_preview_body_non_existent_template_var(
     data = {"template_name": "testonly_title_template", "template_body": "foobar: {{ foobar.does_not_exist }}"}
     response = client.post(url, data, format="json", **make_user_auth_headers(user, token))
 
+    # Return errors as preview body instead of None
     assert response.status_code == status.HTTP_200_OK
-    assert response.json()["preview"] is None
+    assert response.json()["preview"] == "Template Warning: 'foobar' is undefined"
 
 
 @pytest.mark.django_db
@@ -1468,4 +1469,6 @@ def test_alert_group_preview_body_invalid_template_syntax(
     data = {"template_name": "testonly_title_template", "template_body": "{{'' if foo is None else foo}}"}
     response = client.post(url, data, format="json", **make_user_auth_headers(user, token))
 
-    assert response.status_code == status.HTTP_400_BAD_REQUEST
+    # Errors now returned preview content
+    assert response.status_code == status.HTTP_200_OK
+    assert response.data["preview"] == "Template Error: No test named 'None' found."

engine/apps/api/tests/test_custom_button.py

@@ -236,23 +236,7 @@ def test_create_invalid_data_custom_button(custom_button_internal_api_setup, mak
     data = {
         "name": "amixr_button_invalid_data",
         "webhook": TEST_URL,
-        "data": "invalid_json",
-    }
-    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_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 }}",
+        "data": "{{%",
     }
     response = client.post(url, data, format="json", **make_user_auth_headers(user, token))
     assert response.status_code == status.HTTP_400_BAD_REQUEST

engine/apps/api/views/alert_receive_channel_template.py

@@ -1,12 +1,14 @@
-from rest_framework import mixins, viewsets
+from rest_framework import mixins, status, viewsets
 from rest_framework.permissions import IsAuthenticated
+from rest_framework.response import Response
 
 from apps.alerts.models import AlertReceiveChannel
 from apps.api.permissions import MODIFY_ACTIONS, READ_ACTIONS, ActionPermission, AnyRole, IsAdmin
 from apps.api.serializers.alert_receive_channel import AlertReceiveChannelTemplatesSerializer
 from apps.auth_token.auth import PluginAuthentication
 from common.api_helpers.mixins import PublicPrimaryKeyMixin
 from common.insight_log import EntityEvent, write_resource_insight_log
+from common.jinja_templater.apply_jinja_template import JinjaTemplateError
 
 
 class AlertReceiveChannelTemplateView(
@@ -36,7 +38,10 @@ def get_queryset(self):
     def update(self, request, *args, **kwargs):
         instance = self.get_object()
         prev_state = instance.insight_logs_serialized
-        result = super().update(request, *args, **kwargs)
+        try:
+            result = super().update(request, *args, **kwargs)
+        except JinjaTemplateError as e:
+            return Response(e.fallback_message, status.HTTP_400_BAD_REQUEST)
         instance = self.get_object()
         new_state = instance.insight_logs_serialized
         write_resource_insight_log(

engine/common/api_helpers/mixins.py

@@ -4,7 +4,6 @@
 from django.core.exceptions import ObjectDoesNotExist
 from django.db.models import Q
 from django.utils.functional import cached_property
-from jinja2.exceptions import TemplateRuntimeError
 from rest_framework import status
 from rest_framework.decorators import action
 from rest_framework.exceptions import NotFound, Throttled
@@ -23,6 +22,7 @@
 from apps.user_management.models import Team
 from common.api_helpers.exceptions import BadRequest
 from common.jinja_templater import apply_jinja_template
+from common.jinja_templater.apply_jinja_template import JinjaTemplateError, JinjaTemplateWarning
 
 
 class UpdateSerializerMixin:
@@ -324,13 +324,16 @@ def get_attr_template(self, attr, alert_receive_channel, render_for=None):
             templater.template_manager = PreviewTemplateLoader()
             try:
                 templated_alert = templater.render()
-            except TemplateRuntimeError:
-                raise BadRequest(detail={"template_body": "Invalid template syntax"})
+            except (JinjaTemplateError, JinjaTemplateWarning) as e:
+                return Response({"preview": e.fallback_message}, status.HTTP_200_OK)
 
             templated_attr = getattr(templated_alert, attr_name)
 
         elif attr_name in TEMPLATE_NAMES_WITHOUT_NOTIFICATION_CHANNEL:
-            templated_attr, _ = apply_jinja_template(template_body, payload=alert_to_template.raw_request_data)
+            try:
+                templated_attr = apply_jinja_template(template_body, payload=alert_to_template.raw_request_data)
+            except (JinjaTemplateError, JinjaTemplateWarning) as e:
+                return Response({"preview": e.fallback_message}, status.HTTP_200_OK)
         else:
             templated_attr = None
         response = {"preview": templated_attr}

engine/common/jinja_templater/apply_jinja_template.py

@@ -1,12 +1,42 @@
-from jinja2 import TemplateSyntaxError, UndefinedError
+import logging
+
+from django.conf import settings
+from jinja2 import TemplateAssertionError, TemplateSyntaxError, UndefinedError
+from jinja2.exceptions import SecurityError
 
 from .jinja_template_env import jinja_template_env
 
+logger = logging.getLogger(__name__)
+
+
+class JinjaTemplateError(Exception):
+    def __init__(self, fallback_message):
+        self.fallback_message = f"Template Error: {fallback_message}"
+
+
+class JinjaTemplateWarning(Exception):
+    def __init__(self, fallback_message):
+        self.fallback_message = f"Template Warning: {fallback_message}"
+
+
+def apply_jinja_template(template, payload=None, result_length_limit=settings.JINJA_RESULT_MAX_LENGTH, **kwargs):
+    if len(template) > settings.JINJA_TEMPLATE_MAX_LENGTH:
+        raise JinjaTemplateError(
+            f"Template exceeds length limit ({len(template)} > {settings.JINJA_TEMPLATE_MAX_LENGTH})"
+        )
 
-def apply_jinja_template(template, payload=None, **kwargs):
     try:
-        template = jinja_template_env.from_string(template)
-        result = template.render(payload=payload, **kwargs)
-        return result, True
-    except (UndefinedError, TypeError, ValueError, KeyError, TemplateSyntaxError):
-        return None, False
+        compiled_template = jinja_template_env.from_string(template)
+        result = compiled_template.render(payload=payload, **kwargs)
+    except SecurityError as e:
+        logger.warning(f"SecurityError process template={template} payload={payload}")
+        raise JinjaTemplateError(str(e))
+    except (TemplateAssertionError, TemplateSyntaxError) as e:
+        raise JinjaTemplateError(str(e))
+    except (TypeError, KeyError, ValueError, UndefinedError) as e:
+        raise JinjaTemplateWarning(str(e))
+    except Exception as e:
+        logger.error(f"Unexpected template error: {str(e)} template={template} payload={payload}")
+        raise JinjaTemplateError(str(e))
+
+    return (result[:result_length_limit] + "..") if len(result) > result_length_limit else result

engine/common/jinja_templater/jinja_template_env.py

@@ -1,13 +1,20 @@
 from django.utils import timezone
 from jinja2 import BaseLoader
+from jinja2.exceptions import SecurityError
 from jinja2.sandbox import SandboxedEnvironment
 
 from .filters import datetimeformat, iso8601_to_time, regex_replace, to_pretty_json
 
+
+def raise_security_exception(name):
+    raise SecurityError(f"use of '{name}' is restricted")
+
+
 jinja_template_env = SandboxedEnvironment(loader=BaseLoader())
 
 jinja_template_env.filters["datetimeformat"] = datetimeformat
 jinja_template_env.filters["iso8601_to_time"] = iso8601_to_time
 jinja_template_env.filters["tojson_pretty"] = to_pretty_json
 jinja_template_env.globals["time"] = timezone.now
+jinja_template_env.globals["range"] = lambda *args: raise_security_exception("range")
 jinja_template_env.filters["regex_replace"] = regex_replace