From 9123545cc5c7b60f707f680b02ae8c457ff486e4 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Wed, 25 Jan 2023 15:12:03 -0500 Subject: [PATCH 1/4] Remove custom CORS logic --- readthedocs/core/signals.py | 79 +++---------------- .../rtd_tests/tests/test_middleware.py | 39 ++------- 2 files changed, 16 insertions(+), 102 deletions(-) diff --git a/readthedocs/core/signals.py b/readthedocs/core/signals.py index bf31ea957b7..2a0fa7b904c 100644 --- a/readthedocs/core/signals.py +++ b/readthedocs/core/signals.py @@ -3,7 +3,7 @@ import requests import structlog from allauth.account.signals import email_confirmed -from corsheaders import signals +from corsheaders.signals import check_request_enabled from django.conf import settings from django.db.models.signals import pre_delete from django.dispatch import Signal, receiver @@ -12,20 +12,19 @@ from simple_history.signals import pre_create_historical_record from readthedocs.analytics.utils import get_client_ip -from readthedocs.builds.models import Version from readthedocs.core.models import UserProfile -from readthedocs.core.unresolver import unresolve from readthedocs.organizations.models import Organization from readthedocs.projects.models import Project log = structlog.get_logger(__name__) ALLOWED_URLS = [ - '/api/v2/footer_html', - '/api/v2/search', - '/api/v2/docsearch', - '/api/v2/embed', - '/api/v3/embed', + "/api/v2/footer_html", + "/api/v2/search", + "/api/v2/docsearch", + "/api/v2/embed", + "/api/v3/embed", + "/api/v2/sustainability", ] webhook_github = Signal() @@ -86,15 +85,7 @@ def process_email_confirmed(request, email_address, **kwargs): log.exception("Unknown error subscribing user to newsletter.") -def _has_donate_app(): - """ - Check if the current app has the sustainability API. - - This is a separate function so it's easy to mock. - """ - return 'readthedocsext.donate' in settings.INSTALLED_APPS - - +@receiver(check_request_enabled) def decide_if_cors(sender, request, **kwargs): # pylint: disable=unused-argument """ Decide whether a request should be given CORS access. @@ -103,64 +94,15 @@ def decide_if_cors(sender, request, **kwargs): # pylint: disable=unused-argumen * It's a safe HTTP method * The origin is in ALLOWED_URLS - * The URL is owned by the project that they are requesting data from - * The version is public - - .. note:: - - Requests from the sustainability API are always allowed - if the donate app is installed. :returns: `True` when a request should be given CORS access. """ if 'HTTP_ORIGIN' not in request.META or request.method not in SAFE_METHODS: return False - # Always allow the sustainability API, - # it's used only on .org to check for ad-free users. - if _has_donate_app() and request.path_info.startswith('/api/v2/sustainability'): - return True - - valid_url = None for url in ALLOWED_URLS: if request.path_info.startswith(url): - valid_url = url - break - - if valid_url: - url = request.GET.get('url') - if url: - unresolved = unresolve(url) - if unresolved is None: - # NOTE: Embed APIv3 now supports external sites. In that case - # ``unresolve()`` will return None and we want to allow it - # since the target is a public project. - return True - - project = unresolved.project - version_slug = unresolved.version.slug - else: - project_slug = request.GET.get('project', None) - version_slug = request.GET.get('version', None) - project = Project.objects.filter(slug=project_slug).first() - - if project and version_slug: - # This is from IsAuthorizedToViewVersion, - # we should abstract is a bit perhaps? - is_public = ( - Version.objects - .public( - project=project, - only_active=False, - ) - .filter(slug=version_slug) - .exists() - ) - # Allowing CORS on public versions, - # since they are already public. - if is_public: - return True - + return True return False @@ -196,6 +138,3 @@ def add_extra_historical_fields(sender, **kwargs): if request: history_instance.extra_history_ip = get_client_ip(request) history_instance.extra_history_browser = request.headers.get('User-Agent') - - -signals.check_request_enabled.connect(decide_if_cors) diff --git a/readthedocs/rtd_tests/tests/test_middleware.py b/readthedocs/rtd_tests/tests/test_middleware.py index 3819ee6b9f5..208cd7b1113 100644 --- a/readthedocs/rtd_tests/tests/test_middleware.py +++ b/readthedocs/rtd_tests/tests/test_middleware.py @@ -1,5 +1,3 @@ -from unittest import mock - from corsheaders.middleware import ( ACCESS_CONTROL_ALLOW_CREDENTIALS, ACCESS_CONTROL_ALLOW_ORIGIN, @@ -80,7 +78,7 @@ def test_allow_linked_domain_from_public_version(self): self.assertIn(ACCESS_CONTROL_ALLOW_ORIGIN, resp) self.assertNotIn(ACCESS_CONTROL_ALLOW_CREDENTIALS, resp) - def test_dont_allow_linked_domain_from_private_version(self): + def test_linked_domain_from_private_version(self): self.version.privacy_level = PRIVATE self.version.save() request = self.factory.get( @@ -89,7 +87,7 @@ def test_dont_allow_linked_domain_from_private_version(self): HTTP_ORIGIN='http://my.valid.domain', ) resp = self.middleware.process_response(request, {}) - self.assertNotIn(ACCESS_CONTROL_ALLOW_ORIGIN, resp) + self.assertIn(ACCESS_CONTROL_ALLOW_ORIGIN, resp) self.assertNotIn(ACCESS_CONTROL_ALLOW_CREDENTIALS, resp) def test_allowed_api_public_version_from_another_domain(self): @@ -111,7 +109,7 @@ def test_allowed_api_public_version_from_another_domain(self): self.assertIn(ACCESS_CONTROL_ALLOW_ORIGIN, resp) self.assertNotIn(ACCESS_CONTROL_ALLOW_CREDENTIALS, resp) - def test_not_allowed_api_private_version_from_another_domain(self): + def test_api_private_version_from_another_domain(self): self.version.privacy_level = PRIVATE self.version.save() request = self.factory.get( @@ -120,7 +118,7 @@ def test_not_allowed_api_private_version_from_another_domain(self): HTTP_ORIGIN='http://docs.another.domain', ) resp = self.middleware.process_response(request, {}) - self.assertNotIn(ACCESS_CONTROL_ALLOW_ORIGIN, resp) + self.assertIn(ACCESS_CONTROL_ALLOW_ORIGIN, resp) self.assertNotIn(ACCESS_CONTROL_ALLOW_CREDENTIALS, resp) request = self.factory.get( @@ -129,7 +127,7 @@ def test_not_allowed_api_private_version_from_another_domain(self): HTTP_ORIGIN='http://another.valid.domain', ) resp = self.middleware.process_response(request, {}) - self.assertNotIn(ACCESS_CONTROL_ALLOW_ORIGIN, resp) + self.assertIn(ACCESS_CONTROL_ALLOW_ORIGIN, resp) self.assertNotIn(ACCESS_CONTROL_ALLOW_CREDENTIALS, resp) def test_valid_subproject(self): @@ -157,7 +155,7 @@ def test_embed_api_private_version_linked_domain(self): HTTP_ORIGIN='http://my.valid.domain', ) resp = self.middleware.process_response(request, {}) - self.assertNotIn(ACCESS_CONTROL_ALLOW_ORIGIN, resp) + self.assertIn(ACCESS_CONTROL_ALLOW_ORIGIN, resp) self.assertNotIn(ACCESS_CONTROL_ALLOW_CREDENTIALS, resp) def test_embed_api_external_url(self): @@ -177,9 +175,7 @@ def test_embed_api_external_url(self): resp = self.middleware.process_response(request, {}) self.assertIn("Access-Control-Allow-Origin", resp) - @mock.patch('readthedocs.core.signals._has_donate_app') - def test_sustainability_endpoint_allways_allowed(self, has_donate_app): - has_donate_app.return_value = True + def test_sustainability_endpoint_allways_allowed(self): request = self.factory.get( '/api/v2/sustainability/', {'project': self.project.slug, 'active': True, 'version': self.version.slug}, @@ -198,27 +194,6 @@ def test_sustainability_endpoint_allways_allowed(self, has_donate_app): self.assertIn(ACCESS_CONTROL_ALLOW_ORIGIN, resp) self.assertNotIn(ACCESS_CONTROL_ALLOW_CREDENTIALS, resp) - @mock.patch('readthedocs.core.signals._has_donate_app') - def test_sustainability_endpoint_no_ext(self, has_donate_app): - has_donate_app.return_value = False - request = self.factory.get( - '/api/v2/sustainability/', - {'project': self.project.slug, 'active': True, 'version': self.version.slug}, - HTTP_ORIGIN='http://invalid.domain', - ) - resp = self.middleware.process_response(request, {}) - self.assertNotIn(ACCESS_CONTROL_ALLOW_ORIGIN, resp) - self.assertNotIn(ACCESS_CONTROL_ALLOW_CREDENTIALS, resp) - - request = self.factory.get( - '/api/v2/sustainability/', - {'project': self.project.slug, 'active': True, 'version': self.version.slug}, - HTTP_ORIGIN='http://my.valid.domain', - ) - resp = self.middleware.process_response(request, {}) - self.assertNotIn(ACCESS_CONTROL_ALLOW_ORIGIN, resp) - self.assertNotIn(ACCESS_CONTROL_ALLOW_CREDENTIALS, resp) - def test_apiv2_endpoint_not_allowed(self): request = self.factory.get( '/api/v2/version/', From aa46b74bfa58d93cb1a70b9403d8e277428a39d5 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Wed, 25 Jan 2023 15:22:51 -0500 Subject: [PATCH 2/4] Remove from proxito middleware too --- readthedocs/settings/proxito/base.py | 1 + 1 file changed, 1 insertion(+) diff --git a/readthedocs/settings/proxito/base.py b/readthedocs/settings/proxito/base.py index 9c3867c4c88..3f289fc054d 100644 --- a/readthedocs/settings/proxito/base.py +++ b/readthedocs/settings/proxito/base.py @@ -37,6 +37,7 @@ def MIDDLEWARE(self): # noqa classes.append('readthedocs.proxito.middleware.ProxitoMiddleware') middleware_to_remove = ( + 'corsheaders.middleware.CorsMiddleware', 'csp.middleware.CSPMiddleware', 'django.middleware.clickjacking.XFrameOptionsMiddleware', ) From 3ecbf352d454a4875041ad710317d76b43e1e901 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Wed, 25 Jan 2023 16:11:17 -0500 Subject: [PATCH 3/4] Use the list of URLs instead --- readthedocs/core/signals.py | 32 ---------------------------- readthedocs/settings/base.py | 9 ++++++++ readthedocs/settings/proxito/base.py | 4 ++++ 3 files changed, 13 insertions(+), 32 deletions(-) diff --git a/readthedocs/core/signals.py b/readthedocs/core/signals.py index 2a0fa7b904c..8ea16c56ad9 100644 --- a/readthedocs/core/signals.py +++ b/readthedocs/core/signals.py @@ -3,11 +3,9 @@ import requests import structlog from allauth.account.signals import email_confirmed -from corsheaders.signals import check_request_enabled from django.conf import settings from django.db.models.signals import pre_delete from django.dispatch import Signal, receiver -from rest_framework.permissions import SAFE_METHODS from simple_history.models import HistoricalRecords from simple_history.signals import pre_create_historical_record @@ -18,15 +16,6 @@ log = structlog.get_logger(__name__) -ALLOWED_URLS = [ - "/api/v2/footer_html", - "/api/v2/search", - "/api/v2/docsearch", - "/api/v2/embed", - "/api/v3/embed", - "/api/v2/sustainability", -] - webhook_github = Signal() webhook_gitlab = Signal() webhook_bitbucket = Signal() @@ -85,27 +74,6 @@ def process_email_confirmed(request, email_address, **kwargs): log.exception("Unknown error subscribing user to newsletter.") -@receiver(check_request_enabled) -def decide_if_cors(sender, request, **kwargs): # pylint: disable=unused-argument - """ - Decide whether a request should be given CORS access. - - Allow the request if: - - * It's a safe HTTP method - * The origin is in ALLOWED_URLS - - :returns: `True` when a request should be given CORS access. - """ - if 'HTTP_ORIGIN' not in request.META or request.method not in SAFE_METHODS: - return False - - for url in ALLOWED_URLS: - if request.path_info.startswith(url): - return True - return False - - @receiver(pre_delete, sender=settings.AUTH_USER_MODEL) def delete_projects_and_organizations(sender, instance, *args, **kwargs): """ diff --git a/readthedocs/settings/base.py b/readthedocs/settings/base.py index 07aa98cf9d9..6be59ac5949 100644 --- a/readthedocs/settings/base.py +++ b/readthedocs/settings/base.py @@ -749,6 +749,15 @@ def DOCKER_LIMITS(self): 'HEAD', ] + CORS_URLS_ALLOW_ALL_REGEX = [ + r"^/api/v2/footer_html", + r"^/api/v2/search", + r"^/api/v2/docsearch", + r"^/api/v2/embed", + r"^/api/v3/embed", + r"^/api/v2/sustainability", + ] + # RTD Settings ALLOW_PRIVATE_REPOS = False DEFAULT_PRIVACY_LEVEL = 'public' diff --git a/readthedocs/settings/proxito/base.py b/readthedocs/settings/proxito/base.py index 3f289fc054d..aedf2578f4a 100644 --- a/readthedocs/settings/proxito/base.py +++ b/readthedocs/settings/proxito/base.py @@ -20,6 +20,9 @@ class CommunityProxitoSettingsMixin: # As 'Lax' breaks when the page is embedded in an iframe. SESSION_COOKIE_SAMESITE = None + # We don't need or want to allow cross site requests in proxito. + CORS_URLS_ALLOW_ALL_REGEX = [] + @property def DATABASES(self): # This keeps connections to the DB alive, @@ -37,6 +40,7 @@ def MIDDLEWARE(self): # noqa classes.append('readthedocs.proxito.middleware.ProxitoMiddleware') middleware_to_remove = ( + # We don't need or want to allow cross site requests in proxito. 'corsheaders.middleware.CorsMiddleware', 'csp.middleware.CSPMiddleware', 'django.middleware.clickjacking.XFrameOptionsMiddleware', From fdfcf4d38610ea5f9e5f3913fe76b0ad545c624b Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Tue, 31 Jan 2023 17:28:18 -0500 Subject: [PATCH 4/4] Update readthedocs/settings/base.py Co-authored-by: Eric Holscher <25510+ericholscher@users.noreply.github.com> --- readthedocs/settings/base.py | 1 + 1 file changed, 1 insertion(+) diff --git a/readthedocs/settings/base.py b/readthedocs/settings/base.py index 6be59ac5949..f2f3921323b 100644 --- a/readthedocs/settings/base.py +++ b/readthedocs/settings/base.py @@ -749,6 +749,7 @@ def DOCKER_LIMITS(self): 'HEAD', ] + # URLs to allow CORS to read from unauthed. CORS_URLS_ALLOW_ALL_REGEX = [ r"^/api/v2/footer_html", r"^/api/v2/search",