3 Commits

Author	SHA1	Message	Date
Rip&Tear	7f6367a21c	fix(tools): close NL2SQL read-only mode bypasses ... NL2SQLTool's read-only mode could be bypassed three ways, all confirmed by executing the validators directly. 1. `_AS_PAREN_RE` was `\bAS\s*\(`, which never matches PostgreSQL's `AS [NOT] MATERIALIZED (`. `WITH d AS MATERIALIZED (DELETE FROM users RETURNING *) SELECT * FROM d` therefore parsed as having no CTE body at all, and `_validate_statement` returned without running a single check. 2. `_resolve_explain_command` scanned raw text, so a comment between the keywords (`EXPLAIN /*x*/ ANALYZE DELETE FROM users`) stalled option parsing and the statement was treated as an inert EXPLAIN. EXPLAIN ANALYZE executes its argument. 3. The first-keyword allowlist admits statements that begin with SELECT but write, and those survive a transaction rollback: MySQL `SELECT ... INTO OUTFILE` writes a file on the DB server, and `pg_read_file` / `lo_import` / `dblink_exec` reach its filesystem or open a connection outside the transaction. Changes: - Analyse statements over a mask that blanks string literals, dollar-quoted strings, quoted identifiers and comments while preserving offsets, so a keyword in a literal is never matched and one behind a comment always is. MySQL executable comments (`/*! ... */`) are left visible because the server runs them. - Match the `AS [NOT] MATERIALIZED (` spelling. - Validate CTE bodies against an allowlist of read-only leading keywords instead of a write-command denylist, and fail closed: a WITH statement whose CTE bodies cannot be located, or which has no query after them, is now rejected rather than passed through. - Block INTO OUTFILE/DUMPFILE and known server-filesystem functions. - Mark the transaction `SET TRANSACTION READ ONLY` in read-only mode where the backend supports it, so enforcement no longer rests on parsing alone. Backends without the syntax log and fall back. - Split statements on semicolons outside strings and comments, which also stops a semicolon in a literal from being rejected as multi-statement. - Document that a least-privileged read-only DB role is the actual control and these checks are defence in depth. Adds 30 regression tests. Full file: 111 passed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-07-31 08:38:07 +08:00
Greyson LaLonde	c5ea415cda	chore(crewai-tools): drop self-explanatory comments	2026-05-26 16:25:07 -07:00
alex-clawd	ce56472fc3	fix: harden NL2SQLTool — read-only default, query validation, parameterized queries (#5311) ... * fix: harden NL2SQLTool — read-only by default, parameterized queries, query validation Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix: address CI lint failures and remove unused import - Remove unused `sessionmaker` import from test_nl2sql_security.py - Use `Self` return type on `_apply_env_override` (fixes UP037/F821) - Fix ruff errors auto-fixed in lib/crewai (UP007, etc.) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix: expand _WRITE_COMMANDS and block multi-statement semicolon injection - Add missing write commands: UPSERT, LOAD, COPY, VACUUM, ANALYZE, ANALYSE, REINDEX, CLUSTER, REFRESH, COMMENT, SET, RESET - _validate_query() now splits on ';' and validates each statement independently; multi-statement queries are rejected outright in read-only mode to prevent 'SELECT 1; DROP TABLE users' bypass - Extract single-statement logic into _validate_statement() helper - Add TestSemicolonInjection and TestExtendedWriteCommands test classes Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * ci: retrigger * fix: use typing_extensions.Self for Python 3.10 compat * chore: update tool specifications * docs: document NL2SQLTool read-only default and DML configuration Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix: close three NL2SQLTool security gaps (writable CTEs, EXPLAIN ANALYZE, multi-stmt commit) - Remove WITH from _READ_ONLY_COMMANDS; scan CTE body for write keywords so writable CTEs like `WITH d AS (DELETE …) SELECT …` are blocked in read-only mode. - EXPLAIN ANALYZE/ANALYSE now resolves the underlying command; EXPLAIN ANALYZE DELETE is treated as a write and blocked in read-only mode. - execute_sql commit decision now checks ALL semicolon-separated statements so a SELECT-first batch like `SELECT 1; DROP TABLE t` still triggers a commit when allow_dml=True. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix: handle parenthesized EXPLAIN options syntax; remove unused _seed_db _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 <noreply@anthropic.com> * chore: update tool specifications * fix: smarter CTE write detection, fix commit logic for writable CTEs - Replace naive token-set matching with positional AS() body inspection to avoid false positives on column names like 'comment', 'set', 'reset' - Fix execute_sql commit logic to detect writable CTEs (WITH + DELETE/INSERT) not just top-level write commands - Add tests for false positive cases and writable CTE commit behavior - Format nl2sql_tool.py to pass ruff format check * fix: catch write commands in CTE main query + handle whitespace in AS() - WITH cte AS (SELECT 1) DELETE FROM users now correctly blocked - AS followed by newline/tab/multi-space before ( now detected - execute_sql commit logic updated for both cases - 4 new tests * fix: EXPLAIN ANALYZE VERBOSE handling, string literal paren bypass, commit logic for EXPLAIN ANALYZE - EXPLAIN handler now consumes all known options (ANALYZE, ANALYSE, VERBOSE) before extracting the real command, fixing 'EXPLAIN ANALYZE VERBOSE SELECT' being blocked - Paren walker in _extract_main_query_after_cte now skips string literals, preventing 'WITH cte AS (SELECT '\''('\'' FROM t) DELETE FROM users' from bypassing detection - _is_write_stmt in execute_sql now resolves EXPLAIN ANALYZE to underlying command via _resolve_explain_command, ensuring session.commit() fires for write operations - 10 new tests covering all three fixes * fix: deduplicate EXPLAIN parsing, fix AS( regex in strings, block unknown CTE commands, bump langchain-core - Refactor _validate_statement to use _resolve_explain_command (single source of truth) - _iter_as_paren_matches skips string literals so 'AS (' in data doesn't confuse CTE detection - Unknown commands after CTE definitions now blocked in read-only mode - Bump langchain-core override to >=1.2.28 (GHSA-926x-3r5x-gfhw) * fix: add return type annotation to _iter_as_paren_matches --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>	2026-04-09 03:21:38 -03:00