From 487818e67a075b226724f622957e63f49cb930ce Mon Sep 17 00:00:00 2001 From: Edwin van den Houdt Date: Thu, 27 Aug 2026 10:52:29 +0200 Subject: [PATCH] fix(privacy): mask the owner BSN on the cross-owner case lists (RB-03) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Mappers.ToAdminSummaryDto set Owner to the raw BSN. Both consumers are cross-owner lists read by someone who is not the subject — GET /admin/cases and GET /werkvoorraad — while GET /beoordeling/{id}, the detail view of the same data, already masked it. The detail screen showed ******782 and the list one click earlier showed the whole thing. Masked in the mapper rather than at each endpoint, so a third cross-owner list cannot be added that forgets to. MaskTail moves out of Program.cs into Domain/People/Pii.cs: it now has callers in Contracts, Program.cs and (once RB-04 lands) Data, and a second hand-rolled copy is how one of them drifts into leaking. Documented as idempotent, which is what lets /beoordeling/{id} keep its own call — IZaakSource has a second implementation whose Owner is mapped from the OpenZaak zaak identificatie, so that endpoint should not depend on which source answered. No frontend change: all three consumers display the value, and the parse boundaries only require a non-empty string. Co-Authored-By: Claude Opus 5 --- .../src/BigRegister.Api/Contracts/Mappers.cs | 6 ++- .../src/BigRegister.Api/Domain/People/Pii.cs | 18 ++++++++ backend/src/BigRegister.Api/Program.cs | 14 +++--- .../BigRegister.Tests/AdminCasesTests.cs | 6 ++- .../BigRegister.Tests/WerkvoorraadTests.cs | 3 +- .../refactor-backlog/implementation/rb-03.md | 45 +++++++++++++++++++ 6 files changed, 81 insertions(+), 11 deletions(-) create mode 100644 backend/src/BigRegister.Api/Domain/People/Pii.cs create mode 100644 docs/project/refactor-backlog-setup/refactor-backlog/implementation/rb-03.md diff --git a/backend/src/BigRegister.Api/Contracts/Mappers.cs b/backend/src/BigRegister.Api/Contracts/Mappers.cs index d4af6fb..ae32125 100644 --- a/backend/src/BigRegister.Api/Contracts/Mappers.cs +++ b/backend/src/BigRegister.Api/Contracts/Mappers.cs @@ -70,8 +70,12 @@ public static class Mappers a.CreatedAt.ToString("o"), a.UpdatedAt.ToString("o"), SubmittedAtOf(a)); /// Admin summary — same shape plus the owner (WP-36; the user-facing list leaves Owner null). + /// The owner is a BSN, and both consumers of this mapper are cross-owner lists read by + /// someone who is not the subject (`/admin/cases`, `/werkvoorraad`), so it goes out masked + /// (RB-03/BIO-003). Masking here rather than at each endpoint means a third cross-owner + /// list cannot be added that forgets to. public static ApplicationSummaryDto ToAdminSummaryDto(this Aanvraag a, DateTimeOffset now) => - a.ToSummaryDto(now) with { Owner = a.Owner }; + a.ToSummaryDto(now) with { Owner = Pii.MaskTail(a.Owner, 3) }; public static ApplicationDetailDto ToDetailDto(this Aanvraag a, DateTimeOffset now) => new( a.Id, a.Type, a.ToStatusDto(now), DraftOf(a), a.DocumentIds, diff --git a/backend/src/BigRegister.Api/Domain/People/Pii.cs b/backend/src/BigRegister.Api/Domain/People/Pii.cs new file mode 100644 index 0000000..c98b620 --- /dev/null +++ b/backend/src/BigRegister.Api/Domain/People/Pii.cs @@ -0,0 +1,18 @@ +namespace BigRegister.Domain.People; + +/// +/// One redaction rule for identifiers that must not leave the server in full (BSN, +/// BIG-nummer). Lives in Domain/ because three layers need it — the DTO mappers +/// (Contracts/Mappers.cs), the audit writes (Data/DocumentStore.cs) and the +/// endpoints themselves — and a second hand-rolled copy is exactly how one of them drifts +/// into leaking. Mirrors the FE maskTail (libs/shared/src/ui/debug-state/mask.ts) +/// so wire redaction and the dev panel agree on what a masked value looks like. +/// +public static class Pii +{ + /// Keep the last characters, mask the rest. Idempotent: masking an + /// already-masked value is a no-op, so a defence-in-depth second call is harmless. + public static string MaskTail(string value, int keep) => + value.Length <= keep ? new string('*', value.Length) + : new string('*', value.Length - keep) + value[^keep..]; +} diff --git a/backend/src/BigRegister.Api/Program.cs b/backend/src/BigRegister.Api/Program.cs index 20e3ce4..bc7e98f 100644 --- a/backend/src/BigRegister.Api/Program.cs +++ b/backend/src/BigRegister.Api/Program.cs @@ -12,6 +12,7 @@ using BigRegister.Domain.Documents; using BigRegister.Domain.Features; using BigRegister.Domain.Intake; using BigRegister.Domain.Letters; +using BigRegister.Domain.People; using BigRegister.Domain.Registrations; using BigRegister.Domain.Submissions; using BigRegister.Api.Zgw; @@ -460,7 +461,10 @@ api.MapGet("/beoordeling/{id}", (string id, HttpContext ctx, IZaakSource zaken) if (c is null || c.Status.Tag == "Concept") return Results.NotFound(); var docs = DocumentStore.ByIds(c.DocumentIds) .Select(d => new BeoordelingDocumentDto(d.DocumentId, d.CategoryId, d.FileName)).ToList(); - var masked = c with { Owner = MaskTail(c.Owner!, 3) }; + // Belt and braces: ToAdminSummaryDto already masks the local source (RB-03) and + // MaskTail is idempotent, but IZaakSource has a second implementation whose Owner + // is mapped from OpenZaak, so this stays as the guarantee for this response. + var masked = c with { Owner = Pii.MaskTail(c.Owner!, 3) }; // WP-68 (F3): non-throwing — c.Status.Tag crosses the IZaakSource wire boundary, so an // unrecognised tag degrades to "cannot decide" instead of a 500. var canBesluiten = Enum.TryParse(c.Status.Tag, out var tag) && BeoordelingRules.CanDecide(tag); @@ -869,12 +873,6 @@ void RecordZgwDivergence(HttpContext ctx, string id, string referentie, Exceptio AuthzAuditStore.Record("zgw:divergence", referentie, allowed: false, Authz.ResolvePrincipal(ctx).Role.ToString(), cid); } -// Keep the last `keep` characters, mask the rest — mirrors the FE maskTail -// (src/app/shared/ui/debug-state/mask.ts) so wire redaction and the dev panel agree. -static string MaskTail(string value, int keep) => - value.Length <= keep ? new string('*', value.Length) - : new string('*', value.Length - keep) + value[^keep..]; - static string Now() => DateTimeOffset.UtcNow.ToString("o"); BriefViewDto ToView(HttpContext ctx, BriefEntity e) => new( @@ -888,7 +886,7 @@ BriefViewDto ToView(HttpContext ctx, BriefEntity e) => new( // behandel scherm can show whom/what it concerns without brief/ importing registratie. // The BIG-nummer ships MASKED by default (PRD-0002 §5c, field-level PII); the reveal // endpoint returns the full value, gated + audited. - new CaseContextDto(SeedData.Registration.Naam, MaskTail(SeedData.Registration.BigNummer, 3), e.Beroep, BriefSeed.AanvraagReferentie)); + new CaseContextDto(SeedData.Registration.Naam, Pii.MaskTail(SeedData.Registration.BigNummer, 3), e.Beroep, BriefSeed.AanvraagReferentie)); // Emit (decision flags, via ToView) and enforce (Forbidden/Conflict below) both run // through Authz — see BriefStore.Review and Authz.CanActOn — so they cannot drift. diff --git a/backend/tests/BigRegister.Tests/AdminCasesTests.cs b/backend/tests/BigRegister.Tests/AdminCasesTests.cs index 9bfc9dc..475778a 100644 --- a/backend/tests/BigRegister.Tests/AdminCasesTests.cs +++ b/backend/tests/BigRegister.Tests/AdminCasesTests.cs @@ -1,6 +1,7 @@ using System.Net; using System.Net.Http.Json; using BigRegister.Api.Contracts; +using BigRegister.Api.Data; using Microsoft.AspNetCore.Mvc.Testing; namespace BigRegister.Tests; @@ -34,7 +35,10 @@ public class AdminCasesTests(TestWebApplicationFactory factory) : IClassFixture< list.EnsureSuccessStatusCode(); var cases = (await list.Content.ReadFromJsonAsync>())!; var mine = cases.Single(x => x.Id == a.Id); - Assert.False(string.IsNullOrEmpty(mine.Owner)); // admin list carries the owner + // RB-03/BIO-003: the owner is carried, but masked — it is a BSN, and this list is + // read by someone who is not the subject. + Assert.Equal("******782", mine.Owner); + Assert.DoesNotContain(DocumentStore.DemoOwner, mine.Owner); } finally { diff --git a/backend/tests/BigRegister.Tests/WerkvoorraadTests.cs b/backend/tests/BigRegister.Tests/WerkvoorraadTests.cs index dc76a16..c665f08 100644 --- a/backend/tests/BigRegister.Tests/WerkvoorraadTests.cs +++ b/backend/tests/BigRegister.Tests/WerkvoorraadTests.cs @@ -38,7 +38,8 @@ public class WerkvoorraadTests(TestWebApplicationFactory factory) : IClassFixtur var queue = (await res.Content.ReadFromJsonAsync>())!; var mine = queue.Single(x => x.Id == a.Id); Assert.Equal("InBehandeling", mine.Status.Tag); - Assert.False(string.IsNullOrEmpty(mine.Owner)); // cross-owner, like /admin/cases + // RB-03/BIO-003: masked, like /admin/cases — both inherit ToAdminSummaryDto. + Assert.Equal("******782", mine.Owner); } finally { diff --git a/docs/project/refactor-backlog-setup/refactor-backlog/implementation/rb-03.md b/docs/project/refactor-backlog-setup/refactor-backlog/implementation/rb-03.md new file mode 100644 index 0000000..3cb43a1 --- /dev/null +++ b/docs/project/refactor-backlog-setup/refactor-backlog/implementation/rb-03.md @@ -0,0 +1,45 @@ +# RB-03 — mask the owner BSN on the cross-owner case lists + +Status: **implemented** · 2026-08-27 · Source findings: `07-bio2-compliance.md` BIO-003 · `99-backlog.md` RB-03 + +## What was wrong + +`Mappers.ToAdminSummaryDto` set `Owner = a.Owner` — the raw BSN. Two endpoints consume it, +both cross-owner lists read by someone who is **not** the subject: + +- `GET /admin/cases` (`cases:manage`) +- `GET /werkvoorraad` (`aanvraag:beoordelen`) + +`GET /beoordeling/{id}` — the *detail* view of the same data — already masked. So the +detail screen showed `******782` while the list one click earlier showed the whole BSN. + +## What changed + +| File | Change | +| --------------------------- | ----------------------------------------------------------------------------------- | +| `Domain/People/Pii.cs` | **new** — `Pii.MaskTail`, moved out of `Program.cs` | +| `Contracts/Mappers.cs` | `Owner = Pii.MaskTail(a.Owner, 3)` | +| `Program.cs` | local `MaskTail` deleted; two call sites point at `Pii.MaskTail` | +| `AdminCasesTests.cs` | asserts the masked value and that `DemoOwner` does not appear | +| `WerkvoorraadTests.cs` | same assertion, replacing the `IsNullOrEmpty` one | + +**Masked in the mapper, not at the endpoints.** The point of the ticket is that both +lists *inherit* it, so a third cross-owner list cannot be added that forgets to mask. + +**`MaskTail` moved to `Domain/People/Pii.cs`** because it now has three callers across +three folders (`Contracts`, `Program.cs`, and `Data` once **RB-04** lands), and a second +hand-rolled copy is how one of them drifts into leaking. It is documented as idempotent, +which is what lets `/beoordeling/{id}` keep its own call: `IZaakSource` has a second +implementation (`OpenZaakZaakSource` → `ZgwZaakMapper`, which maps `Owner` from the zaak +`identificatie`), so that endpoint's guarantee should not depend on which source answered. + +## Blast radius on the frontend — none + +Both consumers use the value for display only (`admin-cases.page.ts:101`, +`beoordeling-view.ts:40`, `werkvoorraad-item-view.ts:28`); the `parse*` boundaries require +a non-empty string, which a masked BSN still is. Nothing keys, filters or looks up by owner. + +## Verification + +`dotnet format --verify-no-changes` clean. `dotnet test`: **251 passed, 1 failed** — the +pre-existing `OpenZaakIntegrationTests.Admin_cases_…`, which needs a live container.