From 4ec45373da75d9342b4e68cd9deceb81407afd57 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Rie=C3=9F?= Date: Fri, 3 Jul 2026 14:36:53 +0200 Subject: [PATCH 1/5] Override registerElement for cleaner access to the element values This is mainly done to catch a point before ON_ELEMENT_REGISTERED is called In `ConfigForm`s `ON_ELEMENT_REGISTERED` handler, the early return checked `$element->hasValue(`) in addition to `$key === null`. This meant that if an element already had a value (e.g. a default set by the element itself), the handler would skip seeding it with the config value entirely, and skip setting value candidates. - Dropped the `hasValue()` check from the early-return condition, so the config value is always considered for a keyed element, regardless of any pre-existing default. - On initial render (no value candidates yet), replaced the direct `$element->setValue($configValue)` call with `$this->populate([$element->getName() => $configValue])`, so the value also becomes visible via `getPopulatedValue()`, not just on the element instance itself. --- library/Icinga/Web/Form/ConfigForm.php | 44 +++++++++---------- .../Icinga/Web/Form/ConfigFormTest.php | 13 ++++++ 2 files changed, 34 insertions(+), 23 deletions(-) diff --git a/library/Icinga/Web/Form/ConfigForm.php b/library/Icinga/Web/Form/ConfigForm.php index 510190f73e..92858a1d3c 100644 --- a/library/Icinga/Web/Form/ConfigForm.php +++ b/library/Icinga/Web/Form/ConfigForm.php @@ -8,7 +8,6 @@ use Exception; use Icinga\Application\Config; use ipl\Html\Contract\FormElement; -use ipl\Html\Contract\ValueCandidates; use ipl\Html\HtmlElement; use ipl\Html\ValidHtml; use ipl\Stdlib\Str; @@ -50,34 +49,33 @@ class ConfigForm extends CompatForm public function __construct( protected Config $config, ) { - $this->on(static::ON_ELEMENT_REGISTERED, function (FormElement $element) { - [$section, $key] = Str::symmetricSplit($element->getName(), $this->sectionKeyDelimiter, 2); - if ($key === null || $element->hasValue()) { - return; - } + } + /** + * Register the given element, seeding it with its configuration value first + * + * @param FormElement $element + * + * @return $this + */ + public function registerElement(FormElement $element): static + { + $name = $element->getName(); + [$section, $key] = Str::symmetricSplit($name, $this->sectionKeyDelimiter, 2); + if ($key !== null) { + $submitted = $this->getPopulatedValue($name); + $this->clearPopulatedValue($name); $configValue = $this->config->get($section, $key); - if ($configValue === null) { - return; + if ($configValue !== null) { + $this->populate([$name => $configValue]); } - if (! ($element instanceof ValueCandidates)) { - $element->setValue($configValue); - - return; + if ($submitted !== null) { + $this->populate([$name => $submitted]); } + } - $candidates = $element->getValueCandidates(); - if (empty($candidates)) { - // Initial render: no prior submission, set value and seed candidates - $element->setValue($configValue); - $element->setValueCandidates([$configValue]); - } else { - // POST: candidates were set by registerElement; append config value so - // PasswordElement's (count > 1) condition resolves DUMMYPASSWORD on re-render - $element->setValueCandidates(array_merge($candidates, [$configValue])); - } - }); + return parent::registerElement($element); } /** diff --git a/test/php/library/Icinga/Web/Form/ConfigFormTest.php b/test/php/library/Icinga/Web/Form/ConfigFormTest.php index 65203c3a82..dba5942052 100644 --- a/test/php/library/Icinga/Web/Form/ConfigFormTest.php +++ b/test/php/library/Icinga/Web/Form/ConfigFormTest.php @@ -82,6 +82,19 @@ public function exposeSave(): void $this->assertSame('secret', $config->get('mysection', 'password')); } + public function testElementDefaultIsPreservedWhenConfigKeyIsNotSet(): void + { + $form = new class(Config::fromArray([])) extends ConfigForm { + protected function assemble(): void + { + $this->addElement('text', 'mysection__key', ['value' => 'defaultvalue']); + } + }; + $form->ensureAssembled(); + + $this->assertSame('defaultvalue', $form->getValue('mysection__key')); + } + public function testEmptySectionIsRemovedOnSave(): void { $config = new class(new ConfigObject(['mysection' => ['key' => 'value']])) extends Config { From d7dfa2d9cb0b1bcceb68fbd6b22a6e0e072fa484 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Rie=C3=9F?= Date: Tue, 14 Jul 2026 12:40:14 +0200 Subject: [PATCH 2/5] Clear element when submitted value equals original value --- library/Icinga/Web/Form/ConfigForm.php | 21 +++++- .../Icinga/Web/Form/ConfigFormTest.php | 73 +++++++++++++++++++ 2 files changed, 91 insertions(+), 3 deletions(-) diff --git a/library/Icinga/Web/Form/ConfigForm.php b/library/Icinga/Web/Form/ConfigForm.php index 92858a1d3c..bcb341d2e1 100644 --- a/library/Icinga/Web/Form/ConfigForm.php +++ b/library/Icinga/Web/Form/ConfigForm.php @@ -41,6 +41,16 @@ class ConfigForm extends CompatForm */ protected string $sectionKeyDelimiter = '__'; + /** + * The original values of the form elements + * + * This is used to determine whether an element's submitted value differs from + * the original value. + * + * @var array + */ + protected array $originalValues = []; + /** * Create a new configuration form * @@ -63,14 +73,16 @@ public function registerElement(FormElement $element): static $name = $element->getName(); [$section, $key] = Str::symmetricSplit($name, $this->sectionKeyDelimiter, 2); if ($key !== null) { + $this->originalValues[$name] = $element->getValue(); + $submitted = $this->getPopulatedValue($name); $this->clearPopulatedValue($name); - $configValue = $this->config->get($section, $key); + $configValue = $this->config->get($section, $key, $this->originalValues[$name] ?? null); if ($configValue !== null) { $this->populate([$name => $configValue]); } - if ($submitted !== null) { + if (! Str::isEmpty($submitted)) { $this->populate([$name => $submitted]); } } @@ -158,8 +170,11 @@ protected function save(): void throw new LogicException(sprintf('Cannot save element "%s": array values are not supported', $element)); } + $originalValue = $this->originalValues[$element] ?? ''; $configSection = $this->config->getSection($section); - if (Str::isEmpty($value)) { + if ($originalValue !== '' && Str::isEmpty($value)) { + $configSection[$key] = ''; + } elseif (Str::isEmpty($value) || $originalValue === $value) { unset($configSection[$key]); } else { $configSection[$key] = $value; diff --git a/test/php/library/Icinga/Web/Form/ConfigFormTest.php b/test/php/library/Icinga/Web/Form/ConfigFormTest.php index dba5942052..56ccf9788c 100644 --- a/test/php/library/Icinga/Web/Form/ConfigFormTest.php +++ b/test/php/library/Icinga/Web/Form/ConfigFormTest.php @@ -95,6 +95,79 @@ protected function assemble(): void $this->assertSame('defaultvalue', $form->getValue('mysection__key')); } + public function testElementIsClearedWhenValueIsEmpty(): void + { + $config = new class(new ConfigObject(['mysection' => ['key' => 'value']])) extends Config { + public function saveIni($filePath = null, $fileMode = 0660): void {} + }; + + $form = new class($config) extends ConfigForm { + protected function assemble(): void + { + $this->addElement('text', 'mysection__key', []); + } + + public function exposeSave(): void + { + $this->save(); + } + }; + $form->ensureAssembled(); + $form->populate(['mysection__key' => '']); + $form->exposeSave(); + + $this->assertNull($config->get('mysection', 'key')); + } + + public function testElementIsClearedWhenValueIsOriginalValue(): void + { + $config = new class(new ConfigObject(['mysection' => ['key' => 'value']])) extends Config { + public function saveIni($filePath = null, $fileMode = 0660): void {} + }; + + $form = new class($config) extends ConfigForm { + protected function assemble(): void + { + $this->addElement('text', 'mysection__key', ['value' => 'value']); + } + + public function exposeSave(): void + { + $this->save(); + } + }; + $form->ensureAssembled(); + $form->populate(['mysection__key' => 'value']); + $form->exposeSave(); + + $this->assertNull($config->get('mysection', 'key')); + } + + public function testClearingFieldWithNonEmptyDefaultWritesEmptyStringToConfig(): void + { + $config = new class(new ConfigObject(['mysection' => ['key' => 'value']])) extends Config { + public function saveIni($filePath = null, $fileMode = 0660): void {} + }; + + $form = new class($config) extends ConfigForm { + protected function assemble(): void + { + $this->addElement('text', 'mysection__key', ['value' => 'default']); + } + + public function exposeSave(): void + { + $this->save(); + } + }; + $form->ensureAssembled(); + $form->populate(['mysection__key' => '']); + $form->exposeSave(); + + $this->assertTrue($config->hasSection('mysection')); + $this->assertSame('', $config->get('mysection', 'key', 'not-set')); + } + public function testEmptySectionIsRemovedOnSave(): void { $config = new class(new ConfigObject(['mysection' => ['key' => 'value']])) extends Config { From 2403d60507d7571462624673c4a37c394344df9d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Rie=C3=9F?= Date: Fri, 17 Jul 2026 14:32:18 +0200 Subject: [PATCH 3/5] Prepend config value before any populated value MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The old `registerElement` implementation read the submitted value via `getPopulatedValue`, cleared the queue, pushed the config value, then pushed the submitted value back. This dropped any intermediate populate calls made before assembly and never put the config value into `ValueCandidates`, which broke `PasswordElement`: submitting `DUMMYPASSWORD` could not be resolved back to the stored password because the real value was missing from the candidates list. Since `populatedValues` is now protected on the `FormElements` trait, we can prepend the config value directly to the queue before delegating to `parent::registerElement`. The parent then applies the last queued value as the element's current value and passes the full queue to `setValueCandidates` — which now includes the config value at the front, giving `PasswordElement` the history it needs to resolve dummy passwords correctly. A `stdClass` sentinel replaces `null` as the `Config::get` fallback so that a config key explicitly absent from the file is distinguishable from one that holds a `null` value, avoiding a spurious prepend in that case. --- library/Icinga/Web/Form/ConfigForm.php | 14 +- .../Icinga/Web/Form/ConfigFormTest.php | 147 +++++++++++++++--- 2 files changed, 133 insertions(+), 28 deletions(-) diff --git a/library/Icinga/Web/Form/ConfigForm.php b/library/Icinga/Web/Form/ConfigForm.php index bcb341d2e1..8fee677528 100644 --- a/library/Icinga/Web/Form/ConfigForm.php +++ b/library/Icinga/Web/Form/ConfigForm.php @@ -15,6 +15,7 @@ use ipl\Web\Compat\DisplayFormElement; use ipl\Web\Widget\CopyToClipboard; use LogicException; +use stdClass; /** * Form base-class providing standard functionality for configuration forms @@ -75,15 +76,10 @@ public function registerElement(FormElement $element): static if ($key !== null) { $this->originalValues[$name] = $element->getValue(); - $submitted = $this->getPopulatedValue($name); - $this->clearPopulatedValue($name); - $configValue = $this->config->get($section, $key, $this->originalValues[$name] ?? null); - if ($configValue !== null) { - $this->populate([$name => $configValue]); - } - - if (! Str::isEmpty($submitted)) { - $this->populate([$name => $submitted]); + $sentinel = new stdClass(); + $configValue = $this->config->get($section, $key, $this->originalValues[$name] ?? $sentinel); + if ($configValue !== $sentinel) { + $this->populatedValues[$name] = array_merge([$configValue], $this->populatedValues[$name] ?? []); } } diff --git a/test/php/library/Icinga/Web/Form/ConfigFormTest.php b/test/php/library/Icinga/Web/Form/ConfigFormTest.php index 56ccf9788c..41bedb3cf3 100644 --- a/test/php/library/Icinga/Web/Form/ConfigFormTest.php +++ b/test/php/library/Icinga/Web/Form/ConfigFormTest.php @@ -11,6 +11,7 @@ use Icinga\Web\Form\ConfigForm; use ipl\Html\FormElement\PasswordElement; use LogicException; +use PHPUnit\Framework\Attributes\DataProvider; class ConfigFormTest extends BaseTestCase { @@ -31,7 +32,8 @@ public function testSubmitButtonIsAddedAfterAssembly(): void $this->assertTrue($form->hasElement('store')); } - public function testSaveThrowsForArrayElementValue(): void + #[DataProvider('populationTimings')] + public function testSaveThrowsForArrayElementValue(bool $populateBeforeAssembly): void { $this->expectException(LogicException::class); @@ -53,12 +55,12 @@ public function exposeSave(): void $this->save(); } }; - $form->ensureAssembled(); - $form->populate(['mysection__key' => ['a', 'b']]); + $this->populateAroundAssembly($form, [['mysection__key' => ['a', 'b']]], $populateBeforeAssembly); $form->exposeSave(); } - public function testUnchangedPasswordElementRetainsConfigValueOnSave(): void + #[DataProvider('populationTimings')] + public function testUnchangedPasswordElementRetainsConfigValueOnSave(bool $populateBeforeAssembly): void { $config = new class(new ConfigObject(['mysection' => ['password' => 'secret']])) extends Config { public function saveIni($filePath = null, $fileMode = 0660): void {} @@ -75,8 +77,11 @@ public function exposeSave(): void $this->save(); } }; - $form->populate(['mysection__password' => PasswordElement::DUMMYPASSWORD]); - $form->ensureAssembled(); + $this->populateAroundAssembly( + $form, + [['mysection__password' => PasswordElement::DUMMYPASSWORD]], + $populateBeforeAssembly, + ); $form->exposeSave(); $this->assertSame('secret', $config->get('mysection', 'password')); @@ -93,9 +98,11 @@ protected function assemble(): void $form->ensureAssembled(); $this->assertSame('defaultvalue', $form->getValue('mysection__key')); + $this->assertSame('defaultvalue', $form->getPopulatedValue('mysection__key')); } - public function testElementIsClearedWhenValueIsEmpty(): void + #[DataProvider('populationTimings')] + public function testElementIsClearedWhenValueIsEmpty(bool $populateBeforeAssembly): void { $config = new class(new ConfigObject(['mysection' => ['key' => 'value']])) extends Config { public function saveIni($filePath = null, $fileMode = 0660): void {} @@ -112,14 +119,14 @@ public function exposeSave(): void $this->save(); } }; - $form->ensureAssembled(); - $form->populate(['mysection__key' => '']); + $this->populateAroundAssembly($form, [['mysection__key' => '']], $populateBeforeAssembly); $form->exposeSave(); $this->assertNull($config->get('mysection', 'key')); } - public function testElementIsClearedWhenValueIsOriginalValue(): void + #[DataProvider('populationTimings')] + public function testElementIsClearedWhenValueIsOriginalValue(bool $populateBeforeAssembly): void { $config = new class(new ConfigObject(['mysection' => ['key' => 'value']])) extends Config { public function saveIni($filePath = null, $fileMode = 0660): void {} @@ -136,15 +143,16 @@ public function exposeSave(): void $this->save(); } }; - $form->ensureAssembled(); - $form->populate(['mysection__key' => 'value']); + $this->populateAroundAssembly($form, [['mysection__key' => 'value']], $populateBeforeAssembly); $form->exposeSave(); $this->assertNull($config->get('mysection', 'key')); } - public function testClearingFieldWithNonEmptyDefaultWritesEmptyStringToConfig(): void - { + #[DataProvider('populationTimings')] + public function testClearingFieldWithNonEmptyDefaultWritesEmptyStringToConfig( + bool $populateBeforeAssembly, + ): void { $config = new class(new ConfigObject(['mysection' => ['key' => 'value']])) extends Config { public function saveIni($filePath = null, $fileMode = 0660): void {} }; @@ -160,15 +168,15 @@ public function exposeSave(): void $this->save(); } }; - $form->ensureAssembled(); - $form->populate(['mysection__key' => '']); + $this->populateAroundAssembly($form, [['mysection__key' => '']], $populateBeforeAssembly); $form->exposeSave(); $this->assertTrue($config->hasSection('mysection')); $this->assertSame('', $config->get('mysection', 'key', 'not-set')); } - public function testEmptySectionIsRemovedOnSave(): void + #[DataProvider('populationTimings')] + public function testEmptySectionIsRemovedOnSave(bool $populateBeforeAssembly): void { $config = new class(new ConfigObject(['mysection' => ['key' => 'value']])) extends Config { public function saveIni($filePath = null, $fileMode = 0660): void {} @@ -185,10 +193,111 @@ public function exposeSave(): void $this->save(); } }; - $form->ensureAssembled(); - $form->populate(['mysection__key' => '']); + $this->populateAroundAssembly($form, [['mysection__key' => '']], $populateBeforeAssembly); $form->exposeSave(); $this->assertFalse($config->hasSection('mysection')); } + + #[DataProvider('populationTimings')] + public function testZeroValueIsSaved(bool $populateBeforeAssembly): void + { + $config = new class (new ConfigObject(['mysection' => ['key' => 'value']])) extends Config { + public function saveIni($filePath = null, $fileMode = 0660): void + { + } + }; + + $form = new class ($config) extends ConfigForm { + protected function assemble(): void + { + $this->addElement('text', 'mysection__key'); + } + + public function exposeSave(): void + { + $this->save(); + } + }; + $this->populateAroundAssembly($form, [['mysection__key' => '0']], $populateBeforeAssembly); + $form->exposeSave(); + + $this->assertSame('0', $config->get('mysection', 'key')); + } + + #[DataProvider('populationTimings')] + public function testEveryPopulatedValueIsAppliedInOrder(bool $populateBeforeAssembly): void + { + $config = Config::fromArray(['mysection' => ['key' => 'configured']]); + $form = new class ($config) extends ConfigForm { + protected function assemble(): void + { + $this->addElement('text', 'mysection__key'); + } + }; + $this->populateAroundAssembly( + $form, + [['mysection__key' => ''], ['mysection__key' => '0']], + $populateBeforeAssembly, + ); + + $this->assertSame('0', $form->getValue('mysection__key')); + $this->assertSame('0', $form->getPopulatedValue('mysection__key')); + } + + #[DataProvider('populationTimings')] + public function testNullValueClearsElement(bool $populateBeforeAssembly): void + { + $config = new class (new ConfigObject(['mysection' => ['key' => 'value']])) extends Config { + public function saveIni($filePath = null, $fileMode = 0660): void + { + } + }; + + $form = new class ($config) extends ConfigForm { + protected function assemble(): void + { + $this->addElement('text', 'mysection__key'); + } + + public function exposeSave(): void + { + $this->save(); + } + }; + $this->populateAroundAssembly($form, [['mysection__key' => null]], $populateBeforeAssembly); + $form->exposeSave(); + + $this->assertNull($config->get('mysection', 'key')); + } + + /** @return array */ + public static function populationTimings(): array + { + return [ + 'before assembly' => [true], + 'after assembly' => [false], + ]; + } + + /** + * @param list> $populations + */ + private function populateAroundAssembly( + ConfigForm $form, + array $populations, + bool $populateBeforeAssembly, + ): void { + if (! $populateBeforeAssembly) { + $form->ensureAssembled(); + } + + foreach ($populations as $values) { + $form->populate($values); + } + + if ($populateBeforeAssembly) { + $form->ensureAssembled(); + } + } } From a6c75a7913520901ad61957667b33f4d3a643273 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Rie=C3=9F?= Date: Fri, 17 Jul 2026 14:42:23 +0200 Subject: [PATCH 4/5] Simplify CspConfigForm Use the element's default as the `Config::get` fallback in `ConfigForm` Previously `registerElement` passed no fallback to `Config::get`, so the config value was always `null` when the key was absent. This caused elements with declared defaults to lose their value: the element attribute was set, but the populate queue remained empty, leaving `getPopulatedValue` returning `null`. Passing `$this->originalValues[$name]` as the fallback preserves the element's declared default when no config key exists. The check that decides whether to re-apply the submitted value over the config value is also tightened from `!== null` to `! Str::isEmpty` so that empty and whitespace-only submitted values are not pushed back onto the queue. --- application/controllers/ConfigController.php | 9 --- .../forms/Config/Security/CspConfigForm.php | 24 ++++-- .../Config/Security/CspConfigFormTest.php | 73 +++++++++++++++++++ 3 files changed, 91 insertions(+), 15 deletions(-) create mode 100644 test/php/application/forms/Config/Security/CspConfigFormTest.php diff --git a/application/controllers/ConfigController.php b/application/controllers/ConfigController.php index 1570be4019..12336675e2 100644 --- a/application/controllers/ConfigController.php +++ b/application/controllers/ConfigController.php @@ -134,15 +134,6 @@ public function securityAction(): void $config = Config::app(); $cspForm = new CspConfigForm($config); - // Keep the auto-generation options off by default for installations that already - // had CSP enabled, so their existing behavior isn't changed. For installations - // enabling CSP for the first time, default them on as the recommended setting. - $defaultValue = $cspForm->isCspEnabled() ? '0' : '1'; - $cspForm->populate([ - 'security__csp_enable_modules' => $config->get('security', 'csp_enable_modules', $defaultValue), - 'security__csp_enable_dashboards' => $config->get('security', 'csp_enable_dashboards', $defaultValue), - 'security__csp_enable_navigation' => $config->get('security', 'csp_enable_navigation', $defaultValue), - ]); $cspForm->on(ContractForm::ON_SUBMIT, function (CspConfigForm $form) { if ($form->hasConfigChanged()) { diff --git a/application/forms/Config/Security/CspConfigForm.php b/application/forms/Config/Security/CspConfigForm.php index 30c4dd9605..66844ca477 100644 --- a/application/forms/Config/Security/CspConfigForm.php +++ b/application/forms/Config/Security/CspConfigForm.php @@ -23,6 +23,7 @@ use ipl\Html\HtmlElement; use ipl\Html\Table; use ipl\Html\Text; +use ipl\Stdlib\Str; use ipl\Validator\CallbackValidator; use ipl\Web\Common\CalloutType; use ipl\Web\Common\Csp as CspInstance; @@ -194,7 +195,7 @@ function (StaticCspReason $reason, string $directive, string $expression) { 'Should module defined CSP directives be enabled?' . ' Modules can define or change csp directives at any point.', ), - 'security__csp_enable_modules', + 'csp_enable_modules', ! $useCustomCsp, ); @@ -219,7 +220,7 @@ function (ModuleCspReason $reason, string $directive, string $expression) { . ' header that is sent to the user will only contain the subset of directives that actually' . ' matters to them.', ), - 'security__csp_enable_dashboards', + 'csp_enable_dashboards', ! $useCustomCsp, ); @@ -252,7 +253,7 @@ function (DashboardCspReason $reason, string $directive, string $expression) { . ' all users. The actual header that is sent to the user will only contain the subset of' . ' directives that actually matters to them.', ), - 'security__csp_enable_navigation', + 'csp_enable_navigation', ! $useCustomCsp, ); @@ -307,6 +308,7 @@ function (NavigationCspReason $reason, string $directive, string $expression) { 'class' => 'autosubmit csp-form-content-aligned csp-label-header-h3 csp-form-header', 'checkedValue' => '1', 'uncheckedValue' => '0', + 'value' => '0', ], ); @@ -399,7 +401,7 @@ public function isCustomCspEnabled(): bool * * @param string $label The label of the checkbox * @param string $description The description of the checkbox - * @param string $field The name of the checkbox field + * @param string $key The key of the checkbox field in the config * @param bool $enabled Whether the checkbox should be checked and enabled * * @return void @@ -407,7 +409,7 @@ public function isCustomCspEnabled(): bool protected function addDirectiveCheckboxElement( string $label, string $description, - string $field, + string $key, bool $enabled, ): void { $classList = [ @@ -420,6 +422,17 @@ protected function addDirectiveCheckboxElement( $classList[] = 'csp-disabled'; } + $field = 'security' . $this->sectionKeyDelimiter . $key; + + // Keep the auto-generation options off by default for installations that already + // had CSP enabled, so their existing behavior isn't changed. For installations + // enabling CSP for the first time, default them on as the recommended setting. + $cspWasEnabled = $this->config->get('security', 'use_strict_csp') === '1'; + $defaultValue = $cspWasEnabled ? '0' : '1'; + if (Str::isEmpty($this->getPopulatedValue($field)) && $this->config->get('security', $key) === null) { + $this->populate([$field => $defaultValue]); + } + $this->addElement('checkbox', $field, [ 'label' => $label, 'description' => $description, @@ -427,7 +440,6 @@ protected function addDirectiveCheckboxElement( 'checkedValue' => '1', 'uncheckedValue' => '0', 'disabled' => ! $enabled, - 'value' => $this->getPopulatedValue($field), ]); } diff --git a/test/php/application/forms/Config/Security/CspConfigFormTest.php b/test/php/application/forms/Config/Security/CspConfigFormTest.php new file mode 100644 index 0000000000..bd0b831d33 --- /dev/null +++ b/test/php/application/forms/Config/Security/CspConfigFormTest.php @@ -0,0 +1,73 @@ + +// SPDX-License-Identifier: GPL-3.0-or-later + +namespace Tests\Icinga\Forms\Config\Security; + +use Icinga\Application\Config; +use Icinga\Forms\Config\Security\CspConfigForm; +use Icinga\Test\BaseTestCase; + +class CspConfigFormTest extends BaseTestCase +{ + public function testDirectiveCheckboxesDefaultOnWhenStrictCspIsUnconfigured(): void + { + $this->assertDirectiveDefaults([], '1'); + } + + public function testDirectiveCheckboxesDefaultOnWhenStrictCspWasDisabled(): void + { + $this->assertDirectiveDefaults(['security' => ['use_strict_csp' => '0']], '1'); + } + + public function testDirectiveCheckboxesDefaultOffWhenStrictCspWasEnabled(): void + { + $this->assertDirectiveDefaults(['security' => ['use_strict_csp' => '1']], '0'); + } + + public function testConfiguredDirectiveCheckboxValuesOverrideDefaults(): void + { + $form = $this->createDirectiveForm([ + 'security' => [ + 'use_strict_csp' => '1', + 'csp_enable_modules' => '1', + 'csp_enable_dashboards' => '0', + 'csp_enable_navigation' => '1', + ], + ]); + + $this->assertSame('1', $form->getValue('security__csp_enable_modules')); + $this->assertSame('0', $form->getValue('security__csp_enable_dashboards')); + $this->assertSame('1', $form->getValue('security__csp_enable_navigation')); + } + + private function assertDirectiveDefaults(array $configData, string $expected): void + { + $form = $this->createDirectiveForm($configData); + + foreach (['csp_enable_modules', 'csp_enable_dashboards', 'csp_enable_navigation'] as $key) { + $this->assertSame($expected, $form->getValue('security__' . $key), $key); + } + } + + private function createDirectiveForm(array $configData): CspConfigForm + { + $form = new class (Config::fromArray($configData)) extends CspConfigForm { + protected function assemble(): void + { + } + + public function addDirectiveCheckboxForTest(string $key): void + { + $this->addDirectiveCheckboxElement($key, $key, $key, true); + } + }; + + foreach (['csp_enable_modules', 'csp_enable_dashboards', 'csp_enable_navigation'] as $key) { + $form->addDirectiveCheckboxForTest($key); + } + + return $form; + } +} From 55029797d63779d351e39dc6fc065e315facdeee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Rie=C3=9F?= Date: Wed, 15 Jul 2026 13:27:33 +0200 Subject: [PATCH 5/5] Move setting the CSRF countermeasure id into the controller --- application/controllers/ConfigController.php | 18 +++++++++--------- .../forms/Config/Security/CspConfigForm.php | 3 +-- 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/application/controllers/ConfigController.php b/application/controllers/ConfigController.php index 12336675e2..9996ec0506 100644 --- a/application/controllers/ConfigController.php +++ b/application/controllers/ConfigController.php @@ -8,6 +8,7 @@ use Exception; use GuzzleHttp\Psr7\ServerRequest; use Icinga\Application\Version; +use Icinga\Web\Session; use InvalidArgumentException; use Icinga\Application\Config; use Icinga\Application\Icinga; @@ -132,15 +133,14 @@ public function securityAction(): void $this->view->title = $this->translate('Security'); - $config = Config::app(); - $cspForm = new CspConfigForm($config); - - $cspForm->on(ContractForm::ON_SUBMIT, function (CspConfigForm $form) { - if ($form->hasConfigChanged()) { - $this->getResponse()->setReloadWindow(true); - } - }); - $cspForm->handleRequest(ServerRequest::fromGlobals()); + $cspForm = (new CspConfigForm(Config::app())) + ->setCsrfCounterMeasureId(Session::getSession()->getId()) + ->on(ContractForm::ON_SUBMIT, function (CspConfigForm $form) { + if ($form->hasConfigChanged()) { + $this->getResponse()->setReloadWindow(true); + } + }) + ->handleRequest(ServerRequest::fromGlobals()); $this->view->cspForm = $cspForm; $this->createApplicationTabs()->activate('security'); } diff --git a/application/forms/Config/Security/CspConfigForm.php b/application/forms/Config/Security/CspConfigForm.php index 66844ca477..ea148bfbc7 100644 --- a/application/forms/Config/Security/CspConfigForm.php +++ b/application/forms/Config/Security/CspConfigForm.php @@ -17,7 +17,6 @@ use Icinga\Security\Csp\Reason\StaticCspReason; use Icinga\Util\Csp; use Icinga\Web\Form\ConfigForm; -use Icinga\Web\Session; use ipl\Html\Attributes; use ipl\Html\BaseHtmlElement; use ipl\Html\HtmlElement; @@ -131,7 +130,7 @@ protected function assemble(): void $this->addElement($this->createUidElement()); - $this->addCsrfCounterMeasure(Session::getSession()->getId()); + $this->addCsrfCounterMeasure(); $this->addElement('checkbox', 'security__use_strict_csp', [ 'label' => $this->translate('Send CSP header'),