Admin: strip leading SQL block comments before connect-setup matchers (#5786)

[takaidohigasi]

Fixes #5786.

What

The two connect-setup statement accept blocks in admin_session_handler —
SET SQL_SAFE_UPDATES=1 (fix bug #442) and the BEGIN/COMMIT/ROLLBACK/
SET NAMES/SET AUTOCOMMIT/... block (fix bug #1047) — use strncmp /
strncasecmp prefix matching against the start of query_no_space.

Clients that prepend SQL tracing comments to every query — SQLCommenter,
Datadog Agent's mysql integration CommenterCursor, Sequelize, Hibernate,
etc. — defeat the prefix match. The query falls through to the SQLite
engine, which rejects it with:

ERROR 1045 (28000): ProxySQL Admin Error: near "SET": syntax error

The most visible manifestation is Datadog Agent autodiscovery on the
admin port failing during connection setup, before any custom_queries
run.

How

Add a small helper skip_leading_sql_comments that returns a pointer past
leading whitespace and /* … */ block comments. Apply it once to derive a
local pointer used for matching in both accept blocks. The original query
buffer is unchanged, so audit / digest / error-echo paths (i.e. the
query_no_space passed to send_ok_msg_to_client) see the original text.

const char* mb = skip_leading_sql_comments(query_no_space, query_no_space_length);

// fix bug #442
if (!strncmp("SET SQL_SAFE_UPDATES=1", mb, strlen("SET SQL_SAFE_UPDATES=1"))) { … }

// fix bug #1047
if (
    (!strncasecmp("BEGIN",          mb, strlen("BEGIN")))
    || (!strncasecmp("SET AUTOCOMMIT", mb, strlen("SET AUTOCOMMIT")))
    || ...
) { … }

Block-comments-only scope (deliberate)

Only /* … */ block comments are handled. MySQL -- / # line comments
can't be parsed safely in this code path: remove_spaces() runs before
this matching and collapses \n / \r to a single space, so the
line-comment terminator no longer exists in the buffer. Block-comment
recognition is unaffected because */ survives remove_spaces intact —
and block comments are the form SQL-commenter clients (the originally
affected use case) actually emit.

Out of scope for this PR

• Token-boundary tightening of the existing prefix matchers (e.g.
  strncasecmp("BEGIN", …, 5) currently accepts BEGINXYZ). Pre-existing,
  worth its own PR.
• /*! version-conditional */ and /*+ optimizer hint */ comments. The
  current behaviour is preserved: ADMIN-session inputs of /*!… */
  continue to be handled by the mysqldump-compat accept block at
  Admin_Handler.cpp:~2790 (which fires before this code). For STATS
  sessions, /*!… */-only inputs continue to fall through to SQLite as
  before — the helper consumes the comment, the prefix match fails on the
  empty remainder, and dispatch is unchanged.

Testing

Added a TAP regression test
test/tap/tests/mysql-reg_test_5786_admin_strip_leading_sql_comments-t.cpp
(registered in test/tap/groups/groups.json) that, against a live ProxySQL
admin port:

• For each of the connect-setup statements covered by the two accept blocks
  (SET AUTOCOMMIT, SET NAMES, SET character_set_results,
  SET SQL_AUTO_IS_NULL, SET SQL_SAFE_UPDATES, BEGIN,
  START TRANSACTION, COMMIT, ROLLBACK), exercises the bare form, the
  leading block-comment form, the exact Datadog Agent CommenterCursor
  payload (/*service='datadog-agent'*/ SET AUTOCOMMIT=1), stacked
  comments, and leading+trailing comments. Each accept case asserts
  rc == 0 AND that no result set was returned
  (mysql_field_count() == 0) — the OK-packet path send_ok_msg_to_client
  produces.
• A negative case /* SET AUTOCOMMIT=1 inside comment */ SELECT 1 asserts
  that the SQLite engine actually executes SELECT 1 (result set present,
  first row value is "1"). A false-positive accept would produce an OK
  packet with no result set, so the row assertion would fail.

Local lints pass:

$ python3 test/tap/groups/check_groups.py --source
(no errors)
$ python3 test/tap/groups/lint_groups_json.py
(no errors)
$ git show --check --stat HEAD
(no whitespace issues)

Backwards compatibility

No client-visible behaviour changes for callers that don't prefix queries
with block comments. Callers that DO prefix block comments now get the
intended no-op accept instead of ER_PARSE_ERROR (1045), on both admin
and stats sessions (the existing accept blocks already cover both).

Summary by CodeRabbit

• Bug Fixes

  • Admin interface correctly recognizes and handles transaction/control commands and SQL safety settings even when queries include leading SQL block comments.

• New Features

  • Admin now transparently accepts queries prefixed with tracing-style SQL block comments.

• Tests

  • Added a regression test and test-group entry validating admin handling of comment-prefixed queries.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: a9e04980-6ef1-424e-9155-adf084dc9bea

📥 Commits

Reviewing files that changed from the base of the PR and between 2a5fcb0 and 79dcbef.

📒 Files selected for processing (3)

• lib/Admin_Handler.cpp
• test/tap/groups/groups.json
• test/tap/tests/mysql-reg_test_5786_admin_strip_leading_sql_comments-t.cpp

🚧 Files skipped from review as they are similar to previous changes (3)

• test/tap/groups/groups.json
• lib/Admin_Handler.cpp
• test/tap/tests/mysql-reg_test_5786_admin_strip_leading_sql_comments-t.cpp

---

📝 Walkthrough

Walkthrough

Adds a SQL-comment-stripping utility to ProxySQL's Admin handler, uses the stripped pointer for early command accept-match checks (e.g., SET AUTOCOMMIT, BEGIN/COMMIT), preserves the original query for audit/digest, and adds a TAP regression test plus test-group mapping.

Changes

Admin SQL Comment Stripping

Layer / File(s)	Summary
SQL comment-stripping utility function lib/Admin_Handler.cpp	Introduces internal helper skip_leading_sql_comments() that scans past leading SQL block comments (/* ... */) and whitespace up to a bounded length, returning a pointer to the first non-comment token without mutating the buffer.
Admin command matching with comment stripping lib/Admin_Handler.cpp	Updates admin_session_handler() early command-interception logic to compare against the comment-stripped pointer for commands like SET SQL_SAFE_UPDATES, BEGIN, COMMIT, ROLLBACK, SET NAMES, SET AUTOCOMMIT, etc., while preserving the original query string for audit and digest payloads.
Regression test and group mapping test/tap/tests/mysql-reg_test_5786_admin_strip_leading_sql_comments-t.cpp, test/tap/groups/groups.json	Adds a TAP regression test exercising positive cases (comment-prefixed connect-setup statements silently accepted with no result set) and a negative case, plus a test-group JSON entry for the new test.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested reviewers

• renecannao

Poem

A rabbit trims the comment vine,
Nudges SET and BEGIN in line,
Leading blocks now swept away,
Admin greets the query's play,
Hops on—tests pass, fields okay. 🐇

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely summarizes the main change: adding logic to strip leading SQL block comments before connect-setup matchers in the Admin handler to fix issue #5786.
Linked Issues check	✅ Passed	The PR implementation aligns with the primary objectives from #5786: it introduces skip_leading_sql_comments() helper, applies it to SET SQL_SAFE_UPDATES and BEGIN/COMMIT/ROLLBACK/SET NAMES/SET AUTOCOMMIT matching, preserves original query for audit/digest, and includes regression tests.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to the primary objective: Admin_Handler.cpp modifications target the two connect-setup accept blocks mentioned in #5786, test file additions validate the fix, and the groups.json entry simply registers the new test.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[gemini-code-assist]

Code Review

This pull request introduces several configuration and repository metadata files, including a sample .aider.conf.yml configuration, a .clang-tidy file, a Contributor License Agreement (.github/CLA.md), and a bug report issue template. A review comment points out that the load option in .aider.conf.yml references a non-existent ./AGENT.md file, which will cause startup errors, and suggests commenting it out.

[gemini-code-assist]

The load option is set to ./AGENT.md, but there is no AGENT.md file in this repository. This will cause aider to fail or display an error on startup for anyone running it in this project. Consider commenting this line out to match the rest of the template configuration.

#load: ./AGENT.md

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[takaidohigasi]

(reviewed by codex gpt 5.5 high effort)