fix: harden ReadOnly SQL validation and fix security-review findings#154
Merged
Conversation
A statement starting with '(' carried no readable verb and was waved
through the whitelist unchecked. Parenthesized heads are now unwrapped
and validated recursively, set-operation tails (UNION/INTERSECT/EXCEPT)
validate their right-hand side as a statement, and any statement whose
shape cannot be read is rejected instead of assumed harmless. Also
guards the CTE walker against unterminated body parens, which previously
threw a negative-length substring error.
…dUncommitted ReadUncommitted allowed dirty reads on SQL Server in ReadOnly mode — uncommitted data that may later be rolled back, which the LLM would present as fact. The executor now uses the provider's default isolation (ReadCommitted on PostgreSQL/SQL Server). SQLite is the exception: its default maps to BEGIN IMMEDIATE, which acquires a write lock that PRAGMA query_only itself refuses, so the SQLite enforcer requests a deferred BEGIN via a new ReadOnlyIsolationLevel hint on the enforcer interface.
SchemaProvider is scoped, so its instance-level cache re-ran the full EF model walk (plus a connection open for server info) on every request scope. The cache now lives on the AgentQLOptions singleton, keyed by options lifetime, with a semaphore so the factory runs once under concurrent first calls.
…ndpoint AgentQLChatOptions.MaxOutputTokens was documented but never read; it is now applied through ConfigureOptions when the per-request ChatOptions leaves it unset. The Anthropic factory also honors an explicitly configured Endpoint (BaseUrl) instead of silently ignoring it, while keeping the SDK default when none is set.
…uestion ExtractQuestion skipped any user message merely containing the literal '<system-reminder>' text. Injected reminders always start with the tag, so matching on the prefix keeps genuine user questions that mention the tag in the running.
…tion States explicitly that include/exclude is schema hiding, not access control — the executor does not block SQL referencing excluded tables or columns, so sensitive data must be protected at the database. Also updates the ReadOnly whitelist description for the parenthesized-head validation.
The paren-unwrap, set-operation and CTE recursion had no depth guard, so a MaxSqlLength-sized run of nested parens could recurse thousands of frames deep and kill the process with an uncatchable StackOverflow. Depth is now capped at 64 nested query heads; deeper input is rejected with the usual silent-neutralization contract.
DI hands the last-registered AgentQLOptions singleton to every provider, so a host registering two contexts would have seen one context's cached schema served for the other. The cache dictionary is now keyed by the context CLR type; a failed build caches nothing and is retried.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| // 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 } |
| // 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 }; |
Microsoft.EntityFrameworkCore.Sqlite 10.0.8 transitively brings in SQLitePCLRaw.lib.e_sqlite3 2.1.11, flagged by NuGet audit with a known high-severity vulnerability (GHSA-2m69-gcr7-jv3q), which fails the warnings-as-errors lint build. Enables central transitive pinning and lifts the native lib to 3.53.3 (versioned by SQLite release; the managed bundle/core stay on 2.1.x and accept it).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
(SELECT ...), set-operation chains, trailing clauses) were waved through unvalidated; they are now unwrapped and validated recursively, unreadable statement shapes are rejected, and recursion depth is bounded so adversarial nesting cannot crash the process with a StackOverflow.PRAGMA query_only.MaxOutputTokensoption, honors a custom Anthropic endpoint, and documents that include/exclude is schema hiding rather than access control.Code changes
src/Equibles.AgentQL.EntityFrameworkCore/Query/ReadOnly/ReadOnlyStatementValidator.cs— unwraps parenthesized heads, validates set-operation tails and trailing clauses, rejects unreadable shapes, guards unterminated CTE parens, caps nesting depth at 64.src/Equibles.AgentQL.EntityFrameworkCore/Query/QueryExecutor.cs— provider-default transaction isolation; ReadOnly isolation hint comes from the session enforcer.src/Equibles.AgentQL.EntityFrameworkCore/Query/ReadOnly/IReadOnlySessionEnforcer.cs,SqliteReadOnlySessionEnforcer.cs— newReadOnlyIsolationLevelmember; SQLite overrides to a deferred BEGIN.src/Equibles.AgentQL/Configuration/SchemaDescriptionCache.cs(new),AgentQLOptions.cs,src/Equibles.AgentQL.EntityFrameworkCore/Schema/SchemaProvider.cs— app-lifetime schema cache on the options singleton, keyed by context type, single-flight build with retry on failure.src/Equibles.AgentQL.MicrosoftAI/Extensions/ServiceCollectionExtensions.cs— appliesMaxOutputTokensviaConfigureOptions; Anthropic client usesEndpointwhen set.src/Equibles.AgentQL.MicrosoftAI/SelfCorrectingChatClient.cs— question extraction skips only messages that start with the injected reminder tag.README.md— exclusion-is-not-access-control warning; updated whitelist description.tests/…/ReadOnlyStatementValidatorTests.cs,tests/…/QueryExecutorReadOnlyStatementWhitelistTests.cs— coverage for paren shapes (allowed + rejected), depth bound, malformed WITH, and end-to-end proof parenthesized SELECTs still execute.