From dc85c578a2f3d67e882771c3c70cb0763257e9e1 Mon Sep 17 00:00:00 2001 From: Finn Stutzenstein Date: Wed, 21 Oct 2026 10:48:55 +0000 Subject: [PATCH] Protect against login CSRF without cookies for anonymous users Anonymous users must not get cookies or session store entries, so a CSRF token for /login is not possible. Instead, every unsafe request is checked statelessly via Sec-Fetch-Site, falling back to Origin. Requests without both headers are not from a browser and are accepted. The expected origin comes from the new MAX_ORIGIN setting, which is required outside of debug mode. Further CSRF changes: - Do not delete the CSRF token on a failed check. It added no security, but invalidated forms in open tabs and could be triggered cross-site. - Treat HEAD and OPTIONS as safe methods. Keep the login page free of sessions: - Show login messages via ?msg=login_required|logout (fixed keys only) and the login error via the template context instead of flash(). - Only send the session deletion cookie if the client sent a cookie. Co-Authored-By: Claude Opus 5.5 --- Makefile | 2 +- README.md | 5 +++ docker-compose.yml | 1 + max/app.py | 6 ++++ max/auth/middlewares.py | 4 +-- max/auth/routes.py | 18 ++++++++--- max/csrf.py | 61 ++++++++++++++++++++++++++--------- max/middlewares.py | 12 ++++--- max/sessions.py | 5 ++- max/templates/auth/login.html | 7 ++-- 10 files changed, 89 insertions(+), 32 deletions(-) diff --git a/Makefile b/Makefile index b55fea7..d6fe6fc 100644 --- a/Makefile +++ b/Makefile @@ -10,7 +10,7 @@ run-dev: run-dev-as-prod: mkdir -p log - FLASK_APP=max.app LOG_DIR=log gunicorn -w 1 -b 0.0.0.0:8757 max.app:app + FLASK_APP=max.app LOG_DIR=log MAX_ORIGIN=http://localhost:8757 gunicorn -w 1 -b 0.0.0.0:8757 max.app:app run-cleanup: scripts/dev/cleanup.sh diff --git a/README.md b/README.md index 9659c68..62e46ef 100644 --- a/README.md +++ b/README.md @@ -9,3 +9,8 @@ The proxy should add X-Real-IP to get a better logging. Requirements: Docker, Make Start with `make run-dev` + +## Configuration + +- `MAX_ORIGIN` (required in production): The public origin of MAX, e.g. + `https://max.finn.st` \ No newline at end of file diff --git a/docker-compose.yml b/docker-compose.yml index 7fe3e0b..1469f99 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -10,6 +10,7 @@ services: - PGSQL_USER=max - PGSQL_PASSWORD=max - PGSQL_NAME=max + - MAX_ORIGIN=https://max.finn.st networks: - postgresql - uplink diff --git a/max/app.py b/max/app.py index 89bf4bf..24ae8b7 100644 --- a/max/app.py +++ b/max/app.py @@ -37,7 +37,13 @@ def main(): app.jinja_env.trim_blocks = True app.jinja_env.lstrip_blocks = True app.url_rule_class = Rule + max_origin = os.environ.get("MAX_ORIGIN") + if not max_origin and not app.debug: + raise RuntimeError( + "MAX_ORIGIN must be set to the public origin, e.g. https://max.example.com" + ) app.config.update( + MAX_ORIGIN=max_origin.rstrip("/") if max_origin else None, SECRET_KEY=( "123456" if app.debug else base64.b64encode(os.urandom(64)).decode("utf-8") ), diff --git a/max/auth/middlewares.py b/max/auth/middlewares.py index b07babc..a4e1d85 100644 --- a/max/auth/middlewares.py +++ b/max/auth/middlewares.py @@ -23,12 +23,12 @@ class AuthMiddleware(BaseMiddleware): if not getattr(self.handler, "no_auth", False) and not auth_user: logout() # remove the session (if any) # No usage of flash here to not create a session... - return redirect(url_for("login", a=1)) + return redirect(url_for("login", msg="login_required")) if auth_user: if not auth_user.enabled: logout() - return redirect(url_for("login", a=1)) + return redirect(url_for("login", msg="login_required")) kwargs["auth_user"] = auth_user return self.handler(*args, **kwargs) diff --git a/max/auth/routes.py b/max/auth/routes.py index b45e2de..6011619 100644 --- a/max/auth/routes.py +++ b/max/auth/routes.py @@ -8,6 +8,12 @@ from max.views import BaseTemplateGetView, BaseView from .auth import login, logout from .middlewares import no_authentication_required +# Messages for the login page (text, category), selected by ?msg=. +LOGIN_MESSAGES = { + "login_required": ("Please login to visit the requested url", "error"), + "logout": ("Logout was sucessfull!", "success"), +} + def init_routes(app): app.add_url_rule( @@ -26,7 +32,11 @@ class Login(AllowAnyAccess, BaseTemplateGetView): def get_context(self): context = super().get_context() - context["alert"] = bool(self.request.args.get("a", False)) + if "message" not in context: + message = LOGIN_MESSAGES.get(self.request.args.get("msg")) + if message is not None: + context["message"] = t(message[0]) + context["message_category"] = message[1] return context def get_webpagetitle(self): @@ -41,7 +51,8 @@ class Login(AllowAnyAccess, BaseTemplateGetView): self.app.logger.info( "Failed login attempt from %s", self.request.real_remote_addr ) - flash(error, category="error") + self.context["message"] = error + self.context["message_category"] = "error" self.context["email"] = email else: flash(t("Login was sucessfull!"), category="success") @@ -51,5 +62,4 @@ class Login(AllowAnyAccess, BaseTemplateGetView): class Logout(AllowAnyAccess, BaseView): def post(self): logout() - flash(t("Logout was sucessfull!"), category="success") - return self.redirect(url_for("login")) + return self.redirect(url_for("login", msg="logout")) diff --git a/max/csrf.py b/max/csrf.py index e90c099..2be08e5 100644 --- a/max/csrf.py +++ b/max/csrf.py @@ -3,39 +3,70 @@ from functools import wraps from flask import request, session from max.crypto import constant_time_compare, get_random_string -from max.middlewares import BaseMiddleware +from max.middlewares import BaseMiddleware, get_real_remote_addr CSRF_TOKEN_KEY = "csrf_token" CSRF_TOKEN_LENGTH = 32 +SAFE_METHODS = ("GET", "HEAD", "OPTIONS") +# "none": navigation started by the user (e.g. reload). "same-site" is rejected, +# since other services on a subdomain must not trigger requests. +ALLOWED_FETCH_SITES = ("same-origin", "none") class CsrfMiddleware(BaseMiddleware): def __call__(self, *args, **kwargs): error = self.process_request() if error is not None: - # clear csrf from session - if CSRF_TOKEN_KEY in session: - del session[CSRF_TOKEN_KEY] - return error, 400 + return error return self.handler(*args, **kwargs) def process_request(self): - """If a string is returned, it mist be used as a 400er error""" - if getattr(self.handler, "no_csrf", False): + """If a (message, status) tuple is returned, it must be used as the response""" + no_csrf = getattr(self.handler, "no_csrf", False) + + if request.method in SAFE_METHODS: + if not no_csrf and CSRF_TOKEN_KEY not in session: + session[CSRF_TOKEN_KEY] = get_random_string(CSRF_TOKEN_LENGTH) return - session_has_csrf_token = CSRF_TOKEN_KEY in session - method_is_unsafe = request.method.lower() != "get" + # Stateless check, so it also protects routes without a session (login) + if not self.is_origin_allowed(): + self.app.logger.debug( + 'Blocked cross-origin request %s %s origin="%s" sec_fetch_site="%s" from %s', + request.method, + request.path, + request.headers.get("Origin"), + request.headers.get("Sec-Fetch-Site"), + get_real_remote_addr(self.app), + ) + return "Cross-origin request blocked", 403 - if not session_has_csrf_token and method_is_unsafe: - return "CSRF token is missing" + if no_csrf: + return - elif not session_has_csrf_token and not method_is_unsafe: - session[CSRF_TOKEN_KEY] = get_random_string(CSRF_TOKEN_LENGTH) + if CSRF_TOKEN_KEY not in session: + return "CSRF token is missing", 400 - elif session_has_csrf_token and method_is_unsafe: - return self.check_token(session[CSRF_TOKEN_KEY], request.form) + error = self.check_token(session[CSRF_TOKEN_KEY], request.form) + if error is not None: + return error, 400 + + def is_origin_allowed(self): + fetch_site = request.headers.get("Sec-Fetch-Site") + if fetch_site is not None: + return fetch_site in ALLOWED_FETCH_SITES + + origin = request.headers.get("Origin") + if origin is None: + return True # not a browser, so no CSRF possible + return origin == self.get_expected_origin() + + def get_expected_origin(self): + origin = self.app.config["MAX_ORIGIN"] + if origin is None: + origin = request.host_url.rstrip("/") + return origin def check_token(self, session_token, formdata): if CSRF_TOKEN_KEY not in formdata: diff --git a/max/middlewares.py b/max/middlewares.py index 5f6757f..6bc21ec 100644 --- a/max/middlewares.py +++ b/max/middlewares.py @@ -20,12 +20,16 @@ class BaseMiddleware: raise NotImplementedError() +def get_real_remote_addr(app): + real_ip = request.headers.get("X-Real-IP", None) + if real_ip is None and app.debug: + real_ip = request.remote_addr + return real_ip + + class LoggingMiddleware(BaseMiddleware): def __call__(self, *args, **kwargs): - real_ip = request.headers.get("X-Real-IP", None) - if real_ip is None and self.app.debug: - real_ip = request.remote_addr - setattr(request, "real_remote_addr", real_ip) + setattr(request, "real_remote_addr", get_real_remote_addr(self.app)) start = time.time() diff --git a/max/sessions.py b/max/sessions.py index 9aee311..07f20b2 100644 --- a/max/sessions.py +++ b/max/sessions.py @@ -1,3 +1,4 @@ +from flask import request from flask.sessions import SessionInterface as FlaskSessionInterface, SessionMixin from itsdangerous import BadSignature, Signer, want_bytes from werkzeug.datastructures import CallbackDict @@ -113,7 +114,9 @@ class SessionInterface(FlaskSessionInterface): if session.is_empty(): if session.modified: del session_storage[session.id] - response.delete_cookie(cookie_name, domain=domain, path=path) + # Only send the deletion header if the client sent a cookie + if cookie_name in request.cookies: + response.delete_cookie(cookie_name, domain=domain, path=path) else: httponly = self.get_cookie_httponly(app) secure = self.get_cookie_secure(app) diff --git a/max/templates/auth/login.html b/max/templates/auth/login.html index e0c0d8d..37d4fe7 100644 --- a/max/templates/auth/login.html +++ b/max/templates/auth/login.html @@ -2,11 +2,8 @@ {% block content %}

{{ t("Login") }}

- {% for category, message in get_flashed_messages(with_categories=true) %} -
{{ t(message) }}
- {% endfor %} - {% if alert %} -
{{ t("Please login to visit the requested url") }}
+ {% if message %} +
{{ message }}
{% endif %}