Add more validation to destructive actions (CSRF + additional password checking)

ihatemoney/forms.py

@@ -221,6 +221,36 @@ def validate_id(form, field):
             raise ValidationError(Markup(message))
 
 
+class DestructiveActionProjectForm(FlaskForm):
+    """Used for any important "delete" action linked to a project:
+
+    - delete project itself
+    - delete history
+    - delete IP addresses in history
+    - possibly others in the future
+
+    It asks the user to enter the private code to confirm deletion.
+    """
+
+    password = PasswordField(
+        _("Private code"),
+        description=_("Enter private code to confirm deletion"),
+        validators=[DataRequired()],
+    )
+
+    def __init__(self, *args, **kwargs):
+        # Same trick as EditProjectForm: we need to know the project ID
+        self.id = SimpleNamespace(data=kwargs.pop("id", ""))
+        super().__init__(*args, **kwargs)
+
+    def validate_password(form, field):
+        project = Project.query.get(form.id.data)
+        if project is None:
+            raise ValidationError(_("Unknown error"))
+        if not check_password_hash(project.password, form.password.data):
+            raise ValidationError(_("Invalid private code."))
+
+
 class AuthenticationForm(FlaskForm):
     id = StringField(_("Project identifier"), validators=[DataRequired()])
     password = PasswordField(_("Private code"), validators=[DataRequired()])
@@ -373,3 +403,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

ihatemoney/static/css/main.css

@@ -303,17 +303,12 @@ footer .footer-left {
   color: #fff;
 }
 
-.confirm,
-.confirm:hover {
-  color: red;
-}
-
 .bill-actions {
   padding-top: 10px;
   text-align: center;
 }
 
-.bill-actions > .delete,
+.bill-actions > form > .delete,
 .bill-actions > .edit,
 .bill-actions > .show {
   font-size: 0px;
@@ -323,9 +318,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;
 }
 

ihatemoney/templates/edit_project.html

@@ -1,9 +1,21 @@
 {% extends "layout.html" %}
 
 {% block js %}
