You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@drill.apache.org by cg...@apache.org on 2026/08/12 20:24:55 UTC
(drill-mcp) 18/44: fix: make invalid-credentials detection fail closed, not fail clever
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 a9fe883134ee0d9b83a54caa26933cfb2f3e6387
Author: cgivre <cg...@gmail.com>
AuthorDate: Tue Aug 11 19:07:57 2026 -0400
fix: make invalid-credentials detection fail closed, not fail clever
The previous fix stripped HTML tags with <[^>]+> before matching the
invalid-credentials marker, but that regex is greedy across any
'<...>' span, not just well-formed tags. A stray unmatched '<' before
the marker (e.g. '1 < 2' in unrelated error-page text) makes the
substitution eat everything up to the next unrelated '>' in the
document, including the marker itself -- so a genuinely failed login
could be silently accepted as successful. Verified this against the
previously-committed code before fixing it.
Fix: check both the raw body and the tag-stripped body, treating a
match in EITHER as a failure. The union of two detectors can only find
more than either alone, so no stripping bug can ever suppress a
detection the raw search would have made -- stripping now only adds
the tags-inside-the-phrase case it was introduced for, and can no
longer cause a false negative on its own. Also tightened _HTML_TAG to
require '/', a letter, or '!' after '<' (</?[A-Za-z!][^>]*>), so it
approximates real tag grammar instead of matching any bracketed span;
kept the union check regardless, since that's what makes the
fail-closed property hold independent of how well the tag regex
behaves.
Added five tests: the reviewer's exact stray-'<'-before-marker case
(mechanically confirmed to fail against the prior implementation), an
unclosed '<' with no matching '>' anywhere, a plain marker with no
markup, and two fail-closed-direction checks confirming ordinary
successful logins (plain body, and a body with incidental but
well-formed '<'/'>' characters) still succeed.
---
drill_mcp/client_rest.py | 32 ++++++++++++++++++++-----
tests/test_client_rest.py | 61 +++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 87 insertions(+), 6 deletions(-)
diff --git a/drill_mcp/client_rest.py b/drill_mcp/client_rest.py
index 3138c7a..cfda3a0 100644
--- a/drill_mcp/client_rest.py
+++ b/drill_mcp/client_rest.py
@@ -49,18 +49,38 @@ _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, against the body with HTML
-# tags stripped first -- the marker can arrive with tags inside the phrase
-# (e.g. "Invalid<br>username/password credentials"), which a plain regex
-# search against the raw markup would miss.
+# case-insensitively, with flexible whitespace.
_INVALID_CREDENTIALS = re.compile(r"invalid\s+username\s*/\s*password\s+credentials", re.IGNORECASE)
-_HTML_TAG = re.compile(r"<[^>]+>")
+
+# Approximates real tag grammar (requires '/', a letter, or '!' after '<') so
+# a stray unmatched '<' -- e.g. "1 < 2" in unrelated page text -- can't be
+# mistaken for the start of a tag and swallow everything up to the next
+# unrelated '>' in the document, potentially deleting the marker itself.
+_HTML_TAG = re.compile(r"</?[A-Za-z!][^>]*>")
def _strip_tags(html: str) -> str:
return _HTML_TAG.sub(" ", html)
+def _contains_invalid_credentials_marker(body: str) -> bool:
+ """True if `body` contains Drill's invalid-credentials marker.
+
+ Checks the raw body AND the tag-stripped body, and treats a match in
+ EITHER as a failure. Stripping is needed to catch the marker when tags
+ fall inside the phrase (e.g. "Invalid<br>username/password credentials"),
+ but stripping can never be trusted alone: a tag-stripping regex can only
+ approximate real HTML grammar, and any case where it over-strips (turning
+ unrelated text into something that looks like a tag) would delete the
+ marker and silently accept a failed login as successful. Checking the raw
+ body first means no stripping bug can ever suppress a detection the raw
+ search would have made on its own -- the union can only find more than
+ either check alone, never less. This is the correct posture for a check
+ that gates authentication: fail closed, not fail clever.
+ """
+ return bool(_INVALID_CREDENTIALS.search(body) or _INVALID_CREDENTIALS.search(_strip_tags(body)))
+
+
class DrillError(Exception):
"""Any failure talking to Drill: connection, auth, or query error."""
@@ -167,7 +187,7 @@ class RestClient:
f"authentication endpoint at {self._config.url} returned "
f"HTTP {response.status_code}"
)
- if _INVALID_CREDENTIALS.search(_strip_tags(response.text)):
+ if _contains_invalid_credentials_marker(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 80c8e74..87a0adb 100644
--- a/tests/test_client_rest.py
+++ b/tests/test_client_rest.py
@@ -248,6 +248,50 @@ class TestBasicAuth:
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_rejects_marker_after_a_stray_unmatched_angle_bracket(self):
+ """Regression test: a naive tag-strip regex (`<[^>]+>`) treats any
+ '<...>' span as a tag, so a stray unmatched '<' before the marker
+ (e.g. '1 < 2' in unrelated error text) makes the substitution eat
+ everything up to the next unrelated '>' in the document -- including
+ the marker itself -- turning a genuinely failed login into an
+ apparent success. Checking the raw body as well as the stripped body
+ is what prevents that."""
+ respx.post(f"{BASE}/j_security_check").mock(
+ return_value=httpx.Response(
+ 200,
+ text=(
+ "<div>Warning: 1 < 2 in the system. "
+ "Invalid username/password credentials</div>"
+ ),
+ )
+ )
+ 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_rejects_marker_after_an_unclosed_angle_bracket(self):
+ """A stray '<' with no matching '>' anywhere in the body at all."""
+ respx.post(f"{BASE}/j_security_check").mock(
+ return_value=httpx.Response(
+ 200,
+ text="value < 5. Invalid username/password credentials",
+ )
+ )
+ 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_rejects_plain_marker_with_no_markup(self):
+ respx.post(f"{BASE}/j_security_check").mock(
+ return_value=httpx.Response(200, text="Invalid username/password credentials")
+ )
+ 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(
@@ -260,6 +304,23 @@ class TestBasicAuth:
assert login.called
assert result.columns == ["a"]
+ @respx.mock
+ def test_login_succeeds_with_incidental_angle_brackets_in_a_normal_body(self):
+ """Fail-closed direction: confirm the union check (raw OR stripped)
+ has not made ordinary successful logins start failing just because
+ the body happens to contain '<' and '>' characters."""
+ login = respx.post(f"{BASE}/j_security_check").mock(
+ return_value=httpx.Response(
+ 200, text="<div>Welcome back. Your balance is < 100 and > 0.</div>"
+ )
+ )
+ 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