Review 1.5: the read-only check of typed queries can no longer be bypassed

The synthetic SQL check (and custom rules) used a regex that ended an [identifier]
at the first ] and a block comment at the first */, so "[x]]'] DELETE ..." hid a
DELETE inside what it took for a string. T-SQL also needs no semicolon between
statements, so "SELECT 1 COMMIT" or "SELECT 1 WRITETEXT ..." passed.

- ReadOnlyQuery lexes T-SQL as SQL Server does (nested comments, '' ]] ""),
  refuses unterminated comments/strings/identifiers, every statement keyword
  (COMMIT, ROLLBACK, SET, DECLARE, WRITETEXT, UPDATETEXT, RAISERROR, CHECKPOINT,
  ENABLE/DISABLE, RECEIVE/SEND ...), NEXT VALUE FOR (sequences are not rolled
  back) and update/exclusive lock hints, and a second SELECT unless joined by
  UNION/EXCEPT/INTERSECT.
- CustomRuleDefinition.QueryProblems uses it, so the synthetic check validates the
  same way when saved and before it runs.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

src/SQLInsight.Core/CustomRules/CustomRuleDefinition.cs

@@ -112,28 +112,10 @@ public sealed partial record CustomRuleDefinition
     }
 
     /// <summary>
-    /// The query must be a single read-only SELECT. This is a second line of defence: the agent's login has no write
+    /// The query must be a single read-only SELECT (<see cref="ReadOnlyQuery"/>). This is a second line of defence: the
-    /// permissions to begin with (grant-least-privilege.sql).
+    /// agent's login has no write permissions to begin with (grant-least-privilege.sql).
     /// </summary>
-    public static IEnumerable<string> QueryProblems(string query)
+    public static IEnumerable<string> QueryProblems(string query) => ReadOnlyQuery.Problems(query);
-    {
-        ArgumentNullException.ThrowIfNull(query);
-        var code = StringsAndComments().Replace(query, " ");
-        if (!StartsWithSelect().IsMatch(code))
-        {
-            yield return "Forespørgslen skal starte med SELECT eller WITH.";
-        }
-
-        foreach (Match forbidden in ForbiddenKeywords().Matches(code))
-        {
-            yield return $"Forespørgslen må ikke indeholde {forbidden.Value.ToUpperInvariant()}: regler må kun læse.";
-        }
-
-        if (code.TrimEnd().TrimEnd(';').Contains(';', StringComparison.Ordinal))
-        {
-            yield return "Forespørgslen må kun være én sætning (intet semikolon undervejs).";
-        }
-    }
 
     /// <summary>Replaces {column} with the row's value; unknown placeholders are left as they are.</summary>
     public static string Fill(string template, IReadOnlyDictionary<string, string?> row)
@@ -146,15 +128,6 @@ public sealed partial record CustomRuleDefinition
     [GeneratedRegex(@"^(COM|LOCAL)-[A-Z]{2,10}-[0-9]{3}$")]
     private static partial Regex IdPattern();
 
-    [GeneratedRegex(@"'(?:[^']|'')*'|\[[^\]]*\]|""[^""]*""|--[^\n]*|/\*.*?\*/", RegexOptions.Singleline)]
-    private static partial Regex StringsAndComments();
-
-    [GeneratedRegex(@"^\s*(SELECT|WITH)\b", RegexOptions.IgnoreCase)]
-    private static partial Regex StartsWithSelect();
-
-    [GeneratedRegex(@"\b(INSERT|UPDATE|DELETE|MERGE|DROP|ALTER|CREATE|TRUNCATE|EXEC|EXECUTE|GRANT|REVOKE|DENY|SHUTDOWN|KILL|BACKUP|RESTORE|DBCC|RECONFIGURE|OPENROWSET|OPENDATASOURCE|OPENQUERY|INTO|WAITFOR|USE|BULK|SETUSER)\b|\b(sp|xp)_\w+", RegexOptions.IgnoreCase)]
-    private static partial Regex ForbiddenKeywords();
-
     [GeneratedRegex(@"\{([A-Za-z0-9_ ]+)\}")]
     private static partial Regex Placeholder();
 }

src/SQLInsight.Core/CustomRules/ReadOnlyQuery.cs

