fix(api): scrub driver errors from the preview endpoint and answer 400, not 500 (#258)

* fix(api): scrub driver errors from the preview endpoint and answer 400, not 500

POST /reports/{name}/preview caught only ConfigurationException. It opens a live
connection and runs SQL, so a bad filter value (say Equals "abc" against an
integer column) surfaced the raw SqlException/PostgresException/OracleException —
which names the host, port and database — as an unhandled 500, and on a host
running in Development the developer exception page renders the whole stack
trace. Every sibling endpoint that touches a source already routes failures
through SchemaProblem (logged server-side, generic to the caller); this one now
does too.

That alone would have turned a second bug into a misleading 502: a code-first
report's name is only checked for non-blank, so "sales.daily" is legal, while a
config store validates its argument against the dynamic-name pattern and throws
for it. ReportPreviewRunner probed the store before deciding the report was
typed, so that ArgumentException escaped as a 500. A name the store cannot hold
is definitively not a dynamic report, so it is now recognised as such and the
endpoint returns the clear 400 it always intended.

The regression test initially passed for the wrong reason - TestApp registers no
config store unless AddDynamicReports is called, so the runner short-circuited
before reaching the name check. Hardened to register a real file-backed store;
it now fails without the guard (502 instead of 400).

* test(api): cover the preview endpoint's driver-error scrubbing

The Sonar gate flagged new_coverage 0%: the two new lines are the catch and its
SchemaProblem return, and nothing exercised them — the existing regression test
takes the name-guard path, which returns 400 before any driver call. So the
security fix itself was untested.

Add a filter translator that fails the way a provider does, with a message naming
the connection target, and assert the response is the scrubbed 502 and carries
neither the host, the database, nor the driver text. Verified to fail without the
catch (the exception escapes with the connection string intact).

Author: thiagoluga

CHANGELOG.md

@@ -58,6 +58,14 @@ The `NeoReports.Abstractions` contract follows SemVer strictly.
   the host — D20 — this is a nudge, not a behaviour change).
 
 ### Fixed
+- **The report-preview endpoint no longer leaks driver exceptions, and answers 400 where it meant to.**
+  It caught only `ConfigurationException`, so a bad filter value surfaced the raw `SqlException`/
+  `PostgresException`/`OracleException` — naming the host, port and database — as an unhandled 500
+  (with a full stack trace on a host running in Development). It now routes those through the same
+  scrubbed 502 its sibling schema endpoints use. Separately, a **code-first** report whose name a
+  config store cannot hold (`sales.daily`, say — code-first names are unrestricted) made the preview
+  runner throw while probing that store, another 500; such a name is now recognised as definitively
+  not-dynamic, giving the intended 400 explaining that typed reports have no filterable source.
 - **Sectioned outputs are counted as outputs.** `CompiledReport.OutputCount` (and `OutputFormats`)
   omitted them, so a report with one plain and one sectioned output looked single-output: the API's
   sync mode — which supports single-output reports only — accepted it, the runner wrote both files,

docs/STATUS-AND-BACKLOG.md

@@ -245,11 +245,14 @@ rather than decided:
   source built with defaults fails its very first request until the author calls `.PageSize(100)`.
   Loud, but the default configuration is non-functional. Clamping vs. failing with a clear message is
   a product call.
-- **API: `POST /reports/{name}/preview` is the one data-plane endpoint that doesn't scrub driver
-  exceptions.** It catches only `ConfigurationException`, so a bad filter value surfaces the raw
+- ~~**API: `POST /reports/{name}/preview` is the one data-plane endpoint that doesn't scrub driver
+  exceptions.**~~ **FIXED** (routes through `SchemaProblem` like its siblings). Original description: It catches only `ConfigurationException`, so a bad filter value surfaces the raw
   `SqlException`/`PostgresException` (host, port, database) as a 500 — its siblings all route through
   `SchemaProblem`. Should be a 400 (bad filter) or the scrubbed 502 the others return.
-- **API: schedule/preview write paths reach a name-validating store without the guard.**
+- **API: schedule/preview write paths reach a name-validating store without the guard.** The
+  **preview** half is **FIXED** (a name no config store can hold is now treated as not-dynamic, so the
+  endpoint returns its intended 400). `SetScheduleAsync`/`ClearScheduleAsync` still 500 — original
+  description:
   `SetScheduleAsync`, `ClearScheduleAsync` and `ReportPreviewRunner`'s config-store probe pass the
   report name straight through, so a legitimate **code-first** report whose name is outside
   `^[a-zA-Z][a-zA-Z0-9_-]{0,99}$` (e.g. `sales.daily`) gets a **500** from `ArgumentException`. The

src/Integrations/NeoReports.AspNetCore/NeoReportsEndpointRouteBuilderExtensions.cs

