Security: Strengthen SQL validation to prevent non-SELECT operations

[coderabbitai]

Problem

The current implementation of query_sql in cratedb_mcp/__main__.py attempts to restrict queries to read-only SELECT statements but uses a weak validation mechanism that can be bypassed.

As demonstrated in test_query_sql_permitted_exploit(), the current validation only checks for the substring 'select' in the query string, which allows SQL injection attacks where the word 'select' is included in the values of an INSERT statement or other data modification queries.

Suggested Solution

Replace the naive string check with a more robust validation using a dedicated SQL parser library like sqlparse:

import sqlparse

def query_sql(query: str) -> Dict[str, Any]:
    # Parse the SQL statement
    parsed = sqlparse.parse(query.strip())
    
    # Check if it's a SELECT statement
    if not parsed or parsed[0].get_type().upper() != 'SELECT':
        raise ValueError('Only SELECT statements are allowed')
    
    # Rest of the function...

Benefits of using sqlparse:

• Properly handles SQL syntax
• Correctly identifies statement types
• More resilient against bypass attempts
• Handles edge cases like comments and whitespace

Implementation Steps

1. Add sqlparse as a dependency in pyproject.toml
2. Modify query_sql to use sqlparse for statement validation
3. Update tests to verify the improved validation works correctly

References

• Identified in PR: Boilerplate: Add software tests and CI configuration #9
• Discussion: Boilerplate: Add software tests and CI configuration #9 (comment)
• sqlparse documentation: https://sqlparse.readthedocs.io/

[amotl]

@coderabbitai: GH-11 will improve the situation.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Fetch details about issues #10 and #11
echo "=== Issue #10 Details ==="
gh issue view 10 --repo crate/cratedb-mcp

echo -e "\n=== Issue/PR #11 Details ==="
# Try to view as issue first
gh issue view 11 --repo crate/cratedb-mcp || echo "Not found as issue, trying as PR"
# If not found as issue, try as PR
if [ $? -ne 0 ]; then
  gh pr view 11 --repo crate/cratedb-mcp || echo "Not found as PR either"
fi

Length of output: 2547

---

@amotl Thank you for letting me know! I see that you've created issue #11 "SQL: Stronger read-only mode" to implement the more robust SQL validation using sqlparse as suggested in issue #10. This will indeed address the security vulnerability in the current implementation. If you need any additional assistance with the implementation or testing, please let me know.

[amotl]

@proddata mentioned at #11 (comment) ff.:

I would like to challenge the overall direction of the patch GH-11 and its relevance from a user perspective. It seems the main motivation is to enforce that only SELECT queries are allowed via the MCP endpoint — but that raises the question: what’s the actual user need or use case behind this constraint of restricting that endpoint?

It feels like we’re partly re-implementing CrateDB’s built-in permission system externally here, that's why I am curious to hear the reasoning behind this approach.

@proddata suggested to just limit the database users privileges used by the MCP server instead, and I think it is a good idea.

/cc @coderabbitai

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Fetch details about issues #10 and #11
echo "=== Issue #10 Details ==="
gh issue view 10 --repo crate/cratedb-mcp

echo -e "\n=== Issue/PR #11 Details ==="
# Try to view as issue first
gh issue view 11 --repo crate/cratedb-mcp || echo "Not found as issue, trying as PR"
# If not found as issue, try as PR
if [ $? -ne 0 ]; then
  gh pr view 11 --repo crate/cratedb-mcp || echo "Not found as PR either"
fi

# Find the main SQL endpoint in the codebase
echo -e "\n=== Searching for SQL Endpoint Code ==="
fd -e py -e md . | grep -i "sql\|mcp" | xargs grep -l "endpoint\|route" | head -n 5 | xargs cat

Length of output: 2740

---

🏁 Script executed:

#!/bin/bash
# First, let's look at the query_sql function to understand the current implementation
echo "=== query_sql Function Implementation ==="
fd --type file --glob '*__main__.py' | xargs grep -A 20 -B 20 "def query_sql"

# Check PR #11 to see the proposed changes and conversation
echo -e "\n=== PR #11 Conversation ==="
gh pr view 11 --repo crate/cratedb-mcp --comments

# Let's see how database connections are handled (to understand user permissions context)
echo -e "\n=== Database Connection Code ==="
fd --type file | xargs grep -l "connect\|cursor\|database\|psycopg" | head -n 5 | xargs cat

# Look for any information about user permissions in the database
echo -e "\n=== Database User Permission Info ==="
fd --type file | xargs grep -l "permission\|privilege\|user\|role" | head -n 5 | xargs cat

Length of output: 67558

---

