From 2b1160862aea3de823594c147f83b493207a9824 Mon Sep 17 00:00:00 2001 From: Finn Stutzenstein Date: Wed, 21 Oct 2026 20:04:22 +0000 Subject: [PATCH] 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//ser-toggle-can-use-different-alias-domain. Co-Authored-By: Claude Opus 5.5 --- Dockerfile | 2 ++ Makefile | 4 ++-- README.md | 2 ++ max/app.py | 4 ++++ max/auth/auth.py | 12 +++++++++--- max/exceptions.py | 7 ++++--- max/private_static/i18n/de.json | 5 +++-- max/routes/__init__.py | 2 +- max/routes/alias/edit_note.py | 2 +- max/routes/check_email_mixin.py | 9 +++++++++ max/routes/user/change_password.py | 2 +- max/routes/user/create.py | 3 +-- max/routes/user/edit_note.py | 2 +- max/templates/alerts.html | 2 +- max/templates/alias/create.html | 2 +- max/templates/user/create.html | 6 +++--- max/views.py | 20 ++++++++++++++------ 17 files changed, 59 insertions(+), 27 deletions(-) diff --git a/Dockerfile b/Dockerfile index 1e15bc6..c65665f 100644 --- a/Dockerfile +++ b/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 diff --git a/Makefile b/Makefile index bcbcea1..c1c67eb 100644 --- a/Makefile +++ b/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 diff --git a/README.md b/README.md index a42430a..467e025 100644 --- a/README.md +++ b/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 diff --git a/max/app.py b/max/app.py index bebbd2f..7453a3a 100644 --- a/max/app.py +++ b/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: diff --git a/max/auth/auth.py b/max/auth/auth.py index da1e353..4bb1c29 100644 --- a/max/auth/auth.py +++ b/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() diff --git a/max/exceptions.py b/max/exceptions.py index 708e4c4..80a9564 100644 --- a/max/exceptions.py +++ b/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. + """ diff --git a/max/private_static/i18n/de.json b/max/private_static/i18n/de.json index 2f20b1f..3dbce9e 100644 --- a/max/private_static/i18n/de.json +++ b/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." diff --git a/max/routes/__init__.py b/max/routes/__init__.py index f18309d..c241002 100644 --- a/max/routes/__init__.py +++ b/max/routes/__init__.py @@ -46,7 +46,7 @@ def init_routes(app): UserToggleAdmin.as_view(), ) app.add_url_rule( - "/user//ser-toggle-can-use-different-alias-domain", + "/user//toggle-can-use-different-alias-domain", "user-toggle-can-use-different-alias-domain", UserToggleCanUseDifferentAliasDomain.as_view(), ) diff --git a/max/routes/alias/edit_note.py b/max/routes/alias/edit_note.py index 944cac0..754462f 100644 --- a/max/routes/alias/edit_note.py +++ b/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) diff --git a/max/routes/check_email_mixin.py b/max/routes/check_email_mixin.py index f1da315..168dab0 100644 --- a/max/routes/check_email_mixin.py +++ b/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) diff --git a/max/routes/user/change_password.py b/max/routes/user/change_password.py index 90b119a..1654362 100644 --- a/max/routes/user/change_password.py +++ b/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(): diff --git a/max/routes/user/create.py b/max/routes/user/create.py index e1133fb..af06428 100644 --- a/max/routes/user/create.py +++ b/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", diff --git a/max/routes/user/edit_note.py b/max/routes/user/edit_note.py index 6320ed2..8f6cf60 100644 --- a/max/routes/user/edit_note.py +++ b/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) diff --git a/max/templates/alerts.html b/max/templates/alerts.html index 437900a..190d8f0 100644 --- a/max/templates/alerts.html +++ b/max/templates/alerts.html @@ -1,3 +1,3 @@ {% for category, message in get_flashed_messages(with_categories=true) %} -
{{ t(message) }}
+
{{ message }}
{% endfor %} \ No newline at end of file diff --git a/max/templates/alias/create.html b/max/templates/alias/create.html index f0b1b36..65406ab 100644 --- a/max/templates/alias/create.html +++ b/max/templates/alias/create.html @@ -7,7 +7,7 @@
- +
{% if can_use_different_alias_domain %} diff --git a/max/templates/user/create.html b/max/templates/user/create.html index 461d7ab..7d4c01a 100644 --- a/max/templates/user/create.html +++ b/max/templates/user/create.html @@ -7,7 +7,7 @@
- +
@ @@ -19,12 +19,12 @@
- +
- +
diff --git a/max/views.py b/max/views.py index 5a9a685..ab24da8 100644 --- a/max/views.py +++ b/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