Skip to content

Use new maintained django-cors-headers package #10000

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 2 commits into from
Feb 9, 2023
Merged

Use new maintained django-cors-headers package #10000

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9859bf7
Use new maintained django-cors-headers package
stsewd Feb 7, 2023
4500844
Update settings and tests
stsewd Feb 8, 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
111 changes: 46 additions & 65 deletions readthedocs/rtd_tests/tests/test_middleware.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
from corsheaders.middleware import (
ACCESS_CONTROL_ALLOW_CREDENTIALS,
ACCESS_CONTROL_ALLOW_ORIGIN,
CorsMiddleware,
)
from django.conf import settings
from django.http import HttpResponse
Expand All @@ -25,8 +24,6 @@
class TestCORSMiddleware(TestCase):

def setUp(self):
self.factory = RequestFactory()
self.middleware = CorsMiddleware()
self.url = '/api/v2/search'
self.owner = create_user(username='owner', password='test')
self.project = get(
Expand Down Expand Up @@ -69,66 +66,60 @@ def setUp(self):
)

def test_allow_linked_domain_from_public_version(self):
request = self.factory.get(
resp = self.client.get(
self.url,
{'project': self.project.slug, 'version': self.version.slug},
HTTP_ORIGIN='http://my.valid.domain',
)
resp = self.middleware.process_response(request, {})
self.assertIn(ACCESS_CONTROL_ALLOW_ORIGIN, resp)
self.assertNotIn(ACCESS_CONTROL_ALLOW_CREDENTIALS, resp)
self.assertIn(ACCESS_CONTROL_ALLOW_ORIGIN, resp.headers)
self.assertNotIn(ACCESS_CONTROL_ALLOW_CREDENTIALS, resp.headers)

def test_linked_domain_from_private_version(self):
self.version.privacy_level = PRIVATE
self.version.save()
request = self.factory.get(
resp = self.client.get(
self.url,
{'project': self.project.slug, 'version': self.version.slug},
HTTP_ORIGIN='http://my.valid.domain',
)
resp = self.middleware.process_response(request, {})
self.assertIn(ACCESS_CONTROL_ALLOW_ORIGIN, resp)
self.assertNotIn(ACCESS_CONTROL_ALLOW_CREDENTIALS, resp)
self.assertIn(ACCESS_CONTROL_ALLOW_ORIGIN, resp.headers)
self.assertNotIn(ACCESS_CONTROL_ALLOW_CREDENTIALS, resp.headers)

def test_allowed_api_public_version_from_another_domain(self):
request = self.factory.get(
resp = self.client.get(
self.url,
{'project': self.project.slug, 'version': self.version.slug},
HTTP_ORIGIN='http://docs.another.domain',
)
resp = self.middleware.process_response(request, {})
self.assertIn(ACCESS_CONTROL_ALLOW_ORIGIN, resp)
self.assertNotIn(ACCESS_CONTROL_ALLOW_CREDENTIALS, resp)
self.assertIn(ACCESS_CONTROL_ALLOW_ORIGIN, resp.headers)
self.assertNotIn(ACCESS_CONTROL_ALLOW_CREDENTIALS, resp.headers)

request = self.factory.get(
resp = self.client.get(
self.url,
{'project': self.project.slug, 'version': self.version.slug},
HTTP_ORIGIN='http://another.valid.domain',
)
resp = self.middleware.process_response(request, {})
self.assertIn(ACCESS_CONTROL_ALLOW_ORIGIN, resp)
self.assertNotIn(ACCESS_CONTROL_ALLOW_CREDENTIALS, resp)
self.assertIn(ACCESS_CONTROL_ALLOW_ORIGIN, resp.headers)
self.assertNotIn(ACCESS_CONTROL_ALLOW_CREDENTIALS, resp.headers)

def test_api_private_version_from_another_domain(self):
self.version.privacy_level = PRIVATE
self.version.save()
request = self.factory.get(
resp = self.client.get(
self.url,
{'project': self.project.slug, 'version': self.version.slug},
HTTP_ORIGIN='http://docs.another.domain',
)
resp = self.middleware.process_response(request, {})
self.assertIn(ACCESS_CONTROL_ALLOW_ORIGIN, resp)
self.assertNotIn(ACCESS_CONTROL_ALLOW_CREDENTIALS, resp)
self.assertIn(ACCESS_CONTROL_ALLOW_ORIGIN, resp.headers)
self.assertNotIn(ACCESS_CONTROL_ALLOW_CREDENTIALS, resp.headers)

request = self.factory.get(
resp = self.client.get(
self.url,
{'project': self.project.slug, 'version': self.version.slug},
HTTP_ORIGIN='http://another.valid.domain',
)
resp = self.middleware.process_response(request, {})
self.assertIn(ACCESS_CONTROL_ALLOW_ORIGIN, resp)
self.assertNotIn(ACCESS_CONTROL_ALLOW_CREDENTIALS, resp)
self.assertIn(ACCESS_CONTROL_ALLOW_ORIGIN, resp.headers)
self.assertNotIn(ACCESS_CONTROL_ALLOW_CREDENTIALS, resp.headers)

def test_valid_subproject(self):
self.assertTrue(
Expand All @@ -137,102 +128,92 @@ def test_valid_subproject(self):
subprojects__child=self.subproject,
).exists(),
)
request = self.factory.get(
resp = self.client.get(
self.url,
{'project': self.project.slug, 'version': self.version.slug},
HTTP_ORIGIN='http://my.valid.domain',
)
resp = self.middleware.process_response(request, {})
self.assertIn(ACCESS_CONTROL_ALLOW_ORIGIN, resp)
self.assertNotIn(ACCESS_CONTROL_ALLOW_CREDENTIALS, resp)
self.assertIn(ACCESS_CONTROL_ALLOW_ORIGIN, resp.headers)
self.assertNotIn(ACCESS_CONTROL_ALLOW_CREDENTIALS, resp.headers)

def test_embed_api_private_version_linked_domain(self):
self.version.privacy_level = PRIVATE
self.version.save()
request = self.factory.get(
resp = self.client.get(
'/api/v2/embed/',
{'project': self.project.slug, 'version': self.version.slug},
HTTP_ORIGIN='http://my.valid.domain',
)
resp = self.middleware.process_response(request, {})
self.assertIn(ACCESS_CONTROL_ALLOW_ORIGIN, resp)
self.assertNotIn(ACCESS_CONTROL_ALLOW_CREDENTIALS, resp)
self.assertIn(ACCESS_CONTROL_ALLOW_ORIGIN, resp.headers)
self.assertNotIn(ACCESS_CONTROL_ALLOW_CREDENTIALS, resp.headers)

def test_embed_api_external_url(self):
request = self.factory.get(
resp = self.client.get(
"/api/v2/embed/",
{"url": "https://pip.readthedocs.io/en/latest/index.hml"},
HTTP_ORIGIN="http://my.valid.domain",
)
resp = self.middleware.process_response(request, {})
self.assertIn("Access-Control-Allow-Origin", resp)
self.assertIn("Access-Control-Allow-Origin", resp.headers)

request = self.factory.get(
resp = self.client.get(
"/api/v2/embed/",
{"url": "https://docs.example.com/en/latest/index.hml"},
HTTP_ORIGIN="http://my.valid.domain",
)
resp = self.middleware.process_response(request, {})
self.assertIn("Access-Control-Allow-Origin", resp)
self.assertIn("Access-Control-Allow-Origin", resp.headers)

def test_sustainability_endpoint_allways_allowed(self):
request = self.factory.get(
resp = self.client.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.assertIn(ACCESS_CONTROL_ALLOW_ORIGIN, resp)
self.assertNotIn(ACCESS_CONTROL_ALLOW_CREDENTIALS, resp)
self.assertIn(ACCESS_CONTROL_ALLOW_ORIGIN, resp.headers)
self.assertNotIn(ACCESS_CONTROL_ALLOW_CREDENTIALS, resp.headers)

request = self.factory.get(
resp = self.client.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.assertIn(ACCESS_CONTROL_ALLOW_ORIGIN, resp)
self.assertNotIn(ACCESS_CONTROL_ALLOW_CREDENTIALS, resp)
self.assertIn(ACCESS_CONTROL_ALLOW_ORIGIN, resp.headers)
self.assertNotIn(ACCESS_CONTROL_ALLOW_CREDENTIALS, resp.headers)

def test_apiv2_endpoint_not_allowed(self):
request = self.factory.get(
resp = self.client.get(
'/api/v2/version/',
{'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)
self.assertNotIn(ACCESS_CONTROL_ALLOW_ORIGIN, resp.headers)
self.assertNotIn(ACCESS_CONTROL_ALLOW_CREDENTIALS, resp.headers)

# This also doesn't work on registered domains.
request = self.factory.get(
resp = self.client.get(
'/api/v2/version/',
{'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)
self.assertNotIn(ACCESS_CONTROL_ALLOW_ORIGIN, resp.headers)
self.assertNotIn(ACCESS_CONTROL_ALLOW_CREDENTIALS, resp.headers)

# Or from our public domain.
request = self.factory.get(
resp = self.client.get(
'/api/v2/version/',
{'project': self.project.slug, 'active': True, 'version': self.version.slug},
HTTP_ORIGIN='http://docs.readthedocs.io/',
)
resp = self.middleware.process_response(request, {})
self.assertNotIn(ACCESS_CONTROL_ALLOW_ORIGIN, resp)
self.assertNotIn(ACCESS_CONTROL_ALLOW_CREDENTIALS, resp)
self.assertNotIn(ACCESS_CONTROL_ALLOW_ORIGIN, resp.headers)
self.assertNotIn(ACCESS_CONTROL_ALLOW_CREDENTIALS, resp.headers)

# POST is not allowed
request = self.factory.post(
resp = self.client.post(
'/api/v2/version/',
{'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)
self.assertNotIn(ACCESS_CONTROL_ALLOW_ORIGIN, resp.headers)
self.assertNotIn(ACCESS_CONTROL_ALLOW_CREDENTIALS, resp.headers)


class TestSessionMiddleware(TestCase):
Expand Down
43 changes: 26 additions & 17 deletions readthedocs/settings/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
# pylint: disable=missing-docstring

import os
import re
import subprocess
import socket

Expand All @@ -9,6 +10,7 @@
from celery.schedules import crontab

from readthedocs.core.logs import shared_processors
from corsheaders.defaults import default_headers
from readthedocs.core.settings import Settings


Expand Down Expand Up @@ -277,14 +279,14 @@ def MIDDLEWARE(self):
'readthedocs.core.middleware.NullCharactersMiddleware',
'readthedocs.core.middleware.ReadTheDocsSessionMiddleware',
'django.middleware.locale.LocaleMiddleware',
'corsheaders.middleware.CorsMiddleware',
'django.middleware.common.CommonMiddleware',
'django.middleware.security.SecurityMiddleware',
'django.middleware.csrf.CsrfViewMiddleware',
'django.middleware.clickjacking.XFrameOptionsMiddleware',
'django.contrib.auth.middleware.AuthenticationMiddleware',
'django.contrib.messages.middleware.MessageMiddleware',
'dj_pagination.middleware.PaginationMiddleware',
'corsheaders.middleware.CorsMiddleware',
'csp.middleware.CSPMiddleware',
'readthedocs.core.middleware.ReferrerPolicyMiddleware',
'simple_history.middleware.HistoryRequestMiddleware',
Expand Down Expand Up @@ -734,15 +736,17 @@ def DOCKER_LIMITS(self):
# users to CSRF attacks. The sustainability API is the only view that requires
# cookies to be send cross-site, we override that for that view only.
CORS_ALLOW_CREDENTIALS = False
CORS_ALLOW_HEADERS = (
'x-requested-with',
'content-type',
'accept',
'origin',
'authorization',

# Allow cross-site requests from any origin,
# all information from our allowed endpoits is public.
#
# NOTE: We don't use `CORS_ALLOW_ALL_ORIGINS=True`,
# since that will set the `Access-Control-Allow-Origin` header to `*`,
# we won't be able to pass credentials fo the sustainability API with that value.
CORS_ALLOWED_ORIGIN_REGEXES = [re.compile(".+")]
CORS_ALLOW_HEADERS = list(default_headers) + [
'x-hoverxref-version',
'x-csrftoken'
)
]
Comment on lines +747 to +749
Copy link
Member

Choose a reason for hiding this comment

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

Just to note we are adding 3 extra allowed headers with this change:

    "accept-encoding",
    "dnt",
    "user-agent",

# Additional protection to allow only idempotent methods.
CORS_ALLOW_METHODS = [
'GET',
Expand All @@ -751,14 +755,19 @@ def DOCKER_LIMITS(self):
]

# URLs to allow CORS to read from unauthed.
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",
]
CORS_URLS_REGEX = re.compile(
Copy link
Member

Choose a reason for hiding this comment

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

In the docs it says this is a string. However, it also says Pattern[str] which I'm not sure if that's what re.compile returns or not: https://github.com/adamchainz/django-cors-headers#cors_urls_regex-str--patternstr

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, that's the type https://docs.python.org/3/library/typing.html#typing.Pattern.

r"""
^(
/api/v2/footer_html
|/api/v2/search
|/api/v2/docsearch
|/api/v2/embed
|/api/v3/embed
|/api/v2/sustainability
)
""",
re.VERBOSE,
)

# RTD Settings
ALLOW_PRIVATE_REPOS = False
Expand Down
3 changes: 0 additions & 3 deletions readthedocs/settings/proxito/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,6 @@ 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,
Expand Down
11 changes: 6 additions & 5 deletions requirements/deploy.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,11 @@ billiard==3.6.4.0
# via
# -r requirements/pip.txt
# celery
boto3==1.26.64
boto3==1.26.65
# via
# -r requirements/pip.txt
# django-storages
botocore==1.29.64
botocore==1.29.65
# via
# -r requirements/pip.txt
# boto3
Expand Down Expand Up @@ -104,6 +104,7 @@ django==3.2.17
# dj-stripe
# django-allauth
# django-annoying
# django-cors-headers
# django-csp
# django-debug-toolbar
# django-extensions
Expand All @@ -121,7 +122,7 @@ django-annoying==0.10.6
# via -r requirements/pip.txt
django-autoslug==1.9.8
# via -r requirements/pip.txt
django-cors-middleware==1.4.0
django-cors-headers==3.13.0
# via -r requirements/pip.txt
django-crispy-forms==1.14.0
# via -r requirements/pip.txt
Expand Down Expand Up @@ -338,7 +339,7 @@ s3transfer==0.6.0
# boto3
selectolax==0.3.12
# via -r requirements/pip.txt
sentry-sdk==1.14.0
sentry-sdk==1.15.0
# via structlog-sentry
six==1.16.0
# via
Expand Down Expand Up @@ -438,7 +439,7 @@ vine==5.0.0
# amqp
# celery
# kombu
virtualenv==20.17.1
virtualenv==20.18.0
# via -r requirements/pip.txt
wcwidth==0.2.6
# via
Expand Down
Loading