mirror of
https://github.com/calibrain/shelfmark.git
synced 2026-11-15 14:58:11 +00:00
fix(sources): keep credentials out of a source-unavailable message (#1593)
Code scanning alert 106 (stack trace exposure on the release search 503) turned out to be worth a look. Jackett sends its API key as `?apikey=` in the query string, and when Jackett is unreachable the source builds its "unavailable" message from the requests exception, whose text includes the full request URL. `/api/releases` returns that message to whoever ran the search, so any signed-in user could read the Jackett key while Jackett was down. I reproduced it with Jackett pointed at a closed port: the 503 body had `apikey=<the key>` in it. Jackett was added in #1521 and isn't in a release yet, so nothing shipped with this. `SourceUnavailableError` now masks credentials in its message with the same `redact_secrets` the log formatter uses (query parameters like `apikey=`, `user:pass@` in URLs and so on). I did it in the exception rather than at the two Jackett sites that build the text, since its message is written for the user and any other source that ends up with a key in a URL would hit the same thing. The rest of the message stays readable, for example "Max retries exceeded with url: ...?apikey=(removed)&t=book". Prowlarr and slskd send their keys in headers so they didn't leak a key, but their messages do include the internal host and port. I left that alone here. Tests: two Jackett ones (a connection error on the search and on the indexer lookup, with the key in the exception text) and three on the exception itself. The four that check masking fail on current main. The full suite, ruff and basedpyright are clean.
This commit is contained in:
1 parent
72b094db7a
commit
8b80f32476
3 files changed
+67
-1
No files matched your search
@@ -15,6 +15,7 @@ if TYPE_CHECKING:
|
||||
from shelfmark.core.search_plan import ReleaseSearchPlan
|
||||
from shelfmark.download.postprocess.packs import PackFile
|
||||
|
||||
from shelfmark.core.logger import redact_secrets
|
||||
from shelfmark.metadata_providers import BookMetadata
|
||||
|
||||
|
||||
@@ -29,7 +30,16 @@ class ReleaseProtocol(StrEnum):
|
||||
|
||||
|
||||
class SourceUnavailableError(Exception):
|
||||
"""Raised when a source is configured but currently unreachable."""
|
||||
"""Raised when a source is configured but currently unreachable.
|
||||
|
||||
The message is shown to whoever ran the search, and it is usually built from a requests
|
||||
exception whose text carries the request URL. A source that puts its key in the query
|
||||
string (Jackett's ``apikey=``) would hand that key to every user, so credentials are
|
||||
masked here for every source rather than at each place that raises.
|
||||
"""
|
||||
|
||||
def __init__(self, message: object = "") -> None:
|
||||
super().__init__(redact_secrets(str(message)))
|
||||
|
||||
|
||||
@dataclass
|
||||
|
||||
@@ -0,0 +1,22 @@
|
||||
from shelfmark.release_sources import SourceUnavailableError
|
||||
|
||||
|
||||
def test_a_credential_query_parameter_is_masked_in_the_message():
|
||||
error = SourceUnavailableError(
|
||||
"could not reach it: http://host/api?t=book&apikey=SECRETKEY123&q=x"
|
||||
)
|
||||
|
||||
assert "SECRETKEY123" not in str(error)
|
||||
assert "apikey=(removed)" in str(error)
|
||||
|
||||
|
||||
def test_userinfo_is_masked_in_the_message():
|
||||
error = SourceUnavailableError("could not reach http://user:hunter2@host:9117/api")
|
||||
|
||||
assert "hunter2" not in str(error)
|
||||
|
||||
|
||||
def test_a_plain_message_is_unchanged():
|
||||
message = "every indexer is disabled by Prowlarr after recent failures (until 12:00)"
|
||||
|
||||
assert str(SourceUnavailableError(message)) == message
|
||||
@@ -212,6 +212,40 @@ class TestSource:
|
||||
):
|
||||
JackettSource().search(_book(), _plan())
|
||||
|
||||
def test_an_unreachable_jackett_does_not_put_its_api_key_in_the_error(self, jackett_config):
|
||||
# requests puts the request URL, apikey included, in a connection error's text.
|
||||
client = JackettClient("http://jackett:9117", "SECRETKEY123")
|
||||
error = requests.exceptions.ConnectionError(
|
||||
"HTTPConnectionPool(host='jackett', port=9117): Max retries exceeded with url: "
|
||||
"/api/v2.0/indexers/all/results/torznab/api?apikey=SECRETKEY123&t=book&q=dune"
|
||||
)
|
||||
with (
|
||||
patch.object(JackettSource, "_get_client", return_value=client),
|
||||
patch.object(client._session, "get", side_effect=error),
|
||||
pytest.raises(SourceUnavailableError) as raised,
|
||||
):
|
||||
JackettSource().search(_book(), _plan())
|
||||
|
||||
assert "SECRETKEY123" not in str(raised.value)
|
||||
assert "Max retries exceeded" in str(raised.value)
|
||||
|
||||
def test_the_indexer_lookup_error_does_not_put_the_api_key_in_the_message(self, jackett_config):
|
||||
client = JackettClient("http://jackett:9117", "SECRETKEY123")
|
||||
error = requests.exceptions.ConnectionError(
|
||||
"HTTPConnectionPool(host='jackett', port=9117): Max retries exceeded with url: "
|
||||
"/api/v2.0/indexers/all/results/torznab/api?t=indexers&configured=true"
|
||||
"&apikey=SECRETKEY123"
|
||||
)
|
||||
with (
|
||||
patch.object(JackettSource, "_get_client", return_value=client),
|
||||
patch.object(client._session, "get", side_effect=error),
|
||||
pytest.raises(SourceUnavailableError) as raised,
|
||||
):
|
||||
JackettSource().search(_book(), _plan(indexers=["Alpha"]))
|
||||
|
||||
assert "SECRETKEY123" not in str(raised.value)
|
||||
assert str(raised.value).startswith("could not reach Jackett")
|
||||
|
||||
def test_auto_expand_retries_without_categories(self, jackett_config):
|
||||
jackett_config["JACKETT_AUTO_EXPAND"] = True
|
||||
client = MagicMock()
|
||||
|
||||
Reference in new issue
Block a user