Fix several small security and robustness issues

Login:
- One message for unknown email, wrong password and deactivated
  account. Before, "This account is not enabled!" only appeared after
  the right password and confirmed valid credentials.
- Email and password must be non-empty strings. A missing password
  field became the string "None" and logged in a user with that
  password.

Forms:
- "Create user" no longer renders the password back into the HTML after
  a validation error.
- Password fields for a new password use autocomplete="new-password",
  so a password manager does not fill in the admin's own password. The
  email fields on the create pages use autocomplete="off".
- Email addresses are limited to 64 characters in the local part and
  254 in total (RFC 5321). Longer ones caused a database error and
  HTTP 500.
- alerts.html no longer translates flash messages a second time. Running
  str.format() again on an already formatted message was fragile and
  logged "Unknown translation" for every German message. All flashes
  now translate when flashing, "Invalid data" included.

Debug mode:
- The Docker image sets MAX_PRODUCTION=1, and the app refuses to start
  in debug mode then. FLASK_DEBUG=1 in production would otherwise enable
  the fixed secret key, cookies without Secure, no HSTS and query
  arguments (including password hashes) in the log.
- make run-dev and run-dev-as-prod bind to 127.0.0.1.

Other:
- A denied access is logged with the logger at debug level instead of
  print. PermissionDenied loses its unused message, it is deliberately
  answered with a 404.
- Fix the typo in /user/<id>/ser-toggle-can-use-different-alias-domain.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Dockerfile

@@ -26,6 +26,8 @@ WORKDIR /app
 
 ENV PYTHONPATH=/app/
 ENV FLASK_APP=max.app
+# Refuses to start in debug mode, see max/app.py
+ENV MAX_PRODUCTION=1
 
 # Owned by root, so the app cannot modify its own code
 COPY max/ ./max

Makefile

@@ -6,10 +6,10 @@ clear-dev:
 	scripts/dev/clear-db.sh
 
 run-dev:
-	FLASK_APP=max.app FLASK_DEBUG=1 flask run --host 0.0.0.0 --port 8757
+	FLASK_APP=max.app FLASK_DEBUG=1 flask run --host 127.0.0.1 --port 8757
 
 run-dev-as-prod:
-	FLASK_APP=max.app MAX_ORIGIN=http://localhost:8757 gunicorn -w 1 -b 0.0.0.0:8757 max.app:app
+	FLASK_APP=max.app MAX_ORIGIN=http://localhost:8757 gunicorn -w 1 -b 127.0.0.1:8757 max.app:app
 
 run-cleanup:
 	scripts/dev/cleanup.sh

README.md

@@ -14,6 +14,8 @@ Start with `make run-dev`
 
 - `MAX_ORIGIN` (required in production): The public origin of MAX, e.g.
   `https://max.finn.st`
+- `MAX_PRODUCTION=1` is set in the Docker image and makes MAX refuse to start
+  in debug mode (`FLASK_DEBUG`). Do not override it.
 
 ## Dependencies
 

max/app.py

@@ -30,6 +30,10 @@ def main():
     app = Flask(__name__)
     init_logging(app)
     app.logger.info("Max started.")
+    if app.debug and os.environ.get("MAX_PRODUCTION") == "1":
+        raise RuntimeError(
+            "Debug mode is not allowed in production (MAX_PRODUCTION=1), unset FLASK_DEBUG"
+        )
     if app.debug:
         app.logger.info("!! App is in debug mode !!")
     else:

max/auth/auth.py

@@ -10,18 +10,24 @@ from max.sessions import session_storage
 from max.translations import t
 
 SSHA512_PREFIX = "{SSHA512}"
+LOGIN_FAILED_MESSAGE = "Email or password is wrong, or the account is deactivated"
 
 
 def login(email, password):
+    if not isinstance(email, str) or not isinstance(password, str):
+        return t(LOGIN_FAILED_MESSAGE)
+    if not email or not password:
+        return t(LOGIN_FAILED_MESSAGE)
+
     user = get_user_by_email(email)
     if user is None:
-        return t("Email or password was wrong!")
+        return t(LOGIN_FAILED_MESSAGE)
 
     if not check_password(password, user.passwordhash):
-        return t("Email or password was wrong!")
+        return t(LOGIN_FAILED_MESSAGE)
 
     if not user.enabled:
-        return t("This account is not enabled!")
+        return t(LOGIN_FAILED_MESSAGE)
 
     # Start a new session (new id, new CSRF token) against session fixation.
     session.clear()

