fix(csv_sql): prevent SQL injection via DuckDB parameter binding#1408
Merged
Conversation
Contributor
|
Consider adding a test that uses a path containing a single quote (e.g. |
Contributor
Author
|
Thanks! Added a regression test for a CSV filename containing a single quote (O'Reilly.csv) to lock in parameter binding behavior and avoid regressions. |
Jbheemeswar
force-pushed
the
fix/1256-csv-sql-safe-path
branch
from
January 28, 2026 05:23
88a9207 to
f54c00b
Compare
Contributor
Author
|
Hi maintainers - this PR fixes #1256. All required checks are passing, but there is 1 workflow awaiting approval. Thanks! |
Removed detailed docstring arguments and return information for the CSV query function. Improved security checks for SQL queries.
Added security regression tests to reject non-SELECT queries and multi-statement queries.
Hundao
force-pushed
the
fix/1256-csv-sql-safe-path
branch
from
March 30, 2026 15:02
1919e63 to
da75728
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughEnhanced csv_sql: added SQL validation (allow leading SELECT or WITH; reject multi-statement/comment tokens), switched CSV loading to DuckDB parameter binding for the file path, and added tests verifying quoted paths and rejected malicious SQL patterns. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tools/src/aden_tools/tools/csv_tool/csv_tool.py (1)
333-336: Consider adopting BigQuery's word-boundary regex for more robust keyword detection.The current substring check can produce false positives on column/table names containing blocked keywords (e.g.,
SELECT created_at FROM datawould be rejected because"CREATE"is in"CREATED_AT"). The BigQuery tool uses a more robust approach with word-boundary anchors:WRITE_PATTERN = re.compile(r"\bINSERT\b|\bUPDATE\b|...", re.IGNORECASE) if WRITE_PATTERN.search(query): return {"error": "..."}This is existing behavior (lines 339-352 are unchanged), so not blocking this PR, but worth considering as a follow-up improvement.
Also applies to: 339-352
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/src/aden_tools/tools/csv_tool/csv_tool.py` around lines 333 - 336, Replace the naive startswith check on query_upper in the CSV tool with a regex-based word-boundary detection similar to BigQuery's approach: compile a WRITE_PATTERN (using re.compile with r"\b(INSERT|UPDATE|DELETE|CREATE|DROP|ALTER|TRUNCATE|MERGE|REPLACE)\b", re.IGNORECASE)) and use WRITE_PATTERN.search(query) to detect prohibited keywords anywhere as whole words; update the code that currently checks query/query_upper.startswith(...) to use this pattern and return the same error payload when a match is found, referencing the existing query variable and the new WRITE_PATTERN for locating the change.tools/tests/tools/test_csv_tool.py (1)
756-780: Security regression tests cover key acceptance criteria.These tests validate rejection of non-SELECT queries and multi-statement attempts. Consider adding a few more cases for completeness:
- Comment token rejection (validates
--and/* */blocking):def test_reject_sql_comment_dash(self, csv_tools, products_csv, tmp_path): """Reject queries with SQL line comments.""" with patch("aden_tools.tools.file_system_toolkits.security.WORKSPACES_DIR", str(tmp_path)): result = csv_tools["csv_sql"]( path=products_csv.name, workspace_id=TEST_WORKSPACE_ID, agent_id=TEST_AGENT_ID, session_id=TEST_SESSION_ID, query="SELECT * FROM data -- WHERE id = 1", ) assert "error" in result
- WITH statement positive case (validates CTEs work):
def test_with_cte_allowed(self, csv_tools, products_csv, tmp_path): """Allow valid WITH (CTE) queries.""" with patch("aden_tools.tools.file_system_toolkits.security.WORKSPACES_DIR", str(tmp_path)): result = csv_tools["csv_sql"]( path=products_csv.name, workspace_id=TEST_WORKSPACE_ID, agent_id=TEST_AGENT_ID, session_id=TEST_SESSION_ID, query="WITH electronics AS (SELECT * FROM data WHERE category = 'Electronics') SELECT * FROM electronics", ) assert result["success"] is True🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/tests/tools/test_csv_tool.py` around lines 756 - 780, Add two pytest cases to tools/tests/tools/test_csv_tool.py to cover SQL comment rejection and a positive WITH/CTE case: create test_reject_sql_comment_dash that patches aden_tools.tools.file_system_toolkits.security.WORKSPACES_DIR to tmp_path and calls csv_tools["csv_sql"](path=products_csv.name, workspace_id=TEST_WORKSPACE_ID, agent_id=TEST_AGENT_ID, session_id=TEST_SESSION_ID, query="SELECT * FROM data -- WHERE id = 1") and assert that "error" is in the result; and create test_with_cte_allowed that similarly patches WORKSPACES_DIR and calls csv_tools["csv_sql"] with a CTE query "WITH electronics AS (SELECT * FROM data WHERE category = 'Electronics') SELECT * FROM electronics" and assert result["success"] is True; keep naming consistent with existing tests (test_reject_non_select, test_reject_multi_statement) and use the same fixtures (csv_tools, products_csv, tmp_path) and constants (TEST_WORKSPACE_ID, TEST_AGENT_ID, TEST_SESSION_ID).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tools/src/aden_tools/tools/csv_tool/csv_tool.py`:
- Around line 333-336: Replace the naive startswith check on query_upper in the
CSV tool with a regex-based word-boundary detection similar to BigQuery's
approach: compile a WRITE_PATTERN (using re.compile with
r"\b(INSERT|UPDATE|DELETE|CREATE|DROP|ALTER|TRUNCATE|MERGE|REPLACE)\b",
re.IGNORECASE)) and use WRITE_PATTERN.search(query) to detect prohibited
keywords anywhere as whole words; update the code that currently checks
query/query_upper.startswith(...) to use this pattern and return the same error
payload when a match is found, referencing the existing query variable and the
new WRITE_PATTERN for locating the change.
In `@tools/tests/tools/test_csv_tool.py`:
- Around line 756-780: Add two pytest cases to
tools/tests/tools/test_csv_tool.py to cover SQL comment rejection and a positive
WITH/CTE case: create test_reject_sql_comment_dash that patches
aden_tools.tools.file_system_toolkits.security.WORKSPACES_DIR to tmp_path and
calls csv_tools["csv_sql"](path=products_csv.name,
workspace_id=TEST_WORKSPACE_ID, agent_id=TEST_AGENT_ID,
session_id=TEST_SESSION_ID, query="SELECT * FROM data -- WHERE id = 1") and
assert that "error" is in the result; and create test_with_cte_allowed that
similarly patches WORKSPACES_DIR and calls csv_tools["csv_sql"] with a CTE query
"WITH electronics AS (SELECT * FROM data WHERE category = 'Electronics') SELECT
* FROM electronics" and assert result["success"] is True; keep naming consistent
with existing tests (test_reject_non_select, test_reject_multi_statement) and
use the same fixtures (csv_tools, products_csv, tmp_path) and constants
(TEST_WORKSPACE_ID, TEST_AGENT_ID, TEST_SESSION_ID).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a7e4247f-8cde-45bd-86e1-51b8a9ec8d39
📒 Files selected for processing (2)
tools/src/aden_tools/tools/csv_tool/csv_tool.pytools/tests/tools/test_csv_tool.py
Substring matching caused false positives on column names like created_at, updated_at, deleted_at. Switch to \b word-boundary regex. Also add tests for comment rejection, CTE queries, and keyword-in-column-name.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1256
Description
Prevents SQL injection in
csv_sqlby removing SQL string interpolation and using DuckDB parameter binding for CSV paths.Type of Change
Changes Made
;,--,/* */)Testing
pytest tools/tests/tools/test_csv_tool.py)ruff formatandruff check)Checklist
Summary by CodeRabbit