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() ?? ''; + } }