Added untrusted server fetching control

WKHTMLtoPDF provides limited control for external fetching so that will now be disabled by default unless ALLOW_UNTRUSTED_SERVER_FETCHING=true is specifically set. This new option will also control DOMPDF fetching.
BookStackApp · Aug 31, 2021 · bee5e2c · bee5e2c
1 parent 8f12c8b
commit bee5e2c
Show file tree

Hide file tree

Showing 7 changed files with 52 additions and 18 deletions.
diff --git a/.env.example.complete b/.env.example.complete
@@ -281,6 +281,12 @@ ALLOW_CONTENT_SCRIPTS=false
 # Contents of the robots.txt file can be overridden, making this option obsolete.
 ALLOW_ROBOTS=null
 
+# Allow server-side fetches to be performed to potentially unknown
+# and user-provided locations. Primarily used in exports when loading
+# in externally referenced assets.
+# Can be 'true' or 'false'.
+ALLOW_UNTRUSTED_SERVER_FETCHING=false
+
 # A list of hosts that BookStack can be iframed within.
 # Space separated if multiple. BookStack host domain is auto-inferred.
 # For Example: ALLOWED_IFRAME_HOSTS="https://example.com https://a.example.com"

diff --git a/app/Config/app.php b/app/Config/app.php
@@ -36,6 +36,11 @@
     // Even when overridden the WYSIWYG editor may still escape script content.
     'allow_content_scripts' => env('ALLOW_CONTENT_SCRIPTS', false),
 
+    # Allow server-side fetches to be performed to potentially unknown
+    # and user-provided locations. Primarily used in exports when loading
+    # in externally referenced assets.
+    'allow_untrusted_server_fetching' => env('ALLOW_UNTRUSTED_SERVER_FETCHING', false),
+
     // Override the default behaviour for allowing crawlers to crawl the instance.
     // May be ignored if view has be overridden or modified.
     // Defaults to null since, if not set, 'app-public' status used instead.

diff --git a/app/Config/dompdf.php b/app/Config/dompdf.php
@@ -37,7 +37,7 @@
          * Times-Roman, Times-Bold, Times-BoldItalic, Times-Italic,
          * Symbol, ZapfDingbats.
          */
-        'DOMPDF_FONT_DIR' => storage_path('fonts/'),  // advised by dompdf (https://github.com/dompdf/dompdf/pull/782)
+        'font_dir' => storage_path('fonts/'),  // advised by dompdf (https://github.com/dompdf/dompdf/pull/782)
 
         /**
          * The location of the DOMPDF font cache directory.
@@ -47,7 +47,7 @@
          *
          * Note: This directory must exist and be writable by the webserver process.
          */
-        'DOMPDF_FONT_CACHE' => storage_path('fonts/'),
+        'font_cache' => storage_path('fonts/'),
 
         /**
          * The location of a temporary directory.
@@ -56,7 +56,7 @@
          * The temporary directory is required to download remote images and when
          * using the PFDLib back end.
          */
-        'DOMPDF_TEMP_DIR' => sys_get_temp_dir(),
+        'temp_dir' => sys_get_temp_dir(),
 
         /**
          * ==== IMPORTANT ====.
@@ -70,7 +70,7 @@
          * direct class use like:
          * $dompdf = new DOMPDF();  $dompdf->load_html($htmldata); $dompdf->render(); $pdfdata = $dompdf->output();
          */
-        'DOMPDF_CHROOT' => realpath(base_path()),
+        'chroot' => realpath(base_path()),
 
         /**
          * Whether to use Unicode fonts or not.
@@ -81,12 +81,12 @@
          * When enabled, dompdf can support all Unicode glyphs. Any glyphs used in a
          * document must be present in your fonts, however.
          */
-        'DOMPDF_UNICODE_ENABLED' => true,
+        'unicode_enabled' => true,
 
         /**
          * Whether to enable font subsetting or not.
          */
-        'DOMPDF_ENABLE_FONTSUBSETTING' => false,
+        'enable_fontsubsetting' => false,
 
         /**
          * The PDF rendering backend to use.
@@ -115,7 +115,7 @@
          * @link http://www.ros.co.nz/pdf
          * @link http://www.php.net/image
          */
-        'DOMPDF_PDF_BACKEND' => 'CPDF',
+        'pdf_backend' => 'CPDF',
 
         /**
          * PDFlib license key.
@@ -141,7 +141,7 @@
          * the desired content might be different (e.g. screen or projection view of html file).
          * Therefore allow specification of content here.
          */
-        'DOMPDF_DEFAULT_MEDIA_TYPE' => 'print',
+        'default_media_type' => 'print',
 
         /**
          * The default paper size.
@@ -150,7 +150,7 @@
          *
          * @see CPDF_Adapter::PAPER_SIZES for valid sizes ('letter', 'legal', 'A4', etc.)
          */
-        'DOMPDF_DEFAULT_PAPER_SIZE' => 'a4',
+        'default_paper_size' => 'a4',
 
         /**
          * The default font family.
@@ -159,7 +159,7 @@
          *
          * @var string
          */
-        'DOMPDF_DEFAULT_FONT' => 'dejavu sans',
+        'default_font' => 'dejavu sans',
 
         /**
          * Image DPI setting.
@@ -194,7 +194,7 @@
          *
          * @var int
          */
-        'DOMPDF_DPI' => 96,
+        'dpi' => 96,
 
         /**
          * Enable inline PHP.
@@ -208,7 +208,7 @@
          *
          * @var bool
          */
-        'DOMPDF_ENABLE_PHP' => false,
+        'enable_php' => false,
 
         /**
          * Enable inline Javascript.
@@ -218,7 +218,7 @@
          *
          * @var bool
          */
-        'DOMPDF_ENABLE_JAVASCRIPT' => false,
+        'enable_javascript' => false,
 
         /**
          * Enable remote file access.
@@ -237,12 +237,12 @@
          *
          * @var bool
          */
-        'DOMPDF_ENABLE_REMOTE' => true,
+        'enable_remote' => env('ALLOW_UNTRUSTED_SERVER_FETCHING', false),
 
         /**
          * A ratio applied to the fonts height to be more like browsers' line height.
          */
-        'DOMPDF_FONT_HEIGHT_RATIO' => 1.1,
+        'font_height_ratio' => 1.1,
 
         /**
          * Enable CSS float.
@@ -251,12 +251,12 @@
          *
          * @var bool
          */
-        'DOMPDF_ENABLE_CSS_FLOAT' => true,
+        'enable_css_float' => true,
 
         /**
          * Use the more-than-experimental HTML5 Lib parser.
          */
-        'DOMPDF_ENABLE_HTML5PARSER' => true,
+        'enable_html5parser' => true,
 
     ],
 

diff --git a/app/Entities/Tools/ExportFormatter.php b/app/Entities/Tools/ExportFormatter.php
@@ -140,7 +140,7 @@ public function bookToPdf(Book $book)
     protected function htmlToPdf(string $html): string
     {
         $containedHtml = $this->containHtml($html);
-        $useWKHTML = config('snappy.pdf.binary') !== false;
+        $useWKHTML = config('snappy.pdf.binary') !== false && config('app.allow_untrusted_server_fetching') === true;
         if ($useWKHTML) {
             $pdf = SnappyPDF::loadHTML($containedHtml);
             $pdf->setOption('print-media-type', true);

diff --git a/phpunit.xml b/phpunit.xml
@@ -37,6 +37,7 @@
     <server name="LOG_CHANNEL" value="single"/>
     <server name="AUTH_METHOD" value="standard"/>
     <server name="DISABLE_EXTERNAL_SERVICES" value="true"/>
+    <server name="ALLOW_UNTRUSTED_SERVER_FETCHING" value="false"/>
     <server name="AVATAR_URL" value=""/>
     <server name="LDAP_START_TLS" value="false"/>
     <server name="LDAP_VERSION" value="3"/>

diff --git a/tests/Entity/ExportTest.php b/tests/Entity/ExportTest.php
@@ -366,4 +366,20 @@ public function test_export_option_only_visible_and_accessible_with_permission()
             $this->assertPermissionError($resp);
         }
     }
+
+    public function test_wkhtmltopdf_only_used_when_allow_untrusted_is_true()
+    {
+        /** @var Page $page */
+        $page = Page::query()->first();
+
+        config()->set('snappy.pdf.binary', '/abc123');
+        config()->set('app.allow_untrusted_server_fetching', false);
+
+        $resp = $this->asEditor()->get($page->getUrl('/export/pdf'));
+        $resp->assertStatus(200); // Sucessful response with invalid snappy binary indicates dompdf usage.
+
+        config()->set('app.allow_untrusted_server_fetching', true);
+        $resp = $this->get($page->getUrl('/export/pdf'));
+        $resp->assertStatus(500); // Bad response indicates wkhtml usage
+    }
 }
diff --git a/tests/Unit/ConfigTest.php b/tests/Unit/ConfigTest.php
@@ -76,6 +76,12 @@ public function test_saml2_idp_authn_context_string_parsed_as_space_separated_ar
         );
     }
 
+    public function test_dompdf_remote_fetching_controlled_by_allow_untrusted_server_fetching_false()
+    {
+        $this->checkEnvConfigResult('ALLOW_UNTRUSTED_SERVER_FETCHING', 'false', 'dompdf.defines.enable_remote', false);
+        $this->checkEnvConfigResult('ALLOW_UNTRUSTED_SERVER_FETCHING', 'true', 'dompdf.defines.enable_remote', true);
+    }
+
     /**
      * Set an environment variable of the given name and value
      * then check the given config key to see if it matches the given result.