max/exceptions.py

@@ -15,6 +15,7 @@ class SessionWithoutUser(MaxException):
 
 
 class PermissionDenied(MaxException):
-    def __init__(self, message):
-        super().__init__()
-        self.message = message
+    """
+    Deliberately answered with a 404, like NotFound, so nobody can tell
+    whether the object exists.
+    """

max/private_static/i18n/de.json

@@ -38,8 +38,7 @@
     "Delete alias {alias}": "Lösche Alias {alias}",
     "Alias {alias} for {user}": "Alias {alias} für {user}",
     "Edit note for alias {alias}": "Notiz für Alias {alias} bearbeiten",
-    "Email or password was wrong!": "Email oder Password ist falsch!",
-    "This account is not enabled!": "Dieser Account ist nicht aktiviert!",
+    "Email or password is wrong, or the account is deactivated": "E-Mail oder Passwort falsch, oder der Account ist deaktiviert",
     "Login was sucessfull!": "Erfolgreich angemeldet!",
     "Logout was sucessfull!": "Erfolgreich abgemeldet!",
     "The email is not valid": "Die Email ist nicht valide",
@@ -67,6 +66,8 @@
     "Used as an user.": "Wird für einen Nutzer verwendet",
     "Used as an alias by {alias}.": "Word für den Alias {alias} verwendet",
     "Invalid domain": "Invalide Domain",
+    "Invalid data": "Ungültige Daten",
+    "The email is too long": "Die E-Mail ist zu lang",
     "Can use different alias domains": "Kann unterschiedliche Aliasdomains benutzen",
     "Can use different alias domains is not set": "Kann unterschiedliche Aliasdomains benutzen ist nicht gesetzt",
     "This address is reserved and can only be created by an admin.": "Diese Adresse ist reserviert und kann nur von einem Admin angelegt werden."

max/routes/__init__.py

@@ -46,7 +46,7 @@ def init_routes(app):
         UserToggleAdmin.as_view(),
     )
     app.add_url_rule(
-        "/user/<int:user_id>/ser-toggle-can-use-different-alias-domain",
+        "/user/<int:user_id>/toggle-can-use-different-alias-domain",
         "user-toggle-can-use-different-alias-domain",
         UserToggleCanUseDifferentAliasDomain.as_view(),
     )

max/routes/alias/edit_note.py

