Replace CSRF cookie with CrossOriginProtection

[silverwind]

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

[silverwind]

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

[silverwind]

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.

[lunny]

Maybe use a new test for CrossOriginProtection

[silverwind]

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

[wxiaoguang]

⚠️ 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]

I wonder if it's even necessary to disable COP on any endpoints. It's only active for browsers and if browser headers are not present, the protection will just allow the request which means any API client will be unaffected.

I also consider that's unnecessary to disable CrossOriginProtection. So I think we can remove optSignInAnyOrigin and only use optSignIn

Not right, see below

[silverwind]

Yeah I think we can go strict and have it enabled on all endpoints. If issues arise, we can think about solutions.

[silverwind]

Done in da28335, now it's always enabled unconditionally.

[wxiaoguang]

Done in da28335, now it's always enabled unconditionally.

Wait, it will cause problems with CORS, according to AI:

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.

So we still need to disable CrossOriginProtection for the CORS endpoints.

[silverwind]

For the record, if there are issues, COP offers 2 ways to disable it conditionally:

• https://pkg.go.dev/net/http#CrossOriginProtection.AddInsecureBypassPattern
• https://pkg.go.dev/net/http#CrossOriginProtection.AddTrustedOrigin

Both could be made configurable, if needed.

[silverwind]

So we still need to disable CrossOriginProtection for the CORS endpoints.

I'm not sure but yes sounds plausible that CORS is an exception.

[silverwind]

I've re-added optSignInAnyOrigin and added it anywhere where optionsCorsHandler is active. Maybe the option could also be combined into optionsCorsHandler but I have no idea how to do that.

[silverwind]

I think combining these two options is still not good because those route groups also match on non-CORS requests but we probably only want to disable the protection when the request was identified as CORS, e.g. when corsHandler is non-nil inside the optionsCorsHandler callback.

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

[silverwind]

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]

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]

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.

[silverwind]

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:

[wxiaoguang]

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]

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.