-
Notifications
You must be signed in to change notification settings - Fork 766
The current csrftoken parser breaks on multiple crsftokens #783
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
Conversation
For example: `Cookie: csrftoken=asdfasd; sessionid=asdfasdf; csrftoken=qwertyqwerty` This can happen when multiple sessions, or multiple csrftokens for different paths.
related: #585 |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@allen-munsch can you elaborate more on why there would be multiple csrf tokens set?My understanding is that there should only be 1 token set at any time.
headers['X-CSRFToken'] = cookies.csrftoken.pop(); | ||
console.log('retry', headers) | ||
return getFetch(headers); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There could be other reasons why the GraphQL endpoint doesn't return valid JSON so always assuming it's to do with a csrf token issue doesn't feel right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jkimbo see: https://stackoverflow.com/questions/576535/cookie-path-and-its-accessibility-to-subfolder-pages
From what I understand, the cookie sub path /2/m/2
would have both it's subpath cookie and the root path cookie. /
.
The two paths have different database multitenancy requirements. It would probably be more accurate to read the cookie path directly, instead of just reading, checking them all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I understand that cookies can have different paths but looking at the Django source code the csrf cookie should be set to a path that is defined in the settings: https://github.com/django/django/blob/5a68f024987e6d16c2626a31bf653a2edddea579/django/middleware/csrf.py#L191
So I don't think it's possible to have 2 csrf cookies at the same time and I'm wondering how that came about?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
endpoints per multi tenancy. localhost:8000/:multitenantid/endpoints/here
so multiple users can get different csrftoken cookies on a single machine ... it's not the default of django, but it is a real use case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I didn't realise that was possible! Could you not host the graphql endpoint per tenant as well or those that not work?
Which causes 403 errors.
For example:
Cookie: csrftoken=asdfasd; sessionid=asdfasdf; csrftoken=qwertyqwerty
This can happen when multiple csrftokens for different paths.