CVE-2021-3730: 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-08-23T13:15Z
Repository: https://github.com/firefly-iii/firefly-iii
Language: PHP
Files changed: app/Http/Controllers/Budget/AvailableBudgetController.php, app/Repositories/Budget/AvailableBudgetRepository.php, app/Repositories/Budget/AvailableBudgetRepositoryInterface.php, app/Repositories/Budget/BudgetLimitRepository.php, public/v1/js/ff/budgets/index.js, resources/views/v1/budgets/index.twig, routes/web.php
Commit date: 2021-08-20 10:05:18 +0200
Commit message: Fix https://huntr.dev/bounties/ea181323-51f8-46a2-a60f-6a401907feb7/

Diff: diff --git a/app/Http/Controllers/Budget/AvailableBudgetController.php b/app/Http/Controllers/Budget/AvailableBudgetController.php
index 31ca116e66a..323f0d3d6a0 100644
--- a/app/Http/Controllers/Budget/AvailableBudgetController.php
+++ b/app/Http/Controllers/Budget/AvailableBudgetController.php
@@ -135,14 +135,20 @@ static function (TransactionCurrency $currency) use ($availableBudgets) {
     /**
      * @param AvailableBudget $availableBudget
      *
      * @return RedirectResponse|Redirector
      */
-    public function delete(AvailableBudget $availableBudget)
+    public function delete(Request $request)
     {
-        $this->abRepository->destroyAvailableBudget($availableBudget);
-        session()->flash('success', trans('firefly.deleted_ab'));
+        $id = (int)$request->get('id');
+        if (0 !== $id) {
+            $availableBudget = $this->abRepository->findById($id);
+            if (null !== $availableBudget) {
+                $this->abRepository->destroyAvailableBudget($availableBudget);
+                session()->flash('success', trans('firefly.deleted_ab'));
+            }
+        }
 
         return redirect(route('budgets.index'));
     }
 
     /**
diff --git a/app/Repositories/Budget/AvailableBudgetRepository.php b/app/Repositories/Budget/AvailableBudgetRepository.php
index f5f40ca3c97..25afeb262d2 100644
--- a/app/Repositories/Budget/AvailableBudgetRepository.php
+++ b/app/Repositories/Budget/AvailableBudgetRepository.php
@@ -77,10 +77,18 @@ public function find(TransactionCurrency $currency, Carbon $start, Carbon $end):
                           ->where('end_date', $end->format('Y-m-d'))
                           ->first();
 
     }
 
+    /**
+     * @inheritDoc
+     */
+    public function findById(int $id): ?AvailableBudget
+    {
+        return $this->user->availableBudgets->find($id);
+    }
+
     /**
      * Return a list of all available budgets (in all currencies) (for the selected period).
      *
      * @param Carbon|null $start
      * @param Carbon|null $end
diff --git a/app/Repositories/Budget/AvailableBudgetRepositoryInterface.php b/app/Repositories/Budget/AvailableBudgetRepositoryInterface.php
index 1120dfba3ea..2385127975b 100644
--- a/app/Repositories/Budget/AvailableBudgetRepositoryInterface.php
+++ b/app/Repositories/Budget/AvailableBudgetRepositoryInterface.php
@@ -54,10 +54,17 @@ public function destroyAvailableBudget(AvailableBudget $availableBudget): void;
      *
      * @return AvailableBudget|null
      */
     public function find(TransactionCurrency $currency, Carbon $start, Carbon $end): ?AvailableBudget;
 
+    /**
+     * @param int $id
+     *
+     * @return AvailableBudget|null
+     */
+    public function findById(int $id): ?AvailableBudget;
+
     /**
      * Return a list of all available budgets (in all currencies) (for the selected period).
      *
      * @param Carbon|null $start
      * @param Carbon|null $end
diff --git a/app/Repositories/Budget/BudgetLimitRepository.php b/app/Repositories/Budget/BudgetLimitRepository.php
index d3e20d7da27..cb4df10b1c0 100644
--- a/app/Repositories/Budget/BudgetLimitRepository.php
+++ b/app/Repositories/Budget/BudgetLimitRepository.php
@@ -20,10 +20,11 @@
  */
 
 declare(strict_types=1);
 
 namespace FireflyIII\Repositories\Budget;
+
 use Carbon\Carbon;
 use Exception;
 use FireflyIII\Exceptions\FireflyException;
 use FireflyIII\Factory\TransactionCurrencyFactory;
 use FireflyIII\Models\Budget;
@@ -317,21 +318,21 @@ public function store(array $data): BudgetLimit
         $currency->save();
 
         // find the budget:
         $budget = $this->user->budgets()->find((int)$data['budget_id']);
         if (null === $budget) {
-            throw new FireflyException('200004: Budget does not exist.'); 
+            throw new FireflyException('200004: Budget does not exist.');
         }
 
         // find limit with same date range and currency.
         $limit = $budget->budgetlimits()
                         ->where('budget_limits.start_date', $data['start_date']->format('Y-m-d'))
                         ->where('budget_limits.end_date', $data['end_date']->format('Y-m-d'))
                         ->where('budget_limits.transaction_currency_id', $currency->id)
                         ->first(['budget_limits.*']);
         if (null !== $limit) {
-            throw new FireflyException('200027: Budget limit already exists.'); 
+            throw new FireflyException('200027: Budget limit already exists.');
         }
         Log::debug('No existing budget limit, create a new one');
 
         // or create one and return it.
         $limit = new BudgetLimit;
@@ -423,10 +424,11 @@ public function updateLimitAmount(Budget $budget, Carbon $start, Carbon $end, st
             try {
                 $limit->delete();
             } catch (Exception $e) { // @phpstan-ignore-line
                 // @ignoreException
             }
+
             return null;
         }
         // update if exists:
         if (null !== $limit) {
             Log::debug(sprintf('Existing budget limit is #%d, update this to amount %s', $limit->id, $amount));
diff --git a/public/v1/js/ff/budgets/index.js b/public/v1/js/ff/budgets/index.js
index 0e8ab6f0719..e772587102a 100644
--- a/public/v1/js/ff/budgets/index.js
+++ b/public/v1/js/ff/budgets/index.js
@@ -28,10 +28,11 @@ $(function () {
      */
     drawSpentBars();
     drawBudgetedBars();
 
     $('.update_ab').on('click', updateAvailableBudget);
+    $('.delete_ab').on('click', deleteAvailableBudget);
     $('.create_ab_alt').on('click', createAltAvailableBudget);
 
     $('.budget_amount').on('change', updateBudgetedAmount);
     $('.create_bl').on('click', createBudgetLimit);
     $('.delete_bl').on('click', deleteBudgetLimit);
@@ -239,11 +240,21 @@ function updateAvailableBudget(e) {
             $('#defaultModal').modal('show');
         });
     }
     return false;
 }
-
+function deleteAvailableBudget(e) {
+    //
+    e.preventDefault();
+    var button = $(e.currentTarget);
+    var abId = button.data('id');
+    $.post(deleteABUrl, {_token: token, id: abId}).then(function () {
+        // lame but it works.
+        location.reload();
+    });
+    return false;
+}
 
 function drawBudgetedBars() {
     "use strict";
     $.each($('.budgeted_bar'), function (i, v) {
         var bar = $(v);
@@ -286,135 +297,5 @@ function drawSpentBars() {
             pct = (spent / budgeted) * 100;
             bar.find('.progress-bar-info').css('width', pct + '%');
         }
     });
 }
-
-//
-//
-// function drawSpentBar() {
-//     "use strict";
-//     if ($('.spentBar').length > 0) {
-//         var overspent = spent > budgeted;
-//         var pct;
-//
-//         if (overspent) {
-//             // draw overspent bar
-//             pct = (budgeted / spent) * 100;
-//             $('.spentBar .progress-bar-warning').css('width', pct + '%');
-//             $('.spentBar .progress-bar-danger').css('width', (100 - pct) + '%');
-//         } else {
-//             // draw normal bar:
-//             pct = (spent / budgeted) * 100;
-//             $('.spentBar .progress-bar-info').css('width', pct + '%');
-//         }
-//     }
-// }
-//
-// function drawBudgetedBar() {
-//     "use strict";
-//
-//     if ($('.budgetedBar').length > 0) {
-//         var budgetedMuch = budgeted > available;
-//
-//         // recalculate percentage:
-//
-//         var pct;
-//         if (budgetedMuch) {
-//             // budgeted too much.
-//             pct = (available / budgeted) * 100;
-//             $('.budgetedBar .progress-bar-warning').css('width', pct + '%');
-//             $('.budgetedBar .progress-bar-danger').css('width', (100 - pct) + '%');
-//             $('.budgetedBar .progress-bar-info').css('width', 0);
-//         } else {
-//             pct = (budgeted / available) * 100;
-//             $('.budgetedBar .progress-bar-warning').css('width', 0);
-//             $('.budgetedBar .progress-bar-danger').css('width', 0);
-//             $('.budgetedBar .progress-bar-info').css('width', pct + '%');
-//         }
-//
-//         $('#budgetedAmount').html(currencySymbol + ' ' + budgeted.toFixed(2));
-//     }
-// }
-
-// /**
-//  *
-//  * @param e
-//  */
-// function updateBudgetedAmounts(e) {
-//     "use strict";
-//     var target = $(e.target);
-//     var id = target.data('id');
-//     var leftCell = $('td[class$="left"][data-id="' + id + '"]');
-//     var link = $('a[data-id="' + id + '"][class="budget-link"]');
-//     var value = target.val();
-//     var original = target.data('original');
-//
-//     // disable input
-//     target.prop('disabled', true);
-//
-//     // replace link (for now)
-//     link.attr('href', '#');
-//
-//     // replace "left" with spinner.
-//     leftCell.empty().html('<i class="fa fa-fw fa-spin fa-spinner"></i>');
-//
-//     // send a post to Firefly to update the amount:
-//     var newUri = budgetAmountUri.replace("REPLACE", id);
-//
-//     $.post(newUri, {amount: value, start: periodStart, end: periodEnd, _token: token}).done(function (data) {
-//
-//         // difference between new value and original value
-//         var difference = value - original;
-//
-//         // update budgeted value
-//         budgeted = budgeted + difference;
-//
-//         // fill in "left" value:
-//
-//
-//         if (data.left_per_day !== null) {
-//             leftCell.html(data.left + ' (' + data.left_per_day + ')');
-//         } else {
-//             leftCell.html(data.left);
-//         }
-//
-//         // update "budgeted" input:
-//         target.val(data.amount);
-//
-//         // enable thing again
-//         target.prop('disabled', false);
-//
-//         // set new original value:
-//         target.data('original', data.amount);
-//
-//         // run drawBudgetedBar() again:
-//         drawBudgetedBar();
-//
-//         // update the link if relevant:
-//         link.attr('href', 'budgets/show/' + id);
-//         if (data.limit > 0) {
-//             link.attr('href', 'budgets/show/' + id + '/' + data.limit);
-//         }
-//
-//         // update the warning if relevant:
-//         if (data.large_diff === true) {
-//             $('span[class$="budget_warning"][data-id="' + id + '"]').html(data.warn_text).show();
-//             console.log('Show warning for budget');
-//         } else {
-//             $('span[class$="budget_warning"][data-id="' + id + '"]').empty().hide();
-//         }
-//     });
-// }
-
-// /**
-//  *
-//  * @returns {boolean}
-//  */
-// function updateIncome() {
-//     "use strict";
-//     $('#defaultModal').empty().load(updateIncomeUri, function () {
-//         $('#defaultModal').modal('show');
-//     });
-//
-//     return false;
-// }
diff --git a/resources/views/v1/budgets/index.twig b/resources/views/v1/budgets/index.twig
index 43a63e8974e..16cecb029ec 100644
--- a/resources/views/v1/budgets/index.twig
+++ b/resources/views/v1/budgets/index.twig
@@ -135,12 +135,11 @@
                                            data-id="{{ budget.id }}">{{ trans('firefly.available_between', {start: budget.start_date.formatLocalized(monthAndDayFormat), end: budget.end_date.formatLocalized(monthAndDayFormat) }) }}
                                         :
                                         <span class="available_amount" data-id="{{ budget.id }}"
                                               data-value="{{ budget.amount }}">{{ formatAmountBySymbol(budget.amount, budget.transaction_currency.symbol, budget.transaction_currency.decimal_places, true) }}</span>
                                         <a href="#" data-id="{{ budget.id }}" class="update_ab btn btn-default btn-xs"><span class="fa fa-pencil"></span></a>
-                                        <a href="{{ route('available-budgets.delete', [budget.id]) }}" data-id="{{ budget.id }}"
-                                           class="delete_ab btn btn-danger btn-xs"><span class="fa fa-trash"></span></a>
+                                        <a href="#" data-id="{{ budget.id }}" class="delete_ab btn btn-danger btn-xs"><span class="fa fa-trash"></span></a>
                                     </small>
                                 </div>
                             </div>
                             {# progresss bar to visualise available vs budgeted. #}
                             <div class="row">
@@ -463,10 +462,11 @@
 
         // create available budgets / edit
         var createAvailableBudgetUri = "{{ route('available-budgets.create', [start.format('Y-m-d'), end.format('Y-m-d')]) }}";
         var createAltAvailableBudgetUri = "{{ route('available-budgets.create-alternative', [start.format('Y-m-d'), end.format('Y-m-d')]) }}";
         var editAvailableBudgetUri = "{{ route('available-budgets.edit', ['REPLACEME', start.format('Y-m-d'), end.format('Y-m-d')]) }}";
+        var deleteABUrl = "{{ route('available-budgets.delete') }}";
 
         // budget limit create form.
         var createBudgetLimitUri = "{{ route('budget-limits.create', ['REPLACEME', start.format('Y-m-d'), end.format('Y-m-d')]) }}";
         var storeBudgetLimitUri = "{{ route('budget-limits.store') }}";
         var updateBudgetLimitUri = "{{ route('budget-limits.update', ['REPLACEME']) }}";
diff --git a/routes/web.php b/routes/web.php
index 0d4bad50879..0a0599c8dbe 100644
--- a/routes/web.php
+++ b/routes/web.php
@@ -267,11 +267,11 @@ static function () {
 
         // edit
         Route::get('edit/{availableBudget}/{start_date}/{end_date}', ['uses' => 'Budget\AvailableBudgetController@edit', 'as' => 'edit']);
         Route::post('update/{availableBudget}/{start_date}/{end_date}', ['uses' => 'Budget\AvailableBudgetController@update', 'as' => 'update']);
 
-        Route::get('delete/{availableBudget}', ['uses' => 'Budget\AvailableBudgetController@delete', 'as' => 'delete']);
+        Route::post('delete', ['uses' => 'Budget\AvailableBudgetController@delete', 'as' => 'delete']);
     }
 );
 
 /**
  * Budget Limit Controller.

Vulnerable code:     public function delete(AvailableBudget $availableBudget)
        $this->abRepository->destroyAvailableBudget($availableBudget);
        session()->flash('success', trans('firefly.deleted_ab'));
            throw new FireflyException('200004: Budget does not exist.'); 
            throw new FireflyException('200027: Budget limit already exists.'); 


//
//
// function drawSpentBar() {
//     "use strict";
//     if ($('.spentBar').length > 0) {
//         var overspent = spent > budgeted;
//         var pct;
//
//         if (overspent) {
//             // draw overspent bar
//             pct = (budgeted / spent) * 100;
//             $('.spentBar .progress-bar-warning').css('width', pct + '%');
//             $('.spentBar .progress-bar-danger').css('width', (100 - pct) + '%');
//         } else {
//             // draw normal bar:
//             pct = (spent / budgeted) * 100;
//             $('.spentBar .progress-bar-info').css('width', pct + '%');
//         }
//     }
// }
//
// function drawBudgetedBar() {
//     "use strict";
//
//     if ($('.budgetedBar').length > 0) {
//         var budgetedMuch = budgeted > available;
//
//         // recalculate percentage:
//
//         var pct;
//         if (budgetedMuch) {
//             // budgeted too much.
//             pct = (available / budgeted) * 100;
//             $('.budgetedBar .progress-bar-warning').css('width', pct + '%');
//             $('.budgetedBar .progress-bar-danger').css('width', (100 - pct) + '%');
//             $('.budgetedBar .progress-bar-info').css('width', 0);
//         } else {
//             pct = (budgeted / available) * 100;
//             $('.budgetedBar .progress-bar-warning').css('width', 0);
//             $('.budgetedBar .progress-bar-danger').css('width', 0);
//             $('.budgetedBar .progress-bar-info').css('width', pct + '%');
//         }
//
//         $('#budgetedAmount').html(currencySymbol + ' ' + budgeted.toFixed(2));
//     }
// }

// /**
//  *
//  * @param e
//  */
// function updateBudgetedAmounts(e) {
//     "use strict";
//     var target = $(e.target);
//     var id = target.data('id');
//     var leftCell = $('td[class$="left"][data-id="' + id + '"]');
//     var link = $('a[data-id="' + id + '"][class="budget-link"]');
//     var value = target.val();
//     var original = target.data('original');
//
//     // disable input
//     target.prop('disabled', true);
//
//     // replace link (for now)
//     link.attr('href', '#');
//
//     // replace "left" with spinner.
//     leftCell.empty().html('<i class="fa fa-fw fa-spin fa-spinner"></i>');
//
//     // send a post to Firefly to update the amount:
//     var newUri = budgetAmountUri.replace("REPLACE", id);
//
//     $.post(newUri, {amount: value, start: periodStart, end: periodEnd, _token: token}).done(function (data) {
//
//         // difference between new value and original value
//         var difference = value - original;
//
//         // update budgeted value
//         budgeted = budgeted + difference;
//
//         // fill in "left" value:
//
//
//         if (data.left_per_day !== null) {
//             leftCell.html(data.left + ' (' + data.left_per_day + ')');
//         } else {
//             leftCell.html(data.left);
//         }
//
//         // update "budgeted" input:
//         target.val(data.amount);
//
//         // enable thing again
//         target.prop('disabled', false);
//
//         // set new original value:
//         target.data('original', data.amount);
//
//         // run drawBudgetedBar() again:
//         drawBudgetedBar();
//
//         // update the link if relevant:
//         link.attr('href', 'budgets/show/' + id);
//         if (data.limit > 0) {
//             link.attr('href', 'budgets/show/' + id + '/' + data.limit);
//         }
//
//         // update the warning if relevant:
//         if (data.large_diff === true) {
//             $('span[class$="budget_warning"][data-id="' + id + '"]').html(data.warn_text).show();
//             console.log('Show warning for budget');
//         } else {
//             $('span[class$="budget_warning"][data-id="' + id + '"]').empty().hide();
//         }
//     });
// }

// /**
//  *
//  * @returns {boolean}
//  */
// function updateIncome() {
//     "use strict";
//     $('#defaultModal').empty().load(updateIncomeUri, function () {
//         $('#defaultModal').modal('show');
//     });
//
//     return false;
// }
                                        <a href="{{ route('available-budgets.delete', [budget.id]) }}" data-id="{{ budget.id }}"
                                           class="delete_ab btn btn-danger btn-xs"><span class="fa fa-trash"></span></a>
        Route::get('delete/{availableBudget}', ['uses' => 'Budget\AvailableBudgetController@delete', 'as' => 'delete']);
Fixed code:     public function delete(Request $request)
        $id = (int)$request->get('id');
        if (0 !== $id) {
            $availableBudget = $this->abRepository->findById($id);
            if (null !== $availableBudget) {
                $this->abRepository->destroyAvailableBudget($availableBudget);
                session()->flash('success', trans('firefly.deleted_ab'));
            }
        }
    /**
     * @inheritDoc
     */
    public function findById(int $id): ?AvailableBudget
    {
        return $this->user->availableBudgets->find($id);
    }

    /**
     * @param int $id
     *
     * @return AvailableBudget|null
     */
    public function findById(int $id): ?AvailableBudget;


            throw new FireflyException('200004: Budget does not exist.');
            throw new FireflyException('200027: Budget limit already exists.');

    $('.delete_ab').on('click', deleteAvailableBudget);
function deleteAvailableBudget(e) {
    //
    e.preventDefault();
    var button = $(e.currentTarget);
    var abId = button.data('id');
    $.post(deleteABUrl, {_token: token, id: abId}).then(function () {
        // lame but it works.
        location.reload();
    });
    return false;
}
                                        <a href="#" data-id="{{ budget.id }}" class="delete_ab btn btn-danger btn-xs"><span class="fa fa-trash"></span></a>
        var deleteABUrl = "{{ route('available-budgets.delete') }}";
        Route::post('delete', ['uses' => 'Budget\AvailableBudgetController@delete', 'as' => 'delete']);