fix: Move CSRF check from base to PublicAuth for public.php

This currently prevent directly accessing a ressource when clicking on a link on a third party site. Example, clicking on `https://example.com/public.php/dav/files/pqLWcA269zfzXez/?accept=zip` in a GitHub comment.

Skipping the check is an issue with password protected shares, as it allows third party sites to request the ressource when the user already entered the password, aka CSRF.  So after removing the check from `base.php`, we need to add the it again in the `PublicAuth` plugin.

We also add a redirect to be helpful to the user.

**Warning**: this adds the limitation that clicking on a direct download link for password protected shares will redirect you to the password form, and then to the main share view.

Fix #52482

Signed-off-by: Louis Chemineau <louis@chmn.me>

apps/dav/appinfo/v2/publicremote.php

@@ -29,6 +29,7 @@ use OCP\IPreview;
 use OCP\IRequest;
 use OCP\ISession;
 use OCP\ITagManager;
+use OCP\IURLGenerator;
 use OCP\IUserSession;
 use OCP\L10N\IFactory;
 use OCP\Security\Bruteforce\IThrottler;
@@ -56,7 +57,8 @@ $authBackend = new PublicAuth(
 	Server::get(IManager::class),
 	$session,
 	Server::get(IThrottler::class),
-	Server::get(LoggerInterface::class)
+	Server::get(LoggerInterface::class),
+	Server::get(IURLGenerator::class),
 );
 $authPlugin = new \Sabre\DAV\Auth\Plugin($authBackend);
 

apps/dav/lib/Connector/Sabre/PublicAuth.php

@@ -14,6 +14,7 @@ namespace OCA\DAV\Connector\Sabre;
 use OCP\Defaults;
 use OCP\IRequest;
 use OCP\ISession;
+use OCP\IURLGenerator;
 use OCP\Security\Bruteforce\IThrottler;
 use OCP\Security\Bruteforce\MaxDelayReached;
 use OCP\Share\Exceptions\ShareNotFound;
@@ -23,6 +24,7 @@ use Psr\Log\LoggerInterface;
 use Sabre\DAV\Auth\Backend\AbstractBasic;
 use Sabre\DAV\Exception\NotAuthenticated;
 use Sabre\DAV\Exception\NotFound;
+use Sabre\DAV\Exception\PreconditionFailed;
 use Sabre\DAV\Exception\ServiceUnavailable;
 use Sabre\HTTP;
 use Sabre\HTTP\RequestInterface;
