diff --git a/shelfmark/core/metadata_routes.py b/shelfmark/core/metadata_routes.py index 067134ce..ce11d82c 100644 --- a/shelfmark/core/metadata_routes.py +++ b/shelfmark/core/metadata_routes.py @@ -171,7 +171,7 @@ def register_metadata_routes( ) except _IMPORT_OPERATIONAL_ERRORS as e: logger.error_trace(f"Metadata providers error: {e}") - return jsonify({"error": str(e)}), 500 + return jsonify({"error": "Could not load the metadata providers"}), 500 @app.route("/api/metadata/config", methods=["GET"]) @login_required @@ -239,7 +239,7 @@ def register_metadata_routes( ) except _IMPORT_OPERATIONAL_ERRORS as e: logger.error_trace(f"Metadata config error: {e}") - return jsonify({"error": str(e)}), 500 + return jsonify({"error": "Could not load the metadata configuration"}), 500 @app.route("/api/metadata/search", methods=["GET"]) @login_required @@ -393,7 +393,7 @@ def register_metadata_routes( return jsonify(response_data) except _IMPORT_OPERATIONAL_ERRORS as e: logger.error_trace(f"Metadata search error: {e}") - return jsonify({"error": str(e)}), 500 + return jsonify({"error": "Metadata search failed"}), 500 @app.route("/api/metadata/field-options", methods=["GET"]) @login_required @@ -480,7 +480,7 @@ def register_metadata_routes( return jsonify({"error": str(e)}), 503 except (OSError, TypeError, sqlite3.Error) as e: logger.error_trace(f"Metadata book error: {e}") - return jsonify({"error": str(e)}), 500 + return jsonify({"error": "Could not load the book"}), 500 @app.route("/api/metadata/book///targets", methods=["GET"]) @login_required diff --git a/shelfmark/core/queue_routes.py b/shelfmark/core/queue_routes.py index ee11dae2..c5ddf233 100644 --- a/shelfmark/core/queue_routes.py +++ b/shelfmark/core/queue_routes.py @@ -346,7 +346,7 @@ def register_queue_routes( response.headers["X-Cache"] = "MISS" except _IMPORT_OPERATIONAL_ERRORS as e: logger.error_trace(f"Cover fetch error: {e}") - return jsonify({"error": str(e)}), 500 + return jsonify({"error": "Could not fetch the cover image"}), 500 else: return response @@ -398,7 +398,7 @@ def register_queue_routes( 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}") - return jsonify({"error": str(e)}), 500 + return jsonify({"error": "Could not cancel the download"}), 500 @app.route("/api/download//retry", methods=["POST"]) @login_required @@ -479,7 +479,7 @@ def register_queue_routes( return jsonify({"error": error or "Download cannot be retried"}), 409 except _OPERATIONAL_ERRORS as e: logger.error_trace(f"Retry download error: {e}") - return jsonify({"error": str(e)}), 500 + return jsonify({"error": "Could not retry the download"}), 500 @app.route("/api/queue//priority", methods=["PUT"]) @login_required @@ -531,7 +531,7 @@ def register_queue_routes( return jsonify({"error": "Invalid priority value"}), 400 except _OPERATIONAL_ERRORS as e: logger.error_trace(f"Set priority error: {e}") - return jsonify({"error": str(e)}), 500 + return jsonify({"error": "Could not set the priority"}), 500 @app.route("/api/queue/reorder", methods=["POST"]) @login_required @@ -587,7 +587,7 @@ def register_queue_routes( return jsonify({"error": "Failed to reorder queue"}), 500 except _OPERATIONAL_ERRORS as e: logger.error_trace(f"Reorder queue error: {e}") - return jsonify({"error": str(e)}), 500 + return jsonify({"error": "Could not reorder the queue"}), 500 @app.route("/api/queue/order", methods=["GET"]) @login_required @@ -617,7 +617,7 @@ def register_queue_routes( return jsonify({"queue": queue_order}) except _OPERATIONAL_ERRORS as e: logger.error_trace(f"Queue order error: {e}") - return jsonify({"error": str(e)}), 500 + return jsonify({"error": "Could not read the queue order"}), 500 @app.route("/api/downloads/active", methods=["GET"]) @login_required @@ -647,4 +647,4 @@ def register_queue_routes( return jsonify({"active_downloads": active_downloads}) except _OPERATIONAL_ERRORS as e: logger.error_trace(f"Active downloads error: {e}") - return jsonify({"error": str(e)}), 500 + return jsonify({"error": "Could not read the active downloads"}), 500 diff --git a/shelfmark/core/release_routes.py b/shelfmark/core/release_routes.py index 0d70273b..d7899752 100644 --- a/shelfmark/core/release_routes.py +++ b/shelfmark/core/release_routes.py @@ -459,7 +459,7 @@ def register_release_routes( return jsonify({"error": error_msg or "Failed to queue release"}), 500 except _OPERATIONAL_ERRORS as e: logger.error_trace(f"Release download error: {e}") - return jsonify({"error": str(e)}), 500 + return jsonify({"error": "Could not queue the release"}), 500 @app.route("/api/releases", methods=["GET"]) @login_required @@ -756,7 +756,7 @@ def register_release_routes( return jsonify({"error": str(e)}), 503 except _IMPORT_OPERATIONAL_ERRORS as e: logger.error_trace(f"Releases search error: {e}") - return jsonify({"error": str(e)}), 500 + return jsonify({"error": "Could not search for releases"}), 500 @app.route("/api/release-sources", methods=["GET"]) @login_required @@ -774,7 +774,7 @@ def register_release_routes( return jsonify(sources) except _IMPORT_OPERATIONAL_ERRORS as e: logger.error_trace(f"Release sources error: {e}") - return jsonify({"error": str(e)}), 500 + return jsonify({"error": "Could not list the release sources"}), 500 @app.route("/api/release-sources//records/", methods=["GET"]) @login_required @@ -800,4 +800,4 @@ def register_release_routes( return jsonify({"error": str(e)}), 503 except _OPERATIONAL_ERRORS as e: logger.error_trace(f"Release source record error: {e}") - return jsonify({"error": str(e)}), 500 + return jsonify({"error": "Could not resolve the record"}), 500 diff --git a/shelfmark/core/system_routes.py b/shelfmark/core/system_routes.py index cb212498..24f96405 100644 --- a/shelfmark/core/system_routes.py +++ b/shelfmark/core/system_routes.py @@ -305,7 +305,7 @@ def register_system_routes( return jsonify({"error": f"Debug script failed: {e.stderr}"}), 500 except _OPERATIONAL_ERRORS as e: logger.error_trace(f"Debug endpoint error: {e}") - return jsonify({"error": str(e)}), 500 + return jsonify({"error": "Could not create the debug file"}), 500 @app.route("/api/restart", methods=["GET"]) @admin_required diff --git a/tests/core/test_error_text_in_responses.py b/tests/core/test_error_text_in_responses.py new file mode 100644 index 00000000..792d3d1f --- /dev/null +++ b/tests/core/test_error_text_in_responses.py @@ -0,0 +1,172 @@ +"""An unexpected failure in a route returns a fixed message, never the exception text. + +The exception can carry file paths, URLs with tokens or driver messages. The full error +stays in the log with its trace; the caller gets one sentence per route. Messages that +were written for the user (a provider's "API key is invalid", a source being down) are +a different thing and are covered by the tests of those routes. +""" + +from __future__ import annotations + +import base64 +import importlib +from unittest.mock import patch + +import pytest + +_LEAK = "/srv/shelfmark/secret/path token=abc123" + + +@pytest.fixture(scope="module") +def main_module(): + """Import `shelfmark.main` with background startup disabled.""" + with patch("shelfmark.download.orchestrator.start"): + import shelfmark.main as main + + importlib.reload(main) + return main + + +@pytest.fixture +def client(main_module): + return main_module.app.test_client() + + +def _boom(*_args, **_kwargs): + raise OSError(_LEAK) + + +def _admin_session(client) -> None: + with client.session_transaction() as sess: + sess["user_id"] = "alice" + sess["is_admin"] = True + sess["db_user_id"] = 7 + + +class _Source: + def get_record(self, record_id): + raise OSError(_LEAK) + + +def _cover_url() -> str: + return "/api/covers/c1?url=" + base64.urlsafe_b64encode(b"https://example.com/c.jpg").decode() + + +def _patches(main_module, name: str): + """The patches that make route `name` fail with an OSError carrying _LEAK.""" + backend = main_module.backend + table = { + "cancel": [patch.object(backend.book_queue, "get_task", side_effect=_boom)], + "retry": [patch.object(backend.book_queue, "get_task", side_effect=_boom)], + "priority": [patch.object(backend.book_queue, "get_task", side_effect=_boom)], + "reorder": [patch.object(backend, "reorder_queue", side_effect=_boom)], + "order": [patch.object(backend, "get_queue_order", side_effect=_boom)], + "active": [patch.object(backend, "get_active_downloads", side_effect=_boom)], + "cover": [ + patch("shelfmark.config.env.is_covers_cache_enabled", return_value=True), + patch("shelfmark.core.image_cache.get_image_cache", side_effect=_boom), + ], + "release_download": [patch.object(backend, "queue_release", side_effect=_boom)], + "release_search": [ + patch("shelfmark.metadata_providers.is_provider_registered", return_value=True), + patch("shelfmark.metadata_providers.get_provider_kwargs", side_effect=_boom), + patch( + "shelfmark.release_sources.list_available_sources", + return_value=[{"name": "direct_download", "enabled": True}], + ), + patch("shelfmark.release_sources.source_results_are_releases", return_value=False), + ], + "release_sources": [ + patch("shelfmark.release_sources.list_available_sources", side_effect=_boom) + ], + "release_record": [ + patch("shelfmark.release_sources.get_source", return_value=_Source()), + ], + "metadata_providers": [patch("shelfmark.core.metadata_routes.app_config.refresh", _boom)], + "metadata_config": [patch("shelfmark.core.metadata_routes.app_config.refresh", _boom)], + "metadata_search": [ + patch("shelfmark.metadata_providers.get_configured_provider", side_effect=_boom) + ], + "metadata_book": [ + patch("shelfmark.metadata_providers.is_provider_registered", return_value=True), + patch("shelfmark.metadata_providers.get_provider_kwargs", side_effect=_boom), + ], + } + return table[name] + + +CASES = [ + ("cancel", "delete", "/api/download/t1/cancel", None, "Could not cancel the download"), + ("retry", "post", "/api/download/t1/retry", {}, "Could not retry the download"), + ("priority", "put", "/api/queue/t1/priority", {"priority": 1}, "Could not set the priority"), + ( + "reorder", + "post", + "/api/queue/reorder", + {"book_priorities": {"a": 0}}, + "Could not reorder the queue", + ), + ("order", "get", "/api/queue/order", None, "Could not read the queue order"), + ("active", "get", "/api/downloads/active", None, "Could not read the active downloads"), + ("cover", "get", _cover_url(), None, "Could not fetch the cover image"), + ( + "release_download", + "post", + "/api/releases/download", + {"source": "direct_download", "source_id": "a", "title": "Dune"}, + "Could not queue the release", + ), + ( + "release_search", + "get", + "/api/releases?provider=hardcover&book_id=1", + None, + "Could not search for releases", + ), + ("release_sources", "get", "/api/release-sources", None, "Could not list the release sources"), + ( + "release_record", + "get", + "/api/release-sources/direct_download/records/r1", + None, + "Could not resolve the record", + ), + ( + "metadata_providers", + "get", + "/api/metadata/providers", + None, + "Could not load the metadata providers", + ), + ( + "metadata_config", + "get", + "/api/metadata/config", + None, + "Could not load the metadata configuration", + ), + ("metadata_search", "get", "/api/metadata/search?query=dune", None, "Metadata search failed"), + ("metadata_book", "get", "/api/metadata/book/hardcover/1", None, "Could not load the book"), +] + + +@pytest.mark.parametrize( + ("name", "method", "path", "body", "message"), CASES, ids=[c[0] for c in CASES] +) +def test_failure_returns_a_fixed_message(main_module, client, name, method, path, body, message): + _admin_session(client) + patches = _patches(main_module, name) + kwargs = {"json": body} if body is not None else {} + + with patch.object(main_module, "get_auth_mode", return_value="builtin"): + for p in patches: + p.start() + try: + resp = getattr(client, method)(path, **kwargs) + finally: + for p in reversed(patches): + p.stop() + + assert resp.status_code == 500 + assert resp.get_json() == {"error": message} + assert _LEAK not in resp.get_data(as_text=True) diff --git a/tests/core/test_queue_routes.py b/tests/core/test_queue_routes.py index 0b2cf8b7..523fff0f 100644 --- a/tests/core/test_queue_routes.py +++ b/tests/core/test_queue_routes.py @@ -258,7 +258,7 @@ class TestCancel: "code": "user_identity_unavailable", } - def test_a_failure_in_the_backend_is_a_500_with_its_message(self, main_module, client): + def test_a_failure_in_the_backend_is_a_500_with_a_fixed_message(self, main_module, client): _login(client, is_admin=True) with _builtin(main_module): with patch.object( @@ -267,7 +267,7 @@ class TestCancel: resp = client.delete("/api/download/task-1/cancel") assert resp.status_code == 500 - assert resp.get_json() == {"error": "queue broke"} + assert resp.get_json() == {"error": "Could not cancel the download"} def test_the_opaque_id_a_viewer_sees_cancels_the_real_task(self, main_module, client): user = _create_user(main_module, prefix="reader") @@ -440,7 +440,7 @@ class TestQueueViews: assert order.status_code == active.status_code == 403 assert order.get_json()["code"] == active.get_json()["code"] == "user_identity_unavailable" - def test_a_failure_is_a_500_with_its_message(self, main_module, client): + def test_a_failure_is_a_500_with_a_fixed_message(self, main_module, client): _login(client, is_admin=True) with _builtin(main_module): with patch.object( @@ -449,7 +449,7 @@ class TestQueueViews: resp = client.get("/api/queue/order") assert resp.status_code == 500 - assert resp.get_json() == {"error": "order broke"} + assert resp.get_json() == {"error": "Could not read the queue order"} class TestStatus: