Merge pull request #1417 from nextcloud/strict_CSP_for_OCS

Strict CSP for OCS API

build/integration/features/bootstrap/Sharing.php

@@ -514,5 +514,28 @@ trait Sharing {
 			throw new \Exception('Expected the same link share to be returned');
 		}
 	}
+
+	/**
+	 * @Then The following headers should be set
+	 * @param \Behat\Gherkin\Node\TableNode $table
+	 * @throws \Exception
+	 */
+	public function theFollowingHeadersShouldBeSet(\Behat\Gherkin\Node\TableNode $table) {
+		foreach($table->getTable() as $header) {
+			$headerName = $header[0];
+			$expectedHeaderValue = $header[1];
+			$returnedHeader = $this->response->getHeader($headerName);
+			if($returnedHeader !== $expectedHeaderValue) {
+				throw new \Exception(
+					sprintf(
+						"Expected value '%s' for header '%s', got '%s'",
+						$expectedHeaderValue,
+						$headerName,
+						$returnedHeader
+					)
+				);
+			}
+		}
+	}
 }
 

build/integration/features/sharing-v1.feature

@@ -13,6 +13,8 @@ Feature: sharing
       | shareType | 0 |
     Then the OCS status code should be "100"
     And the HTTP status code should be "200"
+    And The following headers should be set
+      | Content-Security-Policy | default-src 'none' |
 
   Scenario: Creating a share with a group
     Given user "user0" exists

lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php

@@ -37,6 +37,7 @@ use OC\AppFramework\Middleware\Security\Exceptions\StrictCookieMissingException;
 use OC\AppFramework\Utility\ControllerMethodReflector;
 use OC\Security\CSP\ContentSecurityPolicyManager;
 use OCP\AppFramework\Http\ContentSecurityPolicy;
+use OCP\AppFramework\Http\EmptyContentSecurityPolicy;
 use OCP\AppFramework\Http\RedirectResponse;
 use OCP\AppFramework\Http\TemplateResponse;
 use OCP\AppFramework\Middleware;
