From 9d8e7eaad2fde14737057d2ba467263dd605f07c Mon Sep 17 00:00:00 2001 From: splitsec2 <35583321+splitsec2@users.noreply.github.com> Date: Mon, 2 Nov 2026 10:21:08 -0600 Subject: [PATCH] fix: keep exception text out of the queue, release, metadata and debug responses (#1643) Now that the route moves have landed, this takes care of the code scanning alerts they brought along. Sixteen places in `queue_routes.py`, `release_routes.py`, `metadata_routes.py` and `system_routes.py` handed `str(e)` of an unexpected OSError, RuntimeError, TypeError or sqlite3 error back as the 500 body. It can carry file paths, URLs with tokens or driver text. Same idea as #1558 and #1562. Each now returns one fixed sentence ("Could not cancel the download", "Could not read the queue order", "Metadata search failed" and so on) and the full error and trace stay in the log. The routes covered are the cover proxy, cancel, retry, priority, reorder, queue order and active downloads; release download, search, source list and source record; the metadata providers, config, search and book routes; and the debug endpoint. I left alone the messages that are written for the person using the app: what a metadata provider raises as a ValueError or RuntimeError ("Hardcover API key is invalid"), `SourceUnavailableError`, and the reason `queue_release` gives. CodeQL still flags those, as I read it those are the ones that should be dismissed as intended, but that is your call. The same pattern is in the settings and onboarding routes, which aren't flagged, so I didn't touch them. There are 15 new tests, one per fixed route except the debug endpoint, which only exists in debug mode. All 15 fail without the change. Two queue tests that asserted the old text now assert the fixed message. The full suite, ruff and basedpyright are clean. @calibrain I pushed through the refactors keeping them the same functionality, this addresses "issues" flagged in them, I think some of these were dismissed before, but I gathered them all up here in a PR that you should have a say on as it is changing long standing code. ## Summary by CodeRabbit * **Bug Fixes** * Error responses now show generic messages instead of operational exception details across metadata, queue, release, and debug endpoints. HTTP status codes and error logging remain unchanged. --- shelfmark/core/metadata_routes.py | 8 +- shelfmark/core/queue_routes.py | 14 +- shelfmark/core/release_routes.py | 8 +- shelfmark/core/system_routes.py | 2 +- tests/core/test_error_text_in_responses.py | 172 +++++++++++++++++++++ tests/core/test_queue_routes.py | 8 +- 6 files changed, 192 insertions(+), 20 deletions(-) create mode 100644 tests/core/test_error_text_in_responses.py 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: