Merge pull request #656 from nextcloud/ocs_controller_csrf

Dark hackery to not always disable CSRF for OCS controllers

2 files changed, 65 insertions, 2 deletions

diff --git a/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php b/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php
index daac36606f2..08af42b5216 100644
--- a/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php
+++ b/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php
@@ -42,6 +42,7 @@ use OCP\AppFramework\Http\TemplateResponse;
 use OCP\AppFramework\Middleware;
 use OCP\AppFramework\Http\Response;
 use OCP\AppFramework\Http\JSONResponse;
+use OCP\AppFramework\OCSController;
 use OCP\INavigationManager;
 use OCP\IURLGenerator;
 use OCP\IRequest;
@@ -112,7 +113,7 @@ class SecurityMiddleware extends Middleware {
 	 * This runs all the security checks before a method call. The
 	 * security checks are determined by inspecting the controller method
 	 * annotations
-	 * @param string $controller the controllername or string
+	 * @param Controller $controller the controller
 	 * @param string $methodName the name of the method
 	 * @throws SecurityException when a security check fails
 	 */
@@ -145,7 +146,14 @@ class SecurityMiddleware extends Middleware {
 		// CSRF check - also registers the CSRF token since the session may be closed later
 		Util::callRegister();
 		if(!$this->reflector->hasAnnotation('NoCSRFRequired')) {
-			if(!$this->request->passesCSRFCheck()) {
+			/*
+			 * Only allow the CSRF check to fail on OCS Requests. This kind of
+			 * hacks around that we have no full token auth in place yet and we
+			 * do want to offer CSRF checks for web requests.
+			 */
+			if(!$this->request->passesCSRFCheck() && !(
+					$controller instanceof OCSController &&
+					$this->request->getHeader('OCS_APIREQUEST') === true)) {
 				throw new CrossSiteRequestForgeryException();
 			}
 		}
diff --git a/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php b/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php
index 487b83c0bef..6f675932135 100644
--- a/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php
+++ b/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php
@@ -35,22 +35,38 @@ use OC\Appframework\Middleware\Security\Exceptions\StrictCookieMissingException;
 use OC\AppFramework\Middleware\Security\SecurityMiddleware;
 use OC\AppFramework\Utility\ControllerMethodReflector;
 use OC\Security\CSP\ContentSecurityPolicy;
+use OC\Security\CSP\ContentSecurityPolicyManager;
+use OCP\AppFramework\Controller;
 use OCP\AppFramework\Http\RedirectResponse;
 use OCP\AppFramework\Http\JSONResponse;
 use OCP\AppFramework\Http\TemplateResponse;
+use OCP\ILogger;
+use OCP\INavigationManager;
+use OCP\IRequest;
+use OCP\IURLGenerator;
 
 
 class SecurityMiddlewareTest extends \Test\TestCase {
 
+	/** @var SecurityMiddleware|\PHPUnit_Framework_MockObject_MockObject */
 	private $middleware;
+	/** @var Controller|\PHPUnit_Framework_MockObject_MockObject */
 	private $controller;
+	/** @var SecurityException */
 	private $secException;
+	/** @var SecurityException */
 	private $secAjaxException;
+	/** @var IRequest|\PHPUnit_Framework_MockObject_MockObject */
 	private $request;
+	/** @var ControllerMethodReflector */
 	private $reader;
+	/** @var ILogger|\PHPUnit_Framework_MockObject_MockObject */
 	private $logger;
+	/** @var INavigationManager|\PHPUnit_Framework_MockObject_MockObject */
 	private $navigationManager;
+	/** @var IURLGenerator|\PHPUnit_Framework_MockObject_MockObject */
 	private $urlGenerator;
+	/** @var ContentSecurityPolicyManager|\PHPUnit_Framework_MockObject_MockObject */
 	private $contentSecurityPolicyManager;
 
 	protected function setUp() {
@@ -354,6 +370,45 @@ class SecurityMiddlewareTest extends \Test\TestCase {
 		$this->middleware->beforeController(__CLASS__, __FUNCTION__);
 	}
 
+	public function dataCsrfOcsController() {
+		$controller = $this->getMockBuilder('OCP\AppFramework\Controller')
+			->disableOriginalConstructor()
+			->getMock();
+		$ocsController = $this->getMockBuilder('OCP\AppFramework\OCSController')
+			->disableOriginalConstructor()
+			->getMock();
+
+		return [
+			[$controller, false, true],
+			[$controller, true,  true],
+
+			[$ocsController, false, true],
+			[$ocsController, true,  true],
+		];
+	}
+
+	/**
+	 * @dataProvider dataCsrfOcsController
+	 * @param Controller $controller
+	 * @param bool $hasOcsApiHeader
+	 * @param bool $exception
+	 */
+	public function testCsrfOcsController(Controller $controller, $hasOcsApiHeader, $exception) {
+		$this->request
+			->method('getHeader')
+			->willReturn($hasOcsApiHeader ? 'true' : null);
+		$this->request->expects($this->once())
+			->method('passesStrictCookieCheck')
+			->willReturn(true);
+
+		try {
+			$this->middleware->beforeController($controller, 'foo');
+			$this->assertFalse($exception);
+		} catch (CrossSiteRequestForgeryException $e) {
+			$this->assertTrue($exception);
+		}
+	}
+
 	/**
 	 * @NoCSRFRequired
 	 * @NoAdminRequired