feat(CORS): improve cors middleware (#2326)

* typing: type app

* typing: type websocket module

* typing: type asgi.reader, asgi.structures, asgi.stream

* typing: type most of media

* typing: type multipart

* typing: type response

* style: fix spelling in multipart.py

* style(tests): explain referencing the same property multiple times

* style: fix linter errors

* chore: revert behavioral change to cors middleware.

* feat: improve cors header to properly handle missing allow headers

The static resource will now properly supports CORS requests.

Fixes: #2325

* chore: do not build rapidjson on PyPy

docs/_newsfragments/2325.newandimproved.rst

@@ -0,0 +1,4 @@
+The :class:`~CORSMiddleware` now properly handles the missing ``Allow``
+header case, by denying the preflight CORS request.
+The static resource has been updated to properly support CORS request,
+by allowing GET requests.

falcon/app.py

@@ -759,6 +759,15 @@ def add_sink(self, sink: SinkCallable, prefix: SinkPrefix = r'/') -> None:
         impractical. For example, you might use a sink to create a smart
         proxy that forwards requests to one or more backend services.
 
+        Note:
+            To support CORS preflight requests when using the default CORS middleware,
+            either by setting ``App.cors_enable=True`` or by adding the
+            :class:`~.CORSMiddleware` to the ``App.middleware``, the sink should
+            set the ``Allow`` header in the request to the allowed
+            method values when serving an ``OPTIONS`` request. If the ``Allow`` header
+            is missing from the response, the default CORS middleware will deny the
+            preflight request.
+
         Args:
             sink (callable): A callable taking the form ``func(req, resp, **kwargs)``.
 

falcon/middleware.py

@@ -17,6 +17,15 @@ class CORSMiddleware(object):
     * https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS
     * https://www.w3.org/TR/cors/#resource-processing-model
 
+    Note:
+        Falcon will automatically add OPTIONS responders if they are missing from the
+        responder instances added to the routes. When providing a custom ``on_options``
+        method, the ``Allow`` headers in the response should be set to the allowed
+        method values. If the ``Allow`` header is missing from the response,
+        this middleware will deny the preflight request.
+
+        This is also valid when using a sink function.
+
     Keyword Arguments:
         allow_origins (Union[str, Iterable[str]]): List of origins to allow (case
             sensitive). The string ``'*'`` acts as a wildcard, matching every origin.
@@ -120,9 +129,17 @@ def process_response(
                 'Access-Control-Request-Headers', default='*'
             )
 
-            resp.set_header('Access-Control-Allow-Methods', str(allow))
-            resp.set_header('Access-Control-Allow-Headers', allow_headers)
-            resp.set_header('Access-Control-Max-Age', '86400')  # 24 hours
+            if allow is None:
+                # there is no allow set, remove all access control headers
+                resp.delete_header('Access-Control-Allow-Methods')
+                resp.delete_header('Access-Control-Allow-Headers')
+                resp.delete_header('Access-Control-Max-Age')
+                resp.delete_header('Access-Control-Expose-Headers')
+                resp.delete_header('Access-Control-Allow-Origin')
+            else:
+                resp.set_header('Access-Control-Allow-Methods', allow)
+                resp.set_header('Access-Control-Allow-Headers', allow_headers)
+                resp.set_header('Access-Control-Max-Age', '86400')  # 24 hours
 
     async def process_response_async(self, *args: Any) -> None:
         self.process_response(*args)

falcon/routing/static.py

@@ -183,6 +183,12 @@ def match(self, path: str) -> bool:
     def __call__(self, req: Request, resp: Response, **kw: Any) -> None:
         """Resource responder for this route."""
         assert not kw
+        if req.method == 'OPTIONS':
+            # it's likely a CORS request. Set the allow header to the appropriate value.
+            resp.set_header('Allow', 'GET')
+            resp.set_header('Content-Length', '0')
+            return
+
         without_prefix = req.path[len(self._prefix) :]
 
         # NOTE(kgriffs): Check surrounding whitespace and strip trailing
@@ -247,9 +253,9 @@ class StaticRouteAsync(StaticRoute):
 
     async def __call__(self, req: asgi.Request, resp: asgi.Response, **kw: Any) -> None:  # type: ignore[override]
         super().__call__(req, resp, **kw)
-
-        # NOTE(kgriffs): Fixup resp.stream so that it is non-blocking
-        resp.stream = _AsyncFileReader(resp.stream)  # type: ignore[assignment,arg-type]
+        if resp.stream is not None:  # None when in an option request
+            # NOTE(kgriffs): Fixup resp.stream so that it is non-blocking
+            resp.stream = _AsyncFileReader(resp.stream)  # type: ignore[assignment,arg-type]
 
 
 class _AsyncFileReader:

tests/test_cors_middleware.py

@@ -1,3 +1,5 @@
+from pathlib import Path
+
 import pytest
 
 import falcon
@@ -86,8 +88,7 @@ def test_enabled_cors_handles_preflighting(self, cors_client):
             result.headers['Access-Control-Max-Age'] == '86400'
         )  # 24 hours in seconds
 
