Outsource CSRF validation

[kesselb]

Summary

To validate a CSRF token the Request object needs CsrfTokenManager.

CsrfTokenManager is a heavy dependency.

flowchart TD
    CsrfTokenManager
    CsrfTokenManager-->CsrfTokenGenerator-->ISecureRandom
    CsrfTokenManager-->SessionStorage-->ISession-->IUserSession
    IUserSession-->OC\User\Session
    OC\User\Session-->OC\User\Manager
    OC\User\Session-->OCP\ISession
    OC\User\Session-->ISecureRandom
    OC\User\Session-->LoggerInterface
    OC\User\Session-->IEventDispatcher
    OC\User\Manager-->ICacheFactory-->ICache

TODO

• Merge refactor: migrate OC_EventSource to dependency injection #38386

Checklist

• Code is properly formatted
• Sign-off message is added to all commits
• Tests (unit, integration, api and/or acceptance) are included
• Screenshots before/after for front-end changes
• Documentation (manuals or wiki) has been updated or is not required
• Backports requested where applicable (ex: critical bugfixes)

[tcitworld]

Would refactoring OC\AppFramework\Http\Request::passesCSRFCheck to use the new CsrfValidator create more dependency hell? It would avoid to have the logic in two different places.

[kesselb]

It would avoid to have the logic in two different places.

I agree, having the logic in two different places it not an improvement.

The goal is to remove passesCSRFCheck and the CSRFTokenManager from IRequest.
I deprecated it, but it's public api and has to stay for 3 years major releases.

[nickvergessen]

But now all the classes need CsrfValidator and that still needs CsrfTokenManager?
Or did I miss the point?

[kesselb]

But now all the classes need CsrfValidator and that still needs CsrfTokenManager?

That's correct.

IRequest.passesCSRFCheck is only needed in a couple of places, but IRequest is injected in many more classes.
For Nextcloud 30, we can remove passesCSRFCheck and CsrfTokenManager from Request.

[kesselb]

I'm having the idea of making IRequest lighter for a while now.

We are injecting an IRequest instance in a couple of places. For example, the logger. It makes sense, to include some details from the request object (e.g. request id, ip address, etc.) in our logs, but that requires a CsrfTokenManager instance and therefore a working database connection and cache. As soon as db or cache is unavailable, you can't use the logger anymore.

Examples:
#25770 (comment)
#37458 (comment)

[nickvergessen]

For Nextcloud 30, we can remove passesCSRFCheck and CsrfTokenManager from Request.

3 years, not 3 versions xP so 36 😔

[nickvergessen]

But okay, got it now and see how it improves.

Could also remove the dependency already and just depend on \OC::$server->get() for the time being? 😅

[kesselb]

3 years, not 3 versions xP so 36 😔

😭

Could also remove the dependency already and just depend on \OC::$server->get() for the time being? 😅

Good idea 👍
I would like to do this as a follow-up.

[kesselb]

OCP interface

Now that you mentioned it, CsrfValidator needs an interface as well for forms¹ and user_saml².

Footnotes

1. https://github.com/nextcloud/forms/blob/ef5fe2095b764b0dbd32fe64aa7d013029850525/lib/Middleware/PublicCorsMiddleware.php#L75 ↩

2. https://github.com/nextcloud/user_saml/blob/56f3fcd2718ef40800ffd96bf5d4df21ef12b831/lib/Controller/SAMLController.php#L476 ↩

[ChristophWurst]

Suggested change

	* @deprecated 28.0.0 use CsrfValidator.validate instead
	* @deprecated 28.0.0 use \OCP\Security\CSRF\ICsrfValidator::validate instead

[ChristophWurst]

description?

[ChristophWurst]

Suggested change

	namespace OCP\Security\CSRF;
	namespace OCP\Security\Csrf;

maybe

https://stackoverflow.com/questions/2236807/java-naming-convention-with-acronyms

[kesselb]

Moving to 29