🏁 Script executed:

#!/bin/bash
# First, let's look at more details about PR #11 including the comment mentioned
echo "=== PR #11 Comment Details ==="
gh pr view 11 --repo crate/cratedb-mcp --comments

# Let's see how database connections are currently handled
echo -e "\n=== Database Connection Code ==="
fd -t f . | xargs grep -l "db\|connect\|crate\|cursor\|sql" | grep -v "test_" | head -n 10 | xargs cat || echo "No database connection files found"

# Let's get details about how the application is configured
echo -e "\n=== App Configuration ==="
find . -name "settings.py" | xargs cat

Length of output: 85599

---

@amotl I agree with @proddata that restricting the database user's privileges would be a more robust approach to security in this case. Implementing security at the database level has several advantages:

1. More robust enforcement: Database privileges are enforced by the database engine and cannot be bypassed by application code or SQL injection techniques.

2. Simplified application code: It eliminates the need for complex SQL parsing and validation in your application.

3. Reduced maintenance: You don't need to keep up with evolving SQL syntax or edge cases in your validation logic.

To implement this approach, you would:

• Create a dedicated read-only user for the MCP server
• Grant SELECT privileges only, without INSERT/UPDATE/DELETE/etc. permissions
• Use this restricted user for all database connections from the MCP server

That said, the current PR does improve security over the naive string check, so it's not without value. In fact, a defense-in-depth strategy would suggest implementing both:

• Database-level privileges as the primary security mechanism
• Application-level validation for better user experience (providing clear error messages)

Would you like me to help with documentation for setting up the appropriate database user privileges in CrateDB?

[amotl]

@coderabbitai: Yes, it would be sweet if you can assist us here. Optimally, please submit a patch using a GitHub pull request, which will add a section about security matters to the repository's README.md file, outlining a walkthrough how to implement the suggested approach with CrateDB. You may want to consider the resources referenced below to learn about fundamental principles how CrateDB's user and privilege management works.

• https://cratedb.com/docs/crate/reference/en/latest/_sources/admin/user-management.rst.txt
• https://cratedb.com/docs/crate/reference/en/latest/_sources/admin/privileges.rst.txt
• https://community.cratedb.com/raw/1153/1

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

A summary of the edits CodeRabbit can apply:

We will add a Security Considerations section to README.md explaining how to set up a dedicated read-only CrateDB user for MCP.

A plan of each step that CodeRabbit will take in order to execute the edits for each file (with inline rough draft snippets):

➕ README.md

Insert the following block before the Install section (before line 48):

## Security Considerations

To enforce robust access control, it is recommended to configure a dedicated read-only database user for the MCP server. Database-level privileges are enforced by CrateDB and cannot be bypassed by application code.

1. Create a new user in CrateDB:
   ```sql
   CREATE USER mcp_readonly WITH (password = 'your_password');

2. Grant SELECT privileges on all existing tables in your schema:

   GRANT SELECT ON ALL TABLES IN SCHEMA your_schema TO mcp_readonly;

3. (Optional) Ensure future tables inherit SELECT privileges:

   ALTER DEFAULT PRIVILEGES IN SCHEMA your_schema GRANT SELECT ON TABLES TO mcp_readonly;

4. Update the MCP server connection to use the read-only credentials:

   export CRATEDB_MCP_HTTP_URL="http://mcp_readonly:your_password@localhost:4200/"

5. (Optional) Maintain application-level SQL validation for clear error feedback (defense in depth).

Ensure the code fences and indentation match the existing README style.

---

Verify that the new section renders correctly on GitHub by previewing the updated README.md.

---

Commit the changes and push the branch, then open a pull request titled “docs: add security considerations for read-only CrateDB user (GH-11)”.

</details>



A summary of the context that CodeRabbit is considering across the codebase:

> We need to include the correct SQL syntax for creating a user in CrateDB from the official documentation to make the README instructions accurate.

> We need to know the exact syntax for granting SELECT privileges in CrateDB to a user so our README walkthrough is precise.

- [ ] <!-- {"checkboxId": "32599f3c-a2a5-4bb9-b887-dee90db62b97", "radioGroupId": "edit-actions-group-2886347201"} --> ✅ Create PR with these edits
- [ ] <!-- {"checkboxId": "6b3df8e2-79f5-4eb8-9d4a-c7d3a6a5f7b1", "radioGroupId": "edit-actions-group-2886347201"} --> 📋 Get copyable edits

<!-- <agentComponentType>plan-edits</agentComponentType> -->

<!-- <rawResChunk><rationale>
The user requests adding a new section about security and user privilege management for CrateDB in the repository's README.md file. This section should provide a walkthrough on how to implement a restricted read-only database user for the MCP server, referencing CrateDB's user and privilege management principles.

The relevant file to edit is README.md, as it is the main documentation file where the new security section will be added.

No other files need to be edited or created for this request.
</rationale>
<toEditFile>README.md</toEditFile></rawResChunk> -->

<!-- <rawResChunk><rationale>
README.md: This is the primary repository README where the new “Security” section will be added.  
*.rst files (e.g., in docs/): Any existing ReStructuredText documentation may need to be cross-referenced or updated to keep docs consistent.  
settings.py / config*.py: These Python configuration modules likely define how the MCP server connects to CrateDB (host, port, user, password) and are where one would point to the new read-only user.  
docker-compose*.yml / *.yaml: If the repository includes Docker Compose definitions for CrateDB and the MCP server, these files will show how credentials are passed and should be updated to use the new restricted user.  
.env / .env.example: Environment variable files often hold credentials for local development; they will need example entries for the read-only user.  
Python files establishing DB connections (e.g., those importing crate.client or calling connect()): Reviewing these will clarify exactly which connection parameters to document in the README.
</rationale>

<generated_script>
```shell
#!/bin/bash
set -e

echo "=== Locating README files ==="
fd --type f --ignore-case --glob "README.md"

echo -e "\n=== Locating ReStructuredText documentation ==="
fd --type f --ignore-case --glob "*.rst"

echo -e "\n=== Locating Python configuration modules ==="
fd --type f --ignore-case --glob "settings.py"
fd --type f --ignore-case --glob "config*.py"

echo -e "\n=== Locating Docker Compose files ==="
fd --type f --ignore-case --glob "docker-compose*.yml" --glob "*.yaml"

echo -e "\n=== Locating environment variable files ==="
fd --type f --ignore-case --glob ".env*" 

echo -e "\n=== Locating Python files that establish DB connections ==="
# Look for crate.client usage or generic connect patterns
fd --type f --ignore-case --glob "*.py" | xargs grep -H -nE "crate\.client|connect" || true

</generated_script> -->

[amotl]

There is another suggestion to prevent anything else than a SELECT query, by wrapping the generated query into a subselect statement like:

SELECT * FROM (<generated query>) generated;

I am not sure about it, because it would possibly not catch evasive or camouflage SQL statements?

/cc @coderabbitai

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Let's look at issue #10 to understand the security concern
echo "=== Issue #10 Details ==="
gh issue view 10 --repo crate/cratedb-mcp

# Look at PR #11 to understand the proposed changes
echo -e "\n=== PR #11 Details ==="
gh pr view 11 --repo crate/cratedb-mcp

# Let's try to find where SQL queries are executed to understand the current implementation
echo -e "\n=== SQL Query Implementation ==="
fd --type file --extension py | xargs grep -l "execute" | xargs grep -A 5 -B 5 "execute" || echo "No matches found"

# Let's see if there's any implementation of SQL validation or sanitization
echo -e "\n=== SQL Validation/Sanitization ==="
fd --type file --extension py | xargs grep -l "sql" | xargs grep -A 5 -B 5 "sanitize\|validate\|parse\|check" || echo "No matches found"

Length of output: 7426

---

@amotl You're right to be cautious about the subselect wrapper approach. While wrapping queries in SELECT * FROM (<generated query>) generated; would prevent direct execution of most non-SELECT operations, it has several limitations:

1. Limited protection against camouflage SQL:

   • It won't catch all evasive techniques that the current sqlparse implementation detects
   • Functions with side effects could still be executed in subqueries
   • Some databases allow exploits through window functions or CTEs even in subqueries

2. False sense of security:

   • Less robust than proper parsing with sqlparse which performs syntactic analysis
   • Your current implementation already checks for SELECT INTO and multiple statements, which are more thorough

3. Potential compatibility issues:

   • Could break legitimate complex queries (especially those with CTEs)
   • Might alter the shape of result sets in unexpected ways

The current approach using sqlparse is more robust because it:

• Properly parses SQL to identify statement types
• Specifically checks for camouflage patterns
• Provides better error messages

I recommend sticking with your current approach of:

1. Database-level privileges (dedicated read-only user) as primary defense
2. Application-level validation via sqlparse as a secondary layer

The subselect wrapper could be added as a third layer, but it introduces complexity without significantly improving security beyond what you already have.

[amotl]

=> It's not just about preventing non-SELECT operations, it is actually about preventing non-DQL operations, i.e. only permitting read-only ones. I think adding database-level privileges into the mix is a good idea, as suggested:

[Use] database-level privileges (dedicated read-only user) as primary defense.