From 5e6092aaf8fd420202016038286554860bf8ea64 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Thu, 2 Sep 2021 22:02:30 +0100 Subject: [PATCH 1/5] Added extra HTML filtering of dangerous content In particular, That around the casing of dangerous values within attributes. This uses some xpath translation to handle different casing in contains searching. --- app/Util/HtmlContentFilter.php | 19 +++++++++++++++---- tests/Entity/PageContentTest.php | 25 +++++++++++++++++++++++-- 2 files changed, 38 insertions(+), 6 deletions(-) diff --git a/app/Util/HtmlContentFilter.php b/app/Util/HtmlContentFilter.php index f251a22fd..729b80474 100644 --- a/app/Util/HtmlContentFilter.php +++ b/app/Util/HtmlContentFilter.php @@ -28,19 +28,19 @@ class HtmlContentFilter static::removeNodes($scriptElems); // Remove clickable links to JavaScript URI - $badLinks = $xPath->query('//*[contains(@href, \'javascript:\')]'); + $badLinks = $xPath->query('//*[' . static::xpathContains('@href', 'javascript:') . ']'); static::removeNodes($badLinks); // Remove forms with calls to JavaScript URI - $badForms = $xPath->query('//*[contains(@action, \'javascript:\')] | //*[contains(@formaction, \'javascript:\')]'); + $badForms = $xPath->query('//*[' . static::xpathContains('@action', 'javascript:') . '] | //*[' . static::xpathContains('@formaction', 'javascript:') . ']'); static::removeNodes($badForms); // Remove meta tag to prevent external redirects - $metaTags = $xPath->query('//meta[contains(@content, \'url\')]'); + $metaTags = $xPath->query('//meta[' . static::xpathContains('@content', 'url') . ']'); static::removeNodes($metaTags); // Remove data or JavaScript iFrames - $badIframes = $xPath->query('//*[contains(@src, \'data:\')] | //*[contains(@src, \'javascript:\')] | //*[@srcdoc]'); + $badIframes = $xPath->query('//*[' . static::xpathContains('@src', 'data:') . '] | //*[' . static::xpathContains('@src', 'javascript:') . '] | //*[@srcdoc]'); static::removeNodes($badIframes); // Remove 'on*' attributes @@ -60,6 +60,17 @@ class HtmlContentFilter return $html; } + /** + * Create a xpath contains statement with a translation automatically built within + * to affectively search in a cases-insensitive manner. + */ + protected static function xpathContains(string $property, string $value): string + { + $value = strtolower($value); + $upperVal = strtoupper($value); + return 'contains(translate(' . $property . ', \'' . $upperVal . '\', \'' . $value . '\'), \'' . $value . '\')'; + } + /** * Removed all of the given DOMNodes. */ diff --git a/tests/Entity/PageContentTest.php b/tests/Entity/PageContentTest.php index 5aee97887..193f81400 100644 --- a/tests/Entity/PageContentTest.php +++ b/tests/Entity/PageContentTest.php @@ -135,14 +135,26 @@ class PageContentTest extends TestCase } } - public function test_iframe_js_and_base64_urls_are_removed() + public function test_js_and_base64_src_urls_are_removed() { $checks = [ '', + '', + '', '', '', + '', '', + '', + '', + '', + '', + '', + '', + '', '', + '', + '', ]; $this->asEditor(); @@ -155,6 +167,7 @@ class PageContentTest extends TestCase $pageView = $this->get($page->getUrl()); $pageView->assertStatus(200); $pageView->assertElementNotContains('.page-content', ''); $pageView->assertElementNotContains('.page-content', 'src='); $pageView->assertElementNotContains('.page-content', 'javascript:'); @@ -168,6 +181,8 @@ class PageContentTest extends TestCase $checks = [ ''); + $pageView->assertElementNotContains('.page-content', 'assertElementNotContains('.page-content', 'href=javascript:'); } } @@ -188,8 +203,10 @@ class PageContentTest extends TestCase { $checks = [ '
', + '
', '
', '
', + '
', ]; $this->asEditor(); @@ -213,6 +230,8 @@ class PageContentTest extends TestCase { $checks = [ '', + '', + '', ]; $this->asEditor(); @@ -249,11 +268,13 @@ class PageContentTest extends TestCase { $checks = [ '

Hello

', + '

Hello

', '
Lorem ipsum dolor sit amet.

Hello

', '
Lorem ipsum dolor sit amet.

Hello

', '
Lorem ipsum dolor sit amet.

Hello

', '
Lorem ipsum dolor sit amet.

Hello

', 'xss link\', ]; $this->asEditor(); From 040997fdc4414776bcac06a3cbaac3b26b5e8a64 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Fri, 3 Sep 2021 22:34:49 +0100 Subject: [PATCH 2/5] Added filter for xlink:href svg xss Simply remove all such attributes --- app/Util/HtmlContentFilter.php | 26 ++++++++++++++++++++------ tests/Entity/PageContentTest.php | 22 ++++++++++++++++++++++ 2 files changed, 42 insertions(+), 6 deletions(-) diff --git a/app/Util/HtmlContentFilter.php b/app/Util/HtmlContentFilter.php index 729b80474..f3f29ae04 100644 --- a/app/Util/HtmlContentFilter.php +++ b/app/Util/HtmlContentFilter.php @@ -2,6 +2,7 @@ namespace BookStack\Util; +use DOMAttr; use DOMDocument; use DOMNodeList; use DOMXPath; @@ -43,13 +44,14 @@ class HtmlContentFilter $badIframes = $xPath->query('//*[' . static::xpathContains('@src', 'data:') . '] | //*[' . static::xpathContains('@src', 'javascript:') . '] | //*[@srcdoc]'); static::removeNodes($badIframes); + // 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\')]'); + static::removeAttributes($xlinkHrefAttributes); + // Remove 'on*' attributes $onAttributes = $xPath->query('//@*[starts-with(name(), \'on\')]'); - foreach ($onAttributes as $attr) { - /** @var \DOMAttr $attr */ - $attrName = $attr->nodeName; - $attr->parentNode->removeAttribute($attrName); - } + static::removeAttributes($onAttributes); $html = ''; $topElems = $doc->documentElement->childNodes->item(0)->childNodes; @@ -72,7 +74,7 @@ class HtmlContentFilter } /** - * Removed all of the given DOMNodes. + * Remove all the given DOMNodes. */ protected static function removeNodes(DOMNodeList $nodes): void { @@ -80,4 +82,16 @@ class HtmlContentFilter $node->parentNode->removeChild($node); } } + + /** + * Remove all the given attribute nodes. + */ + protected static function removeAttributes(DOMNodeList $attrs): void + { + /** @var DOMAttr $attr */ + foreach ($attrs as $attr) { + $attrName = $attr->nodeName; + $attr->parentNode->removeAttribute($attrName); + } + } } diff --git a/tests/Entity/PageContentTest.php b/tests/Entity/PageContentTest.php index 193f81400..1b2ce2db2 100644 --- a/tests/Entity/PageContentTest.php +++ b/tests/Entity/PageContentTest.php @@ -305,6 +305,28 @@ class PageContentTest extends TestCase $pageView->assertDontSee('abc123abc123'); } + public function test_svg_xlink_hrefs_are_removed() + { + $checks = [ + '', + '' + ]; + + $this->asEditor(); + $page = Page::query()->first(); + + foreach ($checks as $check) { + $page->html = $check; + $page->save(); + + $pageView = $this->get($page->getUrl()); + $pageView->assertStatus(200); + $pageView->assertElementNotContains('.page-content', 'alert'); + $pageView->assertElementNotContains('.page-content', 'xlink:href'); + $pageView->assertElementNotContains('.page-content', 'application/xml'); + } + } + public function test_page_inline_on_attributes_show_if_configured() { $this->asEditor(); From fd44e4ba74b7615e196dcafa4f1eddc634c0b44d Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Fri, 3 Sep 2021 23:32:42 +0100 Subject: [PATCH 3/5] Started application of CSP headers --- app/Http/Kernel.php | 2 +- app/Http/Middleware/ApplyCspRules.php | 69 +++++++++++++++++++ app/Http/Middleware/ControlIframeSecurity.php | 37 ---------- app/Util/HtmlContentFilter.php | 2 +- app/Util/HtmlNonceApplicator.php | 52 ++++++++++++++ resources/views/common/custom-head.blade.php | 2 +- 6 files changed, 124 insertions(+), 40 deletions(-) create mode 100644 app/Http/Middleware/ApplyCspRules.php delete mode 100644 app/Http/Middleware/ControlIframeSecurity.php create mode 100644 app/Util/HtmlNonceApplicator.php diff --git a/app/Http/Kernel.php b/app/Http/Kernel.php index 4b8cdfba4..a98528d0f 100644 --- a/app/Http/Kernel.php +++ b/app/Http/Kernel.php @@ -24,7 +24,7 @@ class Kernel extends HttpKernel */ protected $middlewareGroups = [ 'web' => [ - \BookStack\Http\Middleware\ControlIframeSecurity::class, + \BookStack\Http\Middleware\ApplyCspRules::class, \BookStack\Http\Middleware\EncryptCookies::class, \Illuminate\Cookie\Middleware\AddQueuedCookiesToResponse::class, \Illuminate\Session\Middleware\StartSession::class, diff --git a/app/Http/Middleware/ApplyCspRules.php b/app/Http/Middleware/ApplyCspRules.php new file mode 100644 index 000000000..2889d80b0 --- /dev/null +++ b/app/Http/Middleware/ApplyCspRules.php @@ -0,0 +1,69 @@ +share('cspNonce', $nonce); + + // TODO - Assess whether image/style/iframe CSP rules should be set + // TODO - Extract nonce application to custom head content in a way that's cacheable. + // TODO - Fix remaining CSP issues and test lots + + $response = $next($request); + + $this->setFrameAncestors($response); + $this->setScriptSrc($response, $nonce); + + return $response; + } + + /** + * Sets CSP 'script-src' headers to restrict the forms of script that can + * run on the page. + */ + public function setScriptSrc(Response $response, string $nonce) + { + $parts = [ + '\'self\'', + '\'nonce-' . $nonce . '\'', + '\'strict-dynamic\'', + ]; + $response->headers->set('Content-Security-Policy', 'script-src ' . implode(' ', $parts)); + } + + /** + * Sets CSP "frame-ancestors" headers to restrict the hosts that BookStack can be + * iframed within. Also adjusts the cookie samesite options so that cookies will + * operate in the third-party context. + */ + protected function setFrameAncestors(Response $response) + { + $iframeHosts = collect(explode(' ', config('app.iframe_hosts', '')))->filter(); + + if ($iframeHosts->count() > 0) { + config()->set('session.same_site', 'none'); + } + + $iframeHosts->prepend("'self'"); + $cspValue = 'frame-ancestors ' . $iframeHosts->join(' '); + $response->headers->set('Content-Security-Policy', $cspValue); + } +} diff --git a/app/Http/Middleware/ControlIframeSecurity.php b/app/Http/Middleware/ControlIframeSecurity.php deleted file mode 100644 index 11d9e6d4c..000000000 --- a/app/Http/Middleware/ControlIframeSecurity.php +++ /dev/null @@ -1,37 +0,0 @@ -filter(); - if ($iframeHosts->count() > 0) { - config()->set('session.same_site', 'none'); - } - - $iframeHosts->prepend("'self'"); - - $response = $next($request); - $cspValue = 'frame-ancestors ' . $iframeHosts->join(' '); - $response->headers->set('Content-Security-Policy', $cspValue); - - return $response; - } -} diff --git a/app/Util/HtmlContentFilter.php b/app/Util/HtmlContentFilter.php index f3f29ae04..aa395cc45 100644 --- a/app/Util/HtmlContentFilter.php +++ b/app/Util/HtmlContentFilter.php @@ -10,7 +10,7 @@ use DOMXPath; class HtmlContentFilter { /** - * Remove all of the script elements from the given HTML. + * Remove all the script elements from the given HTML. */ public static function removeScripts(string $html): string { diff --git a/app/Util/HtmlNonceApplicator.php b/app/Util/HtmlNonceApplicator.php new file mode 100644 index 000000000..eb2cf2687 --- /dev/null +++ b/app/Util/HtmlNonceApplicator.php @@ -0,0 +1,52 @@ +' . $html . ''; + libxml_use_internal_errors(true); + $doc = new DOMDocument(); + $doc->loadHTML(mb_convert_encoding($html, 'HTML-ENTITIES', 'UTF-8')); + $xPath = new DOMXPath($doc); + + // Apply to scripts + $scriptElems = $xPath->query('//script'); + static::addNonceAttributes($scriptElems, $nonce); + + // Apply to styles + $styleElems = $xPath->query('//style'); + static::addNonceAttributes($styleElems, $nonce); + + $returnHtml = ''; + $topElems = $doc->documentElement->childNodes->item(0)->childNodes; + foreach ($topElems as $child) { + $returnHtml .= $doc->saveHTML($child); + } + + return $returnHtml; + } + + protected static function addNonceAttributes(DOMNodeList $nodes, string $nonce): void + { + /** @var DOMElement $node */ + foreach ($nodes as $node) { + $node->setAttribute('nonce', $nonce); + } + } + +} diff --git a/resources/views/common/custom-head.blade.php b/resources/views/common/custom-head.blade.php index fa5ba0cc4..3199fc179 100644 --- a/resources/views/common/custom-head.blade.php +++ b/resources/views/common/custom-head.blade.php @@ -1,5 +1,5 @@ @if(setting('app-custom-head') && \Route::currentRouteName() !== 'settings') -{!! setting('app-custom-head') !!} +{!! \BookStack\Util\HtmlNonceApplicator::apply(setting('app-custom-head'), $cspNonce) !!} @endif \ No newline at end of file From 253f386f006eb0bcdf1151008b75213e96c4edf9 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sat, 4 Sep 2021 13:57:04 +0100 Subject: [PATCH 4/5] Finished off script CSP rules - Added caching for custom html head parsing to add nonce. - Also moved api docs page into web routes to prevent issues. --- app/Http/Middleware/ApplyCspRules.php | 60 +++++---------- app/Providers/AppServiceProvider.php | 5 ++ app/Theming/CustomHtmlHeadContentProvider.php | 63 ++++++++++++++++ app/Util/CspService.php | 72 ++++++++++++++++++ app/Util/HtmlNonceApplicator.php | 23 ++++-- resources/views/common/custom-head.blade.php | 4 +- .../views/common/export-custom-head.blade.php | 4 +- resources/views/layouts/base.blade.php | 3 +- resources/views/pages/edit.blade.php | 2 +- routes/api.php | 1 - routes/web.php | 3 + tests/Api/ApiDocsTest.php | 26 ------- tests/SecurityHeaderTest.php | 75 +++++++++++++++---- 13 files changed, 248 insertions(+), 93 deletions(-) create mode 100644 app/Theming/CustomHtmlHeadContentProvider.php create mode 100644 app/Util/CspService.php diff --git a/app/Http/Middleware/ApplyCspRules.php b/app/Http/Middleware/ApplyCspRules.php index 2889d80b0..4c2b1e1da 100644 --- a/app/Http/Middleware/ApplyCspRules.php +++ b/app/Http/Middleware/ApplyCspRules.php @@ -2,14 +2,23 @@ namespace BookStack\Http\Middleware; +use BookStack\Util\CspService; use Closure; use Illuminate\Http\Request; -use Illuminate\Support\Str; -use Symfony\Component\HttpFoundation\Response; - class ApplyCspRules { + + /** + * @var CspService + */ + protected $cspService; + + public function __construct(CspService $cspService) + { + $this->cspService = $cspService; + } + /** * Handle an incoming request. * @@ -20,50 +29,17 @@ class ApplyCspRules */ public function handle($request, Closure $next) { - $nonce = Str::random(24); - view()->share('cspNonce', $nonce); - - // TODO - Assess whether image/style/iframe CSP rules should be set - // TODO - Extract nonce application to custom head content in a way that's cacheable. - // TODO - Fix remaining CSP issues and test lots + view()->share('cspNonce', $this->cspService->getNonce()); + if ($this->cspService->allowedIFrameHostsConfigured()) { + config()->set('session.same_site', 'none'); + } $response = $next($request); - $this->setFrameAncestors($response); - $this->setScriptSrc($response, $nonce); + $this->cspService->setFrameAncestors($response); + $this->cspService->setScriptSrc($response); return $response; } - /** - * Sets CSP 'script-src' headers to restrict the forms of script that can - * run on the page. - */ - public function setScriptSrc(Response $response, string $nonce) - { - $parts = [ - '\'self\'', - '\'nonce-' . $nonce . '\'', - '\'strict-dynamic\'', - ]; - $response->headers->set('Content-Security-Policy', 'script-src ' . implode(' ', $parts)); - } - - /** - * Sets CSP "frame-ancestors" headers to restrict the hosts that BookStack can be - * iframed within. Also adjusts the cookie samesite options so that cookies will - * operate in the third-party context. - */ - protected function setFrameAncestors(Response $response) - { - $iframeHosts = collect(explode(' ', config('app.iframe_hosts', '')))->filter(); - - if ($iframeHosts->count() > 0) { - config()->set('session.same_site', 'none'); - } - - $iframeHosts->prepend("'self'"); - $cspValue = 'frame-ancestors ' . $iframeHosts->join(' '); - $response->headers->set('Content-Security-Policy', $cspValue); - } } diff --git a/app/Providers/AppServiceProvider.php b/app/Providers/AppServiceProvider.php index 145a7645b..1119d87df 100644 --- a/app/Providers/AppServiceProvider.php +++ b/app/Providers/AppServiceProvider.php @@ -12,6 +12,7 @@ use BookStack\Entities\Models\Chapter; use BookStack\Entities\Models\Page; use BookStack\Settings\Setting; use BookStack\Settings\SettingService; +use BookStack\Util\CspService; use Illuminate\Contracts\Cache\Repository; use Illuminate\Database\Eloquent\Relations\Relation; use Illuminate\Support\Facades\View; @@ -71,5 +72,9 @@ class AppServiceProvider extends ServiceProvider $this->app->singleton(SocialAuthService::class, function ($app) { return new SocialAuthService($app->make(SocialiteFactory::class), $app->make(LoginService::class)); }); + + $this->app->singleton(CspService::class, function($app) { + return new CspService(); + }); } } diff --git a/app/Theming/CustomHtmlHeadContentProvider.php b/app/Theming/CustomHtmlHeadContentProvider.php new file mode 100644 index 000000000..6110d5a60 --- /dev/null +++ b/app/Theming/CustomHtmlHeadContentProvider.php @@ -0,0 +1,63 @@ +cspService = $cspService; + $this->cache = $cache; + } + + /** + * Fetch our custom HTML head content prepared for use on web pages. + * Content has a nonce applied for CSP. + */ + public function forWeb(): string + { + $content = $this->getSourceContent(); + $hash = md5($content); + $html = $this->cache->remember('custom-head-web:' . $hash, 86400, function() use ($content) { + return HtmlNonceApplicator::prepare($content); + }); + return HtmlNonceApplicator::apply($html, $this->cspService->getNonce()); + } + + /** + * Fetch our custom HTML head content prepared for use in export formats. + * Scripts are stripped to avoid potential issues. + */ + public function forExport(): string + { + $content = $this->getSourceContent(); + $hash = md5($content); + return $this->cache->remember('custom-head-export:' . $hash, 86400, function() use ($content) { + return HtmlContentFilter::removeScripts($content); + }); + } + + /** + * Get the original custom head content to use. + */ + protected function getSourceContent(): string + { + return setting('app-custom-head', ''); + } + +} \ No newline at end of file diff --git a/app/Util/CspService.php b/app/Util/CspService.php new file mode 100644 index 000000000..2728aae44 --- /dev/null +++ b/app/Util/CspService.php @@ -0,0 +1,72 @@ +nonce = $nonce ?: Str::random(16); + } + + /** + * Get the nonce value for CSP. + */ + public function getNonce(): string + { + return $this->nonce; + } + + /** + * Sets CSP 'script-src' headers to restrict the forms of script that can + * run on the page. + */ + public function setScriptSrc(Response $response) + { + if (config('app.allow_content_scripts')) { + return; + } + + $parts = [ + '\'nonce-' . $this->nonce . '\'', + '\'strict-dynamic\'', + ]; + $value = 'script-src ' . implode(' ', $parts); + $response->headers->set('Content-Security-Policy', $value, false); + } + + /** + * Sets CSP "frame-ancestors" headers to restrict the hosts that BookStack can be + * iframed within. Also adjusts the cookie samesite options so that cookies will + * operate in the third-party context. + */ + public function setFrameAncestors(Response $response) + { + $iframeHosts = $this->getAllowedIframeHosts(); + array_unshift($iframeHosts, "'self'"); + $cspValue = 'frame-ancestors ' . implode(' ', $iframeHosts); + $response->headers->set('Content-Security-Policy', $cspValue, false); + } + + /** + * Check if the user has configured some allowed iframe hosts. + */ + public function allowedIFrameHostsConfigured(): bool + { + return count($this->getAllowedIframeHosts()) > 0; + } + + + protected function getAllowedIframeHosts(): array + { + $hosts = config('app.iframe_hosts', ''); + return array_filter(explode(' ', $hosts)); + } + +} \ No newline at end of file diff --git a/app/Util/HtmlNonceApplicator.php b/app/Util/HtmlNonceApplicator.php index eb2cf2687..e66625bf2 100644 --- a/app/Util/HtmlNonceApplicator.php +++ b/app/Util/HtmlNonceApplicator.php @@ -9,10 +9,13 @@ use DOMXPath; class HtmlNonceApplicator { + protected static $placeholder = '[CSP_NONCE_VALUE]'; + /** - * Apply the given nonce to all scripts and styles in the given html. + * Prepare the given HTML content with nonce attributes including a placeholder + * value which we can target later. */ - public static function apply(string $html, string $nonce): string + public static function prepare(string $html): string { if (empty($html)) { return $html; @@ -26,11 +29,11 @@ class HtmlNonceApplicator // Apply to scripts $scriptElems = $xPath->query('//script'); - static::addNonceAttributes($scriptElems, $nonce); + static::addNonceAttributes($scriptElems, static::$placeholder); // Apply to styles $styleElems = $xPath->query('//style'); - static::addNonceAttributes($styleElems, $nonce); + static::addNonceAttributes($styleElems, static::$placeholder); $returnHtml = ''; $topElems = $doc->documentElement->childNodes->item(0)->childNodes; @@ -41,11 +44,19 @@ class HtmlNonceApplicator return $returnHtml; } - protected static function addNonceAttributes(DOMNodeList $nodes, string $nonce): void + /** + * Apply the give nonce value to the given prepared HTML. + */ + public static function apply(string $html, string $nonce): string + { + return str_replace(static::$placeholder, $nonce, $html); + } + + protected static function addNonceAttributes(DOMNodeList $nodes, string $attrValue): void { /** @var DOMElement $node */ foreach ($nodes as $node) { - $node->setAttribute('nonce', $nonce); + $node->setAttribute('nonce', $attrValue); } } diff --git a/resources/views/common/custom-head.blade.php b/resources/views/common/custom-head.blade.php index 3199fc179..6f88bd43f 100644 --- a/resources/views/common/custom-head.blade.php +++ b/resources/views/common/custom-head.blade.php @@ -1,5 +1,7 @@ +@inject('headContent', 'BookStack\Theming\CustomHtmlHeadContentProvider') + @if(setting('app-custom-head') && \Route::currentRouteName() !== 'settings') -{!! \BookStack\Util\HtmlNonceApplicator::apply(setting('app-custom-head'), $cspNonce) !!} +{!! $headContent->forWeb() !!} @endif \ No newline at end of file diff --git a/resources/views/common/export-custom-head.blade.php b/resources/views/common/export-custom-head.blade.php index f428e9fe9..2452d6b8e 100644 --- a/resources/views/common/export-custom-head.blade.php +++ b/resources/views/common/export-custom-head.blade.php @@ -1,5 +1,7 @@ +@inject('headContent', 'BookStack\Theming\CustomHtmlHeadContentProvider') + @if(setting('app-custom-head')) -{!! \BookStack\Util\HtmlContentFilter::removeScripts(setting('app-custom-head')) !!} +{!! $headContent->forExport() !!} @endif \ No newline at end of file diff --git a/resources/views/layouts/base.blade.php b/resources/views/layouts/base.blade.php index 6a45b4209..1f28e354c 100644 --- a/resources/views/layouts/base.blade.php +++ b/resources/views/layouts/base.blade.php @@ -15,7 +15,6 @@ @stack('social-meta') - @@ -51,7 +50,7 @@ @yield('bottom') - + @yield('scripts') diff --git a/resources/views/pages/edit.blade.php b/resources/views/pages/edit.blade.php index db518b0d4..6d2c3d484 100644 --- a/resources/views/pages/edit.blade.php +++ b/resources/views/pages/edit.blade.php @@ -1,7 +1,7 @@ @extends('layouts.base') @section('head') - + @stop @section('body-class', 'flexbox') diff --git a/routes/api.php b/routes/api.php index a6ed0c8f1..83a411219 100644 --- a/routes/api.php +++ b/routes/api.php @@ -5,7 +5,6 @@ * Routes have a uri prefix of /api/. * Controllers are all within app/Http/Controllers/Api. */ -Route::get('docs', 'ApiDocsController@display'); Route::get('docs.json', 'ApiDocsController@json'); Route::get('books', 'BookApiController@list'); diff --git a/routes/web.php b/routes/web.php index a823b73c8..08adeceb9 100644 --- a/routes/web.php +++ b/routes/web.php @@ -10,6 +10,9 @@ Route::group(['middleware' => 'auth'], function () { Route::get('/uploads/images/{path}', 'Images\ImageController@showImage') ->where('path', '.*$'); + // API docs routes + Route::get('/api/docs', 'Api\ApiDocsController@display'); + Route::get('/pages/recently-updated', 'PageController@showRecentlyUpdated'); // Shelves diff --git a/tests/Api/ApiDocsTest.php b/tests/Api/ApiDocsTest.php index 90d107eb3..062adce53 100644 --- a/tests/Api/ApiDocsTest.php +++ b/tests/Api/ApiDocsTest.php @@ -2,7 +2,6 @@ namespace Tests\Api; -use BookStack\Auth\User; use Tests\TestCase; class ApiDocsTest extends TestCase @@ -11,16 +10,6 @@ class ApiDocsTest extends TestCase protected $endpoint = '/api/docs'; - public function test_docs_page_not_visible_to_normal_viewers() - { - $viewer = $this->getViewer(); - $resp = $this->actingAs($viewer)->get($this->endpoint); - $resp->assertStatus(403); - - $resp = $this->actingAsApiEditor()->get($this->endpoint); - $resp->assertStatus(200); - } - public function test_docs_page_returns_view_with_docs_content() { $resp = $this->actingAsApiEditor()->get($this->endpoint); @@ -42,19 +31,4 @@ class ApiDocsTest extends TestCase ]], ]); } - - public function test_docs_page_visible_by_public_user_if_given_permission() - { - $this->setSettings(['app-public' => true]); - $guest = User::getDefault(); - - $this->startSession(); - $resp = $this->get('/api/docs'); - $resp->assertStatus(403); - - $this->giveUserPermissions($guest, ['access-api']); - - $resp = $this->get('/api/docs'); - $resp->assertStatus(200); - } } diff --git a/tests/SecurityHeaderTest.php b/tests/SecurityHeaderTest.php index 888dac810..57f4ab0df 100644 --- a/tests/SecurityHeaderTest.php +++ b/tests/SecurityHeaderTest.php @@ -2,7 +2,7 @@ namespace Tests; -use Illuminate\Support\Str; +use BookStack\Util\CspService; class SecurityHeaderTest extends TestCase { @@ -44,26 +44,75 @@ class SecurityHeaderTest extends TestCase public function test_iframe_csp_self_only_by_default() { $resp = $this->get('/'); - $cspHeaders = collect($resp->headers->get('Content-Security-Policy')); - $frameHeaders = $cspHeaders->filter(function ($val) { - return Str::startsWith($val, 'frame-ancestors'); - }); + $frameHeader = $this->getCspHeader($resp, 'frame-ancestors'); - $this->assertTrue($frameHeaders->count() === 1); - $this->assertEquals('frame-ancestors \'self\'', $frameHeaders->first()); + $this->assertEquals('frame-ancestors \'self\'', $frameHeader); } public function test_iframe_csp_includes_extra_hosts_if_configured() { $this->runWithEnv('ALLOWED_IFRAME_HOSTS', 'https://a.example.com https://b.example.com', function () { $resp = $this->get('/'); - $cspHeaders = collect($resp->headers->get('Content-Security-Policy')); - $frameHeaders = $cspHeaders->filter(function ($val) { - return Str::startsWith($val, 'frame-ancestors'); - }); + $frameHeader = $this->getCspHeader($resp, 'frame-ancestors'); - $this->assertTrue($frameHeaders->count() === 1); - $this->assertEquals('frame-ancestors \'self\' https://a.example.com https://b.example.com', $frameHeaders->first()); + $this->assertNotEmpty($frameHeader); + $this->assertEquals('frame-ancestors \'self\' https://a.example.com https://b.example.com', $frameHeader); }); } + + public function test_script_csp_set_on_responses() + { + $resp = $this->get('/'); + $scriptHeader = $this->getCspHeader($resp, 'script-src'); + $this->assertStringContainsString('\'strict-dynamic\'', $scriptHeader); + $this->assertStringContainsString('\'nonce-', $scriptHeader); + } + + public function test_script_csp_nonce_matches_nonce_used_in_custom_head() + { + $this->setSettings(['app-custom-head' => '']); + $resp = $this->get('/login'); + $scriptHeader = $this->getCspHeader($resp, 'script-src'); + + $nonce = app()->make(CspService::class)->getNonce(); + $this->assertStringContainsString('nonce-' . $nonce, $scriptHeader); + $resp->assertSee(''); + } + + public function test_script_csp_nonce_changes_per_request() + { + $resp = $this->get('/'); + $firstHeader = $this->getCspHeader($resp, 'script-src'); + + $this->refreshApplication(); + + $resp = $this->get('/'); + $secondHeader = $this->getCspHeader($resp, 'script-src'); + + $this->assertNotEquals($firstHeader, $secondHeader); + } + + public function test_allow_content_scripts_settings_controls_csp_script_headers() + { + config()->set('app.allow_content_scripts', true); + $resp = $this->get('/'); + $scriptHeader = $this->getCspHeader($resp, 'script-src'); + $this->assertEmpty($scriptHeader); + + config()->set('app.allow_content_scripts', false); + $resp = $this->get('/'); + $scriptHeader = $this->getCspHeader($resp, 'script-src'); + $this->assertNotEmpty($scriptHeader); + } + + /** + * Get the value of the first CSP header of the given type. + */ + protected function getCspHeader(TestResponse $resp, string $type): string + { + $cspHeaders = collect($resp->headers->all('Content-Security-Policy')); + return $cspHeaders->filter(function ($val) use ($type) { + return strpos($val, $type) === 0; + })->first() ?? ''; + } } From 492af79c27f089e28c76007f93fef4995eda9d94 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sat, 4 Sep 2021 14:34:43 +0100 Subject: [PATCH 5/5] Added a couple of additional CSP rules As per guidance from google's CSP evaluator. --- app/Http/Middleware/ApplyCspRules.php | 2 ++ app/Util/CspService.php | 24 ++++++++++++++++++++++++ tests/SecurityHeaderTest.php | 14 ++++++++++++++ 3 files changed, 40 insertions(+) diff --git a/app/Http/Middleware/ApplyCspRules.php b/app/Http/Middleware/ApplyCspRules.php index 4c2b1e1da..a65d12a05 100644 --- a/app/Http/Middleware/ApplyCspRules.php +++ b/app/Http/Middleware/ApplyCspRules.php @@ -38,6 +38,8 @@ class ApplyCspRules $this->cspService->setFrameAncestors($response); $this->cspService->setScriptSrc($response); + $this->cspService->setObjectSrc($response); + $this->cspService->setBaseUri($response); return $response; } diff --git a/app/Util/CspService.php b/app/Util/CspService.php index 2728aae44..2979ebc3e 100644 --- a/app/Util/CspService.php +++ b/app/Util/CspService.php @@ -34,9 +34,12 @@ class CspService } $parts = [ + 'http:', + 'https:', '\'nonce-' . $this->nonce . '\'', '\'strict-dynamic\'', ]; + $value = 'script-src ' . implode(' ', $parts); $response->headers->set('Content-Security-Policy', $value, false); } @@ -62,6 +65,27 @@ class CspService return count($this->getAllowedIframeHosts()) > 0; } + /** + * Sets CSP 'object-src' headers to restrict the types of dynamic content + * that can be embedded on the page. + */ + public function setObjectSrc(Response $response) + { + if (config('app.allow_content_scripts')) { + return; + } + + $response->headers->set('Content-Security-Policy', 'object-src \'self\'', false); + } + + /** + * Sets CSP 'base-uri' headers to restrict what base tags can be set on + * the page to prevent manipulation of relative links. + */ + public function setBaseUri(Response $response) + { + $response->headers->set('Content-Security-Policy', 'base-uri \'self\'', false); + } protected function getAllowedIframeHosts(): array { diff --git a/tests/SecurityHeaderTest.php b/tests/SecurityHeaderTest.php index 57f4ab0df..fe25ef3f0 100644 --- a/tests/SecurityHeaderTest.php +++ b/tests/SecurityHeaderTest.php @@ -105,6 +105,20 @@ class SecurityHeaderTest extends TestCase $this->assertNotEmpty($scriptHeader); } + public function test_object_src_csp_header_set() + { + $resp = $this->get('/'); + $scriptHeader = $this->getCspHeader($resp, 'object-src'); + $this->assertEquals('object-src \'self\'', $scriptHeader); + } + + public function test_base_uri_csp_header_set() + { + $resp = $this->get('/'); + $scriptHeader = $this->getCspHeader($resp, 'base-uri'); + $this->assertEquals('base-uri \'self\'', $scriptHeader); + } + /** * Get the value of the first CSP header of the given type. */