From 995a493bd3d9012e7c066d39ce160498756999a8 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Fri, 6 Nov 2026 04:11:36 +0100 Subject: [PATCH] fix(playlists): the push side spends its own allowance, so a scripted loop cannot drain the shared YouTube budget push-plan reads the live list and push reads and writes it, both against the instance-wide budget, and neither had a limit. They now share one per-user allowance of 20 calls per 10 minutes, far above a person's plan-then-push, refused as playlist/push_cooldown with a sentence in both languages. --- backend/app/routes/playlists.py | 20 ++++++- backend/tests/test_abuse_limits.py | 64 +++++++++++++++++++++ frontend/src/i18n/locales/en/playlists.json | 1 + frontend/src/i18n/locales/hu/playlists.json | 1 + 4 files changed, 83 insertions(+), 3 deletions(-) diff --git a/backend/app/routes/playlists.py b/backend/app/routes/playlists.py index 5622dd2f..79ac345a 100644 --- a/backend/app/routes/playlists.py +++ b/backend/app/routes/playlists.py @@ -41,11 +41,16 @@ router = APIRouter(prefix="/api/playlists", tags=["playlists"]) # It guards EVERY route that spends under QuotaAction.PLAYLISTS_PULL, not just the bulk one: a # scripted loop over the single-playlist re-pull drains the same shared budget, and limiting one of # two routes with the same quota action is the asymmetric fix this codebase keeps re-learning. -# Deliberately NOT on the push side (`push-plan` / `push`): those run on every push attempt, -# including the no-op "already up to date" path, so a 3-per-10-minute allowance would refuse -# legitimate work. Their exposure is tracked as SEC-18 instead. _playlist_pull_limiter = RateLimiter(max_events=3, window_seconds=600) +# SEC-18, the push side: `push-plan` reads the live list (1 unit a page) and `push` reads and writes +# it, both against the same instance-wide budget. They get a budget of their own, counted in calls +# and far wider than the pull's: a press in the UI is one plan and, unless nothing changed, one push, +# so no person reaches it, while a scripted loop stops at it. A big push is not this limit's job — +# `push` already refuses one the day's remaining budget cannot cover. +PUSH_ALLOWANCE = 20 +_playlist_push_limiter = RateLimiter(max_events=PUSH_ALLOWANCE, window_seconds=600) + def _check_pull_cooldown(user: User) -> None: """Shared gate so the two PLAYLISTS_PULL routes cannot drift apart again.""" @@ -58,6 +63,7 @@ def _check_pull_cooldown(user: User) -> None: PlaylistRefusal = Literal[ "name_required", "not_on_youtube", + "push_cooldown", "quota_exceeded", "sync_all_busy", "sync_busy", @@ -244,6 +250,12 @@ def revert_youtube( raise youtube_refusal(exc, playlist=True) from exc +def _check_push_cooldown(user: User) -> None: + """Shared gate of the two push routes, so they spend one allowance and cannot drift apart.""" + if not _playlist_push_limiter.allow(str(user.id)): + raise _refused(429, "push_cooldown") + + def _pushable(db: Session, user: User, playlist_id: int) -> "Playlist": """A local playlist the user may push to YouTube (create or update). The built-in Watch later and read-only YouTube mirrors are never pushable.""" @@ -262,6 +274,7 @@ def push_plan( """Dry run: what a two-way 'Sync to YouTube' merge would do (what each side gains/loses) + the quota estimate, so the UI can show a confirm. For a linked playlist this reads the live YouTube state (cheap, 1 unit/page). Write scope is required — syncing is a YouTube write.""" + _check_push_cooldown(user) pl = _pushable(db, user, playlist_id) try: with quota.attribute(user.id, quota.QuotaAction.PLAYLISTS_PUSH): @@ -282,6 +295,7 @@ def push( """Two-way sync a local playlist with YouTube (create it on first push, else 3-way merge so neither side loses an item). Refuses if the estimated quota exceeds what's left today, so a big sync can't strand half-applied. Write scope required (require_write_scope).""" + _check_push_cooldown(user) pl = _pushable(db, user, playlist_id) # One sync of a list at a time, across tabs, devices and the scheduler (`sync_claim`): the # client's own in-flight flag only covers the menus of one page. diff --git a/backend/tests/test_abuse_limits.py b/backend/tests/test_abuse_limits.py index 4333d269..bcdc499b 100644 --- a/backend/tests/test_abuse_limits.py +++ b/backend/tests/test_abuse_limits.py @@ -125,3 +125,67 @@ def test_the_single_playlist_repull_shares_the_pull_cooldown(db, as_user, make_u r = c.post("/api/playlists/sync-youtube") assert r.status_code == 429 assert r.json()["detail"] == {"code": "sync_cooldown"} + + +def _pushable_list(db, make_user, monkeypatch, plroutes): + """A local (never exported) list and a user with the write scope; the merge itself is stubbed — + the limiter is under test, not the YouTube round trip.""" + from app import auth + from app.reader.models import Playlist + + user = make_user() + pl = Playlist(user_id=user.id, name="Local", source="local") + db.add(pl) + db.commit() + monkeypatch.setattr(auth, "has_write_scope", lambda u: True) + monkeypatch.setattr(plroutes, "plan_merge", lambda d, u, p, live=None: {"units_estimate": 0}) + monkeypatch.setattr(plroutes, "merge_playlist", lambda d, u, p, live=None: {"added": 0}) + return user, pl + + +def test_the_push_side_goes_on_cooldown_after_its_allowance(db, as_user, make_user, monkeypatch): + # SEC-18: push-plan reads the live list and push writes it, both against the instance-wide + # YouTube budget. A press in the UI is one plan + one push, so the allowance is generous — but a + # scripted loop stops at it, with a reason the UI can say. + from app.routes import playlists as plroutes + + user, pl = _pushable_list(db, make_user, monkeypatch, plroutes) + c = as_user(user) + allowance = plroutes.PUSH_ALLOWANCE + codes = [c.get(f"/api/playlists/{pl.id}/push-plan").status_code for _ in range(allowance)] + assert codes == [200] * allowance + r = c.get(f"/api/playlists/{pl.id}/push-plan") + assert r.status_code == 429 + assert r.json()["detail"] == {"code": "playlist", "reason": "push_cooldown"} + + +def test_plan_and_push_draw_on_one_allowance(db, as_user, make_user, monkeypatch): + # The asymmetric fix again, avoided up front: a limit on one of the two routes leaves the same + # drain open next door. + from app.routes import playlists as plroutes + + user, pl = _pushable_list(db, make_user, monkeypatch, plroutes) + c = as_user(user) + for i in range(plroutes.PUSH_ALLOWANCE): + if i % 2: + r = c.get(f"/api/playlists/{pl.id}/push-plan") + else: + r = c.post(f"/api/playlists/{pl.id}/push") + assert r.status_code == 200, (i, r.json()) + r = c.post(f"/api/playlists/{pl.id}/push") + assert r.status_code == 429 + assert r.json()["detail"] == {"code": "playlist", "reason": "push_cooldown"} + + +def test_the_push_cooldown_is_per_user(db, as_user, make_user, monkeypatch): + from app.routes import playlists as plroutes + + a, pa = _pushable_list(db, make_user, monkeypatch, plroutes) + b, pb = _pushable_list(db, make_user, monkeypatch, plroutes) + ca = as_user(a) + codes = [ + ca.get(f"/api/playlists/{pa.id}/push-plan").status_code + for _ in range(plroutes.PUSH_ALLOWANCE + 1) + ] + assert codes == [200] * plroutes.PUSH_ALLOWANCE + [429] + assert as_user(b).get(f"/api/playlists/{pb.id}/push-plan").status_code == 200 diff --git a/frontend/src/i18n/locales/en/playlists.json b/frontend/src/i18n/locales/en/playlists.json index 619e565a..dd7574e4 100644 --- a/frontend/src/i18n/locales/en/playlists.json +++ b/frontend/src/i18n/locales/en/playlists.json @@ -89,6 +89,7 @@ "watch_later_not_pushable": "The Watch later list can't be sent to YouTube", "watch_later_undeletable": "The Watch later list can't be deleted", "youtube_editing_off": "Enable YouTube editing in Settings first", + "push_cooldown": "You've synced to YouTube many times just now — give it a few minutes", "quota_exceeded": "Not enough YouTube quota left today for this sync", "sync_busy": "This playlist is already syncing with YouTube — try again when it finishes", "sync_all_busy": "Your playlists are already syncing with YouTube — try again when it finishes" diff --git a/frontend/src/i18n/locales/hu/playlists.json b/frontend/src/i18n/locales/hu/playlists.json index 6c7c4aab..2b84fbbf 100644 --- a/frontend/src/i18n/locales/hu/playlists.json +++ b/frontend/src/i18n/locales/hu/playlists.json @@ -89,6 +89,7 @@ "watch_later_not_pushable": "A „Megnézem később” listát nem lehet a YouTube-ra küldeni", "watch_later_undeletable": "A „Megnézem később” lista nem törölhető", "youtube_editing_off": "Előbb engedélyezd a YouTube-szerkesztést a Beállításokban", + "push_cooldown": "Az imént sokszor szinkronizáltál a YouTube-ra — várj pár percet", "quota_exceeded": "Mára nem maradt elég YouTube-keret ehhez a szinkronhoz", "sync_busy": "Ez a lista épp szinkronizál a YouTube-bal — próbáld újra, ha végzett", "sync_all_busy": "A listáid épp szinkronizálnak a YouTube-bal — próbáld újra, ha végzett"