Integrations: Don't allow webhooks without a secret (#11083)

* Integrations: Don't allow webhooks without a secret

This is just a clean up, we already deprecated the use of webhooks without a secret.

* Linter

Author: stsewd

readthedocs/api/v2/views/integrations.py

@@ -1,6 +1,5 @@
 """Endpoints integrating with Github, Bitbucket, and other webhooks."""
 
-import datetime
 import hashlib
 import hmac
 import json
@@ -10,7 +9,6 @@
 
 import structlog
 from django.shortcuts import get_object_or_404
-from django.utils import timezone
 from django.utils.crypto import constant_time_compare
 from rest_framework import permissions, status
 from rest_framework.exceptions import NotFound, ParseError
@@ -74,14 +72,6 @@ class WebhookMixin:
     integration = None
     integration_type = None
     invalid_payload_msg = 'Payload not valid'
-    missing_secret_for_pr_events_msg = dedent(
-        """
-        This webhook doesn't have a secret configured.
-        For security reasons, webhooks without a secret can't process pull/merge request events.
-        For more information, read our blog post: https://blog.readthedocs.com/security-update-on-incoming-webhooks/.
-        """
-    ).strip()
-
     missing_secret_deprecated_msg = dedent(
         """
         This webhook doesn't have a secret configured.
@@ -116,12 +106,9 @@ def post(self, request, project_slug):
         except Project.DoesNotExist as exc:
             raise NotFound("Project not found") from exc
 
-        # Deprecate webhooks without a secret
+        # Webhooks without a secret are no longer permitted.
         # https://blog.readthedocs.com/security-update-on-incoming-webhooks/.
-        now = timezone.now()
-        deprecation_date = datetime.datetime(2024, 1, 31, tzinfo=datetime.timezone.utc)
-        is_deprecated = now >= deprecation_date
-        if is_deprecated and not self.has_secret():
+        if not self.has_secret():
             return Response(
                 {"detail": self.missing_secret_deprecated_msg},
                 status=HTTP_400_BAD_REQUEST,
@@ -418,12 +405,9 @@ def is_payload_valid(self):
         See https://developer.github.com/webhooks/securing/.
         """
         signature = self.request.headers.get(GITHUB_SIGNATURE_HEADER)
-        secret = self.get_integration().secret
-        if not secret:
-            log.debug('Skipping payload signature validation.')
-            return True
         if not signature:
             return False
+        secret = self.get_integration().secret
         msg = self.request.body.decode()
         digest = WebhookMixin.get_digest(secret, msg)
         result = hmac.compare_digest(
@@ -492,13 +476,6 @@ def handle_webhook(self):
 
         # Handle pull request events.
         if self.project.external_builds_enabled and event == GITHUB_PULL_REQUEST:
-            # Requests from anonymous users are ignored.
-            if not integration.secret:
-                return Response(
-                    {"detail": self.missing_secret_for_pr_events_msg},
-                    status=HTTP_400_BAD_REQUEST,
-                )
-
             if action in [
                 GITHUB_PULL_REQUEST_OPENED,
                 GITHUB_PULL_REQUEST_REOPENED,
@@ -598,10 +575,9 @@ def is_payload_valid(self):
         See https://docs.gitlab.com/ee/user/project/integrations/webhooks.html#secret-token.
         """
         token = self.request.headers.get(GITLAB_TOKEN_HEADER, "")
+        if not token:
+            return False
         secret = self.get_integration().secret
-        if not secret:
-            log.debug('Skipping payload signature validation.')
-            return True
         return constant_time_compare(secret, token)
 
     def get_external_version_data(self):
@@ -636,8 +612,6 @@ def handle_webhook(self):
             event=event,
         )
 
-        integration = self.get_integration()
-
         # Always update `latest` branch to point to the default branch in the repository
         # even if the event is not gonna be handled. This helps us to keep our db in sync.
         default_branch = self.data.get("project", {}).get("default_branch", None)
@@ -665,12 +639,6 @@ def handle_webhook(self):
                 raise ParseError('Parameter "ref" is required') from exc
 
         if self.project.external_builds_enabled and event == GITLAB_MERGE_REQUEST:
-            if not integration.secret:
-                return Response(
-                    {"detail": self.missing_secret_for_pr_events_msg},
-                    status=HTTP_400_BAD_REQUEST,
-                )
-
             if action in [
                 GITLAB_MERGE_REQUEST_OPEN,
                 GITLAB_MERGE_REQUEST_REOPEN,
@@ -779,12 +747,9 @@ def is_payload_valid(self):
         See https://support.atlassian.com/bitbucket-cloud/docs/manage-webhooks/#Secure-webhooks.
         """
         signature = self.request.headers.get(BITBUCKET_SIGNATURE_HEADER)
-        secret = self.get_integration().secret
-        if not secret:
-            log.debug("Skipping payload signature validation.")
-            return True
         if not signature:
             return False
+        secret = self.get_integration().secret
         msg = self.request.body.decode()
         digest = WebhookMixin.get_digest(secret, msg)
         result = hmac.compare_digest(

readthedocs/rtd_tests/tests/test_api.py

@@ -2287,25 +2287,6 @@ def test_github_empty_signature(self, trigger_build):
             GitHubWebhookView.invalid_payload_msg
         )
 
-    def test_github_skip_signature_validation(self, trigger_build):
-        client = APIClient()
-        payload = '{"ref":"master"}'
-        Integration.objects.filter(pk=self.github_integration.pk).update(secret=None)
-        headers = {
-            GITHUB_EVENT_HEADER: GITHUB_PUSH,
-            GITHUB_SIGNATURE_HEADER: 'skipped',
-        }
-        resp = client.post(
-            reverse(
-                'api_webhook_github',
-                kwargs={'project_slug': self.project.slug}
-            ),
-            json.loads(payload),
-            format='json',
-            headers=headers,
-        )
-        self.assertEqual(resp.status_code, 200)
-
     @mock.patch('readthedocs.core.views.hooks.sync_repository_task', mock.MagicMock())
     def test_github_sync_on_push_event(self, trigger_build):
         """Sync if the webhook doesn't have the create/delete events, but we receive a push event with created/deleted."""
@@ -2647,23 +2628,6 @@ def test_gitlab_empty_token(self, trigger_build):
             GitLabWebhookView.invalid_payload_msg
         )
 
-    def test_gitlab_skip_token_validation(self, trigger_build):
-        client = APIClient()
-        Integration.objects.filter(pk=self.gitlab_integration.pk).update(secret=None)
-        headers = {
-            GITLAB_TOKEN_HEADER: 'skipped',
-        }
-        resp = client.post(
-            reverse(
-                'api_webhook_gitlab',
-                kwargs={'project_slug': self.project.slug}
-            ),
-            {'object_kind': 'pull_request'},
-            format='json',
-            headers=headers,
-        )
-        self.assertEqual(resp.status_code, 200)
-
     @mock.patch('readthedocs.core.utils.trigger_build')
     def test_gitlab_merge_request_open_event(self, trigger_build, core_trigger_build):
         client = APIClient()
@@ -3248,9 +3212,7 @@ def test_webhook_build_another_branch(self, trigger_build):
         self.assertTrue(resp.data['build_triggered'])
         self.assertEqual(resp.data['versions'], ['v1.0'])
 
-    @mock.patch("readthedocs.api.v2.views.integrations.timezone.now")
-    def test_deprecate_webhooks_without_a_secret(self, now, trigger_build):
-        now.return_value = datetime.datetime(2024, 1, 31, tzinfo=datetime.timezone.utc)
+    def test_dont_allow_webhooks_without_a_secret(self, trigger_build):
         client = APIClient()
 
         Integration.objects.filter(pk=self.github_integration.pk).update(secret=None)