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

refactor session cookie domain handling #5051

Closed
davidism opened this issue Apr 12, 2023 · 0 comments · Fixed by #5054
Closed

refactor session cookie domain handling #5051

davidism opened this issue Apr 12, 2023 · 0 comments · Fixed by #5054
Milestone

Comments

@davidism
Copy link
Member

davidism commented Apr 12, 2023

If SESSION_COOKIE_DOMAIN is not set, it currently falls back to SERVER_NAME. However, setting a domain is actually less secure than not setting one. When not set, the browser will restrict the cookie to the exact domain. When set, it will allow exact as well as any subdomain.

If SERVER_NAME is not set, that's fine, but it's likely to be set for other reasons.

We should not fall back to SERVER_NAME. Either the session is specific to the domain that set it, or it's explicitly configured to be shared.

We currently add a leading dot . to the domain if the app is at the root path. This was an old way to signal subdomain support, but is not how things work anymore. Modern browsers ignore a leading dot, and only use exact matching if the domain is not set.

Additionally, if the domain does not contain a dot . (localhost) we currently don't set it and show a warning. We also show a warning for IP addresses. This was apparently a limitation of browsers in 2017, but no longer appears to be an issue.

#1784 discussed the SERVER_NAME fallback before, and #2282 was where I refactored this in 2017.

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

Successfully merging a pull request may close this issue.

1 participant