From addb2232a1b08efc31891f8988f909de4d902b3c Mon Sep 17 00:00:00 2001 From: Lukas Remis Date: Wed, 14 Oct 2026 22:08:06 +0200 Subject: [PATCH] Security hardening from the assessment; testing fixes Security (ultimo-security-report.md): - Security headers on every response, the app shell included: they lived at server level and nginx dropped them in any location with its own header. CSP, framing, nosniff, referrer, permissions (docker/nginx-security.conf). - Plain HTTP forwarded by the reverse proxy is redirected to HTTPS; HSTS on HTTPS responses. server_tokens off. - Cross-site state-changing /api requests refused (login CSRF), WebSocket too. - Validation errors in the usual error shape, without the request model. - New-password policy: max 128, common/own-address/repeated refused, breached-password check (HIBP k-anonymity, fails open). - Message HTML: links keep only http(s)/mailto (sanitizer v6); the HTML endpoint carries its own no-script CSP. - Outbound fetches pinned to the checked IP (DNS rebinding); external IMAP/SMTP servers re-checked before each connection; CGNAT and mapped addresses counted as non-public. Fixes from testing: - IDLE loop catches up before idling: mail that came while the worker was away no longer waits up to 10 minutes. - Restore from Spam/Trash/Archive moves only the removed messages, not your reply in Sent. - Narrow desktop list stacks sender/subject/snippet (subject was squeezed to nothing at the default width), with hover actions. - Phone: wide emails scale to fit the screen. - Singular forms ("1 message"); no zip button for a single attachment; app passwords no longer claim "never used" for mail apps. - ruff format across the backend. Co-Authored-By: Claude Opus 5.5 --- backend/app/accounts/discovery.py | 4 +- backend/app/accounts/ssrf.py | 77 +++-- backend/app/accounts/testing.py | 1 + backend/app/api/actions.py | 8 +- backend/app/api/errors.py | 17 ++ backend/app/api/messages.py | 25 +- backend/app/config.py | 3 + backend/app/identity/password_policy.py | 145 +++++++++ backend/app/identity/routes.py | 10 + backend/app/identity/schemas.py | 4 +- backend/app/indexer/sanitize.py | 8 +- backend/app/main.py | 4 + backend/app/models.py | 4 +- backend/app/plugins/http.py | 4 +- backend/app/providers/imap_smtp/provider.py | 18 ++ backend/app/security/cross_site.py | 91 ++++++ backend/app/sync/actions.py | 6 +- backend/app/sync/engine.py | 65 ++-- backend/app/sync/folder_names.py | 140 ++++++--- backend/app/sync/provider_factory.py | 1 + backend/app/ws.py | 7 + backend/scripts/backfill_delivery_id.py | 4 +- backend/scripts/reindex_address_parts.py | 4 +- backend/tests/accounts/test_discovery.py | 8 +- backend/tests/api/test_api.py | 38 ++- backend/tests/api/test_saved_searches.py | 4 +- backend/tests/conftest.py | 2 + backend/tests/mail/test_actions.py | 33 ++ .../tests/plugins/test_calendar_reminders.py | 3 +- backend/tests/plugins/test_filters_plugin.py | 51 ++-- backend/tests/plugins/test_hello_plugin.py | 4 +- backend/tests/plugins/test_migrate.py | 7 +- backend/tests/plugins/test_sdk_messages.py | 18 +- backend/tests/plugins/test_ui_manifest_api.py | 4 +- backend/tests/providers/test_parsing.py | 10 +- backend/tests/search/test_meili.py | 4 +- backend/tests/search/test_search_api.py | 8 +- backend/tests/security/test_hardening.py | 285 ++++++++++++++++++ backend/tests/sync/test_secondary_poll.py | 65 ++++ docker-compose.yml | 2 + docker/Dockerfile.frontend | 1 + docker/nginx-app.conf | 15 +- docker/nginx-security.conf | 31 ++ docker/nginx.conf | 17 ++ docs/nas-install.md | 18 ++ frontend/src/components/list/MessageList.tsx | 68 ++++- .../components/thread/AttachmentPreview.tsx | 3 +- .../src/components/thread/Attachments.tsx | 3 + .../src/components/thread/MessageBody.tsx | 43 ++- frontend/src/i18n/en.ts | 10 + frontend/src/i18n/index.tsx | 10 +- frontend/src/routes/settings/Settings.tsx | 10 +- frontend/src/test/i18n.test.tsx | 24 ++ frontend/vite.config.ts | 3 + 54 files changed, 1265 insertions(+), 187 deletions(-) create mode 100644 backend/app/identity/password_policy.py create mode 100644 backend/app/security/cross_site.py create mode 100644 backend/tests/security/test_hardening.py create mode 100644 docker/nginx-security.conf create mode 100644 frontend/src/test/i18n.test.tsx diff --git a/backend/app/accounts/discovery.py b/backend/app/accounts/discovery.py index 8c5ef16..1c02a91 100644 --- a/backend/app/accounts/discovery.py +++ b/backend/app/accounts/discovery.py @@ -125,9 +125,7 @@ def parse_autoconfig_xml(xml_text: str) -> CandidateConfig | None: smtp = _server("outgoingServer", "smtp") if imap is None or smtp is None: return None - return CandidateConfig( - imap, smtp, "autoconfig", auth_strategy=_strategy_from_auth(imap.auth) - ) + return CandidateConfig(imap, smtp, "autoconfig", auth_strategy=_strategy_from_auth(imap.auth)) # --- DNS SRV (RFC 6186) ------------------------------------------------------ diff --git a/backend/app/accounts/ssrf.py b/backend/app/accounts/ssrf.py index be0ef05..045d9f7 100644 --- a/backend/app/accounts/ssrf.py +++ b/backend/app/accounts/ssrf.py @@ -2,9 +2,10 @@ All autodiscovery HTTP is HTTPS-only, 5 s timeout, response capped at 256 KB, no off-domain redirects, and the target host must resolve exclusively to public IPs -(reject private/loopback/link-local/reserved ranges). A residual TOCTOU gap -(DNS rebinding between our resolve and httpx's own resolve) is accepted for M1; -a pinned-IP transport can close it later. +(reject private/loopback/link-local/reserved ranges). The request then goes to +the very address that was checked — the name is not looked up a second time — +so a DNS answer that changes in between (DNS rebinding) cannot point it inward. +TLS is still verified against the host name. """ from __future__ import annotations @@ -26,15 +27,12 @@ class SsrfError(Exception): def _is_public_ip(ip: str) -> bool: - addr = ipaddress.ip_address(ip) - return not ( - addr.is_private - or addr.is_loopback - or addr.is_link_local - or addr.is_multicast - or addr.is_reserved - or addr.is_unspecified - ) + addr = ipaddress.ip_address(ip.split("%", 1)[0]) # drop an IPv6 zone index + if isinstance(addr, ipaddress.IPv6Address) and addr.ipv4_mapped is not None: + addr = addr.ipv4_mapped # ::ffff:127.0.0.1 is 127.0.0.1 + # is_global: not private, loopback, link-local, reserved, unspecified, nor + # shared/carrier-grade NAT (100.64.0.0/10, which is_private misses). + return addr.is_global and not addr.is_multicast async def _resolve_all(host: str) -> list[str]: @@ -43,8 +41,10 @@ async def _resolve_all(host: str) -> list[str]: return [info[4][0] for info in infos] -async def assert_host_is_public(host: str) -> None: - """Resolve ``host`` and reject if any address is non-public (SPEC §10.6).""" +async def assert_host_is_public(host: str) -> list[str]: + """Resolve ``host`` and reject if any address is non-public (SPEC §10.6). + + Returns the addresses checked, for the caller to connect to.""" try: addresses = await _resolve_all(host) except socket.gaierror as exc: @@ -54,6 +54,31 @@ async def assert_host_is_public(host: str) -> None: for ip in addresses: if not _is_public_ip(ip): raise SsrfError(f"{host} resolves to non-public address {ip}") + return addresses + + +async def _pinned(url: str) -> tuple[str, dict[str, str], dict[str, str]]: + """``url`` aimed at the address just checked: (url, headers, extensions). + + The URL names the IP, the Host header keeps the name (virtual hosting), and + TLS gets the name for SNI and for checking the certificate.""" + parts = urlsplit(url) + if parts.scheme != "https": + raise SsrfError("only https is permitted") + hostname = parts.hostname + if not hostname: + raise SsrfError("missing host") + addresses = await assert_host_is_public(hostname) + # IPv4 first: containers often have no IPv6 route, though DNS returns AAAA. + ip = next((a for a in addresses if ":" not in a), addresses[0]) + literal = f"[{ip}]" if ":" in ip else ip + netloc = f"{literal}:{parts.port}" if parts.port else literal + host_header = f"{hostname}:{parts.port}" if parts.port else hostname + return ( + parts._replace(netloc=netloc).geturl(), + {"Host": host_header}, + {"sni_hostname": hostname}, + ) async def guarded_get(url: str) -> str | None: @@ -63,12 +88,7 @@ async def guarded_get(url: str) -> str | None: (any non-200 response). Raises :class:`SsrfError` on a policy violation (bad scheme, private IP, oversize); lets httpx network errors propagate. """ - parts = urlsplit(url) - if parts.scheme != "https": - raise SsrfError("only https is permitted") - if not parts.hostname: - raise SsrfError("missing host") - await assert_host_is_public(parts.hostname) + target, pin_headers, extensions = await _pinned(url) async with ( httpx.AsyncClient( @@ -76,7 +96,7 @@ async def guarded_get(url: str) -> str | None: timeout=HTTP_TIMEOUT, headers={"User-Agent": "webmail-autodiscovery/1.0"}, ) as client, - client.stream("GET", url) as resp, + client.stream("GET", target, headers=pin_headers, extensions=extensions) as resp, ): if resp.status_code != 200: return None @@ -114,16 +134,17 @@ async def guarded_request( fetch ICS feeds. HTTPS-only, public host required, no off-domain redirects, body capped at ``max_bytes``. ``auth`` is an (user, password) basic-auth pair. Returns status + headers + body (all statuses; the caller decides what's an error).""" - parts = urlsplit(url) - if parts.scheme != "https": - raise SsrfError("only https is permitted") - if not parts.hostname: - raise SsrfError("missing host") - await assert_host_is_public(parts.hostname) + target, pin_headers, extensions = await _pinned(url) async with ( httpx.AsyncClient(follow_redirects=False, timeout=timeout, auth=auth) as client, - client.stream(method, url, headers=headers, content=content) as resp, + client.stream( + method, + target, + headers={**(headers or {}), **pin_headers}, + content=content, + extensions=extensions, + ) as resp, ): chunks: list[bytes] = [] total = 0 diff --git a/backend/app/accounts/testing.py b/backend/app/accounts/testing.py index 969cef1..c26b7d9 100644 --- a/backend/app/accounts/testing.py +++ b/backend/app/accounts/testing.py @@ -89,6 +89,7 @@ async def test_account( smtp_tls=smtp.tls, username=username, password=password, + public_only=True, ) ) try: diff --git a/backend/app/api/actions.py b/backend/app/api/actions.py index b71d29a..968cfa0 100644 --- a/backend/app/api/actions.py +++ b/backend/app/api/actions.py @@ -125,9 +125,7 @@ async def mark_all_read( return {"changed": 0} messages = list( await db.scalars( - select(Message).where( - Message.thread_id.in_(thread_ids), Message.user_id == user.id - ) + select(Message).where(Message.thread_id.in_(thread_ids), Message.user_id == user.id) ) ) add, remove = sanitize_flags(["\\Seen"]), sanitize_flags([]) @@ -137,9 +135,7 @@ async def mark_all_read( await _push_and_publish(user.id, changed, add, remove) bus = get_event_bus() for tid in thread_ids: - await bus.publish( - "thread.updated", {"user_id": str(user.id), "thread_id": str(tid)} - ) + await bus.publish("thread.updated", {"user_id": str(user.id), "thread_id": str(tid)}) return {"changed": len(changed)} diff --git a/backend/app/api/errors.py b/backend/app/api/errors.py index 13ae5b1..8319254 100644 --- a/backend/app/api/errors.py +++ b/backend/app/api/errors.py @@ -5,6 +5,7 @@ from __future__ import annotations from typing import NoReturn from fastapi import FastAPI, HTTPException, Request +from fastapi.exceptions import RequestValidationError from fastapi.responses import JSONResponse from starlette.exceptions import HTTPException as StarletteHTTPException @@ -29,7 +30,23 @@ async def _handler(request: Request, exc: StarletteHTTPException) -> JSONRespons return JSONResponse(status_code=exc.status_code, content=body, headers=headers) +async def _validation_handler(request: Request, exc: RequestValidationError) -> JSONResponse: + """A malformed request, in the usual error shape. Names the first field at + fault and what is wrong with it — enough to show the user — but not the + whole request model the framework's own answer would spell out.""" + errors = exc.errors() + first = errors[0] if errors else {} + loc = [str(p) for p in first.get("loc", ()) if p not in ("body", "query", "path")] + field = loc[-1] if loc else "" + reason = str(first.get("msg", "invalid value")) + message = f"{field}: {reason}" if field else reason + return JSONResponse( + status_code=422, content={"error": {"code": "validation_error", "message": message}} + ) + + def register_error_handlers(app: FastAPI) -> None: """Install the error-shape handler for all HTTP exceptions.""" app.add_exception_handler(StarletteHTTPException, _handler) # type: ignore[arg-type] app.add_exception_handler(HTTPException, _handler) # type: ignore[arg-type] + app.add_exception_handler(RequestValidationError, _validation_handler) # type: ignore[arg-type] diff --git a/backend/app/api/messages.py b/backend/app/api/messages.py index a8c863b..fd469f3 100644 --- a/backend/app/api/messages.py +++ b/backend/app/api/messages.py @@ -88,6 +88,23 @@ def _wrap(fragment: str) -> str: return _DOC_TEMPLATE.format(body=fragment) +# The sanitized message as a document of its own. The app shows it in a +# sandboxed frame (frontend MessageBody), but this URL can also be opened +# directly in a tab, on our own origin: there, this policy is what keeps any +# script the sanitizer might ever miss from running — no scripts, no plugins, +# no forms, a sandbox of its own origin, and nothing framing it elsewhere. +_MESSAGE_CSP = ( + "default-src 'none'; script-src 'none'; object-src 'none'; base-uri 'none'; " + "form-action 'none'; frame-ancestors 'self'; img-src 'self' data: https: http:; " + "style-src 'self' 'unsafe-inline'; font-src 'self' data: https:; media-src 'self'; " + "sandbox allow-popups allow-popups-to-escape-sandbox" +) + + +def _html(document: str) -> HTMLResponse: + return HTMLResponse(content=document, headers={"Content-Security-Policy": _MESSAGE_CSP}) + + async def _owned_message(db: AsyncSession, user_id: uuid.UUID, message_id: uuid.UUID) -> Message: msg = await db.scalar( select(Message).where(Message.id == message_id, Message.user_id == user_id) @@ -123,7 +140,7 @@ async def message_html( stale = html_part is not None and html_part.sanitizer_v < SANITIZER_VERSION if html_part is not None and not allow_remote and not stale: - return HTMLResponse(content=_wrap(html_part.content)) + return _html(_wrap(html_part.content)) # Re-sanitize the original HTML (image-approved sender, or a stale stored copy). # If the raw blob is unavailable, fall back to whatever is already stored. @@ -142,13 +159,13 @@ async def message_html( html_part.content = result.html # persist the upgraded sanitization html_part.sanitizer_v = result.sanitizer_v await db.commit() - return HTMLResponse(content=_wrap(result.html)) + return _html(_wrap(result.html)) if html_part is not None: - return HTMLResponse(content=_wrap(html_part.content)) + return _html(_wrap(html_part.content)) text_part = next((p for p in parts if p.kind == "text"), None) body = html_lib.escape(text_part.content) if text_part else "" - return HTMLResponse(content=_wrap(f'
{body}
')) + return _html(_wrap(f'
{body}
')) @router.post("/{message_id}/allow-remote-images", dependencies=[Depends(require_csrf)]) diff --git a/backend/app/config.py b/backend/app/config.py index 4105a0c..00fd888 100644 --- a/backend/app/config.py +++ b/backend/app/config.py @@ -71,6 +71,9 @@ class Settings(BaseSettings): # Spelling and grammar checking in the compose window (LanguageTool server, # e.g. http://languagetool:8010). Empty: the check is off for everyone. languagetool_url: str = Field(default="", alias="LANGUAGETOOL_URL") + # New passwords are checked against known data breaches (Have I Been Pwned's + # k-anonymity range API: only 5 hex digits of a SHA-1 leave the server). + password_breach_check: bool = Field(default=True, alias="PASSWORD_BREACH_CHECK") # Auto-purge Trash/Spam messages older than this many days (0 disables). trash_retention_days: int = Field(default=30, alias="TRASH_RETENTION_DAYS") diff --git a/backend/app/identity/password_policy.py b/backend/app/identity/password_policy.py new file mode 100644 index 0000000..7af2ea5 --- /dev/null +++ b/backend/app/identity/password_policy.py @@ -0,0 +1,145 @@ +"""What a new account password must not be (sign-up and reset). + +Length is bounded by the request schemas (8–128). On top of that a password is +refused when it is one of the passwords every guessing attack tries first, is +made of the user's own address, is one character repeated, or has turned up in +a known data breach. The breach check asks Have I Been Pwned's range API with +the first five hex digits of the password's SHA-1 only (k-anonymity: neither the +password nor its hash leaves the server) and is skipped, not failed, when that +service cannot be reached — a sign-up never hangs on someone else's outage. +Set ``PASSWORD_BREACH_CHECK=false`` to never ask it. +""" + +from __future__ import annotations + +import hashlib +import logging + +import httpx + +from app.config import get_settings + +logger = logging.getLogger(__name__) + +MIN_LENGTH = 8 +MAX_LENGTH = 128 + +_RANGE_API = "https://api.pwnedpasswords.com/range/" +_TIMEOUT = 3.0 + +# The front of every guessing list (and of every breach), lower-cased. +_COMMON = frozenset( + { + "password", + "password1", + "password12", + "password123", + "password1234", + "passw0rd", + "p@ssw0rd", + "p@ssword", + "12345678", + "123456789", + "1234567890", + "0123456789", + "87654321", + "11111111", + "00000000", + "12341234", + "qwertyui", + "qwerty123", + "qwerty1234", + "qwertyuiop", + "1q2w3e4r", + "1q2w3e4r5t", + "1qaz2wsx", + "zaq12wsx", + "asdfghjk", + "asdfasdf", + "iloveyou", + "iloveyou1", + "sunshine", + "princess", + "football", + "baseball", + "welcome1", + "welcome123", + "letmein1", + "trustno1", + "superman", + "batman123", + "dragon123", + "monkey123", + "abc12345", + "abcd1234", + "aa123456", + "qazwsxedc", + "changeme", + "changeme1", + "administrator", + "admin123", + "adminadmin", + "computer", + "internet", + "whatever", + "starwars", + "michelle", + "jennifer", + "charlie1", + "zaq1zaq1", + "haslo123", + "polska123", + "kochamcie", + "zaq1@wsx", + "qwerty12", + "1234qwer", + "q1w2e3r4", + } +) + + +class PasswordRejected(Exception): + """The password may not be used; ``code`` and ``message`` say why.""" + + def __init__(self, code: str, message: str) -> None: + super().__init__(message) + self.code = code + self.message = message + + +async def _breached(password: str) -> bool: + digest = hashlib.sha1(password.encode("utf-8")).hexdigest().upper() # noqa: S324 - HIBP's format + prefix, suffix = digest[:5], digest[5:] + try: + async with httpx.AsyncClient(timeout=_TIMEOUT) as client: + resp = await client.get(_RANGE_API + prefix, headers={"Add-Padding": "true"}) + resp.raise_for_status() + except httpx.HTTPError as exc: + logger.warning("breached-password check skipped: %s", exc) + return False + for line in resp.text.splitlines(): + candidate, _, count = line.partition(":") + if candidate.strip() == suffix and count.strip() not in ("", "0"): # padding rows say 0 + return True + return False + + +async def check_new_password(password: str, handle: str) -> None: + """Raise :class:`PasswordRejected` if ``password`` may not be used by ``handle``.""" + lowered = password.lower() + if len(set(password)) <= 2: + raise PasswordRejected("weak_password", "this password is too easy to guess") + if lowered in _COMMON: + raise PasswordRejected("weak_password", "this is one of the most common passwords") + localpart = handle.split("@", 1)[0].lower() + if len(localpart) >= 3 and localpart in lowered: + # The name with a year or a few characters added ("jsmith2026") is the + # first thing anyone who knows the address will try. + rest = lowered.replace(localpart, "", 1) + if len(rest) < 4 or all(not c.isalpha() for c in rest): + raise PasswordRejected("weak_password", "the password must not be your address") + if get_settings().password_breach_check and await _breached(password): + raise PasswordRejected( + "breached_password", + "this password has appeared in a data breach — please choose another", + ) diff --git a/backend/app/identity/routes.py b/backend/app/identity/routes.py index 792f6b8..def3c71 100644 --- a/backend/app/identity/routes.py +++ b/backend/app/identity/routes.py @@ -27,6 +27,7 @@ from app.identity.cookies import ( ) from app.identity.deps import CurrentSession, CurrentUser, require_csrf from app.identity.mailer import MailerConfig, MailerError, mailer_config, reset_mail, send_mail +from app.identity.password_policy import PasswordRejected, check_new_password from app.identity.passwords import hash_password, verify_password from app.identity.reset import ( consume_token, @@ -125,6 +126,10 @@ _REGISTRATION_ERROR_STATUS = {"handle_taken": 409, "registration_closed": 403} async def register( body: RegisterRequest, request: Request, response: Response, db: DbSession ) -> UserOut: + try: + await check_new_password(body.password, resolve_handle(body.localpart)) + except PasswordRejected as exc: + raise_api_error(400, exc.code, exc.message) try: user = await register_user( db, @@ -312,6 +317,11 @@ async def password_reset(body: PasswordResetIn, db: DbSession) -> MessageOut: user = await db.get(User, user_id) if user_id else None if user is None: raise_api_error(400, "invalid_token", "this link is invalid or has expired") + # Before the 2FA code and the link are spent: a refused password costs neither. + try: + await check_new_password(body.password, user.handle) + except PasswordRejected as exc: + raise_api_error(400, exc.code, exc.message) if user.totp_secret is not None: if not body.totp: raise_api_error(401, "totp_required", "a 2FA code is required") diff --git a/backend/app/identity/schemas.py b/backend/app/identity/schemas.py index 9fc9763..290e79a 100644 --- a/backend/app/identity/schemas.py +++ b/backend/app/identity/schemas.py @@ -10,7 +10,7 @@ from pydantic import BaseModel, Field class RegisterRequest(BaseModel): localpart: str = Field(min_length=3, max_length=30) - password: str = Field(min_length=8, max_length=1024) + password: str = Field(min_length=8, max_length=128) recovery_email: str | None = None @@ -49,7 +49,7 @@ class PasswordResetRequestIn(BaseModel): class PasswordResetIn(BaseModel): token: str = Field(min_length=1) - password: str = Field(min_length=8, max_length=1024) + password: str = Field(min_length=8, max_length=128) totp: str | None = None # required when the account has 2FA enabled diff --git a/backend/app/indexer/sanitize.py b/backend/app/indexer/sanitize.py index e7f7aff..bde58c0 100644 --- a/backend/app/indexer/sanitize.py +++ b/backend/app/indexer/sanitize.py @@ -18,7 +18,8 @@ import nh3 # Bump when the sanitization policy changes; body_parts.sanitizer_v records it so a # render re-sanitizes stale content from the raw blob (SPEC §10.4). # v5: this server's own inline-image URLs quoted in a message load from here. -SANITIZER_VERSION = 5 +# v6: links keep only http(s) and mailto (no data: pages). +SANITIZER_VERSION = 6 # This server's inline-image URL, however it ended up written: "/api/…", # "https://host/api/…", or mangled to "http:///api/…" by an earlier copy. @@ -202,6 +203,11 @@ def sanitize_html( return _sanitize_style(value, allow_remote_images) if tag == "a" and attr == "target": return "_blank" + if attr == "href": + # A link goes to the web or to a mail address: data: (a page made + # up by the sender) and cid: are kept for inline images only. + scheme = value.strip().lower().split(":", 1)[0] if ":" in value else "" + return value if scheme in ("", "http", "https", "mailto") else None if tag == "img" and attr == "src": lowered = value.strip().lower() if lowered.startswith("cid:"): diff --git a/backend/app/main.py b/backend/app/main.py index 1af8c5a..f4ad0c8 100644 --- a/backend/app/main.py +++ b/backend/app/main.py @@ -37,6 +37,10 @@ def create_app() -> FastAPI: register_error_handlers(app) + from app.security.cross_site import CrossSiteGuard + + app.add_middleware(CrossSiteGuard) + @app.get("/api/healthz") async def healthz() -> dict[str, str]: """Liveness/readiness probe: checks DB and Redis (dev-setup §6).""" diff --git a/backend/app/models.py b/backend/app/models.py index d833492..18cbebf 100644 --- a/backend/app/models.py +++ b/backend/app/models.py @@ -168,9 +168,7 @@ class Mailbox(Base): # IMAP ids are unsigned 32-bit (MODSEQ 63-bit): bigint, not integer. uidvalidity: Mapped[int] = mapped_column(BigInteger, nullable=False, server_default=text("0")) highestmodseq: Mapped[int | None] = mapped_column(BigInteger) - last_seen_uid: Mapped[int] = mapped_column( - BigInteger, nullable=False, server_default=text("0") - ) + last_seen_uid: Mapped[int] = mapped_column(BigInteger, nullable=False, server_default=text("0")) class Thread(Base): diff --git a/backend/app/plugins/http.py b/backend/app/plugins/http.py index 2b190f1..edffd85 100644 --- a/backend/app/plugins/http.py +++ b/backend/app/plugins/http.py @@ -21,9 +21,7 @@ class PluginHttp: content: bytes | str | None = None, auth: tuple[str, str] | None = None, ) -> GuardedResponse: - return await guarded_request( - method, url, headers=headers, content=content, auth=auth - ) + return await guarded_request(method, url, headers=headers, content=content, auth=auth) async def get(self, url: str, **kw: object) -> GuardedResponse: return await self.request("GET", url, **kw) # type: ignore[arg-type] diff --git a/backend/app/providers/imap_smtp/provider.py b/backend/app/providers/imap_smtp/provider.py index 30a6ba1..f83d42d 100644 --- a/backend/app/providers/imap_smtp/provider.py +++ b/backend/app/providers/imap_smtp/provider.py @@ -122,6 +122,16 @@ def _date_criteria(since: date | None, before: date | None) -> str: return " ".join(parts) if parts else "ALL" +async def _require_public(host: str) -> None: + """Refuse a user-named server that now resolves to a non-public address.""" + from app.accounts.ssrf import SsrfError, assert_host_is_public + + try: + await assert_host_is_public(host) + except SsrfError as exc: + raise ProviderConnectError(f"refusing to connect to {host}: {exc}") from exc + + @dataclass(frozen=True) class ProviderConfig: imap_host: str @@ -138,6 +148,10 @@ class ProviderConfig: # The hosted mail server: every user's mailbox is on that one host, so the # per-host connection cap (which spares outside providers) does not apply. internal: bool = False + # A server the user named (every external account): its name is checked + # again before each connection, so DNS changed after the account was added + # cannot turn the worker on the internal network (DNS rebinding). + public_only: bool = False class ImapSmtpProvider(MailProvider): @@ -196,6 +210,8 @@ class ImapSmtpProvider(MailProvider): if self._imap is not None: return self._imap cfg = self._cfg + if cfg.public_only: + await _require_public(cfg.imap_host) await self._acquire_conn_slots() try: try: @@ -540,6 +556,8 @@ class ImapSmtpProvider(MailProvider): async def send(self, raw: bytes, envelope_from: str, envelope_to: list[str]) -> None: cfg = self._cfg + if cfg.public_only: + await _require_public(cfg.smtp_host) use_tls = cfg.smtp_tls == "ssl" start_tls = cfg.smtp_tls == "starttls" host_slot = ( diff --git a/backend/app/security/cross_site.py b/backend/app/security/cross_site.py new file mode 100644 index 0000000..3a92182 --- /dev/null +++ b/backend/app/security/cross_site.py @@ -0,0 +1,91 @@ +"""Refuse state-changing API requests that another web site makes a browser send. + +The double-submit CSRF token (app/identity/deps.py ``require_csrf``) guards +everything a signed-in user can change. The requests made before anyone is +signed in — sign-in itself, sign-up, a password reset — have no token yet, so a +hostile page could make a visitor's browser sign in to an account of the +attacker's choosing (login CSRF): whatever the victim then writes, files or +uploads lands in the attacker's account. + +Browsers say where a request comes from: ``Sec-Fetch-Site`` in every current +browser, ``Origin`` on any cross-origin POST in older ones. A request another +site started is refused here, before any route runs. Mail apps, calendar apps +and servers send neither header and are not affected (DAV lives outside /api +anyway). +""" + +from __future__ import annotations + +import json +from urllib.parse import urlsplit + +from starlette.datastructures import Headers +from starlette.types import ASGIApp, Receive, Scope, Send + +from app.config import get_settings + +_SAFE_METHODS = frozenset({"GET", "HEAD", "OPTIONS"}) +_BODY = json.dumps( + {"error": {"code": "cross_site", "message": "request refused: sent from another site"}} +).encode() + + +def _hostname(value: str) -> str: + """``host[:port]`` -> ``host``, lower-cased (IPv6 brackets kept).""" + value = value.strip().lower() + if value.startswith("["): + return value.split("]", 1)[0] + "]" + return value.rsplit(":", 1)[0] if value.count(":") == 1 else value + + +def is_cross_site(headers: Headers) -> bool: + """Whether a browser reports this request as started by another site.""" + fetch_site = headers.get("sec-fetch-site") + if fetch_site is not None: + # "same-origin", "same-site" (our own subdomains), "none" (typed or + # bookmarked): all fine. Only another site is refused. + return fetch_site.strip().lower() == "cross-site" + origin = headers.get("origin") + if origin is None: + return False # not a browser, or a same-origin request from an old one + if origin.strip().lower() == "null": + return True # sandboxed frame, data: URL, file: page + origin_host = (urlsplit(origin.strip()).hostname or "").lower() + # Our own domain, whatever a reverse proxy did to the Host header. + domain = get_settings().app_domain.lower() + if origin_host == domain or origin_host.endswith("." + domain): + return False + # Otherwise the name the browser used: Host, or the one a reverse proxy saw. + ours = {_hostname(headers.get("host", ""))} + forwarded = headers.get("x-forwarded-host") + if forwarded: + ours.add(_hostname(forwarded.split(",")[0])) + return origin_host not in ours + + +class CrossSiteGuard: + """ASGI middleware: 403 on cross-site POST/PUT/PATCH/DELETE under /api/.""" + + def __init__(self, app: ASGIApp) -> None: + self.app = app + + async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None: + if ( + scope["type"] == "http" + and scope["method"] not in _SAFE_METHODS + and scope["path"].startswith("/api/") + and is_cross_site(Headers(scope=scope)) + ): + await send( + { + "type": "http.response.start", + "status": 403, + "headers": [ + (b"content-type", b"application/json"), + (b"content-length", str(len(_BODY)).encode()), + ], + } + ) + await send({"type": "http.response.body", "body": _BODY}) + return + await self.app(scope, receive, send) diff --git a/backend/app/sync/actions.py b/backend/app/sync/actions.py index 91289be..72884d1 100644 --- a/backend/app/sync/actions.py +++ b/backend/app/sync/actions.py @@ -302,7 +302,8 @@ async def restore_thread_job(ctx: dict[str, Any], thread_id: str) -> None: Each message returns to ``restore_mailbox_id`` (the folder it was in before it was archived/trashed) — e.g. a deleted Sent message goes back to Sent. Messages - with no recorded origin fall back to the account's Inbox. + with no recorded origin fall back to the account's Inbox. Only messages in + Archive, Trash or Spam move: the thread's others (your reply in Sent) stay. """ tid = uuid.UUID(thread_id) async with session_scope() as session: @@ -319,10 +320,13 @@ async def restore_thread_job(ctx: dict[str, Any], thread_id: str) -> None: await session.scalars(select(Mailbox).where(Mailbox.account_id.in_(account_ids))) ) path_by_id = {mb.id: mb.path for mb in mailboxes} + removal_ids = {mb.id for mb in mailboxes if mb.special_use in _REMOVAL_FOLDERS} inbox_by_account = {mb.account_id: mb.id for mb in mailboxes if mb.special_use == "inbox"} # (message_id, account_id, source_path, target_id) — resolve the target now. plan: list[tuple[uuid.UUID, uuid.UUID, str, uuid.UUID, int]] = [] for m in messages: + if m.mailbox_id not in removal_ids: + continue # never removed: nothing to restore target_id = m.restore_mailbox_id if target_id is None or target_id not in path_by_id: target_id = inbox_by_account.get(m.account_id) diff --git a/backend/app/sync/engine.py b/backend/app/sync/engine.py index 5bde1b0..7546813 100644 --- a/backend/app/sync/engine.py +++ b/backend/app/sync/engine.py @@ -172,9 +172,7 @@ class _BackfillProgress: self._mailboxes.add(mailbox_path) async with session_scope() as session: await session.execute( - update(Account) - .where(Account.id == self.account.id) - .values(backfill_done=self.done) + update(Account).where(Account.id == self.account.id).values(backfill_done=self.done) ) await get_event_bus().publish( "sync.progress", @@ -321,16 +319,28 @@ async def bootstrap_and_backfill(account_id: uuid.UUID) -> int: dedupe = gmail and mailbox.special_use == "archive" if since is not None: # 3months: single windowed pass count += await backfill_mailbox( - provider, account_obj, mailbox, progress, since=since, + provider, + account_obj, + mailbox, + progress, + since=since, dedupe_account_scope=dedupe, ) else: # all: recent window first (usable fast), then the remainder count += await backfill_mailbox( - provider, account_obj, mailbox, progress, since=recent_cutoff, + provider, + account_obj, + mailbox, + progress, + since=recent_cutoff, dedupe_account_scope=dedupe, ) count += await backfill_mailbox( - provider, account_obj, mailbox, progress, before=recent_cutoff, + provider, + account_obj, + mailbox, + progress, + before=recent_cutoff, dedupe_account_scope=dedupe, ) await _set_state(account_id, "live") @@ -491,9 +501,7 @@ async def apply_delta( await _reconcile_drafts(provider, account, mailbox) -async def _reconcile_drafts( - provider: ImapSmtpProvider, account: Account, mailbox: Mailbox -) -> None: +async def _reconcile_drafts(provider: ImapSmtpProvider, account: Account, mailbox: Mailbox) -> None: """Drop DB rows for drafts that no longer exist on the server (were expunged).""" from app.db import session_scope @@ -548,13 +556,13 @@ async def live_loop( await provider.verify() # signed in: the account is reachable if health is not None: await health.ok() + first = True while not stop.is_set(): - try: - await provider.idle(mailbox_path) - except ProviderNotSupported: - await asyncio.sleep(idle_fallback) - if stop.is_set(): - break + # Catch up before each IDLE, not only after it: IDLE reports what + # arrives from now on, so mail that came while this loop was not + # running (a new account's first minutes, a reconnect, a worker + # restart) would otherwise wait for the next new message or the + # IDLE timeout (10 min) to show. async with session_scope() as session: mailbox = await session.scalar( select(Mailbox).where( @@ -562,14 +570,23 @@ async def live_loop( ) ) mailbox_obj = mailbox - if mailbox_obj is None: + if mailbox_obj is not None: + delta = await provider.poll( + mailbox_path, + last_seen_uid=mailbox_obj.last_seen_uid, + highestmodseq=mailbox_obj.highestmodseq, + ) + await apply_delta(provider, account_obj, mailbox_obj, delta) + elif not first: + break # the folder is gone + # else: the account's first sync has not recorded the folder yet. + first = False + if stop.is_set(): break - delta = await provider.poll( - mailbox_path, - last_seen_uid=mailbox_obj.last_seen_uid, - highestmodseq=mailbox_obj.highestmodseq, - ) - await apply_delta(provider, account_obj, mailbox_obj, delta) + try: + await provider.idle(mailbox_path) + except ProviderNotSupported: + await asyncio.sleep(idle_fallback) finally: await provider.close() @@ -630,9 +647,7 @@ async def poll_folders_once( await provider.close() -async def _poll_secondary_once( - account_id: uuid.UUID, health: AccountHealth | None = None -) -> None: +async def _poll_secondary_once(account_id: uuid.UUID, health: AccountHealth | None = None) -> None: await poll_folders_once(account_id, include_inbox=False, health=health) diff --git a/backend/app/sync/folder_names.py b/backend/app/sync/folder_names.py index 14ad42d..b61049b 100644 --- a/backend/app/sync/folder_names.py +++ b/backend/app/sync/folder_names.py @@ -28,24 +28,52 @@ def _compact(name: str) -> str: # Localized folder names by special_use. Written readably; compacted at load. _NAMES: dict[str, list[str]] = { "sent": [ - "sent", "sent items", "sent mail", "sent messages", # English / Outlook / Gmail - "wysłane", "elementy wysłane", "wiadomości wysłane", # Polish - "gesendet", "gesendete objekte", "gesendete elemente", # German - "envoyés", "éléments envoyés", "messages envoyés", "courrier envoyé", # French - "enviados", "elementos enviados", "correo enviado", # Spanish - "inviata", "posta inviata", "messaggi inviati", # Italian - "verzonden", "verzonden items", "verzonden berichten", # Dutch - "enviadas", "itens enviados", "mensagens enviadas", # Portuguese - "отправленные", "исходящие", # Russian - "skickat", "skickade", "skickade objekt", # Swedish - "sendt", "sendte", "sendte elementer", # Norwegian / Danish + "sent", + "sent items", + "sent mail", + "sent messages", # English / Outlook / Gmail + "wysłane", + "elementy wysłane", + "wiadomości wysłane", # Polish + "gesendet", + "gesendete objekte", + "gesendete elemente", # German + "envoyés", + "éléments envoyés", + "messages envoyés", + "courrier envoyé", # French + "enviados", + "elementos enviados", + "correo enviado", # Spanish + "inviata", + "posta inviata", + "messaggi inviati", # Italian + "verzonden", + "verzonden items", + "verzonden berichten", # Dutch + "enviadas", + "itens enviados", + "mensagens enviadas", # Portuguese + "отправленные", + "исходящие", # Russian + "skickat", + "skickade", + "skickade objekt", # Swedish + "sendt", + "sendte", + "sendte elementer", # Norwegian / Danish "lähetetyt", # Finnish - "odeslané", "odeslaná pošta", # Czech + "odeslané", + "odeslaná pošta", # Czech ], "drafts": [ - "drafts", "draft", - "robocze", "kopie robocze", "wersje robocze", - "entwürfe", "entwurf", + "drafts", + "draft", + "robocze", + "kopie robocze", + "wersje robocze", + "entwürfe", + "entwurf", "brouillons", "borradores", "bozze", @@ -58,44 +86,80 @@ _NAMES: dict[str, list[str]] = { "koncepty", # Czech ], "trash": [ - "trash", "deleted", "deleted items", "deleted messages", "bin", "recycle bin", + "trash", + "deleted", + "deleted items", + "deleted messages", + "bin", + "recycle bin", "kosz", - "papierkorb", "gelöschte objekte", "gelöschte elemente", - "corbeille", "éléments supprimés", - "papelera", "elementos eliminados", - "cestino", "posta eliminata", - "prullenbak", "verwijderde items", - "lixeira", "lixo", "itens excluídos", - "корзина", "удаленные", - "papperskorg", "borttaget", - "papirkurv", "slettet", - "roskakori", "poistetut", + "papierkorb", + "gelöschte objekte", + "gelöschte elemente", + "corbeille", + "éléments supprimés", + "papelera", + "elementos eliminados", + "cestino", + "posta eliminata", + "prullenbak", + "verwijderde items", + "lixeira", + "lixo", + "itens excluídos", + "корзина", + "удаленные", + "papperskorg", + "borttaget", + "papirkurv", + "slettet", + "roskakori", + "poistetut", "koš", # Czech ], "junk": [ - "junk", "junk email", "junk e-mail", "spam", "bulk mail", "bulk", - "wiadomości-śmieci", "niechciane", - "unerwünscht", "werbung", - "courrier indésirable", "indésirables", "pourriel", - "correo no deseado", "no deseado", "correo basura", - "posta indesiderata", "indesiderata", - "ongewenst", "ongewenste e-mail", + "junk", + "junk email", + "junk e-mail", + "spam", + "bulk mail", + "bulk", + "wiadomości-śmieci", + "niechciane", + "unerwünscht", + "werbung", + "courrier indésirable", + "indésirables", + "pourriel", + "correo no deseado", + "no deseado", + "correo basura", + "posta indesiderata", + "indesiderata", + "ongewenst", + "ongewenste e-mail", "lixo eletrônico", - "спам", "нежелательная почта", - "skräp", "skräppost", + "спам", + "нежелательная почта", + "skräp", + "skräppost", "uønsket", "roskaposti", "nevyžádaná pošta", ], "archive": [ - "archive", "archives", "all mail", + "archive", + "archives", + "all mail", "archiwum", "archiv", "archivio", "archivo", "archief", - "arquivo", "arquivo morto", - "архив", "вся почта", + "arquivo", + "arquivo morto", + "архив", + "вся почта", "arkiv", "arkisto", ], diff --git a/backend/app/sync/provider_factory.py b/backend/app/sync/provider_factory.py index a859bbe..eb64cc1 100644 --- a/backend/app/sync/provider_factory.py +++ b/backend/app/sync/provider_factory.py @@ -33,5 +33,6 @@ def build_provider(account: Account) -> ImapSmtpProvider: smtp_tls=str(smtp.get("tls", "ssl")), username=account.email, password=password, + public_only=True, # a server the user named: never one inside our network ) return ImapSmtpProvider(config) diff --git a/backend/app/ws.py b/backend/app/ws.py index c0d9478..0bed565 100644 --- a/backend/app/ws.py +++ b/backend/app/ws.py @@ -18,6 +18,7 @@ from app.events import get_event_bus from app.identity.cookies import SESSION_COOKIE_INSECURE, SESSION_COOKIE_SECURE from app.identity.sessions import resolve_session from app.redis_client import get_redis +from app.security.cross_site import is_cross_site from app.sync import leases logger = logging.getLogger(__name__) @@ -27,6 +28,12 @@ router = APIRouter() @router.websocket("/ws") async def ws_endpoint(websocket: WebSocket) -> None: + # Only our own pages may listen in: a socket another site opens is refused + # even if the browser sent the session cookie with it (cross-site WebSocket + # hijacking; SameSite=Lax already stops current browsers from sending it). + if is_cross_site(websocket.headers): + await websocket.close(code=1008) + return token = websocket.cookies.get(SESSION_COOKIE_SECURE) or websocket.cookies.get( SESSION_COOKIE_INSECURE, "" ) diff --git a/backend/scripts/backfill_delivery_id.py b/backend/scripts/backfill_delivery_id.py index f5ad05a..58b0b18 100644 --- a/backend/scripts/backfill_delivery_id.py +++ b/backend/scripts/backfill_delivery_id.py @@ -16,9 +16,7 @@ from app.models import Message async def main() -> None: async with session_scope() as session: - rows = list( - await session.scalars(select(Message).where(Message.delivery_id.is_(None))) - ) + rows = list(await session.scalars(select(Message).where(Message.delivery_id.is_(None)))) fixed = 0 for m in rows: try: diff --git a/backend/scripts/reindex_address_parts.py b/backend/scripts/reindex_address_parts.py index 6a9d299..485a70d 100644 --- a/backend/scripts/reindex_address_parts.py +++ b/backend/scripts/reindex_address_parts.py @@ -42,9 +42,7 @@ async def main() -> None: update(Message) .where(Message.id == msg.id) .values( - body_indexed=Message.body_indexed.op("||")( - func.to_tsvector(_TS_CONFIG, blob) - ) + body_indexed=Message.body_indexed.op("||")(func.to_tsvector(_TS_CONFIG, blob)) ) ) updated += 1 diff --git a/backend/tests/accounts/test_discovery.py b/backend/tests/accounts/test_discovery.py index 4840852..754f5a6 100644 --- a/backend/tests/accounts/test_discovery.py +++ b/backend/tests/accounts/test_discovery.py @@ -37,10 +37,14 @@ AUTOCONFIG_XML = """ def _skip_ssrf(monkeypatch) -> None: # type: ignore[no-untyped-def] """Bypass the real DNS/public-IP check so respx can intercept the fetch.""" - async def ok(_host: str) -> None: - return None + async def ok(_host: str) -> list[str]: + return ["93.184.216.34"] + + async def unpinned(url: str) -> tuple[str, dict[str, str], dict[str, str]]: + return url, {}, {} # respx matches on the host name, not the pinned IP monkeypatch.setattr(ssrf, "assert_host_is_public", ok) + monkeypatch.setattr(ssrf, "_pinned", unpinned) # --- XML parsing ------------------------------------------------------------- diff --git a/backend/tests/api/test_api.py b/backend/tests/api/test_api.py index 3bdfc6d..c3ed44c 100644 --- a/backend/tests/api/test_api.py +++ b/backend/tests/api/test_api.py @@ -310,7 +310,8 @@ async def test_attachment_download_non_ascii_filename(api: ApiCtx) -> None: await api.session.commit() aid = next( - a["id"] for a in (await api.client.get(f"/api/messages/{mid}/attachments")).json() + a["id"] + for a in (await api.client.get(f"/api/messages/{mid}/attachments")).json() if a["filename"] == name ) resp = await api.client.get(f"/api/messages/{mid}/attachments/{aid}") @@ -404,8 +405,11 @@ async def test_folder_counts(api: ApiCtx) -> None: async def test_thread_collapses_cross_account_duplicate(api: ApiCtx) -> None: assert api.session is not None # The kickoff thread's first message; give it a real delivery_id. - kickoff = next(t for t in (await api.client.get("/api/threads")).json()["threads"] - if t["message_count"] == 2) + kickoff = next( + t + for t in (await api.client.get("/api/threads")).json()["threads"] + if t["message_count"] == 2 + ) detail = (await api.client.get(f"/api/threads/{kickoff['id']}")).json() first_id = uuid.UUID(detail["messages"][0]["id"]) first = await api.session.get(Message, first_id) @@ -414,20 +418,32 @@ async def test_thread_collapses_cross_account_duplicate(api: ApiCtx) -> None: # A second account receives the SAME email (same delivery_id) into the same thread. acct2 = Account( - user_id=api.user_id, email="me2@example.com", imap_config={}, smtp_config={}, - secret_ct=b"", discovery_src="manual", + user_id=api.user_id, + email="me2@example.com", + imap_config={}, + smtp_config={}, + secret_ct=b"", + discovery_src="manual", ) api.session.add(acct2) await api.session.flush() inbox2 = Mailbox(account_id=acct2.id, user_id=api.user_id, path="INBOX", special_use="inbox") api.session.add(inbox2) await api.session.flush() - api.session.add(Message( - user_id=api.user_id, account_id=acct2.id, mailbox_id=inbox2.id, uid=1, - thread_id=first.thread_id, message_id_hdr="", - delivery_id="", subject=first.subject, - internal_date=datetime.now(UTC), blob_sha256="z" * 64, # later -> first stays the rep - )) + api.session.add( + Message( + user_id=api.user_id, + account_id=acct2.id, + mailbox_id=inbox2.id, + uid=1, + thread_id=first.thread_id, + message_id_hdr="", + delivery_id="", + subject=first.subject, + internal_date=datetime.now(UTC), + blob_sha256="z" * 64, # later -> first stays the rep + ) + ) await api.session.commit() body = (await api.client.get(f"/api/threads/{kickoff['id']}")).json() diff --git a/backend/tests/api/test_saved_searches.py b/backend/tests/api/test_saved_searches.py index 76285d6..b49a62f 100644 --- a/backend/tests/api/test_saved_searches.py +++ b/backend/tests/api/test_saved_searches.py @@ -104,9 +104,7 @@ async def test_saved_search_crud_and_upsert(ctx: Ctx) -> None: async def test_saved_search_requires_csrf(ctx: Ctx) -> None: # No CSRF header -> rejected (state-changing route). - resp = await ctx.client.post( - "/api/saved-searches", json={"name": "x", "query": "y"} - ) + resp = await ctx.client.post("/api/saved-searches", json={"name": "x", "query": "y"}) assert resp.status_code == 403 diff --git a/backend/tests/conftest.py b/backend/tests/conftest.py index 07c0c22..e2a6b33 100644 --- a/backend/tests/conftest.py +++ b/backend/tests/conftest.py @@ -21,6 +21,8 @@ os.environ.setdefault("BLOB_ROOT", "./var/blobs-test") # Hosted mailboxes off: tests that need the mail server turn them on themselves # (tests/hosted/), against the dev stack's Stalwart container. os.environ["HOSTED_MAILBOXES"] = "false" +# No calls to the breached-password service from tests (app/identity/password_policy.py). +os.environ["PASSWORD_BREACH_CHECK"] = "false" for _key, _value in { "STALWART_URL": "http://127.0.0.1:8180", "STALWART_ADMIN_SECRET": "stalwart_admin_dev", diff --git a/backend/tests/mail/test_actions.py b/backend/tests/mail/test_actions.py index ce677f8..bba0f85 100644 --- a/backend/tests/mail/test_actions.py +++ b/backend/tests/mail/test_actions.py @@ -421,3 +421,36 @@ async def test_delete_keeps_rows_while_unreachable(ctx: Ctx, monkeypatch) -> Non select(func.count()).select_from(Message).where(Message.thread_id == ctx.thread_id) ) assert left == len(ctx.message_ids) # kept: the server still has them + + +async def test_restore_moves_only_what_was_removed(ctx: Ctx, monkeypatch) -> None: # type: ignore[no-untyped-def] + """A thread restored from Spam brings its spam back; the user's own reply in + Sent (never removed, so no recorded origin) stays in Sent.""" + sent = Mailbox(account_id=None, user_id=ctx.user_id, path="Sent", special_use="sent") + first = await ctx.session.get(Message, ctx.message_ids[0]) + reply = await ctx.session.get(Message, ctx.message_ids[1]) + assert first is not None and reply is not None + sent.account_id = first.account_id + ctx.session.add(sent) + await ctx.session.flush() + first.mailbox_id = ctx.archive_id # removed, origin unknown (as when synced in) + reply.mailbox_id = sent.id + await ctx.session.commit() + + moves: list[tuple[int, str, str]] = [] + + class Recorder: + async def move(self, uid: int, src: str, dst: str) -> int: + moves.append((uid, src, dst)) + return 100 + uid + + async def close(self) -> None: + return None + + monkeypatch.setattr(sync_actions, "build_provider", lambda _a: Recorder()) + await sync_actions.restore_thread_job({"job_try": 1}, str(ctx.thread_id)) + assert moves == [(first.uid, "Archive", "INBOX")] + ctx.session.expunge_all() + async with session_scope() as verify: + kept = await verify.get(Message, ctx.message_ids[1]) + assert kept is not None and kept.mailbox_id == sent.id diff --git a/backend/tests/plugins/test_calendar_reminders.py b/backend/tests/plugins/test_calendar_reminders.py index b0d9fb4..d4f51df 100644 --- a/backend/tests/plugins/test_calendar_reminders.py +++ b/backend/tests/plugins/test_calendar_reminders.py @@ -104,7 +104,8 @@ async def test_due_reminders_are_sent_once(ctx) -> None: # type: ignore[no-unty assert await run(MON_9 - timedelta(minutes=60)) == 1 # the 1-hour reminder assert sent[-1]["title"] == "Dentist" assert sent[-1]["body"] == "In 1 hour, 09:00 · Main St 4" - assert sent[-1]["url"].startswith("/p/calendar?date=2031-06-02&event=") and sent[-1]["sticky"] is True + assert sent[-1]["url"].startswith("/p/calendar?date=2031-06-02&event=") + assert sent[-1]["sticky"] is True assert await run(MON_9 - timedelta(minutes=59)) == 0 # not again # The worker was down when the 10-minute reminder came due: sent late, once. assert await run(MON_9 - timedelta(minutes=4)) == 1 diff --git a/backend/tests/plugins/test_filters_plugin.py b/backend/tests/plugins/test_filters_plugin.py index fd66411..e902711 100644 --- a/backend/tests/plugins/test_filters_plugin.py +++ b/backend/tests/plugins/test_filters_plugin.py @@ -49,8 +49,12 @@ async def ctx() -> AsyncIterator[_Ctx]: s.add(user) await s.flush() account = Account( - user_id=user.id, email="me@example.com", imap_config={}, smtp_config={}, - secret_ct=b"", discovery_src="manual", + user_id=user.id, + email="me@example.com", + imap_config={}, + smtp_config={}, + secret_ct=b"", + discovery_src="manual", ) s.add(account) await s.flush() @@ -64,12 +68,17 @@ async def ctx() -> AsyncIterator[_Ctx]: s.add(thread) await s.flush() msg = Message( - user_id=user.id, account_id=account.id, mailbox_id=mbox.id, uid=1, + user_id=user.id, + account_id=account.id, + mailbox_id=mbox.id, + uid=1, thread_id=thread.id, subject="Your Invoice 123", from_addr=[{"name": "Billing", "email": "billing@acme.com"}], to_addr=[{"name": "Me", "email": "me@example.com"}], - flags=[], internal_date=datetime.now(UTC), blob_sha256="", + flags=[], + internal_date=datetime.now(UTC), + blob_sha256="", ) s.add(msg) await s.commit() @@ -99,24 +108,30 @@ async def _flags(ctx: _Ctx) -> list[str]: async def test_hook_applies_flag_on_match(ctx: _Ctx) -> None: - await sdk("filters").kv(ctx.user_id).set( - "_rules", [{"field": "subject", "contains": "invoice", "action": "read"}] + await ( + sdk("filters") + .kv(ctx.user_id) + .set("_rules", [{"field": "subject", "contains": "invoice", "action": "read"}]) ) await on_message_new({"user_id": str(ctx.user_id), "message_id": str(ctx.message_id)}) assert "\\Seen" in await _flags(ctx) async def test_hook_no_match_leaves_flags(ctx: _Ctx) -> None: - await sdk("filters").kv(ctx.user_id).set( - "_rules", [{"field": "from", "contains": "newsletter@somewhere", "action": "pin"}] + await ( + sdk("filters") + .kv(ctx.user_id) + .set("_rules", [{"field": "from", "contains": "newsletter@somewhere", "action": "pin"}]) ) await on_message_new({"user_id": str(ctx.user_id), "message_id": str(ctx.message_id)}) assert await _flags(ctx) == [] async def test_hook_pin_applies_flagged(ctx: _Ctx) -> None: - await sdk("filters").kv(ctx.user_id).set( - "_rules", [{"field": "from", "contains": "billing@acme", "action": "pin"}] + await ( + sdk("filters") + .kv(ctx.user_id) + .set("_rules", [{"field": "from", "contains": "billing@acme", "action": "pin"}]) ) await on_message_new({"user_id": str(ctx.user_id), "message_id": str(ctx.message_id)}) assert "\\Flagged" in await _flags(ctx) # "pin" == the \Flagged flag in this app @@ -124,16 +139,16 @@ async def test_hook_pin_applies_flagged(ctx: _Ctx) -> None: async def test_hook_matches_on_to(ctx: _Ctx) -> None: # Multi-account setups match on the recipient address. - await sdk("filters").kv(ctx.user_id).set( - "_rules", [{"field": "to", "contains": "me@example.com", "action": "read"}] + await ( + sdk("filters") + .kv(ctx.user_id) + .set("_rules", [{"field": "to", "contains": "me@example.com", "action": "read"}]) ) await on_message_new({"user_id": str(ctx.user_id), "message_id": str(ctx.message_id)}) assert "\\Seen" in await _flags(ctx) -async def test_hook_trash_moves_thread_to_trash( - ctx: _Ctx, monkeypatch: pytest.MonkeyPatch -) -> None: +async def test_hook_trash_moves_thread_to_trash(ctx: _Ctx, monkeypatch: pytest.MonkeyPatch) -> None: calls: list[tuple[object, ...]] = [] async def fake_enqueue(name: str, *args: object, **kwargs: object) -> None: @@ -141,8 +156,10 @@ async def test_hook_trash_moves_thread_to_trash( # trash() enqueues the same job the UI Delete uses (imported from app.queue). monkeypatch.setattr("app.queue.enqueue_job", fake_enqueue) - await sdk("filters").kv(ctx.user_id).set( - "_rules", [{"field": "subject", "contains": "invoice", "action": "trash"}] + await ( + sdk("filters") + .kv(ctx.user_id) + .set("_rules", [{"field": "subject", "contains": "invoice", "action": "trash"}]) ) await on_message_new({"user_id": str(ctx.user_id), "message_id": str(ctx.message_id)}) assert ("move_thread_to_folder_job", str(ctx.thread_id), "trash") in calls diff --git a/backend/tests/plugins/test_hello_plugin.py b/backend/tests/plugins/test_hello_plugin.py index 71d7985..282b50a 100644 --- a/backend/tests/plugins/test_hello_plugin.py +++ b/backend/tests/plugins/test_hello_plugin.py @@ -68,9 +68,7 @@ async def ctx() -> AsyncIterator[_Ctx]: await c.client_a.aclose() await c.client_b.aclose() async with sm() as s: - await s.execute( - delete(User).where(User.id.in_([c.user_a, c.user_b])) - ) + await s.execute(delete(User).where(User.id.in_([c.user_a, c.user_b]))) await s.execute( text("DELETE FROM plg_hello_note WHERE user_id IN (:a, :b)"), {"a": str(c.user_a), "b": str(c.user_b)}, diff --git a/backend/tests/plugins/test_migrate.py b/backend/tests/plugins/test_migrate.py index 4c5c359..d5708f1 100644 --- a/backend/tests/plugins/test_migrate.py +++ b/backend/tests/plugins/test_migrate.py @@ -51,9 +51,10 @@ async def test_install_run_uninstall_dropdata_roundtrip() -> None: # Table exists, RLS is enabled on it, and the per-plugin version table is present. assert await _regclass("plg_demo_note") == "plg_demo_note" - assert await _scalar( - "SELECT relrowsecurity FROM pg_class WHERE relname = 'plg_demo_note'" - ) is True + assert ( + await _scalar("SELECT relrowsecurity FROM pg_class WHERE relname = 'plg_demo_note'") + is True + ) assert await _regclass("alembic_version_plg_demo") == "alembic_version_plg_demo" # --drop-data removes the table AND the version table: zero residue. diff --git a/backend/tests/plugins/test_sdk_messages.py b/backend/tests/plugins/test_sdk_messages.py index 3a5cb92..08328df 100644 --- a/backend/tests/plugins/test_sdk_messages.py +++ b/backend/tests/plugins/test_sdk_messages.py @@ -27,8 +27,12 @@ async def test_messages_header_reads_raw_header() -> None: s.add(user) await s.flush() account = Account( - user_id=user.id, email="me@example.com", imap_config={}, smtp_config={}, - secret_ct=b"", discovery_src="manual", + user_id=user.id, + email="me@example.com", + imap_config={}, + smtp_config={}, + secret_ct=b"", + discovery_src="manual", ) s.add(account) await s.flush() @@ -44,8 +48,14 @@ async def test_messages_header_reads_raw_header() -> None: sha = blobstore.store(user.id, raw.as_bytes()) msg = Message( - user_id=user.id, account_id=account.id, mailbox_id=mbox.id, uid=1, - subject="Weekly digest", flags=[], internal_date=datetime.now(UTC), blob_sha256=sha, + user_id=user.id, + account_id=account.id, + mailbox_id=mbox.id, + uid=1, + subject="Weekly digest", + flags=[], + internal_date=datetime.now(UTC), + blob_sha256=sha, ) s.add(msg) await s.commit() diff --git a/backend/tests/plugins/test_ui_manifest_api.py b/backend/tests/plugins/test_ui_manifest_api.py index 42c6599..24f695a 100644 --- a/backend/tests/plugins/test_ui_manifest_api.py +++ b/backend/tests/plugins/test_ui_manifest_api.py @@ -44,9 +44,7 @@ async def test_ui_manifest_endpoint() -> None: ) try: # Unauthenticated request is rejected. - async with httpx.AsyncClient( - transport=transport, base_url="http://testserver" - ) as anon: + async with httpx.AsyncClient(transport=transport, base_url="http://testserver") as anon: assert (await anon.get("/api/plugins/ui-manifest")).status_code == 401 resp = await client.get("/api/plugins/ui-manifest") diff --git a/backend/tests/providers/test_parsing.py b/backend/tests/providers/test_parsing.py index 0887a6c..6aad6ac 100644 --- a/backend/tests/providers/test_parsing.py +++ b/backend/tests/providers/test_parsing.py @@ -100,7 +100,15 @@ def test_parse_search_uids() -> None: def test_parse_vanished() -> None: # aioimaplib strips the leading "* " before buffering the line. - assert parse_vanished([b"VANISHED (EARLIER) 300:305,405"]) == [300, 301, 302, 303, 304, 305, 405] # noqa: E501 + assert parse_vanished([b"VANISHED (EARLIER) 300:305,405"]) == [ + 300, + 301, + 302, + 303, + 304, + 305, + 405, + ] # noqa: E501 # Live-expunge form (no EARLIER) and a descending range are both handled. assert parse_vanished([b"VANISHED 41", b"VANISHED (EARLIER) 5:3"]) == [41, 3, 4, 5] # Non-VANISHED lines are ignored; non-bytes entries are skipped. diff --git a/backend/tests/search/test_meili.py b/backend/tests/search/test_meili.py index 54040a1..6b969e0 100644 --- a/backend/tests/search/test_meili.py +++ b/backend/tests/search/test_meili.py @@ -52,9 +52,7 @@ async def test_meili_typo_tolerance_and_filters() -> None: await backend.index(_doc(user_id)) # Acceptance (§12 M2): a typo still finds the message. - hits, _c = await backend.query( - to_search_query("invocie", user_id), limit=10, cursor=None - ) + hits, _c = await backend.query(to_search_query("invocie", user_id), limit=10, cursor=None) if not hits: # wait for the async index task assert await _poll(backend, "invocie", user_id) == 1 hits, _c = await backend.query( diff --git a/backend/tests/search/test_search_api.py b/backend/tests/search/test_search_api.py index bac8839..836e857 100644 --- a/backend/tests/search/test_search_api.py +++ b/backend/tests/search/test_search_api.py @@ -159,8 +159,12 @@ async def test_search_excludes_trash_by_default(ctx: Ctx) -> None: ctx.session.add(trash) await ctx.session.flush() await _ingest( - ctx, frm="X ", subject="zephyr in trash", body="bye", - mailbox_id=trash.id, special="trash", + ctx, + frm="X ", + subject="zephyr in trash", + body="bye", + mailbox_id=trash.id, + special="trash", ) assert await _search(ctx, "zephyr") == 1 # inbox only; the trashed one is hidden assert await _search(ctx, "in:trash zephyr") == 1 # explicit -> searches Trash diff --git a/backend/tests/security/test_hardening.py b/backend/tests/security/test_hardening.py new file mode 100644 index 0000000..ae5dea9 --- /dev/null +++ b/backend/tests/security/test_hardening.py @@ -0,0 +1,285 @@ +"""Fixes from the 2026-10-14 security assessment (ultimo-security-report.md): +cross-site requests refused (login CSRF), terse validation errors, the new-password +policy, link schemes in message HTML, and DNS-rebinding-proof outbound fetches.""" + +from __future__ import annotations + +import asyncio +import uuid +from collections.abc import AsyncIterator + +import httpx +import pytest +import pytest_asyncio +from httpx import ASGITransport +from starlette.datastructures import Headers + +from app.accounts import ssrf +from app.config import get_settings +from app.identity import password_policy +from app.identity.password_policy import PasswordRejected, check_new_password +from app.indexer.sanitize import sanitize_html +from app.main import create_app +from app.security.cross_site import is_cross_site + + +@pytest_asyncio.fixture +async def client() -> AsyncIterator[httpx.AsyncClient]: + async with httpx.AsyncClient( + transport=ASGITransport(app=create_app()), base_url="http://mail.example.org" + ) as c: + yield c + + +def _login(client: httpx.AsyncClient, headers: dict[str, str]): # type: ignore[no-untyped-def] + return client.post( + "/api/auth/login", json={"handle": "nobody", "password": "x"}, headers=headers + ) + + +# --- cross-site requests (finding 3) -------------------------------------------------- + + +@pytest.mark.parametrize( + "headers", + [ + {"Sec-Fetch-Site": "cross-site", "Origin": "https://evil.example"}, + {"Origin": "https://evil.example"}, # an older browser: Origin only + {"Origin": "null"}, # a sandboxed frame + ], +) +async def test_sign_in_from_another_site_is_refused( + client: httpx.AsyncClient, headers: dict[str, str] +) -> None: + resp = await _login(client, headers) + assert resp.status_code == 403 + assert resp.json()["error"]["code"] == "cross_site" + + +@pytest.mark.parametrize( + "headers", + [ + {"Sec-Fetch-Site": "same-origin", "Origin": "http://mail.example.org"}, + {"Origin": "http://mail.example.org"}, + {"Origin": "https://mail.example.org:8443"}, # the port does not matter + {"Sec-Fetch-Site": "same-site"}, # our own subdomain + {"Sec-Fetch-Site": "none"}, # typed into the address bar + {}, # not a browser (scripts, mail apps) + ], +) +async def test_sign_in_from_our_own_pages_goes_through( + client: httpx.AsyncClient, headers: dict[str, str] +) -> None: + resp = await _login(client, headers) + assert resp.status_code == 401 # evaluated: wrong credentials, not refused + assert resp.json()["error"]["code"] == "invalid_credentials" + + +async def test_reads_from_another_site_are_not_refused(client: httpx.AsyncClient) -> None: + resp = await client.get("/api/auth/registration", headers={"Sec-Fetch-Site": "cross-site"}) + assert resp.status_code == 200 + + +def test_origin_on_our_own_domain_passes_whatever_the_host_header() -> None: + domain = get_settings().app_domain + headers = Headers({"origin": f"https://{domain}", "host": "localhost:8088"}) + assert is_cross_site(headers) is False + sub = Headers({"origin": f"https://mail.{domain}", "host": "localhost:8088"}) + assert is_cross_site(sub) is False + lookalike = Headers({"origin": f"https://evil{domain}", "host": "localhost:8088"}) + assert is_cross_site(lookalike) is True + + +def test_origin_matches_the_name_a_reverse_proxy_saw() -> None: + headers = Headers( + {"origin": "https://ultimo.email", "host": "web:80", "x-forwarded-host": "ultimo.email"} + ) + assert is_cross_site(headers) is False + + +# --- validation errors (finding 5) ---------------------------------------------------- + + +async def test_validation_errors_do_not_spell_out_the_request_model( + client: httpx.AsyncClient, +) -> None: + resp = await client.post("/api/auth/login", json={}) + assert resp.status_code == 422 + body = resp.json() + assert "detail" not in body + assert body["error"]["code"] == "validation_error" + assert body["error"]["message"].startswith("handle:") + + +async def test_a_too_short_password_says_so(client: httpx.AsyncClient) -> None: + resp = await client.post("/api/auth/register", json={"localpart": "abcde", "password": "123"}) + assert resp.status_code == 422 + assert "password" in resp.json()["error"]["message"] + + +async def test_new_passwords_are_capped_at_128(client: httpx.AsyncClient) -> None: + resp = await client.post( + "/api/auth/register", json={"localpart": "abcde", "password": "x" * 129} + ) + assert resp.status_code == 422 + + +# --- new-password policy (finding 6) -------------------------------------------------- + + +@pytest.mark.parametrize( + "password", + ["password123", "Password123", "12345678", "aaaaaaaaaa", "abababab", "jsmith2026"], +) +async def test_guessable_passwords_are_refused(password: str) -> None: + with pytest.raises(PasswordRejected): + await check_new_password(password, "jsmith@example.org") + + +async def test_a_decent_password_passes() -> None: + await check_new_password("correct horse battery", "jsmith@example.org") + + +async def test_breached_passwords_are_refused(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setattr(get_settings(), "password_breach_check", True) + sha = "A" * 40 + + async def breached(_password: str) -> bool: + return True + + monkeypatch.setattr(password_policy, "_breached", breached) + with pytest.raises(PasswordRejected) as err: + await check_new_password("long unusual phrase " + sha, "jsmith@example.org") + assert err.value.code == "breached_password" + + +async def test_the_breach_service_being_down_does_not_block_sign_up( + monkeypatch: pytest.MonkeyPatch, +) -> None: + def unreachable(request: httpx.Request) -> httpx.Response: + raise httpx.ConnectError("down", request=request) + + real_client = httpx.AsyncClient + + def client_factory(*args, **kwargs): # type: ignore[no-untyped-def] + return real_client(transport=httpx.MockTransport(unreachable)) + + monkeypatch.setattr(password_policy.httpx, "AsyncClient", client_factory) + assert await password_policy._breached("anything at all") is False + + +async def test_sign_up_with_a_common_password_is_refused(client: httpx.AsyncClient) -> None: + resp = await client.post( + "/api/auth/register", + json={"localpart": f"pw{uuid.uuid4().hex[:8]}", "password": "password123"}, + ) + assert resp.status_code == 400 + assert resp.json()["error"]["code"] == "weak_password" + + +# --- message HTML (V2) ---------------------------------------------------------------- + + +@pytest.mark.parametrize( + "html", + [ + 'x', + 'x', + 'x', + 'x', + "", + "", + "