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.

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

Change the name of the csrf token cookie #375

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Tomatosoup97
Copy link

@Tomatosoup97 Tomatosoup97 commented Aug 14, 2017

This change is pretty crucial for everyone that runs other django applications on subdomains on the same domain that is used as a kobotoolbox base.

Exemplary case:
kobo.example.com - Kobotoolbox app
other_app.example.com - Other, not related to the above django app

The kobo's cookie is established on wildcard *.example.com domain, thus, it is also accessible on the other_app. Assuming that the other_app is also a django application, it will break the POST request since wrong csrf token might be used for request authorization. Please also see the comment in the code for the refrence.

The patch is already tested on production machine, and appears to be working correctly. Please note that this change and according Kpi pull request should be submitted simultaneously, since submitting only one would break the app.

Related KPI change: kobotoolbox/kpi#1428

@dorey
Copy link
Contributor

dorey commented Aug 14, 2017

Is it possible to do this in a way that migrates existing CSRF tokens to the new name? I support this in theory but if we deploy this it would log out every user.

Such a change would likely have to take place in the django middleware.

@dorey
Copy link
Contributor

dorey commented Aug 14, 2017

Alternatively, the default CSRF token could be left unchanged, and you could find a way to pass the CSRF_TOKEN_KEY from settings through to the js layer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants