Implement safe explain #226
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
| return 'The query is not a single safe statement. Only SELECT, INSERT, UPDATE, DELETE, and WITH statements are allowed.'; | ||
| } | ||
|
|
||
| if (query.includes('$1') || query.includes('$2') || query.includes('$3') || query.includes('$4')) { |
Contributor
There was a problem hiding this comment.
Incomplete parameter placeholder detection only checks for $1-$4, but PostgreSQL queries can have many more parameters like $5, $6, etc.
View Details
📝 Patch Details
diff --git a/apps/dbagent/src/lib/targetdb/safe-explain.ts b/apps/dbagent/src/lib/targetdb/safe-explain.ts
index 000d59e..dca7d3a 100644
--- a/apps/dbagent/src/lib/targetdb/safe-explain.ts
+++ b/apps/dbagent/src/lib/targetdb/safe-explain.ts
@@ -19,7 +19,7 @@ export async function safeExplainQuery(client: ClientBase, schema: string, query
return 'The query is not a single safe statement. Only SELECT, INSERT, UPDATE, DELETE, and WITH statements are allowed.';
}
- if (query.includes('$1') || query.includes('$2') || query.includes('$3') || query.includes('$4')) {
+ if (/\$\d+/.test(query)) {
// TODO: we could use `GENERIC_PLAN` to still get the plan in this case.
return 'The query seems to contain placeholders ($1, $2, etc). Replace them with actual values and try again.';
}
diff --git a/apps/dbagent/src/lib/targetdb/unsafe-explain.ts b/apps/dbagent/src/lib/targetdb/unsafe-explain.ts
index 6a44941..ce9d34d 100644
--- a/apps/dbagent/src/lib/targetdb/unsafe-explain.ts
+++ b/apps/dbagent/src/lib/targetdb/unsafe-explain.ts
@@ -262,7 +262,7 @@ export async function unsafeExplainQuery(client: ClientBase, schema: string, que
return 'The query is not a single safe statement. Only SELECT, INSERT, UPDATE, DELETE, and WITH statements are allowed.';
}
- if (query.includes('$1') || query.includes('$2') || query.includes('$3') || query.includes('$4')) {
+ if (/\$\d+/.test(query)) {
// TODO: we could use `GENERIC_PLAN` to still get the plan in this case.
return 'The query seems to contain placeholders ($1, $2, etc). Replace them with actual values and try again.';
}
Analysis
Incomplete parameter placeholder detection allows PostgreSQL queries with $5+ parameters to bypass safety checks
What fails: safeExplainQuery() and unsafeExplainQuery() in safe-explain.ts and unsafe-explain.ts only check for $1, $2, $3, $4 parameters, missing queries with higher-numbered placeholders like $5, $6, etc.
How to reproduce:
// This query would incorrectly pass the safety check:
const query = "SELECT id FROM users WHERE status = $5";
const detected = query.includes('$1') || query.includes('$2') || query.includes('$3') || query.includes('$4');
console.log(detected); // false - query incorrectly passes safety checkResult: Queries with only $5, $6, $7, $8, $9 parameters bypass the placeholder detection and reach PostgreSQL EXPLAIN, which then fails with "bind message supplies 0 parameters, but prepared statement requires X"
Expected: All parameterized queries should be detected and blocked with the message "The query seems to contain placeholders ($1, $2, etc). Replace them with actual values and try again."
Evidence: The test file unsafe-explain.test.ts line 67 contains a query with $8 parameters, confirming that higher-numbered parameters exist in the codebase and should be handled.
Fix: Replace substring matching with regex pattern /\$\d+/.test(query) to detect any parameter placeholder.
tsg
approved these changes
Sep 26, 2025
| } | ||
|
|
||
| if (query.includes('$1') || query.includes('$2') || query.includes('$3') || query.includes('$4')) { | ||
| // TODO: we could use `GENERIC_PLAN` to still get the plan in this case. |
Member
There was a problem hiding this comment.
We've discuss to do this, I imagine it's in a different PR?
Contributor
Author
There was a problem hiding this comment.
It is in the 3rd one (https://github.com/xataio/agent/pull/227/files) but only for safeExplainQuery, I didn't do this yet for unsafeExplainQuery
github-merge-queue bot
pushed a commit
that referenced
this pull request
Sep 26, 2025
Related discussion #223 We already improved it in #224, this PR makes it more secure by 1. Renaming `explainQuery` to `unsafeExplainQuery` while the single query check and transaction wrapping is pretty secure already, there might be edge cases where a query escapes the designated boundaries. Therefore, we rename the tool to `unsafeExplainQuery` 2. Create a new tool called `safeExplainQuery`, this operates on `queryId` and fetches the actual SQL from `pg_stat_statements` table itself thereby eliminating the code path that can lead to any SQL injection. This is done in the following stacked PR #226 3. Use the new `safeExplainQuery` tool instead of `unsafeExplainQuery`, to make it work, we had to additionally return `queryId` from `getSlowQueries` tool in addition to the slow SQL query. This is done in the following stacked PR #227
SolveEve
pushed a commit
to SolveEve/agent
that referenced
this pull request
Nov 15, 2025
Related discussion xataio/agent#223 We already improved it in xataio/agent#224, this PR makes it more secure by 1. Renaming `explainQuery` to `unsafeExplainQuery` while the single query check and transaction wrapping is pretty secure already, there might be edge cases where a query escapes the designated boundaries. Therefore, we rename the tool to `unsafeExplainQuery` 2. Create a new tool called `safeExplainQuery`, this operates on `queryId` and fetches the actual SQL from `pg_stat_statements` table itself thereby eliminating the code path that can lead to any SQL injection. This is done in the following stacked PR xataio/agent#226 3. Use the new `safeExplainQuery` tool instead of `unsafeExplainQuery`, to make it work, we had to additionally return `queryId` from `getSlowQueries` tool in addition to the slow SQL query. This is done in the following stacked PR xataio/agent#227
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See #225