Merge pull request #22424 from owncloud/add-generic-csrf-protection-to-webdav

Require CSRF token for non WebDAV authenticated requests

apps/dav/appinfo/v1/caldav.php

@@ -30,6 +30,7 @@ use Sabre\CalDAV\CalendarRoot;
 $authBackend = new Auth(
 	\OC::$server->getSession(),
 	\OC::$server->getUserSession(),
+	\OC::$server->getRequest(),
 	'principals/'
 );
 $principalBackend = new Principal(

apps/dav/appinfo/v1/carddav.php

@@ -32,6 +32,7 @@ use Sabre\CardDAV\Plugin;
 $authBackend = new Auth(
 	\OC::$server->getSession(),
 	\OC::$server->getUserSession(),
+	\OC::$server->getRequest(),
 	'principals/'
 );
 $principalBackend = new Principal(

apps/dav/appinfo/v1/webdav.php

@@ -41,6 +41,7 @@ $serverFactory = new \OCA\DAV\Connector\Sabre\ServerFactory(
 $authBackend = new \OCA\DAV\Connector\Sabre\Auth(
 	\OC::$server->getSession(),
 	\OC::$server->getUserSession(),
+	\OC::$server->getRequest(),
 	'principals/'
 );
 $requestUri = \OC::$server->getRequest()->getRequestUri();

apps/dav/lib/connector/sabre/auth.php

@@ -30,6 +30,7 @@
 namespace OCA\DAV\Connector\Sabre;
 
 use Exception;
+use OCP\IRequest;
 use OCP\ISession;
 use OCP\IUserSession;
 use Sabre\DAV\Auth\Backend\AbstractBasic;
@@ -45,17 +46,22 @@ class Auth extends AbstractBasic {
 	private $session;
 	/** @var IUserSession */
 	private $userSession;
+	/** @var IRequest */
+	private $request;
 
 	/**
 	 * @param ISession $session
 	 * @param IUserSession $userSession
+	 * @param IRequest $request
 	 * @param string $principalPrefix
 	 */
 	public function __construct(ISession $session,
 								IUserSession $userSession,
+								IRequest $request,
 								$principalPrefix = 'principals/users/') {
 		$this->session = $session;
 		$this->userSession = $userSession;
+		$this->request = $request;
 		$this->principalPrefix = $principalPrefix;
 	}
 
@@ -106,26 +112,6 @@ class Auth extends AbstractBasic {
 		}
 	}
 
-	/**
-	 * Returns information about the currently logged in username.
-	 *
-	 * If nobody is currently logged in, this method should return null.
-	 *
-	 * @return string|null
-	 */
-	public function getCurrentUser() {
-		$user = $this->userSession->getUser() ? $this->userSession->getUser()->getUID() : null;
-		if($user !== null && $this->isDavAuthenticated($user)) {
-			return $user;
-		}
-
-		if($user !== null && is_null($this->session->get(self::DAV_AUTHENTICATED))) {
-			return $user;
-		}
-
-		return null;
-	}
-
 	/**
 	 * @param RequestInterface $request
 	 * @param ResponseInterface $response
@@ -150,8 +136,19 @@ class Auth extends AbstractBasic {
 	 * @param RequestInterface $request
 	 * @param ResponseInterface $response
 	 * @return array
+	 * @throws NotAuthenticated
 	 */
 	private function auth(RequestInterface $request, ResponseInterface $response) {
+		// If request is not GET and not authenticated via WebDAV a requesttoken is required
+		if($this->userSession->isLoggedIn() &&
+			$this->request->getMethod() !== 'GET' &&
+			!$this->isDavAuthenticated($this->userSession->getUser()->getUID())) {
+			if(!$this->request->passesCSRFCheck()) {
+				$response->setStatus(401);
+				throw new \Sabre\DAV\Exception\NotAuthenticated('CSRF check not passed.');
+			}
+		}
+
 		if (\OC_User::handleApacheAuth() ||
 			//Fix for broken webdav clients
 			($this->userSession->isLoggedIn() && is_null($this->session->get(self::DAV_AUTHENTICATED))) ||

apps/dav/lib/dav/sharing/plugin.php

@@ -129,9 +129,6 @@ class Plugin extends ServerPlugin {
 			return;
 		}
 
-		// CSRF protection
-		$this->protectAgainstCSRF();
-
 		$requestBody = $request->getBodyAsString();
 
 		// If this request handler could not deal with this POST request, it
@@ -201,18 +198,4 @@ class Plugin extends ServerPlugin {
 		}
 	}
 
-	private function protectAgainstCSRF() {
-		$user = $this->auth->getCurrentUser();
-		if ($this->auth->isDavAuthenticated($user)) {
-			return true;
-		}
-
-		if ($this->request->passesCSRFCheck()) {
-			return true;
-		}
-
-		throw new BadRequest();
-	}
-
-
 }

apps/dav/lib/server.php

@@ -53,7 +53,8 @@ class Server {
 		// Backends
 		$authBackend = new Auth(
 			\OC::$server->getSession(),
-			\OC::$server->getUserSession()
+			\OC::$server->getUserSession(),
+			\OC::$server->getRequest()
 		);
 
 		// Set URL explicitly due to reverse-proxy situations

apps/dav/tests/unit/connector/sabre/auth.php

@@ -24,6 +24,7 @@
 
 namespace OCA\DAV\Tests\Unit\Connector\Sabre;
 
+use OCP\IRequest;
 use OCP\IUser;
 use Test\TestCase;
 use OCP\ISession;
@@ -42,6 +43,8 @@ class Auth extends TestCase {
 	private $auth;
 	/** @var IUserSession */
 	private $userSession;
+	/** @var IRequest */
+	private $request;
 
 	public function setUp() {
 		parent::setUp();
@@ -49,7 +52,13 @@ class Auth extends TestCase {
 			->disableOriginalConstructor()->getMock();
 		$this->userSession = $this->getMockBuilder('\OCP\IUserSession')
 			->disableOriginalConstructor()->getMock();
-		$this->auth = new \OCA\DAV\Connector\Sabre\Auth($this->session, $this->userSession);
+		$this->request = $this->getMockBuilder('\OCP\IRequest')
+			->disableOriginalConstructor()->getMock();
+		$this->auth = new \OCA\DAV\Connector\Sabre\Auth(
+			$this->session,
+			$this->userSession,
+			$this->request
+		);
 	}
 
 	public function testIsDavAuthenticatedWithoutDavSession() {
@@ -189,51 +198,57 @@ class Auth extends TestCase {
 		$this->assertFalse($this->invokePrivate($this->auth, 'validateUserPass', ['MyTestUser', 'MyTestPassword']));
 	}
 
-	public function testGetCurrentUserWithoutBeingLoggedIn() {
-		$this->assertSame(null, $this->auth->getCurrentUser());
-	}
-
-	public function testGetCurrentUserWithValidDAVLogin() {
+	/**
+	 * @expectedException \Sabre\DAV\Exception\NotAuthenticated
+	 * @expectedExceptionMessage CSRF check not passed.
+	 */
+	public function testAuthenticateAlreadyLoggedInWithoutCsrfTokenForNonGet() {
+		$request = $this->getMockBuilder('Sabre\HTTP\RequestInterface')
+				->disableOriginalConstructor()
+				->getMock();
+		$response = $this->getMockBuilder('Sabre\HTTP\ResponseInterface')
+				->disableOriginalConstructor()
+				->getMock();
+		$this->userSession
+			->expects($this->once())
+			->method('isLoggedIn')
+			->will($this->returnValue(true));
+		$this->session
+			->expects($this->once())
+			->method('get')
+			->with('AUTHENTICATED_TO_DAV_BACKEND')
+			->will($this->returnValue(null));
 		$user = $this->getMockBuilder('\OCP\IUser')
 			->disableOriginalConstructor()
 			->getMock();
 		$user->expects($this->once())
 			->method('getUID')
-			->will($this->returnValue('MyTestUser'));
+			->will($this->returnValue('MyWrongDavUser'));
 		$this->userSession
-			->expects($this->exactly(2))
+			->expects($this->once())
 			->method('getUser')
 			->will($this->returnValue($user));
-		$this->session
-			->expects($this->exactly(2))
-			->method('get')
-			->with('AUTHENTICATED_TO_DAV_BACKEND')
-			->will($this->returnValue('MyTestUser'));
 
-		$this->assertSame('MyTestUser', $this->auth->getCurrentUser());
+		$response = $this->auth->check($request, $response);
+		$this->assertEquals([true, 'principals/users/MyWrongDavUser'], $response);
 	}
 
-	public function testGetCurrentUserWithoutAnyDAVLogin() {
-		$user = $this->getMockBuilder('\OCP\IUser')
+	public function testAuthenticateAlreadyLoggedInWithoutCsrfTokenForGet() {
+		$request = $this->getMockBuilder('Sabre\HTTP\RequestInterface')
+			->disableOriginalConstructor()
+			->getMock();
+		$response = $this->getMockBuilder('Sabre\HTTP\ResponseInterface')
 			->disableOriginalConstructor()
 			->getMock();
-		$user->expects($this->once())
-			->method('getUID')
-			->will($this->returnValue('MyTestUser'));
 		$this->userSession
 			->expects($this->exactly(2))
-			->method('getUser')
-			->will($this->returnValue($user));
+			->method('isLoggedIn')
+			->will($this->returnValue(true));
 		$this->session
-			->expects($this->exactly(2))
+			->expects($this->once())
 			->method('get')
 			->with('AUTHENTICATED_TO_DAV_BACKEND')
 			->will($this->returnValue(null));
-
-		$this->assertSame('MyTestUser', $this->auth->getCurrentUser());
-	}
-
-	public function testGetCurrentUserWithWrongDAVUser() {
 		$user = $this->getMockBuilder('\OCP\IUser')
 			->disableOriginalConstructor()
 			->getMock();
@@ -241,47 +256,49 @@ class Auth extends TestCase {
 			->method('getUID')
 			->will($this->returnValue('MyWrongDavUser'));
 		$this->userSession
-			->expects($this->exactly(2))
+			->expects($this->once())
 			->method('getUser')
 			->will($this->returnValue($user));
-		$this->session
-			->expects($this->exactly(3))
-			->method('get')
-			->with('AUTHENTICATED_TO_DAV_BACKEND')
-			->will($this->returnValue('AnotherUser'));
+		$this->request
+			->expects($this->once())
+			->method('getMethod')
+			->willReturn('GET');
 
-		$this->assertSame(null, $this->auth->getCurrentUser());
+		$response = $this->auth->check($request, $response);
+		$this->assertEquals([true, 'principals/users/MyWrongDavUser'], $response);
 	}
 
-	public function testAuthenticateAlreadyLoggedIn() {
+
+	public function testAuthenticateAlreadyLoggedInWithCsrfTokenForGet() {
 		$request = $this->getMockBuilder('Sabre\HTTP\RequestInterface')
-				->disableOriginalConstructor()
-				->getMock();
+			->disableOriginalConstructor()
+			->getMock();
 		$response = $this->getMockBuilder('Sabre\HTTP\ResponseInterface')
-				->disableOriginalConstructor()
-				->getMock();
+			->disableOriginalConstructor()
+			->getMock();
 		$this->userSession
-			->expects($this->once())
+			->expects($this->exactly(2))
 			->method('isLoggedIn')
 			->will($this->returnValue(true));
 		$this->session
-			->expects($this->once())
+			->expects($this->exactly(2))
 			->method('get')
 			->with('AUTHENTICATED_TO_DAV_BACKEND')
 			->will($this->returnValue(null));
 		$user = $this->getMockBuilder('\OCP\IUser')
 			->disableOriginalConstructor()
 			->getMock();
-		$user->expects($this->once())
+		$user->expects($this->exactly(2))
 			->method('getUID')
 			->will($this->returnValue('MyWrongDavUser'));
 		$this->userSession
-			->expects($this->once())
+			->expects($this->exactly(2))
 			->method('getUser')
 			->will($this->returnValue($user));
-		$this->session
+		$this->request
 			->expects($this->once())
-			->method('close');
+			->method('passesCSRFCheck')
+			->willReturn(true);
 
 		$response = $this->auth->check($request, $response);
 		$this->assertEquals([true, 'principals/users/MyWrongDavUser'], $response);

core/js/files/client.js

@@ -37,7 +37,10 @@
 		}
 
 		url += options.host + this._root;
-		this._defaultHeaders = options.defaultHeaders || {'X-Requested-With': 'XMLHttpRequest'};
+		this._defaultHeaders = options.defaultHeaders || {
+				'X-Requested-With': 'XMLHttpRequest',
+				'requesttoken': OC.requestToken
+			};
 		this._baseUrl = url;
 
 		var clientOptions = {

core/js/oc-backbone-webdav.js

@@ -240,7 +240,8 @@
 			return options.url;
 		};
 		var headers = _.extend({
-			'X-Requested-With': 'XMLHttpRequest'
+			'X-Requested-With': 'XMLHttpRequest',
+			'requesttoken': OC.requestToken
 		}, options.headers);
 		if (options.type === 'PROPFIND') {
 			return callPropFind(client, options, model, headers);