fix: review round 2 — a signed-out tab acts as nobody, honest share-page failures
Deep-review round 2 over main..improvement/watch-background-audio. Account resolution (server) - A request naming an account that is no longer signed in to this browser (X-Siftlode-Account or ?account=) now acts as nobody instead of the session's default account. A tab still pinned to an account signed out in another tab went on acting as the default: its player checkpoints and preference saves landed in that account, the messages socket and Plex playback ran as it, and its Sign out signed THAT account out. A hidden tab never polls /api/me, so a tab playing in the background kept doing it. The /api/me bootstrap probe alone keeps the fallback, which is how such a tab learns who it now is: the app's 401 handler reloads, and BootGate re-pins the tab. BootGate - On an identity change the tab reloads without moving its pin first. The tree being torn down still runs its unmount saves before the page goes, and with the pin already moved they wrote into the new account's local keys (a player volume still pending). The reloaded page corrects the pin. Share page (/watch) - Only a 404 says the link is gone. A 5xx, a dropped connection, and the unlock's own rate limit all rendered "no longer available ... expired or revoked" to a viewer with a valid link, with no way to try again. A load failure now offers Retry; too many password tries says so and keeps the form; an unlock that gets no answer says so instead of doing nothing. Strings in en/hu/de. Downloads - A cookie-authed asset is recognised under the current signature version and the one before it, so a job queued before a deploy that bumps the version still downloads with its cookie instead of failing on the login wall with an error that says no cookie was used. The app chunk is preloaded through lazyWithPreload, so a failed fetch no longer leaves an unhandled rejection beside the ErrorBoundary. The panel layout seed is one helper (seedLayout). Tests: the stale-tab refusal, its harmless Sign out and the probe fallback; the pre-bump authed asset; BootGate keeping the pin on reload and staying mounted through a same-account change; the reveal clock's later-commit hand-off; the prefs, panel layout and demo-scope seeds from the me cache; the share page's failure states. Each mutation-checked RED for its own reason.
This commit is contained in:
+22
-9
@@ -269,21 +269,27 @@ def purge_user(db: Session, user: User, background: BackgroundTasks) -> None:
|
||||
ACTIVE_ACCOUNT_HEADER = "x-siftlode-account"
|
||||
|
||||
|
||||
def resolved_user_id(conn: HTTPConnection) -> tuple[int | None, bool]:
|
||||
def resolved_user_id(conn: HTTPConnection, *, strict: bool = True) -> tuple[int | None, bool]:
|
||||
"""Which account this connection acts as, and whether that's the session's default account.
|
||||
Takes any HTTPConnection — an HTTP Request OR a WebSocket — so the WS push channel shares this
|
||||
exact per-tab resolution instead of re-implementing it.
|
||||
|
||||
The signed cookie holds the *wallet* (`account_ids`, every account signed into this browser)
|
||||
plus a default `user_id`. A caller may override the default with the X-Siftlode-Account header,
|
||||
but ONLY for an account already in the wallet — so a tab can't impersonate an account that never
|
||||
authenticated here. Everything else (WebSocket, plain navigations) keeps using the cookie default.
|
||||
plus a default `user_id`. A caller may name its account with the X-Siftlode-Account header (or
|
||||
`?account=`), but ONLY an account in the wallet — so a tab can't impersonate an account that
|
||||
never authenticated here. A connection that names no account acts as the cookie default.
|
||||
|
||||
A connection naming an account NOT in the wallet acts as nobody (`None`) — typically a tab still
|
||||
pinned to an account that was signed out in another tab. It must not fall back to the default:
|
||||
that tab would go on acting as an account it never showed, its player checkpoints and pref saves
|
||||
landing there and its Sign out signing THAT account out. Only the bootstrap /api/me probe passes
|
||||
`strict=False` and gets the default, which is how such a tab learns who it now is (the frontend
|
||||
re-pins it and reloads).
|
||||
"""
|
||||
default_id = conn.session.get("user_id")
|
||||
wallet = conn.session.get("account_ids") or []
|
||||
# Header for normal XHR; ?account= for contexts that can't set headers (WebSocket, and a
|
||||
# plain <a> file download). Both are wallet-gated below, so neither can impersonate an
|
||||
# account that never signed into this browser.
|
||||
# plain <a> file download). Both are wallet-gated below.
|
||||
hdr = conn.headers.get(ACTIVE_ACCOUNT_HEADER) or conn.query_params.get("account")
|
||||
if hdr:
|
||||
try:
|
||||
@@ -292,11 +298,16 @@ def resolved_user_id(conn: HTTPConnection) -> tuple[int | None, bool]:
|
||||
hid = None
|
||||
if hid is not None and hid in wallet:
|
||||
return hid, hid == default_id
|
||||
if strict:
|
||||
return None, False
|
||||
return default_id, True
|
||||
|
||||
|
||||
def current_user(request: Request, db: Session = Depends(get_db)) -> User:
|
||||
user_id, is_default = resolved_user_id(request)
|
||||
return _session_user(request, db, *resolved_user_id(request))
|
||||
|
||||
|
||||
def _session_user(request: Request, db: Session, user_id: int | None, is_default: bool) -> User:
|
||||
if not user_id:
|
||||
raise HTTPException(status_code=401, detail="Not authenticated")
|
||||
user = db.get(User, user_id)
|
||||
@@ -328,9 +339,11 @@ def current_user(request: Request, db: Session = Depends(get_db)) -> User:
|
||||
def optional_current_user(request: Request, db: Session = Depends(get_db)) -> User | None:
|
||||
"""Like `current_user` but returns None instead of raising 401 when there's no valid session.
|
||||
Used by the bootstrap /api/me probe so the logged-out landing doesn't log a failed request to
|
||||
the browser console (a non-2xx fetch is a console error no matter how the app handles it)."""
|
||||
the browser console (a non-2xx fetch is a console error no matter how the app handles it).
|
||||
The one lenient resolution (`strict=False`): a tab naming an account no longer signed in here gets
|
||||
the default account, which is how it learns who it now is — see `resolved_user_id`."""
|
||||
try:
|
||||
return current_user(request, db)
|
||||
return _session_user(request, db, *resolved_user_id(request, strict=False))
|
||||
except HTTPException:
|
||||
return None
|
||||
|
||||
|
||||
@@ -118,14 +118,17 @@ def is_authed_asset(asset: MediaAsset, spec: dict, user_id: int, url: str | None
|
||||
"""Whether an asset is `user_id`'s private, cookie-salted identity for `url`'s host — the one
|
||||
kind of asset a cookie is ever attached to: the one enqueue creates (`authed_format_sig`), or a
|
||||
Re-download's rebuild of it (`rebuild_salt`). The worker's cookie gate asks it before a download,
|
||||
and the failure diagnosis asks it after one (a failure on this asset happened WITH the cookie)."""
|
||||
and the failure diagnosis asks it after one (a failure on this asset happened WITH the cookie).
|
||||
Either signature version counts (`formats.format_sigs`): a job queued before a deploy that bumped
|
||||
the version still downloads into the asset it was given."""
|
||||
host_key = host_key_for_url(url)
|
||||
if host_key is None:
|
||||
return False
|
||||
if asset.format_sig == authed_format_sig(spec, user_id, host_key):
|
||||
salt = cookie_salt(user_id, host_key)
|
||||
if asset.format_sig in formats.format_sigs(spec, salt=salt):
|
||||
return True
|
||||
return asset.id is not None and asset.format_sig == formats.format_sig(
|
||||
spec, salt=rebuild_salt(asset.id, cookie_salt(user_id, host_key))
|
||||
return asset.id is not None and asset.format_sig in formats.format_sigs(
|
||||
spec, salt=rebuild_salt(asset.id, salt)
|
||||
)
|
||||
|
||||
|
||||
|
||||
@@ -100,20 +100,34 @@ def normalize(spec: dict) -> dict:
|
||||
return s
|
||||
|
||||
|
||||
def format_sig(spec: dict, salt: str | None = None) -> str:
|
||||
def _sig_version(s: dict) -> int:
|
||||
return _AUDIO_SIG_VERSION if s["mode"] == "a" else _SIG_VERSION
|
||||
|
||||
|
||||
def format_sig(spec: dict, salt: str | None = None, *, version: int | None = None) -> str:
|
||||
"""Stable short hash of the output-affecting spec — the MediaAsset cache key component.
|
||||
|
||||
`salt` forks a distinct, non-dedup identity for the same spec (used by force-redownload so it
|
||||
always re-fetches into its own asset instead of sharing a cached file with other jobs)."""
|
||||
always re-fetches into its own asset instead of sharing a cached file with other jobs).
|
||||
`version` hashes under a signature version other than the current one (see `format_sigs`)."""
|
||||
s = normalize(spec)
|
||||
data = {k: s[k] for k in _SIG_KEYS}
|
||||
data["_v"] = _AUDIO_SIG_VERSION if s["mode"] == "a" else _SIG_VERSION
|
||||
data["_v"] = _sig_version(s) if version is None else version
|
||||
if salt is not None:
|
||||
data["_salt"] = salt
|
||||
payload = json.dumps(data, sort_keys=True, separators=(",", ":"))
|
||||
return hashlib.sha256(payload.encode()).hexdigest()[:24]
|
||||
|
||||
|
||||
def format_sigs(spec: dict, salt: str | None = None) -> tuple[str, str]:
|
||||
"""`format_sig` under the current signature version and under the one before it. An asset keeps
|
||||
the signature it was created with, so a job queued before a deploy that bumped the version and
|
||||
run after it downloads into an asset carrying the previous one — which must still be recognised
|
||||
as that user's private asset (`credentials.is_authed_asset`), or it downloads without its cookie."""
|
||||
previous = _sig_version(normalize(spec)) - 1
|
||||
return format_sig(spec, salt), format_sig(spec, salt, version=previous)
|
||||
|
||||
|
||||
def build_ydl_opts(
|
||||
spec: dict,
|
||||
outtmpl: str,
|
||||
|
||||
@@ -631,6 +631,8 @@ async def logout(request: Request):
|
||||
out in one tab doesn't disturb another tab running a different account. If the account being
|
||||
removed was the session default, promote the most recent remaining account (or clear the
|
||||
session when none remain)."""
|
||||
# A tab still pinned to an account already signed out in another tab resolves to nobody, so it
|
||||
# signs nobody out — it used to fall back to the default and sign THAT account out in its place.
|
||||
active, is_default = resolved_user_id(request)
|
||||
remaining = [a for a in (request.session.get("account_ids") or []) if a != active]
|
||||
request.session["account_ids"] = remaining
|
||||
|
||||
@@ -423,9 +423,10 @@ async def messages_ws(ws: WebSocket) -> None:
|
||||
"""Live push channel: authenticated by the session cookie, registers the connection so
|
||||
sent messages reach this user's open tabs instantly. We don't consume client→server frames
|
||||
(sending goes through POST /api/messages); the receive loop only detects disconnect."""
|
||||
# Which signed-in account this socket acts as — the SHARED per-tab resolution (wallet-gated
|
||||
# ?account= override, session default otherwise). A browser WebSocket can't send the
|
||||
# X-Siftlode-Account header, so resolved_user_id falls through to the ?account= query param.
|
||||
# Which signed-in account this socket acts as — the SHARED per-tab resolution (a wallet-gated
|
||||
# ?account=, the session default when none is named, nobody for an account no longer signed in
|
||||
# here). A browser WebSocket can't send the X-Siftlode-Account header, so resolved_user_id falls
|
||||
# through to the ?account= query param.
|
||||
uid, _ = resolved_user_id(ws)
|
||||
if not uid:
|
||||
await ws.close(code=1008)
|
||||
|
||||
@@ -57,6 +57,25 @@ def test_a_non_wallet_account_header_falls_back_to_the_default(client, make_user
|
||||
assert me.json()["email"] == "alice2@example.com" # still Alice, not Mallory
|
||||
|
||||
|
||||
def test_a_tab_pinned_to_an_account_signed_out_elsewhere_acts_as_nobody(client, make_user):
|
||||
# Two accounts in one browser: a tab pinned to Alice, Bob the default. Alice signs out in another
|
||||
# tab. The pinned tab used to go on acting as the default, Bob — its player checkpoints and pref
|
||||
# saves landed in his account, and its own Sign out signed HIM out.
|
||||
alice = make_user(email="alice3@example.com", password=_PW)
|
||||
make_user(email="bob3@example.com", password=_PW)
|
||||
assert _login(client, "alice3@example.com", _PW).status_code == 200
|
||||
assert _login(client, "bob3@example.com", _PW).status_code == 200 # Bob is now the default
|
||||
tab = {ACTIVE_ACCOUNT_HEADER: str(alice.id)}
|
||||
assert client.get("/api/feed", headers=tab).status_code == 200
|
||||
assert client.post("/auth/logout", headers=tab).status_code == 200 # Alice signs out
|
||||
|
||||
assert client.get("/api/feed", headers=tab).status_code == 401
|
||||
assert client.post("/auth/logout", headers=tab).status_code == 200
|
||||
assert client.get("/api/feed").status_code == 200 # Bob is still signed in, the session intact
|
||||
# The bootstrap probe still answers as the default: how the stale tab learns who it is now.
|
||||
assert client.get("/api/me", headers=tab).json()["email"] == "bob3@example.com"
|
||||
|
||||
|
||||
def test_logout_others_revokes_the_other_session_but_keeps_the_caller(client, make_user):
|
||||
# Two independent cookie jars (browsers) signed into the same account. `logout-others` from one
|
||||
# bumps the account epoch so the OTHER's cookie is rejected on its next request, while the caller
|
||||
|
||||
@@ -72,6 +72,21 @@ def test_authed_sig_differs_per_user_and_from_the_shared_sig():
|
||||
assert a != shared and b != shared and a != b
|
||||
|
||||
|
||||
def test_an_authed_asset_queued_before_a_signature_version_bump_still_gets_its_cookie(monkeypatch):
|
||||
# A job keeps the asset it was queued with. A deploy that bumps the signature version between the
|
||||
# queueing and the download used to make the user's private asset stop counting as theirs, so the
|
||||
# download went without its cookie and failed on the login wall.
|
||||
spec = formats.normalize({"mode": "a"})
|
||||
queued = MediaAsset(format_sig=credentials.authed_format_sig(spec, 1, "facebook"))
|
||||
rebuild = credentials.rebuild_salt(7, credentials.cookie_salt(1, "facebook"))
|
||||
rebuilt = MediaAsset(id=7, format_sig=formats.format_sig(spec, salt=rebuild))
|
||||
monkeypatch.setattr(formats, "_AUDIO_SIG_VERSION", formats._AUDIO_SIG_VERSION + 1) # the deploy
|
||||
|
||||
for asset in (queued, rebuilt):
|
||||
assert credentials.is_authed_asset(asset, spec, 1, FB_URL)
|
||||
assert not credentials.is_authed_asset(asset, spec, 2, FB_URL) # still private to its user
|
||||
|
||||
|
||||
def test_write_cookiefile_is_private_and_survives_the_retry_wipe(tmp_path):
|
||||
staging = tmp_path / "staging"
|
||||
staging.mkdir()
|
||||
|
||||
Reference in New Issue
Block a user