fix(imap): address code review feedback - named constants, error context in JS, security docs

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

SECURITY_AUDIT.md

@@ -861,3 +861,21 @@ Security headers implementation is complete and production-ready. The middleware
 ---
 
 **Next Audit Due:** 2026-05-07 (Quarterly)
+
+## Per-User IMAP Account Passwords (Added 2026-03-08)
+
+### Known Limitation: Plain-text Password Storage
+
+IMAP account passwords in the `user_imap_accounts` table are stored in plain text in the database.
+
+**Risk:** Anyone with direct database access (DBA, backup access) can read IMAP credentials for all users.
+
+**Mitigations in place:**
+- Database itself should be protected with appropriate OS-level file permissions (SQLite) or network ACLs (PostgreSQL/MySQL).
+- Passwords are never returned in API responses (the `_to_response` serialiser omits them).
+- Only the account owner can read or update their own accounts (ownership enforced at the API layer).
+- Passwords are never logged.
+
+**Future improvement:** Encrypt IMAP passwords at rest using `cryptography.fernet` (symmetric encryption with the app's `SESSION_SECRET` as key material). This is tracked as a TODO item in `app/api/imap_accounts.py` and should be implemented before this feature is used in high-security environments.
+
+**Recommended admin action:** Use app-specific passwords (Gmail, Outlook) rather than account passwords where possible, so that compromised IMAP credentials can be revoked without affecting the user's primary account.

app/models.py

@@ -402,7 +402,9 @@ class UserImapAccount(Base):
     host = Column(String(255), nullable=False)
     port = Column(Integer, nullable=False, default=993)
     username = Column(String(255), nullable=False)
-    # Password stored in plain text — the admin is responsible for access control
+    # Password stored in plain text — the admin is responsible for access control.
+    # TODO: Encrypt at rest using cryptography.fernet before deploying in high-security
+    # environments. See SECURITY_AUDIT.md for full risk assessment and mitigation notes.
     password = Column(String(1024), nullable=False)
     use_ssl = Column(Boolean, nullable=False, default=True)
 

app/tasks/imap_tasks.py

@@ -18,6 +18,9 @@ from app.utils.allowed_types import ALLOWED_EXTENSIONS, ALLOWED_MIME_TYPES
 # Database session for per-user IMAP accounts (imported lazily to avoid circular imports)
 _db_session_factory = None
 
+# Maximum length to store as last_error to prevent DB bloat
+_MAX_ERROR_LENGTH = 500
+
 
 def _get_db_session():
     """Return a new SQLAlchemy session (lazy import to avoid startup issues)."""
@@ -149,10 +152,11 @@ def _pull_user_imap_accounts() -> None:
             accounts = db.query(UserImapAccount).filter(UserImapAccount.is_active.is_(True)).all()
             logger.info("Processing %d per-user IMAP account(s)", len(accounts))
             for acct in accounts:
-                mailbox_key = f"user_{acct.owner_id}_{acct.id}"
+                # Use a descriptive identifier for logging and processed-email cache keys
+                account_identifier = f"user_{acct.owner_id}_{acct.id}"
                 try:
                     pull_inbox(
-                        mailbox_key=mailbox_key,
+                        mailbox_key=account_identifier,
                         host=acct.host,
                         port=acct.port,
                         username=acct.username,
@@ -165,7 +169,7 @@ def _pull_user_imap_accounts() -> None:
                     acct.last_error = None
                     db.commit()
                 except Exception as exc:  # noqa: BLE001
-                    error_msg = str(exc)[:500]
+                    error_msg = str(exc)[:_MAX_ERROR_LENGTH]
                     logger.error(
                         "Error pulling user IMAP account %d (%s@%s): %s",
                         acct.id,

frontend/templates/imap_accounts.html

@@ -538,7 +538,8 @@ function imapAccountsApp() {
       if (!iso) return '';
       try {
         return new Date(iso).toLocaleString();
-      } catch {
+      } catch (e) {
+        console.warn('Failed to parse date:', iso, e);
         return iso;
       }
     },
@@ -625,7 +626,7 @@ function imapAccountsApp() {
         this.closeModal();
         this.showAlert('success', 'Saved', this.editingAccount ? 'Account updated.' : 'Account added successfully.');
       } catch (err) {
-        this.formError = 'Network error. Please try again.';
+        this.formError = `Network error: ${err.message || 'Unknown error'}. Please try again.`;
       } finally {
         this.saving = false;
       }
@@ -659,8 +660,8 @@ function imapAccountsApp() {
         });
         const data = await resp.json();
         this.testResult = data;
-      } catch {
+      } catch (err) {
-        this.testResult = { success: false, message: 'Network error during test.' };
+        this.testResult = { success: false, message: `Network error: ${err.message || 'Unknown error'}` };
       } finally {
         this.testing = false;
       }
@@ -679,8 +680,8 @@ function imapAccountsApp() {
         } else {
           this.showAlert('error', `${acct.name}: Connection Failed`, data.message);
         }
-      } catch {
+      } catch (err) {
-        this.showAlert('error', 'Test Failed', 'Network error.');
+        this.showAlert('error', 'Test Failed', `Network error: ${err.message || 'Unknown error'}`);
       } finally {
         this.testingId = null;
       }
@@ -712,8 +713,8 @@ function imapAccountsApp() {
           const data = await resp.json();
           this.showAlert('error', 'Delete Failed', data.detail || 'Could not delete account.');
         }
-      } catch {
+      } catch (err) {
-        this.showAlert('error', 'Delete Failed', 'Network error.');
+        this.showAlert('error', 'Delete Failed', `Network error: ${err.message || 'Unknown error'}`);
       } finally {
         this.deleting = false;
       }