@@ -0,0 +1,186 @@
+using System.Text;
+using System.Text.RegularExpressions;
+
+namespace SQLInsight.Core.CustomRules;
+
+/// <summary>
+/// Whether a query someone typed (a custom rule, a synthetic SQL check) is one read-only SELECT. T-SQL is lexed the
+/// way SQL Server reads it: nested block comments, <c>''</c> in strings, <c>]]</c> in [identifiers] and <c>""</c> in
+/// "identifiers", so a keyword cannot hide in what only looks like a comment or a string. T-SQL needs no semicolon
+/// between statements, so besides the writing keywords every statement keyword is refused, and a second SELECT is
+/// only accepted after UNION, EXCEPT or INTERSECT.
+/// This is a second line of defence: the login has no write permissions to begin with (grant-least-privilege.sql), and
+/// a synthetic check also runs the query in a transaction that is always rolled back.
+/// </summary>
+public static partial class ReadOnlyQuery
+{
+    public static IEnumerable<string> Problems(string query)
+    {
+        ArgumentNullException.ThrowIfNull(query);
+        var (code, unterminated) = Code(query);
+        if (unterminated is not null)
+        {
+            yield return $"Forespørgslen har {unterminated}, der ikke er afsluttet.";
+            yield break;
+        }
+
+        if (!StartsWithSelect().IsMatch(code))
+        {
+            yield return "Forespørgslen skal starte med SELECT eller WITH.";
+        }
+
+        foreach (var forbidden in ForbiddenKeywords().Matches(code).Select(m => Regex.Replace(m.Value.ToUpperInvariant(), @"\s+", " ")).Distinct(StringComparer.Ordinal))
+        {
+            yield return $"Forespørgslen må ikke indeholde {forbidden}: forespørgslen må kun læse (sæt [ ] om et kolonnenavn, der hedder sådan).";
+        }
+
+        if (code.TrimEnd().TrimEnd(';').Contains(';', StringComparison.Ordinal) || !OneStatement(code))
+        {
+            yield return "Forespørgslen må kun være én sætning (intet semikolon undervejs, og flere SELECT kun med UNION, EXCEPT eller INTERSECT).";
+        }
+    }
+
+    /// <summary>
+    /// The query with comments, strings and quoted identifiers replaced by a space, and what was left open (a comment,
+    /// a string, an identifier) or null.
+    /// </summary>
+    internal static (string Code, string? Unterminated) Code(string query)
+    {
+        var code = new StringBuilder(query.Length);
+        var i = 0;
+        while (i < query.Length)
+        {
+            var c = query[i];
+            var next = i + 1 < query.Length ? query[i + 1] : '\0';
+            if (c == '-' && next == '-')
+            {
+                var end = query.IndexOf('\n', i);
+                i = end < 0 ? query.Length : end;
+                code.Append(' ');
+            }
+            else if (c == '/' && next == '*')
+            {
+                // SQL Server nests block comments: /* /* */ is still inside the comment.
+                var depth = 0;
+                do
+                {
+                    if (i + 1 < query.Length && query[i] == '/' && query[i + 1] == '*')
+                    {
+                        depth++;
+                        i += 2;
+                    }
+                    else if (i + 1 < query.Length && query[i] == '*' && query[i + 1] == '/')
+                    {
+                        depth--;
+                        i += 2;
+                    }
+                    else
+                    {
+                        i++;
+                    }
+                }
+                while (depth > 0 && i < query.Length);
+
+                if (depth > 0)
+                {
+                    return (code.ToString(), "en kommentar (/* … */)");
+                }
+
+                code.Append(' ');
+            }
+            else if (c is '\'' or '[' or '"')
+            {
+                var close = c == '[' ? ']' : c;
+                var j = i + 1;
+                while (true)
+                {
+                    j = query.IndexOf(close, j);
+                    if (j < 0)
+                    {
+                        return (code.ToString(), c == '\'' ? "en tekst ('…')" : "et navn i " + c + close);
+                    }
+
+                    if (j + 1 < query.Length && query[j + 1] == close)
+                    {
+                        j += 2; // '' or ]] or "": the quote itself, still inside
+                        continue;
+                    }
+
+                    break;
+                }
+
+                i = j + 1;
+                code.Append(' ');
+            }
+            else
+            {
+                code.Append(c);
+                i++;
+            }
+        }
+
+        return (code.ToString(), null);
+    }
+
+    /// <summary>
+    /// One statement: the SELECTs outside parentheses are the query's own SELECT (for WITH, the one right after the
+    /// common table expressions) and those joined to it by UNION, EXCEPT or INTERSECT. "SELECT 1 SELECT 2" is two.
+    /// </summary>
+    private static bool OneStatement(string code)
+    {
+        var depth = 0;
+        var mainSeen = false;
+        var startsWithWith = StartsWithWith().IsMatch(code);
+        string? previous = null;
+        foreach (Match token in Tokens().Matches(code))
+        {
+            var value = token.Value;
+            if (value == "(")
+            {
+                depth++;
+            }
+            else if (value == ")")
+            {
+                depth = Math.Max(0, depth - 1);
+            }
+            else if (depth == 0 && value.Equals("SELECT", StringComparison.OrdinalIgnoreCase))
+            {
+                var joined = previous is not null && (previous.Equals("UNION", StringComparison.OrdinalIgnoreCase) || previous.Equals("ALL", StringComparison.OrdinalIgnoreCase)
+                    || previous.Equals("EXCEPT", StringComparison.OrdinalIgnoreCase) || previous.Equals("INTERSECT", StringComparison.OrdinalIgnoreCase));
+                var first = !mainSeen && (startsWithWith ? previous == ")" : previous is null);
+                if (!joined && !first)
+                {
+                    return false;
+                }
+
+                mainSeen = true;
+            }
+
+            previous = value;
+        }
+
+        return true;
+    }
+
+    [GeneratedRegex(@"^\s*(SELECT|WITH)\b", RegexOptions.IgnoreCase)]
+    private static partial Regex StartsWithSelect();
+
+    [GeneratedRegex(@"^\s*WITH\b", RegexOptions.IgnoreCase)]
+    private static partial Regex StartsWithWith();
+
+    [GeneratedRegex(@"[()]|[A-Za-z_@#][A-Za-z0-9_@#$]*|[^\sA-Za-z_@#()]")]
+    private static partial Regex Tokens();
+
+    /// <summary>
+    /// Writing keywords, every other statement keyword (T-SQL needs no semicolon between statements), sequences
+    /// (NEXT VALUE FOR is not rolled back) and table hints that take update or exclusive locks.
+    /// </summary>
+    [GeneratedRegex(
+        @"\b(INSERT|UPDATE|DELETE|MERGE|DROP|ALTER|CREATE|TRUNCATE|EXEC|EXECUTE|GRANT|REVOKE|DENY|SHUTDOWN|KILL|BACKUP|RESTORE|DBCC|RECONFIGURE"
+        + @"|OPENROWSET|OPENDATASOURCE|OPENQUERY|INTO|WAITFOR|USE|BULK|SETUSER"
+        + @"|WRITETEXT|UPDATETEXT|READTEXT|CHECKPOINT|COMMIT|ROLLBACK|SAVE|BEGIN|TRAN|TRANSACTION|DECLARE|SET|PRINT|RAISERROR|THROW|IF|WHILE|GOTO|RETURN"
+        + @"|BREAK|CONTINUE|OPEN|CLOSE|DEALLOCATE|REVERT|ADD|ENABLE|DISABLE|RECEIVE|SEND|CONVERSATION|DIALOG|LOAD|DUMP"
+        + @"|UPDLOCK|XLOCK|TABLOCK|TABLOCKX|HOLDLOCK|PAGLOCK|SERIALIZABLE|REPEATABLEREAD)\b|\bNEXT\s+VALUE\s+FOR\b|\b(sp|xp)_\w+",
+        RegexOptions.IgnoreCase)]
+    private static partial Regex ForbiddenKeywords();
+}

