Proxito: update CORS settings (#10751)

* 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 <[email protected]>

Author: humitos

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/")

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):

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):

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