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..a65d12a05 --- /dev/null +++ b/app/Http/Middleware/ApplyCspRules.php @@ -0,0 +1,47 @@ +cspService = $cspService; + } + + /** + * Handle an incoming request. + * + * @param Request $request + * @param Closure $next + * + * @return mixed + */ + public function handle($request, Closure $next) + { + view()->share('cspNonce', $this->cspService->getNonce()); + if ($this->cspService->allowedIFrameHostsConfigured()) { + config()->set('session.same_site', 'none'); + } + + $response = $next($request); + + $this->cspService->setFrameAncestors($response); + $this->cspService->setScriptSrc($response); + $this->cspService->setObjectSrc($response); + $this->cspService->setBaseUri($response); + + return $response; + } + +} 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/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..2979ebc3e --- /dev/null +++ b/app/Util/CspService.php @@ -0,0 +1,96 @@ +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 = [ + 'http:', + 'https:', + '\'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; + } + + /** + * 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 + { + $hosts = config('app.iframe_hosts', ''); + return array_filter(explode(' ', $hosts)); + } + +} \ No newline at end of file diff --git a/app/Util/HtmlContentFilter.php b/app/Util/HtmlContentFilter.php index f251a22fd..aa395cc45 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; @@ -9,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 { @@ -28,28 +29,29 @@ 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 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; @@ -61,7 +63,18 @@ class HtmlContentFilter } /** - * Removed all of the given DOMNodes. + * 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 . '\')'; + } + + /** + * Remove all the given DOMNodes. */ protected static function removeNodes(DOMNodeList $nodes): void { @@ -69,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/app/Util/HtmlNonceApplicator.php b/app/Util/HtmlNonceApplicator.php new file mode 100644 index 000000000..e66625bf2 --- /dev/null +++ b/app/Util/HtmlNonceApplicator.php @@ -0,0 +1,63 @@ +' . $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, static::$placeholder); + + // Apply to styles + $styleElems = $xPath->query('//style'); + static::addNonceAttributes($styleElems, static::$placeholder); + + $returnHtml = ''; + $topElems = $doc->documentElement->childNodes->item(0)->childNodes; + foreach ($topElems as $child) { + $returnHtml .= $doc->saveHTML($child); + } + + return $returnHtml; + } + + /** + * 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', $attrValue); + } + } + +} diff --git a/resources/views/common/custom-head.blade.php b/resources/views/common/custom-head.blade.php index fa5ba0cc4..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') -{!! setting('app-custom-head') !!} +{!! $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/Entity/PageContentTest.php b/tests/Entity/PageContentTest.php index 5aee97887..1b2ce2db2 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(); @@ -284,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(); diff --git a/tests/SecurityHeaderTest.php b/tests/SecurityHeaderTest.php index 888dac810..fe25ef3f0 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,89 @@ 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); + } + + 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. + */ + 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() ?? ''; + } }