diff --git a/app/Entities/Managers/PageContent.php b/app/Entities/Managers/PageContent.php index a787e5d99..7338a36b3 100644 --- a/app/Entities/Managers/PageContent.php +++ b/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) { diff --git a/app/Http/Controllers/AttachmentController.php b/app/Http/Controllers/AttachmentController.php index 0830693bc..f52143292 100644 --- a/app/Http/Controllers/AttachmentController.php +++ b/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, diff --git a/app/Providers/AppServiceProvider.php b/app/Providers/AppServiceProvider.php index 1cc3e09c2..f41815399 100644 --- a/app/Providers/AppServiceProvider.php +++ b/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 ""; diff --git a/app/Uploads/AttachmentService.php b/app/Uploads/AttachmentService.php index 02220771a..e85901e17 100644 --- a/app/Uploads/AttachmentService.php +++ b/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; } diff --git a/dev/docker/entrypoint.node.sh b/dev/docker/entrypoint.node.sh index e59e1e8a0..a8f33fd3d 100755 --- a/dev/docker/entrypoint.node.sh +++ b/dev/docker/entrypoint.node.sh @@ -5,4 +5,4 @@ set -e npm install npm rebuild node-sass -exec npm run watch \ No newline at end of file +SHELL=/bin/sh exec npm run watch diff --git a/package.json b/package.json index c3ca2add6..d5e93a31e 100644 --- a/package.json +++ b/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", diff --git a/resources/lang/en/validation.php b/resources/lang/en/validation.php index 76b57a2a3..578ea999f 100644 --- a/resources/lang/en/validation.php +++ b/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.', diff --git a/tests/Entity/PageContentTest.php b/tests/Entity/PageContentTest.php index 99547fd17..e97df2c7e 100644 --- a/tests/Entity/PageContentTest.php +++ b/tests/Entity/PageContentTest.php @@ -159,6 +159,72 @@ class PageContentTest extends TestCase } + public function test_javascript_uri_links_are_removed() + { + $checks = [ + ''); + $pageView->assertElementNotContains('.page-content', 'href=javascript:'); + } + } + public function test_form_actions_with_javascript_are_removed() + { + $checks = [ + '
', + '
', + '
' + ]; + + $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', '