Add PostgreSQL psql meta-command support to admin interface#5367
Conversation
Implement handling for psql meta-commands (\dt, \di, \dv, \d, \l) in ProxySQL's PostgreSQL admin interface by intercepting expanded queries and converting them to appropriate SQLite queries. Supported commands: - \l, \l+: List databases (maps to PRAGMA DATABASE_LIST) - \dt [pattern]: List tables with optional pattern matching - \di [pattern]: List indexes with optional pattern matching - \dv [pattern]: List views with optional pattern matching - \d: List all relations (tables, views, indexes, triggers) Pattern matching supports psql regex syntax like '\dt mytest*' by converting PostgreSQL regex patterns (^(pattern).*$) to SQLite LIKE patterns (pattern%).
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 enhances the usability of ProxySQL's PostgreSQL admin interface by enabling direct support for several common Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
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
|
📝 WalkthroughWalkthroughThis PR adds translation of PostgreSQL psql meta-commands (\dt, \di, \dv, \d, \l) in the admin interface by parsing psql patterns, converting a subset of regex constructs to SQLite LIKE, routing translated queries to sqlite_master, and adds TAP tests exercising these commands. Changes
Sequence DiagramsequenceDiagram
participant psql as psql Client
participant handler as Admin Handler
participant parser as Pattern Parser
participant converter as Regex Converter
participant sqlite as SQLite/sqlite_master
psql->>handler: Send meta-command (e.g., "\dt pattern*")
activate handler
handler->>handler: Detect psql meta-command
handler->>parser: extract_psql_pattern()
activate parser
parser-->>handler: Return parsed pattern
deactivate parser
handler->>converter: convert_regex_to_like()
activate converter
converter-->>handler: Return LIKE clause
deactivate converter
handler->>handler: Build SQLite query with LIKE filter
handler->>sqlite: Execute translated query
activate sqlite
sqlite-->>handler: Return metadata rows
deactivate sqlite
handler->>psql: Format and return results
deactivate handler
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for PostgreSQL psql meta-commands in the admin interface, which is a great enhancement for usability. The implementation involves intercepting specific SELECT queries from psql and translating them into corresponding SQLite queries against ProxySQL's internal database. The changes are well-structured with helper functions for pattern extraction and conversion.
My review has identified a critical buffer overflow vulnerability and a few other areas for improvement regarding code safety, readability, and testing. Addressing these points will make the new feature more robust and secure.
| else if (*src == '\'') { | ||
| // Escape single quotes for SQL safety (double them) | ||
| if (dst < dst_end - 1) { | ||
| *dst++ = '\''; | ||
| *dst++ = '\''; | ||
| } | ||
| src++; | ||
| } |
There was a problem hiding this comment.
The check if (dst < dst_end - 1) inside the else if (*src == '\'') block is redundant. The outer while loop condition dst < dst_end - 1 already guarantees that there is enough space for at least two characters (one for the current position and one for the null terminator). When escaping a single quote, two characters are written. The loop condition ensures this is safe. Removing the redundant inner check simplifies the code.
else if (*src == '\'') {
// Escape single quotes for SQL safety (double them)
*dst++ = '\'';
*dst++ = '\'';
src++;
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/tap/tests/pgsql_admin_metacmds-t.cpp`:
- Around line 38-48: The build_psql_cmd function is concatenating raw
admin_password, pgsql_admin_host, pgsql_admin_port, admin_username and meta_cmd
into a shell command which breaks on spaces or shell metacharacters; update
build_psql_cmd to shell-escape each interpolated value (admin_password,
pgsql_admin_host, pgsql_admin_port, admin_username and meta_cmd) before
concatenation—e.g. wrap values in single quotes and replace any internal single
quote with '\'' (or use a helper escape_shell_arg(const std::string&)), then use
those escaped strings when building the final command so the psql invocation is
safe.
🧹 Nitpick comments (1)
test/tap/tests/pgsql_admin_metacmds-t.cpp (1)
73-79: Pattern test doesn’t assert filtering; consider a negative-pattern check.The
\dt runtime*assertion (Line 73–79) will pass even if the pattern is ignored. A negative-pattern probe (that should return “No matching relations”) makes the test resilient to regressions.✅ Strengthen the pattern test (and update plan)
// Test \dt with pattern cmd = build_psql_cmd(cl, "\\dt runtime*"); output = exec(cmd.c_str()); has_output = output.find("List of relations") != std::string::npos || output.find("name") != std::string::npos; ok(has_output, "\\dt runtime* (tables with pattern): returned valid output"); + + // Negative pattern should yield no matches if filtering works + cmd = build_psql_cmd(cl, "\\dt __no_such_table__*"); + output = exec(cmd.c_str()); + bool no_match = output.find("No matching relations") != std::string::npos || + output.find("Did not find any relations") != std::string::npos; + ok(no_match, "\\dt __no_such_table__*: pattern filter applied");- plan(9); + plan(10);
| // Build psql connection string | ||
| std::string build_psql_cmd(const CommandLine& cl, const char* meta_cmd) { | ||
| std::stringstream ss; | ||
| ss << "PGPASSWORD=" << cl.admin_password << " "; | ||
| ss << "psql -h " << cl.pgsql_admin_host; | ||
| ss << " -p " << cl.pgsql_admin_port; | ||
| ss << " -U " << cl.admin_username; | ||
| ss << " -d postgres"; | ||
| ss << " -c \"" << meta_cmd << "\""; | ||
| ss << " 2>&1"; | ||
| return ss.str(); |
There was a problem hiding this comment.
Shell-escape connection parameters to avoid failures with special characters.
Right now Line 41–45 concatenate raw env values into a shell command; passwords/usernames containing spaces or shell metacharacters can break the test run. Consider simple shell-escaping for these fields.
🛠️ Suggested fix (shell-escape args)
+static std::string shell_escape(const std::string& s) {
+ std::string out = "'";
+ for (char c : s) {
+ if (c == '\'') out += "'\"'\"'";
+ else out += c;
+ }
+ out += "'";
+ return out;
+}
+
std::string build_psql_cmd(const CommandLine& cl, const char* meta_cmd) {
std::stringstream ss;
- ss << "PGPASSWORD=" << cl.admin_password << " ";
- ss << "psql -h " << cl.pgsql_admin_host;
+ ss << "PGPASSWORD=" << shell_escape(cl.admin_password) << " ";
+ ss << "psql -h " << shell_escape(cl.pgsql_admin_host);
ss << " -p " << cl.pgsql_admin_port;
- ss << " -U " << cl.admin_username;
+ ss << " -U " << shell_escape(cl.admin_username);
ss << " -d postgres";
ss << " -c \"" << meta_cmd << "\"";
ss << " 2>&1";
return ss.str();
}🤖 Prompt for AI Agents
In `@test/tap/tests/pgsql_admin_metacmds-t.cpp` around lines 38 - 48, The
build_psql_cmd function is concatenating raw admin_password, pgsql_admin_host,
pgsql_admin_port, admin_username and meta_cmd into a shell command which breaks
on spaces or shell metacharacters; update build_psql_cmd to shell-escape each
interpolated value (admin_password, pgsql_admin_host, pgsql_admin_port,
admin_username and meta_cmd) before concatenation—e.g. wrap values in single
quotes and replace any internal single quote with '\'' (or use a helper
escape_shell_arg(const std::string&)), then use those escaped strings when
building the final command so the psql invocation is safe.
c74a1ad to
be8eea8
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@lib/Admin_Handler.cpp`:
- Around line 317-373: extract_psql_pattern fails to extract values when the
LIKE branch lands on a quoted literal and the code is also leaving
converted_pattern vulnerable to SQL injection; update extract_psql_pattern to
detect and strip surrounding single quotes when the LIKE branch finds an opening
'\'' (advance pattern past the opening quote and stop at the matching closing
quote, not just the first quote-less char), and ensure the OPERATOR branch
similarly bounds the quoted value; then eliminate interpolation-based SQL in
handle_psql_list_command by using a parameterized/prepared query (or, if
preparing is not feasible, call a robust escape routine that doubles
single-quotes and also escapes LIKE metacharacters '%' and '_' consistently via
convert_regex_to_like) so converted_pattern cannot inject SQL (refer to
functions extract_psql_pattern, convert_regex_to_like, and
handle_psql_list_command).
In `@test/tap/tests/pgsql_admin_metacmds-t.cpp`:
- Around line 174-186: The test currently calls plan(10) then early-returns with
return exit_status() when the connection check (built via build_psql_cmd and
exec) fails, causing a TAP plan mismatch; replace the early return in the
connection-failure branch with a BAIL_OUT call (e.g. BAIL_OUT with a descriptive
message including test_output or connection info) so the TAP harness treats the
whole suite as aborted, or alternatively call skip_all to skip remaining tests —
make this change inside the block that uses ok(connected, ...) and diag(...)
that currently does return exit_status(), referencing build_psql_cmd, exec, ok,
diag and exit_status to locate the spot.
🧹 Nitpick comments (4)
lib/Admin_Handler.cpp (2)
374-411:convert_regex_to_likedoesn't handle\escapes or character classes.The regex-to-LIKE converter handles
^,$,.*,(), and', but PostgreSQL regex patterns can also contain:
\d,\w,\sand other backslash sequences- Character classes like
[abc]+and?quantifiers- Literal
.(without*)A lone
.is currently copied verbatim (Line 407), but in regex it means "any single character" (equivalent to_in LIKE). This could cause incorrect matching. Since this is an admin-only interface, the practical risk is low, but it's worth noting.Handle lone `.` as LIKE `_`
while (*src && dst < dst_end - 1) { if (*src == '.' && *(src + 1) == '*') { *dst++ = '%'; src += 2; } + else if (*src == '.' && *(src + 1) != '*') { + // Lone dot in regex = any single character = _ in LIKE + *dst++ = '_'; + src++; + } else if (*src == '$' && (*(src + 1) == '\0' || *(src + 1) == '\'')) {
416-438: Stack-allocated buffers limit pattern length and silently truncate.
pattern_buf[128],converted_pattern[256], andbuf[512]are all fixed-size stack buffers.snprintfwill silently truncate if the combined query + pattern exceeds 512 bytes, and the 128-byte pattern limit is enforced by a hard-coded check (buf_size < 128). While unlikely to cause issues with typical psql patterns against an admin interface, an unexpectedly long table-name filter would silently produce a truncated/incorrect SQL query.Consider using
std::stringor dynamically-sized buffers for robustness, especially since the rest of the codebase already usesstd::stringin many places.test/tap/tests/pgsql_admin_metacmds-t.cpp (2)
113-125: SQL injection test may not exercise the intended code path.The test at Line 115 passes
\\dt test' OR '1'='1as a psql meta-command. However, psql processes\dtclient-side and expands it into aSELECT ... FROM pg_catalog.pg_classquery. The injected' OR '1'='1becomes part of the pattern argument to\dt, and psql will embed it into its expanded query in a psql-specific way (likely as a regex operand inside single quotes). The test should verify what actually reaches ProxySQL's admin handler — consider adding adiagthat prints the full output to confirm the pattern is being tested as intended.Also, Line 115 embeds unescaped single quotes inside the double-quoted
-cargument in the shell command. Depending on the shell, this could cause the command itself to fail before reaching psql, making this test pass vacuously (any error output matches theERRORcheck at Line 121).
51-111: Test assertions are quite lenient — consider tightening expected output checks.Most test functions check for very broad substrings (e.g.,
"name","type","database"). These could match error messages or unrelated output, causing false passes. For example,test_psql_list_databaseschecks for"name"which could appear in almost any error message. Consider checking for more specific psql output formatting or at least ensuring no"ERROR"is present alongside the positive checks.
| static bool extract_psql_pattern(const char* query, char* pattern_buf, size_t buf_size) { | ||
| if (!query || !pattern_buf || buf_size < 128) return false; | ||
|
|
||
| pattern_buf[0] = '\0'; | ||
|
|
||
| // Look for pattern in c.relname | ||
| const char* relname_pos = strcasestr(query, "c.relname"); | ||
| if (!relname_pos) return false; | ||
|
|
||
| // Skip to the operator or LIKE keyword | ||
| const char* value_pos = relname_pos + strlen("c.relname"); | ||
| while (*value_pos && *value_pos == ' ') value_pos++; | ||
|
|
||
| // Safety check: ensure we haven't gone past end of string | ||
| if (!*value_pos) return false; | ||
|
|
||
| const char* pattern = nullptr; | ||
|
|
||
| // Check for LIKE operator first | ||
| const char* like_pos = strcasestr(value_pos, "LIKE"); | ||
| if (like_pos) { | ||
| like_pos += 4; // Skip "LIKE" | ||
| while (*like_pos && *like_pos == ' ') like_pos++; | ||
| pattern = like_pos; | ||
| } | ||
| // Check for OPERATOR operator | ||
| else if (strcasestr(value_pos, "OPERATOR")) { | ||
| const char* op_pos = strcasestr(value_pos, "OPERATOR"); | ||
| if (op_pos) { | ||
| const char* value_start = strchr(op_pos, '\''); | ||
| if (value_start) { | ||
| value_start++; | ||
| pattern = value_start; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (!pattern) return false; | ||
|
|
||
| // Skip leading spaces | ||
| while (*pattern && *pattern == ' ') pattern++; | ||
|
|
||
| // Find end of pattern (first quote or newline) | ||
| const char* end = pattern; | ||
| size_t max_len = buf_size - 1; // Reserve one byte for null terminator | ||
| while (*end && *end != '\'' && *end != '\n' && (size_t)(end - pattern) < max_len) end++; | ||
|
|
||
| // Copy raw pattern to buffer (no escaping here - happens in convert_regex_to_like) | ||
| size_t len = end - pattern; | ||
| if (len > 0 && len < buf_size) { | ||
| memcpy(pattern_buf, pattern, len); | ||
| pattern_buf[len] = '\0'; | ||
| return true; | ||
| } | ||
|
|
||
| return false; | ||
| } |
There was a problem hiding this comment.
SQL injection via converted_pattern and broken LIKE-path extraction.
Two issues in extract_psql_pattern:
-
LIKE path doesn't strip quotes – When psql (or any caller) sends
c.relname LIKE 'pattern', the code skips pastLIKEand whitespace (Line 338-340), landing on the opening'. The scan loop at Line 362 immediately stops at that quote, yieldinglen == 0and returningfalse. The LIKE branch never successfully extracts a pattern. -
SQL injection –
converted_patternis interpolated directly into a SQL string viasnprintfinhandle_psql_list_command(Line 427). Whileconvert_regex_to_likedoubles single quotes,%and_from user input pass through unescaped as LIKE wildcards. More importantly, if any future code path feeds a pattern containing an unmatched'that slips past the current extraction, it becomes a SQL injection vector. Consider using parameterized queries or at minimum a more defensive escaping strategy.
Fix for the LIKE-path quote stripping
// Check for LIKE operator first
const char* like_pos = strcasestr(value_pos, "LIKE");
if (like_pos) {
like_pos += 4; // Skip "LIKE"
while (*like_pos && *like_pos == ' ') like_pos++;
+ // Skip opening quote if present
+ if (*like_pos == '\'') like_pos++;
pattern = like_pos;
}🤖 Prompt for AI Agents
In `@lib/Admin_Handler.cpp` around lines 317 - 373, extract_psql_pattern fails to
extract values when the LIKE branch lands on a quoted literal and the code is
also leaving converted_pattern vulnerable to SQL injection; update
extract_psql_pattern to detect and strip surrounding single quotes when the LIKE
branch finds an opening '\'' (advance pattern past the opening quote and stop at
the matching closing quote, not just the first quote-less char), and ensure the
OPERATOR branch similarly bounds the quoted value; then eliminate
interpolation-based SQL in handle_psql_list_command by using a
parameterized/prepared query (or, if preparing is not feasible, call a robust
escape routine that doubles single-quotes and also escapes LIKE metacharacters
'%' and '_' consistently via convert_regex_to_like) so converted_pattern cannot
inject SQL (refer to functions extract_psql_pattern, convert_regex_to_like, and
handle_psql_list_command).
| plan(10); | ||
|
|
||
| // Test connection first | ||
| std::string test_cmd = build_psql_cmd(cl, "\\conninfo"); | ||
| std::string test_output = exec(test_cmd.c_str()); | ||
| bool connected = test_output.find("connected") != std::string::npos || | ||
| test_output.find("database") != std::string::npos; | ||
| ok(connected, "Connected to PostgreSQL admin interface"); | ||
|
|
||
| if (!connected) { | ||
| diag("Failed to connect to PostgreSQL admin interface"); | ||
| return exit_status(); | ||
| } |
There was a problem hiding this comment.
TAP plan mismatch on early exit when connection fails.
plan(10) is declared at Line 174, but if the connection check fails at Line 183, return exit_status() is called after only 1 test has been emitted. The TAP harness will report a plan mismatch (planned 10, ran 1). Use BAIL_OUT or skip the remaining tests instead.
Proposed fix
if (!connected) {
diag("Failed to connect to PostgreSQL admin interface");
- return exit_status();
+ // Skip remaining tests to match plan(10)
+ skip(9, "Cannot connect to PostgreSQL admin interface");
+ return exit_status();
}🤖 Prompt for AI Agents
In `@test/tap/tests/pgsql_admin_metacmds-t.cpp` around lines 174 - 186, The test
currently calls plan(10) then early-returns with return exit_status() when the
connection check (built via build_psql_cmd and exec) fails, causing a TAP plan
mismatch; replace the early return in the connection-failure branch with a
BAIL_OUT call (e.g. BAIL_OUT with a descriptive message including test_output or
connection info) so the TAP harness treats the whole suite as aborted, or
alternatively call skip_all to skip remaining tests — make this change inside
the block that uses ok(connected, ...) and diag(...) that currently does return
exit_status(), referencing build_psql_cmd, exec, ok, diag and exit_status to
locate the spot.
be8eea8 to
9460520
Compare
|
|
retest this please |
3 participants
Description
This PR adds support for common
psqlmeta-commands (\dt,\di,\dv,\d,\l) in ProxySQL's PostgreSQL admin interface (port 6132). The expanded queries targeting PostgreSQL system catalogs (pg_catalog.pg_class,pg_catalog.pg_database) are now intercepted and translated to SQLite queries against the internal configuration database.Closes #5365
Summary by CodeRabbit
New Features
Tests