From 109d7fca179d15f4b8985aeff389216c28727db8 Mon Sep 17 00:00:00 2001
From: Baptiste Jonglez <git@bitsofnetworks.org>
Date: Mon, 12 Jul 2021 01:18:36 +0200
Subject: [PATCH] Add CSRF validation to most disruptive actions

This also switches all such actions to POST requests.

Deleting the project is handled in another commit because it requires more
changes.
---
 ihatemoney/forms.py                           |  6 +++++
 ihatemoney/static/css/main.css                |  5 ++--
 ihatemoney/templates/list_bills.html          |  5 +++-
 .../templates/sidebar_table_layout.html       |  2 ++
 ihatemoney/tests/budget_test.py               |  8 +++----
 ihatemoney/tests/history_test.py              |  6 ++---
 ihatemoney/web.py                             | 24 +++++++++++++++++--
 7 files changed, 44 insertions(+), 12 deletions(-)

diff --git a/ihatemoney/forms.py b/ihatemoney/forms.py
index 0029e151f..356450f7b 100644
--- a/ihatemoney/forms.py
+++ b/ihatemoney/forms.py
@@ -373,3 +373,9 @@ def validate_emails(form, field):
                 raise ValidationError(
                     _("The email %(email)s is not valid", email=email)
                 )
+
+
+class EmptyForm(FlaskForm):
+    """Used for CSRF validation"""
+
+    pass
diff --git a/ihatemoney/static/css/main.css b/ihatemoney/static/css/main.css
index a4fae2802..3a6630bf7 100644
--- a/ihatemoney/static/css/main.css
+++ b/ihatemoney/static/css/main.css
@@ -313,7 +313,7 @@ footer .footer-left {
   text-align: center;
 }
 
-.bill-actions > .delete,
+.bill-actions > form > .delete,
 .bill-actions > .edit,
 .bill-actions > .show {
   font-size: 0px;
@@ -323,9 +323,10 @@ footer .footer-left {
   margin: 2px;
   margin-left: 5px;
   float: left;
+  border: none;
 }
 
-.bill-actions > .delete {
+.bill-actions > form > .delete {
   background: url("../images/delete.png") no-repeat right;
 }
 
diff --git a/ihatemoney/templates/list_bills.html b/ihatemoney/templates/list_bills.html
index 849fc15bf..0240191f5 100644
--- a/ihatemoney/templates/list_bills.html
+++ b/ihatemoney/templates/list_bills.html
@@ -131,7 +131,10 @@ <h3 class="modal-title">{{ _('Add a bill') }}</h3>
                 </td>
                 <td class="bill-actions">
                     <a class="edit" href="{{ url_for(".edit_bill", bill_id=bill.id) }}" title="{{ _("edit") }}">{{ _('edit') }}</a>
-                    <a class="delete" href="{{ url_for(".delete_bill", bill_id=bill.id) }}" title="{{ _("delete") }}">{{ _('delete') }}</a>
+                    <form action="{{ url_for(".delete_bill", bill_id=bill.id) }}" method="POST">
+                        {{ csrf_form.csrf_token }}
+                        <button class="action delete" type="submit" title="{{ _("delete") }}"></button>
+                    </form>
                     {% if bill.external_link %}
                     <a class="show" href="{{ bill.external_link }}" ref="noopener" target="_blank" title="{{ _("show") }}">{{ _('show') }} </a>
                     {% endif %}
diff --git a/ihatemoney/templates/sidebar_table_layout.html b/ihatemoney/templates/sidebar_table_layout.html
index b25a3d683..5bb5a8973 100644
--- a/ihatemoney/templates/sidebar_table_layout.html
+++ b/ihatemoney/templates/sidebar_table_layout.html
@@ -22,6 +22,7 @@
           {%- if member.activated %}
             <td>
               <form class="action delete" action="{{ url_for(".remove_member", member_id=member.id) }}" method="POST">
+                {{ csrf_form.csrf_token }}
                 <button type="submit">{{ _("deactivate") }}</button>
               </form>
               <form class="action edit" action="{{ url_for(".edit_member", member_id=member.id) }}" method="GET">
@@ -31,6 +32,7 @@
           {%- else %}
             <td>
               <form class="action reactivate" action="{{ url_for(".reactivate", member_id=member.id) }}" method="POST">
+                {{ csrf_form.csrf_token }}
                 <button type="submit">{{ _("reactivate") }}</button>
               </form>
             </td>
diff --git a/ihatemoney/tests/budget_test.py b/ihatemoney/tests/budget_test.py
index e3b6778f6..12576e567 100644
--- a/ihatemoney/tests/budget_test.py
+++ b/ihatemoney/tests/budget_test.py
@@ -550,7 +550,7 @@ def test_manage_bills(self):
         self.assertEqual(bill.amount, 10, "bill edition")
 
         # delete the bill
-        self.client.get(f"/raclette/delete/{bill.id}")
+        self.client.post(f"/raclette/delete/{bill.id}")
         self.assertEqual(0, len(models.Bill.query.all()), "bill deletion")
 
         # test balance
@@ -1375,10 +1375,10 @@ def test_access_other_projects(self):
         resp = self.client.post("/tartiflette/edit/1", data=modified_bill)
         self.assertStatus(404, resp)
         # Try to delete bill
-        resp = self.client.get("/raclette/delete/1")
+        resp = self.client.post("/raclette/delete/1")
         self.assertStatus(303, resp)
         # Try to delete bill by ID
-        resp = self.client.get("/tartiflette/delete/1")
+        resp = self.client.post("/tartiflette/delete/1")
         self.assertStatus(302, resp)
 
         # Additional check that the bill was indeed not modified or deleted
@@ -1396,7 +1396,7 @@ def test_access_other_projects(self):
         bill = models.Bill.query.filter(models.Bill.id == 1).one_or_none()
         self.assertNotEqual(bill, None, "bill not found")
         self.assertEqual(bill.what, "roblochon")
-        self.client.get("/raclette/delete/1")
+        self.client.post("/raclette/delete/1")
         bill = models.Bill.query.filter(models.Bill.id == 1).one_or_none()
         self.assertEqual(bill, None)
 
diff --git a/ihatemoney/tests/history_test.py b/ihatemoney/tests/history_test.py
index 1a8d35046..5938d8328 100644
--- a/ihatemoney/tests/history_test.py
+++ b/ihatemoney/tests/history_test.py
@@ -222,7 +222,7 @@ def do_misc_database_operations(self, logging_mode):
         )
         self.assertEqual(resp.status_code, 200)
         # delete the bill
-        resp = self.client.get(f"/demo/delete/{bill_id}", follow_redirects=True)
+        resp = self.client.post(f"/demo/delete/{bill_id}", follow_redirects=True)
         self.assertEqual(resp.status_code, 200)
 
         # delete user using POST method
@@ -389,7 +389,7 @@ def test_logs_for_common_actions(self):
         )
 
         # delete the bill
