fix(api): Add missing CORS headers on external OPTIONS requests (#252)

We previously did not return the `Access-Control-Allow-Headers`, `Access-Control-Allow-Methods`, and `Access-Control-Max-Age`, headers on `OPTIONS` requests coming from an external (non-whitelisted domain). With this change, we properly allow for 3rd parties to use our API with CORS-enabled requests.

Author: ElioDiNino

apps/codecov-api/codecov/settings_base.py

@@ -368,9 +368,10 @@
 # This is read by our custom CORS middleware and will set the allowed origins header to
 # "*" for all origins other than the ones explicitly allowed in CORS_ALLOWED_ORIGINS
 # and CORS_ALLOWED_ORIGIN_REGEXES, in which case it will rely on the default CORS
-# middleware behavior. This ensures that CORS_ALLOW_CREDENTIALS can never be enabled
-# on a request that does not come from an explicitly allowed origin.
-EXTERNAL_CORS_ALLOW_ALL_ORIGINS = True
+# middleware (which also reads this value) behavior. This ensures that
+# CORS_ALLOW_CREDENTIALS can never be enabled on a request that does not come from an
+# explicitly allowed origin.
+CORS_ALLOW_ALL_ORIGINS = True
 
 CORS_ALLOW_CREDENTIALS = True
 CORS_ALLOW_HEADERS = (

apps/codecov-api/codecov_auth/middleware.py

@@ -1,12 +1,14 @@
 import logging
+from urllib.parse import urlsplit
 
 import jwt
 from asgiref.sync import sync_to_async
+from corsheaders.conf import conf
 from corsheaders.middleware import (
+    ACCESS_CONTROL_ALLOW_CREDENTIALS,
     ACCESS_CONTROL_ALLOW_ORIGIN,
 )
 from corsheaders.middleware import CorsMiddleware as BaseCorsMiddleware
-from django.conf import settings
 from django.http import HttpRequest, HttpResponseForbidden, HttpResponseNotFound
 from django.urls import resolve
 from rest_framework import exceptions
@@ -150,19 +152,21 @@ def middleware(request: HttpRequest):
         # Generate base CORS response
         response = base_cors(request)
 
-        # Perform some of the same checks as the base CORS middleware
-        if not base_cors.is_enabled(request) or not request.headers.get("origin"):
+        # If not present, it means CORS is disabled for this request.
+        if ACCESS_CONTROL_ALLOW_ORIGIN not in response:
             return response
 
         # We rely on the CORS middleware to verify that the origin is
-        # valid and allowed for credentialed requests. Otherwise, we
-        # set the access control headers to allow all origins based on
-        # our custom setting.
-        if (
-            settings.EXTERNAL_CORS_ALLOW_ALL_ORIGINS
-            and ACCESS_CONTROL_ALLOW_ORIGIN not in response.headers
-        ):
-            response.headers[ACCESS_CONTROL_ALLOW_ORIGIN] = "*"
+        # valid and to add necessary headers. We manage overriding the
+        # allow origin header and removing the allow credentials header
+        # if the origin is not whitelisted.
+        if conf.CORS_ALLOW_ALL_ORIGINS and conf.CORS_ALLOW_CREDENTIALS:
+            origin = response[ACCESS_CONTROL_ALLOW_ORIGIN]
+            url = urlsplit(origin)
+
+            if not base_cors.origin_found_in_white_lists(origin, url):
+                response[ACCESS_CONTROL_ALLOW_ORIGIN] = "*"
+                del response[ACCESS_CONTROL_ALLOW_CREDENTIALS]
 
         return response
 

apps/codecov-api/codecov_auth/tests/unit/test_middleware.py

@@ -6,27 +6,93 @@
 @override_settings(
     CORS_ALLOWED_ORIGINS=["http://localhost:3000"],
     CORS_ALLOWED_ORIGIN_REGEXES=[],
-    EXTERNAL_CORS_ALLOW_ALL_ORIGINS=True,
+    CORS_ALLOW_ALL_ORIGINS=True,
+    CORS_ALLOW_CREDENTIALS=True,
+    CORS_EXPOSE_HEADERS=["Test-Expose"],
 )
 class MiddlewareTest(TestCase):
+    options_cors_headers = (
+        "Access-Control-Allow-Headers",
+        "Access-Control-Allow-Methods",
+        "Access-Control-Max-Age",
+    )
+
     def setUp(self):
         self.client = Client()
 
-    def test_whitelisted_origin(self):
+    def test_get_whitelisted_origin(self):
         res = self.client.get("/health", headers={"Origin": "http://localhost:3000"})
 
         assert res.headers["Access-Control-Allow-Origin"] == "http://localhost:3000"
         assert res.headers["Access-Control-Allow-Credentials"] == "true"
+        assert res.headers["Access-Control-Expose-Headers"] == "Test-Expose"
+
+        for header in self.options_cors_headers:
+            assert header not in res.headers
+
+    def test_options_whitelisted_origin(self):
+        res = self.client.options(
+            "/health", headers={"Origin": "http://localhost:3000"}
+        )
+
+        assert res.headers["Access-Control-Allow-Origin"] == "http://localhost:3000"
+        assert res.headers["Access-Control-Allow-Credentials"] == "true"
+        assert res.headers["Access-Control-Expose-Headers"] == "Test-Expose"
 
-    def test_non_whitelisted_origin(self):
+        for header in self.options_cors_headers:
+            assert header in res.headers
+
+    def test_get_non_whitelisted_origin(self):
         res = self.client.get("/health", headers={"Origin": "http://example.com"})
 
         assert res.headers["Access-Control-Allow-Origin"] == "*"
         assert "Access-Control-Allow-Credentials" not in res.headers
+        assert res.headers["Access-Control-Expose-Headers"] == "Test-Expose"
+
+        for header in self.options_cors_headers:
+            assert header not in res.headers
+
+    def test_options_non_whitelisted_origin(self):
+        res = self.client.options("/health", headers={"Origin": "http://example.com"})
+
+        assert res.headers["Access-Control-Allow-Origin"] == "*"
+        assert "Access-Control-Allow-Credentials" not in res.headers
+        assert res.headers["Access-Control-Expose-Headers"] == "Test-Expose"
 
-    @override_settings(EXTERNAL_CORS_ALLOW_ALL_ORIGINS=False)
-    def test_external_cors_allow_all_origins_false(self):
+        for header in self.options_cors_headers:
+            assert header in res.headers
+
+    @override_settings(CORS_ALLOW_ALL_ORIGINS=False)
+    def test_options_cors_allow_all_origins_false(self):
+        res = self.client.options("/health", headers={"Origin": "http://example.com"})
+
+        assert "Access-Control-Allow-Origin" not in res.headers
+        assert "Access-Control-Allow-Credentials" not in res.headers
+        assert "Access-Control-Expose-Headers" not in res.headers
+
+        for header in self.options_cors_headers:
+            assert header not in res.headers
+
+    @override_settings(CORS_ALLOW_ALL_ORIGINS=False, CORS_ALLOW_CREDENTIALS=False)
+    def test_get_cors_allow_all_origins_credentials_false(self):
         res = self.client.get("/health", headers={"Origin": "http://example.com"})
 
         assert "Access-Control-Allow-Origin" not in res.headers
         assert "Access-Control-Allow-Credentials" not in res.headers
+        assert "Access-Control-Expose-Headers" not in res.headers
+
+        for header in self.options_cors_headers:
+            assert header not in res.headers
+
+    @override_settings(CORS_ALLOW_CREDENTIALS=False)
+    def test_options_cors_allow_credentials_false(self):
+        res = self.client.options(
+            "/health", headers={"Origin": "http://localhost:3000"}
+        )
+
+        assert res.headers["Access-Control-Allow-Origin"] == "*"
+        assert "Access-Control-Allow-Credentials" not in res.headers
+        assert res.headers["Access-Control-Expose-Headers"] == "Test-Expose"
+
+        for header in self.options_cors_headers:
+            assert header in res.headers