feat(api): optimistic concurrency on report editing (ADR D87)

Closes the gap D86 recorded and left open. A redaction placeholder carries
the address of the slot its value came from, which is what makes a single
editor's reorder safe — but the address names a slot of the document as it
was at the GET, and the PUT resolves it against the document as it is now.
Between the two, another editor can reorder the destinations, and
destinations[0] then addresses a different bucket than the one the first
editor was shown: the wrong section's credential, restored into a section
the caller defined.

GET /reports/{name}/config now returns an ETag and PUT /reports/{name}
honours If-Match with a 412. The header is optional — requiring it would
break every client of an endpoint that shipped one release ago, and the
realistic scenario is two people in the Builder, which always sends it.
The comparison runs against the same stored read Restore later merges
against; re-reading would reopen the window inside the handler.

The security pass corrected the central choice. The first cut hashed the
STORED document, reasoning that it is what Restore resolves against. That
made the tag a free offline verification oracle for the values the
endpoint exists to withhold: the redacted body and the stored document are
byte-identical apart from those values, so a caller could reconstruct
candidates, hash them, and confirm a guessed connection string with no
failed login to notice. The tag is now over the REDACTED form — the bytes
the caller already holds — which carries no information at all and is
still the right validator, because an address is invalidated by a change
to the document's structure and that structure is fully visible there. A
keyed MAC also closes it but its tags differ between instances, so a
load-balanced host would answer 412 at random.

Code review caught two client-half defects, both of which left the feature
working and the user unable to act on it: the 412 was ProblemDetails while
the client reads `error`, so the user was told the configuration was
invalid and never told to reload; and OriginalVersion was captured once
and never advanced, so any second save from the same page — a retry after
"Run now" failed to start, a double-click — was a guaranteed 412 naming a
conflict with the save that had just succeeded. A successful PUT now
returns the new ETag and the wizard adopts it.

Not fixed, recorded in the ADR: the check is still check-then-act, so a
third writer inside the same instant is not caught. That needs a
compare-and-swap on IReportConfigStore — an interface every custom store
implements — for a race orders of magnitude smaller than the human one
this closes.

Each fix verified by reverting it and confirming the new test fails.
Full suite: 1 687 green across 33 projects.

Author: thiagoluga

CHANGELOG.md

@@ -8,6 +8,16 @@ The `NeoReports.Abstractions` contract follows SemVer strictly.
 
 ## [Unreleased]
 
+### Added
+- **Optimistic concurrency on report editing (ADR D87).** `GET /api/reports/{name}/config` now returns
+  an `ETag`, and `PUT /api/reports/{name}` honours `If-Match`, answering `412 Precondition Failed`
+  when the stored document changed since it was read. This closes the window D86 recorded and left
+  open: the redaction placeholder carries the address of the slot its value came from, so if another
+  editor reorders the destinations between the `GET` and the `PUT`, `destinations[0]` addresses a
+  different bucket than the one the first editor was shown — and the wrong section's credential is
+  restored. The header is **optional**, so clients that do not send it keep working exactly as before;
+  the Builder always sends it.
+
 ### Fixed
 - **Editing a report in the Builder no longer starts from a blank form (ADR D86).** Reported by the
   maintainer: *Edit* prefilled almost nothing the report actually reads from — source type, query, key

DECISIONS.md

@@ -2129,3 +2129,74 @@ formats was opened in the Builder — every step prefilled — its page size cha
 stored document came back with the password intact, the filter intact, the column type and display
 name intact, `90` still a number, and the new page size applied. Saving with no changes at all
 reproduces the original document.