@@ -16,7 +16,7 @@ class AliasEditNote(AllowAdminOrSelf, BackToAliasesMixin, FetchAliasTemplateGetV
     def post(self):
         note = self.request.form.get("note", "")
         if not isinstance(note, str):
-            flash("invalid data", category="error")
+            flash(t("Invalid data"), category="error")
             return
 
         update_alias(self.alias.id, note=note)

max/routes/check_email_mixin.py

@@ -5,6 +5,10 @@ from flask import flash
 from max.db import get_domains
 from max.translations import t
 
+# RFC 5321, both below the varchar(255) of emails.email
+MAX_LOCAL_PART_LENGTH = 64
+MAX_EMAIL_LENGTH = 254
+
 RESERVED_LOCAL_PARTS = frozenset(
     (
         "admin",
@@ -24,6 +28,11 @@ RESERVED_LOCAL_PARTS = frozenset(
 class CheckEmailMixin:
     def is_email_valid(self, email: str):
         """Checks for the email being valid and that it has a valid domain."""
+        local_part = email.split("@")[0]
+        if len(local_part) > MAX_LOCAL_PART_LENGTH or len(email) > MAX_EMAIL_LENGTH:
+            flash(t("The email is too long"), category="error")
+            return False
+
         domains = "|".join(map(re.escape, get_domains()))
         regex = re.compile(f"^[a-zA-Z0-9+_-]+@({domains})$")
         match = regex.match(email)

max/routes/user/change_password.py

@@ -29,7 +29,7 @@ class UserChangePassword(AllowAdminOrSelf, BackToUsersMixin, FetchUserTemplateGe
             return
 
         if not isinstance(pw1, str) or not isinstance(pw2, str):
-            flash("Invalid data", category="error")
+            flash(t("Invalid data"), category="error")
             return
 
         if self.is_own_password():

max/routes/user/create.py

@@ -42,7 +42,7 @@ class UserCreate(AllowAdmin, CheckEmailMixin, BaseTemplateGetView):
             flash(t("Both passwords must be given"), category="error")
             error = True
         elif not isinstance(pw1, str) or not isinstance(pw2, str):
-            flash("Invalid data")
+            flash(t("Invalid data"), category="error")
             error = True
         elif pw1 != pw2:
             flash(t("Both passwords must be the same"), category="error")
@@ -120,7 +120,6 @@ class UserCreate(AllowAdmin, CheckEmailMixin, BaseTemplateGetView):
         for key in (
             "email",
             "domain",
-            "password",
             "enabled",
             "is_admin",
             "can_use_different_alias_domain",

max/routes/user/edit_note.py

@@ -16,7 +16,7 @@ class UserEditNote(AllowAdmin, BackToUsersMixin, FetchUserTemplateGetView):
     def post(self):
         note = self.request.form.get("note", "")
         if not isinstance(note, str):
-            flash("invalid data", category="error")
+            flash(t("Invalid data"), category="error")
             return
 
         update_user(self.user.id, note=note)

max/templates/alerts.html

@@ -1,3 +1,3 @@
 {% for category, message in get_flashed_messages(with_categories=true) %}
-<div class="msg-{{ category }}">{{ t(message) }}</div>
+<div class="msg-{{ category }}">{{ message }}</div>
 {% endfor %}

max/templates/alias/create.html

@@ -7,7 +7,7 @@
     <form id="form" class="pure-form pure-form-aligned" action="{{ url_for('alias-create', user_id=user.id) }}" method="post">
         <div class="pure-control-group">
             <label for="email">{{ t("Email") }}</label>
-            <input id="email" type="text" name="email" {% if email is defined %}value="{{ email }}"{% endif %} placeholder="{{ t("Email") }}">
+            <input id="email" type="text" name="email" {% if email is defined %}value="{{ email }}"{% endif %} autocomplete="off" placeholder="{{ t("Email") }}">
 
             <div class="domain-selector">
                 {% if can_use_different_alias_domain %}

max/templates/user/create.html

@@ -7,7 +7,7 @@
     <form id="form" class="pure-form pure-form-aligned" action="{{ url_for('user-create') }}" method="post">
         <div class="pure-control-group">
             <label for="email">{{ t("Email") }}</label>
-            <input id="email" type="text" name="email" {% if email is defined %}value="{{ email }}"{% endif %} placeholder="{{ t("Email") }}">
+            <input id="email" type="text" name="email" {% if email is defined %}value="{{ email }}"{% endif %} autocomplete="off" placeholder="{{ t("Email") }}">
 
             <div class="domain-selector">
                 @
@@ -19,12 +19,12 @@
 
         <div class="pure-control-group">
             <label for="password1">{{ t("Password") }}</label>
-            <input id="password1" type="password" name="password1" {% if password is defined %}value="{{ password }}"{% endif %} placeholder="{{ t("Password") }}">
+            <input id="password1" type="password" name="password1" autocomplete="new-password" placeholder="{{ t("Password") }}">
         </div>
 
         <div class="pure-control-group">
             <label for="password2">{{ t("Reenter Password") }}</label>
-            <input id="password2" type="password" name="password2" {% if password is defined %}value="{{ password }}"{% endif %} placeholder="{{ t("Reenter Password") }}">
+            <input id="password2" type="password" name="password2" autocomplete="new-password" placeholder="{{ t("Reenter Password") }}">
         </div>
 
         <div class="pure-controls checkbox">

max/views.py

@@ -1,6 +1,12 @@
 from functools import update_wrapper
 
-from flask import current_app, redirect as flask_redirect, render_template, request
+from flask import (
+    current_app,
+    redirect as flask_redirect,
+    render_template,
+    request,
+    session,
+)
 from werkzeug.exceptions import MethodNotAllowed
 
 from max.exceptions import ImproperlyConfigured, PermissionDenied
@@ -72,11 +78,13 @@ class BaseView:
 
     def check_access(self):
         if not self.can_access():
-            print("Cannot access for ", self.__class__.__name__)
-            raise PermissionDenied(
-                "Not found"
-            )  # we do not want to pass PermissionDenied directly to the user.
-            # TODO: Better message.
+            self.app.logger.debug(
+                "Access denied for user %s to %s (%s)",
+                session.get("user_id"),
+                self.request.path,
+                self.__class__.__name__,
+            )
+            raise PermissionDenied()
 
     def can_access(self):
         return False