-    @pytest.mark.xfail(reason='will be fixed in 2325')
-    def test_enabled_cors_handles_preflighting_custom_option(self, cors_client):
+    def test_enabled_cors_handles_preflight_custom_option(self, cors_client):
         cors_client.app.add_route('/', CORSOptionsResource())
         result = cors_client.simulate_options(
             headers=(
@@ -97,11 +98,10 @@ def test_enabled_cors_handles_preflighting_custom_option(self, cors_client):
             )
         )
         assert 'Access-Control-Allow-Methods' not in result.headers
-        assert (
-            result.headers['Access-Control-Allow-Headers']
-            == 'X-PINGOTHER, Content-Type'
-        )
-        assert result.headers['Access-Control-Max-Age'] == '86400'
+        assert 'Access-Control-Allow-Headers' not in result.headers
+        assert 'Access-Control-Max-Age' not in result.headers
+        assert 'Access-Control-Expose-Headers' not in result.headers
+        assert 'Access-Control-Allow-Origin' not in result.headers
 
     def test_enabled_cors_handles_preflighting_no_headers_in_req(self, cors_client):
         cors_client.app.add_route('/', CORSHeaderResource())
@@ -117,6 +117,50 @@ def test_enabled_cors_handles_preflighting_no_headers_in_req(self, cors_client):
             result.headers['Access-Control-Max-Age'] == '86400'
         )  # 24 hours in seconds
 
+    def test_enabled_cors_static_route(self, cors_client):
+        cors_client.app.add_static_route('/static', Path(__file__).parent)
+        result = cors_client.simulate_options(
+            f'/static/{Path(__file__).name}',
+            headers=(
+                ('Origin', 'localhost'),
+                ('Access-Control-Request-Method', 'GET'),
+            ),
+        )
+
+        assert result.headers['Access-Control-Allow-Methods'] == 'GET'
+        assert result.headers['Access-Control-Allow-Headers'] == '*'
+        assert result.headers['Access-Control-Max-Age'] == '86400'
+        assert result.headers['Access-Control-Allow-Origin'] == '*'
+
+    @pytest.mark.parametrize('support_options', [True, False])
+    def test_enabled_cors_sink_route(self, cors_client, support_options):
+        def my_sink(req, resp):
+            if req.method == 'OPTIONS' and support_options:
+                resp.set_header('ALLOW', 'GET')
+            else:
+                resp.text = 'my sink'
+
+        cors_client.app.add_sink(my_sink, '/sink')
+        result = cors_client.simulate_options(
+            '/sink/123',
+            headers=(
+                ('Origin', 'localhost'),
+                ('Access-Control-Request-Method', 'GET'),
+            ),
+        )
+
+        if support_options:
+            assert result.headers['Access-Control-Allow-Methods'] == 'GET'
+            assert result.headers['Access-Control-Allow-Headers'] == '*'
+            assert result.headers['Access-Control-Max-Age'] == '86400'
+            assert result.headers['Access-Control-Allow-Origin'] == '*'
+        else:
+            assert 'Access-Control-Allow-Methods' not in result.headers
+            assert 'Access-Control-Allow-Headers' not in result.headers
+            assert 'Access-Control-Max-Age' not in result.headers
+            assert 'Access-Control-Expose-Headers' not in result.headers
+            assert 'Access-Control-Allow-Origin' not in result.headers
+
 
 @pytest.fixture(scope='function')
 def make_cors_client(asgi, util):

tests/test_static.py

@@ -617,3 +617,19 @@ def test_file_closed(client, patch_open):
 
     assert patch_open.current_file is not None
     assert patch_open.current_file.closed
+
+
+def test_options_request(util, asgi, patch_open):
+    patch_open()
+    app = util.create_app(asgi, cors_enable=True)
+    app.add_static_route('/static', '/var/www/statics')
+    client = testing.TestClient(app)
+
+    resp = client.simulate_options(
+        path='/static/foo/bar.txt',
+        headers={'Origin': 'localhost', 'Access-Control-Request-Method': 'GET'},
+    )
+    assert resp.status_code == 200
+    assert resp.text == ''
+    assert int(resp.headers['Content-Length']) == 0
+    assert resp.headers['Access-Control-Allow-Methods'] == 'GET'