fix(api): an invalid source name in the URL is a 404, not a 500

GET and DELETE /api/sources/{name} passed the raw route segment into the
registry store, whose GetAsync/DeleteAsync called ValidateName and threw
ArgumentException for a name it could never have written. Nothing above
catches that, so GET /api/sources/a%20b answered 500 with the whole
validation regex in the body, where an unknown-but-legal name has always
answered a clean 404. Measured before changing anything: "a b" and
"../evil" both threw; a legal-but-unknown name returned 404.

The rule, applied in both stores: a LOOKUP for a name the store could
never have written is a miss, not an error. Writes still validate —
there a bad name is the caller's mistake, and rejecting it is what keeps
the name from ever becoming a key or a path. The names that reach
GetPath are unchanged; only the action on rejection differs.

Fixing it at the store rather than at each endpoint covers every caller
at once: the same lookup backs source.ref resolution, so a report
referencing an unusable source name now fails with "No source named 'x'
is registered" instead of a 500 out of the compiler.

Documented on ISourceRegistryStore, since this is now part of its
contract rather than an implementation detail.

Found while attempting something else. Adding the Windows device names
(CON, NUL, COM1…) to the name grammar looked like a one-line fix and was
not: DynamicReportName.IsValid serves as both a validator (reject -> 400)
and a discriminator (could a file-backed store hold this?), so narrowing
it turned source.ref: CON and GET /api/sources/CON from clean 404s into
500s, and would have left a nul.json created on Linux un-deletable. That
attempt is backed out and recorded as backlog 1e; this commit fixes the
500 half of its fallout, which was a real pre-existing bug on its own.

An existing test asserted the old contract (Invalid_name_throws_on_every_
operation) and went red — the full suite caught what my targeted runs on
the in-memory store and the endpoints did not. Rewritten to the new one,
including that the store's directory is never created, which is the
strongest available proof the name did not reach GetPath.

Full suite: 1 711 green across 33 projects.

Author: thiagoluga

CHANGELOG.md

@@ -9,6 +9,16 @@ The `NeoReports.Abstractions` contract follows SemVer strictly.
 ## [Unreleased]
 
 ### Fixed
+- **An invalid source name in the URL is a `404`, not a `500`.** `GET`/`DELETE /api/sources/{name}`
+  passed the raw route segment into the registry store, whose `GetAsync`/`DeleteAsync` threw
+  `ArgumentException` for a name it could never have written — and nothing above catches it, so
+  `GET /api/sources/a%20b` answered **500 with the whole validation regex in the body**, where an
+  unknown-but-legal name has always answered a clean `404`. Reads and deletes now treat such a name as
+  a miss; writes still validate, since there a bad name is the caller's mistake and rejecting it is
+  what keeps it from ever becoming a key or a path. The same lookup backs `source.ref` resolution, so
+  a report referencing an unusable source name now fails with "No source named 'x' is registered"
+  instead of a 500.
+
 - **A dynamic report name could end in a newline.** `DynamicReportName.Pattern` was anchored with
   `$`, which in .NET matches at the end of input **and** immediately before a trailing newline — so
   a name ending in one was accepted. That name is remotely creatable via `POST /api/reports`, and

docs/STATUS-AND-BACKLOG.md

@@ -150,6 +150,26 @@ would escape the per-report skip and stop the host from starting at all. Doing t
 deciding where a code-first name is validated and with which exception, which is a design question,
 and the input is the host developer's own literal rather than anything remote.
 
