feat(security): R4 S3 — abuse & leak hardening
- C-3.7: fold a per-link password_version into the signed watch grant (migration
0059 adds the column) and bump it on every password change, so rotating a share
link's password invalidates outstanding grants at once instead of letting them
live out the 6h TTL. make_grant/check_grant take the version; update_link bumps.
- C-3.8: the share recipient picker and share-by-email now reuse the messageable
filter (not-demo AND active AND not-suspended) instead of only excluding demo,
so suspended/deactivated addresses aren't leaked or reachable as targets.
- C-3.9: GET /keys/{user_id} adopts get_thread's MB2 guard — a non-messageable
target with no shared history returns the same "User not found" as a missing id,
so it can't be probed to enumerate users or fetch suspended users' keys.
- C-3.12: _register_account catches IntegrityError on the insert (the select-then-
insert TOCTOU) and treats it as the already-registered no-op.
- C-3.13: trigger_job claims the running flag atomically under _activity_lock
(compare-and-set) instead of a bare check, closing the double-start window.
- Move _messageable/is_messageable_user into a shared app/userscope.py so downloads
doesn't import the messages route module.
This commit is contained in:
@@ -31,6 +31,7 @@ from app.models import (
|
||||
User,
|
||||
)
|
||||
from app.security import hash_password
|
||||
from app.userscope import is_messageable_user, messageable_clauses
|
||||
|
||||
router = APIRouter(prefix="/api/downloads", tags=["downloads"])
|
||||
admin_router = APIRouter(prefix="/api/admin/downloads", tags=["admin-downloads"])
|
||||
@@ -666,7 +667,9 @@ def share_download(
|
||||
recipient = db.execute(
|
||||
select(User).where(func.lower(User.email) == target)
|
||||
).scalar_one_or_none()
|
||||
if recipient is None:
|
||||
# Uniform "no user" for a missing OR non-messageable (demo/suspended/deactivated) target, so
|
||||
# sharing can't be used to probe which addresses exist or to reach a suspended account.
|
||||
if not is_messageable_user(recipient):
|
||||
raise HTTPException(status_code=404, detail="No user with that email.")
|
||||
if recipient.id == user.id:
|
||||
raise HTTPException(status_code=400, detail="That's already your download.")
|
||||
@@ -741,11 +744,12 @@ def remove_shared_with_me(
|
||||
def share_recipients(
|
||||
user: User = Depends(require_human), db: Session = Depends(get_db)
|
||||
) -> list[dict]:
|
||||
"""Registered users this user can share a download with (excludes self + the demo account)."""
|
||||
"""Registered users this user can share a download with (excludes self + any non-messageable
|
||||
account — demo, suspended, or deactivated — so the picker never leaks those addresses)."""
|
||||
rows = (
|
||||
db.execute(
|
||||
select(User)
|
||||
.where(User.id != user.id, User.is_demo.is_(False))
|
||||
.where(User.id != user.id, *messageable_clauses())
|
||||
.order_by(func.lower(User.email))
|
||||
)
|
||||
.scalars()
|
||||
@@ -834,6 +838,9 @@ def update_link(
|
||||
if "password" in payload:
|
||||
pw = (payload.get("password") or "").strip()
|
||||
link.password_hash = hash_password(pw) if pw else None
|
||||
# Bump the version so any grant issued under the previous password stops working now, rather
|
||||
# than living out its 6h TTL (setting/clearing/changing the password all count as a rotation).
|
||||
link.password_version += 1
|
||||
db.commit()
|
||||
return linksmod.owner_view(link)
|
||||
|
||||
|
||||
@@ -26,6 +26,7 @@ from app.models import Message, MessageKey, User
|
||||
from app.ratelimit import RateLimiter
|
||||
from app.realtime import manager
|
||||
from app.security import verify_password
|
||||
from app.userscope import is_messageable_user, messageable_clauses
|
||||
|
||||
router = APIRouter(prefix="/api/messages", tags=["messages"])
|
||||
|
||||
@@ -101,18 +102,6 @@ def _serialize_msg(m: Message) -> dict:
|
||||
}
|
||||
|
||||
|
||||
def _messageable() -> list:
|
||||
"""SQL clauses for "a real, active, non-suspended human" — for directory/recipient queries."""
|
||||
return [
|
||||
User.is_demo.is_(False),
|
||||
User.is_active.is_(True),
|
||||
User.is_suspended.is_(False),
|
||||
]
|
||||
|
||||
|
||||
def is_messageable_user(u: User | None) -> bool:
|
||||
"""Python mirror of `_messageable()` for an already-loaded row (WS auth, send recipient)."""
|
||||
return bool(u and not u.is_demo and u.is_active and not u.is_suspended)
|
||||
|
||||
|
||||
def ensure_welcome(db: Session, user: User) -> None:
|
||||
@@ -221,7 +210,21 @@ def reset_keys(
|
||||
def public_key(
|
||||
user_id: int, user: User = Depends(require_human), db: Session = Depends(get_db)
|
||||
) -> dict:
|
||||
"""A member's public key, for deriving the pairwise conversation key."""
|
||||
"""A member's public key, for deriving the pairwise conversation key. MB2 (like get_thread):
|
||||
resolve the target only if they're messageable OR we already share a thread, else the same
|
||||
"User not found" as a non-existent id — so this can't be probed to enumerate user_ids or to
|
||||
fetch keys for suspended/deactivated/demo accounts."""
|
||||
cond = and_(
|
||||
Message.kind == "user",
|
||||
or_(
|
||||
and_(Message.sender_id == user.id, Message.recipient_id == user_id),
|
||||
and_(Message.sender_id == user_id, Message.recipient_id == user.id),
|
||||
),
|
||||
)
|
||||
p = db.get(User, user_id)
|
||||
has_history = db.scalar(select(Message.id).where(cond).limit(1))
|
||||
if p is None or (not is_messageable_user(p) and not has_history):
|
||||
raise HTTPException(status_code=404, detail="User not found")
|
||||
k = db.get(MessageKey, user_id)
|
||||
if k is None:
|
||||
raise HTTPException(status_code=404, detail="That member hasn't set up messaging yet.")
|
||||
@@ -240,7 +243,7 @@ def list_recipients(
|
||||
db.execute(
|
||||
select(User)
|
||||
.join(MessageKey, MessageKey.user_id == User.id)
|
||||
.where(User.id != user.id, *_messageable())
|
||||
.where(User.id != user.id, *messageable_clauses())
|
||||
.order_by(func.lower(func.coalesce(User.display_name, User.email)))
|
||||
)
|
||||
.scalars()
|
||||
|
||||
@@ -97,7 +97,7 @@ def watch_unlock(token: str, payload: dict, db: Session = Depends(get_db)) -> di
|
||||
job, asset = _ready_target(db, link)
|
||||
link.view_count += 1
|
||||
db.commit()
|
||||
grant = linksmod.make_grant(token)
|
||||
grant = linksmod.make_grant(token, link.password_version)
|
||||
return {
|
||||
"needs_password": False,
|
||||
**linksmod.public_meta(
|
||||
@@ -111,7 +111,7 @@ def watch_file(token: str, g: str | None = None, db: Session = Depends(get_db)):
|
||||
"""Range-aware stream of the shared file. Inline by default (plays in the page); an
|
||||
`allow_download` link serves it as an attachment. Password links require a valid grant."""
|
||||
link = _link_or_404(db, token)
|
||||
if link.password_hash and not linksmod.check_grant(token, g):
|
||||
if link.password_hash and not linksmod.check_grant(token, g, link.password_version):
|
||||
raise HTTPException(status_code=403, detail="This link needs a password.")
|
||||
asset = _ready_asset(db, link)
|
||||
path = storage.safe_abs_path(settings.download_root, asset.rel_path)
|
||||
@@ -129,7 +129,7 @@ def watch_poster(token: str, g: str | None = None, db: Session = Depends(get_db)
|
||||
"""The generated poster frame for a shared thumbnail-less video (used as the page/video poster
|
||||
and the link-preview image). Password links require a valid grant, like the file."""
|
||||
link = _link_or_404(db, token)
|
||||
if link.password_hash and not linksmod.check_grant(token, g):
|
||||
if link.password_hash and not linksmod.check_grant(token, g, link.password_version):
|
||||
raise HTTPException(status_code=403, detail="This link needs a password.")
|
||||
job, asset = _ready_target(db, link)
|
||||
if not asset.poster_path:
|
||||
|
||||
Reference in New Issue
Block a user