diff --git a/lib/Controller/MessagesController.php b/lib/Controller/MessagesController.php index 9c4dbb62cf..c366fcd343 100755 --- a/lib/Controller/MessagesController.php +++ b/lib/Controller/MessagesController.php @@ -31,6 +31,7 @@ namespace OCA\Mail\Controller; use Exception; +use OC\Security\CSP\ContentSecurityPolicyNonceManager; use OCA\Mail\Contracts\IMailManager; use OCA\Mail\Contracts\IMailSearch; use OCA\Mail\Exception\ClientException; @@ -87,16 +88,23 @@ class MessagesController extends Controller { /** @var IURLGenerator */ private $urlGenerator; + /** @var ContentSecurityPolicyNonceManager */ + private $nonceManager; + /** * @param string $appName * @param IRequest $request * @param AccountService $accountService + * @param IMailManager $mailManager + * @param IMailSearch $mailSearch + * @param ItineraryService $itineraryService * @param string $UserId * @param $userFolder * @param LoggerInterface $logger * @param IL10N $l10n * @param IMimeTypeDetector $mimeTypeDetector * @param IURLGenerator $urlGenerator + * @param ContentSecurityPolicyNonceManager $nonceManager */ public function __construct(string $appName, IRequest $request, @@ -109,7 +117,8 @@ public function __construct(string $appName, LoggerInterface $logger, IL10N $l10n, IMimeTypeDetector $mimeTypeDetector, - IURLGenerator $urlGenerator) { + IURLGenerator $urlGenerator, + ContentSecurityPolicyNonceManager $nonceManager) { parent::__construct($appName, $request); $this->accountService = $accountService; @@ -123,6 +132,7 @@ public function __construct(string $appName, $this->mimeTypeDetector = $mimeTypeDetector; $this->urlGenerator = $urlGenerator; $this->mailManager = $mailManager; + $this->nonceManager = $nonceManager; } /** @@ -357,17 +367,23 @@ public function getHtmlBody(int $id, bool $plain=false): Response { ); } - $htmlResponse = new HtmlResponse( - $this->mailManager->getImapMessage( - $account, - $mailbox, - $message->getUid(), - true - )->getHtmlBody( - $id - ), - $plain + $html = $this->mailManager->getImapMessage( + $account, + $mailbox, + $message->getUid(), + true + )->getHtmlBody( + $id ); + $htmlResponse = $plain ? + HtmlResponse::plain($html) : + HtmlResponse::withResizer( + $html, + $this->nonceManager->getNonce(), + $this->urlGenerator->getAbsoluteURL( + $this->urlGenerator->linkTo('mail', 'js/htmlresponse.js') + ) + ); // Harden the default security policy $policy = new ContentSecurityPolicy(); diff --git a/lib/Http/HtmlResponse.php b/lib/Http/HtmlResponse.php index 5918dc16a8..c3b45a03b2 100644 --- a/lib/Http/HtmlResponse.php +++ b/lib/Http/HtmlResponse.php @@ -25,7 +25,6 @@ namespace OCA\Mail\Http; -use OCP\Util; use OCP\AppFramework\Http\Response; class HtmlResponse extends Response { @@ -36,14 +35,42 @@ class HtmlResponse extends Response { /** @var bool */ private $plain; + /** @var string|null */ + private $nonce; + + /** @var string|null */ + private $scriptUrl; + /** * @param string $content message html content * @param bool $plain do not inject scripts if true (default=false) + * @param string|null $nonce + * @param string|null $scriptUrl */ - public function __construct(string $content, bool $plain=false) { + private function __construct(string $content, + bool $plain = false, + string $nonce = null, + string $scriptUrl = null) { parent::__construct(); $this->content = $content; $this->plain = $plain; + $this->nonce = $nonce; + $this->scriptUrl = $scriptUrl; + } + + public static function plain(string $content): self { + return new self($content, true); + } + + public static function withResizer(string $content, + string $nonce, + string $scriptUrl): self { + return new self( + $content, + false, + $nonce, + $scriptUrl, + ); } /** @@ -56,9 +83,6 @@ public function render(): string { return $this->content; } - $nonce = \OC::$server->getContentSecurityPolicyNonceManager()->getNonce(); - $scriptSrc = Util::linkToAbsolute('mail', 'js/htmlresponse.js'); - return '' - . $this->content; + return '' . $this->content; } } diff --git a/psalm.xml b/psalm.xml index 733e15d9dc..82b7d0add9 100644 --- a/psalm.xml +++ b/psalm.xml @@ -26,6 +26,7 @@ + @@ -34,6 +35,7 @@ + diff --git a/tests/Unit/Controller/MessagesControllerTest.php b/tests/Unit/Controller/MessagesControllerTest.php index 65fdd89e3e..c347dd7d51 100644 --- a/tests/Unit/Controller/MessagesControllerTest.php +++ b/tests/Unit/Controller/MessagesControllerTest.php @@ -26,6 +26,7 @@ use ChristophWurst\Nextcloud\Testing\TestCase; use OC\AppFramework\Http\Request; +use OC\Security\CSP\ContentSecurityPolicyNonceManager; use OCA\Mail\Account; use OCA\Mail\Attachment; use OCA\Mail\Contracts\IMailManager; @@ -106,6 +107,9 @@ class MessagesControllerTest extends TestCase { /** @var MockObject|IURLGenerator */ private $urlGenerator; + /** @var MockObject|ContentSecurityPolicyNonceManager */ + private $nonceManager; + /** @var ITimeFactory */ private $oldFactory; @@ -125,6 +129,7 @@ protected function setUp(): void { $this->l10n = $this->createMock(IL10N::class); $this->mimeTypeDetector = $this->createMock(IMimeTypeDetector::class); $this->urlGenerator = $this->createMock(IURLGenerator::class); + $this->nonceManager = $this->createMock(ContentSecurityPolicyNonceManager::class); $timeFactory = $this->createMocK(ITimeFactory::class); $timeFactory->expects($this->any()) @@ -147,7 +152,8 @@ protected function setUp(): void { $this->logger, $this->l10n, $this->mimeTypeDetector, - $this->urlGenerator + $this->urlGenerator, + $this->nonceManager, ); $this->account = $this->createMock(Account::class); @@ -177,50 +183,56 @@ public function testGetHtmlBody() { $message->setUid(123); $mailbox->setAccountId($accountId); $mailbox->setName($folderId); - $this->mailManager->expects($this->exactly(3)) + $this->mailManager->expects($this->exactly(2)) ->method('getMessage') ->with($this->userId, $messageId) ->willReturn($message); - $this->mailManager->expects($this->exactly(3)) + $this->mailManager->expects($this->exactly(2)) ->method('getMailbox') ->with($this->userId, $mailboxId) ->willReturn($mailbox); - $this->accountService->expects($this->exactly(3)) + $this->accountService->expects($this->exactly(2)) ->method('find') ->with($this->equalTo($this->userId), $this->equalTo($accountId)) ->will($this->returnValue($this->account)); $imapMessage = $this->createMock(IMAPMessage::class); - $this->mailManager->expects($this->exactly(3)) + $this->mailManager->expects($this->exactly(2)) ->method('getImapMessage') ->with($this->account, $mailbox, 123, true) ->willReturn($imapMessage); - $expectedDefaultResponse = new HtmlResponse(''); - $expectedDefaultResponse->cacheFor(3600); - - $expectedPlainResponse = new HtmlResponse('', true); + $expectedPlainResponse = HtmlResponse::plain(''); $expectedPlainResponse->cacheFor(3600); - $expectedRichResponse = new HtmlResponse('', false); + $nonce = "abc123"; + $relativeScriptUrl = "/script.js"; + $scriptUrl = "next.cloud/script.js"; + $this->nonceManager->expects($this->once()) + ->method('getNonce') + ->willReturn($nonce); + $this->urlGenerator->expects($this->once()) + ->method('linkTo') + ->with('mail', 'js/htmlresponse.js') + ->willReturn($relativeScriptUrl); + $this->urlGenerator->expects($this->once()) + ->method('getAbsoluteURL') + ->with($relativeScriptUrl) + ->willReturn($scriptUrl); + $expectedRichResponse = HtmlResponse::withResizer('', $nonce, $scriptUrl); $expectedRichResponse->cacheFor(3600); - if (class_exists('\OCP\AppFramework\Http\ContentSecurityPolicy')) { - $policy = new ContentSecurityPolicy(); - $policy->allowEvalScript(false); - $policy->disallowScriptDomain('\'self\''); - $policy->disallowConnectDomain('\'self\''); - $policy->disallowFontDomain('\'self\''); - $policy->disallowMediaDomain('\'self\''); - $expectedDefaultResponse->setContentSecurityPolicy($policy); - $expectedPlainResponse->setContentSecurityPolicy($policy); - $expectedRichResponse->setContentSecurityPolicy($policy); - } - - $actualDefaultResponse = $this->controller->getHtmlBody($messageId); + $policy = new ContentSecurityPolicy(); + $policy->allowEvalScript(false); + $policy->disallowScriptDomain('\'self\''); + $policy->disallowConnectDomain('\'self\''); + $policy->disallowFontDomain('\'self\''); + $policy->disallowMediaDomain('\'self\''); + $expectedPlainResponse->setContentSecurityPolicy($policy); + $expectedRichResponse->setContentSecurityPolicy($policy); + $actualPlainResponse = $this->controller->getHtmlBody($messageId, true); $actualRichResponse = $this->controller->getHtmlBody($messageId, false); - $this->assertEquals($expectedDefaultResponse, $actualDefaultResponse); $this->assertEquals($expectedPlainResponse, $actualPlainResponse); $this->assertEquals($expectedRichResponse, $actualRichResponse); } diff --git a/tests/Unit/Http/HtmlResponseTest.php b/tests/Unit/Http/HtmlResponseTest.php index be36f80889..dba2f53850 100644 --- a/tests/Unit/Http/HtmlResponseTest.php +++ b/tests/Unit/Http/HtmlResponseTest.php @@ -24,7 +24,6 @@ use ChristophWurst\Nextcloud\Testing\TestCase; use OCA\Mail\Http\HtmlResponse; -use OCP\Util; class HtmlResponseTest extends TestCase { @@ -35,18 +34,13 @@ class HtmlResponseTest extends TestCase { * @param $contentType */ public function testIt($content) { - $defaultResp = new HtmlResponse($content); - $plainResp = new HtmlResponse($content, true); - $richResp = new HtmlResponse($content, false); + $nonce = "abc123"; + $scriptUrl = "next.cloud/script.js"; + $plainResp = HtmlResponse::plain($content); + $richResp = HtmlResponse::withResizer($content, $nonce, $scriptUrl); - $scriptSrcRegex = preg_quote(Util::linkToAbsolute('mail', 'js/htmlresponse.js'), '/'); - $contentRegex = preg_quote($content, '/'); - $responseRegex = '/", $richResp->render()); $this->assertEquals($content, $plainResp->render()); - $this->assertMatchesRegularExpression($responseRegex, $richResp->render()); } public function providesResponseData() {