fix(security): proteger le dernier administrateur, et fermer le CORS permissif
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 <[email protected]>
This commit is contained in:
+13
-9
@@ -103,6 +103,19 @@ def create_app(config=None):
|
|||||||
allowed_origins = str(app.config.get('CORS_ALLOWED_ORIGINS') or '').split(',')
|
allowed_origins = str(app.config.get('CORS_ALLOWED_ORIGINS') or '').split(',')
|
||||||
allowed_origins = [origin.strip() for origin in allowed_origins if origin.strip()]
|
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:
|
if allowed_origins:
|
||||||
CORS(
|
CORS(
|
||||||
app,
|
app,
|
||||||
@@ -111,15 +124,6 @@ def create_app(config=None):
|
|||||||
methods=['GET', 'POST', 'PUT', 'PATCH', 'DELETE', 'OPTIONS'],
|
methods=['GET', 'POST', 'PUT', 'PATCH', 'DELETE', 'OPTIONS'],
|
||||||
max_age=3600, # Cache preflight for 1 hour
|
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)
|
db.init_app(app)
|
||||||
login_manager.init_app(app)
|
login_manager.init_app(app)
|
||||||
|
|||||||
@@ -166,6 +166,25 @@ def edit_user(user_id):
|
|||||||
flash('Email already in use by another account.', 'danger')
|
flash('Email already in use by another account.', 'danger')
|
||||||
return _rerender()
|
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.
|
# Change role via raw SQL to avoid polymorphic identity corruption.
|
||||||
# Must discard the entire session because the polymorphic discriminator
|
# Must discard the entire session because the polymorphic discriminator
|
||||||
# change invalidates the identity map for this instance and anything
|
# change invalidates the identity map for this instance and anything
|
||||||
|
|||||||
@@ -302,11 +302,6 @@ class TestInputValidation:
|
|||||||
|
|
||||||
|
|
||||||
class TestAdminSafety:
|
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):
|
def test_the_last_admin_cannot_demote_itself(self, app, client, as_role):
|
||||||
admin_id = as_role('admin')
|
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': '[email protected]', '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': '[email protected]',
|
||||||
|
'role': 'coach', 'is_active_account': 'on',
|
||||||
|
}, follow_redirects=True)
|
||||||
|
|
||||||
|
with app.app_context():
|
||||||
|
assert db.session.get(User, other_id).role == 'coach'
|
||||||
|
|
||||||
|
|
||||||
class TestCsrf:
|
class TestCsrf:
|
||||||
def test_state_changing_post_without_a_token_is_rejected(self, app_with_csrf):
|
def test_state_changing_post_without_a_token_is_rejected(self, app_with_csrf):
|
||||||
"""CSRFProtect is global. This pins that down so a future
|
"""CSRFProtect is global. This pins that down so a future
|
||||||
@@ -331,3 +354,25 @@ class TestCsrf:
|
|||||||
'username': 'someone', 'password': 'Password123',
|
'username': 'someone', 'password': 'Password123',
|
||||||
})
|
})
|
||||||
assert response.status_code == 400
|
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'
|
||||||
|
|||||||
Reference in New Issue
Block a user