+
+## D87 — Optimistic concurrency on report editing (2026-08-18)
+
+Closes the one gap D86 recorded and did not fix.
+
+### The window
+
+D86's redaction placeholder carries the address of the slot its value came from —
+`${neoreports:redacted:destinations[1]}` — so a client may reorder, remove or retype sections and each
+placeholder still resolves to its own stored value. That is exactly what makes a *single* editor safe.
+
+It is blind in the other direction. The address names a slot of the document **as it was at the
+`GET`**, and `PUT` resolves it against the document **as it is now**. Between the two, another editor
+can reorder the destinations, and `destinations[0]` then addresses a different bucket than the one the
+first editor was shown. The result is the wrong section's credential restored into a section the
+caller defined — the precise outcome the address exists to prevent, reached through the stored side
+rather than the incoming one.
+
+### The decision: an HTTP validator, not a lock
+
+`GET /reports/{name}/config` returns an `ETag`. `PUT /reports/{name}` accepts `If-Match` and answers
+`412 Precondition Failed` when it no longer matches. Three properties matter:
+
+- **The validator is over the *redacted* form — the bytes the client was given.** The first cut hashed
+  the *stored* document, on the reasoning that it is what `Restore` resolves against. The security pass
+  showed that reasoning bought a real weakening: the redacted body and the stored document are
+  byte-identical apart from the redacted values, so publishing a hash of the stored document hands any
+  caller a free, offline **verification oracle** — reconstruct a candidate document with a guessed
+  connection string, hash it, compare. The secret still never leaves the host, but the host would
+  confirm guesses about it, at unlimited speed and with no failed login on the database to notice.
+  Hashing the redacted form removes that entirely: the tag carries nothing the client does not already
+  hold. It is also still the *right* validator, which is the part that makes this cheap — an address is
+  invalidated by a change to the document's **structure** (sections added, removed, reordered), and
+  that structure is wholly visible in the redacted body. A change to a secret *value* moves no address,
+  and an editor sending a placeholder back is asking for whatever is stored now, so resolving to the
+  newer secret is the correct outcome rather than a conflict.
+- **A keyed MAC was considered and rejected.** An HMAC under a per-process key also removes the oracle,
+  but its tags do not survive a restart and, worse, differ between instances — a load-balanced host
+  would answer `412` at random depending on which instance received the save. Deriving the key from
+  Data Protection would fix that and costs a dependency and a key-management story, for no benefit over
+  simply not hashing the secret in the first place.
+- **`If-Match` is optional, and honoured when sent.** Requiring it would break every existing client
+  of an endpoint that shipped one release ago, to protect against a window those clients are not
+  exposed to any more than they were yesterday. The realistic scenario is two people in the Builder,
+  and the Builder always sends it. A caller that omits the header gets exactly D86's behaviour.
+- **The comparison happens against the same read that `Restore` uses.** Reading the stored document
+  once and using it for both the check and the merge is what makes the check meaningful; re-reading
+  would reintroduce the window inside the handler.
+
+`*` is accepted with its RFC 9110 meaning — "if the resource exists" — which the handler has already
+established by the time it looks.
+
+### Why not a lock, a version column, or a last-modified date
+
+A lock needs a lifetime and an owner, and a browser tab that closes has neither. A version number in
+the document would be config the user can edit, and editing it is exactly what the field must survive.
+`Last-Modified` has one-second resolution, which is wider than the window it is guarding.
+
+### Not covered
+
+Two editors whose changes do not overlap still lose one of them — the second gets a `412` and has to
+reload. Merging concurrent edits is out of scope for v1 and would need a per-field model the config
+document does not have.
+
+The check is also **check-then-act, not atomic**: `IReportConfigStore` has no compare-and-swap, so a
+third writer landing between the comparison and `SaveAsync` is still not caught. That residual window
+is microseconds of in-process work rather than the human-scale minutes an editor spends on a form,
+which is the window worth closing and the one this closes. Making it atomic would mean a new
+store-contract method — a change to an interface every custom store implements, for a race that needs
+two saves inside the same instant. Recorded rather than papered over: this narrows the window, it does
+not serialise writes.

src/Integrations/NeoReports.AspNetCore/NeoReportsEndpointRouteBuilderExtensions.cs

