Skip to content

fix(security): add SQL injection prevention to ExplainPlanTool#161

Open
RajatRaiD11 wants to merge 2 commits intocrystaldba:mainfrom
RajatRaiD11:fix/explain-sql-injection-prevention
Open

fix(security): add SQL injection prevention to ExplainPlanTool#161
RajatRaiD11 wants to merge 2 commits intocrystaldba:mainfrom
RajatRaiD11:fix/explain-sql-injection-prevention

Conversation

@RajatRaiD11
Copy link
Copy Markdown

Summary

The explain_query tool is annotated with readOnlyHint=True, but in UNRESTRICTED mode (default), user-supplied SQL is concatenated via f-string into EXPLAIN queries without validation. This allows:

  1. Multi-statement SQL injection — e.g., SELECT 1; DROP TABLE users passes through f"EXPLAIN (FORMAT JSON) {sql_input}" and reaches cursor.execute() which supports multi-statement execution via cursor.nextset()
    1. EXPLAIN ANALYZE DML executionEXPLAIN ANALYZE DELETE FROM users actually executes the DELETE, but the tool doesn't restrict this

What this PR does

Adds _validate_explain_input() static method using pglast (already a project dependency) to:

  • Parse input SQL and reject multi-statement injection attempts
    • Restrict EXPLAIN ANALYZE to SELECT-only (since it executes the query)
    • Validate syntax before f-string concatenation
      Integrates validation into explain(), explain_with_hypothetical_indexes(), and generate_explain_plan_with_hypothetical_indexes().

What the codebase already gets right

  • SafeSqlDriver provides excellent pglast-based validation in RESTRICTED mode
    • psycopg.sql.Literal() is used correctly for IndexDefinition (not exploitable)
    • Read-only transaction enforcement in RESTRICTED mode is solid
      This fix extends the same validation approach to UNRESTRICTED mode for the EXPLAIN tool specifically.

Files changed

  • src/postgres_mcp/explain/explain_plan.py — adds _validate_explain_input() and integrates it (57 additions, 0 deletions)
    • tests/unit/explain/test_explain_input_validation.py — 22 test cases (17 unit + 5 integration)

Test coverage

  • Valid queries (SELECT, JOIN, CTE, subquery, INSERT/UPDATE/DELETE without ANALYZE, bind params)
    • Multi-statement injection rejection (semicolons, comments, COPY exfiltration)
    • Empty/invalid input handling
    • EXPLAIN ANALYZE DML restrictions (blocks DELETE, UPDATE, INSERT; allows SELECT)
    • Integration tests verifying explain(), explain_analyze(), and explain_with_hypothetical_indexes() block injection and never send malicious SQL to the database

Add _validate_explain_input() using pglast to parse and validate SQL before f-string concatenation. Prevents multi-statement injection and restricts EXPLAIN ANALYZE to SELECT-only.
17 unit tests + 5 integration tests covering valid queries, multi-statement injection rejection, empty/invalid input, EXPLAIN ANALYZE DML restrictions, and COPY exfiltration prevention.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant