diff --git a/apps/dav/appinfo/v2/publicremote.php b/apps/dav/appinfo/v2/publicremote.php index fbb3ddd2cb346..e089fa7bb6223 100644 --- a/apps/dav/appinfo/v2/publicremote.php +++ b/apps/dav/appinfo/v2/publicremote.php @@ -29,6 +29,7 @@ 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 @@ 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); diff --git a/apps/dav/lib/Connector/Sabre/PublicAuth.php b/apps/dav/lib/Connector/Sabre/PublicAuth.php index b5d9ce3db7211..2ca1c25e2f69a 100644 --- a/apps/dav/lib/Connector/Sabre/PublicAuth.php +++ b/apps/dav/lib/Connector/Sabre/PublicAuth.php @@ -14,6 +14,7 @@ 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 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 @@ public function __construct( private ISession $session, private IThrottler $throttler, private LoggerInterface $logger, + private IURLGenerator $urlGenerator, ) { // setup realm $defaults = new Defaults(); @@ -52,10 +55,6 @@ public function __construct( } /** - * @param RequestInterface $request - * @param ResponseInterface $response - * - * @return array * @throws NotAuthenticated * @throws MaxDelayReached * @throws ServiceUnavailable @@ -64,6 +63,10 @@ public function check(RequestInterface $request, ResponseInterface $response): a 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 @@ public function check(RequestInterface $request, ResponseInterface $response): a } 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 @@ public function check(RequestInterface $request, ResponseInterface $response): a /** * Extract token from request url - * @return string * @throws NotFound */ private function getToken(): string { @@ -107,7 +118,7 @@ private function getToken(): string { /** * Check token validity - * @return array + * * @throws NotFound * @throws NotAuthenticated */ @@ -155,15 +166,13 @@ private function checkToken(): array { 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 @@ protected function validateUserPass($username, $password) { } 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; } } diff --git a/apps/dav/tests/unit/Connector/Sabre/PublicAuthTest.php b/apps/dav/tests/unit/Connector/Sabre/PublicAuthTest.php index 00bddf2e69cd4..67e7f6af675b9 100644 --- a/apps/dav/tests/unit/Connector/Sabre/PublicAuthTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/PublicAuthTest.php @@ -10,10 +10,12 @@ 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 @@ */ 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; - - /** @var string */ - private $oldUser; + 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; + + private string $oldUser; protected function setUp(): void { parent::setUp(); @@ -49,6 +45,7 @@ protected function setUp(): void { $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 @@ protected function setUp(): void { $this->session, $this->throttler, $this->logger, + $this->urlGenerator, ); // Store current user @@ -137,7 +135,7 @@ public function testCheckTokenAlreadyAuthenticated(): void { $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 @@ public function testCheckTokenPasswordNotAuthenticated(): void { ->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 @@ public function testCheckTokenPasswordAuthenticatedWrongShare(): void { $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'); } diff --git a/apps/files_sharing/lib/DefaultPublicShareTemplateProvider.php b/apps/files_sharing/lib/DefaultPublicShareTemplateProvider.php index 686ba32fd49e9..910d00a597251 100644 --- a/apps/files_sharing/lib/DefaultPublicShareTemplateProvider.php +++ b/apps/files_sharing/lib/DefaultPublicShareTemplateProvider.php @@ -149,10 +149,7 @@ public function renderPage(IShare $share, string $token, string $path): Template $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()); diff --git a/apps/files_sharing/tests/Controller/ShareControllerTest.php b/apps/files_sharing/tests/Controller/ShareControllerTest.php index 58cbb4e0b82c7..524336787cb3c 100644 --- a/apps/files_sharing/tests/Controller/ShareControllerTest.php +++ b/apps/files_sharing/tests/Controller/ShareControllerTest.php @@ -261,8 +261,12 @@ public function testShowShare(): void { ['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 @@ public function testShowShareWithPrivateName(): void { ['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); diff --git a/cypress/e2e/files_sharing/public-share/header-menu.cy.ts b/cypress/e2e/files_sharing/public-share/header-menu.cy.ts index a89ee8eb90e53..c127adc56c6f7 100644 --- a/cypress/e2e/files_sharing/public-share/header-menu.cy.ts +++ b/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') diff --git a/lib/base.php b/lib/base.php index 45058db160080..2aa833df3b544 100644 --- a/lib/base.php +++ b/lib/base.php @@ -550,10 +550,10 @@ private static function performSameSiteCookieProtection(IConfig $config): void { $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; }