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 fe4ee79d51c1856ce0980affe69a0569ddbf586d Author: cgivre <[email protected]> AuthorDate: Tue Aug 11 17:20:19 2026 -0400 fix: detect Drill login failure from response body, not status code Drill's j_security_check returns HTTP 200 with an HTML error page when credentials are wrong; it does not use a 4xx status for a bad password. The previous status_code>=400 check therefore treated any wrong password as a successful login, leaving the client believing it was authenticated. Now a 200 response is additionally checked for the 'Invalid username/password credentials' marker in the body (matched case-insensitively, tolerant of surrounding markup); a non-2xx status is reported as an endpoint/connection failure instead. Adds regression coverage: 200-with-invalid-credentials-body fails closed, 200-with-ordinary-body succeeds, and a 200-with-invalid- credentials response during a 401-triggered re-auth fails closed without looping or retrying the query. Confirmed the regression test genuinely fails against the prior status-code-only implementation. --- drill_mcp/client_rest.py | 24 ++++++++++++++++++----- tests/test_client_rest.py | 49 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+), 5 deletions(-) diff --git a/drill_mcp/client_rest.py b/drill_mcp/client_rest.py index 4781462..602efaf 100644 --- a/drill_mcp/client_rest.py +++ b/drill_mcp/client_rest.py @@ -47,6 +47,12 @@ from .config import Config # segments) is rejected too. _IDENTIFIER = re.compile(r"^[A-Za-z0-9_$-]+$") +# Drill's j_security_check returns HTTP 200 even on a wrong password; the only +# signal is this marker string inside the HTML error page body. Matched +# case-insensitively, with flexible whitespace, since it arrives embedded in +# markup rather than as the whole body. +_INVALID_CREDENTIALS = re.compile(r"invalid\s+username\s*/\s*password\s+credentials", re.IGNORECASE) + class DrillError(Exception): """Any failure talking to Drill: connection, auth, or query error.""" @@ -124,12 +130,20 @@ class RestClient: except httpx.HTTPError as exc: raise self._transport_error(exc) from exc # Drill's j_security_check endpoint (standard Java EE FORM auth) returns - # 200 for both outcomes when no redirect target is configured; failure - # is not reliably distinguishable by URL shape (an unredirected success - # response's URL also still points at "/j_security_check", so that - # check is vacuous at best and a false positive at worst). Status code - # is the only signal we can trust here. + # HTTP 200 with an HTML error page in the body when credentials are + # wrong -- it does NOT use a 4xx status for a bad password. So status + # code alone cannot detect an authentication failure; a non-2xx here + # means the endpoint itself is unreachable/misbehaving (a connection + # problem), while a wrong password must be detected from the body. + # (Checking the response URL, as an earlier draft did, is also wrong: + # an unredirected *successful* login's URL still points at + # "/j_security_check", so that check false-positives on success.) if response.status_code >= 400: + raise DrillError( + f"authentication endpoint at {self._config.url} returned " + f"HTTP {response.status_code}" + ) + if _INVALID_CREDENTIALS.search(response.text): raise DrillError( f"authentication failed for user {self._config.user!r} at {self._config.url}" ) diff --git a/tests/test_client_rest.py b/tests/test_client_rest.py index affb997..202b420 100644 --- a/tests/test_client_rest.py +++ b/tests/test_client_rest.py @@ -164,6 +164,55 @@ class TestBasicAuth: with pytest.raises(DrillError, match="authentication"): make_client(auth="basic", user="alice", password="s3cret").query("SELECT 1", max_rows=1) + @respx.mock + def test_login_rejects_200_with_invalid_credentials_body(self): + """Drill's j_security_check returns HTTP 200 even on a wrong password; + the failure is only visible in the HTML error page body. This is the + regression test: without checking the body, a wrong password is + silently treated as a successful login.""" + respx.post(f"{BASE}/j_security_check").mock( + return_value=httpx.Response( + 200, + text="<html><body>Invalid username/password credentials</body></html>", + ) + ) + with pytest.raises(DrillError, match="authentication") as exc: + make_client(auth="basic", user="alice", password="s3cret").query("SELECT 1", max_rows=1) + assert "s3cret" not in str(exc.value) + + @respx.mock + def test_login_succeeds_on_200_with_ordinary_body(self): + login = respx.post(f"{BASE}/j_security_check").mock( + return_value=httpx.Response(200, text="<html><body>Welcome</body></html>") + ) + respx.post(f"{BASE}/query.json").mock( + return_value=httpx.Response(200, json={"columns": ["a"], "rows": []}) + ) + result = make_client(auth="basic", user="alice", password="s3cret").query("SELECT 1", max_rows=1) + assert login.called + assert result.columns == ["a"] + + @respx.mock + def test_reauth_fails_closed_on_invalid_credentials_not_looping(self): + """A 401 mid-session triggers one re-login; if that re-login also + reports invalid credentials, the client must fail closed rather than + retry the query or loop.""" + login = respx.post(f"{BASE}/j_security_check").mock( + side_effect=[ + httpx.Response(200, text="<html>Welcome</html>"), + httpx.Response( + 200, + text="<html>Invalid username/password credentials</html>", + ), + ] + ) + query = respx.post(f"{BASE}/query.json").mock(return_value=httpx.Response(401)) + with pytest.raises(DrillError, match="authentication") as exc: + make_client(auth="basic", user="alice", password="s3cret").query("SELECT 1", max_rows=1) + assert "s3cret" not in str(exc.value) + assert login.call_count == 2 + assert query.call_count == 1 + @respx.mock def test_no_login_when_auth_is_none(self): login = respx.post(f"{BASE}/j_security_check").mock(return_value=httpx.Response(200))
