From 9e1e284175336e5437cabff9f8da9d863947d17e Mon Sep 17 00:00:00 2001 From: Baptiste Jonglez Date: Fri, 28 Jul 2023 18:10:34 +0200 Subject: [PATCH 1/5] Require private code to edit a project settings This is something we had documented in our security documentation [1], but we didn't actually do it... As mentioned in [1], this has good security properties: you can invite somebody with an invitation link, and they will be able to access the project but not change the private code (because they don't know the current private code). This new check also applies to all other settings (email address, history settings, currency), which is desirable. Only somebody with knowledge of the private code can now change these settings. [1] https://ihatemoney.readthedocs.io/en/latest/security.html#giving-access-to-a-project --- ihatemoney/forms.py | 16 ++++++++++- ihatemoney/templates/forms.html | 1 + ihatemoney/tests/api_test.py | 49 ++++++++++++++++++++++++++++++-- ihatemoney/tests/budget_test.py | 30 ++++++++++++++++--- ihatemoney/tests/history_test.py | 23 ++++++++------- 5 files changed, 102 insertions(+), 17 deletions(-) diff --git a/ihatemoney/forms.py b/ihatemoney/forms.py index 1bfb0fe22..177119897 100644 --- a/ihatemoney/forms.py +++ b/ihatemoney/forms.py @@ -121,6 +121,11 @@ def process_formdata(self, valuelist): class EditProjectForm(FlaskForm): name = StringField(_("Project name"), validators=[DataRequired()]) + current_password = PasswordField( + _("Current private code"), + description=_("Enter existing private code to edit project"), + validators=[DataRequired()], + ) # If empty -> don't change the password password = PasswordField( _("New private code"), @@ -154,6 +159,13 @@ def __init__(self, *args, **kwargs): for currency_name in self.currency_helper.get_currencies() ] + def validate_current_password(self, field): + project = Project.query.get(self.id.data) + if project is None: + raise ValidationError(_("Unknown error")) + if not check_password_hash(project.password, self.current_password.data): + raise ValidationError(_("Invalid private code.")) + @property def logging_preference(self): """Get the LoggingMode object corresponding to current form data.""" @@ -212,7 +224,9 @@ class ImportProjectForm(FlaskForm): class ProjectForm(EditProjectForm): id = StringField(_("Project identifier"), validators=[DataRequired()]) - # This field overrides the one from EditProjectForm + # Remove this field that is inherited from EditProjectForm + current_password = None + # This field overrides the one from EditProjectForm (to make it mandatory) password = PasswordField(_("Private code"), validators=[DataRequired()]) submit = SubmitField(_("Create the project")) diff --git a/ihatemoney/templates/forms.html b/ihatemoney/templates/forms.html index 26eb376a3..e339268ec 100644 --- a/ihatemoney/templates/forms.html +++ b/ihatemoney/templates/forms.html @@ -100,6 +100,7 @@ {{ input(form.default_currency) }} + {{ input(form.current_password) }}
diff --git a/ihatemoney/tests/api_test.py b/ihatemoney/tests/api_test.py index b5f11e3ed..73d917da1 100644 --- a/ihatemoney/tests/api_test.py +++ b/ihatemoney/tests/api_test.py @@ -131,7 +131,7 @@ def test_project(self): decoded_resp = json.loads(resp.data.decode("utf-8")) self.assertDictEqual(decoded_resp, expected) - # edit should work + # edit should fail if we don't provide the current private code resp = self.client.put( "/api/projects/raclette", data={ @@ -143,7 +143,36 @@ def test_project(self): }, headers=self.get_auth("raclette"), ) + self.assertEqual(400, resp.status_code) + # edit should fail if we provide the wrong private code + resp = self.client.put( + "/api/projects/raclette", + data={ + "contact_email": "yeah@notmyidea.org", + "default_currency": "XXX", + "current_password": "fromage aux patates", + "password": "raclette", + "name": "The raclette party", + "project_history": "y", + }, + headers=self.get_auth("raclette"), + ) + self.assertEqual(400, resp.status_code) + + # edit with the correct private code should work + resp = self.client.put( + "/api/projects/raclette", + data={ + "contact_email": "yeah@notmyidea.org", + "default_currency": "XXX", + "current_password": "raclette", + "password": "raclette", + "name": "The raclette party", + "project_history": "y", + }, + headers=self.get_auth("raclette"), + ) self.assertEqual(200, resp.status_code) resp = self.client.get( @@ -168,6 +197,7 @@ def test_project(self): data={ "contact_email": "yeah@notmyidea.org", "default_currency": "XXX", + "current_password": "raclette", "password": "tartiflette", "name": "The raclette party", }, @@ -213,9 +243,23 @@ def test_token_creation(self): "/api/projects/raclette/token", headers={"Authorization": f"Basic {decoded_resp['token']}"}, ) - self.assertEqual(200, resp.status_code) + # We shouldn't be able to edit project without private code + resp = self.client.put( + "/api/projects/raclette", + data={ + "contact_email": "yeah@notmyidea.org", + "default_currency": "XXX", + "password": "tartiflette", + "name": "The raclette party", + }, + headers={"Authorization": f"Basic {decoded_resp['token']}"}, + ) + self.assertEqual(400, resp.status_code) + expected_resp = {"current_password": ["This field is required."]} + self.assertEqual(expected_resp, json.loads(resp.data.decode("utf-8"))) + def test_token_login(self): resp = self.api_create("raclette") # Get token @@ -719,6 +763,7 @@ def test_currencies(self): data={ "contact_email": "yeah@notmyidea.org", "default_currency": "XXX", + "current_password": "raclette", "password": "raclette", "name": "The raclette party", }, diff --git a/ihatemoney/tests/budget_test.py b/ihatemoney/tests/budget_test.py index bac565074..1b9792236 100644 --- a/ihatemoney/tests/budget_test.py +++ b/ihatemoney/tests/budget_test.py @@ -181,6 +181,7 @@ def test_invite_code_invalidation(self): data={ "name": "raclette", "contact_email": "zorglub@notmyidea.org", + "current_password": "raclette", "password": "didoudida", "default_currency": "XXX", }, @@ -922,10 +923,30 @@ def test_edit_project(self): "default_currency": "USD", } - resp = self.client.post("/raclette/edit", data=new_data, follow_redirects=True) - self.assertEqual(resp.status_code, 200) + # It should fail if we don't provide the current password + resp = self.client.post("/raclette/edit", data=new_data, follow_redirects=False) + self.assertIn("This field is required", resp.data.decode("utf-8")) + project = self.get_project("raclette") + self.assertNotEqual(project.name, new_data["name"]) + self.assertNotEqual(project.contact_email, new_data["contact_email"]) + self.assertNotEqual(project.default_currency, new_data["default_currency"]) + self.assertFalse(check_password_hash(project.password, new_data["password"])) + + # It should fail if we provide the wrong current password + new_data["current_password"] = "patates au fromage" + resp = self.client.post("/raclette/edit", data=new_data, follow_redirects=False) + self.assertIn("Invalid private code", resp.data.decode("utf-8")) + project = self.get_project("raclette") + self.assertNotEqual(project.name, new_data["name"]) + self.assertNotEqual(project.contact_email, new_data["contact_email"]) + self.assertNotEqual(project.default_currency, new_data["default_currency"]) + self.assertFalse(check_password_hash(project.password, new_data["password"])) + + # It should work if we give the current private code + new_data["current_password"] = "raclette" + resp = self.client.post("/raclette/edit", data=new_data) + self.assertEqual(resp.status_code, 302) project = self.get_project("raclette") - self.assertEqual(project.name, new_data["name"]) self.assertEqual(project.contact_email, new_data["contact_email"]) self.assertEqual(project.default_currency, new_data["default_currency"]) @@ -934,7 +955,7 @@ def test_edit_project(self): # Editing a project with a wrong email address should fail new_data["contact_email"] = "wrong_email" - resp = self.client.post("/raclette/edit", data=new_data, follow_redirects=True) + resp = self.client.post("/raclette/edit", data=new_data) self.assertIn("Invalid email address", resp.data.decode("utf-8")) def test_dashboard(self): @@ -2039,6 +2060,7 @@ def test_rss_feed_invalidated_token(self): data={ "name": "raclette", "contact_email": "zorglub@notmyidea.org", + "current_password": "raclette", "password": "didoudida", "default_currency": "XXX", }, diff --git a/ihatemoney/tests/history_test.py b/ihatemoney/tests/history_test.py index 1cc15cedf..9b4ec33fa 100644 --- a/ihatemoney/tests/history_test.py +++ b/ihatemoney/tests/history_test.py @@ -19,11 +19,12 @@ def test_simple_create_logentry_no_ip(self): self.assertEqual(resp.data.decode("utf-8").count(" -- "), 1) self.assertNotIn("127.0.0.1", resp.data.decode("utf-8")) - def change_privacy_to(self, logging_preference): + def change_privacy_to(self, current_password, logging_preference): # Change only logging_preferences new_data = { "name": "demo", "contact_email": "demo@notmyidea.org", + "current_password": current_password, "password": "demo", "default_currency": "XXX", } @@ -76,6 +77,7 @@ def test_project_edit(self): new_data = { "name": "demo2", "contact_email": "demo2@notmyidea.org", + "current_password": "demo", "password": "123456", "project_history": "y", "default_currency": "USD", # Currency changed from default @@ -114,7 +116,7 @@ def test_project_privacy_edit(self): resp.data.decode("utf-8"), ) - self.change_privacy_to(LoggingMode.DISABLED) + self.change_privacy_to("demo", LoggingMode.DISABLED) resp = self.client.get("/demo/history") self.assertEqual(resp.status_code, 200) @@ -122,7 +124,7 @@ def test_project_privacy_edit(self): self.assertEqual(resp.data.decode("utf-8").count(" -- "), 2) self.assertNotIn("127.0.0.1", resp.data.decode("utf-8")) - self.change_privacy_to(LoggingMode.RECORD_IP) + self.change_privacy_to("demo", LoggingMode.RECORD_IP) resp = self.client.get("/demo/history") self.assertEqual(resp.status_code, 200) @@ -132,7 +134,7 @@ def test_project_privacy_edit(self): self.assertEqual(resp.data.decode("utf-8").count(" -- "), 2) self.assertEqual(resp.data.decode("utf-8").count("127.0.0.1"), 1) - self.change_privacy_to(LoggingMode.ENABLED) + self.change_privacy_to("demo", LoggingMode.ENABLED) resp = self.client.get("/demo/history") self.assertEqual(resp.status_code, 200) @@ -141,7 +143,7 @@ def test_project_privacy_edit(self): self.assertEqual(resp.data.decode("utf-8").count("127.0.0.1"), 2) def test_project_privacy_edit2(self): - self.change_privacy_to(LoggingMode.RECORD_IP) + self.change_privacy_to("demo", LoggingMode.RECORD_IP) resp = self.client.get("/demo/history") self.assertEqual(resp.status_code, 200) @@ -149,7 +151,7 @@ def test_project_privacy_edit2(self): self.assertEqual(resp.data.decode("utf-8").count(" -- "), 1) self.assertEqual(resp.data.decode("utf-8").count("127.0.0.1"), 1) - self.change_privacy_to(LoggingMode.DISABLED) + self.change_privacy_to("demo", LoggingMode.DISABLED) resp = self.client.get("/demo/history") self.assertEqual(resp.status_code, 200) @@ -159,7 +161,7 @@ def test_project_privacy_edit2(self): self.assertEqual(resp.data.decode("utf-8").count(" -- "), 1) self.assertEqual(resp.data.decode("utf-8").count("127.0.0.1"), 2) - self.change_privacy_to(LoggingMode.ENABLED) + self.change_privacy_to("demo", LoggingMode.ENABLED) resp = self.client.get("/demo/history") self.assertEqual(resp.status_code, 200) @@ -171,6 +173,7 @@ def do_misc_database_operations(self, logging_mode): new_data = { "name": "demo2", "contact_email": "demo2@notmyidea.org", + "current_password": "demo", "password": "123456", "default_currency": "USD", } @@ -233,7 +236,7 @@ def do_misc_database_operations(self, logging_mode): def test_disable_clear_no_new_records(self): # Disable logging - self.change_privacy_to(LoggingMode.DISABLED) + self.change_privacy_to("demo", LoggingMode.DISABLED) # Ensure we can't clear history with a GET or with a password-less POST resp = self.client.get("/demo/erase_history") @@ -276,13 +279,13 @@ def test_disable_clear_no_new_records(self): def test_clear_ip_records(self): # Enable IP Recording - self.change_privacy_to(LoggingMode.RECORD_IP) + self.change_privacy_to("demo", LoggingMode.RECORD_IP) # Do lots of database operations to generate IP address entries self.do_misc_database_operations(LoggingMode.RECORD_IP) # Disable IP Recording - self.change_privacy_to(LoggingMode.ENABLED) + self.change_privacy_to("123456", LoggingMode.ENABLED) resp = self.client.get("/demo/history") self.assertEqual(resp.status_code, 200) From b99248ffa64f3159030479dd981fd7480a31d920 Mon Sep 17 00:00:00 2001 From: Baptiste Jonglez Date: Fri, 28 Jul 2023 18:16:00 +0200 Subject: [PATCH 2/5] Better feedback when changing project settings We now display a success flash message, and we stay on the same page (instead of redirecting to the list of bills, which makes little sense) --- ihatemoney/web.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/ihatemoney/web.py b/ihatemoney/web.py index e97b8fc37..8ea187790 100644 --- a/ihatemoney/web.py +++ b/ihatemoney/web.py @@ -429,7 +429,8 @@ def edit_project(): db.session.add(project) db.session.commit() - return redirect(url_for("main.list_bills")) + flash(_("Project settings have been changed successfully.")) + return redirect(url_for("main.edit_project")) else: edit_form.name.data = g.project.name From 24d3980977185b2dbe8a4c9713c84f8024dd6606 Mon Sep 17 00:00:00 2001 From: Baptiste Jonglez Date: Fri, 28 Jul 2023 18:34:54 +0200 Subject: [PATCH 3/5] Invite page: document the security implication of all options Also move the "invitation link" option first, because it's the preferred way to give access to people that only need to handle participants and bills. Sharing the identifier and private becomes the last option, because it gives full access to changing settings. --- ihatemoney/templates/send_invites.html | 29 ++++++++++++++------------ ihatemoney/tests/budget_test.py | 2 +- 2 files changed, 17 insertions(+), 14 deletions(-) diff --git a/ihatemoney/templates/send_invites.html b/ihatemoney/templates/send_invites.html index cf6797b7c..46a2f2399 100644 --- a/ihatemoney/templates/send_invites.html +++ b/ihatemoney/templates/send_invites.html @@ -7,20 +7,10 @@

{{ _("Invite people to join this project") }}

-

{{ _('Share Identifier & code') }}

- - - {{ _("You can share the project identifier and the private code by any communication means.") }} -
- {{ _('Identifier:') }} {{ g.project.id }} - - - - -

{{ _('Share the Link') }}

+

{{ _('Share an invitation link') }}

- {{ _("You can directly share the following link via your prefered medium") }}
+ {{ _("The easiest way to invite people is to give them the following invitation link.
They will be able to access the project, manage participants, add/edit/delete bills. However, they will not have access to important settings such as changing the private code or deleting the whole project.") }}
{{ url_for(".join_project", _external=True, project_id=g.project.id, token=g.project.generate_token()) }} @@ -41,13 +31,26 @@

{{ _('Send via Emails') }}

{{ _("Specify a (comma separated) list of email adresses you want to notify about the - creation of this budget management project and we will send them an email for you.") }}

+ creation of this budget management project and we will send them an email with the invitation link.") }}

{% include "display_errors.html" %}
{{ forms.invites(form) }}
+ + +

{{ _('Share Identifier & code') }}

+ + +

{{ _("You can share the project identifier and the private code by any communication means.
Anyone with the private code will have access to the full project, including changing settings such as the private code or project email address, or even deleting the whole project.") }}

+

+ {{ _('Identifier:') }} {{ g.project.id }} +
+ {{ _('Private code:') }} {{ _('the private code was defined when you created the project') }} +

+ + diff --git a/ihatemoney/tests/budget_test.py b/ihatemoney/tests/budget_test.py index 1b9792236..89efeb2da 100644 --- a/ihatemoney/tests/budget_test.py +++ b/ihatemoney/tests/budget_test.py @@ -167,7 +167,7 @@ def test_invite_code_invalidation(self): self.login("raclette") self.post_project("raclette") response = self.client.get("/raclette/invite").data.decode("utf-8") - link = extract_link(response, "share the following link") + link = extract_link(response, "give them the following invitation link") self.client.post("/exit") response = self.client.get(link) From b9e30d3ae1be8a19ecc706be18caa832316f64bf Mon Sep 17 00:00:00 2001 From: Baptiste Jonglez Date: Sat, 29 Jul 2023 11:59:19 +0200 Subject: [PATCH 4/5] API docs: new current_password field --- docs/api.md | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/docs/api.md b/docs/api.md index 9c94839b4..0ae34073e 100644 --- a/docs/api.md +++ b/docs/api.md @@ -34,9 +34,9 @@ the token (of course, you need to authenticate): $ curl --basic -u demo:demo https://ihatemoney.org/api/projects/demo/token {"token": "WyJ0ZXN0Il0.Rt04fNMmxp9YslCRq8hB6jE9s1Q"} -Make sure to store this token securely: it allows full access to the +Make sure to store this token securely: it allows almost full access to the project. For instance, use it to obtain information about the project -(replace PROJECT_TOKEN with the actual token): +(replace `PROJECT_TOKEN` with the actual token): $ curl --oauth2-bearer "PROJECT_TOKEN" https://ihatemoney.org/api/projects/demo @@ -51,7 +51,8 @@ simply create an URL of the form: https://ihatemoney.org/demo/join/PROJECT_TOKEN -Such a link grants full access to the project associated with the token. +Such a link grants read-write access to the project associated with the token, +but it does not allow to change project settings. ### Projects @@ -67,8 +68,8 @@ A project needs the following arguments: - `name`: the project name (string) - `id`: the project identifier (string without special chars or spaces) -- `password`: the project password / secret code (string) -- `contact_email`: the contact email (string) +- `password`: the project password / private code (string) +- `contact_email`: the contact email, used to recover the private code (string) Optional arguments: @@ -83,7 +84,9 @@ Here is the command: -d 'name=yay&id=yay&password=yay&contact_email=yay@notmyidea.org' "yay" -As you can see, the API returns the identifier of the project. +As you can see, the API returns the identifier of the project. It might be different +from what you requested, because the ID is normalized (remove special characters, +change to lowercase, etc). #### Getting information about the project @@ -108,7 +111,12 @@ Updating a project is done with the `PUT` verb: $ curl --basic -u yay:yay -X PUT\ https://ihatemoney.org/api/projects/yay -d\ - 'name=yay&id=yay&password=yay&contact_email=youpi@notmyidea.org' + 'name=yay&id=yay¤t_password=yay&password=newyay&contact_email=youpi@notmyidea.org' + +You need to give the current private code as the `current_password` field. This is a security +measure to ensure that knowledge of an auth token is not enough to update settings. + +Note that in any case you can never change the ID of a project. #### Deleting a project From 451640ea49f5ea1a9e1368c6e3d9308c6d1347a3 Mon Sep 17 00:00:00 2001 From: Baptiste Jonglez Date: Sat, 29 Jul 2023 12:00:22 +0200 Subject: [PATCH 5/5] security docs: Clarify what is possible with a token --- docs/security.md | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/docs/security.md b/docs/security.md index d4913856e..c8d215878 100644 --- a/docs/security.md +++ b/docs/security.md @@ -26,20 +26,25 @@ A project has four main parameters when it comes to security: Somebody with the **private code** can: - access the project through the web interface or the API +- add, modify or remove participants - add, modify or remove bills +- view statistics of the project - view project history - change basic settings of the project - change the email address associated to the project - change the private code of the project +- delete the project -Somebody with the **auth token** can manipulate the project through the API to do -essentially the same thing: +Somebody with the **auth token** can manipulate the project through the API: - access the project +- add, modify or remove participants - add, modify or remove bills -- change basic settings of the project -- change the email address associated to the project -- change the private code of the project +- view statistics of the project +- delete the project + +The auth token is not enough to change basic settings of the project, +or to change the email address or the private code. The auth token can also be used to build "invitation links". These links allow to login on the web interface without knowing the private code, @@ -61,9 +66,12 @@ The second method is interesting because it does not reveal the private code. In particular, somebody that is logged-in through the invitation link will not be able to change the private code, because the web interface requires a confirmation of the existing private code to change -it. However, a motivated person could extract the auth token from the +it. Similarly, changing other important settings or deleting the project +from the web interface requires knowledge of the private code. + +However, a motivated person could extract the auth token from the invitation link, use it to access the project through the API, and -change the private code through the API. +delete the project through the API. This is a [known issue](https://github.com/spiral-project/ihatemoney/issues/1206). ## Removing access to a project @@ -103,6 +111,6 @@ Note, however, that the history feature is primarily meant to protect against mistakes: a malicious member can easily remove all entries from the history! -The best defense against this kind of issues is\... backups! All data +The best defense against this kind of issues is... backups! All data for a project can be exported through the settings page or through the -API. +API. The server administrator can also backup the database.