Skip to content

Commit 3cb076e

Browse files
kesselbkesselb
committed
feat: move csrf validation out of request
CsrfTokenManager is a heavy dependency that needs a user session and working memory cache. Signed-off-by: Daniel Kesselberg <[email protected]>
1 parent b6bb032 commit 3cb076e

File tree

20 files changed

+372
-71
lines changed

20 files changed

+372
-71
lines changed

‎apps/dav/appinfo/v1/caldav.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@
4343
\OC::$server->getRequest(),
4444
\OC::$server->getTwoFactorAuthManager(),
4545
\OC::$server->getBruteForceThrottler(),
46+
\OC::$server->get(\OC\Security\CSRF\CsrfValidator::class),
4647
'principals/'
4748
);
4849
$principalBackend = new Principal(

‎apps/dav/appinfo/v1/carddav.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@
4848
\OC::$server->getRequest(),
4949
\OC::$server->getTwoFactorAuthManager(),
5050
\OC::$server->getBruteForceThrottler(),
51+
\OC::$server->get(\OC\Security\CSRF\CsrfValidator::class),
5152
'principals/'
5253
);
5354
$principalBackend = new Principal(

‎apps/dav/appinfo/v1/webdav.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,7 @@
5959
\OC::$server->getRequest(),
6060
\OC::$server->getTwoFactorAuthManager(),
6161
\OC::$server->getBruteForceThrottler(),
62+
\OC::$server->get(\OC\Security\CSRF\CsrfValidator::class),
6263
'principals/'
6364
);
6465
$authPlugin = new \Sabre\DAV\Auth\Plugin($authBackend);

