From 4bff77050c35b11b446334b3321aff214aa84f26 Mon Sep 17 00:00:00 2001 From: alperien Date: Tue, 28 Jul 2026 02:17:31 +0500 Subject: [PATCH] fix: honour a review decision over the match, and keep credentials out of errors Ported from fix/robustness-audit, which found both of these but sits on a pre-release base. A decision now outranks the match result, and a Lidarr URL carrying basic-auth credentials no longer appears in error messages. Co-Authored-By: Claude --- CHANGELOG.md | 14 ++++++++ chartarr/cli.py | 28 ++++++++++++---- chartarr/lidarr.py | 48 ++++++++++++++++++++++++---- tests/test_cli.py | 59 ++++++++++++++++++++++++++++++++++ tests/test_lidarr.py | 76 ++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 211 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ebdb96f..0415f32 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,20 @@ alone. ### Fixed +- **A skip in the review screen is now honoured on a row that also + matched.** The push took the automatic match whenever there was one and + only consulted your decision otherwise, so a row that was skipped and + then matched on a later `--rematch` went to Lidarr anyway, and a re-pick + lost to the candidate the matcher had led with. A decision now outranks + the match result. A matched row carrying no release group id — possible + in a state file written by an older version — is left out instead of + failing at Lidarr's lookup. +- **A Lidarr URL with credentials in it no longer appears in error + messages.** `http://user:pw@host` is how you get through a reverse proxy + that asks for basic auth, and the whole URL was quoted back in every + connection error, timeout and proxy error page. The password is stripped + from what's printed; the request still sends it. + - **The review screen pinned a CPU core while it waited for you.** The progress screens poll for a keypress so they can notice `q` mid-run, and that non-blocking mode stayed on the window afterwards — `curses.wrapper` diff --git a/chartarr/cli.py b/chartarr/cli.py index aeb4f35..e58863e 100644 --- a/chartarr/cli.py +++ b/chartarr/cli.py @@ -352,20 +352,34 @@ def stage_review(rows, artist_col, title_col, state: State) -> None: def import_set(rows, state: State) -> list[dict]: + """the albums to push: automatic matches plus accepted review picks. + + a decision outranks the match result. review only offers uncertain + rows, but a row can be decided and then match cleanly on a --rematch, + and the state file is editable — either way the answer the user gave + is the one they meant, so an explicit skip is honoured on a matched + row and a re-pick replaces the automatic choice. + """ by_key = {r["_key"]: r for r in rows} out = [] for key, res in state.results.items(): row = by_key.get(key) if row is None: continue - if res["status"] == "matched": - out.append({"key": key, "row": row, "rgid": res["release_group_mbid"], - "artist_mbid": res.get("artist_mbid")}) + d = state.decisions.get(key) or {} + if d.get("action") == "skip": + continue + if d.get("action") == "accept": + rgid, artist_mbid = d.get("mbid"), d.get("artist_mbid") + elif res["status"] == "matched": + rgid, artist_mbid = res.get("release_group_mbid"), res.get("artist_mbid") else: - d = state.decisions.get(key) - if d and d.get("action") == "accept": - out.append({"key": key, "row": row, "rgid": d["mbid"], - "artist_mbid": d.get("artist_mbid")}) + continue + # an older state file can carry a matched row with no id; pushing + # it would fail at lidarr's lookup, so leave it out + if rgid: + out.append({"key": key, "row": row, "rgid": rgid, + "artist_mbid": artist_mbid}) return out diff --git a/chartarr/lidarr.py b/chartarr/lidarr.py index 623f3ea..92ed0af 100644 --- a/chartarr/lidarr.py +++ b/chartarr/lidarr.py @@ -13,6 +13,7 @@ import json import time +import urllib.parse import requests @@ -26,6 +27,27 @@ def __init__(self, message: str, status: int = 0, body: str = ""): self.body = body +def _safe_url(url: str) -> str: + """the url with any user:password@ removed, for showing in messages. + + a lidarr url can carry basic-auth credentials — behind a reverse proxy + that asks for them, http://user:pw@host is how you get through. those + end up in every connection error otherwise, and errors get pasted into + bug reports. + """ + try: + parts = urllib.parse.urlsplit(url) + host = parts.hostname + except ValueError: + return url + if not host: + return url # nothing parsed as a host; nothing to hide + if parts.port: + host = f"{host}:{parts.port}" + return urllib.parse.urlunsplit( + (parts.scheme, host, parts.path, parts.query, parts.fragment)) + + def _is_duplicate(status: int, body: str) -> bool: """true if lidarr is saying "this album is already here". @@ -59,42 +81,54 @@ def _is_duplicate(status: int, body: str) -> bool: class Lidarr: def __init__(self, url: str, api_key: str): self.base = url.rstrip("/") + # what the user sees in errors: the same address minus any + # credentials, so a pasted traceback doesn't carry a password + self.shown = _safe_url(self.base) self.s = requests.Session() self.s.headers["X-Api-Key"] = api_key + def _scrub(self, text) -> str: + """text with this instance's url swapped for the credential-free one.""" + out = str(text) + return out.replace(self.base, self.shown) if self.base != self.shown else out + def _call(self, path: str, method: str = "GET", **kw): url = f"{self.base}/api/v1/{path}" try: r = self.s.request(method, url, timeout=60, **kw) except requests.ConnectionError as e: raise LidarrError( - f"Can't reach Lidarr at {self.base} — is it running, and is the " + f"Can't reach Lidarr at {self.shown} — is it running, and is the " f"URL right? (the address you use in your browser)") from e except requests.Timeout as e: - raise LidarrError(f"Lidarr at {self.base} timed out.") from e + raise LidarrError(f"Lidarr at {self.shown} timed out.") from e except (requests.exceptions.InvalidSchema, requests.exceptions.MissingSchema, requests.exceptions.InvalidURL) as e: raise LidarrError( - f"{self.base} is not a URL Lidarr can be reached at — it needs " + f"{self.shown} is not a URL Lidarr can be reached at — it needs " f"to start with http:// or https://") from e except requests.RequestException as e: - raise LidarrError(f"Lidarr request failed: {e}") from e + # requests quotes the url it was given, credentials and all + raise LidarrError( + f"Lidarr request to {self.shown} failed: {self._scrub(e)}") from e if r.status_code == 401: raise LidarrError( "Lidarr rejected the API key (401). Copy it from " "Settings → General → Security → API Key.", status=401) if r.status_code >= 400: - raise LidarrError(f"HTTP {r.status_code}: {r.text[:300] or 'no details'}", - status=r.status_code, body=r.text) + # a proxy's error page can quote the request url back at us + raise LidarrError( + f"HTTP {r.status_code}: {self._scrub(r.text)[:300] or 'no details'}", + status=r.status_code, body=r.text) if not r.text: return None try: return r.json() except ValueError as e: raise LidarrError( - f"Lidarr returned something that isn't JSON — is {self.base} " + f"Lidarr returned something that isn't JSON — is {self.shown} " f"really Lidarr, and not a login page or another service?") from e def status(self) -> dict: diff --git a/tests/test_cli.py b/tests/test_cli.py index f1574b2..908d79c 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -437,6 +437,65 @@ def test_without_the_search_flag_nothing_is_searched( assert api.searched == [] +def test_a_skip_is_honoured_even_on_a_row_that_matched(tmp_path): + # a row can match cleanly on a --rematch after being decided, and the + # state file is editable; the answer the user gave is the one they meant + state = State(tmp_path / "s.jsonl") + state.add_result("k", {"status": "matched", "release_group_mbid": "rg-auto", + "artist_mbid": "am"}) + state.add_decision("k", {"action": "skip"}) + rows = [{"_key": "k", "artist": "a", "title": "t"}] + assert cli.import_set(rows, state) == [] + + +def test_a_repick_replaces_the_automatic_choice(tmp_path): + state = State(tmp_path / "s.jsonl") + state.add_result("k", {"status": "matched", "release_group_mbid": "rg-auto", + "artist_mbid": "am-auto"}) + state.add_decision("k", {"action": "accept", "mbid": "rg-user", + "artist_mbid": "am-user"}) + rows = [{"_key": "k", "artist": "a", "title": "t"}] + items = cli.import_set(rows, state) + assert [(i["rgid"], i["artist_mbid"]) for i in items] == [("rg-user", "am-user")] + + +def test_an_undone_decision_falls_back_to_the_match(tmp_path): + # u in the review screen writes a clear record; the row should go back + # to whatever the matcher said rather than dropping out of the push + state = State(tmp_path / "s.jsonl") + state.add_result("k", {"status": "matched", "release_group_mbid": "rg-auto", + "artist_mbid": "am"}) + state.add_decision("k", {"action": "skip"}) + state.add_decision("k", {"action": "clear"}) + rows = [{"_key": "k", "artist": "a", "title": "t"}] + assert [i["rgid"] for i in cli.import_set(rows, state)] == ["rg-auto"] + + +def test_a_matched_row_with_no_release_group_is_left_out(tmp_path): + # an older state file can hold one; pushing it dies at lidarr's lookup + state = State(tmp_path / "s.jsonl") + state.add_result("k", {"status": "matched"}) + rows = [{"_key": "k", "artist": "a", "title": "t"}] + assert cli.import_set(rows, state) == [] + + +def test_an_accepted_review_row_still_pushes(tmp_path): + # the ordinary path, unchanged: uncertain row, user picks a candidate + state = State(tmp_path / "s.jsonl") + state.add_result("k", {"status": "review", "release_group_mbid": "rg-guess"}) + state.add_decision("k", {"action": "accept", "mbid": "rg-picked", + "artist_mbid": "am"}) + rows = [{"_key": "k", "artist": "a", "title": "t"}] + assert [i["rgid"] for i in cli.import_set(rows, state)] == ["rg-picked"] + + +def test_an_undecided_review_row_is_not_pushed(tmp_path): + state = State(tmp_path / "s.jsonl") + state.add_result("k", {"status": "review", "release_group_mbid": "rg-guess"}) + rows = [{"_key": "k", "artist": "a", "title": "t"}] + assert cli.import_set(rows, state) == [] + + def test_rematch_clears_only_the_rows_nothing_was_found_for(tmp_path): state = State(tmp_path / "s.jsonl") state.add_result("miss", {"status": "not_found"}) diff --git a/tests/test_lidarr.py b/tests/test_lidarr.py index 444e95c..6bbf775 100644 --- a/tests/test_lidarr.py +++ b/tests/test_lidarr.py @@ -285,3 +285,79 @@ def test_html_login_page_is_not_mistaken_for_lidarr(): body="Sign in to continue") with pytest.raises(LidarrError, match="really Lidarr"): api().status() + + +# --- credentials in the url stay out of the error text --- + +CREDS = "http://admin:hunter2@lidarr.test:8686" +CREDS_API = CREDS + "/api/v1" + + +def creds_api(): + return Lidarr(CREDS, "sekrit") + + +def test_safe_url_drops_only_the_credentials(): + assert lidarr._safe_url(CREDS) == BASE + assert lidarr._safe_url("http://:tok3n@lidarr.test:8686") == BASE + # everything else about the url survives untouched + assert lidarr._safe_url("https://u:p@host/base?x=1#f") == "https://host/base?x=1#f" + assert lidarr._safe_url("http://lidarr.test:8686") == "http://lidarr.test:8686" + # a default port isn't invented, and unparseable input is left alone + assert lidarr._safe_url("http://lidarr.test") == "http://lidarr.test" + assert lidarr._safe_url("not a url") == "not a url" + + +@responses.activate +def test_connection_error_does_not_echo_the_password(): + responses.add(responses.GET, CREDS_API + "/system/status", + body=requests.ConnectionError("refused")) + with pytest.raises(LidarrError) as e: + creds_api().status() + assert "hunter2" not in str(e.value) + assert "lidarr.test:8686" in str(e.value) # still says where it tried + + +@responses.activate +def test_timeout_does_not_echo_the_password(): + responses.add(responses.GET, CREDS_API + "/system/status", + body=requests.Timeout("60s")) + with pytest.raises(LidarrError) as e: + creds_api().status() + assert "hunter2" not in str(e.value) + + +@responses.activate +def test_a_generic_request_failure_does_not_echo_the_password(): + # requests puts the url it was handed into its own message + responses.add(responses.GET, CREDS_API + "/system/status", + body=requests.TooManyRedirects(f"too many redirects for {CREDS}")) + with pytest.raises(LidarrError) as e: + creds_api().status() + assert "hunter2" not in str(e.value) + + +@responses.activate +def test_a_proxy_error_page_quoting_the_url_is_scrubbed(): + responses.add(responses.GET, CREDS_API + "/system/status", status=502, + body=f"Bad gateway while proxying to {CREDS}/api/v1/system/status") + with pytest.raises(LidarrError) as e: + creds_api().status() + assert "hunter2" not in str(e.value) + + +@responses.activate +def test_non_json_reply_does_not_echo_the_password(): + responses.add(responses.GET, CREDS_API + "/system/status", status=200, + body="login") + with pytest.raises(LidarrError) as e: + creds_api().status() + assert "hunter2" not in str(e.value) + + +@responses.activate +def test_the_credentials_are_still_sent_to_lidarr(): + # scrubbing is for the message only; the request must keep working + responses.add(responses.GET, CREDS_API + "/system/status", json={"version": "3"}) + assert creds_api().status() == {"version": "3"} + assert "admin:hunter2" in responses.calls[0].request.url