@@ -182,6 +183,10 @@ class SecurityMiddleware extends Middleware {
 	public function afterController($controller, $methodName, 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);
 

lib/private/AppFramework/OCS/BaseResponse.php

@@ -23,6 +23,7 @@
 namespace OC\AppFramework\OCS;
 
 use OCP\AppFramework\Http\DataResponse;
+use OCP\AppFramework\Http\EmptyContentSecurityPolicy;
 use OCP\AppFramework\Http\Response;
 
 abstract class BaseResponse extends Response   {
@@ -67,7 +68,7 @@ abstract class BaseResponse extends Response   {
 		$this->setETag($dataResponse->getETag());
 		$this->setLastModified($dataResponse->getLastModified());
 		$this->setCookies($dataResponse->getCookies());
-		$this->setContentSecurityPolicy($dataResponse->getContentSecurityPolicy());
+		$this->setContentSecurityPolicy(new EmptyContentSecurityPolicy());
 
 		if ($format === 'json') {
 			$this->addHeader(

lib/public/AppFramework/Http/Response.php

@@ -248,18 +248,18 @@ class Response {
 
 	/**
 	 * Set a Content-Security-Policy
-	 * @param ContentSecurityPolicy $csp Policy to set for the response object
+	 * @param EmptyContentSecurityPolicy $csp Policy to set for the response object
 	 * @return $this
 	 * @since 8.1.0
 	 */
-	public function setContentSecurityPolicy(ContentSecurityPolicy $csp) {
+	public function setContentSecurityPolicy(EmptyContentSecurityPolicy $csp) {
 		$this->contentSecurityPolicy = $csp;
 		return $this;
 	}
 
 	/**
 	 * Get the currently used Content-Security-Policy
-	 * @return ContentSecurityPolicy|null Used Content-Security-Policy or null if
+	 * @return EmptyContentSecurityPolicy|null Used Content-Security-Policy or null if
 	 *                                    none specified.
 	 * @since 8.1.0
 	 */

tests/lib/AppFramework/Controller/OCSControllerTest.php

@@ -26,6 +26,7 @@ namespace Test\AppFramework\Controller;
 
 use OC\AppFramework\Http\Request;
 use OCP\AppFramework\Http\DataResponse;
+use OCP\AppFramework\Http\EmptyContentSecurityPolicy;
 use OCP\AppFramework\OCSController;
 use OCP\IConfig;
 use OCP\Security\ISecureRandom;
@@ -92,8 +93,9 @@ class OCSControllerTest extends \Test\TestCase {
 
 		$params = new DataResponse(['test' => 'hi']);
 
-		$out = $controller->buildResponse($params, 'xml')->render();
+		$response = $controller->buildResponse($params, 'xml');
-		$this->assertEquals($expected, $out);
+		$this->assertSame(EmptyContentSecurityPolicy::class, get_class($response->getContentSecurityPolicy()));
+		$this->assertEquals($expected, $response->render());
 	}
 
 	public function testJSON() {
@@ -111,8 +113,10 @@ class OCSControllerTest extends \Test\TestCase {
 		            '"totalitems":"","itemsperpage":""},"data":{"test":"hi"}}}';
 		$params = new DataResponse(['test' => 'hi']);
 
-		$out = $controller->buildResponse($params, 'json')->render();
+		$response = $controller->buildResponse($params, 'json');
-		$this->assertEquals($expected, $out);
+		$this->assertSame(EmptyContentSecurityPolicy::class, get_class($response->getContentSecurityPolicy()));
+		$this->assertEquals($expected, $response->render());
+		$this->assertEquals($expected, $response->render());
 	}
 
 	public function testXMLV2() {
@@ -141,8 +145,9 @@ class OCSControllerTest extends \Test\TestCase {
 
 		$params = new DataResponse(['test' => 'hi']);
 
-		$out = $controller->buildResponse($params, 'xml')->render();
+		$response = $controller->buildResponse($params, 'xml');
-		$this->assertEquals($expected, $out);
+		$this->assertSame(EmptyContentSecurityPolicy::class, get_class($response->getContentSecurityPolicy()));
+		$this->assertEquals($expected, $response->render());
 	}
 
 	public function testJSONV2() {
@@ -159,7 +164,8 @@ class OCSControllerTest extends \Test\TestCase {
 		$expected = '{"ocs":{"meta":{"status":"ok","statuscode":200,"message":"OK"},"data":{"test":"hi"}}}';
 		$params = new DataResponse(['test' => 'hi']);
 
-		$out = $controller->buildResponse($params, 'json')->render();
+		$response = $controller->buildResponse($params, 'json');
-		$this->assertEquals($expected, $out);
+		$this->assertSame(EmptyContentSecurityPolicy::class, get_class($response->getContentSecurityPolicy()));
+		$this->assertEquals($expected, $response->render());
 	}
 }

tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php

@@ -37,13 +37,17 @@ use OC\AppFramework\Utility\ControllerMethodReflector;
 use OC\Security\CSP\ContentSecurityPolicy;
 use OC\Security\CSP\ContentSecurityPolicyManager;
 use OCP\AppFramework\Controller;
+use OCP\AppFramework\Http\EmptyContentSecurityPolicy;
 use OCP\AppFramework\Http\RedirectResponse;
 use OCP\AppFramework\Http\JSONResponse;
+use OCP\AppFramework\Http\Response;
 use OCP\AppFramework\Http\TemplateResponse;
+use OCP\IConfig;
 use OCP\ILogger;
 use OCP\INavigationManager;
 use OCP\IRequest;
 use OCP\IURLGenerator;
+use OCP\Security\ISecureRandom;
 
 
 class SecurityMiddlewareTest extends \Test\TestCase {
@@ -72,30 +76,13 @@ class SecurityMiddlewareTest extends \Test\TestCase {
 	protected function setUp() {
 		parent::setUp();
 
-		$this->controller = $this->getMockBuilder('OCP\AppFramework\Controller')
+		$this->controller = $this->createMock(Controller::class);
-			->disableOriginalConstructor()
-			->getMock();
 		$this->reader = new ControllerMethodReflector();
-		$this->logger = $this->getMockBuilder(
+		$this->logger = $this->createMock(ILogger::class);
-			'OCP\ILogger')
+		$this->navigationManager = $this->createMock(INavigationManager::class);
-			->disableOriginalConstructor()
+		$this->urlGenerator = $this->createMock(IURLGenerator::class);
-			->getMock();
+		$this->request = $this->createMock(IRequest::class);
-		$this->navigationManager = $this->getMockBuilder(
+		$this->contentSecurityPolicyManager = $this->createMock(ContentSecurityPolicyManager::class);
-			'OCP\INavigationManager')
-			->disableOriginalConstructor()
-			->getMock();
-		$this->urlGenerator = $this->getMockBuilder(
-			'OCP\IURLGenerator')
-			->disableOriginalConstructor()
-			->getMock();
-		$this->request = $this->getMockBuilder(
-			'OCP\IRequest')
-			->disableOriginalConstructor()
-			->getMock();
-		$this->contentSecurityPolicyManager = $this->getMockBuilder(
-			'OC\Security\CSP\ContentSecurityPolicyManager')
-			->disableOriginalConstructor()
-			->getMock();
 		$this->middleware = $this->getMiddleware(true, true);
 		$this->secException = new SecurityException('hey', false);
 		$this->secAjaxException = new SecurityException('hey', true);
@@ -459,8 +446,8 @@ class SecurityMiddlewareTest extends \Test\TestCase {
 						'REQUEST_URI' => 'owncloud/index.php/apps/specialapp'
 					]
 			],
-			$this->getMockBuilder('\OCP\Security\ISecureRandom')->getMock(),
+			$this->createMock(ISecureRandom::class),
-			$this->getMockBuilder('\OCP\IConfig')->getMock()
+			$this->createMock(IConfig::class)
 		);
 		$this->middleware = $this->getMiddleware(false, false);
 		$this->urlGenerator
@@ -494,8 +481,8 @@ class SecurityMiddlewareTest extends \Test\TestCase {
 					'REQUEST_URI' => 'owncloud/index.php/apps/specialapp',
 				],
 			],
-			$this->getMockBuilder('\OCP\Security\ISecureRandom')->getMock(),
+			$this->createMock(ISecureRandom::class),
-			$this->getMockBuilder('\OCP\IConfig')->getMock()
+			$this->createMock(IConfig::class)
 		);
 
 		$this->middleware = $this->getMiddleware(false, false);
@@ -540,8 +527,8 @@ class SecurityMiddlewareTest extends \Test\TestCase {
 						'REQUEST_URI' => 'owncloud/index.php/apps/specialapp'
 					]
 			],
-			$this->getMockBuilder('\OCP\Security\ISecureRandom')->getMock(),
+			$this->createMock(ISecureRandom::class),
-			$this->getMockBuilder('\OCP\IConfig')->getMock()
+			$this->createMock(IConfig::class)
 		);
 		$this->middleware = $this->getMiddleware(false, false);
 		$this->logger
@@ -566,7 +553,7 @@ class SecurityMiddlewareTest extends \Test\TestCase {
 	}
 
 	public function testAfterController() {
-		$response = $this->getMockBuilder('\OCP\AppFramework\Http\Response')->disableOriginalConstructor()->getMock();
+		$response = $this->createMock(Response::class);
 		$defaultPolicy = new ContentSecurityPolicy();
 		$defaultPolicy->addAllowedImageDomain('defaultpolicy');
 		$currentPolicy = new ContentSecurityPolicy();
@@ -592,4 +579,16 @@ class SecurityMiddlewareTest extends \Test\TestCase {
 
 		$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);
+	}
 }