Refactor auth service and middleware with security improvements - #1
Merged
Merged
Conversation
Endpoints cassés :
- confirmTelService n'était pas importé dans le contrôleur : /confirm-tel
répondait systématiquement 502. Le service lisait de plus
verification_email_code au lieu de verification_tel_code et renseignait
email_verified_at au lieu de tel_verified_at.
- verifyToken vérifiait payload.user_uuid alors que generateToken signe
uuid : /refresh-token échouait toujours et délivrait des tokens portant
un uuid undefined. La fonction lève désormais une erreur explicite et
contrôle aussi le type du token.
- logout n'était pas encapsulé dans asyncHandler et déréférençait
req.headers.authorization sans vérification : requête suspendue quand
l'entête était absente.
- saveToken écrivait dans une colonne user_id inexistante (user_uuid).
- logger n'était pas importé dans tokenService : le catch de
generateAuthTokens masquait l'erreur réelle par une ReferenceError.
- createUser réassignait une const et getTokenByDetails masquait le helper
error() par la variable du catch : TypeError dans les deux cas.
- STATUS_ACTIVE valait 1 alors que la colonne est un ENUM
('active','inactive','deleted','blocked').
Sécurité :
- protect acceptait un refresh token comme token d'accès et ignorait
l'indicateur blacklisted. Le type est désormais vérifié et next() est
appelé hors du try pour ne plus émettre de double réponse.
- change-password ciblait l'uuid fourni dans le body : il provient
maintenant du token d'accès. Le champ reste accepté mais est ignoré.
- Suppression des console.log qui exposaient tokens, codes de vérification
et enregistrements utilisateur.
- Les identifiants de base de données ne sont plus committés : config.json
est remplacé par config.js alimenté par le .env.
- npm audit : 18 vulnérabilités (2 critiques, 11 hautes) ramenées à 3.
Divers :
- Le handler 404 renvoyait un HTTP 200 avec une charge utile « 403 » ;
errorHandler n'était jamais branché et répondait 200 sur erreur.
- Ajout de .sequelizerc : sequelize-cli ne trouvait ni la configuration ni
les migrations situées dans src/.
- Migration Users : uuid était déclaré en seconde clé primaire à côté d'un
id auto-incrémenté (désormais unique + non nul).
- SMTP : port converti en nombre et TLS implicite activé sur le port 465.
- Un échec d'envoi d'email ne fait plus échouer l'inscription.
- Les services renvoyaient undefined sur certains chemins, ce qui faisait
planter les contrôleurs.
- README : les routes documentées (/api/auth/...) ne correspondaient pas
aux routes réelles (/auth/user/...).
- .gitignore nettoyé, .idea retiré du suivi, dépendances inutilisées
supprimées.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VHL2eLC1T5kD31UxYxyxGX
Expiration des codes de vérification : L'expiration s'appuyait sur updatedAt : n'importe quelle mise à jour du compte (changement de profil, de statut...) repoussait la validité du code de 5 minutes supplémentaires. Une colonne verification_code_expires_at est désormais posée à la génération du code et remise à null à sa consommation. Un code sans date d'expiration est considéré comme expiré, afin que les comptes créés avant la migration redemandent simplement un code. La durée de validité est centralisée dans verificationCodeConstant.EXPIRATION_MINUTES et reprise dans le texte des emails. La requête utilisateur redondante de confirmEmailService et confirmTelService, qui n'existait que pour lire updatedAt, est supprimée. Dépendances : - nodemailer 6.x -> 9.x et uuid 9.x -> 14.x, ce qui résout les 3 vulnérabilités restantes. Vérifié par un aller-retour SMTP réel : la compilation du template handlebars et l'envoi fonctionnent avec et sans identifiants. - sendEmail n'envoie plus de bloc auth quand SMTP_EMAIL est vide : les relais de développement sans authentification refusaient la connexion. Les 2 alertes restantes concernent uuid imbriqué dans les dépendances de sequelize 6 ; l'unique remède proposé par npm est une rétrogradation vers sequelize 3, et l'avis ne porte que sur uuid v3/v5/v6 appelés avec un argument buf, ce que le projet ne fait pas. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VHL2eLC1T5kD31UxYxyxGX
Suite de tests (80 tests, 5 fichiers) sur SQLite en mémoire : ni serveur MySQL ni fichier .env requis. Les emails sont mockés partout sauf dans tests/email.test.js, qui démarre un vrai serveur SMTP local pour vérifier que le template handlebars est réellement compilé et remis. Les tests couvrent en particulier les régressions corrigées sur cette branche : typage des tokens, blacklist, expiration des codes, ciblage du compte par le token et non par le corps de la requête, 404 avec le bon code HTTP, déconnexion sans entête Authorization. Deux défauts mis au jour par les tests : - generateToken ne produisait pas de token unique. Le payload ne contenant que uuid, iat (à la seconde), exp et type, deux tokens émis dans la même seconde pour le même compte étaient identiques au bit près. La rotation du refresh token était donc sans effet : le contrôleur détruisait l'ancien puis en réémettait un identique, laissant valide un token censé être révoqué. Ajout d'un jti unique. - Le jti porte le JWT à 272 caractères, au-delà des 255 de la colonne token. MySQL en mode strict aurait rejeté l'insertion, et l'aurait tronquée sinon. La colonne passe à 512 via une migration, et un test compare la longueur émise à la largeur déclarée du modèle, car SQLite n'applique pas les longueurs de colonne. uuid revient de 14.x à ^11.1.1 : les versions 13+ ne fournissent plus de build CommonJS, et require() n'y fonctionne que sur Node >= 20.19 via le support require(esm). Le projet étant en CommonJS, le runtime Jest levait une SyntaxError. La 11.1.1 corrige l'avis de sécurité (qui porte sur < 11.1.1) tout en conservant le build CJS. Le logger n'installe plus de rotation de fichiers en environnement de test : cela laissait des descripteurs ouverts et polluait logs/. CI (.github/workflows/tests.yml) : suite exécutée sur Node 20 et 22, plus un job vérifiant que les migrations s'appliquent et se déroulent dans les deux sens. Node 18 est exclu car sqlite3 exige Node >= 20.17. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VHL2eLC1T5kD31UxYxyxGX
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR refactors the authentication service and middleware to improve security, error handling, and code maintainability. Key improvements include proper token type validation, account status checks, safer logout handling, and migration of database configuration to environment variables.
Key Changes
Security Improvements
verifyToken()to ensure refresh tokens cannot be used where access tokens are requiredchangePasswordto use UUID from authenticated token instead of request body, preventing users from targeting other accountsAuthentication Service (
authService.js)confirmEmailServiceandconfirmTelServiceto validate user existence and verification code presenceconfirmTelServiceto use correct field names (verification_tel_codeinstead ofverification_email_code)sendResetPasswordCodeServiceandsendMobileResetPasswordCodeServicemessagevariable from const to let for conditional updatesAuth Middleware (
authMiddleware.js)protectmiddleware to useverifyToken()service with proper token type checkingverifyRefreshTokento validate token type and blacklist statusToken Service (
tokenService.js)verifyToken()to throw errors instead of returning error objects for better async/await handlinguuidfield (was checking non-existentuser_uuid)saveToken()to useuser_uuidinstead ofuser_idConfiguration Management
src/config/config.jsonwithsrc/config/config.jsthat reads from.env.sequelizercto properly configure Sequelize CLI paths.env.examplewith database configuration variablesDatabase & Models
uuidfrom primary key to unique constraint with UUIDV4 defaultnomfield to be non-nullable'active','inactive','deleted','blocked'Error Handling
errorMiddleware.jsto properly handle response status codes and prevent double responsessendEmail.jswith better error messagesAPI Documentation
/auth/user/prefix)confirm-tel,mobile-reset-password)Code Quality
logoutcontroller use asyncHandler for consistent error handlinghttps://claude.ai/code/session_01VHL2eLC1T5kD31UxYxyxGX