tests/SQLInsight.Core.Tests/ReadOnlyQueryTests.cs

@@ -0,0 +1,55 @@
+using SQLInsight.Core.CustomRules;
+
+namespace SQLInsight.Core.Tests;
+
+/// <summary>A typed query (custom rule, synthetic SQL check) is one read-only SELECT, also when someone tries to hide more.</summary>
+public class ReadOnlyQueryTests
+{
+    [Theory]
+    // A ]] in an [identifier] is not its end: the ' after it is no string, and the DELETE is code.
+    [InlineData("SELECT 1 AS [x]]'] ; DELETE FROM dbo.t; SELECT 'a'")]
+    [InlineData("SELECT 1 AS [x]]'] DELETE FROM dbo.t SELECT 'a'")]
+    // Block comments nest in T-SQL; an unterminated one is refused.
+    [InlineData("SELECT 1 /* /* */ DELETE FROM dbo.t")]
+    [InlineData("SELECT 1 AS \"a\"\"'\" DELETE FROM dbo.t --'")]
+    // No semicolon is needed between statements.
+    [InlineData("SELECT 1 COMMIT")]
+    [InlineData("SELECT 1 SELECT 2")]
+    [InlineData("SELECT 1 WRITETEXT dbo.t.c @p 'x'")]
+    [InlineData("SELECT 1 UPDATETEXT dbo.t.c @p 0 NULL 'x'")]
+    [InlineData("SELECT 1 DECLARE @x int")]
+    [InlineData("SELECT 1 SET CONTEXT_INFO 0x01")]
+    [InlineData("SELECT 1 RAISERROR('x', 16, 1) WITH LOG")]
+    [InlineData("SELECT 1 CHECKPOINT")]
+    [InlineData("SELECT 1 ROLLBACK")]
+    [InlineData("SELECT 1 BEGIN TRAN")]
+    [InlineData("SELECT 1 ENABLE TRIGGER trg ON dbo.t")]
+    [InlineData("SELECT 1 WHILE 1 = 1 SELECT 2")]
+    [InlineData("WITH q AS (SELECT 1 AS a) SELECT a FROM q SELECT 2")]
+    [InlineData("SELECT 1 x RECEIVE TOP (1) * FROM dbo.q")]
+    [InlineData("SELECT 1 ADD SIGNATURE TO dbo.p BY CERTIFICATE c")]
+    // Side effects inside one SELECT: sequences are not rolled back; update and exclusive locks block production.
+    [InlineData("SELECT NEXT VALUE FOR dbo.InvoiceNumbers")]
+    [InlineData("SELECT * FROM dbo.CustTable WITH (UPDLOCK, HOLDLOCK)")]
+    [InlineData("SELECT * FROM dbo.CustTable WITH (TABLOCKX)")]
+    [InlineData("SELECT * FROM dbo.CustTable WITH (XLOCK)")]
+    [InlineData("SELECT * INTO dbo.Copy FROM dbo.CustTable")]
+    [InlineData("SELECT 1 EXEC sp_who")]
+    [InlineData("SELECT * FROM OPENQUERY(LINKED, 'EXEC x')")]
+    [InlineData("select 1 exec('delete dbo.t')")]
+    [InlineData("SELECT 'unterminated")]
+    [InlineData("SELECT [unterminated")]
+    public void Hidden_or_extra_statements_are_refused(string query) => Assert.NotEmpty(ReadOnlyQuery.Problems(query));
+
+    [Theory]
+    [InlineData("SELECT 1")]
+    [InlineData("SELECT COUNT(*) FROM dbo.DATAAREA")]
+    [InlineData("SELECT [update], [into], [x]]y] FROM dbo.audit")]
+    [InlineData("SELECT 'DELETE; EXEC ''x''' AS text /* INSERT /* nested */ COMMIT */ -- DROP")]
+    [InlineData("WITH d AS (SELECT name FROM sys.databases), e (n) AS (SELECT 1) SELECT * FROM d;")]
+    [InlineData("SELECT name FROM sys.databases UNION ALL SELECT name FROM sys.objects EXCEPT SELECT N'x' INTERSECT SELECT N'x'")]
+    [InlineData("SELECT TOP (10) a FROM (SELECT 1 AS a) x WHERE a IN (SELECT 1) ORDER BY a OFFSET 0 ROWS FETCH NEXT 5 ROWS ONLY")]
+    [InlineData("SELECT * FROM dbo.CustTable WITH (NOLOCK) OPTION (MAXDOP 1)")]
+    [InlineData("SELECT IIF(1 = 1, GETDATE(), NULL), \"quoted\"\"name\" FROM sys.objects")]
+    public void Read_only_queries_are_accepted(string query) => Assert.Empty(ReadOnlyQuery.Problems(query));
+}