Skip to content

Commit 4dadcbc

Browse files
committed
SQL: Stronger read-only mode, using sqlparse
1 parent 4f122fd commit 4dadcbc

File tree

11 files changed

+275
-13
lines changed

11 files changed

+275
-13
lines changed

CHANGES.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,3 +14,4 @@
1414
- MCP docs: Used Hishel for transparently caching documentation resources,
1515
by default for one hour. Use the `CRATEDB_MCP_DOCS_CACHE_TTL` environment
1616
variable to adjust (default: 3600)
17+
- SQL: Stronger read-only mode, using `sqlparse`

README.md

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -40,10 +40,12 @@ LLMs can still hallucinate and give incorrect answers.
4040
* What is the storage consumption of my tables, give it in a graph.
4141
* How can I format a timestamp column to '2019 Jan 21'.
4242

43-
# Data integrity
44-
We do not recommend letting the LLMs insert data or modify columns by itself, as such we tell the
45-
LLMs to only allow 'SELECT' statements in the `cratedb_sql` tool, you can jailbreak this against
46-
our recommendation.
43+
# Read-only access
44+
We do not recommend letting LLM-based agents insert or modify data by itself.
45+
As such, only `SELECT` statements are permitted and forwarded to the database.
46+
All other operations will raise a `ValueError` exception, unless the
47+
`CRATEDB_MCP_PERMIT_ALL_STATEMENTS` environment variable is set to a
48+
truthy value.
4749

