]> source.dussan.org Git - nextcloud-server.git/commitdiff
Split up security middleware 16582/head
authorRoeland Jago Douma <roeland@famdouma.nl>
Sat, 27 Jul 2019 14:04:51 +0000 (16:04 +0200)
committerRoeland Jago Douma <roeland@famdouma.nl>
Sat, 27 Jul 2019 14:11:45 +0000 (16:11 +0200)
With upcoming work for the feature policy header. Splitting this in
smaller classes that just do 1 thing makes sense.

I rather have a few small classes that are tiny and do 1 thing right
(and we all understand what is going on) than have big ones.

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
lib/composer/composer/autoload_classmap.php
lib/composer/composer/autoload_static.php
lib/private/AppFramework/DependencyInjection/DIContainer.php
lib/private/AppFramework/Middleware/Security/CSPMiddleware.php [new file with mode: 0644]
lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php
tests/lib/AppFramework/Middleware/Security/CSPMiddlewareTest.php [new file with mode: 0644]
tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php

index d8d9227cb07cfaa4cdbb441d4b18938bab3dbbd9..019af95e2965696a69984279d4a097df83c564ac 100644 (file)
@@ -457,6 +457,7 @@ return array(
     'OC\\AppFramework\\Middleware\\PublicShare\\PublicShareMiddleware' => $baseDir . '/lib/private/AppFramework/Middleware/PublicShare/PublicShareMiddleware.php',
     'OC\\AppFramework\\Middleware\\Security\\BruteForceMiddleware' => $baseDir . '/lib/private/AppFramework/Middleware/Security/BruteForceMiddleware.php',
     'OC\\AppFramework\\Middleware\\Security\\CORSMiddleware' => $baseDir . '/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php',
+    'OC\\AppFramework\\Middleware\\Security\\CSPMiddleware' => $baseDir . '/lib/private/AppFramework/Middleware/Security/CSPMiddleware.php',
     'OC\\AppFramework\\Middleware\\Security\\Exceptions\\AppNotEnabledException' => $baseDir . '/lib/private/AppFramework/Middleware/Security/Exceptions/AppNotEnabledException.php',
     'OC\\AppFramework\\Middleware\\Security\\Exceptions\\CrossSiteRequestForgeryException' => $baseDir . '/lib/private/AppFramework/Middleware/Security/Exceptions/CrossSiteRequestForgeryException.php',
     'OC\\AppFramework\\Middleware\\Security\\Exceptions\\LaxSameSiteCookieFailedException' => $baseDir . '/lib/private/AppFramework/Middleware/Security/Exceptions/LaxSameSiteCookieFailedException.php',
index 043846e04df0fb9b2cb51835af334ec521285d67..87d0b0028000939d0211f2c7a9e5a203eb063d16 100644 (file)
@@ -491,6 +491,7 @@ class ComposerStaticInit53792487c5a8370acc0b06b1a864ff4c
         'OC\\AppFramework\\Middleware\\PublicShare\\PublicShareMiddleware' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/PublicShare/PublicShareMiddleware.php',
         'OC\\AppFramework\\Middleware\\Security\\BruteForceMiddleware' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/Security/BruteForceMiddleware.php',
         'OC\\AppFramework\\Middleware\\Security\\CORSMiddleware' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php',
+        'OC\\AppFramework\\Middleware\\Security\\CSPMiddleware' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/Security/CSPMiddleware.php',
         'OC\\AppFramework\\Middleware\\Security\\Exceptions\\AppNotEnabledException' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/Security/Exceptions/AppNotEnabledException.php',
         'OC\\AppFramework\\Middleware\\Security\\Exceptions\\CrossSiteRequestForgeryException' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/Security/Exceptions/CrossSiteRequestForgeryException.php',
         'OC\\AppFramework\\Middleware\\Security\\Exceptions\\LaxSameSiteCookieFailedException' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/Security/Exceptions/LaxSameSiteCookieFailedException.php',
index 6d337bb9327e7a2562cae83c087dc58bcc6ea5eb..f47af340b38f0086cebecae0757ae401a6f3e5f2 100644 (file)
@@ -220,13 +220,17 @@ class DIContainer extends SimpleContainer implements IAppContainer {
                                $server->getUserSession()->isLoggedIn(),
                                $server->getGroupManager()->isAdmin($this->getUserId()),
                                $server->getUserSession()->getUser() !== null && $server->query(ISubAdmin::class)->isSubAdmin($server->getUserSession()->getUser()),
-                               $server->getContentSecurityPolicyManager(),
-                               $server->getCsrfTokenManager(),
-                               $server->getContentSecurityPolicyNonceManager(),
                                $server->getAppManager(),
                                $server->getL10N('lib')
                        );
                        $dispatcher->registerMiddleware($securityMiddleware);
+                       $dispatcher->registerMiddleware(
+                               new OC\AppFramework\Middleware\Security\CSPMiddleware(
+                                       $server->query(OC\Security\CSP\ContentSecurityPolicyManager::class),
+                                       $server->query(OC\Security\CSP\ContentSecurityPolicyNonceManager::class),
+                                       $server->query(OC\Security\CSRF\CsrfTokenManager::class)
+                               )
+                       );
                        $dispatcher->registerMiddleware(
                                new OC\AppFramework\Middleware\Security\PasswordConfirmationMiddleware(
                                        $c->query(IControllerMethodReflector::class),
diff --git a/lib/private/AppFramework/Middleware/Security/CSPMiddleware.php b/lib/private/AppFramework/Middleware/Security/CSPMiddleware.php
new file mode 100644 (file)
index 0000000..0eaeda2
--- /dev/null
@@ -0,0 +1,80 @@
+<?php
+declare(strict_types=1);
+/**
+ * @copyright Copyright (c) 2019, Roeland Jago Douma <roeland@famdouma.nl>
+ *
+ * @author Roeland Jago Douma <roeland@famdouma.nl>
+ *
+ * @license GNU AGPL version 3 or any later version
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as
+ * published by the Free Software Foundation, either version 3 of the
+ * License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+namespace OC\AppFramework\Middleware\Security;
+
+use OC\Security\CSP\ContentSecurityPolicyManager;
+use OC\Security\CSP\ContentSecurityPolicyNonceManager;
+use OC\Security\CSRF\CsrfTokenManager;
+use OCP\AppFramework\Controller;
+use OCP\AppFramework\Http\ContentSecurityPolicy;
+use OCP\AppFramework\Http\EmptyContentSecurityPolicy;
+use OCP\AppFramework\Http\Response;
+use OCP\AppFramework\Middleware;
+
+class CSPMiddleware extends Middleware {
+
+       /** @var ContentSecurityPolicyManager */
+       private $contentSecurityPolicyManager;
+       /** @var ContentSecurityPolicyNonceManager */
+       private $cspNonceManager;
+       /** @var CsrfTokenManager */
+       private $csrfTokenManager;
+
+       public function __construct(ContentSecurityPolicyManager $policyManager,
+                                                               ContentSecurityPolicyNonceManager $cspNonceManager,
+                                                               CsrfTokenManager $csrfTokenManager) {
+               $this->contentSecurityPolicyManager = $policyManager;
+               $this->cspNonceManager = $cspNonceManager;
+               $this->csrfTokenManager = $csrfTokenManager;
+       }
+
+       /**
+        * Performs the default CSP modifications that may be injected by other
+        * applications
+        *
+        * @param Controller $controller
+        * @param string $methodName
+        * @param Response $response
+        * @return Response
+        */
+       public function afterController($controller, $methodName, Response $response): Response {
+               $policy = !is_null($response->getContentSecurityPolicy()) ? $response->getContentSecurityPolicy() : new ContentSecurityPolicy();
+
+               if (get_class($policy) === EmptyContentSecurityPolicy::class) {
+                       return $response;
+               }
+
+               $defaultPolicy = $this->contentSecurityPolicyManager->getDefaultPolicy();
+               $defaultPolicy = $this->contentSecurityPolicyManager->mergePolicies($defaultPolicy, $policy);
+
+               if($this->cspNonceManager->browserSupportsCspV3()) {
+                       $defaultPolicy->useJsNonce($this->csrfTokenManager->getToken()->getEncryptedValue());
+               }
+
+               $response->setContentSecurityPolicy($defaultPolicy);
+
+               return $response;
+       }
+}
index fef3f226e15d6a06919211f6fd3d1e9c93d2f1f9..4f380f07d917022376c434d1827f6addb540915f 100644 (file)
@@ -84,12 +84,6 @@ class SecurityMiddleware extends Middleware {
        private $isAdminUser;
        /** @var bool */
        private $isSubAdmin;
-       /** @var ContentSecurityPolicyManager */
-       private $contentSecurityPolicyManager;
-       /** @var CsrfTokenManager */
-       private $csrfTokenManager;
-       /** @var ContentSecurityPolicyNonceManager */
-       private $cspNonceManager;
        /** @var IAppManager */
        private $appManager;
        /** @var IL10N */
@@ -104,9 +98,6 @@ class SecurityMiddleware extends Middleware {
                                                                bool $isLoggedIn,
                                                                bool $isAdminUser,
                                                                bool $isSubAdmin,
-                                                               ContentSecurityPolicyManager $contentSecurityPolicyManager,
-                                                               CsrfTokenManager $csrfTokenManager,
-                                                               ContentSecurityPolicyNonceManager $cspNonceManager,
                                                                IAppManager $appManager,
                                                                IL10N $l10n
        ) {
@@ -119,9 +110,6 @@ class SecurityMiddleware extends Middleware {
                $this->isLoggedIn = $isLoggedIn;
                $this->isAdminUser = $isAdminUser;
                $this->isSubAdmin = $isSubAdmin;
-               $this->contentSecurityPolicyManager = $contentSecurityPolicyManager;
-               $this->csrfTokenManager = $csrfTokenManager;
-               $this->cspNonceManager = $cspNonceManager;
                $this->appManager = $appManager;
                $this->l10n = $l10n;
        }
@@ -203,34 +191,6 @@ class SecurityMiddleware extends Middleware {
                }
        }
 
-       /**
-        * Performs the default CSP modifications that may be injected by other
-        * applications
-        *
-        * @param Controller $controller
-        * @param string $methodName
-        * @param Response $response
-        * @return Response
-        */
-       public function afterController($controller, $methodName, Response $response): Response {
-               $policy = !is_null($response->getContentSecurityPolicy()) ? $response->getContentSecurityPolicy() : new ContentSecurityPolicy();
-
-               if (get_class($policy) === EmptyContentSecurityPolicy::class) {
-                       return $response;
-               }
-
-               $defaultPolicy = $this->contentSecurityPolicyManager->getDefaultPolicy();
-               $defaultPolicy = $this->contentSecurityPolicyManager->mergePolicies($defaultPolicy, $policy);
-
-               if($this->cspNonceManager->browserSupportsCspV3()) {
-                       $defaultPolicy->useJsNonce($this->csrfTokenManager->getToken()->getEncryptedValue());
-               }
-
-               $response->setContentSecurityPolicy($defaultPolicy);
-
-               return $response;
-       }
-
        /**
         * If an SecurityException is being caught, ajax requests return a JSON error
         * response and non ajax requests redirect to the index
diff --git a/tests/lib/AppFramework/Middleware/Security/CSPMiddlewareTest.php b/tests/lib/AppFramework/Middleware/Security/CSPMiddlewareTest.php
new file mode 100644 (file)
index 0000000..9f2b4e3
--- /dev/null
@@ -0,0 +1,149 @@
+<?php
+declare(strict_types=1);
+/**
+ * @copyright Copyright (c) 2019, Roeland Jago Douma <roeland@famdouma.nl>
+ *
+ * @author Roeland Jago Douma <roeland@famdouma.nl>
+ *
+ * @license GNU AGPL version 3 or any later version
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as
+ * published by the Free Software Foundation, either version 3 of the
+ * License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+namespace Test\AppFramework\Middleware\Security;
+
+use OC\AppFramework\Middleware\Security\CSPMiddleware;
+use OC\Security\CSP\ContentSecurityPolicy;
+use OC\Security\CSP\ContentSecurityPolicyManager;
+use OC\Security\CSP\ContentSecurityPolicyNonceManager;
+use OC\Security\CSRF\CsrfToken;
+use OC\Security\CSRF\CsrfTokenManager;
+use OCP\AppFramework\Controller;
+use OCP\AppFramework\Http\EmptyContentSecurityPolicy;
+use OCP\AppFramework\Http\Response;
+use PHPUnit\Framework\MockObject\MockObject;
+
+class CSPMiddlewareTest extends \Test\TestCase {
+
+       /** @var CSPMiddleware|MockObject */
+       private $middleware;
+       /** @var Controller|MockObject */
+       private $controller;
+       /** @var ContentSecurityPolicyManager|MockObject */
+       private $contentSecurityPolicyManager;
+       /** @var CsrfTokenManager|MockObject */
+       private $csrfTokenManager;
+       /** @var ContentSecurityPolicyNonceManager|MockObject */
+       private $cspNonceManager;
+
+       protected function setUp() {
+               parent::setUp();
+
+               $this->controller = $this->createMock(Controller::class);
+               $this->contentSecurityPolicyManager = $this->createMock(ContentSecurityPolicyManager::class);
+               $this->csrfTokenManager = $this->createMock(CsrfTokenManager::class);
+               $this->cspNonceManager = $this->createMock(ContentSecurityPolicyNonceManager::class);
+               $this->middleware = new CSPMiddleware(
+                       $this->contentSecurityPolicyManager,
+                       $this->cspNonceManager,
+                       $this->csrfTokenManager
+               );
+       }
+
+       public function testAfterController() {
+               $this->cspNonceManager
+                       ->expects($this->once())
+                       ->method('browserSupportsCspV3')
+                       ->willReturn(false);
+               $response = $this->createMock(Response::class);
+               $defaultPolicy = new ContentSecurityPolicy();
+               $defaultPolicy->addAllowedImageDomain('defaultpolicy');
+               $currentPolicy = new ContentSecurityPolicy();
+               $currentPolicy->addAllowedConnectDomain('currentPolicy');
+               $mergedPolicy = new ContentSecurityPolicy();
+               $mergedPolicy->addAllowedMediaDomain('mergedPolicy');
+               $response
+                       ->expects($this->exactly(2))
+                       ->method('getContentSecurityPolicy')
+                       ->willReturn($currentPolicy);
+               $this->contentSecurityPolicyManager
+                       ->expects($this->once())
+                       ->method('getDefaultPolicy')
+                       ->willReturn($defaultPolicy);
+               $this->contentSecurityPolicyManager
+                       ->expects($this->once())
+                       ->method('mergePolicies')
+                       ->with($defaultPolicy, $currentPolicy)
+                       ->willReturn($mergedPolicy);
+               $response->expects($this->once())
+                       ->method('setContentSecurityPolicy')
+                       ->with($mergedPolicy);
+
+               $this->middleware->afterController($this->controller, 'test', $response);
+       }
+
+       public function testAfterControllerEmptyCSP() {
+               $response = $this->createMock(Response::class);
+               $emptyPolicy = new EmptyContentSecurityPolicy();
+               $response->expects($this->any())
+                       ->method('getContentSecurityPolicy')
+                       ->willReturn($emptyPolicy);
+               $response->expects($this->never())
+                       ->method('setContentSecurityPolicy');
+
+               $this->middleware->afterController($this->controller, 'test', $response);
+       }
+
+       public function testAfterControllerWithContentSecurityPolicy3Support() {
+               $this->cspNonceManager
+                       ->expects($this->once())
+                       ->method('browserSupportsCspV3')
+                       ->willReturn(true);
+               $token = $this->createMock(CsrfToken::class);
+               $token
+                       ->expects($this->once())
+                       ->method('getEncryptedValue')
+                       ->willReturn('MyEncryptedToken');
+               $this->csrfTokenManager
+                       ->expects($this->once())
+                       ->method('getToken')
+                       ->willReturn($token);
+               $response = $this->createMock(Response::class);
+               $defaultPolicy = new ContentSecurityPolicy();
+               $defaultPolicy->addAllowedImageDomain('defaultpolicy');
+               $currentPolicy = new ContentSecurityPolicy();
+               $currentPolicy->addAllowedConnectDomain('currentPolicy');
+               $mergedPolicy = new ContentSecurityPolicy();
+               $mergedPolicy->addAllowedMediaDomain('mergedPolicy');
+               $response
+                       ->expects($this->exactly(2))
+                       ->method('getContentSecurityPolicy')
+                       ->willReturn($currentPolicy);
+               $this->contentSecurityPolicyManager
+                       ->expects($this->once())
+                       ->method('getDefaultPolicy')
+                       ->willReturn($defaultPolicy);
+               $this->contentSecurityPolicyManager
+                       ->expects($this->once())
+                       ->method('mergePolicies')
+                       ->with($defaultPolicy, $currentPolicy)
+                       ->willReturn($mergedPolicy);
+               $response->expects($this->once())
+                       ->method('setContentSecurityPolicy')
+                       ->with($mergedPolicy);
+
+               $this->assertEquals($response, $this->middleware->afterController($this->controller, 'test', $response));
+       }
+}
index ab243616be03b09f725a0cc8e3f46d4f5e384466..6a1adf03b2f10f4050f25843634faec2c8b71214 100644 (file)
@@ -72,12 +72,6 @@ class SecurityMiddlewareTest extends \Test\TestCase {
        private $navigationManager;
        /** @var IURLGenerator|\PHPUnit_Framework_MockObject_MockObject */
        private $urlGenerator;
-       /** @var ContentSecurityPolicyManager|\PHPUnit_Framework_MockObject_MockObject */
-       private $contentSecurityPolicyManager;
-       /** @var CsrfTokenManager|\PHPUnit_Framework_MockObject_MockObject */
-       private $csrfTokenManager;
-       /** @var ContentSecurityPolicyNonceManager|\PHPUnit_Framework_MockObject_MockObject */
-       private $cspNonceManager;
        /** @var IAppManager|\PHPUnit_Framework_MockObject_MockObject */
        private $appManager;
        /** @var IL10N|\PHPUnit_Framework_MockObject_MockObject */
@@ -92,9 +86,6 @@ class SecurityMiddlewareTest extends \Test\TestCase {
                $this->navigationManager = $this->createMock(INavigationManager::class);
                $this->urlGenerator = $this->createMock(IURLGenerator::class);
                $this->request = $this->createMock(IRequest::class);
-               $this->contentSecurityPolicyManager = $this->createMock(ContentSecurityPolicyManager::class);
-               $this->csrfTokenManager = $this->createMock(CsrfTokenManager::class);
-               $this->cspNonceManager = $this->createMock(ContentSecurityPolicyNonceManager::class);
                $this->l10n = $this->createMock(IL10N::class);
                $this->middleware = $this->getMiddleware(true, true, false);
                $this->secException = new SecurityException('hey', false);
@@ -118,9 +109,6 @@ class SecurityMiddlewareTest extends \Test\TestCase {
                        $isLoggedIn,
                        $isAdminUser,
                        $isSubAdmin,
-                       $this->contentSecurityPolicyManager,
-                       $this->csrfTokenManager,
-                       $this->cspNonceManager,
                        $this->appManager,
                        $this->l10n
                );
@@ -611,91 +599,6 @@ class SecurityMiddlewareTest extends \Test\TestCase {
                $this->assertTrue($response instanceof JSONResponse);
        }
 
-       public function testAfterController() {
-               $this->cspNonceManager
-                       ->expects($this->once())
-                       ->method('browserSupportsCspV3')
-                       ->willReturn(false);
-               $response = $this->createMock(Response::class);
-               $defaultPolicy = new ContentSecurityPolicy();
-               $defaultPolicy->addAllowedImageDomain('defaultpolicy');
-               $currentPolicy = new ContentSecurityPolicy();
-               $currentPolicy->addAllowedConnectDomain('currentPolicy');
-               $mergedPolicy = new ContentSecurityPolicy();
-               $mergedPolicy->addAllowedMediaDomain('mergedPolicy');
-               $response
-                       ->expects($this->exactly(2))
-                       ->method('getContentSecurityPolicy')
-                       ->willReturn($currentPolicy);
-               $this->contentSecurityPolicyManager
-                       ->expects($this->once())
-                       ->method('getDefaultPolicy')
-                       ->willReturn($defaultPolicy);
-               $this->contentSecurityPolicyManager
-                       ->expects($this->once())
-                       ->method('mergePolicies')
-                       ->with($defaultPolicy, $currentPolicy)
-                       ->willReturn($mergedPolicy);
-               $response->expects($this->once())
-                       ->method('setContentSecurityPolicy')
-                       ->with($mergedPolicy);
-
-               $this->middleware->afterController($this->controller, 'test', $response);
-       }
-
-       public function testAfterControllerEmptyCSP() {
-               $response = $this->createMock(Response::class);
-               $emptyPolicy = new EmptyContentSecurityPolicy();
-               $response->expects($this->any())
-                       ->method('getContentSecurityPolicy')
-                       ->willReturn($emptyPolicy);
-               $response->expects($this->never())
-                       ->method('setContentSecurityPolicy');
-
-               $this->middleware->afterController($this->controller, 'test', $response);
-       }
-
-       public function testAfterControllerWithContentSecurityPolicy3Support() {
-               $this->cspNonceManager
-                       ->expects($this->once())
-                       ->method('browserSupportsCspV3')
-                       ->willReturn(true);
-               $token = $this->createMock(CsrfToken::class);
-               $token
-                       ->expects($this->once())
-                       ->method('getEncryptedValue')
-                       ->willReturn('MyEncryptedToken');
-               $this->csrfTokenManager
-                       ->expects($this->once())
-                       ->method('getToken')
-                       ->willReturn($token);
-               $response = $this->createMock(Response::class);
-               $defaultPolicy = new ContentSecurityPolicy();
-               $defaultPolicy->addAllowedImageDomain('defaultpolicy');
-               $currentPolicy = new ContentSecurityPolicy();
-               $currentPolicy->addAllowedConnectDomain('currentPolicy');
-               $mergedPolicy = new ContentSecurityPolicy();
-               $mergedPolicy->addAllowedMediaDomain('mergedPolicy');
-               $response
-                       ->expects($this->exactly(2))
-                       ->method('getContentSecurityPolicy')
-                       ->willReturn($currentPolicy);
-               $this->contentSecurityPolicyManager
-                       ->expects($this->once())
-                       ->method('getDefaultPolicy')
-                       ->willReturn($defaultPolicy);
-               $this->contentSecurityPolicyManager
-                       ->expects($this->once())
-                       ->method('mergePolicies')
-                       ->with($defaultPolicy, $currentPolicy)
-                       ->willReturn($mergedPolicy);
-               $response->expects($this->once())
-                       ->method('setContentSecurityPolicy')
-                       ->with($mergedPolicy);
-
-               $this->assertEquals($response, $this->middleware->afterController($this->controller, 'test', $response));
-       }
-
        public function dataRestrictedApp() {
                return [
                        [false, false, false,],