mirror of
https://github.com/tiennm99/DocsGPT.git
synced 2026-10-13 06:14:51 +00:00
fix: keep credentials out of doctor's output, and read the revision from the schema it checked
The Redis check put the whole URL in its failure message. A managed Redis URL carries user:password@host, so a failed ping printed the password to the terminal and into any log or issue the output was pasted into. It names scheme://host:port/db now, and falls back to naming no URL when the value cannot be parsed at all. The Postgres check asked to_regclass about public.alembic_version and then read version_num through search_path, so another schema could answer with a different revision, or the query could fail, and doctor would send you to run migrations against a database that is already fine.
This commit is contained in:
1 parent
bcf2707efa
commit
9326c6edb1
2 files changed
+48
-4
No files matched your search
@@ -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)")
|
||||
|
||||
|
||||
|
||||
@@ -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"
|
||||
|
||||
|
||||
Reference in new issue
Block a user