Skip to content

Commit ca94de7

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

File tree

22 files changed

+380
-78
lines changed

22 files changed

+380
-78
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
@@ -61,6 +61,7 @@
6161
\OC::$server->getRequest(),
6262
\OC::$server->getTwoFactorAuthManager(),
6363
\OC::$server->getBruteForceThrottler(),
64+
\OC::$server->get(\OC\Security\CSRF\CsrfValidator::class),
6465
'principals/'
6566
);
6667
$authPlugin = new \Sabre\DAV\Auth\Plugin($authBackend);

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

Lines changed: 7 additions & 2 deletions
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\Session;
4142
use OCA\DAV\Connector\Sabre\Exception\PasswordLoginForbidden;
4243
use OCA\DAV\Connector\Sabre\Exception\TooManyRequests;
@@ -60,17 +61,21 @@ class Auth extends AbstractBasic {
6061
private Manager $twoFactorManager;
6162
private Throttler $throttler;
6263

64+
private CsrfValidator $csrfValidator;
65+
6366
public function __construct(ISession $session,
6467
Session $userSession,
6568
IRequest $request,
6669
Manager $twoFactorManager,
6770
Throttler $throttler,
71+
CsrfValidator $csrfValidator,
6872
string $principalPrefix = 'principals/users/') {
6973
$this->session = $session;
7074
$this->userSession = $userSession;
7175
$this->twoFactorManager = $twoFactorManager;
7276
$this->request = $request;
7377
$this->throttler = $throttler;
78+
$this->csrfValidator = $csrfValidator;
7479
$this->principalPrefix = $principalPrefix;
7580

7681
// setup realm
@@ -190,7 +195,7 @@ private function requiresCSRFCheck(): bool {
190195
private function auth(RequestInterface $request, ResponseInterface $response): array {
191196
$forcedLogout = false;
192197

193-
if (!$this->request->passesCSRFCheck() &&
198+
if (!$this->csrfValidator->validate($this->request) &&
194199
$this->requiresCSRFCheck()) {
195200
// In case of a fail with POST we need to recheck the credentials
196201
if ($this->request->getMethod() === 'POST') {
@@ -223,7 +228,7 @@ private function auth(RequestInterface $request, ResponseInterface $response): a
223228

224229
if (!$this->userSession->isLoggedIn() && in_array('XMLHttpRequest', explode(',', $request->getHeader('X-Requested-With') ?? ''))) {
225230
// do not re-authenticate over ajax, use dummy auth name to prevent browser popup
226-
$response->addHeader('WWW-Authenticate','DummyBasic realm="' . $this->realm . '"');
231+
$response->addHeader('WWW-Authenticate', 'DummyBasic realm="' . $this->realm . '"');
227232
$response->setStatus(401);
228233
throw new \Sabre\DAV\Exception\NotAuthenticated('Cannot authenticate over ajax calls');
229234
}

‎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 = [
@@ -317,9 +321,9 @@ public function testAuthenticateAlreadyLoggedInWithoutCsrfTokenAndCorrectlyDavAu
317321
->expects($this->any())
318322
->method('getUser')
319323
->willReturn($user);
320-
$this->request
324+
$this->csrfValidator
321325
->expects($this->once())
322-
->method('passesCSRFCheck')
326+
->method('validate')
323327
->willReturn(false);
324328
$this->auth->check($request, $response);
325329
}
@@ -362,9 +366,9 @@ public function testAuthenticateAlreadyLoggedInWithoutTwoFactorChallengePassed()
362366
->expects($this->any())
363367
->method('getUser')
364368
->willReturn($user);
365-
$this->request
369+
$this->csrfValidator
366370
->expects($this->once())
367-
->method('passesCSRFCheck')
371+
->method('validate')
368372
->willReturn(true);
369373
$this->twoFactorManager->expects($this->once())
370374
->method('needsSecondFactor')
@@ -411,9 +415,9 @@ public function testAuthenticateAlreadyLoggedInWithoutCsrfTokenAndIncorrectlyDav
411415
->expects($this->any())
412416
->method('getUser')
413417
->willReturn($user);
414-
$this->request
418+
$this->csrfValidator
415419
->expects($this->once())
416-
->method('passesCSRFCheck')
420+
->method('validate')
417421
->willReturn(false);
418422
$this->auth->check($request, $response);
419423
}
@@ -452,9 +456,9 @@ public function testAuthenticateAlreadyLoggedInWithoutCsrfTokenForNonGetAndDeskt
452456
->expects($this->any())
453457
->method('getUser')
454458
->willReturn($user);
455-
$this->request
459+
$this->csrfValidator
456460
->expects($this->once())
457-
->method('passesCSRFCheck')
461+
->method('validate')
458462
->willReturn(false);
459463

460464
$this->auth->check($request, $response);
@@ -521,9 +525,9 @@ public function testAuthenticateAlreadyLoggedInWithCsrfTokenForGet(): void {
521525
->expects($this->any())
522526
->method('getUser')
523527
->willReturn($user);
524-
$this->request
528+
$this->csrfValidator
525529
->expects($this->once())
526-
->method('passesCSRFCheck')
530+
->method('validate')
527531
->willReturn(true);
528532

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

‎core/Controller/LoginController.php‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@
3939
use OC\Authentication\Login\LoginData;
4040
use OC\Authentication\WebAuthn\Manager as WebAuthnManager;
4141
use OC\Security\Bruteforce\Throttler;
42+
use OC\Security\CSRF\CsrfValidator;
4243
use OC\User\Session;
4344
use OC_App;
4445
use OCP\AppFramework\Controller;
@@ -79,6 +80,7 @@ public function __construct(
7980
private WebAuthnManager $webAuthnManager,
8081
private IManager $manager,
8182
private IL10N $l10n,
83+
private CsrfValidator $csrfValidator,
8284
) {
8385
parent::__construct($appName, $request);
8486
}
@@ -276,7 +278,7 @@ public function tryLogin(Chain $loginChain,
276278
string $redirect_url = null,
277279
string $timezone = '',
278280
string $timezone_offset = ''): RedirectResponse {
279-
if (!$this->request->passesCSRFCheck()) {
281+
if (!$this->csrfValidator->validate($this->request)) {
280282
if ($this->userSession->isLoggedIn()) {
281283
// If the user is already logged in and the CSRF check does not pass then
282284
// 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
@@ -1590,6 +1590,7 @@
15901590
'OC\\Security\\CSRF\\CsrfToken' => $baseDir . '/lib/private/Security/CSRF/CsrfToken.php',
15911591
'OC\\Security\\CSRF\\CsrfTokenGenerator' => $baseDir . '/lib/private/Security/CSRF/CsrfTokenGenerator.php',
15921592
'OC\\Security\\CSRF\\CsrfTokenManager' => $baseDir . '/lib/private/Security/CSRF/CsrfTokenManager.php',
1593+
'OC\\Security\\CSRF\\CsrfValidator' => $baseDir . '/lib/private/Security/CSRF/CsrfValidator.php',
15931594
'OC\\Security\\CSRF\\TokenStorage\\SessionStorage' => $baseDir . '/lib/private/Security/CSRF/TokenStorage/SessionStorage.php',
15941595
'OC\\Security\\Certificate' => $baseDir . '/lib/private/Security/Certificate.php',
15951596
'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
@@ -1623,6 +1623,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2
16231623
'OC\\Security\\CSRF\\CsrfToken' => __DIR__ . '/../../..' . '/lib/private/Security/CSRF/CsrfToken.php',
16241624
'OC\\Security\\CSRF\\CsrfTokenGenerator' => __DIR__ . '/../../..' . '/lib/private/Security/CSRF/CsrfTokenGenerator.php',
16251625
'OC\\Security\\CSRF\\CsrfTokenManager' => __DIR__ . '/../../..' . '/lib/private/Security/CSRF/CsrfTokenManager.php',
1626+
'OC\\Security\\CSRF\\CsrfValidator' => __DIR__ . '/../../..' . '/lib/private/Security/CSRF/CsrfValidator.php',
16261627
'OC\\Security\\CSRF\\TokenStorage\\SessionStorage' => __DIR__ . '/../../..' . '/lib/private/Security/CSRF/TokenStorage/SessionStorage.php',
16271628
'OC\\Security\\Certificate' => __DIR__ . '/../../..' . '/lib/private/Security/Certificate.php',
16281629
'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)