fix(feed): an out-of-range seed or cursor is refused, not a 500
A seed past a float overflowed the shuffle's start point, and a cursor holding Infinity or a 400-digit key overflowed its int/float read: both answered 500. The routes bound the seed (422), the start point folds it to 32 bits, and the cursor read counts an overflow as a bad cursor (400). A plan test holds that the walk reads ix_videos_shuffle_key with no sort over a realistic scope.
This commit is contained in:
@@ -719,8 +719,11 @@ _GOLDEN = 0.6180339887498949
|
|||||||
|
|
||||||
|
|
||||||
def shuffle_start(seed: int) -> float:
|
def shuffle_start(seed: int) -> float:
|
||||||
"""The point in [0, 1) where a seed's shuffle starts on the ring of `Video.shuffle_key`."""
|
"""The point in [0, 1) where a seed's shuffle starts on the ring of `Video.shuffle_key`.
|
||||||
return (seed * _GOLDEN) % 1.0
|
|
||||||
|
Folded to 32 bits first: an int past a float's range overflowed the product, and one past 2**53
|
||||||
|
left the product no fraction to land on. A seed the client draws (under a billion) is unchanged."""
|
||||||
|
return ((seed % 2**32) * _GOLDEN) % 1.0
|
||||||
|
|
||||||
|
|
||||||
def shuffle_walk(
|
def shuffle_walk(
|
||||||
@@ -787,7 +790,7 @@ def feed_page(
|
|||||||
if vals is not None and cursor_id is not None:
|
if vals is not None and cursor_id is not None:
|
||||||
try:
|
try:
|
||||||
after = (int(vals[0]), float(vals[1]), str(cursor_id))
|
after = (int(vals[0]), float(vals[1]), str(cursor_id))
|
||||||
except (TypeError, ValueError) as e:
|
except (TypeError, ValueError, OverflowError) as e: # Infinity, a 400-digit key
|
||||||
raise BadCursor("Invalid feed cursor") from e
|
raise BadCursor("Invalid feed cursor") from e
|
||||||
rows = shuffle_walk(db, query, seed, limit + 1, after)
|
rows = shuffle_walk(db, query, seed, limit + 1, after)
|
||||||
else:
|
else:
|
||||||
|
|||||||
@@ -6,9 +6,9 @@ and "fetch a row this user owns or 404" idioms have one definition, one status c
|
|||||||
one existence-leak policy.
|
one existence-leak policy.
|
||||||
"""
|
"""
|
||||||
|
|
||||||
from typing import Any, Protocol, TypeVar
|
from typing import Annotated, Any, Protocol, TypeVar
|
||||||
|
|
||||||
from fastapi import HTTPException, Request
|
from fastapi import HTTPException, Query, Request
|
||||||
from pydantic import BaseModel, ConfigDict, Field
|
from pydantic import BaseModel, ConfigDict, Field
|
||||||
from sqlalchemy.orm import Session
|
from sqlalchemy.orm import Session
|
||||||
|
|
||||||
@@ -21,6 +21,10 @@ from app.youtube.client import YouTubeError, refusal_reason
|
|||||||
|
|
||||||
M = TypeVar("M", bound=Base)
|
M = TypeVar("M", bound=Base)
|
||||||
|
|
||||||
|
# A shuffle / surprise seed off the query string. The client draws one under a billion; bounded so a
|
||||||
|
# hand-typed 400-digit one is a 422, not an int no float or bigint can hold further down.
|
||||||
|
Seed = Annotated[int, Query(ge=-(2**53), le=2**53)]
|
||||||
|
|
||||||
# Resume-position thresholds of a player that only keeps a resume point (the YouTube player, the
|
# Resume-position thresholds of a player that only keeps a resume point (the YouTube player, the
|
||||||
# library's): positions below this are "didn't really start" and within this of the end are
|
# library's): positions below this are "didn't really start" and within this of the end are
|
||||||
# "basically finished" — neither is worth storing, so the position is cleared instead (no progress
|
# "basically finished" — neither is worth storing, so the position is cleared instead (no progress
|
||||||
|
|||||||
@@ -23,7 +23,7 @@ from app.discover import pins, rails
|
|||||||
from app.discover.budget import Budget, DEFAULT_BUDGET
|
from app.discover.budget import Budget, DEFAULT_BUDGET
|
||||||
from app.models import User
|
from app.models import User
|
||||||
from app.plex.cards import norm_lang
|
from app.plex.cards import norm_lang
|
||||||
from app.routes._common import not_found
|
from app.routes._common import Seed, not_found
|
||||||
|
|
||||||
router = APIRouter(prefix="/api/discover", tags=["discover"])
|
router = APIRouter(prefix="/api/discover", tags=["discover"])
|
||||||
|
|
||||||
@@ -44,7 +44,7 @@ class ReorderIn(BaseModel):
|
|||||||
@router.get("")
|
@router.get("")
|
||||||
def get_discover(
|
def get_discover(
|
||||||
budget: Budget = DEFAULT_BUDGET,
|
budget: Budget = DEFAULT_BUDGET,
|
||||||
seed: int = 0,
|
seed: Seed = 0,
|
||||||
lang: str = "en",
|
lang: str = "en",
|
||||||
user: User = Depends(current_user),
|
user: User = Depends(current_user),
|
||||||
db: Session = Depends(get_db),
|
db: Session = Depends(get_db),
|
||||||
@@ -79,7 +79,7 @@ MAX_EXCLUDE = 32
|
|||||||
@router.get("/pick")
|
@router.get("/pick")
|
||||||
def get_pick(
|
def get_pick(
|
||||||
budget: Budget = DEFAULT_BUDGET,
|
budget: Budget = DEFAULT_BUDGET,
|
||||||
seed: int = 0,
|
seed: Seed = 0,
|
||||||
exclude: str = "",
|
exclude: str = "",
|
||||||
start: int = 0,
|
start: int = 0,
|
||||||
lang: str = "en",
|
lang: str = "en",
|
||||||
|
|||||||
@@ -20,6 +20,7 @@ from app.models import User
|
|||||||
from app.reader import queries, service, takeout
|
from app.reader import queries, service, takeout
|
||||||
from app.routes._common import (
|
from app.routes._common import (
|
||||||
ProgressIn,
|
ProgressIn,
|
||||||
|
Seed,
|
||||||
capped_body,
|
capped_body,
|
||||||
drain_body,
|
drain_body,
|
||||||
kept_position,
|
kept_position,
|
||||||
@@ -89,7 +90,7 @@ def _feed_params(
|
|||||||
def get_feed(
|
def get_feed(
|
||||||
params: dict = Depends(_feed_params),
|
params: dict = Depends(_feed_params),
|
||||||
sort: str = "newest",
|
sort: str = "newest",
|
||||||
seed: int = 0,
|
seed: Seed = 0,
|
||||||
limit: int = Query(default=60, le=200),
|
limit: int = Query(default=60, le=200),
|
||||||
cursor: str | None = None,
|
cursor: str | None = None,
|
||||||
user: User = Depends(current_user),
|
user: User = Depends(current_user),
|
||||||
|
|||||||
@@ -6,10 +6,12 @@ a window of it and shuffles that window by the seed (`discover.queries.yt_surpri
|
|||||||
scope by md5(id || seed) cost ~350 ms per call on prod and grew with every Shuffle page.
|
scope by md5(id || seed) cost ~350 ms per call on prod and grew with every Shuffle page.
|
||||||
"""
|
"""
|
||||||
|
|
||||||
|
import base64
|
||||||
|
import random
|
||||||
from contextlib import contextmanager
|
from contextlib import contextmanager
|
||||||
from datetime import datetime, timedelta, timezone
|
from datetime import datetime, timedelta, timezone
|
||||||
|
|
||||||
from sqlalchemy import event
|
from sqlalchemy import event, insert, text
|
||||||
|
|
||||||
from app import feedquery
|
from app import feedquery
|
||||||
from app.catalog.models import Channel, Subscription, Video
|
from app.catalog.models import Channel, Subscription, Video
|
||||||
@@ -125,6 +127,51 @@ def test_neither_draw_orders_the_scope_by_a_hash(db, make_user):
|
|||||||
assert "ORDER BY videos.shuffle_key, videos.id" in s.replace("\n", " "), s
|
assert "ORDER BY videos.shuffle_key, videos.id" in s.replace("\n", " "), s
|
||||||
|
|
||||||
|
|
||||||
|
def test_the_walk_reads_the_ring_index_and_sorts_nothing(db, make_user):
|
||||||
|
# The text of the query is not the plan: each arc must be served by ix_videos_shuffle_key in key
|
||||||
|
# order (an index on another shape, or none, leaves a Sort of the whole scope). A scope of a few
|
||||||
|
# thousand videos, analysed, so the planner weighs it as it does a real one.
|
||||||
|
user = _seed(db, make_user)
|
||||||
|
rng = random.Random(7)
|
||||||
|
db.execute(
|
||||||
|
insert(Video),
|
||||||
|
[
|
||||||
|
{
|
||||||
|
"id": f"bulk{i:05}",
|
||||||
|
"channel_id": "UCring",
|
||||||
|
"title": "b",
|
||||||
|
"duration_seconds": 600,
|
||||||
|
"published_at": NOW - timedelta(minutes=i),
|
||||||
|
"shuffle_key": rng.random(),
|
||||||
|
}
|
||||||
|
for i in range(4000)
|
||||||
|
],
|
||||||
|
)
|
||||||
|
db.commit()
|
||||||
|
db.execute(text("ANALYZE videos"))
|
||||||
|
query, _ = feedquery.filtered_query(db, user, **_FEED_BASE)
|
||||||
|
captured: list[tuple[str, object]] = []
|
||||||
|
|
||||||
|
def grab(conn, cursor, statement, params, *args):
|
||||||
|
if "FROM videos" in statement and "LIMIT" in statement:
|
||||||
|
captured.append((statement, params))
|
||||||
|
|
||||||
|
engine = db.get_bind()
|
||||||
|
event.listen(engine, "before_cursor_execute", grab)
|
||||||
|
try:
|
||||||
|
seed = _seed_landing_between(0.45, 0.55)
|
||||||
|
feedquery.shuffle_walk(db, query, seed, 6) # the first arc, from the seed's point
|
||||||
|
feedquery.shuffle_walk(db, query, seed, 6, (1, 0.3, "bulk00001")) # the wrap, resumed
|
||||||
|
finally:
|
||||||
|
event.remove(engine, "before_cursor_execute", grab)
|
||||||
|
assert len(captured) == 2
|
||||||
|
conn = db.connection()
|
||||||
|
for statement, params in captured:
|
||||||
|
plan = "\n".join(r[0] for r in conn.exec_driver_sql("EXPLAIN " + statement, params))
|
||||||
|
assert "ix_videos_shuffle_key" in plan, plan
|
||||||
|
assert "Sort Key: videos.shuffle_key" not in plan, plan
|
||||||
|
|
||||||
|
|
||||||
def test_the_surprise_reads_a_window_of_the_ring(db, make_user, monkeypatch):
|
def test_the_surprise_reads_a_window_of_the_ring(db, make_user, monkeypatch):
|
||||||
user = _seed(db, make_user)
|
user = _seed(db, make_user)
|
||||||
monkeypatch.setattr(queries, "SURPRISE_WINDOW", 3)
|
monkeypatch.setattr(queries, "SURPRISE_WINDOW", 3)
|
||||||
@@ -143,6 +190,31 @@ def test_a_wide_gap_on_the_ring_does_not_make_a_favourite(db, make_user):
|
|||||||
assert leads.count("lonely") < 15, leads.count("lonely")
|
assert leads.count("lonely") < 15, leads.count("lonely")
|
||||||
|
|
||||||
|
|
||||||
|
def test_any_seed_lands_on_the_ring():
|
||||||
|
# An int too large for a float overflowed the multiplication: a 500 for a hand-typed seed.
|
||||||
|
for seed in (10**400, -(10**400), 2**53 + 1, -7):
|
||||||
|
assert 0 <= feedquery.shuffle_start(seed) < 1, seed
|
||||||
|
|
||||||
|
|
||||||
|
def test_a_seed_out_of_range_is_refused_not_a_500(db, make_user, as_user):
|
||||||
|
client = as_user(_seed(db, make_user))
|
||||||
|
huge = "9" * 400
|
||||||
|
for url in (
|
||||||
|
f"/api/feed?sort=shuffle&seed={huge}",
|
||||||
|
f"/api/discover?seed={huge}",
|
||||||
|
f"/api/discover/pick?seed={huge}",
|
||||||
|
):
|
||||||
|
assert client.get(url).status_code == 422, url
|
||||||
|
|
||||||
|
|
||||||
|
def test_a_shuffle_cursor_past_a_float_is_a_bad_cursor(db, make_user, as_user):
|
||||||
|
client = as_user(_seed(db, make_user))
|
||||||
|
for k in ("[Infinity, 0.5]", f"[0, {'9' * 400}]"):
|
||||||
|
raw = '{"k": %s, "id": "k20"}' % k
|
||||||
|
cursor = base64.urlsafe_b64encode(raw.encode()).decode()
|
||||||
|
assert client.get(f"/api/feed?sort=shuffle&seed=1&cursor={cursor}").status_code == 400, k
|
||||||
|
|
||||||
|
|
||||||
def test_a_new_video_gets_its_own_key(db, make_user):
|
def test_a_new_video_gets_its_own_key(db, make_user):
|
||||||
_seed(db, make_user, {})
|
_seed(db, make_user, {})
|
||||||
db.add_all([Video(id=f"n{i}", channel_id="UCring", title="n") for i in range(3)])
|
db.add_all([Video(id=f"n{i}", channel_id="UCring", title="n") for i in range(3)])
|
||||||
|
|||||||
Reference in New Issue
Block a user