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

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 @@
         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))
@@ -146,7 +159,7 @@
         return group;
     }
 
     private static async Task<IResult> RunReportAsync(
         string name,
         string? mode,
         RunReportRequest? body,
@@ -172,7 +185,7 @@
             });
         }
 
-        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 @@
         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 @@
 
         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 @@
         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 @@
             });
         }
 
+        if (ScheduleOverridesUnsupportedFor(name) is { } unsupported)
+            return unsupported;
+
         try
         {
             CronValidation.Validate(body.Cron);
@@ -600,6 +657,9 @@
         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 @@
         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 @@
         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));
@@ -1062,7 +1122,7 @@
 
         try
         {
             SchemaCatalog catalog = await explorer!.GetCatalogAsync(definition!, cancellationToken).ConfigureAwait(false);
             return Results.Ok(ToCatalogResponse(catalog));
         }
         catch (Exception ex) when (ex is not OperationCanceledException)
@@ -1084,7 +1144,7 @@
 
         try
         {
             TablePreview preview = await explorer!
                 .PreviewTableAsync(definition!, schema ?? string.Empty, table, SchemaPreviewTop, cancellationToken)
                 .ConfigureAwait(false);
             return Results.Ok(new TablePreviewResponse(preview.Columns, preview.Rows));
@@ -1195,26 +1255,31 @@
             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

@@ -90,7 +90,7 @@
     /// <param name="execution">The execution context (carries job id, parameters, logger, token).</param>
     /// <param name="services">Service provider passed to writer/destination factories.</param>
     /// <param name="cancellationToken">Token that cooperatively cancels the run.</param>
     public static async Task<ReportRunResult> ExecuteAsync(
         CompiledReport report,
         ReportExecutionContext execution,
         IServiceProvider services,
@@ -114,7 +114,7 @@
 
         var outputs = new List<RunningOutput>(report.Outputs.Count);
         var sectioned = new List<RunningSectioned>(report.SectionedOutputs.Count);
         var reader = report.ReaderFactory(execution, services);
 
         long recordsRead = 0, recordsWritten = 0;
         int retries = 0, batches = 0, skipped = 0;
@@ -128,14 +128,14 @@
         long? totalRecords = null;
 
         var jobEventStore = services.GetService(typeof(IJobEventStore)) as IJobEventStore;
         var events = await JobEventEmitter.CreateAsync(jobEventStore, execution.JobId, execution.Logger, cancellationToken)
             .ConfigureAwait(false);
 
         // The retry hook only ever emits an event (ADR D38, ground rule 3: telemetry must never
         // change a run's outcome) — ShouldHandle/backoff/jitter above are untouched. It also logs at
         // Debug so a host that hasn't opted into an IJobEventStore still sees retries in its ILogger
         // sink; the event stream carries the full structured detail regardless.
         var resilience = ResiliencePipelineFactory.Build(report.Retry, async (attempt, delay, ex) =>
         {
             execution.Logger.LogDebug(
                 "Retrying batch {Page} (attempt {Attempt}) after {DelayMs}ms: {ExceptionType}",
@@ -159,9 +159,9 @@
         // all-or-nothing publish guarantee), only in a dedicated partial-artifact store, and only
         // when one is registered. A capture failure (writer refuses to finalize mid-failure, store
         // I/O error, ...) must never change the run's own outcome.
         async Task CaptureOnePartialAsync(
             Func<CancellationToken, Task> finalize, Func<ValueTask> disposeWriter, FileStream stream,
             Action markClosed, string path, string fileName, string mimeType, IPartialArtifactStore partialStore)
         {
             try
             {
@@ -468,13 +468,25 @@
                             // 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,
@@ -496,7 +508,7 @@
             else
             {
                 // status == Failed (from either a read failure or a write failure escalated to
                 // abort) — CompletedPartial runs legitimately publish above and never reach here;
                 // only a genuine failure captures partials (D11's batch-atomicity: the partial file
                 // contains exactly the fully-written batches).
                 await CapturePartialArtifactsAsync().ConfigureAwait(false);

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)
     {