From e70fe545cc85a6a3032481600793362f3a5d26e1 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Tue, 2 Jun 2026 18:44:32 -0400 Subject: [PATCH] =?UTF-8?q?fix(trash):=20owner-scope=20all=20trash=20ops?= =?UTF-8?q?=20=E2=80=94=20close=20cross-tenant=20IDOR/disclosure?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Drift-audit Group 1 (authz/IDOR). Multi-user is live, so these were exploitable ACL bypasses: - trash.py: add _owner_clause() and apply it to _exists_alive, restore, purge, list_trash, and purge_expired. A batch_id is a bearer token; without an owner predicate a leaked/guessed id let one tenant read (list_trash), restore, or PERMANENTLY purge another's content. Topics and rules carried no owner check at all (_OWNER mapped them to None) — ownership now derives through the parent rulebook (or owning project, for project-scoped rules). - purge_expired is now per-user; trash_scheduler iterates every user and applies that user's own trash_retention_days window, instead of applying user 1's window to everyone (early data loss for other users). - rulebooks subscribe/unsubscribe_project now assert project ownership, matching the suppression endpoints. - topic/rule DELETE routes return 404 when nothing owned was removed. Regression test locks in that every model — including topics/rules — gets a real owner clause. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/fabledassistant/routes/rulebooks.py | 6 +- src/fabledassistant/services/rulebooks.py | 2 + src/fabledassistant/services/trash.py | 80 ++++++++++++++----- .../services/trash_scheduler.py | 28 ++++--- tests/test_services_trash.py | 27 ++++++- 5 files changed, 109 insertions(+), 34 deletions(-) diff --git a/src/fabledassistant/routes/rulebooks.py b/src/fabledassistant/routes/rulebooks.py index 4f63d76..5f7c394 100644 --- a/src/fabledassistant/routes/rulebooks.py +++ b/src/fabledassistant/routes/rulebooks.py @@ -118,7 +118,8 @@ async def update_topic(topic_id: int): @rulebooks_bp.delete("/rulebook-topics/") @login_required async def delete_topic(topic_id: int): - await trash_delete(_uid(), "topic", topic_id) + if await trash_delete(_uid(), "topic", topic_id) is None: + return jsonify({"error": "topic not found"}), 404 return "", 204 @@ -196,7 +197,8 @@ async def update_rule(rule_id: int): @rulebooks_bp.delete("/rules/") @login_required async def delete_rule(rule_id: int): - await trash_delete(_uid(), "rule", rule_id) + if await trash_delete(_uid(), "rule", rule_id) is None: + return jsonify({"error": "rule not found"}), 404 return "", 204 diff --git a/src/fabledassistant/services/rulebooks.py b/src/fabledassistant/services/rulebooks.py index 658f6f5..d8f1070 100644 --- a/src/fabledassistant/services/rulebooks.py +++ b/src/fabledassistant/services/rulebooks.py @@ -498,6 +498,7 @@ async def subscribe_project( from fabledassistant.models.rulebook import project_rulebook_subscriptions async with async_session() as session: + await _assert_project_owned(session, project_id, user_id) await _assert_rulebook_owned(session, rulebook_id, user_id) # ON CONFLICT DO NOTHING via try/except to keep dialect-agnostic. try: @@ -517,6 +518,7 @@ async def unsubscribe_project( from fabledassistant.models.rulebook import project_rulebook_subscriptions async with async_session() as session: + await _assert_project_owned(session, project_id, user_id) await _assert_rulebook_owned(session, rulebook_id, user_id) await session.execute( sql_delete(project_rulebook_subscriptions).where( diff --git a/src/fabledassistant/services/trash.py b/src/fabledassistant/services/trash.py index 92ab824..b7996f0 100644 --- a/src/fabledassistant/services/trash.py +++ b/src/fabledassistant/services/trash.py @@ -10,7 +10,7 @@ from __future__ import annotations import uuid from datetime import datetime, timezone -from sqlalchemy import select, update +from sqlalchemy import or_, select, update from fabledassistant.models import async_session from fabledassistant.models.note import Note @@ -19,19 +19,50 @@ from fabledassistant.models.project import Project from fabledassistant.models.milestone import Milestone from fabledassistant.models.rulebook import Rulebook, RulebookTopic, Rule -# entity_type -> (Model, owner_column_name or None). Used by the existence check. -_OWNER = { - "note": (Note, "user_id"), - "task": (Note, "user_id"), - "event": (Event, "user_id"), - "project": (Project, "user_id"), - "milestone": (Milestone, "user_id"), - "rulebook": (Rulebook, "owner_user_id"), - "topic": (RulebookTopic, None), - "rule": (Rule, None), +# entity_type -> Model. Used to resolve which table a trash op targets. +_MODEL_FOR = { + "note": Note, + "task": Note, + "event": Event, + "project": Project, + "milestone": Milestone, + "rulebook": Rulebook, + "topic": RulebookTopic, + "rule": Rule, } +def _owner_clause(model, user_id: int): + """Boolean expr scoping `model` rows to the ones `user_id` owns. + + EVERY trash query (exists-check, restore, purge, list, retention sweep) + must carry this — a batch_id is a bearer token, so without an owner + predicate a leaked/guessed id lets one tenant read, restore, or + permanently destroy another's content. Topics and rules carry no + user_id of their own; ownership is derived through the parent rulebook + (or, for project-scoped rules, the owning project). + """ + if model is Rulebook: + return Rulebook.owner_user_id == user_id + if model is RulebookTopic: + return RulebookTopic.rulebook_id.in_( + select(Rulebook.id).where(Rulebook.owner_user_id == user_id) + ) + if model is Rule: + return or_( + Rule.topic_id.in_( + select(RulebookTopic.id) + .join(Rulebook, RulebookTopic.rulebook_id == Rulebook.id) + .where(Rulebook.owner_user_id == user_id) + ), + Rule.project_id.in_( + select(Project.id).where(Project.user_id == user_id) + ), + ) + # Note, Event, Project, Milestone all carry user_id directly. + return model.user_id == user_id + + async def _set(session, model, where, batch, now) -> None: """Stamp deleted_at + batch on live rows matching `where`.""" await session.execute( @@ -42,10 +73,8 @@ async def _set(session, model, where, batch, now) -> None: async def _exists_alive(session, user_id: int, etype: str, eid: int) -> bool: - model, owner = _OWNER[etype] - where = [model.id == eid, model.deleted_at.is_(None)] - if owner: - where.append(getattr(model, owner) == user_id) + model = _MODEL_FOR[etype] + where = [model.id == eid, model.deleted_at.is_(None), _owner_clause(model, user_id)] return (await session.execute(select(model.id).where(*where))).first() is not None @@ -135,7 +164,7 @@ async def restore(user_id: int, batch_id: str) -> int: for model in _ALL: res = await session.execute( update(model) - .where(model.deleted_batch_id == batch_id) + .where(model.deleted_batch_id == batch_id, _owner_clause(model, user_id)) .values(deleted_at=None, deleted_batch_id=None) ) n += res.rowcount or 0 @@ -150,7 +179,9 @@ async def purge(user_id: int, batch_id: str) -> int: async with async_session() as session: for model in _ALL: res = await session.execute( - sql_delete(model).where(model.deleted_batch_id == batch_id) + sql_delete(model).where( + model.deleted_batch_id == batch_id, _owner_clause(model, user_id) + ) ) n += res.rowcount or 0 await session.commit() @@ -163,7 +194,9 @@ async def list_trash(user_id: int) -> list[dict]: async with async_session() as session: for model in _ALL: rows = (await session.execute( - select(model).where(model.deleted_at.isnot(None)) + select(model).where( + model.deleted_at.isnot(None), _owner_clause(model, user_id) + ) )).scalars().all() for r in rows: grp = batches.setdefault( @@ -185,9 +218,12 @@ async def list_trash(user_id: int) -> list[dict]: return out -async def purge_expired(retention_days: int) -> int: - """Cron entry: hard-delete rows trashed more than retention_days ago. +async def purge_expired(user_id: int, retention_days: int) -> int: + """Cron entry: hard-delete THIS user's rows trashed more than retention_days ago. + Scoped to one owner so the scheduler can apply each user's own + `trash_retention_days` window — a single global sweep would let one + user's short window prematurely destroy another's data. retention_days <= 0 disables auto-purge (returns 0 without touching anything). """ from datetime import timedelta @@ -200,7 +236,9 @@ async def purge_expired(retention_days: int) -> int: for model in _ALL: res = await session.execute( sql_delete(model).where( - model.deleted_at.isnot(None), model.deleted_at < cutoff + model.deleted_at.isnot(None), + model.deleted_at < cutoff, + _owner_clause(model, user_id), ) ) n += res.rowcount or 0 diff --git a/src/fabledassistant/services/trash_scheduler.py b/src/fabledassistant/services/trash_scheduler.py index 4273ca8..12a5ad8 100644 --- a/src/fabledassistant/services/trash_scheduler.py +++ b/src/fabledassistant/services/trash_scheduler.py @@ -1,9 +1,9 @@ """Daily APScheduler cron that purges expired trash. Mirrors version_pinning_scheduler.py: a single global BackgroundScheduler job -at 03:30 UTC bridges into the asyncio loop to run the async purge. Reads the -operator's `trash_retention_days` setting (single-tenant: user 1); 0 disables -auto-purge. +at 03:30 UTC bridges into the asyncio loop to run the async purge. Iterates +every user and applies that user's own `trash_retention_days` setting; 0 +disables auto-purge for that user. """ from __future__ import annotations @@ -30,12 +30,22 @@ def _run_purge_threadsafe() -> None: async def _runner(): try: - raw = await get_setting(1, "trash_retention_days", "90") - try: - days = int(raw) - except (TypeError, ValueError): - days = 90 - purged = await trash_svc.purge_expired(days) + from sqlalchemy import select + + from fabledassistant.models import async_session + from fabledassistant.models.user import User + + async with async_session() as session: + user_ids = (await session.execute(select(User.id))).scalars().all() + + purged = 0 + for uid in user_ids: + raw = await get_setting(uid, "trash_retention_days", "90") + try: + days = int(raw) + except (TypeError, ValueError): + days = 90 + purged += await trash_svc.purge_expired(uid, days) if purged: logger.info("trash purge: removed %d expired row(s)", purged) else: diff --git a/tests/test_services_trash.py b/tests/test_services_trash.py index 0586ec9..fd7f1f8 100644 --- a/tests/test_services_trash.py +++ b/tests/test_services_trash.py @@ -112,7 +112,7 @@ async def test_purge_expired_skips_when_retention_zero(): with patch("fabledassistant.services.trash.async_session") as cls: cls.return_value = session from fabledassistant.services.trash import purge_expired - n = await purge_expired(0) + n = await purge_expired(1, 0) assert n == 0 assert not session.execute.called # never opens a delete @@ -124,11 +124,34 @@ async def test_purge_expired_deletes_across_models_when_positive(): with patch("fabledassistant.services.trash.async_session") as cls: cls.return_value = session from fabledassistant.services.trash import purge_expired - n = await purge_expired(90) + n = await purge_expired(1, 90) assert n == 7 assert session.execute.await_count == 7 +def test_owner_clause_scopes_every_model(): + """Regression: every trash op must owner-scope, including topics/rules + which previously had NO owner check (IDOR across tenants).""" + from fabledassistant.services.trash import _owner_clause, _MODEL_FOR + from fabledassistant.models.note import Note + from fabledassistant.models.rulebook import Rulebook, RulebookTopic, Rule + + # Models with a direct owner column. + assert "user_id" in str(_owner_clause(Note, 7)) + assert "owner_user_id" in str(_owner_clause(Rulebook, 7)) + + # Topics/rules carry no user_id — ownership is derived through the + # parent rulebook (and, for rules, the owning project). + topic_sql = str(_owner_clause(RulebookTopic, 7)) + assert "rulebooks" in topic_sql and "owner_user_id" in topic_sql + rule_sql = str(_owner_clause(Rule, 7)) + assert "owner_user_id" in rule_sql and "projects" in rule_sql + + # Every entity type resolves to a model that yields a non-empty clause. + for model in set(_MODEL_FOR.values()): + assert _owner_clause(model, 1) is not None + + @pytest.mark.asyncio async def test_list_trash_groups_by_batch(): session = _make_mock_session()