Skip to content

Commit a0dd76d

Browse files
kesselbkesselb
committed
feat: move csrf validation out of request
Signed-off-by: Daniel Kesselberg <mail@danielkesselberg.de>
1 parent a5fd623 commit a0dd76d

File tree

24 files changed

+371
-71
lines changed

24 files changed

+371
-71
lines changed

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
use OCA\DAV\Connector\Sabre\MaintenancePlugin;
1717
use OCA\DAV\Connector\Sabre\Principal;
1818
use OCP\Accounts\IAccountManager;
19+
use OCP\Security\CSRF\ICsrfValidator;
1920
use Psr\Log\LoggerInterface;
2021

2122
$authBackend = new Auth(
@@ -24,6 +25,7 @@
2425
\OC::$server->getRequest(),
2526
\OC::$server->getTwoFactorAuthManager(),
2627
\OC::$server->getBruteForceThrottler(),
28+
\OC::$server->get(ICsrfValidator::class),
2729
'principals/'
2830
);
2931
$principalBackend = new Principal(

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
use OCA\DAV\Connector\Sabre\Principal;
1919
use OCP\Accounts\IAccountManager;
2020
use OCP\App\IAppManager;
21+
use OCP\Security\CSRF\ICsrfValidator;
2122
use Psr\Log\LoggerInterface;
2223
use Sabre\CardDAV\Plugin;
2324

@@ -27,6 +28,7 @@
2728
\OC::$server->getRequest(),
2829
\OC::$server->getTwoFactorAuthManager(),
2930
\OC::$server->getBruteForceThrottler(),
31+
\OC::$server->get(ICsrfValidator::class),
3032
'principals/'
3133
);
3234
$principalBackend = new Principal(

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,8 @@
55
* SPDX-FileCopyrightText: 2016 ownCloud, Inc.
66
* SPDX-License-Identifier: AGPL-3.0-only
77
*/
8+
9+
use OCP\Security\CSRF\ICsrfValidator;
810
use Psr\Log\LoggerInterface;
911

1012
// no php execution timeout for webdav
@@ -38,6 +40,7 @@
3840
\OC::$server->getRequest(),
3941
\OC::$server->getTwoFactorAuthManager(),
4042
\OC::$server->getBruteForceThrottler(),
43+
\OC::$server->get(ICsrfValidator::class),
4144
'principals/'
4245
);
4346
$authPlugin = new \Sabre\DAV\Auth\Plugin($authBackend);

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

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
use OCP\ISession;
1818
use OCP\Security\Bruteforce\IThrottler;
1919
use OCP\Security\Bruteforce\MaxDelayReached;
20+
use OCP\Security\CSRF\ICsrfValidator;
2021
use Psr\Log\LoggerInterface;
2122
use Sabre\DAV\Auth\Backend\AbstractBasic;
2223
use Sabre\DAV\Exception\NotAuthenticated;
@@ -39,6 +40,7 @@ public function __construct(ISession $session,
3940
IRequest $request,
4041
Manager $twoFactorManager,
4142
IThrottler $throttler,
43+
private ICsrfValidator $csrfValidator,
4244
string $principalPrefix = 'principals/users/') {
4345
$this->session = $session;
4446
$this->userSession = $userSession;
@@ -164,7 +166,7 @@ private function requiresCSRFCheck(): bool {
164166
private function auth(RequestInterface $request, ResponseInterface $response): array {
165167
$forcedLogout = false;
166168

167-
if (!$this->request->passesCSRFCheck() &&
169+
if (!$this->csrfValidator->validate($this->request) &&
168170
$this->requiresCSRFCheck()) {
169171
// In case of a fail with POST we need to recheck the credentials
170172
if ($this->request->getMethod() === 'POST') {

‎apps/dav/lib/Server.php‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,8 @@
5555
use OCP\IRequest;
5656
use OCP\Profiler\IProfiler;
5757
use OCP\SabrePluginEvent;
58+
use OCP\Security\Bruteforce\IThrottler;
59+
use OCP\Security\CSRF\ICsrfValidator;
5860
use Psr\Log\LoggerInterface;
5961
use Sabre\CardDAV\VCFExportPlugin;
6062
use Sabre\DAV\Auth\Plugin;
@@ -95,7 +97,8 @@ public function __construct(IRequest $request, string $baseUri) {
9597
\OC::$server->getUserSession(),
9698
\OC::$server->getRequest(),
9799
\OC::$server->getTwoFactorAuthManager(),
98-
\OC::$server->getBruteForceThrottler()
100+
\OC::$server->get(IThrottler::class),
101+
\OC::$server->get(ICsrfValidator::class)
99102
);
100103

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

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

Lines changed: 18 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -8,11 +8,13 @@
88
namespace OCA\DAV\Tests\unit\Connector\Sabre;
99

1010
use OC\Authentication\TwoFactorAuth\Manager;
11+
use OC\Security\CSRF\CsrfValidator;
1112
use OC\User\Session;
1213
use OCP\IRequest;
1314
use OCP\ISession;
1415
use OCP\IUser;
1516
use OCP\Security\Bruteforce\IThrottler;
17+
use OCP\Security\CSRF\ICsrfValidator;
1618
use Sabre\DAV\Server;
1719
use Sabre\HTTP\RequestInterface;
1820
use Sabre\HTTP\ResponseInterface;
@@ -37,6 +39,7 @@ class AuthTest extends TestCase {
3739
private $twoFactorManager;
3840
/** @var IThrottler */
3941
private $throttler;
42+
private ICsrfValidator $csrfValidator;
4043

4144
protected function setUp(): void {
4245
parent::setUp();
@@ -52,12 +55,14 @@ protected function setUp(): void {
5255
$this->throttler = $this->getMockBuilder(IThrottler::class)
5356
->disableOriginalConstructor()
5457
->getMock();
58+
$this->csrfValidator = $this->createMock(ICsrfValidator::class);
5559
$this->auth = new \OCA\DAV\Connector\Sabre\Auth(
5660
$this->session,
5761
$this->userSession,
5862
$this->request,
5963
$this->twoFactorManager,
60-
$this->throttler
64+
$this->throttler,
65+
$this->csrfValidator,
6166
);
6267
}
6368

@@ -248,9 +253,9 @@ public function testAuthenticateAlreadyLoggedInWithoutCsrfTokenForNonGet(): void
248253
->expects($this->any())
249254
->method('getUser')
250255
->willReturn($user);
251-
$this->request
256+
$this->csrfValidator
252257
->expects($this->once())
253-
->method('passesCSRFCheck')
258+
->method('validate')
254259
->willReturn(false);
255260

256261
$expectedResponse = [
@@ -295,9 +300,9 @@ public function testAuthenticateAlreadyLoggedInWithoutCsrfTokenAndCorrectlyDavAu
295300
->expects($this->any())
296301
->method('getUser')
297302
->willReturn($user);
298-
$this->request
303+
$this->csrfValidator
299304
->expects($this->once())
300-
->method('passesCSRFCheck')
305+
->method('validate')
301306
->willReturn(false);
302307
$this->auth->check($request, $response);
303308
}
@@ -340,9 +345,9 @@ public function testAuthenticateAlreadyLoggedInWithoutTwoFactorChallengePassed()
340345
->expects($this->any())
341346
->method('getUser')
342347
->willReturn($user);
343-
$this->request
348+
$this->csrfValidator
344349
->expects($this->once())
345-
->method('passesCSRFCheck')
350+
->method('validate')
346351
->willReturn(true);
347352
$this->twoFactorManager->expects($this->once())
348353
->method('needsSecondFactor')
@@ -389,9 +394,9 @@ public function testAuthenticateAlreadyLoggedInWithoutCsrfTokenAndIncorrectlyDav
389394
->expects($this->any())
390395
->method('getUser')
391396
->willReturn($user);
392-
$this->request
397+
$this->csrfValidator
393398
->expects($this->once())
394-
->method('passesCSRFCheck')
399+
->method('validate')
395400
->willReturn(false);
396401
$this->auth->check($request, $response);
397402
}
@@ -430,9 +435,9 @@ public function testAuthenticateAlreadyLoggedInWithoutCsrfTokenForNonGetAndDeskt
430435
->expects($this->any())
431436
->method('getUser')
432437
->willReturn($user);
433-
$this->request
438+
$this->csrfValidator
434439
->expects($this->once())
435-
->method('passesCSRFCheck')
440+
->method('validate')
436441
->willReturn(false);
437442

438443
$this->auth->check($request, $response);
@@ -499,9 +504,9 @@ public function testAuthenticateAlreadyLoggedInWithCsrfTokenForGet(): void {
499504
->expects($this->any())
500505
->method('getUser')
501506
->willReturn($user);
502-
$this->request
507+
$this->csrfValidator
503508
->expects($this->once())
504-
->method('passesCSRFCheck')
509+
->method('validate')
505510
->willReturn(true);
506511

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

‎core/Controller/LoginController.php‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@
3838
use OCP\IUserManager;
3939
use OCP\Notification\IManager;
4040
use OCP\Security\Bruteforce\IThrottler;
41+
use OCP\Security\CSRF\ICsrfValidator;
4142
use OCP\Util;
4243

4344
class LoginController extends Controller {
@@ -60,6 +61,7 @@ public function __construct(
6061
private IManager $manager,
6162
private IL10N $l10n,
6263
private IAppManager $appManager,
64+
private ICsrfValidator $csrfValidator,
6365
) {
6466
parent::__construct($appName, $request);
6567
}
@@ -207,7 +209,7 @@ private function setPasswordResetInitialState(?string $username): void {
207209
$this->canResetPassword($passwordLink, $user)
208210
);
209211
}
210-
212+
211213
/**
212214
* Sets the initial state of whether or not a user is allowed to login with their email
213215
* initial state is passed in the array of 1 for email allowed and 0 for not allowed
@@ -284,7 +286,7 @@ public function tryLogin(Chain $loginChain,
284286
?string $redirect_url = null,
285287
string $timezone = '',
286288
string $timezone_offset = ''): RedirectResponse {
287-
if (!$this->request->passesCSRFCheck()) {
289+
if (!$this->csrfValidator->validate($this->request)) {
288290
if ($this->userSession->isLoggedIn()) {
289291
// If the user is already logged in and the CSRF check does not pass then
290292
// simply redirect the user to the correct page as required. This is the

‎lib/composer/composer/autoload_classmap.php‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -627,6 +627,7 @@
627627
'OCP\\Security\\Bruteforce\\IThrottler' => $baseDir . '/lib/public/Security/Bruteforce/IThrottler.php',
628628
'OCP\\Security\\Bruteforce\\MaxDelayReached' => $baseDir . '/lib/public/Security/Bruteforce/MaxDelayReached.php',
629629
'OCP\\Security\\CSP\\AddContentSecurityPolicyEvent' => $baseDir . '/lib/public/Security/CSP/AddContentSecurityPolicyEvent.php',
630+
'OCP\\Security\\CSRF\\ICsrfValidator' => $baseDir . '/lib/public/Security/CSRF/ICsrfValidator.php',
630631
'OCP\\Security\\Events\\GenerateSecurePasswordEvent' => $baseDir . '/lib/public/Security/Events/GenerateSecurePasswordEvent.php',
631632
'OCP\\Security\\Events\\ValidatePasswordPolicyEvent' => $baseDir . '/lib/public/Security/Events/ValidatePasswordPolicyEvent.php',
632633
'OCP\\Security\\FeaturePolicy\\AddFeaturePolicyEvent' => $baseDir . '/lib/public/Security/FeaturePolicy/AddFeaturePolicyEvent.php',
@@ -1775,6 +1776,7 @@
17751776
'OC\\Security\\CSRF\\CsrfToken' => $baseDir . '/lib/private/Security/CSRF/CsrfToken.php',
17761777
'OC\\Security\\CSRF\\CsrfTokenGenerator' => $baseDir . '/lib/private/Security/CSRF/CsrfTokenGenerator.php',
17771778
'OC\\Security\\CSRF\\CsrfTokenManager' => $baseDir . '/lib/private/Security/CSRF/CsrfTokenManager.php',
1779+
'OC\\Security\\CSRF\\CsrfValidator' => $baseDir . '/lib/private/Security/CSRF/CsrfValidator.php',
17781780
'OC\\Security\\CSRF\\TokenStorage\\SessionStorage' => $baseDir . '/lib/private/Security/CSRF/TokenStorage/SessionStorage.php',
17791781
'OC\\Security\\Certificate' => $baseDir . '/lib/private/Security/Certificate.php',
17801782
'OC\\Security\\CertificateManager' => $baseDir . '/lib/private/Security/CertificateManager.php',

‎lib/composer/composer/autoload_static.php‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -668,6 +668,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2
668668
'OCP\\Security\\Bruteforce\\IThrottler' => __DIR__ . '/../../..' . '/lib/public/Security/Bruteforce/IThrottler.php',
669669
'OCP\\Security\\Bruteforce\\MaxDelayReached' => __DIR__ . '/../../..' . '/lib/public/Security/Bruteforce/MaxDelayReached.php',
670670
'OCP\\Security\\CSP\\AddContentSecurityPolicyEvent' => __DIR__ . '/../../..' . '/lib/public/Security/CSP/AddContentSecurityPolicyEvent.php',
671+
'OCP\\Security\\CSRF\\ICsrfValidator' => __DIR__ . '/../../..' . '/lib/public/Security/CSRF/ICsrfValidator.php',
671672
'OCP\\Security\\Events\\GenerateSecurePasswordEvent' => __DIR__ . '/../../..' . '/lib/public/Security/Events/GenerateSecurePasswordEvent.php',
672673
'OCP\\Security\\Events\\ValidatePasswordPolicyEvent' => __DIR__ . '/../../..' . '/lib/public/Security/Events/ValidatePasswordPolicyEvent.php',
673674
'OCP\\Security\\FeaturePolicy\\AddFeaturePolicyEvent' => __DIR__ . '/../../..' . '/lib/public/Security/FeaturePolicy/AddFeaturePolicyEvent.php',
@@ -1816,6 +1817,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2
18161817
'OC\\Security\\CSRF\\CsrfToken' => __DIR__ . '/../../..' . '/lib/private/Security/CSRF/CsrfToken.php',
18171818
'OC\\Security\\CSRF\\CsrfTokenGenerator' => __DIR__ . '/../../..' . '/lib/private/Security/CSRF/CsrfTokenGenerator.php',
18181819
'OC\\Security\\CSRF\\CsrfTokenManager' => __DIR__ . '/../../..' . '/lib/private/Security/CSRF/CsrfTokenManager.php',
1820+
'OC\\Security\\CSRF\\CsrfValidator' => __DIR__ . '/../../..' . '/lib/private/Security/CSRF/CsrfValidator.php',
18191821
'OC\\Security\\CSRF\\TokenStorage\\SessionStorage' => __DIR__ . '/../../..' . '/lib/private/Security/CSRF/TokenStorage/SessionStorage.php',
18201822
'OC\\Security\\Certificate' => __DIR__ . '/../../..' . '/lib/private/Security/Certificate.php',
18211823
'OC\\Security\\CertificateManager' => __DIR__ . '/../../..' . '/lib/private/Security/CertificateManager.php',

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

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,7 @@
4646
use OCP\IURLGenerator;
4747
use OCP\IUserSession;
4848
use OCP\Security\Bruteforce\IThrottler;
49+
use OCP\Security\CSRF\ICsrfValidator;
4950
use Psr\Container\ContainerInterface;
5051
use Psr\Log\LoggerInterface;
5152

@@ -207,7 +208,8 @@ public function __construct(string $appName, array $urlParams = [], ?ServerConta
207208
$c->get(IRequest::class),
208209
$c->get(IControllerMethodReflector::class),
209210
$c->get(IUserSession::class),
210-
$c->get(IThrottler::class)
211+
$c->get(IThrottler::class),
212+
$c->get(ICsrfValidator::class),
211213
)
212214
);
213215
$dispatcher->registerMiddleware(
@@ -231,7 +233,8 @@ public function __construct(string $appName, array $urlParams = [], ?ServerConta
231233
$server->getAppManager(),
232234
$server->getL10N('lib'),
233235
$c->get(AuthorizedGroupMapper::class),
234-
$server->get(IUserSession::class)
236+
$server->get(IUserSession::class),
237+
$c->get(ICsrfValidator::class),
235238
);
236239
$dispatcher->registerMiddleware($securityMiddleware);
237240
$dispatcher->registerMiddleware(

0 commit comments

Comments
 (0)