webhook_decorator: Support notifying bot owner on invalid JSON.

Our webhook-errors.log file is riddled with exceptions that are
logged when a webhook is incorrectly configured to send data in
a non-JSON format. To avoid this, api_key_only_webhook_view
now supports an additional argument, notify_bot_owner_on_invalid_json.
This argument, when True, will send a PM notification to the bot's
owner notifying them of the configuration issue.

zerver/decorator.py

@@ -17,11 +17,13 @@ from django.shortcuts import resolve_url
 from django.utils.decorators import available_attrs
 from django.utils.timezone import now as timezone_now
 from django.conf import settings
+
 from zerver.lib.queue import queue_json_publish
 from zerver.lib.subdomains import get_subdomain, user_matches_subdomain
 from zerver.lib.timestamp import datetime_to_timestamp, timestamp_to_datetime
 from zerver.lib.utils import statsd, is_remote_server
-from zerver.lib.exceptions import RateLimited, JsonableError, ErrorCode
+from zerver.lib.exceptions import RateLimited, JsonableError, ErrorCode, \
+    InvalidJSONError
 from zerver.lib.types import ViewFuncT
 
 from zerver.lib.rate_limiter import incr_ratelimit, is_ratelimited, \
@@ -326,9 +328,13 @@ def full_webhook_client_name(raw_client_name: Optional[str]=None) -> Optional[st
     return "Zulip{}Webhook".format(raw_client_name)
 
 # Use this for webhook views that don't get an email passed in.
-def api_key_only_webhook_view(webhook_client_name: str) -> Callable[[ViewFuncT], ViewFuncT]:
+def api_key_only_webhook_view(
+        webhook_client_name: str,
+        notify_bot_owner_on_invalid_json: Optional[bool]=False
+) -> Callable[[ViewFuncT], ViewFuncT]:
     # TODO The typing here could be improved by using the Extended Callable types:
     # https://mypy.readthedocs.io/en/latest/kinds_of_types.html#extended-callable-types
+
     def _wrapped_view_func(view_func: ViewFuncT) -> ViewFuncT:
         @csrf_exempt
         @has_request_variables
@@ -342,6 +348,14 @@ def api_key_only_webhook_view(webhook_client_name: str) -> Callable[[ViewFuncT],
                 rate_limit_user(request, user_profile, domain='all')
             try:
                 return view_func(request, user_profile, *args, **kwargs)
+            except InvalidJSONError as e:
+                if not notify_bot_owner_on_invalid_json:
+                    raise e
+                # NOTE: importing this at the top of file leads to a
+                # cyclic import; correct fix is probably to move
+                # notify_bot_owner_about_invalid_json to a smaller file.
+                from zerver.lib.webhooks.common import notify_bot_owner_about_invalid_json
+                notify_bot_owner_about_invalid_json(user_profile, webhook_client_name)
             except Exception as err:
                 log_exception_to_webhook_logger(request, user_profile)
                 raise err

zerver/lib/exceptions.py

@@ -28,6 +28,7 @@ class ErrorCode(AbstractEnum):
     BAD_REQUEST = ()  # Generic name, from the name of HTTP 400.
     REQUEST_VARIABLE_MISSING = ()
     REQUEST_VARIABLE_INVALID = ()
+    INVALID_JSON = ()
     BAD_IMAGE = ()
     REALM_UPLOAD_QUOTA = ()
     BAD_NARROW = ()
@@ -142,5 +143,12 @@ class RateLimited(PermissionDenied):
     def __init__(self, msg: str="") -> None:
         super().__init__(msg)
 
+class InvalidJSONError(JsonableError):
+    code = ErrorCode.INVALID_JSON
+
+    @staticmethod
+    def msg_format() -> str:
+        return _("Malformed JSON")
+
 class BugdownRenderingException(Exception):
     pass

zerver/lib/request.py

@@ -10,7 +10,8 @@ import ujson
 
 from django.utils.translation import ugettext as _
 
-from zerver.lib.exceptions import JsonableError, ErrorCode
+from zerver.lib.exceptions import JsonableError, ErrorCode, \
+    InvalidJSONError
 
 from django.http import HttpRequest, HttpResponse
 
@@ -155,7 +156,7 @@ def has_request_variables(view_func):
                 try:
                     val = ujson.loads(request.body)
                 except ValueError:
-                    raise JsonableError(_('Malformed JSON'))
+                    raise InvalidJSONError(_("Malformed JSON"))
                 kwargs[param.func_var_name] = val
                 continue
             elif param.argument_type is not None:

zerver/lib/webhooks/common.py

@@ -23,9 +23,22 @@ an older version of the third-party service that doesn't provide that header.
 Contact {support_email} if you need help debugging!
 """
 
+INVALID_JSON_MESSAGE = """
+Hi there! It looks like you tried to setup the Zulip {webhook_name} integration,
+but didn't correctly configure the webhook to send data in the JSON format
+that this integration expects!
+"""
+
 # Django prefixes all custom HTTP headers with `HTTP_`
 DJANGO_HTTP_PREFIX = "HTTP_"
 
+def notify_bot_owner_about_invalid_json(user_profile: UserProfile,
+                                        webhook_client_name: str) -> None:
+    send_rate_limited_pm_notification_to_bot_owner(
+        user_profile, user_profile.realm,
+        INVALID_JSON_MESSAGE.format(webhook_name=webhook_client_name).strip()
+    )
+
 class UnexpectedWebhookEventType(JsonableError):
     code = ErrorCode.UNEXPECTED_WEBHOOK_EVENT_TYPE
     data_fields = ['webhook_name', 'event_type']

zerver/tests/test_webhooks_common.py

@@ -1,9 +1,15 @@
 # -*- coding: utf-8 -*-
+from django.http import HttpRequest
+
+from zerver.decorator import api_key_only_webhook_view
+from zerver.lib.exceptions import InvalidJSONError, JsonableError
 from zerver.lib.test_classes import ZulipTestCase, WebhookTestCase
 from zerver.lib.webhooks.common import \
     validate_extract_webhook_http_header, \
-    MISSING_EVENT_HEADER_MESSAGE, MissingHTTPEventHeader
-from zerver.models import get_user, get_realm
+    MISSING_EVENT_HEADER_MESSAGE, MissingHTTPEventHeader, \
+    INVALID_JSON_MESSAGE
+from zerver.models import get_user, get_realm, UserProfile
+from zerver.lib.users import get_api_key
 from zerver.lib.send_email import FromAddress
 from zerver.lib.test_helpers import HostRequestMock
 
@@ -44,6 +50,40 @@ class WebhooksCommonTestCase(ZulipTestCase):
         self.assertEqual(msg.sender.email, notification_bot.email)
         self.assertEqual(msg.content, expected_message)
 
+    def test_notify_bot_owner_on_invalid_json(self)-> None:
+        @api_key_only_webhook_view('ClientName')
+        def my_webhook_raises_exception(request: HttpRequest, user_profile: UserProfile) -> None:
+            raise InvalidJSONError("Malformed JSON")
+
+        @api_key_only_webhook_view('ClientName', notify_bot_owner_on_invalid_json=True)
+        def my_webhook(request: HttpRequest, user_profile: UserProfile) -> None:
+            raise InvalidJSONError("Malformed JSON")
+
+        webhook_bot_email = 'webhook-bot@zulip.com'
+        webhook_bot_realm = get_realm('zulip')
+        webhook_bot = get_user(webhook_bot_email, webhook_bot_realm)
+        webhook_bot_api_key = get_api_key(webhook_bot)
+        request = HostRequestMock()
+        request.POST['api_key'] = webhook_bot_api_key
+        request.host = "zulip.testserver"
+        expected_msg = INVALID_JSON_MESSAGE.format(webhook_name='ClientName')
+
+        last_message_id = self.get_last_message().id
+        with self.assertRaisesRegex(JsonableError, "Malformed JSON"):
+            my_webhook_raises_exception(request)  # type: ignore # mypy doesn't seem to apply the decorator
+
+        # First verify that without the setting, it doesn't send a PM to bot owner.
+        msg = self.get_last_message()
+        self.assertEqual(msg.id, last_message_id)
+        self.assertNotEqual(msg.content, expected_msg.strip())
+
+        # Then verify that with the setting, it does send such a message.
+        my_webhook(request)  # type: ignore # mypy doesn't seem to apply the decorator
+        msg = self.get_last_message()
+        self.assertNotEqual(msg.id, last_message_id)
+        self.assertEqual(msg.sender.email, self.notification_bot().email)
+        self.assertEqual(msg.content, expected_msg.strip())
+
 class MissingEventHeaderTestCase(WebhookTestCase):
     STREAM_NAME = 'groove'
     URL_TEMPLATE = '/api/v1/external/groove?stream={stream}&api_key={api_key}'