Support DJSTRIPE_WEBHOOK_VALIDATION config as per the documentation

* Changed runtests to use DJSTRIPE_WEBHOOK_VALIDATION="verify_signature" since it's the default behaviour and it matches the existing behaviour on master * update test_webhooks settings handling as per other tests (use override_settings and import.reload)
ryancausey · Nov 14, 2018 · 721d11c · 721d11c
1 parent 8393a50
commit 721d11c
Show file tree

Hide file tree

Showing 6 changed files with 85 additions and 9 deletions.
diff --git a/djstripe/checks.py b/djstripe/checks.py
@@ -143,6 +143,40 @@ def check_webhook_secret(app_configs=None, **kwargs):
     return messages
 
 
+@checks.register("djstripe")
+def check_webhook_validation(app_configs=None, **kwargs):
+    """
+    Check that DJSTRIPE_WEBHOOK_VALIDATION is valid
+    """
+    from . import settings as djstripe_settings
+
+    messages = []
+
+    validation_options = ("verify_signature", "retrieve_event")
+
+    if djstripe_settings.WEBHOOK_VALIDATION is None:
+        messages.append(checks.Warning(
+            "Webhook validation is disabled, this is a security risk if the webhook view is enabled",
+            hint="Set DJSTRIPE_WEBHOOK_VALIDATION to one of {}".format(", ".join(validation_options)),
+            id="djstripe.W004"
+        ))
+    elif djstripe_settings.WEBHOOK_VALIDATION == "verify_signature":
+        if not djstripe_settings.WEBHOOK_SECRET:
+            messages.append(checks.Critical(
+                "DJSTRIPE_WEBHOOK_VALIDATION='verify_signature' but DJSTRIPE_WEBHOOK_SECRET is not set",
+                hint="Set DJSTRIPE_WEBHOOK_SECRET or set DJSTRIPE_WEBHOOK_VALIDATION='retrieve_event'",
+                id="djstripe.C006"
+            ))
+    elif djstripe_settings.WEBHOOK_VALIDATION not in validation_options:
+        messages.append(checks.Critical(
+            "DJSTRIPE_WEBHOOK_VALIDATION is invalid",
+            hint="Set DJSTRIPE_WEBHOOK_VALIDATION to one of {} or None".format(", ".join(validation_options)),
+            id="djstripe.C007"
+        ))
+
+    return messages
+
+
 @checks.register("djstripe")
 def check_subscriber_key_length(app_configs=None, **kwargs):
     """

diff --git a/djstripe/models/webhooks.py b/djstripe/models/webhooks.py
@@ -145,8 +145,10 @@ def validate(self, api_key=None):
             logger.info("Test webhook received: {}".format(local_data))
             return False
 
-        # If the DJSTRIPE_WEBHOOK_SECRET setting is set, use signature verification
-        if djstripe_settings.WEBHOOK_SECRET:
+        if djstripe_settings.WEBHOOK_VALIDATION is None:
+            # validation disabled
+            return True
+        elif djstripe_settings.WEBHOOK_VALIDATION == "verify_signature" and djstripe_settings.WEBHOOK_SECRET:
             try:
                 stripe.WebhookSignature.verify_header(
                     self.body,

diff --git a/djstripe/settings.py b/djstripe/settings.py
@@ -70,6 +70,7 @@ def _get_idempotency_key(object_type, action, livemode):
 DJSTRIPE_WEBHOOK_URL = getattr(settings, "DJSTRIPE_WEBHOOK_URL", r"^webhook/$")
 
 WEBHOOK_TOLERANCE = getattr(settings, "DJSTRIPE_WEBHOOK_TOLERANCE", stripe.Webhook.DEFAULT_TOLERANCE)
+WEBHOOK_VALIDATION = getattr(settings, "DJSTRIPE_WEBHOOK_VALIDATION", "verify_signature")
 WEBHOOK_SECRET = getattr(settings, "DJSTRIPE_WEBHOOK_SECRET", "")
 
 # Webhook event callbacks allow an application to take control of what happens

diff --git a/docs/reference/settings.rst b/docs/reference/settings.rst
@@ -208,6 +208,9 @@ If this is set to a non-empty value, webhook signatures will be verified.
 
 .. _Learn more about webhook signature verification: https://stripe.com/docs/webhooks/signatures
 
+DJSTRIPE_WEBHOOK_VALIDATION= (="verify_signature")
+==================================================
+
 This setting controls which type of validation is done on webhooks.
 Value can be ``"verify_signature"`` for signature verification (recommended
 default), ``"retrieve_event"`` for event retrieval (makes an extra HTTP

diff --git a/runtests.py b/runtests.py
@@ -147,7 +147,8 @@ def run_test_suite(args):
         ),
         DJSTRIPE_USE_NATIVE_JSONFIELD=os.environ.get("USE_NATIVE_JSONFIELD", "") == "1",
         DJSTRIPE_SUBSCRIPTION_REDIRECT="test_url_subscribe",
-        DJSTRIPE_WEBHOOK_VALIDATION="retrieve_event",
+        DJSTRIPE_WEBHOOK_VALIDATION="verify_signature",
+        DJSTRIPE_WEBHOOK_SECRET="whsec_XXXXX",
     )
 
     # Avoid AppRegistryNotReady exception

diff --git a/tests/test_webhooks.py b/tests/test_webhooks.py
@@ -10,6 +10,7 @@
 import warnings
 from collections import defaultdict
 from copy import deepcopy
+from importlib import reload
 from unittest.mock import Mock, PropertyMock, call, patch
 
 from django.test import TestCase, override_settings
@@ -31,6 +32,10 @@ def mock_webhook_handler(webhook_event_trigger):
 
 
 class TestWebhook(TestCase):
+
+    def tearDown(self):
+        reload(djstripe_settings)
+
     def _send_event(self, event_data):
         return Client().post(
             reverse("djstripe:webhook"),
@@ -44,51 +49,81 @@ def test_webhook_test_event(self):
         self.assertEqual(resp.status_code, 200)
         self.assertFalse(Event.objects.filter(id=TEST_EVENT_ID).exists())
 
+    @override_settings(DJSTRIPE_WEBHOOK_VALIDATION="retrieve_event")
     @patch("stripe.Transfer.retrieve", return_value=deepcopy(FAKE_TRANSFER))
     @patch("stripe.Event.retrieve", return_value=deepcopy(FAKE_EVENT_TRANSFER_CREATED))
     def test_webhook_retrieve_event_fail(self, event_retrieve_mock, transfer_retrieve_mock):
+        reload(djstripe_settings)
+
         invalid_event = deepcopy(FAKE_EVENT_TRANSFER_CREATED)
         invalid_event["id"] = "evt_invalid"
         invalid_event["data"]["valid"] = "not really"
 
-        djstripe_settings.WEBHOOK_SECRET = ""
         resp = self._send_event(invalid_event)
 
         self.assertEqual(resp.status_code, 400)
         self.assertFalse(Event.objects.filter(id="evt_invalid").exists())
 
+    @override_settings(DJSTRIPE_WEBHOOK_VALIDATION="retrieve_event")
     @patch("stripe.Transfer.retrieve", return_value=deepcopy(FAKE_TRANSFER))
     @patch("stripe.Event.retrieve", return_value=deepcopy(FAKE_EVENT_TRANSFER_CREATED))
     def test_webhook_retrieve_event_pass(self, event_retrieve_mock, transfer_retrieve_mock):
-        djstripe_settings.WEBHOOK_SECRET = ""
+        reload(djstripe_settings)
+
         resp = self._send_event(FAKE_EVENT_TRANSFER_CREATED)
 
         self.assertEqual(resp.status_code, 200)
+        event_retrieve_mock.assert_called_once_with(
+            api_key=djstripe_settings.STRIPE_SECRET_KEY,
+            id=FAKE_EVENT_TRANSFER_CREATED["id"]
+        )
 
-    @override_settings(DJSTRIPE_WEBHOOK_VALIDATION="verify_signature")
+    @override_settings(DJSTRIPE_WEBHOOK_VALIDATION="verify_signature", DJSTRIPE_WEBHOOK_SECRET="whsec_XXXXX")
     @patch("stripe.Transfer.retrieve", return_value=deepcopy(FAKE_TRANSFER))
     @patch("stripe.Event.retrieve", return_value=deepcopy(FAKE_EVENT_TRANSFER_CREATED))
     def test_webhook_invalid_verify_signature_fail(self, event_retrieve_mock, transfer_retrieve_mock):
+        reload(djstripe_settings)
+
         invalid_event = deepcopy(FAKE_EVENT_TRANSFER_CREATED)
         invalid_event["id"] = "evt_invalid"
         invalid_event["data"]["valid"] = "not really"
 
-        djstripe_settings.WEBHOOK_SECRET = "whsec_XXXXX"
         resp = self._send_event(invalid_event)
 
         self.assertEqual(resp.status_code, 400)
         self.assertFalse(Event.objects.filter(id="evt_invalid").exists())
 
+    @override_settings(DJSTRIPE_WEBHOOK_VALIDATION="verify_signature", DJSTRIPE_WEBHOOK_SECRET="whsec_XXXXX")
+    @patch("stripe.WebhookSignature.verify_header", return_value=True)
     @patch("stripe.Transfer.retrieve", return_value=deepcopy(FAKE_TRANSFER))
     @patch("stripe.Event.retrieve", return_value=deepcopy(FAKE_EVENT_TRANSFER_CREATED))
-    @patch("stripe.WebhookSignature.verify_header", return_value=True)
     def test_webhook_verify_signature_pass(self, event_retrieve_mock, transfer_retrieve_mock, verify_signature_mock):
-        djstripe_settings.WEBHOOK_SECRET = "whsec_XXXXX"
+        reload(djstripe_settings)
+
         resp = self._send_event(FAKE_EVENT_TRANSFER_CREATED)
 
         self.assertEqual(resp.status_code, 200)
         self.assertFalse(Event.objects.filter(id="evt_invalid").exists())
         verify_signature_mock.called_once_with(FAKE_EVENT_TRANSFER_CREATED, {})
+        event_retrieve_mock.assert_not_called()
+
+    @override_settings(DJSTRIPE_WEBHOOK_VALIDATION=None)
+    @patch("stripe.WebhookSignature.verify_header")
+    @patch("stripe.Transfer.retrieve", return_value=deepcopy(FAKE_TRANSFER))
+    @patch("stripe.Event.retrieve", return_value=deepcopy(FAKE_EVENT_TRANSFER_CREATED))
+    def test_webhook_no_validation_pass(self, event_retrieve_mock, transfer_retrieve_mock, verify_signature_mock):
+        reload(djstripe_settings)
+
+        invalid_event = deepcopy(FAKE_EVENT_TRANSFER_CREATED)
+        invalid_event["id"] = "evt_invalid"
+        invalid_event["data"]["valid"] = "not really"
+
+        resp = self._send_event(invalid_event)
+
+        self.assertEqual(resp.status_code, 200)
+        self.assertTrue(Event.objects.filter(id="evt_invalid").exists())
+        event_retrieve_mock.assert_not_called()
+        verify_signature_mock.assert_not_called()
 
     def test_webhook_no_signature(self):
         self.assertEqual(WebhookEventTrigger.objects.count(), 0)