diff --git a/readthedocs/embed/v3/tests/test_access.py b/readthedocs/embed/v3/tests/test_access.py index 955916c36e9..0b6a17d0280 100644 --- a/readthedocs/embed/v3/tests/test_access.py +++ b/readthedocs/embed/v3/tests/test_access.py @@ -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 @@ -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) @@ -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) @@ -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( @@ -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/") diff --git a/readthedocs/proxito/middleware.py b/readthedocs/proxito/middleware.py index 667f5ea5391..e65bb1dd801 100644 --- a/readthedocs/proxito/middleware.py +++ b/readthedocs/proxito/middleware.py @@ -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 @@ -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 @@ -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", "") @@ -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): diff --git a/readthedocs/proxito/tests/test_headers.py b/readthedocs/proxito/tests/test_headers.py index 9f619c5655a..df993a445f6 100644 --- a/readthedocs/proxito/tests/test_headers.py +++ b/readthedocs/proxito/tests/test_headers.py @@ -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 @@ -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): @@ -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): diff --git a/readthedocs/search/tests/test_proxied_api.py b/readthedocs/search/tests/test_proxied_api.py index 5ebbb1e3931..8a22bb2b2b3 100644 --- a/readthedocs/search/tests/test_proxied_api.py +++ b/readthedocs/search/tests/test_proxied_api.py @@ -1,4 +1,5 @@ import pytest +from corsheaders.middleware import ACCESS_CONTROL_ALLOW_ORIGIN from readthedocs.search.tests.test_api import BaseTestDocumentSearch @@ -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