From 54ced289143c2f7c6b82cd7f7e933e806833a3e4 Mon Sep 17 00:00:00 2001 From: Anthony Johnson Date: Tue, 4 Oct 2016 23:22:54 -0700 Subject: [PATCH] Fix incoming payload parsing, cleanup, and add logging --- readthedocs/core/views/hooks.py | 71 +++++++++------- readthedocs/restapi/views/integrations.py | 83 +++++++++---------- readthedocs/rtd_tests/tests/test_api.py | 35 ++++++++ .../rtd_tests/tests/test_post_commit_hooks.py | 27 +++++- 4 files changed, 136 insertions(+), 80 deletions(-) diff --git a/readthedocs/core/views/hooks.py b/readthedocs/core/views/hooks.py index ae9082bff38..34cea5fb36c 100644 --- a/readthedocs/core/views/hooks.py +++ b/readthedocs/core/views/hooks.py @@ -141,20 +141,24 @@ def github_build(request): This will search for projects matching either a stripped down HTTP or SSH URL. The search is error prone, use the API v2 webhook for new webhooks. + + Old webhooks may not have specified the content type to POST with, and + therefore can use ``application/x-www-form-urlencoded`` to pass the JSON + payload. More information on the API docs here: + https://developer.github.com/webhooks/creating/#content-type """ if request.method == 'POST': try: - data = json.loads(request.body) - except (ValueError, TypeError): - log.error('Invalid GitHub webhook payload', exc_info=True) - return HttpResponse('Invalid request', status=400) - http_url = data['repository']['url'] - http_search_url = http_url.replace('http://', '').replace('https://', '') - ssh_url = data['repository']['ssh_url'] - ssh_search_url = ssh_url.replace('git@', '').replace('.git', '') - try: + if request.META['CONTENT_TYPE'] == 'application/x-www-form-urlencoded': + data = json.loads(request.POST.get('payload')) + else: + data = json.loads(request.body) + http_url = data['repository']['url'] + http_search_url = http_url.replace('http://', '').replace('https://', '') + ssh_url = data['repository']['ssh_url'] + ssh_search_url = ssh_url.replace('git@', '').replace('.git', '') branches = [data['ref'].replace('refs/heads/', '')] - except KeyError: + except (ValueError, TypeError, KeyError): log.error('Invalid GitHub webhook payload', exc_info=True) return HttpResponse('Invalid request', status=400) try: @@ -178,7 +182,7 @@ def github_build(request): log.error('Project match not found: url=%s', http_search_url) return HttpResponseNotFound('Project not found') else: - return HttpResponse('Method not allowed', status=405) + return HttpResponse('Method not allowed, POST is required', status=405) @csrf_exempt @@ -191,12 +195,12 @@ def gitlab_build(request): if request.method == 'POST': try: data = json.loads(request.body) - except (ValueError, TypeError): + url = data['project']['http_url'] + search_url = re.sub(r'^https?://(.*?)(?:\.git|)$', '\\1', url) + branches = [data['ref'].replace('refs/heads/', '')] + except (ValueError, TypeError, KeyError): log.error('Invalid GitLab webhook payload', exc_info=True) return HttpResponse('Invalid request', status=400) - url = data['project']['http_url'] - search_url = re.sub(r'^https?://(.*?)(?:\.git|)$', '\\1', url) - branches = [data['ref'].replace('refs/heads/', '')] log.info( 'GitLab webhook search: url=%s branches=%s', search_url, @@ -209,49 +213,52 @@ def gitlab_build(request): log.error('Project match not found: url=%s', search_url) return HttpResponseNotFound('Project match not found') else: - return HttpResponse('Method not allowed', status=405) + return HttpResponse('Method not allowed, POST is required', status=405) @csrf_exempt def bitbucket_build(request): """Consume webhooks from multiple versions of Bitbucket's API + New webhooks are set up with v2, but v1 webhooks will still point to this + endpoint. There are also "services" that point here and submit + ``application/x-www-form-urlencoded`` data. + API v1 https://confluence.atlassian.com/bitbucket/events-resources-296095220.html API v2 https://confluence.atlassian.com/bitbucket/event-payloads-740262817.html#EventPayloads-Push + + Services + https://confluence.atlassian.com/bitbucket/post-service-management-223216518.html """ if request.method == 'POST': try: - data = json.loads(request.body) - except (TypeError, ValueError): - log.error('Invalid Bitbucket webhook payload', exc_info=True) - return HttpResponse('Invalid request', status=400) + if request.META['CONTENT_TYPE'] == 'application/x-www-form-urlencoded': + data = json.loads(request.POST.get('payload')) + else: + data = json.loads(request.body) - version = 2 if request.META.get('HTTP_USER_AGENT') == 'Bitbucket-Webhooks/2.0' else 1 - if version == 1: - try: + version = 2 if request.META.get('HTTP_USER_AGENT') == 'Bitbucket-Webhooks/2.0' else 1 + if version == 1: branches = [commit.get('branch', '') for commit in data['commits']] repository = data['repository'] search_url = 'bitbucket.org{0}'.format( repository['absolute_url'].rstrip('/') ) - except KeyError: - log.error('Invalid Bitbucket webhook payload', exc_info=True) - return HttpResponse('Invalid request', status=400) - elif version == 2: - try: + elif version == 2: changes = data['push']['changes'] branches = [change['new']['name'] for change in changes] search_url = 'bitbucket.org/{0}'.format( data['repository']['full_name'] ) - except KeyError: - log.error('Invalid Bitbucket webhook payload', exc_info=True) - return HttpResponse('Invalid request', status=400) + except (TypeError, ValueError, KeyError): + log.error('Invalid Bitbucket webhook payload', exc_info=True) + return HttpResponse('Invalid request', status=400) + log.info( 'Bitbucket webhook search: url=%s branches=%s', search_url, @@ -265,7 +272,7 @@ def bitbucket_build(request): log.error('Project match not found: url=%s', search_url) return HttpResponseNotFound('Project match not found') else: - return HttpResponse('Method not allowed', status=405) + return HttpResponse('Method not allowed, POST is required', status=405) @csrf_exempt diff --git a/readthedocs/restapi/views/integrations.py b/readthedocs/restapi/views/integrations.py index f84083c7904..725337a6aae 100644 --- a/readthedocs/restapi/views/integrations.py +++ b/readthedocs/restapi/views/integrations.py @@ -1,3 +1,5 @@ +import logging + from rest_framework import permissions from rest_framework.views import APIView from rest_framework.renderers import JSONRenderer @@ -13,6 +15,8 @@ from readthedocs.projects.models import Project +log = logging.getLogger(__name__) + GITHUB_PUSH = 'push' GITLAB_PUSH = 'push' BITBUCKET_PUSH = 'repo:push' @@ -27,23 +31,41 @@ def post(self, request, project_slug, format=None): try: project = Project.objects.get(slug=project_slug) resp = self.handle_webhook(request, project, request.data) + if resp is None: + log.info('Unhandled webhook event') + resp = {'detail': 'Unhandled webhook event'} except Project.DoesNotExist: raise Http404('Project does not exist') return Response(resp) def handle_webhook(self, request, project, data=None): - """Handle webhook payload + """Handle webhook payload""" + raise NotImplementedError - If a build is triggered from this method, return a JSON response with - the following:: + def get_response_push(self, project, branches): + """Build branches on push events and return API response + + Return a JSON response with the following:: { "build_triggered": true, "project": "project_name", "versions": [...] } + + :param project: Project instance + :type project: Project + :param branches: List of branch names to build + :type branches: list(str) """ - raise NotImplementedError + to_build, not_building = build_branches(project, branches) + if not_building: + log.info('Skipping project branches: project=%s branches=%s', + project, branches) + triggered = True if to_build else False + return {'build_triggered': triggered, + 'project': project.slug, + 'versions': to_build} class GitHubWebhookView(WebhookMixin, APIView): @@ -60,14 +82,6 @@ class GitHubWebhookView(WebhookMixin, APIView): "ref": "branch-name", ... } - - If a build is triggered, this will return JSON with the following:: - - { - "build_triggered": true, - "project": "project_name", - "versions": [...] - } """ def handle_webhook(self, request, project, data=None): @@ -75,17 +89,12 @@ def handle_webhook(self, request, project, data=None): event = request.META.get('HTTP_X_GITHUB_EVENT', 'push') webhook_github.send(Project, project=project, data=data, event=event) # Handle push events and trigger builds - try: - branches = [request.data['ref'].replace('refs/heads/', '')] - except KeyError: - raise ParseError('Parameter "ref" is required') if event == GITHUB_PUSH: - to_build, not_building = build_branches(project, branches) - triggered = True if to_build else False - return {'build_triggered': triggered, - 'project': project.slug, - 'versions': to_build} - return {'build_triggered': False} + try: + branches = [request.data['ref'].replace('refs/heads/', '')] + return self.get_response_push(project, branches) + except KeyError: + raise ParseError('Parameter "ref" is required') class GitLabWebhookView(WebhookMixin, APIView): @@ -101,14 +110,6 @@ class GitLabWebhookView(WebhookMixin, APIView): "ref": "branch-name", ... } - - If a build is triggered, this will return JSON with the following:: - - { - "build_triggered": true, - "project": "project_name", - "versions": [...] - } """ def handle_webhook(self, request, project, data=None): @@ -116,17 +117,12 @@ def handle_webhook(self, request, project, data=None): event = data.get('object_kind', GITLAB_PUSH) webhook_gitlab.send(Project, project=project, data=data, event=event) # Handle push events and trigger builds - try: - branches = [request.data['ref'].replace('refs/heads/', '')] - except KeyError: - raise ParseError('Parameter "ref" is required') if event == GITLAB_PUSH: - to_build, not_building = build_branches(project, branches) - triggered = True if to_build else False - return {'build_triggered': triggered, - 'project': project.slug, - 'versions': to_build} - return {'build_triggered': False} + try: + branches = [request.data['ref'].replace('refs/heads/', '')] + return self.get_response_push(project, branches) + except KeyError: + raise ParseError('Parameter "ref" is required') class BitbucketWebhookView(WebhookMixin, APIView): @@ -162,11 +158,6 @@ def handle_webhook(self, request, project, data=None): changes = data['push']['changes'] branches = [change['new']['name'] for change in changes] + return self.get_response_push(project, branches) except KeyError: raise ParseError('Invalid request') - to_build, not_building = build_branches(project, branches) - triggered = True if to_build else False - return {'build_triggered': triggered, - 'project': project.slug, - 'versions': to_build} - return {'build_triggered': False} diff --git a/readthedocs/rtd_tests/tests/test_api.py b/readthedocs/rtd_tests/tests/test_api.py index 5ca860927fa..edc716449c7 100644 --- a/readthedocs/rtd_tests/tests/test_api.py +++ b/readthedocs/rtd_tests/tests/test_api.py @@ -324,6 +324,18 @@ def test_github_webhook(self, trigger_build): mock.call(force=True, version=mock.ANY, project=self.project) ]) + def test_github_invalid_webhook(self, trigger_build): + """GitHub webhook unhandled event""" + client = APIClient() + resp = client.post( + '/api/v2/webhook/github/{0}/'.format(self.project.slug), + {'foo': 'bar'}, + format='json', + HTTP_X_GITHUB_EVENT='pull_request', + ) + self.assertEqual(resp.status_code, 200) + self.assertEqual(resp.data['detail'], 'Unhandled webhook event') + def test_gitlab_webhook(self, trigger_build): """GitLab webhook API""" client = APIClient() @@ -344,6 +356,17 @@ def test_gitlab_webhook(self, trigger_build): mock.call(force=True, version=mock.ANY, project=self.project) ]) + def test_gitlab_invalid_webhook(self, trigger_build): + """GitLab webhook unhandled event""" + client = APIClient() + resp = client.post( + '/api/v2/webhook/gitlab/{0}/'.format(self.project.slug), + {'object_kind': 'pull_request'}, + format='json', + ) + self.assertEqual(resp.status_code, 200) + self.assertEqual(resp.data['detail'], 'Unhandled webhook event') + def test_bitbucket_webhook(self, trigger_build): """Bitbucket webhook API""" client = APIClient() @@ -379,3 +402,15 @@ def test_bitbucket_webhook(self, trigger_build): trigger_build.assert_has_calls([ mock.call(force=True, version=mock.ANY, project=self.project) ]) + + def test_bitbucket_invalid_webhook(self, trigger_build): + """Bitbucket webhook unhandled event""" + client = APIClient() + resp = client.post( + '/api/v2/webhook/bitbucket/{0}/'.format(self.project.slug), + {'foo': 'bar'}, + format='json', + HTTP_X_EVENT_KEY='pull_request' + ) + self.assertEqual(resp.status_code, 200) + self.assertEqual(resp.data['detail'], 'Unhandled webhook event') diff --git a/readthedocs/rtd_tests/tests/test_post_commit_hooks.py b/readthedocs/rtd_tests/tests/test_post_commit_hooks.py index 2c484c7f84f..96a9c90774f 100644 --- a/readthedocs/rtd_tests/tests/test_post_commit_hooks.py +++ b/readthedocs/rtd_tests/tests/test_post_commit_hooks.py @@ -1,9 +1,10 @@ -from django.test import TestCase import json import logging -import mock +from urllib import urlencode +import mock from django_dynamic_fixture import get +from django.test import TestCase from readthedocs.builds.models import Version from readthedocs.projects.models import Project @@ -209,6 +210,17 @@ def setUp(self): } } + def test_post_types(self): + """Ensure various POST formats""" + r = self.client.post('/github/', + data=json.dumps(self.payload), + content_type='application/json') + self.assertEqual(r.status_code, 200) + r = self.client.post('/github/', + data=urlencode({'payload': json.dumps(self.payload)}), + content_type='application/x-www-form-urlencoded') + self.assertEqual(r.status_code, 200) + def test_github_upper_case_repo(self): """ Test the github post commit hook will build properly with upper case @@ -404,6 +416,17 @@ def setUp(self): "user": "marcus" } + def test_post_types(self): + """Ensure various POST formats""" + r = self.client.post('/bitbucket/', + data=json.dumps(self.hg_payload), + content_type='application/json') + self.assertEqual(r.status_code, 200) + r = self.client.post('/bitbucket/', + data=urlencode({'payload': json.dumps(self.hg_payload)}), + content_type='application/x-www-form-urlencoded') + self.assertEqual(r.status_code, 200) + def test_bitbucket_post_commit(self): r = self.client.post('/bitbucket/', data=json.dumps(self.hg_payload), content_type='application/json')