From 7a0261878ad9d392dc5bb36c7507138d303c35cd Mon Sep 17 00:00:00 2001 From: Louis Chemineau Date: Wed, 14 May 2025 11:36:09 +0200 Subject: [PATCH 1/2] 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 --- apps/dav/appinfo/v2/publicremote.php | 4 ++- apps/dav/lib/Connector/Sabre/PublicAuth.php | 35 +++++++++++++------ .../unit/Connector/Sabre/PublicAuthTest.php | 34 +++++++++--------- lib/base.php | 8 ++--- 4 files changed, 48 insertions(+), 33 deletions(-) diff --git a/apps/dav/appinfo/v2/publicremote.php b/apps/dav/appinfo/v2/publicremote.php index 27b8716f6ae97..e32f7efb37445 100644 --- a/apps/dav/appinfo/v2/publicremote.php +++ b/apps/dav/appinfo/v2/publicremote.php @@ -26,6 +26,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; @@ -53,7 +54,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 ea59d9efc8f93..0bd267cbd87ce 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\Share\Exceptions\ShareNotFound; use OCP\Share\IManager; @@ -22,6 +23,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; @@ -44,6 +46,7 @@ public function __construct( private ISession $session, private IThrottler $throttler, private LoggerInterface $logger, + private IURLGenerator $urlGenerator, ) { // setup realm $defaults = new Defaults(); @@ -51,10 +54,6 @@ public function __construct( } /** - * @param RequestInterface $request - * @param ResponseInterface $response - * - * @return array * @throws NotAuthenticated * @throws ServiceUnavailable */ @@ -62,6 +61,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, @@ -77,6 +80,15 @@ public function check(RequestInterface $request, ResponseInterface $response): a return $this->checkToken(); } catch (NotAuthenticated $e) { 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(); @@ -87,7 +99,6 @@ public function check(RequestInterface $request, ResponseInterface $response): a /** * Extract token from request url - * @return string * @throws NotFound */ private function getToken(): string { @@ -104,7 +115,7 @@ private function getToken(): string { /** * Check token validity - * @return array + * * @throws NotFound * @throws NotAuthenticated */ @@ -152,15 +163,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 @@ -203,7 +212,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/lib/base.php b/lib/base.php index bfc1c8b2da1ae..f8a90bd63f269 100644 --- a/lib/base.php +++ b/lib/base.php @@ -548,10 +548,10 @@ private static function performSameSiteCookieProtection(\OCP\IConfig $config): v $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; } From 617af2f3cc78907e274abfa27033b0293e71cdec Mon Sep 17 00:00:00 2001 From: Louis Chemineau Date: Wed, 21 May 2025 16:01:54 +0200 Subject: [PATCH 2/2] fix: Replace the deprecated direct download link with the public DAV endpoint Follow-up of #48098 Signed-off-by: Louis Chemineau --- .../lib/DefaultPublicShareTemplateProvider.php | 5 +---- .../tests/Controller/ShareControllerTest.php | 16 ++++++++++++---- .../files_sharing/public-share/header-menu.cy.ts | 4 ++-- 3 files changed, 15 insertions(+), 10 deletions(-) diff --git a/apps/files_sharing/lib/DefaultPublicShareTemplateProvider.php b/apps/files_sharing/lib/DefaultPublicShareTemplateProvider.php index 71180acc4f15d..8212785382fa9 100644 --- a/apps/files_sharing/lib/DefaultPublicShareTemplateProvider.php +++ b/apps/files_sharing/lib/DefaultPublicShareTemplateProvider.php @@ -150,10 +150,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 b13c7a7b6c5a4..ffd2df98df341 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')