Simplify how SQL injection is detected when using query.comment-format

[dominikzalewski]

Description

Instead of failing the query in case a character NOT in the allowlist is detected, now we search for occurence of the closing comment '*/' and only then fail the query.

Release notes

(X) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text:

[kokosing]

I am fine with the approach

[dominikzalewski]

This is copied and modified SessionInterpolatedValues since the original object is also used in BigQuery

[hashhar]

it's ok to limit the scope by not fixing for BigQuery but same problem still exists in connectors who use SessionInterpolatedValues.

[dominikzalewski]

Will create a follow-up ticket then and put it in the backlog

[kokosing]

I think you can simply remove io.trino.plugin.base.logging.SessionInterpolatedValues.SanitizedValuesProvider from SessionInterpolatedValues. I think the case in BigQuery is safe to be changed. There is no way to inject SQL as it is used only to set the label.

[hashhar]

LGTM.

Note that non-JDBC connectors will still fail though.

[hashhar]

it's ok to limit the scope by not fixing for BigQuery but same problem still exists in connectors who use SessionInterpolatedValues.

[kokosing]

it's ok to limit the scope by not fixing for BigQuery

IIRC the case of BigQuery is way different. It does not use comment, but there is a dedicated field where put audit information. In such case there is no way for SQL Injection

[hashhar]

Then instead of calling this new class ...Simple... we should call it "InterpolatedSessionValuesForSql"? Simple anyway doesn't convey any meaning.

Thanks for checking @kokosing , yes it uses a dedicated label field instead of adding to query text itself. So BigQuery isn't impacted.