Skip to content

Commit

Permalink
Proxito: update CORS settings (#10751)
Browse files Browse the repository at this point in the history
* Proxito: update CORS settings

- Only add CORS headers on community site
- Explicit host on `Access-Control-Allow-Origin`
- Only query the database if the host ends with `RTD_EXTERNAL_VERSION_DOMAIN`
- Add more tests

Continuation of #10737

* Add `Vary: Origin` header

* Use Django internals to patch `Vary` header.

* Use the `Origin` header from request to check for the allowed domain

* Update tests to use `origin` header

* Allow cross-origin requests for public versions' docs

---------

Co-authored-by: Santos Gallegos <stsewd@proton.me>
  • Loading branch information
humitos and stsewd authored Sep 20, 2023
1 parent a5af4e5 commit a0285d6
Show file tree
Hide file tree
Showing 4 changed files with 121 additions and 16 deletions.
5 changes: 5 additions & 0 deletions readthedocs/embed/v3/tests/test_access.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
from unittest import mock

import pytest
from corsheaders.middleware import ACCESS_CONTROL_ALLOW_ORIGIN
from django.contrib.auth.models import User
from django.test import TestCase, override_settings
from django.urls import reverse
Expand Down Expand Up @@ -67,6 +68,7 @@ def test_get_content_public_version_anonymous_user(self, storage_mock):
resp = self.get(self.url)
self.assertEqual(resp.status_code, 200)
self.assertIn("Content", resp.json()["content"])
self.assertNotIn(ACCESS_CONTROL_ALLOW_ORIGIN, resp.headers)

def test_get_content_private_version_anonymous_user(self, storage_mock):
self._mock_storage(storage_mock)
Expand All @@ -85,6 +87,7 @@ def test_get_content_public_version_logged_in_user(self, storage_mock):
resp = self.get(self.url)
self.assertEqual(resp.status_code, 200)
self.assertIn("Content", resp.json()["content"])
self.assertNotIn(ACCESS_CONTROL_ALLOW_ORIGIN, resp.headers)

def test_get_content_private_version_logged_in_user(self, storage_mock):
self._mock_storage(storage_mock)
Expand All @@ -96,6 +99,7 @@ def test_get_content_private_version_logged_in_user(self, storage_mock):
resp = self.get(self.url)
self.assertEqual(resp.status_code, 200)
self.assertIn("Content", resp.json()["content"])
self.assertNotIn(ACCESS_CONTROL_ALLOW_ORIGIN, resp.headers)

@mock.patch.object(EmbedAPIBase, "_download_page_content")
def test_get_content_allowed_external_page(
Expand All @@ -108,6 +112,7 @@ def test_get_content_allowed_external_page(
)
self.assertEqual(resp.status_code, 200)
self.assertIn("Content", resp.json()["content"])
self.assertNotIn(ACCESS_CONTROL_ALLOW_ORIGIN, resp.headers)

def test_get_content_not_allowed_external_page(self, storage_mock):
resp = self.get(reverse("embed_api_v3") + "?url=https://example.com/en/latest/")
Expand Down
27 changes: 24 additions & 3 deletions readthedocs/proxito/middleware.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@
from urllib.parse import urlparse

import structlog
from corsheaders.middleware import (
ACCESS_CONTROL_ALLOW_METHODS,
ACCESS_CONTROL_ALLOW_ORIGIN,
)
from django.conf import settings
from django.core.exceptions import SuspiciousOperation
from django.shortcuts import redirect
Expand Down Expand Up @@ -308,7 +312,7 @@ def add_hosting_integrations_headers(self, request, response):

def add_cors_headers(self, request, response):
"""
Add CORS headers only on PUBLIC versions.
Add CORS headers only to files from PUBLIC versions.
DocDiff addons requires making a request from
``RTD_EXTERNAL_VERSION_DOMAIN`` to ``PUBLIC_DOMAIN`` to be able to
Expand All @@ -317,7 +321,23 @@ def add_cors_headers(self, request, response):
This request needs ``Access-Control-Allow-Origin`` HTTP headers to be
accepted by browsers. However, we cannot expose these headers for
documentation that's not PUBLIC.
We set this header to `*`, since the allowed versions are public only,
we don't care about the origin of the request. And we don't have the
need nor want to allow passing credentials from cross-origin requests.
See https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Access-Control-Allow-Origin.
"""

# Disable CORS on "Read the Docs for Business" for now.
# We want to be pretty sure this logic is OK before enabling it there.
if settings.ALLOW_PRIVATE_REPOS:
return

# TODO: se should add these headers to files from docs only,
# proxied APIs and other endpoints should not have CORS headers.
# These attributes aren't currently set for proxied APIs, but we shuold
# find a better way to do this.
project_slug = getattr(request, "path_project_slug", "")
version_slug = getattr(request, "path_version_slug", "")

Expand All @@ -328,8 +348,9 @@ def add_cors_headers(self, request, response):
privacy_level=PUBLIC,
).exists()
if allow_cors:
response.headers["Access-Control-Allow-Origin"] = "*.readthedocs.build"
response.headers["Access-Control-Allow-Methods"] = "OPTIONS, GET"
response.headers[ACCESS_CONTROL_ALLOW_ORIGIN] = "*"
response.headers[ACCESS_CONTROL_ALLOW_METHODS] = "HEAD, OPTIONS, GET"

return response

def _get_https_redirect(self, request):
Expand Down
103 changes: 90 additions & 13 deletions readthedocs/proxito/tests/test_headers.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,16 @@
import django_dynamic_fixture as fixture
from corsheaders.middleware import (
ACCESS_CONTROL_ALLOW_CREDENTIALS,
ACCESS_CONTROL_ALLOW_METHODS,
ACCESS_CONTROL_ALLOW_ORIGIN,
)
from django.test import override_settings
from django.urls import reverse
from django_dynamic_fixture import get

from readthedocs.builds.constants import LATEST
from readthedocs.builds.constants import EXTERNAL, LATEST
from readthedocs.builds.models import Version
from readthedocs.organizations.models import Organization
from readthedocs.projects.constants import PRIVATE, PUBLIC
from readthedocs.projects.models import Domain, HTTPHeader

Expand All @@ -12,6 +20,7 @@
@override_settings(
PUBLIC_DOMAIN="dev.readthedocs.io",
PUBLIC_DOMAIN_USES_HTTPS=True,
RTD_EXTERNAL_VERSION_DOMAIN="dev.readthedocs.build",
)
class ProxitoHeaderTests(BaseDocServing):
def test_redirect_headers(self):
Expand Down Expand Up @@ -159,29 +168,97 @@ def test_hosting_integrations_header(self):
self.assertIsNotNone(r.get("X-RTD-Hosting-Integrations"))
self.assertEqual(r["X-RTD-Hosting-Integrations"], "true")

@override_settings(ALLOW_PRIVATE_REPOS=False)
def test_cors_headers_external_version(self):
get(
Version,
project=self.project,
slug="111",
active=True,
privacy_level=PUBLIC,
type=EXTERNAL,
)

# Normal request
r = self.client.get(
"/en/111/",
secure=True,
headers={"host": "project--111.dev.readthedocs.build"},
)
self.assertEqual(r.status_code, 200)
self.assertEqual(r[ACCESS_CONTROL_ALLOW_ORIGIN], "*")
self.assertNotIn(ACCESS_CONTROL_ALLOW_CREDENTIALS, r.headers)
self.assertEqual(r[ACCESS_CONTROL_ALLOW_METHODS], "HEAD, OPTIONS, GET")

# Cross-origin request
r = self.client.get(
"/en/111/",
secure=True,
headers={
"host": "project--111.dev.readthedocs.build",
"origin": "https://example.com",
},
)
self.assertEqual(r.status_code, 200)
self.assertEqual(r[ACCESS_CONTROL_ALLOW_ORIGIN], "*")
self.assertNotIn(ACCESS_CONTROL_ALLOW_CREDENTIALS, r.headers)
self.assertEqual(r[ACCESS_CONTROL_ALLOW_METHODS], "HEAD, OPTIONS, GET")

@override_settings(ALLOW_PRIVATE_REPOS=True, RTD_ALLOW_ORGANIZATIONS=True)
def test_cors_headers_private_version(self):
version = self.project.versions.get(slug=LATEST)
version.privacy_level = PRIVATE
version.save()
get(Organization, owners=[self.eric], projects=[self.project])
self.version.privacy_level = PRIVATE
self.version.save()

self.client.force_login(self.eric)

# Normal request
r = self.client.get(
"/en/latest/", secure=True, headers={"host": "project.dev.readthedocs.io"}
"/en/latest/",
secure=True,
headers={"host": "project.dev.readthedocs.io"},
)
self.assertEqual(r.status_code, 200)
self.assertIsNone(r.get("Access-Control-Allow-Origin"))
self.assertIsNone(r.get("Access-Control-Allow-Methods"))
self.assertNotIn(ACCESS_CONTROL_ALLOW_ORIGIN, r.headers)

# Cross-origin request
r = self.client.get(
"/en/latest/",
secure=True,
headers={
"host": "project.dev.readthedocs.io",
"origin": "https://example.com",
},
)
self.assertEqual(r.status_code, 200)
self.assertNotIn(ACCESS_CONTROL_ALLOW_ORIGIN, r.headers)

@override_settings(ALLOW_PRIVATE_REPOS=False)
def test_cors_headers_public_version(self):
version = self.project.versions.get(slug=LATEST)
version.privacy_level = PUBLIC
version.save()
# Normal request
r = self.client.get(
"/en/latest/",
secure=True,
headers={"host": "project.dev.readthedocs.io"},
)
self.assertEqual(r.status_code, 200)
self.assertEqual(r[ACCESS_CONTROL_ALLOW_ORIGIN], "*")
self.assertNotIn(ACCESS_CONTROL_ALLOW_CREDENTIALS, r.headers)
self.assertEqual(r[ACCESS_CONTROL_ALLOW_METHODS], "HEAD, OPTIONS, GET")

# Cross-origin request
r = self.client.get(
"/en/latest/", secure=True, headers={"host": "project.dev.readthedocs.io"}
"/en/latest/",
secure=True,
headers={
"host": "project.dev.readthedocs.io",
"origin": "https://example.com",
},
)
self.assertEqual(r.status_code, 200)
self.assertEqual(r["Access-Control-Allow-Origin"], "*.readthedocs.build")
self.assertEqual(r["Access-Control-Allow-Methods"], "OPTIONS, GET")
self.assertEqual(r[ACCESS_CONTROL_ALLOW_ORIGIN], "*")
self.assertNotIn(ACCESS_CONTROL_ALLOW_CREDENTIALS, r.headers)
self.assertEqual(r[ACCESS_CONTROL_ALLOW_METHODS], "HEAD, OPTIONS, GET")

@override_settings(ALLOW_PRIVATE_REPOS=False)
def test_cache_headers_public_version_with_private_projects_not_allowed(self):
Expand Down
2 changes: 2 additions & 0 deletions readthedocs/search/tests/test_proxied_api.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import pytest
from corsheaders.middleware import ACCESS_CONTROL_ALLOW_ORIGIN

from readthedocs.search.tests.test_api import BaseTestDocumentSearch

Expand All @@ -25,3 +26,4 @@ def test_headers(self, api_client, project):
f"{project.slug},{project.slug}:{version.slug},{project.slug}:rtd-search"
)
assert resp["Cache-Tag"] == cache_tags
assert ACCESS_CONTROL_ALLOW_ORIGIN not in resp.headers

0 comments on commit a0285d6

Please sign in to comment.