Add Origin to Vary header on credentialed CORS response (#1111)

* Add Origin to Vary header on credentialed CORS response

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.

* Add type annotations

* Add test to ensure that the vary header does not contain origin if request is non-credentialed

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(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):
-            headers["Access-Control-Allow-Origin"] = origin
-            headers.add_vary_header("Origin")
+            self.allow_explicit_origin(headers, origin)
+
         await send(message)
+
+    @staticmethod
+    def allow_explicit_origin(headers: MutableHeaders, origin: str) -> None:
+        headers["Access-Control-Allow-Origin"] = origin
+        headers.add_vary_header("Origin")

tests/middleware/test_cors.py

@@ -245,12 +245,28 @@ def homepage(request):
     assert response.headers["vary"] == "Origin"
 
 
-def test_cors_vary_header_is_properly_set():
+def test_cors_vary_header_is_not_set_for_non_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={"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()
+
+    app.add_middleware(CORSMiddleware, allow_origins=["*"])
 
     @app.route("/")
     def homepage(request):
@@ -260,13 +276,34 @@ def homepage(request):
 
     client = TestClient(app)
 
-    response = client.get("/", headers=headers)
+    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):
+        return PlainTextResponse(
+            "Homepage", status_code=200, headers={"Vary": "Accept-Encoding"}
+        )
+
+    client = TestClient(app)
+
+    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=["*"]
     )