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
This commit is contained in:
Michael Derynck 2022-11-28 16:46:51 +00:00 committed by GitHub
parent dc6fcf5c05
commit 3582f9b08f
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
18 changed files with 204 additions and 88 deletions

View file

@ -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

View file

@ -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 @@ class AlertTemplater(ABC):
"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

View file

@ -189,12 +189,12 @@ class Alert(models.Model):
# 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 @@ class Alert(models.Model):
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:

View file

@ -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.db.models import F
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 @@ class CustomButton(models.Model):
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):

View file

@ -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:

View file

@ -20,7 +20,8 @@ from common.api_helpers.custom_fields import TeamPrimaryKeyRelatedField, Writabl
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
@ -28,9 +29,10 @@ 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):

View file

@ -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 @@ class CustomButtonSerializer(serializers.ModelSerializer):
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

View file

@ -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."

View file

@ -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

View file

@ -1,5 +1,6 @@
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
@ -7,6 +8,7 @@ from apps.api.serializers.alert_receive_channel import AlertReceiveChannelTempla
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 @@ class AlertReceiveChannelTemplateView(
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(

View file

@ -4,7 +4,6 @@ import math
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.base.messaging import get_messaging_backends
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 @@ class PreviewTemplateMixin:
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}

View file

@ -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

View file

@ -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

View file

@ -0,0 +1,65 @@
import json
import pytest
from django.conf import settings
from common.jinja_templater import apply_jinja_template
from common.jinja_templater.apply_jinja_template import JinjaTemplateError, JinjaTemplateWarning
def test_apply_jinja_template():
payload = {"name": "test"}
rendered = apply_jinja_template("{{ payload | tojson_pretty }}", payload)
result = json.loads(rendered)
assert payload == result
def test_apply_jinja_template_bad_syntax_error():
with pytest.raises(JinjaTemplateError):
apply_jinja_template("{{%", payload={})
def test_apply_jinja_template_unknown_filter_error():
with pytest.raises(JinjaTemplateError):
apply_jinja_template("{{ payload | to_json_pretty }}", payload={})
def test_apply_jinja_template_unsafe_error():
with pytest.raises(JinjaTemplateError):
apply_jinja_template("{{ payload.__init__() }}", payload={})
def test_apply_jinja_template_restricted_error():
with pytest.raises(JinjaTemplateError):
apply_jinja_template("{% for n in range(100) %}Hello{% endfor %}", payload={})
def test_apply_jinja_template_restricted_inside_conditional():
template = "{% if 'blabla' in payload %}{% for n in range(100) %}Hello{% endfor %}{% endif %}"
# No exception when condition == False
apply_jinja_template(template, payload={})
with pytest.raises(JinjaTemplateError):
apply_jinja_template(template, payload={"blabla": "test"})
def test_apply_jinja_template_missing_field_warning():
with pytest.raises(JinjaTemplateWarning):
apply_jinja_template("{{ payload.field.name }}", payload={})
def test_apply_jinja_template_type_warning():
with pytest.raises(JinjaTemplateWarning):
apply_jinja_template("{{ payload.name + 25 }}", payload={"name": "test"})
def test_apply_jinja_template_too_large():
template = "test" * 20000
with pytest.raises(JinjaTemplateError):
apply_jinja_template(template, payload={})
def test_apply_jinja_template_result_truncate():
payload = {"value": "test" * 20000}
result = apply_jinja_template("{{ payload.value }}", payload)
# Length == Limit + 2 to account for '..' appended to end
assert len(result) == settings.JINJA_RESULT_MAX_LENGTH + 2

View file

@ -574,6 +574,9 @@ SELF_HOSTED_SETTINGS = {
GRAFANA_INCIDENT_STATIC_API_KEY = os.environ.get("GRAFANA_INCIDENT_STATIC_API_KEY", None)
DATA_UPLOAD_MAX_MEMORY_SIZE = getenv_integer("DATA_UPLOAD_MAX_MEMORY_SIZE", 1_048_576) # 1mb by default
JINJA_TEMPLATE_MAX_LENGTH = 50000
JINJA_RESULT_TITLE_MAX_LENGTH = 500
JINJA_RESULT_MAX_LENGTH = 50000
# Log inbound/outbound calls as slow=1 if they exceed threshold
SLOW_THRESHOLD_SECONDS = 2.0

View file

@ -27,7 +27,6 @@ const cx = cn.bind(styles);
interface AlertTemplatesFormProps {
templates: any;
onUpdateTemplates: (values: any) => void;
errors: any;
alertReceiveChannelId: AlertReceiveChannel['id'];
alertGroupId?: Alert['pk'];
demoAlertEnabled: boolean;

View file

@ -6,7 +6,7 @@ import AlertTemplatesForm from 'components/AlertTemplates/AlertTemplatesForm';
import { AlertReceiveChannel } from 'models/alert_receive_channel';
import { Alert } from 'models/alertgroup/alertgroup.types';
import { useStore } from 'state/useStore';
import { openNotification } from 'utils';
import { openErrorNotification, openNotification } from 'utils';
interface TeamEditContainerProps {
onHide: () => void;
@ -24,7 +24,6 @@ const AlertTemplatesFormContainer = observer((props: TeamEditContainerProps) =>
const store = useStore();
const [templatesRefreshing, setTemplatesRefreshing] = useState<boolean>(false);
const [errors, setErrors] = useState({});
useEffect(() => {
store.alertReceiveChannelStore.updateItem(alertReceiveChannelId);
@ -41,8 +40,12 @@ const AlertTemplatesFormContainer = observer((props: TeamEditContainerProps) =>
onUpdateTemplates();
}
})
.catch((data) => {
setErrors(data.response.data);
.catch((err) => {
if (err.response?.data?.length > 0) {
openErrorNotification(err.response.data);
} else {
openErrorNotification(err.message);
}
});
},
[alertReceiveChannelId, onUpdateTemplates, store.alertReceiveChannelStore]
@ -64,7 +67,6 @@ const AlertTemplatesFormContainer = observer((props: TeamEditContainerProps) =>
<AlertTemplatesForm
alertReceiveChannelId={alertReceiveChannelId}
alertGroupId={alertGroupId}
errors={errors}
templates={templates}
onUpdateTemplates={onUpdateTemplatesCallback}
demoAlertEnabled={alertReceiveChannel?.demo_alert_enabled}

View file

@ -7,6 +7,7 @@ import { observer } from 'mobx-react';
import { AlertReceiveChannel } from 'models/alert_receive_channel/alert_receive_channel.types';
import { Alert } from 'models/alertgroup/alertgroup.types';
import { useStore } from 'state/useStore';
import { openErrorNotification } from 'utils';
import { useDebouncedCallback } from 'utils/hooks';
import sanitize from 'utils/sanitize';
@ -35,7 +36,15 @@ const TemplatePreview = observer((props: TemplatePreviewProps) => {
(alertGroupId
? alertGroupStore.renderPreview(alertGroupId, templateName, templateBody)
: alertReceiveChannelStore.renderPreview(alertReceiveChannelId, templateName, templateBody)
).then(setResult);
)
.then(setResult)
.catch((err) => {
if (err.response?.data?.length > 0) {
openErrorNotification(err.response.data);
} else {
openErrorNotification(err.message);
}
});
}, 1000);
useEffect(handleTemplateBodyChange, [templateBody]);