fix(auth): SECRET_KEY di default rifiutato, session.permanent, logout solo POST (#33)
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 <noreply@anthropic.com>
This commit is contained in:
@@ -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."""
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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."
|
||||
)
|
||||
|
||||
@@ -192,10 +192,13 @@
|
||||
{{ _('Termina la sessione corrente e torna alla schermata di login') }}
|
||||
</p>
|
||||
<div class="mt-3">
|
||||
<a href="{{ url_for('auth.logout') }}"
|
||||
class="inline-flex items-center px-4 py-2 border border-red-300 dark:border-red-700 rounded-lg shadow-sm text-sm font-medium text-red-700 dark:text-red-300 bg-white dark:bg-slate-800 hover:bg-red-50 dark:hover:bg-red-900/20 focus:outline-none focus:ring-2 focus:ring-offset-2 focus:ring-red-500 transition-all duration-200">
|
||||
{{ _('Logout') }}
|
||||
</a>
|
||||
<form method="post" action="{{ url_for('auth.logout') }}" class="inline">
|
||||
<input type="hidden" name="csrf_token" value="{{ csrf_token() }}">
|
||||
<button type="submit"
|
||||
class="inline-flex items-center px-4 py-2 border border-red-300 dark:border-red-700 rounded-lg shadow-sm text-sm font-medium text-red-700 dark:text-red-300 bg-white dark:bg-slate-800 hover:bg-red-50 dark:hover:bg-red-900/20 focus:outline-none focus:ring-2 focus:ring-offset-2 focus:ring-red-500 transition-all duration-200">
|
||||
{{ _('Logout') }}
|
||||
</button>
|
||||
</form>
|
||||
</div>
|
||||
</div>
|
||||
</div>
|
||||
|
||||
@@ -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);
|
||||
|
||||
@@ -160,14 +160,17 @@
|
||||
{{ _('Logout') }}
|
||||
</span>
|
||||
{% else %}
|
||||
<a href="{{ url_for('auth.logout') }}"
|
||||
class="flex items-center gap-2.5 px-4 py-2.5 text-sm text-measure-fail
|
||||
hover:bg-red-50 dark:hover:bg-red-900/20 transition-colors">
|
||||
<svg class="w-4 h-4" fill="none" stroke="currentColor" stroke-width="1.75" viewBox="0 0 24 24">
|
||||
<path stroke-linecap="round" stroke-linejoin="round" d="M17 16l4-4m0 0l-4-4m4 4H7m6 4v1a3 3 0 01-3 3H6a3 3 0 01-3-3V7a3 3 0 013-3h4a3 3 0 013 3v1"/>
|
||||
</svg>
|
||||
{{ _('Logout') }}
|
||||
</a>
|
||||
<form method="post" action="{{ url_for('auth.logout') }}">
|
||||
<input type="hidden" name="csrf_token" value="{{ csrf_token() }}">
|
||||
<button type="submit"
|
||||
class="w-full flex items-center gap-2.5 px-4 py-2.5 text-sm text-measure-fail
|
||||
hover:bg-red-50 dark:hover:bg-red-900/20 transition-colors">
|
||||
<svg class="w-4 h-4" fill="none" stroke="currentColor" stroke-width="1.75" viewBox="0 0 24 24">
|
||||
<path stroke-linecap="round" stroke-linejoin="round" d="M17 16l4-4m0 0l-4-4m4 4H7m6 4v1a3 3 0 01-3 3H6a3 3 0 01-3-3V7a3 3 0 013-3h4a3 3 0 013 3v1"/>
|
||||
</svg>
|
||||
{{ _('Logout') }}
|
||||
</button>
|
||||
</form>
|
||||
{% endif %}
|
||||
</div>
|
||||
</div>
|
||||
|
||||
@@ -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 <img>) 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."""
|
||||
|
||||
@@ -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
|
||||
Reference in New Issue
Block a user