From 8b064c96f25dcc4e9db08a4bde215089aaf92630 Mon Sep 17 00:00:00 2001 From: Alex Date: Thu, 17 Sep 2026 13:34:59 +0100 Subject: [PATCH] fix: keep credentials and 5000-digit paths out of the Redis URL errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _redis_urls raises before any check runs and cli.main prints what it raises, so every one of its five messages interpolated the URL it was given — password and all — straight to stderr. They render it through _endpoint now. _endpoint itself then echoed the whole path, so an over-long database number came back at full length: 5132 characters of error for one bad setting. It caps the path it renders, which protects every caller, since that string is written into terminals, CI logs and error messages rather than reused as a URL. --- docsgpt/deploy/commands.py | 18 ++++++++++++------ tests/deploy/test_doctor.py | 22 ++++++++++++++++++++++ 2 files changed, 34 insertions(+), 6 deletions(-) diff --git a/docsgpt/deploy/commands.py b/docsgpt/deploy/commands.py index 25a7d9a0..295ae0ac 100644 --- a/docsgpt/deploy/commands.py +++ b/docsgpt/deploy/commands.py @@ -215,30 +215,32 @@ def _redis_urls(base: str) -> dict[str, str]: parts = urlsplit(base) except ValueError as exc: # urlsplit raises on things like redis://[::1 ; that is a typo, not a crash. - raise DeployError(f"the Redis URL {base!r} could not be read: {exc}") from exc + raise DeployError(f"the Redis URL could not be read: {_scrub(str(exc), base)}") from exc try: port = parts.port # a non-numeric or out-of-range port raises here, not when the URL is split except ValueError as exc: raise DeployError( - f"the Redis URL {base!r} has an unusable port: {exc}. Pass a URL like " + f"the Redis URL ({_endpoint(base)}) has an unusable port: {_scrub(str(exc), base)}. " + "Pass a URL like " "redis://host:6379 or redis://host:6379/5." ) from exc if port == 0: # urlsplit is happy with it, since 0 is inside the range, but nothing can connect to it. raise DeployError( - f"the Redis URL {base!r} has an unusable port: 0. Pass a URL like " + f"the Redis URL ({_endpoint(base)}) has an unusable port: 0. Pass a URL like " "redis://host:6379 or redis://host:6379/5." ) if parts.scheme not in ("redis", "rediss"): raise DeployError( - f"the Redis URL {base!r} should start with redis:// or rediss://, with any options as " + f"the Redis URL ({_endpoint(base)}) should start with redis:// or rediss://, with any options as " "query parameters, so the broker, the result backend and the cache can be given a " "database each." ) path = parts.path.rstrip("/").lstrip("/") if path and not (path.isascii() and path.isdigit()): raise DeployError( - f"the Redis URL {base!r} has {path!r} where a database number would go. Pass a URL like " + f"the Redis URL ({_endpoint(base)}) has {path[:20]!r} where a database number would go. " + "Pass a URL like " "redis://host:6379 or redis://host:6379/5." ) try: @@ -247,7 +249,7 @@ def _redis_urls(base: str) -> dict[str, str]: # Python refuses to convert a digit string past its conversion limit, and that is a typo # rather than a crash. raise DeployError( - f"the Redis URL {base!r} has a database number too long to read. Pass a URL like " + f"the Redis URL ({_endpoint(base)}) has a database number too long to read. Pass a URL like " "redis://host:6379 or redis://host:6379/5." ) from exc return { @@ -848,6 +850,10 @@ def _endpoint(url: str) -> str: return "the configured URL" if port: host = f"{host}:{port}" + # The path is capped: this string is for a person to read, and it goes into terminals, CI logs + # and error messages. A 5000-digit database number would otherwise flood all three. + if len(path) > 40: + path = f"{path[:40]}..." return f"{scheme}://{host}{path}" if host else "the configured URL" diff --git a/tests/deploy/test_doctor.py b/tests/deploy/test_doctor.py index eb085038..e3072249 100644 --- a/tests/deploy/test_doctor.py +++ b/tests/deploy/test_doctor.py @@ -472,6 +472,28 @@ class TestRedisSelection: assert chosen == {"broker": "redis://localhost:6379/0"} +class TestRedisUrlErrors: + """The validator raises before any check runs, and cli.main prints what it raises.""" + + SECRET = "sUpErSeCrEt" + + @pytest.mark.parametrize("url", [ + "redis://user:sUpErSeCrEt@host:not-a-port/0", + "redis://user:sUpErSeCrEt@host:0/0", + "redis://user:sUpErSeCrEt@host:6379/queue", + "postgres://user:sUpErSeCrEt@host:6379/0", + "redis://user:sUpErSeCrEt@host:6379/" + "1" * 5000, + ]) + def test_a_rejected_url_never_carries_its_password_into_the_error(self, tmp_path, url): + (tmp_path / ".env").write_text("LLM_PROVIDER=docsgpt\n", encoding="utf-8") + with pytest.raises(DeployError) as raised: + _run(["doctor", "--dir", str(tmp_path), "--redis-url", url], _context()) + message = str(raised.value) + assert self.SECRET not in message + assert "user" not in message + assert len(message) < 400, "an over-long database number must not come back in the message" + + class TestDoctor: def _only(self, monkeypatch, postgres, redis): monkeypatch.setattr(commands, "_check_postgres", lambda uri: postgres)