Security and quality fixes #49
Loading…
Reference in a new issue
No description provided.
Delete branch "feature/security-and-quality-fixes"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Critical and Important fixes from a multi-agent code review (1 of 3 repos).
Auth and session
Errors and validation
Validators and crypto
Document upload hardening
Cache
Repository
Operations
EF migration
Verification
- JwtUtils.VerifyToken now returns the verified ClaimsPrincipal (or null) and enforces ValidateLifetime=true with ClockSkew=30s. Removes the unsafe ReadJwtToken public API; callers must use the verified principal path. - AddJwt in DI explicitly sets ValidateLifetime=true / ClockSkew=30s. - Claim shapes follow the cross-client contract: permissions -> JSON number (long) emailActive/accountActive -> JSON booleans roles -> JSON array (always an array, even for a single role) - Permission.Value: int -> long; bitmask helpers / claim parsing follow. - HashUtils SHA-256-derives keys once via Lazy<byte[]> (no per-call derivation). - HashUtils, JwtUtils, PasswordUtils, TokenUtils made static. - Permission filter uses the new in-memory IPermissionCatalog (constant-time label -> bit lookup, no per-request DB hit) and a 60s-TTL cached user DTO for the defence-in-depth re-check. - AccountActiveRequirement filter reads the same 60s-TTL cached DTO. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>- Validator.ValidateColor: condition was inverted (it threw on a *valid* hex code). Flip the check and fix the "hexadecimal" -> "hexadécimal" typo. - PasswordValidator: the previous regex required runs of 2/2/2/3 of each character class, which is much stricter than intended. Rewrite to the contract documented in the XML comment: * at least 12 chars * at least one digit * at least one upper-case letter * at least one lower-case letter * at least one non-alphanumeric symbol ([^A-Za-z0-9]) Clients must mirror this in cesizen-front/src/app/auth/constants/password.ts and cesizen-mobile/constants/password.ts. - PositiveValidator: "posifitif" -> "positif". Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>- Drop image/svg+xml from MimeTypes.ImageMimeTypes (scriptable format, would enable stored XSS when served inline). - Add FileSignatures: known magic-byte signatures for PNG, JPEG/JPG, GIF, WebP and PDF, plus a sniffed-mime -> allowed-extension map. - FileValidator.ValidateFileType: for any mime in the signatures map, compare the leading bytes of the upload to the candidate signatures and reject mismatches with a ValidationException. - FileValidator.ValidateFileSize: cap at 5 MiB (was 20 MB). - S3Service.GenerateKey: build the extension from the sniffed mime via the allowlist (^[a-z0-9]{1,5}$), never from the client-supplied filename. Refuse null bytes / path-traversal sequences in the id defensively even though the result is not used as a filesystem path. - DocumentController.RequestSizeLimit mirrors the 5 MiB cap. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>- GetOrCreateAsync now coalesces concurrent misses for the same key via a refcounted per-key SemaphoreSlim (ConcurrentDictionary). Cache stampede protection without unbounded growth. - New tag-based invalidation: keys can register against one or more "tag" Redis Sets at write time; InvalidateTagAsync deletes every key the tag set knows about plus the tag set itself. Replaces the KEYS *-style full-DB scan for new code (RemoveByPatternAsync kept for back-compat). - UserCacheKeys constants: users:model -> full User EF model (UserService writers) users:dto -> public ReturnUserDto (PermissionService / filter readers) Two distinct prefixes prevent the shape-collision that issue #16 flagged. Both invalidated together via the "tag:users" tag. - New SetAsync overload takes params string[] tags; GetOrCreateAsync forwards. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>- UserService.LogUser: unify error responses for "unknown username" and "wrong password" (uniform AuthenticationException with the same French message — "Identifiants invalides" — so an attacker can no longer enumerate usernames via differing responses). Add explicit IsActive / IsEmailVerified checks AFTER credentials validate; throw the new AccountDisabledException / EmailNotVerifiedException so the global handler emits a 403 with distinct messages and clients can route accordingly. - UserService.RefreshToken: rewritten — verify signature FIRST, then read claims from the verified principal, then DB lookup, then validator (hash + ValidUntil + absolute cap). New ValidUntil = min(now + rotation, OriginalCreatedAt + maxSession). Also blocks deactivated / unverified accounts at refresh time. - UserService.{UpdateUser,ToggleAccountActivation,DeleteUser,UpdateUserRole}: load from DB and use SavesChanges() (or DeleteAsync); centralized InvalidateUserCaches() invalidates both user prefixes and the user tag. - UserService.{EncryptUserInfo}: first/last name now use RandomizedEncrypt (AES-GCM with random nonce, non-searchable, no plaintext leak); email and username stay deterministic (still searchable in the DB). DecryptUserInfo uses HashUtils.DecryptName which tries the new format first and falls back to deterministic for existing rows. - PasswordService rewrite: * Add LastFailedLoginAt column for time-based unlock; after AuthConstants.FailedLoginResetMinutes (15) the counter resets. * Method renamed VerifiyLoginPasswords -> VerifyLoginPasswords (typo). * UpdatePassword resets FailCount + LastFailedLoginAt on success. * ResetPassword deletes the verification token after a successful reset (mirrors EmailService.CheckVerificationEmail). * Service now depends on IVerificationTokenRepository (DI updated). - UserController: * [AccountActiveRequirement] applied to UpdateUserEmail (self). * GetCurrentUser / DeleteLoggedUser intentionally remain reachable for unverified users (recovery path). * Migrate to ApiResponse.Ok<T>(data, statusCode) factory. * CancellationToken plumbing on every action. - UserRepository: "Aucun utilisater" typo -> "Aucun utilisateur". - PermissionService / EmailService: read from / invalidate the new split user cache prefixes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>- Rate limiting: built-in Microsoft.AspNetCore.RateLimiting; per-IP fixed window of 10 req/min on the "auth" policy. Applied via [EnableRateLimiting("auth")] to: POST /user/login, /user/register, /user/refresh POST /password/reset, /password/reset/request POST /emails/verification/check - Health checks: AddCheck<DatabaseHealthCheck> against EF Core's CanConnect, exposed at GET /health. - DocumentController: CancellationToken plumbing + ApiResponse.Ok factory. - Typo: RespirationExerciseService "respicationExercises" -> "respirationExercises" and "Aucun exercise" -> "Aucun exercice". - .gitignore: also ignore .env.local and .env.*.local. - Migration AllowMultipleRefreshTokensPerUser+AddRefreshTokenOriginalCreatedAt+ AddPermissionValueLong+AddLastFailedLoginAt -- single migration 20260512192455_SecurityAndQualityFixes: * RefreshTokens: drop unique IX on UserId, recreate non-unique, add OriginalCreatedAt column. * Permissions: Value int -> bigint, seeded long literals. * Passwords: add LastFailedLoginAt timestamp (nullable). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>Live integration testing surfaced that ASP.NET Core's built-in model-binding validation returns a ProblemDetails envelope ({type, title, status, errors, traceId}) on 400s — bypassing GlobalExceptionHandler and the new unified envelope. Clients reading body.data.message defensively would silently fall through to "undefined". Wire InvalidModelStateResponseFactory to project ModelState errors into the same ApiResponse<ErrorData> shape used by every other 4xx, so every validation error funnels through the same client-side toast path.