From 921da48eb1a4f7c890095987e848ff54db879010 Mon Sep 17 00:00:00 2001 From: Johan LEROY Date: Thu, 17 Sep 2026 14:35:02 +0200 Subject: [PATCH] fix(auth): corrige 4 failles de la revue de securite sur la PR #90 Anti-enumeration cassee sur /auth/forgot-password : l'envoi SMTP etait synchrone dans le chemin de reponse, donc un email existant prenait plus de temps qu'un email inconnu (et pouvait renvoyer 500 si le relais SMTP echouait, contre 202 sinon). L'envoi part desormais en BackgroundTasks, apres que la reponse 202 a ete envoyee au client, avec un try/except qui logue plutot que de laisser une exception SMTP remonter. confirm_password_reset() ne revalidait pas is_active/kind du compte avant de changer le mot de passe : un compte desactive dans les 15 minutes suivant l'emission du lien pouvait quand meme voir son mot de passe change et son must_change_password efface. Les plages [A-ZA-Y]/[a-za-y] de la regle de complexite incluaient par erreur x et / (U+00D7, U+00F7), donc un mot de passe sans aucune majuscule ou minuscule pouvait passer la validation. Le validateur frontend (JS, \w ASCII) et le validateur backend (Python, \w Unicode) divergeaient sur les caracteres accentues : un mot de passe comme "Securite1" passait cote front puis se faisait rejeter en 422 cote back. Les deux cotes utilisent maintenant le meme jeu explicite de caracteres speciaux (SPECIAL_CHARACTERS, partage aussi avec cli.py). --- apps/backend/app/api/v1/endpoints/auth.py | 4 +- apps/backend/app/cli.py | 5 +- apps/backend/app/schemas/auth.py | 14 +++-- apps/backend/app/services/auth.py | 31 +++++++++- apps/backend/tests/schemas/test_auth.py | 20 +++++++ apps/backend/tests/services/test_auth.py | 60 +++++++++++++++++-- .../validators/password.validator.spec.ts | 26 ++++++++ .../shared/validators/password.validator.ts | 12 +++- 8 files changed, 156 insertions(+), 16 deletions(-) create mode 100644 apps/frontend/src/app/shared/validators/password.validator.spec.ts diff --git a/apps/backend/app/api/v1/endpoints/auth.py b/apps/backend/app/api/v1/endpoints/auth.py index 957775f..975273d 100644 --- a/apps/backend/app/api/v1/endpoints/auth.py +++ b/apps/backend/app/api/v1/endpoints/auth.py @@ -2,7 +2,7 @@ # d'accès ne va jamais dans un cookie. C'est ce qui réduit la surface CSRF aux trois routes de # ce module : partout ailleurs, le navigateur n'attache rien de lui-même. -from fastapi import APIRouter, Depends, HTTPException, Request, Response, status +from fastapi import APIRouter, BackgroundTasks, Depends, HTTPException, Request, Response, status from app.api.deps import ( AuthServiceDep, @@ -299,6 +299,7 @@ async def forgot_password( request: Request, response: Response, service: AuthServiceDep, + background_tasks: BackgroundTasks, client_ip: str | None = Depends(get_client_ip), ) -> None: response.headers["Cache-Control"] = "no-store" @@ -308,6 +309,7 @@ async def forgot_password( email=payload.email, client_ip=client_ip, user_agent=request.headers.get("user-agent"), + background_tasks=background_tasks, ) except RateLimitedError as erreur: logger.warning("auth.password_reset.rate_limited ip=%s", client_ip) diff --git a/apps/backend/app/cli.py b/apps/backend/app/cli.py index 06c5610..f713fa5 100644 --- a/apps/backend/app/cli.py +++ b/apps/backend/app/cli.py @@ -23,10 +23,9 @@ from app.core.roles import Role from app.db.session import get_session_factory from app.main import create_app from app.repositories.user import UserRepository -from app.schemas.auth import PASSWORD_MIN_LENGTH, valide_complexite +from app.schemas.auth import PASSWORD_MIN_LENGTH, SPECIAL_CHARACTERS, valide_complexite LONGUEUR_MOT_DE_PASSE_GENERE = 24 -CARACTERES_SPECIAUX = "!@#$%^&*()-_=+[]{};:,.?" CHEMIN_CONTRAT = Path(__file__).resolve().parent.parent / "openapi.json" @@ -119,7 +118,7 @@ def genere_mot_de_passe() -> str: string.ascii_uppercase, string.ascii_lowercase, string.digits, - CARACTERES_SPECIAUX, + SPECIAL_CHARACTERS, ] reste = LONGUEUR_MOT_DE_PASSE_GENERE - len(classes) caracteres = [tirage.choice(classe) for classe in classes] diff --git a/apps/backend/app/schemas/auth.py b/apps/backend/app/schemas/auth.py index e6785be..6ad50ef 100644 --- a/apps/backend/app/schemas/auth.py +++ b/apps/backend/app/schemas/auth.py @@ -1,5 +1,9 @@ # Contrainte : le mot de passe est borné à 128 caractères. Sans plafond, une chaîne de dix # mégaoctets ferait travailler Argon2 gratuitement, à la charge du serveur. +# Contrainte : `SPECIAL_CHARACTERS` doit rester identique à `password.validator.ts` côté +# frontend. `\w`/`\d` divergent entre Python (Unicode) et JavaScript (ASCII) : une classe +# explicite, plutôt qu'une négation, évite qu'un mot de passe soit accepté d'un côté et +# rejeté de l'autre (ex. "Sécurité1", où "é" comptait comme "spécial" pour Python seul). import re from typing import Literal, Self @@ -13,10 +17,12 @@ from app.core.roles import AccountKind, Role PASSWORD_MIN_LENGTH = 8 PASSWORD_MAX_LENGTH = 128 -_MAJUSCULE = re.compile(r"[A-ZÀ-Ý]") -_MINUSCULE = re.compile(r"[a-zà-ÿ]") -_CHIFFRE = re.compile(r"\d") -_SPECIAL = re.compile(r"[^\w\s]") +SPECIAL_CHARACTERS = "!@#$%^&*()-_=+[]{};:,.?" + +_MAJUSCULE = re.compile(r"[A-ZÀ-ÖØ-Þ]") +_MINUSCULE = re.compile(r"[a-zà-öø-þ]") +_CHIFFRE = re.compile(r"[0-9]") +_SPECIAL = re.compile(r"[" + re.escape(SPECIAL_CHARACTERS) + r"]") def valide_complexite(mot_de_passe: str) -> str: diff --git a/apps/backend/app/services/auth.py b/apps/backend/app/services/auth.py index 02ff04b..de83231 100644 --- a/apps/backend/app/services/auth.py +++ b/apps/backend/app/services/auth.py @@ -14,7 +14,10 @@ from datetime import UTC, datetime, timedelta from typing import NoReturn, Protocol from uuid import UUID, uuid4 +from fastapi import BackgroundTasks + from app.core.hashing import Argon2Hasher +from app.core.logging import get_logger from app.core.mailer import Mailer from app.core.principal import Principal from app.core.roles import AccountKind, Role @@ -34,6 +37,8 @@ from app.repositories.password_reset_token import PasswordResetTokenRepository from app.repositories.refresh_token import RefreshTokenRepository from app.repositories.user import UserRepository +logger = get_logger(__name__) + class Transaction(Protocol): async def commit(self) -> None: ... @@ -225,14 +230,21 @@ class AuthService: return self._session(self._en_principal(rafraichi or compte), secret) async def request_password_reset( - self, *, email: str, client_ip: str | None, user_agent: str | None + self, + *, + email: str, + client_ip: str | None, + user_agent: str | None, + background_tasks: BackgroundTasks, ) -> None: await self._refuse_si_limite_reset(email=email, client_ip=client_ip) compte = await self._users.get_by_email(email) # Piège : le hachage factice équilibre le temps de réponse sur un compte inconnu, comme # `authenticate()`. La réponse et sa forme restent identiques dans tous les cas : compte - # inconnu, compte inactif, ou email envoyé avec succès. + # inconnu, compte inactif, ou email envoyé avec succès. L'envoi SMTP lui-même est différé + # en tâche de fond : le laisser dans le chemin de réponse rouvrirait le même oracle par le + # temps (aller-retour réseau) et par la forme (500 si le relais SMTP échoue, contre 202). if compte is None or not compte.is_active or compte.kind != AccountKind.HUMAIN.value: await self._hasher.verify_dummy() await self._reset_attempts.record(email=email, client_ip=client_ip) @@ -260,7 +272,13 @@ class AuthService: await self._transaction.commit() lien = f"{self._reset_policy.frontend_reset_url}?token={secret}" - await self._mailer.send_password_reset_email(to=compte.email, reset_url=lien) + background_tasks.add_task(self._envoie_email_reset, compte.email, lien) + + async def _envoie_email_reset(self, email: str, reset_url: str) -> None: + try: + await self._mailer.send_password_reset_email(to=email, reset_url=reset_url) + except Exception: + logger.exception("auth.password_reset.mail_failed") async def confirm_password_reset( self, *, token: str, new_password: str, client_ip: str | None, user_agent: str | None @@ -269,6 +287,13 @@ class AuthService: if revendique is None: raise InvalidOrExpiredResetTokenError("Lien invalide ou expiré") + # Piège : le jeton peut avoir été émis avant une désactivation du compte. Sans cette + # relecture, un lien encore valide (15 min) changerait quand même le mot de passe d'un + # compte désactivé, réutilisable dès sa réactivation. + compte = await self._users.get_by_id(revendique.user_id) + if compte is None or not compte.is_active or compte.kind != AccountKind.HUMAIN.value: + raise InvalidOrExpiredResetTokenError("Lien invalide ou expiré") + await self._users.update_password( revendique.user_id, await self._hasher.hash(new_password), must_change_password=False ) diff --git a/apps/backend/tests/schemas/test_auth.py b/apps/backend/tests/schemas/test_auth.py index 7982f56..956e1e4 100644 --- a/apps/backend/tests/schemas/test_auth.py +++ b/apps/backend/tests/schemas/test_auth.py @@ -39,3 +39,23 @@ def test_password_change_request_rejects_a_password_below_the_minimum_length() - def test_valide_complexite_names_every_missing_class_in_the_error() -> None: with pytest.raises(ValueError, match=r"majuscule.*chiffre|chiffre.*majuscule"): valide_complexite("minuscules-seulement") + + +def test_valide_complexite_accepts_an_accented_password() -> None: + assert valide_complexite("Sécurité1!") == "Sécurité1!" + + +@pytest.mark.parametrize("mot_de_passe", ["abcdefg1×", "abcdefg1÷"]) # noqa: RUF001 +def test_valide_complexite_rejects_a_password_without_uppercase_despite_times_or_divide( + mot_de_passe: str, +) -> None: + with pytest.raises(ValueError, match="majuscule"): + valide_complexite(mot_de_passe) + + +@pytest.mark.parametrize("mot_de_passe", ["ABCDEFG1×", "ABCDEFG1÷"]) # noqa: RUF001 +def test_valide_complexite_rejects_a_password_without_lowercase_despite_times_or_divide( + mot_de_passe: str, +) -> None: + with pytest.raises(ValueError, match="minuscule"): + valide_complexite(mot_de_passe) diff --git a/apps/backend/tests/services/test_auth.py b/apps/backend/tests/services/test_auth.py index 52cde71..0c697b6 100644 --- a/apps/backend/tests/services/test_auth.py +++ b/apps/backend/tests/services/test_auth.py @@ -5,6 +5,7 @@ from typing import Any from uuid import UUID, uuid4 import pytest +from fastapi import BackgroundTasks from app.core.principal import Principal from app.core.roles import AccountKind, Role @@ -568,13 +569,16 @@ async def test_change_password_refuses_a_wrong_current_password() -> None: async def test_request_password_reset_emails_a_link_when_the_account_exists() -> None: compte = FauxCompte() attirail = fabrique_service(compte=compte) + taches = BackgroundTasks() await attirail.service.request_password_reset( - email=compte.email, client_ip="203.0.113.10", user_agent="pytest" + email=compte.email, client_ip="203.0.113.10", user_agent="pytest", background_tasks=taches ) assert attirail.jetons_reset.invalidations == [compte.id] assert attirail.jetons_reset.crees == [compte.id] + assert attirail.mailer.envois == [], "l'envoi doit être différé, pas fait dans la réponse" + await taches() assert len(attirail.mailer.envois) == 1 assert attirail.mailer.envois[0][0] == compte.email assert "auth.password_reset_requested" in attirail.audit.lignes[0][0] @@ -582,10 +586,15 @@ async def test_request_password_reset_emails_a_link_when_the_account_exists() -> async def test_request_password_reset_stays_silent_when_the_account_is_unknown() -> None: attirail = fabrique_service(compte=None) + taches = BackgroundTasks() await attirail.service.request_password_reset( - email="inconnu@enervision.fr", client_ip="203.0.113.10", user_agent="pytest" + email="inconnu@enervision.fr", + client_ip="203.0.113.10", + user_agent="pytest", + background_tasks=taches, ) + await taches() assert attirail.jetons_reset.crees == [] assert attirail.mailer.envois == [] @@ -595,10 +604,12 @@ async def test_request_password_reset_stays_silent_when_the_account_is_unknown() async def test_request_password_reset_stays_silent_when_the_account_is_inactive() -> None: compte = FauxCompte(is_active=False) attirail = fabrique_service(compte=compte) + taches = BackgroundTasks() await attirail.service.request_password_reset( - email=compte.email, client_ip="203.0.113.10", user_agent="pytest" + email=compte.email, client_ip="203.0.113.10", user_agent="pytest", background_tasks=taches ) + await taches() assert attirail.jetons_reset.crees == [] assert attirail.mailer.envois == [] @@ -606,15 +617,37 @@ async def test_request_password_reset_stays_silent_when_the_account_is_inactive( async def test_request_password_reset_raises_when_the_rate_limit_is_reached() -> None: attirail = fabrique_service(compteurs_reset=ResetRequestCounts(per_identifier=3, per_ip=0)) + taches = BackgroundTasks() with pytest.raises(RateLimitedError): await attirail.service.request_password_reset( - email="operateur@enervision.fr", client_ip="203.0.113.10", user_agent="pytest" + email="operateur@enervision.fr", + client_ip="203.0.113.10", + user_agent="pytest", + background_tasks=taches, ) + await taches() assert attirail.mailer.envois == [] +async def test_request_password_reset_logs_instead_of_raising_when_the_mailer_fails() -> None: + compte = FauxCompte() + attirail = fabrique_service(compte=compte) + taches = BackgroundTasks() + + async def echoue(*, to: str, reset_url: str) -> None: + raise RuntimeError("relais SMTP indisponible") + + attirail.mailer.send_password_reset_email = echoue # type: ignore[method-assign] + + await attirail.service.request_password_reset( + email=compte.email, client_ip="203.0.113.10", user_agent="pytest", background_tasks=taches + ) + + await taches() + + async def test_confirm_password_reset_revokes_every_session_then_reopens_the_current_one() -> None: compte = FauxCompte() jetons_reset = FauxDepotJetonsReset( @@ -637,6 +670,25 @@ async def test_confirm_password_reset_revokes_every_session_then_reopens_the_cur assert "auth.password_reset_self_service" in attirail.audit.lignes[0][0] +async def test_confirm_password_reset_rejects_a_token_for_an_account_disabled_since() -> None: + compte = FauxCompte(is_active=False) + jetons_reset = FauxDepotJetonsReset( + revendique=ConsumedResetToken(id=uuid4(), user_id=compte.id) + ) + attirail = fabrique_service(compte=compte, jetons_reset=jetons_reset) + + with pytest.raises(InvalidOrExpiredResetTokenError): + await attirail.service.confirm_password_reset( + token="un-secret-opaque", + new_password="Un-nouveau-mot-de-passe1!", + client_ip="203.0.113.10", + user_agent="pytest", + ) + + assert attirail.comptes.mots_de_passe_changes == 0 + assert attirail.jetons.revocations_par_compte == [] + + async def test_confirm_password_reset_rejects_an_invalid_or_expired_token() -> None: attirail = fabrique_service(jetons_reset=FauxDepotJetonsReset(revendique=None)) diff --git a/apps/frontend/src/app/shared/validators/password.validator.spec.ts b/apps/frontend/src/app/shared/validators/password.validator.spec.ts new file mode 100644 index 0000000..455ee36 --- /dev/null +++ b/apps/frontend/src/app/shared/validators/password.validator.spec.ts @@ -0,0 +1,26 @@ +import { FormControl } from '@angular/forms'; +import { passwordValidators } from './password.validator'; + +function estValide(motDePasse: string): boolean { + return new FormControl(motDePasse, passwordValidators).valid; +} + +describe('passwordValidators', () => { + it('accepte un mot de passe couvrant les quatre classes', () => { + expect(estValide('Un-mot-de-passe1!')).toBe(true); + }); + + it('accepte un mot de passe accentué (alignement avec le backend, ex: "Sécurité1")', () => { + expect(estValide('Sécurité1!')).toBe(true); + }); + + it('refuse un mot de passe sans majuscule même avec un "×" ou un "÷"', () => { + expect(estValide('abcdefg1×')).toBe(false); + expect(estValide('abcdefg1÷')).toBe(false); + }); + + it('refuse un mot de passe sans minuscule même avec un "×" ou un "÷"', () => { + expect(estValide('ABCDEFG1×')).toBe(false); + expect(estValide('ABCDEFG1÷')).toBe(false); + }); +}); diff --git a/apps/frontend/src/app/shared/validators/password.validator.ts b/apps/frontend/src/app/shared/validators/password.validator.ts index fac1359..9f95863 100644 --- a/apps/frontend/src/app/shared/validators/password.validator.ts +++ b/apps/frontend/src/app/shared/validators/password.validator.ts @@ -1,3 +1,9 @@ +// Contrainte : `PASSWORD_PATTERN` doit rester identique au validateur Pydantic de +// `app/schemas/auth.py` côté backend (mêmes plages de majuscules/minuscules, excluant +// × et ÷, mêmes chiffres 0-9, même jeu de caractères spéciaux). `\w`/`\d` divergent entre +// JavaScript (ASCII) et Python (Unicode) : une négation aurait accepté ou rejeté un même +// mot de passe différemment d'un côté à l'autre (ex. "Sécurité1"). + import { Validators } from '@angular/forms'; export const PASSWORD_MIN_LENGTH = 8; @@ -5,7 +11,11 @@ export const PASSWORD_MAX_LENGTH = 128; export const PASSWORD_HINT = '8 à 128 caractères, avec au moins 1 majuscule, 1 minuscule, 1 chiffre et 1 caractère spécial'; -const PASSWORD_PATTERN = /^(?=.*[A-ZÀ-Ý])(?=.*[a-zà-ÿ])(?=.*\d)(?=.*[^\w\s]).*$/; +const SPECIAL_CHARACTERS = '!@#$%^&*()\\-_=+[\\]{};:,.?'; +const PASSWORD_PATTERN = new RegExp( + `^(?=.*[A-ZÀ-ÖØ-Þ])(?=.*[a-zà-öø-þ])` + + `(?=.*[0-9])(?=.*[${SPECIAL_CHARACTERS}]).*$`, +); export const passwordValidators = [ Validators.required,