From 550ac015eb88e5eec8c538cffa49fda55bfbe14b Mon Sep 17 00:00:00 2001 From: Alexandre Teixeira <111787685+alteixeira20@users.noreply.github.com> Date: Tue, 20 Oct 2026 10:35:30 +0100 Subject: [PATCH] fix(email): enforce account ownership and correct partial failure accounting - Validate account ownership with _assert_owns_account for every account_id - Deduplicate items in selection to prevent duplicate files and skewed accounting - Fix exception handling in IMAP batching to avoid double-counting on partial failures - Re-raise HTTPException so 413 memory cap errors are not swallowed - Remove unused addField helper in emailLibrary print exporter - Add behavioral test suite covering empty/oversized validation, rfc822 export, zip export, partial failure/dedup, and cross-tenant account isolation --- routes/email/email_routes.py | 19 ++- static/js/emailLibrary/index.js | 13 -- tests/test_email_library_bulk_actions.py | 168 ++++++++++++++++++++++- 3 files changed, 183 insertions(+), 17 deletions(-) diff --git a/routes/email/email_routes.py b/routes/email/email_routes.py index c3d08f785..0af2d0a11 100644 --- a/routes/email/email_routes.py +++ b/routes/email/email_routes.py @@ -4281,6 +4281,7 @@ def setup_email_routes(): logger.debug("Skipping malformed fixture email attachment uid=%s", uid, exc_info=True) return msg.as_bytes(policy=email_policy.SMTP) + seen_items: set[tuple[str | None, str, str]] = set() remote_groups: dict[tuple[str | None, str], dict[str, tuple[str, dict]]] = {} with zipfile.ZipFile(zip_buf, "w", compression=zipfile.ZIP_DEFLATED) as zf: for row in messages: @@ -4293,6 +4294,15 @@ def setup_email_routes(): continue folder = str(row.get("folder") or "INBOX").strip() or "INBOX" account_id = str(row.get("account_id") or row.get("account") or "").strip() or None + if account_id: + _assert_owns_account(account_id, owner) + + item_key = (account_id, folder, uid) + if item_key in seen_items: + skipped += 1 + continue + seen_items.add(item_key) + fixture = _fixture_email_read(uid, folder, owner) if fixture is not None and not fixture.get("error"): filename = _email_filename(row, uid) @@ -4319,6 +4329,7 @@ def setup_email_routes(): # Fetch original RFC822 data in batches and store it byte-for-byte. for (account_id, folder), rows_by_uid in remote_groups.items(): rows = list(rows_by_uid.items()) + remaining_rows = len(rows) try: with _imap(account_id, owner=owner) as conn: conn.select(_q(folder), readonly=True) @@ -4329,6 +4340,7 @@ def setup_email_routes(): status, msg_data = _imap_uid_fetch(conn, uid_set, "(UID RFC822)") if status != "OK" or not msg_data: skipped += len(batch) + remaining_rows -= len(batch) continue found: set[str] = set() for meta, raw_message in _group_uid_fetch_records(msg_data): @@ -4351,9 +4363,12 @@ def setup_email_routes(): found.add(uid) exported += 1 skipped += len(batch) - len(found) + remaining_rows -= len(batch) + except HTTPException: + raise except Exception as exc: - skipped += len(rows) - logger.warning("Bulk email export skipped mailbox folder=%s account=%s count=%s: %s", folder, account_id, len(rows), exc) + skipped += remaining_rows + logger.warning("Bulk email export skipped mailbox folder=%s account=%s count=%s: %s", folder, account_id, remaining_rows, exc) if exported <= 0: raise HTTPException(status_code=404, detail="No selected emails could be exported") diff --git a/static/js/emailLibrary/index.js b/static/js/emailLibrary/index.js index 6dd013bf5..543fdfd55 100644 --- a/static/js/emailLibrary/index.js +++ b/static/js/emailLibrary/index.js @@ -1143,19 +1143,6 @@ async function _printSelectedEmails(messages) { }; await Promise.all(Array.from({ length: Math.min(4, messages.length) }, readWorker)); - const addField = (doc, parent, label, value) => { - if (!value) return; - const row = doc.createElement('div'); - row.className = 'email-export-pdf-field'; - const labelEl = doc.createElement('span'); - labelEl.className = 'email-export-pdf-label'; - labelEl.textContent = label; - const valueEl = doc.createElement('span'); - valueEl.textContent = value; - row.append(labelEl, valueEl); - parent.appendChild(row); - }; - // Build sanitized HTML for the print frame -- scripts and event handlers // cannot execute inside the sandboxed iframe. const articles = []; diff --git a/tests/test_email_library_bulk_actions.py b/tests/test_email_library_bulk_actions.py index 99e77313e..6840b5916 100644 --- a/tests/test_email_library_bulk_actions.py +++ b/tests/test_email_library_bulk_actions.py @@ -134,12 +134,26 @@ def test_email_eml_export_handles_partial_batch_failures(): # The skipped counter increments for UIDs not found in the batch response. assert "skipped += len(batch) - len(found)" in backend - # Whole-connection failures count all rows as skipped. - assert "skipped += len(rows)" in backend + # Whole-connection failures count remaining unprocessed rows as skipped. + assert "skipped += remaining_rows" in backend assert '"X-Exported-Emails"' in backend assert '"X-Skipped-Emails"' in backend +def test_email_eml_export_enforces_account_ownership(): + """The /messages-download-bulk endpoint must assert account ownership.""" + backend = _EMAIL_ROUTES.read_text(encoding="utf-8") + + assert "_assert_owns_account(account_id, owner)" in backend + + +def test_email_eml_export_deduplicates_selection(): + """Duplicate items in selection must be skipped to avoid duplicate files.""" + backend = _EMAIL_ROUTES.read_text(encoding="utf-8") + + assert "seen_items" in backend + + def test_fixture_tag_filter_supports_receipt_tag_flow(): backend = _EMAIL_ROUTES.read_text(encoding="utf-8") @@ -247,3 +261,153 @@ def test_email_compose_can_attach_gallery_images(): assert "GalleryImage" in backend assert "_gallery_image_path(img.filename)" in backend assert "_stage_compose_file" in backend + + +class _MockImapConn: + def __init__(self, messages_by_uid: dict[str, bytes] | None = None, fetch_exc: Exception | None = None): + self.messages = messages_by_uid or {} + self.fetch_exc = fetch_exc + + def select(self, folder, readonly=True): + return "OK", [b"1"] + + def uid(self, command, *args): + if command == "FETCH": + if self.fetch_exc is not None: + raise self.fetch_exc + uid_set_b = args[0] + uid_set = uid_set_b.decode() if isinstance(uid_set_b, bytes) else str(uid_set_b) + uids = [u.strip() for u in uid_set.split(",") if u.strip()] + res = [] + for u in uids: + if u in self.messages: + meta = f"1 (UID {u} RFC822 {{{len(self.messages[u])}}}".encode() + res.append((meta, self.messages[u])) + res.append(b")") + return "OK", res + return "NO", [] + + +def _create_test_app(monkeypatch, mailbox: _MockImapConn | None = None, assert_owns_fn=None): + from contextlib import contextmanager + from fastapi import FastAPI + from fastapi.testclient import TestClient + from routes.email import email_routes as routes + + conn = mailbox or _MockImapConn() + + @contextmanager + def mock_imap(account_id=None, owner=""): + yield conn + + monkeypatch.setattr(routes, "_start_poller", lambda: None) + monkeypatch.setattr(routes, "_POOL_HOOKS", {}) + monkeypatch.setattr(routes, "_imap", mock_imap) + if assert_owns_fn: + monkeypatch.setattr(routes, "_assert_owns_account", assert_owns_fn) + + app = FastAPI() + app.include_router(routes.setup_email_routes()) + app.dependency_overrides[routes.require_owner] = lambda: "test-user" + return TestClient(app) + + +def test_messages_download_bulk_validates_empty_and_oversized_payload(monkeypatch): + client = _create_test_app(monkeypatch) + + res_empty = client.post("/api/email/messages-download-bulk", json={"messages": []}) + assert res_empty.status_code == 400 + assert "No emails selected" in res_empty.text + + oversized = [{"uid": str(i)} for i in range(251)] + res_over = client.post("/api/email/messages-download-bulk", json={"messages": oversized}) + assert res_over.status_code == 400 + assert "Too many emails selected" in res_over.text + + +def test_messages_download_bulk_single_message_eml_export(monkeypatch): + raw_rfc822 = b"From: sender@example.com\r\nTo: test-user\r\nSubject: Report\r\n\r\nReport content" + mailbox = _MockImapConn({"101": raw_rfc822}) + client = _create_test_app(monkeypatch, mailbox=mailbox) + + res = client.post("/api/email/messages-download-bulk", json={ + "messages": [{"uid": "101", "subject": "Quarterly Report", "date": "2026-10-20"}] + }) + + assert res.status_code == 200 + assert res.headers["content-type"].startswith("message/rfc822") + assert res.headers["x-exported-emails"] == "1" + assert res.headers["x-skipped-emails"] == "0" + assert "email.eml" in res.headers["content-disposition"] + assert res.content == raw_rfc822 + + +def test_messages_download_bulk_multi_message_zip_export(monkeypatch): + import io + import zipfile + + msg1 = b"From: a@example.com\r\nSubject: First\r\n\r\nOne" + msg2 = b"From: b@example.com\r\nSubject: Second\r\n\r\nTwo" + mailbox = _MockImapConn({"101": msg1, "102": msg2}) + client = _create_test_app(monkeypatch, mailbox=mailbox) + + res = client.post("/api/email/messages-download-bulk", json={ + "messages": [ + {"uid": "101", "subject": "First Msg", "date": "2026-10-18"}, + {"uid": "102", "subject": "Second Msg", "date": "2026-10-19"}, + ] + }) + + assert res.status_code == 200 + assert res.headers["content-type"] == "application/zip" + assert res.headers["x-exported-emails"] == "2" + assert res.headers["x-skipped-emails"] == "0" + + zf = zipfile.ZipFile(io.BytesIO(res.content)) + namelist = zf.namelist() + assert len(namelist) == 2 + assert any("101.eml" in name for name in namelist) + assert any("102.eml" in name for name in namelist) + + +def test_messages_download_bulk_partial_failure_and_duplicate_dedup(monkeypatch): + import io + import zipfile + + msg1 = b"From: a@example.com\r\nSubject: Exists\r\n\r\nExists body" + # UID 101 exists, UID 999 does not exist in mailbox + mailbox = _MockImapConn({"101": msg1}) + client = _create_test_app(monkeypatch, mailbox=mailbox) + + res = client.post("/api/email/messages-download-bulk", json={ + "messages": [ + {"uid": "101", "subject": "Exists"}, + {"uid": "999", "subject": "Missing"}, + {"uid": "101", "subject": "Duplicate"}, # duplicate must be skipped + ] + }) + + assert res.status_code == 200 + assert res.headers["content-type"] == "application/zip" + assert res.headers["x-exported-emails"] == "1" + assert res.headers["x-skipped-emails"] == "2" + + zf = zipfile.ZipFile(io.BytesIO(res.content)) + assert len(zf.namelist()) == 1 + + +def test_messages_download_bulk_enforces_account_ownership_guard(monkeypatch): + from fastapi import HTTPException + + def mock_assert_owns(account_id, owner): + if account_id == "other-tenant-acct": + raise HTTPException(status_code=404, detail="Account not found") + + client = _create_test_app(monkeypatch, assert_owns_fn=mock_assert_owns) + + res = client.post("/api/email/messages-download-bulk", json={ + "messages": [{"uid": "101", "account_id": "other-tenant-acct"}] + }) + + assert res.status_code == 404 + assert "Account not found" in res.text