Merge pull request #52810 from nextcloud/artonge/feat/do_not_require_samesite_strict_cookie_on_public.php

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');
 	}

apps/files_sharing/lib/DefaultPublicShareTemplateProvider.php

@@ -149,10 +149,7 @@ class DefaultPublicShareTemplateProvider implements IPublicShareTemplateProvider
 		$headerActions = [];
 		if ($view !== 'public-file-drop' && !$share->getHideDownload()) {
 			// The download URL is used for the "download" header action as well as in some cases for the direct link
-			$downloadUrl = $this->urlGenerator->linkToRouteAbsolute('files_sharing.sharecontroller.downloadShare', [
-				'token' => $token,
-				'filename' => ($shareNode instanceof File) ? $shareNode->getName() : null,
-			]);
+			$downloadUrl = $this->urlGenerator->getAbsoluteURL('/public.php/dav/files/' . $token . '/?accept=zip');
 
 			// If not a file drop, then add the download header action
 			$headerActions[] = new SimpleMenuAction('download', $this->l10n->t('Download'), 'icon-download', $downloadUrl, 0, (string)$shareNode->getSize());

apps/files_sharing/tests/Controller/ShareControllerTest.php

@@ -261,8 +261,12 @@ class ShareControllerTest extends \Test\TestCase {
 				['files_sharing.sharecontroller.showShare', ['token' => 'token'], 'shareUrl'],
 				// this share is not an image to the default preview is used
 				['files_sharing.PublicPreview.getPreview', ['x' => 256, 'y' => 256, 'file' => $share->getTarget(), 'token' => 'token'], 'previewUrl'],
-				// for the direct link
-				['files_sharing.sharecontroller.downloadShare', ['token' => 'token', 'filename' => $filename ], 'downloadUrl'],
+			]);
+
+		$this->urlGenerator->expects($this->once())
+			->method('getAbsoluteURL')
+			->willReturnMap([
+				['/public.php/dav/files/token/?accept=zip', 'downloadUrl'],
 			]);
 
 		$this->previewManager->method('isMimeSupported')->with('text/plain')->willReturn(true);
@@ -552,8 +556,12 @@ class ShareControllerTest extends \Test\TestCase {
 				['files_sharing.sharecontroller.showShare', ['token' => 'token'], 'shareUrl'],
 				// this share is not an image to the default preview is used
 				['files_sharing.PublicPreview.getPreview', ['x' => 256, 'y' => 256, 'file' => $share->getTarget(), 'token' => 'token'], 'previewUrl'],
-				// for the direct link
-				['files_sharing.sharecontroller.downloadShare', ['token' => 'token', 'filename' => $filename ], 'downloadUrl'],
+			]);
+
+		$this->urlGenerator->expects($this->once())
+			->method('getAbsoluteURL')
+			->willReturnMap([
+				['/public.php/dav/files/token/?accept=zip', 'downloadUrl'],
 			]);
 
 		$this->previewManager->method('isMimeSupported')->with('text/plain')->willReturn(true);

cypress/e2e/files_sharing/public-share/header-menu.cy.ts

@@ -53,7 +53,7 @@ describe('files_sharing: Public share - header actions menu', { testIsolation: t
 		cy.findByRole('menuitem', { name: 'Direct link' })
 			.should('be.visible')
 			.and('have.attr', 'href')
-			.then((attribute) => expect(attribute).to.match(/^http:\/\/.+\/download$/))
+			.then((attribute) => expect(attribute).to.match(new RegExp(`^${Cypress.env('baseUrl')}/public.php/dav/files/.+/?accept=zip$`)))
 		// see menu closes on click
 		cy.findByRole('menuitem', { name: 'Direct link' })
 			.click()
@@ -188,7 +188,7 @@ describe('files_sharing: Public share - header actions menu', { testIsolation: t
 				cy.findByRole('menuitem', { name: 'Direct link' })
 					.should('be.visible')
 					.and('have.attr', 'href')
-					.then((attribute) => expect(attribute).to.match(/^http:\/\/.+\/download$/))
+					.then((attribute) => expect(attribute).to.match(new RegExp(`^${Cypress.env('baseUrl')}/public.php/dav/files/.+/?accept=zip$`)))
 				// See remote share works
 				cy.findByRole('menuitem', { name: /Add to your/i })
 					.should('be.visible')

lib/base.php

@@ -551,10 +551,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;
 			}