From 92f130479af8c30e2de1e833d2623ce58bd96d75 Mon Sep 17 00:00:00 2001 From: AdrianoDev Date: Mon, 21 Sep 2026 22:37:23 +0200 Subject: [PATCH] fix(auth): SECRET_KEY di default rifiutato, session.permanent, logout solo POST (#33) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit config.py: guard_default_secret_key, verificata a ogni richiesta (prima di create_app, TESTING/DEBUG non sono ancora impostati dai fixture di test) — rifiuta con RuntimeError se CLIENT_SECRET_KEY è rimasto al default committato fuori da debug/testing. auth.py: session.permanent=True al login (senza, PERMANENT_SESSION_LIFETIME non aveva mai effetto); /auth/logout solo POST, con CSRF. Aggiornati i tre punti che lo chiamavano: profile.html e navbar.html (link -> form POST), auto-logout JS in base.html (fetch POST + redirect al login). Co-Authored-By: Claude Sonnet 5 --- src/frontend/flask_app/app.py | 7 +- src/frontend/flask_app/blueprints/auth.py | 7 +- src/frontend/flask_app/config.py | 26 ++++++- .../flask_app/templates/auth/profile.html | 11 ++- src/frontend/flask_app/templates/base.html | 12 ++- .../templates/components/navbar.html | 19 +++-- src/frontend/flask_app/tests/test_auth.py | 12 ++- .../flask_app/tests/test_secret_key_guard.py | 76 +++++++++++++++++++ 8 files changed, 151 insertions(+), 19 deletions(-) create mode 100644 src/frontend/flask_app/tests/test_secret_key_guard.py diff --git a/src/frontend/flask_app/app.py b/src/frontend/flask_app/app.py index 6f92cab..7dfe09d 100644 --- a/src/frontend/flask_app/app.py +++ b/src/frontend/flask_app/app.py @@ -11,7 +11,7 @@ from flask_wtf.csrf import CSRFError, CSRFProtect from markupsafe import Markup, escape from werkzeug.middleware.proxy_fix import ProxyFix -from config import Config +from config import Config, guard_default_secret_key # **bold**, the whole of the markup a task description understands besides the # line break. DOTALL so a phrase that wraps onto the next line still closes. @@ -80,6 +80,11 @@ def create_app() -> Flask: app.register_blueprint(statistics_bp, url_prefix="/statistics") app.register_blueprint(admin_bp, url_prefix="/admin") + @app.before_request + def _check_secret_key(): + """See guard_default_secret_key in config.py (#33).""" + guard_default_secret_key(app) + @app.route("/") def index(): """Root redirect to login or dashboard based on session.""" diff --git a/src/frontend/flask_app/blueprints/auth.py b/src/frontend/flask_app/blueprints/auth.py index e0abfd6..bf43e1b 100644 --- a/src/frontend/flask_app/blueprints/auth.py +++ b/src/frontend/flask_app/blueprints/auth.py @@ -81,7 +81,10 @@ def login(): flash(response.get("detail", _("Credenziali non valide")), "error") return render_template("auth/login.html") - # Salva in sessione + # Salva in sessione. permanent=True fa valere PERMANENT_SESSION_LIFETIME + # (8h, config.py): senza, Flask usa una sessione di durata browser e il + # limite configurato non ha mai effetto (#33). + session.permanent = True session["api_key"] = response["api_key"] session["user"] = response["user"] session["user_id"] = response["user"]["id"] @@ -118,7 +121,7 @@ def login(): return render_template("auth/login.html") -@auth_bp.route("/logout", methods=["GET", "POST"]) +@auth_bp.route("/logout", methods=["POST"]) def logout(): """Clear session and redirect to login.""" # Invalida il token sul server diff --git a/src/frontend/flask_app/config.py b/src/frontend/flask_app/config.py index 3cb396c..7a805c3 100644 --- a/src/frontend/flask_app/config.py +++ b/src/frontend/flask_app/config.py @@ -4,12 +4,17 @@ from dotenv import load_dotenv load_dotenv(os.path.join(os.path.dirname(__file__), '..', '.env')) +# Committed default for CLIENT_SECRET_KEY (also in .env.example). A Flask +# session is only as secret as this value: left at the default, anyone can +# forge a session cookie (roles, is_admin) - see guard_default_secret_key (#33). +DEFAULT_SECRET_KEY = "change-this-to-another-random-secret-key" + class Config: """Flask client configuration.""" # Flask - SECRET_KEY = os.getenv("CLIENT_SECRET_KEY", "change-this-to-another-random-secret-key") + SECRET_KEY = os.getenv("CLIENT_SECRET_KEY", DEFAULT_SECRET_KEY) DEBUG = os.getenv("FLASK_DEBUG", "0") == "1" # API Server connection @@ -41,3 +46,22 @@ class Config: BABEL_DEFAULT_LOCALE = "it" BABEL_DEFAULT_TIMEZONE = "Europe/Rome" LANGUAGES = {"it": "Italiano", "en": "English"} + + +def guard_default_secret_key(app) -> None: + """Refuse to serve when CLIENT_SECRET_KEY was left at its committed default (#33). + + Checked per-request (wired as a ``before_request`` in app.py) rather than + at import/create_app time: pytest and every fixture in this suite call + ``create_app()`` and only *afterwards* set ``app.config["TESTING"]``, so a + check made during app creation would always see TESTING as False and + break the whole test suite and local `flask run` alike. + """ + if app.debug or app.testing: + return + if app.config.get("SECRET_KEY") == DEFAULT_SECRET_KEY: + raise RuntimeError( + "CLIENT_SECRET_KEY non impostato: la sessione Flask (ruoli, is_admin) " + "sarebbe falsificabile da chiunque conosca il default committato. " + "Imposta CLIENT_SECRET_KEY in .env prima di avviare in produzione." + ) diff --git a/src/frontend/flask_app/templates/auth/profile.html b/src/frontend/flask_app/templates/auth/profile.html index 645a2c3..7d145df 100644 --- a/src/frontend/flask_app/templates/auth/profile.html +++ b/src/frontend/flask_app/templates/auth/profile.html @@ -192,10 +192,13 @@ {{ _('Termina la sessione corrente e torna alla schermata di login') }}

- - {{ _('Logout') }} - +
+ + +
diff --git a/src/frontend/flask_app/templates/base.html b/src/frontend/flask_app/templates/base.html index 22f14d1..5177d96 100644 --- a/src/frontend/flask_app/templates/base.html +++ b/src/frontend/flask_app/templates/base.html @@ -151,7 +151,17 @@ var remaining = timeoutMs - elapsed; if (remaining <= 0) { clearInterval(this._interval); - window.location.href = '{{ url_for("auth.logout") }}'; + // #33: /auth/logout is POST-only now (CSRF-protected), so a plain + // navigation would 405. Fire the logout, then leave regardless of + // the outcome - staying logged in client-side is worse than a + // logout call that failed silently. + var csrfMeta = document.querySelector('meta[name=csrf-token]'); + fetch('{{ url_for("auth.logout") }}', { + method: 'POST', + headers: csrfMeta ? { 'X-CSRFToken': csrfMeta.content } : {} + }).finally(function() { + window.location.href = '{{ url_for("auth.login") }}'; + }); } else if (remaining <= warningMs) { this.showWarning = true; this.remainingSeconds = Math.ceil(remaining / 1000); diff --git a/src/frontend/flask_app/templates/components/navbar.html b/src/frontend/flask_app/templates/components/navbar.html index f44b7ec..0be59ad 100644 --- a/src/frontend/flask_app/templates/components/navbar.html +++ b/src/frontend/flask_app/templates/components/navbar.html @@ -160,14 +160,17 @@ {{ _('Logout') }} {% else %} - - - - - {{ _('Logout') }} - +
+ + +
{% endif %} diff --git a/src/frontend/flask_app/tests/test_auth.py b/src/frontend/flask_app/tests/test_auth.py index 0f7572c..d0ae802 100644 --- a/src/frontend/flask_app/tests/test_auth.py +++ b/src/frontend/flask_app/tests/test_auth.py @@ -54,6 +54,9 @@ class TestLoginPost: with client.session_transaction() as sess: assert sess.get("api_key") == "new-api-key-abc" assert sess["user"]["username"] == "testuser" + # #33: without this, PERMANENT_SESSION_LIFETIME (8h) never applied + # and Flask used a browser-length session instead. + assert sess.permanent is True def test_login_post_error_shows_error(self, client, mock_api_client): """Failed login returns the login page with error flash.""" @@ -83,13 +86,13 @@ class TestLoginPost: class TestLogout: - """GET/POST /auth/logout tests.""" + """POST /auth/logout tests (#33: GET is refused - logout must carry CSRF).""" def test_logout_clears_session(self, logged_in_client, mock_api_client): """Logout clears session and redirects to login.""" mock_api_client.post.return_value = {} - resp = logged_in_client.get("/auth/logout", follow_redirects=False) + resp = logged_in_client.post("/auth/logout", follow_redirects=False) assert resp.status_code == 302 assert "/auth/login" in resp.headers["Location"] @@ -98,6 +101,11 @@ class TestLogout: assert "api_key" not in sess assert "user" not in sess + def test_logout_get_is_rejected(self, logged_in_client): + """#33: a bare GET (link, prefetch, forged ) no longer logs anyone out.""" + resp = logged_in_client.get("/auth/logout", follow_redirects=False) + assert resp.status_code == 405 + class TestUnauthenticatedRedirect: """Protected routes redirect unauthenticated users.""" diff --git a/src/frontend/flask_app/tests/test_secret_key_guard.py b/src/frontend/flask_app/tests/test_secret_key_guard.py new file mode 100644 index 0000000..fd750d1 --- /dev/null +++ b/src/frontend/flask_app/tests/test_secret_key_guard.py @@ -0,0 +1,76 @@ +"""Tests for the CLIENT_SECRET_KEY default guard (#33). + +Uses the raw function (config.guard_default_secret_key) for the unit-level +cases, and the app's wired before_request hook for one end-to-end check. +""" +import pytest +from flask import Flask + +from config import DEFAULT_SECRET_KEY, guard_default_secret_key + + +def _app(secret_key, debug=False, testing=False): + app = Flask(__name__) + app.config["SECRET_KEY"] = secret_key + app.config["DEBUG"] = debug + app.config["TESTING"] = testing + return app + + +class TestGuardFunction: + def test_default_key_in_production_raises(self): + app = _app(DEFAULT_SECRET_KEY, debug=False, testing=False) + with pytest.raises(RuntimeError, match="CLIENT_SECRET_KEY"): + guard_default_secret_key(app) + + def test_default_key_with_debug_is_allowed(self): + app = _app(DEFAULT_SECRET_KEY, debug=True, testing=False) + guard_default_secret_key(app) # must not raise + + def test_default_key_with_testing_is_allowed(self): + app = _app(DEFAULT_SECRET_KEY, debug=False, testing=True) + guard_default_secret_key(app) # must not raise + + def test_custom_key_in_production_is_allowed(self): + app = _app("a-real-random-secret", debug=False, testing=False) + guard_default_secret_key(app) # must not raise + + +class TestGuardWiredInApp: + """The before_request hook registered in create_app() behaves the same way.""" + + def test_default_key_not_debug_not_testing_raises_on_request(self, monkeypatch): + monkeypatch.delenv("FLASK_DEBUG", raising=False) + monkeypatch.delenv("CLIENT_SECRET_KEY", raising=False) + monkeypatch.setenv("STATION_CODE", "ST-TEST") + import config + import importlib + importlib.reload(config) + + from app import create_app + app = create_app() + # Neither debug nor testing set - the production-like case (#33). + assert app.config["SECRET_KEY"] == config.DEFAULT_SECRET_KEY + assert app.debug is False + assert app.testing is False + + with app.test_request_context("/auth/login"): + with pytest.raises(RuntimeError, match="CLIENT_SECRET_KEY"): + app.preprocess_request() + + def test_testing_flag_lets_requests_through(self, monkeypatch): + """This is exactly the ordering the whole suite relies on: fixtures + call create_app() then set TESTING=True afterwards, and the guard + must still see it (checked per-request, not at create_app time).""" + monkeypatch.setenv("STATION_CODE", "ST-TEST") + import config + import importlib + importlib.reload(config) + + from app import create_app + app = create_app() + app.config["TESTING"] = True + + with app.test_client() as c: + resp = c.get("/auth/login") + assert resp.status_code == 200