From fb4e9a46ab19d9673b0779f75fd78b40962522bf Mon Sep 17 00:00:00 2001 From: Alkin Tezuysal Date: Fri, 30 Oct 2026 11:14:54 +0000 Subject: [PATCH 1/6] feat(mcp): guard time limit and refusal log (completes RFC 0001 Phase 2) - mcp.max_execution_time_ms: an MCP session's max_execution_time is set at connect; raising it is refused (1290): SET to 0, above the limit or to a non-literal, and MAX_EXECUTION_TIME(n) / SET_VAR(max_execution_time = n) hints. Lowering is allowed. SET text is read in both quoting modes. - mcp.log_denials (ON): refusals go to the error log as [MY-011071] [mcp_guard] with the setting, connection id and statement start (256 bytes) or table, at most 10 lines a minute plus a count of the rest. - Tests: 78 checks on 8.4.11, 9.7.2, 26.10.0 (a 5 s SELECT stopped by a 1 s limit; exactly 10 log lines for 16 refusals on a fresh server); ~20 new policy unit cases. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 4 + docs/MCP_COMPONENTS.md | 27 ++++- scripts/test-mcp-guard.sh | 28 +++++ src/mcp/guard/component_mcp_guard.cc | 152 ++++++++++++++++++++++--- src/mcp/guard/mcp_guard_policy.h | 78 +++++++++++++ src/mcp/guard/test_mcp_guard_policy.cc | 38 +++++++ 6 files changed, 311 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c443d78..42c0904 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] ### Added +- **`component_mcp_guard` time limit and refusal log** (Phase 2 of RFC 0001, completed). + - **`mcp.max_execution_time_ms`:** sets an MCP session's `max_execution_time` at connect. Attempts to raise it are refused with error 1290: via `SET`, or via the `MAX_EXECUTION_TIME(n)` or `SET_VAR(max_execution_time=n)` hints. Lowering it is allowed. + - **`mcp.log_denials` (default ON):** writes each refusal to the error log as `[MY-011071] [mcp_guard]`, at most 10 lines a minute. + - **Tests:** `scripts/test-mcp-guard.sh` now has 78 checks, including a 5 s `SELECT` stopped by a 1 s limit and the log rate limit on a fresh server, passing on MySQL 8.4.11, 9.7.2 and 26.10.0. - **`component_mcp_guard` enforcement** (Phase 2 of [RFC 0001](https://github.com/askdba/mysql-mcp-server/blob/main/docs/rfc/0001-mysql-component-architecture.md)). - **MCP sessions:** sessions of accounts holding `MCP_USER` (and not `MCP_ADMIN`) are checked against `mcp.*` settings inside `mysqld`. The checks run on the server's query, table-access and connection events. - **`mcp.read_only` (default ON):** only reads, even with write grants. It uses an allowlist of `sql_command` classes and also refuses `SELECT … INTO OUTFILE`, locking reads, `SET GLOBAL` and `CALL` outside `mysql_mcp`. diff --git a/docs/MCP_COMPONENTS.md b/docs/MCP_COMPONENTS.md index 116c8c7..ddf941d 100644 --- a/docs/MCP_COMPONENTS.md +++ b/docs/MCP_COMPONENTS.md @@ -4,7 +4,7 @@ Server-side components for [mysql-mcp-server](https://github.com/askdba/mysql-mc | Component | Status | Servers | |---|---|---| -| `component_mcp_guard` | Phase 2: identity, read-only and schema enforcement, status variables | MySQL 8.4, 9.7, 26.10 | +| `component_mcp_guard` | Phase 2: identity, read-only and schema enforcement, time limit, error log, status variables | MySQL 8.4, 9.7, 26.10 | | `mysql_mcp` SQL tool pack ([`sql/mcp/`](https://github.com/askdba/myvector/tree/main/sql/mcp)) | 1.0.0 | MySQL 8.0 to 26.10, MariaDB 11.4, 12.3 | ## component_mcp_guard @@ -39,6 +39,8 @@ A session is an **MCP session** when its account holds `MCP_USER` and not `MCP_A | `mcp.read_only` | `ON` | Only reads, whatever the account's grants (below). | | `mcp.deny_system_schemas` | `ON` | No table access in `mysql` and `sys`. `information_schema` and `performance_schema` stay readable (the tools use them; grants govern them). | | `mcp.allowed_schemas` | empty | Comma-separated schemas whose tables MCP sessions may access; blank means all. A value with no names in it, such as `,`, allows none. Names are compared exactly, or case-insensitively when `lower_case_table_names` is 1 or 2. The system schemas above are governed by `mcp.deny_system_schemas`, and `mysql_mcp` is always allowed. | +| `mcp.max_execution_time_ms` | 0 | Time limit for their `SELECT` statements, which they cannot raise; 0 means none (below). | +| `mcp.log_denials` | `ON` | Write refusals to the error log, at most 10 lines a minute (below). | `SET GLOBAL` and `SET PERSIST` of `mcp.*` need `MCP_ADMIN` on top of `SYSTEM_VARIABLES_ADMIN`. Persisted values load at startup as usual. Values given in `INSTALL COMPONENT ... SET` are taken from the installer. @@ -64,6 +66,25 @@ Because of the double reading, a `SELECT` whose string literal contains a backsl `CALL` is allowed for any routine in the `mysql_mcp` schema, not only the tool pack's. Keep only the tool pack in `mysql_mcp` (it is installed and dropped as a whole). +### Time limit (mcp.max_execution_time_ms) + +When an MCP session connects, the guard sets its session `max_execution_time` to `mcp.max_execution_time_ms`, so its `SELECT` statements stop after that many milliseconds (error 3024). The session may lower it, but not raise it. These are refused with error 1290 naming `mcp.max_execution_time_ms`: + +- `SET [SESSION] max_execution_time` (or `@@max_execution_time`) to 0, above the limit, or to anything but a literal (`DEFAULT`, an expression, a variable). +- The optimizer hints `MAX_EXECUTION_TIME(n)` and `SET_VAR(max_execution_time = n)` with such a value. + +A changed limit applies to sessions that connect afterwards. As with the server's own `max_execution_time`, it covers top-level read-only `SELECT` statements, not statements inside stored programs (such as the `mysql_mcp` procedures). + +### Error log (mcp.log_denials) + +Each refusal is written as a warning, with the setting, the connection id and the start of the statement (up to 256 bytes) or the table: + +``` +[Warning] [MY-011071] [mcp_guard] mcp_guard: refused by mcp.read_only: MCP connection 11: INSERT INTO d.t VALUES (1) +``` + +At most 10 lines are written per minute; the number left out is reported with the next line after the minute. The statement text can contain literal values, as in the general log; turn `mcp.log_denials` off if that is not acceptable. + ### Errors A denial fails the statement with error **1290** (`ER_OPTION_PREVENTS_STATEMENT`), the server's own `read_only` error, naming the setting: @@ -87,8 +108,8 @@ The option is `mcp.read_only`, `mcp.deny_system_schemas`, `mcp.allowed_schemas` - `mcp.allowed_schemas` limits **table access**, not catalog metadata: `information_schema` and `SHOW` still list what the account's grants allow. - With `mcp.deny_system_schemas = ON`, mysql-mcp-server's opt-in `slow_query_log` tool (which reads `mysql.slow_log`) is denied. -- Planned (RFC 0001): a `mcp.max_execution_time_ms` floor, denials in the error log and audit log, `performance_schema.mcp_tool_calls`. +- Planned (RFC 0001 Phase 3): denials in the audit log, `performance_schema.mcp_tool_calls`. ### Tests -`scripts/test-mcp-guard.sh ` runs install, restart, persistence, uninstall and the enforcement cases against the official image (65 checks, including a `lower_case_table_names=1` server), and `src/mcp/guard/test_mcp_guard_policy.cc` unit-tests the classification and the SQL scanner. CI (`integration-scripts.yml`, job `mcp-guard`) runs both on MySQL 8.4.11, 9.7.2 and 26.10.0. +`scripts/test-mcp-guard.sh ` runs install, restart, persistence, uninstall and the enforcement cases against the official image (78 checks, including a `lower_case_table_names=1` server), and `src/mcp/guard/test_mcp_guard_policy.cc` unit-tests the classification and the SQL scanner. CI (`integration-scripts.yml`, job `mcp-guard`) runs both on MySQL 8.4.11, 9.7.2 and 26.10.0. diff --git a/scripts/test-mcp-guard.sh b/scripts/test-mcp-guard.sh index 06efd0b..8b0509e 100755 --- a/scripts/test-mcp-guard.sh +++ b/scripts/test-mcp-guard.sh @@ -144,6 +144,31 @@ expect "mcp.enabled ON again" "$(mcp "SELECT 1")" "^1$" expect "invalid value rejected" "$(sql "SET GLOBAL mcp.read_only = 'maybe'")" "ERROR" +# --- mcp.max_execution_time_ms: a time limit MCP sessions cannot raise ------------------------ +expect "SET GLOBAL mcp.max_execution_time_ms = 1000" "$(sql "SET GLOBAL mcp.max_execution_time_ms = 1000")" "^$" +expect "mcp: session max_execution_time set at connect" "$(mcp "SELECT @@max_execution_time")" "^1000$" +expect "mcp: lowering it is allowed" "$(mcp "SET SESSION max_execution_time = 500; SELECT @@max_execution_time")" "^500$" +denied "mcp: SET max_execution_time = 0" "$(mcp "SET SESSION max_execution_time = 0")" "mcp.max_execution_time_ms" +denied "mcp: SET max_execution_time above the limit" "$(mcp "SET @@max_execution_time = 5000")" "mcp.max_execution_time_ms" +denied "mcp: MAX_EXECUTION_TIME(0) hint" "$(mcp "SELECT /*+ MAX_EXECUTION_TIME(0) */ COUNT(*) FROM d1.t")" "mcp.max_execution_time_ms" +denied "mcp: SET_VAR(max_execution_time=0) hint" "$(mcp "SELECT /*+ SET_VAR(max_execution_time=0) */ COUNT(*) FROM d1.t")" "mcp.max_execution_time_ms" +start=$(date +%s) +mcp "SELECT SLEEP(5) FROM d1.t" >/dev/null +took=$(($(date +%s) - start)) +if [ "$took" -le 3 ]; then ok "mcp: a 5s SELECT is stopped by the 1s limit (${took}s)"; else bad "mcp: time limit enforced" "took ${took}s"; fi +expect "app (no MCP_USER) keeps max_execution_time 0" "$(as app apppass1 "SELECT @@max_execution_time")" "^0$" +sql "SET GLOBAL mcp.max_execution_time_ms = 0" >/dev/null +expect "mcp: no limit once the floor is 0" "$(mcp "SELECT @@max_execution_time")" "^0$" + +# --- mcp.log_denials ----------------------------------------------------------------------------- +expect "refusals are written to the error log" "$(docker logs "$CNAME" 2>&1 | grep 'mcp_guard: refused by' | tail -3)" '\[MY-011071\] \[mcp_guard\] mcp_guard: refused by mcp\.(read_only|max_execution_time_ms): MCP connection [0-9]+' +sql "SET GLOBAL mcp.log_denials = OFF" >/dev/null +before=$(docker logs "$CNAME" 2>&1 | grep -c 'mcp_guard: refused by') +mcp "INSERT INTO d1.t VALUES (8)" >/dev/null +after=$(docker logs "$CNAME" 2>&1 | grep -c 'mcp_guard: refused by') +expect "mcp.log_denials = OFF writes nothing" "$after" "^$before$" +sql "SET GLOBAL mcp.log_denials = ON" >/dev/null + status=$(sql "SHOW GLOBAL STATUS LIKE 'mcp.%'") expect "status: statements_denied counted" "$status" "mcp.statements_denied [1-9]" expect "status: table_access_denied counted" "$status" "mcp.table_access_denied [1-9]" @@ -179,6 +204,9 @@ CREATE USER mcp@'%' IDENTIFIED BY 'mcppass1'; GRANT SELECT ON *.* TO mcp@'%'; GR expect "lower_case_table_names=1: SET GLOBAL mcp.allowed_schemas = 'Shop'" "$(sql "SET GLOBAL mcp.allowed_schemas = 'Shop'")" "^$" expect "lower_case_table_names=1: allowed_schemas 'Shop' allows shop" "$(mcp "SELECT COUNT(*) FROM Shop.t")" "^0$" denied "lower_case_table_names=1: other schemas still denied" "$(mcp "SELECT COUNT(*) FROM other.t")" "mcp.allowed_schemas" + # A fresh server: its first minute of refusals is logged up to 10 lines, then held back. + for _ in $(seq 1 15); do mcp "SELECT COUNT(*) FROM other.t" >/dev/null; done + expect "error log rate limit: 16 refusals in a minute, 10 lines" "$(docker logs "$CNAME" 2>&1 | grep -c 'mcp_guard: refused by')" "^10$" else bad "lower_case_table_names=1 server" "did not start" fi diff --git a/src/mcp/guard/component_mcp_guard.cc b/src/mcp/guard/component_mcp_guard.cc index c2e157c..f0c2db5 100644 --- a/src/mcp/guard/component_mcp_guard.cc +++ b/src/mcp/guard/component_mcp_guard.cc @@ -23,6 +23,9 @@ // SET, and CALL into the mysql_mcp tool pack; no table writes. // mcp.deny_system_schemas ON (default): no table access in mysql and sys. // mcp.allowed_schemas Comma-separated schemas MCP sessions may read; empty: all. +// mcp.max_execution_time_ms Time limit for their SELECTs (session max_execution_time, +// set at connect), which they cannot raise; 0: none. +// mcp.log_denials ON (default): refusals go to the error log, 10 a minute. // // Denials fail with ER_OPTION_PREVENTS_STATEMENT ("... running with the mcp.read_only // option ..."), like the server's own read_only. SET GLOBAL / PERSIST of mcp.* needs @@ -34,8 +37,11 @@ #include #include #include +#include #include #include +#include +#include #include #include #include @@ -44,6 +50,7 @@ #include #include +#include #include #include #include @@ -61,6 +68,15 @@ REQUIRES_SERVICE_PLACEHOLDER(component_sys_variable_register); REQUIRES_SERVICE_PLACEHOLDER(component_sys_variable_unregister); REQUIRES_SERVICE_PLACEHOLDER(status_variable_registration); REQUIRES_SERVICE_PLACEHOLDER(mysql_runtime_error); +REQUIRES_SERVICE_PLACEHOLDER(mysql_system_variable_update_integer); +REQUIRES_SERVICE_PLACEHOLDER(mysql_string_factory); +REQUIRES_SERVICE_PLACEHOLDER(mysql_string_converter); +REQUIRES_SERVICE_PLACEHOLDER(log_builtins); +REQUIRES_SERVICE_PLACEHOLDER(log_builtins_string); + +// LogEvent() uses these. +SERVICE_TYPE(log_builtins) *log_bi = nullptr; +SERVICE_TYPE(log_builtins_string) *log_bs = nullptr; namespace { @@ -70,6 +86,9 @@ constexpr int kErOptionPreventsStatement = 1290; // ER_SPECIFIC_ACCESS_DENIED_ERROR: "Access denied; you need (at least one of) the %-.128s // privilege(s) for this operation". constexpr int kErSpecificAccessDenied = 1227; +// ER_LOG_PRINTF_MSG: the error log code for a free-form component message (MY-011071 on +// 8.4, 9.7 and 26.10). +constexpr int kErLogPrintfMsg = 11071; constexpr const char *kMcpUser = "MCP_USER"; constexpr const char *kMcpAdmin = "MCP_ADMIN"; @@ -132,6 +151,8 @@ MYSQL_THD current_thd_or_null() { bool opt_enabled = true; bool opt_read_only = true; bool opt_deny_system_schemas = true; +bool opt_log_denials = true; +unsigned long opt_max_execution_time_ms = 0; // 0: no floor char *opt_allowed_schemas = nullptr; // server-managed (PLUGIN_VAR_MEMALLOC) // Parsed mcp.allowed_schemas, swapped as a whole on update; readers copy the pointer. @@ -169,17 +190,20 @@ struct BoolVar { bool def; }; -constexpr size_t kBoolVarCount = 3; +constexpr size_t kBoolVarCount = 4; const BoolVar kBoolVars[kBoolVarCount] = { {"enabled", "Allow mysql-mcp-server (MCP_USER) sessions; OFF refuses them all", &opt_enabled, true}, {"read_only", "MCP sessions may only read, whatever their grants", &opt_read_only, true}, {"deny_system_schemas", "MCP sessions may not access tables in mysql and sys", &opt_deny_system_schemas, true}, + {"log_denials", "Write refused MCP statements to the error log (at most 10 a minute)", + &opt_log_denials, true}, }; bool bool_registered[kBoolVarCount] = {}; bool allowed_registered = false; +bool time_registered = false; // Returns true if a variable could not be unregistered; it then stays marked as // registered and deinit fails, so the server keeps this library (and the update callback @@ -201,6 +225,13 @@ bool unregister_sysvars() { else allowed_registered = false; } + if (time_registered) { + if (mysql_service_component_sys_variable_unregister->unregister_variable( + "mcp", "max_execution_time_ms")) + failed = true; + else + time_registered = false; + } return failed; } @@ -242,6 +273,19 @@ bool register_sysvars() { nullptr, update_allowed_schemas, &str_arg, &opt_allowed_schemas)) return true; allowed_registered = true; + INTEGRAL_CHECK_ARG(ulong) time_arg; + time_arg.def_val = 0; + time_arg.min_val = 0; + time_arg.max_val = 4294967295UL; + time_arg.blk_sz = 0; + if (mysql_service_component_sys_variable_register->register_variable( + "mcp", "max_execution_time_ms", + PLUGIN_VAR_LONG | PLUGIN_VAR_UNSIGNED | PLUGIN_VAR_RQCMDARG, + "Time limit in milliseconds for SELECT statements of MCP sessions, which they cannot " + "raise; 0: none", + nullptr, nullptr, &time_arg, &opt_max_execution_time_ms)) + return true; + time_registered = true; // A persisted or command-line value is stored without the update callback: parse it. set_allowed_schemas(opt_allowed_schemas); return false; @@ -286,13 +330,70 @@ void mark_session(MYSQL_THD thd, bool mcp) { sessions_connected += mcp ? 1 : -1; } -bool deny(const char *option, std::atomic &counter) { +// Writes a refusal to the error log, at most kLogBurst lines per minute; the number left +// out is reported with the next line after the minute. +constexpr int kLogBurst = 10; +std::mutex log_mutex; +std::chrono::steady_clock::time_point log_window_start; +int log_window_count = 0; +long long log_suppressed = 0; + +void log_denial(const char *option, unsigned long long connection_id, std::string_view what) { + if (!opt_log_denials || log_bi == nullptr) return; + long long suppressed = 0; + { + std::lock_guard lock(log_mutex); + const auto now = std::chrono::steady_clock::now(); + if (now - log_window_start >= std::chrono::minutes(1)) { + suppressed = log_suppressed; + log_suppressed = 0; + log_window_start = now; + log_window_count = 0; + } + if (log_window_count >= kLogBurst) { + ++log_suppressed; + return; + } + ++log_window_count; + } + if (suppressed > 0) + LogEvent() + .type(LOG_TYPE_ERROR) + .prio(WARNING_LEVEL) + .errcode(kErLogPrintfMsg) + .subsys("mcp_guard") + .component("component: mcp_guard") + .message("mcp_guard: %lld more MCP refusals in the last minute were not logged", suppressed); + const int shown = static_cast(what.size() > 256 ? 256 : what.size()); + LogEvent() + .type(LOG_TYPE_ERROR) + .prio(WARNING_LEVEL) + .errcode(kErLogPrintfMsg) + .subsys("mcp_guard") + .component("component: mcp_guard") + .message("mcp_guard: refused by %s: MCP connection %llu: %.*s%s", option, connection_id, shown, + what.data(), what.size() > 256 ? "..." : ""); +} + +bool deny(const char *option, std::atomic &counter, unsigned long long connection_id, + std::string_view what) { ++counter; + log_denial(option, connection_id, what); mysql_error_service_emit_printf(mysql_service_mysql_runtime_error, kErOptionPreventsStatement, 0, option); return true; } +// Sets the session's max_execution_time to the mcp.max_execution_time_ms floor. +my_h_string max_execution_time_name = nullptr; + +void apply_time_limit(MYSQL_THD thd) { + const unsigned long floor = opt_max_execution_time_ms; + if (floor == 0 || max_execution_time_name == nullptr) return; + mysql_service_mysql_system_variable_update_integer->set_unsigned( + thd, "SESSION", nullptr, max_execution_time_name, floor); +} + } // namespace // ----- Event tracking ----------------------------------------------------------------- @@ -315,9 +416,10 @@ bool Event_tracking_connection_implementation::callback( const bool mcp = thd_has_grant(thd, kMcpUser) && !thd_has_grant(thd, kMcpAdmin); if (mcp && !opt_enabled) { mark_session(thd, false); // refused: no disconnect event follows - return deny("mcp.enabled", statements_denied); + return deny("mcp.enabled", statements_denied, data->connection_id, "connection refused"); } mark_session(thd, mcp); + if (mcp) apply_time_limit(thd); return false; } case EVENT_TRACKING_CONNECTION_DISCONNECT: @@ -361,14 +463,17 @@ bool Event_tracking_query_implementation::callback(const mysql_event_tracking_qu MYSQL_THD thd = current_thd_or_null(); if (!is_mcp_session(thd)) return false; if (data->event_subclass == EVENT_TRACKING_QUERY_START) ++statements_total; - if (!opt_enabled) return deny("mcp.enabled", statements_denied); + const char *cmd = data->sql_command ? data->sql_command : ""; + const char *query = data->query.str ? data->query.str : ""; + const size_t len = data->query.str ? data->query.length : 0; + const std::string_view text(query, len); + if (!opt_enabled) return deny("mcp.enabled", statements_denied, data->connection_id, text); if (opt_read_only) { - const char *cmd = data->sql_command ? data->sql_command : ""; - const char *query = data->query.str ? data->query.str : ""; - const size_t len = data->query.str ? data->query.length : 0; const char *why = mcp_guard::read_only_violation(cmd, query, len); - if (why != nullptr) return deny(why, statements_denied); + if (why != nullptr) return deny(why, statements_denied, data->connection_id, text); } + const char *why = mcp_guard::time_limit_violation(cmd, query, len, opt_max_execution_time_ms); + if (why != nullptr) return deny(why, statements_denied, data->connection_id, text); return false; } @@ -381,7 +486,18 @@ bool Event_tracking_table_access_implementation::callback( const mysql_event_tracking_table_access_data *data) { MYSQL_THD thd = current_thd_or_null(); if (!is_mcp_session(thd)) return false; - if (!opt_enabled) return deny("mcp.enabled", table_access_denied); + const std::string_view db(data->table_database.str ? data->table_database.str : "", + data->table_database.str ? data->table_database.length : 0); + const std::string_view table(data->table_name.str ? data->table_name.str : "", + data->table_name.str ? data->table_name.length : 0); + const bool write = data->event_subclass != EVENT_TRACKING_TABLE_ACCESS_READ; + // Built only for a refusal: table events are frequent. + const auto refuse = [&](const char *option) { + const std::string what = + std::string(write ? "write " : "read ") + std::string(db) + "." + std::string(table); + return deny(option, table_access_denied, data->connection_id, what); + }; + if (!opt_enabled) return refuse("mcp.enabled"); const auto allowed = current_allowed_schemas(); mcp_guard::TablePolicy policy; policy.read_only = opt_read_only; @@ -389,11 +505,8 @@ bool Event_tracking_table_access_implementation::callback( policy.restrict_schemas = allowed->restrict; policy.allowed_schemas = &allowed->names; policy.case_insensitive_names = case_insensitive_names; - const std::string_view db(data->table_database.str ? data->table_database.str : "", - data->table_database.str ? data->table_database.length : 0); - const bool write = data->event_subclass != EVENT_TRACKING_TABLE_ACCESS_READ; const char *why = mcp_guard::table_violation(policy, db, write); - if (why != nullptr) return deny(why, table_access_denied); + if (why != nullptr) return refuse(why); return false; } @@ -422,10 +535,20 @@ mysql_service_status_t mcp_guard_deinit() { session_slot = nullptr; } if (unregister_privileges()) failed = true; + if (max_execution_time_name != nullptr) { + mysql_service_mysql_string_factory->destroy(max_execution_time_name); + max_execution_time_name = nullptr; + } return failed ? 1 : 0; } mysql_service_status_t mcp_guard_init() { + log_bi = mysql_service_log_builtins; + log_bs = mysql_service_log_builtins_string; + static const char kName[] = "max_execution_time"; + if (mysql_service_mysql_string_converter->convert_from_buffer( + &max_execution_time_name, kName, sizeof(kName) - 1, "latin1")) + max_execution_time_name = nullptr; if (register_privileges() || mysql_service_mysql_thd_store->register_slot("component_mcp_guard", free_session_slot, &session_slot) || @@ -458,6 +581,9 @@ REQUIRES_SERVICE(dynamic_privilege_register), REQUIRES_SERVICE(global_grants_che REQUIRES_SERVICE(mysql_thd_store), REQUIRES_SERVICE(component_sys_variable_register), REQUIRES_SERVICE(component_sys_variable_unregister), REQUIRES_SERVICE(status_variable_registration), REQUIRES_SERVICE(mysql_runtime_error), + REQUIRES_SERVICE(mysql_system_variable_update_integer), + REQUIRES_SERVICE(mysql_string_factory), REQUIRES_SERVICE(mysql_string_converter), + REQUIRES_SERVICE(log_builtins), REQUIRES_SERVICE(log_builtins_string), END_COMPONENT_REQUIRES(); BEGIN_COMPONENT_METADATA(mcp_guard) diff --git a/src/mcp/guard/mcp_guard_policy.h b/src/mcp/guard/mcp_guard_policy.h index 18f931e..1aa4c9a 100644 --- a/src/mcp/guard/mcp_guard_policy.h +++ b/src/mcp/guard/mcp_guard_policy.h @@ -24,6 +24,7 @@ #include #include +#include #include #include #include @@ -214,6 +215,83 @@ inline const char *read_only_violation(std::string_view cmd, const char *query, return nullptr; } +// ----- mcp.max_execution_time_ms ------------------------------------------------------ + +inline bool all_digits(std::string_view s) { + if (s.empty() || s.size() > 18) return false; + for (char c : s) + if (!std::isdigit(static_cast(c))) return false; + return true; +} + +// An explicit limit is acceptable when it is a positive literal no larger than the floor. +inline bool within_floor(std::string_view value, unsigned long long floor) { + if (!all_digits(value)) return false; + const unsigned long long v = std::stoull(std::string(value)); + return v > 0 && v <= floor; +} + +// A SET of the session's max_execution_time that would lift the floor: to 0, above it, or +// to anything but a literal (DEFAULT, an expression, a variable). +inline bool set_raises_time_limit(const std::vector &t, unsigned long long floor) { + for (size_t i = 0; i < t.size(); ++i) { + if (t[i].kind != Token::kWord) continue; + if (t[i].text != "MAX_EXECUTION_TIME" && t[i].text != "@@MAX_EXECUTION_TIME") continue; + size_t j = i + 1; + if (j < t.size() && t[j].kind == Token::kPunct && t[j].text == ":") ++j; + if (!(j < t.size() && t[j].kind == Token::kPunct && t[j].text == "=")) continue; // not assigned + ++j; + const bool literal = j < t.size() && t[j].kind == Token::kWord && + (j + 1 == t.size() || (t[j + 1].kind == Token::kPunct && + (t[j + 1].text == "," || t[j + 1].text == ";"))); + if (!literal || !within_floor(t[j].text, floor)) return true; + } + return false; +} + +// An optimizer hint (/*+ ... */) that sets the statement's time limit, either +// MAX_EXECUTION_TIME(n) or SET_VAR(max_execution_time = n), to 0, above the floor, or to +// something that is not a literal. Searched in the raw text, so a hint-like string literal +// is also refused; that only errs on the safe side. +inline bool hint_raises_time_limit(const char *s, size_t n, unsigned long long floor) { + std::string upper(s, n); + for (char &c : upper) c = static_cast(std::toupper(static_cast(c))); + size_t pos = 0; + while ((pos = upper.find("/*+", pos)) != std::string::npos) { + const size_t end = upper.find("*/", pos + 3); + const std::string body = upper.substr(pos + 3, end == std::string::npos ? std::string::npos + : end - pos - 3); + size_t k = 0; + while ((k = body.find("MAX_EXECUTION_TIME", k)) != std::string::npos) { + size_t v = k + std::strlen("MAX_EXECUTION_TIME"); + while (v < body.size() && std::isspace(static_cast(body[v]))) ++v; + if (v < body.size() && (body[v] == '(' || body[v] == '=')) ++v; + while (v < body.size() && std::isspace(static_cast(body[v]))) ++v; + size_t e = v; + while (e < body.size() && std::isdigit(static_cast(body[e]))) ++e; + if (!within_floor(std::string_view(body).substr(v, e - v), floor)) return true; + k = e; + } + if (end == std::string::npos) break; + pos = end + 2; + } + return false; +} + +// Returns "mcp.max_execution_time_ms" when the statement would lift the floor, or nullptr. +// floor == 0 disables the check. Text is read in both quoting modes, as for read_only. +inline const char *time_limit_violation(std::string_view cmd, const char *query, size_t len, + unsigned long long floor) { + if (floor == 0) return nullptr; + if (hint_raises_time_limit(query, len, floor)) return "mcp.max_execution_time_ms"; + if (cmd == "set_option") { + for (bool escapes : {true, false}) + if (set_raises_time_limit(tokenize(query, len, escapes), floor)) + return "mcp.max_execution_time_ms"; + } + return nullptr; +} + // Parses mcp.allowed_schemas: comma-separated schema names, spaces trimmed, empty // entries dropped. An empty list means every schema. inline std::vector parse_schema_list(std::string_view v) { diff --git a/src/mcp/guard/test_mcp_guard_policy.cc b/src/mcp/guard/test_mcp_guard_policy.cc index 1210261..1eaa175 100644 --- a/src/mcp/guard/test_mcp_guard_policy.cc +++ b/src/mcp/guard/test_mcp_guard_policy.cc @@ -96,6 +96,44 @@ int main() { expect_ro("select", "SELECT \"for\" FROM t", false); expect_ro("call", "CALL mysql_mcp.list_tables('\\') ", false); + // mcp.max_execution_time_ms + struct TimeCase { + const char *cmd, *sql; + bool denied; + }; + const TimeCase time_cases[] = { + {"set_option", "SET SESSION max_execution_time = 500", false}, + {"set_option", "SET max_execution_time=1000", false}, + {"set_option", "SET @@session.max_execution_time = 999, sql_mode = ''", false}, + {"set_option", "SET @max_execution_time = 0", false}, // a user variable + {"set_option", "SET sql_mode = ''", false}, + {"set_option", "SET SESSION max_execution_time = 0", true}, + {"set_option", "SET max_execution_time = 5000", true}, + {"set_option", "SET @@max_execution_time := 2000", true}, + {"set_option", "SET max_execution_time = DEFAULT", true}, + {"set_option", "SET max_execution_time = 500 * 100", true}, + {"set_option", "SET max_execution_time = @x", true}, + {"set_option", "SET a = 1, max_execution_time = 0", true}, + {"select", "SELECT /*+ MAX_EXECUTION_TIME(500) */ * FROM t", false}, + {"select", "SELECT /*+ MAX_EXECUTION_TIME(0) */ * FROM t", true}, + {"select", "SELECT /*+ max_execution_time( 5000 ) */ * FROM t", true}, + {"select", "SELECT /*+ SET_VAR(max_execution_time=0) */ * FROM t", true}, + {"select", "SELECT /*+ SET_VAR(max_execution_time = 800) */ * FROM t", false}, + {"select", "SELECT /*+ BKA(t) */ * FROM t", false}, + {"select", "SELECT 'max_execution_time = 0' FROM t", false}, + }; + for (const TimeCase &c : time_cases) { + const char *why = time_limit_violation(c.cmd, c.sql, std::strlen(c.sql), 1000); + if ((why != nullptr) != c.denied) { + std::printf("FAIL time limit %-60s want %s\n", c.sql, c.denied ? "deny" : "allow"); + ++failures; + } + } + if (time_limit_violation("set_option", "SET max_execution_time = 0", 26, 0) != nullptr) { + std::printf("FAIL time limit floor 0 must not check\n"); + ++failures; + } + // Tokenizer details. { const char *q = "SELECT 'it''s' , \"a\\\"b\" FROM `we``ird`"; From 3c4526b3cf0a45ca1bbe56a3f21a744b5182438a Mon Sep 17 00:00:00 2001 From: Alkin Tezuysal Date: Fri, 30 Oct 2026 12:27:23 +0000 Subject: [PATCH 2/6] fix(mcp): validate the whole time-limit hint operand; redact logged literals; reapply changed limit - SET_VAR(max_execution_time=1G) / MAX_EXECUTION_TIME(1K) read only the leading digits and passed, while MySQL scales the suffix: the operand must now be the number alone, followed by ) or , (Codex P1). - Error log: string literals become '?', line breaks spaces. - A changed mcp.max_execution_time_ms reaches connected MCP sessions at their next statement (per-session state records the applied limit). - Docs: 0 turns off only the guard's limit; CALL mysql_mcp.* is not limited (stored programs ignore max_execution_time) and how to cover it. Test asserts the interrupted SELECT's result (1 or 3024). 80 checks on 8.4.11, 9.7.2, 26.10.0. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 4 +- docs/MCP_COMPONENTS.md | 12 +++-- scripts/test-mcp-guard.sh | 17 ++++++- src/mcp/guard/component_mcp_guard.cc | 65 +++++++++++++++++++------- src/mcp/guard/mcp_guard_policy.h | 39 ++++++++++++++++ src/mcp/guard/test_mcp_guard_policy.cc | 19 ++++++++ 6 files changed, 129 insertions(+), 27 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 42c0904..e404d56 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,8 +10,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added - **`component_mcp_guard` time limit and refusal log** (Phase 2 of RFC 0001, completed). - **`mcp.max_execution_time_ms`:** sets an MCP session's `max_execution_time` at connect. Attempts to raise it are refused with error 1290: via `SET`, or via the `MAX_EXECUTION_TIME(n)` or `SET_VAR(max_execution_time=n)` hints. Lowering it is allowed. - - **`mcp.log_denials` (default ON):** writes each refusal to the error log as `[MY-011071] [mcp_guard]`, at most 10 lines a minute. - - **Tests:** `scripts/test-mcp-guard.sh` now has 78 checks, including a 5 s `SELECT` stopped by a 1 s limit and the log rate limit on a fresh server, passing on MySQL 8.4.11, 9.7.2 and 26.10.0. + - **`mcp.log_denials` (default ON):** writes each refusal to the error log as `[MY-011071] [mcp_guard]`, with string literals redacted, at most 10 lines a minute. + - **Tests:** `scripts/test-mcp-guard.sh` now has 80 checks, including a 5 s `SELECT` stopped by a 1 s limit, a tightened limit reaching a connected session, literal redaction in the log and the log rate limit on a fresh server, passing on MySQL 8.4.11, 9.7.2 and 26.10.0. - **`component_mcp_guard` enforcement** (Phase 2 of [RFC 0001](https://github.com/askdba/mysql-mcp-server/blob/main/docs/rfc/0001-mysql-component-architecture.md)). - **MCP sessions:** sessions of accounts holding `MCP_USER` (and not `MCP_ADMIN`) are checked against `mcp.*` settings inside `mysqld`. The checks run on the server's query, table-access and connection events. - **`mcp.read_only` (default ON):** only reads, even with write grants. It uses an allowlist of `sql_command` classes and also refuses `SELECT … INTO OUTFILE`, locking reads, `SET GLOBAL` and `CALL` outside `mysql_mcp`. diff --git a/docs/MCP_COMPONENTS.md b/docs/MCP_COMPONENTS.md index ddf941d..634025a 100644 --- a/docs/MCP_COMPONENTS.md +++ b/docs/MCP_COMPONENTS.md @@ -39,7 +39,7 @@ A session is an **MCP session** when its account holds `MCP_USER` and not `MCP_A | `mcp.read_only` | `ON` | Only reads, whatever the account's grants (below). | | `mcp.deny_system_schemas` | `ON` | No table access in `mysql` and `sys`. `information_schema` and `performance_schema` stay readable (the tools use them; grants govern them). | | `mcp.allowed_schemas` | empty | Comma-separated schemas whose tables MCP sessions may access; blank means all. A value with no names in it, such as `,`, allows none. Names are compared exactly, or case-insensitively when `lower_case_table_names` is 1 or 2. The system schemas above are governed by `mcp.deny_system_schemas`, and `mysql_mcp` is always allowed. | -| `mcp.max_execution_time_ms` | 0 | Time limit for their `SELECT` statements, which they cannot raise; 0 means none (below). | +| `mcp.max_execution_time_ms` | 0 | Time limit for their `SELECT` statements, which they cannot raise; 0 turns the guard's limit off (the server's own `max_execution_time` default still applies) (below). | | `mcp.log_denials` | `ON` | Write refusals to the error log, at most 10 lines a minute (below). | `SET GLOBAL` and `SET PERSIST` of `mcp.*` need `MCP_ADMIN` on top of `SYSTEM_VARIABLES_ADMIN`. Persisted values load at startup as usual. Values given in `INSTALL COMPONENT ... SET` are taken from the installer. @@ -73,17 +73,19 @@ When an MCP session connects, the guard sets its session `max_execution_time` to - `SET [SESSION] max_execution_time` (or `@@max_execution_time`) to 0, above the limit, or to anything but a literal (`DEFAULT`, an expression, a variable). - The optimizer hints `MAX_EXECUTION_TIME(n)` and `SET_VAR(max_execution_time = n)` with such a value. -A changed limit applies to sessions that connect afterwards. As with the server's own `max_execution_time`, it covers top-level read-only `SELECT` statements, not statements inside stored programs (such as the `mysql_mcp` procedures). +A changed limit reaches connected MCP sessions at their next statement. Setting it back to 0 leaves their current session value as it is. + +As with the server's own `max_execution_time`, the limit covers top-level read-only `SELECT` statements. MySQL ignores it inside stored programs, so **`CALL mysql_mcp.*` tool-pack calls are not limited**. Those run catalog queries on `information_schema`. If they must be limited too, revoke `EXECUTE` on `mysql_mcp` from the MCP account: mysql-mcp-server then falls back to its own SQL, whose `SELECT`s the limit covers. ### Error log (mcp.log_denials) -Each refusal is written as a warning, with the setting, the connection id and the start of the statement (up to 256 bytes) or the table: +Each refusal is written as a warning, with the setting, the connection id and the start of the statement (up to 256 bytes, string literals replaced by `'?'`, line breaks by spaces) or the table: ``` [Warning] [MY-011071] [mcp_guard] mcp_guard: refused by mcp.read_only: MCP connection 11: INSERT INTO d.t VALUES (1) ``` -At most 10 lines are written per minute; the number left out is reported with the next line after the minute. The statement text can contain literal values, as in the general log; turn `mcp.log_denials` off if that is not acceptable. +At most 10 lines are written per minute; the number left out is reported with the next line after the minute. String literals are redacted, but numbers and identifiers are kept; turn `mcp.log_denials` off if even those must not reach the error log. ### Errors @@ -112,4 +114,4 @@ The option is `mcp.read_only`, `mcp.deny_system_schemas`, `mcp.allowed_schemas` ### Tests -`scripts/test-mcp-guard.sh ` runs install, restart, persistence, uninstall and the enforcement cases against the official image (78 checks, including a `lower_case_table_names=1` server), and `src/mcp/guard/test_mcp_guard_policy.cc` unit-tests the classification and the SQL scanner. CI (`integration-scripts.yml`, job `mcp-guard`) runs both on MySQL 8.4.11, 9.7.2 and 26.10.0. +`scripts/test-mcp-guard.sh ` runs install, restart, persistence, uninstall and the enforcement cases against the official image (80 checks, including a `lower_case_table_names=1` server), and `src/mcp/guard/test_mcp_guard_policy.cc` unit-tests the classification and the SQL scanner. CI (`integration-scripts.yml`, job `mcp-guard`) runs both on MySQL 8.4.11, 9.7.2 and 26.10.0. diff --git a/scripts/test-mcp-guard.sh b/scripts/test-mcp-guard.sh index 8b0509e..df0c5b2 100755 --- a/scripts/test-mcp-guard.sh +++ b/scripts/test-mcp-guard.sh @@ -153,15 +153,28 @@ denied "mcp: SET max_execution_time above the limit" "$(mcp "SET @@max_execution denied "mcp: MAX_EXECUTION_TIME(0) hint" "$(mcp "SELECT /*+ MAX_EXECUTION_TIME(0) */ COUNT(*) FROM d1.t")" "mcp.max_execution_time_ms" denied "mcp: SET_VAR(max_execution_time=0) hint" "$(mcp "SELECT /*+ SET_VAR(max_execution_time=0) */ COUNT(*) FROM d1.t")" "mcp.max_execution_time_ms" start=$(date +%s) -mcp "SELECT SLEEP(5) FROM d1.t" >/dev/null +slept=$(mcp "SELECT SLEEP(5) FROM d1.t") took=$(($(date +%s) - start)) -if [ "$took" -le 3 ]; then ok "mcp: a 5s SELECT is stopped by the 1s limit (${took}s)"; else bad "mcp: time limit enforced" "took ${took}s"; fi +# Interrupted by the time limit: SLEEP() alone returns 1; reading a table, as here, the +# statement fails with 3024 (ER_QUERY_TIMEOUT). Either way it must not run 5s. +if [ "$took" -le 3 ] && { [ "$slept" = 1 ] || printf '%s' "$slept" | grep -q '^ERROR 3024 '; }; then ok "mcp: a 5s SELECT is stopped by the 1s limit (${took}s)"; else bad "mcp: time limit enforced" "took ${took}s, result '$slept'"; fi expect "app (no MCP_USER) keeps max_execution_time 0" "$(as app apppass1 "SELECT @@max_execution_time")" "^0$" sql "SET GLOBAL mcp.max_execution_time_ms = 0" >/dev/null expect "mcp: no limit once the floor is 0" "$(mcp "SELECT @@max_execution_time")" "^0$" +# A session already connected gets a new limit at its next statement. +mcp "SELECT @@max_execution_time; DO SLEEP(3); SELECT @@max_execution_time" >"/tmp/$CNAME-live" 2>&1 & +live=$! +sleep 1 +sql "SET GLOBAL mcp.max_execution_time_ms = 1500" >/dev/null +wait "$live" +expect "mcp: an existing session gets a tightened limit" "$(tr '\n' ' ' <"/tmp/$CNAME-live")" "^0 1500 $" +rm -f "/tmp/$CNAME-live" +sql "SET GLOBAL mcp.max_execution_time_ms = 0" >/dev/null # --- mcp.log_denials ----------------------------------------------------------------------------- expect "refusals are written to the error log" "$(docker logs "$CNAME" 2>&1 | grep 'mcp_guard: refused by' | tail -3)" '\[MY-011071\] \[mcp_guard\] mcp_guard: refused by mcp\.(read_only|max_execution_time_ms): MCP connection [0-9]+' +mcp "SELECT 'token=hunter2' FROM d1.t FOR UPDATE" >/dev/null +expect "error log redacts string literals" "$(docker logs "$CNAME" 2>&1 | grep 'mcp_guard: refused by' | grep -c 'hunter2')" "^0$" sql "SET GLOBAL mcp.log_denials = OFF" >/dev/null before=$(docker logs "$CNAME" 2>&1 | grep -c 'mcp_guard: refused by') mcp "INSERT INTO d1.t VALUES (8)" >/dev/null diff --git a/src/mcp/guard/component_mcp_guard.cc b/src/mcp/guard/component_mcp_guard.cc index f0c2db5..2bfb2c9 100644 --- a/src/mcp/guard/component_mcp_guard.cc +++ b/src/mcp/guard/component_mcp_guard.cc @@ -53,6 +53,7 @@ #include #include #include +#include #include #include #include @@ -314,19 +315,37 @@ bool status_registered = false; // ----- MCP session marker (per THD) --------------------------------------------------- +// Per-session state of an MCP session, in a THD slot (null for other sessions). +struct SessionState { + unsigned long applied_time_limit = 0; // mcp.max_execution_time_ms last applied +}; + mysql_thd_store_slot session_slot = nullptr; -char mcp_session_marker; // the slot holds &mcp_session_marker for MCP sessions -int free_session_slot(void *) { return 0; } // the marker is static +// Called by the server when the THD ends with a state still attached. +int free_session_slot(void *state) { + delete static_cast(state); + return 0; +} -bool is_mcp_session(MYSQL_THD thd) { - return thd != nullptr && session_slot != nullptr && - mysql_service_mysql_thd_store->get(thd, session_slot) == &mcp_session_marker; +SessionState *session_state(MYSQL_THD thd) { + if (thd == nullptr || session_slot == nullptr) return nullptr; + return static_cast(mysql_service_mysql_thd_store->get(thd, session_slot)); } +bool is_mcp_session(MYSQL_THD thd) { return session_state(thd) != nullptr; } + void mark_session(MYSQL_THD thd, bool mcp) { - if (is_mcp_session(thd) == mcp) return; - mysql_service_mysql_thd_store->set(thd, session_slot, mcp ? &mcp_session_marker : nullptr); + SessionState *state = session_state(thd); + if ((state != nullptr) == mcp) return; + if (mcp) { + state = new (std::nothrow) SessionState(); + if (state == nullptr) return; + mysql_service_mysql_thd_store->set(thd, session_slot, state); + } else { + mysql_service_mysql_thd_store->set(thd, session_slot, nullptr); + delete state; + } sessions_connected += mcp ? 1 : -1; } @@ -364,15 +383,15 @@ void log_denial(const char *option, unsigned long long connection_id, std::strin .subsys("mcp_guard") .component("component: mcp_guard") .message("mcp_guard: %lld more MCP refusals in the last minute were not logged", suppressed); - const int shown = static_cast(what.size() > 256 ? 256 : what.size()); + const std::string shown = mcp_guard::redact_for_log(what.data(), what.size()); LogEvent() .type(LOG_TYPE_ERROR) .prio(WARNING_LEVEL) .errcode(kErLogPrintfMsg) .subsys("mcp_guard") .component("component: mcp_guard") - .message("mcp_guard: refused by %s: MCP connection %llu: %.*s%s", option, connection_id, shown, - what.data(), what.size() > 256 ? "..." : ""); + .message("mcp_guard: refused by %s: MCP connection %llu: %s", option, connection_id, + shown.c_str()); } bool deny(const char *option, std::atomic &counter, unsigned long long connection_id, @@ -384,14 +403,20 @@ bool deny(const char *option, std::atomic &counter, unsigned long lon return true; } -// Sets the session's max_execution_time to the mcp.max_execution_time_ms floor. +// Sets the session's max_execution_time to mcp.max_execution_time_ms when the setting +// differs from what was last applied to this session: at connect, and at the next +// statement after an administrator changes it. Between changes the session may lower +// its own value (and cannot raise it). When the setting goes back to 0 the session +// value is left as it is. my_h_string max_execution_time_name = nullptr; -void apply_time_limit(MYSQL_THD thd) { - const unsigned long floor = opt_max_execution_time_ms; - if (floor == 0 || max_execution_time_name == nullptr) return; +void apply_time_limit(MYSQL_THD thd, SessionState *state) { + const unsigned long limit = opt_max_execution_time_ms; + if (state == nullptr || limit == state->applied_time_limit) return; + state->applied_time_limit = limit; + if (limit == 0 || max_execution_time_name == nullptr) return; mysql_service_mysql_system_variable_update_integer->set_unsigned( - thd, "SESSION", nullptr, max_execution_time_name, floor); + thd, "SESSION", nullptr, max_execution_time_name, limit); } } // namespace @@ -419,7 +444,7 @@ bool Event_tracking_connection_implementation::callback( return deny("mcp.enabled", statements_denied, data->connection_id, "connection refused"); } mark_session(thd, mcp); - if (mcp) apply_time_limit(thd); + if (mcp) apply_time_limit(thd, session_state(thd)); return false; } case EVENT_TRACKING_CONNECTION_DISCONNECT: @@ -461,8 +486,12 @@ mysql_event_tracking_query_subclass_t Event_tracking_query_implementation::filte bool Event_tracking_query_implementation::callback(const mysql_event_tracking_query_data *data) { MYSQL_THD thd = current_thd_or_null(); - if (!is_mcp_session(thd)) return false; - if (data->event_subclass == EVENT_TRACKING_QUERY_START) ++statements_total; + SessionState *state = session_state(thd); + if (state == nullptr) return false; + if (data->event_subclass == EVENT_TRACKING_QUERY_START) { + ++statements_total; + apply_time_limit(thd, state); // picks up a changed mcp.max_execution_time_ms + } const char *cmd = data->sql_command ? data->sql_command : ""; const char *query = data->query.str ? data->query.str : ""; const size_t len = data->query.str ? data->query.length : 0; diff --git a/src/mcp/guard/mcp_guard_policy.h b/src/mcp/guard/mcp_guard_policy.h index 1aa4c9a..96d7b50 100644 --- a/src/mcp/guard/mcp_guard_policy.h +++ b/src/mcp/guard/mcp_guard_policy.h @@ -270,6 +270,10 @@ inline bool hint_raises_time_limit(const char *s, size_t n, unsigned long long f size_t e = v; while (e < body.size() && std::isdigit(static_cast(body[e]))) ++e; if (!within_floor(std::string_view(body).substr(v, e - v), floor)) return true; + // The whole operand must be the number: MySQL reads 1G or 1K as a scaled value. + size_t after = e; + while (after < body.size() && std::isspace(static_cast(body[after]))) ++after; + if (after >= body.size() || (body[after] != ')' && body[after] != ',')) return true; k = e; } if (end == std::string::npos) break; @@ -292,6 +296,41 @@ inline const char *time_limit_violation(std::string_view cmd, const char *query, return nullptr; } +// ----- Error log ----------------------------------------------------------------------- + +// The statement text for the error log: string literals replaced by '?' (they can hold +// secrets), line breaks and tabs by spaces, at most max_len bytes. Quoted identifiers stay. +inline std::string redact_for_log(const char *s, size_t n, size_t max_len = 256) { + std::string out; + size_t i = 0; + while (i < n && out.size() < max_len) { + const char c = s[i]; + if (c == '\'' || c == '"') { + out.push_back(c); + out.push_back('?'); + out.push_back(c); + ++i; + while (i < n) { + if (s[i] == '\\' && i + 1 < n) { + i += 2; + } else if (s[i] == c && i + 1 < n && s[i + 1] == c) { + i += 2; + } else if (s[i] == c) { + ++i; + break; + } else { + ++i; + } + } + continue; + } + out.push_back(c == '\n' || c == '\r' || c == '\t' ? ' ' : c); + ++i; + } + if (i < n) out += "..."; + return out; +} + // Parses mcp.allowed_schemas: comma-separated schema names, spaces trimmed, empty // entries dropped. An empty list means every schema. inline std::vector parse_schema_list(std::string_view v) { diff --git a/src/mcp/guard/test_mcp_guard_policy.cc b/src/mcp/guard/test_mcp_guard_policy.cc index 1eaa175..7c9c951 100644 --- a/src/mcp/guard/test_mcp_guard_policy.cc +++ b/src/mcp/guard/test_mcp_guard_policy.cc @@ -121,6 +121,11 @@ int main() { {"select", "SELECT /*+ SET_VAR(max_execution_time = 800) */ * FROM t", false}, {"select", "SELECT /*+ BKA(t) */ * FROM t", false}, {"select", "SELECT 'max_execution_time = 0' FROM t", false}, + {"select", "SELECT /*+ SET_VAR(max_execution_time=1G) */ SLEEP(5)", true}, + {"select", "SELECT /*+ MAX_EXECUTION_TIME(1K) */ SLEEP(5)", true}, + {"select", "SELECT /*+ SET_VAR(max_execution_time=5.5) */ 1", true}, + {"select", "SELECT /*+ MAX_EXECUTION_TIME(900) BKA(t) */ * FROM t", false}, + {"set_option", "SET max_execution_time = 1G", true}, }; for (const TimeCase &c : time_cases) { const char *why = time_limit_violation(c.cmd, c.sql, std::strlen(c.sql), 1000); @@ -134,6 +139,20 @@ int main() { ++failures; } + // Error log redaction. + { + const char *q = "SELECT 'token=secret', \"pw\" FROM `t`\nWHERE a = 'it''s'"; + const std::string r = redact_for_log(q, std::strlen(q)); + if (r != "SELECT '?', \"?\" FROM `t` WHERE a = '?'") { + std::printf("FAIL redact_for_log: %s\n", r.c_str()); + ++failures; + } + if (redact_for_log(q, std::strlen(q), 10) != "SELECT '?'...") { + std::printf("FAIL redact_for_log truncation: %s\n", redact_for_log(q, std::strlen(q), 10).c_str()); + ++failures; + } + } + // Tokenizer details. { const char *q = "SELECT 'it''s' , \"a\\\"b\" FROM `we``ird`"; From c15191cd47225b7d6d4b6db12eabe7649115acac Mon Sep 17 00:00:00 2001 From: Alkin Tezuysal Date: Fri, 30 Oct 2026 13:37:28 +0000 Subject: [PATCH 3/6] fix(mcp): refuse an MCP connection that cannot be marked; redact in both quoting modes - mark_session() reports failure (allocation or THD-slot set); the connection is then refused (1290, mcp_guard) instead of running unguarded. - Log redaction reads literals with and without backslash escapes; if the readings differ (NO_BACKSLASH_ESCAPES), only the text before the first quote is logged, so 'a\', 'secret' cannot leak. Co-Authored-By: Claude Opus 5.5 --- scripts/test-mcp-guard.sh | 1 + src/mcp/guard/component_mcp_guard.cc | 19 ++++++++++++++----- src/mcp/guard/mcp_guard_policy.h | 21 +++++++++++++++++---- src/mcp/guard/test_mcp_guard_policy.cc | 13 +++++++++++++ 4 files changed, 45 insertions(+), 9 deletions(-) diff --git a/scripts/test-mcp-guard.sh b/scripts/test-mcp-guard.sh index df0c5b2..c439bdb 100755 --- a/scripts/test-mcp-guard.sh +++ b/scripts/test-mcp-guard.sh @@ -174,6 +174,7 @@ sql "SET GLOBAL mcp.max_execution_time_ms = 0" >/dev/null # --- mcp.log_denials ----------------------------------------------------------------------------- expect "refusals are written to the error log" "$(docker logs "$CNAME" 2>&1 | grep 'mcp_guard: refused by' | tail -3)" '\[MY-011071\] \[mcp_guard\] mcp_guard: refused by mcp\.(read_only|max_execution_time_ms): MCP connection [0-9]+' mcp "SELECT 'token=hunter2' FROM d1.t FOR UPDATE" >/dev/null +mcp "SET SESSION sql_mode = 'NO_BACKSLASH_ESCAPES'; SELECT 'a\\', 'token=hunter2' FROM d1.t FOR UPDATE" >/dev/null expect "error log redacts string literals" "$(docker logs "$CNAME" 2>&1 | grep 'mcp_guard: refused by' | grep -c 'hunter2')" "^0$" sql "SET GLOBAL mcp.log_denials = OFF" >/dev/null before=$(docker logs "$CNAME" 2>&1 | grep -c 'mcp_guard: refused by') diff --git a/src/mcp/guard/component_mcp_guard.cc b/src/mcp/guard/component_mcp_guard.cc index 2bfb2c9..5c364ec 100644 --- a/src/mcp/guard/component_mcp_guard.cc +++ b/src/mcp/guard/component_mcp_guard.cc @@ -335,18 +335,25 @@ SessionState *session_state(MYSQL_THD thd) { bool is_mcp_session(MYSQL_THD thd) { return session_state(thd) != nullptr; } -void mark_session(MYSQL_THD thd, bool mcp) { +// Returns true on failure. An MCP session that cannot be marked would not be guarded, so +// the caller refuses it. +bool mark_session(MYSQL_THD thd, bool mcp) { + if (session_slot == nullptr) return mcp; SessionState *state = session_state(thd); - if ((state != nullptr) == mcp) return; + if ((state != nullptr) == mcp) return false; if (mcp) { state = new (std::nothrow) SessionState(); - if (state == nullptr) return; - mysql_service_mysql_thd_store->set(thd, session_slot, state); + if (state == nullptr) return true; + if (mysql_service_mysql_thd_store->set(thd, session_slot, state)) { + delete state; + return true; + } } else { mysql_service_mysql_thd_store->set(thd, session_slot, nullptr); delete state; } sessions_connected += mcp ? 1 : -1; + return false; } // Writes a refusal to the error log, at most kLogBurst lines per minute; the number left @@ -443,7 +450,9 @@ bool Event_tracking_connection_implementation::callback( mark_session(thd, false); // refused: no disconnect event follows return deny("mcp.enabled", statements_denied, data->connection_id, "connection refused"); } - mark_session(thd, mcp); + if (mark_session(thd, mcp)) // could not mark an MCP session: fail closed + return deny("mcp_guard", statements_denied, data->connection_id, + "connection refused: session state unavailable"); if (mcp) apply_time_limit(thd, session_state(thd)); return false; } diff --git a/src/mcp/guard/mcp_guard_policy.h b/src/mcp/guard/mcp_guard_policy.h index 96d7b50..9e6b05b 100644 --- a/src/mcp/guard/mcp_guard_policy.h +++ b/src/mcp/guard/mcp_guard_policy.h @@ -298,9 +298,9 @@ inline const char *time_limit_violation(std::string_view cmd, const char *query, // ----- Error log ----------------------------------------------------------------------- -// The statement text for the error log: string literals replaced by '?' (they can hold -// secrets), line breaks and tabs by spaces, at most max_len bytes. Quoted identifiers stay. -inline std::string redact_for_log(const char *s, size_t n, size_t max_len = 256) { +// One reading of redact_for_log(): backslash_escapes as in tokenize(). +inline std::string redact_literals(const char *s, size_t n, size_t max_len, + bool backslash_escapes) { std::string out; size_t i = 0; while (i < n && out.size() < max_len) { @@ -311,7 +311,7 @@ inline std::string redact_for_log(const char *s, size_t n, size_t max_len = 256) out.push_back(c); ++i; while (i < n) { - if (s[i] == '\\' && i + 1 < n) { + if (backslash_escapes && s[i] == '\\' && i + 1 < n) { i += 2; } else if (s[i] == c && i + 1 < n && s[i + 1] == c) { i += 2; @@ -331,6 +331,19 @@ inline std::string redact_for_log(const char *s, size_t n, size_t max_len = 256) return out; } +// The statement text for the error log: string literals replaced by '?' (they can hold +// secrets), line breaks and tabs by spaces, at most max_len bytes. Quoted identifiers stay. +// Where a literal ends depends on sql_mode (NO_BACKSLASH_ESCAPES), so both readings are +// made; if they differ, only the text before the first quote is kept. +inline std::string redact_for_log(const char *s, size_t n, size_t max_len = 256) { + std::string with = redact_literals(s, n, max_len, true); + if (with == redact_literals(s, n, max_len, false)) return with; + size_t q = 0; + while (q < n && q < max_len && s[q] != '\'' && s[q] != '"') ++q; + std::string out = redact_literals(s, q, max_len, true); + return out + "'?'..."; +} + // Parses mcp.allowed_schemas: comma-separated schema names, spaces trimmed, empty // entries dropped. An empty list means every schema. inline std::vector parse_schema_list(std::string_view v) { diff --git a/src/mcp/guard/test_mcp_guard_policy.cc b/src/mcp/guard/test_mcp_guard_policy.cc index 7c9c951..ff2f16c 100644 --- a/src/mcp/guard/test_mcp_guard_policy.cc +++ b/src/mcp/guard/test_mcp_guard_policy.cc @@ -147,6 +147,19 @@ int main() { std::printf("FAIL redact_for_log: %s\n", r.c_str()); ++failures; } + // NO_BACKSLASH_ESCAPES: 'a\' is a whole literal there; the readings differ, so only + // the text before the first quote is kept. + const char *nbe = "SELECT 'a\\', 'token=hunter2' FROM t FOR UPDATE"; + const std::string rn = redact_for_log(nbe, std::strlen(nbe)); + if (rn.find("hunter2") != std::string::npos || rn != "SELECT '?'...") { + std::printf("FAIL redact_for_log NO_BACKSLASH_ESCAPES: %s\n", rn.c_str()); + ++failures; + } + const char *esc = "SELECT 'it\\'s secret' FROM t"; + if (redact_for_log(esc, std::strlen(esc)).find("secret") != std::string::npos) { + std::printf("FAIL redact_for_log escaped quote: %s\n", redact_for_log(esc, std::strlen(esc)).c_str()); + ++failures; + } if (redact_for_log(q, std::strlen(q), 10) != "SELECT '?'...") { std::printf("FAIL redact_for_log truncation: %s\n", redact_for_log(q, std::strlen(q), 10).c_str()); ++failures; From c20da6d0ecc09285f5d5701d313b368a18c1ff81 Mon Sep 17 00:00:00 2001 From: Alkin Tezuysal Date: Fri, 30 Oct 2026 14:47:54 +0000 Subject: [PATCH 4/6] fix(mcp): reapply the time limit after COM_CHANGE_USER MySQL resets session variables on change-user, but an MCP-to-MCP change kept the cached applied limit, so it was not set again. The cache is cleared on change-user (and starts as 'not applied'). Verified with mysql-connector-python cmd_change_user against 9.7.2: the session keeps the 1000 ms limit (0 before this fix). Co-Authored-By: Claude Opus 5.5 --- src/mcp/guard/component_mcp_guard.cc | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/mcp/guard/component_mcp_guard.cc b/src/mcp/guard/component_mcp_guard.cc index 5c364ec..4ec513f 100644 --- a/src/mcp/guard/component_mcp_guard.cc +++ b/src/mcp/guard/component_mcp_guard.cc @@ -316,8 +316,11 @@ bool status_registered = false; // ----- MCP session marker (per THD) --------------------------------------------------- // Per-session state of an MCP session, in a THD slot (null for other sessions). +// applied_time_limit when no limit has been applied to the current session variables. +constexpr unsigned long kTimeLimitNotApplied = ~0UL; + struct SessionState { - unsigned long applied_time_limit = 0; // mcp.max_execution_time_ms last applied + unsigned long applied_time_limit = kTimeLimitNotApplied; // last mcp.max_execution_time_ms set }; mysql_thd_store_slot session_slot = nullptr; @@ -453,7 +456,13 @@ bool Event_tracking_connection_implementation::callback( if (mark_session(thd, mcp)) // could not mark an MCP session: fail closed return deny("mcp_guard", statements_denied, data->connection_id, "connection refused: session state unavailable"); - if (mcp) apply_time_limit(thd, session_state(thd)); + if (mcp) { + SessionState *state = session_state(thd); + // COM_CHANGE_USER resets session variables: apply the limit again. + if (state != nullptr && data->event_subclass == EVENT_TRACKING_CONNECTION_CHANGE_USER) + state->applied_time_limit = kTimeLimitNotApplied; + apply_time_limit(thd, state); + } return false; } case EVENT_TRACKING_CONNECTION_DISCONNECT: From ac919105512202e1d9d6efb7eae77cde95b26f77 Mon Sep 17 00:00:00 2001 From: Alkin Tezuysal Date: Fri, 30 Oct 2026 15:58:24 +0000 Subject: [PATCH 5/6] fix(mcp): reapply the time limit after COM_RESET_CONNECTION A connection reset restores session variables from the globals but raises no connection event, so the cached applied limit stayed and the session ran without it. The guard now also tracks command events and clears the cache on a successful 'Reset Connection'. Verified with mysql-connector-python cmd_reset_connection against 9.7.2 (1000 ms kept; 0 before this fix); 80 checks pass on 8.4.11, 9.7.2, 26.10.0. Co-Authored-By: Claude Opus 5.5 --- docs/MCP_COMPONENTS.md | 2 +- src/mcp/guard/component_mcp_guard.cc | 22 +++++++++++++++++++++- 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/docs/MCP_COMPONENTS.md b/docs/MCP_COMPONENTS.md index 634025a..9aa6f80 100644 --- a/docs/MCP_COMPONENTS.md +++ b/docs/MCP_COMPONENTS.md @@ -73,7 +73,7 @@ When an MCP session connects, the guard sets its session `max_execution_time` to - `SET [SESSION] max_execution_time` (or `@@max_execution_time`) to 0, above the limit, or to anything but a literal (`DEFAULT`, an expression, a variable). - The optimizer hints `MAX_EXECUTION_TIME(n)` and `SET_VAR(max_execution_time = n)` with such a value. -A changed limit reaches connected MCP sessions at their next statement. Setting it back to 0 leaves their current session value as it is. +A changed limit reaches connected MCP sessions at their next statement, and the limit is applied again after `COM_CHANGE_USER` and `COM_RESET_CONNECTION`, which reset session variables. Setting it back to 0 leaves their current session value as it is. As with the server's own `max_execution_time`, the limit covers top-level read-only `SELECT` statements. MySQL ignores it inside stored programs, so **`CALL mysql_mcp.*` tool-pack calls are not limited**. Those run catalog queries on `information_schema`. If they must be limited too, revoke `EXECUTE` on `mysql_mcp` from the MCP account: mysql-mcp-server then falls back to its own SQL, whose `SELECT`s the limit covers. diff --git a/src/mcp/guard/component_mcp_guard.cc b/src/mcp/guard/component_mcp_guard.cc index 4ec513f..fc384a2 100644 --- a/src/mcp/guard/component_mcp_guard.cc +++ b/src/mcp/guard/component_mcp_guard.cc @@ -44,6 +44,7 @@ #include #include #include +#include #include #include #include @@ -473,6 +474,23 @@ bool Event_tracking_connection_implementation::callback( } } +// Command events: COM_RESET_CONNECTION resets session variables (raising no connection +// event), so the time limit is applied again at the next statement. +mysql_event_tracking_command_subclass_t Event_tracking_command_implementation::filtered_sub_events = + EVENT_TRACKING_COMMAND_START; + +bool Event_tracking_command_implementation::callback( + const mysql_event_tracking_command_data *data) { + if (data->status != 0) return false; + SessionState *state = session_state(current_thd_or_null()); + if (state == nullptr) return false; + static constexpr std::string_view kReset = "Reset Connection"; + const std::string_view command(data->command.str ? data->command.str : "", + data->command.str ? data->command.length : 0); + if (command == kReset) state->applied_time_limit = kTimeLimitNotApplied; + return false; +} + // Global variable events: SET GLOBAL / SET PERSIST of mcp.* needs MCP_ADMIN, on top of // the SYSTEM_VARIABLES_ADMIN the server requires. Only a client's SET statement // (sql_command "set_option") is checked: the server applies persisted values through the @@ -611,13 +629,15 @@ mysql_service_status_t mcp_guard_init() { } // namespace +IMPLEMENTS_SERVICE_EVENT_TRACKING_COMMAND(mcp_guard); IMPLEMENTS_SERVICE_EVENT_TRACKING_CONNECTION(mcp_guard); IMPLEMENTS_SERVICE_EVENT_TRACKING_GLOBAL_VARIABLE(mcp_guard); IMPLEMENTS_SERVICE_EVENT_TRACKING_QUERY(mcp_guard); IMPLEMENTS_SERVICE_EVENT_TRACKING_TABLE_ACCESS(mcp_guard); BEGIN_COMPONENT_PROVIDES(mcp_guard) -PROVIDES_SERVICE_EVENT_TRACKING_CONNECTION(mcp_guard), +PROVIDES_SERVICE_EVENT_TRACKING_COMMAND(mcp_guard), + PROVIDES_SERVICE_EVENT_TRACKING_CONNECTION(mcp_guard), PROVIDES_SERVICE_EVENT_TRACKING_GLOBAL_VARIABLE(mcp_guard), PROVIDES_SERVICE_EVENT_TRACKING_QUERY(mcp_guard), PROVIDES_SERVICE_EVENT_TRACKING_TABLE_ACCESS(mcp_guard), END_COMPONENT_PROVIDES(); From ba8944f5b43eddd5bdff51d212d118fe1c6c59eb Mon Sep 17 00:00:00 2001 From: Alkin Tezuysal Date: Fri, 30 Oct 2026 17:15:10 +0000 Subject: [PATCH 6/6] perf(mcp): check for Reset Connection before the session lookup Co-Authored-By: Claude Opus 5.5 --- src/mcp/guard/component_mcp_guard.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/mcp/guard/component_mcp_guard.cc b/src/mcp/guard/component_mcp_guard.cc index fc384a2..0806de4 100644 --- a/src/mcp/guard/component_mcp_guard.cc +++ b/src/mcp/guard/component_mcp_guard.cc @@ -482,12 +482,12 @@ mysql_event_tracking_command_subclass_t Event_tracking_command_implementation::f bool Event_tracking_command_implementation::callback( const mysql_event_tracking_command_data *data) { if (data->status != 0) return false; - SessionState *state = session_state(current_thd_or_null()); - if (state == nullptr) return false; static constexpr std::string_view kReset = "Reset Connection"; const std::string_view command(data->command.str ? data->command.str : "", data->command.str ? data->command.length : 0); - if (command == kReset) state->applied_time_limit = kTimeLimitNotApplied; + if (command != kReset) return false; // cheap check first: command events are frequent + SessionState *state = session_state(current_thd_or_null()); + if (state != nullptr) state->applied_time_limit = kTimeLimitNotApplied; return false; }