Fix bug where activation of forward/vacation rule could activate a wrong script (#7423)

Also should fix bug where forward/vacation rule could end up being duplicated (#7349)

CHANGELOG

@@ -13,6 +13,8 @@ CHANGELOG Roundcube Webmail
 - Archive: Added options to split archive by year or year+month and folder (#7216)
 - Managesieve: Allow display name with email address in vacation :from field (#6760)
 - Managesieve: Improve UX on custom header input (#7207)
+- Managesieve: Fix bug where activation of forward/vacation rule could activate a wrong script (#7423)
+- Managesieve: Fix bug where forward/vacation rule could end up being duplicated (#7349)
 - Password: Added 'pwned' password strength driver (#7274)
 - Add support for SameSite cookie attribute via session_samesite option (req PHP >= 7.3.0) (#6772)
 - Elastic: Display a special icon for other users and shared namespace roots (#5012)

plugins/managesieve/Changelog

@@ -4,6 +4,8 @@
 - Fix so modifier type select wasn't hidden after hiding modifier select on header change
 - Fix filter selection after removing a first filter (#7079)
 - Fix bug where it wasn't possible to save flag actions (#7188)
+- Fix bug where activation of forward/vacation rule could activate a wrong script (#7423)
+- Fix bug where forward/vacation rule could end up being duplicated (#7349)
 
 * version 9.3 [2019-04-21]
 -----------------------------------------------------------

plugins/managesieve/lib/Roundcube/rcube_sieve_engine.php

@@ -248,25 +248,9 @@ class rcube_sieve_engine
             else if ($list) {
                 $script_name = $list[0];
             }
-            // create a new (initial) script
             else {
-                // if script not exists build default script contents
+                // if script does not exist create one with default content
-                $script_file = $this->rc->config->get('managesieve_default');
+                $this->create_default_script();
-                $script_name = $this->rc->config->get('managesieve_script_name');
-
-                if (empty($script_name)) {
-                    $script_name = 'roundcube';
-                }
-
-                if ($script_file && is_readable($script_file) && !is_dir($script_file)) {
-                    $content = file_get_contents($script_file);
-                }
-
-                // add script and set it active
-                if ($this->sieve->save_script($script_name, $content)) {
-                    $this->activate_script($script_name);
-                    $this->list[] = $script_name;
-                }
             }
         }
 
@@ -3126,4 +3110,135 @@ class rcube_sieve_engine
 
         return $select_comp->show($comparator);
     }
+
+    /**
+     * Merge a rule into the script
+     */
+    protected function merge_rule($rule, $existing, &$script_name = null)
+    {
+        // if script does not exist create a new one
+        if ($script_name === null || $script_name === false) {
+            $script_name = $this->create_default_script();
+            $this->sieve->load($script_name);
+            $this->init_script();
+        }
+
+        if (!$this->sieve->script) {
+            return false;
+        }
+
+        $script_active   = in_array($script_name, $this->active);
+        $rule_active     = empty($rule['disabled']);
+        $activate_script = false;
+
+        // If the script is not active, but the rule is,
+        // put the rule in an active script if there is one
+        if (!$script_active && $rule_active && !empty($this->active)) {
+            // Remove the rule from current (inactive) script
+            if (isset($existing['idx'])) {
+                unset($this->script[$existing['idx']]);
+                $this->sieve->script->content = $this->script;
+                $this->save_script($script_name);
+            }
+
+            // Load and init the active script, add the rule there
+            $this->sieve->load($script_name = $this->active[0]);
+            $this->init_script();
+            array_unshift($this->script, $rule);
+        }
+        // update original forward rule/script
+        else {
+            // re-order rules if needed
+            if (isset($rule['after']) && $rule['after'] !== '') {
+                // unset the original rule
+                if (isset($existing['idx'])) {
+                    $this->script[$existing['idx']] = null;
+                }
+
+                // add at target position
+                if ($rule['after'] >= count($this->script) - 1) {
+                    $this->script[] = $rule;
+                    $this->script = array_values(array_filter($this->script));
+                    $rule_index = count($this->script);
+                }
+                else {
+                    $script = array();
+
+                    foreach ($this->script as $idx => $r) {
+                        if ($r) {
+                            $script[] = $r;
+                        }
+
+                        if ($idx == $rule['after']) {
+                            $script[] = $rule;
+                            $rule_index = count($script);
+                        }
+                    }
+
+                    $this->script = $script;
+                }
+            }
+            // rule exists, update it "in place"
+            else if (isset($existing['idx'])) {
+                $this->script[$existing['idx']] = $rule;
+                $rule_index = $existing['idx'];
+            }
+            // otherwise put the rule on top
+            else {
+                array_unshift($this->script, $rule);
+                $rule_index = 0;
+            }
+
+            // if the script is not active, but the rule is, we need to de-activate
+            // all rules except the forward rule
+            if (!$script_active && $rule_active) {
+                $activate_script = true;
+                foreach ($this->script as $idx => $r) {
+                    if ($idx !== $rule_index) {
+                        $this->script[$idx]['disabled'] = true;
+                    }
+                }
+            }
+        }
+
+        $this->sieve->script->content = $this->script;
+
+        // save the script
+        $saved = $this->save_script($script_name);
+
+        // activate the script
+        if ($saved && $activate_script) {
+            $this->activate_script($script_name);
+        }
+
+        return $saved;
+    }
+
+    /**
+     * Create default script
+     */
+    protected function create_default_script()
+    {
+        // if script not exists build default script contents
+        $script_file  = $this->rc->config->get('managesieve_default');
+        $script_name  = $this->rc->config->get('managesieve_script_name');
+        $kolab_master = $this->rc->config->get('managesieve_kolab_master');
+        $content      = '';
+
+        if (empty($script_name)) {
+            $script_name = 'roundcube';
+        }
+
+        if ($script_file && !$kolab_master && is_readable($script_file) && !is_dir($script_file)) {
+            $content = file_get_contents($script_file);
+        }
+
+        // add script and set it active
+        if ($this->sieve->save_script($script_name, $content)) {
+            $this->activate_script($script_name);
+            $this->list[] = $script_name;
+        }
+
+        return $script_name;
+    }
 }

plugins/managesieve/lib/Roundcube/rcube_sieve_forward.php

@@ -148,7 +148,7 @@ class rcube_sieve_forward extends rcube_sieve_engine
                         $action = 'copy';
                     }
                     else if ($act['type'] == 'stop') {
-                        // we might loose information if there rules after the stop
+                        // we might loose information if there are rules after the stop
                         $stop_found = true;
                     }
                     else if ($act['type'] == 'discard') {
@@ -207,10 +207,8 @@ class rcube_sieve_forward extends rcube_sieve_engine
         $date_extension = in_array('date', $this->exts);
         $forward_tests  = (array) $this->forward['tests'];
 
-        if ($action == 'redirect' || $action == 'copy') {
+        if (empty($target) || !rcube_utils::check_email($target)) {
-            if (empty($target) || !rcube_utils::check_email($target)) {
+            $error = 'noemailwarning';
-                $error = 'noemailwarning';
-            }
         }
 
         if (empty($forward_tests)) {
@@ -224,18 +222,13 @@ class rcube_sieve_forward extends rcube_sieve_engine
             $rule['disabled']   = $status == 'off';
             $rule['tests']      = $forward_tests;
             $rule['join']       = $date_extension ? count($forward_tests) > 1 : false;
-            $rule['actions']    = array();
+            $rule['actions']    = array(array(
-            $rule['after']      = $after;
+                    'type'   => 'redirect',
-
-            if ($action && $action != 'keep') {
-                $rule['actions'][] = array(
-                    'type'   => $action == 'discard' ? 'discard' : 'redirect',
                     'copy'   => $action == 'copy',
-                    'target' => $action != 'discard' ? $target : '',
+                    'target' => $target,
-                );
+            ));
-            }
 
-            if ($this->save_forward_script($rule)) {
+            if ($this->merge_rule($rule, $this->forward, $this->script_name)) {
                 $this->rc->output->show_message('managesieve.forwardsaved', 'confirmation');
                 $this->rc->output->send();
             }
@@ -320,99 +313,6 @@ class rcube_sieve_forward extends rcube_sieve_engine
         return $out;
     }
 
-    /**
-     * Saves forward script (adding some variables)
-     */
-    protected function save_forward_script($rule)
-    {
-        // if script does not exist create a new one
-        if ($this->script_name === null || $this->script_name === false) {
-            $this->script_name = $this->rc->config->get('managesieve_script_name');
-            if (empty($this->script_name)) {
-                $this->script_name = 'roundcube';
-            }
-
-            // use default script contents
-            if (!$this->rc->config->get('managesieve_kolab_master')) {
-                $script_file = $this->rc->config->get('managesieve_default');
-                if ($script_file && is_readable($script_file)) {
-                    $content = file_get_contents($script_file);
-                }
-            }
-
-            // create and load script
-            if ($this->sieve->save_script($this->script_name, $content)) {
-                $this->sieve->load($this->script_name);
-            }
-        }
-
-        $script_active = in_array($this->script_name, $this->active);
-
-        // re-order rules if needed
-        if (isset($rule['after']) && $rule['after'] !== '') {
-            // reset original forward rule
-            if (isset($this->forward['idx'])) {
-                $this->script[$this->forward['idx']] = null;
-            }
-
-            // add at target position
-            if ($rule['after'] >= count($this->script) - 1) {
-                $this->script[] = $rule;
-            }
-            else {
-                $script = array();
-
-                foreach ($this->script as $idx => $r) {
-                    if ($r) {
-                        $script[] = $r;
-                    }
-
-                    if ($idx == $rule['after']) {
-                        $script[] = $rule;
-                    }
-                }
-
-                $this->script = $script;
-            }
-
-            $this->script = array_values(array_filter($this->script));
-        }
-        // update original forward rule if it exists
-        else if (isset($this->forward['idx'])) {
-            $this->script[$this->forward['idx']] = $rule;
-        }
-        // otherwise put forward rule on top
-        else {
-            array_unshift($this->script, $rule);
-        }
-
-        // if the script was not active, we need to de-activate
-        // all rules except the forward rule, but only if it is not disabled
-        if (!$script_active && !$rule['disabled']) {
-            foreach ($this->script as $idx => $r) {
-                if (empty($r['actions']) || $r['actions'][0]['type'] != 'forward') {
-                    $this->script[$idx]['disabled'] = true;
-                }
-            }
-        }
-
-        if (!$this->sieve->script) {
-            return false;
-        }
-
-        $this->sieve->script->content = $this->script;
-
-        // save the script
-        $saved = $this->save_script($this->script_name);
-
-        // activate the script
-        if ($saved && !$script_active && !$rule['disabled']) {
-            $this->activate_script($this->script_name);
-        }
-
-        return $saved;
-    }
-
     /**
      * API: get forward rule
      *
@@ -472,17 +372,13 @@ class rcube_sieve_forward extends rcube_sieve_engine
         $rule['disabled'] = isset($data['enabled']) && !$data['enabled'];
         $rule['tests']    = $forward_tests;
         $rule['join']     = $date_extension ? count($forward_tests) > 1 : false;
-        $rule['actions']  = array();
+        $rule['actions']  = array(array(
-
+                'type'   => 'redirect',
-        if ($data['action'] && $data['action'] != 'keep') {
-            $rule['actions'][] = array(
-                'type'   => $data['action'] == 'discard' ? 'discard' : 'redirect',
                 'copy'   => $data['action'] == 'copy',
-                'target' => $data['action'] != 'discard' ? $data['target'] : '',
+                'target' => $data['target'],
-            );
+        ));
-        }
 
-        return $this->save_forward_script($rule);
+        return $this->merge_rule($rule, $this->forward, $this->script_name);
     }
 
     /**

plugins/managesieve/lib/Roundcube/rcube_sieve_vacation.php

@@ -331,7 +331,7 @@ class rcube_sieve_vacation extends rcube_sieve_engine
                 );
             }
 
-            if ($this->save_vacation_script($rule)) {
+            if ($this->merge_rule($rule, $this->vacation, $this->script_name)) {
                 $this->rc->output->show_message('managesieve.vacationsaved', 'confirmation');
                 $this->rc->output->send();
             }
@@ -408,7 +408,11 @@ class rcube_sieve_vacation extends rcube_sieve_engine
             $action->add($this->plugin->gettext('vacation.copy'), 'copy');
         }
 
-        if ($this->rc->config->get('managesieve_vacation') != 2 && !empty($this->vacation['list'])) {
+        if (
+            $this->rc->config->get('managesieve_vacation') != 2
+            && !empty($this->vacation['list'])
+            && in_array($this->script_name, $this->active)
+        ) {
             $after = new html_select(array('name' => 'vacation_after', 'id' => 'vacation_after'));
 
             $after->add('---', '');
@@ -529,7 +533,7 @@ class rcube_sieve_vacation extends rcube_sieve_engine
         $table->add('title', html::label('vacation_interval', $this->plugin->gettext('vacation.interval')));
         $table->add(null, html::span('input-group', $interval_txt));
 
-        if ($after) {
+        if (!empty($after)) {
             $table->add('title', html::label('vacation_after', $this->plugin->gettext('vacation.after')));
             $table->add(null, $after->show($this->vacation['idx'] - 1));
         }
@@ -645,99 +649,6 @@ class rcube_sieve_vacation extends rcube_sieve_engine
         return $interval ?: '';
     }
 
-    /**
-     * Saves vacation script (adding some variables)
-     */
-    protected function save_vacation_script($rule)
-    {
-        // if script does not exist create a new one
-        if ($this->script_name === null || $this->script_name === false) {
-            $this->script_name = $this->rc->config->get('managesieve_script_name');
-            if (empty($this->script_name)) {
-                $this->script_name = 'roundcube';
-            }
-
-            // use default script contents
-            if (!$this->rc->config->get('managesieve_kolab_master')) {
-                $script_file = $this->rc->config->get('managesieve_default');
-                if ($script_file && is_readable($script_file)) {
-                    $content = file_get_contents($script_file);
-                }
-            }
-
-            // create and load script
-            if ($this->sieve->save_script($this->script_name, $content)) {
-                $this->sieve->load($this->script_name);
-            }
-        }
-
-        $script_active = in_array($this->script_name, $this->active);
-
-        // re-order rules if needed
-        if (isset($rule['after']) && $rule['after'] !== '') {
-            // reset original vacation rule
-            if (isset($this->vacation['idx'])) {
-                $this->script[$this->vacation['idx']] = null;
-            }
-
-            // add at target position
-            if ($rule['after'] >= count($this->script) - 1) {
-                $this->script[] = $rule;
-            }
-            else {
-                $script = array();
-
-                foreach ($this->script as $idx => $r) {
-                    if ($r) {
-                        $script[] = $r;
-                    }
-
-                    if ($idx == $rule['after']) {
-                        $script[] = $rule;
-                    }
-                }
-
-                $this->script = $script;
-            }
-
-            $this->script = array_values(array_filter($this->script));
-        }
-        // update original vacation rule if it exists
-        else if (isset($this->vacation['idx'])) {
-            $this->script[$this->vacation['idx']] = $rule;
-        }
-        // otherwise put vacation rule on top
-        else {
-            array_unshift($this->script, $rule);
-        }
-
-        // if the script was not active, we need to de-activate
-        // all rules except the vacation rule, but only if it is not disabled
-        if (!$script_active && !$rule['disabled']) {
-            foreach ($this->script as $idx => $r) {
-                if (empty($r['actions']) || $r['actions'][0]['type'] != 'vacation') {
-                    $this->script[$idx]['disabled'] = true;
-                }
-            }
-        }
-
-        if (!$this->sieve->script) {
-            return false;
-        }
-
-        $this->sieve->script->content = $this->script;
-
-        // save the script
-        $saved = $this->save_script($this->script_name);
-
-        // activate the script
-        if ($saved && !$script_active && !$rule['disabled']) {
-            $this->activate_script($this->script_name);
-        }
-
-        return $saved;
-    }
-
     /**
      * API: get vacation rule
      *
@@ -937,7 +848,7 @@ class rcube_sieve_vacation extends rcube_sieve_engine
             );
         }
 
-        return $this->save_vacation_script($rule);
+        return $this->merge_rule($rule, $this->vacation, $this->script_name);
     }
 
     /**