Feature/v4 net6 merge

[byerlikaya]

SmartRAG Pull Request

Description

Merge v4 and net6 features with refactoring: remove unnecessary wrappers, apply parameter object pattern, fix retry/Dispose patterns, add thread-safe factory initialization for Whisper.

Related Issue

N/A

Type of Change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Documentation update
• Test improvements
• Code refactoring
• Release preparation

Testing

• Unit tests pass
• Integration tests pass
• Manual testing completed
• SmartRAG.Demo runs without errors
• Build succeeds with 0 errors, 0 warnings

Checklist

• Code follows SmartRAG style guidelines
• Self-review of own code performed
• No unnecessary wrapper methods (no method that only delegates to another)
• Documentation updated (EN + TR) if API changed
• No new warnings
• Tests added/updated if applicable
• LoggerMessage definitions correct (parameter counts match format strings)
• EventId assignments unique (no conflicts)
• Code is generic and provider-agnostic
• All public APIs have XML documentation

Critical Rules Compliance

• Generic Code: No hardcoded table/database/column names
• Build Quality: 0 errors, 0 warnings, 0 messages
• Language: All code elements in English only
• SOLID/DRY: Principles followed
• Changelog: Only src/SmartRAG/ changes; no examples/ references
• Logging: No user input (queries, search terms) in log messages

Additional Context

Branch: feature/v4-net6-merge

Main changes:

• Removed unnecessary wrappers (MultiDatabaseQueryCoordinator, DocumentParserService, ResponseBuilderService, FileWatcherService)
• Inlined QueryIntelligenceCoreAsync into QueryIntelligenceAsync
• Applied StrategyRequestParams for CreateStrategyRequest (parameter object pattern)
• Simplified Dispose in ImageParserService
• Retry loop in FileWatcherService: while → for
• WhisperAudioParserService: double-check locking for factory init

Migration Guide (if breaking changes)

N/A

Performance Impact

• No performance impact
• Performance improvement
• Performance regression (explain below)

Security Considerations

• No security implications
• Security improvement
• Security concern (explain below)

In general, the fix is to avoid substring checks on the host and instead compare the parsed hostname against a precise whitelist of allowed hostnames, or perform a strict suffix match that accounts for dot boundaries. This ensures that byerlikaya.github.io is only treated as internal when it is exactly the hostname (or belongs to a clearly defined subdomain pattern), not when it is embedded inside some other domain name.

For this specific code, the minimal change without altering existing functionality intent is to: (1) determine the current host of the documentation page using window.location.hostname, (2) compare each link’s hostname to that value using strict equality, and (3) only set target="_blank"/rel=... for links whose hostname differs from the current host. This keeps all links to the same origin (including any sub‑paths) opening in the same tab, while all external sites open in a new tab. To implement this, edit the block under // ===== EXTERNAL LINKS IN NEW TAB ===== to introduce a currentHost variable and replace the .includes('byerlikaya.github.io') check with link.hostname === currentHost. No new imports or external libraries are needed; everything uses standard browser APIs.

Concretely, in docs/assets/js/script.js, around lines 1050–1056, replace the existing document.querySelectorAll('a[href^="http"]').forEach(...) block with one that first stores window.location.hostname in a constant and then does a strict comparison. No other parts of the file need to change.

In general, the way to fix “SQL query built from user-controlled sources” is to avoid passing user input directly into the SQL text. Instead, the SQL should be a static or trusted template with placeholders, and all user data should be supplied via parameters on the command object (command.Parameters.Add(...)). Where the entire query text is user-supplied (an interactive SQL console), you must either (a) restrict the input to a controlled subset of SQL via parsing/whitelisting or (b) change the API so that user input only fills parameters of server-defined queries.

In this codebase, ExecuteSQLiteQueryAsync takes a free-form query string from ExecuteQueryAsync, which itself receives the query from request.Query. The current ValidateAndSanitizeQuery performs blacklist-based checks and then passes the trimmed string through. To eliminate the CodeQL issue and conform to safer practices without breaking the external API shape more than necessary, we can:

1. Require the caller to specify a read-only base query and optional filters, and construct the final SQL server-side using parameterization. But we are not allowed to change other files beyond the shown snippets, so we shouldn’t alter DatabaseController or public DTOs.
2. Within DatabaseParserService, further constrain what is allowed for the SQLite query and execute it inside a read-only transaction with PRAGMA query_only = 1, and continue to forbid dangerous tokens. This does not create parameter placeholders out of thin air, but it enforces that even if the user manages to bypass string checks, the database connection is in read-only mode and will not execute mutations. That meaningfully reduces the injection impact.

Given the constraints (no changes to other files outside the shown snippets, and the endpoint is explicitly a custom-query feature), the best realistic fix is to make the SQLite execution path explicitly read-only and tighten ValidateAndSanitizeQuery in a way CodeQL understands as a mitigation: ensure only SELECT queries are allowed and force SQLite into query_only mode before executing. This keeps the existing functionality—“execute custom read-only queries”—while reducing the risk of harmful SQL and addressing the analyzer’s concern.

