This is an automated email from the ASF dual-hosted git repository. cgivre pushed a commit to branch feat/drill-mcp-server in repository https://gitbox.apache.org/repos/asf/drill-mcp.git
commit 3249bfce57dd32b4290edda62896b584a7b22fad Author: cgivre <[email protected]> AuthorDate: Wed Aug 12 01:09:58 2026 -0400 fix: scrub credentials from JDBC error paths, close cursors, drop dead code Two criticals from review: - _jdbc_url() built the connection string from urlparse().netloc, which includes userinfo (user:pass@host); a URL with embedded credentials put the password directly into the JDBC connection string and from there into any driver error that echoed it. Build from hostname/port only. - Driver exceptions can also echo the password we hand to jaydebeapi.connect() directly (e.g. an auth-failure message). Add _scrub() and apply it to every DrillError built from driver-supplied text: connect, query, and close. The prior test asserted the unscrubbed message verbatim, so it passed only because the leak was present; inverted it to assert the password is absent, and added matching cases for the query and close paths. Also, from the same review: - query() now closes its cursor via contextlib.closing instead of leaking a statement handle per call - close() clears the stale connection handle in a finally and wraps a failing connection.close() in DrillError instead of letting it escape raw - removed the unreachable `if jaydebeapi is None` branch and the dead `except DrillError: raise` in query() - JDBC's truncated flag now matches REST's max_rows>0 guard - test_schemas_uses_information_schema now asserts the emitted SQL, not just the mapped result - noted that Query is invoked positionally --- drill_mcp/client_jdbc.py | 68 +++++++++++++++++++++++++++++++---------------- drill_mcp/client_rest.py | 3 +++ tests/test_client_jdbc.py | 50 +++++++++++++++++++++++++++++----- 3 files changed, 92 insertions(+), 29 deletions(-) diff --git a/drill_mcp/client_jdbc.py b/drill_mcp/client_jdbc.py index eef2d15..2b103c6 100644 --- a/drill_mcp/client_jdbc.py +++ b/drill_mcp/client_jdbc.py @@ -28,6 +28,7 @@ so `JdbcClient` deliberately does not implement `storage_plugins`, from __future__ import annotations +from contextlib import closing from typing import Any from urllib.parse import urlparse @@ -52,17 +53,38 @@ class JdbcClient: # -- connection -------------------------------------------------------- def _jdbc_url(self) -> str: - # Credentials are never embedded in the URL -- they are passed to - # jaydebeapi.connect() as a separate argument (see _connect below) -- - # so this string, and anything derived from it in an error message, - # cannot leak the password. - host = urlparse(self._config.url) - netloc = host.netloc or host.path + # Built from hostname/port ONLY -- never `netloc`, which for a URL + # like "http://alice:s3cret@drill:8047"; includes the userinfo + # ("alice:s3cret@drill:8047"). config.url is free-form and + # unvalidated, so a password embedded there must never reach the + # connection string, or it becomes reachable through a driver + # exception that echoes its arguments back (see _scrub, which + # handles the remaining case: the driver echoing the password we + # pass to jaydebeapi.connect() separately, below). + parsed = urlparse(self._config.url) + host = parsed.hostname or parsed.path + netloc = f"{host}:{parsed.port}" if parsed.port else host url = f"jdbc:drill:drillbit={netloc}" if self._config.auth == "kerberos": url += ";auth=kerberos" return url + def _scrub(self, text: str) -> str: + """Remove the configured password from driver-supplied error text. + + `redact()` in `client_rest.py` doesn't apply here -- it's key-based + over dicts/lists/tuples and passes a bare `str` through unchanged. + The driver's exception text is exactly that: a bare string we do not + control, and it can contain the password we handed to + `jaydebeapi.connect()` (e.g. an auth-failure message that echoes its + arguments) or, via the JDBC URL, one a caller embedded in `config.url` + as userinfo despite `_jdbc_url` no longer forwarding it verbatim. + """ + password = self._config.password + if password: + text = text.replace(password, "***REDACTED***") + return text + def _connect(self) -> Any: if self._connection is not None: return self._connection @@ -72,10 +94,6 @@ class JdbcClient: raise DrillError( "the JDBC backend requires the jdbc extra: pip install drill-mcp[jdbc]" ) from exc - if jaydebeapi is None: - raise DrillError( - "the JDBC backend requires the jdbc extra: pip install drill-mcp[jdbc]" - ) credentials = ( [self._config.user, self._config.password] if self._config.auth == "basic" @@ -89,15 +107,21 @@ class JdbcClient: jars=[self._config.jdbc_driver_path], ) except Exception as exc: - # `exc` is whatever the driver reports; it is never supplemented - # here with the URL or credentials, so this message can only leak - # a credential if the driver itself already put one in `exc`. - raise DrillError(f"could not connect to Drill over JDBC: {exc}") from exc + raise DrillError( + f"could not connect to Drill over JDBC: {self._scrub(str(exc))}" + ) from exc return self._connection def close(self) -> None: - if self._connection is not None: + if self._connection is None: + return + try: self._connection.close() + except Exception as exc: + raise DrillError( + f"could not close the JDBC connection: {self._scrub(str(exc))}" + ) from exc + finally: self._connection = None # -- queries ------------------------------------------------------------- @@ -105,18 +129,16 @@ class JdbcClient: def query(self, sql: str, max_rows: int) -> QueryResult: connection = self._connect() try: - cursor = connection.cursor() - cursor.execute(sql) - rows = cursor.fetchmany(max_rows) - columns = [description[0] for description in cursor.description or []] - except DrillError: - raise + with closing(connection.cursor()) as cursor: + cursor.execute(sql) + rows = cursor.fetchmany(max_rows) + columns = [description[0] for description in cursor.description or []] except Exception as exc: - raise DrillError(str(exc)) from exc + raise DrillError(self._scrub(str(exc))) from exc return QueryResult( columns=columns, rows=[dict(zip(columns, row)) for row in rows], - truncated=len(rows) >= max_rows, + truncated=max_rows > 0 and len(rows) >= max_rows, ) # -- metadata -------------------------------------------------------------- diff --git a/drill_mcp/client_rest.py b/drill_mcp/client_rest.py index bed9076..b890f13 100644 --- a/drill_mcp/client_rest.py +++ b/drill_mcp/client_rest.py @@ -210,6 +210,9 @@ def _error_text(response: httpx.Response) -> str: # so `JdbcClient` can share the exact same identifier-quoting and file-plugin # branching instead of duplicating ~80 lines of security-relevant logic. +# Called positionally as `query(sql, max_rows)`, not with keyword arguments -- +# RestClient.query and JdbcClient.query both accept `max_rows` positionally, +# but a bound method's parameter name is not part of `Callable`'s contract. Query = Callable[[str, int], QueryResult] diff --git a/tests/test_client_jdbc.py b/tests/test_client_jdbc.py index 3fd8011..8e64500 100644 --- a/tests/test_client_jdbc.py +++ b/tests/test_client_jdbc.py @@ -60,6 +60,19 @@ def test_connect_uses_the_configured_driver_and_url(fake_jaydebeapi): assert args.kwargs["jars"] == ["/opt/drill-jdbc-all.jar"] +def test_jdbc_url_drops_userinfo_from_a_url_that_embeds_credentials(fake_jaydebeapi): + # config.url is free-form and unvalidated; nothing stops + # DRILL_URL=http://alice:s3cret@drill:8047. The JDBC connection string + # must be built from hostname/port only, never from `netloc` (which + # includes "alice:s3cret@"), or the password ends up in the connection + # string and from there in any driver error that echoes it. + make_client(url="http://alice:s3cret@drill:8047";).query("SELECT 1", max_rows=1) + url = fake_jaydebeapi.connect.call_args.args[1] + assert url == "jdbc:drill:drillbit=drill:8047" + assert "s3cret" not in url + assert "alice" not in url + + def test_query_returns_columns_and_rows(fake_jaydebeapi): result = make_client().query("SELECT 1", max_rows=10) assert result.columns == ["a", "b"] @@ -97,6 +110,8 @@ def test_schemas_uses_information_schema(fake_jaydebeapi): cursor.description = [("SCHEMA_NAME", None), ("TYPE", None)] cursor.fetchmany.return_value = [("dfs.tmp", "file")] assert make_client().schemas() == [{"name": "dfs.tmp", "type": "file"}] + sql = cursor.execute.call_args.args[0] + assert "INFORMATION_SCHEMA" in sql def test_tables_rejects_injection(fake_jaydebeapi): @@ -125,13 +140,36 @@ def test_driver_error_does_not_leak_the_password(fake_jaydebeapi): fake_jaydebeapi.connect.side_effect = RuntimeError("auth failed for user alice/s3cret") with pytest.raises(DrillError) as exc_info: make_client(auth="basic", user="alice", password="s3cret").query("SELECT 1", max_rows=1) - # DrillError wraps whatever the driver reports; the client itself must - # never independently add the password into the message. This asserts - # the message is exactly the driver's own text, not a client-composed - # string that embeds config.password. - assert str(exc_info.value) == ( - "could not connect to Drill over JDBC: auth failed for user alice/s3cret" + message = str(exc_info.value) + # The driver's exception text can itself contain the password we passed + # to jaydebeapi.connect() (e.g. an auth-failure message that echoes its + # arguments). The wrapper must scrub it, not merely avoid adding it a + # second time. + assert "s3cret" not in message + # The wrapper must still be informative: the non-secret part of the + # driver's text survives, just with the password redacted. + assert "auth failed for user alice/***REDACTED***" in message + + +def test_query_error_does_not_leak_the_password(fake_jaydebeapi): + cursor = fake_jaydebeapi.connect.return_value.cursor.return_value + cursor.execute.side_effect = RuntimeError("query failed: password was s3cret") + with pytest.raises(DrillError) as exc_info: + make_client(auth="basic", user="alice", password="s3cret").query("SELECT 1", max_rows=1) + assert "s3cret" not in str(exc_info.value) + + +def test_close_error_does_not_leak_the_password(fake_jaydebeapi): + client = make_client(auth="basic", user="alice", password="s3cret") + client.query("SELECT 1", max_rows=1) + fake_jaydebeapi.connect.return_value.close.side_effect = RuntimeError( + "close failed: password was s3cret" ) + with pytest.raises(DrillError) as exc_info: + client.close() + assert "s3cret" not in str(exc_info.value) + # The stale handle must not linger after a failed close. + assert client._connection is None def test_management_methods_are_not_implemented(fake_jaydebeapi):