‎apps/dav/lib/Connector/Sabre/Auth.php‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@
3737
use OC\Authentication\Exceptions\PasswordLoginForbiddenException;
3838
use OC\Authentication\TwoFactorAuth\Manager;
3939
use OC\Security\Bruteforce\Throttler;
40+
use OC\Security\CSRF\CsrfValidator;
4041
use OC\User\LoginException;
4142
use OC\User\Session;
4243
use OCA\DAV\Connector\Sabre\Exception\PasswordLoginForbidden;
@@ -61,17 +62,21 @@ class Auth extends AbstractBasic {
6162
private Manager $twoFactorManager;
6263
private Throttler $throttler;
6364

65+
private CsrfValidator $csrfValidator;
66+
6467
public function __construct(ISession $session,
6568
Session $userSession,
6669
IRequest $request,
6770
Manager $twoFactorManager,
6871
Throttler $throttler,
72+
CsrfValidator $csrfValidator,
6973
string $principalPrefix = 'principals/users/') {
7074
$this->session = $session;
7175
$this->userSession = $userSession;
7276
$this->twoFactorManager = $twoFactorManager;
7377
$this->request = $request;
7478
$this->throttler = $throttler;
79+
$this->csrfValidator = $csrfValidator;
7580
$this->principalPrefix = $principalPrefix;
7681

7782
// setup realm
@@ -191,7 +196,7 @@ private function requiresCSRFCheck(): bool {
191196
private function auth(RequestInterface $request, ResponseInterface $response): array {
192197
$forcedLogout = false;
193198

194-
if (!$this->request->passesCSRFCheck() &&
199+
if (!$this->csrfValidator->validate($this->request) &&
195200
$this->requiresCSRFCheck()) {
196201
// In case of a fail with POST we need to recheck the credentials
197202
if ($this->request->getMethod() === 'POST') {

‎apps/dav/lib/Server.php‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,8 @@
3535
*/
3636
namespace OCA\DAV;
3737

38+
use OC\Security\Bruteforce\Throttler;
39+
use OC\Security\CSRF\CsrfValidator;
3840
use OCA\DAV\AppInfo\PluginManager;
3941
use OCA\DAV\BulkUpload\BulkUploadPlugin;
4042
use OCA\DAV\CalDAV\BirthdayService;
@@ -120,7 +122,8 @@ public function __construct(IRequest $request, string $baseUri) {
120122
\OC::$server->getUserSession(),
121123
\OC::$server->getRequest(),
122124
\OC::$server->getTwoFactorAuthManager(),
123-
\OC::$server->getBruteForceThrottler()
125+
\OC::$server->get(Throttler::class),
126+
\OC::$server->get(CsrfValidator::class)
124127
);
125128

126129
// Set URL explicitly due to reverse-proxy situations

‎apps/dav/tests/unit/Connector/Sabre/AuthTest.php‎

Lines changed: 17 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131

3232
use OC\Authentication\TwoFactorAuth\Manager;
3333
use OC\Security\Bruteforce\Throttler;
34+
use OC\Security\CSRF\CsrfValidator;
3435
use OC\User\Session;
3536
use OCP\IRequest;
3637
use OCP\ISession;
@@ -59,6 +60,7 @@ class AuthTest extends TestCase {
5960
private $twoFactorManager;
6061
/** @var Throttler */
6162
private $throttler;
63+
private CsrfValidator $csrfValidator;
6264

6365
protected function setUp(): void {
6466
parent::setUp();
@@ -74,12 +76,14 @@ protected function setUp(): void {
7476
$this->throttler = $this->getMockBuilder(Throttler::class)
7577
->disableOriginalConstructor()
7678
->getMock();
79+
$this->csrfValidator = $this->createMock(CsrfValidator::class);
7780
$this->auth = new \OCA\DAV\Connector\Sabre\Auth(
7881
$this->session,
7982
$this->userSession,
8083
$this->request,
8184
$this->twoFactorManager,
82-
$this->throttler
85+
$this->throttler,
86+
$this->csrfValidator,
8387
);
8488
}
8589

@@ -270,9 +274,9 @@ public function testAuthenticateAlreadyLoggedInWithoutCsrfTokenForNonGet(): void
270274
->expects($this->any())
271275
->method('getUser')
272276
->willReturn($user);
273-
$this->request
277+
$this->csrfValidator
274278
->expects($this->once())
275-
->method('passesCSRFCheck')
279+
->method('validate')
276280
->willReturn(false);
277281

278282
$expectedResponse = [
@@ -322,9 +326,9 @@ public function testAuthenticateAlreadyLoggedInWithoutCsrfTokenAndCorrectlyDavAu
322326
->expects($this->any())
323327
->method('getUser')
324328
->willReturn($user);
325-
$this->request
329+
$this->csrfValidator
326330
->expects($this->once())
327-
->method('passesCSRFCheck')
331+
->method('validate')
328332
->willReturn(false);
329333
$this->auth->check($request, $response);
330334
}
@@ -372,9 +376,9 @@ public function testAuthenticateAlreadyLoggedInWithoutTwoFactorChallengePassed()
372376
->expects($this->any())
373377
->method('getUser')
374378
->willReturn($user);
375-
$this->request
379+
$this->csrfValidator
376380
->expects($this->once())
377-
->method('passesCSRFCheck')
381+
->method('validate')
378382
->willReturn(true);
379383
$this->twoFactorManager->expects($this->once())
380384
->method('needsSecondFactor')
@@ -426,9 +430,9 @@ public function testAuthenticateAlreadyLoggedInWithoutCsrfTokenAndIncorrectlyDav
426430
->expects($this->any())
427431
->method('getUser')
428432
->willReturn($user);
429-
$this->request
433+
$this->csrfValidator
430434
->expects($this->once())
431-
->method('passesCSRFCheck')
435+
->method('validate')
432436
->willReturn(false);
433437
$this->auth->check($request, $response);
434438
}
@@ -472,9 +476,9 @@ public function testAuthenticateAlreadyLoggedInWithoutCsrfTokenForNonGetAndDeskt
472476
->expects($this->any())
473477
->method('getUser')
474478
->willReturn($user);
475-
$this->request
479+
$this->csrfValidator
476480
->expects($this->once())
477-
->method('passesCSRFCheck')
481+
->method('validate')
478482
->willReturn(false);
479483

480484
$this->auth->check($request, $response);
@@ -541,9 +545,9 @@ public function testAuthenticateAlreadyLoggedInWithCsrfTokenForGet(): void {
541545
->expects($this->any())
542546
->method('getUser')
543547
->willReturn($user);
544-
$this->request
548+
$this->csrfValidator
545549
->expects($this->once())
546-
->method('passesCSRFCheck')
550+
->method('validate')
547551
->willReturn(true);
548552

549553
$response = $this->auth->check($request, $response);

‎core/Controller/LoginController.php‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@
3838
use OC\Authentication\Login\LoginData;
3939
use OC\Authentication\WebAuthn\Manager as WebAuthnManager;
4040
use OC\Security\Bruteforce\Throttler;
41+
use OC\Security\CSRF\CsrfValidator;
4142
use OC\User\Session;
4243
use OC_App;
4344
use OCP\AppFramework\Controller;
@@ -75,6 +76,7 @@ class LoginController extends Controller {
7576
private WebAuthnManager $webAuthnManager;
7677
private IManager $manager;
7778
private IL10N $l10n;
79+
private CsrfValidator $csrfValidator;
7880

7981
public function __construct(?string $appName,
8082
IRequest $request,
@@ -88,7 +90,8 @@ public function __construct(?string $appName,
8890
IInitialStateService $initialStateService,
8991
WebAuthnManager $webAuthnManager,
9092
IManager $manager,
91-
IL10N $l10n) {
93+
IL10N $l10n,
94+
CsrfValidator $csrfValidator) {
9295
parent::__construct($appName, $request);
9396
$this->userManager = $userManager;
9497
$this->config = $config;
@@ -101,6 +104,7 @@ public function __construct(?string $appName,
101104
$this->webAuthnManager = $webAuthnManager;
102105
$this->manager = $manager;
103106
$this->l10n = $l10n;
107+
$this->csrfValidator = $csrfValidator;
104108
}
105109

106110
/**
@@ -296,7 +300,7 @@ public function tryLogin(Chain $loginChain,
296300
string $redirect_url = null,
297301
string $timezone = '',
298302
string $timezone_offset = ''): RedirectResponse {
299-
if (!$this->request->passesCSRFCheck()) {
303+
if (!$this->csrfValidator->validate($this->request)) {
300304
if ($this->userSession->isLoggedIn()) {
301305
// If the user is already logged in and the CSRF check does not pass then
302306
// simply redirect the user to the correct page as required. This is the

‎lib/composer/composer/autoload_classmap.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1550,6 +1550,7 @@
15501550
'OC\\Security\\CSRF\\CsrfToken' => $baseDir . '/lib/private/Security/CSRF/CsrfToken.php',
15511551
'OC\\Security\\CSRF\\CsrfTokenGenerator' => $baseDir . '/lib/private/Security/CSRF/CsrfTokenGenerator.php',
15521552
'OC\\Security\\CSRF\\CsrfTokenManager' => $baseDir . '/lib/private/Security/CSRF/CsrfTokenManager.php',
1553+
'OC\\Security\\CSRF\\CsrfValidator' => $baseDir . '/lib/private/Security/CSRF/CsrfValidator.php',
15531554
'OC\\Security\\CSRF\\TokenStorage\\SessionStorage' => $baseDir . '/lib/private/Security/CSRF/TokenStorage/SessionStorage.php',
15541555
'OC\\Security\\Certificate' => $baseDir . '/lib/private/Security/Certificate.php',
15551556
'OC\\Security\\CertificateManager' => $baseDir . '/lib/private/Security/CertificateManager.php',

‎lib/composer/composer/autoload_static.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1583,6 +1583,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2
15831583
'OC\\Security\\CSRF\\CsrfToken' => __DIR__ . '/../../..' . '/lib/private/Security/CSRF/CsrfToken.php',
15841584
'OC\\Security\\CSRF\\CsrfTokenGenerator' => __DIR__ . '/../../..' . '/lib/private/Security/CSRF/CsrfTokenGenerator.php',
15851585
'OC\\Security\\CSRF\\CsrfTokenManager' => __DIR__ . '/../../..' . '/lib/private/Security/CSRF/CsrfTokenManager.php',
1586+
'OC\\Security\\CSRF\\CsrfValidator' => __DIR__ . '/../../..' . '/lib/private/Security/CSRF/CsrfValidator.php',
15861587
'OC\\Security\\CSRF\\TokenStorage\\SessionStorage' => __DIR__ . '/../../..' . '/lib/private/Security/CSRF/TokenStorage/SessionStorage.php',
15871588
'OC\\Security\\Certificate' => __DIR__ . '/../../..' . '/lib/private/Security/Certificate.php',
15881589
'OC\\Security\\CertificateManager' => __DIR__ . '/../../..' . '/lib/private/Security/CertificateManager.php',

‎lib/private/AppFramework/DependencyInjection/DIContainer.php‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -233,7 +233,8 @@ public function __construct(string $appName, array $urlParams = [], ServerContai
233233
$c->get(IRequest::class),
234234
$c->get(IControllerMethodReflector::class),
235235
$c->get(IUserSession::class),
236-
$c->get(OC\Security\Bruteforce\Throttler::class)
236+
$c->get(OC\Security\Bruteforce\Throttler::class),
237+
$c->get(OC\Security\CSRF\CsrfValidator::class),
237238
)
238239
);
239240
$dispatcher->registerMiddleware(
@@ -257,7 +258,8 @@ public function __construct(string $appName, array $urlParams = [], ServerContai
257258
$server->getAppManager(),
258259
$server->getL10N('lib'),
259260
$c->get(AuthorizedGroupMapper::class),
260-
$server->get(IUserSession::class)
261+
$server->get(IUserSession::class),
262+
$c->get(OC\Security\CSRF\CsrfValidator::class),
261263
);
262264
$dispatcher->registerMiddleware($securityMiddleware);
263265
$dispatcher->registerMiddleware(

0 commit comments

Comments
 (0)