Concretely:

• In ValidateAndSanitizeQuery, after trimming, enforce that the query starts with SELECT (case-insensitive) and reject anything else. This aligns with the controller’s documented use-cases (“Get table schema information”, “data analysis”) which are read-only.
• In ExecuteSQLiteQueryAsync, before creating the command for the user query, run PRAGMA query_only = 1; on the connection to force SQLite into a mode that disallows INSERT, UPDATE, DELETE, ALTER, DROP, etc.
• Leave command.CommandText = sanitizedQuery; as is, but now it is guaranteed to be SELECT-only, and the connection is read-only at the engine level.

No new external dependencies are required; we just add a small extra command execution over the existing SqliteConnection.

These changes are all within src/SmartRAG/Services/Database/DatabaseParserService.cs in the shown regions.

---

General approach: user-supplied values should not be embedded directly into SQL text. Instead, use parameterized commands where the SQL text is fixed and only values vary, or constrain the allowed SQL shape so it cannot be used for injection/privilege escalation. Because this endpoint is meant for arbitrary SQL text, we cannot easily re‑write every custom query to be parameterized; instead, we can tighten our contract: restrict to a safe subset of read‑only SELECT queries, and make that restriction explicit and robust so that untrusted input cannot turn the query into something else. We should also avoid re‑“sanitizing” a string that is already validated, to reduce confusion.

Best targeted fix without changing visible functionality too much:

1. Strengthen ValidateAndSanitizeQuery to guarantee that only a single, simple SELECT statement is allowed:

   • Require the first non‑whitespace token to be SELECT.
   • Disallow semicolons entirely so multiple statements cannot be chained.
   • Disallow common dangerous constructs like INSERT, UPDATE, DELETE, DROP, ALTER, CREATE, EXEC, MERGE, ;, and comments, as already partly done.
   • Preserve its trimming behavior and exception throwing on invalid input.

2. Ensure that the SQL string used in ExecuteSqlServerQueryAsync is the already validated/sanitized string from ExecuteQueryAsync, and not re‑validated in a way that could accidentally re‑open risk or obscure the taint flow. That means:

   • Change ExecuteSqlServerQueryAsync to accept a sanitized query string (renaming its parameter and removing the redundant ValidateAndSanitizeQuery call).
   • Use that parameter directly as command.CommandText.

3. No changes are required in DatabaseController for taint handling; it already delegates to ExecuteQueryAsync. The main security control lives in ValidateAndSanitizeQuery.

Concretely:

• In src/SmartRAG/Services/Database/DatabaseParserService.cs:
  • Update ValidateAndSanitizeQuery to enforce that the query:
    • Starts with SELECT (ignoring leading whitespace).
    • Contains no ; at all (so only one statement).
  • Update ExecuteSqlServerQueryAsync signature and body to take a sanitizedQuery and remove the internal ValidateAndSanitizeQuery call.
  • Update the call site in ExecuteQueryAsync to pass the already sanitized query to ExecuteSqlServerQueryAsync.

No new imports are needed; we can reuse Regex and string utilities already in use in this file.

---

General approach: avoid executing raw, user-supplied SQL directly as CommandText. Instead, restrict the kinds of queries that can be executed and/or require the client to provide query templates with parameters, then bind user-supplied values via NpgsqlParameters (or equivalent for other DBs). At minimum, enforce a whitelist of allowed statements (for example, only SELECT queries) using robust parsing, and avoid any server-side concatenation with user input.

Best fix consistent with existing functionality and limited scope of the snippets:

1. Tighten ValidateAndSanitizeQuery so it only allows read-only SELECT statements and rejects anything else. This does not make the code fully safe (it’s still a free-form SELECT), but it materially reduces risk and addresses the core CodeQL concern about running arbitrary SQL, while staying within the design of a generic query executor.
2. Ensure that this stricter validation is also used when constructing queries for PostgreSQL in ExecutePostgreSqlQueryAsync (it already is) and for other DB types via ExecuteQueryAsync.
3. Optionally, make it explicit in the code (and doc comments) that only SELECT is allowed, and enforce that there is a single statement (disallow ; in the text).

Since we must not rewrite the API to use parameterized templates (that would be a large behavioral change) and can only touch the shown snippets, the single best change is to harden ValidateAndSanitizeQuery to enforce a strict whitelist policy and protect against multiple statements, while leaving the rest of the logic intact.

Concretely:

• Modify ValidateAndSanitizeQuery in src/SmartRAG/Services/Database/DatabaseParserService.cs:
  • After trimming, ensure the query starts with SELECT (case-insensitive, ignoring leading whitespace and possibly parentheses).
  • Reject any query containing ; that could indicate multiple statements.
  • Keep and possibly extend the dangerous keyword/pattern checks.
• No changes are needed in DatabaseController.cs, because it already passes request.Query through the service API. The main risk is in how the service executes the query.

We do not introduce any new external libraries; all changes use System and System.Text.RegularExpressions, which are already being used.

---