Public CORS decorator (PP-5045)

src/palace/manager/api/routes.py

@@ -1,11 +1,15 @@
 import logging
+from collections.abc import Callable
 from functools import update_wrapper, wraps
 
 import flask
-from flask import Response, make_response, request
+from flask import Response, current_app, make_response, request
+from flask_cors import cross_origin
 from flask_cors.core import get_cors_options, set_cors_headers
 from werkzeug.exceptions import MethodNotAllowed
 
+from palace.util.exceptions import PalaceValueError
+
 from palace.manager.api.app import app
 from palace.manager.core.app_server import (
     cache_control_headers,
@@ -85,6 +89,12 @@ def decorated(*args, **kwargs):
 # can't use that decorator because we aren't able to look up the patron web
 # client url configuration setting at the time we create the decorator.
 def allows_patron_web(f):
+    if getattr(f, "allows_public_cors", False):
+        raise PalaceValueError(
+            "allows_patron_web and allows_public_cors must not be stacked "
+            "on one route."
+        )
+
     # Override Flask's default behavior and intercept the OPTIONS method for
     # every request so CORS headers can be added.
     f.required_methods = getattr(f, "required_methods", set())
@@ -106,7 +116,81 @@ def wrapped_function(*args, **kwargs):
 
         return resp
 
-    return update_wrapper(wrapped_function, f)
+    wrapper = update_wrapper(wrapped_function, f)
+    # Marker checked by allows_public_cors to reject stacking both CORS
+    # decorators on one route.
+    wrapper.allows_patron_web = True
+    return wrapper
+
+
+_public_cors = cross_origin(
+    methods=["GET", "HEAD", "OPTIONS"],
+    max_age=3600,
+    send_wildcard=True,
+    supports_credentials=False,
+)
+
+
+def allows_public_cors[**P](f: Callable[P, object]) -> Callable[P, Response]:
+    """Decorator that adds permissive CORS headers to a public route.
+
+    Anyone can already read these routes without authenticating, so every
+    web origin is allowed and no configuration is needed. The
+    Access-Control-Allow-Credentials header is never sent, so browsers
+    refuse to share these responses with cross-origin scripts that make
+    cookie-authenticated requests, which is what keeps the wildcard origin
+    safe. Patron-specific behavior on these routes works only through the
+    Authorization header, which a cross-origin script must set explicitly.
+
+    Use either this decorator or allows_patron_web on a route, never both;
+    stacking them raises PalaceValueError at decoration time. The
+    advertised methods list only limits what a preflight advertises; it
+    does not block other methods on the actual response, so apply this
+    decorator only to GET/HEAD routes.
+
+    Place this decorator outside has_library and any other decorator that
+    can answer a request without calling the view. That way this wrapper
+    answers OPTIONS preflights itself, with the full CORS header set a
+    preflight needs, and adds headers to short-circuit responses. The
+    add_public_cors_to_error_responses hook covers what remains, mainly
+    responses built by the app-level error handler.
+    """
+    if getattr(f, "allows_patron_web", False):
+        raise PalaceValueError(
+            "allows_public_cors and allows_patron_web must not be stacked "
+            "on one route."
+        )
+    if getattr(f, "allows_public_cors", False):
+        raise PalaceValueError("allows_public_cors is already applied to this route.")
+
+    wrapper = _public_cors(f)
+    # The marker attribute is read by add_public_cors_to_error_responses
+    # and by allows_patron_web's stacking check. It survives outer
+    # decorators because functools.wraps copies __dict__.
+    setattr(wrapper, "allows_public_cors", True)
+    return wrapper
+
+
+@app.after_request
+def add_public_cors_to_error_responses(response: Response) -> Response:
+    """Back-fill the wildcard CORS header on public routes.
+
+    The allows_public_cors decorator cannot add headers to a response it
+    never sees. A view that raises gets its response built by the app-level
+    error handler, and cross-origin clients need the header on that
+    response to read the error body. As a safety net, the hook also covers
+    stacks that ignore the placement rule in allows_public_cors, where an
+    outer decorator returns a problem detail without calling the view.
+
+    Responses produced before routing resolves an endpoint, such as a 405
+    for a method the route does not allow, cannot be attributed to a view
+    and are not back-filled.
+    """
+    if "Access-Control-Allow-Origin" not in response.headers and request.endpoint:
+        view = current_app.view_functions.get(request.endpoint)
+        if view is not None and getattr(view, "allows_public_cors", False):
+            response.headers["Access-Control-Allow-Origin"] = "*"
+    return response
 
 
 def has_library(f):

tests/manager/api/test_routes.py

@@ -1,10 +1,15 @@
 import logging
+from collections.abc import Callable
+from functools import wraps
 from unittest.mock import MagicMock, patch
 
 import flask
 import pytest
+from flask.testing import FlaskClient
 from werkzeug.datastructures import ImmutableMultiDict
 
+from palace.util.exceptions import PalaceValueError
+
 from palace.manager.api import routes
 from palace.manager.api.problem_details import LIBRARY_NOT_FOUND
 from palace.manager.sqlalchemy.listeners import site_configuration_has_changed
@@ -17,6 +22,226 @@ class TestAppConfiguration:
     # Test the configuration of the real Flask app.
     def test_configuration(self):
         assert False == routes.app.url_map.merge_slashes
+        # The CORS back-fill hook must be registered on the real app.
+        assert (
+            routes.add_public_cors_to_error_responses
+            in routes.app.after_request_funcs[None]
+        )
+
+
+class TestAllowsPublicCors:
+    @pytest.fixture
+    def client(self) -> FlaskClient:
+        # Use a minimal Flask app so the decorator is tested in isolation.
+        app = flask.Flask(__name__)
+
+        @app.route("/public")
+        @routes.allows_public_cors
+        def public_route() -> str:
+            return "public"
+
+        return app.test_client()
+
+    @pytest.mark.parametrize(
+        "headers",
+        [
+            pytest.param({}, id="no-origin"),
+            pytest.param({"Origin": "http://any.web.client"}, id="with-origin"),
+        ],
+    )
+    def test_get_allows_any_origin(
+        self, client: FlaskClient, headers: dict[str, str]
+    ) -> None:
+        response = client.get("/public", headers=headers)
+        assert response.status_code == 200
+        assert response.get_data(as_text=True) == "public"
+        assert response.headers["Access-Control-Allow-Origin"] == "*"
+        assert "Access-Control-Allow-Credentials" not in response.headers
+        # The wildcard origin is constant, so responses must stay cacheable
+        # without a Vary: Origin header.
+        assert "Origin" not in response.headers.get("Vary", "")
+
+    def test_preflight(self, client: FlaskClient) -> None:
+        response = client.options(
+            "/public",
+            headers={
+                "Origin": "http://any.web.client",
+                "Access-Control-Request-Method": "GET",
+                "Access-Control-Request-Headers": "Authorization",
+            },
+        )
+        assert response.status_code == 200
+        assert response.headers["Access-Control-Allow-Origin"] == "*"
+        # Only the read-only methods are advertised.
+        assert {
+            method.strip()
+            for method in response.headers["Access-Control-Allow-Methods"].split(",")
+        } == {"GET", "HEAD", "OPTIONS"}
+        assert (
+            "authorization" in response.headers["Access-Control-Allow-Headers"].lower()
+        )
+        assert response.headers["Access-Control-Max-Age"] == "3600"
+        assert "Access-Control-Allow-Credentials" not in response.headers
+
+    def test_preflight_does_not_advertise_write_methods(
+        self, client: FlaskClient
+    ) -> None:
+        response = client.options(
+            "/public",
+            headers={
+                "Origin": "http://any.web.client",
+                "Access-Control-Request-Method": "POST",
+            },
+        )
+        assert response.headers["Access-Control-Allow-Origin"] == "*"
+        assert "Access-Control-Allow-Methods" not in response.headers
+
+    def test_error_responses_keep_cors(self) -> None:
+        # A raising view never returns through the decorator, so the
+        # after_request hook must back-fill the wildcard header. Routes
+        # without the decorator must stay untouched.
+        app = flask.Flask(__name__)
+        app.after_request(routes.add_public_cors_to_error_responses)
+
+        @app.route("/public-ok")
+        @routes.allows_public_cors
+        def public_ok() -> str:
+            return "ok"
+
+        @app.route("/public-error")
+        @routes.allows_public_cors
+        def public_error() -> str:
+            flask.abort(404)
+
+        @app.route("/private-error")
+        def private_error() -> str:
+            flask.abort(404)
+
+        # An outer decorator that returns a response without calling the
+        # view. Such decorators belong inside allows_public_cors, but the
+        # hook still back-fills the header when a stack gets that wrong.
+        def short_circuit(
+            f: Callable[..., flask.Response],
+        ) -> Callable[..., flask.Response]:
+            @wraps(f)
+            def decorated(*args: object, **kwargs: object) -> flask.Response:
+                return flask.Response("no library", status=404)
+
+            return decorated
+
+        @app.route("/short-circuit")
+        @short_circuit
+        @routes.allows_public_cors
+        def short_circuit_route() -> str:
+            return "unreachable"
+
+        client = app.test_client()
+        response = client.get(
+            "/public-error", headers={"Origin": "http://any.web.client"}
+        )
+        assert response.status_code == 404
+        assert response.headers["Access-Control-Allow-Origin"] == "*"
+
+        response = client.get(
+            "/private-error", headers={"Origin": "http://any.web.client"}
+        )
+        assert response.status_code == 404
+        assert "Access-Control-Allow-Origin" not in response.headers
+
+        # The view never runs, but the marker propagated through the outer
+        # decorator's functools.wraps, so the hook still adds the header.
+        response = client.get(
+            "/short-circuit", headers={"Origin": "http://any.web.client"}
+        )
+        assert response.status_code == 404
+        assert response.get_data(as_text=True) == "no library"
+        assert response.headers["Access-Control-Allow-Origin"] == "*"
+
+        # A successful decorated response already carries the header, so
+        # the hook leaves it alone.
+        response = client.get("/public-ok", headers={"Origin": "http://any.web.client"})
+        assert response.status_code == 200
+        assert response.headers["Access-Control-Allow-Origin"] == "*"
+
+        # A URL that matches no route has no endpoint, so the hook has
+        # nothing to check and adds no header.
+        response = client.get(
+            "/no-such-route", headers={"Origin": "http://any.web.client"}
+        )
+        assert response.status_code == 404
+        assert "Access-Control-Allow-Origin" not in response.headers
+
+    @pytest.mark.parametrize(
+        "outer,inner",
+        [
+            pytest.param(
+                routes.allows_public_cors,
+                routes.allows_patron_web,
+                id="public-cors-outer",
+            ),
+            pytest.param(
+                routes.allows_patron_web,
+                routes.allows_public_cors,
+                id="patron-web-outer",
+            ),
+        ],
+    )
+    def test_stacking_with_allows_patron_web_raises(
+        self, outer: Callable[..., object], inner: Callable[..., object]
+    ) -> None:
+        # The two CORS decorators are mutually exclusive on a route, and
+        # misuse must fail at decoration time in either stacking order.
+        def view() -> str:
+            return "view"
+
+        with pytest.raises(PalaceValueError, match="must not be stacked"):
+            outer(inner(view))
+
+    def test_double_application_raises(self) -> None:
+        # Applying the decorator twice is a route table mistake, and it
+        # fails at decoration time like the patron web stacking check.
+        def view() -> str:
+            return "view"
+
+        decorated = routes.allows_public_cors(view)
+        with pytest.raises(PalaceValueError, match="already applied"):
+            routes.allows_public_cors(decorated)
+
+    def test_stacked_with_wrapping_decorator(self) -> None:
+        # The decorator's OPTIONS interception attributes and marker must
+        # survive an outer wraps-based decorator, whatever the stack looks
+        # like, so Flask still routes preflights to the view.
+        app = flask.Flask(__name__)
+
+        def passthrough(
+            f: Callable[..., flask.Response],
+        ) -> Callable[..., flask.Response]:
+            @wraps(f)
+            def decorated(*args: object, **kwargs: object) -> flask.Response:
+                return f(*args, **kwargs)
+
+            return decorated
+
+        @app.route("/stacked")
+        @passthrough
+        @routes.allows_public_cors
+        def stacked_route() -> str:
+            return "stacked"
+
+        client = app.test_client()
+        response = client.options(
+            "/stacked",
+            headers={
+                "Origin": "http://any.web.client",
+                "Access-Control-Request-Method": "GET",
+            },
+        )
+        assert response.status_code == 200
+        assert response.headers["Access-Control-Allow-Origin"] == "*"
+
+        response = client.get("/stacked", headers={"Origin": "http://any.web.client"})
+        assert response.get_data(as_text=True) == "stacked"
+        assert response.headers["Access-Control-Allow-Origin"] == "*"
 
 
 class TestAdminRequestLifecycle: