Do not perform deletion before user input is validated in `FieldConfi…

…gForm` and `TemplateConfigForm` This fixes susceptibility to CSRF attacks.
Icinga · Jul 3, 2023 · 7f0c53b · 7f0c53b
1 parent 59c05fa
commit 7f0c53b
Show file tree

Hide file tree

Showing 2 changed files with 58 additions and 30 deletions.
diff --git a/application/forms/Config/FieldConfigForm.php b/application/forms/Config/FieldConfigForm.php
@@ -31,9 +31,6 @@ class FieldConfigForm extends CompatForm
     /** @var string */
     protected $templateName;
 
-    /** @var bool Hack used for delete button */
-    protected $callOnSuccess;
-
     /** @var string */
     protected $fieldId;
 
@@ -47,8 +44,9 @@ public function __construct(RestApi $jira, string $templateName, $fieldId = null
         $this->templateName = $templateName;
 
         if ($fieldId !== null) {
+            // obtain field key in case the fieldId is field label
             if (! array_key_exists($fieldId, $this->fields)) {
-                $this->fieldId = array_search($fieldId, $this->fields);
+                $this->fieldId = array_search($fieldId, $this->fields) ?: $fieldId;
             } else {
                 $this->fieldId = $fieldId;
             }
@@ -118,6 +116,7 @@ protected function assemble()
                     'Callback' => function ($value, $validator) {
                         /** @var CallbackValidator $validator */
                         $templateFieldKeys = $this->templateConfig->getSection($this->templateName)->keys();
+
                         $selected = $this->fields[$value];
 
                         if (
@@ -337,23 +336,31 @@ protected function assemble()
             $this->getElement('submit')
                 ->getWrapper()
                 ->prepend($deleteButton);
+        }
+    }
 
-            if ($deleteButton->hasBeenPressed()) {
-                $templateFields =  $this->templateConfig->getSection($this->templateName)->toArray();
-
-                $field = isset($templateFields[$this->fieldId]) ? $this->fieldId : $this->fields[$this->fieldId];
-
-                unset($templateFields[$field]);
+    public function hasBeenSubmitted()
+    {
+        if ($this->getPressedSubmitElement() !== null && $this->getPressedSubmitElement()->getName() === 'delete') {
+            return true;
+        }
 
-                $this->templateConfig->setSection($this->templateName, $templateFields);
-                $this->templateConfig->saveIni();
-                $this->getSubmitButton()->setValue($this->getSubmitButton()->getButtonLabel());
+        return parent::hasBeenSubmitted();
+    }
 
-                $this->callOnSuccess = false;
+    public function isValid()
+    {
+        if ($this->getPressedSubmitElement()->getName() === 'delete') {
+            $csrfElement = $this->getElement('CSRFToken');
 
-                return;
+            if (! $csrfElement->isValid()) {
+                return false;
             }
+
+            return true;
         }
+
+        return parent::isValid();
     }
 
     /**
@@ -375,8 +382,15 @@ public function optionalEnum($enum, $nullLabel = null)
 
     public function onSuccess()
     {
-        if ($this->callOnSuccess === false) {
-            $this->getPressedSubmitElement()->setValue($this->getElement('delete')->getLabel());
+        if ($this->getPressedSubmitElement()->getName() === 'delete') {
+            $templateFields =  $this->templateConfig->getSection($this->templateName)->toArray();
+
+            $field = isset($templateFields[$this->fieldId]) ? $this->fieldId : $this->fields[$this->fieldId];
+
+            unset($templateFields[$field]);
+
+            $this->templateConfig->setSection($this->templateName, $templateFields);
+            $this->templateConfig->saveIni();
 
             return;
         }

diff --git a/application/forms/Config/TemplateConfigForm.php b/application/forms/Config/TemplateConfigForm.php
@@ -21,9 +21,6 @@ class TemplateConfigForm extends CompatForm
     /** @var string|null */
     protected $templateName;
 
-    /** @var bool Hack used for delete button */
-    protected $callOnSuccess;
-
     public function __construct($templateName = null)
     {
         $this->config = Config::module('jira', 'templates');
@@ -84,25 +81,42 @@ protected function assemble()
             );
 
             $this->registerElement($deleteButton);
-            $this->getElement('submit')->getWrapper()->prepend($deleteButton);
+            $this->getElement('submit')
+                ->getWrapper()
+                ->prepend($deleteButton);
+        }
+    }
 
-            if ($deleteButton->hasBeenPressed()) {
-                $this->config->removeSection($this->templateName);
-                $this->config->saveIni();
+    public function hasBeenSubmitted()
+    {
+        if ($this->getPressedSubmitElement() !== null && $this->getPressedSubmitElement()->getName() === 'delete') {
+            return true;
+        }
 
-                // Stupid cheat because ipl/html is not capable of multiple submit buttons
-                $this->getSubmitButton()->setValue($this->getSubmitButton()->getButtonLabel());
-                $this->callOnSuccess = false;
+        return parent::hasBeenSubmitted();
+    }
 
-                return;
+    public function isValid()
+    {
+        if ($this->getPressedSubmitElement()->getName() === 'delete') {
+            $csrfElement = $this->getElement('CSRFToken');
+
+            if (! $csrfElement->isValid()) {
+                return false;
             }
+
+            return true;
         }
+
+        return parent::isValid();
     }
 
     public function onSuccess()
     {
-        if ($this->callOnSuccess === false) {
-            $this->getSubmitButton()->setValue($this->getElement('delete')->getButtonLabel());
+        if ($this->getPressedSubmitElement()->getName() === 'delete') {
+            $this->config->removeSection($this->templateName);
+            $this->config->saveIni();
+
             return;
         }