CVE-2021-3900: Cross-Site Request Forgery (CSRF) (CWE-352)
value: firefly-iii is vulnerable to Cross-Site Request Forgery (CSRF)
CWE description: The web application does not, or can not, sufficiently verify whether a well-formed, valid, consistent request was intentionally provided by the user who submitted the request.
CVSS v3 base score: 6.5
Published: 2021-10-27T18:15Z
Repository: https://github.com/firefly-iii/firefly-iii
Language: PHP
Files changed: app/Http/Controllers/Rule/CreateController.php, app/Http/Controllers/RuleGroup/EditController.php, app/Repositories/RuleGroup/RuleGroupRepository.php, public/v1/js/ff/rules/index.js, resources/views/v1/rules/index.twig, routes/web.php
Commit date: 2021-10-23 09:29:07 +0200
Commit message: Catch CSRF issues

Diff: diff --git a/app/Http/Controllers/Rule/CreateController.php b/app/Http/Controllers/Rule/CreateController.php
index c9d2173d277..e0915c010e8 100644
--- a/app/Http/Controllers/Rule/CreateController.php
+++ b/app/Http/Controllers/Rule/CreateController.php
@@ -32,10 +32,11 @@
 use FireflyIII\Repositories\Rule\RuleRepositoryInterface;
 use FireflyIII\Support\Http\Controllers\ModelInformation;
 use FireflyIII\Support\Http\Controllers\RuleManagement;
 use FireflyIII\Support\Search\SearchInterface;
 use Illuminate\Contracts\View\Factory;
+use Illuminate\Http\JsonResponse;
 use Illuminate\Http\RedirectResponse;
 use Illuminate\Http\Request;
 use Illuminate\Routing\Redirector;
 use Illuminate\View\View;
 
@@ -235,19 +236,21 @@ public function createFromJournal(Request $request, TransactionJournal $journal)
     }
 
     /**
      * @param Rule $rule
      *
-     * @return RedirectResponse
+     * @return JsonResponse
      */
-    public function duplicate(Rule $rule): RedirectResponse
+    public function duplicate(Request $request): JsonResponse
     {
-        $newRule = $this->ruleRepos->duplicate($rule);
-
-        session()->flash('success', trans('firefly.duplicated_rule', ['title' => $rule->title, 'newTitle' => $newRule->title]));
+        $ruleId = (int)$request->get('id');
+        $rule   = $this->ruleRepos->find($ruleId);
+        if (null !== $rule) {
+            $this->ruleRepos->duplicate($rule);
+        }
 
-        return redirect(route('rules.index'));
+        return new JsonResponse(['OK']);
     }
 
     /**
      * Store the new rule.
      *
diff --git a/app/Http/Controllers/RuleGroup/EditController.php b/app/Http/Controllers/RuleGroup/EditController.php
index 0a962da6ee2..9a0532fbb94 100644
--- a/app/Http/Controllers/RuleGroup/EditController.php
+++ b/app/Http/Controllers/RuleGroup/EditController.php
@@ -26,10 +26,11 @@
 use FireflyIII\Http\Controllers\Controller;
 use FireflyIII\Http\Requests\RuleGroupFormRequest;
 use FireflyIII\Models\RuleGroup;
 use FireflyIII\Repositories\RuleGroup\RuleGroupRepositoryInterface;
 use Illuminate\Contracts\View\Factory;
+use Illuminate\Http\JsonResponse;
 use Illuminate\Http\RedirectResponse;
 use Illuminate\Http\Request;
 use Illuminate\Routing\Redirector;
 use Illuminate\View\View;
 
@@ -60,28 +61,42 @@ function ($request, $next) {
             }
         );
     }
 
     /**
-     * Move a rule group down.
+     * Move a rule group in either direction.
      *
-     * @param RuleGroup $ruleGroup
+     * @param Request $request
      *
-     * @return RedirectResponse|Redirector
+     * @return JsonResponse
      */
