Clean up HTML response

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();

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 '<script nonce="' . $nonce. '" src="' . $scriptSrc . '"></script>'
-			.  $this->content;
+		return '<script nonce="' . $this->nonce . '" src="' . $this->scriptUrl . '"></script>' . $this->content;
 	}
 }

psalm.xml

@@ -26,6 +26,7 @@
 		<UndefinedClass>
 			<errorLevel type="suppress">
 				<referencedClass name="OC" />
+				<referencedClass name="OC\Security\CSP\ContentSecurityPolicyNonceManager" />
 			</errorLevel>
 		</UndefinedClass>
 		<UndefinedDocblockClass>
@@ -34,6 +35,7 @@
 				<referencedClass name="Doctrine\DBAL\Schema\SchemaException" />
 				<referencedClass name="Doctrine\DBAL\Driver\Statement" />
 				<referencedClass name="Doctrine\DBAL\Schema\Table" />
+				<referencedClass name="OC\Security\CSP\ContentSecurityPolicyNonceManager" />
 			</errorLevel>
 		</UndefinedDocblockClass>
 	</issueHandlers>

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);
 	}

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 = '/<script nonce=".+" src="' . $scriptSrcRegex . '"><\/script>'
-			. $contentRegex . '/';
-
-		$this->assertMatchesRegularExpression($responseRegex, $defaultResp->render());
+		$this->assertStringContainsString("<script nonce=\"$nonce\" src=\"$scriptUrl\"></script>", $richResp->render());
 		$this->assertEquals($content, $plainResp->render());
-		$this->assertMatchesRegularExpression($responseRegex, $richResp->render());
 	}
 
 	public function providesResponseData() {