[fix] validate sql reaching the alert query executor - #4275
Draft
Duansg wants to merge 9 commits into
Draft
Conversation
Member
|
This is useful executor-level read-only validation, but it does not close the complete alert-query exposure. Low-privilege callers can still issue broad telemetry reads, expensive SELECTs, and raw PromQL; SELECT-only parsing also permits unions/subqueries across available tables. Please add an authorization/ownership boundary for preview and authoring, scope readable sources/tables, and enforce time/range/cost limits for both SQL and PromQL. I would treat the current patch as one defense-in-depth layer, not a complete remediation. |
…elling
The read only check only covered the sql("...") spelling, so an expression that
carried a statement as promql("...") still reached the executor unvalidated
whenever the caller named a sql datasource. The preview endpoint takes that
datasource straight from the path, so the spelling could not decide anything.
Guard QueryExecutor.execute instead: every route into the executor is covered,
including the ones added later, and the raw log query path shares the same seam
rather than repeating the check.
Fix two holes in the validator while it is the only thing standing between an
alert rule and the database credentials. CCJSqlParserUtil.parse returns the
first statement and discards the rest, so "select 1; drop table x" validated as
a plain select while the caller still handed the whole string over. And
"select * into backup from cpu" parses as a select but writes.
Read only mode no longer rejects what JSqlParser cannot parse. The only sql
executor talks to GreptimeDB, whose range query syntax the parser does not
cover although it is an ordinary read, and failing those rules would trade a
working feature for nothing. The two properties this mode has to guarantee, one
statement and read only, are established by scanning outside literals and
comments instead, which no dialect can confuse. The whitelisting mode keeps
failing closed, because enumerating table names needs the parse tree.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The read only check looked at the outermost node of the parse tree, which proves nothing about the rest of it. JSqlParser reports a plain select as the outermost node of all three of these, and each carries a write: WITH x AS (DELETE FROM cpu RETURNING *) SELECT * FROM x SELECT * INTO backup FROM cpu UNION SELECT * FROM cpu SELECT * FROM (SELECT * INTO backup FROM cpu) t Walk the whole tree instead. A walk that ends in an exception rejects too: a data modifying cte makes JSqlParser's own finder cast a ParenthesedDelete to a ParenthesedSelect, which used to escape as a 500 rather than a rejection. The tree is only available when the parser can read the dialect, and the whole point of read only mode is that it often cannot, so the statement scan now also rejects a string carrying a word that only a write contains. That is what holds for a nested write in a GreptimeDB range query. It matches whole words outside literals, so delete_count and truncate(value, 2) keep working, and it lists no word that doubles as an ordinary function. With writes caught wherever they sit, a cte no longer has to parse to be accepted, so range queries inside a WITH work now. The whitelisting mode had the same hole from the other side: it says which tables a statement may touch, so "select * into backup from hertzbeat_logs" named only allowed tables and passed. It walks the tree now too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Duansg <siguoduan@gmail.com>
Skipping the literal closed one hole by opening its mirror. A dialect that has dollar quoting hides a stacked statement behind the comment marker in SELECT $$--$$; DROP TABLE cpu and skipping the literal is what catches that. But a dialect that does not have dollar quoting means the semicolon in SELECT 1 $$;DROP TABLE cpu$$ separates statements for real, and there the skip is the thing doing the hiding: that string was rejected before the literal was understood and accepted after. Refusing needs no assumption in either direction. A dialect with dollar quoting is refused; one without was going to fail on the syntax anyway. No read of a metric table spells anything this way, and a dollar sign that opens no literal stays an ordinary character, so $1 and a$b and '$5' still parse as reads. That the mirror case was reachable at all came from leaning on the parser: it reads $t$...$t$ as two statements but $$...$$ as one, and the whole reason this scan exists is that the parser cannot be relied on for the dialect in front of it. A dollar sign now also ends a word, so a literal opening straight after an identifier is still seen rather than swallowed into it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Duansg
marked this pull request as draft
August 9, 2026 16:20
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
What's changed?
Apply read-only validation at the point where SQL reaches the executor, and fix the validator's handling of multiple statements.
Checklist
Add or update API