Skip to content

explain refuses valid Postgres queries with a ";" inside a dollar-quoted body #65

Description

@KARTIKrocks

Problem

analyzer.IsMultiStatement counts a ; as a statement separator when any
dialect reading leaves it outside a literal, because neither reading of $$
is safe alone (PR #61):

  • Read as a dollar delimiter, UPDATE t AS $$ SET id = 1; DROP TABLE t hides
    its ; behind an unterminated body — a real bypass on MySQL, where $$ is
    just identifier bytes.
  • Read as ordinary bytes, SELECT $$'$$; DROP TABLE t hides its ; behind an
    unterminated ordinary literal — a real bypass on PostgreSQL.

Running both is correct for a package-level function with no dialect context.
The cost is over-rejection:

$ sqlguard explain --dialect postgres "SELECT $$a ; b$$"
explain: refusing to explain multi-statement input

That is a valid single PostgreSQL statement.

Why this is fixable without new API

explain.PlanAnalyzer already knows its dialect — it is a constructor
argument validated to "postgres" or "mysql":

type PlanAnalyzer struct {
	db       *sql.DB
	dialect  string // "postgres" or "mysql"
	allowDML bool
}

but validate calls analyzer.IsMultiStatement(q) with no dialect context
(explain/explain.go). The OR exists because the function cannot know the
target; the caller can. On PostgreSQL only the dollar-quote reading applies;
on MySQL only the ordinary reading does. Either way the correct single reading
is known, so the over-rejection is avoidable.

Caution

This relaxes a security control based on a user-supplied flag, so it needs a
deliberate decision, not just a patch. Points in favour: --dialect already
drives EXPLAIN syntax and the transaction mode (ReadOnly: !dml), so it is
already trusted for safety-relevant behaviour. Points against: a mismatched
flag would weaken the check, and the MySQL --allow-dml path runs read-write
where DDL implicit-commits past the rollback.

A dialect-aware separator check would also share plumbing with the dialect
hint proposed in #62 for Redact, so the two are worth designing together.

Acceptance criteria

  • sqlguard explain --dialect postgres "SELECT $$a ; b$$" is accepted.
  • Both bypass payloads above stay refused on the dialect where they are a
    threat; TestIsMultiStatementNeedsBothReadings still passes for the
    dialect-less path.
  • The dialect-less analyzer.IsMultiStatement keeps its current fail-closed
    behaviour for out-of-tree callers.
  • website/docs/explain.md updated with a version marker.

References: #61, #62

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or request

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions