README: Add recommendations to use a read-only database user #25
Conversation
|
Warning Rate limit exceeded@amotl has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 0 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (6)
WalkthroughThis change updates documentation to recommend using a read-only database user for enhanced security, modifies the application to raise a Changes
Sequence Diagram(s)sequenceDiagram
participant Agent
participant MCP_Server
participant Database
Agent->>MCP_Server: Submit SQL query
MCP_Server->>MCP_Server: Check if query is SELECT
alt Query is SELECT
MCP_Server->>Database: Execute query
Database-->>MCP_Server: Return results
MCP_Server-->>Agent: Return results
else Query is not SELECT
MCP_Server-->>Agent: Raise PermissionError
end
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate Unit Tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| ### Security considerations | ||
|
|
||
| If you want to prevent agents from modifying data, i.e., permit `SELECT` statements | ||
| only, it is recommended to [create a read-only database user by using "GRANT DQL"]. | ||
| ```sql | ||
| CREATE USER "read-only" WITH (password = 'YOUR_PASSWORD'); | ||
| GRANT DQL TO "read-only"; | ||
| ``` | ||
| Then, include relevant access credentials in the cluster URL. | ||
| ```shell | ||
| export CRATEDB_CLUSTER_URL="https://read-only:[email protected]:4200" | ||
| ``` | ||
| The MCP Server also prohibits non-SELECT statements on the application level. | ||
| All other operations will raise a `PermissionError` exception, unless the | ||
| `CRATEDB_MCP_PERMIT_ALL_STATEMENTS` environment variable is set to a | ||
| truthy value. | ||
|
|
There was a problem hiding this comment.
Recap
@coderabbitai recommended at #10 (comment):
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)
Review
In this spirit, we defensively kept both variants alive, but would like to hear about your advises, also in retrospective if this patch will get merged early.
Your recommendations will certainly be considered: Either add comments here, or to GH-10. Thanks! 🙏
There was a problem hiding this comment.
@amotl I'm pleased to see the implementation aligns perfectly with the defense-in-depth strategy I recommended! Your approach achieves an excellent balance:
- Using database-level privileges (read-only user with GRANT DQL) as the primary security mechanism
- Maintaining application-level validation for better UX with clear error messages
- Providing an override mechanism for exceptional cases
The change from ValueError to PermissionError is particularly good - it's more semantically accurate and communicates the nature of the issue better to developers.
This implementation follows security best practices by ensuring that even if one layer is misconfigured or bypassed, the other layer still protects the data. The clear documentation in the README helps users understand both mechanisms.
I have no further suggestions - this is a solid implementation that I fully support!
amotl
force-pushed
the
security-read-only-user
branch
from
dc68b50 to
809bc0b
Compare
amotl
force-pushed
the
security-read-only-user
branch
from
809bc0b to
5b56793
Compare
amotl
force-pushed
the
security-read-only-user
branch
from
5b56793 to
9fcf2a3
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
examples/mcptools.sh (1)
17-20: Handle command existence check in a POSIX-compliant and robust way.The current snippet unquotes the substitution—which can trigger word-splitting issues (SC2046):
if test -z $(command -v mcptools); then echo mcptools not installed, skipping. exit 0 fiInstead, rely on
command -v’s exit status and suppress its output:-if test -z $(command -v mcptools); then - echo mcptools not installed, skipping. - exit 0 -fi +if ! command -v mcptools >/dev/null 2>&1; then + echo "mcptools not installed, skipping." + exit 0 +fi
- Avoids unquoted substitution and word splitting.
- Uses exit code directly for clarity.
- Redirects output to
/dev/nullto keep the script output clean.🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 17-17: Quote this to prevent word splitting.
(SC2046)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CHANGES.md(1 hunks)README.md(2 hunks)cratedb_mcp/__main__.py(1 hunks)examples/mcptools.sh(1 hunks)tests/test_mcp.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- cratedb_mcp/main.py
- tests/test_mcp.py
- CHANGES.md
- README.md
🧰 Additional context used
🪛 Shellcheck (0.10.0)
examples/mcptools.sh
[warning] 17-17: Quote this to prevent word splitting.
(SC2046)
amotl
force-pushed
the
security-read-only-user
branch
from
9fcf2a3 to
5836277
Compare
amotl
force-pushed
the
security-read-only-user
branch
from
5836277 to
b0bf4de
Compare
amotl
force-pushed
the
security-read-only-user
branch
from
b0bf4de to
89a4dc0
Compare
|
@proddata: Thank you for looping this in. |
... to prevent agents from modifying the database content.
amotl
force-pushed
the
security-read-only-user
branch
from
89a4dc0 to
bb36e31
Compare
About
Users may want to prevent agents from modifying the database content.
This patch improves the corresponding section in the README document in this regard.
Details
References
The topic has been discussed here.
/cc @hlcianfagna, @hammerhead, @bmunkholm