From 5f7cd735ea6904b909c176cd040a5f5e3f7eec90 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Thu, 11 Aug 2022 10:26:33 +0100 Subject: [PATCH 1/2] Added content filtering of tags with javascript or data in values attr Case would be blocked by CSP but adding for cases where CSP may not be active when content taken externally. For #3636 --- app/Util/HtmlContentFilter.php | 5 +++++ tests/Entity/PageContentTest.php | 13 +++++++++---- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/app/Util/HtmlContentFilter.php b/app/Util/HtmlContentFilter.php index 08dde7048..182f6e635 100644 --- a/app/Util/HtmlContentFilter.php +++ b/app/Util/HtmlContentFilter.php @@ -45,6 +45,11 @@ class HtmlContentFilter $badIframes = $xPath->query('//*[' . static::xpathContains('@src', 'data:') . '] | //*[' . static::xpathContains('@src', 'javascript:') . '] | //*[@srcdoc]'); static::removeNodes($badIframes); + // Remove tags hiding JavaScript or data uris in values attribute. + // For example, SVG animate tag can exploit javascript in values. + $badValuesTags = $xPath->query('//*[' . static::xpathContains('@values', 'data:') . '] | //*[' . static::xpathContains('@values', 'javascript:') . ']'); + static::removeNodes($badValuesTags); + // Remove elements with a xlink:href attribute // Used in SVG but deprecated anyway, so we'll be a bit more heavy-handed here. $xlinkHrefAttributes = $xPath->query('//@*[contains(name(), \'xlink:href\')]'); diff --git a/tests/Entity/PageContentTest.php b/tests/Entity/PageContentTest.php index d433c8b88..f88e4d513 100644 --- a/tests/Entity/PageContentTest.php +++ b/tests/Entity/PageContentTest.php @@ -325,11 +325,14 @@ class PageContentTest extends TestCase $pageView->assertDontSee('abc123abc123'); } - public function test_svg_xlink_hrefs_are_removed() + public function test_svg_script_usage_is_removed() { $checks = [ '', '', + '', + '', + '', ]; $this->asEditor(); @@ -341,9 +344,11 @@ class PageContentTest extends TestCase $pageView = $this->get($page->getUrl()); $pageView->assertStatus(200); - $this->withHtml($pageView)->assertElementNotContains('.page-content', 'alert'); - $this->withHtml($pageView)->assertElementNotContains('.page-content', 'xlink:href'); - $this->withHtml($pageView)->assertElementNotContains('.page-content', 'application/xml'); + $html = $this->withHtml($pageView); + $html->assertElementNotContains('.page-content', 'alert'); + $html->assertElementNotContains('.page-content', 'xlink:href'); + $html->assertElementNotContains('.page-content', 'application/xml'); + $html->assertElementNotContains('.page-content', 'javascript'); } } From e02bd5e57e86c3006c757fdfdbce299c4e265368 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Thu, 11 Aug 2022 10:49:45 +0100 Subject: [PATCH 2/2] Added content security section to the api docs Related to #3636 --- app/Http/Controllers/Api/PageApiController.php | 3 +++ resources/views/api-docs/index.blade.php | 1 + .../api-docs/parts/getting-started.blade.php | 16 ++++++++++++++++ 3 files changed, 20 insertions(+) diff --git a/app/Http/Controllers/Api/PageApiController.php b/app/Http/Controllers/Api/PageApiController.php index 9749985a5..de729b469 100644 --- a/app/Http/Controllers/Api/PageApiController.php +++ b/app/Http/Controllers/Api/PageApiController.php @@ -86,6 +86,9 @@ class PageApiController extends ApiController * * Pages will always have HTML content. They may have markdown content * if the markdown editor was used to last update the page. + * + * See the "Content Security" section of these docs for security considerations when using + * the page content returned from this endpoint. */ public function read(string $id) { diff --git a/resources/views/api-docs/index.blade.php b/resources/views/api-docs/index.blade.php index 52ad9f8f4..8ce24baae 100644 --- a/resources/views/api-docs/index.blade.php +++ b/resources/views/api-docs/index.blade.php @@ -16,6 +16,7 @@
Listing Endpoints
Error Handling
Rate Limits
+
Content Security
@foreach($docs as $model => $endpoints) diff --git a/resources/views/api-docs/parts/getting-started.blade.php b/resources/views/api-docs/parts/getting-started.blade.php index edc526971..0d271ec5d 100644 --- a/resources/views/api-docs/parts/getting-started.blade.php +++ b/resources/views/api-docs/parts/getting-started.blade.php @@ -179,4 +179,20 @@ API_REQUESTS_PER_MIN=180 It's generally good practice to limit requests made from your API client, where possible, to avoid affecting normal use of the system caused by over-consuming system resources. Keep in mind there may be other rate-limiting factors such as web-server & firewall controls. +

+ +
+ +
Content Security
+

+ Many of the available endpoints will return content that has been provided by user input. + Some of this content may be provided in a certain data-format (Such as HTML or Markdown for page content). + Such content is not guaranteed to be safe so keep security in mind when dealing with such user-input. + In some cases, the system will apply some filtering to content in an attempt to prevent certain vulnerabilities, but + this is not assured to be a bullet-proof defence. +

+

+ Within its own interfaces, unless disabled, the system makes use of Content Security Policy (CSP) rules to heavily negate + cross-site scripting vulnerabilities from user content. If displaying user content externally, it's advised you + also use defences such as CSP or the disabling of JavaScript completely.

\ No newline at end of file