@@ -45,6 +47,7 @@ class PublicAuth extends AbstractBasic {
 		private ISession $session,
 		private IThrottler $throttler,
 		private LoggerInterface $logger,
+		private IURLGenerator $urlGenerator,
 	) {
 		// setup realm
 		$defaults = new Defaults();
@@ -52,10 +55,6 @@ class PublicAuth extends AbstractBasic {
 	}
 
 	/**
-	 * @param RequestInterface $request
-	 * @param ResponseInterface $response
-	 *
-	 * @return array
 	 * @throws NotAuthenticated
 	 * @throws MaxDelayReached
 	 * @throws ServiceUnavailable
@@ -64,6 +63,10 @@ class PublicAuth extends AbstractBasic {
 		try {
 			$this->throttler->sleepDelayOrThrowOnMax($this->request->getRemoteAddress(), self::BRUTEFORCE_ACTION);
 
+			if (count($_COOKIE) > 0 && !$this->request->passesStrictCookieCheck() && $this->getShare()->getPassword() !== null) {
+				throw new PreconditionFailed('Strict cookie check failed');
+			}
+
 			$auth = new HTTP\Auth\Basic(
 				$this->realm,
 				$request,
@@ -80,6 +83,15 @@ class PublicAuth extends AbstractBasic {
 		} catch (NotAuthenticated|MaxDelayReached $e) {
 			$this->throttler->registerAttempt(self::BRUTEFORCE_ACTION, $this->request->getRemoteAddress());
 			throw $e;
+		} catch (PreconditionFailed $e) {
+			$response->setHeader(
+				'Location',
+				$this->urlGenerator->linkToRoute(
+					'files_sharing.share.showShare',
+					[ 'token' => $this->getToken() ],
+				),
+			);
+			throw $e;
 		} catch (\Exception $e) {
 			$class = get_class($e);
 			$msg = $e->getMessage();
@@ -90,7 +102,6 @@ class PublicAuth extends AbstractBasic {
 
 	/**
 	 * Extract token from request url
-	 * @return string
 	 * @throws NotFound
 	 */
 	private function getToken(): string {
@@ -107,7 +118,7 @@ class PublicAuth extends AbstractBasic {
 
 	/**
 	 * Check token validity
-	 * @return array
+	 *
 	 * @throws NotFound
 	 * @throws NotAuthenticated
 	 */
@@ -155,15 +166,13 @@ class PublicAuth extends AbstractBasic {
 	protected function validateUserPass($username, $password) {
 		$this->throttler->sleepDelayOrThrowOnMax($this->request->getRemoteAddress(), self::BRUTEFORCE_ACTION);
 
-		$token = $this->getToken();
 		try {
-			$share = $this->shareManager->getShareByToken($token);
+			$share = $this->getShare();
 		} catch (ShareNotFound $e) {
 			$this->throttler->registerAttempt(self::BRUTEFORCE_ACTION, $this->request->getRemoteAddress());
 			return false;
 		}
 
-		$this->share = $share;
 		\OC_User::setIncognitoMode(true);
 
 		// check if the share is password protected
@@ -206,7 +215,13 @@ class PublicAuth extends AbstractBasic {
 	}
 
 	public function getShare(): IShare {
-		assert($this->share !== null);
+		$token = $this->getToken();
+
+		if ($this->share === null) {
+			$share = $this->shareManager->getShareByToken($token);
+			$this->share = $share;
+		}
+
 		return $this->share;
 	}
 }

apps/dav/tests/unit/Connector/Sabre/PublicAuthTest.php

@@ -10,10 +10,12 @@ namespace OCA\DAV\Tests\unit\Connector;
 use OCA\DAV\Connector\Sabre\PublicAuth;
 use OCP\IRequest;
 use OCP\ISession;
+use OCP\IURLGenerator;
 use OCP\Security\Bruteforce\IThrottler;
 use OCP\Share\Exceptions\ShareNotFound;
 use OCP\Share\IManager;
 use OCP\Share\IShare;
+use PHPUnit\Framework\MockObject\MockObject;
 use Psr\Log\LoggerInterface;
 
 /**
@@ -25,21 +27,15 @@ use Psr\Log\LoggerInterface;
  */
 class PublicAuthTest extends \Test\TestCase {
 
-	/** @var ISession|MockObject */
-	private $session;
-	/** @var IRequest|MockObject */
-	private $request;
-	/** @var IManager|MockObject */
-	private $shareManager;
-	/** @var PublicAuth */
-	private $auth;
-	/** @var IThrottler|MockObject */
-	private $throttler;
-	/** @var LoggerInterface|MockObject */
-	private $logger;
+	private ISession&MockObject $session;
+	private IRequest&MockObject $request;
+	private IManager&MockObject $shareManager;
+	private PublicAuth $auth;
+	private IThrottler&MockObject $throttler;
+	private LoggerInterface&MockObject $logger;
+	private IURLGenerator&MockObject $urlGenerator;
 
-	/** @var string */
-	private $oldUser;
+	private string $oldUser;
 
 	protected function setUp(): void {
 		parent::setUp();
@@ -49,6 +45,7 @@ class PublicAuthTest extends \Test\TestCase {
 		$this->shareManager = $this->createMock(IManager::class);
 		$this->throttler = $this->createMock(IThrottler::class);
 		$this->logger = $this->createMock(LoggerInterface::class);
+		$this->urlGenerator = $this->createMock(IURLGenerator::class);
 
 		$this->auth = new PublicAuth(
 			$this->request,
@@ -56,6 +53,7 @@ class PublicAuthTest extends \Test\TestCase {
 			$this->session,
 			$this->throttler,
 			$this->logger,
+			$this->urlGenerator,
 		);
 
 		// Store current user
@@ -137,7 +135,7 @@ class PublicAuthTest extends \Test\TestCase {
 
 		$this->session->method('exists')->with('public_link_authenticated')->willReturn(true);
 		$this->session->method('get')->with('public_link_authenticated')->willReturn('42');
-	
+
 		$result = $this->invokePrivate($this->auth, 'checkToken');
 		$this->assertSame([true, 'principals/GX9HSGQrGE'], $result);
 	}
@@ -158,7 +156,7 @@ class PublicAuthTest extends \Test\TestCase {
 			->willReturn($share);
 
 		$this->session->method('exists')->with('public_link_authenticated')->willReturn(false);
-	
+
 		$this->expectException(\Sabre\DAV\Exception\NotAuthenticated::class);
 		$this->invokePrivate($this->auth, 'checkToken');
 	}
@@ -180,7 +178,7 @@ class PublicAuthTest extends \Test\TestCase {
 
 		$this->session->method('exists')->with('public_link_authenticated')->willReturn(false);
 		$this->session->method('get')->with('public_link_authenticated')->willReturn('43');
-	
+
 		$this->expectException(\Sabre\DAV\Exception\NotAuthenticated::class);
 		$this->invokePrivate($this->auth, 'checkToken');
 	}

lib/base.php

@@ -550,10 +550,10 @@ class OC {
 			$processingScript = explode('/', $requestUri);
 			$processingScript = $processingScript[count($processingScript) - 1];
 
-			// index.php routes are handled in the middleware
-			// and cron.php does not need any authentication at all
-			if ($processingScript === 'index.php'
-				|| $processingScript === 'cron.php') {
+			if ($processingScript === 'index.php' // index.php routes are handled in the middleware
+				|| $processingScript === 'cron.php' // and cron.php does not need any authentication at all
+				|| $processingScript === 'public.php' // For public.php, auth for password protected shares is done in the PublicAuth plugin
+			) {
 				return;
 			}