From 0f3a06bc9c176d131032b745e6eb73c38efc3997 Mon Sep 17 00:00:00 2001 From: Alex <25013571+alexhb1@users.noreply.github.com> Date: Fri, 27 Feb 2026 21:06:39 +0000 Subject: [PATCH] Fix: User DB hardening and apprise tweak (#668) --- shelfmark/core/activity_routes.py | 69 +++++++++++++++++++++++--- shelfmark/core/notifications.py | 38 ++++++++++---- tests/core/test_activity_routes_api.py | 59 ++++++++++++++++++++++ 3 files changed, 148 insertions(+), 18 deletions(-) diff --git a/shelfmark/core/activity_routes.py b/shelfmark/core/activity_routes.py index c58d070..6d9cd24 100644 --- a/shelfmark/core/activity_routes.py +++ b/shelfmark/core/activity_routes.py @@ -24,7 +24,11 @@ def _require_authenticated(resolve_auth_mode: Callable[[], str]): return None -def _resolve_db_user_id(require_in_auth_mode: bool = True): +def _resolve_db_user_id( + require_in_auth_mode: bool = True, + *, + user_db: UserDB | None = None, +): raw_db_user_id = session.get("db_user_id") if raw_db_user_id is None: if not require_in_auth_mode: @@ -39,8 +43,10 @@ def _resolve_db_user_id(require_in_auth_mode: bool = True): 403, ) try: - return int(raw_db_user_id), None + parsed_db_user_id = int(raw_db_user_id) except (TypeError, ValueError): + if not require_in_auth_mode: + return None, None return None, ( jsonify( { @@ -51,6 +57,40 @@ def _resolve_db_user_id(require_in_auth_mode: bool = True): 403, ) + if parsed_db_user_id < 1: + if not require_in_auth_mode: + return None, None + return None, ( + jsonify( + { + "error": "User identity unavailable for activity workflow", + "code": "user_identity_unavailable", + } + ), + 403, + ) + + if user_db is not None: + try: + db_user = user_db.get_user(user_id=parsed_db_user_id) + except Exception as exc: + logger.warning("Failed to validate activity db identity %s: %s", parsed_db_user_id, exc) + db_user = None + if db_user is None: + if not require_in_auth_mode: + return None, None + return None, ( + jsonify( + { + "error": "User identity unavailable for activity workflow", + "code": "user_identity_unavailable", + } + ), + 403, + ) + + return parsed_db_user_id, None + def _ensure_no_auth_activity_user_id(user_db: UserDB) -> int | None: """Resolve a stable users.db identity for no-auth activity state.""" @@ -83,14 +123,23 @@ def _resolve_activity_actor_user_id( resolve_auth_mode: Callable[[], str], ) -> tuple[int | None, Any | None]: """Resolve acting user identity for activity mutations.""" - db_user_id, db_gate = _resolve_db_user_id() - if db_user_id is not None: - return db_user_id, None - if resolve_auth_mode() == "none": no_auth_user_id = _ensure_no_auth_activity_user_id(user_db) if no_auth_user_id is not None: return no_auth_user_id, None + return None, ( + jsonify( + { + "error": "User identity unavailable for activity workflow", + "code": "user_identity_unavailable", + } + ), + 403, + ) + + db_user_id, db_gate = _resolve_db_user_id(user_db=user_db) + if db_user_id is not None: + return db_user_id, None return None, db_gate @@ -360,9 +409,13 @@ def register_activity_routes( 403, ) - viewer_db_user_id, _ = _resolve_db_user_id(require_in_auth_mode=False) - if viewer_db_user_id is None and resolve_auth_mode() == "none": + if resolve_auth_mode() == "none": viewer_db_user_id = _ensure_no_auth_activity_user_id(user_db) + else: + viewer_db_user_id, _ = _resolve_db_user_id( + require_in_auth_mode=False, + user_db=user_db, + ) scoped_user_id = None if is_admin else db_user_id status = queue_status(user_id=scoped_user_id) updated_requests = sync_request_delivery_states( diff --git a/shelfmark/core/notifications.py b/shelfmark/core/notifications.py index 201589f..f822a4e 100644 --- a/shelfmark/core/notifications.py +++ b/shelfmark/core/notifications.py @@ -74,6 +74,13 @@ def _normalize_urls(value: Any) -> list[str]: seen: set[str] = set() for raw_url in raw_values: url = str(raw_url or "").strip() + if not url: + continue + # Strip invisible/non-ASCII characters that can sneak in via copy-paste + # (zero-width spaces, smart quotes, non-breaking spaces, etc.). + # These pass Apprise URL validation but cause UnicodeEncodeError when + # requests tries to latin-1 encode credentials for Basic Auth headers. + url = url.encode("ascii", errors="ignore").decode("ascii").strip() if not url: continue if url in seen: @@ -98,7 +105,7 @@ def _extract_url_schemes(urls: Iterable[str]) -> list[str]: class _AppriseLogCapture(logging.Handler): def __init__(self, *, thread_id: int): super().__init__(level=logging.INFO) - self.records: list[tuple[int, str, str]] = [] + self.records: list[tuple[int, str, str, str]] = [] self._thread_id = thread_id def emit(self, record: logging.LogRecord) -> None: @@ -107,11 +114,19 @@ class _AppriseLogCapture(logging.Handler): message = record.getMessage() if message: - self.records.append((record.levelno, record.name, str(message))) + exception_summary = "" + if record.exc_info and record.exc_info[0]: + exc_type = getattr(record.exc_info[0], "__name__", "Exception") + exc = record.exc_info[1] + exception_summary = f"{exc_type}: {exc}" + elif record.exc_text: + exception_summary = str(record.exc_text).strip() + + self.records.append((record.levelno, record.name, str(message), exception_summary)) @contextmanager -def _capture_apprise_logs(*, min_level: int = logging.INFO) -> Iterator[list[tuple[int, str, str]]]: +def _capture_apprise_logs(*, min_level: int = logging.INFO) -> Iterator[list[tuple[int, str, str, str]]]: apprise_logger = logging.getLogger(_APPRISE_LOGGER_NAME) previous_level = apprise_logger.level handler = _AppriseLogCapture(thread_id=threading.get_ident()) @@ -127,22 +142,25 @@ def _capture_apprise_logs(*, min_level: int = logging.INFO) -> Iterator[list[tup apprise_logger.setLevel(previous_level) -def _log_apprise_records(records: Iterable[tuple[int, str, str]]) -> None: - seen: set[tuple[int, str, str]] = set() - for level, source, raw_message in records: +def _log_apprise_records(records: Iterable[tuple[int, str, str, str]]) -> None: + seen: set[tuple[int, str, str, str]] = set() + for level, source, raw_message, raw_exception_summary in records: message = str(raw_message or "").strip() source_name = str(source or "").strip() or _APPRISE_LOGGER_NAME - key = (int(level), source_name, message) + exception_summary = str(raw_exception_summary or "").strip() + key = (int(level), source_name, message, exception_summary) if not message or key in seen: continue seen.add(key) + full_message = message if not exception_summary else f"{message} ({exception_summary})" + if level >= logging.ERROR: - logger.error("Apprise source [%s]: %s", source_name, message) + logger.error("Apprise source [%s]: %s", source_name, full_message) elif level >= logging.WARNING: - logger.warning("Apprise source [%s]: %s", source_name, message) + logger.warning("Apprise source [%s]: %s", source_name, full_message) else: - logger.info("Apprise source [%s]: %s", source_name, message) + logger.info("Apprise source [%s]: %s", source_name, full_message) def _normalize_routes(value: Any) -> list[dict[str, str]]: diff --git a/tests/core/test_activity_routes_api.py b/tests/core/test_activity_routes_api.py index 7301801..a978177 100644 --- a/tests/core/test_activity_routes_api.py +++ b/tests/core/test_activity_routes_api.py @@ -243,6 +243,65 @@ class TestActivityRoutes: assert history_one.status_code == 200 assert any(row["item_key"] == item_key for row in history_one.json) + def test_no_auth_dismiss_many_ignores_stale_session_db_identity(self, main_module, client): + stale_db_user_id = 999999999 + _set_session(client, user_id="stale-session-user", db_user_id=stale_db_user_id, is_admin=False) + + item_key = f"download:no-auth-stale-{uuid.uuid4().hex[:8]}" + with patch.object(main_module, "get_auth_mode", return_value="none"): + response = client.post( + "/api/activity/dismiss-many", + json={"items": [{"item_type": "download", "item_key": item_key}]}, + ) + + assert response.status_code == 200 + assert response.json["status"] == "dismissed" + + no_auth_user = main_module.user_db.get_user(username="__shelfmark_noauth_activity__") + assert no_auth_user is not None + dismissals = main_module.activity_service.get_dismissal_set(no_auth_user["id"]) + assert {"item_type": "download", "item_key": item_key} in dismissals + + def test_no_auth_dismiss_many_uses_shared_identity_even_with_valid_session_db_user( + self, + main_module, + client, + ): + existing_user = _create_user(main_module, prefix="legacy-reader") + _set_session( + client, + user_id=existing_user["username"], + db_user_id=existing_user["id"], + is_admin=False, + ) + + item_key = f"download:no-auth-valid-{uuid.uuid4().hex[:8]}" + other_client = main_module.app.test_client() + + with patch.object(main_module, "get_auth_mode", return_value="none"): + dismiss_response = client.post( + "/api/activity/dismiss-many", + json={"items": [{"item_type": "download", "item_key": item_key}]}, + ) + with patch.object(main_module.backend, "queue_status", return_value=_sample_status_payload()): + snapshot_response = other_client.get("/api/activity/snapshot") + + assert dismiss_response.status_code == 200 + assert snapshot_response.status_code == 200 + assert {"item_type": "download", "item_key": item_key} in snapshot_response.json["dismissed"] + + def test_dismiss_many_with_stale_db_identity_returns_identity_unavailable(self, main_module, client): + _set_session(client, user_id="stale-session-user", db_user_id=999999999, is_admin=False) + + with patch.object(main_module, "get_auth_mode", return_value="builtin"): + response = client.post( + "/api/activity/dismiss-many", + json={"items": [{"item_type": "download", "item_key": "download:test-stale"}]}, + ) + + assert response.status_code == 403 + assert response.json["code"] == "user_identity_unavailable" + def test_queue_clear_does_not_set_request_delivery_state_to_cleared(self, main_module, client): user = _create_user(main_module, prefix="reader") _set_session(client, user_id=user["username"], db_user_id=user["id"], is_admin=False)