Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Set secure and httponly on formplayer_session cookie #35150

Merged
merged 3 commits into from
Sep 30, 2024
Merged

Set secure and httponly on formplayer_session cookie #35150

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7d98a3c
Make formplayer_session cookie httponly
nospame Sep 20, 2024
f189561
Reorder SecureCookiesMiddleware and add docstring
nospame Sep 20, 2024
1301079
Remove redundant constant
nospame Sep 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions corehq/apps/cloudcare/middleware.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
from django.conf import settings
from django.utils.deprecation import MiddlewareMixin


FORMPLAYER_SESSION_COOKIE_NAME = 'formplayer_session'
FORMPLAYER_SESSION_COOKIE_HTTPONLY = settings.SESSION_COOKIE_HTTPONLY
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not worth changing, but just curious what made you put this in a var rather than reference settings.SESSION_COOKIE_HTTPONLY directly?

Copy link
Contributor Author

@nospame nospame Sep 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think it'd be nicer to just reference it directly? For me it feels like it represents a different concept, that whether the Formplayer session cookie should be httponly can be inferred based on whether the Django session cookie is httponly, but doesn't have to be based on that. I also thought about just setting it to True or even using httponly=True directly in set_cookie() - I think in practice they're all the same.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wanted to check that the reasoning was along those lines. I don't have great context here, but from an initial read of this setting it seems like isn't that different of a concept? This is a session cookie, and we are using the SESSION_COOKIE_HTTPONLY settings when setting the httponly flag? It doesn't help that we don't use this setting anywhere else, but it seems like we should probably be referencing this same setting in other places where we set session cookies right?

Copy link
Contributor Author

@nospame nospame Sep 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it depends whether we're taking "session cookie" to mean "a cookie tied to the session" or "Django's sessionid cookie". I'm fine with the former interpretation, since in practice we do want it do be the same. 1301079



class CloudcareMiddleware(MiddlewareMixin):
Expand All @@ -27,4 +28,5 @@ def _set_formplayer_session_cookie(request, response):
couch_user = getattr(request, 'couch_user', None)
if couch_user:
if request.COOKIES.get(FORMPLAYER_SESSION_COOKIE_NAME) != couch_user.user_id:
response.set_cookie(FORMPLAYER_SESSION_COOKIE_NAME, couch_user.user_id)
response.set_cookie(FORMPLAYER_SESSION_COOKIE_NAME, couch_user.user_id,
httponly=FORMPLAYER_SESSION_COOKIE_HTTPONLY)
4 changes: 4 additions & 0 deletions corehq/middleware.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -340,6 +340,10 @@ def get_view_func(view_fn, view_kwargs):


class SecureCookiesMiddleware(MiddlewareMixin):
"""Sets `secure` flag for cookies on the response object.
Must be come before middleware that adds cookies, because of order and layering.
https://docs.djangoproject.com/en/4.2/topics/http/middleware/#middleware-order-and-layering
"""

def process_response(self, request, response):
if hasattr(response, 'cookies') and response.cookies:
Expand Down
3 changes: 1 addition & 2 deletions settings.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,7 @@

MIDDLEWARE = [
'corehq.middleware.NoCacheMiddleware',
'corehq.middleware.SecureCookiesMiddleware',
'corehq.middleware.SelectiveSessionMiddleware',
'django.middleware.locale.LocaleMiddleware',
'django.middleware.common.CommonMiddleware',
Expand All @@ -171,8 +172,6 @@
'no_exceptions.middleware.NoExceptionsMiddleware',
'corehq.apps.locations.middleware.LocationAccessMiddleware',
'corehq.apps.cloudcare.middleware.CloudcareMiddleware',
# middleware that adds cookies must come before SecureCookiesMiddleware
'corehq.middleware.SecureCookiesMiddleware',
'field_audit.middleware.FieldAuditMiddleware',
]

Expand Down
Loading