From bac0b9356640e6f649b239efa8ca18f66cdef113 Mon Sep 17 00:00:00 2001 From: CaliBrain Date: Fri, 2 Oct 2026 20:38:58 -0400 Subject: [PATCH] fix: keep https:// for qBittorrent and hide release sources from non-admins (#1422) qBittorrent over HTTPS (#1417) qbittorrent-api ignores the scheme in `host` and probes HTTP and HTTPS itself. When the HTTPS probe fails, for instance on a certificate Shelfmark doesn't trust, it falls back to plain HTTP against the TLS port, and a reverse proxy answers "400 The plain HTTP request was sent to HTTPS port". The download client and the Test Connection button now pass FORCE_SCHEME_FROM_HOST for https:// URLs, so the configured scheme is used as-is and a certificate problem is reported as one. http:// and bare host:port URLs keep the probe: normalization adds http:// to a bare host, and the probe follows a proxy's redirect to HTTPS. Release sources leaked to non-admins (#1418) A download queued from an approved request carries the release an admin picked, and the requester could read it from their own activity feed: the request's release_data (source_id, indexer, info URL, torrent attributes), the download's full server path, and the download id itself, which for Prowlarr is ":" and for a private tracker is a URL into it. That id keys every status payload, the /api/localdownload query and the cover proxy URL, so hiding release_data alone would not have closed the leak. For non-admin viewers: - request rows keep only the release fields the activity cards display - download_path is reduced to the file name, which the browser download reveals anyway - request-linked downloads are addressed by an opaque id (a keyed HMAC of the task id) in the snapshot, history, /api/status, queue order, active downloads, dismissed keys, cover URLs and the websocket status and progress events. Routes that take a download id translate it back, searching only the caller's own downloads. Downloads a user queued directly keep their real id: the user picked that release from search results that already showed it, and the release list matches its buttons to the queue by that id. Admin views are unchanged. The activity sidebar now links a fulfilled request to its download by request_id instead of release_data.source_id. Closes #1417 Closes #1418 --- shelfmark/api/websocket.py | 15 +- shelfmark/core/activity_routes.py | 130 +++++++++- shelfmark/core/download_history_service.py | 17 ++ shelfmark/core/request_routes.py | 6 + shelfmark/core/viewer_redaction.py | 149 +++++++++++ shelfmark/download/clients/qbittorrent.py | 6 +- shelfmark/download/clients/settings.py | 2 + shelfmark/download/clients/torbox.py | 27 +- shelfmark/download/orchestrator.py | 7 +- shelfmark/main.py | 72 +++++- .../components/activity/ActivitySidebar.tsx | 31 +-- .../components/activity/activityMappers.ts | 22 ++ .../src/tests/activityMappers.test.ts | 30 +++ tests/core/test_activity_routes_api.py | 233 +++++++++++++++++- tests/core/test_viewer_redaction.py | 117 +++++++++ tests/prowlarr/test_qbittorrent_client.py | 36 +++ tests/prowlarr/test_qbittorrent_settings.py | 20 ++ tests/prowlarr/test_torbox_client.py | 40 ++- 18 files changed, 906 insertions(+), 54 deletions(-) create mode 100644 shelfmark/core/viewer_redaction.py create mode 100644 tests/core/test_viewer_redaction.py diff --git a/shelfmark/api/websocket.py b/shelfmark/api/websocket.py index 12bfaec8..6c1be11e 100644 --- a/shelfmark/api/websocket.py +++ b/shelfmark/api/websocket.py @@ -8,6 +8,8 @@ from typing import TYPE_CHECKING, Any from flask_socketio import SocketIO, join_room, leave_room +from shelfmark.core.viewer_redaction import viewer_download_id + if TYPE_CHECKING: from collections.abc import Callable @@ -167,7 +169,12 @@ class WebSocketManager: logger.exception("Failed to send status update for room %s", room) def broadcast_download_progress( - self, book_id: str, progress: float, status: str, user_id: int | None = None + self, + book_id: str, + progress: float, + status: str, + user_id: int | None = None, + request_id: int | None = None, ) -> None: """Broadcast download progress update for a specific book.""" socketio = self._get_socketio() @@ -178,12 +185,14 @@ class WebSocketManager: data = {"book_id": book_id, "progress": progress, "status": status} # Admins always see all progress socketio.emit("download_progress", data, to="admins") - # If task belongs to a specific user, send to their room too + # If task belongs to a specific user, send to their room too, under the id + # their status uses: an opaque one for a request-linked download (#1418). if user_id is not None: room = f"user_{user_id}" with self._rooms_lock: if room in self._user_rooms: - socketio.emit("download_progress", data, to=room) + user_data = {**data, "book_id": viewer_download_id(book_id, request_id)} + socketio.emit("download_progress", user_data, to=room) logger.debug("Broadcasted progress for book %s: %s%%", book_id, progress) except Exception: logger.exception("Error broadcasting download progress") diff --git a/shelfmark/core/activity_routes.py b/shelfmark/core/activity_routes.py index 4bf8ffc3..de63457a 100644 --- a/shelfmark/core/activity_routes.py +++ b/shelfmark/core/activity_routes.py @@ -31,6 +31,15 @@ from shelfmark.core.request_helpers import ( populate_request_usernames, ) from shelfmark.core.request_validation import RequestStatus +from shelfmark.core.viewer_redaction import ( + is_public_download_id, + match_public_download_id, + public_download_id, + redact_download_payload, + redact_request_row, + redact_status, + viewer_download_id, +) if TYPE_CHECKING: from collections.abc import Callable @@ -501,6 +510,77 @@ def _request_history_entry( } +def _request_linked_task_ids( + actor: _ActorContext, + *, + queue_status: Callable[..., dict[str, dict[str, Any]]], + download_history_service: DownloadHistoryService, +) -> list[str]: + """Task ids of the actor's request-linked downloads, which they see by opaque id.""" + if actor.db_user_id is None: + return [] + live_task_ids = [ + task_id + for bucket in queue_status(user_id=actor.owner_scope).values() + if isinstance(bucket, dict) + for task_id, payload in bucket.items() + if isinstance(payload, dict) and payload.get("request_id") is not None + ] + return [ + *live_task_ids, + *download_history_service.list_request_task_ids(user_id=actor.db_user_id), + ] + + +def _viewer_dismissed_entries( + entries: list[dict[str, str]], + *, + request_ids_by_task: dict[str, object], +) -> list[dict[str, str]]: + """Rewrite a non-admin's dismissed download keys to the ids their snapshot uses. + + Downloads outside the recent rows and the live queue never reach the snapshot, + so their entries are dropped rather than looked up one by one. + """ + viewer_entries: list[dict[str, str]] = [] + for entry in entries: + if entry["item_type"] != "download": + viewer_entries.append(entry) + continue + task_id = _parse_item_key(entry["item_key"], "download") + if task_id is None or task_id not in request_ids_by_task: + continue + viewer_id = viewer_download_id(task_id, request_ids_by_task[task_id]) + viewer_entries.append({"item_type": "download", "item_key": f"download:{viewer_id}"}) + return viewer_entries + + +def _redact_history_entry(entry: dict[str, Any]) -> dict[str, Any]: + """Return a history entry with the same redactions as the non-admin snapshot.""" + redacted = dict(entry) + snapshot = entry.get("snapshot") + source_id = entry.get("source_id") + if entry.get("item_type") == "download": + if isinstance(source_id, str) and source_id: + viewer_id = viewer_download_id(source_id, entry.get("request_id")) + item_key = f"download:{viewer_id}" + redacted.update(id=item_key, item_key=item_key, source_id=viewer_id) + if isinstance(snapshot, dict) and isinstance(snapshot.get("download"), dict): + redacted["snapshot"] = { + **snapshot, + "download": redact_download_payload(snapshot["download"], task_id=source_id), + } + return redacted + + # A request's source_id names the download it was fulfilled with, which the + # requester sees by its opaque id. + if isinstance(source_id, str) and source_id: + redacted["source_id"] = public_download_id(source_id) + if isinstance(snapshot, dict) and isinstance(snapshot.get("request"), dict): + redacted["snapshot"] = {**snapshot, "request": redact_request_row(snapshot["request"])} + return redacted + + def register_activity_routes( app: Flask, user_db: UserDB, @@ -577,6 +657,25 @@ def register_activity_routes( continue visible_request_rows.append(row) + if not actor.is_admin: + # Redacted only now: the delivery-state sync above matches on real task ids. + request_ids_by_task: dict[str, object] = { + task_id: payload.get("request_id") + for bucket in live_queue.values() + if isinstance(bucket, dict) + for task_id, payload in bucket.items() + if isinstance(payload, dict) + } + request_ids_by_task.update( + (str(row.get("task_id") or "").strip(), row.get("request_id")) for row in db_rows + ) + dismissed_entries = _viewer_dismissed_entries( + dismissed_entries, + request_ids_by_task=request_ids_by_task, + ) + status = redact_status(status) + visible_request_rows = [redact_request_row(row) for row in visible_request_rows] + return jsonify( { "status": status, @@ -622,6 +721,21 @@ def register_activity_routes( item_key=item_key, ) + # The id as this viewer knows it, which is what the response echoes. + viewer_task_id = task_id + if not actor.is_admin and is_public_download_id(task_id): + task_id = ( + match_public_download_id( + task_id, + _request_linked_task_ids( + actor, + queue_status=queue_status, + download_history_service=download_history_service, + ), + ) + or task_id + ) + existing = download_history_service.get_by_task_id(task_id) if existing is None: return _activity_error_response( @@ -671,7 +785,7 @@ def register_activity_routes( ) dismissal_item = { "item_type": "download", - "item_key": f"download:{task_id}", + "item_key": f"download:{viewer_task_id}", } elif item_type == "request": @@ -796,6 +910,7 @@ def register_activity_routes( dismissal_items: list[dict[str, str]] = [] missing_item_keys: list[str] = [] live_queue_index: dict[str, tuple[str, dict[str, Any]]] | None = None + request_linked_task_ids: list[str] | None = None for item in items: if not isinstance(item, dict): @@ -824,9 +939,18 @@ def register_activity_routes( item_key=item_key, item_count=len(items), ) + viewer_task_id = task_id + if not actor.is_admin and is_public_download_id(task_id): + if request_linked_task_ids is None: + request_linked_task_ids = _request_linked_task_ids( + actor, + queue_status=queue_status, + download_history_service=download_history_service, + ) + task_id = match_public_download_id(task_id, request_linked_task_ids) or task_id existing = download_history_service.get_by_task_id(task_id) if existing is None: - missing_item_keys.append(f"download:{task_id}") + missing_item_keys.append(f"download:{viewer_task_id}") continue if live_queue_index is None: live_queue_index = _build_queue_index(queue_status(user_id=actor.owner_scope)) @@ -1060,6 +1184,8 @@ def register_activity_routes( msg = f"Unknown activity history item_type: {item_type}" raise RuntimeError(msg) + if not actor.is_admin: + payload = [_redact_history_entry(entry) for entry in payload] return jsonify(payload) @app.route("/api/activity/history", methods=["DELETE"]) diff --git a/shelfmark/core/download_history_service.py b/shelfmark/core/download_history_service.py index a3087e74..28dc0965 100644 --- a/shelfmark/core/download_history_service.py +++ b/shelfmark/core/download_history_service.py @@ -407,6 +407,23 @@ class DownloadHistoryService: finally: conn.close() + def list_request_task_ids(self, *, user_id: int) -> list[str]: + """Return the task ids of every request-linked download owned by ``user_id``. + + A non-admin addresses these downloads by an opaque id, so routes resolve it + against this set, which also confines the lookup to the user's own downloads. + """ + normalized_user_id = normalize_optional_positive_int(user_id, "user_id") + conn = self._connect() + try: + rows = conn.execute( + "SELECT task_id FROM download_history WHERE user_id = ? AND request_id IS NOT NULL", + (normalized_user_id,), + ).fetchall() + return [str(row["task_id"]) for row in rows] + finally: + conn.close() + def list_recent( self, *, diff --git a/shelfmark/core/request_routes.py b/shelfmark/core/request_routes.py index 2a0fd767..d34ec1ac 100644 --- a/shelfmark/core/request_routes.py +++ b/shelfmark/core/request_routes.py @@ -41,6 +41,7 @@ from shelfmark.core.requests_service import ( fulfil_request, reject_request, ) +from shelfmark.core.viewer_redaction import redact_request_row if TYPE_CHECKING: from collections.abc import Callable @@ -847,6 +848,9 @@ def register_request_routes( ) except ValueError as exc: return jsonify({"error": str(exc)}), 400 + if not session.get("is_admin", False): + # A fulfilled request carries the release an admin picked (#1418). + rows = [redact_request_row(row) for row in rows] return jsonify(rows) @app.route("/api/requests/", methods=["DELETE"]) @@ -901,6 +905,8 @@ def register_request_routes( room="admins", ) + if not session.get("is_admin", False): + updated = redact_request_row(updated) return jsonify(updated) @app.route("/api/admin/requests", methods=["GET"]) diff --git a/shelfmark/core/viewer_redaction.py b/shelfmark/core/viewer_redaction.py new file mode 100644 index 00000000..6a26f155 --- /dev/null +++ b/shelfmark/core/viewer_redaction.py @@ -0,0 +1,149 @@ +"""Hide release-source details from non-admin viewers (#1418). + +A download queued from an approved request carries the release an admin picked. +Its task id is that release's ``source_id``, which for Prowlarr is +``:``, and a private tracker's guid is a URL into the tracker. +The requester never browsed those releases, so for them a request-linked +download is addressed by an opaque id instead. Request rows keep only the +release fields the activity cards display, and a download path shrinks to the +file name, which the browser download reveals anyway. + +Downloads a user queued directly keep their real id: the user picked that +release from search results that already showed its ``source_id``, and the +release list matches its buttons to the queue by that id. +""" + +from __future__ import annotations + +import hashlib +import hmac +import os +from pathlib import PurePath +from typing import TYPE_CHECKING, Any + +from shelfmark.core.request_helpers import normalize_positive_int + +if TYPE_CHECKING: + from collections.abc import Iterable + +PUBLIC_DOWNLOAD_ID_PREFIX = "dl_" + +# Release fields a non-admin's activity cards display. Everything else (source_id, +# indexer, info and download URLs, torrent attributes) stays server-side. +_RELEASE_DISPLAY_FIELDS = frozenset( + { + "author", + "content_type", + "extension", + "filetype", + "format", + "language", + "preview", + "series_count", + "series_name", + "series_position", + "size", + "source", + "source_display_name", + "subtitle", + "title", + "year", + } +) + +# Replaced with a key derived from the app secret at startup, so public ids survive +# restarts. A random key still keeps them unguessable until then. +_public_id_key = os.urandom(32) + + +def configure_public_id_key(secret: bytes) -> None: + """Derive the public download id key from the app's persisted secret.""" + global _public_id_key + _public_id_key = hmac.new(secret, b"shelfmark-public-download-id", hashlib.sha256).digest() + + +def public_download_id(task_id: str) -> str: + """Return the opaque, stable id a non-admin sees for ``task_id``. + + Keyed so the task id can't be recovered by hashing guesses: tracker torrent + ids are sequential, so a plain hash would be easy to reverse. + """ + digest = hmac.new(_public_id_key, task_id.encode(), hashlib.sha256).hexdigest() + return f"{PUBLIC_DOWNLOAD_ID_PREFIX}{digest[:32]}" + + +def is_public_download_id(value: object) -> bool: + """Whether ``value`` is an opaque id produced by :func:`public_download_id`.""" + return isinstance(value, str) and value.startswith(PUBLIC_DOWNLOAD_ID_PREFIX) + + +def viewer_download_id(task_id: str, request_id: object) -> str: + """Return the id a non-admin sees for a download, given its request link.""" + if normalize_positive_int(request_id) is None: + return task_id + return public_download_id(task_id) + + +def match_public_download_id(download_id: str, task_ids: Iterable[str]) -> str | None: + """Return the task among ``task_ids`` whose public id is ``download_id``.""" + if not is_public_download_id(download_id): + return None + for task_id in task_ids: + if hmac.compare_digest(public_download_id(task_id), download_id): + return task_id + return None + + +def redact_release_data(release_data: object) -> object: + """Keep only the release fields a non-admin's activity cards display.""" + if not isinstance(release_data, dict): + return release_data + return {key: value for key, value in release_data.items() if key in _RELEASE_DISPLAY_FIELDS} + + +def redact_request_row(row: dict[str, Any]) -> dict[str, Any]: + """Return a copy of a request row that is safe to show its non-admin owner.""" + if "release_data" not in row: + return row + return {**row, "release_data": redact_release_data(row["release_data"])} + + +def redact_download_payload(payload: dict[str, Any], task_id: str | None = None) -> dict[str, Any]: + """Return a copy of a download payload that is safe to show its non-admin owner.""" + redacted = dict(payload) + raw_id = task_id if task_id is not None else payload.get("id") + if isinstance(raw_id, str) and raw_id: + viewer_id = viewer_download_id(raw_id, payload.get("request_id")) + if viewer_id != raw_id: + redacted["id"] = viewer_id + # Live queue covers are proxied under the task id as their cache key. + preview = payload.get("preview") + if isinstance(preview, str): + redacted["preview"] = preview.replace( + f"/api/covers/{raw_id}?", f"/api/covers/{viewer_id}?" + ) + + download_path = payload.get("download_path") + if isinstance(download_path, str) and download_path: + redacted["download_path"] = PurePath(download_path).name or None + return redacted + + +def redact_status(status: dict[str, Any]) -> dict[str, Any]: + """Redact every download in a status dict, re-keying request-linked ones.""" + redacted: dict[str, Any] = {} + for bucket, entries in status.items(): + if not isinstance(entries, dict): + redacted[bucket] = entries + continue + bucket_entries: dict[str, Any] = {} + for task_id, payload in entries.items(): + if not isinstance(payload, dict): + bucket_entries[task_id] = payload + continue + redacted_payload = redact_download_payload(payload, task_id=task_id) + bucket_entries[viewer_download_id(task_id, payload.get("request_id"))] = ( + redacted_payload + ) + redacted[bucket] = bucket_entries + return redacted diff --git a/shelfmark/download/clients/qbittorrent.py b/shelfmark/download/clients/qbittorrent.py index c65dc772..ec68fe80 100644 --- a/shelfmark/download/clients/qbittorrent.py +++ b/shelfmark/download/clients/qbittorrent.py @@ -214,13 +214,17 @@ class QBittorrentClient(DownloadClient): self._api_key = config_text(config.get("QBITTORRENT_API_KEY", "")) # qbittorrent-api accepts either a full URL or host:port; prefer the normalized URL - # for consistency. + # for consistency. It ignores the URL's scheme and probes HTTP and HTTPS instead, + # which can settle on plain HTTP against a TLS port behind a reverse proxy (#1417), + # so an explicit https:// is kept. http:// is still probed: normalization adds it to + # a bare host:port, and the probe follows a proxy's redirect to HTTPS. self._client = Client( host=self._base_url, username=username, password=password, api_key=self._api_key or None, VERIFY_WEBUI_CERTIFICATE=get_ssl_verify(self._base_url), + FORCE_SCHEME_FROM_HOST=self._base_url.lower().startswith("https://"), ) self._category = config_text(config.get("QBITTORRENT_CATEGORY", "books")) self._download_dir = config_text(config.get("QBITTORRENT_DOWNLOAD_DIR", "")) diff --git a/shelfmark/download/clients/settings.py b/shelfmark/download/clients/settings.py index 451aec85..3c7128af 100644 --- a/shelfmark/download/clients/settings.py +++ b/shelfmark/download/clients/settings.py @@ -177,6 +177,8 @@ def _test_qbittorrent_connection(current_values: dict[str, Any] | None = None) - password=password, api_key=api_key or None, VERIFY_WEBUI_CERTIFICATE=get_ssl_verify(url), + # Same scheme rule as QBittorrentClient (#1417). + FORCE_SCHEME_FROM_HOST=url.lower().startswith("https://"), ) client.auth_log_in() api_version = client.app.web_api_version diff --git a/shelfmark/download/clients/torbox.py b/shelfmark/download/clients/torbox.py index 85e1b743..4ab689de 100644 --- a/shelfmark/download/clients/torbox.py +++ b/shelfmark/download/clients/torbox.py @@ -5,6 +5,7 @@ from __future__ import annotations import math import shutil import threading +import time from dataclasses import dataclass, field from pathlib import Path, PurePosixPath, PureWindowsPath from typing import Any, ClassVar, NoReturn @@ -36,6 +37,8 @@ _API_BASE = "https://api.torbox.app/v1/api" _API_TIMEOUT = 30 _STATUS_TIMEOUT = 15 _WORKER_JOIN_TIMEOUT = 5.0 +# TorBox can report a torrent finished shortly before its files are present. +_UNAVAILABLE_GRACE_SECONDS = 120.0 _BOOK_EXTENSIONS = ( ".aac", @@ -81,6 +84,7 @@ class _DownloadState: phase: str = "waiting_torbox" error_message: str | None = None progress: float = 0.0 + unavailable_since: float | None = None download_thread: threading.Thread | None = None cancel_event: threading.Event = field(default_factory=threading.Event) lock: threading.Lock = field(default_factory=threading.Lock) @@ -397,10 +401,11 @@ class TorBoxClient(DownloadClient): finished = torrent.get("download_finished") is True present = torrent.get("download_present") is True if finished and not present: - message = "TorBox finished processing but the download is unavailable" - self._set_error(state, message) - return DownloadStatus.error(message) - if finished and present: + if self._unavailable_grace_expired(state): + message = "TorBox finished processing but the download is unavailable" + self._set_error(state, message) + return DownloadStatus.error(message) + elif finished and present: files = torrent.get("files") if not isinstance(files, list): message = "TorBox returned no file list for a completed torrent" @@ -429,6 +434,20 @@ class TorBoxClient(DownloadClient): eta=eta, ) + @staticmethod + def _unavailable_grace_expired(state: _DownloadState) -> bool: + """Return True once finished TorBox files have stayed absent past the grace period.""" + now = time.monotonic() + with state.lock: + if state.unavailable_since is None: + state.unavailable_since = now + logger.info( + "TorBox finished torrent %s but its files are not present yet; waiting", + state.torrent_id, + extra={"torrent_id": state.torrent_id}, + ) + return now - state.unavailable_since >= _UNAVAILABLE_GRACE_SECONDS + @staticmethod def _normalize_remote_progress(value: object) -> float: """Normalize fractional or percentage TorBox progress to 0 through 100.""" diff --git a/shelfmark/download/orchestrator.py b/shelfmark/download/orchestrator.py index 841cdcaa..26201603 100644 --- a/shelfmark/download/orchestrator.py +++ b/shelfmark/download/orchestrator.py @@ -889,9 +889,12 @@ def update_download_progress(book_id: str, progress: float) -> None: if should_broadcast: task = book_queue.get_task(book_id) - task_user_id = task.user_id if task else None ws_manager.broadcast_download_progress( - book_id, progress, "downloading", user_id=task_user_id + book_id, + progress, + "downloading", + user_id=task.user_id if task else None, + request_id=task.request_id if task else None, ) diff --git a/shelfmark/main.py b/shelfmark/main.py index 40d0b3bf..479167dd 100644 --- a/shelfmark/main.py +++ b/shelfmark/main.py @@ -86,6 +86,13 @@ from shelfmark.core.requests_service import ( ) from shelfmark.core.user_db import UserDB from shelfmark.core.utils import AUDIOBOOK_FORMATS, normalize_base_path +from shelfmark.core.viewer_redaction import ( + configure_public_id_key, + is_public_download_id, + match_public_download_id, + redact_status, + viewer_download_id, +) from shelfmark.download import orchestrator as backend from shelfmark.download import warmup from shelfmark.release_sources import ( @@ -157,7 +164,14 @@ socketio = SocketIO(app, **socketio_init_kwargs) # Initialize WebSocket manager ws_manager.init_app(app, socketio) -ws_manager.set_queue_status_fn(backend.queue_status) + + +def _user_room_queue_status(user_id: int | None = None) -> dict[str, dict[str, Any]]: + """Queue status for a non-admin user room, with release details hidden (#1418).""" + return redact_status(backend.queue_status(user_id=user_id)) + + +ws_manager.set_queue_status_fn(_user_room_queue_status) logger.info("Flask-SocketIO initialized with async_mode='%s'", async_mode) logger.info("Socket.IO CORS allowed origins: %s", socketio_cors_allowed_origins) @@ -673,6 +687,7 @@ app.config.update( SESSION_COOKIE_NAME=SESSION_COOKIE_NAME, PERMANENT_SESSION_LIFETIME=604800, # 7 days in seconds ) +configure_public_id_key(app.config["SECRET_KEY"]) logger.info( "Session cookie secure setting: %s (from env: %s)", @@ -1648,6 +1663,34 @@ def _queue_task_visible_to_actor( ) +def _resolve_viewer_download_id(book_id: str) -> str: + """Map a non-admin's opaque download id back to the task it names (#1418). + + Only the caller's own request-linked downloads are searched, so an id that + doesn't resolve comes back unchanged and fails the route's usual lookup. + """ + if not is_public_download_id(book_id): + return book_id + is_admin, db_user_id, _ = _resolve_status_scope() + if is_admin or db_user_id is None: + return book_id + candidates = [ + task_id + for tasks in backend.book_queue.get_status(user_id=db_user_id).values() + for task_id, task in tasks.items() + if getattr(task, "request_id", None) is not None + ] + if download_history_service is not None: + candidates.extend(download_history_service.list_request_task_ids(user_id=db_user_id)) + return match_public_download_id(book_id, candidates) or book_id + + +def _viewer_queue_task_id(task_id: str) -> str: + """Return the id a non-admin sees for a live queue task.""" + task = backend.book_queue.get_task(task_id) + return viewer_download_id(task_id, getattr(task, "request_id", None)) + + backend.book_queue.set_queue_hook(_record_download_queued) backend.book_queue.set_terminal_status_hook(_record_download_terminal_snapshot) @@ -1701,7 +1744,7 @@ def api_status() -> Response | tuple[Response, int]: user_id=user_id, ) _emit_request_update_events(updated_requests) - return jsonify(status) + return jsonify(status if is_admin else redact_status(status)) except _OPERATIONAL_ERRORS as e: logger.error_trace(f"Status error: {e}") return jsonify({"error": str(e)}), 500 @@ -1722,6 +1765,7 @@ def api_local_download() -> Response | tuple[Response, int]: book_id = request.args.get("id", "") if not book_id: return jsonify({"error": "No book ID provided"}), 400 + book_id = _resolve_viewer_download_id(book_id) try: # The path, not the bytes: send_file streams it off disk, where reading it @@ -1845,6 +1889,8 @@ def api_cancel_download(book_id: str) -> Response | tuple[Response, int]: flask.Response: JSON status indicating success or failure. """ + viewer_book_id = book_id + book_id = _resolve_viewer_download_id(book_id) try: task = backend.book_queue.get_task(book_id) if task is None: @@ -1873,7 +1919,7 @@ def api_cancel_download(book_id: str) -> Response | tuple[Response, int]: success = backend.cancel_download(book_id) if success: - return jsonify({"status": "cancelled", "book_id": book_id}) + return jsonify({"status": "cancelled", "book_id": viewer_book_id}) return jsonify({"error": "Failed to cancel download or book not found"}), 404 except _OPERATIONAL_ERRORS as e: logger.error_trace(f"Cancel download error: {e}") @@ -1884,6 +1930,8 @@ def api_cancel_download(book_id: str) -> Response | tuple[Response, int]: @login_required def api_retry_download(book_id: str) -> Response | tuple[Response, int]: """Retry a failed download.""" + viewer_book_id = book_id + book_id = _resolve_viewer_download_id(book_id) try: task = backend.book_queue.get_task(book_id) history_row = None @@ -1948,7 +1996,7 @@ def api_retry_download(book_id: str) -> Response | tuple[Response, int]: ) if success: - return jsonify({"status": "queued", "book_id": book_id}) + return jsonify({"status": "queued", "book_id": viewer_book_id}) if error == "Download not found": return jsonify({"error": error}), 404 @@ -1985,6 +2033,8 @@ def api_set_priority(book_id: str) -> Response | tuple[Response, int]: if identity_error is not None: return identity_error, 403 + viewer_book_id = book_id + book_id = _resolve_viewer_download_id(book_id) task = backend.book_queue.get_task(book_id) if task is None: return jsonify({"error": "Failed to update priority or book not found"}), 404 @@ -1999,7 +2049,7 @@ def api_set_priority(book_id: str) -> Response | tuple[Response, int]: success = backend.set_book_priority(book_id, priority) if success: - return jsonify({"status": "updated", "book_id": book_id, "priority": priority}) + return jsonify({"status": "updated", "book_id": viewer_book_id, "priority": priority}) return jsonify({"error": "Failed to update priority or book not found"}), 404 except ValueError: return jsonify({"error": "Invalid priority value"}), 400 @@ -2039,6 +2089,10 @@ def api_reorder_queue() -> Response | tuple[Response, int]: return identity_error, 403 if not is_admin: + book_priorities = { + _resolve_viewer_download_id(str(book_id)): priority + for book_id, priority in book_priorities.items() + } owned_book_priorities = {} for book_id in book_priorities: task = backend.book_queue.get_task(str(book_id)) @@ -2077,7 +2131,7 @@ def api_queue_order() -> Response | tuple[Response, int]: return identity_error, 403 if not is_admin: queue_order = [ - item + {**item, "id": _viewer_queue_task_id(str(item.get("id", "")))} for item in queue_order if _queue_task_visible_to_actor( str(item.get("id", "")), @@ -2108,7 +2162,7 @@ def api_active_downloads() -> Response | tuple[Response, int]: return identity_error, 403 if not is_admin: active_downloads = [ - task_id + _viewer_queue_task_id(task_id) for task_id in active_downloads if _queue_task_visible_to_actor( task_id, @@ -3518,7 +3572,7 @@ def handle_connect() -> None: user_id = None if is_admin else db_user_id status = backend.queue_status(user_id=user_id) - emit("status_update", status) + emit("status_update", status if is_admin else redact_status(status)) except _OPERATIONAL_ERRORS: logger.exception("Error sending initial status") @@ -3555,7 +3609,7 @@ def handle_status_request() -> None: user_id = None if is_admin else db_user_id status = backend.queue_status(user_id=user_id) - emit("status_update", status) + emit("status_update", status if is_admin else redact_status(status)) except _OPERATIONAL_ERRORS: logger.exception("Error handling status request") emit("error", {"message": "Failed to get status"}) diff --git a/src/frontend/src/components/activity/ActivitySidebar.tsx b/src/frontend/src/components/activity/ActivitySidebar.tsx index d64f38dd..45b88b65 100644 --- a/src/frontend/src/components/activity/ActivitySidebar.tsx +++ b/src/frontend/src/components/activity/ActivitySidebar.tsx @@ -7,7 +7,7 @@ import type { RequestRecord, StatusData } from '../../types'; import { Dropdown } from '../Dropdown'; import { ActivityCard } from './ActivityCard'; import type { DownloadStatusKey } from './activityMappers'; -import { downloadToActivityItem } from './activityMappers'; +import { downloadToActivityItem, linkedDownloadIdForRequest } from './activityMappers'; import type { ActivityItem } from './activityTypes'; interface ActivitySidebarProps { @@ -146,25 +146,6 @@ const getActivityCategory = (item: ActivityItem): ActivityCategoryKey => { return 'in_progress'; }; -const getLinkedDownloadIdFromRequestItem = (item: ActivityItem): string | null => { - if (item.kind !== 'request' || item.visualStatus !== 'fulfilled') { - return null; - } - - const releaseData = item.requestRecord?.release_data; - if (!releaseData || typeof releaseData !== 'object') { - return null; - } - - const sourceId = releaseData.source_id; - if (typeof sourceId !== 'string') { - return null; - } - - const trimmed = sourceId.trim(); - return trimmed ? trimmed : null; -}; - const mergeRequestWithDownload = ( requestItem: ActivityItem, downloadItem: ActivityItem, @@ -320,9 +301,17 @@ export const ActivitySidebar = ({ const { mergedRequestItems, mergedDownloadItems } = useMemo(() => { const downloadsById = new Map(); + // downloadItems is newest first, so a retried request keeps its latest download. + const latestDownloadIdByRequestId = new Map(); downloadItems.forEach((item) => { if (item.downloadBookId) { downloadsById.set(item.downloadBookId, item); + if ( + typeof item.requestId === 'number' && + !latestDownloadIdByRequestId.has(item.requestId) + ) { + latestDownloadIdByRequestId.set(item.requestId, item.downloadBookId); + } } }); @@ -345,7 +334,7 @@ export const ActivitySidebar = ({ }); const nextRequestItems = visibleRequestItems.map((requestItem) => { - const linkedDownloadId = getLinkedDownloadIdFromRequestItem(requestItem); + const linkedDownloadId = linkedDownloadIdForRequest(requestItem, latestDownloadIdByRequestId); if (!linkedDownloadId) { return requestItem; } diff --git a/src/frontend/src/components/activity/activityMappers.ts b/src/frontend/src/components/activity/activityMappers.ts index 697c3ccf..16642696 100644 --- a/src/frontend/src/components/activity/activityMappers.ts +++ b/src/frontend/src/components/activity/activityMappers.ts @@ -188,3 +188,25 @@ export const requestToActivityItem = ( requestRecord: record, }; }; + +/** The id of the download a fulfilled request item was delivered through, if listed. */ +export const linkedDownloadIdForRequest = ( + item: ActivityItem, + latestDownloadIdByRequestId: Map, +): string | null => { + if (item.kind !== 'request' || item.visualStatus !== 'fulfilled') { + return null; + } + + const sourceId = item.requestRecord?.release_data?.source_id; + if (typeof sourceId === 'string' && sourceId.trim()) { + return sourceId.trim(); + } + + // Non-admins don't receive the release's source_id (#1418), so their request is + // matched to the download queued for it instead. + if (typeof item.requestId !== 'number') { + return null; + } + return latestDownloadIdByRequestId.get(item.requestId) ?? null; +}; diff --git a/src/frontend/src/tests/activityMappers.test.ts b/src/frontend/src/tests/activityMappers.test.ts index c13312a6..95392ec8 100644 --- a/src/frontend/src/tests/activityMappers.test.ts +++ b/src/frontend/src/tests/activityMappers.test.ts @@ -2,6 +2,7 @@ import { describe, it, expect } from 'vitest'; import { downloadToActivityItem, + linkedDownloadIdForRequest, requestToActivityItem, } from '../components/activity/activityMappers'; import type { Book, RequestRecord } from '../types/index'; @@ -191,3 +192,32 @@ describe('activityMappers.requestToActivityItem', () => { expect(item.metaLine).toBe('EPUB · 2 MB · Prowlarr'); }); }); + +const fulfilled = (releaseData: Record | null) => + requestToActivityItem( + makeRequest({ status: 'fulfilled', request_level: 'book', release_data: releaseData }), + 'user', + ); + +describe('activityMappers.linkedDownloadIdForRequest', () => { + it("links through the release's source_id when the viewer has it", () => { + const item = fulfilled({ source_id: '21:https://tracker.example/t/1', format: 'epub' }); + + expect(linkedDownloadIdForRequest(item, new Map([[42, 'dl_other']]))).toBe( + '21:https://tracker.example/t/1', + ); + }); + + it('falls back to the download queued for the request when source_id is withheld', () => { + const item = fulfilled({ format: 'epub', size: '2 MB' }); + + expect(linkedDownloadIdForRequest(item, new Map([[42, 'dl_abc']]))).toBe('dl_abc'); + expect(linkedDownloadIdForRequest(item, new Map())).toBeNull(); + }); + + it('does not link a request that has not been fulfilled', () => { + const item = requestToActivityItem(makeRequest({ status: 'pending' }), 'user'); + + expect(linkedDownloadIdForRequest(item, new Map([[42, 'dl_abc']]))).toBeNull(); + }); +}); diff --git a/tests/core/test_activity_routes_api.py b/tests/core/test_activity_routes_api.py index f65b85b4..f1381d6a 100644 --- a/tests/core/test_activity_routes_api.py +++ b/tests/core/test_activity_routes_api.py @@ -5,12 +5,14 @@ from __future__ import annotations import importlib import sqlite3 import uuid +from pathlib import Path from types import SimpleNamespace from unittest.mock import ANY, patch import pytest from shelfmark.core.models import DownloadTask, QueueStatus +from shelfmark.core.viewer_redaction import public_download_id @pytest.fixture(scope="module") @@ -165,15 +167,16 @@ class TestActivityRoutes: assert dismiss_response.status_code == 200 assert dismiss_response.json["status"] == "dismissed" + # A request-linked download reaches its non-admin owner under an opaque id (#1418). assert snapshot_response.status_code == 200 assert { "item_type": "download", - "item_key": "download:test-task", + "item_key": f"download:{public_download_id('test-task')}", } in snapshot_response.json["dismissed"] assert history_response.status_code == 200 assert len(history_response.json) == 1 - assert history_response.json[0]["item_key"] == "download:test-task" + assert history_response.json[0]["item_key"] == f"download:{public_download_id('test-task')}" assert history_response.json[0]["snapshot"]["kind"] == "download" assert history_response.json[0]["snapshot"]["download"]["title"] == "Dismiss Me" @@ -737,7 +740,7 @@ class TestActivityRoutes: assert dismiss_many_response.json["status"] == "dismissed" assert history_response.status_code == 200 assert len(history_response.json) == 1 - assert history_response.json[0]["item_key"] == f"download:{task_id}" + assert history_response.json[0]["item_key"] == f"download:{public_download_id(task_id)}" assert history_response.json[0]["snapshot"]["download"]["status_message"] == "Interrupted" assert history_response.json[0]["snapshot"]["download"]["retry_available"] is True @@ -1133,8 +1136,9 @@ class TestActivityRoutes: response = client.get("/api/activity/snapshot") assert response.status_code == 200 - assert response.json["status"]["error"][task_id]["status_message"] == "Interrupted" - assert response.json["status"]["error"][task_id]["retry_available"] is True + viewer_id = public_download_id(task_id) + assert response.json["status"]["error"][viewer_id]["status_message"] == "Interrupted" + assert response.json["status"]["error"][viewer_id]["retry_available"] is True def test_snapshot_includes_retry_available_for_live_terminal_downloads( self, main_module, client @@ -1166,9 +1170,8 @@ class TestActivityRoutes: response = client.get("/api/activity/snapshot") assert response.status_code == 200 - assert ( - response.json["status"]["error"]["retryable-terminal-task"]["retry_available"] is True - ) + viewer_id = public_download_id("retryable-terminal-task") + assert response.json["status"]["error"][viewer_id]["retry_available"] is True def test_snapshot_reopens_request_when_error_retry_is_no_longer_available( self, main_module, client @@ -1670,3 +1673,217 @@ class TestActivityRoutes: ANY, to="admins", ) + + +class TestNonAdminReleaseRedaction: + """A requester never sees the release an admin picked for them (#1418).""" + + @staticmethod + def _approved_tracker_download(main_module, tmp_path, *, user: dict) -> tuple[dict, str, Path]: + """A book-level request fulfilled from a private tracker, and its finished file.""" + source_id = f"21:https://www.myanonamouse.net/t/{uuid.uuid4().int % 10**6}" + pending = main_module.user_db.create_request( + user_id=user["id"], + content_type="audiobook", + request_level="book", + policy_mode="request_book", + book_data={ + "title": "Example Book Title", + "author": "Example Author", + "provider": "openlibrary", + "provider_id": "ol-1418", + }, + ) + # Approval attaches the release the admin picked, as fulfil_request does. + request_row = main_module.user_db.update_request( + pending["id"], + status="fulfilled", + delivery_state="complete", + release_data={ + "source": "prowlarr", + "source_id": source_id, + "indexer": "MyAnonamouse", + "info_url": source_id.split(":", 1)[1], + "title": "Example Book Title", + "format": "m4b", + "size": "300 MB", + "extra": {"info_hash": "abc123", "indexer_id": 21}, + }, + ) + book_file = ( + tmp_path / "audiobooks" / "Example Author" / "Example Book Title" / "Book Title.m4b" + ) + book_file.parent.mkdir(parents=True) + book_file.write_bytes(b"audiobook bytes") + _record_terminal_download( + main_module, + task_id=source_id, + user_id=user["id"], + username=user["username"], + title="Example Book Title", + source="prowlarr", + source_display_name="Prowlarr", + origin="requested", + request_id=request_row["id"], + download_path=str(book_file), + ) + return request_row, source_id, book_file + + def test_snapshot_hides_the_tracker_and_server_paths(self, main_module, client, tmp_path): + user = _create_user(main_module, prefix="requester") + request_row, source_id, _ = self._approved_tracker_download( + main_module, tmp_path, user=user + ) + _set_session(client, user_id=user["username"], db_user_id=user["id"], is_admin=False) + + with patch.object(main_module, "get_auth_mode", return_value="builtin"): + with patch.object( + main_module.backend, "queue_status", return_value=_sample_status_payload() + ): + response = client.get("/api/activity/snapshot") + + assert response.status_code == 200 + body = response.get_data(as_text=True) + assert "myanonamouse" not in body.lower() + assert str(tmp_path) not in body + + request = next(row for row in response.json["requests"] if row["id"] == request_row["id"]) + assert request["release_data"] == { + "source": "prowlarr", + "title": "Example Book Title", + "format": "m4b", + "size": "300 MB", + } + viewer_id = public_download_id(source_id) + download = response.json["status"]["complete"][viewer_id] + assert download["id"] == viewer_id + assert download["request_id"] == request_row["id"] + assert download["download_path"] == "Book Title.m4b" + + def test_admin_snapshot_keeps_the_release_details(self, main_module, client, tmp_path): + user = _create_user(main_module, prefix="requester") + admin = _create_user(main_module, prefix="admin", role="admin") + _, source_id, book_file = self._approved_tracker_download(main_module, tmp_path, user=user) + _set_session(client, user_id=admin["username"], db_user_id=admin["id"], is_admin=True) + + with patch.object(main_module, "get_auth_mode", return_value="builtin"): + with patch.object( + main_module.backend, "queue_status", return_value=_sample_status_payload() + ): + response = client.get("/api/activity/snapshot") + + assert response.status_code == 200 + assert response.json["status"]["complete"][source_id]["download_path"] == str(book_file) + + def test_a_direct_download_keeps_its_release_id(self, main_module, client, tmp_path): + """The user picked that release from search results showing its id already.""" + user = _create_user(main_module, prefix="reader") + book_file = tmp_path / "library" / "Direct Book.epub" + book_file.parent.mkdir(parents=True) + book_file.write_bytes(b"epub") + task_id = f"direct-{uuid.uuid4().hex[:8]}" + _record_terminal_download( + main_module, + task_id=task_id, + user_id=user["id"], + username=user["username"], + download_path=str(book_file), + ) + _set_session(client, user_id=user["username"], db_user_id=user["id"], is_admin=False) + + with patch.object(main_module, "get_auth_mode", return_value="builtin"): + with patch.object( + main_module.backend, "queue_status", return_value=_sample_status_payload() + ): + response = client.get("/api/activity/snapshot") + + download = response.json["status"]["complete"][task_id] + assert download["id"] == task_id + assert download["download_path"] == "Direct Book.epub" + + def test_live_status_is_rekeyed_for_the_requester(self, main_module, client): + user = _create_user(main_module, prefix="requester") + source_id = "21:https://www.myanonamouse.net/t/777" + queue_status_payload = _sample_status_payload() + queue_status_payload["downloading"][source_id] = { + "id": source_id, + "title": "Live Book", + "request_id": 99, + "preview": f"/api/covers/{source_id}?url=aHR0cHM6Ly9leGFtcGxlLmNvbS9jLmpwZw==", + "download_path": None, + } + _set_session(client, user_id=user["username"], db_user_id=user["id"], is_admin=False) + + with patch.object(main_module, "get_auth_mode", return_value="builtin"): + with patch.object( + main_module.backend, "queue_status", return_value=queue_status_payload + ): + response = client.get("/api/status") + + assert response.status_code == 200 + assert "myanonamouse" not in response.get_data(as_text=True).lower() + viewer_id = public_download_id(source_id) + live = response.json["downloading"][viewer_id] + assert live["id"] == viewer_id + assert live["preview"].startswith(f"/api/covers/{viewer_id}?url=") + + def test_request_list_withholds_the_release(self, main_module, client, tmp_path): + user = _create_user(main_module, prefix="requester") + request_row, _, _ = self._approved_tracker_download(main_module, tmp_path, user=user) + _set_session(client, user_id=user["username"], db_user_id=user["id"], is_admin=False) + + with patch.object(main_module, "get_auth_mode", return_value="builtin"): + response = client.get("/api/requests") + + assert response.status_code == 200 + assert "myanonamouse" not in response.get_data(as_text=True).lower() + listed = next(row for row in response.json if row["id"] == request_row["id"]) + assert "source_id" not in listed["release_data"] + + def test_localdownload_resolves_the_opaque_id_only_for_its_owner( + self, main_module, client, tmp_path + ): + owner = _create_user(main_module, prefix="requester") + other = _create_user(main_module, prefix="other") + _, source_id, _ = self._approved_tracker_download(main_module, tmp_path, user=owner) + url = f"/api/localdownload?id={public_download_id(source_id)}" + + with patch.object(main_module, "get_auth_mode", return_value="builtin"): + _set_session(client, user_id=owner["username"], db_user_id=owner["id"], is_admin=False) + owner_response = client.get(url) + _set_session(client, user_id=other["username"], db_user_id=other["id"], is_admin=False) + other_response = client.get(url) + + assert owner_response.status_code == 200 + assert owner_response.data == b"audiobook bytes" + assert other_response.status_code == 404 + + def test_dismiss_and_history_speak_the_opaque_id(self, main_module, client, tmp_path): + user = _create_user(main_module, prefix="requester") + _, source_id, _ = self._approved_tracker_download(main_module, tmp_path, user=user) + viewer_key = f"download:{public_download_id(source_id)}" + _set_session(client, user_id=user["username"], db_user_id=user["id"], is_admin=False) + + with patch.object(main_module, "get_auth_mode", return_value="builtin"): + with patch.object( + main_module.backend, "queue_status", return_value=_sample_status_payload() + ): + dismiss_response = client.post( + "/api/activity/dismiss", + json={"item_type": "download", "item_key": viewer_key}, + ) + history_response = client.get("/api/activity/history?limit=10&offset=0") + + assert dismiss_response.status_code == 200 + assert dismiss_response.json["item"]["item_key"] == viewer_key + # Stored under the real task id, so the admin view and the queue hooks still match it. + assert f"download:{source_id}" in _hidden_item_keys( + main_module, viewer_scope=f"user:{user['id']}" + ) + + assert history_response.status_code == 200 + assert "myanonamouse" not in history_response.get_data(as_text=True).lower() + entry = history_response.json[0] + assert entry["item_key"] == viewer_key + assert entry["source_id"] == public_download_id(source_id) + assert entry["snapshot"]["download"]["download_path"] == "Book Title.m4b" diff --git a/tests/core/test_viewer_redaction.py b/tests/core/test_viewer_redaction.py new file mode 100644 index 00000000..f6e176d6 --- /dev/null +++ b/tests/core/test_viewer_redaction.py @@ -0,0 +1,117 @@ +"""Tests for hiding release-source details from non-admin viewers (#1418).""" + +from __future__ import annotations + +from unittest.mock import MagicMock + +import pytest + +from shelfmark.api.websocket import WebSocketManager +from shelfmark.core import viewer_redaction +from shelfmark.core.viewer_redaction import ( + match_public_download_id, + public_download_id, + redact_download_payload, + redact_release_data, + redact_status, + viewer_download_id, +) + +_TRACKER_ID = "21:https://www.myanonamouse.net/t/123456" + + +@pytest.fixture(autouse=True) +def _restore_key(): + original = viewer_redaction._public_id_key + yield + viewer_redaction._public_id_key = original + + +def test_public_id_is_stable_opaque_and_keyed(): + viewer_redaction.configure_public_id_key(b"a" * 64) + first = public_download_id(_TRACKER_ID) + + assert first == public_download_id(_TRACKER_ID) + assert first.startswith("dl_") + assert "myanonamouse" not in first + # Keyed: a plain hash of a guessed tracker URL must not reproduce it. + viewer_redaction.configure_public_id_key(b"b" * 64) + assert public_download_id(_TRACKER_ID) != first + + +def test_only_request_linked_downloads_get_an_opaque_id(): + assert viewer_download_id(_TRACKER_ID, 7) == public_download_id(_TRACKER_ID) + assert viewer_download_id("direct-task", None) == "direct-task" + + +def test_an_opaque_id_resolves_only_among_the_given_tasks(): + viewer_id = public_download_id(_TRACKER_ID) + + assert match_public_download_id(viewer_id, ["other", _TRACKER_ID]) == _TRACKER_ID + assert match_public_download_id(viewer_id, ["other"]) is None + assert match_public_download_id(_TRACKER_ID, [_TRACKER_ID]) is None + + +def test_release_data_keeps_only_display_fields(): + redacted = redact_release_data( + { + "source": "prowlarr", + "source_id": _TRACKER_ID, + "indexer": "MyAnonamouse", + "info_url": "https://www.myanonamouse.net/t/123456", + "download_url": "https://prowlarr.local/api/v1/indexer/21/download?apikey=k", + "protocol": "torrent", + "extra": {"info_hash": "abc"}, + "title": "Book", + "format": "m4b", + "size": "300 MB", + } + ) + + assert redacted == {"source": "prowlarr", "title": "Book", "format": "m4b", "size": "300 MB"} + assert redact_release_data(None) is None + + +def test_download_payload_loses_its_path_and_tracker_id(): + payload = { + "id": _TRACKER_ID, + "request_id": 7, + "preview": f"/api/covers/{_TRACKER_ID}?url=abc", + "download_path": "/audiobooks/Example Author/Example Title/Example Title.m4b", + } + + redacted = redact_download_payload(payload) + + viewer_id = public_download_id(_TRACKER_ID) + assert redacted["id"] == viewer_id + assert redacted["preview"] == f"/api/covers/{viewer_id}?url=abc" + assert redacted["download_path"] == "Example Title.m4b" + assert payload["id"] == _TRACKER_ID # the caller's copy is untouched + + +def test_status_is_rekeyed_only_for_request_linked_downloads(): + status = { + "complete": { + _TRACKER_ID: {"id": _TRACKER_ID, "request_id": 7}, + "direct-task": {"id": "direct-task", "request_id": None}, + }, + "queued": {}, + } + + redacted = redact_status(status) + + assert set(redacted["complete"]) == {public_download_id(_TRACKER_ID), "direct-task"} + assert redacted["queued"] == {} + + +def test_progress_reaches_the_requester_under_the_opaque_id(): + manager = WebSocketManager() + socketio = MagicMock() + manager.init_app(MagicMock(), socketio) + manager._user_rooms["user_5"] = 1 + + manager.broadcast_download_progress(_TRACKER_ID, 42.0, "downloading", user_id=5, request_id=7) + + emitted = {call.kwargs["to"]: call.args[1] for call in socketio.emit.call_args_list} + assert emitted["admins"]["book_id"] == _TRACKER_ID + assert emitted["user_5"]["book_id"] == public_download_id(_TRACKER_ID) diff --git a/tests/prowlarr/test_qbittorrent_client.py b/tests/prowlarr/test_qbittorrent_client.py index 48728b6c..991cf12a 100644 --- a/tests/prowlarr/test_qbittorrent_client.py +++ b/tests/prowlarr/test_qbittorrent_client.py @@ -203,6 +203,42 @@ class TestQBittorrentClientTestConnection: assert "401" in message or "failed" in message.lower() +class TestQBittorrentClientScheme: + """qbittorrent-api probes for a scheme unless told to keep the configured one (#1417).""" + + @pytest.mark.parametrize( + ("url", "forced"), + [ + ("https://qbittorrent.example.com", True), + ("HTTPS://qbittorrent.example.com:443", True), + ("http://localhost:8080", False), + ("localhost:8080", False), + ], + ) + def test_only_an_explicit_https_scheme_is_kept(self, monkeypatch, url, forced): + """https:// must not fall back to plain HTTP; http:// and bare hosts keep the probe.""" + config_values = { + "QBITTORRENT_URL": url, + "QBITTORRENT_USERNAME": "admin", + "QBITTORRENT_PASSWORD": "password", + } + monkeypatch.setattr( + "shelfmark.download.clients.qbittorrent.config.get", + lambda key, default="": config_values.get(key, default), + ) + mock_client_class = MagicMock(return_value=MagicMock()) + + with patch.dict("sys.modules", {"qbittorrentapi": MagicMock(Client=mock_client_class)}): + import importlib + + import shelfmark.download.clients.qbittorrent as qb_module + + importlib.reload(qb_module) + qb_module.QBittorrentClient() + + assert mock_client_class.call_args.kwargs["FORCE_SCHEME_FROM_HOST"] is forced + + class TestQBittorrentClientApiKeyAuth: """Tests for API key authentication (qBittorrent 5.2.0+).""" diff --git a/tests/prowlarr/test_qbittorrent_settings.py b/tests/prowlarr/test_qbittorrent_settings.py index 6b164952..436f90a2 100644 --- a/tests/prowlarr/test_qbittorrent_settings.py +++ b/tests/prowlarr/test_qbittorrent_settings.py @@ -110,3 +110,23 @@ def test_settings_test_connection_names_rejected_credential(monkeypatch, api_key ) assert result == {"success": False, "message": f"qBittorrent rejected the {rejected}"} + + +@pytest.mark.parametrize( + ("url", "forced"), + [("https://qbittorrent.example.com", True), ("http://localhost:8080", False)], +) +def test_settings_test_connection_keeps_an_explicit_https_scheme(monkeypatch, url, forced): + """The button connects the same way downloads do, so https:// can't drop to HTTP (#1417).""" + captured = {} + _run_test_connection( + monkeypatch, + { + "QBITTORRENT_URL": url, + "QBITTORRENT_USERNAME": "admin", + "QBITTORRENT_PASSWORD": "password", + }, + fake_qbittorrentapi(captured=captured), + ) + + assert captured["FORCE_SCHEME_FROM_HOST"] is forced diff --git a/tests/prowlarr/test_torbox_client.py b/tests/prowlarr/test_torbox_client.py index 811f9428..1c92ee2d 100644 --- a/tests/prowlarr/test_torbox_client.py +++ b/tests/prowlarr/test_torbox_client.py @@ -193,13 +193,45 @@ class TestTorBoxStatus: assert state.progress == 50.0 thread.start.assert_called_once() - def test_finished_torrent_without_available_content_is_error(self, tmp_path): + def test_finished_torrent_waits_for_content_to_become_present(self, monkeypatch, tmp_path): client = TorBoxClient.__new__(TorBoxClient) state = _state(tmp_path) + start = MagicMock() + monkeypatch.setattr(client, "_maybe_start_download_thread", start) + torrent = { + "download_state": "completed", + "download_finished": True, + "download_present": False, + "progress": 1, + "files": [{"id": 1, "name": "Dune.epub"}], + } - status = client._handle_torrent_status( - {"download_finished": True, "download_present": False}, state - ) + status = client._handle_torrent_status(torrent, state) + + assert status.state == DownloadState.DOWNLOADING + assert status.progress == 50.0 + assert state.phase == "waiting_torbox" + start.assert_not_called() + + status = client._handle_torrent_status({**torrent, "download_present": True}, state) + + assert status.state == DownloadState.DOWNLOADING + start.assert_called_once_with(state, torrent["files"]) + + def test_finished_torrent_without_available_content_errors_after_grace( + self, monkeypatch, tmp_path + ): + client = TorBoxClient.__new__(TorBoxClient) + state = _state(tmp_path) + now = [1000.0] + monkeypatch.setattr("shelfmark.download.clients.torbox.time.monotonic", lambda: now[0]) + torrent = {"download_finished": True, "download_present": False} + + assert client._handle_torrent_status(torrent, state).state == DownloadState.DOWNLOADING + now[0] += 119.0 + assert client._handle_torrent_status(torrent, state).state == DownloadState.DOWNLOADING + now[0] += 1.0 + status = client._handle_torrent_status(torrent, state) assert status.state == DownloadState.ERROR assert "unavailable" in status.message