mirror of
https://github.com/pewdiepie-archdaemon/odysseus.git
synced 2026-10-20 22:02:20 +02:00
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
This commit is contained in:
@@ -4281,6 +4281,7 @@ def setup_email_routes():
|
|||||||
logger.debug("Skipping malformed fixture email attachment uid=%s", uid, exc_info=True)
|
logger.debug("Skipping malformed fixture email attachment uid=%s", uid, exc_info=True)
|
||||||
return msg.as_bytes(policy=email_policy.SMTP)
|
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]]] = {}
|
remote_groups: dict[tuple[str | None, str], dict[str, tuple[str, dict]]] = {}
|
||||||
with zipfile.ZipFile(zip_buf, "w", compression=zipfile.ZIP_DEFLATED) as zf:
|
with zipfile.ZipFile(zip_buf, "w", compression=zipfile.ZIP_DEFLATED) as zf:
|
||||||
for row in messages:
|
for row in messages:
|
||||||
@@ -4293,6 +4294,15 @@ def setup_email_routes():
|
|||||||
continue
|
continue
|
||||||
folder = str(row.get("folder") or "INBOX").strip() or "INBOX"
|
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
|
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)
|
fixture = _fixture_email_read(uid, folder, owner)
|
||||||
if fixture is not None and not fixture.get("error"):
|
if fixture is not None and not fixture.get("error"):
|
||||||
filename = _email_filename(row, uid)
|
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.
|
# Fetch original RFC822 data in batches and store it byte-for-byte.
|
||||||
for (account_id, folder), rows_by_uid in remote_groups.items():
|
for (account_id, folder), rows_by_uid in remote_groups.items():
|
||||||
rows = list(rows_by_uid.items())
|
rows = list(rows_by_uid.items())
|
||||||
|
remaining_rows = len(rows)
|
||||||
try:
|
try:
|
||||||
with _imap(account_id, owner=owner) as conn:
|
with _imap(account_id, owner=owner) as conn:
|
||||||
conn.select(_q(folder), readonly=True)
|
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)")
|
status, msg_data = _imap_uid_fetch(conn, uid_set, "(UID RFC822)")
|
||||||
if status != "OK" or not msg_data:
|
if status != "OK" or not msg_data:
|
||||||
skipped += len(batch)
|
skipped += len(batch)
|
||||||
|
remaining_rows -= len(batch)
|
||||||
continue
|
continue
|
||||||
found: set[str] = set()
|
found: set[str] = set()
|
||||||
for meta, raw_message in _group_uid_fetch_records(msg_data):
|
for meta, raw_message in _group_uid_fetch_records(msg_data):
|
||||||
@@ -4351,9 +4363,12 @@ def setup_email_routes():
|
|||||||
found.add(uid)
|
found.add(uid)
|
||||||
exported += 1
|
exported += 1
|
||||||
skipped += len(batch) - len(found)
|
skipped += len(batch) - len(found)
|
||||||
|
remaining_rows -= len(batch)
|
||||||
|
except HTTPException:
|
||||||
|
raise
|
||||||
except Exception as exc:
|
except Exception as exc:
|
||||||
skipped += len(rows)
|
skipped += remaining_rows
|
||||||
logger.warning("Bulk email export skipped mailbox folder=%s account=%s count=%s: %s", folder, account_id, len(rows), exc)
|
logger.warning("Bulk email export skipped mailbox folder=%s account=%s count=%s: %s", folder, account_id, remaining_rows, exc)
|
||||||
|
|
||||||
if exported <= 0:
|
if exported <= 0:
|
||||||
raise HTTPException(status_code=404, detail="No selected emails could be exported")
|
raise HTTPException(status_code=404, detail="No selected emails could be exported")
|
||||||
|
|||||||
@@ -1143,19 +1143,6 @@ async function _printSelectedEmails(messages) {
|
|||||||
};
|
};
|
||||||
await Promise.all(Array.from({ length: Math.min(4, messages.length) }, readWorker));
|
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
|
// Build sanitized HTML for the print frame -- scripts and event handlers
|
||||||
// cannot execute inside the sandboxed iframe.
|
// cannot execute inside the sandboxed iframe.
|
||||||
const articles = [];
|
const articles = [];
|
||||||
|
|||||||
@@ -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.
|
# The skipped counter increments for UIDs not found in the batch response.
|
||||||
assert "skipped += len(batch) - len(found)" in backend
|
assert "skipped += len(batch) - len(found)" in backend
|
||||||
# Whole-connection failures count all rows as skipped.
|
# Whole-connection failures count remaining unprocessed rows as skipped.
|
||||||
assert "skipped += len(rows)" in backend
|
assert "skipped += remaining_rows" in backend
|
||||||
assert '"X-Exported-Emails"' in backend
|
assert '"X-Exported-Emails"' in backend
|
||||||
assert '"X-Skipped-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():
|
def test_fixture_tag_filter_supports_receipt_tag_flow():
|
||||||
backend = _EMAIL_ROUTES.read_text(encoding="utf-8")
|
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 "GalleryImage" in backend
|
||||||
assert "_gallery_image_path(img.filename)" in backend
|
assert "_gallery_image_path(img.filename)" in backend
|
||||||
assert "_stage_compose_file" 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
|
||||||
|
|||||||
Reference in New Issue
Block a user