From bee5e2c7ca637d034c6985c0328cef0ce068778e Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Tue, 31 Aug 2021 20:22:42 +0100 Subject: [PATCH] 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. --- .env.example.complete | 6 +++++ app/Config/app.php | 5 ++++ app/Config/dompdf.php | 34 +++++++++++++------------- app/Entities/Tools/ExportFormatter.php | 2 +- phpunit.xml | 1 + tests/Entity/ExportTest.php | 16 ++++++++++++ tests/Unit/ConfigTest.php | 6 +++++ 7 files changed, 52 insertions(+), 18 deletions(-) diff --git a/.env.example.complete b/.env.example.complete index 26df8f3cb..49d834ff7 100644 --- 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 index a09c329ce..6f1901065 100755 --- a/app/Config/app.php +++ b/app/Config/app.php @@ -36,6 +36,11 @@ return [ // 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 index 71ea716f3..cf07312e8 100644 --- a/app/Config/dompdf.php +++ b/app/Config/dompdf.php @@ -37,7 +37,7 @@ return [ * 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 @@ return [ * * 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 @@ return [ * 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 @@ return [ * 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 @@ return [ * 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 @@ return [ * @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 @@ return [ * 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 @@ return [ * * @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 @@ return [ * * @var string */ - 'DOMPDF_DEFAULT_FONT' => 'dejavu sans', + 'default_font' => 'dejavu sans', /** * Image DPI setting. @@ -194,7 +194,7 @@ return [ * * @var int */ - 'DOMPDF_DPI' => 96, + 'dpi' => 96, /** * Enable inline PHP. @@ -208,7 +208,7 @@ return [ * * @var bool */ - 'DOMPDF_ENABLE_PHP' => false, + 'enable_php' => false, /** * Enable inline Javascript. @@ -218,7 +218,7 @@ return [ * * @var bool */ - 'DOMPDF_ENABLE_JAVASCRIPT' => false, + 'enable_javascript' => false, /** * Enable remote file access. @@ -237,12 +237,12 @@ return [ * * @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 @@ return [ * * @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 index c299f9c71..05d0ff134 100644 --- a/app/Entities/Tools/ExportFormatter.php +++ b/app/Entities/Tools/ExportFormatter.php @@ -140,7 +140,7 @@ class ExportFormatter 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 index 75c89ec33..7e0da05d4 100644 --- a/phpunit.xml +++ b/phpunit.xml @@ -37,6 +37,7 @@ + diff --git a/tests/Entity/ExportTest.php b/tests/Entity/ExportTest.php index 7031c3875..aebc5f245 100644 --- a/tests/Entity/ExportTest.php +++ b/tests/Entity/ExportTest.php @@ -366,4 +366,20 @@ class ExportTest extends TestCase $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 index f45d20136..207fb7f59 100644 --- a/tests/Unit/ConfigTest.php +++ b/tests/Unit/ConfigTest.php @@ -76,6 +76,12 @@ class ConfigTest extends TestCase ); } + 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.