Skip to content

Remove custom CORS logic #9945

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Feb 1, 2023
Merged

Remove custom CORS logic #9945

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9123545
Remove custom CORS logic
stsewd Jan 25, 2023
aa46b74
Remove from proxito middleware too
stsewd Jan 25, 2023
3ecbf35
Use the list of URLs instead
stsewd Jan 25, 2023
fdfcf4d
Update readthedocs/settings/base.py
stsewd Jan 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
93 changes: 0 additions & 93 deletions readthedocs/core/signals.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,31 +3,19 @@
import requests
import structlog
from allauth.account.signals import email_confirmed
from corsheaders import signals
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

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',
]

webhook_github = Signal()
webhook_gitlab = Signal()
webhook_bitbucket = Signal()
Expand Down Expand Up @@ -86,84 +74,6 @@ 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


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
* 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 False


@receiver(pre_delete, sender=settings.AUTH_USER_MODEL)
def delete_projects_and_organizations(sender, instance, *args, **kwargs):
"""
Expand Down Expand Up @@ -196,6 +106,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)
39 changes: 7 additions & 32 deletions readthedocs/rtd_tests/tests/test_middleware.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
from unittest import mock

from corsheaders.middleware import (
ACCESS_CONTROL_ALLOW_CREDENTIALS,
ACCESS_CONTROL_ALLOW_ORIGIN,
Expand Down Expand Up @@ -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(
Expand All @@ -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):
Expand All @@ -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(
Expand All @@ -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(
Expand All @@ -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):
Expand Down Expand Up @@ -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):
Expand All @@ -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},
Expand All @@ -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/',
Expand Down
9 changes: 9 additions & 0 deletions readthedocs/settings/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down
5 changes: 5 additions & 0 deletions readthedocs/settings/proxito/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 = []
Comment on lines +23 to +24
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removing the middleware should be enough, but just in case if we added it here we don't want to inherit the endpoints from the dashboard.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry to have to post a comment in a merged PR. I'm trying to understand what the security hardening was about to write a sentence or two in the newsletter. Which landed me here.

And that had me wondering that I cannot find CORS_URLS_ALLOW_ALL_REGEX in the documentation of django-cors-headers: https://pypi.org/project/django-cors-headers/

But I was looking at the wrong project.

We seem to be using this one: https://pypi.org/project/django-cors-headers/ - which hasn't been updated for 4 years and has 135 stars. django-cors-headers is cruising at over 4800 stars :)

It looks like it's been previously "saved by the bell" so to speak: #4694 - but maybe this is a good "tech debt" roadmap candidate.

oh yeah and that configuration setting is:

Specify a list of URL regex for which to allow all origins

So I guess that in all other cases, we are setting CORS headers to deny. I'll probably be asking how it works and what we changed :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we are using the forked version, but we should be able to migrate to the new version, see #8713, we just need to be careful making sure the settings from the fork match the settings from the new package.

I'll probably be asking how it works and what we changed :)

Before we were allowing cross site requests allowing credentials to be passed, now we are allowing cross site requests as before, but we don't allow credentials to be passed (cookies).


@property
def DATABASES(self):
# This keeps connections to the DB alive,
Expand All @@ -37,6 +40,8 @@ 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',
)
Expand Down