diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml new file mode 100644 index 0000000..467c975 --- /dev/null +++ b/.github/workflows/ci.yml @@ -0,0 +1,105 @@ +name: CI + +on: + push: + branches: [master] + pull_request: + +permissions: + contents: read + +jobs: + test: + name: Build and test against all four databases + runs-on: ubuntu-latest + timeout-minutes: 30 + + services: + postgres: + image: postgres:16-alpine + env: + POSTGRES_PASSWORD: pw + ports: ["5432:5432"] + options: >- + --health-cmd "pg_isready -U postgres" --health-interval 5s --health-retries 30 + + mysql: + image: mysql:8.4 + env: + MYSQL_ROOT_PASSWORD: pw + MYSQL_DATABASE: t + ports: ["3306:3306"] + options: >- + --health-cmd "mysqladmin ping -h 127.0.0.1 -uroot -ppw" --health-interval 5s --health-retries 30 + + sqlserver: + image: mcr.microsoft.com/mssql/server:2022-latest + env: + ACCEPT_EULA: "Y" + MSSQL_SA_PASSWORD: Str0ng!Passw0rd + ports: ["1433:1433"] + options: >- + --health-cmd "/opt/mssql-tools18/bin/sqlcmd -S localhost -U sa -P 'Str0ng!Passw0rd' -C -Q 'SELECT 1'" + --health-interval 10s --health-retries 30 + + oracle: + image: gvenzl/oracle-free:23-slim-faststart + env: + ORACLE_PASSWORD: pw + ports: ["1521:1521"] + options: >- + --health-cmd healthcheck.sh --health-interval 10s --health-retries 60 + + env: + DOTNET_NOLOGO: "true" + SQLMCP_TEST_POSTGRES: Host=localhost;Port=5432;Username=postgres;Password=pw + SQLMCP_TEST_MYSQL: Server=localhost;Port=3306;Database=t;User Id=root;Password=pw + SQLMCP_TEST_SQLSERVER: Server=localhost,1433;User Id=sa;Password=Str0ng!Passw0rd;TrustServerCertificate=true + SQLMCP_TEST_ORACLE: Data Source=localhost:1521/FREEPDB1;User Id=system;Password=pw + + steps: + - uses: actions/checkout@v4 + + - uses: actions/setup-dotnet@v4 + with: + dotnet-version: 10.0.x + + # Service containers can't take server flags; the tests create a function with binary logging on. + - name: Allow MySQL function creation + run: docker exec ${{ job.services.mysql.id }} mysql -uroot -ppw -e "SET GLOBAL log_bin_trust_function_creators = 1" + + - run: dotnet restore sql-mcp.sln + + - run: dotnet build sql-mcp.sln --no-restore -c Release -warnaserror + + - name: Check for vulnerable packages + run: | + dotnet list sql-mcp.sln package --vulnerable --include-transitive | tee vulnerable.txt + ! grep -q "has the following vulnerable packages" vulnerable.txt + + # Fails if any integration test was skipped, so a database that didn't come up can't pass silently. + - name: Test + run: | + dotnet test sql-mcp.sln --no-build -c Release | tee test.txt + grep -qE "Skipped: +0," test.txt + + docker: + name: Build and smoke-test the image + runs-on: ubuntu-latest + timeout-minutes: 15 + steps: + - uses: actions/checkout@v4 + + - run: docker build -t sql-mcp . + + - name: Refuses to start without a token + run: "! docker run --rm sql-mcp" + + - name: Serves health, rejects requests without the token, runs as non-root + run: | + docker run -d --name app -e App__AuthToken=ci-token -p 8080:8080 sql-mcp + for i in $(seq 30); do curl -sf localhost:8080/health && break; sleep 1; done + test "$(curl -s -o /dev/null -w '%{http_code}' localhost:8080/health)" = 200 + test "$(curl -s -o /dev/null -w '%{http_code}' -X POST localhost:8080/mcp)" = 401 + test "$(docker exec app id -u)" != 0 + docker exec app dotnet sql-mcp.dll --healthcheck diff --git a/Dockerfile b/Dockerfile index 44477f9..971bb2d 100644 --- a/Dockerfile +++ b/Dockerfile @@ -1,7 +1,7 @@ FROM mcr.microsoft.com/dotnet/sdk:10.0 AS build WORKDIR /src -COPY *.csproj . -RUN dotnet restore +COPY sql-mcp.csproj . +RUN dotnet restore sql-mcp.csproj COPY . . RUN dotnet publish sql-mcp.csproj -c Release -o /app/publish --no-restore @@ -11,4 +11,8 @@ COPY --from=build /app/publish . ENV ASPNETCORE_ENVIRONMENT=Production ENV App__Urls=http://*:8080 EXPOSE 8080 +# The base image's unprivileged "app" user; 8080 needs no root. +USER $APP_UID +HEALTHCHECK --interval=30s --timeout=10s --start-period=10s --retries=3 \ + CMD ["dotnet", "sql-mcp.dll", "--healthcheck"] ENTRYPOINT ["dotnet", "sql-mcp.dll"] diff --git a/Http/HealthCheck.cs b/Http/HealthCheck.cs new file mode 100644 index 0000000..5f91c7f --- /dev/null +++ b/Http/HealthCheck.cs @@ -0,0 +1,43 @@ +using System.Text.RegularExpressions; + +namespace SqlMcp.Http; + +/// +/// "sql-mcp --healthcheck" asks the running server's /health endpoint and exits 0 if it +/// answers 200, else 1. The container image uses it as its HEALTHCHECK, since the +/// ASP.NET base image has no curl or wget. +/// +public static partial class HealthCheck +{ + public static async Task RunAsync(IConfiguration config) + { + var url = LocalUrl(config["App:Urls"] ?? HttpSecurity.DefaultUrls); + try + { + using var client = new HttpClient { Timeout = TimeSpan.FromSeconds(5) }; + using var response = await client.GetAsync(new Uri(new Uri(url), "/health")); + return response.IsSuccessStatusCode ? 0 : 1; + } + catch (Exception ex) when (ex is HttpRequestException or TaskCanceledException or UriFormatException) + { + Console.Error.WriteLine($"Health check of {url} failed: {ex.Message}"); + return 1; + } + } + + /// + /// The first bound URL, with a wildcard host ("*", "+", "0.0.0.0", "[::]") replaced by + /// localhost so it can be dialled. + /// + public static string LocalUrl(string urls) + { + var url = HttpSecurity.ParseUrls(urls).First().TrimEnd('/'); + var m = HostPattern().Match(url); + return m.Success && m.Groups["host"].Value is "*" or "+" or "0.0.0.0" or "[::]" + ? m.Groups["scheme"].Value + "localhost" + m.Groups["rest"].Value + : url; + } + + [GeneratedRegex(@"^(?[a-z]+://)(?\[[^\]]*\]|[^:/]+)(?.*)$", RegexOptions.IgnoreCase)] + private static partial Regex HostPattern(); +} diff --git a/Models/QueryRequest.cs b/Models/QueryRequest.cs index f9cbabd..6d533e0 100644 --- a/Models/QueryRequest.cs +++ b/Models/QueryRequest.cs @@ -1,7 +1,9 @@ namespace SqlMcp.Models; +using System.Text.Json; using System.Text.Json.Serialization; public record QueryRequest( [property: JsonPropertyName("dbName")] string DbName, - [property: JsonPropertyName("sql")] string Sql); \ No newline at end of file + [property: JsonPropertyName("sql")] string Sql, + [property: JsonPropertyName("parameters")] Dictionary? Parameters = null); diff --git a/Program.cs b/Program.cs index 7e1fcd0..348aab7 100644 --- a/Program.cs +++ b/Program.cs @@ -6,6 +6,17 @@ // appsettings.json next to the executable rather than relative to the cwd. var contentRoot = AppContext.BaseDirectory; +if (args.Contains("--healthcheck")) +{ + var config = new ConfigurationBuilder() + .SetBasePath(contentRoot) + .AddJsonFile("appsettings.json", optional: true) + .AddEnvironmentVariables() + .AddCommandLine(args.Where(a => a != "--healthcheck").ToArray()) + .Build(); + return await HealthCheck.RunAsync(config); +} + if (args.Contains("--stdio")) { var builder = Host.CreateApplicationBuilder(new HostApplicationBuilderSettings @@ -22,7 +33,9 @@ .WithStdioServerTransport() .WithToolsFromAssembly(); - await builder.Build().RunAsync(); + var host = builder.Build(); + ExitOnInvalidConfig(() => host.Services.GetRequiredService()); + await host.RunAsync(); } else { @@ -42,13 +55,28 @@ var app = builder.Build(); - HttpSecurity.ValidateStartup(app.Configuration); + ExitOnInvalidConfig(() => + { + HttpSecurity.ValidateStartup(app.Configuration); + app.Services.GetRequiredService(); + }); app.UseHttpSecurity(); app.MapMcp("/mcp"); + // Liveness: the process is up. Open, so probes need no token. app.MapGet("/health", () => Results.Ok(new { status = "healthy" })); + // Readiness: every database connects and passes the login audit. Requires the + // token when one is set, since it names the databases and why any are down. + app.MapGet("/health/ready", async (QueryService svc, CancellationToken ct) => + { + var databases = await svc.CheckDatabasesAsync(ct); + var ready = databases.Values.All(v => v == "ok"); + return Results.Json(new { status = ready ? "ready" : "unavailable", databases }, + statusCode: ready ? StatusCodes.Status200OK : StatusCodes.Status503ServiceUnavailable); + }); + app.MapPost("/api/query", async (QueryService svc, QueryRequest req, CancellationToken ct) => { var result = await svc.ExecuteAsync(req, ct); @@ -61,5 +89,22 @@ await app.RunAsync(urls); } +return 0; + +// Resolving QueryService validates the config, so a bad one stops the server before it +// serves anything, with the problems listed and no stack trace. +static void ExitOnInvalidConfig(Action validate) +{ + try + { + validate(); + } + catch (InvalidOperationException ex) + { + Console.Error.WriteLine(ex.Message); + Environment.Exit(1); + } +} + // Visible to WebApplicationFactory in the tests. public partial class Program; diff --git a/README.md b/README.md index ccc7cce..0b6f2e9 100644 --- a/README.md +++ b/README.md @@ -89,8 +89,13 @@ All settings live in `appsettings.json`. | `App:AuthToken` | _(none)_ | Bearer token required on `/mcp` and `/api/*`. Set it with env var `App__AuthToken` rather than in the file. Required to listen beyond localhost. | | `App:AllowedOrigins` | `[]` | Browser origins allowed to call the server. Requests carrying any other `Origin` header are rejected. | | `App:AllowUnauthenticated` | `false` | Allow listening beyond localhost without `AuthToken`, for when a proxy in front authenticates. | -| `App:MaxRows` | `1000` | Maximum rows returned per query. Response includes `"truncated": true` when the limit is hit. | -| `App:QueryTimeoutSeconds` | `30` | Per-query timeout in seconds. | +| `App:MaxRows` | `1000` | Maximum rows returned per query, and tables per `list_tables`. Response includes `"truncated": true` when the limit is hit. | +| `App:QueryTimeoutSeconds` | `30` | Per-query timeout in seconds. Also how long a query waits for a free slot when `MaxConcurrentQueries` are already running. | +| `App:MaxValueLength` | `10000` | Longest string returned for a single value, in characters. Longer text and binary values are cut off and marked `… [truncated: N characters in total]`. | +| `App:MaxResultBytes` | `4000000` | Approximate size limit for one query's result. Rows stop being added once it's reached, and the response says `"truncated": true`. | +| `App:MaxConcurrentQueries` | `8` | Queries that may run at once against each database. Further queries wait up to `QueryTimeoutSeconds`, then fail with "Database 'x' is busy". | + +The configuration is checked at startup. An invalid limit, a duplicate or missing database name, a missing or unknown `Provider`, an unparseable connection string, or a `PasswordEnvVar` without a `{password}` placeholder stops the server with every problem listed. A database whose `PasswordEnvVar` isn't set is logged as a warning and only that database is unavailable. ### Database entries @@ -98,7 +103,7 @@ Each entry under `Databases` has: | Field | Required | Description | |-------|----------|-------------| -| `Name` | yes | Identifier used in API calls. | +| `Name` | yes | Identifier used in API calls. Unique, ignoring case; agents may use any case. | | `Provider` | yes | One of `SqlServer`, `Postgres`, `MySql`, `Oracle`. | | `ConnectionString` | yes | Standard ADO.NET connection string. Use `{password}` as a placeholder if using `PasswordEnvVar`. | | `PasswordEnvVar` | no | Environment variable name whose value is substituted for `{password}` in the connection string. | @@ -126,6 +131,10 @@ Example: For local development, put real connection strings in `appsettings.Development.json` (gitignored). +### Logging + +Each tool call is logged by `SqlMcp.Services.QueryService`: the database, the SQL (or the schema lookup), the names of any parameters, how long it took and how many rows it returned. Parameter values are never logged. Failures are logged as warnings. `appsettings.Production.json` keeps this category at `Information` while everything else is at `Warning`; set it to `Warning` to log only failures. In stdio mode all logs go to stderr. + ### Environment variable overrides Any `appsettings.json` value can be overridden with an environment variable using `__` as the section separator: @@ -184,6 +193,20 @@ A table the login can't read is reported as not found. |----------|------|-------------| | `dbName` | string | Database name from `list_databases`. | | `sql` | string | A single `SELECT` or `WITH` query. See [Security](#security). | +| `parameters` | object, optional | Values for placeholders in `sql`, by name. Values may be strings, numbers, booleans or `null`. | + +Put values in `parameters` instead of writing them into the SQL. They are sent to the database separately from the query text, so a value like `O'Brien` needs no escaping and a value like `x' OR '1'='1` is only ever compared as a string. Placeholders use each database's own syntax: + +| Provider | Placeholder | +|----------|-------------| +| SQL Server, Postgres, MySQL | `@name` | +| Oracle | `:name` | + +```json +{ "dbName": "prod", "sql": "SELECT id, total FROM orders WHERE customer = @customer AND total >= @min", "parameters": { "customer": "O'Brien", "min": 100 } } +``` + +Names are letters, digits and underscores (a leading `@` or `:` in the key is ignored), up to 100 per query. A placeholder can be used more than once. Whole numbers are bound as 64-bit integers, other numbers as decimals. On Postgres, strings are sent untyped so the server infers the type from where the placeholder is used, as it does for a quoted literal: `WHERE created_at > @since` with `"2024-01-01"` compares dates. Where Postgres can't infer a type (`@x IS NULL`), cast the placeholder: `CAST(@x AS text) IS NULL`. Returns the result as JSON text: @@ -191,6 +214,14 @@ Returns the result as JSON text: { "columns": ["id", "name"], "rows": [{ "id": 1, "name": "Ada" }], "truncated": false } ``` +Every column keeps its value: unnamed columns are called `column1`, `column2`, ... and repeated names get a suffix (`SELECT a.id, b.id` returns `id` and `id_2`). Values JSON can't hold are converted: +- `NaN` and infinite floats become the strings `"NaN"`, `"Infinity"`, `"-Infinity"`. +- Oracle `NUMBER`s beyond 28 significant digits are rounded (`1/3`), and integers too large for a decimal come back as strings. +- Binary values are base64. +- Types without a JSON form (Postgres `inet`, ranges, geometric types; Oracle intervals) are returned as their text. + +A value the driver can't read at all, such as a SQL Server `geography` column (whose .NET types aren't shipped), is replaced by a note saying so, and the rest of the result is unaffected. Select it as text instead, e.g. `geo.ToString()`. + Example over Streamable HTTP (the server runs stateless, so no `initialize` handshake is needed for a one-off call): ```bash @@ -205,12 +236,20 @@ curl -s http://localhost:8080/mcp \ **`POST /api/query`** (HTTP mode only) — plain REST equivalent of `query_database`, for non-MCP callers. ```json -{ "dbName": "prod", "sql": "SELECT 1 AS test" } +{ "dbName": "prod", "sql": "SELECT name FROM users WHERE id = @id", "parameters": { "id": 7 } } ``` ### Health check -**`GET /health`** (HTTP mode only) — returns `200 {"status":"healthy"}`. Suitable for load balancer and orchestrator probes. +**`GET /health`** (HTTP mode only): liveness. Returns `200 {"status":"healthy"}` while the process is up, and needs no token, so it suits load balancer and liveness probes. + +**`GET /health/ready`** (HTTP mode only): readiness. Connects to every configured database, runs the login audit and opens the read-only transaction, without running a query. It returns `200` when all of them succeed, otherwise `503`, with each database's status: + +```json +{ "status": "unavailable", "databases": { "prod": "ok", "reporting": "Database 'reporting' is disabled because its login can modify data. The server log lists why." } } +``` + +It requires the bearer token when `App:AuthToken` is set, since it names the databases. Readiness checks aren't written to the query log. --- @@ -250,6 +289,8 @@ Queries come from AI agents, so every query passes three layers before anything **HTTP transport.** By default the server listens on `localhost` only. To listen beyond it, set `App:AuthToken`: clients then send `Authorization: Bearer ` on `/mcp` and `/api/*` (`/health` stays open for probes). The server refuses to start on a non-loopback address without a token unless `App:AllowUnauthenticated` is `true`. Requests that carry an `Origin` header not listed in `App:AllowedOrigins` are rejected, so a web page can't reach a local server through DNS rebinding. The stdio transport is only reachable by the process that launched it. +**Values in queries.** Agents should pass values through `parameters` (see [`query_database`](#query_database)) rather than splicing them into the SQL. Bound values can't change a query's structure, whatever quotes or keywords they contain. Statement validation still applies to the SQL text itself, and catches injected statements in SQL an agent built by concatenating strings. + **Credentials.** Connection strings can be committed safely: put `{password}` where the password goes and name the environment variable to fill it from with `PasswordEnvVar`. --- @@ -281,7 +322,7 @@ docker run -p 8080:8080 \ sql-mcp ``` -The image listens on all interfaces inside the container, so it won't start without `App__AuthToken`. +The image listens on all interfaces inside the container, so it won't start without `App__AuthToken`. It runs as the base image's unprivileged `app` user (uid 1654), so a mounted `appsettings.json` must be readable by that user. Its `HEALTHCHECK` runs `dotnet sql-mcp.dll --healthcheck`, which calls `/health` on the configured port and exits 0 or 1. Use the same command for health checks anywhere curl isn't installed. ### Linux — systemd service @@ -327,10 +368,13 @@ sql-mcp/ ├── Program.cs # Transport selection (stdio / HTTP), endpoints ├── Tools/SqlTools.cs # MCP tool definitions ├── Services/QueryService.cs # Query execution, read-only transaction, row limiting +├── Services/QueryParameters.cs # Bind-parameter parsing and binding +├── Services/ResultValues.cs # Converts database values to plain JSON ├── Services/SchemaCatalog.cs # Per-provider catalog queries for list_tables / describe_table ├── Services/SqlGuard.cs # Provider-aware statement validation ├── Services/LoginAudit.cs # Per-provider check that the login can only read ├── Http/HttpSecurity.cs # Bearer token, Origin check, safe binding +├── Http/HealthCheck.cs # --healthcheck mode for container probes ├── Models/ │ ├── DbConfig.cs # Database config model │ ├── QueryRequest.cs # Request DTO @@ -342,6 +386,7 @@ sql-mcp/ ├── appsettings.json # Default config ├── appsettings.Production.json # Production log levels ├── Dockerfile +├── .github/workflows/ci.yml # Build, tests against all four databases, image smoke test └── tests/SqlMcp.Tests/ # Validator unit tests, database integration tests ``` @@ -364,3 +409,8 @@ dotnet test ``` (MySQL with binary logging enabled also needs `log_bin_trust_function_creators=1` to create the test function.) + +CI (`.github/workflows/ci.yml`) runs the same suite on every push and pull request, against Postgres 16, MySQL 8.4, SQL Server 2022 and Oracle 23 Free service containers. The job fails if any test is skipped, so a database that didn't start can't let it pass. CI also builds with warnings treated as errors, fails on vulnerable packages, and checks that the Docker image: +- refuses to start without a token +- serves `/health` and returns 401 on `/mcp` without the token +- runs as non-root diff --git a/Services/QueryParameters.cs b/Services/QueryParameters.cs new file mode 100644 index 0000000..6f4524e --- /dev/null +++ b/Services/QueryParameters.cs @@ -0,0 +1,86 @@ +using Npgsql; +using NpgsqlTypes; +using Oracle.ManagedDataAccess.Client; +using SqlMcp.Models; +using System.Data.Common; +using System.Text.Json; +using System.Text.RegularExpressions; + +namespace SqlMcp.Services; + +/// +/// Bind parameters for agent queries. Values travel to the database separately from the +/// SQL text, so a value can't end a string literal or add a statement, and quotes in +/// values need no escaping. Placeholders use the provider's own syntax: @name on SQL +/// Server, Postgres and MySQL, :name on Oracle. +/// +public static partial class QueryParameters +{ + public const int MaxCount = 100; + + [GeneratedRegex("^[A-Za-z_][A-Za-z0-9_]{0,127}$")] + private static partial Regex ValidName(); + + /// + /// Converts JSON values to CLR values: strings, numbers (long, else decimal, else + /// double), booleans and null. A leading @ or : on a name is dropped. + /// + public static (List> Parameters, string? Error) Parse(IReadOnlyDictionary? raw) + { + var parameters = new List>(); + if (raw is null || raw.Count == 0) + return (parameters, null); + if (raw.Count > MaxCount) + return (parameters, $"At most {MaxCount} parameters are allowed."); + + var seen = new HashSet(StringComparer.OrdinalIgnoreCase); + foreach (var (key, json) in raw) + { + var name = key.TrimStart('@', ':'); + if (!ValidName().IsMatch(name)) + return (parameters, $"Parameter name '{key}' is invalid: use letters, digits and underscores, starting with a letter or underscore."); + if (!seen.Add(name)) + return (parameters, $"Parameter '{name}' is given more than once."); + + object? value = json.ValueKind switch + { + JsonValueKind.String => json.GetString(), + JsonValueKind.True => true, + JsonValueKind.False => false, + JsonValueKind.Null => null, + JsonValueKind.Number when json.TryGetInt64(out var l) => l, + JsonValueKind.Number when json.TryGetDecimal(out var m) => m, + JsonValueKind.Number => json.GetDouble(), + _ => Invalid.Value + }; + if (value == Invalid.Value) + return (parameters, $"Parameter '{name}' must be a string, number, boolean or null, not {json.ValueKind.ToString().ToLowerInvariant()}."); + + parameters.Add(new(name, value)); + } + return (parameters, null); + } + + private sealed class Invalid + { + public static readonly Invalid Value = new(); + } + + public static void Add(DbCommand cmd, DbProvider provider, string name, object? value) + { + if (cmd is OracleCommand oracle) + oracle.BindByName = true; + + var parameter = cmd.CreateParameter(); + parameter.ParameterName = provider == DbProvider.Oracle ? name : "@" + name; + parameter.Value = value ?? DBNull.Value; + + // Npgsql sends strings as text, and Postgres won't compare text with a date, + // uuid or numeric column. Sent untyped, the server infers the type from context + // as it does for a quoted literal. + if (parameter is NpgsqlParameter npgsql && value is string or null) + npgsql.NpgsqlDbType = NpgsqlDbType.Unknown; + + cmd.Parameters.Add(parameter); + } +} diff --git a/Services/QueryService.cs b/Services/QueryService.cs index 69eeaed..575546a 100644 --- a/Services/QueryService.cs +++ b/Services/QueryService.cs @@ -5,6 +5,8 @@ using System.Collections.Concurrent; using System.Data; using System.Data.Common; +using System.Diagnostics; +using System.Text.Json; namespace SqlMcp.Services; @@ -22,20 +24,120 @@ public class QueryService private readonly List _dbs; private readonly int _maxRows; private readonly int _queryTimeoutSeconds; + private readonly int _maxValueLength; + private readonly long _maxResultBytes; private readonly ILogger _logger; private readonly ConcurrentDictionary Findings)> _audits = new(); + private readonly Dictionary _slots; + /// + /// Reads and validates the configuration; throws InvalidOperationException listing + /// every problem, so a bad config stops the server at startup. + /// public QueryService(IConfiguration config, ILogger? logger = null) { - _dbs = config.GetSection("Databases").Get>() ?? []; - _maxRows = config.GetValue("App:MaxRows", 1000); - _queryTimeoutSeconds = config.GetValue("App:QueryTimeoutSeconds", 30); _logger = logger ?? NullLogger.Instance; + var errors = new List(); + try + { + _dbs = config.GetSection("Databases").Get>() ?? []; + } + catch (InvalidOperationException ex) + { + // e.g. an unknown Provider; the binder names the bad value. + throw new InvalidOperationException($"Invalid configuration: {ex.Message} {ex.InnerException?.Message}".Trim(), ex); + } + + _maxRows = Positive(config, "App:MaxRows", 1000, errors); + _queryTimeoutSeconds = Positive(config, "App:QueryTimeoutSeconds", 30, errors); + _maxValueLength = Positive(config, "App:MaxValueLength", 10_000, errors); + _maxResultBytes = Positive(config, "App:MaxResultBytes", 4_000_000, errors); + int maxConcurrent = Positive(config, "App:MaxConcurrentQueries", 8, errors); + + // The binder turns a missing or misspelled Provider into SqlServer, the enum's + // default, so check the raw values. + foreach (var (entry, i) in config.GetSection("Databases").GetChildren().Select((e, i) => (e, i))) + { + var provider = entry["Provider"]; + if (!Enum.TryParse(provider, ignoreCase: true, out var parsed) || !Enum.IsDefined(parsed) || int.TryParse(provider, out _)) + errors.Add($"{(string.IsNullOrWhiteSpace(entry["Name"]) ? $"Databases:{i}" : $"Database '{entry["Name"]}'")} has " + + $"{(string.IsNullOrWhiteSpace(provider) ? "no Provider" : $"unknown Provider '{provider}'")}; " + + $"use one of {string.Join(", ", Enum.GetNames())}."); + } + + ValidateDatabases(errors); + if (errors.Count > 0) + throw new InvalidOperationException("Invalid configuration:" + string.Concat(errors.Select(e => $"{Environment.NewLine} - {e}"))); + + _slots = _dbs.ToDictionary(d => d.Name, _ => new SemaphoreSlim(maxConcurrent, maxConcurrent)); + + if (_dbs.Count == 0) + _logger.LogWarning("No databases are configured; add entries under Databases."); foreach (var db in _dbs.Where(d => d.AllowWritableLogin)) _logger.LogWarning("Database {Database} has AllowWritableLogin set; its login is not checked for write access.", db.Name); + foreach (var db in _dbs.Where(d => MissingPassword(d) is not null)) + _logger.LogWarning("Database {Database} can't be queried: {Problem}", db.Name, MissingPassword(db)); + } + + private static int Positive(IConfiguration config, string key, int fallback, List errors) + { + int value; + try + { + value = config.GetValue(key, fallback); + } + catch (InvalidOperationException) + { + errors.Add($"{key} must be a whole number, not '{config[key]}'."); + return fallback; + } + if (value < 1) + errors.Add($"{key} must be at least 1, not {value}."); + return value; } + private void ValidateDatabases(List errors) + { + var names = new HashSet(StringComparer.OrdinalIgnoreCase); + for (int i = 0; i < _dbs.Count; i++) + { + var db = _dbs[i]; + var label = string.IsNullOrWhiteSpace(db.Name) ? $"Databases:{i}" : $"Database '{db.Name}'"; + if (string.IsNullOrWhiteSpace(db.Name)) + errors.Add($"{label} has no Name."); + else if (!names.Add(db.Name)) + errors.Add($"{label} is configured more than once (names are compared ignoring case)."); + + if (string.IsNullOrWhiteSpace(db.ConnectionString)) + { + errors.Add($"{label} has no ConnectionString."); + continue; + } + + try + { + var builder = db.GetFactory().CreateConnectionStringBuilder()!; + builder.ConnectionString = db.ConnectionString; + } + catch (Exception ex) + { + errors.Add($"{label} has an invalid {db.Provider} ConnectionString: {ex.Message}"); + } + + if (!string.IsNullOrEmpty(db.PasswordEnvVar) && !db.ConnectionString.Contains("{password}", StringComparison.OrdinalIgnoreCase)) + errors.Add($"{label} sets PasswordEnvVar but its ConnectionString has no {{password}} placeholder."); + } + } + + /// + /// A missing password variable disables only its own database, so the others stay up. + /// + private static string? MissingPassword(DbConfig db) => + !string.IsNullOrEmpty(db.PasswordEnvVar) && string.IsNullOrEmpty(Environment.GetEnvironmentVariable(db.PasswordEnvVar)) + ? $"environment variable {db.PasswordEnvVar}, which holds its password, is not set." + : null; + public async Task> ExecuteAsync(QueryRequest req, CancellationToken ct = default) { if (string.IsNullOrWhiteSpace(req.DbName) || req.Sql is null) @@ -49,10 +151,19 @@ public async Task> ExecuteAsync(QueryRequest req, Ca if (!verdict.IsAllowed) return new(null!, verdict.Error!); - return await RunAsync(db, async newCommand => + var (parameters, parameterError) = QueryParameters.Parse(req.Parameters); + if (parameterError is not null) + return new(null!, parameterError); + + var operation = parameters.Count == 0 + ? $"query: {verdict.Sql}" + : $"query with parameters {string.Join(", ", parameters.Select(p => p.Key))}: {verdict.Sql}"; + return await RunAsync(db, operation, async newCommand => { await using var cmd = newCommand(); cmd.CommandText = verdict.Sql; + foreach (var (name, value) in parameters) + QueryParameters.Add(cmd, db.Provider, name, value); return await ReadResultAsync(cmd, ct); }, ct); } @@ -66,7 +177,7 @@ public async Task> ListTablesAsync(string dbName, stri if (db == null) return new(null!, UnknownDatabase(dbName)); - return await RunAsync(db, async newCommand => + return await RunAsync(db, schema is null ? "list_tables" : $"list_tables in {schema}", async newCommand => { var tables = await SchemaCatalog.FindTablesAsync(newCommand, db.Provider, NullIfBlank(schema), null, _maxRows, ct); return tables.Count > _maxRows @@ -95,7 +206,7 @@ public async Task> DescribeTableAsync( (schema, table) = (table[..dot], table[(dot + 1)..]); string? notFound = null; - var result = await RunAsync(db, async newCommand => + var result = await RunAsync(db, $"describe_table {(schema is null ? table : $"{schema}.{table}")}", async newCommand => { var matches = await SchemaCatalog.FindTablesAsync(newCommand, db.Provider, schema, table, 50, ct); @@ -124,7 +235,56 @@ public async Task> DescribeTableAsync( /// Connects, checks the login, and runs work inside a read-only transaction that is /// always rolled back. Commands from newCommand are bound to that transaction. /// - private async Task> RunAsync(DbConfig db, Func, Task> work, CancellationToken ct) + private async Task> RunAsync(DbConfig db, string operation, Func, Task> work, CancellationToken ct, bool log = true) + { + if (MissingPassword(db) is { } missing) + return new(default, $"Database '{db.Name}' is unavailable: {missing}"); + + // Bounds the connections one busy agent can hold open against a database. Callers + // wait up to the query timeout for a free slot. + var slot = _slots[db.Name]; + if (!await slot.WaitAsync(TimeSpan.FromSeconds(_queryTimeoutSeconds), ct)) + { + _logger.LogWarning("Rejected {Operation} on {Database}: all query slots stayed busy for {Seconds} s.", operation, db.Name, _queryTimeoutSeconds); + return new(default, $"Database '{db.Name}' is busy: too many queries are already running. Try again shortly."); + } + + var started = Stopwatch.GetTimestamp(); + try + { + var result = await RunWithRetryAsync(db, work, ct); + var elapsed = Stopwatch.GetElapsedTime(started).TotalMilliseconds; + if (!log) + return result; + if (result.IsError) + _logger.LogWarning("Failed {Operation} on {Database} after {ElapsedMs:F0} ms: {Error}", operation, db.Name, elapsed, result.Error); + else + _logger.LogInformation("Ran {Operation} on {Database} in {ElapsedMs:F0} ms, {Rows} rows{Truncated}.", + operation, db.Name, elapsed, CountRows(result.Value), IsTruncated(result.Value) ? " (truncated)" : ""); + return result; + } + catch (OperationCanceledException) + { + _logger.LogInformation("Cancelled {Operation} on {Database} after {ElapsedMs:F0} ms.", operation, db.Name, Stopwatch.GetElapsedTime(started).TotalMilliseconds); + throw; + } + finally + { + slot.Release(); + } + } + + private static int CountRows(object? value) => value switch + { + QueryResult q => q.Rows.Length, + TableList t => t.Tables.Length, + TableDescription d => d.Columns.Length, + _ => 0 + }; + + private static bool IsTruncated(object? value) => value is QueryResult { Truncated: true } or TableList { Truncated: true }; + + private async Task> RunWithRetryAsync(DbConfig db, Func, Task> work, CancellationToken ct) { for (int attempt = 0; ; attempt++) { @@ -174,7 +334,8 @@ private async Task> RunOnceAsync(DbConfig db, Func _dbs.FirstOrDefault(d => d.Name == name); + private DbConfig? FindDatabase(string? name) => + _dbs.FirstOrDefault(d => d.Name == name) ?? _dbs.FirstOrDefault(d => string.Equals(d.Name, name, StringComparison.OrdinalIgnoreCase)); private static string UnknownDatabase(string? name) => $"Unknown database '{name}'. Call list_databases to see available names."; @@ -205,16 +366,30 @@ private async Task IsLoginReadOnlyAsync(DbConfig db, DbConnection conn, Ca public string[] ListDatabases() => [.. _dbs.Select(d => d.Name)]; + /// + /// Connects to every database, runs the login audit and opens the read-only + /// transaction, without running a query. Returns "ok" or the error for each. + /// + public async Task> CheckDatabasesAsync(CancellationToken ct = default) + { + var checks = _dbs.Select(async db => + { + var result = await RunAsync(db, "readiness check", _ => Task.FromResult(true), ct, log: false); + return (db.Name, Status: result.Error ?? "ok"); + }); + return (await Task.WhenAll(checks)).ToDictionary(c => c.Name, c => c.Status); + } + private async Task ReadResultAsync(DbCommand cmd, CancellationToken ct) { await using var reader = await cmd.ExecuteReaderAsync(ct); + ResultValues.Prepare(reader); - var columns = Enumerable.Range(0, reader.FieldCount) - .Select(reader.GetName) - .ToArray(); + var columns = UniqueColumnNames(Enumerable.Range(0, reader.FieldCount).Select(reader.GetName)); var rows = new List>(); var truncated = false; + long size = columns.Sum(c => c.Length + 4L); while (await reader.ReadAsync(ct)) { if (rows.Count == _maxRows) @@ -223,15 +398,73 @@ private async Task ReadResultAsync(DbCommand cmd, CancellationToken break; } - var row = new Dictionary(); + var row = new Dictionary(columns.Length); + long rowSize = 2; for (int i = 0; i < columns.Length; i++) - row[columns[i]] = reader.IsDBNull(i) ? null : reader.GetValue(i); + { + var value = LimitLength(ResultValues.Read(reader, i)); + row[columns[i]] = value; + rowSize += columns[i].Length + 4 + EstimateSize(value); + } + + // Stop before the response outgrows App:MaxResultBytes; the first row is + // always returned, since its values are already capped. + if (rows.Count > 0 && size + rowSize > _maxResultBytes) + { + truncated = true; + break; + } + size += rowSize; rows.Add(row); } return new QueryResult(columns, [.. rows], truncated); } + /// + /// Rows are keyed by column name, so "SELECT 1, 2" or "SELECT a.id, b.id" would + /// otherwise lose all but one value. Unnamed columns become column1, column2, ... + /// and repeats get a suffix: id, id_2. + /// + public static string[] UniqueColumnNames(IEnumerable names) + { + var given = names.Select((n, i) => string.IsNullOrWhiteSpace(n) || n == "?column?" ? $"column{i + 1}" : n).ToArray(); + var used = new HashSet(StringComparer.OrdinalIgnoreCase); + var result = new string[given.Length]; + for (int i = 0; i < given.Length; i++) + { + var name = given[i]; + for (int n = 2; !used.Add(name); n++) + name = $"{given[i]}_{n}"; + result[i] = name; + } + return result; + } + + /// Caps long strings and binary values at App:MaxValueLength characters. + private object? LimitLength(object? value) + { + switch (value) + { + case string s when s.Length > _maxValueLength: + return $"{s[.._maxValueLength]}… [truncated: {s.Length} characters in total]"; + case byte[] b when (b.Length + 2) / 3 * 4 > _maxValueLength: + var keep = _maxValueLength / 4 * 3; + return $"{Convert.ToBase64String(b, 0, keep)}… [truncated: {b.Length} bytes in total, base64]"; + default: + return value; + } + } + + private static long EstimateSize(object? value) => value switch + { + null => 4, + string s => s.Length + 2, + byte[] b => (b.Length + 2) / 3 * 4 + 2, + System.Collections.IEnumerable => JsonSerializer.Serialize(value).Length, + _ => 24 + }; + /// /// Opens a transaction the database itself refuses to write in, where the provider /// has such a mode. SQL Server has none, so there the guarantee is SqlGuard plus the diff --git a/Services/ResultValues.cs b/Services/ResultValues.cs new file mode 100644 index 0000000..7113252 --- /dev/null +++ b/Services/ResultValues.cs @@ -0,0 +1,81 @@ +using Oracle.ManagedDataAccess.Client; +using System.Collections; +using System.Data.Common; +using System.Globalization; +using System.Numerics; + +namespace SqlMcp.Services; + +/// +/// Turns database values into values that serialize to plain JSON. Providers return +/// types that don't: Oracle NUMBERs beyond decimal precision, NaN and infinite doubles, +/// Postgres inet, ranges and geometric types, SQL Server CLR types. One unreadable +/// value must not fail the whole query, so it is replaced by a note saying how to +/// select it instead. +/// +public static class ResultValues +{ + public static void Prepare(DbDataReader reader) + { + // 1/3 is a 40-digit NUMBER; without this, reading it throws InvalidCastException + // rather than rounding to the 28 digits a decimal holds. + if (reader is OracleDataReader oracle) + oracle.SuppressGetDecimalInvalidCastException = true; + } + + public static object? Read(DbDataReader reader, int ordinal) + { + if (reader.IsDBNull(ordinal)) + return null; + + try + { + return Normalize(reader.GetValue(ordinal)); + } + catch (Exception ex) when (ex is not OperationCanceledException) + { + return ReadFallback(reader, ordinal, ex); + } + } + + private static object? ReadFallback(DbDataReader reader, int ordinal, Exception error) + { + try + { + if (reader is OracleDataReader oracle) + return oracle.GetOracleValue(ordinal).ToString(); + } + catch (Exception ex) when (ex is not OperationCanceledException) + { + // Fall through to the note. + } + + string type; + try + { + type = reader.GetDataTypeName(ordinal); + } + catch + { + type = "this"; + } + var reason = error.Message.Split('\n')[0].Trim().TrimEnd('.'); + return $"(cannot read {type} value: {reason}. Convert it to a string in the query.)"; + } + + public static object? Normalize(object? value) => value switch + { + null or DBNull => null, + string or bool or char or Guid => value, + byte or sbyte or short or ushort or int or uint or long or ulong or decimal => value, + DateTime or DateTimeOffset or DateOnly or TimeOnly or TimeSpan => value, + double d => double.IsFinite(d) ? d : d.ToString(CultureInfo.InvariantCulture), + float f => float.IsFinite(f) ? f : f.ToString(CultureInfo.InvariantCulture), + BigInteger b => b.ToString(CultureInfo.InvariantCulture), + byte[] => value, + BitArray bits => string.Concat(bits.Cast().Select(b => b ? '1' : '0')), + IDictionary dict => dict.Keys.Cast().ToDictionary(k => Convert.ToString(k, CultureInfo.InvariantCulture)!, k => Normalize(dict[k])), + IEnumerable items => items.Cast().Select(Normalize).ToList(), + _ => Convert.ToString(value, CultureInfo.InvariantCulture) + }; +} diff --git a/Services/SchemaCatalog.cs b/Services/SchemaCatalog.cs index 98bf789..4cf09be 100644 --- a/Services/SchemaCatalog.cs +++ b/Services/SchemaCatalog.cs @@ -1,4 +1,3 @@ -using Oracle.ManagedDataAccess.Client; using SqlMcp.Models; using System.Data.Common; @@ -139,10 +138,10 @@ public static async Task> FindTablesAsync( AND ({p}p_any_table = 1 OR LOWER(table_name) = LOWER({p}p_table)) ORDER BY schema_name, table_name """; - AddParameter(cmd, provider, "p_any_schema", schema is null ? 1 : 0); - AddParameter(cmd, provider, "p_schema", schema ?? ""); - AddParameter(cmd, provider, "p_any_table", table is null ? 1 : 0); - AddParameter(cmd, provider, "p_table", table ?? ""); + QueryParameters.Add(cmd, provider, "p_any_schema", schema is null ? 1 : 0); + QueryParameters.Add(cmd, provider, "p_schema", schema ?? ""); + QueryParameters.Add(cmd, provider, "p_any_table", table is null ? 1 : 0); + QueryParameters.Add(cmd, provider, "p_table", table ?? ""); var tables = new List(); await using var reader = await cmd.ExecuteReaderAsync(ct); @@ -164,8 +163,8 @@ public static async Task GetColumnsAsync( DbProvider.Oracle => OracleColumns, _ => throw new ArgumentOutOfRangeException(nameof(provider)) }; - AddParameter(cmd, provider, "p_schema", table.Schema); - AddParameter(cmd, provider, "p_table", table.Name); + QueryParameters.Add(cmd, provider, "p_schema", table.Schema); + QueryParameters.Add(cmd, provider, "p_table", table.Name); var columns = new List(); await using var reader = await cmd.ExecuteReaderAsync(ct); @@ -179,15 +178,4 @@ public static async Task GetColumnsAsync( } return [.. columns]; } - - private static void AddParameter(DbCommand cmd, DbProvider provider, string name, object value) - { - if (cmd is OracleCommand oracle) - oracle.BindByName = true; - - var parameter = cmd.CreateParameter(); - parameter.ParameterName = provider == DbProvider.Oracle ? name : "@" + name; - parameter.Value = value; - cmd.Parameters.Add(parameter); - } } diff --git a/Tools/SqlTools.cs b/Tools/SqlTools.cs index 5c0a439..5275399 100644 --- a/Tools/SqlTools.cs +++ b/Tools/SqlTools.cs @@ -39,12 +39,16 @@ public async Task DescribeTable( Serialize(await svc.DescribeTableAsync(dbName, table, schema, cancellationToken)); [McpServerTool(Name = "query_database", ReadOnly = true)] - [Description("Execute a read-only SELECT query on the specified database and return the columns and rows as JSON.")] + [Description("Execute a read-only SELECT query on the specified database and return the columns and rows as JSON. " + + "Pass values through parameters rather than writing them into the SQL: they are bound by the database, " + + "so quotes in values need no escaping and can't change the query.")] public async Task QueryDatabase( [Description("Database name from list_databases.")] string dbName, - [Description("SELECT query to execute.")] string sql, + [Description("SELECT query to execute. Refer to parameters as @name on SQL Server, Postgres and MySQL, and as :name on Oracle.")] string sql, + [Description("Values for the query's placeholders, by name without the @ or :, e.g. {\"customer\": \"O'Brien\", \"minTotal\": 100}. " + + "Values may be strings, numbers, booleans or null. Optional.")] Dictionary? parameters = null, CancellationToken cancellationToken = default) => - Serialize(await svc.ExecuteAsync(new QueryRequest(dbName, sql), cancellationToken)); + Serialize(await svc.ExecuteAsync(new QueryRequest(dbName, sql, parameters), cancellationToken)); private static string Serialize(QueryService.Result result) { diff --git a/appsettings.Production.json b/appsettings.Production.json index bcdd3fc..de6eb6e 100644 --- a/appsettings.Production.json +++ b/appsettings.Production.json @@ -2,7 +2,8 @@ "Logging": { "LogLevel": { "Default": "Warning", - "Microsoft.AspNetCore": "Warning" + "Microsoft.AspNetCore": "Warning", + "SqlMcp.Services.QueryService": "Information" } } } diff --git a/appsettings.json b/appsettings.json index c481562..38222b6 100644 --- a/appsettings.json +++ b/appsettings.json @@ -4,30 +4,34 @@ "AuthToken": "", "AllowedOrigins": [], "MaxRows": 1000, - "QueryTimeoutSeconds": 30 + "QueryTimeoutSeconds": 30, + "MaxValueLength": 10000, + "MaxResultBytes": 4000000, + "MaxConcurrentQueries": 8 }, "Databases": [ { "Name": "sampleSql", "Provider": "SqlServer", - "ConnectionString": "Server=localhost;Database=testdb;User Id=sa;Password={password};TrustServerCertificate=true;", + "ConnectionString": "Server=localhost;Database=testdb;User Id=agent;Password={password};TrustServerCertificate=true;", "PasswordEnvVar": "DB_SAMPLESQL_PASSWORD" }, { "Name": "sampleOracle", "Provider": "Oracle", - "ConnectionString": "Data Source=localhost:1521/ORCL;User Id=myuser;Authentication=Kerberos;" + "ConnectionString": "Data Source=localhost:1521/ORCL;User Id=agent;Password={password};", + "PasswordEnvVar": "DB_SAMPLEORACLE_PASSWORD" }, { "Name": "samplePostgres", "Provider": "Postgres", - "ConnectionString": "Host=localhost;Database=testdb;Username=postgres;Password={password};", + "ConnectionString": "Host=localhost;Database=testdb;Username=agent;Password={password};", "PasswordEnvVar": "DB_SAMPLEPG_PASSWORD" }, { "Name": "sampleMySql", "Provider": "MySql", - "ConnectionString": "Server=localhost;Database=testdb;User Id=root;Password={password};", + "ConnectionString": "Server=localhost;Database=testdb;User Id=agent;Password={password};", "PasswordEnvVar": "DB_SAMPLEMYSQL_PASSWORD" } ], diff --git a/tests/SqlMcp.Tests/HealthTests.cs b/tests/SqlMcp.Tests/HealthTests.cs new file mode 100644 index 0000000..f848dfe --- /dev/null +++ b/tests/SqlMcp.Tests/HealthTests.cs @@ -0,0 +1,52 @@ +using Microsoft.AspNetCore.Hosting; +using Microsoft.AspNetCore.Mvc.Testing; +using SqlMcp.Http; +using System.Net; +using System.Net.Http.Headers; +using System.Text.Json; + +namespace SqlMcp.Tests; + +public class HealthTests +{ + [Theory] + [InlineData("http://*:8080", "http://localhost:8080")] + [InlineData("http://+:8080", "http://localhost:8080")] + [InlineData("http://0.0.0.0:9000/", "http://localhost:9000")] + [InlineData("http://[::]:8080", "http://localhost:8080")] + [InlineData("http://127.0.0.1:8080;http://*:9090", "http://127.0.0.1:8080")] + [InlineData("http://[::1]:8080", "http://[::1]:8080")] + [InlineData("http://localhost", "http://localhost")] + public void Dials_the_first_url_on_localhost(string urls, string expected) => + Assert.Equal(expected, HealthCheck.LocalUrl(urls)); + + private static HttpClient Client(string? token = null) => + new WebApplicationFactory().WithWebHostBuilder(b => b.UseSetting("App:AuthToken", token ?? "")).CreateClient(); + + // The sample config's databases have no passwords set, so none are ready. + [Fact] + public async Task Readiness_reports_each_database() + { + using var client = Client(); + + var response = await client.GetAsync("/health/ready"); + + Assert.Equal(HttpStatusCode.ServiceUnavailable, response.StatusCode); + var body = JsonDocument.Parse(await response.Content.ReadAsStringAsync()).RootElement; + Assert.Equal("unavailable", body.GetProperty("status").GetString()); + Assert.Contains("DB_SAMPLEPG_PASSWORD", body.GetProperty("databases").GetProperty("samplePostgres").GetString()); + } + + [Fact] + public async Task Readiness_requires_the_token_but_liveness_does_not() + { + using var client = Client("secret"); + + Assert.Equal(HttpStatusCode.Unauthorized, (await client.GetAsync("/health/ready")).StatusCode); + Assert.Equal(HttpStatusCode.OK, (await client.GetAsync("/health")).StatusCode); + + var request = new HttpRequestMessage(HttpMethod.Get, "/health/ready"); + request.Headers.Authorization = new AuthenticationHeaderValue("Bearer", "secret"); + Assert.Equal(HttpStatusCode.ServiceUnavailable, (await client.SendAsync(request)).StatusCode); + } +} diff --git a/tests/SqlMcp.Tests/QueryParametersTests.cs b/tests/SqlMcp.Tests/QueryParametersTests.cs new file mode 100644 index 0000000..b5cccb6 --- /dev/null +++ b/tests/SqlMcp.Tests/QueryParametersTests.cs @@ -0,0 +1,70 @@ +using SqlMcp.Services; +using System.Text.Json; + +namespace SqlMcp.Tests; + +public class QueryParametersTests +{ + private static Dictionary Json(string json) => + JsonSerializer.Deserialize>(json)!; + + [Fact] + public void Converts_json_values() + { + var (parameters, error) = QueryParameters.Parse(Json( + """{"s": "O'Brien", "i": 42, "big": 9007199254740993, "d": 2.5, "e": 1e400, "t": true, "f": false, "n": null}""")); + + Assert.Null(error); + var values = parameters.ToDictionary(p => p.Key, p => p.Value); + Assert.Equal("O'Brien", values["s"]); + Assert.Equal(42L, values["i"]); + Assert.Equal(9007199254740993L, values["big"]); + Assert.Equal(2.5m, values["d"]); + Assert.Equal(true, values["t"]); + Assert.Equal(false, values["f"]); + Assert.Null(values["n"]); + Assert.IsType(values["e"]); + } + + [Fact] + public void Drops_a_leading_placeholder_prefix() + { + var (parameters, error) = QueryParameters.Parse(Json("""{"@a": 1, ":b": 2, "c_3": 3}""")); + + Assert.Null(error); + Assert.Equal(["a", "b", "c_3"], parameters.Select(p => p.Key)); + } + + [Fact] + public void Accepts_no_parameters() + { + Assert.Empty(QueryParameters.Parse(null).Parameters); + Assert.Empty(QueryParameters.Parse(Json("{}")).Parameters); + } + + [Theory] + [InlineData("""{"a b": 1}""", "invalid")] + [InlineData("""{"1a": 1}""", "invalid")] + [InlineData("""{"a;DROP": 1}""", "invalid")] + [InlineData("""{"": 1}""", "invalid")] + [InlineData("""{"a": 1, "@a": 2}""", "more than once")] + [InlineData("""{"a": 1, "A": 2}""", "more than once")] + [InlineData("""{"a": [1, 2]}""", "not array")] + [InlineData("""{"a": {"b": 1}}""", "not object")] + public void Rejects_bad_names_and_values(string json, string expected) + { + var (_, error) = QueryParameters.Parse(Json(json)); + + Assert.NotNull(error); + Assert.Contains(expected, error); + } + + [Fact] + public void Limits_the_count() + { + var many = Enumerable.Range(0, QueryParameters.MaxCount + 1) + .ToDictionary(i => $"p{i}", i => JsonSerializer.SerializeToElement(i)); + + Assert.Contains("At most", QueryParameters.Parse(many).Error); + } +} diff --git a/tests/SqlMcp.Tests/QueryServiceConfigTests.cs b/tests/SqlMcp.Tests/QueryServiceConfigTests.cs new file mode 100644 index 0000000..9a143de --- /dev/null +++ b/tests/SqlMcp.Tests/QueryServiceConfigTests.cs @@ -0,0 +1,110 @@ +using Microsoft.Extensions.Configuration; +using SqlMcp.Services; +using System.Collections; +using System.Net; +using System.Numerics; + +namespace SqlMcp.Tests; + +public class QueryServiceConfigTests +{ + private static QueryService Create(params (string Key, string? Value)[] settings) => + new(new ConfigurationBuilder() + .AddInMemoryCollection(settings.Select(s => new KeyValuePair(s.Key, s.Value))) + .Build()); + + private static (string, string?)[] Db(int i, string name, string provider = "Postgres", string cs = "Host=localhost;Username=agent") => + [($"Databases:{i}:Name", name), ($"Databases:{i}:Provider", provider), ($"Databases:{i}:ConnectionString", cs)]; + + [Fact] + public void Accepts_a_valid_config() => + Create([.. Db(0, "a"), .. Db(1, "b", "SqlServer", "Server=localhost;User Id=agent;Password={password}"), ("Databases:1:PasswordEnvVar", "SQLMCP_TEST_UNSET_VAR")]); + + [Theory] + [InlineData("App:MaxRows", "0", "App:MaxRows must be at least 1")] + [InlineData("App:QueryTimeoutSeconds", "0", "App:QueryTimeoutSeconds must be at least 1")] + [InlineData("App:MaxConcurrentQueries", "-1", "App:MaxConcurrentQueries must be at least 1")] + [InlineData("App:MaxValueLength", "lots", "App:MaxValueLength must be a whole number")] + public void Rejects_bad_limits(string key, string value, string expected) + { + var ex = Assert.Throws(() => Create([.. Db(0, "a"), (key, value)])); + Assert.Contains(expected, ex.Message); + } + + [Fact] + public void Lists_every_database_problem_at_once() + { + var ex = Assert.Throws(() => Create( + [ + .. Db(0, "prod"), + .. Db(1, "PROD"), + .. Db(2, ""), + .. Db(3, "empty", cs: ""), + .. Db(4, "garbled", cs: "Hots=localhost"), + .. Db(5, "noplaceholder"), ("Databases:5:PasswordEnvVar", "PW"), + ])); + + Assert.Contains("Database 'PROD' is configured more than once", ex.Message); + Assert.Contains("Databases:2 has no Name", ex.Message); + Assert.Contains("Database 'empty' has no ConnectionString", ex.Message); + Assert.Contains("Database 'garbled' has an invalid Postgres ConnectionString", ex.Message); + Assert.Contains("Database 'noplaceholder' sets PasswordEnvVar but its ConnectionString has no {password} placeholder", ex.Message); + } + + [Theory] + [InlineData("Sqlite", "Database 'a' has unknown Provider 'Sqlite'; use one of SqlServer, Oracle, Postgres, MySql.")] + [InlineData("1", "Database 'a' has unknown Provider '1'")] + [InlineData("", "Database 'a' has no Provider")] + public void Rejects_a_missing_or_unknown_provider(string provider, string expected) + { + var ex = Assert.Throws(() => Create(Db(0, "a", provider))); + Assert.Contains(expected, ex.Message); + } + + [Fact] + public void Accepts_a_provider_in_any_case() => + Create(Db(0, "a", "postgres")); + + [Fact] + public async Task A_missing_password_variable_disables_only_its_database() + { + var svc = Create([.. Db(0, "a", cs: "Host=localhost;Username=agent;Password={password}"), ("Databases:0:PasswordEnvVar", "SQLMCP_TEST_UNSET_VAR")]); + + var result = await svc.ExecuteAsync(new("a", "SELECT 1")); + + Assert.Equal("Database 'a' is unavailable: environment variable SQLMCP_TEST_UNSET_VAR, which holds its password, is not set.", result.Error); + } + + [Fact] + public async Task Matches_database_names_ignoring_case() + { + var svc = Create([.. Db(0, "Reporting", cs: "Host=localhost;Username=agent;Password={password}"), ("Databases:0:PasswordEnvVar", "SQLMCP_TEST_UNSET_VAR")]); + + var result = await svc.ExecuteAsync(new("reporting", "SELECT 1")); + + Assert.StartsWith("Database 'Reporting' is unavailable", result.Error); + } + + [Theory] + [InlineData(new[] { "id", "name" }, new[] { "id", "name" })] + [InlineData(new[] { "id", "id", "ID" }, new[] { "id", "id_2", "ID_3" })] + [InlineData(new[] { "", "?column?", "column1" }, new[] { "column1", "column2", "column1_2" })] + [InlineData(new[] { "a", "a_2", "a" }, new[] { "a", "a_2", "a_3" })] + public void Makes_column_names_unique(string[] names, string[] expected) => + Assert.Equal(expected, QueryService.UniqueColumnNames(names)); + + [Fact] + public void Normalizes_values_json_cannot_represent() + { + Assert.Equal("NaN", ResultValues.Normalize(double.NaN)); + Assert.Equal("-Infinity", ResultValues.Normalize(float.NegativeInfinity)); + Assert.Equal(1.5, ResultValues.Normalize(1.5)); + Assert.Equal("123456789012345678901234567890", ResultValues.Normalize(BigInteger.Parse("123456789012345678901234567890"))); + Assert.Equal("10.0.0.1", ResultValues.Normalize(IPAddress.Parse("10.0.0.1"))); + Assert.Equal("101", ResultValues.Normalize(new BitArray(new[] { true, false, true }))); + Assert.Equal(new List { 1, "NaN", null }, ResultValues.Normalize(new object?[] { 1, double.NaN, DBNull.Value })); + Assert.Equal(new Dictionary { ["k"] = "v" }, ResultValues.Normalize(new Hashtable { ["k"] = "v" })); + Assert.Equal("text", ResultValues.Normalize("text")); + Assert.Equal(new byte[] { 1, 2 }, ResultValues.Normalize(new byte[] { 1, 2 })); + } +} diff --git a/tests/SqlMcp.Tests/QueryServiceIntegrationTests.cs b/tests/SqlMcp.Tests/QueryServiceIntegrationTests.cs index b88b9d2..6256963 100644 --- a/tests/SqlMcp.Tests/QueryServiceIntegrationTests.cs +++ b/tests/SqlMcp.Tests/QueryServiceIntegrationTests.cs @@ -1,7 +1,9 @@ using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.Logging; using SqlMcp.Models; using SqlMcp.Services; using System.Data.Common; +using System.Text.Json; namespace SqlMcp.Tests; @@ -29,15 +31,21 @@ private static DbConfig RequireDb(DbProvider provider) // The admin logins the tests run as can write, so the audit is opted out of unless // a test is about the audit. - private static QueryService Service(DbConfig db, int maxRows = 1000, bool allowWritableLogin = true) => - new(new ConfigurationBuilder().AddInMemoryCollection(new Dictionary + private static QueryService Service(DbConfig db, int maxRows = 1000, bool allowWritableLogin = true, + Dictionary? settings = null, ILogger? logger = null) + { + var config = new Dictionary { ["App:MaxRows"] = maxRows.ToString(), ["Databases:0:Name"] = db.Name, ["Databases:0:Provider"] = db.Provider.ToString(), ["Databases:0:ConnectionString"] = db.ConnectionString, ["Databases:0:AllowWritableLogin"] = allowWritableLogin.ToString(), - }).Build()); + }; + foreach (var (key, value) in settings ?? []) + config[key] = value; + return new(new ConfigurationBuilder().AddInMemoryCollection(config).Build(), logger); + } private const string ReadOnlyUser = "sqlmcp_ro"; private const string ReadOnlyPassword = "Ro_passw0rd1"; @@ -565,4 +573,251 @@ public async Task Schema_tools_refuse_a_login_that_can_write(DbProvider provider Assert.Contains("is disabled", (await svc.ListTablesAsync("test", null)).Error); Assert.Contains("is disabled", (await svc.DescribeTableAsync("test", Table, null)).Error); } + + private static string P(DbProvider provider, string name) => (provider == DbProvider.Oracle ? ":" : "@") + name; + + private static Dictionary Params(string json) => + JsonSerializer.Deserialize>(json)!; + + private static QueryRequest Query(string sql, string parameters) => new("test", sql, Params(parameters)); + + [SkippableTheory] + [MemberData(nameof(Providers))] + public async Task Binds_a_value_that_contains_quotes(DbProvider provider) + { + var db = RequireDb(provider); + await SeedAsync(db, $"INSERT INTO {Table} (id, name) VALUES (4, 'O''Brien')"); + + var result = await Service(db).ExecuteAsync(Query( + $"SELECT id FROM {Table} WHERE name = {P(provider, "name")}", """{"name": "O'Brien"}""")); + + Assert.True(result.Error is null, result.Error); + Assert.Equal(4, Convert.ToInt32(Assert.Single(result.Value!.Rows).Values.Single())); + } + + // Written into the SQL, each of these would change the query. Bound, they are just + // strings that no row's name equals. + [SkippableTheory] + [MemberData(nameof(Providers))] + public async Task Bound_values_cannot_change_the_query(DbProvider provider) + { + var db = RequireDb(provider); + await SeedAsync(db); + var svc = Service(db); + + foreach (var attack in new[] { "x' OR '1'='1", "'; DELETE FROM sqlmcp_fixture; --", "x' UNION SELECT 99 FROM sqlmcp_fixture --", "\\'; DROP TABLE sqlmcp_fixture; --" }) + { + var result = await svc.ExecuteAsync(new("test", $"SELECT id FROM {Table} WHERE name = {P(provider, "name")}", + new() { ["name"] = JsonSerializer.SerializeToElement(attack) })); + + Assert.True(result.Error is null, $"{attack}: {result.Error}"); + Assert.Empty(result.Value!.Rows); + } + Assert.Equal(3, await CountAsync(db)); + } + + [SkippableTheory] + [MemberData(nameof(Providers))] + public async Task Binds_numbers_and_null(DbProvider provider) + { + var db = RequireDb(provider); + await SeedAsync(db); + var svc = Service(db); + var sql = $"SELECT id FROM {Table} WHERE id >= {P(provider, "min")} ORDER BY id"; + + Assert.Equal(2, (await svc.ExecuteAsync(Query(sql, """{"min": 2}"""))).Value!.Rows.Length); + Assert.Single((await svc.ExecuteAsync(Query(sql, """{"min": 2.5}"""))).Value!.Rows); + + var none = await svc.ExecuteAsync(Query($"SELECT id FROM {Table} WHERE name = {P(provider, "name")}", """{"name": null}""")); + Assert.True(none.Error is null, none.Error); + Assert.Empty(none.Value!.Rows); + } + + [SkippableTheory] + [MemberData(nameof(Providers))] + public async Task Binds_several_parameters_including_one_used_twice(DbProvider provider) + { + var db = RequireDb(provider); + await SeedAsync(db); + + var result = await Service(db).ExecuteAsync(Query( + $"SELECT id FROM {Table} WHERE (id = {P(provider, "a")} OR id = {P(provider, "b")}) AND id <> {P(provider, "a")} + 10 ORDER BY id", + """{"a": 1, "b": 3}""")); + + Assert.True(result.Error is null, result.Error); + Assert.Equal([1, 3], result.Value!.Rows.Select(r => Convert.ToInt32(r.Values.Single()))); + } + + // Postgres won't compare text with a date; strings are sent untyped so it infers the type. + [SkippableFact] + public async Task Postgres_infers_the_type_of_a_string_value() + { + var db = RequireDb(DbProvider.Postgres); + + var result = await Service(db).ExecuteAsync(Query( + "SELECT 1 AS one WHERE DATE '2024-01-02' > @d AND 5 > @n", """{"d": "2024-01-01", "n": "4"}""")); + + Assert.True(result.Error is null, result.Error); + Assert.Single(result.Value!.Rows); + } + + [SkippableTheory] + [MemberData(nameof(Providers))] + public async Task Reports_a_missing_parameter(DbProvider provider) + { + var db = RequireDb(provider); + await SeedAsync(db); + + var result = await Service(db).ExecuteAsync(Query( + $"SELECT id FROM {Table} WHERE id = {P(provider, "missing")}", """{"other": 1}""")); + + Assert.NotNull(result.Error); + } + + [SkippableTheory] + [MemberData(nameof(Providers))] + public async Task Rejects_bad_parameters_before_connecting(DbProvider provider) + { + var db = RequireDb(provider); + + var result = await Service(db).ExecuteAsync(Query($"SELECT 1 AS one{From(provider)}", """{"a b": 1}""")); + + Assert.Contains("invalid", result.Error); + } + + [SkippableTheory] + [MemberData(nameof(Providers))] + public async Task Keeps_every_value_when_column_names_repeat(DbProvider provider) + { + var db = RequireDb(provider); + + var result = await Service(db).ExecuteAsync(new("test", $"SELECT 1 AS a, 2 AS a, 3 AS b{From(provider)}")); + + Assert.True(result.Error is null, result.Error); + Assert.Equal(["a", "a_2", "b"], result.Value!.Columns.Select(c => c.ToLowerInvariant())); + Assert.Equal([1, 2, 3], result.Value.Rows.Single().Values.Select(Convert.ToInt32)); + } + + [SkippableTheory] + [InlineData(DbProvider.Postgres, "SELECT 1, 2")] + [InlineData(DbProvider.SqlServer, "SELECT 1, 2")] + public async Task Names_unnamed_columns(DbProvider provider, string sql) + { + var db = RequireDb(provider); + + var result = await Service(db).ExecuteAsync(new("test", sql)); + + Assert.Equal(["column1", "column2"], result.Value!.Columns); + } + + [SkippableTheory] + [InlineData(DbProvider.Postgres, "SELECT repeat('x', 500) AS v")] + [InlineData(DbProvider.MySql, "SELECT REPEAT('x', 500) AS v")] + [InlineData(DbProvider.SqlServer, "SELECT REPLICATE('x', 500) AS v")] + [InlineData(DbProvider.Oracle, "SELECT RPAD('x', 500, 'x') AS v FROM dual")] + public async Task Truncates_long_values(DbProvider provider, string sql) + { + var db = RequireDb(provider); + + var result = await Service(db, settings: new() { ["App:MaxValueLength"] = "100" }).ExecuteAsync(new("test", sql)); + + var value = Assert.IsType(result.Value!.Rows.Single().Values.Single()); + Assert.StartsWith(new string('x', 100) + "…", value); + Assert.Contains("[truncated: 500 characters in total]", value); + } + + [SkippableTheory] + [MemberData(nameof(Providers))] + public async Task Stops_adding_rows_at_the_result_size_limit(DbProvider provider) + { + var db = RequireDb(provider); + await SeedAsync(db); + + var result = await Service(db, settings: new() { ["App:MaxResultBytes"] = "60" }) + .ExecuteAsync(new("test", $"SELECT id, name FROM {Table} ORDER BY id")); + + Assert.True(result.Error is null, result.Error); + Assert.InRange(result.Value!.Rows.Length, 1, 2); + Assert.True(result.Value.Truncated); + } + + // Each of these failed the whole query, or threw while serializing it. + [SkippableTheory] + [InlineData(DbProvider.Oracle, "SELECT 1/3 AS v FROM dual", "0.33333")] + [InlineData(DbProvider.Oracle, "SELECT 123456789012345678901234567890123456789 AS v FROM dual", "123456789012345678901234567890123456789")] + [InlineData(DbProvider.Oracle, "SELECT BINARY_DOUBLE_NAN AS v FROM dual", "\"NaN\"")] + [InlineData(DbProvider.Postgres, "SELECT 'NaN'::float8 AS v", "\"NaN\"")] + [InlineData(DbProvider.Postgres, "SELECT '-infinity'::float8 AS v", "\"-Infinity\"")] + [InlineData(DbProvider.Postgres, "SELECT '10.0.0.1'::inet AS v", "\"10.0.0.1\"")] + [InlineData(DbProvider.Postgres, "SELECT ARRAY[1, 2] AS v", "[1,2]")] + [InlineData(DbProvider.Postgres, "SELECT int4range(1, 5) AS v", "\"[1,5)\"")] + [InlineData(DbProvider.Postgres, "SELECT B'101' AS v", "\"101\"")] + [InlineData(DbProvider.Postgres, "SELECT 'NaN'::numeric AS v", "Convert it to a string in the query")] + [InlineData(DbProvider.SqlServer, "SELECT geography::Point(1, 2, 4326) AS v", "Convert it to a string in the query")] + public async Task Returns_awkward_values_as_json(DbProvider provider, string sql, string expected) + { + var db = RequireDb(provider); + + var result = await Service(db).ExecuteAsync(new("test", sql)); + + Assert.True(result.Error is null, result.Error); + var json = JsonSerializer.Serialize(result.Value!.Rows.Single().Values.Single()); + Assert.Contains(expected, json); + } + + [SkippableFact] + public async Task Rejects_queries_beyond_the_concurrency_limit() + { + var db = RequireDb(DbProvider.Postgres); + var svc = Service(db, settings: new() { ["App:MaxConcurrentQueries"] = "1", ["App:QueryTimeoutSeconds"] = "3" }); + + // One slot: the first holds it for 2.5 s, the second gets it after waiting, and + // the third would have to wait past the 3 s timeout. + var results = await Task.WhenAll(Enumerable.Range(0, 3).Select(async i => + { + await Task.Delay(i * 100); + return await svc.ExecuteAsync(new("test", "SELECT pg_sleep(2.5) AS s")); + })); + + Assert.Null(results[0].Error); + Assert.Contains(results, r => r.Error?.Contains("is busy") == true); + } + + [SkippableFact] + public async Task Logs_each_query_without_parameter_values() + { + var db = RequireDb(DbProvider.Postgres); + var logger = new ListLogger(); + + await Service(db, logger: logger).ExecuteAsync(Query("SELECT @secret AS v", """{"secret": "hunter2"}""")); + await Service(db, logger: logger).ExecuteAsync(new("test", "SELECT * FROM sqlmcp_missing_table")); + + var ran = Assert.Single(logger.Messages, m => m.Level == LogLevel.Information); + Assert.Contains("query with parameters secret: SELECT @secret AS v", ran.Text); + Assert.Contains("1 rows", ran.Text); + Assert.Contains(logger.Messages, m => m.Level == LogLevel.Warning && m.Text.Contains("sqlmcp_missing_table")); + Assert.DoesNotContain(logger.Messages, m => m.Text.Contains("hunter2")); + } + + private sealed class ListLogger : ILogger + { + public List<(LogLevel Level, string Text)> Messages { get; } = []; + public IDisposable? BeginScope(TState state) where TState : notnull => null; + public bool IsEnabled(LogLevel logLevel) => true; + public void Log(LogLevel logLevel, EventId eventId, TState state, Exception? exception, Func formatter) + { + lock (Messages) + Messages.Add((logLevel, formatter(state, exception))); + } + } + + [SkippableTheory] + [MemberData(nameof(Providers))] + public async Task Readiness_check_passes_a_reachable_database_and_reports_a_writable_login(DbProvider provider) + { + var db = RequireDb(provider); + + Assert.Equal("ok", (await Service(db).CheckDatabasesAsync())["test"]); + Assert.Contains("is disabled", (await Service(db, allowWritableLogin: false).CheckDatabasesAsync())["test"]); + } } diff --git a/tests/SqlMcp.Tests/SqlGuardTests.cs b/tests/SqlMcp.Tests/SqlGuardTests.cs index c6b976a..01d4d09 100644 --- a/tests/SqlMcp.Tests/SqlGuardTests.cs +++ b/tests/SqlMcp.Tests/SqlGuardTests.cs @@ -196,4 +196,12 @@ public void Allows_reads_that_resemble_sequence_calls(DbProvider provider, strin [InlineData(DbProvider.SqlServer, "SELECT [sp_executesql]")] public void Denies_functions_that_reach_outside_or_hide_sql(DbProvider provider, string sql) => Assert.False(SqlGuard.Validate(sql, provider).IsAllowed); + + [Theory] + [InlineData(DbProvider.SqlServer, "SELECT * FROM t WHERE name = @name AND id > @min_id")] + [InlineData(DbProvider.Postgres, "SELECT * FROM t WHERE name = @name AND id > @min_id")] + [InlineData(DbProvider.MySql, "SELECT * FROM t WHERE name = @name AND id > @min_id")] + [InlineData(DbProvider.Oracle, "SELECT * FROM t WHERE name = :name AND id > :min_id")] + public void Allows_parameter_placeholders(DbProvider provider, string sql) => + Assert.True(SqlGuard.Validate(sql, provider).IsAllowed); }