From 5dff83cfb8d227f2da660a9c18747b39f5261271 Mon Sep 17 00:00:00 2001 From: AJ Isaacs Date: Mon, 19 Oct 2026 01:30:37 -0400 Subject: [PATCH] fix(server): reject duplicate nest ids, invalid metadata and bare --database - POST /api/nests with an existing id (active or deleted) now returns 409 with the current record instead of a 500 from the UNIQUE constraint. - Uploaded nest metadata must have string text fields and a defined status; null fields and undefined statuses (e.g. 99) are 400 instead of a 500 or a stored invalid value. - --database is accepted alongside --database=; a missing or repeated path fails startup instead of silently using OPENNEST_DB/default. Co-Authored-By: Claude Opus 5.5 --- .../NestUploadValidationTests.cs | 152 ++++++++++++++++++ OpenNest.Server.Tests/ServerOptionsTests.cs | 34 ++++ OpenNest.Server/NestDatabase.cs | 6 +- OpenNest.Server/NestMetadata.cs | 60 +++++++ OpenNest.Server/Program.cs | 40 +---- OpenNest.Server/ServerOptions.cs | 43 +++++ docs/nest-storage.md | 11 +- 7 files changed, 309 insertions(+), 37 deletions(-) create mode 100644 OpenNest.Server.Tests/NestUploadValidationTests.cs create mode 100644 OpenNest.Server.Tests/ServerOptionsTests.cs create mode 100644 OpenNest.Server/NestMetadata.cs create mode 100644 OpenNest.Server/ServerOptions.cs diff --git a/OpenNest.Server.Tests/NestUploadValidationTests.cs b/OpenNest.Server.Tests/NestUploadValidationTests.cs new file mode 100644 index 00000000..4256fe02 --- /dev/null +++ b/OpenNest.Server.Tests/NestUploadValidationTests.cs @@ -0,0 +1,152 @@ +using System.Net; +using System.Net.Http.Json; +using System.Text; +using System.Text.Json; +using OpenNest.Data; + +namespace OpenNest.Server.Tests; + +/// +/// Client-allocated ids and uploaded nest metadata: conflicts are 409 and invalid +/// metadata is 400, never a 500, and neither changes stored records. +/// +public sealed class NestUploadValidationTests +{ + private static readonly JsonSerializerOptions Options = new(JsonSerializerDefaults.Web) + { Converters = { new System.Text.Json.Serialization.JsonStringEnumConverter() } }; + + [Fact] + public async Task Create_WithExistingId_Returns409WithCurrentRecordAndKeepsIt() + { + using var factory = new ServerFactory(); + using var client = factory.CreateClient(); + var id = Guid.NewGuid(); + using var created = await client.PostAsync("/api/nests", NestRevisionTests.Body(new NestRecord { Id = id, Name = "Original" }, [1])); + Assert.Equal(HttpStatusCode.OK, created.StatusCode); + + using var duplicate = await client.PostAsync("/api/nests", NestRevisionTests.Body(new NestRecord { Id = id, Name = "Replacement" }, [2])); + + Assert.Equal(HttpStatusCode.Conflict, duplicate.StatusCode); + var current = (await duplicate.Content.ReadFromJsonAsync(Options))!; + Assert.Equal(id, current.Id); + Assert.Equal("Original", current.Name); + Assert.Equal(new byte[] { 1 }, await client.GetByteArrayAsync($"/api/nests/{id}/file")); + using var reopened = new NestDatabase(factory.DatabasePath, pooling: false); + Assert.Equal(1, reopened.History(id, 0, 100)!.Total); + } + + [Fact] + public async Task Create_WithIdOfDeletedNest_Returns409AndKeepsItRecoverable() + { + using var factory = new ServerFactory(); + using var client = factory.CreateClient(); + var id = Guid.NewGuid(); + using var created = await client.PostAsync("/api/nests", NestRevisionTests.Body(new NestRecord { Id = id, Name = "Original" }, [1])); + using var delete = new HttpRequestMessage(HttpMethod.Delete, $"/api/nests/{id}"); + delete.Headers.TryAddWithoutValidation("If-Match", "\"1\""); + using var deleted = await client.SendAsync(delete); + Assert.Equal(HttpStatusCode.OK, deleted.StatusCode); + + using var duplicate = await client.PostAsync("/api/nests", NestRevisionTests.Body(new NestRecord { Id = id, Name = "Replacement" }, [2])); + + Assert.Equal(HttpStatusCode.Conflict, duplicate.StatusCode); + var current = (await duplicate.Content.ReadFromJsonAsync(Options))!; + Assert.True(current.Deleted); + Assert.Equal(2, current.Revision); + Assert.Equal(HttpStatusCode.NotFound, (await client.GetAsync($"/api/nests/{id}")).StatusCode); + Assert.Equal(new byte[] { 1 }, await client.GetByteArrayAsync($"/api/nests/deleted/{id}/file")); + } + + [Theory] + [InlineData("""{"name":null}""")] + [InlineData("""{"name":"A","customer":null}""")] + [InlineData("""{"name":"A","material":null}""")] + [InlineData("""{"name":"A","comments":null}""")] + [InlineData("""{"name":"A","madeBy":null}""")] + [InlineData("""{"name":"A","status":99}""")] + [InlineData("""{"name":"A","status":"cancelled"}""")] + public async Task Create_WithInvalidMetadata_Returns400AndStoresNothing(string metadata) + { + using var factory = new ServerFactory(); + using var client = factory.CreateClient(); + + using var response = await client.PostAsync("/api/nests", RawBody(metadata, [1])); + + Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); + using var reopened = new NestDatabase(factory.DatabasePath, pooling: false); + Assert.Empty(reopened.List()); + } + + [Theory] + [InlineData("""{"name":"A","status":"toBeCut"}""", NestStatus.ToBeCut)] + [InlineData("""{"name":"A","status":2}""", NestStatus.HasBeenCut)] + public async Task Create_WithDefinedStatus_StoresIt(string metadata, NestStatus expected) + { + using var factory = new ServerFactory(); + using var client = factory.CreateClient(); + + using var response = await client.PostAsync("/api/nests", RawBody(metadata, [1])); + + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + Assert.Equal(expected, (await response.Content.ReadFromJsonAsync(Options))!.Status); + } + + [Theory] + [InlineData("""{"name":null}""")] + [InlineData("""{"name":"A","status":99}""")] + public async Task UpdateFile_WithInvalidMetadata_Returns400AndLeavesRecordUnchanged(string metadata) + { + using var factory = new ServerFactory(); + using var client = factory.CreateClient(); + var id = await Create(client); + client.DefaultRequestHeaders.TryAddWithoutValidation("If-Match", "\"1\""); + + using var response = await client.PutAsync($"/api/nests/{id}/file", RawBody(metadata, [2])); + + Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); + await AssertUnchanged(factory, client, id); + } + + [Theory] + [InlineData("""{"name":null}""")] + [InlineData("""{"name":"A","comments":null}""")] + [InlineData("""{"name":"A","status":99}""")] + public async Task UpdateMetadata_WithInvalidMetadata_Returns400AndLeavesRecordUnchanged(string metadata) + { + using var factory = new ServerFactory(); + using var client = factory.CreateClient(); + var id = await Create(client); + client.DefaultRequestHeaders.TryAddWithoutValidation("If-Match", "\"1\""); + + using var response = await client.PutAsync($"/api/nests/{id}/metadata", + new StringContent(metadata, Encoding.UTF8, "application/json")); + + Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); + await AssertUnchanged(factory, client, id); + } + + private static async Task Create(HttpClient client) + { + using var created = await client.PostAsync("/api/nests", NestRevisionTests.Body(new NestRecord { Name = "Original" }, [1])); + Assert.Equal(HttpStatusCode.OK, created.StatusCode); + return (await created.Content.ReadFromJsonAsync(Options))!.Id; + } + + private static async Task AssertUnchanged(ServerFactory factory, HttpClient client, Guid id) + { + Assert.Equal(new byte[] { 1 }, await client.GetByteArrayAsync($"/api/nests/{id}/file")); + using var reopened = new NestDatabase(factory.DatabasePath, pooling: false); + var current = reopened.Get(id)!; + Assert.Equal("Original", current.Name); + Assert.Equal(1, current.Revision); + Assert.Equal(1, reopened.History(id, 0, 100)!.Total); + } + + private static MultipartFormDataContent RawBody(string metadata, byte[] bytes) + { + var body = new MultipartFormDataContent(); + body.Add(new StringContent(metadata), "metadata"); + body.Add(new ByteArrayContent(bytes), "file", "fixture.nest"); + return body; + } +} diff --git a/OpenNest.Server.Tests/ServerOptionsTests.cs b/OpenNest.Server.Tests/ServerOptionsTests.cs new file mode 100644 index 00000000..b82a0c33 --- /dev/null +++ b/OpenNest.Server.Tests/ServerOptionsTests.cs @@ -0,0 +1,34 @@ +namespace OpenNest.Server.Tests; + +public sealed class ServerOptionsTests +{ + private static readonly string Default = Path.Combine("data", "nests.db"); + + [Theory] + [InlineData(new[] { "--database=/srv/a.db" }, "/srv/a.db")] + [InlineData(new[] { "--database", "/srv/a.db" }, "/srv/a.db")] + [InlineData(new[] { "--urls=http://127.0.0.1:8090", "--database", "/srv/a.db" }, "/srv/a.db")] + [InlineData(new[] { "--database=/srv/a=b.db" }, "/srv/a=b.db")] + public void DatabasePath_AcceptsBothForms_AndOverridesEnvironment(string[] args, string expected) + { + Assert.Equal(expected, ServerOptions.DatabasePath(args, "/env/nests.db")); + } + + [Fact] + public void DatabasePath_WithoutArgument_UsesEnvironmentThenDefault() + { + Assert.Equal("/env/nests.db", ServerOptions.DatabasePath(["--urls=http://127.0.0.1:8090"], "/env/nests.db")); + Assert.Equal(Default, ServerOptions.DatabasePath([], null)); + } + + [Theory] + [InlineData("--database")] + [InlineData("--database", "--urls=http://127.0.0.1:8090")] + [InlineData("--database=")] + [InlineData("--database= ")] + [InlineData("--database=/a.db", "--database", "/b.db")] + public void DatabasePath_MissingOrRepeated_FailsInsteadOfFallingBack(params string[] args) + { + Assert.Throws(() => ServerOptions.DatabasePath(args, "/env/nests.db")); + } +} diff --git a/OpenNest.Server/NestDatabase.cs b/OpenNest.Server/NestDatabase.cs index 1456cc41..2c982ccc 100644 --- a/OpenNest.Server/NestDatabase.cs +++ b/OpenNest.Server/NestDatabase.cs @@ -142,6 +142,9 @@ public sealed partial class NestDatabase : IDisposable lock (_sync) { using var transaction = _connection.BeginTransaction(); + // Ids are client-allocated: an existing id, active or deleted, is a conflict, not a replacement. + if ((Get(id) ?? Get(id, deleted: true)) is { } existing) + throw new RevisionConflictException(existing, "A nest with this id already exists."); using var command = _connection.CreateCommand(); command.Transaction = transaction; command.CommandText = """ @@ -488,7 +491,8 @@ public sealed partial class NestDatabase : IDisposable } } -public sealed class RevisionConflictException(NestRecord current) : Exception("The nest has changed. Reload or Save As.") +public sealed class RevisionConflictException(NestRecord current, string message = "The nest has changed. Reload or Save As.") + : Exception(message) { public NestRecord Current { get; } = current; } diff --git a/OpenNest.Server/NestMetadata.cs b/OpenNest.Server/NestMetadata.cs new file mode 100644 index 00000000..9a7d53de --- /dev/null +++ b/OpenNest.Server/NestMetadata.cs @@ -0,0 +1,60 @@ +using System.Text.Json; +using System.Text.Json.Serialization; +using OpenNest.Data; + +namespace OpenNest.Server; + +/// +/// Reads and checks uploaded nest metadata. Text fields must be present and the status +/// must be a defined value, so invalid metadata is a 400 and never reaches storage. +/// +internal static class NestMetadata +{ + // Integer statuses stay accepted for existing callers; GetError refuses undefined ones. + public static readonly JsonSerializerOptions Options = new(JsonSerializerDefaults.Web) + { + PropertyNameCaseInsensitive = true, + Converters = { new JsonStringEnumConverter(JsonNamingPolicy.CamelCase) }, + }; + + public static bool TryRead(string? json, out NestRecord record, out string error) + { + record = new NestRecord(); + if (string.IsNullOrWhiteSpace(json)) + { + error = "Missing 'metadata' part."; + return false; + } + + try + { + if (JsonSerializer.Deserialize(json, Options) is not { } parsed) + { + error = "Missing metadata."; + return false; + } + + record = parsed; + } + catch (JsonException) + { + error = "Invalid metadata JSON."; + return false; + } + + error = GetError(record) ?? ""; + return error.Length == 0; + } + + public static string? GetError(NestRecord record) + { + if (record.Name is null) return "name must be a string."; + if (record.Customer is null) return "customer must be a string."; + if (record.Material is null) return "material must be a string."; + if (record.Comments is null) return "comments must be a string."; + if (record.MadeBy is null) return "madeBy must be a string."; + if (!Enum.IsDefined(record.Status)) return "status must be quote, toBeCut or hasBeenCut."; + if (!double.IsFinite(record.Thickness)) return "thickness must be a finite number."; + return null; + } +} diff --git a/OpenNest.Server/Program.cs b/OpenNest.Server/Program.cs index eebe89ba..677ea6ff 100644 --- a/OpenNest.Server/Program.cs +++ b/OpenNest.Server/Program.cs @@ -20,11 +20,8 @@ builder.Services.ConfigureHttpJsonOptions(options => options.SerializerOptions.Converters.Add(converter); }); -// Storage path: --database or OPENNEST_DB, default ./data/nests.db (a Docker volume). -var databasePath = - args.FirstOrDefault(a => a.StartsWith("--database=", StringComparison.Ordinal))?.Split('=', 2)[1] - ?? Environment.GetEnvironmentVariable("OPENNEST_DB") - ?? Path.Combine("data", "nests.db"); +// Storage path: --database= or --database , then OPENNEST_DB, default ./data/nests.db (a Docker volume). +var databasePath = ServerOptions.DatabasePath(args, Environment.GetEnvironmentVariable(ServerOptions.DatabaseVariable)); builder.Services.AddSingleton(_ => new NestDatabase(databasePath)); // Listen port: --urls or ASPNETCORE_URLS; Dockerfile defaults to 8090. @@ -60,7 +57,8 @@ app.MapPost("/api/nests", async (HttpContext http, NestDatabase db) => return error; var id = record!.Id == Guid.Empty ? Guid.NewGuid() : record.Id; - return Results.Ok(db.Insert(id, record, file!)); + try { return Results.Ok(db.Insert(id, record, file!)); } + catch (RevisionConflictException conflict) { return Results.Json(conflict.Current, statusCode: 409); } }); app.MapGet("/api/nests/{id:guid}/revisions", (Guid id, HttpRequest request, NestDatabase db) => ReadHistory(id, request, db)); @@ -90,10 +88,11 @@ app.MapPut("/api/nests/{id:guid}/metadata", async (Guid id, HttpRequest request, var error = IfMatch.Read(request, out var expected); if (error is not null) return error; NestRecord? record; - try { record = await request.ReadFromJsonAsync(); } + try { record = await request.ReadFromJsonAsync(NestMetadata.Options); } catch (Exception ex) when (ex is JsonException or InvalidOperationException or BadHttpRequestException) { return Results.BadRequest("Invalid metadata JSON."); } if (record is null) return Results.BadRequest("Missing metadata."); + if (NestMetadata.GetError(record) is { } invalid) return Results.BadRequest(invalid); try { var updated = db.UpdateMetadata(id, record, expected); @@ -184,22 +183,8 @@ static async Task<(NestRecord? Record, byte[]? File, IResult? Error)> ReadMultip return (null, null, Results.BadRequest("Malformed multipart body.")); } - var metadataPart = form["metadata"].FirstOrDefault(); - if (string.IsNullOrWhiteSpace(metadataPart)) - return (null, null, Results.BadRequest("Missing 'metadata' part.")); - - NestRecord? record; - try - { - record = JsonSerializer.Deserialize(metadataPart, MultipartJson.Options); - } - catch (JsonException) - { - return (null, null, Results.BadRequest("Invalid metadata JSON.")); - } - - if (record is null) - return (null, null, Results.BadRequest("Missing metadata.")); + if (!NestMetadata.TryRead(form["metadata"].FirstOrDefault(), out var record, out var metadataError)) + return (null, null, Results.BadRequest(metadataError)); var filePart = form.Files["file"]; if (filePart is null || filePart.Length == 0) @@ -211,14 +196,5 @@ static async Task<(NestRecord? Record, byte[]? File, IResult? Error)> ReadMultip return (record, buffer.ToArray(), null); } -internal static class MultipartJson -{ - public static readonly JsonSerializerOptions Options = new(JsonSerializerDefaults.Web) - { - PropertyNameCaseInsensitive = true, - Converters = { new JsonStringEnumConverter(JsonNamingPolicy.CamelCase) }, - }; -} - // Entry-point type for in-memory integration tests (WebApplicationFactory). public partial class Program { } diff --git a/OpenNest.Server/ServerOptions.cs b/OpenNest.Server/ServerOptions.cs new file mode 100644 index 00000000..849b5cef --- /dev/null +++ b/OpenNest.Server/ServerOptions.cs @@ -0,0 +1,43 @@ +namespace OpenNest.Server; + +/// Startup settings read from the command line and environment. +public static class ServerOptions +{ + public const string DatabaseVariable = "OPENNEST_DB"; + + /// + /// The database path: --database=<path> or --database <path>, then + /// OPENNEST_DB, then ./data/nests.db. A --database without a path, + /// or given twice, fails startup instead of silently storing nests elsewhere. + /// + public static string DatabasePath(IReadOnlyList args, string? environmentPath) + { + string? path = null; + for (var i = 0; i < args.Count; i++) + { + string value; + if (args[i] == "--database") + { + if (i + 1 >= args.Count || args[i + 1].StartsWith("--", StringComparison.Ordinal)) + throw new ArgumentException("--database requires a path.", nameof(args)); + value = args[++i]; + } + else if (args[i].StartsWith("--database=", StringComparison.Ordinal)) + { + value = args[i]["--database=".Length..]; + } + else + { + continue; + } + + if (string.IsNullOrWhiteSpace(value)) + throw new ArgumentException("--database requires a path.", nameof(args)); + if (path is not null) + throw new ArgumentException("--database was given more than once.", nameof(args)); + path = value; + } + + return path ?? environmentPath ?? Path.Combine("data", "nests.db"); + } +} diff --git a/docs/nest-storage.md b/docs/nest-storage.md index 3295b94b..b5ffc165 100644 --- a/docs/nest-storage.md +++ b/docs/nest-storage.md @@ -65,7 +65,9 @@ installs are unaffected until an operator opts in via **File > Storage Mode...** (`RemoteNestRepository`) and the server (`OpenNest.Server`). JSON is camelCase with enums serialized as strings (`JsonSerializerDefaults.Web` + `JsonStringEnumConverter(JsonNamingPolicy.CamelCase)`), matching the rest of -`OpenNest.Data`'s local JSON stores. +`OpenNest.Data`'s local JSON stores. Uploaded metadata (POST and both PUTs) must +carry string text fields (`null` is 400) and a defined `status`: a name, or for +existing callers its integer value 0–2; an undefined value is 400 and stores nothing. | Field | Type | Notes | |---|---|---| @@ -94,7 +96,7 @@ with enums serialized as strings (`JsonSerializerDefaults.Web` + | GET | `/api/nests/query?search=&sort=&order=&offset=&limit=` | — | `{ "items": NestRecord[], "total", "offset", "limit" }`: one bounded page filtered in SQL (see below), or 400 for an invalid parameter | | GET | `/api/nests/{id}` | — | `NestRecord` or 404 | | GET | `/api/nests/{id}/file` | — | `.nest` archive bytes (`application/zip`) or 404 | -| POST | `/api/nests` | multipart: `metadata` (JSON `NestRecord`) + `file` (`.nest` bytes) | `NestRecord` with server-assigned `id` when the client sends an empty guid | +| POST | `/api/nests` | multipart: `metadata` (JSON `NestRecord`) + `file` (`.nest` bytes) | `NestRecord` with server-assigned `id` when the client sends an empty guid; 409 with the current record (active or deleted) when the `id` already exists, storing nothing | | PUT | `/api/nests/{id}/file` | multipart: `metadata` + `file` | Updated `NestRecord`, or 404 if `id` is unknown | | PUT | `/api/nests/{id}/metadata` | JSON `NestRecord` | Updated `NestRecord` (archive untouched), or 404 | | DELETE | `/api/nests/{id}` | — | Updated hidden `NestRecord`, or 404 | @@ -291,8 +293,9 @@ revision; save the nest to keep it. `OpenNest.Server.NestDatabase` uses SQLite (`Microsoft.Data.Sqlite`) with `journal_mode=WAL`. Each row holds the metadata columns plus the `.nest` -archive as a `BLOB`. The database file path comes from `--database=`, -then `OPENNEST_DB`, defaulting to `./data/nests.db`. The server opens the +archive as a `BLOB`. The database file path comes from `--database=` or +`--database `, then `OPENNEST_DB`, defaulting to `./data/nests.db`; a +`--database` without a path, or given twice, fails startup. The server opens the database at startup, so an unusable path fails startup rather than the first request. Run **one service instance with its database on a local filesystem**. Multiple PCs