@@ -262,6 +262,15 @@ private static async Task<IResult> PreviewReportAsync(
         {
             return Results.BadRequest(new { error = ex.Message });
         }
+        catch (Exception ex) when (ex is not OperationCanceledException)
+        {
+            // Preview opens a live connection and runs SQL, so a bad filter value surfaces the raw
+            // driver exception — which names the host, port and database. Every sibling endpoint that
+            // touches a source routes failures through SchemaProblem (logged server-side, generic to
+            // the caller); this one used to let them escape as an unhandled 500, which on a host
+            // running in Development also renders the full stack trace.
+            return SchemaProblem(http, ex, name, "Could not run the report preview.");
+        }
     }
 
     private static IReadOnlyList<PreviewFilter> ParseFilters(IReadOnlyList<PreviewFilterRequest>? filters)

src/NeoReports.Core/Preview/ReportPreviewRunner.cs

@@ -51,7 +51,13 @@ public static async Task<PreviewResult> PreviewAsync(
             return await PreviewUnfilteredAsync(report, pageSize, execution, services, cancellationToken).ConfigureAwait(false);
 
         IReportConfigStore? configStore = services.GetService<IReportConfigStore>();
+        // Check the name is one a config store could even hold before probing it: the file-backed
+        // store validates its argument and throws for anything outside the dynamic-name pattern, and
+        // a code-first report is under no such restriction (a name like "sales.daily" is legal). That
+        // threw out of the endpoint as an unhandled ArgumentException — a 500 — instead of the clear
+        // "typed report" message below. A name the store cannot hold is definitively not dynamic.
         bool isDynamic = configStore is not null
+            && DynamicReportName.IsValid(report.Name)
             && await configStore.ExistsAsync(report.Name, cancellationToken).ConfigureAwait(false);
 
         if (!isDynamic)

tests/NeoReports.AspNetCore.IntegrationTests/PreviewEndpointTests.cs

@@ -71,6 +71,50 @@ public async Task PageSize_is_capped_to_the_request()
         body.GetProperty("hasMore").GetBoolean().ShouldBeTrue();
     }
 
+    [Fact]
+    public async Task A_driver_failure_during_preview_is_scrubbed_and_not_returned_to_the_caller()
+    {
+        // Preview opens a live connection and runs SQL, so a driver exception is reachable from a
+        // request. Its message routinely echoes the connection target; it must never reach the caller.
+        using Microsoft.Extensions.Hosting.IHost host =
+            await StartWithDynamicReportAsync("fake-sql", new ThrowingFilterTranslator("fake-sql"));
+        var client = host.GetTestClient();
+
+        var response = await PostJsonAsync(client, "/api/reports/dyn/preview",
+            """{ "filters": [ { "column": "Id", "operator": "Equals", "value": 1 } ] }""");
+
+        response.StatusCode.ShouldBe(HttpStatusCode.BadGateway);
+        var body = await response.Content.ReadAsStringAsync();
+        body.ShouldNotContain("db.internal");
+        body.ShouldNotContain("payroll");
+        body.ShouldNotContain("Login failed");
+    }
+
+    [Fact]
+    public async Task Filters_on_a_typed_report_whose_name_the_config_store_cannot_hold_return_400()
+    {
+        // A code-first report's name is only checked for non-blank, so "sales.daily" is legal — but a
+        // config store validates its argument against the dynamic-name pattern and throws for it. The
+        // preview runner probed the store before deciding the report was typed, so that ArgumentException
+        // escaped as a 500 instead of the clear "typed report" 400 this endpoint means to return.
+        // A real (file-backed) config store must be registered, otherwise the runner short-circuits on
+        // `configStore is null` and never reaches the name check this test is about.
+        using var host = await TestApp.StartAsync(services =>
+        {
+            services.AddDynamicReports(o => o.Directory = _configDir);
+            services.AddReport<Sale>("sales.daily", b => b
+                .From(new InMemorySource(rows: 3, pageSize: 10))
+                .Column(v => v.Id, "Id")
+                .To(NeoReports.Formats.Csv.Format.Csv()));
+        });
+        var client = host.GetTestClient();
+
+        var response = await PostJsonAsync(client, "/api/reports/sales.daily/preview",
+            """{ "filters": [ { "column": "Id", "operator": "Equals", "value": 1 } ] }""");
+
+        response.StatusCode.ShouldBe(HttpStatusCode.BadRequest);
+    }
+
     [Fact]
     public async Task Filters_on_a_typed_report_return_400()
     {
@@ -329,3 +373,19 @@ public bool TryTranslate(
         return true;
     }
 }
+
+/// <summary>Translator that fails the way a real provider does — an exception whose message names the
+/// connection target — so the endpoint's scrubbing can be asserted.</summary>
+public sealed class ThrowingFilterTranslator : IFilterTranslator
+{
+    public ThrowingFilterTranslator(string type) => Type = type;
+
+    public string Type { get; }
+
+    public bool TryTranslate(
+        IReadOnlyDictionary<string, object?> properties, IReadOnlyList<PreviewFilter> filters, ReportSchema schema,
+        out IReadOnlyDictionary<string, object?> propertyOverrides,
+        out IReadOnlyDictionary<string, object?> parameters) =>
+        throw new InvalidOperationException(
+            "Login failed for user 'sa'. Server=db.internal:1433;Database=payroll");
+}