diff options
5 files changed, 251 insertions, 16 deletions
diff --git a/packages/meshbay-hub/src/meshbay_hub/api/groups.py b/packages/meshbay-hub/src/meshbay_hub/api/groups.py index b1a22f7..c44888a 100644 --- a/packages/meshbay-hub/src/meshbay_hub/api/groups.py +++ b/packages/meshbay-hub/src/meshbay_hub/api/groups.py @@ -2,7 +2,7 @@ import re -from fastapi import APIRouter, Depends, HTTPException, Request +from fastapi import APIRouter, Depends, HTTPException, Query, Request from pydantic import BaseModel from datetime import datetime, timezone from sqlalchemy import func, or_, select, update @@ -140,9 +140,12 @@ async def group_online_nodes( @router.get("") async def list_public_groups( db: AsyncSession = Depends(get_db), - q: str = "", - limit: int = 50, - offset: int = 0, + q: str = Query(default="", max_length=200), + # This one takes no authentication at all, and had no upper bound: any + # stranger could ask the hub for the entire public directory in a single + # query, repeatedly. Bounded like every list in admin.py. + limit: int = Query(default=50, ge=1, le=200), + offset: int = Query(default=0, ge=0), include_federated: bool = True, ): """List/search public groups — local and optionally federated. No auth required.""" @@ -777,7 +780,8 @@ async def invite_notify( return {"status": "no_email"} try: - mail.send_invite_notification( + await mail.send_off_loop( + mail.send_invite_notification, email, body.code, current_user.username, group.name) except Exception: return {"status": "send_failed"} diff --git a/packages/meshbay-hub/src/meshbay_hub/api/notifications.py b/packages/meshbay-hub/src/meshbay_hub/api/notifications.py index 9d5c125..b5783ab 100644 --- a/packages/meshbay-hub/src/meshbay_hub/api/notifications.py +++ b/packages/meshbay-hub/src/meshbay_hub/api/notifications.py @@ -19,7 +19,7 @@ migration for no gain, and `unread_only` stays because it is what an older interface asks for and it still answers correctly — every row is unread. """ -from fastapi import APIRouter, Depends, HTTPException +from fastapi import APIRouter, Depends, HTTPException, Query from datetime import datetime, timezone from sqlalchemy import delete, func, select @@ -36,8 +36,11 @@ router = APIRouter(prefix="/v1/notifications", tags=["notifications"]) async def list_notifications( current_user: User = Depends(get_current_user), db: AsyncSession = Depends(get_db), - limit: int = 50, - offset: int = 0, + # Bounded like every list in admin.py. These two were not, so one caller + # could ask for the whole table in one query — and a negative limit is a + # 500 on PostgreSQL rather than an empty page. + limit: int = Query(default=50, ge=1, le=200), + offset: int = Query(default=0, ge=0), unread_only: bool = False, ): query = select(Notification).where(Notification.user_id == current_user.id) diff --git a/packages/meshbay-hub/src/meshbay_hub/api/users.py b/packages/meshbay-hub/src/meshbay_hub/api/users.py index a1b436e..05bf075 100644 --- a/packages/meshbay-hub/src/meshbay_hub/api/users.py +++ b/packages/meshbay-hub/src/meshbay_hub/api/users.py @@ -2,6 +2,7 @@ import base64 import logging +import re import secrets import time import uuid @@ -10,7 +11,7 @@ from datetime import datetime, timedelta, timezone from cryptography.hazmat.primitives.asymmetric.ed25519 import Ed25519PublicKey from fastapi import APIRouter, Depends, HTTPException, Request from pydantic import BaseModel, field_validator -from sqlalchemy import delete, select, update +from sqlalchemy import delete, func, select, update from sqlalchemy.ext.asyncio import AsyncSession from meshbay_hub import mail @@ -222,7 +223,8 @@ async def _create_and_send_verification( expires_at=datetime.now(timezone.utc) + timedelta(seconds=VERIFICATION_TTL), )) await db.flush() - mail.send_verification_code(email, code, recovery_key=recovery_key) + await mail.send_off_loop( + mail.send_verification_code, email, code, recovery_key=recovery_key) class VerifyEmailRequest(BaseModel): @@ -634,17 +636,50 @@ class UpdateProfileRequest(BaseModel): return v +# One change-of-address mail per account per this many seconds. The window is +# the account's, not the caller's IP: the point is the mailbox on the receiving +# end, and an IP is not what fills it. +EMAIL_CHANGE_COOLDOWN = 60 + + @router.patch("/me") +@limiter.limit("10/minute") async def update_profile( body: UpdateProfileRequest, + request: Request, current_user: User = Depends(require_user_scope), db: AsyncSession = Depends(get_db), ): + """Update the signed-in account. Changing the address sends a code to it. + + Availability: this is the third path that makes the hub send mail, and it + was the one with no rate limit and no captcha — while `register` and + `password/reset-request` have both. The address is any string the caller + types, and the duplicate check below only rejects one already held by an + account here, so every address that is *not* registered on this hub was a + valid target. Any signed-in user could therefore have the hub mail + arbitrary strangers, from its own domain, as fast as it would go: a relay + for verification codes with someone else's reputation attached. + """ pending_email = None if body.email is not None: new_email = body.email.strip() eh = hash_email_blind(new_email) + # A per-account floor under the rate limit above, which counts by IP + # and so is not a bound on how much mail one account can cause. + recent = await db.execute( + select(EmailVerification).where( + EmailVerification.user_id == current_user.id, + EmailVerification.purpose == "email_change", + EmailVerification.created_at + > datetime.now(timezone.utc) - timedelta(seconds=EMAIL_CHANGE_COOLDOWN), + )) + if recent.first(): + raise HTTPException( + status_code=429, + detail="A code was just sent. Wait a minute before asking again.") + # Check that no other active/pending account uses this email dup = await db.execute( select(User).where( @@ -675,7 +710,7 @@ async def update_profile( expires_at=datetime.now(timezone.utc) + timedelta(seconds=VERIFICATION_TTL), )) await db.flush() - mail.send_email_change_code(new_email, code) + await mail.send_off_loop(mail.send_email_change_code, new_email, code) pending_email = new_email await db.commit() @@ -850,6 +885,12 @@ class ResetPasswordRequest(BaseModel): new_auth_key: str +# One reset mail per account per this many seconds, whoever asks and from +# wherever. The rate limit above counts by IP, which bounds a caller, not an +# inbox. +RESET_MAIL_COOLDOWN = 60 + + @router.post("/password/reset-request") @limiter.limit("5/minute") async def password_reset_request( @@ -874,6 +915,19 @@ async def password_reset_request( ) if matched: + # Per account, under the per-IP limit above. Knowing the pair is the + # hard part and this endpoint is careful about it, but once someone + # does, the cost of repeating lands in a mailbox that is not theirs. + recent = await db.execute( + select(EmailVerification).where( + EmailVerification.user_id == user.id, + EmailVerification.purpose == "password_reset", + EmailVerification.created_at + > datetime.now(timezone.utc) - timedelta(seconds=RESET_MAIL_COOLDOWN), + )) + if recent.first(): + return {"status": "sent_if_exists"} + prev = await db.execute( select(EmailVerification).where( EmailVerification.user_id == user.id, @@ -896,7 +950,8 @@ async def password_reset_request( ip_address=client_ip(request))) await db.flush() try: - mail.send_password_reset_code(decrypt_email(user.email), code) + await mail.send_off_loop( + mail.send_password_reset_code, decrypt_email(user.email), code) except Exception: log.exception("Failed to send passphrase reset code") await db.commit() @@ -974,12 +1029,27 @@ ALLOWED_PREF_KEYS = frozenset([ "music_keep_screen_on", ]) +# `default_tab:<group_id>`, which is what the SPA writes (group-page.js). The +# suffix used to be unchecked, and the route is `{key:path}`, so any string of +# any length was a distinct key: one account could write unbounded rows into a +# table shared with everyone, each with an unbounded `value` (the column is +# Text). A key over 64 characters was also not a 400 but a 500 — the column is +# String(64), which PostgreSQL enforces and SQLite does not, so it would have +# appeared in production and in no test. +_PREF_GROUP_KEY = re.compile( + r"^default_tab:[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}" + r"-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}$") + +# A person cannot be in more groups than this on one hub without noticing; the +# bound is on rows, because that is what is shared. +MAX_PREFERENCES_PER_ACCOUNT = 500 +MAX_PREFERENCE_VALUE = 256 + + def _valid_pref_key(key: str) -> bool: if key in ALLOWED_PREF_KEYS: return True - if key.startswith("default_tab:"): - return True - return False + return bool(_PREF_GROUP_KEY.match(key)) @router.get("/me/preferences") @@ -1005,7 +1075,9 @@ async def set_preference( db: AsyncSession = Depends(get_db), ): if not _valid_pref_key(key): - raise HTTPException(status_code=400, detail=f"Unknown preference key: {key}") + raise HTTPException(status_code=400, detail=f"Unknown preference key: {key[:80]}") + if len(body.value) > MAX_PREFERENCE_VALUE: + raise HTTPException(status_code=422, detail="Preference value too long") result = await db.execute( select(UserPreference).where( UserPreference.user_id == current_user.id, @@ -1014,6 +1086,12 @@ async def set_preference( if pref: pref.value = body.value else: + held = (await db.execute( + select(func.count()).select_from(UserPreference) + .where(UserPreference.user_id == current_user.id))).scalar() or 0 + if held >= MAX_PREFERENCES_PER_ACCOUNT: + raise HTTPException( + status_code=429, detail="Too many stored preferences") db.add(UserPreference( user_id=current_user.id, key=key, value=body.value)) await db.commit() diff --git a/packages/meshbay-hub/src/meshbay_hub/mail.py b/packages/meshbay-hub/src/meshbay_hub/mail.py index 4f776bf..8373204 100644 --- a/packages/meshbay-hub/src/meshbay_hub/mail.py +++ b/packages/meshbay-hub/src/meshbay_hub/mail.py @@ -5,6 +5,7 @@ Postfix listens on loopback only (inet_interfaces = loopback-only), so no authentication is needed. See docs/MAIL-SERVER.md for the full setup. """ +import asyncio import logging import smtplib from email.message import EmailMessage @@ -22,6 +23,7 @@ def configure(hub_id: str) -> None: def _send(msg: EmailMessage) -> bool: + """Blocking. Every caller in an async handler must use `send_off_loop`.""" try: with smtplib.SMTP("localhost", 25, timeout=10) as s: s.send_message(msg) @@ -31,6 +33,22 @@ def _send(msg: EmailMessage) -> bool: return False +async def send_off_loop(fn, *args, **kwargs) -> None: + """Run one of the `send_*` functions below in a worker thread. + + `smtplib` is synchronous and this one waits up to ten seconds. Called + directly from an async handler — which is what all four call sites did — + that ten seconds is not one request's, it is **the whole hub's**: no other + request is served, no node socket is read, no WebRTC offer is relayed, + for as long as the MTA takes to answer. An unreachable mail server made + the instance stop responding to everyone, and one of the three paths that + reaches it (`PATCH /v1/users/me`) had no rate limit at all. + + So the cost of a slow MTA is one request now, not the instance. + """ + await asyncio.to_thread(fn, *args, **kwargs) + + def send_verification_code(to: str, code: str, recovery_key: str | None = None) -> None: """ Registration verification e-mail. When `recovery_key` is given it is diff --git a/packages/meshbay-hub/tests/test_availability_between_members.py b/packages/meshbay-hub/tests/test_availability_between_members.py index f2c9a4f..282169d 100644 --- a/packages/meshbay-hub/tests/test_availability_between_members.py +++ b/packages/meshbay-hub/tests/test_availability_between_members.py @@ -386,3 +386,135 @@ async def test_a_captured_relay_registration_is_not_replayable(client): assert r.status_code == 401, r.text finally: relay_mod._relays.pop("r2", None) + + +# ── Mail: three paths out of the hub, one of them unmetered ────────────────── + +@pytest.mark.asyncio +async def test_changing_your_address_cannot_mail_strangers_at_will( + client, monkeypatch): + """ + `PATCH /v1/users/me` is the third path that makes the hub send mail, and + it was the one with no rate limit and no captcha — while `register` and + `password/reset-request` have both. The address is any string the caller + types, and the duplicate check only rejects one already held by an account + here, so every address *not* registered on this hub was a valid target. + """ + sent: list = [] + import meshbay_hub.mail as mail_mod + monkeypatch.setattr(mail_mod, "send_email_change_code", + lambda *a, **kw: sent.append(a)) + + user = await _make_user(client, "av_mailer") + headers = {"Authorization": f"Bearer {user['token']}"} + + r = await client.patch("/v1/users/me", headers=headers, + json={"email": "a-stranger@example.test"}) + assert r.status_code == 200, r.text + assert len(sent) == 1 + + r = await client.patch("/v1/users/me", headers=headers, + json={"email": "another-stranger@example.test"}) + assert r.status_code == 429, r.text + assert len(sent) == 1, "the hub mailed a second stranger on demand" + + +@pytest.mark.asyncio +async def test_a_reset_mail_lands_once_per_account_per_window(client, monkeypatch): + """Knowing the username/email pair is the hard part, and this endpoint is + careful about it. Once someone does, the cost of repeating lands in a + mailbox that is not theirs — and the rate limit above counts by IP.""" + sent: list = [] + import meshbay_hub.mail as mail_mod + monkeypatch.setattr(mail_mod, "send_password_reset_code", + lambda *a, **kw: sent.append(a)) + + user = await _make_user(client, "av_resettee") + body = {"username": user["username"], "email": "av_resettee@example.test"} + + for _ in range(3): + r = await client.post("/v1/users/password/reset-request", json=body) + assert r.status_code == 200, r.text + assert len(sent) == 1, f"{len(sent)} reset mails for one account in one window" + + +def test_no_mail_is_sent_from_the_event_loop(): + """ + `smtplib` is synchronous and waits up to ten seconds. Called straight from + an async handler — which is what all four call sites did — that wait is not + one request's, it is the whole hub's: nothing else is served, no node + socket is read, no offer relayed, until the MTA answers. + + Read from the source because the failure has no symptom a test can catch: + everything works, slowly, for everyone, whenever the mail server is having + a bad day. + """ + import pathlib + import re as _re + + root = pathlib.Path(__file__).resolve().parents[1] / "src" / "meshbay_hub" + # A direct *call* — `mail.send_x(`. A bare `mail.send_x` with no paren is + # the function being handed to send_off_loop, which is the point. The + # first version of this matched those continuation lines and so failed on + # the fixed code: look for the call, not for the name. + direct_call = _re.compile(r"\bmail\.send_(?!off_loop)\w+\s*\(") + offenders = [] + for path in root.rglob("*.py"): + if path.name == "mail.py": + continue + for n, line in enumerate(path.read_text().splitlines(), 1): + if direct_call.search(line): + offenders.append(f"{path.name}:{n}: {line.strip()}") + assert not offenders, ( + "these call a blocking SMTP send directly; use mail.send_off_loop:\n" + + "\n".join(offenders)) + + +# ── One account's rows are not the whole table ─────────────────────────────── + +@pytest.mark.asyncio +async def test_the_preference_namespace_is_not_open(client): + """ + `default_tab:` accepted any suffix, on a `{key:path}` route, with an + unbounded Text value: one account could write unbounded rows into a table + shared with everyone. The suffix is a group id — that is what the SPA + writes — so it is checked as one. + """ + user = await _make_user(client, "av_prefs") + headers = {"Authorization": f"Bearer {user['token']}"} + gid = await _make_group(client, user, "prefs-group") + + r = await client.put(f"/v1/users/me/preferences/default_tab:{gid}", + headers=headers, json={"value": "files"}) + assert r.status_code == 200, r.text + + for bad in ("default_tab:" + "x" * 300, "default_tab:not-a-uuid", + "default_tab:", "default_tab:../../etc"): + r = await client.put(f"/v1/users/me/preferences/{bad}", + headers=headers, json={"value": "files"}) + assert r.status_code == 400, f"{bad!r} was accepted: {r.text}" + + r = await client.put(f"/v1/users/me/preferences/default_tab:{gid}", + headers=headers, json={"value": "f" * 5000}) + assert r.status_code == 422, r.text + + +@pytest.mark.asyncio +async def test_a_list_cannot_be_asked_for_the_whole_table(client): + """Every list in admin.py carries `le=200`. These two did not — and the + public group directory takes no authentication at all.""" + user = await _make_user(client, "av_lister") + headers = {"Authorization": f"Bearer {user['token']}"} + + r = await client.get("/v1/notifications?limit=1000000", headers=headers) + assert r.status_code == 422, r.text + r = await client.get("/v1/notifications?limit=-1", headers=headers) + assert r.status_code == 422, r.text + + r = await client.get("/v1/groups?limit=1000000") + assert r.status_code == 422, r.text + r = await client.get("/v1/groups?offset=-5") + assert r.status_code == 422, r.text + + r = await client.get("/v1/notifications?limit=20", headers=headers) + assert r.status_code == 200, r.text |