From cf593c538db7946378badfca4593dbd647d27203 Mon Sep 17 00:00:00 2001 From: Abhinav Ohri Date: Sat, 19 Sep 2026 15:58:23 +0530 Subject: [PATCH] fix(delegation): load proxied images in shared mailboxes The image proxy checked message ownership against the viewing user, so delegates received 400 responses for remote images. Resolve access to the message first, then use the effective account owner for the existing lookup. Assisted-by: Codex:gpt-5 Signed-off-by: Abhinav Ohri --- lib/Controller/ProxyController.php | 8 ++- tests/Unit/Controller/ProxyControllerTest.php | 68 +++++++++++++++++++ 2 files changed, 74 insertions(+), 2 deletions(-) diff --git a/lib/Controller/ProxyController.php b/lib/Controller/ProxyController.php index 3aa9a35f0f..c7a6f23af7 100644 --- a/lib/Controller/ProxyController.php +++ b/lib/Controller/ProxyController.php @@ -10,8 +10,10 @@ namespace OCA\Mail\Controller; +use OCA\Mail\Exception\ClientException; use OCA\Mail\Html\ProxyHmacGenerator; use OCA\Mail\Http\ProxyDownloadResponse; +use OCA\Mail\Service\DelegationService; use OCA\Mail\Service\MailManager; use OCP\AppFramework\Controller; use OCP\AppFramework\Db\DoesNotExistException; @@ -44,6 +46,7 @@ public function __construct( private ProxyHmacGenerator $hmacGenerator, private LoggerInterface $logger, private MailManager $mailManager, + private DelegationService $delegationService, private ?string $userId, ) { parent::__construct($appName, $request); @@ -82,8 +85,9 @@ public function proxy(string $src, ?int $id, ?string $hmac): Response { return new Response(Http::STATUS_BAD_REQUEST); } try { - $this->mailManager->getMessage($this->userId, $id); - } catch (DoesNotExistException $e) { + $effectiveUserId = $this->delegationService->resolveMessageUserId($id, $this->userId); + $this->mailManager->getMessage($effectiveUserId, $id); + } catch (DoesNotExistException|ClientException $e) { return new Response(Http::STATUS_BAD_REQUEST); } if (!hash_equals($this->hmacGenerator->generate($id, $src), $hmac)) { diff --git a/tests/Unit/Controller/ProxyControllerTest.php b/tests/Unit/Controller/ProxyControllerTest.php index 51b5c41dd1..6e3d285c74 100644 --- a/tests/Unit/Controller/ProxyControllerTest.php +++ b/tests/Unit/Controller/ProxyControllerTest.php @@ -14,6 +14,7 @@ use OCA\Mail\Controller\ProxyController; use OCA\Mail\Html\ProxyHmacGenerator; use OCA\Mail\Http\ProxyDownloadResponse; +use OCA\Mail\Service\DelegationService; use OCA\Mail\Service\MailManager; use OCP\AppFramework\Db\DoesNotExistException; use OCP\AppFramework\Http; @@ -53,6 +54,8 @@ class ProxyControllerTest extends TestCase { /** @var MailManager|MockObject */ private $mailManager; + private DelegationService&MockObject $delegationService; + private string $userId = 'user'; /** @var ProxyController */ @@ -68,6 +71,7 @@ protected function setUp(): void { $this->clientService = $this->createMock(IClientService::class); $this->hmacGenerator = $this->createMock(ProxyHmacGenerator::class); $this->mailManager = $this->createMock(MailManager::class); + $this->delegationService = $this->createMock(DelegationService::class); $this->logger = new NullLogger(); } @@ -89,6 +93,7 @@ public function testProxyWithoutCookies(): void { $this->hmacGenerator, $this->logger, $this->mailManager, + $this->delegationService, $this->userId, ); @@ -112,6 +117,10 @@ public function testProxy(): void { ->method('generate') ->with($id, $src) ->willReturn($validHmac); + $this->delegationService->expects($this->once()) + ->method('resolveMessageUserId') + ->with($id, $this->userId) + ->willReturn($this->userId); $this->mailManager->expects($this->once()) ->method('getMessage') ->with($this->userId, $id); @@ -135,6 +144,54 @@ public function testProxy(): void { $this->hmacGenerator, $this->logger, $this->mailManager, + $this->delegationService, + $this->userId, + ); + + $response = $this->controller->proxy($src, $id, $validHmac); + + $this->assertInstanceOf(ProxyDownloadResponse::class, $response); + } + + public function testProxyForDelegatedMessage(): void { + $src = 'http://example.com'; + $id = 1; + $validHmac = 'valid-hmac-hash'; + $content = 'mock image data'; + $ownerUserId = 'owner'; + $httpResponse = $this->createMock(IResponse::class); + $this->request->method('passesStrictCookieCheck')->willReturn(true); + $this->hmacGenerator->method('generate') + ->with($id, $src) + ->willReturn($validHmac); + $this->delegationService->expects($this->once()) + ->method('resolveMessageUserId') + ->with($id, $this->userId) + ->willReturn($ownerUserId); + $this->mailManager->expects($this->once()) + ->method('getMessage') + ->with($ownerUserId, $id); + $client = $this->getMockBuilder(IClient::class)->getMock(); + $this->clientService->expects($this->once()) + ->method('newClient') + ->willReturn($client); + $client->expects($this->once()) + ->method('get') + ->with($src) + ->willReturn($httpResponse); + $httpResponse->expects($this->once()) + ->method('getBody') + ->willReturn($content); + $this->controller = new ProxyController( + $this->appName, + $this->request, + $this->urlGenerator, + $this->session, + $this->clientService, + $this->hmacGenerator, + $this->logger, + $this->mailManager, + $this->delegationService, $this->userId, ); @@ -157,6 +214,10 @@ public function testProxyWithInvalidHmac(): void { ->method('generate') ->with($id, $src) ->willReturn($expectedHmac); + $this->delegationService->expects($this->once()) + ->method('resolveMessageUserId') + ->with($id, $this->userId) + ->willReturn($this->userId); $this->mailManager->expects($this->once()) ->method('getMessage') ->with($this->userId, $id); @@ -171,6 +232,7 @@ public function testProxyWithInvalidHmac(): void { $this->hmacGenerator, $this->logger, $this->mailManager, + $this->delegationService, $this->userId, ); @@ -198,6 +260,7 @@ public function testProxyWithMissingHmacParameters(): void { $this->hmacGenerator, $this->logger, $this->mailManager, + $this->delegationService, $this->userId, ); @@ -215,6 +278,10 @@ public function testProxyWithMessageNotOwnedByUser(): void { ->willReturn(true); $this->session->expects($this->once()) ->method('close'); + $this->delegationService->expects($this->once()) + ->method('resolveMessageUserId') + ->with($id, $this->userId) + ->willReturn($this->userId); $this->mailManager->expects($this->once()) ->method('getMessage') ->with($this->userId, $id) @@ -230,6 +297,7 @@ public function testProxyWithMessageNotOwnedByUser(): void { $this->hmacGenerator, $this->logger, $this->mailManager, + $this->delegationService, $this->userId, );