From 5b6e4c4bfb8706b323bce61d05378cc0d92205f7 Mon Sep 17 00:00:00 2001 From: Vytautas Liuolia Date: Wed, 20 Dec 2023 00:14:36 +0100 Subject: [PATCH] feat(cors): disallow `cors_enable` & additional `CORSMiddleware` combo (#2201) * feat(cors): disallow `cors_enable` & additional CORSMiddleware combo * chore: ignore typing for 1 line because it is unclear how to fix it --- docs/_newsfragments/1977.breakingchange.rst | 4 +++ docs/api/cors.rst | 12 ++++++--- falcon/app.py | 25 ++++++++++++++--- tests/test_middleware.py | 30 +++++++++++++-------- 4 files changed, 54 insertions(+), 17 deletions(-) create mode 100644 docs/_newsfragments/1977.breakingchange.rst diff --git a/docs/_newsfragments/1977.breakingchange.rst b/docs/_newsfragments/1977.breakingchange.rst new file mode 100644 index 000000000..ff6b9c1c3 --- /dev/null +++ b/docs/_newsfragments/1977.breakingchange.rst @@ -0,0 +1,4 @@ +Previously, it was possible to create an :class:`~falcon.App` with the +``cors_enable`` option, and add additional :class:`~falcon.CORSMiddleware`, +leading to unexpected behavior and dysfunctional CORS. This combination now +explicitly results in a :class:`ValueError`. diff --git a/docs/api/cors.rst b/docs/api/cors.rst index 40bec453a..d19bf7a78 100644 --- a/docs/api/cors.rst +++ b/docs/api/cors.rst @@ -60,9 +60,15 @@ Usage allow_origins='example.com', allow_credentials='*')) .. note:: - Passing the ``cors_enable`` parameter set to ``True`` should be seen as - mutually exclusive with directly passing an instance of - :class:`~falcon.CORSMiddleware` to the application's initializer. + Passing the ``cors_enable`` parameter set to ``True`` is mutually exclusive + with directly passing an instance of :class:`~falcon.CORSMiddleware` to the + application's initializer. + + .. versionchanged:: 4.0 + + Attempt to use the combination of ``cors_enable=True`` and an additional + instance of :class:`~falcon.CORSMiddleware` now results in a + :class:`ValueError`. CORSMiddleware -------------- diff --git a/falcon/app.py b/falcon/app.py index 34186b418..434dd6b39 100644 --- a/falcon/app.py +++ b/falcon/app.py @@ -244,6 +244,7 @@ def __init__( cors_enable=False, sink_before_static_route=True, ): + self._cors_enable = cors_enable self._sink_before_static_route = sink_before_static_route self._sinks = [] self._static_routes = [] @@ -447,7 +448,7 @@ def __call__( # noqa: C901 def router_options(self): return self._router.options - def add_middleware(self, middleware: object) -> None: + def add_middleware(self, middleware: Union[object, Iterable]) -> None: """Add one or more additional middleware components. Arguments: @@ -461,10 +462,28 @@ def add_middleware(self, middleware: object) -> None: # the chance that middleware may be None. if middleware: try: - self._unprepared_middleware += middleware + middleware = list(middleware) # type: ignore except TypeError: # middleware is not iterable; assume it is just one bare component - self._unprepared_middleware.append(middleware) + middleware = [middleware] + + if ( + self._cors_enable + and len( + [ + mc + for mc in self._unprepared_middleware + middleware + if isinstance(mc, CORSMiddleware) + ] + ) + > 1 + ): + raise ValueError( + 'CORSMiddleware is not allowed in conjunction with ' + 'cors_enable (which already constructs one instance)' + ) + + self._unprepared_middleware += middleware # NOTE(kgriffs): Even if middleware is None or an empty list, we still # need to make sure self._middleware is initialized if this is the diff --git a/tests/test_middleware.py b/tests/test_middleware.py index 52516622e..b6b1b9d30 100644 --- a/tests/test_middleware.py +++ b/tests/test_middleware.py @@ -1000,20 +1000,28 @@ def test_process_resource_cached(self, asgi, independent_middleware): class TestCORSMiddlewareWithAnotherMiddleware(TestMiddleware): @pytest.mark.parametrize( - 'mw', + 'mw,allowed', [ - CaptureResponseMiddleware(), - [CaptureResponseMiddleware()], - (CaptureResponseMiddleware(),), - iter([CaptureResponseMiddleware()]), + (CaptureResponseMiddleware(), True), + ([CaptureResponseMiddleware()], True), + ((CaptureResponseMiddleware(),), True), + (iter([CaptureResponseMiddleware()]), True), + (falcon.CORSMiddleware(), False), + ([falcon.CORSMiddleware()], False), ], ) - def test_api_initialization_with_cors_enabled_and_middleware_param(self, mw, asgi): - app = create_app(asgi, middleware=mw, cors_enable=True) - app.add_route('/', TestCorsResource()) - client = testing.TestClient(app) - result = client.simulate_get(headers={'Origin': 'localhost'}) - assert result.headers['Access-Control-Allow-Origin'] == '*' + def test_api_initialization_with_cors_enabled_and_middleware_param( + self, mw, asgi, allowed + ): + if allowed: + app = create_app(asgi, middleware=mw, cors_enable=True) + app.add_route('/', TestCorsResource()) + client = testing.TestClient(app) + result = client.simulate_get(headers={'Origin': 'localhost'}) + assert result.headers['Access-Control-Allow-Origin'] == '*' + else: + with pytest.raises(ValueError, match='CORSMiddleware'): + app = create_app(asgi, middleware=mw, cors_enable=True) @pytest.mark.skipif(cython, reason='Cythonized coroutine functions cannot be detected')