Skip to content

Commit

Permalink
[gear] Make csrf cookie samesite=strict (#14180)
Browse files Browse the repository at this point in the history
Currently, the `_csrf` cookie is made available to all subdomains of
`.hail.is`. This means that if I first visit `batch.hail.is` I get a
`_csrf` cookie set for `.hail.is`. That cookie is then reused if I visit
`ci.hail.is`. Even more awkward, the same value of the cookie will get
reused if I then visit `batch.azure.hail.is`. This isn't that big of a
deal, these can all be considered part of the same application that the
hail team delivers and secures, but it is very little work to set
stricter bounds on where this cookie is sent. By removing the `domain`
attribute and using `samesite='strict'`, the cookie's domain will be set
by the browser to the domain of the request whose response included the
`Set-Cookie` header, e.g. `batch.hail.is` or `internal.hail.is`.
`Strict` mode then ensures that the cookie will only be sent to that
exact domain, meaning that each application is guaranteed to receive the
`_csrf` token that it itself delivered, and a `_csrf` token from CI
cannot be used to take actions against Batch.

This should not have an adverse impact on existing users' browser
sessions. In `render_template` we preserve the value of an existing
`_csrf` cookie so this change should do the following:
- Logged in user visits a page with an existing widely scoped
(`.hail.is`) `_csrf` cookie
- The server returns a `Set-Cookie` header with a new `_csrf` cookie for
strictly the `batch.hail.is` domain but with the same token value as the
original `_csrf` cookie
- The user now has two cookies and the browser could send either one on
a given request, but it does not matter because they have the same value
- If the user logs out and back in, their old widely scoped cookie will
be cleared and they only get the strict cookie from now on.
  • Loading branch information
daniel-goldstein authored Jan 24, 2024
1 parent 42a072e commit 0411c89
Show file tree
Hide file tree
Showing 2 changed files with 2 additions and 5 deletions.
2 changes: 1 addition & 1 deletion devbin/dev_proxy.py
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ async def render_html(request: web.Request, context: dict):
# Make links point back to the local dev server and not use
# the dev namespace path rewrite shenanigans.
context['page_context']['base_path'] = ''
return await render_template(SERVICE, request, **context, cookie_domain='localhost:8000')
return await render_template(SERVICE, request, **context)


async def on_startup(app: web.Application):
Expand Down
5 changes: 1 addition & 4 deletions web_common/web_common/web_common.py
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,6 @@ async def render_template(
userdata: Optional[UserData],
file: str,
page_context: Dict[str, Any],
*,
cookie_domain: Optional[str] = None,
) -> web.Response:
if request.headers.get('x-hail-return-jinja-context'):
if userdata and userdata['is_developer']:
Expand All @@ -98,6 +96,5 @@ async def render_template(
context['csrf_token'] = csrf_token

response = aiohttp_jinja2.render_template(file, request, context)
domain = cookie_domain or deploy_config._domain
response.set_cookie('_csrf', csrf_token, domain=domain, secure=True, httponly=True)
response.set_cookie('_csrf', csrf_token, secure=True, httponly=True, samesite='strict')
return response

0 comments on commit 0411c89

Please sign in to comment.