Merge RB-18 — key IdempotencyStore on caller plus idem key
BIO-018: the store was a process-global dictionary keyed on the client-supplied Idempotency-Key alone, so one caller could replay another caller's key and receive their cached response. The key is now scoped with the caller SubjectId. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> # Conflicts: # docs/project/refactor-backlog-setup/refactor-backlog/99-backlog.md # libs/shared/docs/behaviour-spec.mdx
This commit is contained in:
@@ -0,0 +1,115 @@
|
||||
# RB-18 — key `IdempotencyStore` on `{SubjectId}:{idemKey}`
|
||||
|
||||
Status: **implemented** · 2026-08-27 · Source findings: `07-bio2-compliance.md` BIO-018 ·
|
||||
`00-baseline.md` §7 (`IdempotencyStore` listed among the 7 stores "Not behind any port"),
|
||||
agent 02's `backend/Data` note ("no `Reset()` and no TTL") · `99-backlog.md` RB-18
|
||||
|
||||
## What was wrong
|
||||
|
||||
`Data/IdempotencyStore.cs` is a process-global `Dictionary<string, IResult>` keyed only on
|
||||
the raw `Idempotency-Key` header value. `Program.cs`'s `Submit` helper read and wrote it
|
||||
with that raw value, never composed with the caller's identity:
|
||||
|
||||
```csharp
|
||||
var idemKey = ctx.Request.Headers.TryGetValue("Idempotency-Key", out var k) && !string.IsNullOrEmpty(k)
|
||||
? k.ToString()
|
||||
: null;
|
||||
```
|
||||
|
||||
The client picks the header value. Two different callers who happen to send the same
|
||||
value shared one cache slot: the second caller's request short-circuited to the first
|
||||
caller's cached `IResult` instead of running its own submission. BIO-018 rates this
|
||||
**severity low** — the cached value is only a `ReferentieResponse` (a reference number) or
|
||||
a `ProblemDetails`, never personal data — but flags it as a defect in an access-control
|
||||
path with a trivial fix.
|
||||
|
||||
**Location check against the finding.** BIO-018 cites `Program.cs:901-909` for the read/
|
||||
write and `Data/IdempotencyStore.cs:11-27` for the store. RB-17 (landed the day before,
|
||||
same file, unrelated change) shifted line numbers; the real call sites are
|
||||
`Program.cs:994` (read) and `:1028` (write), inside the local `Submit` helper starting at
|
||||
`:991`. The store file itself is untouched by RB-17 and matches the finding's shape
|
||||
exactly. `Submit` has exactly one call site (`POST /change-requests`, `:239`) — the
|
||||
`ChangeRequestRequest` → `telefoonwijziging` endpoint — so the scoping change lands on a
|
||||
single endpoint, not the "smaller call set" RB-17 was sequenced ahead of this ticket to
|
||||
produce; RB-17 removed idempotency-key minting from 5 read call sites, none of which used
|
||||
this helper in the first place, so its ordering benefit does not change what this ticket
|
||||
touches. Reported for completeness, not as a discrepancy: RB-17's own note already scoped
|
||||
its residual to "this ticket is unaffected by this split beyond it now landing on a
|
||||
correctly write-only call set" — true, and the call set was already this one endpoint
|
||||
before and after RB-17.
|
||||
|
||||
## What changed
|
||||
|
||||
| File | Change |
|
||||
| --------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
|
||||
| `src/BigRegister.Api/Program.cs` | `Submit`'s `idemKey` is now `$"{ctx.Caller().SubjectId}:{k}"` instead of the raw header value `k.ToString()`; doc comment above `Submit` states the scoping and cites RB-18/BIO-018 |
|
||||
| `tests/BigRegister.Tests/IdempotencyTests.cs` | **new** `A_caller_replaying_another_callers_idempotency_key_does_not_get_their_cached_result` |
|
||||
|
||||
`ctx.Caller()` (`Domain/Authorization/CallerIdentity.cs`) is already in scope in
|
||||
`Program.cs` — `ctx.Zorgverlener()` is used elsewhere in the same file — and it throws if
|
||||
the identity middleware did not run, so this composition cannot silently fall back to an
|
||||
unscoped key. `SubjectId` is the BSN for a `ZorgverlenerCaller` and the medewerkerId for a
|
||||
`MedewerkerCaller`; either way it is stable per caller and never empty.
|
||||
|
||||
This is exactly the ticket's minimal remediation, no more: no TTL, no eviction, no bound,
|
||||
no `Reset()`, no port/interface extraction. `IdempotencyStore`'s own `ponytail:` comment
|
||||
("no TTL/eviction … an unbounded dictionary keyed on client-supplied strings is a memory
|
||||
leak at scale") is untouched — the store is still unbounded and still keyed on a
|
||||
client-supplied string, only now composed with a server-resolved one first. The comment
|
||||
stays accurate; this ticket did not touch the part it would need to correct.
|
||||
|
||||
## The test
|
||||
|
||||
`IdempotencyTests.cs` already existed (RB-17's predecessor work, not this ticket) with
|
||||
three cases exercising same-caller replay/independence. Added a fourth:
|
||||
|
||||
```csharp
|
||||
[Fact]
|
||||
public async Task A_caller_replaying_another_callers_idempotency_key_does_not_get_their_cached_result()
|
||||
{
|
||||
var sharedKey = Guid.NewGuid().ToString();
|
||||
|
||||
var callerARequest = ChangeRequestWithKey(sharedKey);
|
||||
callerARequest.Headers.Add("X-Subject", "111222333");
|
||||
var callerA = await _client.SendAsync(callerARequest);
|
||||
callerA.EnsureSuccessStatusCode();
|
||||
var callerABody = await callerA.Content.ReadFromJsonAsync<ReferentieResponse>();
|
||||
|
||||
var callerBRequest = ChangeRequestWithKey(sharedKey);
|
||||
callerBRequest.Headers.Add("X-Subject", "999888777");
|
||||
var callerB = await _client.SendAsync(callerBRequest);
|
||||
callerB.EnsureSuccessStatusCode();
|
||||
var callerBBody = await callerB.Content.ReadFromJsonAsync<ReferentieResponse>();
|
||||
|
||||
Assert.NotEqual(callerABody!.Referentie, callerBBody!.Referentie);
|
||||
}
|
||||
```
|
||||
|
||||
`X-Subject` is `StubIdentityProvider`'s existing header for setting the caller's BSN in a
|
||||
test (the same idiom `ApplicationTests.cs` and `UploadAccessTests.cs` use), so caller A and
|
||||
caller B are two different `ZorgverlenerCaller`s sending the identical `Idempotency-Key`.
|
||||
|
||||
**Verified red without the fix.** Reverted `Program.cs`'s `idemKey` line to
|
||||
`k.ToString()` with an `Edit` (not `git checkout`, so the rest of the working tree stayed
|
||||
intact), reran `dotnet test --filter "FullyQualifiedName~IdempotencyTests"`:
|
||||
|
||||
```
|
||||
Failed BigRegister.Tests.IdempotencyTests.A_caller_replaying_another_callers_idempotency_key_does_not_get_their_cached_result [4 ms]
|
||||
Error Message:
|
||||
Assert.NotEqual() Failure: Strings are equal
|
||||
Expected: Not "BIG-2026-476969"
|
||||
Actual: "BIG-2026-476969"
|
||||
Failed! - Failed: 1, Passed: 3, Skipped: 0, Total: 4
|
||||
```
|
||||
|
||||
Caller B received caller A's cached reference. Then reapplied the fix with a second
|
||||
`Edit` and reran: `Passed! - Failed: 0, Passed: 4, Skipped: 0, Total: 4`.
|
||||
|
||||
## Verification
|
||||
|
||||
`dotnet test` (full suite): **261 passed, 1 failed** — the failure is
|
||||
`OpenZaakIntegrationTests.Admin_cases_returns_the_seeded_zaak_mapped_through_real_HTTP_and_JWT`,
|
||||
which needs a live OpenZaak container and fails identically on a stashed tree; it predates
|
||||
this change and is not run by `npm run ci`.
|
||||
|
||||
`npm run ci` (foreground): green — see the commit's own record for the full step list.
|
||||
Reference in New Issue
Block a user