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 <[email protected]> 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