4850
# Install
4951
```shell

cratedb_mcp/__main__.py

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44

55
from .knowledge import DOCUMENTATION_INDEX, Queries
66
from .settings import DOCS_CACHE_TTL, HTTP_URL
7+
from .util.sql import sql_is_permitted
78

89
# Configure Hishel, an httpx client with caching.
910
# Define one hour of caching time.
@@ -22,7 +23,7 @@ def query_cratedb(query: str) -> list[dict]:
2223
@mcp.tool(description="Send a SQL query to CrateDB, only 'SELECT' queries are allows, queries that"
2324
" modify data, columns or are otherwise deemed un-safe are rejected.")
2425
def query_sql(query: str):
25-
if 'select' not in query.lower():
26+
if not sql_is_permitted(query):
2627
raise ValueError('Only queries that have a SELECT statement are allowed.')
2728
return query_cratedb(query)
2829

cratedb_mcp/settings.py

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
import os
22
import warnings
33

4+
from attr.converters import to_bool
5+
46
HTTP_URL: str = os.getenv("CRATEDB_MCP_HTTP_URL", "http://localhost:4200")
57

68
# Configure cache lifetime for documentation resources.
@@ -12,3 +14,12 @@
1214
# TODO: Add software test after refactoring away from module scope.
1315
warnings.warn(f"Environment variable `CRATEDB_MCP_DOCS_CACHE_TTL` invalid: {e}. "
1416
f"Using default value: {DOCS_CACHE_TTL}.", category=UserWarning, stacklevel=2)
17+
18+
19+
# Whether to permit all statements. By default, only SELECT operations are permitted.
20+
PERMIT_ALL_STATEMENTS: bool = to_bool(os.getenv("CRATEDB_MCP_PERMIT_ALL_STATEMENTS", "false"))
21+
22+
# TODO: Refactor into code which is not on the module level. Use OOM early.
23+
if PERMIT_ALL_STATEMENTS: # pragma: no cover
24+
warnings.warn("All types of SQL statements are permitted. This means the LLM "
25+
"agent can write and modify the connected database", category=UserWarning, stacklevel=2)

cratedb_mcp/util/__init__.py

Whitespace-only changes.

cratedb_mcp/util/sql.py

Lines changed: 126 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,126 @@
1+
import dataclasses
2+
import logging
3+
import typing as t
4+
5+
import sqlparse
6+
from sqlparse.tokens import Keyword
7+
8+
from cratedb_mcp.settings import PERMIT_ALL_STATEMENTS
9+
10+
logger = logging.getLogger(__name__)
11+
12+
13+
def sql_is_permitted(expression: str) -> bool:
14+
"""
15+
Validate the SQL expression, only permit read queries by default.
16+
17+
When the `CRATEDB_MCP_PERMIT_ALL_STATEMENTS` environment variable is set,
18+
allow all types of statements.
19+
20+
FIXME: Revisit implementation, it might be too naive or weak.
21+
Issue: https://github.com/crate/cratedb-mcp/issues/10
22+
Question: Does SQLAlchemy provide a solid read-only mode, or any other library?
23+
"""
24+
is_dql = SqlStatementClassifier(expression=expression, permit_all=PERMIT_ALL_STATEMENTS).is_dql
25+
if is_dql:
26+
logger.info(f"Permitted SQL expression: {expression and expression[:50]}...")
27+
else:
28+
logger.warning(f"Denied SQL expression: {expression and expression[:50]}...")
29+
return is_dql
30+
31+
32+
@dataclasses.dataclass
33+
class SqlStatementClassifier:
34+
"""
35+
Helper to classify an SQL statement.
36+
37+
Here, most importantly: Provide the `is_dql` property that
38+
signals truthfulness for read-only SQL SELECT statements only.
39+
"""
40+
expression: str
41+
permit_all: bool = False
42+
43+
_parsed_sqlparse: t.Any = dataclasses.field(init=False, default=None)
44+
45+
def __post_init__(self) -> None:
46+
if self.expression is None:
47+
self.expression = ""
48+
if self.expression:
49+
self.expression = self.expression.strip()
50+
51+
def parse_sqlparse(self) -> t.List[sqlparse.sql.Statement]:
52+
"""
53+
Parse expression using traditional `sqlparse` library.
54+
"""
55+
if self._parsed_sqlparse is None:
56+
self._parsed_sqlparse = sqlparse.parse(self.expression)
57+
return self._parsed_sqlparse
58+
59+
@property
60+
def is_dql(self) -> bool:
61+
"""
62+
Whether the statement is a DQL statement, which effectively invokes read-only operations only.
63+
"""
64+
65+
if not self.expression:
66+
return False
67+
68+
if self.permit_all:
69+
return True
70+
71+
# Check if the expression is valid and if it's a DQL/SELECT statement,
72+
# also trying to consider `SELECT ... INTO ...` and evasive
73+
# `SELECT * FROM users; \uff1b DROP TABLE users` statements.
74+
return self.is_select and not self.is_camouflage
75+
76+
@property
77+
def is_select(self) -> bool:
78+
"""
79+
Whether the expression is an SQL SELECT statement.
80+
"""
81+
return self.operation == 'SELECT'
82+
83+
@property
84+
def operation(self) -> str:
85+
"""
86+
The SQL operation: SELECT, INSERT, UPDATE, DELETE, CREATE, etc.
87+
"""
88+
parsed = self.parse_sqlparse()
89+
return parsed[0].get_type().upper()
90+
91+
@property
92+
def is_camouflage(self) -> bool:
93+
"""
94+
Innocent-looking `SELECT` statements can evade filters.
95+
"""
96+
return self.is_select_into or self.is_evasive
97+
98+
@property
99+
def is_select_into(self) -> bool:
100+
"""
101+
Use traditional `sqlparse` for catching `SELECT ... INTO ...` statements.
102+
Examples:
103+
SELECT * INTO foobar FROM bazqux
104+
SELECT * FROM bazqux INTO foobar
105+
"""
106+
# Flatten all tokens (including nested ones) and match on type+value.
107+
statement = self.parse_sqlparse()[0]
108+
return any(
109+
token.ttype is Keyword and token.value.upper() == "INTO"
110+
for token in statement.flatten()
111+
)
112+
113+
@property
114+
def is_evasive(self) -> bool:
115+
"""
116+
Use traditional `sqlparse` for catching evasive SQL statements.
117+
118+
A practice picked up from CodeRabbit was to reject multiple statements
119+
to prevent potential SQL injections. Is it a viable suggestion?
120+
121+
Examples:
122+
123+
SELECT * FROM users; \uff1b DROP TABLE users
124+
"""
125+
parsed = self.parse_sqlparse()
126+
return len(parsed) > 1

docs/backlog.md

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
# CrateDB MCP backlog
2+
3+
## Iteration +1
4+
- Docs: HTTP caching
5+
- Docs: Load documentation index from custom outline file
6+
- SQL: Extract `SqlFilter` or `SqlGateway` functionality to `cratedb-sqlparse`
7+
8+
## Done
9+
- SQL: Stronger read-only mode

pyproject.toml

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,8 +20,10 @@ dynamic = [
2020
"version",
2121
]
2222
dependencies = [
23+
"attrs",
2324
"hishel<0.2",
2425
"mcp[cli]>=1.5.0",
26+
"sqlparse<0.6",
2527
]
2628
optional-dependencies.develop = [
2729
"mypy<1.16",

tests/test_knowledge.py

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,3 +16,5 @@ def test_queries():
1616
assert "sys.health" in Queries.TABLES_METADATA
1717
assert "WITH partitions_health" in Queries.TABLES_METADATA
1818
assert "LEFT JOIN" in Queries.TABLES_METADATA
19+
20+

tests/test_mcp.py

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -24,19 +24,20 @@ def test_fetch_docs_permitted():
2424
assert "initcap" in response
2525

2626

27-
def test_query_sql_forbidden():
27+
def test_query_sql_permitted():
28+
assert query_sql("SELECT 42")["rows"] == [[42]]
29+
30+
31+
def test_query_sql_forbidden_easy():
2832
with pytest.raises(ValueError) as ex:
2933
assert "RelationUnknown" in str(query_sql("INSERT INTO foobar (id) VALUES (42) RETURNING id"))
3034
assert ex.match("Only queries that have a SELECT statement are allowed")
3135

3236

33-
def test_query_sql_permitted():
34-
assert query_sql("SELECT 42")["rows"] == [[42]]
35-
36-
37-
def test_query_sql_permitted_exploit():
38-
# FIXME: Read-only protection must become stronger.
39-
query_sql("INSERT INTO foobar (operation) VALUES ('select')")
37+
def test_query_sql_forbidden_sneak_value():
38+
with pytest.raises(ValueError) as ex:
39+
query_sql("INSERT INTO foobar (operation) VALUES ('select')")
40+
assert ex.match("Only queries that have a SELECT statement are allowed")
4041

4142

4243
def test_get_table_metadata():

0 commit comments

Comments
 (0)