Merge pull request #6630 from nextcloud/same_site_cookie_middleware

Handle SameSiteCookie check for index.php in AppFramework Middleware

lib/base.php

@@ -559,24 +559,20 @@ class OC {
 			if($currentUrl === '/index.php/apps/user_saml/saml/acs' || $currentUrl === '/apps/user_saml/saml/acs') {
 				return;
 			}
-			// For the "index.php" endpoint only a lax cookie is required.
+			// index.php routes are handled in the middleware
 			if($processingScript === 'index.php') {
-				if(!$request->passesLaxCookieCheck()) {
-					self::sendSameSiteCookies();
-					header('Location: '.$_SERVER['REQUEST_URI']);
+				return;
+			}
+
+			// All other endpoints require the lax and the strict cookie
+			if(!$request->passesStrictCookieCheck()) {
+				self::sendSameSiteCookies();
+				// Debug mode gets access to the resources without strict cookie
+				// due to the fact that the SabreDAV browser also lives there.
+				if(!\OC::$server->getConfig()->getSystemValue('debug', false)) {
+					http_response_code(\OCP\AppFramework\Http::STATUS_SERVICE_UNAVAILABLE);
 					exit();
 				}
-			} else {
-				// All other endpoints require the lax and the strict cookie
-				if(!$request->passesStrictCookieCheck()) {
-					self::sendSameSiteCookies();
-					// Debug mode gets access to the resources without strict cookie
-					// due to the fact that the SabreDAV browser also lives there.
-					if(!\OC::$server->getConfig()->getSystemValue('debug', false)) {
-						http_response_code(\OCP\AppFramework\Http::STATUS_SERVICE_UNAVAILABLE);
-						exit();
-					}
-				}
 			}
 		} elseif(!isset($_COOKIE['nc_sameSiteCookielax']) || !isset($_COOKIE['nc_sameSiteCookiestrict'])) {
 			self::sendSameSiteCookies();

lib/composer/composer/autoload_classmap.php

@@ -306,12 +306,14 @@ return array(
     'OC\\AppFramework\\Middleware\\Security\\CORSMiddleware' => $baseDir . '/lib/private/AppFramework/Middleware/Security/CORSMiddleware.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',
     'OC\\AppFramework\\Middleware\\Security\\Exceptions\\NotAdminException' => $baseDir . '/lib/private/AppFramework/Middleware/Security/Exceptions/NotAdminException.php',
     'OC\\AppFramework\\Middleware\\Security\\Exceptions\\NotConfirmedException' => $baseDir . '/lib/private/AppFramework/Middleware/Security/Exceptions/NotConfirmedException.php',
     'OC\\AppFramework\\Middleware\\Security\\Exceptions\\NotLoggedInException' => $baseDir . '/lib/private/AppFramework/Middleware/Security/Exceptions/NotLoggedInException.php',
     'OC\\AppFramework\\Middleware\\Security\\Exceptions\\SecurityException' => $baseDir . '/lib/private/AppFramework/Middleware/Security/Exceptions/SecurityException.php',
     'OC\\AppFramework\\Middleware\\Security\\Exceptions\\StrictCookieMissingException' => $baseDir . '/lib/private/AppFramework/Middleware/Security/Exceptions/StrictCookieMissingException.php',
     'OC\\AppFramework\\Middleware\\Security\\RateLimitingMiddleware' => $baseDir . '/lib/private/AppFramework/Middleware/Security/RateLimitingMiddleware.php',
+    'OC\\AppFramework\\Middleware\\Security\\SameSiteCookieMiddleware' => $baseDir . '/lib/private/AppFramework/Middleware/Security/SameSiteCookieMiddleware.php',
     'OC\\AppFramework\\Middleware\\Security\\SecurityMiddleware' => $baseDir . '/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php',
     'OC\\AppFramework\\Middleware\\SessionMiddleware' => $baseDir . '/lib/private/AppFramework/Middleware/SessionMiddleware.php',
     'OC\\AppFramework\\OCS\\BaseResponse' => $baseDir . '/lib/private/AppFramework/OCS/BaseResponse.php',

lib/composer/composer/autoload_static.php

@@ -336,12 +336,14 @@ class ComposerStaticInit53792487c5a8370acc0b06b1a864ff4c
         'OC\\AppFramework\\Middleware\\Security\\CORSMiddleware' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/Security/CORSMiddleware.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',
         'OC\\AppFramework\\Middleware\\Security\\Exceptions\\NotAdminException' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/Security/Exceptions/NotAdminException.php',
         'OC\\AppFramework\\Middleware\\Security\\Exceptions\\NotConfirmedException' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/Security/Exceptions/NotConfirmedException.php',
         'OC\\AppFramework\\Middleware\\Security\\Exceptions\\NotLoggedInException' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/Security/Exceptions/NotLoggedInException.php',
         'OC\\AppFramework\\Middleware\\Security\\Exceptions\\SecurityException' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/Security/Exceptions/SecurityException.php',
         'OC\\AppFramework\\Middleware\\Security\\Exceptions\\StrictCookieMissingException' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/Security/Exceptions/StrictCookieMissingException.php',
         'OC\\AppFramework\\Middleware\\Security\\RateLimitingMiddleware' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/Security/RateLimitingMiddleware.php',
+        'OC\\AppFramework\\Middleware\\Security\\SameSiteCookieMiddleware' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/Security/SameSiteCookieMiddleware.php',
         'OC\\AppFramework\\Middleware\\Security\\SecurityMiddleware' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php',
         'OC\\AppFramework\\Middleware\\SessionMiddleware' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/SessionMiddleware.php',
         'OC\\AppFramework\\OCS\\BaseResponse' => __DIR__ . '/../../..' . '/lib/private/AppFramework/OCS/BaseResponse.php',

lib/private/AppFramework/DependencyInjection/DIContainer.php

@@ -291,9 +291,17 @@ class DIContainer extends SimpleContainer implements IAppContainer {
 			);
 		});
 
+		$this->registerService(OC\AppFramework\Middleware\Security\SameSiteCookieMiddleware::class, function (SimpleContainer $c) {
+			return new OC\AppFramework\Middleware\Security\SameSiteCookieMiddleware(
+				$c['Request'],
+				$c['ControllerMethodReflector']
+			);
+		});
+
 		$middleWares = &$this->middleWares;
 		$this->registerService('MiddlewareDispatcher', function($c) use (&$middleWares) {
 			$dispatcher = new MiddlewareDispatcher();
+			$dispatcher->registerMiddleware($c[OC\AppFramework\Middleware\Security\SameSiteCookieMiddleware::class]);
 			$dispatcher->registerMiddleware($c['CORSMiddleware']);
 			$dispatcher->registerMiddleware($c['OCSMiddleware']);
 			$dispatcher->registerMiddleware($c['SecurityMiddleware']);

lib/private/AppFramework/Http/Request.php

@@ -507,7 +507,7 @@ class Request implements \ArrayAccess, \Countable, IRequest {
 	 *
 	 * @return array
 	 */
-	protected function getCookieParams() {
+	public function getCookieParams() {
 		return session_get_cookie_params();
 	}
 

lib/private/AppFramework/Middleware/Security/Exceptions/LaxSameSiteCookieFailedException.php

@@ -0,0 +1,38 @@
+<?php
+/**
+ * @copyright 2017, 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\Exceptions;
+
+use OCP\AppFramework\Http;
+
+/**
+ * Class LaxSameSiteCookieFailedException is thrown when a request doesn't pass
+ * the required LaxCookie check on index.php
+ *
+ * @package OC\AppFramework\Middleware\Security\Exceptions
+ */
+class LaxSameSiteCookieFailedException extends SecurityException {
+	public function __construct() {
+		parent::__construct('Lax Same Site Cookie is invalid in request.', Http::STATUS_PRECONDITION_FAILED);
+	}
+}

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

@@ -0,0 +1,106 @@
+<?php
+/**
+ * @copyright 2017, 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\AppFramework\Http\Request;
+use OC\AppFramework\Middleware\Security\Exceptions\LaxSameSiteCookieFailedException;
+use OC\AppFramework\Utility\ControllerMethodReflector;
+use OCP\AppFramework\Http;
+use OCP\AppFramework\Http\Response;
+use OCP\AppFramework\Middleware;
+
+class SameSiteCookieMiddleware extends Middleware {
+
+	/** @var Request */
+	private $request;
+
+	/** @var ControllerMethodReflector */
+	private $reflector;
+
+	public function __construct(Request $request,
+								ControllerMethodReflector $reflector) {
+		$this->request = $request;
+		$this->reflector = $reflector;
+	}
+
+	public function beforeController($controller, $methodName) {
+		$requestUri = $this->request->getScriptName();
+		$processingScript = explode('/', $requestUri);
+		$processingScript = $processingScript[count($processingScript)-1];
+
+		if ($processingScript !== 'index.php') {
+			return;
+		}
+
+		$noSSC = $this->reflector->hasAnnotation('NoSameSiteCookieRequired');
+		if ($noSSC) {
+			return;
+		}
+
+		if (!$this->request->passesLaxCookieCheck()) {
+			throw new LaxSameSiteCookieFailedException();
+		}
+	}
+
+	public function afterException($controller, $methodName, \Exception $exception) {
+		if ($exception instanceof LaxSameSiteCookieFailedException) {
+			$respone = new Response();
+			$respone->setStatus(Http::STATUS_FOUND);
+			$respone->addHeader('Location', $this->request->getRequestUri());
+
+			$this->setSameSiteCookie();
+
+			return $respone;
+		}
+
+		throw $exception;
+	}
+
+	protected function setSameSiteCookie() {
+		$cookieParams = $this->request->getCookieParams();
+		$secureCookie = ($cookieParams['secure'] === true) ? 'secure; ' : '';
+		$policies = [
+			'lax',
+			'strict',
+		];
+
+		// Append __Host to the cookie if it meets the requirements
+		$cookiePrefix = '';
+		if($cookieParams['secure'] === true && $cookieParams['path'] === '/') {
+			$cookiePrefix = '__Host-';
+		}
+
+		foreach($policies as $policy) {
+			header(
+				sprintf(
+					'Set-Cookie: %snc_sameSiteCookie%s=true; path=%s; httponly;' . $secureCookie . 'expires=Fri, 31-Dec-2100 23:59:59 GMT; SameSite=%s',
+					$cookiePrefix,
+					$policy,
+					$cookieParams['path'],
+					$policy
+				),
+				false
+			);
+		}
+	}
+}

tests/lib/AppFramework/Http/RequestTest.php

@@ -1568,8 +1568,18 @@ class RequestTest extends \Test\TestCase {
 	}
 
 	public function testGetCookieParams() {
-		$request = $this->createMock(Request::class);
-		$actual = $this->invokePrivate($request, 'getCookieParams');
+		/** @var Request $request */
+		$request = $this->getMockBuilder(Request::class)
+			->setMethods(['getScriptName'])
+			->setConstructorArgs([
+				[],
+				$this->secureRandom,
+				$this->config,
+				$this->csrfTokenManager,
+				$this->stream
+			])
+			->getMock();
+		$actual = $request->getCookieParams();
 		$this->assertSame(session_get_cookie_params(), $actual);
 	}
 

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

@@ -0,0 +1,133 @@
+<?php
+/**
+ * @copyright 2017, 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\Http\Request;
+use OC\AppFramework\Middleware\Security\Exceptions\LaxSameSiteCookieFailedException;
+use OC\AppFramework\Middleware\Security\Exceptions\SecurityException;
+use OC\AppFramework\Middleware\Security\SameSiteCookieMiddleware;
+use OC\AppFramework\Utility\ControllerMethodReflector;
+use OCP\AppFramework\Controller;
+use OCP\AppFramework\Http;
+use Test\TestCase;
+
+class SameSiteCookieMiddlewareTest extends TestCase {
+
+	/** @var SameSiteCookieMiddleware */
+	private $middleware;
+
+	/** @var Request|\PHPUnit_Framework_MockObject_MockObject */
+	private $request;
+
+	/** @var ControllerMethodReflector|\PHPUnit_Framework_MockObject_MockObject */
+	private $reflector;
+
+	public function setUp() {
+		parent::setUp();
+
+		$this->request = $this->createMock(Request::class);
+		$this->reflector = $this->createMock(ControllerMethodReflector::class);
+		$this->middleware = new SameSiteCookieMiddleware($this->request, $this->reflector);
+	}
+
+	public function testBeforeControllerNoIndex() {
+		$this->request->method('getScriptName')
+			->willReturn('/ocs/v2.php');
+
+		$this->middleware->beforeController($this->createMock(Controller::class), 'foo');
+	}
+
+	public function testBeforeControllerIndexHasAnnotation() {
+		$this->request->method('getScriptName')
+			->willReturn('/index.php');
+
+		$this->reflector->method('hasAnnotation')
+			->with('NoSameSiteCookieRequired')
+			->willReturn(true);
+
+		$this->middleware->beforeController($this->createMock(Controller::class), 'foo');
+	}
+
+	public function testBeforeControllerIndexNoAnnotationPassingCheck() {
+		$this->request->method('getScriptName')
+			->willReturn('/index.php');
+
+		$this->reflector->method('hasAnnotation')
+			->with('NoSameSiteCookieRequired')
+			->willReturn(false);
+
+		$this->request->method('passesLaxCookieCheck')
+			->willReturn(true);
+
+		$this->middleware->beforeController($this->createMock(Controller::class), 'foo');
+	}
+
+	public function testBeforeControllerIndexNoAnnotationFailingCheck() {
+		$this->expectException(LaxSameSiteCookieFailedException::class);
+
+		$this->request->method('getScriptName')
+			->willReturn('/index.php');
+
+		$this->reflector->method('hasAnnotation')
+			->with('NoSameSiteCookieRequired')
+			->willReturn(false);
+
+		$this->request->method('passesLaxCookieCheck')
+			->willReturn(false);
+
+		$this->middleware->beforeController($this->createMock(Controller::class), 'foo');
+	}
+
+	public function testAfterExceptionNoLaxCookie() {
+		$ex = new SecurityException();
+
+		try {
+			$this->middleware->afterException($this->createMock(Controller::class), 'foo', $ex);
+			$this->fail();
+		} catch (\Exception $e) {
+			$this->assertSame($ex, $e);
+		}
+	}
+
+	public function testAfterExceptionLaxCookie() {
+		$ex = new LaxSameSiteCookieFailedException();
+
+		$this->request->method('getRequestUri')
+			->willReturn('/myrequri');
+
+		$middleware = $this->getMockBuilder(SameSiteCookieMiddleware::class)
+			->setConstructorArgs([$this->request, $this->reflector])
+			->setMethods(['setSameSiteCookie'])
+			->getMock();
+
+		$middleware->expects($this->once())
+			->method('setSameSiteCookie');
+
+		$resp = $middleware->afterException($this->createMock(Controller::class), 'foo', $ex);
+
+		$this->assertSame(Http::STATUS_FOUND, $resp->getStatus());
+
+		$headers = $resp->getHeaders();
+		$this->assertSame('/myrequri', $headers['Location']);
+	}
+}