kaxil commented on code in PR #75411:
URL: https://github.com/apache/airflow/pull/75411#discussion_r4354587827
##########
airflow-core/src/airflow/api_fastapi/auth/middlewares/refresh_token.py:
##########
@@ -102,12 +102,16 @@ async def send_with_cookies(message: Message) -> None:
if message["type"] == "http.response.start":
response_started = True
headers = MutableHeaders(scope=message)
- cookie_values = cleanup_cookies
- # The token set by the route itself wins: appending ours after
it would override it.
- if token_cookie and not any(
- value.startswith(f"{COOKIE_NAME_JWT_TOKEN}=") for value in
headers.getlist("set-cookie")
- ):
- cookie_values = [token_cookie, *cleanup_cookies]
+ route_cookie_names = {value.split("=", 1)[0] for value in
headers.getlist("set-cookie")}
+ cookie_values = []
+ # Cookies set by the route itself win: appending ours after
them would override them.
Review Comment:
Could the `get_extra_cookies` docstring say this too? It still says an empty
value clears the cookie, which no longer holds when the route sets a cookie
with that name, and the docstring is all a subclass author has to go on.
##########
airflow-core/src/airflow/api_fastapi/auth/middlewares/refresh_token.py:
##########
@@ -102,12 +102,16 @@ async def send_with_cookies(message: Message) -> None:
if message["type"] == "http.response.start":
response_started = True
headers = MutableHeaders(scope=message)
- cookie_values = cleanup_cookies
- # The token set by the route itself wins: appending ours after
it would override it.
- if token_cookie and not any(
- value.startswith(f"{COOKIE_NAME_JWT_TOKEN}=") for value in
headers.getlist("set-cookie")
- ):
- cookie_values = [token_cookie, *cleanup_cookies]
+ route_cookie_names = {value.split("=", 1)[0] for value in
headers.getlist("set-cookie")}
+ cookie_values = []
+ # Cookies set by the route itself win: appending ours after
them would override them.
+ if token_cookie and COOKIE_NAME_JWT_TOKEN not in
route_cookie_names:
+ cookie_values.append(token_cookie)
+ for value in cleanup_cookies:
+ name = value.split("=", 1)[0]
+ # The root-path token cleanup targets "/", not the cookie
path the route sets the token on.
+ if name == COOKIE_NAME_JWT_TOKEN or name not in
route_cookie_names:
Review Comment:
Browsers key a cookie on name and path, but this only compares names. So
with a subpath `base_url`, a route that deletes `foo` at `Path=/` while
`get_extra_cookies` returns `{"foo": ""}` now loses our clear at the cookie
path, and the stale `foo` sticks around (before this change both headers went
out). The `name == COOKIE_NAME_JWT_TOKEN` carve-out is there for the same
reason.
What if we put our headers before the route's instead of after? For the same
name and path the later Set-Cookie wins, so I think that gets you route-wins
without parsing names at all.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