+### 1e. Windows device names still pass the report/source name grammar — open
+
+`CON`, `NUL`, `PRN`, `AUX`, `COM1`–`COM9` and `LPT1`–`LPT9` satisfy `DynamicReportName`, but a name
+becomes a file name and on Windows `CON.json` resolves to a device: the save throws
+`FileNotFoundException`, so `POST /api/reports` answers `500` for what is really an invalid name.
+Measured, not assumed — CON/NUL/PRN/AUX/COM1/LPT1 all failed to write on Windows 10 while `CON1` wrote
+normally, and `COM0`/`LPT0` turned out to be ordinary writable files despite being documented as
+reserved. No data is at risk: the create path already rolls the registry back.
+
+**Attempted and backed out.** Adding the device names to the pattern looks like a one-line fix and is
+not, because `DynamicReportName.IsValid` serves two roles: a *validator* (reject → 400) and a
+*discriminator* (could a file-backed store hold this?). Narrowing it flips the second role — a
+`source.ref` of `CON` and `GET /api/sources/CON` both turned from a clean 404/400 into a 500, and a
+`nul.json` created on Linux before the change would rehydrate and run but answer `409 "code-registered"`
+on delete, unrecoverable except by deleting the file by hand.
+
+The `500` half of that fallout is fixed (see the source-store lookup change above); the remaining work
+is separating the two roles, and deciding what happens to an already-stored name that the new rule
+would reject. That is a design decision, and the bug it fixes is a wrong status code.
+
 ### 2. CI hardening
 - **Fail (not skip) the Testcontainers integration tests when Docker is absent in CI.** — **done**:
   the five container `ServerFixture`s now swallow a start failure only through an exception filter,

src/NeoReports.Core/SourceRegistry/FileSourceRegistryStore.cs

