From d0626f00d33300752abb45b0e92b38b98a770798 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?H=C3=A9ctor=20Franco?= Date: Wed, 28 Oct 2026 13:08:59 +0100 Subject: [PATCH] fix(security): empty access_roles deny and CSRF fails closed - ConfigurablePageLayoutKitAccessChecker returned true for access_roles: [], opening the layout admin and block edit endpoints to every user. An empty list now grants nothing and the extension refuses to compile it unless access_checker or allow_unauthenticated is set. - CsrfProtectionPass refuses to compile without security.csrf.token_manager and form.type_extension.csrf, or with framework.form.csrf_protection: false. symfony/security-csrf is now required. --- composer.json | 1 + composer.lock | 2 +- docs/CHANGELOG.md | 11 +++ docs/CONFIGURATION.md | 5 +- docs/SECURITY.md | 5 +- docs/UPGRADING.md | 13 ++++ .../Compiler/CsrfProtectionPass.php | 40 ++++++++++ src/DependencyInjection/Configuration.php | 1 + .../NowoPageLayoutKitExtension.php | 4 + src/NowoPageLayoutKitBundle.php | 2 + ...ConfigurablePageLayoutKitAccessChecker.php | 17 ++--- .../Compiler/CsrfProtectionPassTest.php | 74 +++++++++++++++++++ .../NowoPageLayoutKitExtensionTest.php | 29 ++++++++ tests/Unit/NowoPageLayoutKitBundleTest.php | 2 + .../PageLayoutKitAccessCheckerTest.php | 4 +- 15 files changed, 193 insertions(+), 17 deletions(-) create mode 100644 src/DependencyInjection/Compiler/CsrfProtectionPass.php create mode 100644 tests/Unit/DependencyInjection/Compiler/CsrfProtectionPassTest.php diff --git a/composer.json b/composer.json index bbbe135..825ca28 100644 --- a/composer.json +++ b/composer.json @@ -45,6 +45,7 @@ "symfony/http-kernel": "^7.4 || ^8.0", "symfony/routing": "^7.4 || ^8.0", "symfony/security-bundle": "^7.4 || ^8.0", + "symfony/security-csrf": "^7.4 || ^8.0", "symfony/translation": "^7.4 || ^8.0", "symfony/twig-bundle": "^7.4 || ^8.0", "symfony/yaml": "^7.4 || ^8.0", diff --git a/composer.lock b/composer.lock index d3af9d1..a247344 100644 --- a/composer.lock +++ b/composer.lock @@ -4,7 +4,7 @@ "Read more about it at https://getcomposer.org/doc/01-basic-usage.md#installing-dependencies", "This file is @generated automatically" ], - "content-hash": "ddcaf0bc3435e30f4a45155a11e04f2e", + "content-hash": "3842c819b7112de774d94c4fc918a9b5", "packages": [ { "name": "doctrine/collections", diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index ca390c6..01a2d06 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -28,6 +28,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Security + +- **Empty role list denies (P1):** `ConfigurablePageLayoutKitAccessChecker` returned `true` for `security.access_roles: []`, + opening the layout admin and block edit endpoints to every user reaching them. An empty list now grants nothing, and + `NowoPageLayoutKitExtension` throws a `LogicException` at compile time when `access_roles` is empty while + `allow_unauthenticated` is `false` and no `access_checker` is set. +- **CSRF fails closed (P2):** new `CsrfProtectionPass` refuses to compile without `security.csrf.token_manager` and + `form.type_extension.csrf`, or with `framework.form.csrf_protection: false`. Before, block reorder and block edit + accepted forged POSTs on such hosts. `symfony/security-csrf` is now a required dependency. + See [UPGRADING](UPGRADING.md#unreleased). + ## [1.7.0] - 2026-10-22 Dependency refresh, logged legacy page-block migration (psr/log), demo login via Symfony form and raw-form release gate. See [UPGRADING](UPGRADING.md#to-170). diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index 1ff5cd6..ef71e9a 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -63,12 +63,15 @@ Sanitization runs on Doctrine persist/update for text/compare/cta block translat | Key | Default | Description | | --- | --- | --- | -| `access_roles` | `[ROLE_EDITOR]` | Any matching role grants admin access when no custom checker is configured. | +| `access_roles` | `[ROLE_EDITOR]` | Any matching role grants admin access when no custom checker is configured. An empty list **denies** (never "everyone") and fails container compilation unless `access_checker` or `allow_unauthenticated` is set. | | `access_checker` | `null` | Optional service id implementing `PageLayoutKitAccessCheckerInterface`. | | `allow_unauthenticated` | `false` | When `true`, the bundle uses an allow-all checker. Intended only for trusted demos or internal tools. | The bundle enforces access on route names beginning with `admin_page_layout` and `admin_page_blocks`. +The container also refuses to compile without form CSRF protection (`security.csrf.token_manager` + +`form.type_extension.csrf`, `framework.form.csrf_protection` not `false`): block reorder and block edit rely on it. + ## web_ui | Key | Default | Description | diff --git a/docs/SECURITY.md b/docs/SECURITY.md index 82edb0f..d56029c 100644 --- a/docs/SECURITY.md +++ b/docs/SECURITY.md @@ -19,8 +19,9 @@ Security considerations for page layout editing, inline CMS modals, and public b | Risk | Mitigation | | --- | --- | | Unauthorized editors reach layout admin or block edit endpoints | `access_roles`, custom `access_checker`, and Symfony Security | +| Misconfigured empty `access_roles` opens the admin to every user | An empty list grants nothing (`ConfigurablePageLayoutKitAccessChecker`); `access_roles: []` without `access_checker` / `allow_unauthenticated` → container compile error (`NowoPageLayoutKitExtension`) | | Stored XSS through rich text block content | Default Twig auto-escaping where possible; review templates that intentionally use `|raw` | -| CSRF on reorder or modal save actions | Symfony forms and CSRF-protected admin flows | +| CSRF on reorder or modal save actions | Symfony forms with CSRF tokens; `CsrfProtectionPass` fails compilation (fail closed) when `security.csrf.token_manager` / `form.type_extension.csrf` is missing or `framework.form.csrf_protection` is false | | Overly broad demo access | `allow_unauthenticated` defaults to `false`; enable only for trusted demos | | Shared-database table collisions | Optional `doctrine.table_prefix` | @@ -59,7 +60,7 @@ The reorder form and modal update forms are submitted through Symfony forms, not Recommendations: - Do not expose admin routes outside authenticated editor sessions. -- Do not remove CSRF handling from overridden forms. +- Do not remove CSRF handling from overridden forms. The bundle refuses to compile when form CSRF protection is off (`DependencyInjection\Compiler\CsrfProtectionPass`). - Keep modal edit endpoints behind the same editor policy as the reorder UI. ## Rich text rendering diff --git a/docs/UPGRADING.md b/docs/UPGRADING.md index 68078e7..925379a 100644 --- a/docs/UPGRADING.md +++ b/docs/UPGRADING.md @@ -23,6 +23,19 @@ ## Unreleased +```bash +composer update nowo-tech/page-layout-kit-bundle +``` + +**Behaviour changes (security):** + +- `nowo_page_layout_kit.security.access_roles: []` used to allow everyone; it now fails container compilation (and + an empty list denies). Configure a role, set `security.access_checker`, or — trusted demos only — + `security.allow_unauthenticated: true`. +- The container no longer compiles without form CSRF protection. Keep `framework.csrf_protection` and + `framework.form.csrf_protection` enabled; `symfony/security-csrf` is now a required dependency. +- Default configuration (`[ROLE_EDITOR]`, CSRF on) is unaffected. + ## To 1.7.0 From **1.6.1**: diff --git a/src/DependencyInjection/Compiler/CsrfProtectionPass.php b/src/DependencyInjection/Compiler/CsrfProtectionPass.php new file mode 100644 index 0000000..f99685a --- /dev/null +++ b/src/DependencyInjection/Compiler/CsrfProtectionPass.php @@ -0,0 +1,40 @@ +hasParameter('nowo_page_layout_kit.security.access_roles')) { + return; + } + + $formCsrfDisabled = $container->hasParameter('form.type_extension.csrf.enabled') + && $container->getParameter('form.type_extension.csrf.enabled') === false; + + if ( + !$container->has('security.csrf.token_manager') + || !$container->hasDefinition('form.type_extension.csrf') + || $formCsrfDisabled + ) { + throw new LogicException('NowoPageLayoutKitBundle: the layout admin requires CSRF protection. Enable framework.csrf_protection and framework.form.csrf_protection (composer require symfony/security-csrf).'); + } + } +} diff --git a/src/DependencyInjection/Configuration.php b/src/DependencyInjection/Configuration.php index d90a071..03c8926 100644 --- a/src/DependencyInjection/Configuration.php +++ b/src/DependencyInjection/Configuration.php @@ -51,6 +51,7 @@ public function getConfigTreeBuilder(): TreeBuilder ->arrayNode('access_roles') ->scalarPrototype()->end() ->defaultValue(['ROLE_EDITOR']) + ->info('Roles allowed to use the layout admin (user needs at least one). Empty list → grants nothing; fails container compilation unless access_checker or allow_unauthenticated is set.') ->end() ->scalarNode('access_checker')->defaultNull()->end() ->booleanNode('allow_unauthenticated')->defaultFalse()->end() diff --git a/src/DependencyInjection/NowoPageLayoutKitExtension.php b/src/DependencyInjection/NowoPageLayoutKitExtension.php index 7a20bf8..ac69d3b 100644 --- a/src/DependencyInjection/NowoPageLayoutKitExtension.php +++ b/src/DependencyInjection/NowoPageLayoutKitExtension.php @@ -131,6 +131,10 @@ private function registerAccessChecker(ContainerBuilder $container, array $secur return; } + if ($security['access_roles'] === []) { + throw new LogicException('NowoPageLayoutKitBundle: security.access_roles is empty, so nobody could use the admin UI (an empty list denies). Configure at least one role, set security.access_checker, or set security.allow_unauthenticated for a trusted demo.'); + } + $id = 'nowo_page_layout_kit.access_checker.default'; $definition = new Definition(ConfigurablePageLayoutKitAccessChecker::class); $definition->setArgument('$accessRoles', $security['access_roles']); diff --git a/src/NowoPageLayoutKitBundle.php b/src/NowoPageLayoutKitBundle.php index 38cb6d9..de45ad4 100644 --- a/src/NowoPageLayoutKitBundle.php +++ b/src/NowoPageLayoutKitBundle.php @@ -5,6 +5,7 @@ namespace Nowo\PageLayoutKitBundle; use Doctrine\Bundle\DoctrineBundle\DependencyInjection\Compiler\DoctrineOrmMappingsPass; +use Nowo\PageLayoutKitBundle\DependencyInjection\Compiler\CsrfProtectionPass; use Nowo\PageLayoutKitBundle\DependencyInjection\Compiler\TwigPathsPass; use Nowo\PageLayoutKitBundle\DependencyInjection\NowoPageLayoutKitExtension; use Nowo\PageLayoutKitBundle\Locale\PageLocales; @@ -19,6 +20,7 @@ class NowoPageLayoutKitBundle extends Bundle { public function build(ContainerBuilder $container): void { + $container->addCompilerPass(new CsrfProtectionPass()); $container->addCompilerPass(new TwigPathsPass()); $entityDir = __DIR__ . '/Entity'; diff --git a/src/Security/ConfigurablePageLayoutKitAccessChecker.php b/src/Security/ConfigurablePageLayoutKitAccessChecker.php index 49e8161..388cf1b 100644 --- a/src/Security/ConfigurablePageLayoutKitAccessChecker.php +++ b/src/Security/ConfigurablePageLayoutKitAccessChecker.php @@ -6,6 +6,11 @@ use Symfony\Component\Security\Core\Authorization\AuthorizationCheckerInterface; +/** + * Role-based access driven by nowo_page_layout_kit.security.access_roles. + * + * An empty role list grants nothing (fail closed); the extension refuses to compile that configuration. + */ final readonly class ConfigurablePageLayoutKitAccessChecker implements PageLayoutKitAccessCheckerInterface { /** @@ -19,16 +24,6 @@ public function __construct( public function canAccess(): bool { - if ($this->accessRoles === []) { - return true; - } - - foreach ($this->accessRoles as $role) { - if ($this->authorizationChecker->isGranted($role)) { - return true; - } - } - - return false; + return array_any($this->accessRoles, fn (string $role): bool => $this->authorizationChecker->isGranted($role)); } } diff --git a/tests/Unit/DependencyInjection/Compiler/CsrfProtectionPassTest.php b/tests/Unit/DependencyInjection/Compiler/CsrfProtectionPassTest.php new file mode 100644 index 0000000..b357104 --- /dev/null +++ b/tests/Unit/DependencyInjection/Compiler/CsrfProtectionPassTest.php @@ -0,0 +1,74 @@ +process($container); + + self::assertFalse($container->has('security.csrf.token_manager')); + } + + public function testPassesWithTokenManagerAndFormExtension(): void + { + $container = $this->protectedContainer(); + $container->setParameter('form.type_extension.csrf.enabled', true); + + (new CsrfProtectionPass())->process($container); + + self::assertTrue($container->hasDefinition('form.type_extension.csrf')); + } + + public function testFailsWithoutTokenManager(): void + { + $container = $this->protectedContainer(); + $container->removeDefinition('security.csrf.token_manager'); + + $this->expectException(LogicException::class); + $this->expectExceptionMessage('requires CSRF protection'); + + (new CsrfProtectionPass())->process($container); + } + + public function testFailsWithoutFormCsrfExtension(): void + { + $container = $this->protectedContainer(); + $container->removeDefinition('form.type_extension.csrf'); + + $this->expectException(LogicException::class); + + (new CsrfProtectionPass())->process($container); + } + + public function testFailsWhenFormCsrfDisabled(): void + { + $container = $this->protectedContainer(); + $container->setParameter('form.type_extension.csrf.enabled', false); + + $this->expectException(LogicException::class); + + (new CsrfProtectionPass())->process($container); + } + + private function protectedContainer(): ContainerBuilder + { + $container = new ContainerBuilder(); + $container->setParameter('nowo_page_layout_kit.security.access_roles', ['ROLE_EDITOR']); + $container->setDefinition('security.csrf.token_manager', new Definition()); + $container->setDefinition('form.type_extension.csrf', new Definition()); + + return $container; + } +} diff --git a/tests/Unit/DependencyInjection/NowoPageLayoutKitExtensionTest.php b/tests/Unit/DependencyInjection/NowoPageLayoutKitExtensionTest.php index 48451cb..0c9fd50 100644 --- a/tests/Unit/DependencyInjection/NowoPageLayoutKitExtensionTest.php +++ b/tests/Unit/DependencyInjection/NowoPageLayoutKitExtensionTest.php @@ -192,6 +192,35 @@ public function testLoadUsesCustomAccessCheckerAliasWhenConfigured(): void self::assertFalse($container->hasDefinition(TablePrefixListener::class)); } + public function testLoadRejectsEmptyAccessRoles(): void + { + $container = new ContainerBuilder(); + $container->setParameter('kernel.bundles', [ + 'SecurityBundle' => SecurityBundle::class, + ]); + + $this->expectException(LogicException::class); + $this->expectExceptionMessage('security.access_roles is empty'); + + (new NowoPageLayoutKitExtension())->load([ + ['security' => ['allow_unauthenticated' => false, 'access_roles' => []]], + ], $container); + } + + public function testLoadAcceptsEmptyAccessRolesWithCustomChecker(): void + { + $container = new ContainerBuilder(); + $container->setParameter('kernel.bundles', [ + 'SecurityBundle' => SecurityBundle::class, + ]); + + (new NowoPageLayoutKitExtension())->load([ + ['security' => ['access_roles' => [], 'access_checker' => 'app.page_layout_checker']], + ], $container); + + self::assertSame('app.page_layout_checker', (string) $container->getAlias(PageLayoutKitAccessCheckerInterface::class)); + } + public function testLoadRegistersDefaultConfigurableAccessCheckerWhenNeeded(): void { $container = new ContainerBuilder(); diff --git a/tests/Unit/NowoPageLayoutKitBundleTest.php b/tests/Unit/NowoPageLayoutKitBundleTest.php index 460aa88..5745e46 100644 --- a/tests/Unit/NowoPageLayoutKitBundleTest.php +++ b/tests/Unit/NowoPageLayoutKitBundleTest.php @@ -5,6 +5,7 @@ namespace Nowo\PageLayoutKitBundle\Tests\Unit; use Doctrine\Bundle\DoctrineBundle\DependencyInjection\Compiler\DoctrineOrmMappingsPass; +use Nowo\PageLayoutKitBundle\DependencyInjection\Compiler\CsrfProtectionPass; use Nowo\PageLayoutKitBundle\DependencyInjection\Compiler\TwigPathsPass; use Nowo\PageLayoutKitBundle\DependencyInjection\NowoPageLayoutKitExtension; use Nowo\PageLayoutKitBundle\Locale\PageLocales; @@ -25,6 +26,7 @@ public function testBuildRegistersTwigAndDoctrineCompilerPasses(): void $passes = $container->getCompilerPassConfig()->getPasses(); self::assertNotEmpty(array_filter($passes, static fn (object $pass): bool => $pass instanceof TwigPathsPass)); + self::assertNotEmpty(array_filter($passes, static fn (object $pass): bool => $pass instanceof CsrfProtectionPass)); self::assertNotEmpty(array_filter($passes, static fn (object $pass): bool => $pass instanceof DoctrineOrmMappingsPass)); } diff --git a/tests/Unit/Security/PageLayoutKitAccessCheckerTest.php b/tests/Unit/Security/PageLayoutKitAccessCheckerTest.php index 6845b8e..3363b5b 100644 --- a/tests/Unit/Security/PageLayoutKitAccessCheckerTest.php +++ b/tests/Unit/Security/PageLayoutKitAccessCheckerTest.php @@ -16,14 +16,14 @@ public function testAllowAllCheckerAlwaysReturnsTrue(): void self::assertTrue((new AllowAllPageLayoutKitAccessChecker())->canAccess()); } - public function testConfigurableCheckerAllowsWhenNoRolesConfigured(): void + public function testConfigurableCheckerDeniesWhenNoRolesConfigured(): void { $authorizationChecker = $this->createMock(AuthorizationCheckerInterface::class); $authorizationChecker->expects(self::never())->method('isGranted'); $checker = new ConfigurablePageLayoutKitAccessChecker($authorizationChecker, []); - self::assertTrue($checker->canAccess()); + self::assertFalse($checker->canAccess()); } public function testConfigurableCheckerReturnsTrueWhenAnyRoleIsGranted(): void