From 731b4cde4883b6aa65b9c03e46640e8d00907382 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 11 Sep 2019 15:46:54 +0200 Subject: [PATCH] Improve CORS configuration The goal of this commit is to allow access to our resources from different hosts: * everything coming from `*.readthedocs.io` * everything hitting any URL under `/api/` if it's safe method, no matter the host where the request comes from Compared with the previous implementation, this reduces the number of queries to our database since we are not checking that the host where the request was made is a known Domain for us. Besides, this opens access to `/api/v3` as well. --- readthedocs/core/signals.py | 57 ++----------------- .../rtd_tests/tests/test_middleware.py | 23 ++++++-- readthedocs/settings/base.py | 21 ++++--- 3 files changed, 36 insertions(+), 65 deletions(-) diff --git a/readthedocs/core/signals.py b/readthedocs/core/signals.py index a4f029c6b3e..7e8c81d4929 100644 --- a/readthedocs/core/signals.py +++ b/readthedocs/core/signals.py @@ -18,13 +18,6 @@ log = logging.getLogger(__name__) -WHITELIST_URLS = [ - '/api/v2/footer_html', - '/api/v2/search', - '/api/v2/docsearch', - '/api/v2/sustainability', -] - webhook_github = Signal(providing_args=['project', 'data', 'event']) webhook_gitlab = Signal(providing_args=['project', 'data', 'event']) webhook_bitbucket = Signal(providing_args=['project', 'data', 'event']) @@ -34,52 +27,14 @@ def decide_if_cors(sender, request, **kwargs): # pylint: disable=unused-argumen """ Decide whether a request should be given CORS access. - This checks that: - * The URL is whitelisted against our CORS-allowed domains - * The Domain exists in our database, and belongs to the project being queried. + This checks that the URL is under ``/api/`` and it's a safe method. - Returns True when a request should be given CORS access. + Returns ``True`` when a request should be given CORS access. """ - if 'HTTP_ORIGIN' not in request.META: - return False - host = urlparse(request.META['HTTP_ORIGIN']).netloc.split(':')[0] - - # Don't do domain checking for this API for now - if request.path_info.startswith('/api/v2/sustainability'): - return True - - # Don't do domain checking for APIv2 when the Domain is known - if request.path_info.startswith('/api/v2/') and request.method in SAFE_METHODS: - domain = Domain.objects.filter(domain__icontains=host) - if domain.exists(): - return True - - valid_url = False - for url in WHITELIST_URLS: - if request.path_info.startswith(url): - valid_url = True - break - - if valid_url: - project_slug = request.GET.get('project', None) - try: - project = Project.objects.get(slug=project_slug) - except Project.DoesNotExist: - log.warning( - 'Invalid project passed to domain. [%s:%s]', - project_slug, - host, - ) - return False - - domain = Domain.objects.filter( - Q(domain__icontains=host), - Q(project=project) | Q(project__subprojects__child=project), - ) - if domain.exists(): - return True - - return False + return all([ + request.method in SAFE_METHODS, + request.path_info.startswith('/api/'), + ]) @receiver(pre_delete, sender=settings.AUTH_USER_MODEL) diff --git a/readthedocs/rtd_tests/tests/test_middleware.py b/readthedocs/rtd_tests/tests/test_middleware.py index 5e33a1e4d55..a15ae81b8ce 100644 --- a/readthedocs/rtd_tests/tests/test_middleware.py +++ b/readthedocs/rtd_tests/tests/test_middleware.py @@ -185,7 +185,7 @@ def setUp(self): ) self.domain = get(Domain, domain='my.valid.domain', project=self.project) - def test_proper_domain(self): + def test_known_domain(self): request = self.factory.get( self.url, {'project': self.project.slug}, @@ -194,11 +194,24 @@ def test_proper_domain(self): resp = self.middleware.process_response(request, {}) self.assertIn('Access-Control-Allow-Origin', resp) - def test_invalid_domain(self): + def test_unknown_domain(self): + """ + Test accessing the API from a domain that it's not registered in our DB. + + Only the endpoints under ``/api/`` should work. + """ request = self.factory.get( self.url, {'project': self.project.slug}, - HTTP_ORIGIN='http://invalid.domain', + HTTP_ORIGIN='http://unknown.domain', + ) + resp = self.middleware.process_response(request, {}) + self.assertIn('Access-Control-Allow-Origin', resp) + + request = self.factory.get( + '/dashboard/', + {'project': self.project.slug}, + HTTP_ORIGIN='http://unknown.domain', ) resp = self.middleware.process_response(request, {}) self.assertNotIn('Access-Control-Allow-Origin', resp) @@ -227,15 +240,15 @@ def test_apiv2_endpoint_allowed(self): resp = self.middleware.process_response(request, {}) self.assertIn('Access-Control-Allow-Origin', resp) - def test_apiv2_endpoint_not_allowed(self): request = self.factory.get( '/api/v2/version/', {'project__slug': self.project.slug, 'active': True}, HTTP_ORIGIN='http://invalid.domain', ) resp = self.middleware.process_response(request, {}) - self.assertNotIn('Access-Control-Allow-Origin', resp) + self.assertIn('Access-Control-Allow-Origin', resp) + def test_apiv2_method_not_allowed(self): # POST is not allowed request = self.factory.post( '/api/v2/version/', diff --git a/readthedocs/settings/base.py b/readthedocs/settings/base.py index 6fafb4d96b0..39d89667e3d 100644 --- a/readthedocs/settings/base.py +++ b/readthedocs/settings/base.py @@ -403,16 +403,19 @@ def USE_PROMOS(self): # noqa r'^http://(.+)\.readthedocs\.io$', r'^https://(.+)\.readthedocs\.io$', ) - # So people can post to their accounts + + # We need to allow credentials (cookies) to identify the user and remove ads + # if it's Gold member. Note: in Django 2.1 the ``SESSION_COOKIE_SAMESITE`` + # setting was added, set to 'Lax' by default, which will prevent Django's + # session cookie being sent cross-domain. Change it to None to bypass this + # security restriction. CORS_ALLOW_CREDENTIALS = True - CORS_ALLOW_HEADERS = ( - 'x-requested-with', - 'content-type', - 'accept', - 'origin', - 'authorization', - 'x-csrftoken' - ) + + CORS_ALLOW_METHODS = [ + 'GET', + 'OPTIONS', + 'HEAD', + ] # RTD Settings REPO_LOCK_SECONDS = 30