@@ -46,7 +46,14 @@ public async Task SaveAsync(SourceDefinition definition, CancellationToken cance
     /// <inheritdoc />
     public async Task<SourceDefinition?> GetAsync(string name, CancellationToken cancellationToken)
     {
-        ValidateName(name);
+        // A LOOKUP for a name this store could never have written is a miss, not an error. It used to
+        // throw, and nothing above catches ArgumentException: GET /api/sources/{name} with a name like
+        // "a b" answered 500 with the whole validation regex in the body, where an unknown-but-legal
+        // name answers a clean 404. Writes still validate — there a bad name is the caller's mistake,
+        // and rejecting it is what keeps the name from ever becoming a path.
+        if (!DynamicReportName.IsValid(name))
+            return null;
+
         string path = GetPath(name);
         if (!File.Exists(path))
             return null;
@@ -58,7 +65,12 @@ public async Task SaveAsync(SourceDefinition definition, CancellationToken cance
     /// <inheritdoc />
     public Task<bool> DeleteAsync(string name, CancellationToken cancellationToken)
     {
-        ValidateName(name);
+        // Same rule as GetAsync: nothing under an unwritable name can be there to remove, so report
+        // "removed nothing" rather than throwing at a caller that only asked. Also keeps the name away
+        // from GetPath, which is the whole reason it was validated here.
+        if (!DynamicReportName.IsValid(name))
+            return Task.FromResult(false);
+
         string path = GetPath(name);
         if (!File.Exists(path))
             return Task.FromResult(false);

src/NeoReports.Core/SourceRegistry/ISourceRegistryStore.cs

@@ -5,19 +5,34 @@ namespace NeoReports.Core.SourceRegistry;
 /// including any <c>${VAR}</c> placeholders — the store never resolves environment variables;
 /// that is <see cref="ISourceRegistry"/>'s job, at run time.
 /// </summary>
+/// <remarks>
+/// <b>Names.</b> A write rejects a name the store cannot key — it becomes a file name in a
+/// file-backed implementation, so this is what keeps a caller-supplied name from becoming a path.
+/// A <i>read</i> or <i>delete</i> under such a name is a miss rather than an error: an implementation
+/// returns <c>null</c>/<c>false</c> instead of throwing, because a lookup for something that could
+/// never have been written is simply not found. Throwing there made an endpoint answer 500 for a name
+/// that a plain 404 already describes.
+/// </remarks>
 public interface ISourceRegistryStore
 {
     /// <summary>Creates or fully replaces the definition under <c>definition.Name</c>.</summary>
     /// <param name="definition">The source definition to persist.</param>
+    /// <exception cref="ArgumentException">Thrown when the name is one the store cannot key.</exception>
     /// <param name="cancellationToken">Cancellation token.</param>
     Task SaveAsync(SourceDefinition definition, CancellationToken cancellationToken);
 
-    /// <summary>Reads a source definition by name, or <c>null</c> when it doesn't exist.</summary>
+    /// <summary>
+    /// Reads a source definition by name, or <c>null</c> when it doesn't exist — including when the
+    /// name is one this store could never have written.
+    /// </summary>
     /// <param name="name">The source name.</param>
     /// <param name="cancellationToken">Cancellation token.</param>
     Task<SourceDefinition?> GetAsync(string name, CancellationToken cancellationToken);
 
-    /// <summary>Deletes a source definition. A no-op (returns <c>false</c>) when it doesn't exist.</summary>
+    /// <summary>
+    /// Deletes a source definition. A no-op (returns <c>false</c>) when it doesn't exist, including
+    /// when the name is one this store could never have written.
+    /// </summary>
     /// <param name="name">The source name.</param>
     /// <param name="cancellationToken">Cancellation token.</param>
     /// <returns><c>true</c> when a definition existed and was removed.</returns>

src/NeoReports.Core/SourceRegistry/InMemorySourceRegistryStore.cs

@@ -24,14 +24,24 @@ public Task SaveAsync(SourceDefinition definition, CancellationToken cancellatio
     /// <inheritdoc />
     public Task<SourceDefinition?> GetAsync(string name, CancellationToken cancellationToken)
     {
-        ValidateName(name);
+        // A LOOKUP for a name this store could never have written is a miss, not an error. It used to
+        // throw, and nothing above catches ArgumentException: GET /api/sources/{name} with a name like
+        // "a b" answered 500 with the whole validation regex in the body, where an unknown-but-legal
+        // name answers a clean 404. Writes still validate — there a bad name is the caller's mistake.
+        if (!DynamicReportName.IsValid(name))
+            return Task.FromResult<SourceDefinition?>(null);
+
         return Task.FromResult(_definitions.TryGetValue(name, out SourceDefinition? definition) ? definition : null);
     }
 
     /// <inheritdoc />
     public Task<bool> DeleteAsync(string name, CancellationToken cancellationToken)
     {
-        ValidateName(name);
+        // Same rule as GetAsync: nothing under an unwritable name can be there to remove, so report
+        // "removed nothing" rather than throwing at a caller that only asked.
+        if (!DynamicReportName.IsValid(name))
+            return Task.FromResult(false);
+
         return Task.FromResult(_definitions.TryRemove(name, out _));
     }
 

tests/NeoReports.AspNetCore.IntegrationTests/SourceEndpointTests.cs

@@ -24,6 +24,31 @@ private static void AddSourceRegistryHost(IServiceCollection services)
         services.AddSingleton<ISourceHealthCheck, FakeSourceHealthCheck>();
     }
 
+    /// <summary>
+    /// A route name the registry could never have written used to reach the store's ValidateName,
+    /// which throws ArgumentException — and nothing above catches it, so the caller got a 500 whose
+    /// body carried the whole validation regex. An unknown-but-legal name has always answered a clean
+    /// 404; these now do the same.
+    /// </summary>
+    [Theory]
+    [InlineData("a b")]
+    [InlineData("../evil")]
+    [InlineData("1abc")]
+    public async Task An_unwritable_source_name_is_404_not_500(string name)
+    {
+        using var host = await TestApp.StartAsync(AddSourceRegistryHost);
+        HttpClient client = host.GetTestClient();
+        string escaped = Uri.EscapeDataString(name);
+
+        HttpResponseMessage read = await client.GetAsync($"/api/sources/{escaped}");
+        read.StatusCode.ShouldBe(HttpStatusCode.NotFound);
+
+        // And the answer must not hand back the validation rule.
+        (await read.Content.ReadAsStringAsync()).ShouldNotContain("[a-zA-Z]");
+
+        (await client.DeleteAsync($"/api/sources/{escaped}")).StatusCode.ShouldBe(HttpStatusCode.NotFound);
+    }
+
     /// <summary>
     /// A source's <c>properties</c> bag is typed <c>object?</c>, so System.Text.Json hands each value
     /// over as a <c>JsonElement</c>. <c>FileSourceRegistryStore</c> launders that away by serializing,

tests/NeoReports.Core.UnitTests/SourceRegistry/FileSourceRegistryStoreTests.cs

@@ -128,16 +128,31 @@ public async Task Get_a_corrupt_file_returns_null_rather_than_throwing()
         (await store.GetAsync("broken", CancellationToken.None)).ShouldBeNull();
     }
 
+    /// <summary>
+    /// A write under a name the store cannot key is the caller's mistake and still throws — rejecting
+    /// it is what keeps the name from ever becoming a path. A read or delete is a miss instead: a
+    /// lookup for something that could never have been written is simply not found, and throwing there
+    /// made <c>GET /api/sources/{name}</c> answer 500 (with the validation regex in the body) for a
+    /// name a plain 404 already describes.
+    /// </summary>
     [Theory]
     [InlineData("../evil")]
     [InlineData("a b")]
     [InlineData("")]
-    public async Task Invalid_name_throws_on_every_operation(string invalidName)
+    public async Task Invalid_name_is_refused_on_write_and_a_miss_on_read(string invalidName)
     {
         var store = new FileSourceRegistryStore(_directory);
-        await Should.ThrowAsync<ArgumentException>(() => store.SaveAsync(new SourceDefinition(invalidName, "sql"), CancellationToken.None));
-        await Should.ThrowAsync<ArgumentException>(() => store.GetAsync(invalidName, CancellationToken.None));
-        await Should.ThrowAsync<ArgumentException>(() => store.DeleteAsync(invalidName, CancellationToken.None));
+
+        await Should.ThrowAsync<ArgumentException>(
+            () => store.SaveAsync(new SourceDefinition(invalidName, "sql"), CancellationToken.None));
+
+        (await store.GetAsync(invalidName, CancellationToken.None)).ShouldBeNull();
+        (await store.DeleteAsync(invalidName, CancellationToken.None)).ShouldBeFalse();
+
+        // None of the three touched the filesystem: the store only creates its directory on a
+        // successful write, so its absence is the strongest available proof that the name never
+        // reached GetPath.
+        Directory.Exists(_directory).ShouldBeFalse();
     }
 
     public void Dispose()

tests/NeoReports.Core.UnitTests/SourceRegistry/InMemorySourceRegistryStoreTests.cs

@@ -35,4 +35,34 @@ public async Task List_returns_every_definition_sorted_by_name()
 
         (await store.ListAsync(CancellationToken.None)).Select(d => d.Name).ShouldBe(new[] { "alpha-db", "beta-db" });
     }
+    // A lookup for a name the store could never have written is a miss, not an error. It used to
+    // throw ArgumentException, which nothing above catches — GET /api/sources/{name} with such a name
+    // answered 500 (echoing the validation regex) where an unknown-but-legal name answers 404.
+    [Theory]
+    [InlineData("a b")]
+    [InlineData("../evil")]
+    [InlineData("1abc")]
+    [InlineData("")]
+    public async Task A_lookup_for_an_unwritable_name_is_a_miss(string name)
+    {
+        var store = new InMemorySourceRegistryStore();
+        await store.SaveAsync(new SourceDefinition("real-db", "sql"), CancellationToken.None);
+
+        (await store.GetAsync(name, CancellationToken.None)).ShouldBeNull();
+        (await store.DeleteAsync(name, CancellationToken.None)).ShouldBeFalse();
+
+        // And the store is untouched by the attempt.
+        (await store.ListAsync(CancellationToken.None)).Count.ShouldBe(1);
+    }
+
+    [Fact]
+    public async Task A_write_under_an_unwritable_name_still_throws()
+    {
+        var store = new InMemorySourceRegistryStore();
+
+        // Writes keep validating: there a bad name is the caller's mistake, and rejecting it is what
+        // keeps the name from ever becoming a key or a path.
+        await Should.ThrowAsync<ArgumentException>(
+            () => store.SaveAsync(new SourceDefinition("a b", "sql"), CancellationToken.None));
+    }
 }