From 827df7494ce5e2a99f7eaba20511d50b8a1428a4 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Thu, 15 Oct 2026 04:58:16 +0200 Subject: [PATCH] fix(files): ending every session ends every API token, and the share links hold their folder - A password reset or change, "log out everywhere" and a revoked untrusted password revoke all of the account's API tokens too (audited): one minted from a stolen session no longer outlives the answer. - A bearer token or link grant with a non-ASCII character is refused (401/403), not a 500. - Making a token's grants or a folder link holds the folder, so a racing delete cannot fail the foreign key; a link's view count is one atomic UPDATE. - A folder link's card counts exactly the files its page lists (one `links.folder_files`). - A file with no extension is served without a trailing dot. - 0106's downgrade names a Hub folder so it never collides with one of the user's folders. - The speed baseline reads share tokens through the app (the database keeps them hashed/encrypted). - Tests: the token scope on the real session stack, the folder poster, the Hub refused for a link, 0106's downgrade data step, 0107 without a key; docs/comments follow (three folder kinds, folder links, key change, the cleanup job's token warnings). --- backend/alembic/versions/0106_api_tokens.py | 19 ++++- backend/app/apitokens.py | 30 +++++++- backend/app/audit.py | 3 +- backend/app/downloads/folders.py | 16 +++-- backend/app/downloads/links.py | 42 ++++++++++- backend/app/downloads/og.py | 10 +-- backend/app/downloads/storage.py | 6 +- backend/app/routes/auth.py | 30 ++++---- backend/app/routes/downloads.py | 11 ++- backend/app/routes/public.py | 41 ++++------- backend/app/routes/tokens.py | 4 +- backend/app/scheduler.py | 3 +- backend/tests/test_api_tokens.py | 78 ++++++++++++++++++++- backend/tests/test_auth_session.py | 2 +- backend/tests/test_folder_links.py | 53 +++++++++++++- backend/tests/test_link_token_at_rest.py | 40 +++++++++++ backend/tests/test_links.py | 3 + backend/tests/test_migration_0106.py | 50 +++++++++++++ backend/tests/test_storage.py | 3 + docs/self-hosting.md | 8 ++- frontend/e2e/speed.measure.ts | 39 +++++++---- frontend/src/i18n/locales/en/scheduler.json | 2 +- frontend/src/i18n/locales/hu/scheduler.json | 2 +- 23 files changed, 399 insertions(+), 96 deletions(-) create mode 100644 backend/tests/test_migration_0106.py diff --git a/backend/alembic/versions/0106_api_tokens.py b/backend/alembic/versions/0106_api_tokens.py index 11151462..6ea0e68b 100644 --- a/backend/alembic/versions/0106_api_tokens.py +++ b/backend/alembic/versions/0106_api_tokens.py @@ -89,7 +89,22 @@ def downgrade() -> None: op.drop_table("api_token_grants") op.drop_table("api_tokens") op.drop_index("uq_file_folder_hub", table_name="file_folders") - # A Hub folder may hold files (their foreign key RESTRICTs a delete): it becomes an own folder. - op.execute("UPDATE file_folders SET kind = 'own', name = 'Hub ' || id WHERE kind = 'hub'") + # A Hub folder may hold files (their foreign key RESTRICTs a delete): it becomes an own folder, + # named "Hub " — or "Hub (2)", … when its user already has a folder of that name, + # which `uq_file_folder_own_name` (per user, case-blind) would refuse. + conn = op.get_bind() + hubs = conn.execute(sa.text("SELECT id, user_id FROM file_folders WHERE kind = 'hub'")).all() + for folder_id, user_id in hubs: + name, n = f"Hub {folder_id}", 1 + while conn.execute( + sa.text("SELECT 1 FROM file_folders WHERE user_id = :u AND lower(name) = lower(:n)"), + {"u": user_id, "n": name}, + ).first(): + n += 1 + name = f"Hub {folder_id} ({n})" + conn.execute( + sa.text("UPDATE file_folders SET kind = 'own', name = :n WHERE id = :i"), + {"n": name, "i": folder_id}, + ) op.drop_constraint("ck_file_folder_kind", "file_folders", type_="check") op.create_check_constraint("ck_file_folder_kind", "file_folders", _KIND_OLD) diff --git a/backend/app/apitokens.py b/backend/app/apitokens.py index 6514399c..8ee6f4e2 100644 --- a/backend/app/apitokens.py +++ b/backend/app/apitokens.py @@ -23,10 +23,11 @@ from datetime import datetime, timedelta, timezone from enum import StrEnum from fastapi import Depends, Request -from sqlalchemy import or_, select, update +from sqlalchemy import delete, or_, select, update from sqlalchemy.orm import Session -from app import debugsession, logs +from app import audit, debugsession, logs +from app.audit import AuditAction from app.auth import current_user from app.db import get_db from app.models import ApiToken, ApiTokenGrant, User @@ -99,7 +100,8 @@ def well_formed(raw: str) -> bool: if len(raw) != TOKEN_LEN or not raw.startswith(PREFIX): return False body, check = raw[len(PREFIX) : -_CHECK_LEN], raw[-_CHECK_LEN:] - if any(c not in _ALPHABET for c in body): + # The check too: compare_digest refuses (TypeError, a 500) a str with anything non-ASCII in it. + if any(c not in _ALPHABET for c in body + check): return False return secrets.compare_digest(_check(body), check) @@ -215,6 +217,28 @@ def _token_caller(request: Request, db: Session, raw: str) -> FilesCaller: return caller +def revoke_all(db: Session, user_id: int) -> int: + """Revoke every token of `user_id`, each with its audit row (NOT committed). A token is a + credential like a session, so whatever ends every session of an account — a password reset or + change, "log out everywhere", a revoked untrusted password — ends its tokens too: one minted + from a stolen session must not outlive the owner's answer to that theft. Returns how many.""" + gone = ( + db.execute(delete(ApiToken).where(ApiToken.user_id == user_id).returning(ApiToken.id)) + .scalars() + .all() + ) + for token_id in gone: + audit.record( + db, + user_id, + AuditAction.API_TOKEN_REVOKE, + target_type="api_token", + target_id=token_id, + summary="revoked with every session of the account", + ) + return len(gone) + + def expiring(token: ApiToken, now: datetime) -> bool: """Whether `token` runs out within the warning span (and has not yet).""" return token.expires_at is not None and now < token.expires_at <= now + EXPIRY_WARNING diff --git a/backend/app/audit.py b/backend/app/audit.py index d44606ba..d014d313 100644 --- a/backend/app/audit.py +++ b/backend/app/audit.py @@ -73,7 +73,8 @@ class AuditAction: # What a token then did is the file rows above, their summary naming it. API_TOKEN_CREATE = "api_token_create" API_TOKEN_REVOKE = "api_token_revoke" - # A public `/watch` link to a file (`routes.downloads`): made, its settings changed, revoked. + # A public `/watch` link to a file or a folder (`routes.downloads`): made, its settings changed, + # revoked. # Never its token or password — only whether it has one. DOWNLOAD_LINK_CREATE = "download_link_create" DOWNLOAD_LINK_UPDATE = "download_link_update" diff --git a/backend/app/downloads/folders.py b/backend/app/downloads/folders.py index 46aa3682..50f2d98e 100644 --- a/backend/app/downloads/folders.py +++ b/backend/app/downloads/folders.py @@ -1,7 +1,7 @@ """The folders of the Files library — where each of a user's files sits. A folder is a row, never a directory: on disk a file stays where the download layout put it (one -shared asset can be held by several users), so a rename or a move is a column update. Two kinds: +shared asset can be held by several users), so a rename or a move is a column update. Three kinds: - a SOURCE folder is made automatically, one per origin a user has files from (YouTube, Plex clips, uploads, one per other site). Its `source_key` names the origin; the client renders its name @@ -13,11 +13,11 @@ shared asset can be held by several users), so a rename or a move is a column up Every file of a user is in exactly one folder. The folder a new file lands in is decided here (`source_key_for` + `source_folder`), at every place a job is created, so the rule lives once. A file -moves only into one of the user's own folders or back to its own source folder (`move`): a YouTube -file in the TikTok folder would be a lie the folder's name tells. +moves only into one of the user's own folders, their Hub folder, or back to its own source folder +(`move`): a YouTube file in the TikTok folder would be a lie the folder's name tells. Retention is the folder's: a source folder keeps a file `download_retention_days` (the value when the -folder was made; the user can change it), Uploads and own folders keep it until it is deleted. A +folder was made; the user can change it), Uploads, own folders and the Hub keep it until it is deleted. A done file carries its own `expires_at`, stamped by `done_expiry` when it finishes — unless the user kept it — and its `ready_at`, the moment it finished (`stamp_done`). """ @@ -202,8 +202,9 @@ def own_folder(db: Session, user_id: int, folder_id: int, *, hold: bool = True) def upload_target(db: Session, user_id: int, folder_id: int, *, hold: bool = True) -> FileFolder: - """The folder an upload asked to land in, if it may: one of the user's own folders, or their - Uploads folder. A file put in a site's folder would claim an origin it does not have.""" + """The folder an upload asked to land in, if it may: one of the user's own folders, their Hub + folder, or their Uploads folder. A file put in a site's folder would claim an origin it does not + have.""" folder = own_folder(db, user_id, folder_id, hold=hold) if folder.kind == FolderKind.SOURCE and folder.source_key != SourceKey.UPLOAD: raise FolderError("file_folder_not_its_source") @@ -225,7 +226,8 @@ def set_kept(db: Session, job: DownloadJob, kept: bool) -> None: def move(db: Session, job: DownloadJob, folder: FileFolder) -> None: - """Put a file in `folder` — one of its user's own folders, or the source folder it came from. + """Put a file in `folder` — one of its user's own folders, their Hub folder, or the source folder + it came from. The caller has checked that `folder` is the job's user's.""" if folder.kind == FolderKind.SOURCE and folder.source_key != source_key_for(db, job): raise FolderError("file_folder_not_its_source") diff --git a/backend/app/downloads/links.py b/backend/app/downloads/links.py index f4a2c3cf..80a0ac77 100644 --- a/backend/app/downloads/links.py +++ b/backend/app/downloads/links.py @@ -1,7 +1,8 @@ """Public "share by link" helpers for the download center. A DownloadLink is a capability URL: the unguessable token grants anyone read access to one -download's file on the login-free `/watch/{token}` page. So the token is never stored as written: +download's file, or to the files in one of its owner's own folders (`folder_files`), on the +login-free `/watch/{token}` page. So the token is never stored as written: the link is found by its SHA-256, and the owner's copy is Fernet-encrypted — a database copy alone opens nothing. Without that copy (no TOKEN_ENCRYPTION_KEY, or a key since changed) the link still opens; only its address cannot be shown again, and the owner makes a new link to share. @@ -21,9 +22,14 @@ from sqlalchemy.orm import Session from app import security from app.config import settings -from app.models import DownloadLink +from app.downloads import storage +from app.downloads.status import JobStatus +from app.models import DownloadJob, DownloadLink, FileFolder, MediaAsset _GRANT_TTL = 6 * 3600 # a watch session's grant is good for 6h, then the viewer re-unlocks +# The most files a folder link lists: as many as the owner's own Files list shows (`routes.files. +# MAX_LIST`; a test holds the two equal — this layer may not import that one). +FOLDER_MAX = 500 def new_token() -> str: @@ -48,6 +54,34 @@ def recover(link: DownloadLink) -> str | None: return security.decrypt_optional(link.token_enc) +def in_folder(folder: FileFolder) -> tuple: + """Which jobs a link to `folder` reaches: the done files in it NOW, the folder owner's. A file + moved out stops opening through the link at its next request; one moved in starts.""" + return ( + DownloadJob.folder_id == folder.id, + DownloadJob.user_id == folder.user_id, + DownloadJob.status == JobStatus.DONE, + ) + + +def folder_files(db: Session, folder: FileFolder) -> list[tuple[DownloadJob, MediaAsset]]: + """What a link to `folder` shows — its page lists exactly these, and its link card counts them: + each file `in_folder` whose asset is ready and whose bytes are on disk (so the list never offers + a dead row), newest first, at most `FOLDER_MAX`.""" + rows = db.execute( + select(DownloadJob, MediaAsset) + .join(MediaAsset, MediaAsset.id == DownloadJob.asset_id) + .where(*in_folder(folder), MediaAsset.status == "ready") + .order_by(DownloadJob.ready_at.desc().nulls_last(), DownloadJob.id.desc()) + .limit(FOLDER_MAX) + ).all() + return [ + (job, asset) + for job, asset in rows + if storage.safe_abs_path(settings.files_root, asset.rel_path) is not None + ] + + def is_expired(link: DownloadLink) -> bool: return link.expires_at is not None and link.expires_at <= datetime.now(timezone.utc) @@ -81,7 +115,9 @@ def check_grant(token: str, grant: str | None, password_version: int) -> bool: return False if exp < datetime.now(timezone.utc).timestamp(): return False - return hmac.compare_digest(sig, _sign(f"{token}.{exp}.{password_version}")) + # As bytes: compare_digest refuses (TypeError, a 500) a str with anything non-ASCII in it, and + # `sig` is whatever the URL carried. + return hmac.compare_digest(sig.encode(), _sign(f"{token}.{exp}.{password_version}").encode()) # --- serialization ------------------------------------------------------------------------- diff --git a/backend/app/downloads/og.py b/backend/app/downloads/og.py index a9cf65ed..10a0e37e 100644 --- a/backend/app/downloads/og.py +++ b/backend/app/downloads/og.py @@ -11,13 +11,11 @@ import html import re from pathlib import Path -from sqlalchemy import func, select from sqlalchemy.orm import Session from app.config import settings from app.downloads import links as linksmod from app.downloads import storage -from app.downloads.status import JobStatus from app.models import DownloadJob, DownloadLink, FileFolder, MediaAsset _OG_BLOCK = re.compile(r".*?", re.DOTALL) @@ -157,13 +155,7 @@ def _folder_tags(db: Session, link: DownloadLink, url: str) -> str | None: folder = db.get(FileFolder, link.folder_id) if folder is None or not folder.name: return None - count = db.execute( - select(func.count(DownloadJob.id)).where( - DownloadJob.folder_id == folder.id, - DownloadJob.user_id == folder.user_id, - DownloadJob.status == JobStatus.DONE, - ) - ).scalar_one() + count = len(linksmod.folder_files(db, folder)) # the page's own list, so the two agree title = folder.name.strip() about = f"{count} file{'' if count == 1 else 's'} · Shared via Siftlode" return "\n ".join( diff --git a/backend/app/downloads/storage.py b/backend/app/downloads/storage.py index c3de9bf8..892a3ed8 100644 --- a/backend/app/downloads/storage.py +++ b/backend/app/downloads/storage.py @@ -294,9 +294,13 @@ def served_media_type(media_kind: str | None, container: str | None) -> str | No def download_filename(display_name: str, container: str | None, path: Path) -> str: """The Content-Disposition filename for a served download: the cleaned display name carrying a single correct extension — the asset container, else the file's own suffix — without doubling it when the - name already ends in that extension. Shared by the private download and the public watch endpoints.""" + name already ends in that extension. A file with no extension at all is served without one — + never as `name.` (Windows drops a trailing dot, so the saved file would not be the one named). + Shared by the private download and the public watch endpoints.""" ext = container or path.suffix.lstrip(".") base = display_filename(display_name) + if not ext: + return base if base.lower().endswith(f".{ext.lower()}"): base = base[: -(len(ext) + 1)] return f"{base}.{ext}" diff --git a/backend/app/routes/auth.py b/backend/app/routes/auth.py index 7a4ba873..7ff41634 100644 --- a/backend/app/routes/auth.py +++ b/backend/app/routes/auth.py @@ -16,6 +16,7 @@ from sqlalchemy import select, update from sqlalchemy.exc import IntegrityError from sqlalchemy.orm import Session +from app import apitokens from app import email as email_mod from app import logs from app import mailtext @@ -446,7 +447,7 @@ async def callback( # SEC-10: drop any password nobody proved the mailbox for, now that Google attests # ownership (bumps the epoch to revoke any session it minted). Runs before the # activation below, which is what it reads. - revoke_untrusted_preset_password(user) + revoke_untrusted_preset_password(db, user) # Google verified the email and is_allowed granted access, so finish activating a # previously pending password registration (otherwise current_user would bounce it). user.email_verified = True @@ -486,7 +487,7 @@ async def callback( # real owner, signed in through Google right now, sets a new password in Settings. if canonical_email(email) in env_admin_canonicals(): if user.role != "admin" and user.password_hash is not None: - _revoke_password(user) + _revoke_password(db, user) user.role = "admin" elif not user.role: user.role = "user" @@ -1118,9 +1119,8 @@ def password_reset_confirm(body: TokenPasswordIn, db: Session = Depends(get_db)) # about. The person still confirms their address the normal way. if kind == "reset": user.email_verified = True - bump_session_epoch( - user - ) # SA4: a reset (often a compromise response) kills all existing sessions. + # SA4: a reset (often a compromise response) kills all existing sessions — and API tokens. + bump_session_epoch(db, user) # Burn any other outstanding reset tokens for this user. for row in db.execute( select(AuthToken).where( @@ -1143,13 +1143,17 @@ def _remember_epoch(request: Request, user: User) -> None: request.session["epochs"] = epochs -def bump_session_epoch(user: User) -> None: +def bump_session_epoch(db: Session, user: User) -> None: """Invalidate every EXISTING signed cookie for this account by advancing its epoch (SA4). Cookies - minted before the bump carry a now-stale epoch and are rejected by `current_user`.""" + minted before the bump carry a now-stale epoch and are rejected by `current_user`. The account's + API tokens go with them (`apitokens.revoke_all`): a token minted from a stolen session must not + outlive the answer to that theft. Not committed — the caller commits it with its change.""" user.session_epoch = (user.session_epoch or 0) + 1 + if user.id is not None: + apitokens.revoke_all(db, user.id) -def revoke_untrusted_preset_password(user: User) -> None: +def revoke_untrusted_preset_password(db: Session, user: User) -> None: """SEC-10 (account pre-emption): a password set on an account whose email was NEVER verified is untrusted — whoever registered the address (to pre-empt the real owner) could have chosen it. When the real owner attests ownership through Google, drop that password and bump the epoch to revoke any @@ -1183,15 +1187,15 @@ def revoke_untrusted_preset_password(user: User) -> None: _send_reset_if_eligible keyed off exactly that, so a revoked account silently got no reset link and was locked out for good (it could not sign in either). The stamp separates the two cases.""" if not user.email_verified or (not user.is_active and user.password_hash is not None): - _revoke_password(user) + _revoke_password(db, user) -def _revoke_password(user: User) -> None: +def _revoke_password(db: Session, user: User) -> None: """Drop the account's password, stamp why (so password-reset still accepts it) and end every session it signed in.""" user.password_hash = None user.password_revoked_at = datetime.now(timezone.utc) - bump_session_epoch(user) + bump_session_epoch(db, user) @router.post("/logout-others") @@ -1203,7 +1207,7 @@ def logout_others( in somewhere" / a suspected session compromise.""" # Commit the bump BEFORE re-stamping this cookie, so a failed commit can't strand the current # session at a newer epoch than the DB (see set_password). - bump_session_epoch(user) + bump_session_epoch(db, user) db.commit() _remember_epoch(request, user) log.info("Logout-others: uid=%s (epoch=%s)", user.id, user.session_epoch) @@ -1255,7 +1259,7 @@ def set_password( # SA4: a password change logs out every OTHER session; re-stamp this cookie at the new epoch so # the person making the change stays signed in here. Commit BEFORE re-stamping so a failed commit # can't leave the cookie at a newer epoch than the DB (which would wrongly 401 THIS session). - bump_session_epoch(user) + bump_session_epoch(db, user) db.commit() _remember_epoch(request, user) log.info("Password set/changed: uid=%s", user.id) diff --git a/backend/app/routes/downloads.py b/backend/app/routes/downloads.py index 2bebf57e..873b6cf7 100644 --- a/backend/app/routes/downloads.py +++ b/backend/app/routes/downloads.py @@ -27,6 +27,7 @@ from app.db import get_db from app.downloads import clip as clipmod from app.downloads import credentials as dlcreds from app.downloads import edit as editmod +from app.downloads import folders from app.downloads import links as linksmod from app.downloads import quota, service, storage from app.downloads import urlguard @@ -806,8 +807,14 @@ def create_link( def _shareable_folder(db: Session, user: User, folder_id: int) -> FileFolder: """One of the user's own folders. A source folder lets its files go on its own clock, and the Hub - is the user's channel to a machine of theirs: neither is a shelf to hand out.""" - folder = owned_or_404(db, FileFolder, folder_id, user, what="folder") + is the user's channel to a machine of theirs: neither is a shelf to hand out. Held (FOR SHARE) + for the rest of the transaction, so a delete of it at the same moment waits and takes the new + link with it, rather than failing the link's foreign key.""" + owned_or_404(db, FileFolder, folder_id, user, what="folder") + try: + folder = folders.own_folder(db, user.id, folder_id) + except folders.FolderError as e: + raise refused(409, e.reason) from e if folder.kind != FolderKind.OWN: raise refused(409, "share_folder_kind") return folder diff --git a/backend/app/routes/public.py b/backend/app/routes/public.py index 1dd34877..d0d69ff8 100644 --- a/backend/app/routes/public.py +++ b/backend/app/routes/public.py @@ -9,7 +9,7 @@ a short-lived signed grant that the media URL carries (a `