From 4f97fc9beca58940afbd0994c6a4e22bdbd49a82 Mon Sep 17 00:00:00 2001
From: splitsec2 <35583321+splitsec2@users.noreply.github.com>
Date: Fri, 23 Oct 2026 01:52:39 -0600
Subject: [PATCH] fix: keep server paths and URLs out of an error response and
 a debug log (#1558)

Two small leaks from going through the open CodeQL alerts (code scanning
alerts 100 and 123).

The local download route returned `str(e)` on an OSError, and for a
missing or unreadable file that text is the file's path on the server
(`[Errno 2] No such file or directory:
'/audiobooks/Author/Title.epub'`). #1422 cuts a non-admin's download
path down to the file name everywhere else, so this put it back. The
route now answers "Could not read the file" and the detail stays in the
log.

`should_bypass_proxy` logged the whole request URL at debug when it
could not be parsed. URLs can carry credentials, so it logs only the
exception now.

Each change has a test that fails without it. The full non-e2e suite,
ruff and basedpyright pass.
---
 shelfmark/download/network.py                 |  2 +-
 shelfmark/main.py                             |  4 +-
 tests/core/test_activity_routes_api.py        | 38 +++++++++++++++++++
 .../download/test_network_proxy_selection.py  | 16 ++++++++
 4 files changed, 58 insertions(+), 2 deletions(-)

diff --git a/shelfmark/download/network.py b/shelfmark/download/network.py
index 11427813..f124c266 100644
--- a/shelfmark/download/network.py
+++ b/shelfmark/download/network.py
@@ -53,7 +53,7 @@ def should_bypass_proxy(url: str) -> bool:
         parsed = urllib.parse.urlparse(url)
         hostname = (parsed.hostname or "").lower()
     except ValueError as e:
-        logger.debug("Failed to parse URL for proxy bypass check: %s - %s", url, e)
+        logger.debug("Failed to parse URL for proxy bypass check: %s", e)
         return False
 
     if not hostname:
diff --git a/shelfmark/main.py b/shelfmark/main.py
index 72f2c642..aeb63620 100644
--- a/shelfmark/main.py
+++ b/shelfmark/main.py
@@ -2022,8 +2022,10 @@ def api_local_download() -> Response | tuple[Response, int]:
         return send_file(file_paths[0], download_name=file_name, as_attachment=True)
 
     except _OPERATIONAL_ERRORS as e:
+        # The details are logged. The message of an OSError names the file's path on the server,
+        # which a non-admin never sees anywhere else.
         logger.error_trace(f"Local download error: {e}")
-        return jsonify({"error": str(e)}), 500
+        return jsonify({"error": "Could not read the file"}), 500
 
 
 @app.route("/api/covers/<cover_id>", methods=["GET"])
diff --git a/tests/core/test_activity_routes_api.py b/tests/core/test_activity_routes_api.py
index c5f262af..ac63ba57 100644
--- a/tests/core/test_activity_routes_api.py
+++ b/tests/core/test_activity_routes_api.py
@@ -357,6 +357,44 @@ class TestActivityRoutes:
         assert response.status_code == 200
         assert response.data == file_bytes
 
+    def test_localdownload_error_does_not_reveal_the_server_path(
+        self, main_module, client, tmp_path
+    ):
+        """A failed read answers with a fixed message, not the OSError text and its path."""
+        user = _create_user(main_module, prefix="reader")
+        _set_session(client, user_id=user["username"], db_user_id=user["id"], is_admin=False)
+
+        task_id = f"live-localdownload-{uuid.uuid4().hex[:8]}"
+        file_path = tmp_path / "Hidden Author" / "Hidden Title.epub"
+        file_path.parent.mkdir()
+        file_path.write_bytes(b"payload")
+
+        task = DownloadTask(
+            task_id=task_id,
+            source="direct_download",
+            title="Live Local Download",
+            user_id=user["id"],
+            username=user["username"],
+            download_path=str(file_path),
+        )
+        assert main_module.backend.book_queue.add(task) is True
+
+        failure = FileNotFoundError(2, "No such file or directory", str(file_path))
+        try:
+            main_module.backend.book_queue.update_status(task_id, QueueStatus.COMPLETE)
+            with (
+                patch.object(main_module, "get_auth_mode", return_value="builtin"),
+                patch.object(main_module, "send_file", side_effect=failure),
+            ):
+                response = client.get(f"/api/localdownload?id={task_id}")
+        finally:
+            main_module.backend.book_queue.cancel_download(task_id)
+
+        assert response.status_code == 500
+        assert response.get_json() == {"error": "Could not read the file"}
+        assert "Hidden" not in response.get_data(as_text=True)
+        assert str(tmp_path) not in response.get_data(as_text=True)
+
     def test_localdownload_zips_a_live_multi_file_download(self, main_module, client, tmp_path):
         user = _create_user(main_module, prefix="reader")
         _set_session(client, user_id=user["username"], db_user_id=user["id"], is_admin=False)
diff --git a/tests/download/test_network_proxy_selection.py b/tests/download/test_network_proxy_selection.py
index c0bb7ba3..9200ce18 100644
--- a/tests/download/test_network_proxy_selection.py
+++ b/tests/download/test_network_proxy_selection.py
@@ -59,3 +59,19 @@ def test_get_proxies_returns_socks_proxy_for_both_schemes(monkeypatch):
         "http": "socks5://proxy.local:1080",
         "https": "socks5://proxy.local:1080",
     }
+
+
+def test_a_url_that_fails_to_parse_is_not_written_to_the_log(monkeypatch):
+    """The proxy check logs why a URL did not parse, never the URL itself."""
+    from unittest.mock import patch
+
+    network = _set_proxy_config(monkeypatch, NO_PROXY="localhost")
+    # An unterminated IPv6 literal makes urlparse raise ValueError.
+    url = "http://user:s3cret@[::1/path?apikey=s3cret"
+
+    with patch.object(network.logger, "debug") as debug:
+        assert network.should_bypass_proxy(url) is False
+
+    logged = " ".join(str(arg) for call in debug.call_args_list for arg in call.args)
+    assert "Failed to parse URL" in logged
+    assert "s3cret" not in logged