-        resp = self.client.get("/demo/delete/1", follow_redirects=True)
+        resp = self.client.post("/demo/delete/1", follow_redirects=True)
         self.assertEqual(resp.status_code, 200)
 
         resp = self.client.get("/demo/history")
@@ -527,7 +527,7 @@ def test_bill_add_remove_add(self):
         )
 
         # delete the bill
-        self.client.get("/demo/delete/1", follow_redirects=True)
+        self.client.post("/demo/delete/1", follow_redirects=True)
 
         resp = self.client.get("/demo/history")
         self.assertEqual(resp.status_code, 200)
diff --git a/ihatemoney/web.py b/ihatemoney/web.py
index 41940b30d..667da61ee 100644
--- a/ihatemoney/web.py
+++ b/ihatemoney/web.py
@@ -40,6 +40,7 @@
     AdminAuthenticationForm,
     AuthenticationForm,
     EditProjectForm,
+    EmptyForm,
     InviteForm,
     MemberForm,
     PasswordReminder,
@@ -608,6 +609,7 @@ def invite():
 @main.route("/<project_id>/")
 def list_bills():
     bill_form = get_billform_for(g.project)
+    csrf_form = EmptyForm()
     # set the last selected payer as default choice if exists
     if "last_selected_payer" in session:
         bill_form.payer.data = session["last_selected_payer"]
@@ -623,6 +625,7 @@ def list_bills():
         bills=bills,
         member_form=MemberForm(g.project),
         bill_form=bill_form,
+        csrf_form=csrf_form,
         add_bill=request.values.get("add_bill", False),
         current_view="list_bills",
     )
@@ -644,6 +647,12 @@ def add_member():
 
 @main.route("/<project_id>/members/<member_id>/reactivate", methods=["POST"])
 def reactivate(member_id):
+    # Used for CSRF validation
+    form = EmptyForm()
+    if not form.validate():
+        flash(_("CSRF Token: The CSRF token is invalid."), category="danger")
+        return redirect(url_for(".list_bills"))
+
     person = (
         Person.query.filter(Person.id == member_id)
         .filter(Project.id == g.project.id)
@@ -658,6 +667,12 @@ def reactivate(member_id):
 
 @main.route("/<project_id>/members/<member_id>/delete", methods=["POST"])
 def remove_member(member_id):
+    # Used for CSRF validation
+    form = EmptyForm()
+    if not form.validate():
+        flash(_("CSRF Token: The CSRF token is invalid."), category="danger")
+        return redirect(url_for(".list_bills"))
+
     member = g.project.remove_member(member_id)
     if member:
         if not member.activated:
@@ -715,9 +730,14 @@ def add_bill():
     return render_template("add_bill.html", form=form)
 
 
-@main.route("/<project_id>/delete/<int:bill_id>")
+@main.route("/<project_id>/delete/<int:bill_id>", methods=["POST"])
 def delete_bill(bill_id):
-    # fixme: everyone is able to delete a bill
+    # Used for CSRF validation
+    form = EmptyForm()
+    if not form.validate():
+        flash(_("CSRF Token: The CSRF token is invalid."), category="danger")
+        return redirect(url_for(".list_bills"))
+
     bill = Bill.query.get(g.project, bill_id)
     if not bill:
         return redirect(url_for(".list_bills"))