-    public function down(RuleGroup $ruleGroup)
+    public function moveGroup(Request $request): JsonResponse
     {
-        $maxOrder = $this->repository->maxOrder();
-        $order    = (int)$ruleGroup->order;
-        if ($order < $maxOrder) {
-            $newOrder = $order + 1;
-            $this->repository->setOrder($ruleGroup, $newOrder);
+        $groupId = (int)$request->get('id');
+        $ruleGroup= $this->repository->find($groupId);
+        if(null !== $ruleGroup) {
+            $direction = $request->get('direction');
+            if('down' === $direction) {
+                $maxOrder = $this->repository->maxOrder();
+                $order    = (int)$ruleGroup->order;
+                if ($order < $maxOrder) {
+                    $newOrder = $order + 1;
+                    $this->repository->setOrder($ruleGroup, $newOrder);
+                }
+            }
+            if('up' === $direction) {
+                $order = (int)$ruleGroup->order;
+                if ($order > 1) {
+                    $newOrder = $order - 1;
+                    $this->repository->setOrder($ruleGroup, $newOrder);
+                }
+            }
         }
-
-        return redirect(route('rules.index'));
+        return new JsonResponse(['OK']);
     }
 
+
     /**
      * Edit a rule group.
      *
      * @param Request   $request
      * @param RuleGroup $ruleGroup
@@ -104,29 +119,10 @@ public function edit(Request $request, RuleGroup $ruleGroup)
         session()->flash('preFilled', $preFilled);
 
         return prefixView('rules.rule-group.edit', compact('ruleGroup', 'subTitle'));
     }
 
-    /**
-     * Move the rule group up.
-     *
-     * @param RuleGroup $ruleGroup
-     *
-     * @return RedirectResponse|Redirector
-     *
-     */
-    public function up(RuleGroup $ruleGroup)
-    {
-        $order = (int)$ruleGroup->order;
-        if ($order > 1) {
-            $newOrder = $order - 1;
-            $this->repository->setOrder($ruleGroup, $newOrder);
-        }
-
-        return redirect(route('rules.index'));
-    }
-
     /**
      * Update the rule group.
      *
      * @param RuleGroupFormRequest $request
      * @param RuleGroup            $ruleGroup
diff --git a/app/Repositories/RuleGroup/RuleGroupRepository.php b/app/Repositories/RuleGroup/RuleGroupRepository.php
index f4b76585b61..9bdb1f4d330 100644
--- a/app/Repositories/RuleGroup/RuleGroupRepository.php
+++ b/app/Repositories/RuleGroup/RuleGroupRepository.php
@@ -327,14 +327,12 @@ public function maxOrder(): int
     /**
      * @return bool
      */
     public function resetOrder(): bool
     {
-        $this->user->ruleGroups()->where('active', false)->update(['order' => 0]);
         $set   = $this->user
             ->ruleGroups()
-            ->where('active', true)
             ->whereNull('deleted_at')
             ->orderBy('order', 'ASC')
             ->orderBy('title', 'DESC')
             ->get();
         $count = 1;
@@ -361,11 +359,10 @@ public function resetOrder(): bool
      */
     public function resetRuleOrder(RuleGroup $ruleGroup): bool
     {
         $set   = $ruleGroup->rules()
                            ->orderBy('order', 'ASC')
-                           ->where('active', true)
                            ->orderBy('title', 'DESC')
                            ->orderBy('updated_at', 'DESC')
                            ->get(['rules.*']);
         $count = 1;
         /** @var Rule $entry */
diff --git a/public/v1/js/ff/rules/index.js b/public/v1/js/ff/rules/index.js
index 6caf69298c6..cae9c524bc4 100644
--- a/public/v1/js/ff/rules/index.js
+++ b/public/v1/js/ff/rules/index.js
@@ -57,10 +57,36 @@ function readCookie(name) {
         }
     }
     return null;
 }
 
+function moveRuleGroup(e) {
+    let box = $(e.currentTarget);
+    var direction = box.data('direction');
+    var groupId = box.data('id');
+
+    $.post(moveRuleGroupUrl, {_token: token, direction: direction, id: groupId}).then(function () {
+        location.reload();
+    }).fail(function() {
+        alert('I failed :(');
+    });
+
+    return false;
+}
+
+function duplicateRule(e) {
+    let box = $(e.currentTarget);
+    var ruleId = box.data('id');
+    $.post(duplicateRuleUrl, {_token: token, id: ruleId}).then(function () {
+        location.reload();
+    }).fail(function() {
+        alert('I failed :(');
+    });
+
+    return false;
+}
+
 
 $(function () {
       "use strict";
       $('.group-rules').find('tbody').sortable(
           {
@@ -69,10 +95,13 @@ $(function () {
               handle: '.rule-handle',
               cursor: "move"
           }
       );
 
+      $('.move-group').click(moveRuleGroup);
+      $('.duplicate-rule').click(duplicateRule);
+
       $('.rules-box').each(function (i, v) {
           var box = $(v);
           var groupId = box.data('group');
           var cookieName = 'rule-box-collapse-' + groupId;
           if ('collapsed' === readCookie(cookieName)) {
diff --git a/resources/views/v1/rules/index.twig b/resources/views/v1/rules/index.twig
index e39e18fa0a8..4e66394ae8b 100644
--- a/resources/views/v1/rules/index.twig
+++ b/resources/views/v1/rules/index.twig
@@ -43,15 +43,15 @@
                                     <li><a href="{{ route('rule-groups.delete',ruleGroup.id) }}"><span class="fa fa-fw fa-trash"></span> {{ 'delete'|_ }}</a></li>
                                     <li><a href="{{ route('rule-groups.select-transactions',ruleGroup.id) }}"><span
                                                     class="fa fa-fw fa-power-off"></span> {{ trans('firefly.apply_rule_group_selection', {title: ruleGroup.title}) }}
                                         </a></li>
                                     {% if ruleGroup.order > 1 %}
-                                        <li><a href="{{ route('rule-groups.up',ruleGroup.id) }}"><span
+                                        <li><a href="#" class="move-group" data-direction="up" data-id="{{ ruleGroup.id }}"><span
                                                         class="fa fa-fw fa-arrow-up"></span> {{ 'move_rule_group_up'|_ }}</a></li>
                                     {% endif %}
                                     {% if ruleGroup.order < ruleGroups|length %}
-                                        <li><a href="{{ route('rule-groups.down',ruleGroup.id) }}"><span
+                                        <li><a href="#" class="move-group" data-direction="down" data-id="{{ ruleGroup.id }}"><span
                                                         class="fa fa-fw fa-arrow-down"></span> {{ 'move_rule_group_down'|_ }}
                                             </a></li>
                                     {% endif %}
                                 </ul>
                             </div>
@@ -103,11 +103,11 @@
                                                     {# actually execute rule #}
                                                 <a href="{{ route('rules.select-transactions',rule.id) }}" class="btn btn-default" title=" {{ trans('firefly.apply_rule_selection', {title: rule.title}) }}"><span class="fa fa-fw fa-power-off "></span></a>
                                                 {% endif %}
 
                                                 {# duplicate rule #}
-                                                <a href="{{ route('rules.duplicate',rule.id) }}" class="btn btn-default" title=" {{ trans('firefly.duplicate_rule', {title: rule.title}) }}"><span class="fa fa-fw fa-copy"></span></a>
+                                                <a href="#" class="btn btn-default duplicate-rule" data-id="{{ rule.id }}" title=" {{ trans('firefly.duplicate_rule', {title: rule.title}) }}"><span class="fa fa-fw fa-copy"></span></a>
                                             </div>
                                         </td>
                                         <td class="markdown">
                                             {% if rule.active %}
                                                 {{ rule.title }}
@@ -193,8 +193,12 @@
         </div>
     </div>
 
 {% endblock %}
 {% block scripts %}
+    <script type="text/javascript" nonce="{{ JS_NONCE }}">
+        var moveRuleGroupUrl = '{{ route('rule-groups.move') }}';
+        var duplicateRuleUrl = '{{ route('rules.duplicate') }}';
+    </script>
     <script type="text/javascript" src="v1/js/lib/jquery-ui.min.js?v={{ FF_VERSION }}" nonce="{{ JS_NONCE }}"></script>
     <script type="text/javascript" src="v1/js/ff/rules/index.js?v={{ FF_VERSION }}" nonce="{{ JS_NONCE }}"></script>
 {% endblock %}
diff --git a/routes/web.php b/routes/web.php
index 51fb1ca59f6..9a0473a0bb5 100644
--- a/routes/web.php
+++ b/routes/web.php
@@ -915,11 +915,11 @@ static function () {
     // create controller
         Route::get('create/{ruleGroup?}', ['uses' => 'Rule\CreateController@create', 'as' => 'create']);
         Route::get('create-from-bill/{bill}', ['uses' => 'Rule\CreateController@createFromBill', 'as' => 'create-from-bill']);
         Route::get('create-from-journal/{tj}', ['uses' => 'Rule\CreateController@createFromJournal', 'as' => 'create-from-journal']);
         Route::post('store', ['uses' => 'Rule\CreateController@store', 'as' => 'store']);
-        Route::get('duplicate/{rule}', ['uses' => 'Rule\CreateController@duplicate', 'as' => 'duplicate']);
+        Route::post('duplicate', ['uses' => 'Rule\CreateController@duplicate', 'as' => 'duplicate']);
 
         // delete controller
         Route::get('delete/{rule}', ['uses' => 'Rule\DeleteController@delete', 'as' => 'delete']);
         Route::post('destroy/{rule}', ['uses' => 'Rule\DeleteController@destroy', 'as' => 'destroy']);
 
@@ -947,14 +947,15 @@ static function () {
     ['middleware' => 'user-full-auth', 'namespace' => 'FireflyIII\Http\Controllers', 'prefix' => 'rule-groups', 'as' => 'rule-groups.'],
     static function () {
         Route::get('create', ['uses' => 'RuleGroup\CreateController@create', 'as' => 'create']);
         Route::get('edit/{ruleGroup}', ['uses' => 'RuleGroup\EditController@edit', 'as' => 'edit']);
         Route::get('delete/{ruleGroup}', ['uses' => 'RuleGroup\DeleteController@delete', 'as' => 'delete']);
-        Route::get('up/{ruleGroup}', ['uses' => 'RuleGroup\EditController@up', 'as' => 'up']);
-        Route::get('down/{ruleGroup}', ['uses' => 'RuleGroup\EditController@down', 'as' => 'down']);
-        Route::get('select/{ruleGroup}', ['uses' => 'RuleGroup\ExecutionController@selectTransactions', 'as' => 'select-transactions']);
 
+        // new route to move rule groups:
+        Route::post('move', ['uses' => 'RuleGroup\EditController@moveGroup', 'as' => 'move']);
+
+        Route::get('select/{ruleGroup}', ['uses' => 'RuleGroup\ExecutionController@selectTransactions', 'as' => 'select-transactions']);
         Route::post('store', ['uses' => 'RuleGroup\CreateController@store', 'as' => 'store']);
         Route::post('update/{ruleGroup}', ['uses' => 'RuleGroup\EditController@update', 'as' => 'update']);
         Route::post('destroy/{ruleGroup}', ['uses' => 'RuleGroup\DeleteController@destroy', 'as' => 'destroy']);
         Route::post('execute/{ruleGroup}', ['uses' => 'RuleGroup\ExecutionController@execute', 'as' => 'execute']);
     }

Vulnerable code:      * @return RedirectResponse
    public function duplicate(Rule $rule): RedirectResponse
        $newRule = $this->ruleRepos->duplicate($rule);

        session()->flash('success', trans('firefly.duplicated_rule', ['title' => $rule->title, 'newTitle' => $newRule->title]));
        return redirect(route('rules.index'));
     * Move a rule group down.
     * @param RuleGroup $ruleGroup
     * @return RedirectResponse|Redirector
    public function down(RuleGroup $ruleGroup)
        $maxOrder = $this->repository->maxOrder();
        $order    = (int)$ruleGroup->order;
        if ($order < $maxOrder) {
            $newOrder = $order + 1;
            $this->repository->setOrder($ruleGroup, $newOrder);

        return redirect(route('rules.index'));
    /**
     * Move the rule group up.
     *
     * @param RuleGroup $ruleGroup
     *
     * @return RedirectResponse|Redirector
     *
     */
    public function up(RuleGroup $ruleGroup)
    {
        $order = (int)$ruleGroup->order;
        if ($order > 1) {
            $newOrder = $order - 1;
            $this->repository->setOrder($ruleGroup, $newOrder);
        }

        return redirect(route('rules.index'));
    }

        $this->user->ruleGroups()->where('active', false)->update(['order' => 0]);
            ->where('active', true)
                           ->where('active', true)
                                        <li><a href="{{ route('rule-groups.up',ruleGroup.id) }}"><span
                                        <li><a href="{{ route('rule-groups.down',ruleGroup.id) }}"><span
                                                <a href="{{ route('rules.duplicate',rule.id) }}" class="btn btn-default" title=" {{ trans('firefly.duplicate_rule', {title: rule.title}) }}"><span class="fa fa-fw fa-copy"></span></a>
        Route::get('duplicate/{rule}', ['uses' => 'Rule\CreateController@duplicate', 'as' => 'duplicate']);
        Route::get('up/{ruleGroup}', ['uses' => 'RuleGroup\EditController@up', 'as' => 'up']);
        Route::get('down/{ruleGroup}', ['uses' => 'RuleGroup\EditController@down', 'as' => 'down']);
        Route::get('select/{ruleGroup}', ['uses' => 'RuleGroup\ExecutionController@selectTransactions', 'as' => 'select-transactions']);
Fixed code: use Illuminate\Http\JsonResponse;
     * @return JsonResponse
    public function duplicate(Request $request): JsonResponse
        $ruleId = (int)$request->get('id');
        $rule   = $this->ruleRepos->find($ruleId);
        if (null !== $rule) {
            $this->ruleRepos->duplicate($rule);
        }
        return new JsonResponse(['OK']);
use Illuminate\Http\JsonResponse;
     * Move a rule group in either direction.
     * @param Request $request
     * @return JsonResponse
    public function moveGroup(Request $request): JsonResponse
        $groupId = (int)$request->get('id');
        $ruleGroup= $this->repository->find($groupId);
        if(null !== $ruleGroup) {
            $direction = $request->get('direction');
            if('down' === $direction) {
                $maxOrder = $this->repository->maxOrder();
                $order    = (int)$ruleGroup->order;
                if ($order < $maxOrder) {
                    $newOrder = $order + 1;
                    $this->repository->setOrder($ruleGroup, $newOrder);
                }
            }
            if('up' === $direction) {
                $order = (int)$ruleGroup->order;
                if ($order > 1) {
                    $newOrder = $order - 1;
                    $this->repository->setOrder($ruleGroup, $newOrder);
                }
            }
        return new JsonResponse(['OK']);

function moveRuleGroup(e) {
    let box = $(e.currentTarget);
    var direction = box.data('direction');
    var groupId = box.data('id');

    $.post(moveRuleGroupUrl, {_token: token, direction: direction, id: groupId}).then(function () {
        location.reload();
    }).fail(function() {
        alert('I failed :(');
    });

    return false;
}

function duplicateRule(e) {
    let box = $(e.currentTarget);
    var ruleId = box.data('id');
    $.post(duplicateRuleUrl, {_token: token, id: ruleId}).then(function () {
        location.reload();
    }).fail(function() {
        alert('I failed :(');
    });

    return false;
}

      $('.move-group').click(moveRuleGroup);
      $('.duplicate-rule').click(duplicateRule);

                                        <li><a href="#" class="move-group" data-direction="up" data-id="{{ ruleGroup.id }}"><span
                                        <li><a href="#" class="move-group" data-direction="down" data-id="{{ ruleGroup.id }}"><span
                                                <a href="#" class="btn btn-default duplicate-rule" data-id="{{ rule.id }}" title=" {{ trans('firefly.duplicate_rule', {title: rule.title}) }}"><span class="fa fa-fw fa-copy"></span></a>
    <script type="text/javascript" nonce="{{ JS_NONCE }}">
        var moveRuleGroupUrl = '{{ route('rule-groups.move') }}';
        var duplicateRuleUrl = '{{ route('rules.duplicate') }}';
    </script>
        Route::post('duplicate', ['uses' => 'Rule\CreateController@duplicate', 'as' => 'duplicate']);
        // new route to move rule groups:
        Route::post('move', ['uses' => 'RuleGroup\EditController@moveGroup', 'as' => 'move']);

        Route::get('select/{ruleGroup}', ['uses' => 'RuleGroup\ExecutionController@selectTransactions', 'as' => 'select-transactions']);