@@ -407,7 +407,13 @@ private static async Task<IResult> GetReportConfigAsync(
 
         try
         {
-            return Results.Text(ReportConfigSecrets.Redact(document), "application/json");
+            string redacted = ReportConfigSecrets.Redact(document);
+
+            // ADR D87. Deliberately a validator over what the CLIENT can see: hashing the stored
+            // document instead would let a caller confirm a guessed connection string offline, since
+            // the two differ only in the redacted values.
+            http.Response.Headers.ETag = ReportConfigETag.For(document);
+            return Results.Text(redacted, "application/json");
         }
         catch (ConfigurationException ex)
         {
@@ -597,6 +603,26 @@ private static async Task<IResult> ReplaceReportAsync(
                 statusCode: StatusCodes.Status500InternalServerError);
         }
 
+        // Optimistic concurrency (ADR D87), checked against the very document Restore is about to
+        // resolve against — re-reading here would reopen the window it exists to close. A request
+        // without If-Match states no precondition and behaves exactly as it did before D87.
+        if (!ReportConfigETag.Allows(http.Request.Headers.IfMatch, stored))
+        {
+            // Shaped like every other rejection this endpoint returns, not as ProblemDetails: the
+            // client reads `error`, so a ProblemDetails body left it with nothing to show and the
+            // user was told the configuration was invalid instead of being told to reload — which is
+            // the entire point of answering 412 rather than saving.
+            return Results.Json(
+                new
+                {
+                    error = $"'{name}' changed since you opened it — another editor saved it in the " +
+                            "meantime. Reload the report and apply your change again. Saving now could " +
+                            "resolve a held-back value against a section that is no longer the one it " +
+                            "came from.",
+                },
+                statusCode: StatusCodes.Status412PreconditionFailed);
+        }
+
         string document = await ReadBodyAsync(http, cancellationToken).ConfigureAwait(false);
 
         ReportConfig config;
@@ -668,6 +694,12 @@ private static async Task<IResult> ReplaceReportAsync(
 
         await ReconcileScheduleAsync(name, compiled, scheduler, http, cancellationToken).ConfigureAwait(false);
 
+        // The validator for what was just stored (ADR D87). Without it an editor's captured tag goes
+        // stale the instant its own save succeeds, so any second save from the same page — a retry
+        // after "Run now" failed to start, a double-click — would be refused with a 412 that names a
+        // conflict with itself.
+        http.Response.Headers.ETag = ReportConfigETag.For(document);
+
         var columns = compiled.Schema.Columns.Select(c => c.Name).ToArray();
         return Results.Ok(new ReportCreatedResponse(name, columns));
     }

src/Integrations/NeoReports.AspNetCore/README.md

@@ -16,7 +16,7 @@ reports and jobs, via Minimal API.
 | GET | `/reports/{name}/config` | the stored config document, credential-bearing values replaced by `${neoreports:redacted}` (ADR D86) → `404` for a code-registered report |
 | POST | `/reports` | register a report at runtime from a config document → `201` (`409` if the name exists, `400` if the config is invalid) |
 | POST | `/reports/validate` | dry-run compile a config document → `200 { valid, error, name, columns, nameTaken }`; never registers or persists |
-| PUT | `/reports/{name}` | replace a runtime-registered report in one step → `200` (`400` if the config is invalid — nothing is changed, `409` for a code-registered report, `404` if unknown) |
+| PUT | `/reports/{name}` | replace a runtime-registered report in one step → `200` (`400` if the config is invalid — nothing is changed, `409` for a code-registered report, `404` if unknown, `412` if `If-Match` no longer matches the stored document) |
 | DELETE | `/reports/{name}` | remove a runtime-registered report → `204` (`409` for a code-registered report, `404` if unknown) |
 | GET | `/capabilities` | source/format/destination type ids the host has registered |
 | GET | `/jobs` | list jobs, filterable by `status`/`report`/`since`, paged (`limit` ≤ 200, `offset`) |
@@ -39,6 +39,14 @@ by the reserved placeholder `${neoreports:redacted}`; sending that placeholder b
 the user having to retype a connection string, and without the secret ever leaving the host. A
 `${VAR}` placeholder is not a secret and comes back verbatim.
 
+**Concurrent edits.** That same response carries an `ETag`, computed over the **stored** document
+rather than the redacted body. Send it back as `If-Match` on the `PUT` and the engine answers
+`412 Precondition Failed` when another editor saved in between: a placeholder addressed
+`destinations[0]` would otherwise resolve against a document whose destinations have since been
+reordered, restoring the wrong section's credential (ADR D87). The header is **optional** — a request
+that omits it states no precondition and behaves as it did before D87 — and a successful `PUT` returns
+the new `ETag`, so an editor can save twice in a row.
+
 `POST /reports/validate?for={name}` resolves the placeholder the same way, so a dry run means the
 same thing while editing as it does while creating. `POST /reports` rejects the placeholder outright:
 there is no stored document to resolve it against.

src/Integrations/NeoReports.AspNetCore/ReportConfigETag.cs

@@ -0,0 +1,81 @@
+using System.Security.Cryptography;
+using System.Text;
+using Microsoft.Extensions.Primitives;
+using NeoReports.Abstractions;
+using NeoReports.Core.Configuration;
+
+namespace NeoReports.AspNetCore;
+
+/// <summary>
+/// The entity-tag for a report configuration document (ADR D87). Lets an editor detect that the
+/// document changed under it between reading the configuration and saving it back.
+/// </summary>
+/// <remarks>
+/// <para>
+/// Computed over the <b>redacted</b> form — the same bytes the client was given — and never over the
+/// stored document. Hashing the stored document made the tag a free, offline <i>verification oracle</i>
+/// for the very values the endpoint exists to withhold: the redacted body and the stored document are
+/// byte-identical apart from the redacted values, so a holder of the body could reconstruct candidate
+/// documents, hash them, and confirm a guessed connection string without ever contacting the database.
+/// </para>
+/// <para>
+/// It is still the right validator. What a placeholder's address can be invalidated by is a change to
+/// the document's <i>structure</i> — sections added, removed or reordered — and that structure is
+/// wholly visible in the redacted body. A change to a secret <i>value</i> moves no address, and an
+/// editor that sends a placeholder back is asking for whatever is stored now, so resolving to the
+/// newer secret is the correct outcome rather than a conflict.
+/// </para>
+/// </remarks>
+internal static class ReportConfigETag
+{
+    /// <summary>The strong entity-tag for a stored document, quoted, ready for a header.</summary>
+    /// <param name="storedDocument">The document as the config store holds it.</param>
+    /// <exception cref="ConfigurationException">Thrown when the document is not readable.</exception>
+    /// <remarks>
+    /// Takes the <i>stored</i> document and redacts it here rather than offering an overload for the
+    /// already-redacted body: one entry point means no call site can hash the wrong form, which is the
+    /// mistake this whole type is now shaped to prevent.
+    /// </remarks>
+    internal static string For(string storedDocument)
+    {
+        byte[] hash = SHA256.HashData(Encoding.UTF8.GetBytes(ReportConfigSecrets.Redact(storedDocument)));
+
+        // Base64url so the value needs no escaping inside the quotes, and truncated to 128 bits —
+        // this detects an edit, it does not authenticate one, and a shorter header reads better in a
+        // log. Collisions at this width are not reachable by a report being edited twice.
+        return '"' + Convert.ToBase64String(hash, 0, 16).Replace('+', '-').Replace('/', '_').TrimEnd('=') + '"';
+    }
+
+    /// <summary>
+    /// Whether the request may proceed under RFC 9110 <c>If-Match</c>: no header at all means "no
+    /// precondition" (D87 keeps the header optional so clients from before it still work), <c>*</c>
+    /// means "if the resource exists", and otherwise one of the supplied tags has to match.
+    /// </summary>
+    /// <param name="ifMatch">The raw <c>If-Match</c> header values, as the request carries them.</param>
+    /// <param name="storedDocument">The document the request is about to be applied to.</param>
+    /// <exception cref="ConfigurationException">Thrown when the document is not readable.</exception>
+    internal static bool Allows(StringValues ifMatch, string storedDocument)
+    {
+        if (StringValues.IsNullOrEmpty(ifMatch))
+            return true;
+
+        string[] candidates = ifMatch
+            .Where(value => !string.IsNullOrWhiteSpace(value))
+            .SelectMany(value => value!.Split(
+                ',', StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries))
+            .ToArray();
+
+        // "*" first, and before computing anything: the tag now costs a parse of the stored document,
+        // and a wildcard has already said it does not care what the tag is.
+        if (candidates.Contains("*", StringComparer.Ordinal))
+            return true;
+
+        // A weak tag ("W/…") never satisfies If-Match, which requires strong comparison — it simply
+        // fails to equal the strong tag rather than needing a rule of its own.
+        if (candidates.Length > 0)
+            return candidates.Contains(For(storedDocument), StringComparer.Ordinal);
+
+        // A header present but empty carries no tag to match, so it states no precondition either.
+        return true;
+    }
+}

src/UI/NeoReports.UI/Pages/Builder.razor

@@ -211,6 +211,10 @@
         _configUnavailable = config is not { Outcome: ApiConfigOutcome.Ok, Document: { } document }
             || !BuilderConfigMapper.Hydrate(Wizard, document, RegisteredSourceType);
 
+        // Kept whatever the outcome: Hydrate clears it through Reset(), and a null here simply means
+        // the save states no precondition, which is the pre-D87 behaviour rather than a failure.
+        Wizard.OriginalVersion = config.Version;
+
         if (_configUnavailable)
             return;
 

src/UI/NeoReports.UI/Pages/BuilderReview.razor

@@ -160,9 +160,21 @@
     private async Task<ApiCreateResult> PersistAsync()
     {
         string configJson = BuilderConfigMapper.ToConfigJson(Wizard);
-        return Wizard.IsEditing
-            ? await Api.TryReplaceReportAsync(Wizard.EditingOriginalName, configJson)
-            : await Api.TryCreateReportAsync(configJson);
+        if (!Wizard.IsEditing)
+            return await Api.TryCreateReportAsync(configJson);
+
+        ApiCreateResult result = await Api.TryReplaceReportAsync(
+            Wizard.EditingOriginalName, configJson, Wizard.OriginalVersion);
+
+        if (result.Outcome == ApiCreateOutcome.Created && result.Version is not null)
+            Wizard.OriginalVersion = result.Version;
+
+        // A successful save invalidates the validator this page is holding — it described the
+        // document as it was BEFORE the save. Keeping the old one would refuse the next save from
+        // this same page (a retry after "Run now" failed to start, a double-click) as a conflict
+        // with itself. Only advanced on success; a rejected save changed nothing.
+
+        return result;
     }
 
     private static string DescribeFailure(ApiCreateOutcome outcome) => outcome switch

src/UI/NeoReports.UI/Services/BuilderState.cs

@@ -202,6 +202,13 @@ public string ConnectionSummary
     /// </summary>
     public string? OriginalDocument { get; set; }
 
+    /// <summary>
+    /// The entity-tag the configuration was read under (ADR D87), echoed back as <c>If-Match</c> when
+    /// the edit is saved so the engine can refuse it if another editor changed the report meanwhile.
+    /// <c>null</c> when creating, or when the engine did not supply one.
+    /// </summary>
+    public string? OriginalVersion { get; set; }
+
     /// <summary>
     /// Identifies the source the loaded document described, so the patch can tell "the user changed
     /// the page size" from "the user pointed this report at a different source". Properties from the
@@ -273,6 +280,7 @@ public void Reset()
         IsEditing = false;
         EditingOriginalName = "";
         OriginalDocument = null;
+        OriginalVersion = null;
         LoadedSourceIdentity = "";
         AdditionalDestinationCount = 0;
         AdditionalOutputCount = 0;