From 6382901f817eee68aba94db274831f2c5328716a Mon Sep 17 00:00:00 2001 From: Drew Cain Date: Thu, 11 Jun 2026 01:10:21 -0500 Subject: [PATCH 1/7] feat(mcp): add basic_memory_diagnostics tool for version and system info Adds a new read-only MCP tool `basic_memory_diagnostics` that surfaces: - Basic Memory package version and API version (from __init__.py) - Python version and platform/architecture details - Config file path and full config.json contents (cloud_api_key redacted) Resolves #187. Co-Authored-By: Claude Signed-off-by: Drew Cain --- src/basic_memory/mcp/tools/__init__.py | 4 + .../mcp/tools/basic_memory_diagnostics.py | 84 ++++++++++ .../mcp/test_tool_basic_memory_diagnostics.py | 146 ++++++++++++++++++ 3 files changed, 234 insertions(+) create mode 100644 src/basic_memory/mcp/tools/basic_memory_diagnostics.py create mode 100644 tests/mcp/test_tool_basic_memory_diagnostics.py diff --git a/src/basic_memory/mcp/tools/__init__.py b/src/basic_memory/mcp/tools/__init__.py index 66e931253..a46cdb378 100644 --- a/src/basic_memory/mcp/tools/__init__.py +++ b/src/basic_memory/mcp/tools/__init__.py @@ -36,7 +36,11 @@ # Schema tools from basic_memory.mcp.tools.schema import schema_validate, schema_infer, schema_diff +# Diagnostics tool +from basic_memory.mcp.tools.basic_memory_diagnostics import basic_memory_diagnostics + __all__ = [ + "basic_memory_diagnostics", "build_context", "canvas", "cloud_info", diff --git a/src/basic_memory/mcp/tools/basic_memory_diagnostics.py b/src/basic_memory/mcp/tools/basic_memory_diagnostics.py new file mode 100644 index 000000000..5c1a1e0e9 --- /dev/null +++ b/src/basic_memory/mcp/tools/basic_memory_diagnostics.py @@ -0,0 +1,84 @@ +"""Diagnostic tool for Basic Memory version and system information.""" + +import json +import platform +import sys + +import basic_memory +from basic_memory.config import CONFIG_FILE_NAME, ConfigManager +from basic_memory.mcp.server import mcp + +# Fields in BasicMemoryConfig that contain secrets and must never be surfaced. +_SECRET_FIELDS = frozenset({"cloud_api_key"}) + + +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. + """ + return {k: v for k, v in raw.items() if k not in _SECRET_FIELDS} + + +@mcp.tool( + "basic_memory_diagnostics", + annotations={"readOnlyHint": True, "openWorldHint": False}, +) +def basic_memory_diagnostics() -> str: + """Return version, system, and configuration diagnostics for Basic Memory. + + Provides: + - Basic Memory package version and API version + - Python version and platform details + - Config file path and its contents (secrets redacted) + + Useful for troubleshooting installations and gathering information for + support requests. Read-only; never emits secrets or API keys. + """ + # --- Version information --- + bm_version = basic_memory.__version__ + api_version = basic_memory.__api_version__ + + # --- System information --- + python_version = sys.version + platform_info = platform.platform() + machine = platform.machine() + + # --- Configuration --- + manager = ConfigManager() + config_file = manager.config_dir / CONFIG_FILE_NAME + config_exists = config_file.exists() + + if config_exists: + try: + raw_config = json.loads(config_file.read_text(encoding="utf-8")) + safe_config = _redact_config(raw_config) + config_dump = json.dumps(safe_config, indent=2, default=str) + except Exception as exc: # pragma: no cover + config_dump = f"" + else: + config_dump = "" + + lines = [ + "# Basic Memory Diagnostics", + "", + "## Version", + f"- basic-memory: {bm_version}", + f"- API version: {api_version}", + "", + "## System", + f"- Python: {python_version}", + f"- Platform: {platform_info}", + f"- Architecture: {machine}", + "", + "## Configuration", + f"- Config path: {config_file}", + f"- Config exists: {config_exists}", + "", + "```json", + config_dump, + "```", + ] + return "\n".join(lines) diff --git a/tests/mcp/test_tool_basic_memory_diagnostics.py b/tests/mcp/test_tool_basic_memory_diagnostics.py new file mode 100644 index 000000000..8a1445101 --- /dev/null +++ b/tests/mcp/test_tool_basic_memory_diagnostics.py @@ -0,0 +1,146 @@ +"""Tests for the basic_memory_diagnostics MCP tool.""" + +import json +import platform +import sys +from unittest.mock import MagicMock, patch + +import basic_memory +from basic_memory.mcp.tools.basic_memory_diagnostics import ( + _redact_config, + basic_memory_diagnostics, +) + + +# --------------------------------------------------------------------------- +# Unit tests for _redact_config helper +# --------------------------------------------------------------------------- + + +def test_redact_config_removes_cloud_api_key(): + raw = {"cloud_api_key": "bmc_secret", "default_project": "main", "projects": {}} + result = _redact_config(raw) + assert "cloud_api_key" not in result + assert result["default_project"] == "main" + assert "projects" in result + + +def test_redact_config_passes_through_safe_fields(): + raw = {"default_project": "main", "log_level": "INFO", "env": "dev"} + result = _redact_config(raw) + assert result == raw + + +def test_redact_config_empty_dict(): + assert _redact_config({}) == {} + + +# --------------------------------------------------------------------------- +# Tests for the basic_memory_diagnostics tool +# --------------------------------------------------------------------------- + + +def test_diagnostics_returns_string(): + result = basic_memory_diagnostics() + assert isinstance(result, str) + + +def test_diagnostics_includes_version(): + result = basic_memory_diagnostics() + assert basic_memory.__version__ in result + assert basic_memory.__api_version__ in result + + +def test_diagnostics_includes_python_version(): + result = basic_memory_diagnostics() + # sys.version can be multi-line; just check the version tuple prefix + major_minor = f"{sys.version_info.major}.{sys.version_info.minor}" + assert major_minor in result + + +def test_diagnostics_includes_platform(): + result = basic_memory_diagnostics() + assert platform.machine() in result + + +def test_diagnostics_includes_config_path(tmp_path): + """Config path section should appear in output.""" + 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({"default_project": "main", "projects": {}})) + + result = basic_memory_diagnostics() + + assert str(tmp_path) in result + assert "Config path:" in result + + +def test_diagnostics_config_exists_with_valid_json(tmp_path): + """When config file exists, its safe contents should appear as JSON.""" + config_data = { + "default_project": "research", + "projects": {"research": {"path": str(tmp_path / "research")}}, + } + 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 "research" in result + assert "```json" in result + + +def test_diagnostics_redacts_cloud_api_key(tmp_path): + """cloud_api_key must never appear in diagnostic output.""" + config_data = { + "default_project": "main", + "cloud_api_key": "bmc_super_secret_token", + "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 "bmc_super_secret_token" not in result + assert "cloud_api_key" not in result + + +def test_diagnostics_config_missing(tmp_path): + """When config file does not exist, output should say so.""" + 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 + + # Ensure no config.json is present + config_file = tmp_path / "config.json" + assert not config_file.exists() + + result = basic_memory_diagnostics() + + assert "Config exists: False" in result + assert "" in result + + +def test_diagnostics_output_sections(): + """All expected section headers should be present.""" + result = basic_memory_diagnostics() + assert "# Basic Memory Diagnostics" in result + assert "## Version" in result + assert "## System" in result + assert "## Configuration" in result From a1e8a2cffbca521aeb884d10dae048b225f7b4b3 Mon Sep 17 00:00:00 2001 From: Drew Cain Date: Thu, 11 Jun 2026 09:37:00 -0500 Subject: [PATCH 2/7] 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 5c1a1e0e9..400733ad7 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 @@ # 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 8a1445101..54a1a2d58 100644 --- a/tests/mcp/test_tool_basic_memory_diagnostics.py +++ b/tests/mcp/test_tool_basic_memory_diagnostics.py @@ -8,6 +8,7 @@ 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 From 7a957f55b98944fbffef305f33db767fdb4e8697 Mon Sep 17 00:00:00 2001 From: Drew Cain Date: Thu, 11 Jun 2026 09:42:22 -0500 Subject: [PATCH 3/7] test(mcp): use exact-equality assertions for redacted URLs CodeQL flags dotted-host substring checks against URLs (py/incomplete-url-substring-sanitization); exact matches are also stronger assertions. Co-Authored-By: Claude Signed-off-by: Drew Cain --- tests/mcp/test_tool_basic_memory_diagnostics.py | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/tests/mcp/test_tool_basic_memory_diagnostics.py b/tests/mcp/test_tool_basic_memory_diagnostics.py index 54a1a2d58..fe8fbed4b 100644 --- a/tests/mcp/test_tool_basic_memory_diagnostics.py +++ b/tests/mcp/test_tool_basic_memory_diagnostics.py @@ -165,17 +165,12 @@ def test_redact_url_strips_password(): 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 + assert _redact_url(url) == "postgresql://***@db.example.com/app" 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 + assert _redact_url(url) == "postgresql://***@db.internal:5432/prod" def test_redact_url_no_credentials_unchanged(): @@ -201,11 +196,8 @@ def test_redact_config_scrubs_database_url_credentials(): "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"] + # Exact match: credentials replaced, host/port/db preserved for diagnostics. + assert result["database_url"] == "postgresql://***@host.example.com:5432/bm" def test_redact_config_leaves_database_url_without_credentials(): From a801e2f942925af2a4f56086e176ba0537a1c10a Mon Sep 17 00:00:00 2001 From: Drew Cain Date: Thu, 11 Jun 2026 14:56:35 -0500 Subject: [PATCH 4/7] fix(mcp): drop stale API version from diagnostics, suppress duplicated structured output - Remove orphaned __api_version__ ('v0' since 2025; real API routes are /v2, and nothing else reads the constant) - output_schema=None so FastMCP doesn't duplicate the markdown report into structuredContent (halves the payload) - Add title/tags annotations so #963 and #968 are merge-order independent Co-Authored-By: Claude Signed-off-by: Drew Cain --- src/basic_memory/mcp/tools/basic_memory_diagnostics.py | 9 ++++++--- tests/mcp/test_tool_basic_memory_diagnostics.py | 1 - 2 files changed, 6 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 400733ad7..75b203b7d 100644 --- a/src/basic_memory/mcp/tools/basic_memory_diagnostics.py +++ b/src/basic_memory/mcp/tools/basic_memory_diagnostics.py @@ -69,13 +69,18 @@ def _redact_config(raw: dict) -> dict: @mcp.tool( "basic_memory_diagnostics", + title="Basic Memory Diagnostics", + tags={"diagnostics"}, annotations={"readOnlyHint": True, "openWorldHint": False}, + # The report is a markdown string; suppress FastMCP's wrap_result so the + # payload isn't duplicated into structuredContent. + output_schema=None, ) def basic_memory_diagnostics() -> str: """Return version, system, and configuration diagnostics for Basic Memory. Provides: - - Basic Memory package version and API version + - Basic Memory package version - Python version and platform details - Config file path and its contents (secrets redacted) @@ -84,7 +89,6 @@ def basic_memory_diagnostics() -> str: """ # --- Version information --- bm_version = basic_memory.__version__ - api_version = basic_memory.__api_version__ # --- System information --- python_version = sys.version @@ -111,7 +115,6 @@ def basic_memory_diagnostics() -> str: "", "## Version", f"- basic-memory: {bm_version}", - f"- API version: {api_version}", "", "## System", f"- Python: {python_version}", diff --git a/tests/mcp/test_tool_basic_memory_diagnostics.py b/tests/mcp/test_tool_basic_memory_diagnostics.py index fe8fbed4b..985f26297 100644 --- a/tests/mcp/test_tool_basic_memory_diagnostics.py +++ b/tests/mcp/test_tool_basic_memory_diagnostics.py @@ -49,7 +49,6 @@ def test_diagnostics_returns_string(): def test_diagnostics_includes_version(): result = basic_memory_diagnostics() assert basic_memory.__version__ in result - assert basic_memory.__api_version__ in result def test_diagnostics_includes_python_version(): From 7be4f1d8b1b2a10e9280606715564fd85e2829ca Mon Sep 17 00:00:00 2001 From: phernandez Date: Tue, 14 Jul 2026 22:03:24 -0500 Subject: [PATCH 5/7] fix(mcp): harden diagnostics output Signed-off-by: phernandez --- src/basic_memory/__init__.py | 2 +- src/basic_memory/mcp/tools/__init__.py | 4 +- .../mcp/tools/basic_memory_diagnostics.py | 51 ++++--- .../mcp/test_tool_basic_memory_diagnostics.py | 125 ++++++++++++------ tests/mcp/test_tool_contracts.py | 3 + 5 files changed, 120 insertions(+), 65 deletions(-) diff --git a/src/basic_memory/__init__.py b/src/basic_memory/__init__.py index 4b753b1ad..10bd93938 100644 --- a/src/basic_memory/__init__.py +++ b/src/basic_memory/__init__.py @@ -4,4 +4,4 @@ __version__ = "0.22.1" # API version for FastAPI - independent of package version -__api_version__ = "v0" +__api_version__ = "v2" diff --git a/src/basic_memory/mcp/tools/__init__.py b/src/basic_memory/mcp/tools/__init__.py index a46cdb378..12211c3b3 100644 --- a/src/basic_memory/mcp/tools/__init__.py +++ b/src/basic_memory/mcp/tools/__init__.py @@ -6,6 +6,7 @@ """ # Import tools to register them with MCP +from basic_memory.mcp.tools.basic_memory_diagnostics import basic_memory_diagnostics from basic_memory.mcp.tools.delete_note import delete_note from basic_memory.mcp.tools.read_content import read_content from basic_memory.mcp.tools.build_context import build_context @@ -36,9 +37,6 @@ # Schema tools from basic_memory.mcp.tools.schema import schema_validate, schema_infer, schema_diff -# Diagnostics tool -from basic_memory.mcp.tools.basic_memory_diagnostics import basic_memory_diagnostics - __all__ = [ "basic_memory_diagnostics", "build_context", diff --git a/src/basic_memory/mcp/tools/basic_memory_diagnostics.py b/src/basic_memory/mcp/tools/basic_memory_diagnostics.py index 75b203b7d..2f2ce7654 100644 --- a/src/basic_memory/mcp/tools/basic_memory_diagnostics.py +++ b/src/basic_memory/mcp/tools/basic_memory_diagnostics.py @@ -6,7 +6,7 @@ from urllib.parse import urlparse, urlunparse import basic_memory -from basic_memory.config import CONFIG_FILE_NAME, ConfigManager +from basic_memory.config import CONFIG_FILE_NAME, resolve_data_dir from basic_memory.mcp.server import mcp # Fields in BasicMemoryConfig that contain secrets and must never be surfaced. @@ -26,22 +26,23 @@ def _redact_url(url: str) -> str: """ try: parsed = urlparse(url) - except Exception: # pragma: no cover — urlparse is very permissive + except ValueError: + # A malformed authority can still contain credentials. Redact the + # userinfo conservatively rather than returning a secret unchanged. + scheme, separator, remainder = url.partition("://") + if separator and "@" in remainder: + _, _, authority = remainder.rpartition("@") + return f"{scheme}://***@{authority}" return url - if not parsed.hostname: - # Not a meaningful URL (e.g. a bare file path); leave it alone. + if "@" not in parsed.netloc: + # No URL userinfo is present, so there are no credentials to redact. 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 + # Preserve the authority verbatim after the final @. In particular, using + # parsed.hostname here would discard the brackets required around IPv6 hosts. + _, _, authority = parsed.netloc.rpartition("@") + return urlunparse(parsed._replace(netloc=f"***@{authority}")) def _redact_config(raw: dict) -> dict: @@ -71,7 +72,12 @@ def _redact_config(raw: dict) -> dict: "basic_memory_diagnostics", title="Basic Memory Diagnostics", tags={"diagnostics"}, - annotations={"readOnlyHint": True, "openWorldHint": False}, + annotations={ + "title": "Basic Memory Diagnostics", + "readOnlyHint": True, + "destructiveHint": False, + "openWorldHint": False, + }, # The report is a markdown string; suppress FastMCP's wrap_result so the # payload isn't duplicated into structuredContent. output_schema=None, @@ -89,6 +95,7 @@ def basic_memory_diagnostics() -> str: """ # --- Version information --- bm_version = basic_memory.__version__ + api_version = basic_memory.__api_version__ # --- System information --- python_version = sys.version @@ -96,17 +103,22 @@ def basic_memory_diagnostics() -> str: machine = platform.machine() # --- Configuration --- - manager = ConfigManager() - config_file = manager.config_dir / CONFIG_FILE_NAME + # resolve_data_dir only computes the path. ConfigManager would create and + # chmod the directory, violating this tool's read-only contract. + config_file = resolve_data_dir() / CONFIG_FILE_NAME config_exists = config_file.exists() if config_exists: try: raw_config = json.loads(config_file.read_text(encoding="utf-8")) - safe_config = _redact_config(raw_config) - config_dump = json.dumps(safe_config, indent=2, default=str) - except Exception as exc: # pragma: no cover + except (OSError, json.JSONDecodeError) as exc: config_dump = f"" + else: + if isinstance(raw_config, dict): + safe_config = _redact_config(raw_config) + config_dump = json.dumps(safe_config, indent=2, default=str) + else: + config_dump = "" else: config_dump = "" @@ -115,6 +127,7 @@ def basic_memory_diagnostics() -> str: "", "## Version", f"- basic-memory: {bm_version}", + f"- API: {api_version}", "", "## System", f"- Python: {python_version}", diff --git a/tests/mcp/test_tool_basic_memory_diagnostics.py b/tests/mcp/test_tool_basic_memory_diagnostics.py index 985f26297..1af2c77b6 100644 --- a/tests/mcp/test_tool_basic_memory_diagnostics.py +++ b/tests/mcp/test_tool_basic_memory_diagnostics.py @@ -3,9 +3,10 @@ import json import platform import sys -from unittest.mock import MagicMock, patch +from pathlib import Path import basic_memory +import pytest from basic_memory.mcp.tools.basic_memory_diagnostics import ( _redact_config, _redact_url, @@ -13,6 +14,12 @@ ) +@pytest.fixture(autouse=True) +def isolate_config_dir(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: + """Keep every diagnostics test isolated from the developer's real config.""" + monkeypatch.setenv("BASIC_MEMORY_CONFIG_DIR", str(tmp_path)) + + # --------------------------------------------------------------------------- # Unit tests for _redact_config helper # --------------------------------------------------------------------------- @@ -49,6 +56,8 @@ def test_diagnostics_returns_string(): def test_diagnostics_includes_version(): result = basic_memory_diagnostics() assert basic_memory.__version__ in result + assert basic_memory.__api_version__ == "v2" + assert f"API: {basic_memory.__api_version__}" in result def test_diagnostics_includes_python_version(): @@ -65,15 +74,10 @@ def test_diagnostics_includes_platform(): def test_diagnostics_includes_config_path(tmp_path): """Config path section should appear in output.""" - 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({"default_project": "main", "projects": {}})) + config_file = tmp_path / "config.json" + config_file.write_text(json.dumps({"default_project": "main", "projects": {}})) - result = basic_memory_diagnostics() + result = basic_memory_diagnostics() assert str(tmp_path) in result assert "Config path:" in result @@ -85,15 +89,10 @@ def test_diagnostics_config_exists_with_valid_json(tmp_path): "default_project": "research", "projects": {"research": {"path": str(tmp_path / "research")}}, } - 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)) + config_file = tmp_path / "config.json" + config_file.write_text(json.dumps(config_data)) - result = basic_memory_diagnostics() + result = basic_memory_diagnostics() assert "research" in result assert "```json" in result @@ -106,15 +105,10 @@ def test_diagnostics_redacts_cloud_api_key(tmp_path): "cloud_api_key": "bmc_super_secret_token", "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)) - config_file = tmp_path / "config.json" - config_file.write_text(json.dumps(config_data)) - - result = basic_memory_diagnostics() + result = basic_memory_diagnostics() assert "bmc_super_secret_token" not in result assert "cloud_api_key" not in result @@ -122,21 +116,58 @@ def test_diagnostics_redacts_cloud_api_key(tmp_path): def test_diagnostics_config_missing(tmp_path): """When config file does not exist, output should say so.""" - 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 - - # Ensure no config.json is present - config_file = tmp_path / "config.json" - assert not config_file.exists() + config_file = tmp_path / "config.json" + assert not config_file.exists() - result = basic_memory_diagnostics() + result = basic_memory_diagnostics() assert "Config exists: False" in result assert "" in result +def test_diagnostics_does_not_create_config_directory(monkeypatch, tmp_path): + """Resolving diagnostics must remain read-only when no config exists.""" + missing_config_dir = tmp_path / "does-not-exist" + monkeypatch.setenv("BASIC_MEMORY_CONFIG_DIR", str(missing_config_dir)) + + result = basic_memory_diagnostics() + + assert "Config exists: False" in result + assert not missing_config_dir.exists() + + +def test_diagnostics_reports_invalid_json(tmp_path): + config_file = tmp_path / "config.json" + config_file.write_text("{not valid json", encoding="utf-8") + + result = basic_memory_diagnostics() + + assert "" in result + + +def test_diagnostics_reports_config_read_error(monkeypatch, tmp_path): + config_file = tmp_path / "config.json" + config_file.write_text("{}", encoding="utf-8") + + def fail_read_text(self, *args, **kwargs): + raise OSError("permission denied") + + monkeypatch.setattr(Path, "read_text", fail_read_text) + + result = basic_memory_diagnostics() + + assert "" in result + + def test_diagnostics_output_sections(): """All expected section headers should be present.""" result = basic_memory_diagnostics() @@ -172,6 +203,21 @@ def test_redact_url_preserves_port(): assert _redact_url(url) == "postgresql://***@db.internal:5432/prod" +def test_redact_url_preserves_ipv6_brackets(): + url = "postgresql://admin:pw@[::1]:5432/prod" + assert _redact_url(url) == "postgresql://***@[::1]:5432/prod" + + +def test_redact_url_scrubs_credentials_from_malformed_url(): + url = "postgresql://admin:pw@[::1" + assert _redact_url(url) == "postgresql://***@[::1" + + +def test_redact_url_leaves_malformed_url_without_credentials_unchanged(): + url = "postgresql://[::1" + assert _redact_url(url) == url + + def test_redact_url_no_credentials_unchanged(): url = "postgresql://db.internal:5432/prod" assert _redact_url(url) == url @@ -230,15 +276,10 @@ def test_diagnostics_redacts_database_url_password(tmp_path): "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)) - config_file = tmp_path / "config.json" - config_file.write_text(json.dumps(config_data)) - - result = basic_memory_diagnostics() + result = basic_memory_diagnostics() assert "supersecret" not in result assert "pguser" not in result diff --git a/tests/mcp/test_tool_contracts.py b/tests/mcp/test_tool_contracts.py index 8c4d71e72..2eefcf0dc 100644 --- a/tests/mcp/test_tool_contracts.py +++ b/tests/mcp/test_tool_contracts.py @@ -13,6 +13,7 @@ EXPECTED_TOOL_SIGNATURES: dict[str, list[str]] = { + "basic_memory_diagnostics": [], "build_context": [ "url", "project", @@ -124,6 +125,7 @@ # every tool must set annotations.title and explicit readOnlyHint, destructiveHint, and # openWorldHint values. EXPECTED_TOOL_ANNOTATIONS: dict[str, dict[str, bool]] = { + "basic_memory_diagnostics": {"readOnlyHint": True, "destructiveHint": False}, "build_context": {"readOnlyHint": True, "destructiveHint": False}, "cloud_info": {"readOnlyHint": True, "destructiveHint": False}, "fetch": {"readOnlyHint": True, "destructiveHint": False}, @@ -176,6 +178,7 @@ TOOL_FUNCTIONS: dict[str, object] = { + "basic_memory_diagnostics": tools.basic_memory_diagnostics, "build_context": tools.build_context, "canvas": tools.canvas, "cloud_info": tools.cloud_info, From cef13fa564377089401ef69f971f4df71848a22d Mon Sep 17 00:00:00 2001 From: phernandez Date: Tue, 14 Jul 2026 23:54:42 -0500 Subject: [PATCH 6/7] fix(mcp): redact database URL query secrets Signed-off-by: phernandez --- .../mcp/tools/basic_memory_diagnostics.py | 63 +++++++++++++++---- .../mcp/test_tool_basic_memory_diagnostics.py | 43 +++++++++++++ 2 files changed, 93 insertions(+), 13 deletions(-) diff --git a/src/basic_memory/mcp/tools/basic_memory_diagnostics.py b/src/basic_memory/mcp/tools/basic_memory_diagnostics.py index 2f2ce7654..eb1009808 100644 --- a/src/basic_memory/mcp/tools/basic_memory_diagnostics.py +++ b/src/basic_memory/mcp/tools/basic_memory_diagnostics.py @@ -3,7 +3,7 @@ import json import platform import sys -from urllib.parse import urlparse, urlunparse +from urllib.parse import parse_qsl, urlencode, urlparse, urlunparse import basic_memory from basic_memory.config import CONFIG_FILE_NAME, resolve_data_dir @@ -16,9 +16,39 @@ # The userinfo component is stripped before surfacing. _URL_FIELDS = frozenset({"database_url"}) +_SECRET_QUERY_KEYS = frozenset( + { + "api_key", + "apikey", + "credential", + "password", + "passwd", + "pwd", + "secret", + "secret_key", + "sslpassword", + "token", + } +) + + +def _query_key_is_secret(key: str) -> bool: + normalized = key.strip().lower().replace("-", "_") + return normalized in _SECRET_QUERY_KEYS or normalized.endswith( + ("_password", "_secret", "_token", "_key") + ) + + +def _redact_query_secrets(query: str) -> str: + """Mask credential-bearing query values while preserving diagnostic options.""" + pairs = parse_qsl(query, keep_blank_values=True) + if not any(_query_key_is_secret(key) for key, _ in pairs): + return query + return urlencode([(key, "***" if _query_key_is_secret(key) else value) for key, value in pairs]) + def _redact_url(url: str) -> str: - """Strip the userinfo (user:password) from a URL string. + """Strip userinfo and credential-bearing query values from a URL string. Replaces any credentials with *** so the host/path remain visible for diagnostics (e.g. ``postgresql://***@localhost/mydb``). If the value @@ -28,29 +58,36 @@ def _redact_url(url: str) -> str: parsed = urlparse(url) except ValueError: # A malformed authority can still contain credentials. Redact the - # userinfo conservatively rather than returning a secret unchanged. - scheme, separator, remainder = url.partition("://") + # userinfo and query conservatively rather than returning a secret unchanged. + base, query_separator, query = url.partition("?") + safe_url = f"{base}?{_redact_query_secrets(query)}" if query_separator else base + scheme, separator, remainder = safe_url.partition("://") if separator and "@" in remainder: _, _, authority = remainder.rpartition("@") return f"{scheme}://***@{authority}" - return url + return safe_url - if "@" not in parsed.netloc: - # No URL userinfo is present, so there are no credentials to redact. + redacted_query = _redact_query_secrets(parsed.query) + if "@" not in parsed.netloc and redacted_query == parsed.query: + # Neither URL userinfo nor known secret query parameters are present. return url - # Preserve the authority verbatim after the final @. In particular, using - # parsed.hostname here would discard the brackets required around IPv6 hosts. - _, _, authority = parsed.netloc.rpartition("@") - return urlunparse(parsed._replace(netloc=f"***@{authority}")) + redacted_netloc = parsed.netloc + if "@" in parsed.netloc: + # Preserve the authority verbatim after the final @. In particular, using + # parsed.hostname here would discard the brackets required around IPv6 hosts. + _, _, authority = parsed.netloc.rpartition("@") + redacted_netloc = f"***@{authority}" + + return urlunparse(parsed._replace(netloc=redacted_netloc, query=redacted_query)) def _redact_config(raw: dict) -> dict: """Return a copy of the raw config dict with secret fields removed. - 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. + - Keys in ``_URL_FIELDS`` have userinfo and credential-bearing query values + stripped so that safe host, database, and connection options remain visible. Only top-level keys are processed. Nested keys within project entries are not currently credential-bearing, but the two sets make the pattern easy diff --git a/tests/mcp/test_tool_basic_memory_diagnostics.py b/tests/mcp/test_tool_basic_memory_diagnostics.py index 1af2c77b6..62cc39da1 100644 --- a/tests/mcp/test_tool_basic_memory_diagnostics.py +++ b/tests/mcp/test_tool_basic_memory_diagnostics.py @@ -213,6 +213,11 @@ def test_redact_url_scrubs_credentials_from_malformed_url(): assert _redact_url(url) == "postgresql://***@[::1" +def test_redact_url_scrubs_query_credentials_from_malformed_url(): + url = "postgresql://[::1?sslpassword=query-secret" + assert _redact_url(url) == "postgresql://[::1?sslpassword=%2A%2A%2A" + + def test_redact_url_leaves_malformed_url_without_credentials_unchanged(): url = "postgresql://[::1" assert _redact_url(url) == url @@ -223,6 +228,24 @@ def test_redact_url_no_credentials_unchanged(): assert _redact_url(url) == url +def test_redact_url_masks_query_password_and_preserves_safe_options(): + url = "postgresql://db.internal/prod?sslmode=require&sslpassword=query-secret" + result = _redact_url(url) + + assert "query-secret" not in result + assert result == "postgresql://db.internal/prod?sslmode=require&sslpassword=%2A%2A%2A" + + +def test_redact_url_masks_userinfo_and_query_secrets_together(): + url = "postgresql://dbuser:user-secret@db.internal/prod?password=query-secret" + result = _redact_url(url) + + assert "dbuser" not in result + assert "user-secret" not in result + assert "query-secret" not in result + assert result == "postgresql://***@db.internal/prod?password=%2A%2A%2A" + + 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" @@ -286,3 +309,23 @@ def test_diagnostics_redacts_database_url_password(tmp_path): # Host and port remain visible for diagnostics. assert "db.internal" in result assert "5432" in result + + +def test_diagnostics_redacts_database_url_query_password(tmp_path): + """Query-string credentials must not escape through diagnostic output.""" + config_data = { + "default_project": "main", + "database_url": ( + "postgresql://db.internal:5432/basicmemory" + "?sslmode=require&sslpassword=query-supersecret" + ), + "projects": {}, + } + config_file = tmp_path / "config.json" + config_file.write_text(json.dumps(config_data)) + + result = basic_memory_diagnostics() + + assert "query-supersecret" not in result + assert "sslmode=require" in result + assert "sslpassword=%2A%2A%2A" in result From ef6929852c4f475ec4419740d0063254f243bcf2 Mon Sep 17 00:00:00 2001 From: phernandez Date: Wed, 15 Jul 2026 00:13:41 -0500 Subject: [PATCH 7/7] fix(mcp): handle invalid diagnostics config encoding Signed-off-by: phernandez --- src/basic_memory/mcp/tools/basic_memory_diagnostics.py | 2 +- tests/mcp/test_tool_basic_memory_diagnostics.py | 10 ++++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/src/basic_memory/mcp/tools/basic_memory_diagnostics.py b/src/basic_memory/mcp/tools/basic_memory_diagnostics.py index eb1009808..b6b658d04 100644 --- a/src/basic_memory/mcp/tools/basic_memory_diagnostics.py +++ b/src/basic_memory/mcp/tools/basic_memory_diagnostics.py @@ -148,7 +148,7 @@ def basic_memory_diagnostics() -> str: if config_exists: try: raw_config = json.loads(config_file.read_text(encoding="utf-8")) - except (OSError, json.JSONDecodeError) as exc: + except (OSError, UnicodeDecodeError, json.JSONDecodeError) as exc: config_dump = f"" else: if isinstance(raw_config, dict): diff --git a/tests/mcp/test_tool_basic_memory_diagnostics.py b/tests/mcp/test_tool_basic_memory_diagnostics.py index 62cc39da1..a5b7c5f94 100644 --- a/tests/mcp/test_tool_basic_memory_diagnostics.py +++ b/tests/mcp/test_tool_basic_memory_diagnostics.py @@ -145,6 +145,16 @@ def test_diagnostics_reports_invalid_json(tmp_path): assert "