This is an automated email from the ASF dual-hosted git repository. cgivre pushed a commit to branch main in repository https://gitbox.apache.org/repos/asf/drill-mcp.git
commit a5329bb8206a9651f341334a8bbf4f4fc876489b Author: cgivre <[email protected]> AuthorDate: Wed Aug 12 16:27:33 2026 -0400 fix: redact and hidden-schema-filter list_profiles/get_profile output list_profiles and get_profile were the only Drill-sourced tool outputs bypassing both redact() and hidden-schema filtering. Profiles are cluster-wide: they carry other users' query text (a hidden schema name can leak out there as data, the enumeration path the guard and the metadata-tool filtering were built to close) and a full profile embeds Drill's serialized physical plan, which for JDBC/HTTP plugins can carry plugin configuration. Pass both through redact(); drop list_profiles entries, and refuse get_profile, when the profile's query text case-insensitively names a hidden_schemas entry. Updates docs/tools.md, which said get_profile's payload was returned unmodified. --- docs/tools.md | 18 ++++++++++++++---- drill_mcp/server.py | 33 +++++++++++++++++++++++++++++++-- tests/test_server.py | 37 +++++++++++++++++++++++++++++++++++++ 3 files changed, 82 insertions(+), 6 deletions(-) diff --git a/docs/tools.md b/docs/tools.md index 7897218..1c8ff5e 100644 --- a/docs/tools.md +++ b/docs/tools.md @@ -397,7 +397,12 @@ client surfaces defaults, since a caller passing no `limit` at all still gets at most 20 profiles back, not an unbounded list. **Returns** a list of profile summary dicts, shape defined by Drill's -`/profiles.json`: +`/profiles.json`, passed through the same secret redaction as +`list_storage_plugins` (profiles are cluster-wide and can carry other +users' connection strings). Any entry whose query text names a +`hidden_schemas` entry is dropped entirely, the same protection +`list_schemas`/`list_tables` apply — otherwise a hidden schema's name would +leak out as data in another user's query text: ```json [ @@ -431,9 +436,12 @@ Fetches the full profile for one query id. |---|---|---|---| | `query_id` | string | no | Drill's query UUID, as returned in `run_query`'s `query_id` field (REST backend only — always `null` on JDBC). Validated against `[A-Za-z0-9-]+`. | -**Returns** Drill's full profile JSON for that query, unmodified — the -complete `/profiles/<query_id>.json` payload (fragments, operator metrics, -timing, etc.), which can be large. +**Returns** Drill's full profile JSON for that query — the complete +`/profiles/<query_id>.json` payload (fragments, operator metrics, timing, +etc.), which can be large — passed through the same secret redaction as +`list_storage_plugins`. A full profile embeds Drill's serialized physical +plan, which for JDBC and HTTP storage plugins can carry plugin +configuration, so this is not returned unmodified. **Errors** @@ -442,6 +450,8 @@ timing, etc.), which can be large. - `query_id must be a string` — wrong type. - `invalid query id: '<x>'` — `query_id` contains characters outside `[A-Za-z0-9-]`. Rejected before any request is made. +- `profile '<id>' references a hidden schema` — the profile's query text + names a `hidden_schemas` entry. - Drill's own error text otherwise (e.g. no profile with that id). --- diff --git a/drill_mcp/server.py b/drill_mcp/server.py index 2d9cae7..349efe8 100644 --- a/drill_mcp/server.py +++ b/drill_mcp/server.py @@ -39,6 +39,7 @@ from mcp.server.mcpserver import MCPServer from .client_rest import DrillError, RestClient from .config import Config, ConfigError, load_config from .guard import Policy, PolicyError, check, is_show_command, matches_prefix +from .redact import redact if TYPE_CHECKING: # Imported only for the type checker: build_client's lazy, in-function @@ -77,6 +78,25 @@ class DrillTools: if matches_prefix(schema, self._policy.hidden_schemas): raise ToolError(f"schema '{schema}' is hidden by configuration") + def _profile_mentions_hidden_schema(self, profile: dict[str, Any]) -> bool: + """True if a profile's query text names a hidden schema. + + Profiles are cluster-wide: `list_profiles`/`get_profile` surface + *other users'* query text, so a hidden schema name can leak out here + as data even though it was never queryable directly -- the one + enumeration path the guard and the metadata-tool filtering were + built to close. A case-insensitive substring match against the + query text is deliberately coarse (over-filtering a profile whose + SQL merely mentions a hidden schema's name in a string literal is an + acceptable false positive; missing one is a leak). + """ + if not self._policy.hidden_schemas: + return False + query_text = str(profile.get("query") or "").lower() + if not query_text: + return False + return any(schema.lower() in query_text for schema in self._policy.hidden_schemas) + def _visible(self, schema: str | None) -> bool: # Fail closed, not open: an item this function cannot identify (no # name at all) is filtered out rather than shown by default. Drill @@ -200,18 +220,27 @@ class DrillTools: if not isinstance(limit, int) or isinstance(limit, bool): raise ToolError("limit must be an integer") try: - return self._require_management("profiles")(limit=limit) + profiles = self._require_management("profiles")(limit=limit) except DrillError as exc: raise ToolError(str(exc)) from exc + profiles = redact(profiles) + return [ + p + for p in profiles + if isinstance(p, dict) and not self._profile_mentions_hidden_schema(p) + ] def get_profile(self, query_id: str) -> dict[str, Any]: """Fetch the full profile for one query id.""" if not isinstance(query_id, str): raise ToolError("query_id must be a string") try: - return self._require_management("profile")(query_id) + profile = self._require_management("profile")(query_id) except DrillError as exc: raise ToolError(str(exc)) from exc + if isinstance(profile, dict) and self._profile_mentions_hidden_schema(profile): + raise ToolError(f"profile {query_id!r} references a hidden schema") + return redact(profile) def cancel_query(self, query_id: str) -> str: """Cancel a running query by its query id.""" diff --git a/tests/test_server.py b/tests/test_server.py index 3678132..9d9dcbf 100644 --- a/tests/test_server.py +++ b/tests/test_server.py @@ -223,6 +223,43 @@ class TestManagementTools: client.profile.return_value = {"queryId": "abc"} assert make_tools(client).get_profile("abc")["queryId"] == "abc" + def test_get_profile_redacts_secret_looking_keys(self): + # Profiles are cluster-wide: a full profile embeds Drill's + # serialized physical plan, which for JDBC/HTTP plugins can carry + # plugin configuration (passwords, tokens). This must go through the + # same redaction as list_storage_plugins, not be returned unmodified. + client = MagicMock() + client.profile.return_value = {"queryId": "abc", "password": "hunter2"} + result = make_tools(client).get_profile("abc") + assert result["password"] == "***REDACTED***" + assert result["queryId"] == "abc" + + def test_get_profile_is_refused_when_its_query_text_names_a_hidden_schema(self): + # A profile carries the query TEXT of whatever user ran it -- other + # users' queries, not just the caller's own. A hidden schema's name + # can leak out here as data even though it is unreachable directly, + # which is exactly the enumeration path the guard and hidden-schema + # filtering elsewhere were built to close. + client = MagicMock() + client.profile.return_value = {"queryId": "abc", "query": "SELECT * FROM sys.options"} + with pytest.raises(ToolError, match="hidden"): + make_tools(client, hidden_schemas=["sys"]).get_profile("abc") + + def test_list_profiles_redacts_secret_looking_keys(self): + client = MagicMock() + client.profiles.return_value = [{"queryId": "abc", "password": "hunter2"}] + result = make_tools(client).list_profiles() + assert result[0]["password"] == "***REDACTED***" + + def test_list_profiles_drops_entries_whose_query_text_names_a_hidden_schema(self): + client = MagicMock() + client.profiles.return_value = [ + {"queryId": "abc", "query": "SELECT * FROM sys.options"}, + {"queryId": "def", "query": "SELECT * FROM dfs.tmp.x"}, + ] + result = make_tools(client, hidden_schemas=["sys"]).list_profiles() + assert [p["queryId"] for p in result] == ["def"] + def test_cancel_query(self): client = MagicMock() client.cancel_query.return_value = "Cancelled"
