From edcbdb648ef26762f6480cd13ec037b4453eb32a Mon Sep 17 00:00:00 2001 From: Christoph Wurst Date: Thu, 4 May 2023 08:29:51 +0200 Subject: [PATCH 1/2] fix(imap): Only fetch mailbox STATUS once Signed-off-by: Christoph Wurst --- lib/Folder.php | 8 ++- lib/IMAP/FolderMapper.php | 52 ++++---------- lib/IMAP/MailboxSync.php | 1 - lib/Service/MailManager.php | 1 - tests/FolderTest.php | 2 +- tests/Unit/IMAP/FolderMapperTest.php | 93 +++++--------------------- tests/Unit/Service/MailManagerTest.php | 3 - 7 files changed, 37 insertions(+), 123 deletions(-) diff --git a/lib/Folder.php b/lib/Folder.php index 2d9a21bf9e..90e85e3061 100644 --- a/lib/Folder.php +++ b/lib/Folder.php @@ -44,12 +44,16 @@ class Folder { /** @var string[] */ private $specialUse; - public function __construct(int $accountId, Horde_Imap_Client_Mailbox $mailbox, array $attributes, ?string $delimiter) { + public function __construct(int $accountId, + Horde_Imap_Client_Mailbox $mailbox, + array $attributes, + ?string $delimiter, + array $status) { $this->accountId = $accountId; $this->mailbox = $mailbox; $this->attributes = $attributes; $this->delimiter = $delimiter; - $this->status = []; + $this->status = $status; $this->specialUse = []; } diff --git a/lib/IMAP/FolderMapper.php b/lib/IMAP/FolderMapper.php index f8326096d5..c18f985d0a 100644 --- a/lib/IMAP/FolderMapper.php +++ b/lib/IMAP/FolderMapper.php @@ -59,31 +59,21 @@ public function getFolders(Account $account, Horde_Imap_Client_Socket $client, 'delimiter' => true, 'attributes' => true, 'special_use' => true, + 'status' => Horde_Imap_Client::STATUS_ALL, ]); - return array_filter(array_map(function (array $mailbox) use ($account, $client) { + return array_filter(array_map(static function (array $mailbox) use ($account) { if (in_array($mailbox['mailbox']->utf8, self::DOVECOT_SIEVE_FOLDERS, true)) { // This is a special folder that must not be shown return null; } - try { - $client->status($mailbox["mailbox"]); - } catch (Horde_Imap_Client_Exception $e) { - // ignore folders which cause errors on access - // (i.e. server-side system I/O errors) - if (in_array($e->getCode(), [ - Horde_Imap_Client_Exception::UNSPECIFIED, - ], true)) { - return null; - } - } - return new Folder( $account->getId(), $mailbox['mailbox'], $mailbox['attributes'], - $mailbox['delimiter'] + $mailbox['delimiter'], + $mailbox['status'], ); }, $mailboxes)); } @@ -97,6 +87,7 @@ public function createFolder(Horde_Imap_Client_Socket $client, 'delimiter' => true, 'attributes' => true, 'special_use' => true, + 'status' => Horde_Imap_Client::STATUS_ALL, ]); $mb = reset($list); @@ -104,32 +95,13 @@ public function createFolder(Horde_Imap_Client_Socket $client, throw new ServiceException("Created mailbox does not exist"); } - return new Folder($account->getId(), $mb['mailbox'], $mb['attributes'], $mb['delimiter']); - } - - /** - * @param Folder[] $folders - * @param Horde_Imap_Client_Socket $client - * - * @throws Horde_Imap_Client_Exception - * - * @return void - */ - public function getFoldersStatus(array $folders, - Horde_Imap_Client_Socket $client): void { - $mailboxes = array_map(function (Folder $folder) { - return $folder->getMailbox(); - }, array_filter($folders, function (Folder $folder) { - return !in_array('\noselect', $folder->getAttributes()); - })); - - $status = $client->status($mailboxes); - - foreach ($folders as $folder) { - if (isset($status[$folder->getMailbox()])) { - $folder->setStatus($status[$folder->getMailbox()]); - } - } + return new Folder( + $account->getId(), + $mb['mailbox'], + $mb['attributes'], + $mb['delimiter'], + $mb['status'], + ); } /** diff --git a/lib/IMAP/MailboxSync.php b/lib/IMAP/MailboxSync.php index 4da58c3d9a..e3cba7569e 100644 --- a/lib/IMAP/MailboxSync.php +++ b/lib/IMAP/MailboxSync.php @@ -102,7 +102,6 @@ public function sync(Account $account, try { $folders = $this->folderMapper->getFolders($account, $client); - $this->folderMapper->getFoldersStatus($folders, $client); } catch (Horde_Imap_Client_Exception $e) { throw new ServiceException( sprintf("IMAP error synchronizing account %d: %s", $account->getId(), $e->getMessage()), diff --git a/lib/Service/MailManager.php b/lib/Service/MailManager.php index 8f3af974a1..dcce23d4e0 100644 --- a/lib/Service/MailManager.php +++ b/lib/Service/MailManager.php @@ -153,7 +153,6 @@ public function createMailbox(Account $account, string $name): Mailbox { $client = $this->imapClientFactory->getClient($account); try { $folder = $this->folderMapper->createFolder($client, $account, $name); - $this->folderMapper->getFoldersStatus([$folder], $client); } catch (Horde_Imap_Client_Exception $e) { throw new ServiceException( "Could not get mailbox status: " . diff --git a/tests/FolderTest.php b/tests/FolderTest.php index 31256fafb8..e62ef35acf 100644 --- a/tests/FolderTest.php +++ b/tests/FolderTest.php @@ -40,7 +40,7 @@ private function mockFolder(array $attributes = [], $delimiter = '.') { $this->accountId = 15; $this->mailbox = $this->createMock(Horde_Imap_Client_Mailbox::class); - $this->folder = new Folder($this->accountId, $this->mailbox, $attributes, $delimiter); + $this->folder = new Folder($this->accountId, $this->mailbox, $attributes, $delimiter, []); } public function testGetMailbox() { diff --git a/tests/Unit/IMAP/FolderMapperTest.php b/tests/Unit/IMAP/FolderMapperTest.php index acaff84db6..6cd8ebfb8f 100644 --- a/tests/Unit/IMAP/FolderMapperTest.php +++ b/tests/Unit/IMAP/FolderMapperTest.php @@ -42,7 +42,7 @@ protected function setUp(): void { $this->mapper = new FolderMapper(); } - public function testGetFoldersEmtpyAccount() { + public function testGetFoldersEmtpyAccount(): void { $account = $this->createMock(Account::class); $client = $this->createMock(Horde_Imap_Client_Socket::class); $client->expects($this->once()) @@ -52,6 +52,7 @@ public function testGetFoldersEmtpyAccount() { 'delimiter' => true, 'attributes' => true, 'special_use' => true, + 'status' => Horde_Imap_Client::STATUS_ALL, ])) ->willReturn([]); @@ -60,7 +61,7 @@ public function testGetFoldersEmtpyAccount() { $this->assertEquals([], $folders); } - public function testGetFolders() { + public function testGetFolders(): void { $account = $this->createMock(Account::class); $account->method('getId')->willReturn(27); $client = $this->createMock(Horde_Imap_Client_Socket::class); @@ -71,12 +72,16 @@ public function testGetFolders() { 'delimiter' => true, 'attributes' => true, 'special_use' => true, + 'status' => Horde_Imap_Client::STATUS_ALL, ])) ->willReturn([ [ 'mailbox' => new Horde_Imap_Client_Mailbox('INBOX'), 'attributes' => [], 'delimiter' => '.', + 'status' => [ + 'unseen' => 0, + ], ], [ 'mailbox' => new Horde_Imap_Client_Mailbox('Sent'), @@ -84,11 +89,14 @@ public function testGetFolders() { '\sent', ], 'delimiter' => '.', + 'status' => [ + 'unseen' => 1, + ], ], ]); $expected = [ - new Folder(27, new Horde_Imap_Client_Mailbox('INBOX'), [], '.'), - new Folder(27, new Horde_Imap_Client_Mailbox('Sent'), ['\sent'], '.'), + new Folder(27, new Horde_Imap_Client_Mailbox('INBOX'), [], '.', ['unseen' => 0]), + new Folder(27, new Horde_Imap_Client_Mailbox('Sent'), ['\sent'], '.', ['unseen' => 1]), ]; $folders = $this->mapper->getFolders($account, $client); @@ -96,7 +104,7 @@ public function testGetFolders() { $this->assertEquals($expected, $folders); } - public function testCreateFolder() { + public function testCreateFolder(): void { $account = $this->createMock(Account::class); $account->method('getId')->willReturn(42); $client = $this->createMock(Horde_Imap_Client_Socket::class); @@ -110,90 +118,25 @@ public function testCreateFolder() { 'delimiter' => true, 'attributes' => true, 'special_use' => true, + 'status' => Horde_Imap_Client::STATUS_ALL, ])) ->willReturn([ [ 'mailbox' => new Horde_Imap_Client_Mailbox('new'), 'attributes' => [], 'delimiter' => '.', + 'status' => [ + 'unseen' => 0, + ], ], ]); $created = $this->mapper->createFolder($client, $account, 'new'); - $expected = new Folder(42, new Horde_Imap_Client_Mailbox('new'), [], '.'); + $expected = new Folder(42, new Horde_Imap_Client_Mailbox('new'), [], '.', ['unseen' => 0]); $this->assertEquals($expected, $created); } - public function testGetFoldersStatus() { - $folders = [ - $this->createMock(Folder::class), - ]; - $client = $this->createMock(Horde_Imap_Client_Socket::class); - $folders[0]->expects($this->any()) - ->method('getMailbox') - ->willReturn('folder1'); - $folders[0]->expects($this->once()) - ->method('getAttributes') - ->willReturn([]); - $client->expects($this->once()) - ->method('status') - ->with($this->equalTo(['folder1'])) - ->willReturn([ - 'folder1' => [ - 'total' => 123 - ], - ]); - $folders[0]->expects($this->once()) - ->method('setStatus'); - - $this->mapper->getFoldersStatus($folders, $client); - } - - public function testGetFoldersStatusNoStatusReported() { - $folders = [ - $this->createMock(Folder::class), - ]; - $client = $this->createMock(Horde_Imap_Client_Socket::class); - $folders[0]->expects($this->any()) - ->method('getMailbox') - ->willReturn('folder1'); - $folders[0]->expects($this->once()) - ->method('getAttributes') - ->willReturn([]); - $client->expects($this->once()) - ->method('status') - ->with($this->equalTo(['folder1'])) - ->willReturn([ - // Nothing reported for this folder - ]); - $folders[0]->expects($this->never()) - ->method('setStatus'); - - $this->mapper->getFoldersStatus($folders, $client); - } - - public function testGetFoldersStatusNotSearchable() { - $folders = [ - $this->createMock(Folder::class), - ]; - $client = $this->createMock(Horde_Imap_Client_Socket::class); - $folders[0]->expects($this->any()) - ->method('getMailbox') - ->willReturn('folder1'); - $folders[0]->expects($this->once()) - ->method('getAttributes') - ->willReturn(['\\noselect']); - $client->expects($this->once()) - ->method('status') - ->with($this->equalTo([])) - ->willReturn([]); - $folders[0]->expects($this->never()) - ->method('setStatus'); - - $this->mapper->getFoldersStatus($folders, $client); - } - public function testGetFoldersStatusAsObject() { $client = $this->createMock(Horde_Imap_Client_Socket::class); $client->expects($this->once()) diff --git a/tests/Unit/Service/MailManagerTest.php b/tests/Unit/Service/MailManagerTest.php index f3f552a89e..41d912a6c8 100644 --- a/tests/Unit/Service/MailManagerTest.php +++ b/tests/Unit/Service/MailManagerTest.php @@ -141,9 +141,6 @@ public function testCreateFolder() { ->method('createFolder') ->with($this->equalTo($client), $this->equalTo($account), $this->equalTo('new')) ->willReturn($folder); - $this->folderMapper->expects($this->once()) - ->method('getFoldersStatus') - ->with($this->equalTo([$folder])); $this->folderMapper->expects($this->once()) ->method('detectFolderSpecialUse') ->with($this->equalTo([$folder])); From dc7d08581a6c850a4e8eee80dcccc71b9a305c1d Mon Sep 17 00:00:00 2001 From: Christoph Wurst Date: Mon, 8 May 2023 19:26:28 +0200 Subject: [PATCH 2/2] fix: Check strict cookies for image proxy Signed-off-by: Christoph Wurst --- lib/Controller/ProxyController.php | 7 ++++ tests/Unit/Controller/ProxyControllerTest.php | 36 +++++++++++++++++-- 2 files changed, 41 insertions(+), 2 deletions(-) diff --git a/lib/Controller/ProxyController.php b/lib/Controller/ProxyController.php index d3e313fd2d..cf17ed9569 100644 --- a/lib/Controller/ProxyController.php +++ b/lib/Controller/ProxyController.php @@ -36,6 +36,7 @@ use OCP\IURLGenerator; use Psr\Http\Client\ClientExceptionInterface; use Psr\Log\LoggerInterface; +use function file_get_contents; class ProxyController extends Controller { private IURLGenerator $urlGenerator; @@ -105,6 +106,12 @@ public function proxy(string $src): ProxyDownloadResponse { // close the session to allow parallel downloads $this->session->close(); + // If strict cookies are set it means we come from the same domain so no open redirect + if (!$this->request->passesStrictCookieCheck()) { + $content = file_get_contents(__DIR__ . '/../../img/blocked-image.png'); + return new ProxyDownloadResponse($content, $src, 'application/octet-stream'); + } + $client = $this->clientService->newClient(); try { $response = $client->get($src); diff --git a/tests/Unit/Controller/ProxyControllerTest.php b/tests/Unit/Controller/ProxyControllerTest.php index c8774854c6..8fd2b824b9 100644 --- a/tests/Unit/Controller/ProxyControllerTest.php +++ b/tests/Unit/Controller/ProxyControllerTest.php @@ -1,5 +1,7 @@ * @@ -156,11 +158,41 @@ public function testRedirectInvalidUrl() { $this->controller->redirect('ftps://example.com'); } - public function testProxy() { + public function testProxyWithoutCookies(): void { $src = 'http://example.com'; - $httpResponse = $this->createMock(IResponse::class); $content = '🐵🐵🐵'; + $this->session->expects($this->once()) + ->method('close'); + $client = $this->getMockBuilder(IClient::class)->getMock(); + $this->clientService->expects(self::never()) + ->method('newClient') + ->willReturn($client); + $unexpected = new ProxyDownloadResponse( + $content, + $src, + 'application/octet-stream' + ); + $this->controller = new ProxyController( + $this->appName, + $this->request, + $this->urlGenerator, + $this->session, + $this->clientService, + $this->logger + ); + + $response = $this->controller->proxy($src); + $this->assertNotEquals($unexpected, $response); + } + + public function testProxy(): void { + $src = 'http://example.com'; + $httpResponse = $this->createMock(IResponse::class); + $content = '🐵🐵🐵'; + $this->request->expects(self::once()) + ->method('passesStrictCookieCheck') + ->willReturn(true); $this->session->expects($this->once()) ->method('close'); $client = $this->getMockBuilder(IClient::class)->getMock();