feat(api): optimistic concurrency on report editing (ADR D87) - #291
Merged
Conversation
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.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
dotnet format wanted the { Content = content } initializer on its own
line, which reads worse than assigning the property. CI checks out LF, so
this was a real formatting failure and not the CRLF false positive local
runs produce.
CodeQL cs/redundant-tostring-call (alert #309): string concatenation already calls ToString, so the explicit call and its null-forgiving operator were both noise.
The §1b entry arrived with #290 and this branch is what fixes it, but the two were in flight at once, so marking it here would have conflicted. #290 merged and master came back into this branch, so it can be closed where it lives. Records what the security pass changed as well as the fix: the validator is over the REDACTED form, because hashing the stored document made the tag an offline verification oracle for the values the endpoint exists to withhold. And what stays uncovered — two non-overlapping edits still cost one a reload, and the check is check-then-act rather than atomic. That leaves one open backlog item, the PostgreSQL timetz zone loss in §5, which needs a frozen-enum split and a maintainer decision.
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes the gap D86 recorded and left open, with the maintainer's go-ahead on the new API surface.
The window
A redaction placeholder carries the address of the slot its value came from —
${neoreports:redacted:destinations[1]}— which is what makes a single editor's reorder safe. But the address names a slot of the document as it was at theGET, and thePUTresolves it against the document as it is now. Between the two, another editor can reorder the destinations, anddestinations[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.The change
GET /reports/{name}/configETagPUT /reports/{name}If-Match, answering412when it no longer matches; returns the newETagon successIf-Matchis optional. Requiring it would break every client of an endpoint that shipped one release ago; the realistic scenario is two people in the Builder, which always sends it. The comparison runs against the same stored readRestorelater merges against — re-reading would reopen the window inside the handler.What the security pass changed
The first cut hashed the stored document, reasoning that it is what
Restoreresolves 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 on the database to notice.The tag is now over the redacted form, the bytes the caller already holds, so it carries no information at all. It is still the right validator: an address is invalidated by a change to the document's structure, and that structure is fully 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.
A keyed MAC also closes the oracle but its tags differ between instances, so a load-balanced host would answer
412at random.What code review changed
Two client-half defects, both leaving the feature working and the user unable to act on it:
412wasProblemDetailswhile the client readserror— so the user was told the configuration was invalid and never told to reload, and would re-save into the same412forever.OriginalVersionwas 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 guaranteed412naming a conflict with the save that had just succeeded.Not fixed, recorded in the ADR
The check is still check-then-act: a third writer inside the same instant is not caught. Closing 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.Verification
Each fix verified by reverting it and confirming the new test fails. Full suite 1 687 green across 33 projects.
Note:
docs/STATUS-AND-BACKLOG.md§1b is marked open by #290, which is not merged yet — it should be marked fixed once both land, and is deliberately not touched here to avoid a conflict.