From b2d8b376719ed77cf1cb5cf6848d89fc29e35c86 Mon Sep 17 00:00:00 2001 From: Josh Wilson Date: Thu, 26 Nov 2020 01:59:09 -0800 Subject: [PATCH 1/3] Add Origin to Vary header on credentialed CORS response MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit According to the [MDN CORS docs] (https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS#Access-Control-Allow-Origin), the `Origin` item should be added to the `Vary` header when the `Access-Control-Allow-Origin` is set to an explicit origin value, as opposed to the `*` wildcard. >If the server specifies a single origin (that may dynamically change based on the requesting origin as part of a white-list) rather than the "*" wildcard, then the server should also include Origin in the Vary response header — to indicate to clients that server responses will differ based on the value of the Origin request header. The existing code fails to update the `Vary` list when the server is configured to allow all origins (`*`) and the request has a `Cookie` header (ie. credentialed). In that situation, the `Access-Control-Allow-Origin` header will be set to the request's `Origin` value. It appears this may have just been a simple oversight in the original implementation. This updates the code to add `Origin` to the `Vary` header under these circumstancesIf it was intentionally omitted, I'd be delighted to learn why. --- starlette/middleware/cors.py | 11 ++++++++--- tests/middleware/test_cors.py | 27 +++++++++++++++++++++++---- 2 files changed, 31 insertions(+), 7 deletions(-) diff --git a/starlette/middleware/cors.py b/starlette/middleware/cors.py index 0c9bdfb38..38b540318 100644 --- a/starlette/middleware/cors.py +++ b/starlette/middleware/cors.py @@ -157,11 +157,16 @@ async def send( # If request includes any cookie headers, then we must respond # with the specific origin instead of '*'. if self.allow_all_origins and has_cookie: - headers["Access-Control-Allow-Origin"] = origin + self.allow_explicit_origin(origin, headers) # If we only allow specific origins, then we have to mirror back # the Origin header in the response. elif not self.allow_all_origins and self.is_allowed_origin(origin=origin): - headers["Access-Control-Allow-Origin"] = origin - headers.add_vary_header("Origin") + self.allow_explicit_origin(origin, headers) + await send(message) + + @staticmethod + def allow_explicit_origin(origin, headers): + headers["Access-Control-Allow-Origin"] = origin + headers.add_vary_header("Origin") diff --git a/tests/middleware/test_cors.py b/tests/middleware/test_cors.py index 2048cb11e..1082c3288 100644 --- a/tests/middleware/test_cors.py +++ b/tests/middleware/test_cors.py @@ -245,12 +245,30 @@ def homepage(request): assert response.headers["vary"] == "Origin" -def test_cors_vary_header_is_properly_set(): +def test_cors_vary_header_is_properly_set_for_credentialed_request(): app = Starlette() - app.add_middleware(CORSMiddleware, allow_origins=["https://example.org"]) + app.add_middleware(CORSMiddleware, allow_origins=["*"]) - headers = {"Origin": "https://example.org"} + @app.route("/") + def homepage(request): + return PlainTextResponse( + "Homepage", status_code=200, headers={"Vary": "Accept-Encoding"} + ) + + client = TestClient(app) + + response = client.get( + "/", headers={"Cookie": "foo=bar", "Origin": "https://someplace.org"} + ) + assert response.status_code == 200 + assert response.headers["vary"] == "Accept-Encoding, Origin" + + +def test_cors_vary_header_is_properly_set_when_allow_origins_is_not_wildcard(): + app = Starlette() + + app.add_middleware(CORSMiddleware, allow_origins=["https://example.org"]) @app.route("/") def homepage(request): @@ -260,13 +278,14 @@ def homepage(request): client = TestClient(app) - response = client.get("/", headers=headers) + response = client.get("/", headers={"Origin": "https://example.org"}) assert response.status_code == 200 assert response.headers["vary"] == "Accept-Encoding, Origin" def test_cors_allowed_origin_does_not_leak_between_credentialed_requests(): app = Starlette() + app.add_middleware( CORSMiddleware, allow_origins=["*"], allow_headers=["*"], allow_methods=["*"] ) From ba3fe37492595054c87b394cd0cd8885411f5888 Mon Sep 17 00:00:00 2001 From: Josh Wilson Date: Fri, 11 Dec 2020 02:17:25 -0800 Subject: [PATCH 2/3] Add type annotations --- starlette/middleware/cors.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/starlette/middleware/cors.py b/starlette/middleware/cors.py index 38b540318..582aa942a 100644 --- a/starlette/middleware/cors.py +++ b/starlette/middleware/cors.py @@ -157,16 +157,16 @@ async def send( # If request includes any cookie headers, then we must respond # with the specific origin instead of '*'. if self.allow_all_origins and has_cookie: - self.allow_explicit_origin(origin, headers) + self.allow_explicit_origin(headers, origin) # If we only allow specific origins, then we have to mirror back # the Origin header in the response. elif not self.allow_all_origins and self.is_allowed_origin(origin=origin): - self.allow_explicit_origin(origin, headers) + self.allow_explicit_origin(headers, origin) await send(message) @staticmethod - def allow_explicit_origin(origin, headers): + def allow_explicit_origin(headers: MutableHeaders, origin: str) -> None: headers["Access-Control-Allow-Origin"] = origin headers.add_vary_header("Origin") From 167398308da2db167fc139dd95a36c0b5ac7b2a2 Mon Sep 17 00:00:00 2001 From: Josh Wilson Date: Thu, 17 Dec 2020 14:34:22 -0800 Subject: [PATCH 3/3] Add test to ensure that the vary header does not contain origin if request is non-credentialed --- tests/middleware/test_cors.py | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/tests/middleware/test_cors.py b/tests/middleware/test_cors.py index 1082c3288..121902b0a 100644 --- a/tests/middleware/test_cors.py +++ b/tests/middleware/test_cors.py @@ -245,6 +245,24 @@ def homepage(request): assert response.headers["vary"] == "Origin" +def test_cors_vary_header_is_not_set_for_non_credentialed_request(): + app = Starlette() + + app.add_middleware(CORSMiddleware, allow_origins=["*"]) + + @app.route("/") + def homepage(request): + return PlainTextResponse( + "Homepage", status_code=200, headers={"Vary": "Accept-Encoding"} + ) + + client = TestClient(app) + + response = client.get("/", headers={"Origin": "https://someplace.org"}) + assert response.status_code == 200 + assert response.headers["vary"] == "Accept-Encoding" + + def test_cors_vary_header_is_properly_set_for_credentialed_request(): app = Starlette()