Quatre constats de la liste des gains rapides, tous sur auth.py. SEC-017 -- enumeration de comptes Le formulaire repondait "il vous reste 3 tentative(s)" a un compte connu et "verifiez le nom d utilisateur et le mot de passe" a un inconnu. Le decompte lui-meme etait la fuite : la meme information, etalee sur cinq requetes. S y ajoutait un ecart de temps de reponse, check_password n etant appele que si la ligne existait -- scrypt est cher, l ecart est mesurable. Un seul message pour tous les echecs, et la verification s execute desormais sur les deux branches : contre un hachage aleatoire tire une fois par processus quand l identifiant n existe pas. SEC-018 -- verrou de compte declenchable par un tiers Cinq mauvaises reponses mettaient un compte connu hors service pendant quinze minutes, indefiniment renouvelables. Sur un compte president, c est toute l administration, et aucun ecran ne permettait de defaire. Le compteur et la fenetre restent -- ce sont la trace qu un administrateur lit quand un compte est pilonne, et la fenetre double jusqu a un plafond. Ce qui change : de bons identifiants passent, fenetre ouverte ou non, et remettent le compteur a zero. Le proprietaire du compte ne peut plus etre bloque par un tiers. Ce que cela coute, dit franchement : un verrou dur n arretait de toute facon pas un attaquant ayant trouve le mot de passe -- il lui suffisait d attendre. Le debit de tentatives reste borne par la limite de 10/minute par IP. Une limite par couple (compte, IP) demanderait un stockage dedie ; elle attend Alembic. L evenement account.locked devient account.throttled : "locked" affirmait plus que ce qui se passe. SEC-019 -- deconnexion en GET /auth/logout n avait pas de methods, donc GET, donc hors protection CSRF : n importe quelle page pouvait deconnecter un visiteur avec une balise img. La route passe en POST et l entree de navigation devient un formulaire avec jeton. Le style suit -- les regles .nav-links visaient les liens seuls. SEC-020 -- validation de redirection is_safe_url interrogeait urlparse().netloc. urlparse lit /\evil.com comme un chemin, sans netloc ; plusieurs navigateurs normalisent l antislash en barre oblique avant de resoudre, ce qui en fait //evil.com. La fonction refuse maintenant antislash et caracteres de controle, exige un chemin enracine, et compare l origine explicitement. Le xfail(strict) qui documentait SEC-017 est leve. 40 tests dans test_auth_session.py, dont la table des cibles refusees. Co-Authored-By: Claude Opus 5 <[email protected]>
120 lines
3.9 KiB
Python
120 lines
3.9 KiB
Python
"""Changing a user's role — ARCH-008.
|
|
|
|
`role` is the polymorphic discriminator, and SQLAlchemy fixes an instance's
|
|
class at load time. edit_user therefore writes the column with a
|
|
statement-level UPDATE and re-reads the row, which is correct. What it did
|
|
next was not: it called db.session.remove(), throwing away the whole
|
|
session, and it committed the role on its own before the rest of the edit
|
|
had been applied.
|
|
|
|
The two consequences these tests pin down:
|
|
|
|
- everything the request still held became detached, current_user
|
|
included. The route only survived because it had copied the actor's
|
|
name and id into locals beforehand — a workaround, in place of the fix.
|
|
- the role change was already committed when the rest of the edit ran, so
|
|
a failure afterwards left the account promoted and nothing else
|
|
applied.
|
|
"""
|
|
|
|
import pytest
|
|
|
|
from app.extensions import db
|
|
from app.models import Coach, Player, User
|
|
|
|
|
|
def _edit(client, user_id, **overrides):
|
|
payload = {
|
|
'full_name': 'Promoted Person',
|
|
'email': '[email protected]',
|
|
'role': 'coach',
|
|
'is_active_account': 'on',
|
|
}
|
|
payload.update(overrides)
|
|
return client.post(f'/users/{user_id}/edit', data=payload, follow_redirects=True)
|
|
|
|
|
|
class TestPolymorphicIdentity:
|
|
def test_the_account_takes_its_new_class(self, app, client, as_role, make_user):
|
|
target_id = make_user('player')
|
|
as_role('admin')
|
|
|
|
_edit(client, target_id)
|
|
|
|
with app.app_context():
|
|
assert isinstance(db.session.get(User, target_id), Coach)
|
|
|
|
def test_the_new_role_grants_its_pages_right_away(
|
|
self, app, client, as_role, make_user, login
|
|
):
|
|
"""isinstance() is how this application authorises; a stale class in
|
|
the identity map is a stale permission set."""
|
|
target_id = make_user('player')
|
|
as_role('admin')
|
|
_edit(client, target_id)
|
|
|
|
with app.app_context():
|
|
username = db.session.get(User, target_id).username
|
|
client.post('/auth/logout')
|
|
login(username)
|
|
|
|
# Coach-only, and it is a coach's own page rather than a redirect.
|
|
assert client.get('/users/notes-dashboard').status_code == 200
|
|
|
|
def test_the_other_fields_of_the_edit_land_too(
|
|
self, app, client, as_role, make_user
|
|
):
|
|
target_id = make_user('player')
|
|
as_role('admin')
|
|
|
|
_edit(client, target_id, full_name='Renamed Too', phone='555-0100')
|
|
|
|
with app.app_context():
|
|
user = db.session.get(User, target_id)
|
|
assert user.full_name == 'Renamed Too'
|
|
assert user.phone == '555-0100'
|
|
|
|
|
|
class TestTheActingPresident:
|
|
"""db.session.remove() detached current_user mid-request."""
|
|
|
|
def test_the_president_keeps_their_session(self, client, as_role, make_user):
|
|
target_id = make_user('player')
|
|
as_role('admin')
|
|
|
|
_edit(client, target_id)
|
|
|
|
assert client.get('/users').status_code == 200
|
|
|
|
def test_the_response_renders(self, client, as_role, make_user):
|
|
target_id = make_user('player')
|
|
as_role('admin')
|
|
|
|
response = _edit(client, target_id)
|
|
|
|
assert response.status_code == 200
|
|
|
|
|
|
class TestAtomicity:
|
|
def test_a_failure_after_the_update_leaves_the_role_alone(
|
|
self, app, client, as_role, make_user, monkeypatch
|
|
):
|
|
"""The role used to be committed on its own, before the rest of the
|
|
edit ran. A failure afterwards left an account promoted, and no
|
|
interface reported it."""
|
|
target_id = make_user('player')
|
|
as_role('admin')
|
|
|
|
def _explode(user, selected_games):
|
|
raise RuntimeError('storage unavailable')
|
|
|
|
monkeypatch.setattr('app.routes.users.update_user_gamertags', _explode)
|
|
|
|
with pytest.raises(RuntimeError):
|
|
_edit(client, target_id)
|
|
|
|
with app.app_context():
|
|
user = db.session.get(User, target_id)
|
|
assert isinstance(user, Player)
|
|
assert user.role == 'player'
|