From 16d9cc12af45aeff9973c3c99495f7a45cdaf0c3 Mon Sep 17 00:00:00 2001
From: Zero King <l2dy@icloud.com>
Date: Sat, 11 Nov 2023 15:45:21 +0800
Subject: [PATCH] Enforce viewable MIME types config on PDF documents

Summary:
Let instance admins decide whether to allow PDFs to be viewable as a Web page. See <https://github.com/mozilla-conduit/phabricator/commit/5ec132bf9ebfb90558f1b7f646772176629f86d0>.

    MOZILLA: Instead of always allowing PDFs to be viewable in the web UI, [...]
    This checks that the PDF mimetype is viewable according to the system
    configuration.

Ref Q83.

Test Plan:
1. Set `files.viewable-mime-types` to exclude application/pdf.
2. Upload a pdf file.
3. See "No document engine can render the contents of this file." in web UI.

Reviewers: O1 Blessed Committers, speck

Reviewed By: O1 Blessed Committers, speck

Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno

Differential Revision: https://we.phorge.it/D25464
---
 .../config/PhabricatorFilesConfigOptions.php     |  8 +++++---
 .../document/PhabricatorPDFDocumentEngine.php    | 16 +++++++++-------
 2 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/src/applications/files/config/PhabricatorFilesConfigOptions.php b/src/applications/files/config/PhabricatorFilesConfigOptions.php
index 7ed96a412b..67d123e2cb 100644
--- a/src/applications/files/config/PhabricatorFilesConfigOptions.php
+++ b/src/applications/files/config/PhabricatorFilesConfigOptions.php
@@ -134,9 +134,11 @@ final class PhabricatorFilesConfigOptions
         ->setDescription(
           pht(
             "Configure which uploaded file types may be viewed directly ".
-            "in the browser. Other file types will be downloaded instead ".
-            "of displayed. This is mainly a usability consideration, since ".
-            "browsers tend to freak out when viewing very large binary files.".
+            "in the browser. Other types will be downloaded instead of ".
+            "displayed. This is a usability and security consideration, ".
+            "since browsers tend to freak out when viewing very large ".
+            "binary files, and some types may be vulnerable to XSS attacks ".
+            "when viewed in a browser.".
             "\n\n".
             "The keys in this map are viewable MIME types; the values are ".
             "the MIME types they are delivered as when they are viewed in ".
diff --git a/src/applications/files/document/PhabricatorPDFDocumentEngine.php b/src/applications/files/document/PhabricatorPDFDocumentEngine.php
index 1e85bd4ae5..bc6433290a 100644
--- a/src/applications/files/document/PhabricatorPDFDocumentEngine.php
+++ b/src/applications/files/document/PhabricatorPDFDocumentEngine.php
@@ -14,14 +14,16 @@ final class PhabricatorPDFDocumentEngine
   }
 
   protected function canRenderDocumentType(PhabricatorDocumentRef $ref) {
-    // Since we just render a link to the document anyway, we don't need to
-    // check anything fancy in config to see if the MIME type is actually
-    // viewable.
+    $viewable_types = PhabricatorEnv::getEnvConfig('files.viewable-mime-types');
+    $viewable_types = array_keys($viewable_types);
 
-    return $ref->hasAnyMimeType(
-      array(
-        'application/pdf',
-      ));
+    $pdf_types = array(
+      'application/pdf',
+    );
+
+    return
+      $ref->hasAnyMimeType($viewable_types) &&
+      $ref->hasAnyMimeType($pdf_types);
   }
 
   protected function newDocumentContent(PhabricatorDocumentRef $ref) {