Skip to content

Conversation

@silverwind
Copy link
Member

@silverwind silverwind commented Dec 17, 2025

Removes the CSRF cookie in favor of CrossOriginProtection which relies purely on HTTP headers.

Fixes: #11188
Fixes: #30333
Helps: #35107

TODOs:

  • Fix tests
  • Ideally add tests to validates the protection

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 17, 2025
@github-actions github-actions bot added modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files modifies/frontend labels Dec 17, 2025
@github-actions github-actions bot added the docs-update-needed The document needs to be updated synchronously label Dec 17, 2025
@silverwind silverwind changed the title Replace csrf cookie with Go 1.25 CrossOriginProtection Replace csrf cookie with CrossOriginProtection Dec 17, 2025
@silverwind silverwind changed the title Replace csrf cookie with CrossOriginProtection Replace CSRF cookie with CrossOriginProtection Dec 17, 2025
@silverwind silverwind marked this pull request as draft December 17, 2025 23:14
@silverwind
Copy link
Member Author

silverwind commented Dec 17, 2025

Test failures need investigation. I think I was quite thorough with this removal, rg csrf returns clean.

@silverwind silverwind added the pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! label Dec 18, 2025
@silverwind
Copy link
Member Author

silverwind commented Dec 18, 2025

Unexpectedly, the fix in 1eb058d worked to fix the last issue. I think there may be a underlying bug why this test is sensitive to a GET request, but as far as this PR is concerned, its ok.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe use a new test for CrossOriginProtection

Copy link
Member Author

@silverwind silverwind Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I would still like to add a integration test that leverages Sec-Fetch-Site to validate this protection.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Dec 18, 2025

⚠️ BREAKING ⚠️

The _crsf cookie will no longer be set and is no longer necessary to be sent. GET requests that were used to retrieve it are no longer neccessary.

"breaking" usually means end users or site admins must do something, or experience something totally different.

But for this one, I think it isn't really "breaking". WDYT?


And, the TODOs are not necessary (or, we shouldn't "add additional trusted origins", there is no such use case)

@wxiaoguang wxiaoguang dismissed their stale review December 18, 2025 11:23

Leave for a while. The current logic isn't right.

@GiteaBot GiteaBot added lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Dec 18, 2025
@silverwind
Copy link
Member Author

I think the cors handler could indicate in the request context that it had a match with a boolean flag, and then the COP handler could act on that flag and be disabled.

@silverwind
Copy link
Member Author

silverwind commented Dec 18, 2025

I'm not sure your AI answer is right. Go's protection relies on Sec-Fetch-Site or Origin headers and I think one, if not both headers will be present on CORS request, at least for the unsafe methods. There is even a Sec-Fetch-Mode: cors, which further reinforces my assumption.

So I'd lean towards having all endpoints protected, without exception. Restrictions can be relaxed later if issues are demonstrated. I will push again the full strict settings.

@wxiaoguang
Copy link
Contributor

I'm not sure your AI answer is right.

AI's answer is right, I asked a general question.

Go's protection relies on Sec-Fetch-Site or Origin headers and I think one, if not both headers will be present on CORS request, at least for the unsafe methods.

That's Golang's implementation. And you MUST make CrossOriginProtection work with CORS config, but not just make a new default CrossOriginProtection.

image

@silverwind
Copy link
Member Author

I don't mind having unprotected CORS routes but that AI answer seems nonsensical to me:

If your server implements a policy that rejects Sec-Fetch-Site: cross-site for certain endpoints, the server can block it (e.g., respond 403) even if CORS would otherwise allow it.

My understanding is that all requests, including CORS preflights will send one of Sec-Fetch-Site or Origin and when I ask google ai, it confirms this:

image

@wxiaoguang
Copy link
Contributor

That's Golang's implementation. And you MUST make CrossOriginProtection work with CORS config, but not just make a new default CrossOriginProtection.

See my comment above. "CrossOriginProtection" must accept (trust) that origin, if I understand correctly.

@silverwind
Copy link
Member Author

I've read a bit more on these topics, these seem like good resources:

https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.html
https://www.alexedwards.net/blog/preventing-csrf-in-go

From what I gather, these header checks alone are not recommended, but header checks combined with a SameSite cookie (lax or strict) is recommended as adequate protection.

@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 Dec 25, 2025
@wxiaoguang wxiaoguang marked this pull request as ready for review December 25, 2025 05:08
@wxiaoguang wxiaoguang added this to the 1.26.0 milestone Dec 25, 2025
Co-authored-by: Lunny Xiao <[email protected]>
Signed-off-by: wxiaoguang <[email protected]>
@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 Dec 25, 2025
@lafriks lafriks merged commit 42d2949 into go-gitea:main Dec 25, 2025
24 checks passed
@wxiaoguang wxiaoguang deleted the nocsrf branch December 25, 2025 11:51
zjjhot added a commit to zjjhot/gitea that referenced this pull request Dec 26, 2025
* giteaofficial/main:
  Fix panic when get editor config file (go-gitea#36241)
  Refactor compare router param parse (go-gitea#36105)
  [skip ci] Updated translations via Crowdin
  Use flatten translation keys (go-gitea#36225)
  Replace CSRF cookie with `CrossOriginProtection` (go-gitea#36183)
  Remove fomantic form module (go-gitea#36222)
  Fix panic in blame view when a file has only a single commit (go-gitea#36230)
  fix: spelling error in migrate-storage cmd utility (go-gitea#36226)

# Conflicts:
#	templates/user/settings/security/twofa.tmpl
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs-update-needed The document needs to be updated synchronously lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/frontend modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files type/refactoring Existing code has been cleaned up. There should be no new functionality.

Projects

None yet

5 participants