diff --git a/docsgpt/deploy/commands.py b/docsgpt/deploy/commands.py index 0811134f..2da4929a 100644 --- a/docsgpt/deploy/commands.py +++ b/docsgpt/deploy/commands.py @@ -796,7 +796,8 @@ def _check_postgres(uri: Optional[str]) -> Check: applied = cursor.fetchone()[0] is not None current = None if applied: - cursor.execute("select version_num from alembic_version") + # public, like the to_regclass check above: search_path could resolve another one. + cursor.execute("select version_num from public.alembic_version") row = cursor.fetchone() current = row[0] if row else None except (psycopg.Error, OSError, ValueError) as exc: @@ -810,6 +811,18 @@ def _check_postgres(uri: Optional[str]) -> Check: return Check("postgres", "ok", f"PostgreSQL {version}, schema at {current}") +def _redis_endpoint(url: str) -> str: + """A Redis URL without its credentials: this ends up on a terminal, in CI logs and in issues.""" + try: + parts = urlsplit(url) + except ValueError: + return "the configured URL" + host = parts.hostname or "" + if parts.port: + host = f"{host}:{parts.port}" + return f"{parts.scheme}://{host}{parts.path}" if host else "the configured URL" + + def _check_redis(urls: Mapping[str, str]) -> Check: """Ping every Redis the settings name; they are usually one server, three databases.""" if not urls: @@ -822,7 +835,7 @@ def _check_redis(urls: Mapping[str, str]) -> Check: try: redis.Redis.from_url(url, socket_connect_timeout=3).ping() except Exception as exc: # noqa: BLE001 - every client error here is the same finding - return Check("redis", "fail", f"{label} ({url}) does not answer: {str(exc).strip()}") + return Check("redis", "fail", f"{label} ({_redis_endpoint(url)}) does not answer: {str(exc).strip()}") return Check("redis", "ok", f"answering on {len(urls)} database(s)") diff --git a/tests/deploy/test_doctor.py b/tests/deploy/test_doctor.py index 3cc94dfd..9c73b77e 100644 --- a/tests/deploy/test_doctor.py +++ b/tests/deploy/test_doctor.py @@ -134,9 +134,10 @@ class FakeCursor: def __init__(self, version, table, revision): self.answers = [(version,), (table,), (revision,) if revision else None] self.given = 0 + self.statements = [] def execute(self, statement): - self.statement = statement + self.statements.append(statement) def fetchone(self): answer = self.answers[self.given] @@ -167,8 +168,10 @@ class FakeConnection: def _postgres_answering(monkeypatch, version="16.2", table="alembic_version", revision="0031_x", head="0031_x"): import psycopg - monkeypatch.setattr(psycopg, "connect", lambda *a, **k: FakeConnection(FakeCursor(version, table, revision))) + cursor = FakeCursor(version, table, revision) + monkeypatch.setattr(psycopg, "connect", lambda *a, **k: FakeConnection(cursor)) monkeypatch.setattr(commands, "_migration_head", lambda: head) + return cursor class TestPostgresCheck: @@ -178,6 +181,13 @@ class TestPostgresCheck: assert check.level == "ok" assert "16.2" in check.detail and "0031_x" in check.detail + def test_the_revision_comes_from_the_schema_that_was_checked(self, monkeypatch): + """to_regclass looks in public, so the second query must not resolve through search_path.""" + cursor = _postgres_answering(monkeypatch) + commands._check_postgres("postgresql://localhost/d") + revision_query = [statement for statement in cursor.statements if "version_num" in statement] + assert revision_query and all("public.alembic_version" in statement for statement in revision_query) + def test_a_database_with_no_schema_yet(self, monkeypatch): """The commonest first-run state: the database exists, nothing has been migrated into it.""" _postgres_answering(monkeypatch, table=None, revision=None) @@ -234,6 +244,27 @@ class TestRedisCheck: assert check.level == "fail" assert "cache" in check.detail and "6379/2" in check.detail + def test_a_password_in_the_url_is_never_printed(self, monkeypatch): + """doctor output goes into terminals, CI logs and pasted issue reports.""" + import redis + + def from_url(cls, url, **kwargs): + class Client: + def ping(self): + raise ConnectionError("connection refused") + + return Client() + + monkeypatch.setattr(redis.Redis, "from_url", classmethod(from_url)) + check = commands._check_redis({"broker": "rediss://default:sUpErSeCrEt@redis.example.com:6380/0"}) + assert check.level == "fail" + assert "sUpErSeCrEt" not in check.detail + assert "default" not in check.detail + assert "redis.example.com:6380/0" in check.detail, "the endpoint still has to be identifiable" + + def test_a_malformed_url_is_not_echoed_either(self): + assert commands._redis_endpoint("redis://[::1") == "the configured URL" + def test_without_any_redis_configured(self): assert commands._check_redis({}).level == "fail"