diff --git a/Directory.Packages.props b/Directory.Packages.props index bc1dd88..b227cd7 100644 --- a/Directory.Packages.props +++ b/Directory.Packages.props @@ -1,6 +1,11 @@ true + + true @@ -11,6 +16,7 @@ + diff --git a/README.md b/README.md index 2f623b8..f9d708d 100644 --- a/README.md +++ b/README.md @@ -132,7 +132,7 @@ Passed via `configureAgentQL` when calling `AddAgentQL` or `AddAgentQLChat`: With `ReadOnly = true` the executor applies three layers of defense: -1. **Statement whitelist (provider-agnostic).** Before the connection is opened, the SQL is parsed and any statement that is not a `SELECT`, `VALUES`, `TABLE`, or `WITH` with a read-shaped body (CTEs validated recursively) is silently neutralized — the result returns `Success=true` with empty data, the write never reaches the database. Every DML, DDL, transaction-control, permission, session-settings, anonymous-block, and stored-procedure call is refused at this layer. +1. **Statement whitelist (provider-agnostic).** Before the connection is opened, the SQL is parsed and any statement that is not a `SELECT`, `VALUES`, `TABLE`, or `WITH` with a read-shaped body (CTEs validated recursively) is silently neutralized — the result returns `Success=true` with empty data, the write never reaches the database. Parenthesized query heads (`(SELECT ...)`, alone or combined via `UNION`/`INTERSECT`/`EXCEPT`) are unwrapped and validated the same way, and any statement whose shape cannot be read is rejected rather than assumed harmless. Every DML, DDL, transaction-control, permission, session-settings, anonymous-block, and stored-procedure call is refused at this layer. 2. **DBMS session read-only (where the provider supports it).** The connection's session is flipped to read-only so the database itself refuses any write that did somehow get past the whitelist — including writes attempted under an implicit autocommit transaction after an embedded `COMMIT`. 3. **End-of-query rollback.** The executor's transaction rolls back at the end of every query regardless of outcome. @@ -250,6 +250,8 @@ Each level overrides the one above it: Primary keys and discriminator columns are always included regardless of configuration. +> **Exclusion is schema hiding, not access control.** Include/exclude only affects the schema description shown to the model — the query executor does not parse or block SQL that references excluded tables or columns. A model that guesses (or is prompt-injected into trying) `SELECT "Ssn" FROM "Customers"` will get real data back, and database error messages returned to the model can confirm that hidden columns exist. Treat exclusion as a token-budget and relevance tool; protect genuinely sensitive data at the database itself — column grants, views, or the least-privileged principal described under **ReadOnly mode**. + ## AI Providers ### OpenAI diff --git a/src/Equibles.AgentQL.EntityFrameworkCore/Query/QueryExecutor.cs b/src/Equibles.AgentQL.EntityFrameworkCore/Query/QueryExecutor.cs index 443e424..8b392c8 100644 --- a/src/Equibles.AgentQL.EntityFrameworkCore/Query/QueryExecutor.cs +++ b/src/Equibles.AgentQL.EntityFrameworkCore/Query/QueryExecutor.cs @@ -68,10 +68,6 @@ public async Task Execute(string sql) } } - var isolationLevel = _options.ReadOnly - ? IsolationLevel.ReadUncommitted - : IsolationLevel.ReadCommitted; - var enforcer = _options.ReadOnly ? ReadOnlySessionEnforcerFactory.Resolve(_context.Database.ProviderName) : NullReadOnlySessionEnforcer.Instance; @@ -96,9 +92,16 @@ public async Task Execute(string sql) try { - await using var transaction = await _context.Database.BeginTransactionAsync( - isolationLevel - ); + // Provider-default isolation (ReadCommitted on PostgreSQL / + // SQL Server) unless the ReadOnly enforcer needs a + // specific level (SQLite). ReadUncommitted — formerly used + // for all ReadOnly queries — allows dirty reads on SQL + // Server: uncommitted data that may be rolled back, which + // the LLM would then present as fact. + var isolationLevel = _options.ReadOnly ? enforcer.ReadOnlyIsolationLevel : null; + await using var transaction = isolationLevel.HasValue + ? await _context.Database.BeginTransactionAsync(isolationLevel.Value) + : await _context.Database.BeginTransactionAsync(); try { diff --git a/src/Equibles.AgentQL.EntityFrameworkCore/Query/ReadOnly/IReadOnlySessionEnforcer.cs b/src/Equibles.AgentQL.EntityFrameworkCore/Query/ReadOnly/IReadOnlySessionEnforcer.cs index c3ece11..aed2628 100644 --- a/src/Equibles.AgentQL.EntityFrameworkCore/Query/ReadOnly/IReadOnlySessionEnforcer.cs +++ b/src/Equibles.AgentQL.EntityFrameworkCore/Query/ReadOnly/IReadOnlySessionEnforcer.cs @@ -1,3 +1,4 @@ +using System.Data; using System.Data.Common; namespace Equibles.AgentQL.EntityFrameworkCore.Query.ReadOnly; @@ -21,4 +22,11 @@ internal interface IReadOnlySessionEnforcer // DBMS — the executor logs this message once so operators see what is // and is not guaranteed, and what to do about it. string Limitation => null; + + // Isolation level for the executor's wrapper transaction in ReadOnly + // mode; null means the provider default. SQLite must override this: + // Microsoft.Data.Sqlite maps the default (Serializable) to BEGIN + // IMMEDIATE, which acquires a write lock that PRAGMA query_only itself + // refuses — ReadUncommitted maps to a plain deferred BEGIN instead. + IsolationLevel? ReadOnlyIsolationLevel => null; } diff --git a/src/Equibles.AgentQL.EntityFrameworkCore/Query/ReadOnly/ReadOnlyStatementValidator.cs b/src/Equibles.AgentQL.EntityFrameworkCore/Query/ReadOnly/ReadOnlyStatementValidator.cs index e7fad38..cfe158a 100644 --- a/src/Equibles.AgentQL.EntityFrameworkCore/Query/ReadOnly/ReadOnlyStatementValidator.cs +++ b/src/Equibles.AgentQL.EntityFrameworkCore/Query/ReadOnly/ReadOnlyStatementValidator.cs @@ -17,7 +17,10 @@ namespace Equibles.AgentQL.EntityFrameworkCore.Query.ReadOnly; // // CTE bodies are validated recursively — `WITH x AS (INSERT INTO ...) // SELECT * FROM x` is rejected because PG's writable CTEs would otherwise -// be a trivial bypass. +// be a trivial bypass. Parenthesized query heads (`(SELECT ...)`, alone or +// combined via UNION / INTERSECT / EXCEPT) are unwrapped and validated the +// same way; a statement whose shape cannot be read is rejected, never +// waved through. internal static class ReadOnlyStatementValidator { private static readonly HashSet AllowedReadVerbs = new(StringComparer.OrdinalIgnoreCase) @@ -27,6 +30,34 @@ internal static class ReadOnlyStatementValidator "TABLE", }; + // Set operations that may combine a parenthesized query head with a + // right-hand side, which is then validated as its own statement. + private static readonly HashSet SetOperationKeywords = new( + StringComparer.OrdinalIgnoreCase + ) + { + "UNION", + "INTERSECT", + "EXCEPT", + "MINUS", + }; + + // Clauses that may legally trail a parenthesized query head (ORDER BY / + // LIMIT / OFFSET / FETCH / FOR ...). Everything after them is + // expression-level content — the same trust level as the tail of a plain + // SELECT statement, which the whitelist never inspects either: an + // expression position cannot hold a DML/DDL statement in any dialect. + private static readonly HashSet TrailingClauseKeywords = new( + StringComparer.OrdinalIgnoreCase + ) + { + "ORDER", + "LIMIT", + "OFFSET", + "FETCH", + "FOR", + }; + // Returns null when the SQL is acceptable; otherwise a human-readable // explanation of which statement was rejected. The whole batch is // rejected on the first violation — partial execution under ReadOnly is @@ -38,7 +69,7 @@ public static string Validate(string sanitizedSql) foreach (var statement in SplitStatements(sanitizedSql)) { - var violation = ValidateStatement(statement); + var violation = ValidateStatement(statement, depth: 0); if (violation != null) return violation; } @@ -46,11 +77,44 @@ public static string Validate(string sanitizedSql) return null; } - private static string ValidateStatement(string statement) + // Every recursive hop (paren unwrap, set-operation tail, CTE body, WITH + // outer body) increments depth. The cap bounds stack usage: without it a + // MaxSqlLength-sized run of nested parens recurses thousands of frames + // deep and a StackOverflowException kills the process uncatchably. No + // legitimate read query nests query heads anywhere near this deep. + private const int MaxNestingDepth = 64; + + private static string ValidateStatement(string statement, int depth) { - var verb = ReadIdentifier(statement, 0, out var afterVerb); + if (depth > MaxNestingDepth) + return "Statement is nested too deeply"; + + var pos = SkipWhitespace(statement, 0); + if (pos >= statement.Length) + return "Statement is empty"; + + // A parenthesized head — `(SELECT ...)`, optionally followed by a set + // operation or a trailing clause — hides its verb inside the parens. + // Unwrap and validate the inner statement recursively, then vet what + // follows the closing paren. Waving these through unread would be a + // fail-open hole in the whitelist. + if (statement[pos] == '(') + { + var end = SkipBalancedParens(statement, pos); + if (end - pos < 2 || statement[end - 1] != ')') + return "Statement has an unterminated parenthesized query"; + + var inner = statement.Substring(pos + 1, end - pos - 2); + var violation = ValidateStatement(inner, depth + 1); + if (violation != null) + return violation; + + return ValidateParenthesizedTail(statement, end, depth + 1); + } + + var verb = ReadIdentifier(statement, pos, out var afterVerb); if (verb == null) - return null; + return $"Statement starting with '{statement[pos]}' is not permitted in ReadOnly mode"; if (AllowedReadVerbs.Contains(verb)) return null; @@ -58,12 +122,51 @@ private static string ValidateStatement(string statement) if (!verb.Equals("WITH", StringComparison.OrdinalIgnoreCase)) return $"Statement '{verb.ToUpperInvariant()}' is not permitted in ReadOnly mode"; - return ValidateWithStatement(statement, afterVerb); + return ValidateWithStatement(statement, afterVerb, depth); + } + + // Validates what follows a parenthesized query head. Only two + // continuations are read-shaped: a set operation whose right-hand side is + // validated as its own statement, or a trailing clause keyword. Anything + // else is rejected. + private static string ValidateParenthesizedTail(string sql, int pos, int depth) + { + if (depth > MaxNestingDepth) + return "Statement is nested too deeply"; + + pos = SkipWhitespace(sql, pos); + if (pos >= sql.Length) + return null; + + var keyword = ReadIdentifier(sql, pos, out var afterKeyword); + if (keyword == null) + return "Statement content after a parenthesized query is not permitted in ReadOnly mode"; + + if (SetOperationKeywords.Contains(keyword)) + { + pos = afterKeyword; + var modifier = ReadIdentifier(sql, pos, out var afterModifier); + if ( + modifier != null + && ( + modifier.Equals("ALL", StringComparison.OrdinalIgnoreCase) + || modifier.Equals("DISTINCT", StringComparison.OrdinalIgnoreCase) + ) + ) + pos = afterModifier; + + return ValidateStatement(sql.Substring(pos), depth + 1); + } + + if (TrailingClauseKeywords.Contains(keyword)) + return null; + + return $"'{keyword.ToUpperInvariant()}' after a parenthesized query is not permitted in ReadOnly mode"; } // Walks the CTE definitions of a WITH-led statement, recursively // validating each CTE body, then validates the outer body verb. - private static string ValidateWithStatement(string sql, int pos) + private static string ValidateWithStatement(string sql, int pos, int depth) { var maybeRecursive = ReadIdentifier(sql, pos, out var afterMaybe); if ( @@ -113,10 +216,13 @@ private static string ValidateWithStatement(string sql, int pos) return "WITH statement is malformed"; // Recursively validate the CTE body as its own statement, then - // skip past the parens to continue the walk. + // skip past the parens to continue the walk. An unterminated body + // paren is malformed, not something to guess at. var bodyEnd = SkipBalancedParens(sql, pos); + if (bodyEnd - pos < 2 || sql[bodyEnd - 1] != ')') + return "WITH statement is malformed"; var bodyInner = sql.Substring(pos + 1, bodyEnd - pos - 2); - var nestedViolation = ValidateStatement(bodyInner); + var nestedViolation = ValidateStatement(bodyInner, depth + 1); if (nestedViolation != null) return nestedViolation; pos = bodyEnd; @@ -130,16 +236,13 @@ private static string ValidateWithStatement(string sql, int pos) break; } - var outerVerb = ReadIdentifier(sql, pos, out _); - if (outerVerb == null) + var body = sql.Substring(pos); + if (string.IsNullOrWhiteSpace(body)) return "WITH statement has no body"; - if ( - outerVerb.Equals("WITH", StringComparison.OrdinalIgnoreCase) - || AllowedReadVerbs.Contains(outerVerb) - ) - return ValidateStatement(sql.Substring(pos)); - return $"WITH ... {outerVerb.ToUpperInvariant()} is not permitted in ReadOnly mode"; + // The outer body is validated as its own statement — it may itself be + // a WITH, an allowed read verb, or a parenthesized query. + return ValidateStatement(body, depth + 1); } // Splits the SQL into statements on top-level ';'. Quoted runs are diff --git a/src/Equibles.AgentQL.EntityFrameworkCore/Query/ReadOnly/SqliteReadOnlySessionEnforcer.cs b/src/Equibles.AgentQL.EntityFrameworkCore/Query/ReadOnly/SqliteReadOnlySessionEnforcer.cs index ae1352d..dcca8f4 100644 --- a/src/Equibles.AgentQL.EntityFrameworkCore/Query/ReadOnly/SqliteReadOnlySessionEnforcer.cs +++ b/src/Equibles.AgentQL.EntityFrameworkCore/Query/ReadOnly/SqliteReadOnlySessionEnforcer.cs @@ -1,3 +1,4 @@ +using System.Data; using System.Data.Common; namespace Equibles.AgentQL.EntityFrameworkCore.Query.ReadOnly; @@ -23,6 +24,13 @@ internal sealed class SqliteReadOnlySessionEnforcer : IReadOnlySessionEnforcer private SqliteReadOnlySessionEnforcer() { } + // Microsoft.Data.Sqlite maps Serializable (its default) to BEGIN + // IMMEDIATE, which acquires a write lock — refused once query_only is + // set. ReadUncommitted maps to a plain deferred BEGIN, which is what a + // read-only wrapper transaction needs; without a shared cache it has no + // dirty-read semantics on SQLite. + public IsolationLevel? ReadOnlyIsolationLevel => IsolationLevel.ReadUncommitted; + public Task Apply(DbConnection connection) => Execute(connection, "PRAGMA query_only = 1"); public Task Reset(DbConnection connection) => Execute(connection, "PRAGMA query_only = 0"); diff --git a/src/Equibles.AgentQL.EntityFrameworkCore/Schema/SchemaProvider.cs b/src/Equibles.AgentQL.EntityFrameworkCore/Schema/SchemaProvider.cs index 8b73d0d..c9c1f60 100644 --- a/src/Equibles.AgentQL.EntityFrameworkCore/Schema/SchemaProvider.cs +++ b/src/Equibles.AgentQL.EntityFrameworkCore/Schema/SchemaProvider.cs @@ -12,7 +12,6 @@ public class SchemaProvider : ISchemaProvider { private readonly TContext _context; private readonly AgentQLOptions _options; - private string _cachedSchema; public SchemaProvider(TContext context, AgentQLOptions options) { @@ -20,11 +19,17 @@ public SchemaProvider(TContext context, AgentQLOptions options) _options = options; } - public async Task GetSchemaDescription() + // Cached on the options singleton, not on this scoped instance — the + // schema depends only on the EF model and the options, so one model walk + // (plus one connection open for server info) serves the whole application + // lifetime instead of every request scope. + public Task GetSchemaDescription() { - if (!string.IsNullOrEmpty(_cachedSchema)) - return _cachedSchema; + return _options.SchemaCache.GetOrCreate(typeof(TContext), BuildSchemaDescription); + } + private async Task BuildSchemaDescription() + { var schema = new StringBuilder(); schema.AppendLine("=== DATABASE SCHEMA ==="); schema.AppendLine("Generated by Entity Framework Core using Code First approach"); @@ -61,8 +66,7 @@ public async Task GetSchemaDescription() AppendTableInfo(schema, table); } - _cachedSchema = schema.ToString(); - return _cachedSchema; + return schema.ToString(); } private bool HasIncludedEntity(ITable table) diff --git a/src/Equibles.AgentQL.MicrosoftAI/Extensions/ServiceCollectionExtensions.cs b/src/Equibles.AgentQL.MicrosoftAI/Extensions/ServiceCollectionExtensions.cs index 7f67f1a..0a293dd 100644 --- a/src/Equibles.AgentQL.MicrosoftAI/Extensions/ServiceCollectionExtensions.cs +++ b/src/Equibles.AgentQL.MicrosoftAI/Extensions/ServiceCollectionExtensions.cs @@ -45,7 +45,10 @@ public static IServiceCollection AddAgentQLChat( // Self-correction is added before function invocation so it ends up // the outer wrapper around the whole tool loop (see // ChatClientBuilderExtensions.UseAgentQLSelfCorrection). + // ConfigureOptions fills MaxOutputTokens only when the caller's + // per-request ChatOptions left it unset. return new ChatClientBuilder(innerClient) + .ConfigureOptions(chat => chat.MaxOutputTokens ??= options.MaxOutputTokens) .UseAgentQLSelfCorrection(selfCorrectionOptions) .UseFunctionInvocation() .Build(); @@ -65,7 +68,14 @@ private static IChatClient CreateOpenAIClient(AgentQLChatOptions options) private static IChatClient CreateAnthropicClient(AgentQLChatOptions options) { - var client = new AnthropicClient { ApiKey = options.ApiKey }; + // Only override BaseUrl when explicitly configured — the SDK's own + // default base address differs from GetEndpoint()'s OpenAI-style + // default, so falling back to GetEndpoint() here would break the + // out-of-the-box Anthropic setup. + var client = string.IsNullOrEmpty(options.Endpoint) + ? new AnthropicClient { ApiKey = options.ApiKey } + : new AnthropicClient { ApiKey = options.ApiKey, BaseUrl = options.Endpoint }; + return client.AsIChatClient(options.ModelName); } } diff --git a/src/Equibles.AgentQL.MicrosoftAI/SelfCorrectingChatClient.cs b/src/Equibles.AgentQL.MicrosoftAI/SelfCorrectingChatClient.cs index 7a782e5..5f967a9 100644 --- a/src/Equibles.AgentQL.MicrosoftAI/SelfCorrectingChatClient.cs +++ b/src/Equibles.AgentQL.MicrosoftAI/SelfCorrectingChatClient.cs @@ -212,10 +212,15 @@ private static ChatMessage BuildReminder(string question) private static string ExtractQuestion(IEnumerable messages) { // Skip any reminder this guard injected on a prior (replayed) turn so the - // restated question is the real user question, not a reminder. + // restated question is the real user question, not a reminder. Injected + // reminders always start with the tag (see BuildReminder), so a genuine + // user question that merely mentions the tag is not skipped. return messages .Where(m => m.Role == ChatRole.User) .Select(m => m.Text) - .LastOrDefault(text => text != null && !text.Contains("")); + .LastOrDefault(text => + text != null + && !text.TrimStart().StartsWith("", StringComparison.Ordinal) + ); } } diff --git a/src/Equibles.AgentQL/Configuration/AgentQLOptions.cs b/src/Equibles.AgentQL/Configuration/AgentQLOptions.cs index 12079f7..116c068 100644 --- a/src/Equibles.AgentQL/Configuration/AgentQLOptions.cs +++ b/src/Equibles.AgentQL/Configuration/AgentQLOptions.cs @@ -14,6 +14,12 @@ public class AgentQLOptions internal Dictionary EntityConfigurations { get; } = new(); + // The generated schema description depends only on the EF model and this + // options instance, so the cache shares the options' lifetime (a singleton + // in DI) instead of the scoped SchemaProvider's — one model walk per + // application, not per request scope. + internal SchemaDescriptionCache SchemaCache { get; } = new(); + public EntityConfiguration Entity() where T : class { diff --git a/src/Equibles.AgentQL/Configuration/SchemaDescriptionCache.cs b/src/Equibles.AgentQL/Configuration/SchemaDescriptionCache.cs new file mode 100644 index 0000000..7efcbe0 --- /dev/null +++ b/src/Equibles.AgentQL/Configuration/SchemaDescriptionCache.cs @@ -0,0 +1,38 @@ +using System.Collections.Concurrent; + +namespace Equibles.AgentQL.Configuration; + +// Application-lifetime cache for the generated schema description. Owned by +// AgentQLOptions (see its SchemaCache property) so scoped SchemaProvider +// instances share one cached result per container; distinct options instances +// (separate containers, or tests) never share it. Keyed by DbContext CLR type +// because DI hands the last-registered options singleton to every provider — +// a host that registers two contexts must not see one context's schema served +// for the other. The semaphore ensures a factory runs once even under +// concurrent first calls; a failed factory caches nothing and is retried. +internal sealed class SchemaDescriptionCache +{ + private readonly ConcurrentDictionary _values = new(); + private readonly SemaphoreSlim _gate = new(1, 1); + + public async Task GetOrCreate(Type contextType, Func> factory) + { + if (_values.TryGetValue(contextType, out var cached)) + return cached; + + await _gate.WaitAsync(); + try + { + if (_values.TryGetValue(contextType, out cached)) + return cached; + + var value = await factory(); + _values[contextType] = value; + return value; + } + finally + { + _gate.Release(); + } + } +} diff --git a/tests/Equibles.AgentQL.IntegrationTests/QueryExecutorReadOnlyStatementWhitelistTests.cs b/tests/Equibles.AgentQL.IntegrationTests/QueryExecutorReadOnlyStatementWhitelistTests.cs index e31eccd..5c713f1 100644 --- a/tests/Equibles.AgentQL.IntegrationTests/QueryExecutorReadOnlyStatementWhitelistTests.cs +++ b/tests/Equibles.AgentQL.IntegrationTests/QueryExecutorReadOnlyStatementWhitelistTests.cs @@ -63,6 +63,15 @@ public QueryExecutorReadOnlyStatementWhitelistTests(PostgresFixture fixture) )] [InlineData("WITH x AS (UPDATE \"Customers\" SET \"Name\" = 'M' RETURNING *) SELECT * FROM x")] [InlineData("WITH x AS (DELETE FROM \"Customers\" RETURNING *) SELECT * FROM x")] + // Parenthesized heads must be unwrapped and validated, not waved through + // because no verb is readable at position zero. + [InlineData("(INSERT INTO \"Customers\" (\"Name\", \"Tier\") VALUES ('M', 0))")] + [InlineData( + "(WITH x AS (INSERT INTO \"Customers\" (\"Name\", \"Tier\") VALUES ('M', 0) RETURNING *) SELECT * FROM x)" + )] + [InlineData( + "(SELECT 1) UNION (INSERT INTO \"Customers\" (\"Name\", \"Tier\") VALUES ('M', 0) RETURNING 1)" + )] public async Task Execute_ReadOnlyForbiddenStatement_NeutralizedAndDoesNotExecute(string sql) { await using var executorContext = Fixture.CreateContext(); @@ -108,6 +117,13 @@ public async Task Execute_ReadOnlyForbiddenStatement_NeutralizedAndDoesNotExecut // accepted even when its data mentions a forbidden keyword. [InlineData("SELECT 'please UPDATE your settings' AS message")] [InlineData("SELECT \"Name\" AS \"UPDATE_TIME\" FROM \"Customers\"")] + // Parenthesized read shapes are valid PostgreSQL and must survive the + // unwrap-and-validate path. + [InlineData("(SELECT 1 AS x)")] + [InlineData("((SELECT 1 AS x))")] + [InlineData("(SELECT 1 AS x) UNION ALL (SELECT 2)")] + [InlineData("(SELECT 1 AS x) ORDER BY 1 LIMIT 1")] + [InlineData("(WITH y AS (SELECT 1 AS a) SELECT * FROM y)")] public async Task Execute_ReadOnlyAllowedStatement_Succeeds(string sql) { await using var executorContext = Fixture.CreateContext(); @@ -148,4 +164,23 @@ public async Task Execute_QuotedCteName_ExecutesAndReturnsRows_NotNeutralized() // Seed has two Lisbon bookings and one Tokyo booking. Convert.ToInt32(result.Data![0]["BookingCount"]).Should().Be(2); } + + [Fact] + public async Task Execute_ParenthesizedSelect_ExecutesAndReturnsRows_NotNeutralized() + { + // A neutralized statement also returns Success=true with empty data — + // this proves a parenthesized head actually reaches the database and + // returns a row rather than being silently rejected. + await using var context = Fixture.CreateContext(); + var executor = new QueryExecutor( + context, + new AgentQLOptions { ReadOnly = true }, + NullLogger>.Instance + ); + + var result = await executor.Execute("(SELECT COUNT(*) AS c FROM \"Customers\")"); + + result.Success.Should().BeTrue(result.ErrorMessage); + result.Data.Should().HaveCount(1); + } } diff --git a/tests/Equibles.AgentQL.UnitTests/Query/ReadOnly/ReadOnlyStatementValidatorTests.cs b/tests/Equibles.AgentQL.UnitTests/Query/ReadOnly/ReadOnlyStatementValidatorTests.cs index 4485a88..d1aa02e 100644 --- a/tests/Equibles.AgentQL.UnitTests/Query/ReadOnly/ReadOnlyStatementValidatorTests.cs +++ b/tests/Equibles.AgentQL.UnitTests/Query/ReadOnly/ReadOnlyStatementValidatorTests.cs @@ -221,6 +221,10 @@ public void Validate_LeadingSemicolons_AreSkipped() [InlineData("WITH x")] [InlineData("WITH x AS")] [InlineData("WITH x AS (SELECT 1)")] + // Unterminated body parens must be reported as malformed, not crash the + // CTE walker with a negative-length substring. + [InlineData("WITH x AS (")] + [InlineData("WITH x AS (SELECT 1")] public void Validate_MalformedWith_ReturnsViolation(string sql) { // Each of these is missing either a CTE name, the AS keyword, the @@ -228,4 +232,99 @@ public void Validate_MalformedWith_ReturnsViolation(string sql) // guess and treats them as malformed. ReadOnlyStatementValidator.Validate(sql).Should().NotBeNull(); } + + [Theory] + [InlineData("(SELECT 1)")] + [InlineData("((SELECT 1))")] + [InlineData("(SELECT 1) UNION SELECT 2")] + [InlineData("(SELECT 1) UNION ALL (SELECT 2)")] + [InlineData("(SELECT 1) UNION DISTINCT (SELECT 2)")] + [InlineData("(SELECT 1) INTERSECT (SELECT 2)")] + [InlineData("(SELECT 1) EXCEPT (SELECT 2)")] + [InlineData("(SELECT 1) UNION (SELECT 2) UNION (SELECT 3)")] + [InlineData("(SELECT 1 AS a) ORDER BY a")] + [InlineData("(SELECT 1 AS a) LIMIT 1")] + [InlineData("(SELECT 1 AS a) ORDER BY a LIMIT 1 OFFSET 0")] + [InlineData("(VALUES (1), (2))")] + [InlineData("(TABLE foo)")] + [InlineData("(WITH x AS (SELECT 1) SELECT * FROM x)")] + public void Validate_ParenthesizedReadShapes_ReturnsNull(string sql) + { + // A parenthesized head hides its verb inside the parens — the + // validator must unwrap and accept read-shaped content rather than + // reject every paren-led statement outright. + ReadOnlyStatementValidator.Validate(sql).Should().BeNull(); + } + + [Theory] + [InlineData("(INSERT INTO foo VALUES (1))", "INSERT")] + [InlineData("((DELETE FROM foo))", "DELETE")] + [InlineData("(WITH x AS (INSERT INTO foo VALUES (1) RETURNING *) SELECT * FROM x)", "INSERT")] + [InlineData("(SELECT 1) UNION (DELETE FROM foo RETURNING 1)", "DELETE")] + [InlineData("(SELECT 1) UNION ALL DELETE FROM foo", "DELETE")] + [InlineData("(SELECT 1) INTERSECT (UPDATE foo SET a = 1 RETURNING a)", "UPDATE")] + public void Validate_WriteInsideParenthesizedStatement_IsRejected(string sql, string verb) + { + // Before parenthesized heads were unwrapped, a statement starting + // with '(' carried no readable verb and was waved through — a + // fail-open hole in the whitelist. These lock the fail-closed + // behavior in. + var violation = ReadOnlyStatementValidator.Validate(sql); + violation.Should().NotBeNull(); + violation.Should().Contain(verb); + } + + [Theory] + [InlineData("(SELECT 1")] // unterminated paren + [InlineData("(SELECT 1) INTO foo")] // non-read continuation after the head + [InlineData("(SELECT 1) )")] // stray content after the head + [InlineData("1")] // no readable verb at all + [InlineData("?")] + public void Validate_UnreadableStatementShape_IsRejected(string sql) + { + // The whitelist is fail-closed: a statement whose shape cannot be + // read as one of the permitted forms is rejected, never assumed + // harmless. + ReadOnlyStatementValidator.Validate(sql).Should().NotBeNull(); + } + + [Fact] + public void Validate_DeeplyNestedParens_IsRejectedNotStackOverflow() + { + // Recursion depth is bounded — a MaxSqlLength-sized run of nested + // parens must produce a clean violation, not an uncatchable + // StackOverflowException that kills the process. + var sql = new string('(', 2000) + "SELECT 1" + new string(')', 2000); + + ReadOnlyStatementValidator.Validate(sql).Should().NotBeNull(); + } + + [Fact] + public void Validate_ModeratelyNestedParens_IsAccepted() + { + // Realistic nesting stays well under the depth cap. + var sql = new string('(', 10) + "SELECT 1" + new string(')', 10); + + ReadOnlyStatementValidator.Validate(sql).Should().BeNull(); + } + + [Fact] + public void Validate_WithOuterBodyParenthesized_IsAccepted() + { + // PG accepts a parenthesized outer body after the CTE list. + ReadOnlyStatementValidator + .Validate("WITH x AS (SELECT 1) (SELECT * FROM x)") + .Should() + .BeNull(); + } + + [Fact] + public void Validate_WithOuterBodyParenthesizedWrite_IsRejected() + { + var violation = ReadOnlyStatementValidator.Validate( + "WITH x AS (SELECT 1) (INSERT INTO foo VALUES (1))" + ); + violation.Should().NotBeNull(); + violation.Should().Contain("INSERT"); + } }