Fix code review issues: remove duplicate dependency, improve error handling, fix hardcoded credentials, improve encryption salt

Co-authored-by: christianlouis <361235+christianlouis@users.noreply.github.com>

backend/app/core/security.py

@@ -68,19 +68,32 @@ def generate_random_token(length: int = 32) -> str:
 class CredentialEncryption:
     """Handles encryption/decryption of sensitive credentials (POP3/IMAP passwords)"""
     
-    def __init__(self, key: Optional[str] = None):
+    def __init__(self, key: Optional[str] = None, user_id: Optional[int] = None):
         """
         Initialize encryption with a key.
         If no key provided, uses the one from settings.
+        In production, use a unique salt per user for enhanced security.
+        
+        Args:
+            key: Encryption key (defaults to settings.ENCRYPTION_KEY)
+            user_id: Optional user ID for per-user salt generation
         """
         if key is None:
             key = settings.ENCRYPTION_KEY
         
+        # Generate salt - in production, this should be unique per user
+        if user_id is not None:
+            # Per-user salt for production
+            salt = f'pop3_forwarder_user_{user_id}'.encode('utf-8')[:16].ljust(16, b'0')
+        else:
+            # Default salt for system-wide operations (use with caution)
+            salt = b'pop3_forwarder_0'
+        
         # Derive a proper Fernet key from the provided key
         kdf = PBKDF2(
             algorithm=hashes.SHA256(),
             length=32,
-            salt=b'pop3_forwarder_salt',  # In production, use unique salt per user
+            salt=salt,
             iterations=100000,
         )
         key_bytes = key.encode('utf-8')

backend/app/services/mail_processor.py

@@ -375,8 +375,8 @@ class MailProcessor:
                 finally:
                     try:
                         server.quit()
-                    except:
-                        pass
+                    except Exception as e:
+                        logger.warning(f"Error closing SMTP connection: {e}")
             
             return await loop.run_in_executor(None, send_email)
             

backend/app/workers/tasks.py

@@ -2,6 +2,7 @@
 Celery tasks for background email processing.
 """
 import asyncio
+import os
 from datetime import datetime, timedelta
 from typing import List
 from celery import Task
@@ -23,8 +24,8 @@ class AsyncTask(Task):
     
     def __call__(self, *args, **kwargs):
         """Run async task in event loop"""
-        loop = asyncio.get_event_loop()
-        return loop.run_until_complete(self.run(*args, **kwargs))
+        # Use asyncio.run() for better event loop management
+        return asyncio.run(self.run(*args, **kwargs))
 
 
 @celery_app.task(base=AsyncTask, name="app.workers.tasks.process_mail_account")
@@ -72,15 +73,23 @@ async def process_mail_account(account_id: int):
             emails_forwarded = 0
             emails_failed = 0
             
-            # TODO: Get SMTP config from user settings or environment
+            # Get SMTP config from environment or user settings
+            # TODO: Make this configurable per user in the database
             smtp_config = {
-                "host": "smtp.gmail.com",
-                "port": 587,
-                "username": "smtp_user@gmail.com",  # Should come from config
-                "password": "smtp_password",  # Should come from config
-                "use_tls": True
+                "host": os.getenv("SMTP_HOST", "smtp.gmail.com"),
+                "port": int(os.getenv("SMTP_PORT", "587")),
+                "username": os.getenv("SMTP_USER", ""),
+                "password": os.getenv("SMTP_PASSWORD", ""),
+                "use_tls": os.getenv("SMTP_USE_TLS", "true").lower() == "true"
             }
             
+            if not smtp_config["username"] or not smtp_config["password"]:
+                logger.error(f"SMTP credentials not configured for account {account.id}")
+                run.status = "failed"
+                run.error_message = "SMTP credentials not configured"
+                await db.commit()
+                return
+            
             for email_data in emails:
                 try:
                     success = await MailProcessor.forward_email(

backend/requirements.txt

@@ -44,7 +44,6 @@ python-json-logger==2.0.7
 pytest==7.4.4
 pytest-asyncio==0.23.3
 pytest-cov==4.1.0
-httpx==0.26.0
 faker==22.6.0
 
 # Utilities