fix(api): stop leaking destination errors, honour the mapped prefix, guard schedule names (#261)

* fix(api): stop leaking destination errors, honour the mapped prefix, guard schedule names

Four API-layer defects recorded in docs/STATUS-AND-BACKLOG.md §6, each with a single
defensible answer, so none needed a product decision.

- `GET /jobs/{id}` and `GET /jobs/{id}/events` returned a destination's own failure text
  verbatim. `S3Destination` interpolates `s3://{bucket}/{key}` plus the AWS SDK's message
  and `LocalDestination` an `IOException` carrying the full server path, so both routes
  handed infrastructure detail to any API caller. The runner now persists and emits only
  the file name and destination type and keeps the reason in the log, matching what the
  read-failure path beside it already does. Scrubbing at the runner rather than in each
  destination also covers third-party `IDestination` implementations, and `UploadResult`
  itself is untouched (`Abstractions` is frozen).
- `Location` was built from a hardcoded `/api`, so under `MapNeoReports("/v2")` the 202's
  header was a 404 for any client that followed it. A group-level endpoint filter carries
  the mapped prefix on the request and the three `Created`/`Accepted` sites build from it.
- `PUT`/`DELETE /reports/{name}/schedule` reached a name-validating override store
  unguarded, so a legitimate code-first report named `sales.daily` got an
  `ArgumentException` 500. Both now answer 409 naming the pattern, like the
  "this host cannot do that" response already next to them. The read path guarded
  already, which is what made this an oversight.
- `POST`/`PUT /sources` stored the caller's property bag unnormalized, so under
  `AddInMemorySourceRegistry()` a source created over HTTP failed later with "requires a
  non-empty 'connectionString' property" — the value was a `JsonElement`. Both handlers
  now reuse the run endpoint's normalizer, renamed `NormalizeJsonValues` as it serves two
  request shapes.

Seven tests, each verified to fail against the unfixed code. The `Location` test follows
the header instead of string-matching it, so a well-formed-but-wrong URL still fails, and
the upload assertions cover the events feed as well as the job's error.

* test(api): address CodeQL — drop the redundant cast and document TestApp.StartAsync

Author: thiagoluga

docs/STATUS-AND-BACKLOG.md

@@ -249,15 +249,20 @@ rather than decided:
   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.** 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:
+- ~~**API: schedule/preview write paths reach a name-validating store without the guard.**~~
+  **FIXED** — the preview half first (a name no config store can hold is treated as not-dynamic, so
+  the endpoint returns its intended 400), and now `SetScheduleAsync`/`ClearScheduleAsync` too: both
+  answer **409 Conflict** naming the pattern, matching the "this host cannot do that" response
+  already next to them, instead of an `ArgumentException` 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
   read paths already guard, which shows it is an oversight.
-- **API: `GET /jobs/{id}` returns raw destination-exception text** (server paths, S3 bucket + key, AWS
+- ~~**API: `GET /jobs/{id}` returns raw destination-exception text**~~ **FIXED** — the runner now
+  persists (and emits as the `UploadFailed` event) only the file name and destination type, keeping the
+  destination's own wording in the log. Scrubbing at the runner rather than in each destination is what
+  also covers third-party `IDestination` implementations. `GET /jobs/{id}/events` was a second route
+  out for the same string and is covered too. Original description: (server paths, S3 bucket + key, AWS
   error strings). The read- and write-failure paths in the same method scrub; the **upload** path does
   not — and the sync endpoint deliberately suppresses the very same string, so one route hides what
   the other returns verbatim.
@@ -267,20 +272,24 @@ rather than decided:
   `Outputs`, so a report with one plain and one sectioned output passes the guard, the runner writes
   two artifacts, and the caller silently receives **one** — which one decided by directory-enumeration
   order. The same undercount makes `GET /reports` under-report a sectioned report's formats.
-- **API: `Location`/`Content-Location` headers hardcode `/api`**, ignoring `MapNeoReports`'s
-  configurable prefix — under `MapNeoReports("/v2")` the 202's `Location` is a 404 for any client that
-  follows it.
+- ~~**API: `Location`/`Content-Location` headers hardcode `/api`**~~ **FIXED** — a group-level
+  endpoint filter puts the mapped prefix on the request, and the three `Created`/`Accepted` sites build
+  their URL from it. The test follows the returned `Location` under `MapNeoReports("/v2")` rather than
+  string-matching it, so a well-formed-but-wrong header still fails. Original description: ignoring
+  `MapNeoReports`'s configurable prefix — under `MapNeoReports("/v2")` the 202's `Location` is a 404
+  for any client that follows it.
 - **Array/object run parameters still diverge by backend.** Complex parameter values are documented
   out of scope for v1, but nothing rejects them: sync/in-memory hand the source a `JsonElement` (the
   very thing an ADO provider can't bind) while Hangfire hands it the raw JSON text. Either reject them
   at the boundary with a 400, or agree one representation — the current silence produces a driver
   error at read time.
-- **`POST/PUT /sources` property bags are not normalized.** `SourceRequest.Properties` is the same
+- ~~**`POST/PUT /sources` property bags are not normalized.**~~ **FIXED** — both handlers now run the
+  bag through the same normalizer the run endpoint uses (renamed `NormalizeJsonValues`, since it serves
+  two request shapes). Original description: `SourceRequest.Properties` is the same
   caller-supplied `object?` bag as run parameters. `FileSourceRegistryStore` launders it on write and
   read, but `InMemorySourceRegistryStore` stores it as-is — so with `AddInMemorySourceRegistry()` a
   source created over HTTP fails later with *"requires a non-empty 'connectionString' property"*
-  because the value is a `JsonElement`, not a `string`. Pre-existing; the same one-line normalization
-  the run endpoint now does would close it.
+  because the value is a `JsonElement`, not a `string`.
 
 Verified correct in the same pass (worth not re-auditing): artifact download path handling (no
 caller-supplied filename reaches disk; no zip-slip; `0600` temp), job-id handling, SQL-injection

src/Integrations/NeoReports.AspNetCore/NeoReportsEndpointRouteBuilderExtensions.cs

@@ -75,6 +75,19 @@ public static RouteGroupBuilder MapNeoReports(
         configure?.Invoke(options);
 
         var group = endpoints.MapGroup(prefix);
+
+        // A handler that returns a Location must point back at the prefix this group was actually
+        // mapped under; hardcoding "/api" makes the 202's Location a 404 under MapNeoReports("/v2").
+        // The handlers are static method groups with no closure over `prefix`, so the mapped prefix
+        // rides along on the request instead. Trailing '/' is trimmed so MapNeoReports("/") does not
+        // produce a protocol-relative "//jobs/..." URL.
+        string mappedPrefix = prefix.TrimEnd('/');
+        group.AddEndpointFilter(async (context, next) =>
+        {
+            context.HttpContext.Items[MappedPrefixKey] = mappedPrefix;
+            return await next(context).ConfigureAwait(false);
+        });
+
         if (options.RequireAuthorization)
         {
             if (string.IsNullOrEmpty(options.AuthorizationPolicy))
@@ -172,7 +185,7 @@ private static async Task<IResult> RunReportAsync(
             });
         }
 
-        IReadOnlyDictionary<string, object?>? parameters = NormalizeParameters(body?.Parameters);
+        IReadOnlyDictionary<string, object?>? parameters = NormalizeJsonValues(body?.Parameters);
 
         if (string.Equals(mode, "sync", StringComparison.OrdinalIgnoreCase))
         {
@@ -217,7 +230,7 @@ private static async Task<IResult> RunReportAsync(
         var enqueuedId = await scheduler.EnqueueAsync(
             new ReportJobRequest(name, parameters), cancellationToken).ConfigureAwait(false);
         return Results.Accepted(
-            $"{http.Request.PathBase}/api/jobs/{enqueuedId}",
+            ApiUrl(http, $"/jobs/{enqueuedId}"),
             new RunAcceptedResponse(enqueuedId, ReportJobStatus.Queued));
     }
 
@@ -457,7 +470,7 @@ private static async Task<IResult> CreateReportAsync(
 
         var columns = compiled.Schema.Columns.Select(c => c.Name).ToArray();
         return Results.Created(
-            $"{http.Request.PathBase}/api/reports/{config.Name}", new ReportCreatedResponse(config.Name, columns));
+            ApiUrl(http, $"/reports/{config.Name}"), new ReportCreatedResponse(config.Name, columns));
     }
 
     private static async Task<IResult> ValidateReportAsync(
@@ -549,6 +562,47 @@ private static IResult GetCapabilities(HttpContext http)
         return Results.Ok(new CapabilitiesResponse(sources, formats, destinations, scheduling));
     }
 
+    /// <summary><see cref="HttpContext.Items"/> key carrying the prefix this group was mapped at.</summary>
+    private const string MappedPrefixKey = "NeoReports.MappedPrefix";
+
+    /// <summary>
+    /// Builds a URL for another endpoint of this API, honouring both the host's path base and the
+    /// prefix <see cref="MapNeoReports"/> was called with. Falls back to the documented default only
+    /// if the filter that stashes the prefix somehow did not run.
+    /// </summary>
+    /// <param name="http">The current request.</param>
+    /// <param name="relativePath">Path below the prefix, starting with <c>/</c>.</param>
+    private static string ApiUrl(HttpContext http, string relativePath)
+    {
+        string prefix = http.Items.TryGetValue(MappedPrefixKey, out object? mapped) && mapped is string s
+            ? s
+            : "/api";
+        return $"{http.Request.PathBase}{prefix}{relativePath}";
+    }
+
+    /// <summary>
+    /// Rejects a schedule write for a report whose name an override store cannot key.
+    /// <para>
+    /// A schedule override is stored by report name, and a store persists it as a file name, so it
+    /// only accepts <see cref="DynamicReportName.Pattern"/>. A <b>code-first</b> report is under no
+    /// such constraint — <c>sales.daily</c> is perfectly legal — so a legitimately registered report
+    /// can reach the store with a name it refuses, which surfaced as an <see cref="ArgumentException"/>
+    /// and a <b>500</b>. The read path (<c>ResolveScheduleAsync</c>) already skips the lookup for such
+    /// a name, which is what makes the missing guard here an oversight rather than a design.
+    /// </para>
+    /// </summary>
+    /// <param name="name">The report name from the route.</param>
+    /// <returns><see langword="null"/> when the name is storable; otherwise the response to return.</returns>
+    private static IResult? ScheduleOverridesUnsupportedFor(string name) =>
+        DynamicReportName.IsValid(name)
+            ? null
+            : Results.Conflict(new
+            {
+                error = $"The schedule for '{name}' cannot be changed over HTTP: overrides are stored " +
+                        $"by report name, and a name must match {DynamicReportName.Pattern}. Declare " +
+                        "this report's schedule in code, or rename it.",
+            });
+
     private static async Task<IResult> SetScheduleAsync(
         string name, SetScheduleRequest? body, HttpContext http,
         [FromServices] IReportRegistry registry, CancellationToken cancellationToken)
@@ -570,6 +624,9 @@ private static async Task<IResult> SetScheduleAsync(
             });
         }
 
+        if (ScheduleOverridesUnsupportedFor(name) is { } unsupported)
+            return unsupported;
+
         try
         {
             CronValidation.Validate(body.Cron);
@@ -600,6 +657,9 @@ private static async Task<IResult> ClearScheduleAsync(
         if (scheduler is null || overrides is null)
             return Results.Conflict(new { error = "No recurring scheduler is registered on this host." });
 
+        if (ScheduleOverridesUnsupportedFor(name) is { } unsupported)
+            return unsupported;
+
         // A declared schedule needs an explicit "unscheduled" tombstone — merely removing any prior
         // override would let the declaration re-apply on the next reconciliation. A report with no
         // declaration has nothing to tombstone, so the override entry (if any) is just removed —
@@ -955,8 +1015,8 @@ private static async Task<IResult> CreateSourceAsync(HttpContext http, Cancellat
         if (await registry.GetAsync(body!.Name, cancellationToken).ConfigureAwait(false) is not null)
             return Results.Conflict(new { error = $"A source named '{body.Name}' already exists." });
 
-        await registry.SaveAsync(new SourceDefinition(body.Name, body.Type, body.Properties, body.Description), cancellationToken).ConfigureAwait(false);
-        return Results.Created($"{http.Request.PathBase}/api/sources/{body.Name}", ToSourceView(
+        await registry.SaveAsync(new SourceDefinition(body.Name, body.Type, NormalizeJsonValues(body.Properties), body.Description), cancellationToken).ConfigureAwait(false);
+        return Results.Created(ApiUrl(http, $"/sources/{body.Name}"), ToSourceView(
             new SourceDefinition(body.Name, body.Type, Description: body.Description), reportRegistry: http.RequestServices.GetRequiredService<IReportRegistry>(), healthCache: null));
     }
 
@@ -974,7 +1034,7 @@ private static async Task<IResult> ReplaceSourceAsync(string name, HttpContext h
         if (await registry.GetAsync(name, cancellationToken).ConfigureAwait(false) is null)
             return Results.NotFound(new { error = $"No source named '{name}' is registered." });
 
-        await registry.SaveAsync(new SourceDefinition(name, body!.Type, body.Properties, body.Description), cancellationToken).ConfigureAwait(false);
+        await registry.SaveAsync(new SourceDefinition(name, body!.Type, NormalizeJsonValues(body.Properties), body.Description), cancellationToken).ConfigureAwait(false);
         IReportRegistry reportRegistry = http.RequestServices.GetRequiredService<IReportRegistry>();
         ISourceHealthCache? healthCache = http.RequestServices.GetService<ISourceHealthCache>();
         return Results.Ok(ToSourceView(new SourceDefinition(name, body.Type, Description: body.Description), reportRegistry, healthCache));
@@ -1195,26 +1255,31 @@ private static IResult SchemaProblem(HttpContext http, Exception ex, string sour
             statusCode: StatusCodes.Status502BadGateway);
     }
 
-    // The run request types its parameter values as `object?`, so System.Text.Json materializes each
-    // one as a JsonElement. Nothing downstream can use that: an ADO provider rejects it outright
+    // Two request types carry a caller-supplied bag of `object?` values — a run's `Parameters` and a
+    // source's `Properties` — so System.Text.Json materializes each value as a JsonElement. Nothing
+    // downstream can use that: an ADO provider rejects it outright
     // ("No mapping exists from object type System.Text.Json.JsonElement"), so every parameterized
     // report failed on the sync and in-memory-job paths — while the Hangfire path happened to work,
     // because it round-trips parameters through JobParameters, which converts them. Convert here
     // instead, at the one boundary where they enter, so every backend behaves the same. Round-tripping
     // through PrimitiveObjectConverter reuses the repo's single definition of "JSON value → CLR
     // primitive" (string/long/double/bool/ISO-8601 DateTime, nested objects left as JsonElement)
-    // rather than restating it; parameter bags are a handful of scalars, so the cost is irrelevant.
-    private static readonly JsonSerializerOptions ParameterJson =
+    // rather than restating it; these bags are a handful of scalars, so the cost is irrelevant.
+    // Source properties had the identical split: FileSourceRegistryStore launders the bag through JSON
+    // on write and read, but InMemorySourceRegistryStore keeps it as given — so under
+    // AddInMemorySourceRegistry() a source created over HTTP failed later with "requires a non-empty
+    // 'connectionString' property", the value being a JsonElement rather than a string.
+    private static readonly JsonSerializerOptions ValueBagJson =
         new(JsonSerializerDefaults.Web) { Converters = { new PrimitiveObjectConverter() } };
 
-    private static IReadOnlyDictionary<string, object?>? NormalizeParameters(
+    private static IReadOnlyDictionary<string, object?>? NormalizeJsonValues(
         IReadOnlyDictionary<string, object?>? parameters)
     {
         if (parameters is null || parameters.Count == 0)
             return parameters;
 
-        string json = JsonSerializer.Serialize(parameters, ParameterJson);
-        return JsonSerializer.Deserialize<Dictionary<string, object?>>(json, ParameterJson);
+        string json = JsonSerializer.Serialize(parameters, ValueBagJson);
+        return JsonSerializer.Deserialize<Dictionary<string, object?>>(json, ValueBagJson);
     }
 
     // A failed health check's raw Error is the underlying driver/IO exception message, which can echo

src/NeoReports.Core/Pipeline/ReportRunner.cs

@@ -468,13 +468,25 @@ await writer.InitializeAsync(
                             // the run fails and says which destination/file did not land, instead of
                             // completing green with the file still on the box (delivery-integrity).
                             uploadFailed = true;
-                            uploadError ??= $"Upload of '{finished.FileName}' to destination " +
-                                $"'{destSpec.Factory.Type}' failed: {uploadResult.ErrorMessage}";
+
+                            // The destination's own message is NOT persisted or emitted: it is built by
+                            // the destination and routinely embeds infrastructure detail — S3Destination
+                            // interpolates `s3://{bucket}/{key}` plus the AWS SDK's text, LocalDestination
+                            // an IOException carrying the full server path — and both `GET /jobs/{id}` and
+                            // `GET /jobs/{id}/events` return these verbatim to any API caller. The read
+                            // failure path above already reduces a non-NeoReports exception to its type
+                            // name for exactly this reason, and the sync endpoint suppresses this same
+                            // string. Scrubbing here (rather than in each destination) is what also covers
+                            // third-party IDestination implementations, whose messages we do not control.
+                            // The full reason stays in the log below, which is not caller-visible.
+                            string safeUploadError = $"Upload of '{finished.FileName}' to destination " +
+                                $"'{destSpec.Factory.Type}' failed. See the server logs for the reason.";
+                            uploadError ??= safeUploadError;
                             execution.Logger.LogError(
                                 "Report {Report} (job {JobId}) failed to upload '{FileName}' to destination '{DestinationType}': {Reason}",
                                 report.Name, execution.JobId, finished.FileName, destSpec.Factory.Type, uploadResult.ErrorMessage);
 
-                            await events.EmitAsync(JobEventTypes.UploadFailed, uploadResult.ErrorMessage, new Dictionary<string, string>
+                            await events.EmitAsync(JobEventTypes.UploadFailed, safeUploadError, new Dictionary<string, string>
                             {
                                 ["destinationType"] = destSpec.Factory.Type,
                                 [FileNameKey] = finished.FileName,

tests/NeoReports.AspNetCore.IntegrationTests/DynamicReportEndpointsTests.cs

@@ -54,7 +54,7 @@ private Task<IHost> StartAsync([System.Runtime.CompilerServices.CallerMemberName
             services.AddSingleton<IConfigSourceProvider>(new FakeConfigSourceProvider(
                 new[] { new object?[] { 1L, "Acme" }, new object?[] { 2L, "Globex" } }));
             services.AddSingleton<IWriterFactory>(new CsvWriterFactory(new CsvOptions()));
-        }, testName);
+        }, testName: testName);
 
     private static async Task<HttpResponseMessage> PostJsonAsync(HttpClient client, string url, string json)
     {