-    $('#delete-project').click(function ()
-    {
-        $(this).html("<a style='color:red; ' href='{{ url_for('.delete_project') }}' >{{_("you sure?")}}</a>");
+
+    let link = $('#delete-project').find('button');
+    let deleteOriginalHTML = link.html();
+    link.click(function() {
+        if (link.hasClass("confirm")){
+            return true;
+        }
+        link.html("{{_("Are you sure?")}}");
+        link.addClass("confirm btn-danger");
+        return false;
+    });
+
+    $('#delete-project').focusout(function() {
+        link.removeClass("confirm btn-danger");
+        link.html(deleteOriginalHTML);
     });
 
     $('.custom-file-input').on('change', function(event) {
@@ -21,6 +33,10 @@ <h2>{{ _("Edit project") }}</h2>
         {{ forms.edit_project(edit_form) }}
     </form>
 
+    <h2>{{ _("Delete project") }}</h2>
+    <form id="delete-project" class="form-horizontal" action="{{ url_for(".delete_project") }}" method="post" >
+        {{ forms.delete_project(delete_form) }}
+    </form>
 
     <h2>{{ _("Import JSON") }}</h2>
     <form class="form-horizontal" method="post" enctype="multipart/form-data">

ihatemoney/templates/forms.html

@@ -100,7 +100,18 @@
     {{ input(form.default_currency) }}
     <div class="actions">
         <button class="btn btn-primary">{{ _("Edit the project") }}</button>
-        <a id="delete-project" style="color:red; margin-left:10px; cursor:pointer; ">{{ _("delete") }}</a>
+    </div>
+
+{% endmacro %}
+
+{% macro delete_project(form) %}
+
+    {% include "display_errors.html" %}
+    <p><strong>{{ _("This will remove all bills and participants in this project!") }}</strong></p>
+    {{ form.hidden_tag() }}
+    {{ input(form.password) }}
+    <div class="actions">
+        <button class="btn btn-primary">{{ _("Delete project") }}</button>
     </div>
 
 {% endmacro %}
@@ -114,6 +125,22 @@
     </div>
 {% endmacro %}
 
+{% macro delete_project_history(form) %}
+
+    {% include "display_errors.html" %}
+    {{ form.hidden_tag() }}
+    {{ input(form.password, inline=True) }}
+
+{% endmacro %}
+
+{% macro delete_ip_addresses(form) %}
+
+    {% include "display_errors.html" %}
+    {{ form.hidden_tag() }}
+    {{ input(form.password) }}
+
+{% endmacro %}
+
 {% macro add_bill(form, edit=False, title=True) %}
 
     <fieldset>

ihatemoney/templates/history.html

@@ -43,42 +43,40 @@
 <!-- Modal -->
 <div id="confirm-ip-delete" class="modal fade show" role="dialog">
     <div class="modal-dialog" role="document">
-        <div class="modal-content">
+        <form class="modal-content" action="{{ url_for(".strip_ip_addresses") }}" method="post">
             <div class="modal-header">
                 <h3 class="modal-title">{{ _('Confirm Remove IP Adresses') }}</h3>
                 <a href="#" class="close" data-dismiss="modal">&times;</a>
             </div>
             <div class="modal-body">
                 <p>{{ _("Are you sure you want to delete all recorded IP addresses from this project?
                 The rest of the project history will be unaffected. This action cannot be undone.") }}</p>
+                {{ forms.delete_ip_addresses(delete_form) }}
             </div>
             <div class="modal-footer">
-                <button type="button" class="btn btn-secondary" data-dismiss="modal">{{ _("Close") }}</button>
-                <form action="{{ url_for(".strip_ip_addresses") }}" method="post">
-                    <input type="submit" class="btn btn-danger" value="{{ _("Confirm Delete") }}" name="{{ _("Confirm Delete") }}"/>
-                </form>
-              </div>
-        </div>
+              <button type="submit" class="btn btn-danger">{{ _("Confirm deletion") }}</button>
+              <button type="button" class="btn btn-secondary" data-dismiss="modal">{{ _("Close") }}</button>
+            </div>
+        </form>
     </div>
 </div>
 <!-- Modal -->
     <div id="confirm-erase" class="modal fade show" role="dialog">
     <div class="modal-dialog" role="document">
-        <div class="modal-content">
+        <form class="modal-content" action="{{ url_for(".erase_history") }}" method="post">
             <div class="modal-header">
                 <h3 class="modal-title">{{ _('Delete Confirmation') }}</h3>
                 <a href="#" class="close" data-dismiss="modal">&times;</a>
             </div>
             <div class="modal-body">
                 <p>{{ _("Are you sure you want to erase all history for this project? This action cannot be undone.") }}</p>
+                {{ forms.delete_project_history(delete_form) }}
             </div>
             <div class="modal-footer">
+                <button type="submit" class="btn btn-danger">{{ _("Confirm deletion") }}</button>
                 <button type="button" class="btn btn-secondary" data-dismiss="modal">{{ _("Close") }}</button>
-                <form action="{{ url_for(".erase_history") }}" method="post">
-                    <input type="submit" class="btn btn-danger" value="{{ _("Confirm Delete") }}" name="{{ _("Confirm Delete") }}"/>
-                </form>
-              </div>
-        </div>
+            </div>
+        </form>
     </div>
 </div>
 {% endmacro %}

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 %}

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>

ihatemoney/tests/budget_test.py

@@ -252,7 +252,18 @@ def test_project_deletion(self):
             # project added
             self.assertEqual(len(models.Project.query.all()), 1)
 
-            c.get("/raclette/delete")
+            # Check that we can't delete project with a GET or with a
+            # password-less POST.
+            resp = self.client.get("/raclette/delete")
+            self.assertEqual(resp.status_code, 405)
+            self.client.post("/raclette/delete")
+            self.assertEqual(len(models.Project.query.all()), 1)
+
+            # Delete for real
+            c.post(
+                "/raclette/delete",
+                data={"password": "party"},
+            )
 
             # project removed
             self.assertEqual(len(models.Project.query.all()), 0)
@@ -549,8 +560,12 @@ def test_manage_bills(self):
         bill = models.Bill.query.one()
         self.assertEqual(bill.amount, 10, "bill edition")
 
-        # delete the bill
-        self.client.get(f"/raclette/delete/{bill.id}")
+        # Try to delete the bill with a GET: it should fail
+        response = self.client.get(f"/raclette/delete/{bill.id}")
+        self.assertEqual(response.status_code, 405)
+        self.assertEqual(1, len(models.Bill.query.all()), "bill deletion")
+        # Really delete the bill
+        self.client.post(f"/raclette/delete/{bill.id}")
         self.assertEqual(0, len(models.Bill.query.all()), "bill deletion")
 
         # test balance
@@ -1375,10 +1390,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 +1411,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)
 

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
@@ -235,6 +235,16 @@ def test_disable_clear_no_new_records(self):
         # Disable logging
         self.change_privacy_to(LoggingMode.DISABLED)
 
+        # Ensure we can't clear history with a GET or with a password-less POST
+        resp = self.client.get("/demo/erase_history")
+        self.assertEqual(resp.status_code, 405)
+        resp = self.client.post("/demo/erase_history", follow_redirects=True)
+        self.assertIn(
+            "Error deleting project history",
+            resp.data.decode("utf-8"),
+        )
+
+        # List history
         resp = self.client.get("/demo/history")
         self.assertEqual(resp.status_code, 200)
         self.assertIn(
@@ -251,7 +261,11 @@ def test_disable_clear_no_new_records(self):
         )
 
         # Clear Existing Entries
-        resp = self.client.post("/demo/erase_history", follow_redirects=True)
+        resp = self.client.post(
+            "/demo/erase_history",
+            data={"password": "demo"},
+            follow_redirects=True,
+        )
         self.assertEqual(resp.status_code, 200)
         self.assert_empty_history_logging_disabled()
 
@@ -295,8 +309,27 @@ def test_clear_ip_records(self):
         self.assertEqual(resp.data.decode("utf-8").count("127.0.0.1"), 12)
         self.assertEqual(resp.data.decode("utf-8").count("<td> -- </td>"), 7)
 
-        # Clear IP Data
+        # Ensure we can't clear IP data with a GET or with a password-less POST
+        resp = self.client.get("/demo/strip_ip_addresses")
+        self.assertEqual(resp.status_code, 405)
         resp = self.client.post("/demo/strip_ip_addresses", follow_redirects=True)
+        self.assertIn(
+            "Error deleting recorded IP addresses",
+            resp.data.decode("utf-8"),
+        )
+
+        resp = self.client.get("/demo/history")
+        self.assertEqual(resp.status_code, 200)
+        self.assertEqual(resp.data.decode("utf-8").count("127.0.0.1"), 12)
+        self.assertEqual(resp.data.decode("utf-8").count("<td> -- </td>"), 7)
+
+        # Clear IP Data
+        resp = self.client.post(
+            "/demo/strip_ip_addresses",
+            data={"password": "123456"},
+            follow_redirects=True,
+        )
+
         self.assertEqual(resp.status_code, 200)
         self.assertNotIn(
             "This project has history disabled. New actions won't appear below. ",
@@ -389,7 +422,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 +560,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)