Sanitize alias, contacts, mailboxes and users before creating them (#1829)

* Sanitize alias, contacts, mailboxes and users before creating them

* Updated comments and moved crons to run when load is low

* Run the stats at the same time as previously

---------

Co-authored-by: Adrià Casajús <adria.casajus@proton.ch>

app/api/views/mailbox.py

@@ -13,8 +13,8 @@ from app.db import Session
 from app.email_utils import (
     mailbox_already_used,
     email_can_be_used_as_mailbox,
-    is_valid_email,
 )
+from app.email_validation import is_valid_email
 from app.log import LOG
 from app.models import Mailbox, Job
 from app.utils import sanitize_email

app/dashboard/views/alias_contact_manager.py

@@ -13,10 +13,10 @@ from app import config, parallel_limiter
 from app.dashboard.base import dashboard_bp
 from app.db import Session
 from app.email_utils import (
-    is_valid_email,
     generate_reply_email,
     parse_full_address,
 )
+from app.email_validation import is_valid_email
 from app.errors import (
     CannotCreateContactForReverseAlias,
     ErrContactErrorUpgradeNeeded,

app/dashboard/views/mailbox.py

@@ -19,8 +19,8 @@ from app.email_utils import (
     mailbox_already_used,
     render,
     send_email,
-    is_valid_email,
 )
+from app.email_validation import is_valid_email
 from app.log import LOG
 from app.models import Mailbox, Job
 from app.utils import CSRFValidationForm

app/email_utils.py

@@ -828,19 +828,6 @@ def should_add_dkim_signature(domain: str) -> bool:
     return False
 
 
-def is_valid_email(email_address: str) -> bool:
-    """
-    Used to check whether an email address is valid
-    NOT run MX check.
-    NOT allow unicode.
-    """
-    try:
-        validate_email(email_address, check_deliverability=False, allow_smtputf8=False)
-        return True
-    except EmailNotValidError:
-        return False
-
-
 class EmailEncoding(enum.Enum):
     BASE64 = "base64"
     QUOTED = "quoted-printable"
@@ -1116,26 +1103,6 @@ def is_reverse_alias(address: str) -> bool:
     )
 
 
-# allow also + and @ that are present in a reply address
-_ALLOWED_CHARS = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789_-.+@"
-
-
-def normalize_reply_email(reply_email: str) -> str:
-    """Handle the case where reply email contains *strange* char that was wrongly generated in the past"""
-    if not reply_email.isascii():
-        reply_email = convert_to_id(reply_email)
-
-    ret = []
-    # drop all control characters like shift, separator, etc
-    for c in reply_email:
-        if c not in _ALLOWED_CHARS:
-            ret.append("_")
-        else:
-            ret.append(c)
-
-    return "".join(ret)
-
-
 def should_disable(alias: Alias) -> (bool, str):
     """
     Return whether an alias should be disabled and if yes, the reason why

app/email_validation.py

@@ -0,0 +1,38 @@
+from email_validator import (
+    validate_email,
+    EmailNotValidError,
+)
+
+from app.utils import convert_to_id
+
+# allow also + and @ that are present in a reply address
+_ALLOWED_CHARS = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789_-.+@"
+
+
+def is_valid_email(email_address: str) -> bool:
+    """
+    Used to check whether an email address is valid
+    NOT run MX check.
+    NOT allow unicode.
+    """
+    try:
+        validate_email(email_address, check_deliverability=False, allow_smtputf8=False)
+        return True
+    except EmailNotValidError:
+        return False
+
+
+def normalize_reply_email(reply_email: str) -> str:
+    """Handle the case where reply email contains *strange* char that was wrongly generated in the past"""
+    if not reply_email.isascii():
+        reply_email = convert_to_id(reply_email)
+
+    ret = []
+    # drop all control characters like shift, separator, etc
+    for c in reply_email:
+        if c not in _ALLOWED_CHARS:
+            ret.append("_")
+        else:
+            ret.append(c)
+
+    return "".join(ret)

app/errors.py

@@ -84,6 +84,14 @@ class ErrAddressInvalid(SLException):
         return f"{self.address} is not a valid email address"
 
 
+class InvalidContactEmailError(SLException):
+    def __init__(self, website_email: str):  # noqa: F821
+        self.website_email = website_email
+
+    def error_for_user(self) -> str:
+        return f"Cannot create contact with invalid email {self.website_email}"
+
+
 class ErrContactAlreadyExists(SLException):
     """raised when a contact already exists"""
 

app/models.py

@@ -30,11 +30,13 @@ from sqlalchemy_utils import ArrowType
 from app import config
 from app import s3
 from app.db import Session
+from app.email_validation import is_valid_email, normalize_reply_email
 from app.errors import (
     AliasInTrashError,
     DirectoryInTrashError,
     SubdomainInTrashError,
     CannotCreateContactForReverseAlias,
+    InvalidContactEmailError,
 )
 from app.handler.unsubscribe_encoder import UnsubscribeAction, UnsubscribeEncoder
 from app.log import LOG
@@ -577,6 +579,7 @@ class User(Base, ModelMixin, UserMixin, PasswordOracle):
 
     @classmethod
     def create(cls, email, name="", password=None, from_partner=False, **kwargs):
+        email = sanitize_email(email)
         user: User = super(User, cls).create(email=email, name=name[:100], **kwargs)
 
         if password:
@@ -1802,11 +1805,11 @@ class Contact(Base, ModelMixin):
         commit = kw.pop("commit", False)
         flush = kw.pop("flush", False)
 
-        new_contact = cls(**kw)
-
-        website_email = kw["website_email"]
         # make sure email is lowercase and doesn't have any whitespace
-        website_email = sanitize_email(website_email)
+        website_email = sanitize_email(kw["website_email"])
+        kw["website_email"] = website_email
+        if "reply_email" in kw:
+            kw["reply_email"] = normalize_reply_email(sanitize_email(kw["reply_email"]))
 
         # make sure contact.website_email isn't a reverse alias
         if website_email != config.NOREPLY:
@@ -1814,6 +1817,10 @@ class Contact(Base, ModelMixin):
             if orig_contact:
                 raise CannotCreateContactForReverseAlias(str(orig_contact))
 
+        if not is_valid_email(website_email):
+            raise InvalidContactEmailError(website_email)
+
+        new_contact = cls(**kw)
         Session.add(new_contact)
 
         if commit:
@@ -2300,6 +2307,7 @@ class CustomDomain(Base, ModelMixin):
     @classmethod
     def create(cls, **kwargs):
         domain = kwargs.get("domain")
+        kwargs["domain"] = domain.replace("\n", "")
         if DeletedSubdomain.get_by(domain=domain):
             raise SubdomainInTrashError
 
@@ -2599,6 +2607,12 @@ class Mailbox(Base, ModelMixin):
 
         return ret
 
+    @classmethod
+    def create(cls, **kw):
+        if "email" in kw:
+            kw["email"] = sanitize_email(kw["email"])
+        return super().create(**kw)
+
     def __repr__(self):
         return f"<Mailbox {self.id} {self.email}>"
 

app/utils.py

@@ -99,7 +99,7 @@ def sanitize_email(email_address: str, not_lower=False) -> str:
         email_address = email_address.strip().replace(" ", "").replace("\n", " ")
         if not not_lower:
             email_address = email_address.lower()
-    return email_address
+    return email_address.replace("\u200f", "")
 
 
 class NextUrlSanitizer:

cron.py

@@ -22,10 +22,9 @@ from app.email_utils import (
     render,
     email_can_be_used_as_mailbox,
     send_email_with_rate_control,
-    normalize_reply_email,
-    is_valid_email,
     get_email_domain_part,
 )
+from app.email_validation import is_valid_email, normalize_reply_email
 from app.errors import ProtonPartnerNotSetUp
 from app.log import LOG
 from app.mail_sender import load_unsent_mails_from_fs_and_resend

crontab.yml

@@ -5,62 +5,64 @@ jobs:
     schedule: "0 0 * * *"
     captureStderr: true
 
-  - name: SimpleLogin Notify Trial Ends
-    command: python /code/cron.py -j notify_trial_end
-    shell: /bin/bash
-    schedule: "0 8 * * *"
-    captureStderr: true
-
-  - name: SimpleLogin Notify Manual Subscription Ends
-    command: python /code/cron.py -j notify_manual_subscription_end
-    shell: /bin/bash
-    schedule: "0 9 * * *"
-    captureStderr: true
-
-  - name: SimpleLogin Notify Premium Ends
-    command: python /code/cron.py -j notify_premium_end
-    shell: /bin/bash
-    schedule: "0 10 * * *"
-    captureStderr: true
-
-  - name: SimpleLogin Delete Logs
-    command: python /code/cron.py -j delete_logs
-    shell: /bin/bash
-    schedule: "0 11 * * *"
-    captureStderr: true
-
-  - name: SimpleLogin Poll Apple Subscriptions
-    command: python /code/cron.py -j poll_apple_subscription
-    shell: /bin/bash
-    schedule: "0 12 * * *"
-    captureStderr: true
-
   - name: SimpleLogin Delete Old Monitoring records
     command: python /code/cron.py -j delete_old_monitoring
     shell: /bin/bash
-    schedule: "0 14 * * *"
+    schedule: "15 1 * * *"
     captureStderr: true
 
   - name: SimpleLogin Custom Domain check
     command: python /code/cron.py -j check_custom_domain
     shell: /bin/bash
-    schedule: "0 15 * * *"
+    schedule: "15 2 * * *"
     captureStderr: true
 
   - name: SimpleLogin HIBP check
     command: python /code/cron.py -j check_hibp
     shell: /bin/bash
-    schedule: "0 18 * * *"
+    schedule: "15 3 * * *"
     captureStderr: true
     concurrencyPolicy: Forbid
 
   - name: SimpleLogin Notify HIBP breaches
     command: python /code/cron.py -j notify_hibp
     shell: /bin/bash
-    schedule: "0 19 * * *"
+    schedule: "15 4 * * *"
     captureStderr: true
     concurrencyPolicy: Forbid
 
+  - name: SimpleLogin Delete Logs
+    command: python /code/cron.py -j delete_logs
+    shell: /bin/bash
+    schedule: "15 5 * * *"
+    captureStderr: true
+
+  - name: SimpleLogin Poll Apple Subscriptions
+    command: python /code/cron.py -j poll_apple_subscription
+    shell: /bin/bash
+    schedule: "15 6 * * *"
+    captureStderr: true
+
+  - name: SimpleLogin Notify Trial Ends
+    command: python /code/cron.py -j notify_trial_end
+    shell: /bin/bash
+    schedule: "15 8 * * *"
+    captureStderr: true
+
+  - name: SimpleLogin Notify Manual Subscription Ends
+    command: python /code/cron.py -j notify_manual_subscription_end
+    shell: /bin/bash
+    schedule: "15 9 * * *"
+    captureStderr: true
+
+  - name: SimpleLogin Notify Premium Ends
+    command: python /code/cron.py -j notify_premium_end
+    shell: /bin/bash
+    schedule: "15 10 * * *"
+    captureStderr: true
+
+
+
   - name: SimpleLogin send unsent emails
     command: python /code/cron.py -j send_undelivered_mails
     shell: /bin/bash

email_handler.py

@@ -106,8 +106,6 @@ from app.email_utils import (
     get_header_unicode,
     generate_reply_email,
     is_reverse_alias,
-    normalize_reply_email,
-    is_valid_email,
     replace,
     should_disable,
     parse_id_from_bounce,
@@ -123,6 +121,7 @@ from app.email_utils import (
     generate_verp_email,
     sl_formataddr,
 )
+from app.email_validation import is_valid_email, normalize_reply_email
 from app.errors import (
     NonReverseAliasInReplyPhase,
     VERPTransactional,

tests/test_email_utils.py

@@ -19,10 +19,8 @@ from app.email_utils import (
     copy,
     get_spam_from_header,
     get_header_from_bounce,
-    is_valid_email,
     add_header,
     generate_reply_email,
-    normalize_reply_email,
     get_encoding,
     encode_text,
     EmailEncoding,
@@ -41,6 +39,7 @@ from app.email_utils import (
     get_verp_info_from_email,
     sl_formataddr,
 )
+from app.email_validation import is_valid_email, normalize_reply_email
 from app.models import (
     CustomDomain,
     Alias,

tests/test_models.py

@@ -78,20 +78,20 @@ def test_website_send_to(flask_client):
         user_id=user.id,
         alias_id=alias.id,
         website_email=f"{prefix}@example.com",
-        reply_email="rep@SL",
+        reply_email="rep@sl",
         name="First Last",
     )
-    assert c1.website_send_to() == f'"First Last | {prefix} at example.com" <rep@SL>'
+    assert c1.website_send_to() == f'"First Last | {prefix} at example.com" <rep@sl>'
 
     # empty name, ascii website_from, easy case
     c1.name = None
     c1.website_from = f"First Last <{prefix}@example.com>"
-    assert c1.website_send_to() == f'"First Last | {prefix} at example.com" <rep@SL>'
+    assert c1.website_send_to() == f'"First Last | {prefix} at example.com" <rep@sl>'
 
     # empty name, RFC 2047 website_from
     c1.name = None
     c1.website_from = f"=?UTF-8?B?TmjGoW4gTmd1eeG7hW4=?= <{prefix}@example.com>"
-    assert c1.website_send_to() == f'"Nhơn Nguyễn | {prefix} at example.com" <rep@SL>'
+    assert c1.website_send_to() == f'"Nhơn Nguyễn | {prefix} at example.com" <rep@sl>'
 
 
 def test_new_addr_default_sender_format(flask_client):
@@ -103,16 +103,16 @@ def test_new_addr_default_sender_format(flask_client):
         user_id=user.id,
         alias_id=alias.id,
         website_email=f"{prefix}@example.com",
-        reply_email="rep@SL",
+        reply_email="rep@sl",
         name="First Last",
         commit=True,
     )
 
-    assert contact.new_addr() == f'"First Last - {prefix} at example.com" <rep@SL>'
+    assert contact.new_addr() == f'"First Last - {prefix} at example.com" <rep@sl>'
 
     # Make sure email isn't duplicated if sender name equals email
     contact.name = f"{prefix}@example.com"
-    assert contact.new_addr() == f'"{prefix} at example.com" <rep@SL>'
+    assert contact.new_addr() == f'"{prefix} at example.com" <rep@sl>'
 
 
 def test_new_addr_a_sender_format(flask_client):
@@ -126,12 +126,12 @@ def test_new_addr_a_sender_format(flask_client):
         user_id=user.id,
         alias_id=alias.id,
         website_email=f"{prefix}@example.com",
-        reply_email="rep@SL",
+        reply_email="rep@sl",
         name="First Last",
         commit=True,
     )
 
-    assert contact.new_addr() == f'"First Last - {prefix}(a)example.com" <rep@SL>'
+    assert contact.new_addr() == f'"First Last - {prefix}(a)example.com" <rep@sl>'
 
 
 def test_new_addr_no_name_sender_format(flask_client):
@@ -145,12 +145,12 @@ def test_new_addr_no_name_sender_format(flask_client):
         user_id=user.id,
         alias_id=alias.id,
         website_email=f"{prefix}@example.com",
-        reply_email="rep@SL",
+        reply_email="rep@sl",
         name="First Last",
         commit=True,
     )
 
-    assert contact.new_addr() == "rep@SL"
+    assert contact.new_addr() == "rep@sl"
 
 
 def test_new_addr_name_only_sender_format(flask_client):
@@ -164,12 +164,12 @@ def test_new_addr_name_only_sender_format(flask_client):
         user_id=user.id,
         alias_id=alias.id,
         website_email=f"{prefix}@example.com",
-        reply_email="rep@SL",
+        reply_email="rep@sl",
         name="First Last",
         commit=True,
     )
 
-    assert contact.new_addr() == "First Last <rep@SL>"
+    assert contact.new_addr() == "First Last <rep@sl>"
 
 
 def test_new_addr_at_only_sender_format(flask_client):
@@ -183,12 +183,12 @@ def test_new_addr_at_only_sender_format(flask_client):
         user_id=user.id,
         alias_id=alias.id,
         website_email=f"{prefix}@example.com",
-        reply_email="rep@SL",
+        reply_email="rep@sl",
         name="First Last",
         commit=True,
     )
 
-    assert contact.new_addr() == f'"{prefix} at example.com" <rep@SL>'
+    assert contact.new_addr() == f'"{prefix} at example.com" <rep@sl>'
 
 
 def test_new_addr_unicode(flask_client):
@@ -200,14 +200,14 @@ def test_new_addr_unicode(flask_client):
         user_id=user.id,
         alias_id=alias.id,
         website_email=f"{random_prefix}@example.com",
-        reply_email="rep@SL",
+        reply_email="rep@sl",
         name="Nhơn Nguyễn",
         commit=True,
     )
 
     assert (
         contact.new_addr()
-        == f"=?utf-8?q?Nh=C6=A1n_Nguy=E1=BB=85n_-_{random_prefix}_at_example=2Ecom?= <rep@SL>"
+        == f"=?utf-8?q?Nh=C6=A1n_Nguy=E1=BB=85n_-_{random_prefix}_at_example=2Ecom?= <rep@sl>"
     )
 
     # sanity check