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

Add SameSite setting for cookies #14900

Merged

Conversation

zeripath
Copy link
Contributor

@zeripath zeripath commented Mar 5, 2021

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

Fix go-gitea#5583

Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath zeripath added the type/enhancement An improvement of existing functionality label Mar 5, 2021
@zeripath zeripath added this to the 1.14.0 milestone Mar 5, 2021
@zeripath
Copy link
Contributor Author

zeripath commented Mar 5, 2021

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 also taken the liberty of removing the crazy 1<<31-1 time from the lang attribute from the bottom menu. Removed as per comments

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 5, 2021
@zeripath
Copy link
Contributor Author

zeripath commented Mar 5, 2021

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.

@6543
Copy link
Member

6543 commented Mar 6, 2021

@zeripath panic: session.Options.SameSite: readUint64: unexpected character: , error found in #10 byte of ...|ameSite":"strict"}|..., bigger context ...|ime":86400,"Secure":false,"Domain":"","SameSite":"strict"}|...

Signed-off-by: Andrew Thornton <art27@cantab.net>
Signed-off-by: Andrew Thornton <art27@cantab.net>
@lafriks
Copy link
Member

lafriks commented Mar 6, 2021

It should also set path to cookies already does

6543
6543 approved these changes Mar 6, 2021
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Mar 6, 2021
@codecov-io
Copy link

Codecov Report

Merging #14900 (f9dec41) into master (f4efa10) will decrease coverage by 0.00%.
The diff coverage is 29.68%.

Impacted file tree graph

@@            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              
Impacted Files Coverage Δ
modules/auth/sso/sso.go 21.87% <0.00%> (-5.05%) ⬇️
modules/web/middleware/locale.go 52.17% <0.00%> (-18.42%) ⬇️
routers/user/auth_openid.go 0.00% <0.00%> (ø)
modules/context/auth.go 21.18% <33.33%> (+1.00%) ⬆️
routers/user/auth.go 12.20% <41.17%> (ø)
modules/setting/session.go 78.57% <50.00%> (-11.43%) ⬇️
modules/context/context.go 65.62% <100.00%> (+0.29%) ⬆️
routers/user/setting/profile.go 40.00% <100.00%> (ø)
modules/util/timer.go 85.71% <0.00%> (-14.29%) ⬇️
modules/queue/workerpool.go 58.77% <0.00%> (-1.23%) ⬇️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f4efa10...f9dec41. Read the comment docs.

Signed-off-by: Andrew Thornton <art27@cantab.net>
…ripath/gitea into fix-5583-samesite-setting-for-cookies
@6543 6543 added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Mar 6, 2021
@zeripath zeripath requested a review from lafriks March 6, 2021 20:25
zeripath and others added 2 commits March 6, 2021 22:02
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Mar 6, 2021
@zeripath
Copy link
Contributor Author

zeripath commented Mar 6, 2021

@CirnoT I recall you had thoughts on defaulting to strict and that we should be defaulting to lax?

@zeripath zeripath added the status/blocked This PR cannot be merged yet, i.e. because it depends on another unmerged PR label Mar 6, 2021
@zeripath
Copy link
Contributor Author

zeripath commented Mar 6, 2021

Just need to ensure that strict is the right default I have a feeling we should have lax actually

@zeripath
Copy link
Contributor Author

zeripath commented Mar 6, 2021

Yup we should default to lax

custom/conf/app.example.ini Outdated Show resolved Hide resolved
docs/content/doc/advanced/config-cheat-sheet.en-us.md Outdated Show resolved Hide resolved
modules/setting/session.go Outdated Show resolved Hide resolved
modules/setting/session.go Outdated Show resolved Hide resolved
modules/setting/session.go Show resolved Hide resolved
modules/setting/session.go Outdated Show resolved Hide resolved
modules/setting/session.go Outdated Show resolved Hide resolved
modules/setting/session.go Outdated Show resolved Hide resolved
@zeripath zeripath removed the status/blocked This PR cannot be merged yet, i.e. because it depends on another unmerged PR label Mar 7, 2021
@zeripath zeripath closed this Mar 7, 2021
@zeripath zeripath reopened this Mar 7, 2021
@CirnoT
Copy link
Contributor

CirnoT commented Mar 7, 2021

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.

@zeripath
Copy link
Contributor Author

zeripath commented Mar 7, 2021

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.

@zeripath zeripath merged commit 9b261f5 into go-gitea:master Mar 7, 2021
@zeripath zeripath deleted the fix-5583-samesite-setting-for-cookies branch March 7, 2021 08:12
@go-gitea go-gitea locked and limited conversation to collaborators May 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/enhancement An improvement of existing functionality type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SameSite Setting for Cookies
6 participants