fix: resolve critical security vulnerabilities and reliability issues - #18
Merged
Merged
Conversation
Critical fixes: - Remove hardcoded S3 credentials from download-practice-resources.ps1 (use environment variables OSCA_ACCESS_KEY_ID/OSCA_SECRET_ACCESS_KEY) - Fix timing attack in FileService GatewayUser (use FixedTimeEquals) - Fix timing attack in InternalServiceAccessPolicy (use FixedTimeEquals) - Replace plaintext admin password with hashed storage (PasswordHasher) - Add X-Gateway-Key authentication to OCRService (was completely unauthenticated) High-priority fixes: - Increase password reset token entropy from 6-digit numeric (~20 bits) to 8-char alphanumeric (~39 bits) using unambiguous alphabet - Fix email enumeration in password-reset-requests endpoint (always 202) - Add 50-page PDF limit to OCRService to prevent resource exhaustion DoS - Fix exception message leakage in GalGameService job failure response - Make MongoGameStore.SavePackage atomic via MongoDB transactions (with graceful fallback for standalone MongoDB without replica sets) Configuration updates: - compose.integration.yaml: pass GATEWAY_KEY to ocr-service, use Admin__PasswordHash instead of Admin__Password - appsettings.Development.json: use PBKDF2 hash for admin password
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
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 addresses 4 critical and 5 high-priority security vulnerabilities and reliability issues identified during a comprehensive code audit of the GalReview repository.
Critical Fixes
1. Hardcoded S3 Credentials Removed (
scripts/download-practice-resources.ps1)ZMVBNId4c52092lL53Jg) and secret key (upDcWNLGKlhe2qVSKOYG) were hardcoded in the repositoryOSCA_ACCESS_KEY_IDandOSCA_SECRET_ACCESS_KEYenvironment variables2. Timing Attack in FileService (
backend/FileService/Program.cs,InternalServiceAccessPolicy.cs)GatewayUser()andHasSingleExactValue()used==/string.Equals()for gateway key comparison, vulnerable to timing side-channel attacksCryptographicOperations.FixedTimeEquals()for constant-time comparison3. Plaintext Admin Password (
backend/AuthService/Program.cs,appsettings.Development.json)"Password": "admin")Admin:PasswordHashusing ASP.NET Core IdentityPasswordHasher(PBKDF2-HMAC-SHA256). LegacyAdmin:Passwordis supported as fallback for migration.compose.integration.yamlupdated to useAdmin__PasswordHash.4. Unauthenticated OCRService (
backend/OCRService/app.py)X-Gateway-Keyauthentication middleware usinghmac.compare_digest()for constant-time validation. FileService updated to send the key header.compose.integration.yamlupdated to passGATEWAY_KEYto ocr-service.High-Priority Fixes
5. Weak Password Reset Token (
backend/AuthService/Program.cs)6. Email Enumeration (
backend/AuthService/Program.cs)7. PDF DoS Vulnerability (
backend/OCRService/app.py)MAX_PDF_PAGES = 50limit with HTTP 413 response8. Exception Message Leakage (
backend/GalGameService/Program.cs)ApiErroron job failure9. Non-Atomic Cross-Collection Write (
backend/GalGameService/MongoGameStore.cs)SavePackage()wrote to 3 MongoDB collections (packages,manifests,owners) without a transaction — partial failures could leave inconsistent stateFiles Changed
scripts/download-practice-resources.ps1backend/FileService/Program.csGatewayUser, pass gateway key to OCRbackend/FileService/InternalServiceAccessPolicy.csbackend/AuthService/Program.csbackend/AuthService/appsettings.Development.jsonbackend/AuthService/InMemoryRepositories.csbackend/OCRService/app.pybackend/GalGameService/Program.csbackend/GalGameService/MongoGameStore.cscompose.integration.yamlTesting