From ca45be80db92631ea7aa35ff474ed6c20d92742a Mon Sep 17 00:00:00 2001 From: GGThed Date: Fri, 7 Aug 2026 20:03:50 -0400 Subject: [PATCH] fix(security): proteger le dernier administrateur, et fermer le CORS permissif MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SEC-AUTHZ-007 - auto-verrouillage de l'administration Le changement de role n'excluait ni l'utilisateur courant, ni le dernier compte admin actif. Une seule manipulation suffisait a transformer le seul president en joueur, et plus aucune interface ne permettait de revenir en arriere : il fallait une intervention directe en base. Deux gardes distinctes, parce que ce sont deux erreurs differentes : - changer son propre role est refuse, meme s'il reste d'autres admins. Un president qui veut se retrograder doit le faire faire par un autre. - retrograder le dernier admin actif est refuse. Le decompte exclut les comptes desactives : trois admins dont deux desactives, cela fait un seul administrateur reel. SEC-WEB-003 - CORS ouvert par defaut Sans CORS_ALLOWED_ORIGINS, la branche else appelait CORS(app, supports_credentials=True) sans argument origins. flask-cors retient alors '*' et, les identifiants etant autorises, renvoie en echo l'Origin de l'appelant avec Access-Control-Allow-Credentials: true -- l'inverse exact de ce qu'annonçait le commentaire. L'exploitation etait bloquee par SESSION_COOKIE_SAMESITE = 'Lax', qui empeche le navigateur de joindre le cookie de session a une requete fetch inter-site. Toute la protection tenait donc a ce seul reglage. Cette application rend du HTML en meme origine : elle n'a besoin d'aucune politique CORS. La branche par defaut est supprimee, la configuration explicite reste possible. 5 tests ajoutes, dont deux verifient que les chemins legitimes continuent de fonctionner : un autre administrateur reste retrogradable, et une origine explicitement configuree est toujours honoree. Co-Authored-By: Claude Opus 5 --- app/app.py | 22 +++++++++------ app/routes/users.py | 19 +++++++++++++ tests/test_authorization.py | 55 +++++++++++++++++++++++++++++++++---- 3 files changed, 82 insertions(+), 14 deletions(-) diff --git a/app/app.py b/app/app.py index 8a603f2..1252f0a 100644 --- a/app/app.py +++ b/app/app.py @@ -103,6 +103,19 @@ def create_app(config=None): allowed_origins = str(app.config.get('CORS_ALLOWED_ORIGINS') or '').split(',') allowed_origins = [origin.strip() for origin in allowed_origins if origin.strip()] + # CORS is only configured when origins are named explicitly. + # + # The previous else-branch called CORS(app, supports_credentials=True) + # with no origins argument. flask-cors then defaults to '*' and, because + # credentials are allowed, echoes back whatever Origin the caller sent + # together with Access-Control-Allow-Credentials: true — the opposite of + # the "allow all (development) or none (production)" the comment claimed. + # + # Exploitation was blocked by SESSION_COOKIE_SAMESITE = 'Lax', which stops + # the browser attaching the session cookie to a cross-site fetch. That is + # a single setting standing between a misconfiguration and a cross-origin + # data leak. This application renders server-side HTML on one origin and + # needs no CORS policy at all. if allowed_origins: CORS( app, @@ -111,15 +124,6 @@ def create_app(config=None): methods=['GET', 'POST', 'PUT', 'PATCH', 'DELETE', 'OPTIONS'], max_age=3600, # Cache preflight for 1 hour ) - else: - # When no origins specified, allow all (development) or none (production) - # In production with a reverse proxy, CORS is handled at the Nginx level - CORS( - app, - supports_credentials=True, - methods=['GET', 'POST', 'PUT', 'PATCH', 'DELETE', 'OPTIONS'], - max_age=3600, - ) db.init_app(app) login_manager.init_app(app) diff --git a/app/routes/users.py b/app/routes/users.py index 147d8b3..3e86767 100644 --- a/app/routes/users.py +++ b/app/routes/users.py @@ -166,6 +166,25 @@ def edit_user(user_id): flash('Email already in use by another account.', 'danger') return _rerender() + if user.role != role: + # Two ways to lock everyone out of administration, neither of + # which any interface can undo afterwards. + if user.id == actor_id: + flash('You cannot change your own role. Ask another president ' + 'to do it.', 'danger') + return _rerender() + + if user.role == 'admin': + remaining_admins = User.query.filter( + User.role == 'admin', + User.is_active_account.is_(True), + User.id != user.id, + ).count() + if remaining_admins == 0: + flash('This is the last active president. Promote another ' + 'account before changing this one.', 'danger') + return _rerender() + # Change role via raw SQL to avoid polymorphic identity corruption. # Must discard the entire session because the polymorphic discriminator # change invalidates the identity map for this instance and anything diff --git a/tests/test_authorization.py b/tests/test_authorization.py index bf6d860..1df8eb9 100644 --- a/tests/test_authorization.py +++ b/tests/test_authorization.py @@ -302,11 +302,6 @@ class TestInputValidation: class TestAdminSafety: - @pytest.mark.xfail( - strict=True, - reason='SEC-AUTHZ-007: the role change excludes neither the current ' - 'user nor the last remaining admin', - ) def test_the_last_admin_cannot_demote_itself(self, app, client, as_role): admin_id = as_role('admin') @@ -322,6 +317,34 @@ class TestAdminSafety: ) + def test_an_admin_cannot_change_its_own_role_even_with_others_present( + self, app, client, as_role, make_user + ): + """Self-demotion is refused on its own, not only when last.""" + make_user('admin') + admin_id = as_role('admin') + + client.post(f'/users/{admin_id}/edit', data={ + 'full_name': 'Admin', 'email': 'self@example.test', 'role': 'player', + }, follow_redirects=True) + + with app.app_context(): + assert db.session.get(User, admin_id).role == 'admin' + + def test_another_admin_can_still_be_demoted(self, app, client, as_role, make_user): + """Guard against over-correcting: the legitimate path must work.""" + other_id = make_user('admin') + as_role('admin') + + client.post(f'/users/{other_id}/edit', data={ + 'full_name': 'Other', 'email': 'other-demoted@example.test', + 'role': 'coach', 'is_active_account': 'on', + }, follow_redirects=True) + + with app.app_context(): + assert db.session.get(User, other_id).role == 'coach' + + class TestCsrf: def test_state_changing_post_without_a_token_is_rejected(self, app_with_csrf): """CSRFProtect is global. This pins that down so a future @@ -331,3 +354,25 @@ class TestCsrf: 'username': 'someone', 'password': 'Password123', }) assert response.status_code == 400 + +class TestCorsPolicy: + """SEC-WEB-003 — with no origins configured, flask-cors defaulted to '*' + and, credentials being allowed, echoed back the caller's Origin.""" + + def test_no_cors_headers_without_explicit_configuration(self, client): + response = client.get('/auth/login', headers={'Origin': 'https://evil.test'}) + assert 'Access-Control-Allow-Origin' not in response.headers + assert 'Access-Control-Allow-Credentials' not in response.headers + + def test_configured_origins_are_still_honoured(self, app_with_csrf): + from app.app import create_app + + application = create_app({ + 'SECRET_KEY': 'test', 'SQLALCHEMY_DATABASE_URI': 'sqlite:///:memory:', + 'TESTING': True, 'FORCE_HTTPS': False, 'ENABLE_DISCORD_BOT': False, + 'AUTO_CREATE_TABLES': False, 'CORS_ALLOWED_ORIGINS': 'https://trusted.test', + }) + response = application.test_client().get( + '/auth/login', headers={'Origin': 'https://trusted.test'}) + + assert response.headers.get('Access-Control-Allow-Origin') == 'https://trusted.test'