-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
Add SameSite setting for cookies #14900
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
Add SameSite setting for cookies #14900
Conversation
Fix go-gitea#5583 Signed-off-by: Andrew Thornton <art27@cantab.net>
Please check and test I've set these correctly - I am by no means certain I've set these right. There are also multiple places where we essentially have the same code duplicated multiple times so we should really do a small refactor sorting this out.
|
I've marked this as 1.14 as I suspect that between 1.14 being released and 1.15 being released we may find ourselves in a situation whereby this is forced upon us. Better to put it in now than later. |
@zeripath |
Signed-off-by: Andrew Thornton <art27@cantab.net>
Signed-off-by: Andrew Thornton <art27@cantab.net>
|
Codecov Report
@@ Coverage Diff @@
## master #14900 +/- ##
==========================================
- Coverage 42.09% 42.09% -0.01%
==========================================
Files 774 774
Lines 82796 82834 +38
==========================================
+ Hits 34853 34866 +13
- Misses 42269 42294 +25
Partials 5674 5674
Continue to review full report at Codecov.
|
Signed-off-by: Andrew Thornton <art27@cantab.net>
…ripath/gitea into fix-5583-samesite-setting-for-cookies
@CirnoT I recall you had thoughts on defaulting to strict and that we should be defaulting to lax? |
Just need to ensure that strict is the right default I have a feeling we should have lax actually |
Yup we should default to lax |
Certain cookies such as flash messages or transient session (2FA state, CAPTCHA or w/e) ones can be Strict as they refer to a single observed browsing session. The only one that needs to be Lax is the logon session. |
OK well I think we can differentiate the few other cookies later. I think setting everything to lax for the moment is going to be correct enough. |
Add SameSite setting for cookies and rationalise the cookie setting code. Switches SameSite to Lax by default.
There is a possible future extension of differentiating which cookies could be set at Strict by default but that is for a future PR.
Fix #5583
Signed-off-by: Andrew Thornton art27@cantab.net