Updated version and assets for release v0.30.4

This filters out potentially malicious javascript: or data: uri's coming
through to be attached to attachments.
Added tests to cover.

Thanks to Yassine ABOUKIR (@yassineaboukir on twitter) for reporting this
vulnerability.

.github/translators.txt

@@ -122,3 +122,4 @@ fadiapp :: Arabic
 Jakub Bouček (jakubboucek) :: Czech
 Marco (cdrfun) :: German
 10935336 :: Chinese Simplified
+孟繁阳 (FanyangMeng) :: Chinese Simplified

app/Entities/Managers/PageContent.php

@@ -296,6 +296,24 @@ class PageContent
             $scriptElem->parentNode->removeChild($scriptElem);
         }
 
+        // Remove clickable links to JavaScript URI
+        $badLinks = $xPath->query('//*[contains(@href, \'javascript:\')]');
+        foreach ($badLinks as $badLink) {
+            $badLink->parentNode->removeChild($badLink);
+        }
+
+        // Remove forms with calls to JavaScript URI
+        $badForms = $xPath->query('//*[contains(@action, \'javascript:\')] | //*[contains(@formaction, \'javascript:\')]');
+        foreach ($badForms as $badForm) {
+            $badForm->parentNode->removeChild($badForm);
+        }
+
+        // Remove meta tag to prevent external redirects
+        $metaTags = $xPath->query('//meta[contains(@content, \'url\')]');
+        foreach ($metaTags as $metaTag) {
+            $metaTag->parentNode->removeChild($metaTag);
+        }
+
         // Remove data or JavaScript iFrames
         $badIframes = $xPath->query('//*[contains(@src, \'data:\')] | //*[contains(@src, \'javascript:\')] | //*[@srcdoc]');
         foreach ($badIframes as $badIframe) {

app/Http/Controllers/AttachmentController.php

@@ -110,7 +110,7 @@ class AttachmentController extends Controller
         try {
             $this->validate($request, [
                 'attachment_edit_name' => 'required|string|min:1|max:255',
-                'attachment_edit_url' =>  'string|min:1|max:255'
+                'attachment_edit_url' =>  'string|min:1|max:255|safe_url'
             ]);
         } catch (ValidationException $exception) {
             return response()->view('attachments.manager-edit-form', array_merge($request->only(['attachment_edit_name', 'attachment_edit_url']), [
@@ -145,7 +145,7 @@ class AttachmentController extends Controller
             $this->validate($request, [
                 'attachment_link_uploaded_to' => 'required|integer|exists:pages,id',
                 'attachment_link_name' => 'required|string|min:1|max:255',
-                'attachment_link_url' =>  'required|string|min:1|max:255'
+                'attachment_link_url' =>  'required|string|min:1|max:255|safe_url'
             ]);
         } catch (ValidationException $exception) {
             return response()->view('attachments.manager-link-form', array_merge($request->only(['attachment_link_name', 'attachment_link_url']), [
@@ -161,7 +161,7 @@ class AttachmentController extends Controller
 
         $attachmentName = $request->get('attachment_link_name');
         $link = $request->get('attachment_link_url');
-        $attachment = $this->attachmentService->saveNewFromLink($attachmentName, $link, $pageId);
+        $attachment = $this->attachmentService->saveNewFromLink($attachmentName, $link, intval($pageId));
 
         return view('attachments.manager-link-form', [
             'pageId' => $pageId,

app/Providers/AppServiceProvider.php

@@ -43,6 +43,13 @@ class AppServiceProvider extends ServiceProvider
             return substr_count($uploadName, '.') < 2;
         });
 
+        Validator::extend('safe_url', function ($attribute, $value, $parameters, $validator) {
+            $cleanLinkName = strtolower(trim($value));
+            $isJs = strpos($cleanLinkName, 'javascript:') === 0;
+            $isData = strpos($cleanLinkName, 'data:') === 0;
+            return !$isJs && !$isData;
+        });
+
         // Custom blade view directives
         Blade::directive('icon', function ($expression) {
             return "<?php echo icon($expression); ?>";

app/Uploads/AttachmentService.php

@@ -88,12 +88,8 @@ class AttachmentService extends UploadService
 
     /**
      * Save a new File attachment from a given link and name.
-     * @param string $name
-     * @param string $link
-     * @param int $page_id
-     * @return Attachment
      */
-    public function saveNewFromLink($name, $link, $page_id)
+    public function saveNewFromLink(string $name, string $link, int $page_id): Attachment
     {
         $largestExistingOrder = Attachment::where('uploaded_to', '=', $page_id)->max('order');
         return Attachment::forceCreate([
@@ -123,13 +119,11 @@ class AttachmentService extends UploadService
 
     /**
      * Update the details of a file.
-     * @param Attachment $attachment
-     * @param $requestData
-     * @return Attachment
      */
-    public function updateFile(Attachment $attachment, $requestData)
+    public function updateFile(Attachment $attachment, array $requestData): Attachment
     {
         $attachment->name = $requestData['name'];
+
         if (isset($requestData['link']) && trim($requestData['link']) !== '') {
             $attachment->path = $requestData['link'];
             if (!$attachment->external) {
@@ -137,6 +131,7 @@ class AttachmentService extends UploadService
                 $attachment->external = true;
             }
         }
+
         $attachment->save();
         return $attachment;
     }

dev/docker/entrypoint.node.sh

@@ -5,4 +5,4 @@ set -e
 npm install
 npm rebuild node-sass
 
-exec npm run watch
+SHELL=/bin/sh exec npm run watch

package.json

@@ -5,7 +5,7 @@
     "build:css:watch": "sass ./resources/sass:./public/dist --watch",
     "build:css:production": "sass ./resources/sass:./public/dist -s compressed",
     "build:js:dev": "esbuild --bundle ./resources/js/index.js --outfile=public/dist/app.js --sourcemap --target=es2019 --main-fields=module,main",
-    "build:js:watch": "chokidar \"./resources/**/*.js\" -c \"npm run build:js:dev\"",
+    "build:js:watch": "chokidar --initial \"./resources/**/*.js\" -c \"npm run build:js:dev\"",
     "build:js:production": "NODE_ENV=production esbuild --bundle ./resources/js/index.js --outfile=public/dist/app.js --sourcemap --target=es2019 --main-fields=module,main --minify",
     "build": "npm-run-all --parallel build:*:dev",
     "production": "npm-run-all --parallel build:*:production",

resources/js/components/markdown-editor.js

@@ -440,10 +440,10 @@ class MarkdownEditor {
 
             const data = {
                 image: pngData,
-                uploaded_to: Number(document.getElementById('page-editor').getAttribute('page-id'))
+                uploaded_to: Number(this.pageId),
             };
 
-            window.$http.post(window.baseUrl('/images/drawio'), data).then(resp => {
+            window.$http.post("/images/drawio", data).then(resp => {
                 this.insertDrawing(resp.data, cursorPos);
                 DrawIO.close();
             }).catch(err => {
@@ -476,10 +476,10 @@ class MarkdownEditor {
 
             let data = {
                 image: pngData,
-                uploaded_to: Number(document.getElementById('page-editor').getAttribute('page-id'))
+                uploaded_to: Number(this.pageId),
             };
 
-            window.$http.post(window.baseUrl(`/images/drawio`), data).then(resp => {
+            window.$http.post("/images/drawio", data).then(resp => {
                 let newText = `<div drawio-diagram="${resp.data.id}"><img src="${resp.data.url}"></div>`;
                 let newContent = this.cm.getValue().split('\n').map(line => {
                     if (line.indexOf(`drawio-diagram="${drawingId}"`) !== -1) {

resources/js/services/code.js

@@ -26,6 +26,7 @@ import 'codemirror/mode/rust/rust';
 import 'codemirror/mode/shell/shell';
 import 'codemirror/mode/sql/sql';
 import 'codemirror/mode/toml/toml';
+import 'codemirror/mode/vbscript/vbscript';
 import 'codemirror/mode/xml/xml';
 import 'codemirror/mode/yaml/yaml';
 
@@ -84,6 +85,8 @@ const modeMap = {
     bash: 'shell',
     toml: 'toml',
     sql: 'text/x-sql',
+    vbs: 'vbscript',
+    vbscript: 'vbscript',
     xml: 'xml',
     yaml: 'yaml',
     yml: 'yaml',

resources/lang/en/validation.php

@@ -90,6 +90,7 @@ return [
     'required_without'     => 'The :attribute field is required when :values is not present.',
     'required_without_all' => 'The :attribute field is required when none of :values are present.',
     'same'                 => 'The :attribute and :other must match.',
+    'safe_url'             => 'The provided link may not be safe.',
     'size'                 => [
         'numeric' => 'The :attribute must be :size.',
         'file'    => 'The :attribute must be :size kilobytes.',

resources/lang/es/settings.php

@@ -179,7 +179,7 @@ return [
     'user_api_token_name_desc' => 'Dale a tu token un nombre legible como un recordatorio futuro de su propósito.',
     'user_api_token_expiry' => 'Fecha de expiración',
     'user_api_token_expiry_desc' => 'Establece una fecha en la que este token expira. Después de esta fecha, las solicitudes realizadas usando este token ya no funcionarán. Dejar este campo en blanco fijará un vencimiento de 100 años en el futuro.',
-    'user_api_token_create_secret_message' => 'Immediately after creating this token a "Token ID" & "Token Secret" will be generated and displayed. The secret will only be shown a single time so be sure to copy the value to somewhere safe and secure before proceeding.',
+    'user_api_token_create_secret_message' => 'Inmediatamente después de crear este token se generarán y mostrarán sus correspondientes "Token ID" y "Token Secret". El "Token Secret" sólo se mostrará una vez, así que asegúrese de copiar el valor a un lugar seguro antes de proceder.',
     'user_api_token_create_success' => 'Token API creado correctamente',
     'user_api_token_update_success' => 'Token API actualizado correctamente',
     'user_api_token' => 'Token API',
@@ -187,8 +187,8 @@ return [
     'user_api_token_id_desc' => 'Este es un identificador no editable generado por el sistema y único para este token que necesitará ser proporcionado en solicitudes de API.',
     'user_api_token_secret' => 'Token Secret',
     'user_api_token_secret_desc' => 'Esta es una clave no editable generada por el sistema que necesitará ser proporcionada en solicitudes de API. Solo se monstraré esta vez así que guarde su valor en un lugar seguro.',
-    'user_api_token_created' => 'Token created :timeAgo',
-    'user_api_token_updated' => 'Token updated :timeAgo',
+    'user_api_token_created' => 'Token creado :timeAgo',
+    'user_api_token_updated' => 'Token actualizado :timeAgo',
     'user_api_token_delete' => 'Borrar token',
     'user_api_token_delete_warning' => 'Esto eliminará completamente este token API con el nombre \':tokenName\' del sistema.',
     'user_api_token_delete_confirm' => '¿Está seguro de que desea borrar este API token?',

resources/lang/zh_CN/settings.php

@@ -57,7 +57,7 @@ return [
     'reg_enable_desc' => '启用注册后，用户将可以自己注册为站点用户。 注册后，他们将获得一个默认的单一用户角色。',
     'reg_default_role' => '注册后的默认用户角色',
     'reg_enable_external_warning' => '当启用外部LDAP或者SAML认证时，上面的选项会被忽略。当使用外部系统认证认证成功时，将自动创建非现有会员的用户账户。',
-    'reg_email_confirmation' => '邮箱确认n',
+    'reg_email_confirmation' => '邮件确认',
     'reg_email_confirmation_toggle' => '需要电子邮件确认',
     'reg_confirm_email_desc' => '如果使用域名限制，则需要Email验证，并且该值将被忽略。',
     'reg_confirm_restrict_domain' => '域名限制',
@@ -92,8 +92,8 @@ return [
     'audit_table_event' => '事件',
     'audit_table_item' => '相关项目',
     'audit_table_date' => '活动日期',
-    'audit_date_from' => 'Date Range From',
-    'audit_date_to' => 'Date Range To',
+    'audit_date_from' => '日期范围从',
+    'audit_date_to' => '日期范围至',
 
     // Role Settings
     'roles' => '角色',

resources/views/components/code-editor.blade.php

@@ -34,6 +34,7 @@
                             <a refs="code-editor@languageLink" data-lang="Ruby">Ruby</a>
                             <a refs="code-editor@languageLink" data-lang="shell">Shell/Bash</a>
                             <a refs="code-editor@languageLink" data-lang="SQL">SQL</a>
+                            <a refs="code-editor@languageLink" data-lang="VBScript">VBScript</a>
                             <a refs="code-editor@languageLink" data-lang="XML">XML</a>
                             <a refs="code-editor@languageLink" data-lang="YAML">YAML</a>
                         </small>
@@ -66,4 +67,4 @@
 
         </div>
     </div>
-</div>
+</div>

tests/Entity/PageContentTest.php

@@ -159,6 +159,72 @@ class PageContentTest extends TestCase
 
     }
 
+    public function test_javascript_uri_links_are_removed()
+    {
+        $checks = [
+            '<a id="xss" href="javascript:alert(document.cookie)>Click me</a>',
+            '<a id="xss" href="javascript: alert(document.cookie)>Click me</a>'
+        ];
+
+        $this->asEditor();
+        $page = Page::first();
+
+        foreach ($checks as $check) {
+            $page->html = $check;
+            $page->save();
+
+            $pageView = $this->get($page->getUrl());
+            $pageView->assertStatus(200);
+            $pageView->assertElementNotContains('.page-content', '<a id="xss">');
+            $pageView->assertElementNotContains('.page-content', 'href=javascript:');
+        }
+    }
+    public function test_form_actions_with_javascript_are_removed()
+    {
+        $checks = [
+            '<form><input id="xss" type=submit formaction=javascript:alert(document.domain) value=Submit><input></form>',
+            '<form ><button id="xss" formaction=javascript:alert(document.domain)>Click me</button></form>',
+            '<form id="xss" action=javascript:alert(document.domain)><input type=submit value=Submit></form>'
+        ];
+
+        $this->asEditor();
+        $page = Page::first();
+
+        foreach ($checks as $check) {
+            $page->html = $check;
+            $page->save();
+
+            $pageView = $this->get($page->getUrl());
+            $pageView->assertStatus(200);
+            $pageView->assertElementNotContains('.page-content', '<button id="xss"');
+            $pageView->assertElementNotContains('.page-content', '<input id="xss"');
+            $pageView->assertElementNotContains('.page-content', '<form id="xss"');
+            $pageView->assertElementNotContains('.page-content', 'action=javascript:');
+            $pageView->assertElementNotContains('.page-content', 'formaction=javascript:');
+        }
+    }
+    
+    public function test_metadata_redirects_are_removed()
+    {
+        $checks = [
+            '<meta http-equiv="refresh" content="0; url=//external_url">',
+        ];
+
+        $this->asEditor();
+        $page = Page::first();
+
+        foreach ($checks as $check) {
+            $page->html = $check;
+            $page->save();
+
+            $pageView = $this->get($page->getUrl());
+            $pageView->assertStatus(200);
+            $pageView->assertElementNotContains('.page-content', '<meta>');
+            $pageView->assertElementNotContains('.page-content', '</meta>');
+            $pageView->assertElementNotContains('.page-content', 'content=');
+            $pageView->assertElementNotContains('.page-content', 'external_url');
+        }
+    }
     public function test_page_inline_on_attributes_removed_by_default()
     {
         $this->asEditor();

tests/Uploads/AttachmentTest.php

@@ -3,39 +3,51 @@
 use BookStack\Uploads\Attachment;
 use BookStack\Entities\Page;
 use BookStack\Auth\Permissions\PermissionService;
+use BookStack\Uploads\AttachmentService;
+use Illuminate\Http\UploadedFile;
 use Tests\TestCase;
+use Tests\TestResponse;
 
 class AttachmentTest extends TestCase
 {
     /**
      * Get a test file that can be uploaded
-     * @param $fileName
-     * @return \Illuminate\Http\UploadedFile
      */
-    protected function getTestFile($fileName)
+    protected function getTestFile(string $fileName): UploadedFile
     {
-        return new \Illuminate\Http\UploadedFile(base_path('tests/test-data/test-file.txt'), $fileName, 'text/plain', 55, null, true);
+        return new UploadedFile(base_path('tests/test-data/test-file.txt'), $fileName, 'text/plain', 55, null, true);
     }
 
     /**
      * Uploads a file with the given name.
-     * @param $name
-     * @param int $uploadedTo
-     * @return \Illuminate\Foundation\Testing\TestResponse
      */
-    protected function uploadFile($name, $uploadedTo = 0)
+    protected function uploadFile(string $name, int $uploadedTo = 0): \Illuminate\Foundation\Testing\TestResponse
     {
         $file = $this->getTestFile($name);
         return $this->call('POST', '/attachments/upload', ['uploaded_to' => $uploadedTo], [], ['file' => $file], []);
     }
 
+    /**
+     * Create a new attachment
+     */
+    protected function createAttachment(Page $page): Attachment
+    {
+        $this->post('attachments/link', [
+            'attachment_link_url' => 'https://example.com',
+            'attachment_link_name' => 'Example Attachment Link',
+            'attachment_link_uploaded_to' => $page->id,
+        ]);
+
+        return Attachment::query()->latest()->first();
+    }
+
     /**
      * Delete all uploaded files.
      * To assist with cleanup.
      */
     protected function deleteUploads()
     {
-        $fileService = $this->app->make(\BookStack\Uploads\AttachmentService::class);
+        $fileService = $this->app->make(AttachmentService::class);
         foreach (Attachment::all() as $file) {
             $fileService->deleteFile($file);
         }
@@ -145,21 +157,14 @@ class AttachmentTest extends TestCase
         $page = Page::first();
         $this->asAdmin();
 
-        $this->call('POST', 'attachments/link', [
-            'attachment_link_url' => 'https://example.com',
-            'attachment_link_name' => 'Example Attachment Link',
-            'attachment_link_uploaded_to' => $page->id,
-        ]);
-
-        $attachmentId = Attachment::first()->id;
-
-        $update = $this->call('PUT', 'attachments/' . $attachmentId, [
+        $attachment = $this->createAttachment($page);
+        $update = $this->call('PUT', 'attachments/' . $attachment->id, [
             'attachment_edit_name' => 'My new attachment name',
             'attachment_edit_url' => 'https://test.example.com'
         ]);
 
         $expectedData = [
-            'id' => $attachmentId,
+            'id' => $attachment->id,
             'path' => 'https://test.example.com',
             'name' => 'My new attachment name',
             'uploaded_to' => $page->id
@@ -242,4 +247,45 @@ class AttachmentTest extends TestCase
 
         $this->deleteUploads();
     }
+
+    public function test_data_and_js_links_cannot_be_attached_to_a_page()
+    {
+        $page = Page::first();
+        $this->asAdmin();
+
+        $badLinks = [
+            'javascript:alert("bunny")',
+            ' javascript:alert("bunny")',
+            'JavaScript:alert("bunny")',
+            "\t\n\t\nJavaScript:alert(\"bunny\")",
+            "data:text/html;<a></a>",
+            "Data:text/html;<a></a>",
+            "Data:text/html;<a></a>",
+        ];
+
+        foreach ($badLinks as $badLink) {
+            $linkReq = $this->post('attachments/link', [
+                'attachment_link_url' => $badLink,
+                'attachment_link_name' => 'Example Attachment Link',
+                'attachment_link_uploaded_to' => $page->id,
+            ]);
+            $linkReq->assertStatus(422);
+            $this->assertDatabaseMissing('attachments', [
+                'path' => $badLink,
+            ]);
+        }
+
+        $attachment = $this->createAttachment($page);
+
+        foreach ($badLinks as $badLink) {
+            $linkReq = $this->put('attachments/' . $attachment->id, [
+                'attachment_edit_url' => $badLink,
+                'attachment_edit_name' => 'Example Attachment Link',
+            ]);
+            $linkReq->assertStatus(422);
+            $this->assertDatabaseMissing('attachments', [
+                'path' => $badLink,
+            ]);
+        }
+    }
 }

version

@@ -1 +1 @@
-v0.30.2
+v0.30.4