From cbc2d5feb1488d24b1e3075d34284f4339cd2d89 Mon Sep 17 00:00:00 2001 From: Drew Cain Date: Thu, 11 Jun 2026 09:37:00 -0500 Subject: [PATCH] fix(mcp): redact database_url credentials in diagnostics output Extends the existing _SECRET_FIELDS redaction to also strip the user:password userinfo component from any URL-bearing config fields (database_url) before surfacing them in diagnostics output. Adds _redact_url() helper and a _URL_FIELDS set so future credential-bearing URL fields are easy to add. Postgres passwords no longer leak to MCP clients or support transcripts; host/port/db-name remain visible. Fixes the P1 security finding raised in review of PR #963. Co-Authored-By: Claude Signed-off-by: Drew Cain --- .../mcp/tools/basic_memory_diagnostics.py | 53 ++++++++- .../mcp/test_tool_basic_memory_diagnostics.py | 110 ++++++++++++++++++ 2 files changed, 159 insertions(+), 4 deletions(-) diff --git a/src/basic_memory/mcp/tools/basic_memory_diagnostics.py b/src/basic_memory/mcp/tools/basic_memory_diagnostics.py index 5c1a1e0e..400733ad 100644 --- a/src/basic_memory/mcp/tools/basic_memory_diagnostics.py +++ b/src/basic_memory/mcp/tools/basic_memory_diagnostics.py @@ -3,6 +3,7 @@ import json import platform import sys +from urllib.parse import urlparse, urlunparse import basic_memory from basic_memory.config import CONFIG_FILE_NAME, ConfigManager @@ -11,15 +12,59 @@ from basic_memory.mcp.server import mcp # Fields in BasicMemoryConfig that contain secrets and must never be surfaced. _SECRET_FIELDS = frozenset({"cloud_api_key"}) +# Fields whose values are URLs that may embed user:password credentials. +# The userinfo component is stripped before surfacing. +_URL_FIELDS = frozenset({"database_url"}) + + +def _redact_url(url: str) -> str: + """Strip the userinfo (user:password) from a URL string. + + Replaces any credentials with *** so the host/path remain visible for + diagnostics (e.g. ``postgresql://***@localhost/mydb``). If the value + cannot be parsed as a URL it is returned unchanged. + """ + try: + parsed = urlparse(url) + except Exception: # pragma: no cover — urlparse is very permissive + return url + + if not parsed.hostname: + # Not a meaningful URL (e.g. a bare file path); leave it alone. + return url + + if parsed.username or parsed.password: + # Rebuild with credentials replaced by a placeholder. + netloc = f"***@{parsed.hostname}" + if parsed.port: + netloc = f"{netloc}:{parsed.port}" + redacted = parsed._replace(netloc=netloc) + return urlunparse(redacted) + + return url + def _redact_config(raw: dict) -> dict: """Return a copy of the raw config dict with secret fields removed. - Only top-level keys are redacted. Nested secret-looking keys within - project entries are not currently present, but the pattern is explicit - so it is easy to extend. + - Keys in ``_SECRET_FIELDS`` are dropped entirely. + - Keys in ``_URL_FIELDS`` have their userinfo component stripped so that + host and database name remain visible for diagnostics. + + Only top-level keys are processed. Nested keys within project entries are + not currently credential-bearing, but the two sets make the pattern easy + to extend. """ - return {k: v for k, v in raw.items() if k not in _SECRET_FIELDS} + result: dict = {} + for k, v in raw.items(): + if k in _SECRET_FIELDS: + # Drop entirely — value has no diagnostic value. + continue + if k in _URL_FIELDS and isinstance(v, str): + result[k] = _redact_url(v) + else: + result[k] = v + return result @mcp.tool( diff --git a/tests/mcp/test_tool_basic_memory_diagnostics.py b/tests/mcp/test_tool_basic_memory_diagnostics.py index 8a144510..54a1a2d5 100644 --- a/tests/mcp/test_tool_basic_memory_diagnostics.py +++ b/tests/mcp/test_tool_basic_memory_diagnostics.py @@ -8,6 +8,7 @@ from unittest.mock import MagicMock, patch import basic_memory from basic_memory.mcp.tools.basic_memory_diagnostics import ( _redact_config, + _redact_url, basic_memory_diagnostics, ) @@ -144,3 +145,112 @@ def test_diagnostics_output_sections(): assert "## Version" in result assert "## System" in result assert "## Configuration" in result + + +# --------------------------------------------------------------------------- +# Unit tests for _redact_url helper +# --------------------------------------------------------------------------- + + +def test_redact_url_strips_password(): + url = "postgresql://user:secret@localhost/mydb" + result = _redact_url(url) + assert "secret" not in result + assert "user" not in result + assert "localhost" in result + assert "mydb" in result + assert "***" in result + + +def test_redact_url_strips_only_password_when_no_username(): + # password-only userinfo (unusual but valid per RFC) + url = "postgresql://:secret@db.example.com/app" + result = _redact_url(url) + assert "secret" not in result + assert "db.example.com" in result + + +def test_redact_url_preserves_port(): + url = "postgresql://admin:pw@db.internal:5432/prod" + result = _redact_url(url) + assert "pw" not in result + assert "5432" in result + assert "db.internal" in result + + +def test_redact_url_no_credentials_unchanged(): + url = "postgresql://db.internal:5432/prod" + assert _redact_url(url) == url + + +def test_redact_url_non_url_string_unchanged(): + # Bare file paths / non-URL values must not be mangled. + path = "/home/user/.local/share/basic-memory/main.db" + assert _redact_url(path) == path + + +# --------------------------------------------------------------------------- +# _redact_config tests for database_url +# --------------------------------------------------------------------------- + + +def test_redact_config_scrubs_database_url_credentials(): + raw = { + "default_project": "main", + "database_url": "postgresql://dbuser:dbpass@host.example.com:5432/bm", + "projects": {}, + } + result = _redact_config(raw) + assert "dbpass" not in result["database_url"] + assert "dbuser" not in result["database_url"] + # Host and db should still be present for diagnostic value. + assert "host.example.com" in result["database_url"] + assert "bm" in result["database_url"] + + +def test_redact_config_leaves_database_url_without_credentials(): + raw = {"database_url": "sqlite:////tmp/basic-memory/main.db"} + result = _redact_config(raw) + assert result["database_url"] == "sqlite:////tmp/basic-memory/main.db" + + +def test_redact_config_drops_secret_fields_independently(): + raw = { + "cloud_api_key": "bmc_top_secret", + "database_url": "postgresql://dbuser:dbpassword@host/db", + "default_project": "main", + } + result = _redact_config(raw) + assert "cloud_api_key" not in result + assert "dbpassword" not in result["database_url"] + assert "dbuser" not in result["database_url"] + assert "main" == result["default_project"] + + +# --------------------------------------------------------------------------- +# Integration: database_url redaction surfaces in diagnostic output +# --------------------------------------------------------------------------- + + +def test_diagnostics_redacts_database_url_password(tmp_path): + """Postgres password in database_url must not appear in diagnostic output.""" + config_data = { + "default_project": "main", + "database_url": "postgresql://pguser:supersecret@db.internal:5432/basicmemory", + "projects": {}, + } + with patch("basic_memory.mcp.tools.basic_memory_diagnostics.ConfigManager") as MockMgr: + mock_mgr = MagicMock() + mock_mgr.config_dir = tmp_path + MockMgr.return_value = mock_mgr + + config_file = tmp_path / "config.json" + config_file.write_text(json.dumps(config_data)) + + result = basic_memory_diagnostics() + + assert "supersecret" not in result + assert "pguser" not in result + # Host and port remain visible for diagnostics. + assert "db.internal" in result + assert "5432" in result