MCP Server HTTP/SSL Toggle, FTS5 Search Fix & Enhanced Transport Support#16
Conversation
…e the
MCP server was already running. The issue was caused by improper cleanup of
handler objects during reinitialization.
Root cause:
- ProxySQL_MCP_Server destructor deletes mysql_tool_handler
- The old code tried to delete handlers again after deleting the server,
causing double-free corruption
The fix properly handles handler lifecycle during reinitialization:
1. Delete Query_Tool_Handler first (server destructor doesn't clean this)
2. Delete the server (which also deletes MySQL_Tool_Handler via destructor)
3. Delete other handlers (config/admin/cache/observe) created by old server
4. Create new MySQL_Tool_Handler with updated configuration
5. Create new Query_Tool_Handler
6. Create new server (recreates all handlers with new endpoints)
This ensures proper cleanup and prevents double-free issues while allowing
runtime reconfiguration of MySQL connection parameters.
* Add full support for both HTTP and HTTPS modes in MCP server via the mcp_use_ssl configuration variable, enabling plain HTTP for development and HTTPS for production with proper certificate validation * Server now automatically restarts when SSL mode or port configuration changes, fixing silent configuration failures where changes appeared to succeed but didn't take effect until manual restart. Features: - Explicit support for HTTP mode (mcp_use_ssl=false) without SSL certificates - Explicit support for HTTPS mode (mcp_use_ssl=true) with certificate validation - Configurable via configure_mcp.sh with --no-ssl or --use-ssl flags - Settable via admin interface: SET mcp-use_ssl=true/false - Automatic restart detection for SSL mode changes (HTTP ↔ HTTPS) - Automatic restart detection for port changes (mcp_port)
- Add render_GET() returning 405 Method Not Allowed - Add render_OPTIONS()
Change id parameter from string to json& to support JSON-RPC 2.0 spec (id can be string, number, or null)
Summary of ChangesHello @rahim-kanji, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly upgrades the MCP server in ProxySQL by introducing a flexible HTTP/HTTPS toggle, enabling automatic server restarts upon configuration changes, and improving compliance with MCP and JSON-RPC 2.0 specifications. A major performance enhancement was implemented for the catalog's full-text search, now leveraging FTS5 with BM25 ranking. These changes aim to improve usability, stability, and performance, supported by an expanded test suite. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds nullable JSON id handling and explicit GET/OPTIONS/DELETE handlers to the MCP JSON‑RPC endpoint; introduces configurable MCP SSL (mcp_use_ssl) with conditional HTTP/HTTPS server creation and full server restart on port/SSL changes; enhances catalog FTS search and updates MCP scripts/tests for SSL and new tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant MCP_Endpoint
participant ToolHandlers
Note right of MCP_Endpoint: rgba(100,149,237,0.5) HTTP handler (GET/OPTIONS/POST/DELETE)
Client->>MCP_Endpoint: OPTIONS /mcp
MCP_Endpoint-->>Client: 200 + CORS, Content-Type: application/json
Client->>MCP_Endpoint: GET /mcp
MCP_Endpoint-->>Client: 405 + Allow: POST
Client->>MCP_Endpoint: POST (JSON‑RPC, id: json|null)
MCP_Endpoint->>ToolHandlers: dispatch method (id passed as json)
ToolHandlers-->>MCP_Endpoint: result or error (id preserved)
MCP_Endpoint-->>Client: 200 + JSON‑RPC response (id echoed if not null)
sequenceDiagram
participant AdminCLI
participant ConfigStore
participant MCP_Server
participant ToolHandlers
Note right of MCP_Server: rgba(144,238,144,0.5) Mode: use_ssl, port
AdminCLI->>ConfigStore: set mcp_use_ssl / mcp_port
ConfigStore-->>MCP_Server: notify config change
MCP_Server->>MCP_Server: detect changed port or use_ssl
MCP_Server->>MCP_Server: stop & delete webserver instance
MCP_Server->>ToolHandlers: destructor cleans handlers
MCP_Server->>MCP_Server: create webserver (HTTP or HTTPS)
MCP_Server->>ToolHandlers: reinitialize handlers and bind endpoints
MCP_Server-->>AdminCLI: "Server restarted" status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
There was a problem hiding this comment.
Code Review
This pull request introduces significant and valuable enhancements, including a flexible HTTP/HTTPS mode for the MCP server, automatic restarts on configuration changes, and a critical performance fix for FTS5 search. The increased compliance with JSON-RPC 2.0 and the MCP specification, along with the expanded test suite, are also great improvements.
However, I have identified a critical double-free bug in the server restart logic and a high-severity SQL injection vulnerability in the new FTS5 search implementation. Additionally, there are opportunities to reduce code duplication and remove a hardcoded path from the configuration file. Please address these issues to ensure the stability and security of the new features.
lib/Admin_FlushVariables.cpp
Outdated
| // 1. Delete Query_Tool_Handler first (server destructor doesn't delete this) | ||
| if (GloMCPH->query_tool_handler) { | ||
| proxy_info("MCP: Deleting old Query Tool Handler\n"); | ||
| delete GloMCPH->query_tool_handler; | ||
| GloMCPH->query_tool_handler = NULL; | ||
| } | ||
|
|
||
| // Create new tool handler with current configuration | ||
| proxy_info("MCP: Reinitializing MySQL Tool Handler with current configuration\n"); | ||
| // 2. Stop and delete the server (server destructor also deletes MySQL_Tool_Handler) | ||
| proxy_info("MCP: Stopping and deleting old server\n"); | ||
| delete GloMCPH->mcp_server; | ||
| GloMCPH->mcp_server = NULL; | ||
| // Note: mysql_tool_handler is already deleted by server destructor and set to NULL | ||
| proxy_info("MCP: Old server deleted\n"); | ||
|
|
||
| // 3. Delete other handlers that were created by old server | ||
| // The server destructor doesn't clean these up, so we need to do it manually | ||
| if (GloMCPH->config_tool_handler) { | ||
| delete GloMCPH->config_tool_handler; | ||
| GloMCPH->config_tool_handler = NULL; | ||
| } | ||
| if (GloMCPH->admin_tool_handler) { | ||
| delete GloMCPH->admin_tool_handler; | ||
| GloMCPH->admin_tool_handler = NULL; | ||
| } | ||
| if (GloMCPH->cache_tool_handler) { | ||
| delete GloMCPH->cache_tool_handler; | ||
| GloMCPH->cache_tool_handler = NULL; | ||
| } | ||
| if (GloMCPH->observe_tool_handler) { | ||
| delete GloMCPH->observe_tool_handler; | ||
| GloMCPH->observe_tool_handler = NULL; | ||
| } |
There was a problem hiding this comment.
This block of code manually deletes several tool handlers (query_tool_handler, config_tool_handler, etc.) before and after deleting GloMCPH->mcp_server. The destructor for ProxySQL_MCP_Server (in lib/ProxySQL_MCP_Server.cpp) has been updated in this same PR to clean up all these handlers. This will lead to the handlers being freed twice, which is a double-free bug.
The comments here are also misleading. For instance, line 1632 states the destructor doesn't clean up handlers, but the updated destructor does.
To fix this, you should remove this manual handler deletion logic. The call to delete GloMCPH->mcp_server; on line 1626 is sufficient to trigger the cleanup of all associated handlers.
| sql << "SELECT c.kind, c.key, c.document, c.tags, c.links " | ||
| << "FROM catalog c " | ||
| << "INNER JOIN catalog_fts f ON c.id = f.rowid " | ||
| << "WHERE catalog_fts MATCH '" << query << "'"; | ||
|
|
||
| // Add kind filter | ||
| if (!kind.empty()) { | ||
| sql << " AND kind = '" << kind << "'"; | ||
| sql << " AND c.kind = '" << kind << "'"; | ||
| } | ||
|
|
||
| // Add tags filter | ||
| if (!tags.empty()) { | ||
| sql << " AND tags LIKE '%" << tags << "%'"; | ||
| sql << " AND c.tags LIKE '%" << tags << "%'"; | ||
| } |
There was a problem hiding this comment.
The query, kind, and tags parameters are directly concatenated into the SQL query string, which creates a SQL injection vulnerability. An attacker could provide a malicious string to manipulate the query. For example, a query string like foo' OR 1=1 -- would alter the logic of the WHERE clause.
To fix this, you should use prepared statements with parameter binding. The SQLite3DB class appears to support this via prepare_v2 and bind functions. If that's not feasible, you must at least escape the input strings to prevent injection. A simple way to do this for SQLite is to replace every single quote (') with two single quotes ('').
src/proxysql.cfg
Outdated
| restart_on_missing_heartbeats=10 | ||
| #set_thread_name=false | ||
| datadir="/var/lib/proxysql" | ||
| datadir="/home/rkanji/claude_code/proxysql_vec/proxysql-vec/src/" |
There was a problem hiding this comment.
This change hardcodes a user-specific absolute path for datadir. Default configuration files should use generic or relative paths to be portable across different environments. Please revert this to the original /var/lib/proxysql or another suitable default.
datadir="/var/lib/proxysql"
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@lib/Admin_FlushVariables.cpp`:
- Around line 1668-1692: The code currently constructs a Query_Tool_Handler
(GloMCPH->query_tool_handler = new Query_Tool_Handler(...)) and then later
constructs a ProxySQL_MCP_Server which also creates a Query_Tool_Handler when
mysql_tool_handler exists, causing a leak/overwrite; fix by removing the manual
creation of GloMCPH->query_tool_handler (or alternatively, add a null-check
guard in the server constructor so it doesn't overwrite an existing handler).
Concretely: either delete the block that new's Query_Tool_Handler and let
ProxySQL_MCP_Server(port, GloMCPH) instantiate it when needed, or modify
ProxySQL_MCP_Server's constructor to only create a Query_Tool_Handler if
GloMCPH->query_tool_handler is NULL, and ensure any allocated instance is
properly deleted before overwrite.
In `@lib/MySQL_Catalog.cpp`:
- Around line 191-219: The search(), list(), and delete_entry() methods
currently build SQL by concatenating user input (query, kind, tags, limit,
offset, key) which is vulnerable to SQL injection; change these to use
db->prepare_v2() with parameter placeholders (e.g., "WHERE catalog_fts MATCH ?
AND c.kind = ? AND c.tags LIKE ? ORDER BY bm25(f) ASC, c.updated_at DESC LIMIT ?
OFFSET ?"), then bind values with proxy_sqlite3_bind_text() for strings (wrap
tags with %...% before binding) and proxy_sqlite3_bind_int() for numeric
limit/offset, execute, and finalize the statement; follow the existing upsert()
and get() patterns for error checking and resource cleanup so no user input is
ever concatenated into the SQL string.
In `@src/proxysql.cfg`:
- Line 11: The sample config currently hardcodes a developer-specific datadir
value for the datadir setting; change the datadir entry (datadir=...) back to a
standard, portable default (e.g., the typical /var/lib/proxysql or another
project-standard placeholder) so the example config is not tied to a user path
and is portable for other environments.
- Line 64: The config sets mcp_use_ssl=false which conflicts with the documented
and runtime default of true; change the mcp_use_ssl setting to true and update
the inline comment to correctly state the secure default (e.g., "Enable/disable
SSL/TLS (default: true for security)"), and scan nearby related MySQL/MCP SSL
settings to ensure consistency with the runtime defaults and secure
configuration expectations.
lib/MySQL_Catalog.cpp
Outdated
| // FTS5 search requires a query | ||
| if (query.empty()) { | ||
| proxy_error("Catalog search requires a query parameter\n"); | ||
| return "[]"; | ||
| } | ||
|
|
||
| std::ostringstream sql; | ||
| sql << "SELECT kind, key, document, tags, links FROM catalog WHERE 1=1"; | ||
| char* error = NULL; | ||
| int cols = 0, affected = 0; | ||
| SQLite3_result* resultset = NULL; | ||
|
|
||
| // FTS5 search with BM25 ranking | ||
| sql << "SELECT c.kind, c.key, c.document, c.tags, c.links " | ||
| << "FROM catalog c " | ||
| << "INNER JOIN catalog_fts f ON c.id = f.rowid " | ||
| << "WHERE catalog_fts MATCH '" << query << "'"; | ||
|
|
||
| // Add kind filter | ||
| if (!kind.empty()) { | ||
| sql << " AND kind = '" << kind << "'"; | ||
| sql << " AND c.kind = '" << kind << "'"; | ||
| } | ||
|
|
||
| // Add tags filter | ||
| if (!tags.empty()) { | ||
| sql << " AND tags LIKE '%" << tags << "%'"; | ||
| sql << " AND c.tags LIKE '%" << tags << "%'"; | ||
| } | ||
|
|
||
| // Add search query | ||
| if (!query.empty()) { | ||
| sql << " AND (key LIKE '%" << query << "%' " | ||
| << "OR document LIKE '%" << query << "%' " | ||
| << "OR tags LIKE '%" << query << "%')"; | ||
| } | ||
|
|
||
| sql << " ORDER BY updated_at DESC LIMIT " << limit << " OFFSET " << offset; | ||
|
|
||
| char* error = NULL; | ||
| int cols = 0, affected = 0; | ||
| SQLite3_result* resultset = NULL; | ||
| // Order by relevance (BM25) and recency | ||
| sql << " ORDER BY bm25(f) ASC, c.updated_at DESC LIMIT " << limit << " OFFSET " << offset; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the file
cd /tmp/repo 2>/dev/null || find . -name "MySQL_Catalog.cpp" -type f | head -5Repository: ProxySQL/proxysql-vec
Length of output: 88
🏁 Script executed:
# Read the file to verify the code context and surrounding function
find . -name "MySQL_Catalog.cpp" -type f -exec wc -l {} \;Repository: ProxySQL/proxysql-vec
Length of output: 92
🏁 Script executed:
# Get the actual code around the specified lines
find . -name "MySQL_Catalog.cpp" -type f -exec sed -n '160,230p' {} +Repository: ProxySQL/proxysql-vec
Length of output: 1806
🏁 Script executed:
# Search for the function containing this code to understand parameter sources
find . -name "MySQL_Catalog.cpp" -type f -exec sed -n '100,200p' {} +Repository: ProxySQL/proxysql-vec
Length of output: 2681
🏁 Script executed:
# Check for any escaping/sanitization functions in the codebase
rg "escape|sanitize|prepared|bind" lib/ --type cpp -B 2 -A 2Repository: ProxySQL/proxysql-vec
Length of output: 50378
🏁 Script executed:
# Look for other SQL query patterns in the same file
rg "ostringstream.*sql|sql <<|MATCH|FTS5" lib/MySQL_Catalog.cpp -B 2 -A 2Repository: ProxySQL/proxysql-vec
Length of output: 1553
Use prepared statements with parameter binding instead of raw SQL concatenation for query, kind, tags, and limit/offset.
The search() method at lines 206, 210, and 215 concatenates user input directly into SQL, creating a SQL injection vulnerability. The same file already demonstrates the correct pattern in upsert() and get() methods using db->prepare_v2() and proxy_sqlite3_bind_text(). Apply the same approach here.
Additionally, the list() and delete_entry() methods have identical vulnerabilities with raw concatenation of kind and key parameters.
🤖 Prompt for AI Agents
In `@lib/MySQL_Catalog.cpp` around lines 191 - 219, The search(), list(), and
delete_entry() methods currently build SQL by concatenating user input (query,
kind, tags, limit, offset, key) which is vulnerable to SQL injection; change
these to use db->prepare_v2() with parameter placeholders (e.g., "WHERE
catalog_fts MATCH ? AND c.kind = ? AND c.tags LIKE ? ORDER BY bm25(f) ASC,
c.updated_at DESC LIMIT ? OFFSET ?"), then bind values with
proxy_sqlite3_bind_text() for strings (wrap tags with %...% before binding) and
proxy_sqlite3_bind_int() for numeric limit/offset, execute, and finalize the
statement; follow the existing upsert() and get() patterns for error checking
and resource cleanup so no user input is ever concatenated into the SQL string.
| { | ||
| mcp_enabled=false | ||
| mcp_port=6071 | ||
| mcp_use_ssl=false # Enable/disable SSL/TLS (default: true for security) |
There was a problem hiding this comment.
Align mcp_use_ssl with secure defaults and documentation.
Line 64 sets mcp_use_ssl=false but the comment says default is true. This also contradicts the runtime default (true) and weakens security for anyone using this example config.
🔐 Proposed fix
-mcp_use_ssl=false # Enable/disable SSL/TLS (default: true for security)
+mcp_use_ssl=true # Enable/disable SSL/TLS (default: true for security)🤖 Prompt for AI Agents
In `@src/proxysql.cfg` at line 64, The config sets mcp_use_ssl=false which
conflicts with the documented and runtime default of true; change the
mcp_use_ssl setting to true and update the inline comment to correctly state the
secure default (e.g., "Enable/disable SSL/TLS (default: true for security)"),
and scan nearby related MySQL/MCP SSL settings to ensure consistency with the
runtime defaults and secure configuration expectations.
The catalog_fts FTS5 virtual table was being created but the search() function was using slow LIKE queries instead of FTS5 MATCH operator.
Changes to lib/MySQL_Catalog.cpp:
- Use FTS5 MATCH with INNER JOIN to catalog_fts when query provided
- Add BM25 relevance ranking (ORDER BY bm25(f) ASC)
- Significant performance improvement: O(log n) vs O(n)
Changes to scripts/mcp/test_catalog.sh:
- Add 8 new FTS5-specific tests (CAT013-CAT020):
- Multi-term search (AND logic)
- Phrase search with quotes
- Boolean operators (OR, NOT)
- Prefix search with wildcards
- Kind and tags filter combinations
- Relevance ranking verification
- Add SSL/HTTP support with auto-detection
- New options: --ssl, --no-ssl, MCP_USE_SSL env var
- Fix endpoint path: /query -> /mcp/querywq
11143d2 to
f7397f6
Compare
…ded path
This commit addresses three issues identified by code review:
1. CRITICAL: Fix double-free bug in MCP server restart logic
- Remove manual handler deletions in Admin_FlushVariables.cpp
- ProxySQL_MCP_Server destructor already properly cleans up all handlers
- Previously caused crashes when toggling SSL mode or changing port
- Simplified restart: delete server (destructor cleanup) → create new server
- Verified with 10+ rapid SSL toggles without crashes
2. HIGH: Fix SQL injection vulnerability in catalog search
- Rewrite MySQL_Catalog::search() to use prepared statements
- Use parameter binding (proxy_sqlite3_bind_text/bind_int) for user input
- Escape single quotes in FTS5 MATCH clause (doesn't support parameters)
- Tested against multiple injection attempts (single quote, backslash,
comments, UNION SELECT, kind/tags parameter injection)
- All 21 catalog tests still pass with new implementation
3. MEDIUM: Fix hardcoded user-specific path in config
- Revert datadir from user-specific absolute path to /var/lib/proxysql
- Ensures portability across different environments
Testing:
- SSL toggle: 7 tests passed (HTTP↔HTTPS, port changes, stress test)
- SQL injection: 10 tests passed (various injection attempts blocked)
- Catalog functionality: 21 tests passed (FTS5, BM25 ranking, etc.)
- Total: 38 tests passed, 0 failed
Fixes issues identified in GitHub PR sysown#16 review.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@lib/Admin_FlushVariables.cpp`:
- Around line 1613-1636: The reinitialization path creates a new
ProxySQL_MCP_Server unconditionally and can start HTTPS without validating
certificates; before allocating GloMCPH->mcp_server in the else branch, check
GloMCPH->variables.mcp_use_ssl and perform the same certificate
loading/validation used in the server-NULL branch (the SSL cert checks around
the earlier create/start logic), log an error and skip creating/starting the
server if certs are missing or invalid, and ensure you do not call start() when
validation fails (update the else-branch around ProxySQL_MCP_Server construction
where GloMCPH->mcp_server is set).
In `@lib/MySQL_Catalog.cpp`:
- Around line 271-277: The JSON string fields are written without escaping in
the json-building expression in MySQL_Catalog.cpp (the stream that writes
"kind","key","tags","links"), so implement a helper like escape_json(const char*
str) that returns a std::string with quotes, backslashes, control chars
(newline, carriage return, tab) and other <0x20 bytes escaped (use \", \\, \n,
\r, \t, and \uXXXX for control bytes), then replace direct uses of kind_val,
key_val, tags_val, and links_val in the json << ... expression with
escape_json(kind_val), escape_json(key_val), escape_json(tags_val), and
escape_json(links_val) respectively so all string values are safe and JSON
remains well-formed.
♻️ Duplicate comments (1)
lib/MySQL_Catalog.cpp (1)
298-302: Note: SQL injection vulnerability remains in unchanged methods.While the
search()method now uses prepared statements forkindandtags, thelist()method (here) andremove()method (line 387-391) still concatenate parameters directly into SQL. Consider addressing these in a follow-up.
🧹 Nitpick comments (3)
lib/MySQL_Catalog.cpp (2)
197-211: FTS5 query syntax characters are not escaped.The
escape_sqllambda only escapes single quotes for the SQL string literal, but FTS5 MATCH has its own query syntax with special operators (*,",-,+,^,AND,OR,NOT,NEAR, parentheses). User input likefoo*orfoo" OR barcould produce unexpected search behavior or FTS5 parse errors.Consider stripping or escaping FTS5 metacharacters, or wrapping the query in double quotes to treat it as a literal phrase:
escaped_query = "\"" + escape_sql(query) + "\"";Alternatively, filter to alphanumeric and whitespace only for stricter sanitization.
220-220: Redundant initialization.
param_countis initialized to1here but immediately reassigned on line 244. Remove this dead initialization.♻️ Suggested fix
- int param_count = 1; // Track parameter binding position - // Add kind filter with parameter placeholder if (!kind.empty()) { sql << " AND c.kind = ?"; }lib/Admin_FlushVariables.cpp (1)
1577-1647: Consider extracting MCP server lifecycle management into a helper function.The server startup, restart, and stop logic is duplicated between
flush_mcp_variables___database_to_runtime(lines 1371-1478) andflush_mcp_variables___runtime_to_database(lines 1577-1647). This duplication contributed to the SSL validation being missed in one path. A helper function likemanage_mcp_server_state(bool enabled)would consolidate this logic and reduce maintenance burden.
| } else { | ||
| // Server is already running - check if MySQL configuration changed | ||
| // and reinitialize the tool handler if needed | ||
| proxy_info("MCP: Server already running, checking MySQL tool handler reinitialization\n"); | ||
| if (GloMCPH->mysql_tool_handler) { | ||
| // Delete old handler | ||
| delete GloMCPH->mysql_tool_handler; | ||
| GloMCPH->mysql_tool_handler = NULL; | ||
| } | ||
| // Server is already running - need to stop, delete server, and recreate everything | ||
| proxy_info("MCP: Server already running, reinitializing\n"); | ||
|
|
||
| // Create new tool handler with current configuration | ||
| proxy_info("MCP: Reinitializing MySQL Tool Handler with current configuration\n"); | ||
| GloMCPH->mysql_tool_handler = new MySQL_Tool_Handler( | ||
| GloMCPH->variables.mcp_mysql_hosts ? GloMCPH->variables.mcp_mysql_hosts : "", | ||
| GloMCPH->variables.mcp_mysql_ports ? GloMCPH->variables.mcp_mysql_ports : "", | ||
| GloMCPH->variables.mcp_mysql_user ? GloMCPH->variables.mcp_mysql_user : "", | ||
| GloMCPH->variables.mcp_mysql_password ? GloMCPH->variables.mcp_mysql_password : "", | ||
| GloMCPH->variables.mcp_mysql_schema ? GloMCPH->variables.mcp_mysql_schema : "", | ||
| GloMCPH->variables.mcp_catalog_path ? GloMCPH->variables.mcp_catalog_path : "" | ||
| ); | ||
|
|
||
| if (GloMCPH->mysql_tool_handler->init() != 0) { | ||
| proxy_error("MCP: Failed to reinitialize MySQL Tool Handler\n"); | ||
| delete GloMCPH->mysql_tool_handler; | ||
| GloMCPH->mysql_tool_handler = NULL; | ||
| // Delete the old server - its destructor will clean up all handlers | ||
| // (mysql_tool_handler, config_tool_handler, query_tool_handler, | ||
| // admin_tool_handler, cache_tool_handler, observe_tool_handler) | ||
| proxy_info("MCP: Stopping and deleting old server\n"); | ||
| delete GloMCPH->mcp_server; | ||
| GloMCPH->mcp_server = NULL; | ||
| // All handlers are now deleted and set to NULL by the destructor | ||
| proxy_info("MCP: Old server deleted\n"); | ||
|
|
||
| // Create and start new server with current configuration | ||
| // The server constructor will recreate all handlers with updated settings | ||
| proxy_info("MCP: Creating and starting new server\n"); | ||
| int port = GloMCPH->variables.mcp_port; | ||
| GloMCPH->mcp_server = new ProxySQL_MCP_Server(port, GloMCPH); | ||
| if (GloMCPH->mcp_server) { | ||
| GloMCPH->mcp_server->start(); | ||
| proxy_info("MCP: New server created and started successfully\n"); | ||
| } else { | ||
| proxy_info("MCP: MySQL Tool Handler reinitialized successfully\n"); | ||
| proxy_error("MCP: Failed to create new server instance\n"); | ||
| } |
There was a problem hiding this comment.
Missing SSL certificate validation when reinitializing server.
When the server is already running and gets reinitialized, this code path unconditionally creates a new server without validating SSL certificates. Compare this to:
- Lines 1584-1612 (server NULL case in same function): properly validates SSL certs
- Lines 1440-1465 (
database_to_runtimerestart): properly validates SSL certs
If mcp_use_ssl=true but certificates are not loaded, this path will attempt to start an HTTPS server without the required certificates.
🐛 Proposed fix to add SSL certificate validation
} else {
// Server is already running - need to stop, delete server, and recreate everything
proxy_info("MCP: Server already running, reinitializing\n");
// Delete the old server - its destructor will clean up all handlers
proxy_info("MCP: Stopping and deleting old server\n");
delete GloMCPH->mcp_server;
GloMCPH->mcp_server = NULL;
proxy_info("MCP: Old server deleted\n");
// Create and start new server with current configuration
- proxy_info("MCP: Creating and starting new server\n");
int port = GloMCPH->variables.mcp_port;
- GloMCPH->mcp_server = new ProxySQL_MCP_Server(port, GloMCPH);
- if (GloMCPH->mcp_server) {
- GloMCPH->mcp_server->start();
- proxy_info("MCP: New server created and started successfully\n");
+ if (GloMCPH->variables.mcp_use_ssl) {
+ if (!GloVars.global.ssl_key_pem_mem || !GloVars.global.ssl_cert_pem_mem) {
+ proxy_error("MCP: Cannot start server in SSL mode - SSL certificates not loaded. "
+ "Please configure ssl_key_fp and ssl_cert_fp, or set mcp_use_ssl=false.\n");
+ } else {
+ proxy_info("MCP: Creating and starting new HTTPS server on port %d\n", port);
+ GloMCPH->mcp_server = new ProxySQL_MCP_Server(port, GloMCPH);
+ if (GloMCPH->mcp_server) {
+ GloMCPH->mcp_server->start();
+ proxy_info("MCP: New server created and started successfully\n");
+ } else {
+ proxy_error("MCP: Failed to create new server instance\n");
+ }
+ }
} else {
- proxy_error("MCP: Failed to create new server instance\n");
+ proxy_info("MCP: Creating and starting new HTTP server on port %d (unencrypted)\n", port);
+ GloMCPH->mcp_server = new ProxySQL_MCP_Server(port, GloMCPH);
+ if (GloMCPH->mcp_server) {
+ GloMCPH->mcp_server->start();
+ proxy_info("MCP: New server created and started successfully\n");
+ } else {
+ proxy_error("MCP: Failed to create new server instance\n");
+ }
}
}🤖 Prompt for AI Agents
In `@lib/Admin_FlushVariables.cpp` around lines 1613 - 1636, The reinitialization
path creates a new ProxySQL_MCP_Server unconditionally and can start HTTPS
without validating certificates; before allocating GloMCPH->mcp_server in the
else branch, check GloMCPH->variables.mcp_use_ssl and perform the same
certificate loading/validation used in the server-NULL branch (the SSL cert
checks around the earlier create/start logic), log an error and skip
creating/starting the server if certs are missing or invalid, and ensure you do
not call start() when validation fails (update the else-branch around
ProxySQL_MCP_Server construction where GloMCPH->mcp_server is set).
- Version 2024-11-05 only supports HTTP_SSE as transport. - ProxySQL's MCP implemenation aligns more with the StreamableHTTP transport specified in version 2025-06-18. - Support for SSE in StreamableHTTP transport is optional. Signed-off-by: Wazir Ahmed <wazir@proxysql.com>
- Fix incorrect method name for notification - Handle all notification messages in a generic way - Respond with HTTP 202 Accepted (no response body) Signed-off-by: Wazir Ahmed <wazir@proxysql.com>
Issue ----- - ProxySQL only supports the `tools` feature of the MCP protocol and does not support features such as `prompts` and `resources`. - Although ProxySQL expresses this in its `initialize` response, (server capabilities list contains only the `tools` object), clients such as Warp Terminal ignore it and continue to send requests for methods such `prompts/list` and `resources/list`. - Any response other than `HTTP 200 OK` is treated as an error and client fails to initialize. Fix --- - Handle prompt and resource list requests by returning an empty array. Signed-off-by: Wazir Ahmed <wazir@proxysql.com>
- Respond with 405 Method Not Allowed, when clients send DELETE request for session termination. Signed-off-by: Wazir Ahmed <wazir@proxysql.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@lib/MCP_Endpoint.cpp`:
- Line 117: The Allow header values are inconsistent: update the
response->with_header("Allow", ...) calls so render_GET and render_DELETE
advertise the same allowed methods; locate the calls in render_GET and
render_DELETE and change the header value to the common set (e.g., "POST,
OPTIONS") so both handlers return identical Allow listings.
🧹 Nitpick comments (1)
lib/MCP_Endpoint.cpp (1)
126-148: Consider security implications of wildcard CORS origin.The
Access-Control-Allow-Origin: "*"allows any origin to make requests to this endpoint. While this may be intentional for broad MCP client compatibility, consider whether a configurable or more restrictive origin policy is appropriate for your deployment scenarios.Also, the static analysis hint about null pointer at line 138 is a false positive—C++
newthrowsstd::bad_allocon allocation failure rather than returning null.
| "", | ||
| http::http_utils::http_method_not_allowed // 405 | ||
| )); | ||
| response->with_header("Allow", "POST"); // Tell client what IS allowed |
There was a problem hiding this comment.
Inconsistent Allow header values between GET and DELETE handlers.
render_GET returns Allow: POST while render_DELETE returns Allow: POST, OPTIONS. For consistency and accuracy, both should advertise the same set of allowed methods.
Proposed fix
auto response = std::shared_ptr<http_response>(new string_response(
"",
http::http_utils::http_method_not_allowed // 405
));
- response->with_header("Allow", "POST"); // Tell client what IS allowed
+ response->with_header("Allow", "POST, OPTIONS"); // Tell client what IS allowedAlso applies to: 162-162
🤖 Prompt for AI Agents
In `@lib/MCP_Endpoint.cpp` at line 117, The Allow header values are inconsistent:
update the response->with_header("Allow", ...) calls so render_GET and
render_DELETE advertise the same allowed methods; locate the calls in render_GET
and render_DELETE and change the header value to the common set (e.g., "POST,
OPTIONS") so both handlers return identical Allow listings.
There was a problem hiding this comment.
Pull request overview
This PR adds flexible HTTP/HTTPS mode support to the MCP server, implements automatic server restart on configuration changes, improves JSON-RPC 2.0 compliance, fixes FTS5 full-text search implementation, and significantly enhances test coverage. The changes enable development-friendly HTTP mode while maintaining production-ready HTTPS as the default, with comprehensive protocol compliance improvements.
Changes:
- Added
mcp_use_sslconfiguration variable to toggle between HTTP and HTTPS modes - Implemented automatic server restart when port or SSL mode changes
- Fixed FTS5 search to use MATCH operator with BM25 ranking instead of LIKE queries
- Added GET/OPTIONS/DELETE handlers for proper MCP HTTP transport compliance
- Added 9 new FTS5-specific tests with SSL/HTTP auto-detection
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/proxysql.cfg | Added mcp_use_ssl configuration variable with example |
| scripts/mcp/test_catalog.sh | Added SSL auto-detection and 9 new FTS5 tests (CAT013-CAT021) |
| scripts/mcp/configure_mcp.sh | Added --use-ssl/--no-ssl flags and SSL-aware testing |
| lib/ProxySQL_MCP_Server.cpp | Implemented conditional HTTP/HTTPS server creation based on mcp_use_ssl |
| lib/MySQL_Catalog.cpp | Replaced LIKE queries with FTS5 MATCH and BM25 ranking |
| lib/MCP_Endpoint.cpp | Added GET/OPTIONS/DELETE handlers, fixed JSON-RPC ID type handling, added prompts/list and resources/list |
| lib/Admin_FlushVariables.cpp | Added automatic restart detection for port and SSL mode changes |
| lib/MCP_Thread.cpp | Added mcp_use_ssl variable getter/setter with true/false/1/0 support |
| include/ProxySQL_MCP_Server.hpp | Added get_port() and is_using_ssl() methods, updated documentation |
| include/MCP_Thread.h | Added mcp_use_ssl variable declaration |
| include/MCP_Endpoint.h | Updated create_jsonrpc_response/error signatures to accept json ID type, added new method declarations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| )); | ||
| response->with_header("Content-Type", "application/json"); | ||
| return response; | ||
| return response; |
There was a problem hiding this comment.
Inconsistent indentation: the return statement has an extra tab compared to the lines above it. Should align with the previous lines at the same indentation level.
| return response; | |
| return response; |
| { | ||
| mcp_enabled=false | ||
| mcp_port=6071 | ||
| mcp_use_ssl=false # Enable/disable SSL/TLS (default: true for security) |
There was a problem hiding this comment.
The default value for mcp_use_ssl is set to false, but the comment states "default: true for security". This contradicts the code in lib/MCP_Thread.cpp line 46 which sets the default to true, and also contradicts the security-first principle mentioned throughout the PR description.
Setting the default to false in the configuration file is dangerous as it means users who copy this example config will unknowingly run the MCP server in unencrypted HTTP mode. The value should be set to true to match the code default and the security intent.
| mcp_use_ssl=false # Enable/disable SSL/TLS (default: true for security) | |
| mcp_use_ssl=true # Enable/disable SSL/TLS (default: true for security) |
| // Helper lambda to escape single quotes for SQLite SQL literals | ||
| auto escape_sql = [](const std::string& str) -> std::string { | ||
| std::string result; | ||
| result.reserve(str.length() * 2); // Reserve space for potential escaping | ||
| for (char c : str) { | ||
| if (c == '\'') { | ||
| result += '\''; // Escape single quote by doubling it | ||
| } | ||
| result += c; | ||
| } | ||
| return result; | ||
| }; | ||
|
|
||
| // Escape query for use in FTS5 MATCH (MATCH doesn't support parameter binding) | ||
| std::string escaped_query = escape_sql(query); | ||
|
|
||
| // Build SQL query with placeholders for parameters | ||
| std::ostringstream sql; | ||
| sql << "SELECT kind, key, document, tags, links FROM catalog WHERE 1=1"; | ||
| sql << "SELECT c.kind, c.key, c.document, c.tags, c.links " | ||
| << "FROM catalog c " | ||
| << "INNER JOIN catalog_fts f ON c.id = f.rowid " | ||
| << "WHERE catalog_fts MATCH '" << escaped_query << "'"; |
There was a problem hiding this comment.
The custom SQL escaping implementation for FTS5 MATCH queries is insufficient and potentially vulnerable to FTS5 injection attacks. While doubling single quotes prevents basic SQL injection, FTS5 has its own query syntax with special operators (OR, AND, NOT, *, quotes, parentheses, etc.) that can be exploited.
For example, a malicious query like OR 1=1 or * could alter the search semantics. The comment states "MATCH doesn't support parameter binding," but this is incorrect. SQLite FTS5 MATCH does support parameter binding using the following pattern:
WHERE catalog_fts MATCH ?
Then bind the query parameter normally. This is the recommended and secure approach per SQLite documentation.
| result["protocolVersion"] = "2025-06-18"; | ||
| result["capabilities"]["tools"] = json::object(); // Explicitly declare tools support | ||
| result["serverInfo"] = { | ||
| {"name", "proxysql-mcp-mcp-mysql-tools"}, |
There was a problem hiding this comment.
There's a typo in the server name. It should be "proxysql-mcp-mysql-tools" but it currently says "proxysql-mcp-mcp-mysql-tools" with a duplicated "mcp".
| {"name", "proxysql-mcp-mcp-mysql-tools"}, | |
| {"name", "proxysql-mcp-mysql-tools"}, |
| } else { | ||
| // Server is already running - check if MySQL configuration changed | ||
| // and reinitialize the tool handler if needed | ||
| proxy_info("MCP: Server already running, checking MySQL tool handler reinitialization\n"); | ||
| if (GloMCPH->mysql_tool_handler) { | ||
| // Delete old handler | ||
| delete GloMCPH->mysql_tool_handler; | ||
| GloMCPH->mysql_tool_handler = NULL; | ||
| } | ||
| // Server is already running - need to stop, delete server, and recreate everything | ||
| proxy_info("MCP: Server already running, reinitializing\n"); | ||
|
|
||
| // Create new tool handler with current configuration | ||
| proxy_info("MCP: Reinitializing MySQL Tool Handler with current configuration\n"); | ||
| GloMCPH->mysql_tool_handler = new MySQL_Tool_Handler( | ||
| GloMCPH->variables.mcp_mysql_hosts ? GloMCPH->variables.mcp_mysql_hosts : "", | ||
| GloMCPH->variables.mcp_mysql_ports ? GloMCPH->variables.mcp_mysql_ports : "", | ||
| GloMCPH->variables.mcp_mysql_user ? GloMCPH->variables.mcp_mysql_user : "", | ||
| GloMCPH->variables.mcp_mysql_password ? GloMCPH->variables.mcp_mysql_password : "", | ||
| GloMCPH->variables.mcp_mysql_schema ? GloMCPH->variables.mcp_mysql_schema : "", | ||
| GloMCPH->variables.mcp_catalog_path ? GloMCPH->variables.mcp_catalog_path : "" | ||
| ); | ||
|
|
||
| if (GloMCPH->mysql_tool_handler->init() != 0) { | ||
| proxy_error("MCP: Failed to reinitialize MySQL Tool Handler\n"); | ||
| delete GloMCPH->mysql_tool_handler; | ||
| GloMCPH->mysql_tool_handler = NULL; | ||
| // Delete the old server - its destructor will clean up all handlers | ||
| // (mysql_tool_handler, config_tool_handler, query_tool_handler, | ||
| // admin_tool_handler, cache_tool_handler, observe_tool_handler) | ||
| proxy_info("MCP: Stopping and deleting old server\n"); | ||
| delete GloMCPH->mcp_server; | ||
| GloMCPH->mcp_server = NULL; | ||
| // All handlers are now deleted and set to NULL by the destructor | ||
| proxy_info("MCP: Old server deleted\n"); | ||
|
|
||
| // Create and start new server with current configuration | ||
| // The server constructor will recreate all handlers with updated settings | ||
| proxy_info("MCP: Creating and starting new server\n"); | ||
| int port = GloMCPH->variables.mcp_port; | ||
| GloMCPH->mcp_server = new ProxySQL_MCP_Server(port, GloMCPH); | ||
| if (GloMCPH->mcp_server) { | ||
| GloMCPH->mcp_server->start(); | ||
| proxy_info("MCP: New server created and started successfully\n"); | ||
| } else { | ||
| proxy_info("MCP: MySQL Tool Handler reinitialized successfully\n"); | ||
| proxy_error("MCP: Failed to create new server instance\n"); | ||
| } |
There was a problem hiding this comment.
The runtime_to_database flush function unconditionally recreates the MCP server when it's already running, without checking if SSL certificates are available when SSL mode is enabled. This differs from the database_to_runtime function (lines 1418-1483) which properly validates SSL certificates before restarting.
The server recreation at lines 1644-1645 should be wrapped in SSL certificate validation logic similar to the database_to_runtime flow, to prevent the server from failing to start after deletion if SSL certificates aren't available.
| response=$(curl -s -X POST "${proto}://${PROXYSQL_ADMIN_HOST}:${MCP_PORT}/mcp/config" \ | ||
| -H "Content-Type: application/json" \ | ||
| -d '{"jsonrpc":"2.0","method":"ping","id":1}' 2>/dev/null || echo "") |
There was a problem hiding this comment.
The curl command for testing the MCP server in HTTPS mode is missing the -k flag to allow self-signed certificates. This will cause the test to fail when using self-signed certificates (which is the typical development setup). The test should use -k flag when proto is "https", similar to how it's done in test_catalog.sh at line 82.
| if (!ssl_key || !ssl_cert) { | ||
| proxy_error("Cannot start MCP server in SSL mode: SSL certificates not loaded. " | ||
| "Please configure ssl_key_fp and ssl_cert_fp, or set mcp_use_ssl=false.\n"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
When SSL certificates are not available in SSL mode, the constructor returns early (line 55) but continues to initialize tool handlers (lines 77-189) even though the webserver (ws) was never created. This results in an object in an inconsistent state where tool handlers exist but the webserver doesn't.
While start() and stop() methods safely check for null ws, this wastes resources by creating handlers for a server that cannot start. Consider returning early before initializing any tool handlers, or wrapping all handler initialization in a check that ws was successfully created.
Overview
This PR introduces significant enhancements to the MCP (Model Context Protocol) server implementation in ProxySQL, including flexible HTTP/HTTPS mode support, automatic server restart on configuration changes, improved JSON-RPC 2.0 compliance, proper FTS5 full-text search implementation, and enhanced test coverage
Summary of Changes
1. HTTP/HTTPS Mode Toggle Support
New Configuration Variable:
mcp_use_sslPreviously, the MCP server only supported HTTPS mode, requiring valid SSL certificates to start. This added friction for development environments and made it difficult to test without proper certificate infrastructure.
What Changed:
mcp_use_sslconfiguration variable (default:truefor security)Configuration:
Via
configure_mcp.sh:2. Automatic Server Restart on Configuration Changes
Previously, changing
mcp_portor SSL settings would appear to succeed but silently fail to take effect until a manual ProxySQL restart.What Changed:
mcp_portand restarts automaticallymcp_use_ssland restarts automaticallyFiles Modified:
lib/Admin_FlushVariables.cpp- Added restart detection logic inflush_mcp_variables___database_to_runtime()andflush_mcp_variables___runtime_to_database()3. MCP HTTP Transport Compliance
Added GET and OPTIONS Handlers
The MCP specification defines requirements for HTTP transport beyond just POST requests. This implementation now complies with:
What Changed:
MCP_JSONRPC_Resource::render_GET()- Returns 405 withAllow: POSTheaderMCP_JSONRPC_Resource::render_OPTIONS()- CORS preflight support with proper headersFiles Modified:
include/MCP_Endpoint.h- New method declarationslib/MCP_Endpoint.cpp- Implementation with proper HTTP status codes4. JSON-RPC 2.0 Specification Compliance
Fixed ID Parameter Type
The JSON-RPC 2.0 specification defines the
idfield as:Previously, the implementation only accepted string IDs, causing protocol violations for numeric or null IDs.
What Changed:
create_jsonrpc_response()andcreate_jsonrpc_error()to acceptconst json&instead ofconst std::string&Files Modified:
include/MCP_Endpoint.h- Updated function signatureslib/MCP_Endpoint.cpp- Updated implementation5. MCP notifications/initialized Support
The MCP specification defines
notifications/initializedas a special notification sent by clients after the initialization handshake.What Changed:
notifications/initializedmethod{}per MCP spec6. FTS5 Full-Text Search Fix with BM25 Ranking
Critical Performance Fix
The catalog search was creating an FTS5 virtual table (
catalog_fts) but thesearch()function was still using slowLIKEqueries instead of FTS5'sMATCHoperator. This defeated the purpose of having an FTS index.What Changed:
Before:
After:
Benefits:
MATCHoperatorFiles Modified:
lib/MySQL_Catalog.cpp- Updatedsearch()function7. Enhanced Test Suite
Catalog Test Suite Expansion
Added 9 new tests specifically for FTS5 functionality:
SSL/HTTP Auto-Detection:
--ssland--no-sslflags for explicit mode selectionMCP_USE_SSLenvironment variable supportFiles Modified:
scripts/mcp/test_catalog.sh- 409 lines of new test codescripts/mcp/configure_mcp.sh- SSL mode configuration supportFiles Changed
lib/Admin_FlushVariables.cpplib/ProxySQL_MCP_Server.cpplib/MCP_Endpoint.cppscripts/mcp/test_catalog.shscripts/mcp/configure_mcp.shlib/MySQL_Catalog.cpplib/MCP_Thread.cppmcp_use_sslvariable handlinginclude/MCP_Endpoint.hinclude/ProxySQL_MCP_Server.hppinclude/MCP_Thread.hmcp_use_sslvariablesrc/proxysql.cfgConfiguration Examples
Development Mode (HTTP)
Production Mode (HTTPS)
Port Change (Auto-Restart)
Testing
Run All Catalog Tests (Auto-Detect SSL)
Force HTTP Mode
Force HTTPS Mode
Verbose Output
Backwards Compatibility
mcp_use_ssl=truemaintains security defaultSecurity Considerations
mcp_use_ssl=trueis the default for securityRelated Documentation