From 831bab9a725c379d27f37a5a758ad67471f10f87 Mon Sep 17 00:00:00 2001 From: Alkin Tezuysal Date: Sun, 1 Nov 2026 16:00:08 +0000 Subject: [PATCH] fix(mcp-guard): refuse table reads without table access events Found reviewing the code behind #247. The server raises table access events only for SELECT, data changes, ANALYZE and HANDLER READ, so the schema rules (mcp.allowed_schemas, mcp.deny_system_schemas) did not see: - a subquery in SET or DO: SET @x = (SELECT ... FROM mysql.user) returned the data, also through PREPARE/EXECUTE; - a subquery in SHOW ... WHERE or a CALL argument (a yes/no oracle); - CHECK TABLE / CHECKSUM TABLE. Under the schema rules these are now refused (hides_table_reads, scanned in both quoting modes). System schema names compare case-insensitively under lower_case_table_names 1 or 2. Tests: policy unit cases; test-mcp-guard.sh 110 checks on 8.4.11, 9.7.2, 26.10.0 (the unfixed guard fails all 8 new ones); 3 parity rows. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 6 ++++ docs/MCP_COMPONENTS.md | 4 ++- scripts/test-mcp-guard.sh | 9 ++++++ sql/mcp/guard-parity.tsv | 3 ++ src/mcp/guard/component_mcp_guard.cc | 7 +++++ src/mcp/guard/mcp_guard_policy.h | 33 ++++++++++++++++++-- src/mcp/guard/test_mcp_guard_policy.cc | 42 ++++++++++++++++++++++++++ 7 files changed, 100 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c2ed9b09..183e1fd4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -135,6 +135,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 and rebuild the index. See [Declaring a Vector Column](docs/usage.md#declaring-a-vector-column). ### Fixed +- **`component_mcp_guard`: table reads the schema rules could not see** (found reviewing the code behind #247). The server raises table access events only for `SELECT`, data changes, `ANALYZE` and `HANDLER READ`. An MCP session could therefore read any table, `mysql.user` included, past `mcp.allowed_schemas` and `mcp.deny_system_schemas`: + - directly, with a subquery in `SET` or `DO` (`SET @x = (SELECT … FROM mysql.user); SELECT @x`), also through `PREPARE`/`EXECUTE`; + - as a yes/no oracle, with a subquery in `SHOW … WHERE` or in a `CALL` argument; + - checksums only, with `CHECKSUM TABLE` / `CHECK TABLE`. + + These statements are now refused while a schema rule is in effect. System schema names compare case-insensitively under `lower_case_table_names` 1 or 2. `scripts/test-mcp-guard.sh` has 110 checks. The unfixed guard fails all 8 new ones. Three parity rows were added. - **Post-merge review of #244, #245, #246, #248** (#249; these were merged after rate-limited, not completed, CodeRabbit runs). Fixes: - **`mysql-mcp` image:** the settings are checked by an entrypoint wrapper before the data directory is initialized. `MYSQL_MCP_USER` equal to `MYSQL_USER` (or the user in `MYSQL_USER_FILE`) is refused. So is a `MYSQL_USER` with a backslash or quote, which the official entrypoint passes to SQL unescaped (`app\q` creates `appq`), or with leading or trailing spaces, which MySQL trims. The checks run only before initialization, so an initialized volume keeps starting. `MYSQL_MCP_USER` must be a plain name. Before, `CREATE USER` failed in first-start setup and a restart skipped the setup. The health check now asks mysqld for its port (`mysqld --verbose --help`, with a throwaway `--log-bin-index` as the official entrypoint uses), so option files, option groups, `--defaults-file` and `--no-defaults` apply exactly as for the server. It reports not ready while first-start setup runs. - **`scripts/build-docker-local.sh`:** builds 9.7 with GCC 11 (9.7.4 needs 11+), 8.0/8.4 with GCC 10, as `docker-publish.yml` does. diff --git a/docs/MCP_COMPONENTS.md b/docs/MCP_COMPONENTS.md index 03bd6d9b..8d9feb42 100644 --- a/docs/MCP_COMPONENTS.md +++ b/docs/MCP_COMPONENTS.md @@ -169,9 +169,11 @@ The option is `mcp.read_only`, `mcp.deny_system_schemas`, `mcp.allowed_schemas` - The schema rules apply to a statement's **base tables**. A view in an allowed schema over a table in another schema (or in `mysql`) is denied, nested views included. A view in another schema whose base tables are all allowed is allowed. - **Stored functions and triggers: their reads are not checked by the schema rules.** Their statements run on tables the calling statement has already locked, so the server raises no table access event for them. For example, with `mcp.allowed_schemas = 'd1'`, `SELECT d1.f()` returns data from `d2` if `d1.f()` reads `d2`. Writes inside them are still refused by `mcp.read_only` (a nested statement check), and stored procedures are not affected (`CALL` is limited to `mysql_mcp` under `mcp.read_only`, and a procedure's statements raise their own events). Control this with grants: don't give MCP accounts `EXECUTE` on schemas with functions that read data they must not see. A `SQL SECURITY INVOKER` function runs with the account's own grants, but `mcp.allowed_schemas` doesn't limit it. Triggers need no `EXECUTE`: they run, with their definer's privileges, when a statement writes their table. Under `mcp.read_only` (the default) MCP sessions cannot write, so no trigger runs for them. If you turn `mcp.read_only` off, don't give MCP accounts write grants on tables whose triggers read data outside the account's allowed schemas, and review trigger definers. `scripts/test-mcp-guard.sh` keeps a check that shows this, so a server change is noticed ([#247](https://github.com/askdba/myvector/issues/247)). - A multi-statement packet is checked statement by statement: each statement in it raises its own events. +- **Statements whose table reads raise no table access events are refused** while `mcp.deny_system_schemas` is on (the default) or `mcp.allowed_schemas` is set. The server raises table access events only for `SELECT`, data changes, `ANALYZE` and `HANDLER READ`, so these statements would otherwise read any table unseen: a subquery in `SET`, `DO`, a `CALL` argument or `SHOW ... WHERE`, and `CHECK TABLE` / `CHECKSUM TABLE`. Read through `SELECT` instead (`SELECT ... INTO @x`). The refusal names the schema rule in effect. `SET`, `SHOW` and `CALL` without subqueries are unaffected. +- With `lower_case_table_names` 1 or 2, the system schema names are compared case-insensitively, as the server compares them. ### Tests -`scripts/test-mcp-guard.sh ` runs install, restart, persistence, uninstall and the enforcement cases, the tool-call table and its labels, views, multi-statement packets and the parity table below against the official image (102 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, the tool-call table and its labels, views, multi-statement packets, statements without table access events and the parity table below against the official image (110 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. **Parity with mysql-mcp-server** (RFC 0001 §5.2 item 7): [`sql/mcp/guard-parity.tsv`](https://github.com/askdba/myvector/blob/main/sql/mcp/guard-parity.tsv) lists statements a `run_query` call might send, with the verdict of mysql-mcp-server's validator (`go`) and of the guard under the default settings (`guard`). Every row where they differ says why. The test checks the guard column, sending each statement as one packet. mysql-mcp-server vendors the file and checks the go column. Both reject every write and locking read. The guard allows a few things the validator does not (session `SET`, `information_schema`, `CALL mysql_mcp.*`) and catches what text checks cannot (a write inside a function called from `SELECT`). diff --git a/scripts/test-mcp-guard.sh b/scripts/test-mcp-guard.sh index c25842b2..94448783 100755 --- a/scripts/test-mcp-guard.sh +++ b/scripts/test-mcp-guard.sh @@ -158,7 +158,16 @@ denied "nested view over another schema" "$(mcp "SELECT * FROM d1.v_nested")" "m expect "view in another schema over allowed tables (rules apply to base tables)" "$(mcp "SELECT COUNT(*) FROM d2.v_d1")" "^[0-9]+$" denied "multi-statement packet: disallowed read in the second statement" "$(packet "SELECT 1; SELECT * FROM d2.t")" "mcp.allowed_schemas" expect "KNOWN LIMITATION: a stored function's reads are not seen by the schema rules" "$(mcp "SELECT d1.f_read_d2()")" "^42$" +# Statements whose table reads raise no table access events are refused under the rules. +denied "SET with a subquery over another schema" "$(mcp "SET @x = (SELECT MAX(i) FROM d2.t); SELECT @x")" "mcp.allowed_schemas" +denied "DO with a subquery" "$(mcp "DO @x := (SELECT MAX(i) FROM d2.t)")" "mcp.allowed_schemas" +denied "SHOW ... WHERE with a subquery" "$(mcp "SHOW DATABASES WHERE (SELECT MAX(i) FROM d2.t) = 42")" "mcp.allowed_schemas" +denied "CALL mysql_mcp with a subquery argument" "$(mcp "CALL mysql_mcp.list_tables((SELECT 'd2'))")" "mcp.allowed_schemas" +denied "SET with a subquery through PREPARE/EXECUTE" "$(mcp "PREPARE s FROM 'SET @p = (SELECT MAX(i) FROM d2.t)'; EXECUTE s; SELECT @p")" "mcp.allowed_schemas" +denied "CHECKSUM TABLE" "$(mcp "CHECKSUM TABLE d2.t")" "mcp.allowed_schemas" +expect "SET, SHOW and CALL without subqueries still work" "$(mcp "SET @x = 1; SHOW TABLES FROM d1 LIKE 't'; CALL mysql_mcp.version()" | tr '\n' ' ')" "^t [0-9]+\.[0-9]+\.[0-9]+ $" sql "SET GLOBAL mcp.allowed_schemas = ''" >/dev/null +denied "SET with a subquery over mysql (deny_system_schemas)" "$(mcp "SET @x = (SELECT COUNT(*) FROM mysql.user); SELECT @x")" "mcp.deny_system_schemas" denied "multi-statement packet: write in the second statement" "$(packet "SELECT 1; INSERT INTO d1.t VALUES (5)")" "mcp.read_only" denied "multi-statement packet: write after a session SET" "$(packet "SET @a = 1; DELETE FROM d1.t")" "mcp.read_only" expect "multi-statement packets wrote nothing" "$(sql "SELECT GROUP_CONCAT(i ORDER BY i) FROM d1.t")" "^1$" diff --git a/sql/mcp/guard-parity.tsv b/sql/mcp/guard-parity.tsv index 21af34a9..e1aef367 100644 --- a/sql/mcp/guard-parity.tsv +++ b/sql/mcp/guard-parity.tsv @@ -40,3 +40,6 @@ deny allow SET SESSION sql_mode = '' go accepts only reads; the guard allows ses deny allow CALL mysql_mcp.version() go runs the tool pack itself, not through run_query; the guard allows CALL of mysql_mcp routines deny allow SELECT SLEEP(0) go blocks SLEEP; the guard bounds run time with mcp.max_execution_time_ms instead allow deny SELECT d1.f_write() go cannot see what a stored function does; the guard refuses the write inside it +deny deny SET @x = (SELECT MAX(i) FROM d1.t) go accepts only reads; guard: a subquery in SET reads tables without table access events, so it is refused under the schema rules +allow deny SHOW TABLES FROM d1 WHERE (SELECT COUNT(*) FROM d1.t) > 0 go checks schema names in the text; the guard cannot see a subquery's tables in SHOW, so it refuses it under the schema rules +deny deny CHECKSUM TABLE d1.t go accepts only reads; guard: CHECKSUM TABLE reads without table access events diff --git a/src/mcp/guard/component_mcp_guard.cc b/src/mcp/guard/component_mcp_guard.cc index 6106065c..cee1c911 100644 --- a/src/mcp/guard/component_mcp_guard.cc +++ b/src/mcp/guard/component_mcp_guard.cc @@ -865,6 +865,13 @@ bool Event_tracking_query_implementation::callback(const mysql_event_tracking_qu const char *why = mcp_guard::read_only_violation(cmd, query, len); if (why != nullptr) return refuse(why); } + // Table reads the schema rules cannot see (no table access event): refused while a rule + // is in effect. + if (opt_deny_system_schemas || current_allowed_schemas()->restrict) { + if (mcp_guard::hides_table_reads(cmd, query, len)) + return refuse(current_allowed_schemas()->restrict ? "mcp.allowed_schemas" + : "mcp.deny_system_schemas"); + } const char *why = mcp_guard::time_limit_violation(cmd, query, len, opt_max_execution_time_ms); if (why != nullptr) return refuse(why); return false; diff --git a/src/mcp/guard/mcp_guard_policy.h b/src/mcp/guard/mcp_guard_policy.h index e3f825a1..80b27192 100644 --- a/src/mcp/guard/mcp_guard_policy.h +++ b/src/mcp/guard/mcp_guard_policy.h @@ -198,6 +198,30 @@ inline bool read_only_command(std::string_view cmd) { return false; } +// A subquery: every subquery is parenthesized, "(" followed by SELECT, TABLE, VALUES or WITH. +inline bool has_subquery(const std::vector &t) { + for (size_t i = 0; i + 1 < t.size(); ++i) + if (t[i].kind == Token::kPunct && t[i].text == "(" && + (word_is(t, i + 1, "SELECT") || word_is(t, i + 1, "TABLE") || + word_is(t, i + 1, "VALUES") || word_is(t, i + 1, "WITH"))) + return true; + return false; +} + +// Statements that read tables without the table access events the schema rules +// (mcp.allowed_schemas, mcp.deny_system_schemas) are enforced on: the server raises them +// only for SELECT, data changes, ANALYZE and HANDLER READ. So a subquery in SET, DO, a CALL +// argument or SHOW ... WHERE reads any table unseen, as do CHECK TABLE and CHECKSUM TABLE. +// Under the schema rules such statements are refused (scanned in both quoting modes). +inline bool hides_table_reads(std::string_view cmd, const char *query, size_t len) { + if (cmd == "check" || cmd == "checksum") return true; + if (cmd != "set_option" && cmd != "do" && cmd != "call" && cmd.substr(0, 5) != "show_") + return false; + for (bool escapes : {true, false}) + if (has_subquery(tokenize(query, len, escapes))) return true; + return false; +} + // Returns the option that denies the statement under mcp.read_only, or nullptr. // Where a quoted string ends depends on sql_mode (NO_BACKSLASH_ESCAPES, ANSI_QUOTES), which // a session may change. The text is therefore scanned both with and without backslash @@ -415,13 +439,16 @@ inline bool is_system_schema(std::string_view db) { // write is true for insert, update and delete accesses. inline const char *table_violation(const TablePolicy &p, std::string_view db, bool write) { if (write && p.read_only) return "mcp.read_only"; - if (is_system_schema(db)) { + // With lower_case_table_names 1 or 2 schema names compare case-insensitively (MYSQL is + // mysql); with 0 they are distinct schemas. + const auto is = [&](std::string_view name) { return p.case_insensitive_names ? ieq(db, name) : db == name; }; + if (is("mysql") || is("sys") || is("information_schema") || is("performance_schema")) { // information_schema and performance_schema stay readable: the tools use them, and // grants govern them. mysql (account and grant tables) and sys are denied. - if (p.deny_system_schemas && (db == "mysql" || db == "sys")) return "mcp.deny_system_schemas"; + if (p.deny_system_schemas && (is("mysql") || is("sys"))) return "mcp.deny_system_schemas"; return nullptr; } - if (db == "mysql_mcp") return nullptr; + if (is("mysql_mcp")) return nullptr; if (p.restrict_schemas) { if (p.allowed_schemas) for (const std::string &s : *p.allowed_schemas) diff --git a/src/mcp/guard/test_mcp_guard_policy.cc b/src/mcp/guard/test_mcp_guard_policy.cc index 70d498a5..e26bc46e 100644 --- a/src/mcp/guard/test_mcp_guard_policy.cc +++ b/src/mcp/guard/test_mcp_guard_policy.cc @@ -38,6 +38,13 @@ static void expect_table(const TablePolicy &p, const char *db, bool write, const } } +static void expect_hidden(const char *cmd, const char *sql, bool hidden) { + if (hides_table_reads(cmd, sql, std::strlen(sql)) != hidden) { + std::printf("FAIL hidden %-12s %-60s want %s\n", cmd, sql, hidden ? "refuse" : "allow"); + ++failures; + } +} + int main() { // Allowed reads. expect_ro("select", "SELECT * FROM t WHERE note = 'FOR UPDATE'", false); @@ -229,6 +236,41 @@ int main() { expect_table(p, "mysql", false, nullptr); expect_table(p, "shop", true, nullptr); expect_table(p, "hr", true, "mcp.allowed_schemas"); + // System schema names compare as the server does: MYSQL is mysql only with + // lower_case_table_names 1 or 2. + p.deny_system_schemas = true; + p.restrict_schemas = false; + expect_table(p, "MYSQL", false, nullptr); // lower_case_table_names = 0: another schema + p.case_insensitive_names = true; + expect_table(p, "MYSQL", false, "mcp.deny_system_schemas"); + expect_table(p, "Sys", false, "mcp.deny_system_schemas"); + expect_table(p, "Information_Schema", false, nullptr); + p.case_insensitive_names = false; + + // Table reads without table access events (subqueries in SET, DO, CALL, SHOW; CHECK and + // CHECKSUM TABLE) are refused under the schema rules. + expect_hidden("set_option", "SET @x = (SELECT MAX(i) FROM d2.t)", true); + expect_hidden("set_option", "SET @a = 1, @b = (SELECT 1 FROM mysql.user)", true); + expect_hidden("set_option", "SET SESSION sql_select_limit = (TABLE d2.t)", true); + expect_hidden("set_option", "SET @x = ( WITH c AS (SELECT 1) SELECT * FROM c)", true); + expect_hidden("do", "DO @x := (SELECT MAX(i) FROM d2.t)", true); + expect_hidden("call", "CALL mysql_mcp.list_tables((SELECT 'd2'))", true); + expect_hidden("show_databases", "SHOW DATABASES WHERE (SELECT COUNT(*) FROM mysql.user) > 0", true); + expect_hidden("show_tables", "SHOW TABLES WHERE 1 IN (VALUES ROW(1))", true); + expect_hidden("checksum", "CHECKSUM TABLE d2.t", true); + expect_hidden("check", "CHECK TABLE d2.t", true); + expect_hidden("set_option", "SET @x = 1", false); + expect_hidden("set_option", "SET SESSION sql_mode = ''", false); + expect_hidden("set_option", "SET @x = '(SELECT 1)'", false); // a string + expect_hidden("set_option", "SET @x = 1 /* (SELECT 1) */", false); // a comment + expect_hidden("set_option", "SET @x = UPPER('a')", false); + expect_hidden("do", "DO SLEEP(0)", false); + expect_hidden("call", "CALL mysql_mcp.list_tables('d1')", false); + expect_hidden("show_tables", "SHOW TABLES FROM d1 LIKE 't%'", false); + expect_hidden("show_create_table", "SHOW CREATE TABLE d1.t", false); + expect_hidden("select", "SELECT (SELECT 1 FROM d2.t)", false); // SELECT raises table events + // A string that ends differently under NO_BACKSLASH_ESCAPES hides no subquery from the scan. + expect_hidden("set_option", "SET @x = '\\', @y = (SELECT 1) -- '", true); std::printf("%s (%d failures)\n", failures ? "FAILED" : "ok", failures); return failures ? 1 : 0;