From d324e2fa79c5bad2faa9e0cd657b824f75ff05c8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adri=C3=A0=20Casaj=C3=BAs?= Date: Thu, 27 Oct 2022 10:04:47 +0200 Subject: [PATCH] Fix: Add csrf verification to directory updates (#1358) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Fix: Add csrf verification to directory updates * Update templates/dashboard/directory.html * Added csrf for delete account form * Fix tests * Added CSRF check for settings page * Added csrf to batch import * Added CSRF to alias dashboard and alias transfer * Added csrf to contact manager * Added csrf to mailbox * Added csrf for mailbox detail * Added csrf to domain detail * Lint Co-authored-by: Adrià Casajús --- app/dashboard/views/alias_contact_manager.py | 7 +- app/dashboard/views/alias_transfer.py | 6 ++ app/dashboard/views/batch_import.py | 15 +++- app/dashboard/views/delete_account.py | 15 +++- app/dashboard/views/directory.py | 83 ++++++++++++++----- app/dashboard/views/domain_detail.py | 15 +++- app/dashboard/views/index.py | 6 ++ app/dashboard/views/mailbox.py | 6 ++ app/dashboard/views/mailbox_detail.py | 6 +- app/dashboard/views/setting.py | 14 +++- app/utils.py | 5 ++ .../dashboard/alias_contact_manager.html | 2 + templates/dashboard/alias_transfer_send.html | 5 +- templates/dashboard/batch_import.html | 1 + templates/dashboard/delete_account.html | 1 + templates/dashboard/directory.html | 26 +++--- templates/dashboard/domain_detail/dns.html | 5 ++ templates/dashboard/domain_detail/info.html | 5 ++ templates/dashboard/domain_detail/trash.html | 2 + templates/dashboard/index.html | 6 ++ templates/dashboard/mailbox.html | 2 + templates/dashboard/mailbox_detail.html | 6 ++ templates/dashboard/setting.html | 28 +++++-- tests/dashboard/test_directory.py | 31 ++++--- 24 files changed, 235 insertions(+), 63 deletions(-) diff --git a/app/dashboard/views/alias_contact_manager.py b/app/dashboard/views/alias_contact_manager.py index f5c54321..a4ea49a5 100644 --- a/app/dashboard/views/alias_contact_manager.py +++ b/app/dashboard/views/alias_contact_manager.py @@ -25,7 +25,7 @@ from app.errors import ( ) from app.log import LOG from app.models import Alias, Contact, EmailLog, User -from app.utils import sanitize_email +from app.utils import sanitize_email, CSRFValidationForm def email_validator(): @@ -258,8 +258,12 @@ def alias_contact_manager(alias_id): return redirect(url_for("dashboard.index")) new_contact_form = NewContactForm() + csrf_form = CSRFValidationForm() if request.method == "POST": + if not csrf_form.validate(): + flash("Invalid request", "warning") + return redirect(request.url) if request.form.get("form-name") == "create": if new_contact_form.validate(): contact_address = new_contact_form.email.data.strip() @@ -323,4 +327,5 @@ def alias_contact_manager(alias_id): query=query, nb_contact=nb_contact, can_create_contacts=user_can_create_contacts(current_user), + csrf_form=csrf_form, ) diff --git a/app/dashboard/views/alias_transfer.py b/app/dashboard/views/alias_transfer.py index d8750fe4..ec2b1528 100644 --- a/app/dashboard/views/alias_transfer.py +++ b/app/dashboard/views/alias_transfer.py @@ -22,6 +22,7 @@ from app.models import ( ClientUser, ) from app.models import Mailbox +from app.utils import CSRFValidationForm def transfer(alias, new_user, new_mailboxes: [Mailbox]): @@ -105,8 +106,12 @@ def alias_transfer_send_route(alias_id): return redirect(url_for("dashboard.index")) alias_transfer_url = None + csrf_form = CSRFValidationForm() if request.method == "POST": + if not csrf_form.validate(): + flash("Invalid request", "warning") + return redirect(request.url) # generate a new transfer_token if request.form.get("form-name") == "create": transfer_token = f"{alias.id}.{secrets.token_urlsafe(32)}" @@ -133,6 +138,7 @@ def alias_transfer_send_route(alias_id): alias_transfer_url=alias_transfer_url, link_active=alias.transfer_token_expiration is not None and alias.transfer_token_expiration > arrow.utcnow(), + csrf_form=csrf_form, ) diff --git a/app/dashboard/views/batch_import.py b/app/dashboard/views/batch_import.py index c8a81af1..6d064a71 100644 --- a/app/dashboard/views/batch_import.py +++ b/app/dashboard/views/batch_import.py @@ -8,7 +8,7 @@ from app.dashboard.base import dashboard_bp from app.db import Session from app.log import LOG from app.models import File, BatchImport, Job -from app.utils import random_string +from app.utils import random_string, CSRFValidationForm @dashboard_bp.route("/batch_import", methods=["GET", "POST"]) @@ -29,14 +29,21 @@ def batch_import_route(): user_id=current_user.id, processed=False ).all() + csrf_form = CSRFValidationForm() + if request.method == "POST": + if not csrf_form.validate(): + flash("Invalid request", "warning") + redirect(request.url) if len(batch_imports) > 10: flash( "You have too many imports already. Wait until some get cleaned up", "error", ) return render_template( - "dashboard/batch_import.html", batch_imports=batch_imports + "dashboard/batch_import.html", + batch_imports=batch_imports, + csrf_form=csrf_form, ) alias_file = request.files["alias-file"] @@ -66,4 +73,6 @@ def batch_import_route(): return redirect(url_for("dashboard.batch_import_route")) - return render_template("dashboard/batch_import.html", batch_imports=batch_imports) + return render_template( + "dashboard/batch_import.html", batch_imports=batch_imports, csrf_form=csrf_form + ) diff --git a/app/dashboard/views/delete_account.py b/app/dashboard/views/delete_account.py index 42c964ef..b3281b1e 100644 --- a/app/dashboard/views/delete_account.py +++ b/app/dashboard/views/delete_account.py @@ -1,6 +1,7 @@ import arrow from flask import flash, redirect, url_for, request, render_template from flask_login import login_required, current_user +from flask_wtf import FlaskForm from app.config import JOB_DELETE_ACCOUNT from app.dashboard.base import dashboard_bp @@ -9,11 +10,21 @@ from app.log import LOG from app.models import Subscription, Job +class DeleteDirForm(FlaskForm): + pass + + @dashboard_bp.route("/delete_account", methods=["GET", "POST"]) @login_required @sudo_required def delete_account(): + delete_form = DeleteDirForm() if request.method == "POST" and request.form.get("form-name") == "delete-account": + if not delete_form.validate(): + flash("Invalid request", "warning") + return render_template( + "dashboard/delete_account.html", delete_form=delete_form + ) sub: Subscription = current_user.get_paddle_subscription() # user who has canceled can also re-subscribe if sub and not sub.cancelled: @@ -36,6 +47,4 @@ def delete_account(): ) return redirect(url_for("dashboard.setting")) - return render_template( - "dashboard/delete_account.html", - ) + return render_template("dashboard/delete_account.html", delete_form=delete_form) diff --git a/app/dashboard/views/directory.py b/app/dashboard/views/directory.py index cb130f5a..e8c9a7ab 100644 --- a/app/dashboard/views/directory.py +++ b/app/dashboard/views/directory.py @@ -1,7 +1,13 @@ from flask import render_template, request, redirect, url_for, flash from flask_login import login_required, current_user from flask_wtf import FlaskForm -from wtforms import StringField, validators +from wtforms import ( + StringField, + validators, + SelectMultipleField, + BooleanField, + IntegerField, +) from app.config import ( EMAIL_DOMAIN, @@ -21,6 +27,22 @@ class NewDirForm(FlaskForm): ) +class ToggleDirForm(FlaskForm): + directory_id = IntegerField(validators=[validators.DataRequired()]) + directory_enabled = BooleanField(validators=[]) + + +class UpdateDirForm(FlaskForm): + directory_id = IntegerField(validators=[validators.DataRequired()]) + mailbox_ids = SelectMultipleField( + validators=[validators.DataRequired()], validate_choice=False, choices=[] + ) + + +class DeleteDirForm(FlaskForm): + directory_id = IntegerField(validators=[validators.DataRequired()]) + + @dashboard_bp.route("/directory", methods=["GET", "POST"]) @login_required def directory(): @@ -33,54 +55,68 @@ def directory(): mailboxes = current_user.mailboxes() new_dir_form = NewDirForm() + toggle_dir_form = ToggleDirForm() + update_dir_form = UpdateDirForm() + update_dir_form.mailbox_ids.choices = [ + (str(mailbox.id), str(mailbox.id)) for mailbox in mailboxes + ] + delete_dir_form = DeleteDirForm() if request.method == "POST": if request.form.get("form-name") == "delete": - dir_id = request.form.get("dir-id") - dir = Directory.get(dir_id) + if not delete_dir_form.validate(): + flash(f"Invalid request", "warning") + return redirect(url_for("dashboard.directory")) + dir_obj = Directory.get(delete_dir_form.directory_id.data) - if not dir: + if not dir_obj: flash("Unknown error. Refresh the page", "warning") return redirect(url_for("dashboard.directory")) - elif dir.user_id != current_user.id: + elif dir_obj.user_id != current_user.id: flash("You cannot delete this directory", "warning") return redirect(url_for("dashboard.directory")) - name = dir.name - Directory.delete(dir_id) + name = dir_obj.name + Directory.delete(dir_obj.id) Session.commit() flash(f"Directory {name} has been deleted", "success") return redirect(url_for("dashboard.directory")) if request.form.get("form-name") == "toggle-directory": - dir_id = request.form.get("dir-id") - dir = Directory.get(dir_id) + if not toggle_dir_form.validate(): + flash(f"Invalid request", "warning") + return redirect(url_for("dashboard.directory")) + dir_id = toggle_dir_form.directory_id.data + dir_obj = Directory.get(dir_id) - if not dir or dir.user_id != current_user.id: + if not dir_obj or dir_obj.user_id != current_user.id: flash("Unknown error. Refresh the page", "warning") return redirect(url_for("dashboard.directory")) - if request.form.get("dir-status") == "on": - dir.disabled = False - flash(f"On-the-fly is enabled for {dir.name}", "success") + if toggle_dir_form.directory_enabled.data: + dir_obj.disabled = False + flash(f"On-the-fly is enabled for {dir_obj.name}", "success") else: - dir.disabled = True - flash(f"On-the-fly is disabled for {dir.name}", "warning") + dir_obj.disabled = True + flash(f"On-the-fly is disabled for {dir_obj.name}", "warning") Session.commit() return redirect(url_for("dashboard.directory")) elif request.form.get("form-name") == "update": - dir_id = request.form.get("dir-id") - dir = Directory.get(dir_id) + if not update_dir_form.validate(): + flash(f"Invalid request", "warning") + return redirect(url_for("dashboard.directory")) + dir_id = update_dir_form.directory_id.data + dir_obj = Directory.get(dir_id) - if not dir or dir.user_id != current_user.id: + if not dir_obj or dir_obj.user_id != current_user.id: flash("Unknown error. Refresh the page", "warning") return redirect(url_for("dashboard.directory")) - mailbox_ids = request.form.getlist("mailbox_ids") + mailbox_ids = update_dir_form.mailbox_ids.data # check if mailbox is not tempered with mailboxes = [] for mailbox_id in mailbox_ids: @@ -99,14 +135,14 @@ def directory(): return redirect(url_for("dashboard.directory")) # first remove all existing directory-mailboxes links - DirectoryMailbox.filter_by(directory_id=dir.id).delete() + DirectoryMailbox.filter_by(directory_id=dir_obj.id).delete() Session.flush() for mailbox in mailboxes: - DirectoryMailbox.create(directory_id=dir.id, mailbox_id=mailbox.id) + DirectoryMailbox.create(directory_id=dir_obj.id, mailbox_id=mailbox.id) Session.commit() - flash(f"Directory {dir.name} has been updated", "success") + flash(f"Directory {dir_obj.name} has been updated", "success") return redirect(url_for("dashboard.directory")) elif request.form.get("form-name") == "create": @@ -181,6 +217,9 @@ def directory(): return render_template( "dashboard/directory.html", dirs=dirs, + toggle_dir_form=toggle_dir_form, + update_dir_form=update_dir_form, + delete_dir_form=delete_dir_form, new_dir_form=new_dir_form, mailboxes=mailboxes, EMAIL_DOMAIN=EMAIL_DOMAIN, diff --git a/app/dashboard/views/domain_detail.py b/app/dashboard/views/domain_detail.py index 19727085..29089a38 100644 --- a/app/dashboard/views/domain_detail.py +++ b/app/dashboard/views/domain_detail.py @@ -28,7 +28,7 @@ from app.models import ( Job, ) from app.regex_utils import regex_match -from app.utils import random_string +from app.utils import random_string, CSRFValidationForm @dashboard_bp.route("/domains//dns", methods=["GET", "POST"]) @@ -47,6 +47,7 @@ def domain_detail_dns(custom_domain_id): spf_record = f"v=spf1 include:{EMAIL_DOMAIN} ~all" domain_validator = CustomDomainValidation(EMAIL_DOMAIN) + csrf_form = CSRFValidationForm() dmarc_record = "v=DMARC1; p=quarantine; pct=100; adkim=s; aspf=s" @@ -54,6 +55,9 @@ def domain_detail_dns(custom_domain_id): mx_errors = spf_errors = dkim_errors = dmarc_errors = ownership_errors = [] if request.method == "POST": + if not csrf_form.validate(): + flash("Invalid request", "warning") + return redirect(request.url) if request.form.get("form-name") == "check-ownership": txt_records = get_txt_record(custom_domain.domain) @@ -165,6 +169,7 @@ def domain_detail_dns(custom_domain_id): @dashboard_bp.route("/domains//info", methods=["GET", "POST"]) @login_required def domain_detail(custom_domain_id): + csrf_form = CSRFValidationForm() custom_domain: CustomDomain = CustomDomain.get(custom_domain_id) mailboxes = current_user.mailboxes() @@ -173,6 +178,9 @@ def domain_detail(custom_domain_id): return redirect(url_for("dashboard.index")) if request.method == "POST": + if not csrf_form.validate(): + flash("Invalid request", "warning") + return redirect(request.url) if request.form.get("form-name") == "switch-catch-all": custom_domain.catch_all = not custom_domain.catch_all Session.commit() @@ -301,12 +309,16 @@ def domain_detail(custom_domain_id): @dashboard_bp.route("/domains//trash", methods=["GET", "POST"]) @login_required def domain_detail_trash(custom_domain_id): + csrf_form = CSRFValidationForm() custom_domain = CustomDomain.get(custom_domain_id) if not custom_domain or custom_domain.user_id != current_user.id: flash("You cannot see this page", "warning") return redirect(url_for("dashboard.index")) if request.method == "POST": + if not csrf_form.validate(): + flash("Invalid request", "warning") + return redirect(request.url) if request.form.get("form-name") == "empty-all": DomainDeletedAlias.filter_by(domain_id=custom_domain.id).delete() Session.commit() @@ -350,6 +362,7 @@ def domain_detail_trash(custom_domain_id): "dashboard/domain_detail/trash.html", domain_deleted_aliases=domain_deleted_aliases, custom_domain=custom_domain, + csrf_form=csrf_form, ) diff --git a/app/dashboard/views/index.py b/app/dashboard/views/index.py index f74a7932..602f7360 100644 --- a/app/dashboard/views/index.py +++ b/app/dashboard/views/index.py @@ -17,6 +17,7 @@ from app.models import ( EmailLog, Contact, ) +from app.utils import CSRFValidationForm @dataclass @@ -75,8 +76,12 @@ def index(): "highlight_alias_id must be a number, received %s", request.args.get("highlight_alias_id"), ) + csrf_form = CSRFValidationForm() if request.method == "POST": + if not csrf_form.validate(): + flash("Invalid request", "warning") + return redirect(request.url) if request.form.get("form-name") == "create-custom-email": if current_user.can_create_new_alias(): return redirect(url_for("dashboard.custom_alias")) @@ -204,6 +209,7 @@ def index(): sort=sort, filter=alias_filter, stats=stats, + csrf_form=csrf_form, ) diff --git a/app/dashboard/views/mailbox.py b/app/dashboard/views/mailbox.py index 8c9e41b4..57bbfb0c 100644 --- a/app/dashboard/views/mailbox.py +++ b/app/dashboard/views/mailbox.py @@ -18,6 +18,7 @@ from app.email_utils import ( ) from app.log import LOG from app.models import Mailbox, Job +from app.utils import CSRFValidationForm class NewMailboxForm(FlaskForm): @@ -36,8 +37,12 @@ def mailbox_route(): ) new_mailbox_form = NewMailboxForm() + csrf_form = CSRFValidationForm() if request.method == "POST": + if not csrf_form.validate(): + flash("Invalid request", "warning") + return redirect(request.url) if request.form.get("form-name") == "delete": mailbox_id = request.form.get("mailbox-id") mailbox = Mailbox.get(mailbox_id) @@ -127,6 +132,7 @@ def mailbox_route(): "dashboard/mailbox.html", mailboxes=mailboxes, new_mailbox_form=new_mailbox_form, + csrf_form=csrf_form, ) diff --git a/app/dashboard/views/mailbox_detail.py b/app/dashboard/views/mailbox_detail.py index e7bb1ee5..67017022 100644 --- a/app/dashboard/views/mailbox_detail.py +++ b/app/dashboard/views/mailbox_detail.py @@ -17,7 +17,7 @@ from app.log import LOG from app.models import Alias, AuthorizedAddress from app.models import Mailbox from app.pgp_utils import PGPException, load_public_key_and_check -from app.utils import sanitize_email +from app.utils import sanitize_email, CSRFValidationForm class ChangeEmailForm(FlaskForm): @@ -35,6 +35,7 @@ def mailbox_detail_route(mailbox_id): return redirect(url_for("dashboard.index")) change_email_form = ChangeEmailForm() + csrf_form = CSRFValidationForm() if mailbox.new_email: pending_email = mailbox.new_email @@ -42,6 +43,9 @@ def mailbox_detail_route(mailbox_id): pending_email = None if request.method == "POST": + if not csrf_form.validate(): + flash("Invalid request", "warning") + return redirect(request.url) if ( request.form.get("form-name") == "update-email" and change_email_form.validate_on_submit() diff --git a/app/dashboard/views/setting.py b/app/dashboard/views/setting.py index d08375b0..a72941a2 100644 --- a/app/dashboard/views/setting.py +++ b/app/dashboard/views/setting.py @@ -53,7 +53,7 @@ from app.models import ( UnsubscribeBehaviourEnum, ) from app.proton.utils import get_proton_partner, perform_proton_account_unlink -from app.utils import random_string, sanitize_email +from app.utils import random_string, sanitize_email, CSRFValidationForm class SettingForm(FlaskForm): @@ -104,6 +104,7 @@ def setting(): form = SettingForm() promo_form = PromoCodeForm() change_email_form = ChangeEmailForm() + csrf_form = CSRFValidationForm() email_change = EmailChange.get_by(user_id=current_user.id) if email_change: @@ -112,6 +113,9 @@ def setting(): pending_email = None if request.method == "POST": + if not csrf_form.validate(): + flash("Invalid request", "warning") + return redirect(url_for("dashboard.setting")) if request.form.get("form-name") == "update-email": if change_email_form.validate(): # whether user can proceed with the email update @@ -395,6 +399,7 @@ def setting(): return render_template( "dashboard/setting.html", + csrf_form=csrf_form, form=form, PlanEnum=PlanEnum, SenderFormatEnum=SenderFormatEnum, @@ -477,9 +482,14 @@ def cancel_email_change(): return redirect(url_for("dashboard.setting")) -@dashboard_bp.route("/unlink_proton_account", methods=["GET", "POST"]) +@dashboard_bp.route("/unlink_proton_account", methods=["POST"]) @login_required def unlink_proton_account(): + csrf_form = CSRFValidationForm() + if not csrf_form.validate(): + flash("Invalid request", "warning") + return redirect(url_for("dashboard.setting")) + perform_proton_account_unlink(current_user) flash("Your Proton account has been unlinked", "success") return redirect(url_for("dashboard.setting")) diff --git a/app/utils.py b/app/utils.py index 4bf95442..0dd2cdbe 100644 --- a/app/utils.py +++ b/app/utils.py @@ -6,6 +6,7 @@ import urllib.parse from functools import wraps from typing import List, Optional +from flask_wtf import FlaskForm from unidecode import unidecode from .config import WORDS_FILE_PATH, ALLOWED_REDIRECT_DOMAINS @@ -126,3 +127,7 @@ def debug_info(func): return ret return wrap + + +class CSRFValidationForm(FlaskForm): + pass diff --git a/templates/dashboard/alias_contact_manager.html b/templates/dashboard/alias_contact_manager.html index 948564b3..a1fff07a 100644 --- a/templates/dashboard/alias_contact_manager.html +++ b/templates/dashboard/alias_contact_manager.html @@ -80,6 +80,7 @@
+ {{ csrf_form.csrf_token }} Edit ➡ + {{ csrf_form.csrf_token }} Delete diff --git a/templates/dashboard/alias_transfer_send.html b/templates/dashboard/alias_transfer_send.html index cae963ce..7dfa08cb 100644 --- a/templates/dashboard/alias_transfer_send.html +++ b/templates/dashboard/alias_transfer_send.html @@ -26,6 +26,7 @@ This transfer URL is valid for 24 hours. If it hasn't been used by then it will be automatically disabled.

+ {{ csrf_form.csrf_token }}
If you don't want to share this alias anymore, you can remove the share URL.
@@ -34,9 +35,10 @@ {% if link_active %}

- You have an active transfer link. If you don't want to share this alias any more, please delete the link. + You have an active transfer link. If you don't want to share this alias anymore, please delete the link.

+ {{ csrf_form.csrf_token }}
@@ -46,6 +48,7 @@ please create the Share URL 👇 and send it to the other person.

+ {{ csrf_form.csrf_token }}
diff --git a/templates/dashboard/batch_import.html b/templates/dashboard/batch_import.html index 83a007c9..7d3024a1 100644 --- a/templates/dashboard/batch_import.html +++ b/templates/dashboard/batch_import.html @@ -24,6 +24,7 @@ download>Download CSV Template
+ {{ csrf_form.csrf_token }} + {{ delete_form.csrf_token }}
diff --git a/templates/dashboard/directory.html b/templates/dashboard/directory.html index 500eace4..e38895cc 100644 --- a/templates/dashboard/directory.html +++ b/templates/dashboard/directory.html @@ -66,11 +66,12 @@
{{ dir.name }}
+ {{ toggle_dir_form.csrf_token }} - + {{ toggle_dir_form.directory_id( type="hidden", value=dir.id) }}
@@ -92,8 +93,9 @@
{% set dir_mailboxes=dir.mailboxes %}
+ {{ update_dir_form.csrf_token }} - + {{ update_dir_form.directory_id( type="hidden", value=dir.id) }} - - + {{ delete_dir_form.directory_id( type="hidden", value=dir.id) }} Delete
@@ -173,18 +175,20 @@