Skip to content

PrepareBatch insert-query parser does not recognize // or /* */ comments, so a commented-out INSERT hijacks the target table and column list #1950

Description

@claude

Description

extractInsertQueryComponents (batch.go:25) parses the INSERT statement out of the user's query with

var normalizeInsertQueryMatch = regexp.MustCompile(`(?i)(?:(?:--[^\n]*|#![^\n]*|#\s[^\n]*)\n\s*)*(INSERT\s+INTO\s+([^(]+)(?:\s*\([^()]*(?:\([^()]*\)[^()]*)*\))?)(?:\s*VALUES)?`)

The leading (?:--…|#!…|#\s…) group exists to skip comments so the regex does not latch onto an
INSERT INTO that lives inside a comment (added in #1693). But the ClickHouse lexer also accepts
// line comments and /* … */ block comments, and neither is in that group. Because the regex is
unanchored, the first INSERT INTO anywhere in the string wins — so when a // or /* */ comment
mentions an INSERT, the client silently parses the commented-out statement instead of the real one.

extractInsertQueryComponents / extractNormalizedInsertQueryAndColumns feed:

  • conn_batch.go:23 — native PrepareBatch
  • conn_http_batch.go:77 — HTTP PrepareBatch
  • conn_http_format.go:338 and format.go:93InsertFormat

so the consequence is that a batch is prepared against the wrong table with the wrong column list.
If the commented-out table exists and its schema is compatible, rows land in the wrong table with no
error at all; otherwise the user gets an UNKNOWN_TABLE / column mismatch error naming a table they
did not ask to write to.

This mirrors ClickHouse/clickhouse-connect#925, where the same class of gap (//, #, nested block
comments, backtick identifiers, heredocs missing from the comment stripper) misclassifies queries. In
clickhouse-go the exposure is narrower — there is no remove_sql_comments and no query_limit
rewriting — but the insert-query parser has the same incomplete comment coverage.

ClickHouse server version

Not verified against a running server: no ClickHouse instance was reachable in this environment
(nothing listening on :8123/:9000). The finding is a code-analysis result confirmed by the unit
test below, which exercises the library's own parser directly — the exact function PrepareBatch
calls before any bytes hit the wire. Server-side, // and /* … */ are both documented ClickHouse
comment syntaxes (SELECT 1 //x and SELECT 1 /* x */ both return 1).

Reproduction

batch_comment_test.go in the root package:

package clickhouse

import (
	"testing"

	"github.com/stretchr/testify/assert"
)

func TestInsertCommentSkipping(t *testing.T) {
	cases := []struct {
		name  string
		query string
	}{
		{"dash-dash", "-- INSERT INTO wrong_table (a)\nINSERT INTO right_table (b, c)"},
		{"hash-space", "# INSERT INTO wrong_table (a)\nINSERT INTO right_table (b, c)"},
		{"hash-bang", "#! INSERT INTO wrong_table (a)\nINSERT INTO right_table (b, c)"},
		{"double-slash", "// INSERT INTO wrong_table (a)\nINSERT INTO right_table (b, c)"},
		{"block", "/* INSERT INTO wrong_table (a) */ INSERT INTO right_table (b, c)"},
		{"block-multiline", "/*\nINSERT INTO wrong_table (a)\n*/\nINSERT INTO right_table (b, c)"},
	}
	for _, tc := range cases {
		t.Run(tc.name, func(t *testing.T) {
			stmt, table, cols, err := extractInsertQueryComponents(tc.query)
			t.Logf("stmt=%q table=%q cols=%v err=%v", stmt, table, cols, err)
			assert.NoError(t, err)
			assert.Equal(t, "right_table", table)
			assert.Equal(t, []string{"b", "c"}, cols)
		})
	}
}

go test -run TestInsertCommentSkipping -v .

comment style parsed statement table columns result
-- INSERT INTO right_table (b, c) right_table [b c] pass
# + space INSERT INTO right_table (b, c) right_table [b c] pass
#! INSERT INTO right_table (b, c) right_table [b c] pass
// INSERT INTO wrong_table (a) wrong_table [a] FAIL
/* … */ INSERT INTO wrong_table (a) wrong_table [a] FAIL
/* … */ multiline INSERT INTO wrong_table (a) wrong_table [a] FAIL

Expected in all six rows: table == "right_table", cols == ["b", "c"].
Actual for the // and /* */ rows: table == "wrong_table", cols == ["a"].

At the client level this means:

query := "// INSERT INTO wrong_table (a)\nINSERT INTO right_table (b, c)"
batch, _ := conn.PrepareBatch(ctx, query)   // prepares "INSERT INTO wrong_table (a) FORMAT Native"
batch.Append(uint64(42), "hello")
batch.Send()                                // targets wrong_table, not right_table

Suggested fix

batch.go:9 (and the duplicate insertMatch at conn_batch.go:19): the leading-comment group needs
a //[^\n]* alternative and a block-comment alternative. Given that the same function also blindly
strips FORMAT … (truncateFormat, batch.go:10) and VALUES … (truncateValues, batch.go:11)
from anywhere in the text — including inside string literals, backtick identifiers and heredocs — the
more robust fix is the one suggested upstream: a single linear left-to-right scan that follows the
server lexer (--, //, # + space and #! line comments; nested /* */; single-quote,
double-quote and backtick quoting with backslash and doubled-quote escapes; heredocs) and strips
comments before the INSERT statement is matched.

Also note that insertMatch and columnMatch in conn_batch.go:19-20 appear to be dead code —
nothing references them now that prepareBatch goes through
extractNormalizedInsertQueryAndColumns — so whatever fix lands should probably delete them rather
than keep a second copy of the pattern in sync.

Link

Relayed from ClickHouse/clickhouse-connect#925

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions