From 887b4e546cb8042715c2c316e7b2cd9938557d6e Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sun, 4 Oct 2026 02:14:04 +0200 Subject: [PATCH] fix(feed): a malformed saved view cannot break the autotag cleanup, and a foreign private tag reads as missing E32 review round 1 (B5, B6). - The autotag orphan cleanup reads every user's feed views. A view whose tag pick was not a list ("tags": 5) raised for everyone's job, True passed as tag id 1, and ~66k ids pushed the NOT IN past psycopg's 65535 bind parameters. The views are now read defensively and the kept ids filtered in Python; the saved-views route holds filters.tags to a list of at most 500 ids, on create and on update. - tags_by_category resolved any tag id, so another user's private tag landed in its real category and the feed summary answered differently than for an id that does not exist, which told the caller which foreign ids exist. It is owner-scoped now (pre-existing; S2's missing-tag branch made the difference visible). --- backend/app/feedquery.py | 16 +++++++---- backend/app/routes/saved_views.py | 23 +++++++++++++++- backend/app/sync/autotag.py | 35 ++++++++++++++---------- backend/tests/test_autotag_orphans.py | 28 +++++++++++++++++++ backend/tests/test_feed_views.py | 22 +++++++++++++++ backend/tests/test_saved_views_source.py | 15 ++++++++++ 6 files changed, 118 insertions(+), 21 deletions(-) diff --git a/backend/app/feedquery.py b/backend/app/feedquery.py index 6e6d8714..aa84af0e 100644 --- a/backend/app/feedquery.py +++ b/backend/app/feedquery.py @@ -113,15 +113,19 @@ def _tag_visible(user: User): MISSING_TAG_CATEGORY = "missing" -def tags_by_category(db: Session, tag_ids: list[int]) -> dict[str, list[int]]: +def tags_by_category(db: Session, user: User, tag_ids: list[int]) -> dict[str, list[int]]: """Group tag ids by their category (language/topic/other). The single source for the category split shared by the feed's tag WHERE (filtered_query) and the facet/count summary (feed_summary), so the two can't disagree on which selected tag belongs to which category. An unknown id lands in - MISSING_TAG_CATEGORY.""" + MISSING_TAG_CATEGORY — and so does another user's private tag: resolving it to its real category + told the caller which foreign ids exist (E32 review).""" if not tag_ids: return {} # skip the pointless `WHERE id IN ()` round-trip on the no-tag-filter hot path out: dict[str, list[int]] = {} - for tid, cat in db.execute(select(Tag.id, Tag.category).where(Tag.id.in_(tag_ids))).all(): + visible = or_(Tag.user_id.is_(None), Tag.user_id == user.id) + for tid, cat in db.execute( + select(Tag.id, Tag.category).where(Tag.id.in_(tag_ids), visible) + ).all(): out.setdefault(cat, []).append(tid) known = {tid for ids in out.values() for tid in ids} if missing := [tid for tid in tag_ids if tid not in known]: @@ -315,7 +319,7 @@ def filtered_query( # ALL its selected tags. ⚠️ feed_summary.passes() reimplements this exact rule in Python for the # sidebar count/facets — keep the two in lockstep (equivalence-tested by # test_feed_summary_matches_count_plus_facets). - by_category = tags_by_category(db, tags) + by_category = tags_by_category(db, user, tags) # Facet counting drops the category being counted so its own chips don't zero each # other out (standard drill-down faceting: a category's count ignores its own # selections but still honours the other categories' filters). @@ -406,7 +410,7 @@ def feed_summary(db: Session, user: User, **params) -> dict: return {"count": 0, "facets": {}} # channel -> its visible tag ids, plus each tag's category (for grouping the facet output). - visible = or_(ChannelTag.user_id.is_(None), ChannelTag.user_id == user.id) + visible = _tag_visible(user) channel_tags: dict[str, set[int]] = {} tag_category: dict[int, str] = {} for cid, tid, cat in db.execute( @@ -421,7 +425,7 @@ def feed_summary(db: Session, user: User, **params) -> dict: # base channel carries still filters everything out for its category — matching the SQL `IN (empty)` # behaviour). selected_by_cat: dict[str, set[int]] = { - cat: set(ids) for cat, ids in tags_by_category(db, params.get("tags") or []).items() + cat: set(ids) for cat, ids in tags_by_category(db, user, params.get("tags") or []).items() } def passes(ch_tags: set[int], exclude_cat: str | None) -> bool: diff --git a/backend/app/routes/saved_views.py b/backend/app/routes/saved_views.py index c400cd0a..c4691d73 100644 --- a/backend/app/routes/saved_views.py +++ b/backend/app/routes/saved_views.py @@ -1,5 +1,5 @@ from fastapi import APIRouter, Depends -from pydantic import BaseModel +from pydantic import BaseModel, field_validator from sqlalchemy import func, select, update from sqlalchemy.exc import IntegrityError from sqlalchemy.orm import Session @@ -53,11 +53,30 @@ def list_views( return [_serialize(v) for v in rows] +# A feed view's tag pick is read by more than the feed: the autotag job keeps every tag a view names +# (sync/autotag.py). Held here to the shape the feed sends — a list of tag ids, at most this many — so +# a hand-made body (`"tags": 5`, or 60k ids) can't break that job for every user (E32 review). +_MAX_VIEW_TAGS = 500 + + +def _check_filters(filters: dict | None) -> dict | None: + tags = (filters or {}).get("tags") + if tags is None: + return filters + if not isinstance(tags, list) or len(tags) > _MAX_VIEW_TAGS: + raise ValueError(f"filters.tags must be a list of at most {_MAX_VIEW_TAGS} tag ids") + if any(type(t) is not int for t in tags): # `type`, not isinstance: True is not a tag id + raise ValueError("filters.tags must hold tag ids") + return filters + + class CreateViewIn(BaseModel): name: str filters: dict source: str = "feed" + _filters = field_validator("filters")(_check_filters) + @router.post("") def create_view( @@ -98,6 +117,8 @@ class UpdateViewIn(BaseModel): filters: dict | None = None is_default: bool | None = None + _filters = field_validator("filters")(_check_filters) + @router.patch("/{view_id}") def update_view( diff --git a/backend/app/sync/autotag.py b/backend/app/sync/autotag.py index 5ce0ba07..7f3aa788 100644 --- a/backend/app/sync/autotag.py +++ b/backend/app/sync/autotag.py @@ -288,28 +288,35 @@ def run_autotag_all(db: Session, only_missing: bool = False) -> dict: return {"channels_tagged": tagged, "orphan_tags_removed": removed} +def _view_tag_ids(filters: object) -> list[int]: + """The tag ids a stored view's filters name; anything else in that slot names none.""" + tags = filters.get("tags") if isinstance(filters, dict) else None + if not isinstance(tags, list): + return [] + return [t for t in tags if type(t) is int] # `type`, not isinstance: True is not a tag id + + def _cleanup_orphan_system_tags(db: Session) -> int: """Delete system tags that no longer link to any channel (e.g. after a mapping change) — but not one a saved feed view filters on. Views hold tag IDS, and a deleted tag comes back under a new id when a channel earns it again, so the view's filter would point at nothing; the feed drops an - unknown id, and the view showed the whole feed under its old name (E32 S2).""" + unknown id, and the view showed the whole feed under its old name (E32 S2). + + The views are read defensively, and the kept ids are filtered here rather than in SQL: a view + stored before its route checked the shape (or written around it) must not raise for everyone's + job, nor push a `NOT IN` past the driver's bind-parameter limit (E32 review).""" in_views = { t for (filters,) in db.execute(select(SavedView.filters).where(SavedView.source == "feed")) - for t in ((filters or {}).get("tags") or []) - if isinstance(t, int) + for t in _view_tag_ids(filters) } - orphans = ( - db.execute( - select(Tag).where( - Tag.user_id.is_(None), - ~exists().where(ChannelTag.tag_id == Tag.id), - Tag.id.notin_(in_views), - ) - ) - .scalars() - .all() - ) + orphans = [ + tag + for tag in db.execute( + select(Tag).where(Tag.user_id.is_(None), ~exists().where(ChannelTag.tag_id == Tag.id)) + ).scalars() + if tag.id not in in_views + ] for tag in orphans: db.delete(tag) db.commit() diff --git a/backend/tests/test_autotag_orphans.py b/backend/tests/test_autotag_orphans.py index ef2b2759..a9eb5605 100644 --- a/backend/tests/test_autotag_orphans.py +++ b/backend/tests/test_autotag_orphans.py @@ -34,3 +34,31 @@ def test_an_orphan_system_tag_goes_but_one_a_saved_view_filters_on_stays(db, mak db.expire_all() names = set(db.execute(select(Tag.name)).scalars()) assert names == {"Gaming", "Music", "Mine"} # only the unreferenced system orphan went + + +def test_a_malformed_saved_view_cannot_break_the_cleanup(db, make_user): + """A view stored around the route's shape check (or before it) must not raise for everyone's job + — `"tags": 5` raised TypeError — and `True` is not tag id 1 (E32 review).""" + user = make_user() + orphan = Tag(user_id=None, name="Cooking", category="topic") + db.add(orphan) + db.flush() + for i, filters in enumerate( + ({"tags": 5}, {"tags": [True]}, ["not", "a", "dict"], {"tags": None}) + ): + db.add(SavedView(user_id=user.id, source="feed", name=f"bad{i}", filters=filters)) + db.commit() + assert autotag._cleanup_orphan_system_tags(db) == 1 + db.expire_all() + assert db.execute(select(Tag).where(Tag.name == "Cooking")).first() is None + + +def test_a_view_kept_tag_list_larger_than_a_bind_parameter_limit_still_cleans_up(db, make_user): + user = make_user() + orphan = Tag(user_id=None, name="Cooking", category="topic") + db.add(orphan) + db.flush() + huge = list(range(10_000_000, 10_070_000)) # past psycopg's 65535 bind parameters + db.add(SavedView(user_id=user.id, source="feed", name="huge", filters={"tags": huge})) + db.commit() + assert autotag._cleanup_orphan_system_tags(db) == 1 diff --git a/backend/tests/test_feed_views.py b/backend/tests/test_feed_views.py index 9b647160..ec8d8c78 100644 --- a/backend/tests/test_feed_views.py +++ b/backend/tests/test_feed_views.py @@ -12,6 +12,8 @@ from datetime import datetime, timezone import pytest +from app import feedquery + from app.models import Channel, ChannelTag, Subscription, Tag, Video, VideoState @@ -192,6 +194,26 @@ def test_a_deleted_tags_id_filters_to_nothing_not_to_everything(db, as_user, mak assert r.json()["items"] == [], qs +def test_another_users_private_tag_answers_like_one_that_does_not_exist(db, as_user, make_user): + """The category split resolved any tag id, so a foreign PRIVATE tag landed in its real category + and the summary's facets differed from a non-existent id's: an oracle for which foreign ids exist + (E32 review). Both must now read as "missing".""" + user, tag_ids = _seed_tagged(db, make_user) + stranger = make_user(email="stranger@example.com") + private = Tag(user_id=stranger.id, name="Secret", category="language") + db.add(private) + db.commit() + assert feedquery.tags_by_category(db, user, [tag_ids["L1"], private.id]) == { + "language": [tag_ids["L1"]], + feedquery.MISSING_TAG_CATEGORY: [private.id], + } + client = as_user(user) + foreign = client.get(f"/api/feed/summary?show=all&tags={tag_ids['T1']}&tags={private.id}") + absent = client.get(f"/api/feed/summary?show=all&tags={tag_ids['T1']}&tags=987654") + assert foreign.status_code == absent.status_code == 200 + assert foreign.json() == absent.json() + + def test_untagged_filter_keeps_only_channels_the_user_has_not_tagged(db, as_user, make_user): """The "No tag" filter (item B): `untagged=true` keeps only channels with NO tag the USER personally attached (no user-owned ChannelTag). System/auto tags (user_id NULL) don't count, and another user's diff --git a/backend/tests/test_saved_views_source.py b/backend/tests/test_saved_views_source.py index f7e078e3..a45938b5 100644 --- a/backend/tests/test_saved_views_source.py +++ b/backend/tests/test_saved_views_source.py @@ -67,3 +67,18 @@ def test_duplicate_name_within_a_source_conflicts(db, monkeypatch, override_dep) b = c.post("/api/saved-views", json={"name": "Dup", "filters": {}, "source": "plex"}) assert a.status_code == 200 assert b.status_code == 409 # same (user, source, name) + + +def test_a_feed_views_tag_pick_must_be_a_short_list_of_ids(db, monkeypatch, override_dep): + """The autotag job reads every view's tag pick (E32 review): the route holds it to the shape the + feed sends, on create and on update.""" + user = _user(db) + c = _client(db, user, monkeypatch, override_dep) + for bad in (5, "1", [1, "2"], [True], list(range(501))): + r = c.post("/api/saved-views", json={"name": "Bad", "filters": {"tags": bad}}) + assert r.status_code == 422, (bad, r.text) + ok = c.post("/api/saved-views", json={"name": "Ok", "filters": {"tags": [1, 2]}}) + assert ok.status_code == 200, ok.text + vid = ok.json()["id"] + assert c.patch(f"/api/saved-views/{vid}", json={"filters": {"tags": 5}}).status_code == 422 + assert c.patch(f"/api/saved-views/{vid}", json={"filters": {"tags": []}}).status_code == 200