From fec180ea5c06bbec677353442f2db9da2a29636e Mon Sep 17 00:00:00 2001 From: Alex Date: Tue, 7 Apr 2026 08:42:25 -0700 Subject: [PATCH] fix: handle parenthesized EXPLAIN options syntax; remove unused _seed_db MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _validate_statement now strips parenthesized options from EXPLAIN (e.g. EXPLAIN (ANALYZE) DELETE, EXPLAIN (ANALYZE, VERBOSE) DELETE) before checking whether ANALYZE/ANALYSE is present — closing the bypass where the options-list form was silently allowed in read-only mode. Adds three new tests: - EXPLAIN (ANALYZE) DELETE → blocked - EXPLAIN (ANALYZE, VERBOSE) DELETE → blocked - EXPLAIN (VERBOSE) SELECT → allowed Also removes the unused _seed_db helper from test_nl2sql_security.py. Co-Authored-By: Claude Sonnet 4.6 --- .../crewai_tools/tools/nl2sql/nl2sql_tool.py | 33 ++++++++++++++----- .../tests/tools/test_nl2sql_security.py | 26 ++++++++++----- 2 files changed, 41 insertions(+), 18 deletions(-) diff --git a/lib/crewai-tools/src/crewai_tools/tools/nl2sql/nl2sql_tool.py b/lib/crewai-tools/src/crewai_tools/tools/nl2sql/nl2sql_tool.py index c742c26b7..54024ba71 100644 --- a/lib/crewai-tools/src/crewai_tools/tools/nl2sql/nl2sql_tool.py +++ b/lib/crewai-tools/src/crewai_tools/tools/nl2sql/nl2sql_tool.py @@ -178,16 +178,31 @@ class NL2SQLTool(BaseTool): # EXPLAIN ANALYZE / EXPLAIN ANALYSE actually *executes* the underlying # query. Resolve the real command so write operations are caught. + # Handles both space-separated ("EXPLAIN ANALYZE DELETE …") and + # parenthesized ("EXPLAIN (ANALYZE) DELETE …", "EXPLAIN (ANALYZE, VERBOSE) DELETE …"). if command == "EXPLAIN": - tokens = stmt.strip().lstrip("(").split() - if len(tokens) >= 2 and tokens[1].upper().rstrip(";") in ( - "ANALYZE", - "ANALYSE", - ): - # The statement being explained starts at the third token. - if len(tokens) >= 3: - command = tokens[2].upper().rstrip(";") - # else: bare "EXPLAIN ANALYZE" with no query — treat as read-only. + rest = stmt.strip()[len("EXPLAIN"):].strip() + analyze_found = False + + if rest.startswith("("): + # Parenthesized options: EXPLAIN (ANALYZE, VERBOSE, …) + close = rest.find(")") + if close != -1: + options_str = rest[1:close].upper() + analyze_found = any( + opt.strip() in ("ANALYZE", "ANALYSE") + for opt in options_str.split(",") + ) + rest = rest[close + 1:].strip() + else: + # Space-separated: EXPLAIN ANALYZE + first_opt = rest.split()[0].upper().rstrip(";") if rest.split() else "" + if first_opt in ("ANALYZE", "ANALYSE"): + analyze_found = True + rest = rest[len(first_opt):].strip() + + if analyze_found and rest: + command = rest.split()[0].upper().rstrip(";") # WITH starts a CTE. Read-only CTEs are fine; writable CTEs # (e.g. WITH d AS (DELETE …) SELECT …) must be blocked in read-only mode. diff --git a/lib/crewai-tools/tests/tools/test_nl2sql_security.py b/lib/crewai-tools/tests/tools/test_nl2sql_security.py index 60d4f2959..614c82d6f 100644 --- a/lib/crewai-tools/tests/tools/test_nl2sql_security.py +++ b/lib/crewai-tools/tests/tools/test_nl2sql_security.py @@ -36,15 +36,6 @@ def _make_tool(allow_dml: bool = False, **kwargs) -> NL2SQLTool: return NL2SQLTool(db_uri=SQLITE_URI, allow_dml=allow_dml, **kwargs) -def _seed_db(uri: str) -> None: - """Create a tiny table in the target database for DML tests.""" - engine = create_engine(uri) - with engine.connect() as conn: - conn.execute(text("CREATE TABLE IF NOT EXISTS users (id INTEGER PRIMARY KEY, name TEXT)")) - conn.execute(text("INSERT INTO users VALUES (1, 'alice')")) - conn.commit() - - # --------------------------------------------------------------------------- # Read-only enforcement (allow_dml=False) # --------------------------------------------------------------------------- @@ -404,6 +395,23 @@ class TestExplainAnalyze: tool = _make_tool(allow_dml=True) tool._validate_query("EXPLAIN ANALYZE DELETE FROM users") + def test_explain_paren_analyze_delete_blocked_in_read_only(self): + """EXPLAIN (ANALYZE) DELETE actually runs the delete — block it.""" + tool = _make_tool(allow_dml=False) + with pytest.raises(ValueError, match="read-only mode"): + tool._validate_query("EXPLAIN (ANALYZE) DELETE FROM users") + + def test_explain_paren_analyze_verbose_delete_blocked_in_read_only(self): + """EXPLAIN (ANALYZE, VERBOSE) DELETE actually runs the delete — block it.""" + tool = _make_tool(allow_dml=False) + with pytest.raises(ValueError, match="read-only mode"): + tool._validate_query("EXPLAIN (ANALYZE, VERBOSE) DELETE FROM users") + + def test_explain_paren_verbose_select_allowed_in_read_only(self): + """EXPLAIN (VERBOSE) SELECT is safe — no ANALYZE means no execution.""" + tool = _make_tool(allow_dml=False) + tool._validate_query("EXPLAIN (VERBOSE) SELECT * FROM users") + # --------------------------------------------------------------------------- # Multi-statement commit covers ALL statements (not just the first)