From 73727dc56a9a5dc46d49c9f96065dd4bec7681b7 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 8 Mar 2026 13:08:31 +0000 Subject: [PATCH] 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 | 18 ++++++++++++++++++ app/models.py | 4 +++- app/tasks/imap_tasks.py | 10 +++++++--- frontend/templates/imap_accounts.html | 17 +++++++++-------- 4 files changed, 37 insertions(+), 12 deletions(-) diff --git a/SECURITY_AUDIT.md b/SECURITY_AUDIT.md index 03b229fd..3f63b48e 100644 --- a/SECURITY_AUDIT.md +++ b/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. diff --git a/app/models.py b/app/models.py index a1bb99d5..309f76d8 100644 --- a/app/models.py +++ b/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) diff --git a/app/tasks/imap_tasks.py b/app/tasks/imap_tasks.py index 4677fa41..2b85dc9d 100644 --- a/app/tasks/imap_tasks.py +++ b/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, diff --git a/frontend/templates/imap_accounts.html b/frontend/templates/imap_accounts.html index c4471b11..c6c7ed70 100644 --- a/frontend/templates/imap_accounts.html +++ b/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 { - this.testResult = { success: false, message: 'Network error during test.' }; + } catch (err) { + 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 { - this.showAlert('error', 'Test Failed', 'Network error.'); + } catch (err) { + 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 { - this.showAlert('error', 'Delete Failed', 'Network error.'); + } catch (err) { + this.showAlert('error', 'Delete Failed', `Network error: ${err.message || 'Unknown error'}`); } finally { this.deleting = false; }